Re: [PATCH v2 00/35] acpi: refactor error prone build_header() and packed structures usage in ACPI tables

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Jul 14, 2021 at 10:53:59AM +0200, Igor Mammedov wrote:
> On Tue, 13 Jul 2021 11:45:46 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Thu, Jul 08, 2021 at 11:45:42AM -0400, Igor Mammedov wrote:
> > > v2:
> > >   - drop test related patches, they will be sent as a separate series
> > >   - fix bios_loader pointer initialization ordering when using TPM1.2
> > >   - extend commit message of [1/35] and add extra comment about
> > >     table length patching   
> > 
> > 
> > So I think this is in good shape, and I really like the code size
> > reduction. Given a rush to push things before the freeze, I'm inclined
> > to just defer API rework to the next release. This kind of thing is
> > better at the beginning of the cycle so we can iron out API kinks if any
> > surface.
> 
> ok, I'll respin once another merge window is open.
> 
> As for tests part, that I dropped, should I repost it
> for merging during soft-freeze or we postpone that as well?

I'm inclined to say it's best to defer that too.
The point of the freeze is so we can focus on fixing errors,
mostly regressions.


> > 
> > > 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 refactoring to build_append_int_noprefix() was small,
> > > it was squashed with acpi_init_table()/acpi_table_composed() patch.
> > > Conversion reduced code size quite a bit despite me adding doc comments
> > > for every table row.
> > > 
> > > 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_v2
> > > 
> > > CC: mst@redhat.com
> > > 
> > > 
> > > 
> > > Igor Mammedov (35):
> > >   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 +-
> > >  hw/acpi/acpi-x86-stub.c              |   3 +-
> > >  hw/acpi/aml-build.c                  | 193 ++++++----
> > >  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                 | 284 ++++++++------
> > >  hw/i386/acpi-common.c                | 158 ++++----
> > >  hw/i386/acpi-microvm.c               |  13 +-
> > >  16 files changed, 750 insertions(+), 1149 deletions(-)
> > > 
> > > -- 
> > > 2.27.0  
> >