[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.