[PATCH 0/2] cpu: Avoid latent bug calling cpu_reset() on uninitialized vCPU

[PATCH 0/2] cpu: Avoid latent bug calling cpu_reset() on uninitialized vCPU

[Philippe Mathieu-Daudé]

Two trivial patches to avoid each (new) architecture to track
a bug already resolved once on ARM: cpu_reset() modify fields
in the CpuState while the state is not yet allocated.

Philippe Mathieu-Daudé (2):
  cpu: Do not reset a vCPU before it is created
  cpu: Assert a vCPU is created before resetting it

 hw/core/cpu.c        | 1 +
 target/cris/cpu.c    | 2 +-
 target/lm32/cpu.c    | 3 +--
 target/m68k/cpu.c    | 2 +-
 target/mips/cpu.c    | 2 +-
 target/sh4/cpu.c     | 2 +-
 target/tilegx/cpu.c  | 2 +-
 target/tricore/cpu.c | 2 +-
 8 files changed, 8 insertions(+), 8 deletions(-)

-- 
2.21.1

[PATCH 1/2] cpu: Do not reset a vCPU before it is created

[Philippe Mathieu-Daudé]

cpu_reset() might modify architecture-specific fields allocated
by qemu_init_vcpu(). To avoid bugs similar to the one fixed in
commit 00d0f7cb66 when introducing new architectures, move the
cpu_reset() calls after qemu_init_vcpu().

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 target/cris/cpu.c    | 2 +-
 target/lm32/cpu.c    | 3 +--
 target/m68k/cpu.c    | 2 +-
 target/mips/cpu.c    | 2 +-
 target/sh4/cpu.c     | 2 +-
 target/tilegx/cpu.c  | 2 +-
 target/tricore/cpu.c | 2 +-
 7 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/target/cris/cpu.c b/target/cris/cpu.c
index 17c6712e29..9b8b99840d 100644
--- a/target/cris/cpu.c
+++ b/target/cris/cpu.c
@@ -134,8 +134,8 @@ static void cris_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    cpu_reset(cs);
     qemu_init_vcpu(cs);
+    cpu_reset(cs);
 
     ccc->parent_realize(dev, errp);
 }
diff --git a/target/lm32/cpu.c b/target/lm32/cpu.c
index 687bf35e65..56f7b97c8f 100644
--- a/target/lm32/cpu.c
+++ b/target/lm32/cpu.c
@@ -132,9 +132,8 @@ static void lm32_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    cpu_reset(cs);
-
     qemu_init_vcpu(cs);
+    cpu_reset(cs);
 
     lcc->parent_realize(dev, errp);
 }
diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index f0653cda2f..51ca62694e 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -247,8 +247,8 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error **errp)
 
     m68k_cpu_init_gdb(cpu);
 
-    cpu_reset(cs);
     qemu_init_vcpu(cs);
+    cpu_reset(cs);
 
     mcc->parent_realize(dev, errp);
 }
diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index 6cd6b9650b..553945005f 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -149,8 +149,8 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
 
     cpu_mips_realize_env(&cpu->env);
 
-    cpu_reset(cs);
     qemu_init_vcpu(cs);
+    cpu_reset(cs);
 
     mcc->parent_realize(dev, errp);
 }
diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c
index 70c8d8170f..2564436719 100644
--- a/target/sh4/cpu.c
+++ b/target/sh4/cpu.c
@@ -184,8 +184,8 @@ static void superh_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    cpu_reset(cs);
     qemu_init_vcpu(cs);
+    cpu_reset(cs);
 
     scc->parent_realize(dev, errp);
 }
diff --git a/target/tilegx/cpu.c b/target/tilegx/cpu.c
index cd422a0467..7e9982197f 100644
--- a/target/tilegx/cpu.c
+++ b/target/tilegx/cpu.c
@@ -91,8 +91,8 @@ static void tilegx_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    cpu_reset(cs);
     qemu_init_vcpu(cs);
+    cpu_reset(cs);
 
     tcc->parent_realize(dev, errp);
 }
diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c
index 85bc9f03a1..c5a5d54569 100644
--- a/target/tricore/cpu.c
+++ b/target/tricore/cpu.c
@@ -94,8 +94,8 @@ static void tricore_cpu_realizefn(DeviceState *dev, Error **errp)
     if (tricore_feature(env, TRICORE_FEATURE_131)) {
         set_feature(env, TRICORE_FEATURE_13);
     }
-    cpu_reset(cs);
     qemu_init_vcpu(cs);
+    cpu_reset(cs);
 
     tcc->parent_realize(dev, errp);
 }
-- 
2.21.1

Re: [PATCH 1/2] cpu: Do not reset a vCPU before it is created

[Laurent Vivier]

Le 09/03/2020 à 13:11, Philippe Mathieu-Daudé a écrit :
> cpu_reset() might modify architecture-specific fields allocated
> by qemu_init_vcpu(). To avoid bugs similar to the one fixed in
> commit 00d0f7cb66 when introducing new architectures, move the
> cpu_reset() calls after qemu_init_vcpu().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  target/cris/cpu.c    | 2 +-
>  target/lm32/cpu.c    | 3 +--
>  target/m68k/cpu.c    | 2 +-
>  target/mips/cpu.c    | 2 +-
>  target/sh4/cpu.c     | 2 +-
>  target/tilegx/cpu.c  | 2 +-
>  target/tricore/cpu.c | 2 +-
>  7 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/target/cris/cpu.c b/target/cris/cpu.c
> index 17c6712e29..9b8b99840d 100644
> --- a/target/cris/cpu.c
> +++ b/target/cris/cpu.c
> @@ -134,8 +134,8 @@ static void cris_cpu_realizefn(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    cpu_reset(cs);
>      qemu_init_vcpu(cs);
> +    cpu_reset(cs);
>  
>      ccc->parent_realize(dev, errp);
>  }
> diff --git a/target/lm32/cpu.c b/target/lm32/cpu.c
> index 687bf35e65..56f7b97c8f 100644
> --- a/target/lm32/cpu.c
> +++ b/target/lm32/cpu.c
> @@ -132,9 +132,8 @@ static void lm32_cpu_realizefn(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    cpu_reset(cs);
> -
>      qemu_init_vcpu(cs);
> +    cpu_reset(cs);
>  
>      lcc->parent_realize(dev, errp);
>  }
> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
> index f0653cda2f..51ca62694e 100644
> --- a/target/m68k/cpu.c
> +++ b/target/m68k/cpu.c
> @@ -247,8 +247,8 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error **errp)
>  
>      m68k_cpu_init_gdb(cpu);
>  
> -    cpu_reset(cs);
>      qemu_init_vcpu(cs);
> +    cpu_reset(cs);
>  
>      mcc->parent_realize(dev, errp);
>  }
> diff --git a/target/mips/cpu.c b/target/mips/cpu.c
> index 6cd6b9650b..553945005f 100644
> --- a/target/mips/cpu.c
> +++ b/target/mips/cpu.c
> @@ -149,8 +149,8 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
>  
>      cpu_mips_realize_env(&cpu->env);
>  
> -    cpu_reset(cs);
>      qemu_init_vcpu(cs);
> +    cpu_reset(cs);
>  
>      mcc->parent_realize(dev, errp);
>  }
> diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c
> index 70c8d8170f..2564436719 100644
> --- a/target/sh4/cpu.c
> +++ b/target/sh4/cpu.c
> @@ -184,8 +184,8 @@ static void superh_cpu_realizefn(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    cpu_reset(cs);
>      qemu_init_vcpu(cs);
> +    cpu_reset(cs);
>  
>      scc->parent_realize(dev, errp);
>  }
> diff --git a/target/tilegx/cpu.c b/target/tilegx/cpu.c
> index cd422a0467..7e9982197f 100644
> --- a/target/tilegx/cpu.c
> +++ b/target/tilegx/cpu.c
> @@ -91,8 +91,8 @@ static void tilegx_cpu_realizefn(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    cpu_reset(cs);
>      qemu_init_vcpu(cs);
> +    cpu_reset(cs);
>  
>      tcc->parent_realize(dev, errp);
>  }
> diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c
> index 85bc9f03a1..c5a5d54569 100644
> --- a/target/tricore/cpu.c
> +++ b/target/tricore/cpu.c
> @@ -94,8 +94,8 @@ static void tricore_cpu_realizefn(DeviceState *dev, Error **errp)
>      if (tricore_feature(env, TRICORE_FEATURE_131)) {
>          set_feature(env, TRICORE_FEATURE_13);
>      }
> -    cpu_reset(cs);
>      qemu_init_vcpu(cs);
> +    cpu_reset(cs);
>  
>      tcc->parent_realize(dev, errp);
>  }
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH 1/2] cpu: Do not reset a vCPU before it is created

[Peter Maydell]

On Mon, 9 Mar 2020 at 12:11, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> cpu_reset() might modify architecture-specific fields allocated
> by qemu_init_vcpu(). To avoid bugs similar to the one fixed in
> commit 00d0f7cb66 when introducing new architectures, move the
> cpu_reset() calls after qemu_init_vcpu().
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Why do we need to call cpu_reset() from realize anyway?
Generally for devices this is incorrect as they should be
being reset by some other mechanism.

Obviously actually determining that dropping the cpu_reset()
call is safe would require some tedious auditing.

If we do do a cpu_reset() in realize, should it be after
the call to the parent realize function ?

thanks
-- PMM

Re: [PATCH 1/2] cpu: Do not reset a vCPU before it is created

[Laurent Vivier]

Le 09/03/2020 à 14:09, Peter Maydell a écrit :
> On Mon, 9 Mar 2020 at 12:11, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> cpu_reset() might modify architecture-specific fields allocated
>> by qemu_init_vcpu(). To avoid bugs similar to the one fixed in
>> commit 00d0f7cb66 when introducing new architectures, move the
>> cpu_reset() calls after qemu_init_vcpu().
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> Why do we need to call cpu_reset() from realize anyway?
> Generally for devices this is incorrect as they should be
> being reset by some other mechanism.

I have this same change in my branch for q800 to set the initial PC and
stack pointer read from memory on cold boot.

Do we have another way to do that?

Thanks,
Laurent

Re: [PATCH 1/2] cpu: Do not reset a vCPU before it is created

[Peter Maydell]

On Mon, 9 Mar 2020 at 13:13, Laurent Vivier <laurent@vivier.eu> wrote:
>
> Le 09/03/2020 à 14:09, Peter Maydell a écrit :
> > On Mon, 9 Mar 2020 at 12:11, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >>
> >> cpu_reset() might modify architecture-specific fields allocated
> >> by qemu_init_vcpu(). To avoid bugs similar to the one fixed in
> >> commit 00d0f7cb66 when introducing new architectures, move the
> >> cpu_reset() calls after qemu_init_vcpu().
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >
> > Why do we need to call cpu_reset() from realize anyway?
> > Generally for devices this is incorrect as they should be
> > being reset by some other mechanism.
>
> I have this same change in my branch for q800 to set the initial PC and
> stack pointer read from memory on cold boot.
>
> Do we have another way to do that?

The expectation at the moment is that the board code should
register a reset function with qemu_register_reset() which
calls cpu_reset(). Relying on doing a reset in realize won't
work for the case where there's a QEMU system reset, because
we don't re-init/realize everything, we just call all the
reset hooks.

If m68k reads pc/sp from memory on reset you'll probably run
into the same reset-ordering vs hw/cpu/loader.c that Arm M-profile
has; we currently work around that in the arm reset function.
I had hoped that 3-phase reset would resolve this reset order
issue, but it has not turned out to be so easy.

thanks
-- PMM

Re: [PATCH 1/2] cpu: Do not reset a vCPU before it is created

[Laurent Vivier]

Le 09/03/2020 à 14:18, Peter Maydell a écrit :
> On Mon, 9 Mar 2020 at 13:13, Laurent Vivier <laurent@vivier.eu> wrote:
>>
>> Le 09/03/2020 à 14:09, Peter Maydell a écrit :
>>> On Mon, 9 Mar 2020 at 12:11, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>>>
>>>> cpu_reset() might modify architecture-specific fields allocated
>>>> by qemu_init_vcpu(). To avoid bugs similar to the one fixed in
>>>> commit 00d0f7cb66 when introducing new architectures, move the
>>>> cpu_reset() calls after qemu_init_vcpu().
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>
>>> Why do we need to call cpu_reset() from realize anyway?
>>> Generally for devices this is incorrect as they should be
>>> being reset by some other mechanism.
>>
>> I have this same change in my branch for q800 to set the initial PC and
>> stack pointer read from memory on cold boot.
>>
>> Do we have another way to do that?
> 
> The expectation at the moment is that the board code should
> register a reset function with qemu_register_reset() which
> calls cpu_reset(). Relying on doing a reset in realize won't
> work for the case where there's a QEMU system reset, because
> we don't re-init/realize everything, we just call all the
> reset hooks.
> 
> If m68k reads pc/sp from memory on reset you'll probably run
> into the same reset-ordering vs hw/cpu/loader.c that Arm M-profile
> has; we currently work around that in the arm reset function.
> I had hoped that 3-phase reset would resolve this reset order
> issue, but it has not turned out to be so easy.


Thank you for the hint, I'll have a look.

Laurent

[PATCH 2/2] cpu: Assert a vCPU is created before resetting it

[Philippe Mathieu-Daudé]

cpu_reset() might modify architecture-specific fields allocated
by qemu_init_vcpu(). To avoid bugs similar to the one fixed in
commit 00d0f7cb66 when introducing new architectures, assert a
vCPU is created before resetting it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/core/cpu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/core/cpu.c b/hw/core/cpu.c
index fe65ca62ac..09e49f8d6a 100644
--- a/hw/core/cpu.c
+++ b/hw/core/cpu.c
@@ -251,6 +251,7 @@ void cpu_reset(CPUState *cpu)
 {
     CPUClass *klass = CPU_GET_CLASS(cpu);
 
+    assert(cpu->created);
     if (klass->reset != NULL) {
         (*klass->reset)(cpu);
     }
-- 
2.21.1

Re: [PATCH 2/2] cpu: Assert a vCPU is created before resetting it

[Peter Maydell]

On Mon, 9 Mar 2020 at 12:11, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> cpu_reset() might modify architecture-specific fields allocated
> by qemu_init_vcpu(). To avoid bugs similar to the one fixed in
> commit 00d0f7cb66 when introducing new architectures, assert a
> vCPU is created before resetting it.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/core/cpu.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/core/cpu.c b/hw/core/cpu.c
> index fe65ca62ac..09e49f8d6a 100644
> --- a/hw/core/cpu.c
> +++ b/hw/core/cpu.c
> @@ -251,6 +251,7 @@ void cpu_reset(CPUState *cpu)
>  {
>      CPUClass *klass = CPU_GET_CLASS(cpu);
>
> +    assert(cpu->created);
>      if (klass->reset != NULL) {
>          (*klass->reset)(cpu);
>      }

This will conflict with the change to use DeviceClass::reset.

Ideally we should do an equivalent assert in the DeviceClass
(and flush out all the bugs where we forgot to realize the
device before using it).

thanks
-- PMM

Re: [PATCH 2/2] cpu: Assert a vCPU is created before resetting it

[Philippe Mathieu-Daudé]

On 3/9/20 2:10 PM, Peter Maydell wrote:
> On Mon, 9 Mar 2020 at 12:11, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> cpu_reset() might modify architecture-specific fields allocated
>> by qemu_init_vcpu(). To avoid bugs similar to the one fixed in
>> commit 00d0f7cb66 when introducing new architectures, assert a
>> vCPU is created before resetting it.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   hw/core/cpu.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/core/cpu.c b/hw/core/cpu.c
>> index fe65ca62ac..09e49f8d6a 100644
>> --- a/hw/core/cpu.c
>> +++ b/hw/core/cpu.c
>> @@ -251,6 +251,7 @@ void cpu_reset(CPUState *cpu)
>>   {
>>       CPUClass *klass = CPU_GET_CLASS(cpu);
>>
>> +    assert(cpu->created);
>>       if (klass->reset != NULL) {
>>           (*klass->reset)(cpu);
>>       }
> 
> This will conflict with the change to use DeviceClass::reset.
> 
> Ideally we should do an equivalent assert in the DeviceClass
> (and flush out all the bugs where we forgot to realize the
> device before using it).

OK (I should have sent as RFC probably).

Anyway this fails the ppc64le/s390x linux-user tests on Travis-CI:

qemu-ppc64le: hw/core/cpu.c:254: cpu_reset: Assertion `cpu->created' failed.

qemu-s390x: hw/core/cpu.c:254: cpu_reset: Assertion `cpu->created' failed.

> 
> thanks
> -- PMM
>