| Message ID | 20210625091818.1047980-1-imammedo@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | acpi: refactor error prone build_header() and packed structures usage in ACPI tables | expand |
On Fri, Jun 25, 2021 at 05:17:24AM -0400, Igor Mammedov wrote: > Highlights: > * drop pointer arithmetic in ACPI tables code > * use endian agnostic API > * simplifies review of tables. /in most cases just line by line comparision with spec/ A hue amount of work, thank you! To make it easier to merge, how about splitting it up a bit? E.g. I think first 10-11 patches make sense on their own, right? > Series replaces build_header() with acpi_init_table()/acpi_table_composed() > API that hides pointer/offset arithmetic from user, to prevent > errors caused by it [1]. > While doing above, it was necessary to split table header from > packed structure that was describing the table, which is > counter-productive since it still leaves packed structure drawbacks. > So that sort of forced me to rewrite tables that were composed with > help of packed structures to preferred build_append_int_noprefix() API. > In cases where switch to build_append_int_noprefix() was small, it was > combined with acpi_init_table()/acpi_table_composed() patch. > Conversion reduced code size quite a bit despite me adding doc comments > for every table row. > > Series also includes optional qtest patches that add missing acpi > tests for tables that I'm touching to verify conversion changes. > That includes an alternative build time based impl. of > qtest_has_accel() API. So if we start bike-shedding this qtest_has_accel() > we can safely drop all tests included, till the time discussion settles > and some form of a qtest_has_accel() is merged, at which point I'd respin > depended tests. > > 1) commits > bb9feea43179 x86: acpi: use offset instead of pointer when using build_header() > 4d027afeb3a9 Virt: ACPI: fix qemu assert due to re-assigned table data address > > Link to repo: > https://gitlab.com/imammedo/qemu/-/commits/acpi_build_header_refactoring > > CC: mst@redhat.com > > Igor Mammedov (53): > tests: acpi: dump table with failed checksum > tests: qtest: add qtest_has_accel() to check if tested binary supports > accelerator > tests: acpi: whitelist expected tables for acpi/q35/numamem testcase > tests: acpi: q35: test for x2APIC entries in SRAT > tests: acpi: update expected tables blobs > tests: acpi: whitelist new expected table > tests/data/acpi/q35/DMAR.dmar > tests: acpi: add testcase for intel_iommu (DMAR table) > tests: acpi: add expected blob for DMAR table > tests: acpi: whitelist expected blobs for new acpi/q35/ivrs testcase > tests: acpi: add testcase for amd-iommu (IVRS table) > tests: acpi: update expected blobs > tests: acpi: arm/virt: drop redundant test_acpi_one() in > test_acpi_virt_tcg() > tests: acpi: whitelist expected tables for acpi/virt/iort testcase > tests: acpi: arm/virt: use kvm to test IORT table > tests: acpi: add expected IORT table blob > tests: arm-cpu-features: use qtest_has_kvm() API > tests: migration-test: use qtest_has_accel() API > tests: bios-tables-test: use qtest_has_accel() API to register TCG > only tests > acpi: add helper routines to initialize ACPI tables > acpi: build_rsdt: use acpi_init_table()/acpi_table_composed() instead > of build_header() > acpi: build_xsdt: use acpi_init_table()/acpi_table_composed() instead > of build_header() > acpi: build_slit: use acpi_init_table()/acpi_table_composed() instead > of build_header() > acpi: build_fadt: use acpi_init_table()/acpi_table_composed() instead > of build_header() > acpi: build_tpm2: use acpi_init_table()/acpi_table_composed() instead > of build_header() > acpi: acpi_build_hest: use acpi_init_table()/acpi_table_composed() > instead of build_header() > acpi: build_mcfg: use acpi_init_table()/acpi_table_composed() instead > of build_header() > acpi: build_hmat: use acpi_init_table()/acpi_table_composed() instead > of build_header() > acpi: nvdimm_build_nfit: use acpi_init_table()/acpi_table_composed() > instead of build_header() > acpi: nvdimm_build_ssdt: use acpi_init_table()/acpi_table_composed() > instead of build_header() > acpi: vmgenid_build_acpi: use acpi_init_table()/acpi_table_composed() > instead of build_header() > acpi: x86: build_dsdt: use acpi_init_table()/acpi_table_composed() > instead of build_header() > acpi: build_hpet: use acpi_init_table()/acpi_table_composed() instead > of build_header() > acpi: build_tpm_tcpa: use acpi_init_table()/acpi_table_composed() > instead of build_header() > acpi: arm/x86: build_srat: use acpi_init_table()/acpi_table_composed() > instead of build_header() > acpi: use build_append_int_noprefix() API to compose SRAT table > acpi: build_dmar_q35: use acpi_init_table()/acpi_table_composed() > instead of build_header() > acpi: build_waet: use acpi_init_table()/acpi_table_composed() instead > of build_header() > acpi: build_amd_iommu: use acpi_init_table()/acpi_table_composed() > instead of build_header() > acpi: madt: arm/x86: use acpi_init_table()/acpi_table_composed() > instead of build_header() > acpi: x86: remove dead code > acpi: x86: set enabled when composing _MAT entries > acpi: x86: madt: use build_append_int_noprefix() API to compose MADT > table > acpi: arm/virt: madt: use build_append_int_noprefix() API to compose > MADT table > acpi: build_dsdt_microvm: use acpi_init_table()/acpi_table_composed() > instead of build_header() > acpi: arm: virt: build_dsdt: use > acpi_init_table()/acpi_table_composed() instead of build_header() > acpi: arm: virt: build_iort: use > acpi_init_table()/acpi_table_composed() instead of build_header() > acpi: arm/virt: convert build_iort() to endian agnostic > build_append_FOO() API > acpi: arm/virt: build_spcr: fix invalid cast > acpi: arm/virt: build_spcr: use > acpi_init_table()/acpi_table_composed() instead of build_header() > acpi: arm/virt: build_gtdt: use > acpi_init_table()/acpi_table_composed() instead of build_header() > acpi: build_facs: use build_append_int_noprefix() API to compose table > acpi: remove no longer used build_header() > acpi: AcpiGenericAddress no longer used to map/access fields of MMIO, > drop packed attribute > > include/hw/acpi/acpi-defs.h | 528 +------------------------ > include/hw/acpi/acpi_dev_interface.h | 3 +- > include/hw/acpi/aml-build.h | 21 +- > include/hw/i386/pc.h | 7 +- > tests/qtest/libqos/libqtest.h | 8 + > hw/acpi/acpi-x86-stub.c | 3 +- > hw/acpi/aml-build.c | 188 +++++---- > hw/acpi/cpu.c | 17 +- > hw/acpi/ghes.c | 10 +- > hw/acpi/hmat.c | 14 +- > hw/acpi/nvdimm.c | 64 ++-- > hw/acpi/pci.c | 18 +- > hw/acpi/vmgenid.c | 16 +- > hw/arm/virt-acpi-build.c | 550 +++++++++++++++------------ > hw/i386/acpi-build.c | 285 +++++++------- > hw/i386/acpi-common.c | 158 ++++---- > hw/i386/acpi-microvm.c | 13 +- > meson.build | 6 + > tests/data/acpi/q35/APIC.numamem | Bin 0 -> 2686 bytes > tests/data/acpi/q35/DMAR.dmar | Bin 0 -> 80 bytes > tests/data/acpi/q35/DSDT.ivrs | Bin 0 -> 7877 bytes > tests/data/acpi/q35/DSDT.numamem | Bin 7865 -> 35222 bytes > tests/data/acpi/q35/FACP.numamem | Bin 0 -> 244 bytes > tests/data/acpi/q35/IVRS.ivrs | Bin 0 -> 104 bytes > tests/data/acpi/q35/SRAT.numamem | Bin 224 -> 5080 bytes > tests/data/acpi/virt/IORT.iort | Bin 0 -> 124 bytes > tests/qtest/acpi-utils.c | 14 + > tests/qtest/arm-cpu-features.c | 29 +- > tests/qtest/bios-tables-test.c | 85 ++++- > tests/qtest/libqtest.c | 27 ++ > tests/qtest/migration-test.c | 15 +- > 31 files changed, 873 insertions(+), 1206 deletions(-) > create mode 100644 tests/data/acpi/q35/APIC.numamem > create mode 100644 tests/data/acpi/q35/DMAR.dmar > create mode 100644 tests/data/acpi/q35/DSDT.ivrs > create mode 100644 tests/data/acpi/q35/FACP.numamem > create mode 100644 tests/data/acpi/q35/IVRS.ivrs > create mode 100644 tests/data/acpi/virt/IORT.iort > > -- > 2.27.0
On Fri, 2 Jul 2021 10:47:20 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Fri, Jun 25, 2021 at 05:17:24AM -0400, Igor Mammedov wrote: > > Highlights: > > * drop pointer arithmetic in ACPI tables code > > * use endian agnostic API > > * simplifies review of tables. /in most cases just line by line comparision with spec/ > > > A hue amount of work, thank you! > To make it easier to merge, how about splitting it up a bit? > E.g. I think first 10-11 patches make sense on their own, right? I think you've meant 01-11 patches, and answer is yes, it's in-depended of actual ACPI refactoring as was mentioned is cover letter, see below. [...] > > > > Series also includes optional qtest patches that add missing acpi > > tests for tables that I'm touching to verify conversion changes. > > That includes an alternative build time based impl. of > > qtest_has_accel() API. So if we start bike-shedding this qtest_has_accel() > > we can safely drop all tests included, till the time discussion settles > > and some form of a qtest_has_accel() is merged, at which point I'd respin > > depended tests. [...] it's ok to split tests into a separate series if that's what you prefer.
On Mon, Jul 05, 2021 at 09:56:18AM +0200, Igor Mammedov wrote: > On Fri, 2 Jul 2021 10:47:20 -0400 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Fri, Jun 25, 2021 at 05:17:24AM -0400, Igor Mammedov wrote: > > > Highlights: > > > * drop pointer arithmetic in ACPI tables code > > > * use endian agnostic API > > > * simplifies review of tables. /in most cases just line by line comparision with spec/ > > > > > > A hue amount of work, thank you! > > To make it easier to merge, how about splitting it up a bit? > > E.g. I think first 10-11 patches make sense on their own, right? > > I think you've meant 01-11 patches, and answer is yes, it's in-depended > of actual ACPI refactoring as was mentioned is cover letter, see below. > > [...] > > > > > > Series also includes optional qtest patches that add missing acpi > > > tests for tables that I'm touching to verify conversion changes. > > > That includes an alternative build time based impl. of > > > qtest_has_accel() API. So if we start bike-shedding this qtest_has_accel() > > > we can safely drop all tests included, till the time discussion settles > > > and some form of a qtest_has_accel() is merged, at which point I'd respin > > > depended tests. > [...] > > it's ok to split tests into a separate series if that's what you prefer. Let's start with a smaller series that still makes sense. I'll merge that we'll look at the next chunk.
On Mon, 5 Jul 2021 14:00:51 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Mon, Jul 05, 2021 at 09:56:18AM +0200, Igor Mammedov wrote: > > On Fri, 2 Jul 2021 10:47:20 -0400 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Fri, Jun 25, 2021 at 05:17:24AM -0400, Igor Mammedov wrote: > > > > Highlights: > > > > * drop pointer arithmetic in ACPI tables code > > > > * use endian agnostic API > > > > * simplifies review of tables. /in most cases just line by line comparision with spec/ > > > > > > > > > A hue amount of work, thank you! > > > To make it easier to merge, how about splitting it up a bit? > > > E.g. I think first 10-11 patches make sense on their own, right? > > > > I think you've meant 01-11 patches, and answer is yes, it's in-depended > > of actual ACPI refactoring as was mentioned is cover letter, see below. > > > > [...] > > > > > > > > Series also includes optional qtest patches that add missing acpi > > > > tests for tables that I'm touching to verify conversion changes. > > > > That includes an alternative build time based impl. of > > > > qtest_has_accel() API. So if we start bike-shedding this qtest_has_accel() > > > > we can safely drop all tests included, till the time discussion settles > > > > and some form of a qtest_has_accel() is merged, at which point I'd respin > > > > depended tests. > > [...] > > > > it's ok to split tests into a separate series if that's what you prefer. > > Let's start with a smaller series that still makes sense. > I'll merge that we'll look at the next chunk. > Ok, I'll drop tests for now and respin as v2 with TPM fixes.
Highlights: * drop pointer arithmetic in ACPI tables code * use endian agnostic API * simplifies review of tables. /in most cases just line by line comparision with spec/ Series replaces build_header() with acpi_init_table()/acpi_table_composed() API that hides pointer/offset arithmetic from user, to prevent errors caused by it [1]. While doing above, it was necessary to split table header from packed structure that was describing the table, which is counter-productive since it still leaves packed structure drawbacks. So that sort of forced me to rewrite tables that were composed with help of packed structures to preferred build_append_int_noprefix() API. In cases where switch to build_append_int_noprefix() was small, it was combined with acpi_init_table()/acpi_table_composed() patch. Conversion reduced code size quite a bit despite me adding doc comments for every table row. Series also includes optional qtest patches that add missing acpi tests for tables that I'm touching to verify conversion changes. That includes an alternative build time based impl. of qtest_has_accel() API. So if we start bike-shedding this qtest_has_accel() we can safely drop all tests included, till the time discussion settles and some form of a qtest_has_accel() is merged, at which point I'd respin depended tests. 1) commits bb9feea43179 x86: acpi: use offset instead of pointer when using build_header() 4d027afeb3a9 Virt: ACPI: fix qemu assert due to re-assigned table data address Link to repo: https://gitlab.com/imammedo/qemu/-/commits/acpi_build_header_refactoring CC: mst@redhat.com Igor Mammedov (53): tests: acpi: dump table with failed checksum tests: qtest: add qtest_has_accel() to check if tested binary supports accelerator tests: acpi: whitelist expected tables for acpi/q35/numamem testcase tests: acpi: q35: test for x2APIC entries in SRAT tests: acpi: update expected tables blobs tests: acpi: whitelist new expected table tests/data/acpi/q35/DMAR.dmar tests: acpi: add testcase for intel_iommu (DMAR table) tests: acpi: add expected blob for DMAR table tests: acpi: whitelist expected blobs for new acpi/q35/ivrs testcase tests: acpi: add testcase for amd-iommu (IVRS table) tests: acpi: update expected blobs tests: acpi: arm/virt: drop redundant test_acpi_one() in test_acpi_virt_tcg() tests: acpi: whitelist expected tables for acpi/virt/iort testcase tests: acpi: arm/virt: use kvm to test IORT table tests: acpi: add expected IORT table blob tests: arm-cpu-features: use qtest_has_kvm() API tests: migration-test: use qtest_has_accel() API tests: bios-tables-test: use qtest_has_accel() API to register TCG only tests acpi: add helper routines to initialize ACPI tables acpi: build_rsdt: use acpi_init_table()/acpi_table_composed() instead of build_header() acpi: build_xsdt: use acpi_init_table()/acpi_table_composed() instead of build_header() acpi: build_slit: use acpi_init_table()/acpi_table_composed() instead of build_header() acpi: build_fadt: use acpi_init_table()/acpi_table_composed() instead of build_header() acpi: build_tpm2: use acpi_init_table()/acpi_table_composed() instead of build_header() acpi: acpi_build_hest: use acpi_init_table()/acpi_table_composed() instead of build_header() acpi: build_mcfg: use acpi_init_table()/acpi_table_composed() instead of build_header() acpi: build_hmat: use acpi_init_table()/acpi_table_composed() instead of build_header() acpi: nvdimm_build_nfit: use acpi_init_table()/acpi_table_composed() instead of build_header() acpi: nvdimm_build_ssdt: use acpi_init_table()/acpi_table_composed() instead of build_header() acpi: vmgenid_build_acpi: use acpi_init_table()/acpi_table_composed() instead of build_header() acpi: x86: build_dsdt: use acpi_init_table()/acpi_table_composed() instead of build_header() acpi: build_hpet: use acpi_init_table()/acpi_table_composed() instead of build_header() acpi: build_tpm_tcpa: use acpi_init_table()/acpi_table_composed() instead of build_header() acpi: arm/x86: build_srat: use acpi_init_table()/acpi_table_composed() instead of build_header() acpi: use build_append_int_noprefix() API to compose SRAT table acpi: build_dmar_q35: use acpi_init_table()/acpi_table_composed() instead of build_header() acpi: build_waet: use acpi_init_table()/acpi_table_composed() instead of build_header() acpi: build_amd_iommu: use acpi_init_table()/acpi_table_composed() instead of build_header() acpi: madt: arm/x86: use acpi_init_table()/acpi_table_composed() instead of build_header() acpi: x86: remove dead code acpi: x86: set enabled when composing _MAT entries acpi: x86: madt: use build_append_int_noprefix() API to compose MADT table acpi: arm/virt: madt: use build_append_int_noprefix() API to compose MADT table acpi: build_dsdt_microvm: use acpi_init_table()/acpi_table_composed() instead of build_header() acpi: arm: virt: build_dsdt: use acpi_init_table()/acpi_table_composed() instead of build_header() acpi: arm: virt: build_iort: use acpi_init_table()/acpi_table_composed() instead of build_header() acpi: arm/virt: convert build_iort() to endian agnostic build_append_FOO() API acpi: arm/virt: build_spcr: fix invalid cast acpi: arm/virt: build_spcr: use acpi_init_table()/acpi_table_composed() instead of build_header() acpi: arm/virt: build_gtdt: use acpi_init_table()/acpi_table_composed() instead of build_header() acpi: build_facs: use build_append_int_noprefix() API to compose table acpi: remove no longer used build_header() acpi: AcpiGenericAddress no longer used to map/access fields of MMIO, drop packed attribute include/hw/acpi/acpi-defs.h | 528 +------------------------ include/hw/acpi/acpi_dev_interface.h | 3 +- include/hw/acpi/aml-build.h | 21 +- include/hw/i386/pc.h | 7 +- tests/qtest/libqos/libqtest.h | 8 + hw/acpi/acpi-x86-stub.c | 3 +- hw/acpi/aml-build.c | 188 +++++---- hw/acpi/cpu.c | 17 +- hw/acpi/ghes.c | 10 +- hw/acpi/hmat.c | 14 +- hw/acpi/nvdimm.c | 64 ++-- hw/acpi/pci.c | 18 +- hw/acpi/vmgenid.c | 16 +- hw/arm/virt-acpi-build.c | 550 +++++++++++++++------------ hw/i386/acpi-build.c | 285 +++++++------- hw/i386/acpi-common.c | 158 ++++---- hw/i386/acpi-microvm.c | 13 +- meson.build | 6 + tests/data/acpi/q35/APIC.numamem | Bin 0 -> 2686 bytes tests/data/acpi/q35/DMAR.dmar | Bin 0 -> 80 bytes tests/data/acpi/q35/DSDT.ivrs | Bin 0 -> 7877 bytes tests/data/acpi/q35/DSDT.numamem | Bin 7865 -> 35222 bytes tests/data/acpi/q35/FACP.numamem | Bin 0 -> 244 bytes tests/data/acpi/q35/IVRS.ivrs | Bin 0 -> 104 bytes tests/data/acpi/q35/SRAT.numamem | Bin 224 -> 5080 bytes tests/data/acpi/virt/IORT.iort | Bin 0 -> 124 bytes tests/qtest/acpi-utils.c | 14 + tests/qtest/arm-cpu-features.c | 29 +- tests/qtest/bios-tables-test.c | 85 ++++- tests/qtest/libqtest.c | 27 ++ tests/qtest/migration-test.c | 15 +- 31 files changed, 873 insertions(+), 1206 deletions(-) create mode 100644 tests/data/acpi/q35/APIC.numamem create mode 100644 tests/data/acpi/q35/DMAR.dmar create mode 100644 tests/data/acpi/q35/DSDT.ivrs create mode 100644 tests/data/acpi/q35/FACP.numamem create mode 100644 tests/data/acpi/q35/IVRS.ivrs create mode 100644 tests/data/acpi/virt/IORT.iort -- 2.27.0