[PATCH 0/2] OvmfPkg: Replace the OVMF-specific SataControllerDxe with the generic one

public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed

* [PATCH 0/2] OvmfPkg: Replace the OVMF-specific SataControllerDxe with the generic one
@ 2023-05-08 21:52 Pedro Falcato
  2023-05-08 21:52 ` [PATCH 1/2] MdeModulePkg/SataControllerDxe: Remove useless null check Pedro Falcato
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Pedro Falcato @ 2023-05-08 21:52 UTC (permalink / raw)
  To: devel
  Cc: Pedro Falcato, Ard Biesheuvel, Jiewen Yao, Gerd Hoffmann,
	Erdem Aktas, James Bottomley, Min Xu, Tom Lendacky, Michael Roth,
	Rebecca Cran, Peter Grehan, Corvin Köhne, Sebastien Boeuf,
	Anthony Perard, Julien Grall, Laszlo Ersek

This patch-set replaces the OVMF specific SataControllerDxe with the MdeModulePkg/Bus/Pci one.
They were both forked from the same code, and are code-and-functionality similar. As such, there
seems to be no need for duplication here.

First I manually replayed OvmfPkg/SataControllerDxe's patches on top of the generic one. Only one
seemed to make sense. The second patch removes OvmfPkg/SataControllerDxe and replaces it for all platforms
under OvmfPkg. 

Tested by booting in QEMU (Q35 (AHCI) and PC (IDE)).
More testing from other, alternative platforms is desired, although breakage seems unlikely.

(+CC Laszlo as the author of the original SataControllerDxe patches)

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc :Jordan Justen <jordan.l.justen@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Michael Roth <michael.roth@amd.com>
Cc: Rebecca Cran <rebecca@bsdio.com>
Cc: Peter Grehan <grehan@freebsd.org>
Cc: Corvin Köhne <corvink@freebsd.org>
Cc: Sebastien Boeuf <sebastien.boeuf@intel.com>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Julien Grall <julien@xen.org>
Cc: Laszlo Ersek <lersek@redhat.com>

Pedro Falcato (2):
  MdeModulePkg/SataControllerDxe: Remove useless null check
  OvmfPkg: Replace the OVMF-specific SataControllerDxe with a generic
    one

 .../Pci/SataControllerDxe/SataController.c    |   44 +-
 OvmfPkg/AmdSev/AmdSevX64.dsc                  |    2 +-
 OvmfPkg/AmdSev/AmdSevX64.fdf                  |    2 +-
 OvmfPkg/Bhyve/BhyveX64.dsc                    |    2 +-
 OvmfPkg/Bhyve/BhyveX64.fdf                    |    2 +-
 OvmfPkg/CloudHv/CloudHvX64.dsc                |    2 +-
 OvmfPkg/CloudHv/CloudHvX64.fdf                |    2 +-
 OvmfPkg/IntelTdx/IntelTdxX64.dsc              |    2 +-
 OvmfPkg/IntelTdx/IntelTdxX64.fdf              |    2 +-
 OvmfPkg/Microvm/MicrovmX64.dsc                |    2 +-
 OvmfPkg/Microvm/MicrovmX64.fdf                |    2 +-
 OvmfPkg/OvmfPkgIa32.dsc                       |    2 +-
 OvmfPkg/OvmfPkgIa32.fdf                       |    2 +-
 OvmfPkg/OvmfPkgIa32X64.dsc                    |    2 +-
 OvmfPkg/OvmfPkgIa32X64.fdf                    |    2 +-
 OvmfPkg/OvmfPkgX64.dsc                        |    2 +-
 OvmfPkg/OvmfPkgX64.fdf                        |    2 +-
 OvmfPkg/OvmfXen.dsc                           |    2 +-
 OvmfPkg/OvmfXen.fdf                           |    2 +-
 OvmfPkg/SataControllerDxe/ComponentName.c     |  170 ---
 OvmfPkg/SataControllerDxe/SataController.c    | 1112 -----------------
 OvmfPkg/SataControllerDxe/SataController.h    |  544 --------
 .../SataControllerDxe/SataControllerDxe.inf   |   43 -
 23 files changed, 39 insertions(+), 1910 deletions(-)
 delete mode 100644 OvmfPkg/SataControllerDxe/ComponentName.c
 delete mode 100644 OvmfPkg/SataControllerDxe/SataController.c
 delete mode 100644 OvmfPkg/SataControllerDxe/SataController.h
 delete mode 100644 OvmfPkg/SataControllerDxe/SataControllerDxe.inf

-- 
2.40.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/2] MdeModulePkg/SataControllerDxe: Remove useless null check
  2023-05-08 21:52 [PATCH 0/2] OvmfPkg: Replace the OVMF-specific SataControllerDxe with the generic one Pedro Falcato
@ 2023-05-08 21:52 ` Pedro Falcato
  2023-05-08 22:28   ` [edk2-devel] " Mike Maslenkin
  2023-05-08 21:52 ` [PATCH 2/2] OvmfPkg: Replace the OVMF-specific SataControllerDxe with a generic one Pedro Falcato
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Pedro Falcato @ 2023-05-08 21:52 UTC (permalink / raw)
  To: devel; +Cc: Pedro Falcato, Jian J Wang, Liming Gao, Hao A Wu, Ray Ni

ASSERT (Private != NULL) already covers this check.
See commit 81310a6.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
---
 .../Pci/SataControllerDxe/SataController.c    | 44 +++++++++----------
 1 file changed, 21 insertions(+), 23 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c
index f661efaec7e9..ab069845fd02 100644
--- a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c
+++ b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c
@@ -625,34 +625,32 @@ SataControllerStop (
     return Status;
   }
 
-  if (Private != NULL) {
-    if (Private->DisqualifiedModes != NULL) {
-      FreePool (Private->DisqualifiedModes);
-    }
-
-    if (Private->IdentifyData != NULL) {
-      FreePool (Private->IdentifyData);
-    }
+  if (Private->DisqualifiedModes != NULL) {
+    FreePool (Private->DisqualifiedModes);
+  }
 
-    if (Private->IdentifyValid != NULL) {
-      FreePool (Private->IdentifyValid);
-    }
+  if (Private->IdentifyData != NULL) {
+    FreePool (Private->IdentifyData);
+  }
 
-    if (Private->PciAttributesChanged) {
-      //
-      // Restore original PCI attributes
-      //
-      Private->PciIo->Attributes (
-                        Private->PciIo,
-                        EfiPciIoAttributeOperationSet,
-                        Private->OriginalPciAttributes,
-                        NULL
-                        );
-    }
+  if (Private->IdentifyValid != NULL) {
+    FreePool (Private->IdentifyValid);
+  }
 
-    FreePool (Private);
+  if (Private->PciAttributesChanged) {
+    //
+    // Restore original PCI attributes
+    //
+    Private->PciIo->Attributes (
+                      Private->PciIo,
+                      EfiPciIoAttributeOperationSet,
+                      Private->OriginalPciAttributes,
+                      NULL
+                      );
   }
 
+  FreePool (Private);
+
   //
   // Close protocols opened by Sata Controller driver
   //
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Remove useless null check
  2023-05-08 21:52 ` [PATCH 1/2] MdeModulePkg/SataControllerDxe: Remove useless null check Pedro Falcato
@ 2023-05-08 22:28   ` Mike Maslenkin
  2023-05-08 22:46     ` Pedro Falcato
  0 siblings, 1 reply; 9+ messages in thread
From: Mike Maslenkin @ 2023-05-08 22:28 UTC (permalink / raw)
  To: devel, pedro.falcato

Hello Pedro,
Technically speaking  ASSERT (Private != NULL) doesn't cover this branch.
It should crash before as result of UninstallMultipleProtocolInterfaces() call.
Obviously it make no sense in release target (under normal condition
when assertion is turned off), while this code does.
But I would suggest to remove ASSERT (Private != NULL) as well since
it is useless also.
It needs to be very lucky to get NULL as result of BASE_CR(), but
actually SATA_CONTROLLER_PRIVATE_DATA_FROM_THIS() and CR()
definition care about this. There will be assert if signature doesn't
match to dereferenced memory area before Private != NULL check.

In fact, this patch just reduces indentation level by removing useless checks.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [edk2-devel] [PATCH 1/2] MdeModulePkg/SataControllerDxe: Remove useless null check
  2023-05-08 22:28   ` [edk2-devel] " Mike Maslenkin
@ 2023-05-08 22:46     ` Pedro Falcato
  0 siblings, 0 replies; 9+ messages in thread
From: Pedro Falcato @ 2023-05-08 22:46 UTC (permalink / raw)
  To: Mike Maslenkin; +Cc: devel, Jian J Wang, Liming Gao, Hao A Wu, Ray Ni

On Mon, May 8, 2023 at 11:28 PM Mike Maslenkin <mike.maslenkin@gmail.com> wrote:
>
> Hello Pedro,
> Technically speaking  ASSERT (Private != NULL) doesn't cover this branch.
> It should crash before as result of UninstallMultipleProtocolInterfaces() call.
> Obviously it make no sense in release target (under normal condition
> when assertion is turned off), while this code does.
> But I would suggest to remove ASSERT (Private != NULL) as well since
> it is useless also.
> It needs to be very lucky to get NULL as result of BASE_CR(), but
> actually SATA_CONTROLLER_PRIVATE_DATA_FROM_THIS() and CR()
> definition care about this. There will be assert if signature doesn't
> match to dereferenced memory area before Private != NULL check.
>
> In fact, this patch just reduces indentation level by removing useless checks.

Hmm yes, I agree. I don't see how that ASSERT can realistically fire.
It should also be removed.
I'm keeping this patch as-is since it's essentially a cheap backport
from OVMF SataControllerDxe; maintainers, pull it if you'd like, or I
can send a v2 later on that cleans up that ASSERT as well.

The OVMF patch doesn't really depend on this, as this is just refactoring.

(sidenote: you're dropping all CCs accidentally)

-- 
Pedro

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 2/2] OvmfPkg: Replace the OVMF-specific SataControllerDxe with a generic one
  2023-05-08 21:52 [PATCH 0/2] OvmfPkg: Replace the OVMF-specific SataControllerDxe with the generic one Pedro Falcato
  2023-05-08 21:52 ` [PATCH 1/2] MdeModulePkg/SataControllerDxe: Remove useless null check Pedro Falcato
@ 2023-05-08 21:52 ` Pedro Falcato
  2023-05-09  8:10   ` Laszlo Ersek
  2023-05-09  7:36 ` [PATCH 0/2] OvmfPkg: Replace the OVMF-specific SataControllerDxe with the " Gerd Hoffmann
  2023-05-09  8:06 ` Laszlo Ersek
  3 siblings, 1 reply; 9+ messages in thread
From: Pedro Falcato @ 2023-05-08 21:52 UTC (permalink / raw)
  To: devel
  Cc: Pedro Falcato, Ard Biesheuvel, Jiewen Yao, Gerd Hoffmann,
	Erdem Aktas, James Bottomley, Min Xu, Tom Lendacky, Michael Roth,
	Rebecca Cran, Peter Grehan, Corvin Köhne, Sebastien Boeuf,
	Anthony Perard, Julien Grall, Laszlo Ersek

Previously, OVMF had forked DuetPkg's SataControllerDxe (see commit
12e92a2). However, in commit fda951d a generic SataControllerDxe was
added to MdeModulePkg/Bus/Pci.
Since they are most similar (both code-wise and functionally), let's
unify them and de-duplicate code.

Tested by booting in QEMU, in both Q35 and PC (to test IDE and AHCI
functionality).

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc :Jordan Justen <jordan.l.justen@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Michael Roth <michael.roth@amd.com>
Cc: Rebecca Cran <rebecca@bsdio.com>
Cc: Peter Grehan <grehan@freebsd.org>
Cc: Corvin Köhne <corvink@freebsd.org>
Cc: Sebastien Boeuf <sebastien.boeuf@intel.com>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Julien Grall <julien@xen.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
---
 OvmfPkg/AmdSev/AmdSevX64.dsc                  |    2 +-
 OvmfPkg/AmdSev/AmdSevX64.fdf                  |    2 +-
 OvmfPkg/Bhyve/BhyveX64.dsc                    |    2 +-
 OvmfPkg/Bhyve/BhyveX64.fdf                    |    2 +-
 OvmfPkg/CloudHv/CloudHvX64.dsc                |    2 +-
 OvmfPkg/CloudHv/CloudHvX64.fdf                |    2 +-
 OvmfPkg/IntelTdx/IntelTdxX64.dsc              |    2 +-
 OvmfPkg/IntelTdx/IntelTdxX64.fdf              |    2 +-
 OvmfPkg/Microvm/MicrovmX64.dsc                |    2 +-
 OvmfPkg/Microvm/MicrovmX64.fdf                |    2 +-
 OvmfPkg/OvmfPkgIa32.dsc                       |    2 +-
 OvmfPkg/OvmfPkgIa32.fdf                       |    2 +-
 OvmfPkg/OvmfPkgIa32X64.dsc                    |    2 +-
 OvmfPkg/OvmfPkgIa32X64.fdf                    |    2 +-
 OvmfPkg/OvmfPkgX64.dsc                        |    2 +-
 OvmfPkg/OvmfPkgX64.fdf                        |    2 +-
 OvmfPkg/OvmfXen.dsc                           |    2 +-
 OvmfPkg/OvmfXen.fdf                           |    2 +-
 OvmfPkg/SataControllerDxe/ComponentName.c     |  170 ---
 OvmfPkg/SataControllerDxe/SataController.c    | 1112 -----------------
 OvmfPkg/SataControllerDxe/SataController.h    |  544 --------
 .../SataControllerDxe/SataControllerDxe.inf   |   43 -
 22 files changed, 18 insertions(+), 1887 deletions(-)
 delete mode 100644 OvmfPkg/SataControllerDxe/ComponentName.c
 delete mode 100644 OvmfPkg/SataControllerDxe/SataController.c
 delete mode 100644 OvmfPkg/SataControllerDxe/SataController.h
 delete mode 100644 OvmfPkg/SataControllerDxe/SataControllerDxe.inf

diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
index 943c4eed9831..4c726fd55529 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -668,7 +668,7 @@
   MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf
   MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
   MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
-  OvmfPkg/SataControllerDxe/SataControllerDxe.inf
+  MdeModulePkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf
   MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
   MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf
   MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
diff --git a/OvmfPkg/AmdSev/AmdSevX64.fdf b/OvmfPkg/AmdSev/AmdSevX64.fdf
index fec08468d3e0..463bd3e9ef15 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.fdf
+++ b/OvmfPkg/AmdSev/AmdSevX64.fdf
@@ -250,7 +250,7 @@ INF  MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
 INF  MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf
 INF  MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
 INF  MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
-INF  OvmfPkg/SataControllerDxe/SataControllerDxe.inf
+INF  MdeModulePkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf
 INF  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
 INF  MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf
 INF  MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
index d0d2712c5662..bb317a50e6af 100644
--- a/OvmfPkg/Bhyve/BhyveX64.dsc
+++ b/OvmfPkg/Bhyve/BhyveX64.dsc
@@ -698,7 +698,7 @@
   MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf
   MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
   MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
-  OvmfPkg/SataControllerDxe/SataControllerDxe.inf
+  MdeModulePkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf
   MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
   MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf
   MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
diff --git a/OvmfPkg/Bhyve/BhyveX64.fdf b/OvmfPkg/Bhyve/BhyveX64.fdf
index 153b3cfeba55..3f6270c048cc 100644
--- a/OvmfPkg/Bhyve/BhyveX64.fdf
+++ b/OvmfPkg/Bhyve/BhyveX64.fdf
@@ -239,7 +239,7 @@ INF  MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
 INF  MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf
 INF  MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
 INF  MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
-INF  OvmfPkg/SataControllerDxe/SataControllerDxe.inf
+INF  MdeModulePkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf
 INF  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
 INF  MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf
 INF  MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
diff --git a/OvmfPkg/CloudHv/CloudHvX64.dsc b/OvmfPkg/CloudHv/CloudHvX64.dsc
index 2a1139daaa19..fcc07ecc4167 100644
--- a/OvmfPkg/CloudHv/CloudHvX64.dsc
+++ b/OvmfPkg/CloudHv/CloudHvX64.dsc
@@ -787,7 +787,7 @@
   OvmfPkg/VirtioFsDxe/VirtioFsDxe.inf
   MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
   MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
-  OvmfPkg/SataControllerDxe/SataControllerDxe.inf
+  MdeModulePkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf
   MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
   MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf
   MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
diff --git a/OvmfPkg/CloudHv/CloudHvX64.fdf b/OvmfPkg/CloudHv/CloudHvX64.fdf
index 72de7bcdad66..387f305ed8cf 100644
--- a/OvmfPkg/CloudHv/CloudHvX64.fdf
+++ b/OvmfPkg/CloudHv/CloudHvX64.fdf
@@ -262,7 +262,7 @@ INF  MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
 INF  MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf
 INF  MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
 INF  MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
-INF  OvmfPkg/SataControllerDxe/SataControllerDxe.inf
+INF  MdeModulePkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf
 INF  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
 INF  MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf
 INF  MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
diff --git a/OvmfPkg/IntelTdx/IntelTdxX64.dsc b/OvmfPkg/IntelTdx/IntelTdxX64.dsc
index d4403f11a7c6..eec41d3f6527 100644
--- a/OvmfPkg/IntelTdx/IntelTdxX64.dsc
+++ b/OvmfPkg/IntelTdx/IntelTdxX64.dsc
@@ -667,7 +667,7 @@
   OvmfPkg/VirtioFsDxe/VirtioFsDxe.inf
   MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
   MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
-  OvmfPkg/SataControllerDxe/SataControllerDxe.inf
+  MdeModulePkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf
   MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
   MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf
   MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
diff --git a/OvmfPkg/IntelTdx/IntelTdxX64.fdf b/OvmfPkg/IntelTdx/IntelTdxX64.fdf
index 73dffc104301..69ed7a9bc6f4 100644
--- a/OvmfPkg/IntelTdx/IntelTdxX64.fdf
+++ b/OvmfPkg/IntelTdx/IntelTdxX64.fdf
@@ -222,7 +222,7 @@ INF  MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
 INF  MdeModulePkg/Universal/Disk/DiskIoDxe/DiskIoDxe.inf
 INF  MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe.inf
 INF  MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf
-INF  OvmfPkg/SataControllerDxe/SataControllerDxe.inf
+INF  MdeModulePkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf
 INF  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
 INF  MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf
 INF  MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
diff --git a/OvmfPkg/Microvm/MicrovmX64.dsc b/OvmfPkg/Microvm/MicrovmX64.dsc
index 5f671bc3840d..8f8db7556667 100644
--- a/OvmfPkg/Microvm/MicrovmX64.dsc
+++ b/OvmfPkg/Microvm/MicrovmX64.dsc
@@ -768,7 +768,7 @@
   OvmfPkg/VirtioFsDxe/VirtioFsDxe.inf
   MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
   MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
-  OvmfPkg/SataControllerDxe/SataControllerDxe.inf
+  MdeModulePkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf
   MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
   MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf
   MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
diff --git a/OvmfPkg/Microvm/MicrovmX64.fdf b/OvmfPkg/Microvm/MicrovmX64.fdf
index 0d44e8dfbae8..eda24a3ec9bc 100644
--- a/OvmfPkg/Microvm/MicrovmX64.fdf
+++ b/OvmfPkg/Microvm/MicrovmX64.fdf
@@ -229,7 +229,7 @@ INF  MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
 INF  MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf
 INF  MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
 INF  MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
-INF  OvmfPkg/SataControllerDxe/SataControllerDxe.inf
+INF  MdeModulePkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf
 INF  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
 INF  MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf
 INF  MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index e333b8b41803..42cc28f8adce 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -830,7 +830,7 @@
   OvmfPkg/VirtioFsDxe/VirtioFsDxe.inf
   MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
   MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
-  OvmfPkg/SataControllerDxe/SataControllerDxe.inf
+  MdeModulePkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf
   MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
   MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf
   MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
index c9c938439759..4c9be963a74d 100644
--- a/OvmfPkg/OvmfPkgIa32.fdf
+++ b/OvmfPkg/OvmfPkgIa32.fdf
@@ -271,7 +271,7 @@ INF  MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
 INF  MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf
 INF  MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
 INF  MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
-INF  OvmfPkg/SataControllerDxe/SataControllerDxe.inf
+INF  MdeModulePkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf
 INF  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
 INF  MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf
 INF  MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 25974230a27e..9f01384d037f 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -844,7 +844,7 @@
   OvmfPkg/VirtioFsDxe/VirtioFsDxe.inf
   MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
   MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
-  OvmfPkg/SataControllerDxe/SataControllerDxe.inf
+  MdeModulePkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf
   MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
   MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf
   MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
index f52219e0c26d..7f599f15e341 100644
--- a/OvmfPkg/OvmfPkgIa32X64.fdf
+++ b/OvmfPkg/OvmfPkgIa32X64.fdf
@@ -272,7 +272,7 @@ INF  MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
 INF  MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf
 INF  MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
 INF  MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
-INF  OvmfPkg/SataControllerDxe/SataControllerDxe.inf
+INF  MdeModulePkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf
 INF  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
 INF  MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf
 INF  MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index c1762ffca445..e59871af211b 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -918,7 +918,7 @@
   OvmfPkg/VirtioFsDxe/VirtioFsDxe.inf
   MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
   MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
-  OvmfPkg/SataControllerDxe/SataControllerDxe.inf
+  MdeModulePkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf
   MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
   MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf
   MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
index 00c7f8849fb8..41912fc1bece 100644
--- a/OvmfPkg/OvmfPkgX64.fdf
+++ b/OvmfPkg/OvmfPkgX64.fdf
@@ -297,7 +297,7 @@ INF  MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
 INF  MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf
 INF  MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
 INF  MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
-INF  OvmfPkg/SataControllerDxe/SataControllerDxe.inf
+INF  MdeModulePkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf
 INF  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
 INF  MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf
 INF  MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
index 1f44ec86c9c7..fa2a732fe33d 100644
--- a/OvmfPkg/OvmfXen.dsc
+++ b/OvmfPkg/OvmfXen.dsc
@@ -636,7 +636,7 @@
   MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf
   MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
   MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
-  OvmfPkg/SataControllerDxe/SataControllerDxe.inf
+  MdeModulePkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf
   MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
   MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf
   MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
diff --git a/OvmfPkg/OvmfXen.fdf b/OvmfPkg/OvmfXen.fdf
index 8be69338c7b6..bdff7c52d80a 100644
--- a/OvmfPkg/OvmfXen.fdf
+++ b/OvmfPkg/OvmfXen.fdf
@@ -279,7 +279,7 @@ INF  MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
 INF  MdeModulePkg/Universal/Disk/UnicodeCollation/EnglishDxe/EnglishDxe.inf
 INF  MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
 INF  MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
-INF  OvmfPkg/SataControllerDxe/SataControllerDxe.inf
+INF  MdeModulePkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf
 INF  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AtaAtapiPassThru.inf
 INF  MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf
 INF  MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
diff --git a/OvmfPkg/SataControllerDxe/ComponentName.c b/OvmfPkg/SataControllerDxe/ComponentName.c
deleted file mode 100644
index 7b5912bab822..000000000000
--- a/OvmfPkg/SataControllerDxe/ComponentName.c
+++ /dev/null
@@ -1,170 +0,0 @@
-/** @file
-  UEFI Component Name(2) protocol implementation for Sata Controller driver.
-
-  Copyright (c) 2011, Intel Corporation. All rights reserved.<BR>
-  SPDX-License-Identifier: BSD-2-Clause-Patent
-
-**/
-
-#include "SataController.h"
-
-//
-/// EFI Component Name Protocol
-///
-GLOBAL_REMOVE_IF_UNREFERENCED EFI_COMPONENT_NAME_PROTOCOL  gSataControllerComponentName = {
-  SataControllerComponentNameGetDriverName,
-  SataControllerComponentNameGetControllerName,
-  "eng"
-};
-
-//
-/// EFI Component Name 2 Protocol
-///
-GLOBAL_REMOVE_IF_UNREFERENCED EFI_COMPONENT_NAME2_PROTOCOL  gSataControllerComponentName2 = {
-  (EFI_COMPONENT_NAME2_GET_DRIVER_NAME)SataControllerComponentNameGetDriverName,
-  (EFI_COMPONENT_NAME2_GET_CONTROLLER_NAME)SataControllerComponentNameGetControllerName,
-  "en"
-};
-
-//
-/// Driver Name Strings
-///
-GLOBAL_REMOVE_IF_UNREFERENCED EFI_UNICODE_STRING_TABLE  mSataControllerDriverNameTable[] = {
-  {
-    "eng;en",
-    (CHAR16 *)L"Sata Controller Init Driver"
-  },
-  {
-    NULL,
-    NULL
-  }
-};
-
-///
-/// Controller Name Strings
-///
-GLOBAL_REMOVE_IF_UNREFERENCED EFI_UNICODE_STRING_TABLE  mSataControllerControllerNameTable[] = {
-  {
-    "eng;en",
-    (CHAR16 *)L"Sata Controller"
-  },
-  {
-    NULL,
-    NULL
-  }
-};
-
-/**
-  Retrieves a Unicode string that is the user readable name of the UEFI Driver.
-
-  @param This           A pointer to the EFI_COMPONENT_NAME_PROTOCOL instance.
-  @param Language       A pointer to a three character ISO 639-2 language identifier.
-                        This is the language of the driver name that the caller
-                        is requesting, and it must match one of the languages specified
-                        in SupportedLanguages.  The number of languages supported by a
-                        driver is up to the driver writer.
-  @param DriverName     A pointer to the Unicode string to return.  This Unicode string
-                        is the name of the driver specified by This in the language
-                        specified by Language.
-
-  @retval EFI_SUCCESS           The Unicode string for the Driver specified by This
-                                and the language specified by Language was returned
-                                in DriverName.
-  @retval EFI_INVALID_PARAMETER Language is NULL.
-  @retval EFI_INVALID_PARAMETER DriverName is NULL.
-  @retval EFI_UNSUPPORTED       The driver specified by This does not support the
-                                language specified by Language.
-**/
-EFI_STATUS
-EFIAPI
-SataControllerComponentNameGetDriverName (
-  IN EFI_COMPONENT_NAME_PROTOCOL  *This,
-  IN CHAR8                        *Language,
-  OUT CHAR16                      **DriverName
-  )
-{
-  return LookupUnicodeString2 (
-           Language,
-           This->SupportedLanguages,
-           mSataControllerDriverNameTable,
-           DriverName,
-           (BOOLEAN)(This == &gSataControllerComponentName)
-           );
-}
-
-/**
-  Retrieves a Unicode string that is the user readable name of the controller
-  that is being managed by an UEFI Driver.
-
-  @param This                   A pointer to the EFI_COMPONENT_NAME_PROTOCOL instance.
-  @param ControllerHandle       The handle of a controller that the driver specified by
-                                This is managing.  This handle specifies the controller
-                                whose name is to be returned.
-  @param ChildHandle OPTIONAL   The handle of the child controller to retrieve the name
-                                of.  This is an optional parameter that may be NULL.  It
-                                will be NULL for device drivers.  It will also be NULL
-                                for a bus drivers that wish to retrieve the name of the
-                                bus controller.  It will not be NULL for a bus driver
-                                that wishes to retrieve the name of a child controller.
-  @param Language               A pointer to a three character ISO 639-2 language
-                                identifier.  This is the language of the controller name
-                                that the caller is requesting, and it must match one
-                                of the languages specified in SupportedLanguages.  The
-                                number of languages supported by a driver is up to the
-                                driver writer.
-  @param ControllerName         A pointer to the Unicode string to return.  This Unicode
-                                string is the name of the controller specified by
-                                ControllerHandle and ChildHandle in the language
-                                specified by Language from the point of view of the
-                                driver specified by This.
-
-  @retval EFI_SUCCESS           The Unicode string for the user readable name in the
-                                language specified by Language for the driver
-                                specified by This was returned in DriverName.
-  @retval EFI_INVALID_PARAMETER ControllerHandle is not a valid EFI_HANDLE.
-  @retval EFI_INVALID_PARAMETER ChildHandle is not NULL and it is not a valid
-                                EFI_HANDLE.
-  @retval EFI_INVALID_PARAMETER Language is NULL.
-  @retval EFI_INVALID_PARAMETER ControllerName is NULL.
-  @retval EFI_UNSUPPORTED       The driver specified by This is not currently
-                                managing the controller specified by
-                                ControllerHandle and ChildHandle.
-  @retval EFI_UNSUPPORTED       The driver specified by This does not support the
-                                language specified by Language.
-**/
-EFI_STATUS
-EFIAPI
-SataControllerComponentNameGetControllerName (
-  IN EFI_COMPONENT_NAME_PROTOCOL  *This,
-  IN EFI_HANDLE                   ControllerHandle,
-  IN EFI_HANDLE                   ChildHandle OPTIONAL,
-  IN CHAR8                        *Language,
-  OUT CHAR16                      **ControllerName
-  )
-{
-  EFI_STATUS  Status;
-
-  //
-  // Make sure this driver is currently managing ControllHandle
-  //
-  Status = EfiTestManagedDevice (
-             ControllerHandle,
-             gSataControllerDriverBinding.DriverBindingHandle,
-             &gEfiPciIoProtocolGuid
-             );
-  if (EFI_ERROR (Status)) {
-    return Status;
-  }
-
-  if (ChildHandle != NULL) {
-    return EFI_UNSUPPORTED;
-  }
-
-  return LookupUnicodeString2 (
-           Language,
-           This->SupportedLanguages,
-           mSataControllerControllerNameTable,
-           ControllerName,
-           (BOOLEAN)(This == &gSataControllerComponentName)
-           );
-}
diff --git a/OvmfPkg/SataControllerDxe/SataController.c b/OvmfPkg/SataControllerDxe/SataController.c
deleted file mode 100644
index 2b550b0a3ec0..000000000000
--- a/OvmfPkg/SataControllerDxe/SataController.c
+++ /dev/null
@@ -1,1112 +0,0 @@
-/** @file
-  This driver module produces IDE_CONTROLLER_INIT protocol for Sata Controllers.
-
-  Copyright (c) 2011, Intel Corporation. All rights reserved.<BR>
-  SPDX-License-Identifier: BSD-2-Clause-Patent
-
-**/
-
-#include "SataController.h"
-
-///
-/// EFI_DRIVER_BINDING_PROTOCOL instance
-///
-EFI_DRIVER_BINDING_PROTOCOL  gSataControllerDriverBinding = {
-  SataControllerSupported,
-  SataControllerStart,
-  SataControllerStop,
-  0xa,
-  NULL,
-  NULL
-};
-
-/**
-  Read AHCI Operation register.
-
-  @param PciIo      The PCI IO protocol instance.
-  @param Offset     The operation register offset.
-
-  @return The register content read.
-
-**/
-UINT32
-EFIAPI
-AhciReadReg (
-  IN EFI_PCI_IO_PROTOCOL  *PciIo,
-  IN UINT32               Offset
-  )
-{
-  UINT32  Data;
-
-  ASSERT (PciIo != NULL);
-
-  Data = 0;
-
-  PciIo->Mem.Read (
-               PciIo,
-               EfiPciIoWidthUint32,
-               AHCI_BAR_INDEX,
-               (UINT64)Offset,
-               1,
-               &Data
-               );
-
-  return Data;
-}
-
-/**
-  Write AHCI Operation register.
-
-  @param PciIo      The PCI IO protocol instance.
-  @param Offset     The operation register offset.
-  @param Data       The data used to write down.
-
-**/
-VOID
-EFIAPI
-AhciWriteReg (
-  IN EFI_PCI_IO_PROTOCOL  *PciIo,
-  IN UINT32               Offset,
-  IN UINT32               Data
-  )
-{
-  ASSERT (PciIo != NULL);
-
-  PciIo->Mem.Write (
-               PciIo,
-               EfiPciIoWidthUint32,
-               AHCI_BAR_INDEX,
-               (UINT64)Offset,
-               1,
-               &Data
-               );
-
-  return;
-}
-
-/**
-  This function is used to calculate the best PIO mode supported by specific IDE device
-
-  @param IdentifyData   The identify data of specific IDE device.
-  @param DisPioMode     Disqualified PIO modes collection.
-  @param SelectedMode   Available PIO modes collection.
-
-  @retval EFI_SUCCESS       Best PIO modes are returned.
-  @retval EFI_UNSUPPORTED   The device doesn't support PIO mode,
-                            or all supported modes have been disqualified.
-**/
-EFI_STATUS
-CalculateBestPioMode (
-  IN EFI_IDENTIFY_DATA  *IdentifyData,
-  IN UINT16             *DisPioMode OPTIONAL,
-  OUT UINT16            *SelectedMode
-  )
-{
-  UINT16  PioMode;
-  UINT16  AdvancedPioMode;
-  UINT16  Temp;
-  UINT16  Index;
-  UINT16  MinimumPioCycleTime;
-
-  Temp = 0xff;
-
-  PioMode = (UINT8)(((ATA5_IDENTIFY_DATA *)(&(IdentifyData->AtaData)))->pio_cycle_timing >> 8);
-
-  //
-  // See whether Identify Data word 64 - 70 are valid
-  //
-  if ((IdentifyData->AtaData.field_validity & 0x02) == 0x02) {
-    AdvancedPioMode = IdentifyData->AtaData.advanced_pio_modes;
-    DEBUG ((DEBUG_INFO, "CalculateBestPioMode: AdvancedPioMode = %x\n", AdvancedPioMode));
-
-    for (Index = 0; Index < 8; Index++) {
-      if ((AdvancedPioMode & 0x01) != 0) {
-        Temp = Index;
-      }
-
-      AdvancedPioMode >>= 1;
-    }
-
-    //
-    // If Temp is modified, mean the advanced_pio_modes is not zero;
-    // if Temp is not modified, mean there is no advanced PIO mode supported,
-    // the best PIO Mode is the value in pio_cycle_timing.
-    //
-    if (Temp != 0xff) {
-      AdvancedPioMode = (UINT16)(Temp + 3);
-    } else {
-      AdvancedPioMode = PioMode;
-    }
-
-    //
-    // Limit the PIO mode to at most PIO4.
-    //
-    PioMode = (UINT16)MIN (AdvancedPioMode, 4);
-
-    MinimumPioCycleTime = IdentifyData->AtaData.min_pio_cycle_time_with_flow_control;
-
-    if (MinimumPioCycleTime <= 120) {
-      PioMode = (UINT16)MIN (4, PioMode);
-    } else if (MinimumPioCycleTime <= 180) {
-      PioMode = (UINT16)MIN (3, PioMode);
-    } else if (MinimumPioCycleTime <= 240) {
-      PioMode = (UINT16)MIN (2, PioMode);
-    } else {
-      PioMode = 0;
-    }
-
-    //
-    // Degrade the PIO mode if the mode has been disqualified
-    //
-    if (DisPioMode != NULL) {
-      if (*DisPioMode < 2) {
-        return EFI_UNSUPPORTED; // no mode below ATA_PIO_MODE_BELOW_2
-      }
-
-      if (PioMode >= *DisPioMode) {
-        PioMode = (UINT16)(*DisPioMode - 1);
-      }
-    }
-
-    if (PioMode < 2) {
-      *SelectedMode = 1;        // ATA_PIO_MODE_BELOW_2;
-    } else {
-      *SelectedMode = PioMode;  // ATA_PIO_MODE_2 to ATA_PIO_MODE_4;
-    }
-  } else {
-    //
-    // Identify Data word 64 - 70 are not valid
-    // Degrade the PIO mode if the mode has been disqualified
-    //
-    if (DisPioMode != NULL) {
-      if (*DisPioMode < 2) {
-        return EFI_UNSUPPORTED; // no mode below ATA_PIO_MODE_BELOW_2
-      }
-
-      if (PioMode == *DisPioMode) {
-        PioMode--;
-      }
-    }
-
-    if (PioMode < 2) {
-      *SelectedMode = 1;        // ATA_PIO_MODE_BELOW_2;
-    } else {
-      *SelectedMode = 2;        // ATA_PIO_MODE_2;
-    }
-  }
-
-  return EFI_SUCCESS;
-}
-
-/**
-  This function is used to calculate the best UDMA mode supported by specific IDE device
-
-  @param IdentifyData   The identify data of specific IDE device.
-  @param DisUDmaMode     Disqualified UDMA modes collection.
-  @param SelectedMode   Available UDMA modes collection.
-
-  @retval EFI_SUCCESS       Best UDMA modes are returned.
-  @retval EFI_UNSUPPORTED   The device doesn't support UDMA mode,
-                            or all supported modes have been disqualified.
-**/
-EFI_STATUS
-CalculateBestUdmaMode (
-  IN EFI_IDENTIFY_DATA  *IdentifyData,
-  IN UINT16             *DisUDmaMode OPTIONAL,
-  OUT UINT16            *SelectedMode
-  )
-{
-  UINT16  TempMode;
-  UINT16  DeviceUDmaMode;
-
-  DeviceUDmaMode = 0;
-
-  //
-  // Check whether the WORD 88 (supported UltraDMA by drive) is valid
-  //
-  if ((IdentifyData->AtaData.field_validity & 0x04) == 0x00) {
-    return EFI_UNSUPPORTED;
-  }
-
-  DeviceUDmaMode = IdentifyData->AtaData.ultra_dma_mode;
-  DEBUG ((DEBUG_INFO, "CalculateBestUdmaMode: DeviceUDmaMode = %x\n", DeviceUDmaMode));
-  DeviceUDmaMode &= 0x3f;
-  TempMode        = 0;          // initialize it to UDMA-0
-
-  while ((DeviceUDmaMode >>= 1) != 0) {
-    TempMode++;
-  }
-
-  //
-  // Degrade the UDMA mode if the mode has been disqualified
-  //
-  if (DisUDmaMode != NULL) {
-    if (*DisUDmaMode == 0) {
-      *SelectedMode = 0;
-      return EFI_UNSUPPORTED;   // no mode below ATA_UDMA_MODE_0
-    }
-
-    if (TempMode >= *DisUDmaMode) {
-      TempMode = (UINT16)(*DisUDmaMode - 1);
-    }
-  }
-
-  //
-  // Possible returned mode is between ATA_UDMA_MODE_0 and ATA_UDMA_MODE_5
-  //
-  *SelectedMode = TempMode;
-
-  return EFI_SUCCESS;
-}
-
-/**
-  The Entry Point of module. It follows the standard UEFI driver model.
-
-  @param[in] ImageHandle    The firmware allocated handle for the EFI image.
-  @param[in] SystemTable    A pointer to the EFI System Table.
-
-  @retval EFI_SUCCESS   The entry point is executed successfully.
-  @retval other         Some error occurs when executing this entry point.
-
-**/
-EFI_STATUS
-EFIAPI
-InitializeSataControllerDriver (
-  IN EFI_HANDLE        ImageHandle,
-  IN EFI_SYSTEM_TABLE  *SystemTable
-  )
-{
-  EFI_STATUS  Status;
-
-  //
-  // Install driver model protocol(s).
-  //
-  Status = EfiLibInstallDriverBindingComponentName2 (
-             ImageHandle,
-             SystemTable,
-             &gSataControllerDriverBinding,
-             ImageHandle,
-             &gSataControllerComponentName,
-             &gSataControllerComponentName2
-             );
-  ASSERT_EFI_ERROR (Status);
-
-  return Status;
-}
-
-/**
-  Supported function of Driver Binding protocol for this driver.
-  Test to see if this driver supports ControllerHandle.
-
-  @param This                   Protocol instance pointer.
-  @param Controller             Handle of device to test.
-  @param RemainingDevicePath    A pointer to the device path.
-                                it should be ignored by device driver.
-
-  @retval EFI_SUCCESS           This driver supports this device.
-  @retval EFI_ALREADY_STARTED   This driver is already running on this device.
-  @retval other                 This driver does not support this device.
-
-**/
-EFI_STATUS
-EFIAPI
-SataControllerSupported (
-  IN EFI_DRIVER_BINDING_PROTOCOL  *This,
-  IN EFI_HANDLE                   Controller,
-  IN EFI_DEVICE_PATH_PROTOCOL     *RemainingDevicePath
-  )
-{
-  EFI_STATUS           Status;
-  EFI_PCI_IO_PROTOCOL  *PciIo;
-  PCI_TYPE00           PciData;
-
-  //
-  // Attempt to open PCI I/O Protocol
-  //
-  Status = gBS->OpenProtocol (
-                  Controller,
-                  &gEfiPciIoProtocolGuid,
-                  (VOID **)&PciIo,
-                  This->DriverBindingHandle,
-                  Controller,
-                  EFI_OPEN_PROTOCOL_GET_PROTOCOL
-                  );
-  if (EFI_ERROR (Status)) {
-    return Status;
-  }
-
-  //
-  // Now further check the PCI header: Base Class (offset 0x0B) and
-  // Sub Class (offset 0x0A). This controller should be an SATA controller
-  //
-  Status = PciIo->Pci.Read (
-                        PciIo,
-                        EfiPciIoWidthUint8,
-                        PCI_CLASSCODE_OFFSET,
-                        sizeof (PciData.Hdr.ClassCode),
-                        PciData.Hdr.ClassCode
-                        );
-  if (EFI_ERROR (Status)) {
-    return EFI_UNSUPPORTED;
-  }
-
-  if (IS_PCI_IDE (&PciData) || IS_PCI_SATADPA (&PciData)) {
-    return EFI_SUCCESS;
-  }
-
-  return EFI_UNSUPPORTED;
-}
-
-/**
-  This routine is called right after the .Supported() called and
-  Start this driver on ControllerHandle.
-
-  @param This                   Protocol instance pointer.
-  @param Controller             Handle of device to bind driver to.
-  @param RemainingDevicePath    A pointer to the device path.
-                                it should be ignored by device driver.
-
-  @retval EFI_SUCCESS           This driver is added to this device.
-  @retval EFI_ALREADY_STARTED   This driver is already running on this device.
-  @retval other                 Some error occurs when binding this driver to this device.
-
-**/
-EFI_STATUS
-EFIAPI
-SataControllerStart (
-  IN EFI_DRIVER_BINDING_PROTOCOL  *This,
-  IN EFI_HANDLE                   Controller,
-  IN EFI_DEVICE_PATH_PROTOCOL     *RemainingDevicePath
-  )
-{
-  UINTN                             BailLogMask;
-  EFI_STATUS                        Status;
-  EFI_PCI_IO_PROTOCOL               *PciIo;
-  UINT64                            OriginalPciAttributes;
-  PCI_TYPE00                        PciData;
-  EFI_SATA_CONTROLLER_PRIVATE_DATA  *SataPrivateData;
-  UINT32                            Data32;
-  UINTN                             ChannelDeviceCount;
-
-  DEBUG ((DEBUG_INFO, "SataControllerStart START\n"));
-
-  BailLogMask     = DEBUG_ERROR;
-  SataPrivateData = NULL;
-
-  //
-  // Now test and open PCI I/O Protocol
-  //
-  Status = gBS->OpenProtocol (
-                  Controller,
-                  &gEfiPciIoProtocolGuid,
-                  (VOID **)&PciIo,
-                  This->DriverBindingHandle,
-                  Controller,
-                  EFI_OPEN_PROTOCOL_BY_DRIVER
-                  );
-  if (EFI_ERROR (Status)) {
-    if (Status == EFI_ALREADY_STARTED) {
-      //
-      // This is an expected condition for OpenProtocol() / BY_DRIVER, in a
-      // DriverBindingStart() member function; degrade the log mask to
-      // DEBUG_INFO.
-      //
-      BailLogMask = DEBUG_INFO;
-    }
-
-    goto Bail;
-  }
-
-  //
-  // Save original PCI attributes, and enable IO space access, memory space
-  // access, and Bus Master (DMA).
-  //
-  Status = PciIo->Attributes (
-                    PciIo,
-                    EfiPciIoAttributeOperationGet,
-                    0,
-                    &OriginalPciAttributes
-                    );
-  if (EFI_ERROR (Status)) {
-    goto ClosePciIo;
-  }
-
-  Status = PciIo->Attributes (
-                    PciIo,
-                    EfiPciIoAttributeOperationEnable,
-                    EFI_PCI_DEVICE_ENABLE,
-                    NULL
-                    );
-  if (EFI_ERROR (Status)) {
-    goto ClosePciIo;
-  }
-
-  //
-  // Allocate Sata Private Data structure
-  //
-  SataPrivateData = AllocateZeroPool (sizeof (EFI_SATA_CONTROLLER_PRIVATE_DATA));
-  if (SataPrivateData == NULL) {
-    Status = EFI_OUT_OF_RESOURCES;
-    goto RestorePciAttributes;
-  }
-
-  //
-  // Initialize Sata Private Data
-  //
-  SataPrivateData->Signature              = SATA_CONTROLLER_SIGNATURE;
-  SataPrivateData->PciIo                  = PciIo;
-  SataPrivateData->OriginalPciAttributes  = OriginalPciAttributes;
-  SataPrivateData->IdeInit.GetChannelInfo = IdeInitGetChannelInfo;
-  SataPrivateData->IdeInit.NotifyPhase    = IdeInitNotifyPhase;
-  SataPrivateData->IdeInit.SubmitData     = IdeInitSubmitData;
-  SataPrivateData->IdeInit.DisqualifyMode = IdeInitDisqualifyMode;
-  SataPrivateData->IdeInit.CalculateMode  = IdeInitCalculateMode;
-  SataPrivateData->IdeInit.SetTiming      = IdeInitSetTiming;
-  SataPrivateData->IdeInit.EnumAll        = SATA_ENUMER_ALL;
-
-  Status = PciIo->Pci.Read (
-                        PciIo,
-                        EfiPciIoWidthUint8,
-                        PCI_CLASSCODE_OFFSET,
-                        sizeof (PciData.Hdr.ClassCode),
-                        PciData.Hdr.ClassCode
-                        );
-  if (EFI_ERROR (Status)) {
-    goto FreeSataPrivateData;
-  }
-
-  if (IS_PCI_IDE (&PciData)) {
-    SataPrivateData->IdeInit.ChannelCount = IDE_MAX_CHANNEL;
-    SataPrivateData->DeviceCount          = IDE_MAX_DEVICES;
-  } else if (IS_PCI_SATADPA (&PciData)) {
-    //
-    // Read Host Capability Register(CAP) to get Number of Ports(NPS) and Supports Port Multiplier(SPM)
-    //   NPS is 0's based value indicating the maximum number of ports supported by the HBA silicon.
-    //   A maximum of 32 ports can be supported. A value of '0h', indicating one port, is the minimum requirement.
-    //
-    Data32                                = AhciReadReg (PciIo, R_AHCI_CAP);
-    SataPrivateData->IdeInit.ChannelCount = (UINT8)((Data32 & B_AHCI_CAP_NPS) + 1);
-    SataPrivateData->DeviceCount          = AHCI_MAX_DEVICES;
-    if ((Data32 & B_AHCI_CAP_SPM) == B_AHCI_CAP_SPM) {
-      SataPrivateData->DeviceCount = AHCI_MULTI_MAX_DEVICES;
-    }
-  }
-
-  ChannelDeviceCount                 = (UINTN)(SataPrivateData->IdeInit.ChannelCount) * (UINTN)(SataPrivateData->DeviceCount);
-  SataPrivateData->DisqualifiedModes = AllocateZeroPool ((sizeof (EFI_ATA_COLLECTIVE_MODE)) * ChannelDeviceCount);
-  if (SataPrivateData->DisqualifiedModes == NULL) {
-    Status = EFI_OUT_OF_RESOURCES;
-    goto FreeSataPrivateData;
-  }
-
-  SataPrivateData->IdentifyData = AllocateZeroPool ((sizeof (EFI_IDENTIFY_DATA)) * ChannelDeviceCount);
-  if (SataPrivateData->IdentifyData == NULL) {
-    Status = EFI_OUT_OF_RESOURCES;
-    goto FreeDisqualifiedModes;
-  }
-
-  SataPrivateData->IdentifyValid = AllocateZeroPool ((sizeof (BOOLEAN)) * ChannelDeviceCount);
-  if (SataPrivateData->IdentifyValid == NULL) {
-    Status = EFI_OUT_OF_RESOURCES;
-    goto FreeIdentifyData;
-  }
-
-  //
-  // Install IDE Controller Init Protocol to this instance
-  //
-  Status = gBS->InstallMultipleProtocolInterfaces (
-                  &Controller,
-                  &gEfiIdeControllerInitProtocolGuid,
-                  &(SataPrivateData->IdeInit),
-                  NULL
-                  );
-
-  if (EFI_ERROR (Status)) {
-    goto FreeIdentifyValid;
-  }
-
-  DEBUG ((DEBUG_INFO, "SataControllerStart END status = %r\n", Status));
-  return Status;
-
-FreeIdentifyValid:
-  FreePool (SataPrivateData->IdentifyValid);
-
-FreeIdentifyData:
-  FreePool (SataPrivateData->IdentifyData);
-
-FreeDisqualifiedModes:
-  FreePool (SataPrivateData->DisqualifiedModes);
-
-FreeSataPrivateData:
-  FreePool (SataPrivateData);
-
-RestorePciAttributes:
-  PciIo->Attributes (
-           PciIo,
-           EfiPciIoAttributeOperationSet,
-           OriginalPciAttributes,
-           NULL
-           );
-
-ClosePciIo:
-  gBS->CloseProtocol (
-         Controller,
-         &gEfiPciIoProtocolGuid,
-         This->DriverBindingHandle,
-         Controller
-         );
-
-Bail:
-  DEBUG ((
-    BailLogMask,
-    "SataControllerStart error return status = %r\n",
-    Status
-    ));
-  return Status;
-}
-
-/**
-  Stop this driver on ControllerHandle.
-
-  @param This               Protocol instance pointer.
-  @param Controller         Handle of device to stop driver on.
-  @param NumberOfChildren   Not used.
-  @param ChildHandleBuffer  Not used.
-
-  @retval EFI_SUCCESS   This driver is removed from this device.
-  @retval other         Some error occurs when removing this driver from this device.
-
-**/
-EFI_STATUS
-EFIAPI
-SataControllerStop (
-  IN EFI_DRIVER_BINDING_PROTOCOL  *This,
-  IN EFI_HANDLE                   Controller,
-  IN UINTN                        NumberOfChildren,
-  IN EFI_HANDLE                   *ChildHandleBuffer
-  )
-{
-  EFI_STATUS                        Status;
-  EFI_IDE_CONTROLLER_INIT_PROTOCOL  *IdeInit;
-  EFI_SATA_CONTROLLER_PRIVATE_DATA  *SataPrivateData;
-  EFI_PCI_IO_PROTOCOL               *PciIo;
-  UINT64                            OriginalPciAttributes;
-
-  //
-  // Open the produced protocol
-  //
-  Status = gBS->OpenProtocol (
-                  Controller,
-                  &gEfiIdeControllerInitProtocolGuid,
-                  (VOID **)&IdeInit,
-                  This->DriverBindingHandle,
-                  Controller,
-                  EFI_OPEN_PROTOCOL_GET_PROTOCOL
-                  );
-  if (EFI_ERROR (Status)) {
-    return EFI_UNSUPPORTED;
-  }
-
-  SataPrivateData = SATA_CONTROLLER_PRIVATE_DATA_FROM_THIS (IdeInit);
-  ASSERT (SataPrivateData != NULL);
-
-  PciIo                 = SataPrivateData->PciIo;
-  OriginalPciAttributes = SataPrivateData->OriginalPciAttributes;
-
-  //
-  // Uninstall the IDE Controller Init Protocol from this instance
-  //
-  Status = gBS->UninstallMultipleProtocolInterfaces (
-                  Controller,
-                  &gEfiIdeControllerInitProtocolGuid,
-                  &(SataPrivateData->IdeInit),
-                  NULL
-                  );
-  if (EFI_ERROR (Status)) {
-    return Status;
-  }
-
-  if (SataPrivateData->DisqualifiedModes != NULL) {
-    FreePool (SataPrivateData->DisqualifiedModes);
-  }
-
-  if (SataPrivateData->IdentifyData != NULL) {
-    FreePool (SataPrivateData->IdentifyData);
-  }
-
-  if (SataPrivateData->IdentifyValid != NULL) {
-    FreePool (SataPrivateData->IdentifyValid);
-  }
-
-  FreePool (SataPrivateData);
-
-  //
-  // Restore original PCI attributes
-  //
-  PciIo->Attributes (
-           PciIo,
-           EfiPciIoAttributeOperationSet,
-           OriginalPciAttributes,
-           NULL
-           );
-
-  //
-  // Close protocols opened by Sata Controller driver
-  //
-  return gBS->CloseProtocol (
-                Controller,
-                &gEfiPciIoProtocolGuid,
-                This->DriverBindingHandle,
-                Controller
-                );
-}
-
-/**
-  Calculate the flat array subscript of a (Channel, Device) pair.
-
-  @param[in] SataPrivateData  The private data structure corresponding to the
-                              SATA controller that attaches the device for
-                              which the flat array subscript is being
-                              calculated.
-
-  @param[in] Channel          The channel (ie. port) number on the SATA
-                              controller that the device is attached to.
-
-  @param[in] Device           The device number on the channel.
-
-  @return  The flat array subscript suitable for indexing DisqualifiedModes,
-           IdentifyData, and IdentifyValid.
-**/
-STATIC
-UINTN
-FlatDeviceIndex (
-  IN CONST EFI_SATA_CONTROLLER_PRIVATE_DATA  *SataPrivateData,
-  IN UINTN                                   Channel,
-  IN UINTN                                   Device
-  )
-{
-  ASSERT (SataPrivateData != NULL);
-  ASSERT (Channel < SataPrivateData->IdeInit.ChannelCount);
-  ASSERT (Device < SataPrivateData->DeviceCount);
-
-  return Channel * SataPrivateData->DeviceCount + Device;
-}
-
-//
-// Interface functions of IDE_CONTROLLER_INIT protocol
-//
-
-/**
-  Returns the information about the specified IDE channel.
-
-  This function can be used to obtain information about a particular IDE channel.
-  The driver entity uses this information during the enumeration process.
-
-  If Enabled is set to FALSE, the driver entity will not scan the channel. Note
-  that it will not prevent an operating system driver from scanning the channel.
-
-  For most of today's controllers, MaxDevices will either be 1 or 2. For SATA
-  controllers, this value will always be 1. SATA configurations can contain SATA
-  port multipliers. SATA port multipliers behave like SATA bridges and can support
-  up to 16 devices on the other side. If a SATA port out of the IDE controller
-  is connected to a port multiplier, MaxDevices will be set to the number of SATA
-  devices that the port multiplier supports. Because today's port multipliers
-  support up to fifteen SATA devices, this number can be as large as fifteen. The IDE
-  bus driver is required to scan for the presence of port multipliers behind an SATA
-  controller and enumerate up to MaxDevices number of devices behind the port
-  multiplier.
-
-  In this context, the devices behind a port multiplier constitute a channel.
-
-  @param[in]  This         The pointer to the EFI_IDE_CONTROLLER_INIT_PROTOCOL instance.
-  @param[in]  Channel      Zero-based channel number.
-  @param[out] Enabled      TRUE if this channel is enabled.  Disabled channels
-                           are not scanned to see if any devices are present.
-  @param[out] MaxDevices   The maximum number of IDE devices that the bus driver
-                           can expect on this channel.  For the ATA/ATAPI
-                           specification, version 6, this number will either be
-                           one or two. For Serial ATA (SATA) configurations with a
-                           port multiplier, this number can be as large as fifteen.
-
-  @retval EFI_SUCCESS             Information was returned without any errors.
-  @retval EFI_INVALID_PARAMETER   Channel is invalid (Channel >= ChannelCount).
-
-**/
-EFI_STATUS
-EFIAPI
-IdeInitGetChannelInfo (
-  IN EFI_IDE_CONTROLLER_INIT_PROTOCOL  *This,
-  IN UINT8                             Channel,
-  OUT BOOLEAN                          *Enabled,
-  OUT UINT8                            *MaxDevices
-  )
-{
-  EFI_SATA_CONTROLLER_PRIVATE_DATA  *SataPrivateData;
-
-  SataPrivateData = SATA_CONTROLLER_PRIVATE_DATA_FROM_THIS (This);
-  ASSERT (SataPrivateData != NULL);
-
-  if (Channel < This->ChannelCount) {
-    *Enabled    = TRUE;
-    *MaxDevices = SataPrivateData->DeviceCount;
-    return EFI_SUCCESS;
-  }
-
-  *Enabled = FALSE;
-  return EFI_INVALID_PARAMETER;
-}
-
-/**
-  The notifications from the driver entity that it is about to enter a certain
-  phase of the IDE channel enumeration process.
-
-  This function can be used to notify the IDE controller driver to perform
-  specific actions, including any chipset-specific initialization, so that the
-  chipset is ready to enter the next phase. Seven notification points are defined
-  at this time.
-
-  More synchronization points may be added as required in the future.
-
-  @param[in] This      The pointer to the EFI_IDE_CONTROLLER_INIT_PROTOCOL instance.
-  @param[in] Phase     The phase during enumeration.
-  @param[in] Channel   Zero-based channel number.
-
-  @retval EFI_SUCCESS             The notification was accepted without any errors.
-  @retval EFI_UNSUPPORTED         Phase is not supported.
-  @retval EFI_INVALID_PARAMETER   Channel is invalid (Channel >= ChannelCount).
-  @retval EFI_NOT_READY           This phase cannot be entered at this time; for
-                                  example, an attempt was made to enter a Phase
-                                  without having entered one or more previous
-                                  Phase.
-
-**/
-EFI_STATUS
-EFIAPI
-IdeInitNotifyPhase (
-  IN EFI_IDE_CONTROLLER_INIT_PROTOCOL  *This,
-  IN EFI_IDE_CONTROLLER_ENUM_PHASE     Phase,
-  IN UINT8                             Channel
-  )
-{
-  return EFI_SUCCESS;
-}
-
-/**
-  Submits the device information to the IDE controller driver.
-
-  This function is used by the driver entity to pass detailed information about
-  a particular device to the IDE controller driver. The driver entity obtains
-  this information by issuing an ATA or ATAPI IDENTIFY_DEVICE command. IdentifyData
-  is the pointer to the response data buffer. The IdentifyData buffer is owned
-  by the driver entity, and the IDE controller driver must make a local copy
-  of the entire buffer or parts of the buffer as needed. The original IdentifyData
-  buffer pointer may not be valid when
-
-    - EFI_IDE_CONTROLLER_INIT_PROTOCOL.CalculateMode() or
-    - EFI_IDE_CONTROLLER_INIT_PROTOCOL.DisqualifyMode() is called at a later point.
-
-  The IDE controller driver may consult various fields of EFI_IDENTIFY_DATA to
-  compute the optimum mode for the device. These fields are not limited to the
-  timing information. For example, an implementation of the IDE controller driver
-  may examine the vendor and type/mode field to match known bad drives.
-
-  The driver entity may submit drive information in any order, as long as it
-  submits information for all the devices belonging to the enumeration group
-  before EFI_IDE_CONTROLLER_INIT_PROTOCOL.CalculateMode() is called for any device
-  in that enumeration group. If a device is absent, EFI_IDE_CONTROLLER_INIT_PROTOCOL.SubmitData()
-  should be called with IdentifyData set to NULL.  The IDE controller driver may
-  not have any other mechanism to know whether a device is present or not. Therefore,
-  setting IdentifyData to NULL does not constitute an error condition.
-  EFI_IDE_CONTROLLER_INIT_PROTOCOL.SubmitData() can be called only once for a
-  given (Channel, Device) pair.
-
-  @param[in] This           A pointer to the EFI_IDE_CONTROLLER_INIT_PROTOCOL instance.
-  @param[in] Channel        Zero-based channel number.
-  @param[in] Device         Zero-based device number on the Channel.
-  @param[in] IdentifyData   The device's response to the ATA IDENTIFY_DEVICE command.
-
-  @retval EFI_SUCCESS             The information was accepted without any errors.
-  @retval EFI_INVALID_PARAMETER   Channel is invalid (Channel >= ChannelCount).
-  @retval EFI_INVALID_PARAMETER   Device is invalid.
-
-**/
-EFI_STATUS
-EFIAPI
-IdeInitSubmitData (
-  IN EFI_IDE_CONTROLLER_INIT_PROTOCOL  *This,
-  IN UINT8                             Channel,
-  IN UINT8                             Device,
-  IN EFI_IDENTIFY_DATA                 *IdentifyData
-  )
-{
-  EFI_SATA_CONTROLLER_PRIVATE_DATA  *SataPrivateData;
-  UINTN                             DeviceIndex;
-
-  SataPrivateData = SATA_CONTROLLER_PRIVATE_DATA_FROM_THIS (This);
-  ASSERT (SataPrivateData != NULL);
-
-  if ((Channel >= This->ChannelCount) || (Device >= SataPrivateData->DeviceCount)) {
-    return EFI_INVALID_PARAMETER;
-  }
-
-  DeviceIndex = FlatDeviceIndex (SataPrivateData, Channel, Device);
-
-  //
-  // Make a local copy of device's IdentifyData and mark the valid flag
-  //
-  if (IdentifyData != NULL) {
-    CopyMem (
-      &(SataPrivateData->IdentifyData[DeviceIndex]),
-      IdentifyData,
-      sizeof (EFI_IDENTIFY_DATA)
-      );
-
-    SataPrivateData->IdentifyValid[DeviceIndex] = TRUE;
-  } else {
-    SataPrivateData->IdentifyValid[DeviceIndex] = FALSE;
-  }
-
-  return EFI_SUCCESS;
-}
-
-/**
-  Disqualifies specific modes for an IDE device.
-
-  This function allows the driver entity or other drivers (such as platform
-  drivers) to reject certain timing modes and request the IDE controller driver
-  to recalculate modes. This function allows the driver entity and the IDE
-  controller driver to negotiate the timings on a per-device basis. This function
-  is useful in the case of drives that lie about their capabilities. An example
-  is when the IDE device fails to accept the timing modes that are calculated
-  by the IDE controller driver based on the response to the Identify Drive command.
-
-  If the driver entity does not want to limit the ATA timing modes and leave that
-  decision to the IDE controller driver, it can either not call this function for
-  the given device or call this function and set the Valid flag to FALSE for all
-  modes that are listed in EFI_ATA_COLLECTIVE_MODE.
-
-  The driver entity may disqualify modes for a device in any order and any number
-  of times.
-
-  This function can be called multiple times to invalidate multiple modes of the
-  same type (e.g., Programmed Input/Output [PIO] modes 3 and 4). See the ATA/ATAPI
-  specification for more information on PIO modes.
-
-  For Serial ATA (SATA) controllers, this member function can be used to disqualify
-  a higher transfer rate mode on a given channel. For example, a platform driver
-  may inform the IDE controller driver to not use second-generation (Gen2) speeds
-  for a certain SATA drive.
-
-  @param[in] This       The pointer to the EFI_IDE_CONTROLLER_INIT_PROTOCOL instance.
-  @param[in] Channel    The zero-based channel number.
-  @param[in] Device     The zero-based device number on the Channel.
-  @param[in] BadModes   The modes that the device does not support and that
-                        should be disqualified.
-
-  @retval EFI_SUCCESS             The modes were accepted without any errors.
-  @retval EFI_INVALID_PARAMETER   Channel is invalid (Channel >= ChannelCount).
-  @retval EFI_INVALID_PARAMETER   Device is invalid.
-  @retval EFI_INVALID_PARAMETER   IdentifyData is NULL.
-
-**/
-EFI_STATUS
-EFIAPI
-IdeInitDisqualifyMode (
-  IN EFI_IDE_CONTROLLER_INIT_PROTOCOL  *This,
-  IN UINT8                             Channel,
-  IN UINT8                             Device,
-  IN EFI_ATA_COLLECTIVE_MODE           *BadModes
-  )
-{
-  EFI_SATA_CONTROLLER_PRIVATE_DATA  *SataPrivateData;
-  UINTN                             DeviceIndex;
-
-  SataPrivateData = SATA_CONTROLLER_PRIVATE_DATA_FROM_THIS (This);
-  ASSERT (SataPrivateData != NULL);
-
-  if ((Channel >= This->ChannelCount) || (BadModes == NULL) || (Device >= SataPrivateData->DeviceCount)) {
-    return EFI_INVALID_PARAMETER;
-  }
-
-  DeviceIndex = FlatDeviceIndex (SataPrivateData, Channel, Device);
-
-  //
-  // Record the disqualified modes per channel per device. From ATA/ATAPI spec,
-  // if a mode is not supported, the modes higher than it is also not supported.
-  //
-  CopyMem (
-    &(SataPrivateData->DisqualifiedModes[DeviceIndex]),
-    BadModes,
-    sizeof (EFI_ATA_COLLECTIVE_MODE)
-    );
-
-  return EFI_SUCCESS;
-}
-
-/**
-  Returns the information about the optimum modes for the specified IDE device.
-
-  This function is used by the driver entity to obtain the optimum ATA modes for
-  a specific device.  The IDE controller driver takes into account the following
-  while calculating the mode:
-    - The IdentifyData inputs to EFI_IDE_CONTROLLER_INIT_PROTOCOL.SubmitData()
-    - The BadModes inputs to EFI_IDE_CONTROLLER_INIT_PROTOCOL.DisqualifyMode()
-
-  The driver entity is required to call EFI_IDE_CONTROLLER_INIT_PROTOCOL.SubmitData()
-  for all the devices that belong to an enumeration group before calling
-  EFI_IDE_CONTROLLER_INIT_PROTOCOL.CalculateMode() for any device in the same group.
-
-  The IDE controller driver will use controller- and possibly platform-specific
-  algorithms to arrive at SupportedModes.  The IDE controller may base its
-  decision on user preferences and other considerations as well. This function
-  may be called multiple times because the driver entity may renegotiate the mode
-  with the IDE controller driver using EFI_IDE_CONTROLLER_INIT_PROTOCOL.DisqualifyMode().
-
-  The driver entity may collect timing information for various devices in any
-  order. The driver entity is responsible for making sure that all the dependencies
-  are satisfied. For example, the SupportedModes information for device A that
-  was previously returned may become stale after a call to
-  EFI_IDE_CONTROLLER_INIT_PROTOCOL.DisqualifyMode() for device B.
-
-  The buffer SupportedModes is allocated by the callee because the caller does
-  not necessarily know the size of the buffer. The type EFI_ATA_COLLECTIVE_MODE
-  is defined in a way that allows for future extensibility and can be of variable
-  length. This memory pool should be deallocated by the caller when it is no
-  longer necessary.
-
-  The IDE controller driver for a Serial ATA (SATA) controller can use this
-  member function to force a lower speed (first-generation [Gen1] speeds on a
-  second-generation [Gen2]-capable hardware).  The IDE controller driver can
-  also allow the driver entity to stay with the speed that has been negotiated
-  by the physical layer.
-
-  @param[in]  This             The pointer to the EFI_IDE_CONTROLLER_INIT_PROTOCOL instance.
-  @param[in]  Channel          A zero-based channel number.
-  @param[in]  Device           A zero-based device number on the Channel.
-  @param[out] SupportedModes   The optimum modes for the device.
-
-  @retval EFI_SUCCESS             SupportedModes was returned.
-  @retval EFI_INVALID_PARAMETER   Channel is invalid (Channel >= ChannelCount).
-  @retval EFI_INVALID_PARAMETER   Device is invalid.
-  @retval EFI_INVALID_PARAMETER   SupportedModes is NULL.
-  @retval EFI_NOT_READY           Modes cannot be calculated due to a lack of
-                                  data.  This error may happen if
-                                  EFI_IDE_CONTROLLER_INIT_PROTOCOL.SubmitData()
-                                  and EFI_IDE_CONTROLLER_INIT_PROTOCOL.DisqualifyData()
-                                  were not called for at least one drive in the
-                                  same enumeration group.
-
-**/
-EFI_STATUS
-EFIAPI
-IdeInitCalculateMode (
-  IN EFI_IDE_CONTROLLER_INIT_PROTOCOL  *This,
-  IN UINT8                             Channel,
-  IN UINT8                             Device,
-  OUT EFI_ATA_COLLECTIVE_MODE          **SupportedModes
-  )
-{
-  EFI_SATA_CONTROLLER_PRIVATE_DATA  *SataPrivateData;
-  EFI_IDENTIFY_DATA                 *IdentifyData;
-  BOOLEAN                           IdentifyValid;
-  EFI_ATA_COLLECTIVE_MODE           *DisqualifiedModes;
-  UINT16                            SelectedMode;
-  EFI_STATUS                        Status;
-  UINTN                             DeviceIndex;
-
-  SataPrivateData = SATA_CONTROLLER_PRIVATE_DATA_FROM_THIS (This);
-  ASSERT (SataPrivateData != NULL);
-
-  if ((Channel >= This->ChannelCount) || (SupportedModes == NULL) || (Device >= SataPrivateData->DeviceCount)) {
-    return EFI_INVALID_PARAMETER;
-  }
-
-  *SupportedModes = AllocateZeroPool (sizeof (EFI_ATA_COLLECTIVE_MODE));
-  if (*SupportedModes == NULL) {
-    ASSERT (*SupportedModes != NULL);
-    return EFI_OUT_OF_RESOURCES;
-  }
-
-  DeviceIndex = FlatDeviceIndex (SataPrivateData, Channel, Device);
-
-  IdentifyData      = &(SataPrivateData->IdentifyData[DeviceIndex]);
-  IdentifyValid     = SataPrivateData->IdentifyValid[DeviceIndex];
-  DisqualifiedModes = &(SataPrivateData->DisqualifiedModes[DeviceIndex]);
-
-  //
-  // Make sure we've got the valid identify data of the device from SubmitData()
-  //
-  if (!IdentifyValid) {
-    FreePool (*SupportedModes);
-    return EFI_NOT_READY;
-  }
-
-  Status = CalculateBestPioMode (
-             IdentifyData,
-             (DisqualifiedModes->PioMode.Valid ? ((UINT16 *)&(DisqualifiedModes->PioMode.Mode)) : NULL),
-             &SelectedMode
-             );
-  if (!EFI_ERROR (Status)) {
-    (*SupportedModes)->PioMode.Valid = TRUE;
-    (*SupportedModes)->PioMode.Mode  = SelectedMode;
-  } else {
-    (*SupportedModes)->PioMode.Valid = FALSE;
-  }
-
-  DEBUG ((DEBUG_INFO, "IdeInitCalculateMode: PioMode = %x\n", (*SupportedModes)->PioMode.Mode));
-
-  Status = CalculateBestUdmaMode (
-             IdentifyData,
-             (DisqualifiedModes->UdmaMode.Valid ? ((UINT16 *)&(DisqualifiedModes->UdmaMode.Mode)) : NULL),
-             &SelectedMode
-             );
-
-  if (!EFI_ERROR (Status)) {
-    (*SupportedModes)->UdmaMode.Valid = TRUE;
-    (*SupportedModes)->UdmaMode.Mode  = SelectedMode;
-  } else {
-    (*SupportedModes)->UdmaMode.Valid = FALSE;
-  }
-
-  DEBUG ((DEBUG_INFO, "IdeInitCalculateMode: UdmaMode = %x\n", (*SupportedModes)->UdmaMode.Mode));
-
-  //
-  // The modes other than PIO and UDMA are not supported
-  //
-  return EFI_SUCCESS;
-}
-
-/**
-  Commands the IDE controller driver to program the IDE controller hardware
-  so that the specified device can operate at the specified mode.
-
-  This function is used by the driver entity to instruct the IDE controller
-  driver to program the IDE controller hardware to the specified modes. This
-  function can be called only once for a particular device. For a Serial ATA
-  (SATA) Advanced Host Controller Interface (AHCI) controller, no controller-
-  specific programming may be required.
-
-  @param[in] This      Pointer to the EFI_IDE_CONTROLLER_INIT_PROTOCOL instance.
-  @param[in] Channel   Zero-based channel number.
-  @param[in] Device    Zero-based device number on the Channel.
-  @param[in] Modes     The modes to set.
-
-  @retval EFI_SUCCESS             The command was accepted without any errors.
-  @retval EFI_INVALID_PARAMETER   Channel is invalid (Channel >= ChannelCount).
-  @retval EFI_INVALID_PARAMETER   Device is invalid.
-  @retval EFI_NOT_READY           Modes cannot be set at this time due to lack of data.
-  @retval EFI_DEVICE_ERROR        Modes cannot be set due to hardware failure.
-                                  The driver entity should not use this device.
-
-**/
-EFI_STATUS
-EFIAPI
-IdeInitSetTiming (
-  IN EFI_IDE_CONTROLLER_INIT_PROTOCOL  *This,
-  IN UINT8                             Channel,
-  IN UINT8                             Device,
-  IN EFI_ATA_COLLECTIVE_MODE           *Modes
-  )
-{
-  return EFI_SUCCESS;
-}
diff --git a/OvmfPkg/SataControllerDxe/SataController.h b/OvmfPkg/SataControllerDxe/SataController.h
deleted file mode 100644
index cb1abacfdc0f..000000000000
--- a/OvmfPkg/SataControllerDxe/SataController.h
+++ /dev/null
@@ -1,544 +0,0 @@
-/** @file
-  Header file for Sata Controller driver.
-
-  Copyright (c) 2011, Intel Corporation. All rights reserved.<BR>
-  SPDX-License-Identifier: BSD-2-Clause-Patent
-
-**/
-
-#ifndef _SATA_CONTROLLER_H_
-#define _SATA_CONTROLLER_H_
-
-#include <Uefi.h>
-#include <Protocol/ComponentName.h>
-#include <Protocol/DriverBinding.h>
-#include <Protocol/PciIo.h>
-#include <Protocol/IdeControllerInit.h>
-#include <Library/UefiDriverEntryPoint.h>
-#include <Library/DebugLib.h>
-#include <Library/UefiLib.h>
-#include <Library/BaseLib.h>
-#include <Library/BaseMemoryLib.h>
-#include <Library/MemoryAllocationLib.h>
-#include <Library/UefiBootServicesTableLib.h>
-#include <IndustryStandard/Pci.h>
-
-//
-// Global Variables definitions
-//
-extern EFI_DRIVER_BINDING_PROTOCOL   gSataControllerDriverBinding;
-extern EFI_COMPONENT_NAME_PROTOCOL   gSataControllerComponentName;
-extern EFI_COMPONENT_NAME2_PROTOCOL  gSataControllerComponentName2;
-
-#define AHCI_BAR_INDEX    0x05
-#define R_AHCI_CAP        0x0
-#define   B_AHCI_CAP_NPS  (BIT4 | BIT3 | BIT2 | BIT1 | BIT0) // Number of Ports
-#define   B_AHCI_CAP_SPM  BIT17                              // Supports Port Multiplier
-
-///
-/// AHCI each channel can have up to 1 device
-///
-#define AHCI_MAX_DEVICES  0x01
-
-///
-/// AHCI each channel can have 15 devices in the presence of a multiplier
-///
-#define AHCI_MULTI_MAX_DEVICES  0x0F
-
-///
-/// IDE supports 2 channel max
-///
-#define IDE_MAX_CHANNEL  0x02
-
-///
-/// IDE supports 2 devices max
-///
-#define IDE_MAX_DEVICES  0x02
-
-#define SATA_ENUMER_ALL  FALSE
-
-//
-// Sata Controller driver private data structure
-//
-
-#define SATA_CONTROLLER_SIGNATURE  SIGNATURE_32('S','A','T','A')
-
-typedef struct _EFI_SATA_CONTROLLER_PRIVATE_DATA {
-  //
-  // Standard signature used to identify Sata Controller private data
-  //
-  UINT32                              Signature;
-
-  //
-  // Protocol instance of IDE_CONTROLLER_INIT produced by this driver
-  //
-  EFI_IDE_CONTROLLER_INIT_PROTOCOL    IdeInit;
-
-  //
-  // Copy of protocol pointers used by this driver
-  //
-  EFI_PCI_IO_PROTOCOL                 *PciIo;
-
-  //
-  // Original PCI attributes
-  //
-  UINT64                              OriginalPciAttributes;
-
-  //
-  // The number of devices that are supported by this channel
-  //
-  UINT8                               DeviceCount;
-
-  //
-  // The highest disqulified mode for each attached device,
-  // From ATA/ATAPI spec, if a mode is not supported,
-  // the modes higher than it is also not supported
-  //
-  EFI_ATA_COLLECTIVE_MODE             *DisqualifiedModes;
-
-  //
-  // A copy of EFI_IDENTIFY_DATA data for each attached SATA device and its flag
-  //
-  EFI_IDENTIFY_DATA                   *IdentifyData;
-  BOOLEAN                             *IdentifyValid;
-} EFI_SATA_CONTROLLER_PRIVATE_DATA;
-
-#define SATA_CONTROLLER_PRIVATE_DATA_FROM_THIS(a)  CR(a, EFI_SATA_CONTROLLER_PRIVATE_DATA, IdeInit, SATA_CONTROLLER_SIGNATURE)
-
-//
-// Driver binding functions declaration
-//
-
-/**
-  Supported function of Driver Binding protocol for this driver.
-  Test to see if this driver supports ControllerHandle.
-
-  @param This                   Protocol instance pointer.
-  @param Controller             Handle of device to test.
-  @param RemainingDevicePath    A pointer to the device path. Should be ignored by
-                                device driver.
-
-  @retval EFI_SUCCESS           This driver supports this device.
-  @retval EFI_ALREADY_STARTED   This driver is already running on this device.
-  @retval other                 This driver does not support this device.
-
-**/
-EFI_STATUS
-EFIAPI
-SataControllerSupported (
-  IN EFI_DRIVER_BINDING_PROTOCOL  *This,
-  IN EFI_HANDLE                   Controller,
-  IN EFI_DEVICE_PATH_PROTOCOL     *RemainingDevicePath
-  )
-;
-
-/**
-  This routine is called right after the .Supported() called and
-  Start this driver on ControllerHandle.
-
-  @param This                   Protocol instance pointer.
-  @param Controller             Handle of device to bind driver to.
-  @param RemainingDevicePath    A pointer to the device path. Should be ignored by
-                                device driver.
-
-  @retval EFI_SUCCESS           This driver is added to this device.
-  @retval EFI_ALREADY_STARTED   This driver is already running on this device.
-  @retval other                 Some error occurs when binding this driver to this device.
-
-**/
-EFI_STATUS
-EFIAPI
-SataControllerStart (
-  IN EFI_DRIVER_BINDING_PROTOCOL  *This,
-  IN EFI_HANDLE                   Controller,
-  IN EFI_DEVICE_PATH_PROTOCOL     *RemainingDevicePath
-  )
-;
-
-/**
-  Stop this driver on ControllerHandle.
-
-  @param This               Protocol instance pointer.
-  @param Controller         Handle of device to stop driver on.
-  @param NumberOfChildren   Not used.
-  @param ChildHandleBuffer  Not used.
-
-  @retval EFI_SUCCESS   This driver is removed from this device.
-  @retval other         Some error occurs when removing this driver from this device.
-
-**/
-EFI_STATUS
-EFIAPI
-SataControllerStop (
-  IN EFI_DRIVER_BINDING_PROTOCOL  *This,
-  IN EFI_HANDLE                   Controller,
-  IN UINTN                        NumberOfChildren,
-  IN EFI_HANDLE                   *ChildHandleBuffer
-  )
-;
-
-//
-// IDE controller init functions declaration
-//
-
-/**
-  Returns the information about the specified IDE channel.
-
-  This function can be used to obtain information about a particular IDE channel.
-  The driver entity uses this information during the enumeration process.
-
-  If Enabled is set to FALSE, the driver entity will not scan the channel. Note
-  that it will not prevent an operating system driver from scanning the channel.
-
-  For most of today's controllers, MaxDevices will either be 1 or 2. For SATA
-  controllers, this value will always be 1. SATA configurations can contain SATA
-  port multipliers. SATA port multipliers behave like SATA bridges and can support
-  up to 16 devices on the other side. If a SATA port out of the IDE controller
-  is connected to a port multiplier, MaxDevices will be set to the number of SATA
-  devices that the port multiplier supports. Because today's port multipliers
-  support up to fifteen SATA devices, this number can be as large as fifteen. The IDE
-  bus driver is required to scan for the presence of port multipliers behind an SATA
-  controller and enumerate up to MaxDevices number of devices behind the port
-  multiplier.
-
-  In this context, the devices behind a port multiplier constitute a channel.
-
-  @param[in]  This         The pointer to the EFI_IDE_CONTROLLER_INIT_PROTOCOL instance.
-  @param[in]  Channel      Zero-based channel number.
-  @param[out] Enabled      TRUE if this channel is enabled.  Disabled channels
-                           are not scanned to see if any devices are present.
-  @param[out] MaxDevices   The maximum number of IDE devices that the bus driver
-                           can expect on this channel.  For the ATA/ATAPI
-                           specification, version 6, this number will either be
-                           one or two. For Serial ATA (SATA) configurations with a
-                           port multiplier, this number can be as large as fifteen.
-
-  @retval EFI_SUCCESS             Information was returned without any errors.
-  @retval EFI_INVALID_PARAMETER   Channel is invalid (Channel >= ChannelCount).
-
-**/
-EFI_STATUS
-EFIAPI
-IdeInitGetChannelInfo (
-  IN EFI_IDE_CONTROLLER_INIT_PROTOCOL  *This,
-  IN UINT8                             Channel,
-  OUT BOOLEAN                          *Enabled,
-  OUT UINT8                            *MaxDevices
-  )
-;
-
-/**
-  The notifications from the driver entity that it is about to enter a certain
-  phase of the IDE channel enumeration process.
-
-  This function can be used to notify the IDE controller driver to perform
-  specific actions, including any chipset-specific initialization, so that the
-  chipset is ready to enter the next phase. Seven notification points are defined
-  at this time.
-
-  More synchronization points may be added as required in the future.
-
-  @param[in] This      The pointer to the EFI_IDE_CONTROLLER_INIT_PROTOCOL instance.
-  @param[in] Phase     The phase during enumeration.
-  @param[in] Channel   Zero-based channel number.
-
-  @retval EFI_SUCCESS             The notification was accepted without any errors.
-  @retval EFI_UNSUPPORTED         Phase is not supported.
-  @retval EFI_INVALID_PARAMETER   Channel is invalid (Channel >= ChannelCount).
-  @retval EFI_NOT_READY           This phase cannot be entered at this time; for
-                                  example, an attempt was made to enter a Phase
-                                  without having entered one or more previous
-                                  Phase.
-
-**/
-EFI_STATUS
-EFIAPI
-IdeInitNotifyPhase (
-  IN EFI_IDE_CONTROLLER_INIT_PROTOCOL  *This,
-  IN EFI_IDE_CONTROLLER_ENUM_PHASE     Phase,
-  IN UINT8                             Channel
-  )
-;
-
-/**
-  Submits the device information to the IDE controller driver.
-
-  This function is used by the driver entity to pass detailed information about
-  a particular device to the IDE controller driver. The driver entity obtains
-  this information by issuing an ATA or ATAPI IDENTIFY_DEVICE command. IdentifyData
-  is the pointer to the response data buffer. The IdentifyData buffer is owned
-  by the driver entity, and the IDE controller driver must make a local copy
-  of the entire buffer or parts of the buffer as needed. The original IdentifyData
-  buffer pointer may not be valid when
-
-    - EFI_IDE_CONTROLLER_INIT_PROTOCOL.CalculateMode() or
-    - EFI_IDE_CONTROLLER_INIT_PROTOCOL.DisqualifyMode() is called at a later point.
-
-  The IDE controller driver may consult various fields of EFI_IDENTIFY_DATA to
-  compute the optimum mode for the device. These fields are not limited to the
-  timing information. For example, an implementation of the IDE controller driver
-  may examine the vendor and type/mode field to match known bad drives.
-
-  The driver entity may submit drive information in any order, as long as it
-  submits information for all the devices belonging to the enumeration group
-  before EFI_IDE_CONTROLLER_INIT_PROTOCOL.CalculateMode() is called for any device
-  in that enumeration group. If a device is absent, EFI_IDE_CONTROLLER_INIT_PROTOCOL.SubmitData()
-  should be called with IdentifyData set to NULL.  The IDE controller driver may
-  not have any other mechanism to know whether a device is present or not. Therefore,
-  setting IdentifyData to NULL does not constitute an error condition.
-  EFI_IDE_CONTROLLER_INIT_PROTOCOL.SubmitData() can be called only once for a
-  given (Channel, Device) pair.
-
-  @param[in] This           A pointer to the EFI_IDE_CONTROLLER_INIT_PROTOCOL instance.
-  @param[in] Channel        Zero-based channel number.
-  @param[in] Device         Zero-based device number on the Channel.
-  @param[in] IdentifyData   The device's response to the ATA IDENTIFY_DEVICE command.
-
-  @retval EFI_SUCCESS             The information was accepted without any errors.
-  @retval EFI_INVALID_PARAMETER   Channel is invalid (Channel >= ChannelCount).
-  @retval EFI_INVALID_PARAMETER   Device is invalid.
-
-**/
-EFI_STATUS
-EFIAPI
-IdeInitSubmitData (
-  IN EFI_IDE_CONTROLLER_INIT_PROTOCOL  *This,
-  IN UINT8                             Channel,
-  IN UINT8                             Device,
-  IN EFI_IDENTIFY_DATA                 *IdentifyData
-  )
-;
-
-/**
-  Disqualifies specific modes for an IDE device.
-
-  This function allows the driver entity or other drivers (such as platform
-  drivers) to reject certain timing modes and request the IDE controller driver
-  to recalculate modes. This function allows the driver entity and the IDE
-  controller driver to negotiate the timings on a per-device basis. This function
-  is useful in the case of drives that lie about their capabilities. An example
-  is when the IDE device fails to accept the timing modes that are calculated
-  by the IDE controller driver based on the response to the Identify Drive command.
-
-  If the driver entity does not want to limit the ATA timing modes and leave that
-  decision to the IDE controller driver, it can either not call this function for
-  the given device or call this function and set the Valid flag to FALSE for all
-  modes that are listed in EFI_ATA_COLLECTIVE_MODE.
-
-  The driver entity may disqualify modes for a device in any order and any number
-  of times.
-
-  This function can be called multiple times to invalidate multiple modes of the
-  same type (e.g., Programmed Input/Output [PIO] modes 3 and 4). See the ATA/ATAPI
-  specification for more information on PIO modes.
-
-  For Serial ATA (SATA) controllers, this member function can be used to disqualify
-  a higher transfer rate mode on a given channel. For example, a platform driver
-  may inform the IDE controller driver to not use second-generation (Gen2) speeds
-  for a certain SATA drive.
-
-  @param[in] This       The pointer to the EFI_IDE_CONTROLLER_INIT_PROTOCOL instance.
-  @param[in] Channel    The zero-based channel number.
-  @param[in] Device     The zero-based device number on the Channel.
-  @param[in] BadModes   The modes that the device does not support and that
-                        should be disqualified.
-
-  @retval EFI_SUCCESS             The modes were accepted without any errors.
-  @retval EFI_INVALID_PARAMETER   Channel is invalid (Channel >= ChannelCount).
-  @retval EFI_INVALID_PARAMETER   Device is invalid.
-  @retval EFI_INVALID_PARAMETER   IdentifyData is NULL.
-
-**/
-EFI_STATUS
-EFIAPI
-IdeInitDisqualifyMode (
-  IN EFI_IDE_CONTROLLER_INIT_PROTOCOL  *This,
-  IN UINT8                             Channel,
-  IN UINT8                             Device,
-  IN EFI_ATA_COLLECTIVE_MODE           *BadModes
-  )
-;
-
-/**
-  Returns the information about the optimum modes for the specified IDE device.
-
-  This function is used by the driver entity to obtain the optimum ATA modes for
-  a specific device.  The IDE controller driver takes into account the following
-  while calculating the mode:
-    - The IdentifyData inputs to EFI_IDE_CONTROLLER_INIT_PROTOCOL.SubmitData()
-    - The BadModes inputs to EFI_IDE_CONTROLLER_INIT_PROTOCOL.DisqualifyMode()
-
-  The driver entity is required to call EFI_IDE_CONTROLLER_INIT_PROTOCOL.SubmitData()
-  for all the devices that belong to an enumeration group before calling
-  EFI_IDE_CONTROLLER_INIT_PROTOCOL.CalculateMode() for any device in the same group.
-
-  The IDE controller driver will use controller- and possibly platform-specific
-  algorithms to arrive at SupportedModes.  The IDE controller may base its
-  decision on user preferences and other considerations as well. This function
-  may be called multiple times because the driver entity may renegotiate the mode
-  with the IDE controller driver using EFI_IDE_CONTROLLER_INIT_PROTOCOL.DisqualifyMode().
-
-  The driver entity may collect timing information for various devices in any
-  order. The driver entity is responsible for making sure that all the dependencies
-  are satisfied. For example, the SupportedModes information for device A that
-  was previously returned may become stale after a call to
-  EFI_IDE_CONTROLLER_INIT_PROTOCOL.DisqualifyMode() for device B.
-
-  The buffer SupportedModes is allocated by the callee because the caller does
-  not necessarily know the size of the buffer. The type EFI_ATA_COLLECTIVE_MODE
-  is defined in a way that allows for future extensibility and can be of variable
-  length. This memory pool should be deallocated by the caller when it is no
-  longer necessary.
-
-  The IDE controller driver for a Serial ATA (SATA) controller can use this
-  member function to force a lower speed (first-generation [Gen1] speeds on a
-  second-generation [Gen2]-capable hardware).  The IDE controller driver can
-  also allow the driver entity to stay with the speed that has been negotiated
-  by the physical layer.
-
-  @param[in]  This             The pointer to the EFI_IDE_CONTROLLER_INIT_PROTOCOL instance.
-  @param[in]  Channel          A zero-based channel number.
-  @param[in]  Device           A zero-based device number on the Channel.
-  @param[out] SupportedModes   The optimum modes for the device.
-
-  @retval EFI_SUCCESS             SupportedModes was returned.
-  @retval EFI_INVALID_PARAMETER   Channel is invalid (Channel >= ChannelCount).
-  @retval EFI_INVALID_PARAMETER   Device is invalid.
-  @retval EFI_INVALID_PARAMETER   SupportedModes is NULL.
-  @retval EFI_NOT_READY           Modes cannot be calculated due to a lack of
-                                  data.  This error may happen if
-                                  EFI_IDE_CONTROLLER_INIT_PROTOCOL.SubmitData()
-                                  and EFI_IDE_CONTROLLER_INIT_PROTOCOL.DisqualifyData()
-                                  were not called for at least one drive in the
-                                  same enumeration group.
-
-**/
-EFI_STATUS
-EFIAPI
-IdeInitCalculateMode (
-  IN EFI_IDE_CONTROLLER_INIT_PROTOCOL  *This,
-  IN UINT8                             Channel,
-  IN UINT8                             Device,
-  OUT EFI_ATA_COLLECTIVE_MODE          **SupportedModes
-  )
-;
-
-/**
-  Commands the IDE controller driver to program the IDE controller hardware
-  so that the specified device can operate at the specified mode.
-
-  This function is used by the driver entity to instruct the IDE controller
-  driver to program the IDE controller hardware to the specified modes. This
-  function can be called only once for a particular device. For a Serial ATA
-  (SATA) Advanced Host Controller Interface (AHCI) controller, no controller-
-  specific programming may be required.
-
-  @param[in] This      Pointer to the EFI_IDE_CONTROLLER_INIT_PROTOCOL instance.
-  @param[in] Channel   Zero-based channel number.
-  @param[in] Device    Zero-based device number on the Channel.
-  @param[in] Modes     The modes to set.
-
-  @retval EFI_SUCCESS             The command was accepted without any errors.
-  @retval EFI_INVALID_PARAMETER   Channel is invalid (Channel >= ChannelCount).
-  @retval EFI_INVALID_PARAMETER   Device is invalid.
-  @retval EFI_NOT_READY           Modes cannot be set at this time due to lack of data.
-  @retval EFI_DEVICE_ERROR        Modes cannot be set due to hardware failure.
-                                  The driver entity should not use this device.
-
-**/
-EFI_STATUS
-EFIAPI
-IdeInitSetTiming (
-  IN EFI_IDE_CONTROLLER_INIT_PROTOCOL  *This,
-  IN UINT8                             Channel,
-  IN UINT8                             Device,
-  IN EFI_ATA_COLLECTIVE_MODE           *Modes
-  )
-;
-
-//
-// Forward reference declaration
-//
-
-/**
-  Retrieves a Unicode string that is the user readable name of the UEFI Driver.
-
-  @param This           A pointer to the EFI_COMPONENT_NAME_PROTOCOL instance.
-  @param Language       A pointer to a three character ISO 639-2 language identifier.
-                        This is the language of the driver name that the caller
-                        is requesting, and it must match one of the languages specified
-                        in SupportedLanguages.  The number of languages supported by a
-                        driver is up to the driver writer.
-  @param DriverName     A pointer to the Unicode string to return.  This Unicode string
-                        is the name of the driver specified by This in the language
-                        specified by Language.
-
-  @retval EFI_SUCCESS           The Unicode string for the Driver specified by This
-                                and the language specified by Language was returned
-                                in DriverName.
-  @retval EFI_INVALID_PARAMETER Language is NULL.
-  @retval EFI_INVALID_PARAMETER DriverName is NULL.
-  @retval EFI_UNSUPPORTED       The driver specified by This does not support the
-                                language specified by Language.
-**/
-EFI_STATUS
-EFIAPI
-SataControllerComponentNameGetDriverName (
-  IN EFI_COMPONENT_NAME_PROTOCOL  *This,
-  IN CHAR8                        *Language,
-  OUT CHAR16                      **DriverName
-  )
-;
-
-/**
-  Retrieves a Unicode string that is the user readable name of the controller
-  that is being managed by an UEFI Driver.
-
-  @param This                   A pointer to the EFI_COMPONENT_NAME_PROTOCOL instance.
-  @param ControllerHandle       The handle of a controller that the driver specified by
-                                This is managing.  This handle specifies the controller
-                                whose name is to be returned.
-  @param OPTIONAL   ChildHandle The handle of the child controller to retrieve the name
-                                of.  This is an optional parameter that may be NULL.  It
-                                will be NULL for device drivers.  It will also be NULL
-                                for a bus drivers that wish to retrieve the name of the
-                                bus controller.  It will not be NULL for a bus driver
-                                that wishes to retrieve the name of a child controller.
-  @param Language               A pointer to a three character ISO 639-2 language
-                                identifier.  This is the language of the controller name
-                                that the caller is requesting, and it must match one
-                                of the languages specified in SupportedLanguages.  The
-                                number of languages supported by a driver is up to the
-                                driver writer.
-  @param ControllerName         A pointer to the Unicode string to return.  This Unicode
-                                string is the name of the controller specified by
-                                ControllerHandle and ChildHandle in the language
-                                specified by Language from the point of view of the
-                                driver specified by This.
-
-  @retval EFI_SUCCESS           The Unicode string for the user readable name in the
-                                language specified by Language for the driver
-                                specified by This was returned in DriverName.
-  @retval EFI_INVALID_PARAMETER ControllerHandle is not a valid EFI_HANDLE.
-  @retval EFI_INVALID_PARAMETER ChildHandle is not NULL and it is not a valid
-                                EFI_HANDLE.
-  @retval EFI_INVALID_PARAMETER Language is NULL.
-  @retval EFI_INVALID_PARAMETER ControllerName is NULL.
-  @retval EFI_UNSUPPORTED       The driver specified by This is not currently
-                                managing the controller specified by
-                                ControllerHandle and ChildHandle.
-  @retval EFI_UNSUPPORTED       The driver specified by This does not support the
-                                language specified by Language.
-**/
-EFI_STATUS
-EFIAPI
-SataControllerComponentNameGetControllerName (
-  IN EFI_COMPONENT_NAME_PROTOCOL  *This,
-  IN EFI_HANDLE                   ControllerHandle,
-  IN EFI_HANDLE                   ChildHandle OPTIONAL,
-  IN CHAR8                        *Language,
-  OUT CHAR16                      **ControllerName
-  )
-;
-
-#endif
diff --git a/OvmfPkg/SataControllerDxe/SataControllerDxe.inf b/OvmfPkg/SataControllerDxe/SataControllerDxe.inf
deleted file mode 100644
index 2bc416971b79..000000000000
--- a/OvmfPkg/SataControllerDxe/SataControllerDxe.inf
+++ /dev/null
@@ -1,43 +0,0 @@
-## @file
-#
-#    Component description file for the Sata Controller driver.
-#
-#  Copyright (c) 2011 - 2018, Intel Corporation. All rights reserved.<BR>
-#  SPDX-License-Identifier: BSD-2-Clause-Patent
-#
-##
-
-[Defines]
-  INF_VERSION                    = 0x00010005
-  BASE_NAME                      = SataController
-  FILE_GUID                      = 021722D8-522B-4079-852A-FE44C2C13F49
-  MODULE_TYPE                    = UEFI_DRIVER
-  VERSION_STRING                 = 1.0
-  ENTRY_POINT                    = InitializeSataControllerDriver
-
-#
-# The following information is for reference only and not required by the build tools.
-#
-#  VALID_ARCHITECTURES           = IA32 X64 EBC
-#
-
-[Sources]
-  ComponentName.c
-  SataController.c
-  SataController.h
-
-[Packages]
-  MdePkg/MdePkg.dec
-
-[LibraryClasses]
-  UefiDriverEntryPoint
-  DebugLib
-  UefiLib
-  BaseLib
-  BaseMemoryLib
-  MemoryAllocationLib
-  UefiBootServicesTableLib
-
-[Protocols]
-  gEfiPciIoProtocolGuid
-  gEfiIdeControllerInitProtocolGuid
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] OvmfPkg: Replace the OVMF-specific SataControllerDxe with a generic one
  2023-05-08 21:52 ` [PATCH 2/2] OvmfPkg: Replace the OVMF-specific SataControllerDxe with a generic one Pedro Falcato
@ 2023-05-09  8:10   ` Laszlo Ersek
  0 siblings, 0 replies; 9+ messages in thread
From: Laszlo Ersek @ 2023-05-09  8:10 UTC (permalink / raw)
  To: Pedro Falcato, devel
  Cc: Ard Biesheuvel, Jiewen Yao, Gerd Hoffmann, Erdem Aktas,
	James Bottomley, Min Xu, Tom Lendacky, Michael Roth, Rebecca Cran,
	Peter Grehan, Corvin Köhne, Sebastien Boeuf, Anthony Perard,
	Julien Grall

On 5/8/23 23:52, Pedro Falcato wrote:
> Previously, OVMF had forked DuetPkg's SataControllerDxe (see commit
> 12e92a2). However, in commit fda951d a generic SataControllerDxe was
> added to MdeModulePkg/Bus/Pci.
> Since they are most similar (both code-wise and functionally), let's
> unify them and de-duplicate code.
> 
> Tested by booting in QEMU, in both Q35 and PC (to test IDE and AHCI
> functionality).
> 
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc :Jordan Justen <jordan.l.justen@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Michael Roth <michael.roth@amd.com>
> Cc: Rebecca Cran <rebecca@bsdio.com>
> Cc: Peter Grehan <grehan@freebsd.org>
> Cc: Corvin Köhne <corvink@freebsd.org>
> Cc: Sebastien Boeuf <sebastien.boeuf@intel.com>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Julien Grall <julien@xen.org>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
> ---
>  OvmfPkg/AmdSev/AmdSevX64.dsc                  |    2 +-
>  OvmfPkg/AmdSev/AmdSevX64.fdf                  |    2 +-
>  OvmfPkg/Bhyve/BhyveX64.dsc                    |    2 +-
>  OvmfPkg/Bhyve/BhyveX64.fdf                    |    2 +-
>  OvmfPkg/CloudHv/CloudHvX64.dsc                |    2 +-
>  OvmfPkg/CloudHv/CloudHvX64.fdf                |    2 +-
>  OvmfPkg/IntelTdx/IntelTdxX64.dsc              |    2 +-
>  OvmfPkg/IntelTdx/IntelTdxX64.fdf              |    2 +-
>  OvmfPkg/Microvm/MicrovmX64.dsc                |    2 +-
>  OvmfPkg/Microvm/MicrovmX64.fdf                |    2 +-
>  OvmfPkg/OvmfPkgIa32.dsc                       |    2 +-
>  OvmfPkg/OvmfPkgIa32.fdf                       |    2 +-
>  OvmfPkg/OvmfPkgIa32X64.dsc                    |    2 +-
>  OvmfPkg/OvmfPkgIa32X64.fdf                    |    2 +-
>  OvmfPkg/OvmfPkgX64.dsc                        |    2 +-
>  OvmfPkg/OvmfPkgX64.fdf                        |    2 +-
>  OvmfPkg/OvmfXen.dsc                           |    2 +-
>  OvmfPkg/OvmfXen.fdf                           |    2 +-
>  OvmfPkg/SataControllerDxe/ComponentName.c     |  170 ---
>  OvmfPkg/SataControllerDxe/SataController.c    | 1112 -----------------
>  OvmfPkg/SataControllerDxe/SataController.h    |  544 --------
>  .../SataControllerDxe/SataControllerDxe.inf   |   43 -
>  22 files changed, 18 insertions(+), 1887 deletions(-)
>  delete mode 100644 OvmfPkg/SataControllerDxe/ComponentName.c
>  delete mode 100644 OvmfPkg/SataControllerDxe/SataController.c
>  delete mode 100644 OvmfPkg/SataControllerDxe/SataController.h
>  delete mode 100644 OvmfPkg/SataControllerDxe/SataControllerDxe.inf

Just to make this patch a bit more tractable, I'd suggest splitting it.

First, update only the DSC/FDF files. In particular, if you do that
alongside review/maintainer responsibilities -- that is, for example,
you create a separate FDF+DSC patch for Bhyve and another FDF+DSC patch
for Xen --, then your reviewers will thank you for the effort, as they
won't have to wade through platform DSC+FDF code they don't care about.

Second, removing "OvmfPkg/SataControllerDxe" can be its own patch at the
very end of the series; and that one need not be CC'd to the various
platform maintainers. With smaller / more focused patches,
"GetMaintainer.py" will provide more targeted CC lists.

Thanks!
Laszlo


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/2] OvmfPkg: Replace the OVMF-specific SataControllerDxe with the generic one
  2023-05-08 21:52 [PATCH 0/2] OvmfPkg: Replace the OVMF-specific SataControllerDxe with the generic one Pedro Falcato
  2023-05-08 21:52 ` [PATCH 1/2] MdeModulePkg/SataControllerDxe: Remove useless null check Pedro Falcato
  2023-05-08 21:52 ` [PATCH 2/2] OvmfPkg: Replace the OVMF-specific SataControllerDxe with a generic one Pedro Falcato
@ 2023-05-09  7:36 ` Gerd Hoffmann
  2023-05-09  8:06 ` Laszlo Ersek
  3 siblings, 0 replies; 9+ messages in thread
From: Gerd Hoffmann @ 2023-05-09  7:36 UTC (permalink / raw)
  To: Pedro Falcato
  Cc: devel, Ard Biesheuvel, Jiewen Yao, Erdem Aktas, James Bottomley,
	Min Xu, Tom Lendacky, Michael Roth, Rebecca Cran, Peter Grehan,
	Corvin Köhne, Sebastien Boeuf, Anthony Perard, Julien Grall,
	Laszlo Ersek

On Mon, May 08, 2023 at 10:52:44PM +0100, Pedro Falcato wrote:
> This patch-set replaces the OVMF specific SataControllerDxe with the MdeModulePkg/Bus/Pci one.
> They were both forked from the same code, and are code-and-functionality similar. As such, there
> seems to be no need for duplication here.
> 
> First I manually replayed OvmfPkg/SataControllerDxe's patches on top of the generic one. Only one
> seemed to make sense. The second patch removes OvmfPkg/SataControllerDxe and replaces it for all platforms
> under OvmfPkg. 
> 
> Tested by booting in QEMU (Q35 (AHCI) and PC (IDE)).
> More testing from other, alternative platforms is desired, although breakage seems unlikely.
> 
> (+CC Laszlo as the author of the original SataControllerDxe patches)

Tested-by: Gerd Hoffmann <kraxel@redhat.com>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>

take care,
  Gerd


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/2] OvmfPkg: Replace the OVMF-specific SataControllerDxe with the generic one
  2023-05-08 21:52 [PATCH 0/2] OvmfPkg: Replace the OVMF-specific SataControllerDxe with the generic one Pedro Falcato
                   ` (2 preceding siblings ...)
  2023-05-09  7:36 ` [PATCH 0/2] OvmfPkg: Replace the OVMF-specific SataControllerDxe with the " Gerd Hoffmann
@ 2023-05-09  8:06 ` Laszlo Ersek
  2023-05-09 16:46   ` Pedro Falcato
  3 siblings, 1 reply; 9+ messages in thread
From: Laszlo Ersek @ 2023-05-09  8:06 UTC (permalink / raw)
  To: Pedro Falcato, devel
  Cc: Ard Biesheuvel, Jiewen Yao, Gerd Hoffmann, Erdem Aktas,
	James Bottomley, Min Xu, Tom Lendacky, Michael Roth, Rebecca Cran,
	Peter Grehan, Corvin Köhne, Sebastien Boeuf, Anthony Perard,
	Julien Grall

On 5/8/23 23:52, Pedro Falcato wrote:
> This patch-set replaces the OVMF specific SataControllerDxe with the
> MdeModulePkg/Bus/Pci one. They were both forked from the same code,
> and are code-and-functionality similar. As such, there seems to be no
> need for duplication here.

Man, the *coat-turning* of the MdeModulePkg maintainers is just
staggering here.

When we first wanted to use SataControllerDxe in OvmfPkg, the driver
used to exist in DuetPkg. Clearly we attempted to upstream it to
MdeModulePkg too, and to consume it in OvmfPkg from there. But noooo,
the argument was that SataControllerDxe was inherently platform
specific, and so every platform was going to need its own.

Do read the first paragraph of commit 12e92a23ada7 ("OvmfPkg: copy
SataControllerDxe from DuetPkg", 2015-09-22):

    Edk2 maintainers reached the consensus that SataControllerDxe was
    inherently platform specific, for which reason it was not appropriate for
    either PcAtChipsetPkg nor MdeModulePkg. Hence, if OvmfPkg wanted to use
    it, it should either reference it directly from under DuetPkg, or copy it.

Also note the date: September 2015.

And then, less than a year later, Intel introduced a so-called "generic"
SataController driver, in commit fda951df6827 ("MdeModulePkg: add
generic SataController driver.", 2016-08-03). Completely useless
(empty!) commit message of course, as it was usual back then. Splendid
example of pretending ignorance, of falsifying history, and of *not*
reaching out to people to trim their platform code now that "we have
changed our minds". Stay classy, Intel.

(But, I need not tell you, Pedro, this; there's a reason why the Ext4
driver is not under MdeModulePkg/Universal/Disk, alongside UdfDxe; or at
least in edk2, alongside FatPkg. Until Intel doesn't need the driver,
it's not a "generic enough" driver; period.)

If you review "Maintainers.txt" exactly at commit fda951df6827, it gets
funnier. Back then, MdeModulePkg had two maintainers, Feng Tian and Star
Zeng. The patch was authored by Feng, i.e., one of the package
maintainers. The other maintainer (Star) didn't review the patch (based
on the commit message); two other Intel developers did. I see this as a
lack of accountability.

And then for comparison, consider:

- PciSegmentInfoLib (from commit e457c1f65d18),

- BasePciSegmentLibSegmentInfo and DxeRuntimePciSegmentLibSegmentInfo
  instances of PciSegmentLib (from child commit 5c9bb86f171c), which
  consume the above.

These were added to MdePkg in August 2017. And if you check the tree:

- DxeRuntimePciSegmentLibSegmentInfo remains unused *to this day* (even
  in edk2-platforms!),

- BasePciSegmentLibSegmentInfo was first put to use in edk2 nearly three
  years later, in UefiPayloadPkg -- in commit 3900a63e3a1b
  ("UefiPayloadPkg/Pci: Use the PCIE Base Addr stored in AcpiBoardInfo
  HOB", 2020-06-24).

So DxeRuntimePciSegmentLibSegmentInfo has been "generic enough" to be in
*MdePkg* for 5+ years without a single open source consumer, and
BasePciSegmentLibSegmentInfo had been generic enough to exist in MdePkg
for ~3 years without a single open-source consumer.

It's difficult to get used to this double standard.

Anyway, end of history lesson.

> First I manually replayed OvmfPkg/SataControllerDxe's patches on top
> of the generic one. Only one seemed to make sense. The second patch
> removes OvmfPkg/SataControllerDxe and replaces it for all platforms
> under OvmfPkg.

bcab71413407 --> 24fee0528c32; OK
81310a62be31 --> your patch#1 in this series (which I wasn't CC'd on,
                 apparently)
0b448dd8b27c --> not necessary
5dfba97c4d59 --> missing

I disagree with your assessment that commit 5dfba97c4d59 does not make
sense for the MdeModulePkg driver. If you have a "silent" firmware build
that only enables the DEBUG_ERROR bit in the debug message mask (I'm too
lazy to look up the precise PCD name now), then the driver will
needlessly pollute the error log.

Therefore I suggest porting 5dfba97c4d59 as well.

In turn, 5dfba97c4d59 depends (contextually at least) on commit
379b17965f0f ("OvmfPkg: SataControllerDxe: add cascading error handling
to Start()", 2015-09-22), so you might or might not want to port that
one too.

> Tested by booting in QEMU (Q35 (AHCI) and PC (IDE)).
> More testing from other, alternative platforms is desired, although
> breakage seems unlikely.

Eliminating code duplication is almost always a good thing. Duplicating
code is justified when it alleviates friction across responsibility
boundaries. In this instance, I agree that the one driver should exist
in MdeModulePkg; that's how it always should have been. I'm just shaking
my head as to why Intel foisted this 7+ year detour on us.

I suggest porting 5dfba97c4d59 as well, in v2.

> (+CC Laszlo as the author of the original SataControllerDxe patches)

Thanks for the CC.

Judged from my own emotional reaction, it's quite possible that I'm
learning of the existence of MdeModulePkg/Bus/Pci/SataControllerDxe only
now, from you. (I figure if I had seen it earlier, it would have riled
me sufficiently that now I'd remember it. I could be wrong; thankfully,
I do forget.)

Thanks
Laszlo

>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc :Jordan Justen <jordan.l.justen@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Michael Roth <michael.roth@amd.com>
> Cc: Rebecca Cran <rebecca@bsdio.com>
> Cc: Peter Grehan <grehan@freebsd.org>
> Cc: Corvin Köhne <corvink@freebsd.org>
> Cc: Sebastien Boeuf <sebastien.boeuf@intel.com>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Julien Grall <julien@xen.org>
> Cc: Laszlo Ersek <lersek@redhat.com>
>
> Pedro Falcato (2):
>   MdeModulePkg/SataControllerDxe: Remove useless null check
>   OvmfPkg: Replace the OVMF-specific SataControllerDxe with a generic
>     one
>
>  .../Pci/SataControllerDxe/SataController.c    |   44 +-
>  OvmfPkg/AmdSev/AmdSevX64.dsc                  |    2 +-
>  OvmfPkg/AmdSev/AmdSevX64.fdf                  |    2 +-
>  OvmfPkg/Bhyve/BhyveX64.dsc                    |    2 +-
>  OvmfPkg/Bhyve/BhyveX64.fdf                    |    2 +-
>  OvmfPkg/CloudHv/CloudHvX64.dsc                |    2 +-
>  OvmfPkg/CloudHv/CloudHvX64.fdf                |    2 +-
>  OvmfPkg/IntelTdx/IntelTdxX64.dsc              |    2 +-
>  OvmfPkg/IntelTdx/IntelTdxX64.fdf              |    2 +-
>  OvmfPkg/Microvm/MicrovmX64.dsc                |    2 +-
>  OvmfPkg/Microvm/MicrovmX64.fdf                |    2 +-
>  OvmfPkg/OvmfPkgIa32.dsc                       |    2 +-
>  OvmfPkg/OvmfPkgIa32.fdf                       |    2 +-
>  OvmfPkg/OvmfPkgIa32X64.dsc                    |    2 +-
>  OvmfPkg/OvmfPkgIa32X64.fdf                    |    2 +-
>  OvmfPkg/OvmfPkgX64.dsc                        |    2 +-
>  OvmfPkg/OvmfPkgX64.fdf                        |    2 +-
>  OvmfPkg/OvmfXen.dsc                           |    2 +-
>  OvmfPkg/OvmfXen.fdf                           |    2 +-
>  OvmfPkg/SataControllerDxe/ComponentName.c     |  170 ---
>  OvmfPkg/SataControllerDxe/SataController.c    | 1112 -----------------
>  OvmfPkg/SataControllerDxe/SataController.h    |  544 --------
>  .../SataControllerDxe/SataControllerDxe.inf   |   43 -
>  23 files changed, 39 insertions(+), 1910 deletions(-)
>  delete mode 100644 OvmfPkg/SataControllerDxe/ComponentName.c
>  delete mode 100644 OvmfPkg/SataControllerDxe/SataController.c
>  delete mode 100644 OvmfPkg/SataControllerDxe/SataController.h
>  delete mode 100644 OvmfPkg/SataControllerDxe/SataControllerDxe.inf
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/2] OvmfPkg: Replace the OVMF-specific SataControllerDxe with the generic one
  2023-05-09  8:06 ` Laszlo Ersek
@ 2023-05-09 16:46   ` Pedro Falcato
  0 siblings, 0 replies; 9+ messages in thread
From: Pedro Falcato @ 2023-05-09 16:46 UTC (permalink / raw)
  To: Laszlo Ersek
  Cc: devel, Ard Biesheuvel, Jiewen Yao, Gerd Hoffmann, Erdem Aktas,
	James Bottomley, Min Xu, Tom Lendacky, Michael Roth, Rebecca Cran,
	Peter Grehan, Corvin Köhne, Sebastien Boeuf, Anthony Perard,
	Julien Grall

On Tue, May 9, 2023 at 9:06 AM Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 5/8/23 23:52, Pedro Falcato wrote:
> > This patch-set replaces the OVMF specific SataControllerDxe with the
> > MdeModulePkg/Bus/Pci one. They were both forked from the same code,
> > and are code-and-functionality similar. As such, there seems to be no
> > need for duplication here.
>
> Man, the *coat-turning* of the MdeModulePkg maintainers is just
> staggering here.
>
> When we first wanted to use SataControllerDxe in OvmfPkg, the driver
> used to exist in DuetPkg. Clearly we attempted to upstream it to
> MdeModulePkg too, and to consume it in OvmfPkg from there. But noooo,
> the argument was that SataControllerDxe was inherently platform
> specific, and so every platform was going to need its own.
>
> Do read the first paragraph of commit 12e92a23ada7 ("OvmfPkg: copy
> SataControllerDxe from DuetPkg", 2015-09-22):
>
>     Edk2 maintainers reached the consensus that SataControllerDxe was
>     inherently platform specific, for which reason it was not appropriate for
>     either PcAtChipsetPkg nor MdeModulePkg. Hence, if OvmfPkg wanted to use
>     it, it should either reference it directly from under DuetPkg, or copy it.
>
> Also note the date: September 2015.
>
> And then, less than a year later, Intel introduced a so-called "generic"
> SataController driver, in commit fda951df6827 ("MdeModulePkg: add
> generic SataController driver.", 2016-08-03). Completely useless
> (empty!) commit message of course, as it was usual back then. Splendid
Timeless.

> example of pretending ignorance, of falsifying history, and of *not*
> reaching out to people to trim their platform code now that "we have
> changed our minds". Stay classy, Intel.
>
> (But, I need not tell you, Pedro, this; there's a reason why the Ext4
> driver is not under MdeModulePkg/Universal/Disk, alongside UdfDxe; or at
> least in edk2, alongside FatPkg. Until Intel doesn't need the driver,
> it's not a "generic enough" driver; period.)

:))))))))))))))))))))

>
> If you review "Maintainers.txt" exactly at commit fda951df6827, it gets
> funnier. Back then, MdeModulePkg had two maintainers, Feng Tian and Star
> Zeng. The patch was authored by Feng, i.e., one of the package
> maintainers. The other maintainer (Star) didn't review the patch (based
> on the commit message); two other Intel developers did. I see this as a
> lack of accountability.
>
> And then for comparison, consider:
>
> - PciSegmentInfoLib (from commit e457c1f65d18),
>
> - BasePciSegmentLibSegmentInfo and DxeRuntimePciSegmentLibSegmentInfo
>   instances of PciSegmentLib (from child commit 5c9bb86f171c), which
>   consume the above.
>
> These were added to MdePkg in August 2017. And if you check the tree:
>
> - DxeRuntimePciSegmentLibSegmentInfo remains unused *to this day* (even
>   in edk2-platforms!),
>
> - BasePciSegmentLibSegmentInfo was first put to use in edk2 nearly three
>   years later, in UefiPayloadPkg -- in commit 3900a63e3a1b
>   ("UefiPayloadPkg/Pci: Use the PCIE Base Addr stored in AcpiBoardInfo
>   HOB", 2020-06-24).
>
> So DxeRuntimePciSegmentLibSegmentInfo has been "generic enough" to be in
> *MdePkg* for 5+ years without a single open source consumer, and
> BasePciSegmentLibSegmentInfo had been generic enough to exist in MdePkg
> for ~3 years without a single open-source consumer.
>
> It's difficult to get used to this double standard.
>
> Anyway, end of history lesson.

Hah. Thanks for the history lesson. I had understood most of the story
behind SataControllerDxe by reading commit messages but those Pci libs
are new to me :v

>
> > First I manually replayed OvmfPkg/SataControllerDxe's patches on top
> > of the generic one. Only one seemed to make sense. The second patch
> > removes OvmfPkg/SataControllerDxe and replaces it for all platforms
> > under OvmfPkg.
>
> bcab71413407 --> 24fee0528c32; OK
> 81310a62be31 --> your patch#1 in this series (which I wasn't CC'd on,
>                  apparently)
Sorry for that, CC'd you on all patches now (sorry for the spam!)

> 0b448dd8b27c --> not necessary
> 5dfba97c4d59 --> missing
>
> I disagree with your assessment that commit 5dfba97c4d59 does not make
> sense for the MdeModulePkg driver. If you have a "silent" firmware build
> that only enables the DEBUG_ERROR bit in the debug message mask (I'm too
> lazy to look up the precise PCD name now), then the driver will
> needlessly pollute the error log.
>
> Therefore I suggest porting 5dfba97c4d59 as well.
>
> In turn, 5dfba97c4d59 depends (contextually at least) on commit
> 379b17965f0f ("OvmfPkg: SataControllerDxe: add cascading error handling
> to Start()", 2015-09-22), so you might or might not want to port that
> one too.

Ack. I ported 5dfba97... as you suggested and 379b17... as the error
handling ultimately gets cleaner.
>
> > Tested by booting in QEMU (Q35 (AHCI) and PC (IDE)).
> > More testing from other, alternative platforms is desired, although
> > breakage seems unlikely.
>
> Eliminating code duplication is almost always a good thing. Duplicating
> code is justified when it alleviates friction across responsibility
> boundaries. In this instance, I agree that the one driver should exist
> in MdeModulePkg; that's how it always should have been. I'm just shaking
> my head as to why Intel foisted this 7+ year detour on us.
>
> I suggest porting 5dfba97c4d59 as well, in v2.
>
> > (+CC Laszlo as the author of the original SataControllerDxe patches)
>
> Thanks for the CC.
Not a problem, I figured you'd wanna know :p

>
> Judged from my own emotional reaction, it's quite possible that I'm
> learning of the existence of MdeModulePkg/Bus/Pci/SataControllerDxe only
> now, from you. (I figure if I had seen it earlier, it would have riled
> me sufficiently that now I'd remember it. I could be wrong; thankfully,
> I do forget.)
>
> Thanks
> Laszlo
>

Replying to your other email...

> Just to make this patch a bit more tractable, I'd suggest splitting it.

> First, update only the DSC/FDF files. In particular, if you do that
> alongside review/maintainer responsibilities -- that is, for example,
> you create a separate FDF+DSC patch for Bhyve and another FDF+DSC patch
> for Xen --, then your reviewers will thank you for the effort, as they
> won't have to wade through platform DSC+FDF code they don't care about.
>
> Second, removing "OvmfPkg/SataControllerDxe" can be its own patch at the
> very end of the series; and that one need not be CC'd to the various
> platform maintainers. With smaller / more focused patches,
> "GetMaintainer.py" will provide more targeted CC lists.

Thanks, this makes sense. I tried to consolidate the changes into less
patches in v1 due to the amount of patches
I would need to send, but oh well. 12 patches it is! They should be
fairly succinct now.

Also found a bunch of users in edk2-platforms which should be dealt
with before the v2 can go through and get merged. But we're in a
freeze
so *hopefully* there's enough time for everything in edk2-platforms to settle.

Thank you for the thorough review and comments :)

-- 
Pedro

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2023-05-09 16:46 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-08 21:52 [PATCH 0/2] OvmfPkg: Replace the OVMF-specific SataControllerDxe with the generic one Pedro Falcato
2023-05-08 21:52 ` [PATCH 1/2] MdeModulePkg/SataControllerDxe: Remove useless null check Pedro Falcato
2023-05-08 22:28   ` [edk2-devel] " Mike Maslenkin
2023-05-08 22:46     ` Pedro Falcato
2023-05-08 21:52 ` [PATCH 2/2] OvmfPkg: Replace the OVMF-specific SataControllerDxe with a generic one Pedro Falcato
2023-05-09  8:10   ` Laszlo Ersek
2023-05-09  7:36 ` [PATCH 0/2] OvmfPkg: Replace the OVMF-specific SataControllerDxe with the " Gerd Hoffmann
2023-05-09  8:06 ` Laszlo Ersek
2023-05-09 16:46   ` Pedro Falcato

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox