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

[Philippe Mathieu-Daudé]

On 12/9/20 6:53 PM, Philippe Mathieu-Daudé wrote:
> 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?

Anyhow, from a QOM design point of view, resetfn() is not the correct
place to set a property IMHO, so Cc'ing Eduardo.