Re: [Qemu-arm] [Qemu-devel] [PATCH v4 6/6] acpi: pci: use build_append_foo() API to construct MCFG

[Michael S. Tsirkin]

On Thu, May 16, 2019 at 07:00:33PM +0200, Igor Mammedov wrote:
> On Thu, 16 May 2019 13:01:31 +0200
> Philippe Mathieu-Daudé <address@hidden> wrote:
> 
> > On Thu, May 16, 2019 at 9:41 AM Wei Yang <address@hidden> wrote:
> > >
> > > On Wed, May 15, 2019 at 07:29:17AM +0200, Philippe Mathieu-Daudé wrote:  
> > > >
> > > >Thanks Michael for testing...
> > > >
> > > >Wei, can you add a MCFG test in tests/bios-tables-test.c?
> > > >  
> > >
> > > I took a look into the test, current q35 has already has a reference MCFG 
> > > in
> > > tests/data/acpi/q35/MCFG.
> > >
> > > And there would be a warning message when reserved[8] is missed.
> > >
> > >     /x86_64/acpi/q35/bridge: acpi-test: Warning! MCFG mismatch.
> > >
> > > Is this enough? Or what more information prefer to add?  
> > 
> > Well, the test has to fail for any mismatch (not a simple warning).
> > 
> > A mismatch failure seems to be enough IMHO.
> Warning is sufficient, we do not fail ACPI tests on mismatch.
> It was a policy decision for APCI tests as far as I remember.
> We might reconsider it in the future but it shouldn't affect this patch.

Yes. And the reason is that conflicts in binary expected files are
impossible to resolve. So it's important that we can
fix expected files after a patch that changes them.

I actually have an idea for a better way to fix this:
a special list of "warn on mismatch" files.

A patch changing tables will add the changed tables to the list.
Then maintainer knows to inspec the diff manually
and re-generate expected files, and remove the
changed tables from the list.



Another thing we should do is drop dependency on IASL:

if IASL is present we should use it to show diff to simplify debugging
but at this point a verbatim difference is good enough if IASL is not
installed.


And I agree 100%: all this is a subject for a separate patch(set).



> 
> > 
> > > >>> -    AcpiMcfgAllocation allocation[0];
> > > >>> -} QEMU_PACKED;
> > > >>> -typedef struct AcpiTableMcfg AcpiTableMcfg;
> > > >>> -
> > > >>>  /*
> > > >>>   * TCPA Description Table
> > > >>>   *
> > > >>> --
> > > >>> 2.19.1  
> > >
> > > --
> > > Wei Yang
> > > Help you, Help me  
> >