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

[Samuel Ortiz]

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 <address@hidden> 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.