[RFC,6/6] KVM: s390: generalize kvm_s390_get_tod_clock_ext()
diff mbox

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

Commit Message

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

Comments

Cornelia Huck Feb. 15, 2018, 2:08 p.m. UTC | #1
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;
>
David Hildenbrand Feb. 15, 2018, 2:14 p.m. UTC | #2
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()
Cornelia Huck Feb. 15, 2018, 2:17 p.m. UTC | #3
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>
David Hildenbrand Feb. 15, 2018, 2:25 p.m. UTC | #4
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!

Patch
diff mbox

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;