| Message ID | 20240814115736.1580337-1-ribalda@chromium.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | hw/i386/acpi: Pre-compute the _PRT table | expand |
On Wed, 14 Aug 2024 11:56:08 +0000 Ricardo Ribalda <ribalda@chromium.org> wrote: > Today for x86 the _PRT() table is computed in runtime. > Under some configurations, computing the _PRT table can take more than > 30 seconds and the ACPI timeout is violated. > > This patchset modifies _PRT() to return a pre-computed table. To be sure we are not breaking anything boot tested it with rhel6.7/9.0, winxp, ws2022 > Changelog v3->v4 Thanks Richard: > - Make link_name struct static > > Changelog v3->v4 Thanks Igor: > - Add missing files to tests/qtest/bios-tables-test-allowed-diff.h > > Changelog v2->v3 Thanks Michael: > - Code style > - Add cover letter > > Ricardo Ribalda (3): > tests/acpi: pc: allow DSDT acpi table changes > hw/i386/acpi-build: Return a pre-computed _PRT table > tests/acpi: pc: update golden masters for DSDT > > hw/i386/acpi-build.c | 120 +++++---------------------- > tests/data/acpi/x86/pc/DSDT | Bin 6830 -> 8527 bytes > tests/data/acpi/x86/pc/DSDT.acpierst | Bin 6741 -> 8438 bytes > tests/data/acpi/x86/pc/DSDT.acpihmat | Bin 8155 -> 9852 bytes > tests/data/acpi/x86/pc/DSDT.bridge | Bin 13701 -> 15398 bytes > tests/data/acpi/x86/pc/DSDT.cphp | Bin 7294 -> 8991 bytes > tests/data/acpi/x86/pc/DSDT.dimmpxm | Bin 8484 -> 10181 bytes > tests/data/acpi/x86/pc/DSDT.hpbridge | Bin 6781 -> 8478 bytes > tests/data/acpi/x86/pc/DSDT.hpbrroot | Bin 3337 -> 5034 bytes > tests/data/acpi/x86/pc/DSDT.ipmikcs | Bin 6902 -> 8599 bytes > tests/data/acpi/x86/pc/DSDT.memhp | Bin 8189 -> 9886 bytes > tests/data/acpi/x86/pc/DSDT.nohpet | Bin 6688 -> 8385 bytes > tests/data/acpi/x86/pc/DSDT.numamem | Bin 6836 -> 8533 bytes > tests/data/acpi/x86/pc/DSDT.roothp | Bin 10623 -> 12320 bytes > tests/data/acpi/x86/q35/DSDT.cxl | Bin 9714 -> 13148 bytes > tests/data/acpi/x86/q35/DSDT.viot | Bin 9464 -> 14615 bytes > 16 files changed, 22 insertions(+), 98 deletions(-) >
On 21/08/2024 15:45, Igor Mammedov wrote: > On Wed, 14 Aug 2024 11:56:08 +0000 > Ricardo Ribalda <ribalda@chromium.org> wrote: > >> Today for x86 the _PRT() table is computed in runtime. >> Under some configurations, computing the _PRT table can take more than >> 30 seconds and the ACPI timeout is violated. >> >> This patchset modifies _PRT() to return a pre-computed table. > > To be sure we are not breaking anything boot tested it with > rhel6.7/9.0, winxp, ws2022 Hi Ricardo/Igor, Unfortunately it seems that this series (and in particular commit 99cb2c6c7b ("hw/i386/acpi-build: Return a pre-computed _PRT table") breaks my WinXP ISO boot test case using the dc390/am53C974 SCSI device. Current master hangs when booting WinXP from a dc390 SCSI CD just after loading the kernel device drivers and displaying "Setup is starting Windows" instead of proceeding to the "Welcome to Setup" screen. Note that there is a separate timeout issue caused by a bug in SeaBIOS which is fixed in the SeaBIOS repository as commit 2424e4c0 ("esp-scsi: indicate acceptance of MESSAGE IN phase data"). As the QEMU SeaBIOS binaries haven't yet been updated to contain this fix, I've uploaded a pre-built bios.bin to https://www.ilande.co.uk/tmp/qemu/bios.bin to help reproduce the issue. Once the above file is downloaded the issue can be reproduced with the command line below: ./qemu-system-x86_64 \ -m 1G \ -device dc390,id=scsi0 \ -device scsi-cd,drive=drive0,bus=scsi0.0,channel=0,scsi-id=6,lun=0 \ -drive file=winxp.iso,if=none,id=drive0 \ -vga cirrus \ -boot d \ -trace 'esp*' \ -bios bios.bin With the ESP tracing enabled it is possible to see that WinXP appears to get stuck in a loop trying to send a SCSI command followed by a "Bus Reset". Reverting 99cb2c6c7b allows the WinXP ISO to boot to the "Welcome to Setup" screen as before. ATB, Mark. >> Changelog v3->v4 Thanks Richard: >> - Make link_name struct static >> >> Changelog v3->v4 Thanks Igor: >> - Add missing files to tests/qtest/bios-tables-test-allowed-diff.h >> >> Changelog v2->v3 Thanks Michael: >> - Code style >> - Add cover letter >> >> Ricardo Ribalda (3): >> tests/acpi: pc: allow DSDT acpi table changes >> hw/i386/acpi-build: Return a pre-computed _PRT table >> tests/acpi: pc: update golden masters for DSDT >> >> hw/i386/acpi-build.c | 120 +++++---------------------- >> tests/data/acpi/x86/pc/DSDT | Bin 6830 -> 8527 bytes >> tests/data/acpi/x86/pc/DSDT.acpierst | Bin 6741 -> 8438 bytes >> tests/data/acpi/x86/pc/DSDT.acpihmat | Bin 8155 -> 9852 bytes >> tests/data/acpi/x86/pc/DSDT.bridge | Bin 13701 -> 15398 bytes >> tests/data/acpi/x86/pc/DSDT.cphp | Bin 7294 -> 8991 bytes >> tests/data/acpi/x86/pc/DSDT.dimmpxm | Bin 8484 -> 10181 bytes >> tests/data/acpi/x86/pc/DSDT.hpbridge | Bin 6781 -> 8478 bytes >> tests/data/acpi/x86/pc/DSDT.hpbrroot | Bin 3337 -> 5034 bytes >> tests/data/acpi/x86/pc/DSDT.ipmikcs | Bin 6902 -> 8599 bytes >> tests/data/acpi/x86/pc/DSDT.memhp | Bin 8189 -> 9886 bytes >> tests/data/acpi/x86/pc/DSDT.nohpet | Bin 6688 -> 8385 bytes >> tests/data/acpi/x86/pc/DSDT.numamem | Bin 6836 -> 8533 bytes >> tests/data/acpi/x86/pc/DSDT.roothp | Bin 10623 -> 12320 bytes >> tests/data/acpi/x86/q35/DSDT.cxl | Bin 9714 -> 13148 bytes >> tests/data/acpi/x86/q35/DSDT.viot | Bin 9464 -> 14615 bytes >> 16 files changed, 22 insertions(+), 98 deletions(-)
Hi Mark On Sun, 22 Sept 2024 at 13:57, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > > On 21/08/2024 15:45, Igor Mammedov wrote: > > > On Wed, 14 Aug 2024 11:56:08 +0000 > > Ricardo Ribalda <ribalda@chromium.org> wrote: > > > >> Today for x86 the _PRT() table is computed in runtime. > >> Under some configurations, computing the _PRT table can take more than > >> 30 seconds and the ACPI timeout is violated. > >> > >> This patchset modifies _PRT() to return a pre-computed table. > > > > To be sure we are not breaking anything boot tested it with > > rhel6.7/9.0, winxp, ws2022 > > Hi Ricardo/Igor, > > Unfortunately it seems that this series (and in particular commit 99cb2c6c7b > ("hw/i386/acpi-build: Return a pre-computed _PRT table") breaks my WinXP ISO boot > test case using the dc390/am53C974 SCSI device. > > Current master hangs when booting WinXP from a dc390 SCSI CD just after loading the > kernel device drivers and displaying "Setup is starting Windows" instead of > proceeding to the "Welcome to Setup" screen. > > Note that there is a separate timeout issue caused by a bug in SeaBIOS which is fixed > in the SeaBIOS repository as commit 2424e4c0 ("esp-scsi: indicate acceptance of > MESSAGE IN phase data"). As the QEMU SeaBIOS binaries haven't yet been updated to > contain this fix, I've uploaded a pre-built bios.bin to > https://www.ilande.co.uk/tmp/qemu/bios.bin to help reproduce the issue. > > Once the above file is downloaded the issue can be reproduced with the command line > below: > > ./qemu-system-x86_64 \ > -m 1G \ > -device dc390,id=scsi0 \ > -device scsi-cd,drive=drive0,bus=scsi0.0,channel=0,scsi-id=6,lun=0 \ > -drive file=winxp.iso,if=none,id=drive0 \ > -vga cirrus \ > -boot d \ > -trace 'esp*' \ > -bios bios.bin > > With the ESP tracing enabled it is possible to see that WinXP appears to get stuck in > a loop trying to send a SCSI command followed by a "Bus Reset". Reverting 99cb2c6c7b > allows the WinXP ISO to boot to the "Welcome to Setup" screen as before. > > > ATB, > > Mark. Thanks for the detailed report, and sorry for breaking your testcase. I managed to reproduce locally. Could you check if this patch fixes your issue and the rest of your testcases? If so, I will prepare a proper patchset. Thanks! diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 4967aa7459..e7db51afba 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -741,7 +741,7 @@ static Aml *build_prt(bool is_pci0_prt) int pin; method = aml_method("_PRT", 0, AML_NOTSERIALIZED); - rt_pkg = aml_varpackage(nroutes); + rt_pkg = aml_package(nroutes); for (pin = 0; pin < nroutes; pin++) { Aml *pkg = aml_package(4); > > >> Changelog v3->v4 Thanks Richard: > >> - Make link_name struct static > >> > >> Changelog v3->v4 Thanks Igor: > >> - Add missing files to tests/qtest/bios-tables-test-allowed-diff.h > >> > >> Changelog v2->v3 Thanks Michael: > >> - Code style > >> - Add cover letter > >> > >> Ricardo Ribalda (3): > >> tests/acpi: pc: allow DSDT acpi table changes > >> hw/i386/acpi-build: Return a pre-computed _PRT table > >> tests/acpi: pc: update golden masters for DSDT > >> > >> hw/i386/acpi-build.c | 120 +++++---------------------- > >> tests/data/acpi/x86/pc/DSDT | Bin 6830 -> 8527 bytes > >> tests/data/acpi/x86/pc/DSDT.acpierst | Bin 6741 -> 8438 bytes > >> tests/data/acpi/x86/pc/DSDT.acpihmat | Bin 8155 -> 9852 bytes > >> tests/data/acpi/x86/pc/DSDT.bridge | Bin 13701 -> 15398 bytes > >> tests/data/acpi/x86/pc/DSDT.cphp | Bin 7294 -> 8991 bytes > >> tests/data/acpi/x86/pc/DSDT.dimmpxm | Bin 8484 -> 10181 bytes > >> tests/data/acpi/x86/pc/DSDT.hpbridge | Bin 6781 -> 8478 bytes > >> tests/data/acpi/x86/pc/DSDT.hpbrroot | Bin 3337 -> 5034 bytes > >> tests/data/acpi/x86/pc/DSDT.ipmikcs | Bin 6902 -> 8599 bytes > >> tests/data/acpi/x86/pc/DSDT.memhp | Bin 8189 -> 9886 bytes > >> tests/data/acpi/x86/pc/DSDT.nohpet | Bin 6688 -> 8385 bytes > >> tests/data/acpi/x86/pc/DSDT.numamem | Bin 6836 -> 8533 bytes > >> tests/data/acpi/x86/pc/DSDT.roothp | Bin 10623 -> 12320 bytes > >> tests/data/acpi/x86/q35/DSDT.cxl | Bin 9714 -> 13148 bytes > >> tests/data/acpi/x86/q35/DSDT.viot | Bin 9464 -> 14615 bytes > >> 16 files changed, 22 insertions(+), 98 deletions(-) >
On 22/09/2024 14:11, Ricardo Ribalda wrote: > Hi Mark > > On Sun, 22 Sept 2024 at 13:57, Mark Cave-Ayland > <mark.cave-ayland@ilande.co.uk> wrote: >> >> On 21/08/2024 15:45, Igor Mammedov wrote: >> >>> On Wed, 14 Aug 2024 11:56:08 +0000 >>> Ricardo Ribalda <ribalda@chromium.org> wrote: >>> >>>> Today for x86 the _PRT() table is computed in runtime. >>>> Under some configurations, computing the _PRT table can take more than >>>> 30 seconds and the ACPI timeout is violated. >>>> >>>> This patchset modifies _PRT() to return a pre-computed table. >>> >>> To be sure we are not breaking anything boot tested it with >>> rhel6.7/9.0, winxp, ws2022 >> >> Hi Ricardo/Igor, >> >> Unfortunately it seems that this series (and in particular commit 99cb2c6c7b >> ("hw/i386/acpi-build: Return a pre-computed _PRT table") breaks my WinXP ISO boot >> test case using the dc390/am53C974 SCSI device. >> >> Current master hangs when booting WinXP from a dc390 SCSI CD just after loading the >> kernel device drivers and displaying "Setup is starting Windows" instead of >> proceeding to the "Welcome to Setup" screen. >> >> Note that there is a separate timeout issue caused by a bug in SeaBIOS which is fixed >> in the SeaBIOS repository as commit 2424e4c0 ("esp-scsi: indicate acceptance of >> MESSAGE IN phase data"). As the QEMU SeaBIOS binaries haven't yet been updated to >> contain this fix, I've uploaded a pre-built bios.bin to >> https://www.ilande.co.uk/tmp/qemu/bios.bin to help reproduce the issue. >> >> Once the above file is downloaded the issue can be reproduced with the command line >> below: >> >> ./qemu-system-x86_64 \ >> -m 1G \ >> -device dc390,id=scsi0 \ >> -device scsi-cd,drive=drive0,bus=scsi0.0,channel=0,scsi-id=6,lun=0 \ >> -drive file=winxp.iso,if=none,id=drive0 \ >> -vga cirrus \ >> -boot d \ >> -trace 'esp*' \ >> -bios bios.bin >> >> With the ESP tracing enabled it is possible to see that WinXP appears to get stuck in >> a loop trying to send a SCSI command followed by a "Bus Reset". Reverting 99cb2c6c7b >> allows the WinXP ISO to boot to the "Welcome to Setup" screen as before. >> >> >> ATB, >> >> Mark. > > Thanks for the detailed report, and sorry for breaking your testcase. > > I managed to reproduce locally. > > Could you check if this patch fixes your issue and the rest of your testcases? > If so, I will prepare a proper patchset. > > Thanks! > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index 4967aa7459..e7db51afba 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -741,7 +741,7 @@ static Aml *build_prt(bool is_pci0_prt) > int pin; > > method = aml_method("_PRT", 0, AML_NOTSERIALIZED); > - rt_pkg = aml_varpackage(nroutes); > + rt_pkg = aml_package(nroutes); > > for (pin = 0; pin < nroutes; pin++) { > Aml *pkg = aml_package(4); Hi Ricardo, I can confirm that the above patch fixes my particular test case here - thanks so much for the quick reply! ATB, Mark. >>>> Changelog v3->v4 Thanks Richard: >>>> - Make link_name struct static >>>> >>>> Changelog v3->v4 Thanks Igor: >>>> - Add missing files to tests/qtest/bios-tables-test-allowed-diff.h >>>> >>>> Changelog v2->v3 Thanks Michael: >>>> - Code style >>>> - Add cover letter >>>> >>>> Ricardo Ribalda (3): >>>> tests/acpi: pc: allow DSDT acpi table changes >>>> hw/i386/acpi-build: Return a pre-computed _PRT table >>>> tests/acpi: pc: update golden masters for DSDT >>>> >>>> hw/i386/acpi-build.c | 120 +++++---------------------- >>>> tests/data/acpi/x86/pc/DSDT | Bin 6830 -> 8527 bytes >>>> tests/data/acpi/x86/pc/DSDT.acpierst | Bin 6741 -> 8438 bytes >>>> tests/data/acpi/x86/pc/DSDT.acpihmat | Bin 8155 -> 9852 bytes >>>> tests/data/acpi/x86/pc/DSDT.bridge | Bin 13701 -> 15398 bytes >>>> tests/data/acpi/x86/pc/DSDT.cphp | Bin 7294 -> 8991 bytes >>>> tests/data/acpi/x86/pc/DSDT.dimmpxm | Bin 8484 -> 10181 bytes >>>> tests/data/acpi/x86/pc/DSDT.hpbridge | Bin 6781 -> 8478 bytes >>>> tests/data/acpi/x86/pc/DSDT.hpbrroot | Bin 3337 -> 5034 bytes >>>> tests/data/acpi/x86/pc/DSDT.ipmikcs | Bin 6902 -> 8599 bytes >>>> tests/data/acpi/x86/pc/DSDT.memhp | Bin 8189 -> 9886 bytes >>>> tests/data/acpi/x86/pc/DSDT.nohpet | Bin 6688 -> 8385 bytes >>>> tests/data/acpi/x86/pc/DSDT.numamem | Bin 6836 -> 8533 bytes >>>> tests/data/acpi/x86/pc/DSDT.roothp | Bin 10623 -> 12320 bytes >>>> tests/data/acpi/x86/q35/DSDT.cxl | Bin 9714 -> 13148 bytes >>>> tests/data/acpi/x86/q35/DSDT.viot | Bin 9464 -> 14615 bytes >>>> 16 files changed, 22 insertions(+), 98 deletions(-)