Re: [PULL v2 39/58] Transmit vhost-user memory regions individually
On Fri, Jun 19, 2020 at 6:03 AM Peter Maydell wrote: > > > As noted in my other email, I think the best fix for this is to > have vhost_user_fill_msg_region() take an extra mmap_offset > argument to fill in the mmap_offset itself. In this callsite in > send_add_regions() we would pass in 'offset' and delete the manual > assignment to .mmap_offset. I'm not sure about the call in > send_remove_regions() but I guess if the intention is that the > payload field is not relevant then passing in '0' would be right. > Yes - for the places where the mmap_offset isn't needed 0 is right. Sounds good to me - I just sent in a patch. > thanks > -- PMM >
Re: [PULL v2 39/58] Transmit vhost-user memory regions individually
On Fri, 12 Jun 2020 at 15:52, Michael S. Tsirkin wrote:
>
> From: Raphael Norwitz
>
> With this change, when the VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS
> protocol feature has been negotiated, Qemu no longer sends the backend
> all the memory regions in a single message. Rather, when the memory
> tables are set or updated, a series of VHOST_USER_ADD_MEM_REG and
> VHOST_USER_REM_MEM_REG messages are sent to transmit the regions to map
> and/or unmap instead of sending send all the regions in one fixed size
> VHOST_USER_SET_MEM_TABLE message.
Hi; Coverity reports some issues with this change, which are
basically the same as the issue I noted in my other email.
> +static int send_remove_regions(struct vhost_dev *dev,
> + struct scrub_regions *remove_reg,
> + int nr_rem_reg, VhostUserMsg *msg,
> + bool reply_supported)
> +{
> +struct vhost_user *u = dev->opaque;
> +struct vhost_memory_region *shadow_reg;
> +int i, fd, shadow_reg_idx, ret;
> +ram_addr_t offset;
> +VhostUserMemoryRegion region_buffer;
Here region_buffer is uninitialized...
> +
> +/*
> + * The regions in remove_reg appear in the same order they do in the
> + * shadow table. Therefore we can minimize memory copies by iterating
> + * through remove_reg backwards.
> + */
> +for (i = nr_rem_reg - 1; i >= 0; i--) {
> +shadow_reg = remove_reg[i].region;
> +shadow_reg_idx = remove_reg[i].reg_idx;
> +
> +vhost_user_get_mr_data(shadow_reg->userspace_addr, , );
> +
> +if (fd > 0) {
> +msg->hdr.request = VHOST_USER_REM_MEM_REG;
> +vhost_user_fill_msg_region(_buffer, shadow_reg);
...we pass it to vhost_user_fill_msg_region(), but that function
only initializes 3 out of 4 of the struct's fields...
> +msg->payload.mem_reg.region = region_buffer;
...so here we copy the uninitialized region_buffer.mmap_offset.
(CID 1429803)
I think in this case we are genuinely going to use uninitialized
data, unlike the other two places.
> +static int send_add_regions(struct vhost_dev *dev,
> +struct scrub_regions *add_reg, int nr_add_reg,
> +VhostUserMsg *msg, uint64_t *shadow_pcb,
> +bool reply_supported, bool track_ramblocks)
> +{
> +struct vhost_user *u = dev->opaque;
> +int i, fd, ret, reg_idx, reg_fd_idx;
> +struct vhost_memory_region *reg;
> +MemoryRegion *mr;
> +ram_addr_t offset;
> +VhostUserMsg msg_reply;
> +VhostUserMemoryRegion region_buffer;
Similarly, here region_buffer is uninitialized...
> +
> +for (i = 0; i < nr_add_reg; i++) {
> +reg = add_reg[i].region;
> +reg_idx = add_reg[i].reg_idx;
> +reg_fd_idx = add_reg[i].fd_idx;
> +
> +mr = vhost_user_get_mr_data(reg->userspace_addr, , );
> +
> +if (fd > 0) {
> +if (track_ramblocks) {
> +trace_vhost_user_set_mem_table_withfd(reg_fd_idx, mr->name,
> + reg->memory_size,
> + reg->guest_phys_addr,
> + reg->userspace_addr,
> + offset);
> +u->region_rb_offset[reg_idx] = offset;
> +u->region_rb[reg_idx] = mr->ram_block;
> +}
> +msg->hdr.request = VHOST_USER_ADD_MEM_REG;
> +vhost_user_fill_msg_region(_buffer, reg);
> +msg->payload.mem_reg.region = region_buffer;
...so here we're copying across uninitialized data, which makes
Coverity unhappy (CID 1429802)...
> +msg->payload.mem_reg.region.mmap_offset = offset;
...even if in this case we end up filling the value in afterwards.
As noted in my other email, I think the best fix for this is to
have vhost_user_fill_msg_region() take an extra mmap_offset
argument to fill in the mmap_offset itself. In this callsite in
send_add_regions() we would pass in 'offset' and delete the manual
assignment to .mmap_offset. I'm not sure about the call in
send_remove_regions() but I guess if the intention is that the
payload field is not relevant then passing in '0' would be right.
thanks
-- PMM
[PULL v2 39/58] Transmit vhost-user memory regions individually
From: Raphael Norwitz
With this change, when the VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS
protocol feature has been negotiated, Qemu no longer sends the backend
all the memory regions in a single message. Rather, when the memory
tables are set or updated, a series of VHOST_USER_ADD_MEM_REG and
VHOST_USER_REM_MEM_REG messages are sent to transmit the regions to map
and/or unmap instead of sending send all the regions in one fixed size
VHOST_USER_SET_MEM_TABLE message.
The vhost_user struct maintains a shadow state of the VM’s memory
regions. When the memory tables are modified, the
vhost_user_set_mem_table() function compares the new device memory state
to the shadow state and only sends regions which need to be unmapped or
mapped in. The regions which must be unmapped are sent first, followed
by the new regions to be mapped in. After all the messages have been
sent, the shadow state is set to the current virtual device state.
Existing backends which do not support
VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS are unaffected.
Signed-off-by: Raphael Norwitz
Signed-off-by: Swapnil Ingle
Signed-off-by: Peter Turschmid
Suggested-by: Mike Cui
Message-Id: <1588533678-23450-5-git-send-email-raphael.norw...@nutanix.com>
Reviewed-by: Michael S. Tsirkin
Signed-off-by: Michael S. Tsirkin
Acked-by: Marc-André Lureau
---
hw/virtio/vhost-user.c | 516 ++--
docs/interop/vhost-user.rst | 33 ++-
2 files changed, 472 insertions(+), 77 deletions(-)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 754ad885cf..3640f017a2 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -104,6 +104,8 @@ typedef enum VhostUserRequest {
VHOST_USER_RESET_DEVICE = 34,
/* Message number 35 reserved for VHOST_USER_VRING_KICK. */
VHOST_USER_GET_MAX_MEM_SLOTS = 36,
+VHOST_USER_ADD_MEM_REG = 37,
+VHOST_USER_REM_MEM_REG = 38,
VHOST_USER_MAX
} VhostUserRequest;
@@ -128,6 +130,11 @@ typedef struct VhostUserMemory {
VhostUserMemoryRegion regions[VHOST_MEMORY_MAX_NREGIONS];
} VhostUserMemory;
+typedef struct VhostUserMemRegMsg {
+uint32_t padding;
+VhostUserMemoryRegion region;
+} VhostUserMemRegMsg;
+
typedef struct VhostUserLog {
uint64_t mmap_size;
uint64_t mmap_offset;
@@ -186,6 +193,7 @@ typedef union {
struct vhost_vring_state state;
struct vhost_vring_addr addr;
VhostUserMemory memory;
+VhostUserMemRegMsg mem_reg;
VhostUserLog log;
struct vhost_iotlb_msg iotlb;
VhostUserConfig config;
@@ -226,6 +234,16 @@ struct vhost_user {
/* True once we've entered postcopy_listen */
bool postcopy_listen;
+
+/* Our current regions */
+int num_shadow_regions;
+struct vhost_memory_region shadow_regions[VHOST_MEMORY_MAX_NREGIONS];
+};
+
+struct scrub_regions {
+struct vhost_memory_region *region;
+int reg_idx;
+int fd_idx;
};
static bool ioeventfd_enabled(void)
@@ -489,8 +507,332 @@ static int vhost_user_fill_set_mem_table_msg(struct
vhost_user *u,
return 1;
}
+static inline bool reg_equal(struct vhost_memory_region *shadow_reg,
+ struct vhost_memory_region *vdev_reg)
+{
+return shadow_reg->guest_phys_addr == vdev_reg->guest_phys_addr &&
+shadow_reg->userspace_addr == vdev_reg->userspace_addr &&
+shadow_reg->memory_size == vdev_reg->memory_size;
+}
+
+static void scrub_shadow_regions(struct vhost_dev *dev,
+ struct scrub_regions *add_reg,
+ int *nr_add_reg,
+ struct scrub_regions *rem_reg,
+ int *nr_rem_reg, uint64_t *shadow_pcb,
+ bool track_ramblocks)
+{
+struct vhost_user *u = dev->opaque;
+bool found[VHOST_MEMORY_MAX_NREGIONS] = {};
+struct vhost_memory_region *reg, *shadow_reg;
+int i, j, fd, add_idx = 0, rm_idx = 0, fd_num = 0;
+ram_addr_t offset;
+MemoryRegion *mr;
+bool matching;
+
+/*
+ * Find memory regions present in our shadow state which are not in
+ * the device's current memory state.
+ *
+ * Mark regions in both the shadow and device state as "found".
+ */
+for (i = 0; i < u->num_shadow_regions; i++) {
+shadow_reg = >shadow_regions[i];
+matching = false;
+
+for (j = 0; j < dev->mem->nregions; j++) {
+reg = >mem->regions[j];
+
+mr = vhost_user_get_mr_data(reg->userspace_addr, , );
+
+if (reg_equal(shadow_reg, reg)) {
+matching = true;
+found[j] = true;
+if (track_ramblocks) {
+/*
+ * Reset postcopy client bases, region_rb, and
+ * region_rb_offset in case regions are removed.
+ */
+if (fd > 0) {
+
