Re: [PATCH v5 0/3] hw/i386/acpi: Pre-compute the _PRT table

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

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(-)