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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 6 Mar 2018, Julien Grall wrote:
> Hi Stefano,
> 
> Something is wrong with your threading again. Patch #2-7 have "In-Reply-To"
> matching patch #1 and not the cover letter.

Thanks, I lost my git configuration.


> 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@xxxxxxx>
> > Signed-off-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > 
> > ---
> > 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@xxxxxxx>

I fixed that and the other in-code comment, and committed the whole
series, thanks!


> >         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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel