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

[David Hildenbrand <david@redhat.com>]

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