Re: [PATCH 08/14] cris: replace PROP_PTR with PROP_LINK for interrupt vector

[Peter Maydell]

On Fri, 18 Oct 2019 at 16:43, Marc-André Lureau
<address@hidden> wrote:
>
> Instead of using raw interrupt vector pointer, store the associated
> CPU with a link property.
>
> Signed-off-by: Marc-André Lureau <address@hidden>

A link property is reasonable for a tightly coupled CPU and
interrupt-controller. But in this case the binding is not
actually very tight, and we can avoid it.

> @@ -298,7 +296,7 @@ void axisdev88_init(MachineState *machine)
>
>      dev = qdev_create(NULL, "etraxfs,pic");
>      /* FIXME: Is there a proper way to signal vectors to the CPU core?  */
> -    qdev_prop_set_ptr(dev, "interrupt_vector", &env->interrupt_vector);
> +    object_property_set_link(OBJECT(dev), OBJECT(cpu), "cpu", &error_abort);

Rather than using a link property like this, we could
instead make use of the fact that a qemu_irq line is
actually capable of passing an arbitrary "int" value,
not merely a bool. To do this we would:

 * remove the FIXME comment and the ptr prop/link prop code here
 * remove the definition of the property from the PIC device

> @@ -79,9 +78,10 @@ static void pic_update(struct etrax_pic *fs)
>          }
>      }
>
> -    if (fs->interrupt_vector) {
> -        /* hack alert: ptr property */
> -        *(uint32_t*)(fs->interrupt_vector) = vector;
> +    if (fs->cpu) {
> +        /* hack alert: cpu link property */
> +        int32_t *int_vec = &fs->cpu->env.interrupt_vector;
> +        *int_vec = (uint32_t)vector;
>      }
>      qemu_set_irq(fs->parent_irq, !!vector);

 * here, instead of setting the CPU interrupt_vector field
   and passing !!vector to qemu_set_irq, we just pass "vector",
   so the other end gets the whole integer

 * in target/cris/cpu.c:cris_cpu_set_irq(), we add something like
   if (irq == CRIS_CPU_IRQ) {
       /*
        * The PIC passes us the vector for the IRQ as the value it sends
        * over the qemu_irq line
        */
       cpu->interrupt_vector = value;
   }
   at the top of the function.

It would also be nice somewhere to have a comment documenting
that this is the semantics the CPU expects for its incoming
IRQ line. Unless anybody has a better place, then perhaps
in the part of target/cris/cpu.h that defines CRIS_CPU_IRQ.
(If the PIC followed the just-recently-invented qdev convention
of having a .h file with a comment defining the "QEMU interface"
to the device, as eg include/hw/misc/armsse-mhu.h, then that
comment would be a good place to note that its sysbus IRQ 0
has these value-is-the-vector semantics. But it doesn't.)

>  }
> @@ -164,7 +164,7 @@ static void etraxfs_pic_init(Object *obj)
>  }
>
>  static Property etraxfs_pic_properties[] = {
> -    DEFINE_PROP_PTR("interrupt_vector", struct etrax_pic, interrupt_vector),
> +    DEFINE_PROP_LINK("cpu", struct etrax_pic, cpu, TYPE_CRIS_CPU, CRISCPU *),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> @@ -173,10 +173,6 @@ static void etraxfs_pic_class_init(ObjectClass *klass, 
> void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>
>      dc->props = etraxfs_pic_properties;
> -    /*
> -     * Note: pointer property "interrupt_vector" may remain null, thus
> -     * no need for dc->user_creatable = false;
> -     */
>  }

Incidentally this is a sysbus device, so it's not user
creatable anyway.

thanks
-- PMM