[Qemu-devel] [PATCH] target/arm: Allow to switch from MON->HYP on AArch32

[Qemu-devel] [PATCH] target/arm: Allow to switch from MON->HYP on AArch32

[Alexander Graf]

In U-boot, we switch from S-SVC -> MON -> HYP when we want to enter
HYP mode. This dance seems to work ok (hence it's there in the code
base), but breaks with current QEMU.

The reason seems to be that we try to see whether we are entering
HYP mode from the NS side. However, MON is always considered secure,
so we always fall into an error case when going MON->HYP and never
manage to actually do the switch.

Fix this by not using a different helper function that does not bork
when we're in MON state, as that switch is supposed to work.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 target/arm/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index f00c141ef9..9bf8fbd8f9 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6297,7 +6297,7 @@ static int bad_mode_switch(CPUARMState *env, int mode, CPSRWriteType write_type)
         return 0;
     case ARM_CPU_MODE_HYP:
         return !arm_feature(env, ARM_FEATURE_EL2)
-            || arm_current_el(env) < 2 || arm_is_secure(env);
+            || arm_current_el(env) < 2 || arm_is_secure_below_el3(env);
     case ARM_CPU_MODE_MON:
         return arm_current_el(env) < 3;
     default:
-- 
2.12.3

Re: [Qemu-devel] [PATCH] target/arm: Allow to switch from MON->HYP on AArch32

[Alex Bennée]

Alexander Graf <agraf@suse.de> writes:

> In U-boot, we switch from S-SVC -> MON -> HYP when we want to enter
> HYP mode. This dance seems to work ok (hence it's there in the code
> base), but breaks with current QEMU.

What EL is MON in this case? EL3? In which case I'm confused by the
terminology as the ARM ARM states:

  The principles of the ARMv8-A security model are:
  • If the implementation includes EL3, then it has two Security states, Secure and Non-secure, and:
    — EL3 exists only in Secure state.
    — A change from Non-secure state to Secure state can only occur on taking an exception to EL3.
    — A change from Secure state to Non-secure state can only occur on an exception return from EL3.

We don't currently implement ARMv8.4-SecEL2 but that adds secure EL2 but
as you can only switch security state in in/out of EL3 you have to be go
to the secure monitor before you enter it.

> The reason seems to be that we try to see whether we are entering
> HYP mode from the NS side. However, MON is always considered secure,
> so we always fall into an error case when going MON->HYP and never
> manage to actually do the switch.
>
> Fix this by not using a different helper function that does not bork
> when we're in MON state, as that switch is supposed to work.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  target/arm/helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index f00c141ef9..9bf8fbd8f9 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6297,7 +6297,7 @@ static int bad_mode_switch(CPUARMState *env, int mode, CPSRWriteType write_type)
>          return 0;
>      case ARM_CPU_MODE_HYP:
>          return !arm_feature(env, ARM_FEATURE_EL2)
> -            || arm_current_el(env) < 2 || arm_is_secure(env);
> +            || arm_current_el(env) < 2 ||
> arm_is_secure_below_el3(env);

This seems to violate the rule that you can enter a NS state from
anything other than EL3.

>      case ARM_CPU_MODE_MON:
>          return arm_current_el(env) < 3;
>      default:


--
Alex Bennée

Re: [Qemu-devel] [PATCH] target/arm: Allow to switch from MON->HYP on AArch32

[Alexander Graf]

On 01/09/2019 05:10 PM, Alex Bennée wrote:
> Alexander Graf <agraf@suse.de> writes:
>
>> In U-boot, we switch from S-SVC -> MON -> HYP when we want to enter
>> HYP mode. This dance seems to work ok (hence it's there in the code
>> base), but breaks with current QEMU.
> What EL is MON in this case? EL3? In which case I'm confused by the
> terminology as the ARM ARM states:
>
>    The principles of the ARMv8-A security model are:
>    • If the implementation includes EL3, then it has two Security states, Secure and Non-secure, and:
>      — EL3 exists only in Secure state.
>      — A change from Non-secure state to Secure state can only occur on taking an exception to EL3.
>      — A change from Secure state to Non-secure state can only occur on an exception return from EL3.
>
> We don't currently implement ARMv8.4-SecEL2 but that adds secure EL2 but
> as you can only switch security state in in/out of EL3 you have to be go
> to the secure monitor before you enter it.

Right - and exactly that switch seems to be prohibited here.

>
>> The reason seems to be that we try to see whether we are entering
>> HYP mode from the NS side. However, MON is always considered secure,
>> so we always fall into an error case when going MON->HYP and never
>> manage to actually do the switch.
>>
>> Fix this by not using a different helper function that does not bork
>> when we're in MON state, as that switch is supposed to work.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>>   target/arm/helper.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index f00c141ef9..9bf8fbd8f9 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -6297,7 +6297,7 @@ static int bad_mode_switch(CPUARMState *env, int mode, CPSRWriteType write_type)
>>           return 0;
>>       case ARM_CPU_MODE_HYP:
>>           return !arm_feature(env, ARM_FEATURE_EL2)
>> -            || arm_current_el(env) < 2 || arm_is_secure(env);
>> +            || arm_current_el(env) < 2 ||
>> arm_is_secure_below_el3(env);
> This seems to violate the rule that you can enter a NS state from
> anything other than EL3.

How so? This statement basically says "Only return to HYP from NS-HYP or 
EL3" now. Before it said "Only return to HYP from HYP".

Alex

>
>>       case ARM_CPU_MODE_MON:
>>           return arm_current_el(env) < 3;
>>       default:
>
> --
> Alex Bennée
>

Re: [Qemu-devel] [Qemu-arm] [PATCH] target/arm: Allow to switch from MON->HYP on AArch32

[Peter Maydell]

On Wed, 9 Jan 2019 at 16:11, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Alexander Graf <agraf@suse.de> writes:
>
> > In U-boot, we switch from S-SVC -> MON -> HYP when we want to enter
> > HYP mode. This dance seems to work ok (hence it's there in the code
> > base), but breaks with current QEMU.
>
> What EL is MON in this case? EL3?

Mon is an AArch32 term and EL is really AArch64, but in
practice Mon is always (Secure) EL3. Note that if EL3
is AArch32 then S-SVC is *also* EL3 !

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target/arm: Allow to switch from MON->HYP on AArch32

[Peter Maydell]

On Wed, 9 Jan 2019 at 15:26, Alexander Graf <agraf@suse.de> wrote:
>
> In U-boot, we switch from S-SVC -> MON -> HYP when we want to enter
> HYP mode. This dance seems to work ok (hence it's there in the code
> base), but breaks with current QEMU.
>
> The reason seems to be that we try to see whether we are entering
> HYP mode from the NS side. However, MON is always considered secure,
> so we always fall into an error case when going MON->HYP and never
> manage to actually do the switch.
>
> Fix this by not using a different helper function that does not bork
> when we're in MON state, as that switch is supposed to work.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  target/arm/helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index f00c141ef9..9bf8fbd8f9 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6297,7 +6297,7 @@ static int bad_mode_switch(CPUARMState *env, int mode, CPSRWriteType write_type)
>          return 0;
>      case ARM_CPU_MODE_HYP:
>          return !arm_feature(env, ARM_FEATURE_EL2)
> -            || arm_current_el(env) < 2 || arm_is_secure(env);
> +            || arm_current_el(env) < 2 || arm_is_secure_below_el3(env);
>      case ARM_CPU_MODE_MON:
>          return arm_current_el(env) < 3;
>      default:
> --

This took me a while to figure out, because the
bad_mode_switch() code is called for doing checks
in two situations:
 (1) changes to mode by instructions writing to
CPSR.M (ie not exception take/return) -- this
corresponds to the Armv8 Arm ARM pseudocode
AArch32.WriteModeByInstr, and using this to enter or
leave Hyp mode is forbidden in v8 and UNPREDICTABLE in v7
 (2) changes to mode by exception return

Changing to or from Hyp by writing CPSR.M is
forbidden, but we catch that already at the top
of the bad_mode_switch() function. So what we
need to consider here is the exception-return case.
An exception return from Mon to Hyp with the NS bit 0
is OK. So this change is the right thing.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target/arm: Allow to switch from MON->HYP on AArch32

[Peter Maydell]

On Wed, 9 Jan 2019 at 16:52, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 9 Jan 2019 at 15:26, Alexander Graf <agraf@suse.de> wrote:
> >
> > In U-boot, we switch from S-SVC -> MON -> HYP when we want to enter
> > HYP mode. This dance seems to work ok (hence it's there in the code
> > base), but breaks with current QEMU.

PS: it would be helpful if the commit message said how u-boot
is trying to go from Mon to Hyp -- some ways to try to do
this are OK, and some are not, so whether it's supposed to
work or not depends on what u-boot is actually doing...

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target/arm: Allow to switch from MON->HYP on AArch32

[Alexander Graf]

On 01/09/2019 05:59 PM, Peter Maydell wrote:
> On Wed, 9 Jan 2019 at 16:52, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On Wed, 9 Jan 2019 at 15:26, Alexander Graf <agraf@suse.de> wrote:
>>> In U-boot, we switch from S-SVC -> MON -> HYP when we want to enter
>>> HYP mode. This dance seems to work ok (hence it's there in the code
>>> base), but breaks with current QEMU.
> PS: it would be helpful if the commit message said how u-boot
> is trying to go from Mon to Hyp -- some ways to try to do
> this are OK, and some are not, so whether it's supposed to
> work or not depends on what u-boot is actually doing...

I don't fully understand all of it to be honest :). But the code is here:

http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/cpu/armv7/nonsec_virt.S

What I managed to understand so far is that it goes to MON using the smc 
#0 call and then changes SPSR so that on return (movs pc) the mode will 
be different.

But please double check whether this is supposed to work - I don't find 
mode switching on ARM terribly intuitive ;).


Alex

Re: [Qemu-devel] [PATCH] target/arm: Allow to switch from MON->HYP on AArch32

[Peter Maydell]

On Wed, 9 Jan 2019 at 17:14, Alexander Graf <agraf@suse.de> wrote:
>
> On 01/09/2019 05:59 PM, Peter Maydell wrote:
> > On Wed, 9 Jan 2019 at 16:52, Peter Maydell <peter.maydell@linaro.org> wrote:
> >> On Wed, 9 Jan 2019 at 15:26, Alexander Graf <agraf@suse.de> wrote:
> >>> In U-boot, we switch from S-SVC -> MON -> HYP when we want to enter
> >>> HYP mode. This dance seems to work ok (hence it's there in the code
> >>> base), but breaks with current QEMU.
> > PS: it would be helpful if the commit message said how u-boot
> > is trying to go from Mon to Hyp -- some ways to try to do
> > this are OK, and some are not, so whether it's supposed to
> > work or not depends on what u-boot is actually doing...
>
> I don't fully understand all of it to be honest :). But the code is here:
>
> http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/cpu/armv7/nonsec_virt.S
>
> What I managed to understand so far is that it goes to MON using the smc
> #0 call and then changes SPSR so that on return (movs pc) the mode will
> be different.

Thanks -- yes, that's an exception return so it's the
expected way to go from Mon to Hyp.

-- PMM

Re: [Qemu-devel] [PATCH] target/arm: Allow to switch from MON->HYP on AArch32

[Alexander Graf]

On 10.01.19 00:08, Peter Maydell wrote:
> On Wed, 9 Jan 2019 at 17:14, Alexander Graf <agraf@suse.de> wrote:
>>
>> On 01/09/2019 05:59 PM, Peter Maydell wrote:
>>> On Wed, 9 Jan 2019 at 16:52, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>> On Wed, 9 Jan 2019 at 15:26, Alexander Graf <agraf@suse.de> wrote:
>>>>> In U-boot, we switch from S-SVC -> MON -> HYP when we want to enter
>>>>> HYP mode. This dance seems to work ok (hence it's there in the code
>>>>> base), but breaks with current QEMU.
>>> PS: it would be helpful if the commit message said how u-boot
>>> is trying to go from Mon to Hyp -- some ways to try to do
>>> this are OK, and some are not, so whether it's supposed to
>>> work or not depends on what u-boot is actually doing...
>>
>> I don't fully understand all of it to be honest :). But the code is here:
>>
>> http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/cpu/armv7/nonsec_virt.S
>>
>> What I managed to understand so far is that it goes to MON using the smc
>> #0 call and then changes SPSR so that on return (movs pc) the mode will
>> be different.
> 
> Thanks -- yes, that's an exception return so it's the
> expected way to go from Mon to Hyp.

That was my understanding, yes. Do you still want me to change the
commit message to mention that or will you just do it when applying?

Thanks,

Alex

Re: [Qemu-devel] [PATCH] target/arm: Allow to switch from MON->HYP on AArch32

[Peter Maydell]

On Thu, 10 Jan 2019 at 06:43, Alexander Graf <agraf@suse.de> wrote:
>
>
>
> On 10.01.19 00:08, Peter Maydell wrote:
> > On Wed, 9 Jan 2019 at 17:14, Alexander Graf <agraf@suse.de> wrote:
> >>
> >> On 01/09/2019 05:59 PM, Peter Maydell wrote:
> >>> On Wed, 9 Jan 2019 at 16:52, Peter Maydell <peter.maydell@linaro.org> wrote:
> >>>> On Wed, 9 Jan 2019 at 15:26, Alexander Graf <agraf@suse.de> wrote:
> >>>>> In U-boot, we switch from S-SVC -> MON -> HYP when we want to enter
> >>>>> HYP mode. This dance seems to work ok (hence it's there in the code
> >>>>> base), but breaks with current QEMU.
> >>> PS: it would be helpful if the commit message said how u-boot
> >>> is trying to go from Mon to Hyp -- some ways to try to do
> >>> this are OK, and some are not, so whether it's supposed to
> >>> work or not depends on what u-boot is actually doing...
> >>
> >> I don't fully understand all of it to be honest :). But the code is here:
> >>
> >> http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/cpu/armv7/nonsec_virt.S
> >>
> >> What I managed to understand so far is that it goes to MON using the smc
> >> #0 call and then changes SPSR so that on return (movs pc) the mode will
> >> be different.
> >
> > Thanks -- yes, that's an exception return so it's the
> > expected way to go from Mon to Hyp.
>
> That was my understanding, yes. Do you still want me to change the
> commit message to mention that or will you just do it when applying?

I'll add a note when I apply it; thanks.

-- PMM

Re: [Qemu-devel] [PATCH] target/arm: Allow to switch from MON->HYP on AArch32

[Peter Maydell]

On Thu, 10 Jan 2019 at 10:34, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 10 Jan 2019 at 06:43, Alexander Graf <agraf@suse.de> wrote:
> >
> >
> >
> > On 10.01.19 00:08, Peter Maydell wrote:
> > > On Wed, 9 Jan 2019 at 17:14, Alexander Graf <agraf@suse.de> wrote:
> > >>
> > >> On 01/09/2019 05:59 PM, Peter Maydell wrote:
> > >>> On Wed, 9 Jan 2019 at 16:52, Peter Maydell <peter.maydell@linaro.org> wrote:
> > >>>> On Wed, 9 Jan 2019 at 15:26, Alexander Graf <agraf@suse.de> wrote:
> > >>>>> In U-boot, we switch from S-SVC -> MON -> HYP when we want to enter
> > >>>>> HYP mode. This dance seems to work ok (hence it's there in the code
> > >>>>> base), but breaks with current QEMU.
> > >>> PS: it would be helpful if the commit message said how u-boot
> > >>> is trying to go from Mon to Hyp -- some ways to try to do
> > >>> this are OK, and some are not, so whether it's supposed to
> > >>> work or not depends on what u-boot is actually doing...
> > >>
> > >> I don't fully understand all of it to be honest :). But the code is here:
> > >>
> > >> http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/cpu/armv7/nonsec_virt.S
> > >>
> > >> What I managed to understand so far is that it goes to MON using the smc
> > >> #0 call and then changes SPSR so that on return (movs pc) the mode will
> > >> be different.
> > >
> > > Thanks -- yes, that's an exception return so it's the
> > > expected way to go from Mon to Hyp.
> >
> > That was my understanding, yes. Do you still want me to change the
> > commit message to mention that or will you just do it when applying?
>
> I'll add a note when I apply it; thanks.

Now applied to target-arm.next with a rewritten commit message; thanks.

-- PMM