On Tue, Oct 11, 2022 at 4:33 PM Miguel Luis <miguel.l...@oracle.com> wrote:
>
>
> > On 11 Oct 2022, at 05:02, Ani Sinha <a...@anisinha.ca> wrote:
> >
> > On Mon, Oct 10, 2022 at 6:53 PM Miguel Luis <miguel.l...@oracle.com> wrote:
> >>
> >> Update the Fixed ACPI Description Table (FADT) to revision 6.0 of the ACPI
> >> specification adding the field "Hypervisor Vendor Identity" that was
> >> missing.
> >>
> >> This field's description states the following: "64-bit identifier of
> >> hypervisor
> >> vendor. All bytes in this field are considered part of the vendor identity.
> >> These identifiers are defined independently by the vendors themselves,
> >> usually following the name of the hypervisor product. Version information
> >> should NOT be included in this field - this shall simply denote the
> >> vendor's
> >> name or identifier. Version information can be communicated through a
> >> supplemental vendor-specific hypervisor API. Firmware implementers would
> >> place zero bytes into this field, denoting that no hypervisor is present in
> >> the actual firmware."
> >>
> >> Hereupon, what should a valid identifier of an Hypervisor Vendor ID be and
> >> where should QEMU provide that information?
> >>
> >> On the v1 [1] of this RFC there's the suggestion of having this information
> >> in sync by the current acceleration name. This also seems to imply that
> >> QEMU,
> >> which generates the FADT table, and the FADT consumer need to be in sync
> >> with
> >> the values of this field.
> >>
> >> This version follows Ani Sinha's suggestion [2] of using "QEMU" for the
> >> hypervisor vendor ID.
> >>
> >> [1]: https://lists.nongnu.org/archive/html/qemu-devel/2022-10/msg00911.html
> >> [2]: https://lists.nongnu.org/archive/html/qemu-devel/2022-10/msg00989.html
> >>
> >> Signed-off-by: Miguel Luis <miguel.l...@oracle.com>
> >
> > Reviewed-by: Ani Sinha <a...@anisinha.ca>
> >
>
> Thank you Ani. In the meanwhile, taking the description part of: “Firmware
> implementers would place zero bytes into this field, denoting that no
> hypervisor is present in the actual firmware.", I reached to something along
> the lines below:
That line is meant for hardware vendors when shipping bioses with
physical HW. All VMs run with QEMU are run from a hypervisor
environment.
>
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 42feb4d4d7..e719afe0cb 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -2198,7 +2198,11 @@ void build_fadt(GArray *tbl, BIOSLinker *linker, const
> AcpiFadtData *f,
> }
>
> /* Hypervisor Vendor Identity */
> - build_append_padded_str(tbl, "QEMU", 8, '\0');
> + if (f->hyp_is_present) {
> + build_append_padded_str(tbl, "QEMU", 8, '\0');
> + } else {
> + build_append_int_noprefix(tbl, 0, 8);
> + }
>
> /* TODO: extra fields need to be added to support revisions above rev6 */
> assert(f->rev == 6);
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 72bb6f61a5..d238ce2b88 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -818,6 +818,7 @@ static void build_fadt_rev6(GArray *table_data,
> BIOSLinker *linker,
> .minor_ver = 0,
> .flags = 1 << ACPI_FADT_F_HW_REDUCED_ACPI,
> .xdsdt_tbl_offset = &dsdt_tbl_offset,
> + .hyp_is_present = vms->virt && (kvm_enabled() || hvf_enabled()),
> };
>
> switch (vms->psci_conduit) {
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index 2b42e4192b..2aff5304af 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -79,7 +79,7 @@ typedef struct AcpiFadtData {
> uint16_t arm_boot_arch; /* ARM_BOOT_ARCH */
> uint16_t iapc_boot_arch; /* IAPC_BOOT_ARCH */
> uint8_t minor_ver; /* FADT Minor Version */
> -
> + bool hyp_is_present;
> /*
> * respective tables offsets within ACPI_BUILD_TABLE_FILE,
> * NULL if table doesn't exist (in that case field's value
>
> Any thoughts on this?
>
> Thanks
> Miguel
>
> >> ---
> >> hw/acpi/aml-build.c | 13 ++++++++++---
> >> hw/arm/virt-acpi-build.c | 10 +++++-----
> >> 2 files changed, 15 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> >> index e6bfac95c7..42feb4d4d7 100644
> >> --- a/hw/acpi/aml-build.c
> >> +++ b/hw/acpi/aml-build.c
> >> @@ -2070,7 +2070,7 @@ void build_pptt(GArray *table_data, BIOSLinker
> >> *linker, MachineState *ms,
> >> acpi_table_end(linker, &table);
> >> }
> >>
> >> -/* build rev1/rev3/rev5.1 FADT */
> >> +/* build rev1/rev3/rev5.1/rev6.0 FADT */
> >> void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
> >> const char *oem_id, const char *oem_table_id)
> >> {
> >> @@ -2193,8 +2193,15 @@ void build_fadt(GArray *tbl, BIOSLinker *linker,
> >> const AcpiFadtData *f,
> >> /* SLEEP_STATUS_REG */
> >> build_append_gas_from_struct(tbl, &f->sleep_sts);
> >>
> >> - /* TODO: extra fields need to be added to support revisions above
> >> rev5 */
> >> - assert(f->rev == 5);
> >> + if (f->rev == 5) {
> >> + goto done;
> >> + }
> >> +
> >> + /* Hypervisor Vendor Identity */
> >> + build_append_padded_str(tbl, "QEMU", 8, '\0');
> >> +
> >> + /* TODO: extra fields need to be added to support revisions above
> >> rev6 */
> >> + assert(f->rev == 6);
> >>
> >> done:
> >> acpi_table_end(linker, &table);
> >> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> >> index 9b3aee01bf..72bb6f61a5 100644
> >> --- a/hw/arm/virt-acpi-build.c
> >> +++ b/hw/arm/virt-acpi-build.c
> >> @@ -809,13 +809,13 @@ build_madt(GArray *table_data, BIOSLinker *linker,
> >> VirtMachineState *vms)
> >> }
> >>
> >> /* FADT */
> >> -static void build_fadt_rev5(GArray *table_data, BIOSLinker *linker,
> >> +static void build_fadt_rev6(GArray *table_data, BIOSLinker *linker,
> >> VirtMachineState *vms, unsigned
> >> dsdt_tbl_offset)
> >> {
> >> - /* ACPI v5.1 */
> >> + /* ACPI v6.0 */
> >> AcpiFadtData fadt = {
> >> - .rev = 5,
> >> - .minor_ver = 1,
> >> + .rev = 6,
> >> + .minor_ver = 0,
> >> .flags = 1 << ACPI_FADT_F_HW_REDUCED_ACPI,
> >> .xdsdt_tbl_offset = &dsdt_tbl_offset,
> >> };
> >> @@ -945,7 +945,7 @@ void virt_acpi_build(VirtMachineState *vms,
> >> AcpiBuildTables *tables)
> >>
> >> /* FADT MADT PPTT GTDT MCFG SPCR DBG2 pointed to by RSDT */
> >> acpi_add_table(table_offsets, tables_blob);
> >> - build_fadt_rev5(tables_blob, tables->linker, vms, dsdt);
> >> + build_fadt_rev6(tables_blob, tables->linker, vms, dsdt);
> >>
> >> acpi_add_table(table_offsets, tables_blob);
> >> build_madt(tables_blob, tables->linker, vms);
> >> --
> >> 2.37.3
> >>
>
