On Tue, 5 May 2020 18:16:31 +0100
Andrew Scull <ascull@xxxxxxxxxx> wrote:
Hi Andrew,
> > +#define __tlbi_level(op, addr, level) \
> > + do { \
> > + u64 arg = addr; \
> > + \
> > + if (cpus_have_const_cap(ARM64_HAS_ARMv8_4_TTL) && \
> > + level) { \
> > + u64 ttl = level; \
> > + \
> > + switch (PAGE_SIZE) { \
> > + case SZ_4K: \
> > + ttl |= 1 << 2; \
> > + break; \
> > + case SZ_16K: \
> > + ttl |= 2 << 2; \
> > + break; \
> > + case SZ_64K: \
> > + ttl |= 3 << 2; \
> > + break; \
> > + } \
> > + \
> > + arg &= ~TLBI_TTL_MASK; \
> > + arg |= FIELD_PREP(TLBI_TTL_MASK, ttl); \
>
> Despite the spec saying both tables apply to TLB maintenance
> instructions that do not apply to a range of addresses I think it only
> means the 4-bit version (bug report to Arm, or I'm on the wrong spec).
I'm not quite sure what you mean by the 4-bit version here. Do you mean
the instructions taking a VA or IPA as input address? In this case,
yes, this macro is solely for the use of the invalidation of a given
translation, identified by a VA/IPA and a level (which is an implicit
TLB size for a given translation granule).
And yes, it looks like there is a bad copy-paste bug in the ARM ARM
(Rev F.a).
> This is consistent with Table D5-53 and the macro takes a single address
> argument to make misuse with range based tlbi less likely.
>
> It relies on the caller to get the level right and getting it wrong
> could be pretty bad as the spec says all bets are off in that case. Is
> it worth adding a check of the level against the address (seems a bit
> involved), or that it is just 2 bits or adding a short doc comment to
> explain it?
I'd like to avoid checking code at that level, to be honest. If you are
writing TLB invalidation code, you'd better know what you are doing.
Happy to document it a bit more thoroughly though, and set the
expectations that if you misuse the level, you're in for a treat.
> (Looks like we get some constants for the levels in a later patch that
> could be referenced with some form of time travel)
I'm happy to bring these definitions forward, maybe in a more generic
form (they are very S2-specific at the moment).
>
> > + } \
> > + \
> > + __tlbi(op, arg); \
>
> cosmetic nit: double space in here
Well spotted.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
