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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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()?

> +{
> +    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()? And what about invocations for
the wrong device (e.g. MSI-X request for MSI-X incapable device)?

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

> +    /* 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.

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



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