| Message ID | 20180207114647.6220-7-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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(>od, 0, sizeof(gtod)); > - > - if (test_kvm_facility(kvm, 139)) > - kvm_s390_get_tod_clock_ext(kvm, >od); > - else > - gtod.tod = kvm_s390_get_tod_clock_fast(kvm); > - > + kvm_s390_get_tod_clock(kvm, >od); > if (copy_to_user((void __user *)attr->addr, >od, sizeof(gtod))) > return -EFAULT; >
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()
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>
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!
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(>od, 0, sizeof(gtod)); - - if (test_kvm_facility(kvm, 139)) - kvm_s390_get_tod_clock_ext(kvm, >od); - else - gtod.tod = kvm_s390_get_tod_clock_fast(kvm); - + kvm_s390_get_tod_clock(kvm, >od); if (copy_to_user((void __user *)attr->addr, >od, sizeof(gtod))) return -EFAULT;
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(-)