From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.ispras.ru (mail.ispras.ru [83.149.199.84]) by mx.groups.io with SMTP id smtpd.web09.4973.1608111072084433054 for ; Wed, 16 Dec 2020 01:31:13 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: ispras.ru, ip: 83.149.199.84, mailfrom: cheptsov@ispras.ru) Received: from [192.168.1.2] (unknown [77.232.9.83]) by mail.ispras.ru (Postfix) with ESMTPSA id 27C0840D403E; Wed, 16 Dec 2020 09:31:08 +0000 (UTC) From: "Vitaly Cheptsov" Message-Id: <4246207D-8EA5-4A6C-8580-72ED9D1F4ADF@ispras.ru> Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.40.0.2.32\)) Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode Date: Wed, 16 Dec 2020 12:31:07 +0300 In-Reply-To: Cc: "devel@edk2.groups.io" , "Wu, Hao A" , "Ni, Ray" , "Wang, Jian J" , "Albecki, Mateusz" , Laszlo Ersek To: "Kinney, Michael D" References: <2653EB72-8FFE-47E1-8D1F-A6CD1D95044B@ispras.ru> X-Mailer: Apple Mail (2.3654.40.0.2.32) X-Groupsio-MsgNum: 68947 Content-Type: multipart/signed; boundary="Apple-Mail=_62211FAC-4AFF-41F5-AEC5-C463D79492D7"; protocol="application/pgp-signature"; micalg=pgp-sha512 --Apple-Mail=_62211FAC-4AFF-41F5-AEC5-C463D79492D7 Content-Type: multipart/alternative; boundary="Apple-Mail=_B537F40B-D556-4F38-8A4C-4BCBB0BB353C" --Apple-Mail=_B537F40B-D556-4F38-8A4C-4BCBB0BB353C Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Mike, That=E2=80=99s right. And due to that interface being vendor-specific, Int= el controllers work fine. I am not positive more logic is needed since it i= s opt-in. These patches are not new, and so far they worked reliably on a b= road amount of systems for several years. >>From what it looks like, you are strongly opposed to getting this land int= o EDK II mainline, since it is too specific (at least that is how I underst= and your arguments). If this is the case, I guess we could abandon these ch= anges. Best regards, Vitaly > 15 =D0=B4=D0=B5=D0=BA. 2020 =D0=B3., =D0=B2 23:01, Kinney, Michael D =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0= ): >=20 > Another issue with this approach is that the formal PCI definition of th= is class code is in the following spec. >=20 > https://pcisig.com/sites/default/files/files/PCI_Code-ID_r_1_11__v24_Jan= _2019.pdf >=20 > 04h 00h RAID controller - vendor-specific interface >=20 >=20 > #define PCI_CLASS_MASS_STORAGE_IDE 0x01 >=20 > #define PCI_CLASS_MASS_STORAGE_SATADPA 0x06 > #define PCI_IF_MASS_STORAGE_SATA 0x00 > #define PCI_IF_MASS_STORAGE_AHCI 0x01 >=20 > #define PCI_CLASS_MASS_STORAGE_RAID 0x04 >=20 >=20 > #define IS_PCI_IDE(_p) IS_CLASS2 (_p, PCI_CLASS_MASS_STOR= AGE, PCI_CLASS_MASS_STORAGE_IDE) > #define IS_PCI_SATADPA(_p) IS_CLASS2 (_p, PCI_CLASS_MASS_STORAGE, PCI_CL= ASS_MASS_STORAGE_SATADPA) > #define IS_PCI_RAID(_p) IS_CLASS2 (_p, PCI_CLASS_MASS_STOR= AGE, PCI_CLASS_MASS_STORAGE_RAID) >=20 > So the IS_PCI_RAID() macro checks for the RAID class code and the PCI sp= ec states that the interface is vendor specific. There is no guarantee wha= t so ever that the controller that passes IS_PCI_RAID() check has a SATA in= terface. >=20 > There are lost of risks in using this macro to see if it is a SATA contr= oller (even if enabled by a PCD). You need to add more logic to even know = it is safe to assume SATA registers. >=20 > Mike >=20 >=20 >=20 >=20 >=20 > From: devel@edk2.groups.io > On Behalf Of Vitaly Cheptsov > Sent: Tuesday, December 15, 2020 11:47 AM > To: Kinney, Michael D > > Cc: devel@edk2.groups.io ; Wu, Hao A >; Ni, Ray >; Wang, Jian J >; Albecki, Mateusz >; Laszlo Ersek > > Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Ad= d support for drives in RAID mode >=20 > Mike, >=20 > I understand that very well and thus the PCD rather than my original pat= ch :) >=20 > Best, > Vitaly >=20 >=20 > On 15 Dec 2020, at 22:41, Kinney, Michael D > wrote: >=20 > =EF=BB=BF > Vitaly, >=20 > I am concerned about platforms that use this driver with this change out= side your use case. >=20 > Mike >=20 > From: devel@edk2.groups.io > On Behalf Of Vitaly Cheptsov > Sent: Tuesday, December 15, 2020 11:40 AM > To: Kinney, Michael D > > Cc: devel@edk2.groups.io ; Wu, Hao A >; Ni, Ray >; Wang, Jian J >; Albecki, Mateusz >; Laszlo Ersek > > Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Ad= d support for drives in RAID mode >=20 > As long as we do not write to a RAID array it will not cause any issues,= and we do not. So I do not see an issue here. >=20 > Vitaly >=20 >=20 > On 15 Dec 2020, at 22:00, Kinney, Michael D > wrote: >=20 > =EF=BB=BF > What about platforms that are in RAID mode and have configured a RAID se= t. Your suggested change could potentially corrupt data on those different= systems. >=20 > Mike >=20 > From: Vitaly Cheptsov > > Sent: Tuesday, December 15, 2020 10:56 AM > To: Kinney, Michael D > > Cc: devel@edk2.groups.io ; Wu, Hao A >; Ni, Ray >; Wang, Jian J >; Albecki, Mateusz >; Laszlo Ersek > > Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Ad= d support for drives in RAID mode >=20 > Not correct, these systems do not have hard RAID support or anything ali= ke. It is standard G45 from what I remember. I believe the vendor simply le= ft the firmware supplier defaults or something alike as there is a way to u= se IDE mode but nothing for AHCI. >=20 > Vitaly >=20 >=20 > On 15 Dec 2020, at 21:09, Kinney, Michael D > wrote: >=20 > =EF=BB=BF > So those types of systems must have a RAID enabled FW driver. Right? S= o the drives could be configured as a RAID set and using the patch you sugg= est below could corrupt data. >=20 > It is difficult to support a change that could corrupt data. >=20 > Mike >=20 > From: Vitaly Cheptsov > > Sent: Tuesday, December 15, 2020 9:44 AM > To: Kinney, Michael D > > Cc: devel@edk2.groups.io ; Wu, Hao A >; Ni, Ray >; Wang, Jian J >; Albecki, Mateusz >; Laszlo Ersek > > Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Ad= d support for drives in RAID mode >=20 > Unfortunately not. That is basically the issue. When there is a preferen= ce, it is possible to ask the user to set it. However, for certain Dell mac= hines, we have an issue with, it is not possible to select AHCI mode in the= firmware preferences, and these users end up with unconfigurable RAID. >=20 > Best regards, > Vitaly >=20 >=20 > 15 =D0=B4=D0=B5=D0=BA. 2020 =D0=B3., =D0=B2 20:41, Kinney, Michael D > =D0=BD=D0=B0= =D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0): >=20 > But do the systems allow the user to configure the FW that runs earlier?= Can you require to users to configure their platforms correctly? >=20 > Thanks, >=20 > Mike >=20 > From: devel@edk2.groups.io > On Behalf Of Vitaly Cheptsov > Sent: Tuesday, December 15, 2020 9:34 AM > To: Kinney, Michael D > > Cc: devel@edk2.groups.io ; Wu, Hao A >; Ni, Ray >; Wang, Jian J >; Albecki, Mateusz >; Laszlo Ersek > > Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Ad= d support for drives in RAID mode >=20 > Hi Michael, >=20 > I believe Intel SATA controllers have non-standard lockdown bits, which = do not let you reconfigure them as soon as the initialisation is over. Sinc= e we start much later (outside of the firmware), we can no longer control t= his. >=20 > Best regards, > Vitaly >=20 >=20 >=20 > 15 =D0=B4=D0=B5=D0=BA. 2020 =D0=B3., =D0=B2 19:58, Kinney, Michael D > =D0=BD=D0=B0= =D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0): >=20 > Hi Vitaly, >=20 > Can you please explain why the controller can not be configured in a non= -RAID mode? >=20 > Thanks, >=20 > Mike >=20 > From: devel@edk2.groups.io > On Behalf Of Vitaly Cheptsov > Sent: Tuesday, December 15, 2020 12:58 AM > To: Wu, Hao A >; devel@ed= k2.groups.io > Cc: Ni, Ray >; Wang, Jian J <= jian.j.wang@intel.com >; Albecki, Mateusz >; Laszlo Ersek <= lersek@redhat.com >; Kinney, Michael D > > Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Ad= d support for drives in RAID mode >=20 > Hello, >=20 > 1. Could you help to change the BZ tracker https://bugzilla.tianocore.or= g/show_bug.cgi?id=3D3118 to a "Tiano Feature Requests"? > For me, it looks more like a feature request. >=20 > Sure, done. >=20 > 2. Could you help to include 'AtaAtapiPassThru' in the BZ tracker subjec= t for better reference? >=20 > Also done. >=20 > 3. For Patch 2/2, is there any reason to clear the bits: > EFI_ATA_PASS_THRU_ATTRIBUTES_PHYSICAL > EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_PHYSICAL > If the drives are intended to be used as non-RAID devices, I think both = the ATTRIBUTES_PHYSICAL & ATTRIBUTES_LOGICAL should be set for the controll= er according to the UEFI Spec. >=20 > I am not quite positive why this was needed (the patch was prepared a fe= w months ago), but I will make a comment in V2 when we test it on real hard= ware. I think it was required to take the right path in the driver. >=20 > DuetPkg was removed from edk2. > If the change is specially for DUET use case, I am afraid we cannot acce= pt this change. >=20 > This is not the DuetPkg from EDK II, but ours[1]. Thus your claim does n= ot apply. >=20 > I agree it would be better to configure the platform so the SATA control= ler is in its non-RAID mode. >=20 > Agree, but in this case it is not feasible. >=20 > If the controller is in RAID mode, then the OS that is booted may have a= SATA RAID driver > that can configure the drives in RAID mode. Then, if the UEFI FW treats= it as non RAID, it > may not be bootable, and configuration actions in UEFI may corrupt data = on the RAID configured > drives. For this reason, I am not in favor of adding a PCD. >=20 > Actually some operating systems have to introduce workarounds for this a= s well, and no, in this particular case the OS does not treat the drive as = RAID either. >=20 > If there are no other review comments besides the attributes, I will pro= ceed with V2 in the coming days. >=20 > Best regards, > Vitaly >=20 > [1] https://github.com/acidanthera/OpenCorePkg >=20 >=20 >=20 >=20 >=20 > 15 =D0=B4=D0=B5=D0=BA. 2020 =D0=B3., =D0=B2 06:54, Kinney, Michael D > =D0=BD=D0=B0= =D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0): >=20 > I agree it would be better to configure the platform so the SATA control= ler is in its non-RAID mode. >=20 > If the controller is in RAID mode, then the OS that is booted may have a= SATA RAID driver > that can configure the drives in RAID mode. Then, if the UEFI FW treats= it as non RAID, it > may not be bootable, and configuration actions in UEFI may corrupt data = on the RAID configured > drives. For this reason, I am not in favor of adding a PCD. >=20 > Mike >=20 >=20 >=20 >=20 > -----Original Message----- > From: devel@edk2.groups.io > On Behalf Of Wu, Hao A > Sent: Monday, December 14, 2020 5:53 PM > To: Ni, Ray >; devel@edk2.gro= ups.io ; cheptsov@ispras.ru > Cc: Wang, Jian J >;= Albecki, Mateusz >; Laszlo Ersek > > Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Ad= d support for drives in RAID mode >=20 >=20 >=20 >=20 > -----Original Message----- > From: Ni, Ray > > Sent: Tuesday, December 15, 2020 9:45 AM > To: devel@edk2.groups.io ; cheptsov@ispras.= ru > Cc: Wang, Jian J >;= Wu, Hao A >; > Albecki, Mateusz >; Laszlo Ersek > > > Subject: RE: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: > Add support for drives in RAID mode >=20 > DuetPkg was removed from edk2. > If the change is specially for DUET use case, I am afraid we cannot acce= pt this > change. >=20 > Hao, > Can this change benefit a general use case? >=20 >=20 > Hello Ray, >=20 > My understanding to the proposed PCD is that drives behind a RAID mode S= ATA controller can be configured to working in > non-RAID mode (acting like individual drives). >=20 > As for the DuetPkg, below is a previous comment from Vitaly: > "there is no firmware preference for that (Hao: configure the controller= to non-RAID mode). The firmware does not support > UEFI, and we are running through DuetPkg." >=20 > Best Regards, > Hao Wu >=20 >=20 >=20 >=20 >=20 >=20 > Thanks, > Ray >=20 >=20 >=20 >=20 > -----Original Message----- > From: devel@edk2.groups.io > On Behalf Of Vitaly > Cheptsov > Sent: Friday, December 11, 2020 5:25 PM > To: devel@edk2.groups.io > Cc: Vitaly Cheptsov >; Wa= ng, Jian J > >; Wu, Hao A >; Albecki, > Mateusz >; = Laszlo Ersek > > Subject: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: > Add >=20 >=20 >=20 > support for drives in RAID mode >=20 > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3118 >=20 > This resolves the problem of using drivers connected to Intel G33 > builtin SATA controller when run from DuetPkg when it can only be > configured in RAID mode through the firmware settings. >=20 > Cc: Jian J Wang > > Cc: Hao A Wu > > Cc: Mateusz Albecki > > Cc: Laszlo Ersek > > Signed-off-by: Vitaly Cheptsov > > --- > MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) >=20 > diff --git a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c > b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c > index ab06e2833c..301335c967 100644 > --- a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c > +++ b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c > @@ -324,7 +324,7 @@ SataControllerSupported ( > return EFI_UNSUPPORTED; > } >=20 > - if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData)) { > + if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData) || > + IS_PCI_RAID (&PciData)) { > return EFI_SUCCESS; > } >=20 > @@ -465,7 +465,7 @@ SataControllerStart ( > if (IS_PCI_IDE (&PciData)) { > Private->IdeInit.ChannelCount =3D IDE_MAX_CHANNEL; > Private->DeviceCount =3D IDE_MAX_DEVICES; > - } else if (IS_PCI_SATADPA (&PciData)) { > + } else if (IS_PCI_SATADPA (&PciData) || IS_PCI_RAID (&PciData)) { > // > // Read Ports Implemented(PI) to calculate max port number (0 based)= . > // > -- > 2.24.3 (Apple Git-128) >=20 >=20 >=20 > -=3D-=3D-=3D-=3D-=3D-=3D > Groups.io Links: You receive all messages sent to th= is group. > View/Reply Online (#68707): > https://edk2.groups.io/g/devel/message/68707 > Mute This Topic: https://groups.io/mt/78875596/1712937 > Group Owner: devel+owner@edk2.groups.io > Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com ] > -=3D-=3D-=3D-=3D-=3D-=3D >=20 >=20 >=20 >=20 >=20 >=20 >=20 >=20 >=20 >=20 >=20 >=20 >=20 --Apple-Mail=_B537F40B-D556-4F38-8A4C-4BCBB0BB353C Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 Mike,

That=E2=80=99s right. And due to that inte= rface being vendor-specific, Intel controllers work fine. I am not positive= more logic is needed since it is opt-in. These patches are not new, and so= far they worked reliably on a broad amount of systems for several years.

From what it looks= like, you are strongly opposed to getting this land into EDK II mainline, = since it is too specific (at least that is how I understand your arguments)= . If this is the case, I guess we could abandon these changes.

Best regards,
Vitaly

15 =D0=B4=D0=B5=D0=BA. 2020 =D0=B3., =D0=B2 23:01= , Kinney, Michael D <michael.d.kinney@intel.com> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0= = =B0=D0=BB(=D0=B0):

Another issue with this approach is that the formal PCI definitio= n of this class code is in the following spec.=
 
https= ://pcisig.com/sites/default/files/files/PCI_Code-ID_r_1_11__v24_Jan_2019.pd= f
 
04h 00h RAID controller - vendor-specific interface=
 
 
#define   PCI_CLASS_MASS_STOR= AGE_IDE    0x01
 <= /div>
#define<= span class=3D"Apple-converted-space"> PCI_CLASS_MASS_STORAGE_SA= TADPA  &nb= sp;0x06
#define &n= bsp; PCI_IF_MASS_S= TORAGE_SATA        = ; 0x00
#= define   PCI_IF_MASS_STORAGE_AHCI&n= bsp;        0x01
 
#define   <= /span>PCI_CLASS_MASS_STORAGE_RAID   0x04
 
 
#define = ;IS_PCI_IDE(_p)      =           IS_CLASS2 (_p, PCI_CLASS_MASS_STORAGE,= PCI_CLASS_MASS_STORAGE_IDE)
#define IS_PCI_SATADPA(_p) IS_CLASS2 (_p, PCI_C= LASS_MASS_STORAGE, PCI_CLASS_MASS_STORAGE_SATADPA)
#define IS_PCI_RAID(_p)          =      = IS_CLASS2 (_p, PCI_CLASS_MASS_STORAGE, PCI_CLASS_MASS_STORAGE_RAID)<= o:p class=3D"">
 
So the IS_PCI_RAID() macro checks for the RAID class code= and the PCI spec states that the interface is vendor specific.  Ther= e is no guarantee what so ever that the controller that passes IS_PCI_RAID(= ) check has a SATA interface.
 
There are lost of risks in u= sing this macro to see if it is a SATA controller (even if enabled by a PCD= ).  You need to add more logic to even know it is safe to assume SATA r= egisters.
 
Mike
 
 =
&= nbsp;
 
 
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Vitaly Cheptsov
Sent: Tuesday, December 15, 2020 11:47 AM
To: Kinney, Michael D <michael.d.kinney@intel.com>
Cc: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Albecki, Mateusz <mateusz.albecki@intel.com>; Laszlo Ersek <= ;lersek@redhat.com>
Subject: Re: [= edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for dri= ves in RAID mode
 
Mike,
 
I understand that very well and thus the PCD r= ather than my original patch :)
&= nbsp;
<= span class=3D"">Best,
Vitaly


On 15 Dec 2020, at 22:41, Kinney, = Michael D <michael.d.kinney@intel.com= > wrote:

=EF=BB=BF 
Vitaly,<= /div>
 
I am concer= ned about platforms that use this driver with this change outside your use = case.
 
Mike
 
From: devel@edk2.groups.io <devel@edk2.g= roups.io> On Behalf Of V= italy Cheptsov
Sent: Tuesday, December 15, 2020 11:40 AM
To: = Kinney, Michael D <michael.d.kinne= y@intel.com>
Cc: devel@edk2.grou= ps.io; Wu, Hao A <hao.a.wu@intel.com&= gt;; Ni, Ray <ray.ni@intel.com>; Wang, J= ian J <jian.j.wang@intel.com>; Albe= cki, Mateusz <mateusz.albecki@intel.co= m>; Laszlo Ersek <lersek@redhat.com= >
Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControll= erDxe: Add support for drives in RAID mode
 
As long as we= do not write to a RAID array it will not cause any issues, and we do not. = So I do not see an issue here.
 
Vitaly


On 15 Dec 2020, at 22:00, Kinney, Michael D <michael.d.kinney@intel.com> wrote:

= =EF=BB=BF 
What about platforms that are in RAID mode and have configured a RAI= D set.  Your suggested change could potentially corrupt data on those d= ifferent systems.
 
Mike
=
 <= /o:p>
=
From: = Vitaly Cheptsov <cheptsov@ispras.ru> 
Sent: Tuesday, Dec= ember 15, 2020 10:56 AM
To: Kinney, Michael D <michael.d.kinney@intel.com>
Cc: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Wang, Jian J <j= ian.j.wang@intel.com>; Albecki, Mateusz <mateusz.albecki@intel.com>; Laszlo Ersek <lersek@redhat.com>
Subject:=  Re: [edk2-devel] [PA= TCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mod= e
 
Not correct, these systems do not have hard RAID su= pport or anything alike. It is standard G45 from what I remember. I believe= the vendor simply left the firmware supplier defaults or something alike a= s there is a way to use IDE mode but nothing for AHCI.
 
Vitaly=

On 15 Dec 2020, at 21:09, Kinney, Michael D = <michael.d.kinney@intel.com> w= rote:

=EF=BB=BF 
So those types of systems must have a RAID en= abled FW driver.  Right?  So the drives could be configured as a RA= ID set and using the patch you suggest below could corrupt data.
 =
It is difficult to support a change that could corrupt data.<= o:p class=3D"">
&nb= sp;
Mike
=  
From: Vitaly Cheptsov <cheptsov@ispras.ru> 
Sent: Tuesday, December 15, 2020 9:44 AM
To: = ;Kinney, Michael D <michael.d.= kinney@intel.com>
Cc: devel@edk2= .groups.io; Wu, Hao A <hao.a.wu@intel.= com>; Ni, Ray <ray.ni@intel.com>;= Wang, Jian J <jian.j.wang@intel.com&g= t;; Albecki, Mateusz <mateusz.albecki= @intel.com>; Laszlo Ersek <lersek@redha= t.com>
Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/Sat= aControllerDxe: Add support for drives in RAID mode<= /o:p>
 
Unfortunately not. T= hat is basically the issue. When there is a preference, it is possible to a= sk the user to set it. However, for certain Dell machines, we have an issue= with, it is not possible to select AHCI mode in the firmware preferences, = and these users end up with unconfigurable RAID.
 =
Best regards,
=
Vitaly


15 =D0= =B4=D0=B5=D0=BA. 2020 =D0=B3., =D0=B2 20:41, Kinney, Michael D <michael.d.kinney@intel.com> =D0=BD=D0= =B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0):
 
But do the systems allow the user to configure the = FW that runs earlier?  Can you require to users to configure their platforms correctly?<= o:p class=3D"">
 
Thanks,
 
<= div style=3D"margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibr= i, sans-serif;" class=3D"">Mike
=  
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Vitaly Cheptsov
Sent: Tuesday, Dec= ember 15, 2020 9:34 AM
To: Kinney, Michael D <michael.d.kinney@= intel.com>
Cc: devel@edk2.groups.io; Wu, Hao A = <ha= o.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Wang,= Jian J <jian.j.wang@intel.com>; Albecki, Mateusz <mateu= sz.albecki@intel.com>; Laszlo Ersek <lersek@redhat.com>
Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataContro= llerDxe: Add support for drives in RAID mode
 
Hi Michael,=
 
= I believe Intel SATA controllers have non-standard lockdown bits, which do = not let you reconfigure them as soon as the initialisation is over. Since w= e start much later (outside of the firmware), we can no longer control this= .
 
Best regards,
Vitaly


15 =D0=B4=D0=B5=D0=BA. 2020 =D0=B3., =D0=B2 19:58, Kinney, Michael= D <michael.d.kinney@intel.com> =D0=BD=D0=B0=D0=BF=D0=B8= = =D1=81=D0=B0=D0=BB(=D0=B0):
=
 <= o:p class=3D"">
Hi Vitaly,
 
<= span class=3D"">Can you please explain why the controller can not be config= ured in a non-RAID mode?
&n= bsp;
Thanks,
 
<= span class=3D"">Mike
 =
Fro= m: <= /span>devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Vitaly Cheptsov
Sent: <= /span>Tuesday, December 15, 2020 12:58 AM
To: Wu, Hao A <hao.a.wu@int= el.com>; devel@= edk2.groups.io
Cc: Ni, Ray <<= span style=3D"color: purple;" class=3D"">ray.ni@intel.com>; W= ang, Jian J <jian.j.wang@intel.com>; Albecki, Mateusz <ma= teusz.albecki@intel.com>; Laszlo Ersek <lersek@redhat.com<= /a>>; Kinney, Michael D <michael.d.kinney@intel.com><= br class=3D"">Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe= : Add support for drives in RAID mode
 
Hello,
 
= 1. Could you help to change the BZ tracker https= ://bugzilla.tianocore.org/show_bug.cgi?id=3D3118 to a "Tian= o Feature Requests"?
For me, it looks more like a feature req= uest.
 
Sure,= done.
=
 
2= . Could you help to include 'AtaAtapiPassThru' in the BZ tracker subject fo= r better reference?
 
Also done.
<= span class=3D""> 
=
3. For Patch 2/2, is there any reason to clear the bits:
EFI_ATA_PASS_THRU_ATTRIBUTES_PHYSICAL
EFI_EXT_SCSI_P= ASS_THRU_ATTRIBUTES_PHYSICAL
If the drives are intended to be= used as non-RAID devices, I think both the ATTRIBUTES_PHYSICAL & ATTRI= BUTES_LOGICAL should be set for the controller according to the UEFI Spec.<= /span>
=
 
I am not qui= te positive why this was needed (the patch was prepared a few months ago), = but I will make a comment in V2 when we test it on real hardware. I think i= t was required to take the right path in the driver.=
 
=
DuetPkg was removed from edk2.=
If the change is specially for DUET use case, I am afraid we= cannot accept this change.
<= /blockquote>
 
<= span class=3D"">This is not the DuetPkg from EDK II, but ours[1]. Thus your= claim does not apply.
=
 
I agree it would be better to configure the platform so the = SATA controller is in its non-RAID mode.
=
 
<= /div>
Agree, but in this case it is not feasible.
 <= /span>
If the = controller is in RAID mode, then the OS that is booted may have a SATA RAID= driver
that can configure the drives in RAID mode.  The= n, if the UEFI FW treats it as non RAID, it
may not be bootab= le, and configuration actions in UEFI may corrupt data on the RAID configur= ed
drives.  For this reason, I am not in favor of adding= a PCD.
 =
Actu= ally some operating systems have to introduce workarounds for this as well,= and no, in this particular case the OS does not treat the drive as RAID ei= ther.
<= div class=3D"">
 
If there are no other review comments besides the attributes, I will pro= ceed with V2 in the coming days.
 
=
Best regards,
Vitaly<= /div>
 
[1] https://gith= ub.com/acidanthera/OpenCorePkg
 




=
15 =D0=B4=D0=B5=D0=BA. 2020 =D0=B3., =D0=B2 06:54,= Kinney, Michael D <michael.d.kinney@intel.com> =D0=BD= = =D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0):=
 
I agree it would be better to configu= re the platform so the SATA controller is in its non-RAID mode.

If the controller is in RAID mode, then the OS that is bo= oted may have a SATA RAID driver
that can configure the drive= s in RAID mode.  Then, if the UEFI FW treats it as non RAID, it
may not be bootable, and configuration actions in UEFI may corrupt = data on the RAID configured
drives.  For this reason, I = am not in favor of adding a PCD.

Mike




<= o:p class=3D"">
-----Original Message-----
From: devel@edk2.groups.io<= /span> <devel@edk2.gro= ups.io> On Behalf Of Wu, Hao A
Sent: Monday, De= cember 14, 2020 5:53 PM
To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; cheptsov@ispras.ru
Cc: Wang, Jian J <= jian.j.wang@intel.com&= gt;; Albecki, Mateusz <mateusz.albecki@intel.com>; Laszlo= Ersek <lersek@redhat.com>
Subject: Re: [edk2-de= vel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in = RAID mode




-----Original Messa= ge-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Tuesday, December 15, 2020 9:45 AM
To: devel@edk2.groups.io; cheptsov@ispras.ru
Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A = <ha= o.a.wu@intel.com>;
Albecki, Mateusz <mateu= sz.albecki@intel.com>; Laszlo Ersek
<lersek@redhat= .com>
Subject: RE: [edk2-devel] [PATCH 1/2] Mde= ModulePkg/SataControllerDxe:
Add support for drives in RAID m= ode

DuetPkg was removed from edk2.
If the change is specially for DUET use case, I am afraid we cannot acce= pt this
change.

Hao,
Can this change benefit a general use case?=


Hello Ray= ,

My understanding to the proposed PCD is that= drives behind a RAID mode SATA controller can be configured to working in<= br class=3D"">non-RAID mode (acting like individual drives).
=
As for the DuetPkg, below is a previous comment from Vitaly:=
"there is no firmware preference for that (Hao: configure th= e controller to non-RAID mode). The firmware does not support
UEFI, and we are running through DuetPkg."

Be= st Regards,
Hao Wu





<= span class=3D"">
Thanks,
Ray




-----Original Message-----
From:<= span class=3D"apple-converted-space"> devel@edk2.groups.io <devel@edk2.groups.i= o> On Behalf Of Vitaly
Cheptsov
S= ent: Friday, December 11, 2020 5:25 PM
To: devel@edk2.groups.io
C= c: Vitaly Cheptsov <cheptsov@ispras.ru>; Wang, Jian J
<jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Albecki,
Mateusz <mateusz.albecki@intel.com= >; Laszlo Ersek <lersek@redhat.com>
S= ubject: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe:
Add



=

support for drives in RAID mode

REF: https://bugzilla.tianocore.org/show_bug.cgi?id= =3D3118

This resolves the problem = of using drivers connected to Intel G33
builtin SATA controll= er when run from DuetPkg when it can only be
configured in RA= ID mode through the firmware settings.

Cc: Jia= n J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu &= lt;hao= .a.wu@intel.com>
Cc: Mateusz Albecki <mateu= sz.albecki@intel.com>
Cc: Laszlo Ersek <lersek@redh= at.com>
Signed-off-by: Vitaly Cheptsov <cheptsov@i= spras.ru>
---
MdeModulePkg/Bus/Pc= i/SataControllerDxe/SataController.c | 4 ++--
1 file changed,= 2 insertions(+), 2 deletions(-)

diff --git a/= MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c
b/Mde= ModulePkg/Bus/Pci/SataControllerDxe/SataController.c
index ab= 06e2833c..301335c967 100644
--- a/MdeModulePkg/Bus/Pci/SataCo= ntrollerDxe/SataController.c
+++ b/MdeModulePkg/Bus/Pci/SataC= ontrollerDxe/SataController.c
@@ -324,7 +324,7 @@ SataControl= lerSupported (
    return EFI_UNSUPPORTED= ;
  }

-  if (IS_P= CI_IDE (&PciData) || IS_PCI_SATADPA (&PciData)) {
+ &= nbsp;if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData) ||
+ IS_PCI_RAID (&PciData)) {
   &= nbsp;return EFI_SUCCESS;
  }

@@ -465,7 +465,7 @@ SataControllerStart (
  = if (IS_PCI_IDE (&PciData)) {
    Priv= ate->IdeInit.ChannelCount =3D IDE_MAX_CHANNEL;
  = ;  Private->DeviceCount       &n= bsp;  =3D IDE_MAX_DEVICES;
-  } else if (IS_PC= I_SATADPA (&PciData)) {
+  } else if (IS_PCI_SATADPA= (&PciData) || IS_PCI_RAID (&PciData)) {
  =   //
    // Read Ports Implemen= ted(PI) to calculate max port number (0 based).
  &= nbsp; //
--
2.24.3 (Apple Git-128)



-=3D-=3D-=3D-=3D-=3D-= =3D
Groups.io Links: You receive all messages sent to this group.
View/R= eply Online (#68707):
https://edk2.groups.io/g/d= evel/message/68707
Mute This Topic: = https://groups.io/mt/78875596/171= 2937
Group Owner: devel+owner@edk2.groups.io
Uns= ubscribe: https://ed= k2.groups.io/g/devel/unsub=  [ray.ni@intel.com]
-=3D-=3D-=3D-=3D-=3D-=3D<= o:p class=3D"">







 
&= nbsp;
 =
 

--Apple-Mail=_B537F40B-D556-4F38-8A4C-4BCBB0BB353C-- --Apple-Mail=_62211FAC-4AFF-41F5-AEC5-C463D79492D7 Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename=signature.asc Content-Type: application/pgp-signature; name=signature.asc Content-Description: Message signed with OpenPGP -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEsLABAI5Y5VbvBdmpL8K2O86Eyz4FAl/Z09sACgkQL8K2O86E yz6HwxAAoSRmf7QprX7Eji190jbi/XG00X2GrgESDXe6PmTX9IMM8y7ZzoeAuURC d8ANQwGh6SV7jQphwCxS974GkNFuNv5ro5zsjIzxuMyHDT0X0UnKzF6ryQX76T56 43dURhOWdrYTixxEZRuinTqNOjwP2LMNNhiLNP5Pmz7ulmRmiAODEG0DD/sHYE84 zlYZTJBJYdfuDcbFLTRLKBUZnZHtLm+Fkm61TK8bX+lH0+dcfiicSZUxl5f2eslJ YRG0shLzPm/AoUkWmJHqvg56gtpPVQb7VgHxeWZ5S3DzTfSz59KmoqnWX23JAJFn D1NHRAxUDcHD503boQQa7UJWKsi6yIYbqrVoyfYHxKqPXpiMSqypuLhGnE0YS0BJ nxcRxCI64rpXkaGWk85TFZSxbmsxfaeMl+9HW5lTlobaM8lYxjEwFviyMKqT9M7K Foy5svXMlIYA4J7S01naMflI58x80ybim3QQDYGQpVN7alvrGvkOK7RBcnVq8AhW VznpHprYNfuMBb5kemVA5mHRpmsg9js6M5l+Q9Y2C8QKJecNf7Fal761ePj4XxcX +6UwNkNg1auRQ3sjkuX6GKuWEf0k1ThwNkfjLLMPJHeQspLDYFXv5fZ+fvbdLKBO R57xnfDd4BQooP034UQJJ2eyLL7Li+T8eL3Pm9osTFJH1IOBh7E= =18Pm -----END PGP SIGNATURE----- --Apple-Mail=_62211FAC-4AFF-41F5-AEC5-C463D79492D7--