[libvirt] [PATCH] libxl: add memory attach support

[Bob Liu]

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 at 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