|
[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 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@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.
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:
signature.asc _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |