Re: [PATCH V2 3/8] xen/arm: Add funtions of mapping between vCPUID and vMPIDR

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

Hi Chen,

Title: s/funtions/functions/

On 23/05/2015 14:52, Chen Baozi wrote:
> From: Chen Baozi <baozich@gmail.com>
>
> GICv3 restricts that the maximum number of CPUs in affinity 0 (one
> cluster) is 16. That is to say the upper 4 bits of affinity 0 is unused.
> Current implementation considers that AFF0 is equal to vCPUID, which
> makes all vCPUs in one cluster, limiting its number to 16. If we would
> like to support more than 16 number of vCPU in one guest, we need to
> make use of AFF1. Considering the unused upper 4 bits, we need to create
> a pair of functions mapping the vCPUID and vMPIDR.

The functions you are adding don't deal with the vMPIDR but only a part 
of it used for the affinity.

I would rename the title and modify this last sentence to reflect it.

> Signed-off-by: Chen Baozi <baozich@gmail.com>
> ---
>   xen/include/asm-arm/domain.h | 34 ++++++++++++++++++++++++++++++++++
>   1 file changed, 34 insertions(+)
>
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 75b17af..9d3e406 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -266,6 +266,40 @@ static inline unsigned int domain_max_vcpus(const struct domain *d)
>       return MAX_VIRT_CPUS;
>   }
>
> +/*
> + * Due to the restriction of GICv3, the number of vCPUs in AFF0 is
> + * limited to 16, thus only the first 4 bits of AFF0 are legal. We will
> + * use the first 2 affinity levels here, expanding the number of vCPU up
> + * to 4096 (16*256), which is more than 128 PEs that GIC-500 supports.
> + *
> + * Since we don't save information of vCPU's topology (affinity) in
> + * vMPIDR at the moment, we map the vcpuid to the vMPIDR linearly.
> + *
> + * XXX: May have multi-threading or virtual cluster information in the

We may have ...

> + * future.
> + */
> +static inline unsigned int vaffinity_to_vcpuid(register_t vaff)
> +{
> +    unsigned int vcpuid;
> +
> +    vaff &= MPIDR_HWID_MASK;
> +
> +    vcpuid = (vaff >> MPIDR_LEVEL_SHIFT(0)) & 0x0f;

You can use MPIDR_AFFINITY_LEVEL(0)

> +    vcpuid |= ((vaff >> MPIDR_LEVEL_SHIFT(1)) & 0xff) << 4;

Same here with 1.

> +    return vcpuid;
> +}
> +
> +static inline register_t vcpuid_to_vaffinity(unsigned int vcpuid)
> +{
> +    register_t vaff;

I would add a BUILD_BUG_ON(MAX_VIRT_CPUS < ((1 << 12))) in order to 
catch MAX_VIRT_CPUS increasing without changing the mapping between the 
VCPU ID and the affinity.

> +    vaff = (vcpuid & 0x0f) << MPIDR_LEVEL_SHIFT(0);
> +    vaff |= ((vcpuid >> 4) & 0xff) << MPIDR_LEVEL_SHIFT(1);

s/0xff/MPIDR_LEVEL_MASK/

> +
> +    return vaff;
> +}
> +
>   #endif /* __ASM_DOMAIN_H__ */
>
>   /*
>

Regards,

-- 
Julien Grall