| Message ID | 20180205104030.643-1-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@redhat.com> > --- > > 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. > > 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. On the other hand your code is closer to POP.
Really adding Collin... On 02/05/2018 01:46 PM, 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@redhat.com> >> --- >> >> 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. > >> >> 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. > On the other hand your code is closer to POP. >
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@redhat.com> >> --- >> >> 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!
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@redhat.com> > --- > > 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.
>> 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? Yes indeed. > > >> + 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) That leads to ugly long lines (for the comparison and the calculation) > >> + 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! :) > >> + } >> + /* 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. >
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; + 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);
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@redhat.com> --- 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(-)