Re: [PATCH V2 05/10] xen/arm: vgic: Use dynamic memory allocation for vgic_rdist_region

[Shanker Donthineni <shankerd@codeaurora.org>]

On 06/27/2016 07:38 AM, Julien Grall wrote:
> Hi Shanker,
>
> On 26/06/16 18:48, Shanker Donthineni wrote:
>> The number of Re-distributor regions allowed for dom0 is hardcoded
>
> s/Re-distributor/Redistributor/
>
>> to a compile time macro MAX_RDIST_COUNT which is 4. On some systems,
>
> s/a compile time macro/a define/
> s/On some/Some/
>
>> especially latest server chips might have more than 4 redistributors.
>
> I would add a comma after 'chips'.
>
> NIT: s/redistributors/Redistributors/
>
>> Either we have to increase MAX_RDIST_COUNT to a bigger number or
>> allocate memory based on number of redistributors that are found in
>
> s/based on number/based on the number/
>
>> MADT table. In the worst case scenario, the macro MAX_RDIST_COUNT
>> should be equal to CONFIG_NR_CPUS in order to support per CPU
>> Redistributors.
>>
>> Increasing MAX_RDIST_COUNT has side effect, it blows 'struct domain'
>
> s/has side/has a/
>
>> size and hits BUILD_BUG_ON() in domain build code path.
>>
>> struct domain *alloc_domain_struct(void)
>> {
>>      struct domain *d;
>>      BUILD_BUG_ON(sizeof(*d) > PAGE_SIZE);
>>      d = alloc_xenheap_pages(0, 0);
>>      if ( d == NULL )
>>          return NULL;
>> ...
>>
>> This patch uses the second approach to fix the BUILD_BUG().
>>
>> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
>> ---
>> Changes since v1:
>>    Keep 'struct vgic_rdist_region' definition inside 'struct
> arch_domain'.
>>
>>   xen/arch/arm/vgic-v2.c       |  6 ++++++
>>   xen/arch/arm/vgic-v3.c       | 22 +++++++++++++++++++---
>>   xen/arch/arm/vgic.c          |  1 +
>>   xen/include/asm-arm/domain.h |  2 +-
>>   xen/include/asm-arm/vgic.h   |  2 ++
>>   5 files changed, 29 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
>> index 9adb4a9..f5778e6 100644
>> --- a/xen/arch/arm/vgic-v2.c
>> +++ b/xen/arch/arm/vgic-v2.c
>> @@ -699,9 +699,15 @@ static int vgic_v2_domain_init(struct domain *d)
>>       return 0;
>>   }
>>
>> +static void vgic_v2_domain_free(struct domain *d)
>> +{
>> +    /* Nothing to be cleanup for this driver */
>> +}
>> +
>>   static const struct vgic_ops vgic_v2_ops = {
>>       .vcpu_init   = vgic_v2_vcpu_init,
>>       .domain_init = vgic_v2_domain_init,
>> +    .domain_free = vgic_v2_domain_free,
>>       .max_vcpus = 8,
>>   };
>>
>> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
>> index b37a7c0..e877e9e 100644
>> --- a/xen/arch/arm/vgic-v3.c
>> +++ b/xen/arch/arm/vgic-v3.c
>> @@ -1393,7 +1393,19 @@ static int vgic_v3_vcpu_init(struct vcpu *v)
>>
>>   static int vgic_v3_domain_init(struct domain *d)
>>   {
>> -    int i;
>> +    struct vgic_rdist_region *rdist_regions;
>> +    int rdist_count, i;
>> +
>> +    /* Allocate memory for Re-distributor regions */
>> +    rdist_count = is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions :
>> +                   GUEST_GICV3_RDIST_REGIONS;
>
> I would directly introduce the inline helper in this patch, rather 
> than in patch #10.
>
Okay, I'll add a helper function as part of this patch.

>> +
>> +    rdist_regions = xzalloc_array(struct vgic_rdist_region,
> rdist_count);
>> +    if ( !rdist_regions )
>> +        return -ENOMEM;
>> +
>> +    d->arch.vgic.nr_regions = rdist_count;
>> +    d->arch.vgic.rdist_regions = rdist_regions;
>>
>>       /*
>>        * Domain 0 gets the hardware address.
>
> [...]
>
>> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
>> index a2fccc0..fbb763a 100644
>> --- a/xen/include/asm-arm/vgic.h
>> +++ b/xen/include/asm-arm/vgic.h
>> @@ -128,6 +128,8 @@ struct vgic_ops {
>>       int (*vcpu_init)(struct vcpu *v);
>>       /* Domain specific initialization of vGIC */
>>       int (*domain_init)(struct domain *d);
>> +    /* Release resources that are allocated by domain_init */
>
> s/are/were/
>
>> +    void (*domain_free)(struct domain *d);
>>       /* vGIC sysreg emulation */
>>       int (*emulate_sysreg)(struct cpu_user_regs *regs, union hsr hsr);
>>       /* Maximum number of vCPU supported */
>>
>
> Regards,
>

-- 
Shanker Donthineni
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project


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