On Fri, 30 Apr 2021 at 00:42, Shashi Mallela <shashi.mall...@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.mall...@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.mall...@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
