Re: [Xen-devel] [PATCH 2/7] ARM: vGICv3: drop GUEST_GICV3_RDIST_REGIONS symbol

[Andre Przywara <andre.przywara@xxxxxxxxxx>]

Hi,

On 24/01/18 16:13, Julien Grall wrote:
> Hi Andre,
> 
> On 24/01/18 14:35, Andre Przywara wrote:
>> Architecturally there is only one GICv3 redistributor region.
>> Drop the symbol which suggested that was a delibarate choice for Xen
>> guests, instead hard code the "1" in the appropriate places, along with
>> a comment to explain the reasons.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
>> ---
>>   xen/arch/arm/vgic-v3.c        | 17 ++++++++++++-----
>>   xen/include/public/arch-arm.h |  1 -
>>   2 files changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
>> index af16dfd005..7d3ea171b4 100644
>> --- a/xen/arch/arm/vgic-v3.c
>> +++ b/xen/arch/arm/vgic-v3.c
>> @@ -1640,8 +1640,18 @@ static int vgic_v3_vcpu_init(struct vcpu *v)
>>     static inline unsigned int vgic_v3_rdist_count(struct domain *d)
>>   {
>> -    return is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions :
>> -               GUEST_GICV3_RDIST_REGIONS;
>> +    /*
>> +     * Architecturally there is only one GICv3 redistributor region.
>> +     * The GICv3 DT binding provisions for multiple regions, since
>> there are
>> +     * platforms out there which break this architectural assumption.
>> +     * ACPI does not support this workaround at all.
> 
> This is not true. The ACPI spec supports multiple regions of
> redistributors. What ACPI does not support is a different stride.

Ah, that's true. Sorry for that, I was a bit too enthusiastic here ;-)
Will change the comment, definitely.
However I would still be interested in dropping the
GUEST_GICV3_RDIST_REGIONS symbol, possibly replacing it with
DEFAULT_GICV3_RDIST_REGIONS, avoiding the misleading GUEST_ prefix.

Cheers,
Andre.

>> +     * For Dom0 we have to live with the MMIO layout the hardware
>> provides,
>> +     * so we have to copy the multiple regions - as the first region
>> may not
>> +     * provide enough space to hold all redistributors we need.
>> +     * However DomU get a constructed memory map, so we can go with
>> +     * the architected single redistributor region.
>> +     */
>> +    return is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions : 1;
>> >   }
>>     static int vgic_v3_domain_init(struct domain *d)
>> @@ -1700,9 +1710,6 @@ static int vgic_v3_domain_init(struct domain *d)
>>       {
>>           d->arch.vgic.dbase = GUEST_GICV3_GICD_BASE;
>>   -        /* XXX: Only one Re-distributor region mapped for the guest */
>> -        BUILD_BUG_ON(GUEST_GICV3_RDIST_REGIONS != 1);
>> -
>>           d->arch.vgic.rdist_stride = GUEST_GICV3_RDIST_STRIDE;
>>             /* The first redistributor should contain enough space for
>> all CPUs */
>> diff --git a/xen/include/public/arch-arm.h
>> b/xen/include/public/arch-arm.h
>> index 05fd11ca38..ca79ab6284 100644
>> --- a/xen/include/public/arch-arm.h
>> +++ b/xen/include/public/arch-arm.h
>> @@ -402,7 +402,6 @@ typedef uint64_t xen_callback_t;
>>   #define GUEST_GICV3_GICD_SIZE      xen_mk_ullong(0x00010000)
>>     #define GUEST_GICV3_RDIST_STRIDE   xen_mk_ullong(0x00020000)
>> -#define GUEST_GICV3_RDIST_REGIONS  1
>>     #define GUEST_GICV3_GICR0_BASE     xen_mk_ullong(0x03020000) /*
>> vCPU0..127 */
>>   #define GUEST_GICV3_GICR0_SIZE     xen_mk_ullong(0x01000000)
>>
> 
> Cheers,
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel