Re: [PATCH v2 13/15] xen/arm: Add support for GIC v3

[Ian Campbell <Ian.Campbell@citrix.com>]

On Fri, 2014-04-11 at 18:29 +0530, Vijay Kilari wrote:
> On Thu, Apr 10, 2014 at 3:30 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> >> +
> >> +static void gic_clear_lr(int lr)
> >> +{
> >> +    gich_write_lr(lr, 0);
> >> +}
> >> +
> >> +static void gic_read_lr(int lr, struct gic_lr *lr_reg)
> >
> > I can't find struct gic_lr anywhere.
> Already defined in previous patch

Which? I looked at the time and couldn't find it.

> >
> >> +{
> >> +    u64 lrv;
> >> +    lrv = gich_read_lr(lr);
> >> +    lr_reg->pirq = (lrv >> GICH_LR_PHYSICAL_SHIFT) & GICH_LR_PHYSICAL_MASK;
> >> +    lr_reg->virq = (lrv >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
> >> +    lr_reg->priority = (lrv >> GICH_LR_PRIORITY_SHIFT) & GICH_LR_PRIORITY_MASK;
> >> +    lr_reg->state    = (lrv >> GICH_LR_STATE_SHIFT) & GICH_LR_STATE_MASK;
> >> +    lr_reg->hw_status = (lrv >> GICH_LR_HW_SHIFT) & GICH_LR_HW_MASK;
> >> +    lr_reg->grp = (lrv >> GICH_LR_GRP_SHIFT) & GICH_LR_GRP_MASK;
> >
> > If you want to be able to do field-by-field accesses then please do what
> > the page table code does and use a union and bit fields. See lpae_pt_t.
> >
> >         typedef union __packed {
> >                 uint64_t bits;
> >                 struct {
> >                         unsigned long pirq:4;
> >                         unsugned long virq:4;
> >                 etc, including explicit padding
> >                 };
> >         } gicv3_lr;
> >
> > Then:
> >         gicv3 lrv = {.bits = gich_read_lr(lr)};
> >
> The complexity is LR register is 64 bit in v3 and 32 bit v2.
> Though I define this like above for v2 & v3, generic code still need to access
> based on hw version. So I have make it without bit-fields

How does the generic code access without knowing the size? And if it
knows the size it can equally choose between gicv3_lr and gicv2_lr at
the appropriate point.

> >> +      WRITE_SYSREG32((READ_SYSREG32(ICH_HCR_EL2) | flag), ICH_HCR_EL2);
> >> +    else
> >> +      WRITE_SYSREG32((READ_SYSREG32(ICH_HCR_EL2) & (~flag)), ICH_HCR_EL2);
> >> +}
> >> +
> >> +    rdist_regs = xzalloc_array(struct rdist_region, gic.rdist_count);
> >
> > I thought I saw a comment at the top saying that only a single region
> > was supported? Shouldn't this be checked somewhere, or even better
> > fixed.
> Yes, only rdist_region[0] is read from dt, which supports upto 32 cores.
> So one can add if more than 32 cores is required.

Something should error out if more than 32 cores are present then. Does
it?

> > Is there a limit on gic.rdist_count? How large is it? Can rdist_regs be
> > a static array?
> The rdist count is read from dt. so it is implementation defined

Surely there must be some architectural limit which sets the maximum
size an implementation can use?

Ian.