[Qemu-devel] [PATCH v2] target/s390x: Fix broken user mode

[Qemu-devel] [PATCH v2] target/s390x: Fix broken user mode

[Stefan Weil]

Returning NULL from get_max_cpu_model results in a SIGSEGV runtime error.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---

v2: Re-sent as v1 was damaged by my mailer.

This is also broken in Debian.

In addition, there is no default CPU ("any"), so binfmt and related
actions currently don't work. I hacked my local installation by
duplicating the "qemu" cpu definition for "any", but maybe there is
a better solution.

Regards
Stefan

 target/s390x/cpu_models.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 2a894ee..6e34763 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -660,7 +660,6 @@ static void check_compatibility(const S390CPUModel *max_model,
 
 static S390CPUModel *get_max_cpu_model(Error **errp)
 {
-#ifndef CONFIG_USER_ONLY
     static S390CPUModel max_model;
     static bool cached;
 
@@ -680,7 +679,6 @@ static S390CPUModel *get_max_cpu_model(Error **errp)
         cached = true;
         return &max_model;
     }
-#endif
     return NULL;
 }
 
-- 
2.1.4

Re: [Qemu-devel] [PATCH v2] target/s390x: Fix broken user mode

[David Hildenbrand]

Am 30.01.2017 um 14:15 schrieb Stefan Weil:
> Returning NULL from get_max_cpu_model results in a SIGSEGV runtime error.
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
> 
> v2: Re-sent as v1 was damaged by my mailer.
> 
> This is also broken in Debian.
> 
> In addition, there is no default CPU ("any"), so binfmt and related
> actions currently don't work. I hacked my local installation by
> duplicating the "qemu" cpu definition for "any", but maybe there is
> a better solution.

we have the "qemu" model for that (or simply drop the -cpu parameter
completely). Eduardo currently proposed "max" as an alternative for x86.
There is also no "any" model on x86.

> 
> Regards
> Stefan
> 
>  target/s390x/cpu_models.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index 2a894ee..6e34763 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -660,7 +660,6 @@ static void check_compatibility(const S390CPUModel *max_model,
>  
>  static S390CPUModel *get_max_cpu_model(Error **errp)
>  {
> -#ifndef CONFIG_USER_ONLY
>      static S390CPUModel max_model;
>      static bool cached;
>  
> @@ -680,7 +679,6 @@ static S390CPUModel *get_max_cpu_model(Error **errp)
>          cached = true;
>          return &max_model;
>      }
> -#endif
>      return NULL;
>  }
>  
> 

Okay, the main problem is that get_max_cpu_model() doesn't return an
error in this case, therefor s390_realize_cpu_model() doesn't fail
correctly.

But it seems like cpu models also make sense for user mode (as you
recognized, we tried to not touch it but failed). However, for now, just
as for ordinary tcg, only "qemu"/z900 is supported, until proper support
for tcg is implemented.

Then, also the CONFIG_USER_ONLY in apply_cpu_model() has to go.

So this makes sense to me and should fix the current problem.

-- 

David

Re: [Qemu-devel] [PATCH v2] target/s390x: Fix broken user mode

[David Hildenbrand]

Am 30.01.2017 um 14:15 schrieb Stefan Weil:
> Returning NULL from get_max_cpu_model results in a SIGSEGV runtime error.
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
> 
> v2: Re-sent as v1 was damaged by my mailer.
> 

Just tested your patch:

Reviewed-by: David Hildenbrand <david@redhat.com>

> This is also broken in Debian.
> 
> In addition, there is no default CPU ("any"), so binfmt and related
> actions currently don't work. I hacked my local installation by
> duplicating the "qemu" cpu definition for "any", but maybe there is
> a better solution.
> 

This should do the trick, will send it as an official patch:

>From e7109d1c235e463c12feeaf5d3f1d6763f34be85 Mon Sep 17 00:00:00 2001
From: David Hildenbrand <david@redhat.com>
Date: Mon, 30 Jan 2017 15:33:05 +0100
Subject: [PATCH] target/s390x: "qemu" is the name od the default model

"any" does not exist, therefore resulting in a misleading error message.

Reported-by: Stefan Weil <sw@weilnetz.de>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 linux-user/main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/linux-user/main.c b/linux-user/main.c
index 3004958..e588f58 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -4322,6 +4322,8 @@ int main(int argc, char **argv, char **envp)
 # endif
 #elif defined TARGET_SH4
         cpu_model = TYPE_SH7785_CPU;
+#elif defined TARGET_S390X
+        cpu_model = "qemu";
 #else
         cpu_model = "any";
 #endif
-- 
2.9.3

-- 

David

Re: [Qemu-devel] [PATCH v2] target/s390x: Fix broken user mode

[Peter Maydell]

On 30 January 2017 at 14:10, David Hildenbrand <david@redhat.com> wrote:
> Am 30.01.2017 um 14:15 schrieb Stefan Weil:
>> Returning NULL from get_max_cpu_model results in a SIGSEGV runtime error.
>>
>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>> ---
>>
>> v2: Re-sent as v1 was damaged by my mailer.
>>
>> This is also broken in Debian.
>>
>> In addition, there is no default CPU ("any"), so binfmt and related
>> actions currently don't work. I hacked my local installation by
>> duplicating the "qemu" cpu definition for "any", but maybe there is
>> a better solution.
>
> we have the "qemu" model for that (or simply drop the -cpu parameter
> completely). Eduardo currently proposed "max" as an alternative for x86.
> There is also no "any" model on x86.

"any" isn't quite the same as the proposed "max", necessarily.
For instance on ARM "any" turns on some features which in real
hardware you don't ever see in combination, because for userspace
they don't conflict and this lets us run the widest possible
range of userspace binaries.

If "any" isn't right for s390 then you should fix the code
in linux-user/main.c which picks the default CPU model (for
instance on x86 it selects either "qemu64" or "qemu32").

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] target/s390x: Fix broken user mode

[David Hildenbrand]

Am 30.01.2017 um 14:15 schrieb Stefan Weil:
> Returning NULL from get_max_cpu_model results in a SIGSEGV runtime error.
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
> 
> v2: Re-sent as v1 was damaged by my mailer.
> 
> This is also broken in Debian.
> 
> In addition, there is no default CPU ("any"), so binfmt and related
> actions currently don't work. I hacked my local installation by
> duplicating the "qemu" cpu definition for "any", but maybe there is
> a better solution.

That should then already work.

> 
> Regards
> Stefan
> 
>  target/s390x/cpu_models.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index 2a894ee..6e34763 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -660,7 +660,6 @@ static void check_compatibility(const S390CPUModel *max_model,
>  
>  static S390CPUModel *get_max_cpu_model(Error **errp)
>  {
> -#ifndef CONFIG_USER_ONLY
>      static S390CPUModel max_model;
>      static bool cached;
>  
> @@ -680,7 +679,6 @@ static S390CPUModel *get_max_cpu_model(Error **errp)
>          cached = true;
>          return &max_model;
>      }
> -#endif
>      return NULL;
>  }
>  
> 

I thought this was already picked up for stable...
But looks like only the "any" model fixup got.

So

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David

Re: [Qemu-devel] [PATCH v2] target/s390x: Fix broken user mode

[David Hildenbrand]

On 02.03.2017 20:56, David Hildenbrand wrote:
> Am 30.01.2017 um 14:15 schrieb Stefan Weil:
>> Returning NULL from get_max_cpu_model results in a SIGSEGV runtime error.
>>
>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>> ---
>>
>> v2: Re-sent as v1 was damaged by my mailer.
>>
>> This is also broken in Debian.
>>
>> In addition, there is no default CPU ("any"), so binfmt and related
>> actions currently don't work. I hacked my local installation by
>> duplicating the "qemu" cpu definition for "any", but maybe there is
>> a better solution.
> 
> That should then already work.
> 
>>
>> Regards
>> Stefan
>>
>>  target/s390x/cpu_models.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
>> index 2a894ee..6e34763 100644
>> --- a/target/s390x/cpu_models.c
>> +++ b/target/s390x/cpu_models.c
>> @@ -660,7 +660,6 @@ static void check_compatibility(const S390CPUModel *max_model,
>>  
>>  static S390CPUModel *get_max_cpu_model(Error **errp)
>>  {
>> -#ifndef CONFIG_USER_ONLY
>>      static S390CPUModel max_model;
>>      static bool cached;
>>  
>> @@ -680,7 +679,6 @@ static S390CPUModel *get_max_cpu_model(Error **errp)
>>          cached = true;
>>          return &max_model;
>>      }
>> -#endif
>>      return NULL;
>>  }
>>  
>>
> 
> I thought this was already picked up for stable...
> But looks like only the "any" model fixup got.
> 
> So
> 
> Acked-by: David Hildenbrand <david@redhat.com>
> 

Ping, can somebody please pick this up?

-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v2] target/s390x: Fix broken user mode

[Philippe Mathieu-Daudé]

Hi Stefan, Richard.

On 03/16/2017 10:28 AM, David Hildenbrand wrote:
> On 02.03.2017 20:56, David Hildenbrand wrote:
>> Am 30.01.2017 um 14:15 schrieb Stefan Weil:
>>> Returning NULL from get_max_cpu_model results in a SIGSEGV runtime error.
>>>
>>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>>> ---
>>>
>>> v2: Re-sent as v1 was damaged by my mailer.
>>>
>>> This is also broken in Debian.
>>>
>>> In addition, there is no default CPU ("any"), so binfmt and related
>>> actions currently don't work. I hacked my local installation by
>>> duplicating the "qemu" cpu definition for "any", but maybe there is
>>> a better solution.
>>
>> That should then already work.
>>
>>>
>>> Regards
>>> Stefan
>>>
>>>  target/s390x/cpu_models.c | 2 --
>>>  1 file changed, 2 deletions(-)
>>>
>>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
>>> index 2a894ee..6e34763 100644
>>> --- a/target/s390x/cpu_models.c
>>> +++ b/target/s390x/cpu_models.c
>>> @@ -660,7 +660,6 @@ static void check_compatibility(const S390CPUModel *max_model,
>>>
>>>  static S390CPUModel *get_max_cpu_model(Error **errp)
>>>  {
>>> -#ifndef CONFIG_USER_ONLY
>>>      static S390CPUModel max_model;
>>>      static bool cached;
>>>
>>> @@ -680,7 +679,6 @@ static S390CPUModel *get_max_cpu_model(Error **errp)
>>>          cached = true;
>>>          return &max_model;
>>>      }
>>> -#endif
>>>      return NULL;
>>>  }
>>>
>>>
>>
>> I thought this was already picked up for stable...
>> But looks like only the "any" model fixup got.
>>
>> So
>>
>> Acked-by: David Hildenbrand <david@redhat.com>
>>
>
> Ping, can somebody please pick this up?

I see at least two possible changes:

the nasty one to catch bug:

@@ -681,7 +681,7 @@ static S390CPUModel *get_max_cpu_model(Error **errp)
          return &max_model;
      }
  #endif
-    return NULL;
+    abort();
  }

and the nicer one to avoid SIGSEGV:

@@ -734,8 +734,10 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
      }

      max_model = get_max_cpu_model(errp);
-    if (*errp) {
-        error_prepend(errp, "CPU models are not available: ");
+    if (max_model == NULL) {
+        if (*errp) {
+            error_prepend(errp, "CPU models are not available: ");
+        } else {
+            error_setg(errp, "CPU models are not available");
+        }
          return;
      }

Re: [Qemu-devel] [PATCH v2] target/s390x: Fix broken user mode

[David Hildenbrand]

On 16.03.2017 15:46, Philippe Mathieu-Daudé wrote:
> Hi Stefan, Richard.
> 
> On 03/16/2017 10:28 AM, David Hildenbrand wrote:
>> On 02.03.2017 20:56, David Hildenbrand wrote:
>>> Am 30.01.2017 um 14:15 schrieb Stefan Weil:
>>>> Returning NULL from get_max_cpu_model results in a SIGSEGV runtime error.
>>>>
>>>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>>>> ---
>>>>
>>>> v2: Re-sent as v1 was damaged by my mailer.
>>>>
>>>> This is also broken in Debian.
>>>>
>>>> In addition, there is no default CPU ("any"), so binfmt and related
>>>> actions currently don't work. I hacked my local installation by
>>>> duplicating the "qemu" cpu definition for "any", but maybe there is
>>>> a better solution.
>>>
>>> That should then already work.
>>>
>>>>
>>>> Regards
>>>> Stefan
>>>>
>>>>  target/s390x/cpu_models.c | 2 --
>>>>  1 file changed, 2 deletions(-)
>>>>
>>>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
>>>> index 2a894ee..6e34763 100644
>>>> --- a/target/s390x/cpu_models.c
>>>> +++ b/target/s390x/cpu_models.c
>>>> @@ -660,7 +660,6 @@ static void check_compatibility(const S390CPUModel *max_model,
>>>>
>>>>  static S390CPUModel *get_max_cpu_model(Error **errp)
>>>>  {
>>>> -#ifndef CONFIG_USER_ONLY
>>>>      static S390CPUModel max_model;
>>>>      static bool cached;
>>>>
>>>> @@ -680,7 +679,6 @@ static S390CPUModel *get_max_cpu_model(Error **errp)
>>>>          cached = true;
>>>>          return &max_model;
>>>>      }
>>>> -#endif
>>>>      return NULL;
>>>>  }
>>>>
>>>>
>>>
>>> I thought this was already picked up for stable...
>>> But looks like only the "any" model fixup got.
>>>
>>> So
>>>
>>> Acked-by: David Hildenbrand <david@redhat.com>
>>>
>>
>> Ping, can somebody please pick this up?
> 
> I see at least two possible changes:
> 
> the nasty one to catch bug:
> 
> @@ -681,7 +681,7 @@ static S390CPUModel *get_max_cpu_model(Error **errp)
>           return &max_model;
>       }
>   #endif
> -    return NULL;
> +    abort();
>   }
> 
> and the nicer one to avoid SIGSEGV:
> 
> @@ -734,8 +734,10 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
>       }
> 
>       max_model = get_max_cpu_model(errp);
> -    if (*errp) {
> -        error_prepend(errp, "CPU models are not available: ");
> +    if (max_model == NULL) {
> +        if (*errp) {
> +            error_prepend(errp, "CPU models are not available: ");
> +        } else {
> +            error_setg(errp, "CPU models are not available");
> +        }
>           return;
>       }
> 

As already discussed, user mode should behave just like tcg. No special
handling. So the original patch here is just fine in my opinion.

-- 

Thanks,

David

Re: [Qemu-devel] [PATCH v2] target/s390x: Fix broken user mode

[Christian Borntraeger]

On 01/30/2017 02:15 PM, Stefan Weil wrote:
> Returning NULL from get_max_cpu_model results in a SIGSEGV runtime error.
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
> 
> v2: Re-sent as v1 was damaged by my mailer.
> 
> This is also broken in Debian.
> 
> In addition, there is no default CPU ("any"), so binfmt and related
> actions currently don't work. I hacked my local installation by
> duplicating the "qemu" cpu definition for "any", but maybe there is
> a better solution.

applied to our tree. Do we need cc stable as well?


> 
> Regards
> Stefan
> 
>  target/s390x/cpu_models.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index 2a894ee..6e34763 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -660,7 +660,6 @@ static void check_compatibility(const S390CPUModel *max_model,
> 
>  static S390CPUModel *get_max_cpu_model(Error **errp)
>  {
> -#ifndef CONFIG_USER_ONLY
>      static S390CPUModel max_model;
>      static bool cached;
> 
> @@ -680,7 +679,6 @@ static S390CPUModel *get_max_cpu_model(Error **errp)
>          cached = true;
>          return &max_model;
>      }
> -#endif
>      return NULL;
>  }
> 

Re: [Qemu-devel] [PATCH v2] target/s390x: Fix broken user mode

[Christian Borntraeger]

On 01/30/2017 02:15 PM, Stefan Weil wrote:
> Returning NULL from get_max_cpu_model results in a SIGSEGV runtime error.
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
> 
> v2: Re-sent as v1 was damaged by my mailer.
> 
> This is also broken in Debian.
> 
> In addition, there is no default CPU ("any"), so binfmt and related
> actions currently don't work. I hacked my local installation by
> duplicating the "qemu" cpu definition for "any", but maybe there is
> a better solution.
> 
> Regards
> Stefan
> 
>  target/s390x/cpu_models.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index 2a894ee..6e34763 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -660,7 +660,6 @@ static void check_compatibility(const S390CPUModel *max_model,
> 
>  static S390CPUModel *get_max_cpu_model(Error **errp)
>  {
> -#ifndef CONFIG_USER_ONLY
>      static S390CPUModel max_model;
>      static bool cached;
> 
> @@ -680,7 +679,6 @@ static S390CPUModel *get_max_cpu_model(Error **errp)
>          cached = true;
>          return &max_model;
>      }
> -#endif
>      return NULL;
>  }
> 

applied to our tree. Do we need cc stable?

Re: [Qemu-devel] [PATCH v2] target/s390x: Fix broken user mode

[David Hildenbrand]

On 22.03.2017 10:07, Christian Borntraeger wrote:
> On 01/30/2017 02:15 PM, Stefan Weil wrote:
>> Returning NULL from get_max_cpu_model results in a SIGSEGV runtime error.
>>
>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>> ---
>>
>> v2: Re-sent as v1 was damaged by my mailer.
>>
>> This is also broken in Debian.
>>
>> In addition, there is no default CPU ("any"), so binfmt and related
>> actions currently don't work. I hacked my local installation by
>> duplicating the "qemu" cpu definition for "any", but maybe there is
>> a better solution.
> 
> applied to our tree. Do we need cc stable as well?

Yes, I think so.

Thanks!

-- 

Thanks,

David