Re: [PATCH v3 1/8] hw/intc: GICv3 ITS initial framework

[Peter Maydell]

On Fri, 30 Apr 2021 at 00:42, Shashi Mallela <shashi.mallela@linaro.org> wrote:
>
> Added register definitions relevant to ITS,implemented overall
> ITS device framework with stubs for ITS control and translater
> regions read/write,extended ITS common to handle mmio init between
> existing kvm device and newer qemu device.
>
> Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
> ---
>  hw/intc/arm_gicv3_its.c                | 239 +++++++++++++++++++++++++
>  hw/intc/arm_gicv3_its_common.c         |  11 +-
>  hw/intc/arm_gicv3_its_kvm.c            |   2 +-
>  hw/intc/gicv3_internal.h               |  88 +++++++--
>  hw/intc/meson.build                    |   1 +
>  include/hw/intc/arm_gicv3_its_common.h |  10 +-
>  6 files changed, 331 insertions(+), 20 deletions(-)
>  create mode 100644 hw/intc/arm_gicv3_its.c
>
> diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
> new file mode 100644
> index 0000000000..7b11330e01
> --- /dev/null
> +++ b/hw/intc/arm_gicv3_its.c
> @@ -0,0 +1,239 @@
> +/*
> + * ITS emulation for a GICv3-based system
> + *
> + * Copyright Linaro.org 2021
> + *
> + * Authors:
> + *  Shashi Mallela <shashi.mallela@linaro.org>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at 
> your
> + * option) any later version.  See the COPYING file in the top-level 
> directory.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/intc/arm_gicv3_its_common.h"
> +#include "gicv3_internal.h"
> +#include "qom/object.h"
> +
> +typedef struct GICv3ITSClass GICv3ITSClass;
> +/* This is reusing the GICv3ITSState typedef from ARM_GICV3_ITS_COMMON */
> +DECLARE_OBJ_CHECKERS(GICv3ITSState, GICv3ITSClass,
> +                     ARM_GICV3_ITS, TYPE_ARM_GICV3_ITS)
> +
> +struct GICv3ITSClass {
> +    GICv3ITSCommonClass parent_class;
> +    void (*parent_reset)(DeviceState *dev);
> +};
> +
> +static MemTxResult gicv3_its_translation_write(void *opaque, hwaddr offset,
> +                               uint64_t data, unsigned size, MemTxAttrs 
> attrs)

Your indentation on function prototypes is still messed up; please fix it.

> +    if (s->gicv3->cpu->gicr_typer & GICR_TYPER_PLPIS) {
> +        /* set the ITS default features supported */
> +        s->typer = FIELD_DP64(s->typer, GITS_TYPER, PHYSICAL,
> +                                       GITS_TYPE_PHYSICAL);

More odd indentation. Second lines of function calls etc should line
up with the first character after the '(' on the first line.
Please fix this through the whole patchset.

> +type_init(gicv3_its_register_types)
> diff --git a/hw/intc/arm_gicv3_its_common.c b/hw/intc/arm_gicv3_its_common.c
> index 66c4c6a188..b4dddb16b8 100644
> --- a/hw/intc/arm_gicv3_its_common.c
> +++ b/hw/intc/arm_gicv3_its_common.c
> @@ -50,12 +50,13 @@ static int gicv3_its_post_load(void *opaque, int 
> version_id)
>
>  static const VMStateDescription vmstate_its = {
>      .name = "arm_gicv3_its",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
>      .pre_save = gicv3_its_pre_save,
>      .post_load = gicv3_its_post_load,
>      .priority = MIG_PRI_GICV3_ITS,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT32(ctlr, GICv3ITSState),
> -        VMSTATE_UINT32(iidr, GICv3ITSState),
>          VMSTATE_UINT64(cbaser, GICv3ITSState),
>          VMSTATE_UINT64(cwriter, GICv3ITSState),
>          VMSTATE_UINT64(creadr, GICv3ITSState),

You can't change the vmstate like this, you break migration compatibility.
Why are you deleting the 'iidr' field anyway ?

If we do need to remove the 'iidr', there is a mechanism for saying
"ignore the UINT64 we get from a migration source, we don't need it".
If we need to do that it should be a standalone patch at the start of
the series.

> @@ -51,7 +56,7 @@ struct GICv3ITSState {
>
>      /* Registers */
>      uint32_t ctlr;
> -    uint32_t iidr;
> +    uint64_t typer;
>      uint64_t cbaser;
>      uint64_t cwriter;
>      uint64_t creadr;

This will break compilation of the KVM support code on aarch64 hosts,
which is still using the 'iidr' field.

thanks
-- PMM