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

[Alex Bennée]

Richard Henderson <address@hidden> writes:

> On 01/27/2017 02:39 AM, Alex Bennée wrote:
>> +    for (mmu_idx = 0; mmu_idx < NB_MMU_MODES; mmu_idx++) {
>>
>> -        tlb_debug("%d\n", mmu_idx);
>> +        if (test_bit(mmu_idx, &mmu_idx_bitmask)) {
>> +            tlb_debug("%d\n", mmu_idx);
>>
>> -        memset(env->tlb_table[mmu_idx], -1, sizeof(env->tlb_table[0]));
>> -        memset(env->tlb_v_table[mmu_idx], -1, sizeof(env->tlb_v_table[0]));
>> +            memset(env->tlb_table[mmu_idx], -1, sizeof(env->tlb_table[0]));
>> +            memset(env->tlb_v_table[mmu_idx], -1, 
>> sizeof(env->tlb_v_table[0]));
>> +        }
>
> Perhaps it doesn't matter since NB_MMU_MODES is so small but
>
>    for (; idxmap != 0; idxmap &= idxmap - 1) {
>      int mmu_idx = ctz32(idxmap);
>      ...
>    }
>
> iterates only for the bits that are set.
>
>> -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;
>>
>> -#define MMU_USER_IDX 0
>> +typedef int ARMMMUIdx;
>> +
>> +static inline ARMMMUIdx arm_mmu_bit_to_idx(ARMMMUBitMap bit)
>> +{
>> +    g_assert(ctpop16(bit) == 1);
>> +    return ctz32(bit);
>> +}
>> +
>> +static inline ARMMMUBitMap arm_mmu_idx_to_bit(ARMMMUIdx idx)
>> +{
>> +    return 1 << idx;
>> +}
>
> I don't understand this redefinition though, causing...
>
>> @@ -2109,13 +2109,13 @@ static void ats_write(CPUARMState *env, const 
>> ARMCPRegInfo *ri, uint64_t value)
>>          /* stage 1 current state PL1: ATS1CPR, ATS1CPW */
>>          switch (el) {
>>          case 3:
>> -            mmu_idx = ARMMMUIdx_S1E3;
>> +            mmu_bit = ARMMMUBit_S1E3;
>>              break;
>>          case 2:
>> -            mmu_idx = ARMMMUIdx_S1NSE1;
>> +            mmu_bit = ARMMMUBit_S1NSE1;
>>              break;
>>          case 1:
>> -            mmu_idx = secure ? ARMMMUIdx_S1SE1 : ARMMMUIdx_S1NSE1;
>> +            mmu_bit = secure ? ARMMMUBit_S1SE1 : ARMMMUBit_S1NSE1;
>>              break;
>>          default:
>>              g_assert_not_reached();
>> @@ -2125,13 +2125,13 @@ static void ats_write(CPUARMState *env, const 
>> ARMCPRegInfo *ri, uint64_t value)
>>          /* stage 1 current state PL0: ATS1CUR, ATS1CUW */
>>          switch (el) {
>>          case 3:
>> -            mmu_idx = ARMMMUIdx_S1SE0;
>> +            mmu_bit = ARMMMUBit_S1SE0;
>>              break;
>>          case 2:
>> -            mmu_idx = ARMMMUIdx_S1NSE0;
>> +            mmu_bit = ARMMMUBit_S1NSE0;
>>              break;
>>          case 1:
>> -            mmu_idx = secure ? ARMMMUIdx_S1SE0 : ARMMMUIdx_S1NSE0;
>> +            mmu_bit = secure ? ARMMMUBit_S1SE0 : ARMMMUBit_S1NSE0;
>>              break;
>>          default:
>>              g_assert_not_reached();
>> @@ -2139,17 +2139,17 @@ static void ats_write(CPUARMState *env, const 
>> ARMCPRegInfo *ri, uint64_t value)
>>          break;
>>      case 4:
>>          /* stage 1+2 NonSecure PL1: ATS12NSOPR, ATS12NSOPW */
>> -        mmu_idx = ARMMMUIdx_S12NSE1;
>> +        mmu_bit = ARMMMUBit_S12NSE1;
>>          break;
>>      case 6:
>>          /* stage 1+2 NonSecure PL0: ATS12NSOUR, ATS12NSOUW */
>> -        mmu_idx = ARMMMUIdx_S12NSE0;
>> +        mmu_bit = ARMMMUBit_S12NSE0;
>>          break;
>>      default:
>>          g_assert_not_reached();
>>      }
>>
>> -    par64 = do_ats_write(env, value, access_type, mmu_idx);
>> +    par64 = do_ats_write(env, value, access_type, 
>> arm_mmu_bit_to_idx(mmu_bit));
>
> ... this sort of churn, only to convert *back* to an index.

Yeah I've dropped this is v9, ARM now justs does 1 << ARMMMUIdx_foo at
the tlb_flush call sites.

>
>> @@ -2185,26 +2186,26 @@ static void ats_write64(CPUARMState *env, const 
>> ARMCPRegInfo *ri,
>>      case 0:
>>          switch (ri->opc1) {
>>          case 0: /* AT S1E1R, AT S1E1W */
>> -            mmu_idx = secure ? ARMMMUIdx_S1SE1 : ARMMMUIdx_S1NSE1;
>> +            mmu_idx = secure ? ARMMMUBit_S1SE1 : ARMMMUBit_S1NSE1;
>>              break;
>>          case 4: /* AT S1E2R, AT S1E2W */
>> -            mmu_idx = ARMMMUIdx_S1E2;
>> +            mmu_idx = ARMMMUBit_S1E2;
>>              break;
>>          case 6: /* AT S1E3R, AT S1E3W */
>> -            mmu_idx = ARMMMUIdx_S1E3;
>> +            mmu_idx = ARMMMUBit_S1E3;
>>              break;
>>          default:
>>              g_assert_not_reached();
>>          }
>>          break;
>>      case 2: /* AT S1E0R, AT S1E0W */
>> -        mmu_idx = secure ? ARMMMUIdx_S1SE0 : ARMMMUIdx_S1NSE0;
>> +        mmu_idx = secure ? ARMMMUBit_S1SE0 : ARMMMUBit_S1NSE0;
>>          break;
>>      case 4: /* AT S12E1R, AT S12E1W */
>> -        mmu_idx = secure ? ARMMMUIdx_S1SE1 : ARMMMUIdx_S12NSE1;
>> +        mmu_idx = secure ? ARMMMUBit_S1SE1 : ARMMMUBit_S12NSE1;
>>          break;
>>      case 6: /* AT S12E0R, AT S12E0W */
>> -        mmu_idx = secure ? ARMMMUIdx_S1SE0 : ARMMMUIdx_S12NSE0;
>> +        mmu_idx = secure ? ARMMMUBit_S1SE0 : ARMMMUBit_S12NSE0;
>>          break;
>>      default:
>>          g_assert_not_reached();
>> @@ -2499,8 +2500,8 @@ static void vttbr_write(CPUARMState *env, const 
>> ARMCPRegInfo *ri,
>
> ... and then there's this one where you don't rename the variable, and as far
> as I can tell don't adjust the argument for do_ats_write.
>
> Which is probably a mistake?
>
> Just define both Idx and Bit symbols and be done with it.
>
>
> r~


--
Alex Bennée