Re: [Xen-devel] [PATCH v2 4/4] xen/x86: Allow stubdom access to irq created for msi.

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Wed, Jan 16, 2019 at 01:20:04PM +0100, Roger Pau Monné wrote:
> On Wed, Jan 16, 2019 at 11:52:18AM +0100, Marek Marczykowski-Górecki wrote:
> > On Wed, Jan 16, 2019 at 10:21:29AM +0100, Roger Pau Monné wrote:
> > > On Tue, Jan 15, 2019 at 04:36:31PM +0100, Marek Marczykowski-Górecki 
> > > wrote:
> > > > From: Simon Gaiser <simon@xxxxxxxxxxxxxxxxxxxxxx>
> > > > 
> > > > Stubdomains need to be given sufficient privilege over the guest which 
> > > > it
> > > > provides emulation for in order for PCI passthrough to work correctly.
> > > > When a HVM domain try to enable MSI, QEMU in stubdomain calls
> > > > PHYSDEVOP_map_pirq, but later it needs to call XEN_DOMCTL_bind_pt_irq as
> > > > part of xc_domain_update_msi_irq. Allow for that as part of
> > > > PHYSDEVOP_map_pirq.
> > > 
> > > I see, that's not a problem AFAICT for PCI INTx because the IRQ in
> > > that case is known beforehand, and the stubdomain is given permissions
> > > over this IRQ by libxl__device_pci_add (there's a do_pci_add against
> > > the stubdomain).
> > 
> > Exactly.
> 
> I would maybe consider adding something like this to the commit
> message, so it's clear why PCI INTx works but not MSI interrupts.
> 
> > 
> > > > 
> > > > Based on 
> > > > https://github.com/OpenXT/xenclient-oe/blob/5e0e7304a5a3c75ef01240a1e3673665b2aaf05e/recipes-extended/xen/files/stubdomain-msi-irq-access.patch
> > > >  by Eric Chanudet <chanudete@xxxxxxxxxxxx>.
> > > > 
> > > > Signed-off-by: Simon Gaiser <simon@xxxxxxxxxxxxxxxxxxxxxx>
> > > > Signed-off-by: Marek Marczykowski-Górecki 
> > > > <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> > > > ---
> > > > This is only one part of fixing MSI with QEMU in stubdomain. The other
> > > > part is allowing stubdomain to actually enable MSI in PCI config space.
> > > > QEMU does that through pcifront/back connected to the stubdomain (see
> > > > hw/xen/xen_pt_msi.c:msi_msix_enable()), but pciback by default refuse
> > > > write to that register.
> > > > Easy, less safe solution: enable permissive mode for the device.
> > > > Safer solution - enable access to this register for stubdomain only
> > > > (pciback patch that add such flag + libxl patch to set it for relevant
> > > >  devices)
> > > > The whole story:
> > > > https://www.qubes-os.org/news/2017/10/18/msi-support/
> > > > 
> > > > Any other ideas? Which one is preferred upstream?
> > > 
> > > IMO, and please correct me if I'm wrong, QEMU in the stubdomain will
> > > receive the PCI config space write to enable MSI, and since this
> > > stub-QEMU runs in PV mode I think it should use the PV way to enable
> > > MSI, ie: the same that Linux pcifront uses to enable MSI for
> > > passed-through devices.
> > > 
> > > Is this something that sounds sensible?
> > 
> > We've considered this option too. Let me quote Simon on that (from the
> > link above):
> > 
> >     The enable command that pcifront sends is intended for the normal PV use
> >     case where the device is passed to the VM itself (via pcifront) rather
> >     than to the stub domain target. While the command is called enable_msi,
> >     pciback does much more than simply setting the enable flag. It also
> >     configures IRQ handling in the dom0 kernel, adapts the MSI masking, and
> >     more. This makes sense in the PV case, but in the HVM case, the MSI
> >     configuration is done by QEMU, so this most likely won’t work correctly.
> 
> Oh great, that's unfortunate. Both pciback functions end up calling
> into msi_capability_init in the Linux kernel, which does indeed more
> than just toggling the PCI config space enable bit.
> 
> OTOH adding a bypass to pciback so the stubdom is able to write to the
> PCI register in order to toggle the enable bit seems quite clumsy. Not
> to mention that you would be required to update Dom0 kernel in order to
> fix the issue.
> 
> Do you think it makes sense to add a domctl to enable/disable MSI(X)?
> 
> This way the bug could be fixed by just updating Xen (and the
> stubdomain).

Indeed in case of stubdomain, that would make sens, as other PCI passthrough
related operations already bypass pcifront/back anyway.

And I agree with Jan, that physdevop makes more sense, if going this way.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel