Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/4] spapr: add pre_plug function for memory

[Laurent Vivier <lvivier@redhat.com>]

On 23/05/2017 17:28, Greg Kurz wrote:
> On Tue, 23 May 2017 13:18:09 +0200
> Laurent Vivier <lvivier@redhat.com> wrote:
> 
>> This allows to manage errors before the memory
>> has started to be hotplugged. We already have
>> the function for the CPU cores.
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>>  hw/ppc/spapr.c | 45 ++++++++++++++++++++++++++++++---------------
>>  1 file changed, 30 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 0980d73..0e8d8d1 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -2569,20 +2569,6 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>      uint64_t align = memory_region_get_alignment(mr);
>>      uint64_t size = memory_region_size(mr);
>>      uint64_t addr;
>> -    char *mem_dev;
>> -
>> -    if (size % SPAPR_MEMORY_BLOCK_SIZE) {
>> -        error_setg(&local_err, "Hotplugged memory size must be a multiple of "
>> -                      "%lld MB", SPAPR_MEMORY_BLOCK_SIZE/M_BYTE);
>> -        goto out;
>> -    }
>> -
>> -    mem_dev = object_property_get_str(OBJECT(dimm), PC_DIMM_MEMDEV_PROP, NULL);
>> -    if (mem_dev && !kvmppc_is_mem_backend_page_size_ok(mem_dev)) {
>> -        error_setg(&local_err, "Memory backend has bad page size. "
>> -                   "Use 'memory-backend-file' with correct mem-path.");
>> -        goto out;
>> -    }
>>  
>>      pc_dimm_memory_plug(dev, &ms->hotplug_memory, mr, align, &local_err);
>>      if (local_err) {
>> @@ -2603,6 +2589,33 @@ out:
>>      error_propagate(errp, local_err);
>>  }
>>  
>> +static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>> +                                Error **errp)
> 
> Indentation nit

ok

> 
>> +{
>> +    PCDIMMDevice *dimm = PC_DIMM(dev);
>> +    PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>> +    MemoryRegion *mr = ddc->get_memory_region(dimm);
>> +    uint64_t size = memory_region_size(mr);
>> +    Error *local_err = NULL;
>> +    char *mem_dev;
>> +
>> +    if (size % SPAPR_MEMORY_BLOCK_SIZE) {
>> +        error_setg(&local_err, "Hotplugged memory size must be a multiple of "
>> +                      "%lld MB", SPAPR_MEMORY_BLOCK_SIZE / M_BYTE);
>> +        goto out;
>> +    }
>> +
>> +    mem_dev = object_property_get_str(OBJECT(dimm), PC_DIMM_MEMDEV_PROP, NULL);
>> +    if (mem_dev && !kvmppc_is_mem_backend_page_size_ok(mem_dev)) {
>> +        error_setg(&local_err, "Memory backend has bad page size. "
>> +                   "Use 'memory-backend-file' with correct mem-path.");
>> +        goto out;
>> +    }
>> +
>> +out:
>> +    error_propagate(errp, local_err);
> 
> As recently discussed with Markus Armbruster, it isn't necessary to have a
> local Error * if you don't do anything else with it but propagate it.

Yes, you are right, it's a stupid cut'n'paste.

Thanks,
Laurent