Re: [PATCH v2 06/15] xen/arm: vgic-v3: Set stride during domain initialization

[Julien Grall <julien.grall@linaro.org>]

On 02/02/15 15:40, Ian Campbell wrote:
> On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote:
>> The stride may not be set if the hardware GIC is using the default
>> layout. It happens on the Foundation model.
>>
>> On GICv3, the default stride is 2 * 64K. Therefore it's possible to avoid
>> checking at every redistributor MMIO access if the stride is not set.
> 
> Can this defaulting not be pulled further to the initialisation of
> gicv3.rdist_stride?

With the upcoming GICv4, the stride may be different for each
distributor (see the check on GICR_TYPER.VLPIS in gicv3_populate_rdist).

So I'd like to avoid the check of rdist_stride.

>> This is only happening for DOM0,
> 
> Please say instead "Because domU uses a static stride configuration this
> only happens for dom0..." or similar (i.e. include the reason why domU
> is excluded)

I will do.

>>  so we can move this code in
>> gicv_v3_init. Take the opportunity to move the stride setting a bit ealier
> 
> "earlier".
> 
>> because the loop to set regions will require the stride.
>>
>> Also, use 2 * 64K rather than 128K and explain the reason.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> ---
>>     I wasn't not sure where to move this code. I find very confusion the
>>     splitting between vgic and gicv. Maybe we should introduce a
>>     hwdom_gicv_init and giccc_map callbacks. Then move most of the
>>     initialization in the vgic one.
>>
>>     Changes in v2:
>>         - Patch added
>> ---
>>  xen/arch/arm/gic-v3.c  | 11 ++++++++++-
>>  xen/arch/arm/vgic-v3.c |  6 +-----
>>  2 files changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index 47452ca..7b33ff7 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -897,12 +897,21 @@ static int gicv_v3_init(struct domain *d)
>>      {
>>          d->arch.vgic.dbase = gicv3.dbase;
>>          d->arch.vgic.dbase_size = gicv3.dbase_size;
>> +
>> +        d->arch.vgic.rdist_stride = gicv3.rdist_stride;
>> +        /*
>> +         * If the stride is not set, the default stride for GICv3 is 2 * 64K:
>> +         *     - first 64k page for Control and Physical LPIs
>> +         *     - second 64k page for Control and Generation of SGIs
>> +         */
>> +        if ( !d->arch.vgic.rdist_stride )
>> +            d->arch.vgic.rdist_stride = 2 * SZ_64K;
>> +
>>          for ( i = 0; i < gicv3.rdist_count; i++ )
>>          {
>>              d->arch.vgic.rbase[i] = gicv3.rdist_regions[i].base;
>>              d->arch.vgic.rbase_size[i] = gicv3.rdist_regions[i].size;
>>          }
>> -        d->arch.vgic.rdist_stride = gicv3.rdist_stride;
>>          d->arch.vgic.rdist_count = gicv3.rdist_count;
>>      }
>>      else
>> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
>> index 2c14717..db6b514 100644
>> --- a/xen/arch/arm/vgic-v3.c
>> +++ b/xen/arch/arm/vgic-v3.c
>> @@ -625,11 +625,7 @@ static int vgic_v3_rdistr_mmio_read(struct vcpu *v, mmio_info_t *info)
> 
> Why not the write case too?

By mistake it has been dropped in a following patch ("Emulate correctly
the re-distributor"). I will move the changes here.

Regards,

-- 
Julien Grall