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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 2 Mar 2018, Stefano Stabellini wrote:
> 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.

Or min_dcache_line_bytes ?

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