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

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

On Thu, Jul 18, 2019 at 01:54:26AM +0200, Marek Marczykowski-Górecki wrote:
> On Wed, Jul 17, 2019 at 12:18:43PM +0200, Roger Pau Monné wrote:
> > On Wed, Jul 17, 2019 at 03:00:43AM +0200, Marek Marczykowski-Górecki wrote:
> > > Allow device model running in stubdomain to enable/disable MSI(-X),
> > > bypassing pciback. While pciback is still used to access config space
> > > from within stubdomain, it refuse to write to
> > > PCI_MSI_FLAGS_ENABLE/PCI_MSIX_FLAGS_ENABLE in non-permissive mode. Which
> > > is the right thing to do for PV domain (the main use case for pciback),
> > > as PV domain should use XEN_PCI_OP_* commands for that. Unfortunately
> > > those commands are not good for stubdomain use, as they configure MSI in
> > > dom0's kernel too, which should not happen for HVM domain.
> > > 
> > > This new physdevop is allowed only for stubdomain controlling the domain
> > > which own the device.
> > 
> > I think I'm missing that part, is this maybe done by the XSM stuff?
> 
> Yes, specifically xsm_msi_control(XSM_DM_PRIV, pdev->domain, ...) call.
> XSM_DM_PRIV allows call if src->is_privileged, or if src->target ==
> target. Note the target being owner of the device here.

Oh thanks, I'm certainly quite lost when it comes to XSM.

> > > 
> > > Signed-off-by: Marek Marczykowski-Górecki 
> > > <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> > > ---
> > > Changes in v3:
> > >  - new patch
> > > Changes in v4:
> > >  - adjust code style
> > >  - s/msi_msix/msi/
> > >  - add msi_set_enable XSM hook
> > >  - flatten struct physdev_msi_set_enable
> > >  - add to include/xlat.lst
> > > Changes in v5:
> > >  - rename to PHYSDEVOP_msi_control
> > >  - combine "mode" and "enable" into "flags"
> > >  - refuse to enable both MSI and MSI-X, and also to enable MSI(-X) on
> > >    incapable device
> > >  - disable/enable INTx when enabling/disabling MSI (?)
> > 
> > You don't enable INTx when MSI is disabled.
> 
> Ah, indeed. When I look for similar code in Xen elsewhere, I found
> __pci_disable_msi() has extra conditions for pci_intx(dev, true):
> 
>     if ( entry->irq > 0 && !(irq_to_desc(entry->irq)->status & IRQ_GUEST) )
>         pci_intx(dev, true);
> 
> Should this be mirrored there too? Isn't IRQ_GUEST always set in case of
> passthrough to HVM (the case this patch care)?

It is, but you would have to iterate over all the entries, which I
don't think it's intended here. A guest could disable MSI(-X) while
having entries setup, and I don't think tearing down those entries
should be done here.

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.

> > if ( enable )
> > {
> >     pci_intx(pdev, false);
> >     if ( msix )
> >         msi_set_enable(pdev, false);
> >     else
> >         msix_set_enable(pdev, false);
> > }
> > 
> > if ( msix )
> >     msix_set_enable(pdev, enable);
> > else
> >     msi_set_enable(pdev, enable);
> > 
> > Note that in the same way you make sure INTx is disabled, you should
> > also make sure MSI and MSI-X are not enabled at the same time.
> 
> This is exactly what the code in the patch already does.

See my rant above, I don't think this hypercall should be touching
INTx, or disabling/enabling other MSI capabilities, and instead just
returning -EBUSY if the requested operation is not accepted because
there are other capabilities enabled.

> > > +/* when PHYSDEVOP_MSI_CONTROL_MSIX not set, control MSI */
> > > +#define PHYSDEVOP_MSI_CONTROL_MSIX    1
> > > +/* when PHYSDEVOP_MSI_CONTROL_ENABLE not set, disable */
> > > +#define PHYSDEVOP_MSI_CONTROL_ENABLE  2
> > > +
> > > +#define PHYSDEVOP_msi_control   32
> > > +struct physdev_msi_control {
> > > +    /* IN */
> > > +    uint16_t seg;
> > > +    uint8_t bus;
> > > +    uint8_t devfn;
> > > +    uint8_t flags;
> > 
> > I would just make flags uint32_t, the padding to align is going to be
> > added in any case.
> 
> That would make the structure 8 bytes, not 6. Did you mean uint16_t? 
> But structure size here doesn't really matter anyway.

Yes sorry, uint16_t. I don't have a strong opinion, but since the
structure is already 6bytes in size, I thought it might be better to
have that padding assigned to flags instead of being hidden.

Thanks, Roger.

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