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

[Vijay Kilari <vijay.kilari@gmail.com>]

On Thu, Apr 10, 2014 at 3:30 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Fri, 2014-04-04 at 17:26 +0530, vijay.kilari@gmail.com wrote:
>
>> +static void write_aprn_regs(struct gic_state_data *d)
>> +{
>> +    switch ( nr_priorities )
>> +    {
>> +    case 7:
>> +        WRITE_SYSREG32(d->v3.gic_apr0[2], ICH_AP0R2_EL2);
>> +        WRITE_SYSREG32(d->v3.gic_apr1[2], ICH_AP1R2_EL2);
>   +        /* Fall-thru */
>
> when doing so deliberately please.
>
> Is it harmful/illegal to write to AP0R2 etc if priorities < 7?

The APR register is implementation defined. It is upto the platform
to support number of APR registers. So based on nr_priorities value
we read save and restore only platform supported registers

>> +
>> +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
>
>> +{
>> +    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

>
>> +static struct dt_irq * gic_maintenance_irq(void)
>> +{
>> +    return &gic.maintenance;
>> +}
>> +
>> +static void gic_hcr_status(uint32_t flag, uint8_t status)
>> +{
>> +    if ( status )
>
> status is actually "bool_t set" ?
>
>> +      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.

>
> 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
>
> Ian.
>
>