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é <phi...@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 <gr...@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.
