From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web10.26565.1683619615742304730 for ; Tue, 09 May 2023 01:06:55 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=CwDC5t69; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1683619614; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Q7uNlaTI30o+wI+AjaIqSYnWoBAxmr/FcJM5r8wda4k=; b=CwDC5t69RVM9dJICgk7Sbw8wWRhIwOFYLvHHW8qWkMibwJ1yBaKbplAVBGoOj6RqgxO2Ax G1vbynqFFaAh9oVI4ZrN++CigTAzSmQH6wdG2390JQn3RUcKkLMJKXL3Cw5DzWnV+2vS87 Y2RswV2mFTRQFR53cyjn2YCc0qvP4X0= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-578-jdNvYbKKP-SSr9UhZd3N9A-1; Tue, 09 May 2023 04:06:51 -0400 X-MC-Unique: jdNvYbKKP-SSr9UhZd3N9A-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 7AC5E101A55C; Tue, 9 May 2023 08:06:50 +0000 (UTC) Received: from [10.39.192.152] (unknown [10.39.192.152]) by smtp.corp.redhat.com (Postfix) with ESMTPS id BC12340C2063; Tue, 9 May 2023 08:06:47 +0000 (UTC) Message-ID: Date: Tue, 9 May 2023 10:06:46 +0200 MIME-Version: 1.0 Subject: Re: [PATCH 0/2] OvmfPkg: Replace the OVMF-specific SataControllerDxe with the generic one To: Pedro Falcato , devel@edk2.groups.io Cc: 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 References: <20230508215246.217002-1-pedro.falcato@gmail.com> From: "Laszlo Ersek" In-Reply-To: <20230508215246.217002-1-pedro.falcato@gmail.com> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 > Cc: Jiewen Yao > Cc :Jordan Justen > Cc: Gerd Hoffmann > Cc: Erdem Aktas > Cc: James Bottomley > Cc: Min Xu > Cc: Tom Lendacky > Cc: Michael Roth > Cc: Rebecca Cran > Cc: Peter Grehan > Cc: Corvin Köhne > Cc: Sebastien Boeuf > Cc: Anthony Perard > Cc: Julien Grall > Cc: Laszlo Ersek > > 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 >