|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v5 5/6] xen/x86: add PHYSDEVOP_msi_control
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?
>
> 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.
> - 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;
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.
> +
> + 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.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |