From nobody Tue May 7 09:43:45 2024 Delivered-To: importer@patchew.org Received-SPF: pass (zoho.com: domain of gnu.org designates 208.118.235.17 as permitted sender) client-ip=208.118.235.17; envelope-from=qemu-devel-bounces+importer=patchew.org@nongnu.org; helo=lists.gnu.org; Authentication-Results: mx.zohomail.com; spf=pass (zoho.com: domain of gnu.org designates 208.118.235.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org Return-Path: Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) by mx.zohomail.com with SMTPS id 15121435484511007.8427037068072; Fri, 1 Dec 2017 07:52:28 -0800 (PST) Received: from localhost ([::1]:58230 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eKnbr-0004Y8-H4 for importer@patchew.org; Fri, 01 Dec 2017 10:52:15 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38302) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eKnSG-0004Q4-Ko for qemu-devel@nongnu.org; Fri, 01 Dec 2017 10:43:53 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eKnRc-0000XU-1e for qemu-devel@nongnu.org; Fri, 01 Dec 2017 10:42:20 -0500 Received: from mx1.redhat.com ([209.132.183.28]:38922) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1eKnRb-0000TV-1Z for qemu-devel@nongnu.org; Fri, 01 Dec 2017 10:41:39 -0500 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id ECCD161E51; Fri, 1 Dec 2017 14:23:32 +0000 (UTC) Received: from dell-r430-03.lab.eng.brq.redhat.com (dell-r430-03.lab.eng.brq.redhat.com [10.37.153.18]) by smtp.corp.redhat.com (Postfix) with ESMTP id BE67B5D9CA; Fri, 1 Dec 2017 14:23:28 +0000 (UTC) From: Igor Mammedov To: qemu-devel@nongnu.org Date: Fri, 1 Dec 2017 15:22:55 +0100 Message-Id: <1512138175-185428-1-git-send-email-imammedo@redhat.com> In-Reply-To: <20171129185026.23632-1-dgilbert@redhat.com> References: <20171129185026.23632-1-dgilbert@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Fri, 01 Dec 2017 14:23:33 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 209.132.183.28 Subject: [Qemu-devel] [RFC] vhost: check if ring mapping is still valid when building memmap X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: stefanha@gmail.com, groug@kaod.org, dgilbert@redhat.com, mst@redhat.com Errors-To: qemu-devel-bounces+importer=patchew.org@nongnu.org Sender: "Qemu-devel" X-ZohoMail: RSF_0 Z_629925259 SPT_0 Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" vhost_verify_ring_mappings() were used to verify that rings are still accessible and related memory hasn't been moved after flatview is updated. 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 =3D GPA(ring) - GPA(MemoryRegionSection) ring_hva =3D=3D 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=3Dtrue) which is overkill for the task at hand. Signed-off-by: Igor Mammedov --- 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, } } =20 -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 =3D 0; + uint64_t hva_ring_offset; + uint64_t ring_last =3D range_get_last(ring_gpa, ring_size); + uint64_t reg_last =3D range_get_last(reg_gpa, reg_size); =20 - 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 =3D part_size; - p =3D vhost_memory_map(dev, part_addr, &l, 1); - if (!p || l !=3D part_size) { - r =3D -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 !=3D part) { - r =3D -EBUSY; + + /* check that ring's MemoryRegion wasn't replaced */ + hva_ring_offset =3D ring_gpa - reg_gpa; + if (ring_hva !=3D reg_hva + hva_ring_offset) { + return -ENOMEM; } - vhost_memory_unmap(dev, p, l, 0, 0); - return r; + + return 0; } =20 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 =3D 0; @@ -338,23 +346,26 @@ static int vhost_verify_ring_mappings(struct vhost_de= v *dev, struct vhost_virtqueue *vq =3D dev->vqs + i; =20 j =3D 0; - r =3D vhost_verify_ring_part_mapping(dev, vq->desc, vq->desc_phys, - vq->desc_size, start_addr, size= ); - if (!r) { + r =3D vhost_verify_ring_part_mapping( + vq->desc, vq->desc_phys, vq->desc_size, + reg_hva, reg_gpa, reg_size); + if (r) { break; } =20 j++; - r =3D vhost_verify_ring_part_mapping(dev, vq->avail, vq->avail_phy= s, - vq->avail_size, start_addr, siz= e); - if (!r) { + r =3D vhost_verify_ring_part_mapping( + vq->desc, vq->desc_phys, vq->desc_size, + reg_hva, reg_gpa, reg_size); + if (r) { break; } =20 j++; - r =3D vhost_verify_ring_part_mapping(dev, vq->used, vq->used_phys, - vq->used_size, start_addr, size= ); - if (!r) { + r =3D 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 *sectio= n) return result; } =20 -static void vhost_begin(MemoryListener *listener) -{ - struct vhost_dev *dev =3D container_of(listener, struct vhost_dev, - memory_listener); - dev->mem_changed_end_addr =3D 0; - dev->mem_changed_start_addr =3D -1; -} - struct vhost_update_mem_tmp { struct vhost_dev *dev; uint32_t nregions; struct vhost_memory_region *regions; }; =20 + /* Called for each MRS from vhost_update_mem */ static int vhost_update_mem_cb(MemoryRegionSection *mrs, void *opaque) { struct vhost_update_mem_tmp *vtmp =3D opaque; + struct vhost_dev *dev =3D vtmp->dev; struct vhost_memory_region *cur_vmr; bool need_add =3D true; uint64_t mrs_size; @@ -416,8 +421,7 @@ static int vhost_update_mem_cb(MemoryRegionSection *mrs= , void *opaque) mrs_host =3D (uintptr_t)memory_region_get_ram_ptr(mrs->mr) + mrs->offset_within_region; =20 - 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); =20 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 *m= rs, void *opaque) cur_vmr->flags_padding =3D 0; } =20 - 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 =3D 0; - *change_end =3D 0; - - for (newi =3D 0, oldi =3D 0; newi < vtmp->nregions; newi++) { - struct vhost_memory_region *newr =3D &vtmp->regions[newi]; - hwaddr newr_last =3D 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 =3D true; - while (oldi < dev->mem->nregions) { - struct vhost_memory_region *oldr =3D &dev->mem->regions[oldi]; - hwaddr oldr_last =3D 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 =3D=3D oldr->guest_phys_addr && - newr->memory_size =3D=3D oldr->memory_size) { - /* Match in GPA and size, but it could be different - * in host address or flags - */ - whole_change =3D newr->userspace_addr !=3D oldr->userspace= _addr || - newr->flags_padding !=3D oldr->flags_paddin= g; - 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 =3D MIN(*change_start, oldr->guest_phys_addr= ); - *change_end =3D 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 overl= apped - * this one - */ - break; /* Inner old loop */ - } - /* Overlap case */ - *change_start =3D MIN(*change_start, - MIN(oldr->guest_phys_addr, - newr->guest_phys_addr)); - *change_end =3D MAX(*change_end, - MAX(oldr_last, newr_last)); - whole_change =3D 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 =3D MIN(*change_start, newr->guest_phys_addr); - *change_end =3D MAX(*change_end, newr_last); - } + if (dev->started && + vhost_verify_ring_mappings(dev, (void *)mrs_host, mrs_gpa, mrs_siz= e)) { + abort(); } =20 - return *change_start || *change_end; + return 0; } =20 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 =3D 0; - vtmp.nregions =3D 0; - vtmp.dev =3D dev; + size_t mem_size; + struct vhost_update_mem_tmp vtmp =3D {.dev =3D dev}; =20 res =3D 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; } =20 - need_update =3D 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 =3D offsetof(struct vhost_memory, regions) + - (vtmp.nregions + 1) * sizeof dev->mem->regions[0]; + mem_size =3D offsetof(struct vhost_memory, regions) + + (vtmp.nregions + 1) * sizeof dev->mem->regions[0]; =20 + if(vtmp.nregions !=3D dev->mem->nregions || + memcmp(vtmp.regions, dev->mem->regions, mem_size)) { + /* Update the main regions list from our tmp */ dev->mem =3D g_realloc(dev->mem, mem_size); dev->mem->nregions =3D vtmp.nregions; memcpy(dev->mem->regions, vtmp.regions, vtmp.nregions * sizeof dev->mem->regions[0]); used_memslots =3D vtmp.nregions; - - dev->mem_changed_start_addr =3D change_start; - dev->mem_changed_end_addr =3D change_end; } =20 out: @@ -574,8 +505,6 @@ static void vhost_commit(MemoryListener *listener) { struct vhost_dev *dev =3D container_of(listener, struct vhost_dev, memory_listener); - hwaddr start_addr =3D 0; - ram_addr_t size =3D 0; uint64_t log_size; int r; =20 @@ -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; - } =20 if (vhost_update_mem(dev)) { return; } =20 - if (dev->started) { - start_addr =3D dev->mem_changed_start_addr; - size =3D dev->mem_changed_end_addr - dev->mem_changed_start_addr += 1; - - r =3D vhost_verify_ring_mappings(dev, start_addr, size); - assert(r >=3D 0); - } - if (!dev->log_enabled) { r =3D 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 *opaq= ue, hdev->features =3D features; =20 hdev->memory_listener =3D (MemoryListener) { - .begin =3D vhost_begin, .commit =3D vhost_commit, .region_add =3D vhost_region_add, .region_del =3D vhost_region_del, @@ -1458,7 +1375,6 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODev= ice *vdev) /* should only be called after backend is connected */ assert(hdev->vhost_ops); =20 - hdev->started =3D true; hdev->vdev =3D vdev; =20 r =3D vhost_dev_set_features(hdev, hdev->log_enabled); @@ -1469,6 +1385,9 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODev= ice *vdev) if (vhost_update_mem(hdev)) { goto fail_mem; } + + hdev->started =3D true; + if (vhost_dev_has_iommu(hdev)) { memory_listener_register(&hdev->iommu_listener, vdev->dma_as); } --=20 2.7.4