| Message ID | 20201209170052.1431440-2-groug@kaod.org |
|---|---|
| State | New |
| Headers | show |
| Series | spapr: Drop some users of qdev_get_machine() | expand |
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()? > } > } > > @@ -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() > }; > >
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. > > } > > } > > > > @@ -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() > > }; > > > > >
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() >>> }; >>> >>> >> >
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.
On Wed, Dec 09, 2020 at 07:11:40PM +0100, Philippe Mathieu-Daudé wrote: [...] > >>>> @@ -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. This patch is not setting the property on resetfn(), it is setting it on CPU core pre_plug(). This is more complex than simply using qdev_get_machine() and I don't see why it would be better, but I don't think it's wrong.
On Wed, 9 Dec 2020 13:26:17 -0500 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Wed, Dec 09, 2020 at 07:11:40PM +0100, Philippe Mathieu-Daudé wrote: > [...] > > >>>> @@ -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. > > This patch is not setting the property on resetfn(), it is > setting it on CPU core pre_plug(). > > This is more complex than simply using qdev_get_machine() and I > don't see why it would be better, but I don't think it's wrong. > The reference to the machine state is basically needed to setup/reset/teardown interrupt presenters in the IRQ chip backend. It is a bit unfortunate to express this dependency at realize(), reset() and unrealize(). Maybe having an "irq_chip" property linked to the IRQ chip backend would make more sense ?
On Wed, Dec 09, 2020 at 09:24:36PM +0100, Greg Kurz wrote: > On Wed, 9 Dec 2020 13:26:17 -0500 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Wed, Dec 09, 2020 at 07:11:40PM +0100, Philippe Mathieu-Daudé wrote: > > [...] > > > >>>> @@ -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. > > > > This patch is not setting the property on resetfn(), it is > > setting it on CPU core pre_plug(). > > > > This is more complex than simply using qdev_get_machine() and I > > don't see why it would be better, but I don't think it's wrong. > > > > The reference to the machine state is basically needed to > setup/reset/teardown interrupt presenters in the IRQ chip > backend. It is a bit unfortunate to express this dependency > at realize(), reset() and unrealize(). Maybe having an > "irq_chip" property linked to the IRQ chip backend would > make more sense ? > Considering that the spapr_irq_*() functions get a SpaprMachineState argument and deal with two interrupt controllers, maybe you won't be able to save what you need in a single irq_chip field? I don't have a strong opinion here. It feels weird to me to save a reference to the global machine object that is always available, but I don't think that's a problem if you believe the resulting code looks better.
On Wed, Dec 09, 2020 at 01:26:17PM -0500, Eduardo Habkost wrote: > On Wed, Dec 09, 2020 at 07:11:40PM +0100, Philippe Mathieu-Daudé wrote: > [...] > > >>>> @@ -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. > > This patch is not setting the property on resetfn(), it is > setting it on CPU core pre_plug(). Well, also machine reset, but the point is it's not the resetfn() of the cpu. Basically what this is doing is machine specific (rather than just cpu specific) initialization of the cpu state - we need that because the pseries machine is implementing an explicitly paravirtualized platform which starts things off in a state a bit different from the "raw" cpu behaviour. So, although it's working on a CPU's state, this function actually belongs to the machine, rather than the cpu. > This is more complex than simply using qdev_get_machine() and I > don't see why it would be better, but I don't think it's wrong. But, yeah, this... I've applied some of the later patches in this series, but I'm not convinced on this one or 2/6. It seems like they're just replacing one ugly (access to qdev_machine_state() as a global) with a different ugly (duplicating something which has to equal the global machine pointer as properties in a bunch of other objects). Both 1/6 and 2/6 are altering explicitly spapr specific devices, they have interactions with the overall platform model which mean they have to sit in that environment, so I think trying to add a property here implies an abstraction that can't actually be used in practice.
On 12/9/20 9:54 PM, Eduardo Habkost wrote: > On Wed, Dec 09, 2020 at 09:24:36PM +0100, Greg Kurz wrote: >> On Wed, 9 Dec 2020 13:26:17 -0500 >> Eduardo Habkost <ehabkost@redhat.com> wrote: >> >>> On Wed, Dec 09, 2020 at 07:11:40PM +0100, Philippe Mathieu-Daudé wrote: >>> [...] >>>>>>>> @@ -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. >>> >>> This patch is not setting the property on resetfn(), it is >>> setting it on CPU core pre_plug(). >>> >>> This is more complex than simply using qdev_get_machine() and I >>> don't see why it would be better, but I don't think it's wrong. >>> >> >> The reference to the machine state is basically needed to >> setup/reset/teardown interrupt presenters in the IRQ chip >> backend. It is a bit unfortunate to express this dependency >> at realize(), reset() and unrealize(). Maybe having an >> "irq_chip" property linked to the IRQ chip backend would >> make more sense ? >> > > Considering that the spapr_irq_*() functions get a > SpaprMachineState argument and deal with two interrupt > controllers, maybe you won't be able to save what you need in a > single irq_chip field? The sPAPR machine needs to operate on all available interrupt controllers and the array is built under the spapr_irq* routines with : SpaprInterruptController *intcs[] = ALL_INTCS(spapr); We could add the array under the machine and use a link to that instead but we need to keep the existing interrupt controllers anyway because of migration compatibility. > I don't have a strong opinion here. It feels weird to me to save > a reference to the global machine object that is always > available, but I don't think that's a problem if you believe the > resulting code looks better. I think it's a good cleanup to remove globals. QEMU might emulate multiple "machines" in a single binary one day. C.
On Thu, 10 Dec 2020 14:53:02 +1100 David Gibson <david@gibson.dropbear.id.au> wrote: > On Wed, Dec 09, 2020 at 01:26:17PM -0500, Eduardo Habkost wrote: > > On Wed, Dec 09, 2020 at 07:11:40PM +0100, Philippe Mathieu-Daudé wrote: > > [...] > > > >>>> @@ -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. > > > > This patch is not setting the property on resetfn(), it is > > setting it on CPU core pre_plug(). > > Well, also machine reset, but the point is it's not the resetfn() of > the cpu. > > Basically what this is doing is machine specific (rather than just cpu > specific) initialization of the cpu state - we need that because the > pseries machine is implementing an explicitly paravirtualized platform > which starts things off in a state a bit different from the "raw" cpu > behaviour. > > So, although it's working on a CPU's state, this function actually > belongs to the machine, rather than the cpu. > > > This is more complex than simply using qdev_get_machine() and I > > don't see why it would be better, but I don't think it's wrong. > > But, yeah, this... > > I've applied some of the later patches in this series, but I'm not > convinced on this one or 2/6. It seems like they're just replacing > one ugly (access to qdev_machine_state() as a global) with a different > ugly (duplicating something which has to equal the global machine > pointer as properties in a bunch of other objects). > > Both 1/6 and 2/6 are altering explicitly spapr specific devices, they > have interactions with the overall platform model which mean they have > to sit in that environment, so I think trying to add a property here > implies an abstraction that can't actually be used in practice. > Eduardo and you convinced me that 1/6 and 2/6 might not be an improvement in the end, but rather making things more complex than simply calling qdev_get_machine() when needed.
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); } } @@ -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() };
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(-)