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.947.1608053653134257288 for ; Tue, 15 Dec 2020 09:34:14 -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 6F14640A1DB3; Tue, 15 Dec 2020 17:34:09 +0000 (UTC) From: "Vitaly Cheptsov" Message-Id: 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: Tue, 15 Dec 2020 20:34:08 +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: <20201211092502.21763-1-cheptsov@ispras.ru> <64FCA78C-6E93-4126-AF7F-D586865C408B@ispras.ru> X-Mailer: Apple Mail (2.3654.40.0.2.32) X-Groupsio-MsgNum: 68866 Content-Type: multipart/signed; boundary="Apple-Mail=_A60FF06D-114D-4F9E-88FA-EA70BCD1121B"; protocol="application/pgp-signature"; micalg=pgp-sha512 --Apple-Mail=_A60FF06D-114D-4F9E-88FA-EA70BCD1121B Content-Type: multipart/alternative; boundary="Apple-Mail=_936B5D40-77A0-446C-84BF-367F16623A39" --Apple-Mail=_936B5D40-77A0-446C-84BF-367F16623A39 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 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 = we start much later (outside of the firmware), we can no longer control thi= s. Best regards, Vitaly > 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 > 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 > -----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 > -----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 > Thanks, > Ray >=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 > 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 --Apple-Mail=_936B5D40-77A0-446C-84BF-367F16623A39 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 Hi Michael,

I believe Intel SATA controllers hav= e non-standard lockdown bits, which do not let you reconfigure them as soon= as the initialisation is over. Since we start much later (outside of the f= irmware), 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.kin= ney@intel.com> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0):
Hi Vitaly,=
 
Can you please explain why the controller can not be conf= igured in a non-RAID mode?
 
Thanks,
 = ;
Mike
<= o:p class=3D""> 
<= b class=3D"">From: devel= @edk2.groups.io <<= a href=3D"mailto:devel@edk2.groups.io" style=3D"color: purple; text-decorat= ion: underline;" class=3D"">devel@edk2.groups.io> On Behalf Of Vitaly Cheptsov
Sent: Tuesday,= December 15, 2020 12:58 AM
To: Wu, Hao A <hao.a.wu@intel.com>;&nb= sp;devel@edk2.groups.io
Cc: = 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>;= Kinney, Michael D <michael.d.kinne= y@intel.com>
Subject: Re: [edk2-devel] [PATCH 1/2] MdeModuleP= kg/SataControllerDxe: Add support for drives in RAID mode
&n= bsp;
Hello,
 
1. Could you help to ch= ange the BZ tracker https://bugzilla.tianocore.org/show_bug.cgi?id= = =3D3118 to a "Tiano Feature Requests"= ?
For me, it looks more like a feature request.
 
<= div style=3D"margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibr= i, sans-serif;" class=3D"">Sure, done.
 
2. Could you help to include= 'AtaAtapiPassThru' in the BZ tracker subject for better reference?<= span class=3D"">
 <= /o:p>
Also d= one.
 
3. Fo= r Patch 2/2, is there any reason to clear the bits:
EFI_ATA_P= ASS_THRU_ATTRIBUTES_PHYSICAL
EFI_EXT_SCSI_PASS_THRU_ATTRIBUTE= S_PHYSICAL
If the drives are intended to be used as non-RAID = devices, I think both the ATTRIBUTES_PHYSICAL & ATTRIBUTES_LOGICAL shou= ld be set for the controller according to the UEFI Spec.
 
I am not quite po= sitive 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 it was= required to take the right path in the driver.
<= o:p class=3D""> 
DuetPkg was removed from edk2.
If the change is specially for DUET use case, I am afraid we cannot= accept this change.
 
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 t= he SATA controller is in its non-RAID mode.
 
Agree, but in this case it is no= t feasible.
 
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.
=
&= nbsp;
<= span class=3D"">Actually some operating systems have to introduce workaroun= ds for this as well, and no, in this particular case the OS does not treat = the drive as RAID either.
 <= /o:p>
If there are no other review comments besides the attributes, I w= ill proceed with V2 in the coming days.
<= /div>
 
Best regards,
Vitaly
 
[1] https://github.com/acidanthera/OpenCo= rePkg
 


=
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):
 <= /div>
I agree it would be better to configure the platform so the SATA con= troller is in its non-RAID mode.

If the contro= ller is in RAID mode, then the OS that is booted may have a SATA RAID drive= r
that can configure the drives in RAID mode.  Then, if = the UEFI FW treats it as non RAID, it
may not be bootable, an= d 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


-----Original Message-----
From: devel@edk2.grou= ps.io <devel@edk2.groups.io> On Behalf Of Wu, Hao A
Sent: Monday, December 14, 2020 5:53 PM
To: Ni, Ray = <ray.ni@intel.com>; devel@edk2.grou= ps.io; cheptsov@ispras.ru
Cc: Wang, Jian J <jian.j.wang@intel.com>; Albecki, Mateusz &= lt;mateusz.albecki@intel.com>; Las= zlo Ersek <lersek@redhat.com>
Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe= : Add support for drives in RAID mode


-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Tuesday, De= cember 15, 2020 9:45 AM
To: devel@edk2.groups.io; c= heptsov@ispras.ru
Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
Albecki, Mateusz <mateusz.albecki@intel.com>; Laszlo Er= sek
<lersek@redhat.com>Subject: RE: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControl= lerDxe:
Add support for drives in RAID mode
DuetPkg was removed from edk2.
If the change is s= pecially for DUET use case, I am afraid we cannot accept this
change.

Hao,
Can this change be= nefit a general use case?


He= llo Ray,

My understanding to the proposed PCD = is that drives behind a RAID mode SATA controller can be configured to work= ing in
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: confi= gure the controller to non-RAID mode). The firmware does not support
UEFI, and we are running through DuetPkg."

Best Regards,
Hao Wu




Thanks,
Ray


-----Original Message-----
From: devel@edk2.groups.io=  <devel@edk2.groups.io> = On Behalf Of Vitaly
Cheptsov
Sent: Friday, Dece= mber 11, 2020 5:25 PM
To: devel@edk2.groups.io
Cc: Vitaly Cheptsov <cheptsov@ispras= .ru>; Wang, Jian J
<j= ian.j.wang@intel.com>; Wu, Hao A <hao.= a.wu@intel.com>; Albecki,
Mateusz <mateusz.albecki@intel.com>; Laszlo Ersek <lersek@redhat.com>
Subject: [edk2= -devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe:
Add

support for drives in RAID mode

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

This resolve= s the problem of using drivers connected to Intel G33
builtin= SATA controller when run from DuetPkg when it can only be
co= nfigured in RAID mode through the firmware settings.

Cc: Jian J Wang <jian.j.wang@int= el.com>
Cc: Hao A Wu <ha= o.a.wu@intel.com>
Cc: Mateusz Albecki <mateusz.albecki@intel.com>
Cc: L= aszlo Ersek <lersek@redhat.com>
Signed-off-by: Vitaly Cheptsov <chepts= ov@ispras.ru>
---
MdeModulePkg/Bus/Pci/S= ataControllerDxe/SataController.c | 4 ++--
1 file changed, 2 = insertions(+), 2 deletions(-)

diff --git a/Mde= ModulePkg/Bus/Pci/SataControllerDxe/SataController.c
b/MdeMod= ulePkg/Bus/Pci/SataControllerDxe/SataController.c
index ab06e= 2833c..301335c967 100644
--- a/MdeModulePkg/Bus/Pci/SataContr= ollerDxe/SataController.c
+++ b/MdeModulePkg/Bus/Pci/SataCont= rollerDxe/SataController.c
@@ -324,7 +324,7 @@ SataController= Supported (
    return EFI_UNSUPPORTED;  }

-  if (IS_PCI_= IDE (&PciData) || IS_PCI_SATADPA (&PciData)) {
+ &nbs= p;if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData) ||
+ IS_PCI_RAID (&PciData)) {
   &nbs= p;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 gro= up.
View/Reply Online (#68707):
https://edk2.groups.io/g/devel/message/68707=
Mute This Topic:&n= bsp;https://groups.io/mt/7887= 5596/1712937
Group Owner: devel+owner@edk2.grou= ps.io
Unsubscribe:&= nbsp;https://edk2.groups.io/g/= devel/unsub [ray.ni@intel.com]
-=3D-=3D-=3D-=3D-=3D= -=3D





 
 

--Apple-Mail=_936B5D40-77A0-446C-84BF-367F16623A39-- --Apple-Mail=_A60FF06D-114D-4F9E-88FA-EA70BCD1121B 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/Y85AACgkQL8K2O86E yz6pFhAAgCKBvGPhaM8daiA5jt9pnCy5TV39sC/XykhnMbsdblsed9yriHLiSjlS SkP/uCue00k5iONnni9Xi1xHif4mv3HkYOlAqi4KeXfcnsBaB0DX0Eiw7A/YWm5M tCwzUo3jF1khaxk93OJdJchFnARX6MvmENnixkyq7sT9t6CnxXBmob2Xkqgwl7wx Oliw0G4iObFj6zkOsRp1WpesA+GhAabKetqIPFw3zbV452B9UA8cHRkRVdaTZRRi n2638WfrTAAIdEoybEDJzXoi2l5+FaU/4n+4fFq+FWcCAZGyTIKtSGJzvuT65Xxu I1Sr2Pv+zXhnPiDcivFROgplHDb9fAcxPXQTwu9OloHetU6zl8hZwGnPdb5w//0m /digmRJ6Cr8aVeSK66FQ3GiRHvfSFglcRwPihI6xJudnAYAL/OP9M4YUOZUy6Oe1 rRWwsIwMnej6lk7e3UOCEPCUaHZsV+qxY9qcb08FABfcpSZ1YtYYZpTDqbv0k8sQ 8eD7P3JIj49xshavMW80sIc+cF7NfDrzGgThw1kzMhk+0n9CKlSm870qfWRtGGWn ltPtqlU7fPXq8rUEbp8KLN5VCEap6ltOkR5q6e3ckx/P+1DUMc9p37XWv56AQJAU Nae1iNFlFdSYh8wv2hwpCUJllIMlmvI7gc35T2MXz+32u+9IP2A= =6U7w -----END PGP SIGNATURE----- --Apple-Mail=_A60FF06D-114D-4F9E-88FA-EA70BCD1121B--