Re: [RFC v4 15/16] vfio/type1: Check MSI remapping at irq domain level
Hi Eric,
On 12/13/2016 10:32 PM, Eric Auger wrote:
> In case the IOMMU does not bypass MSI transactions (typical
> case on ARM), we check all MSI controllers are IRQ remapping
> capable. If not the IRQ assignment may be unsafe.
>
> At this stage the arm-smmu-(v3) still advertise the
> IOMMU_CAP_INTR_REMAP capability at IOMMU level. This will be
> removed in subsequent patches.
>
> Signed-off-by: Eric Auger
> ---
> drivers/vfio/vfio_iommu_type1.c | 9 ++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index d07fe73..a05648b 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -37,6 +37,7 @@
> #include
> #include
> #include
> +#include
>
> #define DRIVER_VERSION "0.2"
> #define DRIVER_AUTHOR "Alex Williamson "
> @@ -765,7 +766,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
> struct vfio_domain *domain, *d;
> struct bus_type *bus = NULL;
> int ret;
> - bool resv_msi;
> + bool resv_msi, msi_remap;
> phys_addr_t resv_msi_base;
>
> mutex_lock(&iommu->lock);
> @@ -818,8 +819,10 @@ static int vfio_iommu_type1_attach_group(void
> *iommu_data,
> INIT_LIST_HEAD(&domain->group_list);
> list_add(&group->next, &domain->group_list);
>
> - if (!allow_unsafe_interrupts &&
> - !iommu_capable(bus, IOMMU_CAP_INTR_REMAP)) {
> + msi_remap = resv_msi ? irq_domain_check_msi_remap() :
> +iommu_capable(bus, IOMMU_CAP_INTR_REMAP);
> +
> + if (!allow_unsafe_interrupts && !msi_remap) {
> pr_warn("%s: No interrupt remapping support. Use the module
> param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support on this
> platform\n",
> __func__);
> ret = -EPERM;
I tested your v4.9-reserved-v4 branch on a ITS capable hardware (NXP
LS2080), so I did not set allow_unsafe_interrupts. It fails here
complaining that the there is no interrupt remapping support. The
irq_domain_check_msi_remap function returns false as none of the checked
domains has the IRQ_DOMAIN_FLAG_MSI_REMAP flag set. I think the reason
is that the flags are not propagated through the domain hierarchy when
the domain is created.
Thanks,
Diana
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v14 14/16] vfio/type1: Check doorbell safety
Hi Eric,
On 10/12/2016 04:23 PM, Eric Auger wrote:
> On x86 IRQ remapping is abstracted by the IOMMU. On ARM this is abstracted
> by the msi controller.
>
> Since we currently have no way to detect whether the MSI controller is
> upstream or downstream to the IOMMU we rely on the MSI doorbell information
> registered by the interrupt controllers. In case at least one doorbell
> does not implement proper isolation, we state the assignment is unsafe
> with regard to interrupts. This is a coarse assessment but should allow to
> wait for a better system description.
>
> At this point ARM sMMU still advertises IOMMU_CAP_INTR_REMAP. This is
> removed in next patch.
>
> Signed-off-by: Eric Auger
>
> ---
> v13 -> v15:
> - check vfio_msi_resv before checking whether msi doorbell is safe
>
> v9 -> v10:
> - coarse safety assessment based on MSI doorbell info
>
> v3 -> v4:
> - rename vfio_msi_parent_irq_remapping_capable into vfio_safe_irq_domain
> and irq_remapping into safe_irq_domains
>
> v2 -> v3:
> - protect vfio_msi_parent_irq_remapping_capable with
> CONFIG_GENERIC_MSI_IRQ_DOMAIN
> ---
> drivers/vfio/vfio_iommu_type1.c | 30 +-
> 1 file changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index e0c97ef..c18ba9d 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -442,6 +442,29 @@ static void vfio_unmap_unpin(struct vfio_iommu *iommu,
> struct vfio_dma *dma)
> }
>
> /**
> + * vfio_msi_resv - Return whether any VFIO iommu domain requires
> + * MSI mapping
> + *
> + * @iommu: vfio iommu handle
> + *
> + * Return: true of MSI mapping is needed, false otherwise
> + */
> +static bool vfio_msi_resv(struct vfio_iommu *iommu)
> +{
> + struct iommu_domain_msi_resv msi_resv;
> + struct vfio_domain *d;
> + int ret;
> +
> + list_for_each_entry(d, &iommu->domain_list, next) {
> + ret = iommu_domain_get_attr(d->domain, DOMAIN_ATTR_MSI_RESV,
> + &msi_resv);
> + if (!ret)
> + return true;
> + }
> + return false;
> +}
> +
> +/**
> * vfio_set_msi_aperture - Sets the msi aperture on all domains
> * requesting MSI mapping
> *
> @@ -945,8 +968,13 @@ static int vfio_iommu_type1_attach_group(void
> *iommu_data,
> INIT_LIST_HEAD(&domain->group_list);
> list_add(&group->next, &domain->group_list);
>
> + /*
> + * to advertise safe interrupts either the IOMMU or the MSI controllers
> + * must support IRQ remapping (aka. interrupt translation)
> + */
> if (!allow_unsafe_interrupts &&
> - !iommu_capable(bus, IOMMU_CAP_INTR_REMAP)) {
> + (!iommu_capable(bus, IOMMU_CAP_INTR_REMAP) &&
> + !(vfio_msi_resv(iommu) && iommu_msi_doorbell_safe( {
> pr_warn("%s: No interrupt remapping support. Use the module
> param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support on this
> platform\n",
> __func__);
> ret = -EPERM;
I understand from the other discussions that you will respin these
series, but anyway I have tested this version with GICV3 + ITS and it
stops here. As I have a GICv3 I am not supposed to enable allow unsafe
interrupts. What I see is that vfio_msi_resv returns false just because
the iommu->domain_list list is empty. The newly created domain is
actually added to the domain_list at the end of this function, so it
seems normal for the list to be empty at this point.
Thanks,
Diana
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: SMR masking and PCI
Hi Robin,
On 10/28/2016 07:16 PM, Robin Murphy wrote:
> Hi Stuart,
>
> On 27/10/16 18:10, Stuart Yoder wrote:
>> Hi Robin,
>>
>> A question about how the SMR masking defined in the arm,smmu binding
>> relates to the PCI iommu-map.
>>
>> The #iommu-cells property defines the number of cells an "IOMMU specifier"
>> takes and 2 is specified to be:
>>
>>SMMUs with stream matching support and complex masters
>>may use a value of 2, where the second cell represents
>>an SMR mask to combine with the ID in the first cell.
>>
>> An iommu-map entry is defined as:
>>
>>(rid-base,iommu,iommu-base,length)
>>
>> What seems to be currently missing in the iommu-map support is
>> the possibility the case where #iommu-cells=<2>.
> Indeed. The bindings have so far rather implicitly assumed the case of
> #{msi,iommu}-cells = 1, and the code has followed suit.
>
>> In this case iommu-base which is an IOMMU specifier should
>> occupy 2 cells. For example on an ls2085a we would want:
>>
>> iommu-map = <0x0 0x6 0x7 0x3ff 0x1
>> 0x100 0x6 0x8 0x3ff 0x1>;
>>
>> ...to mask our stream IDs to 10 bits.
>>
>> This should work in theory and comply with the bindings, no?
> In theory, but now consider:
>
> iommu-map = <0x0 0x6 0x7 0x3ff 0x2>;
>
> faced with ID 1. The input base is 0, the output base is the 2-cell
> value 0x703ff, so the final ID value should be 0x70400, right?
>
>> of_pci_map_rid() seems to have a hardcoded assumption that
>> each field in the map is 4 bytes.
> It does. I guess we should explicitly check that #{msi,iommu}-cells = 1
> on the target node, and maybe clarify in the binding that that cell
> should represent a plain linear ID value (although that's pretty obvious
> from context IMO).
>
>> (Also, I guess that msi-map is not affected by this since it
>> is not related to the IOMMU...but we do have common code
>> handling both maps.)
> I'd say it's definitely affected, since #msi-cells can equally be more
> than 1, and encodes an equally opaque value.
>
> It seems pretty reasonable to me that we could extend the binding to
> accommodate #cells > 1 iff length == 1. Mark?
>
> That said, is there a concrete need for this, i.e. do you actually have
> one device with a single requester ID, which maps to multiple output IDs
> (which differ only in the upper bits) in a non-predictable manner?
>
Actually in the example presented by Stuart, the SMR mask should be
0x7C00 (as 0 means that the bit is relevant for matching). So, we have
the stream ID 7, but the SMMU 500 is appending the TBU bits which makes
the stream ID look like 0x1407 (TBU 5). In our platform the relationship
device-TBU is not exposed and documented. The SMMU-500 ref manual
describes this case:
"If the Stream ID presented to each TBU is already unique, and the TBU
ID addition is not required, then you must ensure that the TBU ID field
is masked in the SMR."
This is the reason that we need the SMR mask, to mask the TBU bits in
the SMR.
Thank you,
Diana
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v12 0/8] KVM PCIe/MSI passthrough on ARM/ARM64: kernel part 3/3: vfio changes
Hi Eric,
I have tested these patches in a VFIO PCI scenario (using the ITS
emulation) on a NXP LS2080 board. It worked fine with one e1000 card
assigned to the guest. However, when I tried to assign two cards to the
guest I got a crash. I narrowed down the problem to this code:
drivers/vfio/vfio_iommu_type1.c, vfio_iommu_type1_attach_group function
/*
* Try to match an existing compatible domain. We don't want to
* preclude an IOMMU driver supporting multiple bus_types and being
* able to include different bus_types in the same IOMMU domain, so
* we test whether the domains use the same iommu_ops rather than
* testing if they're on the same bus_type.
*/
list_for_each_entry(d, &iommu->domain_list, next) {
if (d->domain->ops == domain->domain->ops &&
d->prot == domain->prot) {
iommu_detach_group(domain->domain, iommu_group);
if (!iommu_attach_group(d->domain, iommu_group)) {
list_add(&group->next, &d->group_list);
iommu_domain_free(domain->domain);
kfree(domain);
mutex_unlock(&iommu->lock);
return 0;
}
ret = iommu_attach_group(domain->domain, iommu_group);
if (ret)
goto out_domain;
}
}
The iommu_domain_free function eventually calls iommu_put_dma_cookie
which calls put_iova_domain. This function (put_iova_domain) tries to
acquire some spinlocks which were not yet initialized. They will be
initialized later when the first DMA mapping will be performed. Because
the spinlock was not yet initialized, it crashes when attempting to
acquire it.
With the following fix I was able to successfully assign two cards to
the guest:
diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index ba764a0..b43e34f 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -457,6 +457,9 @@ void put_iova_domain(struct iova_domain *iovad)
struct rb_node *node;
unsigned long flags;
+ if (!iovad->start_pfn)
+ return;
+
free_iova_rcaches(iovad);
spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
node = rb_first(&iovad->rbroot);
However, I am not sure if this is the right fix.
Thanks,
Diana
On 08/02/2016 08:30 PM, Eric Auger wrote:
> This series allows the user-space to register a reserved IOVA domain.
> This completes the kernel integration of the whole functionality on top
> of the 2 previous parts (v11).
>
> We reuse the VFIO DMA MAP ioctl with a new flag to bridge to the
> msi-iommu API. The need for provisioning such MSI IOVA range is reported
> through capability chain, using VFIO_IOMMU_TYPE1_INFO_CAP_MSI_GEOMETRY.
>
> vfio_iommu_type1 checks if the MSI mapping is safe when attaching the
> vfio group to the container (allow_unsafe_interrupts modality). This is
> done in a very coarse way, looking at all the registered doorbells and
> returning assignment is safe wrt IRQ if all the doorbells are safe.
>
> More details & context can be found at:
> http://www.linaro.org/blog/core-dump/kvm-pciemsi-passthrough-armarm64/
>
> Best Regards
>
> Eric
>
> Testing:
> - functional on ARM64 AMD Overdrive HW (single GICv2m frame) with
> Intel I350T2 (SR-IOV capable, igb/igbvf) and Intel 82574L (e1000e)
> - functional on Cavium ThunderX (ARM GICv3 ITS) with Intel 82574L (e1000e)
>
> References:
> [1] [RFC 0/2] VFIO: Add virtual MSI doorbell support
> (https://lkml.org/lkml/2015/7/24/135)
> [2] [RFC PATCH 0/6] vfio: Add interface to map MSI pages
>
> (https://lists.cs.columbia.edu/pipermail/kvmarm/2015-September/016607.html)
> [3] [PATCH v2 0/3] Introduce MSI hardware mapping for VFIO
> (http://permalink.gmane.org/gmane.comp.emulators.kvm.arm.devel/3858)
>
> Git: complete series available at
> https://github.com/eauger/linux/tree/v4.7-rc7-passthrough-v12
> previous: https://github.com/eauger/linux/tree/v4.7-rc7-passthrough-v11
>
> the above branch includes a temporary patch to work around a ThunderX pci
> bus reset crash (which I think unrelated to this series):
> "vfio: pci: HACK! workaround thunderx pci_try_reset_bus crash"
> Do not take this one for other platforms.
>
> History:
> v11 -> v12:
> - no functional change. Only adapt to renamings done in PART II
>
> v10 -> v11:
> no change to this series, just incremented for consistency
>
> v9 -> v10:
> Took into account Alex' comments:
> - split "vfio/type1: vfio_find_dma accepting a type argument" into 2 patches
> - properly implement replay of MSI DMA slots (by setting the aperture on the
> new domain); fixes the assignment of several devices
> - rework user api for vfio_iommu_type1_info capability chains (cap_offset
> at the end of the struct and fix padding issues)
> - VFIO_IOVA_RESERVED renamed into VFIO_IOVA_MSI_RESERVED
> - explicit dma->type setting to VFIO_IOVA_USER
>
> v8 -> v9:
> - report MSI geometry through capability chain (last patch only);
> with the current
