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

[Igor Mammedov]

On Mon, 5 Nov 2018 03:10:28 +0100
Samuel Ortiz <address@hidden> 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 <address@hidden> 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.

[...]