Re: [Qemu-arm] [Qemu-devel] [PATCH v2 8/8] hw: acpi: Remove AcpiRsdpDescriptor and fix tests

[Igor Mammedov]

On Thu, 29 Nov 2018 14:24:28 +0100
Samuel Ortiz <address@hidden> 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 <address@hidden>
> ---
>  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(-)
> 
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index 8425ecb8c6..5021cb9e79 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -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 */
> diff --git a/tests/acpi-utils.h b/tests/acpi-utils.h
> index ac52abd0dd..c26c8976a6 100644
> --- a/tests/acpi-utils.h
> +++ b/tests/acpi-utils.h
> @@ -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 */
> diff --git a/tests/acpi-utils.c b/tests/acpi-utils.c
> index 41dc1ea9b4..b1113bda8b 100644
> --- a/tests/acpi-utils.c
> +++ b/tests/acpi-utils.c
> @@ -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;
> +    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?

> +
> +    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

> +        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

> +    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 ");
>  }
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index d661d9be62..84f1500920 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -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)
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.

>  {
> -    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);
> diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c
> index 0a6fb55f2e..5f30dee059 100644
> --- a/tests/vmgenid-test.c
> +++ b/tests/vmgenid-test.c
> @@ -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");