Patchwork [5/6] target-alpha: Fix cpu_alpha_init

login
register
mail settings
Submitter Richard Henderson
Date Sept. 15, 2012, 8:24 p.m.
Message ID <1347740649-28646-6-git-send-email-rth@twiddle.net>
Download mbox | patch
Permalink /patch/184100/
State New
Headers show

Comments

Richard Henderson - Sept. 15, 2012, 8:24 p.m.
Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target-alpha/translate.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
Andreas Färber - Sept. 16, 2012, 1:01 p.m.
Am 15.09.2012 22:24, schrieb Richard Henderson:
> Signed-off-by: Richard Henderson <rth@twiddle.net>

This is lacking a proper description. I'd be very ashamed if we lost
something so obvious during the QOM conversion. So what's the symptoms here?

Andreas

> ---
>  target-alpha/translate.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/target-alpha/translate.c b/target-alpha/translate.c
> index 12de6a3..f998f75 100644
> --- a/target-alpha/translate.c
> +++ b/target-alpha/translate.c
> @@ -3525,6 +3525,7 @@ static const struct cpu_def_t cpu_defs[] = {
>  
>  CPUAlphaState * cpu_alpha_init (const char *cpu_model)
>  {
> +    static bool inited;
>      AlphaCPU *cpu;
>      CPUAlphaState *env;
>      int implver, amask, i, max;
> @@ -3532,7 +3533,10 @@ CPUAlphaState * cpu_alpha_init (const char *cpu_model)
>      cpu = ALPHA_CPU(object_new(TYPE_ALPHA_CPU));
>      env = &cpu->env;
>  
> -    alpha_translate_init();
> +    if (!inited) {
> +        inited = true;
> +        alpha_translate_init();
> +    }
>  
>      /* Default to ev67; no reason not to emulate insns by default.  */
>      implver = IMPLVER_21264;
> @@ -3549,6 +3553,7 @@ CPUAlphaState * cpu_alpha_init (const char *cpu_model)
>      }
>      env->implver = implver;
>      env->amask = amask;
> +    env->cpu_model_str = cpu_model;
>  
>      qemu_init_vcpu(env);
>      return env;
Richard Henderson - Sept. 16, 2012, 4:56 p.m.
On 2012-09-16 06:01, Andreas Färber wrote:
> This is lacking a proper description. I'd be very ashamed if we lost
> something so obvious during the QOM conversion. So what's the symptoms here?

The most important thing here is saving cpu_model_str.
The symptom being a SEGV on clone, when cpu_model_str is null.


r~
Andreas Färber - Sept. 16, 2012, 6 p.m.
Am 16.09.2012 18:56, schrieb Richard Henderson:
> On 2012-09-16 06:01, Andreas Färber wrote:
>> This is lacking a proper description. I'd be very ashamed if we lost
>> something so obvious during the QOM conversion. So what's the symptoms here?
> 
> The most important thing here is saving cpu_model_str.
> The symptom being a SEGV on clone, when cpu_model_str is null.

Ack on that. My understanding is it's not a regression but a bugfix.

As for the reentrancy of the translation initialization, I would prefer
that being handled in initfn, but we can first introduce it here and
move it later.

So, if you suggest a wording for the commit message I can apply this as
part of my next CPU pull (assuming that'll be earlier than linux-user).
I also have an include path fix cpu-qom.h -> cpu.h queued, and
cpu_alpha_init() is not yet returning AlphaCPU.

Andreas
Richard Henderson - Sept. 16, 2012, 6:34 p.m.
On 2012-09-16 11:00, Andreas Färber wrote:
> As for the reentrancy of the translation initialization, I would prefer
> that being handled in initfn, but we can first introduce it here and
> move it later.

Sure.  I was monkey copying the arm cpu_init for that.

> So, if you suggest a wording for the commit message I can apply this as
> part of my next CPU pull (assuming that'll be earlier than linux-user).

target-alpha: Initialize env->cpu_model_str

Save the cpu_model_str so that we have a non-null value when
creating a new cpu during clone.

Signed-off-by: Richard Henderson <rth@twiddle.net>


r~

Patch

diff --git a/target-alpha/translate.c b/target-alpha/translate.c
index 12de6a3..f998f75 100644
--- a/target-alpha/translate.c
+++ b/target-alpha/translate.c
@@ -3525,6 +3525,7 @@  static const struct cpu_def_t cpu_defs[] = {
 
 CPUAlphaState * cpu_alpha_init (const char *cpu_model)
 {
+    static bool inited;
     AlphaCPU *cpu;
     CPUAlphaState *env;
     int implver, amask, i, max;
@@ -3532,7 +3533,10 @@  CPUAlphaState * cpu_alpha_init (const char *cpu_model)
     cpu = ALPHA_CPU(object_new(TYPE_ALPHA_CPU));
     env = &cpu->env;
 
-    alpha_translate_init();
+    if (!inited) {
+        inited = true;
+        alpha_translate_init();
+    }
 
     /* Default to ev67; no reason not to emulate insns by default.  */
     implver = IMPLVER_21264;
@@ -3549,6 +3553,7 @@  CPUAlphaState * cpu_alpha_init (const char *cpu_model)
     }
     env->implver = implver;
     env->amask = amask;
+    env->cpu_model_str = cpu_model;
 
     qemu_init_vcpu(env);
     return env;