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.web11.469.1608058549890482404 for ; Tue, 15 Dec 2020 10:55:50 -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.113] (unknown [77.232.9.83]) by mail.ispras.ru (Postfix) with ESMTPSA id BDCF040A9285; Tue, 15 Dec 2020 18:55:47 +0000 (UTC) From: "Vitaly Cheptsov" Mime-Version: 1.0 (1.0) Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode Date: Tue, 15 Dec 2020 21:55:47 +0300 Message-Id: <8D2E00B5-7781-4FB9-9FD2-229923D7957D@ispras.ru> References: Cc: devel@edk2.groups.io, "Wu, Hao A" , "Ni, Ray" , "Wang, Jian J" , "Albecki, Mateusz" , Laszlo Ersek In-Reply-To: To: "Kinney, Michael D" X-Mailer: iPhone Mail (18B92) Content-Type: multipart/alternative; boundary=Apple-Mail-B610ADC5-49A7-46D8-A32F-549677FC6965 Content-Transfer-Encoding: 7bit --Apple-Mail-B610ADC5-49A7-46D8-A32F-549677FC6965 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Not correct, these systems do not have hard RAID support or anything alike.= It is standard G45 from what I remember. I believe the vendor simply left = the firmware supplier defaults or something alike as there is a way to use = IDE mode but nothing for AHCI. Vitaly > 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. > > It is difficult to support a change that could corrupt data. > > Mike > > From: Vitaly Cheptsov =20 > 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 > > 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. > > 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= ): > > But do the systems allow the user to configure the FW that runs earlier?= Can you require to users to configure their platforms correctly? > > Thanks, > > Mike > > From: devel@edk2.groups.io On Behalf Of Vitaly Ch= eptsov > 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 > > 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. Sinc= e we start much later (outside of the firmware), we can no longer control t= his. > > 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= ): > > Hi Vitaly, > > Can you please explain why the controller can not be configured in a non= -RAID mode? > > Thanks, > > Mike > > From: devel@edk2.groups.io On Behalf Of Vitaly Ch= eptsov > Sent: Tuesday, December 15, 2020 12:58 AM > To: Wu, Hao A ; devel@edk2.groups.io > Cc: Ni, Ray ; Wang, Jian J ; Al= becki, Mateusz ; Laszlo Ersek ; Kinney, Michael D > Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Ad= d support for drives in RAID mode > > 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 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. > > 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 n= ot 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 a= s well, and no, in this particular case the OS does not treat the drive as = RAID either. > > If there are no other review comments besides the attributes, I will pro= ceed with V2 in the coming days. > > Best regards, > Vitaly > > [1] https://github.com/acidanthera/OpenCorePkg > >=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= ): > > 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.groups.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 ; Wang, 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 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 >=20 > > > >=20 > --Apple-Mail-B610ADC5-49A7-46D8-A32F-549677FC6965 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable
Not correct, these sy= stems do not have hard RAID support or anything alike. It is standard G45 f= rom what I remember. I believe the vendor simply left the firmware supplier= defaults or something alike as 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> wrote:

<= /div>
=EF=BB=BF

S= o those types of systems must have a RAID enabled FW driver.  Right?  So the drives = could be configured as a RAID set and using the patch you suggest below cou= ld corrupt data.

<= o:p> 

I= t is difficult to support a change that could corrupt data.

<= o:p> 

M= ike

<= o:p> 

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>; = Albecki, Mateusz <mateusz.albecki@intel.com>; Laszlo Ersek <lersek= @redhat.com>
Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDx= e: Add support for drives in RAID mode

 

Unfortunately not. That is basically the issue. When there= is a preference, it is possible to ask the user to set it. However, for ce= rtain Dell machines, we have an issue with, it is not possible to select AHCI mode in the firmware preferences, and thes= e 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.kinn= ey@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?

 

Thanks,

 

Mike

 

From: = devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Vitaly Ch= eptsov
Sent: Tuesday, De= cember 15, 2020 9:34 AM
To: Kinney, Micha= el D <michael.d.kinney@intel.com>
Cc: devel@edk2.groups.i= o; 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: [edk= 2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives= in RAID mode

 

Hi Michael,

 

I believe Intel SATA controllers have non-standard lockdow= n 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 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):

 

Hi Vitaly,

 

Can you please explain why the controller can not be confi= gured in a non-RAID mode?

 

Thanks,

 

Mike

 

From: = devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Vitaly Ch= eptsov
Sent: Tuesday, De= cember 15, 2020 12:58 AM
To: Wu, Hao A <= ;hao.a.w= u@intel.com>; dev= el@edk2.groups.io
Cc: Ni, Ray <<= a href=3D"mailto:ray.ni@intel.com">ray.ni@inte= l.com>; Wang, Jian J <jian.j.wang@intel.com>; Albecki, Mateusz <mateusz.albecki@intel.com>; Laszlo Erse= k <ler= sek@redhat.com>; Kinney, Michael D <michael.d.kinney@intel= .com>
Subject: Re: [edk= 2-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&= nbsp;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 subject 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 th= e ATTRIBUTES_PHYSICAL & ATTRIBUTES_LOGICAL should be set for the contro= ller 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 i= t on real hardware. 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 accept= this change.

 

This is not the DuetPkg from EDK II, but ours[1]. Thus you= r claim does not apply.

 

I agree it would be better to configure the platform so th= e SATA controller 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 boo= ted may have a SATA RAID driver
that can configure the drives in RAID mode.  Then, if the UEFI FW tre= ats 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 workarou= nds for this as well, and no, in this particular case the OS does not treat= the drive as RAID either.

 

If there are no other review comments besides the attribut= es, I will proceed with V2 in the coming days.

 

Best regards,

Vitaly

 

 





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 configure the platform so th= e SATA controller is in its non-RAID mode.

If the controller is in RAID mode, then the OS that is booted may have a S= ATA RAID driver
that can configure the drives in RAID mode.  Then, if the UEFI FW tre= ats 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




-----Original Message----- From: devel@edk2.groups.io <devel@edk2.groups.i= o> On Behalf Of Wu, Hao A
Sent: Monday, December 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>; Albecki, Mateusz &= lt;mateusz.albecki@intel.com>; Laszlo Ersek <ler= sek@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, December 15, 2020 9:45 AM
To: devel@edk2.groups.io; cheptsov@ispras.ru<= /a>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@int= el.com>;
Albecki, Mateusz <mateusz.albecki@intel.com>; Laszlo Ersek=
<lers= ek@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 cannot 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 (acting like individual drives).

As for the DuetPkg, below is a previous comment from Vitaly:
"there is no firmware preference for that (Hao: configure the controller t= o 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.i= o> 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@intel.com&g= t;; Albecki,
Mateusz <mateusz.albecki@intel.com>; Laszlo Ersek <lersek@redhat= .com>
Subject: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe:

Add



support for drives i= n 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 controller when run from DuetPkg when it can only be
configured in RAID mode through the firmware 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 <<= span style=3D"color:purple">cheptsov@ispras.ru>
---
MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

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

-  if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData)) {<= br> +  if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData) ||<= br> + IS_PCI_RAID (&PciData)) {
    return EFI_SUCCESS;
  }

@@ -465,7 +465,7 @@ SataControllerStart (
  if (IS_PCI_IDE (&PciData)) {
    Private->IdeInit.ChannelCount =3D IDE_MAX_CHANN= EL;
    Private->DeviceCount     &n= bsp;    =3D IDE_MAX_DEVICES;
-  } else if (IS_PCI_SATADPA (&PciData)) {
+  } else if (IS_PCI_SATADPA (&PciData) || IS_PCI_RAID (&PciD= ata)) {
    //
    // Read Ports Implemented(PI) to calculate max por= t number (0 based).
    //
--
2.24.3 (Apple Git-128)



-=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: ht= tps://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







 

 

 

 

--Apple-Mail-B610ADC5-49A7-46D8-A32F-549677FC6965--