Re: [PATCH v8] xen/pt: reserve PCI slot 2 for Intel igd-passthru

[Chuck Zmudzinski <brchuckz@xxxxxxx>]

On 1/20/2023 11:01 AM, Igor Mammedov wrote:
> On Tue, 17 Jan 2023 09:50:23 -0500
> Chuck Zmudzinski <brchuckz@xxxxxxx> wrote:
>
> > On 1/17/2023 5:35 AM, Igor Mammedov wrote:
> > > On Mon, 16 Jan 2023 13:00:53 -0500
> > > Chuck Zmudzinski <brchuckz@xxxxxxxxxxxx> wrote:
> > >  
> > > > On 1/16/23 10:33, Igor Mammedov wrote:  
> > > > > On Fri, 13 Jan 2023 16:31:26 -0500
> > > > > Chuck Zmudzinski <brchuckz@xxxxxxx> wrote:
> > > > >     
> > > > >> On 1/13/23 4:33 AM, Igor Mammedov wrote:    
> > > > >> > On Thu, 12 Jan 2023 23:14:26 -0500
> > > > >> > Chuck Zmudzinski <brchuckz@xxxxxxx> 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.  
> > > above hints on presence of bug[s] in igd-passthru implementation,
> > > and this patch effectively hides problem instead of trying to figure
> > > out what's wrong and fixing it.
> > >  
> > 
> > I agree that the with the current Qemu there is a bug in the igd-passthru
> > implementation. My proposed patch fixes it. So why not support the
> > proposed patch with a Reviewed-by tag?
> I see the patch as workaround (it might be a reasonable one) and
> it's upto xen maintainers to give Reviewed-by/accept it.
>
> Though I'd add to commit message something about performance issues
> you are experiencing when trying to passthrough IGD manually

No, the device that needs to be passed through manually with the
workaround is the Xen platform device, and the workaround (as I
understand it) is the patch to libxl, not this patch to Qemu. The igd
is passed through the same way whether or not Qemu or libxl is
patched, with these patches I have proposed, and IIUC that is by
using QMP and the Qemu device listener.

> as one of justifications for workaround (it might push Xen folks
> to investigate the issue and it won't be lost/forgotten on mail-list).
>

I could add that on the next iteration.

As far as Xen folks investigating further, that would be welcome.
I think the best way would be for the pc_xen_hvm_init functions
to add the igd at slot 2 and the xen platform device at slot 3
when igd-passthru=on is set on the command line. But I don't
know if it is possible for the pc_xen_hvm_init functions to
attach a type XEN_PT, which is the device type of the igd when
using xen, without changing the interface between libxl and
Qemu. My patches fix this without changing that interface,
but the Qemu patch does it in a way that achieves better
startup performance that the patch to libxl because of the
fact that the xen platform device can be added sooner
during guest creation by patching Qemu than by patching
libxl.

Currently, IIUC, the XEN_PT devices are added through QMP
and the Qemu device listener. If there was a way for Qemu
(the pc_xen_hvm_init_pci function) to actively attach the igd
instead of having libxl add it via QMP and the Qemu device
listener, that would also work and might improve performance
more because then both the igd and the xen platform device
would be added early during the creation of the guest.

Thanks.