[21/27] arm64/sve: KVM: Prevent guests from using SVE

Submitted by Dave Martin on Aug. 9, 2017, 12:05 p.m.

Details

Message ID 1502280338-23002-22-git-send-email-Dave.Martin@arm.com
State New
Headers show

Commit Message

Dave Martin Aug. 9, 2017, 12:05 p.m.
Until KVM has full SVE support, guests must not be allowed to
execute SVE instructions.

This patch enables the necessary traps, and also ensures that the
traps are disabled again on exit from the guest so that the host
can still use SVE if it wants to.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/include/asm/kvm_arm.h | 3 ++-
 arch/arm64/kvm/hyp/switch.c      | 6 +++---
 2 files changed, 5 insertions(+), 4 deletions(-)

Comments

Marc Zyngier Aug. 15, 2017, 4:33 p.m.
On 09/08/17 13:05, Dave Martin wrote:
> Until KVM has full SVE support, guests must not be allowed to
> execute SVE instructions.
> 
> This patch enables the necessary traps, and also ensures that the
> traps are disabled again on exit from the guest so that the host
> can still use SVE if it wants to.
> 
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> ---
>  arch/arm64/include/asm/kvm_arm.h | 3 ++-
>  arch/arm64/kvm/hyp/switch.c      | 6 +++---
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> index dbf0537..8a19651 100644
> --- a/arch/arm64/include/asm/kvm_arm.h
> +++ b/arch/arm64/include/asm/kvm_arm.h
> @@ -186,7 +186,7 @@
>  #define CPTR_EL2_TTA	(1 << 20)
>  #define CPTR_EL2_TFP	(1 << CPTR_EL2_TFP_SHIFT)
>  #define CPTR_EL2_TZ	(1 << 8)
> -#define CPTR_EL2_DEFAULT	0x000033ff
> +#define CPTR_EL2_DEFAULT	(0x000033ff & ~CPTR_EL2_TZ)

I must say I'm not overly fond of this construct. I'd rather introduce a
RES1 field that matches the v8.2 description, instead of this ugly
constant and something that clears it.

>  
>  /* Hyp Debug Configuration Register bits */
>  #define MDCR_EL2_TPMS		(1 << 14)
> @@ -237,5 +237,6 @@
>  
>  #define CPACR_EL1_FPEN		(3 << 20)
>  #define CPACR_EL1_TTA		(1 << 28)
> +#define CPACR_EL1_DEFAULT	(CPACR_EL1_FPEN | CPACR_EL1_ZEN_EL1EN)
>  
>  #endif /* __ARM64_KVM_ARM_H__ */
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index 35a90b8..951f3eb 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -48,7 +48,7 @@ static void __hyp_text __activate_traps_vhe(void)
>  
>  	val = read_sysreg(cpacr_el1);
>  	val |= CPACR_EL1_TTA;
> -	val &= ~CPACR_EL1_FPEN;
> +	val &= ~(CPACR_EL1_FPEN | CPACR_EL1_ZEN);
>  	write_sysreg(val, cpacr_el1);
>  
>  	write_sysreg(__kvm_hyp_vector, vbar_el1);
> @@ -59,7 +59,7 @@ static void __hyp_text __activate_traps_nvhe(void)
>  	u64 val;
>  
>  	val = CPTR_EL2_DEFAULT;
> -	val |= CPTR_EL2_TTA | CPTR_EL2_TFP;
> +	val |= CPTR_EL2_TTA | CPTR_EL2_TFP | CPTR_EL2_TZ;
>  	write_sysreg(val, cptr_el2);
>  }
>  
> @@ -117,7 +117,7 @@ static void __hyp_text __deactivate_traps_vhe(void)
>  
>  	write_sysreg(mdcr_el2, mdcr_el2);
>  	write_sysreg(HCR_HOST_VHE_FLAGS, hcr_el2);
> -	write_sysreg(CPACR_EL1_FPEN, cpacr_el1);
> +	write_sysreg(CPACR_EL1_DEFAULT, cpacr_el1);
>  	write_sysreg(vectors, vbar_el1);
>  }
>  
> 

Thanks,

	M.
Dave Martin Aug. 16, 2017, 10:50 a.m.
On Tue, Aug 15, 2017 at 05:33:15PM +0100, Marc Zyngier wrote:
> On 09/08/17 13:05, Dave Martin wrote:
> > Until KVM has full SVE support, guests must not be allowed to
> > execute SVE instructions.
> > 
> > This patch enables the necessary traps, and also ensures that the
> > traps are disabled again on exit from the guest so that the host
> > can still use SVE if it wants to.
> > 
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > ---
> >  arch/arm64/include/asm/kvm_arm.h | 3 ++-
> >  arch/arm64/kvm/hyp/switch.c      | 6 +++---
> >  2 files changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> > index dbf0537..8a19651 100644
> > --- a/arch/arm64/include/asm/kvm_arm.h
> > +++ b/arch/arm64/include/asm/kvm_arm.h
> > @@ -186,7 +186,7 @@
> >  #define CPTR_EL2_TTA	(1 << 20)
> >  #define CPTR_EL2_TFP	(1 << CPTR_EL2_TFP_SHIFT)
> >  #define CPTR_EL2_TZ	(1 << 8)
> > -#define CPTR_EL2_DEFAULT	0x000033ff
> > +#define CPTR_EL2_DEFAULT	(0x000033ff & ~CPTR_EL2_TZ)
> 
> I must say I'm not overly fond of this construct. I'd rather introduce a
> RES1 field that matches the v8.2 description, instead of this ugly
> constant and something that clears it.

Sorry, I don't get your meaning here.  v8.2 neither immediately predates
or postdates SVE.  What are you propsing?

I guess we could just write 0x000032ff now that the only meaning the
architecture can ever assign to bit 8 is known.

[...]

Cheers
---Dave
Marc Zyngier Aug. 16, 2017, 11:20 a.m.
On 16/08/17 11:50, Dave Martin wrote:
> On Tue, Aug 15, 2017 at 05:33:15PM +0100, Marc Zyngier wrote:
>> On 09/08/17 13:05, Dave Martin wrote:
>>> Until KVM has full SVE support, guests must not be allowed to
>>> execute SVE instructions.
>>>
>>> This patch enables the necessary traps, and also ensures that the
>>> traps are disabled again on exit from the guest so that the host
>>> can still use SVE if it wants to.
>>>
>>> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
>>> ---
>>>  arch/arm64/include/asm/kvm_arm.h | 3 ++-
>>>  arch/arm64/kvm/hyp/switch.c      | 6 +++---
>>>  2 files changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
>>> index dbf0537..8a19651 100644
>>> --- a/arch/arm64/include/asm/kvm_arm.h
>>> +++ b/arch/arm64/include/asm/kvm_arm.h
>>> @@ -186,7 +186,7 @@
>>>  #define CPTR_EL2_TTA	(1 << 20)
>>>  #define CPTR_EL2_TFP	(1 << CPTR_EL2_TFP_SHIFT)
>>>  #define CPTR_EL2_TZ	(1 << 8)
>>> -#define CPTR_EL2_DEFAULT	0x000033ff
>>> +#define CPTR_EL2_DEFAULT	(0x000033ff & ~CPTR_EL2_TZ)
>>
>> I must say I'm not overly fond of this construct. I'd rather introduce a
>> RES1 field that matches the v8.2 description, instead of this ugly
>> constant and something that clears it.
> 
> Sorry, I don't get your meaning here.  v8.2 neither immediately predates
> or postdates SVE.  

The ARMv8 ARM (DDI406B_a, D7.2.19) says otherwise. This bit is only
defined as having a possibility of being 0 on an v8.2 implementation if
SVE is implemented.

> What are you propsing?

What I'm proposing is:

#define CPTR_EL2_RES1		0x32ff
#define CPTR_EL2_DEFAULT	CPTR_EL2_RES1

and none of that pointless constant clearing.

> I guess we could just write 0x000032ff now that the only meaning the
> architecture can ever assign to bit 8 is known.
Exactly.

	M.
Marc Zyngier Aug. 16, 2017, 11:22 a.m.
On 16/08/17 12:20, Marc Zyngier wrote:
> On 16/08/17 11:50, Dave Martin wrote:
>> On Tue, Aug 15, 2017 at 05:33:15PM +0100, Marc Zyngier wrote:
>>> On 09/08/17 13:05, Dave Martin wrote:
>>>> Until KVM has full SVE support, guests must not be allowed to
>>>> execute SVE instructions.
>>>>
>>>> This patch enables the necessary traps, and also ensures that the
>>>> traps are disabled again on exit from the guest so that the host
>>>> can still use SVE if it wants to.
>>>>
>>>> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
>>>> ---
>>>>  arch/arm64/include/asm/kvm_arm.h | 3 ++-
>>>>  arch/arm64/kvm/hyp/switch.c      | 6 +++---
>>>>  2 files changed, 5 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
>>>> index dbf0537..8a19651 100644
>>>> --- a/arch/arm64/include/asm/kvm_arm.h
>>>> +++ b/arch/arm64/include/asm/kvm_arm.h
>>>> @@ -186,7 +186,7 @@
>>>>  #define CPTR_EL2_TTA	(1 << 20)
>>>>  #define CPTR_EL2_TFP	(1 << CPTR_EL2_TFP_SHIFT)
>>>>  #define CPTR_EL2_TZ	(1 << 8)
>>>> -#define CPTR_EL2_DEFAULT	0x000033ff
>>>> +#define CPTR_EL2_DEFAULT	(0x000033ff & ~CPTR_EL2_TZ)
>>>
>>> I must say I'm not overly fond of this construct. I'd rather introduce a
>>> RES1 field that matches the v8.2 description, instead of this ugly
>>> constant and something that clears it.
>>
>> Sorry, I don't get your meaning here.  v8.2 neither immediately predates
>> or postdates SVE.  
> 
> The ARMv8 ARM (DDI406B_a, D7.2.19) says otherwise. This bit is only

Of course, the days of the ARMv7 ARM are still haunting me. This should
read DDI487B_a.

Apologies for the confusion.

	M.
Dave Martin Aug. 16, 2017, 11:35 a.m.
On Wed, Aug 16, 2017 at 12:20:41PM +0100, Marc Zyngier wrote:
> On 16/08/17 11:50, Dave Martin wrote:
> > On Tue, Aug 15, 2017 at 05:33:15PM +0100, Marc Zyngier wrote:
> >> On 09/08/17 13:05, Dave Martin wrote:
> >>> Until KVM has full SVE support, guests must not be allowed to
> >>> execute SVE instructions.
> >>>
> >>> This patch enables the necessary traps, and also ensures that the
> >>> traps are disabled again on exit from the guest so that the host
> >>> can still use SVE if it wants to.
> >>>
> >>> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> >>> ---
> >>>  arch/arm64/include/asm/kvm_arm.h | 3 ++-
> >>>  arch/arm64/kvm/hyp/switch.c      | 6 +++---
> >>>  2 files changed, 5 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> >>> index dbf0537..8a19651 100644
> >>> --- a/arch/arm64/include/asm/kvm_arm.h
> >>> +++ b/arch/arm64/include/asm/kvm_arm.h
> >>> @@ -186,7 +186,7 @@
> >>>  #define CPTR_EL2_TTA	(1 << 20)
> >>>  #define CPTR_EL2_TFP	(1 << CPTR_EL2_TFP_SHIFT)
> >>>  #define CPTR_EL2_TZ	(1 << 8)
> >>> -#define CPTR_EL2_DEFAULT	0x000033ff
> >>> +#define CPTR_EL2_DEFAULT	(0x000033ff & ~CPTR_EL2_TZ)
> >>
> >> I must say I'm not overly fond of this construct. I'd rather introduce a
> >> RES1 field that matches the v8.2 description, instead of this ugly
> >> constant and something that clears it.
> > 
> > Sorry, I don't get your meaning here.  v8.2 neither immediately predates
> > or postdates SVE.  
> 
> The ARMv8 ARM (DDI406B_a, D7.2.19) says otherwise. This bit is only
> defined as having a possibility of being 0 on an v8.2 implementation if
> SVE is implemented.

Right.  I was confused by the v8.2 reference, since if this weren't true
for v8.0 of the architecture, we couldn't simply change a compile time
constant here.  In fact, there's a compatible retroactive change to all
arch versions >= v8.0.

> > What are you propsing?
> 
> What I'm proposing is:
> 
> #define CPTR_EL2_RES1		0x32ff
> #define CPTR_EL2_DEFAULT	CPTR_EL2_RES1
> 
> and none of that pointless constant clearing.
> 
> > I guess we could just write 0x000032ff now that the only meaning the
> > architecture can ever assign to bit 8 is known.
> Exactly.

OK, good -- I'll change that.

I was trying to avoid magic numberness, but it's a bit futile when
talking about RESx bits.

Cheers
---Dave

Patch hide | download patch | download mbox

diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
index dbf0537..8a19651 100644
--- a/arch/arm64/include/asm/kvm_arm.h
+++ b/arch/arm64/include/asm/kvm_arm.h
@@ -186,7 +186,7 @@ 
 #define CPTR_EL2_TTA	(1 << 20)
 #define CPTR_EL2_TFP	(1 << CPTR_EL2_TFP_SHIFT)
 #define CPTR_EL2_TZ	(1 << 8)
-#define CPTR_EL2_DEFAULT	0x000033ff
+#define CPTR_EL2_DEFAULT	(0x000033ff & ~CPTR_EL2_TZ)
 
 /* Hyp Debug Configuration Register bits */
 #define MDCR_EL2_TPMS		(1 << 14)
@@ -237,5 +237,6 @@ 
 
 #define CPACR_EL1_FPEN		(3 << 20)
 #define CPACR_EL1_TTA		(1 << 28)
+#define CPACR_EL1_DEFAULT	(CPACR_EL1_FPEN | CPACR_EL1_ZEN_EL1EN)
 
 #endif /* __ARM64_KVM_ARM_H__ */
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index 35a90b8..951f3eb 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -48,7 +48,7 @@  static void __hyp_text __activate_traps_vhe(void)
 
 	val = read_sysreg(cpacr_el1);
 	val |= CPACR_EL1_TTA;
-	val &= ~CPACR_EL1_FPEN;
+	val &= ~(CPACR_EL1_FPEN | CPACR_EL1_ZEN);
 	write_sysreg(val, cpacr_el1);
 
 	write_sysreg(__kvm_hyp_vector, vbar_el1);
@@ -59,7 +59,7 @@  static void __hyp_text __activate_traps_nvhe(void)
 	u64 val;
 
 	val = CPTR_EL2_DEFAULT;
-	val |= CPTR_EL2_TTA | CPTR_EL2_TFP;
+	val |= CPTR_EL2_TTA | CPTR_EL2_TFP | CPTR_EL2_TZ;
 	write_sysreg(val, cptr_el2);
 }
 
@@ -117,7 +117,7 @@  static void __hyp_text __deactivate_traps_vhe(void)
 
 	write_sysreg(mdcr_el2, mdcr_el2);
 	write_sysreg(HCR_HOST_VHE_FLAGS, hcr_el2);
-	write_sysreg(CPACR_EL1_FPEN, cpacr_el1);
+	write_sysreg(CPACR_EL1_DEFAULT, cpacr_el1);
 	write_sysreg(vectors, vbar_el1);
 }