[PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode

[PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode

[Vitaly Cheptsov]

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

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 <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)) {
+  if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData) || IS_PCI_RAID (&PciData)) {
     return EFI_SUCCESS;
   }
 
@@ -465,7 +465,7 @@ SataControllerStart (
   if (IS_PCI_IDE (&PciData)) {
     Private->IdeInit.ChannelCount = IDE_MAX_CHANNEL;
     Private->DeviceCount          = 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)

[PATCH 2/2] MdeModulePkg/AtaAtapiPassThru: Add support for drives in RAID mode

[Vitaly Cheptsov]

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

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 <cheptsov@ispras.ru>
---
 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
index 86fe9d954f..5892508aef 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.c
@@ -620,7 +620,7 @@ AtaAtapiPassThruSupported (
     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;
   }
 
@@ -1208,6 +1208,12 @@ EnumerateAttachedDevice (
         goto Done;
       }
       break;
+    case PCI_CLASS_MASS_STORAGE_RAID :
+      Instance->AtaPassThruMode.Attributes &= ~EFI_ATA_PASS_THRU_ATTRIBUTES_PHYSICAL;
+      Instance->ExtScsiPassThruMode.Attributes &= ~EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_PHYSICAL;
+      //
+      // Fall through to AHCI
+      //
     case PCI_CLASS_MASS_STORAGE_SATADPA :
       //
       // The ATA controller is working at AHCI mode
-- 
2.24.3 (Apple Git-128)

Re: [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode

[Wu, Hao A]

> -----Original Message-----
> From: Vitaly Cheptsov <cheptsov@ispras.ru>
> 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>; Albecki,
> Mateusz <mateusz.albecki@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for
> drives in RAID mode
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3118
> 
> 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.


Hello Vitaly,

If my understanding is correct, this driver (SataControllerDxe) and the
AtaAtapiPassThru driver are written for non-RAID case only.

Both drivers (especially AtaAtapiPassThru) do not distinguish logic/physical
SCSI channels, which I think only works for the non-RAID case. I am not sure if
this patch series will have an impact to existing RAID drivers.

Best Regards,
Hao Wu


> 
> 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 <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)) {
> +  if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData) || IS_PCI_RAID
> + (&PciData)) {
>      return EFI_SUCCESS;
>    }
> 
> @@ -465,7 +465,7 @@ SataControllerStart (
>    if (IS_PCI_IDE (&PciData)) {
>      Private->IdeInit.ChannelCount = IDE_MAX_CHANNEL;
>      Private->DeviceCount          = 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)

Re: [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode

[Vitaly Cheptsov]

Hello Hao,

This is for the case when the drives are not used as a RAID, but the controller is initialised in RAID mode. However, you are right that if a dedicated RAID driver is present, it is best to use it instead. To support both cases can we introduce an off-by-default PCD (e.g. TreatRaidAsSata) to workaround this?

Best regards,
Vitaly

> On 14 Dec 2020, at 09:22, Wu, Hao A <hao.a.wu@intel.com> wrote:
> 
> 
>> 
>> -----Original Message-----
>> From: Vitaly Cheptsov <cheptsov@ispras.ru>
>> 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>; Albecki,
>> Mateusz <mateusz.albecki@intel.com>; Laszlo Ersek <lersek@redhat.com>
>> Subject: [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for
>> drives in RAID mode
>> 
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3118
>> 
>> 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.
> 
> 
> Hello Vitaly,
> 
> If my understanding is correct, this driver (SataControllerDxe) and the
> AtaAtapiPassThru driver are written for non-RAID case only.
> 
> Both drivers (especially AtaAtapiPassThru) do not distinguish logic/physical
> SCSI channels, which I think only works for the non-RAID case. I am not sure if
> this patch series will have an impact to existing RAID drivers.
> 
> Best Regards,
> Hao Wu
> 
> 
>> 
>> 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 <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)) {
>> +  if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData) || IS_PCI_RAID
>> + (&PciData)) {
>>     return EFI_SUCCESS;
>>   }
>> 
>> @@ -465,7 +465,7 @@ SataControllerStart (
>>   if (IS_PCI_IDE (&PciData)) {
>>     Private->IdeInit.ChannelCount = IDE_MAX_CHANNEL;
>>     Private->DeviceCount          = 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)
> 

Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode

[Wu, Hao A]

Hello Vitaly,

It sounds to me that the controller driver should properly reflect the mode
according to the actual configuration. Is it possible to update the behavior of
the controller driver?

In my opinion, it seems weird to add WA in these general drivers for platform
driver issue. It might also cause confusion for users of AtaAtapiPassThru to
think it has RAID support.

Best Regards,
Hao Wu

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Vitaly
> Cheptsov
> Sent: Monday, December 14, 2020 3:34 PM
> To: Wu, Hao A <hao.a.wu@intel.com>
> Cc: devel@edk2.groups.io; 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 drives in RAID mode
> 
> Hello Hao,
> 
> This is for the case when the drives are not used as a RAID, but the controller is
> initialised in RAID mode. However, you are right that if a dedicated RAID driver is
> present, it is best to use it instead. To support both cases can we introduce an
> off-by-default PCD (e.g. TreatRaidAsSata) to workaround this?
> 
> Best regards,
> Vitaly
> 
> > On 14 Dec 2020, at 09:22, Wu, Hao A <hao.a.wu@intel.com> wrote:
> >
> > 
> >>
> >> -----Original Message-----
> >> From: Vitaly Cheptsov <cheptsov@ispras.ru>
> >> 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>; Albecki,
> >> Mateusz <mateusz.albecki@intel.com>; Laszlo Ersek <lersek@redhat.com>
> >> Subject: [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for
> >> drives in RAID mode
> >>
> >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3118
> >>
> >> 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.
> >
> >
> > Hello Vitaly,
> >
> > If my understanding is correct, this driver (SataControllerDxe) and
> > the AtaAtapiPassThru driver are written for non-RAID case only.
> >
> > Both drivers (especially AtaAtapiPassThru) do not distinguish
> > logic/physical SCSI channels, which I think only works for the
> > non-RAID case. I am not sure if this patch series will have an impact to existing
> RAID drivers.
> >
> > Best Regards,
> > Hao Wu
> >
> >
> >>
> >> 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 <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)) {
> >> +  if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData) ||
> >> + IS_PCI_RAID
> >> + (&PciData)) {
> >>     return EFI_SUCCESS;
> >>   }
> >>
> >> @@ -465,7 +465,7 @@ SataControllerStart (
> >>   if (IS_PCI_IDE (&PciData)) {
> >>     Private->IdeInit.ChannelCount = IDE_MAX_CHANNEL;
> >>     Private->DeviceCount          = 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)
> >
> 
> 
> 
> 

Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode

[Vitaly Cheptsov]

Hello Hao,

No, it is not possible to change that, since there is no firmware preference for that. The firmware does not support UEFI, and we are running through DuetPkg.

I believe it is not quite a workaround as the fact that an actual RAID array is installed and the fact that a RAID array is supported are separate matters and may not be distinguishable via IS_RAID. At least this is how Intel controllers work to date. A clear name for the PCD should not cause any confusion. If you think TreatRaidAsSata is not great, we could choose ForceRaidAsSingleDrive.

Best regards,
Vitaly

> On 14 Dec 2020, at 10:56, Wu, Hao A <hao.a.wu@intel.com> wrote:
> 
> Hello Vitaly,
> 
> It sounds to me that the controller driver should properly reflect the mode
> according to the actual configuration. Is it possible to update the behavior of
> the controller driver?
> 
> In my opinion, it seems weird to add WA in these general drivers for platform
> driver issue. It might also cause confusion for users of AtaAtapiPassThru to
> think it has RAID support.
> 
> Best Regards,
> Hao Wu
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Vitaly
>> Cheptsov
>> Sent: Monday, December 14, 2020 3:34 PM
>> To: Wu, Hao A <hao.a.wu@intel.com>
>> Cc: devel@edk2.groups.io; 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 drives in RAID mode
>> 
>> Hello Hao,
>> 
>> This is for the case when the drives are not used as a RAID, but the controller is
>> initialised in RAID mode. However, you are right that if a dedicated RAID driver is
>> present, it is best to use it instead. To support both cases can we introduce an
>> off-by-default PCD (e.g. TreatRaidAsSata) to workaround this?
>> 
>> Best regards,
>> Vitaly
>> 
>>>> On 14 Dec 2020, at 09:22, Wu, Hao A <hao.a.wu@intel.com> wrote:
>>> 
>>> 
>>>> 
>>>> -----Original Message-----
>>>> From: Vitaly Cheptsov <cheptsov@ispras.ru>
>>>> 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>; Albecki,
>>>> Mateusz <mateusz.albecki@intel.com>; Laszlo Ersek <lersek@redhat.com>
>>>> Subject: [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for
>>>> drives in RAID mode
>>>> 
>>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3118
>>>> 
>>>> 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.
>>> 
>>> 
>>> Hello Vitaly,
>>> 
>>> If my understanding is correct, this driver (SataControllerDxe) and
>>> the AtaAtapiPassThru driver are written for non-RAID case only.
>>> 
>>> Both drivers (especially AtaAtapiPassThru) do not distinguish
>>> logic/physical SCSI channels, which I think only works for the
>>> non-RAID case. I am not sure if this patch series will have an impact to existing
>> RAID drivers.
>>> 
>>> Best Regards,
>>> Hao Wu
>>> 
>>> 
>>>> 
>>>> 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 <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)) {
>>>> +  if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData) ||
>>>> + IS_PCI_RAID
>>>> + (&PciData)) {
>>>>    return EFI_SUCCESS;
>>>>  }
>>>> 
>>>> @@ -465,7 +465,7 @@ SataControllerStart (
>>>>  if (IS_PCI_IDE (&PciData)) {
>>>>    Private->IdeInit.ChannelCount = IDE_MAX_CHANNEL;
>>>>    Private->DeviceCount          = 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)
>>> 
>> 
>> 
>> 
>> 
> 

Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode

[Wu, Hao A]

(Looping in Mike and Ray to see if they have additional comments)

Hello Vitaly,

I am okay with introducing a PCD to force the drives behind a RAID mode controller to be used as normal non-RAID devices.
The name PcdForceRaidAsSingleDrive is fine with me.

If no comment from other reviewers:
1. Could you help to change the BZ tracker https://bugzilla.tianocore.org/show_bug.cgi?id=3118 to a "Tiano Feature Requests"?
For me, it looks more like a feature request.

2. Could you help to include 'AtaAtapiPassThru' in the BZ tracker subject for better reference?

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 controller according to the UEFI Spec.

Best Regards,
Hao Wu

> -----Original Message-----
> From: Vitaly Cheptsov <cheptsov@ispras.ru>
> Sent: Monday, December 14, 2020 4:29 PM
> To: Wu, Hao A <hao.a.wu@intel.com>
> Cc: devel@edk2.groups.io; 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 drives in RAID mode
> 
> Hello Hao,
> 
> No, it is not possible to change that, since there is no firmware preference
> for that. The firmware does not support UEFI, and we are running through
> DuetPkg.
> 
> I believe it is not quite a workaround as the fact that an actual RAID array is
> installed and the fact that a RAID array is supported are separate matters and
> may not be distinguishable via IS_RAID. At least this is how Intel controllers
> work to date. A clear name for the PCD should not cause any confusion. If
> you think TreatRaidAsSata is not great, we could choose
> ForceRaidAsSingleDrive.
> 
> Best regards,
> Vitaly
> 
> > On 14 Dec 2020, at 10:56, Wu, Hao A <hao.a.wu@intel.com> wrote:
> >
> > Hello Vitaly,
> >
> > It sounds to me that the controller driver should properly reflect the
> > mode according to the actual configuration. Is it possible to update
> > the behavior of the controller driver?
> >
> > In my opinion, it seems weird to add WA in these general drivers for
> > platform driver issue. It might also cause confusion for users of
> > AtaAtapiPassThru to think it has RAID support.
> >
> > Best Regards,
> > Hao Wu
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> Vitaly
> >> Cheptsov
> >> Sent: Monday, December 14, 2020 3:34 PM
> >> To: Wu, Hao A <hao.a.wu@intel.com>
> >> Cc: devel@edk2.groups.io; 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 drives in RAID mode
> >>
> >> Hello Hao,
> >>
> >> This is for the case when the drives are not used as a RAID, but the
> >> controller is initialised in RAID mode. However, you are right that
> >> if a dedicated RAID driver is present, it is best to use it instead.
> >> To support both cases can we introduce an off-by-default PCD (e.g.
> TreatRaidAsSata) to workaround this?
> >>
> >> Best regards,
> >> Vitaly
> >>
> >>>> On 14 Dec 2020, at 09:22, Wu, Hao A <hao.a.wu@intel.com> wrote:
> >>>
> >>>
> >>>>
> >>>> -----Original Message-----
> >>>> From: Vitaly Cheptsov <cheptsov@ispras.ru>
> >>>> 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>; Albecki,
> >>>> Mateusz <mateusz.albecki@intel.com>; Laszlo Ersek
> >>>> <lersek@redhat.com>
> >>>> Subject: [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support
> >>>> for drives in RAID mode
> >>>>
> >>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3118
> >>>>
> >>>> 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.
> >>>
> >>>
> >>> Hello Vitaly,
> >>>
> >>> If my understanding is correct, this driver (SataControllerDxe) and
> >>> the AtaAtapiPassThru driver are written for non-RAID case only.
> >>>
> >>> Both drivers (especially AtaAtapiPassThru) do not distinguish
> >>> logic/physical SCSI channels, which I think only works for the
> >>> non-RAID case. I am not sure if this patch series will have an
> >>> impact to existing
> >> RAID drivers.
> >>>
> >>> Best Regards,
> >>> Hao Wu
> >>>
> >>>
> >>>>
> >>>> 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 <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)) {
> >>>> +  if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData) ||
> >>>> + IS_PCI_RAID
> >>>> + (&PciData)) {
> >>>>    return EFI_SUCCESS;
> >>>>  }
> >>>>
> >>>> @@ -465,7 +465,7 @@ SataControllerStart (  if (IS_PCI_IDE
> >>>> (&PciData)) {
> >>>>    Private->IdeInit.ChannelCount = IDE_MAX_CHANNEL;
> >>>>    Private->DeviceCount          = 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)
> >>>
> >>
> >>
> >> 
> >>
> >

Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode

[Ni, Ray]

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?

Thanks,
Ray

> -----Original Message-----
> From: devel@edk2.groups.io <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 <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>
> Subject: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3118
> 
> 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 <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)) {
> +  if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData) || IS_PCI_RAID (&PciData)) {
>      return EFI_SUCCESS;
>    }
> 
> @@ -465,7 +465,7 @@ SataControllerStart (
>    if (IS_PCI_IDE (&PciData)) {
>      Private->IdeInit.ChannelCount = IDE_MAX_CHANNEL;
>      Private->DeviceCount          = 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)
> 
> 
> 
> -=-=-=-=-=-=
> 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]
> -=-=-=-=-=-=
> 

Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode

[Wu, Hao A]

> -----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
> Cc: 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>
> 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 SATA 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 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, 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>; 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.tianocore.org/show_bug.cgi?id=3118
> >
> > 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 <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)) {
> > +  if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData) ||
> > + IS_PCI_RAID (&PciData)) {
> >      return EFI_SUCCESS;
> >    }
> >
> > @@ -465,7 +465,7 @@ SataControllerStart (
> >    if (IS_PCI_IDE (&PciData)) {
> >      Private->IdeInit.ChannelCount = IDE_MAX_CHANNEL;
> >      Private->DeviceCount          = 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)
> >
> >
> >
> > -=-=-=-=-=-=
> > 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]
> > -=-=-=-=-=-=
> >

Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode

[Michael D Kinney]

I agree it would be better to configure 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, 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.io> 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 <mateusz.albecki@intel.com>; Laszlo 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, 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 <hao.a.wu@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 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 SATA 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 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, 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>; 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.tianocore.org/show_bug.cgi?id=3118
> > >
> > > 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 <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)) {
> > > +  if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData) ||
> > > + IS_PCI_RAID (&PciData)) {
> > >      return EFI_SUCCESS;
> > >    }
> > >
> > > @@ -465,7 +465,7 @@ SataControllerStart (
> > >    if (IS_PCI_IDE (&PciData)) {
> > >      Private->IdeInit.ChannelCount = IDE_MAX_CHANNEL;
> > >      Private->DeviceCount          = 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)
> > >
> > >
> > >
> > > -=-=-=-=-=-=
> > > 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]
> > > -=-=-=-=-=-=
> > >
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode

[Vitaly Cheptsov]

[-- Attachment #1.1: Type: text/plain, Size: 6999 bytes --]

Hello,

> 1. Could you help to change the BZ tracker https://bugzilla.tianocore.org/show_bug.cgi?id=3118 <https://bugzilla.tianocore.org/show_bug.cgi?id=3118> 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 the ATTRIBUTES_PHYSICAL & ATTRIBUTES_LOGICAL should be 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 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 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.

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 RAID either.

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

Best regards,
Vitaly

[1] https://github.com/acidanthera/OpenCorePkg <https://github.com/acidanthera/OpenCorePkg>


> 15 дек. 2020 г., в 06:54, Kinney, Michael D <michael.d.kinney@intel.com> написал(а):
> 
> I agree it would be better to configure 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, 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.io> 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 <mateusz.albecki@intel.com>; Laszlo 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, 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 <hao.a.wu@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 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 SATA 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 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, 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>; 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.tianocore.org/show_bug.cgi?id=3118
>>>> 
>>>> 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 <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)) {
>>>> +  if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData) ||
>>>> + IS_PCI_RAID (&PciData)) {
>>>>     return EFI_SUCCESS;
>>>>   }
>>>> 
>>>> @@ -465,7 +465,7 @@ SataControllerStart (
>>>>   if (IS_PCI_IDE (&PciData)) {
>>>>     Private->IdeInit.ChannelCount = IDE_MAX_CHANNEL;
>>>>     Private->DeviceCount          = 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)
>>>> 
>>>> 
>>>> 
>>>> -=-=-=-=-=-=
>>>> 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]
>>>> -=-=-=-=-=-=
>>>> 
>> 
>> 
>> 
>> 
>> 
> 


[-- Attachment #1.2: Type: text/html, Size: 13443 bytes --]

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode

[Wu, Hao A]

[-- Attachment #1: Type: text/plain, Size: 7886 bytes --]

Thanks Vitaly,

Please help to wait one or two days to see if Ray and Mike have further comments for the changes.
Sorry for the inconvenience.

Best Regards,
Hao Wu

From: Vitaly Cheptsov <cheptsov@ispras.ru>
Sent: Tuesday, December 15, 2020 4:58 PM
To: Wu, Hao A <hao.a.wu@intel.com>; 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.kinney@intel.com>
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=3118 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 the ATTRIBUTES_PHYSICAL & ATTRIBUTES_LOGICAL should be 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 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 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.

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 RAID either.

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

Best regards,
Vitaly

[1] https://github.com/acidanthera/OpenCorePkg



15 дек. 2020 г., в 06:54, Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> написал(а):

I agree it would be better to configure 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, 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<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto: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<mailto:ray.ni@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; cheptsov@ispras.ru<mailto:cheptsov@ispras.ru>
Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto: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<mailto:ray.ni@intel.com>>
Sent: Tuesday, December 15, 2020 9:45 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; cheptsov@ispras.ru<mailto:cheptsov@ispras.ru>
Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>;
Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek
<lersek@redhat.com<mailto: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 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 SATA 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 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<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Vitaly
Cheptsov
Sent: Friday, December 11, 2020 5:25 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Vitaly Cheptsov <cheptsov@ispras.ru<mailto:cheptsov@ispras.ru>>; Wang, Jian J
<jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Albecki,
Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Subject: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe:
Add

support for drives in RAID mode

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

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<mailto:jian.j.wang@intel.com>>
Cc: Hao A Wu <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>
Cc: Mateusz Albecki <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>
Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Signed-off-by: Vitaly Cheptsov <cheptsov@ispras.ru<mailto: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)) {
+  if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData) ||
+ IS_PCI_RAID (&PciData)) {
    return EFI_SUCCESS;
  }

@@ -465,7 +465,7 @@ SataControllerStart (
  if (IS_PCI_IDE (&PciData)) {
    Private->IdeInit.ChannelCount = IDE_MAX_CHANNEL;
    Private->DeviceCount          = 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)



-=-=-=-=-=-=
Groups.io<http://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<mailto:devel+owner@edk2.groups.io>
Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com]
-=-=-=-=-=-=







[-- Attachment #2: Type: text/html, Size: 15408 bytes --]

Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode

[Michael D Kinney]

[-- Attachment #1: Type: text/plain, Size: 7871 bytes --]

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 <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>; 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.kinney@intel.com>
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=3118 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 the ATTRIBUTES_PHYSICAL & ATTRIBUTES_LOGICAL should be 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 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 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.

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 RAID either.

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

Best regards,
Vitaly

[1] https://github.com/acidanthera/OpenCorePkg



15 дек. 2020 г., в 06:54, Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> написал(а):

I agree it would be better to configure 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, 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<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto: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<mailto:ray.ni@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; cheptsov@ispras.ru<mailto:cheptsov@ispras.ru>
Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto: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<mailto:ray.ni@intel.com>>
Sent: Tuesday, December 15, 2020 9:45 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; cheptsov@ispras.ru<mailto:cheptsov@ispras.ru>
Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>;
Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek
<lersek@redhat.com<mailto: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 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 SATA 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 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<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Vitaly
Cheptsov
Sent: Friday, December 11, 2020 5:25 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Vitaly Cheptsov <cheptsov@ispras.ru<mailto:cheptsov@ispras.ru>>; Wang, Jian J
<jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Albecki,
Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Subject: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe:
Add

support for drives in RAID mode

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

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<mailto:jian.j.wang@intel.com>>
Cc: Hao A Wu <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>
Cc: Mateusz Albecki <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>
Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Signed-off-by: Vitaly Cheptsov <cheptsov@ispras.ru<mailto: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)) {
+  if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData) ||
+ IS_PCI_RAID (&PciData)) {
    return EFI_SUCCESS;
  }

@@ -465,7 +465,7 @@ SataControllerStart (
  if (IS_PCI_IDE (&PciData)) {
    Private->IdeInit.ChannelCount = IDE_MAX_CHANNEL;
    Private->DeviceCount          = 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)



-=-=-=-=-=-=
Groups.io<http://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<mailto:devel+owner@edk2.groups.io>
Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com]
-=-=-=-=-=-=








[-- Attachment #2: Type: text/html, Size: 56952 bytes --]

Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode

[Vitaly Cheptsov]

[-- Attachment #1.1: Type: text/plain, Size: 9238 bytes --]

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 this.

Best regards,
Vitaly

> 15 дек. 2020 г., в 19:58, Kinney, Michael D <michael.d.kinney@intel.com> написал(а):
> 
> 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 <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto: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 <mailto:hao.a.wu@intel.com>>; devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> Cc: Ni, Ray <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Wang, Jian J <jian.j.wang@intel.com <mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com <mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>; Kinney, Michael D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>
> 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=3118 <https://bugzilla.tianocore.org/show_bug.cgi?id=3118> 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 the ATTRIBUTES_PHYSICAL & ATTRIBUTES_LOGICAL should be 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 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 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.
> 
> 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 RAID either.
> 
> If there are no other review comments besides the attributes, I will proceed with V2 in the coming days.
> 
> Best regards,
> Vitaly
> 
> [1] https://github.com/acidanthera/OpenCorePkg <https://github.com/acidanthera/OpenCorePkg>
> 
> 
> 
> 15 дек. 2020 г., в 06:54, Kinney, Michael D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>> написал(а):
> 
> I agree it would be better to configure 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, 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 <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto: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 <mailto:ray.ni@intel.com>>; devel@edk2.groups.io <mailto:devel@edk2.groups.io>; cheptsov@ispras.ru <mailto:cheptsov@ispras.ru>
> Cc: Wang, Jian J <jian.j.wang@intel.com <mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com <mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com <mailto: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 <mailto:ray.ni@intel.com>>
> Sent: Tuesday, December 15, 2020 9:45 AM
> To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; cheptsov@ispras.ru <mailto:cheptsov@ispras.ru>
> Cc: Wang, Jian J <jian.j.wang@intel.com <mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>;
> Albecki, Mateusz <mateusz.albecki@intel.com <mailto:mateusz.albecki@intel.com>>; Laszlo Ersek
> <lersek@redhat.com <mailto: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 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 SATA 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 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 <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> On Behalf Of Vitaly
> Cheptsov
> Sent: Friday, December 11, 2020 5:25 PM
> To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> Cc: Vitaly Cheptsov <cheptsov@ispras.ru <mailto:cheptsov@ispras.ru>>; Wang, Jian J
> <jian.j.wang@intel.com <mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Albecki,
> Mateusz <mateusz.albecki@intel.com <mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>
> Subject: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe:
> Add
> 
> support for drives in RAID mode
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3118 <https://bugzilla.tianocore.org/show_bug.cgi?id=3118>
> 
> 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 <mailto:jian.j.wang@intel.com>>
> Cc: Hao A Wu <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>
> Cc: Mateusz Albecki <mateusz.albecki@intel.com <mailto:mateusz.albecki@intel.com>>
> Cc: Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>
> Signed-off-by: Vitaly Cheptsov <cheptsov@ispras.ru <mailto: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)) {
> +  if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData) ||
> + IS_PCI_RAID (&PciData)) {
>     return EFI_SUCCESS;
>   }
> 
> @@ -465,7 +465,7 @@ SataControllerStart (
>   if (IS_PCI_IDE (&PciData)) {
>     Private->IdeInit.ChannelCount = IDE_MAX_CHANNEL;
>     Private->DeviceCount          = 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)
> 
> 
> 
> -=-=-=-=-=-=
> Groups.io <http://groups.io/> Links: You receive all messages sent to this group.
> View/Reply Online (#68707):
> https://edk2.groups.io/g/devel/message/68707 <https://edk2.groups.io/g/devel/message/68707>
> Mute This Topic: https://groups.io/mt/78875596/1712937 <https://groups.io/mt/78875596/1712937>
> Group Owner: devel+owner@edk2.groups.io <mailto:devel+owner@edk2.groups.io>
> Unsubscribe: https://edk2.groups.io/g/devel/unsub <https://edk2.groups.io/g/devel/unsub> [ray.ni@intel.com <mailto:ray.ni@intel.com>]
> -=-=-=-=-=-=
> 
> 
> 
> 
> 
> 
> 
> 
> 


[-- Attachment #1.2: Type: text/html, Size: 26922 bytes --]

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode

[Michael D Kinney]

[-- Attachment #1: Type: text/plain, Size: 9233 bytes --]

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 Cheptsov
Sent: Tuesday, December 15, 2020 9:34 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 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 we start much later (outside of the firmware), we can no longer control this.

Best regards,
Vitaly


15 дек. 2020 г., в 19:58, Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> написал(а):

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<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto: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<mailto:hao.a.wu@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
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=3118 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 the ATTRIBUTES_PHYSICAL & ATTRIBUTES_LOGICAL should be 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 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 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.

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 RAID either.

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

Best regards,
Vitaly

[1] https://github.com/acidanthera/OpenCorePkg




15 дек. 2020 г., в 06:54, Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> написал(а):

I agree it would be better to configure 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, 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<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto: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<mailto:ray.ni@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; cheptsov@ispras.ru<mailto:cheptsov@ispras.ru>
Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto: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<mailto:ray.ni@intel.com>>
Sent: Tuesday, December 15, 2020 9:45 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; cheptsov@ispras.ru<mailto:cheptsov@ispras.ru>
Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>;
Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek
<lersek@redhat.com<mailto: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 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 SATA 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 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<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Vitaly
Cheptsov
Sent: Friday, December 11, 2020 5:25 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Vitaly Cheptsov <cheptsov@ispras.ru<mailto:cheptsov@ispras.ru>>; Wang, Jian J
<jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Albecki,
Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Subject: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe:
Add


support for drives in RAID mode

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

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<mailto:jian.j.wang@intel.com>>
Cc: Hao A Wu <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>
Cc: Mateusz Albecki <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>
Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Signed-off-by: Vitaly Cheptsov <cheptsov@ispras.ru<mailto: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)) {
+  if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData) ||
+ IS_PCI_RAID (&PciData)) {
    return EFI_SUCCESS;
  }

@@ -465,7 +465,7 @@ SataControllerStart (
  if (IS_PCI_IDE (&PciData)) {
    Private->IdeInit.ChannelCount = IDE_MAX_CHANNEL;
    Private->DeviceCount          = 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)



-=-=-=-=-=-=
Groups.io<http://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<mailto:devel+owner@edk2.groups.io>
Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com<mailto:ray.ni@intel.com>]
-=-=-=-=-=-=










[-- Attachment #2: Type: text/html, Size: 64151 bytes --]

Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode

[Vitaly Cheptsov]

[-- Attachment #1.1: Type: text/plain, Size: 10696 bytes --]

Unfortunately not. That is basically the issue. When there is a preference, it is possible to ask 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 дек. 2020 г., в 20:41, Kinney, Michael D <michael.d.kinney@intel.com> написал(а):
> 
> 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 <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> On Behalf Of Vitaly Cheptsov
> Sent: Tuesday, December 15, 2020 9:34 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>
> Cc: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Wu, Hao A <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Wang, Jian J <jian.j.wang@intel.com <mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com <mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>
> Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: 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 we start much later (outside of the firmware), we can no longer control this.
> 
> Best regards,
> Vitaly
> 
> 
> 15 дек. 2020 г., в 19:58, Kinney, Michael D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>> написал(а):
> 
> 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 <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto: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 <mailto:hao.a.wu@intel.com>>; devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> Cc: Ni, Ray <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Wang, Jian J <jian.j.wang@intel.com <mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com <mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>; Kinney, Michael D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>
> 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=3118 <https://bugzilla.tianocore.org/show_bug.cgi?id=3118> 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 the ATTRIBUTES_PHYSICAL & ATTRIBUTES_LOGICAL should be 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 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 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.
> 
> 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 RAID either.
> 
> If there are no other review comments besides the attributes, I will proceed with V2 in the coming days.
> 
> Best regards,
> Vitaly
> 
> [1] https://github.com/acidanthera/OpenCorePkg <https://github.com/acidanthera/OpenCorePkg>
> 
> 
> 
> 
> 15 дек. 2020 г., в 06:54, Kinney, Michael D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>> написал(а):
> 
> I agree it would be better to configure 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, 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 <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto: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 <mailto:ray.ni@intel.com>>; devel@edk2.groups.io <mailto:devel@edk2.groups.io>; cheptsov@ispras.ru <mailto:cheptsov@ispras.ru>
> Cc: Wang, Jian J <jian.j.wang@intel.com <mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com <mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com <mailto: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 <mailto:ray.ni@intel.com>>
> Sent: Tuesday, December 15, 2020 9:45 AM
> To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; cheptsov@ispras.ru <mailto:cheptsov@ispras.ru>
> Cc: Wang, Jian J <jian.j.wang@intel.com <mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>;
> Albecki, Mateusz <mateusz.albecki@intel.com <mailto:mateusz.albecki@intel.com>>; Laszlo Ersek
> <lersek@redhat.com <mailto: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 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 SATA 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 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 <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> On Behalf Of Vitaly
> Cheptsov
> Sent: Friday, December 11, 2020 5:25 PM
> To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> Cc: Vitaly Cheptsov <cheptsov@ispras.ru <mailto:cheptsov@ispras.ru>>; Wang, Jian J
> <jian.j.wang@intel.com <mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Albecki,
> Mateusz <mateusz.albecki@intel.com <mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>
> Subject: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe:
> Add
> 
> 
> support for drives in RAID mode
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3118 <https://bugzilla.tianocore.org/show_bug.cgi?id=3118>
> 
> 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 <mailto:jian.j.wang@intel.com>>
> Cc: Hao A Wu <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>
> Cc: Mateusz Albecki <mateusz.albecki@intel.com <mailto:mateusz.albecki@intel.com>>
> Cc: Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>
> Signed-off-by: Vitaly Cheptsov <cheptsov@ispras.ru <mailto: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)) {
> +  if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData) ||
> + IS_PCI_RAID (&PciData)) {
>     return EFI_SUCCESS;
>   }
> 
> @@ -465,7 +465,7 @@ SataControllerStart (
>   if (IS_PCI_IDE (&PciData)) {
>     Private->IdeInit.ChannelCount = IDE_MAX_CHANNEL;
>     Private->DeviceCount          = 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)
> 
> 
> 
> -=-=-=-=-=-=
> Groups.io <http://groups.io/> Links: You receive all messages sent to this group.
> View/Reply Online (#68707):
> https://edk2.groups.io/g/devel/message/68707 <https://edk2.groups.io/g/devel/message/68707>
> Mute This Topic: https://groups.io/mt/78875596/1712937 <https://groups.io/mt/78875596/1712937>
> Group Owner: devel+owner@edk2.groups.io <mailto:devel+owner@edk2.groups.io>
> Unsubscribe: https://edk2.groups.io/g/devel/unsub <https://edk2.groups.io/g/devel/unsub> [ray.ni@intel.com <mailto:ray.ni@intel.com>]
> -=-=-=-=-=-=
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 


[-- Attachment #1.2: Type: text/html, Size: 35796 bytes --]

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode

[Michael D Kinney]

[-- Attachment #1: Type: text/plain, Size: 10678 bytes --]

So 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 could corrupt data.

It is difficult to support a change that could corrupt data.

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>; Albecki, Mateusz <mateusz.albecki@intel.com>; Laszlo Ersek <lersek@redhat.com>
Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: 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 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 дек. 2020 г., в 20:41, Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> написал(а):

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<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Vitaly Cheptsov
Sent: Tuesday, December 15, 2020 9:34 AM
To: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: 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 we start much later (outside of the firmware), we can no longer control this.

Best regards,
Vitaly



15 дек. 2020 г., в 19:58, Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> написал(а):

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<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto: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<mailto:hao.a.wu@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
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=3118 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 the ATTRIBUTES_PHYSICAL & ATTRIBUTES_LOGICAL should be 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 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 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.

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 RAID either.

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

Best regards,
Vitaly

[1] https://github.com/acidanthera/OpenCorePkg





15 дек. 2020 г., в 06:54, Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> написал(а):

I agree it would be better to configure 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, 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<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto: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<mailto:ray.ni@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; cheptsov@ispras.ru<mailto:cheptsov@ispras.ru>
Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto: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<mailto:ray.ni@intel.com>>
Sent: Tuesday, December 15, 2020 9:45 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; cheptsov@ispras.ru<mailto:cheptsov@ispras.ru>
Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>;
Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek
<lersek@redhat.com<mailto: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 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 SATA 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 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<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Vitaly
Cheptsov
Sent: Friday, December 11, 2020 5:25 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Vitaly Cheptsov <cheptsov@ispras.ru<mailto:cheptsov@ispras.ru>>; Wang, Jian J
<jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Albecki,
Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Subject: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe:
Add



support for drives in RAID mode

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

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<mailto:jian.j.wang@intel.com>>
Cc: Hao A Wu <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>
Cc: Mateusz Albecki <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>
Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Signed-off-by: Vitaly Cheptsov <cheptsov@ispras.ru<mailto: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)) {
+  if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData) ||
+ IS_PCI_RAID (&PciData)) {
    return EFI_SUCCESS;
  }

@@ -465,7 +465,7 @@ SataControllerStart (
  if (IS_PCI_IDE (&PciData)) {
    Private->IdeInit.ChannelCount = IDE_MAX_CHANNEL;
    Private->DeviceCount          = 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)



-=-=-=-=-=-=
Groups.io<http://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<mailto:devel+owner@edk2.groups.io>
Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com<mailto:ray.ni@intel.com>]
-=-=-=-=-=-=












[-- Attachment #2: Type: text/html, Size: 70510 bytes --]

Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode

[Vitaly Cheptsov]

[-- Attachment #1: Type: text/plain, Size: 9987 bytes --]

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 <michael.d.kinney@intel.com> wrote:
> 
> 
> So 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 could corrupt data.
>
> It is difficult to support a change that could corrupt data.
>
> 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>; Albecki, Mateusz <mateusz.albecki@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: 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 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 дек. 2020 г., в 20:41, Kinney, Michael D <michael.d.kinney@intel.com> написал(а):
>
> 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 Cheptsov
> Sent: Tuesday, December 15, 2020 9:34 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 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 we start much later (outside of the firmware), we can no longer control this.
>
> Best regards,
> Vitaly
> 
> 
> 
> 15 дек. 2020 г., в 19:58, Kinney, Michael D <michael.d.kinney@intel.com> написал(а):
>
> 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 <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>; 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.kinney@intel.com>
> 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=3118 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 the ATTRIBUTES_PHYSICAL & ATTRIBUTES_LOGICAL should be 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 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 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.
>
> 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 RAID either.
>
> If there are no other review comments besides the attributes, I will proceed with V2 in the coming days.
>
> Best regards,
> Vitaly
>
> [1] https://github.com/acidanthera/OpenCorePkg
>
> 
> 
> 
> 
> 15 дек. 2020 г., в 06:54, Kinney, Michael D <michael.d.kinney@intel.com> написал(а):
>
> I agree it would be better to configure 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, 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.io> 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 <mateusz.albecki@intel.com>; Laszlo 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, 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 <hao.a.wu@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 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 SATA 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 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, 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>; 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.tianocore.org/show_bug.cgi?id=3118
> 
> 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 <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)) {
> +  if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData) ||
> + IS_PCI_RAID (&PciData)) {
>     return EFI_SUCCESS;
>   }
> 
> @@ -465,7 +465,7 @@ SataControllerStart (
>   if (IS_PCI_IDE (&PciData)) {
>     Private->IdeInit.ChannelCount = IDE_MAX_CHANNEL;
>     Private->DeviceCount          = 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)
> 
> 
> 
> -=-=-=-=-=-=
> 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]
> -=-=-=-=-=-=
> 
> 
> 
> 
> 
> 
> 
> 
>
>
>
> 
>

[-- Attachment #2: Type: text/html, Size: 70883 bytes --]

Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode

[Michael D Kinney]

[-- Attachment #1: Type: text/plain, Size: 11923 bytes --]

What about platforms that are in RAID mode and have configured a RAID set.  Your suggested change could potentially corrupt data on those different systems.

Mike

From: Vitaly Cheptsov <cheptsov@ispras.ru>
Sent: Tuesday, December 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 <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 drives in RAID mode

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 <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> wrote:

So 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 could corrupt data.

It is difficult to support a change that could corrupt data.

Mike

From: Vitaly Cheptsov <cheptsov@ispras.ru<mailto:cheptsov@ispras.ru>>
Sent: Tuesday, December 15, 2020 9:44 AM
To: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: 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 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 дек. 2020 г., в 20:41, Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> написал(а):

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<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Vitaly Cheptsov
Sent: Tuesday, December 15, 2020 9:34 AM
To: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: 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 we start much later (outside of the firmware), we can no longer control this.

Best regards,
Vitaly



15 дек. 2020 г., в 19:58, Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> написал(а):

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<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto: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<mailto:hao.a.wu@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
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=3118 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 the ATTRIBUTES_PHYSICAL & ATTRIBUTES_LOGICAL should be 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 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 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.

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 RAID either.

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

Best regards,
Vitaly

[1] https://github.com/acidanthera/OpenCorePkg





15 дек. 2020 г., в 06:54, Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> написал(а):

I agree it would be better to configure 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, 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<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto: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<mailto:ray.ni@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; cheptsov@ispras.ru<mailto:cheptsov@ispras.ru>
Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto: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<mailto:ray.ni@intel.com>>
Sent: Tuesday, December 15, 2020 9:45 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; cheptsov@ispras.ru<mailto:cheptsov@ispras.ru>
Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>;
Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek
<lersek@redhat.com<mailto: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 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 SATA 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 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<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Vitaly
Cheptsov
Sent: Friday, December 11, 2020 5:25 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Vitaly Cheptsov <cheptsov@ispras.ru<mailto:cheptsov@ispras.ru>>; Wang, Jian J
<jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Albecki,
Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Subject: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe:
Add



support for drives in RAID mode

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

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<mailto:jian.j.wang@intel.com>>
Cc: Hao A Wu <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>
Cc: Mateusz Albecki <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>
Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Signed-off-by: Vitaly Cheptsov <cheptsov@ispras.ru<mailto: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)) {
+  if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData) ||
+ IS_PCI_RAID (&PciData)) {
    return EFI_SUCCESS;
  }

@@ -465,7 +465,7 @@ SataControllerStart (
  if (IS_PCI_IDE (&PciData)) {
    Private->IdeInit.ChannelCount = IDE_MAX_CHANNEL;
    Private->DeviceCount          = 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)



-=-=-=-=-=-=
Groups.io<http://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<mailto:devel+owner@edk2.groups.io>
Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com<mailto:ray.ni@intel.com>]
-=-=-=-=-=-=












[-- Attachment #2: Type: text/html, Size: 74165 bytes --]

Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode

[Vitaly Cheptsov]

[-- Attachment #1: Type: text/plain, Size: 10862 bytes --]

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:
> 
> 
> What about platforms that are in RAID mode and have configured a RAID set.  Your suggested change could potentially corrupt data on those different systems.
>
> Mike
>
> From: Vitaly Cheptsov <cheptsov@ispras.ru> 
> Sent: Tuesday, December 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 <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 drives in RAID mode
>
> 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 <michael.d.kinney@intel.com> wrote:
> 
> 
> So 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 could corrupt data.
>
> It is difficult to support a change that could corrupt data.
>
> 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>; Albecki, Mateusz <mateusz.albecki@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: 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 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 дек. 2020 г., в 20:41, Kinney, Michael D <michael.d.kinney@intel.com> написал(а):
>
> 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 Cheptsov
> Sent: Tuesday, December 15, 2020 9:34 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 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 we start much later (outside of the firmware), we can no longer control this.
>
> Best regards,
> Vitaly
> 
> 
> 
> 15 дек. 2020 г., в 19:58, Kinney, Michael D <michael.d.kinney@intel.com> написал(а):
>
> 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 <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>; 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.kinney@intel.com>
> 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=3118 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 the ATTRIBUTES_PHYSICAL & ATTRIBUTES_LOGICAL should be 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 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 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.
>
> 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 RAID either.
>
> If there are no other review comments besides the attributes, I will proceed with V2 in the coming days.
>
> Best regards,
> Vitaly
>
> [1] https://github.com/acidanthera/OpenCorePkg
>
> 
> 
> 
> 
> 15 дек. 2020 г., в 06:54, Kinney, Michael D <michael.d.kinney@intel.com> написал(а):
>
> I agree it would be better to configure 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, 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.io> 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 <mateusz.albecki@intel.com>; Laszlo 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, 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 <hao.a.wu@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 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 SATA 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 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, 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>; 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.tianocore.org/show_bug.cgi?id=3118
> 
> 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 <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)) {
> +  if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData) ||
> + IS_PCI_RAID (&PciData)) {
>     return EFI_SUCCESS;
>   }
> 
> @@ -465,7 +465,7 @@ SataControllerStart (
>   if (IS_PCI_IDE (&PciData)) {
>     Private->IdeInit.ChannelCount = IDE_MAX_CHANNEL;
>     Private->DeviceCount          = 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)
> 
> 
> 
> -=-=-=-=-=-=
> 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]
> -=-=-=-=-=-=
> 
> 
> 
> 
> 
> 
> 
> 
>
>
>
> 
>

[-- Attachment #2: Type: text/html, Size: 74409 bytes --]

Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode

[Michael D Kinney]

[-- Attachment #1: Type: text/plain, Size: 13013 bytes --]

Vitaly,

I am concerned about platforms that use this driver with this change outside your use case.

Mike

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Vitaly Cheptsov
Sent: Tuesday, December 15, 2020 11:40 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 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<mailto:michael.d.kinney@intel.com>> wrote:

What about platforms that are in RAID mode and have configured a RAID set.  Your suggested change could potentially corrupt data on those different systems.

Mike

From: Vitaly Cheptsov <cheptsov@ispras.ru<mailto:cheptsov@ispras.ru>>
Sent: Tuesday, December 15, 2020 10:56 AM
To: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode

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 <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> wrote:

So 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 could corrupt data.

It is difficult to support a change that could corrupt data.

Mike

From: Vitaly Cheptsov <cheptsov@ispras.ru<mailto:cheptsov@ispras.ru>>
Sent: Tuesday, December 15, 2020 9:44 AM
To: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: 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 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 дек. 2020 г., в 20:41, Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> написал(а):

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<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Vitaly Cheptsov
Sent: Tuesday, December 15, 2020 9:34 AM
To: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: 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 we start much later (outside of the firmware), we can no longer control this.

Best regards,
Vitaly



15 дек. 2020 г., в 19:58, Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> написал(а):

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<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto: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<mailto:hao.a.wu@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
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=3118 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 the ATTRIBUTES_PHYSICAL & ATTRIBUTES_LOGICAL should be 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 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 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.

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 RAID either.

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

Best regards,
Vitaly

[1] https://github.com/acidanthera/OpenCorePkg





15 дек. 2020 г., в 06:54, Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> написал(а):

I agree it would be better to configure 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, 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<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto: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<mailto:ray.ni@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; cheptsov@ispras.ru<mailto:cheptsov@ispras.ru>
Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto: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<mailto:ray.ni@intel.com>>
Sent: Tuesday, December 15, 2020 9:45 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; cheptsov@ispras.ru<mailto:cheptsov@ispras.ru>
Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>;
Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek
<lersek@redhat.com<mailto: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 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 SATA 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 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<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Vitaly
Cheptsov
Sent: Friday, December 11, 2020 5:25 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Vitaly Cheptsov <cheptsov@ispras.ru<mailto:cheptsov@ispras.ru>>; Wang, Jian J
<jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Albecki,
Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Subject: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe:
Add



support for drives in RAID mode

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

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<mailto:jian.j.wang@intel.com>>
Cc: Hao A Wu <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>
Cc: Mateusz Albecki <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>
Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Signed-off-by: Vitaly Cheptsov <cheptsov@ispras.ru<mailto: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)) {
+  if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData) ||
+ IS_PCI_RAID (&PciData)) {
    return EFI_SUCCESS;
  }

@@ -465,7 +465,7 @@ SataControllerStart (
  if (IS_PCI_IDE (&PciData)) {
    Private->IdeInit.ChannelCount = IDE_MAX_CHANNEL;
    Private->DeviceCount          = 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)



-=-=-=-=-=-=
Groups.io<http://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<mailto:devel+owner@edk2.groups.io>
Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com<mailto:ray.ni@intel.com>]
-=-=-=-=-=-=












[-- Attachment #2: Type: text/html, Size: 77836 bytes --]

Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode

[Vitaly Cheptsov]

[-- Attachment #1: Type: text/plain, Size: 11696 bytes --]

Mike,

I understand that very well and thus the PCD rather than my original patch :)

Best,
Vitaly

> On 15 Dec 2020, at 22:41, Kinney, Michael D <michael.d.kinney@intel.com> wrote:
> 
> 
> Vitaly,
>
> I am concerned about platforms that use this driver with this change outside your use case.
>
> Mike
>
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Vitaly Cheptsov
> Sent: Tuesday, December 15, 2020 11:40 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 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:
> 
> 
> What about platforms that are in RAID mode and have configured a RAID set.  Your suggested change could potentially corrupt data on those different systems.
>
> Mike
>
> From: Vitaly Cheptsov <cheptsov@ispras.ru> 
> Sent: Tuesday, December 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 <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 drives in RAID mode
>
> 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 <michael.d.kinney@intel.com> wrote:
> 
> 
> So 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 could corrupt data.
>
> It is difficult to support a change that could corrupt data.
>
> 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>; Albecki, Mateusz <mateusz.albecki@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: 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 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 дек. 2020 г., в 20:41, Kinney, Michael D <michael.d.kinney@intel.com> написал(а):
>
> 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 Cheptsov
> Sent: Tuesday, December 15, 2020 9:34 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 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 we start much later (outside of the firmware), we can no longer control this.
>
> Best regards,
> Vitaly
> 
> 
> 
> 15 дек. 2020 г., в 19:58, Kinney, Michael D <michael.d.kinney@intel.com> написал(а):
>
> 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 <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>; 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.kinney@intel.com>
> 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=3118 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 the ATTRIBUTES_PHYSICAL & ATTRIBUTES_LOGICAL should be 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 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 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.
>
> 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 RAID either.
>
> If there are no other review comments besides the attributes, I will proceed with V2 in the coming days.
>
> Best regards,
> Vitaly
>
> [1] https://github.com/acidanthera/OpenCorePkg
>
> 
> 
> 
> 
> 15 дек. 2020 г., в 06:54, Kinney, Michael D <michael.d.kinney@intel.com> написал(а):
>
> I agree it would be better to configure 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, 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.io> 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 <mateusz.albecki@intel.com>; Laszlo 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, 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 <hao.a.wu@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 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 SATA 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 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, 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>; 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.tianocore.org/show_bug.cgi?id=3118
> 
> 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 <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)) {
> +  if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData) ||
> + IS_PCI_RAID (&PciData)) {
>     return EFI_SUCCESS;
>   }
> 
> @@ -465,7 +465,7 @@ SataControllerStart (
>   if (IS_PCI_IDE (&PciData)) {
>     Private->IdeInit.ChannelCount = IDE_MAX_CHANNEL;
>     Private->DeviceCount          = 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)
> 
> 
> 
> -=-=-=-=-=-=
> 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]
> -=-=-=-=-=-=
> 
> 
> 
> 
> 
> 
> 
> 
>
>
>
>
> 

[-- Attachment #2: Type: text/html, Size: 78124 bytes --]

Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode

[Michael D Kinney]

[-- Attachment #1: Type: text/plain, Size: 15248 bytes --]

Another issue with this approach is that the formal PCI definition 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.pdf

04h 00h RAID controller - vendor-specific interface


#define   PCI_CLASS_MASS_STORAGE_IDE    0x01

#define PCI_CLASS_MASS_STORAGE_SATADPA   0x06
#define   PCI_IF_MASS_STORAGE_SATA         0x00
#define   PCI_IF_MASS_STORAGE_AHCI         0x01

#define   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_CLASS_MASS_STORAGE, PCI_CLASS_MASS_STORAGE_SATADPA)
#define IS_PCI_RAID(_p)               IS_CLASS2 (_p, PCI_CLASS_MASS_STORAGE, PCI_CLASS_MASS_STORAGE_RAID)

So the IS_PCI_RAID() macro checks for the RAID class code and the PCI spec states that the interface is vendor specific.  There 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 using 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 registers.

Mike





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 drives in RAID mode

Mike,

I understand that very well and thus the PCD rather than my original patch :)

Best,
Vitaly


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

Vitaly,

I am concerned about platforms that use this driver with this change outside your use case.

Mike

From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Vitaly Cheptsov
Sent: Tuesday, December 15, 2020 11:40 AM
To: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: 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<mailto:michael.d.kinney@intel.com>> wrote:

What about platforms that are in RAID mode and have configured a RAID set.  Your suggested change could potentially corrupt data on those different systems.

Mike

From: Vitaly Cheptsov <cheptsov@ispras.ru<mailto:cheptsov@ispras.ru>>
Sent: Tuesday, December 15, 2020 10:56 AM
To: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode

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 <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> wrote:

So 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 could corrupt data.

It is difficult to support a change that could corrupt data.

Mike

From: Vitaly Cheptsov <cheptsov@ispras.ru<mailto:cheptsov@ispras.ru>>
Sent: Tuesday, December 15, 2020 9:44 AM
To: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: 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 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 дек. 2020 г., в 20:41, Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> написал(а):

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<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Vitaly Cheptsov
Sent: Tuesday, December 15, 2020 9:34 AM
To: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: 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 we start much later (outside of the firmware), we can no longer control this.

Best regards,
Vitaly



15 дек. 2020 г., в 19:58, Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> написал(а):

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<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto: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<mailto:hao.a.wu@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
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=3118 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 the ATTRIBUTES_PHYSICAL & ATTRIBUTES_LOGICAL should be 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 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 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.

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 RAID either.

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

Best regards,
Vitaly

[1] https://github.com/acidanthera/OpenCorePkg





15 дек. 2020 г., в 06:54, Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> написал(а):

I agree it would be better to configure 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, 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<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto: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<mailto:ray.ni@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; cheptsov@ispras.ru<mailto:cheptsov@ispras.ru>
Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto: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<mailto:ray.ni@intel.com>>
Sent: Tuesday, December 15, 2020 9:45 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; cheptsov@ispras.ru<mailto:cheptsov@ispras.ru>
Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>;
Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek
<lersek@redhat.com<mailto: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 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 SATA 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 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<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Vitaly
Cheptsov
Sent: Friday, December 11, 2020 5:25 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Vitaly Cheptsov <cheptsov@ispras.ru<mailto:cheptsov@ispras.ru>>; Wang, Jian J
<jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Albecki,
Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Subject: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe:
Add



support for drives in RAID mode

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

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<mailto:jian.j.wang@intel.com>>
Cc: Hao A Wu <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>
Cc: Mateusz Albecki <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>
Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Signed-off-by: Vitaly Cheptsov <cheptsov@ispras.ru<mailto: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)) {
+  if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData) ||
+ IS_PCI_RAID (&PciData)) {
    return EFI_SUCCESS;
  }

@@ -465,7 +465,7 @@ SataControllerStart (
  if (IS_PCI_IDE (&PciData)) {
    Private->IdeInit.ChannelCount = IDE_MAX_CHANNEL;
    Private->DeviceCount          = 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)



-=-=-=-=-=-=
Groups.io<http://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<mailto:devel+owner@edk2.groups.io>
Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com<mailto:ray.ni@intel.com>]
-=-=-=-=-=-=












[-- Attachment #2: Type: text/html, Size: 90370 bytes --]

Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode

[Vitaly Cheptsov]

[-- Attachment #1.1: Type: text/plain, Size: 17277 bytes --]

Mike,

That’s right. And due to that interface 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 дек. 2020 г., в 23:01, Kinney, Michael D <michael.d.kinney@intel.com> написал(а):
> 
> Another issue with this approach is that the formal PCI definition 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.pdf <https://pcisig.com/sites/default/files/files/PCI_Code-ID_r_1_11__v24_Jan_2019.pdf>
> 
> 04h 00h RAID controller - vendor-specific interface
> 
> 
> #define   PCI_CLASS_MASS_STORAGE_IDE    0x01
> 
> #define PCI_CLASS_MASS_STORAGE_SATADPA   0x06
> #define   PCI_IF_MASS_STORAGE_SATA         0x00
> #define   PCI_IF_MASS_STORAGE_AHCI         0x01
> 
> #define   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_CLASS_MASS_STORAGE, PCI_CLASS_MASS_STORAGE_SATADPA)
> #define IS_PCI_RAID(_p)               IS_CLASS2 (_p, PCI_CLASS_MASS_STORAGE, PCI_CLASS_MASS_STORAGE_RAID)
> 
> So the IS_PCI_RAID() macro checks for the RAID class code and the PCI spec states that the interface is vendor specific.  There 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 using 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 registers.
> 
> Mike
> 
> 
> 
> 
> 
> From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto: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 <mailto:michael.d.kinney@intel.com>>
> Cc: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Wu, Hao A <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Wang, Jian J <jian.j.wang@intel.com <mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com <mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>
> Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode
> 
> Mike,
> 
> I understand that very well and thus the PCD rather than my original patch :)
> 
> Best,
> Vitaly
> 
> 
> On 15 Dec 2020, at 22:41, Kinney, Michael D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>> wrote:
> 
> 
> Vitaly,
> 
> I am concerned about platforms that use this driver with this change outside your use case.
> 
> Mike
> 
> From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> On Behalf Of Vitaly Cheptsov
> Sent: Tuesday, December 15, 2020 11:40 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>
> Cc: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Wu, Hao A <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Wang, Jian J <jian.j.wang@intel.com <mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com <mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>
> Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: 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 <mailto:michael.d.kinney@intel.com>> wrote:
> 
> 
> What about platforms that are in RAID mode and have configured a RAID set.  Your suggested change could potentially corrupt data on those different systems.
> 
> Mike
> 
> From: Vitaly Cheptsov <cheptsov@ispras.ru <mailto:cheptsov@ispras.ru>>
> Sent: Tuesday, December 15, 2020 10:56 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>
> Cc: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Wu, Hao A <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Wang, Jian J <jian.j.wang@intel.com <mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com <mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>
> Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode
> 
> 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 <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>> wrote:
> 
> 
> So 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 could corrupt data.
> 
> It is difficult to support a change that could corrupt data.
> 
> Mike
> 
> From: Vitaly Cheptsov <cheptsov@ispras.ru <mailto:cheptsov@ispras.ru>>
> Sent: Tuesday, December 15, 2020 9:44 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>
> Cc: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Wu, Hao A <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Wang, Jian J <jian.j.wang@intel.com <mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com <mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>
> Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: 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 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 дек. 2020 г., в 20:41, Kinney, Michael D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>> написал(а):
> 
> 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 <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> On Behalf Of Vitaly Cheptsov
> Sent: Tuesday, December 15, 2020 9:34 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>
> Cc: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Wu, Hao A <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Wang, Jian J <jian.j.wang@intel.com <mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com <mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>
> Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: 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 we start much later (outside of the firmware), we can no longer control this.
> 
> Best regards,
> Vitaly
> 
> 
> 
> 15 дек. 2020 г., в 19:58, Kinney, Michael D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>> написал(а):
> 
> 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 <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto: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 <mailto:hao.a.wu@intel.com>>; devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> Cc: Ni, Ray <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Wang, Jian J <jian.j.wang@intel.com <mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com <mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>; Kinney, Michael D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>
> 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=3118 <https://bugzilla.tianocore.org/show_bug.cgi?id=3118> 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 the ATTRIBUTES_PHYSICAL & ATTRIBUTES_LOGICAL should be 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 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 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.
> 
> 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 RAID either.
> 
> If there are no other review comments besides the attributes, I will proceed with V2 in the coming days.
> 
> Best regards,
> Vitaly
> 
> [1] https://github.com/acidanthera/OpenCorePkg <https://github.com/acidanthera/OpenCorePkg>
> 
> 
> 
> 
> 
> 15 дек. 2020 г., в 06:54, Kinney, Michael D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>> написал(а):
> 
> I agree it would be better to configure 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, 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 <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto: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 <mailto:ray.ni@intel.com>>; devel@edk2.groups.io <mailto:devel@edk2.groups.io>; cheptsov@ispras.ru <mailto:cheptsov@ispras.ru>
> Cc: Wang, Jian J <jian.j.wang@intel.com <mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com <mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com <mailto: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 <mailto:ray.ni@intel.com>>
> Sent: Tuesday, December 15, 2020 9:45 AM
> To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; cheptsov@ispras.ru <mailto:cheptsov@ispras.ru>
> Cc: Wang, Jian J <jian.j.wang@intel.com <mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>;
> Albecki, Mateusz <mateusz.albecki@intel.com <mailto:mateusz.albecki@intel.com>>; Laszlo Ersek
> <lersek@redhat.com <mailto: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 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 SATA 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 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 <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> On Behalf Of Vitaly
> Cheptsov
> Sent: Friday, December 11, 2020 5:25 PM
> To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> Cc: Vitaly Cheptsov <cheptsov@ispras.ru <mailto:cheptsov@ispras.ru>>; Wang, Jian J
> <jian.j.wang@intel.com <mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Albecki,
> Mateusz <mateusz.albecki@intel.com <mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>
> Subject: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe:
> Add
> 
> 
> 
> support for drives in RAID mode
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3118 <https://bugzilla.tianocore.org/show_bug.cgi?id=3118>
> 
> 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 <mailto:jian.j.wang@intel.com>>
> Cc: Hao A Wu <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>
> Cc: Mateusz Albecki <mateusz.albecki@intel.com <mailto:mateusz.albecki@intel.com>>
> Cc: Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>
> Signed-off-by: Vitaly Cheptsov <cheptsov@ispras.ru <mailto: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)) {
> +  if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData) ||
> + IS_PCI_RAID (&PciData)) {
>     return EFI_SUCCESS;
>   }
> 
> @@ -465,7 +465,7 @@ SataControllerStart (
>   if (IS_PCI_IDE (&PciData)) {
>     Private->IdeInit.ChannelCount = IDE_MAX_CHANNEL;
>     Private->DeviceCount          = 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)
> 
> 
> 
> -=-=-=-=-=-=
> Groups.io <http://groups.io/> Links: You receive all messages sent to this group.
> View/Reply Online (#68707):
> https://edk2.groups.io/g/devel/message/68707 <https://edk2.groups.io/g/devel/message/68707>
> Mute This Topic: https://groups.io/mt/78875596/1712937 <https://groups.io/mt/78875596/1712937>
> Group Owner: devel+owner@edk2.groups.io <mailto:devel+owner@edk2.groups.io>
> Unsubscribe: https://edk2.groups.io/g/devel/unsub <https://edk2.groups.io/g/devel/unsub> [ray.ni@intel.com <mailto:ray.ni@intel.com>]
> -=-=-=-=-=-=
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 


[-- Attachment #1.2: Type: text/html, Size: 66663 bytes --]

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode

[Albecki, Mateusz]

[-- Attachment #1: Type: text/plain, Size: 19488 bytes --]

Hello,

Sorry for getting in so late on the conversation but I want to make sure we are not going to break some platforms with this change. Excuse me if I am simply repeating what was being said before.


  1.  We can’t have SataControllerDxe report supported for all raid interface devices on the market. As far as I know Intel RAID is rather special in that it implements AHCI interface underneath and if we encounter a RAID controller like that we will falsely claim that we support it. It’s hard to say whether this will have any consequences for the data integrity on the RAID array but at the very least we will prevent appropriate driver from binding to this RAID controller(potentially unbootable system).
  2.  On newer Intel systems(not sure about G33) we have a dedicated RAID driver(RST driver) which will bind to integrated SATA in RAID mode. On such systems we have both RST driver and traditional SATA stack(SataControllerDxe, AtaAtapiPassThru etc) to allow runtime change between AHCI and RAID modes. If SataControllerDxe starts claiming SATA controller it will potentially make the system unbootable in RAID mode(if RAID is configured). What is more on those systems going through RAID config with volume managed by RST to standard AHCI config will corrupt RAID volumes. I can’t go into details on why is that but it has to do with how RST configures GPT partition.

In summary I don’t think we can add simple checks for RAID mode. Even adding a build flag could be potentially problematic since on desktop systems you never know what kind of controller user will plug in their slots. One potential solution would be to keep a list of controllers which do support AHCI interface even though they are reporting RAID class code but even then the solution could potentially break systems with both RST and AHCI stack present. Maybe we should have a platform provided protocol that would say whether AHCI drivers should load on such RAID controller?

Thanks,
Mateusz
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Vitaly Cheptsov
Sent: Wednesday, December 16, 2020 10:31 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 drives in RAID mode

Mike,

That’s right. And due to that interface 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 дек. 2020 г., в 23:01, Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> написал(а):

Another issue with this approach is that the formal PCI definition 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.pdf

04h 00h RAID controller - vendor-specific interface


#define   PCI_CLASS_MASS_STORAGE_IDE    0x01

#define PCI_CLASS_MASS_STORAGE_SATADPA   0x06
#define   PCI_IF_MASS_STORAGE_SATA         0x00
#define   PCI_IF_MASS_STORAGE_AHCI         0x01

#define   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_CLASS_MASS_STORAGE, PCI_CLASS_MASS_STORAGE_SATADPA)
#define IS_PCI_RAID(_p)               IS_CLASS2 (_p, PCI_CLASS_MASS_STORAGE, PCI_CLASS_MASS_STORAGE_RAID)

So the IS_PCI_RAID() macro checks for the RAID class code and the PCI spec states that the interface is vendor specific.  There 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 using 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 registers.

Mike





From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto: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<mailto:michael.d.kinney@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode

Mike,

I understand that very well and thus the PCD rather than my original patch :)

Best,
Vitaly


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

Vitaly,

I am concerned about platforms that use this driver with this change outside your use case.

Mike

From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Vitaly Cheptsov
Sent: Tuesday, December 15, 2020 11:40 AM
To: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: 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<mailto:michael.d.kinney@intel.com>> wrote:

What about platforms that are in RAID mode and have configured a RAID set.  Your suggested change could potentially corrupt data on those different systems.

Mike

From: Vitaly Cheptsov <cheptsov@ispras.ru<mailto:cheptsov@ispras.ru>>
Sent: Tuesday, December 15, 2020 10:56 AM
To: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode

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 <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> wrote:

So 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 could corrupt data.

It is difficult to support a change that could corrupt data.

Mike

From: Vitaly Cheptsov <cheptsov@ispras.ru<mailto:cheptsov@ispras.ru>>
Sent: Tuesday, December 15, 2020 9:44 AM
To: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: 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 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 дек. 2020 г., в 20:41, Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> написал(а):

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<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Vitaly Cheptsov
Sent: Tuesday, December 15, 2020 9:34 AM
To: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: 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 we start much later (outside of the firmware), we can no longer control this.

Best regards,
Vitaly



15 дек. 2020 г., в 19:58, Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> написал(а):

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<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto: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<mailto:hao.a.wu@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
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=3118 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 the ATTRIBUTES_PHYSICAL & ATTRIBUTES_LOGICAL should be 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 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 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.

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 RAID either.

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

Best regards,
Vitaly

[1] https://github.com/acidanthera/OpenCorePkg





15 дек. 2020 г., в 06:54, Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> написал(а):

I agree it would be better to configure 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, 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<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto: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<mailto:ray.ni@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; cheptsov@ispras.ru<mailto:cheptsov@ispras.ru>
Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto: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<mailto:ray.ni@intel.com>>
Sent: Tuesday, December 15, 2020 9:45 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; cheptsov@ispras.ru<mailto:cheptsov@ispras.ru>
Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>;
Albecki, Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek
<lersek@redhat.com<mailto: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 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 SATA 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 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<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Vitaly
Cheptsov
Sent: Friday, December 11, 2020 5:25 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Vitaly Cheptsov <cheptsov@ispras.ru<mailto:cheptsov@ispras.ru>>; Wang, Jian J
<jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Albecki,
Mateusz <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Subject: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe:
Add



support for drives in RAID mode

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

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<mailto:jian.j.wang@intel.com>>
Cc: Hao A Wu <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>
Cc: Mateusz Albecki <mateusz.albecki@intel.com<mailto:mateusz.albecki@intel.com>>
Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Signed-off-by: Vitaly Cheptsov <cheptsov@ispras.ru<mailto: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)) {
+  if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData) ||
+ IS_PCI_RAID (&PciData)) {
    return EFI_SUCCESS;
  }

@@ -465,7 +465,7 @@ SataControllerStart (
  if (IS_PCI_IDE (&PciData)) {
    Private->IdeInit.ChannelCount = IDE_MAX_CHANNEL;
    Private->DeviceCount          = 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)



-=-=-=-=-=-=
Groups.io<http://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<mailto:devel+owner@edk2.groups.io>
Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com<mailto:ray.ni@intel.com>]
-=-=-=-=-=-=












---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Sowackiego 173 | 80-298 Gdask | Sd Rejonowy Gdask Pnoc | VII Wydzia Gospodarczy Krajowego Rejestru Sdowego - KRS 101882 | NIP 957-07-52-316 | Kapita zakadowy 200.000 PLN.
Ta wiadomo wraz z zacznikami jest przeznaczona dla okrelonego adresata i moe zawiera informacje poufne. W razie przypadkowego otrzymania tej wiadomoci, prosimy o powiadomienie nadawcy oraz trwae jej usunicie; jakiekolwiek przegldanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.
 

[-- Attachment #2: Type: text/html, Size: 51746 bytes --]