KVM: s390: take care of clock-comparator sign control
PATCH v2 1/1 KVM: s390: take care of clock-comparator sign control
Missed when enabling the Multiple-epoch facility. If the facility is installed and the control is set, a sign based comaprison has to be performed.
Right now we would inject wrong interrupts and ignore interrupt conditions. Also the sleep time is calculated in a wrong way.
---
We might be able to drop the checks for "test_kvm_facility(vcpu->kvm, 139)", as the architecture states:
"When the multiple-epoch facility is not installed in the configuration and the clock-comparator sign control is one, it is unpredictable whether the comparison follows the rules of unsigned or signed binary arithmetic."
Have no machine to test this with :(
arch/s390/kvm/interrupt.c | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-)
arch/s390/kvm/interrupt.c +26 -6
@@ -170,7 +170,16 @@ static int ckc_interrupts_enabled(struct kvm_vcpu *vcpu)static int ckc_irq_pending (struct kvm_vcpu *vcpu) { -if (vcpu->arch.sie_block->ckc >=kvm_s390_get_tod_clock_fast (vcpu->kvm)) +int64_t ckc, tod; + +if (vcpu->arch.sie_block->gcr[0 ] &0x0020000000000000ul && +test_kvm_facility (vcpu->kvm,139 )) { + ckc = vcpu->arch.sie_block->ckc; + tod =kvm_s390_get_tod_clock_fast (vcpu->kvm); +if (ckc >= tod) +return 0 ; + }else if (vcpu->arch.sie_block->ckc >= +kvm_s390_get_tod_clock_fast (vcpu->kvm))return 0 ;return ckc_interrupts_enabled (vcpu); }
@@ -1011,13 +1020,24 @@ int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu)static u64 __calculate_sltime (struct kvm_vcpu *vcpu) { -u64 now, cputm, sltime =0 ; +u64 now, cputm, ckc, sltime =0 ; +int64_t ckc_signed, now_signed;if (ckc_interrupts_enabled (vcpu)) { - now =kvm_s390_get_tod_clock_fast (vcpu->kvm); - sltime =tod_to_ns (vcpu->arch.sie_block->ckc - now); -/* already expired or overflow? */ -if (!sltime || vcpu->arch.sie_block->ckc <= now) +if (vcpu->arch.sie_block->gcr[0 ] &0x0020000000000000ul && +test_kvm_facility (vcpu->kvm,139 )) { + now =kvm_s390_get_tod_clock_fast (vcpu->kvm); + ckc = vcpu->arch.sie_block->ckc; +if (ckc < now) + sltime =tod_to_ns (now - ckc); + }else { + now_signed =kvm_s390_get_tod_clock_fast (vcpu->kvm); + ckc_signed = vcpu->arch.sie_block->ckc; +if (ckc_signed < now_signed) + sltime =tod_to_ns (now_signed - ckc_signed); + } +/* already expired */ +if (!sltime)return 0 ;if (cpu_timer_interrupts_enabled (vcpu)) { cputm =kvm_s390_get_cpu_timer (vcpu);
Signature · 1 line
2.14.3
Adding Collin.
Quoted 10 lines · David Hildenbrand
Missed when enabling the Multiple-epoch facility. If the facility is installed and the control is set, a sign based comaprison has to be performed.
Right now we would inject wrong interrupts and ignore interrupt conditions. Also the sleep time is calculated in a wrong way.
Signed-off-by David Hildenbrand <[email protected]>---
Quoted 8 lines
We might be able to drop the checks for "
test_kvm_facility(vcpu->kvm, 139)", as the architecture states:"When the multiple-epoch facility is not installed in the configuration and the clock-comparator sign control is one, it is unpredictable whether the comparison follows the rules of unsigned or signed binary arithmetic."
Have no machine to test this with :(
It will be somewhat hard to test anyway since the compare only differs for the case where one value (TOD or CKC) is before the 2042 date and the other one is after. have to think about a good test without needing an LPAR that is close to the wraparound.
Quoted 14 lines
arch/s390/kvm/interrupt.c | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-)arch/s390/kvm/interrupt.c +1
-1@@ -170,7 +170,16 @@ static int ckc_interrupts_enabled(struct kvm_vcpu *vcpu)static int ckc_irq_pending (struct kvm_vcpu *vcpu) { -if (vcpu->arch.sie_block->ckc >=kvm_s390_get_tod_clock_fast (vcpu->kvm)) +int64_t ckc, tod;
arch/s390/kvm/interrupt.c static int ckc_interrupts_enabled(struct kvm_vcpu *vcpu) · line 183
Quoted 10 lines
arch/s390/kvm/interrupt.c +9
-0+ +if (vcpu->arch.sie_block->gcr[0 ] &0x0020000000000000ul && +test_kvm_facility (vcpu->kvm,139 )) { + ckc = vcpu->arch.sie_block->ckc; + tod =kvm_s390_get_tod_clock_fast (vcpu->kvm); +if (ckc >= tod) +return 0 ; + }else if (vcpu->arch.sie_block->ckc >= +kvm_s390_get_tod_clock_fast (vcpu->kvm))return 0 ;
Instead of changing the compare depending on another compare, maybe adding 0x8000000000000000 to the unsigned values makes the change easier to grasp. On the other hand your code is closer to POP.
Really adding Collin...
Quoted 56 lines · Christian Borntraeger
Adding Collin.
Quoted 10 lines · David Hildenbrand
Missed when enabling the Multiple-epoch facility. If the facility is installed and the control is set, a sign based comaprison has to be performed.
Right now we would inject wrong interrupts and ignore interrupt conditions. Also the sleep time is calculated in a wrong way.
Signed-off-by David Hildenbrand <[email protected]>---
Quoted 8 lines
We might be able to drop the checks for "
test_kvm_facility(vcpu->kvm, 139)", as the architecture states:"When the multiple-epoch facility is not installed in the configuration and the clock-comparator sign control is one, it is unpredictable whether the comparison follows the rules of unsigned or signed binary arithmetic."
Have no machine to test this with :(
It will be somewhat hard to test anyway since the compare only differs for the case where one value (TOD or CKC) is before the 2042 date and the other one is after. have to think about a good test without needing an LPAR that is close to the wraparound.
Quoted 14 lines
arch/s390/kvm/interrupt.c | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-)arch/s390/kvm/interrupt.c +1
-1@@ -170,7 +170,16 @@ static int ckc_interrupts_enabled(struct kvm_vcpu *vcpu)static int ckc_irq_pending (struct kvm_vcpu *vcpu) { -if (vcpu->arch.sie_block->ckc >=kvm_s390_get_tod_clock_fast (vcpu->kvm)) +int64_t ckc, tod;arch/s390/kvm/interrupt.c static int ckc_interrupts_enabled(struct kvm_vcpu *vcpu) · line 183
Quoted 10 lines
arch/s390/kvm/interrupt.c +9
-0+ +if (vcpu->arch.sie_block->gcr[0 ] &0x0020000000000000ul && +test_kvm_facility (vcpu->kvm,139 )) { + ckc = vcpu->arch.sie_block->ckc; + tod =kvm_s390_get_tod_clock_fast (vcpu->kvm); +if (ckc >= tod) +return 0 ; + }else if (vcpu->arch.sie_block->ckc >= +kvm_s390_get_tod_clock_fast (vcpu->kvm))return 0 ;Instead of changing the compare depending on another compare, maybe adding
0x8000000000000000to the unsigned values makes the change easier to grasp. On the other hand your code is closer to POP.
Quoted 20 lines · Christian Borntraeger
Adding Collin.
Quoted 17 lines · David Hildenbrand
Missed when enabling the Multiple-epoch facility. If the facility is installed and the control is set, a sign based comaprison has to be performed.
Right now we would inject wrong interrupts and ignore interrupt conditions. Also the sleep time is calculated in a wrong way.
Signed-off-by David Hildenbrand <[email protected]>---
We might be able to drop the checks for "
test_kvm_facility(vcpu->kvm, 139)", as the architecture states:"When the multiple-epoch facility is not installed in the configuration and the clock-comparator sign control is one, it is unpredictable whether the comparison follows the rules of unsigned or signed binary arithmetic."
Quoted 7 lines
Quoted 1 line
Have no machine to test this with :(
It will be somewhat hard to test anyway since the compare only differs for the case where one value (TOD or CKC) is before the 2042 date and the other one is after. have to think about a good test without needing an LPAR that is close to the wraparound.
You can set the TOD in the guest to a certain value just before/after overflowing. But you would also need a guest that actually makes use of the overflow bit. Could be done in a unit test (e.g. kvm-unit-tests).
Quoted 19 lines
Quoted 19 lines
arch/s390/kvm/interrupt.c | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-)arch/s390/kvm/interrupt.c +6
-1@@ -170,7 +170,16 @@ static int ckc_interrupts_enabled(struct kvm_vcpu *vcpu)static int ckc_irq_pending (struct kvm_vcpu *vcpu) { -if (vcpu->arch.sie_block->ckc >=kvm_s390_get_tod_clock_fast (vcpu->kvm)) +int64_t ckc, tod; + +if (vcpu->arch.sie_block->gcr[0 ] &0x0020000000000000ul && +test_kvm_facility (vcpu->kvm,139 )) { + ckc = vcpu->arch.sie_block->ckc; + tod =kvm_s390_get_tod_clock_fast (vcpu->kvm);
Quoted 8 lines
arch/s390/kvm/interrupt.c static int ckc_interrupts_enabled(struct kvm_vcpu *vcpu) · line 183
Quoted 5 lines
arch/s390/kvm/interrupt.c +4
-0+if (ckc >= tod) +return 0 ; + }else if (vcpu->arch.sie_block->ckc >= +kvm_s390_get_tod_clock_fast (vcpu->kvm))return 0 ;Instead of changing the compare depending on another compare, maybe adding
0x8000000000000000to the unsigned values makes the change easier to grasp.
Not sure if that is easier to understand, but would also work.
Quoted 2 lines
On the other hand your code is closer to POP.
Thanks!
Signature · 3 lines
Thanks, David / dhildenb
Quoted 8 lines · David Hildenbrand
Missed when enabling the Multiple-epoch facility. If the facility is installed and the control is set, a sign based comaprison has to be performed.
Right now we would inject wrong interrupts and ignore interrupt conditions. Also the sleep time is calculated in a wrong way.
Signed-off-by David Hildenbrand <[email protected]>
Quoted 8 lines
---
We might be able to drop the checks for "
test_kvm_facility(vcpu->kvm, 139)", as the architecture states:"When the multiple-epoch facility is not installed in the configuration and the clock-comparator sign control is one, it is unpredictable whether the comparison follows the rules of unsigned or signed binary arithmetic."
I would drop the MEF check. We only compare the ckc with the 64-bit TOD-Clock regardless if the facility is present or not.
Quoted 35 lines
Have no machine to test this with :(
arch/s390/kvm/interrupt.c | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-)arch/s390/kvm/interrupt.c +12
-2@@ -170,7 +170,16 @@ static int ckc_interrupts_enabled(struct kvm_vcpu *vcpu)static int ckc_irq_pending (struct kvm_vcpu *vcpu) { -if (vcpu->arch.sie_block->ckc >=kvm_s390_get_tod_clock_fast (vcpu->kvm)) +int64_t ckc, tod; + +if (vcpu->arch.sie_block->gcr[0 ] &0x0020000000000000ul && +test_kvm_facility (vcpu->kvm,139 )) { + ckc = vcpu->arch.sie_block->ckc; + tod =kvm_s390_get_tod_clock_fast (vcpu->kvm); +if (ckc >= tod) +return 0 ; + }else if (vcpu->arch.sie_block->ckc >= +kvm_s390_get_tod_clock_fast (vcpu->kvm))return 0 ;return ckc_interrupts_enabled (vcpu); }@@ -1011,13 +1020,24 @@ int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu)static u64 __calculate_sltime (struct kvm_vcpu *vcpu) { -u64 now, cputm, sltime =0 ; +u64 now, cputm, ckc, sltime =0 ; +int64_t ckc_signed, now_signed;
arch/s390/kvm/interrupt.c int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu) · line 1030
Quoted 10 lines
arch/s390/kvm/interrupt.c +4
-4if (ckc_interrupts_enabled (vcpu)) { - now =kvm_s390_get_tod_clock_fast (vcpu->kvm); - sltime =tod_to_ns (vcpu->arch.sie_block->ckc - now); -/* already expired or overflow? */ -if (!sltime || vcpu->arch.sie_block->ckc <= now) +if (vcpu->arch.sie_block->gcr[0 ] &0x0020000000000000ul && +test_kvm_facility (vcpu->kvm,139 )) { + now =kvm_s390_get_tod_clock_fast (vcpu->kvm); + ckc = vcpu->arch.sie_block->ckc;
Shouldn't you be using now_signed and ckc_signed here?
arch/s390/kvm/interrupt.c int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu) · line 1035
Quoted 5 lines
arch/s390/kvm/interrupt.c +5
-0+if (ckc < now) + sltime =tod_to_ns (now - ckc); + }else { + now_signed =kvm_s390_get_tod_clock_fast (vcpu->kvm); + ckc_signed = vcpu->arch.sie_block->ckc;
and the unsigned ones here?
Also you could just compare vcpu->arch.sie_block->ckc and kvm_s390_get_tod_clock_fast(vcpu->kvm)
arch/s390/kvm/interrupt.c int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu) · line 1037
Quoted 2 lines
arch/s390/kvm/interrupt.c +2
-0+if (ckc_signed < now_signed) + sltime =tod_to_ns (now_signed - ckc_signed);
Shouldn't we only calculate sleep time if ckc is greater than now (in both cases)?
arch/s390/kvm/interrupt.c int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu) · line 1043
Quoted 6 lines
arch/s390/kvm/interrupt.c +3
-0+ } +/* already expired */ +if (!sltime)return 0 ;if (cpu_timer_interrupts_enabled (vcpu)) { cputm =kvm_s390_get_cpu_timer (vcpu);
Other than that, this is a heck of a lot easier to read than what we had before.
Signature · 1 line
- Collin L Walling
Quoted 7 lines
Quoted 7 lines
static u64 __calculate_sltime(struct kvm_vcpu *vcpu) {arch/s390/kvm/interrupt.c +2
-1-u64 now, cputm, sltime =0 ; +u64 now, cputm, ckc, sltime =0 ; +int64_t ckc_signed, now_signed;
Quoted 12 lines
arch/s390/kvm/interrupt.c int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu) · line 1030
Quoted 9 lines
if (ckc_interrupts_enabled(vcpu)) {arch/s390/kvm/interrupt.c +4
-4- now =kvm_s390_get_tod_clock_fast (vcpu->kvm); - sltime =tod_to_ns (vcpu->arch.sie_block->ckc - now); -/* already expired or overflow? */ -if (!sltime || vcpu->arch.sie_block->ckc <= now) +if (vcpu->arch.sie_block->gcr[0 ] &0x0020000000000000ul && +test_kvm_facility (vcpu->kvm,139 )) { + now =kvm_s390_get_tod_clock_fast (vcpu->kvm); + ckc = vcpu->arch.sie_block->ckc;Shouldn't you be using
now_signedandckc_signedhere?
Yes indeed.
Quoted 10 lines
arch/s390/kvm/interrupt.c int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu) · line 1035
Quoted 7 lines
arch/s390/kvm/interrupt.c +5
-0+if (ckc < now) + sltime =tod_to_ns (now - ckc); + }else { + now_signed =kvm_s390_get_tod_clock_fast (vcpu->kvm); + ckc_signed = vcpu->arch.sie_block->ckc;and the unsigned ones here?
Quoted 2 lines
Also you could just compare
vcpu->arch.sie_block->ckcandkvm_s390_get_tod_clock_fast(vcpu->kvm)
That leads to ugly long lines (for the comparison and the calculation)
Quoted 8 lines
arch/s390/kvm/interrupt.c int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu) · line 1037
Quoted 3 lines
arch/s390/kvm/interrupt.c +2
-0+if (ckc_signed < now_signed) + sltime =tod_to_ns (now_signed - ckc_signed);Shouldn't we only calculate sleep time if ckc is greater than now (in both cases)?
I think you're right!
(this was a 5min hack originally titled RFC, but I screwed up my command line because I was sending a QEMU patch in between)
Will resend, thanks for having a look! :)
Quoted 11 lines
arch/s390/kvm/interrupt.c int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu) · line 1043
Quoted 7 lines
arch/s390/kvm/interrupt.c +3
-0+ } +/* already expired */ +if (!sltime)return 0 ;if (cpu_timer_interrupts_enabled (vcpu)) { cputm =kvm_s390_get_cpu_timer (vcpu);Other than that, this is a heck of a lot easier to read than what we had before.
Signature · 3 lines
Thanks, David / dhildenb