[Qemu-devel] [PATCH v4 00/23] ACPI reorganization for hardware-reduced support

Samuel Ortiz posted 23 patches 5 years, 5 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20181101102303.16439-1-sameo@linux.intel.com
Test docker-clang@ubuntu passed
Test checkpatch passed
Test asan passed
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
hw/acpi/Makefile.objs          |    1 +
hw/acpi/aml-build.c            |  985 +++++++++++++++++++++++++++++
hw/acpi/builder.c              |   97 +++
hw/acpi/cpu.c                  |    8 +-
hw/acpi/cpu_hotplug.c          |    9 +-
hw/acpi/memory_hotplug.c       |   21 +-
hw/acpi/pcihp.c                |   10 +-
hw/arm/virt-acpi-build.c       |   94 +--
hw/i386/acpi-build.c           | 1071 +++-----------------------------
hw/i386/acpi-build.h           |    9 +-
hw/i386/pc.c                   |  198 +++---
hw/i386/pc_piix.c              |   36 +-
hw/i386/pc_q35.c               |   22 +-
hw/i386/xen/xen-hvm.c          |   19 +-
hw/pci-host/piix.c             |   32 +-
include/hw/acpi/acpi-defs.h    |   14 +
include/hw/acpi/acpi.h         |   44 ++
include/hw/acpi/aml-build.h    |   47 ++
include/hw/acpi/builder.h      |  100 +++
include/hw/i386/acpi.h         |   27 +
include/hw/i386/pc.h           |   49 +-
include/hw/mem/memory-device.h |    2 +
include/hw/pci/pci_host.h      |    6 +
stubs/Makefile.objs            |    1 -
stubs/pci-host-piix.c          |    6 -
25 files changed, 1648 insertions(+), 1260 deletions(-)
create mode 100644 hw/acpi/builder.c
create mode 100644 include/hw/acpi/builder.h
create mode 100644 include/hw/i386/acpi.h
delete mode 100644 stubs/pci-host-piix.c
[Qemu-devel] [PATCH v4 00/23] ACPI reorganization for hardware-reduced support
Posted by Samuel Ortiz 5 years, 5 months ago
This patch set provides an ACPI code reorganization in preparation for
adding hardware-reduced support to QEMU.

The changes are coming from the NEMU [1] project where we're defining
a new x86 machine type: i386/virt. This is an EFI only, ACPI
hardware-reduced platform and as such we had to implement support
for the latter.

As a preliminary for adding hardware-reduced support to QEMU, we did
some ACPI code reorganization with the following goals:

* Share as much as possible of the current ACPI build APIs between
  legacy and hardware-reduced ACPI.
* Share the ACPI build code across machine types and architectures and
  remove the typical PC machine type dependency.
  Eventually we hope to see arm/virt also re-use much of that code.

The patches are also available in their own git branch [2].

[1] https://github.com/intel/nemu
[2] https://github.com/intel/nemu/tree/topic/upstream/acpi

v1 -> v2:
   * Drop the hardware-reduced implementation for now. Our next patch set
     will add hardware-reduced and convert arm/virt to it.
   * Implement the ACPI build methods as a QOM Interface Class and convert
     the PC machine type to it.
   * acpi_conf_pc_init() uses a PCMachineState pointer and not a
     MachineState one as its argument.

v2 -> v3:
   * Cc all relevant maintainers, no functional changes.

v3 -> v4:
   * Renamed all AcpiConfiguration pointers from conf to acpi_conf.
   * Removed the ACPI_BUILD_ALIGN_SIZE export.
   * Temporarily updated the arm virt build_rsdp() prototype for
     bisectability purposes.
   * Removed unneeded pci headers from acpi-build.c.
   * Refactor the acpi PCI host getter so that it truly is architecture
     agnostic, by carrying the PCI host pointer through the
     AcpiConfiguration structure.
   * Splitted the PCI host AML builder API export patch from the PCI host
     and holes getter one.
   * Reduced the build_srat() export scope to hw/i386 instead of the broader
     hw/acpi. SRAT builders are truly architecture specific and can hardly be
     generalized.
   * Completed the ACPI builder documentation.

Samuel Ortiz (15):
  hw: i386: Decouple the ACPI build from the PC machine type
  hw: acpi: Export ACPI build alignment API
  hw: acpi: Export the RSDP build API
  hw: acpi: Implement XSDT support for RSDP
  hw: arm: Switch to the AML build RSDP building routine
  hw: i386: Move PCI host definitions to pci_host.h
  hw: acpi: Export the PCI host and holes getters
  hw: acpi: Do not create hotplug method when handler is not defined
  hw: i386: Make the hotpluggable memory size property more generic
  hw: i386: Export the i386 ACPI SRAT build method
  hw: i386: Export the MADT build method
  hw: acpi: Define ACPI tables builder interface
  hw: i386: Implement the ACPI builder interface for PC
  hw: pci-host: piix: Return PCI host pointer instead of PCI bus
  hw: i386: Set ACPI configuration PCI host pointer

Sebastien Boeuf (2):
  hw: acpi: Export the PCI hotplug API
  hw: acpi: Retrieve the PCI bus from AcpiPciHpState

Yang Zhong (6):
  hw: acpi: Generalize AML build routines
  hw: acpi: Factorize _OSC AML across architectures
  hw: acpi: Export and generalize the PCI host AML API
  hw: acpi: Export the MCFG getter
  hw: acpi: Fix memory hotplug AML generation error
  hw: i386: Refactor PCI host getter

 hw/acpi/Makefile.objs          |    1 +
 hw/acpi/aml-build.c            |  985 +++++++++++++++++++++++++++++
 hw/acpi/builder.c              |   97 +++
 hw/acpi/cpu.c                  |    8 +-
 hw/acpi/cpu_hotplug.c          |    9 +-
 hw/acpi/memory_hotplug.c       |   21 +-
 hw/acpi/pcihp.c                |   10 +-
 hw/arm/virt-acpi-build.c       |   94 +--
 hw/i386/acpi-build.c           | 1071 +++-----------------------------
 hw/i386/acpi-build.h           |    9 +-
 hw/i386/pc.c                   |  198 +++---
 hw/i386/pc_piix.c              |   36 +-
 hw/i386/pc_q35.c               |   22 +-
 hw/i386/xen/xen-hvm.c          |   19 +-
 hw/pci-host/piix.c             |   32 +-
 include/hw/acpi/acpi-defs.h    |   14 +
 include/hw/acpi/acpi.h         |   44 ++
 include/hw/acpi/aml-build.h    |   47 ++
 include/hw/acpi/builder.h      |  100 +++
 include/hw/i386/acpi.h         |   27 +
 include/hw/i386/pc.h           |   49 +-
 include/hw/mem/memory-device.h |    2 +
 include/hw/pci/pci_host.h      |    6 +
 stubs/Makefile.objs            |    1 -
 stubs/pci-host-piix.c          |    6 -
 25 files changed, 1648 insertions(+), 1260 deletions(-)
 create mode 100644 hw/acpi/builder.c
 create mode 100644 include/hw/acpi/builder.h
 create mode 100644 include/hw/i386/acpi.h
 delete mode 100644 stubs/pci-host-piix.c

-- 
2.19.1


Re: [Qemu-devel] [PATCH v4 00/23] ACPI reorganization for hardware-reduced support
Posted by Igor Mammedov 5 years, 5 months ago
On Thu,  1 Nov 2018 11:22:40 +0100
Samuel Ortiz <sameo@linux.intel.com> wrote:

Thanks for looking at ACPI mess we have in QEMU and trying to make it better,
this series look a bit hackish probably because it was written to suit new
virt board, so it needs some more clean up to be done.

> This patch set provides an ACPI code reorganization in preparation for
> adding hardware-reduced support to QEMU.
QEMU already has hw reduced implementation, specifically in arm/virt board

> The changes are coming from the NEMU [1] project where we're defining
> a new x86 machine type: i386/virt. This is an EFI only, ACPI
> hardware-reduced platform and as such we had to implement support
> for the latter.
> 
> As a preliminary for adding hardware-reduced support to QEMU, we did
s:support to QEMU:support for new i386/virt machine:

> some ACPI code reorganization with the following goals:
> 
> * Share as much as possible of the current ACPI build APIs between
>   legacy and hardware-reduced ACPI.
> * Share the ACPI build code across machine types and architectures and
>   remove the typical PC machine type dependency.
>   Eventually we hope to see arm/virt also re-use much of that code.
it probably should be other way around, generalize and reuse as much of
arm/virt acpi code, instead of adding new duplicated code without
an actual user and then swapping/dropping old arm version in favor of the
new one. It's hard to review when it done in this order and easy to miss
issues that would be easier to spot if you reused arm versions (where
applicable) as starting point for generalization.

Here are some generic suggestions/nits that apply to whole series:
  * s/Factorize/Factor out/
  * try to restructure series in following way
     1. put bug fixes at the beginning of series.
        'make V=1 check' should produce tables diffs to account for
        changes made in the tables.
        After error fixing, add an extra patch to update reference
        ACPI tables to simplify testing for reviewing and so for
        self-check when you'll be doing refactoring to make sure
        there aren't any changes during generalization/refactoring
        later.

     2. instead of adding 'new' implementations try to generalize
        existing ones so that a user for new code will always exist.
        It's also makes patches easier to review/test.
        
     3. Since you are touching/moving around existing fixed tables
        that are using legacy 'struct' based approach to construct
        them, it's a good as opportunity to switch to a newer approach
        and use build_append_int_noprefix() API to construct tables.
        Use build_amd_iommu() as example. And it's doubly true if/when
        you are adding new fixed tables (i.e. only build_append_int_noprefix()
        based ones are acceptable).

     4. for patches during #2 and #3 stages, 'make V=1 check'
        should pass without any warnings, that will speed up review
        process.

     5. add i386/virt board and related hardware reduced ACPI code
        that's specific to it.

I'll try to review series during next week and will do per patch
suggestions how to structure it or do other way.

Hopefully after doing refactoring we would end up with
simpler/smaller and cleaner ACPI code to the benefit of everyone.

PS:
if you need a quick advice wrt APCI parts, feel free to ping me on IRC
(Paris timezone).

> The patches are also available in their own git branch [2].
> 
> [1] https://github.com/intel/nemu
> [2] https://github.com/intel/nemu/tree/topic/upstream/acpi
> 
> v1 -> v2:
>    * Drop the hardware-reduced implementation for now. Our next patch set
>      will add hardware-reduced and convert arm/virt to it.
>    * Implement the ACPI build methods as a QOM Interface Class and convert
>      the PC machine type to it.
>    * acpi_conf_pc_init() uses a PCMachineState pointer and not a
>      MachineState one as its argument.
> 
> v2 -> v3:
>    * Cc all relevant maintainers, no functional changes.
> 
> v3 -> v4:
>    * Renamed all AcpiConfiguration pointers from conf to acpi_conf.
>    * Removed the ACPI_BUILD_ALIGN_SIZE export.
>    * Temporarily updated the arm virt build_rsdp() prototype for
>      bisectability purposes.
>    * Removed unneeded pci headers from acpi-build.c.
>    * Refactor the acpi PCI host getter so that it truly is architecture
>      agnostic, by carrying the PCI host pointer through the
>      AcpiConfiguration structure.
>    * Splitted the PCI host AML builder API export patch from the PCI host
>      and holes getter one.
>    * Reduced the build_srat() export scope to hw/i386 instead of the broader
>      hw/acpi. SRAT builders are truly architecture specific and can hardly be
>      generalized.
>    * Completed the ACPI builder documentation.
> 
> Samuel Ortiz (15):
>   hw: i386: Decouple the ACPI build from the PC machine type
>   hw: acpi: Export ACPI build alignment API
>   hw: acpi: Export the RSDP build API
>   hw: acpi: Implement XSDT support for RSDP
>   hw: arm: Switch to the AML build RSDP building routine
>   hw: i386: Move PCI host definitions to pci_host.h
>   hw: acpi: Export the PCI host and holes getters
>   hw: acpi: Do not create hotplug method when handler is not defined
>   hw: i386: Make the hotpluggable memory size property more generic
>   hw: i386: Export the i386 ACPI SRAT build method
>   hw: i386: Export the MADT build method
>   hw: acpi: Define ACPI tables builder interface
>   hw: i386: Implement the ACPI builder interface for PC
>   hw: pci-host: piix: Return PCI host pointer instead of PCI bus
>   hw: i386: Set ACPI configuration PCI host pointer
> 
> Sebastien Boeuf (2):
>   hw: acpi: Export the PCI hotplug API
>   hw: acpi: Retrieve the PCI bus from AcpiPciHpState
> 
> Yang Zhong (6):
>   hw: acpi: Generalize AML build routines
>   hw: acpi: Factorize _OSC AML across architectures
>   hw: acpi: Export and generalize the PCI host AML API
>   hw: acpi: Export the MCFG getter
>   hw: acpi: Fix memory hotplug AML generation error
>   hw: i386: Refactor PCI host getter
> 
>  hw/acpi/Makefile.objs          |    1 +
>  hw/acpi/aml-build.c            |  985 +++++++++++++++++++++++++++++
>  hw/acpi/builder.c              |   97 +++
>  hw/acpi/cpu.c                  |    8 +-
>  hw/acpi/cpu_hotplug.c          |    9 +-
>  hw/acpi/memory_hotplug.c       |   21 +-
>  hw/acpi/pcihp.c                |   10 +-
>  hw/arm/virt-acpi-build.c       |   94 +--
>  hw/i386/acpi-build.c           | 1071 +++-----------------------------
>  hw/i386/acpi-build.h           |    9 +-
>  hw/i386/pc.c                   |  198 +++---
>  hw/i386/pc_piix.c              |   36 +-
>  hw/i386/pc_q35.c               |   22 +-
>  hw/i386/xen/xen-hvm.c          |   19 +-
>  hw/pci-host/piix.c             |   32 +-
>  include/hw/acpi/acpi-defs.h    |   14 +
>  include/hw/acpi/acpi.h         |   44 ++
>  include/hw/acpi/aml-build.h    |   47 ++
>  include/hw/acpi/builder.h      |  100 +++
>  include/hw/i386/acpi.h         |   27 +
>  include/hw/i386/pc.h           |   49 +-
>  include/hw/mem/memory-device.h |    2 +
>  include/hw/pci/pci_host.h      |    6 +
>  stubs/Makefile.objs            |    1 -
>  stubs/pci-host-piix.c          |    6 -
>  25 files changed, 1648 insertions(+), 1260 deletions(-)
>  create mode 100644 hw/acpi/builder.c
>  create mode 100644 include/hw/acpi/builder.h
>  create mode 100644 include/hw/i386/acpi.h
>  delete mode 100644 stubs/pci-host-piix.c
> 


Re: [Qemu-devel] [PATCH v4 00/23] ACPI reorganization for hardware-reduced support
Posted by Samuel Ortiz 5 years, 5 months ago
Hi Igor,

On Fri, Nov 02, 2018 at 01:29:25PM +0100, Igor Mammedov wrote:
> On Thu,  1 Nov 2018 11:22:40 +0100
> Samuel Ortiz <sameo@linux.intel.com> wrote:
> 
> Thanks for looking at ACPI mess we have in QEMU and trying to make it better,
Thanks for the initial review and feedback.

> this series look a bit hackish probably because it was written to suit new
> virt board, so it needs some more clean up to be done.
>
> > This patch set provides an ACPI code reorganization in preparation for
> > adding hardware-reduced support to QEMU.
> QEMU already has hw reduced implementation, specifically in arm/virt board
Back in May, I tried booting a virt machine type with "acpi=force" with
no success, and today's HEAD still fails. With "acpi=on":

[    0.000000] ACPI: Early table checksum verification disabled
[    0.000000] ACPI: Failed to init ACPI tables

So this code has been broken for several months and I suspect it's not
really run by anyone.
Moreover, even though the virt-acpi-build.c code aims at building a
hardware-reduced ACPI compliant set of tables, the implementation is
largely specific to the arm/virt board. We did take the generic parts
from it in order to build a shareable API across architectures.

So by "adding hardware-reduced support to QEMU", we meant providing a
architecture and machine type agnostic hardware-reduced ACPI
implementation that all QEMU machine types could use.
I'll make sure to change the cover letter wording and rephrase it to
reflect our intention.

> > The changes are coming from the NEMU [1] project where we're defining
> > a new x86 machine type: i386/virt. This is an EFI only, ACPI
> > hardware-reduced platform and as such we had to implement support
> > for the latter.
> > 
> > As a preliminary for adding hardware-reduced support to QEMU, we did
> s:support to QEMU:support for new i386/virt machine:
Most of this serie moves non x86 specific stuff out of i386 into the
(theoretically) architecture agnostic hw/acpi folder in order to a) reuse
and share as much as possible of the non x86 specific ACPI code that
currently lives under hw/i386 and b) improve and extend the current
hw/acpi APIs.
So on one hand I agree this cover letter needs massaging, but on the
other hand I feel it's unfair to say there's anything specific to
i386/virt on this serie. At least we tried really hard to avoid falling
into that trap.

> > some ACPI code reorganization with the following goals:
> > 
> > * Share as much as possible of the current ACPI build APIs between
> >   legacy and hardware-reduced ACPI.
> > * Share the ACPI build code across machine types and architectures and
> >   remove the typical PC machine type dependency.
> >   Eventually we hope to see arm/virt also re-use much of that code.
> it probably should be other way around, generalize and reuse as much of
> arm/virt acpi code,
Our hardware-reduced implementation does that indeed, and the next
patchset following this one will add the actual hardware-reduced ACPI
API implementation together with the arm/virt switch to it (which will
mostly be code removal from virt-acpi-build.c).

> instead of adding new duplicated code without
> an actual user and then swapping/dropping old arm version in favor of the
> new one.
This patch set is not adding any code duplication, I hope.
It's really preparing the current ACPI code in order to precisely NOT add
code duplication when building a machine type agnostic hardware reduced
ACPI API.
I initally added our hardware-reduced ACPI API to that patch serie but
decided to remove it. Here is what it looks like:
https://github.com/intel/nemu/blob/topic/virt-x86/hw/acpi/reduced.c

It's consuming the factorization work that this serie provides, and the
arm/virt machine type will use a fairly large part of that API.

> It's hard to review when it done in this order and easy to miss
> issues that would be easier to spot if you reused arm versions (where
> applicable) as starting point for generalization.
> 
> Here are some generic suggestions/nits that apply to whole series:
>   * s/Factorize/Factor out/
>   * try to restructure series in following way
>      1. put bug fixes at the beginning of series.
I'll try to do that, yes.


>         'make V=1 check' should produce tables diffs to account for
>         changes made in the tables.
I'll run the make check tests, thanks for the reminder.


>         After error fixing, add an extra patch to update reference
>         ACPI tables to simplify testing for reviewing and so for
>         self-check when you'll be doing refactoring to make sure
>         there aren't any changes during generalization/refactoring
>         later.
>
>      2. instead of adding 'new' implementations try to generalize
>         existing ones so that a user for new code will always exist.
>         It's also makes patches easier to review/test.
The PC machine type is consuming all the exported API and e.g. the
new ACPI builder interface as well.


>      3. Since you are touching/moving around existing fixed tables
>         that are using legacy 'struct' based approach to construct
>         them, it's a good as opportunity to switch to a newer approach
>         and use build_append_int_noprefix() API to construct tables.
>         Use build_amd_iommu() as example. And it's doubly true if/when
>         you are adding new fixed tables (i.e. only build_append_int_noprefix()
>         based ones are acceptable).
I'd be happy to do that, but I'd appreciate if that could be done as a
follow up patch set, in order to decouple the code movement/export from
the actual reimplementation.


>      4. for patches during #2 and #3 stages, 'make V=1 check'
>         should pass without any warnings, that will speed up review
>         process.
> 
>      5. add i386/virt board and related hardware reduced ACPI code
>         that's specific to it.
That is definitely going to be our last steps, but I think this would
make a very large patch set to review at once.


> I'll try to review series during next week and will do per patch
> suggestions how to structure it or do other way.
Thanks in advance. FWIW I just sent v5, addressing Philippe's comments.
I'll wait for your feedback before sending v6.

> Hopefully after doing refactoring we would end up with
> simpler/smaller and cleaner ACPI code to the benefit of everyone.
> 
> PS:
> if you need a quick advice wrt APCI parts, feel free to ping me on IRC
> (Paris timezone).
Nice, same timezone for me :)
I'll get in touch next week.

Cheers,
Samuel.

Re: [Qemu-devel] [PATCH v4 00/23] ACPI reorganization for hardware-reduced support
Posted by Samuel Ortiz 5 years, 5 months ago
Igor,

On Mon, Nov 05, 2018 at 03:10:28AM +0100, Samuel Ortiz wrote:
> > QEMU already has hw reduced implementation, specifically in arm/virt board
> Back in May, I tried booting a virt machine type with "acpi=force" with
> no success, and today's HEAD still fails. With "acpi=on":
> 
> [    0.000000] ACPI: Early table checksum verification disabled
> [    0.000000] ACPI: Failed to init ACPI tables
My bad, I was using the wrong firmware/pflash options. With the right
ones, I can boot with an edk2 firmware and an EFI variable space. Thanks
Philippe and Eric for the help.

Cheers,
Samuel.

Re: [Qemu-devel] [PATCH v4 00/23] ACPI reorganization for hardware-reduced support
Posted by Andrew Jones 5 years, 5 months ago
On Mon, Nov 05, 2018 at 03:10:28AM +0100, Samuel Ortiz wrote:
> Hi Igor,
> 
> On Fri, Nov 02, 2018 at 01:29:25PM +0100, Igor Mammedov wrote:
> > On Thu,  1 Nov 2018 11:22:40 +0100
> > Samuel Ortiz <sameo@linux.intel.com> wrote:
> > 
> > Thanks for looking at ACPI mess we have in QEMU and trying to make it better,
> Thanks for the initial review and feedback.
> 
> > this series look a bit hackish probably because it was written to suit new
> > virt board, so it needs some more clean up to be done.
> >
> > > This patch set provides an ACPI code reorganization in preparation for
> > > adding hardware-reduced support to QEMU.
> > QEMU already has hw reduced implementation, specifically in arm/virt board
> Back in May, I tried booting a virt machine type with "acpi=force" with
> no success, and today's HEAD still fails. With "acpi=on":
> 
> [    0.000000] ACPI: Early table checksum verification disabled
> [    0.000000] ACPI: Failed to init ACPI tables
> 
> So this code has been broken for several months and I suspect it's not
> really run by anyone.

Hi Samuel,

We use it all the time. To see if there had been an upstream regression I
just tried latest qemu (v3.0.0-1763-gb2f7a038bb4c) with a latest kernel
(v4.20-rc1) in my guest. It worked just fine, and there was no need for
any kernel command line parameters.

What guest kernel version did you try?

Thanks,
drew

Re: [Qemu-devel] [PATCH v4 00/23] ACPI reorganization for hardware-reduced support
Posted by Samuel Ortiz 5 years, 5 months ago
Hi Andrew,

On Mon, Nov 05, 2018 at 05:07:53PM +0100, Andrew Jones wrote:
> > > QEMU already has hw reduced implementation, specifically in arm/virt board
> > Back in May, I tried booting a virt machine type with "acpi=force" with
> > no success, and today's HEAD still fails. With "acpi=on":
> > 
> > [    0.000000] ACPI: Early table checksum verification disabled
> > [    0.000000] ACPI: Failed to init ACPI tables
> > 
> > So this code has been broken for several months and I suspect it's not
> > really run by anyone.
> 
> Hi Samuel,
> 
> We use it all the time. To see if there had been an upstream regression I
> just tried latest qemu (v3.0.0-1763-gb2f7a038bb4c) with a latest kernel
> (v4.20-rc1) in my guest. It worked just fine, and there was no need for
> any kernel command line parameters.
> 
> What guest kernel version did you try?
This is my bad, apologies for jumping the gun. I was not using the right
pflash options and thus the ACPI tables were simply not passed to the
kernel. Philippe and Eric helped me fix that, and now I can boot
arm/virt on top of ACPI. Sorry for the bad noise.

Cheers,
Samuel.

Re: [Qemu-devel] [PATCH v4 00/23] ACPI reorganization for hardware-reduced support
Posted by Igor Mammedov 5 years, 5 months ago
On Mon, 5 Nov 2018 03:10:28 +0100
Samuel Ortiz <sameo@linux.intel.com> wrote:

> Hi Igor,
> 
> On Fri, Nov 02, 2018 at 01:29:25PM +0100, Igor Mammedov wrote:
> > On Thu,  1 Nov 2018 11:22:40 +0100
> > Samuel Ortiz <sameo@linux.intel.com> wrote:
[...]
> > > some ACPI code reorganization with the following goals:
> > > 
> > > * Share as much as possible of the current ACPI build APIs between
> > >   legacy and hardware-reduced ACPI.
> > > * Share the ACPI build code across machine types and architectures and
> > >   remove the typical PC machine type dependency.
> > >   Eventually we hope to see arm/virt also re-use much of that code.  
> > it probably should be other way around, generalize and reuse as much of
> > arm/virt acpi code,  
> Our hardware-reduced implementation does that indeed, and the next
> patchset following this one will add the actual hardware-reduced ACPI
> API implementation together with the arm/virt switch to it (which will
> mostly be code removal from virt-acpi-build.c).
That's where I disagree with patch ordering, i.e. you are adding impl. without
user and than do sudden switch from old API (dropping old one in the process)
to the new one some time in the future.

For example 04/23 add new impl. of build_rsdp() with xsdt support without
actual user, which is duplicate of arm/virt code modulo checksum fix
and than later in 05/23 drops old arm impl. in favor of the new one.

As you saw it causes confusion during review even within one series
and it's much worse in cases where similar changes happen across
several series. In addition bisection would point into wrong patch
05/23 (where code started to be used but not where it was introduced).


> > instead of adding new duplicated code without
> > an actual user and then swapping/dropping old arm version in favor of the
> > new one.  
> This patch set is not adding any code duplication, I hope.
> It's really preparing the current ACPI code in order to precisely NOT add
> code duplication when building a machine type agnostic hardware reduced
> ACPI API.
> I initally added our hardware-reduced ACPI API to that patch serie but
> decided to remove it. Here is what it looks like:
> https://github.com/intel/nemu/blob/topic/virt-x86/hw/acpi/reduced.c
> 
> It's consuming the factorization work that this serie provides, and the
> arm/virt machine type will use a fairly large part of that API.
the point is existing arm/virt machine or any other machine should use
new API right away if applicable instead of some point in the future,
otherwise we are doing something wrong, change might not belong to series.

It's hard to find reasoning for merging new API/verify correctness
without immediate user.


> > It's hard to review when it done in this order and easy to miss
> > issues that would be easier to spot if you reused arm versions (where
> > applicable) as starting point for generalization.

[...]

> >      3. Since you are touching/moving around existing fixed tables
> >         that are using legacy 'struct' based approach to construct
> >         them, it's a good as opportunity to switch to a newer approach
> >         and use build_append_int_noprefix() API to construct tables.
> >         Use build_amd_iommu() as example. And it's doubly true if/when
> >         you are adding new fixed tables (i.e. only build_append_int_noprefix()
> >         based ones are acceptable).  
> I'd be happy to do that, but I'd appreciate if that could be done as a
> follow up patch set, in order to decouple the code movement/export from
> the actual reimplementation.
Well, it fine to keep old style if one fixes code but in all other cases
it's good opportunity to replace legacy approach with the preferred one.
So unless it's a fix, the new code (including generalization) should
use preferred approach (there are exceptions but this series is not the case).
Considering this series is re-factoring to make ACPI code more reusable,
we should do it properly instead of cutting corners.

I apply this rule myself 'ex: build_fadt()' and others if re-factoring
is related the series topic.
 
 
> >      4. for patches during #2 and #3 stages, 'make V=1 check'
> >         should pass without any warnings, that will speed up review
> >         process.
> > 
> >      5. add i386/virt board and related hardware reduced ACPI code
> >         that's specific to it.  
> That is definitely going to be our last steps, but I think this would
> make a very large patch set to review at once.
Agreed, as far as new API is used by already existing code. If API isn't
used then it should be moved out of this series to the one that would
actually use it.

[...]