Re: [Xen-devel] [PATCH v3 14/16] xen/arm: Add virtual GICv3 support

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Thu, Apr 24, 2014 at 4:00 PM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> On Tue, 2014-04-15 at 16:47 +0530, vijay.kilari@xxxxxxxxx wrote:
>> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
>>
>> Add virtual GICv3 driver support
>>
>> This patch adds only basic v3 support.
>> Does not support Interrupt Translation support (ITS)
>>
>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
>> +/*
>> + * Offset of GICD_<FOO><n> with its rank, for GICD_<FOO> with
>> + * <b>-bits-per-interrupt.
>> + */
>> +/* Shift n >> 2 to make it byte register diff */
>
> These two comments should be merged.
>
> What is a "byte register diff"? Do you mean offset in bytes?
>
> The shift still confuses me, even with the comment. Isn't it shifting
> the wrong way to convert something to bytes?
>
>> +#define REG_RANK_INDEX(b, n) (((n) >> 2) & ((b)-1))
>> +
>> +/*
>> + * Returns rank corresponding to a GICD_<FOO><n> register for
>> + * GICD_<FOO> with <b>-bits-per-interrupt.
>> + */
>> +static struct vgic_irq_rank *vgic_irq_rank(struct vcpu *v, int b, int n)
>> +{
>> +    int rank;
>> +
>> +    /* divide by 4 to make it byte size register difference
>> +      as n is difference of 4 byte size register */
>> +    n = n >> 2;
>
> I'm confused again by this comment, if the input n is at word
> granularity why divide and not multiple to get bytes? Or maybe the
> comment is backwards and you have bytes and want a word sized offset?
>
   When REG_RANK_INDEX is called, this macro assumes
that the n value is difference of register in bytes to compute rank

Ex: In old implementation of vgic, the register definitions are divided / 4
So when REG_RANK_INDEX is called, then n value is (x/4 -  y/4)

In the same case in GICv3 I defined all the register definitions are not
divided /4 . Hence n value is x - y which is always multiples of 4.
So I right shifted n by 2 to get the right rank index

In GICv3 there are some register which are 8 bytes (64 bit) for
those I right shift by 3.

>> +        goto read_as_zero;
>> +    case GICR_SETLPIR:
>> +    case GICR_CLRLPIR:
>> +        /* WO return fail */
>> +        return 0;
>
> What is the defined/expected hardware behaviour if you read this
> register? Read garbage or trap?
  Write on RO register is read as zero and Write on RO is write ignored

>
>> +static int vgic_rdistr_mmio_read(struct vcpu *v, mmio_info_t *info)
>> +{
>> +    uint32_t offset;
>> +
>> +    if ( !v->domain->arch.vgic.rdist_stride )
>> +        offset = info->gpa & (v->domain->arch.vgic.rdist_stride - 1);
>> +    else
>> +        offset = info->gpa & 0x1FFFF;
>
> Please initialiase rdist_stride to a default which would cause this same
> behaviour when it is not explicit and avoid the conditional here.

   The rdist_stride value is the value that is received from dt.
   So I have not changed the value

>> +    case REG(0x00c):
>> +    case REG(0x044):
>> +    case REG(0x04c):
>> +    case REG(0x05c) ... REG(0x07c):
>> +    case REG(0xf30) ... REG(0x5fcc):
>> +    case REG(0x8000) ... REG(0xbfcc):
>> +    case REG(0xc000) ... REG(0xffcc):
>
> What distinguishes these registers from those captured by the default
> clause? Are these unimplemented implementation defined registers? Is
> there a reason to expect a guest to legitimately prod them?

   This is no way different from default. These are reserved register addresses.
    Just retained similar to vgic.c (v2).

>> +    for ( i = 0; i < d->arch.vgic.rdist_count; i++ )
>> +    {
>> +        vgic_rdistr_mmio_handler.addr = d->arch.vgic.rbase[i];
>> +        vgic_rdistr_mmio_handler.size = d->arch.vgic.rbase_size[i];
>> +        register_mmio_handler(d, &vgic_rdistr_mmio_handler);
>
> Can we processes the rdistr stride here (or when we setup
> d->arch.vgic.rbase*) and register the sgi region separately?

   sgi region is per core. So we have to register for every core.
   But handling is same for all. So one handler is good enough

-Vijay

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel