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

["Pedro Falcato" <pedro.falcato@gmail.com>]

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