On 05.02.2018 13:46, Christian Borntraeger wrote: > Adding Collin. > > On 02/05/2018 11: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." >> >> 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). >> >> 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; > > Instead of changing the compare depending on another compare, maybe adding > 0x8000000000000000 to the unsigned values makes the change easier to grasp. Not sure if that is easier to understand, but would also work. > On the other hand your code is closer to POP. > Thanks! -- Thanks, David / dhildenb -- To unsubscribe from this list: send the line "unsubscribe linux-s390" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html