From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36735) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YGC8l-0002pG-2K for qemu-devel@nongnu.org; Tue, 27 Jan 2015 14:49:36 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YGC8h-000355-Rc for qemu-devel@nongnu.org; Tue, 27 Jan 2015 14:49:34 -0500 Received: from mail-qg0-f51.google.com ([209.85.192.51]:54947) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YGC8h-00034z-Nw for qemu-devel@nongnu.org; Tue, 27 Jan 2015 14:49:31 -0500 Received: by mail-qg0-f51.google.com with SMTP id z107so13357474qgd.10 for ; Tue, 27 Jan 2015 11:49:31 -0800 (PST) MIME-Version: 1.0 In-Reply-To: References: <1422037228-5363-1-git-send-email-peter.maydell@linaro.org> <1422037228-5363-10-git-send-email-peter.maydell@linaro.org> Date: Tue, 27 Jan 2015 13:49:31 -0600 Message-ID: From: Greg Bellows Content-Type: multipart/alternative; boundary=001a113a5198ad9592050da78d41 Subject: Re: [Qemu-devel] [PATCH 09/11] target-arm: Use mmu_idx in get_phys_addr() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: "Edgar E. Iglesias" , Andrew Jones , =?UTF-8?B?QWxleCBCZW5uw6ll?= , QEMU Developers , Patch Tracking --001a113a5198ad9592050da78d41 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On Tue, Jan 27, 2015 at 12:12 PM, Peter Maydell wrote: > On 27 January 2015 at 17:57, Greg Bellows wrote= : > > > > > > On Fri, Jan 23, 2015 at 12:20 PM, Peter Maydell < > peter.maydell@linaro.org> > > wrote: > >> +/* Return the exception level which controls this address translation > >> regime */ > >> +static inline uint32_t regime_el(CPUARMState *env, ARMMMUIdx mmu_idx) > >> +{ > >> + switch (mmu_idx) { > >> + case ARMMMUIdx_S2NS: > >> + case ARMMMUIdx_S1E2: > >> + return 2; > >> + case ARMMMUIdx_S1E3: > >> + return 3; > >> + case ARMMMUIdx_S1SE0: > >> + return arm_el_is_aa64(env, 3) ? 1 : 3; > >> + case ARMMMUIdx_S1SE1: > > > > > > I think this should be handled the same way as S1SE0 as Secure EL1 maps > to > > EL3 when EL3 is AArch32. > > If EL3 is AArch32 then you'll never be using this MMU index. > By definition the S1SE1 index is for code executing at > Secure EL1, and there isn't any of that unless EL3 is 64 bit. > (Secure EL1 doesn't "map to" anything, it just doesn't > exist/have any code running in it.) > =E2=80=8BAh yes and thanks for the pointer to the origin of mmu index I hav= e now connected where we prevent that index with this code. > > >> + case ARMMMUIdx_S1NSE0: > >> + case ARMMMUIdx_S1NSE1: > >> + return 1; > >> + default: > >> + g_assert_not_reached(); > >> + } > >> +} > >> + > >> +/* Return the SCTLR value which controls this address translation > regime > >> */ > >> +static inline uint32_t regime_sctlr(CPUARMState *env, ARMMMUIdx > mmu_idx) > >> +{ > >> + return env->cp15.sctlr_el[regime_el(env, mmu_idx)]; > > > > > > Given the above regime_el(), for S1SE1, this would return the non-secur= e > > SCTLR bank on a secure translation. Same below for TCR and all uses > > thereafter. > > That's correct, because S1SE1 implies "secure EL1 under a 64 bit > EL3", in which case there is no system register banking and > both Secure and NonSecure use the same underlying register. > Compare the way that A32_BANKED_CURRENT_REG_GET/SET always > use the NS version if arm_el_is_aa64(env, 3). > > >> +static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx= ) > >> +{ > >> + switch (mmu_idx) { > >> + case ARMMMUIdx_S1SE0: > >> + case ARMMMUIdx_S1NSE0: > > > > > > The get_phys_addr path filters out ARMMMUIdx_S12NSE0 so calls to this > > function in this context don't matter, but what if it is called outside > this > > context? How should it handle this index? > > g_assert_not_reached(), but it didn't seem worth cluttering > the switch with a bunch of extra labels just to assert that > they weren't reachable. > > =E2=80=8BI see how it could clutter things, but given that the routine is g= eneric we probably should just like we do in regime_el().=E2=80=8B > thanks > -- PMM > --001a113a5198ad9592050da78d41 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


On Tue, Jan 27, 2015 at 12:12 PM, Peter Maydell <peter= .maydell@linaro.org> wrote:
On 27 January 2015 at 17:57, Greg Bellows <greg.bellows@linaro.org> wrote:
>
>
> On Fri, Jan 23, 2015 at 12:20 PM, Peter Maydell <peter.maydell@linaro.org>
> wrote:
>> +/* Return the exception level which contr= ols this address translation
>> regime */
>> +static inline uint32_t regime_el(CPUARMState *env, ARMMMUIdx mmu_= idx)
>> +{
>> +=C2=A0 =C2=A0 switch (mmu_idx) {
>> +=C2=A0 =C2=A0 case ARMMMUIdx_S2NS:
>> +=C2=A0 =C2=A0 case ARMMMUIdx_S1E2:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return 2;
>> +=C2=A0 =C2=A0 case ARMMMUIdx_S1E3:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return 3;
>> +=C2=A0 =C2=A0 case ARMMMUIdx_S1SE0:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return arm_el_is_aa64(env, 3) ? 1 : 3= ;
>> +=C2=A0 =C2=A0 case ARMMMUIdx_S1SE1:
>
>
> I think this should be handled the same way as S1SE0 as Secure EL1 map= s to
> EL3 when EL3 is AArch32.

If EL3 is AArch32 then you'll never be using this MMU index.
By definition the S1SE1 index is for code executing at
Secure EL1, and there isn't any of that unless EL3 is 64 bit.
(Secure EL1 doesn't "map to" anything, it just doesn't exist/have any code running in it.)

= =E2=80=8BAh yes and thanks for the pointer to the origin of mmu index I hav= e now connected where we prevent that index with this code.
=C2=A0

>> +=C2=A0 =C2=A0 case ARMMMUIdx_S1NSE0:
>> +=C2=A0 =C2=A0 case ARMMMUIdx_S1NSE1:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return 1;
>> +=C2=A0 =C2=A0 default:
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 g_assert_not_reached();
>> +=C2=A0 =C2=A0 }
>> +}
>> +
>> +/* Return the SCTLR value which controls this address translation= regime
>> */
>> +static inline uint32_t regime_sctlr(CPUARMState *env, ARMMMUIdx m= mu_idx)
>> +{
>> +=C2=A0 =C2=A0 return env->cp15.sctlr_el[regime_el(env, mmu_idx= )];
>
>
> Given the above regime_el(), for S1SE1, this would return the non-secu= re
> SCTLR bank on a secure translation.=C2=A0 Same below for TCR and all u= ses
> thereafter.

That's correct, because S1SE1 implies "secure EL1 under a 6= 4 bit
EL3", in which case there is no system register banking and
both Secure and NonSecure use the same underlying register.
Compare the way that A32_BANKED_CURRENT_REG_GET/SET always
use the NS version if arm_el_is_aa64(env, 3).

>> +static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu= _idx)
>> +{
>> +=C2=A0 =C2=A0 switch (mmu_idx) {
>> +=C2=A0 =C2=A0 case ARMMMUIdx_S1SE0:
>> +=C2=A0 =C2=A0 case ARMMMUIdx_S1NSE0:
>
>
> The get_phys_addr path filters out ARMMMUIdx_S12NSE0 so calls to this<= br> > function in this context don't matter, but what if it is called ou= tside this
> context?=C2=A0 How should it handle this index?

g_assert_not_reached(),=C2=A0 but it didn't seem worth clutterin= g
the switch with a bunch of extra labels just to assert that
they weren't reachable.


=E2=80=8BI see how it could clutter= things, but given that the routine is generic we probably should just like= we do in regime_el().=E2=80=8B

=C2=A0
thanks
-- PMM

--001a113a5198ad9592050da78d41--