Re: [Xen-devel] [PATCH v5 5/6] xen/x86: add PHYSDEVOP_msi_control

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

On Tue, Aug 06, 2019 at 02:05:48PM +0200, Jan Beulich wrote:
> On 06.08.2019 12:53, Marek Marczykowski-Górecki  wrote:
> > On Tue, Aug 06, 2019 at 12:33:39PM +0200, Jan Beulich wrote:
> > > On 06.08.2019 11:46, Marek Marczykowski-Górecki  wrote:
> > > > On Tue, Aug 06, 2019 at 07:56:39AM +0000, Jan Beulich wrote:
> > > > > On 05.08.2019 15:44, Marek Marczykowski-Górecki  wrote:
> > > > > > I'm trying to get it working and it isn't clear to me what should I
> > > > > > check for "INTx is also enabled". I assumed PCI_COMMAND_INTX_DISABLE
> > > > > > bit, but it looks like guest has no control over this bit, even in
> > > > > > permissive mode.  This means enabling MSI(-X) always fails because 
> > > > > > guest
> > > > > > has no way to set PCI_COMMAND_INTX_DISABLE bit before.
> > > > > 
> > > > > Well, the guest has no control, but in order to enable MSI{,-X} I'd
> > > > > have expected qemu or the Dom0 kernel to set this bit up front.
> > > > 
> > > > qemu would do that, when running in dom0. But in PV stubdomain it talks
> > > > to pciback, which filters it out.
> > > 
> > > Filtering out the guest attempt is fine, but it should then set the
> > > bit while preparing MSI/MSI-X for the guest. (I'm not sure it would
> > > need to do so explicitly, or whether it couldn't rely on code
> > > elsewhere in the kernel doing so.)
> > ...
> > > Well, I think I've made my position clear: So far we use pci_intx()
> > > only on devices used by Xen itself. I think this should remain to be
> > > that way. Devices in possession of domains (including Dom0) should
> > > be under Dom0's control in this regard.
> > 
> > The thing is, in case of using stubdomain, it's mostly stubdomain
> > handling it. Especially all the config space interception and applying
> > logic to it is done by qemu in stubdomain. Do you suggest duplicating /
> > moving that part to dom0 instead? What would be the point for stubdomain
> > then?
> 
> Nothing should be moved between components if possible (and if placed
> sensibly). But isn't stubdom (being a PV DomU) relying on pciback in
> Dom0 anyway? And hence doesn't the flow of events include
> pci_enable_msi{,x}() invoked by pciback? I'd have expected that to
> (also) take care of INTx.

This was discussed in v2 of this series[1], where you also responded.
Relevant part of Roger's email:

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

[1] https://lists.xenproject.org/archives/html/xen-devel/2019-01/msg01271.html

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