Re: [Qemu-devel] [RFC PATCH v3 1/5] hw/intc: Implement ITS base class

[Peter Maydell <peter.maydell@linaro.org>]

On 24 November 2015 at 10:13, Pavel Fedin <p.fedin@samsung.com> wrote:
> This is the basic skeleton for both KVM and software-emulated ITS.
> Since we already prepare status structure, we also introduce complete
> VMState description. But, because we currently have no migratable
> implementations, we also set unmigratable flag.
>
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>

This mostly looks good, I just have a few minor points:

> +static uint64_t gicv3_its_trans_read(void *opaque, hwaddr offset, unsigned size)
> +{
> +    qemu_log_mask(LOG_GUEST_ERROR, "ITS read at offset 0x%jX\n", offset);
> +    return ~0ULL;

I don't think %j is the right format string character here or below:
(we don't use it anywhere else in QEMU and the docs say it is
for uintmax_t/intmax_t types).

> +}

> +static const MemoryRegionOps gicv3_its_trans_ops = {
> +    .read = gicv3_its_trans_read,
> +    .write_with_attrs = gicv3_its_trans_write,

Please make both read and write accessor be the _with_attrs form.
I know the read accessor doesn't need the attributes, but I think
it's neater to have them both the same within a given memory region.

> +    .impl = {
> +         .min_access_size = 4,
> +         .max_access_size = 4,

The spec says that 16-bit access to bits [15:0] of GITS_TRANSLATER
must be supported.

> +     },
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +void gicv3_its_init_mmio(GICv3ITSState *s, const MemoryRegionOps *ops)
> +{
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(s);
> +
> +    memory_region_init_io(&s->iomem_its_cntrl, OBJECT(s), ops, s,
> +                          "control", ITS_CONTROL_SIZE);
> +    memory_region_init_io(&s->iomem_its, OBJECT(s), &gicv3_its_trans_ops, s,
> +                          "translation", ITS_TRANS_SIZE);
> +
> +    /* Our two regions are always adjacent, therefore we now combine them
> +     * into a single one in order to make our users' life easier.
> +     */
> +    memory_region_init(&s->iomem_main, OBJECT(s), "gicv3_its", ITS_SIZE);
> +    memory_region_add_subregion(&s->iomem_main, 0, &s->iomem_its_cntrl);
> +    memory_region_add_subregion(&s->iomem_main, ITS_CONTROL_SIZE,
> +                                &s->iomem_its);
> +    sysbus_init_mmio(sbd, &s->iomem_main);

So we have two memory subregions:
 * the control register page, whose ops are passed in by the subclass
 * the translation register page, whose only register is implemented
   here by looking up the subclass and calling its send_msi method

Does that complexity gain us anything later? There doesn't really
seem to be anything much actually in common here, which would
suggest just having the subclasses create their own mmio region(s)
(which could then just directly implement the right behaviour for
GITS_TRANSLATER). Or does the emulated ITS have extra stuff that
makes this worthwhile?


> --- a/target-arm/kvm_arm.h
> +++ b/target-arm/kvm_arm.h
> @@ -215,4 +215,14 @@ static inline const char *gic_class_name(void)
>   */
>  const char *gicv3_class_name(void);
>
> +/**
> + * its_class_name
> + *
> + * Return name of ITS class to use depending on whether KVM acceleration is
> + * in use, or NULL if the chosen implementation is not available.
> + *
> + * Returns: class name to use or NULL
> + */
> +const char *its_class_name(void);
> +
>  #endif
> diff --git a/target-arm/machine.c b/target-arm/machine.c
> index 36a0d15..6c59c53 100644
> --- a/target-arm/machine.c
> +++ b/target-arm/machine.c
> @@ -346,3 +346,19 @@ const char *gicv3_class_name(void)
>
>      exit(1);
>  }
> +
> +const char *its_class_name(void)
> +{
> +    if (kvm_irqchip_in_kernel()) {
> +#ifdef TARGET_AARCH64
> +        /* KVM implementation requires this capability */
> +        if (kvm_direct_msi_enabled()) {
> +            return "arm-its-kvm";
> +        }
> +#endif

Why is this in an #ifdef? In theory we could support
the GICv3 in a 32-bit system. If it isn't supported
then I'd expect kvm_direct_msi_enabled() to return false
on a non-AArch64 system anyway.

> +        return NULL;
> +    } else {
> +        /* Software emulation is not implemented yet */
> +        return NULL;
> +    }
> +}

This doesn't seem like it should be in machine.c, which is
purely related to code for migration. I would just make it
be an inline implementation in kvm-arm.h, like gic_class_name().
(I know gicv3_class_name is in machine.c, but I must have missed
that it was in an odd file when that patch went through review.
Sorry.)

thanks
-- PMM