On Wed, 28 May 2025 12:04:26 -0300 Gustavo Romero <gustavo.rom...@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.rom...@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.au...@redhat.com> wrote: > >>> > >>>> From: Gustavo Romero <gustavo.rom...@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.rom...@linaro.org> > >>>> Signed-off-by: Eric Auger <eric.au...@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); > >>>> } > >>> > >> > > >
