[PATCH v1] KVM: s390: store DXC/VXC in fpc on DATA/Vector-processing exceptions

[PATCH v1] KVM: s390: store DXC/VXC in fpc on DATA/Vector-processing exceptions

[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).

Signed-off-by: David Hildenbrand <david@redhat.com>
---

Only compile-tested.

 arch/s390/include/asm/ctl_reg.h | 1 +
 arch/s390/kvm/interrupt.c       | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/arch/s390/include/asm/ctl_reg.h b/arch/s390/include/asm/ctl_reg.h
index 4600453536c2..88f3f14baee9 100644
--- a/arch/s390/include/asm/ctl_reg.h
+++ b/arch/s390/include/asm/ctl_reg.h
@@ -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)
diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index fcb55b02990e..5b5754d8f460 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -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;
-- 
2.17.1

Re: [PATCH v1] KVM: s390: store DXC/VXC in fpc on DATA/Vector-processing exceptions

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 2351 bytes --]

On 22.08.2018 11:53, David Hildenbrand wrote:
> 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).
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> 
> 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 <frankja@linux.ibm.com>


> 
>  arch/s390/include/asm/ctl_reg.h | 1 +
>  arch/s390/kvm/interrupt.c       | 8 ++++++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/arch/s390/include/asm/ctl_reg.h b/arch/s390/include/asm/ctl_reg.h
> index 4600453536c2..88f3f14baee9 100644
> --- a/arch/s390/include/asm/ctl_reg.h
> +++ b/arch/s390/include/asm/ctl_reg.h
> @@ -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)
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index fcb55b02990e..5b5754d8f460 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -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?

> +		}
>  		rc = put_guest_lc(vcpu, pgm_info.data_exc_code,
>  				  (u32 *)__LC_DATA_EXC_CODE);
>  		break;
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v1] KVM: s390: store DXC/VXC in fpc on DATA/Vector-processing exceptions

[David Hildenbrand]

On 22.08.2018 12:31, Janosch Frank wrote:
> On 22.08.2018 11:53, David Hildenbrand wrote:
>> 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).
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>
>> 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 <frankja@linux.ibm.com>
> 
> 
>>
>>  arch/s390/include/asm/ctl_reg.h | 1 +
>>  arch/s390/kvm/interrupt.c       | 8 ++++++++
>>  2 files changed, 9 insertions(+)
>>
>> diff --git a/arch/s390/include/asm/ctl_reg.h b/arch/s390/include/asm/ctl_reg.h
>> index 4600453536c2..88f3f14baee9 100644
>> --- a/arch/s390/include/asm/ctl_reg.h
>> +++ b/arch/s390/include/asm/ctl_reg.h
>> @@ -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)
>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>> index fcb55b02990e..5b5754d8f460 100644
>> --- a/arch/s390/kvm/interrupt.c
>> +++ b/arch/s390/kvm/interrupt.c
>> @@ -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?

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!

> 
>> +		}
>>  		rc = put_guest_lc(vcpu, pgm_info.data_exc_code,
>>  				  (u32 *)__LC_DATA_EXC_CODE);
>>  		break;
>>
> 
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH v1] KVM: s390: store DXC/VXC in fpc on DATA/Vector-processing exceptions

[Christian Borntraeger]

On 08/22/2018 11:53 AM, David Hildenbrand wrote:
> 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).
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> 
> 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 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.

Still trying to look further if I missed something.

> 
>  arch/s390/include/asm/ctl_reg.h | 1 +
>  arch/s390/kvm/interrupt.c       | 8 ++++++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/arch/s390/include/asm/ctl_reg.h b/arch/s390/include/asm/ctl_reg.h
> index 4600453536c2..88f3f14baee9 100644
> --- a/arch/s390/include/asm/ctl_reg.h
> +++ b/arch/s390/include/asm/ctl_reg.h
> @@ -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)
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index fcb55b02990e..5b5754d8f460 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -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 ?




> +		}
>  		rc = put_guest_lc(vcpu, pgm_info.data_exc_code,
>  				  (u32 *)__LC_DATA_EXC_CODE);
>  		break;
> 

Re: [PATCH v1] KVM: s390: store DXC/VXC in fpc on DATA/Vector-processing exceptions

[David Hildenbrand]

On 23.08.2018 17:43, Christian Borntraeger wrote:
> 
> 
> On 08/22/2018 11:53 AM, David Hildenbrand wrote:
>> 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).
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>
>> 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 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)

> 
> Still trying to look further if I missed something.
> 
>>
>>  arch/s390/include/asm/ctl_reg.h | 1 +
>>  arch/s390/kvm/interrupt.c       | 8 ++++++++
>>  2 files changed, 9 insertions(+)
>>
>> diff --git a/arch/s390/include/asm/ctl_reg.h b/arch/s390/include/asm/ctl_reg.h
>> index 4600453536c2..88f3f14baee9 100644
>> --- a/arch/s390/include/asm/ctl_reg.h
>> +++ b/arch/s390/include/asm/ctl_reg.h
>> @@ -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)
>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>> index fcb55b02990e..5b5754d8f460 100644
>> --- a/arch/s390/kvm/interrupt.c
>> +++ b/arch/s390/kvm/interrupt.c
>> @@ -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 ?
> 

Sure, didn't know about that.


-- 

Thanks,

David / dhildenb

Re: [PATCH v1] KVM: s390: store DXC/VXC in fpc on DATA/Vector-processing exceptions

[Christian Borntraeger]

On 08/23/2018 07:44 PM, David Hildenbrand wrote:
> On 23.08.2018 17:43, Christian Borntraeger wrote:
>>
>>
>> On 08/22/2018 11:53 AM, David Hildenbrand wrote:
>>> 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).
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>
>>> 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 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.


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. 

> 
> 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.


> 
>>
>> Still trying to look further if I missed something.
>>
>>>
>>>  arch/s390/include/asm/ctl_reg.h | 1 +
>>>  arch/s390/kvm/interrupt.c       | 8 ++++++++
>>>  2 files changed, 9 insertions(+)
>>>
>>> diff --git a/arch/s390/include/asm/ctl_reg.h b/arch/s390/include/asm/ctl_reg.h
>>> index 4600453536c2..88f3f14baee9 100644
>>> --- a/arch/s390/include/asm/ctl_reg.h
>>> +++ b/arch/s390/include/asm/ctl_reg.h
>>> @@ -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)
>>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>>> index fcb55b02990e..5b5754d8f460 100644
>>> --- a/arch/s390/kvm/interrupt.c
>>> +++ b/arch/s390/kvm/interrupt.c
>>> @@ -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 ?
>>
> 
> Sure, didn't know about that.
> 
> 

Re: [PATCH v1] KVM: s390: store DXC/VXC in fpc on DATA/Vector-processing exceptions

[David Hildenbrand]

On 24.08.2018 08:34, Christian Borntraeger wrote:
> 
> 
> On 08/23/2018 07:44 PM, David Hildenbrand wrote:
>> On 23.08.2018 17:43, Christian Borntraeger wrote:
>>>
>>>
>>> On 08/22/2018 11:53 AM, David Hildenbrand wrote:
>>>> 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).
>>>>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>>
>>>> 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 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.
> 
> 
> 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. 
> 
>>
>> 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.

Just tested CRTG, and it also seems to work fine when single-stepping
over it, landing in the PGM handler.

-- 

Thanks,

David / dhildenb