Re: [Xen-devel] [PATCH] xen/arm: Read the cacheline size from CTR register

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 2 Mar 2018, Julien Grall wrote:
> Hi,
> 
> On 01/03/18 23:27, Stefano Stabellini wrote:
> > See the corresponding Linux commit as reference:
> > 
> >    commit f91e2c3bd427239c198351f44814dd39db91afe0
> >    Author: Catalin Marinas <catalin.marinas@xxxxxxx>
> >    Date:   Tue Dec 7 16:52:04 2010 +0100
> > 
> >        ARM: 6527/1: Use CTR instead of CCSIDR for the D-cache line size on
> > ARMv7
> > 
> >        The current implementation of the dcache_line_size macro reads the L1
> >        cache size from the CCSIDR register. This, however, is not guaranteed
> > to
> >        be the smallest cache line in the cache hierarchy. The patch changes
> > to
> >        the macro to use the more architecturally correct CTR register.
> > 
> >        Reported-by: Kevin Sapp <ksapp@xxxxxxxxxxx>
> >        Signed-off-by: Catalin Marinas <catalin.marinas@xxxxxxx>
> >        Signed-off-by: Russell King <rmk+kernel@xxxxxxxxxxxxxxxx>
> > 
> > Suggested-by: Julien Grall <julien.grall@xxxxxxx>
> > Signed-off-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > 
> > ---
> > 
> > This patch depends on "unsafe big.LITTLE support".
> 
> I still really don't think this should depend on "unsafe big.LITTLE support".
> We want to backport this patch but I am still unconvinced that this is the
> case of the big.LITTLE one. So can you please reshuffle the patches?

I'll move it earlier. For simplicity, I'll make it the first patch of
"unsafe big.LITTLE support", althought I understand that they are
separate fixes.


> > 
> > Previously, we discussed the possibility of reading the cacheline size
> > when needed from the register, instead of reading it from a variable,
> > but going through the code it doesn't seem like a worthwhile
> > optimization.
> 
> Well there are a couple of reasons I wanted this to avoid the a variable:
>       1) Potentially reading a system register + few instructions is faster
> than a memory access
>       2) The name of the variable leads to confusing. It is named
> cacheline_bytes but stores the minimum cacheline size of for the data cache.
> 
> 1) is arguable and I don't much care whether it is done. However, I really
> want to avoid a wrong variable name that could lead to more misuse. So we
> should at least rename read_cacheline_size() and cacheline_bytes.

Sure, I can rename. Would min_cacheline_bytes and
read_min_cacheline_bytes() be clearer? Otherwise, please suggest an
alternative naming scheme.


> 
> > ---
> >   xen/include/asm-arm/cpregs.h |  2 ++
> >   xen/include/asm-arm/page.h   | 11 +++++------
> >   2 files changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/xen/include/asm-arm/cpregs.h b/xen/include/asm-arm/cpregs.h
> > index 9e13848..8db65d5 100644
> > --- a/xen/include/asm-arm/cpregs.h
> > +++ b/xen/include/asm-arm/cpregs.h
> > @@ -106,6 +106,7 @@
> >     /* CP15 CR0: CPUID and Cache Type Registers */
> >   #define MIDR            p15,0,c0,c0,0   /* Main ID Register */
> > +#define CTR             p15,0,c0,c0,1   /* Cache Type Register */
> >   #define MPIDR           p15,0,c0,c0,5   /* Multiprocessor Affinity
> > Register */
> >   #define ID_PFR0         p15,0,c0,c1,0   /* Processor Feature Register 0 */
> >   #define ID_PFR1         p15,0,c0,c1,1   /* Processor Feature Register 1 */
> > @@ -303,6 +304,7 @@
> >   #define CPACR_EL1               CPACR
> >   #define CPTR_EL2                HCPTR
> >   #define CSSELR_EL1              CSSELR
> > +#define CTR_EL0                 CTR
> >   #define DACR32_EL2              DACR
> >   #define ESR_EL1                 DFSR
> >   #define ESR_EL2                 HSR
> > diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> > index 9fbf232..4c81878 100644
> > --- a/xen/include/asm-arm/page.h
> > +++ b/xen/include/asm-arm/page.h
> > @@ -140,14 +140,13 @@ extern size_t cacheline_bytes;
> >     static inline size_t read_cacheline_size(void)
> >   {
> > -    uint32_t ccsid;
> > +    uint32_t ctr;
> >   -    /* Read the cache size ID register for the level-0 data cache */
> > -    WRITE_SYSREG32(0, CSSELR_EL1);
> > -    ccsid = READ_SYSREG32(CCSIDR_EL1);
> > +    /* Read CTR */
> > +    ctr = READ_SYSREG32(CTR_EL0);
> >   -    /* Low 3 bits are log2(cacheline size in words) - 2. */
> > -    return (size_t) (1U << (4 + (ccsid & 0x7)));
> > +    /* 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.
> > 
> 
> -- 
> Julien Grall
> 

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