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

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

On Fri, Jul 19, 2019 at 09:43:26AM +0000, Jan Beulich wrote:
> On 19.07.2019 11:02, Roger Pau Monné  wrote:
> > On Fri, Jul 19, 2019 at 08:04:45AM +0000, Jan Beulich wrote:
> >> On 18.07.2019 18:52, Roger Pau Monné  wrote:
> >>> On Thu, Jul 18, 2019 at 03:17:27PM +0000, Jan Beulich wrote:
> >>>> On 18.07.2019 15:46, Roger Pau Monné  wrote:
> >>>>> In fact I don't think INTx should be enabled when MSI(-X) is disabled,
> >>>>> QEMU already traps writes to the command register, and it will manage
> >>>>> INTx enabling/disabling by itself. I think the only check required is
> >>>>> that MSI(-X) cannot be enabled if INTx is also enabled. In the same
> >>>>> way both MSI caspabilities cannot be enabled simultaneously. The
> >>>>> function should not explicitly disable any of the other capabilities,
> >>>>> and just return -EBUSY if the caller attempts for example to enable
> >>>>> MSI while INTx or MSI-X is enabled.
> >>>>
> >>>> You do realize that pci_intx() only ever gets called for Xen
> >>>> internally used interrupts, i.e. mainly the serial console one?
> >>>
> >>> You will have to bear with me because I'm not sure I understand why
> >>> it does matter. Do you mean to point out that dom0 is the one in full
> >>> control of INTx, and thus Xen shouldn't care of whether INTx and
> >>> MSI(-X) are enabled at the same time?
> >>>
> >>> I still think that at least a warning should be printed if a caller
> >>> tries to enable MSI(-X) while INTx is also enabled, but unless there's
> >>> a reason to have both MSI(-X) and INTx enabled at the same time (maybe
> >>> a quirk for some hardware issue?) it shouldn't be allowed on this new
> >>> interface.
> >>
> >> I don't mind improvements to the current situation (i.e. such a
> >> warning may indeed make sense); I merely stated how things currently
> >> are. INTx treatment was completely left aside when MSI support was
> >> introduced into Xen.
> > 
> > In order to give Marek a more concise reply, would you agree to return
> > -EBUSY (or some error code) and print a warning message if the caller
> > attempts to enable MSI(-X) while INTx is also enabled?
> 
> As to returning an error - I think so, yes. I'm less sure about logging
> a message.

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.

Should I check something different? Or change back to disabling/enabling
INTx as part of msi_control call? Or maybe allow guest/qemu to control
PCI_COMMAND_INTX_DISABLE bit? In that case, I'd have very similar
problem as with MSI - xen-pciback doesn't allow that, so I'd either need
to patch pciback (and I could move this whole patch into Linux
instead), or get around with a hypercall.

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