Re: [PATCH v4 7/7] xen/arm: disable CPUs with different dcache line sizes

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

Hi Stefano,

Something is wrong with your threading again. Patch #2-7 have 
"In-Reply-To" matching patch #1 and not the cover letter.

On 02/03/18 19:06, Stefano Stabellini wrote:
> Even different cpus in big.LITTLE systems are expected to have the same
> dcache line size. Unless the minimum of all dcache line sizes is used
> across all cpu cores, cache coherency protocols can go wrong. Instead,
> for now, just disable any cpu with a different dcache line 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
> dcache line 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 dcache_line_bytes at the beginning of start_xen as
> before.
> 
> In start_secondary we check that the dcache level 1 line sizes match,
> otherwise we disable the cpu.
> 
> Suggested-by: Julien Grall <julien.grall@arm.com>
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> ---
> Changes in v4:
> - improve commit message
> - use %zu
> ---
>   xen/arch/arm/setup.c       | 14 +-------------
>   xen/arch/arm/smpboot.c     |  8 ++++++++
>   xen/include/asm-arm/page.h | 11 +++++++++++
>   3 files changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index fced75a..6ccfdab 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -682,18 +682,6 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
>   
>   size_t __read_mostly dcache_line_bytes;
>   
> -/* Very early check of the CPU cache properties */
> -void __init setup_cache(void)
> -{
> -    uint32_t ctr;
> -
> -    /* Read CTR */
> -    ctr = READ_SYSREG32(CTR_EL0);
> -
> -    /* Bits 16-19 are the log2 number of words in the cacheline. */
> -    dcache_line_bytes = (size_t) (4 << ((ctr >> 16) & 0xf));
> -}
> -
>   /* C entry point for boot CPU */
>   void __init start_xen(unsigned long boot_phys_offset,
>                         unsigned long fdt_paddr,
> @@ -707,7 +695,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>       struct domain *dom0;
>       struct xen_arch_domainconfig config;
>   
> -    setup_cache();
> +    dcache_line_bytes = read_dcache_line_size();

It feels a bit odd to have one call dcache_line_bytes and the other call 
read_dcache_line_size. Would it be possible to uniformize the name?

With that:

Reviewed-by: Julien Grall <julien.grall@arm.com>

Cheers,

>   
>       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..d15230b 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 ( dcache_line_bytes != read_dcache_line_size() )
> +    {
> +        printk(XENLOG_ERR "CPU%u dcache line size (%zu) does not match the boot CPU (%zu)\n",
> +               smp_processor_id(), read_dcache_line_size(),
> +               dcache_line_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 ce18f0c..e539f83 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -138,6 +138,17 @@ extern size_t dcache_line_bytes;
>   
>   #define copy_page(dp, sp) memcpy(dp, sp, PAGE_SIZE)
>   
> +static inline size_t read_dcache_line_size(void)
> +{
> +    uint32_t ctr;
> +
> +    /* Read CTR */
> +    ctr = READ_SYSREG32(CTR_EL0);
> +
> +    /* Bits 16-19 are the log2 number of words in the cacheline. */
> +    return (size_t) (4 << ((ctr >> 16) & 0xf));
> +}
> +
>   /* Functions for flushing medium-sized areas.
>    * if 'range' is large enough we might want to use model-specific
>    * full-cache flushes. */
> 

Cheers,

-- 
Julien Grall

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