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.8540.1608022706161479590 for ; Tue, 15 Dec 2020 00:58:26 -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 CAB2F40A1DD1; Tue, 15 Dec 2020 08:58:22 +0000 (UTC) From: "Vitaly Cheptsov" Message-Id: <64FCA78C-6E93-4126-AF7F-D586865C408B@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: Tue, 15 Dec 2020 11:58:21 +0300 In-Reply-To: Cc: "Ni, Ray" , "Wang, Jian J" , "Albecki, Mateusz" , Laszlo Ersek , "Kinney, Michael D" To: "Wu, Hao A" , "devel@edk2.groups.io" References: <20201211092502.21763-1-cheptsov@ispras.ru> X-Mailer: Apple Mail (2.3654.40.0.2.32) X-Groupsio-MsgNum: 68856 Content-Type: multipart/signed; boundary="Apple-Mail=_6A8420C3-F519-47E1-AC0D-070A8EB32EA4"; protocol="application/pgp-signature"; micalg=pgp-sha512 --Apple-Mail=_6A8420C3-F519-47E1-AC0D-070A8EB32EA4 Content-Type: multipart/alternative; boundary="Apple-Mail=_8D5E2396-6205-402E-9001-DCDB4C0E46C5" --Apple-Mail=_8D5E2396-6205-402E-9001-DCDB4C0E46C5 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Hello, > 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. Sure, done. > 2. Could you help to include 'AtaAtapiPassThru' in the BZ tracker subjec= t for better reference? Also done. > 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. I am not quite 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 hardwa= re. I think it 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 acce= pt 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 the SATA control= ler is in its non-RAID mode. Agree, but in this case it is not 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. Actually 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 RA= ID either. If there are no other review comments besides the attributes, I will proce= ed with V2 in the coming days. Best regards, Vitaly [1] https://github.com/acidanthera/OpenCorePkg > 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 >> -----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.groups.io; cheptsov@ispras.r= u >> Cc: Wang, Jian J ; Albecki, Mateusz ; Laszlo Ersek >> Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: A= dd support for drives in RAID mode >>=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 ac= cept 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 = SATA 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 controlle= r to non-RAID mode). The firmware does not support >> UEFI, and we are running through DuetPkg." >>=20 >> Best Regards, >> Hao Wu >>=20 >>=20 >>>=20 >>> Thanks, >>> Ray >>>=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 ; Wang, Jian J >>>> ; Wu, Hao A ; Albecki, >>>> Mateusz ; Laszlo Ersek >>>> Subject: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: >>> Add >>>> 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 bas= ed). >>>> // >>>> -- >>>> 2.24.3 (Apple Git-128) >>>>=20 >>>>=20 >>>>=20 >>>> -=3D-=3D-=3D-=3D-=3D-=3D >>>> Groups.io Links: You receive all messages sent to this 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 --Apple-Mail=_8D5E2396-6205-402E-9001-DCDB4C0E46C5 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 Hello,
1. C= ould you help to change the BZ tracker https://bugzilla.tiano= core.org/show_bug.cgi?id=3D3118 to a "Tiano Feature Requests"?
For me, it looks more like a feature request.

Sure, done.

2. Could you he= lp to include 'AtaAtapiPassThru' in the BZ tracker subject for better refer= ence?

Also done.

3. For Patch 2/2, is there any reason to clear the bits:<= /span>
EFI_ATA_PASS_THRU_ATTRIBUTES_PHYSICAL
EFI_EXT_SCSI_PASS_THRU_ATT= RIBUTES_PHYSICAL
If the drives are intended to be used as non-RAID devi= ces, I think both the ATTRIBUTES_PHYSICAL & ATTRIBUTES_LOGICAL should b= e set for the controller according to the UEFI Spec.

I am not quite 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 it was required to take the righ= t path in the driver.

DuetPkg was removed from edk2.<= /span>
If the change is specially for DUET use case, I am afraid we cannot = accept this change.

T= his is not the DuetPkg from EDK II, but ours[1]. Thus your claim does not a= pply.

I agree it would be better to configure the platf= orm so the SATA controller is in its non-RAID mode.

Agree, but in this case it is not feasible.<= /div>

If the controller is in RAID mode, then the OS that i= s booted may have a SATA RAID driver
that can configure the drives in R= AID 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.

Actually som= e 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 either.

If there are no oth= er review comments besides the attributes, I will proceed with V2 in the co= ming days.

Best r= egards,
Vitaly

[1] https://github.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 co= nfigure the platform so the SATA controller is in its non-RAID mode.

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, itmay not be bootable, and configuration actions in UEFI may cor= rupt data on the RAID configured
drives.  For this reaso= n, I am not in favor of adding a PCD.

Mike

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

-----Original Me= ssage-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Tuesday, Decemb= er 15, 2020 9:45 AM
To: devel@edk2.groups.io; cheptsov@ispras.ru
Cc: Wang, Jian J <jian.j.wang@intel.com&= gt;; Wu, Hao A <hao.a.w= u@intel.com>;
Albecki, Mateusz <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

DuetPkg was removed from edk2.
If the change is specially for DUET use case, I am afraid we cann= ot accept 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 SAT= A controller can be configured to working in
non-RAID mode (a= cting like individual drives).

As for the Duet= Pkg, below is a previous comment from Vitaly:
"there is no fi= rmware preference for that (Hao: configure the controller to non-RAID mode)= . The firmware does not support
UEFI, and we are running thro= ugh DuetPkg."

Best Regards,
Hao = Wu



Thanks,
Ray

-----Original Message-----
From: devel@ed= k2.groups.io <dev= el@edk2.groups.io> On Behalf Of Vitaly
Cheptsov
Sent: Friday, December 11, 2020 5:25 PM
To: devel@edk2.groups.io
Cc: Vitaly Cheptsov <cheptsov@ispras.ru>; Wang, Jian J
<jian.j.wang@intel.com>= ; Wu, Hao A <hao.a.wu@i= ntel.com>; Albecki,
Mateusz <mateusz.albecki@intel.com>; Laszlo= Ersek <lersek@redhat.co= m>
Subject: [edk2-devel] [PATCH 1/2] MdeModulePkg/Sata= ControllerDxe:
Add
support for drives in RAID mode

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3118
This resolves the problem of using drivers conne= cted to Intel G33
builtin SATA controller when run from DuetP= kg when it can only be
configured in RAID mode through the fi= rmware settings.

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

diff --git a/M= deModulePkg/Bus/Pci/SataControllerDxe/SataController.c
b/MdeM= odulePkg/Bus/Pci/SataControllerDxe/SataController.c
index ab0= 6e2833c..301335c967 100644
--- a/MdeModulePkg/Bus/Pci/SataCon= trollerDxe/SataController.c
+++ b/MdeModulePkg/Bus/Pci/SataCo= ntrollerDxe/SataController.c
@@ -324,7 +324,7 @@ SataControll= erSupported (
    return EFI_UNSUPPORTED= ;
  }

-  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;
  }

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



-=3D-=3D-=3D-= =3D-=3D-=3D
Groups.i= o Links: You receive all messages sent to this group.
Vie= w/Reply Online (#68707):
https://edk2.groups.io/g/devel/message/6870= 7
Mute This Topic: https://groups.io/mt/78875596/1712937<= br class=3D"">Group Owner: devel+owner@edk2.groups.io
Unsubsc= ribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com]
-=3D-=3D-=3D-=3D-=3D-=3D








--Apple-Mail=_8D5E2396-6205-402E-9001-DCDB4C0E46C5-- --Apple-Mail=_6A8420C3-F519-47E1-AC0D-070A8EB32EA4 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/Yeq0ACgkQL8K2O86E yz5MIg/+PeE5tiNI/MWjXgFB8FxH+7kYsR+48fhGVnqBKmxlz3i3Q78f+9zzKGQ1 I/5FacdCjYzTFRW39uV1RT1NcYXNdIdCnJXesndLXCGNYxKTTqnEuunlc+BVXPeQ TQ704K/ass3Lc2ghO2j7Xax73HIgUSLJKCdqHJ6RJBHOy6u0DmwioZ69RncXv34g KgxuN7CrtjOD9xqtYq0EbYbIjJ4ZQoZK+A6IXD4MlOkeSk9v7TPjuOr7lNe9cMYO Hbmxwm2tW9Nrqn4kZc6HENx3YGmpirfYNtOEDC5RGrEAFXmsjW79yWZHI1fdD/kR p+i75F1rTNP5ytjgJ94D4O0o1R2kNWv36n0N1WLyvrDdxeibYZITso/zHLj/bN8c JbLT22LPwUTr673MFHaIirqCDtuwdsKEHjOWvi/YC3hpSNIEGWonBCjxpIWUY0i2 LamWNNWRCAu0Naws5ob//EWMgUGOjbl7ma9lqdwaj5XOqRMdUOHA158XN7cn6jLj 9+2nQYFm3Uk/QP5Qe/r/i/4EnWJtQkxO1Ci89sR7YbHRlWNq2zWGFhsnQPQcXYrS EUc4N3ekpxhs3yGSAAdAI2OL8VDlFjXV+2mqp1FuZa7HS7QKU/oZV3jnWVNkKDrS KA1GsIU/Gt5n81lyd9BmBnh7C4nooYoXpZsZjBbDAEXcEDR57So= =Y7Dw -----END PGP SIGNATURE----- --Apple-Mail=_6A8420C3-F519-47E1-AC0D-070A8EB32EA4--