[PATCH] kvm: arm: Enforce some NS-SVC initialisation

[PATCH] kvm: arm: Enforce some NS-SVC initialisation

[Robin Murphy]

Since the non-secure copies of banked registers lack architecturally
defined reset values, there is no actual guarantee when entering in Hyp
from secure-only firmware that the non-secure PL1 state will look the
way that kernel entry (in particular the decompressor stub) expects.
So far, we've been getting away with it thanks to implementation details
of ARMv7 cores and/or bootloader behaviour, but for the sake of forwards
compatibility let's try to ensure that we have a minimally sane state
before dropping down into it.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 arch/arm/kernel/hyp-stub.S | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm/kernel/hyp-stub.S b/arch/arm/kernel/hyp-stub.S
index 0b1e4a93d67e..7de3fe15ab21 100644
--- a/arch/arm/kernel/hyp-stub.S
+++ b/arch/arm/kernel/hyp-stub.S
@@ -142,6 +142,18 @@ ARM_BE8(orr	r7, r7, #(1 << 25))     @ HSCTLR.EE
 	and	r7, #0x1f		@ Preserve HPMN
 	mcr	p15, 4, r7, c1, c1, 1	@ HDCR
 
+	@ Make sure NS-SVC is initialised appropriately
+	mrc	p15, 0, r7, c1, c0, 0	@ SCTLR
+	orr	r7, #(1 << 5)		@ CP15 barriers enabled
+	bic	r7, #(3 << 19)		@ WXN and UWXN disabled
+	mcr	p15, 0, r7, c1, c0, 0	@ SCTLR
+
+	mrc	p15, 0, r7, c0, c0, 0	@ MIDR
+	mcr	p15, 4, r7, c0, c0, 0	@ VPIDR
+
+	mrc	p15, 0, r7, c0, c0, 5	@ MPIDR
+	mcr	p15, 4, r7, c0, c0, 5	@ VMPIDR
+
 #if !defined(ZIMAGE) && defined(CONFIG_ARM_ARCH_TIMER)
 	@ make CNTP_* and CNTPCT accessible from PL1
 	mrc	p15, 0, r7, c0, c1, 1	@ ID_PFR1
-- 
2.8.1.dirty

[PATCH] kvm: arm: Enforce some NS-SVC initialisation

[Marc Zyngier]

On 21/07/16 13:01, Robin Murphy wrote:
> Since the non-secure copies of banked registers lack architecturally
> defined reset values, there is no actual guarantee when entering in Hyp
> from secure-only firmware that the non-secure PL1 state will look the
> way that kernel entry (in particular the decompressor stub) expects.
> So far, we've been getting away with it thanks to implementation details
> of ARMv7 cores and/or bootloader behaviour, but for the sake of forwards
> compatibility let's try to ensure that we have a minimally sane state
> before dropping down into it.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  arch/arm/kernel/hyp-stub.S | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/arch/arm/kernel/hyp-stub.S b/arch/arm/kernel/hyp-stub.S
> index 0b1e4a93d67e..7de3fe15ab21 100644
> --- a/arch/arm/kernel/hyp-stub.S
> +++ b/arch/arm/kernel/hyp-stub.S
> @@ -142,6 +142,18 @@ ARM_BE8(orr	r7, r7, #(1 << 25))     @ HSCTLR.EE
>  	and	r7, #0x1f		@ Preserve HPMN
>  	mcr	p15, 4, r7, c1, c1, 1	@ HDCR
>  
> +	@ Make sure NS-SVC is initialised appropriately
> +	mrc	p15, 0, r7, c1, c0, 0	@ SCTLR
> +	orr	r7, #(1 << 5)		@ CP15 barriers enabled
> +	bic	r7, #(3 << 19)		@ WXN and UWXN disabled

I think that while you're doing this, you also may want to clear SED and
ITD so that a BE kernel has a chance to survive its first instruction
(assuming it it uses the decompressor...).

> +	mcr	p15, 0, r7, c1, c0, 0	@ SCTLR
> +
> +	mrc	p15, 0, r7, c0, c0, 0	@ MIDR
> +	mcr	p15, 4, r7, c0, c0, 0	@ VPIDR
> +
> +	mrc	p15, 0, r7, c0, c0, 5	@ MPIDR
> +	mcr	p15, 4, r7, c0, c0, 5	@ VMPIDR
> +
>  #if !defined(ZIMAGE) && defined(CONFIG_ARM_ARCH_TIMER)
>  	@ make CNTP_* and CNTPCT accessible from PL1
>  	mrc	p15, 0, r7, c0, c1, 1	@ ID_PFR1
> 

Otherwise looks good.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH] kvm: arm: Enforce some NS-SVC initialisation

[Robin Murphy]

Hi Marc,

On 16/08/16 14:33, Marc Zyngier wrote:
> On 21/07/16 13:01, Robin Murphy wrote:
>> Since the non-secure copies of banked registers lack architecturally
>> defined reset values, there is no actual guarantee when entering in Hyp
>> from secure-only firmware that the non-secure PL1 state will look the
>> way that kernel entry (in particular the decompressor stub) expects.
>> So far, we've been getting away with it thanks to implementation details
>> of ARMv7 cores and/or bootloader behaviour, but for the sake of forwards
>> compatibility let's try to ensure that we have a minimally sane state
>> before dropping down into it.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>  arch/arm/kernel/hyp-stub.S | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/arch/arm/kernel/hyp-stub.S b/arch/arm/kernel/hyp-stub.S
>> index 0b1e4a93d67e..7de3fe15ab21 100644
>> --- a/arch/arm/kernel/hyp-stub.S
>> +++ b/arch/arm/kernel/hyp-stub.S
>> @@ -142,6 +142,18 @@ ARM_BE8(orr	r7, r7, #(1 << 25))     @ HSCTLR.EE
>>  	and	r7, #0x1f		@ Preserve HPMN
>>  	mcr	p15, 4, r7, c1, c1, 1	@ HDCR
>>  
>> +	@ Make sure NS-SVC is initialised appropriately
>> +	mrc	p15, 0, r7, c1, c0, 0	@ SCTLR
>> +	orr	r7, #(1 << 5)		@ CP15 barriers enabled
>> +	bic	r7, #(3 << 19)		@ WXN and UWXN disabled
> 
> I think that while you're doing this, you also may want to clear SED and
> ITD so that a BE kernel has a chance to survive its first instruction
> (assuming it it uses the decompressor...).

Good point; I wrote this from the v7 perspective and neglected those,
and I think I was actually trying to achieve something useful at the
time which precluded cracking out the big-endian Thumb-2 kernel ;)

>From a quick correlation between ARM ARMs, those bits should be reliably
safe to unconditionally clear on v7VE, so let's do it. I'll respin shortly.

>> +	mcr	p15, 0, r7, c1, c0, 0	@ SCTLR
>> +
>> +	mrc	p15, 0, r7, c0, c0, 0	@ MIDR
>> +	mcr	p15, 4, r7, c0, c0, 0	@ VPIDR
>> +
>> +	mrc	p15, 0, r7, c0, c0, 5	@ MPIDR
>> +	mcr	p15, 4, r7, c0, c0, 5	@ VMPIDR
>> +
>>  #if !defined(ZIMAGE) && defined(CONFIG_ARM_ARCH_TIMER)
>>  	@ make CNTP_* and CNTPCT accessible from PL1
>>  	mrc	p15, 0, r7, c0, c1, 1	@ ID_PFR1
>>
> 
> Otherwise looks good.

Cheers,
Robin.

> 
> Thanks,
> 
> 	M.
> 

[PATCH v2] kvm: arm: Enforce some NS-SVC initialisation

[Robin Murphy]

Since the non-secure copies of banked registers lack architecturally
defined reset values, there is no actual guarantee when entering in Hyp
from secure-only firmware that the Non-Secure PL1 state will look the
way that kernel entry (in particular the decompressor stub) expects.
So far, we've been getting away with it thanks to implementation details
of ARMv7 cores and/or bootloader behaviour, but for the sake of forwards
compatibility let's try to ensure that we have a minimally sane state
before dropping down into it.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v2: Initialise SED/ITD to safe values as well.

 arch/arm/kernel/hyp-stub.S | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/arm/kernel/hyp-stub.S b/arch/arm/kernel/hyp-stub.S
index 0b1e4a93d67e..15d073ae5da2 100644
--- a/arch/arm/kernel/hyp-stub.S
+++ b/arch/arm/kernel/hyp-stub.S
@@ -142,6 +142,19 @@ ARM_BE8(orr	r7, r7, #(1 << 25))     @ HSCTLR.EE
 	and	r7, #0x1f		@ Preserve HPMN
 	mcr	p15, 4, r7, c1, c1, 1	@ HDCR
 
+	@ Make sure NS-SVC is initialised appropriately
+	mrc	p15, 0, r7, c1, c0, 0	@ SCTLR
+	orr	r7, #(1 << 5)		@ CP15 barriers enabled
+	bic	r7, #(3 << 7)		@ Clear SED/ITD for v8 (RES0 for v7)
+	bic	r7, #(3 << 19)		@ WXN and UWXN disabled
+	mcr	p15, 0, r7, c1, c0, 0	@ SCTLR
+
+	mrc	p15, 0, r7, c0, c0, 0	@ MIDR
+	mcr	p15, 4, r7, c0, c0, 0	@ VPIDR
+
+	mrc	p15, 0, r7, c0, c0, 5	@ MPIDR
+	mcr	p15, 4, r7, c0, c0, 5	@ VMPIDR
+
 #if !defined(ZIMAGE) && defined(CONFIG_ARM_ARCH_TIMER)
 	@ make CNTP_* and CNTPCT accessible from PL1
 	mrc	p15, 0, r7, c0, c1, 1	@ ID_PFR1
-- 
2.8.1.dirty

[PATCH v2] kvm: arm: Enforce some NS-SVC initialisation

[Marc Zyngier]

On 16/08/16 18:49, Robin Murphy wrote:
> Since the non-secure copies of banked registers lack architecturally
> defined reset values, there is no actual guarantee when entering in Hyp
> from secure-only firmware that the Non-Secure PL1 state will look the
> way that kernel entry (in particular the decompressor stub) expects.
> So far, we've been getting away with it thanks to implementation details
> of ARMv7 cores and/or bootloader behaviour, but for the sake of forwards
> compatibility let's try to ensure that we have a minimally sane state
> before dropping down into it.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH v2] kvm: arm: Enforce some NS-SVC initialisation

[Christoffer Dall]

On Tue, Aug 16, 2016 at 06:49:18PM +0100, Robin Murphy wrote:
> Since the non-secure copies of banked registers lack architecturally
> defined reset values, there is no actual guarantee when entering in Hyp
> from secure-only firmware that the Non-Secure PL1 state will look the
> way that kernel entry (in particular the decompressor stub) expects.
> So far, we've been getting away with it thanks to implementation details
> of ARMv7 cores and/or bootloader behaviour, but for the sake of forwards
> compatibility let's try to ensure that we have a minimally sane state
> before dropping down into it.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

> ---
> 
> v2: Initialise SED/ITD to safe values as well.
> 
>  arch/arm/kernel/hyp-stub.S | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/arch/arm/kernel/hyp-stub.S b/arch/arm/kernel/hyp-stub.S
> index 0b1e4a93d67e..15d073ae5da2 100644
> --- a/arch/arm/kernel/hyp-stub.S
> +++ b/arch/arm/kernel/hyp-stub.S
> @@ -142,6 +142,19 @@ ARM_BE8(orr	r7, r7, #(1 << 25))     @ HSCTLR.EE
>  	and	r7, #0x1f		@ Preserve HPMN
>  	mcr	p15, 4, r7, c1, c1, 1	@ HDCR
>  
> +	@ Make sure NS-SVC is initialised appropriately
> +	mrc	p15, 0, r7, c1, c0, 0	@ SCTLR
> +	orr	r7, #(1 << 5)		@ CP15 barriers enabled
> +	bic	r7, #(3 << 7)		@ Clear SED/ITD for v8 (RES0 for v7)
> +	bic	r7, #(3 << 19)		@ WXN and UWXN disabled
> +	mcr	p15, 0, r7, c1, c0, 0	@ SCTLR
> +
> +	mrc	p15, 0, r7, c0, c0, 0	@ MIDR
> +	mcr	p15, 4, r7, c0, c0, 0	@ VPIDR
> +
> +	mrc	p15, 0, r7, c0, c0, 5	@ MPIDR
> +	mcr	p15, 4, r7, c0, c0, 5	@ VMPIDR
> +
>  #if !defined(ZIMAGE) && defined(CONFIG_ARM_ARCH_TIMER)
>  	@ make CNTP_* and CNTPCT accessible from PL1
>  	mrc	p15, 0, r7, c0, c1, 1	@ ID_PFR1
> -- 
> 2.8.1.dirty
> 

[PATCH v2] kvm: arm: Enforce some NS-SVC initialisation

[Christoffer Dall]

Hi Russell,

On Tue, Aug 16, 2016 at 06:49:18PM +0100, Robin Murphy wrote:
> Since the non-secure copies of banked registers lack architecturally
> defined reset values, there is no actual guarantee when entering in Hyp
> from secure-only firmware that the Non-Secure PL1 state will look the
> way that kernel entry (in particular the decompressor stub) expects.
> So far, we've been getting away with it thanks to implementation details
> of ARMv7 cores and/or bootloader behaviour, but for the sake of forwards
> compatibility let's try to ensure that we have a minimally sane state
> before dropping down into it.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

Can we have your acked-by on this patch to merge it via the KVM tree or
do you prefer merging this one directly yourself?

Thanks,
-Christoffer

[PATCH v2] kvm: arm: Enforce some NS-SVC initialisation

[Christoffer Dall]

Hi Russell,

On Tue, Aug 16, 2016 at 06:49:18PM +0100, Robin Murphy wrote:
> Since the non-secure copies of banked registers lack architecturally
> defined reset values, there is no actual guarantee when entering in Hyp
> from secure-only firmware that the Non-Secure PL1 state will look the
> way that kernel entry (in particular the decompressor stub) expects.
> So far, we've been getting away with it thanks to implementation details
> of ARMv7 cores and/or bootloader behaviour, but for the sake of forwards
> compatibility let's try to ensure that we have a minimally sane state
> before dropping down into it.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

Can we have your acked-by on this patch to merge it via the KVM tree or
do you prefer merging this one directly yourself?

Thanks,
-Christoffer

[PATCH v2] kvm: arm: Enforce some NS-SVC initialisation

[Christoffer Dall]

On Wed, Aug 17, 2016 at 01:32:49PM +0200, Christoffer Dall wrote:
> Hi Russell,
> 
> On Tue, Aug 16, 2016 at 06:49:18PM +0100, Robin Murphy wrote:
> > Since the non-secure copies of banked registers lack architecturally
> > defined reset values, there is no actual guarantee when entering in Hyp
> > from secure-only firmware that the Non-Secure PL1 state will look the
> > way that kernel entry (in particular the decompressor stub) expects.
> > So far, we've been getting away with it thanks to implementation details
> > of ARMv7 cores and/or bootloader behaviour, but for the sake of forwards
> > compatibility let's try to ensure that we have a minimally sane state
> > before dropping down into it.
> > 
> > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> 
> Can we have your acked-by on this patch to merge it via the KVM tree or
> do you prefer merging this one directly yourself?
> 
I have queued this via the KVM/ARM tree.  If you have any objections,
please let me know asap.

Thanks,
-Christoffer

[PATCH v2] kvm: arm: Enforce some NS-SVC initialisation

[Robin Murphy]

Hi Christoffer,

On 06/09/16 17:41, Christoffer Dall wrote:
> On Wed, Aug 17, 2016 at 01:32:49PM +0200, Christoffer Dall wrote:
>> Hi Russell,
>>
>> On Tue, Aug 16, 2016 at 06:49:18PM +0100, Robin Murphy wrote:
>>> Since the non-secure copies of banked registers lack architecturally
>>> defined reset values, there is no actual guarantee when entering in Hyp
>>> from secure-only firmware that the Non-Secure PL1 state will look the
>>> way that kernel entry (in particular the decompressor stub) expects.
>>> So far, we've been getting away with it thanks to implementation details
>>> of ARMv7 cores and/or bootloader behaviour, but for the sake of forwards
>>> compatibility let's try to ensure that we have a minimally sane state
>>> before dropping down into it.
>>>
>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>
>> Can we have your acked-by on this patch to merge it via the KVM tree or
>> do you prefer merging this one directly yourself?
>>
> I have queued this via the KVM/ARM tree.  If you have any objections,
> please let me know asap.

Ah, I should have replied on this thread - I put this one into Russell's
patch system the other week, and it's in -next already.

Sorry for the lack of communication,
Robin.

> 
> Thanks,
> -Christoffer
> 

[PATCH v2] kvm: arm: Enforce some NS-SVC initialisation

[Christoffer Dall]

On Tue, Sep 6, 2016 at 6:44 PM, Robin Murphy <robin.murphy@arm.com> wrote:
> Hi Christoffer,
>
> On 06/09/16 17:41, Christoffer Dall wrote:
>> On Wed, Aug 17, 2016 at 01:32:49PM +0200, Christoffer Dall wrote:
>>> Hi Russell,
>>>
>>> On Tue, Aug 16, 2016 at 06:49:18PM +0100, Robin Murphy wrote:
>>>> Since the non-secure copies of banked registers lack architecturally
>>>> defined reset values, there is no actual guarantee when entering in Hyp
>>>> from secure-only firmware that the Non-Secure PL1 state will look the
>>>> way that kernel entry (in particular the decompressor stub) expects.
>>>> So far, we've been getting away with it thanks to implementation details
>>>> of ARMv7 cores and/or bootloader behaviour, but for the sake of forwards
>>>> compatibility let's try to ensure that we have a minimally sane state
>>>> before dropping down into it.
>>>>
>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>>
>>> Can we have your acked-by on this patch to merge it via the KVM tree or
>>> do you prefer merging this one directly yourself?
>>>
>> I have queued this via the KVM/ARM tree.  If you have any objections,
>> please let me know asap.
>
> Ah, I should have replied on this thread - I put this one into Russell's
> patch system the other week, and it's in -next already.
>
> Sorry for the lack of communication,

No worries, I removed it from the queue again.

-Christoffer