* [Qemu-devel] [PATCH] acpi unit-test: resolved iasl crash @ 2013-12-29 12:32 Marcel Apfelbaum 2014-01-06 11:43 ` Michael S. Tsirkin 0 siblings, 1 reply; 5+ messages in thread From: Marcel Apfelbaum @ 2013-12-29 12:32 UTC (permalink / raw) To: qemu-devel; +Cc: mst It seems that iasl has an issue when disassembles some ACPI tables using the command line: iasl -e DSDT -e SSDT -d HPET I opened a bug on iasl project: https://github.com/acpica/acpica/issues/20 Modified the iasl command line to "iasl -d HPET" until the problem is solved. The command line remained the same for DSDT and SSDT tables. Reported-by:Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> --- tests/acpi-test.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/tests/acpi-test.c b/tests/acpi-test.c index 2d32b69..26d2e26 100644 --- a/tests/acpi-test.c +++ b/tests/acpi-test.c @@ -403,7 +403,7 @@ static void dump_aml_files(test_data *data, bool rebuild) } } -static void load_asl(GArray *sdts, AcpiSdtTable *sdt) +static void load_asl(GArray *sdts, AcpiSdtTable *sdt, bool referenceDsdt) { AcpiSdtTable *temp; GError *error = NULL; @@ -419,9 +419,11 @@ static void load_asl(GArray *sdts, AcpiSdtTable *sdt) /* build command line */ g_string_append_printf(command_line, " -p %s ", sdt->asl_file); - for (i = 0; i < 2; ++i) { /* reference DSDT and SSDT */ - temp = &g_array_index(sdts, AcpiSdtTable, i); - g_string_append_printf(command_line, "-e %s ", temp->aml_file); + if (referenceDsdt) { + for (i = 0; i < 2; ++i) { /* reference DSDT and SSDT */ + temp = &g_array_index(sdts, AcpiSdtTable, i); + g_string_append_printf(command_line, "-e %s ", temp->aml_file); + } } g_string_append_printf(command_line, "-d %s", sdt->aml_file); @@ -510,14 +512,14 @@ static void test_acpi_asl(test_data *data) dump_aml_files(data, false); for (i = 0; i < data->ssdt_tables->len; ++i) { GString *asl, *exp_asl; - + bool referenceDsdt = (i < 2); /* only for DSDT and SSDT */ sdt = &g_array_index(data->ssdt_tables, AcpiSdtTable, i); exp_sdt = &g_array_index(exp_data.ssdt_tables, AcpiSdtTable, i); - load_asl(data->ssdt_tables, sdt); + load_asl(data->ssdt_tables, sdt, referenceDsdt); asl = normalize_asl(sdt->asl); - load_asl(exp_data.ssdt_tables, exp_sdt); + load_asl(exp_data.ssdt_tables, exp_sdt, referenceDsdt); exp_asl = normalize_asl(exp_sdt->asl); g_assert(!g_strcmp0(asl->str, exp_asl->str)); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] acpi unit-test: resolved iasl crash 2013-12-29 12:32 [Qemu-devel] [PATCH] acpi unit-test: resolved iasl crash Marcel Apfelbaum @ 2014-01-06 11:43 ` Michael S. Tsirkin 2014-01-06 13:21 ` Marcel Apfelbaum 0 siblings, 1 reply; 5+ messages in thread From: Michael S. Tsirkin @ 2014-01-06 11:43 UTC (permalink / raw) To: Marcel Apfelbaum; +Cc: qemu-devel On Sun, Dec 29, 2013 at 02:32:50PM +0200, Marcel Apfelbaum wrote: > It seems that iasl has an issue when disassembles > some ACPI tables using the command line: > iasl -e DSDT -e SSDT -d HPET > I opened a bug on iasl project: > https://github.com/acpica/acpica/issues/20 > > Modified the iasl command line to "iasl -d HPET" > until the problem is solved. The command line > remained the same for DSDT and SSDT tables. > > Reported-by:Michael S. Tsirkin <mst@redhat.com> space needed after : > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > --- > tests/acpi-test.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/tests/acpi-test.c b/tests/acpi-test.c > index 2d32b69..26d2e26 100644 > --- a/tests/acpi-test.c > +++ b/tests/acpi-test.c > @@ -403,7 +403,7 @@ static void dump_aml_files(test_data *data, bool rebuild) > } > } > > -static void load_asl(GArray *sdts, AcpiSdtTable *sdt) > +static void load_asl(GArray *sdts, AcpiSdtTable *sdt, bool referenceDsdt) > { > AcpiSdtTable *temp; > GError *error = NULL; > @@ -419,9 +419,11 @@ static void load_asl(GArray *sdts, AcpiSdtTable *sdt) > > /* build command line */ > g_string_append_printf(command_line, " -p %s ", sdt->asl_file); > - for (i = 0; i < 2; ++i) { /* reference DSDT and SSDT */ > - temp = &g_array_index(sdts, AcpiSdtTable, i); > - g_string_append_printf(command_line, "-e %s ", temp->aml_file); > + if (referenceDsdt) { > + for (i = 0; i < 2; ++i) { /* reference DSDT and SSDT */ > + temp = &g_array_index(sdts, AcpiSdtTable, i); > + g_string_append_printf(command_line, "-e %s ", temp->aml_file); > + } > } > g_string_append_printf(command_line, "-d %s", sdt->aml_file); > > @@ -510,14 +512,14 @@ static void test_acpi_asl(test_data *data) > dump_aml_files(data, false); > for (i = 0; i < data->ssdt_tables->len; ++i) { > GString *asl, *exp_asl; > - > + bool referenceDsdt = (i < 2); /* only for DSDT and SSDT */ Please don't use camelCase for variables. Can we check the table signature instead of looking at the index? > sdt = &g_array_index(data->ssdt_tables, AcpiSdtTable, i); > exp_sdt = &g_array_index(exp_data.ssdt_tables, AcpiSdtTable, i); > > - load_asl(data->ssdt_tables, sdt); > + load_asl(data->ssdt_tables, sdt, referenceDsdt); > asl = normalize_asl(sdt->asl); > > - load_asl(exp_data.ssdt_tables, exp_sdt); > + load_asl(exp_data.ssdt_tables, exp_sdt, referenceDsdt); > exp_asl = normalize_asl(exp_sdt->asl); > > g_assert(!g_strcmp0(asl->str, exp_asl->str)); > -- > 1.8.3.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] acpi unit-test: resolved iasl crash 2014-01-06 11:43 ` Michael S. Tsirkin @ 2014-01-06 13:21 ` Marcel Apfelbaum 2014-01-06 14:35 ` Michael S. Tsirkin 0 siblings, 1 reply; 5+ messages in thread From: Marcel Apfelbaum @ 2014-01-06 13:21 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel On Mon, 2014-01-06 at 13:43 +0200, Michael S. Tsirkin wrote: > On Sun, Dec 29, 2013 at 02:32:50PM +0200, Marcel Apfelbaum wrote: > > It seems that iasl has an issue when disassembles > > some ACPI tables using the command line: > > iasl -e DSDT -e SSDT -d HPET > > I opened a bug on iasl project: > > https://github.com/acpica/acpica/issues/20 > > > > Modified the iasl command line to "iasl -d HPET" > > until the problem is solved. The command line > > remained the same for DSDT and SSDT tables. > > > > Reported-by:Michael S. Tsirkin <mst@redhat.com> > > space needed after : Sure, missed that > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > --- > > tests/acpi-test.c | 16 +++++++++------- > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > diff --git a/tests/acpi-test.c b/tests/acpi-test.c > > index 2d32b69..26d2e26 100644 > > --- a/tests/acpi-test.c > > +++ b/tests/acpi-test.c > > @@ -403,7 +403,7 @@ static void dump_aml_files(test_data *data, bool rebuild) > > } > > } > > > > -static void load_asl(GArray *sdts, AcpiSdtTable *sdt) > > +static void load_asl(GArray *sdts, AcpiSdtTable *sdt, bool referenceDsdt) > > { > > AcpiSdtTable *temp; > > GError *error = NULL; > > @@ -419,9 +419,11 @@ static void load_asl(GArray *sdts, AcpiSdtTable *sdt) > > > > /* build command line */ > > g_string_append_printf(command_line, " -p %s ", sdt->asl_file); > > - for (i = 0; i < 2; ++i) { /* reference DSDT and SSDT */ > > - temp = &g_array_index(sdts, AcpiSdtTable, i); > > - g_string_append_printf(command_line, "-e %s ", temp->aml_file); > > + if (referenceDsdt) { > > + for (i = 0; i < 2; ++i) { /* reference DSDT and SSDT */ > > + temp = &g_array_index(sdts, AcpiSdtTable, i); > > + g_string_append_printf(command_line, "-e %s ", temp->aml_file); > > + } > > } > > g_string_append_printf(command_line, "-d %s", sdt->aml_file); > > > > @@ -510,14 +512,14 @@ static void test_acpi_asl(test_data *data) > > dump_aml_files(data, false); > > for (i = 0; i < data->ssdt_tables->len; ++i) { > > GString *asl, *exp_asl; > > - > > + bool referenceDsdt = (i < 2); /* only for DSDT and SSDT */ > > > Please don't use camelCase for variables. Sure, no idea why I did that in the first place... > Can we check the table signature instead of looking at the index? It is possible, but not really necessary because: 1. The array of expected tables it is created as a shadow of the original tables. (We load the expected tables in the exact order as the originals) 2. The loading is done based on table signatures. So going over the index in *this* case it means going over the signatures. Thanks for the review, Marcel > > > sdt = &g_array_index(data->ssdt_tables, AcpiSdtTable, i); > > exp_sdt = &g_array_index(exp_data.ssdt_tables, AcpiSdtTable, i); > > > > - load_asl(data->ssdt_tables, sdt); > > + load_asl(data->ssdt_tables, sdt, referenceDsdt); > > asl = normalize_asl(sdt->asl); > > > > - load_asl(exp_data.ssdt_tables, exp_sdt); > > + load_asl(exp_data.ssdt_tables, exp_sdt, referenceDsdt); > > exp_asl = normalize_asl(exp_sdt->asl); > > > > g_assert(!g_strcmp0(asl->str, exp_asl->str)); > > -- > > 1.8.3.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] acpi unit-test: resolved iasl crash 2014-01-06 13:21 ` Marcel Apfelbaum @ 2014-01-06 14:35 ` Michael S. Tsirkin 2014-01-06 14:58 ` Marcel Apfelbaum 0 siblings, 1 reply; 5+ messages in thread From: Michael S. Tsirkin @ 2014-01-06 14:35 UTC (permalink / raw) To: Marcel Apfelbaum; +Cc: qemu-devel On Mon, Jan 06, 2014 at 03:21:52PM +0200, Marcel Apfelbaum wrote: > On Mon, 2014-01-06 at 13:43 +0200, Michael S. Tsirkin wrote: > > On Sun, Dec 29, 2013 at 02:32:50PM +0200, Marcel Apfelbaum wrote: > > > It seems that iasl has an issue when disassembles > > > some ACPI tables using the command line: > > > iasl -e DSDT -e SSDT -d HPET > > > I opened a bug on iasl project: > > > https://github.com/acpica/acpica/issues/20 > > > > > > Modified the iasl command line to "iasl -d HPET" > > > until the problem is solved. The command line > > > remained the same for DSDT and SSDT tables. > > > > > > Reported-by:Michael S. Tsirkin <mst@redhat.com> > > > > space needed after : > Sure, missed that > > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > > --- > > > tests/acpi-test.c | 16 +++++++++------- > > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > > > diff --git a/tests/acpi-test.c b/tests/acpi-test.c > > > index 2d32b69..26d2e26 100644 > > > --- a/tests/acpi-test.c > > > +++ b/tests/acpi-test.c > > > @@ -403,7 +403,7 @@ static void dump_aml_files(test_data *data, bool rebuild) > > > } > > > } > > > > > > -static void load_asl(GArray *sdts, AcpiSdtTable *sdt) > > > +static void load_asl(GArray *sdts, AcpiSdtTable *sdt, bool referenceDsdt) > > > { > > > AcpiSdtTable *temp; > > > GError *error = NULL; > > > @@ -419,9 +419,11 @@ static void load_asl(GArray *sdts, AcpiSdtTable *sdt) > > > > > > /* build command line */ > > > g_string_append_printf(command_line, " -p %s ", sdt->asl_file); > > > - for (i = 0; i < 2; ++i) { /* reference DSDT and SSDT */ > > > - temp = &g_array_index(sdts, AcpiSdtTable, i); > > > - g_string_append_printf(command_line, "-e %s ", temp->aml_file); > > > + if (referenceDsdt) { > > > + for (i = 0; i < 2; ++i) { /* reference DSDT and SSDT */ > > > + temp = &g_array_index(sdts, AcpiSdtTable, i); > > > + g_string_append_printf(command_line, "-e %s ", temp->aml_file); > > > + } > > > } > > > g_string_append_printf(command_line, "-d %s", sdt->aml_file); > > > > > > @@ -510,14 +512,14 @@ static void test_acpi_asl(test_data *data) > > > dump_aml_files(data, false); > > > for (i = 0; i < data->ssdt_tables->len; ++i) { > > > GString *asl, *exp_asl; > > > - > > > + bool referenceDsdt = (i < 2); /* only for DSDT and SSDT */ > > > > > > Please don't use camelCase for variables. > Sure, no idea why I did that in the first place... > > > Can we check the table signature instead of looking at the index? > It is possible, but not really necessary because: > 1. The array of expected tables it is created as a shadow of the > original tables. (We load the expected tables in the exact order as the originals) > 2. The loading is done based on table signatures. > So going over the index in *this* case it means going over the signatures. Yes but it's really not a good idea to assume that there's a single SSDT, the fact you need a comment shows that it's fragile. How about a simple API like compare_signature(GArray *, const char *signature) We should also probably rename ssdt_tables to something else if it holds other tables besides ssdt. all_tables? > Thanks for the review, > Marcel > > > > > > sdt = &g_array_index(data->ssdt_tables, AcpiSdtTable, i); > > > exp_sdt = &g_array_index(exp_data.ssdt_tables, AcpiSdtTable, i); > > > > > > - load_asl(data->ssdt_tables, sdt); > > > + load_asl(data->ssdt_tables, sdt, referenceDsdt); > > > asl = normalize_asl(sdt->asl); > > > > > > - load_asl(exp_data.ssdt_tables, exp_sdt); > > > + load_asl(exp_data.ssdt_tables, exp_sdt, referenceDsdt); > > > exp_asl = normalize_asl(exp_sdt->asl); > > > > > > g_assert(!g_strcmp0(asl->str, exp_asl->str)); > > > -- > > > 1.8.3.1 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] acpi unit-test: resolved iasl crash 2014-01-06 14:35 ` Michael S. Tsirkin @ 2014-01-06 14:58 ` Marcel Apfelbaum 0 siblings, 0 replies; 5+ messages in thread From: Marcel Apfelbaum @ 2014-01-06 14:58 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel On Mon, 2014-01-06 at 16:35 +0200, Michael S. Tsirkin wrote: > On Mon, Jan 06, 2014 at 03:21:52PM +0200, Marcel Apfelbaum wrote: > > On Mon, 2014-01-06 at 13:43 +0200, Michael S. Tsirkin wrote: > > > On Sun, Dec 29, 2013 at 02:32:50PM +0200, Marcel Apfelbaum wrote: > > > > It seems that iasl has an issue when disassembles > > > > some ACPI tables using the command line: > > > > iasl -e DSDT -e SSDT -d HPET > > > > I opened a bug on iasl project: > > > > https://github.com/acpica/acpica/issues/20 > > > > > > > > Modified the iasl command line to "iasl -d HPET" > > > > until the problem is solved. The command line > > > > remained the same for DSDT and SSDT tables. > > > > > > > > Reported-by:Michael S. Tsirkin <mst@redhat.com> > > > > > > space needed after : > > Sure, missed that > > > > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > > > --- > > > > tests/acpi-test.c | 16 +++++++++------- > > > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/tests/acpi-test.c b/tests/acpi-test.c > > > > index 2d32b69..26d2e26 100644 > > > > --- a/tests/acpi-test.c > > > > +++ b/tests/acpi-test.c > > > > @@ -403,7 +403,7 @@ static void dump_aml_files(test_data *data, bool rebuild) > > > > } > > > > } > > > > > > > > -static void load_asl(GArray *sdts, AcpiSdtTable *sdt) > > > > +static void load_asl(GArray *sdts, AcpiSdtTable *sdt, bool referenceDsdt) > > > > { > > > > AcpiSdtTable *temp; > > > > GError *error = NULL; > > > > @@ -419,9 +419,11 @@ static void load_asl(GArray *sdts, AcpiSdtTable *sdt) > > > > > > > > /* build command line */ > > > > g_string_append_printf(command_line, " -p %s ", sdt->asl_file); > > > > - for (i = 0; i < 2; ++i) { /* reference DSDT and SSDT */ > > > > - temp = &g_array_index(sdts, AcpiSdtTable, i); > > > > - g_string_append_printf(command_line, "-e %s ", temp->aml_file); > > > > + if (referenceDsdt) { > > > > + for (i = 0; i < 2; ++i) { /* reference DSDT and SSDT */ > > > > + temp = &g_array_index(sdts, AcpiSdtTable, i); > > > > + g_string_append_printf(command_line, "-e %s ", temp->aml_file); > > > > + } > > > > } > > > > g_string_append_printf(command_line, "-d %s", sdt->aml_file); > > > > > > > > @@ -510,14 +512,14 @@ static void test_acpi_asl(test_data *data) > > > > dump_aml_files(data, false); > > > > for (i = 0; i < data->ssdt_tables->len; ++i) { > > > > GString *asl, *exp_asl; > > > > - > > > > + bool referenceDsdt = (i < 2); /* only for DSDT and SSDT */ > > > > > > > > > Please don't use camelCase for variables. > > Sure, no idea why I did that in the first place... > > > > > Can we check the table signature instead of looking at the index? > > It is possible, but not really necessary because: > > 1. The array of expected tables it is created as a shadow of the > > original tables. (We load the expected tables in the exact order as the originals) > > 2. The loading is done based on table signatures. > > So going over the index in *this* case it means going over the signatures. > > Yes but it's really not a good idea to assume that there's > a single SSDT, the fact you need a comment shows that > it's fragile. > How about a simple API like > compare_signature(GArray *, const char *signature) > > > We should also probably rename ssdt_tables to something else > if it holds other tables besides ssdt. all_tables? OK, I'll follow this path Thanks, Marcel > > > Thanks for the review, > > Marcel > > > > > > > > > sdt = &g_array_index(data->ssdt_tables, AcpiSdtTable, i); > > > > exp_sdt = &g_array_index(exp_data.ssdt_tables, AcpiSdtTable, i); > > > > > > > > - load_asl(data->ssdt_tables, sdt); > > > > + load_asl(data->ssdt_tables, sdt, referenceDsdt); > > > > asl = normalize_asl(sdt->asl); > > > > > > > > - load_asl(exp_data.ssdt_tables, exp_sdt); > > > > + load_asl(exp_data.ssdt_tables, exp_sdt, referenceDsdt); > > > > exp_asl = normalize_asl(exp_sdt->asl); > > > > > > > > g_assert(!g_strcmp0(asl->str, exp_asl->str)); > > > > -- > > > > 1.8.3.1 > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-01-06 14:58 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-12-29 12:32 [Qemu-devel] [PATCH] acpi unit-test: resolved iasl crash Marcel Apfelbaum 2014-01-06 11:43 ` Michael S. Tsirkin 2014-01-06 13:21 ` Marcel Apfelbaum 2014-01-06 14:35 ` Michael S. Tsirkin 2014-01-06 14:58 ` Marcel Apfelbaum
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).