Re: [RFC PATCH v2 2/4] acpi: fadt: support revision 6.0 of the ACPI specification

[Ani Sinha]

On Tue, Oct 11, 2022 at 4:33 PM Miguel Luis <miguel.luis@oracle.com> wrote:
>
>
> > On 11 Oct 2022, at 05:02, Ani Sinha <ani@anisinha.ca> wrote:
> >
> > On Mon, Oct 10, 2022 at 6:53 PM Miguel Luis <miguel.luis@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.luis@oracle.com>
> >
> > Reviewed-by: Ani Sinha <ani@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
> >>
>