Re: [PATCH-for-10.1 v3 6/9] qtest/bios-tables-test: Whitelist aarch64/virt 'its_off' variant blobs

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 10 Apr 2025 13:22:06 -0300
Gustavo Romero <gustavo.romero@linaro.org> wrote:

> Hi Igor,
> 
> On 4/10/25 03:50, Igor Mammedov wrote:
> > On Wed, 9 Apr 2025 12:49:36 -0300
> > Gustavo Romero <gustavo.romero@linaro.org> wrote:
> >   
> >> Hi Igor,
> >>
> >> On 4/9/25 11:05, Igor Mammedov wrote:  
> >>> On Fri, 4 Apr 2025 00:01:22 -0300
> >>> Gustavo Romero <gustavo.romero@linaro.org> wrote:
> >>>      
> >>>> Hi Phil,
> >>>>
> >>>> On 4/3/25 17:40, Philippe Mathieu-Daudé wrote:  
> >>>>> We are going to fix the test_acpi_aarch64_virt_tcg_its_off()
> >>>>> test. In preparation, copy the ACPI tables which will be
> >>>>> altered as 'its_off' variants, and whitelist them.
> >>>>>
> >>>>> Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>
> >>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >>>>> ---
> >>>>>     tests/qtest/bios-tables-test-allowed-diff.h |   3 +++
> >>>>>     tests/qtest/bios-tables-test.c              |   1 +
> >>>>>     tests/data/acpi/aarch64/virt/APIC.its_off   | Bin 0 -> 184 bytes
> >>>>>     tests/data/acpi/aarch64/virt/FACP.its_off   | Bin 0 -> 276 bytes
> >>>>>     tests/data/acpi/aarch64/virt/IORT.its_off   | Bin 0 -> 236 bytes
> >>>>>     5 files changed, 4 insertions(+)
> >>>>>     create mode 100644 tests/data/acpi/aarch64/virt/APIC.its_off
> >>>>>     create mode 100644 tests/data/acpi/aarch64/virt/FACP.its_off
> >>>>>     create mode 100644 tests/data/acpi/aarch64/virt/IORT.its_off
> >>>>>
> >>>>> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> >>>>> index dfb8523c8bf..3421dd5adf3 100644
> >>>>> --- a/tests/qtest/bios-tables-test-allowed-diff.h
> >>>>> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> >>>>> @@ -1 +1,4 @@
> >>>>>     /* List of comma-separated changed AML files to ignore */
> >>>>> +"tests/data/acpi/aarch64/virt/APIC.its_off",
> >>>>> +"tests/data/acpi/aarch64/virt/FACP.its_off",
> >>>>> +"tests/data/acpi/aarch64/virt/IORT.its_off",  
> >>>>
> >>>> I think your first approach is the correct one: you add the blobs
> >>>> when adding the new test, so they would go into patch 5/9 in this series,
> >>>> making the test pass without adding anything to bios-tables-test-allowed-diff.h.
> >>>> Then in this patch only add the APIC.its_off table to the bios-tables-test-allowed-diff.h
> >>>> since that's the table that changes when the fix is in place, as you did in:  
> >>>
> >>> if APIC.its_off is the only one that's changing, but FACP/IORT blobs are the same
> >>> as suffix-less blobs, one can omit copying FACP/IORT as test harness will fallback
> >>> to suffix-less blob if the one with suffix isn't found.  
> >>
> >> OK. Just clarifying and for the records, this is not the case for this series
> >>
> >>  
> >>> if blobs are different from defaults then create empty blobs and whitelist them in the same patch
> >>> then do your changes and then update blobs & wipeout withe list.  
> >>
> >> Thanks for confirming it. That's what I suggested to Phil in my first review and what
> >> I understood from the prescription in bios-tables-test.c.
> >>
> >> However, on second thoughts, for this particular series, isn't it better to have the following commit sequence instead:
> >>
> >> 1) Add the new test and the new blobs that make the test pass, i.e. APIC.suffix, FACP.suffix, and IORT.suffix (they are different than the default suffix-less blobs)  
> > 
> > blobs should be a separate commit (that way it's easier for maintainer to rebase them,
> > if they clash during merge with some other change.  
> 
> I see. What is a bit confusing here is that the series consists in
> one blob addition act (for the new test) and one blob update/removal act (after the fix).
> 
> 
> >> 2) Whitelist only the APIC.suffix since that's the table that will change with the fix
> >> 3) Add the fix (which changes the APIC table so a new APIC.suffix blob is needed and also stops generating the IORT table, so no more IORT.suffix blob is necessary)
> >> 4) Finally, update only the APIC.suffix blob and remove the IORT.suffix blob and wipe out the whitelist
> >>
> >> This way:
> >>
> >> A) It's clear that only ACPI blob changed with the fix, because there is no addition of a FACP.suffix blob in 4) (it remains the same)
> >> B) It's clear that the IORT table is removed with the fix and is not relevant anymore for the test  
> > 
> > I'd just mention it in commit log so  that later no one would wonder why we are adding and then removing tables
> > 
> > As for the rest of suggestions, it looks fine to me.  
> 
> Well, 2) won't make sense anymore since APIC.suffix would be already in the
> whitelist in the previous patch that added the empty blobs. Since there won't
> be a commit that adds _only_ the APIC.suffix to the whitelist, in preparation
> for the fix, this info is "lost" in the series, even tho it's possible to
> mention in the commit message.
> 
> Hence, what I think is not ideal from a maintainer's/reviewer's perspective,
> is that in one commit all the blobs are updated/removed at once, which is
> confusing because the fix did not touch the FACP table (for instance) and
> this table is updated with APIC and with the removal of IORT, altogether,
> in the last commit.
> 
> So, for this series, which adds new blobs and _also_ updates and removes some
> of them, how about the following organization:
> 
> - Patch 1     : Add the new test, add the empty blobs *.suffix files, whitelist such a blobs
> - Patch 2     : Update the blobs in Patch 1 with the ones that make the new test pass and remove them from the whitelist
> 
> - Patch 3     : Add the APIC.suffix blob to the whitelist (the table that changes due to the fix)
> - Patch 4 - n : Fix(es)

3 is not bynanry so it can be folded into 4 or be a separate patch (either way works for me)

> - Patch (n+1) : Update the APIC.suffix blob, remove IORT.suffix blob, and remove the APIC.suffix blob from the whitelist
>                * Add the APIC diff to the commit log
>                * Mention in the commit log that IORT.suffix is removed because IORT table is no long generated after the fix
> 
> This way: a) no commit fails the test and b) blobs are added/updated/removed in separate commits
> 
> What do you think?

LGTM

> 
> Cheers,
> Gustavo
>