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

[Igor Mammedov <imammedo@xxxxxxxxxx>]

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)

> However, the performance hit can be prevented by assigning slot
> 3 instead of slot 2 for the xen platform device if igd passthrough
> is configured on the command line instead of doing slot reservation,
> but there would still be less convenience and, for libxl users, an
> inability to easily configure the command line so that the igd can
> still have slot 2 without a hacky and error-prone patch to libxl to
> deal with this problem.
libvirt manages to get it right on management side without quirks on
QEMU side.

> I did post a patch on xen-devel to fix this using libxl, but so far
> it has not yet been reviewed and I mentioned in that patch that the
> approach of patching qemu so qemu reserves slot 2 for the igd is less
> prone to coding errors and is easier to maintain than the patch that
> would be required to implement the fix in libxl.

the patch is not trivial, and adds maintenance on QEMU.
Though I don't object to it as long as it's constrained to xen only
code and doesn't spill into generic PCI.
All I wanted is just point out there are other approach to problem
(i.e. do force user to user to provide correct configuration instead
of adding quirks whenever it's possible).