qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] pc-bios/s390-ccw: Allow building with Clang, too
@ 2021-05-02 17:48 Thomas Huth
  2021-05-02 17:48 ` [PATCH 1/4] pc-bios/s390-ccw: Silence warning from Clang by marking panic() as noreturn Thomas Huth
                   ` (3 more replies)
  0 siblings, 4 replies; 26+ messages in thread
From: Thomas Huth @ 2021-05-02 17:48 UTC (permalink / raw)
  To: qemu-s390x, Christian Borntraeger; +Cc: Cornelia Huck, qemu-devel

Clang can provide some additional warnings compared to GCC which can
sometimes help to catch some more bugs. So it would be good to be
able to build the s390-ccw bios with Clang, too. Only caveat: Clang
does not support the z900 anymore which is the lowest guest CPU that
could be used in QEMU, so when compiling the firmware with Clang,
it is only possible to use guest CPUs >= z10. Since the additional
compiler test coverage is worth the effort, allow it anyway and just
emit a warning in the configure step.

Philippe Mathieu-Daudé (1):
  pc-bios/s390-ccw: Silence GCC 11 stringop-overflow warning

Thomas Huth (3):
  pc-bios/s390-ccw: Silence warning from Clang by marking panic() as
    noreturn
  pc-bios/s390-ccw: Fix the cc-option macro in the Makefile
  pc-bios/s390-ccw: Allow building with Clang, too

 configure                   | 9 ++++++++-
 pc-bios/s390-ccw/Makefile   | 8 +++++---
 pc-bios/s390-ccw/s390-ccw.h | 1 +
 3 files changed, 14 insertions(+), 4 deletions(-)

-- 
2.27.0



^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/4] pc-bios/s390-ccw: Silence warning from Clang by marking panic() as noreturn
  2021-05-02 17:48 [PATCH 0/4] pc-bios/s390-ccw: Allow building with Clang, too Thomas Huth
@ 2021-05-02 17:48 ` Thomas Huth
  2021-05-02 18:57   ` Philippe Mathieu-Daudé
  2021-05-03  8:54   ` Cornelia Huck
  2021-05-02 17:48 ` [PATCH 2/4] pc-bios/s390-ccw: Fix the cc-option macro in the Makefile Thomas Huth
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 26+ messages in thread
From: Thomas Huth @ 2021-05-02 17:48 UTC (permalink / raw)
  To: qemu-s390x, Christian Borntraeger; +Cc: Cornelia Huck, qemu-devel

When compiling the s390-ccw bios with Clang, the compiler emits a warning:

 pc-bios/s390-ccw/main.c:210:5: warning: variable 'found' is used uninitialized
  whenever switch default is taken [-Wsometimes-uninitialized]
     default:
     ^~~~~~~
 pc-bios/s390-ccw/main.c:214:16: note: uninitialized use occurs here
     IPL_assert(found, "Boot device not found\n");
                ^~~~~

It's a false positive, it only happens because Clang is not smart enough
to see that the panic() function in the "default:" case can never return.

Anyway, let's explicitely mark panic() with "noreturn" to shut up the
warning.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/s390-ccw.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index 6cd92669e9..79db69ff54 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -89,6 +89,7 @@ bool menu_is_enabled_enum(void);
 
 #define MAX_BOOT_ENTRIES  31
 
+__attribute__ ((__noreturn__))
 static inline void panic(const char *string)
 {
     sclp_print(string);
-- 
2.27.0



^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 2/4] pc-bios/s390-ccw: Fix the cc-option macro in the Makefile
  2021-05-02 17:48 [PATCH 0/4] pc-bios/s390-ccw: Allow building with Clang, too Thomas Huth
  2021-05-02 17:48 ` [PATCH 1/4] pc-bios/s390-ccw: Silence warning from Clang by marking panic() as noreturn Thomas Huth
@ 2021-05-02 17:48 ` Thomas Huth
  2021-05-02 17:48 ` [PATCH 3/4] pc-bios/s390-ccw: Silence GCC 11 stringop-overflow warning Thomas Huth
  2021-05-02 17:48 ` [PATCH 4/4] pc-bios/s390-ccw: Allow building with Clang, too Thomas Huth
  3 siblings, 0 replies; 26+ messages in thread
From: Thomas Huth @ 2021-05-02 17:48 UTC (permalink / raw)
  To: qemu-s390x, Christian Borntraeger; +Cc: Cornelia Huck, qemu-devel

The cc-option macro is not doing what it should - compared with the
original from the rules.mak file that got removed with commit
660f793093 ("Makefile: inline the relevant parts of rules.mak"),
the arguments got changed and thus the macro is rather doubling
the QEMU_CFLAGS than adding the flag that should be tested.

Fixes: 22fb2ab096 ("pc-bios/s390-ccw: do not use rules.mak")
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/pc-bios/s390-ccw/Makefile b/pc-bios/s390-ccw/Makefile
index 29fd9019b8..f0fe84c9eb 100644
--- a/pc-bios/s390-ccw/Makefile
+++ b/pc-bios/s390-ccw/Makefile
@@ -6,8 +6,8 @@ include ../../config-host.mak
 CFLAGS = -O2 -g
 
 quiet-command = $(if $(V),$1,$(if $(2),@printf "  %-7s %s\n" $2 $3 && $1, @$1))
-cc-option = $(if $(shell $(CC) $1 -S -o /dev/null -xc /dev/null > /dev/null \
-	      2>&1 && echo OK), $1, $2)
+cc-option = $(if $(shell $(CC) $1 $2 -S -o /dev/null -xc /dev/null \
+			 >/dev/null 2>&1 && echo OK),$2,$3)
 
 VPATH_SUFFIXES = %.c %.h %.S %.m %.mak %.sh %.rc Kconfig% %.json.in
 set-vpath = $(if $1,$(foreach PATTERN,$(VPATH_SUFFIXES),$(eval vpath $(PATTERN) $1)))
-- 
2.27.0



^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 3/4] pc-bios/s390-ccw: Silence GCC 11 stringop-overflow warning
  2021-05-02 17:48 [PATCH 0/4] pc-bios/s390-ccw: Allow building with Clang, too Thomas Huth
  2021-05-02 17:48 ` [PATCH 1/4] pc-bios/s390-ccw: Silence warning from Clang by marking panic() as noreturn Thomas Huth
  2021-05-02 17:48 ` [PATCH 2/4] pc-bios/s390-ccw: Fix the cc-option macro in the Makefile Thomas Huth
@ 2021-05-02 17:48 ` Thomas Huth
  2021-05-03  9:00   ` Cornelia Huck
  2021-05-02 17:48 ` [PATCH 4/4] pc-bios/s390-ccw: Allow building with Clang, too Thomas Huth
  3 siblings, 1 reply; 26+ messages in thread
From: Thomas Huth @ 2021-05-02 17:48 UTC (permalink / raw)
  To: qemu-s390x, Christian Borntraeger; +Cc: Cornelia Huck, qemu-devel

From: Philippe Mathieu-Daudé <philmd@redhat.com>

When building on Fedora 34 (gcc version 11.0.0 20210210) we get:

  In file included from pc-bios/s390-ccw/main.c:11:
  In function ‘memset’,
      inlined from ‘boot_setup’ at pc-bios/s390-ccw/main.c:185:5,
      inlined from ‘main’ at pc-bios/s390-ccw/main.c:288:5:
  pc-bios/s390-ccw/libc.h:28:14: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
     28 |         p[i] = c;
        |         ~~~~~^~~

The offending code is:

  memset((char *)S390EP, 0, 6);

where S390EP is a const address:

  #define S390EP 0x10008

The compiler doesn't now how big that pointed area is, so assume its
length is zero. This has been reported as BZ#99578 to GCC:
"gcc-11 -Warray-bounds or -Wstringop-overread warning when accessing a
pointer from integer literal"
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578

As this warning does us more harm than good in the BIOS code (where
lot of direct accesses to low memory are done), silence this warning
for all BIOS objects.

Suggested-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20210422145911.2513980-1-philmd@redhat.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
[thuth: Use the pre-existing cc-option macro instead of adding a new one]
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/pc-bios/s390-ccw/Makefile b/pc-bios/s390-ccw/Makefile
index f0fe84c9eb..83fb1afb73 100644
--- a/pc-bios/s390-ccw/Makefile
+++ b/pc-bios/s390-ccw/Makefile
@@ -30,6 +30,7 @@ OBJECTS = start.o main.o bootmap.o jump2ipl.o sclp.o menu.o \
 	  virtio.o virtio-scsi.o virtio-blkdev.o libc.o cio.o dasd-ipl.o
 
 QEMU_CFLAGS := -Wall $(filter -W%, $(QEMU_CFLAGS))
+QEMU_CFLAGS += $(call cc-option,-Werror $(QEMU_CFLAGS),-Wno-stringop-overflow)
 QEMU_CFLAGS += -ffreestanding -fno-delete-null-pointer-checks -fno-common -fPIE
 QEMU_CFLAGS += -fwrapv -fno-strict-aliasing -fno-asynchronous-unwind-tables
 QEMU_CFLAGS += $(call cc-option, $(QEMU_CFLAGS), -fno-stack-protector)
-- 
2.27.0



^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 4/4] pc-bios/s390-ccw: Allow building with Clang, too
  2021-05-02 17:48 [PATCH 0/4] pc-bios/s390-ccw: Allow building with Clang, too Thomas Huth
                   ` (2 preceding siblings ...)
  2021-05-02 17:48 ` [PATCH 3/4] pc-bios/s390-ccw: Silence GCC 11 stringop-overflow warning Thomas Huth
@ 2021-05-02 17:48 ` Thomas Huth
  2021-05-02 19:00   ` Philippe Mathieu-Daudé
                     ` (2 more replies)
  3 siblings, 3 replies; 26+ messages in thread
From: Thomas Huth @ 2021-05-02 17:48 UTC (permalink / raw)
  To: qemu-s390x, Christian Borntraeger; +Cc: Cornelia Huck, qemu-devel

Clang unfortunately does not support generating code for the z900
architecture level and starts with the z10 instead. Thus to be able
to support compiling with Clang, we have to check for the supported
compiler flags. The disadvantage is of course that the bios image
will only run with z10 guest CPUs upwards (which is what most people
use anyway), so just in case let's also emit a warning in that case.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 configure                 | 9 ++++++++-
 pc-bios/s390-ccw/Makefile | 3 ++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index 4f374b4889..5ebc937746 100755
--- a/configure
+++ b/configure
@@ -5417,9 +5417,16 @@ if { test "$cpu" = "i386" || test "$cpu" = "x86_64"; } && \
 fi
 
 # Only build s390-ccw bios if we're on s390x and the compiler has -march=z900
+# or -march=z10 (which is the lowest architecture level that Clang supports)
 if test "$cpu" = "s390x" ; then
   write_c_skeleton
-  if compile_prog "-march=z900" ""; then
+  compile_prog "-march=z900" ""
+  has_z900=$?
+  if [ $has_z900 = 0 ] || compile_prog "-march=z10" ""; then
+    if [ $has_z900 != 0 ]; then
+      echo "WARNING: Your compiler does not support the z900!"
+      echo "         The s390-ccw bios will only work with guest CPUs >= z10."
+    fi
     roms="$roms s390-ccw"
     # SLOF is required for building the s390-ccw firmware on s390x,
     # since it is using the libnet code from SLOF for network booting.
diff --git a/pc-bios/s390-ccw/Makefile b/pc-bios/s390-ccw/Makefile
index 83fb1afb73..cee9d2c63b 100644
--- a/pc-bios/s390-ccw/Makefile
+++ b/pc-bios/s390-ccw/Makefile
@@ -34,7 +34,8 @@ QEMU_CFLAGS += $(call cc-option,-Werror $(QEMU_CFLAGS),-Wno-stringop-overflow)
 QEMU_CFLAGS += -ffreestanding -fno-delete-null-pointer-checks -fno-common -fPIE
 QEMU_CFLAGS += -fwrapv -fno-strict-aliasing -fno-asynchronous-unwind-tables
 QEMU_CFLAGS += $(call cc-option, $(QEMU_CFLAGS), -fno-stack-protector)
-QEMU_CFLAGS += -msoft-float -march=z900
+QEMU_CFLAGS += -msoft-float
+QEMU_CFLAGS += $(call cc-option, $(QEMU_CFLAGS),-march=z900,-march=z10)
 QEMU_CFLAGS += -std=gnu99
 LDFLAGS += -Wl,-pie -nostdlib
 
-- 
2.27.0



^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] pc-bios/s390-ccw: Silence warning from Clang by marking panic() as noreturn
  2021-05-02 17:48 ` [PATCH 1/4] pc-bios/s390-ccw: Silence warning from Clang by marking panic() as noreturn Thomas Huth
@ 2021-05-02 18:57   ` Philippe Mathieu-Daudé
  2021-05-03  4:53     ` Thomas Huth
  2021-05-03  4:56     ` Markus Armbruster
  2021-05-03  8:54   ` Cornelia Huck
  1 sibling, 2 replies; 26+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-02 18:57 UTC (permalink / raw)
  To: Thomas Huth, qemu-s390x, Christian Borntraeger; +Cc: Cornelia Huck, qemu-devel

On 5/2/21 7:48 PM, Thomas Huth wrote:
> When compiling the s390-ccw bios with Clang, the compiler emits a warning:
> 
>  pc-bios/s390-ccw/main.c:210:5: warning: variable 'found' is used uninitialized
>   whenever switch default is taken [-Wsometimes-uninitialized]
>      default:
>      ^~~~~~~
>  pc-bios/s390-ccw/main.c:214:16: note: uninitialized use occurs here
>      IPL_assert(found, "Boot device not found\n");
>                 ^~~~~
> 
> It's a false positive, it only happens because Clang is not smart enough
> to see that the panic() function in the "default:" case can never return.
> 
> Anyway, let's explicitely mark panic() with "noreturn" to shut up the
> warning.

Why not simply initialize the variable instead?

> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  pc-bios/s390-ccw/s390-ccw.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
> index 6cd92669e9..79db69ff54 100644
> --- a/pc-bios/s390-ccw/s390-ccw.h
> +++ b/pc-bios/s390-ccw/s390-ccw.h
> @@ -89,6 +89,7 @@ bool menu_is_enabled_enum(void);
>  
>  #define MAX_BOOT_ENTRIES  31
>  
> +__attribute__ ((__noreturn__))
>  static inline void panic(const char *string)
>  {
>      sclp_print(string);
> 



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/4] pc-bios/s390-ccw: Allow building with Clang, too
  2021-05-02 17:48 ` [PATCH 4/4] pc-bios/s390-ccw: Allow building with Clang, too Thomas Huth
@ 2021-05-02 19:00   ` Philippe Mathieu-Daudé
  2021-05-03  4:58   ` Markus Armbruster
  2021-05-03 10:00   ` Cornelia Huck
  2 siblings, 0 replies; 26+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-02 19:00 UTC (permalink / raw)
  To: Thomas Huth, qemu-s390x, Christian Borntraeger; +Cc: Cornelia Huck, qemu-devel

On 5/2/21 7:48 PM, Thomas Huth wrote:
> Clang unfortunately does not support generating code for the z900
> architecture level and starts with the z10 instead. Thus to be able
> to support compiling with Clang, we have to check for the supported
> compiler flags. The disadvantage is of course that the bios image
> will only run with z10 guest CPUs upwards (which is what most people
> use anyway), so just in case let's also emit a warning in that case.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  configure                 | 9 ++++++++-
>  pc-bios/s390-ccw/Makefile | 3 ++-
>  2 files changed, 10 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] pc-bios/s390-ccw: Silence warning from Clang by marking panic() as noreturn
  2021-05-02 18:57   ` Philippe Mathieu-Daudé
@ 2021-05-03  4:53     ` Thomas Huth
  2021-05-03  7:40       ` Philippe Mathieu-Daudé
  2021-05-03  4:56     ` Markus Armbruster
  1 sibling, 1 reply; 26+ messages in thread
From: Thomas Huth @ 2021-05-03  4:53 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-s390x, Christian Borntraeger
  Cc: Cornelia Huck, qemu-devel

On 02/05/2021 20.57, Philippe Mathieu-Daudé wrote:
> On 5/2/21 7:48 PM, Thomas Huth wrote:
>> When compiling the s390-ccw bios with Clang, the compiler emits a warning:
>>
>>   pc-bios/s390-ccw/main.c:210:5: warning: variable 'found' is used uninitialized
>>    whenever switch default is taken [-Wsometimes-uninitialized]
>>       default:
>>       ^~~~~~~
>>   pc-bios/s390-ccw/main.c:214:16: note: uninitialized use occurs here
>>       IPL_assert(found, "Boot device not found\n");
>>                  ^~~~~
>>
>> It's a false positive, it only happens because Clang is not smart enough
>> to see that the panic() function in the "default:" case can never return.
>>
>> Anyway, let's explicitely mark panic() with "noreturn" to shut up the
>> warning.
> 
> Why not simply initialize the variable instead?

First, it's a false positive. If you only look at the code, someone might 
later wonder whether it's really necessary or not and try to remove it again 
(and since there is no warning with gcc, this patch would also have a good 
chance to get merged, bringing us back to where we are right now).

Second, declaring panic() as noreturn is certainly more sustainable, since 
it might prevent similar situations in other parts of the code in the future.

  Thomas


>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   pc-bios/s390-ccw/s390-ccw.h | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
>> index 6cd92669e9..79db69ff54 100644
>> --- a/pc-bios/s390-ccw/s390-ccw.h
>> +++ b/pc-bios/s390-ccw/s390-ccw.h
>> @@ -89,6 +89,7 @@ bool menu_is_enabled_enum(void);
>>   
>>   #define MAX_BOOT_ENTRIES  31
>>   
>> +__attribute__ ((__noreturn__))
>>   static inline void panic(const char *string)
>>   {
>>       sclp_print(string);
>>
> 



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] pc-bios/s390-ccw: Silence warning from Clang by marking panic() as noreturn
  2021-05-02 18:57   ` Philippe Mathieu-Daudé
  2021-05-03  4:53     ` Thomas Huth
@ 2021-05-03  4:56     ` Markus Armbruster
  1 sibling, 0 replies; 26+ messages in thread
From: Markus Armbruster @ 2021-05-03  4:56 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Christian Borntraeger, Thomas Huth, Cornelia Huck, qemu-s390x,
	qemu-devel

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 5/2/21 7:48 PM, Thomas Huth wrote:
>> When compiling the s390-ccw bios with Clang, the compiler emits a warning:
>> 
>>  pc-bios/s390-ccw/main.c:210:5: warning: variable 'found' is used uninitialized
>>   whenever switch default is taken [-Wsometimes-uninitialized]
>>      default:
>>      ^~~~~~~
>>  pc-bios/s390-ccw/main.c:214:16: note: uninitialized use occurs here
>>      IPL_assert(found, "Boot device not found\n");
>>                 ^~~~~
>> 
>> It's a false positive, it only happens because Clang is not smart enough
>> to see that the panic() function in the "default:" case can never return.
>> 
>> Anyway, let's explicitely mark panic() with "noreturn" to shut up the
>> warning.
>
> Why not simply initialize the variable instead?

Because telling an optimizing compiler the truth is a good idea?



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/4] pc-bios/s390-ccw: Allow building with Clang, too
  2021-05-02 17:48 ` [PATCH 4/4] pc-bios/s390-ccw: Allow building with Clang, too Thomas Huth
  2021-05-02 19:00   ` Philippe Mathieu-Daudé
@ 2021-05-03  4:58   ` Markus Armbruster
  2021-05-03  5:17     ` Thomas Huth
  2021-05-03 10:00   ` Cornelia Huck
  2 siblings, 1 reply; 26+ messages in thread
From: Markus Armbruster @ 2021-05-03  4:58 UTC (permalink / raw)
  To: Thomas Huth; +Cc: Christian Borntraeger, qemu-s390x, Cornelia Huck, qemu-devel

Thomas Huth <thuth@redhat.com> writes:

> Clang unfortunately does not support generating code for the z900
> architecture level and starts with the z10 instead. Thus to be able
> to support compiling with Clang, we have to check for the supported
> compiler flags. The disadvantage is of course that the bios image
> will only run with z10 guest CPUs upwards (which is what most people
> use anyway), so just in case let's also emit a warning in that case.

What happens when you try to use this bios with an old CPU anyway?



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/4] pc-bios/s390-ccw: Allow building with Clang, too
  2021-05-03  4:58   ` Markus Armbruster
@ 2021-05-03  5:17     ` Thomas Huth
  2021-05-03  8:10       ` David Hildenbrand
  0 siblings, 1 reply; 26+ messages in thread
From: Thomas Huth @ 2021-05-03  5:17 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: David Hildenbrand, Cornelia Huck, Richard Henderson, qemu-devel,
	Christian Borntraeger, qemu-s390x

On 03/05/2021 06.58, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> Clang unfortunately does not support generating code for the z900
>> architecture level and starts with the z10 instead. Thus to be able
>> to support compiling with Clang, we have to check for the supported
>> compiler flags. The disadvantage is of course that the bios image
>> will only run with z10 guest CPUs upwards (which is what most people
>> use anyway), so just in case let's also emit a warning in that case.
> 
> What happens when you try to use this bios with an old CPU anyway?

Interesting question. I was expecting the guest to crash since it would be 
using a CPU instruction that is not supported on the old CPU model. But I 
just gave it a try, and there was no crash. The guest booted just fine. 
Either Clang only emits instructions that work with the old z900 anyway, or 
our emulation in QEMU is imprecise and we allow newer instructions to be 
executed on old models, too.

  Thomas



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] pc-bios/s390-ccw: Silence warning from Clang by marking panic() as noreturn
  2021-05-03  4:53     ` Thomas Huth
@ 2021-05-03  7:40       ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 26+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-03  7:40 UTC (permalink / raw)
  To: Thomas Huth, qemu-s390x, Christian Borntraeger; +Cc: Cornelia Huck, qemu-devel

On 5/3/21 6:53 AM, Thomas Huth wrote:
> On 02/05/2021 20.57, Philippe Mathieu-Daudé wrote:
>> On 5/2/21 7:48 PM, Thomas Huth wrote:
>>> When compiling the s390-ccw bios with Clang, the compiler emits a
>>> warning:
>>>
>>>   pc-bios/s390-ccw/main.c:210:5: warning: variable 'found' is used
>>> uninitialized
>>>    whenever switch default is taken [-Wsometimes-uninitialized]
>>>       default:
>>>       ^~~~~~~
>>>   pc-bios/s390-ccw/main.c:214:16: note: uninitialized use occurs here
>>>       IPL_assert(found, "Boot device not found\n");
>>>                  ^~~~~
>>>
>>> It's a false positive, it only happens because Clang is not smart enough
>>> to see that the panic() function in the "default:" case can never
>>> return.
>>>
>>> Anyway, let's explicitely mark panic() with "noreturn" to shut up the
>>> warning.
>>
>> Why not simply initialize the variable instead?
> 
> First, it's a false positive. If you only look at the code, someone
> might later wonder whether it's really necessary or not and try to
> remove it again (and since there is no warning with gcc, this patch
> would also have a good chance to get merged, bringing us back to where
> we are right now).
> 
> Second, declaring panic() as noreturn is certainly more sustainable,
> since it might prevent similar situations in other parts of the code in
> the future.

Fine then:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/4] pc-bios/s390-ccw: Allow building with Clang, too
  2021-05-03  5:17     ` Thomas Huth
@ 2021-05-03  8:10       ` David Hildenbrand
  2021-05-03  8:23         ` Markus Armbruster
  0 siblings, 1 reply; 26+ messages in thread
From: David Hildenbrand @ 2021-05-03  8:10 UTC (permalink / raw)
  To: Thomas Huth, Markus Armbruster
  Cc: Christian Borntraeger, qemu-s390x, Cornelia Huck,
	Richard Henderson, qemu-devel

On 03.05.21 07:17, Thomas Huth wrote:
> On 03/05/2021 06.58, Markus Armbruster wrote:
>> Thomas Huth <thuth@redhat.com> writes:
>>
>>> Clang unfortunately does not support generating code for the z900
>>> architecture level and starts with the z10 instead. Thus to be able
>>> to support compiling with Clang, we have to check for the supported
>>> compiler flags. The disadvantage is of course that the bios image
>>> will only run with z10 guest CPUs upwards (which is what most people
>>> use anyway), so just in case let's also emit a warning in that case.
>>
>> What happens when you try to use this bios with an old CPU anyway?
> 
> Interesting question. I was expecting the guest to crash since it would be
> using a CPU instruction that is not supported on the old CPU model. But I
> just gave it a try, and there was no crash. The guest booted just fine.
> Either Clang only emits instructions that work with the old z900 anyway, or
> our emulation in QEMU is imprecise and we allow newer instructions to be
> executed on old models, too.

Yes, that's currently still done. We once thought about disabling that 
(there was a patch from Richard), but decided against it because -- back 
then -- the default QEMU model was still very basic and would have 
essentially disabled all more recent instructions as default.

We can most probably do that change soon as we have a "fairly new" 
default QEMU CPU model. I can glue it to my z14 change.



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/4] pc-bios/s390-ccw: Allow building with Clang, too
  2021-05-03  8:10       ` David Hildenbrand
@ 2021-05-03  8:23         ` Markus Armbruster
  2021-05-03  9:14           ` Cornelia Huck
  0 siblings, 1 reply; 26+ messages in thread
From: Markus Armbruster @ 2021-05-03  8:23 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: Thomas Huth, Cornelia Huck, Richard Henderson, qemu-devel,
	Christian Borntraeger, qemu-s390x

David Hildenbrand <david@redhat.com> writes:

> On 03.05.21 07:17, Thomas Huth wrote:
>> On 03/05/2021 06.58, Markus Armbruster wrote:
>>> Thomas Huth <thuth@redhat.com> writes:
>>>
>>>> Clang unfortunately does not support generating code for the z900
>>>> architecture level and starts with the z10 instead. Thus to be able
>>>> to support compiling with Clang, we have to check for the supported
>>>> compiler flags. The disadvantage is of course that the bios image
>>>> will only run with z10 guest CPUs upwards (which is what most people
>>>> use anyway), so just in case let's also emit a warning in that case.
>>>
>>> What happens when you try to use this bios with an old CPU anyway?
>> 
>> Interesting question. I was expecting the guest to crash since it would be
>> using a CPU instruction that is not supported on the old CPU model. But I
>> just gave it a try, and there was no crash. The guest booted just fine.
>> Either Clang only emits instructions that work with the old z900 anyway, or
>> our emulation in QEMU is imprecise and we allow newer instructions to be
>> executed on old models, too.
>
> Yes, that's currently still done. We once thought about disabling that 
> (there was a patch from Richard), but decided against it because -- back 
> then -- the default QEMU model was still very basic and would have 
> essentially disabled all more recent instructions as default.
>
> We can most probably do that change soon as we have a "fairly new" 
> default QEMU CPU model. I can glue it to my z14 change.

In case this makes the BIOS crash with old CPUs: when a guest refuses to
start because the BIOS was compiled the wrong way for it, configure
having told you so back then is not a nice user experience.  Can we do
better, with reasonable effort?



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] pc-bios/s390-ccw: Silence warning from Clang by marking panic() as noreturn
  2021-05-02 17:48 ` [PATCH 1/4] pc-bios/s390-ccw: Silence warning from Clang by marking panic() as noreturn Thomas Huth
  2021-05-02 18:57   ` Philippe Mathieu-Daudé
@ 2021-05-03  8:54   ` Cornelia Huck
  1 sibling, 0 replies; 26+ messages in thread
From: Cornelia Huck @ 2021-05-03  8:54 UTC (permalink / raw)
  To: Thomas Huth; +Cc: Christian Borntraeger, qemu-s390x, qemu-devel

On Sun,  2 May 2021 19:48:33 +0200
Thomas Huth <thuth@redhat.com> wrote:

> When compiling the s390-ccw bios with Clang, the compiler emits a warning:
> 
>  pc-bios/s390-ccw/main.c:210:5: warning: variable 'found' is used uninitialized
>   whenever switch default is taken [-Wsometimes-uninitialized]
>      default:
>      ^~~~~~~
>  pc-bios/s390-ccw/main.c:214:16: note: uninitialized use occurs here
>      IPL_assert(found, "Boot device not found\n");
>                 ^~~~~
> 
> It's a false positive, it only happens because Clang is not smart enough
> to see that the panic() function in the "default:" case can never return.
> 
> Anyway, let's explicitely mark panic() with "noreturn" to shut up the
> warning.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  pc-bios/s390-ccw/s390-ccw.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
> index 6cd92669e9..79db69ff54 100644
> --- a/pc-bios/s390-ccw/s390-ccw.h
> +++ b/pc-bios/s390-ccw/s390-ccw.h
> @@ -89,6 +89,7 @@ bool menu_is_enabled_enum(void);
>  
>  #define MAX_BOOT_ENTRIES  31
>  
> +__attribute__ ((__noreturn__))
>  static inline void panic(const char *string)
>  {
>      sclp_print(string);

I'm surprised that the noreturn annotation of disabled_wait (called
right after the sclp_print()) is not enough. Anyway,

Reviewed-by: Cornelia Huck <cohuck@redhat.com>



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/4] pc-bios/s390-ccw: Silence GCC 11 stringop-overflow warning
  2021-05-02 17:48 ` [PATCH 3/4] pc-bios/s390-ccw: Silence GCC 11 stringop-overflow warning Thomas Huth
@ 2021-05-03  9:00   ` Cornelia Huck
  2021-05-03  9:30     ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 26+ messages in thread
From: Cornelia Huck @ 2021-05-03  9:00 UTC (permalink / raw)
  To: Thomas Huth; +Cc: Christian Borntraeger, qemu-s390x, qemu-devel

On Sun,  2 May 2021 19:48:35 +0200
Thomas Huth <thuth@redhat.com> wrote:

> From: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> When building on Fedora 34 (gcc version 11.0.0 20210210) we get:
> 
>   In file included from pc-bios/s390-ccw/main.c:11:
>   In function ‘memset’,
>       inlined from ‘boot_setup’ at pc-bios/s390-ccw/main.c:185:5,
>       inlined from ‘main’ at pc-bios/s390-ccw/main.c:288:5:
>   pc-bios/s390-ccw/libc.h:28:14: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
>      28 |         p[i] = c;
>         |         ~~~~~^~~
> 
> The offending code is:
> 
>   memset((char *)S390EP, 0, 6);
> 
> where S390EP is a const address:
> 
>   #define S390EP 0x10008
> 
> The compiler doesn't now how big that pointed area is, so assume its

s/now/know/
s/assume/it assumes that/

> length is zero. This has been reported as BZ#99578 to GCC:
> "gcc-11 -Warray-bounds or -Wstringop-overread warning when accessing a
> pointer from integer literal"
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
> 
> As this warning does us more harm than good in the BIOS code (where
> lot of direct accesses to low memory are done), silence this warning
> for all BIOS objects.
> 
> Suggested-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Message-Id: <20210422145911.2513980-1-philmd@redhat.com>
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> [thuth: Use the pre-existing cc-option macro instead of adding a new one]
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  pc-bios/s390-ccw/Makefile | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/pc-bios/s390-ccw/Makefile b/pc-bios/s390-ccw/Makefile
> index f0fe84c9eb..83fb1afb73 100644
> --- a/pc-bios/s390-ccw/Makefile
> +++ b/pc-bios/s390-ccw/Makefile
> @@ -30,6 +30,7 @@ OBJECTS = start.o main.o bootmap.o jump2ipl.o sclp.o menu.o \
>  	  virtio.o virtio-scsi.o virtio-blkdev.o libc.o cio.o dasd-ipl.o
>  
>  QEMU_CFLAGS := -Wall $(filter -W%, $(QEMU_CFLAGS))
> +QEMU_CFLAGS += $(call cc-option,-Werror $(QEMU_CFLAGS),-Wno-stringop-overflow)
>  QEMU_CFLAGS += -ffreestanding -fno-delete-null-pointer-checks -fno-common -fPIE
>  QEMU_CFLAGS += -fwrapv -fno-strict-aliasing -fno-asynchronous-unwind-tables
>  QEMU_CFLAGS += $(call cc-option, $(QEMU_CFLAGS), -fno-stack-protector)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/4] pc-bios/s390-ccw: Allow building with Clang, too
  2021-05-03  8:23         ` Markus Armbruster
@ 2021-05-03  9:14           ` Cornelia Huck
  2021-05-03  9:17             ` David Hildenbrand
  2021-05-03  9:31             ` Thomas Huth
  0 siblings, 2 replies; 26+ messages in thread
From: Cornelia Huck @ 2021-05-03  9:14 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Thomas Huth, David Hildenbrand, Richard Henderson, qemu-devel,
	Christian Borntraeger, qemu-s390x

On Mon, 03 May 2021 10:23:20 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> David Hildenbrand <david@redhat.com> writes:
> 
> > On 03.05.21 07:17, Thomas Huth wrote:  
> >> On 03/05/2021 06.58, Markus Armbruster wrote:  
> >>> Thomas Huth <thuth@redhat.com> writes:
> >>>  
> >>>> Clang unfortunately does not support generating code for the z900
> >>>> architecture level and starts with the z10 instead. Thus to be able
> >>>> to support compiling with Clang, we have to check for the supported
> >>>> compiler flags. The disadvantage is of course that the bios image
> >>>> will only run with z10 guest CPUs upwards (which is what most people
> >>>> use anyway), so just in case let's also emit a warning in that case.  
> >>>
> >>> What happens when you try to use this bios with an old CPU anyway?  
> >> 
> >> Interesting question. I was expecting the guest to crash since it would be
> >> using a CPU instruction that is not supported on the old CPU model. But I
> >> just gave it a try, and there was no crash. The guest booted just fine.
> >> Either Clang only emits instructions that work with the old z900 anyway, or
> >> our emulation in QEMU is imprecise and we allow newer instructions to be
> >> executed on old models, too.  
> >
> > Yes, that's currently still done. We once thought about disabling that 
> > (there was a patch from Richard), but decided against it because -- back 
> > then -- the default QEMU model was still very basic and would have 
> > essentially disabled all more recent instructions as default.
> >
> > We can most probably do that change soon as we have a "fairly new" 
> > default QEMU CPU model. I can glue it to my z14 change.  
> 
> In case this makes the BIOS crash with old CPUs: when a guest refuses to
> start because the BIOS was compiled the wrong way for it, configure
> having told you so back then is not a nice user experience.  Can we do
> better, with reasonable effort?

I fear the experience will be as bad as for any guest that is using
features from a newer cpu level (i.e. random crashes when the guest
actually tries to use that newer instruction.)

I see two options:
- Just try to start and hope that it works.
- Deprecate any cpu model older than z10.

Anyone have a better idea? I don't particularly like any of the two.



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/4] pc-bios/s390-ccw: Allow building with Clang, too
  2021-05-03  9:14           ` Cornelia Huck
@ 2021-05-03  9:17             ` David Hildenbrand
  2021-05-03  9:23               ` Cornelia Huck
  2021-05-03  9:31             ` Thomas Huth
  1 sibling, 1 reply; 26+ messages in thread
From: David Hildenbrand @ 2021-05-03  9:17 UTC (permalink / raw)
  To: Cornelia Huck, Markus Armbruster
  Cc: Christian Borntraeger, Thomas Huth, Richard Henderson,
	qemu-s390x, qemu-devel

On 03.05.21 11:14, Cornelia Huck wrote:
> On Mon, 03 May 2021 10:23:20 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
> 
>> David Hildenbrand <david@redhat.com> writes:
>>
>>> On 03.05.21 07:17, Thomas Huth wrote:
>>>> On 03/05/2021 06.58, Markus Armbruster wrote:
>>>>> Thomas Huth <thuth@redhat.com> writes:
>>>>>   
>>>>>> Clang unfortunately does not support generating code for the z900
>>>>>> architecture level and starts with the z10 instead. Thus to be able
>>>>>> to support compiling with Clang, we have to check for the supported
>>>>>> compiler flags. The disadvantage is of course that the bios image
>>>>>> will only run with z10 guest CPUs upwards (which is what most people
>>>>>> use anyway), so just in case let's also emit a warning in that case.
>>>>>
>>>>> What happens when you try to use this bios with an old CPU anyway?
>>>>
>>>> Interesting question. I was expecting the guest to crash since it would be
>>>> using a CPU instruction that is not supported on the old CPU model. But I
>>>> just gave it a try, and there was no crash. The guest booted just fine.
>>>> Either Clang only emits instructions that work with the old z900 anyway, or
>>>> our emulation in QEMU is imprecise and we allow newer instructions to be
>>>> executed on old models, too.
>>>
>>> Yes, that's currently still done. We once thought about disabling that
>>> (there was a patch from Richard), but decided against it because -- back
>>> then -- the default QEMU model was still very basic and would have
>>> essentially disabled all more recent instructions as default.
>>>
>>> We can most probably do that change soon as we have a "fairly new"
>>> default QEMU CPU model. I can glue it to my z14 change.
>>
>> In case this makes the BIOS crash with old CPUs: when a guest refuses to
>> start because the BIOS was compiled the wrong way for it, configure
>> having told you so back then is not a nice user experience.  Can we do
>> better, with reasonable effort?
> 
> I fear the experience will be as bad as for any guest that is using
> features from a newer cpu level (i.e. random crashes when the guest
> actually tries to use that newer instruction.)
> 
> I see two options:
> - Just try to start and hope that it works.
> - Deprecate any cpu model older than z10.
> 
> Anyone have a better idea? I don't particularly like any of the two.

As the default CPU model with new compat machines is >= z13, I wouldn't 
lose sleep about this. Even with a broken bios one can still boot an 
external kernel+initrd for testing purposes.

-- 
Thanks,

David / dhildenb



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/4] pc-bios/s390-ccw: Allow building with Clang, too
  2021-05-03  9:17             ` David Hildenbrand
@ 2021-05-03  9:23               ` Cornelia Huck
  0 siblings, 0 replies; 26+ messages in thread
From: Cornelia Huck @ 2021-05-03  9:23 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: Thomas Huth, Richard Henderson, Markus Armbruster, qemu-devel,
	Christian Borntraeger, qemu-s390x

On Mon, 3 May 2021 11:17:54 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 03.05.21 11:14, Cornelia Huck wrote:
> > On Mon, 03 May 2021 10:23:20 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >   
> >> David Hildenbrand <david@redhat.com> writes:
> >>  
> >>> On 03.05.21 07:17, Thomas Huth wrote:  
> >>>> On 03/05/2021 06.58, Markus Armbruster wrote:  
> >>>>> Thomas Huth <thuth@redhat.com> writes:
> >>>>>     
> >>>>>> Clang unfortunately does not support generating code for the z900
> >>>>>> architecture level and starts with the z10 instead. Thus to be able
> >>>>>> to support compiling with Clang, we have to check for the supported
> >>>>>> compiler flags. The disadvantage is of course that the bios image
> >>>>>> will only run with z10 guest CPUs upwards (which is what most people
> >>>>>> use anyway), so just in case let's also emit a warning in that case.  
> >>>>>
> >>>>> What happens when you try to use this bios with an old CPU anyway?  
> >>>>
> >>>> Interesting question. I was expecting the guest to crash since it would be
> >>>> using a CPU instruction that is not supported on the old CPU model. But I
> >>>> just gave it a try, and there was no crash. The guest booted just fine.
> >>>> Either Clang only emits instructions that work with the old z900 anyway, or
> >>>> our emulation in QEMU is imprecise and we allow newer instructions to be
> >>>> executed on old models, too.  
> >>>
> >>> Yes, that's currently still done. We once thought about disabling that
> >>> (there was a patch from Richard), but decided against it because -- back
> >>> then -- the default QEMU model was still very basic and would have
> >>> essentially disabled all more recent instructions as default.
> >>>
> >>> We can most probably do that change soon as we have a "fairly new"
> >>> default QEMU CPU model. I can glue it to my z14 change.  
> >>
> >> In case this makes the BIOS crash with old CPUs: when a guest refuses to
> >> start because the BIOS was compiled the wrong way for it, configure
> >> having told you so back then is not a nice user experience.  Can we do
> >> better, with reasonable effort?  
> > 
> > I fear the experience will be as bad as for any guest that is using
> > features from a newer cpu level (i.e. random crashes when the guest
> > actually tries to use that newer instruction.)
> > 
> > I see two options:
> > - Just try to start and hope that it works.
> > - Deprecate any cpu model older than z10.
> > 
> > Anyone have a better idea? I don't particularly like any of the two.  
> 
> As the default CPU model with new compat machines is >= z13, I wouldn't 
> lose sleep about this. Even with a broken bios one can still boot an 
> external kernel+initrd for testing purposes.

Yes, I do not see many people running into this problem. Still, I fear
it will be hard to figure out what exactly the problem is, when it
arises...



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/4] pc-bios/s390-ccw: Silence GCC 11 stringop-overflow warning
  2021-05-03  9:00   ` Cornelia Huck
@ 2021-05-03  9:30     ` Philippe Mathieu-Daudé
  2021-05-03  9:31       ` Thomas Huth
  0 siblings, 1 reply; 26+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-03  9:30 UTC (permalink / raw)
  To: Cornelia Huck, Thomas Huth; +Cc: Christian Borntraeger, qemu-s390x, qemu-devel

On 5/3/21 11:00 AM, Cornelia Huck wrote:
> On Sun,  2 May 2021 19:48:35 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> From: Philippe Mathieu-Daudé <philmd@redhat.com>
>>
>> When building on Fedora 34 (gcc version 11.0.0 20210210) we get:
>>
>>   In file included from pc-bios/s390-ccw/main.c:11:
>>   In function ‘memset’,
>>       inlined from ‘boot_setup’ at pc-bios/s390-ccw/main.c:185:5,
>>       inlined from ‘main’ at pc-bios/s390-ccw/main.c:288:5:
>>   pc-bios/s390-ccw/libc.h:28:14: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
>>      28 |         p[i] = c;
>>         |         ~~~~~^~~
>>
>> The offending code is:
>>
>>   memset((char *)S390EP, 0, 6);
>>
>> where S390EP is a const address:
>>
>>   #define S390EP 0x10008
>>
>> The compiler doesn't now how big that pointed area is, so assume its
> 
> s/now/know/
> s/assume/it assumes that/

Oops, thanks. Thomas, do you want me to repost this patch fixed?

>> length is zero. This has been reported as BZ#99578 to GCC:
>> "gcc-11 -Warray-bounds or -Wstringop-overread warning when accessing a
>> pointer from integer literal"
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
>>
>> As this warning does us more harm than good in the BIOS code (where
>> lot of direct accesses to low memory are done), silence this warning
>> for all BIOS objects.
>>
>> Suggested-by: Thomas Huth <thuth@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Message-Id: <20210422145911.2513980-1-philmd@redhat.com>
>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> [thuth: Use the pre-existing cc-option macro instead of adding a new one]
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  pc-bios/s390-ccw/Makefile | 1 +
>>  1 file changed, 1 insertion(+)



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/4] pc-bios/s390-ccw: Allow building with Clang, too
  2021-05-03  9:14           ` Cornelia Huck
  2021-05-03  9:17             ` David Hildenbrand
@ 2021-05-03  9:31             ` Thomas Huth
  2021-05-03  9:56               ` Cornelia Huck
  2021-05-03 10:10               ` Christian Borntraeger
  1 sibling, 2 replies; 26+ messages in thread
From: Thomas Huth @ 2021-05-03  9:31 UTC (permalink / raw)
  To: Cornelia Huck, Markus Armbruster
  Cc: Christian Borntraeger, qemu-s390x, Richard Henderson, qemu-devel,
	David Hildenbrand

On 03/05/2021 11.14, Cornelia Huck wrote:
> On Mon, 03 May 2021 10:23:20 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
> 
>> David Hildenbrand <david@redhat.com> writes:
>>
>>> On 03.05.21 07:17, Thomas Huth wrote:
>>>> On 03/05/2021 06.58, Markus Armbruster wrote:
>>>>> Thomas Huth <thuth@redhat.com> writes:
>>>>>   
>>>>>> Clang unfortunately does not support generating code for the z900
>>>>>> architecture level and starts with the z10 instead. Thus to be able
>>>>>> to support compiling with Clang, we have to check for the supported
>>>>>> compiler flags. The disadvantage is of course that the bios image
>>>>>> will only run with z10 guest CPUs upwards (which is what most people
>>>>>> use anyway), so just in case let's also emit a warning in that case.
>>>>>
>>>>> What happens when you try to use this bios with an old CPU anyway?
>>>>
>>>> Interesting question. I was expecting the guest to crash since it would be
>>>> using a CPU instruction that is not supported on the old CPU model. But I
>>>> just gave it a try, and there was no crash. The guest booted just fine.
>>>> Either Clang only emits instructions that work with the old z900 anyway, or
>>>> our emulation in QEMU is imprecise and we allow newer instructions to be
>>>> executed on old models, too.
>>>
>>> Yes, that's currently still done. We once thought about disabling that
>>> (there was a patch from Richard), but decided against it because -- back
>>> then -- the default QEMU model was still very basic and would have
>>> essentially disabled all more recent instructions as default.
>>>
>>> We can most probably do that change soon as we have a "fairly new"
>>> default QEMU CPU model. I can glue it to my z14 change.
>>
>> In case this makes the BIOS crash with old CPUs: when a guest refuses to
>> start because the BIOS was compiled the wrong way for it, configure
>> having told you so back then is not a nice user experience.  Can we do
>> better, with reasonable effort?
> 
> I fear the experience will be as bad as for any guest that is using
> features from a newer cpu level (i.e. random crashes when the guest
> actually tries to use that newer instruction.)
> 
> I see two options:
> - Just try to start and hope that it works.
> - Deprecate any cpu model older than z10.
> 
> Anyone have a better idea? I don't particularly like any of the two.

I think we should simply continue to build the default bios with GCC and 
-mz900. So the normal user (who does not explicitly use the freshly compiled 
binaries but the pre-built ones) will never experience any problem here. The 
Clang builds are (at least right now) rather only meant for us developers to 
check the sources from time to time with this compiler, to see whether it 
detects some additional issues compared to GCC.

  Thomas



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/4] pc-bios/s390-ccw: Silence GCC 11 stringop-overflow warning
  2021-05-03  9:30     ` Philippe Mathieu-Daudé
@ 2021-05-03  9:31       ` Thomas Huth
  2021-05-03  9:50         ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 26+ messages in thread
From: Thomas Huth @ 2021-05-03  9:31 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, Cornelia Huck
  Cc: Christian Borntraeger, qemu-s390x, qemu-devel

On 03/05/2021 11.30, Philippe Mathieu-Daudé wrote:
> On 5/3/21 11:00 AM, Cornelia Huck wrote:
>> On Sun,  2 May 2021 19:48:35 +0200
>> Thomas Huth <thuth@redhat.com> wrote:
>>
>>> From: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>
>>> When building on Fedora 34 (gcc version 11.0.0 20210210) we get:
>>>
>>>    In file included from pc-bios/s390-ccw/main.c:11:
>>>    In function ‘memset’,
>>>        inlined from ‘boot_setup’ at pc-bios/s390-ccw/main.c:185:5,
>>>        inlined from ‘main’ at pc-bios/s390-ccw/main.c:288:5:
>>>    pc-bios/s390-ccw/libc.h:28:14: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
>>>       28 |         p[i] = c;
>>>          |         ~~~~~^~~
>>>
>>> The offending code is:
>>>
>>>    memset((char *)S390EP, 0, 6);
>>>
>>> where S390EP is a const address:
>>>
>>>    #define S390EP 0x10008
>>>
>>> The compiler doesn't now how big that pointed area is, so assume its
>>
>> s/now/know/
>> s/assume/it assumes that/
> 
> Oops, thanks. Thomas, do you want me to repost this patch fixed?

I can fix it up in my tree here, no need to resend.

  Thomas



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/4] pc-bios/s390-ccw: Silence GCC 11 stringop-overflow warning
  2021-05-03  9:31       ` Thomas Huth
@ 2021-05-03  9:50         ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 26+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-03  9:50 UTC (permalink / raw)
  To: Thomas Huth, Cornelia Huck; +Cc: Christian Borntraeger, qemu-s390x, qemu-devel

On 5/3/21 11:31 AM, Thomas Huth wrote:
> On 03/05/2021 11.30, Philippe Mathieu-Daudé wrote:
>> On 5/3/21 11:00 AM, Cornelia Huck wrote:
>>> On Sun,  2 May 2021 19:48:35 +0200
>>> Thomas Huth <thuth@redhat.com> wrote:
>>>
>>>> From: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>>
>>>> When building on Fedora 34 (gcc version 11.0.0 20210210) we get:
>>>>
>>>>    In file included from pc-bios/s390-ccw/main.c:11:
>>>>    In function ‘memset’,
>>>>        inlined from ‘boot_setup’ at pc-bios/s390-ccw/main.c:185:5,
>>>>        inlined from ‘main’ at pc-bios/s390-ccw/main.c:288:5:
>>>>    pc-bios/s390-ccw/libc.h:28:14: warning: writing 1 byte into a
>>>> region of size 0 [-Wstringop-overflow=]
>>>>       28 |         p[i] = c;
>>>>          |         ~~~~~^~~
>>>>
>>>> The offending code is:
>>>>
>>>>    memset((char *)S390EP, 0, 6);
>>>>
>>>> where S390EP is a const address:
>>>>
>>>>    #define S390EP 0x10008
>>>>
>>>> The compiler doesn't now how big that pointed area is, so assume its
>>>
>>> s/now/know/
>>> s/assume/it assumes that/
>>
>> Oops, thanks. Thomas, do you want me to repost this patch fixed?
> 
> I can fix it up in my tree here, no need to resend.

Great, thank you!

Phil.



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/4] pc-bios/s390-ccw: Allow building with Clang, too
  2021-05-03  9:31             ` Thomas Huth
@ 2021-05-03  9:56               ` Cornelia Huck
  2021-05-03 10:10               ` Christian Borntraeger
  1 sibling, 0 replies; 26+ messages in thread
From: Cornelia Huck @ 2021-05-03  9:56 UTC (permalink / raw)
  To: Thomas Huth
  Cc: David Hildenbrand, Richard Henderson, qemu-devel,
	Markus Armbruster, Christian Borntraeger, qemu-s390x

On Mon, 3 May 2021 11:31:00 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 03/05/2021 11.14, Cornelia Huck wrote:
> > On Mon, 03 May 2021 10:23:20 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >   
> >> David Hildenbrand <david@redhat.com> writes:
> >>  
> >>> On 03.05.21 07:17, Thomas Huth wrote:  
> >>>> On 03/05/2021 06.58, Markus Armbruster wrote:  
> >>>>> Thomas Huth <thuth@redhat.com> writes:
> >>>>>     
> >>>>>> Clang unfortunately does not support generating code for the z900
> >>>>>> architecture level and starts with the z10 instead. Thus to be able
> >>>>>> to support compiling with Clang, we have to check for the supported
> >>>>>> compiler flags. The disadvantage is of course that the bios image
> >>>>>> will only run with z10 guest CPUs upwards (which is what most people
> >>>>>> use anyway), so just in case let's also emit a warning in that case.  
> >>>>>
> >>>>> What happens when you try to use this bios with an old CPU anyway?  
> >>>>
> >>>> Interesting question. I was expecting the guest to crash since it would be
> >>>> using a CPU instruction that is not supported on the old CPU model. But I
> >>>> just gave it a try, and there was no crash. The guest booted just fine.
> >>>> Either Clang only emits instructions that work with the old z900 anyway, or
> >>>> our emulation in QEMU is imprecise and we allow newer instructions to be
> >>>> executed on old models, too.  
> >>>
> >>> Yes, that's currently still done. We once thought about disabling that
> >>> (there was a patch from Richard), but decided against it because -- back
> >>> then -- the default QEMU model was still very basic and would have
> >>> essentially disabled all more recent instructions as default.
> >>>
> >>> We can most probably do that change soon as we have a "fairly new"
> >>> default QEMU CPU model. I can glue it to my z14 change.  
> >>
> >> In case this makes the BIOS crash with old CPUs: when a guest refuses to
> >> start because the BIOS was compiled the wrong way for it, configure
> >> having told you so back then is not a nice user experience.  Can we do
> >> better, with reasonable effort?  
> > 
> > I fear the experience will be as bad as for any guest that is using
> > features from a newer cpu level (i.e. random crashes when the guest
> > actually tries to use that newer instruction.)
> > 
> > I see two options:
> > - Just try to start and hope that it works.
> > - Deprecate any cpu model older than z10.
> > 
> > Anyone have a better idea? I don't particularly like any of the two.  
> 
> I think we should simply continue to build the default bios with GCC and 
> -mz900. So the normal user (who does not explicitly use the freshly compiled 
> binaries but the pre-built ones) will never experience any problem here. The 
> Clang builds are (at least right now) rather only meant for us developers to 
> check the sources from time to time with this compiler, to see whether it 
> detects some additional issues compared to GCC.

OK, sounds reasonable to me.



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/4] pc-bios/s390-ccw: Allow building with Clang, too
  2021-05-02 17:48 ` [PATCH 4/4] pc-bios/s390-ccw: Allow building with Clang, too Thomas Huth
  2021-05-02 19:00   ` Philippe Mathieu-Daudé
  2021-05-03  4:58   ` Markus Armbruster
@ 2021-05-03 10:00   ` Cornelia Huck
  2 siblings, 0 replies; 26+ messages in thread
From: Cornelia Huck @ 2021-05-03 10:00 UTC (permalink / raw)
  To: Thomas Huth; +Cc: Christian Borntraeger, qemu-s390x, qemu-devel

On Sun,  2 May 2021 19:48:36 +0200
Thomas Huth <thuth@redhat.com> wrote:

> Clang unfortunately does not support generating code for the z900
> architecture level and starts with the z10 instead. Thus to be able
> to support compiling with Clang, we have to check for the supported
> compiler flags. The disadvantage is of course that the bios image
> will only run with z10 guest CPUs upwards (which is what most people
> use anyway), so just in case let's also emit a warning in that case.

Maybe add a note here that the pre-built image will continue to be
built with gcc for the z900, so for most people nothing will change?

> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  configure                 | 9 ++++++++-
>  pc-bios/s390-ccw/Makefile | 3 ++-
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 

Reviewed-by: Cornelia Huck <cohuck@redhat.com>



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/4] pc-bios/s390-ccw: Allow building with Clang, too
  2021-05-03  9:31             ` Thomas Huth
  2021-05-03  9:56               ` Cornelia Huck
@ 2021-05-03 10:10               ` Christian Borntraeger
  1 sibling, 0 replies; 26+ messages in thread
From: Christian Borntraeger @ 2021-05-03 10:10 UTC (permalink / raw)
  To: Thomas Huth, Cornelia Huck, Markus Armbruster
  Cc: qemu-s390x, Richard Henderson, qemu-devel, David Hildenbrand



On 03.05.21 11:31, Thomas Huth wrote:
> On 03/05/2021 11.14, Cornelia Huck wrote:
>> On Mon, 03 May 2021 10:23:20 +0200
>> Markus Armbruster <armbru@redhat.com> wrote:
>>
>>> David Hildenbrand <david@redhat.com> writes:
>>>
>>>> On 03.05.21 07:17, Thomas Huth wrote:
>>>>> On 03/05/2021 06.58, Markus Armbruster wrote:
>>>>>> Thomas Huth <thuth@redhat.com> writes:
>>>>>>> Clang unfortunately does not support generating code for the z900
>>>>>>> architecture level and starts with the z10 instead. Thus to be able
>>>>>>> to support compiling with Clang, we have to check for the supported
>>>>>>> compiler flags. The disadvantage is of course that the bios image
>>>>>>> will only run with z10 guest CPUs upwards (which is what most people
>>>>>>> use anyway), so just in case let's also emit a warning in that case.
>>>>>>
>>>>>> What happens when you try to use this bios with an old CPU anyway?
>>>>>
>>>>> Interesting question. I was expecting the guest to crash since it would be
>>>>> using a CPU instruction that is not supported on the old CPU model. But I
>>>>> just gave it a try, and there was no crash. The guest booted just fine.
>>>>> Either Clang only emits instructions that work with the old z900 anyway, or
>>>>> our emulation in QEMU is imprecise and we allow newer instructions to be
>>>>> executed on old models, too.
>>>>
>>>> Yes, that's currently still done. We once thought about disabling that
>>>> (there was a patch from Richard), but decided against it because -- back
>>>> then -- the default QEMU model was still very basic and would have
>>>> essentially disabled all more recent instructions as default.
>>>>
>>>> We can most probably do that change soon as we have a "fairly new"
>>>> default QEMU CPU model. I can glue it to my z14 change.
>>>
>>> In case this makes the BIOS crash with old CPUs: when a guest refuses to
>>> start because the BIOS was compiled the wrong way for it, configure
>>> having told you so back then is not a nice user experience.  Can we do
>>> better, with reasonable effort?
>>
>> I fear the experience will be as bad as for any guest that is using
>> features from a newer cpu level (i.e. random crashes when the guest
>> actually tries to use that newer instruction.)
>>
>> I see two options:
>> - Just try to start and hope that it works.
>> - Deprecate any cpu model older than z10.
>>
>> Anyone have a better idea? I don't particularly like any of the two.
> 
> I think we should simply continue to build the default bios with GCC and -mz900. So the normal user (who does not explicitly use the freshly compiled binaries but the pre-built ones) will never experience any problem here. The Clang builds are (at least right now) rather only meant for us developers to check the sources from time to time with this compiler, to see whether it detects some additional issues compared to GCC.

Ack.


^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2021-05-03 10:13 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-02 17:48 [PATCH 0/4] pc-bios/s390-ccw: Allow building with Clang, too Thomas Huth
2021-05-02 17:48 ` [PATCH 1/4] pc-bios/s390-ccw: Silence warning from Clang by marking panic() as noreturn Thomas Huth
2021-05-02 18:57   ` Philippe Mathieu-Daudé
2021-05-03  4:53     ` Thomas Huth
2021-05-03  7:40       ` Philippe Mathieu-Daudé
2021-05-03  4:56     ` Markus Armbruster
2021-05-03  8:54   ` Cornelia Huck
2021-05-02 17:48 ` [PATCH 2/4] pc-bios/s390-ccw: Fix the cc-option macro in the Makefile Thomas Huth
2021-05-02 17:48 ` [PATCH 3/4] pc-bios/s390-ccw: Silence GCC 11 stringop-overflow warning Thomas Huth
2021-05-03  9:00   ` Cornelia Huck
2021-05-03  9:30     ` Philippe Mathieu-Daudé
2021-05-03  9:31       ` Thomas Huth
2021-05-03  9:50         ` Philippe Mathieu-Daudé
2021-05-02 17:48 ` [PATCH 4/4] pc-bios/s390-ccw: Allow building with Clang, too Thomas Huth
2021-05-02 19:00   ` Philippe Mathieu-Daudé
2021-05-03  4:58   ` Markus Armbruster
2021-05-03  5:17     ` Thomas Huth
2021-05-03  8:10       ` David Hildenbrand
2021-05-03  8:23         ` Markus Armbruster
2021-05-03  9:14           ` Cornelia Huck
2021-05-03  9:17             ` David Hildenbrand
2021-05-03  9:23               ` Cornelia Huck
2021-05-03  9:31             ` Thomas Huth
2021-05-03  9:56               ` Cornelia Huck
2021-05-03 10:10               ` Christian Borntraeger
2021-05-03 10:00   ` Cornelia Huck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).