[RFC,1/6] KVM: s390: take care of clock-comparator sign control
diff mbox

Message ID 20180207114647.6220-2-david@redhat.com
State New
Headers show

Commit Message

David Hildenbrand Feb. 7, 2018, 11:46 a.m. UTC
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>
---
 arch/s390/kvm/interrupt.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

Comments

Collin L. Walling Feb. 7, 2018, 1:47 p.m. UTC | #1
On 02/07/2018 06:46 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>
> ---
>   arch/s390/kvm/interrupt.c | 25 +++++++++++++++++++------
>   1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 3ea9cfa31b16..a616e9b65f91 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -169,8 +169,15 @@ 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))
> +	const u64 now = kvm_s390_get_tod_clock_fast(vcpu->kvm);
> +	const u64 ckc = vcpu->arch.sie_block->ckc;
> +
> +	if (vcpu->arch.sie_block->gcr[0] & 0x0020000000000000ul) {
> +		if ((s64)ckc >= (s64)now)
> +			return 0;
> +	} else if (ckc >= now) {
>   		return 0;
> +	}
>   	return ckc_interrupts_enabled(vcpu);
>   }
>
> @@ -1042,13 +1049,19 @@ int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu)
>
>   static u64 __calculate_sltime(struct kvm_vcpu *vcpu)
>   {
> -	u64 now, cputm, sltime = 0;
> +	const u64 now = kvm_s390_get_tod_clock_fast(vcpu->kvm);
> +	const u64 ckc = vcpu->arch.sie_block->ckc;
> +	u64 cputm, sltime = 0;
>
>   	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) {
> +			if ((s64)now < (s64)ckc)
> +				sltime = tod_to_ns((s64)ckc - (s64)now);
> +		} else if (now < ckc) {
> +			sltime = tod_to_ns(ckc - now);
> +		}
> +		/* already expired */
> +		if (!sltime)
>   			return 0;
>   		if (cpu_timer_interrupts_enabled(vcpu)) {
>   			cputm = kvm_s390_get_cpu_timer(vcpu);
I think it would assist with readability if you defined the sign 
comparison bit. Seeing
something that yells "SIGNED" would make sense as to what's going on here.

Other than that, I don't see anything wrong.

I'll get to reviewing the rest of these patches throughout the day. I 
have to revisit
the docs :)
David Hildenbrand Feb. 7, 2018, 1:58 p.m. UTC | #2
On 07.02.2018 14:47, Collin L. Walling wrote:
> On 02/07/2018 06:46 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>
>> ---
>>   arch/s390/kvm/interrupt.c | 25 +++++++++++++++++++------
>>   1 file changed, 19 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>> index 3ea9cfa31b16..a616e9b65f91 100644
>> --- a/arch/s390/kvm/interrupt.c
>> +++ b/arch/s390/kvm/interrupt.c
>> @@ -169,8 +169,15 @@ 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))
>> +	const u64 now = kvm_s390_get_tod_clock_fast(vcpu->kvm);
>> +	const u64 ckc = vcpu->arch.sie_block->ckc;
>> +
>> +	if (vcpu->arch.sie_block->gcr[0] & 0x0020000000000000ul) {
>> +		if ((s64)ckc >= (s64)now)
>> +			return 0;
>> +	} else if (ckc >= now) {
>>   		return 0;
>> +	}
>>   	return ckc_interrupts_enabled(vcpu);
>>   }
>>
>> @@ -1042,13 +1049,19 @@ int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu)
>>
>>   static u64 __calculate_sltime(struct kvm_vcpu *vcpu)
>>   {
>> -	u64 now, cputm, sltime = 0;
>> +	const u64 now = kvm_s390_get_tod_clock_fast(vcpu->kvm);
>> +	const u64 ckc = vcpu->arch.sie_block->ckc;
>> +	u64 cputm, sltime = 0;
>>
>>   	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) {
>> +			if ((s64)now < (s64)ckc)
>> +				sltime = tod_to_ns((s64)ckc - (s64)now);
>> +		} else if (now < ckc) {
>> +			sltime = tod_to_ns(ckc - now);
>> +		}
>> +		/* already expired */
>> +		if (!sltime)
>>   			return 0;
>>   		if (cpu_timer_interrupts_enabled(vcpu)) {
>>   			cputm = kvm_s390_get_cpu_timer(vcpu);
> I think it would assist with readability if you defined the sign 
> comparison bit. Seeing
> something that yells "SIGNED" would make sense as to what's going on here.

If we want that than I suggest introducing defines for all control
registers we use in kvm code in a separate patch.

> 
> Other than that, I don't see anything wrong.
> 

Thanks!

> I'll get to reviewing the rest of these patches throughout the day. I 
> have to revisit
> the docs :)
>
Christian Borntraeger Feb. 7, 2018, 2:06 p.m. UTC | #3
On 02/07/2018 02:58 PM, David Hildenbrand wrote:
> On 07.02.2018 14:47, Collin L. Walling wrote:
>> On 02/07/2018 06:46 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>
>>> ---
>>>   arch/s390/kvm/interrupt.c | 25 +++++++++++++++++++------
>>>   1 file changed, 19 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>>> index 3ea9cfa31b16..a616e9b65f91 100644
>>> --- a/arch/s390/kvm/interrupt.c
>>> +++ b/arch/s390/kvm/interrupt.c
>>> @@ -169,8 +169,15 @@ 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))
>>> +	const u64 now = kvm_s390_get_tod_clock_fast(vcpu->kvm);
>>> +	const u64 ckc = vcpu->arch.sie_block->ckc;
>>> +
>>> +	if (vcpu->arch.sie_block->gcr[0] & 0x0020000000000000ul) {
>>> +		if ((s64)ckc >= (s64)now)
>>> +			return 0;
>>> +	} else if (ckc >= now) {
>>>   		return 0;
>>> +	}
>>>   	return ckc_interrupts_enabled(vcpu);
>>>   }
>>>
>>> @@ -1042,13 +1049,19 @@ int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu)
>>>
>>>   static u64 __calculate_sltime(struct kvm_vcpu *vcpu)
>>>   {
>>> -	u64 now, cputm, sltime = 0;
>>> +	const u64 now = kvm_s390_get_tod_clock_fast(vcpu->kvm);
>>> +	const u64 ckc = vcpu->arch.sie_block->ckc;
>>> +	u64 cputm, sltime = 0;
>>>
>>>   	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) {
>>> +			if ((s64)now < (s64)ckc)
>>> +				sltime = tod_to_ns((s64)ckc - (s64)now);
>>> +		} else if (now < ckc) {
>>> +			sltime = tod_to_ns(ckc - now);
>>> +		}
>>> +		/* already expired */
>>> +		if (!sltime)
>>>   			return 0;
>>>   		if (cpu_timer_interrupts_enabled(vcpu)) {
>>>   			cputm = kvm_s390_get_cpu_timer(vcpu);
>> I think it would assist with readability if you defined the sign 
>> comparison bit. Seeing
>> something that yells "SIGNED" would make sense as to what's going on here.
> 
> If we want that than I suggest introducing defines for all control
> registers we use in kvm code in a separate patch.

We could add those defines to 
arch/s390/include/asm/ctl_reg.h
Christian Borntraeger Feb. 16, 2018, 9:45 a.m. UTC | #4
On 02/07/2018 12:46 PM, 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>

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>

applied. This will need some more testing but it probably deserves a CC stable.


> ---
>  arch/s390/kvm/interrupt.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 3ea9cfa31b16..a616e9b65f91 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -169,8 +169,15 @@ 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))
> +	const u64 now = kvm_s390_get_tod_clock_fast(vcpu->kvm);
> +	const u64 ckc = vcpu->arch.sie_block->ckc;
> +
> +	if (vcpu->arch.sie_block->gcr[0] & 0x0020000000000000ul) {
> +		if ((s64)ckc >= (s64)now)
> +			return 0;
> +	} else if (ckc >= now) {
>  		return 0;
> +	}
>  	return ckc_interrupts_enabled(vcpu);
>  }
> 
> @@ -1042,13 +1049,19 @@ int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu)
> 
>  static u64 __calculate_sltime(struct kvm_vcpu *vcpu)
>  {
> -	u64 now, cputm, sltime = 0;
> +	const u64 now = kvm_s390_get_tod_clock_fast(vcpu->kvm);
> +	const u64 ckc = vcpu->arch.sie_block->ckc;
> +	u64 cputm, sltime = 0;
> 
>  	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) {
> +			if ((s64)now < (s64)ckc)
> +				sltime = tod_to_ns((s64)ckc - (s64)now);
> +		} else if (now < ckc) {
> +			sltime = tod_to_ns(ckc - now);
> +		}
> +		/* already expired */
> +		if (!sltime)
>  			return 0;
>  		if (cpu_timer_interrupts_enabled(vcpu)) {
>  			cputm = kvm_s390_get_cpu_timer(vcpu);
>

Patch
diff mbox

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 3ea9cfa31b16..a616e9b65f91 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -169,8 +169,15 @@  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))
+	const u64 now = kvm_s390_get_tod_clock_fast(vcpu->kvm);
+	const u64 ckc = vcpu->arch.sie_block->ckc;
+
+	if (vcpu->arch.sie_block->gcr[0] & 0x0020000000000000ul) {
+		if ((s64)ckc >= (s64)now)
+			return 0;
+	} else if (ckc >= now) {
 		return 0;
+	}
 	return ckc_interrupts_enabled(vcpu);
 }
 
@@ -1042,13 +1049,19 @@  int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu)
 
 static u64 __calculate_sltime(struct kvm_vcpu *vcpu)
 {
-	u64 now, cputm, sltime = 0;
+	const u64 now = kvm_s390_get_tod_clock_fast(vcpu->kvm);
+	const u64 ckc = vcpu->arch.sie_block->ckc;
+	u64 cputm, sltime = 0;
 
 	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) {
+			if ((s64)now < (s64)ckc)
+				sltime = tod_to_ns((s64)ckc - (s64)now);
+		} else if (now < ckc) {
+			sltime = tod_to_ns(ckc - now);
+		}
+		/* already expired */
+		if (!sltime)
 			return 0;
 		if (cpu_timer_interrupts_enabled(vcpu)) {
 			cputm = kvm_s390_get_cpu_timer(vcpu);