| Message ID | 20180207114647.6220-3-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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(>od, (void __user *)attr->addr, sizeof(gtod))) > return -EFAULT; > > - if (test_kvm_facility(kvm, 139)) > - kvm_s390_set_tod_clock_ext(kvm, >od); > - 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, >od); > > 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(>od, (void __user *)attr->addr, sizeof(gtod))) > + if (copy_from_user(>od.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, >od); > + 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, >od.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, >od); > > 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?
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.
>> >> -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, >od.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, >od); >> >> 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!
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(>od, (void __user *)attr->addr, sizeof(gtod))) > return -EFAULT; > > - if (test_kvm_facility(kvm, 139)) > - kvm_s390_set_tod_clock_ext(kvm, >od); > - 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, >od); > > 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(>od, (void __user *)attr->addr, sizeof(gtod))) > + if (copy_from_user(>od.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, >od); > + 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, >od.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, >od); > > kvm_s390_set_psw_cc(vcpu, 0); > return 0; >
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").
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(>od, (void __user *)attr->addr, sizeof(gtod))) > return -EFAULT; > > - if (test_kvm_facility(kvm, 139)) > - kvm_s390_set_tod_clock_ext(kvm, >od); > - 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, >od); > > 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(>od, (void __user *)attr->addr, sizeof(gtod))) > + if (copy_from_user(>od.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, >od); > + 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, >od.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, >od); > > kvm_s390_set_psw_cc(vcpu, 0); > return 0; >
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(>od, (void __user *)attr->addr, sizeof(gtod))) return -EFAULT; - if (test_kvm_facility(kvm, 139)) - kvm_s390_set_tod_clock_ext(kvm, >od); - 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, >od); 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(>od, (void __user *)attr->addr, sizeof(gtod))) + if (copy_from_user(>od.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, >od); + 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, >od.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, >od); kvm_s390_set_psw_cc(vcpu, 0); return 0;
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(-)