| Message ID | 1502280338-23002-22-git-send-email-Dave.Martin@arm.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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.
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.
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
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); }
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(-)