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

["Laszlo Ersek" <lersek@redhat.com>]

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
>