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

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