Re: [PATCH v2 (add signed-off-by)] OvmfPkg: Add bhyve support into AcpiTimerLib

["Laszlo Ersek" <lersek@redhat.com>]

Hi Rebecca,

On 03/25/20 21:16, Rebecca Cran wrote:
> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> ---
>  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