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

[Julien Grall <julien.grall@arm.com>]

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.

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

-- 
Julien Grall

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