Re: [PATCH 1/6] spapr: Add an "spapr" property to sPAPR CPU core

[Philippe Mathieu-Daudé]

On 12/9/20 6:42 PM, Greg Kurz wrote:
> On Wed, 9 Dec 2020 18:34:31 +0100
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
>> On 12/9/20 6:00 PM, Greg Kurz wrote:
>>> The sPAPR CPU core device can only work with pseries machine types.
>>> This is currently checked in the realize function with a dynamic
>>> cast of qdev_get_machine(). Some other places also need to reach
>>> out to the machine using qdev_get_machine().
>>>
>>> Make this dependency explicit by introducing an "spapr" link
>>> property which officialy points to the machine. This link is
>>> set by pseries machine types only in the pre-plug handler. This
>>> allows to drop some users of qdev_get_machine().
>>>
>>> Signed-off-by: Greg Kurz <groug@kaod.org>
>>> ---
>>>  include/hw/ppc/spapr_cpu_core.h |  2 ++
>>>  hw/ppc/spapr.c                  |  4 ++++
>>>  hw/ppc/spapr_cpu_core.c         | 17 +++++++----------
>>>  3 files changed, 13 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/include/hw/ppc/spapr_cpu_core.h 
>>> b/include/hw/ppc/spapr_cpu_core.h
>>> index dab3dfc76c0a..0969b29fd96c 100644
>>> --- a/include/hw/ppc/spapr_cpu_core.h
>>> +++ b/include/hw/ppc/spapr_cpu_core.h
>>> @@ -10,6 +10,7 @@
>>>  #define HW_SPAPR_CPU_CORE_H
>>>  
>>>  #include "hw/cpu/core.h"
>>> +#include "hw/ppc/spapr.h"
>>>  #include "hw/qdev-core.h"
>>>  #include "target/ppc/cpu-qom.h"
>>>  #include "target/ppc/cpu.h"
>>> @@ -24,6 +25,7 @@ OBJECT_DECLARE_TYPE(SpaprCpuCore, SpaprCpuCoreClass,
>>>  struct SpaprCpuCore {
>>>      /*< private >*/
>>>      CPUCore parent_obj;
>>> +    SpaprMachineState *spapr;
>>>  
>>>      /*< public >*/
>>>      PowerPCCPU **threads;
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>> index d1dcf3ab2c94..4cc51723c62e 100644
>>> --- a/hw/ppc/spapr.c
>>> +++ b/hw/ppc/spapr.c
>>> @@ -3816,6 +3816,10 @@ static void spapr_core_pre_plug(HotplugHandler 
>>> *hotplug_dev, DeviceState *dev,
>>>      int index;
>>>      unsigned int smp_threads = machine->smp.threads;
>>>  
>>> +    /* Required by spapr_cpu_core_realize() */
>>> +    object_property_set_link(OBJECT(dev), "spapr", OBJECT(hotplug_dev),
>>> +                             &error_abort);
>>> +
>>>      if (dev->hotplugged && !mc->has_hotpluggable_cpus) {
>>>          error_setg(errp, "CPU hotplug not supported for this machine");
>>>          return;
>>> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
>>> index 2f7dc3c23ded..dec09367e4a0 100644
>>> --- a/hw/ppc/spapr_cpu_core.c
>>> +++ b/hw/ppc/spapr_cpu_core.c
>>> @@ -25,14 +25,13 @@
>>>  #include "sysemu/hw_accel.h"
>>>  #include "qemu/error-report.h"
>>>  
>>> -static void spapr_reset_vcpu(PowerPCCPU *cpu)
>>> +static void spapr_reset_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr)
>>>  {
>>>      CPUState *cs = CPU(cpu);
>>>      CPUPPCState *env = &cpu->env;
>>>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>>>      SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>>>      target_ulong lpcr;
>>> -    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>>>  
>>>      cpu_reset(cs);
>>>  
>>> @@ -186,7 +185,7 @@ static void spapr_unrealize_vcpu(PowerPCCPU *cpu, 
>>> SpaprCpuCore *sc)
>>>      if (!sc->pre_3_0_migration) {
>>>          vmstate_unregister(NULL, &vmstate_spapr_cpu_state, 
>>> cpu->machine_data);
>>>      }
>>> -    spapr_irq_cpu_intc_destroy(SPAPR_MACHINE(qdev_get_machine()), cpu);
>>> +    spapr_irq_cpu_intc_destroy(sc->spapr, cpu);
>>>      qdev_unrealize(DEVICE(cpu));
>>>  }
>>>  
>>> @@ -200,7 +199,7 @@ static void spapr_cpu_core_reset(DeviceState *dev)
>>>      int i;
>>>  
>>>      for (i = 0; i < cc->nr_threads; i++) {
>>> -        spapr_reset_vcpu(sc->threads[i]);
>>> +        spapr_reset_vcpu(sc->threads[i], sc->spapr);
>>
>> Why reset() needs access to the machine state, don't
>> you have it in realize()?
>>
> 
> This is for the vCPU threads of the sPAPR CPU core. They don't have the
> link to the machine state.

They are created by spapr_create_vcpu() + spapr_realize_vcpu() in
spapr_cpu_core_realize(), which has sc->spapr set... Am I missing
something?

> 
>>>      }
>>>  }
>>>  
>>> @@ -314,16 +313,12 @@ err:
>>>  
>>>  static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>>>  {
>>> -    /* We don't use SPAPR_MACHINE() in order to exit gracefully if the user
>>> -     * tries to add a sPAPR CPU core to a non-pseries machine.
>>> -     */
>>> -    SpaprMachineState *spapr =
>>> -        (SpaprMachineState *) object_dynamic_cast(qdev_get_machine(),
>>> -                                                  TYPE_SPAPR_MACHINE);
>>>      SpaprCpuCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
>>> +    SpaprMachineState *spapr = sc->spapr;
>>>      CPUCore *cc = CPU_CORE(OBJECT(dev));
>>>      int i;
>>>  
>>> +    /* Set in spapr_core_pre_plug() */
>>>      if (!spapr) {
>>>          error_setg(errp, TYPE_SPAPR_CPU_CORE " needs a pseries machine");
>>>          return;
>>> @@ -345,6 +340,8 @@ static Property spapr_cpu_core_properties[] = {
>>>      DEFINE_PROP_INT32("node-id", SpaprCpuCore, node_id, 
>>> CPU_UNSET_NUMA_NODE_ID),
>>>      DEFINE_PROP_BOOL("pre-3.0-migration", SpaprCpuCore, pre_3_0_migration,
>>>                       false),
>>> +    DEFINE_PROP_LINK("spapr", SpaprCpuCore, spapr, TYPE_SPAPR_MACHINE,
>>> +                     SpaprMachineState *),
>>>      DEFINE_PROP_END_OF_LIST()
>>>  };
>>>  
>>>
>>
>