Re: [Xen-devel] [PATCH v3 5/6] xen/x86: add PHYSDEVOP_msi_msix_set_enable

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

On Sat, Jan 26, 2019 at 03:31:16AM +0100, 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.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>

Thanks!

> ---
> Changes in v3:
>  - new patch
> 
> This is rather RFC. Any suggestions for shorter name? Also, I'm not sure
> if physdev_msi_msix_set_enable.flag is the best name/idea.

I've made some comments below.

> Should it be plugged into XSM? Any suggestions how exactly? New
> function with XSM_DM_PRIV default action? Should it get target domain
> only, or also machine_bdf?

You should Cc the XSM maintainer I think, which I've done now.

> ---
>  xen/arch/x86/msi.c           | 16 ++++++++++++++++
>  xen/arch/x86/physdev.c       | 24 ++++++++++++++++++++++++
>  xen/include/asm-x86/msi.h    |  1 +
>  xen/include/public/physdev.h | 13 +++++++++++++
>  4 files changed, 54 insertions(+)
> 
> diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
> index babc414..9ba934c 100644
> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -1474,6 +1474,22 @@ int pci_restore_msi_state(struct pci_dev *pdev)
>      return 0;
>  }
>  
> +int msi_msix_set_enable(struct pci_dev *pdev, int flag, int enable)
> +{
> +    if ( !current->domain->target || pdev->domain != current->domain->target 
> )
> +        return -EPERM;
> +
> +    switch ( flag ) {
> +        case PHYSDEVOP_MSI_MSIX_SET_ENABLE_MSI:
> +            msi_set_enable(pdev, enable);
> +            break;

Please add a newline here.

> +        case PHYSDEVOP_MSI_MSIX_SET_ENABLE_MSIX:
> +            msix_set_enable(pdev, enable);
> +            break;
> +    }
> +    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 de59e39..822846a 100644
> --- 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_msix_set_enable: {
> +        struct physdev_msi_msix_set_enable op;
> +        struct pci_dev *pdev;
> +
> +        ret = -EFAULT;
> +        if ( copy_from_guest(&op, arg, 1) )
> +            break;
> +
> +        ret = -EINVAL;
> +        if ( op.flag != PHYSDEVOP_MSI_MSIX_SET_ENABLE_MSI &&
> +                op.flag != PHYSDEVOP_MSI_MSIX_SET_ENABLE_MSIX )

Align.

> +            break;
> +
> +        pcidevs_lock();
> +        pdev = pci_get_pdev(op.pci.seg, op.pci.bus, op.pci.devfn);
> +        if ( pdev )
> +            ret = msi_msix_set_enable(pdev, op.flag, !!op.enable);
> +        else
> +            ret = -ENODEV;
> +        pcidevs_unlock();
> +        break;
> +
> +    }
> +
>      default:
>          ret = -ENOSYS;
>          break;
> diff --git a/xen/include/asm-x86/msi.h b/xen/include/asm-x86/msi.h
> index 10387dc..080bf24 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_msix_set_enable(struct pci_dev *pdev, int flag, int enable);
>  
>  #endif /* __ASM_MSI_H */
> diff --git a/xen/include/public/physdev.h b/xen/include/public/physdev.h
> index b6faf83..fd797c6 100644
> --- a/xen/include/public/physdev.h
> +++ b/xen/include/public/physdev.h
> @@ -344,6 +344,19 @@ struct physdev_dbgp_op {
>  typedef struct physdev_dbgp_op physdev_dbgp_op_t;
>  DEFINE_XEN_GUEST_HANDLE(physdev_dbgp_op_t);
>  
> +#define PHYSDEVOP_MSI_MSIX_SET_ENABLE_MSI  0
> +#define PHYSDEVOP_MSI_MSIX_SET_ENABLE_MSIX 1
> +
> +#define PHYSDEVOP_msi_msix_set_enable   32

There's no need for the 'msi_msix' name, there are already other
hypercalls that deal with both msi and msix and just have msi in the
name: PHYSDEVOP_msi_set_enable.

> +struct physdev_msi_msix_set_enable {
> +    /* IN */
> +    struct physdev_pci_device pci;
> +    uint8_t flag;

But this is not really a flags field, I would rather rename this
to 'mode' maybe.

> +    uint8_t enable;
> +};
> +typedef struct physdev_msi_msix_set_enable physdev_msi_msix_set_enable_t;
> +DEFINE_XEN_GUEST_HANDLE(physdev_msi_msix_set_enable_t);

I think you need to add the new hypercall to include/xlat.lst, AFAICT
it requires no translation, so you should add it as '?'.

Thanks, Roger.

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