From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41309) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Whyd2-0001cf-0b for qemu-devel@nongnu.org; Wed, 07 May 2014 05:59:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Whycx-0003dF-Ax for qemu-devel@nongnu.org; Wed, 07 May 2014 05:59:07 -0400 Received: from [59.151.112.132] (port=48294 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Whycw-0003XQ-HH for qemu-devel@nongnu.org; Wed, 07 May 2014 05:59:03 -0400 Message-ID: <536A03DC.2070305@cn.fujitsu.com> Date: Wed, 7 May 2014 17:58:52 +0800 From: Tang Chen MIME-Version: 1.0 References: <1396618620-27823-1-git-send-email-imammedo@redhat.com> <1396618620-27823-18-git-send-email-imammedo@redhat.com> In-Reply-To: <1396618620-27823-18-git-send-email-imammedo@redhat.com> Content-Type: multipart/mixed; boundary="------------050003000007090209090601" Subject: Re: [Qemu-devel] [PATCH 17/35] dimm: add busy address check and address auto-allocation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: peter.maydell@linaro.org, mst@redhat.com, aik@ozlabs.ru, Hu Tao , mjt@tls.msk.ru, qemu-devel@nongnu.org, lcapitulino@redhat.com, kraxel@redhat.com, akong@redhat.com, quintela@redhat.com, armbru@redhat.com, Yasuaki Ishimatsu , aliguori@amazon.com, jan.kiszka@siemens.com, lersek@redhat.com, ehabkost@redhat.com, marcel.a@redhat.com, stefanha@redhat.com, chegu_vinod@hp.com, rth@twiddle.net, kwolf@redhat.com, s.priebe@profihost.ag, mreitz@redhat.com, vasilis.liaskovitis@profitbricks.com, "guz.fnst@cn.fujitsu.com" , pbonzini@redhat.com, afaerber@suse.de --------------050003000007090209090601 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Hi Igor, On 04/04/2014 09:36 PM, Igor Mammedov wrote: ...... > +uint64_t dimm_get_free_addr(uint64_t address_space_start, > + uint64_t address_space_size, > + uint64_t *hint, uint64_t size, > + Error **errp) > +{ > + GSList *list = NULL, *item; > + uint64_t new_start, ret; > + uint64_t address_space_end = address_space_start + address_space_size; > + > + object_child_foreach(qdev_get_machine(), dimm_built_list,&list); > + > + if (hint) { > + new_start = *hint; > + } else { > + new_start = address_space_start; > + } > + > + /* find address range that will fit new DIMM */ > + for (item = list; item; item = g_slist_next(item)) { > + DimmDevice *dimm = item->data; > + uint64_t dimm_size = object_property_get_int(OBJECT(dimm), > + "size",&error_abort); > + if (ranges_overlap(dimm->start, dimm_size, new_start, size)) { > + if (hint) { > + DeviceState *d = DEVICE(dimm); > + error_setg(errp, "address range conflicts with '%s'", d->id); > + break; Actually we got an error here. If we break from here, new_start is set to *hint, and ret will also be set to *hint. So the return value is a little ambiguous. I can understand that caller will check errp to find out if there is an error. And I also understand we need to free list. But please also see below. > + } > + new_start = dimm->start + dimm_size; > + } > + } > + ret = new_start; > + > + g_slist_free(list); > + > + if (new_start< address_space_start) { > + error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64 > + "] at 0x%" PRIx64, new_start, size, address_space_start); > + } else if ((new_start + size)> address_space_end) { > + error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64 > + "] beyond 0x%" PRIx64, new_start, size, address_space_end); > + } The error here could also be triggered. If so, errp will be reset, not the error status set above. > + return ret; > +} > So, how about the attached patch ? Thanks. :) --------------050003000007090209090601 Content-Type: text/x-patch; name="0001-dimm-add-busy-address-check-and-address-auto-allocat.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="0001-dimm-add-busy-address-check-and-address-auto-allocat.pa"; filename*1="tch" >>From 2af81231d7a5479f70b1444589b01a47d37145c2 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Fri, 4 Apr 2014 15:36:42 +0200 Subject: [PATCH] dimm: add busy address check and address auto-allocation - if 'start' property is not specified on -device/device_add command, treat default value as request for assigning DimmDevice to the first free memory region. - if 'start' is provided with -device/device_add command, attempt to use it or fail command if it's already occupied or falls inside of an existing DimmDevice memory region. Signed-off-by: Igor Mammedov Signed-off-by: Tang Chen --- hw/i386/pc.c | 16 +++++++++++- hw/mem/dimm.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++ include/hw/mem/dimm.h | 5 ++++ 3 files changed, 90 insertions(+), 1 deletion(-) diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 4038e2c..29382ec 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1483,15 +1483,29 @@ void qemu_register_pc_machine(QEMUMachine *m) static void pc_dimm_plug(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { + Error *local_err = NULL; PCMachineState *pcms = PC_MACHINE(hotplug_dev); DimmDevice *dimm = DIMM(dev); DimmDeviceClass *ddc = DIMM_GET_CLASS(dimm); MemoryRegion *mr = ddc->get_memory_region(dimm); + ram_addr_t addr = dimm->start; + + addr = dimm_get_free_addr(pcms->hotplug_memory_base, + memory_region_size(&pcms->hotplug_memory), + !addr ? NULL : &addr, + memory_region_size(mr), &local_err); + if (local_err) { + goto out; + } + object_property_set_int(OBJECT(dev), addr, "start", &local_err); memory_region_add_subregion(&pcms->hotplug_memory, - dimm->start - pcms->hotplug_memory_base, + addr - pcms->hotplug_memory_base, mr); vmstate_register_ram(mr, dev); + +out: + error_propagate(errp, local_err); } static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev, diff --git a/hw/mem/dimm.c b/hw/mem/dimm.c index 65d4cd0..d8a3c5e 100644 --- a/hw/mem/dimm.c +++ b/hw/mem/dimm.c @@ -21,6 +21,76 @@ #include "hw/mem/dimm.h" #include "qemu/config-file.h" #include "qapi/visitor.h" +#include "qemu/range.h" + +static gint dimm_addr_sort(gconstpointer a, gconstpointer b) +{ + DimmDevice *x = DIMM(a); + DimmDevice *y = DIMM(b); + + return x->start - y->start; +} + +static int dimm_built_list(Object *obj, void *opaque) +{ + GSList **list = opaque; + + if (object_dynamic_cast(obj, TYPE_DIMM)) { + DeviceState *dev = DEVICE(obj); + if (dev->realized) { /* only realized DIMMs matter */ + *list = g_slist_insert_sorted(*list, dev, dimm_addr_sort); + } + } + + object_child_foreach(obj, dimm_built_list, opaque); + return 0; +} + +uint64_t dimm_get_free_addr(uint64_t address_space_start, + uint64_t address_space_size, + uint64_t *hint, uint64_t size, + Error **errp) +{ + GSList *list = NULL, *item; + uint64_t new_start, ret = 0; + uint64_t address_space_end = address_space_start + address_space_size; + + object_child_foreach(qdev_get_machine(), dimm_built_list, &list); + + if (hint) { + new_start = *hint; + } else { + new_start = address_space_start; + } + + /* find address range that will fit new DIMM */ + for (item = list; item; item = g_slist_next(item)) { + DimmDevice *dimm = item->data; + uint64_t dimm_size = object_property_get_int(OBJECT(dimm), + "size", &error_abort); + if (ranges_overlap(dimm->start, dimm_size, new_start, size)) { + if (hint) { + DeviceState *d = DEVICE(dimm); + error_setg(errp, "address range conflicts with '%s'", d->id); + goto out; + } + new_start = dimm->start + dimm_size; + } + } + ret = new_start; + + if (new_start < address_space_start) { + error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64 + "] at 0x%" PRIx64, new_start, size, address_space_start); + } else if ((new_start + size) > address_space_end) { + error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64 + "] beyond 0x%" PRIx64, new_start, size, address_space_end); + } + +out: + g_slist_free(list); + return ret; +} static Property dimm_properties[] = { DEFINE_PROP_UINT64("start", DimmDevice, start, 0), diff --git a/include/hw/mem/dimm.h b/include/hw/mem/dimm.h index 4bbf0ed..d6e4d65 100644 --- a/include/hw/mem/dimm.h +++ b/include/hw/mem/dimm.h @@ -62,4 +62,9 @@ typedef struct DimmDeviceClass { MemoryRegion *(*get_memory_region)(DimmDevice *dimm); } DimmDeviceClass; + +uint64_t dimm_get_free_addr(uint64_t address_space_start, + uint64_t address_space_size, + uint64_t *hint, uint64_t size, + Error **errp); #endif -- 1.8.0 --------------050003000007090209090601--