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

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

[Rebecca Cran]

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;
-- 
2.25.2

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

[Laszlo Ersek]

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

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

[Rebecca Cran]

On 3/27/2020 1:01 PM, Laszlo Ersek wrote:
>
> 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.

Thanks! I was wondering if I should try and add new code into OvmfPkg, 
or keep it separate. Also, good point about the commit message: I get 
frustrated when people don't write proper/full messages, so I'm happy 
you called me out on it.


The existing bhyve port removes everything related to QemuFwCfgLib, such 
as calls to QemuFwCfgFindFile in PciHostBridgeLib.c. I'm not sure how I 
should proceed given that there's so much commonality between the ovmf 
and bhyve versions of the file: currently calls to QemuFwCfgFindFile 
don't resolve since references are absent from the .dsc file, so I'm 
wondering if I should re-add it, add a "#ifndef BHYVE" or similar to 
avoid attempting to compile that code, or duplicate the file with that 
code removed?


-- 
Rebecca Cran

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

[Laszlo Ersek]

On 04/09/20 07:26, Rebecca Cran wrote:
> On 3/27/2020 1:01 PM, Laszlo Ersek wrote:
>>
>> 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.
> 
> Thanks! I was wondering if I should try and add new code into OvmfPkg,
> or keep it separate. Also, good point about the commit message: I get
> frustrated when people don't write proper/full messages, so I'm happy
> you called me out on it.
> 
> 
> The existing bhyve port removes everything related to QemuFwCfgLib, such
> as calls to QemuFwCfgFindFile in PciHostBridgeLib.c. I'm not sure how I
> should proceed given that there's so much commonality between the ovmf
> and bhyve versions of the file: currently calls to QemuFwCfgFindFile
> don't resolve since references are absent from the .dsc file, so I'm
> wondering if I should re-add it, add a "#ifndef BHYVE" or similar to
> avoid attempting to compile that code, or duplicate the file with that
> code removed?

I'd recommend the following approaches for your consideration:


(1) Yubo Miao is in the process of factoring out logic that is shared
between ArmVirtPkg's and OvmfPkg's PciHostBridgeLib instances:

http://mid.mail-archive.com/57fd0043-d63a-55b0-9c55-4ca079331885@redhat.com

https://edk2.groups.io/g/devel/message/57062

I proposed that the new lib class be called PciHostBridgeUtilityLib.

You could follow that development, with the intent of building
BhyvePkg's PciHostBridgeLib instance in terms of the new
PciHostBridgeUtilityLib. That would decrease code duplication in BhyvePkg.


(2) If OvmfPkg's PciHostBridgeLib already does the right thing for
BhyvePkg, assuming the

  QemuFwCfgFindFile ("etc/extra-pci-roots", &FwCfgItem, &FwCfgSize)

call returns an error code, then you could introduce a Null instance of
the QemuFwCfgLib class, under OvmfPkg/Library/QemuFwCfgLib. The new INF
file could be called "QemuFwCfgLibNull.inf". There would be a new source
file called "QemuFwCfgLibNull.c", with the following function definitions:

- QemuFwCfgIsAvailable: return constant FALSE
- QemuFwCfgSelectItem, QemuFwCfgReadBytes, QemuFwCfgWriteBytes,
QemuFwCfgSkipBytes: do nothing
- QemuFwCfgRead8, QemuFwCfgRead16, QemuFwCfgRead32, QemuFwCfgRead64:
return 0
- QemuFwCfgFindFile: return RETURN_UNSUPPORTED

Then you could resolve QemuFwCfgLib to this Null instance in the bhyve
DSC file.

(This approach might help you reuse further OvmfPkg modules in BhyvePkg.
A new QemuFwCfgLibNull instance could simplify the existent
"OvmfPkg/OvmfXen.dsc" platform too.)

Thanks
Laszlo