On Fri, 20 Oct 2023 at 17:36, Philippe Mathieu-Daudé <philmd@xxxxxxxxxx> wrote:
>
> Heterogeneous machines have different type of CPU.
> qemu_get_cpu() returning unfiltered CPUs doesn't make
> sense anymore. Add a 'type' argument to filter CPU by
> QOM type.
I'm not sure "filter by CPU type" is ever really the
correct answer to this problem, though.
Picking out a handful of arm-related parts to this patchset
as examples of different situations where we're currently
using qemu_get_cpu():
> diff --git a/hw/arm/fsl-imx7.c b/hw/arm/fsl-imx7.c
> index 474cfdc87c..1c1585f3e1 100644
> --- a/hw/arm/fsl-imx7.c
> +++ b/hw/arm/fsl-imx7.c
> @@ -212,7 +212,7 @@ static void fsl_imx7_realize(DeviceState *dev, Error **errp)
>
> for (i = 0; i < smp_cpus; i++) {
> SysBusDevice *sbd = SYS_BUS_DEVICE(&s->a7mpcore);
> - DeviceState *d = DEVICE(qemu_get_cpu(i));
> + DeviceState *d = DEVICE(qemu_get_cpu(i, NULL));
>
> irq = qdev_get_gpio_in(d, ARM_CPU_IRQ);
> sysbus_connect_irq(sbd, i, irq);
This is an Arm SoC object. What it wants is not "the i'th Arm
CPU in the whole system", but "the i'th CPU in this SoC object".
Conveniently, it has easy access to that: s->cpu[i].
> diff --git a/hw/arm/pxa2xx_gpio.c b/hw/arm/pxa2xx_gpio.c
> index e7c3d99224..0a698171ab 100644
> --- a/hw/arm/pxa2xx_gpio.c
> +++ b/hw/arm/pxa2xx_gpio.c
> @@ -303,7 +303,7 @@ static void pxa2xx_gpio_realize(DeviceState *dev, Error **errp)
> {
> PXA2xxGPIOInfo *s = PXA2XX_GPIO(dev);
>
> - s->cpu = ARM_CPU(qemu_get_cpu(s->ncpu));
> + s->cpu = ARM_CPU(qemu_get_cpu(s->ncpu, NULL));
>
> qdev_init_gpio_in(dev, pxa2xx_gpio_set, s->lines);
> qdev_init_gpio_out(dev, s->handler, s->lines);
This is grabbing a private pointer to the CPU object[*], which
we can do more cleanly by setting a link property, and getting
the board code to pass a pointer to the CPU.
[*] it then uses that pointer to mess with the internals of
the CPU to implement wake-up-on-GPIO in a completely horrible
way, but let's assume we don't want to try to clean that up now...
> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> index 3c7dfcd6dc..3571d5038f 100644
> --- a/hw/arm/sbsa-ref.c
> +++ b/hw/arm/sbsa-ref.c
> @@ -275,7 +275,7 @@ static void create_fdt(SBSAMachineState *sms)
>
> for (cpu = sms->smp_cpus - 1; cpu >= 0; cpu--) {
> char *nodename = g_strdup_printf("/cpus/cpu@%d", cpu);
> - ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(cpu));
> + ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(cpu, NULL));
> CPUState *cs = CPU(armcpu);
> uint64_t mpidr = sbsa_ref_cpu_mp_affinity(sms, cpu);
This is in a board model. By definition the board model for a
non-heterogenous board knows it isn't in a heterogenous system
model, and it doesn't need to say "specifically the first Arm CPU".
So I think we should be able to leave it alone...
> diff --git a/hw/cpu/a15mpcore.c b/hw/cpu/a15mpcore.c
> index bfd8aa5644..8c9098d5d3 100644
> --- a/hw/cpu/a15mpcore.c
> +++ b/hw/cpu/a15mpcore.c
> @@ -65,7 +65,7 @@ static void a15mp_priv_realize(DeviceState *dev, Error **errp)
> /* Make the GIC's TZ support match the CPUs. We assume that
> * either all the CPUs have TZ, or none do.
> */
> - cpuobj = OBJECT(qemu_get_cpu(0));
> + cpuobj = OBJECT(qemu_get_cpu(0, NULL));
> has_el3 = object_property_find(cpuobj, "has_el3") &&
> object_property_get_bool(cpuobj, "has_el3", &error_abort);
> qdev_prop_set_bit(gicdev, "has-security-extensions", has_el3);
> @@ -90,7 +90,7 @@ static void a15mp_priv_realize(DeviceState *dev, Error **errp)
> * appropriate GIC PPI inputs
> */
> for (i = 0; i < s->num_cpu; i++) {
> - DeviceState *cpudev = DEVICE(qemu_get_cpu(i));
> + DeviceState *cpudev = DEVICE(qemu_get_cpu(i, NULL));
> int ppibase = s->num_irq - 32 + i * 32;
> int irq;
> /* Mapping from the output timer irq lines from the CPU to the
This is another case where what we want is "the Nth CPU
associated with this peripheral block", not the Nth CPU of
some particular architecture. (It's not as easy to figure
out where we would get that from as it is in the fsl-imx7
case, though -- perhaps we would need to tweak the API
these objects have somehow to pass in pointers to the CPUs?)
> diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
> index 2ebf880ead..cdf21dfc11 100644
> --- a/hw/intc/arm_gicv3_common.c
> +++ b/hw/intc/arm_gicv3_common.c
> @@ -392,7 +392,7 @@ static void arm_gicv3_common_realize(DeviceState *dev, Error **errp)
> s->cpu = g_new0(GICv3CPUState, s->num_cpu);
>
> for (i = 0; i < s->num_cpu; i++) {
> - CPUState *cpu = qemu_get_cpu(i);
> + CPUState *cpu = qemu_get_cpu(i, NULL);
> uint64_t cpu_affid;
>
> s->cpu[i].cpu = cpu;
These gicv3 uses of qemu_get_cpu() are because instead of doing
the theoretical Right Thing and having the GIC object have to
be told which CPUs it is responsible for, we took a shortcut
and said "there's only one GIC, and it's connected to all the CPUs".
The fix is, again, not filtering by CPU type, but having the
board and SoC models do the work to explicitly represent
"this GIC object is attached to these CPU objects" (via link
properties or otherwise).
So overall there are some places where figuring out the right
replacement for qemu_get_cpu() is tricky, and some places where
it's probably fairly straightforward but just an annoying
amount of extra code to write, and some places where we don't
care because we know the board model is not heterogenous.
But I don't think "filter by CPU architecture type" is usually
going to be what we want.
thanks
-- PMM
