[RFC,2/6] KVM: s390: provide only a single function for setting the tod
diff mbox

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

Commit Message

David Hildenbrand Feb. 7, 2018, 11:46 a.m. UTC
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(-)

Comments

Collin L. Walling Feb. 7, 2018, 8:13 p.m. UTC | #1
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 Feb. 7, 2018, 8:15 p.m. UTC | #2
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.
David Hildenbrand Feb. 7, 2018, 9:42 p.m. UTC | #3
>>
>> -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!
Christian Borntraeger Feb. 20, 2018, 3:15 p.m. UTC | #4
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.
> 

POP says 


The SET CLOCK instruction provides no means
by which an epoch index can be set. When the
multiple-epoch facility is installed, the use of SET
CLOCK may result in inconsistent values stored
by STORE CLOCK EXTENDED if the epoch
index was previously set to a nonzero value. In
this case, the PTFF control function PTFF-STOE
(set TOD offset extended) should be used rather
than SET CLOCK.

At some future date, the SET CLOCK instruction
may be removed from the architecture.


so I think we are compliant, no?


> 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;
>
David Hildenbrand Feb. 20, 2018, 3:53 p.m. UTC | #5
On 20.02.2018 16:15, Christian Borntraeger wrote:
> 
> 
> 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.
>>
> 
> POP says 
> 
> 
> The SET CLOCK instruction provides no means
> by which an epoch index can be set. When the
> multiple-epoch facility is installed, the use of SET
> CLOCK may result in inconsistent values stored
> by STORE CLOCK EXTENDED if the epoch
> index was previously set to a nonzero value. In
> this case, the PTFF control function PTFF-STOE
> (set TOD offset extended) should be used rather
> than SET CLOCK.
> 
> At some future date, the SET CLOCK instruction
> may be removed from the architecture.
> 
> 
> so I think we are compliant, no?
> 

/* KVM: epoch_idx = 0; epoch = 0 */
get_tod_clock_ext(&idx, &tod);
/* assume idx == 0, some time passes */
set_tod_clock(tod);
/* e.g. epoch_idx = 0, epoch = -1 */
get_tod_clock_ext(&idx, &tod);
/* as we missed to set epoch_idx to -1, we can now get idx=1 */

Or am I wrong? The guest never modified the epoch index ("if the epoch
index was previously set to a nonzero value").
Christian Borntraeger Feb. 20, 2018, 7:06 p.m. UTC | #6
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;
>

Patch
diff mbox

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;