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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3497 bytes --]

On Tue, May 23, 2017 at 06:09:00PM +0200, Laurent Vivier wrote:
> 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.

This patch seems like a good idea regardless of the rest, so I've
fixed the minor nits Greg pointed out and merged to ppc-for-2.10.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]