From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42910) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gMXmS-0003dL-GH for qemu-devel@nongnu.org; Tue, 13 Nov 2018 07:26:57 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gMXmO-0000ei-Ge for qemu-devel@nongnu.org; Tue, 13 Nov 2018 07:26:56 -0500 Received: from mx1.redhat.com ([209.132.183.28]:57028) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1gMXmM-0000UV-OS for qemu-devel@nongnu.org; Tue, 13 Nov 2018 07:26:51 -0500 Date: Tue, 13 Nov 2018 13:26:14 +0100 From: Igor Mammedov Message-ID: <20181113132614.3abdaa12@redhat.com> In-Reply-To: <20181023152306.3123-8-david@redhat.com> References: <20181023152306.3123-1-david@redhat.com> <20181023152306.3123-8-david@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 7/7] memory-device: rewrite address assignment using ranges List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Hildenbrand Cc: qemu-devel@nongnu.org, Eduardo Habkost , "Michael S . Tsirkin" , Michael Roth , Markus Armbruster , "Dr . David Alan Gilbert" , David Gibson On Tue, 23 Oct 2018 17:23:06 +0200 David Hildenbrand wrote: > Let's rewrite it properly using ranges. This fixes certain overflows that > are right now possible. E.g. > > qemu-system-x86_64 -m 4G,slots=20,maxmem=40G -M pc \ > -object memory-backend-file,id=mem1,share,mem-path=/dev/zero,size=2G > -device pc-dimm,memdev=mem1,id=dimm1,addr=-0x40000000 > > Now properly reports an error instead of succeeding. s/error/error out/ > > "can't add memory device [0xffffffffc0000000:0x80000000], range overflow" > > Signed-off-by: David Hildenbrand > --- > hw/mem/memory-device.c | 53 ++++++++++++++++++++++++------------------ > 1 file changed, 30 insertions(+), 23 deletions(-) > > diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c > index 8be63c8032..2fb6fc2145 100644 > --- a/hw/mem/memory-device.c > +++ b/hw/mem/memory-device.c > @@ -100,9 +100,8 @@ static uint64_t memory_device_get_free_addr(MachineState *ms, > uint64_t align, uint64_t size, > Error **errp) > { > - uint64_t address_space_start, address_space_end; > GSList *list = NULL, *item; > - uint64_t new_addr = 0; > + Range as, new = range_empty; > > if (!ms->device_memory) { > error_setg(errp, "memory devices (e.g. for memory hotplug) are not " > @@ -115,13 +114,11 @@ static uint64_t memory_device_get_free_addr(MachineState *ms, > "enabled, please specify the maxmem option"); > return 0; > } > - address_space_start = ms->device_memory->base; > - address_space_end = address_space_start + > - memory_region_size(&ms->device_memory->mr); > - g_assert(address_space_end >= address_space_start); > + range_init_nofail(&as, ms->device_memory->base, > + memory_region_size(&ms->device_memory->mr)); > > - /* address_space_start indicates the maximum alignment we expect */ > - if (!QEMU_IS_ALIGNED(address_space_start, align)) { > + /* start of address space indicates the maximum alignment we expect */ > + if (!QEMU_IS_ALIGNED(range_lob(&as), align)) { > error_setg(errp, "the alignment (0x%" PRIx64 ") is not supported", > align); > return 0; > @@ -145,20 +142,24 @@ static uint64_t memory_device_get_free_addr(MachineState *ms, > } > > if (hint) { > - new_addr = *hint; > - if (new_addr < address_space_start) { > + if (range_init(&new, *hint, size)) { > error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" PRIx64 > - "] before 0x%" PRIx64, new_addr, size, > - address_space_start); > + "], range overflow", *hint, size); > return 0; > - } else if ((new_addr + size) > address_space_end) { > + } > + if (!range_contains_range(&as, &new)) { > error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" PRIx64 > - "] beyond 0x%" PRIx64, new_addr, size, > - address_space_end); > + "], usable range for memory devices [0x%" PRIx64 ":0x%" > + PRIx64 "]", range_lob(&new), range_size(&new), > + range_lob(&as), range_size(&as)); > return 0; > } > } else { > - new_addr = address_space_start; > + if (range_init(&new, range_lob(&as), size)) { > + error_setg(errp, "can't add memory device [0x%" PRIx64 ":0x%" PRIx64 > + "], range overflow", *hint, size); maybe replace "range overflow" with "too big" or something else more user friendly > + return 0; > + } > } > > /* find address range that will fit new memory device */ > @@ -166,30 +167,36 @@ static uint64_t memory_device_get_free_addr(MachineState *ms, > for (item = list; item; item = g_slist_next(item)) { > const MemoryDeviceState *md = item->data; > const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(OBJECT(md)); > - uint64_t md_size, md_addr; > + uint64_t next_addr; > + Range tmp; > > - md_addr = mdc->get_addr(md); > - md_size = memory_device_get_region_size(md, &error_abort); > + range_init_nofail(&tmp, mdc->get_addr(md), > + memory_device_get_region_size(md, &error_abort)); > > - if (ranges_overlap(md_addr, md_size, new_addr, size)) { > + if (range_overlaps_range(&tmp, &new)) { > if (hint) { > const DeviceState *d = DEVICE(md); > error_setg(errp, "address range conflicts with memory device" > " id='%s'", d->id ? d->id : "(unnamed)"); > goto out; > } > - new_addr = QEMU_ALIGN_UP(md_addr + md_size, align); > + > + next_addr = QEMU_ALIGN_UP(range_upb(&tmp) + 1, align); ^^^^^^^^^^^^^^^^^^^ this theoretically could overflow and already past 'as' check so it would return an invalid address without erroring out. But in practice we don't have memory device container ending right on 64bit limit, so it's not really an issue. > + if (range_init(&new, next_addr, range_size(&new))) { > + range_make_empty(&new); > + break; > + } > } > } > > - if (new_addr + size > address_space_end) { > + if (!range_contains_range(&as, &new)) { > error_setg(errp, "could not find position in guest address space for " > "memory device - memory fragmented due to alignments"); > goto out; > } > out: > g_slist_free(list); > - return new_addr; > + return range_lob(&new); > } > > MemoryDeviceInfoList *qmp_memory_device_list(void) beside minor notes patch looks good