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

[Chuck Zmudzinski <brchuckz@xxxxxxxxxxxx>]

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.

> 
>> 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 think the reason libvirt/kvm gets it right is simply because the
code implementing the libvirt/kvm approach got more attention and testing
than the code that implements the libxl/Xen approach. This patch
really represents what should have been done when support for the
igd-passthru=on option for xenfv machines was added seven years ago,
but the code was apparently added without much testing and is stale now
and needs this fix which is entirely implemented in either files maintained
by Xen maintainers or, in the case of the small patch to pc_piix.c,
entirely within a section guarded by #ifdef CONFIG_XEN. Not much
maintenance burden for hw/i386 maintainers.

> 
>> 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.

For all practical purposes, the only additional maintenance would
be handled by Xen maintainers, and the Xen maintainer of the Xen
files being patched gave a Reviewed-by to an earlier iteration of
this patch. So I think the decision about the maintenance cost of
this patch should be made by the Xen maintainers. In fact, if I
were a Xen maintainer, I think this patch to Qemu would be much
easier for the Xen maintainers to maintain than the proposed patch
to libxl to fix this. So ultimately, I think it makes sense for
the Xen maintainers to decide on the maintenance cost. So far
they have not weighed in since the Reviewed-by that Anthony
gave to an earlier iteration of this patch. So far, they have
not responded to my patch to libxl, and I don't blame them because
that would be more difficult for them to maintain than this patch
to some of the Xen-specific code within Qemu.

For reference, the patch for libxl that fixes this is here:

https://lore.kernel.org/qemu-devel/20230110073201.mdUvSjy1vKtxPriqMQuWAxIjQzf1eAqIlZgal1u3GBI@z/

> Though I don't object to it as long as it's constrained to xen only
> code

It already is constrained to Xen only code - the small patch to
pc_piix.c is entirely guarded by #ifdef CONFIG_XEN.

and doesn't spill into generic PCI.

In comments on an earlier iteration of this patch, Michael indicated
he would not object a patch to core pci if it added some useful
functionality.

Michael, do I misunderstand you?

I have already proposed a patch that does that, which, if accepted,
would address the objection that unconditionally reserving the slot
during initialization is not desirable. He pointed out that a patch
to core pci could fix that, and I have proposed such a patch,
independent of this patch, here:

https://lore.kernel.org/qemu-devel/ad5f5cf8bc4bd4a720724ed41e47565a7f27adf5.1673829387.git.brchuckz@xxxxxxx/

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

I disagree that the default configuration should configure the hardware
in a way that does not conform to the requirements of the device and thereby
force users to add non-default settings to configure the machine correctly.
That is simply not being friendly to Xen users of Qemu, and that,
unfortunately is what Qemu code currently does and has done for the
past seven years as regards the configuration by Qemu of igd passthru on
Xen. IMO, it is unreasonable to not fix this, and since the fix can be
implemented in entirely Xen-specific code, I hope and expect that
eventually the Xen maintainers will fix this. I hope they are just waiting
until I implement the fixes that you and Michael have requested which
are mostly reasonable and admittedly, not completed yet.

Perhaps this approach is what you call a "quirk" because of the limitations
of how slot_reserved_mask works. That can be fixed by patching core pci.
That, IMO, is the best and most maintainable way to fix this.

So my plan is to wait and see how my proposed patch to core pci is received.
If it gets accepted, I will do a v10 of this patch which will use the
improved management capability added by the patch to core pci that addresses
the concerns that this patch will interfere with the libvirt/kvm approach
of manually assigning the slots by causing the slot_reserved_mask to
only take effect when the device being added is configured for auto
assignment of the slot address. When libvirt adds a pci device to a xenfv
machine configured for igd-patthru, my proposed v10, with the patch to core
pci as a prerequisite, will not introduce any change to how Qemu configures
the machine in response to a libvirt configuration that manually assigns the
slot addresses.

I do accept that v8/v9 of the patch is stalled, and I am working to address
all the concerns being raised here. Thanks for your comments!

Kind regards,

Chuck