| Message ID | 20221117221758.66326-3-scgl@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: s390: Extend MEM_OP ioctl by storage key checked cmpxchg | expand |
On 11/18/22 05:17, Janis Schoetterl-Glausch wrote: > +For write accesses, the KVM_S390_MEMOP_F_CMPXCHG might be supported. > +In this case, instead of doing an unconditional write, the access occurs only > +if the target location contains the "size" byte long value pointed to by > +"old_p". This is performed as an atomic cmpxchg. "size" must be a power of two > +up to and including 16. > +The value at the target location is written to the location "old_p" points to. > +If the exchange did not take place because the target value doesn't match the > +old value KVM_S390_MEMOP_R_NO_XCHG is returned. > +The KVM_S390_MEMOP_F_CMPXCHG flag is supported if KVM_CAP_S390_MEM_OP_EXTENSION > +has bit 1 (i.e. bit with value 2) set. > Is KVM_S390_MEMOP_F_CMPXCHG supported with conditions (as you implied)?
On Fri, 2022-11-18 at 08:50 +0700, Bagas Sanjaya wrote: > On 11/18/22 05:17, Janis Schoetterl-Glausch wrote: > > +For write accesses, the KVM_S390_MEMOP_F_CMPXCHG might be supported. > > +In this case, instead of doing an unconditional write, the access occurs only > > +if the target location contains the "size" byte long value pointed to by > > +"old_p". This is performed as an atomic cmpxchg. "size" must be a power of two > > +up to and including 16. > > +The value at the target location is written to the location "old_p" points to. > > +If the exchange did not take place because the target value doesn't match the > > +old value KVM_S390_MEMOP_R_NO_XCHG is returned. > > +The KVM_S390_MEMOP_F_CMPXCHG flag is supported if KVM_CAP_S390_MEM_OP_EXTENSION > > +has bit 1 (i.e. bit with value 2) set. > > > > Is KVM_S390_MEMOP_F_CMPXCHG supported with conditions (as you implied)? > I'm afraid I don't quite understand the question. It is supported if the capability says it is, i.e. if bit 1 is set.
On 17/11/2022 23.17, Janis Schoetterl-Glausch wrote: > Describe the semantics of the new KVM_S390_MEMOP_F_CMPXCHG flag for > absolute vm write memops which allows user space to perform (storage key > checked) cmpxchg operations on guest memory. > > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> > --- ... > Supported flags: > * ``KVM_S390_MEMOP_F_CHECK_ONLY`` > * ``KVM_S390_MEMOP_F_SKEY_PROTECTION`` > + * ``KVM_S390_MEMOP_F_CMPXCHG`` > + > +The semantics of the flags common with logical acesses are as for logical > +accesses. > + > +For write accesses, the KVM_S390_MEMOP_F_CMPXCHG might be supported. I'd maybe merge this with the last sentence: For write accesses, the KVM_S390_MEMOP_F_CMPXCHG flag is supported if KVM_CAP_S390_MEM_OP_EXTENSION has bit 1 (i.e. bit with value 2) set. ... and speaking of that, I wonder whether it's maybe a good idea to introduce some #defines for bit 1 / value 2, to avoid the confusion ? > +In this case, instead of doing an unconditional write, the access occurs only > +if the target location contains the "size" byte long value pointed to by > +"old_p". This is performed as an atomic cmpxchg. I had to read the first sentence twice to understand it ... maybe it's easier to understand if you move the "size" part to the second sentence: In this case, instead of doing an unconditional write, the access occurs only if the target location contains value pointed to by "old_p". This is performed as an atomic cmpxchg with the length specified by the "size" parameter. ? > "size" must be a power of two > +up to and including 16. > +The value at the target location is written to the location "old_p" points to. IMHO something like this would be better: The value at the target location is replaced with the value from the location that "old_p" points to. > +If the exchange did not take place because the target value doesn't match the > +old value KVM_S390_MEMOP_R_NO_XCHG is returned. > +The KVM_S390_MEMOP_F_CMPXCHG flag is supported if KVM_CAP_S390_MEM_OP_EXTENSION > +has bit 1 (i.e. bit with value 2) set. Thomas PS: Please take my suggestions with a grain of salt ... I'm not a native speaker either.
On Tue, 2022-11-22 at 08:47 +0100, Thomas Huth wrote: > On 17/11/2022 23.17, Janis Schoetterl-Glausch wrote: > > Describe the semantics of the new KVM_S390_MEMOP_F_CMPXCHG flag for > > absolute vm write memops which allows user space to perform (storage key > > checked) cmpxchg operations on guest memory. > > > > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> > > --- > ... > > Supported flags: > > * ``KVM_S390_MEMOP_F_CHECK_ONLY`` > > * ``KVM_S390_MEMOP_F_SKEY_PROTECTION`` > > + * ``KVM_S390_MEMOP_F_CMPXCHG`` > > + > > +The semantics of the flags common with logical acesses are as for logical > > +accesses. > > + > > +For write accesses, the KVM_S390_MEMOP_F_CMPXCHG might be supported. > > I'd maybe merge this with the last sentence: > > For write accesses, the KVM_S390_MEMOP_F_CMPXCHG flag is supported if > KVM_CAP_S390_MEM_OP_EXTENSION has bit 1 (i.e. bit with value 2) set. Ok. > > ... and speaking of that, I wonder whether it's maybe a good idea to > introduce some #defines for bit 1 / value 2, to avoid the confusion ? Not sure, I don't feel it's too complicated. Where would you define it? Next to the mem_op struct? KVM_S390_MEMOP_EXTENSION_CAP_CMPXCHG? > > > +In this case, instead of doing an unconditional write, the access occurs only > > +if the target location contains the "size" byte long value pointed to by > > +"old_p". This is performed as an atomic cmpxchg. > > I had to read the first sentence twice to understand it ... maybe it's > easier to understand if you move the "size" part to the second sentence: > > In this case, instead of doing an unconditional write, the access occurs > only if the target location contains value pointed to by "old_p". This is > performed as an atomic cmpxchg with the length specified by the "size" > parameter. > > ? Ok. > > > "size" must be a power of two > > +up to and including 16. > > +The value at the target location is written to the location "old_p" points to. > > IMHO something like this would be better: > > The value at the target location is replaced with the value from the > location that "old_p" points to. I'm trying to say the opposite :). I went with this: If the exchange did not take place because the target value doesn't match the old value, KVM_S390_MEMOP_R_NO_XCHG is returned. In this case the value "old_addr" points to is replaced by the target value. > > > +If the exchange did not take place because the target value doesn't match the > > +old value KVM_S390_MEMOP_R_NO_XCHG is returned. > > +The KVM_S390_MEMOP_F_CMPXCHG flag is supported if KVM_CAP_S390_MEM_OP_EXTENSION > > +has bit 1 (i.e. bit with value 2) set. > > Thomas > > PS: Please take my suggestions with a grain of salt ... I'm not a native > speaker either. >
Quoting Janis Schoetterl-Glausch (2022-11-22 14:10:41) > On Tue, 2022-11-22 at 08:47 +0100, Thomas Huth wrote: > > On 17/11/2022 23.17, Janis Schoetterl-Glausch wrote: [...] > > > Supported flags: > > > * ``KVM_S390_MEMOP_F_CHECK_ONLY`` > > > * ``KVM_S390_MEMOP_F_SKEY_PROTECTION`` > > > + * ``KVM_S390_MEMOP_F_CMPXCHG`` > > > + > > > +The semantics of the flags common with logical acesses are as for logical > > > +accesses. > > > + > > > +For write accesses, the KVM_S390_MEMOP_F_CMPXCHG might be supported. > > > > I'd maybe merge this with the last sentence: > > > > For write accesses, the KVM_S390_MEMOP_F_CMPXCHG flag is supported if > > KVM_CAP_S390_MEM_OP_EXTENSION has bit 1 (i.e. bit with value 2) set. > > Ok. > > > > ... and speaking of that, I wonder whether it's maybe a good idea to > > introduce some #defines for bit 1 / value 2, to avoid the confusion ? > > Not sure, I don't feel it's too complicated. Where would you define it? > Next to the mem_op struct? KVM_S390_MEMOP_EXTENSION_CAP_CMPXCHG? I think the define would be a good idea. Location and name sound good to me. You could also replace the hard-coded 0x3 in kvm_vm_ioctl_check_extension() when you have the define.
On Thu, 17 Nov 2022 23:17:51 +0100 Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote: > Describe the semantics of the new KVM_S390_MEMOP_F_CMPXCHG flag for > absolute vm write memops which allows user space to perform (storage key > checked) cmpxchg operations on guest memory. > > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> > --- > Documentation/virt/kvm/api.rst | 21 +++++++++++++++++++-- > 1 file changed, 19 insertions(+), 2 deletions(-) > > diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst > index eee9f857a986..204d128f23e0 100644 > --- a/Documentation/virt/kvm/api.rst > +++ b/Documentation/virt/kvm/api.rst > @@ -3753,7 +3753,8 @@ The fields in each entry are defined as follows: > :Parameters: struct kvm_s390_mem_op (in) > :Returns: = 0 on success, > < 0 on generic error (e.g. -EFAULT or -ENOMEM), > - > 0 if an exception occurred while walking the page tables > + 16 bit program exception code if the access causes such an exception > + other code > maximum 16 bit value with special meaning I would write the number explicitly ( > 65535 or > 0xffff ) > > Read or write data from/to the VM's memory. > The KVM_CAP_S390_MEM_OP_EXTENSION capability specifies what functionality is > @@ -3771,6 +3772,8 @@ Parameters are specified via the following structure:: > struct { > __u8 ar; /* the access register number */ > __u8 key; /* access key, ignored if flag unset */ > + __u8 pad1[6]; /* ignored */ > + __u64 old_p; /* ignored if flag unset */ > }; > __u32 sida_offset; /* offset into the sida */ > __u8 reserved[32]; /* ignored */ > @@ -3853,8 +3856,22 @@ Absolute accesses are permitted for non-protected guests only. > Supported flags: > * ``KVM_S390_MEMOP_F_CHECK_ONLY`` > * ``KVM_S390_MEMOP_F_SKEY_PROTECTION`` > + * ``KVM_S390_MEMOP_F_CMPXCHG`` > + > +The semantics of the flags common with logical acesses are as for logical > +accesses. > + > +For write accesses, the KVM_S390_MEMOP_F_CMPXCHG might be supported. > +In this case, instead of doing an unconditional write, the access occurs only > +if the target location contains the "size" byte long value pointed to by > +"old_p". This is performed as an atomic cmpxchg. "size" must be a power of two > +up to and including 16. > +The value at the target location is written to the location "old_p" points to. > +If the exchange did not take place because the target value doesn't match the > +old value KVM_S390_MEMOP_R_NO_XCHG is returned. > +The KVM_S390_MEMOP_F_CMPXCHG flag is supported if KVM_CAP_S390_MEM_OP_EXTENSION > +has bit 1 (i.e. bit with value 2) set. > > -The semantics of the flags are as for logical accesses. > > SIDA read/write: > ^^^^^^^^^^^^^^^^
diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst index eee9f857a986..204d128f23e0 100644 --- a/Documentation/virt/kvm/api.rst +++ b/Documentation/virt/kvm/api.rst @@ -3753,7 +3753,8 @@ The fields in each entry are defined as follows: :Parameters: struct kvm_s390_mem_op (in) :Returns: = 0 on success, < 0 on generic error (e.g. -EFAULT or -ENOMEM), - > 0 if an exception occurred while walking the page tables + 16 bit program exception code if the access causes such an exception + other code > maximum 16 bit value with special meaning Read or write data from/to the VM's memory. The KVM_CAP_S390_MEM_OP_EXTENSION capability specifies what functionality is @@ -3771,6 +3772,8 @@ Parameters are specified via the following structure:: struct { __u8 ar; /* the access register number */ __u8 key; /* access key, ignored if flag unset */ + __u8 pad1[6]; /* ignored */ + __u64 old_p; /* ignored if flag unset */ }; __u32 sida_offset; /* offset into the sida */ __u8 reserved[32]; /* ignored */ @@ -3853,8 +3856,22 @@ Absolute accesses are permitted for non-protected guests only. Supported flags: * ``KVM_S390_MEMOP_F_CHECK_ONLY`` * ``KVM_S390_MEMOP_F_SKEY_PROTECTION`` + * ``KVM_S390_MEMOP_F_CMPXCHG`` + +The semantics of the flags common with logical acesses are as for logical +accesses. + +For write accesses, the KVM_S390_MEMOP_F_CMPXCHG might be supported. +In this case, instead of doing an unconditional write, the access occurs only +if the target location contains the "size" byte long value pointed to by +"old_p". This is performed as an atomic cmpxchg. "size" must be a power of two +up to and including 16. +The value at the target location is written to the location "old_p" points to. +If the exchange did not take place because the target value doesn't match the +old value KVM_S390_MEMOP_R_NO_XCHG is returned. +The KVM_S390_MEMOP_F_CMPXCHG flag is supported if KVM_CAP_S390_MEM_OP_EXTENSION +has bit 1 (i.e. bit with value 2) set. -The semantics of the flags are as for logical accesses. SIDA read/write: ^^^^^^^^^^^^^^^^
Describe the semantics of the new KVM_S390_MEMOP_F_CMPXCHG flag for absolute vm write memops which allows user space to perform (storage key checked) cmpxchg operations on guest memory. Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> --- Documentation/virt/kvm/api.rst | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)