[PATCH 00/22] exec/cpu: Call cpu_exec_realizefn() once in cpu_common_realize()

[PATCH 00/22] exec/cpu: Call cpu_exec_realizefn() once in cpu_common_realize()

[Philippe Mathieu-Daudé]

Hi,

TL;DR: This series factor duplicated common code in CPUs
DeviceRealize() handlers out, moving as a single call in
cpu_common_realize().

In an effort to have most of:
- CPU core code independant of accelerators
- CPU core code target agnostic
- CPU target code independant of accelerators
- and cpu_reset() called automatically without having to
  rely on global QEMUResetHandler,
I'm working on the core CPU code, unfortunately touching
files in all targets.

I suppose them term "exec" used in various areas of QEMU
started from what we call today "accel[erators]" [*]. So
cpu_exec_realizefn() can be read as cpu_accel_realize(),
or "generic code where an accelerator realizes its internal
fields on an abstract (target independent) CPU".

This series moves a common pattern used in all target's
cpu_realize() handlers to the common cpu_exec_realizefn().

Some optional code is used to check CPU requested features
are compatible with the accelerator possibilities. We
extracted this code as CPUClass::verify_accel_features()
handler. Better name welcomed :)

Some targets were calling cpu_reset() *before*
cpu_common_realizefn(), we moved it *after* (since RESET
shouldn't happen before REALIZE). I still have to audit
each target to confirm there are no side effects.
Besides this cpu_reset() change, the rest should be
relatively trivial to review, still I'd like feedback
from the respective target maintainers for the "move HW
creation after vCPU one" patches.

Regards,

Phil.

Follow-up: Make cpu_reset() accel-agnostic and move it
           to cpu_common_realize() (not trivial due to
           KVM run_on_cpu() calls).

[*] If Paolo/Richard confirm, I might post series renaming
    various APIs s/exec/accel/, because various headers
    meaning aren't clear to me.

Philippe Mathieu-Daudé (21):
  target/i386: Only realize existing APIC device
  hw/intc/apic: Pass CPU using QOM link property
  target/i386/kvm: Correct comment in kvm_cpu_realize()
  exec/cpu: Never call cpu_reset() before cpu_realize()
  exec/cpu: Call qemu_init_vcpu() once in cpu_common_realize()
  exec/cpu: Call cpu_remove_sync() once in cpu_common_unrealize()
  exec/cpu: RFC Destroy vCPU address spaces in cpu_common_unrealize()
  target/arm: Create timers *after* accelerator vCPU is realized
  target/hppa: Create timer *after* accelerator vCPU is realized
  target/nios2: Create IRQs *after* accelerator vCPU is realized
  target/mips: Create clock *after* accelerator vCPU is realized
  target/xtensa: Create IRQs *after* accelerator vCPU is realized
  target/sparc: Init CPU environment *after* accelerator vCPU is
    realized
  exec/cpu: Introduce CPUClass::verify_accel_features()
  target/arm: Extract verify_accel_features() from cpu_realize()
  target/i386: Extract verify_accel_features() from cpu_realize()
  target/s390x: Call s390_cpu_realize_sysemu from s390_realize_cpu_model
  target/s390x: Have s390_realize_cpu_model() return a boolean
  target/s390x: Use s390_realize_cpu_model() as verify_accel_features()
  exec/cpu: Have cpu_exec_realize() return a boolean
  exec/cpu: Call cpu_exec_realizefn() once in cpu_common_realize()

xianglai li (1):
  exec/cpu: Introduce the CPU address space destruction function

 include/exec/cpu-common.h     |  8 ++++
 include/hw/core/cpu.h         |  7 +++-
 target/s390x/s390x-internal.h |  4 +-
 cpu.c                         | 11 ++++-
 hw/core/cpu-common.c          | 15 +++++++
 hw/intc/apic_common.c         |  2 +
 softmmu/physmem.c             | 24 +++++++++++
 target/alpha/cpu.c            | 10 -----
 target/arm/cpu.c              | 53 ++++++++++++------------
 target/avr/cpu.c              | 10 +----
 target/cris/cpu.c             | 11 +----
 target/hexagon/cpu.c          | 11 +----
 target/hppa/cpu.c             | 20 ++-------
 target/i386/cpu-sysemu.c      | 20 ++++-----
 target/i386/cpu.c             | 77 ++++++++++++++++++-----------------
 target/i386/kvm/kvm-cpu.c     |  3 +-
 target/loongarch/cpu.c        | 11 +----
 target/m68k/cpu.c             | 11 +----
 target/microblaze/cpu.c       |  9 ----
 target/mips/cpu.c             | 27 ++++--------
 target/nios2/cpu.c            | 20 +++------
 target/openrisc/cpu.c         | 11 +----
 target/ppc/cpu_init.c         |  8 ----
 target/riscv/cpu.c            | 10 +----
 target/rx/cpu.c               | 11 +----
 target/s390x/cpu-sysemu.c     |  3 +-
 target/s390x/cpu.c            | 21 +---------
 target/s390x/cpu_models.c     | 16 +++++---
 target/sh4/cpu.c              | 11 +----
 target/sparc/cpu.c            | 10 -----
 target/tricore/cpu.c          | 10 +----
 target/xtensa/cpu.c           | 13 +-----
 32 files changed, 189 insertions(+), 299 deletions(-)

-- 
2.41.0

[PATCH 01/22] target/i386: Only realize existing APIC device

[Philippe Mathieu-Daudé]

APIC state is created under a certain condition,
use the same condition to realize it.
Having a NULL APIC state is a bug: use assert().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/i386/cpu-sysemu.c | 9 +++------
 target/i386/cpu.c        | 8 +++++---
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
index 2375e48178..6a164d3769 100644
--- a/target/i386/cpu-sysemu.c
+++ b/target/i386/cpu-sysemu.c
@@ -272,9 +272,7 @@ void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
     APICCommonState *apic;
     APICCommonClass *apic_class = apic_get_class(errp);
 
-    if (!apic_class) {
-        return;
-    }
+    assert(apic_class);
 
     cpu->apic_state = DEVICE(object_new_with_class(OBJECT_CLASS(apic_class)));
     object_property_add_child(OBJECT(cpu), "lapic",
@@ -293,9 +291,8 @@ void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
     APICCommonState *apic;
     static bool apic_mmio_map_once;
 
-    if (cpu->apic_state == NULL) {
-        return;
-    }
+    assert(cpu->apic_state);
+
     qdev_realize(DEVICE(cpu->apic_state), NULL, errp);
 
     /* Map APIC MMIO area */
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index b2a20365e1..a23d4795e0 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -7448,9 +7448,11 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 
 #ifndef CONFIG_USER_ONLY
-    x86_cpu_apic_realize(cpu, &local_err);
-    if (local_err != NULL) {
-        goto out;
+    if (cpu->env.features[FEAT_1_EDX] & CPUID_APIC || ms->smp.cpus > 1) {
+        x86_cpu_apic_realize(cpu, &local_err);
+        if (local_err != NULL) {
+            goto out;
+        }
     }
 #endif /* !CONFIG_USER_ONLY */
     cpu_reset(cs);
-- 
2.41.0

Re: [PATCH 01/22] target/i386: Only realize existing APIC device

[Richard Henderson]

On 9/18/23 09:02, Philippe Mathieu-Daudé wrote:
> APIC state is created under a certain condition,
> use the same condition to realize it.
> Having a NULL APIC state is a bug: use assert().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/i386/cpu-sysemu.c | 9 +++------
>   target/i386/cpu.c        | 8 +++++---
>   2 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
> index 2375e48178..6a164d3769 100644
> --- a/target/i386/cpu-sysemu.c
> +++ b/target/i386/cpu-sysemu.c
> @@ -272,9 +272,7 @@ void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
>       APICCommonState *apic;
>       APICCommonClass *apic_class = apic_get_class(errp);
>   
> -    if (!apic_class) {
> -        return;
> -    }
> +    assert(apic_class);

Ok.

>       cpu->apic_state = DEVICE(object_new_with_class(OBJECT_CLASS(apic_class)));
>       object_property_add_child(OBJECT(cpu), "lapic",
> @@ -293,9 +291,8 @@ void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
>       APICCommonState *apic;
>       static bool apic_mmio_map_once;
>   
> -    if (cpu->apic_state == NULL) {
> -        return;
> -    }
> +    assert(cpu->apic_state);
> +
>       qdev_realize(DEVICE(cpu->apic_state), NULL, errp);
>   
>       /* Map APIC MMIO area */
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index b2a20365e1..a23d4795e0 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -7448,9 +7448,11 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>       }
>   
>   #ifndef CONFIG_USER_ONLY
> -    x86_cpu_apic_realize(cpu, &local_err);
> -    if (local_err != NULL) {
> -        goto out;
> +    if (cpu->env.features[FEAT_1_EDX] & CPUID_APIC || ms->smp.cpus > 1) {
> +        x86_cpu_apic_realize(cpu, &local_err);
> +        if (local_err != NULL) {
> +            goto out;
> +        }

I'm not keen on the replication of the condition.  Testing whether the apic object was 
created seems reasonable.  It probably is clearer with the IF in the caller, as you do.


r~

Re: [PATCH 01/22] target/i386: Only realize existing APIC device

[Igor Mammedov]

On Mon, 18 Sep 2023 18:02:34 +0200
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> APIC state is created under a certain condition,
> use the same condition to realize it.
> Having a NULL APIC state is a bug: use assert().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  target/i386/cpu-sysemu.c | 9 +++------
>  target/i386/cpu.c        | 8 +++++---
>  2 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
> index 2375e48178..6a164d3769 100644
> --- a/target/i386/cpu-sysemu.c
> +++ b/target/i386/cpu-sysemu.c
> @@ -272,9 +272,7 @@ void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
>      APICCommonState *apic;
>      APICCommonClass *apic_class = apic_get_class(errp);
>  
> -    if (!apic_class) {
> -        return;
> -    }
> +    assert(apic_class);

if errp doesn't lead to error_fatal/abort, wouldn't that effectively mask
following error
 apic_get_class():
      error_setg(errp, "KVM does not support userspace APIC");
      return NULL;
???

>  
>      cpu->apic_state = DEVICE(object_new_with_class(OBJECT_CLASS(apic_class)));
>      object_property_add_child(OBJECT(cpu), "lapic",
> @@ -293,9 +291,8 @@ void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
>      APICCommonState *apic;
>      static bool apic_mmio_map_once;
>  
> -    if (cpu->apic_state == NULL) {
> -        return;
> -    }
> +    assert(cpu->apic_state);

it would be better to explode in one place only inside apic_get_class(),
provided !kvm_irqchip_in_kernel() case is dealt with properly.


>      qdev_realize(DEVICE(cpu->apic_state), NULL, errp);
>  
>      /* Map APIC MMIO area */
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index b2a20365e1..a23d4795e0 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -7448,9 +7448,11 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>      }
>  
>  #ifndef CONFIG_USER_ONLY
> -    x86_cpu_apic_realize(cpu, &local_err);
> -    if (local_err != NULL) {
> -        goto out;
> +    if (cpu->env.features[FEAT_1_EDX] & CPUID_APIC || ms->smp.cpus > 1) {
> +        x86_cpu_apic_realize(cpu, &local_err);
> +        if (local_err != NULL) {
> +            goto out;
> +        }

better move 'if (cpu->apic_state == NULL) {' from x86_cpu_apic_realize()
to the caller, instead of repeating test again.

>      }
>  #endif /* !CONFIG_USER_ONLY */
>      cpu_reset(cs);

[PATCH 02/22] hw/intc/apic: Pass CPU using QOM link property

[Philippe Mathieu-Daudé]

QOM objects shouldn't access each other internals fields
except using the QOM API.

Declare the 'cpu' and 'base-addr' properties, set them
using object_property_set_link() and qdev_prop_set_uint32()
respectively.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/intc/apic_common.c    |  2 ++
 target/i386/cpu-sysemu.c | 11 ++++++-----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index 68ad30e2f5..e28f7402ab 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -394,6 +394,8 @@ static Property apic_properties_common[] = {
                     true),
     DEFINE_PROP_BOOL("legacy-instance-id", APICCommonState, legacy_instance_id,
                      false),
+    DEFINE_PROP_LINK("cpu", APICCommonState, cpu, TYPE_X86_CPU, X86CPU *),
+    DEFINE_PROP_UINT32("base-addr", APICCommonState, apicbase, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
index 6a164d3769..6edfb7e2af 100644
--- a/target/i386/cpu-sysemu.c
+++ b/target/i386/cpu-sysemu.c
@@ -269,7 +269,6 @@ APICCommonClass *apic_get_class(Error **errp)
 
 void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
 {
-    APICCommonState *apic;
     APICCommonClass *apic_class = apic_get_class(errp);
 
     assert(apic_class);
@@ -279,11 +278,13 @@ void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
                               OBJECT(cpu->apic_state));
     object_unref(OBJECT(cpu->apic_state));
 
+    if (!object_property_set_link(OBJECT(cpu->apic_state), "cpu",
+                                  OBJECT(cpu), errp)) {
+        return;
+    }
     qdev_prop_set_uint32(cpu->apic_state, "id", cpu->apic_id);
-    /* TODO: convert to link<> */
-    apic = APIC_COMMON(cpu->apic_state);
-    apic->cpu = cpu;
-    apic->apicbase = APIC_DEFAULT_ADDRESS | MSR_IA32_APICBASE_ENABLE;
+    qdev_prop_set_uint32(cpu->apic_state, "base-addr",
+                         APIC_DEFAULT_ADDRESS | MSR_IA32_APICBASE_ENABLE);
 }
 
 void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
-- 
2.41.0

Re: [PATCH 02/22] hw/intc/apic: Pass CPU using QOM link property

[Richard Henderson]

On 9/18/23 09:02, Philippe Mathieu-Daudé wrote:
> QOM objects shouldn't access each other internals fields
> except using the QOM API.
> 
> Declare the 'cpu' and 'base-addr' properties, set them
> using object_property_set_link() and qdev_prop_set_uint32()
> respectively.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/intc/apic_common.c    |  2 ++
>   target/i386/cpu-sysemu.c | 11 ++++++-----
>   2 files changed, 8 insertions(+), 5 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 02/22] hw/intc/apic: Pass CPU using QOM link property

[Igor Mammedov]

On Mon, 18 Sep 2023 18:02:35 +0200
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> QOM objects shouldn't access each other internals fields
> except using the QOM API.
> 
> Declare the 'cpu' and 'base-addr' properties, set them
> using object_property_set_link() and qdev_prop_set_uint32()
> respectively.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/intc/apic_common.c    |  2 ++
>  target/i386/cpu-sysemu.c | 11 ++++++-----
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> index 68ad30e2f5..e28f7402ab 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -394,6 +394,8 @@ static Property apic_properties_common[] = {
>                      true),
>      DEFINE_PROP_BOOL("legacy-instance-id", APICCommonState, legacy_instance_id,
>                       false),
> +    DEFINE_PROP_LINK("cpu", APICCommonState, cpu, TYPE_X86_CPU, X86CPU *),
> +    DEFINE_PROP_UINT32("base-addr", APICCommonState, apicbase, 0),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
> index 6a164d3769..6edfb7e2af 100644
> --- a/target/i386/cpu-sysemu.c
> +++ b/target/i386/cpu-sysemu.c
> @@ -269,7 +269,6 @@ APICCommonClass *apic_get_class(Error **errp)
>  
>  void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
>  {
> -    APICCommonState *apic;
>      APICCommonClass *apic_class = apic_get_class(errp);
>  
>      assert(apic_class);
> @@ -279,11 +278,13 @@ void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
>                                OBJECT(cpu->apic_state));
>      object_unref(OBJECT(cpu->apic_state));
>  
> +    if (!object_property_set_link(OBJECT(cpu->apic_state), "cpu",
> +                                  OBJECT(cpu), errp)) {
> +        return;
> +    }
>      qdev_prop_set_uint32(cpu->apic_state, "id", cpu->apic_id);
> -    /* TODO: convert to link<> */
> -    apic = APIC_COMMON(cpu->apic_state);
> -    apic->cpu = cpu;
> -    apic->apicbase = APIC_DEFAULT_ADDRESS | MSR_IA32_APICBASE_ENABLE;
> +    qdev_prop_set_uint32(cpu->apic_state, "base-addr",
> +                         APIC_DEFAULT_ADDRESS | MSR_IA32_APICBASE_ENABLE);
>  }
>  
>  void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)

[PATCH 03/22] target/i386/kvm: Correct comment in kvm_cpu_realize()

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/i386/kvm/kvm-cpu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
index 7237378a7d..1fe62ce176 100644
--- a/target/i386/kvm/kvm-cpu.c
+++ b/target/i386/kvm/kvm-cpu.c
@@ -37,6 +37,7 @@ static bool kvm_cpu_realizefn(CPUState *cs, Error **errp)
      *  -> cpu_exec_realizefn():
      *            -> accel_cpu_realizefn()
      *               kvm_cpu_realizefn() -> host_cpu_realizefn()
+     *  -> cpu_common_realizefn()
      *  -> check/update ucode_rev, phys_bits, mwait
      */
     if (cpu->max_features) {
-- 
2.41.0

Re: [PATCH 03/22] target/i386/kvm: Correct comment in kvm_cpu_realize()

[Richard Henderson]

On 9/18/23 09:02, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   target/i386/kvm/kvm-cpu.c | 1 +
>   1 file changed, 1 insertion(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 03/22] target/i386/kvm: Correct comment in kvm_cpu_realize()

[Igor Mammedov]

On Mon, 18 Sep 2023 18:02:36 +0200
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  target/i386/kvm/kvm-cpu.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
> index 7237378a7d..1fe62ce176 100644
> --- a/target/i386/kvm/kvm-cpu.c
> +++ b/target/i386/kvm/kvm-cpu.c
> @@ -37,6 +37,7 @@ static bool kvm_cpu_realizefn(CPUState *cs, Error **errp)
>       *  -> cpu_exec_realizefn():
>       *            -> accel_cpu_realizefn()
>       *               kvm_cpu_realizefn() -> host_cpu_realizefn()
> +     *  -> cpu_common_realizefn()
>       *  -> check/update ucode_rev, phys_bits, mwait
>       */
>      if (cpu->max_features) {

[RFC PATCH 04/22] exec/cpu: Never call cpu_reset() before cpu_realize()

[Philippe Mathieu-Daudé]

QDev instance is expected to be in an unknown state until full
object realization. Thus we shouldn't call DeviceReset() on an
unrealized instance. Move the cpu_reset() call from *before*
the parent realize() handler (effectively cpu_common_realizefn)
to *after* it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
RFC:
I haven't audited all the call sites, but plan to do it,
amending the result to this patch description. This used
to be a problem on some targets, but as of today's master
this series pass make check-{qtest,avocado}.
---
 target/arm/cpu.c       | 2 +-
 target/avr/cpu.c       | 2 +-
 target/cris/cpu.c      | 2 +-
 target/hexagon/cpu.c   | 3 +--
 target/i386/cpu.c      | 2 +-
 target/loongarch/cpu.c | 2 +-
 target/m68k/cpu.c      | 2 +-
 target/mips/cpu.c      | 2 +-
 target/nios2/cpu.c     | 2 +-
 target/openrisc/cpu.c  | 2 +-
 target/riscv/cpu.c     | 2 +-
 target/rx/cpu.c        | 2 +-
 target/s390x/cpu.c     | 2 +-
 target/sh4/cpu.c       | 2 +-
 target/tricore/cpu.c   | 2 +-
 15 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index b9e09a702d..6aca036b85 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2278,9 +2278,9 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 
     qemu_init_vcpu(cs);
-    cpu_reset(cs);
 
     acc->parent_realize(dev, errp);
+    cpu_reset(cs);
 }
 
 static ObjectClass *arm_cpu_class_by_name(const char *cpu_model)
diff --git a/target/avr/cpu.c b/target/avr/cpu.c
index 8f741f258c..84d353f30e 100644
--- a/target/avr/cpu.c
+++ b/target/avr/cpu.c
@@ -120,9 +120,9 @@ static void avr_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
     qemu_init_vcpu(cs);
-    cpu_reset(cs);
 
     mcc->parent_realize(dev, errp);
+    cpu_reset(cs);
 }
 
 static void avr_cpu_set_int(void *opaque, int irq, int level)
diff --git a/target/cris/cpu.c b/target/cris/cpu.c
index a6a93c2359..079872a5cc 100644
--- a/target/cris/cpu.c
+++ b/target/cris/cpu.c
@@ -152,10 +152,10 @@ static void cris_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    cpu_reset(cs);
     qemu_init_vcpu(cs);
 
     ccc->parent_realize(dev, errp);
+    cpu_reset(cs);
 }
 
 #ifndef CONFIG_USER_ONLY
diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c
index f155936289..7edc32701f 100644
--- a/target/hexagon/cpu.c
+++ b/target/hexagon/cpu.c
@@ -346,9 +346,8 @@ static void hexagon_cpu_realize(DeviceState *dev, Error **errp)
                              "hexagon-hvx.xml", 0);
 
     qemu_init_vcpu(cs);
-    cpu_reset(cs);
-
     mcc->parent_realize(dev, errp);
+    cpu_reset(cs);
 }
 
 static void hexagon_cpu_init(Object *obj)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index a23d4795e0..7faaa6915f 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -7455,9 +7455,9 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
         }
     }
 #endif /* !CONFIG_USER_ONLY */
-    cpu_reset(cs);
 
     xcc->parent_realize(dev, &local_err);
+    cpu_reset(cs);
 
 out:
     if (local_err != NULL) {
diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
index 65f9320e34..8029e70e76 100644
--- a/target/loongarch/cpu.c
+++ b/target/loongarch/cpu.c
@@ -565,10 +565,10 @@ static void loongarch_cpu_realizefn(DeviceState *dev, Error **errp)
 
     loongarch_cpu_register_gdb_regs_for_features(cs);
 
-    cpu_reset(cs);
     qemu_init_vcpu(cs);
 
     lacc->parent_realize(dev, errp);
+    cpu_reset(cs);
 }
 
 #ifndef CONFIG_USER_ONLY
diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index 70d58471dc..2bc0a62f0e 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -321,10 +321,10 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error **errp)
 
     m68k_cpu_init_gdb(cpu);
 
-    cpu_reset(cs);
     qemu_init_vcpu(cs);
 
     mcc->parent_realize(dev, errp);
+    cpu_reset(cs);
 }
 
 static void m68k_cpu_initfn(Object *obj)
diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index 63da1948fd..8d6f633f72 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -492,10 +492,10 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
     fpu_init(env, env->cpu_model);
     mvp_init(env);
 
-    cpu_reset(cs);
     qemu_init_vcpu(cs);
 
     mcc->parent_realize(dev, errp);
+    cpu_reset(cs);
 }
 
 static void mips_cpu_initfn(Object *obj)
diff --git a/target/nios2/cpu.c b/target/nios2/cpu.c
index bc5cbf81c2..876a6dcad2 100644
--- a/target/nios2/cpu.c
+++ b/target/nios2/cpu.c
@@ -217,12 +217,12 @@ static void nios2_cpu_realizefn(DeviceState *dev, Error **errp)
 
     realize_cr_status(cs);
     qemu_init_vcpu(cs);
-    cpu_reset(cs);
 
     /* We have reserved storage for cpuid; might as well use it. */
     cpu->env.ctrl[CR_CPUID] = cs->cpu_index;
 
     ncc->parent_realize(dev, errp);
+    cpu_reset(cs);
 }
 
 #ifndef CONFIG_USER_ONLY
diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c
index 61d748cfdc..cd25f1e9d5 100644
--- a/target/openrisc/cpu.c
+++ b/target/openrisc/cpu.c
@@ -142,9 +142,9 @@ static void openrisc_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 
     qemu_init_vcpu(cs);
-    cpu_reset(cs);
 
     occ->parent_realize(dev, errp);
+    cpu_reset(cs);
 }
 
 static void openrisc_cpu_initfn(Object *obj)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index f227c7664e..7566757346 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1532,9 +1532,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
 #endif
 
     qemu_init_vcpu(cs);
-    cpu_reset(cs);
 
     mcc->parent_realize(dev, errp);
+    cpu_reset(cs);
 }
 
 #ifndef CONFIG_USER_ONLY
diff --git a/target/rx/cpu.c b/target/rx/cpu.c
index 157e57da0f..c9c8443cbd 100644
--- a/target/rx/cpu.c
+++ b/target/rx/cpu.c
@@ -138,9 +138,9 @@ static void rx_cpu_realize(DeviceState *dev, Error **errp)
     }
 
     qemu_init_vcpu(cs);
-    cpu_reset(cs);
 
     rcc->parent_realize(dev, errp);
+    cpu_reset(cs);
 }
 
 static void rx_cpu_set_irq(void *opaque, int no, int request)
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index df167493c3..0f0b11fd73 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -254,6 +254,7 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
     s390_cpu_gdb_init(cs);
     qemu_init_vcpu(cs);
 
+    scc->parent_realize(dev, &err);
     /*
      * KVM requires the initial CPU reset ioctl to be executed on the target
      * CPU thread. CPU hotplug under single-threaded TCG will not work with
@@ -266,7 +267,6 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
         cpu_reset(cs);
     }
 
-    scc->parent_realize(dev, &err);
 out:
     error_propagate(errp, err);
 }
diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c
index 61769ffdfa..656d71f74a 100644
--- a/target/sh4/cpu.c
+++ b/target/sh4/cpu.c
@@ -228,10 +228,10 @@ static void superh_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    cpu_reset(cs);
     qemu_init_vcpu(cs);
 
     scc->parent_realize(dev, errp);
+    cpu_reset(cs);
 }
 
 static void superh_cpu_initfn(Object *obj)
diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c
index 133a9ac70e..a3610aecca 100644
--- a/target/tricore/cpu.c
+++ b/target/tricore/cpu.c
@@ -118,10 +118,10 @@ static void tricore_cpu_realizefn(DeviceState *dev, Error **errp)
     if (tricore_has_feature(env, TRICORE_FEATURE_131)) {
         set_feature(env, TRICORE_FEATURE_13);
     }
-    cpu_reset(cs);
     qemu_init_vcpu(cs);
 
     tcc->parent_realize(dev, errp);
+    cpu_reset(cs);
 }
 
 
-- 
2.41.0

Re: [RFC PATCH 04/22] exec/cpu: Never call cpu_reset() before cpu_realize()

[Richard Henderson]

On 9/18/23 09:02, Philippe Mathieu-Daudé wrote:
> QDev instance is expected to be in an unknown state until full
> object realization. Thus we shouldn't call DeviceReset() on an
> unrealized instance. Move the cpu_reset() call from*before*
> the parent realize() handler (effectively cpu_common_realizefn)
> to*after*  it.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
> RFC:
> I haven't audited all the call sites, but plan to do it,
> amending the result to this patch description. This used
> to be a problem on some targets, but as of today's master
> this series pass make check-{qtest,avocado}.
> ---
>   target/arm/cpu.c       | 2 +-
>   target/avr/cpu.c       | 2 +-
>   target/cris/cpu.c      | 2 +-
>   target/hexagon/cpu.c   | 3 +--
>   target/i386/cpu.c      | 2 +-
>   target/loongarch/cpu.c | 2 +-
>   target/m68k/cpu.c      | 2 +-
>   target/mips/cpu.c      | 2 +-
>   target/nios2/cpu.c     | 2 +-
>   target/openrisc/cpu.c  | 2 +-
>   target/riscv/cpu.c     | 2 +-
>   target/rx/cpu.c        | 2 +-
>   target/s390x/cpu.c     | 2 +-
>   target/sh4/cpu.c       | 2 +-
>   target/tricore/cpu.c   | 2 +-
>   15 files changed, 15 insertions(+), 16 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [RFC PATCH 04/22] exec/cpu: Never call cpu_reset() before cpu_realize()

[Igor Mammedov]

On Mon, 18 Sep 2023 18:02:37 +0200
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> QDev instance is expected to be in an unknown state until full
> object realization. Thus we shouldn't call DeviceReset() on an
> unrealized instance. Move the cpu_reset() call from *before*
> the parent realize() handler (effectively cpu_common_realizefn)
> to *after* it.

looking at:
 
 --cpu_reset()  <- brings cpu to known (after reset|poweron) state
   cpu_common_realizefn()
       if (dev->hotplugged) {                                                       
           cpu_synchronize_post_init(cpu);                                          
           cpu_resume(cpu);                                                         
       }
 ++cpu_reset()  <-- looks to be too late, we already told hypervisor to run undefined CPU, didn't we?



> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> RFC:
> I haven't audited all the call sites, but plan to do it,
> amending the result to this patch description. This used
> to be a problem on some targets, but as of today's master
> this series pass make check-{qtest,avocado}.
> ---
>  target/arm/cpu.c       | 2 +-
>  target/avr/cpu.c       | 2 +-
>  target/cris/cpu.c      | 2 +-
>  target/hexagon/cpu.c   | 3 +--
>  target/i386/cpu.c      | 2 +-
>  target/loongarch/cpu.c | 2 +-
>  target/m68k/cpu.c      | 2 +-
>  target/mips/cpu.c      | 2 +-
>  target/nios2/cpu.c     | 2 +-
>  target/openrisc/cpu.c  | 2 +-
>  target/riscv/cpu.c     | 2 +-
>  target/rx/cpu.c        | 2 +-
>  target/s390x/cpu.c     | 2 +-
>  target/sh4/cpu.c       | 2 +-
>  target/tricore/cpu.c   | 2 +-
>  15 files changed, 15 insertions(+), 16 deletions(-)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index b9e09a702d..6aca036b85 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -2278,9 +2278,9 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>      }
>  
>      qemu_init_vcpu(cs);
> -    cpu_reset(cs);
>  
>      acc->parent_realize(dev, errp);
> +    cpu_reset(cs);
>  }
>  
>  static ObjectClass *arm_cpu_class_by_name(const char *cpu_model)
> diff --git a/target/avr/cpu.c b/target/avr/cpu.c
> index 8f741f258c..84d353f30e 100644
> --- a/target/avr/cpu.c
> +++ b/target/avr/cpu.c
> @@ -120,9 +120,9 @@ static void avr_cpu_realizefn(DeviceState *dev, Error **errp)
>          return;
>      }
>      qemu_init_vcpu(cs);
> -    cpu_reset(cs);
>  
>      mcc->parent_realize(dev, errp);
> +    cpu_reset(cs);
>  }
>  
>  static void avr_cpu_set_int(void *opaque, int irq, int level)
> diff --git a/target/cris/cpu.c b/target/cris/cpu.c
> index a6a93c2359..079872a5cc 100644
> --- a/target/cris/cpu.c
> +++ b/target/cris/cpu.c
> @@ -152,10 +152,10 @@ static void cris_cpu_realizefn(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    cpu_reset(cs);
>      qemu_init_vcpu(cs);
>  
>      ccc->parent_realize(dev, errp);
> +    cpu_reset(cs);
>  }
>  
>  #ifndef CONFIG_USER_ONLY
> diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c
> index f155936289..7edc32701f 100644
> --- a/target/hexagon/cpu.c
> +++ b/target/hexagon/cpu.c
> @@ -346,9 +346,8 @@ static void hexagon_cpu_realize(DeviceState *dev, Error **errp)
>                               "hexagon-hvx.xml", 0);
>  
>      qemu_init_vcpu(cs);
> -    cpu_reset(cs);
> -
>      mcc->parent_realize(dev, errp);
> +    cpu_reset(cs);
>  }
>  
>  static void hexagon_cpu_init(Object *obj)
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index a23d4795e0..7faaa6915f 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -7455,9 +7455,9 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>          }
>      }
>  #endif /* !CONFIG_USER_ONLY */
> -    cpu_reset(cs);
>  
>      xcc->parent_realize(dev, &local_err);
> +    cpu_reset(cs);
>  
>  out:
>      if (local_err != NULL) {
> diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
> index 65f9320e34..8029e70e76 100644
> --- a/target/loongarch/cpu.c
> +++ b/target/loongarch/cpu.c
> @@ -565,10 +565,10 @@ static void loongarch_cpu_realizefn(DeviceState *dev, Error **errp)
>  
>      loongarch_cpu_register_gdb_regs_for_features(cs);
>  
> -    cpu_reset(cs);
>      qemu_init_vcpu(cs);
>  
>      lacc->parent_realize(dev, errp);
> +    cpu_reset(cs);
>  }
>  
>  #ifndef CONFIG_USER_ONLY
> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
> index 70d58471dc..2bc0a62f0e 100644
> --- a/target/m68k/cpu.c
> +++ b/target/m68k/cpu.c
> @@ -321,10 +321,10 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error **errp)
>  
>      m68k_cpu_init_gdb(cpu);
>  
> -    cpu_reset(cs);
>      qemu_init_vcpu(cs);
>  
>      mcc->parent_realize(dev, errp);
> +    cpu_reset(cs);
>  }
>  
>  static void m68k_cpu_initfn(Object *obj)
> diff --git a/target/mips/cpu.c b/target/mips/cpu.c
> index 63da1948fd..8d6f633f72 100644
> --- a/target/mips/cpu.c
> +++ b/target/mips/cpu.c
> @@ -492,10 +492,10 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
>      fpu_init(env, env->cpu_model);
>      mvp_init(env);
>  
> -    cpu_reset(cs);
>      qemu_init_vcpu(cs);
>  
>      mcc->parent_realize(dev, errp);
> +    cpu_reset(cs);
>  }
>  
>  static void mips_cpu_initfn(Object *obj)
> diff --git a/target/nios2/cpu.c b/target/nios2/cpu.c
> index bc5cbf81c2..876a6dcad2 100644
> --- a/target/nios2/cpu.c
> +++ b/target/nios2/cpu.c
> @@ -217,12 +217,12 @@ static void nios2_cpu_realizefn(DeviceState *dev, Error **errp)
>  
>      realize_cr_status(cs);
>      qemu_init_vcpu(cs);
> -    cpu_reset(cs);
>  
>      /* We have reserved storage for cpuid; might as well use it. */
>      cpu->env.ctrl[CR_CPUID] = cs->cpu_index;
>  
>      ncc->parent_realize(dev, errp);
> +    cpu_reset(cs);
>  }
>  
>  #ifndef CONFIG_USER_ONLY
> diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c
> index 61d748cfdc..cd25f1e9d5 100644
> --- a/target/openrisc/cpu.c
> +++ b/target/openrisc/cpu.c
> @@ -142,9 +142,9 @@ static void openrisc_cpu_realizefn(DeviceState *dev, Error **errp)
>      }
>  
>      qemu_init_vcpu(cs);
> -    cpu_reset(cs);
>  
>      occ->parent_realize(dev, errp);
> +    cpu_reset(cs);
>  }
>  
>  static void openrisc_cpu_initfn(Object *obj)
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index f227c7664e..7566757346 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1532,9 +1532,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
>  #endif
>  
>      qemu_init_vcpu(cs);
> -    cpu_reset(cs);
>  
>      mcc->parent_realize(dev, errp);
> +    cpu_reset(cs);
>  }
>  
>  #ifndef CONFIG_USER_ONLY
> diff --git a/target/rx/cpu.c b/target/rx/cpu.c
> index 157e57da0f..c9c8443cbd 100644
> --- a/target/rx/cpu.c
> +++ b/target/rx/cpu.c
> @@ -138,9 +138,9 @@ static void rx_cpu_realize(DeviceState *dev, Error **errp)
>      }
>  
>      qemu_init_vcpu(cs);
> -    cpu_reset(cs);
>  
>      rcc->parent_realize(dev, errp);
> +    cpu_reset(cs);
>  }
>  
>  static void rx_cpu_set_irq(void *opaque, int no, int request)
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index df167493c3..0f0b11fd73 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -254,6 +254,7 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>      s390_cpu_gdb_init(cs);
>      qemu_init_vcpu(cs);
>  
> +    scc->parent_realize(dev, &err);
>      /*
>       * KVM requires the initial CPU reset ioctl to be executed on the target
>       * CPU thread. CPU hotplug under single-threaded TCG will not work with
> @@ -266,7 +267,6 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>          cpu_reset(cs);
>      }
>  
> -    scc->parent_realize(dev, &err);
>  out:
>      error_propagate(errp, err);
>  }
> diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c
> index 61769ffdfa..656d71f74a 100644
> --- a/target/sh4/cpu.c
> +++ b/target/sh4/cpu.c
> @@ -228,10 +228,10 @@ static void superh_cpu_realizefn(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    cpu_reset(cs);
>      qemu_init_vcpu(cs);
>  
>      scc->parent_realize(dev, errp);
> +    cpu_reset(cs);
>  }
>  
>  static void superh_cpu_initfn(Object *obj)
> diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c
> index 133a9ac70e..a3610aecca 100644
> --- a/target/tricore/cpu.c
> +++ b/target/tricore/cpu.c
> @@ -118,10 +118,10 @@ static void tricore_cpu_realizefn(DeviceState *dev, Error **errp)
>      if (tricore_has_feature(env, TRICORE_FEATURE_131)) {
>          set_feature(env, TRICORE_FEATURE_13);
>      }
> -    cpu_reset(cs);
>      qemu_init_vcpu(cs);
>  
>      tcc->parent_realize(dev, errp);
> +    cpu_reset(cs);
>  }
>  
>  

[PATCH 05/22] exec/cpu: Call qemu_init_vcpu() once in cpu_common_realize()

[Philippe Mathieu-Daudé]

qemu_init_vcpu() is called in each ${target}_cpu_realize() before
the call to parent_realize(), which is cpu_common_realizefn().
Call it once there.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/core/cpu-common.c    | 3 +++
 target/alpha/cpu.c      | 2 --
 target/arm/cpu.c        | 2 --
 target/avr/cpu.c        | 1 -
 target/cris/cpu.c       | 2 --
 target/hexagon/cpu.c    | 1 -
 target/hppa/cpu.c       | 1 -
 target/i386/cpu.c       | 4 +---
 target/loongarch/cpu.c  | 2 --
 target/m68k/cpu.c       | 2 --
 target/microblaze/cpu.c | 2 --
 target/mips/cpu.c       | 2 --
 target/nios2/cpu.c      | 1 -
 target/openrisc/cpu.c   | 2 --
 target/ppc/cpu_init.c   | 1 -
 target/riscv/cpu.c      | 2 --
 target/rx/cpu.c         | 2 --
 target/s390x/cpu.c      | 1 -
 target/sh4/cpu.c        | 2 --
 target/sparc/cpu.c      | 2 --
 target/tricore/cpu.c    | 1 -
 target/xtensa/cpu.c     | 2 --
 22 files changed, 4 insertions(+), 36 deletions(-)

diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index ced66c2b34..a3b8de7054 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -204,6 +204,9 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp)
         }
     }
 
+    /* Create CPU address space and vCPU thread */
+    qemu_init_vcpu(cpu);
+
     if (dev->hotplugged) {
         cpu_synchronize_post_init(cpu);
         cpu_resume(cpu);
diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
index 270ae787b1..eb78318bb8 100644
--- a/target/alpha/cpu.c
+++ b/target/alpha/cpu.c
@@ -82,8 +82,6 @@ static void alpha_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
-
     acc->parent_realize(dev, errp);
 }
 
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 6aca036b85..fc3772025c 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2277,8 +2277,6 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
         }
     }
 
-    qemu_init_vcpu(cs);
-
     acc->parent_realize(dev, errp);
     cpu_reset(cs);
 }
diff --git a/target/avr/cpu.c b/target/avr/cpu.c
index 84d353f30e..d3460b3960 100644
--- a/target/avr/cpu.c
+++ b/target/avr/cpu.c
@@ -119,7 +119,6 @@ static void avr_cpu_realizefn(DeviceState *dev, Error **errp)
         error_propagate(errp, local_err);
         return;
     }
-    qemu_init_vcpu(cs);
 
     mcc->parent_realize(dev, errp);
     cpu_reset(cs);
diff --git a/target/cris/cpu.c b/target/cris/cpu.c
index 079872a5cc..671693a362 100644
--- a/target/cris/cpu.c
+++ b/target/cris/cpu.c
@@ -152,8 +152,6 @@ static void cris_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
-
     ccc->parent_realize(dev, errp);
     cpu_reset(cs);
 }
diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c
index 7edc32701f..5b9bb3fe83 100644
--- a/target/hexagon/cpu.c
+++ b/target/hexagon/cpu.c
@@ -345,7 +345,6 @@ static void hexagon_cpu_realize(DeviceState *dev, Error **errp)
                              NUM_VREGS + NUM_QREGS,
                              "hexagon-hvx.xml", 0);
 
-    qemu_init_vcpu(cs);
     mcc->parent_realize(dev, errp);
     cpu_reset(cs);
 }
diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c
index 11022f9c99..49082bd2ba 100644
--- a/target/hppa/cpu.c
+++ b/target/hppa/cpu.c
@@ -131,7 +131,6 @@ static void hppa_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
     acc->parent_realize(dev, errp);
 
 #ifndef CONFIG_USER_ONLY
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 7faaa6915f..cb41d30aab 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -7425,15 +7425,13 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 
     mce_init(cpu);
 
-    qemu_init_vcpu(cs);
-
     /*
      * Most Intel and certain AMD CPUs support hyperthreading. Even though QEMU
      * fixes this issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX
      * based on inputs (sockets,cores,threads), it is still better to give
      * users a warning.
      *
-     * NOTE: the following code has to follow qemu_init_vcpu(). Otherwise
+     * NOTE: the following code has to follow cpu_common_realize(). Otherwise
      * cs->nr_threads hasn't be populated yet and the checking is incorrect.
      */
     if (IS_AMD_CPU(env) &&
diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
index 8029e70e76..dc0ac39833 100644
--- a/target/loongarch/cpu.c
+++ b/target/loongarch/cpu.c
@@ -565,8 +565,6 @@ static void loongarch_cpu_realizefn(DeviceState *dev, Error **errp)
 
     loongarch_cpu_register_gdb_regs_for_features(cs);
 
-    qemu_init_vcpu(cs);
-
     lacc->parent_realize(dev, errp);
     cpu_reset(cs);
 }
diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index 2bc0a62f0e..3da316bc30 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -321,8 +321,6 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error **errp)
 
     m68k_cpu_init_gdb(cpu);
 
-    qemu_init_vcpu(cs);
-
     mcc->parent_realize(dev, errp);
     cpu_reset(cs);
 }
diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c
index 03c2c4db1f..1f19a6e07d 100644
--- a/target/microblaze/cpu.c
+++ b/target/microblaze/cpu.c
@@ -221,8 +221,6 @@ static void mb_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
-
     version = cpu->cfg.version ? cpu->cfg.version : DEFAULT_CPU_VERSION;
     for (i = 0; mb_cpu_lookup[i].name && version; i++) {
         if (strcmp(mb_cpu_lookup[i].name, version) == 0) {
diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index 8d6f633f72..0aea69aaf9 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -492,8 +492,6 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
     fpu_init(env, env->cpu_model);
     mvp_init(env);
 
-    qemu_init_vcpu(cs);
-
     mcc->parent_realize(dev, errp);
     cpu_reset(cs);
 }
diff --git a/target/nios2/cpu.c b/target/nios2/cpu.c
index 876a6dcad2..7a92fc5f2c 100644
--- a/target/nios2/cpu.c
+++ b/target/nios2/cpu.c
@@ -216,7 +216,6 @@ static void nios2_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 
     realize_cr_status(cs);
-    qemu_init_vcpu(cs);
 
     /* We have reserved storage for cpuid; might as well use it. */
     cpu->env.ctrl[CR_CPUID] = cs->cpu_index;
diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c
index cd25f1e9d5..e4ec95ca7f 100644
--- a/target/openrisc/cpu.c
+++ b/target/openrisc/cpu.c
@@ -141,8 +141,6 @@ static void openrisc_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
-
     occ->parent_realize(dev, errp);
     cpu_reset(cs);
 }
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 7ab5ee92d9..e2c06c1f32 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -6833,7 +6833,6 @@ static void ppc_cpu_realize(DeviceState *dev, Error **errp)
     init_ppc_proc(cpu);
 
     ppc_gdb_init(cs, pcc);
-    qemu_init_vcpu(cs);
 
     pcc->parent_realize(dev, errp);
 
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 7566757346..4f7ae55359 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1531,8 +1531,6 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
     }
 #endif
 
-    qemu_init_vcpu(cs);
-
     mcc->parent_realize(dev, errp);
     cpu_reset(cs);
 }
diff --git a/target/rx/cpu.c b/target/rx/cpu.c
index c9c8443cbd..089df61790 100644
--- a/target/rx/cpu.c
+++ b/target/rx/cpu.c
@@ -137,8 +137,6 @@ static void rx_cpu_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
-
     rcc->parent_realize(dev, errp);
     cpu_reset(cs);
 }
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 0f0b11fd73..416ac6c4e0 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -252,7 +252,6 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
     qemu_register_reset(s390_cpu_machine_reset_cb, S390_CPU(dev));
 #endif
     s390_cpu_gdb_init(cs);
-    qemu_init_vcpu(cs);
 
     scc->parent_realize(dev, &err);
     /*
diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c
index 656d71f74a..e6690daf9a 100644
--- a/target/sh4/cpu.c
+++ b/target/sh4/cpu.c
@@ -228,8 +228,6 @@ static void superh_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
-
     scc->parent_realize(dev, errp);
     cpu_reset(cs);
 }
diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
index 130ab8f578..2fdc95eda9 100644
--- a/target/sparc/cpu.c
+++ b/target/sparc/cpu.c
@@ -782,8 +782,6 @@ static void sparc_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    qemu_init_vcpu(cs);
-
     scc->parent_realize(dev, errp);
 }
 
diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c
index a3610aecca..0142cf556d 100644
--- a/target/tricore/cpu.c
+++ b/target/tricore/cpu.c
@@ -118,7 +118,6 @@ static void tricore_cpu_realizefn(DeviceState *dev, Error **errp)
     if (tricore_has_feature(env, TRICORE_FEATURE_131)) {
         set_feature(env, TRICORE_FEATURE_13);
     }
-    qemu_init_vcpu(cs);
 
     tcc->parent_realize(dev, errp);
     cpu_reset(cs);
diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
index acaf8c905f..300d19d45c 100644
--- a/target/xtensa/cpu.c
+++ b/target/xtensa/cpu.c
@@ -174,8 +174,6 @@ static void xtensa_cpu_realizefn(DeviceState *dev, Error **errp)
 
     cs->gdb_num_regs = xcc->config->gdb_regmap.num_regs;
 
-    qemu_init_vcpu(cs);
-
     xcc->parent_realize(dev, errp);
 }
 
-- 
2.41.0

Re: [PATCH 05/22] exec/cpu: Call qemu_init_vcpu() once in cpu_common_realize()

[Richard Henderson]

On 9/18/23 09:02, Philippe Mathieu-Daudé wrote:
> qemu_init_vcpu() is called in each ${target}_cpu_realize() before
> the call to parent_realize(), which is cpu_common_realizefn().
> Call it once there.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/core/cpu-common.c    | 3 +++
>   target/alpha/cpu.c      | 2 --
>   target/arm/cpu.c        | 2 --
>   target/avr/cpu.c        | 1 -
>   target/cris/cpu.c       | 2 --
>   target/hexagon/cpu.c    | 1 -
>   target/hppa/cpu.c       | 1 -
>   target/i386/cpu.c       | 4 +---
>   target/loongarch/cpu.c  | 2 --
>   target/m68k/cpu.c       | 2 --
>   target/microblaze/cpu.c | 2 --
>   target/mips/cpu.c       | 2 --
>   target/nios2/cpu.c      | 1 -
>   target/openrisc/cpu.c   | 2 --
>   target/ppc/cpu_init.c   | 1 -
>   target/riscv/cpu.c      | 2 --
>   target/rx/cpu.c         | 2 --
>   target/s390x/cpu.c      | 1 -
>   target/sh4/cpu.c        | 2 --
>   target/sparc/cpu.c      | 2 --
>   target/tricore/cpu.c    | 1 -
>   target/xtensa/cpu.c     | 2 --
>   22 files changed, 4 insertions(+), 36 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 05/22] exec/cpu: Call qemu_init_vcpu() once in cpu_common_realize()

[Igor Mammedov]

On Mon, 18 Sep 2023 18:02:38 +0200
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> qemu_init_vcpu() is called in each ${target}_cpu_realize() before
> the call to parent_realize(), which is cpu_common_realizefn().
> Call it once there.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/core/cpu-common.c    | 3 +++
>  target/alpha/cpu.c      | 2 --
>  target/arm/cpu.c        | 2 --
>  target/avr/cpu.c        | 1 -
>  target/cris/cpu.c       | 2 --
>  target/hexagon/cpu.c    | 1 -
>  target/hppa/cpu.c       | 1 -
>  target/i386/cpu.c       | 4 +---
>  target/loongarch/cpu.c  | 2 --
>  target/m68k/cpu.c       | 2 --
>  target/microblaze/cpu.c | 2 --
>  target/mips/cpu.c       | 2 --
>  target/nios2/cpu.c      | 1 -
>  target/openrisc/cpu.c   | 2 --
>  target/ppc/cpu_init.c   | 1 -
>  target/riscv/cpu.c      | 2 --
>  target/rx/cpu.c         | 2 --
>  target/s390x/cpu.c      | 1 -
>  target/sh4/cpu.c        | 2 --
>  target/sparc/cpu.c      | 2 --
>  target/tricore/cpu.c    | 1 -
>  target/xtensa/cpu.c     | 2 --
>  22 files changed, 4 insertions(+), 36 deletions(-)
> 
> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> index ced66c2b34..a3b8de7054 100644
> --- a/hw/core/cpu-common.c
> +++ b/hw/core/cpu-common.c
> @@ -204,6 +204,9 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp)
>          }
>      }
>  
> +    /* Create CPU address space and vCPU thread */
> +    qemu_init_vcpu(cpu);
> +
>      if (dev->hotplugged) {
>          cpu_synchronize_post_init(cpu);
>          cpu_resume(cpu);
> diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
> index 270ae787b1..eb78318bb8 100644
> --- a/target/alpha/cpu.c
> +++ b/target/alpha/cpu.c
> @@ -82,8 +82,6 @@ static void alpha_cpu_realizefn(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    qemu_init_vcpu(cs);
> -
>      acc->parent_realize(dev, errp);
>  }
>  
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 6aca036b85..fc3772025c 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -2277,8 +2277,6 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>          }
>      }
>  
> -    qemu_init_vcpu(cs);
> -
>      acc->parent_realize(dev, errp);
>      cpu_reset(cs);
>  }
> diff --git a/target/avr/cpu.c b/target/avr/cpu.c
> index 84d353f30e..d3460b3960 100644
> --- a/target/avr/cpu.c
> +++ b/target/avr/cpu.c
> @@ -119,7 +119,6 @@ static void avr_cpu_realizefn(DeviceState *dev, Error **errp)
>          error_propagate(errp, local_err);
>          return;
>      }
> -    qemu_init_vcpu(cs);
>  
>      mcc->parent_realize(dev, errp);
>      cpu_reset(cs);
> diff --git a/target/cris/cpu.c b/target/cris/cpu.c
> index 079872a5cc..671693a362 100644
> --- a/target/cris/cpu.c
> +++ b/target/cris/cpu.c
> @@ -152,8 +152,6 @@ static void cris_cpu_realizefn(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    qemu_init_vcpu(cs);
> -
>      ccc->parent_realize(dev, errp);
>      cpu_reset(cs);
>  }
> diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c
> index 7edc32701f..5b9bb3fe83 100644
> --- a/target/hexagon/cpu.c
> +++ b/target/hexagon/cpu.c
> @@ -345,7 +345,6 @@ static void hexagon_cpu_realize(DeviceState *dev, Error **errp)
>                               NUM_VREGS + NUM_QREGS,
>                               "hexagon-hvx.xml", 0);
>  
> -    qemu_init_vcpu(cs);
>      mcc->parent_realize(dev, errp);
>      cpu_reset(cs);
>  }
> diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c
> index 11022f9c99..49082bd2ba 100644
> --- a/target/hppa/cpu.c
> +++ b/target/hppa/cpu.c
> @@ -131,7 +131,6 @@ static void hppa_cpu_realizefn(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    qemu_init_vcpu(cs);
>      acc->parent_realize(dev, errp);
>  
>  #ifndef CONFIG_USER_ONLY
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 7faaa6915f..cb41d30aab 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -7425,15 +7425,13 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>  
>      mce_init(cpu);
>  
> -    qemu_init_vcpu(cs);
> -
>      /*
>       * Most Intel and certain AMD CPUs support hyperthreading. Even though QEMU
>       * fixes this issue by adjusting CPUID_0000_0001_EBX and CPUID_8000_0008_ECX
>       * based on inputs (sockets,cores,threads), it is still better to give
>       * users a warning.
>       *
> -     * NOTE: the following code has to follow qemu_init_vcpu(). Otherwise
> +     * NOTE: the following code has to follow cpu_common_realize(). Otherwise

Does moving qemu_init_vcpu() past check and altering comment here
makes cs->nr_threads somehow correct for following if()?

>       * cs->nr_threads hasn't be populated yet and the checking is incorrect.
>       */
>      if (IS_AMD_CPU(env) &&

missing context:
       if (IS_AMD_CPU(env) &&
           !(env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) &&
           cs->nr_threads > 1 && !ht_warned) {

> diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
> index 8029e70e76..dc0ac39833 100644
> --- a/target/loongarch/cpu.c
> +++ b/target/loongarch/cpu.c
> @@ -565,8 +565,6 @@ static void loongarch_cpu_realizefn(DeviceState *dev, Error **errp)
>  
>      loongarch_cpu_register_gdb_regs_for_features(cs);
>  
> -    qemu_init_vcpu(cs);
> -
>      lacc->parent_realize(dev, errp);
>      cpu_reset(cs);
>  }
> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
> index 2bc0a62f0e..3da316bc30 100644
> --- a/target/m68k/cpu.c
> +++ b/target/m68k/cpu.c
> @@ -321,8 +321,6 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error **errp)
>  
>      m68k_cpu_init_gdb(cpu);
>  
> -    qemu_init_vcpu(cs);
> -
>      mcc->parent_realize(dev, errp);
>      cpu_reset(cs);
>  }
> diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c
> index 03c2c4db1f..1f19a6e07d 100644
> --- a/target/microblaze/cpu.c
> +++ b/target/microblaze/cpu.c
> @@ -221,8 +221,6 @@ static void mb_cpu_realizefn(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    qemu_init_vcpu(cs);
> -
>      version = cpu->cfg.version ? cpu->cfg.version : DEFAULT_CPU_VERSION;
>      for (i = 0; mb_cpu_lookup[i].name && version; i++) {
>          if (strcmp(mb_cpu_lookup[i].name, version) == 0) {
> diff --git a/target/mips/cpu.c b/target/mips/cpu.c
> index 8d6f633f72..0aea69aaf9 100644
> --- a/target/mips/cpu.c
> +++ b/target/mips/cpu.c
> @@ -492,8 +492,6 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
>      fpu_init(env, env->cpu_model);
>      mvp_init(env);
>  
> -    qemu_init_vcpu(cs);
> -
>      mcc->parent_realize(dev, errp);
>      cpu_reset(cs);
>  }
> diff --git a/target/nios2/cpu.c b/target/nios2/cpu.c
> index 876a6dcad2..7a92fc5f2c 100644
> --- a/target/nios2/cpu.c
> +++ b/target/nios2/cpu.c
> @@ -216,7 +216,6 @@ static void nios2_cpu_realizefn(DeviceState *dev, Error **errp)
>      }
>  
>      realize_cr_status(cs);
> -    qemu_init_vcpu(cs);
>  
>      /* We have reserved storage for cpuid; might as well use it. */
>      cpu->env.ctrl[CR_CPUID] = cs->cpu_index;
> diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c
> index cd25f1e9d5..e4ec95ca7f 100644
> --- a/target/openrisc/cpu.c
> +++ b/target/openrisc/cpu.c
> @@ -141,8 +141,6 @@ static void openrisc_cpu_realizefn(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    qemu_init_vcpu(cs);
> -
>      occ->parent_realize(dev, errp);
>      cpu_reset(cs);
>  }
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 7ab5ee92d9..e2c06c1f32 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -6833,7 +6833,6 @@ static void ppc_cpu_realize(DeviceState *dev, Error **errp)
>      init_ppc_proc(cpu);
>  
>      ppc_gdb_init(cs, pcc);
> -    qemu_init_vcpu(cs);
>  
>      pcc->parent_realize(dev, errp);
>  
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 7566757346..4f7ae55359 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1531,8 +1531,6 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
>      }
>  #endif
>  
> -    qemu_init_vcpu(cs);
> -
>      mcc->parent_realize(dev, errp);
>      cpu_reset(cs);
>  }
> diff --git a/target/rx/cpu.c b/target/rx/cpu.c
> index c9c8443cbd..089df61790 100644
> --- a/target/rx/cpu.c
> +++ b/target/rx/cpu.c
> @@ -137,8 +137,6 @@ static void rx_cpu_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    qemu_init_vcpu(cs);
> -
>      rcc->parent_realize(dev, errp);
>      cpu_reset(cs);
>  }
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 0f0b11fd73..416ac6c4e0 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -252,7 +252,6 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>      qemu_register_reset(s390_cpu_machine_reset_cb, S390_CPU(dev));
>  #endif
>      s390_cpu_gdb_init(cs);
> -    qemu_init_vcpu(cs);
>  
>      scc->parent_realize(dev, &err);
>      /*
> diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c
> index 656d71f74a..e6690daf9a 100644
> --- a/target/sh4/cpu.c
> +++ b/target/sh4/cpu.c
> @@ -228,8 +228,6 @@ static void superh_cpu_realizefn(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    qemu_init_vcpu(cs);
> -
>      scc->parent_realize(dev, errp);
>      cpu_reset(cs);
>  }
> diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
> index 130ab8f578..2fdc95eda9 100644
> --- a/target/sparc/cpu.c
> +++ b/target/sparc/cpu.c
> @@ -782,8 +782,6 @@ static void sparc_cpu_realizefn(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    qemu_init_vcpu(cs);
> -
>      scc->parent_realize(dev, errp);
>  }
>  
> diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c
> index a3610aecca..0142cf556d 100644
> --- a/target/tricore/cpu.c
> +++ b/target/tricore/cpu.c
> @@ -118,7 +118,6 @@ static void tricore_cpu_realizefn(DeviceState *dev, Error **errp)
>      if (tricore_has_feature(env, TRICORE_FEATURE_131)) {
>          set_feature(env, TRICORE_FEATURE_13);
>      }
> -    qemu_init_vcpu(cs);
>  
>      tcc->parent_realize(dev, errp);
>      cpu_reset(cs);
> diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
> index acaf8c905f..300d19d45c 100644
> --- a/target/xtensa/cpu.c
> +++ b/target/xtensa/cpu.c
> @@ -174,8 +174,6 @@ static void xtensa_cpu_realizefn(DeviceState *dev, Error **errp)
>  
>      cs->gdb_num_regs = xcc->config->gdb_regmap.num_regs;
>  
> -    qemu_init_vcpu(cs);
> -
>      xcc->parent_realize(dev, errp);
>  }
>  

[PATCH 06/22] exec/cpu: Call cpu_remove_sync() once in cpu_common_unrealize()

[Philippe Mathieu-Daudé]

While create_vcpu_thread() creates a vCPU thread, its counterpart
is cpu_remove_sync(), which join and destroy the thread.

create_vcpu_thread() is called in qemu_init_vcpu(), itself called
in cpu_common_realizefn(). Since we don't have qemu_deinit_vcpu()
helper (we probably don't need any), simply destroy the thread in
cpu_common_unrealizefn().

Note: only the PPC and X86 targets were calling cpu_remove_sync(),
meaning all other targets were leaking the thread when the vCPU
was unrealized (mostly when vCPU are hot-unplugged).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/core/cpu-common.c  | 3 +++
 target/i386/cpu.c     | 1 -
 target/ppc/cpu_init.c | 2 --
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index a3b8de7054..e5841c59df 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -221,6 +221,9 @@ static void cpu_common_unrealizefn(DeviceState *dev)
 
     /* NOTE: latest generic point before the cpu is fully unrealized */
     cpu_exec_unrealizefn(cpu);
+
+    /* Destroy vCPU thread */
+    cpu_remove_sync(cpu);
 }
 
 static void cpu_common_initfn(Object *obj)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index cb41d30aab..d79797d963 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -7470,7 +7470,6 @@ static void x86_cpu_unrealizefn(DeviceState *dev)
     X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
 
 #ifndef CONFIG_USER_ONLY
-    cpu_remove_sync(CPU(dev));
     qemu_unregister_reset(x86_cpu_machine_reset_cb, dev);
 #endif
 
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index e2c06c1f32..24d4e8fa7e 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -6853,8 +6853,6 @@ static void ppc_cpu_unrealize(DeviceState *dev)
 
     pcc->parent_unrealize(dev);
 
-    cpu_remove_sync(CPU(cpu));
-
     destroy_ppc_opcodes(cpu);
 }
 
-- 
2.41.0

Re: [PATCH 06/22] exec/cpu: Call cpu_remove_sync() once in cpu_common_unrealize()

[Richard Henderson]

On 9/18/23 09:02, Philippe Mathieu-Daudé wrote:
> While create_vcpu_thread() creates a vCPU thread, its counterpart
> is cpu_remove_sync(), which join and destroy the thread.
> 
> create_vcpu_thread() is called in qemu_init_vcpu(), itself called
> in cpu_common_realizefn(). Since we don't have qemu_deinit_vcpu()
> helper (we probably don't need any), simply destroy the thread in
> cpu_common_unrealizefn().
> 
> Note: only the PPC and X86 targets were calling cpu_remove_sync(),
> meaning all other targets were leaking the thread when the vCPU
> was unrealized (mostly when vCPU are hot-unplugged).
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/core/cpu-common.c  | 3 +++
>   target/i386/cpu.c     | 1 -
>   target/ppc/cpu_init.c | 2 --
>   3 files changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 06/22] exec/cpu: Call cpu_remove_sync() once in cpu_common_unrealize()

[Igor Mammedov]

On Mon, 18 Sep 2023 18:02:39 +0200
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> While create_vcpu_thread() creates a vCPU thread, its counterpart
> is cpu_remove_sync(), which join and destroy the thread.
> 
> create_vcpu_thread() is called in qemu_init_vcpu(), itself called
> in cpu_common_realizefn(). Since we don't have qemu_deinit_vcpu()
> helper (we probably don't need any), simply destroy the thread in
> cpu_common_unrealizefn().
> 
> Note: only the PPC and X86 targets were calling cpu_remove_sync(),
> meaning all other targets were leaking the thread when the vCPU
> was unrealized (mostly when vCPU are hot-unplugged).
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/core/cpu-common.c  | 3 +++
>  target/i386/cpu.c     | 1 -
>  target/ppc/cpu_init.c | 2 --
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> index a3b8de7054..e5841c59df 100644
> --- a/hw/core/cpu-common.c
> +++ b/hw/core/cpu-common.c
> @@ -221,6 +221,9 @@ static void cpu_common_unrealizefn(DeviceState *dev)
>  
>      /* NOTE: latest generic point before the cpu is fully unrealized */
>      cpu_exec_unrealizefn(cpu);
> +
> +    /* Destroy vCPU thread */
> +    cpu_remove_sync(cpu);
>  }
>  
>  static void cpu_common_initfn(Object *obj)
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index cb41d30aab..d79797d963 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -7470,7 +7470,6 @@ static void x86_cpu_unrealizefn(DeviceState *dev)
>      X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
>  
>  #ifndef CONFIG_USER_ONLY
> -    cpu_remove_sync(CPU(dev));
>      qemu_unregister_reset(x86_cpu_machine_reset_cb, dev);
>  #endif

missing  followup context:
    ...
    xcc->parent_unrealize(dev); 

Before the patch, vcpu thread is stopped and onnly then
clean up happens.

After the patch we have cleanup while vcpu thread is still running.

Even if it doesn't explode, such ordering still seems to be wrong.

> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index e2c06c1f32..24d4e8fa7e 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -6853,8 +6853,6 @@ static void ppc_cpu_unrealize(DeviceState *dev)
>  
>      pcc->parent_unrealize(dev);
>  
> -    cpu_remove_sync(CPU(cpu));

bug in current code?

> -
>      destroy_ppc_opcodes(cpu);
>  }
>  

Re: [PATCH 06/22] exec/cpu: Call cpu_remove_sync() once in cpu_common_unrealize()

[Philippe Mathieu-Daudé]

On 28/11/23 17:42, Igor Mammedov wrote:
> On Mon, 18 Sep 2023 18:02:39 +0200
> Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> 
>> While create_vcpu_thread() creates a vCPU thread, its counterpart
>> is cpu_remove_sync(), which join and destroy the thread.
>>
>> create_vcpu_thread() is called in qemu_init_vcpu(), itself called
>> in cpu_common_realizefn(). Since we don't have qemu_deinit_vcpu()
>> helper (we probably don't need any), simply destroy the thread in
>> cpu_common_unrealizefn().
>>
>> Note: only the PPC and X86 targets were calling cpu_remove_sync(),
>> meaning all other targets were leaking the thread when the vCPU
>> was unrealized (mostly when vCPU are hot-unplugged).
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/core/cpu-common.c  | 3 +++
>>   target/i386/cpu.c     | 1 -
>>   target/ppc/cpu_init.c | 2 --
>>   3 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
>> index a3b8de7054..e5841c59df 100644
>> --- a/hw/core/cpu-common.c
>> +++ b/hw/core/cpu-common.c
>> @@ -221,6 +221,9 @@ static void cpu_common_unrealizefn(DeviceState *dev)
>>   
>>       /* NOTE: latest generic point before the cpu is fully unrealized */
>>       cpu_exec_unrealizefn(cpu);
>> +
>> +    /* Destroy vCPU thread */
>> +    cpu_remove_sync(cpu);
>>   }
>>   
>>   static void cpu_common_initfn(Object *obj)
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index cb41d30aab..d79797d963 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -7470,7 +7470,6 @@ static void x86_cpu_unrealizefn(DeviceState *dev)
>>       X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
>>   
>>   #ifndef CONFIG_USER_ONLY
>> -    cpu_remove_sync(CPU(dev));
>>       qemu_unregister_reset(x86_cpu_machine_reset_cb, dev);
>>   #endif
> 
> missing  followup context:
>      ...
>      xcc->parent_unrealize(dev);
> 
> Before the patch, vcpu thread is stopped and onnly then
> clean up happens.
> 
> After the patch we have cleanup while vcpu thread is still running.
> 
> Even if it doesn't explode, such ordering still seems to be wrong.

OK.

>> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
>> index e2c06c1f32..24d4e8fa7e 100644
>> --- a/target/ppc/cpu_init.c
>> +++ b/target/ppc/cpu_init.c
>> @@ -6853,8 +6853,6 @@ static void ppc_cpu_unrealize(DeviceState *dev)
>>   
>>       pcc->parent_unrealize(dev);
>>   
>> -    cpu_remove_sync(CPU(cpu));
> 
> bug in current code?

Plausibly. See:

commit f1023d21e81b7bf523ddf2ac91a48117f20ef9d7
Author: Greg Kurz <groug@kaod.org>
Date:   Thu Oct 15 23:18:32 2020 +0200

     spapr: Unrealize vCPUs with qdev_unrealize()

     Since we introduced CPU hot-unplug in sPAPR, we don't unrealize the
     vCPU objects explicitly. Instead, we let QOM handle that for us
     under object_property_del_all() when the CPU core object is
     finalized. The only thing we do is calling cpu_remove_sync() to
     tear the vCPU thread down.

     This happens to work but it is ugly because:
     - we call qdev_realize() but the corresponding qdev_unrealize() is
       buried deep in the QOM code
     - we call cpu_remove_sync() to undo qemu_init_vcpu() called by
       ppc_cpu_realize() in target/ppc/translate_init.c.inc
     - the CPU init and teardown paths aren't really symmetrical

     The latter didn't bite us so far but a future patch that greatly
     simplifies the CPU core realize path needs it to avoid a crash
     in QOM.

     For all these reasons, have ppc_cpu_unrealize() to undo the changes
     of ppc_cpu_realize() by calling cpu_remove_sync() at the right
     place, and have the sPAPR CPU core code to call qdev_unrealize().

     This requires to add a missing stub because translate_init.c.inc is
     also compiled for user mode.

> 
>> -
>>       destroy_ppc_opcodes(cpu);
>>   }
>>   

Re: [PATCH 06/22] exec/cpu: Call cpu_remove_sync() once in cpu_common_unrealize()

[Philippe Mathieu-Daudé]

On 16/1/25 19:05, Philippe Mathieu-Daudé wrote:
> On 28/11/23 17:42, Igor Mammedov wrote:
>> On Mon, 18 Sep 2023 18:02:39 +0200
>> Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>>> While create_vcpu_thread() creates a vCPU thread, its counterpart
>>> is cpu_remove_sync(), which join and destroy the thread.
>>>
>>> create_vcpu_thread() is called in qemu_init_vcpu(), itself called
>>> in cpu_common_realizefn(). Since we don't have qemu_deinit_vcpu()
>>> helper (we probably don't need any), simply destroy the thread in
>>> cpu_common_unrealizefn().
>>>
>>> Note: only the PPC and X86 targets were calling cpu_remove_sync(),
>>> meaning all other targets were leaking the thread when the vCPU
>>> was unrealized (mostly when vCPU are hot-unplugged).
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   hw/core/cpu-common.c  | 3 +++
>>>   target/i386/cpu.c     | 1 -
>>>   target/ppc/cpu_init.c | 2 --
>>>   3 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
>>> index a3b8de7054..e5841c59df 100644
>>> --- a/hw/core/cpu-common.c
>>> +++ b/hw/core/cpu-common.c
>>> @@ -221,6 +221,9 @@ static void cpu_common_unrealizefn(DeviceState *dev)
>>>       /* NOTE: latest generic point before the cpu is fully 
>>> unrealized */
>>>       cpu_exec_unrealizefn(cpu);
>>> +
>>> +    /* Destroy vCPU thread */
>>> +    cpu_remove_sync(cpu);
>>>   }
>>>   static void cpu_common_initfn(Object *obj)
>>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>>> index cb41d30aab..d79797d963 100644
>>> --- a/target/i386/cpu.c
>>> +++ b/target/i386/cpu.c
>>> @@ -7470,7 +7470,6 @@ static void x86_cpu_unrealizefn(DeviceState *dev)
>>>       X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
>>>   #ifndef CONFIG_USER_ONLY
>>> -    cpu_remove_sync(CPU(dev));
>>>       qemu_unregister_reset(x86_cpu_machine_reset_cb, dev);
>>>   #endif
>>
>> missing  followup context:
>>      ...
>>      xcc->parent_unrealize(dev);
>>
>> Before the patch, vcpu thread is stopped and onnly then
>> clean up happens.
>>
>> After the patch we have cleanup while vcpu thread is still running.
>>
>> Even if it doesn't explode, such ordering still seems to be wrong.
> 
> OK.
> 
>>> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
>>> index e2c06c1f32..24d4e8fa7e 100644
>>> --- a/target/ppc/cpu_init.c
>>> +++ b/target/ppc/cpu_init.c
>>> @@ -6853,8 +6853,6 @@ static void ppc_cpu_unrealize(DeviceState *dev)
>>>       pcc->parent_unrealize(dev);
>>> -    cpu_remove_sync(CPU(cpu));
>>
>> bug in current code?
> 
> Plausibly. See:
> 
> commit f1023d21e81b7bf523ddf2ac91a48117f20ef9d7
> Author: Greg Kurz <groug@kaod.org>
> Date:   Thu Oct 15 23:18:32 2020 +0200
> 
>      spapr: Unrealize vCPUs with qdev_unrealize()
> 
>      Since we introduced CPU hot-unplug in sPAPR, we don't unrealize the
>      vCPU objects explicitly. Instead, we let QOM handle that for us
>      under object_property_del_all() when the CPU core object is
>      finalized. The only thing we do is calling cpu_remove_sync() to
>      tear the vCPU thread down.
> 
>      This happens to work but it is ugly because:
>      - we call qdev_realize() but the corresponding qdev_unrealize() is
>        buried deep in the QOM code
>      - we call cpu_remove_sync() to undo qemu_init_vcpu() called by
>        ppc_cpu_realize() in target/ppc/translate_init.c.inc
>      - the CPU init and teardown paths aren't really symmetrical
> 
>      The latter didn't bite us so far but a future patch that greatly
>      simplifies the CPU core realize path needs it to avoid a crash
>      in QOM.
> 
>      For all these reasons, have ppc_cpu_unrealize() to undo the changes
>      of ppc_cpu_realize() by calling cpu_remove_sync() at the right
>      place, and have the sPAPR CPU core code to call qdev_unrealize().
> 
>      This requires to add a missing stub because translate_init.c.inc is
>      also compiled for user mode.

See also:

commit 5e22e29201d80124bca0124f2034e72b698cbb6f
Author: David Gibson <david@gibson.dropbear.id.au>
Date:   Wed Jun 13 12:08:42 2018 +1000

     pnv: Add cpu unrealize path

     Currently we don't have any unrealize path for pnv cpu cores.
     We get away with this because we don't yet support cpu hotplug
     for pnv.

     However, we're going to want it eventually, and in the meantime,
     it makes it non-obvious why there are a bunch of allocations on
     the realize() path that don't have matching frees.

     So, implement the missing unrealize path.

diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
index f4c41d89d6d..f7cf33f547a 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -186,6 +186,26 @@ err:
      error_propagate(errp, local_err);
  }

+static void pnv_unrealize_vcpu(PowerPCCPU *cpu)
+{
+    qemu_unregister_reset(pnv_cpu_reset, cpu);
+    object_unparent(cpu->intc);
+    cpu_remove_sync(CPU(cpu));
+    object_unparent(OBJECT(cpu));
+}

Re: [PATCH 06/22] exec/cpu: Call cpu_remove_sync() once in cpu_common_unrealize()

[Igor Mammedov]

On Thu, 16 Jan 2025 19:05:46 +0100
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> On 28/11/23 17:42, Igor Mammedov wrote:
> > On Mon, 18 Sep 2023 18:02:39 +0200
> > Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >   
> >> While create_vcpu_thread() creates a vCPU thread, its counterpart
> >> is cpu_remove_sync(), which join and destroy the thread.
> >>
> >> create_vcpu_thread() is called in qemu_init_vcpu(), itself called
> >> in cpu_common_realizefn(). Since we don't have qemu_deinit_vcpu()
> >> helper (we probably don't need any), simply destroy the thread in
> >> cpu_common_unrealizefn().
> >>
> >> Note: only the PPC and X86 targets were calling cpu_remove_sync(),
> >> meaning all other targets were leaking the thread when the vCPU
> >> was unrealized (mostly when vCPU are hot-unplugged).
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >> ---
> >>   hw/core/cpu-common.c  | 3 +++
> >>   target/i386/cpu.c     | 1 -
> >>   target/ppc/cpu_init.c | 2 --
> >>   3 files changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> >> index a3b8de7054..e5841c59df 100644
> >> --- a/hw/core/cpu-common.c
> >> +++ b/hw/core/cpu-common.c
> >> @@ -221,6 +221,9 @@ static void cpu_common_unrealizefn(DeviceState *dev)
> >>   
> >>       /* NOTE: latest generic point before the cpu is fully unrealized */
> >>       cpu_exec_unrealizefn(cpu);
> >> +
> >> +    /* Destroy vCPU thread */
> >> +    cpu_remove_sync(cpu);
> >>   }
> >>   
> >>   static void cpu_common_initfn(Object *obj)
> >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> >> index cb41d30aab..d79797d963 100644
> >> --- a/target/i386/cpu.c
> >> +++ b/target/i386/cpu.c
> >> @@ -7470,7 +7470,6 @@ static void x86_cpu_unrealizefn(DeviceState *dev)
> >>       X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
> >>   
> >>   #ifndef CONFIG_USER_ONLY
> >> -    cpu_remove_sync(CPU(dev));
> >>       qemu_unregister_reset(x86_cpu_machine_reset_cb, dev);
> >>   #endif  
> > 
> > missing  followup context:
> >      ...
> >      xcc->parent_unrealize(dev);
> > 
> > Before the patch, vcpu thread is stopped and onnly then
> > clean up happens.
> > 
> > After the patch we have cleanup while vcpu thread is still running.
> > 
> > Even if it doesn't explode, such ordering still seems to be wrong.  
> 
> OK.

looking at all users, some do stop vcpu thread before tearing down vcpu object
and interrupt controller, while some do it other way around or mix of both.

It's probably safe to stop vcpu thread wrt intc cleanup.
Can you check what would happen if there were a pending interrupt,
but then flowing would happen:
 1. destroying intc
 2. can vcpu thread just kicked out from KVM_RUN,
    trip over missing/invalid intc state while thread runs towards its exit point?

If above can't crash then,
I'd prefer to stop vcpu at least before vcpu cleanup is run.
i.e. put cpu_remove_sync() as the very 1st call inside of cpu_common_unrealizefn()

> >> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> >> index e2c06c1f32..24d4e8fa7e 100644
> >> --- a/target/ppc/cpu_init.c
> >> +++ b/target/ppc/cpu_init.c
> >> @@ -6853,8 +6853,6 @@ static void ppc_cpu_unrealize(DeviceState *dev)
> >>   
> >>       pcc->parent_unrealize(dev);
> >>   
> >> -    cpu_remove_sync(CPU(cpu));  
> > 
> > bug in current code?  
> 
> Plausibly. See:
> 
> commit f1023d21e81b7bf523ddf2ac91a48117f20ef9d7
> Author: Greg Kurz <groug@kaod.org>
> Date:   Thu Oct 15 23:18:32 2020 +0200
> 
>      spapr: Unrealize vCPUs with qdev_unrealize()
> 
>      Since we introduced CPU hot-unplug in sPAPR, we don't unrealize the
>      vCPU objects explicitly. Instead, we let QOM handle that for us
>      under object_property_del_all() when the CPU core object is
>      finalized. The only thing we do is calling cpu_remove_sync() to
>      tear the vCPU thread down.
> 
>      This happens to work but it is ugly because:
>      - we call qdev_realize() but the corresponding qdev_unrealize() is
>        buried deep in the QOM code
>      - we call cpu_remove_sync() to undo qemu_init_vcpu() called by
>        ppc_cpu_realize() in target/ppc/translate_init.c.inc
>      - the CPU init and teardown paths aren't really symmetrical
> 
>      The latter didn't bite us so far but a future patch that greatly
>      simplifies the CPU core realize path needs it to avoid a crash
>      in QOM.
> 
>      For all these reasons, have ppc_cpu_unrealize() to undo the changes
>      of ppc_cpu_realize() by calling cpu_remove_sync() at the right
>      place, and have the sPAPR CPU core code to call qdev_unrealize().
> 
>      This requires to add a missing stub because translate_init.c.inc is
>      also compiled for user mode.
> 
> >   
> >> -
> >>       destroy_ppc_opcodes(cpu);
> >>   }
> >>     
> 

[PATCH 07/22] exec/cpu: Introduce the CPU address space destruction function

[Philippe Mathieu-Daudé]

From: xianglai li <lixianglai@loongson.cn>

Introduce new function to destroy CPU address space resources
for cpu hot-(un)plug.

Co-authored-by: "Salil Mehta" <salil.mehta@opnsrc.net>
Cc: "Salil Mehta" <salil.mehta@opnsrc.net>
Cc: Xiaojuan Yang <yangxiaojuan@loongson.cn>
Cc: Song Gao <gaosong@loongson.cn>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Ani Sinha <anisinha@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Eduardo Habkost <eduardo@habkost.net>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Cc: "Philippe Mathieu-Daudé" <philmd@linaro.org>
Cc: Yanan Wang <wangyanan55@huawei.com>
Cc: "Daniel P. Berrangé" <berrange@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Bibo Mao <maobibo@loongson.cn>
Signed-off-by: xianglai li <lixianglai@loongson.cn>
Message-ID: <3a4fc2a3df4b767c3c296a7da3bc15ca9c251316.1694433326.git.lixianglai@loongson.cn>
---
 include/exec/cpu-common.h |  8 ++++++++
 include/hw/core/cpu.h     |  1 +
 softmmu/physmem.c         | 24 ++++++++++++++++++++++++
 3 files changed, 33 insertions(+)

diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index 41788c0bdd..eb56a228a2 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -120,6 +120,14 @@ size_t qemu_ram_pagesize_largest(void);
  */
 void cpu_address_space_init(CPUState *cpu, int asidx,
                             const char *prefix, MemoryRegion *mr);
+/**
+ * cpu_address_space_destroy:
+ * @cpu: CPU for which address space needs to be destroyed
+ * @asidx: integer index of this address space
+ *
+ * Note that with KVM only one address space is supported.
+ */
+void cpu_address_space_destroy(CPUState *cpu, int asidx);
 
 void cpu_physical_memory_rw(hwaddr addr, void *buf,
                             hwaddr len, bool is_write);
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 92a4234439..c90cf3a162 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -366,6 +366,7 @@ struct CPUState {
     QSIMPLEQ_HEAD(, qemu_work_item) work_list;
 
     CPUAddressSpace *cpu_ases;
+    int cpu_ases_ref_count;
     int num_ases;
     AddressSpace *as;
     MemoryRegion *memory;
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 18277ddd67..c75e3e8042 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -761,6 +761,7 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
 
     if (!cpu->cpu_ases) {
         cpu->cpu_ases = g_new0(CPUAddressSpace, cpu->num_ases);
+        cpu->cpu_ases_ref_count = cpu->num_ases;
     }
 
     newas = &cpu->cpu_ases[asidx];
@@ -774,6 +775,29 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
     }
 }
 
+void cpu_address_space_destroy(CPUState *cpu, int asidx)
+{
+    CPUAddressSpace *cpuas;
+
+    assert(asidx < cpu->num_ases);
+    assert(asidx == 0 || !kvm_enabled());
+    assert(cpu->cpu_ases);
+
+    cpuas = &cpu->cpu_ases[asidx];
+    if (tcg_enabled()) {
+        memory_listener_unregister(&cpuas->tcg_as_listener);
+    }
+
+    address_space_destroy(cpuas->as);
+
+    cpu->cpu_ases_ref_count--;
+    if (cpu->cpu_ases_ref_count == 0) {
+        g_free(cpu->cpu_ases);
+        cpu->cpu_ases = NULL;
+    }
+
+}
+
 AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx)
 {
     /* Return the AddressSpace corresponding to the specified index */
-- 
2.41.0

Re: [PATCH 07/22] exec/cpu: Introduce the CPU address space destruction function

[Richard Henderson]

On 9/18/23 09:02, Philippe Mathieu-Daudé wrote:
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -761,6 +761,7 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
>   
>       if (!cpu->cpu_ases) {
>           cpu->cpu_ases = g_new0(CPUAddressSpace, cpu->num_ases);
> +        cpu->cpu_ases_ref_count = cpu->num_ases;
>       }
>   
...
>   
> +void cpu_address_space_destroy(CPUState *cpu, int asidx)
> +{
> +    CPUAddressSpace *cpuas;
> +
> +    assert(asidx < cpu->num_ases);
> +    assert(asidx == 0 || !kvm_enabled());
> +    assert(cpu->cpu_ases);
> +
> +    cpuas = &cpu->cpu_ases[asidx];
> +    if (tcg_enabled()) {
> +        memory_listener_unregister(&cpuas->tcg_as_listener);
> +    }
> +
> +    address_space_destroy(cpuas->as);
> +
> +    cpu->cpu_ases_ref_count--;
> +    if (cpu->cpu_ases_ref_count == 0) {
> +        g_free(cpu->cpu_ases);
> +        cpu->cpu_ases = NULL;
> +    }
> +
> +}

I think it would be better to destroy all address spaces at once, so that you don't need 
to invent a reference count that isn't used for anything else.


r~

RE: [PATCH 07/22] exec/cpu: Introduce the CPU address space destruction function

[Salil Mehta via]

Hi Phillipe,
This patch has been taken as it is from ARM's RFC and the common patch-set mentioned below.
So SOBs are all wrong everywhere.


Original RFC posted in the year 2020.
[1] https://lore.kernel.org/qemu-devel/20200613213629.21984-23-salil.mehta@huawei.com/

Recently posted RFC V2
[2] https://lore.kernel.org/qemu-devel/20230926100436.28284-1-salil.mehta@huawei.com/T/#m5f5ae40b091d69d01012880d7500d96874a9d39c

Recent common patch-set for Virtual CPU Hotplug
[3] https://lore.kernel.org/qemu-devel/20230930001933.2660-9-salil.mehta@huawei.com/

[4] https://lore.kernel.org/qemu-devel/20230930001933.2660-10-salil.mehta@huawei.com/

Beside my original patch-set had bug which you have inherited in this patch-set.
 
Thanks
Salil


> From: qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org <qemu-arm-
> bounces+salil.mehta=huawei.com@nongnu.org> On Behalf Of Philippe Mathieu-
> Daudé
> Sent: Monday, September 18, 2023 5:03 PM
> To: qemu-devel@nongnu.org
> Cc: Laurent Vivier <laurent@vivier.eu>; Paolo Bonzini
> <pbonzini@redhat.com>; Max Filippov <jcmvbkbc@gmail.com>; David Hildenbrand
> <david@redhat.com>; Peter Xu <peterx@redhat.com>; Anton Johansson
> <anjo@rev.ng>; Peter Maydell <peter.maydell@linaro.org>;
> kvm@vger.kernel.org; Marek Vasut <marex@denx.de>; David Gibson
> <david@gibson.dropbear.id.au>; Brian Cain <bcain@quicinc.com>; Yoshinori
> Sato <ysato@users.sourceforge.jp>; Edgar E . Iglesias
> <edgar.iglesias@gmail.com>; Claudio Fontana <cfontana@suse.de>; Daniel
> Henrique Barboza <dbarboza@ventanamicro.com>; Artyom Tarasenko
> <atar4qemu@gmail.com>; Marcelo Tosatti <mtosatti@redhat.com>; qemu-
> ppc@nongnu.org; Liu Zhiwei <zhiwei_liu@linux.alibaba.com>; Aurelien Jarno
> <aurelien@aurel32.net>; Ilya Leoshkevich <iii@linux.ibm.com>; Daniel
> Henrique Barboza <danielhb413@gmail.com>; Bastian Koppelmann
> <kbastian@mail.uni-paderborn.de>; Cédric Le Goater <clg@kaod.org>; Alistair
> Francis <alistair.francis@wdc.com>; Alessandro Di Federico <ale@rev.ng>;
> Song Gao <gaosong@loongson.cn>; Marcel Apfelbaum
> <marcel.apfelbaum@gmail.com>; Chris Wulff <crwulff@gmail.com>; Michael S.
> Tsirkin <mst@redhat.com>; Alistair Francis <alistair@alistair23.me>;
> Fabiano Rosas <farosas@suse.de>; qemu-s390x@nongnu.org; wangyanan (Y)
> <wangyanan55@huawei.com>; Luc Michel <luc@lmichel.fr>; Weiwei Li
> <liweiwei@iscas.ac.cn>; Bin Meng <bin.meng@windriver.com>; Stafford Horne
> <shorne@gmail.com>; Xiaojuan Yang <yangxiaojuan@loongson.cn>; Daniel P .
> Berrange <berrange@redhat.com>; Thomas Huth <thuth@redhat.com>; Philippe
> Mathieu-Daudé <philmd@linaro.org>; qemu-arm@nongnu.org; Jiaxun Yang
> <jiaxun.yang@flygoat.com>; Richard Henderson
> <richard.henderson@linaro.org>; Aleksandar Rikalo
> <aleksandar.rikalo@syrmia.com>; Bernhard Beschow <shentey@gmail.com>; Mark
> Cave-Ayland <mark.cave-ayland@ilande.co.uk>; qemu-riscv@nongnu.org; Alex
> Bennée <alex.bennee@linaro.org>; Nicholas Piggin <npiggin@gmail.com>; Greg
> Kurz <groug@kaod.org>; Michael Rolnik <mrolnik@gmail.com>; Eduardo Habkost
> <eduardo@habkost.net>; Markus Armbruster <armbru@redhat.com>; Palmer
> Dabbelt <palmer@dabbelt.com>; xianglai li <lixianglai@loongson.cn>; Salil
> Mehta <salil.mehta@opnsrc.net>; Igor Mammedov <imammedo@redhat.com>; Ani
> Sinha <anisinha@redhat.com>; Bibo Mao <maobibo@loongson.cn>
> Subject: [PATCH 07/22] exec/cpu: Introduce the CPU address space
> destruction function
> 
> From: xianglai li <lixianglai@loongson.cn>
> 
> Introduce new function to destroy CPU address space resources
> for cpu hot-(un)plug.
> 
> Co-authored-by: "Salil Mehta" <salil.mehta@opnsrc.net>
> Cc: "Salil Mehta" <salil.mehta@opnsrc.net>
> Cc: Xiaojuan Yang <yangxiaojuan@loongson.cn>
> Cc: Song Gao <gaosong@loongson.cn>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Ani Sinha <anisinha@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Eduardo Habkost <eduardo@habkost.net>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Cc: "Philippe Mathieu-Daudé" <philmd@linaro.org>
> Cc: Yanan Wang <wangyanan55@huawei.com>
> Cc: "Daniel P. Berrangé" <berrange@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Bibo Mao <maobibo@loongson.cn>
> Signed-off-by: xianglai li <lixianglai@loongson.cn>
> Message-ID:
> <3a4fc2a3df4b767c3c296a7da3bc15ca9c251316.1694433326.git.lixianglai@loongso
> n.cn>
> ---
>  include/exec/cpu-common.h |  8 ++++++++
>  include/hw/core/cpu.h     |  1 +
>  softmmu/physmem.c         | 24 ++++++++++++++++++++++++
>  3 files changed, 33 insertions(+)
> 
> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index 41788c0bdd..eb56a228a2 100644
> --- a/include/exec/cpu-common.h
> +++ b/include/exec/cpu-common.h
> @@ -120,6 +120,14 @@ size_t qemu_ram_pagesize_largest(void);
>   */
>  void cpu_address_space_init(CPUState *cpu, int asidx,
>                              const char *prefix, MemoryRegion *mr);
> +/**
> + * cpu_address_space_destroy:
> + * @cpu: CPU for which address space needs to be destroyed
> + * @asidx: integer index of this address space
> + *
> + * Note that with KVM only one address space is supported.
> + */
> +void cpu_address_space_destroy(CPUState *cpu, int asidx);
> 
>  void cpu_physical_memory_rw(hwaddr addr, void *buf,
>                              hwaddr len, bool is_write);
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 92a4234439..c90cf3a162 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -366,6 +366,7 @@ struct CPUState {
>      QSIMPLEQ_HEAD(, qemu_work_item) work_list;
> 
>      CPUAddressSpace *cpu_ases;
> +    int cpu_ases_ref_count;
>      int num_ases;
>      AddressSpace *as;
>      MemoryRegion *memory;
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index 18277ddd67..c75e3e8042 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -761,6 +761,7 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
> 
>      if (!cpu->cpu_ases) {
>          cpu->cpu_ases = g_new0(CPUAddressSpace, cpu->num_ases);
> +        cpu->cpu_ases_ref_count = cpu->num_ases;
>      }
> 
>      newas = &cpu->cpu_ases[asidx];
> @@ -774,6 +775,29 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
>      }
>  }
> 
> +void cpu_address_space_destroy(CPUState *cpu, int asidx)
> +{
> +    CPUAddressSpace *cpuas;
> +
> +    assert(asidx < cpu->num_ases);
> +    assert(asidx == 0 || !kvm_enabled());
> +    assert(cpu->cpu_ases);
> +
> +    cpuas = &cpu->cpu_ases[asidx];
> +    if (tcg_enabled()) {
> +        memory_listener_unregister(&cpuas->tcg_as_listener);
> +    }
> +
> +    address_space_destroy(cpuas->as);
> +
> +    cpu->cpu_ases_ref_count--;
> +    if (cpu->cpu_ases_ref_count == 0) {
> +        g_free(cpu->cpu_ases);
> +        cpu->cpu_ases = NULL;
> +    }
> +
> +}
> +
>  AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx)
>  {
>      /* Return the AddressSpace corresponding to the specified index */
> --
> 2.41.0
> 
> 

RE: [PATCH 07/22] exec/cpu: Introduce the CPU address space destruction function

[Salil Mehta]

Hi Phillipe,
This patch has been taken as it is from ARM's RFC and the common patch-set mentioned below.
So SOBs are all wrong everywhere.


Original RFC posted in the year 2020.
[1] https://lore.kernel.org/qemu-devel/20200613213629.21984-23-salil.mehta@huawei.com/

Recently posted RFC V2
[2] https://lore.kernel.org/qemu-devel/20230926100436.28284-1-salil.mehta@huawei.com/T/#m5f5ae40b091d69d01012880d7500d96874a9d39c

Recent common patch-set for Virtual CPU Hotplug
[3] https://lore.kernel.org/qemu-devel/20230930001933.2660-9-salil.mehta@huawei.com/

[4] https://lore.kernel.org/qemu-devel/20230930001933.2660-10-salil.mehta@huawei.com/

Beside my original patch-set had bug which you have inherited in this patch-set.
 
Thanks
Salil


> From: qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org <qemu-arm-
> bounces+salil.mehta=huawei.com@nongnu.org> On Behalf Of Philippe Mathieu-
> Daudé
> Sent: Monday, September 18, 2023 5:03 PM
> To: qemu-devel@nongnu.org
> Cc: Laurent Vivier <laurent@vivier.eu>; Paolo Bonzini
> <pbonzini@redhat.com>; Max Filippov <jcmvbkbc@gmail.com>; David Hildenbrand
> <david@redhat.com>; Peter Xu <peterx@redhat.com>; Anton Johansson
> <anjo@rev.ng>; Peter Maydell <peter.maydell@linaro.org>;
> kvm@vger.kernel.org; Marek Vasut <marex@denx.de>; David Gibson
> <david@gibson.dropbear.id.au>; Brian Cain <bcain@quicinc.com>; Yoshinori
> Sato <ysato@users.sourceforge.jp>; Edgar E . Iglesias
> <edgar.iglesias@gmail.com>; Claudio Fontana <cfontana@suse.de>; Daniel
> Henrique Barboza <dbarboza@ventanamicro.com>; Artyom Tarasenko
> <atar4qemu@gmail.com>; Marcelo Tosatti <mtosatti@redhat.com>; qemu-
> ppc@nongnu.org; Liu Zhiwei <zhiwei_liu@linux.alibaba.com>; Aurelien Jarno
> <aurelien@aurel32.net>; Ilya Leoshkevich <iii@linux.ibm.com>; Daniel
> Henrique Barboza <danielhb413@gmail.com>; Bastian Koppelmann
> <kbastian@mail.uni-paderborn.de>; Cédric Le Goater <clg@kaod.org>; Alistair
> Francis <alistair.francis@wdc.com>; Alessandro Di Federico <ale@rev.ng>;
> Song Gao <gaosong@loongson.cn>; Marcel Apfelbaum
> <marcel.apfelbaum@gmail.com>; Chris Wulff <crwulff@gmail.com>; Michael S.
> Tsirkin <mst@redhat.com>; Alistair Francis <alistair@alistair23.me>;
> Fabiano Rosas <farosas@suse.de>; qemu-s390x@nongnu.org; wangyanan (Y)
> <wangyanan55@huawei.com>; Luc Michel <luc@lmichel.fr>; Weiwei Li
> <liweiwei@iscas.ac.cn>; Bin Meng <bin.meng@windriver.com>; Stafford Horne
> <shorne@gmail.com>; Xiaojuan Yang <yangxiaojuan@loongson.cn>; Daniel P .
> Berrange <berrange@redhat.com>; Thomas Huth <thuth@redhat.com>; Philippe
> Mathieu-Daudé <philmd@linaro.org>; qemu-arm@nongnu.org; Jiaxun Yang
> <jiaxun.yang@flygoat.com>; Richard Henderson
> <richard.henderson@linaro.org>; Aleksandar Rikalo
> <aleksandar.rikalo@syrmia.com>; Bernhard Beschow <shentey@gmail.com>; Mark
> Cave-Ayland <mark.cave-ayland@ilande.co.uk>; qemu-riscv@nongnu.org; Alex
> Bennée <alex.bennee@linaro.org>; Nicholas Piggin <npiggin@gmail.com>; Greg
> Kurz <groug@kaod.org>; Michael Rolnik <mrolnik@gmail.com>; Eduardo Habkost
> <eduardo@habkost.net>; Markus Armbruster <armbru@redhat.com>; Palmer
> Dabbelt <palmer@dabbelt.com>; xianglai li <lixianglai@loongson.cn>; Salil
> Mehta <salil.mehta@opnsrc.net>; Igor Mammedov <imammedo@redhat.com>; Ani
> Sinha <anisinha@redhat.com>; Bibo Mao <maobibo@loongson.cn>
> Subject: [PATCH 07/22] exec/cpu: Introduce the CPU address space
> destruction function
> 
> From: xianglai li <lixianglai@loongson.cn>
> 
> Introduce new function to destroy CPU address space resources
> for cpu hot-(un)plug.
> 
> Co-authored-by: "Salil Mehta" <salil.mehta@opnsrc.net>
> Cc: "Salil Mehta" <salil.mehta@opnsrc.net>
> Cc: Xiaojuan Yang <yangxiaojuan@loongson.cn>
> Cc: Song Gao <gaosong@loongson.cn>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Ani Sinha <anisinha@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Eduardo Habkost <eduardo@habkost.net>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Cc: "Philippe Mathieu-Daudé" <philmd@linaro.org>
> Cc: Yanan Wang <wangyanan55@huawei.com>
> Cc: "Daniel P. Berrangé" <berrange@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Bibo Mao <maobibo@loongson.cn>
> Signed-off-by: xianglai li <lixianglai@loongson.cn>
> Message-ID:
> <3a4fc2a3df4b767c3c296a7da3bc15ca9c251316.1694433326.git.lixianglai@loongso
> n.cn>
> ---
>  include/exec/cpu-common.h |  8 ++++++++
>  include/hw/core/cpu.h     |  1 +
>  softmmu/physmem.c         | 24 ++++++++++++++++++++++++
>  3 files changed, 33 insertions(+)
> 
> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index 41788c0bdd..eb56a228a2 100644
> --- a/include/exec/cpu-common.h
> +++ b/include/exec/cpu-common.h
> @@ -120,6 +120,14 @@ size_t qemu_ram_pagesize_largest(void);
>   */
>  void cpu_address_space_init(CPUState *cpu, int asidx,
>                              const char *prefix, MemoryRegion *mr);
> +/**
> + * cpu_address_space_destroy:
> + * @cpu: CPU for which address space needs to be destroyed
> + * @asidx: integer index of this address space
> + *
> + * Note that with KVM only one address space is supported.
> + */
> +void cpu_address_space_destroy(CPUState *cpu, int asidx);
> 
>  void cpu_physical_memory_rw(hwaddr addr, void *buf,
>                              hwaddr len, bool is_write);
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 92a4234439..c90cf3a162 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -366,6 +366,7 @@ struct CPUState {
>      QSIMPLEQ_HEAD(, qemu_work_item) work_list;
> 
>      CPUAddressSpace *cpu_ases;
> +    int cpu_ases_ref_count;
>      int num_ases;
>      AddressSpace *as;
>      MemoryRegion *memory;
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index 18277ddd67..c75e3e8042 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -761,6 +761,7 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
> 
>      if (!cpu->cpu_ases) {
>          cpu->cpu_ases = g_new0(CPUAddressSpace, cpu->num_ases);
> +        cpu->cpu_ases_ref_count = cpu->num_ases;
>      }
> 
>      newas = &cpu->cpu_ases[asidx];
> @@ -774,6 +775,29 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
>      }
>  }
> 
> +void cpu_address_space_destroy(CPUState *cpu, int asidx)
> +{
> +    CPUAddressSpace *cpuas;
> +
> +    assert(asidx < cpu->num_ases);
> +    assert(asidx == 0 || !kvm_enabled());
> +    assert(cpu->cpu_ases);
> +
> +    cpuas = &cpu->cpu_ases[asidx];
> +    if (tcg_enabled()) {
> +        memory_listener_unregister(&cpuas->tcg_as_listener);
> +    }
> +
> +    address_space_destroy(cpuas->as);
> +
> +    cpu->cpu_ases_ref_count--;
> +    if (cpu->cpu_ases_ref_count == 0) {
> +        g_free(cpu->cpu_ases);
> +        cpu->cpu_ases = NULL;
> +    }
> +
> +}
> +
>  AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx)
>  {
>      /* Return the AddressSpace corresponding to the specified index */
> --
> 2.41.0
> 
> 

[PATCH 08/22] exec/cpu: RFC Destroy vCPU address spaces in cpu_common_unrealize()

[Philippe Mathieu-Daudé]

We create at least one vCPU address space by default in
qemu_init_vcpu(), itself called in cpu_common_realizefn().
Since we don't have qemu_deinit_vcpu() helper (we probably
don't need any), simply destroy all the address spaces in
cpu_common_unrealizefn(), *after* the thread is destroyed.

Note: all targets were leaking the vCPU address spaces upon
vCPU unrealize (like hot-unplugged actions).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/core/cpu-common.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index e5841c59df..35c0cc4dad 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -224,6 +224,11 @@ static void cpu_common_unrealizefn(DeviceState *dev)
 
     /* Destroy vCPU thread */
     cpu_remove_sync(cpu);
+
+    /* Destroy CPU address space */
+    for (unsigned idx = 0; idx < cpu->num_ases; idx++) {
+        cpu_address_space_destroy(cpu, idx);
+    }
 }
 
 static void cpu_common_initfn(Object *obj)
-- 
2.41.0

Re: [PATCH 08/22] exec/cpu: RFC Destroy vCPU address spaces in cpu_common_unrealize()

[Richard Henderson]

On 9/18/23 09:02, Philippe Mathieu-Daudé wrote:
> +++ b/hw/core/cpu-common.c
> @@ -224,6 +224,11 @@ static void cpu_common_unrealizefn(DeviceState *dev)
>   
>       /* Destroy vCPU thread */
>       cpu_remove_sync(cpu);
> +
> +    /* Destroy CPU address space */
> +    for (unsigned idx = 0; idx < cpu->num_ases; idx++) {
> +        cpu_address_space_destroy(cpu, idx);
> +    }


Merged with the previous patch, with the single call.


r~

[PATCH 09/22] target/arm: Create timers *after* accelerator vCPU is realized

[Philippe Mathieu-Daudé]

Architecture specific hardware doesn't have a particular dependency
on the accelerator vCPU (created with cpu_exec_realizefn), and can
be initialized *after* the vCPU is realized. Doing so allows further
generic API simplification (in few commits).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/arm/cpu.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index fc3772025c..46d3f70d63 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1748,7 +1748,15 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
             return;
         }
     }
+#endif
 
+    cpu_exec_realizefn(cs, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+#ifndef CONFIG_USER_ONLY
     {
         uint64_t scale;
 
@@ -1776,12 +1784,6 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 #endif
 
-    cpu_exec_realizefn(cs, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
-    }
-
     arm_cpu_finalize_features(cpu, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
-- 
2.41.0

Re: [PATCH 09/22] target/arm: Create timers *after* accelerator vCPU is realized

[Richard Henderson]

On 9/18/23 09:02, Philippe Mathieu-Daudé wrote:
> Architecture specific hardware doesn't have a particular dependency
> on the accelerator vCPU (created with cpu_exec_realizefn), and can
> be initialized*after*  the vCPU is realized. Doing so allows further
> generic API simplification (in few commits).
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   target/arm/cpu.c | 14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)

Hmm.  I suppose so.  Considering we have no error return from timer_new().

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[PATCH 10/22] target/hppa: Create timer *after* accelerator vCPU is realized

[Philippe Mathieu-Daudé]

Architecture specific hardware doesn't have a particular dependency
on the accelerator vCPU (created with cpu_exec_realizefn), and can
be initialized *after* the vCPU is realized. Doing so allows further
generic API simplification (in few commits).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/hppa/cpu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c
index 49082bd2ba..b0d106b6c7 100644
--- a/target/hppa/cpu.c
+++ b/target/hppa/cpu.c
@@ -131,8 +131,6 @@ static void hppa_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    acc->parent_realize(dev, errp);
-
 #ifndef CONFIG_USER_ONLY
     {
         HPPACPU *cpu = HPPA_CPU(cs);
@@ -140,6 +138,8 @@ static void hppa_cpu_realizefn(DeviceState *dev, Error **errp)
                                         hppa_cpu_alarm_timer, cpu);
     }
 #endif
+
+    acc->parent_realize(dev, errp);
 }
 
 static void hppa_cpu_initfn(Object *obj)
-- 
2.41.0

Re: [PATCH 10/22] target/hppa: Create timer *after* accelerator vCPU is realized

[Richard Henderson]

On 9/18/23 09:02, Philippe Mathieu-Daudé wrote:
> Architecture specific hardware doesn't have a particular dependency
> on the accelerator vCPU (created with cpu_exec_realizefn), and can
> be initialized *after* the vCPU is realized. Doing so allows further
> generic API simplification (in few commits).
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/hppa/cpu.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c
> index 49082bd2ba..b0d106b6c7 100644
> --- a/target/hppa/cpu.c
> +++ b/target/hppa/cpu.c
> @@ -131,8 +131,6 @@ static void hppa_cpu_realizefn(DeviceState *dev, Error **errp)
>           return;
>       }
>   
> -    acc->parent_realize(dev, errp);
> -
>   #ifndef CONFIG_USER_ONLY
>       {
>           HPPACPU *cpu = HPPA_CPU(cs);
> @@ -140,6 +138,8 @@ static void hppa_cpu_realizefn(DeviceState *dev, Error **errp)
>                                           hppa_cpu_alarm_timer, cpu);
>       }
>   #endif
> +
> +    acc->parent_realize(dev, errp);
>   }
>   
>   static void hppa_cpu_initfn(Object *obj)

This appears to delay final realization of the vcpu, not advance it...


r~

[PATCH 11/22] target/nios2: Create IRQs *after* accelerator vCPU is realized

[Philippe Mathieu-Daudé]

Architecture specific hardware doesn't have a particular dependency
on the accelerator vCPU (created with cpu_exec_realizefn), and can
be initialized *after* the vCPU is realized. Doing so allows further
generic API simplification (in few commits).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/nios2/cpu.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/target/nios2/cpu.c b/target/nios2/cpu.c
index 7a92fc5f2c..f500ca7ba2 100644
--- a/target/nios2/cpu.c
+++ b/target/nios2/cpu.c
@@ -201,14 +201,6 @@ static void nios2_cpu_realizefn(DeviceState *dev, Error **errp)
     Nios2CPUClass *ncc = NIOS2_CPU_GET_CLASS(dev);
     Error *local_err = NULL;
 
-#ifndef CONFIG_USER_ONLY
-    if (cpu->eic_present) {
-        qdev_init_gpio_in_named(DEVICE(cpu), eic_set_irq, "EIC", 1);
-    } else {
-        qdev_init_gpio_in_named(DEVICE(cpu), iic_set_irq, "IRQ", 32);
-    }
-#endif
-
     cpu_exec_realizefn(cs, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
@@ -220,6 +212,14 @@ static void nios2_cpu_realizefn(DeviceState *dev, Error **errp)
     /* We have reserved storage for cpuid; might as well use it. */
     cpu->env.ctrl[CR_CPUID] = cs->cpu_index;
 
+#ifndef CONFIG_USER_ONLY
+    if (cpu->eic_present) {
+        qdev_init_gpio_in_named(DEVICE(cpu), eic_set_irq, "EIC", 1);
+    } else {
+        qdev_init_gpio_in_named(DEVICE(cpu), iic_set_irq, "IRQ", 32);
+    }
+#endif
+
     ncc->parent_realize(dev, errp);
     cpu_reset(cs);
 }
-- 
2.41.0

Re: [PATCH 11/22] target/nios2: Create IRQs *after* accelerator vCPU is realized

[Richard Henderson]

On 9/18/23 09:02, Philippe Mathieu-Daudé wrote:
> Architecture specific hardware doesn't have a particular dependency
> on the accelerator vCPU (created with cpu_exec_realizefn), and can
> be initialized*after*  the vCPU is realized. Doing so allows further
> generic API simplification (in few commits).
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   target/nios2/cpu.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 12/22] target/mips: Create clock *after* accelerator vCPU is realized

[Philippe Mathieu-Daudé]

Architecture specific hardware doesn't have a particular dependency
on the accelerator vCPU (created with cpu_exec_realizefn), and can
be initialized *after* the vCPU is realized. Doing so allows further
generic API simplification (in few commits).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/mips/cpu.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index 0aea69aaf9..7c81e6c356 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -464,20 +464,6 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
     MIPSCPUClass *mcc = MIPS_CPU_GET_CLASS(dev);
     Error *local_err = NULL;
 
-    if (!clock_get(cpu->clock)) {
-#ifndef CONFIG_USER_ONLY
-        if (!qtest_enabled()) {
-            g_autofree char *cpu_freq_str = freq_to_str(CPU_FREQ_HZ_DEFAULT);
-
-            warn_report("CPU input clock is not connected to any output clock, "
-                        "using default frequency of %s.", cpu_freq_str);
-        }
-#endif
-        /* Initialize the frequency in case the clock remains unconnected. */
-        clock_set_hz(cpu->clock, CPU_FREQ_HZ_DEFAULT);
-    }
-    mips_cp0_period_set(cpu);
-
     cpu_exec_realizefn(cs, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
@@ -492,6 +478,20 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
     fpu_init(env, env->cpu_model);
     mvp_init(env);
 
+    if (!clock_get(cpu->clock)) {
+#ifndef CONFIG_USER_ONLY
+        if (!qtest_enabled()) {
+            g_autofree char *cpu_freq_str = freq_to_str(CPU_FREQ_HZ_DEFAULT);
+
+            warn_report("CPU input clock is not connected to any output clock, "
+                        "using default frequency of %s.", cpu_freq_str);
+        }
+#endif
+        /* Initialize the frequency in case the clock remains unconnected. */
+        clock_set_hz(cpu->clock, CPU_FREQ_HZ_DEFAULT);
+    }
+    mips_cp0_period_set(cpu);
+
     mcc->parent_realize(dev, errp);
     cpu_reset(cs);
 }
-- 
2.41.0

Re: [PATCH 12/22] target/mips: Create clock *after* accelerator vCPU is realized

[Richard Henderson]

On 9/18/23 09:02, Philippe Mathieu-Daudé wrote:
> Architecture specific hardware doesn't have a particular dependency
> on the accelerator vCPU (created with cpu_exec_realizefn), and can
> be initialized*after*  the vCPU is realized. Doing so allows further
> generic API simplification (in few commits).
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   target/mips/cpu.c | 28 ++++++++++++++--------------
>   1 file changed, 14 insertions(+), 14 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 13/22] target/xtensa: Create IRQs *after* accelerator vCPU is realized

[Philippe Mathieu-Daudé]

Architecture specific hardware doesn't have a particular dependency
on the accelerator vCPU (created with cpu_exec_realizefn), and can
be initialized *after* the vCPU is realized. Doing so allows further
generic API simplification (in few commits).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/xtensa/cpu.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
index 300d19d45c..bbfd2d42a8 100644
--- a/target/xtensa/cpu.c
+++ b/target/xtensa/cpu.c
@@ -162,10 +162,6 @@ static void xtensa_cpu_realizefn(DeviceState *dev, Error **errp)
     XtensaCPUClass *xcc = XTENSA_CPU_GET_CLASS(dev);
     Error *local_err = NULL;
 
-#ifndef CONFIG_USER_ONLY
-    xtensa_irq_init(&XTENSA_CPU(dev)->env);
-#endif
-
     cpu_exec_realizefn(cs, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
@@ -174,6 +170,10 @@ static void xtensa_cpu_realizefn(DeviceState *dev, Error **errp)
 
     cs->gdb_num_regs = xcc->config->gdb_regmap.num_regs;
 
+#ifndef CONFIG_USER_ONLY
+    xtensa_irq_init(&XTENSA_CPU(dev)->env);
+#endif
+
     xcc->parent_realize(dev, errp);
 }
 
-- 
2.41.0

Re: [PATCH 13/22] target/xtensa: Create IRQs *after* accelerator vCPU is realized

[Richard Henderson]

On 9/18/23 09:02, Philippe Mathieu-Daudé wrote:
> Architecture specific hardware doesn't have a particular dependency
> on the accelerator vCPU (created with cpu_exec_realizefn), and can
> be initialized*after*  the vCPU is realized. Doing so allows further
> generic API simplification (in few commits).
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   target/xtensa/cpu.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 14/22] target/sparc: Init CPU environment *after* accelerator vCPU is realized

[Philippe Mathieu-Daudé]

These fields from the environment don't affect how accelerators
create their vCPU thread. We can safely reorder them *after* the
cpu_exec_realizefn() call. Doing so allows further generic API
simplification (in the next commit).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/sparc/cpu.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
index 2fdc95eda9..88157fcd33 100644
--- a/target/sparc/cpu.c
+++ b/target/sparc/cpu.c
@@ -756,6 +756,12 @@ static void sparc_cpu_realizefn(DeviceState *dev, Error **errp)
     SPARCCPU *cpu = SPARC_CPU(dev);
     CPUSPARCState *env = &cpu->env;
 
+    cpu_exec_realizefn(cs, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
 #if defined(CONFIG_USER_ONLY)
     if ((env->def.features & CPU_FEATURE_FLOAT)) {
         env->def.features |= CPU_FEATURE_FLOAT128;
@@ -776,12 +782,6 @@ static void sparc_cpu_realizefn(DeviceState *dev, Error **errp)
     env->version |= env->def.nwindows - 1;
 #endif
 
-    cpu_exec_realizefn(cs, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
-    }
-
     scc->parent_realize(dev, errp);
 }
 
-- 
2.41.0

Re: [PATCH 14/22] target/sparc: Init CPU environment *after* accelerator vCPU is realized

[Richard Henderson]

On 9/18/23 09:02, Philippe Mathieu-Daudé wrote:
> These fields from the environment don't affect how accelerators
> create their vCPU thread. We can safely reorder them*after*  the
> cpu_exec_realizefn() call. Doing so allows further generic API
> simplification (in the next commit).
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   target/sparc/cpu.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 15/22] exec/cpu: Introduce CPUClass::verify_accel_features()

[Philippe Mathieu-Daudé]

Some CPUs need to check the requested features are compatible
with the requested accelerator. This has to be done *before*
the accelerator realizes a vCPU.
Introduce the verify_accel_features() handler and call it
just before accel_cpu_realizefn().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/core/cpu.h | 4 ++++
 cpu.c                 | 5 +++++
 2 files changed, 9 insertions(+)

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index c90cf3a162..1e940f6bb5 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -103,6 +103,9 @@ struct SysemuCPUOps;
  * @class_by_name: Callback to map -cpu command line model name to an
  * instantiatable CPU type.
  * @parse_features: Callback to parse command line arguments.
+ * @verify_accel_features: Callback to verify if all requested CPU are
+ *        compatible with the requested accelerator. Called before the
+ *        accelerator realize a vCPU.
  * @reset_dump_flags: #CPUDumpFlags to use for reset logging.
  * @has_work: Callback for checking if there is work to do.
  * @memory_rw_debug: Callback for GDB memory access.
@@ -183,6 +186,7 @@ struct CPUClass {
      * class data that depends on the accelerator, see accel/accel-common.c.
      */
     void (*init_accel_cpu)(struct AccelCPUClass *accel_cpu, CPUClass *cc);
+    bool (*verify_accel_features)(CPUState *cs, Error **errp);
 
     /*
      * Keep non-pointer data at the end to minimize holes.
diff --git a/cpu.c b/cpu.c
index 0769b0b153..84b03c09ac 100644
--- a/cpu.c
+++ b/cpu.c
@@ -136,6 +136,11 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp)
     /* cache the cpu class for the hotpath */
     cpu->cc = CPU_GET_CLASS(cpu);
 
+    if (cpu->cc->verify_accel_features
+        && !cpu->cc->verify_accel_features(cpu, errp)) {
+        return false;
+    }
+
     if (!accel_cpu_realizefn(cpu, errp)) {
         return;
     }
-- 
2.41.0

Re: [PATCH 15/22] exec/cpu: Introduce CPUClass::verify_accel_features()

[Richard Henderson]

On 9/18/23 09:02, Philippe Mathieu-Daudé wrote:
> Some CPUs need to check the requested features are compatible
> with the requested accelerator. This has to be done*before*
> the accelerator realizes a vCPU.
> Introduce the verify_accel_features() handler and call it
> just before accel_cpu_realizefn().
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   include/hw/core/cpu.h | 4 ++++
>   cpu.c                 | 5 +++++
>   2 files changed, 9 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 16/22] target/arm: Extract verify_accel_features() from cpu_realize()

[Philippe Mathieu-Daudé]

When looking at the arm_cpu_realizefn() method, most of the
code run before the cpu_exec_realizefn() call checks whether
the requested CPU features are compatible with the requested
accelerator. Extract this code to a dedicated handler matching
our recently added CPUClass::verify_accel_features() handler.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/arm/cpu.c | 41 ++++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 46d3f70d63..a551383fd3 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1675,19 +1675,10 @@ void arm_cpu_finalize_features(ARMCPU *cpu, Error **errp)
     }
 }
 
-static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
+static bool arm_cpu_verify_accel_features(CPUState *cs, Error **errp)
 {
-    CPUState *cs = CPU(dev);
-    ARMCPU *cpu = ARM_CPU(dev);
-    ARMCPUClass *acc = ARM_CPU_GET_CLASS(dev);
+    ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
-    int pagebits;
-    Error *local_err = NULL;
-
-    /* Use pc-relative instructions in system-mode */
-#ifndef CONFIG_USER_ONLY
-    cs->tcg_cflags |= CF_PCREL;
-#endif
 
     /* If we needed to query the host kernel for the CPU features
      * then it's possible that might have failed in the initfn, but
@@ -1699,10 +1690,13 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
         } else {
             error_setg(errp, "Failed to retrieve host CPU features");
         }
-        return;
+        return false;
     }
 
 #ifndef CONFIG_USER_ONLY
+    /* Use pc-relative instructions in system-mode */
+    cs->tcg_cflags |= CF_PCREL;
+
     /* The NVIC and M-profile CPU are two halves of a single piece of
      * hardware; trying to use one without the other is a command line
      * error and will result in segfaults if not caught here.
@@ -1710,12 +1704,12 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
     if (arm_feature(env, ARM_FEATURE_M)) {
         if (!env->nvic) {
             error_setg(errp, "This board cannot be used with Cortex-M CPUs");
-            return;
+            return false;
         }
     } else {
         if (env->nvic) {
             error_setg(errp, "This board can only be used with Cortex-M CPUs");
-            return;
+            return false;
         }
     }
 
@@ -1733,23 +1727,35 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
             error_setg(errp,
                        "Cannot enable %s when using an M-profile guest CPU",
                        current_accel_name());
-            return;
+            return false;
         }
         if (cpu->has_el3) {
             error_setg(errp,
                        "Cannot enable %s when guest CPU has EL3 enabled",
                        current_accel_name());
-            return;
+            return false;
         }
         if (cpu->tag_memory) {
             error_setg(errp,
                        "Cannot enable %s when guest CPUs has MTE enabled",
                        current_accel_name());
-            return;
+            return false;
         }
     }
 #endif
 
+    return true;
+}
+
+static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
+{
+    CPUState *cs = CPU(dev);
+    ARMCPU *cpu = ARM_CPU(dev);
+    ARMCPUClass *acc = ARM_CPU_GET_CLASS(dev);
+    CPUARMState *env = &cpu->env;
+    int pagebits;
+    Error *local_err = NULL;
+
     cpu_exec_realizefn(cs, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
@@ -2383,6 +2389,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
                                        &acc->parent_phases);
 
     cc->class_by_name = arm_cpu_class_by_name;
+    cc->verify_accel_features = arm_cpu_verify_accel_features;
     cc->has_work = arm_cpu_has_work;
     cc->dump_state = arm_cpu_dump_state;
     cc->set_pc = arm_cpu_set_pc;
-- 
2.41.0

Re: [PATCH 16/22] target/arm: Extract verify_accel_features() from cpu_realize()

[Richard Henderson]

On 9/18/23 09:02, Philippe Mathieu-Daudé wrote:
> When looking at the arm_cpu_realizefn() method, most of the
> code run before the cpu_exec_realizefn() call checks whether
> the requested CPU features are compatible with the requested
> accelerator. Extract this code to a dedicated handler matching
> our recently added CPUClass::verify_accel_features() handler.

How much verification is this intended to do?
Can realize now be required to succeed?

For instance, the error for unset gt_cntfrq_hz just before timer_new could be moved to 
within this new function.


r~

[PATCH 17/22] target/i386: Extract verify_accel_features() from cpu_realize()

[Philippe Mathieu-Daudé]

When looking at the x86_cpu_realizefn() method, most of the
code run before the cpu_exec_realizefn() call checks whether
the requested CPU features are compatible with the requested
accelerator. Extract this code to a dedicated handler matching
our recently added CPUClass::verify_accel_features() handler.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/i386/cpu.c | 62 +++++++++++++++++++++++++++--------------------
 1 file changed, 36 insertions(+), 26 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index d79797d963..2884733397 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -7208,26 +7208,19 @@ static void x86_cpu_hyperv_realize(X86CPU *cpu)
     cpu->hyperv_limits[2] = 0;
 }
 
-static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
+/*
+ * note: the call to the framework needs to happen after feature expansion,
+ * but before the checks/modifications to ucode_rev, mwait, phys_bits.
+ * These may be set by the accel-specific code,
+ * and the results are subsequently checked / assumed in x86_cpu_realizefn().
+ */
+static bool x86_cpu_verify_accel_features(CPUState *cs, Error **errp)
 {
-    CPUState *cs = CPU(dev);
-    X86CPU *cpu = X86_CPU(dev);
-    X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
+    X86CPU *cpu = X86_CPU(cs);
     CPUX86State *env = &cpu->env;
     Error *local_err = NULL;
-    static bool ht_warned;
     unsigned requested_lbr_fmt;
 
-    /* Use pc-relative instructions in system-mode */
-#ifndef CONFIG_USER_ONLY
-    cs->tcg_cflags |= CF_PCREL;
-#endif
-
-    if (cpu->apic_id == UNASSIGNED_APIC_ID) {
-        error_setg(errp, "apic-id property was not initialized properly");
-        return;
-    }
-
     /*
      * Process Hyper-V enlightenments.
      * Note: this currently has to happen before the expansion of CPU features.
@@ -7236,7 +7229,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 
     x86_cpu_expand_features(cpu, &local_err);
     if (local_err) {
-        goto out;
+        return false;
     }
 
     /*
@@ -7246,7 +7239,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     if (cpu->lbr_fmt != ~PERF_CAP_LBR_FMT) {
         if ((cpu->lbr_fmt & PERF_CAP_LBR_FMT) != cpu->lbr_fmt) {
             error_setg(errp, "invalid lbr-fmt");
-            return;
+            return false;
         }
         env->features[FEAT_PERF_CAPABILITIES] &= ~PERF_CAP_LBR_FMT;
         env->features[FEAT_PERF_CAPABILITIES] |= cpu->lbr_fmt;
@@ -7265,13 +7258,13 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 
         if (!cpu->enable_pmu) {
             error_setg(errp, "vPMU: LBR is unsupported without pmu=on");
-            return;
+            return false;
         }
         if (requested_lbr_fmt != host_lbr_fmt) {
             error_setg(errp, "vPMU: the lbr-fmt value (0x%x) does not match "
                         "the host value (0x%x).",
                         requested_lbr_fmt, host_lbr_fmt);
-            return;
+            return false;
         }
     }
 
@@ -7282,7 +7275,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
                    accel_uses_host_cpuid() ?
                        "Host doesn't support requested features" :
                        "TCG doesn't support requested features");
-        goto out;
+        return false;
     }
 
     /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
@@ -7296,12 +7289,28 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 
     x86_cpu_set_sgxlepubkeyhash(env);
 
-    /*
-     * note: the call to the framework needs to happen after feature expansion,
-     * but before the checks/modifications to ucode_rev, mwait, phys_bits.
-     * These may be set by the accel-specific code,
-     * and the results are subsequently checked / assumed in this function.
-     */
+    return true;
+}
+
+static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
+{
+    CPUState *cs = CPU(dev);
+    X86CPU *cpu = X86_CPU(dev);
+    X86CPUClass *xcc = X86_CPU_GET_CLASS(dev);
+    CPUX86State *env = &cpu->env;
+    Error *local_err = NULL;
+    static bool ht_warned;
+
+    /* Use pc-relative instructions in system-mode */
+#ifndef CONFIG_USER_ONLY
+    cs->tcg_cflags |= CF_PCREL;
+#endif
+
+    if (cpu->apic_id == UNASSIGNED_APIC_ID) {
+        error_setg(errp, "apic-id property was not initialized properly");
+        return;
+    }
+
     cpu_exec_realizefn(cs, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
@@ -7950,6 +7959,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
 
     cc->class_by_name = x86_cpu_class_by_name;
     cc->parse_features = x86_cpu_parse_featurestr;
+    cc->verify_accel_features = x86_cpu_verify_accel_features;
     cc->has_work = x86_cpu_has_work;
     cc->dump_state = x86_cpu_dump_state;
     cc->set_pc = x86_cpu_set_pc;
-- 
2.41.0

[PATCH 18/22] target/s390x: Call s390_cpu_realize_sysemu from s390_realize_cpu_model

[Philippe Mathieu-Daudé]

s390_cpu_realize_sysemu() runs some checks for the TCG accelerator,
previous to creating the vCPU. s390_realize_cpu_model() also does
run some checks for KVM.
Move the sysemu call to s390_realize_cpu_model(). Having a single
call before cpu_exec_realizefn() will allow us to factor a
verify_accel_features() handler out in a pair of commits.

Directly pass a S390CPU* to s390_cpu_realize_sysemu() to simplify.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/s390x/s390x-internal.h | 2 +-
 target/s390x/cpu-sysemu.c     | 3 +--
 target/s390x/cpu.c            | 6 ------
 target/s390x/cpu_models.c     | 4 ++++
 4 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/target/s390x/s390x-internal.h b/target/s390x/s390x-internal.h
index 825252d728..781ac08458 100644
--- a/target/s390x/s390x-internal.h
+++ b/target/s390x/s390x-internal.h
@@ -241,7 +241,7 @@ uint32_t calc_cc(CPUS390XState *env, uint32_t cc_op, uint64_t src, uint64_t dst,
 unsigned int s390_cpu_halt(S390CPU *cpu);
 void s390_cpu_unhalt(S390CPU *cpu);
 void s390_cpu_init_sysemu(Object *obj);
-bool s390_cpu_realize_sysemu(DeviceState *dev, Error **errp);
+bool s390_cpu_realize_sysemu(S390CPU *cpu, Error **errp);
 void s390_cpu_finalize(Object *obj);
 void s390_cpu_class_init_sysemu(CPUClass *cc);
 void s390_cpu_machine_reset_cb(void *opaque);
diff --git a/target/s390x/cpu-sysemu.c b/target/s390x/cpu-sysemu.c
index 8112561e5e..5178736c46 100644
--- a/target/s390x/cpu-sysemu.c
+++ b/target/s390x/cpu-sysemu.c
@@ -122,9 +122,8 @@ void s390_cpu_init_sysemu(Object *obj)
     s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
 }
 
-bool s390_cpu_realize_sysemu(DeviceState *dev, Error **errp)
+bool s390_cpu_realize_sysemu(S390CPU *cpu, Error **errp)
 {
-    S390CPU *cpu = S390_CPU(dev);
     MachineState *ms = MACHINE(qdev_get_machine());
     unsigned int max_cpus = ms->smp.max_cpus;
 
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 416ac6c4e0..7257d4bc19 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -237,12 +237,6 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
         goto out;
     }
 
-#if !defined(CONFIG_USER_ONLY)
-    if (!s390_cpu_realize_sysemu(dev, &err)) {
-        goto out;
-    }
-#endif
-
     cpu_exec_realizefn(cs, &err);
     if (err != NULL) {
         goto out;
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 98f14c09c2..f030be0d55 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -612,6 +612,10 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
         cpu->env.cpuid = deposit64(cpu->env.cpuid, CPU_PHYS_ADDR_SHIFT,
                                    CPU_PHYS_ADDR_BITS, cpu->env.core_id);
     }
+
+    if (!s390_cpu_realize_sysemu(cpu, &err)) {
+        return;
+    }
 #endif
 }
 
-- 
2.41.0

Re: [PATCH 18/22] target/s390x: Call s390_cpu_realize_sysemu from s390_realize_cpu_model

[David Hildenbrand]

On 18.09.23 18:02, Philippe Mathieu-Daudé wrote:
> s390_cpu_realize_sysemu() runs some checks for the TCG accelerator,
> previous to creating the vCPU. s390_realize_cpu_model() also does
> run some checks for KVM.
> Move the sysemu call to s390_realize_cpu_model(). Having a single
> call before cpu_exec_realizefn() will allow us to factor a
> verify_accel_features() handler out in a pair of commits.
> 
> Directly pass a S390CPU* to s390_cpu_realize_sysemu() to simplify.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/s390x/s390x-internal.h | 2 +-
>   target/s390x/cpu-sysemu.c     | 3 +--
>   target/s390x/cpu.c            | 6 ------
>   target/s390x/cpu_models.c     | 4 ++++
>   4 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/target/s390x/s390x-internal.h b/target/s390x/s390x-internal.h
> index 825252d728..781ac08458 100644
> --- a/target/s390x/s390x-internal.h
> +++ b/target/s390x/s390x-internal.h
> @@ -241,7 +241,7 @@ uint32_t calc_cc(CPUS390XState *env, uint32_t cc_op, uint64_t src, uint64_t dst,
>   unsigned int s390_cpu_halt(S390CPU *cpu);
>   void s390_cpu_unhalt(S390CPU *cpu);
>   void s390_cpu_init_sysemu(Object *obj);
> -bool s390_cpu_realize_sysemu(DeviceState *dev, Error **errp);
> +bool s390_cpu_realize_sysemu(S390CPU *cpu, Error **errp);
>   void s390_cpu_finalize(Object *obj);
>   void s390_cpu_class_init_sysemu(CPUClass *cc);
>   void s390_cpu_machine_reset_cb(void *opaque);
> diff --git a/target/s390x/cpu-sysemu.c b/target/s390x/cpu-sysemu.c
> index 8112561e5e..5178736c46 100644
> --- a/target/s390x/cpu-sysemu.c
> +++ b/target/s390x/cpu-sysemu.c
> @@ -122,9 +122,8 @@ void s390_cpu_init_sysemu(Object *obj)
>       s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
>   }
>   
> -bool s390_cpu_realize_sysemu(DeviceState *dev, Error **errp)
> +bool s390_cpu_realize_sysemu(S390CPU *cpu, Error **errp)
>   {
> -    S390CPU *cpu = S390_CPU(dev);
>       MachineState *ms = MACHINE(qdev_get_machine());
>       unsigned int max_cpus = ms->smp.max_cpus;
>   
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 416ac6c4e0..7257d4bc19 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -237,12 +237,6 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>           goto out;
>       }
>   
> -#if !defined(CONFIG_USER_ONLY)
> -    if (!s390_cpu_realize_sysemu(dev, &err)) {
> -        goto out;
> -    }
> -#endif
> -
>       cpu_exec_realizefn(cs, &err);
>       if (err != NULL) {
>           goto out;
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index 98f14c09c2..f030be0d55 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -612,6 +612,10 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
>           cpu->env.cpuid = deposit64(cpu->env.cpuid, CPU_PHYS_ADDR_SHIFT,
>                                      CPU_PHYS_ADDR_BITS, cpu->env.core_id);
>       }
> +
> +    if (!s390_cpu_realize_sysemu(cpu, &err)) {
> +        return;
> +    }
>   #endif
>   }
>   

That has nothing to do with CPU models and is, therefore, completely 
misplaced ... :/

Or what am I missing?

-- 
Cheers,

David / dhildenb

[PATCH 19/22] target/s390x: Have s390_realize_cpu_model() return a boolean

[Philippe Mathieu-Daudé]

Following the example documented since commit e3fe3988d7 ("error:
Document Error API usage rules"), have s390_realize_cpu_model()
return a boolean indicating whether an error is set or not.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/s390x/s390x-internal.h |  2 +-
 target/s390x/cpu.c            |  3 +--
 target/s390x/cpu_models.c     | 12 +++++++-----
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/target/s390x/s390x-internal.h b/target/s390x/s390x-internal.h
index 781ac08458..67f21f53a9 100644
--- a/target/s390x/s390x-internal.h
+++ b/target/s390x/s390x-internal.h
@@ -260,7 +260,7 @@ static inline void s390_cpu_unhalt(S390CPU *cpu)
 
 /* cpu_models.c */
 void s390_cpu_model_class_register_props(ObjectClass *oc);
-void s390_realize_cpu_model(CPUState *cs, Error **errp);
+bool s390_realize_cpu_model(CPUState *cs, Error **errp);
 S390CPUModel *get_max_cpu_model(Error **errp);
 void apply_cpu_model(const S390CPUModel *model, Error **errp);
 ObjectClass *s390_cpu_class_by_name(const char *name);
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 7257d4bc19..1a44a6d2b2 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -232,8 +232,7 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
     Error *err = NULL;
 
     /* the model has to be realized before qemu_init_vcpu() due to kvm */
-    s390_realize_cpu_model(cs, &err);
-    if (err) {
+    if (!s390_realize_cpu_model(cs, &err)) {
         goto out;
     }
 
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index f030be0d55..0605073dc3 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -567,7 +567,7 @@ S390CPUModel *get_max_cpu_model(Error **errp)
     return &max_model;
 }
 
-void s390_realize_cpu_model(CPUState *cs, Error **errp)
+bool s390_realize_cpu_model(CPUState *cs, Error **errp)
 {
     Error *err = NULL;
     S390CPUClass *xcc = S390_CPU_GET_CLASS(cs);
@@ -576,19 +576,19 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
 
     if (xcc->kvm_required && !kvm_enabled()) {
         error_setg(errp, "CPU definition requires KVM");
-        return;
+        return false;
     }
 
     if (!cpu->model) {
         /* no host model support -> perform compatibility stuff */
         apply_cpu_model(NULL, errp);
-        return;
+        return false;
     }
 
     max_model = get_max_cpu_model(errp);
     if (!max_model) {
         error_prepend(errp, "CPU models are not available: ");
-        return;
+        return false;
     }
 
     /* copy over properties that can vary */
@@ -601,7 +601,7 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
     check_compatibility(max_model, cpu->model, &err);
     if (err) {
         error_propagate(errp, err);
-        return;
+        return false;
     }
 
     apply_cpu_model(cpu->model, errp);
@@ -617,6 +617,8 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
         return;
     }
 #endif
+
+    return true;
 }
 
 static void get_feature(Object *obj, Visitor *v, const char *name,
-- 
2.41.0

[PATCH 20/22] target/s390x: Use s390_realize_cpu_model() as verify_accel_features()

[Philippe Mathieu-Daudé]

s390_realize_cpu_model() checks if CPU model and definitions
are compatible with the KVM / TCG accelerators, before realizing
the vCPU. Use it directly as CPUClass::verify_accel_features()
handler (called from cpu_exec_realizefn()).

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/s390x/cpu.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 1a44a6d2b2..983dbfe563 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -231,11 +231,6 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
     S390CPUClass *scc = S390_CPU_GET_CLASS(dev);
     Error *err = NULL;
 
-    /* the model has to be realized before qemu_init_vcpu() due to kvm */
-    if (!s390_realize_cpu_model(cs, &err)) {
-        goto out;
-    }
-
     cpu_exec_realizefn(cs, &err);
     if (err != NULL) {
         goto out;
@@ -329,6 +324,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
 
     scc->reset = s390_cpu_reset;
     cc->class_by_name = s390_cpu_class_by_name,
+    cc->verify_accel_features = s390_realize_cpu_model;
     cc->has_work = s390_cpu_has_work;
     cc->dump_state = s390_cpu_dump_state;
     cc->query_cpu_fast = s390_query_cpu_fast;
-- 
2.41.0

[PATCH 21/22] exec/cpu: Have cpu_exec_realize() return a boolean

[Philippe Mathieu-Daudé]

Following the example documented since commit e3fe3988d7 ("error:
Document Error API usage rules"), have cpu_exec_realizefn()
return a boolean indicating whether an error is set or not.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/core/cpu.h | 2 +-
 cpu.c                 | 6 ++++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 1e940f6bb5..3dc6968428 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -1014,7 +1014,7 @@ G_NORETURN void cpu_abort(CPUState *cpu, const char *fmt, ...)
 /* $(top_srcdir)/cpu.c */
 void cpu_class_init_props(DeviceClass *dc);
 void cpu_exec_initfn(CPUState *cpu);
-void cpu_exec_realizefn(CPUState *cpu, Error **errp);
+bool cpu_exec_realizefn(CPUState *cpu, Error **errp);
 void cpu_exec_unrealizefn(CPUState *cpu);
 
 /**
diff --git a/cpu.c b/cpu.c
index 84b03c09ac..96ae440b81 100644
--- a/cpu.c
+++ b/cpu.c
@@ -131,7 +131,7 @@ const VMStateDescription vmstate_cpu_common = {
 };
 #endif
 
-void cpu_exec_realizefn(CPUState *cpu, Error **errp)
+bool cpu_exec_realizefn(CPUState *cpu, Error **errp)
 {
     /* cache the cpu class for the hotpath */
     cpu->cc = CPU_GET_CLASS(cpu);
@@ -142,7 +142,7 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp)
     }
 
     if (!accel_cpu_realizefn(cpu, errp)) {
-        return;
+        return false;
     }
 
     /* NB: errp parameter is unused currently */
@@ -169,6 +169,8 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp)
         vmstate_register(NULL, cpu->cpu_index, cpu->cc->sysemu_ops->legacy_vmsd, cpu);
     }
 #endif /* CONFIG_USER_ONLY */
+
+    return true;
 }
 
 void cpu_exec_unrealizefn(CPUState *cpu)
-- 
2.41.0

Re: [PATCH 21/22] exec/cpu: Have cpu_exec_realize() return a boolean

[Richard Henderson]

On 9/18/23 09:02, Philippe Mathieu-Daudé wrote:
> Following the example documented since commit e3fe3988d7 ("error:
> Document Error API usage rules"), have cpu_exec_realizefn()
> return a boolean indicating whether an error is set or not.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   include/hw/core/cpu.h | 2 +-
>   cpu.c                 | 6 ++++--
>   2 files changed, 5 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 22/22] exec/cpu: Call cpu_exec_realizefn() once in cpu_common_realize()

[Philippe Mathieu-Daudé]

cpu_exec_realizefn() is called in each ${target}_cpu_realize(),
before calling their parent_realize(), which is simply
cpu_common_realizefn(). Directly call it there instead.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/core/cpu-common.c      |  4 ++++
 target/alpha/cpu.c        |  8 --------
 target/arm/cpu.c          |  6 ------
 target/avr/cpu.c          |  7 -------
 target/cris/cpu.c         |  7 -------
 target/hexagon/cpu.c      |  7 -------
 target/hppa/cpu.c         | 15 ++-------------
 target/i386/cpu.c         |  6 ------
 target/i386/kvm/kvm-cpu.c |  4 ++--
 target/loongarch/cpu.c    |  7 -------
 target/m68k/cpu.c         |  7 -------
 target/microblaze/cpu.c   |  7 -------
 target/mips/cpu.c         |  7 -------
 target/nios2/cpu.c        |  7 -------
 target/openrisc/cpu.c     |  7 -------
 target/ppc/cpu_init.c     |  5 -----
 target/riscv/cpu.c        |  6 ------
 target/rx/cpu.c           |  7 -------
 target/s390x/cpu.c        |  5 -----
 target/sh4/cpu.c          |  7 -------
 target/sparc/cpu.c        |  8 --------
 target/tricore/cpu.c      |  7 -------
 target/xtensa/cpu.c       |  7 -------
 23 files changed, 8 insertions(+), 150 deletions(-)

diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index 35c0cc4dad..8901c482a0 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -204,6 +204,10 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp)
         }
     }
 
+    if (!cpu_exec_realizefn(cpu, errp)) {
+        return;
+    }
+
     /* Create CPU address space and vCPU thread */
     qemu_init_vcpu(cpu);
 
diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
index eb78318bb8..85834c4d61 100644
--- a/target/alpha/cpu.c
+++ b/target/alpha/cpu.c
@@ -72,15 +72,7 @@ static void alpha_cpu_disas_set_info(CPUState *cpu, disassemble_info *info)
 
 static void alpha_cpu_realizefn(DeviceState *dev, Error **errp)
 {
-    CPUState *cs = CPU(dev);
     AlphaCPUClass *acc = ALPHA_CPU_GET_CLASS(dev);
-    Error *local_err = NULL;
-
-    cpu_exec_realizefn(cs, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
-    }
 
     acc->parent_realize(dev, errp);
 }
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index a551383fd3..d8eaa186cd 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1756,12 +1756,6 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
     int pagebits;
     Error *local_err = NULL;
 
-    cpu_exec_realizefn(cs, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
-    }
-
 #ifndef CONFIG_USER_ONLY
     {
         uint64_t scale;
diff --git a/target/avr/cpu.c b/target/avr/cpu.c
index d3460b3960..e512ad46d3 100644
--- a/target/avr/cpu.c
+++ b/target/avr/cpu.c
@@ -112,13 +112,6 @@ static void avr_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
     AVRCPUClass *mcc = AVR_CPU_GET_CLASS(dev);
-    Error *local_err = NULL;
-
-    cpu_exec_realizefn(cs, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
-    }
 
     mcc->parent_realize(dev, errp);
     cpu_reset(cs);
diff --git a/target/cris/cpu.c b/target/cris/cpu.c
index 671693a362..9fb69ecda4 100644
--- a/target/cris/cpu.c
+++ b/target/cris/cpu.c
@@ -144,13 +144,6 @@ static void cris_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
     CRISCPUClass *ccc = CRIS_CPU_GET_CLASS(dev);
-    Error *local_err = NULL;
-
-    cpu_exec_realizefn(cs, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
-    }
 
     ccc->parent_realize(dev, errp);
     cpu_reset(cs);
diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c
index 5b9bb3fe83..17785e2921 100644
--- a/target/hexagon/cpu.c
+++ b/target/hexagon/cpu.c
@@ -332,13 +332,6 @@ static void hexagon_cpu_realize(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
     HexagonCPUClass *mcc = HEXAGON_CPU_GET_CLASS(dev);
-    Error *local_err = NULL;
-
-    cpu_exec_realizefn(cs, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
-    }
 
     gdb_register_coprocessor(cs, hexagon_hvx_gdb_read_register,
                              hexagon_hvx_gdb_write_register,
diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c
index b0d106b6c7..a87028b275 100644
--- a/target/hppa/cpu.c
+++ b/target/hppa/cpu.c
@@ -121,22 +121,11 @@ void hppa_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
 
 static void hppa_cpu_realizefn(DeviceState *dev, Error **errp)
 {
-    CPUState *cs = CPU(dev);
     HPPACPUClass *acc = HPPA_CPU_GET_CLASS(dev);
-    Error *local_err = NULL;
-
-    cpu_exec_realizefn(cs, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
-    }
 
 #ifndef CONFIG_USER_ONLY
-    {
-        HPPACPU *cpu = HPPA_CPU(cs);
-        cpu->alarm_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
-                                        hppa_cpu_alarm_timer, cpu);
-    }
+    cpu->alarm_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+                                    hppa_cpu_alarm_timer, HPPA_CPU(dev));
 #endif
 
     acc->parent_realize(dev, errp);
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 2884733397..c170e2976b 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -7311,12 +7311,6 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    cpu_exec_realizefn(cs, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
-    }
-
     if (xcc->host_cpuid_required && !accel_uses_host_cpuid()) {
         g_autofree char *name = x86_cpu_class_get_model_name(xcc);
         error_setg(&local_err, "CPU model '%s' requires KVM or HVF", name);
diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
index 1fe62ce176..0f52649779 100644
--- a/target/i386/kvm/kvm-cpu.c
+++ b/target/i386/kvm/kvm-cpu.c
@@ -34,10 +34,10 @@ static bool kvm_cpu_realizefn(CPUState *cs, Error **errp)
      *
      * x86_cpu_realize():
      *  -> x86_cpu_expand_features()
-     *  -> cpu_exec_realizefn():
+     *  -> cpu_common_realizefn()
+     *      -> cpu_exec_realizefn():
      *            -> accel_cpu_realizefn()
      *               kvm_cpu_realizefn() -> host_cpu_realizefn()
-     *  -> cpu_common_realizefn()
      *  -> check/update ucode_rev, phys_bits, mwait
      */
     if (cpu->max_features) {
diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
index dc0ac39833..d61dcaebca 100644
--- a/target/loongarch/cpu.c
+++ b/target/loongarch/cpu.c
@@ -555,13 +555,6 @@ static void loongarch_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
     LoongArchCPUClass *lacc = LOONGARCH_CPU_GET_CLASS(dev);
-    Error *local_err = NULL;
-
-    cpu_exec_realizefn(cs, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
-    }
 
     loongarch_cpu_register_gdb_regs_for_features(cs);
 
diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index 3da316bc30..c6740e0e78 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -309,16 +309,9 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error **errp)
     CPUState *cs = CPU(dev);
     M68kCPU *cpu = M68K_CPU(dev);
     M68kCPUClass *mcc = M68K_CPU_GET_CLASS(dev);
-    Error *local_err = NULL;
 
     register_m68k_insns(&cpu->env);
 
-    cpu_exec_realizefn(cs, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
-    }
-
     m68k_cpu_init_gdb(cpu);
 
     mcc->parent_realize(dev, errp);
diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c
index 1f19a6e07d..5194911ad4 100644
--- a/target/microblaze/cpu.c
+++ b/target/microblaze/cpu.c
@@ -207,13 +207,6 @@ static void mb_cpu_realizefn(DeviceState *dev, Error **errp)
     uint8_t version_code = 0;
     const char *version;
     int i = 0;
-    Error *local_err = NULL;
-
-    cpu_exec_realizefn(cs, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
-    }
 
     if (cpu->cfg.addr_size < 32 || cpu->cfg.addr_size > 64) {
         error_setg(errp, "addr-size %d is out of range (32 - 64)",
diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index 7c81e6c356..4f15dcea44 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -462,13 +462,6 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
     MIPSCPU *cpu = MIPS_CPU(dev);
     CPUMIPSState *env = &cpu->env;
     MIPSCPUClass *mcc = MIPS_CPU_GET_CLASS(dev);
-    Error *local_err = NULL;
-
-    cpu_exec_realizefn(cs, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
-    }
 
     env->exception_base = (int32_t)0xBFC00000;
 
diff --git a/target/nios2/cpu.c b/target/nios2/cpu.c
index f500ca7ba2..fc753bb1be 100644
--- a/target/nios2/cpu.c
+++ b/target/nios2/cpu.c
@@ -199,13 +199,6 @@ static void nios2_cpu_realizefn(DeviceState *dev, Error **errp)
     CPUState *cs = CPU(dev);
     Nios2CPU *cpu = NIOS2_CPU(cs);
     Nios2CPUClass *ncc = NIOS2_CPU_GET_CLASS(dev);
-    Error *local_err = NULL;
-
-    cpu_exec_realizefn(cs, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
-    }
 
     realize_cr_status(cs);
 
diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c
index e4ec95ca7f..438146c681 100644
--- a/target/openrisc/cpu.c
+++ b/target/openrisc/cpu.c
@@ -133,13 +133,6 @@ static void openrisc_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
     OpenRISCCPUClass *occ = OPENRISC_CPU_GET_CLASS(dev);
-    Error *local_err = NULL;
-
-    cpu_exec_realizefn(cs, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
-    }
 
     occ->parent_realize(dev, errp);
     cpu_reset(cs);
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 24d4e8fa7e..99087ee57c 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -6809,11 +6809,6 @@ static void ppc_cpu_realize(DeviceState *dev, Error **errp)
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
     Error *local_err = NULL;
 
-    cpu_exec_realizefn(cs, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
-    }
     if (cpu->vcpu_id == UNASSIGNED_CPU_INDEX) {
         cpu->vcpu_id = cs->cpu_index;
     }
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 4f7ae55359..62be6d88fc 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1503,12 +1503,6 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
     RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(dev);
     Error *local_err = NULL;
 
-    cpu_exec_realizefn(cs, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
-    }
-
     if (tcg_enabled()) {
         riscv_cpu_realize_tcg(dev, &local_err);
         if (local_err != NULL) {
diff --git a/target/rx/cpu.c b/target/rx/cpu.c
index 089df61790..db951ff988 100644
--- a/target/rx/cpu.c
+++ b/target/rx/cpu.c
@@ -129,13 +129,6 @@ static void rx_cpu_realize(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
     RXCPUClass *rcc = RX_CPU_GET_CLASS(dev);
-    Error *local_err = NULL;
-
-    cpu_exec_realizefn(cs, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
-    }
 
     rcc->parent_realize(dev, errp);
     cpu_reset(cs);
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 983dbfe563..e305928651 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -231,11 +231,6 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
     S390CPUClass *scc = S390_CPU_GET_CLASS(dev);
     Error *err = NULL;
 
-    cpu_exec_realizefn(cs, &err);
-    if (err != NULL) {
-        goto out;
-    }
-
 #if !defined(CONFIG_USER_ONLY)
     qemu_register_reset(s390_cpu_machine_reset_cb, S390_CPU(dev));
 #endif
diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c
index e6690daf9a..a3fc034ea5 100644
--- a/target/sh4/cpu.c
+++ b/target/sh4/cpu.c
@@ -220,13 +220,6 @@ static void superh_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
     SuperHCPUClass *scc = SUPERH_CPU_GET_CLASS(dev);
-    Error *local_err = NULL;
-
-    cpu_exec_realizefn(cs, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
-    }
 
     scc->parent_realize(dev, errp);
     cpu_reset(cs);
diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
index 88157fcd33..f0b2187f3b 100644
--- a/target/sparc/cpu.c
+++ b/target/sparc/cpu.c
@@ -750,18 +750,10 @@ static ObjectClass *sparc_cpu_class_by_name(const char *cpu_model)
 
 static void sparc_cpu_realizefn(DeviceState *dev, Error **errp)
 {
-    CPUState *cs = CPU(dev);
     SPARCCPUClass *scc = SPARC_CPU_GET_CLASS(dev);
-    Error *local_err = NULL;
     SPARCCPU *cpu = SPARC_CPU(dev);
     CPUSPARCState *env = &cpu->env;
 
-    cpu_exec_realizefn(cs, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
-    }
-
 #if defined(CONFIG_USER_ONLY)
     if ((env->def.features & CPU_FEATURE_FLOAT)) {
         env->def.features |= CPU_FEATURE_FLOAT128;
diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c
index 0142cf556d..5319a6841e 100644
--- a/target/tricore/cpu.c
+++ b/target/tricore/cpu.c
@@ -95,13 +95,6 @@ static void tricore_cpu_realizefn(DeviceState *dev, Error **errp)
     TriCoreCPU *cpu = TRICORE_CPU(dev);
     TriCoreCPUClass *tcc = TRICORE_CPU_GET_CLASS(dev);
     CPUTriCoreState *env = &cpu->env;
-    Error *local_err = NULL;
-
-    cpu_exec_realizefn(cs, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
-    }
 
     /* Some features automatically imply others */
     if (tricore_has_feature(env, TRICORE_FEATURE_162)) {
diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
index bbfd2d42a8..c7bdd0980a 100644
--- a/target/xtensa/cpu.c
+++ b/target/xtensa/cpu.c
@@ -160,13 +160,6 @@ static void xtensa_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
     XtensaCPUClass *xcc = XTENSA_CPU_GET_CLASS(dev);
-    Error *local_err = NULL;
-
-    cpu_exec_realizefn(cs, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
-    }
 
     cs->gdb_num_regs = xcc->config->gdb_regmap.num_regs;
 
-- 
2.41.0

Re: [PATCH 22/22] exec/cpu: Call cpu_exec_realizefn() once in cpu_common_realize()

[Richard Henderson]

On 9/18/23 09:02, Philippe Mathieu-Daudé wrote:
> +++ b/target/hppa/cpu.c
> @@ -121,22 +121,11 @@ void hppa_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
>   
>   static void hppa_cpu_realizefn(DeviceState *dev, Error **errp)
>   {
> -    CPUState *cs = CPU(dev);
>       HPPACPUClass *acc = HPPA_CPU_GET_CLASS(dev);
> -    Error *local_err = NULL;
> -
> -    cpu_exec_realizefn(cs, &local_err);
> -    if (local_err != NULL) {
> -        error_propagate(errp, local_err);
> -        return;
> -    }
>   
>   #ifndef CONFIG_USER_ONLY
> -    {
> -        HPPACPU *cpu = HPPA_CPU(cs);
> -        cpu->alarm_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> -                                        hppa_cpu_alarm_timer, cpu);
> -    }
> +    cpu->alarm_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> +                                    hppa_cpu_alarm_timer, HPPA_CPU(dev));
>   #endif
>   
>       acc->parent_realize(dev, errp);

Quoting one example.

In a previous patch you moved cpu_exec_realizefn() earlier, now you're moving it later, 
into parent_realize().  I think the previous patch needs adjustment.


r~