[kvm-unit-tests PATCH] lib: s390x: css: Name inline assembly arguments and clean them up

[kvm-unit-tests PATCH] lib: s390x: css: Name inline assembly arguments and clean them up

[Janosch Frank]

Less need to count the operands makes the code easier to read.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---

This one has been gathering dust for a while.
rfc->v1: Moved to Q constraint (thanks Heiko)

---
 lib/s390x/css.h | 76 ++++++++++++++++++++++++-------------------------
 1 file changed, 38 insertions(+), 38 deletions(-)

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
index 504b3f14..42c5830c 100644
--- a/lib/s390x/css.h
+++ b/lib/s390x/css.h
@@ -135,11 +135,11 @@ static inline int ssch(unsigned long schid, struct orb *addr)
 	int cc;
 
 	asm volatile(
-		"	ssch	0(%2)\n"
-		"	ipm	%0\n"
-		"	srl	%0,28\n"
-		: "=d" (cc)
-		: "d" (reg1), "a" (addr), "m" (*addr)
+		"	ssch	%[addr]\n"
+		"	ipm	%[cc]\n"
+		"	srl	%[cc],28\n"
+		: [cc] "=d" (cc)
+		: "d" (reg1), [addr] "Q" (*addr)
 		: "cc", "memory");
 	return cc;
 }
@@ -152,11 +152,11 @@ static inline int stsch(unsigned long schid, struct schib *addr)
 
 	asm volatile(
 		"       tmll    %[bogus_cc],3\n"
-		"	stsch	0(%3)\n"
-		"	ipm	%0\n"
-		"	srl	%0,28"
-		: "=d" (cc), "=m" (*addr)
-		: "d" (reg1), "a" (addr), [bogus_cc] "d" (bogus_cc)
+		"	stsch	%[addr]\n"
+		"	ipm	%[cc]\n"
+		"	srl	%[cc],28"
+		: [cc] "=d" (cc), [addr] "=Q" (*addr)
+		: "d" (reg1), [bogus_cc] "d" (bogus_cc)
 		: "cc");
 	return cc;
 }
@@ -167,11 +167,11 @@ static inline int msch(unsigned long schid, struct schib *addr)
 	int cc;
 
 	asm volatile(
-		"	msch	0(%3)\n"
-		"	ipm	%0\n"
-		"	srl	%0,28"
-		: "=d" (cc)
-		: "d" (reg1), "m" (*addr), "a" (addr)
+		"	msch	%[addr]\n"
+		"	ipm	%[cc]\n"
+		"	srl	%[cc],28"
+		: [cc] "=d" (cc)
+		: "d" (reg1), [addr] "Q" (*addr)
 		: "cc");
 	return cc;
 }
@@ -184,11 +184,11 @@ static inline int tsch(unsigned long schid, struct irb *addr)
 
 	asm volatile(
 		"       tmll    %[bogus_cc],3\n"
-		"	tsch	0(%3)\n"
-		"	ipm	%0\n"
-		"	srl	%0,28"
-		: "=d" (cc), "=m" (*addr)
-		: "d" (reg1), "a" (addr), [bogus_cc] "d" (bogus_cc)
+		"	tsch	%[addr]\n"
+		"	ipm	%[cc]\n"
+		"	srl	%[cc],28"
+		: [cc] "=d" (cc), [addr] "=Q" (*addr)
+		: "d" (reg1), [bogus_cc] "d" (bogus_cc)
 		: "cc");
 	return cc;
 }
@@ -200,9 +200,9 @@ static inline int hsch(unsigned long schid)
 
 	asm volatile(
 		"	hsch\n"
-		"	ipm	%0\n"
-		"	srl	%0,28"
-		: "=d" (cc)
+		"	ipm	%[cc]\n"
+		"	srl	%[cc],28"
+		: [cc] "=d" (cc)
 		: "d" (reg1)
 		: "cc");
 	return cc;
@@ -215,9 +215,9 @@ static inline int xsch(unsigned long schid)
 
 	asm volatile(
 		"	xsch\n"
-		"	ipm	%0\n"
-		"	srl	%0,28"
-		: "=d" (cc)
+		"	ipm	%[cc]\n"
+		"	srl	%cc,28"
+		: [cc] "=d" (cc)
 		: "d" (reg1)
 		: "cc");
 	return cc;
@@ -230,9 +230,9 @@ static inline int csch(unsigned long schid)
 
 	asm volatile(
 		"	csch\n"
-		"	ipm	%0\n"
-		"	srl	%0,28"
-		: "=d" (cc)
+		"	ipm	%[cc]\n"
+		"	srl	%[cc],28"
+		: [cc] "=d" (cc)
 		: "d" (reg1)
 		: "cc");
 	return cc;
@@ -245,9 +245,9 @@ static inline int rsch(unsigned long schid)
 
 	asm volatile(
 		"	rsch\n"
-		"	ipm	%0\n"
-		"	srl	%0,28"
-		: "=d" (cc)
+		"	ipm	%[cc]\n"
+		"	srl	%[cc],28"
+		: [cc] "=d" (cc)
 		: "d" (reg1)
 		: "cc");
 	return cc;
@@ -262,9 +262,9 @@ static inline int rchp(unsigned long chpid)
 	asm volatile(
 		"       tmll    %[bogus_cc],3\n"
 		"	rchp\n"
-		"	ipm	%0\n"
-		"	srl	%0,28"
-		: "=d" (cc)
+		"	ipm	%[cc]\n"
+		"	srl	%[cc],28"
+		: [cc] "=d" (cc)
 		: "d" (reg1), [bogus_cc] "d" (bogus_cc)
 		: "cc");
 	return cc;
@@ -369,9 +369,9 @@ static inline int _chsc(void *p)
 	int cc;
 
 	asm volatile(" .insn   rre,0xb25f0000,%2,0\n"
-		     " ipm     %0\n"
-		     " srl     %0,28\n"
-		     : "=d" (cc), "=m" (p)
+		     " ipm     %[cc]\n"
+		     " srl     %[cc],28\n"
+		     : [cc] "=d" (cc), "=m" (p)
 		     : "d" (p), "m" (p)
 		     : "cc");
 
-- 
2.43.0

Re: [kvm-unit-tests PATCH] lib: s390x: css: Name inline assembly arguments and clean them up

[Nico Boehr]

On Tue Feb 4, 2025 at 10:51 AM CET, Janosch Frank wrote:
> @@ -215,9 +215,9 @@ static inline int xsch(unsigned long schid)
>  
>  	asm volatile(
>  		"	xsch\n"
> -		"	ipm	%0\n"
> -		"	srl	%0,28"
> -		: "=d" (cc)
> +		"	ipm	%[cc]\n"
> +		"	srl	%cc,28"

Should this be:
		"	srl	%[cc],28"
instead?

With that fixed (if it needs fixing):

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

Re: [kvm-unit-tests PATCH] lib: s390x: css: Name inline assembly arguments and clean them up

[Janosch Frank]

On 2/4/25 1:55 PM, Nico Boehr wrote:
> On Tue Feb 4, 2025 at 10:51 AM CET, Janosch Frank wrote:
>> @@ -215,9 +215,9 @@ static inline int xsch(unsigned long schid)
>>   
>>   	asm volatile(
>>   		"	xsch\n"
>> -		"	ipm	%0\n"
>> -		"	srl	%0,28"
>> -		: "=d" (cc)
>> +		"	ipm	%[cc]\n"
>> +		"	srl	%cc,28"
> 
> Should this be:
> 		"	srl	%[cc],28"
> instead?
> 
> With that fixed (if it needs fixing):
> 
> Reviewed-by: Nico Boehr <nrb@linux.ibm.com>
> 

Thanks!
Fixed and applied.

Re: [kvm-unit-tests PATCH] lib: s390x: css: Name inline assembly arguments and clean them up

[Claudio Imbrenda]

On Tue,  4 Feb 2025 09:51:33 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> Less need to count the operands makes the code easier to read.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> 
> This one has been gathering dust for a while.
> rfc->v1: Moved to Q constraint (thanks Heiko)
> 
> ---

[...]

>  	asm volatile(" .insn   rre,0xb25f0000,%2,0\n"
> -		     " ipm     %0\n"
> -		     " srl     %0,28\n"
> -		     : "=d" (cc), "=m" (p)
> +		     " ipm     %[cc]\n"
> +		     " srl     %[cc],28\n"
> +		     : [cc] "=d" (cc), "=m" (p)
>  		     : "d" (p), "m" (p)

this bit (which you did not touch) is actually the most confusing to me.
what's the point of separately specifying both "d" and "m" constraints
for (p) ? (and it also has a "=m" in the output clobberlist)

>  		     : "cc");
>  

Re: [kvm-unit-tests PATCH] lib: s390x: css: Name inline assembly arguments and clean them up

[Janosch Frank]

On 2/5/25 11:25 AM, Claudio Imbrenda wrote:
> On Tue,  4 Feb 2025 09:51:33 +0000
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> Less need to count the operands makes the code easier to read.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>
>> This one has been gathering dust for a while.
>> rfc->v1: Moved to Q constraint (thanks Heiko)
>>
>> ---
> 
> [...]
> 
>>   	asm volatile(" .insn   rre,0xb25f0000,%2,0\n"
>> -		     " ipm     %0\n"
>> -		     " srl     %0,28\n"
>> -		     : "=d" (cc), "=m" (p)
>> +		     " ipm     %[cc]\n"
>> +		     " srl     %[cc],28\n"
>> +		     : [cc] "=d" (cc), "=m" (p)
>>   		     : "d" (p), "m" (p)
> 
> this bit (which you did not touch) is actually the most confusing to me.
> what's the point of separately specifying both "d" and "m" constraints
> for (p) ? (and it also has a "=m" in the output clobberlist)

I'll add a second patch to bring this into line with the kernel's 
ioasm.c implementation.

Re: [kvm-unit-tests PATCH] lib: s390x: css: Name inline assembly arguments and clean them up

[Janosch Frank]

On 2/5/25 11:25 AM, Claudio Imbrenda wrote:
> On Tue,  4 Feb 2025 09:51:33 +0000
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> Less need to count the operands makes the code easier to read.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>
>> This one has been gathering dust for a while.
>> rfc->v1: Moved to Q constraint (thanks Heiko)
>>
>> ---
> 
> [...]
> 
>>   	asm volatile(" .insn   rre,0xb25f0000,%2,0\n"
>> -		     " ipm     %0\n"
>> -		     " srl     %0,28\n"
>> -		     : "=d" (cc), "=m" (p)
>> +		     " ipm     %[cc]\n"
>> +		     " srl     %[cc],28\n"
>> +		     : [cc] "=d" (cc), "=m" (p)
>>   		     : "d" (p), "m" (p)
> 
> this bit (which you did not touch) is actually the most confusing to me.
> what's the point of separately specifying both "d" and "m" constraints
> for (p) ? (and it also has a "=m" in the output clobberlist)

I consulted the kernel code as well as Heiko and the architecture.

CHSC is one of those request/response do everything instructions and is 
similar to sclp. A header is read from memory and a response is written 
below the header. The addressed memory needs to be page aligned and can 
be up to a page in size.

Which means:
  - We need the address in R1
  - CPU reads from the memory area designated by R1
  - CPU writes to the memory area designated by R1

My guess is that nobody bothered defining all of the structs and that's 
how we ended up here. If you look at the kernel assembly you'll notice a 
page size typedef for the +m clobber of the memory area pointer.

Heiko suggested to drop the two "m" clobbers and just add a generic 
memory clobber. If we even want to touch this at all...

Re: [kvm-unit-tests PATCH] lib: s390x: css: Name inline assembly arguments and clean them up

[Claudio Imbrenda]

On Thu, 6 Feb 2025 11:36:11 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 2/5/25 11:25 AM, Claudio Imbrenda wrote:
> > On Tue,  4 Feb 2025 09:51:33 +0000
> > Janosch Frank <frankja@linux.ibm.com> wrote:
> >   
> >> Less need to count the operands makes the code easier to read.
> >>
> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> >> ---
> >>
> >> This one has been gathering dust for a while.
> >> rfc->v1: Moved to Q constraint (thanks Heiko)
> >>
> >> ---  
> > 
> > [...]
> >   
> >>   	asm volatile(" .insn   rre,0xb25f0000,%2,0\n"
> >> -		     " ipm     %0\n"
> >> -		     " srl     %0,28\n"
> >> -		     : "=d" (cc), "=m" (p)
> >> +		     " ipm     %[cc]\n"
> >> +		     " srl     %[cc],28\n"
> >> +		     : [cc] "=d" (cc), "=m" (p)
> >>   		     : "d" (p), "m" (p)  
> > 
> > this bit (which you did not touch) is actually the most confusing to me.
> > what's the point of separately specifying both "d" and "m" constraints
> > for (p) ? (and it also has a "=m" in the output clobberlist)  
> 
> I consulted the kernel code as well as Heiko and the architecture.
> 
> CHSC is one of those request/response do everything instructions and is 
> similar to sclp. A header is read from memory and a response is written 
> below the header. The addressed memory needs to be page aligned and can 
> be up to a page in size.
> 
> Which means:
>   - We need the address in R1
>   - CPU reads from the memory area designated by R1
>   - CPU writes to the memory area designated by R1
> 
> My guess is that nobody bothered defining all of the structs and that's 
> how we ended up here. If you look at the kernel assembly you'll notice a 
> page size typedef for the +m clobber of the memory area pointer.
> 
> Heiko suggested to drop the two "m" clobbers and just add a generic 
> memory clobber. If we even want to touch this at all...

tbh, yes, I would like to have it as simple as possible.
this is not super performance-sensitive, IMHO it's more important that
the code is readable and understandable.

Re: [kvm-unit-tests PATCH] lib: s390x: css: Name inline assembly arguments and clean them up

[Nico Boehr]

On Thu Feb 6, 2025 at 11:36 AM CET, Janosch Frank wrote:
[...]
> Heiko suggested to drop the two "m" clobbers and just add a generic 
> memory clobber. If we even want to touch this at all...

We are a testing framework and don't care about the last bit of
performance. I would argue that we can just always go with a memory
clobber, since they are less error-prone and easier to understand.

If you don't mind, go with a memory clobber in v2. Otherwise let me know, I
think I may have a few other cleanups lying around and could do it when I
send them out.

Re: [kvm-unit-tests PATCH] lib: s390x: css: Name inline assembly arguments and clean them up

[Claudio Imbrenda]

On Tue,  4 Feb 2025 09:51:33 +0000
Janosch Frank <frankja@linux.ibm.com> wrote:

> Less need to count the operands makes the code easier to read.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>


> ---
> 
> This one has been gathering dust for a while.
> rfc->v1: Moved to Q constraint (thanks Heiko)
> 
> ---
>  lib/s390x/css.h | 76 ++++++++++++++++++++++++-------------------------
>  1 file changed, 38 insertions(+), 38 deletions(-)
> 
> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
> index 504b3f14..42c5830c 100644
> --- a/lib/s390x/css.h
> +++ b/lib/s390x/css.h
> @@ -135,11 +135,11 @@ static inline int ssch(unsigned long schid, struct orb *addr)
>  	int cc;
>  
>  	asm volatile(
> -		"	ssch	0(%2)\n"
> -		"	ipm	%0\n"
> -		"	srl	%0,28\n"
> -		: "=d" (cc)
> -		: "d" (reg1), "a" (addr), "m" (*addr)
> +		"	ssch	%[addr]\n"
> +		"	ipm	%[cc]\n"
> +		"	srl	%[cc],28\n"
> +		: [cc] "=d" (cc)
> +		: "d" (reg1), [addr] "Q" (*addr)
>  		: "cc", "memory");
>  	return cc;
>  }
> @@ -152,11 +152,11 @@ static inline int stsch(unsigned long schid, struct schib *addr)
>  
>  	asm volatile(
>  		"       tmll    %[bogus_cc],3\n"
> -		"	stsch	0(%3)\n"
> -		"	ipm	%0\n"
> -		"	srl	%0,28"
> -		: "=d" (cc), "=m" (*addr)
> -		: "d" (reg1), "a" (addr), [bogus_cc] "d" (bogus_cc)
> +		"	stsch	%[addr]\n"
> +		"	ipm	%[cc]\n"
> +		"	srl	%[cc],28"
> +		: [cc] "=d" (cc), [addr] "=Q" (*addr)
> +		: "d" (reg1), [bogus_cc] "d" (bogus_cc)
>  		: "cc");
>  	return cc;
>  }
> @@ -167,11 +167,11 @@ static inline int msch(unsigned long schid, struct schib *addr)
>  	int cc;
>  
>  	asm volatile(
> -		"	msch	0(%3)\n"
> -		"	ipm	%0\n"
> -		"	srl	%0,28"
> -		: "=d" (cc)
> -		: "d" (reg1), "m" (*addr), "a" (addr)
> +		"	msch	%[addr]\n"
> +		"	ipm	%[cc]\n"
> +		"	srl	%[cc],28"
> +		: [cc] "=d" (cc)
> +		: "d" (reg1), [addr] "Q" (*addr)
>  		: "cc");
>  	return cc;
>  }
> @@ -184,11 +184,11 @@ static inline int tsch(unsigned long schid, struct irb *addr)
>  
>  	asm volatile(
>  		"       tmll    %[bogus_cc],3\n"
> -		"	tsch	0(%3)\n"
> -		"	ipm	%0\n"
> -		"	srl	%0,28"
> -		: "=d" (cc), "=m" (*addr)
> -		: "d" (reg1), "a" (addr), [bogus_cc] "d" (bogus_cc)
> +		"	tsch	%[addr]\n"
> +		"	ipm	%[cc]\n"
> +		"	srl	%[cc],28"
> +		: [cc] "=d" (cc), [addr] "=Q" (*addr)
> +		: "d" (reg1), [bogus_cc] "d" (bogus_cc)
>  		: "cc");
>  	return cc;
>  }
> @@ -200,9 +200,9 @@ static inline int hsch(unsigned long schid)
>  
>  	asm volatile(
>  		"	hsch\n"
> -		"	ipm	%0\n"
> -		"	srl	%0,28"
> -		: "=d" (cc)
> +		"	ipm	%[cc]\n"
> +		"	srl	%[cc],28"
> +		: [cc] "=d" (cc)
>  		: "d" (reg1)
>  		: "cc");
>  	return cc;
> @@ -215,9 +215,9 @@ static inline int xsch(unsigned long schid)
>  
>  	asm volatile(
>  		"	xsch\n"
> -		"	ipm	%0\n"
> -		"	srl	%0,28"
> -		: "=d" (cc)
> +		"	ipm	%[cc]\n"
> +		"	srl	%cc,28"
> +		: [cc] "=d" (cc)
>  		: "d" (reg1)
>  		: "cc");
>  	return cc;
> @@ -230,9 +230,9 @@ static inline int csch(unsigned long schid)
>  
>  	asm volatile(
>  		"	csch\n"
> -		"	ipm	%0\n"
> -		"	srl	%0,28"
> -		: "=d" (cc)
> +		"	ipm	%[cc]\n"
> +		"	srl	%[cc],28"
> +		: [cc] "=d" (cc)
>  		: "d" (reg1)
>  		: "cc");
>  	return cc;
> @@ -245,9 +245,9 @@ static inline int rsch(unsigned long schid)
>  
>  	asm volatile(
>  		"	rsch\n"
> -		"	ipm	%0\n"
> -		"	srl	%0,28"
> -		: "=d" (cc)
> +		"	ipm	%[cc]\n"
> +		"	srl	%[cc],28"
> +		: [cc] "=d" (cc)
>  		: "d" (reg1)
>  		: "cc");
>  	return cc;
> @@ -262,9 +262,9 @@ static inline int rchp(unsigned long chpid)
>  	asm volatile(
>  		"       tmll    %[bogus_cc],3\n"
>  		"	rchp\n"
> -		"	ipm	%0\n"
> -		"	srl	%0,28"
> -		: "=d" (cc)
> +		"	ipm	%[cc]\n"
> +		"	srl	%[cc],28"
> +		: [cc] "=d" (cc)
>  		: "d" (reg1), [bogus_cc] "d" (bogus_cc)
>  		: "cc");
>  	return cc;
> @@ -369,9 +369,9 @@ static inline int _chsc(void *p)
>  	int cc;
>  
>  	asm volatile(" .insn   rre,0xb25f0000,%2,0\n"
> -		     " ipm     %0\n"
> -		     " srl     %0,28\n"
> -		     : "=d" (cc), "=m" (p)
> +		     " ipm     %[cc]\n"
> +		     " srl     %[cc],28\n"
> +		     : [cc] "=d" (cc), "=m" (p)
>  		     : "d" (p), "m" (p)
>  		     : "cc");
>