[kvm-unit-tests PATCH] s390x: Ignore gcc 12 warnings for low addresses

[kvm-unit-tests PATCH] s390x: Ignore gcc 12 warnings for low addresses

[Janis Schoetterl-Glausch]

gcc 12 warns if a memory operand to inline asm points to memory in the
first 4k bytes. However, in our case, these operands are fine, either
because we actually want to use that memory, or expect and handle the
resulting exception.
Therefore, silence the warning.

Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---

Alternatives:
 * Use memory clobber instead of memory output
   Use address in register input instead of memory input
       (may require WRITE_ONCE)
 * Disable the warning globally
 * Don't use gcc 12.0, with newer versions --param=min-pagesize=0 might
   avoid the problem

 lib/s390x/asm/cpacf.h | 7 +++++++
 s390x/skey.c          | 7 +++++++
 2 files changed, 14 insertions(+)

diff --git a/lib/s390x/asm/cpacf.h b/lib/s390x/asm/cpacf.h
index 685262b0..02e603c8 100644
--- a/lib/s390x/asm/cpacf.h
+++ b/lib/s390x/asm/cpacf.h
@@ -152,6 +152,12 @@ static __always_inline void __cpacf_query(unsigned int opcode, cpacf_mask_t *mas
 	register unsigned long r0 asm("0") = 0;	/* query function */
 	register unsigned long r1 asm("1") = (unsigned long) mask;
 
+/*
+ * gcc 12.0.1 warns if mask is < 4k.
+ * We use such addresses to test invalid or protected mask arguments.
+ */
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Warray-bounds"
 	asm volatile(
 		"	spm 0\n" /* pckmo doesn't change the cc */
 		/* Parameter regs are ignored, but must be nonzero and unique */
@@ -160,6 +166,7 @@ static __always_inline void __cpacf_query(unsigned int opcode, cpacf_mask_t *mas
 		: "=m" (*mask)
 		: [fc] "d" (r0), [pba] "a" (r1), [opc] "i" (opcode)
 		: "cc");
+#pragma GCC diagnostic pop
 }
 
 static inline int __cpacf_check_opcode(unsigned int opcode)
diff --git a/s390x/skey.c b/s390x/skey.c
index 32bf1070..7aa91d19 100644
--- a/s390x/skey.c
+++ b/s390x/skey.c
@@ -242,12 +242,19 @@ static void test_store_cpu_address(void)
  */
 static void set_prefix_key_1(uint32_t *prefix_ptr)
 {
+/*
+ * gcc 12.0.1 warns if prefix_ptr is < 4k.
+ * We need such addresses to test fetch protection override.
+ */
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Warray-bounds"
 	asm volatile (
 		"spka	0x10\n\t"
 		"spx	%0\n\t"
 		"spka	0\n"
 	     :: "Q" (*prefix_ptr)
 	);
+#pragma GCC diagnostic pop
 }
 
 /*

base-commit: c315f52b88b967cfb4cd58f3b4e1987378c47f3b
-- 
2.33.1

Re: [kvm-unit-tests PATCH] s390x: Ignore gcc 12 warnings for low addresses

[Claudio Imbrenda]

On Mon, 16 May 2022 16:43:32 +0200
Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:

> gcc 12 warns if a memory operand to inline asm points to memory in the
> first 4k bytes. However, in our case, these operands are fine, either
> because we actually want to use that memory, or expect and handle the
> resulting exception.
> Therefore, silence the warning.

I really dislike this

> 
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
> 
> Alternatives:
>  * Use memory clobber instead of memory output
>    Use address in register input instead of memory input
>        (may require WRITE_ONCE)

this sounds better. would you also get rid of the asm("0") annotations
on the variables? I really dislike those too

>  * Disable the warning globally
>  * Don't use gcc 12.0, with newer versions --param=min-pagesize=0 might
>    avoid the problem
> 
>  lib/s390x/asm/cpacf.h | 7 +++++++
>  s390x/skey.c          | 7 +++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/lib/s390x/asm/cpacf.h b/lib/s390x/asm/cpacf.h
> index 685262b0..02e603c8 100644
> --- a/lib/s390x/asm/cpacf.h
> +++ b/lib/s390x/asm/cpacf.h
> @@ -152,6 +152,12 @@ static __always_inline void __cpacf_query(unsigned int opcode, cpacf_mask_t *mas
>  	register unsigned long r0 asm("0") = 0;	/* query function */
>  	register unsigned long r1 asm("1") = (unsigned long) mask;
>  
> +/*
> + * gcc 12.0.1 warns if mask is < 4k.
> + * We use such addresses to test invalid or protected mask arguments.
> + */
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Warray-bounds"
>  	asm volatile(
>  		"	spm 0\n" /* pckmo doesn't change the cc */
>  		/* Parameter regs are ignored, but must be nonzero and unique */
> @@ -160,6 +166,7 @@ static __always_inline void __cpacf_query(unsigned int opcode, cpacf_mask_t *mas
>  		: "=m" (*mask)
>  		: [fc] "d" (r0), [pba] "a" (r1), [opc] "i" (opcode)
>  		: "cc");
> +#pragma GCC diagnostic pop
>  }
>  
>  static inline int __cpacf_check_opcode(unsigned int opcode)
> diff --git a/s390x/skey.c b/s390x/skey.c
> index 32bf1070..7aa91d19 100644
> --- a/s390x/skey.c
> +++ b/s390x/skey.c
> @@ -242,12 +242,19 @@ static void test_store_cpu_address(void)
>   */
>  static void set_prefix_key_1(uint32_t *prefix_ptr)
>  {
> +/*
> + * gcc 12.0.1 warns if prefix_ptr is < 4k.
> + * We need such addresses to test fetch protection override.
> + */
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Warray-bounds"
>  	asm volatile (
>  		"spka	0x10\n\t"
>  		"spx	%0\n\t"
>  		"spka	0\n"
>  	     :: "Q" (*prefix_ptr)
>  	);
> +#pragma GCC diagnostic pop
>  }
>  
>  /*
> 
> base-commit: c315f52b88b967cfb4cd58f3b4e1987378c47f3b

Re: [kvm-unit-tests PATCH] s390x: Ignore gcc 12 warnings for low addresses

[Thomas Huth]

On 17/05/2022 14.02, Claudio Imbrenda wrote:
> On Mon, 16 May 2022 16:43:32 +0200
> Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
> 
>> gcc 12 warns if a memory operand to inline asm points to memory in the
>> first 4k bytes. However, in our case, these operands are fine, either
>> because we actually want to use that memory, or expect and handle the
>> resulting exception.
>> Therefore, silence the warning.
> 
> I really dislike this

I agree the pragmas are ugly. But maybe we should mimic what the kernel
is doing here?

$ git show 8b202ee218395
commit 8b202ee218395319aec1ef44f72043e1fbaccdd6
Author: Sven Schnelle <svens@linux.ibm.com>
Date:   Mon Apr 25 14:17:42 2022 +0200

     s390: disable -Warray-bounds
     
     gcc-12 shows a lot of array bound warnings on s390. This is caused
     by the S390_lowcore macro which uses a hardcoded address of 0.
     
     Wrapping that with absolute_pointer() works, but gcc no longer knows
     that a 12 bit displacement is sufficient to access lowcore. So it
     emits instructions like 'lghi %r1,0; l %rx,xxx(%r1)' instead of a
     single load/store instruction. As s390 stores variables often
     read/written in lowcore, this is considered problematic. Therefore
     disable -Warray-bounds on s390 for gcc-12 for the time being, until
     there is a better solution.

... so we should maybe disable it in the Makefile, too, until the
kernel folks found a nicer solution?

  Thomas

Re: [kvm-unit-tests PATCH] s390x: Ignore gcc 12 warnings for low addresses

[Claudio Imbrenda]

On Tue, 17 May 2022 18:09:54 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 17/05/2022 14.02, Claudio Imbrenda wrote:
> > On Mon, 16 May 2022 16:43:32 +0200
> > Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
> >   
> >> gcc 12 warns if a memory operand to inline asm points to memory in the
> >> first 4k bytes. However, in our case, these operands are fine, either
> >> because we actually want to use that memory, or expect and handle the
> >> resulting exception.
> >> Therefore, silence the warning.  
> > 
> > I really dislike this  
> 
> I agree the pragmas are ugly. But maybe we should mimic what the kernel
> is doing here?
> 
> $ git show 8b202ee218395
> commit 8b202ee218395319aec1ef44f72043e1fbaccdd6
> Author: Sven Schnelle <svens@linux.ibm.com>
> Date:   Mon Apr 25 14:17:42 2022 +0200
> 
>      s390: disable -Warray-bounds
>      
>      gcc-12 shows a lot of array bound warnings on s390. This is caused
>      by the S390_lowcore macro which uses a hardcoded address of 0.
>      
>      Wrapping that with absolute_pointer() works, but gcc no longer knows
>      that a 12 bit displacement is sufficient to access lowcore. So it
>      emits instructions like 'lghi %r1,0; l %rx,xxx(%r1)' instead of a
>      single load/store instruction. As s390 stores variables often
>      read/written in lowcore, this is considered problematic. Therefore
>      disable -Warray-bounds on s390 for gcc-12 for the time being, until
>      there is a better solution.
> 
> ... so we should maybe disable it in the Makefile, too, until the
> kernel folks found a nicer solution?

it's a bit extreme, but if it's good enough for the kernel, it's good
enough for us

> 
>   Thomas
> 

Re: [kvm-unit-tests PATCH] s390x: Ignore gcc 12 warnings for low addresses

[Janis Schoetterl-Glausch]

On 5/17/22 18:09, Thomas Huth wrote:
> On 17/05/2022 14.02, Claudio Imbrenda wrote:
>> On Mon, 16 May 2022 16:43:32 +0200
>> Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
>>
>>> gcc 12 warns if a memory operand to inline asm points to memory in the
>>> first 4k bytes. However, in our case, these operands are fine, either
>>> because we actually want to use that memory, or expect and handle the
>>> resulting exception.
>>> Therefore, silence the warning.
>>
>> I really dislike this
> 
> I agree the pragmas are ugly. But maybe we should mimic what the kernel
> is doing here?
> 
> $ git show 8b202ee218395
> commit 8b202ee218395319aec1ef44f72043e1fbaccdd6
> Author: Sven Schnelle <svens@linux.ibm.com>
> Date:   Mon Apr 25 14:17:42 2022 +0200
> 
>     s390: disable -Warray-bounds
>         gcc-12 shows a lot of array bound warnings on s390. This is caused
>     by the S390_lowcore macro which uses a hardcoded address of 0.
>         Wrapping that with absolute_pointer() works, but gcc no longer knows
>     that a 12 bit displacement is sufficient to access lowcore. So it
>     emits instructions like 'lghi %r1,0; l %rx,xxx(%r1)' instead of a
>     single load/store instruction. As s390 stores variables often
>     read/written in lowcore, this is considered problematic. Therefore
>     disable -Warray-bounds on s390 for gcc-12 for the time being, until
>     there is a better solution.
> 
> ... so we should maybe disable it in the Makefile, too, until the
> kernel folks found a nicer solution?
> 
>  Thomas
> 

Neat, wasn't aware of that commit.

I don't think we need to concern ourselves with performance in this case and can define

+#define HIDE_PTR(ptr)                          \
+({                                             \
+       uint64_t __ptr;                         \
+       asm ("" : "=d" (__ptr) : "0" (ptr));    \
+       (typeof(ptr))__ptr;                     \
+})
+

in some header (which?).

Another alternative would be to define some extern symbols for the addresses we want to use.
It might be nice to have a symbol for the lowcore anyway, then we can get rid of

static struct lowcore *lc;
struct lowcore *lc = (struct lowcore *)0x0;
...

in a bunch of tests.

And use that symbol to derive the addresses we want to use.
emulator.c uses -1 to generate an addressing exception, we either need another symbol for
that or use another invalid address. (Can't get to -1 from lowcore/0 because the max array
size is signed int64 max)

Re: [kvm-unit-tests PATCH] s390x: Ignore gcc 12 warnings for low addresses

[Thomas Huth]

On 18/05/2022 13.07, Janis Schoetterl-Glausch wrote:
> On 5/17/22 18:09, Thomas Huth wrote:
>> On 17/05/2022 14.02, Claudio Imbrenda wrote:
>>> On Mon, 16 May 2022 16:43:32 +0200
>>> Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
>>>
>>>> gcc 12 warns if a memory operand to inline asm points to memory in the
>>>> first 4k bytes. However, in our case, these operands are fine, either
>>>> because we actually want to use that memory, or expect and handle the
>>>> resulting exception.
>>>> Therefore, silence the warning.
>>>
>>> I really dislike this
>>
>> I agree the pragmas are ugly. But maybe we should mimic what the kernel
>> is doing here?
>>
>> $ git show 8b202ee218395
>> commit 8b202ee218395319aec1ef44f72043e1fbaccdd6
>> Author: Sven Schnelle <svens@linux.ibm.com>
>> Date:   Mon Apr 25 14:17:42 2022 +0200
>>
>>      s390: disable -Warray-bounds
>>          gcc-12 shows a lot of array bound warnings on s390. This is caused
>>      by the S390_lowcore macro which uses a hardcoded address of 0.
>>          Wrapping that with absolute_pointer() works, but gcc no longer knows
>>      that a 12 bit displacement is sufficient to access lowcore. So it
>>      emits instructions like 'lghi %r1,0; l %rx,xxx(%r1)' instead of a
>>      single load/store instruction. As s390 stores variables often
>>      read/written in lowcore, this is considered problematic. Therefore
>>      disable -Warray-bounds on s390 for gcc-12 for the time being, until
>>      there is a better solution.
>>
>> ... so we should maybe disable it in the Makefile, too, until the
>> kernel folks found a nicer solution?
>>
>>   Thomas
>>
> 
> Neat, wasn't aware of that commit.
> 
> I don't think we need to concern ourselves with performance in this case and can define
> 
> +#define HIDE_PTR(ptr)                          \
> +({                                             \
> +       uint64_t __ptr;                         \
> +       asm ("" : "=d" (__ptr) : "0" (ptr));    \
> +       (typeof(ptr))__ptr;                     \
> +})
> +
> 
> in some header (which?).
> 
> Another alternative would be to define some extern symbols for the addresses we want to use.
> It might be nice to have a symbol for the lowcore anyway, then we can get rid of
> 
> static struct lowcore *lc;
> struct lowcore *lc = (struct lowcore *)0x0;
> ...
> 
> in a bunch of tests.

I like that idea.

> And use that symbol to derive the addresses we want to use.
> emulator.c uses -1 to generate an addressing exception, we either need another symbol for
> that or use another invalid address. (Can't get to -1 from lowcore/0 because the max array
> size is signed int64 max)

Maybe use INT64_MAX or something similar? Would that work?

  Thomas

Re: [kvm-unit-tests PATCH] s390x: Ignore gcc 12 warnings for low addresses

[Janis Schoetterl-Glausch]

On 5/19/22 11:46, Thomas Huth wrote:
> On 18/05/2022 13.07, Janis Schoetterl-Glausch wrote:
>> On 5/17/22 18:09, Thomas Huth wrote:
>>> On 17/05/2022 14.02, Claudio Imbrenda wrote:
>>>> On Mon, 16 May 2022 16:43:32 +0200
>>>> Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote:
>>>>
>>>>> gcc 12 warns if a memory operand to inline asm points to memory in the
>>>>> first 4k bytes. However, in our case, these operands are fine, either
>>>>> because we actually want to use that memory, or expect and handle the
>>>>> resulting exception.
>>>>> Therefore, silence the warning.
>>>>
>>>> I really dislike this
>>>
>>> I agree the pragmas are ugly. But maybe we should mimic what the kernel
>>> is doing here?
>>>
>>> $ git show 8b202ee218395
>>> commit 8b202ee218395319aec1ef44f72043e1fbaccdd6
>>> Author: Sven Schnelle <svens@linux.ibm.com>
>>> Date:   Mon Apr 25 14:17:42 2022 +0200
>>>
>>>      s390: disable -Warray-bounds
>>>          gcc-12 shows a lot of array bound warnings on s390. This is caused
>>>      by the S390_lowcore macro which uses a hardcoded address of 0.
>>>          Wrapping that with absolute_pointer() works, but gcc no longer knows
>>>      that a 12 bit displacement is sufficient to access lowcore. So it
>>>      emits instructions like 'lghi %r1,0; l %rx,xxx(%r1)' instead of a
>>>      single load/store instruction. As s390 stores variables often
>>>      read/written in lowcore, this is considered problematic. Therefore
>>>      disable -Warray-bounds on s390 for gcc-12 for the time being, until
>>>      there is a better solution.
>>>
>>> ... so we should maybe disable it in the Makefile, too, until the
>>> kernel folks found a nicer solution?
>>>
>>>   Thomas
>>>
>>
>> Neat, wasn't aware of that commit.
>>
>> I don't think we need to concern ourselves with performance in this case and can define
>>
>> +#define HIDE_PTR(ptr)                          \
>> +({                                             \
>> +       uint64_t __ptr;                         \
>> +       asm ("" : "=d" (__ptr) : "0" (ptr));    \
>> +       (typeof(ptr))__ptr;                     \
>> +})
>> +
>>
>> in some header (which?).
>>
>> Another alternative would be to define some extern symbols for the addresses we want to use.
>> It might be nice to have a symbol for the lowcore anyway, then we can get rid of
>>
>> static struct lowcore *lc;
>> struct lowcore *lc = (struct lowcore *)0x0;
>> ...
>>
>> in a bunch of tests.
> 
> I like that idea.
> 
>> And use that symbol to derive the addresses we want to use.
>> emulator.c uses -1 to generate an addressing exception, we either need another symbol for
>> that or use another invalid address. (Can't get to -1 from lowcore/0 because the max array
>> size is signed int64 max)
> 
> Maybe use INT64_MAX or something similar? Would that work?

I did it slightly different than in my prototype --- used a pointer instead of an array,
doesn't matter then.
> 
>  Thomas
>