Re: [PATCH 12/18] hw/arm/digic: Replace object_initialize(ARMCPU) by object_new(ARMCPU)

[Alex Bennée]

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