Re: [libvirt] [PATCH] libxl: add memory attach support
On 08/30/2016 11:20 PM, Joao Martins wrote:
Hey!
On 08/30/2016 11:00 AM, Bob Liu wrote:
> Support for VIR_DOMAIN_DEVICE_MEMORY on domainAttachDeviceFlags API in libxl
> driver, using libxl_set_memory_target in xen libxl.
>
> With "virsh attach-device" command we can then hotplug memory to a domain:
> <memory model='dimm'>
> <target>
> <size unit='MiB'>128</size>
> <node>0</node>
> </target>
> </memory>
>
> $ virsh attach-device domain_name this.xml --live
>
> Signed-off-by: Bob Liu <bob.liu(a)oracle.com>
See few very minor comments below, but overall LGTM.
> ---
> src/libxl/libxl_domain.c | 1 +
> src/libxl/libxl_driver.c | 29 +++++++++++++++++++++++++++++
> 2 files changed, 30 insertions(+)
>
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index f529a2e..3924ba0 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -414,6 +414,7 @@ virDomainDefParserConfig libxlDomainDefParserConfig = {
> .macPrefix = { 0x00, 0x16, 0x3e },
> .devicesPostParseCallback = libxlDomainDeviceDefPostParse,
> .domainPostParseCallback = libxlDomainDefPostParse,
> + .features = VIR_DOMAIN_DEF_FEATURE_MEMORY_HOTPLUG
> };
>
>
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index a34eb02..541ea3b 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -3085,6 +3085,30 @@ libxlDomainAttachHostUSBDevice(libxlDriverPrivatePtr driver,
> #endif
>
> static int
> +libxlDomainAttachMemory(libxlDriverPrivatePtr driver,
> + virDomainObjPtr vm,
> + virDomainMemoryDefPtr mem)
> +{
> + int res = 0;
I think you don't need to initialize the variable.
> + libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
> +
> + if (mem->targetNode != 0) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("unsupport non-zero target node for memory
device"));
Probably changing the message to "non-zero target node not supported" instead?
The
messages sounds like you are removing support for it. But english is not my
mothertongue so may be it's just my wrong reading :)
Should we fail with other error as this patch, or VIR_WARN the user and ignore
Agree to use VIR_WARN(), will be updated.
targetNode. AFAIK libxl doesn't yet support ballooning for a
specific node. What do
others think?
> + return -1;
> + }
> +
> + res = libxl_set_memory_target(cfg->ctx, vm->def->id, mem->size, 1,
1);
Should we unlock virDomainObj while ballooning, as similarly done in
libxlDomainSetMemory* ?
Yes, that's necessary.
> + if (res < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to attach %lluKB memory for domain %d"),
> + mem->size, vm->def->id);
> + return -1;
> + }
> + return 0;
> +}
> +
> +static int
> libxlDomainAttachHostDevice(libxlDriverPrivatePtr driver,
> virDomainObjPtr vm,
> virDomainHostdevDefPtr hostdev)
> @@ -3284,6 +3308,11 @@ libxlDomainAttachDeviceLive(libxlDriverPrivatePtr driver,
> dev->data.hostdev = NULL;
> break;
>
> + case VIR_DOMAIN_DEVICE_MEMORY:
> + ret = libxlDomainAttachMemory(driver, vm, dev->data.memory);
> + dev->data.memory = NULL;
This should probably be:
ret = libxlDomainAttachMemory(driver, vm, dev->data.memory);
if (!ret)
dev->data.memory = NULL;
Right?
This code was from qemu:
7408 /* note that qemuDomainAttachMemory always consumes dev->data.memory
7409 * and dispatches DeviceAdded event on success */
7410 ret = qemuDomainAttachMemory(driver, vm,
7411 dev->data.memory);
7412 dev->data.memory = NULL;
But I also forgot to free mem in libxlDomainAttachMemory(), thank you for the reminding.
> + break;
> +
> default:
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("device type '%s' cannot be
attached"),
>
--
Regards,
-Bob