On Tue, 17 Jan 2023, Chuck Zmudzinski wrote: > On 1/17/2023 6:04 AM, Igor Mammedov wrote: > > On Mon, 16 Jan 2023 13:00:53 -0500 > > Chuck Zmudzinski <brchu...@netscape.net> wrote: > > > > > On 1/16/23 10:33, Igor Mammedov wrote: > > > > On Fri, 13 Jan 2023 16:31:26 -0500 > > > > Chuck Zmudzinski <brchu...@aol.com> wrote: > > > > > > > >> On 1/13/23 4:33 AM, Igor Mammedov wrote: > > > >> > On Thu, 12 Jan 2023 23:14:26 -0500 > > > >> > Chuck Zmudzinski <brchu...@aol.com> wrote: > > > >> > > > > >> >> On 1/12/23 6:03 PM, Michael S. Tsirkin wrote: > > > >> >> > On Thu, Jan 12, 2023 at 10:55:25PM +0000, Bernhard Beschow wrote: > > > >> >> > > > > >> >> >> I think the change Michael suggests is very minimalistic: Move > > > >> >> >> the if > > > >> >> >> condition around xen_igd_reserve_slot() into the function itself > > > >> >> >> and > > > >> >> >> always call it there unconditionally -- basically turning three > > > >> >> >> lines > > > >> >> >> into one. Since xen_igd_reserve_slot() seems very problem > > > >> >> >> specific, > > > >> >> >> Michael further suggests to rename it to something more general. > > > >> >> >> All > > > >> >> >> in all no big changes required. > > > >> >> > > > > >> >> > yes, exactly. > > > >> >> > > > > >> >> > > > >> >> OK, got it. I can do that along with the other suggestions. > > > >> > > > > >> > have you considered instead of reservation, putting a slot check in > > > >> > device model > > > >> > and if it's intel igd being passed through, fail at realize time if > > > >> > it can't take > > > >> > required slot (with a error directing user to fix command line)? > > > >> > > > >> Yes, but the core pci code currently already fails at realize time > > > >> with a useful error message if the user tries to use slot 2 for the > > > >> igd, because of the xen platform device which has slot 2. The user > > > >> can fix this without patching qemu, but having the user fix it on > > > >> the command line is not the best way to solve the problem, primarily > > > >> because the user would need to hotplug the xen platform device via a > > > >> command line option instead of having the xen platform device added by > > > >> pc_xen_hvm_init functions almost immediately after creating the pci > > > >> bus, and that delay in adding the xen platform device degrades > > > >> startup performance of the guest. > > > >> > > > >> > That could be less complicated than dealing with slot reservations > > > >> > at the cost of > > > >> > being less convenient. > > > >> > > > >> And also a cost of reduced startup performance > > > > > > > > Could you clarify how it affects performance (and how much). > > > > (as I see, setup done at board_init time is roughly the same > > > > as with '-device foo' CLI options, modulo time needed to parse > > > > options which should be negligible. and both ways are done before > > > > guest runs) > > > > > > I preface my answer by saying there is a v9, but you don't > > > need to look at that. I will answer all your questions here. > > > > > > I am going by what I observe on the main HDMI display with the > > > different approaches. With the approach of not patching Qemu > > > to fix this, which requires adding the Xen platform device a > > > little later, the length of time it takes to fully load the > > > guest is increased. I also noticed with Linux guests that use > > > the grub bootoader, the grub vga driver cannot display the > > > grub boot menu at the native resolution of the display, which > > > in the tested case is 1920x1080, when the Xen platform device > > > is added via a command line option instead of by the > > > pc_xen_hvm_init_pci fucntion in pc_piix.c, but with this patch > > > to Qemu, the grub menu is displayed at the full, 1920x1080 > > > native resolution of the display. Once the guest fully loads, > > > there is no noticeable difference in performance. It is mainly > > > a degradation in startup performance, not performance once > > > the guest OS is fully loaded. > > > > Looking at igd-assign.txt, it recommends to add IGD using '-device' CLI > > option, and actually drop at least graphics defaults explicitly. > > So it is expected to work fine even when IGD is constructed with > > '-device'. > > > > Could you provide full CLI current xen starts QEMU with and then > > a CLI you used (with explicit -device for IGD) that leads > > to reduced performance? > > > > CCing vfio folks who might have an idea what could be wrong based > > on vfio experience. > > Actually, the igd is not added with an explicit -device option using Xen: > > 1573 ? Ssl 0:42 /usr/bin/qemu-system-i386 -xen-domid 1 > -no-shutdown -chardev > socket,id=libxl-cmd,path=/var/run/xen/qmp-libxl-1,server,nowait -mon > chardev=libxl-cmd,mode=control -chardev > socket,id=libxenstat-cmd,path=/var/run/xen/qmp-libxenstat-1,server,nowait > -mon chardev=libxenstat-cmd,mode=control -nodefaults -no-user-config -name > windows -vnc none -display none -serial pty -boot order=c -smp 4,maxcpus=4 > -net none -machine xenfv,max-ram-below-4g=3758096384,igd-passthru=on -m 6144 > -drive file=/dev/loop0,if=ide,index=0,media=disk,format=raw,cache=writeback > -drive > file=/dev/disk/by-uuid/A44AA4984AA468AE,if=ide,index=1,media=disk,format=raw,cache=writeback > > I think it is added by xl (libxl management tool) when the guest is created > using the qmp-libxl socket that appears on the command line, but I am not 100 > percent sure. So, with libxl, the command line alone does not tell the whole > story. The xl.cfg file has a line like this to define the pci devices passed > through, > and in qemu they are type XEN_PT devices, not VFIO devices: > > pci = [ '00:1b.0','00:14.0','00:02.0@02' ] > > This means three host pci devices are passed through, the ones on the > host at slot 1b.0, 14.0, and 02.0. Of course the device at 02 is the igd. > The @02 means libxl is requesting slot 2 in the guest for the igd, the > other 2 devices are just auto assigned a slot by Qemu. Qemu cannot > assign the igd to slot 2 for xenfv machines without a patch that prevents > the Xen platform device from grabbing slot 2. That is what this patch > accomplishes.
In principle I think this change is OK. Apologies that this patch is at v9 and none of the Xen/QEMU maintainers took a look at it yet. I'll try to look at it today.
