| Message ID | 20171019145807.23251-16-james.morse@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2017-10-19 22:58 GMT+08:00 James Morse <james.morse@arm.com>: > Prior to v8.2's RAS Extensions, the HCR_EL2.VSE 'virtual SError' feature > generated an SError with an implementation defined ESR_EL1.ISS, because we > had no mechanism to specify the ESR value. > > On Juno this generates an all-zero ESR, the most significant bit 'ISV' > is clear indicating the remainder of the ISS field is invalid. > > With the RAS Extensions we have a mechanism to specify this value, and the > most significant bit has a new meaning: 'IDS - Implementation Defined > Syndrome'. An all-zero SError ESR now means: 'RAS error: Uncategorized' > instead of 'no valid ISS'. consider again. I still consider that it is not better set "Implementation Defined Syndrome" here. I know your meaning that An all-zero SError means Uncategorized From the beginning, our starting point is KVM is the architecture related and Qemu/kvmtool is platform related. Qemu/kvmtool is the role of host firmware. so we let Qemu to create APEI/GHES table and record CPER, and also injects SEA/SEI in the Qemu. About the vsesr_el2 which is used to specify the guest ESR, also all set by Qemu/kvmtool are better. including the three cases even the ESR are all-zero(uncategorized). 1. IMPLEMENTATION DEFINED 2 categorized, 3 uncategorized, For this case, Qemu just set the ESR, nothing else, not passing RAS-error to userspace.
Hi gengdongjiu, On 20/10/17 17:44, gengdongjiu wrote: > 2017-10-19 22:58 GMT+08:00 James Morse <james.morse@arm.com>: >> Prior to v8.2's RAS Extensions, the HCR_EL2.VSE 'virtual SError' feature >> generated an SError with an implementation defined ESR_EL1.ISS, because we >> had no mechanism to specify the ESR value. >> >> On Juno this generates an all-zero ESR, the most significant bit 'ISV' >> is clear indicating the remainder of the ISS field is invalid. >> >> With the RAS Extensions we have a mechanism to specify this value, and the >> most significant bit has a new meaning: 'IDS - Implementation Defined >> Syndrome'. An all-zero SError ESR now means: 'RAS error: Uncategorized' >> instead of 'no valid ISS'. > > consider again. > I still consider that it is not better set "Implementation Defined > Syndrome" here. Do you agree KVM does this today? ddb3d07cfe90 ("arm64: KVM: Inject a Virtual SError if it was pending") All I've changed here is that you don't get a RAS error passed into the guest. Its either deemed non-blocking and ignored, or we kick the guest with an impdef SError. To do any better we need kernel first support, or a firmware first notification. > I know your meaning that An all-zero SError means Uncategorized Uncategorized RAS error. RAS errors should be handled by the host, we should not inject them into the guest on a path like this. Which impdef ESR would you like to use, and why does it matter? > From the beginning, our starting point is KVM is the architecture > related and Qemu/kvmtool is platform related. > Qemu/kvmtool is the role of host firmware. > so we let Qemu to create APEI/GHES table and record CPER, > and also injects SEA/SEI in the Qemu. For RAS events it generated all by itself, or was notified by the kernel using signals. This is not one of those cases. But that's fine as we aren't injecting a RAS error. > About the vsesr_el2 which is used to specify the guest ESR, also all > set by Qemu/kvmtool are better. > including the three cases even the ESR are all-zero(uncategorized). > 1. IMPLEMENTATION DEFINED > 2 categorized, > 3 uncategorized, I can't work out what you're saying here. If you're suggesting Qemu should set a default 'unknown' ESR for use when KVM doesn't know what to do, and SError is pretty much its only option: Why would Qemu set anything other than impdef:all-zeros? The only use would be to fake this back into a RAS error. Qemu isn't involved here so this can't be used to emulate NOTIFY_SEI without the kernel support. We don't have support for emulating the RAS ERR registers either, so this can't be used to emulate kernel first. If you're suggesting Qemu should specify a set of ESR values for the different cases where KVM doesn't know what do: this would be an early shortcut that burns us in the future, these things are going to be changed. This is too specific to KVMs internals: When we add NOTIFY_SEI support, we will call out of this KVM code and APEI should 'claim' any SError that arrived with CPER records. I expect this code to change dramatically if/when we add kernel first support. The KVM patches on the end of this series are just the minimum needed to enable IESB and keep the status quo for all other behaviour. And this is just so we can tackle the other bits of support that depends on the cpufeature without reposting all the same code. James > For this case, Qemu just set the ESR, nothing else, not passing > RAS-error to userspace.
Hi James, On 2017/10/23 23:26, James Morse wrote: > If you're suggesting Qemu should set a default 'unknown' ESR for use when KVM > doesn't know what to do, and SError is pretty much its only option: > > Why would Qemu set anything other than impdef:all-zeros? > > The only use would be to fake this back into a RAS error. Qemu isn't involved > here so this can't be used to emulate NOTIFY_SEI without the kernel support. We > don't have support for emulating the RAS ERR registers either, so this can't be > used to emulate kernel first. > > > If you're suggesting Qemu should specify a set of ESR values for the different > cases where KVM doesn't know what do: this would be an early shortcut that burns > us in the future, these things are going to be changed. This is too specific to > KVMs internals: > > When we add NOTIFY_SEI support, we will call out of this KVM code and APEI > should 'claim' any SError that arrived with CPER records. > > I expect this code to change dramatically if/when we add kernel first support. > > > The KVM patches on the end of this series are just the minimum needed to enable > IESB and keep the status quo for all other behaviour. And this is just so we can > tackle the other bits of support that depends on the cpufeature without > reposting all the same code. Thanks very much for your explanation, understand it now.
On Thu, Oct 19, 2017 at 03:58:01PM +0100, James Morse wrote: > Prior to v8.2's RAS Extensions, the HCR_EL2.VSE 'virtual SError' feature > generated an SError with an implementation defined ESR_EL1.ISS, because we > had no mechanism to specify the ESR value. > > On Juno this generates an all-zero ESR, the most significant bit 'ISV' > is clear indicating the remainder of the ISS field is invalid. > > With the RAS Extensions we have a mechanism to specify this value, and the > most significant bit has a new meaning: 'IDS - Implementation Defined > Syndrome'. An all-zero SError ESR now means: 'RAS error: Uncategorized' > instead of 'no valid ISS'. > > Add KVM support for the VSESR_EL2 register to specify an ESR value when > HCR_EL2.VSE generates a virtual SError. Change kvm_inject_vabt() to > specify an implementation-defined value. > > We only need to restore the VSESR_EL2 value when HCR_EL2.VSE is set, KVM > save/restores this bit during __deactivate_traps() and hardware clears the > bit once the guest has consumed the virtual-SError. > > Future patches may add an API (or KVM CAP) to pend a virtual SError with > a specified ESR. > > Cc: Dongjiu Geng <gengdongjiu@huawei.com> > Signed-off-by: James Morse <james.morse@arm.com> > --- > arch/arm64/include/asm/kvm_emulate.h | 5 +++++ > arch/arm64/include/asm/kvm_host.h | 3 +++ > arch/arm64/include/asm/sysreg.h | 1 + > arch/arm64/kvm/hyp/switch.c | 4 ++++ > arch/arm64/kvm/inject_fault.c | 13 ++++++++++++- > 5 files changed, 25 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h > index e5df3fce0008..8a7a838eb17a 100644 > --- a/arch/arm64/include/asm/kvm_emulate.h > +++ b/arch/arm64/include/asm/kvm_emulate.h > @@ -61,6 +61,11 @@ static inline void vcpu_set_hcr(struct kvm_vcpu *vcpu, unsigned long hcr) > vcpu->arch.hcr_el2 = hcr; > } > > +static inline void vcpu_set_vsesr(struct kvm_vcpu *vcpu, u64 vsesr) > +{ > + vcpu->arch.vsesr_el2 = vsesr; > +} > + > static inline unsigned long *vcpu_pc(const struct kvm_vcpu *vcpu) > { > return (unsigned long *)&vcpu_gp_regs(vcpu)->regs.pc; > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index a0e2f7962401..28a4de85edee 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -277,6 +277,9 @@ struct kvm_vcpu_arch { > > /* Detect first run of a vcpu */ > bool has_run_once; > + > + /* Virtual SError ESR to restore when HCR_EL2.VSE is set */ > + u64 vsesr_el2; > }; > > #define vcpu_gp_regs(v) (&(v)->arch.ctxt.gp_regs) > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h > index 427c36bc5dd6..a493e93de296 100644 > --- a/arch/arm64/include/asm/sysreg.h > +++ b/arch/arm64/include/asm/sysreg.h > @@ -253,6 +253,7 @@ > > #define SYS_DACR32_EL2 sys_reg(3, 4, 3, 0, 0) > #define SYS_IFSR32_EL2 sys_reg(3, 4, 5, 0, 1) > +#define SYS_VSESR_EL2 sys_reg(3, 4, 5, 2, 3) > #define SYS_FPEXC32_EL2 sys_reg(3, 4, 5, 3, 0) > > #define __SYS__AP0Rx_EL2(x) sys_reg(3, 4, 12, 8, x) > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c > index 945e79c641c4..af37658223a0 100644 > --- a/arch/arm64/kvm/hyp/switch.c > +++ b/arch/arm64/kvm/hyp/switch.c > @@ -86,6 +86,10 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu) > isb(); > } > write_sysreg(val, hcr_el2); > + > + if (cpus_have_const_cap(ARM64_HAS_RAS_EXTN) && (val & HCR_VSE)) > + write_sysreg_s(vcpu->arch.vsesr_el2, SYS_VSESR_EL2); > + Just a heads up: If my optimization work gets merged, that will eventually move stuff like this in to load/put hooks for system registers, but I can deal with this easily, also adding a direct write in pend_guest_serror when moving the logic around. However, if we start architecting something more complex, it would be good to keep in mind how to maintain minimum work on the switching path after we've optimized the hypervisor. > /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */ > write_sysreg(1 << 15, hstr_el2); > /* > diff --git a/arch/arm64/kvm/inject_fault.c b/arch/arm64/kvm/inject_fault.c > index da6a8cfa54a0..52f7f66f1356 100644 > --- a/arch/arm64/kvm/inject_fault.c > +++ b/arch/arm64/kvm/inject_fault.c > @@ -232,14 +232,25 @@ void kvm_inject_undefined(struct kvm_vcpu *vcpu) > inject_undef64(vcpu); > } > > +static void pend_guest_serror(struct kvm_vcpu *vcpu, u64 esr) > +{ > + vcpu_set_vsesr(vcpu, esr); > + vcpu_set_hcr(vcpu, vcpu_get_hcr(vcpu) | HCR_VSE); > +} > + > /** > * kvm_inject_vabt - inject an async abort / SError into the guest > * @vcpu: The VCPU to receive the exception > * > * It is assumed that this code is called from the VCPU thread and that the > * VCPU therefore is not currently executing guest code. > + * > + * Systems with the RAS Extensions specify an imp-def ESR (ISV/IDS = 1) with > + * the remaining ISS all-zeros so that this error is not interpreted as an > + * uncatagorized RAS error. Without the RAS Extensions we can't specify an ESR nit: uncategorized > + * value, so the CPU generates an imp-def value. > */ > void kvm_inject_vabt(struct kvm_vcpu *vcpu) > { > - vcpu_set_hcr(vcpu, vcpu_get_hcr(vcpu) | HCR_VSE); > + pend_guest_serror(vcpu, ESR_ELx_ISV); > } > -- > 2.13.3 > Otherwise: Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
On Mon, Oct 30, 2017 at 08:59:51AM +0100, Christoffer Dall wrote: > On Thu, Oct 19, 2017 at 03:58:01PM +0100, James Morse wrote: > > Prior to v8.2's RAS Extensions, the HCR_EL2.VSE 'virtual SError' feature > > generated an SError with an implementation defined ESR_EL1.ISS, because we > > had no mechanism to specify the ESR value. > > > > On Juno this generates an all-zero ESR, the most significant bit 'ISV' > > is clear indicating the remainder of the ISS field is invalid. > > > > With the RAS Extensions we have a mechanism to specify this value, and the > > most significant bit has a new meaning: 'IDS - Implementation Defined > > Syndrome'. An all-zero SError ESR now means: 'RAS error: Uncategorized' > > instead of 'no valid ISS'. > > > > Add KVM support for the VSESR_EL2 register to specify an ESR value when > > HCR_EL2.VSE generates a virtual SError. Change kvm_inject_vabt() to > > specify an implementation-defined value. > > > > We only need to restore the VSESR_EL2 value when HCR_EL2.VSE is set, KVM > > save/restores this bit during __deactivate_traps() and hardware clears the > > bit once the guest has consumed the virtual-SError. > > > > Future patches may add an API (or KVM CAP) to pend a virtual SError with > > a specified ESR. > > > > Cc: Dongjiu Geng <gengdongjiu@huawei.com> > > Signed-off-by: James Morse <james.morse@arm.com> > > --- > > arch/arm64/include/asm/kvm_emulate.h | 5 +++++ > > arch/arm64/include/asm/kvm_host.h | 3 +++ > > arch/arm64/include/asm/sysreg.h | 1 + > > arch/arm64/kvm/hyp/switch.c | 4 ++++ > > arch/arm64/kvm/inject_fault.c | 13 ++++++++++++- > > 5 files changed, 25 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h > > index e5df3fce0008..8a7a838eb17a 100644 > > --- a/arch/arm64/include/asm/kvm_emulate.h > > +++ b/arch/arm64/include/asm/kvm_emulate.h > > @@ -61,6 +61,11 @@ static inline void vcpu_set_hcr(struct kvm_vcpu *vcpu, unsigned long hcr) > > vcpu->arch.hcr_el2 = hcr; > > } > > > > +static inline void vcpu_set_vsesr(struct kvm_vcpu *vcpu, u64 vsesr) > > +{ > > + vcpu->arch.vsesr_el2 = vsesr; > > +} > > + > > static inline unsigned long *vcpu_pc(const struct kvm_vcpu *vcpu) > > { > > return (unsigned long *)&vcpu_gp_regs(vcpu)->regs.pc; > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > > index a0e2f7962401..28a4de85edee 100644 > > --- a/arch/arm64/include/asm/kvm_host.h > > +++ b/arch/arm64/include/asm/kvm_host.h > > @@ -277,6 +277,9 @@ struct kvm_vcpu_arch { > > > > /* Detect first run of a vcpu */ > > bool has_run_once; > > + > > + /* Virtual SError ESR to restore when HCR_EL2.VSE is set */ > > + u64 vsesr_el2; > > }; > > > > #define vcpu_gp_regs(v) (&(v)->arch.ctxt.gp_regs) > > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h > > index 427c36bc5dd6..a493e93de296 100644 > > --- a/arch/arm64/include/asm/sysreg.h > > +++ b/arch/arm64/include/asm/sysreg.h > > @@ -253,6 +253,7 @@ > > > > #define SYS_DACR32_EL2 sys_reg(3, 4, 3, 0, 0) > > #define SYS_IFSR32_EL2 sys_reg(3, 4, 5, 0, 1) > > +#define SYS_VSESR_EL2 sys_reg(3, 4, 5, 2, 3) > > #define SYS_FPEXC32_EL2 sys_reg(3, 4, 5, 3, 0) > > > > #define __SYS__AP0Rx_EL2(x) sys_reg(3, 4, 12, 8, x) > > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c > > index 945e79c641c4..af37658223a0 100644 > > --- a/arch/arm64/kvm/hyp/switch.c > > +++ b/arch/arm64/kvm/hyp/switch.c > > @@ -86,6 +86,10 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu) > > isb(); > > } > > write_sysreg(val, hcr_el2); > > + > > + if (cpus_have_const_cap(ARM64_HAS_RAS_EXTN) && (val & HCR_VSE)) > > + write_sysreg_s(vcpu->arch.vsesr_el2, SYS_VSESR_EL2); > > + > > Just a heads up: If my optimization work gets merged, that will > eventually move stuff like this in to load/put hooks for system > registers, but I can deal with this easily, also adding a direct write > in pend_guest_serror when moving the logic around. > > However, if we start architecting something more complex, it would be > good to keep in mind how to maintain minimum work on the switching path > after we've optimized the hypervisor. > Actually, after thinking about this, if the guest can only see this via the ESR if we set the HCR_EL2.VSE, wouldn't it make sense to just set this value in pend_guest_serror, and if we're on a non-VHE system -- assuming that's something we want to support with this 8.2 feature -- we jump to EL2 and back to set the value? Thanks, -Christoffer
Hi Christoffer, On 30/10/17 10:51, Christoffer Dall wrote: > On Mon, Oct 30, 2017 at 08:59:51AM +0100, Christoffer Dall wrote: >> On Thu, Oct 19, 2017 at 03:58:01PM +0100, James Morse wrote: >>> Prior to v8.2's RAS Extensions, the HCR_EL2.VSE 'virtual SError' feature >>> generated an SError with an implementation defined ESR_EL1.ISS, because we >>> had no mechanism to specify the ESR value. >>> >>> On Juno this generates an all-zero ESR, the most significant bit 'ISV' >>> is clear indicating the remainder of the ISS field is invalid. >>> >>> With the RAS Extensions we have a mechanism to specify this value, and the >>> most significant bit has a new meaning: 'IDS - Implementation Defined >>> Syndrome'. An all-zero SError ESR now means: 'RAS error: Uncategorized' >>> instead of 'no valid ISS'. >>> >>> Add KVM support for the VSESR_EL2 register to specify an ESR value when >>> HCR_EL2.VSE generates a virtual SError. Change kvm_inject_vabt() to >>> specify an implementation-defined value. >>> >>> We only need to restore the VSESR_EL2 value when HCR_EL2.VSE is set, KVM >>> save/restores this bit during __deactivate_traps() and hardware clears the >>> bit once the guest has consumed the virtual-SError. >>> >>> Future patches may add an API (or KVM CAP) to pend a virtual SError with >>> a specified ESR. >>> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c >>> index 945e79c641c4..af37658223a0 100644 >>> --- a/arch/arm64/kvm/hyp/switch.c >>> +++ b/arch/arm64/kvm/hyp/switch.c >>> @@ -86,6 +86,10 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu) >>> isb(); >>> } >>> write_sysreg(val, hcr_el2); >>> + >>> + if (cpus_have_const_cap(ARM64_HAS_RAS_EXTN) && (val & HCR_VSE)) >>> + write_sysreg_s(vcpu->arch.vsesr_el2, SYS_VSESR_EL2); >>> + >> Just a heads up: If my optimization work gets merged, that will >> eventually move stuff like this in to load/put hooks for system >> registers, but I can deal with this easily, also adding a direct write >> in pend_guest_serror when moving the logic around. Sure. This would always be called when the vcpu is loaded, so yes it should end up as a direct write to the system register. >> However, if we start architecting something more complex, it would be >> good to keep in mind how to maintain minimum work on the switching path >> after we've optimized the hypervisor. I think gengdongjiu's trick of only restoring VSESR if HCR_EL2.VSE is set is the best we can do here. (Hence the Celebrate-Contribution tag). For VDISR_EL2 we can probably only save/restore it if its non-zero. On most systems it will never be touched so the cost is testing that whenever we exit the guest/unload the vcpu. > Actually, after thinking about this, if the guest can only see this via > the ESR if we set the HCR_EL2.VSE, wouldn't it make sense to just set > this value in pend_guest_serror, and if we're on a non-VHE system -- > assuming that's something we want to support with this 8.2 feature > -- we jump to EL2 and back to set the value? It thought this was the 'eventually ... direct write' above. Once your load/put hooks are merged? Yes, just write it straight to the CPU register and set the guests HCR_EL2.VSE. Now? Wouldn't this get lost if we reschedule onto another cpu... Thanks, James
On Mon, Oct 30, 2017 at 03:44:17PM +0000, James Morse wrote: > Hi Christoffer, > > On 30/10/17 10:51, Christoffer Dall wrote: > > On Mon, Oct 30, 2017 at 08:59:51AM +0100, Christoffer Dall wrote: > >> On Thu, Oct 19, 2017 at 03:58:01PM +0100, James Morse wrote: > >>> Prior to v8.2's RAS Extensions, the HCR_EL2.VSE 'virtual SError' feature > >>> generated an SError with an implementation defined ESR_EL1.ISS, because we > >>> had no mechanism to specify the ESR value. > >>> > >>> On Juno this generates an all-zero ESR, the most significant bit 'ISV' > >>> is clear indicating the remainder of the ISS field is invalid. > >>> > >>> With the RAS Extensions we have a mechanism to specify this value, and the > >>> most significant bit has a new meaning: 'IDS - Implementation Defined > >>> Syndrome'. An all-zero SError ESR now means: 'RAS error: Uncategorized' > >>> instead of 'no valid ISS'. > >>> > >>> Add KVM support for the VSESR_EL2 register to specify an ESR value when > >>> HCR_EL2.VSE generates a virtual SError. Change kvm_inject_vabt() to > >>> specify an implementation-defined value. > >>> > >>> We only need to restore the VSESR_EL2 value when HCR_EL2.VSE is set, KVM > >>> save/restores this bit during __deactivate_traps() and hardware clears the > >>> bit once the guest has consumed the virtual-SError. > >>> > >>> Future patches may add an API (or KVM CAP) to pend a virtual SError with > >>> a specified ESR. > > > >>> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c > >>> index 945e79c641c4..af37658223a0 100644 > >>> --- a/arch/arm64/kvm/hyp/switch.c > >>> +++ b/arch/arm64/kvm/hyp/switch.c > >>> @@ -86,6 +86,10 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu) > >>> isb(); > >>> } > >>> write_sysreg(val, hcr_el2); > >>> + > >>> + if (cpus_have_const_cap(ARM64_HAS_RAS_EXTN) && (val & HCR_VSE)) > >>> + write_sysreg_s(vcpu->arch.vsesr_el2, SYS_VSESR_EL2); > >>> + > > >> Just a heads up: If my optimization work gets merged, that will > >> eventually move stuff like this in to load/put hooks for system > >> registers, but I can deal with this easily, also adding a direct write > >> in pend_guest_serror when moving the logic around. > > Sure. This would always be called when the vcpu is loaded, so yes it should end > up as a direct write to the system register. > > > >> However, if we start architecting something more complex, it would be > >> good to keep in mind how to maintain minimum work on the switching path > >> after we've optimized the hypervisor. > > I think gengdongjiu's trick of only restoring VSESR if HCR_EL2.VSE is set is the > best we can do here. (Hence the Celebrate-Contribution tag). yes, I agree. > > For VDISR_EL2 we can probably only save/restore it if its non-zero. On most > systems it will never be touched so the cost is testing that whenever we exit > the guest/unload the vcpu. > I think VDISR_EL2 should just be saved/restored in vcpu_put/load after the optimization. > > > Actually, after thinking about this, if the guest can only see this via > > the ESR if we set the HCR_EL2.VSE, wouldn't it make sense to just set > > this value in pend_guest_serror, and if we're on a non-VHE system -- > > assuming that's something we want to support with this 8.2 feature > > -- we jump to EL2 and back to set the value? > > It thought this was the 'eventually ... direct write' above. Yes, that's what I mean. > Once your load/put hooks are merged? Yes, just write it straight to the CPU > register and set the guests HCR_EL2.VSE. > > Now? Wouldn't this get lost if we reschedule onto another cpu... > > That's why we'd also save/restore it in vcpu_put/vcpu_load. So, for VSESR, we'd save/restore it in put/load (conditionally on VSE being set if we like), and we'd also set it from pend_guest_serror. For VDISR, it's just saved/restored in put/load. Thanks, -Christoffer
On Thu, Oct 19 2017 at 4:58:01 pm BST, James Morse <james.morse@arm.com> wrote: > Prior to v8.2's RAS Extensions, the HCR_EL2.VSE 'virtual SError' feature > generated an SError with an implementation defined ESR_EL1.ISS, because we > had no mechanism to specify the ESR value. > > On Juno this generates an all-zero ESR, the most significant bit 'ISV' > is clear indicating the remainder of the ISS field is invalid. > > With the RAS Extensions we have a mechanism to specify this value, and the > most significant bit has a new meaning: 'IDS - Implementation Defined > Syndrome'. An all-zero SError ESR now means: 'RAS error: Uncategorized' > instead of 'no valid ISS'. > > Add KVM support for the VSESR_EL2 register to specify an ESR value when > HCR_EL2.VSE generates a virtual SError. Change kvm_inject_vabt() to > specify an implementation-defined value. > > We only need to restore the VSESR_EL2 value when HCR_EL2.VSE is set, KVM > save/restores this bit during __deactivate_traps() and hardware clears the > bit once the guest has consumed the virtual-SError. > > Future patches may add an API (or KVM CAP) to pend a virtual SError with > a specified ESR. > > Cc: Dongjiu Geng <gengdongjiu@huawei.com> > Signed-off-by: James Morse <james.morse@arm.com> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> M.
diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h index e5df3fce0008..8a7a838eb17a 100644 --- a/arch/arm64/include/asm/kvm_emulate.h +++ b/arch/arm64/include/asm/kvm_emulate.h @@ -61,6 +61,11 @@ static inline void vcpu_set_hcr(struct kvm_vcpu *vcpu, unsigned long hcr) vcpu->arch.hcr_el2 = hcr; } +static inline void vcpu_set_vsesr(struct kvm_vcpu *vcpu, u64 vsesr) +{ + vcpu->arch.vsesr_el2 = vsesr; +} + static inline unsigned long *vcpu_pc(const struct kvm_vcpu *vcpu) { return (unsigned long *)&vcpu_gp_regs(vcpu)->regs.pc; diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index a0e2f7962401..28a4de85edee 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -277,6 +277,9 @@ struct kvm_vcpu_arch { /* Detect first run of a vcpu */ bool has_run_once; + + /* Virtual SError ESR to restore when HCR_EL2.VSE is set */ + u64 vsesr_el2; }; #define vcpu_gp_regs(v) (&(v)->arch.ctxt.gp_regs) diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h index 427c36bc5dd6..a493e93de296 100644 --- a/arch/arm64/include/asm/sysreg.h +++ b/arch/arm64/include/asm/sysreg.h @@ -253,6 +253,7 @@ #define SYS_DACR32_EL2 sys_reg(3, 4, 3, 0, 0) #define SYS_IFSR32_EL2 sys_reg(3, 4, 5, 0, 1) +#define SYS_VSESR_EL2 sys_reg(3, 4, 5, 2, 3) #define SYS_FPEXC32_EL2 sys_reg(3, 4, 5, 3, 0) #define __SYS__AP0Rx_EL2(x) sys_reg(3, 4, 12, 8, x) diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c index 945e79c641c4..af37658223a0 100644 --- a/arch/arm64/kvm/hyp/switch.c +++ b/arch/arm64/kvm/hyp/switch.c @@ -86,6 +86,10 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu) isb(); } write_sysreg(val, hcr_el2); + + if (cpus_have_const_cap(ARM64_HAS_RAS_EXTN) && (val & HCR_VSE)) + write_sysreg_s(vcpu->arch.vsesr_el2, SYS_VSESR_EL2); + /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */ write_sysreg(1 << 15, hstr_el2); /* diff --git a/arch/arm64/kvm/inject_fault.c b/arch/arm64/kvm/inject_fault.c index da6a8cfa54a0..52f7f66f1356 100644 --- a/arch/arm64/kvm/inject_fault.c +++ b/arch/arm64/kvm/inject_fault.c @@ -232,14 +232,25 @@ void kvm_inject_undefined(struct kvm_vcpu *vcpu) inject_undef64(vcpu); } +static void pend_guest_serror(struct kvm_vcpu *vcpu, u64 esr) +{ + vcpu_set_vsesr(vcpu, esr); + vcpu_set_hcr(vcpu, vcpu_get_hcr(vcpu) | HCR_VSE); +} + /** * kvm_inject_vabt - inject an async abort / SError into the guest * @vcpu: The VCPU to receive the exception * * It is assumed that this code is called from the VCPU thread and that the * VCPU therefore is not currently executing guest code. + * + * Systems with the RAS Extensions specify an imp-def ESR (ISV/IDS = 1) with + * the remaining ISS all-zeros so that this error is not interpreted as an + * uncatagorized RAS error. Without the RAS Extensions we can't specify an ESR + * value, so the CPU generates an imp-def value. */ void kvm_inject_vabt(struct kvm_vcpu *vcpu) { - vcpu_set_hcr(vcpu, vcpu_get_hcr(vcpu) | HCR_VSE); + pend_guest_serror(vcpu, ESR_ELx_ISV); }
Prior to v8.2's RAS Extensions, the HCR_EL2.VSE 'virtual SError' feature generated an SError with an implementation defined ESR_EL1.ISS, because we had no mechanism to specify the ESR value. On Juno this generates an all-zero ESR, the most significant bit 'ISV' is clear indicating the remainder of the ISS field is invalid. With the RAS Extensions we have a mechanism to specify this value, and the most significant bit has a new meaning: 'IDS - Implementation Defined Syndrome'. An all-zero SError ESR now means: 'RAS error: Uncategorized' instead of 'no valid ISS'. Add KVM support for the VSESR_EL2 register to specify an ESR value when HCR_EL2.VSE generates a virtual SError. Change kvm_inject_vabt() to specify an implementation-defined value. We only need to restore the VSESR_EL2 value when HCR_EL2.VSE is set, KVM save/restores this bit during __deactivate_traps() and hardware clears the bit once the guest has consumed the virtual-SError. Future patches may add an API (or KVM CAP) to pend a virtual SError with a specified ESR. Cc: Dongjiu Geng <gengdongjiu@huawei.com> Signed-off-by: James Morse <james.morse@arm.com> --- arch/arm64/include/asm/kvm_emulate.h | 5 +++++ arch/arm64/include/asm/kvm_host.h | 3 +++ arch/arm64/include/asm/sysreg.h | 1 + arch/arm64/kvm/hyp/switch.c | 4 ++++ arch/arm64/kvm/inject_fault.c | 13 ++++++++++++- 5 files changed, 25 insertions(+), 1 deletion(-)