|
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3118This 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@...> Cc: Hao A Wu <hao.a.wu@...> Cc: Mateusz Albecki <mateusz.albecki@...> Cc: Laszlo Ersek <lersek@...> Signed-off-by: Vitaly Cheptsov <cheptsov@...> --- MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c b/Md= eModulePkg/Bus/Pci/SataControllerDxe/SataController.c index ab06e2833c..301335c967 100644 --- a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c +++ b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c @@ -324,7 +324,7 @@ SataControllerSupported ( return EFI_UNSUPPORTED; } =20 - if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData)) { + if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData) || IS_PCI_RAID = (&PciData)) { return EFI_SUCCESS; } =20 @@ -465,7 +465,7 @@ SataControllerStart ( if (IS_PCI_IDE (&PciData)) { Private->IdeInit.ChannelCount =3D IDE_MAX_CHANNEL; Private->DeviceCount =3D IDE_MAX_DEVICES; - } else if (IS_PCI_SATADPA (&PciData)) { + } else if (IS_PCI_SATADPA (&PciData) || IS_PCI_RAID (&PciData)) { // // Read Ports Implemented(PI) to calculate max port number (0 based)= . // --=20 2.24.3 (Apple Git-128)
|
|
|
toggle quoted messageShow quoted text
-----Original Message-----
From: Vitaly Cheptsov <cheptsov@...>
Sent: Friday, December 11, 2020 5:25 PM
To: devel@edk2.groups.io
Cc: Vitaly Cheptsov <cheptsov@...>; Wang, Jian J
<jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>; Albecki,
Mateusz <mateusz.albecki@...>; Laszlo Ersek <lersek@...>
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@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Mateusz Albecki <mateusz.albecki@...>
Cc: Laszlo Ersek <lersek@...>
Signed-off-by: Vitaly Cheptsov <cheptsov@...>
---
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)
|
|
|
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
toggle quoted messageShow quoted text
On 14 Dec 2020, at 09:22, Wu, Hao A <hao.a.wu@...> wrote:
-----Original Message-----
From: Vitaly Cheptsov <cheptsov@...>
Sent: Friday, December 11, 2020 5:25 PM
To: devel@edk2.groups.io
Cc: Vitaly Cheptsov <cheptsov@...>; Wang, Jian J
<jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>; Albecki,
Mateusz <mateusz.albecki@...>; Laszlo Ersek <lersek@...>
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@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Mateusz Albecki <mateusz.albecki@...>
Cc: Laszlo Ersek <lersek@...>
Signed-off-by: Vitaly Cheptsov <cheptsov@...>
---
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)
|
|
|
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
toggle quoted messageShow quoted text
-----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@...>
Cc: devel@edk2.groups.io; Wang, Jian J <jian.j.wang@...>; Albecki,
Mateusz <mateusz.albecki@...>; Laszlo Ersek <lersek@...>
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@...> wrote:
-----Original Message-----
From: Vitaly Cheptsov <cheptsov@...>
Sent: Friday, December 11, 2020 5:25 PM
To: devel@edk2.groups.io
Cc: Vitaly Cheptsov <cheptsov@...>; Wang, Jian J
<jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>; Albecki,
Mateusz <mateusz.albecki@...>; Laszlo Ersek <lersek@...>
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@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Mateusz Albecki <mateusz.albecki@...>
Cc: Laszlo Ersek <lersek@...>
Signed-off-by: Vitaly Cheptsov <cheptsov@...>
---
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)
|
|
|
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
toggle quoted messageShow quoted text
On 14 Dec 2020, at 10:56, Wu, Hao A <hao.a.wu@...> 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@...>
Cc: devel@edk2.groups.io; Wang, Jian J <jian.j.wang@...>; Albecki,
Mateusz <mateusz.albecki@...>; Laszlo Ersek <lersek@...>
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@...> wrote:
-----Original Message-----
From: Vitaly Cheptsov <cheptsov@...>
Sent: Friday, December 11, 2020 5:25 PM
To: devel@edk2.groups.io
Cc: Vitaly Cheptsov <cheptsov@...>; Wang, Jian J
<jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>; Albecki,
Mateusz <mateusz.albecki@...>; Laszlo Ersek <lersek@...>
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@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Mateusz Albecki <mateusz.albecki@...>
Cc: Laszlo Ersek <lersek@...>
Signed-off-by: Vitaly Cheptsov <cheptsov@...>
---
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)
|
|
|
(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@...>
Sent: Monday, December 14, 2020 4:29 PM
To: Wu, Hao A <hao.a.wu@...>
Cc: devel@edk2.groups.io; Wang, Jian J <jian.j.wang@...>; Albecki,
Mateusz <mateusz.albecki@...>; Laszlo Ersek <lersek@...>
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@...> 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@...>
Cc: devel@edk2.groups.io; Wang, Jian J <jian.j.wang@...>;
Albecki, Mateusz <mateusz.albecki@...>; Laszlo Ersek
<lersek@...>
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@...> wrote:
-----Original Message-----
From: Vitaly Cheptsov <cheptsov@...>
Sent: Friday, December 11, 2020 5:25 PM
To: devel@edk2.groups.io
Cc: Vitaly Cheptsov <cheptsov@...>; Wang, Jian J
<jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>; Albecki,
Mateusz <mateusz.albecki@...>; Laszlo Ersek
<lersek@...>
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@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Mateusz Albecki <mateusz.albecki@...>
Cc: Laszlo Ersek <lersek@...>
Signed-off-by: Vitaly Cheptsov <cheptsov@...>
---
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)
|
|
|
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
toggle quoted messageShow quoted text
-----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@...>; Wang, Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>;
Albecki, Mateusz <mateusz.albecki@...>; Laszlo Ersek <lersek@...>
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@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Mateusz Albecki <mateusz.albecki@...>
Cc: Laszlo Ersek <lersek@...>
Signed-off-by: Vitaly Cheptsov <cheptsov@...>
---
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@...]
-=-=-=-=-=-=
|
|
|
toggle quoted messageShow quoted text
-----Original Message-----
From: Ni, Ray <ray.ni@...>
Sent: Tuesday, December 15, 2020 9:45 AM
To: devel@edk2.groups.io; cheptsov@...
Cc: Wang, Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>;
Albecki, Mateusz <mateusz.albecki@...>; Laszlo Ersek
<lersek@...>
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@...>; Wang, Jian J
<jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>; Albecki,
Mateusz <mateusz.albecki@...>; Laszlo Ersek <lersek@...>
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@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Mateusz Albecki <mateusz.albecki@...>
Cc: Laszlo Ersek <lersek@...>
Signed-off-by: Vitaly Cheptsov <cheptsov@...>
---
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@...]
-=-=-=-=-=-=
|
|
|
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
toggle quoted messageShow quoted text
-----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@...>; devel@edk2.groups.io; cheptsov@...
Cc: Wang, Jian J <jian.j.wang@...>; Albecki, Mateusz <mateusz.albecki@...>; Laszlo Ersek <lersek@...>
Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode
-----Original Message-----
From: Ni, Ray <ray.ni@...>
Sent: Tuesday, December 15, 2020 9:45 AM
To: devel@edk2.groups.io; cheptsov@...
Cc: Wang, Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>;
Albecki, Mateusz <mateusz.albecki@...>; Laszlo Ersek
<lersek@...>
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@...>; Wang, Jian J
<jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>; Albecki,
Mateusz <mateusz.albecki@...>; Laszlo Ersek <lersek@...>
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@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Mateusz Albecki <mateusz.albecki@...>
Cc: Laszlo Ersek <lersek@...>
Signed-off-by: Vitaly Cheptsov <cheptsov@...>
---
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@...]
-=-=-=-=-=-=
|
|
|
Hello,
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
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@...>; devel@edk2.groups.io; cheptsov@...
Cc: Wang, Jian J <jian.j.wang@...>; Albecki, Mateusz <mateusz.albecki@...>; Laszlo Ersek <lersek@...>
Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode
-----Original Message-----
From: Ni, Ray <ray.ni@...>
Sent: Tuesday, December 15, 2020 9:45 AM
To: devel@edk2.groups.io; cheptsov@...
Cc: Wang, Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>;
Albecki, Mateusz <mateusz.albecki@...>; Laszlo Ersek
<lersek@...>
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@...>; Wang, Jian J
<jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>; Albecki,
Mateusz <mateusz.albecki@...>; Laszlo Ersek <lersek@...>
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@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Mateusz Albecki <mateusz.albecki@...>
Cc: Laszlo Ersek <lersek@...>
Signed-off-by: Vitaly Cheptsov <cheptsov@...>
---
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@...]
-=-=-=-=-=-=
|
|
|
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.
From: Vitaly Cheptsov <cheptsov@...>
Sent: Tuesday, December 15, 2020 4:58 PM
To: Wu, Hao A <hao.a.wu@...>; devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@...>; Wang, Jian J <jian.j.wang@...>; Albecki, Mateusz <mateusz.albecki@...>; Laszlo Ersek <lersek@...>; Kinney, Michael D <michael.d.kinney@...>
Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode
Hello,
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.
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@...>;
devel@edk2.groups.io; cheptsov@...
Cc: Wang, Jian J <jian.j.wang@...>; Albecki, Mateusz <mateusz.albecki@...>; Laszlo Ersek <lersek@...>
Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode
-----Original Message-----
From: Ni, Ray <ray.ni@...>
Sent: Tuesday, December 15, 2020 9:45 AM
To: devel@edk2.groups.io;
cheptsov@...
Cc: Wang, Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>;
Albecki, Mateusz <mateusz.albecki@...>; Laszlo Ersek
<lersek@...>
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@...>; Wang, Jian J
<jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>; Albecki,
Mateusz <mateusz.albecki@...>; Laszlo Ersek <lersek@...>
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@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Mateusz Albecki <mateusz.albecki@...>
Cc: Laszlo Ersek <lersek@...>
Signed-off-by: Vitaly Cheptsov <cheptsov@...>
---
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@...]
-=-=-=-=-=-=
|
|
|
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@...>; devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@...>; Wang, Jian J <jian.j.wang@...>; Albecki, Mateusz <mateusz.albecki@...>; Laszlo Ersek <lersek@...>; Kinney, Michael D <michael.d.kinney@...>
Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode
Hello,
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.
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@...>;
devel@edk2.groups.io; cheptsov@...
Cc: Wang, Jian J <jian.j.wang@...>; Albecki, Mateusz <mateusz.albecki@...>; Laszlo Ersek <lersek@...>
Subject: Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Add support for drives in RAID mode
-----Original Message-----
From: Ni, Ray <ray.ni@...>
Sent: Tuesday, December 15, 2020 9:45 AM
To: devel@edk2.groups.io;
cheptsov@...
Cc: Wang, Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>;
Albecki, Mateusz <mateusz.albecki@...>; Laszlo Ersek
<lersek@...>
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@...>; Wang, Jian J
<jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>; Albecki,
Mateusz <mateusz.albecki@...>; Laszlo Ersek <lersek@...>
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@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Mateusz Albecki <mateusz.albecki@...>
Cc: Laszlo Ersek <lersek@...>
Signed-off-by: Vitaly Cheptsov <cheptsov@...>
---
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@...]
-=-=-=-=-=-=
|
|
|
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,
Hi Vitaly, Can you please explain why the controller can not be configured in a non-RAID mode? Thanks, Mike Hello, 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.
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: Ni, Ray <ray.ni@...>
Sent: Tuesday, December 15, 2020 9:45 AM
To: devel@edk2.groups.io; cheptsov@...
Cc: Wang, Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>;
Albecki, Mateusz <mateusz.albecki@...>; Laszlo Ersek
<lersek@...>
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
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@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Mateusz Albecki <mateusz.albecki@...>
Cc: Laszlo Ersek <lersek@...>
Signed-off-by: Vitaly Cheptsov <cheptsov@...>
---
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@...]
-=-=-=-=-=-=
|
|
|
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@...>
Cc: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@...>; Ni, Ray <ray.ni@...>; Wang, Jian J <jian.j.wang@...>; Albecki, Mateusz <mateusz.albecki@...>; Laszlo Ersek <lersek@...>
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.
toggle quoted messageShow quoted text
Can you please explain why the controller can not be configured in a non-RAID mode?
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.
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.
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: Ni, Ray <ray.ni@...>
Sent: Tuesday, December 15, 2020 9:45 AM
To: devel@edk2.groups.io; cheptsov@...
Cc: Wang, Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>;
Albecki, Mateusz <mateusz.albecki@...>; Laszlo Ersek
<lersek@...>
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
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@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Mateusz Albecki <mateusz.albecki@...>
Cc: Laszlo Ersek <lersek@...>
Signed-off-by: Vitaly Cheptsov <cheptsov@...>
---
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@...]
-=-=-=-=-=-=
|
|
|
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,
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 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. Vitaly
Can you please explain why the controller can not be configured in a non-RAID mode? 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.
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. 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: Ni, Ray <ray.ni@...>
Sent: Tuesday, December 15, 2020 9:45 AM
To: devel@edk2.groups.io; cheptsov@...
Cc: Wang, Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>;
Albecki, Mateusz <mateusz.albecki@...>; Laszlo Ersek
<lersek@...>
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
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@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Mateusz Albecki <mateusz.albecki@...>
Cc: Laszlo Ersek <lersek@...>
Signed-off-by: Vitaly Cheptsov <cheptsov@...>
---
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@...]
-=-=-=-=-=-=
|
|
|
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@...>
Sent: Tuesday, December 15, 2020 9:44 AM
To: Kinney, Michael D <michael.d.kinney@...>
Cc: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@...>; Ni, Ray <ray.ni@...>; Wang, Jian J <jian.j.wang@...>; Albecki, Mateusz <mateusz.albecki@...>; Laszlo Ersek <lersek@...>
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.
toggle quoted messageShow quoted text
But do the systems allow the user to configure the FW that runs earlier? Can you require to users to configure their platforms correctly?
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.
Can you please explain why the controller can not be configured in a non-RAID mode?
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.
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.
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: Ni, Ray <ray.ni@...>
Sent: Tuesday, December 15, 2020 9:45 AM
To: devel@edk2.groups.io; cheptsov@...
Cc: Wang, Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>;
Albecki, Mateusz <mateusz.albecki@...>; Laszlo Ersek
<lersek@...>
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
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@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Mateusz Albecki <mateusz.albecki@...>
Cc: Laszlo Ersek <lersek@...>
Signed-off-by: Vitaly Cheptsov <cheptsov@...>
---
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@...]
-=-=-=-=-=-=
|
|
|
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@...> 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@...>
Sent: Tuesday, December 15, 2020 9:44 AM
To: Kinney, Michael D <michael.d.kinney@...>
Cc: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@...>; Ni, Ray <ray.ni@...>; Wang, Jian J <jian.j.wang@...>; Albecki, Mateusz <mateusz.albecki@...>; Laszlo Ersek <lersek@...>
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.
Vitaly
But do the systems allow the user to configure the FW that runs earlier? Can you require to users to configure their platforms correctly?
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.
Can you please explain why the controller can not be configured in a non-RAID mode?
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.
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.
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: Ni, Ray <ray.ni@...>
Sent: Tuesday, December 15, 2020 9:45 AM
To: devel@edk2.groups.io; cheptsov@...
Cc: Wang, Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>;
Albecki, Mateusz <mateusz.albecki@...>; Laszlo Ersek
<lersek@...>
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
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@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Mateusz Albecki <mateusz.albecki@...>
Cc: Laszlo Ersek <lersek@...>
Signed-off-by: Vitaly Cheptsov <cheptsov@...>
---
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@...]
-=-=-=-=-=-=
|
|
|
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@...>
Sent: Tuesday, December 15, 2020 10:56 AM
To: Kinney, Michael D <michael.d.kinney@...>
Cc: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@...>; Ni, Ray <ray.ni@...>; Wang, Jian J <jian.j.wang@...>; Albecki, Mateusz <mateusz.albecki@...>; Laszlo Ersek <lersek@...>
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.
toggle quoted messageShow quoted text
On 15 Dec 2020, at 21:09, Kinney, Michael D <michael.d.kinney@...> 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
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.
Vitaly
But do the systems allow the user to configure the FW that runs earlier? Can you require to users to configure their platforms correctly?
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.
Can you please explain why the controller can not be configured in a non-RAID mode?
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.
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.
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: Ni, Ray <ray.ni@...>
Sent: Tuesday, December 15, 2020 9:45 AM
To: devel@edk2.groups.io; cheptsov@...
Cc: Wang, Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>;
Albecki, Mateusz <mateusz.albecki@...>; Laszlo Ersek
<lersek@...>
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
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@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Mateusz Albecki <mateusz.albecki@...>
Cc: Laszlo Ersek <lersek@...>
Signed-off-by: Vitaly Cheptsov <cheptsov@...>
---
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@...]
-=-=-=-=-=-=
|
|
|
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@...> 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@...>
Sent: Tuesday, December 15, 2020 10:56 AM
To: Kinney, Michael D <michael.d.kinney@...>
Cc: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@...>; Ni, Ray <ray.ni@...>; Wang, Jian J <jian.j.wang@...>; Albecki, Mateusz <mateusz.albecki@...>; Laszlo Ersek <lersek@...>
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.
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
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.
Vitaly
But do the systems allow the user to configure the FW that runs earlier? Can you require to users to configure their platforms correctly?
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.
Can you please explain why the controller can not be configured in a non-RAID mode?
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.
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.
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: Ni, Ray <ray.ni@...>
Sent: Tuesday, December 15, 2020 9:45 AM
To: devel@edk2.groups.io; cheptsov@...
Cc: Wang, Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>;
Albecki, Mateusz <mateusz.albecki@...>; Laszlo Ersek
<lersek@...>
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
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@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Mateusz Albecki <mateusz.albecki@...>
Cc: Laszlo Ersek <lersek@...>
Signed-off-by: Vitaly Cheptsov <cheptsov@...>
---
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@...]
-=-=-=-=-=-=
|
|
|
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@...>
Cc: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@...>; Ni, Ray <ray.ni@...>; Wang, Jian J <jian.j.wang@...>; Albecki, Mateusz <mateusz.albecki@...>; Laszlo Ersek <lersek@...>
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.
toggle quoted messageShow quoted text
On 15 Dec 2020, at 22:00, Kinney, Michael D <michael.d.kinney@...> 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
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.
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
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.
Vitaly
But do the systems allow the user to configure the FW that runs earlier? Can you require to users to configure their platforms correctly?
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.
Can you please explain why the controller can not be configured in a non-RAID mode?
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.
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.
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: Ni, Ray <ray.ni@...>
Sent: Tuesday, December 15, 2020 9:45 AM
To: devel@edk2.groups.io; cheptsov@...
Cc: Wang, Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>;
Albecki, Mateusz <mateusz.albecki@...>; Laszlo Ersek
<lersek@...>
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
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@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Mateusz Albecki <mateusz.albecki@...>
Cc: Laszlo Ersek <lersek@...>
Signed-off-by: Vitaly Cheptsov <cheptsov@...>
---
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@...]
-=-=-=-=-=-=
|
|
|
