Re: [Qemu-devel] [PATCH v2 1/2] s390/kvm: Support for get/set of extended TOD-Clock for guest

[Christian Borntraeger]

On 10/04/2017 01:42 PM, Thomas Huth wrote:
> On 04.10.2017 12:57, Christian Borntraeger wrote:
>> From: "Collin L. Walling" <address@hidden>
>>
>> Provides an interface for getting and setting the guest's extended
>> TOD-Clock via a single ioctl to kvm. If the ioctl fails because it
>> is not support by kvm, then we fall back to the old style of
>> retrieving the clock via two ioctls.
>>
>> Signed-off-by: Collin L. Walling <address@hidden>
>> Reviewed-by: Eric Farman <address@hidden>
>> Reviewed-by: Claudio Imbrenda <address@hidden>
>> Signed-off-by: Christian Borntraeger <address@hidden>
>> [split failure change from epoch index change]
>> ---
>>  target/s390x/cpu.c       | 26 +++++++++++++++++++-------
>>  target/s390x/kvm-stub.c  | 10 ++++++++++
>>  target/s390x/kvm.c       | 35 ++++++++++++++++++++++++++++++++++-
>>  target/s390x/kvm_s390x.h |  2 ++
>>  4 files changed, 65 insertions(+), 8 deletions(-)
>>
>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
>> index 34538c3..c8f1219 100644
>> --- a/target/s390x/cpu.c
>> +++ b/target/s390x/cpu.c
>> @@ -357,22 +357,34 @@ unsigned int s390_cpu_set_state(uint8_t cpu_state, 
>> S390CPU *cpu)
>>  
>>  int s390_get_clock(uint8_t *tod_high, uint64_t *tod_low)
>>  {
>> +    int r = 0;
>> +
>>      if (kvm_enabled()) {
>> -        return kvm_s390_get_clock(tod_high, tod_low);
>> +        r = kvm_s390_get_clock_ext(tod_high, tod_low);
>> +        if (r == -ENXIO) {
>> +            return kvm_s390_get_clock(tod_high, tod_low);
>> +        }
>> +    } else {
>> +        /* Fixme TCG */
>> +        *tod_high = 0;
>> +        *tod_low = 0;
>>      }
>> -    /* Fixme TCG */
>> -    *tod_high = 0;
>> -    *tod_low = 0;
>> -    return 0;
>> +
>> +    return r;
>>  }
>>  
>>  int s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)
>>  {
>> +    int r = 0;
>> +
>>      if (kvm_enabled()) {
>> -        return kvm_s390_set_clock(tod_high, tod_low);
>> +        r = kvm_s390_set_clock_ext(tod_high, tod_low);
>> +        if (r == -ENXIO) {
>> +            return kvm_s390_set_clock(tod_high, tod_low);
>> +        }
>>      }
>>      /* Fixme TCG */
>> -    return 0;
>> +    return r;
>>  }
>>  
>>  int s390_set_memory_limit(uint64_t new_limit, uint64_t *hw_limit)
>> diff --git a/target/s390x/kvm-stub.c b/target/s390x/kvm-stub.c
>> index 261e1cd..43f02c2 100644
>> --- a/target/s390x/kvm-stub.c
>> +++ b/target/s390x/kvm-stub.c
>> @@ -68,11 +68,21 @@ int kvm_s390_get_clock(uint8_t *tod_high, uint64_t 
>> *tod_low)
>>      return -ENOSYS;
>>  }
>>  
>> +int kvm_s390_get_clock_ext(uint8_t *tod_high, uint64_t *tod_low)
>> +{
>> +    return -ENOSYS;
>> +}
>> +
>>  int kvm_s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)
>>  {
>>      return -ENOSYS;
>>  }
>>  
>> +int kvm_s390_set_clock_ext(uint8_t *tod_high, uint64_t *tod_low)
>> +{
>> +    return -ENOSYS;
>> +}
>> +
>>  void kvm_s390_enable_css_support(S390CPU *cpu)
>>  {
>>  }
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index ebb75ca..4c944a5 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -643,10 +643,27 @@ int kvm_s390_get_clock(uint8_t *tod_high, uint64_t 
>> *tod_low)
>>      return kvm_vm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr);
>>  }
>>  
>> +int kvm_s390_get_clock_ext(uint8_t *tod_high, uint64_t *tod_low)
>> +{
>> +    int r;
>> +    struct kvm_s390_vm_tod_clock gtod;
>> +
> 
> So you've got a blank line here...

Yes, seems that I have forgotten this one. 
I will let Conny decide if I should resend or if she can fixup.


> 
>> +    struct kvm_device_attr attr = {
>> +        .group = KVM_S390_VM_TOD,
>> +        .attr = KVM_S390_VM_TOD_EXT,
>> +        .addr = (uint64_t)(&gtod),
>> +    };
>> +
>> +    r = kvm_vm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr);
>> +    *tod_high = gtod.epoch_idx;
>> +    *tod_low  = gtod.tod;
>> +
>> +    return r;
>> +}
>> +
>>  int kvm_s390_set_clock(uint8_t *tod_high, uint64_t *tod_low)
>>  {
>>      int r;
>> -
> 
> ... but removed the blank line here ... it would be more consequent to
> do it the same way everywhere?
> 
>>      struct kvm_device_attr attr = {
>>          .group = KVM_S390_VM_TOD,
>>          .attr = KVM_S390_VM_TOD_LOW,
>> @@ -663,6 +680,22 @@ int kvm_s390_set_clock(uint8_t *tod_high, uint64_t 
>> *tod_low)
>>      return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
>>  }
>>  
>> +int kvm_s390_set_clock_ext(uint8_t *tod_high, uint64_t *tod_low)
>> +{
>> +    struct kvm_s390_vm_tod_clock gtod = {
>> +        .epoch_idx = *tod_high,
>> +        .tod  = *tod_low,
>> +    };
>> +
>> +    struct kvm_device_attr attr = {
>> +        .group = KVM_S390_VM_TOD,
>> +        .attr = KVM_S390_VM_TOD_EXT,
>> +        .addr = (uint64_t)(&gtod),
> 
> You could remove the parentheses around "&gtod" (same in the
> get_clock_ext function).

dito.
> 
>> +    };
>> +
>> +    return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
>> +}
>> +
>>  /**
>>   * kvm_s390_mem_op:
>>   * @addr:      the logical start address in guest memory
>> diff --git a/target/s390x/kvm_s390x.h b/target/s390x/kvm_s390x.h
>> index 2d594bd..501fc5a 100644
>> --- a/target/s390x/kvm_s390x.h
>> +++ b/target/s390x/kvm_s390x.h
>> @@ -29,7 +29,9 @@ int kvm_s390_vcpu_interrupt_post_load(S390CPU *cpu);
>>  int kvm_s390_get_ri(void);
>>  int kvm_s390_get_gs(void);
>>  int kvm_s390_get_clock(uint8_t *tod_high, uint64_t *tod_clock);
>> +int kvm_s390_get_clock_ext(uint8_t *tod_high, uint64_t *tod_clock);
>>  int kvm_s390_set_clock(uint8_t *tod_high, uint64_t *tod_clock);
>> +int kvm_s390_set_clock_ext(uint8_t *tod_high, uint64_t *tod_clock);
>>  void kvm_s390_enable_css_support(S390CPU *cpu);
>>  int kvm_s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch,
>>                                      int vq, bool assign);
>>
> 
> Just cosmetic nits... patch looks fine otherwise, so:
> 
> Reviewed-by: Thomas Huth <address@hidden>
>