qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 17/35] dimm: add busy address check and address


From: Tang Chen
Subject: Re: [Qemu-devel] [PATCH 17/35] dimm: add busy address check and address auto-allocation
Date: Wed, 7 May 2014 17:58:52 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:12.0) Gecko/20120430 Thunderbird/12.0.1

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. :)

Attachment: 0001-dimm-add-busy-address-check-and-address-auto-allocat.patch
Description: Text Data


reply via email to

[Prev in Thread] Current Thread [Next in Thread]