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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 2 Mar 2018, Julien Grall wrote:
> 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".

I forgot to update the commit message, I'll fix.


> 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.

Yes, I understand. I'll reshuffle. For simplicity I'll make that patch
part of this series as first patch, although we both understand that
conceptually they are separate.


> > 
> > 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.

It's C99 only :-(


> > ---
> >   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