| Message ID | 20230110164406.94366-1-philmd@linaro.org |
|---|---|
| Headers | show |
| Series | hw/arm: Move various objects to softmmu_ss to build them once (part 1) | expand |
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
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.
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
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
Hi, This series unifies various objects from libqemu-arm-softmmu.fa.p and libqemu-aarch64-softmmu.fa.p into libcommon.fa.p. So instead of building each object twice, one for 32-bit ARM ARM and another for 64-bit Aarch64, hardware-related objects are built once. Part #1 is the low hanging fruits :) Also I cut the series under 20 patches to ease review digestion. The overall logic is to only access ARMCPU as opaque pointer when possible. This way we don't depend on the (target/arm/) "cpu.h" header which contains target-specific definitions and forces units to be added in Meson's arm_ss[] specific source set. In order to reduce use of "target/arm/cpu.h" by hardware units, we split the hardware-facing definitions in the new "hw/arm/cpu.h" header. Finally, instead of using object_initialize() on the full ARMCPU variable, we use object_new(TYPE ARM_CPU). Since QOM types are registered with their class/instance size, we don't need to provide sizeof(ARMCPU) to allocate the object. Please review, Phil. Based-on: <20230109115316.2235-1-philmd@linaro.org> "hw/arm: Cleanups before pflash refactor" Philippe Mathieu-Daudé (18): hw/arm: Move various units to softmmu_ss[] hw/arm/boot: Include missing 'exec/cpu-all.h' header target/arm/cpregs: Include missing 'target/arm/cpu.h' header hw/arm: Use full "target/arm/cpu.h" path to include target's "cpu.h" target/arm: Move CPU QOM type definitions to "hw/arm/cpu.h" target/arm: Move CPU definitions consumed by HW model to "hw/arm/cpu.h" hw/arm: Move more units to softmmu_ss[] hw/arm: Move units to softmmu[] by replacing "{target -> hw}/arm/cpu.h" hw/arm/armv7m: Remove 'target/arm/cpu.h' from NVIC header hw/arm: Move various armv7m-related units to softmmu_ss[] hw/arm/digic: Remove unnecessary target_long use hw/arm/digic: Replace object_initialize(ARMCPU) by object_new(ARMCPU) hw/arm/fsl-imx: Correct GPIO/GPT index in QOM tree hw/arm/fsl-imx25: Replace object_initialize(ARMCPU) by object_new() hw/arm/fsl-imx31: Replace object_initialize(ARMCPU) by object_new() hw/arm/fsl-imx7: Replace object_initialize(ARMCPU) by object_new() hw/arm/fsl-imx6: Replace object_initialize(ARMCPU) by object_new() hw/arm/allwinner: Replace object_initialize(ARMCPU) by object_new() hw/arm/allwinner-a10.c | 10 ++-- hw/arm/allwinner-h3.c | 14 +++--- hw/arm/armv7m.c | 2 + hw/arm/boot.c | 1 + hw/arm/collie.c | 1 - hw/arm/cubieboard.c | 2 +- hw/arm/digic.c | 7 +-- hw/arm/digic_boards.c | 2 +- hw/arm/fsl-imx25.c | 9 ++-- hw/arm/fsl-imx31.c | 9 ++-- hw/arm/fsl-imx6.c | 14 +++--- hw/arm/fsl-imx6ul.c | 12 ++--- hw/arm/fsl-imx7.c | 10 ++-- hw/arm/gumstix.c | 1 - hw/arm/highbank.c | 2 +- hw/arm/imx25_pdk.c | 2 +- hw/arm/integratorcp.c | 2 +- hw/arm/kzm.c | 2 +- hw/arm/mainstone.c | 2 +- hw/arm/mcimx6ul-evk.c | 2 +- hw/arm/mcimx7d-sabre.c | 2 +- hw/arm/meson.build | 83 ++++++++++++++++++---------------- hw/arm/musicpal.c | 2 +- hw/arm/omap_sx1.c | 1 - hw/arm/palm.c | 2 +- hw/arm/sabrelite.c | 2 +- hw/arm/spitz.c | 2 +- hw/arm/strongarm.c | 2 +- hw/arm/z2.c | 1 - include/hw/arm/allwinner-a10.h | 4 +- include/hw/arm/allwinner-h3.h | 4 +- include/hw/arm/cpu.h | 77 +++++++++++++++++++++++++++++++ include/hw/arm/digic.h | 4 +- include/hw/arm/fsl-imx25.h | 4 +- include/hw/arm/fsl-imx31.h | 4 +- include/hw/arm/fsl-imx6.h | 4 +- include/hw/arm/fsl-imx6ul.h | 4 +- include/hw/arm/fsl-imx7.h | 4 +- include/hw/intc/armv7m_nvic.h | 2 +- target/arm/cpregs.h | 2 + target/arm/cpu-qom.h | 28 +----------- target/arm/cpu.h | 42 ++--------------- 42 files changed, 205 insertions(+), 181 deletions(-) create mode 100644 include/hw/arm/cpu.h