Re: [Qemu-ppc] [PATCH v1 05/11] spapr: move memory hotplug size check into plug code

[David Gibson]

On Mon, Jun 11, 2018 at 02:16:49PM +0200, David Hildenbrand wrote:
> This might look like a step backwards, but it is not. get_memory_region()
> should not be called on uninititalized devices. In general, only
> properties should be access, but no "derived" satte like the memory
> region.
> 
> 1. We need duplicate error checks if memdev is actually already set.
>    realize() performs these checks, no need to duplicate.
> 2. This is bad practise as one can see when looking at the NVDIMM
>    implemetation. The call does not return sane data before the device
>    is realized. Although spapr does not use NVDIMM, conceptually it is
>    wrong.
> 
> So let's just move this call to the right place. We can then cleanup
> get_memory_region().
> 
> Signed-off-by: David Hildenbrand <address@hidden>

Acked-by: David Gibson <address@hidden>

> ---
>  hw/ppc/spapr.c | 21 ++++++---------------
>  1 file changed, 6 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f59999daac..a5f1bbd58a 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3153,6 +3153,12 @@ static void spapr_memory_plug(HotplugHandler 
> *hotplug_dev, DeviceState *dev,
>      align = memory_region_get_alignment(mr);
>      size = memory_region_size(mr);
>  
> +    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;
> +    }
> +
>      pc_dimm_memory_plug(dev, MACHINE(ms), align, &local_err);
>      if (local_err) {
>          goto out;
> @@ -3186,9 +3192,6 @@ static void spapr_memory_pre_plug(HotplugHandler 
> *hotplug_dev, DeviceState *dev,
>  {
>      const sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(hotplug_dev);
>      PCDIMMDevice *dimm = PC_DIMM(dev);
> -    PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *mr;
> -    uint64_t size;
>      char *mem_dev;
>  
>      if (!smc->dr_lmb_enabled) {
> @@ -3196,18 +3199,6 @@ static void spapr_memory_pre_plug(HotplugHandler 
> *hotplug_dev, DeviceState *dev,
>          return;
>      }
>  
> -    mr = ddc->get_memory_region(dimm, errp);
> -    if (!mr) {
> -        return;
> -    }
> -    size = memory_region_size(mr);
> -
> -    if (size % SPAPR_MEMORY_BLOCK_SIZE) {
> -        error_setg(errp, "Hotplugged memory size must be a multiple of "
> -                      "%lld MB", SPAPR_MEMORY_BLOCK_SIZE / M_BYTE);
> -        return;
> -    }
> -
>      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(errp, "Memory backend has bad page size. "

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

signature.asc
Description: PGP signature