Re: [PATCH v2 02/13] tests/acpi: virt: allow acpi table changes for a new table: HEST

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 30 Jan 2025 14:03:24 +0100
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Em Wed, 29 Jan 2025 16:03:28 +0100
> Igor Mammedov <imammedo@redhat.com> escreveu:
> 
> > On Wed, 29 Jan 2025 09:04:08 +0100
> > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> >   
> > > The DSDT table will also be affected by such change.
> > > 
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>    
> > 
> > move it right before the patch that would actually make changes to tables (10/13)  
> 
> Table changes happens on two patches:
> 
> - patch 03/13: acpi/ghes: add a firmware file with HEST address

this one shouldn't affect bios tables test as it only checks ACPI and SMBIOS tables,
and hest addr file is not either.

Do you really see test failing on this patch?


> 	HEST table was added here
> 
> - patch 10/13: arm/virt: Wire up a GED error device for ACPI / GHES 
> 
> 	DSDT changes happen here.
> 
> If the idea is to avoid make check to fail between those two patches,
> we need either to split them on 4 patches (one before/one after each
> change) or do like I did on this series: whitelist before patch 3,
> update after patch 10.

It would be better to group patches that should change ACPI tables
close together so that a pair of whitelist/update could cover it.
However it depends on how many changes are there, i.e. acpi diff
should be digestible for a reader. So there is no hard border here,
just use common sense.

However when the whitelist is covers all series where only few patches
actually result in tables change, that miss-leads the reader since
whitelist patch basically tells 'watch out for changes since this moment'
and 'update' patch declares no more changes should happen.
The same applies to bisection, where closer the gap between
whitelist/update the better if the test case is the trigger.
No need to be fanatical and do it around each patch,
just make it observable (i.e. some small range of commits). 

  
> 
> Regards,
> Mauro
> 
> > 
> >   
> > > ---
> > >  tests/data/acpi/aarch64/virt/HEST           | 0
> > >  tests/qtest/bios-tables-test-allowed-diff.h | 2 ++
> > >  2 files changed, 2 insertions(+)
> > >  create mode 100644 tests/data/acpi/aarch64/virt/HEST
> > > 
> > > diff --git a/tests/data/acpi/aarch64/virt/HEST b/tests/data/acpi/aarch64/virt/HEST
> > > new file mode 100644
> > > index 000000000000..e69de29bb2d1
> > > diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> > > index dfb8523c8bf4..46298e38e7b8 100644
> > > --- a/tests/qtest/bios-tables-test-allowed-diff.h
> > > +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> > > @@ -1 +1,3 @@
> > >  /* List of comma-separated changed AML files to ignore */
> > > +"tests/data/acpi/aarch64/virt/HEST",
> > > +"tests/data/acpi/aarch64/virt/DSDT",    
> > 
> > the list in not complete so 'make check-qtest' still fails
> > [12/13] has complete list of changed files
> >   
> 
> 
> 
> Thanks,
> Mauro
>