Re: [PATCH 0/6] Resolve TYPE_PIIX3_XEN_DEVICE

[Bernhard Beschow]

Am 4. Januar 2023 08:18:59 UTC schrieb Chuck Zmudzinski <brchuckz@aol.com>:
>On 1/3/2023 8:38 AM, Bernhard Beschow wrote:
>>
>>
>> On Tue, Jan 3, 2023 at 2:17 PM Philippe Mathieu-Daudé <philmd@linaro.org> 
>> wrote:
>>
>>     Hi Chuck,
>>
>>     On 3/1/23 04:15, Chuck Zmudzinski wrote:
>>     > On 1/2/23 4:34 PM, Bernhard Beschow wrote:
>>     >> This series first renders TYPE_PIIX3_XEN_DEVICE redundant and finally 
>> removes
>>     >> it. The motivation is to 1/ decouple PIIX from Xen and 2/ to make Xen 
>> in the PC
>>     >> machine agnostic to the precise southbridge being used. 2/ will become
>>     >> particularily interesting once PIIX4 becomes usable in the PC 
>> machine, avoiding
>>     >> the "Frankenstein" use of PIIX4_ACPI in PIIX3.
>>     >>
>>     >> Testing done:
>>     >> None, because I don't know how to conduct this properly :(
>>     >>
>>     >> Based-on: <20221221170003.2929-1-shentey@gmail.com>
>>     >>            "[PATCH v4 00/30] Consolidate PIIX south bridges"
>>
>>     This series is based on a previous series:
>>     
>> https://lore.kernel.org/qemu-devel/20221221170003.2929-1-shentey@gmail.com/
>>     (which itself also is).
>>
>>     >> Bernhard Beschow (6):
>>     >>    include/hw/xen/xen: Make xen_piix3_set_irq() generic and rename it
>>     >>    hw/isa/piix: Reuse piix3_realize() in piix3_xen_realize()
>>     >>    hw/isa/piix: Wire up Xen PCI IRQ handling outside of PIIX3
>>     >>    hw/isa/piix: Avoid Xen-specific variant of piix_write_config()
>>     >>    hw/isa/piix: Resolve redundant k->config_write assignments
>>     >>    hw/isa/piix: Resolve redundant TYPE_PIIX3_XEN_DEVICE
>>     >>
>>     >>   hw/i386/pc_piix.c             | 34 ++++++++++++++++--
>>     >>   hw/i386/xen/xen-hvm.c         |  9 +++--
>>     >>   hw/isa/piix.c                 | 66 
>> +----------------------------------
>>     >
>>     > This file does not exist on the Qemu master branch.
>>     > But hw/isa/piix3.c and hw/isa/piix4.c do exist.
>>     >
>>     > I tried renaming it from piix.c to piix3.c in the patch, but
>>     > the patch set still does not apply cleanly on my tree.
>>     >
>>     > Is this patch set re-based against something other than
>>     > the current master Qemu branch?
>>     >
>>     > I have a system that is suitable for testing this patch set, but
>>     > I need guidance on how to apply it to the Qemu source tree.
>>
>>     You can ask Bernhard to publish a branch with the full work,
>>
>>
>> Hi Chuck,
>>
>> ... or just visit 
>> 20230102213504.14646-1-shentey@gmail.com/">https://patchew.org/QEMU/20230102213504.14646-1-shentey@gmail.com/ . There 
>> you'll find a git tag with a complete history and all instructions!
>>
>> Thanks for giving my series a test ride!
>>
>> Best regards,
>> Bernhard
>>
>>     or apply each series locally. I use the b4 tool for that:
>>     https://b4.docs.kernel.org/en/latest/installing.html
>>
>>     i.e.:
>>
>>     $ git checkout -b shentey_work
>>     $ b4 am 20221120150550.63059-1-shentey@gmail.com
>>     $ git am
>>     
>> ./v2_20221120_shentey_decouple_intx_to_lnkx_routing_from_south_bridges.mbx
>>     $ b4 am 20221221170003.2929-1-shentey@gmail.com
>>     $ git am
>>     
>> ./v4_20221221_shentey_this_series_consolidates_the_implementations_of_the_piix3_and_piix4_south.mbx
>>     $ b4 am 20230102213504.14646-1-shentey@gmail.com
>>     $ git am ./20230102_shentey_resolve_type_piix3_xen_device.mbx
>>
>>     Now the branch 'shentey_work' contains all the patches and you can test.
>>
>>     Regards,
>>
>>     Phil.
>>
>
>Hi Phil and Bernard,
>
>I tried applying these 3 patch series on top of the current qemu
>master branch.
>
>Unfortunately, I saw a regression, so I can't give a tested-by tag yet.

Hi Chuck,

Thanks for your valuable test report! I think the culprit may be commit 
https://lists.nongnu.org/archive/html/qemu-devel/2023-01/msg00102.html where 
now 128 PIRQs are considered rather than four. I'll revisit my series and will 
prepare a v2 in the next days. I think there is no need for further testing v1.

Thanks,
Bernhard

>
>Here are the details of the testing I did so far:
>
>Xen only needs one target, the i386-softmmu target which creates
>the qemu-system-i386 binary that Xen uses for its device model.
>That target compiled and linked with no problems with these 3
>patch series applied on top of qemu master. I didn't try building
>any other targets.
>
>My tests used the xenfv machine type with the xen platform
>pci device, which is ordinarily called a xen hvm guest with xen
>paravirtualized network and block device drivers. It is based on the
>i440fx machine type and so emulates piix3. I tested the xen
>hvm guests with two different configurations as described below.
>
>I tested both Linux and Windows guests, with mixed results. With the
>current Qemu master (commit 222059a0fccf4 without the 3 patch
>series applied), all tested guest configurations work normally for both
>Linux and Windows guests.
>
>With these 3 patch series applied on top of the qemu master branch,
>which tries to consolidate piix3 and piix4 and resolve the xen piix3
>device that my guests use, I unfortunately got a regression.
>
>The regression occurred with a configuration that uses the qemu
>bochs stdvga graphics device with a vnc display, and the qemu
>usb-tablet device to emulate the mouse and keyboard. After applying
>the 3 patch series, the emulated mouse is not working at all for Linux
>guests. It works for Windows guests, but the mouse pointer in the
>guest does not follow the mouse pointer in the vnc window as closely
>as it does without the 3 patch series. So this is the bad news of a
>regression introduced somewhere in these 3 patch series.
>
>The good news is that by using guests in a configuration that does
>not use the qemu usb-tablet device or the bochs stdvga device but
>instead uses a real passed through usb3 controller with a real usb
>mouse and a real usb keyboard connected, and also the real sound
>card and vga device passed through and a 1920x1080 HDMI monitor,
>there is no regression introduced by the 3 patch series and both Linux
>and Windows guests in that configuration work perfectly.
>
>My next test will be to test Bernhard's published git tag without
>trying to merge the 3 patch series into master and see if that also
>has the regression. I also will double check that I didn't make
>any mistakes in merging the 3 patch series by creating the shentey_work
>branch with b4 and git as Phil described and compare that to my
>working tree.
>
>I also will try testing only the first series, then the first series and the
>second series, to try to determine in which of the 3 series the regression
>is introduced.
>
>Best regards,
>
>Chuck