* [PATCH v3 01/10] memory: Introduce RamDiscardMgr for RAM memory regions
2020-12-16 14:11 [PATCH v3 00/10] virtio-mem: vfio support David Hildenbrand
@ 2020-12-16 14:11 ` David Hildenbrand
2020-12-16 14:11 ` [PATCH v3 02/10] virtio-mem: Factor out traversing unplugged ranges David Hildenbrand
` (8 subsequent siblings)
9 siblings, 0 replies; 21+ messages in thread
From: David Hildenbrand @ 2020-12-16 14:11 UTC (permalink / raw)
To: qemu-devel
Cc: Marek Kedzierski, Pankaj Gupta, Wei Yang, David Hildenbrand,
Michael S. Tsirkin, Alex Williamson, Peter Xu,
Dr . David Alan Gilbert, Auger Eric, Pankaj Gupta, teawater,
Jonathan Cameron, Paolo Bonzini, Igor Mammedov
We have some special RAM memory regions (managed by virtio-mem), whereby
the guest agreed to only use selected memory ranges. "unused" parts are
discarded so they won't consume memory - to logically unplug these memory
ranges. Before the VM is allowed to use such logically unplugged memory
again, coordination with the hypervisor is required.
This results in "sparse" mmaps/RAMBlocks/memory regions, whereby only
coordinated parts are valid to be used/accessed by the VM.
In most cases, we don't care about that - e.g., in KVM, we simply have a
single KVM memory slot. However, in case of vfio, registering the
whole region with the kernel results in all pages getting pinned, and
therefore an unexpected high memory consumption - discarding of RAM in
that context is broken.
Let's introduce a way to coordinate discarding/populating memory within a
RAM memory region with such special consumers of RAM memory regions: they
can register as listeners and get updates on memory getting discarded and
populated. Using this machinery, vfio will be able to map only the
currently populated parts, resulting in discarded parts not getting pinned
and not consuming memory.
A RamDiscardMgr has to be set for a memory region before it is getting
mapped, and cannot change while the memory region is mapped.
Note: At some point, we might want to let RAMBlock users (esp. vfio used
for nvme://) consume this interface as well. We'll need RAMBlock notifier
calls when a RAMBlock is getting mapped/unmapped (via the corresponding
memory region), so we can properly register a listener there as well.
Reviewed-by: Pankaj Gupta <pankaj.gupta@cloud.ionos.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: Auger Eric <eric.auger@redhat.com>
Cc: Wei Yang <richard.weiyang@linux.alibaba.com>
Cc: teawater <teawaterz@linux.alibaba.com>
Cc: Marek Kedzierski <mkedzier@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
include/exec/memory.h | 231 ++++++++++++++++++++++++++++++++++++++++++
softmmu/memory.c | 22 ++++
2 files changed, 253 insertions(+)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 521d9901d7..67d9f1f3a7 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -42,6 +42,12 @@ typedef struct IOMMUMemoryRegionClass IOMMUMemoryRegionClass;
DECLARE_OBJ_CHECKERS(IOMMUMemoryRegion, IOMMUMemoryRegionClass,
IOMMU_MEMORY_REGION, TYPE_IOMMU_MEMORY_REGION)
+#define TYPE_RAM_DISCARD_MGR "qemu:ram-discard-mgr"
+typedef struct RamDiscardMgrClass RamDiscardMgrClass;
+typedef struct RamDiscardMgr RamDiscardMgr;
+DECLARE_OBJ_CHECKERS(RamDiscardMgr, RamDiscardMgrClass, RAM_DISCARD_MGR,
+ TYPE_RAM_DISCARD_MGR);
+
#ifdef CONFIG_FUZZ
void fuzz_dma_read_cb(size_t addr,
size_t len,
@@ -126,6 +132,66 @@ typedef struct IOMMUTLBEvent {
IOMMUTLBEntry entry;
} IOMMUTLBEvent;
+struct RamDiscardListener;
+typedef int (*NotifyRamPopulate)(struct RamDiscardListener *rdl,
+ const MemoryRegion *mr, ram_addr_t offset,
+ ram_addr_t size);
+typedef void (*NotifyRamDiscard)(struct RamDiscardListener *rdl,
+ const MemoryRegion *mr, ram_addr_t offset,
+ ram_addr_t size);
+typedef void (*NotifyRamDiscardAll)(struct RamDiscardListener *rdl,
+ const MemoryRegion *mr);
+
+typedef struct RamDiscardListener {
+ /*
+ * @notify_populate:
+ *
+ * Notification that previously discarded memory is about to get populated.
+ * Listeners are able to object. If any listener objects, already
+ * successfully notified listeners are notified about a discard again.
+ *
+ * @rdl: the #RamDiscardListener getting notified
+ * @mr: the relevant #MemoryRegion
+ * @offset: offset into the #MemoryRegion, aligned to minimum granularity of
+ * the #RamDiscardMgr
+ * @size: the size, aligned to minimum granularity of the #RamDiscardMgr
+ *
+ * Returns 0 on success. If the notification is rejected by the listener,
+ * an error is returned.
+ */
+ NotifyRamPopulate notify_populate;
+
+ /*
+ * @notify_discard:
+ *
+ * Notification that previously populated memory was discarded successfully
+ * and listeners should drop all references to such memory and prevent
+ * new population (e.g., unmap).
+ *
+ * @rdl: the #RamDiscardListener getting notified
+ * @mr: the relevant #MemoryRegion
+ * @offset: offset into the #MemoryRegion, aligned to minimum granularity of
+ * the #RamDiscardMgr
+ * @size: the size, aligned to minimum granularity of the #RamDiscardMgr
+ */
+ NotifyRamDiscard notify_discard;
+
+ /*
+ * @notify_discard_all:
+ *
+ * Notification that all previously populated memory was discarded
+ * successfully.
+ *
+ * Note: this callback is optional. If not set, individual notify_populate()
+ * notifications are triggered.
+ *
+ * @rdl: the #RamDiscardListener getting notified
+ * @mr: the relevant #MemoryRegion
+ */
+ NotifyRamDiscardAll notify_discard_all;
+ QLIST_ENTRY(RamDiscardListener) next;
+} RamDiscardListener;
+
/* RAM is pre-allocated and passed into qemu_ram_alloc_from_ptr */
#define RAM_PREALLOC (1 << 0)
@@ -161,6 +227,16 @@ static inline void iommu_notifier_init(IOMMUNotifier *n, IOMMUNotify fn,
n->iommu_idx = iommu_idx;
}
+static inline void ram_discard_listener_init(RamDiscardListener *rdl,
+ NotifyRamPopulate populate_fn,
+ NotifyRamDiscard discard_fn,
+ NotifyRamDiscardAll discard_all_fn)
+{
+ rdl->notify_populate = populate_fn;
+ rdl->notify_discard = discard_fn;
+ rdl->notify_discard_all = discard_all_fn;
+}
+
/*
* Memory region callbacks
*/
@@ -435,6 +511,126 @@ struct IOMMUMemoryRegionClass {
Error **errp);
};
+/*
+ * RamDiscardMgrClass:
+ *
+ * A #RamDiscardMgr coordinates which parts of specific RAM #MemoryRegion
+ * regions are currently populated to be used/accessed by the VM, notifying
+ * after parts were discarded (freeing up memory) and before parts will be
+ * populated (consuming memory), to be used/acessed by the VM.
+ *
+ * A #RamDiscardMgr can only be set for a RAM #MemoryRegion while the
+ * #MemoryRegion isn't mapped yet; it cannot change while the #MemoryRegion is
+ * mapped.
+ *
+ * The #RamDiscardMgr is intended to be used by technologies that are
+ * incompatible with discarding of RAM (e.g., VFIO, which may pin all
+ * memory inside a #MemoryRegion), and require proper coordination to only
+ * map the currently populated parts, to hinder parts that are expected to
+ * remain discarded from silently getting populated and consuming memory.
+ * Technologies that support discarding of RAM don't have to bother and can
+ * simply map the whole #MemoryRegion.
+ *
+ * An example #RamDiscardMgr is virtio-mem, which logically (un)plugs
+ * memory within an assigned RAM #MemoryRegion, coordinated with the VM.
+ * Logically unplugging memory consists of discarding RAM. The VM agreed to not
+ * access unplugged (discarded) memory - especially via DMA. virtio-mem will
+ * properly coordinate with listeners before memory is plugged (populated),
+ * and after memory is unplugged (discarded).
+ *
+ * Listeners are called in multiples of the minimum granularity and changes are
+ * aligned to the minimum granularity within the #MemoryRegion. Listeners have
+ * to prepare for memory becomming discarded in a different granularity than it
+ * was populated and the other way around.
+ */
+struct RamDiscardMgrClass {
+ /* private */
+ InterfaceClass parent_class;
+
+ /* public */
+
+ /**
+ * @get_min_granularity:
+ *
+ * Get the minimum granularity in which listeners will get notified
+ * about changes within the #MemoryRegion via the #RamDiscardMgr.
+ *
+ * @rdm: the #RamDiscardMgr
+ * @mr: the #MemoryRegion
+ *
+ * Returns the minimum granularity.
+ */
+ uint64_t (*get_min_granularity)(const RamDiscardMgr *rdm,
+ const MemoryRegion *mr);
+
+ /**
+ * @is_populated:
+ *
+ * Check whether the given range within the #MemoryRegion is completely
+ * populated (i.e., no parts are currently discarded). There are no
+ * alignment requirements for the range.
+ *
+ * @rdm: the #RamDiscardMgr
+ * @mr: the #MemoryRegion
+ * @offset: offset into the #MemoryRegion
+ * @size: size in the #MemoryRegion
+ *
+ * Returns whether the given range is completely populated.
+ */
+ bool (*is_populated)(const RamDiscardMgr *rdm, const MemoryRegion *mr,
+ ram_addr_t start, ram_addr_t offset);
+
+ /**
+ * @register_listener:
+ *
+ * Register a #RamDiscardListener for a #MemoryRegion via the
+ * #RamDiscardMgr and immediately notify the #RamDiscardListener about all
+ * populated parts within the #MemoryRegion via the #RamDiscardMgr.
+ *
+ * In case any notification fails, no further notifications are triggered
+ * and an error is logged.
+ *
+ * @rdm: the #RamDiscardMgr
+ * @mr: the #MemoryRegion
+ * @rdl: the #RamDiscardListener
+ */
+ void (*register_listener)(RamDiscardMgr *rdm, const MemoryRegion *mr,
+ RamDiscardListener *rdl);
+
+ /**
+ * @unregister_listener:
+ *
+ * Unregister a previously registered #RamDiscardListener for a
+ * #MemoryRegion via the #RamDiscardMgr after notifying the
+ * #RamDiscardListener about all populated parts becoming unpopulated
+ * within the #MemoryRegion via the #RamDiscardMgr.
+ *
+ * @rdm: the #RamDiscardMgr
+ * @mr: the #MemoryRegion
+ * @rdl: the #RamDiscardListener
+ */
+ void (*unregister_listener)(RamDiscardMgr *rdm, const MemoryRegion *mr,
+ RamDiscardListener *rdl);
+
+ /**
+ * @replay_populated:
+ *
+ * Notify the #RamDiscardListener about all populated parts within the
+ * #MemoryRegion via the #RamDiscardMgr.
+ *
+ * In case any notification fails, no further notifications are triggered.
+ * The listener is not required to be registered.
+ *
+ * @rdm: the #RamDiscardMgr
+ * @mr: the #MemoryRegion
+ * @rdl: the #RamDiscardListener
+ *
+ * Returns 0 on success, or a negative error if any notification failed.
+ */
+ int (*replay_populated)(const RamDiscardMgr *rdm, const MemoryRegion *mr,
+ RamDiscardListener *rdl);
+};
+
typedef struct CoalescedMemoryRange CoalescedMemoryRange;
typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
@@ -481,6 +677,7 @@ struct MemoryRegion {
const char *name;
unsigned ioeventfd_nb;
MemoryRegionIoeventfd *ioeventfds;
+ RamDiscardMgr *rdm; /* Only for RAM */
};
struct IOMMUMemoryRegion {
@@ -1969,6 +2166,40 @@ bool memory_region_present(MemoryRegion *container, hwaddr addr);
*/
bool memory_region_is_mapped(MemoryRegion *mr);
+/**
+ * memory_region_get_ram_discard_mgr: get the #RamDiscardMgr for a
+ * #MemoryRegion
+ *
+ * The #RamDiscardMgr cannot change while a memory region is mapped.
+ *
+ * @mr: the #MemoryRegion
+ */
+RamDiscardMgr *memory_region_get_ram_discard_mgr(MemoryRegion *mr);
+
+/**
+ * memory_region_has_ram_discard_mgr: check whether a #MemoryRegion has a
+ * #RamDiscardMgr assigned
+ *
+ * @mr: the #MemoryRegion
+ */
+static inline bool memory_region_has_ram_discard_mgr(MemoryRegion *mr)
+{
+ return !!memory_region_get_ram_discard_mgr(mr);
+}
+
+/**
+ * memory_region_set_ram_discard_mgr: set the #RamDiscardMgr for a
+ * #MemoryRegion
+ *
+ * This function must not be called for a mapped #MemoryRegion, a #MemoryRegion
+ * that does not cover RAM, or a #MemoryRegion that already has a
+ * #RamDiscardMgr assigned.
+ *
+ * @mr: the #MemoryRegion
+ * @urn: #RamDiscardMgr to set
+ */
+void memory_region_set_ram_discard_mgr(MemoryRegion *mr, RamDiscardMgr *rdm);
+
/**
* memory_region_find: translate an address/size relative to a
* MemoryRegion into a #MemoryRegionSection.
diff --git a/softmmu/memory.c b/softmmu/memory.c
index 333e1ed7b0..61fa6290e4 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -2025,6 +2025,21 @@ int memory_region_iommu_num_indexes(IOMMUMemoryRegion *iommu_mr)
return imrc->num_indexes(iommu_mr);
}
+RamDiscardMgr *memory_region_get_ram_discard_mgr(MemoryRegion *mr)
+{
+ if (!memory_region_is_mapped(mr) || !memory_region_is_ram(mr)) {
+ return NULL;
+ }
+ return mr->rdm;
+}
+
+void memory_region_set_ram_discard_mgr(MemoryRegion *mr, RamDiscardMgr *rdm)
+{
+ g_assert(memory_region_is_ram(mr) && !memory_region_is_mapped(mr));
+ g_assert(!rdm || !mr->rdm);
+ mr->rdm = rdm;
+}
+
void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
{
uint8_t mask = 1 << client;
@@ -3306,10 +3321,17 @@ static const TypeInfo iommu_memory_region_info = {
.abstract = true,
};
+static const TypeInfo ram_discard_mgr_info = {
+ .parent = TYPE_INTERFACE,
+ .name = TYPE_RAM_DISCARD_MGR,
+ .class_size = sizeof(RamDiscardMgrClass),
+};
+
static void memory_register_types(void)
{
type_register_static(&memory_region_info);
type_register_static(&iommu_memory_region_info);
+ type_register_static(&ram_discard_mgr_info);
}
type_init(memory_register_types)
--
2.29.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v3 02/10] virtio-mem: Factor out traversing unplugged ranges
2020-12-16 14:11 [PATCH v3 00/10] virtio-mem: vfio support David Hildenbrand
2020-12-16 14:11 ` [PATCH v3 01/10] memory: Introduce RamDiscardMgr for RAM memory regions David Hildenbrand
@ 2020-12-16 14:11 ` David Hildenbrand
2020-12-16 14:11 ` [PATCH v3 03/10] virtio-mem: Implement RamDiscardMgr interface David Hildenbrand
` (7 subsequent siblings)
9 siblings, 0 replies; 21+ messages in thread
From: David Hildenbrand @ 2020-12-16 14:11 UTC (permalink / raw)
To: qemu-devel
Cc: Marek Kedzierski, Pankaj Gupta, Wei Yang, David Hildenbrand,
Michael S. Tsirkin, Alex Williamson, Peter Xu,
Dr . David Alan Gilbert, Auger Eric, Pankaj Gupta, teawater,
Jonathan Cameron, Paolo Bonzini, Igor Mammedov
Let's factor out the core logic, to be reused soon.
Reviewed-by: Pankaj Gupta <pankaj.gupta@cloud.ionos.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: Auger Eric <eric.auger@redhat.com>
Cc: Wei Yang <richard.weiyang@linux.alibaba.com>
Cc: teawater <teawaterz@linux.alibaba.com>
Cc: Marek Kedzierski <mkedzier@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/virtio/virtio-mem.c | 86 ++++++++++++++++++++++++------------------
1 file changed, 49 insertions(+), 37 deletions(-)
diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index 655824ff81..471e464171 100644
--- a/hw/virtio/virtio-mem.c
+++ b/hw/virtio/virtio-mem.c
@@ -145,6 +145,33 @@ static bool virtio_mem_is_busy(void)
return migration_in_incoming_postcopy() || !migration_is_idle();
}
+typedef int (*virtio_mem_range_cb)(const VirtIOMEM *vmem, void *arg,
+ uint64_t offset, uint64_t size);
+
+static int virtio_mem_for_each_unplugged_range(const VirtIOMEM *vmem, void *arg,
+ virtio_mem_range_cb cb)
+{
+ unsigned long first_zero_bit, last_zero_bit;
+ uint64_t offset, size;
+ int ret = 0;
+
+ first_zero_bit = find_first_zero_bit(vmem->bitmap, vmem->bitmap_size);
+ while (first_zero_bit < vmem->bitmap_size) {
+ offset = first_zero_bit * vmem->block_size;
+ last_zero_bit = find_next_bit(vmem->bitmap, vmem->bitmap_size,
+ first_zero_bit + 1) - 1;
+ size = (last_zero_bit - first_zero_bit + 1) * vmem->block_size;
+
+ ret = cb(vmem, arg, offset, size);
+ if (ret) {
+ break;
+ }
+ first_zero_bit = find_next_zero_bit(vmem->bitmap, vmem->bitmap_size,
+ last_zero_bit + 2);
+ }
+ return ret;
+}
+
static bool virtio_mem_test_bitmap(VirtIOMEM *vmem, uint64_t start_gpa,
uint64_t size, bool plugged)
{
@@ -594,33 +621,27 @@ static void virtio_mem_device_unrealize(DeviceState *dev)
ram_block_discard_require(false);
}
-static int virtio_mem_restore_unplugged(VirtIOMEM *vmem)
+static int virtio_mem_discard_range_cb(const VirtIOMEM *vmem, void *arg,
+ uint64_t offset, uint64_t size)
{
RAMBlock *rb = vmem->memdev->mr.ram_block;
- unsigned long first_zero_bit, last_zero_bit;
- uint64_t offset, length;
int ret;
- /* Find consecutive unplugged blocks and discard the consecutive range. */
- first_zero_bit = find_first_zero_bit(vmem->bitmap, vmem->bitmap_size);
- while (first_zero_bit < vmem->bitmap_size) {
- offset = first_zero_bit * vmem->block_size;
- last_zero_bit = find_next_bit(vmem->bitmap, vmem->bitmap_size,
- first_zero_bit + 1) - 1;
- length = (last_zero_bit - first_zero_bit + 1) * vmem->block_size;
-
- ret = ram_block_discard_range(rb, offset, length);
- if (ret) {
- error_report("Unexpected error discarding RAM: %s",
- strerror(-ret));
- return -EINVAL;
- }
- first_zero_bit = find_next_zero_bit(vmem->bitmap, vmem->bitmap_size,
- last_zero_bit + 2);
+ ret = ram_block_discard_range(rb, offset, size);
+ if (ret) {
+ error_report("Unexpected error discarding RAM: %s", strerror(-ret));
+ return -EINVAL;
}
return 0;
}
+static int virtio_mem_restore_unplugged(VirtIOMEM *vmem)
+{
+ /* Make sure all memory is really discarded after migration. */
+ return virtio_mem_for_each_unplugged_range(vmem, NULL,
+ virtio_mem_discard_range_cb);
+}
+
static int virtio_mem_post_load(void *opaque, int version_id)
{
if (migration_in_incoming_postcopy()) {
@@ -872,28 +893,19 @@ static void virtio_mem_set_block_size(Object *obj, Visitor *v, const char *name,
vmem->block_size = value;
}
-static void virtio_mem_precopy_exclude_unplugged(VirtIOMEM *vmem)
+static int virtio_mem_precopy_exclude_range_cb(const VirtIOMEM *vmem, void *arg,
+ uint64_t offset, uint64_t size)
{
void * const host = qemu_ram_get_host_addr(vmem->memdev->mr.ram_block);
- unsigned long first_zero_bit, last_zero_bit;
- uint64_t offset, length;
- /*
- * Find consecutive unplugged blocks and exclude them from migration.
- *
- * Note: Blocks cannot get (un)plugged during precopy, no locking needed.
- */
- first_zero_bit = find_first_zero_bit(vmem->bitmap, vmem->bitmap_size);
- while (first_zero_bit < vmem->bitmap_size) {
- offset = first_zero_bit * vmem->block_size;
- last_zero_bit = find_next_bit(vmem->bitmap, vmem->bitmap_size,
- first_zero_bit + 1) - 1;
- length = (last_zero_bit - first_zero_bit + 1) * vmem->block_size;
+ qemu_guest_free_page_hint(host + offset, size);
+ return 0;
+}
- qemu_guest_free_page_hint(host + offset, length);
- first_zero_bit = find_next_zero_bit(vmem->bitmap, vmem->bitmap_size,
- last_zero_bit + 2);
- }
+static void virtio_mem_precopy_exclude_unplugged(VirtIOMEM *vmem)
+{
+ virtio_mem_for_each_unplugged_range(vmem, NULL,
+ virtio_mem_precopy_exclude_range_cb);
}
static int virtio_mem_precopy_notify(NotifierWithReturn *n, void *data)
--
2.29.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v3 03/10] virtio-mem: Implement RamDiscardMgr interface
2020-12-16 14:11 [PATCH v3 00/10] virtio-mem: vfio support David Hildenbrand
2020-12-16 14:11 ` [PATCH v3 01/10] memory: Introduce RamDiscardMgr for RAM memory regions David Hildenbrand
2020-12-16 14:11 ` [PATCH v3 02/10] virtio-mem: Factor out traversing unplugged ranges David Hildenbrand
@ 2020-12-16 14:11 ` David Hildenbrand
2020-12-16 14:11 ` [PATCH v3 04/10] vfio: Query and store the maximum number of DMA mappings David Hildenbrand
` (6 subsequent siblings)
9 siblings, 0 replies; 21+ messages in thread
From: David Hildenbrand @ 2020-12-16 14:11 UTC (permalink / raw)
To: qemu-devel
Cc: Pankaj Gupta, Wei Yang, David Hildenbrand, Michael S. Tsirkin,
Dr . David Alan Gilbert, Peter Xu, Marek Kedzierski, Auger Eric,
Alex Williamson, teawater, Jonathan Cameron, Paolo Bonzini,
Igor Mammedov
Let's properly notify when (un)plugging blocks, after discarding memory
and before allowing the guest to consume memory. Handle errors from
notifiers gracefully (e.g., no remaining VFIO mappings) when plugging,
rolling back the change and telling the guest that the VM is busy.
One special case to take care of is replaying all notifications after
restoring the vmstate. The device starts out with all memory discarded,
so after loading the vmstate, we have to notify about all plugged
blocks.
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: Auger Eric <eric.auger@redhat.com>
Cc: Wei Yang <richard.weiyang@linux.alibaba.com>
Cc: teawater <teawaterz@linux.alibaba.com>
Cc: Marek Kedzierski <mkedzier@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/virtio/virtio-mem.c | 258 ++++++++++++++++++++++++++++++++-
include/hw/virtio/virtio-mem.h | 3 +
2 files changed, 258 insertions(+), 3 deletions(-)
diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index 471e464171..6200813bb8 100644
--- a/hw/virtio/virtio-mem.c
+++ b/hw/virtio/virtio-mem.c
@@ -172,7 +172,110 @@ static int virtio_mem_for_each_unplugged_range(const VirtIOMEM *vmem, void *arg,
return ret;
}
-static bool virtio_mem_test_bitmap(VirtIOMEM *vmem, uint64_t start_gpa,
+static int virtio_mem_for_each_plugged_range(const VirtIOMEM *vmem, void *arg,
+ virtio_mem_range_cb cb)
+{
+ unsigned long first_bit, last_bit;
+ uint64_t offset, size;
+ int ret = 0;
+
+ first_bit = find_first_bit(vmem->bitmap, vmem->bitmap_size);
+ while (first_bit < vmem->bitmap_size) {
+ offset = first_bit * vmem->block_size;
+ last_bit = find_next_zero_bit(vmem->bitmap, vmem->bitmap_size,
+ first_bit + 1) - 1;
+ size = (last_bit - first_bit + 1) * vmem->block_size;
+
+ ret = cb(vmem, arg, offset, size);
+ if (ret) {
+ break;
+ }
+ first_bit = find_next_bit(vmem->bitmap, vmem->bitmap_size,
+ last_bit + 2);
+ }
+ return ret;
+}
+
+static void virtio_mem_notify_unplug(VirtIOMEM *vmem, uint64_t offset,
+ uint64_t size)
+{
+ RamDiscardListener *rdl;
+
+ QLIST_FOREACH(rdl, &vmem->rdl_list, next) {
+ rdl->notify_discard(rdl, &vmem->memdev->mr, offset, size);
+ }
+}
+
+static int virtio_mem_notify_plug(VirtIOMEM *vmem, uint64_t offset,
+ uint64_t size)
+{
+ RamDiscardListener *rdl, *rdl2;
+ int ret = 0, ret2;
+
+ QLIST_FOREACH(rdl, &vmem->rdl_list, next) {
+ ret = rdl->notify_populate(rdl, &vmem->memdev->mr, offset, size);
+ if (ret) {
+ break;
+ }
+ }
+
+ if (ret) {
+ /* Could be a mapping attempt resulted in memory getting populated. */
+ ret2 = ram_block_discard_range(vmem->memdev->mr.ram_block, offset,
+ size);
+ if (ret2) {
+ error_report("Unexpected error discarding RAM: %s",
+ strerror(-ret2));
+ }
+
+ /* Notify all already-notified listeners. */
+ QLIST_FOREACH(rdl2, &vmem->rdl_list, next) {
+ if (rdl2 == rdl) {
+ break;
+ }
+ rdl2->notify_discard(rdl2, &vmem->memdev->mr, offset, size);
+ }
+ }
+ return ret;
+}
+
+static int virtio_mem_notify_discard_range_cb(const VirtIOMEM *vmem, void *arg,
+ uint64_t offset, uint64_t size)
+{
+ RamDiscardListener *rdl;
+
+ QLIST_FOREACH(rdl, &vmem->rdl_list, next) {
+ if (!rdl->notify_discard_all) {
+ rdl->notify_discard(rdl, &vmem->memdev->mr, offset, size);
+ }
+ }
+ return 0;
+}
+
+static void virtio_mem_notify_unplug_all(VirtIOMEM *vmem)
+{
+ bool individual_calls = false;
+ RamDiscardListener *rdl;
+
+ if (!vmem->size) {
+ return;
+ }
+
+ QLIST_FOREACH(rdl, &vmem->rdl_list, next) {
+ if (rdl->notify_discard_all) {
+ rdl->notify_discard_all(rdl, &vmem->memdev->mr);
+ } else {
+ individual_calls = true;
+ }
+ }
+
+ if (individual_calls) {
+ virtio_mem_for_each_unplugged_range(vmem, NULL,
+ virtio_mem_notify_discard_range_cb);
+ }
+}
+
+static bool virtio_mem_test_bitmap(const VirtIOMEM *vmem, uint64_t start_gpa,
uint64_t size, bool plugged)
{
const unsigned long first_bit = (start_gpa - vmem->addr) / vmem->block_size;
@@ -225,7 +328,8 @@ static void virtio_mem_send_response_simple(VirtIOMEM *vmem,
virtio_mem_send_response(vmem, elem, &resp);
}
-static bool virtio_mem_valid_range(VirtIOMEM *vmem, uint64_t gpa, uint64_t size)
+static bool virtio_mem_valid_range(const VirtIOMEM *vmem, uint64_t gpa,
+ uint64_t size)
{
if (!QEMU_IS_ALIGNED(gpa, vmem->block_size)) {
return false;
@@ -259,6 +363,9 @@ static int virtio_mem_set_block_state(VirtIOMEM *vmem, uint64_t start_gpa,
strerror(-ret));
return -EBUSY;
}
+ virtio_mem_notify_unplug(vmem, offset, size);
+ } else if (virtio_mem_notify_plug(vmem, offset, size)) {
+ return -EBUSY;
}
virtio_mem_set_bitmap(vmem, start_gpa, size, plug);
return 0;
@@ -356,6 +463,8 @@ static int virtio_mem_unplug_all(VirtIOMEM *vmem)
error_report("Unexpected error discarding RAM: %s", strerror(-ret));
return -EBUSY;
}
+ virtio_mem_notify_unplug_all(vmem);
+
bitmap_clear(vmem->bitmap, 0, vmem->bitmap_size);
if (vmem->size) {
vmem->size = 0;
@@ -604,6 +713,12 @@ static void virtio_mem_device_realize(DeviceState *dev, Error **errp)
vmstate_register_ram(&vmem->memdev->mr, DEVICE(vmem));
qemu_register_reset(virtio_mem_system_reset, vmem);
precopy_add_notifier(&vmem->precopy_notifier);
+
+ /*
+ * Set ourselves as RamDiscardMgr before the plug handler maps the memory
+ * region and exposes it via an address space.
+ */
+ memory_region_set_ram_discard_mgr(&vmem->memdev->mr, RAM_DISCARD_MGR(vmem));
}
static void virtio_mem_device_unrealize(DeviceState *dev)
@@ -611,6 +726,11 @@ static void virtio_mem_device_unrealize(DeviceState *dev)
VirtIODevice *vdev = VIRTIO_DEVICE(dev);
VirtIOMEM *vmem = VIRTIO_MEM(dev);
+ /*
+ * The unplug handler unmapped the memory region, it cannot be
+ * found via an address space anymore. Unset ourselves.
+ */
+ memory_region_set_ram_discard_mgr(&vmem->memdev->mr, NULL);
precopy_remove_notifier(&vmem->precopy_notifier);
qemu_unregister_reset(virtio_mem_system_reset, vmem);
vmstate_unregister_ram(&vmem->memdev->mr, DEVICE(vmem));
@@ -642,13 +762,41 @@ static int virtio_mem_restore_unplugged(VirtIOMEM *vmem)
virtio_mem_discard_range_cb);
}
+static int virtio_mem_post_load_replay_cb(const VirtIOMEM *vmem, void *arg,
+ uint64_t offset, uint64_t size)
+{
+ RamDiscardListener *rdl;
+ int ret = 0;
+
+ QLIST_FOREACH(rdl, &vmem->rdl_list, next) {
+ ret = rdl->notify_populate(rdl, &vmem->memdev->mr, offset, size);
+ if (ret) {
+ break;
+ }
+ }
+ return ret;
+}
+
static int virtio_mem_post_load(void *opaque, int version_id)
{
+ VirtIOMEM *vmem = VIRTIO_MEM(opaque);
+ int ret;
+
+ /*
+ * We started out with all memory discarded and our memory region is mapped
+ * into an address space. Replay, now that we updated the bitmap.
+ */
+ ret = virtio_mem_for_each_plugged_range(vmem, NULL,
+ virtio_mem_post_load_replay_cb);
+ if (ret) {
+ return ret;
+ }
+
if (migration_in_incoming_postcopy()) {
return 0;
}
- return virtio_mem_restore_unplugged(VIRTIO_MEM(opaque));
+ return virtio_mem_restore_unplugged(vmem);
}
typedef struct VirtIOMEMMigSanityChecks {
@@ -933,6 +1081,7 @@ static void virtio_mem_instance_init(Object *obj)
notifier_list_init(&vmem->size_change_notifiers);
vmem->precopy_notifier.notify = virtio_mem_precopy_notify;
+ QLIST_INIT(&vmem->rdl_list);
object_property_add(obj, VIRTIO_MEM_SIZE_PROP, "size", virtio_mem_get_size,
NULL, NULL, NULL);
@@ -952,11 +1101,104 @@ static Property virtio_mem_properties[] = {
DEFINE_PROP_END_OF_LIST(),
};
+static uint64_t virtio_mem_rdm_get_min_granularity(const RamDiscardMgr *rdm,
+ const MemoryRegion *mr)
+{
+ const VirtIOMEM *vmem = VIRTIO_MEM(rdm);
+
+ g_assert(mr == &vmem->memdev->mr);
+ return vmem->block_size;
+}
+
+static bool virtio_mem_rdm_is_populated(const RamDiscardMgr *rdm,
+ const MemoryRegion *mr,
+ ram_addr_t offset, ram_addr_t size)
+{
+ const VirtIOMEM *vmem = VIRTIO_MEM(rdm);
+ uint64_t start_gpa = QEMU_ALIGN_DOWN(vmem->addr + offset, vmem->block_size);
+ uint64_t end_gpa = QEMU_ALIGN_UP(vmem->addr + offset + size,
+ vmem->block_size);
+
+ g_assert(mr == &vmem->memdev->mr);
+ if (!virtio_mem_valid_range(vmem, start_gpa, end_gpa - start_gpa)) {
+ return false;
+ }
+
+ return virtio_mem_test_bitmap(vmem, start_gpa, end_gpa - start_gpa, true);
+}
+
+static int virtio_mem_notify_populate_range_single_cb(const VirtIOMEM *vmem,
+ void *arg,
+ uint64_t offset,
+ uint64_t size)
+{
+ RamDiscardListener *rdl = arg;
+
+ return rdl->notify_populate(rdl, &vmem->memdev->mr, offset, size);
+}
+
+static int virtio_mem_notify_discard_range_single_cb(const VirtIOMEM *vmem,
+ void *arg,
+ uint64_t offset,
+ uint64_t size)
+{
+ RamDiscardListener *rdl = arg;
+
+ rdl->notify_discard(rdl, &vmem->memdev->mr, offset, size);
+ return 0;
+}
+
+static void virtio_mem_rdm_register_listener(RamDiscardMgr *rdm,
+ const MemoryRegion *mr,
+ RamDiscardListener *rdl)
+{
+ VirtIOMEM *vmem = VIRTIO_MEM(rdm);
+ int ret;
+
+ g_assert(mr == &vmem->memdev->mr);
+ QLIST_INSERT_HEAD(&vmem->rdl_list, rdl, next);
+ ret = virtio_mem_for_each_plugged_range(vmem, rdl,
+ virtio_mem_notify_populate_range_single_cb);
+ if (ret) {
+ error_report("%s: Replaying plugged ranges failed: %s", __func__,
+ strerror(-ret));
+ }
+}
+
+static void virtio_mem_rdm_unregister_listener(RamDiscardMgr *rdm,
+ const MemoryRegion *mr,
+ RamDiscardListener *rdl)
+{
+ VirtIOMEM *vmem = VIRTIO_MEM(rdm);
+
+ g_assert(mr == &vmem->memdev->mr);
+ if (rdl->notify_discard_all) {
+ rdl->notify_discard_all(rdl, &vmem->memdev->mr);
+ } else {
+ virtio_mem_for_each_plugged_range(vmem, rdl,
+ virtio_mem_notify_discard_range_single_cb);
+
+ }
+ QLIST_REMOVE(rdl, next);
+}
+
+static int virtio_mem_rdm_replay_populated(const RamDiscardMgr *rdm,
+ const MemoryRegion *mr,
+ RamDiscardListener *rdl)
+{
+ const VirtIOMEM *vmem = VIRTIO_MEM(rdm);
+
+ g_assert(mr == &vmem->memdev->mr);
+ return virtio_mem_for_each_plugged_range(vmem, rdl,
+ virtio_mem_notify_populate_range_single_cb);
+}
+
static void virtio_mem_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
VirtIOMEMClass *vmc = VIRTIO_MEM_CLASS(klass);
+ RamDiscardMgrClass *rdmc = RAM_DISCARD_MGR_CLASS(klass);
device_class_set_props(dc, virtio_mem_properties);
dc->vmsd = &vmstate_virtio_mem;
@@ -972,6 +1214,12 @@ static void virtio_mem_class_init(ObjectClass *klass, void *data)
vmc->get_memory_region = virtio_mem_get_memory_region;
vmc->add_size_change_notifier = virtio_mem_add_size_change_notifier;
vmc->remove_size_change_notifier = virtio_mem_remove_size_change_notifier;
+
+ rdmc->get_min_granularity = virtio_mem_rdm_get_min_granularity;
+ rdmc->is_populated = virtio_mem_rdm_is_populated;
+ rdmc->register_listener = virtio_mem_rdm_register_listener;
+ rdmc->unregister_listener = virtio_mem_rdm_unregister_listener;
+ rdmc->replay_populated = virtio_mem_rdm_replay_populated;
}
static const TypeInfo virtio_mem_info = {
@@ -981,6 +1229,10 @@ static const TypeInfo virtio_mem_info = {
.instance_init = virtio_mem_instance_init,
.class_init = virtio_mem_class_init,
.class_size = sizeof(VirtIOMEMClass),
+ .interfaces = (InterfaceInfo[]) {
+ { TYPE_RAM_DISCARD_MGR },
+ { }
+ },
};
static void virtio_register_types(void)
diff --git a/include/hw/virtio/virtio-mem.h b/include/hw/virtio/virtio-mem.h
index 4eeb82d5dd..9a6e348fa2 100644
--- a/include/hw/virtio/virtio-mem.h
+++ b/include/hw/virtio/virtio-mem.h
@@ -67,6 +67,9 @@ struct VirtIOMEM {
/* don't migrate unplugged memory */
NotifierWithReturn precopy_notifier;
+
+ /* listeners to notify on plug/unplug activity. */
+ QLIST_HEAD(, RamDiscardListener) rdl_list;
};
struct VirtIOMEMClass {
--
2.29.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v3 04/10] vfio: Query and store the maximum number of DMA mappings
2020-12-16 14:11 [PATCH v3 00/10] virtio-mem: vfio support David Hildenbrand
` (2 preceding siblings ...)
2020-12-16 14:11 ` [PATCH v3 03/10] virtio-mem: Implement RamDiscardMgr interface David Hildenbrand
@ 2020-12-16 14:11 ` David Hildenbrand
2020-12-17 7:43 ` Pankaj Gupta
2020-12-17 17:55 ` Alex Williamson
2020-12-16 14:11 ` [PATCH v3 05/10] vfio: Support for RamDiscardMgr in the !vIOMMU case David Hildenbrand
` (5 subsequent siblings)
9 siblings, 2 replies; 21+ messages in thread
From: David Hildenbrand @ 2020-12-16 14:11 UTC (permalink / raw)
To: qemu-devel
Cc: Pankaj Gupta, Wei Yang, David Hildenbrand, Michael S. Tsirkin,
Dr . David Alan Gilbert, Peter Xu, Marek Kedzierski, Auger Eric,
Alex Williamson, teawater, Jonathan Cameron, Paolo Bonzini,
Igor Mammedov
Let's query the maximum number of DMA mappings by querying the available
mappings when creating the container.
In addition, count the number of DMA mappings and warn when we would
exceed it. This is a preparation for RamDiscardMgr which might
create quite some DMA mappings over time, and we at least want to warn
early that the QEMU setup might be problematic. Use "reserved"
terminology, so we can use this to reserve mappings before they are
actually created.
Note: don't reserve vIOMMU DMA mappings - using the vIOMMU region size
divided by the mapping page size might be a bad indication of what will
happen in practice - we might end up warning all the time.
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: Auger Eric <eric.auger@redhat.com>
Cc: Wei Yang <richard.weiyang@linux.alibaba.com>
Cc: teawater <teawaterz@linux.alibaba.com>
Cc: Marek Kedzierski <mkedzier@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/vfio/common.c | 34 ++++++++++++++++++++++++++++++++++
include/hw/vfio/vfio-common.h | 2 ++
2 files changed, 36 insertions(+)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 6ff1daa763..5ad88d476f 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -288,6 +288,26 @@ const MemoryRegionOps vfio_region_ops = {
},
};
+static void vfio_container_dma_reserve(VFIOContainer *container,
+ unsigned long dma_mappings)
+{
+ bool warned = container->dma_reserved > container->dma_max;
+
+ container->dma_reserved += dma_mappings;
+ if (!warned && container->dma_max &&
+ container->dma_reserved > container->dma_max) {
+ warn_report("%s: possibly running out of DMA mappings. "
+ " Maximum number of DMA mappings: %d", __func__,
+ container->dma_max);
+ }
+}
+
+static void vfio_container_dma_unreserve(VFIOContainer *container,
+ unsigned long dma_mappings)
+{
+ container->dma_reserved -= dma_mappings;
+}
+
/*
* Device state interfaces
*/
@@ -835,6 +855,9 @@ static void vfio_listener_region_add(MemoryListener *listener,
}
}
+ /* We'll need one DMA mapping. */
+ vfio_container_dma_reserve(container, 1);
+
ret = vfio_dma_map(container, iova, int128_get64(llsize),
vaddr, section->readonly);
if (ret) {
@@ -879,6 +902,7 @@ static void vfio_listener_region_del(MemoryListener *listener,
MemoryRegionSection *section)
{
VFIOContainer *container = container_of(listener, VFIOContainer, listener);
+ bool unreserve_on_unmap = true;
hwaddr iova, end;
Int128 llend, llsize;
int ret;
@@ -919,6 +943,7 @@ static void vfio_listener_region_del(MemoryListener *listener,
* based IOMMU where a big unmap flattens a large range of IO-PTEs.
* That may not be true for all IOMMU types.
*/
+ unreserve_on_unmap = false;
}
iova = TARGET_PAGE_ALIGN(section->offset_within_address_space);
@@ -970,6 +995,11 @@ static void vfio_listener_region_del(MemoryListener *listener,
"0x%"HWADDR_PRIx") = %d (%m)",
container, iova, int128_get64(llsize), ret);
}
+
+ /* We previously reserved one DMA mapping. */
+ if (unreserve_on_unmap) {
+ vfio_container_dma_unreserve(container, 1);
+ }
}
memory_region_unref(section->mr);
@@ -1735,6 +1765,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
container->fd = fd;
container->error = NULL;
container->dirty_pages_supported = false;
+ container->dma_max = 0;
QLIST_INIT(&container->giommu_list);
QLIST_INIT(&container->hostwin_list);
@@ -1765,7 +1796,10 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
vfio_host_win_add(container, 0, (hwaddr)-1, info->iova_pgsizes);
container->pgsizes = info->iova_pgsizes;
+ /* The default in the kernel ("dma_entry_limit") is 65535. */
+ container->dma_max = 65535;
if (!ret) {
+ vfio_get_info_dma_avail(info, &container->dma_max);
vfio_get_iommu_info_migration(container, info);
}
g_free(info);
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 6141162d7a..fed0e85f66 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -88,6 +88,8 @@ typedef struct VFIOContainer {
uint64_t dirty_pgsizes;
uint64_t max_dirty_bitmap_size;
unsigned long pgsizes;
+ unsigned int dma_max;
+ unsigned long dma_reserved;
QLIST_HEAD(, VFIOGuestIOMMU) giommu_list;
QLIST_HEAD(, VFIOHostDMAWindow) hostwin_list;
QLIST_HEAD(, VFIOGroup) group_list;
--
2.29.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v3 04/10] vfio: Query and store the maximum number of DMA mappings
2020-12-16 14:11 ` [PATCH v3 04/10] vfio: Query and store the maximum number of DMA mappings David Hildenbrand
@ 2020-12-17 7:43 ` Pankaj Gupta
2020-12-17 17:55 ` Alex Williamson
1 sibling, 0 replies; 21+ messages in thread
From: Pankaj Gupta @ 2020-12-17 7:43 UTC (permalink / raw)
To: David Hildenbrand
Cc: Marek Kedzierski, Wei Yang, Michael S. Tsirkin, Qemu Developers,
Peter Xu, Dr . David Alan Gilbert, Auger Eric, Alex Williamson,
teawater, Jonathan Cameron, Paolo Bonzini, Igor Mammedov
> Let's query the maximum number of DMA mappings by querying the available
> mappings when creating the container.
>
> In addition, count the number of DMA mappings and warn when we would
> exceed it. This is a preparation for RamDiscardMgr which might
> create quite some DMA mappings over time, and we at least want to warn
> early that the QEMU setup might be problematic. Use "reserved"
> terminology, so we can use this to reserve mappings before they are
> actually created.
>
> Note: don't reserve vIOMMU DMA mappings - using the vIOMMU region size
> divided by the mapping page size might be a bad indication of what will
> happen in practice - we might end up warning all the time.
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Auger Eric <eric.auger@redhat.com>
> Cc: Wei Yang <richard.weiyang@linux.alibaba.com>
> Cc: teawater <teawaterz@linux.alibaba.com>
> Cc: Marek Kedzierski <mkedzier@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> hw/vfio/common.c | 34 ++++++++++++++++++++++++++++++++++
> include/hw/vfio/vfio-common.h | 2 ++
> 2 files changed, 36 insertions(+)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 6ff1daa763..5ad88d476f 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -288,6 +288,26 @@ const MemoryRegionOps vfio_region_ops = {
> },
> };
>
> +static void vfio_container_dma_reserve(VFIOContainer *container,
> + unsigned long dma_mappings)
> +{
> + bool warned = container->dma_reserved > container->dma_max;
> +
> + container->dma_reserved += dma_mappings;
> + if (!warned && container->dma_max &&
> + container->dma_reserved > container->dma_max) {
> + warn_report("%s: possibly running out of DMA mappings. "
> + " Maximum number of DMA mappings: %d", __func__,
> + container->dma_max);
> + }
> +}
> +
> +static void vfio_container_dma_unreserve(VFIOContainer *container,
> + unsigned long dma_mappings)
> +{
> + container->dma_reserved -= dma_mappings;
> +}
> +
> /*
> * Device state interfaces
> */
> @@ -835,6 +855,9 @@ static void vfio_listener_region_add(MemoryListener *listener,
> }
> }
>
> + /* We'll need one DMA mapping. */
> + vfio_container_dma_reserve(container, 1);
> +
> ret = vfio_dma_map(container, iova, int128_get64(llsize),
> vaddr, section->readonly);
> if (ret) {
> @@ -879,6 +902,7 @@ static void vfio_listener_region_del(MemoryListener *listener,
> MemoryRegionSection *section)
> {
> VFIOContainer *container = container_of(listener, VFIOContainer, listener);
> + bool unreserve_on_unmap = true;
> hwaddr iova, end;
> Int128 llend, llsize;
> int ret;
> @@ -919,6 +943,7 @@ static void vfio_listener_region_del(MemoryListener *listener,
> * based IOMMU where a big unmap flattens a large range of IO-PTEs.
> * That may not be true for all IOMMU types.
> */
> + unreserve_on_unmap = false;
> }
>
> iova = TARGET_PAGE_ALIGN(section->offset_within_address_space);
> @@ -970,6 +995,11 @@ static void vfio_listener_region_del(MemoryListener *listener,
> "0x%"HWADDR_PRIx") = %d (%m)",
> container, iova, int128_get64(llsize), ret);
> }
> +
> + /* We previously reserved one DMA mapping. */
> + if (unreserve_on_unmap) {
> + vfio_container_dma_unreserve(container, 1);
> + }
> }
>
> memory_region_unref(section->mr);
> @@ -1735,6 +1765,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
> container->fd = fd;
> container->error = NULL;
> container->dirty_pages_supported = false;
> + container->dma_max = 0;
> QLIST_INIT(&container->giommu_list);
> QLIST_INIT(&container->hostwin_list);
>
> @@ -1765,7 +1796,10 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
> vfio_host_win_add(container, 0, (hwaddr)-1, info->iova_pgsizes);
> container->pgsizes = info->iova_pgsizes;
>
> + /* The default in the kernel ("dma_entry_limit") is 65535. */
> + container->dma_max = 65535;
> if (!ret) {
> + vfio_get_info_dma_avail(info, &container->dma_max);
> vfio_get_iommu_info_migration(container, info);
> }
> g_free(info);
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 6141162d7a..fed0e85f66 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -88,6 +88,8 @@ typedef struct VFIOContainer {
> uint64_t dirty_pgsizes;
> uint64_t max_dirty_bitmap_size;
> unsigned long pgsizes;
> + unsigned int dma_max;
> + unsigned long dma_reserved;
> QLIST_HEAD(, VFIOGuestIOMMU) giommu_list;
> QLIST_HEAD(, VFIOHostDMAWindow) hostwin_list;
> QLIST_HEAD(, VFIOGroup) group_list;
Reviewed-by: Pankaj Gupta <pankaj.gupta@cloud.ionos.com>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3 04/10] vfio: Query and store the maximum number of DMA mappings
2020-12-16 14:11 ` [PATCH v3 04/10] vfio: Query and store the maximum number of DMA mappings David Hildenbrand
2020-12-17 7:43 ` Pankaj Gupta
@ 2020-12-17 17:55 ` Alex Williamson
2020-12-17 19:04 ` David Hildenbrand
1 sibling, 1 reply; 21+ messages in thread
From: Alex Williamson @ 2020-12-17 17:55 UTC (permalink / raw)
To: David Hildenbrand
Cc: Pankaj Gupta, Wei Yang, Michael S. Tsirkin, Jonathan Cameron,
qemu-devel, Peter Xu, Dr . David Alan Gilbert, Auger Eric,
teawater, Igor Mammedov, Paolo Bonzini, Marek Kedzierski
On Wed, 16 Dec 2020 15:11:54 +0100
David Hildenbrand <david@redhat.com> wrote:
> Let's query the maximum number of DMA mappings by querying the available
> mappings when creating the container.
>
> In addition, count the number of DMA mappings and warn when we would
> exceed it. This is a preparation for RamDiscardMgr which might
> create quite some DMA mappings over time, and we at least want to warn
> early that the QEMU setup might be problematic. Use "reserved"
> terminology, so we can use this to reserve mappings before they are
> actually created.
This terminology doesn't make much sense to me, we're not actually
performing any kind of reservation.
> Note: don't reserve vIOMMU DMA mappings - using the vIOMMU region size
> divided by the mapping page size might be a bad indication of what will
> happen in practice - we might end up warning all the time.
This suggests we're not really tracking DMA "reservations" at all.
Would something like dma_regions_mappings be a more appropriate
identifier for the thing you're trying to count? We might as well also
keep a counter for dma_iommu_mappings where the sum of those two should
stay below dma_max_mappings.
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Auger Eric <eric.auger@redhat.com>
> Cc: Wei Yang <richard.weiyang@linux.alibaba.com>
> Cc: teawater <teawaterz@linux.alibaba.com>
> Cc: Marek Kedzierski <mkedzier@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> hw/vfio/common.c | 34 ++++++++++++++++++++++++++++++++++
> include/hw/vfio/vfio-common.h | 2 ++
> 2 files changed, 36 insertions(+)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 6ff1daa763..5ad88d476f 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -288,6 +288,26 @@ const MemoryRegionOps vfio_region_ops = {
> },
> };
>
> +static void vfio_container_dma_reserve(VFIOContainer *container,
> + unsigned long dma_mappings)
> +{
> + bool warned = container->dma_reserved > container->dma_max;
> +
> + container->dma_reserved += dma_mappings;
> + if (!warned && container->dma_max &&
> + container->dma_reserved > container->dma_max) {
> + warn_report("%s: possibly running out of DMA mappings. "
> + " Maximum number of DMA mappings: %d", __func__,
> + container->dma_max);
If we kept track of all the mappings we could predict better than
"possibly". Tracing support to track a high water mark might be useful
too.
> + }
> +}
> +
> +static void vfio_container_dma_unreserve(VFIOContainer *container,
> + unsigned long dma_mappings)
> +{
> + container->dma_reserved -= dma_mappings;
> +}
> +
> /*
> * Device state interfaces
> */
> @@ -835,6 +855,9 @@ static void vfio_listener_region_add(MemoryListener *listener,
> }
> }
>
> + /* We'll need one DMA mapping. */
> + vfio_container_dma_reserve(container, 1);
> +
> ret = vfio_dma_map(container, iova, int128_get64(llsize),
> vaddr, section->readonly);
> if (ret) {
> @@ -879,6 +902,7 @@ static void vfio_listener_region_del(MemoryListener *listener,
> MemoryRegionSection *section)
> {
> VFIOContainer *container = container_of(listener, VFIOContainer, listener);
> + bool unreserve_on_unmap = true;
> hwaddr iova, end;
> Int128 llend, llsize;
> int ret;
> @@ -919,6 +943,7 @@ static void vfio_listener_region_del(MemoryListener *listener,
> * based IOMMU where a big unmap flattens a large range of IO-PTEs.
> * That may not be true for all IOMMU types.
> */
> + unreserve_on_unmap = false;
> }
>
> iova = TARGET_PAGE_ALIGN(section->offset_within_address_space);
> @@ -970,6 +995,11 @@ static void vfio_listener_region_del(MemoryListener *listener,
> "0x%"HWADDR_PRIx") = %d (%m)",
> container, iova, int128_get64(llsize), ret);
> }
> +
> + /* We previously reserved one DMA mapping. */
> + if (unreserve_on_unmap) {
> + vfio_container_dma_unreserve(container, 1);
> + }
> }
>
> memory_region_unref(section->mr);
> @@ -1735,6 +1765,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
> container->fd = fd;
> container->error = NULL;
> container->dirty_pages_supported = false;
> + container->dma_max = 0;
> QLIST_INIT(&container->giommu_list);
> QLIST_INIT(&container->hostwin_list);
>
> @@ -1765,7 +1796,10 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
> vfio_host_win_add(container, 0, (hwaddr)-1, info->iova_pgsizes);
> container->pgsizes = info->iova_pgsizes;
>
> + /* The default in the kernel ("dma_entry_limit") is 65535. */
> + container->dma_max = 65535;
> if (!ret) {
> + vfio_get_info_dma_avail(info, &container->dma_max);
> vfio_get_iommu_info_migration(container, info);
> }
> g_free(info);
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 6141162d7a..fed0e85f66 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -88,6 +88,8 @@ typedef struct VFIOContainer {
> uint64_t dirty_pgsizes;
> uint64_t max_dirty_bitmap_size;
> unsigned long pgsizes;
> + unsigned int dma_max;
> + unsigned long dma_reserved;
If dma_max is unsigned int, why do we need an unsigned long to track
how many are in use? Thanks,
Alex
> QLIST_HEAD(, VFIOGuestIOMMU) giommu_list;
> QLIST_HEAD(, VFIOHostDMAWindow) hostwin_list;
> QLIST_HEAD(, VFIOGroup) group_list;
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3 04/10] vfio: Query and store the maximum number of DMA mappings
2020-12-17 17:55 ` Alex Williamson
@ 2020-12-17 19:04 ` David Hildenbrand
2020-12-17 19:37 ` David Hildenbrand
2020-12-17 19:47 ` Alex Williamson
0 siblings, 2 replies; 21+ messages in thread
From: David Hildenbrand @ 2020-12-17 19:04 UTC (permalink / raw)
To: Alex Williamson
Cc: Pankaj Gupta, Wei Yang, Michael S. Tsirkin, Jonathan Cameron,
qemu-devel, Peter Xu, Dr . David Alan Gilbert, Auger Eric,
teawater, Igor Mammedov, Paolo Bonzini, Marek Kedzierski
On 17.12.20 18:55, Alex Williamson wrote:
> On Wed, 16 Dec 2020 15:11:54 +0100
> David Hildenbrand <david@redhat.com> wrote:
>
>> Let's query the maximum number of DMA mappings by querying the available
>> mappings when creating the container.
>>
>> In addition, count the number of DMA mappings and warn when we would
>> exceed it. This is a preparation for RamDiscardMgr which might
>> create quite some DMA mappings over time, and we at least want to warn
>> early that the QEMU setup might be problematic. Use "reserved"
>> terminology, so we can use this to reserve mappings before they are
>> actually created.
>
> This terminology doesn't make much sense to me, we're not actually
> performing any kind of reservation.
I see you spotted the second user which actually performs reservations.
>
>> Note: don't reserve vIOMMU DMA mappings - using the vIOMMU region size
>> divided by the mapping page size might be a bad indication of what will
>> happen in practice - we might end up warning all the time.
>
> This suggests we're not really tracking DMA "reservations" at all.
> Would something like dma_regions_mappings be a more appropriate
> identifier for the thing you're trying to count? We might as well also
Right now I want to count
- Mappings we know we will definitely have (counted in this patch)
- Mappings we know we could eventually have later (RamDiscardMgr)
> keep a counter for dma_iommu_mappings where the sum of those two should
> stay below dma_max_mappings.
We could, however, tracking active IOMMU mappings when removing a memory
region from the address space isn't easily possible - we do a single
vfio_dma_unmap() which might span multiple mappings. Same applies to
RamDiscardMgr. Hard to count how many mappings we actually *currently*
have using that approach.
>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Alex Williamson <alex.williamson@redhat.com>
>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
>> Cc: Peter Xu <peterx@redhat.com>
>> Cc: Auger Eric <eric.auger@redhat.com>
>> Cc: Wei Yang <richard.weiyang@linux.alibaba.com>
>> Cc: teawater <teawaterz@linux.alibaba.com>
>> Cc: Marek Kedzierski <mkedzier@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>> hw/vfio/common.c | 34 ++++++++++++++++++++++++++++++++++
>> include/hw/vfio/vfio-common.h | 2 ++
>> 2 files changed, 36 insertions(+)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 6ff1daa763..5ad88d476f 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -288,6 +288,26 @@ const MemoryRegionOps vfio_region_ops = {
>> },
>> };
>>
>> +static void vfio_container_dma_reserve(VFIOContainer *container,
>> + unsigned long dma_mappings)
>> +{
>> + bool warned = container->dma_reserved > container->dma_max;
>> +
>> + container->dma_reserved += dma_mappings;
>> + if (!warned && container->dma_max &&
>> + container->dma_reserved > container->dma_max) {
>> + warn_report("%s: possibly running out of DMA mappings. "
>> + " Maximum number of DMA mappings: %d", __func__,
>> + container->dma_max);
>
> If we kept track of all the mappings we could predict better than
> "possibly". Tracing support to track a high water mark might be useful
> too.
It's an early warning for reservations e.g., on fundamental setup issues
with virtio-mem.
[...]
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>> index 6141162d7a..fed0e85f66 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -88,6 +88,8 @@ typedef struct VFIOContainer {
>> uint64_t dirty_pgsizes;
>> uint64_t max_dirty_bitmap_size;
>> unsigned long pgsizes;
>> + unsigned int dma_max;
>> + unsigned long dma_reserved;
>
> If dma_max is unsigned int, why do we need an unsigned long to track
> how many are in use? Thanks,
My thinking was that some vfio IOMMU types don't have such a limit
(dma_max == -1) and could allow for more. But thinking again, such a
huge number of mappings is highly unrealistic :)
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3 04/10] vfio: Query and store the maximum number of DMA mappings
2020-12-17 19:04 ` David Hildenbrand
@ 2020-12-17 19:37 ` David Hildenbrand
2020-12-17 19:47 ` Alex Williamson
1 sibling, 0 replies; 21+ messages in thread
From: David Hildenbrand @ 2020-12-17 19:37 UTC (permalink / raw)
To: David Hildenbrand
Cc: Marek Kedzierski, Pankaj Gupta, Wei Yang, Michael S. Tsirkin,
qemu-devel, Peter Xu, Dr . David Alan Gilbert, Auger Eric,
Alex Williamson, teawater, Jonathan Cameron, Paolo Bonzini,
Igor Mammedov
> Am 17.12.2020 um 20:04 schrieb David Hildenbrand <david@redhat.com>:
>
> On 17.12.20 18:55, Alex Williamson wrote:
>>> On Wed, 16 Dec 2020 15:11:54 +0100
>>> David Hildenbrand <david@redhat.com> wrote:
>>>
>>> Let's query the maximum number of DMA mappings by querying the available
>>> mappings when creating the container.
>>>
>>> In addition, count the number of DMA mappings and warn when we would
>>> exceed it. This is a preparation for RamDiscardMgr which might
>>> create quite some DMA mappings over time, and we at least want to warn
>>> early that the QEMU setup might be problematic. Use "reserved"
>>> terminology, so we can use this to reserve mappings before they are
>>> actually created.
>>
>> This terminology doesn't make much sense to me, we're not actually
>> performing any kind of reservation.
>
> I see you spotted the second user which actually performs reservations.
>
>>
>>> Note: don't reserve vIOMMU DMA mappings - using the vIOMMU region size
>>> divided by the mapping page size might be a bad indication of what will
>>> happen in practice - we might end up warning all the time.
>>
>> This suggests we're not really tracking DMA "reservations" at all.
>> Would something like dma_regions_mappings be a more appropriate
>> identifier for the thing you're trying to count? We might as well also
>
> Right now I want to count
> - Mappings we know we will definitely have (counted in this patch)
> - Mappings we know we could eventually have later (RamDiscardMgr)
>
>> keep a counter for dma_iommu_mappings where the sum of those two should
>> stay below dma_max_mappings.
>
> We could, however, tracking active IOMMU mappings when removing a memory
> region from the address space isn't easily possible - we do a single
> vfio_dma_unmap() which might span multiple mappings. Same applies to
> RamDiscardMgr. Hard to count how many mappings we actually *currently*
> have using that approach.
>
Thinking about it, might actually be possible when tracking active mappings per iommu / ram discard mgr as well. Will have a look in the new year - thanks.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 04/10] vfio: Query and store the maximum number of DMA mappings
2020-12-17 19:04 ` David Hildenbrand
2020-12-17 19:37 ` David Hildenbrand
@ 2020-12-17 19:47 ` Alex Williamson
2021-01-07 12:56 ` David Hildenbrand
1 sibling, 1 reply; 21+ messages in thread
From: Alex Williamson @ 2020-12-17 19:47 UTC (permalink / raw)
To: David Hildenbrand
Cc: Pankaj Gupta, Wei Yang, Michael S. Tsirkin, Jonathan Cameron,
qemu-devel, Peter Xu, Dr . David Alan Gilbert, Auger Eric,
teawater, Igor Mammedov, Paolo Bonzini, Marek Kedzierski
On Thu, 17 Dec 2020 20:04:28 +0100
David Hildenbrand <david@redhat.com> wrote:
> On 17.12.20 18:55, Alex Williamson wrote:
> > On Wed, 16 Dec 2020 15:11:54 +0100
> > David Hildenbrand <david@redhat.com> wrote:
> >
> >> Let's query the maximum number of DMA mappings by querying the available
> >> mappings when creating the container.
> >>
> >> In addition, count the number of DMA mappings and warn when we would
> >> exceed it. This is a preparation for RamDiscardMgr which might
> >> create quite some DMA mappings over time, and we at least want to warn
> >> early that the QEMU setup might be problematic. Use "reserved"
> >> terminology, so we can use this to reserve mappings before they are
> >> actually created.
> >
> > This terminology doesn't make much sense to me, we're not actually
> > performing any kind of reservation.
>
> I see you spotted the second user which actually performs reservations.
>
> >
> >> Note: don't reserve vIOMMU DMA mappings - using the vIOMMU region size
> >> divided by the mapping page size might be a bad indication of what will
> >> happen in practice - we might end up warning all the time.
> >
> > This suggests we're not really tracking DMA "reservations" at all.
> > Would something like dma_regions_mappings be a more appropriate
> > identifier for the thing you're trying to count? We might as well also
>
> Right now I want to count
> - Mappings we know we will definitely have (counted in this patch)
> - Mappings we know we could eventually have later (RamDiscardMgr)
>
> > keep a counter for dma_iommu_mappings where the sum of those two should
> > stay below dma_max_mappings.
>
> We could, however, tracking active IOMMU mappings when removing a memory
> region from the address space isn't easily possible - we do a single
> vfio_dma_unmap() which might span multiple mappings. Same applies to
> RamDiscardMgr. Hard to count how many mappings we actually *currently*
> have using that approach.
It's actually easy for the RamDiscardMgr regions, the unmap ioctl
returns the total size of the unmapped extents. Therefore since we
only map granule sized extents, simple math should tell us how many
entries we freed. OTOH, if there are other ways that we unmap multiple
mappings where we don't have those semantics, then it would be
prohibitive.
> >> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> >> Cc: Alex Williamson <alex.williamson@redhat.com>
> >> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >> Cc: Igor Mammedov <imammedo@redhat.com>
> >> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
> >> Cc: Peter Xu <peterx@redhat.com>
> >> Cc: Auger Eric <eric.auger@redhat.com>
> >> Cc: Wei Yang <richard.weiyang@linux.alibaba.com>
> >> Cc: teawater <teawaterz@linux.alibaba.com>
> >> Cc: Marek Kedzierski <mkedzier@redhat.com>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >> hw/vfio/common.c | 34 ++++++++++++++++++++++++++++++++++
> >> include/hw/vfio/vfio-common.h | 2 ++
> >> 2 files changed, 36 insertions(+)
> >>
> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> >> index 6ff1daa763..5ad88d476f 100644
> >> --- a/hw/vfio/common.c
> >> +++ b/hw/vfio/common.c
> >> @@ -288,6 +288,26 @@ const MemoryRegionOps vfio_region_ops = {
> >> },
> >> };
> >>
> >> +static void vfio_container_dma_reserve(VFIOContainer *container,
> >> + unsigned long dma_mappings)
> >> +{
> >> + bool warned = container->dma_reserved > container->dma_max;
> >> +
> >> + container->dma_reserved += dma_mappings;
> >> + if (!warned && container->dma_max &&
> >> + container->dma_reserved > container->dma_max) {
> >> + warn_report("%s: possibly running out of DMA mappings. "
> >> + " Maximum number of DMA mappings: %d", __func__,
> >> + container->dma_max);
> >
> > If we kept track of all the mappings we could predict better than
> > "possibly". Tracing support to track a high water mark might be useful
> > too.
>
> It's an early warning for reservations e.g., on fundamental setup issues
> with virtio-mem.
>
> [...]
>
> >> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> >> index 6141162d7a..fed0e85f66 100644
> >> --- a/include/hw/vfio/vfio-common.h
> >> +++ b/include/hw/vfio/vfio-common.h
> >> @@ -88,6 +88,8 @@ typedef struct VFIOContainer {
> >> uint64_t dirty_pgsizes;
> >> uint64_t max_dirty_bitmap_size;
> >> unsigned long pgsizes;
> >> + unsigned int dma_max;
> >> + unsigned long dma_reserved;
> >
> > If dma_max is unsigned int, why do we need an unsigned long to track
> > how many are in use? Thanks,
>
> My thinking was that some vfio IOMMU types don't have such a limit
> (dma_max == -1) and could allow for more. But thinking again, such a
> huge number of mappings is highly unrealistic :)
Yeah, that's why it's only an unsigned int from the kernel, even with
4K mappings we could handle 16TB worth of individual 4K mappings.
Thanks,
Alex
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3 04/10] vfio: Query and store the maximum number of DMA mappings
2020-12-17 19:47 ` Alex Williamson
@ 2021-01-07 12:56 ` David Hildenbrand
0 siblings, 0 replies; 21+ messages in thread
From: David Hildenbrand @ 2021-01-07 12:56 UTC (permalink / raw)
To: Alex Williamson
Cc: Pankaj Gupta, Wei Yang, Michael S. Tsirkin, Jonathan Cameron,
qemu-devel, Peter Xu, Dr . David Alan Gilbert, Auger Eric,
teawater, Igor Mammedov, Paolo Bonzini, Marek Kedzierski
On 17.12.20 20:47, Alex Williamson wrote:
> On Thu, 17 Dec 2020 20:04:28 +0100
> David Hildenbrand <david@redhat.com> wrote:
>
>> On 17.12.20 18:55, Alex Williamson wrote:
>>> On Wed, 16 Dec 2020 15:11:54 +0100
>>> David Hildenbrand <david@redhat.com> wrote:
>>>
>>>> Let's query the maximum number of DMA mappings by querying the available
>>>> mappings when creating the container.
>>>>
>>>> In addition, count the number of DMA mappings and warn when we would
>>>> exceed it. This is a preparation for RamDiscardMgr which might
>>>> create quite some DMA mappings over time, and we at least want to warn
>>>> early that the QEMU setup might be problematic. Use "reserved"
>>>> terminology, so we can use this to reserve mappings before they are
>>>> actually created.
>>>
>>> This terminology doesn't make much sense to me, we're not actually
>>> performing any kind of reservation.
>>
>> I see you spotted the second user which actually performs reservations.
>>
>>>
>>>> Note: don't reserve vIOMMU DMA mappings - using the vIOMMU region size
>>>> divided by the mapping page size might be a bad indication of what will
>>>> happen in practice - we might end up warning all the time.
>>>
>>> This suggests we're not really tracking DMA "reservations" at all.
>>> Would something like dma_regions_mappings be a more appropriate
>>> identifier for the thing you're trying to count? We might as well also
>>
>> Right now I want to count
>> - Mappings we know we will definitely have (counted in this patch)
>> - Mappings we know we could eventually have later (RamDiscardMgr)
>>
>>> keep a counter for dma_iommu_mappings where the sum of those two should
>>> stay below dma_max_mappings.
>>
>> We could, however, tracking active IOMMU mappings when removing a memory
>> region from the address space isn't easily possible - we do a single
>> vfio_dma_unmap() which might span multiple mappings. Same applies to
>> RamDiscardMgr. Hard to count how many mappings we actually *currently*
>> have using that approach.
>
> It's actually easy for the RamDiscardMgr regions, the unmap ioctl
> returns the total size of the unmapped extents. Therefore since we
> only map granule sized extents, simple math should tell us how many
> entries we freed. OTOH, if there are other ways that we unmap multiple
> mappings where we don't have those semantics, then it would be
> prohibitive.
So, I decided to not track the number of current mappings for now, but
instead use your suggestion to sanity-check via
1. Consulting kvm_get_max_memslots() to guess how many DMA mappings we
might have in the worst case apart from the one via RamDiscardMgr.
2. Calculating the maximum number of DMA mappings that could be consumed
by RamDiscardMgr in the given setup by looking at all entries in the
vrdl list.
Looks much cleaner now.
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 05/10] vfio: Support for RamDiscardMgr in the !vIOMMU case
2020-12-16 14:11 [PATCH v3 00/10] virtio-mem: vfio support David Hildenbrand
` (3 preceding siblings ...)
2020-12-16 14:11 ` [PATCH v3 04/10] vfio: Query and store the maximum number of DMA mappings David Hildenbrand
@ 2020-12-16 14:11 ` David Hildenbrand
2020-12-17 18:36 ` Alex Williamson
2020-12-16 14:11 ` [PATCH v3 06/10] vfio: Support for RamDiscardMgr in the vIOMMU case David Hildenbrand
` (4 subsequent siblings)
9 siblings, 1 reply; 21+ messages in thread
From: David Hildenbrand @ 2020-12-16 14:11 UTC (permalink / raw)
To: qemu-devel
Cc: Pankaj Gupta, Wei Yang, David Hildenbrand, Michael S. Tsirkin,
Dr . David Alan Gilbert, Peter Xu, Marek Kedzierski, Auger Eric,
Alex Williamson, teawater, Jonathan Cameron, Paolo Bonzini,
Igor Mammedov
Implement support for RamDiscardMgr, to prepare for virtio-mem
support. Instead of mapping the whole memory section, we only map
"populated" parts and update the mapping when notified about
discarding/population of memory via the RamDiscardListener. Similarly, when
syncing the dirty bitmaps, sync only the actually mapped (populated) parts
by replaying via the notifier.
Small mapping granularity is problematic for vfio, because we might run out
of mappings. Indicate virito-mem as one of the problematic parts when
warning in vfio_container_dma_reserve() to at least make users aware that
there is such a limitation.
Using virtio-mem with vfio is still blocked via
ram_block_discard_disable()/ram_block_discard_require() after this patch.
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: Auger Eric <eric.auger@redhat.com>
Cc: Wei Yang <richard.weiyang@linux.alibaba.com>
Cc: teawater <teawaterz@linux.alibaba.com>
Cc: Marek Kedzierski <mkedzier@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/vfio/common.c | 213 +++++++++++++++++++++++++++++++++-
include/hw/vfio/vfio-common.h | 13 +++
2 files changed, 225 insertions(+), 1 deletion(-)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 5ad88d476f..b1582be1e8 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -296,7 +296,8 @@ static void vfio_container_dma_reserve(VFIOContainer *container,
container->dma_reserved += dma_mappings;
if (!warned && container->dma_max &&
container->dma_reserved > container->dma_max) {
- warn_report("%s: possibly running out of DMA mappings. "
+ warn_report("%s: possibly running out of DMA mappings. E.g., try"
+ " increasing the 'block-size' of virtio-mem devies."
" Maximum number of DMA mappings: %d", __func__,
container->dma_max);
}
@@ -674,6 +675,146 @@ out:
rcu_read_unlock();
}
+static void vfio_ram_discard_notify_discard(RamDiscardListener *rdl,
+ const MemoryRegion *mr,
+ ram_addr_t offset, ram_addr_t size)
+{
+ VFIORamDiscardListener *vrdl = container_of(rdl, VFIORamDiscardListener,
+ listener);
+ const hwaddr mr_start = MAX(offset, vrdl->offset_within_region);
+ const hwaddr mr_end = MIN(offset + size,
+ vrdl->offset_within_region + vrdl->size);
+ const hwaddr iova = mr_start - vrdl->offset_within_region +
+ vrdl->offset_within_address_space;
+ int ret;
+
+ if (mr_start >= mr_end) {
+ return;
+ }
+
+ /* Unmap with a single call. */
+ ret = vfio_dma_unmap(vrdl->container, iova, mr_end - mr_start, NULL);
+ if (ret) {
+ error_report("%s: vfio_dma_unmap() failed: %s", __func__,
+ strerror(-ret));
+ }
+}
+
+static int vfio_ram_discard_notify_populate(RamDiscardListener *rdl,
+ const MemoryRegion *mr,
+ ram_addr_t offset, ram_addr_t size)
+{
+ VFIORamDiscardListener *vrdl = container_of(rdl, VFIORamDiscardListener,
+ listener);
+ const hwaddr mr_end = MIN(offset + size,
+ vrdl->offset_within_region + vrdl->size);
+ hwaddr mr_start = MAX(offset, vrdl->offset_within_region);
+ hwaddr mr_next, iova;
+ void *vaddr;
+ int ret;
+
+ /*
+ * Map in (aligned within memory region) minimum granularity, so we can
+ * unmap in minimum granularity later.
+ */
+ for (; mr_start < mr_end; mr_start = mr_next) {
+ mr_next = QEMU_ALIGN_UP(mr_start + 1, vrdl->granularity);
+ mr_next = MIN(mr_next, mr_end);
+
+ iova = mr_start - vrdl->offset_within_region +
+ vrdl->offset_within_address_space;
+ vaddr = memory_region_get_ram_ptr(vrdl->mr) + mr_start;
+
+ ret = vfio_dma_map(vrdl->container, iova, mr_next - mr_start,
+ vaddr, mr->readonly);
+ if (ret) {
+ /* Rollback */
+ vfio_ram_discard_notify_discard(rdl, mr, offset, size);
+ return ret;
+ }
+ }
+ return 0;
+}
+
+static void vfio_ram_discard_notify_discard_all(RamDiscardListener *rdl,
+ const MemoryRegion *mr)
+{
+ VFIORamDiscardListener *vrdl = container_of(rdl, VFIORamDiscardListener,
+ listener);
+ int ret;
+
+ /* Unmap with a single call. */
+ ret = vfio_dma_unmap(vrdl->container, vrdl->offset_within_address_space,
+ vrdl->size, NULL);
+ if (ret) {
+ error_report("%s: vfio_dma_unmap() failed: %s", __func__,
+ strerror(-ret));
+ }
+}
+
+static void vfio_register_ram_discard_notifier(VFIOContainer *container,
+ MemoryRegionSection *section)
+{
+ RamDiscardMgr *rdm = memory_region_get_ram_discard_mgr(section->mr);
+ RamDiscardMgrClass *rdmc = RAM_DISCARD_MGR_GET_CLASS(rdm);
+ VFIORamDiscardListener *vrdl;
+
+ vrdl = g_new0(VFIORamDiscardListener, 1);
+ vrdl->container = container;
+ vrdl->mr = section->mr;
+ vrdl->offset_within_region = section->offset_within_region;
+ vrdl->offset_within_address_space = section->offset_within_address_space;
+ vrdl->size = int128_get64(section->size);
+ vrdl->granularity = rdmc->get_min_granularity(rdm, section->mr);
+ vrdl->dma_max = vrdl->size / vrdl->granularity;
+ if (!QEMU_IS_ALIGNED(vrdl->size, vrdl->granularity) ||
+ !QEMU_IS_ALIGNED(vrdl->offset_within_region, vrdl->granularity)) {
+ vrdl->dma_max++;
+ }
+
+ /* Ignore some corner cases not relevant in practice. */
+ g_assert(QEMU_IS_ALIGNED(vrdl->offset_within_region, TARGET_PAGE_SIZE));
+ g_assert(QEMU_IS_ALIGNED(vrdl->offset_within_address_space,
+ TARGET_PAGE_SIZE));
+ g_assert(QEMU_IS_ALIGNED(vrdl->size, TARGET_PAGE_SIZE));
+
+ /* We could consume quite some mappings later. */
+ vfio_container_dma_reserve(container, vrdl->dma_max);
+
+ ram_discard_listener_init(&vrdl->listener,
+ vfio_ram_discard_notify_populate,
+ vfio_ram_discard_notify_discard,
+ vfio_ram_discard_notify_discard_all);
+ rdmc->register_listener(rdm, section->mr, &vrdl->listener);
+ QLIST_INSERT_HEAD(&container->vrdl_list, vrdl, next);
+}
+
+static void vfio_unregister_ram_discard_listener(VFIOContainer *container,
+ MemoryRegionSection *section)
+{
+ RamDiscardMgr *rdm = memory_region_get_ram_discard_mgr(section->mr);
+ RamDiscardMgrClass *rdmc = RAM_DISCARD_MGR_GET_CLASS(rdm);
+ VFIORamDiscardListener *vrdl = NULL;
+
+ QLIST_FOREACH(vrdl, &container->vrdl_list, next) {
+ if (vrdl->mr == section->mr &&
+ vrdl->offset_within_region == section->offset_within_region) {
+ break;
+ }
+ }
+
+ if (!vrdl) {
+ hw_error("vfio: Trying to unregister missing RAM discard listener");
+ }
+
+ rdmc->unregister_listener(rdm, section->mr, &vrdl->listener);
+ QLIST_REMOVE(vrdl, next);
+
+ vfio_container_dma_unreserve(container, vrdl->dma_max);
+
+ g_free(vrdl);
+}
+
static void vfio_listener_region_add(MemoryListener *listener,
MemoryRegionSection *section)
{
@@ -834,6 +975,16 @@ static void vfio_listener_region_add(MemoryListener *listener,
/* Here we assume that memory_region_is_ram(section->mr)==true */
+ /*
+ * For RAM memory regions with a RamDiscardMgr, we only want to
+ * register the actually "used" parts - and update the mapping whenever
+ * we're notified about changes.
+ */
+ if (memory_region_has_ram_discard_mgr(section->mr)) {
+ vfio_register_ram_discard_notifier(container, section);
+ return;
+ }
+
vaddr = memory_region_get_ram_ptr(section->mr) +
section->offset_within_region +
(iova - section->offset_within_address_space);
@@ -975,6 +1126,10 @@ static void vfio_listener_region_del(MemoryListener *listener,
pgmask = (1ULL << ctz64(hostwin->iova_pgsizes)) - 1;
try_unmap = !((iova & pgmask) || (int128_get64(llsize) & pgmask));
+ } else if (memory_region_has_ram_discard_mgr(section->mr)) {
+ vfio_unregister_ram_discard_listener(container, section);
+ /* Unregistering will trigger an unmap. */
+ try_unmap = false;
}
if (try_unmap) {
@@ -1107,6 +1262,59 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
rcu_read_unlock();
}
+static int vfio_ram_discard_notify_dirty_bitmap(RamDiscardListener *rdl,
+ const MemoryRegion *mr,
+ ram_addr_t offset,
+ ram_addr_t size)
+{
+ VFIORamDiscardListener *vrdl = container_of(rdl, VFIORamDiscardListener,
+ listener);
+ const hwaddr mr_start = MAX(offset, vrdl->offset_within_region);
+ const hwaddr mr_end = MIN(offset + size,
+ vrdl->offset_within_region + vrdl->size);
+ const hwaddr iova = mr_start - vrdl->offset_within_region +
+ vrdl->offset_within_address_space;
+ ram_addr_t ram_addr;
+ int ret;
+
+ if (mr_start >= mr_end) {
+ return 0;
+ }
+
+ /*
+ * Sync the whole mapped region (spanning multiple individual mappings)
+ * in one go.
+ */
+ ram_addr = memory_region_get_ram_addr(vrdl->mr) + mr_start;
+ ret = vfio_get_dirty_bitmap(vrdl->container, iova, mr_end - mr_start,
+ ram_addr);
+ return ret;
+}
+
+static int vfio_sync_ram_discard_listener_dirty_bitmap(VFIOContainer *container,
+ MemoryRegionSection *section)
+{
+ RamDiscardMgr *rdm = memory_region_get_ram_discard_mgr(section->mr);
+ RamDiscardMgrClass *rdmc = RAM_DISCARD_MGR_GET_CLASS(rdm);
+ VFIORamDiscardListener tmp_vrdl, *vrdl = NULL;
+
+ QLIST_FOREACH(vrdl, &container->vrdl_list, next) {
+ if (vrdl->mr == section->mr &&
+ vrdl->offset_within_region == section->offset_within_region) {
+ break;
+ }
+ }
+
+ if (!vrdl) {
+ hw_error("vfio: Trying to sync missing RAM discard listener");
+ }
+
+ tmp_vrdl = *vrdl;
+ ram_discard_listener_init(&tmp_vrdl.listener,
+ vfio_ram_discard_notify_dirty_bitmap, NULL, NULL);
+ return rdmc->replay_populated(rdm, section->mr, &tmp_vrdl.listener);
+}
+
static int vfio_sync_dirty_bitmap(VFIOContainer *container,
MemoryRegionSection *section)
{
@@ -1138,6 +1346,8 @@ static int vfio_sync_dirty_bitmap(VFIOContainer *container,
}
}
return 0;
+ } else if (memory_region_has_ram_discard_mgr(section->mr)) {
+ return vfio_sync_ram_discard_listener_dirty_bitmap(container, section);
}
ram_addr = memory_region_get_ram_addr(section->mr) +
@@ -1768,6 +1978,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
container->dma_max = 0;
QLIST_INIT(&container->giommu_list);
QLIST_INIT(&container->hostwin_list);
+ QLIST_INIT(&container->vrdl_list);
ret = vfio_init_container(container, group->fd, errp);
if (ret) {
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index fed0e85f66..fba5a14c8b 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -93,6 +93,7 @@ typedef struct VFIOContainer {
QLIST_HEAD(, VFIOGuestIOMMU) giommu_list;
QLIST_HEAD(, VFIOHostDMAWindow) hostwin_list;
QLIST_HEAD(, VFIOGroup) group_list;
+ QLIST_HEAD(, VFIORamDiscardListener) vrdl_list;
QLIST_ENTRY(VFIOContainer) next;
} VFIOContainer;
@@ -104,6 +105,18 @@ typedef struct VFIOGuestIOMMU {
QLIST_ENTRY(VFIOGuestIOMMU) giommu_next;
} VFIOGuestIOMMU;
+typedef struct VFIORamDiscardListener {
+ VFIOContainer *container;
+ MemoryRegion *mr;
+ hwaddr offset_within_region;
+ hwaddr offset_within_address_space;
+ hwaddr size;
+ uint64_t granularity;
+ unsigned long dma_max;
+ RamDiscardListener listener;
+ QLIST_ENTRY(VFIORamDiscardListener) next;
+} VFIORamDiscardListener;
+
typedef struct VFIOHostDMAWindow {
hwaddr min_iova;
hwaddr max_iova;
--
2.29.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v3 05/10] vfio: Support for RamDiscardMgr in the !vIOMMU case
2020-12-16 14:11 ` [PATCH v3 05/10] vfio: Support for RamDiscardMgr in the !vIOMMU case David Hildenbrand
@ 2020-12-17 18:36 ` Alex Williamson
2020-12-17 18:55 ` David Hildenbrand
0 siblings, 1 reply; 21+ messages in thread
From: Alex Williamson @ 2020-12-17 18:36 UTC (permalink / raw)
To: David Hildenbrand
Cc: Pankaj Gupta, Wei Yang, Michael S. Tsirkin, Jonathan Cameron,
qemu-devel, Peter Xu, Dr . David Alan Gilbert, Auger Eric,
teawater, Igor Mammedov, Paolo Bonzini, Marek Kedzierski
On Wed, 16 Dec 2020 15:11:55 +0100
David Hildenbrand <david@redhat.com> wrote:
> Implement support for RamDiscardMgr, to prepare for virtio-mem
> support. Instead of mapping the whole memory section, we only map
> "populated" parts and update the mapping when notified about
> discarding/population of memory via the RamDiscardListener. Similarly, when
> syncing the dirty bitmaps, sync only the actually mapped (populated) parts
> by replaying via the notifier.
>
> Small mapping granularity is problematic for vfio, because we might run out
> of mappings. Indicate virito-mem as one of the problematic parts when
> warning in vfio_container_dma_reserve() to at least make users aware that
> there is such a limitation.
>
> Using virtio-mem with vfio is still blocked via
> ram_block_discard_disable()/ram_block_discard_require() after this patch.
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Auger Eric <eric.auger@redhat.com>
> Cc: Wei Yang <richard.weiyang@linux.alibaba.com>
> Cc: teawater <teawaterz@linux.alibaba.com>
> Cc: Marek Kedzierski <mkedzier@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> hw/vfio/common.c | 213 +++++++++++++++++++++++++++++++++-
> include/hw/vfio/vfio-common.h | 13 +++
> 2 files changed, 225 insertions(+), 1 deletion(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 5ad88d476f..b1582be1e8 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -296,7 +296,8 @@ static void vfio_container_dma_reserve(VFIOContainer *container,
> container->dma_reserved += dma_mappings;
> if (!warned && container->dma_max &&
> container->dma_reserved > container->dma_max) {
> - warn_report("%s: possibly running out of DMA mappings. "
> + warn_report("%s: possibly running out of DMA mappings. E.g., try"
> + " increasing the 'block-size' of virtio-mem devies."
> " Maximum number of DMA mappings: %d", __func__,
> container->dma_max);
> }
> @@ -674,6 +675,146 @@ out:
> rcu_read_unlock();
> }
>
> +static void vfio_ram_discard_notify_discard(RamDiscardListener *rdl,
> + const MemoryRegion *mr,
> + ram_addr_t offset, ram_addr_t size)
> +{
> + VFIORamDiscardListener *vrdl = container_of(rdl, VFIORamDiscardListener,
> + listener);
> + const hwaddr mr_start = MAX(offset, vrdl->offset_within_region);
> + const hwaddr mr_end = MIN(offset + size,
> + vrdl->offset_within_region + vrdl->size);
> + const hwaddr iova = mr_start - vrdl->offset_within_region +
> + vrdl->offset_within_address_space;
> + int ret;
> +
> + if (mr_start >= mr_end) {
> + return;
> + }
> +
> + /* Unmap with a single call. */
> + ret = vfio_dma_unmap(vrdl->container, iova, mr_end - mr_start, NULL);
> + if (ret) {
> + error_report("%s: vfio_dma_unmap() failed: %s", __func__,
> + strerror(-ret));
> + }
> +}
> +
> +static int vfio_ram_discard_notify_populate(RamDiscardListener *rdl,
> + const MemoryRegion *mr,
> + ram_addr_t offset, ram_addr_t size)
> +{
> + VFIORamDiscardListener *vrdl = container_of(rdl, VFIORamDiscardListener,
> + listener);
> + const hwaddr mr_end = MIN(offset + size,
> + vrdl->offset_within_region + vrdl->size);
> + hwaddr mr_start = MAX(offset, vrdl->offset_within_region);
> + hwaddr mr_next, iova;
> + void *vaddr;
> + int ret;
> +
> + /*
> + * Map in (aligned within memory region) minimum granularity, so we can
> + * unmap in minimum granularity later.
> + */
> + for (; mr_start < mr_end; mr_start = mr_next) {
> + mr_next = QEMU_ALIGN_UP(mr_start + 1, vrdl->granularity);
> + mr_next = MIN(mr_next, mr_end);
> +
> + iova = mr_start - vrdl->offset_within_region +
> + vrdl->offset_within_address_space;
> + vaddr = memory_region_get_ram_ptr(vrdl->mr) + mr_start;
> +
> + ret = vfio_dma_map(vrdl->container, iova, mr_next - mr_start,
> + vaddr, mr->readonly);
> + if (ret) {
> + /* Rollback */
> + vfio_ram_discard_notify_discard(rdl, mr, offset, size);
> + return ret;
> + }
> + }
> + return 0;
> +}
> +
> +static void vfio_ram_discard_notify_discard_all(RamDiscardListener *rdl,
> + const MemoryRegion *mr)
> +{
> + VFIORamDiscardListener *vrdl = container_of(rdl, VFIORamDiscardListener,
> + listener);
> + int ret;
> +
> + /* Unmap with a single call. */
> + ret = vfio_dma_unmap(vrdl->container, vrdl->offset_within_address_space,
> + vrdl->size, NULL);
> + if (ret) {
> + error_report("%s: vfio_dma_unmap() failed: %s", __func__,
> + strerror(-ret));
> + }
> +}
> +
> +static void vfio_register_ram_discard_notifier(VFIOContainer *container,
> + MemoryRegionSection *section)
> +{
> + RamDiscardMgr *rdm = memory_region_get_ram_discard_mgr(section->mr);
> + RamDiscardMgrClass *rdmc = RAM_DISCARD_MGR_GET_CLASS(rdm);
> + VFIORamDiscardListener *vrdl;
> +
> + vrdl = g_new0(VFIORamDiscardListener, 1);
> + vrdl->container = container;
> + vrdl->mr = section->mr;
> + vrdl->offset_within_region = section->offset_within_region;
> + vrdl->offset_within_address_space = section->offset_within_address_space;
> + vrdl->size = int128_get64(section->size);
> + vrdl->granularity = rdmc->get_min_granularity(rdm, section->mr);
> + vrdl->dma_max = vrdl->size / vrdl->granularity;
> + if (!QEMU_IS_ALIGNED(vrdl->size, vrdl->granularity) ||
> + !QEMU_IS_ALIGNED(vrdl->offset_within_region, vrdl->granularity)) {
> + vrdl->dma_max++;
> + }
> +
> + /* Ignore some corner cases not relevant in practice. */
> + g_assert(QEMU_IS_ALIGNED(vrdl->offset_within_region, TARGET_PAGE_SIZE));
> + g_assert(QEMU_IS_ALIGNED(vrdl->offset_within_address_space,
> + TARGET_PAGE_SIZE));
> + g_assert(QEMU_IS_ALIGNED(vrdl->size, TARGET_PAGE_SIZE));
> +
> + /* We could consume quite some mappings later. */
> + vfio_container_dma_reserve(container, vrdl->dma_max);
Aha, I guess this is where the "reservation" aspect begins to appear.
Should this be its own counter though, perhaps
dma_discard_max_mappings? The populate and discard callbacks could
further divide this into used and outstanding counters. However, TBH
I'm not sure I understand the counters since this is probably the most
robust mapping path where we can actually safely nak a populate
callback. Maybe rather than any of these runtime counters we should
just walk the vrdl_list, calculate max mappings, and if that exceeds
some large fraction of available mappings, issue a warning (not that
they wouldn't be useful for tracing). Thanks,
Alex
> +
> + ram_discard_listener_init(&vrdl->listener,
> + vfio_ram_discard_notify_populate,
> + vfio_ram_discard_notify_discard,
> + vfio_ram_discard_notify_discard_all);
> + rdmc->register_listener(rdm, section->mr, &vrdl->listener);
> + QLIST_INSERT_HEAD(&container->vrdl_list, vrdl, next);
> +}
> +
> +static void vfio_unregister_ram_discard_listener(VFIOContainer *container,
> + MemoryRegionSection *section)
> +{
> + RamDiscardMgr *rdm = memory_region_get_ram_discard_mgr(section->mr);
> + RamDiscardMgrClass *rdmc = RAM_DISCARD_MGR_GET_CLASS(rdm);
> + VFIORamDiscardListener *vrdl = NULL;
> +
> + QLIST_FOREACH(vrdl, &container->vrdl_list, next) {
> + if (vrdl->mr == section->mr &&
> + vrdl->offset_within_region == section->offset_within_region) {
> + break;
> + }
> + }
> +
> + if (!vrdl) {
> + hw_error("vfio: Trying to unregister missing RAM discard listener");
> + }
> +
> + rdmc->unregister_listener(rdm, section->mr, &vrdl->listener);
> + QLIST_REMOVE(vrdl, next);
> +
> + vfio_container_dma_unreserve(container, vrdl->dma_max);
> +
> + g_free(vrdl);
> +}
> +
> static void vfio_listener_region_add(MemoryListener *listener,
> MemoryRegionSection *section)
> {
> @@ -834,6 +975,16 @@ static void vfio_listener_region_add(MemoryListener *listener,
>
> /* Here we assume that memory_region_is_ram(section->mr)==true */
>
> + /*
> + * For RAM memory regions with a RamDiscardMgr, we only want to
> + * register the actually "used" parts - and update the mapping whenever
> + * we're notified about changes.
> + */
> + if (memory_region_has_ram_discard_mgr(section->mr)) {
> + vfio_register_ram_discard_notifier(container, section);
> + return;
> + }
> +
> vaddr = memory_region_get_ram_ptr(section->mr) +
> section->offset_within_region +
> (iova - section->offset_within_address_space);
> @@ -975,6 +1126,10 @@ static void vfio_listener_region_del(MemoryListener *listener,
>
> pgmask = (1ULL << ctz64(hostwin->iova_pgsizes)) - 1;
> try_unmap = !((iova & pgmask) || (int128_get64(llsize) & pgmask));
> + } else if (memory_region_has_ram_discard_mgr(section->mr)) {
> + vfio_unregister_ram_discard_listener(container, section);
> + /* Unregistering will trigger an unmap. */
> + try_unmap = false;
> }
>
> if (try_unmap) {
> @@ -1107,6 +1262,59 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> rcu_read_unlock();
> }
>
> +static int vfio_ram_discard_notify_dirty_bitmap(RamDiscardListener *rdl,
> + const MemoryRegion *mr,
> + ram_addr_t offset,
> + ram_addr_t size)
> +{
> + VFIORamDiscardListener *vrdl = container_of(rdl, VFIORamDiscardListener,
> + listener);
> + const hwaddr mr_start = MAX(offset, vrdl->offset_within_region);
> + const hwaddr mr_end = MIN(offset + size,
> + vrdl->offset_within_region + vrdl->size);
> + const hwaddr iova = mr_start - vrdl->offset_within_region +
> + vrdl->offset_within_address_space;
> + ram_addr_t ram_addr;
> + int ret;
> +
> + if (mr_start >= mr_end) {
> + return 0;
> + }
> +
> + /*
> + * Sync the whole mapped region (spanning multiple individual mappings)
> + * in one go.
> + */
> + ram_addr = memory_region_get_ram_addr(vrdl->mr) + mr_start;
> + ret = vfio_get_dirty_bitmap(vrdl->container, iova, mr_end - mr_start,
> + ram_addr);
> + return ret;
> +}
> +
> +static int vfio_sync_ram_discard_listener_dirty_bitmap(VFIOContainer *container,
> + MemoryRegionSection *section)
> +{
> + RamDiscardMgr *rdm = memory_region_get_ram_discard_mgr(section->mr);
> + RamDiscardMgrClass *rdmc = RAM_DISCARD_MGR_GET_CLASS(rdm);
> + VFIORamDiscardListener tmp_vrdl, *vrdl = NULL;
> +
> + QLIST_FOREACH(vrdl, &container->vrdl_list, next) {
> + if (vrdl->mr == section->mr &&
> + vrdl->offset_within_region == section->offset_within_region) {
> + break;
> + }
> + }
> +
> + if (!vrdl) {
> + hw_error("vfio: Trying to sync missing RAM discard listener");
> + }
> +
> + tmp_vrdl = *vrdl;
> + ram_discard_listener_init(&tmp_vrdl.listener,
> + vfio_ram_discard_notify_dirty_bitmap, NULL, NULL);
> + return rdmc->replay_populated(rdm, section->mr, &tmp_vrdl.listener);
> +}
> +
> static int vfio_sync_dirty_bitmap(VFIOContainer *container,
> MemoryRegionSection *section)
> {
> @@ -1138,6 +1346,8 @@ static int vfio_sync_dirty_bitmap(VFIOContainer *container,
> }
> }
> return 0;
> + } else if (memory_region_has_ram_discard_mgr(section->mr)) {
> + return vfio_sync_ram_discard_listener_dirty_bitmap(container, section);
> }
>
> ram_addr = memory_region_get_ram_addr(section->mr) +
> @@ -1768,6 +1978,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
> container->dma_max = 0;
> QLIST_INIT(&container->giommu_list);
> QLIST_INIT(&container->hostwin_list);
> + QLIST_INIT(&container->vrdl_list);
>
> ret = vfio_init_container(container, group->fd, errp);
> if (ret) {
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index fed0e85f66..fba5a14c8b 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -93,6 +93,7 @@ typedef struct VFIOContainer {
> QLIST_HEAD(, VFIOGuestIOMMU) giommu_list;
> QLIST_HEAD(, VFIOHostDMAWindow) hostwin_list;
> QLIST_HEAD(, VFIOGroup) group_list;
> + QLIST_HEAD(, VFIORamDiscardListener) vrdl_list;
> QLIST_ENTRY(VFIOContainer) next;
> } VFIOContainer;
>
> @@ -104,6 +105,18 @@ typedef struct VFIOGuestIOMMU {
> QLIST_ENTRY(VFIOGuestIOMMU) giommu_next;
> } VFIOGuestIOMMU;
>
> +typedef struct VFIORamDiscardListener {
> + VFIOContainer *container;
> + MemoryRegion *mr;
> + hwaddr offset_within_region;
> + hwaddr offset_within_address_space;
> + hwaddr size;
> + uint64_t granularity;
> + unsigned long dma_max;
> + RamDiscardListener listener;
> + QLIST_ENTRY(VFIORamDiscardListener) next;
> +} VFIORamDiscardListener;
> +
> typedef struct VFIOHostDMAWindow {
> hwaddr min_iova;
> hwaddr max_iova;
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3 05/10] vfio: Support for RamDiscardMgr in the !vIOMMU case
2020-12-17 18:36 ` Alex Williamson
@ 2020-12-17 18:55 ` David Hildenbrand
2020-12-17 19:59 ` Alex Williamson
0 siblings, 1 reply; 21+ messages in thread
From: David Hildenbrand @ 2020-12-17 18:55 UTC (permalink / raw)
To: Alex Williamson
Cc: Pankaj Gupta, Wei Yang, Michael S. Tsirkin, Jonathan Cameron,
qemu-devel, Peter Xu, Dr . David Alan Gilbert, Auger Eric,
teawater, Igor Mammedov, Paolo Bonzini, Marek Kedzierski
On 17.12.20 19:36, Alex Williamson wrote:
> On Wed, 16 Dec 2020 15:11:55 +0100
> David Hildenbrand <david@redhat.com> wrote:
>
>> Implement support for RamDiscardMgr, to prepare for virtio-mem
>> support. Instead of mapping the whole memory section, we only map
>> "populated" parts and update the mapping when notified about
>> discarding/population of memory via the RamDiscardListener. Similarly, when
>> syncing the dirty bitmaps, sync only the actually mapped (populated) parts
>> by replaying via the notifier.
>>
>> Small mapping granularity is problematic for vfio, because we might run out
>> of mappings. Indicate virito-mem as one of the problematic parts when
>> warning in vfio_container_dma_reserve() to at least make users aware that
>> there is such a limitation.
>>
>> Using virtio-mem with vfio is still blocked via
>> ram_block_discard_disable()/ram_block_discard_require() after this patch.
>>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Alex Williamson <alex.williamson@redhat.com>
>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
>> Cc: Peter Xu <peterx@redhat.com>
>> Cc: Auger Eric <eric.auger@redhat.com>
>> Cc: Wei Yang <richard.weiyang@linux.alibaba.com>
>> Cc: teawater <teawaterz@linux.alibaba.com>
>> Cc: Marek Kedzierski <mkedzier@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>> hw/vfio/common.c | 213 +++++++++++++++++++++++++++++++++-
>> include/hw/vfio/vfio-common.h | 13 +++
>> 2 files changed, 225 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 5ad88d476f..b1582be1e8 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -296,7 +296,8 @@ static void vfio_container_dma_reserve(VFIOContainer *container,
>> container->dma_reserved += dma_mappings;
>> if (!warned && container->dma_max &&
>> container->dma_reserved > container->dma_max) {
>> - warn_report("%s: possibly running out of DMA mappings. "
>> + warn_report("%s: possibly running out of DMA mappings. E.g., try"
>> + " increasing the 'block-size' of virtio-mem devies."
>> " Maximum number of DMA mappings: %d", __func__,
>> container->dma_max);
>> }
>> @@ -674,6 +675,146 @@ out:
>> rcu_read_unlock();
>> }
>>
>> +static void vfio_ram_discard_notify_discard(RamDiscardListener *rdl,
>> + const MemoryRegion *mr,
>> + ram_addr_t offset, ram_addr_t size)
>> +{
>> + VFIORamDiscardListener *vrdl = container_of(rdl, VFIORamDiscardListener,
>> + listener);
>> + const hwaddr mr_start = MAX(offset, vrdl->offset_within_region);
>> + const hwaddr mr_end = MIN(offset + size,
>> + vrdl->offset_within_region + vrdl->size);
>> + const hwaddr iova = mr_start - vrdl->offset_within_region +
>> + vrdl->offset_within_address_space;
>> + int ret;
>> +
>> + if (mr_start >= mr_end) {
>> + return;
>> + }
>> +
>> + /* Unmap with a single call. */
>> + ret = vfio_dma_unmap(vrdl->container, iova, mr_end - mr_start, NULL);
>> + if (ret) {
>> + error_report("%s: vfio_dma_unmap() failed: %s", __func__,
>> + strerror(-ret));
>> + }
>> +}
>> +
>> +static int vfio_ram_discard_notify_populate(RamDiscardListener *rdl,
>> + const MemoryRegion *mr,
>> + ram_addr_t offset, ram_addr_t size)
>> +{
>> + VFIORamDiscardListener *vrdl = container_of(rdl, VFIORamDiscardListener,
>> + listener);
>> + const hwaddr mr_end = MIN(offset + size,
>> + vrdl->offset_within_region + vrdl->size);
>> + hwaddr mr_start = MAX(offset, vrdl->offset_within_region);
>> + hwaddr mr_next, iova;
>> + void *vaddr;
>> + int ret;
>> +
>> + /*
>> + * Map in (aligned within memory region) minimum granularity, so we can
>> + * unmap in minimum granularity later.
>> + */
>> + for (; mr_start < mr_end; mr_start = mr_next) {
>> + mr_next = QEMU_ALIGN_UP(mr_start + 1, vrdl->granularity);
>> + mr_next = MIN(mr_next, mr_end);
>> +
>> + iova = mr_start - vrdl->offset_within_region +
>> + vrdl->offset_within_address_space;
>> + vaddr = memory_region_get_ram_ptr(vrdl->mr) + mr_start;
>> +
>> + ret = vfio_dma_map(vrdl->container, iova, mr_next - mr_start,
>> + vaddr, mr->readonly);
>> + if (ret) {
>> + /* Rollback */
>> + vfio_ram_discard_notify_discard(rdl, mr, offset, size);
>> + return ret;
>> + }
>> + }
>> + return 0;
>> +}
>> +
>> +static void vfio_ram_discard_notify_discard_all(RamDiscardListener *rdl,
>> + const MemoryRegion *mr)
>> +{
>> + VFIORamDiscardListener *vrdl = container_of(rdl, VFIORamDiscardListener,
>> + listener);
>> + int ret;
>> +
>> + /* Unmap with a single call. */
>> + ret = vfio_dma_unmap(vrdl->container, vrdl->offset_within_address_space,
>> + vrdl->size, NULL);
>> + if (ret) {
>> + error_report("%s: vfio_dma_unmap() failed: %s", __func__,
>> + strerror(-ret));
>> + }
>> +}
>> +
>> +static void vfio_register_ram_discard_notifier(VFIOContainer *container,
>> + MemoryRegionSection *section)
>> +{
>> + RamDiscardMgr *rdm = memory_region_get_ram_discard_mgr(section->mr);
>> + RamDiscardMgrClass *rdmc = RAM_DISCARD_MGR_GET_CLASS(rdm);
>> + VFIORamDiscardListener *vrdl;
>> +
>> + vrdl = g_new0(VFIORamDiscardListener, 1);
>> + vrdl->container = container;
>> + vrdl->mr = section->mr;
>> + vrdl->offset_within_region = section->offset_within_region;
>> + vrdl->offset_within_address_space = section->offset_within_address_space;
>> + vrdl->size = int128_get64(section->size);
>> + vrdl->granularity = rdmc->get_min_granularity(rdm, section->mr);
>> + vrdl->dma_max = vrdl->size / vrdl->granularity;
>> + if (!QEMU_IS_ALIGNED(vrdl->size, vrdl->granularity) ||
>> + !QEMU_IS_ALIGNED(vrdl->offset_within_region, vrdl->granularity)) {
>> + vrdl->dma_max++;
>> + }
>> +
>> + /* Ignore some corner cases not relevant in practice. */
>> + g_assert(QEMU_IS_ALIGNED(vrdl->offset_within_region, TARGET_PAGE_SIZE));
>> + g_assert(QEMU_IS_ALIGNED(vrdl->offset_within_address_space,
>> + TARGET_PAGE_SIZE));
>> + g_assert(QEMU_IS_ALIGNED(vrdl->size, TARGET_PAGE_SIZE));
>> +
>> + /* We could consume quite some mappings later. */
>> + vfio_container_dma_reserve(container, vrdl->dma_max);
>
>
> Aha, I guess this is where the "reservation" aspect begins to appear.
> Should this be its own counter though, perhaps
> dma_discard_max_mappings? The populate and discard callbacks could
> further divide this into used and outstanding counters. However, TBH
> I'm not sure I understand the counters since this is probably the most
> robust mapping path where we can actually safely nak a populate
I'd like to be able to warn early on fundamental setup issues, not only
when accidentally running into these limits later.
> callback. Maybe rather than any of these runtime counters we should
> just walk the vrdl_list, calculate max mappings, and if that exceeds
> some large fraction of available mappings, issue a warning (not that
> they wouldn't be useful for tracing). Thanks,
Sure, we can calculate max mappings from the vrdl_list. But which
fraction to chose? The reservation approach simply considers any
mappings (well, except IOMMU because they are kind of special)
Guidance on the fraction / #mappings to assume we can use appreciated.
Thanks!
>
> Alex
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3 05/10] vfio: Support for RamDiscardMgr in the !vIOMMU case
2020-12-17 18:55 ` David Hildenbrand
@ 2020-12-17 19:59 ` Alex Williamson
2020-12-18 9:11 ` David Hildenbrand
0 siblings, 1 reply; 21+ messages in thread
From: Alex Williamson @ 2020-12-17 19:59 UTC (permalink / raw)
To: David Hildenbrand
Cc: Pankaj Gupta, Wei Yang, Michael S. Tsirkin, Jonathan Cameron,
qemu-devel, Peter Xu, Dr . David Alan Gilbert, Auger Eric,
teawater, Igor Mammedov, Paolo Bonzini, Marek Kedzierski
On Thu, 17 Dec 2020 19:55:55 +0100
David Hildenbrand <david@redhat.com> wrote:
> On 17.12.20 19:36, Alex Williamson wrote:
> > On Wed, 16 Dec 2020 15:11:55 +0100
> > David Hildenbrand <david@redhat.com> wrote:
> >
> >> Implement support for RamDiscardMgr, to prepare for virtio-mem
> >> support. Instead of mapping the whole memory section, we only map
> >> "populated" parts and update the mapping when notified about
> >> discarding/population of memory via the RamDiscardListener. Similarly, when
> >> syncing the dirty bitmaps, sync only the actually mapped (populated) parts
> >> by replaying via the notifier.
> >>
> >> Small mapping granularity is problematic for vfio, because we might run out
> >> of mappings. Indicate virito-mem as one of the problematic parts when
> >> warning in vfio_container_dma_reserve() to at least make users aware that
> >> there is such a limitation.
> >>
> >> Using virtio-mem with vfio is still blocked via
> >> ram_block_discard_disable()/ram_block_discard_require() after this patch.
> >>
> >> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> >> Cc: Alex Williamson <alex.williamson@redhat.com>
> >> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >> Cc: Igor Mammedov <imammedo@redhat.com>
> >> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
> >> Cc: Peter Xu <peterx@redhat.com>
> >> Cc: Auger Eric <eric.auger@redhat.com>
> >> Cc: Wei Yang <richard.weiyang@linux.alibaba.com>
> >> Cc: teawater <teawaterz@linux.alibaba.com>
> >> Cc: Marek Kedzierski <mkedzier@redhat.com>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >> hw/vfio/common.c | 213 +++++++++++++++++++++++++++++++++-
> >> include/hw/vfio/vfio-common.h | 13 +++
> >> 2 files changed, 225 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> >> index 5ad88d476f..b1582be1e8 100644
> >> --- a/hw/vfio/common.c
> >> +++ b/hw/vfio/common.c
> >> @@ -296,7 +296,8 @@ static void vfio_container_dma_reserve(VFIOContainer *container,
> >> container->dma_reserved += dma_mappings;
> >> if (!warned && container->dma_max &&
> >> container->dma_reserved > container->dma_max) {
> >> - warn_report("%s: possibly running out of DMA mappings. "
> >> + warn_report("%s: possibly running out of DMA mappings. E.g., try"
> >> + " increasing the 'block-size' of virtio-mem devies."
> >> " Maximum number of DMA mappings: %d", __func__,
> >> container->dma_max);
> >> }
> >> @@ -674,6 +675,146 @@ out:
> >> rcu_read_unlock();
> >> }
> >>
> >> +static void vfio_ram_discard_notify_discard(RamDiscardListener *rdl,
> >> + const MemoryRegion *mr,
> >> + ram_addr_t offset, ram_addr_t size)
> >> +{
> >> + VFIORamDiscardListener *vrdl = container_of(rdl, VFIORamDiscardListener,
> >> + listener);
> >> + const hwaddr mr_start = MAX(offset, vrdl->offset_within_region);
> >> + const hwaddr mr_end = MIN(offset + size,
> >> + vrdl->offset_within_region + vrdl->size);
> >> + const hwaddr iova = mr_start - vrdl->offset_within_region +
> >> + vrdl->offset_within_address_space;
> >> + int ret;
> >> +
> >> + if (mr_start >= mr_end) {
> >> + return;
> >> + }
> >> +
> >> + /* Unmap with a single call. */
> >> + ret = vfio_dma_unmap(vrdl->container, iova, mr_end - mr_start, NULL);
> >> + if (ret) {
> >> + error_report("%s: vfio_dma_unmap() failed: %s", __func__,
> >> + strerror(-ret));
> >> + }
> >> +}
> >> +
> >> +static int vfio_ram_discard_notify_populate(RamDiscardListener *rdl,
> >> + const MemoryRegion *mr,
> >> + ram_addr_t offset, ram_addr_t size)
> >> +{
> >> + VFIORamDiscardListener *vrdl = container_of(rdl, VFIORamDiscardListener,
> >> + listener);
> >> + const hwaddr mr_end = MIN(offset + size,
> >> + vrdl->offset_within_region + vrdl->size);
> >> + hwaddr mr_start = MAX(offset, vrdl->offset_within_region);
> >> + hwaddr mr_next, iova;
> >> + void *vaddr;
> >> + int ret;
> >> +
> >> + /*
> >> + * Map in (aligned within memory region) minimum granularity, so we can
> >> + * unmap in minimum granularity later.
> >> + */
> >> + for (; mr_start < mr_end; mr_start = mr_next) {
> >> + mr_next = QEMU_ALIGN_UP(mr_start + 1, vrdl->granularity);
> >> + mr_next = MIN(mr_next, mr_end);
> >> +
> >> + iova = mr_start - vrdl->offset_within_region +
> >> + vrdl->offset_within_address_space;
> >> + vaddr = memory_region_get_ram_ptr(vrdl->mr) + mr_start;
> >> +
> >> + ret = vfio_dma_map(vrdl->container, iova, mr_next - mr_start,
> >> + vaddr, mr->readonly);
> >> + if (ret) {
> >> + /* Rollback */
> >> + vfio_ram_discard_notify_discard(rdl, mr, offset, size);
> >> + return ret;
> >> + }
> >> + }
> >> + return 0;
> >> +}
> >> +
> >> +static void vfio_ram_discard_notify_discard_all(RamDiscardListener *rdl,
> >> + const MemoryRegion *mr)
> >> +{
> >> + VFIORamDiscardListener *vrdl = container_of(rdl, VFIORamDiscardListener,
> >> + listener);
> >> + int ret;
> >> +
> >> + /* Unmap with a single call. */
> >> + ret = vfio_dma_unmap(vrdl->container, vrdl->offset_within_address_space,
> >> + vrdl->size, NULL);
> >> + if (ret) {
> >> + error_report("%s: vfio_dma_unmap() failed: %s", __func__,
> >> + strerror(-ret));
> >> + }
> >> +}
> >> +
> >> +static void vfio_register_ram_discard_notifier(VFIOContainer *container,
> >> + MemoryRegionSection *section)
> >> +{
> >> + RamDiscardMgr *rdm = memory_region_get_ram_discard_mgr(section->mr);
> >> + RamDiscardMgrClass *rdmc = RAM_DISCARD_MGR_GET_CLASS(rdm);
> >> + VFIORamDiscardListener *vrdl;
> >> +
> >> + vrdl = g_new0(VFIORamDiscardListener, 1);
> >> + vrdl->container = container;
> >> + vrdl->mr = section->mr;
> >> + vrdl->offset_within_region = section->offset_within_region;
> >> + vrdl->offset_within_address_space = section->offset_within_address_space;
> >> + vrdl->size = int128_get64(section->size);
> >> + vrdl->granularity = rdmc->get_min_granularity(rdm, section->mr);
> >> + vrdl->dma_max = vrdl->size / vrdl->granularity;
> >> + if (!QEMU_IS_ALIGNED(vrdl->size, vrdl->granularity) ||
> >> + !QEMU_IS_ALIGNED(vrdl->offset_within_region, vrdl->granularity)) {
> >> + vrdl->dma_max++;
> >> + }
> >> +
> >> + /* Ignore some corner cases not relevant in practice. */
> >> + g_assert(QEMU_IS_ALIGNED(vrdl->offset_within_region, TARGET_PAGE_SIZE));
> >> + g_assert(QEMU_IS_ALIGNED(vrdl->offset_within_address_space,
> >> + TARGET_PAGE_SIZE));
> >> + g_assert(QEMU_IS_ALIGNED(vrdl->size, TARGET_PAGE_SIZE));
> >> +
> >> + /* We could consume quite some mappings later. */
> >> + vfio_container_dma_reserve(container, vrdl->dma_max);
> >
> >
> > Aha, I guess this is where the "reservation" aspect begins to appear.
> > Should this be its own counter though, perhaps
> > dma_discard_max_mappings? The populate and discard callbacks could
> > further divide this into used and outstanding counters. However, TBH
> > I'm not sure I understand the counters since this is probably the most
> > robust mapping path where we can actually safely nak a populate
>
> I'd like to be able to warn early on fundamental setup issues, not only
> when accidentally running into these limits later.
>
> > callback. Maybe rather than any of these runtime counters we should
> > just walk the vrdl_list, calculate max mappings, and if that exceeds
> > some large fraction of available mappings, issue a warning (not that
> > they wouldn't be useful for tracing). Thanks,
>
> Sure, we can calculate max mappings from the vrdl_list. But which
> fraction to chose? The reservation approach simply considers any
> mappings (well, except IOMMU because they are kind of special)
Right, but we're looking at the address space of a device, which should
be exclusively system memory or an IOMMU range, right? There are IOMMUs
that don't restrict the device to the IOVA window, but I'm not sure if
we care about those. If that's true, I'm not sure we need to worry
about the complicated intersection of RamDiscardMgr and vIOMMU both
creating mappings.
> Guidance on the fraction / #mappings to assume we can use appreciated.
Can we use the number of KVM memory slots as a guide? This is
essentially a mechanism for sub-dividing things that exist in a KVM
memory slot, so it seems like (dma_avail - KVM-memory-slots) should be
greater than the # of possible granules we'd map across all the
RamDiscardMgr regions. Maybe a good starting point? Thanks,
Alex
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v3 05/10] vfio: Support for RamDiscardMgr in the !vIOMMU case
2020-12-17 19:59 ` Alex Williamson
@ 2020-12-18 9:11 ` David Hildenbrand
0 siblings, 0 replies; 21+ messages in thread
From: David Hildenbrand @ 2020-12-18 9:11 UTC (permalink / raw)
To: Alex Williamson
Cc: Pankaj Gupta, Wei Yang, Michael S. Tsirkin, Jonathan Cameron,
qemu-devel, Peter Xu, Dr . David Alan Gilbert, Auger Eric,
teawater, Igor Mammedov, Paolo Bonzini, Marek Kedzierski
On 17.12.20 20:59, Alex Williamson wrote:
> On Thu, 17 Dec 2020 19:55:55 +0100
> David Hildenbrand <david@redhat.com> wrote:
>
>> On 17.12.20 19:36, Alex Williamson wrote:
>>> On Wed, 16 Dec 2020 15:11:55 +0100
>>> David Hildenbrand <david@redhat.com> wrote:
>>>
>>>> Implement support for RamDiscardMgr, to prepare for virtio-mem
>>>> support. Instead of mapping the whole memory section, we only map
>>>> "populated" parts and update the mapping when notified about
>>>> discarding/population of memory via the RamDiscardListener. Similarly, when
>>>> syncing the dirty bitmaps, sync only the actually mapped (populated) parts
>>>> by replaying via the notifier.
>>>>
>>>> Small mapping granularity is problematic for vfio, because we might run out
>>>> of mappings. Indicate virito-mem as one of the problematic parts when
>>>> warning in vfio_container_dma_reserve() to at least make users aware that
>>>> there is such a limitation.
>>>>
>>>> Using virtio-mem with vfio is still blocked via
>>>> ram_block_discard_disable()/ram_block_discard_require() after this patch.
>>>>
>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>>> Cc: Alex Williamson <alex.williamson@redhat.com>
>>>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>>> Cc: Igor Mammedov <imammedo@redhat.com>
>>>> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
>>>> Cc: Peter Xu <peterx@redhat.com>
>>>> Cc: Auger Eric <eric.auger@redhat.com>
>>>> Cc: Wei Yang <richard.weiyang@linux.alibaba.com>
>>>> Cc: teawater <teawaterz@linux.alibaba.com>
>>>> Cc: Marek Kedzierski <mkedzier@redhat.com>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>> hw/vfio/common.c | 213 +++++++++++++++++++++++++++++++++-
>>>> include/hw/vfio/vfio-common.h | 13 +++
>>>> 2 files changed, 225 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>>> index 5ad88d476f..b1582be1e8 100644
>>>> --- a/hw/vfio/common.c
>>>> +++ b/hw/vfio/common.c
>>>> @@ -296,7 +296,8 @@ static void vfio_container_dma_reserve(VFIOContainer *container,
>>>> container->dma_reserved += dma_mappings;
>>>> if (!warned && container->dma_max &&
>>>> container->dma_reserved > container->dma_max) {
>>>> - warn_report("%s: possibly running out of DMA mappings. "
>>>> + warn_report("%s: possibly running out of DMA mappings. E.g., try"
>>>> + " increasing the 'block-size' of virtio-mem devies."
>>>> " Maximum number of DMA mappings: %d", __func__,
>>>> container->dma_max);
>>>> }
>>>> @@ -674,6 +675,146 @@ out:
>>>> rcu_read_unlock();
>>>> }
>>>>
>>>> +static void vfio_ram_discard_notify_discard(RamDiscardListener *rdl,
>>>> + const MemoryRegion *mr,
>>>> + ram_addr_t offset, ram_addr_t size)
>>>> +{
>>>> + VFIORamDiscardListener *vrdl = container_of(rdl, VFIORamDiscardListener,
>>>> + listener);
>>>> + const hwaddr mr_start = MAX(offset, vrdl->offset_within_region);
>>>> + const hwaddr mr_end = MIN(offset + size,
>>>> + vrdl->offset_within_region + vrdl->size);
>>>> + const hwaddr iova = mr_start - vrdl->offset_within_region +
>>>> + vrdl->offset_within_address_space;
>>>> + int ret;
>>>> +
>>>> + if (mr_start >= mr_end) {
>>>> + return;
>>>> + }
>>>> +
>>>> + /* Unmap with a single call. */
>>>> + ret = vfio_dma_unmap(vrdl->container, iova, mr_end - mr_start, NULL);
>>>> + if (ret) {
>>>> + error_report("%s: vfio_dma_unmap() failed: %s", __func__,
>>>> + strerror(-ret));
>>>> + }
>>>> +}
>>>> +
>>>> +static int vfio_ram_discard_notify_populate(RamDiscardListener *rdl,
>>>> + const MemoryRegion *mr,
>>>> + ram_addr_t offset, ram_addr_t size)
>>>> +{
>>>> + VFIORamDiscardListener *vrdl = container_of(rdl, VFIORamDiscardListener,
>>>> + listener);
>>>> + const hwaddr mr_end = MIN(offset + size,
>>>> + vrdl->offset_within_region + vrdl->size);
>>>> + hwaddr mr_start = MAX(offset, vrdl->offset_within_region);
>>>> + hwaddr mr_next, iova;
>>>> + void *vaddr;
>>>> + int ret;
>>>> +
>>>> + /*
>>>> + * Map in (aligned within memory region) minimum granularity, so we can
>>>> + * unmap in minimum granularity later.
>>>> + */
>>>> + for (; mr_start < mr_end; mr_start = mr_next) {
>>>> + mr_next = QEMU_ALIGN_UP(mr_start + 1, vrdl->granularity);
>>>> + mr_next = MIN(mr_next, mr_end);
>>>> +
>>>> + iova = mr_start - vrdl->offset_within_region +
>>>> + vrdl->offset_within_address_space;
>>>> + vaddr = memory_region_get_ram_ptr(vrdl->mr) + mr_start;
>>>> +
>>>> + ret = vfio_dma_map(vrdl->container, iova, mr_next - mr_start,
>>>> + vaddr, mr->readonly);
>>>> + if (ret) {
>>>> + /* Rollback */
>>>> + vfio_ram_discard_notify_discard(rdl, mr, offset, size);
>>>> + return ret;
>>>> + }
>>>> + }
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static void vfio_ram_discard_notify_discard_all(RamDiscardListener *rdl,
>>>> + const MemoryRegion *mr)
>>>> +{
>>>> + VFIORamDiscardListener *vrdl = container_of(rdl, VFIORamDiscardListener,
>>>> + listener);
>>>> + int ret;
>>>> +
>>>> + /* Unmap with a single call. */
>>>> + ret = vfio_dma_unmap(vrdl->container, vrdl->offset_within_address_space,
>>>> + vrdl->size, NULL);
>>>> + if (ret) {
>>>> + error_report("%s: vfio_dma_unmap() failed: %s", __func__,
>>>> + strerror(-ret));
>>>> + }
>>>> +}
>>>> +
>>>> +static void vfio_register_ram_discard_notifier(VFIOContainer *container,
>>>> + MemoryRegionSection *section)
>>>> +{
>>>> + RamDiscardMgr *rdm = memory_region_get_ram_discard_mgr(section->mr);
>>>> + RamDiscardMgrClass *rdmc = RAM_DISCARD_MGR_GET_CLASS(rdm);
>>>> + VFIORamDiscardListener *vrdl;
>>>> +
>>>> + vrdl = g_new0(VFIORamDiscardListener, 1);
>>>> + vrdl->container = container;
>>>> + vrdl->mr = section->mr;
>>>> + vrdl->offset_within_region = section->offset_within_region;
>>>> + vrdl->offset_within_address_space = section->offset_within_address_space;
>>>> + vrdl->size = int128_get64(section->size);
>>>> + vrdl->granularity = rdmc->get_min_granularity(rdm, section->mr);
>>>> + vrdl->dma_max = vrdl->size / vrdl->granularity;
>>>> + if (!QEMU_IS_ALIGNED(vrdl->size, vrdl->granularity) ||
>>>> + !QEMU_IS_ALIGNED(vrdl->offset_within_region, vrdl->granularity)) {
>>>> + vrdl->dma_max++;
>>>> + }
>>>> +
>>>> + /* Ignore some corner cases not relevant in practice. */
>>>> + g_assert(QEMU_IS_ALIGNED(vrdl->offset_within_region, TARGET_PAGE_SIZE));
>>>> + g_assert(QEMU_IS_ALIGNED(vrdl->offset_within_address_space,
>>>> + TARGET_PAGE_SIZE));
>>>> + g_assert(QEMU_IS_ALIGNED(vrdl->size, TARGET_PAGE_SIZE));
>>>> +
>>>> + /* We could consume quite some mappings later. */
>>>> + vfio_container_dma_reserve(container, vrdl->dma_max);
>>>
>>>
>>> Aha, I guess this is where the "reservation" aspect begins to appear.
>>> Should this be its own counter though, perhaps
>>> dma_discard_max_mappings? The populate and discard callbacks could
>>> further divide this into used and outstanding counters. However, TBH
>>> I'm not sure I understand the counters since this is probably the most
>>> robust mapping path where we can actually safely nak a populate
>>
>> I'd like to be able to warn early on fundamental setup issues, not only
>> when accidentally running into these limits later.
>>
>>> callback. Maybe rather than any of these runtime counters we should
>>> just walk the vrdl_list, calculate max mappings, and if that exceeds
>>> some large fraction of available mappings, issue a warning (not that
>>> they wouldn't be useful for tracing). Thanks,
>>
>> Sure, we can calculate max mappings from the vrdl_list. But which
>> fraction to chose? The reservation approach simply considers any
>> mappings (well, except IOMMU because they are kind of special)
>
> Right, but we're looking at the address space of a device, which should
> be exclusively system memory or an IOMMU range, right? There are IOMMUs
Yes, that's why I ignored the IOMMU case for now - RamDiscardMgr and
vIOMMUs should be mutually exclusive in an address space.
> that don't restrict the device to the IOVA window, but I'm not sure if
> we care about those. If that's true, I'm not sure we need to worry
> about the complicated intersection of RamDiscardMgr and vIOMMU both
> creating mappings.
Yes.
>
>> Guidance on the fraction / #mappings to assume we can use appreciated.
>
> Can we use the number of KVM memory slots as a guide? This is
> essentially a mechanism for sub-dividing things that exist in a KVM
> memory slot, so it seems like (dma_avail - KVM-memory-slots) should be
> greater than the # of possible granules we'd map across all the
> RamDiscardMgr regions. Maybe a good starting point?
Good idea. I stared with "maximum #DIMMs", but then thought "what about
mapping other things, like PCI bars into address space".
#max-KVM-memory-slots should be a good starting point for now.
So, when mapping a new RamDiscardMgr I'll
1. Loop over vrdl_list and calculate the max #mappings
2. Check against "dma_max - #max-KVM-memory-slots" and issue a
RamDiscardMgr specific error message
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 06/10] vfio: Support for RamDiscardMgr in the vIOMMU case
2020-12-16 14:11 [PATCH v3 00/10] virtio-mem: vfio support David Hildenbrand
` (4 preceding siblings ...)
2020-12-16 14:11 ` [PATCH v3 05/10] vfio: Support for RamDiscardMgr in the !vIOMMU case David Hildenbrand
@ 2020-12-16 14:11 ` David Hildenbrand
2020-12-16 14:11 ` [PATCH v3 07/10] softmmu/physmem: Don't use atomic operations in ram_block_discard_(disable|require) David Hildenbrand
` (3 subsequent siblings)
9 siblings, 0 replies; 21+ messages in thread
From: David Hildenbrand @ 2020-12-16 14:11 UTC (permalink / raw)
To: qemu-devel
Cc: Pankaj Gupta, Wei Yang, David Hildenbrand, Michael S. Tsirkin,
Dr . David Alan Gilbert, Peter Xu, Marek Kedzierski, Auger Eric,
Alex Williamson, teawater, Jonathan Cameron, Paolo Bonzini,
Igor Mammedov
vIOMMU support works already with RamDiscardMgr as long as guests only
map populated memory. Both, populated and discarded memory is mapped
into &address_space_memory, where vfio_get_xlat_addr() will find that
memory, to create the vfio mapping.
Sane guests will never map discarded memory (e.g., unplugged memory
blocks in virtio-mem) into an IOMMU - or keep it mapped into an IOMMU while
memory is getting discarded. However, there are two cases where a malicious
guests could trigger pinning of more memory than intended.
One case is easy to handle: the guest trying to map discarded memory
into an IOMMU.
The other case is harder to handle: the guest keeping memory mapped in
the IOMMU while it is getting discarded. We would have to walk over all
mappings when discarding memory and identify if any mapping would be a
violation. Let's keep it simple for now and print a warning, indicating
that setting RLIMIT_MEMLOCK can mitigate such attacks.
We have to take care of incoming migration: at the point the
IOMMUs get restored and start creating mappings in vfio, RamDiscardMgr
implementations might not be back up and running yet. Let's rely on the
runstate. An alternative would be using vmstate priorities - but current
handling is cleaner and more obvious.
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: Auger Eric <eric.auger@redhat.com>
Cc: Wei Yang <richard.weiyang@linux.alibaba.com>
Cc: teawater <teawaterz@linux.alibaba.com>
Cc: Marek Kedzierski <mkedzier@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/vfio/common.c | 35 +++++++++++++++++++++++++++++++++++
hw/virtio/virtio-mem.c | 1 +
include/migration/vmstate.h | 1 +
3 files changed, 37 insertions(+)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index b1582be1e8..57c83a2f14 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -36,6 +36,7 @@
#include "qemu/range.h"
#include "sysemu/kvm.h"
#include "sysemu/reset.h"
+#include "sysemu/runstate.h"
#include "trace.h"
#include "qapi/error.h"
#include "migration/migration.h"
@@ -595,6 +596,40 @@ static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
error_report("iommu map to non memory area %"HWADDR_PRIx"",
xlat);
return false;
+ } else if (memory_region_has_ram_discard_mgr(mr)) {
+ RamDiscardMgr *rdm = memory_region_get_ram_discard_mgr(mr);
+ RamDiscardMgrClass *rdmc = RAM_DISCARD_MGR_GET_CLASS(rdm);
+
+ /*
+ * Malicious VMs can map memory into the IOMMU, which is expected
+ * to remain discarded. vfio will pin all pages, populating memory.
+ * Disallow that. vmstate priorities make sure any RamDiscardMgr were
+ * already restored before IOMMUs are restored.
+ */
+ if (!rdmc->is_populated(rdm, mr, xlat, len)) {
+ error_report("iommu map to discarded memory (e.g., unplugged via"
+ " virtio-mem): %"HWADDR_PRIx"",
+ iotlb->translated_addr);
+ return false;
+ }
+
+ /*
+ * Malicious VMs might trigger discarding of IOMMU-mapped memory. The
+ * pages will remain pinned inside vfio until unmapped, resulting in a
+ * higher memory consumption than expected. If memory would get
+ * populated again later, there would be an inconsistency between pages
+ * pinned by vfio and pages seen by QEMU. This is the case until
+ * unmapped from the IOMMU (e.g., during device reset).
+ *
+ * With malicious guests, we really only care about pinning more memory
+ * than expected. RLIMIT_MEMLOCK set for the user/process can never be
+ * exceeded and can be used to mitigate this problem.
+ */
+ warn_report_once("Using vfio with vIOMMUs and coordinated discarding of"
+ " RAM (e.g., virtio-mem) works, however, malicious"
+ " guests can trigger pinning of more memory than"
+ " intended via an IOMMU. It's possible to mitigate "
+ " by setting/adjusting RLIMIT_MEMLOCK.");
}
/*
diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index 6200813bb8..f419a758f3 100644
--- a/hw/virtio/virtio-mem.c
+++ b/hw/virtio/virtio-mem.c
@@ -871,6 +871,7 @@ static const VMStateDescription vmstate_virtio_mem_device = {
.name = "virtio-mem-device",
.minimum_version_id = 1,
.version_id = 1,
+ .priority = MIG_PRI_VIRTIO_MEM,
.post_load = virtio_mem_post_load,
.fields = (VMStateField[]) {
VMSTATE_WITH_TMP(VirtIOMEM, VirtIOMEMMigSanityChecks,
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 4d71dc8fba..5b0e930144 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -153,6 +153,7 @@ typedef enum {
MIG_PRI_DEFAULT = 0,
MIG_PRI_IOMMU, /* Must happen before PCI devices */
MIG_PRI_PCI_BUS, /* Must happen before IOMMU */
+ MIG_PRI_VIRTIO_MEM, /* Must happen before IOMMU */
MIG_PRI_GICV3_ITS, /* Must happen before PCI devices */
MIG_PRI_GICV3, /* Must happen before the ITS */
MIG_PRI_MAX,
--
2.29.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v3 07/10] softmmu/physmem: Don't use atomic operations in ram_block_discard_(disable|require)
2020-12-16 14:11 [PATCH v3 00/10] virtio-mem: vfio support David Hildenbrand
` (5 preceding siblings ...)
2020-12-16 14:11 ` [PATCH v3 06/10] vfio: Support for RamDiscardMgr in the vIOMMU case David Hildenbrand
@ 2020-12-16 14:11 ` David Hildenbrand
2020-12-16 14:11 ` [PATCH v3 08/10] softmmu/physmem: Extend ram_block_discard_(require|disable) by two discard types David Hildenbrand
` (2 subsequent siblings)
9 siblings, 0 replies; 21+ messages in thread
From: David Hildenbrand @ 2020-12-16 14:11 UTC (permalink / raw)
To: qemu-devel
Cc: Marek Kedzierski, Pankaj Gupta, Wei Yang, David Hildenbrand,
Michael S. Tsirkin, Alex Williamson, Peter Xu,
Dr . David Alan Gilbert, Auger Eric, Pankaj Gupta, teawater,
Jonathan Cameron, Paolo Bonzini, Igor Mammedov
We have users in migration context that don't hold the BQL (when
finishing migration). To prepare for further changes, use a dedicated mutex
instead of atomic operations. Keep using qatomic_read ("READ_ONCE") for the
functions that only extract the current state (e.g., used by
virtio-balloon), locking isn't necessary.
While at it, split up the counter into two variables to make it easier
to understand.
Suggested-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Pankaj Gupta <pankaj.gupta@cloud.ionos.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: Auger Eric <eric.auger@redhat.com>
Cc: Wei Yang <richard.weiyang@linux.alibaba.com>
Cc: teawater <teawaterz@linux.alibaba.com>
Cc: Marek Kedzierski <mkedzier@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
softmmu/physmem.c | 70 ++++++++++++++++++++++++++---------------------
1 file changed, 39 insertions(+), 31 deletions(-)
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 2cd1de4a2c..6281ef6208 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -3660,56 +3660,64 @@ void mtree_print_dispatch(AddressSpaceDispatch *d, MemoryRegion *root)
}
}
-/*
- * If positive, discarding RAM is disabled. If negative, discarding RAM is
- * required to work and cannot be disabled.
- */
-static int ram_block_discard_disabled;
+static unsigned int ram_block_discard_requirers;
+static unsigned int ram_block_discard_disablers;
+static QemuMutex ram_block_discard_disable_mutex;
+
+static void ram_block_discard_disable_mutex_lock(void)
+{
+ static gsize initialized;
+
+ if (g_once_init_enter(&initialized)) {
+ qemu_mutex_init(&ram_block_discard_disable_mutex);
+ g_once_init_leave(&initialized, 1);
+ }
+ qemu_mutex_lock(&ram_block_discard_disable_mutex);
+}
+
+static void ram_block_discard_disable_mutex_unlock(void)
+{
+ qemu_mutex_unlock(&ram_block_discard_disable_mutex);
+}
int ram_block_discard_disable(bool state)
{
- int old;
+ int ret = 0;
+ ram_block_discard_disable_mutex_lock();
if (!state) {
- qatomic_dec(&ram_block_discard_disabled);
- return 0;
+ ram_block_discard_disablers--;
+ } else if (!ram_block_discard_requirers) {
+ ram_block_discard_disablers++;
+ } else {
+ ret = -EBUSY;
}
-
- do {
- old = qatomic_read(&ram_block_discard_disabled);
- if (old < 0) {
- return -EBUSY;
- }
- } while (qatomic_cmpxchg(&ram_block_discard_disabled,
- old, old + 1) != old);
- return 0;
+ ram_block_discard_disable_mutex_unlock();
+ return ret;
}
int ram_block_discard_require(bool state)
{
- int old;
+ int ret = 0;
+ ram_block_discard_disable_mutex_lock();
if (!state) {
- qatomic_inc(&ram_block_discard_disabled);
- return 0;
+ ram_block_discard_requirers--;
+ } else if (!ram_block_discard_disablers) {
+ ram_block_discard_requirers++;
+ } else {
+ ret = -EBUSY;
}
-
- do {
- old = qatomic_read(&ram_block_discard_disabled);
- if (old > 0) {
- return -EBUSY;
- }
- } while (qatomic_cmpxchg(&ram_block_discard_disabled,
- old, old - 1) != old);
- return 0;
+ ram_block_discard_disable_mutex_unlock();
+ return ret;
}
bool ram_block_discard_is_disabled(void)
{
- return qatomic_read(&ram_block_discard_disabled) > 0;
+ return qatomic_read(&ram_block_discard_disablers);
}
bool ram_block_discard_is_required(void)
{
- return qatomic_read(&ram_block_discard_disabled) < 0;
+ return qatomic_read(&ram_block_discard_requirers);
}
--
2.29.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v3 08/10] softmmu/physmem: Extend ram_block_discard_(require|disable) by two discard types
2020-12-16 14:11 [PATCH v3 00/10] virtio-mem: vfio support David Hildenbrand
` (6 preceding siblings ...)
2020-12-16 14:11 ` [PATCH v3 07/10] softmmu/physmem: Don't use atomic operations in ram_block_discard_(disable|require) David Hildenbrand
@ 2020-12-16 14:11 ` David Hildenbrand
2020-12-16 14:11 ` [PATCH v3 09/10] virtio-mem: Require only coordinated discards David Hildenbrand
2020-12-16 14:12 ` [PATCH v3 10/10] vfio: Disable only uncoordinated discards David Hildenbrand
9 siblings, 0 replies; 21+ messages in thread
From: David Hildenbrand @ 2020-12-16 14:11 UTC (permalink / raw)
To: qemu-devel
Cc: Marek Kedzierski, Pankaj Gupta, Wei Yang, David Hildenbrand,
Michael S. Tsirkin, Alex Williamson, Peter Xu,
Dr . David Alan Gilbert, Auger Eric, Pankaj Gupta, teawater,
Jonathan Cameron, Paolo Bonzini, Igor Mammedov
We want to separate the two cases whereby we discard ram
- uncoordinated: e.g., virito-balloon
- coordinated: e.g., virtio-mem coordinated via the RamDiscardMgr
Reviewed-by: Pankaj Gupta <pankaj.gupta@cloud.ionos.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: Auger Eric <eric.auger@redhat.com>
Cc: Wei Yang <richard.weiyang@linux.alibaba.com>
Cc: teawater <teawaterz@linux.alibaba.com>
Cc: Marek Kedzierski <mkedzier@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
include/exec/memory.h | 18 +++++++++++++--
softmmu/physmem.c | 54 ++++++++++++++++++++++++++++++++++++++-----
2 files changed, 64 insertions(+), 8 deletions(-)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 67d9f1f3a7..11832cce3c 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2808,6 +2808,12 @@ static inline MemOp devend_memop(enum device_endian end)
*/
int ram_block_discard_disable(bool state);
+/*
+ * See ram_block_discard_disable(): only disable uncoordinated discards,
+ * keeping coordinated discards (via the RamDiscardMgr) enabled.
+ */
+int ram_block_uncoordinated_discard_disable(bool state);
+
/*
* Inhibit technologies that disable discarding of pages in RAM blocks.
*
@@ -2817,12 +2823,20 @@ int ram_block_discard_disable(bool state);
int ram_block_discard_require(bool state);
/*
- * Test if discarding of memory in ram blocks is disabled.
+ * See ram_block_discard_require(): only inhibit technologies that disable
+ * uncoordinated discarding of pages in RAM blocks, allowing co-existance with
+ * technologies that only inhibit uncoordinated discards (via the
+ * RamDiscardMgr).
+ */
+int ram_block_coordinated_discard_require(bool state);
+
+/*
+ * Test if any discarding of memory in ram blocks is disabled.
*/
bool ram_block_discard_is_disabled(void);
/*
- * Test if discarding of memory in ram blocks is required to work reliably.
+ * Test if any discarding of memory in ram blocks is required to work reliably.
*/
bool ram_block_discard_is_required(void);
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 6281ef6208..3c7e6f3c53 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -3660,8 +3660,14 @@ void mtree_print_dispatch(AddressSpaceDispatch *d, MemoryRegion *root)
}
}
+/* Require any discards to work. */
static unsigned int ram_block_discard_requirers;
+/* Require only coordinated discards to work. */
+static unsigned int ram_block_coordinated_discard_requirers;
+/* Disable any discards. */
static unsigned int ram_block_discard_disablers;
+/* Disable only uncoordinated discards. */
+static unsigned int ram_block_uncoordinated_discard_disablers;
static QemuMutex ram_block_discard_disable_mutex;
static void ram_block_discard_disable_mutex_lock(void)
@@ -3687,10 +3693,27 @@ int ram_block_discard_disable(bool state)
ram_block_discard_disable_mutex_lock();
if (!state) {
ram_block_discard_disablers--;
- } else if (!ram_block_discard_requirers) {
- ram_block_discard_disablers++;
+ } else if (ram_block_discard_requirers ||
+ ram_block_coordinated_discard_requirers) {
+ ret = -EBUSY;
} else {
+ ram_block_discard_disablers++;
+ }
+ ram_block_discard_disable_mutex_unlock();
+ return ret;
+}
+
+int ram_block_uncoordinated_discard_disable(bool state)
+{
+ int ret = 0;
+
+ ram_block_discard_disable_mutex_lock();
+ if (!state) {
+ ram_block_uncoordinated_discard_disablers--;
+ } else if (ram_block_discard_requirers) {
ret = -EBUSY;
+ } else {
+ ram_block_uncoordinated_discard_disablers++;
}
ram_block_discard_disable_mutex_unlock();
return ret;
@@ -3703,10 +3726,27 @@ int ram_block_discard_require(bool state)
ram_block_discard_disable_mutex_lock();
if (!state) {
ram_block_discard_requirers--;
- } else if (!ram_block_discard_disablers) {
- ram_block_discard_requirers++;
+ } else if (ram_block_discard_disablers ||
+ ram_block_uncoordinated_discard_disablers) {
+ ret = -EBUSY;
} else {
+ ram_block_discard_requirers++;
+ }
+ ram_block_discard_disable_mutex_unlock();
+ return ret;
+}
+
+int ram_block_coordinated_discard_require(bool state)
+{
+ int ret = 0;
+
+ ram_block_discard_disable_mutex_lock();
+ if (!state) {
+ ram_block_coordinated_discard_requirers--;
+ } else if (ram_block_discard_disablers) {
ret = -EBUSY;
+ } else {
+ ram_block_coordinated_discard_requirers++;
}
ram_block_discard_disable_mutex_unlock();
return ret;
@@ -3714,10 +3754,12 @@ int ram_block_discard_require(bool state)
bool ram_block_discard_is_disabled(void)
{
- return qatomic_read(&ram_block_discard_disablers);
+ return qatomic_read(&ram_block_discard_disablers) ||
+ qatomic_read(&ram_block_uncoordinated_discard_disablers);
}
bool ram_block_discard_is_required(void)
{
- return qatomic_read(&ram_block_discard_requirers);
+ return qatomic_read(&ram_block_discard_requirers) ||
+ qatomic_read(&ram_block_coordinated_discard_requirers);
}
--
2.29.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v3 09/10] virtio-mem: Require only coordinated discards
2020-12-16 14:11 [PATCH v3 00/10] virtio-mem: vfio support David Hildenbrand
` (7 preceding siblings ...)
2020-12-16 14:11 ` [PATCH v3 08/10] softmmu/physmem: Extend ram_block_discard_(require|disable) by two discard types David Hildenbrand
@ 2020-12-16 14:11 ` David Hildenbrand
2020-12-16 14:12 ` [PATCH v3 10/10] vfio: Disable only uncoordinated discards David Hildenbrand
9 siblings, 0 replies; 21+ messages in thread
From: David Hildenbrand @ 2020-12-16 14:11 UTC (permalink / raw)
To: qemu-devel
Cc: Pankaj Gupta, Alex Williamson, Wei Yang, David Hildenbrand,
Michael S. Tsirkin, Dr . David Alan Gilbert, Peter Xu,
Marek Kedzierski, Auger Eric, Pankaj Gupta, teawater,
Jonathan Cameron, Paolo Bonzini, Igor Mammedov
We implement the RamDiscardMgr interface and only require coordinated
discarding of RAM to work.
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Pankaj Gupta <pankaj.gupta@cloud.ionos.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: Auger Eric <eric.auger@redhat.com>
Cc: Wei Yang <richard.weiyang@linux.alibaba.com>
Cc: teawater <teawaterz@linux.alibaba.com>
Cc: Marek Kedzierski <mkedzier@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/virtio/virtio-mem.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index f419a758f3..99d0712195 100644
--- a/hw/virtio/virtio-mem.c
+++ b/hw/virtio/virtio-mem.c
@@ -687,7 +687,7 @@ static void virtio_mem_device_realize(DeviceState *dev, Error **errp)
return;
}
- if (ram_block_discard_require(true)) {
+ if (ram_block_coordinated_discard_require(true)) {
error_setg(errp, "Discarding RAM is disabled");
return;
}
@@ -695,7 +695,7 @@ static void virtio_mem_device_realize(DeviceState *dev, Error **errp)
ret = ram_block_discard_range(rb, 0, qemu_ram_get_used_length(rb));
if (ret) {
error_setg_errno(errp, -ret, "Unexpected error discarding RAM");
- ram_block_discard_require(false);
+ ram_block_coordinated_discard_require(false);
return;
}
@@ -738,7 +738,7 @@ static void virtio_mem_device_unrealize(DeviceState *dev)
virtio_del_queue(vdev, 0);
virtio_cleanup(vdev);
g_free(vmem->bitmap);
- ram_block_discard_require(false);
+ ram_block_coordinated_discard_require(false);
}
static int virtio_mem_discard_range_cb(const VirtIOMEM *vmem, void *arg,
--
2.29.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v3 10/10] vfio: Disable only uncoordinated discards
2020-12-16 14:11 [PATCH v3 00/10] virtio-mem: vfio support David Hildenbrand
` (8 preceding siblings ...)
2020-12-16 14:11 ` [PATCH v3 09/10] virtio-mem: Require only coordinated discards David Hildenbrand
@ 2020-12-16 14:12 ` David Hildenbrand
9 siblings, 0 replies; 21+ messages in thread
From: David Hildenbrand @ 2020-12-16 14:12 UTC (permalink / raw)
To: qemu-devel
Cc: Marek Kedzierski, Pankaj Gupta, Wei Yang, David Hildenbrand,
Michael S. Tsirkin, Alex Williamson, Peter Xu,
Dr . David Alan Gilbert, Auger Eric, Pankaj Gupta, teawater,
Jonathan Cameron, Paolo Bonzini, Igor Mammedov
We support coordinated discarding of RAM using the RamDiscardMgr. Let's
unlock support for coordinated discards, keeping uncoordinated discards
(e.g., via virtio-balloon) disabled.
This unlocks virtio-mem + vfio. Note that vfio used via "nvme://" by the
block layer has to be implemented/unlocked separately. For now,
virtio-mem only supports x86-64 - spapr IOMMUs are not tested/affected.
Note: The block size of a virtio-mem device has to be set to sane sizes,
depending on the maximum hotplug size - to not run out of vfio mappings.
The default virtio-mem block size is usually in the range of a couple of
MBs. The maximum number of mapping is 64k, shared with other users.
Assume you want to hotplug 256GB using virtio-mem - the block size would
have to be set to at least 8 MiB (resulting in 32768 separate mappings).
Reviewed-by: Pankaj Gupta <pankaj.gupta@cloud.ionos.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: Auger Eric <eric.auger@redhat.com>
Cc: Wei Yang <richard.weiyang@linux.alibaba.com>
Cc: teawater <teawaterz@linux.alibaba.com>
Cc: Marek Kedzierski <mkedzier@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
hw/vfio/common.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 57c83a2f14..3ce5e26bab 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1974,8 +1974,10 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
* new memory, it will not yet set ram_block_discard_set_required() and
* therefore, neither stops us here or deals with the sudden memory
* consumption of inflated memory.
+ *
+ * We do support discarding of memory coordinated via the RamDiscardMgr.
*/
- ret = ram_block_discard_disable(true);
+ ret = ram_block_uncoordinated_discard_disable(true);
if (ret) {
error_setg_errno(errp, -ret, "Cannot set discarding of RAM broken");
return ret;
@@ -2155,7 +2157,7 @@ close_fd_exit:
close(fd);
put_space_exit:
- ram_block_discard_disable(false);
+ ram_block_uncoordinated_discard_disable(false);
vfio_put_address_space(space);
return ret;
@@ -2277,7 +2279,7 @@ void vfio_put_group(VFIOGroup *group)
}
if (!group->ram_block_discard_allowed) {
- ram_block_discard_disable(false);
+ ram_block_uncoordinated_discard_disable(false);
}
vfio_kvm_device_del_group(group);
vfio_disconnect_container(group);
@@ -2331,7 +2333,7 @@ int vfio_get_device(VFIOGroup *group, const char *name,
if (!group->ram_block_discard_allowed) {
group->ram_block_discard_allowed = true;
- ram_block_discard_disable(false);
+ ram_block_uncoordinated_discard_disable(false);
}
}
--
2.29.2
^ permalink raw reply related [flat|nested] 21+ messages in thread