Re: [PATCH 9/9] hw/arm/armv7m: Pass CPU/NVIC using object_property_add_const_link()

[Philippe Mathieu-Daudé]

On 6/2/23 13:17, Philippe Mathieu-Daudé wrote:
> Avoid having QOM objects poke at each other internals.
> Instead, pass references using QOM link properties.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/arm/armv7m.c       | 4 ++--
>   hw/intc/armv7m_nvic.c | 3 ++-
>   target/arm/cpu.c      | 3 ++-
>   3 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
> index 50a9507c0b..edde774da2 100644
> --- a/hw/arm/armv7m.c
> +++ b/hw/arm/armv7m.c
> @@ -338,8 +338,8 @@ static void armv7m_realize(DeviceState *dev, Error **errp)
>        * Tell the CPU where the NVIC is; it will fail realize if it doesn't
>        * have one. Similarly, tell the NVIC where its CPU is.
>        */
> -    s->cpu->env.nvic = &s->nvic;
> -    s->nvic.cpu = s->cpu;
> +    object_property_add_const_link(OBJECT(s->cpu), "nvic", OBJECT(&s->nvic));
> +    object_property_add_const_link(OBJECT(&s->nvic), "cpu", OBJECT(s->cpu));

Markus mentioned this converts build time types checks to runtime.
Also harder to debug. And the code becomes harder to read.

>       if (!qdev_realize(DEVICE(s->cpu), NULL, errp)) {
>           return;
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index d9c7e414bc..e43898a9e0 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -2668,7 +2668,8 @@ static void armv7m_nvic_realize(DeviceState *dev, Error 
> **errp)
>       NVICState *s = NVIC(dev);
>   
>       /* The armv7m container object will have set our CPU pointer */
> -    if (!s->cpu || !arm_feature(&s->cpu->env, ARM_FEATURE_M)) {
> +    s->cpu = ARM_CPU(object_property_get_link(OBJECT(dev), "cpu", 
> &error_abort));
> +    if (!arm_feature(&s->cpu->env, ARM_FEATURE_M)) {
>           error_setg(errp, "The NVIC can only be used with a Cortex-M CPU");
>           return;
>       }
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 876ab8f3bf..f081861947 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1573,12 +1573,13 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>        * error and will result in segfaults if not caught here.
>        */
>       if (arm_feature(env, ARM_FEATURE_M)) {
> +        env->nvic = NVIC(object_property_get_link(OBJECT(dev), "nvic", NULL));
>           if (!env->nvic) {
>               error_setg(errp, "This board cannot be used with Cortex-M CPUs");
>               return;
>           }
>       } else {
> -        if (env->nvic) {
> +        if (object_property_find(OBJECT(dev), "nvic")) {
>               error_setg(errp, "This board can only be used with Cortex-M 
> CPUs");
>               return;
>           }