[PATCH 2/4] acpi: fadt: support revision 6.0 of the ACPI specification
Update the Fixed ACPI Description Table (FADT) to revision 6.0 of the ACPI
specification adding the field "Hypervisor Vendor Identity".
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."
Signed-off-by: Miguel Luis
Reviewed-by: Ani Sinha
---
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
Re: [External] : Re: [RFC PATCH 2/4] acpi: fadt: support revision 6.0 of the ACPI specification
Hi Ani,
> On 7 Oct 2022, at 04:25, Ani Sinha wrote:
>
>
>
> On Thu, 6 Oct 2022, Miguel Luis 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 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.
>>
>> Signed-off-by: Miguel Luis
>> ---
>> hw/acpi/aml-build.c | 14 +++---
>> hw/arm/virt-acpi-build.c | 10 +-
>> 2 files changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index e6bfac95c7..5258c4ac64 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -31,6 +31,7 @@
>> #include "hw/pci/pci_bus.h"
>> #include "hw/pci/pci_bridge.h"
>> #include "qemu/cutils.h"
>> +#include "qemu/accel.h"
>>
>> static GArray *build_alloc_array(void)
>> {
>> @@ -2070,7 +2071,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 +2194,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) {
>
> <= does not make sense. It should be f->rev == 5.
> The previous code compares f->rev <= 4 since it needs to cover revisions
> 2, 3 and 4.
>
Indeed, that’s right. I will fix.
>> +goto done;
>> +}
>> +
>> +/* Hypervisor Vendor Identity */
>> +build_append_padded_str(tbl, current_accel_name(), 8, '\0');
>
> I do not think the vendor identity should change based on the accelerator.
> The accelerator QEMU uses should be hidden from the guest OS as far as
> possible. I would suggest a string like "QEMU" for vendor ID.
>
Thank you for the suggestion Ani. I will spin the next version with it.
Thanks,
Miguel
>> +
>> +/* 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
>>
>>
Re: [RFC PATCH 2/4] acpi: fadt: support revision 6.0 of the ACPI specification
On Thu, 6 Oct 2022, Miguel Luis 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 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.
>
> Signed-off-by: Miguel Luis
> ---
> hw/acpi/aml-build.c | 14 +++---
> hw/arm/virt-acpi-build.c | 10 +-
> 2 files changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index e6bfac95c7..5258c4ac64 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -31,6 +31,7 @@
> #include "hw/pci/pci_bus.h"
> #include "hw/pci/pci_bridge.h"
> #include "qemu/cutils.h"
> +#include "qemu/accel.h"
>
> static GArray *build_alloc_array(void)
> {
> @@ -2070,7 +2071,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 +2194,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) {
<= does not make sense. It should be f->rev == 5.
The previous code compares f->rev <= 4 since it needs to cover revisions
2, 3 and 4.
> +goto done;
> +}
> +
> +/* Hypervisor Vendor Identity */
> +build_append_padded_str(tbl, current_accel_name(), 8, '\0');
I do not think the vendor identity should change based on the accelerator.
The accelerator QEMU uses should be hidden from the guest OS as far as
possible. I would suggest a string like "QEMU" for vendor ID.
> +
> +/* 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
>
>
[RFC PATCH 2/4] acpi: fadt: support revision 6.0 of the ACPI specification
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 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.
Signed-off-by: Miguel Luis
---
hw/acpi/aml-build.c | 14 +++---
hw/arm/virt-acpi-build.c | 10 +-
2 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index e6bfac95c7..5258c4ac64 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -31,6 +31,7 @@
#include "hw/pci/pci_bus.h"
#include "hw/pci/pci_bridge.h"
#include "qemu/cutils.h"
+#include "qemu/accel.h"
static GArray *build_alloc_array(void)
{
@@ -2070,7 +2071,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 +2194,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, current_accel_name(), 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
