Re: [Qemu-devel] [PATCH v2 3/6] spapr: move memory hotplug support check into spapr_memory_pre_plug()

[Greg Kurz]

On Fri,  8 Jun 2018 14:48:13 +0200
David Hildenbrand <address@hidden> wrote:

> Let's finish cleaning up the hotplug handler. This check can be
> performed in the pre_plug code as the very first thing.
> 
> Signed-off-by: David Hildenbrand <address@hidden>
> ---
>  hw/ppc/spapr.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 1f577b274b..9b8b4068b1 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3184,12 +3184,18 @@ out:
>  static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState 
> *dev,
>                                    Error **errp)
>  {
> +    const sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(hotplug_dev);

Even if const is valid here, it doesn't seem to be widely used in similar
situations:

$ git grep 'Class.* = .*GET_CLASS' | grep const | wc -l
14
$ git grep 'Class.* = .*GET_CLASS' | grep -v const | wc -l
642

>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);

... like here for example. This inconsistency is a bit weird.

The patch is good anyway so:

Reviewed-by: Greg Kurz <address@hidden>

>      MemoryRegion *mr;
>      uint64_t size;
>      char *mem_dev;
>  
> +    if (!smc->dr_lmb_enabled) {
> +        error_setg(errp, "Memory hotplug not supported for this machine");
> +        return;
> +    }
> +
>      mr = ddc->get_memory_region(dimm, errp);
>      if (!mr) {
>          return;
> @@ -3571,14 +3577,7 @@ out:
>  static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
>                                        DeviceState *dev, Error **errp)
>  {
> -    MachineState *ms = MACHINE(hotplug_dev);
> -    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
> -
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -        if (!smc->dr_lmb_enabled) {
> -            error_setg(errp, "Memory hotplug not supported for this 
> machine");
> -            return;
> -        }
>          spapr_memory_plug(hotplug_dev, dev, errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
>          spapr_core_plug(hotplug_dev, dev, errp);