Re: [PATCH v1 6/7] spapr.c: introduce spapr_core_unplug_possible()

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

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

On Thu, Jan 14, 2021 at 03:06:27PM -0300, Daniel Henrique Barboza wrote:
> Next patch is going to add more conditions to allow a CPU core
> hotunplug. Let's put it into a separated function to avoid crowding
> the body of spapr_core_unplug_request().
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

LGTM, except for naming.  I'd expect a function named *_possible() to
return a boolean, where "true" means it *is* possible, rather than a
0-or-negative-error value.

So you could either change the type and sense of the function, or
change the name to, say spapr_core_unplug_check(), which I would
expect to return an error code.

> ---
>  hw/ppc/spapr.c | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 2c403b574e..a2f01c21aa 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3706,22 +3706,35 @@ static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev)
>      qdev_unrealize(dev);
>  }
>  
> -static
> -void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev,
> -                               Error **errp)
> +static int spapr_core_unplug_possible(HotplugHandler *hotplug_dev, CPUCore *cc,
> +                                      Error **errp)
>  {
> -    SpaprMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
>      int index;
> -    SpaprDrc *drc;
> -    CPUCore *cc = CPU_CORE(dev);
>  
>      if (!spapr_find_cpu_slot(MACHINE(hotplug_dev), cc->core_id, &index)) {
>          error_setg(errp, "Unable to find CPU core with core-id: %d",
>                     cc->core_id);
> -        return;
> +        return -1;
>      }
> +
>      if (index == 0) {
>          error_setg(errp, "Boot CPU core may not be unplugged");
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +static
> +void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev,
> +                               Error **errp)
> +{
> +    ERRP_GUARD();
> +    SpaprMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
> +    SpaprDrc *drc;
> +    CPUCore *cc = CPU_CORE(dev);
> +
> +    if (spapr_core_unplug_possible(hotplug_dev, cc, errp) < 0) {
>          return;
>      }
>  

-- 
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: 833 bytes --]