Re: [PATCH v2] KVM: s390: take care of clock-comparator sign control

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

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



[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux