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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#104294): https://edk2.groups.io/g/devel/message/104294
Mute This Topic: https://groups.io/mt/98771779/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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
>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#104353): https://edk2.groups.io/g/devel/message/104353
Mute This Topic: https://groups.io/mt/98771779/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#104420): https://edk2.groups.io/g/devel/message/104420
Mute This Topic: https://groups.io/mt/98771779/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#104340): https://edk2.groups.io/g/devel/message/104340
Mute This Topic: https://groups.io/mt/98771779/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-