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

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

[-- 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 --]