Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1515412 > unrolled thread
| Started by | Kirti Wankhede <kwankhede@nvidia.com> |
|---|---|
| First post | 2016-11-04 22:10 +0100 |
| Last post | 2016-11-07 07:50 +0100 |
| Articles | 20 on this page of 61 — 5 participants |
Back to article view | Back to linux.kernel
[PATCH v11 00/22] Add Mediated device support Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-04 22:10 +0100
[PATCH v11 12/22] vfio: Add notifier callback to parent's ops structure of mdev Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-04 22:20 +0100
Re: [PATCH v11 12/22] vfio: Add notifier callback to parent's ops structure of mdev Alex Williamson <alex.williamson@redhat.com> - 2016-11-08 01:00 +0100
[PATCH v11 06/22] vfio iommu type1: Update arguments of vfio_lock_acct Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-04 22:20 +0100
[PATCH v11 17/22] vfio_platform: Updated to use vfio_set_irqs_validate_and_prepare() Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-04 22:20 +0100
Re: [PATCH v11 17/22] vfio_platform: Updated to use vfio_set_irqs_validate_and_prepare() Alexey Kardashevskiy <aik@ozlabs.ru> - 2016-11-08 11:00 +0100
Re: [PATCH v11 17/22] vfio_platform: Updated to use vfio_set_irqs_validate_and_prepare() Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-08 21:50 +0100
[PATCH v11 20/22] docs: Sysfs ABI for mediated device framework Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-04 22:20 +0100
[PATCH v11 16/22] vfio_pci: Updated to use vfio_set_irqs_validate_and_prepare() Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-04 22:20 +0100
[PATCH v11 04/22] vfio: Common function to increment container_users Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-04 22:20 +0100
[PATCH v11 19/22] docs: Add Documentation for Mediated devices Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-04 22:20 +0100
[PATCH v11 13/22] vfio: Introduce common function to add capabilities Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-04 22:20 +0100
Re: [PATCH v11 13/22] vfio: Introduce common function to add capabilities Alexey Kardashevskiy <aik@ozlabs.ru> - 2016-11-08 08:30 +0100
Re: [PATCH v11 13/22] vfio: Introduce common function to add capabilities Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-08 21:50 +0100
Re: [PATCH v11 13/22] vfio: Introduce common function to add capabilities Alex Williamson <alex.williamson@redhat.com> - 2016-11-08 22:50 +0100
Re: [PATCH v11 13/22] vfio: Introduce common function to add capabilities Alexey Kardashevskiy <aik@ozlabs.ru> - 2016-11-09 03:30 +0100
[PATCH v11 03/22] vfio: Rearrange functions to get vfio_group from dev Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-04 22:20 +0100
[PATCH v11 14/22] vfio_pci: Update vfio_pci to use vfio_info_add_capability() Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-04 22:20 +0100
[PATCH v11 09/22] vfio iommu type1: Add task structure to vfio_dma Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-04 22:20 +0100
Re: [PATCH v11 09/22] vfio iommu type1: Add task structure to vfio_dma Alex Williamson <alex.williamson@redhat.com> - 2016-11-07 22:10 +0100
Re: [PATCH v11 09/22] vfio iommu type1: Add task structure to vfio_dma Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-08 15:20 +0100
Re: [PATCH v11 09/22] vfio iommu type1: Add task structure to vfio_dma Alex Williamson <alex.williamson@redhat.com> - 2016-11-08 17:50 +0100
[PATCH v11 02/22] vfio: VFIO based driver for Mediated devices Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-04 22:20 +0100
[PATCH v11 18/22] vfio: Define device_api strings Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-04 22:20 +0100
[PATCH v11 07/22] vfio iommu type1: Update argument of vaddr_get_pfn() Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-04 22:20 +0100
Re: [PATCH v11 07/22] vfio iommu type1: Update argument of vaddr_get_pfn() Alexey Kardashevskiy <aik@ozlabs.ru> - 2016-11-07 09:50 +0100
[PATCH v11 15/22] vfio: Introduce vfio_set_irqs_validate_and_prepare() Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-04 22:20 +0100
Re: [PATCH v11 15/22] vfio: Introduce vfio_set_irqs_validate_and_prepare() Alexey Kardashevskiy <aik@ozlabs.ru> - 2016-11-08 09:50 +0100
Re: [PATCH v11 15/22] vfio: Introduce vfio_set_irqs_validate_and_prepare() Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-08 21:30 +0100
Re: [PATCH v11 15/22] vfio: Introduce vfio_set_irqs_validate_and_prepare() Alexey Kardashevskiy <aik@ozlabs.ru> - 2016-11-09 04:10 +0100
Re: [PATCH v11 15/22] vfio: Introduce vfio_set_irqs_validate_and_prepare() Alex Williamson <alex.williamson@redhat.com> - 2016-11-09 04:40 +0100
[PATCH v11 10/22] vfio iommu type1: Add support for mediated devices Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-04 22:20 +0100
Re: [PATCH v11 10/22] vfio iommu type1: Add support for mediated devices Alex Williamson <alex.williamson@redhat.com> - 2016-11-08 00:20 +0100
Re: [PATCH v11 10/22] vfio iommu type1: Add support for mediated devices Jike Song <jike.song@intel.com> - 2016-11-08 03:30 +0100
Re: [PATCH v11 10/22] vfio iommu type1: Add support for mediated devices Alex Williamson <alex.williamson@redhat.com> - 2016-11-08 17:20 +0100
Re: [PATCH v11 10/22] vfio iommu type1: Add support for mediated devices Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-08 16:10 +0100
Re: [PATCH v11 10/22] vfio iommu type1: Add support for mediated devices Alex Williamson <alex.williamson@redhat.com> - 2016-11-08 18:10 +0100
Re: [PATCH v11 10/22] vfio iommu type1: Add support for mediated devices Alexey Kardashevskiy <aik@ozlabs.ru> - 2016-11-08 08:00 +0100
[PATCH v11 22/22] MAINTAINERS: Add entry VFIO based Mediated device drivers Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-04 22:20 +0100
[PATCH v11 08/22] vfio iommu type1: Add find_iommu_group() function Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-04 22:20 +0100
[PATCH v11 11/22] vfio iommu: Add blocking notifier to notify DMA_UNMAP Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-04 22:20 +0100
Re: [PATCH v11 11/22] vfio iommu: Add blocking notifier to notify DMA_UNMAP Alex Williamson <alex.williamson@redhat.com> - 2016-11-08 00:50 +0100
Re: [PATCH v11 11/22] vfio iommu: Add blocking notifier to notify DMA_UNMAP Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-08 17:30 +0100
Re: [PATCH v11 11/22] vfio iommu: Add blocking notifier to notify DMA_UNMAP Alex Williamson <alex.williamson@redhat.com> - 2016-11-08 18:50 +0100
Re: [PATCH v11 11/22] vfio iommu: Add blocking notifier to notify DMA_UNMAP Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-08 21:10 +0100
Re: [PATCH v11 11/22] vfio iommu: Add blocking notifier to notify DMA_UNMAP Alex Williamson <alex.williamson@redhat.com> - 2016-11-08 22:30 +0100
[PATCH v11 01/22] vfio: Mediated device Core driver Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-04 22:20 +0100
RE: [PATCH v11 01/22] vfio: Mediated device Core driver "Tian, Kevin" <kevin.tian@intel.com> - 2016-11-07 07:50 +0100
Re: [PATCH v11 01/22] vfio: Mediated device Core driver Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-08 22:20 +0100
[PATCH v11 05/22] vfio iommu: Added pin and unpin callback functions to vfio_iommu_driver_ops Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-04 22:20 +0100
Re: [PATCH v11 05/22] vfio iommu: Added pin and unpin callback functions to vfio_iommu_driver_ops Alex Williamson <alex.williamson@redhat.com> - 2016-11-07 20:40 +0100
Re: [PATCH v11 05/22] vfio iommu: Added pin and unpin callback functions to vfio_iommu_driver_ops Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-08 15:00 +0100
Re: [PATCH v11 05/22] vfio iommu: Added pin and unpin callback functions to vfio_iommu_driver_ops Alex Williamson <alex.williamson@redhat.com> - 2016-11-08 17:50 +0100
Re: [PATCH v11 05/22] vfio iommu: Added pin and unpin callback functions to vfio_iommu_driver_ops Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-08 20:00 +0100
Re: [PATCH v11 05/22] vfio iommu: Added pin and unpin callback functions to vfio_iommu_driver_ops Alex Williamson <alex.williamson@redhat.com> - 2016-11-08 20:20 +0100
Re: [PATCH v11 00/22] Add Mediated device support Alexey Kardashevskiy <aik@ozlabs.ru> - 2016-11-07 04:50 +0100
Re: [PATCH v11 00/22] Add Mediated device support Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-07 05:00 +0100
Re: [PATCH v11 00/22] Add Mediated device support Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-07 06:10 +0100
Re: [PATCH v11 00/22] Add Mediated device support Alexey Kardashevskiy <aik@ozlabs.ru> - 2016-11-07 07:20 +0100
Re: [PATCH v11 00/22] Add Mediated device support Kirti Wankhede <kwankhede@nvidia.com> - 2016-11-07 07:40 +0100
Re: [PATCH v11 00/22] Add Mediated device support Alexey Kardashevskiy <aik@ozlabs.ru> - 2016-11-07 07:50 +0100
Page 2 of 4 — ← Prev page 1 [2] 3 4 Next page →
| From | Kirti Wankhede <kwankhede@nvidia.com> |
|---|---|
| Date | 2016-11-08 15:20 +0100 |
| Subject | Re: [PATCH v11 09/22] vfio iommu type1: Add task structure to vfio_dma |
| Message-ID | <sBfpL-20X-7@gated-at.bofh.it> |
| In reply to | #1516584 |
On 11/8/2016 2:33 AM, Alex Williamson wrote:
> On Sat, 5 Nov 2016 02:40:43 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>
...
>> static int vfio_dma_do_map(struct vfio_iommu *iommu,
>> struct vfio_iommu_type1_dma_map *map)
>> {
>> dma_addr_t iova = map->iova;
>> unsigned long vaddr = map->vaddr;
>> size_t size = map->size;
>> - long npage;
>> int ret = 0, prot = 0;
>> uint64_t mask;
>> struct vfio_dma *dma;
>> - unsigned long pfn;
>> + struct vfio_addr_space *addr_space;
>> + struct mm_struct *mm;
>> + bool free_addr_space_on_err = false;
>>
>> /* Verify that none of our __u64 fields overflow */
>> if (map->size != size || map->vaddr != vaddr || map->iova != iova)
>> @@ -608,47 +685,56 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
>> mutex_lock(&iommu->lock);
>>
>> if (vfio_find_dma(iommu, iova, size)) {
>> - mutex_unlock(&iommu->lock);
>> - return -EEXIST;
>> + ret = -EEXIST;
>> + goto do_map_err;
>> + }
>> +
>> + mm = get_task_mm(current);
>> + if (!mm) {
>> + ret = -ENODEV;
>
> -EFAULT?
>
-ENODEV return is in original code from vfio_pin_pages()
if (!current->mm)
return -ENODEV;
Once I thought of changing it to -EFAULT, but then again changed to
-ENODEV to be consistent with original error code.
Should I still change this return to -EFAULT?
>> + goto do_map_err;
>> + }
>> +
>> + addr_space = vfio_find_addr_space(iommu, mm);
>> + if (addr_space) {
>> + atomic_inc(&addr_space->ref_count);
>> + mmput(mm);
>> + } else {
>> + addr_space = kzalloc(sizeof(*addr_space), GFP_KERNEL);
>> + if (!addr_space) {
>> + ret = -ENOMEM;
>> + goto do_map_err;
>> + }
>> + addr_space->mm = mm;
>> + atomic_set(&addr_space->ref_count, 1);
>> + list_add(&addr_space->next, &iommu->addr_space_list);
>> + free_addr_space_on_err = true;
>> }
>>
>> dma = kzalloc(sizeof(*dma), GFP_KERNEL);
>> if (!dma) {
>> - mutex_unlock(&iommu->lock);
>> - return -ENOMEM;
>> + if (free_addr_space_on_err) {
>> + mmput(mm);
>> + list_del(&addr_space->next);
>> + kfree(addr_space);
>> + }
>> + ret = -ENOMEM;
>> + goto do_map_err;
>> }
>>
>> dma->iova = iova;
>> dma->vaddr = vaddr;
>> dma->prot = prot;
>> + dma->addr_space = addr_space;
>> + get_task_struct(current);
>> + dma->task = current;
>> + dma->mlock_cap = capable(CAP_IPC_LOCK);
>
>
> How do you reason we can cache this? Does the fact that the process
> had this capability at the time that it did a DMA_MAP imply that it
> necessarily still has this capability when an external user (vendor
> driver) tries to pin pages? I don't see how we can make that
> assumption.
>
>
Will process change MEMLOCK limit at runtime? I think it shouldn't,
correct me if I'm wrong. QEMU doesn't do that, right?
The function capable() determines current task's capability. But when
vfio_pin_pages() is called, it could come from other task but pages are
pinned from address space of task who mapped it. So we can't use
capable() in vfio_pin_pages()
If this capability shouldn't be cached, we have to use has_capability()
with dma->task as argument in vfio_pin_pages()
bool has_capability(struct task_struct *t, int cap)
Thanks,
Kirti
[toc] | [prev] | [next] | [standalone]
| From | Alex Williamson <alex.williamson@redhat.com> |
|---|---|
| Date | 2016-11-08 17:50 +0100 |
| Subject | Re: [PATCH v11 09/22] vfio iommu type1: Add task structure to vfio_dma |
| Message-ID | <sBhKV-3nj-3@gated-at.bofh.it> |
| In reply to | #1517230 |
On Tue, 8 Nov 2016 19:43:25 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:
> On 11/8/2016 2:33 AM, Alex Williamson wrote:
> > On Sat, 5 Nov 2016 02:40:43 +0530
> > Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >
>
> ...
>
> >> static int vfio_dma_do_map(struct vfio_iommu *iommu,
> >> struct vfio_iommu_type1_dma_map *map)
> >> {
> >> dma_addr_t iova = map->iova;
> >> unsigned long vaddr = map->vaddr;
> >> size_t size = map->size;
> >> - long npage;
> >> int ret = 0, prot = 0;
> >> uint64_t mask;
> >> struct vfio_dma *dma;
> >> - unsigned long pfn;
> >> + struct vfio_addr_space *addr_space;
> >> + struct mm_struct *mm;
> >> + bool free_addr_space_on_err = false;
> >>
> >> /* Verify that none of our __u64 fields overflow */
> >> if (map->size != size || map->vaddr != vaddr || map->iova != iova)
> >> @@ -608,47 +685,56 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
> >> mutex_lock(&iommu->lock);
> >>
> >> if (vfio_find_dma(iommu, iova, size)) {
> >> - mutex_unlock(&iommu->lock);
> >> - return -EEXIST;
> >> + ret = -EEXIST;
> >> + goto do_map_err;
> >> + }
> >> +
> >> + mm = get_task_mm(current);
> >> + if (!mm) {
> >> + ret = -ENODEV;
> >
> > -EFAULT?
> >
>
> -ENODEV return is in original code from vfio_pin_pages()
> if (!current->mm)
> return -ENODEV;
>
> Once I thought of changing it to -EFAULT, but then again changed to
> -ENODEV to be consistent with original error code.
>
> Should I still change this return to -EFAULT?
Let's keep ENODEV for less code churn, I guess.
> >> + goto do_map_err;
> >> + }
> >> +
> >> + addr_space = vfio_find_addr_space(iommu, mm);
> >> + if (addr_space) {
> >> + atomic_inc(&addr_space->ref_count);
> >> + mmput(mm);
> >> + } else {
> >> + addr_space = kzalloc(sizeof(*addr_space), GFP_KERNEL);
> >> + if (!addr_space) {
> >> + ret = -ENOMEM;
> >> + goto do_map_err;
> >> + }
> >> + addr_space->mm = mm;
> >> + atomic_set(&addr_space->ref_count, 1);
> >> + list_add(&addr_space->next, &iommu->addr_space_list);
> >> + free_addr_space_on_err = true;
> >> }
> >>
> >> dma = kzalloc(sizeof(*dma), GFP_KERNEL);
> >> if (!dma) {
> >> - mutex_unlock(&iommu->lock);
> >> - return -ENOMEM;
> >> + if (free_addr_space_on_err) {
> >> + mmput(mm);
> >> + list_del(&addr_space->next);
> >> + kfree(addr_space);
> >> + }
> >> + ret = -ENOMEM;
> >> + goto do_map_err;
> >> }
> >>
> >> dma->iova = iova;
> >> dma->vaddr = vaddr;
> >> dma->prot = prot;
> >> + dma->addr_space = addr_space;
> >> + get_task_struct(current);
> >> + dma->task = current;
> >> + dma->mlock_cap = capable(CAP_IPC_LOCK);
> >
> >
> > How do you reason we can cache this? Does the fact that the process
> > had this capability at the time that it did a DMA_MAP imply that it
> > necessarily still has this capability when an external user (vendor
> > driver) tries to pin pages? I don't see how we can make that
> > assumption.
> >
> >
>
> Will process change MEMLOCK limit at runtime? I think it shouldn't,
> correct me if I'm wrong. QEMU doesn't do that, right?
What QEMU does or doesn't do isn't relevant, the question is could a
process change CAP_IPC_LOCK runtime. It seems plausible to me.
> The function capable() determines current task's capability. But when
> vfio_pin_pages() is called, it could come from other task but pages are
> pinned from address space of task who mapped it. So we can't use
> capable() in vfio_pin_pages()
>
> If this capability shouldn't be cached, we have to use has_capability()
> with dma->task as argument in vfio_pin_pages()
>
> bool has_capability(struct task_struct *t, int cap)
Yep, that sounds better. Thanks,
Alex
[toc] | [prev] | [next] | [standalone]
| From | Kirti Wankhede <kwankhede@nvidia.com> |
|---|---|
| Date | 2016-11-04 22:20 +0100 |
| Subject | [PATCH v11 02/22] vfio: VFIO based driver for Mediated devices |
| Message-ID | <szU41-71A-17@gated-at.bofh.it> |
| In reply to | #1515412 |
vfio_mdev driver registers with mdev core driver.
mdev core driver creates mediated device and calls probe routine of
vfio_mdev driver for each device.
Probe routine of vfio_mdev driver adds mediated device to VFIO core module
This driver forms a shim layer that pass through VFIO devices operations
to vendor driver for mediated devices.
Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
Signed-off-by: Neo Jia <cjia@nvidia.com>
Change-Id: I583f4734752971d3d112324d69e2508c88f359ec
---
drivers/vfio/mdev/Kconfig | 9 ++-
drivers/vfio/mdev/Makefile | 1 +
drivers/vfio/mdev/vfio_mdev.c | 148 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 157 insertions(+), 1 deletion(-)
create mode 100644 drivers/vfio/mdev/vfio_mdev.c
diff --git a/drivers/vfio/mdev/Kconfig b/drivers/vfio/mdev/Kconfig
index 303c14ce2847..79c9cface7b1 100644
--- a/drivers/vfio/mdev/Kconfig
+++ b/drivers/vfio/mdev/Kconfig
@@ -5,6 +5,13 @@ config VFIO_MDEV
default n
help
Provides a framework to virtualize devices.
- See Documentation/vfio-mdev/vfio-mediated-device.txt for more details.
+ See Documentation/vfio-mediated-device.txt for more details.
If you don't know what do here, say N.
+
+config VFIO_MDEV_DEVICE
+ tristate "VFIO support for Mediated devices"
+ depends on VFIO && VFIO_MDEV
+ default n
+ help
+ VFIO based driver for mediated devices.
diff --git a/drivers/vfio/mdev/Makefile b/drivers/vfio/mdev/Makefile
index 31bc04801d94..fa2d5ea466ee 100644
--- a/drivers/vfio/mdev/Makefile
+++ b/drivers/vfio/mdev/Makefile
@@ -2,3 +2,4 @@
mdev-y := mdev_core.o mdev_sysfs.o mdev_driver.o
obj-$(CONFIG_VFIO_MDEV) += mdev.o
+obj-$(CONFIG_VFIO_MDEV_DEVICE) += vfio_mdev.o
diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c
new file mode 100644
index 000000000000..bb534d19e321
--- /dev/null
+++ b/drivers/vfio/mdev/vfio_mdev.c
@@ -0,0 +1,148 @@
+/*
+ * VFIO based driver for Mediated device
+ *
+ * Copyright (c) 2016, NVIDIA CORPORATION. All rights reserved.
+ * Author: Neo Jia <cjia@nvidia.com>
+ * Kirti Wankhede <kwankhede@nvidia.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/vfio.h>
+#include <linux/mdev.h>
+
+#include "mdev_private.h"
+
+#define DRIVER_VERSION "0.1"
+#define DRIVER_AUTHOR "NVIDIA Corporation"
+#define DRIVER_DESC "VFIO based driver for Mediated device"
+
+static int vfio_mdev_open(void *device_data)
+{
+ struct mdev_device *mdev = device_data;
+ struct parent_device *parent = mdev->parent;
+ int ret;
+
+ if (unlikely(!parent->ops->open))
+ return -EINVAL;
+
+ if (!try_module_get(THIS_MODULE))
+ return -ENODEV;
+
+ ret = parent->ops->open(mdev);
+ if (ret)
+ module_put(THIS_MODULE);
+
+ return ret;
+}
+
+static void vfio_mdev_release(void *device_data)
+{
+ struct mdev_device *mdev = device_data;
+ struct parent_device *parent = mdev->parent;
+
+ if (likely(parent->ops->release))
+ parent->ops->release(mdev);
+
+ module_put(THIS_MODULE);
+}
+
+static long vfio_mdev_unlocked_ioctl(void *device_data,
+ unsigned int cmd, unsigned long arg)
+{
+ struct mdev_device *mdev = device_data;
+ struct parent_device *parent = mdev->parent;
+
+ if (unlikely(!parent->ops->ioctl))
+ return -EINVAL;
+
+ return parent->ops->ioctl(mdev, cmd, arg);
+}
+
+static ssize_t vfio_mdev_read(void *device_data, char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ struct mdev_device *mdev = device_data;
+ struct parent_device *parent = mdev->parent;
+
+ if (unlikely(!parent->ops->read))
+ return -EINVAL;
+
+ return parent->ops->read(mdev, buf, count, ppos);
+}
+
+static ssize_t vfio_mdev_write(void *device_data, const char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ struct mdev_device *mdev = device_data;
+ struct parent_device *parent = mdev->parent;
+
+ if (unlikely(!parent->ops->write))
+ return -EINVAL;
+
+ return parent->ops->write(mdev, buf, count, ppos);
+}
+
+static int vfio_mdev_mmap(void *device_data, struct vm_area_struct *vma)
+{
+ struct mdev_device *mdev = device_data;
+ struct parent_device *parent = mdev->parent;
+
+ if (unlikely(!parent->ops->mmap))
+ return -EINVAL;
+
+ return parent->ops->mmap(mdev, vma);
+}
+
+static const struct vfio_device_ops vfio_mdev_dev_ops = {
+ .name = "vfio-mdev",
+ .open = vfio_mdev_open,
+ .release = vfio_mdev_release,
+ .ioctl = vfio_mdev_unlocked_ioctl,
+ .read = vfio_mdev_read,
+ .write = vfio_mdev_write,
+ .mmap = vfio_mdev_mmap,
+};
+
+int vfio_mdev_probe(struct device *dev)
+{
+ struct mdev_device *mdev = to_mdev_device(dev);
+
+ return vfio_add_group_dev(dev, &vfio_mdev_dev_ops, mdev);
+}
+
+void vfio_mdev_remove(struct device *dev)
+{
+ vfio_del_group_dev(dev);
+}
+
+struct mdev_driver vfio_mdev_driver = {
+ .name = "vfio_mdev",
+ .probe = vfio_mdev_probe,
+ .remove = vfio_mdev_remove,
+};
+
+static int __init vfio_mdev_init(void)
+{
+ return mdev_register_driver(&vfio_mdev_driver, THIS_MODULE);
+}
+
+static void __exit vfio_mdev_exit(void)
+{
+ mdev_unregister_driver(&vfio_mdev_driver);
+}
+
+module_init(vfio_mdev_init)
+module_exit(vfio_mdev_exit)
+
+MODULE_VERSION(DRIVER_VERSION);
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
--
2.7.0
[toc] | [prev] | [next] | [standalone]
| From | Kirti Wankhede <kwankhede@nvidia.com> |
|---|---|
| Date | 2016-11-04 22:20 +0100 |
| Subject | [PATCH v11 18/22] vfio: Define device_api strings |
| Message-ID | <szU42-71A-35@gated-at.bofh.it> |
| In reply to | #1515412 |
Defined device API strings. Vendor driver using mediated device
framework should use corresponding string for device_api attribute.
Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
Signed-off-by: Neo Jia <cjia@nvidia.com>
Change-Id: I42d29f475f02a7132ce13297fbf2b48f1da10995
---
include/uapi/linux/vfio.h | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 255a2113f53c..519eff362c1c 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -203,6 +203,16 @@ struct vfio_device_info {
};
#define VFIO_DEVICE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 7)
+/*
+ * Vendor driver using Mediated device framework should provide device_api
+ * attribute in supported type attribute groups. Device API string should be one
+ * of the following corresponding to device flags in vfio_device_info structure.
+ */
+
+#define VFIO_DEVICE_API_PCI_STRING "vfio-pci"
+#define VFIO_DEVICE_API_PLATFORM_STRING "vfio-platform"
+#define VFIO_DEVICE_API_AMBA_STRING "vfio-amba"
+
/**
* VFIO_DEVICE_GET_REGION_INFO - _IOWR(VFIO_TYPE, VFIO_BASE + 8,
* struct vfio_region_info)
--
2.7.0
[toc] | [prev] | [next] | [standalone]
| From | Kirti Wankhede <kwankhede@nvidia.com> |
|---|---|
| Date | 2016-11-04 22:20 +0100 |
| Subject | [PATCH v11 07/22] vfio iommu type1: Update argument of vaddr_get_pfn() |
| Message-ID | <szU42-71A-45@gated-at.bofh.it> |
| In reply to | #1515412 |
Update arguments of vaddr_get_pfn() to take struct mm_struct *mm as input
argument.
Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
Signed-off-by: Neo Jia <cjia@nvidia.com>
Change-Id: I885fd4cd4a9f66f4ee2c1caf58267464ec239f52
---
drivers/vfio/vfio_iommu_type1.c | 30 +++++++++++++++++++++++-------
1 file changed, 23 insertions(+), 7 deletions(-)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 02b302d0b7de..653386e80e85 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -226,20 +226,36 @@ static int put_pfn(unsigned long pfn, int prot)
return 0;
}
-static int vaddr_get_pfn(unsigned long vaddr, int prot, unsigned long *pfn)
+static int vaddr_get_pfn(struct mm_struct *mm, unsigned long vaddr,
+ int prot, unsigned long *pfn)
{
struct page *page[1];
struct vm_area_struct *vma;
int ret = -EFAULT;
- if (get_user_pages_fast(vaddr, 1, !!(prot & IOMMU_WRITE), page) == 1) {
+ if (mm == current->mm)
+ ret = get_user_pages_fast(vaddr, 1, !!(prot & IOMMU_WRITE),
+ page);
+ else {
+ unsigned int flags = 0;
+
+ if (prot & IOMMU_WRITE)
+ flags |= FOLL_WRITE;
+
+ down_read(&mm->mmap_sem);
+ ret = get_user_pages_remote(NULL, mm, vaddr, 1, flags, page,
+ NULL);
+ up_read(&mm->mmap_sem);
+ }
+
+ if (ret == 1) {
*pfn = page_to_pfn(page[0]);
return 0;
}
- down_read(¤t->mm->mmap_sem);
+ down_read(&mm->mmap_sem);
- vma = find_vma_intersection(current->mm, vaddr, vaddr + 1);
+ vma = find_vma_intersection(mm, vaddr, vaddr + 1);
if (vma && vma->vm_flags & VM_PFNMAP) {
*pfn = ((vaddr - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
@@ -247,7 +263,7 @@ static int vaddr_get_pfn(unsigned long vaddr, int prot, unsigned long *pfn)
ret = 0;
}
- up_read(¤t->mm->mmap_sem);
+ up_read(&mm->mmap_sem);
return ret;
}
@@ -268,7 +284,7 @@ static long __vfio_pin_pages_remote(unsigned long vaddr, long npage,
if (!current->mm)
return -ENODEV;
- ret = vaddr_get_pfn(vaddr, prot, pfn_base);
+ ret = vaddr_get_pfn(current->mm, vaddr, prot, pfn_base);
if (ret)
return ret;
@@ -291,7 +307,7 @@ static long __vfio_pin_pages_remote(unsigned long vaddr, long npage,
for (i = 1, vaddr += PAGE_SIZE; i < npage; i++, vaddr += PAGE_SIZE) {
unsigned long pfn = 0;
- ret = vaddr_get_pfn(vaddr, prot, &pfn);
+ ret = vaddr_get_pfn(current->mm, vaddr, prot, &pfn);
if (ret)
break;
--
2.7.0
[toc] | [prev] | [next] | [standalone]
| From | Alexey Kardashevskiy <aik@ozlabs.ru> |
|---|---|
| Date | 2016-11-07 09:50 +0100 |
| Subject | Re: [PATCH v11 07/22] vfio iommu type1: Update argument of vaddr_get_pfn() |
| Message-ID | <sANMS-wu-5@gated-at.bofh.it> |
| In reply to | #1515426 |
[Multipart message — attachments visible in raw view] — view raw
On 05/11/16 08:10, Kirti Wankhede wrote:
> Update arguments of vaddr_get_pfn() to take struct mm_struct *mm as input
> argument.
>
> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> Signed-off-by: Neo Jia <cjia@nvidia.com>
> Change-Id: I885fd4cd4a9f66f4ee2c1caf58267464ec239f52
> ---
> drivers/vfio/vfio_iommu_type1.c | 30 +++++++++++++++++++++++-------
> 1 file changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 02b302d0b7de..653386e80e85 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -226,20 +226,36 @@ static int put_pfn(unsigned long pfn, int prot)
> return 0;
> }
>
> -static int vaddr_get_pfn(unsigned long vaddr, int prot, unsigned long *pfn)
> +static int vaddr_get_pfn(struct mm_struct *mm, unsigned long vaddr,
> + int prot, unsigned long *pfn)
> {
> struct page *page[1];
> struct vm_area_struct *vma;
> int ret = -EFAULT;
>
> - if (get_user_pages_fast(vaddr, 1, !!(prot & IOMMU_WRITE), page) == 1) {
> + if (mm == current->mm)
afaik the rule is if one branch of "if" uses curly braces, the other should
do so too.
> + ret = get_user_pages_fast(vaddr, 1, !!(prot & IOMMU_WRITE),
> + page);
> + else {
> + unsigned int flags = 0;
> +
> + if (prot & IOMMU_WRITE)
> + flags |= FOLL_WRITE;
> +
> + down_read(&mm->mmap_sem);
> + ret = get_user_pages_remote(NULL, mm, vaddr, 1, flags, page,
> + NULL);
> + up_read(&mm->mmap_sem);
This chunk is not just about passing mm everywhere, it would be nice to see
in the commit log why this change is in this patch (may be it was commented
already, and I just missed it?).
> + }
> +
> + if (ret == 1) {
> *pfn = page_to_pfn(page[0]);
> return 0;
> }
>
> - down_read(¤t->mm->mmap_sem);
> + down_read(&mm->mmap_sem);
>
> - vma = find_vma_intersection(current->mm, vaddr, vaddr + 1);
> + vma = find_vma_intersection(mm, vaddr, vaddr + 1);
>
> if (vma && vma->vm_flags & VM_PFNMAP) {
> *pfn = ((vaddr - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
> @@ -247,7 +263,7 @@ static int vaddr_get_pfn(unsigned long vaddr, int prot, unsigned long *pfn)
> ret = 0;
> }
>
> - up_read(¤t->mm->mmap_sem);
> + up_read(&mm->mmap_sem);
>
> return ret;
> }
> @@ -268,7 +284,7 @@ static long __vfio_pin_pages_remote(unsigned long vaddr, long npage,
> if (!current->mm)
> return -ENODEV;
>
> - ret = vaddr_get_pfn(vaddr, prot, pfn_base);
> + ret = vaddr_get_pfn(current->mm, vaddr, prot, pfn_base);
> if (ret)
> return ret;
>
> @@ -291,7 +307,7 @@ static long __vfio_pin_pages_remote(unsigned long vaddr, long npage,
> for (i = 1, vaddr += PAGE_SIZE; i < npage; i++, vaddr += PAGE_SIZE) {
> unsigned long pfn = 0;
>
> - ret = vaddr_get_pfn(vaddr, prot, &pfn);
> + ret = vaddr_get_pfn(current->mm, vaddr, prot, &pfn);
> if (ret)
> break;
>
>
--
Alexey
[toc] | [prev] | [next] | [standalone]
| From | Kirti Wankhede <kwankhede@nvidia.com> |
|---|---|
| Date | 2016-11-04 22:20 +0100 |
| Subject | [PATCH v11 15/22] vfio: Introduce vfio_set_irqs_validate_and_prepare() |
| Message-ID | <szU42-71A-43@gated-at.bofh.it> |
| In reply to | #1515412 |
Vendor driver using mediated device framework would use same mechnism to
validate and prepare IRQs. Introducing this function to reduce code
replication in multiple drivers.
Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
Signed-off-by: Neo Jia <cjia@nvidia.com>
Change-Id: Ie201f269dda0713ca18a07dc4852500bd8b48309
---
drivers/vfio/vfio.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/vfio.h | 4 ++++
2 files changed, 52 insertions(+)
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 9a03be0942a1..ed2361e4b904 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -1858,6 +1858,54 @@ int vfio_info_add_capability(struct vfio_info_cap *caps, int cap_type_id,
}
EXPORT_SYMBOL(vfio_info_add_capability);
+int vfio_set_irqs_validate_and_prepare(struct vfio_irq_set *hdr, int num_irqs,
+ int max_irq_type, size_t *data_size)
+{
+ unsigned long minsz;
+ size_t size;
+
+ minsz = offsetofend(struct vfio_irq_set, count);
+
+ if ((hdr->argsz < minsz) || (hdr->index >= max_irq_type) ||
+ (hdr->count >= (U32_MAX - hdr->start)) ||
+ (hdr->flags & ~(VFIO_IRQ_SET_DATA_TYPE_MASK |
+ VFIO_IRQ_SET_ACTION_TYPE_MASK)))
+ return -EINVAL;
+
+ if (data_size)
+ *data_size = 0;
+
+ if (hdr->start >= num_irqs || hdr->start + hdr->count > num_irqs)
+ return -EINVAL;
+
+ switch (hdr->flags & VFIO_IRQ_SET_DATA_TYPE_MASK) {
+ case VFIO_IRQ_SET_DATA_NONE:
+ size = 0;
+ break;
+ case VFIO_IRQ_SET_DATA_BOOL:
+ size = sizeof(uint8_t);
+ break;
+ case VFIO_IRQ_SET_DATA_EVENTFD:
+ size = sizeof(int32_t);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (size) {
+ if (hdr->argsz - minsz < hdr->count * size)
+ return -EINVAL;
+
+ if (!data_size)
+ return -EINVAL;
+
+ *data_size = hdr->count * size;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL(vfio_set_irqs_validate_and_prepare);
+
/*
* Pin a set of guest PFNs and return their associated host PFNs for local
* domain only.
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index cf90393a11e2..87c9afecd822 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -116,6 +116,10 @@ extern void vfio_info_cap_shift(struct vfio_info_cap *caps, size_t offset);
extern int vfio_info_add_capability(struct vfio_info_cap *caps,
int cap_type_id, void *cap_type);
+extern int vfio_set_irqs_validate_and_prepare(struct vfio_irq_set *hdr,
+ int num_irqs, int max_irq_type,
+ size_t *data_size);
+
struct pci_dev;
#ifdef CONFIG_EEH
extern void vfio_spapr_pci_eeh_open(struct pci_dev *pdev);
--
2.7.0
[toc] | [prev] | [next] | [standalone]
| From | Alexey Kardashevskiy <aik@ozlabs.ru> |
|---|---|
| Date | 2016-11-08 09:50 +0100 |
| Subject | Re: [PATCH v11 15/22] vfio: Introduce vfio_set_irqs_validate_and_prepare() |
| Message-ID | <sBagp-739-15@gated-at.bofh.it> |
| In reply to | #1515427 |
On 05/11/16 08:10, Kirti Wankhede wrote:
> Vendor driver using mediated device framework would use same mechnism to
> validate and prepare IRQs. Introducing this function to reduce code
> replication in multiple drivers.
>
> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> Signed-off-by: Neo Jia <cjia@nvidia.com>
> Change-Id: Ie201f269dda0713ca18a07dc4852500bd8b48309
> ---
> drivers/vfio/vfio.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/vfio.h | 4 ++++
> 2 files changed, 52 insertions(+)
>
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 9a03be0942a1..ed2361e4b904 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -1858,6 +1858,54 @@ int vfio_info_add_capability(struct vfio_info_cap *caps, int cap_type_id,
> }
> EXPORT_SYMBOL(vfio_info_add_capability);
>
> +int vfio_set_irqs_validate_and_prepare(struct vfio_irq_set *hdr, int num_irqs,
> + int max_irq_type, size_t *data_size)
> +{
> + unsigned long minsz;
> + size_t size;
> +
> + minsz = offsetofend(struct vfio_irq_set, count);
> +
> + if ((hdr->argsz < minsz) || (hdr->index >= max_irq_type) ||
> + (hdr->count >= (U32_MAX - hdr->start)) ||
> + (hdr->flags & ~(VFIO_IRQ_SET_DATA_TYPE_MASK |
> + VFIO_IRQ_SET_ACTION_TYPE_MASK)))
> + return -EINVAL;
> +
> + if (data_size)
Pointless check, the callers will pass non null pointer with value
initialized to 0 anyway.
> + *data_size = 0;
> +
> + if (hdr->start >= num_irqs || hdr->start + hdr->count > num_irqs)
> + return -EINVAL;
> +
> + switch (hdr->flags & VFIO_IRQ_SET_DATA_TYPE_MASK) {
> + case VFIO_IRQ_SET_DATA_NONE:
> + size = 0;
> + break;
> + case VFIO_IRQ_SET_DATA_BOOL:
> + size = sizeof(uint8_t);
> + break;
> + case VFIO_IRQ_SET_DATA_EVENTFD:
> + size = sizeof(int32_t);
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + if (size) {
The whole branch would even work for size == 0.
> + if (hdr->argsz - minsz < hdr->count * size)
> + return -EINVAL;
> +
> + if (!data_size)
> + return -EINVAL;
Redundant check as well.
> +
> + *data_size = hdr->count * size;
> + }
> +
> + return 0;
> +}
It does not really prepare anything as the name suggests. It looks like
this is 2 different helpers actually:
int vfio_set_irqs_validate()
and
size_t vfio_set_irqs_hdr_to_data_size()
And it would make it easier to review/bisect if 16/22 and 17/22 were merged
into this one as this patch alone adds new code which it does not use and
all 3 patches are fairly small.
> +EXPORT_SYMBOL(vfio_set_irqs_validate_and_prepare);
Everything you export in this patchset is EXPORT_SYMBOL() while the
existing code uses EXPORT_SYMBOL_GPL(), is this for a reason?
> +
> /*
> * Pin a set of guest PFNs and return their associated host PFNs for local
> * domain only.
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index cf90393a11e2..87c9afecd822 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -116,6 +116,10 @@ extern void vfio_info_cap_shift(struct vfio_info_cap *caps, size_t offset);
> extern int vfio_info_add_capability(struct vfio_info_cap *caps,
> int cap_type_id, void *cap_type);
>
> +extern int vfio_set_irqs_validate_and_prepare(struct vfio_irq_set *hdr,
> + int num_irqs, int max_irq_type,
> + size_t *data_size);
> +
> struct pci_dev;
> #ifdef CONFIG_EEH
> extern void vfio_spapr_pci_eeh_open(struct pci_dev *pdev);
>
--
Alexey
[toc] | [prev] | [next] | [standalone]
| From | Kirti Wankhede <kwankhede@nvidia.com> |
|---|---|
| Date | 2016-11-08 21:30 +0100 |
| Subject | Re: [PATCH v11 15/22] vfio: Introduce vfio_set_irqs_validate_and_prepare() |
| Message-ID | <sBlbP-5QB-21@gated-at.bofh.it> |
| In reply to | #1516950 |
On 11/8/2016 2:16 PM, Alexey Kardashevskiy wrote:
> On 05/11/16 08:10, Kirti Wankhede wrote:
>> Vendor driver using mediated device framework would use same mechnism to
>> validate and prepare IRQs. Introducing this function to reduce code
>> replication in multiple drivers.
>>
>> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
>> Signed-off-by: Neo Jia <cjia@nvidia.com>
>> Change-Id: Ie201f269dda0713ca18a07dc4852500bd8b48309
>> ---
>> drivers/vfio/vfio.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/vfio.h | 4 ++++
>> 2 files changed, 52 insertions(+)
>>
>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
>> index 9a03be0942a1..ed2361e4b904 100644
>> --- a/drivers/vfio/vfio.c
>> +++ b/drivers/vfio/vfio.c
>> @@ -1858,6 +1858,54 @@ int vfio_info_add_capability(struct vfio_info_cap *caps, int cap_type_id,
>> }
>> EXPORT_SYMBOL(vfio_info_add_capability);
>>
>> +int vfio_set_irqs_validate_and_prepare(struct vfio_irq_set *hdr, int num_irqs,
>> + int max_irq_type, size_t *data_size)
>> +{
>> + unsigned long minsz;
>> + size_t size;
>> +
>> + minsz = offsetofend(struct vfio_irq_set, count);
>> +
>> + if ((hdr->argsz < minsz) || (hdr->index >= max_irq_type) ||
>> + (hdr->count >= (U32_MAX - hdr->start)) ||
>> + (hdr->flags & ~(VFIO_IRQ_SET_DATA_TYPE_MASK |
>> + VFIO_IRQ_SET_ACTION_TYPE_MASK)))
>> + return -EINVAL;
>> +
>> + if (data_size)
>
> Pointless check, the callers will pass non null pointer with value
> initialized to 0 anyway.
>
Not always, When VFIO_IRQ_SET_DATA_NONE flag is set, caller can pass
data_size = NULL.
>
>> + *data_size = 0;
>> +
>> + if (hdr->start >= num_irqs || hdr->start + hdr->count > num_irqs)
>> + return -EINVAL;
>> +
>> + switch (hdr->flags & VFIO_IRQ_SET_DATA_TYPE_MASK) {
>> + case VFIO_IRQ_SET_DATA_NONE:
>> + size = 0;
>> + break;
>> + case VFIO_IRQ_SET_DATA_BOOL:
>> + size = sizeof(uint8_t);
>> + break;
>> + case VFIO_IRQ_SET_DATA_EVENTFD:
>> + size = sizeof(int32_t);
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + if (size) {
>
> The whole branch would even work for size == 0.
>
In that case below check (!data_size) might result in error if data_size
== NULL, whereas its not error case when size == 0, i.e.
VFIO_IRQ_SET_DATA_NONE flag set.
>> + if (hdr->argsz - minsz < hdr->count * size)
>> + return -EINVAL;
>> +
>> + if (!data_size)
>> + return -EINVAL;
>
> Redundant check as well.
>
This is not redundant. If you see above check, it sets its init value to
0 but doesn't fail.
>> +
>> + *data_size = hdr->count * size;
>> + }
>> +
>> + return 0;
>> +}
>
> It does not really prepare anything as the name suggests. It looks like
> this is 2 different helpers actually:
>
> int vfio_set_irqs_validate()
> and
> size_t vfio_set_irqs_hdr_to_data_size()
>
Later one is the prepare.
>
> And it would make it easier to review/bisect if 16/22 and 17/22 were merged
> into this one as this patch alone adds new code which it does not use and
> all 3 patches are fairly small.
>
I do had all 3 patch merged in one in earlier version of patchset. This
is split as per Alex's suggestion.
>
>> +EXPORT_SYMBOL(vfio_set_irqs_validate_and_prepare);
>
> Everything you export in this patchset is EXPORT_SYMBOL() while the
> existing code uses EXPORT_SYMBOL_GPL(), is this for a reason?
>
>
We want these symbols to be available to all drivers.
Thanks,
Kirti
>> +
>> /*
>> * Pin a set of guest PFNs and return their associated host PFNs for local
>> * domain only.
>> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
>> index cf90393a11e2..87c9afecd822 100644
>> --- a/include/linux/vfio.h
>> +++ b/include/linux/vfio.h
>> @@ -116,6 +116,10 @@ extern void vfio_info_cap_shift(struct vfio_info_cap *caps, size_t offset);
>> extern int vfio_info_add_capability(struct vfio_info_cap *caps,
>> int cap_type_id, void *cap_type);
>>
>> +extern int vfio_set_irqs_validate_and_prepare(struct vfio_irq_set *hdr,
>> + int num_irqs, int max_irq_type,
>> + size_t *data_size);
>> +
>> struct pci_dev;
>> #ifdef CONFIG_EEH
>> extern void vfio_spapr_pci_eeh_open(struct pci_dev *pdev);
>>
>
>
[toc] | [prev] | [next] | [standalone]
| From | Alexey Kardashevskiy <aik@ozlabs.ru> |
|---|---|
| Date | 2016-11-09 04:10 +0100 |
| Subject | Re: [PATCH v11 15/22] vfio: Introduce vfio_set_irqs_validate_and_prepare() |
| Message-ID | <sBrqW-1BM-3@gated-at.bofh.it> |
| In reply to | #1517557 |
On 09/11/16 07:22, Kirti Wankhede wrote:
>
>
> On 11/8/2016 2:16 PM, Alexey Kardashevskiy wrote:
>> On 05/11/16 08:10, Kirti Wankhede wrote:
>>> Vendor driver using mediated device framework would use same mechnism to
>>> validate and prepare IRQs. Introducing this function to reduce code
>>> replication in multiple drivers.
>>>
>>> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
>>> Signed-off-by: Neo Jia <cjia@nvidia.com>
>>> Change-Id: Ie201f269dda0713ca18a07dc4852500bd8b48309
>>> ---
>>> drivers/vfio/vfio.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>>> include/linux/vfio.h | 4 ++++
>>> 2 files changed, 52 insertions(+)
>>>
>>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
>>> index 9a03be0942a1..ed2361e4b904 100644
>>> --- a/drivers/vfio/vfio.c
>>> +++ b/drivers/vfio/vfio.c
>>> @@ -1858,6 +1858,54 @@ int vfio_info_add_capability(struct vfio_info_cap *caps, int cap_type_id,
>>> }
>>> EXPORT_SYMBOL(vfio_info_add_capability);
>>>
>>> +int vfio_set_irqs_validate_and_prepare(struct vfio_irq_set *hdr, int num_irqs,
>>> + int max_irq_type, size_t *data_size)
>>> +{
>>> + unsigned long minsz;
>>> + size_t size;
>>> +
>>> + minsz = offsetofend(struct vfio_irq_set, count);
>>> +
>>> + if ((hdr->argsz < minsz) || (hdr->index >= max_irq_type) ||
>>> + (hdr->count >= (U32_MAX - hdr->start)) ||
>>> + (hdr->flags & ~(VFIO_IRQ_SET_DATA_TYPE_MASK |
>>> + VFIO_IRQ_SET_ACTION_TYPE_MASK)))
>>> + return -EINVAL;
>>> +
>>> + if (data_size)
>>
>> Pointless check, the callers will pass non null pointer with value
>> initialized to 0 anyway.
>>
>
> Not always, When VFIO_IRQ_SET_DATA_NONE flag is set, caller can pass
> data_size = NULL.
Today data_size is not NULL in all cases and the way it is used now (ioctl
VFIO_DEVICE_SET_IRQS) gives me an idea that this is not going to change.
>
>>
>>> + *data_size = 0;
>>> +
>>> + if (hdr->start >= num_irqs || hdr->start + hdr->count > num_irqs)
>>> + return -EINVAL;
>>> +
>>> + switch (hdr->flags & VFIO_IRQ_SET_DATA_TYPE_MASK) {
>>> + case VFIO_IRQ_SET_DATA_NONE:
>>> + size = 0;
>>> + break;
>>> + case VFIO_IRQ_SET_DATA_BOOL:
>>> + size = sizeof(uint8_t);
>>> + break;
>>> + case VFIO_IRQ_SET_DATA_EVENTFD:
>>> + size = sizeof(int32_t);
>>> + break;
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +
>>> + if (size) {
>>
>> The whole branch would even work for size == 0.
>>
>
> In that case below check (!data_size) might result in error if data_size
> == NULL, whereas its not error case when size == 0, i.e.
> VFIO_IRQ_SET_DATA_NONE flag set.
>
>>> + if (hdr->argsz - minsz < hdr->count * size)
>>> + return -EINVAL;
>>> +
>>> + if (!data_size)
>>> + return -EINVAL;
>>
>> Redundant check as well.
>>
>
> This is not redundant. If you see above check, it sets its init value to
> 0 but doesn't fail.
>
>>> +
>>> + *data_size = hdr->count * size;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>
>> It does not really prepare anything as the name suggests. It looks like
>> this is 2 different helpers actually:
>>
>> int vfio_set_irqs_validate()
>> and
>> size_t vfio_set_irqs_hdr_to_data_size()
>>
>
> Later one is the prepare.
Does not like it prepares anything, just a simple converter.
>> And it would make it easier to review/bisect if 16/22 and 17/22 were merged
>> into this one as this patch alone adds new code which it does not use and
>> all 3 patches are fairly small.
>>
>
> I do had all 3 patch merged in one in earlier version of patchset. This
> is split as per Alex's suggestion.
I got this from another mail from Alex. Which I find strange but whatever,
this is his realm anyway :)
>
>>
>>> +EXPORT_SYMBOL(vfio_set_irqs_validate_and_prepare);
>>
>> Everything you export in this patchset is EXPORT_SYMBOL() while the
>> existing code uses EXPORT_SYMBOL_GPL(), is this for a reason?
>>
>>
>
> We want these symbols to be available to all drivers.
Right, got it from another mail from Alex as well. Ok, seems all right so
far. A note in the commit log would be useful though.
>
> Thanks,
> Kirti
>
>>> +
>>> /*
>>> * Pin a set of guest PFNs and return their associated host PFNs for local
>>> * domain only.
>>> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
>>> index cf90393a11e2..87c9afecd822 100644
>>> --- a/include/linux/vfio.h
>>> +++ b/include/linux/vfio.h
>>> @@ -116,6 +116,10 @@ extern void vfio_info_cap_shift(struct vfio_info_cap *caps, size_t offset);
>>> extern int vfio_info_add_capability(struct vfio_info_cap *caps,
>>> int cap_type_id, void *cap_type);
>>>
>>> +extern int vfio_set_irqs_validate_and_prepare(struct vfio_irq_set *hdr,
>>> + int num_irqs, int max_irq_type,
>>> + size_t *data_size);
>>> +
>>> struct pci_dev;
>>> #ifdef CONFIG_EEH
>>> extern void vfio_spapr_pci_eeh_open(struct pci_dev *pdev);
>>>
>>
>>
--
Alexey
[toc] | [prev] | [next] | [standalone]
| From | Alex Williamson <alex.williamson@redhat.com> |
|---|---|
| Date | 2016-11-09 04:40 +0100 |
| Subject | Re: [PATCH v11 15/22] vfio: Introduce vfio_set_irqs_validate_and_prepare() |
| Message-ID | <sBrTX-1PK-13@gated-at.bofh.it> |
| In reply to | #1517745 |
On Wed, 9 Nov 2016 14:07:58 +1100
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> On 09/11/16 07:22, Kirti Wankhede wrote:
> > On 11/8/2016 2:16 PM, Alexey Kardashevskiy wrote:
> >> On 05/11/16 08:10, Kirti Wankhede wrote:
> >>> Vendor driver using mediated device framework would use same mechnism to
> >>> validate and prepare IRQs. Introducing this function to reduce code
> >>> replication in multiple drivers.
> >>>
> >>> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> >>> Signed-off-by: Neo Jia <cjia@nvidia.com>
> >>> Change-Id: Ie201f269dda0713ca18a07dc4852500bd8b48309
> >>> ---
> >>> drivers/vfio/vfio.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>> include/linux/vfio.h | 4 ++++
> >>> 2 files changed, 52 insertions(+)
> >>>
> >>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> >>> index 9a03be0942a1..ed2361e4b904 100644
> >>> --- a/drivers/vfio/vfio.c
> >>> +++ b/drivers/vfio/vfio.c
> >>> @@ -1858,6 +1858,54 @@ int vfio_info_add_capability(struct vfio_info_cap *caps, int cap_type_id,
> >>> }
> >>> EXPORT_SYMBOL(vfio_info_add_capability);
> >>>
> >>> +int vfio_set_irqs_validate_and_prepare(struct vfio_irq_set *hdr, int num_irqs,
> >>> + int max_irq_type, size_t *data_size)
> >>> +{
> >>> + unsigned long minsz;
> >>> + size_t size;
> >>> +
> >>> + minsz = offsetofend(struct vfio_irq_set, count);
> >>> +
> >>> + if ((hdr->argsz < minsz) || (hdr->index >= max_irq_type) ||
> >>> + (hdr->count >= (U32_MAX - hdr->start)) ||
> >>> + (hdr->flags & ~(VFIO_IRQ_SET_DATA_TYPE_MASK |
> >>> + VFIO_IRQ_SET_ACTION_TYPE_MASK)))
> >>> + return -EINVAL;
> >>> +
> >>> + if (data_size)
> >>
> >> Pointless check, the callers will pass non null pointer with value
> >> initialized to 0 anyway.
> >>
> >
> > Not always, When VFIO_IRQ_SET_DATA_NONE flag is set, caller can pass
> > data_size = NULL.
>
>
> Today data_size is not NULL in all cases and the way it is used now (ioctl
> VFIO_DEVICE_SET_IRQS) gives me an idea that this is not going to change.
>
> >
> >>
> >>> + *data_size = 0;
> >>> +
> >>> + if (hdr->start >= num_irqs || hdr->start + hdr->count > num_irqs)
> >>> + return -EINVAL;
> >>> +
> >>> + switch (hdr->flags & VFIO_IRQ_SET_DATA_TYPE_MASK) {
> >>> + case VFIO_IRQ_SET_DATA_NONE:
> >>> + size = 0;
> >>> + break;
> >>> + case VFIO_IRQ_SET_DATA_BOOL:
> >>> + size = sizeof(uint8_t);
> >>> + break;
> >>> + case VFIO_IRQ_SET_DATA_EVENTFD:
> >>> + size = sizeof(int32_t);
> >>> + break;
> >>> + default:
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>> + if (size) {
> >>
> >> The whole branch would even work for size == 0.
> >>
> >
> > In that case below check (!data_size) might result in error if data_size
> > == NULL, whereas its not error case when size == 0, i.e.
> > VFIO_IRQ_SET_DATA_NONE flag set.
> >
> >>> + if (hdr->argsz - minsz < hdr->count * size)
> >>> + return -EINVAL;
> >>> +
> >>> + if (!data_size)
> >>> + return -EINVAL;
> >>
> >> Redundant check as well.
> >>
> >
> > This is not redundant. If you see above check, it sets its init value to
> > 0 but doesn't fail.
> >
> >>> +
> >>> + *data_size = hdr->count * size;
> >>> + }
> >>> +
> >>> + return 0;
> >>> +}
> >>
> >> It does not really prepare anything as the name suggests. It looks like
> >> this is 2 different helpers actually:
> >>
> >> int vfio_set_irqs_validate()
> >> and
> >> size_t vfio_set_irqs_hdr_to_data_size()
> >>
> >
> > Later one is the prepare.
>
>
> Does not like it prepares anything, just a simple converter.
>
>
> >> And it would make it easier to review/bisect if 16/22 and 17/22 were merged
> >> into this one as this patch alone adds new code which it does not use and
> >> all 3 patches are fairly small.
> >>
> >
> > I do had all 3 patch merged in one in earlier version of patchset. This
> > is split as per Alex's suggestion.
>
> I got this from another mail from Alex. Which I find strange but whatever,
> this is his realm anyway :)
Maybe you haven't noticed, but your patch series are often difficult to
deal with, they almost always split across functional areas and
maintainers. Splitting out code to common functions and _then_
updating the callers to make use of it is a common way to deal with
that. We're in the same functional area here, but it's still
good practice. Thanks,
Alex
[toc] | [prev] | [next] | [standalone]
| From | Kirti Wankhede <kwankhede@nvidia.com> |
|---|---|
| Date | 2016-11-04 22:20 +0100 |
| Subject | [PATCH v11 10/22] vfio iommu type1: Add support for mediated devices |
| Message-ID | <szU42-71A-47@gated-at.bofh.it> |
| In reply to | #1515412 |
VFIO IOMMU drivers are designed for the devices which are IOMMU capable.
Mediated device only uses IOMMU APIs, the underlying hardware can be
managed by an IOMMU domain.
Aim of this change is:
- To use most of the code of TYPE1 IOMMU driver for mediated devices
- To support direct assigned device and mediated device in single module
This change adds pin and unpin support for mediated device to TYPE1 IOMMU
backend module. More details:
- vfio_pin_pages() callback here uses task and address space of vfio_dma,
that is, of the process who mapped that iova range.
- Added pfn_list tracking logic to address space structure. All pages
pinned through this interface are trached in its address space.
- Pinned pages list is used to verify unpinning request and to unpin
remaining pages while detaching the group for that device.
- Page accounting is updated to account in its address space where the
pages are pinned/unpinned.
- Accouting for mdev device is only done if there is no iommu capable
domain in the container. When there is a direct device assigned to the
container and that domain is iommu capable, all pages are already pinned
during DMA_MAP.
- Page accouting is updated on hot plug and unplug mdev device and pass
through device.
Tested by assigning below combinations of devices to a single VM:
- GPU pass through only
- vGPU device only
- One GPU pass through and one vGPU device
- Linux VM hot plug and unplug vGPU device while GPU pass through device
exist
- Linux VM hot plug and unplug GPU pass through device while vGPU device
exist
Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
Signed-off-by: Neo Jia <cjia@nvidia.com>
Change-Id: I295d6f0f2e0579b8d9882bfd8fd5a4194b97bd9a
---
drivers/vfio/vfio_iommu_type1.c | 538 +++++++++++++++++++++++++++++++++++++---
1 file changed, 500 insertions(+), 38 deletions(-)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 8d64528dcc22..e511073446a0 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -36,6 +36,7 @@
#include <linux/uaccess.h>
#include <linux/vfio.h>
#include <linux/workqueue.h>
+#include <linux/mdev.h>
#define DRIVER_VERSION "0.2"
#define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>"
@@ -56,6 +57,7 @@ MODULE_PARM_DESC(disable_hugepages,
struct vfio_iommu {
struct list_head domain_list;
struct list_head addr_space_list;
+ struct vfio_domain *external_domain; /* domain for external user */
struct mutex lock;
struct rb_root dma_list;
bool v2;
@@ -67,6 +69,9 @@ struct vfio_addr_space {
struct mm_struct *mm;
struct list_head next;
atomic_t ref_count;
+ /* external user pinned pfns */
+ struct rb_root pfn_list; /* pinned Host pfn list */
+ struct mutex pfn_list_lock; /* mutex for pfn_list */
};
struct vfio_domain {
@@ -83,6 +88,7 @@ struct vfio_dma {
unsigned long vaddr; /* Process virtual addr */
size_t size; /* Map size (bytes) */
int prot; /* IOMMU_READ/WRITE */
+ bool iommu_mapped;
struct vfio_addr_space *addr_space;
struct task_struct *task;
bool mlock_cap;
@@ -94,6 +100,19 @@ struct vfio_group {
};
/*
+ * Guest RAM pinning working set or DMA target
+ */
+struct vfio_pfn {
+ struct rb_node node;
+ unsigned long pfn; /* Host pfn */
+ int prot;
+ atomic_t ref_count;
+};
+
+#define IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu) \
+ (!list_empty(&iommu->domain_list))
+
+/*
* This code handles mapping and unmapping of user data buffers
* into DMA'ble space using the IOMMU
*/
@@ -153,6 +172,93 @@ static struct vfio_addr_space *vfio_find_addr_space(struct vfio_iommu *iommu,
return NULL;
}
+/*
+ * Helper Functions for host pfn list
+ */
+static struct vfio_pfn *vfio_find_pfn(struct vfio_addr_space *addr_space,
+ unsigned long pfn)
+{
+ struct vfio_pfn *vpfn;
+ struct rb_node *node = addr_space->pfn_list.rb_node;
+
+ while (node) {
+ vpfn = rb_entry(node, struct vfio_pfn, node);
+
+ if (pfn < vpfn->pfn)
+ node = node->rb_left;
+ else if (pfn > vpfn->pfn)
+ node = node->rb_right;
+ else
+ return vpfn;
+ }
+
+ return NULL;
+}
+
+static void vfio_link_pfn(struct vfio_addr_space *addr_space,
+ struct vfio_pfn *new)
+{
+ struct rb_node **link, *parent = NULL;
+ struct vfio_pfn *vpfn;
+
+ link = &addr_space->pfn_list.rb_node;
+ while (*link) {
+ parent = *link;
+ vpfn = rb_entry(parent, struct vfio_pfn, node);
+
+ if (new->pfn < vpfn->pfn)
+ link = &(*link)->rb_left;
+ else
+ link = &(*link)->rb_right;
+ }
+
+ rb_link_node(&new->node, parent, link);
+ rb_insert_color(&new->node, &addr_space->pfn_list);
+}
+
+static void vfio_unlink_pfn(struct vfio_addr_space *addr_space,
+ struct vfio_pfn *old)
+{
+ rb_erase(&old->node, &addr_space->pfn_list);
+}
+
+static int vfio_add_to_pfn_list(struct vfio_addr_space *addr_space,
+ unsigned long pfn, int prot)
+{
+ struct vfio_pfn *vpfn;
+
+ vpfn = kzalloc(sizeof(*vpfn), GFP_KERNEL);
+ if (!vpfn)
+ return -ENOMEM;
+
+ vpfn->pfn = pfn;
+ vpfn->prot = prot;
+ atomic_set(&vpfn->ref_count, 1);
+ vfio_link_pfn(addr_space, vpfn);
+ return 0;
+}
+
+static void vfio_remove_from_pfn_list(struct vfio_addr_space *addr_space,
+ struct vfio_pfn *vpfn)
+{
+ vfio_unlink_pfn(addr_space, vpfn);
+ kfree(vpfn);
+}
+
+static int vfio_pfn_account(struct vfio_addr_space *addr_space,
+ unsigned long pfn)
+{
+ struct vfio_pfn *p;
+ int ret = 1;
+
+ mutex_lock(&addr_space->pfn_list_lock);
+ p = vfio_find_pfn(addr_space, pfn);
+ if (p)
+ ret = 0;
+ mutex_unlock(&addr_space->pfn_list_lock);
+ return ret;
+}
+
struct vwork {
struct mm_struct *mm;
long npage;
@@ -304,16 +410,18 @@ static long __vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr,
unsigned long limit = task_rlimit(task, RLIMIT_MEMLOCK) >> PAGE_SHIFT;
bool lock_cap = dma->mlock_cap;
struct mm_struct *mm = dma->addr_space->mm;
- long ret, i;
+ long ret, i, lock_acct;
bool rsvd;
ret = vaddr_get_pfn(mm, vaddr, prot, pfn_base);
if (ret)
return ret;
+ lock_acct = vfio_pfn_account(dma->addr_space, *pfn_base);
+
rsvd = is_invalid_reserved_pfn(*pfn_base);
- if (!rsvd && !lock_cap && mm->locked_vm + 1 > limit) {
+ if (!rsvd && !lock_cap && mm->locked_vm + lock_acct > limit) {
put_pfn(*pfn_base, prot);
pr_warn("%s: RLIMIT_MEMLOCK (%ld) exceeded\n", __func__,
limit << PAGE_SHIFT);
@@ -340,8 +448,10 @@ static long __vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr,
break;
}
+ lock_acct += vfio_pfn_account(dma->addr_space, pfn);
+
if (!rsvd && !lock_cap &&
- mm->locked_vm + i + 1 > limit) {
+ mm->locked_vm + lock_acct + 1 > limit) {
put_pfn(pfn, prot);
pr_warn("%s: RLIMIT_MEMLOCK (%ld) exceeded\n",
__func__, limit << PAGE_SHIFT);
@@ -350,7 +460,7 @@ static long __vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr,
}
if (!rsvd)
- vfio_lock_acct(mm, i);
+ vfio_lock_acct(mm, lock_acct);
return i;
}
@@ -370,14 +480,214 @@ static long __vfio_unpin_pages_remote(struct vfio_dma *dma, unsigned long pfn,
return unlocked;
}
-static void vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma)
+static int __vfio_pin_page_external(struct vfio_dma *dma, unsigned long vaddr,
+ int prot, unsigned long *pfn_base,
+ bool do_accounting)
+{
+ struct task_struct *task = dma->task;
+ unsigned long limit = task_rlimit(task, RLIMIT_MEMLOCK) >> PAGE_SHIFT;
+ bool lock_cap = dma->mlock_cap;
+ struct mm_struct *mm = dma->addr_space->mm;
+ int ret;
+ bool rsvd;
+
+ ret = vaddr_get_pfn(mm, vaddr, prot, pfn_base);
+ if (ret)
+ return ret;
+
+ rsvd = is_invalid_reserved_pfn(*pfn_base);
+
+ if (!rsvd && !lock_cap && mm->locked_vm + 1 > limit) {
+ put_pfn(*pfn_base, prot);
+ pr_warn("%s: Task %s (%d) RLIMIT_MEMLOCK (%ld) exceeded\n",
+ __func__, task->comm, task_pid_nr(task),
+ limit << PAGE_SHIFT);
+ return -ENOMEM;
+ }
+
+ if (!rsvd && do_accounting)
+ vfio_lock_acct(mm, 1);
+
+ return 1;
+}
+
+static void __vfio_unpin_page_external(struct vfio_addr_space *addr_space,
+ unsigned long pfn, int prot,
+ bool do_accounting)
+{
+ put_pfn(pfn, prot);
+
+ if (do_accounting)
+ vfio_lock_acct(addr_space->mm, -1);
+}
+
+static int vfio_unpin_pfn(struct vfio_addr_space *addr_space,
+ struct vfio_pfn *vpfn, bool do_accounting)
+{
+ __vfio_unpin_page_external(addr_space, vpfn->pfn, vpfn->prot,
+ do_accounting);
+
+ if (atomic_dec_and_test(&vpfn->ref_count))
+ vfio_remove_from_pfn_list(addr_space, vpfn);
+
+ return 1;
+}
+
+static int vfio_iommu_type1_pin_pages(void *iommu_data,
+ unsigned long *user_pfn,
+ int npage, int prot,
+ unsigned long *phys_pfn)
+{
+ struct vfio_iommu *iommu = iommu_data;
+ int i, j, ret;
+ unsigned long remote_vaddr;
+ unsigned long *pfn = phys_pfn;
+ struct vfio_dma *dma;
+ bool do_accounting;
+
+ if (!iommu || !user_pfn || !phys_pfn)
+ return -EINVAL;
+
+ mutex_lock(&iommu->lock);
+
+ if (!iommu->external_domain) {
+ ret = -EINVAL;
+ goto pin_done;
+ }
+
+ /*
+ * If iommu capable domain exist in the container then all pages are
+ * already pinned and accounted. Accouting should be done if there is no
+ * iommu capable domain in the container.
+ */
+ do_accounting = !IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu);
+
+ for (i = 0; i < npage; i++) {
+ struct vfio_pfn *p;
+ dma_addr_t iova;
+
+ iova = user_pfn[i] << PAGE_SHIFT;
+
+ dma = vfio_find_dma(iommu, iova, 0);
+ if (!dma) {
+ ret = -EINVAL;
+ goto pin_unwind;
+ }
+
+ remote_vaddr = dma->vaddr + iova - dma->iova;
+
+ ret = __vfio_pin_page_external(dma, remote_vaddr, prot,
+ &pfn[i], do_accounting);
+ if (ret <= 0) {
+ WARN_ON(!ret);
+ goto pin_unwind;
+ }
+
+ mutex_lock(&dma->addr_space->pfn_list_lock);
+
+ /* search if pfn exist */
+ p = vfio_find_pfn(dma->addr_space, pfn[i]);
+ if (p) {
+ atomic_inc(&p->ref_count);
+ mutex_unlock(&dma->addr_space->pfn_list_lock);
+ continue;
+ }
+
+ ret = vfio_add_to_pfn_list(dma->addr_space, pfn[i], prot);
+ mutex_unlock(&dma->addr_space->pfn_list_lock);
+
+ if (ret) {
+ __vfio_unpin_page_external(dma->addr_space, pfn[i],
+ prot, do_accounting);
+ goto pin_unwind;
+ }
+ }
+
+ ret = i;
+ goto pin_done;
+
+pin_unwind:
+ pfn[i] = 0;
+ for (j = 0; j < i; j++) {
+ struct vfio_pfn *p;
+ dma_addr_t iova;
+
+ iova = user_pfn[j] << PAGE_SHIFT;
+
+ dma = vfio_find_dma(iommu, iova, 0);
+
+ mutex_lock(&dma->addr_space->pfn_list_lock);
+ p = vfio_find_pfn(dma->addr_space, pfn[j]);
+ if (p)
+ vfio_unpin_pfn(dma->addr_space, p, do_accounting);
+
+ mutex_unlock(&dma->addr_space->pfn_list_lock);
+ pfn[j] = 0;
+ }
+
+pin_done:
+ mutex_unlock(&iommu->lock);
+ return ret;
+}
+
+static int vfio_iommu_type1_unpin_pages(void *iommu_data,
+ unsigned long *user_pfn,
+ unsigned long *pfn,
+ int npage)
+{
+ struct vfio_iommu *iommu = iommu_data;
+ bool do_accounting;
+ int unlocked = 0, i;
+
+ if (!iommu || !user_pfn || !pfn)
+ return -EINVAL;
+
+ mutex_lock(&iommu->lock);
+
+ if (!iommu->external_domain) {
+ mutex_unlock(&iommu->lock);
+ return -EINVAL;
+ }
+
+ do_accounting = !IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu);
+
+ for (i = 0; i < npage; i++) {
+ struct vfio_pfn *p;
+ struct vfio_dma *dma;
+ dma_addr_t iova;
+
+ iova = user_pfn[i] << PAGE_SHIFT;
+
+ dma = vfio_find_dma(iommu, iova, 0);
+ if (!dma)
+ goto unpin_exit;
+
+ mutex_lock(&dma->addr_space->pfn_list_lock);
+ /* verify if pfn exist in pfn_list */
+ p = vfio_find_pfn(dma->addr_space, pfn[i]);
+ if (p)
+ unlocked += vfio_unpin_pfn(dma->addr_space, p,
+ do_accounting);
+ mutex_unlock(&dma->addr_space->pfn_list_lock);
+ }
+unpin_exit:
+ mutex_unlock(&iommu->lock);
+ return unlocked;
+}
+
+static long vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma,
+ bool do_accounting)
{
dma_addr_t iova = dma->iova, end = dma->iova + dma->size;
struct vfio_domain *domain, *d;
long unlocked = 0;
if (!dma->size)
- return;
+ return 0;
+
+ if (!IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu))
+ return 0;
+
/*
* We use the IOMMU to track the physical addresses, otherwise we'd
* need a much more complicated tracking system. Unfortunately that
@@ -427,12 +737,17 @@ static void vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma)
cond_resched();
}
- vfio_lock_acct(dma->addr_space->mm, -unlocked);
+ dma->iommu_mapped = false;
+ if (do_accounting) {
+ vfio_lock_acct(dma->addr_space->mm, -unlocked);
+ return 0;
+ }
+ return unlocked;
}
static void vfio_remove_dma(struct vfio_iommu *iommu, struct vfio_dma *dma)
{
- vfio_unmap_unpin(iommu, dma);
+ vfio_unmap_unpin(iommu, dma, true);
vfio_unlink_dma(iommu, dma);
if (atomic_dec_and_test(&dma->addr_space->ref_count)) {
@@ -642,6 +957,8 @@ static int vfio_pin_map_dma(struct vfio_iommu *iommu, struct vfio_dma *dma,
dma->size += npage << PAGE_SHIFT;
}
+ dma->iommu_mapped = true;
+
if (ret)
vfio_remove_dma(iommu, dma);
@@ -706,6 +1023,8 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
goto do_map_err;
}
addr_space->mm = mm;
+ addr_space->pfn_list = RB_ROOT;
+ mutex_init(&addr_space->pfn_list_lock);
atomic_set(&addr_space->ref_count, 1);
list_add(&addr_space->next, &iommu->addr_space_list);
free_addr_space_on_err = true;
@@ -733,7 +1052,11 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
/* Insert zero-sized and grow as we map chunks of it */
vfio_link_dma(iommu, dma);
- ret = vfio_pin_map_dma(iommu, dma, size);
+ /* Don't pin and map if container doesn't contain IOMMU capable domain*/
+ if (!IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu))
+ dma->size = size;
+ else
+ ret = vfio_pin_map_dma(iommu, dma, size);
do_map_err:
mutex_unlock(&iommu->lock);
return ret;
@@ -762,10 +1085,6 @@ static int vfio_iommu_replay(struct vfio_iommu *iommu,
d = list_first_entry(&iommu->domain_list, struct vfio_domain, next);
n = rb_first(&iommu->dma_list);
- /* If there's not a domain, there better not be any mappings */
- if (WARN_ON(n && !d))
- return -EINVAL;
-
for (; n; n = rb_next(n)) {
struct vfio_dma *dma;
dma_addr_t iova;
@@ -774,20 +1093,43 @@ static int vfio_iommu_replay(struct vfio_iommu *iommu,
iova = dma->iova;
while (iova < dma->iova + dma->size) {
- phys_addr_t phys = iommu_iova_to_phys(d->domain, iova);
+ phys_addr_t phys;
size_t size;
- if (WARN_ON(!phys)) {
- iova += PAGE_SIZE;
- continue;
- }
+ if (dma->iommu_mapped) {
+ phys = iommu_iova_to_phys(d->domain, iova);
+
+ if (WARN_ON(!phys)) {
+ iova += PAGE_SIZE;
+ continue;
+ }
- size = PAGE_SIZE;
+ size = PAGE_SIZE;
- while (iova + size < dma->iova + dma->size &&
- phys + size == iommu_iova_to_phys(d->domain,
+ while (iova + size < dma->iova + dma->size &&
+ phys + size == iommu_iova_to_phys(d->domain,
iova + size))
- size += PAGE_SIZE;
+ size += PAGE_SIZE;
+ } else {
+ unsigned long pfn;
+ unsigned long vaddr = dma->vaddr +
+ (iova - dma->iova);
+ size_t n = dma->iova + dma->size - iova;
+ long npage;
+
+ npage = __vfio_pin_pages_remote(dma, vaddr,
+ n >> PAGE_SHIFT,
+ dma->prot,
+ &pfn);
+ if (npage <= 0) {
+ WARN_ON(!npage);
+ ret = (int)npage;
+ return ret;
+ }
+
+ phys = pfn << PAGE_SHIFT;
+ size = npage << PAGE_SHIFT;
+ }
ret = iommu_map(domain->domain, iova, phys,
size, dma->prot | domain->prot);
@@ -796,6 +1138,8 @@ static int vfio_iommu_replay(struct vfio_iommu *iommu,
iova += size;
}
+
+ dma->iommu_mapped = true;
}
return 0;
@@ -853,7 +1197,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
struct vfio_iommu *iommu = iommu_data;
struct vfio_group *group;
struct vfio_domain *domain, *d;
- struct bus_type *bus = NULL;
+ struct bus_type *bus = NULL, *mdev_bus;
int ret;
mutex_lock(&iommu->lock);
@@ -865,6 +1209,13 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
}
}
+ if (iommu->external_domain) {
+ if (find_iommu_group(iommu->external_domain, iommu_group)) {
+ mutex_unlock(&iommu->lock);
+ return -EINVAL;
+ }
+ }
+
group = kzalloc(sizeof(*group), GFP_KERNEL);
domain = kzalloc(sizeof(*domain), GFP_KERNEL);
if (!group || !domain) {
@@ -879,6 +1230,25 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
if (ret)
goto out_free;
+ mdev_bus = symbol_get(mdev_bus_type);
+
+ if (mdev_bus) {
+ if ((bus == mdev_bus) && !iommu_present(bus)) {
+ symbol_put(mdev_bus_type);
+ if (!iommu->external_domain) {
+ INIT_LIST_HEAD(&domain->group_list);
+ iommu->external_domain = domain;
+ } else
+ kfree(domain);
+
+ list_add(&group->next,
+ &iommu->external_domain->group_list);
+ mutex_unlock(&iommu->lock);
+ return 0;
+ }
+ symbol_put(mdev_bus_type);
+ }
+
domain->domain = iommu_domain_alloc(bus);
if (!domain->domain) {
ret = -EIO;
@@ -969,6 +1339,51 @@ static void vfio_iommu_unmap_unpin_all(struct vfio_iommu *iommu)
vfio_remove_dma(iommu, rb_entry(node, struct vfio_dma, node));
}
+static void vfio_iommu_unmap_unpin_reaccount(struct vfio_iommu *iommu)
+{
+ struct vfio_addr_space *as;
+
+ list_for_each_entry(as, &iommu->addr_space_list, next) {
+ struct rb_node *n, *p;
+ long locked = 0, unlocked = 0;
+
+ n = rb_first(&iommu->dma_list);
+ for (; n; n = rb_next(n)) {
+ struct vfio_dma *dma;
+
+ dma = rb_entry(n, struct vfio_dma, node);
+ if (dma->addr_space == as)
+ unlocked += vfio_unmap_unpin(iommu, dma, false);
+ }
+
+ mutex_lock(&as->pfn_list_lock);
+ p = rb_first(&as->pfn_list);
+ for (; p; p = rb_next(p))
+ locked++;
+
+ mutex_unlock(&as->pfn_list_lock);
+ vfio_lock_acct(as->mm, locked - unlocked);
+ }
+}
+
+static void vfio_external_unpin_all(struct vfio_iommu *iommu,
+ bool do_accounting)
+{
+ struct vfio_addr_space *as;
+
+ list_for_each_entry(as, &iommu->addr_space_list, next) {
+ struct rb_node *node;
+
+ mutex_lock(&as->pfn_list_lock);
+ while ((node = rb_first(&as->pfn_list)))
+ vfio_unpin_pfn(as,
+ rb_entry(node, struct vfio_pfn, node),
+ do_accounting);
+
+ mutex_unlock(&as->pfn_list_lock);
+ }
+}
+
static void vfio_iommu_type1_detach_group(void *iommu_data,
struct iommu_group *iommu_group)
{
@@ -978,6 +1393,28 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
mutex_lock(&iommu->lock);
+ if (iommu->external_domain) {
+ domain = iommu->external_domain;
+ group = find_iommu_group(domain, iommu_group);
+ if (group) {
+ list_del(&group->next);
+ kfree(group);
+
+ if (list_empty(&domain->group_list)) {
+ if (!IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu)) {
+ vfio_external_unpin_all(iommu, true);
+ vfio_iommu_unmap_unpin_all(iommu);
+ } else
+ vfio_external_unpin_all(iommu, false);
+ kfree(domain);
+ iommu->external_domain = NULL;
+ }
+ goto detach_group_done;
+ }
+ }
+
+ if (!IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu))
+ goto detach_group_done;
list_for_each_entry(domain, &iommu->domain_list, next) {
group = find_iommu_group(domain, iommu_group);
@@ -988,21 +1425,27 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
list_del(&group->next);
kfree(group);
/*
- * Group ownership provides privilege, if the group
- * list is empty, the domain goes away. If it's the
- * last domain, then all the mappings go away too.
+ * Group ownership provides privilege, if the group list is
+ * empty, the domain goes away. If it's the last domain with
+ * iommu and external domain doesn't exist, then all the
+ * mappings go away too. If it's the last domain with iommu and
+ * external domain exist, update accounting
*/
if (list_empty(&domain->group_list)) {
- if (list_is_singular(&iommu->domain_list))
- vfio_iommu_unmap_unpin_all(iommu);
+ if (list_is_singular(&iommu->domain_list)) {
+ if (!iommu->external_domain)
+ vfio_iommu_unmap_unpin_all(iommu);
+ else
+ vfio_iommu_unmap_unpin_reaccount(iommu);
+ }
iommu_domain_free(domain->domain);
list_del(&domain->next);
kfree(domain);
}
- goto done;
+ break;
}
-done:
+detach_group_done:
mutex_unlock(&iommu->lock);
}
@@ -1028,29 +1471,46 @@ static void *vfio_iommu_type1_open(unsigned long arg)
}
INIT_LIST_HEAD(&iommu->domain_list);
+ INIT_LIST_HEAD(&iommu->addr_space_list);
iommu->dma_list = RB_ROOT;
mutex_init(&iommu->lock);
return iommu;
}
+static void vfio_release_domain(struct vfio_domain *domain, bool external)
+{
+ struct vfio_group *group, *group_tmp;
+
+ list_for_each_entry_safe(group, group_tmp,
+ &domain->group_list, next) {
+ if (!external)
+ iommu_detach_group(domain->domain, group->iommu_group);
+ list_del(&group->next);
+ kfree(group);
+ }
+
+ if (!external)
+ iommu_domain_free(domain->domain);
+}
+
static void vfio_iommu_type1_release(void *iommu_data)
{
struct vfio_iommu *iommu = iommu_data;
struct vfio_domain *domain, *domain_tmp;
- struct vfio_group *group, *group_tmp;
+
+ if (iommu->external_domain) {
+ vfio_release_domain(iommu->external_domain, true);
+ vfio_external_unpin_all(iommu, false);
+ kfree(iommu->external_domain);
+ iommu->external_domain = NULL;
+ }
vfio_iommu_unmap_unpin_all(iommu);
list_for_each_entry_safe(domain, domain_tmp,
&iommu->domain_list, next) {
- list_for_each_entry_safe(group, group_tmp,
- &domain->group_list, next) {
- iommu_detach_group(domain->domain, group->iommu_group);
- list_del(&group->next);
- kfree(group);
- }
- iommu_domain_free(domain->domain);
+ vfio_release_domain(domain, false);
list_del(&domain->next);
kfree(domain);
}
@@ -1158,6 +1618,8 @@ static const struct vfio_iommu_driver_ops vfio_iommu_driver_ops_type1 = {
.ioctl = vfio_iommu_type1_ioctl,
.attach_group = vfio_iommu_type1_attach_group,
.detach_group = vfio_iommu_type1_detach_group,
+ .pin_pages = vfio_iommu_type1_pin_pages,
+ .unpin_pages = vfio_iommu_type1_unpin_pages,
};
static int __init vfio_iommu_type1_init(void)
--
2.7.0
[toc] | [prev] | [next] | [standalone]
| From | Alex Williamson <alex.williamson@redhat.com> |
|---|---|
| Date | 2016-11-08 00:20 +0100 |
| Subject | Re: [PATCH v11 10/22] vfio iommu type1: Add support for mediated devices |
| Message-ID | <sB1mN-1rD-1@gated-at.bofh.it> |
| In reply to | #1515428 |
On Sat, 5 Nov 2016 02:40:44 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:
> VFIO IOMMU drivers are designed for the devices which are IOMMU capable.
> Mediated device only uses IOMMU APIs, the underlying hardware can be
> managed by an IOMMU domain.
>
> Aim of this change is:
> - To use most of the code of TYPE1 IOMMU driver for mediated devices
> - To support direct assigned device and mediated device in single module
>
> This change adds pin and unpin support for mediated device to TYPE1 IOMMU
> backend module. More details:
> - vfio_pin_pages() callback here uses task and address space of vfio_dma,
> that is, of the process who mapped that iova range.
> - Added pfn_list tracking logic to address space structure. All pages
> pinned through this interface are trached in its address space.
^ k
------------------------------------------|
> - Pinned pages list is used to verify unpinning request and to unpin
> remaining pages while detaching the group for that device.
> - Page accounting is updated to account in its address space where the
> pages are pinned/unpinned.
> - Accouting for mdev device is only done if there is no iommu capable
> domain in the container. When there is a direct device assigned to the
> container and that domain is iommu capable, all pages are already pinned
> during DMA_MAP.
> - Page accouting is updated on hot plug and unplug mdev device and pass
> through device.
>
> Tested by assigning below combinations of devices to a single VM:
> - GPU pass through only
> - vGPU device only
> - One GPU pass through and one vGPU device
> - Linux VM hot plug and unplug vGPU device while GPU pass through device
> exist
> - Linux VM hot plug and unplug GPU pass through device while vGPU device
> exist
>
> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> Signed-off-by: Neo Jia <cjia@nvidia.com>
> Change-Id: I295d6f0f2e0579b8d9882bfd8fd5a4194b97bd9a
> ---
> drivers/vfio/vfio_iommu_type1.c | 538 +++++++++++++++++++++++++++++++++++++---
> 1 file changed, 500 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 8d64528dcc22..e511073446a0 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -36,6 +36,7 @@
> #include <linux/uaccess.h>
> #include <linux/vfio.h>
> #include <linux/workqueue.h>
> +#include <linux/mdev.h>
>
> #define DRIVER_VERSION "0.2"
> #define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>"
> @@ -56,6 +57,7 @@ MODULE_PARM_DESC(disable_hugepages,
> struct vfio_iommu {
> struct list_head domain_list;
> struct list_head addr_space_list;
> + struct vfio_domain *external_domain; /* domain for external user */
> struct mutex lock;
> struct rb_root dma_list;
> bool v2;
> @@ -67,6 +69,9 @@ struct vfio_addr_space {
> struct mm_struct *mm;
> struct list_head next;
> atomic_t ref_count;
> + /* external user pinned pfns */
> + struct rb_root pfn_list; /* pinned Host pfn list */
> + struct mutex pfn_list_lock; /* mutex for pfn_list */
> };
>
> struct vfio_domain {
> @@ -83,6 +88,7 @@ struct vfio_dma {
> unsigned long vaddr; /* Process virtual addr */
> size_t size; /* Map size (bytes) */
> int prot; /* IOMMU_READ/WRITE */
> + bool iommu_mapped;
> struct vfio_addr_space *addr_space;
> struct task_struct *task;
> bool mlock_cap;
> @@ -94,6 +100,19 @@ struct vfio_group {
> };
>
> /*
> + * Guest RAM pinning working set or DMA target
> + */
> +struct vfio_pfn {
> + struct rb_node node;
> + unsigned long pfn; /* Host pfn */
> + int prot;
> + atomic_t ref_count;
> +};
> +
> +#define IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu) \
> + (!list_empty(&iommu->domain_list))
> +
> +/*
> * This code handles mapping and unmapping of user data buffers
> * into DMA'ble space using the IOMMU
> */
> @@ -153,6 +172,93 @@ static struct vfio_addr_space *vfio_find_addr_space(struct vfio_iommu *iommu,
> return NULL;
> }
>
> +/*
> + * Helper Functions for host pfn list
> + */
> +static struct vfio_pfn *vfio_find_pfn(struct vfio_addr_space *addr_space,
> + unsigned long pfn)
> +{
> + struct vfio_pfn *vpfn;
> + struct rb_node *node = addr_space->pfn_list.rb_node;
> +
> + while (node) {
> + vpfn = rb_entry(node, struct vfio_pfn, node);
> +
> + if (pfn < vpfn->pfn)
> + node = node->rb_left;
> + else if (pfn > vpfn->pfn)
> + node = node->rb_right;
> + else
> + return vpfn;
> + }
> +
> + return NULL;
> +}
> +
> +static void vfio_link_pfn(struct vfio_addr_space *addr_space,
> + struct vfio_pfn *new)
> +{
> + struct rb_node **link, *parent = NULL;
> + struct vfio_pfn *vpfn;
> +
> + link = &addr_space->pfn_list.rb_node;
> + while (*link) {
> + parent = *link;
> + vpfn = rb_entry(parent, struct vfio_pfn, node);
> +
> + if (new->pfn < vpfn->pfn)
> + link = &(*link)->rb_left;
> + else
> + link = &(*link)->rb_right;
> + }
> +
> + rb_link_node(&new->node, parent, link);
> + rb_insert_color(&new->node, &addr_space->pfn_list);
> +}
> +
> +static void vfio_unlink_pfn(struct vfio_addr_space *addr_space,
> + struct vfio_pfn *old)
> +{
> + rb_erase(&old->node, &addr_space->pfn_list);
> +}
> +
> +static int vfio_add_to_pfn_list(struct vfio_addr_space *addr_space,
> + unsigned long pfn, int prot)
> +{
> + struct vfio_pfn *vpfn;
> +
> + vpfn = kzalloc(sizeof(*vpfn), GFP_KERNEL);
> + if (!vpfn)
> + return -ENOMEM;
> +
> + vpfn->pfn = pfn;
> + vpfn->prot = prot;
> + atomic_set(&vpfn->ref_count, 1);
> + vfio_link_pfn(addr_space, vpfn);
> + return 0;
> +}
> +
> +static void vfio_remove_from_pfn_list(struct vfio_addr_space *addr_space,
> + struct vfio_pfn *vpfn)
> +{
> + vfio_unlink_pfn(addr_space, vpfn);
> + kfree(vpfn);
> +}
> +
> +static int vfio_pfn_account(struct vfio_addr_space *addr_space,
> + unsigned long pfn)
> +{
> + struct vfio_pfn *p;
> + int ret = 1;
> +
> + mutex_lock(&addr_space->pfn_list_lock);
> + p = vfio_find_pfn(addr_space, pfn);
> + if (p)
> + ret = 0;
> + mutex_unlock(&addr_space->pfn_list_lock);
> + return ret;
> +}
> +
> struct vwork {
> struct mm_struct *mm;
> long npage;
> @@ -304,16 +410,18 @@ static long __vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr,
> unsigned long limit = task_rlimit(task, RLIMIT_MEMLOCK) >> PAGE_SHIFT;
> bool lock_cap = dma->mlock_cap;
> struct mm_struct *mm = dma->addr_space->mm;
> - long ret, i;
> + long ret, i, lock_acct;
> bool rsvd;
>
> ret = vaddr_get_pfn(mm, vaddr, prot, pfn_base);
> if (ret)
> return ret;
>
> + lock_acct = vfio_pfn_account(dma->addr_space, *pfn_base);
> +
> rsvd = is_invalid_reserved_pfn(*pfn_base);
>
> - if (!rsvd && !lock_cap && mm->locked_vm + 1 > limit) {
> + if (!rsvd && !lock_cap && mm->locked_vm + lock_acct > limit) {
> put_pfn(*pfn_base, prot);
> pr_warn("%s: RLIMIT_MEMLOCK (%ld) exceeded\n", __func__,
> limit << PAGE_SHIFT);
> @@ -340,8 +448,10 @@ static long __vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr,
> break;
> }
>
> + lock_acct += vfio_pfn_account(dma->addr_space, pfn);
> +
> if (!rsvd && !lock_cap &&
> - mm->locked_vm + i + 1 > limit) {
> + mm->locked_vm + lock_acct + 1 > limit) {
> put_pfn(pfn, prot);
> pr_warn("%s: RLIMIT_MEMLOCK (%ld) exceeded\n",
> __func__, limit << PAGE_SHIFT);
> @@ -350,7 +460,7 @@ static long __vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr,
> }
>
> if (!rsvd)
> - vfio_lock_acct(mm, i);
> + vfio_lock_acct(mm, lock_acct);
>
> return i;
> }
> @@ -370,14 +480,214 @@ static long __vfio_unpin_pages_remote(struct vfio_dma *dma, unsigned long pfn,
> return unlocked;
> }
>
> -static void vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma)
> +static int __vfio_pin_page_external(struct vfio_dma *dma, unsigned long vaddr,
> + int prot, unsigned long *pfn_base,
> + bool do_accounting)
> +{
> + struct task_struct *task = dma->task;
> + unsigned long limit = task_rlimit(task, RLIMIT_MEMLOCK) >> PAGE_SHIFT;
> + bool lock_cap = dma->mlock_cap;
> + struct mm_struct *mm = dma->addr_space->mm;
> + int ret;
> + bool rsvd;
> +
> + ret = vaddr_get_pfn(mm, vaddr, prot, pfn_base);
> + if (ret)
> + return ret;
> +
> + rsvd = is_invalid_reserved_pfn(*pfn_base);
> +
> + if (!rsvd && !lock_cap && mm->locked_vm + 1 > limit) {
> + put_pfn(*pfn_base, prot);
> + pr_warn("%s: Task %s (%d) RLIMIT_MEMLOCK (%ld) exceeded\n",
> + __func__, task->comm, task_pid_nr(task),
> + limit << PAGE_SHIFT);
> + return -ENOMEM;
> + }
> +
> + if (!rsvd && do_accounting)
> + vfio_lock_acct(mm, 1);
> +
> + return 1;
> +}
> +
> +static void __vfio_unpin_page_external(struct vfio_addr_space *addr_space,
> + unsigned long pfn, int prot,
> + bool do_accounting)
> +{
> + put_pfn(pfn, prot);
> +
> + if (do_accounting)
> + vfio_lock_acct(addr_space->mm, -1);
Can't we batch this like we do elsewhere? Intel folks, AIUI you intend
to pin all VM memory through this side channel, have you tested the
scalability and performance of this with larger VMs? Our vfio_pfn
data structure alone is 40 bytes per pinned page, which means for
each 1GB of VM memory, we have 10MBs worth of struct vfio_pfn!
Additionally, unmapping each 1GB of VM memory will result in 256k
separate vfio_lock_acct() callbacks. I'm concerned that we're not
being efficient enough in either space or time.
One thought might be whether we really need to save the pfn, we better
always get the same result if we pin it again, or maybe we can just do
a lookup through the mm at that point without re-pinning. Could we get
to the point where we only need an atomic_t ref count per page in a
linear array relative to the IOVA? That would give us 1MB per 1GB
overhead. The semantics of the pin and unpin would make more sense then
too, both would take an IOVA range, only pinning would need a return
mechanism. For instance:
int pin_pages(void *iommu_data, dma_addr_t iova_base,
int npage, unsigned long *pfn_base);
This would pin physically contiguous pages up to npage, returning the
base pfn and returning the number of pages pinned (<= npage). The
vendor driver would make multiple calls to fill the necessary range.
Unpin would then simply be:
void unpin_pages(void *iommu_data, dma_addr_t iova_base, int npage);
Hugepage usage would really make such an interface shine (ie. 2MB+
contiguous ranges). A downside would be the overhead of getting the
group and container reference in vfio for each callback, perhaps we'd
need to figure out how the vendor driver could hold that reference.
The current API of passing around pfn arrays, further increasing the
overhead of the whole ecosystem just makes me cringe though.
> +}
> +
> +static int vfio_unpin_pfn(struct vfio_addr_space *addr_space,
> + struct vfio_pfn *vpfn, bool do_accounting)
> +{
> + __vfio_unpin_page_external(addr_space, vpfn->pfn, vpfn->prot,
> + do_accounting);
> +
> + if (atomic_dec_and_test(&vpfn->ref_count))
> + vfio_remove_from_pfn_list(addr_space, vpfn);
> +
> + return 1;
> +}
> +
> +static int vfio_iommu_type1_pin_pages(void *iommu_data,
> + unsigned long *user_pfn,
> + int npage, int prot,
> + unsigned long *phys_pfn)
> +{
> + struct vfio_iommu *iommu = iommu_data;
> + int i, j, ret;
> + unsigned long remote_vaddr;
> + unsigned long *pfn = phys_pfn;
> + struct vfio_dma *dma;
> + bool do_accounting;
> +
> + if (!iommu || !user_pfn || !phys_pfn)
> + return -EINVAL;
> +
> + mutex_lock(&iommu->lock);
> +
> + if (!iommu->external_domain) {
> + ret = -EINVAL;
> + goto pin_done;
> + }
> +
> + /*
> + * If iommu capable domain exist in the container then all pages are
> + * already pinned and accounted. Accouting should be done if there is no
> + * iommu capable domain in the container.
> + */
> + do_accounting = !IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu);
> +
> + for (i = 0; i < npage; i++) {
> + struct vfio_pfn *p;
> + dma_addr_t iova;
> +
> + iova = user_pfn[i] << PAGE_SHIFT;
> +
> + dma = vfio_find_dma(iommu, iova, 0);
> + if (!dma) {
> + ret = -EINVAL;
> + goto pin_unwind;
> + }
> +
> + remote_vaddr = dma->vaddr + iova - dma->iova;
> +
> + ret = __vfio_pin_page_external(dma, remote_vaddr, prot,
> + &pfn[i], do_accounting);
Please wrap such that the following line falls within the relevant ()s
when possible. Above and below on unpin calls.
> + if (ret <= 0) {
> + WARN_ON(!ret);
> + goto pin_unwind;
> + }
> +
> + mutex_lock(&dma->addr_space->pfn_list_lock);
> +
> + /* search if pfn exist */
> + p = vfio_find_pfn(dma->addr_space, pfn[i]);
> + if (p) {
> + atomic_inc(&p->ref_count);
We never test whether (p->prot == prot), shouldn't we be doing
something in that case? In fact, why do we allow the side-channel
through the .{un}pin_pages to specify page protection flags that might
be different than the user specified for the DMA_MAP? If the user
specified read-only, the vendor driver should not be allowed to
override with read-write access.
> + mutex_unlock(&dma->addr_space->pfn_list_lock);
> + continue;
> + }
> +
> + ret = vfio_add_to_pfn_list(dma->addr_space, pfn[i], prot);
> + mutex_unlock(&dma->addr_space->pfn_list_lock);
> +
> + if (ret) {
> + __vfio_unpin_page_external(dma->addr_space, pfn[i],
> + prot, do_accounting);
> + goto pin_unwind;
> + }
> + }
> +
> + ret = i;
> + goto pin_done;
> +
> +pin_unwind:
> + pfn[i] = 0;
> + for (j = 0; j < i; j++) {
> + struct vfio_pfn *p;
> + dma_addr_t iova;
> +
> + iova = user_pfn[j] << PAGE_SHIFT;
> +
> + dma = vfio_find_dma(iommu, iova, 0);
> +
> + mutex_lock(&dma->addr_space->pfn_list_lock);
> + p = vfio_find_pfn(dma->addr_space, pfn[j]);
> + if (p)
> + vfio_unpin_pfn(dma->addr_space, p, do_accounting);
> +
> + mutex_unlock(&dma->addr_space->pfn_list_lock);
> + pfn[j] = 0;
> + }
> +
> +pin_done:
> + mutex_unlock(&iommu->lock);
> + return ret;
> +}
> +
> +static int vfio_iommu_type1_unpin_pages(void *iommu_data,
> + unsigned long *user_pfn,
> + unsigned long *pfn,
> + int npage)
> +{
> + struct vfio_iommu *iommu = iommu_data;
> + bool do_accounting;
> + int unlocked = 0, i;
> +
> + if (!iommu || !user_pfn || !pfn)
> + return -EINVAL;
> +
> + mutex_lock(&iommu->lock);
> +
> + if (!iommu->external_domain) {
> + mutex_unlock(&iommu->lock);
> + return -EINVAL;
> + }
> +
> + do_accounting = !IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu);
> +
> + for (i = 0; i < npage; i++) {
> + struct vfio_pfn *p;
> + struct vfio_dma *dma;
> + dma_addr_t iova;
> +
> + iova = user_pfn[i] << PAGE_SHIFT;
> +
> + dma = vfio_find_dma(iommu, iova, 0);
> + if (!dma)
> + goto unpin_exit;
> +
> + mutex_lock(&dma->addr_space->pfn_list_lock);
> + /* verify if pfn exist in pfn_list */
> + p = vfio_find_pfn(dma->addr_space, pfn[i]);
> + if (p)
> + unlocked += vfio_unpin_pfn(dma->addr_space, p,
> + do_accounting);
> + mutex_unlock(&dma->addr_space->pfn_list_lock);
> + }
> +unpin_exit:
> + mutex_unlock(&iommu->lock);
> + return unlocked;
> +}
> +
> +static long vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma,
> + bool do_accounting)
> {
> dma_addr_t iova = dma->iova, end = dma->iova + dma->size;
> struct vfio_domain *domain, *d;
> long unlocked = 0;
>
> if (!dma->size)
> - return;
> + return 0;
> +
> + if (!IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu))
> + return 0;
> +
> /*
> * We use the IOMMU to track the physical addresses, otherwise we'd
> * need a much more complicated tracking system. Unfortunately that
> @@ -427,12 +737,17 @@ static void vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma)
> cond_resched();
> }
>
> - vfio_lock_acct(dma->addr_space->mm, -unlocked);
> + dma->iommu_mapped = false;
> + if (do_accounting) {
> + vfio_lock_acct(dma->addr_space->mm, -unlocked);
> + return 0;
> + }
> + return unlocked;
> }
>
> static void vfio_remove_dma(struct vfio_iommu *iommu, struct vfio_dma *dma)
> {
> - vfio_unmap_unpin(iommu, dma);
> + vfio_unmap_unpin(iommu, dma, true);
> vfio_unlink_dma(iommu, dma);
>
> if (atomic_dec_and_test(&dma->addr_space->ref_count)) {
> @@ -642,6 +957,8 @@ static int vfio_pin_map_dma(struct vfio_iommu *iommu, struct vfio_dma *dma,
> dma->size += npage << PAGE_SHIFT;
> }
>
> + dma->iommu_mapped = true;
> +
> if (ret)
> vfio_remove_dma(iommu, dma);
>
> @@ -706,6 +1023,8 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
> goto do_map_err;
> }
> addr_space->mm = mm;
> + addr_space->pfn_list = RB_ROOT;
> + mutex_init(&addr_space->pfn_list_lock);
> atomic_set(&addr_space->ref_count, 1);
> list_add(&addr_space->next, &iommu->addr_space_list);
> free_addr_space_on_err = true;
> @@ -733,7 +1052,11 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
> /* Insert zero-sized and grow as we map chunks of it */
> vfio_link_dma(iommu, dma);
>
> - ret = vfio_pin_map_dma(iommu, dma, size);
> + /* Don't pin and map if container doesn't contain IOMMU capable domain*/
> + if (!IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu))
> + dma->size = size;
> + else
> + ret = vfio_pin_map_dma(iommu, dma, size);
> do_map_err:
> mutex_unlock(&iommu->lock);
> return ret;
> @@ -762,10 +1085,6 @@ static int vfio_iommu_replay(struct vfio_iommu *iommu,
> d = list_first_entry(&iommu->domain_list, struct vfio_domain, next);
> n = rb_first(&iommu->dma_list);
>
> - /* If there's not a domain, there better not be any mappings */
> - if (WARN_ON(n && !d))
> - return -EINVAL;
> -
> for (; n; n = rb_next(n)) {
> struct vfio_dma *dma;
> dma_addr_t iova;
> @@ -774,20 +1093,43 @@ static int vfio_iommu_replay(struct vfio_iommu *iommu,
> iova = dma->iova;
>
> while (iova < dma->iova + dma->size) {
> - phys_addr_t phys = iommu_iova_to_phys(d->domain, iova);
> + phys_addr_t phys;
> size_t size;
>
> - if (WARN_ON(!phys)) {
> - iova += PAGE_SIZE;
> - continue;
> - }
> + if (dma->iommu_mapped) {
> + phys = iommu_iova_to_phys(d->domain, iova);
> +
> + if (WARN_ON(!phys)) {
> + iova += PAGE_SIZE;
> + continue;
> + }
>
> - size = PAGE_SIZE;
> + size = PAGE_SIZE;
>
> - while (iova + size < dma->iova + dma->size &&
> - phys + size == iommu_iova_to_phys(d->domain,
> + while (iova + size < dma->iova + dma->size &&
> + phys + size == iommu_iova_to_phys(d->domain,
> iova + size))
> - size += PAGE_SIZE;
> + size += PAGE_SIZE;
> + } else {
> + unsigned long pfn;
> + unsigned long vaddr = dma->vaddr +
> + (iova - dma->iova);
> + size_t n = dma->iova + dma->size - iova;
> + long npage;
> +
> + npage = __vfio_pin_pages_remote(dma, vaddr,
> + n >> PAGE_SHIFT,
> + dma->prot,
> + &pfn);
> + if (npage <= 0) {
> + WARN_ON(!npage);
> + ret = (int)npage;
> + return ret;
> + }
> +
> + phys = pfn << PAGE_SHIFT;
> + size = npage << PAGE_SHIFT;
> + }
>
> ret = iommu_map(domain->domain, iova, phys,
> size, dma->prot | domain->prot);
> @@ -796,6 +1138,8 @@ static int vfio_iommu_replay(struct vfio_iommu *iommu,
>
> iova += size;
> }
> +
> + dma->iommu_mapped = true;
> }
>
> return 0;
> @@ -853,7 +1197,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
> struct vfio_iommu *iommu = iommu_data;
> struct vfio_group *group;
> struct vfio_domain *domain, *d;
> - struct bus_type *bus = NULL;
> + struct bus_type *bus = NULL, *mdev_bus;
> int ret;
>
> mutex_lock(&iommu->lock);
> @@ -865,6 +1209,13 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
> }
> }
>
> + if (iommu->external_domain) {
> + if (find_iommu_group(iommu->external_domain, iommu_group)) {
> + mutex_unlock(&iommu->lock);
> + return -EINVAL;
> + }
> + }
> +
> group = kzalloc(sizeof(*group), GFP_KERNEL);
> domain = kzalloc(sizeof(*domain), GFP_KERNEL);
> if (!group || !domain) {
> @@ -879,6 +1230,25 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
> if (ret)
> goto out_free;
>
> + mdev_bus = symbol_get(mdev_bus_type);
> +
> + if (mdev_bus) {
> + if ((bus == mdev_bus) && !iommu_present(bus)) {
> + symbol_put(mdev_bus_type);
> + if (!iommu->external_domain) {
> + INIT_LIST_HEAD(&domain->group_list);
> + iommu->external_domain = domain;
> + } else
> + kfree(domain);
> +
> + list_add(&group->next,
> + &iommu->external_domain->group_list);
> + mutex_unlock(&iommu->lock);
> + return 0;
> + }
> + symbol_put(mdev_bus_type);
> + }
> +
> domain->domain = iommu_domain_alloc(bus);
> if (!domain->domain) {
> ret = -EIO;
> @@ -969,6 +1339,51 @@ static void vfio_iommu_unmap_unpin_all(struct vfio_iommu *iommu)
> vfio_remove_dma(iommu, rb_entry(node, struct vfio_dma, node));
> }
>
> +static void vfio_iommu_unmap_unpin_reaccount(struct vfio_iommu *iommu)
> +{
> + struct vfio_addr_space *as;
> +
> + list_for_each_entry(as, &iommu->addr_space_list, next) {
> + struct rb_node *n, *p;
> + long locked = 0, unlocked = 0;
> +
> + n = rb_first(&iommu->dma_list);
> + for (; n; n = rb_next(n)) {
> + struct vfio_dma *dma;
> +
> + dma = rb_entry(n, struct vfio_dma, node);
> + if (dma->addr_space == as)
> + unlocked += vfio_unmap_unpin(iommu, dma, false);
> + }
> +
> + mutex_lock(&as->pfn_list_lock);
> + p = rb_first(&as->pfn_list);
> + for (; p; p = rb_next(p))
> + locked++;
> +
> + mutex_unlock(&as->pfn_list_lock);
> + vfio_lock_acct(as->mm, locked - unlocked);
> + }
> +}
> +
> +static void vfio_external_unpin_all(struct vfio_iommu *iommu,
> + bool do_accounting)
> +{
> + struct vfio_addr_space *as;
> +
> + list_for_each_entry(as, &iommu->addr_space_list, next) {
> + struct rb_node *node;
> +
> + mutex_lock(&as->pfn_list_lock);
> + while ((node = rb_first(&as->pfn_list)))
> + vfio_unpin_pfn(as,
> + rb_entry(node, struct vfio_pfn, node),
> + do_accounting);
> +
> + mutex_unlock(&as->pfn_list_lock);
> + }
> +}
> +
> static void vfio_iommu_type1_detach_group(void *iommu_data,
> struct iommu_group *iommu_group)
> {
> @@ -978,6 +1393,28 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
>
> mutex_lock(&iommu->lock);
>
> + if (iommu->external_domain) {
> + domain = iommu->external_domain;
> + group = find_iommu_group(domain, iommu_group);
> + if (group) {
> + list_del(&group->next);
> + kfree(group);
> +
> + if (list_empty(&domain->group_list)) {
> + if (!IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu)) {
> + vfio_external_unpin_all(iommu, true);
> + vfio_iommu_unmap_unpin_all(iommu);
> + } else
> + vfio_external_unpin_all(iommu, false);
> + kfree(domain);
> + iommu->external_domain = NULL;
> + }
> + goto detach_group_done;
> + }
> + }
> +
> + if (!IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu))
> + goto detach_group_done;
>
> list_for_each_entry(domain, &iommu->domain_list, next) {
> group = find_iommu_group(domain, iommu_group);
> @@ -988,21 +1425,27 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
> list_del(&group->next);
> kfree(group);
> /*
> - * Group ownership provides privilege, if the group
> - * list is empty, the domain goes away. If it's the
> - * last domain, then all the mappings go away too.
> + * Group ownership provides privilege, if the group list is
> + * empty, the domain goes away. If it's the last domain with
> + * iommu and external domain doesn't exist, then all the
> + * mappings go away too. If it's the last domain with iommu and
> + * external domain exist, update accounting
> */
> if (list_empty(&domain->group_list)) {
> - if (list_is_singular(&iommu->domain_list))
> - vfio_iommu_unmap_unpin_all(iommu);
> + if (list_is_singular(&iommu->domain_list)) {
> + if (!iommu->external_domain)
> + vfio_iommu_unmap_unpin_all(iommu);
> + else
> + vfio_iommu_unmap_unpin_reaccount(iommu);
> + }
> iommu_domain_free(domain->domain);
> list_del(&domain->next);
> kfree(domain);
> }
> - goto done;
> + break;
> }
>
> -done:
> +detach_group_done:
> mutex_unlock(&iommu->lock);
> }
>
> @@ -1028,29 +1471,46 @@ static void *vfio_iommu_type1_open(unsigned long arg)
> }
>
> INIT_LIST_HEAD(&iommu->domain_list);
> + INIT_LIST_HEAD(&iommu->addr_space_list);
> iommu->dma_list = RB_ROOT;
> mutex_init(&iommu->lock);
>
> return iommu;
> }
>
> +static void vfio_release_domain(struct vfio_domain *domain, bool external)
> +{
> + struct vfio_group *group, *group_tmp;
> +
> + list_for_each_entry_safe(group, group_tmp,
> + &domain->group_list, next) {
> + if (!external)
> + iommu_detach_group(domain->domain, group->iommu_group);
> + list_del(&group->next);
> + kfree(group);
> + }
> +
> + if (!external)
> + iommu_domain_free(domain->domain);
> +}
> +
> static void vfio_iommu_type1_release(void *iommu_data)
> {
> struct vfio_iommu *iommu = iommu_data;
> struct vfio_domain *domain, *domain_tmp;
> - struct vfio_group *group, *group_tmp;
> +
> + if (iommu->external_domain) {
> + vfio_release_domain(iommu->external_domain, true);
> + vfio_external_unpin_all(iommu, false);
> + kfree(iommu->external_domain);
> + iommu->external_domain = NULL;
> + }
>
> vfio_iommu_unmap_unpin_all(iommu);
>
> list_for_each_entry_safe(domain, domain_tmp,
> &iommu->domain_list, next) {
> - list_for_each_entry_safe(group, group_tmp,
> - &domain->group_list, next) {
> - iommu_detach_group(domain->domain, group->iommu_group);
> - list_del(&group->next);
> - kfree(group);
> - }
> - iommu_domain_free(domain->domain);
> + vfio_release_domain(domain, false);
> list_del(&domain->next);
> kfree(domain);
> }
> @@ -1158,6 +1618,8 @@ static const struct vfio_iommu_driver_ops vfio_iommu_driver_ops_type1 = {
> .ioctl = vfio_iommu_type1_ioctl,
> .attach_group = vfio_iommu_type1_attach_group,
> .detach_group = vfio_iommu_type1_detach_group,
> + .pin_pages = vfio_iommu_type1_pin_pages,
> + .unpin_pages = vfio_iommu_type1_unpin_pages,
> };
>
> static int __init vfio_iommu_type1_init(void)
[toc] | [prev] | [next] | [standalone]
| From | Jike Song <jike.song@intel.com> |
|---|---|
| Date | 2016-11-08 03:30 +0100 |
| Subject | Re: [PATCH v11 10/22] vfio iommu type1: Add support for mediated devices |
| Message-ID | <sB4kF-3fU-9@gated-at.bofh.it> |
| In reply to | #1516654 |
On 11/08/2016 07:16 AM, Alex Williamson wrote:
> On Sat, 5 Nov 2016 02:40:44 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>
>> VFIO IOMMU drivers are designed for the devices which are IOMMU capable.
>> Mediated device only uses IOMMU APIs, the underlying hardware can be
>> managed by an IOMMU domain.
>>
>> Aim of this change is:
>> - To use most of the code of TYPE1 IOMMU driver for mediated devices
>> - To support direct assigned device and mediated device in single module
>>
>> This change adds pin and unpin support for mediated device to TYPE1 IOMMU
>> backend module. More details:
>> - vfio_pin_pages() callback here uses task and address space of vfio_dma,
>> that is, of the process who mapped that iova range.
>> - Added pfn_list tracking logic to address space structure. All pages
>> pinned through this interface are trached in its address space.
> ^ k
> ------------------------------------------|
>
>> - Pinned pages list is used to verify unpinning request and to unpin
>> remaining pages while detaching the group for that device.
>> - Page accounting is updated to account in its address space where the
>> pages are pinned/unpinned.
>> - Accouting for mdev device is only done if there is no iommu capable
>> domain in the container. When there is a direct device assigned to the
>> container and that domain is iommu capable, all pages are already pinned
>> during DMA_MAP.
>> - Page accouting is updated on hot plug and unplug mdev device and pass
>> through device.
>>
>> Tested by assigning below combinations of devices to a single VM:
>> - GPU pass through only
>> - vGPU device only
>> - One GPU pass through and one vGPU device
>> - Linux VM hot plug and unplug vGPU device while GPU pass through device
>> exist
>> - Linux VM hot plug and unplug GPU pass through device while vGPU device
>> exist
>>
>> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
>> Signed-off-by: Neo Jia <cjia@nvidia.com>
>> Change-Id: I295d6f0f2e0579b8d9882bfd8fd5a4194b97bd9a
>> ---
>> drivers/vfio/vfio_iommu_type1.c | 538 +++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 500 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>> index 8d64528dcc22..e511073446a0 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -36,6 +36,7 @@
>> #include <linux/uaccess.h>
>> #include <linux/vfio.h>
>> #include <linux/workqueue.h>
>> +#include <linux/mdev.h>
>>
>> #define DRIVER_VERSION "0.2"
>> #define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>"
>> @@ -56,6 +57,7 @@ MODULE_PARM_DESC(disable_hugepages,
>> struct vfio_iommu {
>> struct list_head domain_list;
>> struct list_head addr_space_list;
>> + struct vfio_domain *external_domain; /* domain for external user */
>> struct mutex lock;
>> struct rb_root dma_list;
>> bool v2;
>> @@ -67,6 +69,9 @@ struct vfio_addr_space {
>> struct mm_struct *mm;
>> struct list_head next;
>> atomic_t ref_count;
>> + /* external user pinned pfns */
>> + struct rb_root pfn_list; /* pinned Host pfn list */
>> + struct mutex pfn_list_lock; /* mutex for pfn_list */
>> };
>>
>> struct vfio_domain {
>> @@ -83,6 +88,7 @@ struct vfio_dma {
>> unsigned long vaddr; /* Process virtual addr */
>> size_t size; /* Map size (bytes) */
>> int prot; /* IOMMU_READ/WRITE */
>> + bool iommu_mapped;
>> struct vfio_addr_space *addr_space;
>> struct task_struct *task;
>> bool mlock_cap;
>> @@ -94,6 +100,19 @@ struct vfio_group {
>> };
>>
>> /*
>> + * Guest RAM pinning working set or DMA target
>> + */
>> +struct vfio_pfn {
>> + struct rb_node node;
>> + unsigned long pfn; /* Host pfn */
>> + int prot;
>> + atomic_t ref_count;
>> +};
>> +
>> +#define IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu) \
>> + (!list_empty(&iommu->domain_list))
>> +
>> +/*
>> * This code handles mapping and unmapping of user data buffers
>> * into DMA'ble space using the IOMMU
>> */
>> @@ -153,6 +172,93 @@ static struct vfio_addr_space *vfio_find_addr_space(struct vfio_iommu *iommu,
>> return NULL;
>> }
>>
>> +/*
>> + * Helper Functions for host pfn list
>> + */
>> +static struct vfio_pfn *vfio_find_pfn(struct vfio_addr_space *addr_space,
>> + unsigned long pfn)
>> +{
>> + struct vfio_pfn *vpfn;
>> + struct rb_node *node = addr_space->pfn_list.rb_node;
>> +
>> + while (node) {
>> + vpfn = rb_entry(node, struct vfio_pfn, node);
>> +
>> + if (pfn < vpfn->pfn)
>> + node = node->rb_left;
>> + else if (pfn > vpfn->pfn)
>> + node = node->rb_right;
>> + else
>> + return vpfn;
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +static void vfio_link_pfn(struct vfio_addr_space *addr_space,
>> + struct vfio_pfn *new)
>> +{
>> + struct rb_node **link, *parent = NULL;
>> + struct vfio_pfn *vpfn;
>> +
>> + link = &addr_space->pfn_list.rb_node;
>> + while (*link) {
>> + parent = *link;
>> + vpfn = rb_entry(parent, struct vfio_pfn, node);
>> +
>> + if (new->pfn < vpfn->pfn)
>> + link = &(*link)->rb_left;
>> + else
>> + link = &(*link)->rb_right;
>> + }
>> +
>> + rb_link_node(&new->node, parent, link);
>> + rb_insert_color(&new->node, &addr_space->pfn_list);
>> +}
>> +
>> +static void vfio_unlink_pfn(struct vfio_addr_space *addr_space,
>> + struct vfio_pfn *old)
>> +{
>> + rb_erase(&old->node, &addr_space->pfn_list);
>> +}
>> +
>> +static int vfio_add_to_pfn_list(struct vfio_addr_space *addr_space,
>> + unsigned long pfn, int prot)
>> +{
>> + struct vfio_pfn *vpfn;
>> +
>> + vpfn = kzalloc(sizeof(*vpfn), GFP_KERNEL);
>> + if (!vpfn)
>> + return -ENOMEM;
>> +
>> + vpfn->pfn = pfn;
>> + vpfn->prot = prot;
>> + atomic_set(&vpfn->ref_count, 1);
>> + vfio_link_pfn(addr_space, vpfn);
>> + return 0;
>> +}
>> +
>> +static void vfio_remove_from_pfn_list(struct vfio_addr_space *addr_space,
>> + struct vfio_pfn *vpfn)
>> +{
>> + vfio_unlink_pfn(addr_space, vpfn);
>> + kfree(vpfn);
>> +}
>> +
>> +static int vfio_pfn_account(struct vfio_addr_space *addr_space,
>> + unsigned long pfn)
>> +{
>> + struct vfio_pfn *p;
>> + int ret = 1;
>> +
>> + mutex_lock(&addr_space->pfn_list_lock);
>> + p = vfio_find_pfn(addr_space, pfn);
>> + if (p)
>> + ret = 0;
>> + mutex_unlock(&addr_space->pfn_list_lock);
>> + return ret;
>> +}
>> +
>> struct vwork {
>> struct mm_struct *mm;
>> long npage;
>> @@ -304,16 +410,18 @@ static long __vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr,
>> unsigned long limit = task_rlimit(task, RLIMIT_MEMLOCK) >> PAGE_SHIFT;
>> bool lock_cap = dma->mlock_cap;
>> struct mm_struct *mm = dma->addr_space->mm;
>> - long ret, i;
>> + long ret, i, lock_acct;
>> bool rsvd;
>>
>> ret = vaddr_get_pfn(mm, vaddr, prot, pfn_base);
>> if (ret)
>> return ret;
>>
>> + lock_acct = vfio_pfn_account(dma->addr_space, *pfn_base);
>> +
>> rsvd = is_invalid_reserved_pfn(*pfn_base);
>>
>> - if (!rsvd && !lock_cap && mm->locked_vm + 1 > limit) {
>> + if (!rsvd && !lock_cap && mm->locked_vm + lock_acct > limit) {
>> put_pfn(*pfn_base, prot);
>> pr_warn("%s: RLIMIT_MEMLOCK (%ld) exceeded\n", __func__,
>> limit << PAGE_SHIFT);
>> @@ -340,8 +448,10 @@ static long __vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr,
>> break;
>> }
>>
>> + lock_acct += vfio_pfn_account(dma->addr_space, pfn);
>> +
>> if (!rsvd && !lock_cap &&
>> - mm->locked_vm + i + 1 > limit) {
>> + mm->locked_vm + lock_acct + 1 > limit) {
>> put_pfn(pfn, prot);
>> pr_warn("%s: RLIMIT_MEMLOCK (%ld) exceeded\n",
>> __func__, limit << PAGE_SHIFT);
>> @@ -350,7 +460,7 @@ static long __vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr,
>> }
>>
>> if (!rsvd)
>> - vfio_lock_acct(mm, i);
>> + vfio_lock_acct(mm, lock_acct);
>>
>> return i;
>> }
>> @@ -370,14 +480,214 @@ static long __vfio_unpin_pages_remote(struct vfio_dma *dma, unsigned long pfn,
>> return unlocked;
>> }
>>
>> -static void vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma)
>> +static int __vfio_pin_page_external(struct vfio_dma *dma, unsigned long vaddr,
>> + int prot, unsigned long *pfn_base,
>> + bool do_accounting)
>> +{
>> + struct task_struct *task = dma->task;
>> + unsigned long limit = task_rlimit(task, RLIMIT_MEMLOCK) >> PAGE_SHIFT;
>> + bool lock_cap = dma->mlock_cap;
>> + struct mm_struct *mm = dma->addr_space->mm;
>> + int ret;
>> + bool rsvd;
>> +
>> + ret = vaddr_get_pfn(mm, vaddr, prot, pfn_base);
>> + if (ret)
>> + return ret;
>> +
>> + rsvd = is_invalid_reserved_pfn(*pfn_base);
>> +
>> + if (!rsvd && !lock_cap && mm->locked_vm + 1 > limit) {
>> + put_pfn(*pfn_base, prot);
>> + pr_warn("%s: Task %s (%d) RLIMIT_MEMLOCK (%ld) exceeded\n",
>> + __func__, task->comm, task_pid_nr(task),
>> + limit << PAGE_SHIFT);
>> + return -ENOMEM;
>> + }
>> +
>> + if (!rsvd && do_accounting)
>> + vfio_lock_acct(mm, 1);
>> +
>> + return 1;
>> +}
>> +
>> +static void __vfio_unpin_page_external(struct vfio_addr_space *addr_space,
>> + unsigned long pfn, int prot,
>> + bool do_accounting)
>> +{
>> + put_pfn(pfn, prot);
>> +
>> + if (do_accounting)
>> + vfio_lock_acct(addr_space->mm, -1);
>
> Can't we batch this like we do elsewhere? Intel folks, AIUI you intend
> to pin all VM memory through this side channel, have you tested the
> scalability and performance of this with larger VMs? Our vfio_pfn
> data structure alone is 40 bytes per pinned page, which means for
> each 1GB of VM memory, we have 10MBs worth of struct vfio_pfn!
> Additionally, unmapping each 1GB of VM memory will result in 256k
> separate vfio_lock_acct() callbacks. I'm concerned that we're not
> being efficient enough in either space or time.
Hi Alex,
Sorry for being confusing, Intel vGPU actually doesn't necessarily need
to pin all guest memory. A vGPU has its page table (GTT), whose access
is trapped. Whenever guest driver wants to specify a page for DMA, it
writes the GTT entry - thereby we could know the event and pin that
page only.
Performance data will be shared once available. Thanks :)
--
Thanks,
Jike
[toc] | [prev] | [next] | [standalone]
| From | Alex Williamson <alex.williamson@redhat.com> |
|---|---|
| Date | 2016-11-08 17:20 +0100 |
| Subject | Re: [PATCH v11 10/22] vfio iommu type1: Add support for mediated devices |
| Message-ID | <sBhhT-3dr-17@gated-at.bofh.it> |
| In reply to | #1516745 |
On Tue, 08 Nov 2016 10:20:14 +0800
Jike Song <jike.song@intel.com> wrote:
> On 11/08/2016 07:16 AM, Alex Williamson wrote:
> > On Sat, 5 Nov 2016 02:40:44 +0530
> > Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >
> >> VFIO IOMMU drivers are designed for the devices which are IOMMU capable.
> >> Mediated device only uses IOMMU APIs, the underlying hardware can be
> >> managed by an IOMMU domain.
> >>
> >> Aim of this change is:
> >> - To use most of the code of TYPE1 IOMMU driver for mediated devices
> >> - To support direct assigned device and mediated device in single module
> >>
> >> This change adds pin and unpin support for mediated device to TYPE1 IOMMU
> >> backend module. More details:
> >> - vfio_pin_pages() callback here uses task and address space of vfio_dma,
> >> that is, of the process who mapped that iova range.
> >> - Added pfn_list tracking logic to address space structure. All pages
> >> pinned through this interface are trached in its address space.
> > ^ k
> > ------------------------------------------|
> >
> >> - Pinned pages list is used to verify unpinning request and to unpin
> >> remaining pages while detaching the group for that device.
> >> - Page accounting is updated to account in its address space where the
> >> pages are pinned/unpinned.
> >> - Accouting for mdev device is only done if there is no iommu capable
> >> domain in the container. When there is a direct device assigned to the
> >> container and that domain is iommu capable, all pages are already pinned
> >> during DMA_MAP.
> >> - Page accouting is updated on hot plug and unplug mdev device and pass
> >> through device.
> >>
> >> Tested by assigning below combinations of devices to a single VM:
> >> - GPU pass through only
> >> - vGPU device only
> >> - One GPU pass through and one vGPU device
> >> - Linux VM hot plug and unplug vGPU device while GPU pass through device
> >> exist
> >> - Linux VM hot plug and unplug GPU pass through device while vGPU device
> >> exist
> >>
> >> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> >> Signed-off-by: Neo Jia <cjia@nvidia.com>
> >> Change-Id: I295d6f0f2e0579b8d9882bfd8fd5a4194b97bd9a
> >> ---
> >> drivers/vfio/vfio_iommu_type1.c | 538 +++++++++++++++++++++++++++++++++++++---
> >> 1 file changed, 500 insertions(+), 38 deletions(-)
> >>
> >> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> >> index 8d64528dcc22..e511073446a0 100644
> >> --- a/drivers/vfio/vfio_iommu_type1.c
> >> +++ b/drivers/vfio/vfio_iommu_type1.c
> >> @@ -36,6 +36,7 @@
> >> #include <linux/uaccess.h>
> >> #include <linux/vfio.h>
> >> #include <linux/workqueue.h>
> >> +#include <linux/mdev.h>
> >>
> >> #define DRIVER_VERSION "0.2"
> >> #define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>"
> >> @@ -56,6 +57,7 @@ MODULE_PARM_DESC(disable_hugepages,
> >> struct vfio_iommu {
> >> struct list_head domain_list;
> >> struct list_head addr_space_list;
> >> + struct vfio_domain *external_domain; /* domain for external user */
> >> struct mutex lock;
> >> struct rb_root dma_list;
> >> bool v2;
> >> @@ -67,6 +69,9 @@ struct vfio_addr_space {
> >> struct mm_struct *mm;
> >> struct list_head next;
> >> atomic_t ref_count;
> >> + /* external user pinned pfns */
> >> + struct rb_root pfn_list; /* pinned Host pfn list */
> >> + struct mutex pfn_list_lock; /* mutex for pfn_list */
> >> };
> >>
> >> struct vfio_domain {
> >> @@ -83,6 +88,7 @@ struct vfio_dma {
> >> unsigned long vaddr; /* Process virtual addr */
> >> size_t size; /* Map size (bytes) */
> >> int prot; /* IOMMU_READ/WRITE */
> >> + bool iommu_mapped;
> >> struct vfio_addr_space *addr_space;
> >> struct task_struct *task;
> >> bool mlock_cap;
> >> @@ -94,6 +100,19 @@ struct vfio_group {
> >> };
> >>
> >> /*
> >> + * Guest RAM pinning working set or DMA target
> >> + */
> >> +struct vfio_pfn {
> >> + struct rb_node node;
> >> + unsigned long pfn; /* Host pfn */
> >> + int prot;
> >> + atomic_t ref_count;
> >> +};
> >> +
> >> +#define IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu) \
> >> + (!list_empty(&iommu->domain_list))
> >> +
> >> +/*
> >> * This code handles mapping and unmapping of user data buffers
> >> * into DMA'ble space using the IOMMU
> >> */
> >> @@ -153,6 +172,93 @@ static struct vfio_addr_space *vfio_find_addr_space(struct vfio_iommu *iommu,
> >> return NULL;
> >> }
> >>
> >> +/*
> >> + * Helper Functions for host pfn list
> >> + */
> >> +static struct vfio_pfn *vfio_find_pfn(struct vfio_addr_space *addr_space,
> >> + unsigned long pfn)
> >> +{
> >> + struct vfio_pfn *vpfn;
> >> + struct rb_node *node = addr_space->pfn_list.rb_node;
> >> +
> >> + while (node) {
> >> + vpfn = rb_entry(node, struct vfio_pfn, node);
> >> +
> >> + if (pfn < vpfn->pfn)
> >> + node = node->rb_left;
> >> + else if (pfn > vpfn->pfn)
> >> + node = node->rb_right;
> >> + else
> >> + return vpfn;
> >> + }
> >> +
> >> + return NULL;
> >> +}
> >> +
> >> +static void vfio_link_pfn(struct vfio_addr_space *addr_space,
> >> + struct vfio_pfn *new)
> >> +{
> >> + struct rb_node **link, *parent = NULL;
> >> + struct vfio_pfn *vpfn;
> >> +
> >> + link = &addr_space->pfn_list.rb_node;
> >> + while (*link) {
> >> + parent = *link;
> >> + vpfn = rb_entry(parent, struct vfio_pfn, node);
> >> +
> >> + if (new->pfn < vpfn->pfn)
> >> + link = &(*link)->rb_left;
> >> + else
> >> + link = &(*link)->rb_right;
> >> + }
> >> +
> >> + rb_link_node(&new->node, parent, link);
> >> + rb_insert_color(&new->node, &addr_space->pfn_list);
> >> +}
> >> +
> >> +static void vfio_unlink_pfn(struct vfio_addr_space *addr_space,
> >> + struct vfio_pfn *old)
> >> +{
> >> + rb_erase(&old->node, &addr_space->pfn_list);
> >> +}
> >> +
> >> +static int vfio_add_to_pfn_list(struct vfio_addr_space *addr_space,
> >> + unsigned long pfn, int prot)
> >> +{
> >> + struct vfio_pfn *vpfn;
> >> +
> >> + vpfn = kzalloc(sizeof(*vpfn), GFP_KERNEL);
> >> + if (!vpfn)
> >> + return -ENOMEM;
> >> +
> >> + vpfn->pfn = pfn;
> >> + vpfn->prot = prot;
> >> + atomic_set(&vpfn->ref_count, 1);
> >> + vfio_link_pfn(addr_space, vpfn);
> >> + return 0;
> >> +}
> >> +
> >> +static void vfio_remove_from_pfn_list(struct vfio_addr_space *addr_space,
> >> + struct vfio_pfn *vpfn)
> >> +{
> >> + vfio_unlink_pfn(addr_space, vpfn);
> >> + kfree(vpfn);
> >> +}
> >> +
> >> +static int vfio_pfn_account(struct vfio_addr_space *addr_space,
> >> + unsigned long pfn)
> >> +{
> >> + struct vfio_pfn *p;
> >> + int ret = 1;
> >> +
> >> + mutex_lock(&addr_space->pfn_list_lock);
> >> + p = vfio_find_pfn(addr_space, pfn);
> >> + if (p)
> >> + ret = 0;
> >> + mutex_unlock(&addr_space->pfn_list_lock);
> >> + return ret;
> >> +}
> >> +
> >> struct vwork {
> >> struct mm_struct *mm;
> >> long npage;
> >> @@ -304,16 +410,18 @@ static long __vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr,
> >> unsigned long limit = task_rlimit(task, RLIMIT_MEMLOCK) >> PAGE_SHIFT;
> >> bool lock_cap = dma->mlock_cap;
> >> struct mm_struct *mm = dma->addr_space->mm;
> >> - long ret, i;
> >> + long ret, i, lock_acct;
> >> bool rsvd;
> >>
> >> ret = vaddr_get_pfn(mm, vaddr, prot, pfn_base);
> >> if (ret)
> >> return ret;
> >>
> >> + lock_acct = vfio_pfn_account(dma->addr_space, *pfn_base);
> >> +
> >> rsvd = is_invalid_reserved_pfn(*pfn_base);
> >>
> >> - if (!rsvd && !lock_cap && mm->locked_vm + 1 > limit) {
> >> + if (!rsvd && !lock_cap && mm->locked_vm + lock_acct > limit) {
> >> put_pfn(*pfn_base, prot);
> >> pr_warn("%s: RLIMIT_MEMLOCK (%ld) exceeded\n", __func__,
> >> limit << PAGE_SHIFT);
> >> @@ -340,8 +448,10 @@ static long __vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr,
> >> break;
> >> }
> >>
> >> + lock_acct += vfio_pfn_account(dma->addr_space, pfn);
> >> +
> >> if (!rsvd && !lock_cap &&
> >> - mm->locked_vm + i + 1 > limit) {
> >> + mm->locked_vm + lock_acct + 1 > limit) {
> >> put_pfn(pfn, prot);
> >> pr_warn("%s: RLIMIT_MEMLOCK (%ld) exceeded\n",
> >> __func__, limit << PAGE_SHIFT);
> >> @@ -350,7 +460,7 @@ static long __vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr,
> >> }
> >>
> >> if (!rsvd)
> >> - vfio_lock_acct(mm, i);
> >> + vfio_lock_acct(mm, lock_acct);
> >>
> >> return i;
> >> }
> >> @@ -370,14 +480,214 @@ static long __vfio_unpin_pages_remote(struct vfio_dma *dma, unsigned long pfn,
> >> return unlocked;
> >> }
> >>
> >> -static void vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma)
> >> +static int __vfio_pin_page_external(struct vfio_dma *dma, unsigned long vaddr,
> >> + int prot, unsigned long *pfn_base,
> >> + bool do_accounting)
> >> +{
> >> + struct task_struct *task = dma->task;
> >> + unsigned long limit = task_rlimit(task, RLIMIT_MEMLOCK) >> PAGE_SHIFT;
> >> + bool lock_cap = dma->mlock_cap;
> >> + struct mm_struct *mm = dma->addr_space->mm;
> >> + int ret;
> >> + bool rsvd;
> >> +
> >> + ret = vaddr_get_pfn(mm, vaddr, prot, pfn_base);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + rsvd = is_invalid_reserved_pfn(*pfn_base);
> >> +
> >> + if (!rsvd && !lock_cap && mm->locked_vm + 1 > limit) {
> >> + put_pfn(*pfn_base, prot);
> >> + pr_warn("%s: Task %s (%d) RLIMIT_MEMLOCK (%ld) exceeded\n",
> >> + __func__, task->comm, task_pid_nr(task),
> >> + limit << PAGE_SHIFT);
> >> + return -ENOMEM;
> >> + }
> >> +
> >> + if (!rsvd && do_accounting)
> >> + vfio_lock_acct(mm, 1);
> >> +
> >> + return 1;
> >> +}
> >> +
> >> +static void __vfio_unpin_page_external(struct vfio_addr_space *addr_space,
> >> + unsigned long pfn, int prot,
> >> + bool do_accounting)
> >> +{
> >> + put_pfn(pfn, prot);
> >> +
> >> + if (do_accounting)
> >> + vfio_lock_acct(addr_space->mm, -1);
> >
> > Can't we batch this like we do elsewhere? Intel folks, AIUI you intend
> > to pin all VM memory through this side channel, have you tested the
> > scalability and performance of this with larger VMs? Our vfio_pfn
> > data structure alone is 40 bytes per pinned page, which means for
> > each 1GB of VM memory, we have 10MBs worth of struct vfio_pfn!
> > Additionally, unmapping each 1GB of VM memory will result in 256k
> > separate vfio_lock_acct() callbacks. I'm concerned that we're not
> > being efficient enough in either space or time.
>
> Hi Alex,
>
> Sorry for being confusing, Intel vGPU actually doesn't necessarily need
> to pin all guest memory. A vGPU has its page table (GTT), whose access
> is trapped. Whenever guest driver wants to specify a page for DMA, it
> writes the GTT entry - thereby we could know the event and pin that
> page only.
>
> Performance data will be shared once available. Thanks :)
Ok, so maybe if none of the initial users are mapping full VM memory
then scalability can be saved for future optimization. Thanks,
Alex
[toc] | [prev] | [next] | [standalone]
| From | Kirti Wankhede <kwankhede@nvidia.com> |
|---|---|
| Date | 2016-11-08 16:10 +0100 |
| Subject | Re: [PATCH v11 10/22] vfio iommu type1: Add support for mediated devices |
| Message-ID | <sBgc9-2wv-11@gated-at.bofh.it> |
| In reply to | #1516654 |
On 11/8/2016 4:46 AM, Alex Williamson wrote:
> On Sat, 5 Nov 2016 02:40:44 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>
...
>> -static void vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma)
>> +static int __vfio_pin_page_external(struct vfio_dma *dma, unsigned long vaddr,
>> + int prot, unsigned long *pfn_base,
>> + bool do_accounting)
>> +{
>> + struct task_struct *task = dma->task;
>> + unsigned long limit = task_rlimit(task, RLIMIT_MEMLOCK) >> PAGE_SHIFT;
>> + bool lock_cap = dma->mlock_cap;
>> + struct mm_struct *mm = dma->addr_space->mm;
>> + int ret;
>> + bool rsvd;
>> +
>> + ret = vaddr_get_pfn(mm, vaddr, prot, pfn_base);
>> + if (ret)
>> + return ret;
>> +
>> + rsvd = is_invalid_reserved_pfn(*pfn_base);
>> +
>> + if (!rsvd && !lock_cap && mm->locked_vm + 1 > limit) {
>> + put_pfn(*pfn_base, prot);
>> + pr_warn("%s: Task %s (%d) RLIMIT_MEMLOCK (%ld) exceeded\n",
>> + __func__, task->comm, task_pid_nr(task),
>> + limit << PAGE_SHIFT);
>> + return -ENOMEM;
>> + }
>> +
>> + if (!rsvd && do_accounting)
>> + vfio_lock_acct(mm, 1);
>> +
>> + return 1;
>> +}
>> +
>> +static void __vfio_unpin_page_external(struct vfio_addr_space *addr_space,
>> + unsigned long pfn, int prot,
>> + bool do_accounting)
>> +{
>> + put_pfn(pfn, prot);
>> +
>> + if (do_accounting)
>> + vfio_lock_acct(addr_space->mm, -1);
>
> Can't we batch this like we do elsewhere? Intel folks, AIUI you intend
> to pin all VM memory through this side channel, have you tested the
> scalability and performance of this with larger VMs? Our vfio_pfn
> data structure alone is 40 bytes per pinned page, which means for
> each 1GB of VM memory, we have 10MBs worth of struct vfio_pfn!
> Additionally, unmapping each 1GB of VM memory will result in 256k
> separate vfio_lock_acct() callbacks. I'm concerned that we're not
> being efficient enough in either space or time.
>
> One thought might be whether we really need to save the pfn, we better
> always get the same result if we pin it again, or maybe we can just do
> a lookup through the mm at that point without re-pinning. Could we get
> to the point where we only need an atomic_t ref count per page in a
> linear array relative to the IOVA?
Ok. Is System RAM hot-plug supported? How is system RAM hot-plug
handled? Are there DMA_MAP calls on such hot-plug for additional range?
If we have a linear array/memory, we will have to realloc it on memory
hot-plug?
> That would give us 1MB per 1GB
> overhead. The semantics of the pin and unpin would make more sense then
> too, both would take an IOVA range, only pinning would need a return
> mechanism. For instance:
>
> int pin_pages(void *iommu_data, dma_addr_t iova_base,
> int npage, unsigned long *pfn_base);
>
> This would pin physically contiguous pages up to npage, returning the
> base pfn and returning the number of pages pinned (<= npage). The
> vendor driver would make multiple calls to fill the necessary range.
With the current patch, input is user_pfn[] array and npages.
int vfio_pin_pages(struct device *dev, unsigned long *user_pfn,
int npage, int prot, unsigned long *phys_pfn)
When guest allocates memory with malloc(), gfns would not be contiguous,
right? These gfns (user_pfns) are passed as argument here.
Is there any case where we could get pin/unpin request for contiguous pages?
> Unpin would then simply be:
>
> void unpin_pages(void *iommu_data, dma_addr_t iova_base, int npage);
>
> Hugepage usage would really make such an interface shine (ie. 2MB+
> contiguous ranges). A downside would be the overhead of getting the
> group and container reference in vfio for each callback, perhaps we'd
> need to figure out how the vendor driver could hold that reference.
In very initial phases of proposal, I had suggested to keep pointer to
container->iommu_data in struct mdev_device. But that was discarded.
> The current API of passing around pfn arrays, further increasing the
> overhead of the whole ecosystem just makes me cringe though.
>
...
>> + if (ret <= 0) {
>> + WARN_ON(!ret);
>> + goto pin_unwind;
>> + }
>> +
>> + mutex_lock(&dma->addr_space->pfn_list_lock);
>> +
>> + /* search if pfn exist */
>> + p = vfio_find_pfn(dma->addr_space, pfn[i]);
>> + if (p) {
>> + atomic_inc(&p->ref_count);
>
> We never test whether (p->prot == prot), shouldn't we be doing
> something in that case? In fact, why do we allow the side-channel
> through the .{un}pin_pages to specify page protection flags that might
> be different than the user specified for the DMA_MAP? If the user
> specified read-only, the vendor driver should not be allowed to
> override with read-write access.
>
If user specified protection flags for DMA_MAP could be
prot = IOMMU_WRITE | IOMMU_READ;
But vendor driver can request to pin page to be readonly, i.e.
IOMMU_READ. In that case, pin pages should be allowed, right?
Then the check should be if (p->prot & prot).
Thanks,
Kirti
[toc] | [prev] | [next] | [standalone]
| From | Alex Williamson <alex.williamson@redhat.com> |
|---|---|
| Date | 2016-11-08 18:10 +0100 |
| Subject | Re: [PATCH v11 10/22] vfio iommu type1: Add support for mediated devices |
| Message-ID | <sBi4i-3IU-5@gated-at.bofh.it> |
| In reply to | #1517273 |
On Tue, 8 Nov 2016 20:36:34 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:
> On 11/8/2016 4:46 AM, Alex Williamson wrote:
> > On Sat, 5 Nov 2016 02:40:44 +0530
> > Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >
> ...
>
> >> -static void vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma)
> >> +static int __vfio_pin_page_external(struct vfio_dma *dma, unsigned long vaddr,
> >> + int prot, unsigned long *pfn_base,
> >> + bool do_accounting)
> >> +{
> >> + struct task_struct *task = dma->task;
> >> + unsigned long limit = task_rlimit(task, RLIMIT_MEMLOCK) >> PAGE_SHIFT;
> >> + bool lock_cap = dma->mlock_cap;
> >> + struct mm_struct *mm = dma->addr_space->mm;
> >> + int ret;
> >> + bool rsvd;
> >> +
> >> + ret = vaddr_get_pfn(mm, vaddr, prot, pfn_base);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + rsvd = is_invalid_reserved_pfn(*pfn_base);
> >> +
> >> + if (!rsvd && !lock_cap && mm->locked_vm + 1 > limit) {
> >> + put_pfn(*pfn_base, prot);
> >> + pr_warn("%s: Task %s (%d) RLIMIT_MEMLOCK (%ld) exceeded\n",
> >> + __func__, task->comm, task_pid_nr(task),
> >> + limit << PAGE_SHIFT);
> >> + return -ENOMEM;
> >> + }
> >> +
> >> + if (!rsvd && do_accounting)
> >> + vfio_lock_acct(mm, 1);
> >> +
> >> + return 1;
> >> +}
> >> +
> >> +static void __vfio_unpin_page_external(struct vfio_addr_space *addr_space,
> >> + unsigned long pfn, int prot,
> >> + bool do_accounting)
> >> +{
> >> + put_pfn(pfn, prot);
> >> +
> >> + if (do_accounting)
> >> + vfio_lock_acct(addr_space->mm, -1);
> >
> > Can't we batch this like we do elsewhere? Intel folks, AIUI you intend
> > to pin all VM memory through this side channel, have you tested the
> > scalability and performance of this with larger VMs? Our vfio_pfn
> > data structure alone is 40 bytes per pinned page, which means for
> > each 1GB of VM memory, we have 10MBs worth of struct vfio_pfn!
> > Additionally, unmapping each 1GB of VM memory will result in 256k
> > separate vfio_lock_acct() callbacks. I'm concerned that we're not
> > being efficient enough in either space or time.
> >
> > One thought might be whether we really need to save the pfn, we better
> > always get the same result if we pin it again, or maybe we can just do
> > a lookup through the mm at that point without re-pinning. Could we get
> > to the point where we only need an atomic_t ref count per page in a
> > linear array relative to the IOVA?
>
> Ok. Is System RAM hot-plug supported? How is system RAM hot-plug
> handled? Are there DMA_MAP calls on such hot-plug for additional range?
> If we have a linear array/memory, we will have to realloc it on memory
> hot-plug?
I was thinking a linear array for each IOVA page within a vfio_dma.
The array would track the number of references (pins) of each page. It
might actually need to be a page table given that a single vfio_dma can
nearly map the entire 64bit address space. I don't think RAM hotplug
is a factor here, we need to support and properly account for multiple
IOVAs mapping to the same pfn, but the typical case will be a 1:1
mapping, I think that's what we'd optimize for.
> > That would give us 1MB per 1GB
> > overhead. The semantics of the pin and unpin would make more sense then
> > too, both would take an IOVA range, only pinning would need a return
> > mechanism. For instance:
> >
> > int pin_pages(void *iommu_data, dma_addr_t iova_base,
> > int npage, unsigned long *pfn_base);
> >
> > This would pin physically contiguous pages up to npage, returning the
> > base pfn and returning the number of pages pinned (<= npage). The
> > vendor driver would make multiple calls to fill the necessary range.
>
>
> With the current patch, input is user_pfn[] array and npages.
>
> int vfio_pin_pages(struct device *dev, unsigned long *user_pfn,
> int npage, int prot, unsigned long *phys_pfn)
>
>
> When guest allocates memory with malloc(), gfns would not be contiguous,
> right? These gfns (user_pfns) are passed as argument here.
> Is there any case where we could get pin/unpin request for contiguous pages?
It would depend on whether the user within the guest is actually
optimizing for hugepages.
> > Unpin would then simply be:
> >
> > void unpin_pages(void *iommu_data, dma_addr_t iova_base, int npage);
> >
> > Hugepage usage would really make such an interface shine (ie. 2MB+
> > contiguous ranges). A downside would be the overhead of getting the
> > group and container reference in vfio for each callback, perhaps we'd
> > need to figure out how the vendor driver could hold that reference.
>
> In very initial phases of proposal, I had suggested to keep pointer to
> container->iommu_data in struct mdev_device. But that was discarded.
The referencing is definitely tricky, we run into the same problem that
Alexey had in trying to do that where holding a reference to the
container doesn't actually keep the container. The user can tear down
the container or remove groups from the container, which automatically
removes the iommu backend.
> > The current API of passing around pfn arrays, further increasing the
> > overhead of the whole ecosystem just makes me cringe though.
> >
>
> ...
>
> >> + if (ret <= 0) {
> >> + WARN_ON(!ret);
> >> + goto pin_unwind;
> >> + }
> >> +
> >> + mutex_lock(&dma->addr_space->pfn_list_lock);
> >> +
> >> + /* search if pfn exist */
> >> + p = vfio_find_pfn(dma->addr_space, pfn[i]);
> >> + if (p) {
> >> + atomic_inc(&p->ref_count);
> >
> > We never test whether (p->prot == prot), shouldn't we be doing
> > something in that case? In fact, why do we allow the side-channel
> > through the .{un}pin_pages to specify page protection flags that might
> > be different than the user specified for the DMA_MAP? If the user
> > specified read-only, the vendor driver should not be allowed to
> > override with read-write access.
> >
>
> If user specified protection flags for DMA_MAP could be
> prot = IOMMU_WRITE | IOMMU_READ;
>
> But vendor driver can request to pin page to be readonly, i.e.
> IOMMU_READ. In that case, pin pages should be allowed, right?
>
> Then the check should be if (p->prot & prot).
That doesn't solve the problem, we might have an existing pfn with
prot R and we're asking for RW, (R & RW) is true, but that's not what
we asked for. I think the solution would be to test the vendor driver
requested prot against vfio_dma.prot, but always pin the pages using
the vfio_dma.prot flags. Thus if the vendor driver asks for read-only
and the vfio_dma.prot is read-write, these are compatible and the page
is pinned read-write since that's the user mapping. Otherwise you need
to save prot per page (which you already do), but you're also going to
need to handle promoting pages if we have multiple vendors asking for
different prot attributes. Thanks,
Alex
[toc] | [prev] | [next] | [standalone]
| From | Alexey Kardashevskiy <aik@ozlabs.ru> |
|---|---|
| Date | 2016-11-08 08:00 +0100 |
| Subject | Re: [PATCH v11 10/22] vfio iommu type1: Add support for mediated devices |
| Message-ID | <sB8xX-5U7-11@gated-at.bofh.it> |
| In reply to | #1515428 |
On 05/11/16 08:10, Kirti Wankhede wrote: > VFIO IOMMU drivers are designed for the devices which are IOMMU capable. > Mediated device only uses IOMMU APIs, the underlying hardware can be > managed by an IOMMU domain. > > Aim of this change is: > - To use most of the code of TYPE1 IOMMU driver for mediated devices > - To support direct assigned device and mediated device in single module > > This change adds pin and unpin support for mediated device to TYPE1 IOMMU > backend module. More details: > - vfio_pin_pages() callback here uses task and address space of vfio_dma, > that is, of the process who mapped that iova range. > - Added pfn_list tracking logic to address space structure. All pages > pinned through this interface are trached in its address space. > - Pinned pages list is used to verify unpinning request and to unpin > remaining pages while detaching the group for that device. > - Page accounting is updated to account in its address space where the > pages are pinned/unpinned. > - Accouting for mdev device is only done if there is no iommu capable > domain in the container. When there is a direct device assigned to the > container and that domain is iommu capable, all pages are already pinned > during DMA_MAP. > - Page accouting is updated on hot plug and unplug mdev device and pass > through device. > > Tested by assigning below combinations of devices to a single VM: > - GPU pass through only This does not require this patchset, right? > - vGPU device only Out of curiosity - how exactly did you test this? The exact GPU, how to create vGPU, what was the QEMU command line and the guest does with this passed device? Thanks. -- Alexey
[toc] | [prev] | [next] | [standalone]
| From | Kirti Wankhede <kwankhede@nvidia.com> |
|---|---|
| Date | 2016-11-04 22:20 +0100 |
| Subject | [PATCH v11 22/22] MAINTAINERS: Add entry VFIO based Mediated device drivers |
| Message-ID | <szU42-71A-55@gated-at.bofh.it> |
| In reply to | #1515412 |
Adding myself as a maintainer of mediated device framework, a sub module of VFIO. Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com> Signed-off-by: Neo Jia <cjia@nvidia.com> Change-Id: I58f6717783e0d4008ca31f4a5c4494696bae8571 --- MAINTAINERS | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index f30b8ea700fd..a3165b6407a5 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12729,6 +12729,15 @@ F: drivers/vfio/ F: include/linux/vfio.h F: include/uapi/linux/vfio.h +VFIO MEDIATED DEVICE DRIVERS +M: Kirti Wankhede <kwankhede@nvidia.com> +L: kvm@vger.kernel.org +S: Maintained +F: Documentation/vfio-mediated-device.txt +F: drivers/vfio/mdev/ +F: include/linux/mdev.h +F: samples/vfio-mdev/ + VFIO PLATFORM DRIVER M: Baptiste Reynal <b.reynal@virtualopensystems.com> L: kvm@vger.kernel.org -- 2.7.0
[toc] | [prev] | [next] | [standalone]
| From | Kirti Wankhede <kwankhede@nvidia.com> |
|---|---|
| Date | 2016-11-04 22:20 +0100 |
| Subject | [PATCH v11 08/22] vfio iommu type1: Add find_iommu_group() function |
| Message-ID | <szU42-71A-51@gated-at.bofh.it> |
| In reply to | #1515412 |
Add find_iommu_group()
Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
Signed-off-by: Neo Jia <cjia@nvidia.com>
Change-Id: I9d372f1ebe9eb01a5a21374b8a2b03f7df73601f
---
drivers/vfio/vfio_iommu_type1.c | 58 ++++++++++++++++++++++++-----------------
1 file changed, 34 insertions(+), 24 deletions(-)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 653386e80e85..422c8d198abb 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -748,11 +748,24 @@ static void vfio_test_domain_fgsp(struct vfio_domain *domain)
__free_pages(pages, order);
}
+static struct vfio_group *find_iommu_group(struct vfio_domain *domain,
+ struct iommu_group *iommu_group)
+{
+ struct vfio_group *g;
+
+ list_for_each_entry(g, &domain->group_list, next) {
+ if (g->iommu_group == iommu_group)
+ return g;
+ }
+
+ return NULL;
+}
+
static int vfio_iommu_type1_attach_group(void *iommu_data,
struct iommu_group *iommu_group)
{
struct vfio_iommu *iommu = iommu_data;
- struct vfio_group *group, *g;
+ struct vfio_group *group;
struct vfio_domain *domain, *d;
struct bus_type *bus = NULL;
int ret;
@@ -760,10 +773,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
mutex_lock(&iommu->lock);
list_for_each_entry(d, &iommu->domain_list, next) {
- list_for_each_entry(g, &d->group_list, next) {
- if (g->iommu_group != iommu_group)
- continue;
-
+ if (find_iommu_group(d, iommu_group)) {
mutex_unlock(&iommu->lock);
return -EINVAL;
}
@@ -882,28 +892,28 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
mutex_lock(&iommu->lock);
+
list_for_each_entry(domain, &iommu->domain_list, next) {
- list_for_each_entry(group, &domain->group_list, next) {
- if (group->iommu_group != iommu_group)
- continue;
+ group = find_iommu_group(domain, iommu_group);
+ if (!group)
+ continue;
- iommu_detach_group(domain->domain, iommu_group);
- list_del(&group->next);
- kfree(group);
- /*
- * Group ownership provides privilege, if the group
- * list is empty, the domain goes away. If it's the
- * last domain, then all the mappings go away too.
- */
- if (list_empty(&domain->group_list)) {
- if (list_is_singular(&iommu->domain_list))
- vfio_iommu_unmap_unpin_all(iommu);
- iommu_domain_free(domain->domain);
- list_del(&domain->next);
- kfree(domain);
- }
- goto done;
+ iommu_detach_group(domain->domain, iommu_group);
+ list_del(&group->next);
+ kfree(group);
+ /*
+ * Group ownership provides privilege, if the group
+ * list is empty, the domain goes away. If it's the
+ * last domain, then all the mappings go away too.
+ */
+ if (list_empty(&domain->group_list)) {
+ if (list_is_singular(&iommu->domain_list))
+ vfio_iommu_unmap_unpin_all(iommu);
+ iommu_domain_free(domain->domain);
+ list_del(&domain->next);
+ kfree(domain);
}
+ goto done;
}
done:
--
2.7.0
[toc] | [prev] | [next] | [standalone]
Page 2 of 4 — ← Prev page 1 [2] 3 4 Next page →
Back to top | Article view | linux.kernel
csiph-web