[edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode
Michael D Kinney
michael.d.kinney at intel.com
Tue Dec 15 20:01:58 UTC 2020
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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Vitaly Cheptsov
Sent: Tuesday, December 15, 2020 11:47 AM
To: Kinney, Michael D <michael.d.kinney at intel.com>
Cc: devel at edk2.groups.io; Wu, Hao A <hao.a.wu at intel.com>; Ni, Ray <ray.ni at intel.com>; Wang, Jian J <jian.j.wang at intel.com>; Albecki, Mateusz <mateusz.albecki at intel.com>; Laszlo Ersek <lersek at 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 at intel.com<mailto:michael.d.kinney at intel.com>> wrote:
Vitaly,
I am concerned about platforms that use this driver with this change outside your use case.
Mike
From: devel at edk2.groups.io<mailto:devel at edk2.groups.io> <devel at edk2.groups.io<mailto:devel at edk2.groups.io>> On Behalf Of Vitaly Cheptsov
Sent: Tuesday, December 15, 2020 11:40 AM
To: Kinney, Michael D <michael.d.kinney at intel.com<mailto:michael.d.kinney at intel.com>>
Cc: devel at edk2.groups.io<mailto:devel at edk2.groups.io>; Wu, Hao A <hao.a.wu at intel.com<mailto:hao.a.wu at intel.com>>; Ni, Ray <ray.ni at intel.com<mailto:ray.ni at intel.com>>; Wang, Jian J <jian.j.wang at intel.com<mailto:jian.j.wang at intel.com>>; Albecki, Mateusz <mateusz.albecki at intel.com<mailto:mateusz.albecki at intel.com>>; Laszlo Ersek <lersek at redhat.com<mailto:lersek at 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 at intel.com<mailto:michael.d.kinney at 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 at ispras.ru<mailto:cheptsov at ispras.ru>>
Sent: Tuesday, December 15, 2020 10:56 AM
To: Kinney, Michael D <michael.d.kinney at intel.com<mailto:michael.d.kinney at intel.com>>
Cc: devel at edk2.groups.io<mailto:devel at edk2.groups.io>; Wu, Hao A <hao.a.wu at intel.com<mailto:hao.a.wu at intel.com>>; Ni, Ray <ray.ni at intel.com<mailto:ray.ni at intel.com>>; Wang, Jian J <jian.j.wang at intel.com<mailto:jian.j.wang at intel.com>>; Albecki, Mateusz <mateusz.albecki at intel.com<mailto:mateusz.albecki at intel.com>>; Laszlo Ersek <lersek at redhat.com<mailto:lersek at 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 at intel.com<mailto:michael.d.kinney at 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 at ispras.ru<mailto:cheptsov at ispras.ru>>
Sent: Tuesday, December 15, 2020 9:44 AM
To: Kinney, Michael D <michael.d.kinney at intel.com<mailto:michael.d.kinney at intel.com>>
Cc: devel at edk2.groups.io<mailto:devel at edk2.groups.io>; Wu, Hao A <hao.a.wu at intel.com<mailto:hao.a.wu at intel.com>>; Ni, Ray <ray.ni at intel.com<mailto:ray.ni at intel.com>>; Wang, Jian J <jian.j.wang at intel.com<mailto:jian.j.wang at intel.com>>; Albecki, Mateusz <mateusz.albecki at intel.com<mailto:mateusz.albecki at intel.com>>; Laszlo Ersek <lersek at redhat.com<mailto:lersek at 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 at intel.com<mailto:michael.d.kinney at 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 at edk2.groups.io<mailto:devel at edk2.groups.io> <devel at edk2.groups.io<mailto:devel at edk2.groups.io>> On Behalf Of Vitaly Cheptsov
Sent: Tuesday, December 15, 2020 9:34 AM
To: Kinney, Michael D <michael.d.kinney at intel.com<mailto:michael.d.kinney at intel.com>>
Cc: devel at edk2.groups.io<mailto:devel at edk2.groups.io>; Wu, Hao A <hao.a.wu at intel.com<mailto:hao.a.wu at intel.com>>; Ni, Ray <ray.ni at intel.com<mailto:ray.ni at intel.com>>; Wang, Jian J <jian.j.wang at intel.com<mailto:jian.j.wang at intel.com>>; Albecki, Mateusz <mateusz.albecki at intel.com<mailto:mateusz.albecki at intel.com>>; Laszlo Ersek <lersek at redhat.com<mailto:lersek at 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 at intel.com<mailto:michael.d.kinney at intel.com>> написал(а):
Hi Vitaly,
Can you please explain why the controller can not be configured in a non-RAID mode?
Thanks,
Mike
From: devel at edk2.groups.io<mailto:devel at edk2.groups.io> <devel at edk2.groups.io<mailto:devel at edk2.groups.io>> On Behalf Of Vitaly Cheptsov
Sent: Tuesday, December 15, 2020 12:58 AM
To: Wu, Hao A <hao.a.wu at intel.com<mailto:hao.a.wu at intel.com>>; devel at edk2.groups.io<mailto:devel at edk2.groups.io>
Cc: Ni, Ray <ray.ni at intel.com<mailto:ray.ni at intel.com>>; Wang, Jian J <jian.j.wang at intel.com<mailto:jian.j.wang at intel.com>>; Albecki, Mateusz <mateusz.albecki at intel.com<mailto:mateusz.albecki at intel.com>>; Laszlo Ersek <lersek at redhat.com<mailto:lersek at redhat.com>>; Kinney, Michael D <michael.d.kinney at intel.com<mailto:michael.d.kinney at 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 at intel.com<mailto:michael.d.kinney at 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 at edk2.groups.io<mailto:devel at edk2.groups.io> <devel at edk2.groups.io<mailto:devel at edk2.groups.io>> On Behalf Of Wu, Hao A
Sent: Monday, December 14, 2020 5:53 PM
To: Ni, Ray <ray.ni at intel.com<mailto:ray.ni at intel.com>>; devel at edk2.groups.io<mailto:devel at edk2.groups.io>; cheptsov at ispras.ru<mailto:cheptsov at ispras.ru>
Cc: Wang, Jian J <jian.j.wang at intel.com<mailto:jian.j.wang at intel.com>>; Albecki, Mateusz <mateusz.albecki at intel.com<mailto:mateusz.albecki at intel.com>>; Laszlo Ersek <lersek at redhat.com<mailto:lersek at 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 at intel.com<mailto:ray.ni at intel.com>>
Sent: Tuesday, December 15, 2020 9:45 AM
To: devel at edk2.groups.io<mailto:devel at edk2.groups.io>; cheptsov at ispras.ru<mailto:cheptsov at ispras.ru>
Cc: Wang, Jian J <jian.j.wang at intel.com<mailto:jian.j.wang at intel.com>>; Wu, Hao A <hao.a.wu at intel.com<mailto:hao.a.wu at intel.com>>;
Albecki, Mateusz <mateusz.albecki at intel.com<mailto:mateusz.albecki at intel.com>>; Laszlo Ersek
<lersek at redhat.com<mailto:lersek at 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 at edk2.groups.io<mailto:devel at edk2.groups.io> <devel at edk2.groups.io<mailto:devel at edk2.groups.io>> On Behalf Of Vitaly
Cheptsov
Sent: Friday, December 11, 2020 5:25 PM
To: devel at edk2.groups.io<mailto:devel at edk2.groups.io>
Cc: Vitaly Cheptsov <cheptsov at ispras.ru<mailto:cheptsov at ispras.ru>>; Wang, Jian J
<jian.j.wang at intel.com<mailto:jian.j.wang at intel.com>>; Wu, Hao A <hao.a.wu at intel.com<mailto:hao.a.wu at intel.com>>; Albecki,
Mateusz <mateusz.albecki at intel.com<mailto:mateusz.albecki at intel.com>>; Laszlo Ersek <lersek at redhat.com<mailto:lersek at 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 at intel.com<mailto:jian.j.wang at intel.com>>
Cc: Hao A Wu <hao.a.wu at intel.com<mailto:hao.a.wu at intel.com>>
Cc: Mateusz Albecki <mateusz.albecki at intel.com<mailto:mateusz.albecki at intel.com>>
Cc: Laszlo Ersek <lersek at redhat.com<mailto:lersek at redhat.com>>
Signed-off-by: Vitaly Cheptsov <cheptsov at ispras.ru<mailto:cheptsov at 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 at edk2.groups.io<mailto:devel+owner at edk2.groups.io>
Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni at intel.com<mailto:ray.ni at intel.com>]
-=-=-=-=-=-=
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#68897): https://edk2.groups.io/g/devel/message/68897
Mute This Topic: https://groups.io/mt/78875596/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20201215/7e207647/attachment.htm>
More information about the edk2-devel-archive
mailing list