Re: [PATCH v5 01/10] hw/intc: GICv3 ITS initial framework

[shashi . mallela]

On Mon, 2021-07-05 at 15:58 +0100, Peter Maydell wrote:
> On Wed, 30 Jun 2021 at 16:32, 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>
> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> > +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);
> > +
> > +    if (s->gicv3->cpu->gicr_typer & GICR_TYPER_PLPIS) {
> 
> Can you remind me why we make this check, please? When would we
> have created an ITS device but not have a GICv3 with LPI support?
This check applies to GIC's physical LPI support only as against GIC's
virtual LPI support. 
> 
> Maybe it would be better to either
> (a) simply create the ITS and assume that the board connected it up
> to a GICv3 that supports it
> (b) check every CPU for whether PLPIS is set, and if one of them does
> not have it set then return an error from the ITS realize
> 
> ?
> 
> (Found this by looking for code where we do s->gicv3->cpu-
> >something...)
> 
> > +        /* 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 - 1);
> > +        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);
> > +    }
> > +}
> > +
> > +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) {
> 
> Similarly here.
> 
> > +        c->parent_reset(dev);
> > +
> > +        /* 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 - 1);
> > +
> > +        s->baser[1] = FIELD_DP64(s->baser[1], GITS_BASER, TYPE,
> > +                                 GITS_ITT_TYPE_COLLECTION);
> > +        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 - 1);
> > +    }
> > +}
> 
> thanks
> -- PMM