On 05/27/2015 05:50 AM, Luyao Huang wrote:
> When hot-plug a memory device, we don't check if there
> is a memory device have the same address with the memory device
> we want hot-pluged. Qemu forbid use/hot-plug 2 memory device
> with same slot or the same base(qemu side this elemnt named addr).
>
> Introduce a address check when build memory device qemu command line.
>
> Signed-off-by: Luyao Huang <lhuang@xxxxxxxxxx>
> ---
> v2:
> move the check to qemuBuildMemoryDeviceStr() to check the dimm address
> when start/hot-plug a memory device.
>
> src/qemu/qemu_command.c | 77 ++++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 57 insertions(+), 20 deletions(-)
>
Perhaps a bit easier to review if this was split into two patches the
first being purely code motion and the second being fixing the
problem... That said, I don't think the refactor is necessary. I've
attached an alternative and some notes inline below...
John
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index d8ce511..0380a3b 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -4955,6 +4955,61 @@ qemuBuildMemoryDimmBackendStr(virDomainMemoryDefPtr mem,
> }
>
>
> +static int
> +qemuBuildMemoryDeviceAddr(virBuffer *buf,
> + virDomainDefPtr def,
> + virDomainMemoryDefPtr mem)
static bool
qemuCheckMemoryDimmConflict(virDomainDefPtr def,
virDomainMemoryDefPtr mem)
> +{
> + ssize_t i;
size_t usually, then keep the following checks in the caller
> +
> + if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
> + return 0;
> +
> + if (mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("only 'dimm' addresses are supported for the "
> + "pc-dimm device"));
> + return -1;
> + }
> +
> + if (mem->info.addr.dimm.slot >= def->mem.memory_slots) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("memory device slot '%u' exceeds slots count '%u'"),
> + mem->info.addr.dimm.slot, def->mem.memory_slots);
> + return -1;
> + }
> +
Thru here... Since it seems the following is your bugfix correct?
> + for (i = 0; i < def->nmems; i++) {
> + virDomainMemoryDefPtr tmp = def->mems[i];
> +
> + if (tmp == mem ||
> + tmp->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM)
> + continue;
> +
> + if (mem->info.addr.dimm.slot == tmp->info.addr.dimm.slot) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("memory device slot '%u' already been"
> + " used by other memory device"),
...is already being used by another
> + mem->info.addr.dimm.slot);
> + return -1;
return true;
> + }
> +
> + if (mem->info.addr.dimm.base != 0 && tmp->info.addr.dimm.base != 0 &&
> + mem->info.addr.dimm.base == tmp->info.addr.dimm.base) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("memory device base '0x%llx' already been"
...is already being used by another...
> + " used by other memory device"),
> + mem->info.addr.dimm.base);
> + return -1;
return true
> + }
> + }
return false;
Keep remainder in caller
> +
> + virBufferAsprintf(buf, ",slot=%d", mem->info.addr.dimm.slot);
> + virBufferAsprintf(buf, ",addr=%llu", mem->info.addr.dimm.base);
> +
> + return 0;
> +}
> +
> char *
> qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem,
> virDomainDefPtr def,
> @@ -4976,29 +5031,11 @@ qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem,
> return NULL;
> }
>
> - if (mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM &&
> - mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("only 'dimm' addresses are supported for the "
> - "pc-dimm device"));
Your refactor adjusts this test, so if the type was something else then
the virBufferAsprintf happens before the error... it's a nit, but future
adjustments could do something unexpected...
> - return NULL;
> - }
> -
> - if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM &&
> - mem->info.addr.dimm.slot >= def->mem.memory_slots) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("memory device slot '%u' exceeds slots count '%u'"),
> - mem->info.addr.dimm.slot, def->mem.memory_slots);
> - return NULL;
> - }
> -
Rather than refactoring - why not change the purpose of the called
routine...
if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM &&
qemuCheckMemoryDimmConflict(def, mem))
return NULL;
> virBufferAsprintf(&buf, "pc-dimm,node=%d,memdev=mem%s,id=%s",
> mem->targetNode, mem->info.alias, mem->info.alias);
>
> - if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) {
> - virBufferAsprintf(&buf, ",slot=%d", mem->info.addr.dimm.slot);
> - virBufferAsprintf(&buf, ",addr=%llu", mem->info.addr.dimm.base);
> - }
> + if (qemuBuildMemoryDeviceAddr(&buf, def, mem) < 0)
> + return NULL;
>
> break;
>
>
>From b0f6e2cc6fa3011fe6a4e539e074d18e6a23527d Mon Sep 17 00:00:00 2001
From: Luyao Huang <lhuang@xxxxxxxxxx>
Date: Wed, 10 Jun 2015 14:41:27 -0400
Subject: [PATCH] qemu: Add a check for slot and base dimm address conflicts
When hotplugging a memory device, there wasn't a check to determine
if there is a conflict with the address space being used by the to
be added memory device which is disallowed by qemu.
This patch adds a check to ensure the new device address doesn't
conflict with any existing device.
Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
---
src/qemu/qemu_command.c | 38 ++++++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 0a6d92f..2675782 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4952,6 +4952,40 @@ qemuBuildMemoryDimmBackendStr(virDomainMemoryDefPtr mem,
}
+static bool
+qemuCheckMemoryDimmConflict(virDomainDefPtr def,
+ virDomainMemoryDefPtr mem)
+{
+ size_t i;
+
+ for (i = 0; i < def->nmems; i++) {
+ virDomainMemoryDefPtr tmp = def->mems[i];
+
+ if (tmp == mem ||
+ tmp->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM)
+ continue;
+
+ if (mem->info.addr.dimm.slot == tmp->info.addr.dimm.slot) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("memory device slot '%u' is already being used "
+ "by another memory device"),
+ mem->info.addr.dimm.slot);
+ return true;
+ }
+
+ if (mem->info.addr.dimm.base != 0 && tmp->info.addr.dimm.base != 0 &&
+ mem->info.addr.dimm.base == tmp->info.addr.dimm.base) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("memory device base '0x%llx' is already being "
+ "used by another memory device"),
+ mem->info.addr.dimm.base);
+ return true;
+ }
+ }
+ return false;
+}
+
+
char *
qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem,
virDomainDefPtr def,
@@ -4989,6 +5023,10 @@ qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem,
return NULL;
}
+ if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM &&
+ qemuCheckMemoryDimmConflict(def, mem))
+ return NULL;
+
virBufferAsprintf(&buf, "pc-dimm,node=%d,memdev=mem%s,id=%s",
mem->targetNode, mem->info.alias, mem->info.alias);
--
2.1.0
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list