Re: [Xen-devel] [PATCH v4 5/6] xen/x86: add PHYSDEVOP_msi_set_enable

[Marek Marczykowski <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Wed, Feb 27, 2019 at 04:41:37AM -0700, Jan Beulich wrote:
> >>> On 07.02.19 at 01:07, <marmarek@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/msi.c
> > +++ b/xen/arch/x86/msi.c
> > @@ -1474,6 +1474,30 @@ int pci_restore_msi_state(struct pci_dev *pdev)
> >      return 0;
> >  }
> >  
> > +int msi_msix_set_enable(struct pci_dev *pdev, int mode, int enable)
> 
> unsigned int mode, bool enable
> 
> I'm also not happy about the function name. Perhaps simply msi_enable()
> or msi_control()?

Ok, will change to msi_control().

> > +{
> > +    int ret;
> > +
> > +    ret = xsm_msi_set_enable(XSM_DM_PRIV, pdev->domain,
> > +                             (pdev->seg << 16) | (pdev->bus << 8) | 
> > pdev->devfn,
> > +                             mode, enable);
> > +    if ( ret )
> > +        return ret;
> > +
> > +    switch ( mode )
> > +    {
> > +    case PHYSDEVOP_MSI_SET_ENABLE_MSI:
> > +        msi_set_enable(pdev, enable);
> > +        break;
> > +
> > +    case PHYSDEVOP_MSI_SET_ENABLE_MSIX:
> > +        msix_set_enable(pdev, enable);
> > +        break;
> > +    }
> 
> What about a call to pci_intx()? 

Should pci_intx(dev, !enable) be called in all those cases?

> And what about invocations for
> the wrong device (e.g. MSI-X request for MSI-X incapable device)?

Looking at msi(x)_set_enable(), it is no-op for incapable devices, but
if the function would do anything else, indeed such check should be
added. Is pci_find_cap_offset(..., PCI_CAP_ID_MSI(X)) the correct way
of doing that?

> > --- a/xen/arch/x86/physdev.c
> > +++ b/xen/arch/x86/physdev.c
> > @@ -671,6 +671,30 @@ ret_t do_physdev_op(int cmd, 
> > XEN_GUEST_HANDLE_PARAM(void) arg)
> >          break;
> >      }
> >  
> > +    case PHYSDEVOP_msi_set_enable: {
> > +        struct physdev_msi_set_enable op;
> > +        struct pci_dev *pdev;
> > +
> > +        ret = -EFAULT;
> > +        if ( copy_from_guest(&op, arg, 1) )
> > +            break;
> > +
> > +        ret = -EINVAL;
> > +        if ( op.mode != PHYSDEVOP_MSI_SET_ENABLE_MSI &&
> > +             op.mode != PHYSDEVOP_MSI_SET_ENABLE_MSIX )
> > +            break;
> > +
> > +        pcidevs_lock();
> > +        pdev = pci_get_pdev(op.seg, op.bus, op.devfn);
> > +        if ( pdev )
> > +            ret = msi_msix_set_enable(pdev, op.mode, !!op.enable);
> 
> Unnecessary !! .
> 
> > +        else
> > +            ret = -ENODEV;
> > +        pcidevs_unlock();
> > +        break;
> > +
> > +    }
> 
> Stray blank line above here.
> 
> > --- a/xen/include/public/physdev.h
> > +++ b/xen/include/public/physdev.h
> > @@ -344,6 +344,21 @@ struct physdev_dbgp_op {
> >  typedef struct physdev_dbgp_op physdev_dbgp_op_t;
> >  DEFINE_XEN_GUEST_HANDLE(physdev_dbgp_op_t);
> >  
> > +#define PHYSDEVOP_MSI_SET_ENABLE_MSI  0
> > +#define PHYSDEVOP_MSI_SET_ENABLE_MSIX 1
> > +
> > +#define PHYSDEVOP_msi_set_enable   32
> > +struct physdev_msi_set_enable {
> 
> Can this please also be something like msi_control?

Sure.

> > +    /* IN */
> > +    uint16_t seg;
> > +    uint8_t bus;
> > +    uint8_t devfn;
> > +    uint8_t mode;
> > +    uint8_t enable;
> 
> "mode" and "enable" don't really make clear which of the two is the
> boolean, and which is the operation. I'd anyway prefer a single
> flags field with descriptive #define-s, which will also make more
> obvious how to extend this if need be.

You mean:

#define PHYSDEVOP_MSI_CONTROL_ENABLE 1
#define PHYSDEVOP_MSI_CONTROL_MSI    2 
#define PHYSDEVOP_MSI_CONTROL_MSIX   4

Then use PHYSDEVOP_MSI_CONTROL_MSI(X) with or without
PHYSDEVOP_MSI_CONTROL_ENABLE ?

> > --- a/xen/include/xlat.lst
> > +++ b/xen/include/xlat.lst
> > @@ -106,6 +106,7 @@
> >  ?  physdev_restore_msi             physdev.h
> >  ?  physdev_set_iopl                physdev.h
> >  ?  physdev_setup_gsi               physdev.h
> > +?  physdev_msi_set_enable          physdev.h
> 
> Please insert at the alphabetically correct place.
> 
> Jan
> 
> 

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