diff mbox series

[v3,2/9] Documentation: KVM: s390: Describe KVM_S390_MEMOP_F_CMPXCHG

Message ID 20221117221758.66326-3-scgl@linux.ibm.com
State New
Headers
Series KVM: s390: Extend MEM_OP ioctl by storage key checked cmpxchg |

Commit Message

Janis Schoetterl-Glausch Nov. 17, 2022, 10:17 p.m. UTC 
  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(-)

Comments

Bagas Sanjaya Nov. 18, 2022, 1:50 a.m. UTC | #1 
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)?
Janis Schoetterl-Glausch Nov. 21, 2022, 5:44 p.m. UTC | #2 
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.
Thomas Huth Nov. 22, 2022, 7:47 a.m. UTC | #3 
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.
Janis Schoetterl-Glausch Nov. 22, 2022, 1:10 p.m. UTC | #4 
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.
>
Nico Boehr Nov. 25, 2022, 8:52 a.m. UTC | #5 
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.
Claudio Imbrenda Dec. 1, 2022, 4:21 p.m. UTC | #6 
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 mbox series

Patch

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:
 ^^^^^^^^^^^^^^^^