Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1394182 > unrolled thread
| Started by | Eric Auger <eric.auger@linaro.org> |
|---|---|
| First post | 2016-05-04 14:00 +0200 |
| Last post | 2016-05-11 15:00 +0200 |
| Articles | 20 on this page of 31 — 4 participants |
Back to article view | Back to linux.kernel
[PATCH v9 0/7] KVM PCIe/MSI passthrough on ARM/ARM64: kernel part 3/3: vfio changes Eric Auger <eric.auger@linaro.org> - 2016-05-04 14:00 +0200
[PATCH v9 5/7] vfio/type1: also check IRQ remapping capability at msi domain Eric Auger <eric.auger@linaro.org> - 2016-05-04 14:00 +0200
Re: [PATCH v9 5/7] vfio/type1: also check IRQ remapping capability at msi domain "Chalamarla, Tirumalesh" <Tirumalesh.Chalamarla@caviumnetworks.com> - 2016-05-05 21:30 +0200
Re: [PATCH v9 5/7] vfio/type1: also check IRQ remapping capability at msi domain Eric Auger <eric.auger@linaro.org> - 2016-05-09 10:10 +0200
Re: [PATCH v9 5/7] vfio/type1: also check IRQ remapping capability at msi domain Alex Williamson <alex.williamson@redhat.com> - 2016-05-10 00:50 +0200
Re: [PATCH v9 5/7] vfio/type1: also check IRQ remapping capability at msi domain Eric Auger <eric.auger@linaro.org> - 2016-05-10 18:20 +0200
Re: [PATCH v9 5/7] vfio/type1: also check IRQ remapping capability at msi domain Robin Murphy <robin.murphy@arm.com> - 2016-05-10 19:30 +0200
Re: [PATCH v9 5/7] vfio/type1: also check IRQ remapping capability at msi domain Eric Auger <eric.auger@linaro.org> - 2016-05-11 10:50 +0200
Re: [PATCH v9 5/7] vfio/type1: also check IRQ remapping capability at msi domain Robin Murphy <robin.murphy@arm.com> - 2016-05-11 11:40 +0200
Re: [PATCH v9 5/7] vfio/type1: also check IRQ remapping capability at msi domain Eric Auger <eric.auger@linaro.org> - 2016-05-11 11:50 +0200
Re: [PATCH v9 5/7] vfio/type1: also check IRQ remapping capability at msi domain Robin Murphy <robin.murphy@arm.com> - 2016-05-11 15:50 +0200
Re: [PATCH v9 5/7] vfio/type1: also check IRQ remapping capability at msi domain Eric Auger <eric.auger@linaro.org> - 2016-05-11 16:40 +0200
[PATCH v9 6/7] iommu/arm-smmu: do not advertise IOMMU_CAP_INTR_REMAP Eric Auger <eric.auger@linaro.org> - 2016-05-04 14:00 +0200
[PATCH v9 1/7] vfio: introduce a vfio_dma type field Eric Auger <eric.auger@linaro.org> - 2016-05-04 14:00 +0200
[PATCH v9 7/7] vfio/type1: return MSI geometry through VFIO_IOMMU_GET_INFO capability chains Eric Auger <eric.auger@linaro.org> - 2016-05-04 14:00 +0200
Re: [PATCH v9 7/7] vfio/type1: return MSI geometry through VFIO_IOMMU_GET_INFO capability chains Eric Auger <eric.auger@linaro.org> - 2016-05-04 14:10 +0200
Re: [PATCH v9 7/7] vfio/type1: return MSI geometry through VFIO_IOMMU_GET_INFO capability chains Alex Williamson <alex.williamson@redhat.com> - 2016-05-10 01:10 +0200
Re: [PATCH v9 7/7] vfio/type1: return MSI geometry through VFIO_IOMMU_GET_INFO capability chains Eric Auger <eric.auger@linaro.org> - 2016-05-10 19:00 +0200
Re: [PATCH v9 7/7] vfio/type1: return MSI geometry through VFIO_IOMMU_GET_INFO capability chains Alex Williamson <alex.williamson@redhat.com> - 2016-05-10 01:00 +0200
Re: [PATCH v9 7/7] vfio/type1: return MSI geometry through VFIO_IOMMU_GET_INFO capability chains Eric Auger <eric.auger@linaro.org> - 2016-05-10 18:40 +0200
[PATCH v9 2/7] vfio/type1: vfio_find_dma accepting a type argument Eric Auger <eric.auger@linaro.org> - 2016-05-04 14:00 +0200
Re: [PATCH v9 2/7] vfio/type1: vfio_find_dma accepting a type argument Alex Williamson <alex.williamson@redhat.com> - 2016-05-10 00:50 +0200
Re: [PATCH v9 2/7] vfio/type1: vfio_find_dma accepting a type argument Eric Auger <eric.auger@linaro.org> - 2016-05-10 17:00 +0200
[PATCH v9 4/7] vfio: allow reserved msi iova registration Eric Auger <eric.auger@linaro.org> - 2016-05-04 14:00 +0200
Re: [PATCH v9 4/7] vfio: allow reserved msi iova registration "Chalamarla, Tirumalesh" <Tirumalesh.Chalamarla@caviumnetworks.com> - 2016-05-05 21:40 +0200
Re: [PATCH v9 4/7] vfio: allow reserved msi iova registration Eric Auger <eric.auger@linaro.org> - 2016-05-09 10:00 +0200
Re: [PATCH v9 4/7] vfio: allow reserved msi iova registration Alex Williamson <alex.williamson@redhat.com> - 2016-05-10 17:40 +0200
Re: [PATCH v9 4/7] vfio: allow reserved msi iova registration Eric Auger <eric.auger@linaro.org> - 2016-05-10 17:40 +0200
[PATCH v9 3/7] vfio/type1: bypass unmap/unpin and replay for VFIO_IOVA_RESERVED slots Eric Auger <eric.auger@linaro.org> - 2016-05-04 14:00 +0200
Re: [PATCH v9 3/7] vfio/type1: bypass unmap/unpin and replay for VFIO_IOVA_RESERVED slots Alex Williamson <alex.williamson@redhat.com> - 2016-05-10 00:50 +0200
Re: [PATCH v9 3/7] vfio/type1: bypass unmap/unpin and replay for VFIO_IOVA_RESERVED slots Eric Auger <eric.auger@linaro.org> - 2016-05-11 15:00 +0200
Page 1 of 2 [1] 2 Next page →
| From | Eric Auger <eric.auger@linaro.org> |
|---|---|
| Date | 2016-05-04 14:00 +0200 |
| Subject | [PATCH v9 0/7] KVM PCIe/MSI passthrough on ARM/ARM64: kernel part 3/3: vfio changes |
| Message-ID | <rv3Db-38F-1@gated-at.bofh.it> |
This series allows the user-space to register a reserved IOVA domain.
This completes the kernel integration of the whole functionality on top
of part 1 (v9) & 2 (v8).
It also depends on [PATCH 1/3] iommu: Add MMIO mapping type series,
http://comments.gmane.org/gmane.linux.kernel.iommu/12869
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).
On ARM/ARM64, the IOMMU does not astract IRQ remapping. the modality is
abstracted on MSI controller side. The GICv3 ITS is the first controller
advertising the modality.
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 X540-T2 (SR-IOV capable)
- also tested on Armada-7040 using an intel IXGBE (82599ES) by
Yehuda Yitschak (v8)
- Not tested: ARM GICv3 ITS
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://git.linaro.org/people/eric.auger/linux.git/shortlog/refs/heads/v4.6-rc6-pcie-passthrough-v9
previous version at
https://git.linaro.org/people/eric.auger/linux.git/shortlog/refs/heads/v4.6-rc5-pcie-passthrough-v8
History:
v8 -> v9:
- report MSI geometry through capability chain (last patch only);
with the current limitation that an arbitrary number of 16 page
requirement is reported. To be improved later on.
v7 -> v8:
- use renamed msi-iommu API
- VFIO only responsible for setting the IOVA aperture
- use new DOMAIN_ATTR_MSI_GEOMETRY iommu domain attribute
v6 -> v7:
- vfio_find_dma now accepts a dma_type argument.
- should have recovered the capability to unmap the whole user IOVA range
- remove computation of nb IOVA pages -> will post a separate RFC for that
while respinning the QEMU part
RFC v5 -> patch v6:
- split to ease the review process
RFC v4 -> RFC v5:
- take into account Thomas' comments on MSI related patches
- split "msi: IOMMU map the doorbell address when needed"
- increase readability and add comments
- fix style issues
- split "iommu: Add DOMAIN_ATTR_MSI_MAPPING attribute"
- platform ITS now advertises IOMMU_CAP_INTR_REMAP
- fix compilation issue with CONFIG_IOMMU API unset
- arm-smmu-v3 now advertises DOMAIN_ATTR_MSI_MAPPING
RFC v3 -> v4:
- Move doorbell mapping/unmapping in msi.c
- fix ref count issue on set_affinity: in case of a change in the address
the previous address is decremented
- doorbell map/unmap now is done on msi composition. Should allow the use
case for platform MSI controllers
- create dma-reserved-iommu.h/c exposing/implementing a new API dedicated
to reserved IOVA management (looking like dma-iommu glue)
- series reordering to ease the review:
- first part is related to IOMMU
- second related to MSI sub-system
- third related to VFIO (except arm-smmu IOMMU_CAP_INTR_REMAP removal)
- expose the number of requested IOVA pages through VFIO_IOMMU_GET_INFO
[this partially addresses Marc's comments on iommu_get/put_single_reserved
size/alignment problematic - which I did not ignore - but I don't know
how much I can do at the moment]
RFC v2 -> RFC v3:
- should fix wrong handling of some CONFIG combinations:
CONFIG_IOVA, CONFIG_IOMMU_API, CONFIG_PCI_MSI_IRQ_DOMAIN
- fix MSI_FLAG_IRQ_REMAPPING setting in GICv3 ITS (although not tested)
PATCH v1 -> RFC v2:
- reverted to RFC since it looks more reasonable ;-) the code is split
between VFIO, IOMMU, MSI controller and I am not sure I did the right
choices. Also API need to be further discussed.
- iova API usage in arm-smmu.c.
- MSI controller natively programs the MSI addr with either the PA or IOVA.
This is not done anymore in vfio-pci driver as suggested by Alex.
- check irq remapping capability of the group
RFC v1 [2] -> PATCH v1:
- use the existing dma map/unmap ioctl interface with a flag to register a
reserved IOVA range. Use the legacy Rb to store this special vfio_dma.
- a single reserved IOVA contiguous region now is allowed
- use of an RB tree indexed by PA to store allocated reserved slots
- use of a vfio_domain iova_domain to manage iova allocation within the
window provided by the userspace
- vfio alloc_map/unmap_free take a vfio_group handle
- vfio_group handle is cached in vfio_pci_device
- add ref counting to bindings
- user modality enabled at the end of the series
Eric Auger (7):
vfio: introduce a vfio_dma type field
vfio/type1: vfio_find_dma accepting a type argument
vfio/type1: bypass unmap/unpin and replay for VFIO_IOVA_RESERVED slots
vfio: allow reserved msi iova registration
vfio/type1: also check IRQ remapping capability at msi domain
iommu/arm-smmu: do not advertise IOMMU_CAP_INTR_REMAP
vfio/type1: return MSI geometry through VFIO_IOMMU_GET_INFO capability
chains
drivers/iommu/arm-smmu-v3.c | 3 +-
drivers/iommu/arm-smmu.c | 3 +-
drivers/vfio/vfio_iommu_type1.c | 270 +++++++++++++++++++++++++++++++++++++---
include/uapi/linux/vfio.h | 40 +++++-
4 files changed, 298 insertions(+), 18 deletions(-)
--
1.9.1
[toc] | [next] | [standalone]
| From | Eric Auger <eric.auger@linaro.org> |
|---|---|
| Date | 2016-05-04 14:00 +0200 |
| Subject | [PATCH v9 5/7] vfio/type1: also check IRQ remapping capability at msi domain |
| Message-ID | <rv3Dc-38F-3@gated-at.bofh.it> |
| In reply to | #1394182 |
On x86 IRQ remapping is abstracted by the IOMMU. On ARM this is abstracted
by the msi controller. vfio_safe_irq_domain allows to check whether
interrupts are "safe" for a given device. They are if the device does
not use MSI or if the device uses MSI and the msi-parent controller
supports IRQ remapping.
Then we check at group level if all devices have safe interrupts: if not,
we only allow the group to be attached if allow_unsafe_interrupts is set.
At this point ARM sMMU still advertises IOMMU_CAP_INTR_REMAP. This is
changed in next patch.
Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
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 | 44 +++++++++++++++++++++++++++++++++++++++--
1 file changed, 42 insertions(+), 2 deletions(-)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 4d3a6f1..2fc8197 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -37,6 +37,8 @@
#include <linux/vfio.h>
#include <linux/workqueue.h>
#include <linux/msi-iommu.h>
+#include <linux/irqdomain.h>
+#include <linux/msi.h>
#define DRIVER_VERSION "0.2"
#define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>"
@@ -777,6 +779,33 @@ static int vfio_bus_type(struct device *dev, void *data)
return 0;
}
+/**
+ * vfio_safe_irq_domain: returns whether the irq domain
+ * the device is attached to is safe with respect to MSI isolation.
+ * If the irq domain is not an MSI domain, we return it is safe.
+ *
+ * @dev: device handle
+ * @data: unused
+ * returns 0 if the irq domain is safe, -1 if not.
+ */
+static int vfio_safe_irq_domain(struct device *dev, void *data)
+{
+#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
+ struct irq_domain *domain;
+ struct msi_domain_info *info;
+
+ domain = dev_get_msi_domain(dev);
+ if (!domain)
+ return 0;
+
+ info = msi_get_domain_info(domain);
+
+ if (!(info->flags & MSI_FLAG_IRQ_REMAPPING))
+ return -1;
+#endif
+ return 0;
+}
+
static int vfio_iommu_replay(struct vfio_iommu *iommu,
struct vfio_domain *domain)
{
@@ -870,7 +899,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
struct vfio_group *group, *g;
struct vfio_domain *domain, *d;
struct bus_type *bus = NULL;
- int ret;
+ int ret, safe_irq_domains;
mutex_lock(&iommu->lock);
@@ -893,6 +922,13 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
group->iommu_group = iommu_group;
+ /*
+ * Determine if all the devices of the group have a safe irq domain
+ * with respect to MSI isolation
+ */
+ safe_irq_domains = !iommu_group_for_each_dev(iommu_group, &bus,
+ vfio_safe_irq_domain);
+
/* Determine bus_type in order to allocate a domain */
ret = iommu_group_for_each_dev(iommu_group, &bus, vfio_bus_type);
if (ret)
@@ -920,8 +956,12 @@ 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/interrupt translation
+ */
if (!allow_unsafe_interrupts &&
- !iommu_capable(bus, IOMMU_CAP_INTR_REMAP)) {
+ (!iommu_capable(bus, IOMMU_CAP_INTR_REMAP) && !safe_irq_domains)) {
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;
--
1.9.1
[toc] | [prev] | [next] | [standalone]
| From | "Chalamarla, Tirumalesh" <Tirumalesh.Chalamarla@caviumnetworks.com> |
|---|---|
| Date | 2016-05-05 21:30 +0200 |
| Subject | Re: [PATCH v9 5/7] vfio/type1: also check IRQ remapping capability at msi domain |
| Message-ID | <rvx8e-67B-9@gated-at.bofh.it> |
| In reply to | #1394183 |
On 5/4/16, 4:54 AM, "linux-arm-kernel on behalf of Eric Auger" <linux-arm-kernel-bounces@lists.infradead.org on behalf of eric.auger@linaro.org> wrote:
>On x86 IRQ remapping is abstracted by the IOMMU. On ARM this is abstracted
>by the msi controller. vfio_safe_irq_domain allows to check whether
>interrupts are "safe" for a given device. They are if the device does
>not use MSI or if the device uses MSI and the msi-parent controller
>supports IRQ remapping.
>
>Then we check at group level if all devices have safe interrupts: if not,
>we only allow the group to be attached if allow_unsafe_interrupts is set.
>
>At this point ARM sMMU still advertises IOMMU_CAP_INTR_REMAP. This is
>changed in next patch.
Will this work in systems with multiple ITS?
>
>Signed-off-by: Eric Auger <eric.auger@linaro.org>
>
>---
>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 | 44 +++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 42 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>index 4d3a6f1..2fc8197 100644
>--- a/drivers/vfio/vfio_iommu_type1.c
>+++ b/drivers/vfio/vfio_iommu_type1.c
>@@ -37,6 +37,8 @@
> #include <linux/vfio.h>
> #include <linux/workqueue.h>
> #include <linux/msi-iommu.h>
>+#include <linux/irqdomain.h>
>+#include <linux/msi.h>
>
> #define DRIVER_VERSION "0.2"
> #define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>"
>@@ -777,6 +779,33 @@ static int vfio_bus_type(struct device *dev, void *data)
> return 0;
> }
>
>+/**
>+ * vfio_safe_irq_domain: returns whether the irq domain
>+ * the device is attached to is safe with respect to MSI isolation.
>+ * If the irq domain is not an MSI domain, we return it is safe.
>+ *
>+ * @dev: device handle
>+ * @data: unused
>+ * returns 0 if the irq domain is safe, -1 if not.
>+ */
>+static int vfio_safe_irq_domain(struct device *dev, void *data)
>+{
>+#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
>+ struct irq_domain *domain;
>+ struct msi_domain_info *info;
>+
>+ domain = dev_get_msi_domain(dev);
>+ if (!domain)
>+ return 0;
>+
>+ info = msi_get_domain_info(domain);
>+
>+ if (!(info->flags & MSI_FLAG_IRQ_REMAPPING))
>+ return -1;
>+#endif
>+ return 0;
>+}
>+
> static int vfio_iommu_replay(struct vfio_iommu *iommu,
> struct vfio_domain *domain)
> {
>@@ -870,7 +899,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
> struct vfio_group *group, *g;
> struct vfio_domain *domain, *d;
> struct bus_type *bus = NULL;
>- int ret;
>+ int ret, safe_irq_domains;
>
> mutex_lock(&iommu->lock);
>
>@@ -893,6 +922,13 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>
> group->iommu_group = iommu_group;
>
>+ /*
>+ * Determine if all the devices of the group have a safe irq domain
>+ * with respect to MSI isolation
>+ */
>+ safe_irq_domains = !iommu_group_for_each_dev(iommu_group, &bus,
>+ vfio_safe_irq_domain);
>+
> /* Determine bus_type in order to allocate a domain */
> ret = iommu_group_for_each_dev(iommu_group, &bus, vfio_bus_type);
> if (ret)
>@@ -920,8 +956,12 @@ 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/interrupt translation
>+ */
> if (!allow_unsafe_interrupts &&
>- !iommu_capable(bus, IOMMU_CAP_INTR_REMAP)) {
>+ (!iommu_capable(bus, IOMMU_CAP_INTR_REMAP) && !safe_irq_domains)) {
> 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;
>--
>1.9.1
>
>
>_______________________________________________
>linux-arm-kernel mailing list
>linux-arm-kernel@lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
[toc] | [prev] | [next] | [standalone]
| From | Eric Auger <eric.auger@linaro.org> |
|---|---|
| Date | 2016-05-09 10:10 +0200 |
| Subject | Re: [PATCH v9 5/7] vfio/type1: also check IRQ remapping capability at msi domain |
| Message-ID | <rwOqn-1cZ-39@gated-at.bofh.it> |
| In reply to | #1395317 |
Hi Chalamarla,
On 05/05/2016 09:23 PM, Chalamarla, Tirumalesh wrote:
>
>
>
>
>
> On 5/4/16, 4:54 AM, "linux-arm-kernel on behalf of Eric Auger" <linux-arm-kernel-bounces@lists.infradead.org on behalf of eric.auger@linaro.org> wrote:
>
>> On x86 IRQ remapping is abstracted by the IOMMU. On ARM this is abstracted
>> by the msi controller. vfio_safe_irq_domain allows to check whether
>> interrupts are "safe" for a given device. They are if the device does
>> not use MSI or if the device uses MSI and the msi-parent controller
>> supports IRQ remapping.
>>
>> Then we check at group level if all devices have safe interrupts: if not,
>> we only allow the group to be attached if allow_unsafe_interrupts is set.
>>
>> At this point ARM sMMU still advertises IOMMU_CAP_INTR_REMAP. This is
>> changed in next patch.
>
> Will this work in systems with multiple ITS?
Yes it should support multiple ITS.
Please note however that the series does not yet implement the
msi_doorbell_info callback in GICv3 ITS PCIe/platform irqchip. Without
that, the pci_enable is going to fail. I am going to submit a separate
RFC for that but I can't test it at the moment.
Best Regards
Eric
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>> 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 | 44 +++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 42 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>> index 4d3a6f1..2fc8197 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -37,6 +37,8 @@
>> #include <linux/vfio.h>
>> #include <linux/workqueue.h>
>> #include <linux/msi-iommu.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/msi.h>
>>
>> #define DRIVER_VERSION "0.2"
>> #define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>"
>> @@ -777,6 +779,33 @@ static int vfio_bus_type(struct device *dev, void *data)
>> return 0;
>> }
>>
>> +/**
>> + * vfio_safe_irq_domain: returns whether the irq domain
>> + * the device is attached to is safe with respect to MSI isolation.
>> + * If the irq domain is not an MSI domain, we return it is safe.
>> + *
>> + * @dev: device handle
>> + * @data: unused
>> + * returns 0 if the irq domain is safe, -1 if not.
>> + */
>> +static int vfio_safe_irq_domain(struct device *dev, void *data)
>> +{
>> +#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
>> + struct irq_domain *domain;
>> + struct msi_domain_info *info;
>> +
>> + domain = dev_get_msi_domain(dev);
>> + if (!domain)
>> + return 0;
>> +
>> + info = msi_get_domain_info(domain);
>> +
>> + if (!(info->flags & MSI_FLAG_IRQ_REMAPPING))
>> + return -1;
>> +#endif
>> + return 0;
>> +}
>> +
>> static int vfio_iommu_replay(struct vfio_iommu *iommu,
>> struct vfio_domain *domain)
>> {
>> @@ -870,7 +899,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>> struct vfio_group *group, *g;
>> struct vfio_domain *domain, *d;
>> struct bus_type *bus = NULL;
>> - int ret;
>> + int ret, safe_irq_domains;
>>
>> mutex_lock(&iommu->lock);
>>
>> @@ -893,6 +922,13 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>>
>> group->iommu_group = iommu_group;
>>
>> + /*
>> + * Determine if all the devices of the group have a safe irq domain
>> + * with respect to MSI isolation
>> + */
>> + safe_irq_domains = !iommu_group_for_each_dev(iommu_group, &bus,
>> + vfio_safe_irq_domain);
>> +
>> /* Determine bus_type in order to allocate a domain */
>> ret = iommu_group_for_each_dev(iommu_group, &bus, vfio_bus_type);
>> if (ret)
>> @@ -920,8 +956,12 @@ 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/interrupt translation
>> + */
>> if (!allow_unsafe_interrupts &&
>> - !iommu_capable(bus, IOMMU_CAP_INTR_REMAP)) {
>> + (!iommu_capable(bus, IOMMU_CAP_INTR_REMAP) && !safe_irq_domains)) {
>> 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;
>> --
>> 1.9.1
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
[toc] | [prev] | [next] | [standalone]
| From | Alex Williamson <alex.williamson@redhat.com> |
|---|---|
| Date | 2016-05-10 00:50 +0200 |
| Subject | Re: [PATCH v9 5/7] vfio/type1: also check IRQ remapping capability at msi domain |
| Message-ID | <rx29Y-6uv-13@gated-at.bofh.it> |
| In reply to | #1394183 |
On Wed, 4 May 2016 11:54:16 +0000
Eric Auger <eric.auger@linaro.org> wrote:
> On x86 IRQ remapping is abstracted by the IOMMU. On ARM this is abstracted
> by the msi controller. vfio_safe_irq_domain allows to check whether
> interrupts are "safe" for a given device. They are if the device does
> not use MSI
Are we sure we're not opening a security hole here? An MSI is simply a
DMA write, so really whether or not a device uses MSI is irrelevant.
If it can generate a DMA to the MSI doorbell then we need to be
protected and I think we pretty much need to assume that devices are
DMA capable. Do the MSI domain checks cover this?
> or if the device uses MSI and the msi-parent controller
> supports IRQ remapping.
>
> Then we check at group level if all devices have safe interrupts: if not,
> we only allow the group to be attached if allow_unsafe_interrupts is set.
>
> At this point ARM sMMU still advertises IOMMU_CAP_INTR_REMAP. This is
> changed in next patch.
>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>
> ---
> 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 | 44 +++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 42 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 4d3a6f1..2fc8197 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -37,6 +37,8 @@
> #include <linux/vfio.h>
> #include <linux/workqueue.h>
> #include <linux/msi-iommu.h>
> +#include <linux/irqdomain.h>
> +#include <linux/msi.h>
>
> #define DRIVER_VERSION "0.2"
> #define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>"
> @@ -777,6 +779,33 @@ static int vfio_bus_type(struct device *dev, void *data)
> return 0;
> }
>
> +/**
> + * vfio_safe_irq_domain: returns whether the irq domain
> + * the device is attached to is safe with respect to MSI isolation.
> + * If the irq domain is not an MSI domain, we return it is safe.
> + *
> + * @dev: device handle
> + * @data: unused
> + * returns 0 if the irq domain is safe, -1 if not.
> + */
> +static int vfio_safe_irq_domain(struct device *dev, void *data)
> +{
> +#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
> + struct irq_domain *domain;
> + struct msi_domain_info *info;
> +
> + domain = dev_get_msi_domain(dev);
> + if (!domain)
> + return 0;
> +
> + info = msi_get_domain_info(domain);
> +
> + if (!(info->flags & MSI_FLAG_IRQ_REMAPPING))
> + return -1;
> +#endif
> + return 0;
> +}
> +
> static int vfio_iommu_replay(struct vfio_iommu *iommu,
> struct vfio_domain *domain)
> {
> @@ -870,7 +899,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
> struct vfio_group *group, *g;
> struct vfio_domain *domain, *d;
> struct bus_type *bus = NULL;
> - int ret;
> + int ret, safe_irq_domains;
>
> mutex_lock(&iommu->lock);
>
> @@ -893,6 +922,13 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>
> group->iommu_group = iommu_group;
>
> + /*
> + * Determine if all the devices of the group have a safe irq domain
> + * with respect to MSI isolation
> + */
> + safe_irq_domains = !iommu_group_for_each_dev(iommu_group, &bus,
> + vfio_safe_irq_domain);
> +
> /* Determine bus_type in order to allocate a domain */
> ret = iommu_group_for_each_dev(iommu_group, &bus, vfio_bus_type);
> if (ret)
> @@ -920,8 +956,12 @@ 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/interrupt translation
> + */
> if (!allow_unsafe_interrupts &&
> - !iommu_capable(bus, IOMMU_CAP_INTR_REMAP)) {
> + (!iommu_capable(bus, IOMMU_CAP_INTR_REMAP) && !safe_irq_domains)) {
> 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;
[toc] | [prev] | [next] | [standalone]
| From | Eric Auger <eric.auger@linaro.org> |
|---|---|
| Date | 2016-05-10 18:20 +0200 |
| Subject | Re: [PATCH v9 5/7] vfio/type1: also check IRQ remapping capability at msi domain |
| Message-ID | <rxiy6-5Zu-19@gated-at.bofh.it> |
| In reply to | #1397491 |
Hi Alex,
On 05/10/2016 12:49 AM, Alex Williamson wrote:
> On Wed, 4 May 2016 11:54:16 +0000
> Eric Auger <eric.auger@linaro.org> wrote:
>
>> On x86 IRQ remapping is abstracted by the IOMMU. On ARM this is abstracted
>> by the msi controller. vfio_safe_irq_domain allows to check whether
>> interrupts are "safe" for a given device. They are if the device does
>> not use MSI
>
> Are we sure we're not opening a security hole here? An MSI is simply a
> DMA write, so really whether or not a device uses MSI is irrelevant.
> If it can generate a DMA to the MSI doorbell then we need to be
> protected and I think we pretty much need to assume that devices are
> DMA capable. Do the MSI domain checks cover this?
Let me try to rephrase: we check the device is not attached to an MSI
controller (I think this is the semantic of dev_get_msi_domain(dev)).
If it is not, we don't have to care about MSI isolation: there will be
no IOMMU binding between the device and any MSI doorbell. If it is we
check the msi domain is backed by an MSI controller able to perform MSI
isolation.
So effectively "usage of MSIs" is improper - since it is decided after
the group attachment anyway - and the commit message should rather
state "if the device is linked to an MSI controller" (dt msi-parent
notion I think).
Does it sound better?
Regards
Eric
>
>> or if the device uses MSI and the msi-parent controller
>> supports IRQ remapping.
>>
>> Then we check at group level if all devices have safe interrupts: if not,
>> we only allow the group to be attached if allow_unsafe_interrupts is set.
>>
>> At this point ARM sMMU still advertises IOMMU_CAP_INTR_REMAP. This is
>> changed in next patch.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>> 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 | 44 +++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 42 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>> index 4d3a6f1..2fc8197 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -37,6 +37,8 @@
>> #include <linux/vfio.h>
>> #include <linux/workqueue.h>
>> #include <linux/msi-iommu.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/msi.h>
>>
>> #define DRIVER_VERSION "0.2"
>> #define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>"
>> @@ -777,6 +779,33 @@ static int vfio_bus_type(struct device *dev, void *data)
>> return 0;
>> }
>>
>> +/**
>> + * vfio_safe_irq_domain: returns whether the irq domain
>> + * the device is attached to is safe with respect to MSI isolation.
>> + * If the irq domain is not an MSI domain, we return it is safe.
>> + *
>> + * @dev: device handle
>> + * @data: unused
>> + * returns 0 if the irq domain is safe, -1 if not.
>> + */
>> +static int vfio_safe_irq_domain(struct device *dev, void *data)
>> +{
>> +#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
>> + struct irq_domain *domain;
>> + struct msi_domain_info *info;
>> +
>> + domain = dev_get_msi_domain(dev);
>> + if (!domain)
>> + return 0;
>> +
>> + info = msi_get_domain_info(domain);
>> +
>> + if (!(info->flags & MSI_FLAG_IRQ_REMAPPING))
>> + return -1;
>> +#endif
>> + return 0;
>> +}
>> +
>> static int vfio_iommu_replay(struct vfio_iommu *iommu,
>> struct vfio_domain *domain)
>> {
>> @@ -870,7 +899,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>> struct vfio_group *group, *g;
>> struct vfio_domain *domain, *d;
>> struct bus_type *bus = NULL;
>> - int ret;
>> + int ret, safe_irq_domains;
>>
>> mutex_lock(&iommu->lock);
>>
>> @@ -893,6 +922,13 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>>
>> group->iommu_group = iommu_group;
>>
>> + /*
>> + * Determine if all the devices of the group have a safe irq domain
>> + * with respect to MSI isolation
>> + */
>> + safe_irq_domains = !iommu_group_for_each_dev(iommu_group, &bus,
>> + vfio_safe_irq_domain);
>> +
>> /* Determine bus_type in order to allocate a domain */
>> ret = iommu_group_for_each_dev(iommu_group, &bus, vfio_bus_type);
>> if (ret)
>> @@ -920,8 +956,12 @@ 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/interrupt translation
>> + */
>> if (!allow_unsafe_interrupts &&
>> - !iommu_capable(bus, IOMMU_CAP_INTR_REMAP)) {
>> + (!iommu_capable(bus, IOMMU_CAP_INTR_REMAP) && !safe_irq_domains)) {
>> 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;
>
[toc] | [prev] | [next] | [standalone]
| From | Robin Murphy <robin.murphy@arm.com> |
|---|---|
| Date | 2016-05-10 19:30 +0200 |
| Subject | Re: [PATCH v9 5/7] vfio/type1: also check IRQ remapping capability at msi domain |
| Message-ID | <rxjDP-75T-5@gated-at.bofh.it> |
| In reply to | #1398296 |
Hi Eric,
On 10/05/16 17:10, Eric Auger wrote:
> Hi Alex,
> On 05/10/2016 12:49 AM, Alex Williamson wrote:
>> On Wed, 4 May 2016 11:54:16 +0000
>> Eric Auger <eric.auger@linaro.org> wrote:
>>
>>> On x86 IRQ remapping is abstracted by the IOMMU. On ARM this is abstracted
>>> by the msi controller. vfio_safe_irq_domain allows to check whether
>>> interrupts are "safe" for a given device. They are if the device does
>>> not use MSI
>>
>> Are we sure we're not opening a security hole here? An MSI is simply a
>> DMA write, so really whether or not a device uses MSI is irrelevant.
>> If it can generate a DMA to the MSI doorbell then we need to be
>> protected and I think we pretty much need to assume that devices are
>> DMA capable. Do the MSI domain checks cover this?
> Let me try to rephrase: we check the device is not attached to an MSI
> controller (I think this is the semantic of dev_get_msi_domain(dev)).
>
> If it is not, we don't have to care about MSI isolation: there will be
> no IOMMU binding between the device and any MSI doorbell. If it is we
> check the msi domain is backed by an MSI controller able to perform MSI
> isolation.
>
> So effectively "usage of MSIs" is improper - since it is decided after
> the group attachment anyway - and the commit message should rather
> state "if the device is linked to an MSI controller" (dt msi-parent
> notion I think).
Hmm, I think Alex has a point here - on a GICv2m I can happily fire
arbitrary MSIs from _a shell_ (using /dev/mem), and the CPUs definitely
aren't in an MSI domain, so I don't think it's valid to assume that a
device using only wired interrupts, therefore with no connection to any
MSI controller, isn't still capable of maliciously spewing DMA all over
any and every doorbell region in the system.
Robin.
> Does it sound better?
>
> Regards
>
> Eric
>>
>>> or if the device uses MSI and the msi-parent controller
>>> supports IRQ remapping.
>>>
>>> Then we check at group level if all devices have safe interrupts: if not,
>>> we only allow the group to be attached if allow_unsafe_interrupts is set.
>>>
>>> At this point ARM sMMU still advertises IOMMU_CAP_INTR_REMAP. This is
>>> changed in next patch.
>>>
>>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>>
>>> ---
>>> 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 | 44 +++++++++++++++++++++++++++++++++++++++--
>>> 1 file changed, 42 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>>> index 4d3a6f1..2fc8197 100644
>>> --- a/drivers/vfio/vfio_iommu_type1.c
>>> +++ b/drivers/vfio/vfio_iommu_type1.c
>>> @@ -37,6 +37,8 @@
>>> #include <linux/vfio.h>
>>> #include <linux/workqueue.h>
>>> #include <linux/msi-iommu.h>
>>> +#include <linux/irqdomain.h>
>>> +#include <linux/msi.h>
>>>
>>> #define DRIVER_VERSION "0.2"
>>> #define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>"
>>> @@ -777,6 +779,33 @@ static int vfio_bus_type(struct device *dev, void *data)
>>> return 0;
>>> }
>>>
>>> +/**
>>> + * vfio_safe_irq_domain: returns whether the irq domain
>>> + * the device is attached to is safe with respect to MSI isolation.
>>> + * If the irq domain is not an MSI domain, we return it is safe.
>>> + *
>>> + * @dev: device handle
>>> + * @data: unused
>>> + * returns 0 if the irq domain is safe, -1 if not.
>>> + */
>>> +static int vfio_safe_irq_domain(struct device *dev, void *data)
>>> +{
>>> +#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
>>> + struct irq_domain *domain;
>>> + struct msi_domain_info *info;
>>> +
>>> + domain = dev_get_msi_domain(dev);
>>> + if (!domain)
>>> + return 0;
>>> +
>>> + info = msi_get_domain_info(domain);
>>> +
>>> + if (!(info->flags & MSI_FLAG_IRQ_REMAPPING))
>>> + return -1;
>>> +#endif
>>> + return 0;
>>> +}
>>> +
>>> static int vfio_iommu_replay(struct vfio_iommu *iommu,
>>> struct vfio_domain *domain)
>>> {
>>> @@ -870,7 +899,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>>> struct vfio_group *group, *g;
>>> struct vfio_domain *domain, *d;
>>> struct bus_type *bus = NULL;
>>> - int ret;
>>> + int ret, safe_irq_domains;
>>>
>>> mutex_lock(&iommu->lock);
>>>
>>> @@ -893,6 +922,13 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>>>
>>> group->iommu_group = iommu_group;
>>>
>>> + /*
>>> + * Determine if all the devices of the group have a safe irq domain
>>> + * with respect to MSI isolation
>>> + */
>>> + safe_irq_domains = !iommu_group_for_each_dev(iommu_group, &bus,
>>> + vfio_safe_irq_domain);
>>> +
>>> /* Determine bus_type in order to allocate a domain */
>>> ret = iommu_group_for_each_dev(iommu_group, &bus, vfio_bus_type);
>>> if (ret)
>>> @@ -920,8 +956,12 @@ 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/interrupt translation
>>> + */
>>> if (!allow_unsafe_interrupts &&
>>> - !iommu_capable(bus, IOMMU_CAP_INTR_REMAP)) {
>>> + (!iommu_capable(bus, IOMMU_CAP_INTR_REMAP) && !safe_irq_domains)) {
>>> 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;
>>
>
[toc] | [prev] | [next] | [standalone]
| From | Eric Auger <eric.auger@linaro.org> |
|---|---|
| Date | 2016-05-11 10:50 +0200 |
| Subject | Re: [PATCH v9 5/7] vfio/type1: also check IRQ remapping capability at msi domain |
| Message-ID | <rxy0b-4vB-37@gated-at.bofh.it> |
| In reply to | #1398343 |
Hi Robin, Alex,
On 05/10/2016 07:24 PM, Robin Murphy wrote:
> Hi Eric,
>
> On 10/05/16 17:10, Eric Auger wrote:
>> Hi Alex,
>> On 05/10/2016 12:49 AM, Alex Williamson wrote:
>>> On Wed, 4 May 2016 11:54:16 +0000
>>> Eric Auger <eric.auger@linaro.org> wrote:
>>>
>>>> On x86 IRQ remapping is abstracted by the IOMMU. On ARM this is
>>>> abstracted
>>>> by the msi controller. vfio_safe_irq_domain allows to check whether
>>>> interrupts are "safe" for a given device. They are if the device does
>>>> not use MSI
>>>
>>> Are we sure we're not opening a security hole here? An MSI is simply a
>>> DMA write, so really whether or not a device uses MSI is irrelevant.
>>> If it can generate a DMA to the MSI doorbell then we need to be
>>> protected and I think we pretty much need to assume that devices are
>>> DMA capable. Do the MSI domain checks cover this?
>> Let me try to rephrase: we check the device is not attached to an MSI
>> controller (I think this is the semantic of dev_get_msi_domain(dev)).
>>
>> If it is not, we don't have to care about MSI isolation: there will be
>> no IOMMU binding between the device and any MSI doorbell. If it is we
>> check the msi domain is backed by an MSI controller able to perform MSI
>> isolation.
>>
>> So effectively "usage of MSIs" is improper - since it is decided after
>> the group attachment anyway - and the commit message should rather
>> state "if the device is linked to an MSI controller" (dt msi-parent
>> notion I think).
>
> Hmm, I think Alex has a point here - on a GICv2m I can happily fire
> arbitrary MSIs from _a shell_ (using /dev/mem), and the CPUs definitely
> aren't in an MSI domain, so I don't think it's valid to assume that a
> device using only wired interrupts, therefore with no connection to any
> MSI controller, isn't still capable of maliciously spewing DMA all over
> any and every doorbell region in the system.
Sorry but I still don't get the point. For the device to reach the
doorbell there must be an IOMMU mapping.
- if the device is not attached to an MSI domain, there won't be any
doorbell iommu mapping built by this series, so no risk, right?
The device will be allowed to reach only memory iommu mapped by
userspace with VFIO DMA MAP standard API. Of course if the userspace can
mmap all the host PA that's a more general issue, right?
- If the device is attached to an MSI domain (msi-parent link), 2 cases:
1) the MSI controller advertises MSI isolation (ITS cases), no risk
2) the MSI controller does not advertise MSI isolation (GICv2m), there
is a security hole.
a) by default we reject the device attachment
b) if the userspace overrides the safe interrupt option he accepts
the security hole
What am I missing?
Best Regards
Eric
>
> Robin.
>
>> Does it sound better?
>>
>> Regards
>>
>> Eric
>>>
>>>> or if the device uses MSI and the msi-parent controller
>>>> supports IRQ remapping.
>>>>
>>>> Then we check at group level if all devices have safe interrupts: if
>>>> not,
>>>> we only allow the group to be attached if allow_unsafe_interrupts is
>>>> set.
>>>>
>>>> At this point ARM sMMU still advertises IOMMU_CAP_INTR_REMAP. This is
>>>> changed in next patch.
>>>>
>>>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>>>
>>>> ---
>>>> 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 | 44
>>>> +++++++++++++++++++++++++++++++++++++++--
>>>> 1 file changed, 42 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/vfio/vfio_iommu_type1.c
>>>> b/drivers/vfio/vfio_iommu_type1.c
>>>> index 4d3a6f1..2fc8197 100644
>>>> --- a/drivers/vfio/vfio_iommu_type1.c
>>>> +++ b/drivers/vfio/vfio_iommu_type1.c
>>>> @@ -37,6 +37,8 @@
>>>> #include <linux/vfio.h>
>>>> #include <linux/workqueue.h>
>>>> #include <linux/msi-iommu.h>
>>>> +#include <linux/irqdomain.h>
>>>> +#include <linux/msi.h>
>>>>
>>>> #define DRIVER_VERSION "0.2"
>>>> #define DRIVER_AUTHOR "Alex Williamson
>>>> <alex.williamson@redhat.com>"
>>>> @@ -777,6 +779,33 @@ static int vfio_bus_type(struct device *dev,
>>>> void *data)
>>>> return 0;
>>>> }
>>>>
>>>> +/**
>>>> + * vfio_safe_irq_domain: returns whether the irq domain
>>>> + * the device is attached to is safe with respect to MSI isolation.
>>>> + * If the irq domain is not an MSI domain, we return it is safe.
>>>> + *
>>>> + * @dev: device handle
>>>> + * @data: unused
>>>> + * returns 0 if the irq domain is safe, -1 if not.
>>>> + */
>>>> +static int vfio_safe_irq_domain(struct device *dev, void *data)
>>>> +{
>>>> +#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
>>>> + struct irq_domain *domain;
>>>> + struct msi_domain_info *info;
>>>> +
>>>> + domain = dev_get_msi_domain(dev);
>>>> + if (!domain)
>>>> + return 0;
>>>> +
>>>> + info = msi_get_domain_info(domain);
>>>> +
>>>> + if (!(info->flags & MSI_FLAG_IRQ_REMAPPING))
>>>> + return -1;
>>>> +#endif
>>>> + return 0;
>>>> +}
>>>> +
>>>> static int vfio_iommu_replay(struct vfio_iommu *iommu,
>>>> struct vfio_domain *domain)
>>>> {
>>>> @@ -870,7 +899,7 @@ static int vfio_iommu_type1_attach_group(void
>>>> *iommu_data,
>>>> struct vfio_group *group, *g;
>>>> struct vfio_domain *domain, *d;
>>>> struct bus_type *bus = NULL;
>>>> - int ret;
>>>> + int ret, safe_irq_domains;
>>>>
>>>> mutex_lock(&iommu->lock);
>>>>
>>>> @@ -893,6 +922,13 @@ static int vfio_iommu_type1_attach_group(void
>>>> *iommu_data,
>>>>
>>>> group->iommu_group = iommu_group;
>>>>
>>>> + /*
>>>> + * Determine if all the devices of the group have a safe irq
>>>> domain
>>>> + * with respect to MSI isolation
>>>> + */
>>>> + safe_irq_domains = !iommu_group_for_each_dev(iommu_group, &bus,
>>>> + vfio_safe_irq_domain);
>>>> +
>>>> /* Determine bus_type in order to allocate a domain */
>>>> ret = iommu_group_for_each_dev(iommu_group, &bus, vfio_bus_type);
>>>> if (ret)
>>>> @@ -920,8 +956,12 @@ 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/interrupt translation
>>>> + */
>>>> if (!allow_unsafe_interrupts &&
>>>> - !iommu_capable(bus, IOMMU_CAP_INTR_REMAP)) {
>>>> + (!iommu_capable(bus, IOMMU_CAP_INTR_REMAP) &&
>>>> !safe_irq_domains)) {
>>>> 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;
>>>
>>
>
[toc] | [prev] | [next] | [standalone]
| From | Robin Murphy <robin.murphy@arm.com> |
|---|---|
| Date | 2016-05-11 11:40 +0200 |
| Subject | Re: [PATCH v9 5/7] vfio/type1: also check IRQ remapping capability at msi domain |
| Message-ID | <rxyMy-5vM-23@gated-at.bofh.it> |
| In reply to | #1398799 |
On 11/05/16 09:38, Eric Auger wrote:
> Hi Robin, Alex,
> On 05/10/2016 07:24 PM, Robin Murphy wrote:
>> Hi Eric,
>>
>> On 10/05/16 17:10, Eric Auger wrote:
>>> Hi Alex,
>>> On 05/10/2016 12:49 AM, Alex Williamson wrote:
>>>> On Wed, 4 May 2016 11:54:16 +0000
>>>> Eric Auger <eric.auger@linaro.org> wrote:
>>>>
>>>>> On x86 IRQ remapping is abstracted by the IOMMU. On ARM this is
>>>>> abstracted
>>>>> by the msi controller. vfio_safe_irq_domain allows to check whether
>>>>> interrupts are "safe" for a given device. They are if the device does
>>>>> not use MSI
>>>>
>>>> Are we sure we're not opening a security hole here? An MSI is simply a
>>>> DMA write, so really whether or not a device uses MSI is irrelevant.
>>>> If it can generate a DMA to the MSI doorbell then we need to be
>>>> protected and I think we pretty much need to assume that devices are
>>>> DMA capable. Do the MSI domain checks cover this?
>>> Let me try to rephrase: we check the device is not attached to an MSI
>>> controller (I think this is the semantic of dev_get_msi_domain(dev)).
>>>
>>> If it is not, we don't have to care about MSI isolation: there will be
>>> no IOMMU binding between the device and any MSI doorbell. If it is we
>>> check the msi domain is backed by an MSI controller able to perform MSI
>>> isolation.
>>>
>>> So effectively "usage of MSIs" is improper - since it is decided after
>>> the group attachment anyway - and the commit message should rather
>>> state "if the device is linked to an MSI controller" (dt msi-parent
>>> notion I think).
>>
>> Hmm, I think Alex has a point here - on a GICv2m I can happily fire
>> arbitrary MSIs from _a shell_ (using /dev/mem), and the CPUs definitely
>> aren't in an MSI domain, so I don't think it's valid to assume that a
>> device using only wired interrupts, therefore with no connection to any
>> MSI controller, isn't still capable of maliciously spewing DMA all over
>> any and every doorbell region in the system.
>
> Sorry but I still don't get the point. For the device to reach the
> doorbell there must be an IOMMU mapping.
Only when the doorbell is _downstream_ of IOMMU translation.
> - if the device is not attached to an MSI domain, there won't be any
> doorbell iommu mapping built by this series, so no risk, right?
>
> The device will be allowed to reach only memory iommu mapped by
> userspace with VFIO DMA MAP standard API. Of course if the userspace can
> mmap all the host PA that's a more general issue, right?
>
> - If the device is attached to an MSI domain (msi-parent link), 2 cases:
> 1) the MSI controller advertises MSI isolation (ITS cases), no risk
> 2) the MSI controller does not advertise MSI isolation (GICv2m), there
> is a security hole.
> a) by default we reject the device attachment
> b) if the userspace overrides the safe interrupt option he accepts
> the security hole
>
> What am I missing?
The x86-with-interrupt-remapping-disabled case. Currently, without
unsafe_interrupts, everything is rejected - with this patch, we'll still
reject anything with an MSI domain, but e.g. legacy PCI devices using
INTx would be free to scribble all over the un-translated interrupt
region willy-nilly. That's the subtle, but significant, change in behaviour.
Robin.
> Best Regards
>
> Eric
>>
>> Robin.
>>
>>> Does it sound better?
>>>
>>> Regards
>>>
>>> Eric
>>>>
>>>>> or if the device uses MSI and the msi-parent controller
>>>>> supports IRQ remapping.
>>>>>
>>>>> Then we check at group level if all devices have safe interrupts: if
>>>>> not,
>>>>> we only allow the group to be attached if allow_unsafe_interrupts is
>>>>> set.
>>>>>
>>>>> At this point ARM sMMU still advertises IOMMU_CAP_INTR_REMAP. This is
>>>>> changed in next patch.
>>>>>
>>>>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>>>>
>>>>> ---
>>>>> 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 | 44
>>>>> +++++++++++++++++++++++++++++++++++++++--
>>>>> 1 file changed, 42 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/vfio/vfio_iommu_type1.c
>>>>> b/drivers/vfio/vfio_iommu_type1.c
>>>>> index 4d3a6f1..2fc8197 100644
>>>>> --- a/drivers/vfio/vfio_iommu_type1.c
>>>>> +++ b/drivers/vfio/vfio_iommu_type1.c
>>>>> @@ -37,6 +37,8 @@
>>>>> #include <linux/vfio.h>
>>>>> #include <linux/workqueue.h>
>>>>> #include <linux/msi-iommu.h>
>>>>> +#include <linux/irqdomain.h>
>>>>> +#include <linux/msi.h>
>>>>>
>>>>> #define DRIVER_VERSION "0.2"
>>>>> #define DRIVER_AUTHOR "Alex Williamson
>>>>> <alex.williamson@redhat.com>"
>>>>> @@ -777,6 +779,33 @@ static int vfio_bus_type(struct device *dev,
>>>>> void *data)
>>>>> return 0;
>>>>> }
>>>>>
>>>>> +/**
>>>>> + * vfio_safe_irq_domain: returns whether the irq domain
>>>>> + * the device is attached to is safe with respect to MSI isolation.
>>>>> + * If the irq domain is not an MSI domain, we return it is safe.
>>>>> + *
>>>>> + * @dev: device handle
>>>>> + * @data: unused
>>>>> + * returns 0 if the irq domain is safe, -1 if not.
>>>>> + */
>>>>> +static int vfio_safe_irq_domain(struct device *dev, void *data)
>>>>> +{
>>>>> +#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
>>>>> + struct irq_domain *domain;
>>>>> + struct msi_domain_info *info;
>>>>> +
>>>>> + domain = dev_get_msi_domain(dev);
>>>>> + if (!domain)
>>>>> + return 0;
>>>>> +
>>>>> + info = msi_get_domain_info(domain);
>>>>> +
>>>>> + if (!(info->flags & MSI_FLAG_IRQ_REMAPPING))
>>>>> + return -1;
>>>>> +#endif
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> static int vfio_iommu_replay(struct vfio_iommu *iommu,
>>>>> struct vfio_domain *domain)
>>>>> {
>>>>> @@ -870,7 +899,7 @@ static int vfio_iommu_type1_attach_group(void
>>>>> *iommu_data,
>>>>> struct vfio_group *group, *g;
>>>>> struct vfio_domain *domain, *d;
>>>>> struct bus_type *bus = NULL;
>>>>> - int ret;
>>>>> + int ret, safe_irq_domains;
>>>>>
>>>>> mutex_lock(&iommu->lock);
>>>>>
>>>>> @@ -893,6 +922,13 @@ static int vfio_iommu_type1_attach_group(void
>>>>> *iommu_data,
>>>>>
>>>>> group->iommu_group = iommu_group;
>>>>>
>>>>> + /*
>>>>> + * Determine if all the devices of the group have a safe irq
>>>>> domain
>>>>> + * with respect to MSI isolation
>>>>> + */
>>>>> + safe_irq_domains = !iommu_group_for_each_dev(iommu_group, &bus,
>>>>> + vfio_safe_irq_domain);
>>>>> +
>>>>> /* Determine bus_type in order to allocate a domain */
>>>>> ret = iommu_group_for_each_dev(iommu_group, &bus, vfio_bus_type);
>>>>> if (ret)
>>>>> @@ -920,8 +956,12 @@ 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/interrupt translation
>>>>> + */
>>>>> if (!allow_unsafe_interrupts &&
>>>>> - !iommu_capable(bus, IOMMU_CAP_INTR_REMAP)) {
>>>>> + (!iommu_capable(bus, IOMMU_CAP_INTR_REMAP) &&
>>>>> !safe_irq_domains)) {
>>>>> 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;
>>>>
>>>
>>
>
[toc] | [prev] | [next] | [standalone]
| From | Eric Auger <eric.auger@linaro.org> |
|---|---|
| Date | 2016-05-11 11:50 +0200 |
| Subject | Re: [PATCH v9 5/7] vfio/type1: also check IRQ remapping capability at msi domain |
| Message-ID | <rxyWf-5Bh-39@gated-at.bofh.it> |
| In reply to | #1398862 |
Hi Robin,
On 05/11/2016 11:31 AM, Robin Murphy wrote:
> On 11/05/16 09:38, Eric Auger wrote:
>> Hi Robin, Alex,
>> On 05/10/2016 07:24 PM, Robin Murphy wrote:
>>> Hi Eric,
>>>
>>> On 10/05/16 17:10, Eric Auger wrote:
>>>> Hi Alex,
>>>> On 05/10/2016 12:49 AM, Alex Williamson wrote:
>>>>> On Wed, 4 May 2016 11:54:16 +0000
>>>>> Eric Auger <eric.auger@linaro.org> wrote:
>>>>>
>>>>>> On x86 IRQ remapping is abstracted by the IOMMU. On ARM this is
>>>>>> abstracted
>>>>>> by the msi controller. vfio_safe_irq_domain allows to check whether
>>>>>> interrupts are "safe" for a given device. They are if the device does
>>>>>> not use MSI
>>>>>
>>>>> Are we sure we're not opening a security hole here? An MSI is
>>>>> simply a
>>>>> DMA write, so really whether or not a device uses MSI is irrelevant.
>>>>> If it can generate a DMA to the MSI doorbell then we need to be
>>>>> protected and I think we pretty much need to assume that devices are
>>>>> DMA capable. Do the MSI domain checks cover this?
>>>> Let me try to rephrase: we check the device is not attached to an MSI
>>>> controller (I think this is the semantic of dev_get_msi_domain(dev)).
>>>>
>>>> If it is not, we don't have to care about MSI isolation: there will be
>>>> no IOMMU binding between the device and any MSI doorbell. If it is we
>>>> check the msi domain is backed by an MSI controller able to perform MSI
>>>> isolation.
>>>>
>>>> So effectively "usage of MSIs" is improper - since it is decided after
>>>> the group attachment anyway - and the commit message should rather
>>>> state "if the device is linked to an MSI controller" (dt msi-parent
>>>> notion I think).
>>>
>>> Hmm, I think Alex has a point here - on a GICv2m I can happily fire
>>> arbitrary MSIs from _a shell_ (using /dev/mem), and the CPUs definitely
>>> aren't in an MSI domain, so I don't think it's valid to assume that a
>>> device using only wired interrupts, therefore with no connection to any
>>> MSI controller, isn't still capable of maliciously spewing DMA all over
>>> any and every doorbell region in the system.
>>
>> Sorry but I still don't get the point. For the device to reach the
>> doorbell there must be an IOMMU mapping.
>
> Only when the doorbell is _downstream_ of IOMMU translation.
Yes but that's the root hypothesis with VFIO assignment, right?
Except if the no-iommu option is explicitly set by the userspace, there
must be an IOMMU downstream to the device that protects all the DMA
transactions.
>
>> - if the device is not attached to an MSI domain, there won't be any
>> doorbell iommu mapping built by this series, so no risk, right?
>>
>> The device will be allowed to reach only memory iommu mapped by
>> userspace with VFIO DMA MAP standard API. Of course if the userspace can
>> mmap all the host PA that's a more general issue, right?
>>
>> - If the device is attached to an MSI domain (msi-parent link), 2 cases:
>> 1) the MSI controller advertises MSI isolation (ITS cases), no risk
>> 2) the MSI controller does not advertise MSI isolation (GICv2m), there
>> is a security hole.
>> a) by default we reject the device attachment
>> b) if the userspace overrides the safe interrupt option he accepts
>> the security hole
>>
>> What am I missing?
>
> The x86-with-interrupt-remapping-disabled case. Currently, without
> unsafe_interrupts, everything is rejected - with this patch, we'll still
> reject anything with an MSI domain, but e.g. legacy PCI devices using
> INTx would be free to scribble all over the un-translated interrupt
> region willy-nilly.
Assuming we have an IOMMU, either we have translated transactions or
faults? So if legacy PCI devices try to write into a doorbell, there
must be faults because there is no IOMMU mapping for doorbells?
Best Regards
Eric
That's the subtle, but significant, change in
> behaviour.
>
> Robin.
>
>> Best Regards
>>
>> Eric
>>>
>>> Robin.
>>>
>>>> Does it sound better?
>>>>
>>>> Regards
>>>>
>>>> Eric
>>>>>
>>>>>> or if the device uses MSI and the msi-parent controller
>>>>>> supports IRQ remapping.
>>>>>>
>>>>>> Then we check at group level if all devices have safe interrupts: if
>>>>>> not,
>>>>>> we only allow the group to be attached if allow_unsafe_interrupts is
>>>>>> set.
>>>>>>
>>>>>> At this point ARM sMMU still advertises IOMMU_CAP_INTR_REMAP. This is
>>>>>> changed in next patch.
>>>>>>
>>>>>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>>>>>
>>>>>> ---
>>>>>> 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 | 44
>>>>>> +++++++++++++++++++++++++++++++++++++++--
>>>>>> 1 file changed, 42 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/vfio/vfio_iommu_type1.c
>>>>>> b/drivers/vfio/vfio_iommu_type1.c
>>>>>> index 4d3a6f1..2fc8197 100644
>>>>>> --- a/drivers/vfio/vfio_iommu_type1.c
>>>>>> +++ b/drivers/vfio/vfio_iommu_type1.c
>>>>>> @@ -37,6 +37,8 @@
>>>>>> #include <linux/vfio.h>
>>>>>> #include <linux/workqueue.h>
>>>>>> #include <linux/msi-iommu.h>
>>>>>> +#include <linux/irqdomain.h>
>>>>>> +#include <linux/msi.h>
>>>>>>
>>>>>> #define DRIVER_VERSION "0.2"
>>>>>> #define DRIVER_AUTHOR "Alex Williamson
>>>>>> <alex.williamson@redhat.com>"
>>>>>> @@ -777,6 +779,33 @@ static int vfio_bus_type(struct device *dev,
>>>>>> void *data)
>>>>>> return 0;
>>>>>> }
>>>>>>
>>>>>> +/**
>>>>>> + * vfio_safe_irq_domain: returns whether the irq domain
>>>>>> + * the device is attached to is safe with respect to MSI isolation.
>>>>>> + * If the irq domain is not an MSI domain, we return it is safe.
>>>>>> + *
>>>>>> + * @dev: device handle
>>>>>> + * @data: unused
>>>>>> + * returns 0 if the irq domain is safe, -1 if not.
>>>>>> + */
>>>>>> +static int vfio_safe_irq_domain(struct device *dev, void *data)
>>>>>> +{
>>>>>> +#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
>>>>>> + struct irq_domain *domain;
>>>>>> + struct msi_domain_info *info;
>>>>>> +
>>>>>> + domain = dev_get_msi_domain(dev);
>>>>>> + if (!domain)
>>>>>> + return 0;
>>>>>> +
>>>>>> + info = msi_get_domain_info(domain);
>>>>>> +
>>>>>> + if (!(info->flags & MSI_FLAG_IRQ_REMAPPING))
>>>>>> + return -1;
>>>>>> +#endif
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> static int vfio_iommu_replay(struct vfio_iommu *iommu,
>>>>>> struct vfio_domain *domain)
>>>>>> {
>>>>>> @@ -870,7 +899,7 @@ static int vfio_iommu_type1_attach_group(void
>>>>>> *iommu_data,
>>>>>> struct vfio_group *group, *g;
>>>>>> struct vfio_domain *domain, *d;
>>>>>> struct bus_type *bus = NULL;
>>>>>> - int ret;
>>>>>> + int ret, safe_irq_domains;
>>>>>>
>>>>>> mutex_lock(&iommu->lock);
>>>>>>
>>>>>> @@ -893,6 +922,13 @@ static int vfio_iommu_type1_attach_group(void
>>>>>> *iommu_data,
>>>>>>
>>>>>> group->iommu_group = iommu_group;
>>>>>>
>>>>>> + /*
>>>>>> + * Determine if all the devices of the group have a safe irq
>>>>>> domain
>>>>>> + * with respect to MSI isolation
>>>>>> + */
>>>>>> + safe_irq_domains = !iommu_group_for_each_dev(iommu_group, &bus,
>>>>>> + vfio_safe_irq_domain);
>>>>>> +
>>>>>> /* Determine bus_type in order to allocate a domain */
>>>>>> ret = iommu_group_for_each_dev(iommu_group, &bus,
>>>>>> vfio_bus_type);
>>>>>> if (ret)
>>>>>> @@ -920,8 +956,12 @@ 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/interrupt translation
>>>>>> + */
>>>>>> if (!allow_unsafe_interrupts &&
>>>>>> - !iommu_capable(bus, IOMMU_CAP_INTR_REMAP)) {
>>>>>> + (!iommu_capable(bus, IOMMU_CAP_INTR_REMAP) &&
>>>>>> !safe_irq_domains)) {
>>>>>> 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;
>>>>>
>>>>
>>>
>>
>
[toc] | [prev] | [next] | [standalone]
| From | Robin Murphy <robin.murphy@arm.com> |
|---|---|
| Date | 2016-05-11 15:50 +0200 |
| Subject | Re: [PATCH v9 5/7] vfio/type1: also check IRQ remapping capability at msi domain |
| Message-ID | <rxCGv-Pf-19@gated-at.bofh.it> |
| In reply to | #1398878 |
On 11/05/16 10:44, Eric Auger wrote:
> Hi Robin,
> On 05/11/2016 11:31 AM, Robin Murphy wrote:
>> On 11/05/16 09:38, Eric Auger wrote:
>>> Hi Robin, Alex,
>>> On 05/10/2016 07:24 PM, Robin Murphy wrote:
>>>> Hi Eric,
>>>>
>>>> On 10/05/16 17:10, Eric Auger wrote:
>>>>> Hi Alex,
>>>>> On 05/10/2016 12:49 AM, Alex Williamson wrote:
>>>>>> On Wed, 4 May 2016 11:54:16 +0000
>>>>>> Eric Auger <eric.auger@linaro.org> wrote:
>>>>>>
>>>>>>> On x86 IRQ remapping is abstracted by the IOMMU. On ARM this is
>>>>>>> abstracted
>>>>>>> by the msi controller. vfio_safe_irq_domain allows to check whether
>>>>>>> interrupts are "safe" for a given device. They are if the device does
>>>>>>> not use MSI
>>>>>>
>>>>>> Are we sure we're not opening a security hole here? An MSI is
>>>>>> simply a
>>>>>> DMA write, so really whether or not a device uses MSI is irrelevant.
>>>>>> If it can generate a DMA to the MSI doorbell then we need to be
>>>>>> protected and I think we pretty much need to assume that devices are
>>>>>> DMA capable. Do the MSI domain checks cover this?
>>>>> Let me try to rephrase: we check the device is not attached to an MSI
>>>>> controller (I think this is the semantic of dev_get_msi_domain(dev)).
>>>>>
>>>>> If it is not, we don't have to care about MSI isolation: there will be
>>>>> no IOMMU binding between the device and any MSI doorbell. If it is we
>>>>> check the msi domain is backed by an MSI controller able to perform MSI
>>>>> isolation.
>>>>>
>>>>> So effectively "usage of MSIs" is improper - since it is decided after
>>>>> the group attachment anyway - and the commit message should rather
>>>>> state "if the device is linked to an MSI controller" (dt msi-parent
>>>>> notion I think).
>>>>
>>>> Hmm, I think Alex has a point here - on a GICv2m I can happily fire
>>>> arbitrary MSIs from _a shell_ (using /dev/mem), and the CPUs definitely
>>>> aren't in an MSI domain, so I don't think it's valid to assume that a
>>>> device using only wired interrupts, therefore with no connection to any
>>>> MSI controller, isn't still capable of maliciously spewing DMA all over
>>>> any and every doorbell region in the system.
>>>
>>> Sorry but I still don't get the point. For the device to reach the
>>> doorbell there must be an IOMMU mapping.
>>
>> Only when the doorbell is _downstream_ of IOMMU translation.
> Yes but that's the root hypothesis with VFIO assignment, right?
>
> Except if the no-iommu option is explicitly set by the userspace, there
> must be an IOMMU downstream to the device that protects all the DMA
> transactions.
The IOMMU is downstream of the device, clearly, but the MSI machinery
itself could be _in between_ the two - say, integrated into a PCIe root
complex served by an external SMMU. The doorbell is carved out of the
upstream address space so that a write targeting the appropriate address
hits it directly and never even goes out to the SMMU - e.g.
http://thread.gmane.org/gmane.linux.kernel.pci/47174/focus=47268
>>> - if the device is not attached to an MSI domain, there won't be any
>>> doorbell iommu mapping built by this series, so no risk, right?
>>>
>>> The device will be allowed to reach only memory iommu mapped by
>>> userspace with VFIO DMA MAP standard API. Of course if the userspace can
>>> mmap all the host PA that's a more general issue, right?
>>>
>>> - If the device is attached to an MSI domain (msi-parent link), 2 cases:
>>> 1) the MSI controller advertises MSI isolation (ITS cases), no risk
>>> 2) the MSI controller does not advertise MSI isolation (GICv2m), there
>>> is a security hole.
>>> a) by default we reject the device attachment
>>> b) if the userspace overrides the safe interrupt option he accepts
>>> the security hole
>>>
>>> What am I missing?
>>
>> The x86-with-interrupt-remapping-disabled case. Currently, without
>> unsafe_interrupts, everything is rejected - with this patch, we'll still
>> reject anything with an MSI domain, but e.g. legacy PCI devices using
>> INTx would be free to scribble all over the un-translated interrupt
>> region willy-nilly.
> Assuming we have an IOMMU, either we have translated transactions or
> faults? So if legacy PCI devices try to write into a doorbell, there
> must be faults because there is no IOMMU mapping for doorbells?
To the best of my understanding, having been wading through the VT-d
spec, the case on x86 is as above (OK, so strictly it's more "in
parallel with" than "in front of") - writes outside the 0xFEExxxxx
interrupt region go to the DMA remapping units, whereas writes within
that region go to the interrupt remapping unit, and if that's disabled
or not implemented, simply sail straight through to the APIC beyond.
Robin.
> Best Regards
>
> Eric
> That's the subtle, but significant, change in
>> behaviour.
>>
>> Robin.
>>
>>> Best Regards
>>>
>>> Eric
>>>>
>>>> Robin.
>>>>
>>>>> Does it sound better?
>>>>>
>>>>> Regards
>>>>>
>>>>> Eric
>>>>>>
>>>>>>> or if the device uses MSI and the msi-parent controller
>>>>>>> supports IRQ remapping.
>>>>>>>
>>>>>>> Then we check at group level if all devices have safe interrupts: if
>>>>>>> not,
>>>>>>> we only allow the group to be attached if allow_unsafe_interrupts is
>>>>>>> set.
>>>>>>>
>>>>>>> At this point ARM sMMU still advertises IOMMU_CAP_INTR_REMAP. This is
>>>>>>> changed in next patch.
>>>>>>>
>>>>>>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>>>>>>
>>>>>>> ---
>>>>>>> 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 | 44
>>>>>>> +++++++++++++++++++++++++++++++++++++++--
>>>>>>> 1 file changed, 42 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/vfio/vfio_iommu_type1.c
>>>>>>> b/drivers/vfio/vfio_iommu_type1.c
>>>>>>> index 4d3a6f1..2fc8197 100644
>>>>>>> --- a/drivers/vfio/vfio_iommu_type1.c
>>>>>>> +++ b/drivers/vfio/vfio_iommu_type1.c
>>>>>>> @@ -37,6 +37,8 @@
>>>>>>> #include <linux/vfio.h>
>>>>>>> #include <linux/workqueue.h>
>>>>>>> #include <linux/msi-iommu.h>
>>>>>>> +#include <linux/irqdomain.h>
>>>>>>> +#include <linux/msi.h>
>>>>>>>
>>>>>>> #define DRIVER_VERSION "0.2"
>>>>>>> #define DRIVER_AUTHOR "Alex Williamson
>>>>>>> <alex.williamson@redhat.com>"
>>>>>>> @@ -777,6 +779,33 @@ static int vfio_bus_type(struct device *dev,
>>>>>>> void *data)
>>>>>>> return 0;
>>>>>>> }
>>>>>>>
>>>>>>> +/**
>>>>>>> + * vfio_safe_irq_domain: returns whether the irq domain
>>>>>>> + * the device is attached to is safe with respect to MSI isolation.
>>>>>>> + * If the irq domain is not an MSI domain, we return it is safe.
>>>>>>> + *
>>>>>>> + * @dev: device handle
>>>>>>> + * @data: unused
>>>>>>> + * returns 0 if the irq domain is safe, -1 if not.
>>>>>>> + */
>>>>>>> +static int vfio_safe_irq_domain(struct device *dev, void *data)
>>>>>>> +{
>>>>>>> +#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
>>>>>>> + struct irq_domain *domain;
>>>>>>> + struct msi_domain_info *info;
>>>>>>> +
>>>>>>> + domain = dev_get_msi_domain(dev);
>>>>>>> + if (!domain)
>>>>>>> + return 0;
>>>>>>> +
>>>>>>> + info = msi_get_domain_info(domain);
>>>>>>> +
>>>>>>> + if (!(info->flags & MSI_FLAG_IRQ_REMAPPING))
>>>>>>> + return -1;
>>>>>>> +#endif
>>>>>>> + return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> static int vfio_iommu_replay(struct vfio_iommu *iommu,
>>>>>>> struct vfio_domain *domain)
>>>>>>> {
>>>>>>> @@ -870,7 +899,7 @@ static int vfio_iommu_type1_attach_group(void
>>>>>>> *iommu_data,
>>>>>>> struct vfio_group *group, *g;
>>>>>>> struct vfio_domain *domain, *d;
>>>>>>> struct bus_type *bus = NULL;
>>>>>>> - int ret;
>>>>>>> + int ret, safe_irq_domains;
>>>>>>>
>>>>>>> mutex_lock(&iommu->lock);
>>>>>>>
>>>>>>> @@ -893,6 +922,13 @@ static int vfio_iommu_type1_attach_group(void
>>>>>>> *iommu_data,
>>>>>>>
>>>>>>> group->iommu_group = iommu_group;
>>>>>>>
>>>>>>> + /*
>>>>>>> + * Determine if all the devices of the group have a safe irq
>>>>>>> domain
>>>>>>> + * with respect to MSI isolation
>>>>>>> + */
>>>>>>> + safe_irq_domains = !iommu_group_for_each_dev(iommu_group, &bus,
>>>>>>> + vfio_safe_irq_domain);
>>>>>>> +
>>>>>>> /* Determine bus_type in order to allocate a domain */
>>>>>>> ret = iommu_group_for_each_dev(iommu_group, &bus,
>>>>>>> vfio_bus_type);
>>>>>>> if (ret)
>>>>>>> @@ -920,8 +956,12 @@ 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/interrupt translation
>>>>>>> + */
>>>>>>> if (!allow_unsafe_interrupts &&
>>>>>>> - !iommu_capable(bus, IOMMU_CAP_INTR_REMAP)) {
>>>>>>> + (!iommu_capable(bus, IOMMU_CAP_INTR_REMAP) &&
>>>>>>> !safe_irq_domains)) {
>>>>>>> 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;
>>>>>>
>>>>>
>>>>
>>>
>>
>
[toc] | [prev] | [next] | [standalone]
| From | Eric Auger <eric.auger@linaro.org> |
|---|---|
| Date | 2016-05-11 16:40 +0200 |
| Subject | Re: [PATCH v9 5/7] vfio/type1: also check IRQ remapping capability at msi domain |
| Message-ID | <rxDsS-1yk-15@gated-at.bofh.it> |
| In reply to | #1399095 |
Hi Robin,
On 05/11/2016 03:48 PM, Robin Murphy wrote:
> On 11/05/16 10:44, Eric Auger wrote:
>> Hi Robin,
>> On 05/11/2016 11:31 AM, Robin Murphy wrote:
>>> On 11/05/16 09:38, Eric Auger wrote:
>>>> Hi Robin, Alex,
>>>> On 05/10/2016 07:24 PM, Robin Murphy wrote:
>>>>> Hi Eric,
>>>>>
>>>>> On 10/05/16 17:10, Eric Auger wrote:
>>>>>> Hi Alex,
>>>>>> On 05/10/2016 12:49 AM, Alex Williamson wrote:
>>>>>>> On Wed, 4 May 2016 11:54:16 +0000
>>>>>>> Eric Auger <eric.auger@linaro.org> wrote:
>>>>>>>
>>>>>>>> On x86 IRQ remapping is abstracted by the IOMMU. On ARM this is
>>>>>>>> abstracted
>>>>>>>> by the msi controller. vfio_safe_irq_domain allows to check whether
>>>>>>>> interrupts are "safe" for a given device. They are if the device
>>>>>>>> does
>>>>>>>> not use MSI
>>>>>>>
>>>>>>> Are we sure we're not opening a security hole here? An MSI is
>>>>>>> simply a
>>>>>>> DMA write, so really whether or not a device uses MSI is irrelevant.
>>>>>>> If it can generate a DMA to the MSI doorbell then we need to be
>>>>>>> protected and I think we pretty much need to assume that devices are
>>>>>>> DMA capable. Do the MSI domain checks cover this?
>>>>>> Let me try to rephrase: we check the device is not attached to an MSI
>>>>>> controller (I think this is the semantic of dev_get_msi_domain(dev)).
>>>>>>
>>>>>> If it is not, we don't have to care about MSI isolation: there
>>>>>> will be
>>>>>> no IOMMU binding between the device and any MSI doorbell. If it is we
>>>>>> check the msi domain is backed by an MSI controller able to
>>>>>> perform MSI
>>>>>> isolation.
>>>>>>
>>>>>> So effectively "usage of MSIs" is improper - since it is decided
>>>>>> after
>>>>>> the group attachment anyway - and the commit message should rather
>>>>>> state "if the device is linked to an MSI controller" (dt msi-parent
>>>>>> notion I think).
>>>>>
>>>>> Hmm, I think Alex has a point here - on a GICv2m I can happily fire
>>>>> arbitrary MSIs from _a shell_ (using /dev/mem), and the CPUs
>>>>> definitely
>>>>> aren't in an MSI domain, so I don't think it's valid to assume that a
>>>>> device using only wired interrupts, therefore with no connection to
>>>>> any
>>>>> MSI controller, isn't still capable of maliciously spewing DMA all
>>>>> over
>>>>> any and every doorbell region in the system.
>>>>
>>>> Sorry but I still don't get the point. For the device to reach the
>>>> doorbell there must be an IOMMU mapping.
>>>
>>> Only when the doorbell is _downstream_ of IOMMU translation.
>> Yes but that's the root hypothesis with VFIO assignment, right?
>>
>> Except if the no-iommu option is explicitly set by the userspace, there
>> must be an IOMMU downstream to the device that protects all the DMA
>> transactions.
>
> The IOMMU is downstream of the device, clearly, but the MSI machinery
> itself could be _in between_ the two - say, integrated into a PCIe root
> complex served by an external SMMU. The doorbell is carved out of the
> upstream address space so that a write targeting the appropriate address
> hits it directly and never even goes out to the SMMU - e.g.
> http://thread.gmane.org/gmane.linux.kernel.pci/47174/focus=47268
Ah OK, I now better understand the point. So in that case not all the
DMA accesses of the devices are properly protected by the sMMU. Doesn't
it look as a kind of PCIe Access Control Service issue here where
transactions are able to bypass the translation service (downstream to
the root complex)?
I clearly did not address that kind of topology in this series. In such
a case the MSIs may even not be mapped at all which was the primary
purpose of the series.
Do you have any clue about how to detect that situation? If we cannot
rely on any iommu service I don't find any other solution but to
recognize this situation and reject the assignment.
>
>>>> - if the device is not attached to an MSI domain, there won't be any
>>>> doorbell iommu mapping built by this series, so no risk, right?
>>>>
>>>> The device will be allowed to reach only memory iommu mapped by
>>>> userspace with VFIO DMA MAP standard API. Of course if the userspace
>>>> can
>>>> mmap all the host PA that's a more general issue, right?
>>>>
>>>> - If the device is attached to an MSI domain (msi-parent link), 2
>>>> cases:
>>>> 1) the MSI controller advertises MSI isolation (ITS cases), no risk
>>>> 2) the MSI controller does not advertise MSI isolation (GICv2m), there
>>>> is a security hole.
>>>> a) by default we reject the device attachment
>>>> b) if the userspace overrides the safe interrupt option he accepts
>>>> the security hole
>>>>
>>>> What am I missing?
>>>
>>> The x86-with-interrupt-remapping-disabled case. Currently, without
>>> unsafe_interrupts, everything is rejected - with this patch, we'll still
>>> reject anything with an MSI domain, but e.g. legacy PCI devices using
>>> INTx would be free to scribble all over the un-translated interrupt
>>> region willy-nilly.
>> Assuming we have an IOMMU, either we have translated transactions or
>> faults? So if legacy PCI devices try to write into a doorbell, there
>> must be faults because there is no IOMMU mapping for doorbells?
>
> To the best of my understanding, having been wading through the VT-d
> spec, the case on x86 is as above (OK, so strictly it's more "in
> parallel with" than "in front of") - writes outside the 0xFEExxxxx
> interrupt region go to the DMA remapping units, whereas writes within
> that region go to the interrupt remapping unit, and if that's disabled
> or not implemented, simply sail straight through to the APIC beyond.
that's my understanding too
Best Regards
Eric
>
> Robin.
>
>> Best Regards
>>
>> Eric
>> That's the subtle, but significant, change in
>>> behaviour.
>>>
>>> Robin.
>>>
>>>> Best Regards
>>>>
>>>> Eric
>>>>>
>>>>> Robin.
>>>>>
>>>>>> Does it sound better?
>>>>>>
>>>>>> Regards
>>>>>>
>>>>>> Eric
>>>>>>>
>>>>>>>> or if the device uses MSI and the msi-parent controller
>>>>>>>> supports IRQ remapping.
>>>>>>>>
>>>>>>>> Then we check at group level if all devices have safe
>>>>>>>> interrupts: if
>>>>>>>> not,
>>>>>>>> we only allow the group to be attached if
>>>>>>>> allow_unsafe_interrupts is
>>>>>>>> set.
>>>>>>>>
>>>>>>>> At this point ARM sMMU still advertises IOMMU_CAP_INTR_REMAP.
>>>>>>>> This is
>>>>>>>> changed in next patch.
>>>>>>>>
>>>>>>>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>>>>>>>
>>>>>>>> ---
>>>>>>>> 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 | 44
>>>>>>>> +++++++++++++++++++++++++++++++++++++++--
>>>>>>>> 1 file changed, 42 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/vfio/vfio_iommu_type1.c
>>>>>>>> b/drivers/vfio/vfio_iommu_type1.c
>>>>>>>> index 4d3a6f1..2fc8197 100644
>>>>>>>> --- a/drivers/vfio/vfio_iommu_type1.c
>>>>>>>> +++ b/drivers/vfio/vfio_iommu_type1.c
>>>>>>>> @@ -37,6 +37,8 @@
>>>>>>>> #include <linux/vfio.h>
>>>>>>>> #include <linux/workqueue.h>
>>>>>>>> #include <linux/msi-iommu.h>
>>>>>>>> +#include <linux/irqdomain.h>
>>>>>>>> +#include <linux/msi.h>
>>>>>>>>
>>>>>>>> #define DRIVER_VERSION "0.2"
>>>>>>>> #define DRIVER_AUTHOR "Alex Williamson
>>>>>>>> <alex.williamson@redhat.com>"
>>>>>>>> @@ -777,6 +779,33 @@ static int vfio_bus_type(struct device *dev,
>>>>>>>> void *data)
>>>>>>>> return 0;
>>>>>>>> }
>>>>>>>>
>>>>>>>> +/**
>>>>>>>> + * vfio_safe_irq_domain: returns whether the irq domain
>>>>>>>> + * the device is attached to is safe with respect to MSI
>>>>>>>> isolation.
>>>>>>>> + * If the irq domain is not an MSI domain, we return it is safe.
>>>>>>>> + *
>>>>>>>> + * @dev: device handle
>>>>>>>> + * @data: unused
>>>>>>>> + * returns 0 if the irq domain is safe, -1 if not.
>>>>>>>> + */
>>>>>>>> +static int vfio_safe_irq_domain(struct device *dev, void *data)
>>>>>>>> +{
>>>>>>>> +#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
>>>>>>>> + struct irq_domain *domain;
>>>>>>>> + struct msi_domain_info *info;
>>>>>>>> +
>>>>>>>> + domain = dev_get_msi_domain(dev);
>>>>>>>> + if (!domain)
>>>>>>>> + return 0;
>>>>>>>> +
>>>>>>>> + info = msi_get_domain_info(domain);
>>>>>>>> +
>>>>>>>> + if (!(info->flags & MSI_FLAG_IRQ_REMAPPING))
>>>>>>>> + return -1;
>>>>>>>> +#endif
>>>>>>>> + return 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> static int vfio_iommu_replay(struct vfio_iommu *iommu,
>>>>>>>> struct vfio_domain *domain)
>>>>>>>> {
>>>>>>>> @@ -870,7 +899,7 @@ static int vfio_iommu_type1_attach_group(void
>>>>>>>> *iommu_data,
>>>>>>>> struct vfio_group *group, *g;
>>>>>>>> struct vfio_domain *domain, *d;
>>>>>>>> struct bus_type *bus = NULL;
>>>>>>>> - int ret;
>>>>>>>> + int ret, safe_irq_domains;
>>>>>>>>
>>>>>>>> mutex_lock(&iommu->lock);
>>>>>>>>
>>>>>>>> @@ -893,6 +922,13 @@ static int vfio_iommu_type1_attach_group(void
>>>>>>>> *iommu_data,
>>>>>>>>
>>>>>>>> group->iommu_group = iommu_group;
>>>>>>>>
>>>>>>>> + /*
>>>>>>>> + * Determine if all the devices of the group have a safe irq
>>>>>>>> domain
>>>>>>>> + * with respect to MSI isolation
>>>>>>>> + */
>>>>>>>> + safe_irq_domains = !iommu_group_for_each_dev(iommu_group,
>>>>>>>> &bus,
>>>>>>>> + vfio_safe_irq_domain);
>>>>>>>> +
>>>>>>>> /* Determine bus_type in order to allocate a domain */
>>>>>>>> ret = iommu_group_for_each_dev(iommu_group, &bus,
>>>>>>>> vfio_bus_type);
>>>>>>>> if (ret)
>>>>>>>> @@ -920,8 +956,12 @@ 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/interrupt translation
>>>>>>>> + */
>>>>>>>> if (!allow_unsafe_interrupts &&
>>>>>>>> - !iommu_capable(bus, IOMMU_CAP_INTR_REMAP)) {
>>>>>>>> + (!iommu_capable(bus, IOMMU_CAP_INTR_REMAP) &&
>>>>>>>> !safe_irq_domains)) {
>>>>>>>> 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;
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
[toc] | [prev] | [next] | [standalone]
| From | Eric Auger <eric.auger@linaro.org> |
|---|---|
| Date | 2016-05-04 14:00 +0200 |
| Subject | [PATCH v9 6/7] iommu/arm-smmu: do not advertise IOMMU_CAP_INTR_REMAP |
| Message-ID | <rv3Dc-38F-13@gated-at.bofh.it> |
| In reply to | #1394182 |
Do not advertise IOMMU_CAP_INTR_REMAP for arm-smmu(-v3). Indeed the irq_remapping capability is abstracted on irqchip side for ARM as opposed to Intel IOMMU featuring IRQ remapping HW. So to check IRQ remapping capability, the msi domain needs to be checked instead. This commit needs to be applied after "vfio/type1: also check IRQ remapping capability at msi domain" else the legacy interrupt assignment gets broken with arm-smmu. Signed-off-by: Eric Auger <eric.auger@linaro.org> --- drivers/iommu/arm-smmu-v3.c | 3 ++- drivers/iommu/arm-smmu.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c index b5d9826..778212c 100644 --- a/drivers/iommu/arm-smmu-v3.c +++ b/drivers/iommu/arm-smmu-v3.c @@ -1386,7 +1386,8 @@ static bool arm_smmu_capable(enum iommu_cap cap) case IOMMU_CAP_CACHE_COHERENCY: return true; case IOMMU_CAP_INTR_REMAP: - return true; /* MSIs are just memory writes */ + /* interrupt translation handled at MSI controller level */ + return false; case IOMMU_CAP_NOEXEC: return true; default: diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index 0908985..e7c9e89 100644 --- a/drivers/iommu/arm-smmu.c +++ b/drivers/iommu/arm-smmu.c @@ -1320,7 +1320,8 @@ static bool arm_smmu_capable(enum iommu_cap cap) */ return true; case IOMMU_CAP_INTR_REMAP: - return true; /* MSIs are just memory writes */ + /* interrupt translation handled at MSI controller level */ + return false; case IOMMU_CAP_NOEXEC: return true; default: -- 1.9.1
[toc] | [prev] | [next] | [standalone]
| From | Eric Auger <eric.auger@linaro.org> |
|---|---|
| Date | 2016-05-04 14:00 +0200 |
| Subject | [PATCH v9 1/7] vfio: introduce a vfio_dma type field |
| Message-ID | <rv3Dc-38F-17@gated-at.bofh.it> |
| In reply to | #1394182 |
We introduce a vfio_dma type since we will need to discriminate
different types of dma slots:
- VFIO_IOVA_USER: IOVA region used to map user vaddr
- VFIO_IOVA_RESERVED: IOVA region reserved to map host device PA such
as MSI doorbells
Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
v6 -> v7:
- add VFIO_IOVA_ANY
- do not introduce yet any VFIO_IOVA_RESERVED handling
---
drivers/vfio/vfio_iommu_type1.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 75b24e9..aaf5a6c 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -53,6 +53,16 @@ module_param_named(disable_hugepages,
MODULE_PARM_DESC(disable_hugepages,
"Disable VFIO IOMMU support for IOMMU hugepages.");
+enum vfio_iova_type {
+ VFIO_IOVA_USER = 0, /* standard IOVA used to map user vaddr */
+ /*
+ * IOVA reserved to map special host physical addresses,
+ * MSI frames for instance
+ */
+ VFIO_IOVA_RESERVED,
+ VFIO_IOVA_ANY, /* matches any IOVA type */
+};
+
struct vfio_iommu {
struct list_head domain_list;
struct mutex lock;
@@ -75,6 +85,7 @@ struct vfio_dma {
unsigned long vaddr; /* Process virtual addr */
size_t size; /* Map size (bytes) */
int prot; /* IOMMU_READ/WRITE */
+ enum vfio_iova_type type; /* type of IOVA */
};
struct vfio_group {
--
1.9.1
[toc] | [prev] | [next] | [standalone]
| From | Eric Auger <eric.auger@linaro.org> |
|---|---|
| Date | 2016-05-04 14:00 +0200 |
| Subject | [PATCH v9 7/7] vfio/type1: return MSI geometry through VFIO_IOMMU_GET_INFO capability chains |
| Message-ID | <rv3Dd-38F-29@gated-at.bofh.it> |
| In reply to | #1394182 |
This patch allows the user-space to retrieve the MSI geometry. The
implementation is based on capability chains, now also added to
VFIO_IOMMU_GET_INFO.
The returned info comprise:
- whether the MSI IOVA are constrained to a reserved range (x86 case) and
in the positive, the start/end of the aperture,
- or whether the IOVA aperture need to be set by the userspace. In that
case, the size and alignment of the IOVA region to be provided are
returned.
In case the userspace must provide the IOVA range, we currently return
an arbitrary number of IOVA pages (16), supposed to fulfill the needs of
current ARM platforms. This may be deprecated by a more sophisticated
computation later on.
Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
v8 -> v9:
- use iommu_msi_supported flag instead of programmable
- replace IOMMU_INFO_REQUIRE_MSI_MAP flag by a more sophisticated
capability chain, reporting the MSI geometry
v7 -> v8:
- use iommu_domain_msi_geometry
v6 -> v7:
- remove the computation of the number of IOVA pages to be provisionned.
This number depends on the domain/group/device topology which can
dynamically change. Let's rely instead rely on an arbitrary max depending
on the system
v4 -> v5:
- move msi_info and ret declaration within the conditional code
v3 -> v4:
- replace former vfio_domains_require_msi_mapping by
more complex computation of MSI mapping requirements, especially the
number of pages to be provided by the user-space.
- reword patch title
RFC v1 -> v1:
- derived from
[RFC PATCH 3/6] vfio: Extend iommu-info to return MSIs automap state
- renamed allow_msi_reconfig into require_msi_mapping
- fixed VFIO_IOMMU_GET_INFO
---
drivers/vfio/vfio_iommu_type1.c | 69 +++++++++++++++++++++++++++++++++++++++++
include/uapi/linux/vfio.h | 30 +++++++++++++++++-
2 files changed, 98 insertions(+), 1 deletion(-)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 2fc8197..841360b 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -1134,6 +1134,50 @@ static int vfio_domains_have_iommu_cache(struct vfio_iommu *iommu)
return ret;
}
+static int compute_msi_geometry_caps(struct vfio_iommu *iommu,
+ struct vfio_info_cap *caps)
+{
+ struct vfio_iommu_type1_info_cap_msi_geometry *vfio_msi_geometry;
+ struct iommu_domain_msi_geometry msi_geometry;
+ struct vfio_info_cap_header *header;
+ struct vfio_domain *d;
+ bool mapping_required;
+ size_t size;
+
+ mutex_lock(&iommu->lock);
+ /* All domains have same require_msi_map property, pick first */
+ d = list_first_entry(&iommu->domain_list, struct vfio_domain, next);
+ iommu_domain_get_attr(d->domain, DOMAIN_ATTR_MSI_GEOMETRY,
+ &msi_geometry);
+ mapping_required = msi_geometry.iommu_msi_supported;
+
+ mutex_unlock(&iommu->lock);
+
+ size = sizeof(*vfio_msi_geometry);
+ header = vfio_info_cap_add(caps, size,
+ VFIO_IOMMU_TYPE1_INFO_CAP_MSI_GEOMETRY, 1);
+
+ if (IS_ERR(header))
+ return PTR_ERR(header);
+
+ vfio_msi_geometry = container_of(header,
+ struct vfio_iommu_type1_info_cap_msi_geometry,
+ header);
+
+ vfio_msi_geometry->reserved = !mapping_required;
+ if (vfio_msi_geometry->reserved) {
+ vfio_msi_geometry->aperture_start = msi_geometry.aperture_start;
+ vfio_msi_geometry->aperture_end = msi_geometry.aperture_end;
+ return 0;
+ }
+
+ vfio_msi_geometry->alignment = 1 << __ffs(vfio_pgsize_bitmap(iommu));
+ /* we currently report the need for an arbitray number of 16 pages */
+ vfio_msi_geometry->size = 16 * vfio_msi_geometry->alignment;
+
+ return 0;
+}
+
static long vfio_iommu_type1_ioctl(void *iommu_data,
unsigned int cmd, unsigned long arg)
{
@@ -1155,6 +1199,8 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
}
} else if (cmd == VFIO_IOMMU_GET_INFO) {
struct vfio_iommu_type1_info info;
+ struct vfio_info_cap caps = { .buf = NULL, .size = 0 };
+ int ret;
minsz = offsetofend(struct vfio_iommu_type1_info, iova_pgsizes);
@@ -1168,6 +1214,29 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
info.iova_pgsizes = vfio_pgsize_bitmap(iommu);
+ ret = compute_msi_geometry_caps(iommu, &caps);
+ if (ret)
+ return ret;
+
+ if (caps.size) {
+ info.flags |= VFIO_IOMMU_INFO_CAPS;
+ if (info.argsz < sizeof(info) + caps.size) {
+ info.argsz = sizeof(info) + caps.size;
+ info.cap_offset = 0;
+ } else {
+ vfio_info_cap_shift(&caps, sizeof(info));
+ if (copy_to_user((void __user *)arg +
+ sizeof(info), caps.buf,
+ caps.size)) {
+ kfree(caps.buf);
+ return -EFAULT;
+ }
+ info.cap_offset = sizeof(info);
+ }
+
+ kfree(caps.buf);
+ }
+
return copy_to_user((void __user *)arg, &info, minsz) ?
-EFAULT : 0;
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 4a9dbc2..0ff6a8d 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -488,7 +488,33 @@ struct vfio_iommu_type1_info {
__u32 argsz;
__u32 flags;
#define VFIO_IOMMU_INFO_PGSIZES (1 << 0) /* supported page sizes info */
- __u64 iova_pgsizes; /* Bitmap of supported page sizes */
+#define VFIO_IOMMU_INFO_CAPS (1 << 1) /* Info supports caps */
+ __u32 cap_offset; /* Offset within info struct of first cap */
+ __u64 iova_pgsizes; /* Bitmap of supported page sizes */
+};
+
+#define VFIO_IOMMU_TYPE1_INFO_CAP_MSI_GEOMETRY 1
+
+/*
+ * The MSI geometry capability allows to report the MSI IOVA geometry:
+ * - either the MSI IOVAs are constrained within a reserved IOVA aperture
+ * whose boundaries are given by [@aperture_start, @aperture_end].
+ * this is typically the case on x86 host. The userspace is not allowed
+ * to map userspace memory at IOVAs intersecting this range using
+ * VFIO_IOMMU_MAP_DMA.
+ * - or the MSI IOVAs are not requested to belong to any reserved range;
+ * in that case the userspace must provide an IOVA window characterized by
+ * @size and @alignment using VFIO_IOMMU_MAP_DMA with RESERVED_MSI_IOVA flag.
+ */
+struct vfio_iommu_type1_info_cap_msi_geometry {
+ struct vfio_info_cap_header header;
+ bool reserved; /* Are MSI IOVAs within a reserved aperture? */
+ /* reserved */
+ __u64 aperture_start;
+ __u64 aperture_end;
+ /* not reserved */
+ __u64 size; /* IOVA aperture size in bytes the userspace must provide */
+ __u64 alignment; /* alignment of the window, in bytes */
};
#define VFIO_IOMMU_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
@@ -503,6 +529,8 @@ struct vfio_iommu_type1_info {
* IOVA region that will be used on some platforms to map the host MSI frames.
* In that specific case, vaddr is ignored. Once registered, an MSI reserved
* IOVA region stays until the container is closed.
+ * The requirement for provisioning such reserved IOVA range can be checked by
+ * checking the VFIO_IOMMU_TYPE1_INFO_CAP_MSI_GEOMETRY capability.
*/
struct vfio_iommu_type1_dma_map {
__u32 argsz;
--
1.9.1
[toc] | [prev] | [next] | [standalone]
| From | Eric Auger <eric.auger@linaro.org> |
|---|---|
| Date | 2016-05-04 14:10 +0200 |
| Subject | Re: [PATCH v9 7/7] vfio/type1: return MSI geometry through VFIO_IOMMU_GET_INFO capability chains |
| Message-ID | <rv3MS-3xj-1@gated-at.bofh.it> |
| In reply to | #1394189 |
Hi Alex,
On 05/04/2016 01:54 PM, Eric Auger wrote:
> This patch allows the user-space to retrieve the MSI geometry. The
> implementation is based on capability chains, now also added to
> VFIO_IOMMU_GET_INFO.
If you prefer we could consider this patch outside of the main series
since it brings extra functionalities (MSI geometry reporting). In a
first QEMU integration we would live without knowing the MSI geometry I
think, all the more so I currently report an arbitrary number of
requested IOVA pages. The computation of the exact number of doorbells
to map brings extra complexity and I did not address this issue yet.
It sketches a possible user API to report the MSI geometry based on the
capability chains, as you suggested some time ago. I am currently busy
drafting a QEMU integration.
Best Regards
Eric
>
> The returned info comprise:
> - whether the MSI IOVA are constrained to a reserved range (x86 case) and
> in the positive, the start/end of the aperture,
> - or whether the IOVA aperture need to be set by the userspace. In that
> case, the size and alignment of the IOVA region to be provided are
> returned.
>
> In case the userspace must provide the IOVA range, we currently return
> an arbitrary number of IOVA pages (16), supposed to fulfill the needs of
> current ARM platforms. This may be deprecated by a more sophisticated
> computation later on.
>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>
> ---
> v8 -> v9:
> - use iommu_msi_supported flag instead of programmable
> - replace IOMMU_INFO_REQUIRE_MSI_MAP flag by a more sophisticated
> capability chain, reporting the MSI geometry
>
> v7 -> v8:
> - use iommu_domain_msi_geometry
>
> v6 -> v7:
> - remove the computation of the number of IOVA pages to be provisionned.
> This number depends on the domain/group/device topology which can
> dynamically change. Let's rely instead rely on an arbitrary max depending
> on the system
>
> v4 -> v5:
> - move msi_info and ret declaration within the conditional code
>
> v3 -> v4:
> - replace former vfio_domains_require_msi_mapping by
> more complex computation of MSI mapping requirements, especially the
> number of pages to be provided by the user-space.
> - reword patch title
>
> RFC v1 -> v1:
> - derived from
> [RFC PATCH 3/6] vfio: Extend iommu-info to return MSIs automap state
> - renamed allow_msi_reconfig into require_msi_mapping
> - fixed VFIO_IOMMU_GET_INFO
> ---
> drivers/vfio/vfio_iommu_type1.c | 69 +++++++++++++++++++++++++++++++++++++++++
> include/uapi/linux/vfio.h | 30 +++++++++++++++++-
> 2 files changed, 98 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 2fc8197..841360b 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -1134,6 +1134,50 @@ static int vfio_domains_have_iommu_cache(struct vfio_iommu *iommu)
> return ret;
> }
>
> +static int compute_msi_geometry_caps(struct vfio_iommu *iommu,
> + struct vfio_info_cap *caps)
> +{
> + struct vfio_iommu_type1_info_cap_msi_geometry *vfio_msi_geometry;
> + struct iommu_domain_msi_geometry msi_geometry;
> + struct vfio_info_cap_header *header;
> + struct vfio_domain *d;
> + bool mapping_required;
> + size_t size;
> +
> + mutex_lock(&iommu->lock);
> + /* All domains have same require_msi_map property, pick first */
> + d = list_first_entry(&iommu->domain_list, struct vfio_domain, next);
> + iommu_domain_get_attr(d->domain, DOMAIN_ATTR_MSI_GEOMETRY,
> + &msi_geometry);
> + mapping_required = msi_geometry.iommu_msi_supported;
> +
> + mutex_unlock(&iommu->lock);
> +
> + size = sizeof(*vfio_msi_geometry);
> + header = vfio_info_cap_add(caps, size,
> + VFIO_IOMMU_TYPE1_INFO_CAP_MSI_GEOMETRY, 1);
> +
> + if (IS_ERR(header))
> + return PTR_ERR(header);
> +
> + vfio_msi_geometry = container_of(header,
> + struct vfio_iommu_type1_info_cap_msi_geometry,
> + header);
> +
> + vfio_msi_geometry->reserved = !mapping_required;
> + if (vfio_msi_geometry->reserved) {
> + vfio_msi_geometry->aperture_start = msi_geometry.aperture_start;
> + vfio_msi_geometry->aperture_end = msi_geometry.aperture_end;
> + return 0;
> + }
> +
> + vfio_msi_geometry->alignment = 1 << __ffs(vfio_pgsize_bitmap(iommu));
> + /* we currently report the need for an arbitray number of 16 pages */
> + vfio_msi_geometry->size = 16 * vfio_msi_geometry->alignment;
> +
> + return 0;
> +}
> +
> static long vfio_iommu_type1_ioctl(void *iommu_data,
> unsigned int cmd, unsigned long arg)
> {
> @@ -1155,6 +1199,8 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
> }
> } else if (cmd == VFIO_IOMMU_GET_INFO) {
> struct vfio_iommu_type1_info info;
> + struct vfio_info_cap caps = { .buf = NULL, .size = 0 };
> + int ret;
>
> minsz = offsetofend(struct vfio_iommu_type1_info, iova_pgsizes);
>
> @@ -1168,6 +1214,29 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>
> info.iova_pgsizes = vfio_pgsize_bitmap(iommu);
>
> + ret = compute_msi_geometry_caps(iommu, &caps);
> + if (ret)
> + return ret;
> +
> + if (caps.size) {
> + info.flags |= VFIO_IOMMU_INFO_CAPS;
> + if (info.argsz < sizeof(info) + caps.size) {
> + info.argsz = sizeof(info) + caps.size;
> + info.cap_offset = 0;
> + } else {
> + vfio_info_cap_shift(&caps, sizeof(info));
> + if (copy_to_user((void __user *)arg +
> + sizeof(info), caps.buf,
> + caps.size)) {
> + kfree(caps.buf);
> + return -EFAULT;
> + }
> + info.cap_offset = sizeof(info);
> + }
> +
> + kfree(caps.buf);
> + }
> +
> return copy_to_user((void __user *)arg, &info, minsz) ?
> -EFAULT : 0;
>
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 4a9dbc2..0ff6a8d 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -488,7 +488,33 @@ struct vfio_iommu_type1_info {
> __u32 argsz;
> __u32 flags;
> #define VFIO_IOMMU_INFO_PGSIZES (1 << 0) /* supported page sizes info */
> - __u64 iova_pgsizes; /* Bitmap of supported page sizes */
> +#define VFIO_IOMMU_INFO_CAPS (1 << 1) /* Info supports caps */
> + __u32 cap_offset; /* Offset within info struct of first cap */
> + __u64 iova_pgsizes; /* Bitmap of supported page sizes */
> +};
> +
> +#define VFIO_IOMMU_TYPE1_INFO_CAP_MSI_GEOMETRY 1
> +
> +/*
> + * The MSI geometry capability allows to report the MSI IOVA geometry:
> + * - either the MSI IOVAs are constrained within a reserved IOVA aperture
> + * whose boundaries are given by [@aperture_start, @aperture_end].
> + * this is typically the case on x86 host. The userspace is not allowed
> + * to map userspace memory at IOVAs intersecting this range using
> + * VFIO_IOMMU_MAP_DMA.
> + * - or the MSI IOVAs are not requested to belong to any reserved range;
> + * in that case the userspace must provide an IOVA window characterized by
> + * @size and @alignment using VFIO_IOMMU_MAP_DMA with RESERVED_MSI_IOVA flag.
> + */
> +struct vfio_iommu_type1_info_cap_msi_geometry {
> + struct vfio_info_cap_header header;
> + bool reserved; /* Are MSI IOVAs within a reserved aperture? */
> + /* reserved */
> + __u64 aperture_start;
> + __u64 aperture_end;
> + /* not reserved */
> + __u64 size; /* IOVA aperture size in bytes the userspace must provide */
> + __u64 alignment; /* alignment of the window, in bytes */
> };
>
> #define VFIO_IOMMU_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
> @@ -503,6 +529,8 @@ struct vfio_iommu_type1_info {
> * IOVA region that will be used on some platforms to map the host MSI frames.
> * In that specific case, vaddr is ignored. Once registered, an MSI reserved
> * IOVA region stays until the container is closed.
> + * The requirement for provisioning such reserved IOVA range can be checked by
> + * checking the VFIO_IOMMU_TYPE1_INFO_CAP_MSI_GEOMETRY capability.
> */
> struct vfio_iommu_type1_dma_map {
> __u32 argsz;
>
[toc] | [prev] | [next] | [standalone]
| From | Alex Williamson <alex.williamson@redhat.com> |
|---|---|
| Date | 2016-05-10 01:10 +0200 |
| Subject | Re: [PATCH v9 7/7] vfio/type1: return MSI geometry through VFIO_IOMMU_GET_INFO capability chains |
| Message-ID | <rx2tj-76w-1@gated-at.bofh.it> |
| In reply to | #1394203 |
On Wed, 4 May 2016 14:06:19 +0200 Eric Auger <eric.auger@linaro.org> wrote: > Hi Alex, > On 05/04/2016 01:54 PM, Eric Auger wrote: > > This patch allows the user-space to retrieve the MSI geometry. The > > implementation is based on capability chains, now also added to > > VFIO_IOMMU_GET_INFO. > > If you prefer we could consider this patch outside of the main series > since it brings extra functionalities (MSI geometry reporting). In a > first QEMU integration we would live without knowing the MSI geometry I > think, all the more so I currently report an arbitrary number of > requested IOVA pages. The computation of the exact number of doorbells > to map brings extra complexity and I did not address this issue yet. > > It sketches a possible user API to report the MSI geometry based on the > capability chains, as you suggested some time ago. I am currently busy > drafting a QEMU integration. How would the user know that reserved MSI mappings are requires or available without this? Wouldn't the only option be for userspace to try to map something with the reserved MSI flag set and see if the kernel accepts it? That's not a very desirable programming model. The arbitrary size is pretty ugly, but it at least makes for a consistent user interface. Is it a functional issue if we overestimate the size or is it just a matter of wasting IOVA space? Is there significant harm in making it obscenely large, like 1MB? The reference counting and re-use of IOVA pages seems like we may often only be using a single IOVA page for multiple doorbells. I guess I'm leaning towards defining the API even if the value is somewhat arbitrary because we'd rather have control of this rather than having the user guess and try to rope them back in later to use a kernel recommended value. Thanks, Alex
[toc] | [prev] | [next] | [standalone]
| From | Eric Auger <eric.auger@linaro.org> |
|---|---|
| Date | 2016-05-10 19:00 +0200 |
| Subject | Re: [PATCH v9 7/7] vfio/type1: return MSI geometry through VFIO_IOMMU_GET_INFO capability chains |
| Message-ID | <rxjaP-6ly-17@gated-at.bofh.it> |
| In reply to | #1397498 |
On 05/10/2016 01:03 AM, Alex Williamson wrote: > On Wed, 4 May 2016 14:06:19 +0200 > Eric Auger <eric.auger@linaro.org> wrote: > >> Hi Alex, >> On 05/04/2016 01:54 PM, Eric Auger wrote: >>> This patch allows the user-space to retrieve the MSI geometry. The >>> implementation is based on capability chains, now also added to >>> VFIO_IOMMU_GET_INFO. >> >> If you prefer we could consider this patch outside of the main series >> since it brings extra functionalities (MSI geometry reporting). In a >> first QEMU integration we would live without knowing the MSI geometry I >> think, all the more so I currently report an arbitrary number of >> requested IOVA pages. The computation of the exact number of doorbells >> to map brings extra complexity and I did not address this issue yet. >> >> It sketches a possible user API to report the MSI geometry based on the >> capability chains, as you suggested some time ago. I am currently busy >> drafting a QEMU integration. > > How would the user know that reserved MSI mappings are requires or > available without this? Wouldn't the only option be for userspace to > try to map something with the reserved MSI flag set and see if the > kernel accepts it? Well my first guess was that the (QEMU) virt machine using KVM/PCIe-MSI passthrough could hardcode an arbitrary "large" iova size (currently 16 64kB pages in my QEMU integration). In case this is not sufficient for mapping all host doorbells, we would see MSI allocation failing. In case the need shows up, we could increase the value later on. That's not a very desirable programming model. The > arbitrary size is pretty ugly, but it at least makes for a consistent > user interface. Is it a functional issue if we overestimate the size > or is it just a matter of wasting IOVA space? Is there significant > harm in making it obscenely large, like 1MB? no just waste of IOVA space. To make it as transparent as possible for virt machine I wanted to hide in the platform bus reserved IOVA. The reference counting and > re-use of IOVA pages seems like we may often only be using a single > IOVA page for multiple doorbells. I guess I'm leaning towards defining > the API even if the value is somewhat arbitrary because we'd rather have > control of this rather than having the user guess and try to rope them > back in later to use a kernel recommended value. Thanks, OK Thanks Eric > > Alex >
[toc] | [prev] | [next] | [standalone]
| From | Alex Williamson <alex.williamson@redhat.com> |
|---|---|
| Date | 2016-05-10 01:00 +0200 |
| Subject | Re: [PATCH v9 7/7] vfio/type1: return MSI geometry through VFIO_IOMMU_GET_INFO capability chains |
| Message-ID | <rx2jE-6Am-17@gated-at.bofh.it> |
| In reply to | #1394189 |
On Wed, 4 May 2016 11:54:18 +0000
Eric Auger <eric.auger@linaro.org> wrote:
> This patch allows the user-space to retrieve the MSI geometry. The
> implementation is based on capability chains, now also added to
> VFIO_IOMMU_GET_INFO.
>
> The returned info comprise:
> - whether the MSI IOVA are constrained to a reserved range (x86 case) and
> in the positive, the start/end of the aperture,
> - or whether the IOVA aperture need to be set by the userspace. In that
> case, the size and alignment of the IOVA region to be provided are
> returned.
>
> In case the userspace must provide the IOVA range, we currently return
> an arbitrary number of IOVA pages (16), supposed to fulfill the needs of
> current ARM platforms. This may be deprecated by a more sophisticated
> computation later on.
>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>
> ---
> v8 -> v9:
> - use iommu_msi_supported flag instead of programmable
> - replace IOMMU_INFO_REQUIRE_MSI_MAP flag by a more sophisticated
> capability chain, reporting the MSI geometry
>
> v7 -> v8:
> - use iommu_domain_msi_geometry
>
> v6 -> v7:
> - remove the computation of the number of IOVA pages to be provisionned.
> This number depends on the domain/group/device topology which can
> dynamically change. Let's rely instead rely on an arbitrary max depending
> on the system
>
> v4 -> v5:
> - move msi_info and ret declaration within the conditional code
>
> v3 -> v4:
> - replace former vfio_domains_require_msi_mapping by
> more complex computation of MSI mapping requirements, especially the
> number of pages to be provided by the user-space.
> - reword patch title
>
> RFC v1 -> v1:
> - derived from
> [RFC PATCH 3/6] vfio: Extend iommu-info to return MSIs automap state
> - renamed allow_msi_reconfig into require_msi_mapping
> - fixed VFIO_IOMMU_GET_INFO
> ---
> drivers/vfio/vfio_iommu_type1.c | 69 +++++++++++++++++++++++++++++++++++++++++
> include/uapi/linux/vfio.h | 30 +++++++++++++++++-
> 2 files changed, 98 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 2fc8197..841360b 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -1134,6 +1134,50 @@ static int vfio_domains_have_iommu_cache(struct vfio_iommu *iommu)
> return ret;
> }
>
> +static int compute_msi_geometry_caps(struct vfio_iommu *iommu,
> + struct vfio_info_cap *caps)
> +{
> + struct vfio_iommu_type1_info_cap_msi_geometry *vfio_msi_geometry;
> + struct iommu_domain_msi_geometry msi_geometry;
> + struct vfio_info_cap_header *header;
> + struct vfio_domain *d;
> + bool mapping_required;
> + size_t size;
> +
> + mutex_lock(&iommu->lock);
> + /* All domains have same require_msi_map property, pick first */
> + d = list_first_entry(&iommu->domain_list, struct vfio_domain, next);
> + iommu_domain_get_attr(d->domain, DOMAIN_ATTR_MSI_GEOMETRY,
> + &msi_geometry);
> + mapping_required = msi_geometry.iommu_msi_supported;
> +
> + mutex_unlock(&iommu->lock);
> +
> + size = sizeof(*vfio_msi_geometry);
> + header = vfio_info_cap_add(caps, size,
> + VFIO_IOMMU_TYPE1_INFO_CAP_MSI_GEOMETRY, 1);
> +
> + if (IS_ERR(header))
> + return PTR_ERR(header);
> +
> + vfio_msi_geometry = container_of(header,
> + struct vfio_iommu_type1_info_cap_msi_geometry,
> + header);
> +
> + vfio_msi_geometry->reserved = !mapping_required;
> + if (vfio_msi_geometry->reserved) {
> + vfio_msi_geometry->aperture_start = msi_geometry.aperture_start;
> + vfio_msi_geometry->aperture_end = msi_geometry.aperture_end;
> + return 0;
> + }
> +
> + vfio_msi_geometry->alignment = 1 << __ffs(vfio_pgsize_bitmap(iommu));
> + /* we currently report the need for an arbitray number of 16 pages */
> + vfio_msi_geometry->size = 16 * vfio_msi_geometry->alignment;
Hmm, that really is arbitrary. How could we know a real value here?
> +
> + return 0;
> +}
> +
> static long vfio_iommu_type1_ioctl(void *iommu_data,
> unsigned int cmd, unsigned long arg)
> {
> @@ -1155,6 +1199,8 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
> }
> } else if (cmd == VFIO_IOMMU_GET_INFO) {
> struct vfio_iommu_type1_info info;
> + struct vfio_info_cap caps = { .buf = NULL, .size = 0 };
> + int ret;
>
> minsz = offsetofend(struct vfio_iommu_type1_info, iova_pgsizes);
>
> @@ -1168,6 +1214,29 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>
> info.iova_pgsizes = vfio_pgsize_bitmap(iommu);
>
> + ret = compute_msi_geometry_caps(iommu, &caps);
> + if (ret)
> + return ret;
> +
> + if (caps.size) {
> + info.flags |= VFIO_IOMMU_INFO_CAPS;
> + if (info.argsz < sizeof(info) + caps.size) {
> + info.argsz = sizeof(info) + caps.size;
> + info.cap_offset = 0;
> + } else {
> + vfio_info_cap_shift(&caps, sizeof(info));
> + if (copy_to_user((void __user *)arg +
> + sizeof(info), caps.buf,
> + caps.size)) {
> + kfree(caps.buf);
> + return -EFAULT;
> + }
> + info.cap_offset = sizeof(info);
> + }
> +
> + kfree(caps.buf);
> + }
> +
> return copy_to_user((void __user *)arg, &info, minsz) ?
> -EFAULT : 0;
>
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 4a9dbc2..0ff6a8d 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -488,7 +488,33 @@ struct vfio_iommu_type1_info {
> __u32 argsz;
> __u32 flags;
> #define VFIO_IOMMU_INFO_PGSIZES (1 << 0) /* supported page sizes info */
> - __u64 iova_pgsizes; /* Bitmap of supported page sizes */
> +#define VFIO_IOMMU_INFO_CAPS (1 << 1) /* Info supports caps */
> + __u32 cap_offset; /* Offset within info struct of first cap */
> + __u64 iova_pgsizes; /* Bitmap of supported page sizes */
This would break existing users, we can't arbitrarily change the offset
of iova_pgsizes. We can add cap_offset to the end and I think
everything would work about above if we do that.
> +};
> +
> +#define VFIO_IOMMU_TYPE1_INFO_CAP_MSI_GEOMETRY 1
> +
> +/*
> + * The MSI geometry capability allows to report the MSI IOVA geometry:
> + * - either the MSI IOVAs are constrained within a reserved IOVA aperture
> + * whose boundaries are given by [@aperture_start, @aperture_end].
> + * this is typically the case on x86 host. The userspace is not allowed
> + * to map userspace memory at IOVAs intersecting this range using
> + * VFIO_IOMMU_MAP_DMA.
> + * - or the MSI IOVAs are not requested to belong to any reserved range;
> + * in that case the userspace must provide an IOVA window characterized by
> + * @size and @alignment using VFIO_IOMMU_MAP_DMA with RESERVED_MSI_IOVA flag.
> + */
> +struct vfio_iommu_type1_info_cap_msi_geometry {
> + struct vfio_info_cap_header header;
> + bool reserved; /* Are MSI IOVAs within a reserved aperture? */
Do bools have a guaranteed user size? Let's make this a __u32 and call
it flags with bit 0 defined as reserved. I'm tempted to suggest we
could figure out how to make alignment fit in another __u32 so we have a
properly packed structure, otherwise we should make a reserved __u32.
> + /* reserved */
> + __u64 aperture_start;
> + __u64 aperture_end;
> + /* not reserved */
> + __u64 size; /* IOVA aperture size in bytes the userspace must provide */
> + __u64 alignment; /* alignment of the window, in bytes */
> };
>
> #define VFIO_IOMMU_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
> @@ -503,6 +529,8 @@ struct vfio_iommu_type1_info {
> * IOVA region that will be used on some platforms to map the host MSI frames.
> * In that specific case, vaddr is ignored. Once registered, an MSI reserved
> * IOVA region stays until the container is closed.
> + * The requirement for provisioning such reserved IOVA range can be checked by
> + * checking the VFIO_IOMMU_TYPE1_INFO_CAP_MSI_GEOMETRY capability.
> */
> struct vfio_iommu_type1_dma_map {
> __u32 argsz;
[toc] | [prev] | [next] | [standalone]
| From | Eric Auger <eric.auger@linaro.org> |
|---|---|
| Date | 2016-05-10 18:40 +0200 |
| Subject | Re: [PATCH v9 7/7] vfio/type1: return MSI geometry through VFIO_IOMMU_GET_INFO capability chains |
| Message-ID | <rxiRu-6cC-45@gated-at.bofh.it> |
| In reply to | #1397497 |
Hi Alex,
On 05/10/2016 12:49 AM, Alex Williamson wrote:
> On Wed, 4 May 2016 11:54:18 +0000
> Eric Auger <eric.auger@linaro.org> wrote:
>
>> This patch allows the user-space to retrieve the MSI geometry. The
>> implementation is based on capability chains, now also added to
>> VFIO_IOMMU_GET_INFO.
>>
>> The returned info comprise:
>> - whether the MSI IOVA are constrained to a reserved range (x86 case) and
>> in the positive, the start/end of the aperture,
>> - or whether the IOVA aperture need to be set by the userspace. In that
>> case, the size and alignment of the IOVA region to be provided are
>> returned.
>>
>> In case the userspace must provide the IOVA range, we currently return
>> an arbitrary number of IOVA pages (16), supposed to fulfill the needs of
>> current ARM platforms. This may be deprecated by a more sophisticated
>> computation later on.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>> v8 -> v9:
>> - use iommu_msi_supported flag instead of programmable
>> - replace IOMMU_INFO_REQUIRE_MSI_MAP flag by a more sophisticated
>> capability chain, reporting the MSI geometry
>>
>> v7 -> v8:
>> - use iommu_domain_msi_geometry
>>
>> v6 -> v7:
>> - remove the computation of the number of IOVA pages to be provisionned.
>> This number depends on the domain/group/device topology which can
>> dynamically change. Let's rely instead rely on an arbitrary max depending
>> on the system
>>
>> v4 -> v5:
>> - move msi_info and ret declaration within the conditional code
>>
>> v3 -> v4:
>> - replace former vfio_domains_require_msi_mapping by
>> more complex computation of MSI mapping requirements, especially the
>> number of pages to be provided by the user-space.
>> - reword patch title
>>
>> RFC v1 -> v1:
>> - derived from
>> [RFC PATCH 3/6] vfio: Extend iommu-info to return MSIs automap state
>> - renamed allow_msi_reconfig into require_msi_mapping
>> - fixed VFIO_IOMMU_GET_INFO
>> ---
>> drivers/vfio/vfio_iommu_type1.c | 69 +++++++++++++++++++++++++++++++++++++++++
>> include/uapi/linux/vfio.h | 30 +++++++++++++++++-
>> 2 files changed, 98 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>> index 2fc8197..841360b 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -1134,6 +1134,50 @@ static int vfio_domains_have_iommu_cache(struct vfio_iommu *iommu)
>> return ret;
>> }
>>
>> +static int compute_msi_geometry_caps(struct vfio_iommu *iommu,
>> + struct vfio_info_cap *caps)
>> +{
>> + struct vfio_iommu_type1_info_cap_msi_geometry *vfio_msi_geometry;
>> + struct iommu_domain_msi_geometry msi_geometry;
>> + struct vfio_info_cap_header *header;
>> + struct vfio_domain *d;
>> + bool mapping_required;
>> + size_t size;
>> +
>> + mutex_lock(&iommu->lock);
>> + /* All domains have same require_msi_map property, pick first */
>> + d = list_first_entry(&iommu->domain_list, struct vfio_domain, next);
>> + iommu_domain_get_attr(d->domain, DOMAIN_ATTR_MSI_GEOMETRY,
>> + &msi_geometry);
>> + mapping_required = msi_geometry.iommu_msi_supported;
>> +
>> + mutex_unlock(&iommu->lock);
>> +
>> + size = sizeof(*vfio_msi_geometry);
>> + header = vfio_info_cap_add(caps, size,
>> + VFIO_IOMMU_TYPE1_INFO_CAP_MSI_GEOMETRY, 1);
>> +
>> + if (IS_ERR(header))
>> + return PTR_ERR(header);
>> +
>> + vfio_msi_geometry = container_of(header,
>> + struct vfio_iommu_type1_info_cap_msi_geometry,
>> + header);
>> +
>> + vfio_msi_geometry->reserved = !mapping_required;
>> + if (vfio_msi_geometry->reserved) {
>> + vfio_msi_geometry->aperture_start = msi_geometry.aperture_start;
>> + vfio_msi_geometry->aperture_end = msi_geometry.aperture_end;
>> + return 0;
>> + }
>> +
>> + vfio_msi_geometry->alignment = 1 << __ffs(vfio_pgsize_bitmap(iommu));
>> + /* we currently report the need for an arbitray number of 16 pages */
>> + vfio_msi_geometry->size = 16 * vfio_msi_geometry->alignment;
>
> Hmm, that really is arbitrary. How could we know a real value here?
Yes I fully agree and this is aknowledged in the cover/commit msg. I
dared to do that because this has the benefits to allow introducing the
userspace API while refining this computation later on.
I did not find yet an elegant solution to compute the platform max
number/size of doorbells (besides what I did in the past which was
dependent on the group/device current topology).
Maybe an option would be to have the relevant MSI controllers
registering their doorbells in a global list at probe time and then we
would enumerate all of them. I was reluctant to add this new
functionality in the series at this stage, hence the current simplification.
>
>> +
>> + return 0;
>> +}
>> +
>> static long vfio_iommu_type1_ioctl(void *iommu_data,
>> unsigned int cmd, unsigned long arg)
>> {
>> @@ -1155,6 +1199,8 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>> }
>> } else if (cmd == VFIO_IOMMU_GET_INFO) {
>> struct vfio_iommu_type1_info info;
>> + struct vfio_info_cap caps = { .buf = NULL, .size = 0 };
>> + int ret;
>>
>> minsz = offsetofend(struct vfio_iommu_type1_info, iova_pgsizes);
>>
>> @@ -1168,6 +1214,29 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>>
>> info.iova_pgsizes = vfio_pgsize_bitmap(iommu);
>>
>> + ret = compute_msi_geometry_caps(iommu, &caps);
>> + if (ret)
>> + return ret;
>> +
>> + if (caps.size) {
>> + info.flags |= VFIO_IOMMU_INFO_CAPS;
>> + if (info.argsz < sizeof(info) + caps.size) {
>> + info.argsz = sizeof(info) + caps.size;
>> + info.cap_offset = 0;
>> + } else {
>> + vfio_info_cap_shift(&caps, sizeof(info));
>> + if (copy_to_user((void __user *)arg +
>> + sizeof(info), caps.buf,
>> + caps.size)) {
>> + kfree(caps.buf);
>> + return -EFAULT;
>> + }
>> + info.cap_offset = sizeof(info);
>> + }
>> +
>> + kfree(caps.buf);
>> + }
>> +
>> return copy_to_user((void __user *)arg, &info, minsz) ?
>> -EFAULT : 0;
>>
>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> index 4a9dbc2..0ff6a8d 100644
>> --- a/include/uapi/linux/vfio.h
>> +++ b/include/uapi/linux/vfio.h
>> @@ -488,7 +488,33 @@ struct vfio_iommu_type1_info {
>> __u32 argsz;
>> __u32 flags;
>> #define VFIO_IOMMU_INFO_PGSIZES (1 << 0) /* supported page sizes info */
>> - __u64 iova_pgsizes; /* Bitmap of supported page sizes */
>> +#define VFIO_IOMMU_INFO_CAPS (1 << 1) /* Info supports caps */
>> + __u32 cap_offset; /* Offset within info struct of first cap */
>> + __u64 iova_pgsizes; /* Bitmap of supported page sizes */
>
> This would break existing users, we can't arbitrarily change the offset
> of iova_pgsizes. We can add cap_offset to the end and I think
> everything would work about above if we do that.
Hum yes, sorry for the lack of care.
>
>> +};
>> +
>> +#define VFIO_IOMMU_TYPE1_INFO_CAP_MSI_GEOMETRY 1
>> +
>> +/*
>> + * The MSI geometry capability allows to report the MSI IOVA geometry:
>> + * - either the MSI IOVAs are constrained within a reserved IOVA aperture
>> + * whose boundaries are given by [@aperture_start, @aperture_end].
>> + * this is typically the case on x86 host. The userspace is not allowed
>> + * to map userspace memory at IOVAs intersecting this range using
>> + * VFIO_IOMMU_MAP_DMA.
>> + * - or the MSI IOVAs are not requested to belong to any reserved range;
>> + * in that case the userspace must provide an IOVA window characterized by
>> + * @size and @alignment using VFIO_IOMMU_MAP_DMA with RESERVED_MSI_IOVA flag.
>> + */
>> +struct vfio_iommu_type1_info_cap_msi_geometry {
>> + struct vfio_info_cap_header header;
>> + bool reserved; /* Are MSI IOVAs within a reserved aperture? */
>
> Do bools have a guaranteed user size? Let's make this a __u32 and call
> it flags with bit 0 defined as reserved. I'm tempted to suggest we
> could figure out how to make alignment fit in another __u32 so we have a
> properly packed structure, otherwise we should make a reserved __u32.
OK will rewrite & check that.
Thanks
Eric
>
>> + /* reserved */
>> + __u64 aperture_start;
>> + __u64 aperture_end;
>> + /* not reserved */
>> + __u64 size; /* IOVA aperture size in bytes the userspace must provide */
>> + __u64 alignment; /* alignment of the window, in bytes */
>> };
>>
>> #define VFIO_IOMMU_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
>> @@ -503,6 +529,8 @@ struct vfio_iommu_type1_info {
>> * IOVA region that will be used on some platforms to map the host MSI frames.
>> * In that specific case, vaddr is ignored. Once registered, an MSI reserved
>> * IOVA region stays until the container is closed.
>> + * The requirement for provisioning such reserved IOVA range can be checked by
>> + * checking the VFIO_IOMMU_TYPE1_INFO_CAP_MSI_GEOMETRY capability.
>> */
>> struct vfio_iommu_type1_dma_map {
>> __u32 argsz;
>
[toc] | [prev] | [next] | [standalone]
Page 1 of 2 [1] 2 Next page →
Back to top | Article view | linux.kernel
csiph-web