| Message ID | 7280f16277aa9d7bdc9c2373277ef1b40459090b.1563325215.git-series.marmarek@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix PCI passthrough for HVM with stubdomain | expand |
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@invisiblethingslab.com> > --- > 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.
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@invisiblethingslab.com> > > --- > > 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.
On Thu, Jul 18, 2019 at 01:54:26AM +0200, Marek Marczykowski-Górecki wrote: > 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. Oh thanks, I'm certainly quite lost when it comes to XSM. > > > > > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > > > --- > > > 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)? It is, but you would have to iterate over all the entries, which I don't think it's intended here. A guest could disable MSI(-X) while having entries setup, and I don't think tearing down those entries should be done here. In fact I don't think INTx should be enabled when MSI(-X) is disabled, QEMU already traps writes to the command register, and it will manage INTx enabling/disabling by itself. I think the only check required is that MSI(-X) cannot be enabled if INTx is also enabled. In the same way both MSI caspabilities cannot be enabled simultaneously. The function should not explicitly disable any of the other capabilities, and just return -EBUSY if the caller attempts for example to enable MSI while INTx or MSI-X is enabled. > > 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. See my rant above, I don't think this hypercall should be touching INTx, or disabling/enabling other MSI capabilities, and instead just returning -EBUSY if the requested operation is not accepted because there are other capabilities enabled. > > > +/* 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. Yes sorry, uint16_t. I don't have a strong opinion, but since the structure is already 6bytes in size, I thought it might be better to have that padding assigned to flags instead of being hidden. Thanks, Roger.
On 18.07.2019 15:46, Roger Pau Monné wrote: > In fact I don't think INTx should be enabled when MSI(-X) is disabled, > QEMU already traps writes to the command register, and it will manage > INTx enabling/disabling by itself. I think the only check required is > that MSI(-X) cannot be enabled if INTx is also enabled. In the same > way both MSI caspabilities cannot be enabled simultaneously. The > function should not explicitly disable any of the other capabilities, > and just return -EBUSY if the caller attempts for example to enable > MSI while INTx or MSI-X is enabled. You do realize that pci_intx() only ever gets called for Xen internally used interrupts, i.e. mainly the serial console one? Jan
On Thu, Jul 18, 2019 at 03:17:27PM +0000, Jan Beulich wrote: > On 18.07.2019 15:46, Roger Pau Monné wrote: > > In fact I don't think INTx should be enabled when MSI(-X) is disabled, > > QEMU already traps writes to the command register, and it will manage > > INTx enabling/disabling by itself. I think the only check required is > > that MSI(-X) cannot be enabled if INTx is also enabled. In the same > > way both MSI caspabilities cannot be enabled simultaneously. The > > function should not explicitly disable any of the other capabilities, > > and just return -EBUSY if the caller attempts for example to enable > > MSI while INTx or MSI-X is enabled. > > You do realize that pci_intx() only ever gets called for Xen > internally used interrupts, i.e. mainly the serial console one? You will have to bear with me because I'm not sure I understand why it does matter. Do you mean to point out that dom0 is the one in full control of INTx, and thus Xen shouldn't care of whether INTx and MSI(-X) are enabled at the same time? I still think that at least a warning should be printed if a caller tries to enable MSI(-X) while INTx is also enabled, but unless there's a reason to have both MSI(-X) and INTx enabled at the same time (maybe a quirk for some hardware issue?) it shouldn't be allowed on this new interface. Thanks, Roger.
On 18.07.2019 18:52, Roger Pau Monné wrote: > On Thu, Jul 18, 2019 at 03:17:27PM +0000, Jan Beulich wrote: >> On 18.07.2019 15:46, Roger Pau Monné wrote: >>> In fact I don't think INTx should be enabled when MSI(-X) is disabled, >>> QEMU already traps writes to the command register, and it will manage >>> INTx enabling/disabling by itself. I think the only check required is >>> that MSI(-X) cannot be enabled if INTx is also enabled. In the same >>> way both MSI caspabilities cannot be enabled simultaneously. The >>> function should not explicitly disable any of the other capabilities, >>> and just return -EBUSY if the caller attempts for example to enable >>> MSI while INTx or MSI-X is enabled. >> >> You do realize that pci_intx() only ever gets called for Xen >> internally used interrupts, i.e. mainly the serial console one? > > You will have to bear with me because I'm not sure I understand why > it does matter. Do you mean to point out that dom0 is the one in full > control of INTx, and thus Xen shouldn't care of whether INTx and > MSI(-X) are enabled at the same time? > > I still think that at least a warning should be printed if a caller > tries to enable MSI(-X) while INTx is also enabled, but unless there's > a reason to have both MSI(-X) and INTx enabled at the same time (maybe > a quirk for some hardware issue?) it shouldn't be allowed on this new > interface. I don't mind improvements to the current situation (i.e. such a warning may indeed make sense); I merely stated how things currently are. INTx treatment was completely left aside when MSI support was introduced into Xen. Jan
On Fri, Jul 19, 2019 at 08:04:45AM +0000, Jan Beulich wrote: > On 18.07.2019 18:52, Roger Pau Monné wrote: > > On Thu, Jul 18, 2019 at 03:17:27PM +0000, Jan Beulich wrote: > >> On 18.07.2019 15:46, Roger Pau Monné wrote: > >>> In fact I don't think INTx should be enabled when MSI(-X) is disabled, > >>> QEMU already traps writes to the command register, and it will manage > >>> INTx enabling/disabling by itself. I think the only check required is > >>> that MSI(-X) cannot be enabled if INTx is also enabled. In the same > >>> way both MSI caspabilities cannot be enabled simultaneously. The > >>> function should not explicitly disable any of the other capabilities, > >>> and just return -EBUSY if the caller attempts for example to enable > >>> MSI while INTx or MSI-X is enabled. > >> > >> You do realize that pci_intx() only ever gets called for Xen > >> internally used interrupts, i.e. mainly the serial console one? > > > > You will have to bear with me because I'm not sure I understand why > > it does matter. Do you mean to point out that dom0 is the one in full > > control of INTx, and thus Xen shouldn't care of whether INTx and > > MSI(-X) are enabled at the same time? > > > > I still think that at least a warning should be printed if a caller > > tries to enable MSI(-X) while INTx is also enabled, but unless there's > > a reason to have both MSI(-X) and INTx enabled at the same time (maybe > > a quirk for some hardware issue?) it shouldn't be allowed on this new > > interface. > > I don't mind improvements to the current situation (i.e. such a > warning may indeed make sense); I merely stated how things currently > are. INTx treatment was completely left aside when MSI support was > introduced into Xen. In order to give Marek a more concise reply, would you agree to return -EBUSY (or some error code) and print a warning message if the caller attempts to enable MSI(-X) while INTx is also enabled? The same would apply to enabling both MSI(-X) capabilities at the same time. Thanks, Roger.
On 19.07.2019 11:02, Roger Pau Monné wrote: > On Fri, Jul 19, 2019 at 08:04:45AM +0000, Jan Beulich wrote: >> On 18.07.2019 18:52, Roger Pau Monné wrote: >>> On Thu, Jul 18, 2019 at 03:17:27PM +0000, Jan Beulich wrote: >>>> On 18.07.2019 15:46, Roger Pau Monné wrote: >>>>> In fact I don't think INTx should be enabled when MSI(-X) is disabled, >>>>> QEMU already traps writes to the command register, and it will manage >>>>> INTx enabling/disabling by itself. I think the only check required is >>>>> that MSI(-X) cannot be enabled if INTx is also enabled. In the same >>>>> way both MSI caspabilities cannot be enabled simultaneously. The >>>>> function should not explicitly disable any of the other capabilities, >>>>> and just return -EBUSY if the caller attempts for example to enable >>>>> MSI while INTx or MSI-X is enabled. >>>> >>>> You do realize that pci_intx() only ever gets called for Xen >>>> internally used interrupts, i.e. mainly the serial console one? >>> >>> You will have to bear with me because I'm not sure I understand why >>> it does matter. Do you mean to point out that dom0 is the one in full >>> control of INTx, and thus Xen shouldn't care of whether INTx and >>> MSI(-X) are enabled at the same time? >>> >>> I still think that at least a warning should be printed if a caller >>> tries to enable MSI(-X) while INTx is also enabled, but unless there's >>> a reason to have both MSI(-X) and INTx enabled at the same time (maybe >>> a quirk for some hardware issue?) it shouldn't be allowed on this new >>> interface. >> >> I don't mind improvements to the current situation (i.e. such a >> warning may indeed make sense); I merely stated how things currently >> are. INTx treatment was completely left aside when MSI support was >> introduced into Xen. > > In order to give Marek a more concise reply, would you agree to return > -EBUSY (or some error code) and print a warning message if the caller > attempts to enable MSI(-X) while INTx is also enabled? As to returning an error - I think so, yes. I'm less sure about logging a message. Jan
On 17.07.2019 03:00, Marek Marczykowski-Górecki wrote: > --- 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); Note that pci_get_pdev() returns hidden devices as well. That's not a problem for the other two uses in this file, but I think you want to explicitly deny access to hidden ones here, irrespective of the XSM check. Jan
On Fri, Jul 19, 2019 at 09:43:26AM +0000, Jan Beulich wrote: > On 19.07.2019 11:02, Roger Pau Monné wrote: > > On Fri, Jul 19, 2019 at 08:04:45AM +0000, Jan Beulich wrote: > >> On 18.07.2019 18:52, Roger Pau Monné wrote: > >>> On Thu, Jul 18, 2019 at 03:17:27PM +0000, Jan Beulich wrote: > >>>> On 18.07.2019 15:46, Roger Pau Monné wrote: > >>>>> In fact I don't think INTx should be enabled when MSI(-X) is disabled, > >>>>> QEMU already traps writes to the command register, and it will manage > >>>>> INTx enabling/disabling by itself. I think the only check required is > >>>>> that MSI(-X) cannot be enabled if INTx is also enabled. In the same > >>>>> way both MSI caspabilities cannot be enabled simultaneously. The > >>>>> function should not explicitly disable any of the other capabilities, > >>>>> and just return -EBUSY if the caller attempts for example to enable > >>>>> MSI while INTx or MSI-X is enabled. > >>>> > >>>> You do realize that pci_intx() only ever gets called for Xen > >>>> internally used interrupts, i.e. mainly the serial console one? > >>> > >>> You will have to bear with me because I'm not sure I understand why > >>> it does matter. Do you mean to point out that dom0 is the one in full > >>> control of INTx, and thus Xen shouldn't care of whether INTx and > >>> MSI(-X) are enabled at the same time? > >>> > >>> I still think that at least a warning should be printed if a caller > >>> tries to enable MSI(-X) while INTx is also enabled, but unless there's > >>> a reason to have both MSI(-X) and INTx enabled at the same time (maybe > >>> a quirk for some hardware issue?) it shouldn't be allowed on this new > >>> interface. > >> > >> I don't mind improvements to the current situation (i.e. such a > >> warning may indeed make sense); I merely stated how things currently > >> are. INTx treatment was completely left aside when MSI support was > >> introduced into Xen. > > > > In order to give Marek a more concise reply, would you agree to return > > -EBUSY (or some error code) and print a warning message if the caller > > attempts to enable MSI(-X) while INTx is also enabled? > > As to returning an error - I think so, yes. I'm less sure about logging > a message. I'm trying to get it working and it isn't clear to me what should I check for "INTx is also enabled". I assumed PCI_COMMAND_INTX_DISABLE bit, but it looks like guest has no control over this bit, even in permissive mode. This means enabling MSI(-X) always fails because guest has no way to set PCI_COMMAND_INTX_DISABLE bit before. Should I check something different? Or change back to disabling/enabling INTx as part of msi_control call? Or maybe allow guest/qemu to control PCI_COMMAND_INTX_DISABLE bit? In that case, I'd have very similar problem as with MSI - xen-pciback doesn't allow that, so I'd either need to patch pciback (and I could move this whole patch into Linux instead), or get around with a hypercall.
On 05.08.2019 15:44, Marek Marczykowski-Górecki wrote: > On Fri, Jul 19, 2019 at 09:43:26AM +0000, Jan Beulich wrote: >> On 19.07.2019 11:02, Roger Pau Monné wrote: >>> On Fri, Jul 19, 2019 at 08:04:45AM +0000, Jan Beulich wrote: >>>> On 18.07.2019 18:52, Roger Pau Monné wrote: >>>>> On Thu, Jul 18, 2019 at 03:17:27PM +0000, Jan Beulich wrote: >>>>>> On 18.07.2019 15:46, Roger Pau Monné wrote: >>>>>>> In fact I don't think INTx should be enabled when MSI(-X) is disabled, >>>>>>> QEMU already traps writes to the command register, and it will manage >>>>>>> INTx enabling/disabling by itself. I think the only check required is >>>>>>> that MSI(-X) cannot be enabled if INTx is also enabled. In the same >>>>>>> way both MSI caspabilities cannot be enabled simultaneously. The >>>>>>> function should not explicitly disable any of the other capabilities, >>>>>>> and just return -EBUSY if the caller attempts for example to enable >>>>>>> MSI while INTx or MSI-X is enabled. >>>>>> >>>>>> You do realize that pci_intx() only ever gets called for Xen >>>>>> internally used interrupts, i.e. mainly the serial console one? >>>>> >>>>> You will have to bear with me because I'm not sure I understand why >>>>> it does matter. Do you mean to point out that dom0 is the one in full >>>>> control of INTx, and thus Xen shouldn't care of whether INTx and >>>>> MSI(-X) are enabled at the same time? >>>>> >>>>> I still think that at least a warning should be printed if a caller >>>>> tries to enable MSI(-X) while INTx is also enabled, but unless there's >>>>> a reason to have both MSI(-X) and INTx enabled at the same time (maybe >>>>> a quirk for some hardware issue?) it shouldn't be allowed on this new >>>>> interface. >>>> >>>> I don't mind improvements to the current situation (i.e. such a >>>> warning may indeed make sense); I merely stated how things currently >>>> are. INTx treatment was completely left aside when MSI support was >>>> introduced into Xen. >>> >>> In order to give Marek a more concise reply, would you agree to return >>> -EBUSY (or some error code) and print a warning message if the caller >>> attempts to enable MSI(-X) while INTx is also enabled? >> >> As to returning an error - I think so, yes. I'm less sure about logging >> a message. > > I'm trying to get it working and it isn't clear to me what should I > check for "INTx is also enabled". I assumed PCI_COMMAND_INTX_DISABLE > bit, but it looks like guest has no control over this bit, even in > permissive mode. This means enabling MSI(-X) always fails because guest > has no way to set PCI_COMMAND_INTX_DISABLE bit before. Well, the guest has no control, but in order to enable MSI{,-X} I'd have expected qemu or the Dom0 kernel to set this bit up front. If that's not the case, then of course neither checking nor logging a message is appropriate at this point in time. It may be worthwhile calling out this anomaly then in the description. Jan
On Tue, Aug 06, 2019 at 07:56:39AM +0000, Jan Beulich wrote: > On 05.08.2019 15:44, Marek Marczykowski-Górecki wrote: > > On Fri, Jul 19, 2019 at 09:43:26AM +0000, Jan Beulich wrote: > >> On 19.07.2019 11:02, Roger Pau Monné wrote: > >>> On Fri, Jul 19, 2019 at 08:04:45AM +0000, Jan Beulich wrote: > >>>> On 18.07.2019 18:52, Roger Pau Monné wrote: > >>>>> On Thu, Jul 18, 2019 at 03:17:27PM +0000, Jan Beulich wrote: > >>>>>> On 18.07.2019 15:46, Roger Pau Monné wrote: > >>>>>>> In fact I don't think INTx should be enabled when MSI(-X) is disabled, > >>>>>>> QEMU already traps writes to the command register, and it will manage > >>>>>>> INTx enabling/disabling by itself. I think the only check required is > >>>>>>> that MSI(-X) cannot be enabled if INTx is also enabled. In the same > >>>>>>> way both MSI caspabilities cannot be enabled simultaneously. The > >>>>>>> function should not explicitly disable any of the other capabilities, > >>>>>>> and just return -EBUSY if the caller attempts for example to enable > >>>>>>> MSI while INTx or MSI-X is enabled. > >>>>>> > >>>>>> You do realize that pci_intx() only ever gets called for Xen > >>>>>> internally used interrupts, i.e. mainly the serial console one? > >>>>> > >>>>> You will have to bear with me because I'm not sure I understand why > >>>>> it does matter. Do you mean to point out that dom0 is the one in full > >>>>> control of INTx, and thus Xen shouldn't care of whether INTx and > >>>>> MSI(-X) are enabled at the same time? > >>>>> > >>>>> I still think that at least a warning should be printed if a caller > >>>>> tries to enable MSI(-X) while INTx is also enabled, but unless there's > >>>>> a reason to have both MSI(-X) and INTx enabled at the same time (maybe > >>>>> a quirk for some hardware issue?) it shouldn't be allowed on this new > >>>>> interface. > >>>> > >>>> I don't mind improvements to the current situation (i.e. such a > >>>> warning may indeed make sense); I merely stated how things currently > >>>> are. INTx treatment was completely left aside when MSI support was > >>>> introduced into Xen. > >>> > >>> In order to give Marek a more concise reply, would you agree to return > >>> -EBUSY (or some error code) and print a warning message if the caller > >>> attempts to enable MSI(-X) while INTx is also enabled? > >> > >> As to returning an error - I think so, yes. I'm less sure about logging > >> a message. > > > > I'm trying to get it working and it isn't clear to me what should I > > check for "INTx is also enabled". I assumed PCI_COMMAND_INTX_DISABLE > > bit, but it looks like guest has no control over this bit, even in > > permissive mode. This means enabling MSI(-X) always fails because guest > > has no way to set PCI_COMMAND_INTX_DISABLE bit before. > > Well, the guest has no control, but in order to enable MSI{,-X} I'd > have expected qemu or the Dom0 kernel to set this bit up front. qemu would do that, when running in dom0. But in PV stubdomain it talks to pciback, which filters it out. > If > that's not the case, then of course neither checking nor logging a > message is appropriate at this point in time. It may be worthwhile > calling out this anomaly then in the description. Ok, so I'll go back to setting PCI_COMMAND_INTX_DISABLE instead of just verification. Just to clarify: should I also clear PCI_COMMAND_INTX_DISABLE when disabling MSI? Now I think yes, because nothing else would do that otherwise, but I would like to double check.
On 06.08.2019 11:46, Marek Marczykowski-Górecki wrote: > On Tue, Aug 06, 2019 at 07:56:39AM +0000, Jan Beulich wrote: >> On 05.08.2019 15:44, Marek Marczykowski-Górecki wrote: >>> On Fri, Jul 19, 2019 at 09:43:26AM +0000, Jan Beulich wrote: >>>> On 19.07.2019 11:02, Roger Pau Monné wrote: >>>>> On Fri, Jul 19, 2019 at 08:04:45AM +0000, Jan Beulich wrote: >>>>>> On 18.07.2019 18:52, Roger Pau Monné wrote: >>>>>>> On Thu, Jul 18, 2019 at 03:17:27PM +0000, Jan Beulich wrote: >>>>>>>> On 18.07.2019 15:46, Roger Pau Monné wrote: >>>>>>>>> In fact I don't think INTx should be enabled when MSI(-X) is disabled, >>>>>>>>> QEMU already traps writes to the command register, and it will manage >>>>>>>>> INTx enabling/disabling by itself. I think the only check required is >>>>>>>>> that MSI(-X) cannot be enabled if INTx is also enabled. In the same >>>>>>>>> way both MSI caspabilities cannot be enabled simultaneously. The >>>>>>>>> function should not explicitly disable any of the other capabilities, >>>>>>>>> and just return -EBUSY if the caller attempts for example to enable >>>>>>>>> MSI while INTx or MSI-X is enabled. >>>>>>>> >>>>>>>> You do realize that pci_intx() only ever gets called for Xen >>>>>>>> internally used interrupts, i.e. mainly the serial console one? >>>>>>> >>>>>>> You will have to bear with me because I'm not sure I understand why >>>>>>> it does matter. Do you mean to point out that dom0 is the one in full >>>>>>> control of INTx, and thus Xen shouldn't care of whether INTx and >>>>>>> MSI(-X) are enabled at the same time? >>>>>>> >>>>>>> I still think that at least a warning should be printed if a caller >>>>>>> tries to enable MSI(-X) while INTx is also enabled, but unless there's >>>>>>> a reason to have both MSI(-X) and INTx enabled at the same time (maybe >>>>>>> a quirk for some hardware issue?) it shouldn't be allowed on this new >>>>>>> interface. >>>>>> >>>>>> I don't mind improvements to the current situation (i.e. such a >>>>>> warning may indeed make sense); I merely stated how things currently >>>>>> are. INTx treatment was completely left aside when MSI support was >>>>>> introduced into Xen. >>>>> >>>>> In order to give Marek a more concise reply, would you agree to return >>>>> -EBUSY (or some error code) and print a warning message if the caller >>>>> attempts to enable MSI(-X) while INTx is also enabled? >>>> >>>> As to returning an error - I think so, yes. I'm less sure about logging >>>> a message. >>> >>> I'm trying to get it working and it isn't clear to me what should I >>> check for "INTx is also enabled". I assumed PCI_COMMAND_INTX_DISABLE >>> bit, but it looks like guest has no control over this bit, even in >>> permissive mode. This means enabling MSI(-X) always fails because guest >>> has no way to set PCI_COMMAND_INTX_DISABLE bit before. >> >> Well, the guest has no control, but in order to enable MSI{,-X} I'd >> have expected qemu or the Dom0 kernel to set this bit up front. > > qemu would do that, when running in dom0. But in PV stubdomain it talks > to pciback, which filters it out. Filtering out the guest attempt is fine, but it should then set the bit while preparing MSI/MSI-X for the guest. (I'm not sure it would need to do so explicitly, or whether it couldn't rely on code elsewhere in the kernel doing so.) >> If >> that's not the case, then of course neither checking nor logging a >> message is appropriate at this point in time. It may be worthwhile >> calling out this anomaly then in the description. > > Ok, so I'll go back to setting PCI_COMMAND_INTX_DISABLE instead of just > verification. > > Just to clarify: should I also clear PCI_COMMAND_INTX_DISABLE when > disabling MSI? Now I think yes, because nothing else would do that > otherwise, but I would like to double check. Well, I think I've made my position clear: So far we use pci_intx() only on devices used by Xen itself. I think this should remain to be that way. Devices in possession of domains (including Dom0) should be under Dom0's control in this regard. Jan
On Tue, Aug 06, 2019 at 12:33:39PM +0200, Jan Beulich wrote: > On 06.08.2019 11:46, Marek Marczykowski-Górecki wrote: > > On Tue, Aug 06, 2019 at 07:56:39AM +0000, Jan Beulich wrote: > > > On 05.08.2019 15:44, Marek Marczykowski-Górecki wrote: > > > > On Fri, Jul 19, 2019 at 09:43:26AM +0000, Jan Beulich wrote: > > > > > On 19.07.2019 11:02, Roger Pau Monné wrote: > > > > > > On Fri, Jul 19, 2019 at 08:04:45AM +0000, Jan Beulich wrote: > > > > > > > On 18.07.2019 18:52, Roger Pau Monné wrote: > > > > > > > > On Thu, Jul 18, 2019 at 03:17:27PM +0000, Jan Beulich wrote: > > > > > > > > > On 18.07.2019 15:46, Roger Pau Monné wrote: > > > > > > > > > > In fact I don't think INTx should be enabled when MSI(-X) is disabled, > > > > > > > > > > QEMU already traps writes to the command register, and it will manage > > > > > > > > > > INTx enabling/disabling by itself. I think the only check required is > > > > > > > > > > that MSI(-X) cannot be enabled if INTx is also enabled. In the same > > > > > > > > > > way both MSI caspabilities cannot be enabled simultaneously. The > > > > > > > > > > function should not explicitly disable any of the other capabilities, > > > > > > > > > > and just return -EBUSY if the caller attempts for example to enable > > > > > > > > > > MSI while INTx or MSI-X is enabled. > > > > > > > > > > > > > > > > > > You do realize that pci_intx() only ever gets called for Xen > > > > > > > > > internally used interrupts, i.e. mainly the serial console one? > > > > > > > > > > > > > > > > You will have to bear with me because I'm not sure I understand why > > > > > > > > it does matter. Do you mean to point out that dom0 is the one in full > > > > > > > > control of INTx, and thus Xen shouldn't care of whether INTx and > > > > > > > > MSI(-X) are enabled at the same time? > > > > > > > > > > > > > > > > I still think that at least a warning should be printed if a caller > > > > > > > > tries to enable MSI(-X) while INTx is also enabled, but unless there's > > > > > > > > a reason to have both MSI(-X) and INTx enabled at the same time (maybe > > > > > > > > a quirk for some hardware issue?) it shouldn't be allowed on this new > > > > > > > > interface. > > > > > > > > > > > > > > I don't mind improvements to the current situation (i.e. such a > > > > > > > warning may indeed make sense); I merely stated how things currently > > > > > > > are. INTx treatment was completely left aside when MSI support was > > > > > > > introduced into Xen. > > > > > > > > > > > > In order to give Marek a more concise reply, would you agree to return > > > > > > -EBUSY (or some error code) and print a warning message if the caller > > > > > > attempts to enable MSI(-X) while INTx is also enabled? > > > > > > > > > > As to returning an error - I think so, yes. I'm less sure about logging > > > > > a message. > > > > > > > > I'm trying to get it working and it isn't clear to me what should I > > > > check for "INTx is also enabled". I assumed PCI_COMMAND_INTX_DISABLE > > > > bit, but it looks like guest has no control over this bit, even in > > > > permissive mode. This means enabling MSI(-X) always fails because guest > > > > has no way to set PCI_COMMAND_INTX_DISABLE bit before. > > > > > > Well, the guest has no control, but in order to enable MSI{,-X} I'd > > > have expected qemu or the Dom0 kernel to set this bit up front. > > > > qemu would do that, when running in dom0. But in PV stubdomain it talks > > to pciback, which filters it out. > > Filtering out the guest attempt is fine, but it should then set the > bit while preparing MSI/MSI-X for the guest. (I'm not sure it would > need to do so explicitly, or whether it couldn't rely on code > elsewhere in the kernel doing so.) ... > Well, I think I've made my position clear: So far we use pci_intx() > only on devices used by Xen itself. I think this should remain to be > that way. Devices in possession of domains (including Dom0) should > be under Dom0's control in this regard. The thing is, in case of using stubdomain, it's mostly stubdomain handling it. Especially all the config space interception and applying logic to it is done by qemu in stubdomain. Do you suggest duplicating / moving that part to dom0 instead? What would be the point for stubdomain then?
On 06.08.2019 12:53, Marek Marczykowski-Górecki wrote: > On Tue, Aug 06, 2019 at 12:33:39PM +0200, Jan Beulich wrote: >> On 06.08.2019 11:46, Marek Marczykowski-Górecki wrote: >>> On Tue, Aug 06, 2019 at 07:56:39AM +0000, Jan Beulich wrote: >>>> On 05.08.2019 15:44, Marek Marczykowski-Górecki wrote: >>>>> I'm trying to get it working and it isn't clear to me what should I >>>>> check for "INTx is also enabled". I assumed PCI_COMMAND_INTX_DISABLE >>>>> bit, but it looks like guest has no control over this bit, even in >>>>> permissive mode. This means enabling MSI(-X) always fails because guest >>>>> has no way to set PCI_COMMAND_INTX_DISABLE bit before. >>>> >>>> Well, the guest has no control, but in order to enable MSI{,-X} I'd >>>> have expected qemu or the Dom0 kernel to set this bit up front. >>> >>> qemu would do that, when running in dom0. But in PV stubdomain it talks >>> to pciback, which filters it out. >> >> Filtering out the guest attempt is fine, but it should then set the >> bit while preparing MSI/MSI-X for the guest. (I'm not sure it would >> need to do so explicitly, or whether it couldn't rely on code >> elsewhere in the kernel doing so.) > ... >> Well, I think I've made my position clear: So far we use pci_intx() >> only on devices used by Xen itself. I think this should remain to be >> that way. Devices in possession of domains (including Dom0) should >> be under Dom0's control in this regard. > > The thing is, in case of using stubdomain, it's mostly stubdomain > handling it. Especially all the config space interception and applying > logic to it is done by qemu in stubdomain. Do you suggest duplicating / > moving that part to dom0 instead? What would be the point for stubdomain > then? Nothing should be moved between components if possible (and if placed sensibly). But isn't stubdom (being a PV DomU) relying on pciback in Dom0 anyway? And hence doesn't the flow of events include pci_enable_msi{,x}() invoked by pciback? I'd have expected that to (also) take care of INTx. Jan
On Tue, Aug 06, 2019 at 02:05:48PM +0200, Jan Beulich wrote: > On 06.08.2019 12:53, Marek Marczykowski-Górecki wrote: > > On Tue, Aug 06, 2019 at 12:33:39PM +0200, Jan Beulich wrote: > > > On 06.08.2019 11:46, Marek Marczykowski-Górecki wrote: > > > > On Tue, Aug 06, 2019 at 07:56:39AM +0000, Jan Beulich wrote: > > > > > On 05.08.2019 15:44, Marek Marczykowski-Górecki wrote: > > > > > > I'm trying to get it working and it isn't clear to me what should I > > > > > > check for "INTx is also enabled". I assumed PCI_COMMAND_INTX_DISABLE > > > > > > bit, but it looks like guest has no control over this bit, even in > > > > > > permissive mode. This means enabling MSI(-X) always fails because guest > > > > > > has no way to set PCI_COMMAND_INTX_DISABLE bit before. > > > > > > > > > > Well, the guest has no control, but in order to enable MSI{,-X} I'd > > > > > have expected qemu or the Dom0 kernel to set this bit up front. > > > > > > > > qemu would do that, when running in dom0. But in PV stubdomain it talks > > > > to pciback, which filters it out. > > > > > > Filtering out the guest attempt is fine, but it should then set the > > > bit while preparing MSI/MSI-X for the guest. (I'm not sure it would > > > need to do so explicitly, or whether it couldn't rely on code > > > elsewhere in the kernel doing so.) > > ... > > > Well, I think I've made my position clear: So far we use pci_intx() > > > only on devices used by Xen itself. I think this should remain to be > > > that way. Devices in possession of domains (including Dom0) should > > > be under Dom0's control in this regard. > > > > The thing is, in case of using stubdomain, it's mostly stubdomain > > handling it. Especially all the config space interception and applying > > logic to it is done by qemu in stubdomain. Do you suggest duplicating / > > moving that part to dom0 instead? What would be the point for stubdomain > > then? > > Nothing should be moved between components if possible (and if placed > sensibly). But isn't stubdom (being a PV DomU) relying on pciback in > Dom0 anyway? And hence doesn't the flow of events include > pci_enable_msi{,x}() invoked by pciback? I'd have expected that to > (also) take care of INTx. This was discussed in v2 of this series[1], where you also responded. Relevant part of Roger's email: Oh great, that's unfortunate. Both pciback functions end up calling into msi_capability_init in the Linux kernel, which does indeed more than just toggling the PCI config space enable bit. OTOH adding a bypass to pciback so the stubdom is able to write to the PCI register in order to toggle the enable bit seems quite clumsy. Not to mention that you would be required to update Dom0 kernel in order to fix the issue. Do you think it makes sense to add a domctl to enable/disable MSI(X)? This way the bug could be fixed by just updating Xen (and the stubdomain). [1] https://lists.xenproject.org/archives/html/xen-devel/2019-01/msg01271.html
On 06.08.2019 14:43, Marek Marczykowski-Górecki wrote: > On Tue, Aug 06, 2019 at 02:05:48PM +0200, Jan Beulich wrote: >> On 06.08.2019 12:53, Marek Marczykowski-Górecki wrote: >>> On Tue, Aug 06, 2019 at 12:33:39PM +0200, Jan Beulich wrote: >>>> On 06.08.2019 11:46, Marek Marczykowski-Górecki wrote: >>>>> On Tue, Aug 06, 2019 at 07:56:39AM +0000, Jan Beulich wrote: >>>>>> On 05.08.2019 15:44, Marek Marczykowski-Górecki wrote: >>>>>>> I'm trying to get it working and it isn't clear to me what should I >>>>>>> check for "INTx is also enabled". I assumed PCI_COMMAND_INTX_DISABLE >>>>>>> bit, but it looks like guest has no control over this bit, even in >>>>>>> permissive mode. This means enabling MSI(-X) always fails because guest >>>>>>> has no way to set PCI_COMMAND_INTX_DISABLE bit before. >>>>>> >>>>>> Well, the guest has no control, but in order to enable MSI{,-X} I'd >>>>>> have expected qemu or the Dom0 kernel to set this bit up front. >>>>> >>>>> qemu would do that, when running in dom0. But in PV stubdomain it talks >>>>> to pciback, which filters it out. >>>> >>>> Filtering out the guest attempt is fine, but it should then set the >>>> bit while preparing MSI/MSI-X for the guest. (I'm not sure it would >>>> need to do so explicitly, or whether it couldn't rely on code >>>> elsewhere in the kernel doing so.) >>> ... >>>> Well, I think I've made my position clear: So far we use pci_intx() >>>> only on devices used by Xen itself. I think this should remain to be >>>> that way. Devices in possession of domains (including Dom0) should >>>> be under Dom0's control in this regard. >>> >>> The thing is, in case of using stubdomain, it's mostly stubdomain >>> handling it. Especially all the config space interception and applying >>> logic to it is done by qemu in stubdomain. Do you suggest duplicating / >>> moving that part to dom0 instead? What would be the point for stubdomain >>> then? >> >> Nothing should be moved between components if possible (and if placed >> sensibly). But isn't stubdom (being a PV DomU) relying on pciback in >> Dom0 anyway? And hence doesn't the flow of events include >> pci_enable_msi{,x}() invoked by pciback? I'd have expected that to >> (also) take care of INTx. > > This was discussed in v2 of this series[1], where you also responded. > Relevant part of Roger's email: > > Oh great, that's unfortunate. Both pciback functions end up calling > into msi_capability_init in the Linux kernel, which does indeed more > than just toggling the PCI config space enable bit. > > OTOH adding a bypass to pciback so the stubdom is able to write to the > PCI register in order to toggle the enable bit seems quite clumsy. Not > to mention that you would be required to update Dom0 kernel in order to > fix the issue. > > Do you think it makes sense to add a domctl to enable/disable MSI(X)? > > This way the bug could be fixed by just updating Xen (and the > stubdomain). > > [1] https://lists.xenproject.org/archives/html/xen-devel/2019-01/msg01271.html Hmm, right. But us having closed a circle this way (as it seems) suggests to me that bypassing Dom0 may not have been the right idea here after all (i.e. is, as Roger said, clumsy). Also may I ask whether the quoted there "so this most likely won’t work correctly" has actually been proven to not work, and not be possible to make work? Jan
On Tue, Aug 06, 2019 at 02:43:49PM +0200, Marek Marczykowski-Górecki wrote: > On Tue, Aug 06, 2019 at 02:05:48PM +0200, Jan Beulich wrote: > > On 06.08.2019 12:53, Marek Marczykowski-Górecki wrote: > > > On Tue, Aug 06, 2019 at 12:33:39PM +0200, Jan Beulich wrote: > > > > On 06.08.2019 11:46, Marek Marczykowski-Górecki wrote: > > > > > On Tue, Aug 06, 2019 at 07:56:39AM +0000, Jan Beulich wrote: > > > > > > On 05.08.2019 15:44, Marek Marczykowski-Górecki wrote: > > > > > > > I'm trying to get it working and it isn't clear to me what should I > > > > > > > check for "INTx is also enabled". I assumed PCI_COMMAND_INTX_DISABLE > > > > > > > bit, but it looks like guest has no control over this bit, even in > > > > > > > permissive mode. This means enabling MSI(-X) always fails because guest > > > > > > > has no way to set PCI_COMMAND_INTX_DISABLE bit before. > > > > > > > > > > > > Well, the guest has no control, but in order to enable MSI{,-X} I'd > > > > > > have expected qemu or the Dom0 kernel to set this bit up front. > > > > > > > > > > qemu would do that, when running in dom0. But in PV stubdomain it talks > > > > > to pciback, which filters it out. > > > > > > > > Filtering out the guest attempt is fine, but it should then set the > > > > bit while preparing MSI/MSI-X for the guest. (I'm not sure it would > > > > need to do so explicitly, or whether it couldn't rely on code > > > > elsewhere in the kernel doing so.) > > > ... > > > > Well, I think I've made my position clear: So far we use pci_intx() > > > > only on devices used by Xen itself. I think this should remain to be > > > > that way. Devices in possession of domains (including Dom0) should > > > > be under Dom0's control in this regard. > > > > > > The thing is, in case of using stubdomain, it's mostly stubdomain > > > handling it. Especially all the config space interception and applying > > > logic to it is done by qemu in stubdomain. Do you suggest duplicating / > > > moving that part to dom0 instead? What would be the point for stubdomain > > > then? > > > > Nothing should be moved between components if possible (and if placed > > sensibly). But isn't stubdom (being a PV DomU) relying on pciback in > > Dom0 anyway? And hence doesn't the flow of events include > > pci_enable_msi{,x}() invoked by pciback? I'd have expected that to > > (also) take care of INTx. > > This was discussed in v2 of this series[1], where you also responded. > Relevant part of Roger's email: > > Oh great, that's unfortunate. Both pciback functions end up calling > into msi_capability_init in the Linux kernel, which does indeed more > than just toggling the PCI config space enable bit. > > OTOH adding a bypass to pciback so the stubdom is able to write to the > PCI register in order to toggle the enable bit seems quite clumsy. Not > to mention that you would be required to update Dom0 kernel in order to > fix the issue. > > Do you think it makes sense to add a domctl to enable/disable MSI(X)? > > This way the bug could be fixed by just updating Xen (and the > stubdomain). > > [1] https://lists.xenproject.org/archives/html/xen-devel/2019-01/msg01271.html The main problem here is that PCI passthrough for PV is completely different than passthrough for HVM, and hence trying to use the PV passthrough interface as a proxy to the HVM interface (and implementation in QEMU) is likely to find issues as we have seen. I still think that modifying pciback is not the right call, as that is just adding bodges to the pciif PV interface to suit a non-PV use-case, which pciback/pcifront wasn't designed to handle (as likely no one thought of handling passthrough in a stubdomain). I still think that adding these missing pieces to an hypercall is likely the best solution here, but we need to know beforehand whatever the studomain needs to do so that it can be put inside of a single hypercall if possible. So far this is: modify the MSI/MSI-X control bit and the INTx bit in the command register? In which case the hypercall should maybe be named PHYSDEVOP_interrupt_control or some such? Thanks, Roger.
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); + } + + 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; + + } + 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; +}; +typedef struct physdev_msi_control physdev_msi_control_t; +DEFINE_XEN_GUEST_HANDLE(physdev_msi_control_t); + /* * Notify that some PIRQ-bound event channels have been unmasked. * ** This command is obsolete since interface version 0x00030202 and is ** diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst index 95f5e55..3082761 100644 --- a/xen/include/xlat.lst +++ b/xen/include/xlat.lst @@ -108,6 +108,7 @@ ? physdev_irq_status_query physdev.h ? physdev_manage_pci physdev.h ? physdev_manage_pci_ext physdev.h +? physdev_msi_control physdev.h ? physdev_pci_device physdev.h ? physdev_pci_device_add physdev.h ? physdev_pci_mmcfg_reserved physdev.h diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h index 01d2814..4801838 100644 --- a/xen/include/xsm/dummy.h +++ b/xen/include/xsm/dummy.h @@ -514,6 +514,13 @@ static XSM_INLINE int xsm_pci_config_permission(XSM_DEFAULT_ARG struct domain *d return xsm_default_action(action, current->domain, d); } +static XSM_INLINE int xsm_msi_control(XSM_DEFAULT_ARG struct domain *d, uint32_t machine_bdf, + uint8_t msix, uint8_t enable) +{ + XSM_ASSERT_ACTION(XSM_DM_PRIV); + return xsm_default_action(action, current->domain, d); +} + static XSM_INLINE int xsm_add_to_physmap(XSM_DEFAULT_ARG struct domain *d1, struct domain *d2) { XSM_ASSERT_ACTION(XSM_TARGET); diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h index b6141f6..bf39dbd 100644 --- a/xen/include/xsm/xsm.h +++ b/xen/include/xsm/xsm.h @@ -106,6 +106,7 @@ struct xsm_operations { int (*iomem_permission) (struct domain *d, uint64_t s, uint64_t e, uint8_t allow); int (*iomem_mapping) (struct domain *d, uint64_t s, uint64_t e, uint8_t allow); int (*pci_config_permission) (struct domain *d, uint32_t machine_bdf, uint16_t start, uint16_t end, uint8_t access); + int (*msi_control) (struct domain *d, uint32_t machine_bdf, uint8_t msix, uint8_t enable); #if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_PCI) int (*get_device_group) (uint32_t machine_bdf); @@ -464,6 +465,11 @@ static inline int xsm_pci_config_permission (xsm_default_t def, struct domain *d return xsm_ops->pci_config_permission(d, machine_bdf, start, end, access); } +static inline int xsm_msi_control (xsm_default_t def, struct domain *d, uint32_t machine_bdf, uint8_t msix, uint8_t enable) +{ + return xsm_ops->msi_control(d, machine_bdf, msix, enable); +} + #if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_PCI) static inline int xsm_get_device_group(xsm_default_t def, uint32_t machine_bdf) { diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c index c9a566f..878eefe 100644 --- a/xen/xsm/dummy.c +++ b/xen/xsm/dummy.c @@ -81,6 +81,7 @@ void __init xsm_fixup_ops (struct xsm_operations *ops) set_to_dummy_if_null(ops, iomem_permission); set_to_dummy_if_null(ops, iomem_mapping); set_to_dummy_if_null(ops, pci_config_permission); + set_to_dummy_if_null(ops, msi_control); set_to_dummy_if_null(ops, get_vnumainfo); #if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_PCI) diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c index a7d690a..5fb755e 100644 --- a/xen/xsm/flask/hooks.c +++ b/xen/xsm/flask/hooks.c @@ -1083,6 +1083,29 @@ static int flask_pci_config_permission(struct domain *d, uint32_t machine_bdf, u } +static int flask_msi_control(struct domain *d, uint32_t machine_bdf, uint8_t msix, uint8_t enable) +{ + uint32_t dsid, rsid; + int rc = -EPERM; + struct avc_audit_data ad; + uint32_t perm; + + AVC_AUDIT_DATA_INIT(&ad, DEV); + ad.device = machine_bdf; + + rc = security_device_sid(machine_bdf, &rsid); + if ( rc ) + return rc; + + rc = avc_current_has_perm(rsid, SECCLASS_RESOURCE, RESOURCE__SETUP, &ad); + if ( rc ) + return rc; + + perm = flask_iommu_resource_use_perm(); + dsid = domain_sid(d); + return avc_has_perm(dsid, rsid, SECCLASS_RESOURCE, perm, &ad); +} + static int flask_resource_plug_core(void) { return avc_current_has_perm(SECINITSID_DOMXEN, SECCLASS_RESOURCE, RESOURCE__PLUG, NULL); @@ -1800,6 +1823,7 @@ static struct xsm_operations flask_ops = { .iomem_permission = flask_iomem_permission, .iomem_mapping = flask_iomem_mapping, .pci_config_permission = flask_pci_config_permission, + .msi_control = flask_msi_control, .resource_plug_core = flask_resource_plug_core, .resource_unplug_core = flask_resource_unplug_core, diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors index 194d743..0ddfc91 100644 --- a/xen/xsm/flask/policy/access_vectors +++ b/xen/xsm/flask/policy/access_vectors @@ -466,6 +466,7 @@ class resource # checked for PHYSDEVOP_restore_msi* (target PCI device) # checked for PHYSDEVOP_setup_gsi (target IRQ) # checked for PHYSDEVOP_pci_mmcfg_reserved (target xen_t) +# checked for PHYSDEVOP_msi_control (target PCI device) setup }
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@invisiblethingslab.com> --- 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 (?) - 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. --- 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(+)