On 02/05/2018 05:40 AM, David Hildenbrand wrote:
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 <david@xxxxxxxxxx>
---
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.
Have no machine to test this with :(
arch/s390/kvm/interrupt.c | 32 ++++++++++++++++++++++++++------
1 file changed, 26 insertions(+), 6 deletions(-)
diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 024ad8bcc516..6566a853c0b8 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -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;
Shouldn't you be using now_signed and ckc_signed here?
+ 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)
+ 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)?
+ }
+ /* 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.
--
- Collin L Walling
