Re: [Xen-devel] [PATCH v3 6/6] xen/arm: disable CPUs with different cacheline sizes

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 01/03/18 23:26, Stefano Stabellini wrote:
> Even different cpus in big.LITTLE systems are expected to have the same
> cacheline size. Unless the minimum of all cacheline sizes is used across
> all cpu cores, cache coherency protocols can go wrong. Instead, for
> now, just disable any cpu with a different cacheline size.
>
> This check is not covered by the hmp-unsafe option, because even with
> the correct scheduling and vcpu pinning in place, the system breaks if
> cacheline sizes differ across cores. We don't believe it is a problem
> for most big.LITTLE systems.
>
> This patch moves the implementation of setup_cache to a static inline,
> still setting cacheline_bytes at the beginning of start_xen as before.
>
> In start_secondary we check that the cacheline sizes match, otherwise we
> disable the cpu.
I am afraid that this commit message is only valid after "xen/arm: Read 
the cacheline from CTR register".
What you effectively check in that patch is the D-cache level 1 line 
size is equal on every CPU. You could rewrite the commit message to 
reflect that, but then people may wonder why you impose such restriction 
on Xen? So it would really make sense to fix the way to read the 
D-cacheline size first.
> Suggested-by: Julien Grall <julien.grall@xxxxxxx>
> Signed-off-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>
> ---
> Changes in v3:
> - new patch
>
> ---
> Interestingly I couldn't find a better way in C89 to printk a size_t
> than casting it to unsigned long.
You can use %zu.

> ---
>   xen/arch/arm/setup.c       | 15 +--------------
>   xen/arch/arm/smpboot.c     |  8 ++++++++
>   xen/include/asm-arm/page.h | 12 ++++++++++++
>   3 files changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 032a6a8..b5f4c3a 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -682,19 +682,6 @@ static void __init setup_mm(unsigned long dtb_paddr, 
> size_t dtb_size)
>   
>   size_t __read_mostly cacheline_bytes;
>   
> -/* Very early check of the CPU cache properties */
> -void __init setup_cache(void)
> -{
> -    uint32_t ccsid;
> -
> -    /* Read the cache size ID register for the level-0 data cache */
> -    WRITE_SYSREG32(0, CSSELR_EL1);
> -    ccsid = READ_SYSREG32(CCSIDR_EL1);
> -
> -    /* Low 3 bits are log2(cacheline size in words) - 2. */
> -    cacheline_bytes = 1U << (4 + (ccsid & 0x7));
> -}
> -
>   /* C entry point for boot CPU */
>   void __init start_xen(unsigned long boot_phys_offset,
>                         unsigned long fdt_paddr,
> @@ -708,7 +695,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>       struct domain *dom0;
>       struct xen_arch_domainconfig config;
>   
> -    setup_cache();
> +    cacheline_bytes = read_cacheline_size();
>   
>       percpu_init_areas();
>       set_processor_id(0); /* needed early, for smp_processor_id() */
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 04efb33..153572e 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -323,6 +323,14 @@ void start_secondary(unsigned long boot_phys_offset,
>           stop_cpu();
>       }
>   
> +    if ( cacheline_bytes != read_cacheline_size() )
> +    {
> +        printk(XENLOG_ERR "CPU%u cacheline size (%lu) does not match the boot CPU 
> (%lu)\n",
> +               smp_processor_id(), (unsigned long) read_cacheline_size(),
> +               (unsigned long) cacheline_bytes);
> +        stop_cpu();
> +    }
> +
>       mmu_init_secondary_cpu();
>   
>       gic_init_secondary_cpu();
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index d948250..9fbf232 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -138,6 +138,18 @@ extern size_t cacheline_bytes;
>   
>   #define copy_page(dp, sp) memcpy(dp, sp, PAGE_SIZE)
>   
> +static inline size_t read_cacheline_size(void)
> +{
> +    uint32_t ccsid;
> +
> +    /* Read the cache size ID register for the level-0 data cache */
> +    WRITE_SYSREG32(0, CSSELR_EL1);
> +    ccsid = READ_SYSREG32(CCSIDR_EL1);
> +
> +    /* Low 3 bits are log2(cacheline size in words) - 2. */
> +    return (size_t) (1U << (4 + (ccsid & 0x7)));
> +}
> +
>   /* Functions for flushing medium-sized areas.
>    * if 'range' is large enough we might want to use model-specific
>    * full-cache flushes. */
--
Julien Grall

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