On Mon, 29 Aug 2022 09:56:02 +0200
Nico Boehr <nrb@xxxxxxxxxxxxx> wrote:
> When running under PV, the guest's TOD clock is under control of the
> ultravisor and the hypervisor isn't allowed to change it. Hence, don't
> allow userspace to change the guest's TOD clock by returning
> -EOPNOTSUPP.
>
> When userspace changes the guest's TOD clock, KVM updates its
> kvm.arch.epoch field and, in addition, the epoch field in all state
> descriptions of all VCPUs.
>
> But, under PV, the ultravisor will ignore the epoch field in the state
> description and simply overwrite it on next SIE exit with the actual
> guest epoch. This leads to KVM having an incorrect view of the guest's
> TOD clock: it has updated its internal kvm.arch.epoch field, but the
> ultravisor ignores the field in the state description.
>
> Whenever a guest is now waiting for a clock comparator, KVM will
> incorrectly calculate the time when the guest should wake up, possibly
> causing the guest to sleep for much longer than expected.
>
> With this change, kvm_s390_set_tod() will now take the kvm->lock to be
> able to call kvm_s390_pv_is_protected(). Since kvm_s390_set_tod_clock()
> also takes kvm->lock, use __kvm_s390_set_tod_clock() instead.
>
> Fixes: 0f3035047140 ("KVM: s390: protvirt: Do only reset registers that are accessible")
> Signed-off-by: Nico Boehr <nrb@xxxxxxxxxxxxx>
> ---
> arch/s390/kvm/kvm-s390.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index edfd4bbd0cba..f4ee88e787fe 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -1207,6 +1207,8 @@ static int kvm_s390_vm_get_migration(struct kvm *kvm,
> return 0;
> }
>
> +static void __kvm_s390_set_tod_clock(struct kvm *kvm, const struct kvm_s390_vm_tod_clock *gtod);
are there any users of kvm_s390_set_tod_clock left after this patch?
maybe that wrapper can be removed (also from the headers), and we can
keep the function with the __
it's perhaps cleaner to have a second patch to remove the unused stuff
> +
> static int kvm_s390_set_tod_ext(struct kvm *kvm, struct kvm_device_attr *attr)
> {
> struct kvm_s390_vm_tod_clock gtod;
> @@ -1216,7 +1218,7 @@ static int kvm_s390_set_tod_ext(struct kvm *kvm, struct kvm_device_attr *attr)
>
> if (!test_kvm_facility(kvm, 139) && gtod.epoch_idx)
> return -EINVAL;
> - kvm_s390_set_tod_clock(kvm, >od);
> + __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);
> @@ -1247,7 +1249,7 @@ static int kvm_s390_set_tod_low(struct kvm *kvm, struct kvm_device_attr *attr)
> sizeof(gtod.tod)))
> return -EFAULT;
>
> - kvm_s390_set_tod_clock(kvm, >od);
> + __kvm_s390_set_tod_clock(kvm, >od);
> VM_EVENT(kvm, 3, "SET: TOD base: 0x%llx", gtod.tod);
> return 0;
> }
> @@ -1259,6 +1261,12 @@ static int kvm_s390_set_tod(struct kvm *kvm, struct kvm_device_attr *attr)
> if (attr->flags)
> return -EINVAL;
>
> + mutex_lock(&kvm->lock);
> + if (kvm_s390_pv_is_protected(kvm)) {
> + ret = -EOPNOTSUPP;
> + goto out_unlock;
> + }
> +
> switch (attr->attr) {
> case KVM_S390_VM_TOD_EXT:
> ret = kvm_s390_set_tod_ext(kvm, attr);
> @@ -1273,6 +1281,9 @@ static int kvm_s390_set_tod(struct kvm *kvm, struct kvm_device_attr *attr)
> ret = -ENXIO;
> break;
> }
> +
> +out_unlock:
> + mutex_unlock(&kvm->lock);
> return ret;
> }
>
