From db406cc0ac2d5d8314dfceab8dce3bb4daac9268 Mon Sep 17 00:00:00 2001 From: Robin Murphy Date: Thu, 10 Aug 2017 13:11:49 -0600 Subject: vfio/type1: Cope with hardware MSI reserved regions For ARM-based systems with a GICv3 ITS to provide interrupt isolation, but hardware limitations which are worked around by having MSIs bypass SMMU translation (e.g. HiSilicon Hip06/Hip07), VFIO neglects to check for the IRQ_DOMAIN_FLAG_MSI_REMAP capability, (and thus erroneously demands unsafe_interrupts) if a software-managed MSI region is absent. Fix this by always checking for isolation capability at both the IRQ domain and IOMMU domain levels, rather than predicating that on whether MSIs require an IOMMU mapping (which was always slightly tenuous logic). Signed-off-by: Robin Murphy Tested-by: Shameer Kolothum Reviewed-by: Eric Auger Signed-off-by: Alex Williamson --- drivers/vfio/vfio_iommu_type1.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c index 8549cb111627..2328be628f21 100644 --- a/drivers/vfio/vfio_iommu_type1.c +++ b/drivers/vfio/vfio_iommu_type1.c @@ -1265,8 +1265,8 @@ static int vfio_iommu_type1_attach_group(void *iommu_data, INIT_LIST_HEAD(&domain->group_list); list_add(&group->next, &domain->group_list); - msi_remap = resv_msi ? irq_domain_check_msi_remap() : - iommu_capable(bus, IOMMU_CAP_INTR_REMAP); + msi_remap = 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", -- cgit From f203f7f1dbb298e1ed68f6c2f53715d13d5f3a0f Mon Sep 17 00:00:00 2001 From: Robin Murphy Date: Thu, 10 Aug 2017 13:11:50 -0600 Subject: vfio/type1: Give hardware MSI regions precedence If the IOMMU driver advertises 'real' reserved regions for MSIs, but still includes the software-managed region as well, we are currently blind to the former and will configure the IOMMU domain to map MSIs into the latter, which is unlikely to work as expected. Since it would take a ridiculous hardware topology for both regions to be valid (which would be rather difficult to support in general), we should be safe to assume that the presence of any hardware regions makes the software region irrelevant. However, the IOMMU driver might still advertise the software region by default, particularly if the hardware regions are filled in elsewhere by generic code, so it might not be fair for VFIO to be super-strict about not mixing them. To that end, make vfio_iommu_has_sw_msi() robust against the presence of both region types at once, so that we end up doing what is almost certainly right, rather than what is almost certainly wrong. Signed-off-by: Robin Murphy Tested-by: Shameer Kolothum Reviewed-by: Eric Auger Signed-off-by: Alex Williamson --- drivers/vfio/vfio_iommu_type1.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c index 2328be628f21..92155cce926d 100644 --- a/drivers/vfio/vfio_iommu_type1.c +++ b/drivers/vfio/vfio_iommu_type1.c @@ -1169,13 +1169,21 @@ static bool vfio_iommu_has_sw_msi(struct iommu_group *group, phys_addr_t *base) INIT_LIST_HEAD(&group_resv_regions); iommu_get_group_resv_regions(group, &group_resv_regions); list_for_each_entry(region, &group_resv_regions, list) { + /* + * The presence of any 'real' MSI regions should take + * precedence over the software-managed one if the + * IOMMU driver happens to advertise both types. + */ + if (region->type == IOMMU_RESV_MSI) { + ret = false; + break; + } + if (region->type == IOMMU_RESV_SW_MSI) { *base = region->start; ret = true; - goto out; } } -out: list_for_each_entry_safe(region, next, &group_resv_regions, list) kfree(region); return ret; -- cgit From d935ad91f07d20268fca97b1ddc56a816ac71826 Mon Sep 17 00:00:00 2001 From: Eric Auger Date: Fri, 11 Aug 2017 15:16:06 +0200 Subject: vfio: fix noiommu vfio_iommu_group_get reference count In vfio_iommu_group_get() we want to increase the reference count of the iommu group. In noiommu case, the group does not exist and is allocated. iommu_group_add_device() increases the group ref count. However we then call iommu_group_put() which decrements it. This leads to a "refcount_t: underflow WARN_ON". Only decrement the ref count in case of iommu_group_add_device failure. Signed-off-by: Eric Auger Signed-off-by: Alex Williamson --- drivers/vfio/vfio.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c index 330d50582f40..4ee4f361fe9f 100644 --- a/drivers/vfio/vfio.c +++ b/drivers/vfio/vfio.c @@ -138,9 +138,10 @@ struct iommu_group *vfio_iommu_group_get(struct device *dev) iommu_group_set_name(group, "vfio-noiommu"); iommu_group_set_iommudata(group, &noiommu, NULL); ret = iommu_group_add_device(group, dev); - iommu_group_put(group); - if (ret) + if (ret) { + iommu_group_put(group); return NULL; + } /* * Where to taint? At this point we've added an IOMMU group for a -- cgit From 6586b561a91cd80a91c8f107ed0d144feb3eadc2 Mon Sep 17 00:00:00 2001 From: Alex Williamson Date: Thu, 17 Aug 2017 22:10:20 -0600 Subject: vfio: Stall vfio_del_group_dev() for container group detach When the user unbinds the last device of a group from a vfio bus driver, the devices within that group should be available for other purposes. We currently have a race that makes this generally, but not always true. The device can be unbound from the vfio bus driver, but remaining IOMMU context of the group attached to the container can result in errors as the next driver configures DMA for the device. Wait for the group to be detached from the IOMMU backend before allowing the bus driver remove callback to complete. Signed-off-by: Alex Williamson --- drivers/vfio/vfio.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c index 4ee4f361fe9f..f5a86f651f38 100644 --- a/drivers/vfio/vfio.c +++ b/drivers/vfio/vfio.c @@ -85,6 +85,7 @@ struct vfio_group { struct list_head unbound_list; struct mutex unbound_lock; atomic_t opened; + wait_queue_head_t container_q; bool noiommu; struct kvm *kvm; struct blocking_notifier_head notifier; @@ -338,6 +339,7 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group) mutex_init(&group->unbound_lock); atomic_set(&group->container_users, 0); atomic_set(&group->opened, 0); + init_waitqueue_head(&group->container_q); group->iommu_group = iommu_group; #ifdef CONFIG_VFIO_NOIOMMU group->noiommu = (iommu_group_get_iommudata(iommu_group) == &noiommu); @@ -994,6 +996,23 @@ void *vfio_del_group_dev(struct device *dev) } } while (ret <= 0); + /* + * In order to support multiple devices per group, devices can be + * plucked from the group while other devices in the group are still + * in use. The container persists with this group and those remaining + * devices still attached. If the user creates an isolation violation + * by binding this device to another driver while the group is still in + * use, that's their fault. However, in the case of removing the last, + * or potentially the only, device in the group there can be no other + * in-use devices in the group. The user has done their due diligence + * and we should lay no claims to those devices. In order to do that, + * we need to make sure the group is detached from the container. + * Without this stall, we're potentially racing with a user process + * that may attempt to immediately bind this device to another driver. + */ + if (list_empty(&group->device_list)) + wait_event(group->container_q, !group->container); + vfio_group_put(group); return device_data; @@ -1299,6 +1318,7 @@ static void __vfio_group_unset_container(struct vfio_group *group) group->iommu_group); group->container = NULL; + wake_up(&group->container_q); list_del(&group->container_next); /* Detaching the last group deprivileges a container, remove iommu */ -- cgit From 417fb50d5516e8526769c16ff5b92de47adbe727 Mon Sep 17 00:00:00 2001 From: Arvind Yadav Date: Wed, 23 Aug 2017 22:47:15 +0530 Subject: vfio: platform: constify amba_id amba_id are not supposed to change at runtime. All functions working with const amba_id. So mark the non-const structs as const. Signed-off-by: Arvind Yadav Signed-off-by: Alex Williamson --- drivers/vfio/platform/vfio_amba.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/vfio/platform/vfio_amba.c b/drivers/vfio/platform/vfio_amba.c index 31372fbf6c5b..62dfbfeaabfc 100644 --- a/drivers/vfio/platform/vfio_amba.c +++ b/drivers/vfio/platform/vfio_amba.c @@ -93,7 +93,7 @@ static int vfio_amba_remove(struct amba_device *adev) return -EINVAL; } -static struct amba_id pl330_ids[] = { +static const struct amba_id pl330_ids[] = { { 0, 0 }, }; -- cgit