* [PATCH Kernel v18 0/7] KABIs to support migration for VFIO devices
@ 2020-05-04 15:58 ` Kirti Wankhede
0 siblings, 0 replies; 39+ messages in thread
From: Kirti Wankhede @ 2020-05-04 15:58 UTC (permalink / raw)
To: alex.williamson, cjia
Cc: Zhengxiao.zx, kevin.tian, yi.l.liu, yan.y.zhao, kvm, eskultet,
ziye.yang, qemu-devel, cohuck, shuangtai.tst, dgilbert,
zhi.a.wang, mlevitsk, pasic, aik, Kirti Wankhede, eauger, felipe,
jonathan.davies, changpeng.liu, Ken.Xue
Hi,
This patch set adds:
* IOCTL VFIO_IOMMU_DIRTY_PAGES to get dirty pages bitmap with
respect to IOMMU container rather than per device. All pages pinned by
vendor driver through vfio_pin_pages external API has to be marked as
dirty during migration. When IOMMU capable device is present in the
container and all pages are pinned and mapped, then all pages are marked
dirty.
When there are CPU writes, CPU dirty page tracking can identify dirtied
pages, but any page pinned by vendor driver can also be written by
device. As of now there is no device which has hardware support for
dirty page tracking. So all pages which are pinned should be considered
as dirty.
This ioctl is also used to start/stop dirty pages tracking for pinned and
unpinned pages while migration is active.
* Updated IOCTL VFIO_IOMMU_UNMAP_DMA to get dirty pages bitmap before
unmapping IO virtual address range.
With vIOMMU, during pre-copy phase of migration, while CPUs are still
running, IO virtual address unmap can happen while device still keeping
reference of guest pfns. Those pages should be reported as dirty before
unmap, so that VFIO user space application can copy content of those
pages from source to destination.
* Patch 7 detect if IOMMU capable device driver is smart to report pages
to be marked dirty by pinning pages using vfio_pin_pages() API.
Yet TODO:
Since there is no device which has hardware support for system memmory
dirty bitmap tracking, right now there is no other API from vendor driver
to VFIO IOMMU module to report dirty pages. In future, when such hardware
support will be implemented, an API will be required such that vendor
driver could report dirty pages to VFIO module during migration phases.
Adding revision history from previous QEMU patch set to understand KABI
changes done till now
v17 -> v18
- Add migration capability to the capability chain for VFIO_IOMMU_GET_INFO
ioctl
- Updated UMAP_DMA ioctl to return bitmap of multiple vfio_dma
v16 -> v17
- Fixed errors reported by kbuild test robot <lkp@intel.com> on i386
v15 -> v16
- Minor edits and nit picks (Auger Eric)
- On copying bitmap to user, re-populated bitmap only for pinned pages,
excluding unmapped pages and CPU dirtied pages.
- Patches are on tag: next-20200318 and 1-3 patches from Yan's series
https://lkml.org/lkml/2020/3/12/1255
v14 -> v15
- Minor edits and nit picks.
- In the verification of user allocated bitmap memory, added check of
maximum size.
- Patches are on tag: next-20200318 and 1-3 patches from Yan's series
https://lkml.org/lkml/2020/3/12/1255
v13 -> v14
- Added struct vfio_bitmap to kabi. updated structure
vfio_iommu_type1_dirty_bitmap_get and vfio_iommu_type1_dma_unmap.
- All small changes suggested by Alex.
- Patches are on tag: next-20200318 and 1-3 patches from Yan's series
https://lkml.org/lkml/2020/3/12/1255
v12 -> v13
- Changed bitmap allocation in vfio_iommu_type1 to per vfio_dma
- Changed VFIO_IOMMU_DIRTY_PAGES ioctl behaviour to be per vfio_dma range.
- Changed vfio_iommu_type1_dirty_bitmap structure to have separate data
field.
v11 -> v12
- Changed bitmap allocation in vfio_iommu_type1.
- Remove atomicity of ref_count.
- Updated comments for migration device state structure about error
reporting.
- Nit picks from v11 reviews
v10 -> v11
- Fix pin pages API to free vpfn if it is marked as unpinned tracking page.
- Added proposal to detect if IOMMU capable device calls external pin pages
API to mark pages dirty.
- Nit picks from v10 reviews
v9 -> v10:
- Updated existing VFIO_IOMMU_UNMAP_DMA ioctl to get dirty pages bitmap
during unmap while migration is active
- Added flag in VFIO_IOMMU_GET_INFO to indicate driver support dirty page
tracking.
- If iommu_mapped, mark all pages dirty.
- Added unpinned pages tracking while migration is active.
- Updated comments for migration device state structure with bit
combination table and state transition details.
v8 -> v9:
- Split patch set in 2 sets, Kernel and QEMU.
- Dirty pages bitmap is queried from IOMMU container rather than from
vendor driver for per device. Added 2 ioctls to achieve this.
v7 -> v8:
- Updated comments for KABI
- Added BAR address validation check during PCI device's config space load
as suggested by Dr. David Alan Gilbert.
- Changed vfio_migration_set_state() to set or clear device state flags.
- Some nit fixes.
v6 -> v7:
- Fix build failures.
v5 -> v6:
- Fix build failure.
v4 -> v5:
- Added decriptive comment about the sequence of access of members of
structure vfio_device_migration_info to be followed based on Alex's
suggestion
- Updated get dirty pages sequence.
- As per Cornelia Huck's suggestion, added callbacks to VFIODeviceOps to
get_object, save_config and load_config.
- Fixed multiple nit picks.
- Tested live migration with multiple vfio device assigned to a VM.
v3 -> v4:
- Added one more bit for _RESUMING flag to be set explicitly.
- data_offset field is read-only for user space application.
- data_size is read for every iteration before reading data from migration,
that is removed assumption that data will be till end of migration
region.
- If vendor driver supports mappable sparsed region, map those region
during setup state of save/load, similarly unmap those from cleanup
routines.
- Handles race condition that causes data corruption in migration region
during save device state by adding mutex and serialiaing save_buffer and
get_dirty_pages routines.
- Skip called get_dirty_pages routine for mapped MMIO region of device.
- Added trace events.
- Split into multiple functional patches.
v2 -> v3:
- Removed enum of VFIO device states. Defined VFIO device state with 2
bits.
- Re-structured vfio_device_migration_info to keep it minimal and defined
action on read and write access on its members.
v1 -> v2:
- Defined MIGRATION region type and sub-type which should be used with
region type capability.
- Re-structured vfio_device_migration_info. This structure will be placed
at 0th offset of migration region.
- Replaced ioctl with read/write for trapped part of migration region.
- Added both type of access support, trapped or mmapped, for data section
of the region.
- Moved PCI device functions to pci file.
- Added iteration to get dirty page bitmap until bitmap for all requested
pages are copied.
Thanks,
Kirti
Kirti Wankhede (7):
vfio: UAPI for migration interface for device state
vfio iommu: Remove atomicity of ref_count of pinned pages
vfio iommu: Add ioctl definition for dirty pages tracking.
vfio iommu: Implementation of ioctl for dirty pages tracking.
vfio iommu: Update UNMAP_DMA ioctl to get dirty bitmap before unmap
vfio iommu: Add migration capability to report supported features
vfio: Selective dirty page tracking if IOMMU backed device pins pages
drivers/vfio/vfio.c | 13 +-
drivers/vfio/vfio_iommu_type1.c | 464 ++++++++++++++++++++++++++++++++++++++--
include/linux/vfio.h | 4 +-
include/uapi/linux/vfio.h | 308 ++++++++++++++++++++++++++
4 files changed, 770 insertions(+), 19 deletions(-)
--
2.7.0
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH Kernel v18 1/7] vfio: UAPI for migration interface for device state
2020-05-04 15:58 ` Kirti Wankhede
@ 2020-05-04 15:58 ` Kirti Wankhede
-1 siblings, 0 replies; 39+ messages in thread
From: Kirti Wankhede @ 2020-05-04 15:58 UTC (permalink / raw)
To: alex.williamson, cjia
Cc: kevin.tian, ziye.yang, changpeng.liu, yi.l.liu, mlevitsk,
eskultet, cohuck, dgilbert, jonathan.davies, eauger, aik, pasic,
felipe, Zhengxiao.zx, shuangtai.tst, Ken.Xue, zhi.a.wang,
yan.y.zhao, qemu-devel, kvm, Kirti Wankhede
- Defined MIGRATION region type and sub-type.
- Defined vfio_device_migration_info structure which will be placed at the
0th offset of migration region to get/set VFIO device related
information. Defined members of structure and usage on read/write access.
- Defined device states and state transition details.
- Defined sequence to be followed while saving and resuming VFIO device.
Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
Reviewed-by: Neo Jia <cjia@nvidia.com>
---
include/uapi/linux/vfio.h | 228 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 228 insertions(+)
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 015516bcfaa3..ad9bb5af3463 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -305,6 +305,7 @@ struct vfio_region_info_cap_type {
#define VFIO_REGION_TYPE_PCI_VENDOR_MASK (0xffff)
#define VFIO_REGION_TYPE_GFX (1)
#define VFIO_REGION_TYPE_CCW (2)
+#define VFIO_REGION_TYPE_MIGRATION (3)
/* sub-types for VFIO_REGION_TYPE_PCI_* */
@@ -379,6 +380,233 @@ struct vfio_region_gfx_edid {
/* sub-types for VFIO_REGION_TYPE_CCW */
#define VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD (1)
+/* sub-types for VFIO_REGION_TYPE_MIGRATION */
+#define VFIO_REGION_SUBTYPE_MIGRATION (1)
+
+/*
+ * The structure vfio_device_migration_info is placed at the 0th offset of
+ * the VFIO_REGION_SUBTYPE_MIGRATION region to get and set VFIO device related
+ * migration information. Field accesses from this structure are only supported
+ * at their native width and alignment. Otherwise, the result is undefined and
+ * vendor drivers should return an error.
+ *
+ * device_state: (read/write)
+ * - The user application writes to this field to inform the vendor driver
+ * about the device state to be transitioned to.
+ * - The vendor driver should take the necessary actions to change the
+ * device state. After successful transition to a given state, the
+ * vendor driver should return success on write(device_state, state)
+ * system call. If the device state transition fails, the vendor driver
+ * should return an appropriate -errno for the fault condition.
+ * - On the user application side, if the device state transition fails,
+ * that is, if write(device_state, state) returns an error, read
+ * device_state again to determine the current state of the device from
+ * the vendor driver.
+ * - The vendor driver should return previous state of the device unless
+ * the vendor driver has encountered an internal error, in which case
+ * the vendor driver may report the device_state VFIO_DEVICE_STATE_ERROR.
+ * - The user application must use the device reset ioctl to recover the
+ * device from VFIO_DEVICE_STATE_ERROR state. If the device is
+ * indicated to be in a valid device state by reading device_state, the
+ * user application may attempt to transition the device to any valid
+ * state reachable from the current state or terminate itself.
+ *
+ * device_state consists of 3 bits:
+ * - If bit 0 is set, it indicates the _RUNNING state. If bit 0 is clear,
+ * it indicates the _STOP state. When the device state is changed to
+ * _STOP, driver should stop the device before write() returns.
+ * - If bit 1 is set, it indicates the _SAVING state, which means that the
+ * driver should start gathering device state information that will be
+ * provided to the VFIO user application to save the device's state.
+ * - If bit 2 is set, it indicates the _RESUMING state, which means that
+ * the driver should prepare to resume the device. Data provided through
+ * the migration region should be used to resume the device.
+ * Bits 3 - 31 are reserved for future use. To preserve them, the user
+ * application should perform a read-modify-write operation on this
+ * field when modifying the specified bits.
+ *
+ * +------- _RESUMING
+ * |+------ _SAVING
+ * ||+----- _RUNNING
+ * |||
+ * 000b => Device Stopped, not saving or resuming
+ * 001b => Device running, which is the default state
+ * 010b => Stop the device & save the device state, stop-and-copy state
+ * 011b => Device running and save the device state, pre-copy state
+ * 100b => Device stopped and the device state is resuming
+ * 101b => Invalid state
+ * 110b => Error state
+ * 111b => Invalid state
+ *
+ * State transitions:
+ *
+ * _RESUMING _RUNNING Pre-copy Stop-and-copy _STOP
+ * (100b) (001b) (011b) (010b) (000b)
+ * 0. Running or default state
+ * |
+ *
+ * 1. Normal Shutdown (optional)
+ * |------------------------------------->|
+ *
+ * 2. Save the state or suspend
+ * |------------------------->|---------->|
+ *
+ * 3. Save the state during live migration
+ * |----------->|------------>|---------->|
+ *
+ * 4. Resuming
+ * |<---------|
+ *
+ * 5. Resumed
+ * |--------->|
+ *
+ * 0. Default state of VFIO device is _RUNNNG when the user application starts.
+ * 1. During normal shutdown of the user application, the user application may
+ * optionally change the VFIO device state from _RUNNING to _STOP. This
+ * transition is optional. The vendor driver must support this transition but
+ * must not require it.
+ * 2. When the user application saves state or suspends the application, the
+ * device state transitions from _RUNNING to stop-and-copy and then to _STOP.
+ * On state transition from _RUNNING to stop-and-copy, driver must stop the
+ * device, save the device state and send it to the application through the
+ * migration region. The sequence to be followed for such transition is given
+ * below.
+ * 3. In live migration of user application, the state transitions from _RUNNING
+ * to pre-copy, to stop-and-copy, and to _STOP.
+ * On state transition from _RUNNING to pre-copy, the driver should start
+ * gathering the device state while the application is still running and send
+ * the device state data to application through the migration region.
+ * On state transition from pre-copy to stop-and-copy, the driver must stop
+ * the device, save the device state and send it to the user application
+ * through the migration region.
+ * Vendor drivers must support the pre-copy state even for implementations
+ * where no data is provided to the user before the stop-and-copy state. The
+ * user must not be required to consume all migration data before the device
+ * transitions to a new state, including the stop-and-copy state.
+ * The sequence to be followed for above two transitions is given below.
+ * 4. To start the resuming phase, the device state should be transitioned from
+ * the _RUNNING to the _RESUMING state.
+ * In the _RESUMING state, the driver should use the device state data
+ * received through the migration region to resume the device.
+ * 5. After providing saved device data to the driver, the application should
+ * change the state from _RESUMING to _RUNNING.
+ *
+ * reserved:
+ * Reads on this field return zero and writes are ignored.
+ *
+ * pending_bytes: (read only)
+ * The number of pending bytes still to be migrated from the vendor driver.
+ *
+ * data_offset: (read only)
+ * The user application should read data_offset field from the migration
+ * region. The user application should read the device data from this
+ * offset within the migration region during the _SAVING state or write
+ * the device data during the _RESUMING state. See below for details of
+ * sequence to be followed.
+ *
+ * data_size: (read/write)
+ * The user application should read data_size to get the size in bytes of
+ * the data copied in the migration region during the _SAVING state and
+ * write the size in bytes of the data copied in the migration region
+ * during the _RESUMING state.
+ *
+ * The format of the migration region is as follows:
+ * ------------------------------------------------------------------
+ * |vfio_device_migration_info| data section |
+ * | | /////////////////////////////// |
+ * ------------------------------------------------------------------
+ * ^ ^
+ * offset 0-trapped part data_offset
+ *
+ * The structure vfio_device_migration_info is always followed by the data
+ * section in the region, so data_offset will always be nonzero. The offset
+ * from where the data is copied is decided by the kernel driver. The data
+ * section can be trapped, mmapped, or partitioned, depending on how the kernel
+ * driver defines the data section. The data section partition can be defined
+ * as mapped by the sparse mmap capability. If mmapped, data_offset must be
+ * page aligned, whereas initial section which contains the
+ * vfio_device_migration_info structure, might not end at the offset, which is
+ * page aligned. The user is not required to access through mmap regardless
+ * of the capabilities of the region mmap.
+ * The vendor driver should determine whether and how to partition the data
+ * section. The vendor driver should return data_offset accordingly.
+ *
+ * The sequence to be followed while in pre-copy state and stop-and-copy state
+ * is as follows:
+ * a. Read pending_bytes, indicating the start of a new iteration to get device
+ * data. Repeated read on pending_bytes at this stage should have no side
+ * effects.
+ * If pending_bytes == 0, the user application should not iterate to get data
+ * for that device.
+ * If pending_bytes > 0, perform the following steps.
+ * b. Read data_offset, indicating that the vendor driver should make data
+ * available through the data section. The vendor driver should return this
+ * read operation only after data is available from (region + data_offset)
+ * to (region + data_offset + data_size).
+ * c. Read data_size, which is the amount of data in bytes available through
+ * the migration region.
+ * Read on data_offset and data_size should return the offset and size of
+ * the current buffer if the user application reads data_offset and
+ * data_size more than once here.
+ * d. Read data_size bytes of data from (region + data_offset) from the
+ * migration region.
+ * e. Process the data.
+ * f. Read pending_bytes, which indicates that the data from the previous
+ * iteration has been read. If pending_bytes > 0, go to step b.
+ *
+ * The user application can transition from the _SAVING|_RUNNING
+ * (pre-copy state) to the _SAVING (stop-and-copy) state regardless of the
+ * number of pending bytes. The user application should iterate in _SAVING
+ * (stop-and-copy) until pending_bytes is 0.
+ *
+ * The sequence to be followed while _RESUMING device state is as follows:
+ * While data for this device is available, repeat the following steps:
+ * a. Read data_offset from where the user application should write data.
+ * b. Write migration data starting at the migration region + data_offset for
+ * the length determined by data_size from the migration source.
+ * c. Write data_size, which indicates to the vendor driver that data is
+ * written in the migration region. Vendor driver must return this write
+ * operations on consuming data. Vendor driver should apply the
+ * user-provided migration region data to the device resume state.
+ *
+ * If an error occurs during the above sequences, the vendor driver can return
+ * an error code for next read() or write() operation, which will terminate the
+ * loop. The user application should then take the next necessary action, for
+ * example, failing migration or terminating the user application.
+ *
+ * For the user application, data is opaque. The user application should write
+ * data in the same order as the data is received and the data should be of
+ * same transaction size at the source.
+ */
+
+struct vfio_device_migration_info {
+ __u32 device_state; /* VFIO device state */
+#define VFIO_DEVICE_STATE_STOP (0)
+#define VFIO_DEVICE_STATE_RUNNING (1 << 0)
+#define VFIO_DEVICE_STATE_SAVING (1 << 1)
+#define VFIO_DEVICE_STATE_RESUMING (1 << 2)
+#define VFIO_DEVICE_STATE_MASK (VFIO_DEVICE_STATE_RUNNING | \
+ VFIO_DEVICE_STATE_SAVING | \
+ VFIO_DEVICE_STATE_RESUMING)
+
+#define VFIO_DEVICE_STATE_VALID(state) \
+ (state & VFIO_DEVICE_STATE_RESUMING ? \
+ (state & VFIO_DEVICE_STATE_MASK) == VFIO_DEVICE_STATE_RESUMING : 1)
+
+#define VFIO_DEVICE_STATE_IS_ERROR(state) \
+ ((state & VFIO_DEVICE_STATE_MASK) == (VFIO_DEVICE_STATE_SAVING | \
+ VFIO_DEVICE_STATE_RESUMING))
+
+#define VFIO_DEVICE_STATE_SET_ERROR(state) \
+ ((state & ~VFIO_DEVICE_STATE_MASK) | VFIO_DEVICE_SATE_SAVING | \
+ VFIO_DEVICE_STATE_RESUMING)
+
+ __u32 reserved;
+ __u64 pending_bytes;
+ __u64 data_offset;
+ __u64 data_size;
+};
+
/*
* The MSIX mappable capability informs that MSIX data of a BAR can be mmapped
* which allows direct access to non-MSIX registers which happened to be within
--
2.7.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH Kernel v18 1/7] vfio: UAPI for migration interface for device state
@ 2020-05-04 15:58 ` Kirti Wankhede
0 siblings, 0 replies; 39+ messages in thread
From: Kirti Wankhede @ 2020-05-04 15:58 UTC (permalink / raw)
To: alex.williamson, cjia
Cc: Zhengxiao.zx, kevin.tian, yi.l.liu, yan.y.zhao, kvm, eskultet,
ziye.yang, qemu-devel, cohuck, shuangtai.tst, dgilbert,
zhi.a.wang, mlevitsk, pasic, aik, Kirti Wankhede, eauger, felipe,
jonathan.davies, changpeng.liu, Ken.Xue
- Defined MIGRATION region type and sub-type.
- Defined vfio_device_migration_info structure which will be placed at the
0th offset of migration region to get/set VFIO device related
information. Defined members of structure and usage on read/write access.
- Defined device states and state transition details.
- Defined sequence to be followed while saving and resuming VFIO device.
Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
Reviewed-by: Neo Jia <cjia@nvidia.com>
---
include/uapi/linux/vfio.h | 228 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 228 insertions(+)
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 015516bcfaa3..ad9bb5af3463 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -305,6 +305,7 @@ struct vfio_region_info_cap_type {
#define VFIO_REGION_TYPE_PCI_VENDOR_MASK (0xffff)
#define VFIO_REGION_TYPE_GFX (1)
#define VFIO_REGION_TYPE_CCW (2)
+#define VFIO_REGION_TYPE_MIGRATION (3)
/* sub-types for VFIO_REGION_TYPE_PCI_* */
@@ -379,6 +380,233 @@ struct vfio_region_gfx_edid {
/* sub-types for VFIO_REGION_TYPE_CCW */
#define VFIO_REGION_SUBTYPE_CCW_ASYNC_CMD (1)
+/* sub-types for VFIO_REGION_TYPE_MIGRATION */
+#define VFIO_REGION_SUBTYPE_MIGRATION (1)
+
+/*
+ * The structure vfio_device_migration_info is placed at the 0th offset of
+ * the VFIO_REGION_SUBTYPE_MIGRATION region to get and set VFIO device related
+ * migration information. Field accesses from this structure are only supported
+ * at their native width and alignment. Otherwise, the result is undefined and
+ * vendor drivers should return an error.
+ *
+ * device_state: (read/write)
+ * - The user application writes to this field to inform the vendor driver
+ * about the device state to be transitioned to.
+ * - The vendor driver should take the necessary actions to change the
+ * device state. After successful transition to a given state, the
+ * vendor driver should return success on write(device_state, state)
+ * system call. If the device state transition fails, the vendor driver
+ * should return an appropriate -errno for the fault condition.
+ * - On the user application side, if the device state transition fails,
+ * that is, if write(device_state, state) returns an error, read
+ * device_state again to determine the current state of the device from
+ * the vendor driver.
+ * - The vendor driver should return previous state of the device unless
+ * the vendor driver has encountered an internal error, in which case
+ * the vendor driver may report the device_state VFIO_DEVICE_STATE_ERROR.
+ * - The user application must use the device reset ioctl to recover the
+ * device from VFIO_DEVICE_STATE_ERROR state. If the device is
+ * indicated to be in a valid device state by reading device_state, the
+ * user application may attempt to transition the device to any valid
+ * state reachable from the current state or terminate itself.
+ *
+ * device_state consists of 3 bits:
+ * - If bit 0 is set, it indicates the _RUNNING state. If bit 0 is clear,
+ * it indicates the _STOP state. When the device state is changed to
+ * _STOP, driver should stop the device before write() returns.
+ * - If bit 1 is set, it indicates the _SAVING state, which means that the
+ * driver should start gathering device state information that will be
+ * provided to the VFIO user application to save the device's state.
+ * - If bit 2 is set, it indicates the _RESUMING state, which means that
+ * the driver should prepare to resume the device. Data provided through
+ * the migration region should be used to resume the device.
+ * Bits 3 - 31 are reserved for future use. To preserve them, the user
+ * application should perform a read-modify-write operation on this
+ * field when modifying the specified bits.
+ *
+ * +------- _RESUMING
+ * |+------ _SAVING
+ * ||+----- _RUNNING
+ * |||
+ * 000b => Device Stopped, not saving or resuming
+ * 001b => Device running, which is the default state
+ * 010b => Stop the device & save the device state, stop-and-copy state
+ * 011b => Device running and save the device state, pre-copy state
+ * 100b => Device stopped and the device state is resuming
+ * 101b => Invalid state
+ * 110b => Error state
+ * 111b => Invalid state
+ *
+ * State transitions:
+ *
+ * _RESUMING _RUNNING Pre-copy Stop-and-copy _STOP
+ * (100b) (001b) (011b) (010b) (000b)
+ * 0. Running or default state
+ * |
+ *
+ * 1. Normal Shutdown (optional)
+ * |------------------------------------->|
+ *
+ * 2. Save the state or suspend
+ * |------------------------->|---------->|
+ *
+ * 3. Save the state during live migration
+ * |----------->|------------>|---------->|
+ *
+ * 4. Resuming
+ * |<---------|
+ *
+ * 5. Resumed
+ * |--------->|
+ *
+ * 0. Default state of VFIO device is _RUNNNG when the user application starts.
+ * 1. During normal shutdown of the user application, the user application may
+ * optionally change the VFIO device state from _RUNNING to _STOP. This
+ * transition is optional. The vendor driver must support this transition but
+ * must not require it.
+ * 2. When the user application saves state or suspends the application, the
+ * device state transitions from _RUNNING to stop-and-copy and then to _STOP.
+ * On state transition from _RUNNING to stop-and-copy, driver must stop the
+ * device, save the device state and send it to the application through the
+ * migration region. The sequence to be followed for such transition is given
+ * below.
+ * 3. In live migration of user application, the state transitions from _RUNNING
+ * to pre-copy, to stop-and-copy, and to _STOP.
+ * On state transition from _RUNNING to pre-copy, the driver should start
+ * gathering the device state while the application is still running and send
+ * the device state data to application through the migration region.
+ * On state transition from pre-copy to stop-and-copy, the driver must stop
+ * the device, save the device state and send it to the user application
+ * through the migration region.
+ * Vendor drivers must support the pre-copy state even for implementations
+ * where no data is provided to the user before the stop-and-copy state. The
+ * user must not be required to consume all migration data before the device
+ * transitions to a new state, including the stop-and-copy state.
+ * The sequence to be followed for above two transitions is given below.
+ * 4. To start the resuming phase, the device state should be transitioned from
+ * the _RUNNING to the _RESUMING state.
+ * In the _RESUMING state, the driver should use the device state data
+ * received through the migration region to resume the device.
+ * 5. After providing saved device data to the driver, the application should
+ * change the state from _RESUMING to _RUNNING.
+ *
+ * reserved:
+ * Reads on this field return zero and writes are ignored.
+ *
+ * pending_bytes: (read only)
+ * The number of pending bytes still to be migrated from the vendor driver.
+ *
+ * data_offset: (read only)
+ * The user application should read data_offset field from the migration
+ * region. The user application should read the device data from this
+ * offset within the migration region during the _SAVING state or write
+ * the device data during the _RESUMING state. See below for details of
+ * sequence to be followed.
+ *
+ * data_size: (read/write)
+ * The user application should read data_size to get the size in bytes of
+ * the data copied in the migration region during the _SAVING state and
+ * write the size in bytes of the data copied in the migration region
+ * during the _RESUMING state.
+ *
+ * The format of the migration region is as follows:
+ * ------------------------------------------------------------------
+ * |vfio_device_migration_info| data section |
+ * | | /////////////////////////////// |
+ * ------------------------------------------------------------------
+ * ^ ^
+ * offset 0-trapped part data_offset
+ *
+ * The structure vfio_device_migration_info is always followed by the data
+ * section in the region, so data_offset will always be nonzero. The offset
+ * from where the data is copied is decided by the kernel driver. The data
+ * section can be trapped, mmapped, or partitioned, depending on how the kernel
+ * driver defines the data section. The data section partition can be defined
+ * as mapped by the sparse mmap capability. If mmapped, data_offset must be
+ * page aligned, whereas initial section which contains the
+ * vfio_device_migration_info structure, might not end at the offset, which is
+ * page aligned. The user is not required to access through mmap regardless
+ * of the capabilities of the region mmap.
+ * The vendor driver should determine whether and how to partition the data
+ * section. The vendor driver should return data_offset accordingly.
+ *
+ * The sequence to be followed while in pre-copy state and stop-and-copy state
+ * is as follows:
+ * a. Read pending_bytes, indicating the start of a new iteration to get device
+ * data. Repeated read on pending_bytes at this stage should have no side
+ * effects.
+ * If pending_bytes == 0, the user application should not iterate to get data
+ * for that device.
+ * If pending_bytes > 0, perform the following steps.
+ * b. Read data_offset, indicating that the vendor driver should make data
+ * available through the data section. The vendor driver should return this
+ * read operation only after data is available from (region + data_offset)
+ * to (region + data_offset + data_size).
+ * c. Read data_size, which is the amount of data in bytes available through
+ * the migration region.
+ * Read on data_offset and data_size should return the offset and size of
+ * the current buffer if the user application reads data_offset and
+ * data_size more than once here.
+ * d. Read data_size bytes of data from (region + data_offset) from the
+ * migration region.
+ * e. Process the data.
+ * f. Read pending_bytes, which indicates that the data from the previous
+ * iteration has been read. If pending_bytes > 0, go to step b.
+ *
+ * The user application can transition from the _SAVING|_RUNNING
+ * (pre-copy state) to the _SAVING (stop-and-copy) state regardless of the
+ * number of pending bytes. The user application should iterate in _SAVING
+ * (stop-and-copy) until pending_bytes is 0.
+ *
+ * The sequence to be followed while _RESUMING device state is as follows:
+ * While data for this device is available, repeat the following steps:
+ * a. Read data_offset from where the user application should write data.
+ * b. Write migration data starting at the migration region + data_offset for
+ * the length determined by data_size from the migration source.
+ * c. Write data_size, which indicates to the vendor driver that data is
+ * written in the migration region. Vendor driver must return this write
+ * operations on consuming data. Vendor driver should apply the
+ * user-provided migration region data to the device resume state.
+ *
+ * If an error occurs during the above sequences, the vendor driver can return
+ * an error code for next read() or write() operation, which will terminate the
+ * loop. The user application should then take the next necessary action, for
+ * example, failing migration or terminating the user application.
+ *
+ * For the user application, data is opaque. The user application should write
+ * data in the same order as the data is received and the data should be of
+ * same transaction size at the source.
+ */
+
+struct vfio_device_migration_info {
+ __u32 device_state; /* VFIO device state */
+#define VFIO_DEVICE_STATE_STOP (0)
+#define VFIO_DEVICE_STATE_RUNNING (1 << 0)
+#define VFIO_DEVICE_STATE_SAVING (1 << 1)
+#define VFIO_DEVICE_STATE_RESUMING (1 << 2)
+#define VFIO_DEVICE_STATE_MASK (VFIO_DEVICE_STATE_RUNNING | \
+ VFIO_DEVICE_STATE_SAVING | \
+ VFIO_DEVICE_STATE_RESUMING)
+
+#define VFIO_DEVICE_STATE_VALID(state) \
+ (state & VFIO_DEVICE_STATE_RESUMING ? \
+ (state & VFIO_DEVICE_STATE_MASK) == VFIO_DEVICE_STATE_RESUMING : 1)
+
+#define VFIO_DEVICE_STATE_IS_ERROR(state) \
+ ((state & VFIO_DEVICE_STATE_MASK) == (VFIO_DEVICE_STATE_SAVING | \
+ VFIO_DEVICE_STATE_RESUMING))
+
+#define VFIO_DEVICE_STATE_SET_ERROR(state) \
+ ((state & ~VFIO_DEVICE_STATE_MASK) | VFIO_DEVICE_SATE_SAVING | \
+ VFIO_DEVICE_STATE_RESUMING)
+
+ __u32 reserved;
+ __u64 pending_bytes;
+ __u64 data_offset;
+ __u64 data_size;
+};
+
/*
* The MSIX mappable capability informs that MSIX data of a BAR can be mmapped
* which allows direct access to non-MSIX registers which happened to be within
--
2.7.0
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH Kernel v18 2/7] vfio iommu: Remove atomicity of ref_count of pinned pages
2020-05-04 15:58 ` Kirti Wankhede
@ 2020-05-04 15:58 ` Kirti Wankhede
-1 siblings, 0 replies; 39+ messages in thread
From: Kirti Wankhede @ 2020-05-04 15:58 UTC (permalink / raw)
To: alex.williamson, cjia
Cc: kevin.tian, ziye.yang, changpeng.liu, yi.l.liu, mlevitsk,
eskultet, cohuck, dgilbert, jonathan.davies, eauger, aik, pasic,
felipe, Zhengxiao.zx, shuangtai.tst, Ken.Xue, zhi.a.wang,
yan.y.zhao, qemu-devel, kvm, Kirti Wankhede
vfio_pfn.ref_count is always updated while holding iommu->lock, using
atomic variable is overkill.
Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
Reviewed-by: Neo Jia <cjia@nvidia.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
drivers/vfio/vfio_iommu_type1.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index a0c60f895b24..fa735047b04d 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -112,7 +112,7 @@ struct vfio_pfn {
struct rb_node node;
dma_addr_t iova; /* Device address */
unsigned long pfn; /* Host pfn */
- atomic_t ref_count;
+ unsigned int ref_count;
};
struct vfio_regions {
@@ -233,7 +233,7 @@ static int vfio_add_to_pfn_list(struct vfio_dma *dma, dma_addr_t iova,
vpfn->iova = iova;
vpfn->pfn = pfn;
- atomic_set(&vpfn->ref_count, 1);
+ vpfn->ref_count = 1;
vfio_link_pfn(dma, vpfn);
return 0;
}
@@ -251,7 +251,7 @@ static struct vfio_pfn *vfio_iova_get_vfio_pfn(struct vfio_dma *dma,
struct vfio_pfn *vpfn = vfio_find_vpfn(dma, iova);
if (vpfn)
- atomic_inc(&vpfn->ref_count);
+ vpfn->ref_count++;
return vpfn;
}
@@ -259,7 +259,8 @@ static int vfio_iova_put_vfio_pfn(struct vfio_dma *dma, struct vfio_pfn *vpfn)
{
int ret = 0;
- if (atomic_dec_and_test(&vpfn->ref_count)) {
+ vpfn->ref_count--;
+ if (!vpfn->ref_count) {
ret = put_pfn(vpfn->pfn, dma->prot);
vfio_remove_from_pfn_list(dma, vpfn);
}
--
2.7.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH Kernel v18 2/7] vfio iommu: Remove atomicity of ref_count of pinned pages
@ 2020-05-04 15:58 ` Kirti Wankhede
0 siblings, 0 replies; 39+ messages in thread
From: Kirti Wankhede @ 2020-05-04 15:58 UTC (permalink / raw)
To: alex.williamson, cjia
Cc: Zhengxiao.zx, kevin.tian, yi.l.liu, yan.y.zhao, kvm, eskultet,
ziye.yang, qemu-devel, cohuck, shuangtai.tst, dgilbert,
zhi.a.wang, mlevitsk, pasic, aik, Kirti Wankhede, eauger, felipe,
jonathan.davies, changpeng.liu, Ken.Xue
vfio_pfn.ref_count is always updated while holding iommu->lock, using
atomic variable is overkill.
Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
Reviewed-by: Neo Jia <cjia@nvidia.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
drivers/vfio/vfio_iommu_type1.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index a0c60f895b24..fa735047b04d 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -112,7 +112,7 @@ struct vfio_pfn {
struct rb_node node;
dma_addr_t iova; /* Device address */
unsigned long pfn; /* Host pfn */
- atomic_t ref_count;
+ unsigned int ref_count;
};
struct vfio_regions {
@@ -233,7 +233,7 @@ static int vfio_add_to_pfn_list(struct vfio_dma *dma, dma_addr_t iova,
vpfn->iova = iova;
vpfn->pfn = pfn;
- atomic_set(&vpfn->ref_count, 1);
+ vpfn->ref_count = 1;
vfio_link_pfn(dma, vpfn);
return 0;
}
@@ -251,7 +251,7 @@ static struct vfio_pfn *vfio_iova_get_vfio_pfn(struct vfio_dma *dma,
struct vfio_pfn *vpfn = vfio_find_vpfn(dma, iova);
if (vpfn)
- atomic_inc(&vpfn->ref_count);
+ vpfn->ref_count++;
return vpfn;
}
@@ -259,7 +259,8 @@ static int vfio_iova_put_vfio_pfn(struct vfio_dma *dma, struct vfio_pfn *vpfn)
{
int ret = 0;
- if (atomic_dec_and_test(&vpfn->ref_count)) {
+ vpfn->ref_count--;
+ if (!vpfn->ref_count) {
ret = put_pfn(vpfn->pfn, dma->prot);
vfio_remove_from_pfn_list(dma, vpfn);
}
--
2.7.0
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH Kernel v18 3/7] vfio iommu: Add ioctl definition for dirty pages tracking.
2020-05-04 15:58 ` Kirti Wankhede
@ 2020-05-04 15:58 ` Kirti Wankhede
-1 siblings, 0 replies; 39+ messages in thread
From: Kirti Wankhede @ 2020-05-04 15:58 UTC (permalink / raw)
To: alex.williamson, cjia
Cc: kevin.tian, ziye.yang, changpeng.liu, yi.l.liu, mlevitsk,
eskultet, cohuck, dgilbert, jonathan.davies, eauger, aik, pasic,
felipe, Zhengxiao.zx, shuangtai.tst, Ken.Xue, zhi.a.wang,
yan.y.zhao, qemu-devel, kvm, Kirti Wankhede
IOMMU container maintains a list of all pages pinned by vfio_pin_pages API.
All pages pinned by vendor driver through this API should be considered as
dirty during migration. When container consists of IOMMU capable device and
all pages are pinned and mapped, then all pages are marked dirty.
Added support to start/stop dirtied pages tracking and to get bitmap of all
dirtied pages for requested IO virtual address range.
Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
Reviewed-by: Neo Jia <cjia@nvidia.com>
---
include/uapi/linux/vfio.h | 56 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 56 insertions(+)
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index ad9bb5af3463..5f359c63f5ef 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -1033,6 +1033,12 @@ struct vfio_iommu_type1_dma_map {
#define VFIO_IOMMU_MAP_DMA _IO(VFIO_TYPE, VFIO_BASE + 13)
+struct vfio_bitmap {
+ __u64 pgsize; /* page size for bitmap in bytes */
+ __u64 size; /* in bytes */
+ __u64 __user *data; /* one bit per page */
+};
+
/**
* VFIO_IOMMU_UNMAP_DMA - _IOWR(VFIO_TYPE, VFIO_BASE + 14,
* struct vfio_dma_unmap)
@@ -1059,6 +1065,56 @@ struct vfio_iommu_type1_dma_unmap {
#define VFIO_IOMMU_ENABLE _IO(VFIO_TYPE, VFIO_BASE + 15)
#define VFIO_IOMMU_DISABLE _IO(VFIO_TYPE, VFIO_BASE + 16)
+/**
+ * VFIO_IOMMU_DIRTY_PAGES - _IOWR(VFIO_TYPE, VFIO_BASE + 17,
+ * struct vfio_iommu_type1_dirty_bitmap)
+ * IOCTL is used for dirty pages tracking.
+ * Caller should set flag depending on which operation to perform, details as
+ * below:
+ *
+ * When IOCTL is called with VFIO_IOMMU_DIRTY_PAGES_FLAG_START set, indicates
+ * migration is active and IOMMU module should track pages which are dirtied or
+ * potentially dirtied by device.
+ * Dirty pages are tracked until tracking is stopped by user application by
+ * setting VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP flag.
+ *
+ * When IOCTL is called with VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP set, indicates
+ * IOMMU should stop tracking dirtied pages.
+ *
+ * When IOCTL is called with VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP flag set,
+ * IOCTL returns dirty pages bitmap for IOMMU container during migration for
+ * given IOVA range. User must provide data[] as the structure
+ * vfio_iommu_type1_dirty_bitmap_get through which user provides IOVA range and
+ * pgsize. IOVA range must match to that used in original mapping call. This
+ * interface supports to get bitmap of smallest supported pgsize only and can
+ * be modified in future to get bitmap of specified pgsize.
+ * User must allocate memory for bitmap and set size of allocated memory in
+ * bitmap.size field. One bit is used to represent one page consecutively
+ * starting from iova offset. User should provide page size in bitmap.pgsize
+ * field. Bit set in bitmap indicates page at that offset from iova is
+ * dirty. Caller must set argsz including size of structure
+ * vfio_iommu_type1_dirty_bitmap_get.
+ *
+ * Only one of the flags _START, STOP and _GET may be specified at a time.
+ *
+ */
+struct vfio_iommu_type1_dirty_bitmap {
+ __u32 argsz;
+ __u32 flags;
+#define VFIO_IOMMU_DIRTY_PAGES_FLAG_START (1 << 0)
+#define VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP (1 << 1)
+#define VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP (1 << 2)
+ __u8 data[];
+};
+
+struct vfio_iommu_type1_dirty_bitmap_get {
+ __u64 iova; /* IO virtual address */
+ __u64 size; /* Size of iova range */
+ struct vfio_bitmap bitmap;
+};
+
+#define VFIO_IOMMU_DIRTY_PAGES _IO(VFIO_TYPE, VFIO_BASE + 17)
+
/* -------- Additional API for SPAPR TCE (Server POWERPC) IOMMU -------- */
/*
--
2.7.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH Kernel v18 3/7] vfio iommu: Add ioctl definition for dirty pages tracking.
@ 2020-05-04 15:58 ` Kirti Wankhede
0 siblings, 0 replies; 39+ messages in thread
From: Kirti Wankhede @ 2020-05-04 15:58 UTC (permalink / raw)
To: alex.williamson, cjia
Cc: Zhengxiao.zx, kevin.tian, yi.l.liu, yan.y.zhao, kvm, eskultet,
ziye.yang, qemu-devel, cohuck, shuangtai.tst, dgilbert,
zhi.a.wang, mlevitsk, pasic, aik, Kirti Wankhede, eauger, felipe,
jonathan.davies, changpeng.liu, Ken.Xue
IOMMU container maintains a list of all pages pinned by vfio_pin_pages API.
All pages pinned by vendor driver through this API should be considered as
dirty during migration. When container consists of IOMMU capable device and
all pages are pinned and mapped, then all pages are marked dirty.
Added support to start/stop dirtied pages tracking and to get bitmap of all
dirtied pages for requested IO virtual address range.
Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
Reviewed-by: Neo Jia <cjia@nvidia.com>
---
include/uapi/linux/vfio.h | 56 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 56 insertions(+)
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index ad9bb5af3463..5f359c63f5ef 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -1033,6 +1033,12 @@ struct vfio_iommu_type1_dma_map {
#define VFIO_IOMMU_MAP_DMA _IO(VFIO_TYPE, VFIO_BASE + 13)
+struct vfio_bitmap {
+ __u64 pgsize; /* page size for bitmap in bytes */
+ __u64 size; /* in bytes */
+ __u64 __user *data; /* one bit per page */
+};
+
/**
* VFIO_IOMMU_UNMAP_DMA - _IOWR(VFIO_TYPE, VFIO_BASE + 14,
* struct vfio_dma_unmap)
@@ -1059,6 +1065,56 @@ struct vfio_iommu_type1_dma_unmap {
#define VFIO_IOMMU_ENABLE _IO(VFIO_TYPE, VFIO_BASE + 15)
#define VFIO_IOMMU_DISABLE _IO(VFIO_TYPE, VFIO_BASE + 16)
+/**
+ * VFIO_IOMMU_DIRTY_PAGES - _IOWR(VFIO_TYPE, VFIO_BASE + 17,
+ * struct vfio_iommu_type1_dirty_bitmap)
+ * IOCTL is used for dirty pages tracking.
+ * Caller should set flag depending on which operation to perform, details as
+ * below:
+ *
+ * When IOCTL is called with VFIO_IOMMU_DIRTY_PAGES_FLAG_START set, indicates
+ * migration is active and IOMMU module should track pages which are dirtied or
+ * potentially dirtied by device.
+ * Dirty pages are tracked until tracking is stopped by user application by
+ * setting VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP flag.
+ *
+ * When IOCTL is called with VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP set, indicates
+ * IOMMU should stop tracking dirtied pages.
+ *
+ * When IOCTL is called with VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP flag set,
+ * IOCTL returns dirty pages bitmap for IOMMU container during migration for
+ * given IOVA range. User must provide data[] as the structure
+ * vfio_iommu_type1_dirty_bitmap_get through which user provides IOVA range and
+ * pgsize. IOVA range must match to that used in original mapping call. This
+ * interface supports to get bitmap of smallest supported pgsize only and can
+ * be modified in future to get bitmap of specified pgsize.
+ * User must allocate memory for bitmap and set size of allocated memory in
+ * bitmap.size field. One bit is used to represent one page consecutively
+ * starting from iova offset. User should provide page size in bitmap.pgsize
+ * field. Bit set in bitmap indicates page at that offset from iova is
+ * dirty. Caller must set argsz including size of structure
+ * vfio_iommu_type1_dirty_bitmap_get.
+ *
+ * Only one of the flags _START, STOP and _GET may be specified at a time.
+ *
+ */
+struct vfio_iommu_type1_dirty_bitmap {
+ __u32 argsz;
+ __u32 flags;
+#define VFIO_IOMMU_DIRTY_PAGES_FLAG_START (1 << 0)
+#define VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP (1 << 1)
+#define VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP (1 << 2)
+ __u8 data[];
+};
+
+struct vfio_iommu_type1_dirty_bitmap_get {
+ __u64 iova; /* IO virtual address */
+ __u64 size; /* Size of iova range */
+ struct vfio_bitmap bitmap;
+};
+
+#define VFIO_IOMMU_DIRTY_PAGES _IO(VFIO_TYPE, VFIO_BASE + 17)
+
/* -------- Additional API for SPAPR TCE (Server POWERPC) IOMMU -------- */
/*
--
2.7.0
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH Kernel v18 4/7] vfio iommu: Implementation of ioctl for dirty pages tracking.
2020-05-04 15:58 ` Kirti Wankhede
@ 2020-05-04 15:58 ` Kirti Wankhede
-1 siblings, 0 replies; 39+ messages in thread
From: Kirti Wankhede @ 2020-05-04 15:58 UTC (permalink / raw)
To: alex.williamson, cjia
Cc: kevin.tian, ziye.yang, changpeng.liu, yi.l.liu, mlevitsk,
eskultet, cohuck, dgilbert, jonathan.davies, eauger, aik, pasic,
felipe, Zhengxiao.zx, shuangtai.tst, Ken.Xue, zhi.a.wang,
yan.y.zhao, qemu-devel, kvm, Kirti Wankhede
VFIO_IOMMU_DIRTY_PAGES ioctl performs three operations:
- Start dirty pages tracking while migration is active
- Stop dirty pages tracking.
- Get dirty pages bitmap. Its user space application's responsibility to
copy content of dirty pages from source to destination during migration.
To prevent DoS attack, memory for bitmap is allocated per vfio_dma
structure. Bitmap size is calculated considering smallest supported page
size. Bitmap is allocated for all vfio_dmas when dirty logging is enabled
Bitmap is populated for already pinned pages when bitmap is allocated for
a vfio_dma with the smallest supported page size. Update bitmap from
pinning functions when tracking is enabled. When user application queries
bitmap, check if requested page size is same as page size used to
populated bitmap. If it is equal, copy bitmap, but if not equal, return
error.
Fixed below error by changing pgsize type from uint64_t to size_t.
Reported-by: kbuild test robot <lkp@intel.com>
All errors:
drivers/vfio/vfio_iommu_type1.c:197: undefined reference to `__udivdi3'
drivers/vfio/vfio_iommu_type1.c:225: undefined reference to `__udivdi3'
Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
Reviewed-by: Neo Jia <cjia@nvidia.com>
---
drivers/vfio/vfio_iommu_type1.c | 266 +++++++++++++++++++++++++++++++++++++++-
1 file changed, 260 insertions(+), 6 deletions(-)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index fa735047b04d..01dcb417836f 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -71,6 +71,7 @@ struct vfio_iommu {
unsigned int dma_avail;
bool v2;
bool nesting;
+ bool dirty_page_tracking;
};
struct vfio_domain {
@@ -91,6 +92,7 @@ struct vfio_dma {
bool lock_cap; /* capable(CAP_IPC_LOCK) */
struct task_struct *task;
struct rb_root pfn_list; /* Ex-user pinned pfn list */
+ unsigned long *bitmap;
};
struct vfio_group {
@@ -125,7 +127,21 @@ struct vfio_regions {
#define IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu) \
(!list_empty(&iommu->domain_list))
+#define DIRTY_BITMAP_BYTES(n) (ALIGN(n, BITS_PER_TYPE(u64)) / BITS_PER_BYTE)
+
+/*
+ * Input argument of number of bits to bitmap_set() is unsigned integer, which
+ * further casts to signed integer for unaligned multi-bit operation,
+ * __bitmap_set().
+ * Then maximum bitmap size supported is 2^31 bits divided by 2^3 bits/byte,
+ * that is 2^28 (256 MB) which maps to 2^31 * 2^12 = 2^43 (8TB) on 4K page
+ * system.
+ */
+#define DIRTY_BITMAP_PAGES_MAX ((u64)INT_MAX)
+#define DIRTY_BITMAP_SIZE_MAX DIRTY_BITMAP_BYTES(DIRTY_BITMAP_PAGES_MAX)
+
static int put_pfn(unsigned long pfn, int prot);
+static unsigned long vfio_pgsize_bitmap(struct vfio_iommu *iommu);
/*
* This code handles mapping and unmapping of user data buffers
@@ -175,6 +191,77 @@ static void vfio_unlink_dma(struct vfio_iommu *iommu, struct vfio_dma *old)
rb_erase(&old->node, &iommu->dma_list);
}
+
+static int vfio_dma_bitmap_alloc(struct vfio_dma *dma, size_t pgsize)
+{
+ uint64_t npages = dma->size / pgsize;
+
+ if (npages > DIRTY_BITMAP_PAGES_MAX)
+ return -EINVAL;
+
+ dma->bitmap = kvzalloc(DIRTY_BITMAP_BYTES(npages), GFP_KERNEL);
+ if (!dma->bitmap)
+ return -ENOMEM;
+
+ return 0;
+}
+
+static void vfio_dma_bitmap_free(struct vfio_dma *dma)
+{
+ kfree(dma->bitmap);
+ dma->bitmap = NULL;
+}
+
+static void vfio_dma_populate_bitmap(struct vfio_dma *dma, size_t pgsize)
+{
+ struct rb_node *p;
+
+ if (RB_EMPTY_ROOT(&dma->pfn_list))
+ return;
+
+ for (p = rb_first(&dma->pfn_list); p; p = rb_next(p)) {
+ struct vfio_pfn *vpfn = rb_entry(p, struct vfio_pfn, node);
+
+ bitmap_set(dma->bitmap, (vpfn->iova - dma->iova) / pgsize, 1);
+ }
+}
+
+static int vfio_dma_bitmap_alloc_all(struct vfio_iommu *iommu, size_t pgsize)
+{
+ struct rb_node *n = rb_first(&iommu->dma_list);
+
+ for (; n; n = rb_next(n)) {
+ struct vfio_dma *dma = rb_entry(n, struct vfio_dma, node);
+ int ret;
+
+ ret = vfio_dma_bitmap_alloc(dma, pgsize);
+ if (ret) {
+ struct rb_node *p = rb_prev(n);
+
+ for (; p; p = rb_prev(p)) {
+ struct vfio_dma *dma = rb_entry(n,
+ struct vfio_dma, node);
+
+ vfio_dma_bitmap_free(dma);
+ }
+ return ret;
+ }
+ vfio_dma_populate_bitmap(dma, pgsize);
+ }
+ return 0;
+}
+
+static void vfio_dma_bitmap_free_all(struct vfio_iommu *iommu)
+{
+ struct rb_node *n = rb_first(&iommu->dma_list);
+
+ for (; n; n = rb_next(n)) {
+ struct vfio_dma *dma = rb_entry(n, struct vfio_dma, node);
+
+ vfio_dma_bitmap_free(dma);
+ }
+}
+
/*
* Helper Functions for host iova-pfn list
*/
@@ -567,6 +654,18 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
vfio_unpin_page_external(dma, iova, do_accounting);
goto pin_unwind;
}
+
+ if (iommu->dirty_page_tracking) {
+ unsigned long pgshift =
+ __ffs(vfio_pgsize_bitmap(iommu));
+
+ /*
+ * Bitmap populated with the smallest supported page
+ * size
+ */
+ bitmap_set(dma->bitmap,
+ (vpfn->iova - dma->iova) >> pgshift, 1);
+ }
}
ret = i;
@@ -801,6 +900,7 @@ static void vfio_remove_dma(struct vfio_iommu *iommu, struct vfio_dma *dma)
vfio_unmap_unpin(iommu, dma, true);
vfio_unlink_dma(iommu, dma);
put_task_struct(dma->task);
+ vfio_dma_bitmap_free(dma);
kfree(dma);
iommu->dma_avail++;
}
@@ -831,6 +931,57 @@ static unsigned long vfio_pgsize_bitmap(struct vfio_iommu *iommu)
return bitmap;
}
+static int vfio_iova_dirty_bitmap(struct vfio_iommu *iommu, dma_addr_t iova,
+ size_t size, size_t pgsize,
+ u64 __user *bitmap)
+{
+ struct vfio_dma *dma;
+ unsigned long pgshift = __ffs(pgsize);
+ unsigned int npages, bitmap_size;
+
+ dma = vfio_find_dma(iommu, iova, 1);
+
+ if (!dma)
+ return -EINVAL;
+
+ if (dma->iova != iova || dma->size != size)
+ return -EINVAL;
+
+ npages = dma->size >> pgshift;
+ bitmap_size = DIRTY_BITMAP_BYTES(npages);
+
+ /* mark all pages dirty if all pages are pinned and mapped. */
+ if (dma->iommu_mapped)
+ bitmap_set(dma->bitmap, 0, npages);
+
+ if (copy_to_user((void __user *)bitmap, dma->bitmap, bitmap_size))
+ return -EFAULT;
+
+ /*
+ * Re-populate bitmap to include all pinned pages which are considered
+ * as dirty but exclude pages which are unpinned and pages which are
+ * marked dirty by vfio_dma_rw()
+ */
+ bitmap_clear(dma->bitmap, 0, npages);
+ vfio_dma_populate_bitmap(dma, pgsize);
+ return 0;
+}
+
+static int verify_bitmap_size(uint64_t npages, uint64_t bitmap_size)
+{
+ uint64_t bsize;
+
+ if (!npages || !bitmap_size || (bitmap_size > DIRTY_BITMAP_SIZE_MAX))
+ return -EINVAL;
+
+ bsize = DIRTY_BITMAP_BYTES(npages);
+
+ if (bitmap_size < bsize)
+ return -EINVAL;
+
+ return 0;
+}
+
static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
struct vfio_iommu_type1_dma_unmap *unmap)
{
@@ -1038,16 +1189,16 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
unsigned long vaddr = map->vaddr;
size_t size = map->size;
int ret = 0, prot = 0;
- uint64_t mask;
+ size_t pgsize;
struct vfio_dma *dma;
/* Verify that none of our __u64 fields overflow */
if (map->size != size || map->vaddr != vaddr || map->iova != iova)
return -EINVAL;
- mask = ((uint64_t)1 << __ffs(vfio_pgsize_bitmap(iommu))) - 1;
+ pgsize = (size_t)1 << __ffs(vfio_pgsize_bitmap(iommu));
- WARN_ON(mask & PAGE_MASK);
+ WARN_ON((pgsize - 1) & PAGE_MASK);
/* READ/WRITE from device perspective */
if (map->flags & VFIO_DMA_MAP_FLAG_WRITE)
@@ -1055,7 +1206,7 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
if (map->flags & VFIO_DMA_MAP_FLAG_READ)
prot |= IOMMU_READ;
- if (!prot || !size || (size | iova | vaddr) & mask)
+ if (!prot || !size || (size | iova | vaddr) & (pgsize - 1))
return -EINVAL;
/* Don't allow IOVA or virtual address wrap */
@@ -1130,6 +1281,12 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
else
ret = vfio_pin_map_dma(iommu, dma, size);
+ if (!ret && iommu->dirty_page_tracking) {
+ ret = vfio_dma_bitmap_alloc(dma, pgsize);
+ if (ret)
+ vfio_remove_dma(iommu, dma);
+ }
+
out_unlock:
mutex_unlock(&iommu->lock);
return ret;
@@ -2278,6 +2435,93 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
return copy_to_user((void __user *)arg, &unmap, minsz) ?
-EFAULT : 0;
+ } else if (cmd == VFIO_IOMMU_DIRTY_PAGES) {
+ struct vfio_iommu_type1_dirty_bitmap dirty;
+ uint32_t mask = VFIO_IOMMU_DIRTY_PAGES_FLAG_START |
+ VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP |
+ VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP;
+ int ret = 0;
+
+ if (!iommu->v2)
+ return -EACCES;
+
+ minsz = offsetofend(struct vfio_iommu_type1_dirty_bitmap,
+ flags);
+
+ if (copy_from_user(&dirty, (void __user *)arg, minsz))
+ return -EFAULT;
+
+ if (dirty.argsz < minsz || dirty.flags & ~mask)
+ return -EINVAL;
+
+ /* only one flag should be set at a time */
+ if (__ffs(dirty.flags) != __fls(dirty.flags))
+ return -EINVAL;
+
+ if (dirty.flags & VFIO_IOMMU_DIRTY_PAGES_FLAG_START) {
+ size_t pgsize = 1 << __ffs(vfio_pgsize_bitmap(iommu));
+
+ mutex_lock(&iommu->lock);
+ if (!iommu->dirty_page_tracking) {
+ ret = vfio_dma_bitmap_alloc_all(iommu, pgsize);
+ if (!ret)
+ iommu->dirty_page_tracking = true;
+ }
+ mutex_unlock(&iommu->lock);
+ return ret;
+ } else if (dirty.flags & VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP) {
+ mutex_lock(&iommu->lock);
+ if (iommu->dirty_page_tracking) {
+ iommu->dirty_page_tracking = false;
+ vfio_dma_bitmap_free_all(iommu);
+ }
+ mutex_unlock(&iommu->lock);
+ return 0;
+ } else if (dirty.flags &
+ VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP) {
+ struct vfio_iommu_type1_dirty_bitmap_get range;
+ unsigned long pgshift;
+ size_t data_size = dirty.argsz - minsz;
+ size_t iommu_pgsize =
+ (size_t)1 << __ffs(vfio_pgsize_bitmap(iommu));
+
+ if (!data_size || data_size < sizeof(range))
+ return -EINVAL;
+
+ if (copy_from_user(&range, (void __user *)(arg + minsz),
+ sizeof(range)))
+ return -EFAULT;
+
+ /* allow only smallest supported pgsize */
+ if (range.bitmap.pgsize != iommu_pgsize)
+ return -EINVAL;
+ if (range.iova & (iommu_pgsize - 1))
+ return -EINVAL;
+ if (!range.size || range.size & (iommu_pgsize - 1))
+ return -EINVAL;
+ if (range.iova + range.size < range.iova)
+ return -EINVAL;
+ if (!access_ok((void __user *)range.bitmap.data,
+ range.bitmap.size))
+ return -EINVAL;
+
+ pgshift = __ffs(range.bitmap.pgsize);
+ ret = verify_bitmap_size(range.size >> pgshift,
+ range.bitmap.size);
+ if (ret)
+ return ret;
+
+ mutex_lock(&iommu->lock);
+ if (iommu->dirty_page_tracking)
+ ret = vfio_iova_dirty_bitmap(iommu, range.iova,
+ range.size, range.bitmap.pgsize,
+ range.bitmap.data);
+ else
+ ret = -EINVAL;
+ mutex_unlock(&iommu->lock);
+
+ return ret;
+ }
}
return -ENOTTY;
@@ -2345,10 +2589,20 @@ static int vfio_iommu_type1_dma_rw_chunk(struct vfio_iommu *iommu,
vaddr = dma->vaddr + offset;
- if (write)
+ if (write) {
*copied = copy_to_user((void __user *)vaddr, data,
count) ? 0 : count;
- else
+ if (*copied && iommu->dirty_page_tracking) {
+ unsigned long pgshift =
+ __ffs(vfio_pgsize_bitmap(iommu));
+ /*
+ * Bitmap populated with the smallest supported page
+ * size
+ */
+ bitmap_set(dma->bitmap, offset >> pgshift,
+ *copied >> pgshift);
+ }
+ } else
*copied = copy_from_user(data, (void __user *)vaddr,
count) ? 0 : count;
if (kthread)
--
2.7.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH Kernel v18 4/7] vfio iommu: Implementation of ioctl for dirty pages tracking.
@ 2020-05-04 15:58 ` Kirti Wankhede
0 siblings, 0 replies; 39+ messages in thread
From: Kirti Wankhede @ 2020-05-04 15:58 UTC (permalink / raw)
To: alex.williamson, cjia
Cc: Zhengxiao.zx, kevin.tian, yi.l.liu, yan.y.zhao, kvm, eskultet,
ziye.yang, qemu-devel, cohuck, shuangtai.tst, dgilbert,
zhi.a.wang, mlevitsk, pasic, aik, Kirti Wankhede, eauger, felipe,
jonathan.davies, changpeng.liu, Ken.Xue
VFIO_IOMMU_DIRTY_PAGES ioctl performs three operations:
- Start dirty pages tracking while migration is active
- Stop dirty pages tracking.
- Get dirty pages bitmap. Its user space application's responsibility to
copy content of dirty pages from source to destination during migration.
To prevent DoS attack, memory for bitmap is allocated per vfio_dma
structure. Bitmap size is calculated considering smallest supported page
size. Bitmap is allocated for all vfio_dmas when dirty logging is enabled
Bitmap is populated for already pinned pages when bitmap is allocated for
a vfio_dma with the smallest supported page size. Update bitmap from
pinning functions when tracking is enabled. When user application queries
bitmap, check if requested page size is same as page size used to
populated bitmap. If it is equal, copy bitmap, but if not equal, return
error.
Fixed below error by changing pgsize type from uint64_t to size_t.
Reported-by: kbuild test robot <lkp@intel.com>
All errors:
drivers/vfio/vfio_iommu_type1.c:197: undefined reference to `__udivdi3'
drivers/vfio/vfio_iommu_type1.c:225: undefined reference to `__udivdi3'
Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
Reviewed-by: Neo Jia <cjia@nvidia.com>
---
drivers/vfio/vfio_iommu_type1.c | 266 +++++++++++++++++++++++++++++++++++++++-
1 file changed, 260 insertions(+), 6 deletions(-)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index fa735047b04d..01dcb417836f 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -71,6 +71,7 @@ struct vfio_iommu {
unsigned int dma_avail;
bool v2;
bool nesting;
+ bool dirty_page_tracking;
};
struct vfio_domain {
@@ -91,6 +92,7 @@ struct vfio_dma {
bool lock_cap; /* capable(CAP_IPC_LOCK) */
struct task_struct *task;
struct rb_root pfn_list; /* Ex-user pinned pfn list */
+ unsigned long *bitmap;
};
struct vfio_group {
@@ -125,7 +127,21 @@ struct vfio_regions {
#define IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu) \
(!list_empty(&iommu->domain_list))
+#define DIRTY_BITMAP_BYTES(n) (ALIGN(n, BITS_PER_TYPE(u64)) / BITS_PER_BYTE)
+
+/*
+ * Input argument of number of bits to bitmap_set() is unsigned integer, which
+ * further casts to signed integer for unaligned multi-bit operation,
+ * __bitmap_set().
+ * Then maximum bitmap size supported is 2^31 bits divided by 2^3 bits/byte,
+ * that is 2^28 (256 MB) which maps to 2^31 * 2^12 = 2^43 (8TB) on 4K page
+ * system.
+ */
+#define DIRTY_BITMAP_PAGES_MAX ((u64)INT_MAX)
+#define DIRTY_BITMAP_SIZE_MAX DIRTY_BITMAP_BYTES(DIRTY_BITMAP_PAGES_MAX)
+
static int put_pfn(unsigned long pfn, int prot);
+static unsigned long vfio_pgsize_bitmap(struct vfio_iommu *iommu);
/*
* This code handles mapping and unmapping of user data buffers
@@ -175,6 +191,77 @@ static void vfio_unlink_dma(struct vfio_iommu *iommu, struct vfio_dma *old)
rb_erase(&old->node, &iommu->dma_list);
}
+
+static int vfio_dma_bitmap_alloc(struct vfio_dma *dma, size_t pgsize)
+{
+ uint64_t npages = dma->size / pgsize;
+
+ if (npages > DIRTY_BITMAP_PAGES_MAX)
+ return -EINVAL;
+
+ dma->bitmap = kvzalloc(DIRTY_BITMAP_BYTES(npages), GFP_KERNEL);
+ if (!dma->bitmap)
+ return -ENOMEM;
+
+ return 0;
+}
+
+static void vfio_dma_bitmap_free(struct vfio_dma *dma)
+{
+ kfree(dma->bitmap);
+ dma->bitmap = NULL;
+}
+
+static void vfio_dma_populate_bitmap(struct vfio_dma *dma, size_t pgsize)
+{
+ struct rb_node *p;
+
+ if (RB_EMPTY_ROOT(&dma->pfn_list))
+ return;
+
+ for (p = rb_first(&dma->pfn_list); p; p = rb_next(p)) {
+ struct vfio_pfn *vpfn = rb_entry(p, struct vfio_pfn, node);
+
+ bitmap_set(dma->bitmap, (vpfn->iova - dma->iova) / pgsize, 1);
+ }
+}
+
+static int vfio_dma_bitmap_alloc_all(struct vfio_iommu *iommu, size_t pgsize)
+{
+ struct rb_node *n = rb_first(&iommu->dma_list);
+
+ for (; n; n = rb_next(n)) {
+ struct vfio_dma *dma = rb_entry(n, struct vfio_dma, node);
+ int ret;
+
+ ret = vfio_dma_bitmap_alloc(dma, pgsize);
+ if (ret) {
+ struct rb_node *p = rb_prev(n);
+
+ for (; p; p = rb_prev(p)) {
+ struct vfio_dma *dma = rb_entry(n,
+ struct vfio_dma, node);
+
+ vfio_dma_bitmap_free(dma);
+ }
+ return ret;
+ }
+ vfio_dma_populate_bitmap(dma, pgsize);
+ }
+ return 0;
+}
+
+static void vfio_dma_bitmap_free_all(struct vfio_iommu *iommu)
+{
+ struct rb_node *n = rb_first(&iommu->dma_list);
+
+ for (; n; n = rb_next(n)) {
+ struct vfio_dma *dma = rb_entry(n, struct vfio_dma, node);
+
+ vfio_dma_bitmap_free(dma);
+ }
+}
+
/*
* Helper Functions for host iova-pfn list
*/
@@ -567,6 +654,18 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
vfio_unpin_page_external(dma, iova, do_accounting);
goto pin_unwind;
}
+
+ if (iommu->dirty_page_tracking) {
+ unsigned long pgshift =
+ __ffs(vfio_pgsize_bitmap(iommu));
+
+ /*
+ * Bitmap populated with the smallest supported page
+ * size
+ */
+ bitmap_set(dma->bitmap,
+ (vpfn->iova - dma->iova) >> pgshift, 1);
+ }
}
ret = i;
@@ -801,6 +900,7 @@ static void vfio_remove_dma(struct vfio_iommu *iommu, struct vfio_dma *dma)
vfio_unmap_unpin(iommu, dma, true);
vfio_unlink_dma(iommu, dma);
put_task_struct(dma->task);
+ vfio_dma_bitmap_free(dma);
kfree(dma);
iommu->dma_avail++;
}
@@ -831,6 +931,57 @@ static unsigned long vfio_pgsize_bitmap(struct vfio_iommu *iommu)
return bitmap;
}
+static int vfio_iova_dirty_bitmap(struct vfio_iommu *iommu, dma_addr_t iova,
+ size_t size, size_t pgsize,
+ u64 __user *bitmap)
+{
+ struct vfio_dma *dma;
+ unsigned long pgshift = __ffs(pgsize);
+ unsigned int npages, bitmap_size;
+
+ dma = vfio_find_dma(iommu, iova, 1);
+
+ if (!dma)
+ return -EINVAL;
+
+ if (dma->iova != iova || dma->size != size)
+ return -EINVAL;
+
+ npages = dma->size >> pgshift;
+ bitmap_size = DIRTY_BITMAP_BYTES(npages);
+
+ /* mark all pages dirty if all pages are pinned and mapped. */
+ if (dma->iommu_mapped)
+ bitmap_set(dma->bitmap, 0, npages);
+
+ if (copy_to_user((void __user *)bitmap, dma->bitmap, bitmap_size))
+ return -EFAULT;
+
+ /*
+ * Re-populate bitmap to include all pinned pages which are considered
+ * as dirty but exclude pages which are unpinned and pages which are
+ * marked dirty by vfio_dma_rw()
+ */
+ bitmap_clear(dma->bitmap, 0, npages);
+ vfio_dma_populate_bitmap(dma, pgsize);
+ return 0;
+}
+
+static int verify_bitmap_size(uint64_t npages, uint64_t bitmap_size)
+{
+ uint64_t bsize;
+
+ if (!npages || !bitmap_size || (bitmap_size > DIRTY_BITMAP_SIZE_MAX))
+ return -EINVAL;
+
+ bsize = DIRTY_BITMAP_BYTES(npages);
+
+ if (bitmap_size < bsize)
+ return -EINVAL;
+
+ return 0;
+}
+
static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
struct vfio_iommu_type1_dma_unmap *unmap)
{
@@ -1038,16 +1189,16 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
unsigned long vaddr = map->vaddr;
size_t size = map->size;
int ret = 0, prot = 0;
- uint64_t mask;
+ size_t pgsize;
struct vfio_dma *dma;
/* Verify that none of our __u64 fields overflow */
if (map->size != size || map->vaddr != vaddr || map->iova != iova)
return -EINVAL;
- mask = ((uint64_t)1 << __ffs(vfio_pgsize_bitmap(iommu))) - 1;
+ pgsize = (size_t)1 << __ffs(vfio_pgsize_bitmap(iommu));
- WARN_ON(mask & PAGE_MASK);
+ WARN_ON((pgsize - 1) & PAGE_MASK);
/* READ/WRITE from device perspective */
if (map->flags & VFIO_DMA_MAP_FLAG_WRITE)
@@ -1055,7 +1206,7 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
if (map->flags & VFIO_DMA_MAP_FLAG_READ)
prot |= IOMMU_READ;
- if (!prot || !size || (size | iova | vaddr) & mask)
+ if (!prot || !size || (size | iova | vaddr) & (pgsize - 1))
return -EINVAL;
/* Don't allow IOVA or virtual address wrap */
@@ -1130,6 +1281,12 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
else
ret = vfio_pin_map_dma(iommu, dma, size);
+ if (!ret && iommu->dirty_page_tracking) {
+ ret = vfio_dma_bitmap_alloc(dma, pgsize);
+ if (ret)
+ vfio_remove_dma(iommu, dma);
+ }
+
out_unlock:
mutex_unlock(&iommu->lock);
return ret;
@@ -2278,6 +2435,93 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
return copy_to_user((void __user *)arg, &unmap, minsz) ?
-EFAULT : 0;
+ } else if (cmd == VFIO_IOMMU_DIRTY_PAGES) {
+ struct vfio_iommu_type1_dirty_bitmap dirty;
+ uint32_t mask = VFIO_IOMMU_DIRTY_PAGES_FLAG_START |
+ VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP |
+ VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP;
+ int ret = 0;
+
+ if (!iommu->v2)
+ return -EACCES;
+
+ minsz = offsetofend(struct vfio_iommu_type1_dirty_bitmap,
+ flags);
+
+ if (copy_from_user(&dirty, (void __user *)arg, minsz))
+ return -EFAULT;
+
+ if (dirty.argsz < minsz || dirty.flags & ~mask)
+ return -EINVAL;
+
+ /* only one flag should be set at a time */
+ if (__ffs(dirty.flags) != __fls(dirty.flags))
+ return -EINVAL;
+
+ if (dirty.flags & VFIO_IOMMU_DIRTY_PAGES_FLAG_START) {
+ size_t pgsize = 1 << __ffs(vfio_pgsize_bitmap(iommu));
+
+ mutex_lock(&iommu->lock);
+ if (!iommu->dirty_page_tracking) {
+ ret = vfio_dma_bitmap_alloc_all(iommu, pgsize);
+ if (!ret)
+ iommu->dirty_page_tracking = true;
+ }
+ mutex_unlock(&iommu->lock);
+ return ret;
+ } else if (dirty.flags & VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP) {
+ mutex_lock(&iommu->lock);
+ if (iommu->dirty_page_tracking) {
+ iommu->dirty_page_tracking = false;
+ vfio_dma_bitmap_free_all(iommu);
+ }
+ mutex_unlock(&iommu->lock);
+ return 0;
+ } else if (dirty.flags &
+ VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP) {
+ struct vfio_iommu_type1_dirty_bitmap_get range;
+ unsigned long pgshift;
+ size_t data_size = dirty.argsz - minsz;
+ size_t iommu_pgsize =
+ (size_t)1 << __ffs(vfio_pgsize_bitmap(iommu));
+
+ if (!data_size || data_size < sizeof(range))
+ return -EINVAL;
+
+ if (copy_from_user(&range, (void __user *)(arg + minsz),
+ sizeof(range)))
+ return -EFAULT;
+
+ /* allow only smallest supported pgsize */
+ if (range.bitmap.pgsize != iommu_pgsize)
+ return -EINVAL;
+ if (range.iova & (iommu_pgsize - 1))
+ return -EINVAL;
+ if (!range.size || range.size & (iommu_pgsize - 1))
+ return -EINVAL;
+ if (range.iova + range.size < range.iova)
+ return -EINVAL;
+ if (!access_ok((void __user *)range.bitmap.data,
+ range.bitmap.size))
+ return -EINVAL;
+
+ pgshift = __ffs(range.bitmap.pgsize);
+ ret = verify_bitmap_size(range.size >> pgshift,
+ range.bitmap.size);
+ if (ret)
+ return ret;
+
+ mutex_lock(&iommu->lock);
+ if (iommu->dirty_page_tracking)
+ ret = vfio_iova_dirty_bitmap(iommu, range.iova,
+ range.size, range.bitmap.pgsize,
+ range.bitmap.data);
+ else
+ ret = -EINVAL;
+ mutex_unlock(&iommu->lock);
+
+ return ret;
+ }
}
return -ENOTTY;
@@ -2345,10 +2589,20 @@ static int vfio_iommu_type1_dma_rw_chunk(struct vfio_iommu *iommu,
vaddr = dma->vaddr + offset;
- if (write)
+ if (write) {
*copied = copy_to_user((void __user *)vaddr, data,
count) ? 0 : count;
- else
+ if (*copied && iommu->dirty_page_tracking) {
+ unsigned long pgshift =
+ __ffs(vfio_pgsize_bitmap(iommu));
+ /*
+ * Bitmap populated with the smallest supported page
+ * size
+ */
+ bitmap_set(dma->bitmap, offset >> pgshift,
+ *copied >> pgshift);
+ }
+ } else
*copied = copy_from_user(data, (void __user *)vaddr,
count) ? 0 : count;
if (kthread)
--
2.7.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH Kernel v18 4/7] vfio iommu: Implementation of ioctl for dirty pages tracking.
2020-05-04 15:58 ` Kirti Wankhede
@ 2020-05-06 8:15 ` Yan Zhao
-1 siblings, 0 replies; 39+ messages in thread
From: Yan Zhao @ 2020-05-06 8:15 UTC (permalink / raw)
To: Kirti Wankhede
Cc: alex.williamson, cjia, Tian, Kevin, Yang, Ziye, Liu, Changpeng,
Liu, Yi L, mlevitsk, eskultet, cohuck, dgilbert, jonathan.davies,
eauger, aik, pasic, felipe, Zhengxiao.zx, shuangtai.tst, Ken.Xue,
Wang, Zhi A, qemu-devel, kvm
On Mon, May 04, 2020 at 11:58:56PM +0800, Kirti Wankhede wrote:
> VFIO_IOMMU_DIRTY_PAGES ioctl performs three operations:
> - Start dirty pages tracking while migration is active
> - Stop dirty pages tracking.
> - Get dirty pages bitmap. Its user space application's responsibility to
> copy content of dirty pages from source to destination during migration.
>
> To prevent DoS attack, memory for bitmap is allocated per vfio_dma
> structure. Bitmap size is calculated considering smallest supported page
> size. Bitmap is allocated for all vfio_dmas when dirty logging is enabled
>
> Bitmap is populated for already pinned pages when bitmap is allocated for
> a vfio_dma with the smallest supported page size. Update bitmap from
> pinning functions when tracking is enabled. When user application queries
> bitmap, check if requested page size is same as page size used to
> populated bitmap. If it is equal, copy bitmap, but if not equal, return
> error.
>
> Fixed below error by changing pgsize type from uint64_t to size_t.
> Reported-by: kbuild test robot <lkp@intel.com>
>
> All errors:
> drivers/vfio/vfio_iommu_type1.c:197: undefined reference to `__udivdi3'
>
> drivers/vfio/vfio_iommu_type1.c:225: undefined reference to `__udivdi3'
>
> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> Reviewed-by: Neo Jia <cjia@nvidia.com>
> ---
> drivers/vfio/vfio_iommu_type1.c | 266 +++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 260 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index fa735047b04d..01dcb417836f 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -71,6 +71,7 @@ struct vfio_iommu {
> unsigned int dma_avail;
> bool v2;
> bool nesting;
> + bool dirty_page_tracking;
> };
>
> struct vfio_domain {
> @@ -91,6 +92,7 @@ struct vfio_dma {
> bool lock_cap; /* capable(CAP_IPC_LOCK) */
> struct task_struct *task;
> struct rb_root pfn_list; /* Ex-user pinned pfn list */
> + unsigned long *bitmap;
> };
>
> struct vfio_group {
> @@ -125,7 +127,21 @@ struct vfio_regions {
> #define IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu) \
> (!list_empty(&iommu->domain_list))
>
> +#define DIRTY_BITMAP_BYTES(n) (ALIGN(n, BITS_PER_TYPE(u64)) / BITS_PER_BYTE)
> +
> +/*
> + * Input argument of number of bits to bitmap_set() is unsigned integer, which
> + * further casts to signed integer for unaligned multi-bit operation,
> + * __bitmap_set().
> + * Then maximum bitmap size supported is 2^31 bits divided by 2^3 bits/byte,
> + * that is 2^28 (256 MB) which maps to 2^31 * 2^12 = 2^43 (8TB) on 4K page
> + * system.
> + */
> +#define DIRTY_BITMAP_PAGES_MAX ((u64)INT_MAX)
> +#define DIRTY_BITMAP_SIZE_MAX DIRTY_BITMAP_BYTES(DIRTY_BITMAP_PAGES_MAX)
> +
> static int put_pfn(unsigned long pfn, int prot);
> +static unsigned long vfio_pgsize_bitmap(struct vfio_iommu *iommu);
>
> /*
> * This code handles mapping and unmapping of user data buffers
> @@ -175,6 +191,77 @@ static void vfio_unlink_dma(struct vfio_iommu *iommu, struct vfio_dma *old)
> rb_erase(&old->node, &iommu->dma_list);
> }
>
> +
> +static int vfio_dma_bitmap_alloc(struct vfio_dma *dma, size_t pgsize)
> +{
> + uint64_t npages = dma->size / pgsize;
> +
> + if (npages > DIRTY_BITMAP_PAGES_MAX)
> + return -EINVAL;
> +
> + dma->bitmap = kvzalloc(DIRTY_BITMAP_BYTES(npages), GFP_KERNEL);
> + if (!dma->bitmap)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +static void vfio_dma_bitmap_free(struct vfio_dma *dma)
> +{
> + kfree(dma->bitmap);
> + dma->bitmap = NULL;
> +}
> +
> +static void vfio_dma_populate_bitmap(struct vfio_dma *dma, size_t pgsize)
> +{
> + struct rb_node *p;
> +
> + if (RB_EMPTY_ROOT(&dma->pfn_list))
> + return;
> +
> + for (p = rb_first(&dma->pfn_list); p; p = rb_next(p)) {
> + struct vfio_pfn *vpfn = rb_entry(p, struct vfio_pfn, node);
> +
> + bitmap_set(dma->bitmap, (vpfn->iova - dma->iova) / pgsize, 1);
> + }
> +}
> +
> +static int vfio_dma_bitmap_alloc_all(struct vfio_iommu *iommu, size_t pgsize)
> +{
> + struct rb_node *n = rb_first(&iommu->dma_list);
> +
> + for (; n; n = rb_next(n)) {
> + struct vfio_dma *dma = rb_entry(n, struct vfio_dma, node);
> + int ret;
> +
> + ret = vfio_dma_bitmap_alloc(dma, pgsize);
> + if (ret) {
> + struct rb_node *p = rb_prev(n);
> +
> + for (; p; p = rb_prev(p)) {
> + struct vfio_dma *dma = rb_entry(n,
> + struct vfio_dma, node);
> +
> + vfio_dma_bitmap_free(dma);
> + }
> + return ret;
> + }
> + vfio_dma_populate_bitmap(dma, pgsize);
> + }
> + return 0;
> +}
> +
> +static void vfio_dma_bitmap_free_all(struct vfio_iommu *iommu)
> +{
> + struct rb_node *n = rb_first(&iommu->dma_list);
> +
> + for (; n; n = rb_next(n)) {
> + struct vfio_dma *dma = rb_entry(n, struct vfio_dma, node);
> +
> + vfio_dma_bitmap_free(dma);
> + }
> +}
> +
> /*
> * Helper Functions for host iova-pfn list
> */
> @@ -567,6 +654,18 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
> vfio_unpin_page_external(dma, iova, do_accounting);
> goto pin_unwind;
> }
> +
> + if (iommu->dirty_page_tracking) {
> + unsigned long pgshift =
> + __ffs(vfio_pgsize_bitmap(iommu));
> +
hi Kirti,
may I know if there's any vfio_pin_pages() happpening during NVidia's vGPU migration?
the code would enter into deadlock as I reported in last version.
Thanks
Yan
> + /*
> + * Bitmap populated with the smallest supported page
> + * size
> + */
> + bitmap_set(dma->bitmap,
> + (vpfn->iova - dma->iova) >> pgshift, 1);
> + }
> }
>
> ret = i;
> @@ -801,6 +900,7 @@ static void vfio_remove_dma(struct vfio_iommu *iommu, struct vfio_dma *dma)
> vfio_unmap_unpin(iommu, dma, true);
> vfio_unlink_dma(iommu, dma);
> put_task_struct(dma->task);
> + vfio_dma_bitmap_free(dma);
> kfree(dma);
> iommu->dma_avail++;
> }
> @@ -831,6 +931,57 @@ static unsigned long vfio_pgsize_bitmap(struct vfio_iommu *iommu)
> return bitmap;
> }
>
> +static int vfio_iova_dirty_bitmap(struct vfio_iommu *iommu, dma_addr_t iova,
> + size_t size, size_t pgsize,
> + u64 __user *bitmap)
> +{
> + struct vfio_dma *dma;
> + unsigned long pgshift = __ffs(pgsize);
> + unsigned int npages, bitmap_size;
> +
> + dma = vfio_find_dma(iommu, iova, 1);
> +
> + if (!dma)
> + return -EINVAL;
> +
> + if (dma->iova != iova || dma->size != size)
> + return -EINVAL;
> +
> + npages = dma->size >> pgshift;
> + bitmap_size = DIRTY_BITMAP_BYTES(npages);
> +
> + /* mark all pages dirty if all pages are pinned and mapped. */
> + if (dma->iommu_mapped)
> + bitmap_set(dma->bitmap, 0, npages);
> +
> + if (copy_to_user((void __user *)bitmap, dma->bitmap, bitmap_size))
> + return -EFAULT;
> +
> + /*
> + * Re-populate bitmap to include all pinned pages which are considered
> + * as dirty but exclude pages which are unpinned and pages which are
> + * marked dirty by vfio_dma_rw()
> + */
> + bitmap_clear(dma->bitmap, 0, npages);
> + vfio_dma_populate_bitmap(dma, pgsize);
> + return 0;
> +}
> +
> +static int verify_bitmap_size(uint64_t npages, uint64_t bitmap_size)
> +{
> + uint64_t bsize;
> +
> + if (!npages || !bitmap_size || (bitmap_size > DIRTY_BITMAP_SIZE_MAX))
> + return -EINVAL;
> +
> + bsize = DIRTY_BITMAP_BYTES(npages);
> +
> + if (bitmap_size < bsize)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> struct vfio_iommu_type1_dma_unmap *unmap)
> {
> @@ -1038,16 +1189,16 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
> unsigned long vaddr = map->vaddr;
> size_t size = map->size;
> int ret = 0, prot = 0;
> - uint64_t mask;
> + size_t pgsize;
> struct vfio_dma *dma;
>
> /* Verify that none of our __u64 fields overflow */
> if (map->size != size || map->vaddr != vaddr || map->iova != iova)
> return -EINVAL;
>
> - mask = ((uint64_t)1 << __ffs(vfio_pgsize_bitmap(iommu))) - 1;
> + pgsize = (size_t)1 << __ffs(vfio_pgsize_bitmap(iommu));
>
> - WARN_ON(mask & PAGE_MASK);
> + WARN_ON((pgsize - 1) & PAGE_MASK);
>
> /* READ/WRITE from device perspective */
> if (map->flags & VFIO_DMA_MAP_FLAG_WRITE)
> @@ -1055,7 +1206,7 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
> if (map->flags & VFIO_DMA_MAP_FLAG_READ)
> prot |= IOMMU_READ;
>
> - if (!prot || !size || (size | iova | vaddr) & mask)
> + if (!prot || !size || (size | iova | vaddr) & (pgsize - 1))
> return -EINVAL;
>
> /* Don't allow IOVA or virtual address wrap */
> @@ -1130,6 +1281,12 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
> else
> ret = vfio_pin_map_dma(iommu, dma, size);
>
> + if (!ret && iommu->dirty_page_tracking) {
> + ret = vfio_dma_bitmap_alloc(dma, pgsize);
> + if (ret)
> + vfio_remove_dma(iommu, dma);
> + }
> +
> out_unlock:
> mutex_unlock(&iommu->lock);
> return ret;
> @@ -2278,6 +2435,93 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>
> return copy_to_user((void __user *)arg, &unmap, minsz) ?
> -EFAULT : 0;
> + } else if (cmd == VFIO_IOMMU_DIRTY_PAGES) {
> + struct vfio_iommu_type1_dirty_bitmap dirty;
> + uint32_t mask = VFIO_IOMMU_DIRTY_PAGES_FLAG_START |
> + VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP |
> + VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP;
> + int ret = 0;
> +
> + if (!iommu->v2)
> + return -EACCES;
> +
> + minsz = offsetofend(struct vfio_iommu_type1_dirty_bitmap,
> + flags);
> +
> + if (copy_from_user(&dirty, (void __user *)arg, minsz))
> + return -EFAULT;
> +
> + if (dirty.argsz < minsz || dirty.flags & ~mask)
> + return -EINVAL;
> +
> + /* only one flag should be set at a time */
> + if (__ffs(dirty.flags) != __fls(dirty.flags))
> + return -EINVAL;
> +
> + if (dirty.flags & VFIO_IOMMU_DIRTY_PAGES_FLAG_START) {
> + size_t pgsize = 1 << __ffs(vfio_pgsize_bitmap(iommu));
> +
> + mutex_lock(&iommu->lock);
> + if (!iommu->dirty_page_tracking) {
> + ret = vfio_dma_bitmap_alloc_all(iommu, pgsize);
> + if (!ret)
> + iommu->dirty_page_tracking = true;
> + }
> + mutex_unlock(&iommu->lock);
> + return ret;
> + } else if (dirty.flags & VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP) {
> + mutex_lock(&iommu->lock);
> + if (iommu->dirty_page_tracking) {
> + iommu->dirty_page_tracking = false;
> + vfio_dma_bitmap_free_all(iommu);
> + }
> + mutex_unlock(&iommu->lock);
> + return 0;
> + } else if (dirty.flags &
> + VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP) {
> + struct vfio_iommu_type1_dirty_bitmap_get range;
> + unsigned long pgshift;
> + size_t data_size = dirty.argsz - minsz;
> + size_t iommu_pgsize =
> + (size_t)1 << __ffs(vfio_pgsize_bitmap(iommu));
> +
> + if (!data_size || data_size < sizeof(range))
> + return -EINVAL;
> +
> + if (copy_from_user(&range, (void __user *)(arg + minsz),
> + sizeof(range)))
> + return -EFAULT;
> +
> + /* allow only smallest supported pgsize */
> + if (range.bitmap.pgsize != iommu_pgsize)
> + return -EINVAL;
> + if (range.iova & (iommu_pgsize - 1))
> + return -EINVAL;
> + if (!range.size || range.size & (iommu_pgsize - 1))
> + return -EINVAL;
> + if (range.iova + range.size < range.iova)
> + return -EINVAL;
> + if (!access_ok((void __user *)range.bitmap.data,
> + range.bitmap.size))
> + return -EINVAL;
> +
> + pgshift = __ffs(range.bitmap.pgsize);
> + ret = verify_bitmap_size(range.size >> pgshift,
> + range.bitmap.size);
> + if (ret)
> + return ret;
> +
> + mutex_lock(&iommu->lock);
> + if (iommu->dirty_page_tracking)
> + ret = vfio_iova_dirty_bitmap(iommu, range.iova,
> + range.size, range.bitmap.pgsize,
> + range.bitmap.data);
> + else
> + ret = -EINVAL;
> + mutex_unlock(&iommu->lock);
> +
> + return ret;
> + }
> }
>
> return -ENOTTY;
> @@ -2345,10 +2589,20 @@ static int vfio_iommu_type1_dma_rw_chunk(struct vfio_iommu *iommu,
>
> vaddr = dma->vaddr + offset;
>
> - if (write)
> + if (write) {
> *copied = copy_to_user((void __user *)vaddr, data,
> count) ? 0 : count;
> - else
> + if (*copied && iommu->dirty_page_tracking) {
> + unsigned long pgshift =
> + __ffs(vfio_pgsize_bitmap(iommu));
> + /*
> + * Bitmap populated with the smallest supported page
> + * size
> + */
> + bitmap_set(dma->bitmap, offset >> pgshift,
> + *copied >> pgshift);
> + }
> + } else
> *copied = copy_from_user(data, (void __user *)vaddr,
> count) ? 0 : count;
> if (kthread)
> --
> 2.7.0
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH Kernel v18 4/7] vfio iommu: Implementation of ioctl for dirty pages tracking.
@ 2020-05-06 8:15 ` Yan Zhao
0 siblings, 0 replies; 39+ messages in thread
From: Yan Zhao @ 2020-05-06 8:15 UTC (permalink / raw)
To: Kirti Wankhede
Cc: Zhengxiao.zx, Tian, Kevin, Liu, Yi L, cjia, kvm, eskultet, Yang,
Ziye, qemu-devel, cohuck, shuangtai.tst, dgilbert, Wang, Zhi A,
mlevitsk, pasic, aik, alex.williamson, eauger, felipe,
jonathan.davies, Liu, Changpeng, Ken.Xue
On Mon, May 04, 2020 at 11:58:56PM +0800, Kirti Wankhede wrote:
> VFIO_IOMMU_DIRTY_PAGES ioctl performs three operations:
> - Start dirty pages tracking while migration is active
> - Stop dirty pages tracking.
> - Get dirty pages bitmap. Its user space application's responsibility to
> copy content of dirty pages from source to destination during migration.
>
> To prevent DoS attack, memory for bitmap is allocated per vfio_dma
> structure. Bitmap size is calculated considering smallest supported page
> size. Bitmap is allocated for all vfio_dmas when dirty logging is enabled
>
> Bitmap is populated for already pinned pages when bitmap is allocated for
> a vfio_dma with the smallest supported page size. Update bitmap from
> pinning functions when tracking is enabled. When user application queries
> bitmap, check if requested page size is same as page size used to
> populated bitmap. If it is equal, copy bitmap, but if not equal, return
> error.
>
> Fixed below error by changing pgsize type from uint64_t to size_t.
> Reported-by: kbuild test robot <lkp@intel.com>
>
> All errors:
> drivers/vfio/vfio_iommu_type1.c:197: undefined reference to `__udivdi3'
>
> drivers/vfio/vfio_iommu_type1.c:225: undefined reference to `__udivdi3'
>
> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> Reviewed-by: Neo Jia <cjia@nvidia.com>
> ---
> drivers/vfio/vfio_iommu_type1.c | 266 +++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 260 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index fa735047b04d..01dcb417836f 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -71,6 +71,7 @@ struct vfio_iommu {
> unsigned int dma_avail;
> bool v2;
> bool nesting;
> + bool dirty_page_tracking;
> };
>
> struct vfio_domain {
> @@ -91,6 +92,7 @@ struct vfio_dma {
> bool lock_cap; /* capable(CAP_IPC_LOCK) */
> struct task_struct *task;
> struct rb_root pfn_list; /* Ex-user pinned pfn list */
> + unsigned long *bitmap;
> };
>
> struct vfio_group {
> @@ -125,7 +127,21 @@ struct vfio_regions {
> #define IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu) \
> (!list_empty(&iommu->domain_list))
>
> +#define DIRTY_BITMAP_BYTES(n) (ALIGN(n, BITS_PER_TYPE(u64)) / BITS_PER_BYTE)
> +
> +/*
> + * Input argument of number of bits to bitmap_set() is unsigned integer, which
> + * further casts to signed integer for unaligned multi-bit operation,
> + * __bitmap_set().
> + * Then maximum bitmap size supported is 2^31 bits divided by 2^3 bits/byte,
> + * that is 2^28 (256 MB) which maps to 2^31 * 2^12 = 2^43 (8TB) on 4K page
> + * system.
> + */
> +#define DIRTY_BITMAP_PAGES_MAX ((u64)INT_MAX)
> +#define DIRTY_BITMAP_SIZE_MAX DIRTY_BITMAP_BYTES(DIRTY_BITMAP_PAGES_MAX)
> +
> static int put_pfn(unsigned long pfn, int prot);
> +static unsigned long vfio_pgsize_bitmap(struct vfio_iommu *iommu);
>
> /*
> * This code handles mapping and unmapping of user data buffers
> @@ -175,6 +191,77 @@ static void vfio_unlink_dma(struct vfio_iommu *iommu, struct vfio_dma *old)
> rb_erase(&old->node, &iommu->dma_list);
> }
>
> +
> +static int vfio_dma_bitmap_alloc(struct vfio_dma *dma, size_t pgsize)
> +{
> + uint64_t npages = dma->size / pgsize;
> +
> + if (npages > DIRTY_BITMAP_PAGES_MAX)
> + return -EINVAL;
> +
> + dma->bitmap = kvzalloc(DIRTY_BITMAP_BYTES(npages), GFP_KERNEL);
> + if (!dma->bitmap)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +static void vfio_dma_bitmap_free(struct vfio_dma *dma)
> +{
> + kfree(dma->bitmap);
> + dma->bitmap = NULL;
> +}
> +
> +static void vfio_dma_populate_bitmap(struct vfio_dma *dma, size_t pgsize)
> +{
> + struct rb_node *p;
> +
> + if (RB_EMPTY_ROOT(&dma->pfn_list))
> + return;
> +
> + for (p = rb_first(&dma->pfn_list); p; p = rb_next(p)) {
> + struct vfio_pfn *vpfn = rb_entry(p, struct vfio_pfn, node);
> +
> + bitmap_set(dma->bitmap, (vpfn->iova - dma->iova) / pgsize, 1);
> + }
> +}
> +
> +static int vfio_dma_bitmap_alloc_all(struct vfio_iommu *iommu, size_t pgsize)
> +{
> + struct rb_node *n = rb_first(&iommu->dma_list);
> +
> + for (; n; n = rb_next(n)) {
> + struct vfio_dma *dma = rb_entry(n, struct vfio_dma, node);
> + int ret;
> +
> + ret = vfio_dma_bitmap_alloc(dma, pgsize);
> + if (ret) {
> + struct rb_node *p = rb_prev(n);
> +
> + for (; p; p = rb_prev(p)) {
> + struct vfio_dma *dma = rb_entry(n,
> + struct vfio_dma, node);
> +
> + vfio_dma_bitmap_free(dma);
> + }
> + return ret;
> + }
> + vfio_dma_populate_bitmap(dma, pgsize);
> + }
> + return 0;
> +}
> +
> +static void vfio_dma_bitmap_free_all(struct vfio_iommu *iommu)
> +{
> + struct rb_node *n = rb_first(&iommu->dma_list);
> +
> + for (; n; n = rb_next(n)) {
> + struct vfio_dma *dma = rb_entry(n, struct vfio_dma, node);
> +
> + vfio_dma_bitmap_free(dma);
> + }
> +}
> +
> /*
> * Helper Functions for host iova-pfn list
> */
> @@ -567,6 +654,18 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
> vfio_unpin_page_external(dma, iova, do_accounting);
> goto pin_unwind;
> }
> +
> + if (iommu->dirty_page_tracking) {
> + unsigned long pgshift =
> + __ffs(vfio_pgsize_bitmap(iommu));
> +
hi Kirti,
may I know if there's any vfio_pin_pages() happpening during NVidia's vGPU migration?
the code would enter into deadlock as I reported in last version.
Thanks
Yan
> + /*
> + * Bitmap populated with the smallest supported page
> + * size
> + */
> + bitmap_set(dma->bitmap,
> + (vpfn->iova - dma->iova) >> pgshift, 1);
> + }
> }
>
> ret = i;
> @@ -801,6 +900,7 @@ static void vfio_remove_dma(struct vfio_iommu *iommu, struct vfio_dma *dma)
> vfio_unmap_unpin(iommu, dma, true);
> vfio_unlink_dma(iommu, dma);
> put_task_struct(dma->task);
> + vfio_dma_bitmap_free(dma);
> kfree(dma);
> iommu->dma_avail++;
> }
> @@ -831,6 +931,57 @@ static unsigned long vfio_pgsize_bitmap(struct vfio_iommu *iommu)
> return bitmap;
> }
>
> +static int vfio_iova_dirty_bitmap(struct vfio_iommu *iommu, dma_addr_t iova,
> + size_t size, size_t pgsize,
> + u64 __user *bitmap)
> +{
> + struct vfio_dma *dma;
> + unsigned long pgshift = __ffs(pgsize);
> + unsigned int npages, bitmap_size;
> +
> + dma = vfio_find_dma(iommu, iova, 1);
> +
> + if (!dma)
> + return -EINVAL;
> +
> + if (dma->iova != iova || dma->size != size)
> + return -EINVAL;
> +
> + npages = dma->size >> pgshift;
> + bitmap_size = DIRTY_BITMAP_BYTES(npages);
> +
> + /* mark all pages dirty if all pages are pinned and mapped. */
> + if (dma->iommu_mapped)
> + bitmap_set(dma->bitmap, 0, npages);
> +
> + if (copy_to_user((void __user *)bitmap, dma->bitmap, bitmap_size))
> + return -EFAULT;
> +
> + /*
> + * Re-populate bitmap to include all pinned pages which are considered
> + * as dirty but exclude pages which are unpinned and pages which are
> + * marked dirty by vfio_dma_rw()
> + */
> + bitmap_clear(dma->bitmap, 0, npages);
> + vfio_dma_populate_bitmap(dma, pgsize);
> + return 0;
> +}
> +
> +static int verify_bitmap_size(uint64_t npages, uint64_t bitmap_size)
> +{
> + uint64_t bsize;
> +
> + if (!npages || !bitmap_size || (bitmap_size > DIRTY_BITMAP_SIZE_MAX))
> + return -EINVAL;
> +
> + bsize = DIRTY_BITMAP_BYTES(npages);
> +
> + if (bitmap_size < bsize)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> struct vfio_iommu_type1_dma_unmap *unmap)
> {
> @@ -1038,16 +1189,16 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
> unsigned long vaddr = map->vaddr;
> size_t size = map->size;
> int ret = 0, prot = 0;
> - uint64_t mask;
> + size_t pgsize;
> struct vfio_dma *dma;
>
> /* Verify that none of our __u64 fields overflow */
> if (map->size != size || map->vaddr != vaddr || map->iova != iova)
> return -EINVAL;
>
> - mask = ((uint64_t)1 << __ffs(vfio_pgsize_bitmap(iommu))) - 1;
> + pgsize = (size_t)1 << __ffs(vfio_pgsize_bitmap(iommu));
>
> - WARN_ON(mask & PAGE_MASK);
> + WARN_ON((pgsize - 1) & PAGE_MASK);
>
> /* READ/WRITE from device perspective */
> if (map->flags & VFIO_DMA_MAP_FLAG_WRITE)
> @@ -1055,7 +1206,7 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
> if (map->flags & VFIO_DMA_MAP_FLAG_READ)
> prot |= IOMMU_READ;
>
> - if (!prot || !size || (size | iova | vaddr) & mask)
> + if (!prot || !size || (size | iova | vaddr) & (pgsize - 1))
> return -EINVAL;
>
> /* Don't allow IOVA or virtual address wrap */
> @@ -1130,6 +1281,12 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
> else
> ret = vfio_pin_map_dma(iommu, dma, size);
>
> + if (!ret && iommu->dirty_page_tracking) {
> + ret = vfio_dma_bitmap_alloc(dma, pgsize);
> + if (ret)
> + vfio_remove_dma(iommu, dma);
> + }
> +
> out_unlock:
> mutex_unlock(&iommu->lock);
> return ret;
> @@ -2278,6 +2435,93 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>
> return copy_to_user((void __user *)arg, &unmap, minsz) ?
> -EFAULT : 0;
> + } else if (cmd == VFIO_IOMMU_DIRTY_PAGES) {
> + struct vfio_iommu_type1_dirty_bitmap dirty;
> + uint32_t mask = VFIO_IOMMU_DIRTY_PAGES_FLAG_START |
> + VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP |
> + VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP;
> + int ret = 0;
> +
> + if (!iommu->v2)
> + return -EACCES;
> +
> + minsz = offsetofend(struct vfio_iommu_type1_dirty_bitmap,
> + flags);
> +
> + if (copy_from_user(&dirty, (void __user *)arg, minsz))
> + return -EFAULT;
> +
> + if (dirty.argsz < minsz || dirty.flags & ~mask)
> + return -EINVAL;
> +
> + /* only one flag should be set at a time */
> + if (__ffs(dirty.flags) != __fls(dirty.flags))
> + return -EINVAL;
> +
> + if (dirty.flags & VFIO_IOMMU_DIRTY_PAGES_FLAG_START) {
> + size_t pgsize = 1 << __ffs(vfio_pgsize_bitmap(iommu));
> +
> + mutex_lock(&iommu->lock);
> + if (!iommu->dirty_page_tracking) {
> + ret = vfio_dma_bitmap_alloc_all(iommu, pgsize);
> + if (!ret)
> + iommu->dirty_page_tracking = true;
> + }
> + mutex_unlock(&iommu->lock);
> + return ret;
> + } else if (dirty.flags & VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP) {
> + mutex_lock(&iommu->lock);
> + if (iommu->dirty_page_tracking) {
> + iommu->dirty_page_tracking = false;
> + vfio_dma_bitmap_free_all(iommu);
> + }
> + mutex_unlock(&iommu->lock);
> + return 0;
> + } else if (dirty.flags &
> + VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP) {
> + struct vfio_iommu_type1_dirty_bitmap_get range;
> + unsigned long pgshift;
> + size_t data_size = dirty.argsz - minsz;
> + size_t iommu_pgsize =
> + (size_t)1 << __ffs(vfio_pgsize_bitmap(iommu));
> +
> + if (!data_size || data_size < sizeof(range))
> + return -EINVAL;
> +
> + if (copy_from_user(&range, (void __user *)(arg + minsz),
> + sizeof(range)))
> + return -EFAULT;
> +
> + /* allow only smallest supported pgsize */
> + if (range.bitmap.pgsize != iommu_pgsize)
> + return -EINVAL;
> + if (range.iova & (iommu_pgsize - 1))
> + return -EINVAL;
> + if (!range.size || range.size & (iommu_pgsize - 1))
> + return -EINVAL;
> + if (range.iova + range.size < range.iova)
> + return -EINVAL;
> + if (!access_ok((void __user *)range.bitmap.data,
> + range.bitmap.size))
> + return -EINVAL;
> +
> + pgshift = __ffs(range.bitmap.pgsize);
> + ret = verify_bitmap_size(range.size >> pgshift,
> + range.bitmap.size);
> + if (ret)
> + return ret;
> +
> + mutex_lock(&iommu->lock);
> + if (iommu->dirty_page_tracking)
> + ret = vfio_iova_dirty_bitmap(iommu, range.iova,
> + range.size, range.bitmap.pgsize,
> + range.bitmap.data);
> + else
> + ret = -EINVAL;
> + mutex_unlock(&iommu->lock);
> +
> + return ret;
> + }
> }
>
> return -ENOTTY;
> @@ -2345,10 +2589,20 @@ static int vfio_iommu_type1_dma_rw_chunk(struct vfio_iommu *iommu,
>
> vaddr = dma->vaddr + offset;
>
> - if (write)
> + if (write) {
> *copied = copy_to_user((void __user *)vaddr, data,
> count) ? 0 : count;
> - else
> + if (*copied && iommu->dirty_page_tracking) {
> + unsigned long pgshift =
> + __ffs(vfio_pgsize_bitmap(iommu));
> + /*
> + * Bitmap populated with the smallest supported page
> + * size
> + */
> + bitmap_set(dma->bitmap, offset >> pgshift,
> + *copied >> pgshift);
> + }
> + } else
> *copied = copy_from_user(data, (void __user *)vaddr,
> count) ? 0 : count;
> if (kthread)
> --
> 2.7.0
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH Kernel v18 4/7] vfio iommu: Implementation of ioctl for dirty pages tracking.
2020-05-06 8:15 ` Yan Zhao
@ 2020-05-06 19:42 ` Kirti Wankhede
-1 siblings, 0 replies; 39+ messages in thread
From: Kirti Wankhede @ 2020-05-06 19:42 UTC (permalink / raw)
To: Yan Zhao
Cc: alex.williamson, cjia, Tian, Kevin, Yang, Ziye, Liu, Changpeng,
Liu, Yi L, mlevitsk, eskultet, cohuck, dgilbert, jonathan.davies,
eauger, aik, pasic, felipe, Zhengxiao.zx, shuangtai.tst, Ken.Xue,
Wang, Zhi A, qemu-devel, kvm
On 5/6/2020 1:45 PM, Yan Zhao wrote:
> On Mon, May 04, 2020 at 11:58:56PM +0800, Kirti Wankhede wrote:
<snip>
>> /*
>> * Helper Functions for host iova-pfn list
>> */
>> @@ -567,6 +654,18 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
>> vfio_unpin_page_external(dma, iova, do_accounting);
>> goto pin_unwind;
>> }
>> +
>> + if (iommu->dirty_page_tracking) {
>> + unsigned long pgshift =
>> + __ffs(vfio_pgsize_bitmap(iommu));
>> +
> hi Kirti,
> may I know if there's any vfio_pin_pages() happpening during NVidia's vGPU migration?
> the code would enter into deadlock as I reported in last version.
>
Hm, you are right and same is the case in vfio_iommu_type1_dma_rw_chunk().
Instead of calling vfio_pgsize_bitmap() from lots of places, I'm
thinking of saving pgsize_bitmap in struct vfio_iommu, which should be
populated whenever domain_list is updated. Alex, will that be fine?
Thanks,
Kirti
> Thanks
> Yan
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH Kernel v18 4/7] vfio iommu: Implementation of ioctl for dirty pages tracking.
@ 2020-05-06 19:42 ` Kirti Wankhede
0 siblings, 0 replies; 39+ messages in thread
From: Kirti Wankhede @ 2020-05-06 19:42 UTC (permalink / raw)
To: Yan Zhao
Cc: Zhengxiao.zx, Tian, Kevin, Liu, Yi L, cjia, kvm, eskultet, Yang,
Ziye, qemu-devel, cohuck, shuangtai.tst, dgilbert, Wang, Zhi A,
mlevitsk, pasic, aik, alex.williamson, eauger, felipe,
jonathan.davies, Liu, Changpeng, Ken.Xue
On 5/6/2020 1:45 PM, Yan Zhao wrote:
> On Mon, May 04, 2020 at 11:58:56PM +0800, Kirti Wankhede wrote:
<snip>
>> /*
>> * Helper Functions for host iova-pfn list
>> */
>> @@ -567,6 +654,18 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
>> vfio_unpin_page_external(dma, iova, do_accounting);
>> goto pin_unwind;
>> }
>> +
>> + if (iommu->dirty_page_tracking) {
>> + unsigned long pgshift =
>> + __ffs(vfio_pgsize_bitmap(iommu));
>> +
> hi Kirti,
> may I know if there's any vfio_pin_pages() happpening during NVidia's vGPU migration?
> the code would enter into deadlock as I reported in last version.
>
Hm, you are right and same is the case in vfio_iommu_type1_dma_rw_chunk().
Instead of calling vfio_pgsize_bitmap() from lots of places, I'm
thinking of saving pgsize_bitmap in struct vfio_iommu, which should be
populated whenever domain_list is updated. Alex, will that be fine?
Thanks,
Kirti
> Thanks
> Yan
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH Kernel v18 4/7] vfio iommu: Implementation of ioctl for dirty pages tracking.
2020-05-06 19:42 ` Kirti Wankhede
@ 2020-05-07 18:19 ` Alex Williamson
-1 siblings, 0 replies; 39+ messages in thread
From: Alex Williamson @ 2020-05-07 18:19 UTC (permalink / raw)
To: Kirti Wankhede
Cc: Yan Zhao, cjia, Tian, Kevin, Yang, Ziye, Liu, Changpeng, Liu,
Yi L, mlevitsk, eskultet, cohuck, dgilbert, jonathan.davies,
eauger, aik, pasic, felipe, Zhengxiao.zx, shuangtai.tst, Ken.Xue,
Wang, Zhi A, qemu-devel, kvm
On Thu, 7 May 2020 01:12:25 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:
> On 5/6/2020 1:45 PM, Yan Zhao wrote:
> > On Mon, May 04, 2020 at 11:58:56PM +0800, Kirti Wankhede wrote:
>
> <snip>
>
> >> /*
> >> * Helper Functions for host iova-pfn list
> >> */
> >> @@ -567,6 +654,18 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
> >> vfio_unpin_page_external(dma, iova, do_accounting);
> >> goto pin_unwind;
> >> }
> >> +
> >> + if (iommu->dirty_page_tracking) {
> >> + unsigned long pgshift =
> >> + __ffs(vfio_pgsize_bitmap(iommu));
> >> +
> > hi Kirti,
> > may I know if there's any vfio_pin_pages() happpening during NVidia's vGPU migration?
> > the code would enter into deadlock as I reported in last version.
> >
>
> Hm, you are right and same is the case in vfio_iommu_type1_dma_rw_chunk().
>
> Instead of calling vfio_pgsize_bitmap() from lots of places, I'm
> thinking of saving pgsize_bitmap in struct vfio_iommu, which should be
> populated whenever domain_list is updated. Alex, will that be fine?
I've wondered why we don't already cache this, so yes, that's fine, but
the cached value will only be valid when evaluated under iommu->lock.
Thanks,
Alex
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH Kernel v18 4/7] vfio iommu: Implementation of ioctl for dirty pages tracking.
@ 2020-05-07 18:19 ` Alex Williamson
0 siblings, 0 replies; 39+ messages in thread
From: Alex Williamson @ 2020-05-07 18:19 UTC (permalink / raw)
To: Kirti Wankhede
Cc: Zhengxiao.zx, Tian, Kevin, Liu, Yi L, cjia, kvm, eskultet, Yang,
Ziye, qemu-devel, cohuck, shuangtai.tst, dgilbert, Wang, Zhi A,
mlevitsk, pasic, aik, eauger, felipe, jonathan.davies, Yan Zhao,
Liu, Changpeng, Ken.Xue
On Thu, 7 May 2020 01:12:25 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:
> On 5/6/2020 1:45 PM, Yan Zhao wrote:
> > On Mon, May 04, 2020 at 11:58:56PM +0800, Kirti Wankhede wrote:
>
> <snip>
>
> >> /*
> >> * Helper Functions for host iova-pfn list
> >> */
> >> @@ -567,6 +654,18 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
> >> vfio_unpin_page_external(dma, iova, do_accounting);
> >> goto pin_unwind;
> >> }
> >> +
> >> + if (iommu->dirty_page_tracking) {
> >> + unsigned long pgshift =
> >> + __ffs(vfio_pgsize_bitmap(iommu));
> >> +
> > hi Kirti,
> > may I know if there's any vfio_pin_pages() happpening during NVidia's vGPU migration?
> > the code would enter into deadlock as I reported in last version.
> >
>
> Hm, you are right and same is the case in vfio_iommu_type1_dma_rw_chunk().
>
> Instead of calling vfio_pgsize_bitmap() from lots of places, I'm
> thinking of saving pgsize_bitmap in struct vfio_iommu, which should be
> populated whenever domain_list is updated. Alex, will that be fine?
I've wondered why we don't already cache this, so yes, that's fine, but
the cached value will only be valid when evaluated under iommu->lock.
Thanks,
Alex
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH Kernel v18 4/7] vfio iommu: Implementation of ioctl for dirty pages tracking.
2020-05-04 15:58 ` Kirti Wankhede
@ 2020-05-06 10:54 ` Cornelia Huck
-1 siblings, 0 replies; 39+ messages in thread
From: Cornelia Huck @ 2020-05-06 10:54 UTC (permalink / raw)
To: Kirti Wankhede
Cc: alex.williamson, cjia, kevin.tian, ziye.yang, changpeng.liu,
yi.l.liu, mlevitsk, eskultet, dgilbert, jonathan.davies, eauger,
aik, pasic, felipe, Zhengxiao.zx, shuangtai.tst, Ken.Xue,
zhi.a.wang, yan.y.zhao, qemu-devel, kvm
On Mon, 4 May 2020 21:28:56 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:
> VFIO_IOMMU_DIRTY_PAGES ioctl performs three operations:
> - Start dirty pages tracking while migration is active
> - Stop dirty pages tracking.
> - Get dirty pages bitmap. Its user space application's responsibility to
> copy content of dirty pages from source to destination during migration.
>
> To prevent DoS attack, memory for bitmap is allocated per vfio_dma
> structure. Bitmap size is calculated considering smallest supported page
> size. Bitmap is allocated for all vfio_dmas when dirty logging is enabled
>
> Bitmap is populated for already pinned pages when bitmap is allocated for
> a vfio_dma with the smallest supported page size. Update bitmap from
> pinning functions when tracking is enabled. When user application queries
> bitmap, check if requested page size is same as page size used to
> populated bitmap. If it is equal, copy bitmap, but if not equal, return
> error.
>
> Fixed below error by changing pgsize type from uint64_t to size_t.
> Reported-by: kbuild test robot <lkp@intel.com>
>
> All errors:
> drivers/vfio/vfio_iommu_type1.c:197: undefined reference to `__udivdi3'
>
> drivers/vfio/vfio_iommu_type1.c:225: undefined reference to `__udivdi3'
Move that below the '---' delimiter so that it does not end up in the
commit? (Crediting the build bot is fine, but the details are not
really useful when you look at the code later.)
>
> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> Reviewed-by: Neo Jia <cjia@nvidia.com>
> ---
> drivers/vfio/vfio_iommu_type1.c | 266 +++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 260 insertions(+), 6 deletions(-)
> @@ -2278,6 +2435,93 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>
> return copy_to_user((void __user *)arg, &unmap, minsz) ?
> -EFAULT : 0;
> + } else if (cmd == VFIO_IOMMU_DIRTY_PAGES) {
> + struct vfio_iommu_type1_dirty_bitmap dirty;
> + uint32_t mask = VFIO_IOMMU_DIRTY_PAGES_FLAG_START |
> + VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP |
> + VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP;
> + int ret = 0;
> +
> + if (!iommu->v2)
> + return -EACCES;
> +
> + minsz = offsetofend(struct vfio_iommu_type1_dirty_bitmap,
> + flags);
> +
> + if (copy_from_user(&dirty, (void __user *)arg, minsz))
> + return -EFAULT;
> +
> + if (dirty.argsz < minsz || dirty.flags & ~mask)
> + return -EINVAL;
> +
> + /* only one flag should be set at a time */
> + if (__ffs(dirty.flags) != __fls(dirty.flags))
> + return -EINVAL;
> +
Shouldn't you also check whether the flag that is set is actually
valid? (maybe dirty.flags & ~VFIO_IOMMU_DIRTY_PAGES_FLAG_MASK and do a
switch/case over dirty.flags & VFIO_IOMMU_DIRTY_PAGES_FLAG_MASK)
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH Kernel v18 4/7] vfio iommu: Implementation of ioctl for dirty pages tracking.
@ 2020-05-06 10:54 ` Cornelia Huck
0 siblings, 0 replies; 39+ messages in thread
From: Cornelia Huck @ 2020-05-06 10:54 UTC (permalink / raw)
To: Kirti Wankhede
Cc: kevin.tian, yi.l.liu, cjia, kvm, eskultet, ziye.yang, qemu-devel,
Zhengxiao.zx, shuangtai.tst, dgilbert, zhi.a.wang, mlevitsk,
pasic, aik, alex.williamson, eauger, felipe, jonathan.davies,
yan.y.zhao, changpeng.liu, Ken.Xue
On Mon, 4 May 2020 21:28:56 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:
> VFIO_IOMMU_DIRTY_PAGES ioctl performs three operations:
> - Start dirty pages tracking while migration is active
> - Stop dirty pages tracking.
> - Get dirty pages bitmap. Its user space application's responsibility to
> copy content of dirty pages from source to destination during migration.
>
> To prevent DoS attack, memory for bitmap is allocated per vfio_dma
> structure. Bitmap size is calculated considering smallest supported page
> size. Bitmap is allocated for all vfio_dmas when dirty logging is enabled
>
> Bitmap is populated for already pinned pages when bitmap is allocated for
> a vfio_dma with the smallest supported page size. Update bitmap from
> pinning functions when tracking is enabled. When user application queries
> bitmap, check if requested page size is same as page size used to
> populated bitmap. If it is equal, copy bitmap, but if not equal, return
> error.
>
> Fixed below error by changing pgsize type from uint64_t to size_t.
> Reported-by: kbuild test robot <lkp@intel.com>
>
> All errors:
> drivers/vfio/vfio_iommu_type1.c:197: undefined reference to `__udivdi3'
>
> drivers/vfio/vfio_iommu_type1.c:225: undefined reference to `__udivdi3'
Move that below the '---' delimiter so that it does not end up in the
commit? (Crediting the build bot is fine, but the details are not
really useful when you look at the code later.)
>
> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> Reviewed-by: Neo Jia <cjia@nvidia.com>
> ---
> drivers/vfio/vfio_iommu_type1.c | 266 +++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 260 insertions(+), 6 deletions(-)
> @@ -2278,6 +2435,93 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>
> return copy_to_user((void __user *)arg, &unmap, minsz) ?
> -EFAULT : 0;
> + } else if (cmd == VFIO_IOMMU_DIRTY_PAGES) {
> + struct vfio_iommu_type1_dirty_bitmap dirty;
> + uint32_t mask = VFIO_IOMMU_DIRTY_PAGES_FLAG_START |
> + VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP |
> + VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP;
> + int ret = 0;
> +
> + if (!iommu->v2)
> + return -EACCES;
> +
> + minsz = offsetofend(struct vfio_iommu_type1_dirty_bitmap,
> + flags);
> +
> + if (copy_from_user(&dirty, (void __user *)arg, minsz))
> + return -EFAULT;
> +
> + if (dirty.argsz < minsz || dirty.flags & ~mask)
> + return -EINVAL;
> +
> + /* only one flag should be set at a time */
> + if (__ffs(dirty.flags) != __fls(dirty.flags))
> + return -EINVAL;
> +
Shouldn't you also check whether the flag that is set is actually
valid? (maybe dirty.flags & ~VFIO_IOMMU_DIRTY_PAGES_FLAG_MASK and do a
switch/case over dirty.flags & VFIO_IOMMU_DIRTY_PAGES_FLAG_MASK)
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH Kernel v18 4/7] vfio iommu: Implementation of ioctl for dirty pages tracking.
2020-05-06 10:54 ` Cornelia Huck
@ 2020-05-13 20:26 ` Kirti Wankhede
-1 siblings, 0 replies; 39+ messages in thread
From: Kirti Wankhede @ 2020-05-13 20:26 UTC (permalink / raw)
To: Cornelia Huck
Cc: alex.williamson, cjia, kevin.tian, ziye.yang, changpeng.liu,
yi.l.liu, mlevitsk, eskultet, dgilbert, jonathan.davies, eauger,
aik, pasic, felipe, Zhengxiao.zx, shuangtai.tst, Ken.Xue,
zhi.a.wang, yan.y.zhao, qemu-devel, kvm
On 5/6/2020 4:24 PM, Cornelia Huck wrote:
> On Mon, 4 May 2020 21:28:56 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>
>> VFIO_IOMMU_DIRTY_PAGES ioctl performs three operations:
>> - Start dirty pages tracking while migration is active
>> - Stop dirty pages tracking.
>> - Get dirty pages bitmap. Its user space application's responsibility to
>> copy content of dirty pages from source to destination during migration.
>>
>> To prevent DoS attack, memory for bitmap is allocated per vfio_dma
>> structure. Bitmap size is calculated considering smallest supported page
>> size. Bitmap is allocated for all vfio_dmas when dirty logging is enabled
>>
>> Bitmap is populated for already pinned pages when bitmap is allocated for
>> a vfio_dma with the smallest supported page size. Update bitmap from
>> pinning functions when tracking is enabled. When user application queries
>> bitmap, check if requested page size is same as page size used to
>> populated bitmap. If it is equal, copy bitmap, but if not equal, return
>> error.
>>
>> Fixed below error by changing pgsize type from uint64_t to size_t.
>> Reported-by: kbuild test robot <lkp@intel.com>
>>
>> All errors:
>> drivers/vfio/vfio_iommu_type1.c:197: undefined reference to `__udivdi3'
>>
>> drivers/vfio/vfio_iommu_type1.c:225: undefined reference to `__udivdi3'
>
> Move that below the '---' delimiter so that it does not end up in the
> commit? (Crediting the build bot is fine, but the details are not
> really useful when you look at the code later.)
>
ok, removing errors.
>>
>> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
>> Reviewed-by: Neo Jia <cjia@nvidia.com>
>> ---
>> drivers/vfio/vfio_iommu_type1.c | 266 +++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 260 insertions(+), 6 deletions(-)
>
>> @@ -2278,6 +2435,93 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>>
>> return copy_to_user((void __user *)arg, &unmap, minsz) ?
>> -EFAULT : 0;
>> + } else if (cmd == VFIO_IOMMU_DIRTY_PAGES) {
>> + struct vfio_iommu_type1_dirty_bitmap dirty;
>> + uint32_t mask = VFIO_IOMMU_DIRTY_PAGES_FLAG_START |
>> + VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP |
>> + VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP;
>> + int ret = 0;
>> +
>> + if (!iommu->v2)
>> + return -EACCES;
>> +
>> + minsz = offsetofend(struct vfio_iommu_type1_dirty_bitmap,
>> + flags);
>> +
>> + if (copy_from_user(&dirty, (void __user *)arg, minsz))
>> + return -EFAULT;
>> +
>> + if (dirty.argsz < minsz || dirty.flags & ~mask)
>> + return -EINVAL;
>> +
>> + /* only one flag should be set at a time */
>> + if (__ffs(dirty.flags) != __fls(dirty.flags))
>> + return -EINVAL;
>> +
>
> Shouldn't you also check whether the flag that is set is actually
> valid? (maybe dirty.flags & ~VFIO_IOMMU_DIRTY_PAGES_FLAG_MASK and do a
> switch/case over dirty.flags & VFIO_IOMMU_DIRTY_PAGES_FLAG_MASK)
>
There is a check above this check, dirty.flags & ~mask, which makes sure
that flag is valid.
Thanks,
Kirti
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH Kernel v18 4/7] vfio iommu: Implementation of ioctl for dirty pages tracking.
@ 2020-05-13 20:26 ` Kirti Wankhede
0 siblings, 0 replies; 39+ messages in thread
From: Kirti Wankhede @ 2020-05-13 20:26 UTC (permalink / raw)
To: Cornelia Huck
Cc: kevin.tian, yi.l.liu, cjia, kvm, eskultet, ziye.yang, qemu-devel,
Zhengxiao.zx, shuangtai.tst, dgilbert, zhi.a.wang, mlevitsk,
pasic, aik, alex.williamson, eauger, felipe, jonathan.davies,
yan.y.zhao, changpeng.liu, Ken.Xue
On 5/6/2020 4:24 PM, Cornelia Huck wrote:
> On Mon, 4 May 2020 21:28:56 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>
>> VFIO_IOMMU_DIRTY_PAGES ioctl performs three operations:
>> - Start dirty pages tracking while migration is active
>> - Stop dirty pages tracking.
>> - Get dirty pages bitmap. Its user space application's responsibility to
>> copy content of dirty pages from source to destination during migration.
>>
>> To prevent DoS attack, memory for bitmap is allocated per vfio_dma
>> structure. Bitmap size is calculated considering smallest supported page
>> size. Bitmap is allocated for all vfio_dmas when dirty logging is enabled
>>
>> Bitmap is populated for already pinned pages when bitmap is allocated for
>> a vfio_dma with the smallest supported page size. Update bitmap from
>> pinning functions when tracking is enabled. When user application queries
>> bitmap, check if requested page size is same as page size used to
>> populated bitmap. If it is equal, copy bitmap, but if not equal, return
>> error.
>>
>> Fixed below error by changing pgsize type from uint64_t to size_t.
>> Reported-by: kbuild test robot <lkp@intel.com>
>>
>> All errors:
>> drivers/vfio/vfio_iommu_type1.c:197: undefined reference to `__udivdi3'
>>
>> drivers/vfio/vfio_iommu_type1.c:225: undefined reference to `__udivdi3'
>
> Move that below the '---' delimiter so that it does not end up in the
> commit? (Crediting the build bot is fine, but the details are not
> really useful when you look at the code later.)
>
ok, removing errors.
>>
>> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
>> Reviewed-by: Neo Jia <cjia@nvidia.com>
>> ---
>> drivers/vfio/vfio_iommu_type1.c | 266 +++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 260 insertions(+), 6 deletions(-)
>
>> @@ -2278,6 +2435,93 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>>
>> return copy_to_user((void __user *)arg, &unmap, minsz) ?
>> -EFAULT : 0;
>> + } else if (cmd == VFIO_IOMMU_DIRTY_PAGES) {
>> + struct vfio_iommu_type1_dirty_bitmap dirty;
>> + uint32_t mask = VFIO_IOMMU_DIRTY_PAGES_FLAG_START |
>> + VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP |
>> + VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP;
>> + int ret = 0;
>> +
>> + if (!iommu->v2)
>> + return -EACCES;
>> +
>> + minsz = offsetofend(struct vfio_iommu_type1_dirty_bitmap,
>> + flags);
>> +
>> + if (copy_from_user(&dirty, (void __user *)arg, minsz))
>> + return -EFAULT;
>> +
>> + if (dirty.argsz < minsz || dirty.flags & ~mask)
>> + return -EINVAL;
>> +
>> + /* only one flag should be set at a time */
>> + if (__ffs(dirty.flags) != __fls(dirty.flags))
>> + return -EINVAL;
>> +
>
> Shouldn't you also check whether the flag that is set is actually
> valid? (maybe dirty.flags & ~VFIO_IOMMU_DIRTY_PAGES_FLAG_MASK and do a
> switch/case over dirty.flags & VFIO_IOMMU_DIRTY_PAGES_FLAG_MASK)
>
There is a check above this check, dirty.flags & ~mask, which makes sure
that flag is valid.
Thanks,
Kirti
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH Kernel v18 5/7] vfio iommu: Update UNMAP_DMA ioctl to get dirty bitmap before unmap
2020-05-04 15:58 ` Kirti Wankhede
@ 2020-05-04 15:58 ` Kirti Wankhede
-1 siblings, 0 replies; 39+ messages in thread
From: Kirti Wankhede @ 2020-05-04 15:58 UTC (permalink / raw)
To: alex.williamson, cjia
Cc: kevin.tian, ziye.yang, changpeng.liu, yi.l.liu, mlevitsk,
eskultet, cohuck, dgilbert, jonathan.davies, eauger, aik, pasic,
felipe, Zhengxiao.zx, shuangtai.tst, Ken.Xue, zhi.a.wang,
yan.y.zhao, qemu-devel, kvm, Kirti Wankhede
DMA mapped pages, including those pinned by mdev vendor drivers, might
get unpinned and unmapped while migration is active and device is still
running. For example, in pre-copy phase while guest driver could access
those pages, host device or vendor driver can dirty these mapped pages.
Such pages should be marked dirty so as to maintain memory consistency
for a user making use of dirty page tracking.
To get bitmap during unmap, user should allocate memory for bitmap, set
size of allocated memory, set page size to be considered for bitmap and
set flag VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP.
Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
Reviewed-by: Neo Jia <cjia@nvidia.com>
---
drivers/vfio/vfio_iommu_type1.c | 84 +++++++++++++++++++++++++++++++++++++++--
include/uapi/linux/vfio.h | 10 +++++
2 files changed, 90 insertions(+), 4 deletions(-)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 01dcb417836f..8b27faf1ec38 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -983,12 +983,14 @@ static int verify_bitmap_size(uint64_t npages, uint64_t bitmap_size)
}
static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
- struct vfio_iommu_type1_dma_unmap *unmap)
+ struct vfio_iommu_type1_dma_unmap *unmap,
+ struct vfio_bitmap *bitmap)
{
uint64_t mask;
struct vfio_dma *dma, *dma_last = NULL;
size_t unmapped = 0;
int ret = 0, retries = 0;
+ unsigned long *final_bitmap = NULL, *temp_bitmap = NULL;
mask = ((uint64_t)1 << __ffs(vfio_pgsize_bitmap(iommu))) - 1;
@@ -1041,6 +1043,7 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
ret = -EINVAL;
goto unlock;
}
+
dma = vfio_find_dma(iommu, unmap->iova + unmap->size - 1, 0);
if (dma && dma->iova + dma->size != unmap->iova + unmap->size) {
ret = -EINVAL;
@@ -1048,6 +1051,22 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
}
}
+ if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) &&
+ iommu->dirty_page_tracking) {
+ final_bitmap = kvzalloc(bitmap->size, GFP_KERNEL);
+ if (!final_bitmap) {
+ ret = -ENOMEM;
+ goto unlock;
+ }
+
+ temp_bitmap = kvzalloc(bitmap->size, GFP_KERNEL);
+ if (!temp_bitmap) {
+ ret = -ENOMEM;
+ kfree(final_bitmap);
+ goto unlock;
+ }
+ }
+
while ((dma = vfio_find_dma(iommu, unmap->iova, unmap->size))) {
if (!iommu->v2 && unmap->iova > dma->iova)
break;
@@ -1058,6 +1077,24 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
if (dma->task->mm != current->mm)
break;
+ if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) &&
+ iommu->dirty_page_tracking) {
+ unsigned long pgshift = __ffs(bitmap->pgsize);
+ unsigned int npages = dma->size >> pgshift;
+ unsigned int shift;
+
+ vfio_iova_dirty_bitmap(iommu, dma->iova, dma->size,
+ bitmap->pgsize, (u64 *)temp_bitmap);
+
+ shift = (dma->iova - unmap->iova) >> pgshift;
+ if (shift)
+ bitmap_shift_left(temp_bitmap, temp_bitmap,
+ shift, npages);
+ bitmap_or(final_bitmap, final_bitmap, temp_bitmap,
+ shift + npages);
+ memset(temp_bitmap, 0, bitmap->size);
+ }
+
if (!RB_EMPTY_ROOT(&dma->pfn_list)) {
struct vfio_iommu_type1_dma_unmap nb_unmap;
@@ -1088,6 +1125,16 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
}
unlock:
+ if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) &&
+ iommu->dirty_page_tracking && final_bitmap) {
+ if (copy_to_user((void __user *)bitmap->data, final_bitmap,
+ bitmap->size))
+ ret = -EFAULT;
+
+ kfree(final_bitmap);
+ kfree(temp_bitmap);
+ }
+
mutex_unlock(&iommu->lock);
/* Report how much was unmapped */
@@ -2419,17 +2466,46 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
} else if (cmd == VFIO_IOMMU_UNMAP_DMA) {
struct vfio_iommu_type1_dma_unmap unmap;
- long ret;
+ struct vfio_bitmap bitmap = { 0 };
+ int ret;
minsz = offsetofend(struct vfio_iommu_type1_dma_unmap, size);
if (copy_from_user(&unmap, (void __user *)arg, minsz))
return -EFAULT;
- if (unmap.argsz < minsz || unmap.flags)
+ if (unmap.argsz < minsz ||
+ unmap.flags & ~VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP)
return -EINVAL;
- ret = vfio_dma_do_unmap(iommu, &unmap);
+ if (unmap.flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) {
+ unsigned long pgshift;
+ size_t iommu_pgsize =
+ (size_t)1 << __ffs(vfio_pgsize_bitmap(iommu));
+
+ if (unmap.argsz < (minsz + sizeof(bitmap)))
+ return -EINVAL;
+
+ if (copy_from_user(&bitmap,
+ (void __user *)(arg + minsz),
+ sizeof(bitmap)))
+ return -EFAULT;
+
+ /* allow only min supported pgsize */
+ if (bitmap.pgsize != iommu_pgsize)
+ return -EINVAL;
+ if (!access_ok((void __user *)bitmap.data, bitmap.size))
+ return -EINVAL;
+
+ pgshift = __ffs(bitmap.pgsize);
+ ret = verify_bitmap_size(unmap.size >> pgshift,
+ bitmap.size);
+ if (ret)
+ return ret;
+
+ }
+
+ ret = vfio_dma_do_unmap(iommu, &unmap, &bitmap);
if (ret)
return ret;
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 5f359c63f5ef..e3cbf8b78623 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -1048,12 +1048,22 @@ struct vfio_bitmap {
* field. No guarantee is made to the user that arbitrary unmaps of iova
* or size different from those used in the original mapping call will
* succeed.
+ * VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP should be set to get dirty bitmap
+ * before unmapping IO virtual addresses. When this flag is set, user must
+ * provide data[] as structure vfio_bitmap. User must allocate memory to get
+ * bitmap and must set size of allocated memory in vfio_bitmap.size field.
+ * A bit in bitmap represents one page of user provided page size in 'pgsize',
+ * consecutively starting from iova offset. Bit set indicates page at that
+ * offset from iova is dirty. Bitmap of pages in the range of unmapped size is
+ * returned in vfio_bitmap.data
*/
struct vfio_iommu_type1_dma_unmap {
__u32 argsz;
__u32 flags;
+#define VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP (1 << 0)
__u64 iova; /* IO virtual address */
__u64 size; /* Size of mapping (bytes) */
+ __u8 data[];
};
#define VFIO_IOMMU_UNMAP_DMA _IO(VFIO_TYPE, VFIO_BASE + 14)
--
2.7.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH Kernel v18 5/7] vfio iommu: Update UNMAP_DMA ioctl to get dirty bitmap before unmap
@ 2020-05-04 15:58 ` Kirti Wankhede
0 siblings, 0 replies; 39+ messages in thread
From: Kirti Wankhede @ 2020-05-04 15:58 UTC (permalink / raw)
To: alex.williamson, cjia
Cc: Zhengxiao.zx, kevin.tian, yi.l.liu, yan.y.zhao, kvm, eskultet,
ziye.yang, qemu-devel, cohuck, shuangtai.tst, dgilbert,
zhi.a.wang, mlevitsk, pasic, aik, Kirti Wankhede, eauger, felipe,
jonathan.davies, changpeng.liu, Ken.Xue
DMA mapped pages, including those pinned by mdev vendor drivers, might
get unpinned and unmapped while migration is active and device is still
running. For example, in pre-copy phase while guest driver could access
those pages, host device or vendor driver can dirty these mapped pages.
Such pages should be marked dirty so as to maintain memory consistency
for a user making use of dirty page tracking.
To get bitmap during unmap, user should allocate memory for bitmap, set
size of allocated memory, set page size to be considered for bitmap and
set flag VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP.
Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
Reviewed-by: Neo Jia <cjia@nvidia.com>
---
drivers/vfio/vfio_iommu_type1.c | 84 +++++++++++++++++++++++++++++++++++++++--
include/uapi/linux/vfio.h | 10 +++++
2 files changed, 90 insertions(+), 4 deletions(-)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 01dcb417836f..8b27faf1ec38 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -983,12 +983,14 @@ static int verify_bitmap_size(uint64_t npages, uint64_t bitmap_size)
}
static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
- struct vfio_iommu_type1_dma_unmap *unmap)
+ struct vfio_iommu_type1_dma_unmap *unmap,
+ struct vfio_bitmap *bitmap)
{
uint64_t mask;
struct vfio_dma *dma, *dma_last = NULL;
size_t unmapped = 0;
int ret = 0, retries = 0;
+ unsigned long *final_bitmap = NULL, *temp_bitmap = NULL;
mask = ((uint64_t)1 << __ffs(vfio_pgsize_bitmap(iommu))) - 1;
@@ -1041,6 +1043,7 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
ret = -EINVAL;
goto unlock;
}
+
dma = vfio_find_dma(iommu, unmap->iova + unmap->size - 1, 0);
if (dma && dma->iova + dma->size != unmap->iova + unmap->size) {
ret = -EINVAL;
@@ -1048,6 +1051,22 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
}
}
+ if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) &&
+ iommu->dirty_page_tracking) {
+ final_bitmap = kvzalloc(bitmap->size, GFP_KERNEL);
+ if (!final_bitmap) {
+ ret = -ENOMEM;
+ goto unlock;
+ }
+
+ temp_bitmap = kvzalloc(bitmap->size, GFP_KERNEL);
+ if (!temp_bitmap) {
+ ret = -ENOMEM;
+ kfree(final_bitmap);
+ goto unlock;
+ }
+ }
+
while ((dma = vfio_find_dma(iommu, unmap->iova, unmap->size))) {
if (!iommu->v2 && unmap->iova > dma->iova)
break;
@@ -1058,6 +1077,24 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
if (dma->task->mm != current->mm)
break;
+ if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) &&
+ iommu->dirty_page_tracking) {
+ unsigned long pgshift = __ffs(bitmap->pgsize);
+ unsigned int npages = dma->size >> pgshift;
+ unsigned int shift;
+
+ vfio_iova_dirty_bitmap(iommu, dma->iova, dma->size,
+ bitmap->pgsize, (u64 *)temp_bitmap);
+
+ shift = (dma->iova - unmap->iova) >> pgshift;
+ if (shift)
+ bitmap_shift_left(temp_bitmap, temp_bitmap,
+ shift, npages);
+ bitmap_or(final_bitmap, final_bitmap, temp_bitmap,
+ shift + npages);
+ memset(temp_bitmap, 0, bitmap->size);
+ }
+
if (!RB_EMPTY_ROOT(&dma->pfn_list)) {
struct vfio_iommu_type1_dma_unmap nb_unmap;
@@ -1088,6 +1125,16 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
}
unlock:
+ if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) &&
+ iommu->dirty_page_tracking && final_bitmap) {
+ if (copy_to_user((void __user *)bitmap->data, final_bitmap,
+ bitmap->size))
+ ret = -EFAULT;
+
+ kfree(final_bitmap);
+ kfree(temp_bitmap);
+ }
+
mutex_unlock(&iommu->lock);
/* Report how much was unmapped */
@@ -2419,17 +2466,46 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
} else if (cmd == VFIO_IOMMU_UNMAP_DMA) {
struct vfio_iommu_type1_dma_unmap unmap;
- long ret;
+ struct vfio_bitmap bitmap = { 0 };
+ int ret;
minsz = offsetofend(struct vfio_iommu_type1_dma_unmap, size);
if (copy_from_user(&unmap, (void __user *)arg, minsz))
return -EFAULT;
- if (unmap.argsz < minsz || unmap.flags)
+ if (unmap.argsz < minsz ||
+ unmap.flags & ~VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP)
return -EINVAL;
- ret = vfio_dma_do_unmap(iommu, &unmap);
+ if (unmap.flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) {
+ unsigned long pgshift;
+ size_t iommu_pgsize =
+ (size_t)1 << __ffs(vfio_pgsize_bitmap(iommu));
+
+ if (unmap.argsz < (minsz + sizeof(bitmap)))
+ return -EINVAL;
+
+ if (copy_from_user(&bitmap,
+ (void __user *)(arg + minsz),
+ sizeof(bitmap)))
+ return -EFAULT;
+
+ /* allow only min supported pgsize */
+ if (bitmap.pgsize != iommu_pgsize)
+ return -EINVAL;
+ if (!access_ok((void __user *)bitmap.data, bitmap.size))
+ return -EINVAL;
+
+ pgshift = __ffs(bitmap.pgsize);
+ ret = verify_bitmap_size(unmap.size >> pgshift,
+ bitmap.size);
+ if (ret)
+ return ret;
+
+ }
+
+ ret = vfio_dma_do_unmap(iommu, &unmap, &bitmap);
if (ret)
return ret;
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 5f359c63f5ef..e3cbf8b78623 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -1048,12 +1048,22 @@ struct vfio_bitmap {
* field. No guarantee is made to the user that arbitrary unmaps of iova
* or size different from those used in the original mapping call will
* succeed.
+ * VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP should be set to get dirty bitmap
+ * before unmapping IO virtual addresses. When this flag is set, user must
+ * provide data[] as structure vfio_bitmap. User must allocate memory to get
+ * bitmap and must set size of allocated memory in vfio_bitmap.size field.
+ * A bit in bitmap represents one page of user provided page size in 'pgsize',
+ * consecutively starting from iova offset. Bit set indicates page at that
+ * offset from iova is dirty. Bitmap of pages in the range of unmapped size is
+ * returned in vfio_bitmap.data
*/
struct vfio_iommu_type1_dma_unmap {
__u32 argsz;
__u32 flags;
+#define VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP (1 << 0)
__u64 iova; /* IO virtual address */
__u64 size; /* Size of mapping (bytes) */
+ __u8 data[];
};
#define VFIO_IOMMU_UNMAP_DMA _IO(VFIO_TYPE, VFIO_BASE + 14)
--
2.7.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH Kernel v18 5/7] vfio iommu: Update UNMAP_DMA ioctl to get dirty bitmap before unmap
2020-05-04 15:58 ` Kirti Wankhede
@ 2020-05-06 22:25 ` Alex Williamson
-1 siblings, 0 replies; 39+ messages in thread
From: Alex Williamson @ 2020-05-06 22:25 UTC (permalink / raw)
To: Kirti Wankhede
Cc: cjia, kevin.tian, ziye.yang, changpeng.liu, yi.l.liu, mlevitsk,
eskultet, cohuck, dgilbert, jonathan.davies, eauger, aik, pasic,
felipe, Zhengxiao.zx, shuangtai.tst, Ken.Xue, zhi.a.wang,
yan.y.zhao, qemu-devel, kvm
On Mon, 4 May 2020 21:28:57 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:
> DMA mapped pages, including those pinned by mdev vendor drivers, might
> get unpinned and unmapped while migration is active and device is still
> running. For example, in pre-copy phase while guest driver could access
> those pages, host device or vendor driver can dirty these mapped pages.
> Such pages should be marked dirty so as to maintain memory consistency
> for a user making use of dirty page tracking.
>
> To get bitmap during unmap, user should allocate memory for bitmap, set
> size of allocated memory, set page size to be considered for bitmap and
> set flag VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP.
>
> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> Reviewed-by: Neo Jia <cjia@nvidia.com>
> ---
> drivers/vfio/vfio_iommu_type1.c | 84 +++++++++++++++++++++++++++++++++++++++--
> include/uapi/linux/vfio.h | 10 +++++
> 2 files changed, 90 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 01dcb417836f..8b27faf1ec38 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -983,12 +983,14 @@ static int verify_bitmap_size(uint64_t npages, uint64_t bitmap_size)
> }
>
> static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> - struct vfio_iommu_type1_dma_unmap *unmap)
> + struct vfio_iommu_type1_dma_unmap *unmap,
> + struct vfio_bitmap *bitmap)
> {
> uint64_t mask;
> struct vfio_dma *dma, *dma_last = NULL;
> size_t unmapped = 0;
> int ret = 0, retries = 0;
> + unsigned long *final_bitmap = NULL, *temp_bitmap = NULL;
>
> mask = ((uint64_t)1 << __ffs(vfio_pgsize_bitmap(iommu))) - 1;
>
> @@ -1041,6 +1043,7 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> ret = -EINVAL;
> goto unlock;
> }
> +
> dma = vfio_find_dma(iommu, unmap->iova + unmap->size - 1, 0);
> if (dma && dma->iova + dma->size != unmap->iova + unmap->size) {
> ret = -EINVAL;
> @@ -1048,6 +1051,22 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> }
> }
>
> + if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) &&
> + iommu->dirty_page_tracking) {
Why do we even accept VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP when not
dirty page tracking rather than returning -EINVAL? It would simplify
things here to reject it at the ioctl and silently ignoring a flag is
rarely if ever the right approach.
> + final_bitmap = kvzalloc(bitmap->size, GFP_KERNEL);
> + if (!final_bitmap) {
> + ret = -ENOMEM;
> + goto unlock;
> + }
> +
> + temp_bitmap = kvzalloc(bitmap->size, GFP_KERNEL);
> + if (!temp_bitmap) {
> + ret = -ENOMEM;
> + kfree(final_bitmap);
> + goto unlock;
> + }
YIKES! So the user can instantly trigger the kernel to internally
allocate 2 x 256MB, regardless of how much they can actually map.
> + }
> +
> while ((dma = vfio_find_dma(iommu, unmap->iova, unmap->size))) {
> if (!iommu->v2 && unmap->iova > dma->iova)
> break;
> @@ -1058,6 +1077,24 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> if (dma->task->mm != current->mm)
> break;
>
> + if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) &&
> + iommu->dirty_page_tracking) {
> + unsigned long pgshift = __ffs(bitmap->pgsize);
> + unsigned int npages = dma->size >> pgshift;
> + unsigned int shift;
> +
> + vfio_iova_dirty_bitmap(iommu, dma->iova, dma->size,
> + bitmap->pgsize, (u64 *)temp_bitmap);
vfio_iova_dirty_bitmap() takes a __user bitmap, we're doing
copy_to_user() on a kernel allocated buffer???
> +
> + shift = (dma->iova - unmap->iova) >> pgshift;
> + if (shift)
> + bitmap_shift_left(temp_bitmap, temp_bitmap,
> + shift, npages);
> + bitmap_or(final_bitmap, final_bitmap, temp_bitmap,
> + shift + npages);
> + memset(temp_bitmap, 0, bitmap->size);
> + }
It seems like if the per vfio_dma dirty bitmap was oversized by a long
that we could shift it in place, then we'd only need one working bitmap
buffer and we could size that to fit the vfio_dma (or the largest
vfio_dma if we don't want to free and re-alloc for each vfio_dma).
We'd need to do more copy_to/from_user()s, but we'd also avoid copying
between sparse mappings (user zero'd bitmap required) and we'd have a
far more reasonable memory usage. Thanks,
Alex
> +
> if (!RB_EMPTY_ROOT(&dma->pfn_list)) {
> struct vfio_iommu_type1_dma_unmap nb_unmap;
>
> @@ -1088,6 +1125,16 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> }
>
> unlock:
> + if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) &&
> + iommu->dirty_page_tracking && final_bitmap) {
> + if (copy_to_user((void __user *)bitmap->data, final_bitmap,
> + bitmap->size))
> + ret = -EFAULT;
> +
> + kfree(final_bitmap);
> + kfree(temp_bitmap);
> + }
> +
> mutex_unlock(&iommu->lock);
>
> /* Report how much was unmapped */
> @@ -2419,17 +2466,46 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>
> } else if (cmd == VFIO_IOMMU_UNMAP_DMA) {
> struct vfio_iommu_type1_dma_unmap unmap;
> - long ret;
> + struct vfio_bitmap bitmap = { 0 };
> + int ret;
>
> minsz = offsetofend(struct vfio_iommu_type1_dma_unmap, size);
>
> if (copy_from_user(&unmap, (void __user *)arg, minsz))
> return -EFAULT;
>
> - if (unmap.argsz < minsz || unmap.flags)
> + if (unmap.argsz < minsz ||
> + unmap.flags & ~VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP)
> return -EINVAL;
>
> - ret = vfio_dma_do_unmap(iommu, &unmap);
> + if (unmap.flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) {
> + unsigned long pgshift;
> + size_t iommu_pgsize =
> + (size_t)1 << __ffs(vfio_pgsize_bitmap(iommu));
> +
> + if (unmap.argsz < (minsz + sizeof(bitmap)))
> + return -EINVAL;
> +
> + if (copy_from_user(&bitmap,
> + (void __user *)(arg + minsz),
> + sizeof(bitmap)))
> + return -EFAULT;
> +
> + /* allow only min supported pgsize */
> + if (bitmap.pgsize != iommu_pgsize)
> + return -EINVAL;
> + if (!access_ok((void __user *)bitmap.data, bitmap.size))
> + return -EINVAL;
> +
> + pgshift = __ffs(bitmap.pgsize);
> + ret = verify_bitmap_size(unmap.size >> pgshift,
> + bitmap.size);
> + if (ret)
> + return ret;
> +
> + }
> +
> + ret = vfio_dma_do_unmap(iommu, &unmap, &bitmap);
> if (ret)
> return ret;
>
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 5f359c63f5ef..e3cbf8b78623 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -1048,12 +1048,22 @@ struct vfio_bitmap {
> * field. No guarantee is made to the user that arbitrary unmaps of iova
> * or size different from those used in the original mapping call will
> * succeed.
> + * VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP should be set to get dirty bitmap
> + * before unmapping IO virtual addresses. When this flag is set, user must
> + * provide data[] as structure vfio_bitmap. User must allocate memory to get
> + * bitmap and must set size of allocated memory in vfio_bitmap.size field.
> + * A bit in bitmap represents one page of user provided page size in 'pgsize',
> + * consecutively starting from iova offset. Bit set indicates page at that
> + * offset from iova is dirty. Bitmap of pages in the range of unmapped size is
> + * returned in vfio_bitmap.data
> */
> struct vfio_iommu_type1_dma_unmap {
> __u32 argsz;
> __u32 flags;
> +#define VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP (1 << 0)
> __u64 iova; /* IO virtual address */
> __u64 size; /* Size of mapping (bytes) */
> + __u8 data[];
> };
>
> #define VFIO_IOMMU_UNMAP_DMA _IO(VFIO_TYPE, VFIO_BASE + 14)
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH Kernel v18 5/7] vfio iommu: Update UNMAP_DMA ioctl to get dirty bitmap before unmap
@ 2020-05-06 22:25 ` Alex Williamson
0 siblings, 0 replies; 39+ messages in thread
From: Alex Williamson @ 2020-05-06 22:25 UTC (permalink / raw)
To: Kirti Wankhede
Cc: Zhengxiao.zx, kevin.tian, yi.l.liu, cjia, kvm, eskultet,
ziye.yang, qemu-devel, cohuck, shuangtai.tst, dgilbert,
zhi.a.wang, mlevitsk, pasic, aik, eauger, felipe,
jonathan.davies, yan.y.zhao, changpeng.liu, Ken.Xue
On Mon, 4 May 2020 21:28:57 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:
> DMA mapped pages, including those pinned by mdev vendor drivers, might
> get unpinned and unmapped while migration is active and device is still
> running. For example, in pre-copy phase while guest driver could access
> those pages, host device or vendor driver can dirty these mapped pages.
> Such pages should be marked dirty so as to maintain memory consistency
> for a user making use of dirty page tracking.
>
> To get bitmap during unmap, user should allocate memory for bitmap, set
> size of allocated memory, set page size to be considered for bitmap and
> set flag VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP.
>
> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> Reviewed-by: Neo Jia <cjia@nvidia.com>
> ---
> drivers/vfio/vfio_iommu_type1.c | 84 +++++++++++++++++++++++++++++++++++++++--
> include/uapi/linux/vfio.h | 10 +++++
> 2 files changed, 90 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 01dcb417836f..8b27faf1ec38 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -983,12 +983,14 @@ static int verify_bitmap_size(uint64_t npages, uint64_t bitmap_size)
> }
>
> static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> - struct vfio_iommu_type1_dma_unmap *unmap)
> + struct vfio_iommu_type1_dma_unmap *unmap,
> + struct vfio_bitmap *bitmap)
> {
> uint64_t mask;
> struct vfio_dma *dma, *dma_last = NULL;
> size_t unmapped = 0;
> int ret = 0, retries = 0;
> + unsigned long *final_bitmap = NULL, *temp_bitmap = NULL;
>
> mask = ((uint64_t)1 << __ffs(vfio_pgsize_bitmap(iommu))) - 1;
>
> @@ -1041,6 +1043,7 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> ret = -EINVAL;
> goto unlock;
> }
> +
> dma = vfio_find_dma(iommu, unmap->iova + unmap->size - 1, 0);
> if (dma && dma->iova + dma->size != unmap->iova + unmap->size) {
> ret = -EINVAL;
> @@ -1048,6 +1051,22 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> }
> }
>
> + if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) &&
> + iommu->dirty_page_tracking) {
Why do we even accept VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP when not
dirty page tracking rather than returning -EINVAL? It would simplify
things here to reject it at the ioctl and silently ignoring a flag is
rarely if ever the right approach.
> + final_bitmap = kvzalloc(bitmap->size, GFP_KERNEL);
> + if (!final_bitmap) {
> + ret = -ENOMEM;
> + goto unlock;
> + }
> +
> + temp_bitmap = kvzalloc(bitmap->size, GFP_KERNEL);
> + if (!temp_bitmap) {
> + ret = -ENOMEM;
> + kfree(final_bitmap);
> + goto unlock;
> + }
YIKES! So the user can instantly trigger the kernel to internally
allocate 2 x 256MB, regardless of how much they can actually map.
> + }
> +
> while ((dma = vfio_find_dma(iommu, unmap->iova, unmap->size))) {
> if (!iommu->v2 && unmap->iova > dma->iova)
> break;
> @@ -1058,6 +1077,24 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> if (dma->task->mm != current->mm)
> break;
>
> + if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) &&
> + iommu->dirty_page_tracking) {
> + unsigned long pgshift = __ffs(bitmap->pgsize);
> + unsigned int npages = dma->size >> pgshift;
> + unsigned int shift;
> +
> + vfio_iova_dirty_bitmap(iommu, dma->iova, dma->size,
> + bitmap->pgsize, (u64 *)temp_bitmap);
vfio_iova_dirty_bitmap() takes a __user bitmap, we're doing
copy_to_user() on a kernel allocated buffer???
> +
> + shift = (dma->iova - unmap->iova) >> pgshift;
> + if (shift)
> + bitmap_shift_left(temp_bitmap, temp_bitmap,
> + shift, npages);
> + bitmap_or(final_bitmap, final_bitmap, temp_bitmap,
> + shift + npages);
> + memset(temp_bitmap, 0, bitmap->size);
> + }
It seems like if the per vfio_dma dirty bitmap was oversized by a long
that we could shift it in place, then we'd only need one working bitmap
buffer and we could size that to fit the vfio_dma (or the largest
vfio_dma if we don't want to free and re-alloc for each vfio_dma).
We'd need to do more copy_to/from_user()s, but we'd also avoid copying
between sparse mappings (user zero'd bitmap required) and we'd have a
far more reasonable memory usage. Thanks,
Alex
> +
> if (!RB_EMPTY_ROOT(&dma->pfn_list)) {
> struct vfio_iommu_type1_dma_unmap nb_unmap;
>
> @@ -1088,6 +1125,16 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> }
>
> unlock:
> + if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) &&
> + iommu->dirty_page_tracking && final_bitmap) {
> + if (copy_to_user((void __user *)bitmap->data, final_bitmap,
> + bitmap->size))
> + ret = -EFAULT;
> +
> + kfree(final_bitmap);
> + kfree(temp_bitmap);
> + }
> +
> mutex_unlock(&iommu->lock);
>
> /* Report how much was unmapped */
> @@ -2419,17 +2466,46 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>
> } else if (cmd == VFIO_IOMMU_UNMAP_DMA) {
> struct vfio_iommu_type1_dma_unmap unmap;
> - long ret;
> + struct vfio_bitmap bitmap = { 0 };
> + int ret;
>
> minsz = offsetofend(struct vfio_iommu_type1_dma_unmap, size);
>
> if (copy_from_user(&unmap, (void __user *)arg, minsz))
> return -EFAULT;
>
> - if (unmap.argsz < minsz || unmap.flags)
> + if (unmap.argsz < minsz ||
> + unmap.flags & ~VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP)
> return -EINVAL;
>
> - ret = vfio_dma_do_unmap(iommu, &unmap);
> + if (unmap.flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) {
> + unsigned long pgshift;
> + size_t iommu_pgsize =
> + (size_t)1 << __ffs(vfio_pgsize_bitmap(iommu));
> +
> + if (unmap.argsz < (minsz + sizeof(bitmap)))
> + return -EINVAL;
> +
> + if (copy_from_user(&bitmap,
> + (void __user *)(arg + minsz),
> + sizeof(bitmap)))
> + return -EFAULT;
> +
> + /* allow only min supported pgsize */
> + if (bitmap.pgsize != iommu_pgsize)
> + return -EINVAL;
> + if (!access_ok((void __user *)bitmap.data, bitmap.size))
> + return -EINVAL;
> +
> + pgshift = __ffs(bitmap.pgsize);
> + ret = verify_bitmap_size(unmap.size >> pgshift,
> + bitmap.size);
> + if (ret)
> + return ret;
> +
> + }
> +
> + ret = vfio_dma_do_unmap(iommu, &unmap, &bitmap);
> if (ret)
> return ret;
>
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 5f359c63f5ef..e3cbf8b78623 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -1048,12 +1048,22 @@ struct vfio_bitmap {
> * field. No guarantee is made to the user that arbitrary unmaps of iova
> * or size different from those used in the original mapping call will
> * succeed.
> + * VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP should be set to get dirty bitmap
> + * before unmapping IO virtual addresses. When this flag is set, user must
> + * provide data[] as structure vfio_bitmap. User must allocate memory to get
> + * bitmap and must set size of allocated memory in vfio_bitmap.size field.
> + * A bit in bitmap represents one page of user provided page size in 'pgsize',
> + * consecutively starting from iova offset. Bit set indicates page at that
> + * offset from iova is dirty. Bitmap of pages in the range of unmapped size is
> + * returned in vfio_bitmap.data
> */
> struct vfio_iommu_type1_dma_unmap {
> __u32 argsz;
> __u32 flags;
> +#define VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP (1 << 0)
> __u64 iova; /* IO virtual address */
> __u64 size; /* Size of mapping (bytes) */
> + __u8 data[];
> };
>
> #define VFIO_IOMMU_UNMAP_DMA _IO(VFIO_TYPE, VFIO_BASE + 14)
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH Kernel v18 5/7] vfio iommu: Update UNMAP_DMA ioctl to get dirty bitmap before unmap
2020-05-06 22:25 ` Alex Williamson
@ 2020-05-12 20:30 ` Kirti Wankhede
-1 siblings, 0 replies; 39+ messages in thread
From: Kirti Wankhede @ 2020-05-12 20:30 UTC (permalink / raw)
To: Alex Williamson
Cc: cjia, kevin.tian, ziye.yang, changpeng.liu, yi.l.liu, mlevitsk,
eskultet, cohuck, dgilbert, jonathan.davies, eauger, aik, pasic,
felipe, Zhengxiao.zx, shuangtai.tst, Ken.Xue, zhi.a.wang,
yan.y.zhao, qemu-devel, kvm
On 5/7/2020 3:55 AM, Alex Williamson wrote:
> On Mon, 4 May 2020 21:28:57 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>
>> DMA mapped pages, including those pinned by mdev vendor drivers, might
>> get unpinned and unmapped while migration is active and device is still
>> running. For example, in pre-copy phase while guest driver could access
>> those pages, host device or vendor driver can dirty these mapped pages.
>> Such pages should be marked dirty so as to maintain memory consistency
>> for a user making use of dirty page tracking.
>>
>> To get bitmap during unmap, user should allocate memory for bitmap, set
>> size of allocated memory, set page size to be considered for bitmap and
>> set flag VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP.
>>
>> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
>> Reviewed-by: Neo Jia <cjia@nvidia.com>
>> ---
>> drivers/vfio/vfio_iommu_type1.c | 84 +++++++++++++++++++++++++++++++++++++++--
>> include/uapi/linux/vfio.h | 10 +++++
>> 2 files changed, 90 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>> index 01dcb417836f..8b27faf1ec38 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -983,12 +983,14 @@ static int verify_bitmap_size(uint64_t npages, uint64_t bitmap_size)
>> }
>>
>> static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>> - struct vfio_iommu_type1_dma_unmap *unmap)
>> + struct vfio_iommu_type1_dma_unmap *unmap,
>> + struct vfio_bitmap *bitmap)
>> {
>> uint64_t mask;
>> struct vfio_dma *dma, *dma_last = NULL;
>> size_t unmapped = 0;
>> int ret = 0, retries = 0;
>> + unsigned long *final_bitmap = NULL, *temp_bitmap = NULL;
>>
>> mask = ((uint64_t)1 << __ffs(vfio_pgsize_bitmap(iommu))) - 1;
>>
>> @@ -1041,6 +1043,7 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>> ret = -EINVAL;
>> goto unlock;
>> }
>> +
>> dma = vfio_find_dma(iommu, unmap->iova + unmap->size - 1, 0);
>> if (dma && dma->iova + dma->size != unmap->iova + unmap->size) {
>> ret = -EINVAL;
>> @@ -1048,6 +1051,22 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>> }
>> }
>>
>> + if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) &&
>> + iommu->dirty_page_tracking) {
>
> Why do we even accept VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP when not
> dirty page tracking rather than returning -EINVAL? It would simplify
> things here to reject it at the ioctl and silently ignoring a flag is
> rarely if ever the right approach.
>
>> + final_bitmap = kvzalloc(bitmap->size, GFP_KERNEL);
>> + if (!final_bitmap) {
>> + ret = -ENOMEM;
>> + goto unlock;
>> + }
>> +
>> + temp_bitmap = kvzalloc(bitmap->size, GFP_KERNEL);
>> + if (!temp_bitmap) {
>> + ret = -ENOMEM;
>> + kfree(final_bitmap);
>> + goto unlock;
>> + }
>
> YIKES! So the user can instantly trigger the kernel to internally
> allocate 2 x 256MB, regardless of how much they can actually map.
>
That is worst case senario. I don't think ideally that will ever hit.
More comment below regarding this.
>> + }
>> +
>> while ((dma = vfio_find_dma(iommu, unmap->iova, unmap->size))) {
>> if (!iommu->v2 && unmap->iova > dma->iova)
>> break;
>> @@ -1058,6 +1077,24 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>> if (dma->task->mm != current->mm)
>> break;
>>
>> + if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) &&
>> + iommu->dirty_page_tracking) {
>> + unsigned long pgshift = __ffs(bitmap->pgsize);
>> + unsigned int npages = dma->size >> pgshift;
>> + unsigned int shift;
>> +
>> + vfio_iova_dirty_bitmap(iommu, dma->iova, dma->size,
>> + bitmap->pgsize, (u64 *)temp_bitmap);
>
> vfio_iova_dirty_bitmap() takes a __user bitmap, we're doing
> copy_to_user() on a kernel allocated buffer???
>
Actually, there is no need to call vfio_iova_dirty_bitmap(), dma pointer
is known here and since its getting unmapped, there is no need to
repopulate bitmap. Removing vfio_iova_dirty_bitmap() and changing it as
below:
if (unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) {
unsigned long pgshift = __ffs(bitmap->pgsize);
unsigned int npages = dma->size >> pgshift;
unsigned int bitmap_size = DIRTY_BITMAP_BYTES(npages);
unsigned int shift = (dma->iova - unmap->iova) >>
pgshift;
/*
* mark all pages dirty if all pages are pinned and
* mapped.
*/
if (dma->iommu_mapped)
bitmap_set(temp_bitmap, 0, npages);
else
memcpy(temp_bitmap, dma->bitmap, bitmap_size);
if (shift)
bitmap_shift_left(temp_bitmap, temp_bitmap,
shift, npages);
bitmap_or(final_bitmap, final_bitmap, temp_bitmap,
shift + npages);
memset(temp_bitmap, 0, bitmap->size);
}
>> +
>> + shift = (dma->iova - unmap->iova) >> pgshift;
>> + if (shift)
>> + bitmap_shift_left(temp_bitmap, temp_bitmap,
>> + shift, npages);
>> + bitmap_or(final_bitmap, final_bitmap, temp_bitmap,
>> + shift + npages);
>> + memset(temp_bitmap, 0, bitmap->size);
>> + }
>
> It seems like if the per vfio_dma dirty bitmap was oversized by a long
> that we could shift it in place, then we'd only need one working bitmap
> buffer and we could size that to fit the vfio_dma (or the largest
> vfio_dma if we don't want to free and re-alloc for each vfio_dma).
> We'd need to do more copy_to/from_user()s, but we'd also avoid copying
> between sparse mappings (user zero'd bitmap required) and we'd have a
> far more reasonable memory usage. Thanks,
>
I thought about it, but couldn't optimize to use one bitmap buffer.
This case will only hit during migration with vIOMMU enabled.
Can we keep these 2 bitmap buffers for now and optimize it later?
Thanks,
Kirti
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH Kernel v18 5/7] vfio iommu: Update UNMAP_DMA ioctl to get dirty bitmap before unmap
@ 2020-05-12 20:30 ` Kirti Wankhede
0 siblings, 0 replies; 39+ messages in thread
From: Kirti Wankhede @ 2020-05-12 20:30 UTC (permalink / raw)
To: Alex Williamson
Cc: Zhengxiao.zx, kevin.tian, yi.l.liu, cjia, kvm, eskultet,
ziye.yang, qemu-devel, cohuck, shuangtai.tst, dgilbert,
zhi.a.wang, mlevitsk, pasic, aik, eauger, felipe,
jonathan.davies, yan.y.zhao, changpeng.liu, Ken.Xue
On 5/7/2020 3:55 AM, Alex Williamson wrote:
> On Mon, 4 May 2020 21:28:57 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>
>> DMA mapped pages, including those pinned by mdev vendor drivers, might
>> get unpinned and unmapped while migration is active and device is still
>> running. For example, in pre-copy phase while guest driver could access
>> those pages, host device or vendor driver can dirty these mapped pages.
>> Such pages should be marked dirty so as to maintain memory consistency
>> for a user making use of dirty page tracking.
>>
>> To get bitmap during unmap, user should allocate memory for bitmap, set
>> size of allocated memory, set page size to be considered for bitmap and
>> set flag VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP.
>>
>> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
>> Reviewed-by: Neo Jia <cjia@nvidia.com>
>> ---
>> drivers/vfio/vfio_iommu_type1.c | 84 +++++++++++++++++++++++++++++++++++++++--
>> include/uapi/linux/vfio.h | 10 +++++
>> 2 files changed, 90 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>> index 01dcb417836f..8b27faf1ec38 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -983,12 +983,14 @@ static int verify_bitmap_size(uint64_t npages, uint64_t bitmap_size)
>> }
>>
>> static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>> - struct vfio_iommu_type1_dma_unmap *unmap)
>> + struct vfio_iommu_type1_dma_unmap *unmap,
>> + struct vfio_bitmap *bitmap)
>> {
>> uint64_t mask;
>> struct vfio_dma *dma, *dma_last = NULL;
>> size_t unmapped = 0;
>> int ret = 0, retries = 0;
>> + unsigned long *final_bitmap = NULL, *temp_bitmap = NULL;
>>
>> mask = ((uint64_t)1 << __ffs(vfio_pgsize_bitmap(iommu))) - 1;
>>
>> @@ -1041,6 +1043,7 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>> ret = -EINVAL;
>> goto unlock;
>> }
>> +
>> dma = vfio_find_dma(iommu, unmap->iova + unmap->size - 1, 0);
>> if (dma && dma->iova + dma->size != unmap->iova + unmap->size) {
>> ret = -EINVAL;
>> @@ -1048,6 +1051,22 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>> }
>> }
>>
>> + if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) &&
>> + iommu->dirty_page_tracking) {
>
> Why do we even accept VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP when not
> dirty page tracking rather than returning -EINVAL? It would simplify
> things here to reject it at the ioctl and silently ignoring a flag is
> rarely if ever the right approach.
>
>> + final_bitmap = kvzalloc(bitmap->size, GFP_KERNEL);
>> + if (!final_bitmap) {
>> + ret = -ENOMEM;
>> + goto unlock;
>> + }
>> +
>> + temp_bitmap = kvzalloc(bitmap->size, GFP_KERNEL);
>> + if (!temp_bitmap) {
>> + ret = -ENOMEM;
>> + kfree(final_bitmap);
>> + goto unlock;
>> + }
>
> YIKES! So the user can instantly trigger the kernel to internally
> allocate 2 x 256MB, regardless of how much they can actually map.
>
That is worst case senario. I don't think ideally that will ever hit.
More comment below regarding this.
>> + }
>> +
>> while ((dma = vfio_find_dma(iommu, unmap->iova, unmap->size))) {
>> if (!iommu->v2 && unmap->iova > dma->iova)
>> break;
>> @@ -1058,6 +1077,24 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>> if (dma->task->mm != current->mm)
>> break;
>>
>> + if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) &&
>> + iommu->dirty_page_tracking) {
>> + unsigned long pgshift = __ffs(bitmap->pgsize);
>> + unsigned int npages = dma->size >> pgshift;
>> + unsigned int shift;
>> +
>> + vfio_iova_dirty_bitmap(iommu, dma->iova, dma->size,
>> + bitmap->pgsize, (u64 *)temp_bitmap);
>
> vfio_iova_dirty_bitmap() takes a __user bitmap, we're doing
> copy_to_user() on a kernel allocated buffer???
>
Actually, there is no need to call vfio_iova_dirty_bitmap(), dma pointer
is known here and since its getting unmapped, there is no need to
repopulate bitmap. Removing vfio_iova_dirty_bitmap() and changing it as
below:
if (unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) {
unsigned long pgshift = __ffs(bitmap->pgsize);
unsigned int npages = dma->size >> pgshift;
unsigned int bitmap_size = DIRTY_BITMAP_BYTES(npages);
unsigned int shift = (dma->iova - unmap->iova) >>
pgshift;
/*
* mark all pages dirty if all pages are pinned and
* mapped.
*/
if (dma->iommu_mapped)
bitmap_set(temp_bitmap, 0, npages);
else
memcpy(temp_bitmap, dma->bitmap, bitmap_size);
if (shift)
bitmap_shift_left(temp_bitmap, temp_bitmap,
shift, npages);
bitmap_or(final_bitmap, final_bitmap, temp_bitmap,
shift + npages);
memset(temp_bitmap, 0, bitmap->size);
}
>> +
>> + shift = (dma->iova - unmap->iova) >> pgshift;
>> + if (shift)
>> + bitmap_shift_left(temp_bitmap, temp_bitmap,
>> + shift, npages);
>> + bitmap_or(final_bitmap, final_bitmap, temp_bitmap,
>> + shift + npages);
>> + memset(temp_bitmap, 0, bitmap->size);
>> + }
>
> It seems like if the per vfio_dma dirty bitmap was oversized by a long
> that we could shift it in place, then we'd only need one working bitmap
> buffer and we could size that to fit the vfio_dma (or the largest
> vfio_dma if we don't want to free and re-alloc for each vfio_dma).
> We'd need to do more copy_to/from_user()s, but we'd also avoid copying
> between sparse mappings (user zero'd bitmap required) and we'd have a
> far more reasonable memory usage. Thanks,
>
I thought about it, but couldn't optimize to use one bitmap buffer.
This case will only hit during migration with vIOMMU enabled.
Can we keep these 2 bitmap buffers for now and optimize it later?
Thanks,
Kirti
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH Kernel v18 5/7] vfio iommu: Update UNMAP_DMA ioctl to get dirty bitmap before unmap
2020-05-12 20:30 ` Kirti Wankhede
@ 2020-05-12 21:21 ` Alex Williamson
-1 siblings, 0 replies; 39+ messages in thread
From: Alex Williamson @ 2020-05-12 21:21 UTC (permalink / raw)
To: Kirti Wankhede
Cc: cjia, kevin.tian, ziye.yang, changpeng.liu, yi.l.liu, mlevitsk,
eskultet, cohuck, dgilbert, jonathan.davies, eauger, aik, pasic,
felipe, Zhengxiao.zx, shuangtai.tst, Ken.Xue, zhi.a.wang,
yan.y.zhao, qemu-devel, kvm
On Wed, 13 May 2020 02:00:54 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:
> On 5/7/2020 3:55 AM, Alex Williamson wrote:
> > On Mon, 4 May 2020 21:28:57 +0530
> > Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >
> >> DMA mapped pages, including those pinned by mdev vendor drivers, might
> >> get unpinned and unmapped while migration is active and device is still
> >> running. For example, in pre-copy phase while guest driver could access
> >> those pages, host device or vendor driver can dirty these mapped pages.
> >> Such pages should be marked dirty so as to maintain memory consistency
> >> for a user making use of dirty page tracking.
> >>
> >> To get bitmap during unmap, user should allocate memory for bitmap, set
> >> size of allocated memory, set page size to be considered for bitmap and
> >> set flag VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP.
> >>
> >> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> >> Reviewed-by: Neo Jia <cjia@nvidia.com>
> >> ---
> >> drivers/vfio/vfio_iommu_type1.c | 84 +++++++++++++++++++++++++++++++++++++++--
> >> include/uapi/linux/vfio.h | 10 +++++
> >> 2 files changed, 90 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> >> index 01dcb417836f..8b27faf1ec38 100644
> >> --- a/drivers/vfio/vfio_iommu_type1.c
> >> +++ b/drivers/vfio/vfio_iommu_type1.c
> >> @@ -983,12 +983,14 @@ static int verify_bitmap_size(uint64_t npages, uint64_t bitmap_size)
> >> }
> >>
> >> static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> >> - struct vfio_iommu_type1_dma_unmap *unmap)
> >> + struct vfio_iommu_type1_dma_unmap *unmap,
> >> + struct vfio_bitmap *bitmap)
> >> {
> >> uint64_t mask;
> >> struct vfio_dma *dma, *dma_last = NULL;
> >> size_t unmapped = 0;
> >> int ret = 0, retries = 0;
> >> + unsigned long *final_bitmap = NULL, *temp_bitmap = NULL;
> >>
> >> mask = ((uint64_t)1 << __ffs(vfio_pgsize_bitmap(iommu))) - 1;
> >>
> >> @@ -1041,6 +1043,7 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> >> ret = -EINVAL;
> >> goto unlock;
> >> }
> >> +
> >> dma = vfio_find_dma(iommu, unmap->iova + unmap->size - 1, 0);
> >> if (dma && dma->iova + dma->size != unmap->iova + unmap->size) {
> >> ret = -EINVAL;
> >> @@ -1048,6 +1051,22 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> >> }
> >> }
> >>
> >> + if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) &&
> >> + iommu->dirty_page_tracking) {
> >
> > Why do we even accept VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP when not
> > dirty page tracking rather than returning -EINVAL? It would simplify
> > things here to reject it at the ioctl and silently ignoring a flag is
> > rarely if ever the right approach.
> >
> >> + final_bitmap = kvzalloc(bitmap->size, GFP_KERNEL);
> >> + if (!final_bitmap) {
> >> + ret = -ENOMEM;
> >> + goto unlock;
> >> + }
> >> +
> >> + temp_bitmap = kvzalloc(bitmap->size, GFP_KERNEL);
> >> + if (!temp_bitmap) {
> >> + ret = -ENOMEM;
> >> + kfree(final_bitmap);
> >> + goto unlock;
> >> + }
> >
> > YIKES! So the user can instantly trigger the kernel to internally
> > allocate 2 x 256MB, regardless of how much they can actually map.
> >
>
> That is worst case senario. I don't think ideally that will ever hit.
> More comment below regarding this.
If a user has the ability to lock 8TB of memory, then yeah, triggering
the kernel to allocate 512MB might not be a big deal. But in this case
a malicious user can trigger the kernel to allocate 512MB of memory
regardless of their locked memory limits. I think that's unacceptable.
> >> + }
> >> +
> >> while ((dma = vfio_find_dma(iommu, unmap->iova, unmap->size))) {
> >> if (!iommu->v2 && unmap->iova > dma->iova)
> >> break;
> >> @@ -1058,6 +1077,24 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> >> if (dma->task->mm != current->mm)
> >> break;
> >>
> >> + if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) &&
> >> + iommu->dirty_page_tracking) {
> >> + unsigned long pgshift = __ffs(bitmap->pgsize);
> >> + unsigned int npages = dma->size >> pgshift;
> >> + unsigned int shift;
> >> +
> >> + vfio_iova_dirty_bitmap(iommu, dma->iova, dma->size,
> >> + bitmap->pgsize, (u64 *)temp_bitmap);
> >
> > vfio_iova_dirty_bitmap() takes a __user bitmap, we're doing
> > copy_to_user() on a kernel allocated buffer???
> >
>
> Actually, there is no need to call vfio_iova_dirty_bitmap(), dma pointer
> is known here and since its getting unmapped, there is no need to
> repopulate bitmap. Removing vfio_iova_dirty_bitmap() and changing it as
> below:
>
> if (unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) {
> unsigned long pgshift = __ffs(bitmap->pgsize);
> unsigned int npages = dma->size >> pgshift;
> unsigned int bitmap_size = DIRTY_BITMAP_BYTES(npages);
> unsigned int shift = (dma->iova - unmap->iova) >>
> pgshift;
> /*
> * mark all pages dirty if all pages are pinned and
> * mapped.
> */
> if (dma->iommu_mapped)
> bitmap_set(temp_bitmap, 0, npages);
> else
> memcpy(temp_bitmap, dma->bitmap, bitmap_size);
>
> if (shift)
> bitmap_shift_left(temp_bitmap, temp_bitmap,
> shift, npages);
> bitmap_or(final_bitmap, final_bitmap, temp_bitmap,
> shift + npages);
> memset(temp_bitmap, 0, bitmap->size);
> }
Solves that problem, but I think also illustrates that if we could
shift dma->bitmap in place we could avoid a memcpy and a memset.
> >> +
> >> + shift = (dma->iova - unmap->iova) >> pgshift;
> >> + if (shift)
> >> + bitmap_shift_left(temp_bitmap, temp_bitmap,
> >> + shift, npages);
> >> + bitmap_or(final_bitmap, final_bitmap, temp_bitmap,
> >> + shift + npages);
> >> + memset(temp_bitmap, 0, bitmap->size);
> >> + }
> >
> > It seems like if the per vfio_dma dirty bitmap was oversized by a long
> > that we could shift it in place, then we'd only need one working bitmap
> > buffer and we could size that to fit the vfio_dma (or the largest
> > vfio_dma if we don't want to free and re-alloc for each vfio_dma).
> > We'd need to do more copy_to/from_user()s, but we'd also avoid copying
> > between sparse mappings (user zero'd bitmap required) and we'd have a
> > far more reasonable memory usage. Thanks,
> >
>
> I thought about it, but couldn't optimize to use one bitmap buffer.
> This case will only hit during migration with vIOMMU enabled.
> Can we keep these 2 bitmap buffers for now and optimize it later?
I don't see how we can limit it to that use case, or why that use case
makes it any less important to avoid exploitable inefficiencies like
this. We're also potentially changing the uapi from not requiring a
user zero'd buffer to requiring a user zero'd buffer, which means we'd
need to support a backwards compatible uapi, exposing a really
inefficient means for the kernel to zero user memory.
Can you identify what doesn't work about the above proposal? TBH, once
we shift the dma->bitmap in place, we could make it so that we can
directly copy_to_user and we'd only need to fixup any unaligned
overlaps between mappings, which we could do with just a few bytes
rather than some large final_bitmap buffer. Thanks,
Alex
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH Kernel v18 5/7] vfio iommu: Update UNMAP_DMA ioctl to get dirty bitmap before unmap
@ 2020-05-12 21:21 ` Alex Williamson
0 siblings, 0 replies; 39+ messages in thread
From: Alex Williamson @ 2020-05-12 21:21 UTC (permalink / raw)
To: Kirti Wankhede
Cc: Zhengxiao.zx, kevin.tian, yi.l.liu, cjia, kvm, eskultet,
ziye.yang, qemu-devel, cohuck, shuangtai.tst, dgilbert,
zhi.a.wang, mlevitsk, pasic, aik, eauger, felipe,
jonathan.davies, yan.y.zhao, changpeng.liu, Ken.Xue
On Wed, 13 May 2020 02:00:54 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:
> On 5/7/2020 3:55 AM, Alex Williamson wrote:
> > On Mon, 4 May 2020 21:28:57 +0530
> > Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >
> >> DMA mapped pages, including those pinned by mdev vendor drivers, might
> >> get unpinned and unmapped while migration is active and device is still
> >> running. For example, in pre-copy phase while guest driver could access
> >> those pages, host device or vendor driver can dirty these mapped pages.
> >> Such pages should be marked dirty so as to maintain memory consistency
> >> for a user making use of dirty page tracking.
> >>
> >> To get bitmap during unmap, user should allocate memory for bitmap, set
> >> size of allocated memory, set page size to be considered for bitmap and
> >> set flag VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP.
> >>
> >> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> >> Reviewed-by: Neo Jia <cjia@nvidia.com>
> >> ---
> >> drivers/vfio/vfio_iommu_type1.c | 84 +++++++++++++++++++++++++++++++++++++++--
> >> include/uapi/linux/vfio.h | 10 +++++
> >> 2 files changed, 90 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> >> index 01dcb417836f..8b27faf1ec38 100644
> >> --- a/drivers/vfio/vfio_iommu_type1.c
> >> +++ b/drivers/vfio/vfio_iommu_type1.c
> >> @@ -983,12 +983,14 @@ static int verify_bitmap_size(uint64_t npages, uint64_t bitmap_size)
> >> }
> >>
> >> static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> >> - struct vfio_iommu_type1_dma_unmap *unmap)
> >> + struct vfio_iommu_type1_dma_unmap *unmap,
> >> + struct vfio_bitmap *bitmap)
> >> {
> >> uint64_t mask;
> >> struct vfio_dma *dma, *dma_last = NULL;
> >> size_t unmapped = 0;
> >> int ret = 0, retries = 0;
> >> + unsigned long *final_bitmap = NULL, *temp_bitmap = NULL;
> >>
> >> mask = ((uint64_t)1 << __ffs(vfio_pgsize_bitmap(iommu))) - 1;
> >>
> >> @@ -1041,6 +1043,7 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> >> ret = -EINVAL;
> >> goto unlock;
> >> }
> >> +
> >> dma = vfio_find_dma(iommu, unmap->iova + unmap->size - 1, 0);
> >> if (dma && dma->iova + dma->size != unmap->iova + unmap->size) {
> >> ret = -EINVAL;
> >> @@ -1048,6 +1051,22 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> >> }
> >> }
> >>
> >> + if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) &&
> >> + iommu->dirty_page_tracking) {
> >
> > Why do we even accept VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP when not
> > dirty page tracking rather than returning -EINVAL? It would simplify
> > things here to reject it at the ioctl and silently ignoring a flag is
> > rarely if ever the right approach.
> >
> >> + final_bitmap = kvzalloc(bitmap->size, GFP_KERNEL);
> >> + if (!final_bitmap) {
> >> + ret = -ENOMEM;
> >> + goto unlock;
> >> + }
> >> +
> >> + temp_bitmap = kvzalloc(bitmap->size, GFP_KERNEL);
> >> + if (!temp_bitmap) {
> >> + ret = -ENOMEM;
> >> + kfree(final_bitmap);
> >> + goto unlock;
> >> + }
> >
> > YIKES! So the user can instantly trigger the kernel to internally
> > allocate 2 x 256MB, regardless of how much they can actually map.
> >
>
> That is worst case senario. I don't think ideally that will ever hit.
> More comment below regarding this.
If a user has the ability to lock 8TB of memory, then yeah, triggering
the kernel to allocate 512MB might not be a big deal. But in this case
a malicious user can trigger the kernel to allocate 512MB of memory
regardless of their locked memory limits. I think that's unacceptable.
> >> + }
> >> +
> >> while ((dma = vfio_find_dma(iommu, unmap->iova, unmap->size))) {
> >> if (!iommu->v2 && unmap->iova > dma->iova)
> >> break;
> >> @@ -1058,6 +1077,24 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> >> if (dma->task->mm != current->mm)
> >> break;
> >>
> >> + if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) &&
> >> + iommu->dirty_page_tracking) {
> >> + unsigned long pgshift = __ffs(bitmap->pgsize);
> >> + unsigned int npages = dma->size >> pgshift;
> >> + unsigned int shift;
> >> +
> >> + vfio_iova_dirty_bitmap(iommu, dma->iova, dma->size,
> >> + bitmap->pgsize, (u64 *)temp_bitmap);
> >
> > vfio_iova_dirty_bitmap() takes a __user bitmap, we're doing
> > copy_to_user() on a kernel allocated buffer???
> >
>
> Actually, there is no need to call vfio_iova_dirty_bitmap(), dma pointer
> is known here and since its getting unmapped, there is no need to
> repopulate bitmap. Removing vfio_iova_dirty_bitmap() and changing it as
> below:
>
> if (unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) {
> unsigned long pgshift = __ffs(bitmap->pgsize);
> unsigned int npages = dma->size >> pgshift;
> unsigned int bitmap_size = DIRTY_BITMAP_BYTES(npages);
> unsigned int shift = (dma->iova - unmap->iova) >>
> pgshift;
> /*
> * mark all pages dirty if all pages are pinned and
> * mapped.
> */
> if (dma->iommu_mapped)
> bitmap_set(temp_bitmap, 0, npages);
> else
> memcpy(temp_bitmap, dma->bitmap, bitmap_size);
>
> if (shift)
> bitmap_shift_left(temp_bitmap, temp_bitmap,
> shift, npages);
> bitmap_or(final_bitmap, final_bitmap, temp_bitmap,
> shift + npages);
> memset(temp_bitmap, 0, bitmap->size);
> }
Solves that problem, but I think also illustrates that if we could
shift dma->bitmap in place we could avoid a memcpy and a memset.
> >> +
> >> + shift = (dma->iova - unmap->iova) >> pgshift;
> >> + if (shift)
> >> + bitmap_shift_left(temp_bitmap, temp_bitmap,
> >> + shift, npages);
> >> + bitmap_or(final_bitmap, final_bitmap, temp_bitmap,
> >> + shift + npages);
> >> + memset(temp_bitmap, 0, bitmap->size);
> >> + }
> >
> > It seems like if the per vfio_dma dirty bitmap was oversized by a long
> > that we could shift it in place, then we'd only need one working bitmap
> > buffer and we could size that to fit the vfio_dma (or the largest
> > vfio_dma if we don't want to free and re-alloc for each vfio_dma).
> > We'd need to do more copy_to/from_user()s, but we'd also avoid copying
> > between sparse mappings (user zero'd bitmap required) and we'd have a
> > far more reasonable memory usage. Thanks,
> >
>
> I thought about it, but couldn't optimize to use one bitmap buffer.
> This case will only hit during migration with vIOMMU enabled.
> Can we keep these 2 bitmap buffers for now and optimize it later?
I don't see how we can limit it to that use case, or why that use case
makes it any less important to avoid exploitable inefficiencies like
this. We're also potentially changing the uapi from not requiring a
user zero'd buffer to requiring a user zero'd buffer, which means we'd
need to support a backwards compatible uapi, exposing a really
inefficient means for the kernel to zero user memory.
Can you identify what doesn't work about the above proposal? TBH, once
we shift the dma->bitmap in place, we could make it so that we can
directly copy_to_user and we'd only need to fixup any unaligned
overlaps between mappings, which we could do with just a few bytes
rather than some large final_bitmap buffer. Thanks,
Alex
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH Kernel v18 6/7] vfio iommu: Add migration capability to report supported features
2020-05-04 15:58 ` Kirti Wankhede
@ 2020-05-04 15:58 ` Kirti Wankhede
-1 siblings, 0 replies; 39+ messages in thread
From: Kirti Wankhede @ 2020-05-04 15:58 UTC (permalink / raw)
To: alex.williamson, cjia
Cc: kevin.tian, ziye.yang, changpeng.liu, yi.l.liu, mlevitsk,
eskultet, cohuck, dgilbert, jonathan.davies, eauger, aik, pasic,
felipe, Zhengxiao.zx, shuangtai.tst, Ken.Xue, zhi.a.wang,
yan.y.zhao, qemu-devel, kvm, Kirti Wankhede
Added migration capability in IOMMU info chain.
User application should check IOMMU info chain for migration capability
to use dirty page tracking feature provided by kernel module.
Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
---
drivers/vfio/vfio_iommu_type1.c | 15 +++++++++++++++
include/uapi/linux/vfio.h | 14 ++++++++++++++
2 files changed, 29 insertions(+)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 8b27faf1ec38..b38d278d7bff 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -2378,6 +2378,17 @@ static int vfio_iommu_iova_build_caps(struct vfio_iommu *iommu,
return ret;
}
+static int vfio_iommu_migration_build_caps(struct vfio_info_cap *caps)
+{
+ struct vfio_iommu_type1_info_cap_migration cap_mig;
+
+ cap_mig.header.id = VFIO_IOMMU_TYPE1_INFO_CAP_MIGRATION;
+ cap_mig.header.version = 1;
+ cap_mig.flags = VFIO_IOMMU_INFO_CAPS_MIGRATION_DIRTY_PAGE_TRACK;
+
+ return vfio_info_add_capability(caps, &cap_mig.header, sizeof(cap_mig));
+}
+
static long vfio_iommu_type1_ioctl(void *iommu_data,
unsigned int cmd, unsigned long arg)
{
@@ -2427,6 +2438,10 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
if (ret)
return ret;
+ ret = vfio_iommu_migration_build_caps(&caps);
+ if (ret)
+ return ret;
+
if (caps.size) {
info.flags |= VFIO_IOMMU_INFO_CAPS;
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index e3cbf8b78623..df9ce8aaafab 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -1013,6 +1013,20 @@ struct vfio_iommu_type1_info_cap_iova_range {
struct vfio_iova_range iova_ranges[];
};
+/*
+ * The migration capability allows to report supported features for migration.
+ *
+ * The structures below define version 1 of this capability.
+ */
+#define VFIO_IOMMU_TYPE1_INFO_CAP_MIGRATION 1
+
+struct vfio_iommu_type1_info_cap_migration {
+ struct vfio_info_cap_header header;
+ __u32 flags;
+ /* supports dirty page tracking */
+#define VFIO_IOMMU_INFO_CAPS_MIGRATION_DIRTY_PAGE_TRACK (1 << 0)
+};
+
#define VFIO_IOMMU_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
/**
--
2.7.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH Kernel v18 6/7] vfio iommu: Add migration capability to report supported features
@ 2020-05-04 15:58 ` Kirti Wankhede
0 siblings, 0 replies; 39+ messages in thread
From: Kirti Wankhede @ 2020-05-04 15:58 UTC (permalink / raw)
To: alex.williamson, cjia
Cc: Zhengxiao.zx, kevin.tian, yi.l.liu, yan.y.zhao, kvm, eskultet,
ziye.yang, qemu-devel, cohuck, shuangtai.tst, dgilbert,
zhi.a.wang, mlevitsk, pasic, aik, Kirti Wankhede, eauger, felipe,
jonathan.davies, changpeng.liu, Ken.Xue
Added migration capability in IOMMU info chain.
User application should check IOMMU info chain for migration capability
to use dirty page tracking feature provided by kernel module.
Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
---
drivers/vfio/vfio_iommu_type1.c | 15 +++++++++++++++
include/uapi/linux/vfio.h | 14 ++++++++++++++
2 files changed, 29 insertions(+)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 8b27faf1ec38..b38d278d7bff 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -2378,6 +2378,17 @@ static int vfio_iommu_iova_build_caps(struct vfio_iommu *iommu,
return ret;
}
+static int vfio_iommu_migration_build_caps(struct vfio_info_cap *caps)
+{
+ struct vfio_iommu_type1_info_cap_migration cap_mig;
+
+ cap_mig.header.id = VFIO_IOMMU_TYPE1_INFO_CAP_MIGRATION;
+ cap_mig.header.version = 1;
+ cap_mig.flags = VFIO_IOMMU_INFO_CAPS_MIGRATION_DIRTY_PAGE_TRACK;
+
+ return vfio_info_add_capability(caps, &cap_mig.header, sizeof(cap_mig));
+}
+
static long vfio_iommu_type1_ioctl(void *iommu_data,
unsigned int cmd, unsigned long arg)
{
@@ -2427,6 +2438,10 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
if (ret)
return ret;
+ ret = vfio_iommu_migration_build_caps(&caps);
+ if (ret)
+ return ret;
+
if (caps.size) {
info.flags |= VFIO_IOMMU_INFO_CAPS;
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index e3cbf8b78623..df9ce8aaafab 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -1013,6 +1013,20 @@ struct vfio_iommu_type1_info_cap_iova_range {
struct vfio_iova_range iova_ranges[];
};
+/*
+ * The migration capability allows to report supported features for migration.
+ *
+ * The structures below define version 1 of this capability.
+ */
+#define VFIO_IOMMU_TYPE1_INFO_CAP_MIGRATION 1
+
+struct vfio_iommu_type1_info_cap_migration {
+ struct vfio_info_cap_header header;
+ __u32 flags;
+ /* supports dirty page tracking */
+#define VFIO_IOMMU_INFO_CAPS_MIGRATION_DIRTY_PAGE_TRACK (1 << 0)
+};
+
#define VFIO_IOMMU_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
/**
--
2.7.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH Kernel v18 6/7] vfio iommu: Add migration capability to report supported features
2020-05-04 15:58 ` Kirti Wankhede
@ 2020-05-06 22:27 ` Alex Williamson
-1 siblings, 0 replies; 39+ messages in thread
From: Alex Williamson @ 2020-05-06 22:27 UTC (permalink / raw)
To: Kirti Wankhede
Cc: cjia, kevin.tian, ziye.yang, changpeng.liu, yi.l.liu, mlevitsk,
eskultet, cohuck, dgilbert, jonathan.davies, eauger, aik, pasic,
felipe, Zhengxiao.zx, shuangtai.tst, Ken.Xue, zhi.a.wang,
yan.y.zhao, qemu-devel, kvm
On Mon, 4 May 2020 21:28:58 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:
> Added migration capability in IOMMU info chain.
> User application should check IOMMU info chain for migration capability
> to use dirty page tracking feature provided by kernel module.
>
> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> ---
> drivers/vfio/vfio_iommu_type1.c | 15 +++++++++++++++
> include/uapi/linux/vfio.h | 14 ++++++++++++++
> 2 files changed, 29 insertions(+)
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 8b27faf1ec38..b38d278d7bff 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -2378,6 +2378,17 @@ static int vfio_iommu_iova_build_caps(struct vfio_iommu *iommu,
> return ret;
> }
>
> +static int vfio_iommu_migration_build_caps(struct vfio_info_cap *caps)
> +{
> + struct vfio_iommu_type1_info_cap_migration cap_mig;
> +
> + cap_mig.header.id = VFIO_IOMMU_TYPE1_INFO_CAP_MIGRATION;
> + cap_mig.header.version = 1;
> + cap_mig.flags = VFIO_IOMMU_INFO_CAPS_MIGRATION_DIRTY_PAGE_TRACK;
> +
> + return vfio_info_add_capability(caps, &cap_mig.header, sizeof(cap_mig));
> +}
> +
> static long vfio_iommu_type1_ioctl(void *iommu_data,
> unsigned int cmd, unsigned long arg)
> {
> @@ -2427,6 +2438,10 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
> if (ret)
> return ret;
>
> + ret = vfio_iommu_migration_build_caps(&caps);
> + if (ret)
> + return ret;
> +
> if (caps.size) {
> info.flags |= VFIO_IOMMU_INFO_CAPS;
>
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index e3cbf8b78623..df9ce8aaafab 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -1013,6 +1013,20 @@ struct vfio_iommu_type1_info_cap_iova_range {
> struct vfio_iova_range iova_ranges[];
> };
>
> +/*
> + * The migration capability allows to report supported features for migration.
> + *
> + * The structures below define version 1 of this capability.
> + */
> +#define VFIO_IOMMU_TYPE1_INFO_CAP_MIGRATION 1
> +
> +struct vfio_iommu_type1_info_cap_migration {
> + struct vfio_info_cap_header header;
> + __u32 flags;
> + /* supports dirty page tracking */
> +#define VFIO_IOMMU_INFO_CAPS_MIGRATION_DIRTY_PAGE_TRACK (1 << 0)
> +};
> +
What about exposing the maximum supported dirty bitmap size and the
supported page sizes? Thanks,
Alex
> #define VFIO_IOMMU_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
>
> /**
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH Kernel v18 6/7] vfio iommu: Add migration capability to report supported features
@ 2020-05-06 22:27 ` Alex Williamson
0 siblings, 0 replies; 39+ messages in thread
From: Alex Williamson @ 2020-05-06 22:27 UTC (permalink / raw)
To: Kirti Wankhede
Cc: Zhengxiao.zx, kevin.tian, yi.l.liu, cjia, kvm, eskultet,
ziye.yang, qemu-devel, cohuck, shuangtai.tst, dgilbert,
zhi.a.wang, mlevitsk, pasic, aik, eauger, felipe,
jonathan.davies, yan.y.zhao, changpeng.liu, Ken.Xue
On Mon, 4 May 2020 21:28:58 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:
> Added migration capability in IOMMU info chain.
> User application should check IOMMU info chain for migration capability
> to use dirty page tracking feature provided by kernel module.
>
> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> ---
> drivers/vfio/vfio_iommu_type1.c | 15 +++++++++++++++
> include/uapi/linux/vfio.h | 14 ++++++++++++++
> 2 files changed, 29 insertions(+)
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 8b27faf1ec38..b38d278d7bff 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -2378,6 +2378,17 @@ static int vfio_iommu_iova_build_caps(struct vfio_iommu *iommu,
> return ret;
> }
>
> +static int vfio_iommu_migration_build_caps(struct vfio_info_cap *caps)
> +{
> + struct vfio_iommu_type1_info_cap_migration cap_mig;
> +
> + cap_mig.header.id = VFIO_IOMMU_TYPE1_INFO_CAP_MIGRATION;
> + cap_mig.header.version = 1;
> + cap_mig.flags = VFIO_IOMMU_INFO_CAPS_MIGRATION_DIRTY_PAGE_TRACK;
> +
> + return vfio_info_add_capability(caps, &cap_mig.header, sizeof(cap_mig));
> +}
> +
> static long vfio_iommu_type1_ioctl(void *iommu_data,
> unsigned int cmd, unsigned long arg)
> {
> @@ -2427,6 +2438,10 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
> if (ret)
> return ret;
>
> + ret = vfio_iommu_migration_build_caps(&caps);
> + if (ret)
> + return ret;
> +
> if (caps.size) {
> info.flags |= VFIO_IOMMU_INFO_CAPS;
>
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index e3cbf8b78623..df9ce8aaafab 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -1013,6 +1013,20 @@ struct vfio_iommu_type1_info_cap_iova_range {
> struct vfio_iova_range iova_ranges[];
> };
>
> +/*
> + * The migration capability allows to report supported features for migration.
> + *
> + * The structures below define version 1 of this capability.
> + */
> +#define VFIO_IOMMU_TYPE1_INFO_CAP_MIGRATION 1
> +
> +struct vfio_iommu_type1_info_cap_migration {
> + struct vfio_info_cap_header header;
> + __u32 flags;
> + /* supports dirty page tracking */
> +#define VFIO_IOMMU_INFO_CAPS_MIGRATION_DIRTY_PAGE_TRACK (1 << 0)
> +};
> +
What about exposing the maximum supported dirty bitmap size and the
supported page sizes? Thanks,
Alex
> #define VFIO_IOMMU_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
>
> /**
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH Kernel v18 6/7] vfio iommu: Add migration capability to report supported features
2020-05-06 22:27 ` Alex Williamson
@ 2020-05-07 5:37 ` Kirti Wankhede
-1 siblings, 0 replies; 39+ messages in thread
From: Kirti Wankhede @ 2020-05-07 5:37 UTC (permalink / raw)
To: Alex Williamson
Cc: cjia, kevin.tian, ziye.yang, changpeng.liu, yi.l.liu, mlevitsk,
eskultet, cohuck, dgilbert, jonathan.davies, eauger, aik, pasic,
felipe, Zhengxiao.zx, shuangtai.tst, Ken.Xue, zhi.a.wang,
yan.y.zhao, qemu-devel, kvm
On 5/7/2020 3:57 AM, Alex Williamson wrote:
> On Mon, 4 May 2020 21:28:58 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>
>> Added migration capability in IOMMU info chain.
>> User application should check IOMMU info chain for migration capability
>> to use dirty page tracking feature provided by kernel module.
>>
>> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
>> ---
>> drivers/vfio/vfio_iommu_type1.c | 15 +++++++++++++++
>> include/uapi/linux/vfio.h | 14 ++++++++++++++
>> 2 files changed, 29 insertions(+)
>>
>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>> index 8b27faf1ec38..b38d278d7bff 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -2378,6 +2378,17 @@ static int vfio_iommu_iova_build_caps(struct vfio_iommu *iommu,
>> return ret;
>> }
>>
>> +static int vfio_iommu_migration_build_caps(struct vfio_info_cap *caps)
>> +{
>> + struct vfio_iommu_type1_info_cap_migration cap_mig;
>> +
>> + cap_mig.header.id = VFIO_IOMMU_TYPE1_INFO_CAP_MIGRATION;
>> + cap_mig.header.version = 1;
>> + cap_mig.flags = VFIO_IOMMU_INFO_CAPS_MIGRATION_DIRTY_PAGE_TRACK;
>> +
>> + return vfio_info_add_capability(caps, &cap_mig.header, sizeof(cap_mig));
>> +}
>> +
>> static long vfio_iommu_type1_ioctl(void *iommu_data,
>> unsigned int cmd, unsigned long arg)
>> {
>> @@ -2427,6 +2438,10 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>> if (ret)
>> return ret;
>>
>> + ret = vfio_iommu_migration_build_caps(&caps);
>> + if (ret)
>> + return ret;
>> +
>> if (caps.size) {
>> info.flags |= VFIO_IOMMU_INFO_CAPS;
>>
>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> index e3cbf8b78623..df9ce8aaafab 100644
>> --- a/include/uapi/linux/vfio.h
>> +++ b/include/uapi/linux/vfio.h
>> @@ -1013,6 +1013,20 @@ struct vfio_iommu_type1_info_cap_iova_range {
>> struct vfio_iova_range iova_ranges[];
>> };
>>
>> +/*
>> + * The migration capability allows to report supported features for migration.
>> + *
>> + * The structures below define version 1 of this capability.
>> + */
>> +#define VFIO_IOMMU_TYPE1_INFO_CAP_MIGRATION 1
>> +
>> +struct vfio_iommu_type1_info_cap_migration {
>> + struct vfio_info_cap_header header;
>> + __u32 flags;
>> + /* supports dirty page tracking */
>> +#define VFIO_IOMMU_INFO_CAPS_MIGRATION_DIRTY_PAGE_TRACK (1 << 0)
>> +};
>> +
>
> What about exposing the maximum supported dirty bitmap size and the
> supported page sizes? Thanks,
>
How should user application use that?
Thanks,
Kirti
> Alex
>
>> #define VFIO_IOMMU_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
>>
>> /**
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH Kernel v18 6/7] vfio iommu: Add migration capability to report supported features
@ 2020-05-07 5:37 ` Kirti Wankhede
0 siblings, 0 replies; 39+ messages in thread
From: Kirti Wankhede @ 2020-05-07 5:37 UTC (permalink / raw)
To: Alex Williamson
Cc: Zhengxiao.zx, kevin.tian, yi.l.liu, cjia, kvm, eskultet,
ziye.yang, qemu-devel, cohuck, shuangtai.tst, dgilbert,
zhi.a.wang, mlevitsk, pasic, aik, eauger, felipe,
jonathan.davies, yan.y.zhao, changpeng.liu, Ken.Xue
On 5/7/2020 3:57 AM, Alex Williamson wrote:
> On Mon, 4 May 2020 21:28:58 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>
>> Added migration capability in IOMMU info chain.
>> User application should check IOMMU info chain for migration capability
>> to use dirty page tracking feature provided by kernel module.
>>
>> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
>> ---
>> drivers/vfio/vfio_iommu_type1.c | 15 +++++++++++++++
>> include/uapi/linux/vfio.h | 14 ++++++++++++++
>> 2 files changed, 29 insertions(+)
>>
>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>> index 8b27faf1ec38..b38d278d7bff 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -2378,6 +2378,17 @@ static int vfio_iommu_iova_build_caps(struct vfio_iommu *iommu,
>> return ret;
>> }
>>
>> +static int vfio_iommu_migration_build_caps(struct vfio_info_cap *caps)
>> +{
>> + struct vfio_iommu_type1_info_cap_migration cap_mig;
>> +
>> + cap_mig.header.id = VFIO_IOMMU_TYPE1_INFO_CAP_MIGRATION;
>> + cap_mig.header.version = 1;
>> + cap_mig.flags = VFIO_IOMMU_INFO_CAPS_MIGRATION_DIRTY_PAGE_TRACK;
>> +
>> + return vfio_info_add_capability(caps, &cap_mig.header, sizeof(cap_mig));
>> +}
>> +
>> static long vfio_iommu_type1_ioctl(void *iommu_data,
>> unsigned int cmd, unsigned long arg)
>> {
>> @@ -2427,6 +2438,10 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>> if (ret)
>> return ret;
>>
>> + ret = vfio_iommu_migration_build_caps(&caps);
>> + if (ret)
>> + return ret;
>> +
>> if (caps.size) {
>> info.flags |= VFIO_IOMMU_INFO_CAPS;
>>
>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> index e3cbf8b78623..df9ce8aaafab 100644
>> --- a/include/uapi/linux/vfio.h
>> +++ b/include/uapi/linux/vfio.h
>> @@ -1013,6 +1013,20 @@ struct vfio_iommu_type1_info_cap_iova_range {
>> struct vfio_iova_range iova_ranges[];
>> };
>>
>> +/*
>> + * The migration capability allows to report supported features for migration.
>> + *
>> + * The structures below define version 1 of this capability.
>> + */
>> +#define VFIO_IOMMU_TYPE1_INFO_CAP_MIGRATION 1
>> +
>> +struct vfio_iommu_type1_info_cap_migration {
>> + struct vfio_info_cap_header header;
>> + __u32 flags;
>> + /* supports dirty page tracking */
>> +#define VFIO_IOMMU_INFO_CAPS_MIGRATION_DIRTY_PAGE_TRACK (1 << 0)
>> +};
>> +
>
> What about exposing the maximum supported dirty bitmap size and the
> supported page sizes? Thanks,
>
How should user application use that?
Thanks,
Kirti
> Alex
>
>> #define VFIO_IOMMU_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
>>
>> /**
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH Kernel v18 6/7] vfio iommu: Add migration capability to report supported features
2020-05-07 5:37 ` Kirti Wankhede
@ 2020-05-07 15:17 ` Alex Williamson
-1 siblings, 0 replies; 39+ messages in thread
From: Alex Williamson @ 2020-05-07 15:17 UTC (permalink / raw)
To: Kirti Wankhede
Cc: cjia, kevin.tian, ziye.yang, changpeng.liu, yi.l.liu, mlevitsk,
eskultet, cohuck, dgilbert, jonathan.davies, eauger, aik, pasic,
felipe, Zhengxiao.zx, shuangtai.tst, Ken.Xue, zhi.a.wang,
yan.y.zhao, qemu-devel, kvm
On Thu, 7 May 2020 11:07:26 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:
> On 5/7/2020 3:57 AM, Alex Williamson wrote:
> > On Mon, 4 May 2020 21:28:58 +0530
> > Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >
> >> Added migration capability in IOMMU info chain.
> >> User application should check IOMMU info chain for migration capability
> >> to use dirty page tracking feature provided by kernel module.
> >>
> >> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> >> ---
> >> drivers/vfio/vfio_iommu_type1.c | 15 +++++++++++++++
> >> include/uapi/linux/vfio.h | 14 ++++++++++++++
> >> 2 files changed, 29 insertions(+)
> >>
> >> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> >> index 8b27faf1ec38..b38d278d7bff 100644
> >> --- a/drivers/vfio/vfio_iommu_type1.c
> >> +++ b/drivers/vfio/vfio_iommu_type1.c
> >> @@ -2378,6 +2378,17 @@ static int vfio_iommu_iova_build_caps(struct vfio_iommu *iommu,
> >> return ret;
> >> }
> >>
> >> +static int vfio_iommu_migration_build_caps(struct vfio_info_cap *caps)
> >> +{
> >> + struct vfio_iommu_type1_info_cap_migration cap_mig;
> >> +
> >> + cap_mig.header.id = VFIO_IOMMU_TYPE1_INFO_CAP_MIGRATION;
> >> + cap_mig.header.version = 1;
> >> + cap_mig.flags = VFIO_IOMMU_INFO_CAPS_MIGRATION_DIRTY_PAGE_TRACK;
> >> +
> >> + return vfio_info_add_capability(caps, &cap_mig.header, sizeof(cap_mig));
> >> +}
> >> +
> >> static long vfio_iommu_type1_ioctl(void *iommu_data,
> >> unsigned int cmd, unsigned long arg)
> >> {
> >> @@ -2427,6 +2438,10 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
> >> if (ret)
> >> return ret;
> >>
> >> + ret = vfio_iommu_migration_build_caps(&caps);
> >> + if (ret)
> >> + return ret;
> >> +
> >> if (caps.size) {
> >> info.flags |= VFIO_IOMMU_INFO_CAPS;
> >>
> >> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> >> index e3cbf8b78623..df9ce8aaafab 100644
> >> --- a/include/uapi/linux/vfio.h
> >> +++ b/include/uapi/linux/vfio.h
> >> @@ -1013,6 +1013,20 @@ struct vfio_iommu_type1_info_cap_iova_range {
> >> struct vfio_iova_range iova_ranges[];
> >> };
> >>
> >> +/*
> >> + * The migration capability allows to report supported features for migration.
> >> + *
> >> + * The structures below define version 1 of this capability.
> >> + */
> >> +#define VFIO_IOMMU_TYPE1_INFO_CAP_MIGRATION 1
> >> +
> >> +struct vfio_iommu_type1_info_cap_migration {
> >> + struct vfio_info_cap_header header;
> >> + __u32 flags;
> >> + /* supports dirty page tracking */
> >> +#define VFIO_IOMMU_INFO_CAPS_MIGRATION_DIRTY_PAGE_TRACK (1 << 0)
> >> +};
> >> +
> >
> > What about exposing the maximum supported dirty bitmap size and the
> > supported page sizes? Thanks,
> >
>
> How should user application use that?
How does a user application currently discover that only a PAGE_SIZE
dirty bitmap granularity is supported or that the when performing an
unmap while requesting the dirty bitmap, those unmaps need to be
chunked to no more than 2^31 * PAGE_SIZE? I don't see anything in the
uapi that expresses these restrictions.
It seems we're currently relying on the QEMU implementation to
coincide with bitmap granularity support, with no mechanism other than
trial and error to validate or expand that support. Likewise, I think
it's largely a coincidence that KVM also has the same slot size
restrictions, so we're unlikely to see an unmap exceeding this limit,
but our api does not restrict an unmap to only cover a single mapping.
We probably also need to think about whether we need to expose a
mapping chunk limitation separately or if it should be extrapolated
from the migration support (we might have non-migration reasons to
limit it at some point).
If we leave these as assumptions then we risk breaking userspace or
limiting the usefulness of expending this support. If we include
within the uapi a mechanism for learning about these restrictions, then
it becomes a userspace problem to follow the uapi and take advantage of
the uapi provided. Thanks,
Alex
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH Kernel v18 6/7] vfio iommu: Add migration capability to report supported features
@ 2020-05-07 15:17 ` Alex Williamson
0 siblings, 0 replies; 39+ messages in thread
From: Alex Williamson @ 2020-05-07 15:17 UTC (permalink / raw)
To: Kirti Wankhede
Cc: Zhengxiao.zx, kevin.tian, yi.l.liu, cjia, kvm, eskultet,
ziye.yang, qemu-devel, cohuck, shuangtai.tst, dgilbert,
zhi.a.wang, mlevitsk, pasic, aik, eauger, felipe,
jonathan.davies, yan.y.zhao, changpeng.liu, Ken.Xue
On Thu, 7 May 2020 11:07:26 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:
> On 5/7/2020 3:57 AM, Alex Williamson wrote:
> > On Mon, 4 May 2020 21:28:58 +0530
> > Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >
> >> Added migration capability in IOMMU info chain.
> >> User application should check IOMMU info chain for migration capability
> >> to use dirty page tracking feature provided by kernel module.
> >>
> >> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> >> ---
> >> drivers/vfio/vfio_iommu_type1.c | 15 +++++++++++++++
> >> include/uapi/linux/vfio.h | 14 ++++++++++++++
> >> 2 files changed, 29 insertions(+)
> >>
> >> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> >> index 8b27faf1ec38..b38d278d7bff 100644
> >> --- a/drivers/vfio/vfio_iommu_type1.c
> >> +++ b/drivers/vfio/vfio_iommu_type1.c
> >> @@ -2378,6 +2378,17 @@ static int vfio_iommu_iova_build_caps(struct vfio_iommu *iommu,
> >> return ret;
> >> }
> >>
> >> +static int vfio_iommu_migration_build_caps(struct vfio_info_cap *caps)
> >> +{
> >> + struct vfio_iommu_type1_info_cap_migration cap_mig;
> >> +
> >> + cap_mig.header.id = VFIO_IOMMU_TYPE1_INFO_CAP_MIGRATION;
> >> + cap_mig.header.version = 1;
> >> + cap_mig.flags = VFIO_IOMMU_INFO_CAPS_MIGRATION_DIRTY_PAGE_TRACK;
> >> +
> >> + return vfio_info_add_capability(caps, &cap_mig.header, sizeof(cap_mig));
> >> +}
> >> +
> >> static long vfio_iommu_type1_ioctl(void *iommu_data,
> >> unsigned int cmd, unsigned long arg)
> >> {
> >> @@ -2427,6 +2438,10 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
> >> if (ret)
> >> return ret;
> >>
> >> + ret = vfio_iommu_migration_build_caps(&caps);
> >> + if (ret)
> >> + return ret;
> >> +
> >> if (caps.size) {
> >> info.flags |= VFIO_IOMMU_INFO_CAPS;
> >>
> >> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> >> index e3cbf8b78623..df9ce8aaafab 100644
> >> --- a/include/uapi/linux/vfio.h
> >> +++ b/include/uapi/linux/vfio.h
> >> @@ -1013,6 +1013,20 @@ struct vfio_iommu_type1_info_cap_iova_range {
> >> struct vfio_iova_range iova_ranges[];
> >> };
> >>
> >> +/*
> >> + * The migration capability allows to report supported features for migration.
> >> + *
> >> + * The structures below define version 1 of this capability.
> >> + */
> >> +#define VFIO_IOMMU_TYPE1_INFO_CAP_MIGRATION 1
> >> +
> >> +struct vfio_iommu_type1_info_cap_migration {
> >> + struct vfio_info_cap_header header;
> >> + __u32 flags;
> >> + /* supports dirty page tracking */
> >> +#define VFIO_IOMMU_INFO_CAPS_MIGRATION_DIRTY_PAGE_TRACK (1 << 0)
> >> +};
> >> +
> >
> > What about exposing the maximum supported dirty bitmap size and the
> > supported page sizes? Thanks,
> >
>
> How should user application use that?
How does a user application currently discover that only a PAGE_SIZE
dirty bitmap granularity is supported or that the when performing an
unmap while requesting the dirty bitmap, those unmaps need to be
chunked to no more than 2^31 * PAGE_SIZE? I don't see anything in the
uapi that expresses these restrictions.
It seems we're currently relying on the QEMU implementation to
coincide with bitmap granularity support, with no mechanism other than
trial and error to validate or expand that support. Likewise, I think
it's largely a coincidence that KVM also has the same slot size
restrictions, so we're unlikely to see an unmap exceeding this limit,
but our api does not restrict an unmap to only cover a single mapping.
We probably also need to think about whether we need to expose a
mapping chunk limitation separately or if it should be extrapolated
from the migration support (we might have non-migration reasons to
limit it at some point).
If we leave these as assumptions then we risk breaking userspace or
limiting the usefulness of expending this support. If we include
within the uapi a mechanism for learning about these restrictions, then
it becomes a userspace problem to follow the uapi and take advantage of
the uapi provided. Thanks,
Alex
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH Kernel v18 7/7] vfio: Selective dirty page tracking if IOMMU backed device pins pages
2020-05-04 15:58 ` Kirti Wankhede
@ 2020-05-04 15:58 ` Kirti Wankhede
-1 siblings, 0 replies; 39+ messages in thread
From: Kirti Wankhede @ 2020-05-04 15:58 UTC (permalink / raw)
To: alex.williamson, cjia
Cc: kevin.tian, ziye.yang, changpeng.liu, yi.l.liu, mlevitsk,
eskultet, cohuck, dgilbert, jonathan.davies, eauger, aik, pasic,
felipe, Zhengxiao.zx, shuangtai.tst, Ken.Xue, zhi.a.wang,
yan.y.zhao, qemu-devel, kvm, Kirti Wankhede
Added a check such that only singleton IOMMU groups can pin pages.
From the point when vendor driver pins any pages, consider IOMMU group
dirty page scope to be limited to pinned pages.
To optimize to avoid walking list often, added flag
pinned_page_dirty_scope to indicate if all of the vfio_groups for each
vfio_domain in the domain_list dirty page scope is limited to pinned
pages. This flag is updated on first pinned pages request for that IOMMU
group and on attaching/detaching group.
Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
Reviewed-by: Neo Jia <cjia@nvidia.com>
---
drivers/vfio/vfio.c | 13 ++++--
drivers/vfio/vfio_iommu_type1.c | 94 +++++++++++++++++++++++++++++++++++++++--
include/linux/vfio.h | 4 +-
3 files changed, 104 insertions(+), 7 deletions(-)
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 765e0e5d83ed..580099afeaff 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -85,6 +85,7 @@ struct vfio_group {
atomic_t opened;
wait_queue_head_t container_q;
bool noiommu;
+ unsigned int dev_counter;
struct kvm *kvm;
struct blocking_notifier_head notifier;
};
@@ -555,6 +556,7 @@ struct vfio_device *vfio_group_create_device(struct vfio_group *group,
mutex_lock(&group->device_lock);
list_add(&device->group_next, &group->device_list);
+ group->dev_counter++;
mutex_unlock(&group->device_lock);
return device;
@@ -567,6 +569,7 @@ static void vfio_device_release(struct kref *kref)
struct vfio_group *group = device->group;
list_del(&device->group_next);
+ group->dev_counter--;
mutex_unlock(&group->device_lock);
dev_set_drvdata(device->dev, NULL);
@@ -1945,6 +1948,9 @@ int vfio_pin_pages(struct device *dev, unsigned long *user_pfn, int npage,
if (!group)
return -ENODEV;
+ if (group->dev_counter > 1)
+ return -EINVAL;
+
ret = vfio_group_add_container_user(group);
if (ret)
goto err_pin_pages;
@@ -1952,7 +1958,8 @@ int vfio_pin_pages(struct device *dev, unsigned long *user_pfn, int npage,
container = group->container;
driver = container->iommu_driver;
if (likely(driver && driver->ops->pin_pages))
- ret = driver->ops->pin_pages(container->iommu_data, user_pfn,
+ ret = driver->ops->pin_pages(container->iommu_data,
+ group->iommu_group, user_pfn,
npage, prot, phys_pfn);
else
ret = -ENOTTY;
@@ -2050,8 +2057,8 @@ int vfio_group_pin_pages(struct vfio_group *group,
driver = container->iommu_driver;
if (likely(driver && driver->ops->pin_pages))
ret = driver->ops->pin_pages(container->iommu_data,
- user_iova_pfn, npage,
- prot, phys_pfn);
+ group->iommu_group, user_iova_pfn,
+ npage, prot, phys_pfn);
else
ret = -ENOTTY;
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index b38d278d7bff..f8b354681516 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -72,6 +72,7 @@ struct vfio_iommu {
bool v2;
bool nesting;
bool dirty_page_tracking;
+ bool pinned_page_dirty_scope;
};
struct vfio_domain {
@@ -99,6 +100,7 @@ struct vfio_group {
struct iommu_group *iommu_group;
struct list_head next;
bool mdev_group; /* An mdev group */
+ bool pinned_page_dirty_scope;
};
struct vfio_iova {
@@ -143,6 +145,10 @@ struct vfio_regions {
static int put_pfn(unsigned long pfn, int prot);
static unsigned long vfio_pgsize_bitmap(struct vfio_iommu *iommu);
+static struct vfio_group *vfio_iommu_find_iommu_group(struct vfio_iommu *iommu,
+ struct iommu_group *iommu_group);
+
+static void update_pinned_page_dirty_scope(struct vfio_iommu *iommu);
/*
* This code handles mapping and unmapping of user data buffers
* into DMA'ble space using the IOMMU
@@ -589,11 +595,13 @@ static int vfio_unpin_page_external(struct vfio_dma *dma, dma_addr_t iova,
}
static int vfio_iommu_type1_pin_pages(void *iommu_data,
+ struct iommu_group *iommu_group,
unsigned long *user_pfn,
int npage, int prot,
unsigned long *phys_pfn)
{
struct vfio_iommu *iommu = iommu_data;
+ struct vfio_group *group;
int i, j, ret;
unsigned long remote_vaddr;
struct vfio_dma *dma;
@@ -667,8 +675,14 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
(vpfn->iova - dma->iova) >> pgshift, 1);
}
}
-
ret = i;
+
+ group = vfio_iommu_find_iommu_group(iommu, iommu_group);
+ if (!group->pinned_page_dirty_scope) {
+ group->pinned_page_dirty_scope = true;
+ update_pinned_page_dirty_scope(iommu);
+ }
+
goto pin_done;
pin_unwind:
@@ -950,8 +964,11 @@ static int vfio_iova_dirty_bitmap(struct vfio_iommu *iommu, dma_addr_t iova,
npages = dma->size >> pgshift;
bitmap_size = DIRTY_BITMAP_BYTES(npages);
- /* mark all pages dirty if all pages are pinned and mapped. */
- if (dma->iommu_mapped)
+ /*
+ * mark all pages dirty if any IOMMU capable device is not able
+ * to report dirty pages and all pages are pinned and mapped.
+ */
+ if (!iommu->pinned_page_dirty_scope && dma->iommu_mapped)
bitmap_set(dma->bitmap, 0, npages);
if (copy_to_user((void __user *)bitmap, dma->bitmap, bitmap_size))
@@ -1472,6 +1489,51 @@ static struct vfio_group *find_iommu_group(struct vfio_domain *domain,
return NULL;
}
+static struct vfio_group *vfio_iommu_find_iommu_group(struct vfio_iommu *iommu,
+ struct iommu_group *iommu_group)
+{
+ struct vfio_domain *domain;
+ struct vfio_group *group = NULL;
+
+ list_for_each_entry(domain, &iommu->domain_list, next) {
+ group = find_iommu_group(domain, iommu_group);
+ if (group)
+ return group;
+ }
+
+ if (iommu->external_domain)
+ group = find_iommu_group(iommu->external_domain, iommu_group);
+
+ return group;
+}
+
+static void update_pinned_page_dirty_scope(struct vfio_iommu *iommu)
+{
+ struct vfio_domain *domain;
+ struct vfio_group *group;
+
+ list_for_each_entry(domain, &iommu->domain_list, next) {
+ list_for_each_entry(group, &domain->group_list, next) {
+ if (!group->pinned_page_dirty_scope) {
+ iommu->pinned_page_dirty_scope = false;
+ return;
+ }
+ }
+ }
+
+ if (iommu->external_domain) {
+ domain = iommu->external_domain;
+ list_for_each_entry(group, &domain->group_list, next) {
+ if (!group->pinned_page_dirty_scope) {
+ iommu->pinned_page_dirty_scope = false;
+ return;
+ }
+ }
+ }
+
+ iommu->pinned_page_dirty_scope = true;
+}
+
static bool vfio_iommu_has_sw_msi(struct list_head *group_resv_regions,
phys_addr_t *base)
{
@@ -1878,6 +1940,16 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
list_add(&group->next,
&iommu->external_domain->group_list);
+ /*
+ * Non-iommu backed group cannot dirty memory directly,
+ * it can only use interfaces that provide dirty
+ * tracking.
+ * The iommu scope can only be promoted with the
+ * addition of a dirty tracking group.
+ */
+ group->pinned_page_dirty_scope = true;
+ if (!iommu->pinned_page_dirty_scope)
+ update_pinned_page_dirty_scope(iommu);
mutex_unlock(&iommu->lock);
return 0;
@@ -2000,6 +2072,13 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
done:
/* Delete the old one and insert new iova list */
vfio_iommu_iova_insert_copy(iommu, &iova_copy);
+
+ /*
+ * An iommu backed group can dirty memory directly and therefore
+ * demotes the iommu scope until it declares itself dirty tracking
+ * capable via the page pinning interface.
+ */
+ iommu->pinned_page_dirty_scope = false;
mutex_unlock(&iommu->lock);
vfio_iommu_resv_free(&group_resv_regions);
@@ -2152,6 +2231,7 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
struct vfio_iommu *iommu = iommu_data;
struct vfio_domain *domain;
struct vfio_group *group;
+ bool update_dirty_scope = false;
LIST_HEAD(iova_copy);
mutex_lock(&iommu->lock);
@@ -2159,6 +2239,7 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
if (iommu->external_domain) {
group = find_iommu_group(iommu->external_domain, iommu_group);
if (group) {
+ update_dirty_scope = !group->pinned_page_dirty_scope;
list_del(&group->next);
kfree(group);
@@ -2188,6 +2269,7 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
continue;
vfio_iommu_detach_group(domain, group);
+ update_dirty_scope = !group->pinned_page_dirty_scope;
list_del(&group->next);
kfree(group);
/*
@@ -2218,6 +2300,12 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
vfio_iommu_iova_free(&iova_copy);
detach_group_done:
+ /*
+ * Removal of a group without dirty tracking may allow the iommu scope
+ * to be promoted.
+ */
+ if (update_dirty_scope)
+ update_pinned_page_dirty_scope(iommu);
mutex_unlock(&iommu->lock);
}
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 5d92ee15d098..38d3c6a8dc7e 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -76,7 +76,9 @@ struct vfio_iommu_driver_ops {
struct iommu_group *group);
void (*detach_group)(void *iommu_data,
struct iommu_group *group);
- int (*pin_pages)(void *iommu_data, unsigned long *user_pfn,
+ int (*pin_pages)(void *iommu_data,
+ struct iommu_group *group,
+ unsigned long *user_pfn,
int npage, int prot,
unsigned long *phys_pfn);
int (*unpin_pages)(void *iommu_data,
--
2.7.0
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH Kernel v18 7/7] vfio: Selective dirty page tracking if IOMMU backed device pins pages
@ 2020-05-04 15:58 ` Kirti Wankhede
0 siblings, 0 replies; 39+ messages in thread
From: Kirti Wankhede @ 2020-05-04 15:58 UTC (permalink / raw)
To: alex.williamson, cjia
Cc: Zhengxiao.zx, kevin.tian, yi.l.liu, yan.y.zhao, kvm, eskultet,
ziye.yang, qemu-devel, cohuck, shuangtai.tst, dgilbert,
zhi.a.wang, mlevitsk, pasic, aik, Kirti Wankhede, eauger, felipe,
jonathan.davies, changpeng.liu, Ken.Xue
Added a check such that only singleton IOMMU groups can pin pages.
From the point when vendor driver pins any pages, consider IOMMU group
dirty page scope to be limited to pinned pages.
To optimize to avoid walking list often, added flag
pinned_page_dirty_scope to indicate if all of the vfio_groups for each
vfio_domain in the domain_list dirty page scope is limited to pinned
pages. This flag is updated on first pinned pages request for that IOMMU
group and on attaching/detaching group.
Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
Reviewed-by: Neo Jia <cjia@nvidia.com>
---
drivers/vfio/vfio.c | 13 ++++--
drivers/vfio/vfio_iommu_type1.c | 94 +++++++++++++++++++++++++++++++++++++++--
include/linux/vfio.h | 4 +-
3 files changed, 104 insertions(+), 7 deletions(-)
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 765e0e5d83ed..580099afeaff 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -85,6 +85,7 @@ struct vfio_group {
atomic_t opened;
wait_queue_head_t container_q;
bool noiommu;
+ unsigned int dev_counter;
struct kvm *kvm;
struct blocking_notifier_head notifier;
};
@@ -555,6 +556,7 @@ struct vfio_device *vfio_group_create_device(struct vfio_group *group,
mutex_lock(&group->device_lock);
list_add(&device->group_next, &group->device_list);
+ group->dev_counter++;
mutex_unlock(&group->device_lock);
return device;
@@ -567,6 +569,7 @@ static void vfio_device_release(struct kref *kref)
struct vfio_group *group = device->group;
list_del(&device->group_next);
+ group->dev_counter--;
mutex_unlock(&group->device_lock);
dev_set_drvdata(device->dev, NULL);
@@ -1945,6 +1948,9 @@ int vfio_pin_pages(struct device *dev, unsigned long *user_pfn, int npage,
if (!group)
return -ENODEV;
+ if (group->dev_counter > 1)
+ return -EINVAL;
+
ret = vfio_group_add_container_user(group);
if (ret)
goto err_pin_pages;
@@ -1952,7 +1958,8 @@ int vfio_pin_pages(struct device *dev, unsigned long *user_pfn, int npage,
container = group->container;
driver = container->iommu_driver;
if (likely(driver && driver->ops->pin_pages))
- ret = driver->ops->pin_pages(container->iommu_data, user_pfn,
+ ret = driver->ops->pin_pages(container->iommu_data,
+ group->iommu_group, user_pfn,
npage, prot, phys_pfn);
else
ret = -ENOTTY;
@@ -2050,8 +2057,8 @@ int vfio_group_pin_pages(struct vfio_group *group,
driver = container->iommu_driver;
if (likely(driver && driver->ops->pin_pages))
ret = driver->ops->pin_pages(container->iommu_data,
- user_iova_pfn, npage,
- prot, phys_pfn);
+ group->iommu_group, user_iova_pfn,
+ npage, prot, phys_pfn);
else
ret = -ENOTTY;
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index b38d278d7bff..f8b354681516 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -72,6 +72,7 @@ struct vfio_iommu {
bool v2;
bool nesting;
bool dirty_page_tracking;
+ bool pinned_page_dirty_scope;
};
struct vfio_domain {
@@ -99,6 +100,7 @@ struct vfio_group {
struct iommu_group *iommu_group;
struct list_head next;
bool mdev_group; /* An mdev group */
+ bool pinned_page_dirty_scope;
};
struct vfio_iova {
@@ -143,6 +145,10 @@ struct vfio_regions {
static int put_pfn(unsigned long pfn, int prot);
static unsigned long vfio_pgsize_bitmap(struct vfio_iommu *iommu);
+static struct vfio_group *vfio_iommu_find_iommu_group(struct vfio_iommu *iommu,
+ struct iommu_group *iommu_group);
+
+static void update_pinned_page_dirty_scope(struct vfio_iommu *iommu);
/*
* This code handles mapping and unmapping of user data buffers
* into DMA'ble space using the IOMMU
@@ -589,11 +595,13 @@ static int vfio_unpin_page_external(struct vfio_dma *dma, dma_addr_t iova,
}
static int vfio_iommu_type1_pin_pages(void *iommu_data,
+ struct iommu_group *iommu_group,
unsigned long *user_pfn,
int npage, int prot,
unsigned long *phys_pfn)
{
struct vfio_iommu *iommu = iommu_data;
+ struct vfio_group *group;
int i, j, ret;
unsigned long remote_vaddr;
struct vfio_dma *dma;
@@ -667,8 +675,14 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
(vpfn->iova - dma->iova) >> pgshift, 1);
}
}
-
ret = i;
+
+ group = vfio_iommu_find_iommu_group(iommu, iommu_group);
+ if (!group->pinned_page_dirty_scope) {
+ group->pinned_page_dirty_scope = true;
+ update_pinned_page_dirty_scope(iommu);
+ }
+
goto pin_done;
pin_unwind:
@@ -950,8 +964,11 @@ static int vfio_iova_dirty_bitmap(struct vfio_iommu *iommu, dma_addr_t iova,
npages = dma->size >> pgshift;
bitmap_size = DIRTY_BITMAP_BYTES(npages);
- /* mark all pages dirty if all pages are pinned and mapped. */
- if (dma->iommu_mapped)
+ /*
+ * mark all pages dirty if any IOMMU capable device is not able
+ * to report dirty pages and all pages are pinned and mapped.
+ */
+ if (!iommu->pinned_page_dirty_scope && dma->iommu_mapped)
bitmap_set(dma->bitmap, 0, npages);
if (copy_to_user((void __user *)bitmap, dma->bitmap, bitmap_size))
@@ -1472,6 +1489,51 @@ static struct vfio_group *find_iommu_group(struct vfio_domain *domain,
return NULL;
}
+static struct vfio_group *vfio_iommu_find_iommu_group(struct vfio_iommu *iommu,
+ struct iommu_group *iommu_group)
+{
+ struct vfio_domain *domain;
+ struct vfio_group *group = NULL;
+
+ list_for_each_entry(domain, &iommu->domain_list, next) {
+ group = find_iommu_group(domain, iommu_group);
+ if (group)
+ return group;
+ }
+
+ if (iommu->external_domain)
+ group = find_iommu_group(iommu->external_domain, iommu_group);
+
+ return group;
+}
+
+static void update_pinned_page_dirty_scope(struct vfio_iommu *iommu)
+{
+ struct vfio_domain *domain;
+ struct vfio_group *group;
+
+ list_for_each_entry(domain, &iommu->domain_list, next) {
+ list_for_each_entry(group, &domain->group_list, next) {
+ if (!group->pinned_page_dirty_scope) {
+ iommu->pinned_page_dirty_scope = false;
+ return;
+ }
+ }
+ }
+
+ if (iommu->external_domain) {
+ domain = iommu->external_domain;
+ list_for_each_entry(group, &domain->group_list, next) {
+ if (!group->pinned_page_dirty_scope) {
+ iommu->pinned_page_dirty_scope = false;
+ return;
+ }
+ }
+ }
+
+ iommu->pinned_page_dirty_scope = true;
+}
+
static bool vfio_iommu_has_sw_msi(struct list_head *group_resv_regions,
phys_addr_t *base)
{
@@ -1878,6 +1940,16 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
list_add(&group->next,
&iommu->external_domain->group_list);
+ /*
+ * Non-iommu backed group cannot dirty memory directly,
+ * it can only use interfaces that provide dirty
+ * tracking.
+ * The iommu scope can only be promoted with the
+ * addition of a dirty tracking group.
+ */
+ group->pinned_page_dirty_scope = true;
+ if (!iommu->pinned_page_dirty_scope)
+ update_pinned_page_dirty_scope(iommu);
mutex_unlock(&iommu->lock);
return 0;
@@ -2000,6 +2072,13 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
done:
/* Delete the old one and insert new iova list */
vfio_iommu_iova_insert_copy(iommu, &iova_copy);
+
+ /*
+ * An iommu backed group can dirty memory directly and therefore
+ * demotes the iommu scope until it declares itself dirty tracking
+ * capable via the page pinning interface.
+ */
+ iommu->pinned_page_dirty_scope = false;
mutex_unlock(&iommu->lock);
vfio_iommu_resv_free(&group_resv_regions);
@@ -2152,6 +2231,7 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
struct vfio_iommu *iommu = iommu_data;
struct vfio_domain *domain;
struct vfio_group *group;
+ bool update_dirty_scope = false;
LIST_HEAD(iova_copy);
mutex_lock(&iommu->lock);
@@ -2159,6 +2239,7 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
if (iommu->external_domain) {
group = find_iommu_group(iommu->external_domain, iommu_group);
if (group) {
+ update_dirty_scope = !group->pinned_page_dirty_scope;
list_del(&group->next);
kfree(group);
@@ -2188,6 +2269,7 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
continue;
vfio_iommu_detach_group(domain, group);
+ update_dirty_scope = !group->pinned_page_dirty_scope;
list_del(&group->next);
kfree(group);
/*
@@ -2218,6 +2300,12 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
vfio_iommu_iova_free(&iova_copy);
detach_group_done:
+ /*
+ * Removal of a group without dirty tracking may allow the iommu scope
+ * to be promoted.
+ */
+ if (update_dirty_scope)
+ update_pinned_page_dirty_scope(iommu);
mutex_unlock(&iommu->lock);
}
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 5d92ee15d098..38d3c6a8dc7e 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -76,7 +76,9 @@ struct vfio_iommu_driver_ops {
struct iommu_group *group);
void (*detach_group)(void *iommu_data,
struct iommu_group *group);
- int (*pin_pages)(void *iommu_data, unsigned long *user_pfn,
+ int (*pin_pages)(void *iommu_data,
+ struct iommu_group *group,
+ unsigned long *user_pfn,
int npage, int prot,
unsigned long *phys_pfn);
int (*unpin_pages)(void *iommu_data,
--
2.7.0
^ permalink raw reply related [flat|nested] 39+ messages in thread