From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pj1-f48.google.com (mail-pj1-f48.google.com [209.85.216.48]) by mx.groups.io with SMTP id smtpd.web10.38163.1683650803765641626 for ; Tue, 09 May 2023 09:46:43 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20221208 header.b=pxV8pdKM; spf=pass (domain: gmail.com, ip: 209.85.216.48, mailfrom: pedro.falcato@gmail.com) Received: by mail-pj1-f48.google.com with SMTP id 98e67ed59e1d1-24e16918323so4325307a91.2 for ; Tue, 09 May 2023 09:46:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1683650803; x=1686242803; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=nYcsK2LO/Dkf/c78a21ILALdrr5CGlHUB03xCkm9yss=; b=pxV8pdKM5wXnsGBqMKR4I1S1d/ZYlXBxy/ih7U6i/3qiTuvGQLGcX1mPni2cqB/AxO 16NJWlhoEV2Sbmn0PvriO1bq2Tu6XcxUz2Gev3PgkvPqG/ZyGCaa1xDHZ/KL9lehaKvQ lR5YytK0u8pwo8IowCq1goIPcTDbgPJ1S6shOATIbZZ+NqE/mPt/4ONci+F3x7392CPY mz5828vcZ0LIMf29VrB7blcBBzNjyp9AfZbq0bjFOKvveWQHVkWEPXDa2NEeBw8tJXCh oqtvDeg4mmZj6cZFH/RieObUf83HzZVjrDr4xI2jzWOyeRSXFz582UOfUFZosPM0B3om PBhg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1683650803; x=1686242803; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=nYcsK2LO/Dkf/c78a21ILALdrr5CGlHUB03xCkm9yss=; b=EbzTzWY9kCgTU97PySOyKxeZV9LFXEDV/1RCe36/qwRJy4r3Gyc6D/EBR6YaniSGlI Loci3b/yXQ5LJ+V5gTgX2KY6Eu+Krnx3FXD2efjlaUXY5ditDSoc66b8Nyex6OgEpDt2 tJ1sp/+wtYknb09Cw1Pat9wZGa9vTRwE4hO91b0GiNjB4raJZeOtttdNXOFyNiTL+FvA FJzORPyRscraGyj/UqszSrHVbYygyop8yi/M07ni2PuiDwWRyYTjs2SpKF/zPHyHRGlI wRpuCMAKJWKif+mFtHm2KPdKRYu7lLd9aZJJ1z/Z00D58kz5rBiCn5TSgyhsp354+l4D Korg== X-Gm-Message-State: AC+VfDwKUQndVheDxfp5kEpPIv19+LtC1f9I6uDOMlU9NPkuRXCPXgfD uNXKW2H110ezyCamcmMEdO6jF1oVAQ2DIJUEUw0= X-Google-Smtp-Source: ACHHUZ4bvih6bzIWbva+SuQS979IVcu2opVmteRy4Dy47T1RD/epfL0qUSFV/xZQkqZFESvBwapMNtSpLkVKQB6YYwE= X-Received: by 2002:a17:90a:a097:b0:24c:c75:2531 with SMTP id r23-20020a17090aa09700b0024c0c752531mr14451970pjp.37.1683650803158; Tue, 09 May 2023 09:46:43 -0700 (PDT) MIME-Version: 1.0 References: <20230508215246.217002-1-pedro.falcato@gmail.com> In-Reply-To: From: "Pedro Falcato" Date: Tue, 9 May 2023 17:46:32 +0100 Message-ID: Subject: Re: [PATCH 0/2] OvmfPkg: Replace the OVMF-specific SataControllerDxe with the generic one To: Laszlo Ersek Cc: devel@edk2.groups.io, Ard Biesheuvel , Jiewen Yao , Gerd Hoffmann , Erdem Aktas , James Bottomley , Min Xu , Tom Lendacky , Michael Roth , Rebecca Cran , Peter Grehan , =?UTF-8?Q?Corvin_K=C3=B6hne?= , Sebastien Boeuf , Anthony Perard , Julien Grall Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, May 9, 2023 at 9:06=E2=80=AFAM Laszlo Ersek wro= te: > > 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 u= se > it, it should either reference it directly from under DuetPkg, or cop= y 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 sett= le. Thank you for the thorough review and comments :) --=20 Pedro