| Message ID | 61cbf56ef1c5dd9dbe6bd6625f6c8d2a82c5697f.1717527933.git.mst@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PULL,01/46] vhost: dirty log should be per backend type | expand |
On 6/4/24 14:08, Michael S. Tsirkin wrote: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Given this is a new configuration, there are affects on APIC, CEDT > and DSDT, but the key elements are in SRAT (plus related data in > HMAT). The configuration has node to exercise many different combinations. > > 0) CPUs + Memory > 1) GI only > 2) GP only > 3) CPUS only > 4) Memory only > 5) CPUs + HP memory > > GI node, GP Node, Memory only node, hotplug memory > only node, latency and bandwidth such that in Linux Access0 > (any initiator) and Access1 (CPU initiators only) given different > answers. Following cropped to remove details of each entry. This fails testing: https://gitlab.com/qemu-project/qemu/-/jobs/7021105504 acpi-test: Warning! SRAT binary file mismatch. Actual [aml:/tmp/aml-GHR6O2], Expected [aml:tests/data/acpi/q35/SRAT.acpihmat-generic-x]. See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files. to see ASL diff between mismatched files install IASL, rebuild QEMU from scratch and re-run tests with V=1 environment variable set** ERROR:../alt/tests/qtest/bios-tables-test.c:550:test_acpi_asl: assertion failed: (all_tables_match) Bail out! ERROR:../alt/tests/qtest/bios-tables-test.c:550:test_acpi_asl: assertion failed: (all_tables_match) Aborted (core dumped) r~
On Wed, 5 Jun 2024 07:39:53 -0700 Richard Henderson <richard.henderson@linaro.org> wrote: > On 6/4/24 14:08, Michael S. Tsirkin wrote: > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > Given this is a new configuration, there are affects on APIC, CEDT > > and DSDT, but the key elements are in SRAT (plus related data in > > HMAT). The configuration has node to exercise many different combinations. > > > > 0) CPUs + Memory > > 1) GI only > > 2) GP only > > 3) CPUS only > > 4) Memory only > > 5) CPUs + HP memory > > > > GI node, GP Node, Memory only node, hotplug memory > > only node, latency and bandwidth such that in Linux Access0 > > (any initiator) and Access1 (CPU initiators only) given different > > answers. Following cropped to remove details of each entry. > > > This fails testing: > > https://gitlab.com/qemu-project/qemu/-/jobs/7021105504 > > acpi-test: Warning! SRAT binary file mismatch. Actual [aml:/tmp/aml-GHR6O2], Expected > [aml:tests/data/acpi/q35/SRAT.acpihmat-generic-x]. > See source file tests/qtest/bios-tables-test.c for instructions on how to update expected > files. > to see ASL diff between mismatched files install IASL, rebuild QEMU from scratch and > re-run tests with V=1 environment variable set** > ERROR:../alt/tests/qtest/bios-tables-test.c:550:test_acpi_asl: assertion failed: > (all_tables_match) > Bail out! ERROR:../alt/tests/qtest/bios-tables-test.c:550:test_acpi_asl: assertion failed: > (all_tables_match) > Aborted (core dumped) > s390 and passes on an x86 host, so I guess an endian bug - any chance of a table dump from someone with access to an s390 host? This test covers some stuff that was previously missing tests so may be non trivial to spot. I'll play guess in the meantime. So far I'm not seeing anything that differs from existing ACPI table building code. Jonathan > > r~
On Wed, 5 Jun 2024 16:27:33 +0100 Jonathan Cameron via <qemu-devel@nongnu.org> wrote: > On Wed, 5 Jun 2024 07:39:53 -0700 > Richard Henderson <richard.henderson@linaro.org> wrote: > > > On 6/4/24 14:08, Michael S. Tsirkin wrote: > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > > > Given this is a new configuration, there are affects on APIC, CEDT > > > and DSDT, but the key elements are in SRAT (plus related data in > > > HMAT). The configuration has node to exercise many different combinations. > > > > > > 0) CPUs + Memory > > > 1) GI only > > > 2) GP only > > > 3) CPUS only > > > 4) Memory only > > > 5) CPUs + HP memory > > > > > > GI node, GP Node, Memory only node, hotplug memory > > > only node, latency and bandwidth such that in Linux Access0 > > > (any initiator) and Access1 (CPU initiators only) given different > > > answers. Following cropped to remove details of each entry. > > > > > > This fails testing: > > > > https://gitlab.com/qemu-project/qemu/-/jobs/7021105504 > > > > acpi-test: Warning! SRAT binary file mismatch. Actual [aml:/tmp/aml-GHR6O2], Expected > > [aml:tests/data/acpi/q35/SRAT.acpihmat-generic-x]. > > See source file tests/qtest/bios-tables-test.c for instructions on how to update expected > > files. > > to see ASL diff between mismatched files install IASL, rebuild QEMU from scratch and > > re-run tests with V=1 environment variable set** > > ERROR:../alt/tests/qtest/bios-tables-test.c:550:test_acpi_asl: assertion failed: > > (all_tables_match) > > Bail out! ERROR:../alt/tests/qtest/bios-tables-test.c:550:test_acpi_asl: assertion failed: > > (all_tables_match) > > Aborted (core dumped) > > > > s390 and passes on an x86 host, so I guess an endian bug - any chance of a table dump > from someone with access to an s390 host? > > This test covers some stuff that was previously missing tests > so may be non trivial to spot. > > I'll play guess in the meantime. So far I'm not seeing anything that differs > from existing ACPI table building code. Ok. I think it a bug earlier in this patch set. A table dump would confirm. One item that looks dodgy is the filling in of the ACPI HID. It's messy in the ACPI spec as it's defined as both an integer and a string. Need to take a 4 character string and get it into hid of struct { uint64_t hid; uint32_t uid; } which id done with an 8 byte memcpy. that is then written to SRAT wtih build_append_int_noprefix(table_data, handle->hid, 8); So I need to byte swap it if host doesn't match guest endian. I'll put together a patch, but perhaps best plan is if Michael drops generic ports and we go around again :( At least I don't have much backed up behind this one. Jonathan > > Jonathan > > > > > > r~ > >
On 6/5/24 10:27, Jonathan Cameron wrote: >> This fails testing: >> >> https://gitlab.com/qemu-project/qemu/-/jobs/7021105504 >> >> acpi-test: Warning! SRAT binary file mismatch. Actual [aml:/tmp/aml-GHR6O2], Expected >> [aml:tests/data/acpi/q35/SRAT.acpihmat-generic-x]. >> See source file tests/qtest/bios-tables-test.c for instructions on how to update expected >> files. >> to see ASL diff between mismatched files install IASL, rebuild QEMU from scratch and >> re-run tests with V=1 environment variable set** >> ERROR:../alt/tests/qtest/bios-tables-test.c:550:test_acpi_asl: assertion failed: >> (all_tables_match) >> Bail out! ERROR:../alt/tests/qtest/bios-tables-test.c:550:test_acpi_asl: assertion failed: >> (all_tables_match) >> Aborted (core dumped) >> > > s390 and passes on an x86 host, so I guess an endian bug - any chance of a table dump > from someone with access to an s390 host? Sure. By what incantation do I produce a dump? r~
On Wed, 5 Jun 2024 09:01:37 -0700 Richard Henderson <richard.henderson@linaro.org> wrote: > On 6/5/24 10:27, Jonathan Cameron wrote: > >> This fails testing: > >> > >> https://gitlab.com/qemu-project/qemu/-/jobs/7021105504 > >> > >> acpi-test: Warning! SRAT binary file mismatch. Actual [aml:/tmp/aml-GHR6O2], Expected > >> [aml:tests/data/acpi/q35/SRAT.acpihmat-generic-x]. > >> See source file tests/qtest/bios-tables-test.c for instructions on how to update expected > >> files. > >> to see ASL diff between mismatched files install IASL, rebuild QEMU from scratch and > >> re-run tests with V=1 environment variable set** > >> ERROR:../alt/tests/qtest/bios-tables-test.c:550:test_acpi_asl: assertion failed: > >> (all_tables_match) > >> Bail out! ERROR:../alt/tests/qtest/bios-tables-test.c:550:test_acpi_asl: assertion failed: > >> (all_tables_match) > >> Aborted (core dumped) > >> > > > > s390 and passes on an x86 host, so I guess an endian bug - any chance of a table dump > > from someone with access to an s390 host? > > Sure. By what incantation do I produce a dump? If you still have the /mnt/aml-GHR602 above then either upload that somewhere or iasl -d /mnt/aml-GHR602 should generate you a suitable text file. However generic ports are fairly recent so you may need a newer iasl from acpica-tools to decode. It will moan if it doesn't understand the content. Jonathan > > > r~
On Wed, 5 Jun 2024 17:08:45 +0100 Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > On Wed, 5 Jun 2024 09:01:37 -0700 > Richard Henderson <richard.henderson@linaro.org> wrote: > > > On 6/5/24 10:27, Jonathan Cameron wrote: > > >> This fails testing: > > >> > > >> https://gitlab.com/qemu-project/qemu/-/jobs/7021105504 > > >> > > >> acpi-test: Warning! SRAT binary file mismatch. Actual [aml:/tmp/aml-GHR6O2], Expected > > >> [aml:tests/data/acpi/q35/SRAT.acpihmat-generic-x]. > > >> See source file tests/qtest/bios-tables-test.c for instructions on how to update expected > > >> files. > > >> to see ASL diff between mismatched files install IASL, rebuild QEMU from scratch and > > >> re-run tests with V=1 environment variable set** > > >> ERROR:../alt/tests/qtest/bios-tables-test.c:550:test_acpi_asl: assertion failed: > > >> (all_tables_match) > > >> Bail out! ERROR:../alt/tests/qtest/bios-tables-test.c:550:test_acpi_asl: assertion failed: > > >> (all_tables_match) > > >> Aborted (core dumped) > > >> > > > > > > s390 and passes on an x86 host, so I guess an endian bug - any chance of a table dump > > > from someone with access to an s390 host? > > > > Sure. By what incantation do I produce a dump? > > If you still have the /mnt/aml-GHR602 above then either upload that somewhere or > iasl -d /mnt/aml-GHR602 > should generate you a suitable text file. However generic ports are fairly recent > so you may need a newer iasl from acpica-tools to decode. > It will moan if it doesn't understand the content. > make check-qtest-x86_64 is how I get the test to run in the first place. Alternatively, this 'might' be sufficient if my guess for the problem is correct. Thanks! From 956df037f024783f19b6b00e5e280484380227a0 Mon Sep 17 00:00:00 2001 From: Jonathan Cameron <Jonathan.Cameron@huawei.com> Date: Wed, 5 Jun 2024 17:01:36 +0100 Subject: [PATCH] hw/acpi: Fix big endian host creation of Generic Port Affinity Structures Treating the HID as an integer caused it to get bit reversed on big endian hosts running little endian guests. Treat it as a character array instead. Fixes hw/acpi: Generic Port Affinity Structure Support Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> --- Maybe this is the only problem? I don't have a setup to test so any help would be appreciated. --- include/hw/acpi/acpi_generic_initiator.h | 2 +- hw/acpi/acpi_generic_initiator.c | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/include/hw/acpi/acpi_generic_initiator.h b/include/hw/acpi/acpi_generic_initiator.h index 1a899af30f..5baefda33a 100644 --- a/include/hw/acpi/acpi_generic_initiator.h +++ b/include/hw/acpi/acpi_generic_initiator.h @@ -61,7 +61,7 @@ typedef struct PCIDeviceHandle { uint16_t bdf; }; struct { - uint64_t hid; + char hid[8]; uint32_t uid; }; }; diff --git a/hw/acpi/acpi_generic_initiator.c b/hw/acpi/acpi_generic_initiator.c index 78b80dcf08..c2fb5ab2ef 100644 --- a/hw/acpi/acpi_generic_initiator.c +++ b/hw/acpi/acpi_generic_initiator.c @@ -150,8 +150,12 @@ build_srat_generic_node_affinity(GArray *table_data, int node, build_append_int_noprefix(table_data, handle->bdf, 2); build_append_int_noprefix(table_data, 0, 12); } else { + int i; + /* Device Handle - ACPI */ - build_append_int_noprefix(table_data, handle->hid, 8); + for (i = 0; i < sizeof(handle->hid); i++) { + build_append_int_noprefix(table_data, handle->hid[i], 1); + } build_append_int_noprefix(table_data, handle->uid, 4); build_append_int_noprefix(table_data, 0, 4); }
On 6/5/24 11:11, Jonathan Cameron wrote: >>> Sure. By what incantation do I produce a dump? >> >> If you still have the /mnt/aml-GHR602 above then either upload that somewhere or >> iasl -d /mnt/aml-GHR602 >> should generate you a suitable text file. However generic ports are fairly recent >> so you may need a newer iasl from acpica-tools to decode. >> It will moan if it doesn't understand the content. >> > make check-qtest-x86_64 is how I get the test to run in the first place. Ah, I see, the file is not cleaned up on abort. For the record, I attach the dump. > Alternatively, this 'might' be sufficient if my guess for the problem > is correct. Thanks! > > From 956df037f024783f19b6b00e5e280484380227a0 Mon Sep 17 00:00:00 2001 > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Date: Wed, 5 Jun 2024 17:01:36 +0100 > Subject: [PATCH] hw/acpi: Fix big endian host creation of Generic Port > Affinity Structures > > Treating the HID as an integer caused it to get bit reversed > on big endian hosts running little endian guests. Treat it > as a character array instead. > > Fixes hw/acpi: Generic Port Affinity Structure Support > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > --- > Maybe this is the only problem? I don't have a setup to test > so any help would be appreciated. > --- > include/hw/acpi/acpi_generic_initiator.h | 2 +- > hw/acpi/acpi_generic_initiator.c | 6 +++++- > 2 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/include/hw/acpi/acpi_generic_initiator.h b/include/hw/acpi/acpi_generic_initiator.h > index 1a899af30f..5baefda33a 100644 > --- a/include/hw/acpi/acpi_generic_initiator.h > +++ b/include/hw/acpi/acpi_generic_initiator.h > @@ -61,7 +61,7 @@ typedef struct PCIDeviceHandle { > uint16_t bdf; > }; > struct { > - uint64_t hid; > + char hid[8]; > uint32_t uid; > }; > }; > diff --git a/hw/acpi/acpi_generic_initiator.c b/hw/acpi/acpi_generic_initiator.c > index 78b80dcf08..c2fb5ab2ef 100644 > --- a/hw/acpi/acpi_generic_initiator.c > +++ b/hw/acpi/acpi_generic_initiator.c > @@ -150,8 +150,12 @@ build_srat_generic_node_affinity(GArray *table_data, int node, > build_append_int_noprefix(table_data, handle->bdf, 2); > build_append_int_noprefix(table_data, 0, 12); > } else { > + int i; > + > /* Device Handle - ACPI */ > - build_append_int_noprefix(table_data, handle->hid, 8); > + for (i = 0; i < sizeof(handle->hid); i++) { > + build_append_int_noprefix(table_data, handle->hid[i], 1); > + } > build_append_int_noprefix(table_data, handle->uid, 4); > build_append_int_noprefix(table_data, 0, 4); > } Yes, this fixes the abort. Merge the declaration of i into the for loop? r~
On Wed, 5 Jun 2024 09:54:16 -0700 Richard Henderson <richard.henderson@linaro.org> wrote: > On 6/5/24 11:11, Jonathan Cameron wrote: > >>> Sure. By what incantation do I produce a dump? > >> > >> If you still have the /mnt/aml-GHR602 above then either upload that somewhere or > >> iasl -d /mnt/aml-GHR602 > >> should generate you a suitable text file. However generic ports are fairly recent > >> so you may need a newer iasl from acpica-tools to decode. > >> It will moan if it doesn't understand the content. > >> > > make check-qtest-x86_64 is how I get the test to run in the first place. > > Ah, I see, the file is not cleaned up on abort. Yes. End up with a lot of those after a while! > For the record, I attach the dump. Thanks! > > > Alternatively, this 'might' be sufficient if my guess for the problem > > is correct. Thanks! > > > > From 956df037f024783f19b6b00e5e280484380227a0 Mon Sep 17 00:00:00 2001 > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Date: Wed, 5 Jun 2024 17:01:36 +0100 > > Subject: [PATCH] hw/acpi: Fix big endian host creation of Generic Port > > Affinity Structures > > > > Treating the HID as an integer caused it to get bit reversed > > on big endian hosts running little endian guests. Treat it > > as a character array instead. > > > > Fixes hw/acpi: Generic Port Affinity Structure Support > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > --- > > Maybe this is the only problem? I don't have a setup to test > > so any help would be appreciated. > > --- > > include/hw/acpi/acpi_generic_initiator.h | 2 +- > > hw/acpi/acpi_generic_initiator.c | 6 +++++- > > 2 files changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/include/hw/acpi/acpi_generic_initiator.h b/include/hw/acpi/acpi_generic_initiator.h > > index 1a899af30f..5baefda33a 100644 > > --- a/include/hw/acpi/acpi_generic_initiator.h > > +++ b/include/hw/acpi/acpi_generic_initiator.h > > @@ -61,7 +61,7 @@ typedef struct PCIDeviceHandle { > > uint16_t bdf; > > }; > > struct { > > - uint64_t hid; > > + char hid[8]; > > uint32_t uid; > > }; > > }; > > diff --git a/hw/acpi/acpi_generic_initiator.c b/hw/acpi/acpi_generic_initiator.c > > index 78b80dcf08..c2fb5ab2ef 100644 > > --- a/hw/acpi/acpi_generic_initiator.c > > +++ b/hw/acpi/acpi_generic_initiator.c > > @@ -150,8 +150,12 @@ build_srat_generic_node_affinity(GArray *table_data, int node, > > build_append_int_noprefix(table_data, handle->bdf, 2); > > build_append_int_noprefix(table_data, 0, 12); > > } else { > > + int i; > > + > > /* Device Handle - ACPI */ > > - build_append_int_noprefix(table_data, handle->hid, 8); > > + for (i = 0; i < sizeof(handle->hid); i++) { > > + build_append_int_noprefix(table_data, handle->hid[i], 1); > > + } > > build_append_int_noprefix(table_data, handle->uid, 4); > > build_append_int_noprefix(table_data, 0, 4); > > } > > Yes, this fixes the abort. > Merge the declaration of i into the for loop? sure. Thanks for testing. Seems it's time I tried to get an emulated s390 setup running if that's practical as two of these in the last year :( Jonathan > > > r~
diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h index a5aa801c99..dfb8523c8b 100644 --- a/tests/qtest/bios-tables-test-allowed-diff.h +++ b/tests/qtest/bios-tables-test-allowed-diff.h @@ -1,6 +1 @@ /* List of comma-separated changed AML files to ignore */ -"tests/data/acpi/q35/APIC.acpihmat-generic-x", -"tests/data/acpi/q35/CEDT.acpihmat-generic-x", -"tests/data/acpi/q35/DSDT.acpihmat-generic-x", -"tests/data/acpi/q35/HMAT.acpihmat-generic-x", -"tests/data/acpi/q35/SRAT.acpihmat-generic-x",