Re: [PATCH v3 1/2] KVM: s390: pv: don't allow userspace to set the clock under PV

[Nico Boehr <nrb@xxxxxxxxxxxxx>]

Quoting Janosch Frank (2022-10-10 17:20:10)
[...]
> This will ONLY result in a warning and there's no way that this can 
> result in QEMU crashing, right?

Yes, QEMU code in hw/s390x/tod-kvm.c just sets an Error pointer which is then
passed to warn_report(). So no crash is possible.

> > 
> > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> > index b7ef0b71014d..0a8019b14c8f 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);
> > +
> >   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, &gtod);
> > +     __kvm_s390_set_tod_clock(kvm, &gtod);
> >   
> >       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, &gtod);
> > +     __kvm_s390_set_tod_clock(kvm, &gtod);
> >       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;
> >   
> 
> Add comment:
> For a protected guest the TOD is managed by the Ultravisor so trying to 
> change it will never bring the expected results.

Yes, good point. Done.

> -EOPNOTSUPP is a new return code for the tod attribute, therefore 
> programs using it might need a fix to be able to handle it.

Hmm, yes indeed.

Another alternative to consider might be -EINVAL. That is already specified as a
return for KVM_S390_VM_TOD_HIGH and KVM_S390_VM_TOD_EXT (in different
circumstances though). However, it's missing from KVM_S390_VM_TOD_LOW...

> And as -EOPNOTSUPP has never been used before you'll also need to 
> update: Documentation/virt/kvm/devices/vm.rst

Yeah, I will update the docs and use -EOPNOTSUPP for now. If someone argues for
-EINVAL, I can still change it.