[PATCH RFC 0/6] KVM: s390: Multiple-epoch facility fixes

[PATCH RFC 0/6] KVM: s390: Multiple-epoch facility fixes

[David Hildenbrand]

It think I found various BUGs in the recently added Multiple-epoch facility
support in KVM. Along with it, two cleanups.

1. The clock-comparator sign control is not considered. I sent a patch
   with this previously. This now contains fixes and simplifications.
2. SET CLOCK from the guest does not work reliably if the facility is
   enabled (epoch index not set).
3. Hotplugged CPUs don't inherit the epoch index.
4. TOD clock syncs don't take care of overflows/underflows in the epoch
   value and miss to update the epoch index.


This is RFC as I have basically no machine to test. Hopefully somebody
can jump in and verify that we now handle the epoch index in all
scenarios correctly.

Most importantly, with Multiple-epoch facility, the condition
	Guest TOD = Host TOD - 1
Is represented by both, epoch and epoch_idx containing 0xff. So it is
treated as a 64+8bit signed number - we have to properly take care
of over/underflows when modifying the epoch.


David Hildenbrand (6):
  KVM: s390: take care of clock-comparator sign control
  KVM: s390: provide only a single function for setting the tod
  KVM: s390: consider epoch index on hotplugged CPUs
  KVM: s390: consider epoch index on TOD clock syncs
  KVM: s390: no need to inititalize kvm->arch members to 0
  KVM: s390: generalize kvm_s390_get_tod_clock_ext()

 arch/s390/kvm/interrupt.c |  25 ++++++++---
 arch/s390/kvm/kvm-s390.c  | 109 +++++++++++++++++++++++-----------------------
 arch/s390/kvm/kvm-s390.h  |   5 +--
 arch/s390/kvm/priv.c      |   9 ++--
 4 files changed, 80 insertions(+), 68 deletions(-)

-- 
2.14.3

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

[David Hildenbrand]

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);
-- 
2.14.3

Re: [PATCH RFC 1/6] KVM: s390: take care of clock-comparator sign control

[Collin L. Walling]

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

-- 
- Collin L Walling

Re: [PATCH RFC 1/6] KVM: s390: take care of clock-comparator sign control

[David Hildenbrand]

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


-- 

Thanks,

David / dhildenb

Re: [PATCH RFC 1/6] KVM: s390: take care of clock-comparator sign control

[Christian Borntraeger]

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

Re: [PATCH RFC 1/6] KVM: s390: take care of clock-comparator sign control

[Christian Borntraeger]

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 RFC 2/6] KVM: s390: provide only a single function for setting the tod

[David Hildenbrand]

Right now, SET CLOCK called in the guest does not properly take care of
the epoch index, as the call goes via the old kvm_s390_set_tod_clock()
interface. So the epoch index is neither reset to 0, if required, nor
properly set to e.g. 0xff on negative values.

Fix this by providing a single kvm_s390_set_tod_clock() function. Move
Multiple-epoch facility handling into it.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/s390/kvm/kvm-s390.c | 46 +++++++++++++++-------------------------------
 arch/s390/kvm/kvm-s390.h |  5 ++---
 arch/s390/kvm/priv.c     |  9 +++++----
 3 files changed, 22 insertions(+), 38 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index ba4c7092335a..b514ee427077 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -902,12 +902,9 @@ static int kvm_s390_set_tod_ext(struct kvm *kvm, struct kvm_device_attr *attr)
 	if (copy_from_user(&gtod, (void __user *)attr->addr, sizeof(gtod)))
 		return -EFAULT;
 
-	if (test_kvm_facility(kvm, 139))
-		kvm_s390_set_tod_clock_ext(kvm, &gtod);
-	else if (gtod.epoch_idx == 0)
-		kvm_s390_set_tod_clock(kvm, gtod.tod);
-	else
+	if (!test_kvm_facility(kvm, 139) && gtod.epoch_idx)
 		return -EINVAL;
+	kvm_s390_set_tod_clock(kvm, &gtod);
 
 	VM_EVENT(kvm, 3, "SET: TOD extension: 0x%x, TOD base: 0x%llx",
 		gtod.epoch_idx, gtod.tod);
@@ -932,13 +929,14 @@ static int kvm_s390_set_tod_high(struct kvm *kvm, struct kvm_device_attr *attr)
 
 static int kvm_s390_set_tod_low(struct kvm *kvm, struct kvm_device_attr *attr)
 {
-	u64 gtod;
+	struct kvm_s390_vm_tod_clock gtod = { 0 };
 
-	if (copy_from_user(&gtod, (void __user *)attr->addr, sizeof(gtod)))
+	if (copy_from_user(&gtod.tod, (void __user *)attr->addr,
+			   sizeof(gtod.tod)))
 		return -EFAULT;
 
-	kvm_s390_set_tod_clock(kvm, gtod);
-	VM_EVENT(kvm, 3, "SET: TOD base: 0x%llx", gtod);
+	kvm_s390_set_tod_clock(kvm, &gtod);
+	VM_EVENT(kvm, 3, "SET: TOD base: 0x%llx", gtod.tod);
 	return 0;
 }
 
@@ -3021,8 +3019,8 @@ static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
-void kvm_s390_set_tod_clock_ext(struct kvm *kvm,
-				 const struct kvm_s390_vm_tod_clock *gtod)
+void kvm_s390_set_tod_clock(struct kvm *kvm,
+			    const struct kvm_s390_vm_tod_clock *gtod)
 {
 	struct kvm_vcpu *vcpu;
 	struct kvm_s390_tod_clock_ext htod;
@@ -3034,10 +3032,12 @@ void kvm_s390_set_tod_clock_ext(struct kvm *kvm,
 	get_tod_clock_ext((char *)&htod);
 
 	kvm->arch.epoch = gtod->tod - htod.tod;
-	kvm->arch.epdx = gtod->epoch_idx - htod.epoch_idx;
-
-	if (kvm->arch.epoch > gtod->tod)
-		kvm->arch.epdx -= 1;
+	kvm->arch.epdx = 0;
+	if (test_kvm_facility(kvm, 139)) {
+		kvm->arch.epdx = gtod->epoch_idx - htod.epoch_idx;
+		if (kvm->arch.epoch > gtod->tod)
+			kvm->arch.epdx -= 1;
+	}
 
 	kvm_s390_vcpu_block_all(kvm);
 	kvm_for_each_vcpu(i, vcpu, kvm) {
@@ -3050,22 +3050,6 @@ void kvm_s390_set_tod_clock_ext(struct kvm *kvm,
 	mutex_unlock(&kvm->lock);
 }
 
-void kvm_s390_set_tod_clock(struct kvm *kvm, u64 tod)
-{
-	struct kvm_vcpu *vcpu;
-	int i;
-
-	mutex_lock(&kvm->lock);
-	preempt_disable();
-	kvm->arch.epoch = tod - get_tod_clock();
-	kvm_s390_vcpu_block_all(kvm);
-	kvm_for_each_vcpu(i, vcpu, kvm)
-		vcpu->arch.sie_block->epoch = kvm->arch.epoch;
-	kvm_s390_vcpu_unblock_all(kvm);
-	preempt_enable();
-	mutex_unlock(&kvm->lock);
-}
-
 /**
  * kvm_arch_fault_in_page - fault-in guest page if necessary
  * @vcpu: The corresponding virtual cpu
diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
index bd31b37b0e6f..19d719262765 100644
--- a/arch/s390/kvm/kvm-s390.h
+++ b/arch/s390/kvm/kvm-s390.h
@@ -283,9 +283,8 @@ int kvm_s390_handle_sigp(struct kvm_vcpu *vcpu);
 int kvm_s390_handle_sigp_pei(struct kvm_vcpu *vcpu);
 
 /* implemented in kvm-s390.c */
-void kvm_s390_set_tod_clock_ext(struct kvm *kvm,
-				 const struct kvm_s390_vm_tod_clock *gtod);
-void kvm_s390_set_tod_clock(struct kvm *kvm, u64 tod);
+void kvm_s390_set_tod_clock(struct kvm *kvm,
+			    const struct kvm_s390_vm_tod_clock *gtod);
 long kvm_arch_fault_in_page(struct kvm_vcpu *vcpu, gpa_t gpa, int writable);
 int kvm_s390_store_status_unloaded(struct kvm_vcpu *vcpu, unsigned long addr);
 int kvm_s390_vcpu_store_status(struct kvm_vcpu *vcpu, unsigned long addr);
diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
index c4c4e157c036..33505c32c48a 100644
--- a/arch/s390/kvm/priv.c
+++ b/arch/s390/kvm/priv.c
@@ -85,9 +85,10 @@ int kvm_s390_handle_e3(struct kvm_vcpu *vcpu)
 /* Handle SCK (SET CLOCK) interception */
 static int handle_set_clock(struct kvm_vcpu *vcpu)
 {
+	struct kvm_s390_vm_tod_clock gtod = { 0 };
 	int rc;
 	u8 ar;
-	u64 op2, val;
+	u64 op2;
 
 	vcpu->stat.instruction_sck++;
 
@@ -97,12 +98,12 @@ static int handle_set_clock(struct kvm_vcpu *vcpu)
 	op2 = kvm_s390_get_base_disp_s(vcpu, &ar);
 	if (op2 & 7)	/* Operand must be on a doubleword boundary */
 		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
-	rc = read_guest(vcpu, op2, ar, &val, sizeof(val));
+	rc = read_guest(vcpu, op2, ar, &gtod.tod, sizeof(gtod.tod));
 	if (rc)
 		return kvm_s390_inject_prog_cond(vcpu, rc);
 
-	VCPU_EVENT(vcpu, 3, "SCK: setting guest TOD to 0x%llx", val);
-	kvm_s390_set_tod_clock(vcpu->kvm, val);
+	VCPU_EVENT(vcpu, 3, "SCK: setting guest TOD to 0x%llx", gtod.tod);
+	kvm_s390_set_tod_clock(vcpu->kvm, &gtod);
 
 	kvm_s390_set_psw_cc(vcpu, 0);
 	return 0;
-- 
2.14.3

Re: [PATCH RFC 2/6] KVM: s390: provide only a single function for setting the tod

[Collin L. Walling]

On 02/07/2018 06:46 AM, David Hildenbrand wrote:
> Right now, SET CLOCK called in the guest does not properly take care of
> the epoch index, as the call goes via the old kvm_s390_set_tod_clock()
> interface. So the epoch index is neither reset to 0, if required, nor
> properly set to e.g. 0xff on negative values.
>
> Fix this by providing a single kvm_s390_set_tod_clock() function. Move
> Multiple-epoch facility handling into it.
>
> Signed-off-by: David Hildenbrand<david@redhat.com>
> ---
>   arch/s390/kvm/kvm-s390.c | 46 +++++++++++++++-------------------------------
>   arch/s390/kvm/kvm-s390.h |  5 ++---
>   arch/s390/kvm/priv.c     |  9 +++++----
>   3 files changed, 22 insertions(+), 38 deletions(-)
>
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index ba4c7092335a..b514ee427077 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -902,12 +902,9 @@ static int kvm_s390_set_tod_ext(struct kvm *kvm, struct kvm_device_attr *attr)
>   	if (copy_from_user(&gtod, (void __user *)attr->addr, sizeof(gtod)))
>   		return -EFAULT;
>
> -	if (test_kvm_facility(kvm, 139))
> -		kvm_s390_set_tod_clock_ext(kvm, &gtod);
> -	else if (gtod.epoch_idx == 0)
> -		kvm_s390_set_tod_clock(kvm, gtod.tod);
> -	else
> +	if (!test_kvm_facility(kvm, 139) && gtod.epoch_idx)
>   		return -EINVAL;
> +	kvm_s390_set_tod_clock(kvm, &gtod);
>
>   	VM_EVENT(kvm, 3, "SET: TOD extension: 0x%x, TOD base: 0x%llx",
>   		gtod.epoch_idx, gtod.tod);
> @@ -932,13 +929,14 @@ static int kvm_s390_set_tod_high(struct kvm *kvm, struct kvm_device_attr *attr)
>
>   static int kvm_s390_set_tod_low(struct kvm *kvm, struct kvm_device_attr *attr)
>   {
> -	u64 gtod;
> +	struct kvm_s390_vm_tod_clock gtod = { 0 };
>
> -	if (copy_from_user(&gtod, (void __user *)attr->addr, sizeof(gtod)))
> +	if (copy_from_user(&gtod.tod, (void __user *)attr->addr,
> +			   sizeof(gtod.tod)))
>   		return -EFAULT;
>
> -	kvm_s390_set_tod_clock(kvm, gtod);
> -	VM_EVENT(kvm, 3, "SET: TOD base: 0x%llx", gtod);
> +	kvm_s390_set_tod_clock(kvm, &gtod);
> +	VM_EVENT(kvm, 3, "SET: TOD base: 0x%llx", gtod.tod);
>   	return 0;
>   }
>
> @@ -3021,8 +3019,8 @@ static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu)
>   	return 0;
>   }
>
> -void kvm_s390_set_tod_clock_ext(struct kvm *kvm,
> -				 const struct kvm_s390_vm_tod_clock *gtod)
> +void kvm_s390_set_tod_clock(struct kvm *kvm,
> +			    const struct kvm_s390_vm_tod_clock *gtod)
Nit: off by one space --------^

>   {
>   	struct kvm_vcpu *vcpu;
>   	struct kvm_s390_tod_clock_ext htod;
> @@ -3034,10 +3032,12 @@ void kvm_s390_set_tod_clock_ext(struct kvm *kvm,
>   	get_tod_clock_ext((char *)&htod);
>
>   	kvm->arch.epoch = gtod->tod - htod.tod;
> -	kvm->arch.epdx = gtod->epoch_idx - htod.epoch_idx;
> -
> -	if (kvm->arch.epoch > gtod->tod)
> -		kvm->arch.epdx -= 1;
> +	kvm->arch.epdx = 0;
> +	if (test_kvm_facility(kvm, 139)) {
> +		kvm->arch.epdx = gtod->epoch_idx - htod.epoch_idx;
> +		if (kvm->arch.epoch > gtod->tod)
> +			kvm->arch.epdx -= 1;
> +	}
>
>   	kvm_s390_vcpu_block_all(kvm);
>   	kvm_for_each_vcpu(i, vcpu, kvm) {
> @@ -3050,22 +3050,6 @@ void kvm_s390_set_tod_clock_ext(struct kvm *kvm,
>   	mutex_unlock(&kvm->lock);
>   }
>
> -void kvm_s390_set_tod_clock(struct kvm *kvm, u64 tod)
> -{
> -	struct kvm_vcpu *vcpu;
> -	int i;
> -
> -	mutex_lock(&kvm->lock);
> -	preempt_disable();
> -	kvm->arch.epoch = tod - get_tod_clock();
> -	kvm_s390_vcpu_block_all(kvm);
> -	kvm_for_each_vcpu(i, vcpu, kvm)
> -		vcpu->arch.sie_block->epoch = kvm->arch.epoch;
> -	kvm_s390_vcpu_unblock_all(kvm);
> -	preempt_enable();
> -	mutex_unlock(&kvm->lock);
> -}
> -
>   /**
>    * kvm_arch_fault_in_page - fault-in guest page if necessary
>    * @vcpu: The corresponding virtual cpu
> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
> index bd31b37b0e6f..19d719262765 100644
> --- a/arch/s390/kvm/kvm-s390.h
> +++ b/arch/s390/kvm/kvm-s390.h
> @@ -283,9 +283,8 @@ int kvm_s390_handle_sigp(struct kvm_vcpu *vcpu);
>   int kvm_s390_handle_sigp_pei(struct kvm_vcpu *vcpu);
>
>   /* implemented in kvm-s390.c */
> -void kvm_s390_set_tod_clock_ext(struct kvm *kvm,
> -				 const struct kvm_s390_vm_tod_clock *gtod);
> -void kvm_s390_set_tod_clock(struct kvm *kvm, u64 tod);
> +void kvm_s390_set_tod_clock(struct kvm *kvm,
> +			    const struct kvm_s390_vm_tod_clock *gtod);
Same nit here.
>   long kvm_arch_fault_in_page(struct kvm_vcpu *vcpu, gpa_t gpa, int writable);
>   int kvm_s390_store_status_unloaded(struct kvm_vcpu *vcpu, unsigned long addr);
>   int kvm_s390_vcpu_store_status(struct kvm_vcpu *vcpu, unsigned long addr);
> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
> index c4c4e157c036..33505c32c48a 100644
> --- a/arch/s390/kvm/priv.c
> +++ b/arch/s390/kvm/priv.c
> @@ -85,9 +85,10 @@ int kvm_s390_handle_e3(struct kvm_vcpu *vcpu)
>   /* Handle SCK (SET CLOCK) interception */
>   static int handle_set_clock(struct kvm_vcpu *vcpu)
>   {
> +	struct kvm_s390_vm_tod_clock gtod = { 0 };
>   	int rc;
>   	u8 ar;
> -	u64 op2, val;
> +	u64 op2;
>
>   	vcpu->stat.instruction_sck++;
>
> @@ -97,12 +98,12 @@ static int handle_set_clock(struct kvm_vcpu *vcpu)
>   	op2 = kvm_s390_get_base_disp_s(vcpu, &ar);
>   	if (op2 & 7)	/* Operand must be on a doubleword boundary */
>   		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
> -	rc = read_guest(vcpu, op2, ar, &val, sizeof(val));
> +	rc = read_guest(vcpu, op2, ar, &gtod.tod, sizeof(gtod.tod));
>   	if (rc)
>   		return kvm_s390_inject_prog_cond(vcpu, rc);
>
> -	VCPU_EVENT(vcpu, 3, "SCK: setting guest TOD to 0x%llx", val);
> -	kvm_s390_set_tod_clock(vcpu->kvm, val);
> +	VCPU_EVENT(vcpu, 3, "SCK: setting guest TOD to 0x%llx", gtod.tod);
> +	kvm_s390_set_tod_clock(vcpu->kvm, &gtod);
>
>   	kvm_s390_set_psw_cc(vcpu, 0);
>   	return 0;
Set clock only concerns the left-most 64 bits of the TOD-Clock, right? 
(thinking out
loud) I wonder if we need to also concern ourselves about the epoch 
extension... but
perhaps current use cases of set clock do not warrant that?

-- 
- Collin L Walling

Re: [PATCH RFC 2/6] KVM: s390: provide only a single function for setting the tod

[Collin L. Walling]

On 02/07/2018 03:13 PM, Collin L. Walling wrote:
> On 02/07/2018 06:46 AM, David Hildenbrand wrote:
>> [...]
>>
>> @@ -3021,8 +3019,8 @@ static int kvm_s390_handle_requests(struct 
>> kvm_vcpu *vcpu)
>>       return 0;
>>   }
>>
>> -void kvm_s390_set_tod_clock_ext(struct kvm *kvm,
>> -                 const struct kvm_s390_vm_tod_clock *gtod)
>> +void kvm_s390_set_tod_clock(struct kvm *kvm,
>> +                const struct kvm_s390_vm_tod_clock *gtod)
> Nit: off by one space --------^
or perhaps my email client is lying to me? ignore my comment if it looks 
fine on your end.



-- 
- Collin L Walling

Re: [PATCH RFC 2/6] KVM: s390: provide only a single function for setting the tod

[David Hildenbrand]

>>
>> -void kvm_s390_set_tod_clock_ext(struct kvm *kvm,
>> -				 const struct kvm_s390_vm_tod_clock *gtod)
>> +void kvm_s390_set_tod_clock(struct kvm *kvm,
>> +			    const struct kvm_s390_vm_tod_clock *gtod)
> Nit: off by one space --------^

This looks fine in code, just a quirk in the visual representation of
the diff file.

[...]
>>   /* implemented in kvm-s390.c */
>> -void kvm_s390_set_tod_clock_ext(struct kvm *kvm,
>> -				 const struct kvm_s390_vm_tod_clock *gtod);
>> -void kvm_s390_set_tod_clock(struct kvm *kvm, u64 tod);
>> +void kvm_s390_set_tod_clock(struct kvm *kvm,
>> +			    const struct kvm_s390_vm_tod_clock *gtod);
> Same nit here.

Dito.

>>   long kvm_arch_fault_in_page(struct kvm_vcpu *vcpu, gpa_t gpa, int writable);
>>   int kvm_s390_store_status_unloaded(struct kvm_vcpu *vcpu, unsigned long addr);
>>   int kvm_s390_vcpu_store_status(struct kvm_vcpu *vcpu, unsigned long addr);
>> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
>> index c4c4e157c036..33505c32c48a 100644
>> --- a/arch/s390/kvm/priv.c
>> +++ b/arch/s390/kvm/priv.c
>> @@ -85,9 +85,10 @@ int kvm_s390_handle_e3(struct kvm_vcpu *vcpu)
>>   /* Handle SCK (SET CLOCK) interception */
>>   static int handle_set_clock(struct kvm_vcpu *vcpu)
>>   {
>> +	struct kvm_s390_vm_tod_clock gtod = { 0 };
>>   	int rc;
>>   	u8 ar;
>> -	u64 op2, val;
>> +	u64 op2;
>>
>>   	vcpu->stat.instruction_sck++;
>>
>> @@ -97,12 +98,12 @@ static int handle_set_clock(struct kvm_vcpu *vcpu)
>>   	op2 = kvm_s390_get_base_disp_s(vcpu, &ar);
>>   	if (op2 & 7)	/* Operand must be on a doubleword boundary */
>>   		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
>> -	rc = read_guest(vcpu, op2, ar, &val, sizeof(val));
>> +	rc = read_guest(vcpu, op2, ar, &gtod.tod, sizeof(gtod.tod));
>>   	if (rc)
>>   		return kvm_s390_inject_prog_cond(vcpu, rc);
>>
>> -	VCPU_EVENT(vcpu, 3, "SCK: setting guest TOD to 0x%llx", val);
>> -	kvm_s390_set_tod_clock(vcpu->kvm, val);
>> +	VCPU_EVENT(vcpu, 3, "SCK: setting guest TOD to 0x%llx", gtod.tod);
>> +	kvm_s390_set_tod_clock(vcpu->kvm, &gtod);
>>
>>   	kvm_s390_set_psw_cc(vcpu, 0);
>>   	return 0;
> Set clock only concerns the left-most 64 bits of the TOD-Clock, right? 
> (thinking out
> loud) I wonder if we need to also concern ourselves about the epoch 
> extension... but
> perhaps current use cases of set clock do not warrant that?

SET CLOCK will always set the right-most 64 bits of the TOD. The
left-most bits are assumed to be 0 (that's how I understand the
architecture). SET CLOCK can at one point no longer be reliably used.

But until these years come, SET CLOCK has to continue to work (even if
the guest has Multiple-epoch facility). And that can be guaranteed by
setting the epoch index accordingly.

Especially, if we have with mepoch: "Guest TOD = HOST TOD - 1", the
epoch index has to be set to 0xff and the epoch to 0xffff...fff. Right
now, the epoch index would not be set.

Should be easy to reproduced by doing a

STORE CLOCK %d followed by A SET_CLOCK %d in the guest. As the Host TOD
has been incremented, we have "Guest TOD = Host TOD - X", and not
setting the epoch index to 0xff results in a wrong Guest TOD calculation
on the next STORE CLOCK.

Thanks!


-- 

Thanks,

David / dhildenb

[PATCH RFC 3/6] KVM: s390: consider epoch index on hotplugged CPUs

[David Hildenbrand]

We must copy both, the epoch and the epoch_idx.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/s390/kvm/kvm-s390.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index b514ee427077..d007b737cd4d 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2387,6 +2387,7 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
 	mutex_lock(&vcpu->kvm->lock);
 	preempt_disable();
 	vcpu->arch.sie_block->epoch = vcpu->kvm->arch.epoch;
+	vcpu->arch.sie_block->epdx = vcpu->kvm->arch.epdx;
 	preempt_enable();
 	mutex_unlock(&vcpu->kvm->lock);
 	if (!kvm_is_ucontrol(vcpu->kvm)) {
-- 
2.14.3

Re: [PATCH RFC 3/6] KVM: s390: consider epoch index on hotplugged CPUs

[Cornelia Huck]

On Wed,  7 Feb 2018 12:46:44 +0100
David Hildenbrand <david@redhat.com> wrote:

> We must copy both, the epoch and the epoch_idx.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  arch/s390/kvm/kvm-s390.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index b514ee427077..d007b737cd4d 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2387,6 +2387,7 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
>  	mutex_lock(&vcpu->kvm->lock);
>  	preempt_disable();
>  	vcpu->arch.sie_block->epoch = vcpu->kvm->arch.epoch;
> +	vcpu->arch.sie_block->epdx = vcpu->kvm->arch.epdx;
>  	preempt_enable();
>  	mutex_unlock(&vcpu->kvm->lock);
>  	if (!kvm_is_ucontrol(vcpu->kvm)) {

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH RFC 3/6] KVM: s390: consider epoch index on hotplugged CPUs

[Christian Borntraeger]

On 02/07/2018 12:46 PM, David Hildenbrand wrote:
> We must copy both, the epoch and the epoch_idx.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Thanks applied. Deserves a cc stable I think.

> ---
>  arch/s390/kvm/kvm-s390.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index b514ee427077..d007b737cd4d 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2387,6 +2387,7 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
>  	mutex_lock(&vcpu->kvm->lock);
>  	preempt_disable();
>  	vcpu->arch.sie_block->epoch = vcpu->kvm->arch.epoch;
> +	vcpu->arch.sie_block->epdx = vcpu->kvm->arch.epdx;
>  	preempt_enable();
>  	mutex_unlock(&vcpu->kvm->lock);
>  	if (!kvm_is_ucontrol(vcpu->kvm)) {
> 

[PATCH RFC 4/6] KVM: s390: consider epoch index on TOD clock syncs

[David Hildenbrand]

For now, we don't take care of over/underflows. Especially underflows
are critical:

Assume the epoch is currently 0 and we get a sync request for delta=1,
meaning the TOD is moved forward by 1 and we have to fix it up by
subtracting 1 from the epoch. Right now, this will leave the epoch
index untouched, resulting in epoch=-1, epoch_idx=0, which is wrong.

We have to take care of over and underflows, also for the VSIE case. So
let's factor out calculation into a separate function.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/s390/kvm/kvm-s390.c | 32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index d007b737cd4d..c2b62379049e 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -179,6 +179,28 @@ int kvm_arch_hardware_enable(void)
 static void kvm_gmap_notifier(struct gmap *gmap, unsigned long start,
 			      unsigned long end);
 
+static void kvm_clock_sync_scb(struct kvm_s390_sie_block *scb, u64 delta)
+{
+	u64 delta_idx = 0;
+
+	/*
+	 * The TOD jumps by delta, we have to compensate this by adding
+	 * -delta to the epoch.
+	 */
+	delta = -delta;
+
+	/* sign-extension - we're adding to signed values below */
+	if ((s64)delta < 0)
+		delta_idx = 0xff;
+
+	scb->epoch += delta;
+	if (scb->ecd & ECD_MEF) {
+		scb->epdx += delta_idx;
+		if (scb->epoch < delta)
+			scb->epdx += 1;
+	}
+}
+
 /*
  * This callback is executed during stop_machine(). All CPUs are therefore
  * temporarily stopped. In order not to change guest behavior, we have to
@@ -194,13 +216,17 @@ static int kvm_clock_sync(struct notifier_block *notifier, unsigned long val,
 	unsigned long long *delta = v;
 
 	list_for_each_entry(kvm, &vm_list, vm_list) {
-		kvm->arch.epoch -= *delta;
 		kvm_for_each_vcpu(i, vcpu, kvm) {
-			vcpu->arch.sie_block->epoch -= *delta;
+			kvm_clock_sync_scb(vcpu->arch.sie_block, *delta);
+			if (i == 0) {
+				kvm->arch.epoch = vcpu->arch.sie_block->epoch;
+				kvm->arch.epdx = vcpu->arch.sie_block->epdx;
+			}
 			if (vcpu->arch.cputm_enabled)
 				vcpu->arch.cputm_start += *delta;
 			if (vcpu->arch.vsie_block)
-				vcpu->arch.vsie_block->epoch -= *delta;
+				kvm_clock_sync_scb(vcpu->arch.vsie_block,
+						   *delta);
 		}
 	}
 	return NOTIFY_OK;
-- 
2.14.3

Re: [PATCH RFC 4/6] KVM: s390: consider epoch index on TOD clock syncs

[Collin L. Walling]

On 02/07/2018 06:46 AM, David Hildenbrand wrote:
> For now, we don't take care of over/underflows. Especially underflows
> are critical:
>
> Assume the epoch is currently 0 and we get a sync request for delta=1,
> meaning the TOD is moved forward by 1 and we have to fix it up by
> subtracting 1 from the epoch. Right now, this will leave the epoch
> index untouched, resulting in epoch=-1, epoch_idx=0, which is wrong.
>
> We have to take care of over and underflows, also for the VSIE case. So
> let's factor out calculation into a separate function.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   arch/s390/kvm/kvm-s390.c | 32 +++++++++++++++++++++++++++++---
>   1 file changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index d007b737cd4d..c2b62379049e 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -179,6 +179,28 @@ int kvm_arch_hardware_enable(void)
>   static void kvm_gmap_notifier(struct gmap *gmap, unsigned long start,
>   			      unsigned long end);
>
> +static void kvm_clock_sync_scb(struct kvm_s390_sie_block *scb, u64 delta)
> +{
> +	u64 delta_idx = 0;
> +
> +	/*
> +	 * The TOD jumps by delta, we have to compensate this by adding
> +	 * -delta to the epoch.
> +	 */
> +	delta = -delta;
> +
> +	/* sign-extension - we're adding to signed values below */
> +	if ((s64)delta < 0)
> +		delta_idx = 0xff;
> +
> +	scb->epoch += delta;
> +	if (scb->ecd & ECD_MEF) {
> +		scb->epdx += delta_idx;
> +		if (scb->epoch < delta)
> +			scb->epdx += 1;
> +	}
> +}
> +

Is the sync always a jump forward? Do we need to worry about a borrow 
from the epdx in case of underflow?

>   /*
>    * This callback is executed during stop_machine(). All CPUs are therefore
>    * temporarily stopped. In order not to change guest behavior, we have to
> @@ -194,13 +216,17 @@ static int kvm_clock_sync(struct notifier_block *notifier, unsigned long val,
>   	unsigned long long *delta = v;
>
>   	list_for_each_entry(kvm, &vm_list, vm_list) {
> -		kvm->arch.epoch -= *delta;
>   		kvm_for_each_vcpu(i, vcpu, kvm) {
> -			vcpu->arch.sie_block->epoch -= *delta;
> +			kvm_clock_sync_scb(vcpu->arch.sie_block, *delta);
> +			if (i == 0) {
> +				kvm->arch.epoch = vcpu->arch.sie_block->epoch;
> +				kvm->arch.epdx = vcpu->arch.sie_block->epdx;

Are we safe by setting the kvm epochs to the sie epochs wrt migration?

> +			}
>   			if (vcpu->arch.cputm_enabled)
>   				vcpu->arch.cputm_start += *delta;
>   			if (vcpu->arch.vsie_block)
> -				vcpu->arch.vsie_block->epoch -= *delta;
> +				kvm_clock_sync_scb(vcpu->arch.vsie_block,
> +						   *delta);
>   		}
>   	}
>   	return NOTIFY_OK;

-- 
- Collin L Walling

Re: [PATCH RFC 4/6] KVM: s390: consider epoch index on TOD clock syncs

[David Hildenbrand]

On 07.02.2018 21:08, Collin L. Walling wrote:
> On 02/07/2018 06:46 AM, David Hildenbrand wrote:
>> For now, we don't take care of over/underflows. Especially underflows
>> are critical:
>>
>> Assume the epoch is currently 0 and we get a sync request for delta=1,
>> meaning the TOD is moved forward by 1 and we have to fix it up by
>> subtracting 1 from the epoch. Right now, this will leave the epoch
>> index untouched, resulting in epoch=-1, epoch_idx=0, which is wrong.
>>
>> We have to take care of over and underflows, also for the VSIE case. So
>> let's factor out calculation into a separate function.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>   arch/s390/kvm/kvm-s390.c | 32 +++++++++++++++++++++++++++++---
>>   1 file changed, 29 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index d007b737cd4d..c2b62379049e 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -179,6 +179,28 @@ int kvm_arch_hardware_enable(void)
>>   static void kvm_gmap_notifier(struct gmap *gmap, unsigned long start,
>>   			      unsigned long end);
>>
>> +static void kvm_clock_sync_scb(struct kvm_s390_sie_block *scb, u64 delta)
>> +{
>> +	u64 delta_idx = 0;
>> +
>> +	/*
>> +	 * The TOD jumps by delta, we have to compensate this by adding
>> +	 * -delta to the epoch.
>> +	 */
>> +	delta = -delta;
>> +
>> +	/* sign-extension - we're adding to signed values below */
>> +	if ((s64)delta < 0)
>> +		delta_idx = 0xff;
>> +
>> +	scb->epoch += delta;
>> +	if (scb->ecd & ECD_MEF) {
>> +		scb->epdx += delta_idx;
>> +		if (scb->epoch < delta)
>> +			scb->epdx += 1;
>> +	}
>> +}
>> +
> 
> Is the sync always a jump forward? Do we need to worry about a borrow 
> from the epdx in case of underflow?

I can jump forward and backwards, so delta can be positive or negative,
resulting in -delta being positive or negative.

The rules of unsigned addition should make sure that all cases are
covered. (I tried to find a counter example but wasn't able to find one)

(Especially, this is the same code pattern as found in
arch/s390/kvm/vsie.c:register_shadow_scb(), which also adds two signed
numbers.)

> 
>>   /*
>>    * This callback is executed during stop_machine(). All CPUs are therefore
>>    * temporarily stopped. In order not to change guest behavior, we have to
>> @@ -194,13 +216,17 @@ static int kvm_clock_sync(struct notifier_block *notifier, unsigned long val,
>>   	unsigned long long *delta = v;
>>
>>   	list_for_each_entry(kvm, &vm_list, vm_list) {
>> -		kvm->arch.epoch -= *delta;
>>   		kvm_for_each_vcpu(i, vcpu, kvm) {
>> -			vcpu->arch.sie_block->epoch -= *delta;
>> +			kvm_clock_sync_scb(vcpu->arch.sie_block, *delta);
>> +			if (i == 0) {
>> +				kvm->arch.epoch = vcpu->arch.sie_block->epoch;
>> +				kvm->arch.epdx = vcpu->arch.sie_block->epdx;
> 
> Are we safe by setting the kvm epochs to the sie epochs wrt migration?

Yes, in fact they should be the same for all VCPUs, otherwise we are in
trouble. The TOD has to be the same over all VCPUs.

So we should always have
- kvm->arch.epoch == vcpu->arch.sie_block->epoch
- kvm->arch.epdx == vcpu->arch.sie_block->epdx
for all VCPUs, otherwise their TOD could differ.

This is also guaranteed by the way we calculate and update these numbers.

The only special case is if somebody would be using
set_on_reg/get_one_reg with KVM_REG_S390_EPOCHDIFF, setting different
values - very unlikely and bad.

> 
>> +			}
>>   			if (vcpu->arch.cputm_enabled)
>>   				vcpu->arch.cputm_start += *delta;
>>   			if (vcpu->arch.vsie_block)
>> -				vcpu->arch.vsie_block->epoch -= *delta;
>> +				kvm_clock_sync_scb(vcpu->arch.vsie_block,
>> +						   *delta);
>>   		}
>>   	}
>>   	return NOTIFY_OK;
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH RFC 4/6] KVM: s390: consider epoch index on TOD clock syncs

[Collin L. Walling]

On 02/07/2018 04:35 PM, David Hildenbrand wrote:
> On 07.02.2018 21:08, Collin L. Walling wrote:
>> On 02/07/2018 06:46 AM, David Hildenbrand wrote:
>>> For now, we don't take care of over/underflows. Especially underflows
>>> are critical:
>>>
>>> Assume the epoch is currently 0 and we get a sync request for delta=1,
>>> meaning the TOD is moved forward by 1 and we have to fix it up by
>>> subtracting 1 from the epoch. Right now, this will leave the epoch
>>> index untouched, resulting in epoch=-1, epoch_idx=0, which is wrong.
>>>
>>> We have to take care of over and underflows, also for the VSIE case. So
>>> let's factor out calculation into a separate function.
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>    arch/s390/kvm/kvm-s390.c | 32 +++++++++++++++++++++++++++++---
>>>    1 file changed, 29 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>>> index d007b737cd4d..c2b62379049e 100644
>>> --- a/arch/s390/kvm/kvm-s390.c
>>> +++ b/arch/s390/kvm/kvm-s390.c
>>> @@ -179,6 +179,28 @@ int kvm_arch_hardware_enable(void)
>>>    static void kvm_gmap_notifier(struct gmap *gmap, unsigned long start,
>>>    			      unsigned long end);
>>>
>>> +static void kvm_clock_sync_scb(struct kvm_s390_sie_block *scb, u64 delta)
>>> +{
>>> +	u64 delta_idx = 0;
>>> +
>>> +	/*
>>> +	 * The TOD jumps by delta, we have to compensate this by adding
>>> +	 * -delta to the epoch.
>>> +	 */
>>> +	delta = -delta;
>>> +
>>> +	/* sign-extension - we're adding to signed values below */
>>> +	if ((s64)delta < 0)
>>> +		delta_idx = 0xff;
>>> +
>>> +	scb->epoch += delta;
>>> +	if (scb->ecd & ECD_MEF) {
>>> +		scb->epdx += delta_idx;
>>> +		if (scb->epoch < delta)
>>> +			scb->epdx += 1;
>>> +	}
>>> +}
>>> +
>> Is the sync always a jump forward? Do we need to worry about a borrow
>> from the epdx in case of underflow?
> I can jump forward and backwards, so delta can be positive or negative,
> resulting in -delta being positive or negative.
>
> The rules of unsigned addition should make sure that all cases are
> covered. (I tried to find a counter example but wasn't able to find one)

Agreed. I just wrote down a few edge cases myself... it seems to check 
out nicely.

>
> (Especially, this is the same code pattern as found in
> arch/s390/kvm/vsie.c:register_shadow_scb(), which also adds two signed
> numbers.)
>
>>>    /*
>>>     * This callback is executed during stop_machine(). All CPUs are therefore
>>>     * temporarily stopped. In order not to change guest behavior, we have to
>>> @@ -194,13 +216,17 @@ static int kvm_clock_sync(struct notifier_block *notifier, unsigned long val,
>>>    	unsigned long long *delta = v;
>>>
>>>    	list_for_each_entry(kvm, &vm_list, vm_list) {
>>> -		kvm->arch.epoch -= *delta;
>>>    		kvm_for_each_vcpu(i, vcpu, kvm) {
>>> -			vcpu->arch.sie_block->epoch -= *delta;
>>> +			kvm_clock_sync_scb(vcpu->arch.sie_block, *delta);
>>> +			if (i == 0) {
>>> +				kvm->arch.epoch = vcpu->arch.sie_block->epoch;
>>> +				kvm->arch.epdx = vcpu->arch.sie_block->epdx;
>> Are we safe by setting the kvm epochs to the sie epochs wrt migration?
> Yes, in fact they should be the same for all VCPUs, otherwise we are in
> trouble. The TOD has to be the same over all VCPUs.
>
> So we should always have
> - kvm->arch.epoch == vcpu->arch.sie_block->epoch
> - kvm->arch.epdx == vcpu->arch.sie_block->epdx
> for all VCPUs, otherwise their TOD could differ.

Perhaps then this could be shortened to calculate the epochs only once, 
then set
each vcpu to those values instead ofcalculating on each iteration?

I imagine the number of iterations would never be large enough to cause any
considerable performance hits, though.

>
> This is also guaranteed by the way we calculate and update these numbers.
>
> The only special case is if somebody would be using
> set_on_reg/get_one_reg with KVM_REG_S390_EPOCHDIFF, setting different
> values - very unlikely and bad.
>
>>> +			}
>>>    			if (vcpu->arch.cputm_enabled)
>>>    				vcpu->arch.cputm_start += *delta;
>>>    			if (vcpu->arch.vsie_block)
>>> -				vcpu->arch.vsie_block->epoch -= *delta;
>>> +				kvm_clock_sync_scb(vcpu->arch.vsie_block,
>>> +						   *delta);
>>>    		}
>>>    	}
>>>    	return NOTIFY_OK;
>


-- 
- Collin L Walling

Re: [PATCH RFC 4/6] KVM: s390: consider epoch index on TOD clock syncs

[David Hildenbrand]

>> The rules of unsigned addition should make sure that all cases are
>> covered. (I tried to find a counter example but wasn't able to find one)
> 
> Agreed. I just wrote down a few edge cases myself... it seems to check 
> out nicely.
> 
>>
>> (Especially, this is the same code pattern as found in
>> arch/s390/kvm/vsie.c:register_shadow_scb(), which also adds two signed
>> numbers.)
>>
>>>>    /*
>>>>     * This callback is executed during stop_machine(). All CPUs are therefore
>>>>     * temporarily stopped. In order not to change guest behavior, we have to
>>>> @@ -194,13 +216,17 @@ static int kvm_clock_sync(struct notifier_block *notifier, unsigned long val,
>>>>    	unsigned long long *delta = v;
>>>>
>>>>    	list_for_each_entry(kvm, &vm_list, vm_list) {
>>>> -		kvm->arch.epoch -= *delta;
>>>>    		kvm_for_each_vcpu(i, vcpu, kvm) {
>>>> -			vcpu->arch.sie_block->epoch -= *delta;
>>>> +			kvm_clock_sync_scb(vcpu->arch.sie_block, *delta);
>>>> +			if (i == 0) {
>>>> +				kvm->arch.epoch = vcpu->arch.sie_block->epoch;
>>>> +				kvm->arch.epdx = vcpu->arch.sie_block->epdx;
>>> Are we safe by setting the kvm epochs to the sie epochs wrt migration?
>> Yes, in fact they should be the same for all VCPUs, otherwise we are in
>> trouble. The TOD has to be the same over all VCPUs.
>>
>> So we should always have
>> - kvm->arch.epoch == vcpu->arch.sie_block->epoch
>> - kvm->arch.epdx == vcpu->arch.sie_block->epdx
>> for all VCPUs, otherwise their TOD could differ.
> 
> Perhaps then this could be shortened to calculate the epochs only once, 
> then set
> each vcpu to those values instead ofcalculating on each iteration?
> 

I had that before, but changed it to this. Especially because some weird user
space could set the epochs differently on different CPUs (e.g. for testing
purposes or IDK).

So something like this is not shorter and possibly performs less calculations


        list_for_each_entry(kvm, &vm_list, vm_list) {
                kvm_for_each_vcpu(i, vcpu, kvm) {
-                       kvm_clock_sync_scb(vcpu->arch.sie_block, *delta);
                        if (i == 0) {
+                               kvm_clock_sync_scb(vcpu->arch.sie_block, *delta);
                                kvm->arch.epoch = vcpu->arch.sie_block->epoch;
                                kvm->arch.epdx = vcpu->arch.sie_block->epdx;
+                       } else {
+                               vcpu->arch.sie_block->epoch = kvm->arch.epoch;
+                               vcpu->arch.sie_block->epdx = kvm->arch.epdx;
                        }
                        if (vcpu->arch.cputm_enabled)
                                vcpu->arch.cputm_start += *delta;

I'll let the Maintainers decide :)

> I imagine the number of iterations would never be large enough to cause any
> considerable performance hits, though.

Thanks!

-- 

Thanks,

David / dhildenb

[PATCH RFC 5/6] KVM: s390: no need to inititalize kvm->arch members to 0

[David Hildenbrand]

KVM is allocated with kzalloc(), so these members are already 0.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/s390/kvm/kvm-s390.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index c2b62379049e..0c0eed7d60a8 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -1972,8 +1972,6 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 	for (i = 0; i < S390_ARCH_FAC_LIST_SIZE_U64; i++) {
 		if (i < kvm_s390_fac_list_mask_size())
 			kvm->arch.model.fac_mask[i] &= kvm_s390_fac_list_mask[i];
-		else
-			kvm->arch.model.fac_mask[i] = 0UL;
 	}
 
 	/* Populate the facility list initially. */
@@ -1998,8 +1996,6 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 	kvm_s390_crypto_init(kvm);
 
 	mutex_init(&kvm->arch.float_int.ais_lock);
-	kvm->arch.float_int.simm = 0;
-	kvm->arch.float_int.nimm = 0;
 	spin_lock_init(&kvm->arch.float_int.lock);
 	for (i = 0; i < FIRQ_LIST_COUNT; i++)
 		INIT_LIST_HEAD(&kvm->arch.float_int.lists[i]);
@@ -2025,10 +2021,6 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 		kvm->arch.gmap->pfault_enabled = 0;
 	}
 
-	kvm->arch.css_support = 0;
-	kvm->arch.use_irqchip = 0;
-	kvm->arch.epoch = 0;
-
 	spin_lock_init(&kvm->arch.start_stop_lock);
 	kvm_s390_vsie_init(kvm);
 	kvm_s390_gisa_init(kvm);
-- 
2.14.3

Re: [PATCH RFC 5/6] KVM: s390: no need to inititalize kvm->arch members to 0

[Cornelia Huck]

On Wed,  7 Feb 2018 12:46:46 +0100
David Hildenbrand <david@redhat.com> wrote:

> KVM is allocated with kzalloc(), so these members are already 0.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  arch/s390/kvm/kvm-s390.c | 8 --------
>  1 file changed, 8 deletions(-)

use_esca is also initialized to 0, but we'd lose the comment.

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

[PATCH RFC 6/6] KVM: s390: generalize kvm_s390_get_tod_clock_ext()

[David Hildenbrand]

Move the Multiple-epoch facility handling into it and rename it to
kvm_s390_get_tod_clock().

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/s390/kvm/kvm-s390.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 0c0eed7d60a8..df452b8b4f26 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -990,8 +990,8 @@ static int kvm_s390_set_tod(struct kvm *kvm, struct kvm_device_attr *attr)
 	return ret;
 }
 
-static void kvm_s390_get_tod_clock_ext(struct kvm *kvm,
-					struct kvm_s390_vm_tod_clock *gtod)
+static void kvm_s390_get_tod_clock(struct kvm *kvm,
+				   struct kvm_s390_vm_tod_clock *gtod)
 {
 	struct kvm_s390_tod_clock_ext htod;
 
@@ -1000,10 +1000,12 @@ static void kvm_s390_get_tod_clock_ext(struct kvm *kvm,
 	get_tod_clock_ext((char *)&htod);
 
 	gtod->tod = htod.tod + kvm->arch.epoch;
-	gtod->epoch_idx = htod.epoch_idx + kvm->arch.epdx;
-
-	if (gtod->tod < htod.tod)
-		gtod->epoch_idx += 1;
+	gtod->epoch_idx = 0;
+	if (test_kvm_facility(kvm, 139)) {
+		gtod->epoch_idx = htod.epoch_idx + kvm->arch.epdx;
+		if (gtod->tod < htod.tod)
+			gtod->epoch_idx += 1;
+	}
 
 	preempt_enable();
 }
@@ -1012,13 +1014,7 @@ static int kvm_s390_get_tod_ext(struct kvm *kvm, struct kvm_device_attr *attr)
 {
 	struct kvm_s390_vm_tod_clock gtod;
 
-	memset(&gtod, 0, sizeof(gtod));
-
-	if (test_kvm_facility(kvm, 139))
-		kvm_s390_get_tod_clock_ext(kvm, &gtod);
-	else
-		gtod.tod = kvm_s390_get_tod_clock_fast(kvm);
-
+	kvm_s390_get_tod_clock(kvm, &gtod);
 	if (copy_to_user((void __user *)attr->addr, &gtod, sizeof(gtod)))
 		return -EFAULT;
 
-- 
2.14.3

Re: [PATCH RFC 6/6] KVM: s390: generalize kvm_s390_get_tod_clock_ext()

[Cornelia Huck]

On Wed,  7 Feb 2018 12:46:47 +0100
David Hildenbrand <david@redhat.com> wrote:

> Move the Multiple-epoch facility handling into it and rename it to
> kvm_s390_get_tod_clock().
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  arch/s390/kvm/kvm-s390.c | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)

Looks correct, but I'm not sure what this buys us?

> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 0c0eed7d60a8..df452b8b4f26 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -990,8 +990,8 @@ static int kvm_s390_set_tod(struct kvm *kvm, struct kvm_device_attr *attr)
>  	return ret;
>  }
>  
> -static void kvm_s390_get_tod_clock_ext(struct kvm *kvm,
> -					struct kvm_s390_vm_tod_clock *gtod)
> +static void kvm_s390_get_tod_clock(struct kvm *kvm,
> +				   struct kvm_s390_vm_tod_clock *gtod)
>  {
>  	struct kvm_s390_tod_clock_ext htod;
>  
> @@ -1000,10 +1000,12 @@ static void kvm_s390_get_tod_clock_ext(struct kvm *kvm,
>  	get_tod_clock_ext((char *)&htod);
>  
>  	gtod->tod = htod.tod + kvm->arch.epoch;
> -	gtod->epoch_idx = htod.epoch_idx + kvm->arch.epdx;
> -
> -	if (gtod->tod < htod.tod)
> -		gtod->epoch_idx += 1;
> +	gtod->epoch_idx = 0;
> +	if (test_kvm_facility(kvm, 139)) {
> +		gtod->epoch_idx = htod.epoch_idx + kvm->arch.epdx;
> +		if (gtod->tod < htod.tod)
> +			gtod->epoch_idx += 1;
> +	}
>  
>  	preempt_enable();
>  }
> @@ -1012,13 +1014,7 @@ static int kvm_s390_get_tod_ext(struct kvm *kvm, struct kvm_device_attr *attr)
>  {
>  	struct kvm_s390_vm_tod_clock gtod;
>  
> -	memset(&gtod, 0, sizeof(gtod));
> -
> -	if (test_kvm_facility(kvm, 139))
> -		kvm_s390_get_tod_clock_ext(kvm, &gtod);
> -	else
> -		gtod.tod = kvm_s390_get_tod_clock_fast(kvm);
> -
> +	kvm_s390_get_tod_clock(kvm, &gtod);
>  	if (copy_to_user((void __user *)attr->addr, &gtod, sizeof(gtod)))
>  		return -EFAULT;
>  

Re: [PATCH RFC 6/6] KVM: s390: generalize kvm_s390_get_tod_clock_ext()

[David Hildenbrand]

On 15.02.2018 15:08, Cornelia Huck wrote:
> On Wed,  7 Feb 2018 12:46:47 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Move the Multiple-epoch facility handling into it and rename it to
>> kvm_s390_get_tod_clock().
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  arch/s390/kvm/kvm-s390.c | 22 +++++++++-------------
>>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> Looks correct, but I'm not sure what this buys us?

That we have functions that can be called without having to care about
multiple epoch facility

Namely

kvm_s390_set_tod_clock()
kvm_s390_get_tod_clock()
kvm_s390_get_tod_clock_fast()

-- 

Thanks,

David / dhildenb

Re: [PATCH RFC 6/6] KVM: s390: generalize kvm_s390_get_tod_clock_ext()

[Cornelia Huck]

On Thu, 15 Feb 2018 15:14:37 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 15.02.2018 15:08, Cornelia Huck wrote:
> > On Wed,  7 Feb 2018 12:46:47 +0100
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> Move the Multiple-epoch facility handling into it and rename it to
> >> kvm_s390_get_tod_clock().
> >>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>  arch/s390/kvm/kvm-s390.c | 22 +++++++++-------------
> >>  1 file changed, 9 insertions(+), 13 deletions(-)  
> > 
> > Looks correct, but I'm not sure what this buys us?  
> 
> That we have functions that can be called without having to care about
> multiple epoch facility
> 
> Namely
> 
> kvm_s390_set_tod_clock()
> kvm_s390_get_tod_clock()
> kvm_s390_get_tod_clock_fast()
> 

OK, that makes sense. Maybe add something like that to the patch
description?

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH RFC 6/6] KVM: s390: generalize kvm_s390_get_tod_clock_ext()

[David Hildenbrand]

On 15.02.2018 15:17, Cornelia Huck wrote:
> On Thu, 15 Feb 2018 15:14:37 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 15.02.2018 15:08, Cornelia Huck wrote:
>>> On Wed,  7 Feb 2018 12:46:47 +0100
>>> David Hildenbrand <david@redhat.com> wrote:
>>>   
>>>> Move the Multiple-epoch facility handling into it and rename it to
>>>> kvm_s390_get_tod_clock().
>>>>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>>  arch/s390/kvm/kvm-s390.c | 22 +++++++++-------------
>>>>  1 file changed, 9 insertions(+), 13 deletions(-)  
>>>
>>> Looks correct, but I'm not sure what this buys us?  
>>
>> That we have functions that can be called without having to care about
>> multiple epoch facility
>>
>> Namely
>>
>> kvm_s390_set_tod_clock()
>> kvm_s390_get_tod_clock()
>> kvm_s390_get_tod_clock_fast()
>>
> 
> OK, that makes sense. Maybe add something like that to the patch
> description?
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 

Sure, can do! Thanks!

-- 

Thanks,

David / dhildenb

Re: [PATCH RFC 0/6] KVM: s390: Multiple-epoch facility fixes

[David Hildenbrand]

On 07.02.2018 12:46, David Hildenbrand wrote:
> It think I found various BUGs in the recently added Multiple-epoch facility
> support in KVM. Along with it, two cleanups.
> 
> 1. The clock-comparator sign control is not considered. I sent a patch
>    with this previously. This now contains fixes and simplifications.
> 2. SET CLOCK from the guest does not work reliably if the facility is
>    enabled (epoch index not set).
> 3. Hotplugged CPUs don't inherit the epoch index.
> 4. TOD clock syncs don't take care of overflows/underflows in the epoch
>    value and miss to update the epoch index.
> 
> 
> This is RFC as I have basically no machine to test. Hopefully somebody
> can jump in and verify that we now handle the epoch index in all
> scenarios correctly.
> 
> Most importantly, with Multiple-epoch facility, the condition
> 	Guest TOD = Host TOD - 1
> Is represented by both, epoch and epoch_idx containing 0xff. So it is
> treated as a 64+8bit signed number - we have to properly take care
> of over/underflows when modifying the epoch.
> 
> 
> David Hildenbrand (6):
>   KVM: s390: take care of clock-comparator sign control
>   KVM: s390: provide only a single function for setting the tod
>   KVM: s390: consider epoch index on hotplugged CPUs
>   KVM: s390: consider epoch index on TOD clock syncs
>   KVM: s390: no need to inititalize kvm->arch members to 0
>   KVM: s390: generalize kvm_s390_get_tod_clock_ext()
> 
>  arch/s390/kvm/interrupt.c |  25 ++++++++---
>  arch/s390/kvm/kvm-s390.c  | 109 +++++++++++++++++++++++-----------------------
>  arch/s390/kvm/kvm-s390.h  |   5 +--
>  arch/s390/kvm/priv.c      |   9 ++--
>  4 files changed, 80 insertions(+), 68 deletions(-)
> 

CC'ing Collin (sorry missed you)

-- 

Thanks,

David / dhildenb

Re: [PATCH RFC 2/6] KVM: s390: provide only a single function for setting the tod

[Christian Borntraeger]

On 02/07/2018 12:46 PM, David Hildenbrand wrote:
> Right now, SET CLOCK called in the guest does not properly take care of
> the epoch index, as the call goes via the old kvm_s390_set_tod_clock()
> interface. So the epoch index is neither reset to 0, if required, nor
> properly set to e.g. 0xff on negative values.
> 
> Fix this by providing a single kvm_s390_set_tod_clock() function. Move
> Multiple-epoch facility handling into it.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  arch/s390/kvm/kvm-s390.c | 46 +++++++++++++++-------------------------------
>  arch/s390/kvm/kvm-s390.h |  5 ++---
>  arch/s390/kvm/priv.c     |  9 +++++----
>  3 files changed, 22 insertions(+), 38 deletions(-)

The diffstat and the simplification makes this patch worthwile alone and I think you are
right. We need to fix sck as well.

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

applied thanks.

> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index ba4c7092335a..b514ee427077 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -902,12 +902,9 @@ static int kvm_s390_set_tod_ext(struct kvm *kvm, struct kvm_device_attr *attr)
>  	if (copy_from_user(&gtod, (void __user *)attr->addr, sizeof(gtod)))
>  		return -EFAULT;
> 
> -	if (test_kvm_facility(kvm, 139))
> -		kvm_s390_set_tod_clock_ext(kvm, &gtod);
> -	else if (gtod.epoch_idx == 0)
> -		kvm_s390_set_tod_clock(kvm, gtod.tod);
> -	else
> +	if (!test_kvm_facility(kvm, 139) && gtod.epoch_idx)
>  		return -EINVAL;
> +	kvm_s390_set_tod_clock(kvm, &gtod);
> 
>  	VM_EVENT(kvm, 3, "SET: TOD extension: 0x%x, TOD base: 0x%llx",
>  		gtod.epoch_idx, gtod.tod);
> @@ -932,13 +929,14 @@ static int kvm_s390_set_tod_high(struct kvm *kvm, struct kvm_device_attr *attr)
> 
>  static int kvm_s390_set_tod_low(struct kvm *kvm, struct kvm_device_attr *attr)
>  {
> -	u64 gtod;
> +	struct kvm_s390_vm_tod_clock gtod = { 0 };
> 
> -	if (copy_from_user(&gtod, (void __user *)attr->addr, sizeof(gtod)))
> +	if (copy_from_user(&gtod.tod, (void __user *)attr->addr,
> +			   sizeof(gtod.tod)))
>  		return -EFAULT;
> 
> -	kvm_s390_set_tod_clock(kvm, gtod);
> -	VM_EVENT(kvm, 3, "SET: TOD base: 0x%llx", gtod);
> +	kvm_s390_set_tod_clock(kvm, &gtod);
> +	VM_EVENT(kvm, 3, "SET: TOD base: 0x%llx", gtod.tod);
>  	return 0;
>  }
> 
> @@ -3021,8 +3019,8 @@ static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu)
>  	return 0;
>  }
> 
> -void kvm_s390_set_tod_clock_ext(struct kvm *kvm,
> -				 const struct kvm_s390_vm_tod_clock *gtod)
> +void kvm_s390_set_tod_clock(struct kvm *kvm,
> +			    const struct kvm_s390_vm_tod_clock *gtod)
>  {
>  	struct kvm_vcpu *vcpu;
>  	struct kvm_s390_tod_clock_ext htod;
> @@ -3034,10 +3032,12 @@ void kvm_s390_set_tod_clock_ext(struct kvm *kvm,
>  	get_tod_clock_ext((char *)&htod);
> 
>  	kvm->arch.epoch = gtod->tod - htod.tod;
> -	kvm->arch.epdx = gtod->epoch_idx - htod.epoch_idx;
> -
> -	if (kvm->arch.epoch > gtod->tod)
> -		kvm->arch.epdx -= 1;
> +	kvm->arch.epdx = 0;
> +	if (test_kvm_facility(kvm, 139)) {
> +		kvm->arch.epdx = gtod->epoch_idx - htod.epoch_idx;
> +		if (kvm->arch.epoch > gtod->tod)
> +			kvm->arch.epdx -= 1;
> +	}
> 
>  	kvm_s390_vcpu_block_all(kvm);
>  	kvm_for_each_vcpu(i, vcpu, kvm) {
> @@ -3050,22 +3050,6 @@ void kvm_s390_set_tod_clock_ext(struct kvm *kvm,
>  	mutex_unlock(&kvm->lock);
>  }
> 
> -void kvm_s390_set_tod_clock(struct kvm *kvm, u64 tod)
> -{
> -	struct kvm_vcpu *vcpu;
> -	int i;
> -
> -	mutex_lock(&kvm->lock);
> -	preempt_disable();
> -	kvm->arch.epoch = tod - get_tod_clock();
> -	kvm_s390_vcpu_block_all(kvm);
> -	kvm_for_each_vcpu(i, vcpu, kvm)
> -		vcpu->arch.sie_block->epoch = kvm->arch.epoch;
> -	kvm_s390_vcpu_unblock_all(kvm);
> -	preempt_enable();
> -	mutex_unlock(&kvm->lock);
> -}
> -
>  /**
>   * kvm_arch_fault_in_page - fault-in guest page if necessary
>   * @vcpu: The corresponding virtual cpu
> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
> index bd31b37b0e6f..19d719262765 100644
> --- a/arch/s390/kvm/kvm-s390.h
> +++ b/arch/s390/kvm/kvm-s390.h
> @@ -283,9 +283,8 @@ int kvm_s390_handle_sigp(struct kvm_vcpu *vcpu);
>  int kvm_s390_handle_sigp_pei(struct kvm_vcpu *vcpu);
> 
>  /* implemented in kvm-s390.c */
> -void kvm_s390_set_tod_clock_ext(struct kvm *kvm,
> -				 const struct kvm_s390_vm_tod_clock *gtod);
> -void kvm_s390_set_tod_clock(struct kvm *kvm, u64 tod);
> +void kvm_s390_set_tod_clock(struct kvm *kvm,
> +			    const struct kvm_s390_vm_tod_clock *gtod);
>  long kvm_arch_fault_in_page(struct kvm_vcpu *vcpu, gpa_t gpa, int writable);
>  int kvm_s390_store_status_unloaded(struct kvm_vcpu *vcpu, unsigned long addr);
>  int kvm_s390_vcpu_store_status(struct kvm_vcpu *vcpu, unsigned long addr);
> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
> index c4c4e157c036..33505c32c48a 100644
> --- a/arch/s390/kvm/priv.c
> +++ b/arch/s390/kvm/priv.c
> @@ -85,9 +85,10 @@ int kvm_s390_handle_e3(struct kvm_vcpu *vcpu)
>  /* Handle SCK (SET CLOCK) interception */
>  static int handle_set_clock(struct kvm_vcpu *vcpu)
>  {
> +	struct kvm_s390_vm_tod_clock gtod = { 0 };
>  	int rc;
>  	u8 ar;
> -	u64 op2, val;
> +	u64 op2;
> 
>  	vcpu->stat.instruction_sck++;
> 
> @@ -97,12 +98,12 @@ static int handle_set_clock(struct kvm_vcpu *vcpu)
>  	op2 = kvm_s390_get_base_disp_s(vcpu, &ar);
>  	if (op2 & 7)	/* Operand must be on a doubleword boundary */
>  		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
> -	rc = read_guest(vcpu, op2, ar, &val, sizeof(val));
> +	rc = read_guest(vcpu, op2, ar, &gtod.tod, sizeof(gtod.tod));
>  	if (rc)
>  		return kvm_s390_inject_prog_cond(vcpu, rc);
> 
> -	VCPU_EVENT(vcpu, 3, "SCK: setting guest TOD to 0x%llx", val);
> -	kvm_s390_set_tod_clock(vcpu->kvm, val);
> +	VCPU_EVENT(vcpu, 3, "SCK: setting guest TOD to 0x%llx", gtod.tod);
> +	kvm_s390_set_tod_clock(vcpu->kvm, &gtod);
> 
>  	kvm_s390_set_psw_cc(vcpu, 0);
>  	return 0;
>