[v1,3/3] KVM: s390: generalize kvm_s390_get_tod_clock_ext()
diff mbox

Message ID 20180427123613.20624-4-david@redhat.com
State New
Headers show

Commit Message

David Hildenbrand April 27, 2018, 12:36 p.m. UTC
Move the Multiple-epoch facility handling into it and rename it to
kvm_s390_get_tod_clock().

This leaves us with:
- kvm_s390_set_tod_clock()
- kvm_s390_get_tod_clock()
- kvm_s390_get_tod_clock_fast()

So all Multiple-epoch facility is hidden in these functions.

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

Comments

Collin Walling April 27, 2018, 3:58 p.m. UTC | #1
On 04/27/2018 08:36 AM, David Hildenbrand wrote:
> Move the Multiple-epoch facility handling into it and rename it to
> kvm_s390_get_tod_clock().
> 
> This leaves us with:
> - kvm_s390_set_tod_clock()
> - kvm_s390_get_tod_clock()
> - kvm_s390_get_tod_clock_fast()
> 
> So all Multiple-epoch facility is hidden in these functions.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Aside from the one nit described below, looks good to me!

Reviewed-by: Collin Walling <walling@linux.ibm.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 b3eee3cc6e78..fb753b9439fa 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -1033,8 +1033,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)

Nit: add one more whitespace for the second line of parameters

>  {
>  	struct kvm_s390_tod_clock_ext htod;
>  
> @@ -1043,10 +1043,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();
>  }
> @@ -1055,13 +1057,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;
>  
>
Cornelia Huck April 30, 2018, 1:12 p.m. UTC | #2
On Fri, 27 Apr 2018 14:36:13 +0200
David Hildenbrand <david@redhat.com> wrote:

> Move the Multiple-epoch facility handling into it and rename it to
> kvm_s390_get_tod_clock().
> 
> This leaves us with:
> - kvm_s390_set_tod_clock()
> - kvm_s390_get_tod_clock()
> - kvm_s390_get_tod_clock_fast()
> 
> So all Multiple-epoch facility is hidden in these functions.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  arch/s390/kvm/kvm-s390.c | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>
David Hildenbrand April 30, 2018, 1:24 p.m. UTC | #3
On 27.04.2018 17:58, Collin Walling wrote:
> On 04/27/2018 08:36 AM, David Hildenbrand wrote:
>> Move the Multiple-epoch facility handling into it and rename it to
>> kvm_s390_get_tod_clock().
>>
>> This leaves us with:
>> - kvm_s390_set_tod_clock()
>> - kvm_s390_get_tod_clock()
>> - kvm_s390_get_tod_clock_fast()
>>
>> So all Multiple-epoch facility is hidden in these functions.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
> 
> Aside from the one nit described below, looks good to me!
> 
> Reviewed-by: Collin Walling <walling@linux.ibm.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 b3eee3cc6e78..fb753b9439fa 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -1033,8 +1033,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)
> 
> Nit: add one more whitespace for the second line of parameters

It only looks like it's wrong in the patch file. The end result is correct.

Thanks!
Christian Borntraeger May 2, 2018, 7:36 p.m. UTC | #4
On 04/27/2018 02:36 PM, David Hildenbrand wrote:
> Move the Multiple-epoch facility handling into it and rename it to
[..]


> @@ -1055,13 +1057,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));

We should keep the memset, otherwise the padding might leak kernel info as we copy .
(found by smatch)


    arch/s390/kvm/kvm-s390.c:1068 kvm_s390_get_tod_ext() warn: check that 'gtod' doesn't leak information (struct has a hole after 'epoch_idx')


> -
> -	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 May 2, 2018, 7:40 p.m. UTC | #5
On 02.05.2018 21:36, Christian Borntraeger wrote:
> 
> 
> On 04/27/2018 02:36 PM, David Hildenbrand wrote:
>> Move the Multiple-epoch facility handling into it and rename it to
> [..]
> 
> 
>> @@ -1055,13 +1057,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));
> 
> We should keep the memset, otherwise the padding might leak kernel info as we copy .
> (found by smatch)

Thanks for the info and running the checker.

@Janosch please tell me if I should resend!

> 
> 
>     arch/s390/kvm/kvm-s390.c:1068 kvm_s390_get_tod_ext() warn: check that 'gtod' doesn't leak information (struct has a hole after 'epoch_idx')
> 
> 
>> -
>> -	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;
>>  
>>
>
Janosch Frank May 3, 2018, 6:41 a.m. UTC | #6
On 02.05.2018 21:40, David Hildenbrand wrote:
> On 02.05.2018 21:36, Christian Borntraeger wrote:
>>
>>
>> On 04/27/2018 02:36 PM, David Hildenbrand wrote:
>>> Move the Multiple-epoch facility handling into it and rename it to
>> [..]
>>
>>
>>> @@ -1055,13 +1057,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));
>>
>> We should keep the memset, otherwise the padding might leak kernel info as we copy .
>> (found by smatch)
> 
> Thanks for the info and running the checker.
> 
> @Janosch please tell me if I should resend!

I'll fix that, no need to resend.

> 
>>
>>
>>     arch/s390/kvm/kvm-s390.c:1068 kvm_s390_get_tod_ext() warn: check that 'gtod' doesn't leak information (struct has a hole after 'epoch_idx')
>>
>>
>>> -
>>> -	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;
>>>  
>>>
>>
> 
>

Patch
diff mbox

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index b3eee3cc6e78..fb753b9439fa 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -1033,8 +1033,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;
 
@@ -1043,10 +1043,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();
 }
@@ -1055,13 +1057,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;