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

[Julien Grall <julien.grall@xxxxxxx>]

On 02/03/2018 18:33, Stefano Stabellini wrote:
> 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 ?
I would reduce to dcache_line_bytes to avoid lengthy name.
That would go inline with the macro dcache_line_size we currently have 
in arm64.
What matters is the 'd' for data cache. To avoid confusion which 'i' for 
instruction cache that we may need in the future.
Anyway, I am happy with both.

Cheers,

--
Julien Grall

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