Re: [Qemu-arm] [PATCH v8 16/25] cputlb and arm/sparc targets: convert mmuidx flushes from varg to bitmap

[Peter Maydell]

On 27 January 2017 at 10:34, Alex Bennée <address@hidden> wrote:
> While the vargs approach was flexible the original MTTCG ended up
> having munge the bits to a bitmap so the data could be used in
> deferred work helpers. Instead of hiding that in cputlb we push the
> change to the API to make it take a bitmap of MMU indexes instead.
>
> This change is fairly mechanical but as storing the actual index is
> useful for cases like the current running context. As a result the
> constants are renamed to ARMMMUBit_foo and a couple of helper
> functions added to convert between a single bit and a scalar index.
>
> Signed-off-by: Alex Bennée <address@hidden>

So, as discussed on IRC, I don't think this is really the right
way to go. An MMU index is an index, ie it refers to a particular
TLB. Almost all the time you don't want to think of it as a bit
value. The only place that does care about that is places in
the TLB code which want to say "do some operation on multiple
TLBs at once", which is basically just the tlb_flush_by_mmuidx()
function. So that function should have an API that lets you
specify multiple TLBs (either the one it has now, or maybe one
where you pass it in a uint32_t with bits set).

That avoids either all or almost all of this churn.

> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1954,27 +1954,40 @@ static inline bool arm_excp_unmasked(CPUState *cs, 
> unsigned int excp_idx,
>   * of the AT/ATS operations.
>   * The values used are carefully arranged to make mmu_idx => EL lookup easy.
>   */
> -typedef enum ARMMMUIdx {
> -    ARMMMUIdx_S12NSE0 = 0,
> -    ARMMMUIdx_S12NSE1 = 1,
> -    ARMMMUIdx_S1E2 = 2,
> -    ARMMMUIdx_S1E3 = 3,
> -    ARMMMUIdx_S1SE0 = 4,
> -    ARMMMUIdx_S1SE1 = 5,
> -    ARMMMUIdx_S2NS = 6,
> +typedef enum ARMMMUBitMap {
> +    ARMMMUBit_S12NSE0 = 1 << 0,
> +    ARMMMUBit_S12NSE1 = 1 << 1,
> +    ARMMMUBit_S1E2 = 1 << 2,
> +    ARMMMUBit_S1E3 = 1 << 3,
> +    ARMMMUBit_S1SE0 = 1 << 4,
> +    ARMMMUBit_S1SE1 = 1 << 5,
> +    ARMMMUBit_S2NS = 1 << 6,
>      /* Indexes below here don't have TLBs and are used only for AT system
>       * instructions or for the first stage of an S12 page table walk.
>       */
> -    ARMMMUIdx_S1NSE0 = 7,
> -    ARMMMUIdx_S1NSE1 = 8,
> -} ARMMMUIdx;
> +    ARMMMUBit_S1NSE0 = 1 << 7,
> +    ARMMMUBit_S1NSE1 = 1 << 8,
> +} ARMMMUBitMap;

In particular I'd be very wary of changing these values, because
as the comment says things are arranged carefully to make some
operations easy.

thanks
-- PMM