Re: [PATCH v2 24/25] tests/qtest/bios-tables-test: Keep ACPI PCI hotplug off

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 28 May 2025 12:04:26 -0300
Gustavo Romero <gustavo.romero@linaro.org> wrote:

> Hi Igor,
> 
> On 5/28/25 10:02, Igor Mammedov wrote:
> > On Wed, 28 May 2025 09:41:15 -0300
> > Gustavo Romero <gustavo.romero@linaro.org> wrote:
> >   
> >> Hi Igor,
> >>
> >> On 5/28/25 06:38, Igor Mammedov wrote:  
> >>> On Tue, 27 May 2025 09:40:26 +0200
> >>> Eric Auger <eric.auger@redhat.com> wrote:
> >>>      
> >>>> From: Gustavo Romero <gustavo.romero@linaro.org>
> >>>>
> >>>> ACPI PCI hotplug is now turned on by default so we need to change the
> >>>> existing tests to keep it off. However, even setting the ACPI PCI
> >>>> hotplug off in the existing tests, there will be changes in the ACPI
> >>>> tables because the _OSC method was modified, hence in the next patch of
> >>>> this series the blobs are updated accordingly.
> >>>>
> >>>> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> >>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>  
> >>>
> >>> it would be better to test whatever default we end up with.
> >>> (like x86)  
> >>
> >> hmm maybe there is a confusion here, Igor. We are actually planning what you  
> > 
> > perhaps, see my reply to Eric about my expectations wrt tests.  
> 
> Yip, I read it before my reply here.
> 
> 
> > (i.e. default tests shouldn't have any explicit CLI options,
> > instead it should follow whitelist blobs/set new default patch/update blobs pattern)  
> 
> I see. I agree with that. But this patch is not about the new test. The new test is
> _not_ in this series. Patches 8/25, 10/25, and 24/25 are _not_ about the new test but
> about adapting the _legacy tests_ (native acpi) to the situation when ACPI HP becomes
> the default, because this series makes acpi-pcihp=on the default, hence the CLI option
> "acpi-pcihp=off" added to them. An update to the blobs are also necessary because of the
> change in _OSC method, even when acpi-pcihp=off.
> 
> 
> >> said. This patch and the other two in this series related to the bios-tables-test
> >> (i.e., patches 8/25 and 10/25) are for actually making the current (legacy) test pass,
> >> since the new default as per this series will be acpi-pcihp=on. That's why here we're
> >> adapting the current test here to have acpi-pcihp=off.
> >>
> >> The new test that will test for acpi-pcihp=on (the new default) is not in this series
> >> and we decided to merge it separate. It's in the patch 4/5 and 5/5 of the follow series:  
> 
> We're doing the "blobs/set new default patch/update blobs pattern" in the new test, which
> we can merge later, once this series is merged, no? The step "set new default" then will
> not be necessary because the new test will be merged separate, after this series, so when
> acpi-pcihp=on is already the default.
> 
> Please note that although we're using acpi-pcihp=on in the new test, it's not necessary,
> we can dropped this option, making it implicit as you say, and it will work. This is the
> new test:
> 
> >> https://mail.gnu.org/archive/html/qemu-devel/2025-05/msg05828.html 4/5
> >> https://mail.gnu.org/archive/html/qemu-devel/2025-05/msg05827.html 5/5  
> 
> 
> Thus, there are to "acts" of modifying the bios-tables-test:
> 
> 1) Adapt the current tests to when acpi-pcihp=on becomes the default (hence the addition
>     to them of "acpi-pcihp=off".


that's what I disagree with.

1) Instead adapting majority of tests to legacy before switching defaults,
just do whitelist/modify default/update so all of tests run with new default.

and then

> There is also the need to update the blobs, but it's because
>     of the _OSC method change in DSDT table, which will change anyways, even with "acpi-pcihp=off¨,
>     hence the need for patch 10/25 in this series. This is _done is this series_.


 
> 2) Add a new test for testing the default (i.e. acpi-pcihp-on). It follows what you're
>     saying above: "follow whitelist blobs/set new default patch/update blobs pattern",
>     because we can drop the acpi-pcihp-on option from the CLI in this test without any
>     prejudice to test. While the step "set new default patch" was actually done in 1).

2) add a separate test case for native pcie hoplug (preferably within this series)

3) even better would be to add #2 before #1 (right after 10/25),
this way will guarantee that old native hotplug tables stay
the same regardless of followup patches that add ACPI pcihp aml.


> Cheers,
> Gustavo
> 
> >> https://mail.gnu.org/archive/html/qemu-devel/2025-05/msg05828.html 4/5
> >> https://mail.gnu.org/archive/html/qemu-devel/2025-05/msg05827.html 5/5
> >>
> >>
> >> Cheers,
> >> Gustavo
> >>  
> >>>>
> >>>> ---
> >>>>
> >>>> [Eric] also added acpi-pcihp=off to test_acpi_aarch64_virt_tcg_numamem
> >>>> ---
> >>>>    tests/qtest/bios-tables-test.c | 13 +++++++++----
> >>>>    1 file changed, 9 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> >>>> index 0a333ec435..6379dba714 100644
> >>>> --- a/tests/qtest/bios-tables-test.c
> >>>> +++ b/tests/qtest/bios-tables-test.c
> >>>> @@ -1626,7 +1626,7 @@ static void test_acpi_aarch64_virt_tcg_memhp(void)
> >>>>        };
> >>>>    
> >>>>        data.variant = ".memhp";
> >>>> -    test_acpi_one(" -machine nvdimm=on"
> >>>> +    test_acpi_one(" -machine nvdimm=on,acpi-pcihp=off"
> >>>>                      " -cpu cortex-a57"
> >>>>                      " -m 256M,slots=3,maxmem=1G"
> >>>>                      " -object memory-backend-ram,id=ram0,size=128M"
> >>>> @@ -1747,7 +1747,8 @@ static void test_acpi_aarch64_virt_tcg_numamem(void)
> >>>>        };
> >>>>    
> >>>>        data.variant = ".numamem";
> >>>> -    test_acpi_one(" -cpu cortex-a57"
> >>>> +    test_acpi_one(" -machine acpi-pcihp=off"
> >>>> +                  " -cpu cortex-a57"
> >>>>                      " -object memory-backend-ram,id=ram0,size=128M"
> >>>>                      " -numa node,memdev=ram0",
> >>>>                      &data);
> >>>> @@ -1775,7 +1776,8 @@ static void test_acpi_aarch64_virt_tcg_pxb(void)
> >>>>         * to solve the conflicts.
> >>>>         */
> >>>>        data.variant = ".pxb";
> >>>> -    test_acpi_one(" -device pcie-root-port,chassis=1,id=pci.1"
> >>>> +    test_acpi_one(" -machine acpi-pcihp=off"
> >>>> +                  " -device pcie-root-port,chassis=1,id=pci.1"
> >>>>                      " -device virtio-scsi-pci,id=scsi0,bus=pci.1"
> >>>>                      " -drive file="
> >>>>                      "tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2,"
> >>>> @@ -1846,7 +1848,7 @@ static void test_acpi_aarch64_virt_tcg_acpi_hmat(void)
> >>>>    
> >>>>        data.variant = ".acpihmatvirt";
> >>>>    
> >>>> -    test_acpi_one(" -machine hmat=on"
> >>>> +    test_acpi_one(" -machine hmat=on,acpi-pcihp=off"
> >>>>                      " -cpu cortex-a57"
> >>>>                      " -smp 4,sockets=2"
> >>>>                      " -m 384M"
> >>>> @@ -2123,6 +2125,7 @@ static void test_acpi_aarch64_virt_tcg(void)
> >>>>        data.smbios_cpu_max_speed = 2900;
> >>>>        data.smbios_cpu_curr_speed = 2700;
> >>>>        test_acpi_one("-cpu cortex-a57 "
> >>>> +                  "-machine acpi-pcihp=off "
> >>>>                      "-smbios type=4,max-speed=2900,current-speed=2700", &data);
> >>>>        free_test_data(&data);
> >>>>    }
> >>>> @@ -2142,6 +2145,7 @@ static void test_acpi_aarch64_virt_tcg_topology(void)
> >>>>        };
> >>>>    
> >>>>        test_acpi_one("-cpu cortex-a57 "
> >>>> +                  "-machine acpi-pcihp=off "
> >>>>                      "-smp sockets=1,clusters=2,cores=2,threads=2", &data);
> >>>>        free_test_data(&data);
> >>>>    }
> >>>> @@ -2227,6 +2231,7 @@ static void test_acpi_aarch64_virt_viot(void)
> >>>>        };
> >>>>    
> >>>>        test_acpi_one("-cpu cortex-a57 "
> >>>> +                  "-machine acpi-pcihp=off "
> >>>>                      "-device virtio-iommu-pci", &data);
> >>>>        free_test_data(&data);
> >>>>    }  
> >>>      
> >>  
> >   
>