KVM: s390: store DXC/VXC in fpc on DATA/Vector-processing exceptions
PATCH 1/1 KVM: s390: store DXC/VXC in fpc on DATA/Vector-processing exceptions
When DATA exceptions and vector-processing exceptions (program interrupts) are injected, the DXC/VXC is also to be stored in the fpc, if AFP is enabled in CR0.
This can happen inside KVM when reinjecting an interrupt during program interrupt intercepts. These are triggered for example when debugging the guest (concurrent PER events result in an intercept instead of an injection of such interrupts).
---
Only compile-tested.
arch/s390/include/asm/ctl_reg.h | 1 + arch/s390/kvm/interrupt.c | 8 ++++++++ 2 files changed, 9 insertions(+)
arch/s390/include/asm/ctl_reg.h +1 -0
@@ -11,6 +11,7 @@#include <linux/const.h> #define CR0_CLOCK_COMPARATOR_SIGN _BITUL(63 - 10) +#define CR0_AFP_REGISTER_CONTROL _BITUL(63 - 45)#define CR0_EMERGENCY_SIGNAL_SUBMASK _BITUL(63 - 49)#define CR0_EXTERNAL_CALL_SUBMASK _BITUL(63 - 50)#define CR0_CLOCK_COMPARATOR_SUBMASK _BITUL(63 - 52)
arch/s390/kvm/interrupt.c +8 -0
@@ -765,6 +765,14 @@ static int __must_check __deliver_prog(struct kvm_vcpu *vcpu)break ;case PGM_VECTOR_PROCESSING :case PGM_DATA : +if (vcpu->arch.sie_block->gcr[0 ] &CR0_AFP_REGISTER_CONTROL ) { +/* make sure the new fpc will be lazily loaded */ +save_fpu_regs (); +/* the DXC/VXC cannot make the fpc invalid */ + current->thread.fpu.fpc &= ~0xff00u ; + current->thread.fpu.fpc |= (pgm_info.data_exc_code <<8 ) + &0xff00u ; + } rc =put_guest_lc (vcpu, pgm_info.data_exc_code, (u32 *)__LC_DATA_EXC_CODE);break ;
Signature · 1 line
2.17.1
Quoted 9 lines · David Hildenbrand
When DATA exceptions and vector-processing exceptions (program interrupts) are injected, the DXC/VXC is also to be stored in the fpc, if AFP is enabled in CR0.
This can happen inside KVM when reinjecting an interrupt during program interrupt intercepts. These are triggered for example when debugging the guest (concurrent PER events result in an intercept instead of an injection of such interrupts).
Quoted 4 lines
Signed-off-by David Hildenbrand <[email protected]>---
Only compile-tested.
It baffles me that AFP is still a thing in zArch mode. I would have expected it to be a default 1. But then again, I just found out about it.
POP checks out:
Quoted 22 lines
arch/s390/include/asm/ctl_reg.h | 1 + arch/s390/kvm/interrupt.c | 8 ++++++++ 2 files changed, 9 insertions(+)arch/s390/include/asm/ctl_reg.h +1
-0@@ -11,6 +11,7 @@#include <linux/const.h> #define CR0_CLOCK_COMPARATOR_SIGN _BITUL(63 - 10) +#define CR0_AFP_REGISTER_CONTROL _BITUL(63 - 45)#define CR0_EMERGENCY_SIGNAL_SUBMASK _BITUL(63 - 49)#define CR0_EXTERNAL_CALL_SUBMASK _BITUL(63 - 50)#define CR0_CLOCK_COMPARATOR_SUBMASK _BITUL(63 - 52)arch/s390/kvm/interrupt.c +0
-0
arch/s390/kvm/interrupt.c static int __must_check __deliver_prog(struct kvm_vcpu *vcpu) · line 774
Quoted 10 lines
arch/s390/kvm/interrupt.c +7
-0@@ -765,6 +765,14 @@ static int __must_check __deliver_prog(struct kvm_vcpu *vcpu)break ;case PGM_VECTOR_PROCESSING :case PGM_DATA : +if (vcpu->arch.sie_block->gcr[0 ] &CR0_AFP_REGISTER_CONTROL ) { +/* make sure the new fpc will be lazily loaded */ +save_fpu_regs (); +/* the DXC/VXC cannot make the fpc invalid */ + current->thread.fpu.fpc &= ~0xff00u ; + current->thread.fpu.fpc |= (pgm_info.data_exc_code <<8 ) + &0xff00u ;
Everything except that byte should be 0 anyway when it comes from lowcore, why do you mask?
Quoted 5 lines
arch/s390/kvm/interrupt.c +1
-0+ } rc =put_guest_lc (vcpu, pgm_info.data_exc_code, (u32 *)__LC_DATA_EXC_CODE);break ;
Quoted 49 lines · Janosch Frank
Quoted 9 lines · David Hildenbrand
When DATA exceptions and vector-processing exceptions (program interrupts) are injected, the DXC/VXC is also to be stored in the fpc, if AFP is enabled in CR0.
This can happen inside KVM when reinjecting an interrupt during program interrupt intercepts. These are triggered for example when debugging the guest (concurrent PER events result in an intercept instead of an injection of such interrupts).
Quoted 4 lines
Signed-off-by David Hildenbrand <[email protected]>---
Only compile-tested.
It baffles me that AFP is still a thing in zArch mode. I would have expected it to be a default 1. But then again, I just found out about it.
POP checks out:
Reviewed-by Janosch Frank <[email protected]>Quoted 27 lines
arch/s390/include/asm/ctl_reg.h | 1 + arch/s390/kvm/interrupt.c | 8 ++++++++ 2 files changed, 9 insertions(+)arch/s390/include/asm/ctl_reg.h +1
-0@@ -11,6 +11,7 @@#include <linux/const.h> #define CR0_CLOCK_COMPARATOR_SIGN _BITUL(63 - 10) +#define CR0_AFP_REGISTER_CONTROL _BITUL(63 - 45)#define CR0_EMERGENCY_SIGNAL_SUBMASK _BITUL(63 - 49)#define CR0_EXTERNAL_CALL_SUBMASK _BITUL(63 - 50)#define CR0_CLOCK_COMPARATOR_SUBMASK _BITUL(63 - 52)arch/s390/kvm/interrupt.c +2
-0@@ -765,6 +765,14 @@ static int __must_check __deliver_prog(struct kvm_vcpu *vcpu)break ;case PGM_VECTOR_PROCESSING :case PGM_DATA : +if (vcpu->arch.sie_block->gcr[0 ] &CR0_AFP_REGISTER_CONTROL ) { +/* make sure the new fpc will be lazily loaded */
Quoted 8 lines
arch/s390/kvm/interrupt.c static int __must_check __deliver_prog(struct kvm_vcpu *vcpu) · line 774
Quoted 5 lines
arch/s390/kvm/interrupt.c +5
-0+save_fpu_regs (); +/* the DXC/VXC cannot make the fpc invalid */ + current->thread.fpu.fpc &= ~0xff00u ; + current->thread.fpu.fpc |= (pgm_info.data_exc_code <<8 ) + &0xff00u ;Everything except that byte should be 0 anyway when it comes from lowcore, why do you mask?
User space can inject such an interrupt and could therefore set reserved bits, leading to a crash (fpc invalid when loaded). E.g. via kvm_s390_set_irq_state().
As of now, DXC/VXC is always 1byte. The other ones are to be stored as
0. (what we expect to always hold for now but user space can do crazy things)
Thanks!
Quoted 8 lines
Quoted 6 lines
arch/s390/kvm/interrupt.c +1
-0+ } rc =put_guest_lc (vcpu, pgm_info.data_exc_code, (u32 *)__LC_DATA_EXC_CODE);break ;
Signature · 3 lines
Thanks, David / dhildenb
Quoted 9 lines · David Hildenbrand
When DATA exceptions and vector-processing exceptions (program interrupts) are injected, the DXC/VXC is also to be stored in the fpc, if AFP is enabled in CR0.
This can happen inside KVM when reinjecting an interrupt during program interrupt intercepts. These are triggered for example when debugging the guest (concurrent PER events result in an intercept instead of an injection of such interrupts).
Quoted 4 lines
Signed-off-by David Hildenbrand <[email protected]>---
Only compile-tested.
I checked the Linux code (arch/s390/kernel/traps.c) and Linux uses the FPC (and not the lowcore field) to decide about the signal (SIGFPE) and si_code. So we want to have the correct DXC/VXC value.
Now, I wrote a short test program that doesfeenableexcept(FE_DIVBYZERO);
and a division by zero.
and attached gdb to that guest together with a breakpoint on the divide (and the instruction after).
I get the pint exit for the instruction after (as it is suppressing) and at this point in time the guest fpc already contains the correct DXC value. So you patch will certainly not hurt, but it seems not necessary.
Still trying to look further if I missed something.
Quoted 22 lines
arch/s390/include/asm/ctl_reg.h | 1 + arch/s390/kvm/interrupt.c | 8 ++++++++ 2 files changed, 9 insertions(+)arch/s390/include/asm/ctl_reg.h +1
-0@@ -11,6 +11,7 @@#include <linux/const.h> #define CR0_CLOCK_COMPARATOR_SIGN _BITUL(63 - 10) +#define CR0_AFP_REGISTER_CONTROL _BITUL(63 - 45)#define CR0_EMERGENCY_SIGNAL_SUBMASK _BITUL(63 - 49)#define CR0_EXTERNAL_CALL_SUBMASK _BITUL(63 - 50)#define CR0_CLOCK_COMPARATOR_SUBMASK _BITUL(63 - 52)arch/s390/kvm/interrupt.c +0
-0
arch/s390/kvm/interrupt.c static int __must_check __deliver_prog(struct kvm_vcpu *vcpu) · line 774
Quoted 10 lines
arch/s390/kvm/interrupt.c +7
-0@@ -765,6 +765,14 @@ static int __must_check __deliver_prog(struct kvm_vcpu *vcpu)break ;case PGM_VECTOR_PROCESSING :case PGM_DATA : +if (vcpu->arch.sie_block->gcr[0 ] &CR0_AFP_REGISTER_CONTROL ) { +/* make sure the new fpc will be lazily loaded */ +save_fpu_regs (); +/* the DXC/VXC cannot make the fpc invalid */ + current->thread.fpu.fpc &= ~0xff00u ; + current->thread.fpu.fpc |= (pgm_info.data_exc_code <<8 ) + &0xff00u ;
maybe reuse FPC_DXC_MASK instead of 0xff00 ?
Quoted 5 lines
arch/s390/kvm/interrupt.c +1
-0+ } rc =put_guest_lc (vcpu, pgm_info.data_exc_code, (u32 *)__LC_DATA_EXC_CODE);break ;
Quoted 21 lines · Christian Borntraeger
Quoted 9 lines · David Hildenbrand
When DATA exceptions and vector-processing exceptions (program interrupts) are injected, the DXC/VXC is also to be stored in the fpc, if AFP is enabled in CR0.
This can happen inside KVM when reinjecting an interrupt during program interrupt intercepts. These are triggered for example when debugging the guest (concurrent PER events result in an intercept instead of an injection of such interrupts).
Quoted 4 lines
Signed-off-by David Hildenbrand <[email protected]>---
Only compile-tested.
I checked the Linux code (
arch/s390/kernel/traps.c) and Linux uses the FPC (and not the lowcore field) to decide about the signal (SIGFPE) andsi_code. So we want to have the correct DXC/VXC value.
Quoted 8 lines
Now, I wrote a short test program that does
feenableexcept(FE_DIVBYZERO);
and a division by zero.
and attached gdb to that guest together with a breakpoint on the divide (and the instruction after).
I get the pint exit for the instruction after (as it is suppressing) and at this point in time the guest fpc already contains the correct DXC value. So you patch will certainly not hurt, but it seems not necessary.
Thanks for trying. Wonder if that is documented behavior or just works by pure luck.
E.g. it would be interesting to see what other instructions do that usually don't touch the DXC, except when injecting an exception. E.g. CRT.
But if you believe this is not needed, we can also drop it. (if ever somebody would want to inject from QEMU, he could also just set the fpc directly)
Quoted 29 lines
Still trying to look further if I missed something.
Quoted 26 lines
arch/s390/include/asm/ctl_reg.h | 1 + arch/s390/kvm/interrupt.c | 8 ++++++++ 2 files changed, 9 insertions(+)arch/s390/include/asm/ctl_reg.h +1
-0@@ -11,6 +11,7 @@#include <linux/const.h> #define CR0_CLOCK_COMPARATOR_SIGN _BITUL(63 - 10) +#define CR0_AFP_REGISTER_CONTROL _BITUL(63 - 45)#define CR0_EMERGENCY_SIGNAL_SUBMASK _BITUL(63 - 49)#define CR0_EXTERNAL_CALL_SUBMASK _BITUL(63 - 50)#define CR0_CLOCK_COMPARATOR_SUBMASK _BITUL(63 - 52)arch/s390/kvm/interrupt.c +1
-0@@ -765,6 +765,14 @@ static int __must_check __deliver_prog(struct kvm_vcpu *vcpu)break ;case PGM_VECTOR_PROCESSING :case PGM_DATA : +if (vcpu->arch.sie_block->gcr[0 ] &CR0_AFP_REGISTER_CONTROL ) {
Quoted 9 lines
arch/s390/kvm/interrupt.c static int __must_check __deliver_prog(struct kvm_vcpu *vcpu) · line 774
Quoted 6 lines
arch/s390/kvm/interrupt.c +6
-0+/* make sure the new fpc will be lazily loaded */ +save_fpu_regs (); +/* the DXC/VXC cannot make the fpc invalid */ + current->thread.fpu.fpc &= ~0xff00u ; + current->thread.fpu.fpc |= (pgm_info.data_exc_code <<8 ) + &0xff00u ;maybe reuse
FPC_DXC_MASKinstead of0xff00?
Sure, didn't know about that.
Signature · 3 lines
Thanks, David / dhildenb
Quoted 31 lines · David Hildenbrand
Quoted 21 lines · Christian Borntraeger
Quoted 9 lines · David Hildenbrand
When DATA exceptions and vector-processing exceptions (program interrupts) are injected, the DXC/VXC is also to be stored in the fpc, if AFP is enabled in CR0.
This can happen inside KVM when reinjecting an interrupt during program interrupt intercepts. These are triggered for example when debugging the guest (concurrent PER events result in an intercept instead of an injection of such interrupts).
Quoted 4 lines
Signed-off-by David Hildenbrand <[email protected]>---
Only compile-tested.
I checked the Linux code (
arch/s390/kernel/traps.c) and Linux uses the FPC (and not the lowcore field) to decide about the signal (SIGFPE) andsi_code. So we want to have the correct DXC/VXC value.Quoted 8 lines
Now, I wrote a short test program that does
feenableexcept(FE_DIVBYZERO);
and a division by zero.
and attached gdb to that guest together with a breakpoint on the divide (and the instruction after).
I get the pint exit for the instruction after (as it is suppressing) and at this point in time the guest fpc already contains the correct DXC value. So you patch will certainly not hurt, but it seems not necessary.
Quoted 2 lines
Thanks for trying. Wonder if that is documented behavior or just works by pure luck.
My guess is, that this is works as designed. There is the interruption parameter block that is used instead of the guest lowcore for program interrupt exits. To me it looks like that everything is "prepared" except for the psw swap itself and the data in the lowcore. The data is written to the interruption parameter block instead. So that the hypervisor then just has to move the data and do the psw swap.
Quoted 7 lines
E.g. it would be interesting to see what other instructions do that usually don't touch the DXC, except when injecting an exception. E.g. CRT.
But if you believe this is not needed, we can also drop it. (if ever somebody would want to inject from QEMU, he could also just set the fpc directly)
The (unlikely to ever happen) inject from QEMU is indeed a thing where this patch would simplify things.
I will talk to some hardware folks to verify my assumption but for the time being, lets drop this patch.
Quoted 43 lines
Quoted 30 lines
Still trying to look further if I missed something.
Quoted 26 lines
arch/s390/include/asm/ctl_reg.h | 1 + arch/s390/kvm/interrupt.c | 8 ++++++++ 2 files changed, 9 insertions(+)arch/s390/include/asm/ctl_reg.h +1
-0@@ -11,6 +11,7 @@#include <linux/const.h> #define CR0_CLOCK_COMPARATOR_SIGN _BITUL(63 - 10) +#define CR0_AFP_REGISTER_CONTROL _BITUL(63 - 45)#define CR0_EMERGENCY_SIGNAL_SUBMASK _BITUL(63 - 49)#define CR0_EXTERNAL_CALL_SUBMASK _BITUL(63 - 50)#define CR0_CLOCK_COMPARATOR_SUBMASK _BITUL(63 - 52)arch/s390/kvm/interrupt.c +1
-0@@ -765,6 +765,14 @@ static int __must_check __deliver_prog(struct kvm_vcpu *vcpu)break ;case PGM_VECTOR_PROCESSING :case PGM_DATA : +if (vcpu->arch.sie_block->gcr[0 ] &CR0_AFP_REGISTER_CONTROL ) {Quoted 9 lines
arch/s390/kvm/interrupt.c static int __must_check __deliver_prog(struct kvm_vcpu *vcpu) · line 774
Quoted 6 lines
arch/s390/kvm/interrupt.c +6
-0+/* make sure the new fpc will be lazily loaded */ +save_fpu_regs (); +/* the DXC/VXC cannot make the fpc invalid */ + current->thread.fpu.fpc &= ~0xff00u ; + current->thread.fpu.fpc |= (pgm_info.data_exc_code <<8 ) + &0xff00u ;maybe reuse
FPC_DXC_MASKinstead of0xff00?Sure, didn't know about that.
Quoted 53 lines · Christian Borntraeger
Quoted 31 lines · David Hildenbrand
Quoted 21 lines · Christian Borntraeger
Quoted 9 lines · David Hildenbrand
When DATA exceptions and vector-processing exceptions (program interrupts) are injected, the DXC/VXC is also to be stored in the fpc, if AFP is enabled in CR0.
This can happen inside KVM when reinjecting an interrupt during program interrupt intercepts. These are triggered for example when debugging the guest (concurrent PER events result in an intercept instead of an injection of such interrupts).
Quoted 4 lines
Signed-off-by David Hildenbrand <[email protected]>---
Only compile-tested.
I checked the Linux code (
arch/s390/kernel/traps.c) and Linux uses the FPC (and not the lowcore field) to decide about the signal (SIGFPE) andsi_code. So we want to have the correct DXC/VXC value.Quoted 8 lines
Now, I wrote a short test program that does
feenableexcept(FE_DIVBYZERO);
and a division by zero.
and attached gdb to that guest together with a breakpoint on the divide (and the instruction after).
I get the pint exit for the instruction after (as it is suppressing) and at this point in time the guest fpc already contains the correct DXC value. So you patch will certainly not hurt, but it seems not necessary.Quoted 2 lines
Thanks for trying. Wonder if that is documented behavior or just works by pure luck.
My guess is, that this is works as designed. There is the interruption parameter block that is used instead of the guest lowcore for program interrupt exits. To me it looks like that everything is "prepared" except for the psw swap itself and the data in the lowcore. The data is written to the interruption parameter block instead. So that the hypervisor then just has to move the data and do the psw swap.
Quoted 7 lines
E.g. it would be interesting to see what other instructions do that usually don't touch the DXC, except when injecting an exception. E.g. CRT.
But if you believe this is not needed, we can also drop it. (if ever somebody would want to inject from QEMU, he could also just set the fpc directly)
Quoted 5 lines
The (unlikely to ever happen) inject from QEMU is indeed a thing where this patch would simplify things.
I will talk to some hardware folks to verify my assumption but for the time being, lets drop this patch.
Just tested CRTG, and it also seems to work fine when single-stepping over it, landing in the PGM handler.
Signature · 3 lines
Thanks, David / dhildenb