[PATCH 0/1] OvmfPkg/Bhyve: QemuFwCfg support

[PATCH 0/1] OvmfPkg/Bhyve: QemuFwCfg support

[Corvin Köhne]

Hi,

I'm going to add QemuFwCfg support to bhyve. See
https://reviews.freebsd.org/D31578. Therefore, this patch for OVMF is
neccessary to work properly.

There's one open point on that patch and hopefully one of you has more
insights. Qemu has an item called FW_CFG_MAX_CPUS. It looks very similar to
what we need here, but I'm not sure if we can use it safely as Qemu has a
comment about it:

https://github.com/qemu/qemu/blob/0021c4765a6b83e5b09409b75d50c6caaa6971b9/hw/i386/fw_cfg.c#L110-L121

    /* FW_CFG_MAX_CPUS is a bit confusing/problematic on x86:
     *
     * For machine types prior to 1.8, SeaBIOS needs FW_CFG_MAX_CPUS for
     * building MPTable, ACPI MADT, ACPI CPU hotplug and ACPI SRAT table,
     * that tables are based on xAPIC ID and QEMU<->SeaBIOS interface
     * for CPU hotplug also uses APIC ID and not "CPU index".
     * This means that FW_CFG_MAX_CPUS is not the "maximum number of CPUs",
     * but the "limit to the APIC ID values SeaBIOS may see".
     *
     * So for compatibility reasons with old BIOSes we are stuck with
     * "etc/max-cpus" actually being apic_id_limit
     */


Thanks
Corvin

CC: Ard Biesheuvel <ardb+tianocore@kernel.org>
CC: Jiewen Yao <jiewen.yao@intel.com>
CC: Jordan Justen <jordan.l.justen@intel.com>
CC: Gerd Hoffmann <kraxel@redhat.com>
CC: Rebecca Cran <rebecca@bsdio.com>
CC: Peter Grehan <grehan@freebsd.org>
CC: devel@edk2.groups.io
CC: FreeBSD Virtualization <freebsd-virtualization@freebsd.org>

Corvin Köhne (1):
  OvmfPkg/BhyveBhfPkg: add support for QemuFwCfg

 OvmfPkg/Bhyve/AcpiPlatformDxe/AcpiPlatformDxe.inf |  1 +
 OvmfPkg/Bhyve/AcpiPlatformDxe/Bhyve.c             | 41 ++++++++++++++++++++---
 OvmfPkg/Bhyve/BhyveX64.dsc                        |  4 +--
 3 files changed, 40 insertions(+), 6 deletions(-)

-- 
2.11.0

Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff
Registered office: Verl, Germany | Register court: Guetersloh HRA 7075

[PATCH 1/1] OvmfPkg/BhyveBhfPkg: add support for QemuFwCfg

[Corvin Köhne]

From: Corvin Köhne <CorvinK@beckhoff.com>

QemuFwCfg is much more powerful than BhyveFwCtl. Sadly, BhyveFwCtl
decided to use the same IO ports as QemuFwCfg. It's not possible to use
both interfaces simultaneously. So, prefer QemuFwCfg over BhyveFwCtl.

Signed-off-by: Corvin Köhne <c.koehne@beckhoff.com>
CC: Ard Biesheuvel <ardb+tianocore@kernel.org>
CC: Jiewen Yao <jiewen.yao@intel.com>
CC: Jordan Justen <jordan.l.justen@intel.com>
CC: Gerd Hoffmann <kraxel@redhat.com>
CC: Rebecca Cran <rebecca@bsdio.com>
CC: Peter Grehan <grehan@freebsd.org>
CC: devel@edk2.groups.io
CC: FreeBSD Virtualization <freebsd-virtualization@freebsd.org>
---
 OvmfPkg/Bhyve/AcpiPlatformDxe/AcpiPlatformDxe.inf |  1 +
 OvmfPkg/Bhyve/AcpiPlatformDxe/Bhyve.c             | 41 ++++++++++++++++++++---
 OvmfPkg/Bhyve/BhyveX64.dsc                        |  4 +--
 3 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/OvmfPkg/Bhyve/AcpiPlatformDxe/AcpiPlatformDxe.inf b/OvmfPkg/Bhyve/AcpiPlatformDxe/AcpiPlatformDxe.inf
index 595fd055f9..94c65f32dc 100644
--- a/OvmfPkg/Bhyve/AcpiPlatformDxe/AcpiPlatformDxe.inf
+++ b/OvmfPkg/Bhyve/AcpiPlatformDxe/AcpiPlatformDxe.inf
@@ -43,6 +43,7 @@
   MemoryAllocationLib
   OrderedCollectionLib
   PcdLib
+  QemuFwCfgLib
   UefiBootServicesTableLib
   UefiDriverEntryPoint
   UefiLib
diff --git a/OvmfPkg/Bhyve/AcpiPlatformDxe/Bhyve.c b/OvmfPkg/Bhyve/AcpiPlatformDxe/Bhyve.c
index 8e80aa33e1..e216a21bfa 100644
--- a/OvmfPkg/Bhyve/AcpiPlatformDxe/Bhyve.c
+++ b/OvmfPkg/Bhyve/AcpiPlatformDxe/Bhyve.c
@@ -11,6 +11,41 @@
 #include <Library/BaseMemoryLib.h>
 #include <Library/BhyveFwCtlLib.h>
 #include <Library/MemoryAllocationLib.h>
+#include <Library/QemuFwCfgLib.h>             // QemuFwCfgFindFile()
+
+STATIC
+EFI_STATUS
+EFIAPI
+BhyveGetCpuCount (
+  OUT UINT32  *CpuCount
+  )
+{
+  FIRMWARE_CONFIG_ITEM  Item;
+  UINTN                 Size;
+
+  if (QemuFwCfgIsAvailable ()) {
+    if (EFI_ERROR (QemuFwCfgFindFile ("opt/bhyve/hw.ncpu", &Item, &Size))) {
+      return EFI_NOT_FOUND;
+    } else if (Size != sizeof (*CpuCount)) {
+      return EFI_BAD_BUFFER_SIZE;
+    }
+
+    QemuFwCfgSelectItem (Item);
+    QemuFwCfgReadBytes (Size, CpuCount);
+
+    return EFI_SUCCESS;
+  }
+
+  //
+  // QemuFwCfg not available, try BhyveFwCtl.
+  //
+  Size = sizeof (*CpuCount);
+  if (BhyveFwCtlGet ("hw.ncpu", CpuCount, &Size) == RETURN_SUCCESS) {
+    return EFI_SUCCESS;
+  }
+
+  return EFI_UNSUPPORTED;
+}
 
 STATIC
 EFI_STATUS
@@ -23,7 +58,6 @@ BhyveInstallAcpiMadtTable (
   )
 {
   UINT32                                               CpuCount;
-  UINTN                                                cSize;
   UINTN                                                NewBufferSize;
   EFI_ACPI_1_0_MULTIPLE_APIC_DESCRIPTION_TABLE_HEADER  *Madt;
   EFI_ACPI_1_0_PROCESSOR_LOCAL_APIC_STRUCTURE          *LocalApic;
@@ -36,9 +70,8 @@ BhyveInstallAcpiMadtTable (
   ASSERT (AcpiTableBufferSize >= sizeof (EFI_ACPI_DESCRIPTION_HEADER));
 
   // Query the host for the number of vCPUs
-  CpuCount = 0;
-  cSize    = sizeof (CpuCount);
-  if (BhyveFwCtlGet ("hw.ncpu", &CpuCount, &cSize) == RETURN_SUCCESS) {
+  Status = BhyveGetCpuCount (&CpuCount);
+  if (!EFI_ERROR (Status)) {
     DEBUG ((DEBUG_INFO, "Retrieved CpuCount %d\n", CpuCount));
     ASSERT (CpuCount >= 1);
   } else {
diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
index 5fa08bebd7..14070fd6dd 100644
--- a/OvmfPkg/Bhyve/BhyveX64.dsc
+++ b/OvmfPkg/Bhyve/BhyveX64.dsc
@@ -163,8 +163,7 @@
   SecurityManagementLib|MdeModulePkg/Library/DxeSecurityManagementLib/DxeSecurityManagementLib.inf
   UefiUsbLib|MdePkg/Library/UefiUsbLib/UefiUsbLib.inf
   SerializeVariablesLib|OvmfPkg/Library/SerializeVariablesLib/SerializeVariablesLib.inf
-  QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibNull.inf
-  QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/BaseQemuFwCfgS3LibNull.inf
+  QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxeLib.inf
   BhyveFwCtlLib|OvmfPkg/Library/BhyveFwCtlLib/BhyveFwCtlLib.inf
   VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf
   MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLib.inf
@@ -355,6 +354,7 @@
 !endif
   PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
   MpInitLib|UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.inf
+  QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
 
 [LibraryClasses.common.UEFI_APPLICATION]
   PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
-- 
2.11.0

Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff
Registered office: Verl, Germany | Register court: Guetersloh HRA 7075

Re: [edk2-devel] [PATCH 1/1] OvmfPkg/BhyveBhfPkg: add support for QemuFwCfg

[Gerd Hoffmann]

On Tue, Mar 29, 2022 at 08:54:37AM +0200, Corvin Köhne wrote:
> From: Corvin Köhne <CorvinK@beckhoff.com>
> 
> QemuFwCfg is much more powerful than BhyveFwCtl. Sadly, BhyveFwCtl
> decided to use the same IO ports as QemuFwCfg. It's not possible to use
> both interfaces simultaneously. So, prefer QemuFwCfg over BhyveFwCtl.

Hmm, so QemuFwCfg and BhyveFwCtl are incompatible but use the same
ports?  I guess the signature used is different then so the guest
has a chance to figure which one is active?

Assuming the above is correct the patch looks sane.
Acked-by: Gerd Hoffmann <kraxel@redhat.com>

take care,
  Gerd

Re: [edk2-devel] [PATCH 1/1] OvmfPkg/BhyveBhfPkg: add support for QemuFwCfg

[Corvin Köhne]

Hi Gerd,

> Hmm, so QemuFwCfg and BhyveFwCtl are incompatible but use the same
> ports?  I guess the signature used is different then so the guest
> has a chance to figure which one is active?

Yes, BhyveFwCtl uses another signature.


Best regards
Corvin


Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff
Registered office: Verl, Germany | Register court: Guetersloh HRA 7075

Re: [edk2-devel] [PATCH 0/1] OvmfPkg/Bhyve: QemuFwCfg support

[Gerd Hoffmann]

On Tue, Mar 29, 2022 at 08:54:36AM +0200, Corvin Köhne wrote:
> Hi,
> 
> I'm going to add QemuFwCfg support to bhyve. See
> https://reviews.freebsd.org/D31578. Therefore, this patch for OVMF is
> neccessary to work properly.
> 
> There's one open point on that patch and hopefully one of you has more
> insights. Qemu has an item called FW_CFG_MAX_CPUS. It looks very similar to
> what we need here, but I'm not sure if we can use it safely as Qemu has a
> comment about it:
> 
> https://github.com/qemu/qemu/blob/0021c4765a6b83e5b09409b75d50c6caaa6971b9/hw/i386/fw_cfg.c#L110-L121
> 
>     /* FW_CFG_MAX_CPUS is a bit confusing/problematic on x86:
>      *
>      * For machine types prior to 1.8, SeaBIOS needs FW_CFG_MAX_CPUS for
>      * building MPTable, ACPI MADT, ACPI CPU hotplug and ACPI SRAT table,
>      * that tables are based on xAPIC ID and QEMU<->SeaBIOS interface
>      * for CPU hotplug also uses APIC ID and not "CPU index".
>      * This means that FW_CFG_MAX_CPUS is not the "maximum number of CPUs",
>      * but the "limit to the APIC ID values SeaBIOS may see".
>      *
>      * So for compatibility reasons with old BIOSes we are stuck with
>      * "etc/max-cpus" actually being apic_id_limit
>      */

There is FW_CFG_NB_CPUS + FW_CFG_MAX_CPUS.  ovmf uses different names,
see OvmfPkg/Include/IndustryStandard/QemuFwCfg.h

PlatformPei for qemu uses QemuFwCfgItemSmpCpuCount aka FW_CFG_NB_CPUS,
which is the number of cpus which are online.

I think FW_CFG_MAX_CPUS is basically unused these days.  It played a
role back when seabios created the acpi tables for cpu hotplug as
described in the comment above.  In qemu 2.0 & newer the acpi tables are
generated by qemu instead.  The firmware just downloads them from fw_cfg
and installs them for the OS, it doesn't need to know virtual machine
configuration details any more.

HTH,
  Gerd

Re: [edk2-devel] [PATCH 0/1] OvmfPkg/Bhyve: QemuFwCfg support

[Corvin Köhne]

Hi Gerd,

> There is FW_CFG_NB_CPUS + FW_CFG_MAX_CPUS.  ovmf uses different names,
> see OvmfPkg/Include/IndustryStandard/QemuFwCfg.h
>
> PlatformPei for qemu uses QemuFwCfgItemSmpCpuCount aka FW_CFG_NB_CPUS,
> which is the number of cpus which are online.
>
> I think FW_CFG_MAX_CPUS is basically unused these days.  It played a
> role back when seabios created the acpi tables for cpu hotplug as
> described in the comment above.  In qemu 2.0 & newer the acpi tables are
> generated by qemu instead.  The firmware just downloads them from fw_cfg
> and installs them for the OS, it doesn't need to know virtual machine
> configuration details any more.

The FwCfgItem of this patch is used by bhyve to build the MADT. So, it's
similar to the use case of FW_CFG_MAX_CPUS. At the moment, I'm using
an additional bhyve specific FwCfgItem. I just want to ask, if it makes sense
to use FW_CFG_MAX_CPUS to avoid two items for the same purpose or to
keep it as is.


Best regards
Corvin


Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff
Registered office: Verl, Germany | Register court: Guetersloh HRA 7075

Re: [edk2-devel] [PATCH 0/1] OvmfPkg/Bhyve: QemuFwCfg support

[Gerd Hoffmann]

On Tue, Mar 29, 2022 at 09:57:40AM +0000, Corvin Köhne wrote:
> Hi Gerd,
> 
> > There is FW_CFG_NB_CPUS + FW_CFG_MAX_CPUS.  ovmf uses different names,
> > see OvmfPkg/Include/IndustryStandard/QemuFwCfg.h
> >
> > PlatformPei for qemu uses QemuFwCfgItemSmpCpuCount aka FW_CFG_NB_CPUS,
> > which is the number of cpus which are online.
> >
> > I think FW_CFG_MAX_CPUS is basically unused these days.  It played a
> > role back when seabios created the acpi tables for cpu hotplug as
> > described in the comment above.  In qemu 2.0 & newer the acpi tables are
> > generated by qemu instead.  The firmware just downloads them from fw_cfg
> > and installs them for the OS, it doesn't need to know virtual machine
> > configuration details any more.
> 
> The FwCfgItem of this patch is used by bhyve to build the MADT. So, it's
> similar to the use case of FW_CFG_MAX_CPUS. At the moment, I'm using
> an additional bhyve specific FwCfgItem. I just want to ask, if it makes sense
> to use FW_CFG_MAX_CPUS to avoid two items for the same purpose or to
> keep it as is.

Given that FW_CFG_MAX_CPUS is unused on qemu these days it is unlikely
that reusing it causes problems.  IIRC the problems mentioned in the
comment only matter with VMs having > 255 vCPUs because somewhere only
one byte was used for the cpu / apic index.

But I think I'd tend to keep the bhyve-specific behavior nevertheless,
so you don't have to worry about qemu quirks.

Or go the qemu route and generate the acpi tables on the host instead.
When you generate the acpi tables in the guest firmware you always have
the problem that you need to pass all the virtual machine configuration
information needed to generate the tables to the firmware.  The
information needed changes over time when new features are added, which
requires protocol updates, which in turn requires lockstep updates of
hypervisor and firmware to deploy the new features ...

HTH & take care,
  Gerd

Re: [edk2-devel] [PATCH 0/1] OvmfPkg/Bhyve: QemuFwCfg support

[Corvin Köhne]

Hi Gerd,

> But I think I'd tend to keep the bhyve-specific behavior nevertheless,
> so you don't have to worry about qemu quirks.

Ok. I will leave it as is.

> Or go the qemu route and generate the acpi tables on the host instead.
> When you generate the acpi tables in the guest firmware you always have
> the problem that you need to pass all the virtual machine configuration
> information needed to generate the tables to the firmware.  The
> information needed changes over time when new features are added, which
> requires protocol updates, which in turn requires lockstep updates of
> hypervisor and firmware to deploy the new features ...

Personally, I would like to use plain OVMF without any bhyve specific patches
as firmware for bhyve. So, I want to go the qemu route but there's some more
work to do. I already took a look at how qemu creates ACPI tables but don't
understand it yet. Would be very grateful if you or someone else could help
me with that. If someone knows where to find more information about it,
it would also be helpful.

As first step, I'm going to implement FwCfg support without changing any
behaviour.


Thanks,
Corvin


Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff
Registered office: Verl, Germany | Register court: Guetersloh HRA 7075

Re: [edk2-devel] [PATCH 0/1] OvmfPkg/Bhyve: QemuFwCfg support

[Gerd Hoffmann]

  Hi,

> Personally, I would like to use plain OVMF without any bhyve specific patches
> as firmware for bhyve. So, I want to go the qemu route but there's some more
> work to do. I already took a look at how qemu creates ACPI tables but don't
> understand it yet. Would be very grateful if you or someone else could help
> me with that. If someone knows where to find more information about it,
> it would also be helpful.

I think the best documentation you can find is
hw/acpi/bios-linker-loader.c in the qemu source tree.

It's a mini-language telling the firmware about the allocations needed,
about pointers (xsdt references for example) so tables are relocatable,
about checksum needing updates etc.

I think qemu generates everything meanwhile, but it should be possible
to get started with iasl-compiled blobs for tables which don't change,
i.e. start with a static dsdt so you don't need a aml generator for the
first revision.

take care,
  Gerd