From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-74.mimecast.com (us-smtp-delivery-74.mimecast.com [216.205.24.74]) by mx.groups.io with SMTP id smtpd.web11.19927.1585335736125430481 for ; Fri, 27 Mar 2020 12:02:16 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=hwDP3JYC; spf=pass (domain: redhat.com, ip: 216.205.24.74, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1585335735; 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=6WoPHqoXprXZOHLQX+54YoB0sogaYVXV6b91ls5Ldd0=; b=hwDP3JYCU/gQdHU5Lxt7Q72u2LhXzNGVoy9INn5RvHJJr28DYG4E7z8iVX2WY7MDxZzN4g OhgUCG1cY6kXQl/2FxG3xyOQZhhq4KlckDJV7c/d8FEKnNpJK9/4tGiL0pW0tC0b3pavTD WmISs9LT5Mo5aBd/Dy1LTw1p6dXH3fQ= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-239-CoHfNZuvN7GzXY_LsEuXIQ-1; Fri, 27 Mar 2020 15:02:11 -0400 X-MC-Unique: CoHfNZuvN7GzXY_LsEuXIQ-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 4305C1052BA0; Fri, 27 Mar 2020 19:02:10 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-36.ams2.redhat.com [10.36.114.36]) by smtp.corp.redhat.com (Postfix) with ESMTP id 508E05DA75; Fri, 27 Mar 2020 19:02:01 +0000 (UTC) Subject: Re: [PATCH v2 (add signed-off-by)] OvmfPkg: Add bhyve support into AcpiTimerLib To: Rebecca Cran , devel@edk2.groups.io Cc: Jordan Justen , Ard Biesheuvel References: <20200325201652.54298-1-rebecca@bsdio.com> From: "Laszlo Ersek" Message-ID: Date: Fri, 27 Mar 2020 20:01:56 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200325201652.54298-1-rebecca@bsdio.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Hi Rebecca, On 03/25/20 21:16, Rebecca Cran wrote: > Signed-off-by: Rebecca Cran > --- > OvmfPkg/Include/OvmfPlatforms.h | 7 +++++++ > OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c | 3 +++ > OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.c | 4 ++++ > OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.c | 3 +++ > 4 files changed, 17 insertions(+) > > diff --git a/OvmfPkg/Include/OvmfPlatforms.h b/OvmfPkg/Include/OvmfPlatforms.h > index 59459231e8..3d1a8fd8fa 100644 > --- a/OvmfPkg/Include/OvmfPlatforms.h > +++ b/OvmfPkg/Include/OvmfPlatforms.h > @@ -37,4 +37,11 @@ > // > #define ACPI_TIMER_OFFSET 0x8 > > +// > +// bhyve definitions > +// > +#define BHYVE_PCI_DEVICE_ID 0x1275 // NetApp vendor ID > + > +#define BHYVE_ACPI_TIMER_IO_ADDR 0x408 > + > #endif > diff --git a/OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c b/OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c > index aa2b5faf1c..e32260b134 100644 > --- a/OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c > +++ b/OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c > @@ -41,6 +41,9 @@ AcpiTimerLibConstructor ( > // > HostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID); > switch (HostBridgeDevId) { > + case BHYVE_PCI_DEVICE_ID: > + mAcpiTimerIoAddr = BHYVE_ACPI_TIMER_IO_ADDR; > + return RETURN_SUCCESS; > case INTEL_82441_DEVICE_ID: > Pmba = POWER_MGMT_REGISTER_PIIX4 (PIIX4_PMBA); > PmbaAndVal = ~(UINT32)PIIX4_PMBA_MASK; > diff --git a/OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.c b/OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.c > index dd022aceed..2fec105dea 100644 > --- a/OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.c > +++ b/OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.c > @@ -39,6 +39,8 @@ AcpiTimerLibConstructor ( > // > HostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID); > switch (HostBridgeDevId) { > + case BHYVE_PCI_DEVICE_ID: > + return RETURN_SUCCESS; > case INTEL_82441_DEVICE_ID: > Pmba = POWER_MGMT_REGISTER_PIIX4 (PIIX4_PMBA); > PmbaAndVal = ~(UINT32)PIIX4_PMBA_MASK; > @@ -101,6 +103,8 @@ InternalAcpiGetTimerTick ( > // > HostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID); > switch (HostBridgeDevId) { > + case BHYVE_PCI_DEVICE_ID: > + return IoRead32(BHYVE_ACPI_TIMER_IO_ADDR); > case INTEL_82441_DEVICE_ID: > Pmba = POWER_MGMT_REGISTER_PIIX4 (PIIX4_PMBA); > break; > diff --git a/OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.c b/OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.c > index ae976cbe9e..10c41ff7ed 100644 > --- a/OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.c > +++ b/OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.c > @@ -44,6 +44,9 @@ AcpiTimerLibConstructor ( > // > HostBridgeDevId = PcdGet16 (PcdOvmfHostBridgePciDevId); > switch (HostBridgeDevId) { > + case BHYVE_PCI_DEVICE_ID: > + mAcpiTimerIoAddr = BHYVE_ACPI_TIMER_IO_ADDR; > + return RETURN_SUCCESS; > case INTEL_82441_DEVICE_ID: > Pmba = POWER_MGMT_REGISTER_PIIX4 (PIIX4_PMBA); > break; > I'm quite happy about this patch, but perhaps for an unexpected reason: namely, because it showcases how non-intuitive and unpredictable it can be to customize existent code for a new platform. The commit message is unfortunately empty. If we try to write a commit message for this patch, my point (which I'm about to make) will be obvious. The commit message could go like this: On bhyve, the ACPI timer is located at a fixed IO address; it need not be programmed into, nor fetched from, the PMBA -- power management base address -- register of the PCI host bridge. With this commit message, it's clear that, on bhyve: (1) the InternalAcpiGetTimerTick() function can simply be: return IoRead32 (BHYVE_ACPI_TIMER_IO_ADDR); (2) consequently, there is no need for three separate library instances (the above implementation can be put in a BASE library, and it can work in all firmware phases), (3) furthermore, no constructor function is needed. Therefore the right approach is to introduce a new INF file and a new C file, called: BaseAcpiTimerLibBhyve.c BaseAcpiTimerLibBhyve.inf The INF file should be a copy of "BaseAcpiTimerLib.inf", with the following updates: - update the file-top comment, - add your own copyright, - update BASE_NAME - generate a new FILE_GUID with "uuidgen", - set LIBRARY_CLASS to just "TimerLib" (no module type restriction) - remove CONSTRUCTOR - in the [Sources] section, replace "BaseAcpiTimerLib.c" with "BaseAcpiTimerLibBhyve.c" - in the [LibraryClasses] section, remove BaseLib and PciLib. In the "BaseAcpiTimerLibBhyve.c" source file, implement InternalAcpiGetTimerTick() as shown above, under (1). Finally, in the bhyve DSC file, remove all "TimerLib" class resolutions, except the one in the [LibraryClasses] section. In that section, resolve the library class to "BaseAcpiTimerLibBhyve.inf". As a result, the complexity of OvmfPkg grows virtually by zero, and for bhyve, you'll have a trivial *and* functional ACPI timer lib instance. Importantly, the bhyve platform *will* reuse the "AcpiTimerLib.h" and "AcpiTimerLib.c" files verbatim. Regarding the new macros: - BHYVE_PCI_DEVICE_ID should not be introduced at all, at this point. - BHYVE_ACPI_TIMER_IO_ADDR is nice to have, on the other hand! Please create a new header file in the directory "OvmfPkg/Include/IndustryStandard". Use one of the "I440FxPiix4.h" and "Q35MchIch9.h" files as example, because those stand for the QEMU motherboards / chipsets. Content-wise, you need not #define anything else than BHYVE_ACPI_TIMER_IO_ADDR, but the *name* of the file should somehow refer to bhyve. I can imagine the following complete pathnames (just ideas): OvmfPkg/Include/IndustryStandard/Bhyve.h OvmfPkg/Include/IndustryStandard/BhyveBoard.h OvmfPkg/Include/IndustryStandard/BhyveNetApp.h The leading comment in the new header file should make it clear that the header is not a generic dump for "all constants bhyve"; only board / chipset registers belong there. In summary, the patch should: - include the commit message, - introduce three new files: OvmfPkg/Include/IndustryStandard/Bhyve*.h OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLibBhyve.c OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLibBhyve.inf - touch no other files. Thanks! Laszlo