Re: [PATCH v5 01/10] KVM: s390: Extend MEM_OP ioctl by storage key checked cmpxchg

[Janis Schoetterl-Glausch <scgl@linux.ibm.com>]

On Wed, 2023-01-11 at 18:26 +0100, Janosch Frank wrote:
> On 1/11/23 16:19, Janis Schoetterl-Glausch wrote:
> > On Wed, 2023-01-11 at 10:35 +0100, Janosch Frank wrote:
> > > On 1/10/23 21:26, Janis Schoetterl-Glausch wrote:
> > > > User space can use the MEM_OP ioctl to make storage key checked reads
> > > > and writes to the guest, however, it has no way of performing atomic,
> > > > key checked, accesses to the guest.
> > > > Extend the MEM_OP ioctl in order to allow for this, by adding a cmpxchg
> > > > mode. For now, support this mode for absolute accesses only.
> > > > 
> > > > This mode can be use, for example, to set the device-state-change
> > > > indicator and the adapter-local-summary indicator atomically.
> > > > 
> > > > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> > > > ---
> > > >    include/uapi/linux/kvm.h |   7 +++
> > > >    arch/s390/kvm/gaccess.h  |   3 ++
> > > >    arch/s390/kvm/gaccess.c  | 102 +++++++++++++++++++++++++++++++++++++++
> > > >    arch/s390/kvm/kvm-s390.c |  41 +++++++++++++++-
> > > >    4 files changed, 151 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> > > > index 55155e262646..452f43c1cc34 100644
> > > > --- a/include/uapi/linux/kvm.h
> > > > +++ b/include/uapi/linux/kvm.h
> > > > @@ -583,6 +583,8 @@ struct kvm_s390_mem_op {
> > > >    		struct {
> > > >    			__u8 ar;	/* the access register number */
> > > >    			__u8 key;	/* access key, ignored if flag unset */
> > > > +			__u8 pad1[6];	/* ignored */
> > > > +			__u64 old_addr;	/* ignored if flag unset */
> 
> Which flag?
> Maybe that would be clearer with a cmpxchg_ prefix.

Yes.
> 
> > > >    		};
> > > >    		__u32 sida_offset; /* offset into the sida */
> > > >    		__u8 reserved[32]; /* ignored */
> > > > @@ -599,6 +601,11 @@ struct kvm_s390_mem_op {
> > > >    #define KVM_S390_MEMOP_F_CHECK_ONLY		(1ULL << 0)
> > > >    #define KVM_S390_MEMOP_F_INJECT_EXCEPTION	(1ULL << 1)
> > > >    #define KVM_S390_MEMOP_F_SKEY_PROTECTION	(1ULL << 2)
> > > > +#define KVM_S390_MEMOP_F_CMPXCHG		(1ULL << 3)
> > > > +/* flags specifying extension support */
> 
> via KVM_CAP_S390_MEM_OP_EXTENSION
> 
> This is part of the CAP api but is nestled between the memop api.
> I'm not entirely happy about that.

Yes, I wasn't sure where to put it.
> 
> > > 
> > > Would that fit behind the bit shifts without getting into the "line too
> > > long" territory?
> > 
> > Bit shifts or the next line?
> 
> Seems like I didn't see that this is grouped to the next line so forget 
> about my comment.
> 
> > > 
> > > \n please
> > 
> > Not sure about all that, this is the way it looks right now:
> > 
> > /* types for kvm_s390_mem_op->op */
> > #define KVM_S390_MEMOP_LOGICAL_READ     0
> > #define KVM_S390_MEMOP_LOGICAL_WRITE    1
> > #define KVM_S390_MEMOP_SIDA_READ        2
> > #define KVM_S390_MEMOP_SIDA_WRITE       3
> > #define KVM_S390_MEMOP_ABSOLUTE_READ    4
> > #define KVM_S390_MEMOP_ABSOLUTE_WRITE   5
> 
> > /* flags for kvm_s390_mem_op->flags */
> > #define KVM_S390_MEMOP_F_CHECK_ONLY             (1ULL << 0)
> > #define KVM_S390_MEMOP_F_INJECT_EXCEPTION       (1ULL << 1)
> > #define KVM_S390_MEMOP_F_SKEY_PROTECTION        (1ULL << 2)
> > #define KVM_S390_MEMOP_F_CMPXCHG                (1ULL << 3)
> 
> > /* flags specifying extension support */
> 
> > #define KVM_S390_MEMOP_EXTENSION_CAP_CMPXCHG 0x2
> 
> #define KVM_S390_MEMOP_EXTENSION_CAP_ABSOLUTE 1 << 0

Unfortunately, I designed this badly for the first memop extension,
the absolute memop is supported if extension_cap > 0.
But we can always also set bit 0 in that case.

> #define KVM_S390_MEMOP_EXTENSION_CAP_CMPXCHG 1 << 1
> 
> Or maybe BASE instead of ABSOLUTE
> 
> 
> > /* Non program exception return codes (pgm codes are 16 bit) */
> > #define KVM_S390_MEMOP_R_NO_XCHG                (1 << 16)
> > 
> > Seems consistent to me.
> 
> Consistent and hardly readable once you add two more "categories" of values.

I'll add newlines then.
> 
> > > 
> > > > +/* Non program exception return codes (pgm codes are 16 bit) */
> > > > +#define KVM_S390_MEMOP_R_NO_XCHG		(1 << 16)
> > > >    
> > > >    /* for KVM_INTERRUPT */
> > > >    struct kvm_interrupt {
> > > > diff --git a/arch/s390/kvm/gaccess.h b/arch/s390/kvm/gaccess.h
> > > > index 9408d6cc8e2c..92a3b9fb31ec 100644
> > > > --- a/arch/s390/kvm/gaccess.h
> > > > +++ b/arch/s390/kvm/gaccess.h
> > > > @@ -206,6 +206,9 @@ int access_guest_with_key(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar,
> > > >    int access_guest_real(struct kvm_vcpu *vcpu, unsigned long gra,
> > > >    		      void *data, unsigned long len, enum gacc_mode mode);
> > > >    
> > > > +int cmpxchg_guest_abs_with_key(struct kvm *kvm, gpa_t gpa, int len,
> > > > +			       __uint128_t *old, __uint128_t new, u8 access_key);
> > > > +
> > > >    /**
> > > >     * write_guest_with_key - copy data from kernel space to guest space
> > > >     * @vcpu: virtual cpu
> > > > diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
> > > > index 0243b6e38d36..6165e761a637 100644
> > > > --- a/arch/s390/kvm/gaccess.c
> > > > +++ b/arch/s390/kvm/gaccess.c
> > > > @@ -1161,6 +1161,108 @@ int access_guest_real(struct kvm_vcpu *vcpu, unsigned long gra,
> > > >    	return rc;
> > > >    }
> > > >    
> > > > +/**
> > > > + * cmpxchg_guest_abs_with_key() - Perform cmpxchg on guest absolute address.
> > > > + * @kvm: Virtual machine instance.
> > > > + * @gpa: Absolute guest address of the location to be changed.
> > > > + * @len: Operand length of the cmpxchg, required: 1 <= len <= 16. Providing a
> > > > + *       non power of two will result in failure.
> > > > + * @old_addr: Pointer to old value. If the location at @gpa contains this value, the
> > > > + *         exchange will succeed. After calling cmpxchg_guest_abs_with_key() *@old
> > > > + *         contains the value at @gpa before the attempt to exchange the value.
> > > > + * @new: The value to place at @gpa.
> > > > + * @access_key: The access key to use for the guest access.
> > > > + *
> > > > + * Atomically exchange the value at @gpa by @new, if it contains *@old.
> > > > + * Honors storage keys.
> > > > + *
> > > > + * Return: * 0: successful exchange
> > > > + *         * 1: exchange unsuccessful
> > > > + *         * a program interruption code indicating the reason cmpxchg could
> > > > + *           not be attempted
> > > 
> > >   > 1 Access related program interruption code indicating the reason
> > > cmpxchg could not be attempted
> > > 
> > > < 1 Kernel / input data error codes
> > 
> > I think I'll do it like I said in the email to Thomas, that way it's maximally
> > explicit about the return values one might get.
> 
> This shows me that we're overloading the return value way too much.
> Not just of this function but also of the memop with 
> KVM_S390_MEMOP_R_NO_XCHG.
> 
> A return of < 0, 0, 1 and a passed int reference for the PGM codes 
> that's updated if the rc is 1 would make this clearer.

The return code is complicated, using effectively two return codes does cleanly
separate the possible error types, but also means one has to check two return codes.
I'm fine with that, but in that case it shouldn't be the PGM code that gets separated,
but the success of the xchg. So <0 kernel error, >0 PGM code, like everywhere else
and ==0 -> check *success.
> 
> Btw. could a user specify check + cmpxchange as the flags?
> Are we checking that and I've missed to see such a check?

Yes, what that does is basically to check if the cmpxchg args are valid.
Then it checks if the target address is writable.
Not much code necessary for that other than not doing the cmpxchg if check_only
is set.
> 
> 
> I don't like that we throw in yet another feature as a flag thereby 
> blowing up kvm_s390_vm_mem_op() and adding new branches to it. I'll need 
> to think about that some more.
> 
[...]