diff mbox series

[v4,05/15] tests: acpi: fetch X_DSDT if pointer to DSDT is 0

Message ID 1556808723-226478-6-git-send-email-imammedo@redhat.com (mailing list archive)
State New, archived
Headers show
Series tests: acpi: add UEFI (ARM) testing support | expand

Commit Message

Igor Mammedov May 2, 2019, 2:51 p.m. UTC 
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 <imammedo@redhat.com>
---
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é <philmd@redhat.com>)
---
 tests/bios-tables-test.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Comments

Wei Yang May 5, 2019, 1:27 a.m. UTC | #1 
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 <imammedo@redhat.com>
>---
>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é <philmd@redhat.com>)
>---
> 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.

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
Igor Mammedov May 7, 2019, 10:04 a.m. UTC | #2 
On Sun, 5 May 2019 09:27:45 +0800
Wei Yang <richardw.yang@linux.intel.com> 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 <imammedo@redhat.com>
> >---
> >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é <philmd@redhat.com>)
> >---
> > 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
>
Wei Yang May 8, 2019, 5:51 a.m. UTC | #3 
On Tue, May 07, 2019 at 12:04:08PM +0200, Igor Mammedov wrote:
>On Sun, 5 May 2019 09:27:45 +0800
>Wei Yang <richardw.yang@linux.intel.com> 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 <imammedo@redhat.com>
>> >---
>> >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é <philmd@redhat.com>)
>> >---
>> > 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.
>

That's fine.

Reviewed-by: Wei Yang <richardw.yang@linux.intel.com>
diff mbox series

Patch

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 */;
+        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 */