From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from zen.linaro.local ([81.128.185.34]) by smtp.gmail.com with ESMTPSA id t103sm32678618wrc.43.2017.01.31.23.42.34 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 31 Jan 2017 23:42:34 -0800 (PST) Received: from zen (localhost [127.0.0.1]) by zen.linaro.local (Postfix) with ESMTPS id 78E313E025C; Wed, 1 Feb 2017 07:42:36 +0000 (GMT) References: <20170127103922.19658-1-alex.bennee@linaro.org> <20170127103922.19658-17-alex.bennee@linaro.org> <70fe24bb-c8f1-43c4-010f-a682bdd507ef@twiddle.net> User-agent: mu4e 0.9.19; emacs 25.1.91.6 From: Alex =?utf-8?Q?Benn=C3=A9e?= To: Richard Henderson Cc: mttcg@greensocs.com, qemu-devel@nongnu.org, fred.konrad@greensocs.com, a.rigo@virtualopensystems.com, cota@braap.org, bobby.prani@gmail.com, nikunj@linux.vnet.ibm.com, mark.burton@greensocs.com, pbonzini@redhat.com, jan.kiszka@siemens.com, serge.fdrv@gmail.com, peter.maydell@linaro.org, claudio.fontana@huawei.com, bamvor.zhangjian@linaro.org, Peter Crosthwaite , Mark Cave-Ayland , Artyom Tarasenko , "open list\:ARM" Subject: Re: [PATCH v8 16/25] cputlb and arm/sparc targets: convert mmuidx flushes from varg to bitmap In-reply-to: <70fe24bb-c8f1-43c4-010f-a682bdd507ef@twiddle.net> Date: Wed, 01 Feb 2017 07:42:36 +0000 Message-ID: <87inou4as3.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-TUID: cwiFw/A43Icx Richard Henderson 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46769) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cYpYu-00010Y-Gc for qemu-devel@nongnu.org; Wed, 01 Feb 2017 02:42:41 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cYpYr-0000MV-Aw for qemu-devel@nongnu.org; Wed, 01 Feb 2017 02:42:40 -0500 Received: from mail-wm0-x236.google.com ([2a00:1450:400c:c09::236]:35282) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1cYpYr-0000Kq-1H for qemu-devel@nongnu.org; Wed, 01 Feb 2017 02:42:37 -0500 Received: by mail-wm0-x236.google.com with SMTP id b65so25520904wmf.0 for ; Tue, 31 Jan 2017 23:42:36 -0800 (PST) References: <20170127103922.19658-1-alex.bennee@linaro.org> <20170127103922.19658-17-alex.bennee@linaro.org> <70fe24bb-c8f1-43c4-010f-a682bdd507ef@twiddle.net> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: <70fe24bb-c8f1-43c4-010f-a682bdd507ef@twiddle.net> Date: Wed, 01 Feb 2017 07:42:36 +0000 Message-ID: <87inou4as3.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v8 16/25] cputlb and arm/sparc targets: convert mmuidx flushes from varg to bitmap List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Richard Henderson Cc: mttcg@greensocs.com, qemu-devel@nongnu.org, fred.konrad@greensocs.com, a.rigo@virtualopensystems.com, cota@braap.org, bobby.prani@gmail.com, nikunj@linux.vnet.ibm.com, mark.burton@greensocs.com, pbonzini@redhat.com, jan.kiszka@siemens.com, serge.fdrv@gmail.com, peter.maydell@linaro.org, claudio.fontana@huawei.com, bamvor.zhangjian@linaro.org, Peter Crosthwaite , Mark Cave-Ayland , Artyom Tarasenko , "open list:ARM" Richard Henderson 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