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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Aug 06, 2019 at 02:43:49PM +0200, Marek Marczykowski-Górecki wrote:
> 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

The main problem here is that PCI passthrough for PV is completely
different than passthrough for HVM, and hence trying to use the PV
passthrough interface as a proxy to the HVM interface (and
implementation in QEMU) is likely to find issues as we have seen.

I still think that modifying pciback is not the right call, as that
is just adding bodges to the pciif PV interface to suit a non-PV
use-case, which pciback/pcifront wasn't designed to handle (as likely
no one thought of handling passthrough in a stubdomain).

I still think that adding these missing pieces to an hypercall is
likely the best solution here, but we need to know beforehand whatever
the studomain needs to do so that it can be put inside of a single
hypercall if possible. So far this is: modify the MSI/MSI-X control
bit and the INTx bit in the command register?

In which case the hypercall should maybe be named
PHYSDEVOP_interrupt_control or some such?

Thanks, Roger.

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