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

["Wu, Hao A" <hao.a.wu@intel.com>]

(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)
> >>>
> >>
> >>
> >> 
> >>
> >