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

["Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>]

[-- Attachment #1.1: Type: text/plain, Size: 9526 bytes --]

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.

> > 
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > ---
> > 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)?

> >  - refuse if !use_msi
> >  - adjust flask hook to make more sense (require "setup" access on
> >    device, not on domain)
> >  - rebase on master
> > 
> > I'm not sure if XSM part is correct, compile-tested only, as I'm not
> > sure how to set the policy.
> 
> I'm also not familiar with XSM, so I will have to defer to Daniel on
> this one.
> 
> > ---
> >  xen/arch/x86/msi.c                  | 42 ++++++++++++++++++++++++++++++-
> >  xen/arch/x86/physdev.c              | 25 ++++++++++++++++++-
> >  xen/arch/x86/x86_64/physdev.c       |  4 +++-
> >  xen/include/asm-x86/msi.h           |  1 +-
> >  xen/include/public/physdev.h        | 16 +++++++++++-
> >  xen/include/xlat.lst                |  1 +-
> >  xen/include/xsm/dummy.h             |  7 +++++-
> >  xen/include/xsm/xsm.h               |  6 ++++-
> >  xen/xsm/dummy.c                     |  1 +-
> >  xen/xsm/flask/hooks.c               | 24 +++++++++++++++++-
> >  xen/xsm/flask/policy/access_vectors |  1 +-
> >  11 files changed, 128 insertions(+)
> > 
> > diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
> > index 89e6116..fca1d04 100644
> > --- a/xen/arch/x86/msi.c
> > +++ b/xen/arch/x86/msi.c
> > @@ -1475,6 +1475,48 @@ int pci_restore_msi_state(struct pci_dev *pdev)
> >      return 0;
> >  }
> >  
> > +int msi_control(struct pci_dev *pdev, bool msix, bool enable)
> > +{
> > +    int ret;
> > +    struct msi_desc *old_desc;
> > +
> > +    if ( !use_msi )
> > +        return -EOPNOTSUPP;
> > +
> > +    ret = xsm_msi_control(XSM_DM_PRIV, pdev->domain, pdev->sbdf.sbdf, msix, enable);
> > +    if ( ret )
> > +        return ret;
> > +
> > +    if ( msix )
> > +    {
> > +        if ( !pdev->msix )
> > +            return -ENODEV;
> > +        old_desc = find_msi_entry(pdev, -1, PCI_CAP_ID_MSI);
> > +        if ( old_desc )
> > +            return -EBUSY;
> > +        if ( enable )
> > +            pci_intx(pdev, false);
> > +        msix_set_enable(pdev, enable);
> > +    }
> > +    else
> > +    {
> > +        if ( !pci_find_cap_offset(pdev->seg,
> > +                                  pdev->bus,
> > +                                  PCI_SLOT(pdev->devfn),
> > +                                  PCI_FUNC(pdev->devfn),
> > +                                  PCI_CAP_ID_MSI) )
> > +            return -ENODEV;
> > +        old_desc = find_msi_entry(pdev, -1, PCI_CAP_ID_MSIX);
> > +        if ( old_desc )
> > +            return -EBUSY;
> > +        if ( enable )
> > +            pci_intx(pdev, false);
> > +        msi_set_enable(pdev, enable);
> > +    }
> 
> I think you could just do:
> 
> unsigned int cap = msix ? PCI_CAP_ID_MSIX : PCI_CAP_ID_MSI;
> 
> [...]
> 
> if ( !pci_find_cap_offset(pdev->seg,
>                           pdev->bus,
>                           PCI_SLOT(pdev->devfn),
>                           PCI_FUNC(pdev->devfn), cap) )
>     return -ENODEV;
> 
> old_desc = find_msi_entry(pdev, -1, cap);
> if ( old_desc )
>     return -EBUSY;

Note the check prevents enabling MSI when MSI-X is enabled and vice
versa. Not enabling already enabled MSI. Anyway, if using
pci_find_cap_offset for PCI_CAP_ID_MSIX too, this code indeed can be
deduplicated a little.

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

> > +
> > +    return 0;
> > +}
> > +
> >  void __init early_msi_init(void)
> >  {
> >      if ( use_msi < 0 )
> > diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
> > index 3a3c158..5000998 100644
> > --- a/xen/arch/x86/physdev.c
> > +++ b/xen/arch/x86/physdev.c
> > @@ -662,6 +662,31 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> >          break;
> >      }
> >  
> > +    case PHYSDEVOP_msi_control: {
> > +        struct physdev_msi_control op;
> > +        struct pci_dev *pdev;
> > +
> > +        ret = -EFAULT;
> > +        if ( copy_from_guest(&op, arg, 1) )
> > +            break;
> > +
> > +        ret = -EINVAL;
> > +        if ( op.flags & ~(PHYSDEVOP_MSI_CONTROL_MSIX | PHYSDEVOP_MSI_CONTROL_ENABLE) )
> > +            break;
> > +
> > +        pcidevs_lock();
> > +        pdev = pci_get_pdev(op.seg, op.bus, op.devfn);
> > +        if ( pdev )
> > +            ret = msi_control(pdev,
> > +                              op.flags & PHYSDEVOP_MSI_CONTROL_MSIX,
> > +                              op.flags & PHYSDEVOP_MSI_CONTROL_ENABLE);
> > +        else
> > +            ret = -ENODEV;
> > +        pcidevs_unlock();
> > +        break;
> > +
> 
> Extra newline.
> 
> > +    }
> > +
> >      default:
> >          ret = -ENOSYS;
> >          break;
> > diff --git a/xen/arch/x86/x86_64/physdev.c b/xen/arch/x86/x86_64/physdev.c
> > index c5a00ea..69b4ce3 100644
> > --- a/xen/arch/x86/x86_64/physdev.c
> > +++ b/xen/arch/x86/x86_64/physdev.c
> > @@ -76,6 +76,10 @@ CHECK_physdev_pci_device_add
> >  CHECK_physdev_pci_device
> >  #undef xen_physdev_pci_device
> >  
> > +#define xen_physdev_msi_control physdev_msi_control
> > +CHECK_physdev_msi_control
> > +#undef xen_physdev_msi_control
> > +
> >  #define COMPAT
> >  #undef guest_handle_okay
> >  #define guest_handle_okay          compat_handle_okay
> > diff --git a/xen/include/asm-x86/msi.h b/xen/include/asm-x86/msi.h
> > index 10387dc..05296de 100644
> > --- a/xen/include/asm-x86/msi.h
> > +++ b/xen/include/asm-x86/msi.h
> > @@ -252,5 +252,6 @@ void guest_mask_msi_irq(struct irq_desc *, bool mask);
> >  void ack_nonmaskable_msi_irq(struct irq_desc *);
> >  void end_nonmaskable_msi_irq(struct irq_desc *, u8 vector);
> >  void set_msi_affinity(struct irq_desc *, const cpumask_t *);
> > +int msi_control(struct pci_dev *pdev, bool msix, bool enable);
> >  
> >  #endif /* __ASM_MSI_H */
> > diff --git a/xen/include/public/physdev.h b/xen/include/public/physdev.h
> > index b6faf83..f9b728f 100644
> > --- a/xen/include/public/physdev.h
> > +++ b/xen/include/public/physdev.h
> > @@ -344,6 +344,22 @@ struct physdev_dbgp_op {
> >  typedef struct physdev_dbgp_op physdev_dbgp_op_t;
> >  DEFINE_XEN_GUEST_HANDLE(physdev_dbgp_op_t);
> >  
> > +/* 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.

-- 
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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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