|
From: Julian S. <js...@ac...> - 2012-09-24 09:56:15
|
I agree with almost all of this, with only minor comments.
> /* The various kinds of caches */
> typedef enum {
> DATA_CACHE,
> INSN_CACHE,
> DATA_INSN_CACHE // combined data and insn cache
> } cache_kind;
Like JosefW I would prefer COMBINED_CACHE to DATA_INSN_CACHE.
> /* Information about a particular cache */
> typedef struct {
> cache_kind kind;
> UInt level; /* level this cache is at, e.g. 1 for L1 cache */
> UInt sizeB; /* size of this cache in bytes */
> UInt line_sizeB; /* cache line size in bytes */
> UInt associativity;
> } cache_t;
>
> /* Information about the cache system as a whole */
> typedef struct {
> UInt num_levels;
> UInt num_caches;
> /* Unordered array of caches for this host. NULL if there are
> no caches. Users can assume that the array contains at most one
> cache of a given kind per cache level. */
> cache_t *caches;
> } cacheinfo_t;
IIUC, num_levels is the number of levels the machine really has, and
num_caches <= num_levels is the number of caches for which we have
descriptions. Which may be less than the number of levels. Yes?
> (1) cachegrind / callgrind
> These are perfectly served by cacheinfo_t as shown above.
>
> (2) VG_(invalidate_icache)
> We need to extend the above representation, by, say, adding a
> "Bool icaches_maintain_coherence;" to cacheinfo_t
Am I right to understand (from your comments later) that some s390s
have icaches which are coherent, and some do not? So you need to know
the model number in order to decide whether or not VG_(invalid_icache)
is a no-op?
> (3) VEX: VexArchinfo contains ppc_cache_line_szB
> The cache line size is needed to implement the icbi insn.
> Can be obtained from cacheinfo_t
>
> (4) VEX: functions returning VexInvalRange
> The returned address range indicates whether some cache invalidation
> needs to occur later. What is returned here may, in general, depend
> on the particular machine model of a given architecture. So we need
> to query the cache info before returning anything.
> A different possibility, which may even be cleaner, is to make these
> functions *always* return the address range for the insns that were
> patched. In that case we would not need cache information here.
Yes. I would prefer this solution. The patchers just return the address
range they patched, and it is the call site's problem to figure out what to
do about cache coherence.
> How to make cache information available:
> ----------------------------------------
> Ideally we want the cache information to be provided in one spot only;
> be that a function call returning it or some persistent data structure
> containing it. A related question is where the definition of cacheinfo_t
> resides. If it does not reside in VEX then VEX would be dependent on
> coregrind and that's a no-go. So, adding a cacheinfo_t typed member
> to VexArchInfo looks natural.
Yes.
> It could be filled in the same way hwcaps
> are currently filled in in m_machine.c.
> The type definitions (cache_t, cacheinfo_t etc) would be included in
> libvex.h
>
> Tools can call the existing VG_(machine_get_VexArchInfo) to get the
> cache information. The function would have to be exposed through
> pub_tool_machine.h so it becomes available.
> Alternatively, VexArchInfo could be passed to the "instrument" function
> of the tools.
I would prefer both, in fact -- pass it to the instrument function, and
allow tools to make ad-hoc calls to get it, if they want.
> I can work on an implementation for this but first there should be
> agreement
>
> - about the data structures to represent cache info
What you propose seems good to me.
> - how tools access it (function call or passed to the instrument
> function)
Both!
---------
A couple of other comments:
* pls can we use type names beginning w/ capitals, as in the rest
of the code base
* another piece of complexity to be aware of (a small one, though) is
that ARM sets its hwcaps not only by the normal game of trying insns
and seeing which get SIGILL (in m_machine.c), but also by looking at
the AUXV entries on the stack at startup.
Overall, sounds good. It's quite a tricky area to tidy up since I think
it has grown without much planning, over the years.
J
|