Re: [PATCH v2 08/17] libxl/arm: Factor MPIDR computing codes out as a helper

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

Hi Shannon,

On 23/06/16 04:16, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
>
> Factor MPIDR computing codes out as a helper, so it could be shared
> between DT and ACPI.
>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>   tools/libxl/libxl_arm.c      |  8 +-------
>   tools/libxl/libxl_arm_acpi.h | 12 ++++++++++++
>   2 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index f5db74b..a766732 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -312,13 +312,7 @@ static int make_cpus_node(libxl__gc *gc, void *fdt, int nr_cpus,
>       for (i = 0; i < nr_cpus; i++) {
>           const char *name;
>
> -        /*
> -         * According to ARM CPUs bindings, the reg field should match
> -         * the MPIDR's affinity bits. We will use AFF0 and AFF1 when
> -         * constructing the reg value of the guest at the moment, for it
> -         * is enough for the current max vcpu number.
> -         */
> -        mpidr_aff = (i & 0x0f) | (((i >> 4) & 0xff) << 8);
> +        mpidr_aff = libxl__compute_mpdir(i);
>           name = GCSPRINTF("cpu@%"PRIx64, mpidr_aff);
>
>           res = fdt_begin_node(fdt, name);
> diff --git a/tools/libxl/libxl_arm_acpi.h b/tools/libxl/libxl_arm_acpi.h
> index 5899210..9b58de6 100644
> --- a/tools/libxl/libxl_arm_acpi.h
> +++ b/tools/libxl/libxl_arm_acpi.h

I do not think libxl_arm_acpi.h is the right place to add 
libxl__compute_mpidr. This function is clearly not ACPI specific. You 
may want to rename this file libxl_arm.h instead.

> @@ -23,6 +23,18 @@ int libxl__prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info,
>                           libxl__domain_build_state *state,
>                           struct xc_dom_image *dom);
>
> +static inline uint64_t libxl__compute_mpdir(int index)

'index' should be unsigned int. It would also make sense to name this 
variable 'cpuid'.

> +{
> +    /*
> +     * According to ARM CPUs bindings, the reg field should match
> +     * the MPIDR's affinity bits. We will use AFF0 and AFF1 when
> +     * constructing the reg value of the guest at the moment, for it
> +     * is enough for the current max vcpu number.
> +     */
> +    assert(index >= 0);

This will avoid this assert here.

> +    return (index & 0x0f) | (((index >> 4) & 0xff) << 8);
> +}
> +
>   /*
>    * Local variables:
>    * mode: C
>

Regards,

-- 
Julien Grall

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