Replace the ARMCPU field in DigicState by a reference to
an allocated ARMCPU. Instead of initializing the field
with object_initialize(), allocate it with object_new().

As we don't access ARMCPU internal fields or size, we can
move digic.c from arm_ss[] to the more generic softmmu_ss[].

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/digic.c         | 7 ++++---
 hw/arm/meson.build     | 7 ++-----
 include/hw/arm/digic.h | 4 ++--
 3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/hw/arm/digic.c b/hw/arm/digic.c
index 6df5547977..fe24b91db6 100644
--- a/hw/arm/digic.c
+++ b/hw/arm/digic.c
@@ -36,7 +36,8 @@ static void digic_init(Object *obj)
     DigicState *s = DIGIC(obj);
     int i;
 
-    object_initialize_child(obj, "cpu", &s->cpu, ARM_CPU_TYPE_NAME("arm946"));
+    s->cpu = ARM_CPU(object_new(ARM_CPU_TYPE_NAME("arm946")));
+    object_property_add_child(obj, "cpu", OBJECT(s->cpu));
 
     for (i = 0; i < DIGIC4_NB_TIMERS; i++) {
         g_autofree char *name = g_strdup_printf("timer[%d]", i);
@@ -52,12 +53,12 @@ static void digic_realize(DeviceState *dev, Error **errp)
     SysBusDevice *sbd;
     int i;
 
-    if (!object_property_set_bool(OBJECT(&s->cpu), "reset-hivecs", true,
+    if (!object_property_set_bool(OBJECT(s->cpu), "reset-hivecs", true,
                                   errp)) {
         return;
     }
 
-    if (!qdev_realize(DEVICE(&s->cpu), NULL, errp)) {
+    if (!qdev_realize(DEVICE(s->cpu), NULL, errp)) {
         return;
     }
 
diff --git a/hw/arm/meson.build b/hw/arm/meson.build
index f7e1d4add6..0c7554b763 100644
--- a/hw/arm/meson.build
+++ b/hw/arm/meson.build
@@ -2,10 +2,6 @@ arm_ss = ss.source_set()
 arm_ss.add(files('boot.c'), fdt)
 arm_ss.add(when: 'CONFIG_ARM_VIRT', if_true: files('virt.c'))
 arm_ss.add(when: 'CONFIG_ACPI', if_true: files('virt-acpi-build.c'))
-arm_ss.add(when: 'CONFIG_DIGIC', if_true: files('digic_boards.c'))
-arm_ss.add(when: 'CONFIG_MICROBIT', if_true: files('microbit.c'))
-arm_ss.add(when: 'CONFIG_NETDUINO2', if_true: files('netduino2.c'))
-arm_ss.add(when: 'CONFIG_NETDUINOPLUS2', if_true: files('netduinoplus2.c'))
 arm_ss.add(when: 'CONFIG_NPCM7XX', if_true: files('npcm7xx.c', 'npcm7xx_boards.c'))
 arm_ss.add(when: 'CONFIG_NSERIES', if_true: files('nseries.c'))
 arm_ss.add(when: 'CONFIG_REALVIEW', if_true: files('realview.c'))
@@ -18,7 +14,6 @@ arm_ss.add(when: 'CONFIG_SABRELITE', if_true: files('sabrelite.c'))
 arm_ss.add(when: 'CONFIG_ARM_V7M', if_true: files('armv7m.c'))
 arm_ss.add(when: 'CONFIG_EXYNOS4', if_true: files('exynos4210.c'))
 arm_ss.add(when: 'CONFIG_PXA2XX', if_true: files('pxa2xx.c', 'pxa2xx_gpio.c', 'pxa2xx_pic.c'))
-arm_ss.add(when: 'CONFIG_DIGIC', if_true: files('digic.c'))
 arm_ss.add(when: 'CONFIG_OMAP', if_true: files('omap1.c', 'omap2.c'))
 arm_ss.add(when: 'CONFIG_ALLWINNER_A10', if_true: files('allwinner-a10.c', 'cubieboard.c'))
 arm_ss.add(when: 'CONFIG_ALLWINNER_H3', if_true: files('allwinner-h3.c', 'orangepi.c'))
@@ -42,6 +37,8 @@ softmmu_ss.add(when: 'CONFIG_ARM_SMMUV3', if_true: files('smmu-common.c'))
 softmmu_ss.add(when: 'CONFIG_ARMSSE', if_true: files('armsse.c'))
 softmmu_ss.add(when: 'CONFIG_CHEETAH', if_true: files('palm.c'))
 softmmu_ss.add(when: 'CONFIG_COLLIE', if_true: files('collie.c'))
+softmmu_ss.add(when: 'CONFIG_DIGIC', if_true: files('digic.c'))
+softmmu_ss.add(when: 'CONFIG_DIGIC', if_true: files('digic_boards.c'))
 softmmu_ss.add(when: 'CONFIG_EMCRAFT_SF2', if_true: files('msf2-som.c'))
 softmmu_ss.add(when: 'CONFIG_EXYNOS4', if_true: files('exynos4_boards.c'))
 softmmu_ss.add(when: 'CONFIG_GUMSTIX', if_true: files('gumstix.c'))
diff --git a/include/hw/arm/digic.h b/include/hw/arm/digic.h
index 646802806e..1bfd6788c9 100644
--- a/include/hw/arm/digic.h
+++ b/include/hw/arm/digic.h
@@ -18,7 +18,7 @@
 #ifndef HW_ARM_DIGIC_H
 #define HW_ARM_DIGIC_H
 
-#include "target/arm/cpu.h"
+#include "hw/arm/cpu.h"
 #include "hw/timer/digic-timer.h"
 #include "hw/char/digic-uart.h"
 #include "qom/object.h"
@@ -34,7 +34,7 @@ struct DigicState {
     DeviceState parent_obj;
     /*< public >*/
 
-    ARMCPU cpu;
+    ARMCPU *cpu;
 
     DigicTimerState timer[DIGIC4_NB_TIMERS];
     DigicUartState uart;
-- 
2.38.1

On Tue, 10 Jan 2023 at 16:45, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Replace the ARMCPU field in DigicState by a reference to
> an allocated ARMCPU. Instead of initializing the field
> with object_initialize(), allocate it with object_new().
>
> As we don't access ARMCPU internal fields or size, we can
> move digic.c from arm_ss[] to the more generic softmmu_ss[].

I'm not really a fan of this, because it moves away
from a standard coding pattern we've been using for
new QOM 'container' devices, where all the sub-components
of the device are structs embedded in the device's own
struct. This is as opposed to the old style which tended
to use "allocate memory for the sub-component and have
pointers to it". It means the CPU object is now being
treated differently from all the timers, UARTs, etc.

thanks
-- PMM

Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 10 Jan 2023 at 16:45, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Replace the ARMCPU field in DigicState by a reference to
>> an allocated ARMCPU. Instead of initializing the field
>> with object_initialize(), allocate it with object_new().
>>
>> As we don't access ARMCPU internal fields or size, we can
>> move digic.c from arm_ss[] to the more generic softmmu_ss[].
>
> I'm not really a fan of this, because it moves away
> from a standard coding pattern we've been using for
> new QOM 'container' devices, where all the sub-components
> of the device are structs embedded in the device's own
> struct. This is as opposed to the old style which tended
> to use "allocate memory for the sub-component and have
> pointers to it". It means the CPU object is now being
> treated differently from all the timers, UARTs, etc.

I think you can certainly make the argument that CPU's have always been
treated separately because we pass it around as an anonymous pointer all
the time. We currently can't support two concrete CPU types in the same
structure. For example zyncmp has:

  struct XlnxZynqMPState {
      /*< private >*/
      DeviceState parent_obj;

      /*< public >*/
      CPUClusterState apu_cluster;
      CPUClusterState rpu_cluster;
      ARMCPU apu_cpu[XLNX_ZYNQMP_NUM_APU_CPUS];
      ARMCPU rpu_cpu[XLNX_ZYNQMP_NUM_RPU_CPUS];

which only works because A/R cpus are the same underlying type. However
when we want to add Microblaze how would we do it?

Is the main problem preventing us from including multiple cpu.h
definitions that we overload CPUArch and CPUArchState? What are the
implications if we convert them to fully anonymous pointer types?

>
> thanks
> -- PMM


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

On Wed, 25 Jan 2023 at 12:14, Alex Bennée <alex.bennee@linaro.org> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> > I'm not really a fan of this, because it moves away
> > from a standard coding pattern we've been using for
> > new QOM 'container' devices, where all the sub-components
> > of the device are structs embedded in the device's own
> > struct. This is as opposed to the old style which tended
> > to use "allocate memory for the sub-component and have
> > pointers to it". It means the CPU object is now being
> > treated differently from all the timers, UARTs, etc.
>
> I think you can certainly make the argument that CPU's have always been
> treated separately because we pass it around as an anonymous pointer all
> the time. We currently can't support two concrete CPU types in the same
> structure. For example zyncmp has:
>
>   struct XlnxZynqMPState {
>       /*< private >*/
>       DeviceState parent_obj;
>
>       /*< public >*/
>       CPUClusterState apu_cluster;
>       CPUClusterState rpu_cluster;
>       ARMCPU apu_cpu[XLNX_ZYNQMP_NUM_APU_CPUS];
>       ARMCPU rpu_cpu[XLNX_ZYNQMP_NUM_RPU_CPUS];
>
> which only works because A/R cpus are the same underlying type. However
> when we want to add Microblaze how would we do it?

You'd add fields
    MicroBlazeCPU other_cpu;

As you note, at the moment that doesn't work because cpu.h
is magic and embeds an assumption that it's only included
in compile-per-target objects and therefore any given source
file only includes one of them at once. I think we should be
aiming to remove those assumptions, not work around them.

thanks
-- PMM

On 10/1/23 17:52, Peter Maydell wrote:
> On Tue, 10 Jan 2023 at 16:45, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Replace the ARMCPU field in DigicState by a reference to
>> an allocated ARMCPU. Instead of initializing the field
>> with object_initialize(), allocate it with object_new().
>>
>> As we don't access ARMCPU internal fields or size, we can
>> move digic.c from arm_ss[] to the more generic softmmu_ss[].
> 
> I'm not really a fan of this, because it moves away
> from a standard coding pattern we've been using for
> new QOM 'container' devices, where all the sub-components
> of the device are structs embedded in the device's own
> struct. This is as opposed to the old style which tended
> to use "allocate memory for the sub-component and have
> pointers to it". It means the CPU object is now being
> treated differently from all the timers, UARTs, etc.

OK, at least you don't object on patches 1-11/13 :)

I might still post the other parts of this current approach to not
lose them in case I can't find a better way.

Thanks,

Phil.