* Igor Mammedov (imamm...@redhat.com) wrote:
> vhost_verify_ring_mappings() were used to verify that
> rings are still accessible and related memory hasn't
> been moved after flatview is updated.
I think I've rolled the equivalent into my v2 I've just
posted; please have a look.
Dave
> It were doing checks by mapping ring's GPA+len and
> checking that HVA hasn't changed with new memory map.
> To avoid maybe expensive mapping call, we were
> identifying address range that changed and were doing
> mapping only if ring were in changed range.
>
> However it's no neccessary to perform ringi's GPA
> mapping as we already have it's current HVA and all
> we need is to verify that ring's GPA translates to
> the same HVA in updated flatview.
>
> That could be done during time when we are building
> vhost memmap where vhost_update_mem_cb() already maps
> every RAM MemoryRegionSection to get section HVA. This
> fact can be used to check that ring belongs to the same
> MemoryRegion in the same place, like this:
>
> hva_ring_offset = GPA(ring) - GPA(MemoryRegionSection)
> ring_hva == HVA(MemoryRegionSection) + hva_ring_offset
>
> Doing this would allow us to drop ~100LOC of code which
> figures out changed memory range and avoid calling not
> needed reverse vhost_memory_map(is_write=true) which is
> overkill for the task at hand.
>
> Signed-off-by: Igor Mammedov <imamm...@redhat.com>
> ---
> PS:
> should be applied on top of David's series
>
> ---
> include/hw/virtio/vhost.h | 2 -
> hw/virtio/vhost.c | 195
> ++++++++++++++--------------------------------
> 2 files changed, 57 insertions(+), 140 deletions(-)
>
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index 467dc77..fc4af5c 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -68,8 +68,6 @@ struct vhost_dev {
> uint64_t log_size;
> Error *migration_blocker;
> bool memory_changed;
> - hwaddr mem_changed_start_addr;
> - hwaddr mem_changed_end_addr;
> const VhostOps *vhost_ops;
> void *opaque;
> struct vhost_log *log;
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 5b9a7d7..026bac3 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -296,35 +296,43 @@ static void vhost_memory_unmap(struct vhost_dev *dev,
> void *buffer,
> }
> }
>
> -static int vhost_verify_ring_part_mapping(struct vhost_dev *dev,
> - void *part,
> - uint64_t part_addr,
> - uint64_t part_size,
> - uint64_t start_addr,
> - uint64_t size)
> +static int vhost_verify_ring_part_mapping(void *ring_hva,
> + uint64_t ring_gpa,
> + uint64_t ring_size,
> + void *reg_hva,
> + uint64_t reg_gpa,
> + uint64_t reg_size)
> {
> - hwaddr l;
> - void *p;
> - int r = 0;
> + uint64_t hva_ring_offset;
> + uint64_t ring_last = range_get_last(ring_gpa, ring_size);
> + uint64_t reg_last = range_get_last(reg_gpa, reg_size);
>
> - if (!ranges_overlap(start_addr, size, part_addr, part_size)) {
> + /* start check from the end so that the rest of checks
> + * are done when whole ring is in merged sections range
> + */
> + if (ring_last < reg_last || ring_gpa > reg_last) {
> return 0;
> }
> - l = part_size;
> - p = vhost_memory_map(dev, part_addr, &l, 1);
> - if (!p || l != part_size) {
> - r = -ENOMEM;
> +
> + /* check that whole ring's is mapped */
> + if (range_get_last(ring_gpa, ring_size) >
> + range_get_last(reg_gpa, reg_size)) {
> + return -EBUSY;
> }
> - if (p != part) {
> - r = -EBUSY;
> +
> + /* check that ring's MemoryRegion wasn't replaced */
> + hva_ring_offset = ring_gpa - reg_gpa;
> + if (ring_hva != reg_hva + hva_ring_offset) {
> + return -ENOMEM;
> }
> - vhost_memory_unmap(dev, p, l, 0, 0);
> - return r;
> +
> + return 0;
> }
>
> static int vhost_verify_ring_mappings(struct vhost_dev *dev,
> - uint64_t start_addr,
> - uint64_t size)
> + void *reg_hva,
> + uint64_t reg_gpa,
> + uint64_t reg_size)
> {
> int i, j;
> int r = 0;
> @@ -338,23 +346,26 @@ static int vhost_verify_ring_mappings(struct vhost_dev
> *dev,
> struct vhost_virtqueue *vq = dev->vqs + i;
>
> j = 0;
> - r = vhost_verify_ring_part_mapping(dev, vq->desc, vq->desc_phys,
> - vq->desc_size, start_addr, size);
> - if (!r) {
> + r = vhost_verify_ring_part_mapping(
> + vq->desc, vq->desc_phys, vq->desc_size,
> + reg_hva, reg_gpa, reg_size);
> + if (r) {
> break;
> }
>
> j++;
> - r = vhost_verify_ring_part_mapping(dev, vq->avail, vq->avail_phys,
> - vq->avail_size, start_addr, size);
> - if (!r) {
> + r = vhost_verify_ring_part_mapping(
> + vq->desc, vq->desc_phys, vq->desc_size,
> + reg_hva, reg_gpa, reg_size);
> + if (r) {
> break;
> }
>
> j++;
> - r = vhost_verify_ring_part_mapping(dev, vq->used, vq->used_phys,
> - vq->used_size, start_addr, size);
> - if (!r) {
> + r = vhost_verify_ring_part_mapping(
> + vq->desc, vq->desc_phys, vq->desc_size,
> + reg_hva, reg_gpa, reg_size);
> + if (r) {
> break;
> }
> }
> @@ -384,24 +395,18 @@ static bool vhost_section(MemoryRegionSection *section)
> return result;
> }
>
> -static void vhost_begin(MemoryListener *listener)
> -{
> - struct vhost_dev *dev = container_of(listener, struct vhost_dev,
> - memory_listener);
> - dev->mem_changed_end_addr = 0;
> - dev->mem_changed_start_addr = -1;
> -}
> -
> struct vhost_update_mem_tmp {
> struct vhost_dev *dev;
> uint32_t nregions;
> struct vhost_memory_region *regions;
> };
>
> +
> /* Called for each MRS from vhost_update_mem */
> static int vhost_update_mem_cb(MemoryRegionSection *mrs, void *opaque)
> {
> struct vhost_update_mem_tmp *vtmp = opaque;
> + struct vhost_dev *dev = vtmp->dev;
> struct vhost_memory_region *cur_vmr;
> bool need_add = true;
> uint64_t mrs_size;
> @@ -416,8 +421,7 @@ static int vhost_update_mem_cb(MemoryRegionSection *mrs,
> void *opaque)
> mrs_host = (uintptr_t)memory_region_get_ram_ptr(mrs->mr) +
> mrs->offset_within_region;
>
> - trace_vhost_update_mem_cb(mrs->mr->name, mrs_gpa, mrs_size,
> - (uint64_t)mrs_host);
> + trace_vhost_update_mem_cb(mrs->mr->name, mrs_gpa, mrs_size, mrs_host);
>
> if (vtmp->nregions) {
> /* Since we already have at least one region, lets see if
> @@ -459,85 +463,19 @@ static int vhost_update_mem_cb(MemoryRegionSection
> *mrs, void *opaque)
> cur_vmr->flags_padding = 0;
> }
>
> - return 0;
> -}
> -
> -static bool vhost_update_compare_list(struct vhost_dev *dev,
> - struct vhost_update_mem_tmp *vtmp,
> - hwaddr *change_start, hwaddr
> *change_end)
> -{
> - uint32_t oldi, newi;
> - *change_start = 0;
> - *change_end = 0;
> -
> - for (newi = 0, oldi = 0; newi < vtmp->nregions; newi++) {
> - struct vhost_memory_region *newr = &vtmp->regions[newi];
> - hwaddr newr_last = range_get_last(newr->guest_phys_addr,
> - newr->memory_size);
> - trace_vhost_update_compare_list_loopn(newi, oldi,
> - newr->guest_phys_addr,
> - newr->memory_size);
> - bool whole_change = true;
> - while (oldi < dev->mem->nregions) {
> - struct vhost_memory_region *oldr = &dev->mem->regions[oldi];
> - hwaddr oldr_last = range_get_last(oldr->guest_phys_addr,
> - oldr->memory_size);
> - trace_vhost_update_compare_list_loopo(newi, oldi,
> - oldr->guest_phys_addr,
> - oldr->memory_size);
> - if (newr->guest_phys_addr == oldr->guest_phys_addr &&
> - newr->memory_size == oldr->memory_size) {
> - /* Match in GPA and size, but it could be different
> - * in host address or flags
> - */
> - whole_change = newr->userspace_addr != oldr->userspace_addr
> ||
> - newr->flags_padding != oldr->flags_padding;
> - oldi++;
> - break; /* inner old loop */
> - }
> - /* There's a difference - need to figure out what */
> - if (oldr_last < newr->guest_phys_addr) {
> - /* There used to be a region before us that's gone */
> - *change_start = MIN(*change_start, oldr->guest_phys_addr);
> - *change_end = MAX(*change_end, oldr_last);
> - oldi++;
> - continue; /* inner old loop */
> - }
> - if (oldr->guest_phys_addr > newr_last) {
> - /* We've passed all the old mappings that could have
> overlapped
> - * this one
> - */
> - break; /* Inner old loop */
> - }
> - /* Overlap case */
> - *change_start = MIN(*change_start,
> - MIN(oldr->guest_phys_addr,
> - newr->guest_phys_addr));
> - *change_end = MAX(*change_end,
> - MAX(oldr_last, newr_last));
> - whole_change = false;
> - /* There might be more old mappings that overlap */
> - oldi++;
> - }
> - if (whole_change) {
> - /* No old region to compare against, this must be a change */
> - *change_start = MIN(*change_start, newr->guest_phys_addr);
> - *change_end = MAX(*change_end, newr_last);
> - }
> + if (dev->started &&
> + vhost_verify_ring_mappings(dev, (void *)mrs_host, mrs_gpa,
> mrs_size)) {
> + abort();
> }
>
> - return *change_start || *change_end;
> + return 0;
> }
>
> static int vhost_update_mem(struct vhost_dev *dev)
> {
> int res;
> - struct vhost_update_mem_tmp vtmp;
> - hwaddr change_start, change_end;
> - bool need_update;
> - vtmp.regions = 0;
> - vtmp.nregions = 0;
> - vtmp.dev = dev;
> + size_t mem_size;
> + struct vhost_update_mem_tmp vtmp = {.dev = dev};
>
> res = address_space_iterate(&address_space_memory,
> vhost_update_mem_cb, &vtmp);
> @@ -545,24 +483,17 @@ static int vhost_update_mem(struct vhost_dev *dev)
> goto out;
> }
>
> - need_update = vhost_update_compare_list(dev, &vtmp,
> - &change_start, &change_end);
> - trace_vhost_update_mem_comparison(need_update,
> - (uint64_t)change_start,
> - (uint64_t)change_end);
> - if (need_update) {
> - /* Update the main regions list from our tmp */
> - size_t mem_size = offsetof(struct vhost_memory, regions) +
> - (vtmp.nregions + 1) * sizeof dev->mem->regions[0];
> + mem_size = offsetof(struct vhost_memory, regions) +
> + (vtmp.nregions + 1) * sizeof dev->mem->regions[0];
>
> + if(vtmp.nregions != dev->mem->nregions ||
> + memcmp(vtmp.regions, dev->mem->regions, mem_size)) {
> + /* Update the main regions list from our tmp */
> dev->mem = g_realloc(dev->mem, mem_size);
> dev->mem->nregions = vtmp.nregions;
> memcpy(dev->mem->regions, vtmp.regions,
> vtmp.nregions * sizeof dev->mem->regions[0]);
> used_memslots = vtmp.nregions;
> -
> - dev->mem_changed_start_addr = change_start;
> - dev->mem_changed_end_addr = change_end;
> }
>
> out:
> @@ -574,8 +505,6 @@ static void vhost_commit(MemoryListener *listener)
> {
> struct vhost_dev *dev = container_of(listener, struct vhost_dev,
> memory_listener);
> - hwaddr start_addr = 0;
> - ram_addr_t size = 0;
> uint64_t log_size;
> int r;
>
> @@ -585,22 +514,11 @@ static void vhost_commit(MemoryListener *listener)
> if (!dev->started) {
> return;
> }
> - if (dev->mem_changed_start_addr > dev->mem_changed_end_addr) {
> - return;
> - }
>
> if (vhost_update_mem(dev)) {
> return;
> }
>
> - if (dev->started) {
> - start_addr = dev->mem_changed_start_addr;
> - size = dev->mem_changed_end_addr - dev->mem_changed_start_addr + 1;
> -
> - r = vhost_verify_ring_mappings(dev, start_addr, size);
> - assert(r >= 0);
> - }
> -
> if (!dev->log_enabled) {
> r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem);
> if (r < 0) {
> @@ -1235,7 +1153,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> hdev->features = features;
>
> hdev->memory_listener = (MemoryListener) {
> - .begin = vhost_begin,
> .commit = vhost_commit,
> .region_add = vhost_region_add,
> .region_del = vhost_region_del,
> @@ -1458,7 +1375,6 @@ int vhost_dev_start(struct vhost_dev *hdev,
> VirtIODevice *vdev)
> /* should only be called after backend is connected */
> assert(hdev->vhost_ops);
>
> - hdev->started = true;
> hdev->vdev = vdev;
>
> r = vhost_dev_set_features(hdev, hdev->log_enabled);
> @@ -1469,6 +1385,9 @@ int vhost_dev_start(struct vhost_dev *hdev,
> VirtIODevice *vdev)
> if (vhost_update_mem(hdev)) {
> goto fail_mem;
> }
> +
> + hdev->started = true;
> +
> if (vhost_dev_has_iommu(hdev)) {
> memory_listener_register(&hdev->iommu_listener, vdev->dma_as);
> }
> --
> 2.7.4
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
