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

[shashi.mallela@linaro.org]

On Fri, 2021-04-16 at 18:21 +0100, Peter Maydell wrote:
> On Thu, 1 Apr 2021 at 03:41, 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                | 334
> > +++++++++++++++++++++++++
> >  hw/intc/arm_gicv3_its_common.c         |  12 +-
> >  hw/intc/arm_gicv3_its_kvm.c            |   2 +-
> >  hw/intc/gicv3_internal.h               |  86 ++++++-
> >  hw/intc/meson.build                    |   1 +
> >  include/hw/intc/arm_gicv3_its_common.h |  12 +-
> >  6 files changed, 430 insertions(+), 17 deletions(-)
> >  create mode 100644 hw/intc/arm_gicv3_its.c
> 
> Overall I think the structure of the patchset and of the device
> is good, so I'm just going to dive into more detailed review
> comments.
> 
> Have accepted and addressed all the comments and will share the
> updated patch sets soon,responding to comments which need 
>  clarification in all the relevant patch sets 
> 
> > +static MemTxResult its_trans_writew(GICv3ITSState *s, hwaddr
> > offset,
> > +                               uint64_t value, MemTxAttrs attrs)
> > +{
> > +    MemTxResult result = MEMTX_OK;
> > +
> > +    return result;
> > +}
> > +
> > +static MemTxResult its_trans_writel(GICv3ITSState *s, hwaddr
> > offset,
> > +                               uint64_t value, MemTxAttrs attrs)
> > +{
> > +    MemTxResult result = MEMTX_OK;
> > +
> > +    return result;
> > +}
> > +
> > +static MemTxResult gicv3_its_translation_write(void *opaque,
> > hwaddr offset,
> > +                               uint64_t data, unsigned size,
> > MemTxAttrs attrs)
> > +{
> > +    GICv3ITSState *s = (GICv3ITSState *)opaque;
> > +    MemTxResult result;
> > +
> > +    switch (size) {
> > +    case 2:
> > +        result = its_trans_writew(s, offset, data, attrs);
> > +        break;
> > +    case 4:
> > +        result = its_trans_writel(s, offset, data, attrs);
> > +        break;
> > +    default:
> > +        result = MEMTX_ERROR;
> > +        break;
> > +    }
> > +
> > +    if (result == MEMTX_ERROR) {
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: invalid guest write at offset "
> > TARGET_FMT_plx
> > +                      "size %u\n", __func__, offset, size);
> > +        /*
> > +         * The spec requires that reserved registers are RAZ/WI;
> > +         * so use MEMTX_ERROR returns from leaf functions as a way
> > to
> > +         * trigger the guest-error logging but don't return it to
> > +         * the caller, or we'll cause a spurious guest data abort.
> > +         */
> > +        result = MEMTX_OK;
> > +    }
> > +    return result;
> > +}
> 
> There is exactly one register in the translation register frame, so
> having this generic top level write function that calls out to
> separate writew and writel functions is overkill and ends up
> with duplication of code. I would suggest you just fold the
> implementation into gicv3_its_translation_write(), which would
> end up looking something like
> 
>     switch (offset) {
>     case GITS_TRANSLATER:
>         if (s->ctlr & ITS_CTLR_ENABLED) {
>             /* 16 bit accesses behave as writes with bits [31:16]
> zero */
>             s->translater = value;
>             devid = attrs.requester_id;
>             process_int(s, s->translater, devid, NONE);
>         }
>         break;
>     default:
>         break;
>     }
>     return MEMTX_OK;
> 
> which handles both the 16 and 32 bit case. You can tell the
> core memory system code not to hand you 8 or 64 bit accesses
> using fields in the MemoryRegionOps; see below.
> 
> > +static MemTxResult gicv3_its_translation_read(void *opaque, hwaddr
> > offset,
> > +                              uint64_t *data, unsigned size,
> > MemTxAttrs attrs)
> > +{
> > +    qemu_log_mask(LOG_GUEST_ERROR,
> > +        "%s: Invalid read from translation register area at offset
> > "
> > +        TARGET_FMT_plx "\n", __func__, offset);
> > +    return MEMTX_ERROR;
> > +}
> > +
> > +static MemTxResult its_writeb(GICv3ITSState *s, hwaddr offset,
> > +                               uint64_t value, MemTxAttrs attrs)
> > +{
> > +    qemu_log_mask(LOG_UNIMP,
> > +                "%s: unsupported byte write to register at offset
> > "
> > +                TARGET_FMT_plx "\n", __func__, offset);
> > +    return MEMTX_ERROR;
> > +}
> > +
> > +static MemTxResult its_readb(GICv3ITSState *s, hwaddr offset,
> > +                               uint64_t *data, MemTxAttrs attrs)
> > +{
> > +    qemu_log_mask(LOG_UNIMP,
> > +                "%s: unsupported byte read from register at offset
> > "
> > +                TARGET_FMT_plx "\n", __func__, offset);
> > +    return MEMTX_ERROR;
> > +}
> > +
> > +static MemTxResult its_writew(GICv3ITSState *s, hwaddr offset,
> > +                               uint64_t value, MemTxAttrs attrs)
> > +{
> > +    qemu_log_mask(LOG_UNIMP,
> > +        "%s: unsupported word write to register at offset "
> > +        TARGET_FMT_plx "\n", __func__, offset);
> > +    return MEMTX_ERROR;
> > +}
> > +
> > +static MemTxResult its_readw(GICv3ITSState *s, hwaddr offset,
> > +                               uint64_t *data, MemTxAttrs attrs)
> > +{
> > +    qemu_log_mask(LOG_UNIMP,
> > +        "%s: unsupported word read from register at offset "
> > +        TARGET_FMT_plx "\n", __func__, offset);
> > +    return MEMTX_ERROR;
> > +}
> 
> Similarly, there are no byte or halfword accessible registers
> on the ITS, so these are unnecessary; just use MemoryRegionOps
> to cause the core code to reject them.
> 
> > +static MemTxResult its_writel(GICv3ITSState *s, hwaddr offset,
> > +                               uint64_t value, MemTxAttrs attrs)
> > +{
> > +    MemTxResult result = MEMTX_OK;
> > +
> > +    return result;
> > +}
> > +
> > +static MemTxResult its_readl(GICv3ITSState *s, hwaddr offset,
> > +                               uint64_t *data, MemTxAttrs attrs)
> > +{
> > +    MemTxResult result = MEMTX_OK;
> > +
> > +    return result;
> > +}
> > +
> > +static MemTxResult its_writell(GICv3ITSState *s, hwaddr offset,
> > +                               uint64_t value, MemTxAttrs attrs)
> > +{
> > +    MemTxResult result = MEMTX_OK;
> > +
> > +    return result;
> > +}
> > +
> > +static MemTxResult its_readll(GICv3ITSState *s, hwaddr offset,
> > +                               uint64_t *data, MemTxAttrs attrs)
> > +{
> > +    MemTxResult result = MEMTX_OK;
> > +
> > +    return result;
> > +}
> > +
> > +static MemTxResult gicv3_its_read(void *opaque, hwaddr offset,
> > uint64_t *data,
> > +                              unsigned size, MemTxAttrs attrs)
> > +{
> > +    GICv3ITSState *s = (GICv3ITSState *)opaque;
> > +    MemTxResult result;
> > +
> > +    switch (size) {
> > +    case 1:
> > +        result = its_readb(s, offset, data, attrs);
> > +        break;
> > +    case 2:
> > +        result = its_readw(s, offset, data, attrs);
> > +        break;
> > +    case 4:
> > +        result = its_readl(s, offset, data, attrs);
> > +        break;
> > +    case 8:
> > +        result = its_readll(s, offset, data, attrs);
> > +        break;
> > +    default:
> > +        result = MEMTX_ERROR;
> > +        break;
> > +    }
> > +
> > +    if (result == MEMTX_ERROR) {
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: invalid guest read at offset "
> > TARGET_FMT_plx
> > +                      "size %u\n", __func__, offset, size);
> > +        /*
> > +         * The spec requires that reserved registers are RAZ/WI;
> > +         * so use MEMTX_ERROR returns from leaf functions as a way
> > to
> > +         * trigger the guest-error logging but don't return it to
> > +         * the caller, or we'll cause a spurious guest data abort.
> > +         */
> > +        result = MEMTX_OK;
> > +        *data = 0;
> > +    }
> > +    return result;
> > +}
> > +
> > +static MemTxResult gicv3_its_write(void *opaque, hwaddr offset,
> > uint64_t data,
> > +                               unsigned size, MemTxAttrs attrs)
> > +{
> > +    GICv3ITSState *s = (GICv3ITSState *)opaque;
> > +    MemTxResult result;
> > +
> > +    switch (size) {
> > +    case 1:
> > +        result = its_writeb(s, offset, data, attrs);
> > +        break;
> > +    case 2:
> > +        result = its_writew(s, offset, data, attrs);
> > +        break;
> > +    case 4:
> > +        result = its_writel(s, offset, data, attrs);
> > +        break;
> > +    case 8:
> > +        result = its_writell(s, offset, data, attrs);
> > +        break;
> > +    default:
> > +        result = MEMTX_ERROR;
> > +        break;
> > +    }
> > +
> > +    if (result == MEMTX_ERROR) {
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: invalid guest write at offset "
> > TARGET_FMT_plx
> > +                      "size %u\n", __func__, offset, size);
> > +        /*
> > +         * The spec requires that reserved registers are RAZ/WI;
> > +         * so use MEMTX_ERROR returns from leaf functions as a way
> > to
> > +         * trigger the guest-error logging but don't return it to
> > +         * the caller, or we'll cause a spurious guest data abort.
> > +         */
> > +        result = MEMTX_OK;
> > +    }
> > +    return result;
> > +}
> > +
> > +static const MemoryRegionOps gicv3_its_control_ops = {
> > +    .read_with_attrs = gicv3_its_read,
> > +    .write_with_attrs = gicv3_its_write,
> > +    .endianness = DEVICE_NATIVE_ENDIAN,
> 
>        .valid.min_access_size = 4,
>        .valid.max_access_size = 8,
>        .impl.min_access_size = 4,
>        .impl.max_access_size = 8,
> 
> And then you don't need to handle 1 and 2 in your read/write fns.
> 
> 
> > +};
> > +
> > +static const MemoryRegionOps gicv3_its_translation_ops = {
> > +    .read_with_attrs = gicv3_its_translation_read,
> > +    .write_with_attrs = gicv3_its_translation_write,
> > +    .endianness = DEVICE_NATIVE_ENDIAN,
> 
> Similarly here set the valid and impl min and max to 2 and 4.
> 
> > +};
> > +
> > +static void gicv3_arm_its_realize(DeviceState *dev, Error **errp)
> > +{
> > +    GICv3ITSState *s = ARM_GICV3_ITS_COMMON(dev);
> > +
> > +    gicv3_its_init_mmio(s, &gicv3_its_control_ops,
> > &gicv3_its_translation_ops);
> > +}
> > +
> > +static void gicv3_its_reset(DeviceState *dev)
> > +{
> > +    GICv3ITSState *s = ARM_GICV3_ITS_COMMON(dev);
> > +    GICv3ITSClass *c = ARM_GICV3_ITS_GET_CLASS(s);
> > +
> > +    if (s->gicv3->cpu->gicr_typer & GICR_TYPER_PLPIS) {
> > +        c->parent_reset(dev);
> > +
> > +        /* set the ITS default features supported */
> > +        s->typer = FIELD_DP64(s->typer, GITS_TYPER, PHYSICAL,
> > +                                       GITS_TYPE_PHYSICAL);
> > +        s->typer = FIELD_DP64(s->typer, GITS_TYPER,
> > ITT_ENTRY_SIZE,
> > +                                       ITS_ITT_ENTRY_SIZE);
> > +        s->typer = FIELD_DP64(s->typer, GITS_TYPER, IDBITS,
> > ITS_IDBITS);
> > +        s->typer = FIELD_DP64(s->typer, GITS_TYPER, DEVBITS,
> > ITS_DEVBITS);
> > +        s->typer = FIELD_DP64(s->typer, GITS_TYPER, CIL, 1);
> > +        s->typer = FIELD_DP64(s->typer, GITS_TYPER, CIDBITS,
> > ITS_CIDBITS);
> 
> This is a read-only constant value. Two options:
>  (1) set s->typer in realize, not in reset (this is how we handle
>      GICR_TYPER)
>  (2) don't have an s->typer at all, and just return the right
>      constant value when in the read function (this is how we
>      handle GICD_TYPER)
> Implemented option 1
> 
> > +
> > +        /*
> > +         * We claim to be an ARM r0p0 with a zero ProductID.
> > +         * This is the same as an r0p0 GIC-500.
> > +         */
> > +        s->iidr = gicv3_iidr();
> 
> IIDR is constant, so just call gicv3_iidr() to get its value directly
> in the register read function.
> 
> > +
> > +        /* Quiescent bit reset to 1 */
> > +        s->ctlr = FIELD_DP32(s->ctlr, GITS_CTLR, QUIESCENT, 1);
> > +
> > +        /*
> > +         * setting GITS_BASER0.Type = 0b001 (Device)
> > +         *         GITS_BASER1.Type = 0b100 (Collection Table)
> > +         *         GITS_BASER<n>.Type,where n = 3 to 7 are 0b00
> > (Unimplemented)
> > +         *         GITS_BASER<0,1>.Page_Size = 64KB
> > +         * and default translation table entry size to 16 bytes
> > +         */
> > +        s->baser[0] = FIELD_DP64(s->baser[0], GITS_BASER, TYPE,
> > +                                              GITS_ITT_TYPE_DEVICE
> > );
> > +        s->baser[0] = FIELD_DP64(s->baser[0], GITS_BASER,
> > PAGESIZE,
> > +                                              GITS_BASER_PAGESIZE_
> > 64K);
> > +        s->baser[0] = FIELD_DP64(s->baser[0], GITS_BASER,
> > ENTRYSIZE,
> > +                                              GITS_DTE_SIZE);
> > +
> > +        s->baser[1] = FIELD_DP64(s->baser[1], GITS_BASER, TYPE,
> > +                                              GITS_ITT_TYPE_COLLEC
> > TION);
> > +        s->baser[1] = FIELD_DP64(s->baser[1], GITS_BASER,
> > PAGESIZE,
> > +                                              GITS_BASER_PAGESIZE_
> > 64K);
> > +        s->baser[1] = FIELD_DP64(s->baser[1], GITS_BASER,
> > ENTRYSIZE,
> > +                                              GITS_CTE_SIZE);
> > +    }
> > +}
> > +
> > +static Property gicv3_its_props[] = {
> > +    DEFINE_PROP_LINK("parent-gicv3", GICv3ITSState, gicv3, "arm-
> > gicv3",
> > +                     GICv3State *),
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> > +static void gicv3_its_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +    GICv3ITSClass *ic = ARM_GICV3_ITS_CLASS(klass);
> > +
> > +    dc->realize = gicv3_arm_its_realize;
> > +    device_class_set_props(dc, gicv3_its_props);
> > +    device_class_set_parent_reset(dc, gicv3_its_reset, &ic-
> > >parent_reset);
> > +}
> > +
> > +static const TypeInfo gicv3_its_info = {
> > +    .name = TYPE_ARM_GICV3_ITS,
> > +    .parent = TYPE_ARM_GICV3_ITS_COMMON,
> > +    .instance_size = sizeof(GICv3ITSState),
> > +    .class_init = gicv3_its_class_init,
> > +    .class_size = sizeof(GICv3ITSClass),
> > +};
> > +
> > +static void gicv3_its_register_types(void)
> > +{
> > +    type_register_static(&gicv3_its_info);
> > +}
> > +
> > +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..80cc9ec6d8 100644
> > --- a/hw/intc/arm_gicv3_its_common.c
> > +++ b/hw/intc/arm_gicv3_its_common.c
> > @@ -50,12 +50,16 @@ 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(translater, GICv3ITSState),
> >          VMSTATE_UINT32(iidr, GICv3ITSState),
> > +        VMSTATE_UINT64(typer, GICv3ITSState),
> 
> You cannot change a VMStateDescription like this -- you will break
> migration compatibility. Luckily, in this case there's no need:
> GITS_TYPER is read-only, so it doesn't need to be migrated, and
> GITS_TRANSLATER is not a real contains-state register (it's write-
> only),
> so there's no new state there either. Neither needs to be a new field
> in the GICv3ITSState structure either (though typer could be if you
> prefer).
> 
> >          VMSTATE_UINT64(cbaser, GICv3ITSState),
> >          VMSTATE_UINT64(cwriter, GICv3ITSState),
> >          VMSTATE_UINT64(creadr, GICv3ITSState),
> > @@ -99,15 +103,16 @@ static const MemoryRegionOps
> > gicv3_its_trans_ops = {
> >      .endianness = DEVICE_NATIVE_ENDIAN,
> >  };
> > +/**
> > + * Default features advertised by this version of ITS
> > + */
> > +/* Physical LPIs supported */
> > +#define GITS_TYPE_PHYSICAL           (1U << 0)
> > +
> > +/*
> > + * 11 bytes Interrupt translation Table Entry size
> > + * Valid = 1 bit,InterruptType = 1 bit,
> > + * Size of LPI number space[considering max 24 bits],
> > + * Size of LPI number space[considering max 24 bits],
> > + * ICID = 16 bits,
> > + * vPEID = 16 bits
> > + */
> > +#define ITS_ITT_ENTRY_SIZE            0xB
> > +
> > +/* 16 bits EventId */
> > +#define ITS_IDBITS                   GICD_TYPER_IDBITS
> > +
> > +/* 16 bits DeviceId */
> > +#define ITS_DEVBITS                   0xF
> > +
> > +/* 16 bits CollectionId */
> > +#define ITS_CIDBITS                  0xF
> > +
> > +/*
> > + * 8 bytes Device Table Entry size
> > + * Valid = 1 bit,ITTAddr = 44 bits,Size = 5 bits
> > + */
> > +#define GITS_DTE_SIZE                 (0x8ULL)
> > +
> > +/*
> > + * 8 bytes Collection Table Entry size
> > + * Valid = 1 bit,RDBase = 36 bits(considering max RDBASE)
> > + */
> > +#define GITS_CTE_SIZE                 (0x8ULL)
> > +
> 
> Is this the same data structure the in-kernel KVM ITS uses, or is
> it just one specific to the QEMU emulated implementation ?
> This is specific to QEMU emulated implementation
> 
> >  /* Special interrupt IDs */
> >  #define INTID_SECURE 1020
> >  #define INTID_NONSECURE 1021
> > diff --git a/hw/intc/meson.build b/hw/intc/meson.build
> > index 1c299039f6..53472239f0 100644
> > --- a/hw/intc/meson.build
> > +++ b/hw/intc/meson.build
> > @@ -8,6 +8,7 @@ softmmu_ss.add(when: 'CONFIG_ARM_GIC', if_true:
> > files(
> >    'arm_gicv3_dist.c',
> >    'arm_gicv3_its_common.c',
> >    'arm_gicv3_redist.c',
> > +  'arm_gicv3_its.c',
> >  ))
> >  softmmu_ss.add(when: 'CONFIG_ETRAXFS', if_true:
> > files('etraxfs_pic.c'))
> >  softmmu_ss.add(when: 'CONFIG_HEATHROW_PIC', if_true:
> > files('heathrow_pic.c'))
> > diff --git a/include/hw/intc/arm_gicv3_its_common.h
> > b/include/hw/intc/arm_gicv3_its_common.h
> > index 5a0952b404..08bc5652ed 100644
> > --- a/include/hw/intc/arm_gicv3_its_common.h
> > +++ b/include/hw/intc/arm_gicv3_its_common.h
> > @@ -25,17 +25,24 @@
> >  #include "hw/intc/arm_gicv3_common.h"
> >  #include "qom/object.h"
> > 
> > +#define TYPE_ARM_GICV3_ITS "arm-gicv3-its"
> > +
> >  #define ITS_CONTROL_SIZE 0x10000
> >  #define ITS_TRANS_SIZE   0x10000
> >  #define ITS_SIZE         (ITS_CONTROL_SIZE + ITS_TRANS_SIZE)
> > 
> >  #define GITS_CTLR        0x0
> >  #define GITS_IIDR        0x4
> > +#define GITS_TYPER       0x8
> >  #define GITS_CBASER      0x80
> >  #define GITS_CWRITER     0x88
> >  #define GITS_CREADR      0x90
> >  #define GITS_BASER       0x100
> > 
> > +#define GITS_TRANSLATER  0x0040
> > +
> > +#define GITS_PIDR2       0xFFE8
> 
> You probably don't want an offset for GITS_PIDR2 specifically.
> Compare handling of GICD_IDREGS in the distributor emulation.
> Tried re-using the GICD_IDREGS offset for GITS_PIDR2,but from testing
> observed that the linux kernel driver and kvm-unit-tests both relied
> on using the 0xFFE8 offset instead of 0xFFD0 (used by GICD & GICR) to
> complete ITS initialization and setup
> 
> thanks
> -- PMM