Re: [Qemu-devel] [PATCH v4 05/15] tests: acpi: fetch X_DSDT if pointer to DSDT is 0

[Igor Mammedov]

On Sun, 5 May 2019 09:27:45 +0800
Wei Yang <address@hidden> wrote:

> On Thu, May 02, 2019 at 04:51:53PM +0200, Igor Mammedov wrote:
> >that way it would be possible to test a DSDT pointed by
> >64bit X_DSDT field in FADT.
> >
> >PS:
> >it will allow to enable testing arm/virt board, which sets
> >only newer X_DSDT field.
> >
> >Signed-off-by: Igor Mammedov <address@hidden>
> >---
> >v4:
> > * dropping Reviewed-bys due to acpi_fetch_table() change
> >   introduced by earlier patch:
> >   "tests: acpi: make acpi_fetch_table() take size of fetched table pointer"
> >v2:
> >  add 'val = le32_to_cpu(val)' even if it doesn't necessary
> >  it works as reminder that value copied from table is in
> >  little-endian format (Philippe Mathieu-Daudé <address@hidden>)
> >---
> > tests/bios-tables-test.c | 11 ++++++++++-
> > 1 file changed, 10 insertions(+), 1 deletion(-)
> >
> >diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> >index a164d27..d165a1b 100644
> >--- a/tests/bios-tables-test.c
> >+++ b/tests/bios-tables-test.c
> >@@ -140,6 +140,9 @@ static void test_acpi_fadt_table(test_data *data)
> >     AcpiSdtTable table = g_array_index(data->tables, typeof(table), 0);
> >     uint8_t *fadt_aml = table.aml;
> >     uint32_t fadt_len = table.aml_len;
> >+    uint32_t val;
> >+    int dsdt_offset = 40 /* DSDT */;
> >+    int dsdt_entry_size = 4;
> > 
> >     g_assert(compare_signature(&table, "FACP"));
> > 
> >@@ -148,8 +151,14 @@ static void test_acpi_fadt_table(test_data *data)
> >                      fadt_aml + 36 /* FIRMWARE_CTRL */, 4, "FACS", false);
> >     g_array_append_val(data->tables, table);
> > 
> >+    memcpy(&val, fadt_aml + dsdt_offset, 4);
> >+    val = le32_to_cpu(val);
> >+    if (!val) {
> >+        dsdt_offset = 140 /* X_DSDT */;
> 
> In case we can point out where we get it, e.g. ACPI 5, Table 5-34 FADT Format.
> 
> This may be more helpful for reviewing and maintaining.

for fields we typically use only verbatim field name, so it would be easy
to find by searching for it in spec. In this case it is obvious about which
table it applies to, so reference to spec for a field probably excessive.

Complete reference necessary for tables and API functions that implement
ACPI primitive.

> 
> Do you think so?
> 
> >+        dsdt_entry_size = 8;
> >+    }
> >     acpi_fetch_table(data->qts, &table.aml, &table.aml_len,
> >-                     fadt_aml + 40 /* DSDT */, 4, "DSDT", true);
> >+                     fadt_aml + dsdt_offset, dsdt_entry_size, "DSDT", true);
> >     g_array_append_val(data->tables, table);
> > 
> >     memset(fadt_aml + 36, 0, 4); /* sanitize FIRMWARE_CTRL ptr */
> >-- 
> >2.7.4
>