Re: [Xen-devel] [RFC PATCH 08/49] ARM: VGIC: move max_vcpus VGIC limit to struct arch_domain

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 02/09/2018 02:38 PM, Andre Przywara wrote:
> The VGIC model used for a domain (GICv2 or GICv3) determines the maximum
> number of VCPUs for that guest, as GICv2 can only handle 8 processors.
> In the moment we carry this per-VGIC-model limit in the vgic_ops,
> alongside the model specific functions. That makes some sense, but
> exposes some current VGIC implementation details to generic Xen code.
> Add a new arch specific field in our domain structure to hold this vcpu limit,
> and initialize it when we set the ops. This allows us to plug in the new
> VGIC later without also needing to carry some ops structure.
>
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
> ---
>   xen/arch/arm/domain.c        | 5 ++---
>   xen/arch/arm/vgic.c          | 3 ++-
>   xen/include/asm-arm/domain.h | 1 +
>   3 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index a010443bfd..9ad4cd0a6e 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -975,11 +975,10 @@ unsigned int domain_max_vcpus(const struct domain *d)
>        * allocation when the vgic_ops haven't been initialised yet,
>        * we return MAX_VIRT_CPUS if d->arch.vgic.handler is null.
>        */
> -    if ( !d->arch.vgic.handler )
> +    if ( !d->arch.max_vcpus )
>           return MAX_VIRT_CPUS;
>       else
> -        return min_t(unsigned int, MAX_VIRT_CPUS,
> -                     d->arch.vgic.handler->max_vcpus);
> +        return d->arch.max_vcpus;
>   }
>   
>   /*
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 9921769b15..5f47aa84a9 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -166,7 +166,8 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis)
>   
>   void register_vgic_ops(struct domain *d, const struct vgic_ops *ops)
>   {
> -   d->arch.vgic.handler = ops;
> +    d->arch.vgic.handler = ops;
> +    d->arch.max_vcpus = ops->max_vcpus;
>   }
>   
>   void domain_vgic_free(struct domain *d)
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 1dd9683d25..2fef32eaee 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -149,6 +149,7 @@ struct arch_domain
>   #ifdef CONFIG_SBSA_VUART_CONSOLE
>       struct vpl011 vpl011;
>   #endif
> +    unsigned int max_vcpus;
Now, you have max_vcpus defined in both arch_domain and domain. Which 
makes the code very confusing to read. This is becoming apparent in the 
check if (d->arch.max_vcpus > d->max_vcpus).
If you plan to ditch the ops, then I would prefer a check on the vGIC 
version. Even if it means carrying vGIC specific implementation in 
generic Xen code.
This would also avoid to grow the struct domain just for a "constant 
field" used mostly at guest creation.
Cheers,

--
Julien Grall

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