hw: acpi: RSDP fixes and refactoring
v2 cover hw: acpi: RSDP fixes and refactoring
This patch serie fixes a couple of RSDP checksum related issues:
- On RSDP rev2, we are not adding the extended checksum and computing the checksum on the full table instead of the first 20 bytes.
- On RSDP rev1, we are computing the checksum on 36 bytes instead of 20. We're lucky enough that this is only adding zeroes to the checksum.
A guest Linux kernel does not seem to care about RSDP checksums, so those 2 fixes are mostly for correctness sake.
Any machine type that generates rev2 RSDP will see its RSDP table modified but since x86 builds RSDP v1 and all ACPI tests only run on either pc or q35, the ACPI tests tables are not affected by this fix.
The serie also extends the ARM virt ACPI RSDP build routine to support both RSDP v1 and v2, in order to share this code between x86 and aarch64. While extending, we also convert the routine to the latest build_append_foo() API. The new implementation is a closer reflection of the ACPI spec itself, is endian agnostic and allows for getting rid of the AcpiRsdpDescriptor structure.
v1 -> v2:
* Add missing SOBs.
* Rephrase some commit messages according to the maintainer's input.
* Replace ACPI spec definitions with hard coded constants.
* Replace init_rsdp_data() helper with open coded rsdp variable init.
* Rename acpi_find_[rx]sdt_address to acpi_get_[rx]sdt_address.
Igor Mammedov (2):
hw: arm: acpi: Fix incorrect checksums in RSDP
hw: i386: Use correct RSDT length for checksum
Samuel Ortiz (6):
hw: acpi: The RSDP build API can return void
hw: arm: Carry RSDP specific data through AcpiRsdpData
hw: arm: Convert the RSDP build to the buid_append_foo() API
hw: arm: Support both legacy and current RSDP build
hw: acpi: Export and share the ARM RSDP build
hw: acpi: Remove AcpiRsdpDescriptor and fix tests
include/hw/acpi/acpi-defs.h | 19 ++++++-------- include/hw/acpi/aml-build.h | 2 ++ tests/acpi-utils.h | 5 +++- hw/acpi/aml-build.c | 50 +++++++++++++++++++++++++++++++++++++ hw/arm/virt-acpi-build.c | 40 +++++++---------------------- hw/i386/acpi-build.c | 36 +++++++------------------- tests/acpi-utils.c | 46 ++++++++++++++++++++++++++++------ tests/bios-tables-test.c | 27 ++++++++++++++------ tests/vmgenid-test.c | 8 +++--- 9 files changed, 143 insertions(+), 90 deletions(-)
Signature · 1 line
2.19.2
PATCH v2 1/8 hw: acpi: The RSDP build API can return void
For both x86 and ARM architectures, the internal RSDP build API can return void as the current return value is unused.
--- hw/arm/virt-acpi-build.c | 4 +--- hw/i386/acpi-build.c | 4 +--- 2 files changed, 2 insertions(+), 6 deletions(-)
hw/arm/virt-acpi-build.c +1 -3
@@ -367,7 +367,7 @@ static void acpi_dsdt_add_power_button(Aml *scope) }/* RSDP */ -static GArray * +static void build_rsdp (GArray *rsdp_table,BIOSLinker *linker,unsigned xsdt_tbl_offset) {AcpiRsdpDescriptor *rsdp =acpi_data_push (rsdp_table,sizeof *rsdp);
@@ -392,8 +392,6 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)bios_linker_loader_add_checksum (linker,ACPI_BUILD_RSDP_FILE , (char *)rsdp - rsdp_table->data,sizeof *rsdp, (char *)&rsdp->checksum - rsdp_table->data); - -return rsdp_table; } static void
hw/i386/acpi-build.c +1 -3
@@ -2547,7 +2547,7 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker) "IVRS", table_data->len - iommu_start, 1, NULL, NULL); } -static GArray * +static void build_rsdp (GArray *rsdp_table,BIOSLinker *linker,unsigned rsdt_tbl_offset) {AcpiRsdpDescriptor *rsdp =acpi_data_push (rsdp_table,sizeof *rsdp);
@@ -2569,8 +2569,6 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned rsdt_tbl_offset)bios_linker_loader_add_checksum (linker,ACPI_BUILD_RSDP_FILE , (char *)rsdp - rsdp_table->data,sizeof *rsdp, (char *)&rsdp->checksum - rsdp_table->data); - -return rsdp_table; } typedef
Signature · 1 line
2.19.2
PATCH v2 2/8 hw: arm: acpi: Fix incorrect checksums in RSDP
In reply to Samuel OrtizFrom: Igor Mammedov <[email protected]>
When RSDP table was introduced (d4bec5d87), we calculated only legacy checksum, and that was incorrect as it
- specified rev=2 and forgot about extended checksum.
- legacy checksum calculated on full table instead of the 1st 20 bytes
Fix it by adding extended checksum calculation and using correct size for legacy checksum.
While at it use explicit constants to specify sub/full tables
sizes instead of relying on AcpiRsdpDescriptor size and fields offsets. The follow up commits will convert this table to build_append_int_noprefix() API, will use constants anyway and remove unused AcpiRsdpDescriptor structure.
Based on "[PATCH v5 05/24] hw: acpi: Implement XSDT support for RSDP" by Samuel Ortiz, who did it right in his impl.
--- hw/arm/virt-acpi-build.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
hw/arm/virt-acpi-build.c +6 -1
@@ -390,8 +390,13 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)/* Checksum to be filled by Guest linker */ bios_linker_loader_add_checksum (linker,ACPI_BUILD_RSDP_FILE , - (char *)rsdp - rsdp_table->data,sizeof *rsdp, + (char *)rsdp - rsdp_table->data,20 /* ACPI rev 1.0 RSDP size */ , (char *)&rsdp->checksum - rsdp_table->data); + +/* Extended checksum to be filled by Guest linker */ +bios_linker_loader_add_checksum (linker,ACPI_BUILD_RSDP_FILE , + (char *)rsdp - rsdp_table->data,36 /* ACPI rev 2.0 RSDP size */ , + (char *)&rsdp->extended_checksum - rsdp_table->data); } static void
Signature · 1 line
2.19.2
PATCH v2 3/8 hw: i386: Use correct RSDT length for checksum
In reply to Samuel OrtizFrom: Igor Mammedov <[email protected]>
AcpiRsdpDescriptor describes revision 2 RSDP table so using sizeof(*rsdp) for checksum calculation isn't correct since we are adding extra 16 bytes. But acpi_data_push() zeroes out table, so just by luck we are summing up exta zeros which still yelds correct checksum.
Fix it up by explicitly stating table size instead of using pointer arithmetics on stucture.
PS:
Extra 16 bytes are still wasted, but droping them will break migration for machines older than 2.3 due to size mismatch, for 2.3 and older it's not an issue since they are using resizable memory regions (a1666142d) for ACPI blobs. So keep wasting memory to avoid breaking old machines.
--- hw/i386/acpi-build.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
hw/i386/acpi-build.c +6 -1
@@ -2550,6 +2550,11 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker)static void build_rsdp (GArray *rsdp_table,BIOSLinker *linker,unsigned rsdt_tbl_offset) { +/* AcpiRsdpDescriptor describes revision 2 RSDP table and as result we +* allocate extra 16 bytes for pc/q35 RSDP rev1 as well. Keep extra 16 bytes +* wasted to make sure we won't breake migration for machine types older +* than 2.3 due to size mismatch. +*/ AcpiRsdpDescriptor *rsdp =acpi_data_push (rsdp_table,sizeof *rsdp);unsigned rsdt_pa_size =sizeof (rsdp->rsdt_physical_address); unsigned rsdt_pa_offset =
@@ -2567,7 +2572,7 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned rsdt_tbl_offset)/* Checksum to be filled by Guest linker */ bios_linker_loader_add_checksum (linker,ACPI_BUILD_RSDP_FILE , - (char *)rsdp - rsdp_table->data,sizeof *rsdp, + (char *)rsdp - rsdp_table->data,20 /* ACPI rev 1.0 RSDP size */ , (char *)&rsdp->checksum - rsdp_table->data); }
Signature · 1 line
2.19.2
PATCH v2 4/8 hw: arm: Carry RSDP specific data through AcpiRsdpData
In reply to Samuel OrtizThat will allow us to generalize the ARM build_rsdp() routine to support both legacy RSDP (The current i386 implementation) and extended RSDP (The ARM implementation).
--- include/hw/acpi/acpi-defs.h | 8 ++++++++ hw/arm/virt-acpi-build.c | 18 +++++++++++++----- 2 files changed, 21 insertions(+), 5 deletions(-)
include/hw/acpi/acpi-defs.h +8 -0
@@ -53,6 +53,14 @@ struct AcpiRsdpDescriptor { /* Root System Descriptor Pointer */ }QEMU_PACKED ;typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor ; +typedef struct AcpiRsdpData { +uint8_t oem_id[6 ];/* OEM identification */ +uint8_t revision;/* Must be 0 for 1.0, 2 for 2.0 */ + +unsigned *rsdt_tbl_offset; +unsigned *xsdt_tbl_offset; +}AcpiRsdpData ; +/* Table structure from Linux kernel (the ACPI tables are under the BSD license) */
hw/arm/virt-acpi-build.c +13 -5
@@ -368,7 +368,7 @@ static void acpi_dsdt_add_power_button(Aml *scope)/* RSDP */ static void -build_rsdp (GArray *rsdp_table,BIOSLinker *linker,unsigned xsdt_tbl_offset) +build_rsdp (GArray *rsdp_table,BIOSLinker *linker,AcpiRsdpData *rsdp_data) {AcpiRsdpDescriptor *rsdp =acpi_data_push (rsdp_table,sizeof *rsdp);unsigned xsdt_pa_size =sizeof (rsdp->xsdt_physical_address);
@@ -379,14 +379,14 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset) true /* fseg memory */);memcpy (&rsdp->signature,"RSD PTR " ,sizeof (rsdp->signature)); -memcpy (rsdp->oem_id,ACPI_BUILD_APPNAME6 ,sizeof (rsdp->oem_id)); +memcpy (rsdp->oem_id, rsdp_data->oem_id,sizeof (rsdp->oem_id)); rsdp->length =cpu_to_le32 (sizeof (*rsdp)); - rsdp->revision =0x02 ; + rsdp->revision = rsdp_data->revision;/* Address to be filled by Guest linker */ bios_linker_loader_add_pointer (linker,ACPI_BUILD_RSDP_FILE , xsdt_pa_offset, xsdt_pa_size, -ACPI_BUILD_TABLE_FILE , xsdt_tbl_offset); +ACPI_BUILD_TABLE_FILE , *rsdp_data->xsdt_tbl_offset);/* Checksum to be filled by Guest linker */ bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
@@ -857,7 +857,15 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)build_xsdt (tables_blob, tables->linker, table_offsets,NULL ,NULL );/* RSDP is in FSEG memory, so allocate it separately */ -build_rsdp (tables->rsdp, tables->linker, xsdt); + { +AcpiRsdpData rsdp_data = { + .revision =2 , + .oem_id =ACPI_BUILD_APPNAME6 , + .xsdt_tbl_offset = &xsdt, + .rsdt_tbl_offset =NULL , + }; +build_rsdp (tables->rsdp, tables->linker, &rsdp_data); + }/* Cleanup memory that's no longer used. */ g_array_free (table_offsets, true);
Signature · 1 line
2.19.2
PATCH v2 5/8 hw: arm: Convert the RSDP build to the buid_append_foo() API
In reply to Samuel OrtizInstead of filling a mapped and packed C structure field in random order and being careful about endianness and sizes, build_rsdp() now uses build_append_int_noprefix() to compose RSDP table.
This makes for an easier to review and maintain code that is almost matching 1:1 the ACPI spec itself.
--- hw/arm/virt-acpi-build.c | 42 +++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 20 deletions(-)
hw/arm/virt-acpi-build.c +22 -20
@@ -368,35 +368,37 @@ static void acpi_dsdt_add_power_button(Aml *scope)/* RSDP */ static void -build_rsdp (GArray *rsdp_table,BIOSLinker *linker,AcpiRsdpData *rsdp_data) +build_rsdp (GArray *tbl,BIOSLinker *linker,AcpiRsdpData *rsdp_data) { -AcpiRsdpDescriptor *rsdp =acpi_data_push (rsdp_table,sizeof *rsdp); -unsigned xsdt_pa_size =sizeof (rsdp->xsdt_physical_address); -unsigned xsdt_pa_offset = - (char *)&rsdp->xsdt_physical_address - rsdp_table->data; - -bios_linker_loader_alloc (linker,ACPI_BUILD_RSDP_FILE , rsdp_table,16 , +bios_linker_loader_alloc (linker,ACPI_BUILD_RSDP_FILE , tbl,16 , true/* fseg memory */ ); -memcpy (&rsdp->signature,"RSD PTR " ,sizeof (rsdp->signature)); -memcpy (rsdp->oem_id, rsdp_data->oem_id,sizeof (rsdp->oem_id)); - rsdp->length =cpu_to_le32 (sizeof (*rsdp)); - rsdp->revision = rsdp_data->revision; +g_array_append_vals (tbl,"RSD PTR " ,8 );/* Signature */ +build_append_int_noprefix (tbl,0 ,1 );/* Checksum */ +g_array_append_vals (tbl, rsdp_data->oem_id,6 );/* OEMID */ +build_append_int_noprefix (tbl, rsdp_data->revision,1 );/* Revision */ +build_append_int_noprefix (tbl,0 ,4 );/* RsdtAddress */ +build_append_int_noprefix (tbl,36 ,4 );/* Length */ -/* Address to be filled by Guest linker */ -bios_linker_loader_add_pointer (linker, -ACPI_BUILD_RSDP_FILE , xsdt_pa_offset, xsdt_pa_size, -ACPI_BUILD_TABLE_FILE , *rsdp_data->xsdt_tbl_offset); +/* XSDT address to be filled by guest linker */ +build_append_int_noprefix (tbl,0 ,8 );/* XsdtAddress */ +bios_linker_loader_add_pointer (linker,ACPI_BUILD_RSDP_FILE , +24 ,8 , +ACPI_BUILD_TABLE_FILE , + *rsdp_data->xsdt_tbl_offset); -/* Checksum to be filled by Guest linker */ +build_append_int_noprefix (tbl,0 ,1 );/* Extended Checksum */ +build_append_int_noprefix (tbl,0 ,3 );/* Reserved */ + +/* Checksum to be filled by guest linker */ bios_linker_loader_add_checksum (linker,ACPI_BUILD_RSDP_FILE , - (char *)rsdp - rsdp_table->data,20 /* ACPI rev 1.0 RSDP size */ , - (char *)&rsdp->checksum - rsdp_table->data); +0 ,20 ,/* ACPI rev 1.0 RSDP size */ +8 );/* Extended checksum to be filled by Guest linker */ bios_linker_loader_add_checksum (linker,ACPI_BUILD_RSDP_FILE , - (char *)rsdp - rsdp_table->data,36 /* ACPI rev 2.0 RSDP size */ , - (char *)&rsdp->extended_checksum - rsdp_table->data); +0 ,36 ,/* ACPI rev 2.0 RSDP size */ +32 ); } static void
Signature · 1 line
2.19.2
PATCH v2 6/8 hw: arm: Support both legacy and current RSDP build
In reply to Samuel OrtizWe add the ability to build legacy or current RSDP tables, based on the AcpiRsdpData revision field passed to build_rsdp().
Although arm/virt only uses RSDP v2, adding that capability to build_rsdp will allow us to share the RSDP build code between ARM and x86.
--- hw/arm/virt-acpi-build.c | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-)
hw/arm/virt-acpi-build.c +26 -11
@@ -378,23 +378,38 @@ build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data)g_array_append_vals (tbl, rsdp_data->oem_id,6 );/* OEMID */ build_append_int_noprefix (tbl, rsdp_data->revision,1 );/* Revision */ build_append_int_noprefix (tbl,0 ,4 );/* RsdtAddress */ -build_append_int_noprefix (tbl,36 ,4 );/* Length */ - -/* XSDT address to be filled by guest linker */ -build_append_int_noprefix (tbl,0 ,8 );/* XsdtAddress */ -bios_linker_loader_add_pointer (linker,ACPI_BUILD_RSDP_FILE , -24 ,8 , -ACPI_BUILD_TABLE_FILE , - *rsdp_data->xsdt_tbl_offset); - -build_append_int_noprefix (tbl,0 ,1 );/* Extended Checksum */ -build_append_int_noprefix (tbl,0 ,3 );/* Reserved */ +if (rsdp_data->rsdt_tbl_offset) { +/* RSDT address to be filled by guest linker */ +bios_linker_loader_add_pointer (linker, +ACPI_BUILD_RSDP_FILE ,16 ,4 , +ACPI_BUILD_TABLE_FILE , + *rsdp_data->rsdt_tbl_offset); + }/* Checksum to be filled by guest linker */ bios_linker_loader_add_checksum (linker,ACPI_BUILD_RSDP_FILE ,0 ,20 ,/* ACPI rev 1.0 RSDP size */ 8 ); +if (rsdp_data->revision ==0 ) { +/* ACPI 1.0 RSDP, we're done */ +return ; + } + +/* The RSDP revision is 2 and later, we must have an XSDT pointer */ +g_assert (rsdp_data->xsdt_tbl_offset !=NULL ); + +build_append_int_noprefix (tbl,36 ,4 );/* Length */ + +/* XSDT address to be filled by guest linker */ +build_append_int_noprefix (tbl,0 ,8 );/* XsdtAddress */ +bios_linker_loader_add_pointer (linker,ACPI_BUILD_RSDP_FILE ,24 ,8 , +ACPI_BUILD_TABLE_FILE , + *rsdp_data->xsdt_tbl_offset); + +build_append_int_noprefix (tbl,0 ,1 );/* Extended Checksum */ +build_append_int_noprefix (tbl,0 ,3 );/* Reserved */ +/* Extended checksum to be filled by Guest linker */ bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE, 0, 36, /* ACPI rev 2.0 RSDP size */
Signature · 1 line
2.19.2
PATCH v2 7/8 hw: acpi: Export and share the ARM RSDP build
In reply to Samuel OrtizNow that build_rsdp() supports building both legacy and current RSDP tables, we can move it to a generic folder (hw/acpi) and have the i386 ACPI code reuse it in order to reduce code duplication.
--- include/hw/acpi/aml-build.h | 2 ++ hw/acpi/aml-build.c | 50 +++++++++++++++++++++++++++++++++++++ hw/arm/virt-acpi-build.c | 50 ------------------------------------- hw/i386/acpi-build.c | 39 +++++++---------------------- 4 files changed, 61 insertions(+), 80 deletions(-)
include/hw/acpi/aml-build.h +2 -0
@@ -388,6 +388,8 @@ void acpi_add_table(GArray *table_offsets, GArray *table_data);void acpi_build_tables_init (AcpiBuildTables *tables);void acpi_build_tables_cleanup (AcpiBuildTables *tables,bool mfre);void +build_rsdp (GArray *tbl,BIOSLinker *linker,AcpiRsdpData *rsdp_data); +void build_rsdt (GArray *table_data,BIOSLinker *linker,GArray *table_offsets,const char *oem_id,const char *oem_table_id);void
hw/acpi/aml-build.c +50 -0
56 lines not on this page · raw message
hw/arm/virt-acpi-build.c +0 -50
56 lines not on this page · raw message
hw/i386/acpi-build.c +9 -30
@@ -2547,35 +2547,6 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker) "IVRS", table_data->len - iommu_start, 1, NULL, NULL); } -static void -build_rsdp (GArray *rsdp_table,BIOSLinker *linker,unsigned rsdt_tbl_offset) -{ -/* AcpiRsdpDescriptor describes revision 2 RSDP table and as result we -* allocate extra 16 bytes for pc/q35 RSDP rev1 as well. Keep extra 16 bytes -* wasted to make sure we won't breake migration for machine types older -* than 2.3 due to size mismatch. -*/ -AcpiRsdpDescriptor *rsdp =acpi_data_push (rsdp_table,sizeof *rsdp); -unsigned rsdt_pa_size =sizeof (rsdp->rsdt_physical_address); -unsigned rsdt_pa_offset = - (char *)&rsdp->rsdt_physical_address - rsdp_table->data; - -bios_linker_loader_alloc (linker,ACPI_BUILD_RSDP_FILE , rsdp_table,16 , - true/* fseg memory */ ); - -memcpy (&rsdp->signature,"RSD PTR " ,8 ); -memcpy (rsdp->oem_id,ACPI_BUILD_APPNAME6 ,6 ); -/* Address to be filled by Guest linker */ -bios_linker_loader_add_pointer (linker, -ACPI_BUILD_RSDP_FILE , rsdt_pa_offset, rsdt_pa_size, -ACPI_BUILD_TABLE_FILE , rsdt_tbl_offset); - -/* Checksum to be filled by Guest linker */ -bios_linker_loader_add_checksum (linker,ACPI_BUILD_RSDP_FILE , - (char *)rsdp - rsdp_table->data,20 /* ACPI rev 1.0 RSDP size */ , - (char *)&rsdp->checksum - rsdp_table->data); -} - typedef struct AcpiBuildState { /* Copy of table in RAM (for patching). */
@@ -2732,7 +2703,15 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine) slic_oem.id, slic_oem.table_id);/* RSDP is in FSEG memory, so allocate it separately */ -build_rsdp (tables->rsdp, tables->linker, rsdt); + { +AcpiRsdpData rsdp_data = { + .revision =0 , + .oem_id =ACPI_BUILD_APPNAME6 , + .xsdt_tbl_offset =NULL , + .rsdt_tbl_offset = &rsdt, + }; +build_rsdp (tables->rsdp, tables->linker, &rsdp_data); + }/* We'll expose it all to Guest so we want to reduce * chance of size changes.
Signature · 1 line
2.19.2
PATCH v2 8/8 hw: acpi: Remove AcpiRsdpDescriptor and fix tests
In reply to Samuel OrtizThe only remaining AcpiRsdpDescriptor users are the ACPI utils for the BIOS table tests.
We remove that dependency and can thus remove the structure itself.
--- include/hw/acpi/acpi-defs.h | 13 ----------- tests/acpi-utils.h | 5 +++- tests/acpi-utils.c | 46 ++++++++++++++++++++++++++++++------- tests/bios-tables-test.c | 27 +++++++++++++++------- tests/vmgenid-test.c | 8 ++++--- 5 files changed, 66 insertions(+), 33 deletions(-)
include/hw/acpi/acpi-defs.h +0 -13
@@ -40,19 +40,6 @@ enum { ACPI_FADT_F_LOW_POWER_S0_IDLE_CAPABLE, }; -struct AcpiRsdpDescriptor {/* Root System Descriptor Pointer */ -uint64_t signature;/* ACPI signature, contains "RSD PTR " */ -uint8_t checksum;/* To make sum of struct == 0 */ -uint8_t oem_id [6 ];/* OEM identification */ -uint8_t revision;/* Must be 0 for 1.0, 2 for 2.0 */ -uint32_t rsdt_physical_address;/* 32-bit physical address of RSDT */ -uint32_t length;/* XSDT Length in bytes including hdr */ -uint64_t xsdt_physical_address;/* 64-bit physical address of XSDT */ -uint8_t extended_checksum;/* Checksum of entire table */ -uint8_t reserved [3 ];/* Reserved field must be 0 */ -}QEMU_PACKED ; -typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor ; - typedef struct AcpiRsdpData { uint8_t oem_id[6]; /* OEM identification */ uint8_t revision; /* Must be 0 for 1.0, 2 for 2.0 */
tests/acpi-utils.h +4 -1
@@ -82,6 +82,9 @@ typedef struct {uint8_t acpi_calc_checksum (const uint8_t *data,int len);uint32_t acpi_find_rsdp_address (void ); -void acpi_parse_rsdp_table (uint32_t addr,AcpiRsdpDescriptor *rsdp_table); +uint32_t acpi_get_rsdt_address (uint8_t *rsdp_table); +uint64_t acpi_get_xsdt_address (uint8_t *rsdp_table); +void acpi_parse_rsdp_table (uint32_t addr,uint8_t *rsdp_table, +uint8_t revision);#endif /* TEST_ACPI_UTILS_H */
tests/acpi-utils.c +38 -8
52 lines not on this page · raw message
tests/bios-tables-test.c +19 -8
@@ -27,7 +27,8 @@ typedef struct {const char *machine;const char *variant;uint32_t rsdp_addr; -AcpiRsdpDescriptor rsdp_table; +uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */ ]; +uint32_t rsdt_physical_address;AcpiRsdtDescriptorRev1 rsdt_table;uint32_t dsdt_addr;uint32_t facs_addr;
@@ -83,21 +84,31 @@ static void test_acpi_rsdp_address(test_data *data) data->rsdp_addr = off; } -static void test_acpi_rsdp_table (test_data *data) +static void test_acpi_rsdp_table (test_data *data,uint8_t revision) { -AcpiRsdpDescriptor *rsdp_table = &data->rsdp_table; +uint8_t *rsdp_table = data->rsdp_table;uint32_t addr = data->rsdp_addr; -acpi_parse_rsdp_table (addr, rsdp_table); +acpi_parse_rsdp_table (addr, rsdp_table, revision); -/* rsdp checksum is not for the whole table, but for the first 20 bytes */ -g_assert (!acpi_calc_checksum ((uint8_t *)rsdp_table,20 )); +switch (revision) { +case 0 :/* ACPI 1.0 RSDP */ +/* With rev 1, checksum is only for the first 20 bytes */ +g_assert (!acpi_calc_checksum (rsdp_table,20 )); +break ; +case 2 :/* ACPI 2.0+ RSDP */ +/* With revision 2, we have 2 checksums */ +g_assert (!acpi_calc_checksum (rsdp_table,20 )); +g_assert (!acpi_calc_checksum (rsdp_table,36 )); +default : +g_assert_not_reached (); + } }static void test_acpi_rsdt_table (test_data *data) {AcpiRsdtDescriptorRev1 *rsdt_table = &data->rsdt_table; -uint32_t addr =le32_to_cpu (data->rsdp_table.rsdt_physical_address); +uint32_t addr =acpi_get_rsdt_address (data->rsdp_table);uint32_t *tables;int tables_nr;uint8_t checksum;
@@ -626,7 +637,7 @@ static void test_acpi_one(const char *params, test_data *data) data->tables =g_array_new (false, true,sizeof (AcpiSdtTable));test_acpi_rsdp_address (data); -test_acpi_rsdp_table (data); +test_acpi_rsdp_table (data,0 );/* ACPI 1.0 RSDP */ test_acpi_rsdt_table (data);fadt_fetch_facs_and_dsdt_ptrs (data);test_acpi_facs_table (data);
tests/vmgenid-test.c +5 -3
@@ -35,7 +35,7 @@ static uint32_t acpi_find_vgia(void) {uint32_t rsdp_offset;uint32_t guid_offset =0 ; -AcpiRsdpDescriptor rsdp_table; +uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */ ];uint32_t rsdt, rsdt_table_length;AcpiRsdtDescriptorRev1 rsdt_table;size_t tables_nr;
@@ -52,9 +52,11 @@ static uint32_t acpi_find_vgia(void) g_assert_cmphex(rsdp_offset, <, RSDP_ADDR_INVALID); -acpi_parse_rsdp_table (rsdp_offset, &rsdp_table); +acpi_parse_rsdp_table (rsdp_offset, rsdp_table,0 ); + + rsdt =acpi_get_rsdt_address (rsdp_table); +g_assert (rsdt); - rsdt =le32_to_cpu (rsdp_table.rsdt_physical_address);/* read the header */ ACPI_READ_TABLE_HEADER (&rsdt_table, rsdt);ACPI_ASSERT_CMP (rsdt_table.signature,"RSDT" );
Signature · 1 line
2.19.2
Re: [Qemu-arm] [Qemu-devel] [PATCH v2 1/8] hw: acpi: The RSDP build API can return void
In reply to Samuel OrtizQuoted 8 lines · Samuel Ortiz
For both x86 and ARM architectures, the internal RSDP build API can return void as the current return value is unused.
Signed-off-by Samuel Ortiz <[email protected]>Reviewed-by Igor Mammedov <[email protected]>Reviewed-by Philippe Mathieu-Daudé <[email protected]>Reviewed-by Thomas Huth <[email protected]>
Quoted 5 lines
--- hw/arm/virt-acpi-build.c | 4 +--- hw/i386/acpi-build.c | 4 +--- 2 files changed, 2 insertions(+), 6 deletions(-)
Re: [Qemu-devel] [PATCH v2 2/8] hw: arm: acpi: Fix incorrect checksums in RSDP
In reply to Samuel OrtizQuoted 18 lines · Samuel Ortiz
From: Igor Mammedov <[email protected]>
When RSDP table was introduced (
d4bec5d87), we calculated only legacy checksum, and that was incorrect as it
- specified rev=2 and forgot about extended checksum.
- legacy checksum calculated on full table instead of the 1st 20 bytesFix it by adding extended checksum calculation and using correct size for legacy checksum.
While at it use explicit constants to specify sub/full tables
sizes instead of relying on AcpiRsdpDescriptor size and fields offsets. The follow up commits will convert this table tobuild_append_int_noprefix()API, will use constants anyway and remove unused AcpiRsdpDescriptor structure.Based on "[PATCH v5 05/24] hw: acpi: Implement XSDT support for RSDP" by Samuel Ortiz, who did it right in his impl.
Quoted 6 lines
Fixes d4bec5d87 (hw/arm/virt-acpi-build: Generate RSDP table)Signed-off-by Igor Mammedov <[email protected]>CC Ard Biesheuvel <[email protected]>CC Shannon Zhao <[email protected]>Reviewed-by Samuel Ortiz <[email protected]>Signed-off-by Samuel Ortiz <[email protected]>
I don't see my R-b here, but this patch doesn't appear to have changed since I reviewed it. Here it is again
Quoted 27 lines
--- hw/arm/virt-acpi-build.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)hw/arm/virt-acpi-build.c +6
-1@@ -390,8 +390,13 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)/* Checksum to be filled by Guest linker */ bios_linker_loader_add_checksum (linker,ACPI_BUILD_RSDP_FILE , - (char *)rsdp - rsdp_table->data,sizeof *rsdp, + (char *)rsdp - rsdp_table->data,20 /* ACPI rev 1.0 RSDP size */ , (char *)&rsdp->checksum - rsdp_table->data); + +/* Extended checksum to be filled by Guest linker */ +bios_linker_loader_add_checksum (linker,ACPI_BUILD_RSDP_FILE , + (char *)rsdp - rsdp_table->data,36 /* ACPI rev 2.0 RSDP size */ , + (char *)&rsdp->extended_checksum - rsdp_table->data); } static voidSignature · 1 line
2.19.2
Re: [Qemu-arm] [Qemu-devel] [PATCH v2 4/8] hw: arm: Carry RSDP specific data through AcpiRsdpData
In reply to Samuel OrtizQuoted 15 lines · Samuel Ortiz
That will allow us to generalize the ARM
build_rsdp()routine to support both legacy RSDP (The current i386 implementation) and extended RSDP (The ARM implementation).Signed-off-by Samuel Ortiz <[email protected]>--- include/hw/acpi/acpi-defs.h | 8 ++++++++ hw/arm/virt-acpi-build.c | 18 +++++++++++++----- 2 files changed, 21 insertions(+), 5 deletions(-)include/hw/acpi/acpi-defs.h +0
-0
include/hw/acpi/acpi-defs.h struct AcpiRsdpDescriptor { /* Root System Descriptor Pointer */ · line 61
Quoted 9 lines
include/hw/acpi/acpi-defs.h +6
-0@@ -53,6 +53,14 @@ struct AcpiRsdpDescriptor { /* Root System Descriptor Pointer */ }QEMU_PACKED ;typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor ; +typedef struct AcpiRsdpData { + uint8_t oem_id[6]; /* OEM identification */ + uint8_t revision; /* Must be 0 for 1.0, 2 for 2.0 */ + + unsigned *rsdt_tbl_offset; + unsigned *xsdt_tbl_offset;
Why use pointers?
Quoted 19 lines
include/hw/acpi/acpi-defs.h +2
-0+} AcpiRsdpData; +/* Table structure from Linux kernel (the ACPI tables are under the BSD license) */ hw/arm/virt-acpi-build.c +1
-1@@ -368,7 +368,7 @@ static void acpi_dsdt_add_power_button(Aml *scope)/* RSDP */ static void -build_rsdp (GArray *rsdp_table,BIOSLinker *linker,unsigned xsdt_tbl_offset) +build_rsdp (GArray *rsdp_table,BIOSLinker *linker,AcpiRsdpData *rsdp_data) {AcpiRsdpDescriptor *rsdp =acpi_data_push (rsdp_table,sizeof *rsdp);unsigned xsdt_pa_size =sizeof (rsdp->xsdt_physical_address);
hw/arm/virt-acpi-build.c build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset) · line 382
Quoted 5 lines
hw/arm/virt-acpi-build.c +1
-1@@ -379,14 +379,14 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset) true /* fseg memory */);memcpy (&rsdp->signature,"RSD PTR " ,sizeof (rsdp->signature)); -memcpy (rsdp->oem_id,ACPI_BUILD_APPNAME6 ,sizeof (rsdp->oem_id)); +memcpy (rsdp->oem_id, rsdp_data->oem_id,sizeof (rsdp->oem_id));
sizeof(rsdp_data->oem_id)
Quoted 13 lines
hw/arm/virt-acpi-build.c +2
-2rsdp->length =cpu_to_le32 (sizeof (*rsdp)); - rsdp->revision =0x02 ; + rsdp->revision = rsdp_data->revision;/* Address to be filled by Guest linker */ bios_linker_loader_add_pointer (linker,ACPI_BUILD_RSDP_FILE , xsdt_pa_offset, xsdt_pa_size, -ACPI_BUILD_TABLE_FILE , xsdt_tbl_offset); +ACPI_BUILD_TABLE_FILE , *rsdp_data->xsdt_tbl_offset);/* Checksum to be filled by Guest linker */ bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
hw/arm/virt-acpi-build.c void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) · line 865
Quoted 10 lines
hw/arm/virt-acpi-build.c +6
-1@@ -857,7 +857,15 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)build_xsdt (tables_blob, tables->linker, table_offsets,NULL ,NULL );/* RSDP is in FSEG memory, so allocate it separately */ -build_rsdp (tables->rsdp, tables->linker, xsdt); + { +AcpiRsdpData rsdp_data = { + .revision =2 , + .oem_id =ACPI_BUILD_APPNAME6 , + .xsdt_tbl_offset = &xsdt, + .rsdt_tbl_offset =NULL ,
nit: no need for this explicit NULLing
Quoted 10 lines
hw/arm/virt-acpi-build.c +3
-0+ }; +build_rsdp (tables->rsdp, tables->linker, &rsdp_data); + }/* Cleanup memory that's no longer used. */ g_array_free (table_offsets, true);Signature · 1 line
2.19.2
Thanks,
drew
Re: [Qemu-devel] [PATCH v2 4/8] hw: arm: Carry RSDP specific data through AcpiRsdpData
Quoted 19 lines · Andrew Jones
Quoted 18 lines · Samuel Ortiz
That will allow us to generalize the ARM
build_rsdp()routine to support both legacy RSDP (The current i386 implementation) and extended RSDP (The ARM implementation).Signed-off-by Samuel Ortiz <[email protected]>--- include/hw/acpi/acpi-defs.h | 8 ++++++++ hw/arm/virt-acpi-build.c | 18 +++++++++++++----- 2 files changed, 21 insertions(+), 5 deletions(-)include/hw/acpi/acpi-defs.h +0
-0@@ -53,6 +53,14 @@ struct AcpiRsdpDescriptor { /* Root System Descriptor Pointer */ }QEMU_PACKED ;typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor ;
Quoted 8 lines
include/hw/acpi/acpi-defs.h struct AcpiRsdpDescriptor { /* Root System Descriptor Pointer */ · line 61
Quoted 6 lines
include/hw/acpi/acpi-defs.h +6
-0+typedef struct AcpiRsdpData { + uint8_t oem_id[6]; /* OEM identification */ + uint8_t revision; /* Must be 0 for 1.0, 2 for 2.0 */ + + unsigned *rsdt_tbl_offset; + unsigned *xsdt_tbl_offset;Why use pointers?
Mostly to be consistent with the FADT data structure (AcpiFadtData).
Quoted 21 lines
Quoted 21 lines
include/hw/acpi/acpi-defs.h +2
-0+} AcpiRsdpData; +/* Table structure from Linux kernel (the ACPI tables are under the BSD license) */ hw/arm/virt-acpi-build.c +1
-1@@ -368,7 +368,7 @@ static void acpi_dsdt_add_power_button(Aml *scope)/* RSDP */ static void -build_rsdp (GArray *rsdp_table,BIOSLinker *linker,unsigned xsdt_tbl_offset) +build_rsdp (GArray *rsdp_table,BIOSLinker *linker,AcpiRsdpData *rsdp_data) {AcpiRsdpDescriptor *rsdp =acpi_data_push (rsdp_table,sizeof *rsdp);unsigned xsdt_pa_size =sizeof (rsdp->xsdt_physical_address);@@ -379,14 +379,14 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset) true /* fseg memory */);
Quoted 5 lines
hw/arm/virt-acpi-build.c build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset) · line 382
Quoted 3 lines
hw/arm/virt-acpi-build.c +1
-1memcpy (&rsdp->signature,"RSD PTR " ,sizeof (rsdp->signature)); -memcpy (rsdp->oem_id,ACPI_BUILD_APPNAME6 ,sizeof (rsdp->oem_id)); +memcpy (rsdp->oem_id, rsdp_data->oem_id,sizeof (rsdp->oem_id));
sizeof(rsdp_data->oem_id)
Which is the same thing. Note that this piece of code eventually gets removed later in the serie.
Quoted 15 lines
Quoted 15 lines
hw/arm/virt-acpi-build.c +2
-2rsdp->length =cpu_to_le32 (sizeof (*rsdp)); - rsdp->revision =0x02 ; + rsdp->revision = rsdp_data->revision;/* Address to be filled by Guest linker */ bios_linker_loader_add_pointer (linker,ACPI_BUILD_RSDP_FILE , xsdt_pa_offset, xsdt_pa_size, -ACPI_BUILD_TABLE_FILE , xsdt_tbl_offset); +ACPI_BUILD_TABLE_FILE , *rsdp_data->xsdt_tbl_offset);/* Checksum to be filled by Guest linker */ bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,@@ -857,7 +857,15 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)build_xsdt (tables_blob, tables->linker, table_offsets,NULL ,NULL );
Quoted 10 lines
hw/arm/virt-acpi-build.c void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) · line 865
Quoted 8 lines
hw/arm/virt-acpi-build.c +6
-1/* RSDP is in FSEG memory, so allocate it separately */ -build_rsdp (tables->rsdp, tables->linker, xsdt); + { +AcpiRsdpData rsdp_data = { + .revision =2 , + .oem_id =ACPI_BUILD_APPNAME6 , + .xsdt_tbl_offset = &xsdt, + .rsdt_tbl_offset =NULL ,nit: no need for this explicit NULLing
Unfortunately, yes.
Cheers,
Samuel.
Re: [Qemu-arm] [Qemu-devel] [PATCH v2 5/8] hw: arm: Convert the RSDP build to the buid_append_foo() API
In reply to Samuel OrtizQuoted 11 lines · Samuel Ortiz
Instead of filling a mapped and packed C structure field in random order and being careful about endianness and sizes,
build_rsdp()now usesbuild_append_int_noprefix()to compose RSDP table.This makes for an easier to review and maintain code that is almost matching 1:1 the ACPI spec itself.
Signed-off-by Samuel Ortiz <[email protected]>--- hw/arm/virt-acpi-build.c | 42 +++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 20 deletions(-)
Re: [Qemu-arm] [Qemu-devel] [PATCH v2 7/8] hw: acpi: Export and share the ARM RSDP build
In reply to Samuel OrtizQuoted 7 lines · Samuel Ortiz
Now that
build_rsdp()supports building both legacy and current RSDP tables, we can move it to a generic folder (hw/acpi) and have the i386 ACPI code reuse it in order to reduce code duplication.Signed-off-by Samuel Ortiz <[email protected]>Reviewed-by Philippe Mathieu-Daudé <[email protected]>
Quoted 7 lines
--- include/hw/acpi/aml-build.h | 2 ++ hw/acpi/aml-build.c | 50 +++++++++++++++++++++++++++++++++++++ hw/arm/virt-acpi-build.c | 50 ------------------------------------- hw/i386/acpi-build.c | 39 +++++++---------------------- 4 files changed, 61 insertions(+), 80 deletions(-)
Re: [Qemu-devel] [PATCH v2 6/8] hw: arm: Support both legacy and current RSDP build
In reply to Samuel OrtizQuoted 20 lines · Samuel Ortiz
We add the ability to build legacy or current RSDP tables, based on the AcpiRsdpData revision field passed to
build_rsdp().
Although arm/virt only uses RSDP v2, adding that capability tobuild_rsdpwill allow us to share the RSDP build code between ARM and x86.Signed-off-by Samuel Ortiz <[email protected]>--- hw/arm/virt-acpi-build.c | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-)hw/arm/virt-acpi-build.c +0
-2@@ -378,23 +378,38 @@ build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data)g_array_append_vals (tbl, rsdp_data->oem_id,6 );/* OEMID */ build_append_int_noprefix (tbl, rsdp_data->revision,1 );/* Revision */ build_append_int_noprefix (tbl,0 ,4 );/* RsdtAddress */ -build_append_int_noprefix (tbl,36 ,4 );/* Length */ -
hw/arm/virt-acpi-build.c build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data) · line 381
Quoted 10 lines
hw/arm/virt-acpi-build.c +1
-9-/* XSDT address to be filled by guest linker */ -build_append_int_noprefix (tbl,0 ,8 );/* XsdtAddress */ -bios_linker_loader_add_pointer (linker,ACPI_BUILD_RSDP_FILE , -24 ,8 , -ACPI_BUILD_TABLE_FILE , - *rsdp_data->xsdt_tbl_offset); - -build_append_int_noprefix (tbl,0 ,1 );/* Extended Checksum */ -build_append_int_noprefix (tbl,0 ,3 );/* Reserved */ +if (rsdp_data->rsdt_tbl_offset) {
I see why a pointer was used now. Using a pointer ensures a zero offset won't fail this test. However the test could be replaced with rsdp_data->revision == 0.
Quoted 9 lines
hw/arm/virt-acpi-build.c +6
-0+/* RSDT address to be filled by guest linker */ +bios_linker_loader_add_pointer (linker, +ACPI_BUILD_RSDP_FILE ,16 ,4 , +ACPI_BUILD_TABLE_FILE , + *rsdp_data->rsdt_tbl_offset); + }/* Checksum to be filled by guest linker */ bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
hw/arm/virt-acpi-build.c build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data) · line 400
Quoted 10 lines
hw/arm/virt-acpi-build.c +7
-00 ,20 ,/* ACPI rev 1.0 RSDP size */ 8); +if (rsdp_data->revision ==0 ) { +/* ACPI 1.0 RSDP, we're done */ +return ; + } + +/* The RSDP revision is 2 and later, we must have an XSDT pointer */ +g_assert (rsdp_data->xsdt_tbl_offset !=NULL );
So here's the justification for the pointers. We sanity check the callers. We're missing the (rsdp_data->revision == 0 && rsdp_data->rsdt_tbl_offset) sanity check though.
Quoted 19 lines
hw/arm/virt-acpi-build.c +12
-0+ +build_append_int_noprefix (tbl,36 ,4 );/* Length */ + +/* XSDT address to be filled by guest linker */ +build_append_int_noprefix (tbl,0 ,8 );/* XsdtAddress */ +bios_linker_loader_add_pointer (linker,ACPI_BUILD_RSDP_FILE ,24 ,8 , +ACPI_BUILD_TABLE_FILE , + *rsdp_data->xsdt_tbl_offset); + +build_append_int_noprefix (tbl,0 ,1 );/* Extended Checksum */ +build_append_int_noprefix (tbl,0 ,3 );/* Reserved */ +/* Extended checksum to be filled by Guest linker */ bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE, 0, 36, /* ACPI rev 2.0 RSDP size */Signature · 1 line
2.19.2
Thanks,
drew
Re: [Qemu-arm] [Qemu-devel] [PATCH v2 6/8] hw: arm: Support both legacy and current RSDP build
Quoted 56 lines · Andrew Jones
Quoted 20 lines · Samuel Ortiz
We add the ability to build legacy or current RSDP tables, based on the AcpiRsdpData revision field passed to
build_rsdp().
Although arm/virt only uses RSDP v2, adding that capability tobuild_rsdpwill allow us to share the RSDP build code between ARM and x86.Signed-off-by Samuel Ortiz <[email protected]>--- hw/arm/virt-acpi-build.c | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-)hw/arm/virt-acpi-build.c +0
-2@@ -378,23 +378,38 @@ build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data)g_array_append_vals (tbl, rsdp_data->oem_id,6 );/* OEMID */ build_append_int_noprefix (tbl, rsdp_data->revision,1 );/* Revision */ build_append_int_noprefix (tbl,0 ,4 );/* RsdtAddress */ -build_append_int_noprefix (tbl,36 ,4 );/* Length */ -hw/arm/virt-acpi-build.c build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data) · line 381
Quoted 10 lines
hw/arm/virt-acpi-build.c +1
-9-/* XSDT address to be filled by guest linker */ -build_append_int_noprefix (tbl,0 ,8 );/* XsdtAddress */ -bios_linker_loader_add_pointer (linker,ACPI_BUILD_RSDP_FILE , -24 ,8 , -ACPI_BUILD_TABLE_FILE , - *rsdp_data->xsdt_tbl_offset); - -build_append_int_noprefix (tbl,0 ,1 );/* Extended Checksum */ -build_append_int_noprefix (tbl,0 ,3 );/* Reserved */ +if (rsdp_data->rsdt_tbl_offset) {I see why a pointer was used now. Using a pointer ensures a zero offset won't fail this test. However the test could be replaced with
rsdp_data->revision== 0.Quoted 9 lines
hw/arm/virt-acpi-build.c +6
-0+/* RSDT address to be filled by guest linker */ +bios_linker_loader_add_pointer (linker, +ACPI_BUILD_RSDP_FILE ,16 ,4 , +ACPI_BUILD_TABLE_FILE , + *rsdp_data->rsdt_tbl_offset); + }/* Checksum to be filled by guest linker */ bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,hw/arm/virt-acpi-build.c build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data) · line 400
Quoted 10 lines
hw/arm/virt-acpi-build.c +7
-00 ,20 ,/* ACPI rev 1.0 RSDP size */ 8); +if (rsdp_data->revision ==0 ) { +/* ACPI 1.0 RSDP, we're done */ +return ; + } + +/* The RSDP revision is 2 and later, we must have an XSDT pointer */ +g_assert (rsdp_data->xsdt_tbl_offset !=NULL );
Quoted 1 line
So here's the justification for the pointers. We sanity check the callers.
We could sanity check the callers without pointers as well, I don't think there's a strong advantage for pointers here, except consistence.
Quoted 2 lines
We're missing the (
rsdp_data->revision== 0 &&rsdp_data->rsdt_tbl_offset) sanity check though.
I think there's nothing preventing a caller to include both rsdt and xsdt if it wants to be able to run on both < 2.0 and 2.0+ platforms with the same table. So if rsdt is set we should add it, regardless of the revision.
Cheers,
Samuel.
Re: [Qemu-devel] [PATCH v2 8/8] hw: acpi: Remove AcpiRsdpDescriptor and fix tests
In reply to Samuel OrtizQuoted 66 lines · Samuel Ortiz
The only remaining AcpiRsdpDescriptor users are the ACPI utils for the BIOS table tests.
We remove that dependency and can thus remove the structure itself.Signed-off-by Samuel Ortiz <[email protected]>--- include/hw/acpi/acpi-defs.h | 13 ----------- tests/acpi-utils.h | 5 +++- tests/acpi-utils.c | 46 ++++++++++++++++++++++++++++++------- tests/bios-tables-test.c | 27 +++++++++++++++------- tests/vmgenid-test.c | 8 ++++--- 5 files changed, 66 insertions(+), 33 deletions(-)include/hw/acpi/acpi-defs.h +0
-13@@ -40,19 +40,6 @@ enum { ACPI_FADT_F_LOW_POWER_S0_IDLE_CAPABLE, }; -struct AcpiRsdpDescriptor {/* Root System Descriptor Pointer */ -uint64_t signature;/* ACPI signature, contains "RSD PTR " */ -uint8_t checksum;/* To make sum of struct == 0 */ -uint8_t oem_id [6 ];/* OEM identification */ -uint8_t revision;/* Must be 0 for 1.0, 2 for 2.0 */ -uint32_t rsdt_physical_address;/* 32-bit physical address of RSDT */ -uint32_t length;/* XSDT Length in bytes including hdr */ -uint64_t xsdt_physical_address;/* 64-bit physical address of XSDT */ -uint8_t extended_checksum;/* Checksum of entire table */ -uint8_t reserved [3 ];/* Reserved field must be 0 */ -}QEMU_PACKED ; -typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor ; - typedef struct AcpiRsdpData { uint8_t oem_id[6]; /* OEM identification */ uint8_t revision; /* Must be 0 for 1.0, 2 for 2.0 */tests/acpi-utils.h +4
-1@@ -82,6 +82,9 @@ typedef struct {uint8_t acpi_calc_checksum (const uint8_t *data,int len);uint32_t acpi_find_rsdp_address (void ); -void acpi_parse_rsdp_table (uint32_t addr,AcpiRsdpDescriptor *rsdp_table); +uint32_t acpi_get_rsdt_address (uint8_t *rsdp_table); +uint64_t acpi_get_xsdt_address (uint8_t *rsdp_table); +void acpi_parse_rsdp_table (uint32_t addr,uint8_t *rsdp_table, +uint8_t revision);#endif /* TEST_ACPI_UTILS_H */ tests/acpi-utils.c +2
-3@@ -52,14 +52,44 @@ uint32_t acpi_find_rsdp_address(void)return off; } -void acpi_parse_rsdp_table (uint32_t addr,AcpiRsdpDescriptor *rsdp_table) +uint32_t acpi_get_rsdt_address (uint8_t *rsdp_table) { -ACPI_READ_FIELD (rsdp_table->signature, addr); -ACPI_ASSERT_CMP64 (rsdp_table->signature,"RSD PTR " ); +uint32_t rsdt_physical_address;
tests/acpi-utils.c uint32_t acpi_find_rsdp_address(void) · line 62
Quoted 10 lines
tests/acpi-utils.c +4
-5+uint8_t revision = rsdp_table[15 /* Revision offset */ ]; -ACPI_READ_FIELD (rsdp_table->checksum, addr); -ACPI_READ_ARRAY (rsdp_table->oem_id, addr); -ACPI_READ_FIELD (rsdp_table->revision, addr); -ACPI_READ_FIELD (rsdp_table->rsdt_physical_address, addr); -ACPI_READ_FIELD (rsdp_table->length, addr); +if (revision !=0 ) {/* ACPI 1.0 RSDP */ +return 0 ; + }
So now we assume a zero offset can't be valid, which means we don't need pointers in the struct. Your answer that pointers are used for consistency is good enough for me though.
tests/acpi-utils.c uint32_t acpi_find_rsdp_address(void) · line 73
Quoted 11 lines
tests/acpi-utils.c +11
-0+ +memcpy (&rsdt_physical_address, &rsdp_table[16 /* RsdtAddress offset */ ],4 ); +return le32_to_cpu (rsdt_physical_address); +} + +uint64_t acpi_get_xsdt_address (uint8_t *rsdp_table) +{ +uint64_t xsdt_physical_address; +uint8_t revision = rsdp_table[15 /* Revision offset */ ]; + +if (revision !=2 ) {/* ACPI 2.0+ RSDP */
revision < 2
Quoted 27 lines
tests/acpi-utils.c +21
-0+return 0 ; + } + +memcpy (&xsdt_physical_address, &rsdp_table[24 /* XsdtAddress offset */ ],8 ); +return le64_to_cpu (xsdt_physical_address); +} + +void acpi_parse_rsdp_table (uint32_t addr,uint8_t *rsdp_table,uint8_t revision) +{ +switch (revision) { +case 0 :/* ACPI 1.0 RSDP */ +memread (addr, rsdp_table,20 ); +break ; +case 2 :/* ACPI 2.0+ RSDP */ +memread (addr, rsdp_table,36 ); +break ; +default : +g_assert_not_reached (); + } + +ACPI_ASSERT_CMP64 (*((uint64_t *)(rsdp_table)),"RSD PTR " ); }tests/bios-tables-test.c +0
-0
tests/bios-tables-test.c typedef struct { · line 31
Quoted 6 lines
tests/bios-tables-test.c +2
-1@@ -27,7 +27,8 @@ typedef struct {const char *machine;const char *variant;uint32_t rsdp_addr; -AcpiRsdpDescriptor rsdp_table; +uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */ ]; +uint32_t rsdt_physical_address;
I don't see rsdt_physical_address getting used anyway.
Quoted 81 lines
tests/bios-tables-test.c +17
-7AcpiRsdtDescriptorRev1 rsdt_table;uint32_t dsdt_addr;uint32_t facs_addr;@@ -83,21 +84,31 @@ static void test_acpi_rsdp_address(test_data *data) data->rsdp_addr = off; } -static void test_acpi_rsdp_table (test_data *data) +static void test_acpi_rsdp_table (test_data *data,uint8_t revision) { -AcpiRsdpDescriptor *rsdp_table = &data->rsdp_table; +uint8_t *rsdp_table = data->rsdp_table;uint32_t addr = data->rsdp_addr; -acpi_parse_rsdp_table (addr, rsdp_table); +acpi_parse_rsdp_table (addr, rsdp_table, revision); -/* rsdp checksum is not for the whole table, but for the first 20 bytes */ -g_assert (!acpi_calc_checksum ((uint8_t *)rsdp_table,20 )); +switch (revision) { +case 0 :/* ACPI 1.0 RSDP */ +/* With rev 1, checksum is only for the first 20 bytes */ +g_assert (!acpi_calc_checksum (rsdp_table,20 )); +break ; +case 2 :/* ACPI 2.0+ RSDP */ +/* With revision 2, we have 2 checksums */ +g_assert (!acpi_calc_checksum (rsdp_table,20 )); +g_assert (!acpi_calc_checksum (rsdp_table,36 )); +default : +g_assert_not_reached (); + } }static void test_acpi_rsdt_table (test_data *data) {AcpiRsdtDescriptorRev1 *rsdt_table = &data->rsdt_table; -uint32_t addr =le32_to_cpu (data->rsdp_table.rsdt_physical_address); +uint32_t addr =acpi_get_rsdt_address (data->rsdp_table);uint32_t *tables;int tables_nr;uint8_t checksum;@@ -626,7 +637,7 @@ static void test_acpi_one(const char *params, test_data *data) data->tables =g_array_new (false, true,sizeof (AcpiSdtTable));test_acpi_rsdp_address (data); -test_acpi_rsdp_table (data); +test_acpi_rsdp_table (data,0 );/* ACPI 1.0 RSDP */ test_acpi_rsdt_table (data);fadt_fetch_facs_and_dsdt_ptrs (data);test_acpi_facs_table (data);tests/vmgenid-test.c +5
-3@@ -35,7 +35,7 @@ static uint32_t acpi_find_vgia(void) {uint32_t rsdp_offset;uint32_t guid_offset =0 ; -AcpiRsdpDescriptor rsdp_table; +uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */ ];uint32_t rsdt, rsdt_table_length;AcpiRsdtDescriptorRev1 rsdt_table;size_t tables_nr;@@ -52,9 +52,11 @@ static uint32_t acpi_find_vgia(void) g_assert_cmphex(rsdp_offset, <, RSDP_ADDR_INVALID); -acpi_parse_rsdp_table (rsdp_offset, &rsdp_table); +acpi_parse_rsdp_table (rsdp_offset, rsdp_table,0 ); + + rsdt =acpi_get_rsdt_address (rsdp_table); +g_assert (rsdt); - rsdt =le32_to_cpu (rsdp_table.rsdt_physical_address);/* read the header */ ACPI_READ_TABLE_HEADER (&rsdt_table, rsdt);ACPI_ASSERT_CMP (rsdt_table.signature,"RSDT" );Signature · 1 line
2.19.2
Thanks,
drew
Re: [Qemu-devel] [PATCH v2 6/8] hw: arm: Support both legacy and current RSDP build
In reply to Samuel OrtizQuoted 62 lines · Samuel Ortiz
Quoted 56 lines · Andrew Jones
Quoted 20 lines · Samuel Ortiz
We add the ability to build legacy or current RSDP tables, based on the AcpiRsdpData revision field passed to
build_rsdp().
Although arm/virt only uses RSDP v2, adding that capability tobuild_rsdpwill allow us to share the RSDP build code between ARM and x86.Signed-off-by Samuel Ortiz <[email protected]>--- hw/arm/virt-acpi-build.c | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-)hw/arm/virt-acpi-build.c +0
-2@@ -378,23 +378,38 @@ build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data)g_array_append_vals (tbl, rsdp_data->oem_id,6 );/* OEMID */ build_append_int_noprefix (tbl, rsdp_data->revision,1 );/* Revision */ build_append_int_noprefix (tbl,0 ,4 );/* RsdtAddress */ -build_append_int_noprefix (tbl,36 ,4 );/* Length */ -hw/arm/virt-acpi-build.c build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data) · line 381
Quoted 10 lines
hw/arm/virt-acpi-build.c +1
-9-/* XSDT address to be filled by guest linker */ -build_append_int_noprefix (tbl,0 ,8 );/* XsdtAddress */ -bios_linker_loader_add_pointer (linker,ACPI_BUILD_RSDP_FILE , -24 ,8 , -ACPI_BUILD_TABLE_FILE , - *rsdp_data->xsdt_tbl_offset); - -build_append_int_noprefix (tbl,0 ,1 );/* Extended Checksum */ -build_append_int_noprefix (tbl,0 ,3 );/* Reserved */ +if (rsdp_data->rsdt_tbl_offset) {I see why a pointer was used now. Using a pointer ensures a zero offset won't fail this test. However the test could be replaced with
rsdp_data->revision== 0.Quoted 9 lines
hw/arm/virt-acpi-build.c +6
-0+/* RSDT address to be filled by guest linker */ +bios_linker_loader_add_pointer (linker, +ACPI_BUILD_RSDP_FILE ,16 ,4 , +ACPI_BUILD_TABLE_FILE , + *rsdp_data->rsdt_tbl_offset); + }/* Checksum to be filled by guest linker */ bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,hw/arm/virt-acpi-build.c build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data) · line 400
Quoted 10 lines
hw/arm/virt-acpi-build.c +7
-00 ,20 ,/* ACPI rev 1.0 RSDP size */ 8); +if (rsdp_data->revision ==0 ) { +/* ACPI 1.0 RSDP, we're done */ +return ; + } + +/* The RSDP revision is 2 and later, we must have an XSDT pointer */ +g_assert (rsdp_data->xsdt_tbl_offset !=NULL );Quoted 1 line
So here's the justification for the pointers. We sanity check the callers.
We could sanity check the callers without pointers as well, I don't think there's a strong advantage for pointers here, except consistence.
Quoted 5 lines
Quoted 2 lines
We're missing the (
rsdp_data->revision== 0 &&rsdp_data->rsdt_tbl_offset) sanity check though.I think there's nothing preventing a caller to include both rsdt and xsdt if it wants to be able to run on both < 2.0 and 2.0+ platforms with the same table. So if rsdt is set we should add it, regardless of the revision.
True, but if revision is zero, then we *must* have it. Otherwise we'll silently return from this function with neither an rsdt nor xsdt linked.
Thanks,
drew
Quoted 3 lines
Cheers,
Samuel.
Re: [Qemu-arm] [Qemu-devel] [PATCH v2 6/8] hw: arm: Support both legacy and current RSDP build
Quoted 63 lines · Andrew Jones
Quoted 62 lines · Samuel Ortiz
Quoted 56 lines · Andrew Jones
Quoted 20 lines · Samuel Ortiz
We add the ability to build legacy or current RSDP tables, based on the AcpiRsdpData revision field passed to
build_rsdp().
Although arm/virt only uses RSDP v2, adding that capability tobuild_rsdpwill allow us to share the RSDP build code between ARM and x86.Signed-off-by Samuel Ortiz <[email protected]>--- hw/arm/virt-acpi-build.c | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-)hw/arm/virt-acpi-build.c +0
-2@@ -378,23 +378,38 @@ build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data)g_array_append_vals (tbl, rsdp_data->oem_id,6 );/* OEMID */ build_append_int_noprefix (tbl, rsdp_data->revision,1 );/* Revision */ build_append_int_noprefix (tbl,0 ,4 );/* RsdtAddress */ -build_append_int_noprefix (tbl,36 ,4 );/* Length */ -hw/arm/virt-acpi-build.c build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data) · line 381
Quoted 10 lines
hw/arm/virt-acpi-build.c +1
-9-/* XSDT address to be filled by guest linker */ -build_append_int_noprefix (tbl,0 ,8 );/* XsdtAddress */ -bios_linker_loader_add_pointer (linker,ACPI_BUILD_RSDP_FILE , -24 ,8 , -ACPI_BUILD_TABLE_FILE , - *rsdp_data->xsdt_tbl_offset); - -build_append_int_noprefix (tbl,0 ,1 );/* Extended Checksum */ -build_append_int_noprefix (tbl,0 ,3 );/* Reserved */ +if (rsdp_data->rsdt_tbl_offset) {I see why a pointer was used now. Using a pointer ensures a zero offset won't fail this test. However the test could be replaced with
rsdp_data->revision== 0.Quoted 9 lines
hw/arm/virt-acpi-build.c +6
-0+/* RSDT address to be filled by guest linker */ +bios_linker_loader_add_pointer (linker, +ACPI_BUILD_RSDP_FILE ,16 ,4 , +ACPI_BUILD_TABLE_FILE , + *rsdp_data->rsdt_tbl_offset); + }/* Checksum to be filled by guest linker */ bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,hw/arm/virt-acpi-build.c build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data) · line 400
Quoted 10 lines
hw/arm/virt-acpi-build.c +7
-00 ,20 ,/* ACPI rev 1.0 RSDP size */ 8); +if (rsdp_data->revision ==0 ) { +/* ACPI 1.0 RSDP, we're done */ +return ; + } + +/* The RSDP revision is 2 and later, we must have an XSDT pointer */ +g_assert (rsdp_data->xsdt_tbl_offset !=NULL );Quoted 1 line
So here's the justification for the pointers. We sanity check the callers.
We could sanity check the callers without pointers as well, I don't think there's a strong advantage for pointers here, except consistence.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
We're missing the (
rsdp_data->revision== 0 &&rsdp_data->rsdt_tbl_offset) sanity check though.I think there's nothing preventing a caller to include both rsdt and xsdt if it wants to be able to run on both < 2.0 and 2.0+ platforms with the same table. So if rsdt is set we should add it, regardless of the revision.
True, but if revision is zero, then we *must* have it. Otherwise we'll silently return from this function with neither an rsdt nor xsdt linked.
Correct, thanks. With v3 I'm adding a small piece of code at the start of build_rsdp() that checks for valid rsdt and xsdt pointers (for rev 0 and 2, respectively).
Cheers,
Samuel.
Re: [Qemu-arm] [Qemu-devel] [PATCH v2 4/8] hw: arm: Carry RSDP specific data through AcpiRsdpData
In reply to Samuel OrtizQuoted 28 lines · On Thu, 29 Nov 2018 15:26:27 +0100 Samuel Ortiz <[email protected]> wrote:
Quoted 19 lines · Andrew Jones
Quoted 18 lines · Samuel Ortiz
That will allow us to generalize the ARM
build_rsdp()routine to support both legacy RSDP (The current i386 implementation) and extended RSDP (The ARM implementation).Signed-off-by Samuel Ortiz <[email protected]>--- include/hw/acpi/acpi-defs.h | 8 ++++++++ hw/arm/virt-acpi-build.c | 18 +++++++++++++----- 2 files changed, 21 insertions(+), 5 deletions(-)include/hw/acpi/acpi-defs.h +0
-0@@ -53,6 +53,14 @@ struct AcpiRsdpDescriptor { /* Root System Descriptor Pointer */ }QEMU_PACKED ;typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor ;Quoted 8 lines
include/hw/acpi/acpi-defs.h struct AcpiRsdpDescriptor { /* Root System Descriptor Pointer */ · line 61
Quoted 6 lines
include/hw/acpi/acpi-defs.h +6
-0+typedef struct AcpiRsdpData { + uint8_t oem_id[6]; /* OEM identification */ + uint8_t revision; /* Must be 0 for 1.0, 2 for 2.0 */ + + unsigned *rsdt_tbl_offset; + unsigned *xsdt_tbl_offset;Why use pointers?
Quoted 1 line
Mostly to be consistent with the FADT data structure (AcpiFadtData).
My guess would be that's for detecting an optional field and skipping setting it in build_rsdp(). But it's mostly for consistence with current code.
Quoted 22 lines
Quoted 22 lines
Quoted 22 lines
include/hw/acpi/acpi-defs.h +2
-0+} AcpiRsdpData; +/* Table structure from Linux kernel (the ACPI tables are under the BSD license) */ hw/arm/virt-acpi-build.c +1
-1@@ -368,7 +368,7 @@ static void acpi_dsdt_add_power_button(Aml *scope)/* RSDP */ static void -build_rsdp (GArray *rsdp_table,BIOSLinker *linker,unsigned xsdt_tbl_offset) +build_rsdp (GArray *rsdp_table,BIOSLinker *linker,AcpiRsdpData *rsdp_data) {AcpiRsdpDescriptor *rsdp =acpi_data_push (rsdp_table,sizeof *rsdp);unsigned xsdt_pa_size =sizeof (rsdp->xsdt_physical_address);@@ -379,14 +379,14 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset) true /* fseg memory */);
Quoted 35 lines
Quoted 5 lines
hw/arm/virt-acpi-build.c build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset) · line 382
Quoted 3 lines
hw/arm/virt-acpi-build.c +1
-1memcpy (&rsdp->signature,"RSD PTR " ,sizeof (rsdp->signature)); -memcpy (rsdp->oem_id,ACPI_BUILD_APPNAME6 ,sizeof (rsdp->oem_id)); +memcpy (rsdp->oem_id, rsdp_data->oem_id,sizeof (rsdp->oem_id));
sizeof(rsdp_data->oem_id)Which is the same thing. Note that this piece of code eventually gets removed later in the serie.
Quoted 25 lines
Quoted 13 lines
hw/arm/virt-acpi-build.c +2
-2rsdp->length =cpu_to_le32 (sizeof (*rsdp)); - rsdp->revision =0x02 ; + rsdp->revision = rsdp_data->revision;/* Address to be filled by Guest linker */ bios_linker_loader_add_pointer (linker,ACPI_BUILD_RSDP_FILE , xsdt_pa_offset, xsdt_pa_size, -ACPI_BUILD_TABLE_FILE , xsdt_tbl_offset); +ACPI_BUILD_TABLE_FILE , *rsdp_data->xsdt_tbl_offset);/* Checksum to be filled by Guest linker */ bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,hw/arm/virt-acpi-build.c void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) · line 865
Quoted 10 lines
hw/arm/virt-acpi-build.c +6
-1@@ -857,7 +857,15 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)build_xsdt (tables_blob, tables->linker, table_offsets,NULL ,NULL );/* RSDP is in FSEG memory, so allocate it separately */ -build_rsdp (tables->rsdp, tables->linker, xsdt); + { +AcpiRsdpData rsdp_data = { + .revision =2 , + .oem_id =ACPI_BUILD_APPNAME6 , + .xsdt_tbl_offset = &xsdt, + .rsdt_tbl_offset =NULL ,nit: no need for this explicit NULLing
then one would get random junk from the stack this variable is allocate from in this field.
Quoted 4 lines
Unfortunately, yes.
Cheers,
Samuel.
Re: [Qemu-arm] [Qemu-devel] [PATCH v2 4/8] hw: arm: Carry RSDP specific data through AcpiRsdpData
In reply to Samuel OrtizQuoted 5 lines · On Thu, 29 Nov 2018 14:24:24 +0100 Samuel Ortiz <[email protected]> wrote:
That will allow us to generalize the ARM
build_rsdp()routine to support both legacy RSDP (The current i386 implementation) and extended RSDP (The ARM implementation).Signed-off-by Samuel Ortiz <[email protected]>
Quoted 72 lines
--- include/hw/acpi/acpi-defs.h | 8 ++++++++ hw/arm/virt-acpi-build.c | 18 +++++++++++++----- 2 files changed, 21 insertions(+), 5 deletions(-)include/hw/acpi/acpi-defs.h +8
-0@@ -53,6 +53,14 @@ struct AcpiRsdpDescriptor { /* Root System Descriptor Pointer */ }QEMU_PACKED ;typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor ; +typedef struct AcpiRsdpData { +uint8_t oem_id[6 ];/* OEM identification */ +uint8_t revision;/* Must be 0 for 1.0, 2 for 2.0 */ + +unsigned *rsdt_tbl_offset; +unsigned *xsdt_tbl_offset; +}AcpiRsdpData ; +/* Table structure from Linux kernel (the ACPI tables are under the BSD license) */ hw/arm/virt-acpi-build.c +13
-5@@ -368,7 +368,7 @@ static void acpi_dsdt_add_power_button(Aml *scope)/* RSDP */ static void -build_rsdp (GArray *rsdp_table,BIOSLinker *linker,unsigned xsdt_tbl_offset) +build_rsdp (GArray *rsdp_table,BIOSLinker *linker,AcpiRsdpData *rsdp_data) {AcpiRsdpDescriptor *rsdp =acpi_data_push (rsdp_table,sizeof *rsdp);unsigned xsdt_pa_size =sizeof (rsdp->xsdt_physical_address);@@ -379,14 +379,14 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset) true /* fseg memory */);memcpy (&rsdp->signature,"RSD PTR " ,sizeof (rsdp->signature)); -memcpy (rsdp->oem_id,ACPI_BUILD_APPNAME6 ,sizeof (rsdp->oem_id)); +memcpy (rsdp->oem_id, rsdp_data->oem_id,sizeof (rsdp->oem_id)); rsdp->length =cpu_to_le32 (sizeof (*rsdp)); - rsdp->revision =0x02 ; + rsdp->revision = rsdp_data->revision;/* Address to be filled by Guest linker */ bios_linker_loader_add_pointer (linker,ACPI_BUILD_RSDP_FILE , xsdt_pa_offset, xsdt_pa_size, -ACPI_BUILD_TABLE_FILE , xsdt_tbl_offset); +ACPI_BUILD_TABLE_FILE , *rsdp_data->xsdt_tbl_offset);/* Checksum to be filled by Guest linker */ bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,@@ -857,7 +857,15 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)build_xsdt (tables_blob, tables->linker, table_offsets,NULL ,NULL );/* RSDP is in FSEG memory, so allocate it separately */ -build_rsdp (tables->rsdp, tables->linker, xsdt); + { +AcpiRsdpData rsdp_data = { + .revision =2 , + .oem_id =ACPI_BUILD_APPNAME6 , + .xsdt_tbl_offset = &xsdt, + .rsdt_tbl_offset =NULL , + }; +build_rsdp (tables->rsdp, tables->linker, &rsdp_data); + }/* Cleanup memory that's no longer used. */ g_array_free (table_offsets, true);
Re: [Qemu-arm] [Qemu-devel] [PATCH v2 5/8] hw: arm: Convert the RSDP build to the buid_append_foo() API
In reply to Samuel OrtizQuoted 6 lines · On Thu, 29 Nov 2018 14:24:25 +0100 Samuel Ortiz <[email protected]> wrote:
Instead of filling a mapped and packed C structure field in random order and being careful about endianness and sizes,
build_rsdp()now usesbuild_append_int_noprefix()to compose RSDP table.This makes for an easier to review and maintain code that is almost matching 1:1 the ACPI spec itself.
nit if you happen to respin:
This makes review and maintaining code easier and ..
Quoted 44 lines
Signed-off-by Samuel Ortiz <[email protected]>--- hw/arm/virt-acpi-build.c | 42 +++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 20 deletions(-)hw/arm/virt-acpi-build.c +12
-15@@ -368,35 +368,37 @@ static void acpi_dsdt_add_power_button(Aml *scope)/* RSDP */ static void -build_rsdp (GArray *rsdp_table,BIOSLinker *linker,AcpiRsdpData *rsdp_data) +build_rsdp (GArray *tbl,BIOSLinker *linker,AcpiRsdpData *rsdp_data) { -AcpiRsdpDescriptor *rsdp =acpi_data_push (rsdp_table,sizeof *rsdp); -unsigned xsdt_pa_size =sizeof (rsdp->xsdt_physical_address); -unsigned xsdt_pa_offset = - (char *)&rsdp->xsdt_physical_address - rsdp_table->data; - -bios_linker_loader_alloc (linker,ACPI_BUILD_RSDP_FILE , rsdp_table,16 , +bios_linker_loader_alloc (linker,ACPI_BUILD_RSDP_FILE , tbl,16 , true/* fseg memory */ ); -memcpy (&rsdp->signature,"RSD PTR " ,sizeof (rsdp->signature)); -memcpy (rsdp->oem_id, rsdp_data->oem_id,sizeof (rsdp->oem_id)); - rsdp->length =cpu_to_le32 (sizeof (*rsdp)); - rsdp->revision = rsdp_data->revision; +g_array_append_vals (tbl,"RSD PTR " ,8 );/* Signature */ +build_append_int_noprefix (tbl,0 ,1 );/* Checksum */ +g_array_append_vals (tbl, rsdp_data->oem_id,6 );/* OEMID */ +build_append_int_noprefix (tbl, rsdp_data->revision,1 );/* Revision */ +build_append_int_noprefix (tbl,0 ,4 );/* RsdtAddress */ +build_append_int_noprefix (tbl,36 ,4 );/* Length */ -/* Address to be filled by Guest linker */ -bios_linker_loader_add_pointer (linker, -ACPI_BUILD_RSDP_FILE , xsdt_pa_offset, xsdt_pa_size, -ACPI_BUILD_TABLE_FILE , *rsdp_data->xsdt_tbl_offset); +/* XSDT address to be filled by guest linker */ +build_append_int_noprefix (tbl,0 ,8 );/* XsdtAddress */ + bios_linker_loader_add_pointer(linker, ACPI_BUILD_RSDP_FILE, + 24, 8,
hw/arm/virt-acpi-build.c static void acpi_dsdt_add_power_button(Aml *scope) · line 393
Quoted 10 lines
hw/arm/virt-acpi-build.c +6
-2+ACPI_BUILD_TABLE_FILE , + *rsdp_data->xsdt_tbl_offset); -/* Checksum to be filled by Guest linker */ +build_append_int_noprefix (tbl,0 ,1 );/* Extended Checksum */ +build_append_int_noprefix (tbl,0 ,3 );/* Reserved */ + +/* Checksum to be filled by guest linker */ bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE, - (char *)rsdp - rsdp_table->data, 20 /* ACPI rev 1.0 RSDP size */,
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
this is always 0 with all current users and you a replacing it with 0 constant. That's fine but that assumes that above condition stays always true. I'd add assert at the beginning of function to make sure that
tbl length is 0
or as an alternative
build_rsdp(...)
{
start_off = tbl->len;
...
bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE, start_off, 20 ...
so it won't explode even if RSDP isn't at the beginning of file.
If you go for the later than bios_linker_loader_add_pointer() will also need similar treatment
Quoted 13 lines
hw/arm/virt-acpi-build.c +4
-3- (char *)&rsdp->checksum - rsdp_table->data); +0 ,20 ,/* ACPI rev 1.0 RSDP size */ + 8);/* Extended checksum to be filled by Guest linker */ bios_linker_loader_add_checksum (linker,ACPI_BUILD_RSDP_FILE , - (char *)rsdp - rsdp_table->data,36 /* ACPI rev 2.0 RSDP size */ , - (char *)&rsdp->extended_checksum - rsdp_table->data); +0 ,36 ,/* ACPI rev 2.0 RSDP size */ +32 ); } static void
Re: [Qemu-arm] [Qemu-devel] [PATCH v2 8/8] hw: acpi: Remove AcpiRsdpDescriptor and fix tests
In reply to Andrew JonesQuoted 123 lines · Andrew Jones
Quoted 66 lines · Samuel Ortiz
The only remaining AcpiRsdpDescriptor users are the ACPI utils for the BIOS table tests.
We remove that dependency and can thus remove the structure itself.Signed-off-by Samuel Ortiz <[email protected]>--- include/hw/acpi/acpi-defs.h | 13 ----------- tests/acpi-utils.h | 5 +++- tests/acpi-utils.c | 46 ++++++++++++++++++++++++++++++------- tests/bios-tables-test.c | 27 +++++++++++++++------- tests/vmgenid-test.c | 8 ++++--- 5 files changed, 66 insertions(+), 33 deletions(-)include/hw/acpi/acpi-defs.h +0
-13@@ -40,19 +40,6 @@ enum { ACPI_FADT_F_LOW_POWER_S0_IDLE_CAPABLE, }; -struct AcpiRsdpDescriptor {/* Root System Descriptor Pointer */ -uint64_t signature;/* ACPI signature, contains "RSD PTR " */ -uint8_t checksum;/* To make sum of struct == 0 */ -uint8_t oem_id [6 ];/* OEM identification */ -uint8_t revision;/* Must be 0 for 1.0, 2 for 2.0 */ -uint32_t rsdt_physical_address;/* 32-bit physical address of RSDT */ -uint32_t length;/* XSDT Length in bytes including hdr */ -uint64_t xsdt_physical_address;/* 64-bit physical address of XSDT */ -uint8_t extended_checksum;/* Checksum of entire table */ -uint8_t reserved [3 ];/* Reserved field must be 0 */ -}QEMU_PACKED ; -typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor ; - typedef struct AcpiRsdpData { uint8_t oem_id[6]; /* OEM identification */ uint8_t revision; /* Must be 0 for 1.0, 2 for 2.0 */tests/acpi-utils.h +4
-1@@ -82,6 +82,9 @@ typedef struct {uint8_t acpi_calc_checksum (const uint8_t *data,int len);uint32_t acpi_find_rsdp_address (void ); -void acpi_parse_rsdp_table (uint32_t addr,AcpiRsdpDescriptor *rsdp_table); +uint32_t acpi_get_rsdt_address (uint8_t *rsdp_table); +uint64_t acpi_get_xsdt_address (uint8_t *rsdp_table); +void acpi_parse_rsdp_table (uint32_t addr,uint8_t *rsdp_table, +uint8_t revision);#endif /* TEST_ACPI_UTILS_H */ tests/acpi-utils.c +2
-3@@ -52,14 +52,44 @@ uint32_t acpi_find_rsdp_address(void)return off; } -void acpi_parse_rsdp_table (uint32_t addr,AcpiRsdpDescriptor *rsdp_table) +uint32_t acpi_get_rsdt_address (uint8_t *rsdp_table) { -ACPI_READ_FIELD (rsdp_table->signature, addr); -ACPI_ASSERT_CMP64 (rsdp_table->signature,"RSD PTR " ); +uint32_t rsdt_physical_address;tests/acpi-utils.c uint32_t acpi_find_rsdp_address(void) · line 62
Quoted 10 lines
tests/acpi-utils.c +4
-5+uint8_t revision = rsdp_table[15 /* Revision offset */ ]; -ACPI_READ_FIELD (rsdp_table->checksum, addr); -ACPI_READ_ARRAY (rsdp_table->oem_id, addr); -ACPI_READ_FIELD (rsdp_table->revision, addr); -ACPI_READ_FIELD (rsdp_table->rsdt_physical_address, addr); -ACPI_READ_FIELD (rsdp_table->length, addr); +if (revision !=0 ) {/* ACPI 1.0 RSDP */ +return 0 ; + }So now we assume a zero offset can't be valid, which means we don't need pointers in the struct. Your answer that pointers are used for consistency is good enough for me though.
tests/acpi-utils.c uint32_t acpi_find_rsdp_address(void) · line 73
Quoted 11 lines
tests/acpi-utils.c +11
-0+ +memcpy (&rsdt_physical_address, &rsdp_table[16 /* RsdtAddress offset */ ],4 ); +return le32_to_cpu (rsdt_physical_address); +} + +uint64_t acpi_get_xsdt_address (uint8_t *rsdp_table) +{ +uint64_t xsdt_physical_address; +uint8_t revision = rsdp_table[15 /* Revision offset */ ]; + +if (revision !=2 ) {/* ACPI 2.0+ RSDP */ revision < 2
Quoted 27 lines
tests/acpi-utils.c +21
-0+return 0 ; + } + +memcpy (&xsdt_physical_address, &rsdp_table[24 /* XsdtAddress offset */ ],8 ); +return le64_to_cpu (xsdt_physical_address); +} + +void acpi_parse_rsdp_table (uint32_t addr,uint8_t *rsdp_table,uint8_t revision) +{ +switch (revision) { +case 0 :/* ACPI 1.0 RSDP */ +memread (addr, rsdp_table,20 ); +break ; +case 2 :/* ACPI 2.0+ RSDP */ +memread (addr, rsdp_table,36 ); +break ; +default : +g_assert_not_reached (); + } + +ACPI_ASSERT_CMP64 (*((uint64_t *)(rsdp_table)),"RSD PTR " ); }tests/bios-tables-test.c +0
-0@@ -27,7 +27,8 @@ typedef struct {
Quoted 8 lines
tests/bios-tables-test.c typedef struct { · line 31
Quoted 6 lines
tests/bios-tables-test.c +2
-1const char *machine;const char *variant;uint32_t rsdp_addr; -AcpiRsdpDescriptor rsdp_table; +uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */ ]; +uint32_t rsdt_physical_address;I don't see
rsdt_physical_addressgetting used anyway.
Good catch, thanks. Removed in v3.
Cheers,
Samuel.
Re: [Qemu-arm] [PATCH v2 6/8] hw: arm: Support both legacy and current RSDP build
In reply to Samuel OrtizQuoted 6 lines · On Thu, 29 Nov 2018 14:24:26 +0100 Samuel Ortiz <[email protected]> wrote:
We add the ability to build legacy or current RSDP tables, based on the AcpiRsdpData revision field passed to
build_rsdp().
Although arm/virt only uses RSDP v2, adding that capability tobuild_rsdpwill allow us to share the RSDP build code between ARM and x86.Signed-off-by Samuel Ortiz <[email protected]>
Quoted 58 lines
--- hw/arm/virt-acpi-build.c | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-)hw/arm/virt-acpi-build.c +26
-11@@ -378,23 +378,38 @@ build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data)g_array_append_vals (tbl, rsdp_data->oem_id,6 );/* OEMID */ build_append_int_noprefix (tbl, rsdp_data->revision,1 );/* Revision */ build_append_int_noprefix (tbl,0 ,4 );/* RsdtAddress */ -build_append_int_noprefix (tbl,36 ,4 );/* Length */ - -/* XSDT address to be filled by guest linker */ -build_append_int_noprefix (tbl,0 ,8 );/* XsdtAddress */ -bios_linker_loader_add_pointer (linker,ACPI_BUILD_RSDP_FILE , -24 ,8 , -ACPI_BUILD_TABLE_FILE , - *rsdp_data->xsdt_tbl_offset); - -build_append_int_noprefix (tbl,0 ,1 );/* Extended Checksum */ -build_append_int_noprefix (tbl,0 ,3 );/* Reserved */ +if (rsdp_data->rsdt_tbl_offset) { +/* RSDT address to be filled by guest linker */ +bios_linker_loader_add_pointer (linker, +ACPI_BUILD_RSDP_FILE ,16 ,4 , +ACPI_BUILD_TABLE_FILE , + *rsdp_data->rsdt_tbl_offset); + }/* Checksum to be filled by guest linker */ bios_linker_loader_add_checksum (linker,ACPI_BUILD_RSDP_FILE ,0 ,20 ,/* ACPI rev 1.0 RSDP size */ 8 ); +if (rsdp_data->revision ==0 ) { +/* ACPI 1.0 RSDP, we're done */ +return ; + } + +/* The RSDP revision is 2 and later, we must have an XSDT pointer */ +g_assert (rsdp_data->xsdt_tbl_offset !=NULL ); + +build_append_int_noprefix (tbl,36 ,4 );/* Length */ + +/* XSDT address to be filled by guest linker */ +build_append_int_noprefix (tbl,0 ,8 );/* XsdtAddress */ +bios_linker_loader_add_pointer (linker,ACPI_BUILD_RSDP_FILE ,24 ,8 , +ACPI_BUILD_TABLE_FILE , + *rsdp_data->xsdt_tbl_offset); + +build_append_int_noprefix (tbl,0 ,1 );/* Extended Checksum */ +build_append_int_noprefix (tbl,0 ,3 );/* Reserved */ +/* Extended checksum to be filled by Guest linker */ bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE, 0, 36, /* ACPI rev 2.0 RSDP size */
Re: [Qemu-arm] [Qemu-devel] [PATCH v2 6/8] hw: arm: Support both legacy and current RSDP build
In reply to Samuel OrtizQuoted 57 lines · On Thu, 29 Nov 2018 15:59:42 +0100 Samuel Ortiz <[email protected]> wrote:
Quoted 56 lines · Andrew Jones
Quoted 20 lines · Samuel Ortiz
We add the ability to build legacy or current RSDP tables, based on the AcpiRsdpData revision field passed to
build_rsdp().
Although arm/virt only uses RSDP v2, adding that capability tobuild_rsdpwill allow us to share the RSDP build code between ARM and x86.Signed-off-by Samuel Ortiz <[email protected]>--- hw/arm/virt-acpi-build.c | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-)hw/arm/virt-acpi-build.c +0
-2@@ -378,23 +378,38 @@ build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data)g_array_append_vals (tbl, rsdp_data->oem_id,6 );/* OEMID */ build_append_int_noprefix (tbl, rsdp_data->revision,1 );/* Revision */ build_append_int_noprefix (tbl,0 ,4 );/* RsdtAddress */ -build_append_int_noprefix (tbl,36 ,4 );/* Length */ -hw/arm/virt-acpi-build.c build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data) · line 381
Quoted 10 lines
hw/arm/virt-acpi-build.c +1
-9-/* XSDT address to be filled by guest linker */ -build_append_int_noprefix (tbl,0 ,8 );/* XsdtAddress */ -bios_linker_loader_add_pointer (linker,ACPI_BUILD_RSDP_FILE , -24 ,8 , -ACPI_BUILD_TABLE_FILE , - *rsdp_data->xsdt_tbl_offset); - -build_append_int_noprefix (tbl,0 ,1 );/* Extended Checksum */ -build_append_int_noprefix (tbl,0 ,3 );/* Reserved */ +if (rsdp_data->rsdt_tbl_offset) {I see why a pointer was used now. Using a pointer ensures a zero offset won't fail this test. However the test could be replaced with
rsdp_data->revision== 0.Quoted 19 lines
hw/arm/virt-acpi-build.c +13
-0+/* RSDT address to be filled by guest linker */ +bios_linker_loader_add_pointer (linker, +ACPI_BUILD_RSDP_FILE ,16 ,4 , +ACPI_BUILD_TABLE_FILE , + *rsdp_data->rsdt_tbl_offset); + }/* Checksum to be filled by guest linker */ bios_linker_loader_add_checksum (linker,ACPI_BUILD_RSDP_FILE ,0 ,20 ,/* ACPI rev 1.0 RSDP size */ 8 ); +if (rsdp_data->revision ==0 ) { +/* ACPI 1.0 RSDP, we're done */ +return ; + } + +/* The RSDP revision is 2 and later, we must have an XSDT pointer */ +g_assert (rsdp_data->xsdt_tbl_offset !=NULL );
Quoted 3 lines
Quoted 1 line
So here's the justification for the pointers. We sanity check the callers.
We could sanity check the callers without pointers as well, I don't think there's a strong advantage for pointers here, except consistence.
I remember now,
we need pointer to RSDT offset here so that we could skip bios_linker_loader_add_pointer(rsdt) when RSDT isn't provided (0 is also a valid offset), as for xsdt field it follows rsdt one for consistency and we can do sanity checks on it.
assert here seems not really necessary as it guarantied to SIGSEGV on NULL point dereference at bios_linker_loader_add_pointer(*xdst) callsite. But I don't care here, so I'll leave it up to Samuel.
Quoted 5 lines
Quoted 2 lines
We're missing the (
rsdp_data->revision== 0 &&rsdp_data->rsdt_tbl_offset) sanity check though.I think there's nothing preventing a caller to include both rsdt and xsdt if it wants to be able to run on both < 2.0 and 2.0+ platforms with the same table. So if rsdt is set we should add it, regardless of the revision.
agreed,
we don't do this in QEMU but it's correct, there could be both and it's better follow better document spec in this case.
Quoted 3 lines
Cheers,
Samuel.
Re: [Qemu-devel] [PATCH v2 7/8] hw: acpi: Export and share the ARM RSDP build
In reply to Samuel OrtizQuoted 7 lines · On Thu, 29 Nov 2018 14:24:27 +0100 Samuel Ortiz <[email protected]> wrote:
Now that
build_rsdp()supports building both legacy and current RSDP tables, we can move it to a generic folder (hw/acpi) and have the i386 ACPI code reuse it in order to reduce code duplication.Signed-off-by Samuel Ortiz <[email protected]>Reviewed-by Philippe Mathieu-Daudé <[email protected]>
with nit below fixed
Quoted 86 lines
--- include/hw/acpi/aml-build.h | 2 ++ hw/acpi/aml-build.c | 50 +++++++++++++++++++++++++++++++++++++ hw/arm/virt-acpi-build.c | 50 ------------------------------------- hw/i386/acpi-build.c | 39 +++++++---------------------- 4 files changed, 61 insertions(+), 80 deletions(-)include/hw/acpi/aml-build.h +2
-0@@ -388,6 +388,8 @@ void acpi_add_table(GArray *table_offsets, GArray *table_data);void acpi_build_tables_init (AcpiBuildTables *tables);void acpi_build_tables_cleanup (AcpiBuildTables *tables,bool mfre);void +build_rsdp (GArray *tbl,BIOSLinker *linker,AcpiRsdpData *rsdp_data); +void build_rsdt (GArray *table_data,BIOSLinker *linker,GArray *table_offsets,const char *oem_id,const char *oem_table_id);void hw/acpi/aml-build.c +50
-0@@ -1589,6 +1589,56 @@ void acpi_build_tables_cleanup(AcpiBuildTables *tables, bool mfre)g_array_free (tables->vmgenid, mfre); } +/* RSDP */ +void +build_rsdp (GArray *tbl,BIOSLinker *linker,AcpiRsdpData *rsdp_data) +{ +bios_linker_loader_alloc (linker,ACPI_BUILD_RSDP_FILE , tbl,16 , + true/* fseg memory */ ); + +g_array_append_vals (tbl,"RSD PTR " ,8 );/* Signature */ +build_append_int_noprefix (tbl,0 ,1 );/* Checksum */ +g_array_append_vals (tbl, rsdp_data->oem_id,6 );/* OEMID */ +build_append_int_noprefix (tbl, rsdp_data->revision,1 );/* Revision */ +build_append_int_noprefix (tbl,0 ,4 );/* RsdtAddress */ +if (rsdp_data->rsdt_tbl_offset) { +/* RSDT address to be filled by guest linker */ +bios_linker_loader_add_pointer (linker, +ACPI_BUILD_RSDP_FILE ,16 ,4 , +ACPI_BUILD_TABLE_FILE , + *rsdp_data->rsdt_tbl_offset); + } + +/* Checksum to be filled by guest linker */ +bios_linker_loader_add_checksum (linker,ACPI_BUILD_RSDP_FILE , +0 ,20 ,/* ACPI rev 1.0 RSDP size */ +8 ); + +if (rsdp_data->revision ==0 ) { +/* ACPI 1.0 RSDP, we're done */ +return ; + } + +/* The RSDP revision is 2 and later, we must have an XSDT pointer */ +g_assert (rsdp_data->xsdt_tbl_offset !=NULL ); + +build_append_int_noprefix (tbl,36 ,4 );/* Length */ + +/* XSDT address to be filled by guest linker */ +build_append_int_noprefix (tbl,0 ,8 );/* XsdtAddress */ +bios_linker_loader_add_pointer (linker,ACPI_BUILD_RSDP_FILE ,24 ,8 , +ACPI_BUILD_TABLE_FILE , + *rsdp_data->xsdt_tbl_offset); + +build_append_int_noprefix (tbl,0 ,1 );/* Extended Checksum */ +build_append_int_noprefix (tbl,0 ,3 );/* Reserved */ + +/* Extended checksum to be filled by Guest linker */ +bios_linker_loader_add_checksum (linker,ACPI_BUILD_RSDP_FILE , +0 ,36 ,/* ACPI rev 2.0 RSDP size */ +32 ); +} +/* Build rsdt table */ void build_rsdt(GArray *table_data, BIOSLinker *linker, GArray *table_offsets,hw/arm/virt-acpi-build.c +0
-0
hw/arm/virt-acpi-build.c static void acpi_dsdt_add_power_button(Aml *scope) · line 369
Quoted 4 lines
hw/arm/virt-acpi-build.c +0
-1@@ -366,56 +366,6 @@ static void acpi_dsdt_add_power_button(Aml *scope)aml_append (scope, dev); } -/* RSDP */
I'd make comment more descriptive, see build_slit() for example
Quoted 109 lines
hw/arm/virt-acpi-build.c +0
-49-static void -build_rsdp (GArray *tbl,BIOSLinker *linker,AcpiRsdpData *rsdp_data) -{ -bios_linker_loader_alloc (linker,ACPI_BUILD_RSDP_FILE , tbl,16 , - true/* fseg memory */ ); - -g_array_append_vals (tbl,"RSD PTR " ,8 );/* Signature */ -build_append_int_noprefix (tbl,0 ,1 );/* Checksum */ -g_array_append_vals (tbl, rsdp_data->oem_id,6 );/* OEMID */ -build_append_int_noprefix (tbl, rsdp_data->revision,1 );/* Revision */ -build_append_int_noprefix (tbl,0 ,4 );/* RsdtAddress */ -if (rsdp_data->rsdt_tbl_offset) { -/* RSDT address to be filled by guest linker */ -bios_linker_loader_add_pointer (linker, -ACPI_BUILD_RSDP_FILE ,16 ,4 , -ACPI_BUILD_TABLE_FILE , - *rsdp_data->rsdt_tbl_offset); - } - -/* Checksum to be filled by guest linker */ -bios_linker_loader_add_checksum (linker,ACPI_BUILD_RSDP_FILE , -0 ,20 ,/* ACPI rev 1.0 RSDP size */ -8 ); - -if (rsdp_data->revision ==0 ) { -/* ACPI 1.0 RSDP, we're done */ -return ; - } - -/* The RSDP revision is 2 and later, we must have an XSDT pointer */ -g_assert (rsdp_data->xsdt_tbl_offset !=NULL ); - -build_append_int_noprefix (tbl,36 ,4 );/* Length */ - -/* XSDT address to be filled by guest linker */ -build_append_int_noprefix (tbl,0 ,8 );/* XsdtAddress */ -bios_linker_loader_add_pointer (linker,ACPI_BUILD_RSDP_FILE ,24 ,8 , -ACPI_BUILD_TABLE_FILE , - *rsdp_data->xsdt_tbl_offset); - -build_append_int_noprefix (tbl,0 ,1 );/* Extended Checksum */ -build_append_int_noprefix (tbl,0 ,3 );/* Reserved */ - -/* Extended checksum to be filled by Guest linker */ -bios_linker_loader_add_checksum (linker,ACPI_BUILD_RSDP_FILE , -0 ,36 ,/* ACPI rev 2.0 RSDP size */ -32 ); -} -static void build_iort (GArray *table_data,BIOSLinker *linker,VirtMachineState *vms) {hw/i386/acpi-build.c +9
-30@@ -2547,35 +2547,6 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker) "IVRS", table_data->len - iommu_start, 1, NULL, NULL); } -static void -build_rsdp (GArray *rsdp_table,BIOSLinker *linker,unsigned rsdt_tbl_offset) -{ -/* AcpiRsdpDescriptor describes revision 2 RSDP table and as result we -* allocate extra 16 bytes for pc/q35 RSDP rev1 as well. Keep extra 16 bytes -* wasted to make sure we won't breake migration for machine types older -* than 2.3 due to size mismatch. -*/ -AcpiRsdpDescriptor *rsdp =acpi_data_push (rsdp_table,sizeof *rsdp); -unsigned rsdt_pa_size =sizeof (rsdp->rsdt_physical_address); -unsigned rsdt_pa_offset = - (char *)&rsdp->rsdt_physical_address - rsdp_table->data; - -bios_linker_loader_alloc (linker,ACPI_BUILD_RSDP_FILE , rsdp_table,16 , - true/* fseg memory */ ); - -memcpy (&rsdp->signature,"RSD PTR " ,8 ); -memcpy (rsdp->oem_id,ACPI_BUILD_APPNAME6 ,6 ); -/* Address to be filled by Guest linker */ -bios_linker_loader_add_pointer (linker, -ACPI_BUILD_RSDP_FILE , rsdt_pa_offset, rsdt_pa_size, -ACPI_BUILD_TABLE_FILE , rsdt_tbl_offset); - -/* Checksum to be filled by Guest linker */ -bios_linker_loader_add_checksum (linker,ACPI_BUILD_RSDP_FILE , - (char *)rsdp - rsdp_table->data,20 /* ACPI rev 1.0 RSDP size */ , - (char *)&rsdp->checksum - rsdp_table->data); -} - typedef struct AcpiBuildState { /* Copy of table in RAM (for patching). */@@ -2732,7 +2703,15 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine) slic_oem.id, slic_oem.table_id);/* RSDP is in FSEG memory, so allocate it separately */ -build_rsdp (tables->rsdp, tables->linker, rsdt); + { +AcpiRsdpData rsdp_data = { + .revision =0 , + .oem_id =ACPI_BUILD_APPNAME6 , + .xsdt_tbl_offset =NULL , + .rsdt_tbl_offset = &rsdt, + }; +build_rsdp (tables->rsdp, tables->linker, &rsdp_data); + }/* We'll expose it all to Guest so we want to reduce * chance of size changes.
Re: [Qemu-arm] [Qemu-devel] [PATCH v2 8/8] hw: acpi: Remove AcpiRsdpDescriptor and fix tests
In reply to Samuel OrtizQuoted 66 lines · On Thu, 29 Nov 2018 14:24:28 +0100 Samuel Ortiz <[email protected]> wrote:
The only remaining AcpiRsdpDescriptor users are the ACPI utils for the BIOS table tests.
We remove that dependency and can thus remove the structure itself.Signed-off-by Samuel Ortiz <[email protected]>--- include/hw/acpi/acpi-defs.h | 13 ----------- tests/acpi-utils.h | 5 +++- tests/acpi-utils.c | 46 ++++++++++++++++++++++++++++++------- tests/bios-tables-test.c | 27 +++++++++++++++------- tests/vmgenid-test.c | 8 ++++--- 5 files changed, 66 insertions(+), 33 deletions(-)include/hw/acpi/acpi-defs.h +0
-13@@ -40,19 +40,6 @@ enum { ACPI_FADT_F_LOW_POWER_S0_IDLE_CAPABLE, }; -struct AcpiRsdpDescriptor {/* Root System Descriptor Pointer */ -uint64_t signature;/* ACPI signature, contains "RSD PTR " */ -uint8_t checksum;/* To make sum of struct == 0 */ -uint8_t oem_id [6 ];/* OEM identification */ -uint8_t revision;/* Must be 0 for 1.0, 2 for 2.0 */ -uint32_t rsdt_physical_address;/* 32-bit physical address of RSDT */ -uint32_t length;/* XSDT Length in bytes including hdr */ -uint64_t xsdt_physical_address;/* 64-bit physical address of XSDT */ -uint8_t extended_checksum;/* Checksum of entire table */ -uint8_t reserved [3 ];/* Reserved field must be 0 */ -}QEMU_PACKED ; -typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor ; - typedef struct AcpiRsdpData { uint8_t oem_id[6]; /* OEM identification */ uint8_t revision; /* Must be 0 for 1.0, 2 for 2.0 */tests/acpi-utils.h +4
-1@@ -82,6 +82,9 @@ typedef struct {uint8_t acpi_calc_checksum (const uint8_t *data,int len);uint32_t acpi_find_rsdp_address (void ); -void acpi_parse_rsdp_table (uint32_t addr,AcpiRsdpDescriptor *rsdp_table); +uint32_t acpi_get_rsdt_address (uint8_t *rsdp_table); +uint64_t acpi_get_xsdt_address (uint8_t *rsdp_table); +void acpi_parse_rsdp_table (uint32_t addr,uint8_t *rsdp_table, +uint8_t revision);#endif /* TEST_ACPI_UTILS_H */ tests/acpi-utils.c +2
-3@@ -52,14 +52,44 @@ uint32_t acpi_find_rsdp_address(void)return off; } -void acpi_parse_rsdp_table (uint32_t addr,AcpiRsdpDescriptor *rsdp_table) +uint32_t acpi_get_rsdt_address (uint8_t *rsdp_table) { -ACPI_READ_FIELD (rsdp_table->signature, addr); -ACPI_ASSERT_CMP64 (rsdp_table->signature,"RSD PTR " ); +uint32_t rsdt_physical_address;
tests/acpi-utils.c uint32_t acpi_find_rsdp_address(void) · line 62
Quoted 10 lines
tests/acpi-utils.c +4
-5+uint8_t revision = rsdp_table[15 /* Revision offset */ ]; -ACPI_READ_FIELD (rsdp_table->checksum, addr); -ACPI_READ_ARRAY (rsdp_table->oem_id, addr); -ACPI_READ_FIELD (rsdp_table->revision, addr); -ACPI_READ_FIELD (rsdp_table->rsdt_physical_address, addr); -ACPI_READ_FIELD (rsdp_table->length, addr); +if (revision !=0 ) {/* ACPI 1.0 RSDP */ +return 0 ; + }
It seems not needed,
Why not return whatever is in table?
tests/acpi-utils.c uint32_t acpi_find_rsdp_address(void) · line 73
Quoted 11 lines
tests/acpi-utils.c +11
-0+ +memcpy (&rsdt_physical_address, &rsdp_table[16 /* RsdtAddress offset */ ],4 ); +return le32_to_cpu (rsdt_physical_address); +} + +uint64_t acpi_get_xsdt_address (uint8_t *rsdp_table) +{ +uint64_t xsdt_physical_address; +uint8_t revision = rsdp_table[15 /* Revision offset */ ]; + +if (revision !=2 ) {/* ACPI 2.0+ RSDP */
maybe replace this with assert, if we are getting XSDT, revision must be 2 or more
tests/acpi-utils.c uint32_t acpi_find_rsdp_address(void) · line 82
Quoted 9 lines
tests/acpi-utils.c +9
-0+return 0 ; + } + +memcpy (&xsdt_physical_address, &rsdp_table[24 /* XsdtAddress offset */ ],8 ); +return le64_to_cpu (xsdt_physical_address); +} + +void acpi_parse_rsdp_table (uint32_t addr,uint8_t *rsdp_table,uint8_t revision) +{
why do we need pass in revision?
It would be better if we read sub-table first, then parse revision and then read the rest if necessary.
Then it's up to a caller what do with the table that we've fetched here
Quoted 28 lines
tests/acpi-utils.c +12
-0+switch (revision) { +case 0 :/* ACPI 1.0 RSDP */ +memread (addr, rsdp_table,20 ); +break ; +case 2 :/* ACPI 2.0+ RSDP */ +memread (addr, rsdp_table,36 ); +break ; +default : +g_assert_not_reached (); + } + +ACPI_ASSERT_CMP64 (*((uint64_t *)(rsdp_table)),"RSD PTR " ); }tests/bios-tables-test.c +2
-1@@ -27,7 +27,8 @@ typedef struct {const char *machine;const char *variant;uint32_t rsdp_addr; -AcpiRsdpDescriptor rsdp_table; +uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */ ]; +uint32_t rsdt_physical_address;AcpiRsdtDescriptorRev1 rsdt_table;uint32_t dsdt_addr;uint32_t facs_addr;
tests/bios-tables-test.c static void test_acpi_rsdp_address(test_data *data) · line 87
Quoted 5 lines
tests/bios-tables-test.c +1
-1@@ -83,21 +84,31 @@ static void test_acpi_rsdp_address(test_data *data) data->rsdp_addr = off; } -static void test_acpi_rsdp_table (test_data *data) +static void test_acpi_rsdp_table (test_data *data,uint8_t revision)
like wise, I'd drop revision here and use whatever is in the table if we need to test for specific revision in the future we can add test case for that then.
Quoted 68 lines
tests/bios-tables-test.c +16
-6{ -AcpiRsdpDescriptor *rsdp_table = &data->rsdp_table; +uint8_t *rsdp_table = data->rsdp_table;uint32_t addr = data->rsdp_addr; -acpi_parse_rsdp_table (addr, rsdp_table); +acpi_parse_rsdp_table (addr, rsdp_table, revision); -/* rsdp checksum is not for the whole table, but for the first 20 bytes */ -g_assert (!acpi_calc_checksum ((uint8_t *)rsdp_table,20 )); +switch (revision) { +case 0 :/* ACPI 1.0 RSDP */ +/* With rev 1, checksum is only for the first 20 bytes */ +g_assert (!acpi_calc_checksum (rsdp_table,20 )); +break ; +case 2 :/* ACPI 2.0+ RSDP */ +/* With revision 2, we have 2 checksums */ +g_assert (!acpi_calc_checksum (rsdp_table,20 )); +g_assert (!acpi_calc_checksum (rsdp_table,36 )); +default : +g_assert_not_reached (); + } }static void test_acpi_rsdt_table (test_data *data) {AcpiRsdtDescriptorRev1 *rsdt_table = &data->rsdt_table; -uint32_t addr =le32_to_cpu (data->rsdp_table.rsdt_physical_address); +uint32_t addr =acpi_get_rsdt_address (data->rsdp_table);uint32_t *tables;int tables_nr;uint8_t checksum;@@ -626,7 +637,7 @@ static void test_acpi_one(const char *params, test_data *data) data->tables =g_array_new (false, true,sizeof (AcpiSdtTable));test_acpi_rsdp_address (data); -test_acpi_rsdp_table (data); +test_acpi_rsdp_table (data,0 );/* ACPI 1.0 RSDP */ test_acpi_rsdt_table (data);fadt_fetch_facs_and_dsdt_ptrs (data);test_acpi_facs_table (data);tests/vmgenid-test.c +5
-3@@ -35,7 +35,7 @@ static uint32_t acpi_find_vgia(void) {uint32_t rsdp_offset;uint32_t guid_offset =0 ; -AcpiRsdpDescriptor rsdp_table; +uint8_t rsdp_table[36 /* ACPI 2.0+ RSDP size */ ];uint32_t rsdt, rsdt_table_length;AcpiRsdtDescriptorRev1 rsdt_table;size_t tables_nr;@@ -52,9 +52,11 @@ static uint32_t acpi_find_vgia(void) g_assert_cmphex(rsdp_offset, <, RSDP_ADDR_INVALID); -acpi_parse_rsdp_table (rsdp_offset, &rsdp_table); +acpi_parse_rsdp_table (rsdp_offset, rsdp_table,0 ); + + rsdt =acpi_get_rsdt_address (rsdp_table); +g_assert (rsdt); - rsdt =le32_to_cpu (rsdp_table.rsdt_physical_address);/* read the header */ ACPI_READ_TABLE_HEADER (&rsdt_table, rsdt);ACPI_ASSERT_CMP (rsdt_table.signature,"RSDT" );
Quoted 13 lines · On Thu, 29 Nov 2018 14:24:20 +0100 Samuel Ortiz <[email protected]> wrote:
This patch serie fixes a couple of RSDP checksum related issues:
- On RSDP rev2, we are not adding the extended checksum and computing the checksum on the full table instead of the first 20 bytes.
- On RSDP rev1, we are computing the checksum on 36 bytes instead of 20. We're lucky enough that this is only adding zeroes to the checksum.A guest Linux kernel does not seem to care about RSDP checksums, so those 2 fixes are mostly for correctness sake.
Any machine type that generates rev2 RSDP will see its RSDP table modified but since x86 builds RSDP v1 and all ACPI tests only run on either pc or q35, the ACPI tests tables are not affected by this fix.
Quoted 6 lines
The serie also extends the ARM virt ACPI RSDP build routine to support both RSDP v1 and v2, in order to share this code between x86 and aarch64. While extending, we also convert the routine to the latest
build_append_foo()API. The new implementation is a closer reflection of the ACPI spec itself, is endian agnostic and allows for getting rid of the AcpiRsdpDescriptor structure.
Series needs to fix up minor issues but otherwise it looks good.
Quoted 30 lines
v1 -> v2:
* Add missing SOBs.
* Rephrase some commit messages according to the maintainer's input.
* Replace ACPI spec definitions with hard coded constants.
* Replaceinit_rsdp_data()helper with open coded rsdp variable init.
* Rename acpi_find_[rx]sdt_address to acpi_get_[rx]sdt_address.Igor Mammedov (2):
hw: arm: acpi: Fix incorrect checksums in RSDP
hw: i386: Use correct RSDT length for checksumSamuel Ortiz (6):
hw: acpi: The RSDP build API can return void
hw: arm: Carry RSDP specific data through AcpiRsdpData
hw: arm: Convert the RSDP build to thebuid_append_foo()API
hw: arm: Support both legacy and current RSDP build
hw: acpi: Export and share the ARM RSDP build
hw: acpi: Remove AcpiRsdpDescriptor and fix testsinclude/hw/acpi/acpi-defs.h | 19 ++++++-------- include/hw/acpi/aml-build.h | 2 ++ tests/acpi-utils.h | 5 +++- hw/acpi/aml-build.c | 50 +++++++++++++++++++++++++++++++++++++ hw/arm/virt-acpi-build.c | 40 +++++++---------------------- hw/i386/acpi-build.c | 36 +++++++------------------- tests/acpi-utils.c | 46 ++++++++++++++++++++++++++++------ tests/bios-tables-test.c | 27 ++++++++++++++------ tests/vmgenid-test.c | 8 +++--- 9 files changed, 143 insertions(+), 90 deletions(-)
Re: [Qemu-arm] [Qemu-devel] [PATCH v2 4/8] hw: arm: Carry RSDP specific data through AcpiRsdpData
In reply to Igor MammedovQuoted 93 lines · Igor Mammedov
Quoted 28 lines · On Thu, 29 Nov 2018 15:26:27 +0100 Samuel Ortiz <[email protected]> wrote:
Quoted 19 lines · Andrew Jones
Quoted 18 lines · Samuel Ortiz
That will allow us to generalize the ARM
build_rsdp()routine to support both legacy RSDP (The current i386 implementation) and extended RSDP (The ARM implementation).Signed-off-by Samuel Ortiz <[email protected]>--- include/hw/acpi/acpi-defs.h | 8 ++++++++ hw/arm/virt-acpi-build.c | 18 +++++++++++++----- 2 files changed, 21 insertions(+), 5 deletions(-)include/hw/acpi/acpi-defs.h +0
-0@@ -53,6 +53,14 @@ struct AcpiRsdpDescriptor { /* Root System Descriptor Pointer */ }QEMU_PACKED ;typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor ;Quoted 8 lines
include/hw/acpi/acpi-defs.h struct AcpiRsdpDescriptor { /* Root System Descriptor Pointer */ · line 61
Quoted 6 lines
include/hw/acpi/acpi-defs.h +6
-0+typedef struct AcpiRsdpData { + uint8_t oem_id[6]; /* OEM identification */ + uint8_t revision; /* Must be 0 for 1.0, 2 for 2.0 */ + + unsigned *rsdt_tbl_offset; + unsigned *xsdt_tbl_offset;Why use pointers?
Quoted 1 line
Mostly to be consistent with the FADT data structure (AcpiFadtData).
My guess would be that's for detecting an optional field and skipping setting it in
build_rsdp(). But it's mostly for consistence with current code.Quoted 22 lines
Quoted 22 lines
Quoted 22 lines
include/hw/acpi/acpi-defs.h +2
-0+} AcpiRsdpData; +/* Table structure from Linux kernel (the ACPI tables are under the BSD license) */ hw/arm/virt-acpi-build.c +1
-1@@ -368,7 +368,7 @@ static void acpi_dsdt_add_power_button(Aml *scope)/* RSDP */ static void -build_rsdp (GArray *rsdp_table,BIOSLinker *linker,unsigned xsdt_tbl_offset) +build_rsdp (GArray *rsdp_table,BIOSLinker *linker,AcpiRsdpData *rsdp_data) {AcpiRsdpDescriptor *rsdp =acpi_data_push (rsdp_table,sizeof *rsdp);unsigned xsdt_pa_size =sizeof (rsdp->xsdt_physical_address);@@ -379,14 +379,14 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset) true /* fseg memory */);Quoted 35 lines
Quoted 5 lines
hw/arm/virt-acpi-build.c build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset) · line 382
Quoted 3 lines
hw/arm/virt-acpi-build.c +1
-1memcpy (&rsdp->signature,"RSD PTR " ,sizeof (rsdp->signature)); -memcpy (rsdp->oem_id,ACPI_BUILD_APPNAME6 ,sizeof (rsdp->oem_id)); +memcpy (rsdp->oem_id, rsdp_data->oem_id,sizeof (rsdp->oem_id));
sizeof(rsdp_data->oem_id)Which is the same thing. Note that this piece of code eventually gets removed later in the serie.
Quoted 25 lines
Quoted 13 lines
hw/arm/virt-acpi-build.c +2
-2rsdp->length =cpu_to_le32 (sizeof (*rsdp)); - rsdp->revision =0x02 ; + rsdp->revision = rsdp_data->revision;/* Address to be filled by Guest linker */ bios_linker_loader_add_pointer (linker,ACPI_BUILD_RSDP_FILE , xsdt_pa_offset, xsdt_pa_size, -ACPI_BUILD_TABLE_FILE , xsdt_tbl_offset); +ACPI_BUILD_TABLE_FILE , *rsdp_data->xsdt_tbl_offset);/* Checksum to be filled by Guest linker */ bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,hw/arm/virt-acpi-build.c void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables) · line 865
Quoted 10 lines
hw/arm/virt-acpi-build.c +6
-1@@ -857,7 +857,15 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)build_xsdt (tables_blob, tables->linker, table_offsets,NULL ,NULL );/* RSDP is in FSEG memory, so allocate it separately */ -build_rsdp (tables->rsdp, tables->linker, xsdt); + { +AcpiRsdpData rsdp_data = { + .revision =2 , + .oem_id =ACPI_BUILD_APPNAME6 , + .xsdt_tbl_offset = &xsdt, + .rsdt_tbl_offset =NULL ,nit: no need for this explicit NULLing
Quoted 2 lines
then one would get random junk from the stack this variable is allocate from in this field.
Not when you initialize the structure with = { ... }. All uninitialized pointers will be NULL and all uninitialized arithmetic types will be zero. Of course explicitly setting it to NULL is fine too, which is why this is only a "nit".
Quoted 7 lines
Quoted 5 lines
Unfortunately, yes.
Cheers,
Samuel.