| Message ID | 20221010132300.96935-3-miguel.luis@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ACPI MADT and FADT update according to the ACPI 6.0 spec | expand |
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> > --- > 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 >
> 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: 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 >>
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 > >> >
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);
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> --- hw/acpi/aml-build.c | 13 ++++++++++--- hw/arm/virt-acpi-build.c | 10 +++++----- 2 files changed, 15 insertions(+), 8 deletions(-)