[PATCH] cpu: Add starts_halted() method

[PATCH] cpu: Add starts_halted() method

[Thiago Jung Bauermann]

PowerPC sPAPRs CPUs start in the halted state, but generic QEMU code
assumes that CPUs start in the non-halted state. spapr_reset_vcpu()
attempts to rectify this by setting CPUState::halted to 1. But that's too
late for hotplugged CPUs in a machine configured with 2 or mor threads per
core.

By then, other parts of QEMU have already caused the vCPU to run in an
unitialized state a couple of times. For example, ppc_cpu_reset() calls
ppc_tlb_invalidate_all(), which ends up calling async_run_on_cpu(). This
kicks the new vCPU while it has CPUState::halted = 0, causing QEMU to issue
a KVM_RUN ioctl on the new vCPU before the guest is able to make the
start-cpu RTAS call to initialize its register state.

This doesn't seem to cause visible issues for regular guests, but on a
secure guest running under the Ultravisor it does. The Ultravisor relies on
being able to snoop on the start-cpu RTAS call to map vCPUs to guests, and
this issue causes it to see a stray vCPU that doesn't belong to any guest.

Fix by adding a starts_halted() method to the CPUState class, and making it
return 1 if the machine is an sPAPR guest.

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
---
 hw/core/cpu.c                   |  8 +++++++-
 hw/ppc/spapr_cpu_core.c         |  5 -----
 include/hw/core/cpu.h           |  2 ++
 target/ppc/translate_init.inc.c | 16 ++++++++++++++++
 4 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/hw/core/cpu.c b/hw/core/cpu.c
index 0f23409f1d..8f9a3335d5 100644
--- a/hw/core/cpu.c
+++ b/hw/core/cpu.c
@@ -259,7 +259,7 @@ static void cpu_common_reset(DeviceState *dev)
     }
 
     cpu->interrupt_request = 0;
-    cpu->halted = 0;
+    cpu->halted = cc->starts_halted();
     cpu->mem_io_pc = 0;
     cpu->icount_extra = 0;
     atomic_set(&cpu->icount_decr_ptr->u32, 0);
@@ -275,6 +275,11 @@ static void cpu_common_reset(DeviceState *dev)
     }
 }
 
+static uint32_t cpu_common_starts_halted(void)
+{
+    return 0;
+}
+
 static bool cpu_common_has_work(CPUState *cs)
 {
     return false;
@@ -428,6 +433,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
     k->cpu_exec_exit = cpu_common_noop;
     k->cpu_exec_interrupt = cpu_common_exec_interrupt;
     k->adjust_watchpoint_address = cpu_adjust_watchpoint_address;
+    k->starts_halted = cpu_common_starts_halted;
     set_bit(DEVICE_CATEGORY_CPU, dc->categories);
     dc->realize = cpu_common_realizefn;
     dc->unrealize = cpu_common_unrealizefn;
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 26ad566f42..d0ad92240c 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -36,11 +36,6 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
 
     cpu_reset(cs);
 
-    /* All CPUs start halted.  CPU0 is unhalted from the machine level
-     * reset code and the rest are explicitly started up by the guest
-     * using an RTAS call */
-    cs->halted = 1;
-
     env->spr[SPR_HIOR] = 0;
 
     lpcr = env->spr[SPR_LPCR];
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index b3f4b79318..7c9cd67e8d 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -219,6 +219,8 @@ typedef struct CPUClass {
     vaddr (*adjust_watchpoint_address)(CPUState *cpu, vaddr addr, int len);
     void (*tcg_initialize)(void);
 
+    uint32_t (*starts_halted)(void);
+
     /* Keep non-pointer data at the end to minimize holes.  */
     int gdb_num_core_regs;
     bool gdb_stop_before_watchpoint;
diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index 49212bfd90..1dc1ebbdaf 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -36,6 +36,7 @@
 #include "qapi/visitor.h"
 #include "hw/qdev-properties.h"
 #include "hw/ppc/ppc.h"
+#include "hw/ppc/spapr.h"
 #include "mmu-book3s-v3.h"
 #include "sysemu/qtest.h"
 #include "qemu/cutils.h"
@@ -10646,6 +10647,20 @@ static void ppc_cpu_set_pc(CPUState *cs, vaddr value)
     cpu->env.nip = value;
 }
 
+static uint32_t ppc_cpu_starts_halted(void)
+{
+    SpaprMachineState *spapr =
+        (SpaprMachineState *) object_dynamic_cast(qdev_get_machine(),
+                                                  TYPE_SPAPR_MACHINE);
+
+    /*
+     * In sPAPR, all CPUs start halted. CPU0 is unhalted from the machine level
+     * reset code and the rest are explicitly started up by the guest using an
+     * RTAS call.
+     */
+    return spapr != NULL;
+}
+
 static bool ppc_cpu_has_work(CPUState *cs)
 {
     PowerPCCPU *cpu = POWERPC_CPU(cs);
@@ -10922,6 +10937,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
 #endif
 
     cc->disas_set_info = ppc_disas_set_info;
+    cc->starts_halted = ppc_cpu_starts_halted;
 
     dc->fw_name = "PowerPC,UNKNOWN";
 }

Re: [PATCH] cpu: Add starts_halted() method

[Eduardo Habkost]

On Tue, Jul 07, 2020 at 05:43:33PM -0300, Thiago Jung Bauermann wrote:
> PowerPC sPAPRs CPUs start in the halted state, but generic QEMU code
> assumes that CPUs start in the non-halted state. spapr_reset_vcpu()
> attempts to rectify this by setting CPUState::halted to 1. But that's too
> late for hotplugged CPUs in a machine configured with 2 or mor threads per
> core.
> 
> By then, other parts of QEMU have already caused the vCPU to run in an
> unitialized state a couple of times. For example, ppc_cpu_reset() calls
> ppc_tlb_invalidate_all(), which ends up calling async_run_on_cpu(). This
> kicks the new vCPU while it has CPUState::halted = 0, causing QEMU to issue
> a KVM_RUN ioctl on the new vCPU before the guest is able to make the
> start-cpu RTAS call to initialize its register state.
> 
> This doesn't seem to cause visible issues for regular guests, but on a
> secure guest running under the Ultravisor it does. The Ultravisor relies on
> being able to snoop on the start-cpu RTAS call to map vCPUs to guests, and
> this issue causes it to see a stray vCPU that doesn't belong to any guest.
> 
> Fix by adding a starts_halted() method to the CPUState class, and making it
> return 1 if the machine is an sPAPR guest.
> 
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
[...]
> +static uint32_t ppc_cpu_starts_halted(void)
> +{
> +    SpaprMachineState *spapr =
> +        (SpaprMachineState *) object_dynamic_cast(qdev_get_machine(),
> +                                                  TYPE_SPAPR_MACHINE);

Wouldn't it be simpler to just implement this as a MachineClass
boolean field?  e.g.:

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 426ce5f625..ffadc7a17d 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -215,6 +215,7 @@ struct MachineClass {
     bool nvdimm_supported;
     bool numa_mem_supported;
     bool auto_enable_numa;
+    bool cpu_starts_halted;
     const char *default_ram_id;
 
     HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
diff --git a/hw/core/cpu.c b/hw/core/cpu.c
index 0f23409f1d..08dd504034 100644
--- a/hw/core/cpu.c
+++ b/hw/core/cpu.c
@@ -252,6 +252,7 @@ static void cpu_common_reset(DeviceState *dev)
 {
     CPUState *cpu = CPU(dev);
     CPUClass *cc = CPU_GET_CLASS(cpu);
+    MachineState *machine = object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE);
 
     if (qemu_loglevel_mask(CPU_LOG_RESET)) {
         qemu_log("CPU Reset (CPU %d)\n", cpu->cpu_index);
@@ -259,7 +260,7 @@ static void cpu_common_reset(DeviceState *dev)
     }
 
     cpu->interrupt_request = 0;
-    cpu->halted = 0;
+    cpu->halted = machine ? MACHINE_GET_CLASS(machine)->cpu_starts_halted : 0;
     cpu->mem_io_pc = 0;
     cpu->icount_extra = 0;
     atomic_set(&cpu->icount_decr_ptr->u32, 0);
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f6f034d039..d16ec33033 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4487,6 +4487,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power9_v2.0");
     mc->has_hotpluggable_cpus = true;
     mc->nvdimm_supported = true;
+    mc->cpu_starts_halted = true;
     smc->resize_hpt_default = SPAPR_RESIZE_HPT_ENABLED;
     fwc->get_dev_path = spapr_get_fw_dev_path;
     nc->nmi_monitor_handler = spapr_nmi;

> +
> +    /*
> +     * In sPAPR, all CPUs start halted. CPU0 is unhalted from the machine level
> +     * reset code and the rest are explicitly started up by the guest using an
> +     * RTAS call.
> +     */
> +    return spapr != NULL;
> +}
> +

-- 
Eduardo

Re: [PATCH] cpu: Add starts_halted() method

[Thiago Jung Bauermann]

Hello Eduardo,

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Tue, Jul 07, 2020 at 05:43:33PM -0300, Thiago Jung Bauermann wrote:
>> PowerPC sPAPRs CPUs start in the halted state, but generic QEMU code
>> assumes that CPUs start in the non-halted state. spapr_reset_vcpu()
>> attempts to rectify this by setting CPUState::halted to 1. But that's too
>> late for hotplugged CPUs in a machine configured with 2 or mor threads per
>> core.
>>
>> By then, other parts of QEMU have already caused the vCPU to run in an
>> unitialized state a couple of times. For example, ppc_cpu_reset() calls
>> ppc_tlb_invalidate_all(), which ends up calling async_run_on_cpu(). This
>> kicks the new vCPU while it has CPUState::halted = 0, causing QEMU to issue
>> a KVM_RUN ioctl on the new vCPU before the guest is able to make the
>> start-cpu RTAS call to initialize its register state.
>>
>> This doesn't seem to cause visible issues for regular guests, but on a
>> secure guest running under the Ultravisor it does. The Ultravisor relies on
>> being able to snoop on the start-cpu RTAS call to map vCPUs to guests, and
>> this issue causes it to see a stray vCPU that doesn't belong to any guest.
>>
>> Fix by adding a starts_halted() method to the CPUState class, and making it
>> return 1 if the machine is an sPAPR guest.
>>
>> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> [...]
>> +static uint32_t ppc_cpu_starts_halted(void)
>> +{
>> +    SpaprMachineState *spapr =
>> +        (SpaprMachineState *) object_dynamic_cast(qdev_get_machine(),
>> +                                                  TYPE_SPAPR_MACHINE);
>
> Wouldn't it be simpler to just implement this as a MachineClass
> boolean field?  e.g.:
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Yes, indeed it would. Thanks for this patch. I just tested and it
also solves the problem (except for the nit mentioned below).

Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>

Should I submit a proper patch with these changes (with you as the
author)?

> ---
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 426ce5f625..ffadc7a17d 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -215,6 +215,7 @@ struct MachineClass {
>      bool nvdimm_supported;
>      bool numa_mem_supported;
>      bool auto_enable_numa;
> +    bool cpu_starts_halted;
>      const char *default_ram_id;
>
>      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> diff --git a/hw/core/cpu.c b/hw/core/cpu.c
> index 0f23409f1d..08dd504034 100644
> --- a/hw/core/cpu.c
> +++ b/hw/core/cpu.c
> @@ -252,6 +252,7 @@ static void cpu_common_reset(DeviceState *dev)
>  {
>      CPUState *cpu = CPU(dev);
>      CPUClass *cc = CPU_GET_CLASS(cpu);
> +    MachineState *machine = object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE);

I had to add a (MachineState *) cast here to get the code to compile.

>
>      if (qemu_loglevel_mask(CPU_LOG_RESET)) {
>          qemu_log("CPU Reset (CPU %d)\n", cpu->cpu_index);
> @@ -259,7 +260,7 @@ static void cpu_common_reset(DeviceState *dev)
>      }
>
>      cpu->interrupt_request = 0;
> -    cpu->halted = 0;
> +    cpu->halted = machine ? MACHINE_GET_CLASS(machine)->cpu_starts_halted : 0;
>      cpu->mem_io_pc = 0;
>      cpu->icount_extra = 0;
>      atomic_set(&cpu->icount_decr_ptr->u32, 0);
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f6f034d039..d16ec33033 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4487,6 +4487,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power9_v2.0");
>      mc->has_hotpluggable_cpus = true;
>      mc->nvdimm_supported = true;
> +    mc->cpu_starts_halted = true;
>      smc->resize_hpt_default = SPAPR_RESIZE_HPT_ENABLED;
>      fwc->get_dev_path = spapr_get_fw_dev_path;
>      nc->nmi_monitor_handler = spapr_nmi;
>
>> +
>> +    /*
>> +     * In sPAPR, all CPUs start halted. CPU0 is unhalted from the machine level
>> +     * reset code and the rest are explicitly started up by the guest using an
>> +     * RTAS call.
>> +     */
>> +    return spapr != NULL;
>> +}
>> +


--
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH] cpu: Add starts_halted() method

[Philippe Mathieu-Daudé]

Hi Thiago,

On 7/8/20 1:28 AM, Thiago Jung Bauermann wrote:
> 
> Hello Eduardo,
> 
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
>> On Tue, Jul 07, 2020 at 05:43:33PM -0300, Thiago Jung Bauermann wrote:
>>> PowerPC sPAPRs CPUs start in the halted state, but generic QEMU code
>>> assumes that CPUs start in the non-halted state. spapr_reset_vcpu()
>>> attempts to rectify this by setting CPUState::halted to 1. But that's too
>>> late for hotplugged CPUs in a machine configured with 2 or mor threads per
>>> core.
>>>
>>> By then, other parts of QEMU have already caused the vCPU to run in an
>>> unitialized state a couple of times. For example, ppc_cpu_reset() calls
>>> ppc_tlb_invalidate_all(), which ends up calling async_run_on_cpu(). This
>>> kicks the new vCPU while it has CPUState::halted = 0, causing QEMU to issue
>>> a KVM_RUN ioctl on the new vCPU before the guest is able to make the
>>> start-cpu RTAS call to initialize its register state.
>>>
>>> This doesn't seem to cause visible issues for regular guests, but on a
>>> secure guest running under the Ultravisor it does. The Ultravisor relies on
>>> being able to snoop on the start-cpu RTAS call to map vCPUs to guests, and
>>> this issue causes it to see a stray vCPU that doesn't belong to any guest.
>>>
>>> Fix by adding a starts_halted() method to the CPUState class, and making it
>>> return 1 if the machine is an sPAPR guest.
>>>
>>> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
>> [...]
>>> +static uint32_t ppc_cpu_starts_halted(void)
>>> +{
>>> +    SpaprMachineState *spapr =
>>> +        (SpaprMachineState *) object_dynamic_cast(qdev_get_machine(),
>>> +                                                  TYPE_SPAPR_MACHINE);
>>
>> Wouldn't it be simpler to just implement this as a MachineClass
>> boolean field?  e.g.:

Class boolean field certainly sounds better, but I am not sure this
is a property of the machine. Rather the arch? So move the field
to CPUClass? Maybe not, let's discuss :)

>>
>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> Yes, indeed it would. Thanks for this patch. I just tested and it
> also solves the problem (except for the nit mentioned below).
> 
> Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> 
> Should I submit a proper patch with these changes (with you as the
> author)?
> 
>> ---
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index 426ce5f625..ffadc7a17d 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -215,6 +215,7 @@ struct MachineClass {
>>      bool nvdimm_supported;
>>      bool numa_mem_supported;
>>      bool auto_enable_numa;
>> +    bool cpu_starts_halted;
>>      const char *default_ram_id;
>>
>>      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
>> diff --git a/hw/core/cpu.c b/hw/core/cpu.c
>> index 0f23409f1d..08dd504034 100644
>> --- a/hw/core/cpu.c
>> +++ b/hw/core/cpu.c
>> @@ -252,6 +252,7 @@ static void cpu_common_reset(DeviceState *dev)
>>  {
>>      CPUState *cpu = CPU(dev);
>>      CPUClass *cc = CPU_GET_CLASS(cpu);
>> +    MachineState *machine = object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE);
> 
> I had to add a (MachineState *) cast here to get the code to compile.

Btw why not use MACHINE(qdev_get_machine()) ?

> 
>>
>>      if (qemu_loglevel_mask(CPU_LOG_RESET)) {
>>          qemu_log("CPU Reset (CPU %d)\n", cpu->cpu_index);
>> @@ -259,7 +260,7 @@ static void cpu_common_reset(DeviceState *dev)
>>      }
>>
>>      cpu->interrupt_request = 0;
>> -    cpu->halted = 0;
>> +    cpu->halted = machine ? MACHINE_GET_CLASS(machine)->cpu_starts_halted : 0;
>>      cpu->mem_io_pc = 0;
>>      cpu->icount_extra = 0;
>>      atomic_set(&cpu->icount_decr_ptr->u32, 0);
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index f6f034d039..d16ec33033 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -4487,6 +4487,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>>      mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power9_v2.0");
>>      mc->has_hotpluggable_cpus = true;
>>      mc->nvdimm_supported = true;
>> +    mc->cpu_starts_halted = true;
>>      smc->resize_hpt_default = SPAPR_RESIZE_HPT_ENABLED;
>>      fwc->get_dev_path = spapr_get_fw_dev_path;
>>      nc->nmi_monitor_handler = spapr_nmi;
>>
>>> +
>>> +    /*
>>> +     * In sPAPR, all CPUs start halted. CPU0 is unhalted from the machine level
>>> +     * reset code and the rest are explicitly started up by the guest using an
>>> +     * RTAS call.
>>> +     */
>>> +    return spapr != NULL;
>>> +}
>>> +
> 
> 
> --
> Thiago Jung Bauermann
> IBM Linux Technology Center
> 

Re: [PATCH] cpu: Add starts_halted() method

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2696 bytes --]

On Wed, Jul 08, 2020 at 10:38:29AM +0200, Philippe Mathieu-Daudé wrote:
> Hi Thiago,
> 
> On 7/8/20 1:28 AM, Thiago Jung Bauermann wrote:
> > 
> > Hello Eduardo,
> > 
> > Eduardo Habkost <ehabkost@redhat.com> writes:
> > 
> >> On Tue, Jul 07, 2020 at 05:43:33PM -0300, Thiago Jung Bauermann wrote:
> >>> PowerPC sPAPRs CPUs start in the halted state, but generic QEMU code
> >>> assumes that CPUs start in the non-halted state. spapr_reset_vcpu()
> >>> attempts to rectify this by setting CPUState::halted to 1. But that's too
> >>> late for hotplugged CPUs in a machine configured with 2 or mor threads per
> >>> core.
> >>>
> >>> By then, other parts of QEMU have already caused the vCPU to run in an
> >>> unitialized state a couple of times. For example, ppc_cpu_reset() calls
> >>> ppc_tlb_invalidate_all(), which ends up calling async_run_on_cpu(). This
> >>> kicks the new vCPU while it has CPUState::halted = 0, causing QEMU to issue
> >>> a KVM_RUN ioctl on the new vCPU before the guest is able to make the
> >>> start-cpu RTAS call to initialize its register state.
> >>>
> >>> This doesn't seem to cause visible issues for regular guests, but on a
> >>> secure guest running under the Ultravisor it does. The Ultravisor relies on
> >>> being able to snoop on the start-cpu RTAS call to map vCPUs to guests, and
> >>> this issue causes it to see a stray vCPU that doesn't belong to any guest.
> >>>
> >>> Fix by adding a starts_halted() method to the CPUState class, and making it
> >>> return 1 if the machine is an sPAPR guest.
> >>>
> >>> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> >> [...]
> >>> +static uint32_t ppc_cpu_starts_halted(void)
> >>> +{
> >>> +    SpaprMachineState *spapr =
> >>> +        (SpaprMachineState *) object_dynamic_cast(qdev_get_machine(),
> >>> +                                                  TYPE_SPAPR_MACHINE);
> >>
> >> Wouldn't it be simpler to just implement this as a MachineClass
> >> boolean field?  e.g.:
> 
> Class boolean field certainly sounds better, but I am not sure this
> is a property of the machine. Rather the arch? So move the field
> to CPUClass? Maybe not, let's discuss :)

It is absolutely a property of the machine.  e.g. I don't think we
want this for powernv.  pseries is a bit of a special case since it is
explicitly a paravirt platform.  But even for emulated hardware, the
board can absolutely strap things so that cpus do or don't start
immediately.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] cpu: Add starts_halted() method

[Peter Maydell]

On Wed, 8 Jul 2020 at 12:12, David Gibson <david@gibson.dropbear.id.au> wrote:
> On Wed, Jul 08, 2020 at 10:38:29AM +0200, Philippe Mathieu-Daudé wrote:
> > Class boolean field certainly sounds better, but I am not sure this
> > is a property of the machine. Rather the arch? So move the field
> > to CPUClass? Maybe not, let's discuss :)
>
> It is absolutely a property of the machine.  e.g. I don't think we
> want this for powernv.  pseries is a bit of a special case since it is
> explicitly a paravirt platform.  But even for emulated hardware, the
> board can absolutely strap things so that cpus do or don't start
> immediately.

It's a property of the individual CPU, I think. One common setup
for Arm systems is that the primary CPU starts powered up but
the secondaries all start powered down.

The original bug as described in the commit message sounds
to me like something we should look to fix in the implementation
of async_run_on_cpu() -- it shouldn't cause a CPU that's halfway
through reset to do a KVM_RUN or otherwise run guest code,
whether that CPU is going to start powered-up or powered-down.

thanks
-- PMM

Re: [PATCH] cpu: Add starts_halted() method

[Eduardo Habkost]

On Wed, Jul 08, 2020 at 02:14:03PM +0100, Peter Maydell wrote:
> On Wed, 8 Jul 2020 at 12:12, David Gibson <david@gibson.dropbear.id.au> wrote:
> > On Wed, Jul 08, 2020 at 10:38:29AM +0200, Philippe Mathieu-Daudé wrote:
> > > Class boolean field certainly sounds better, but I am not sure this
> > > is a property of the machine. Rather the arch? So move the field
> > > to CPUClass? Maybe not, let's discuss :)
> >
> > It is absolutely a property of the machine.  e.g. I don't think we
> > want this for powernv.  pseries is a bit of a special case since it is
> > explicitly a paravirt platform.  But even for emulated hardware, the
> > board can absolutely strap things so that cpus do or don't start
> > immediately.
> 
> It's a property of the individual CPU, I think. One common setup
> for Arm systems is that the primary CPU starts powered up but
> the secondaries all start powered down.

Both statements can be true.  It can be a property of the
individual CPU (although I'm not convinced it has to), but it
still needs to be controlled by the machine.

> 
> The original bug as described in the commit message sounds
> to me like something we should look to fix in the implementation
> of async_run_on_cpu() -- it shouldn't cause a CPU that's halfway
> through reset to do a KVM_RUN or otherwise run guest code,
> whether that CPU is going to start powered-up or powered-down.

What "halfway through reset" means, exactly?  Isn't halted==1
enough to indicate the CPU is in that state?

-- 
Eduardo

Re: [PATCH] cpu: Add starts_halted() method

[Peter Maydell]

On Wed, 8 Jul 2020 at 16:25, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Wed, Jul 08, 2020 at 02:14:03PM +0100, Peter Maydell wrote:
> > The original bug as described in the commit message sounds
> > to me like something we should look to fix in the implementation
> > of async_run_on_cpu() -- it shouldn't cause a CPU that's halfway
> > through reset to do a KVM_RUN or otherwise run guest code,
> > whether that CPU is going to start powered-up or powered-down.
>
> What "halfway through reset" means, exactly?  Isn't halted==1
> enough to indicate the CPU is in that state?

I mean "while we're in the middle of the CPU method that's
called by cpu_reset()". "halted==1" says "the CPU is halted";
that's not the same thing. KVM_RUN happening
as a side effect in the middle of that code is a bug
whether the CPU happens to be intended to be put into the
halted state or not. If the CPU is intended to be created
not-halted then KVM_RUN can happen after cpu reset
completes, but not before.

thanks
-- PMM

Re: [PATCH] cpu: Add starts_halted() method

[Eduardo Habkost]

On Wed, Jul 08, 2020 at 04:32:51PM +0100, Peter Maydell wrote:
> On Wed, 8 Jul 2020 at 16:25, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Wed, Jul 08, 2020 at 02:14:03PM +0100, Peter Maydell wrote:
> > > The original bug as described in the commit message sounds
> > > to me like something we should look to fix in the implementation
> > > of async_run_on_cpu() -- it shouldn't cause a CPU that's halfway
> > > through reset to do a KVM_RUN or otherwise run guest code,
> > > whether that CPU is going to start powered-up or powered-down.
> >
> > What "halfway through reset" means, exactly?  Isn't halted==1
> > enough to indicate the CPU is in that state?
> 
> I mean "while we're in the middle of the CPU method that's
> called by cpu_reset()". "halted==1" says "the CPU is halted";
> that's not the same thing. KVM_RUN happening
> as a side effect in the middle of that code is a bug
> whether the CPU happens to be intended to be put into the
> halted state or not. If the CPU is intended to be created
> not-halted then KVM_RUN can happen after cpu reset
> completes, but not before.

Wait, I thought we already had mechanisms to prevent that from
happening.  Otherwise, it would never be safe for cpu_reset() to
touch the CPU registers.

-- 
Eduardo

Re: [PATCH] cpu: Add starts_halted() method

[Peter Maydell]

On Wed, 8 Jul 2020 at 17:03, Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> On Wed, Jul 08, 2020 at 04:32:51PM +0100, Peter Maydell wrote:
> > On Wed, 8 Jul 2020 at 16:25, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > On Wed, Jul 08, 2020 at 02:14:03PM +0100, Peter Maydell wrote:
> > > > The original bug as described in the commit message sounds
> > > > to me like something we should look to fix in the implementation
> > > > of async_run_on_cpu() -- it shouldn't cause a CPU that's halfway
> > > > through reset to do a KVM_RUN or otherwise run guest code,
> > > > whether that CPU is going to start powered-up or powered-down.
> > >
> > > What "halfway through reset" means, exactly?  Isn't halted==1
> > > enough to indicate the CPU is in that state?
> >
> > I mean "while we're in the middle of the CPU method that's
> > called by cpu_reset()". "halted==1" says "the CPU is halted";
> > that's not the same thing. KVM_RUN happening
> > as a side effect in the middle of that code is a bug
> > whether the CPU happens to be intended to be put into the
> > halted state or not. If the CPU is intended to be created
> > not-halted then KVM_RUN can happen after cpu reset
> > completes, but not before.
>
> Wait, I thought we already had mechanisms to prevent that from
> happening.  Otherwise, it would never be safe for cpu_reset() to
> touch the CPU registers.

Exactly. It appears that there's a bug in our mechanisms,
which is why I'm suggesting that the right thing is
to fix that bug rather than marking the CPU as halted
earlier in the reset process so that the KVM_RUN happens
to do nothing...

-- PMM

Re: [PATCH] cpu: Add starts_halted() method

[Eduardo Habkost]

On Wed, Jul 08, 2020 at 06:09:49PM +0100, Peter Maydell wrote:
> On Wed, 8 Jul 2020 at 17:03, Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> > On Wed, Jul 08, 2020 at 04:32:51PM +0100, Peter Maydell wrote:
> > > On Wed, 8 Jul 2020 at 16:25, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > On Wed, Jul 08, 2020 at 02:14:03PM +0100, Peter Maydell wrote:
> > > > > The original bug as described in the commit message sounds
> > > > > to me like something we should look to fix in the implementation
> > > > > of async_run_on_cpu() -- it shouldn't cause a CPU that's halfway
> > > > > through reset to do a KVM_RUN or otherwise run guest code,
> > > > > whether that CPU is going to start powered-up or powered-down.
> > > >
> > > > What "halfway through reset" means, exactly?  Isn't halted==1
> > > > enough to indicate the CPU is in that state?
> > >
> > > I mean "while we're in the middle of the CPU method that's
> > > called by cpu_reset()". "halted==1" says "the CPU is halted";
> > > that's not the same thing. KVM_RUN happening
> > > as a side effect in the middle of that code is a bug
> > > whether the CPU happens to be intended to be put into the
> > > halted state or not. If the CPU is intended to be created
> > > not-halted then KVM_RUN can happen after cpu reset
> > > completes, but not before.
> >
> > Wait, I thought we already had mechanisms to prevent that from
> > happening.  Otherwise, it would never be safe for cpu_reset() to
> > touch the CPU registers.
> 
> Exactly. It appears that there's a bug in our mechanisms,
> which is why I'm suggesting that the right thing is
> to fix that bug rather than marking the CPU as halted
> earlier in the reset process so that the KVM_RUN happens
> to do nothing...

I agree this is necessary, but it doesn't seem sufficient.

Having cpu_reset() set halted=0 on spapr (and probably other
machines) is also a bug, as it could still trigger unwanted
KVM_RUN when cpu_reset() returns (and before machine code sets
halted=1).

-- 
Eduardo

Re: [PATCH] cpu: Add starts_halted() method

[Peter Maydell]

On Wed, 8 Jul 2020 at 18:36, Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> On Wed, Jul 08, 2020 at 06:09:49PM +0100, Peter Maydell wrote:
> > Exactly. It appears that there's a bug in our mechanisms,
> > which is why I'm suggesting that the right thing is
> > to fix that bug rather than marking the CPU as halted
> > earlier in the reset process so that the KVM_RUN happens
> > to do nothing...
>
> I agree this is necessary, but it doesn't seem sufficient.
>
> Having cpu_reset() set halted=0 on spapr (and probably other
> machines) is also a bug, as it could still trigger unwanted
> KVM_RUN when cpu_reset() returns (and before machine code sets
> halted=1).

The Arm handling of starting-halted sets halted=1 within cpu_reset,
based on whether the CPU object was created with a
"start-powered-off" property.

I'm not sure in practice that anything can get in asynchronously
and cause a KVM_RUN in between spapr_reset_vcpu() calling
cpu_reset() and it setting cs->halted (and the other stuff),
though. This function ought to be called with the iothread
lock held, so KVM_RUN will only happen if it calls some
other function which incorrectly lets the CPU run.

thanks
-- PMM

Re: [PATCH] cpu: Add starts_halted() method

[Eduardo Habkost]

On Wed, Jul 08, 2020 at 09:11:55PM +0100, Peter Maydell wrote:
> On Wed, 8 Jul 2020 at 18:36, Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> > On Wed, Jul 08, 2020 at 06:09:49PM +0100, Peter Maydell wrote:
> > > Exactly. It appears that there's a bug in our mechanisms,
> > > which is why I'm suggesting that the right thing is
> > > to fix that bug rather than marking the CPU as halted
> > > earlier in the reset process so that the KVM_RUN happens
> > > to do nothing...
> >
> > I agree this is necessary, but it doesn't seem sufficient.
> >
> > Having cpu_reset() set halted=0 on spapr (and probably other
> > machines) is also a bug, as it could still trigger unwanted
> > KVM_RUN when cpu_reset() returns (and before machine code sets
> > halted=1).
> 
> The Arm handling of starting-halted sets halted=1 within cpu_reset,
> based on whether the CPU object was created with a
> "start-powered-off" property.

Making this mechanism generic sounds like a good idea.

> 
> I'm not sure in practice that anything can get in asynchronously
> and cause a KVM_RUN in between spapr_reset_vcpu() calling
> cpu_reset() and it setting cs->halted (and the other stuff),
> though. This function ought to be called with the iothread
> lock held, so KVM_RUN will only happen if it calls some
> other function which incorrectly lets the CPU run.

Yeah, maybe it won't happen in practice.  It just seems fragile.
The same way ppc_cpu_reset() kicked the CPU by accident, code
outside cpu_reset() might one day kick the CPU by accident before
setting halted=1.

-- 
Eduardo

Re: [PATCH] cpu: Add starts_halted() method

[Thiago Jung Bauermann]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Wed, Jul 08, 2020 at 09:11:55PM +0100, Peter Maydell wrote:
>> On Wed, 8 Jul 2020 at 18:36, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> >
>> > On Wed, Jul 08, 2020 at 06:09:49PM +0100, Peter Maydell wrote:
>> > > Exactly. It appears that there's a bug in our mechanisms,
>> > > which is why I'm suggesting that the right thing is
>> > > to fix that bug rather than marking the CPU as halted
>> > > earlier in the reset process so that the KVM_RUN happens
>> > > to do nothing...
>> >
>> > I agree this is necessary, but it doesn't seem sufficient.
>> >
>> > Having cpu_reset() set halted=0 on spapr (and probably other
>> > machines) is also a bug, as it could still trigger unwanted
>> > KVM_RUN when cpu_reset() returns (and before machine code sets
>> > halted=1).
>>
>> The Arm handling of starting-halted sets halted=1 within cpu_reset,
>> based on whether the CPU object was created with a
>> "start-powered-off" property.
>
> Making this mechanism generic sounds like a good idea.

I'll take a stab at doing that and using it for the spapr machine.

>> I'm not sure in practice that anything can get in asynchronously
>> and cause a KVM_RUN in between spapr_reset_vcpu() calling
>> cpu_reset() and it setting cs->halted (and the other stuff),
>> though. This function ought to be called with the iothread
>> lock held, so KVM_RUN will only happen if it calls some
>> other function which incorrectly lets the CPU run.
>
> Yeah, maybe it won't happen in practice.  It just seems fragile.
> The same way ppc_cpu_reset() kicked the CPU by accident, code
> outside cpu_reset() might one day kick the CPU by accident before
> setting halted=1.

I'm seeing the vcpu being KVM_RUN'd too early twice during hotplug.
Both of them are before cpu_reset() and ppc_cpu_reset().

Here's the backtrace for the first of them (redacted for clarity):

#0  in cpu_resume ()
#1  in cpu_common_realizefn ()
#2  in ppc_cpu_realize ()
#3  in device_set_realized ()
#4  in property_set_bool ()
#5  in object_property_set ()
#6  in object_property_set_qobject ()
#7  in object_property_set_bool ()
#8  in qdev_realize ()
#9  in spapr_realize_vcpu ()
#10 in spapr_cpu_core_realize ()
#11 in device_set_realized ()
#12 in property_set_bool ()
#13 in object_property_set ()
#14 in object_property_set_qobject ()
#15 in object_property_set_bool ()
#16 in qdev_realize ()
#17 in qdev_device_add ()
#18 in qmp_device_add ()

Here's the second:

#0  in qemu_cpu_kick_thread ()
#1  in qemu_cpu_kick ()
#2  in queue_work_on_cpu ()
#3  in async_run_on_cpu ()
#4  in tlb_flush_by_mmuidx ()
#5  in tlb_flush ()
#6  in ppc_tlb_invalidate_all ()
#7  in ppc_cpu_reset ()
#8  in device_transitional_reset ()
#9  in resettable_phase_hold ()
#10 in resettable_assert_reset ()
#11 in device_set_realized ()
#12 in property_set_bool ()
#13 in object_property_set ()
#14 in object_property_set_qobject ()
#15 in object_property_set_bool ()
#16 in qdev_realize ()
#17 in spapr_realize_vcpu ()
#18 in spapr_cpu_core_realize ()
#19 in device_set_realized ()
#20 in property_set_bool ()
#21 in object_property_set ()
#22 in object_property_set_qobject ()
#23 in object_property_set_bool ()
#24 in qdev_realize ()
#25 in qdev_device_add ()
#26 in qmp_device_add ()

Looking closely, both of them ultimately stem from the
qdev_realize(DEVICE(cpu), ...) call in spapr_realize_vcpu(). Is there
something wrong with that? I don't know anything about the QEMU device
model to be able to tell.

One other way I found to avoid the spurious KVM_RUN calls is to remove
the cpu_resume() call in cpu_common_realizefn(), which to me seems to
be placed way too early in the CPU hotplug path. Simply removing it
makes CPU hotplug stop working though. :-)  I still have to see if I can
find a better place for it...

--
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH] cpu: Add starts_halted() method

[Thiago Jung Bauermann]

Thiago Jung Bauermann <bauerman@linux.ibm.com> writes:

> I'm seeing the vcpu being KVM_RUN'd too early twice during hotplug.
> Both of them are before cpu_reset() and ppc_cpu_reset().

Hm, rereading the message obviously the above is partially wrong. The
second case happens during ppc_cpu_reset().

> Here's the second:
>
> #0  in qemu_cpu_kick_thread ()
> #1  in qemu_cpu_kick ()
> #2  in queue_work_on_cpu ()
> #3  in async_run_on_cpu ()
> #4  in tlb_flush_by_mmuidx ()
> #5  in tlb_flush ()
> #6  in ppc_tlb_invalidate_all ()
> #7  in ppc_cpu_reset ()
> #8  in device_transitional_reset ()
> #9  in resettable_phase_hold ()
> #10 in resettable_assert_reset ()
> #11 in device_set_realized ()
> #12 in property_set_bool ()
> #13 in object_property_set ()
> #14 in object_property_set_qobject ()
> #15 in object_property_set_bool ()
> #16 in qdev_realize ()
> #17 in spapr_realize_vcpu ()
> #18 in spapr_cpu_core_realize ()
> #19 in device_set_realized ()
> #20 in property_set_bool ()
> #21 in object_property_set ()
> #22 in object_property_set_qobject ()
> #23 in object_property_set_bool ()
> #24 in qdev_realize ()
> #25 in qdev_device_add ()
> #26 in qmp_device_add ()

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH] cpu: Add starts_halted() method

[Philippe Mathieu-Daudé]

On 7/9/20 5:26 AM, Thiago Jung Bauermann wrote:
> 
> Thiago Jung Bauermann <bauerman@linux.ibm.com> writes:
> 
>> I'm seeing the vcpu being KVM_RUN'd too early twice during hotplug.
>> Both of them are before cpu_reset() and ppc_cpu_reset().
> 
> Hm, rereading the message obviously the above is partially wrong. The
> second case happens during ppc_cpu_reset().
> 
>> Here's the second:
>>
>> #0  in qemu_cpu_kick_thread ()
>> #1  in qemu_cpu_kick ()
>> #2  in queue_work_on_cpu ()
>> #3  in async_run_on_cpu ()
>> #4  in tlb_flush_by_mmuidx ()
>> #5  in tlb_flush ()
>> #6  in ppc_tlb_invalidate_all ()
>> #7  in ppc_cpu_reset ()
>> #8  in device_transitional_reset ()
>> #9  in resettable_phase_hold ()
>> #10 in resettable_assert_reset ()
>> #11 in device_set_realized ()

Dunno if related, might be helpful:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg686477.html

>> #12 in property_set_bool ()
>> #13 in object_property_set ()
>> #14 in object_property_set_qobject ()
>> #15 in object_property_set_bool ()
>> #16 in qdev_realize ()
>> #17 in spapr_realize_vcpu ()
>> #18 in spapr_cpu_core_realize ()
>> #19 in device_set_realized ()
>> #20 in property_set_bool ()
>> #21 in object_property_set ()
>> #22 in object_property_set_qobject ()
>> #23 in object_property_set_bool ()
>> #24 in qdev_realize ()
>> #25 in qdev_device_add ()
>> #26 in qmp_device_add ()
> 

Re: [PATCH] cpu: Add starts_halted() method

[Thiago Jung Bauermann]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 7/9/20 5:26 AM, Thiago Jung Bauermann wrote:
>>
>> Thiago Jung Bauermann <bauerman@linux.ibm.com> writes:
>>
>>> I'm seeing the vcpu being KVM_RUN'd too early twice during hotplug.
>>> Both of them are before cpu_reset() and ppc_cpu_reset().
>>
>> Hm, rereading the message obviously the above is partially wrong. The
>> second case happens during ppc_cpu_reset().
>>
>>> Here's the second:
>>>
>>> #0  in qemu_cpu_kick_thread ()
>>> #1  in qemu_cpu_kick ()
>>> #2  in queue_work_on_cpu ()
>>> #3  in async_run_on_cpu ()
>>> #4  in tlb_flush_by_mmuidx ()
>>> #5  in tlb_flush ()
>>> #6  in ppc_tlb_invalidate_all ()
>>> #7  in ppc_cpu_reset ()
>>> #8  in device_transitional_reset ()
>>> #9  in resettable_phase_hold ()
>>> #10 in resettable_assert_reset ()
>>> #11 in device_set_realized ()
>
> Dunno if related, might be helpful:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg686477.html

Yes, it's helpful. Thanks!

So is was it resolved whether it's appropriate to do a cpu_reset()
within realize?

Is the core of the problem that device_set_realize() ends up calling
ppc_cpu_reset()?

--
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH] cpu: Add starts_halted() method

[Eduardo Habkost]

On Fri, Jul 10, 2020 at 05:02:38PM -0300, Thiago Jung Bauermann wrote:
> 
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 
> > On 7/9/20 5:26 AM, Thiago Jung Bauermann wrote:
> >>
> >> Thiago Jung Bauermann <bauerman@linux.ibm.com> writes:
> >>
> >>> I'm seeing the vcpu being KVM_RUN'd too early twice during hotplug.
> >>> Both of them are before cpu_reset() and ppc_cpu_reset().
> >>
> >> Hm, rereading the message obviously the above is partially wrong. The
> >> second case happens during ppc_cpu_reset().
> >>
> >>> Here's the second:
> >>>
> >>> #0  in qemu_cpu_kick_thread ()
> >>> #1  in qemu_cpu_kick ()
> >>> #2  in queue_work_on_cpu ()
> >>> #3  in async_run_on_cpu ()
> >>> #4  in tlb_flush_by_mmuidx ()
> >>> #5  in tlb_flush ()
> >>> #6  in ppc_tlb_invalidate_all ()
> >>> #7  in ppc_cpu_reset ()
> >>> #8  in device_transitional_reset ()
> >>> #9  in resettable_phase_hold ()
> >>> #10 in resettable_assert_reset ()
> >>> #11 in device_set_realized ()
> >
> > Dunno if related, might be helpful:
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg686477.html
> 
> Yes, it's helpful. Thanks!
> 
> So is was it resolved whether it's appropriate to do a cpu_reset()
> within realize?
> 
> Is the core of the problem that device_set_realize() ends up calling
> ppc_cpu_reset()?

There are 15 realize functions which call cpu_reset(), currently.
Making it safe seems more appropriate than forbidding it.

-- 
Eduardo

Re: [PATCH] cpu: Add starts_halted() method

[Thiago Jung Bauermann]

Alex Bennée <alex.bennee@linaro.org> writes:

> Thiago Jung Bauermann <bauerman@linux.ibm.com> writes:
>
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>>
>>> On Wed, Jul 08, 2020 at 09:11:55PM +0100, Peter Maydell wrote:
>>>> On Wed, 8 Jul 2020 at 18:36, Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>> >
>>>> > On Wed, Jul 08, 2020 at 06:09:49PM +0100, Peter Maydell wrote:
>>>> > > Exactly. It appears that there's a bug in our mechanisms,
>>>> > > which is why I'm suggesting that the right thing is
>>>> > > to fix that bug rather than marking the CPU as halted
>>>> > > earlier in the reset process so that the KVM_RUN happens
>>>> > > to do nothing...
>>>> >
>>>> > I agree this is necessary, but it doesn't seem sufficient.
>>>> >
>>>> > Having cpu_reset() set halted=0 on spapr (and probably other
>>>> > machines) is also a bug, as it could still trigger unwanted
>>>> > KVM_RUN when cpu_reset() returns (and before machine code sets
>>>> > halted=1).
>>>>
>>>> The Arm handling of starting-halted sets halted=1 within cpu_reset,
>>>> based on whether the CPU object was created with a
>>>> "start-powered-off" property.
>>>
>>> Making this mechanism generic sounds like a good idea.
>>
>> I'll take a stab at doing that and using it for the spapr machine.
>>
>>>> I'm not sure in practice that anything can get in asynchronously
>>>> and cause a KVM_RUN in between spapr_reset_vcpu() calling
>>>> cpu_reset() and it setting cs->halted (and the other stuff),
>>>> though. This function ought to be called with the iothread
>>>> lock held, so KVM_RUN will only happen if it calls some
>>>> other function which incorrectly lets the CPU run.
>>>
>>> Yeah, maybe it won't happen in practice.  It just seems fragile.
>>> The same way ppc_cpu_reset() kicked the CPU by accident, code
>>> outside cpu_reset() might one day kick the CPU by accident before
>>> setting halted=1.
>>
>> I'm seeing the vcpu being KVM_RUN'd too early twice during hotplug.
>> Both of them are before cpu_reset() and ppc_cpu_reset().
>>
>> Here's the backtrace for the first of them (redacted for clarity):
>>
>> #0  in cpu_resume ()
>> #1  in cpu_common_realizefn ()
>> #2  in ppc_cpu_realize ()
>> #3  in device_set_realized ()
>> #4  in property_set_bool ()
>> #5  in object_property_set ()
>> #6  in object_property_set_qobject ()
>> #7  in object_property_set_bool ()
>> #8  in qdev_realize ()
> <snip>
>> #18 in qmp_device_add ()
>
> Is this a hotplug event?

Yes, the way I reproduce the problem is starting a pseries guest with
`-smp 2,maxcpus=32,sockets=1,cores=16,threads=2` and then use qmp-shell to
send the command:

device_add id=device-2 driver=host-spapr-cpu-core core-id=2 node-id=0

>> Here's the second:
>>
>> #0  in qemu_cpu_kick_thread ()
>> #1  in qemu_cpu_kick ()
>> #2  in queue_work_on_cpu ()
>> #3  in async_run_on_cpu ()
>> #4  in tlb_flush_by_mmuidx ()
>> #5  in tlb_flush ()
>> #6  in ppc_tlb_invalidate_all ()
>
> FWIW tcg_flush_softmmu_tlb handles a tlb_flush in the common reset code.

Ok, maybe KVM should be doing that too? Or maybe it does but pseries
isn't relying on it. I'll dig further.

--
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH] cpu: Add starts_halted() method

[Alex Bennée]

Thiago Jung Bauermann <bauerman@linux.ibm.com> writes:

> Alex Bennée <alex.bennee@linaro.org> writes:
>
>> Thiago Jung Bauermann <bauerman@linux.ibm.com> writes:
>>
>>> Eduardo Habkost <ehabkost@redhat.com> writes:
>>>
>>>> On Wed, Jul 08, 2020 at 09:11:55PM +0100, Peter Maydell wrote:
>>>>> On Wed, 8 Jul 2020 at 18:36, Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>>> >
>>>>> > On Wed, Jul 08, 2020 at 06:09:49PM +0100, Peter Maydell wrote:
>>>>> > > Exactly. It appears that there's a bug in our mechanisms,
>>>>> > > which is why I'm suggesting that the right thing is
>>>>> > > to fix that bug rather than marking the CPU as halted
>>>>> > > earlier in the reset process so that the KVM_RUN happens
>>>>> > > to do nothing...
>>>>> >
>>>>> > I agree this is necessary, but it doesn't seem sufficient.
>>>>> >
>>>>> > Having cpu_reset() set halted=0 on spapr (and probably other
>>>>> > machines) is also a bug, as it could still trigger unwanted
>>>>> > KVM_RUN when cpu_reset() returns (and before machine code sets
>>>>> > halted=1).
>>>>>
>>>>> The Arm handling of starting-halted sets halted=1 within cpu_reset,
>>>>> based on whether the CPU object was created with a
>>>>> "start-powered-off" property.
>>>>
>>>> Making this mechanism generic sounds like a good idea.
>>>
>>> I'll take a stab at doing that and using it for the spapr machine.
>>>
>>>>> I'm not sure in practice that anything can get in asynchronously
>>>>> and cause a KVM_RUN in between spapr_reset_vcpu() calling
>>>>> cpu_reset() and it setting cs->halted (and the other stuff),
>>>>> though. This function ought to be called with the iothread
>>>>> lock held, so KVM_RUN will only happen if it calls some
>>>>> other function which incorrectly lets the CPU run.
>>>>
>>>> Yeah, maybe it won't happen in practice.  It just seems fragile.
>>>> The same way ppc_cpu_reset() kicked the CPU by accident, code
>>>> outside cpu_reset() might one day kick the CPU by accident before
>>>> setting halted=1.
>>>
>>> I'm seeing the vcpu being KVM_RUN'd too early twice during hotplug.
>>> Both of them are before cpu_reset() and ppc_cpu_reset().
>>>
>>> Here's the backtrace for the first of them (redacted for clarity):
>>>
>>> #0  in cpu_resume ()
>>> #1  in cpu_common_realizefn ()
>>> #2  in ppc_cpu_realize ()
>>> #3  in device_set_realized ()
>>> #4  in property_set_bool ()
>>> #5  in object_property_set ()
>>> #6  in object_property_set_qobject ()
>>> #7  in object_property_set_bool ()
>>> #8  in qdev_realize ()
>> <snip>
>>> #18 in qmp_device_add ()
>>
>> Is this a hotplug event?
>
> Yes, the way I reproduce the problem is starting a pseries guest with
> `-smp 2,maxcpus=32,sockets=1,cores=16,threads=2` and then use qmp-shell to
> send the command:
>
> device_add id=device-2 driver=host-spapr-cpu-core core-id=2 node-id=0
>
>>> Here's the second:
>>>
>>> #0  in qemu_cpu_kick_thread ()
>>> #1  in qemu_cpu_kick ()
>>> #2  in queue_work_on_cpu ()
>>> #3  in async_run_on_cpu ()
>>> #4  in tlb_flush_by_mmuidx ()
>>> #5  in tlb_flush ()
>>> #6  in ppc_tlb_invalidate_all ()
>>
>> FWIW tcg_flush_softmmu_tlb handles a tlb_flush in the common reset code.
>
> Ok, maybe KVM should be doing that too? Or maybe it does but pseries
> isn't relying on it. I'll dig further.

No tlb flush is a softmmu only thing.


-- 
Alex Bennée

Re: [PATCH] cpu: Add starts_halted() method

[Philippe Mathieu-Daudé]

On 7/8/20 5:25 PM, Eduardo Habkost wrote:
> On Wed, Jul 08, 2020 at 02:14:03PM +0100, Peter Maydell wrote:
>> On Wed, 8 Jul 2020 at 12:12, David Gibson <david@gibson.dropbear.id.au> wrote:
>>> On Wed, Jul 08, 2020 at 10:38:29AM +0200, Philippe Mathieu-Daudé wrote:
>>>> Class boolean field certainly sounds better, but I am not sure this
>>>> is a property of the machine. Rather the arch? So move the field
>>>> to CPUClass? Maybe not, let's discuss :)
>>>
>>> It is absolutely a property of the machine.  e.g. I don't think we
>>> want this for powernv.  pseries is a bit of a special case since it is
>>> explicitly a paravirt platform.  But even for emulated hardware, the
>>> board can absolutely strap things so that cpus do or don't start
>>> immediately.
>>
>> It's a property of the individual CPU, I think. One common setup
>> for Arm systems is that the primary CPU starts powered up but
>> the secondaries all start powered down.
> 
> Both statements can be true.  It can be a property of the
> individual CPU (although I'm not convinced it has to), but it
> still needs to be controlled by the machine.

From what said Peter, I understand this is a property of the
chipset. Chipsets are modelled unevenly.

IIUC QEMU started with single-core CPUs.
CPUState had same meaning for 'core' or 'cpu', 1-1 mapping.

Then multicore CPUs could be easily modelled using multiple
single-core CPUs, usually created in the machine code.

Then we moved to SoC models, creating the cores in the SoC.
Some SoCs have array of cores, eventually heterogeneous
(see the ZynqMP). We have containers of CPUState.

On an ARM-based SoC, you might have the first core started
(as said Peter) or not.

BCM2836 / BCM2837 and ZynqMP start will all ARM cores off.
On the BCM chipsets, a DSP core will boot the ARM cores.
On the ZynqMP, a MicroBlaze core boots them.
As QEMU doesn't models heterogeneous architectures, we start
modelling after the unmodelled cores booted us, so either one
or all cores on.

In this case, we narrowed down the 'start-powered-off' field
to the SoC, which happens to be how ARM SoCs are modelled.


Chipsets providing a JTAG interface can have a SRST signal,
the "system reset". When a JTAG probe is attached, it can
keeps the whole chipset in a reset state. This is equivalent
to QEMU '-S' mode (single step mode).


I don't know about pseries hardware, but if this is 'explicit
to paravirt platform', then I expect this to be the same with
other accelerators/architectures.

If paravirtualized -> cores start off by default. Let the
hypervisor start them. So still a property of the CPUState
depending on the accelerator used?


Regards,

Phil.

Re: [PATCH] cpu: Add starts_halted() method

[Eduardo Habkost]

On Wed, Jul 08, 2020 at 06:45:57PM +0200, Philippe Mathieu-Daudé wrote:
> On 7/8/20 5:25 PM, Eduardo Habkost wrote:
> > On Wed, Jul 08, 2020 at 02:14:03PM +0100, Peter Maydell wrote:
> >> On Wed, 8 Jul 2020 at 12:12, David Gibson <david@gibson.dropbear.id.au> wrote:
> >>> On Wed, Jul 08, 2020 at 10:38:29AM +0200, Philippe Mathieu-Daudé wrote:
> >>>> Class boolean field certainly sounds better, but I am not sure this
> >>>> is a property of the machine. Rather the arch? So move the field
> >>>> to CPUClass? Maybe not, let's discuss :)
> >>>
> >>> It is absolutely a property of the machine.  e.g. I don't think we
> >>> want this for powernv.  pseries is a bit of a special case since it is
> >>> explicitly a paravirt platform.  But even for emulated hardware, the
> >>> board can absolutely strap things so that cpus do or don't start
> >>> immediately.
> >>
> >> It's a property of the individual CPU, I think. One common setup
> >> for Arm systems is that the primary CPU starts powered up but
> >> the secondaries all start powered down.
> > 
> > Both statements can be true.  It can be a property of the
> > individual CPU (although I'm not convinced it has to), but it
> > still needs to be controlled by the machine.
> 
> From what said Peter, I understand this is a property of the
> chipset. Chipsets are modelled unevenly.
> 
> IIUC QEMU started with single-core CPUs.
> CPUState had same meaning for 'core' or 'cpu', 1-1 mapping.
> 
> Then multicore CPUs could be easily modelled using multiple
> single-core CPUs, usually created in the machine code.
> 
> Then we moved to SoC models, creating the cores in the SoC.
> Some SoCs have array of cores, eventually heterogeneous
> (see the ZynqMP). We have containers of CPUState.
> 
> On an ARM-based SoC, you might have the first core started
> (as said Peter) or not.
> 
> BCM2836 / BCM2837 and ZynqMP start will all ARM cores off.
> On the BCM chipsets, a DSP core will boot the ARM cores.
> On the ZynqMP, a MicroBlaze core boots them.
> As QEMU doesn't models heterogeneous architectures, we start
> modelling after the unmodelled cores booted us, so either one
> or all cores on.
> 
> In this case, we narrowed down the 'start-powered-off' field
> to the SoC, which happens to be how ARM SoCs are modelled.

I was not aware of the start-powered-off property.  If we make it
generic, we can just let spapr use it.

> 
> 
> Chipsets providing a JTAG interface can have a SRST signal,
> the "system reset". When a JTAG probe is attached, it can
> keeps the whole chipset in a reset state. This is equivalent
> to QEMU '-S' mode (single step mode).
> 
> 
> I don't know about pseries hardware, but if this is 'explicit
> to paravirt platform', then I expect this to be the same with
> other accelerators/architectures.
> 
> If paravirtualized -> cores start off by default. Let the
> hypervisor start them. So still a property of the CPUState
> depending on the accelerator used?

I don't understand this part.  Why would this depend on the
accelerator?

-- 
Eduardo

Re: [PATCH] cpu: Add starts_halted() method

[Philippe Mathieu-Daudé]

On 7/8/20 11:39 PM, Eduardo Habkost wrote:
> On Wed, Jul 08, 2020 at 06:45:57PM +0200, Philippe Mathieu-Daudé wrote:
>> On 7/8/20 5:25 PM, Eduardo Habkost wrote:
>>> On Wed, Jul 08, 2020 at 02:14:03PM +0100, Peter Maydell wrote:
>>>> On Wed, 8 Jul 2020 at 12:12, David Gibson <david@gibson.dropbear.id.au> wrote:
>>>>> On Wed, Jul 08, 2020 at 10:38:29AM +0200, Philippe Mathieu-Daudé wrote:
>>>>>> Class boolean field certainly sounds better, but I am not sure this
>>>>>> is a property of the machine. Rather the arch? So move the field
>>>>>> to CPUClass? Maybe not, let's discuss :)
>>>>>
>>>>> It is absolutely a property of the machine.  e.g. I don't think we
>>>>> want this for powernv.  pseries is a bit of a special case since it is
>>>>> explicitly a paravirt platform.  But even for emulated hardware, the
>>>>> board can absolutely strap things so that cpus do or don't start
>>>>> immediately.
>>>>
>>>> It's a property of the individual CPU, I think. One common setup
>>>> for Arm systems is that the primary CPU starts powered up but
>>>> the secondaries all start powered down.
>>>
>>> Both statements can be true.  It can be a property of the
>>> individual CPU (although I'm not convinced it has to), but it
>>> still needs to be controlled by the machine.
>>
>> From what said Peter, I understand this is a property of the
>> chipset. Chipsets are modelled unevenly.
>>
>> IIUC QEMU started with single-core CPUs.
>> CPUState had same meaning for 'core' or 'cpu', 1-1 mapping.
>>
>> Then multicore CPUs could be easily modelled using multiple
>> single-core CPUs, usually created in the machine code.
>>
>> Then we moved to SoC models, creating the cores in the SoC.
>> Some SoCs have array of cores, eventually heterogeneous
>> (see the ZynqMP). We have containers of CPUState.
>>
>> On an ARM-based SoC, you might have the first core started
>> (as said Peter) or not.
>>
>> BCM2836 / BCM2837 and ZynqMP start will all ARM cores off.
>> On the BCM chipsets, a DSP core will boot the ARM cores.
>> On the ZynqMP, a MicroBlaze core boots them.
>> As QEMU doesn't models heterogeneous architectures, we start
>> modelling after the unmodelled cores booted us, so either one
>> or all cores on.
>>
>> In this case, we narrowed down the 'start-powered-off' field
>> to the SoC, which happens to be how ARM SoCs are modelled.
> 
> I was not aware of the start-powered-off property.  If we make it
> generic, we can just let spapr use it.
> 
>>
>>
>> Chipsets providing a JTAG interface can have a SRST signal,
>> the "system reset". When a JTAG probe is attached, it can
>> keeps the whole chipset in a reset state. This is equivalent
>> to QEMU '-S' mode (single step mode).
>>
>>
>> I don't know about pseries hardware, but if this is 'explicit
>> to paravirt platform', then I expect this to be the same with
>> other accelerators/architectures.
>>
>> If paravirtualized -> cores start off by default. Let the
>> hypervisor start them. So still a property of the CPUState
>> depending on the accelerator used?
> 
> I don't understand this part.  Why would this depend on the
> accelerator?

Because starting a virtualized machine with all cores powered-off
with TCG accelerator should at least emit a warning? Or change
the behavior and start them powered-on? This is machine-specific
although.

Re: [PATCH] cpu: Add starts_halted() method

[Greg Kurz]

On Thu, 9 Jul 2020 07:11:09 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 7/8/20 11:39 PM, Eduardo Habkost wrote:
> > On Wed, Jul 08, 2020 at 06:45:57PM +0200, Philippe Mathieu-Daudé wrote:
> >> On 7/8/20 5:25 PM, Eduardo Habkost wrote:
> >>> On Wed, Jul 08, 2020 at 02:14:03PM +0100, Peter Maydell wrote:
> >>>> On Wed, 8 Jul 2020 at 12:12, David Gibson <david@gibson.dropbear.id.au> wrote:
> >>>>> On Wed, Jul 08, 2020 at 10:38:29AM +0200, Philippe Mathieu-Daudé wrote:
> >>>>>> Class boolean field certainly sounds better, but I am not sure this
> >>>>>> is a property of the machine. Rather the arch? So move the field
> >>>>>> to CPUClass? Maybe not, let's discuss :)
> >>>>>
> >>>>> It is absolutely a property of the machine.  e.g. I don't think we
> >>>>> want this for powernv.  pseries is a bit of a special case since it is
> >>>>> explicitly a paravirt platform.  But even for emulated hardware, the
> >>>>> board can absolutely strap things so that cpus do or don't start
> >>>>> immediately.
> >>>>
> >>>> It's a property of the individual CPU, I think. One common setup
> >>>> for Arm systems is that the primary CPU starts powered up but
> >>>> the secondaries all start powered down.
> >>>
> >>> Both statements can be true.  It can be a property of the
> >>> individual CPU (although I'm not convinced it has to), but it
> >>> still needs to be controlled by the machine.
> >>
> >> From what said Peter, I understand this is a property of the
> >> chipset. Chipsets are modelled unevenly.
> >>
> >> IIUC QEMU started with single-core CPUs.
> >> CPUState had same meaning for 'core' or 'cpu', 1-1 mapping.
> >>
> >> Then multicore CPUs could be easily modelled using multiple
> >> single-core CPUs, usually created in the machine code.
> >>
> >> Then we moved to SoC models, creating the cores in the SoC.
> >> Some SoCs have array of cores, eventually heterogeneous
> >> (see the ZynqMP). We have containers of CPUState.
> >>
> >> On an ARM-based SoC, you might have the first core started
> >> (as said Peter) or not.
> >>
> >> BCM2836 / BCM2837 and ZynqMP start will all ARM cores off.
> >> On the BCM chipsets, a DSP core will boot the ARM cores.
> >> On the ZynqMP, a MicroBlaze core boots them.
> >> As QEMU doesn't models heterogeneous architectures, we start
> >> modelling after the unmodelled cores booted us, so either one
> >> or all cores on.
> >>
> >> In this case, we narrowed down the 'start-powered-off' field
> >> to the SoC, which happens to be how ARM SoCs are modelled.
> > 
> > I was not aware of the start-powered-off property.  If we make it
> > generic, we can just let spapr use it.
> > 
> >>
> >>
> >> Chipsets providing a JTAG interface can have a SRST signal,
> >> the "system reset". When a JTAG probe is attached, it can
> >> keeps the whole chipset in a reset state. This is equivalent
> >> to QEMU '-S' mode (single step mode).
> >>
> >>
> >> I don't know about pseries hardware, but if this is 'explicit
> >> to paravirt platform', then I expect this to be the same with
> >> other accelerators/architectures.
> >>
> >> If paravirtualized -> cores start off by default. Let the
> >> hypervisor start them. So still a property of the CPUState
> >> depending on the accelerator used?
> > 
> > I don't understand this part.  Why would this depend on the
> > accelerator?
> 
> Because starting a virtualized machine with all cores powered-off
> with TCG accelerator should at least emit a warning? Or change
> the behavior and start them powered-on? This is machine-specific
> although.
> 

FYI, PAPR requires all vCPUs to be "stopped" by default. It is up to the
guest to start them explicitly through an RTAS call. The hypervisor is
only responsible to start a single vCPU (see spapr_cpu_set_entry_state()
called from spapr_machine_reset()) to be able to boot the guest.

So I'm not sure to see how that would depend on the accelerator...

> 

Re: [PATCH] cpu: Add starts_halted() method

[Philippe Mathieu-Daudé]

On 7/9/20 11:54 AM, Greg Kurz wrote:
> On Thu, 9 Jul 2020 07:11:09 +0200
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>> On 7/8/20 11:39 PM, Eduardo Habkost wrote:
>>> On Wed, Jul 08, 2020 at 06:45:57PM +0200, Philippe Mathieu-Daudé wrote:
>>>> On 7/8/20 5:25 PM, Eduardo Habkost wrote:
>>>>> On Wed, Jul 08, 2020 at 02:14:03PM +0100, Peter Maydell wrote:
>>>>>> On Wed, 8 Jul 2020 at 12:12, David Gibson <david@gibson.dropbear.id.au> wrote:
>>>>>>> On Wed, Jul 08, 2020 at 10:38:29AM +0200, Philippe Mathieu-Daudé wrote:
>>>>>>>> Class boolean field certainly sounds better, but I am not sure this
>>>>>>>> is a property of the machine. Rather the arch? So move the field
>>>>>>>> to CPUClass? Maybe not, let's discuss :)
>>>>>>>
>>>>>>> It is absolutely a property of the machine.  e.g. I don't think we
>>>>>>> want this for powernv.  pseries is a bit of a special case since it is
>>>>>>> explicitly a paravirt platform.  But even for emulated hardware, the
>>>>>>> board can absolutely strap things so that cpus do or don't start
>>>>>>> immediately.
>>>>>>
>>>>>> It's a property of the individual CPU, I think. One common setup
>>>>>> for Arm systems is that the primary CPU starts powered up but
>>>>>> the secondaries all start powered down.
>>>>>
>>>>> Both statements can be true.  It can be a property of the
>>>>> individual CPU (although I'm not convinced it has to), but it
>>>>> still needs to be controlled by the machine.
>>>>
>>>> From what said Peter, I understand this is a property of the
>>>> chipset. Chipsets are modelled unevenly.
>>>>
>>>> IIUC QEMU started with single-core CPUs.
>>>> CPUState had same meaning for 'core' or 'cpu', 1-1 mapping.
>>>>
>>>> Then multicore CPUs could be easily modelled using multiple
>>>> single-core CPUs, usually created in the machine code.
>>>>
>>>> Then we moved to SoC models, creating the cores in the SoC.
>>>> Some SoCs have array of cores, eventually heterogeneous
>>>> (see the ZynqMP). We have containers of CPUState.
>>>>
>>>> On an ARM-based SoC, you might have the first core started
>>>> (as said Peter) or not.
>>>>
>>>> BCM2836 / BCM2837 and ZynqMP start will all ARM cores off.
>>>> On the BCM chipsets, a DSP core will boot the ARM cores.
>>>> On the ZynqMP, a MicroBlaze core boots them.
>>>> As QEMU doesn't models heterogeneous architectures, we start
>>>> modelling after the unmodelled cores booted us, so either one
>>>> or all cores on.
>>>>
>>>> In this case, we narrowed down the 'start-powered-off' field
>>>> to the SoC, which happens to be how ARM SoCs are modelled.
>>>
>>> I was not aware of the start-powered-off property.  If we make it
>>> generic, we can just let spapr use it.
>>>
>>>>
>>>>
>>>> Chipsets providing a JTAG interface can have a SRST signal,
>>>> the "system reset". When a JTAG probe is attached, it can
>>>> keeps the whole chipset in a reset state. This is equivalent
>>>> to QEMU '-S' mode (single step mode).
>>>>
>>>>
>>>> I don't know about pseries hardware, but if this is 'explicit
>>>> to paravirt platform', then I expect this to be the same with
>>>> other accelerators/architectures.
>>>>
>>>> If paravirtualized -> cores start off by default. Let the
>>>> hypervisor start them. So still a property of the CPUState
>>>> depending on the accelerator used?
>>>
>>> I don't understand this part.  Why would this depend on the
>>> accelerator?
>>
>> Because starting a virtualized machine with all cores powered-off
>> with TCG accelerator should at least emit a warning? Or change
>> the behavior and start them powered-on? This is machine-specific
>> although.
>>
> 
> FYI, PAPR requires all vCPUs to be "stopped" by default. It is up to the
> guest to start them explicitly through an RTAS call. The hypervisor is
> only responsible to start a single vCPU (see spapr_cpu_set_entry_state()
> called from spapr_machine_reset()) to be able to boot the guest.
> 
> So I'm not sure to see how that would depend on the accelerator...

$ qemu-system-ppc64 -M pseries-5.0,accel=tcg -d in_asm
qemu-system-ppc64: warning: TCG doesn't support requested feature,
cap-cfpc=workaround
qemu-system-ppc64: warning: TCG doesn't support requested feature,
cap-sbbc=workaround
qemu-system-ppc64: warning: TCG doesn't support requested feature,
cap-ibs=workaround
qemu-system-ppc64: warning: TCG doesn't support requested feature,
cap-ccf-assist=on
----------------
IN:
0x00000100:  48003f00  b        0x4000

----------------
IN:
0x00004000:  7c7f1b78  mr       r31, r3
0x00004004:  7d6000a6  mfmsr    r11
0x00004008:  3980a000  li       r12, 0xa000
0x0000400c:  798c83c6  sldi     r12, r12, 0x30
0x00004010:  7d6b6378  or       r11, r11, r12
0x00004014:  7d600164  mtmsrd   r11
...

The vCPU doesn't seem stopped to me...

Am I missing something?

Re: [PATCH] cpu: Add starts_halted() method

[Greg Kurz]

On Thu, 9 Jul 2020 12:18:06 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 7/9/20 11:54 AM, Greg Kurz wrote:
> > On Thu, 9 Jul 2020 07:11:09 +0200
> > Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >> On 7/8/20 11:39 PM, Eduardo Habkost wrote:
> >>> On Wed, Jul 08, 2020 at 06:45:57PM +0200, Philippe Mathieu-Daudé wrote:
> >>>> On 7/8/20 5:25 PM, Eduardo Habkost wrote:
> >>>>> On Wed, Jul 08, 2020 at 02:14:03PM +0100, Peter Maydell wrote:
> >>>>>> On Wed, 8 Jul 2020 at 12:12, David Gibson <david@gibson.dropbear.id.au> wrote:
> >>>>>>> On Wed, Jul 08, 2020 at 10:38:29AM +0200, Philippe Mathieu-Daudé wrote:
> >>>>>>>> Class boolean field certainly sounds better, but I am not sure this
> >>>>>>>> is a property of the machine. Rather the arch? So move the field
> >>>>>>>> to CPUClass? Maybe not, let's discuss :)
> >>>>>>>
> >>>>>>> It is absolutely a property of the machine.  e.g. I don't think we
> >>>>>>> want this for powernv.  pseries is a bit of a special case since it is
> >>>>>>> explicitly a paravirt platform.  But even for emulated hardware, the
> >>>>>>> board can absolutely strap things so that cpus do or don't start
> >>>>>>> immediately.
> >>>>>>
> >>>>>> It's a property of the individual CPU, I think. One common setup
> >>>>>> for Arm systems is that the primary CPU starts powered up but
> >>>>>> the secondaries all start powered down.
> >>>>>
> >>>>> Both statements can be true.  It can be a property of the
> >>>>> individual CPU (although I'm not convinced it has to), but it
> >>>>> still needs to be controlled by the machine.
> >>>>
> >>>> From what said Peter, I understand this is a property of the
> >>>> chipset. Chipsets are modelled unevenly.
> >>>>
> >>>> IIUC QEMU started with single-core CPUs.
> >>>> CPUState had same meaning for 'core' or 'cpu', 1-1 mapping.
> >>>>
> >>>> Then multicore CPUs could be easily modelled using multiple
> >>>> single-core CPUs, usually created in the machine code.
> >>>>
> >>>> Then we moved to SoC models, creating the cores in the SoC.
> >>>> Some SoCs have array of cores, eventually heterogeneous
> >>>> (see the ZynqMP). We have containers of CPUState.
> >>>>
> >>>> On an ARM-based SoC, you might have the first core started
> >>>> (as said Peter) or not.
> >>>>
> >>>> BCM2836 / BCM2837 and ZynqMP start will all ARM cores off.
> >>>> On the BCM chipsets, a DSP core will boot the ARM cores.
> >>>> On the ZynqMP, a MicroBlaze core boots them.
> >>>> As QEMU doesn't models heterogeneous architectures, we start
> >>>> modelling after the unmodelled cores booted us, so either one
> >>>> or all cores on.
> >>>>
> >>>> In this case, we narrowed down the 'start-powered-off' field
> >>>> to the SoC, which happens to be how ARM SoCs are modelled.
> >>>
> >>> I was not aware of the start-powered-off property.  If we make it
> >>> generic, we can just let spapr use it.
> >>>
> >>>>
> >>>>
> >>>> Chipsets providing a JTAG interface can have a SRST signal,
> >>>> the "system reset". When a JTAG probe is attached, it can
> >>>> keeps the whole chipset in a reset state. This is equivalent
> >>>> to QEMU '-S' mode (single step mode).
> >>>>
> >>>>
> >>>> I don't know about pseries hardware, but if this is 'explicit
> >>>> to paravirt platform', then I expect this to be the same with
> >>>> other accelerators/architectures.
> >>>>
> >>>> If paravirtualized -> cores start off by default. Let the
> >>>> hypervisor start them. So still a property of the CPUState
> >>>> depending on the accelerator used?
> >>>
> >>> I don't understand this part.  Why would this depend on the
> >>> accelerator?
> >>
> >> Because starting a virtualized machine with all cores powered-off
> >> with TCG accelerator should at least emit a warning? Or change
> >> the behavior and start them powered-on? This is machine-specific
> >> although.
> >>
> > 
> > FYI, PAPR requires all vCPUs to be "stopped" by default. It is up to the
> > guest to start them explicitly through an RTAS call. The hypervisor is
> > only responsible to start a single vCPU (see spapr_cpu_set_entry_state()
> > called from spapr_machine_reset()) to be able to boot the guest.
> > 
> > So I'm not sure to see how that would depend on the accelerator...
> 
> $ qemu-system-ppc64 -M pseries-5.0,accel=tcg -d in_asm
> qemu-system-ppc64: warning: TCG doesn't support requested feature,
> cap-cfpc=workaround
> qemu-system-ppc64: warning: TCG doesn't support requested feature,
> cap-sbbc=workaround
> qemu-system-ppc64: warning: TCG doesn't support requested feature,
> cap-ibs=workaround
> qemu-system-ppc64: warning: TCG doesn't support requested feature,
> cap-ccf-assist=on
> ----------------
> IN:
> 0x00000100:  48003f00  b        0x4000
> 
> ----------------
> IN:
> 0x00004000:  7c7f1b78  mr       r31, r3
> 0x00004004:  7d6000a6  mfmsr    r11
> 0x00004008:  3980a000  li       r12, 0xa000
> 0x0000400c:  798c83c6  sldi     r12, r12, 0x30
> 0x00004010:  7d6b6378  or       r11, r11, r12
> 0x00004014:  7d600164  mtmsrd   r11
> ...
> 
> The vCPU doesn't seem stopped to me...
> 
> Am I missing something?
> 

Yeah this is the boot vCPU which is required to be started
by the platform as explained above, but if you had more
vCPUs the other ones would be stopped until the guest OS
asks us to start them.

Re: [PATCH] cpu: Add starts_halted() method

[Philippe Mathieu-Daudé]

On 7/9/20 12:55 PM, Greg Kurz wrote:
> On Thu, 9 Jul 2020 12:18:06 +0200
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
>> On 7/9/20 11:54 AM, Greg Kurz wrote:
>>> On Thu, 9 Jul 2020 07:11:09 +0200
>>> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>>> On 7/8/20 11:39 PM, Eduardo Habkost wrote:
>>>>> On Wed, Jul 08, 2020 at 06:45:57PM +0200, Philippe Mathieu-Daudé wrote:
>>>>>> On 7/8/20 5:25 PM, Eduardo Habkost wrote:
>>>>>>> On Wed, Jul 08, 2020 at 02:14:03PM +0100, Peter Maydell wrote:
>>>>>>>> On Wed, 8 Jul 2020 at 12:12, David Gibson <david@gibson.dropbear.id.au> wrote:
>>>>>>>>> On Wed, Jul 08, 2020 at 10:38:29AM +0200, Philippe Mathieu-Daudé wrote:
>>>>>>>>>> Class boolean field certainly sounds better, but I am not sure this
>>>>>>>>>> is a property of the machine. Rather the arch? So move the field
>>>>>>>>>> to CPUClass? Maybe not, let's discuss :)
>>>>>>>>>
>>>>>>>>> It is absolutely a property of the machine.  e.g. I don't think we
>>>>>>>>> want this for powernv.  pseries is a bit of a special case since it is
>>>>>>>>> explicitly a paravirt platform.  But even for emulated hardware, the
>>>>>>>>> board can absolutely strap things so that cpus do or don't start
>>>>>>>>> immediately.
>>>>>>>>
>>>>>>>> It's a property of the individual CPU, I think. One common setup
>>>>>>>> for Arm systems is that the primary CPU starts powered up but
>>>>>>>> the secondaries all start powered down.
>>>>>>>
>>>>>>> Both statements can be true.  It can be a property of the
>>>>>>> individual CPU (although I'm not convinced it has to), but it
>>>>>>> still needs to be controlled by the machine.
>>>>>>
>>>>>> From what said Peter, I understand this is a property of the
>>>>>> chipset. Chipsets are modelled unevenly.
>>>>>>
>>>>>> IIUC QEMU started with single-core CPUs.
>>>>>> CPUState had same meaning for 'core' or 'cpu', 1-1 mapping.
>>>>>>
>>>>>> Then multicore CPUs could be easily modelled using multiple
>>>>>> single-core CPUs, usually created in the machine code.
>>>>>>
>>>>>> Then we moved to SoC models, creating the cores in the SoC.
>>>>>> Some SoCs have array of cores, eventually heterogeneous
>>>>>> (see the ZynqMP). We have containers of CPUState.
>>>>>>
>>>>>> On an ARM-based SoC, you might have the first core started
>>>>>> (as said Peter) or not.
>>>>>>
>>>>>> BCM2836 / BCM2837 and ZynqMP start will all ARM cores off.
>>>>>> On the BCM chipsets, a DSP core will boot the ARM cores.
>>>>>> On the ZynqMP, a MicroBlaze core boots them.
>>>>>> As QEMU doesn't models heterogeneous architectures, we start
>>>>>> modelling after the unmodelled cores booted us, so either one
>>>>>> or all cores on.
>>>>>>
>>>>>> In this case, we narrowed down the 'start-powered-off' field
>>>>>> to the SoC, which happens to be how ARM SoCs are modelled.
>>>>>
>>>>> I was not aware of the start-powered-off property.  If we make it
>>>>> generic, we can just let spapr use it.
>>>>>
>>>>>>
>>>>>>
>>>>>> Chipsets providing a JTAG interface can have a SRST signal,
>>>>>> the "system reset". When a JTAG probe is attached, it can
>>>>>> keeps the whole chipset in a reset state. This is equivalent
>>>>>> to QEMU '-S' mode (single step mode).
>>>>>>
>>>>>>
>>>>>> I don't know about pseries hardware, but if this is 'explicit
>>>>>> to paravirt platform', then I expect this to be the same with
>>>>>> other accelerators/architectures.
>>>>>>
>>>>>> If paravirtualized -> cores start off by default. Let the
>>>>>> hypervisor start them. So still a property of the CPUState
>>>>>> depending on the accelerator used?
>>>>>
>>>>> I don't understand this part.  Why would this depend on the
>>>>> accelerator?
>>>>
>>>> Because starting a virtualized machine with all cores powered-off
>>>> with TCG accelerator should at least emit a warning? Or change
>>>> the behavior and start them powered-on? This is machine-specific
>>>> although.
>>>>
>>>
>>> FYI, PAPR requires all vCPUs to be "stopped" by default. It is up to the
>>> guest to start them explicitly through an RTAS call. The hypervisor is
>>> only responsible to start a single vCPU (see spapr_cpu_set_entry_state()
>>> called from spapr_machine_reset()) to be able to boot the guest.
>>>
>>> So I'm not sure to see how that would depend on the accelerator...
>>
>> $ qemu-system-ppc64 -M pseries-5.0,accel=tcg -d in_asm
>> qemu-system-ppc64: warning: TCG doesn't support requested feature,
>> cap-cfpc=workaround
>> qemu-system-ppc64: warning: TCG doesn't support requested feature,
>> cap-sbbc=workaround
>> qemu-system-ppc64: warning: TCG doesn't support requested feature,
>> cap-ibs=workaround
>> qemu-system-ppc64: warning: TCG doesn't support requested feature,
>> cap-ccf-assist=on
>> ----------------
>> IN:
>> 0x00000100:  48003f00  b        0x4000
>>
>> ----------------
>> IN:
>> 0x00004000:  7c7f1b78  mr       r31, r3
>> 0x00004004:  7d6000a6  mfmsr    r11
>> 0x00004008:  3980a000  li       r12, 0xa000
>> 0x0000400c:  798c83c6  sldi     r12, r12, 0x30
>> 0x00004010:  7d6b6378  or       r11, r11, r12
>> 0x00004014:  7d600164  mtmsrd   r11
>> ...
>>
>> The vCPU doesn't seem stopped to me...
>>
>> Am I missing something?
>>
> 
> Yeah this is the boot vCPU which is required to be started
> by the platform as explained above, but if you had more
> vCPUs the other ones would be stopped until the guest OS
> asks us to start them.

Ah OK, so we are good :)

The machine simply has to set the 'start-powered-off' flag on
all vCPUS except the 1st one.

Re: [PATCH] cpu: Add starts_halted() method

[Greg Kurz]

On Thu, 9 Jul 2020 14:21:04 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 7/9/20 12:55 PM, Greg Kurz wrote:
> > On Thu, 9 Jul 2020 12:18:06 +0200
> > Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >

[...]

> >>>
> >>> FYI, PAPR requires all vCPUs to be "stopped" by default. It is up to the
> >>> guest to start them explicitly through an RTAS call. The hypervisor is
> >>> only responsible to start a single vCPU (see spapr_cpu_set_entry_state()
> >>> called from spapr_machine_reset()) to be able to boot the guest.
> >>>
> >>> So I'm not sure to see how that would depend on the accelerator...
> >>
> >> $ qemu-system-ppc64 -M pseries-5.0,accel=tcg -d in_asm
> >> qemu-system-ppc64: warning: TCG doesn't support requested feature,
> >> cap-cfpc=workaround
> >> qemu-system-ppc64: warning: TCG doesn't support requested feature,
> >> cap-sbbc=workaround
> >> qemu-system-ppc64: warning: TCG doesn't support requested feature,
> >> cap-ibs=workaround
> >> qemu-system-ppc64: warning: TCG doesn't support requested feature,
> >> cap-ccf-assist=on
> >> ----------------
> >> IN:
> >> 0x00000100:  48003f00  b        0x4000
> >>
> >> ----------------
> >> IN:
> >> 0x00004000:  7c7f1b78  mr       r31, r3
> >> 0x00004004:  7d6000a6  mfmsr    r11
> >> 0x00004008:  3980a000  li       r12, 0xa000
> >> 0x0000400c:  798c83c6  sldi     r12, r12, 0x30
> >> 0x00004010:  7d6b6378  or       r11, r11, r12
> >> 0x00004014:  7d600164  mtmsrd   r11
> >> ...
> >>
> >> The vCPU doesn't seem stopped to me...
> >>
> >> Am I missing something?
> >>
> > 
> > Yeah this is the boot vCPU which is required to be started
> > by the platform as explained above, but if you had more
> > vCPUs the other ones would be stopped until the guest OS
> > asks us to start them.
> 
> Ah OK, so we are good :)
> 
> The machine simply has to set the 'start-powered-off' flag on
> all vCPUS except the 1st one.
> 

We only want the first vCPU to start when the platform is
fully configured, so I'd rather put 'start-powered-off' on
every body and explicitly power on the first one during
machine reset as we do now.

Re: [PATCH] cpu: Add starts_halted() method

[Philippe Mathieu-Daudé]

On 7/9/20 3:13 PM, Greg Kurz wrote:
> On Thu, 9 Jul 2020 14:21:04 +0200
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
>> On 7/9/20 12:55 PM, Greg Kurz wrote:
>>> On Thu, 9 Jul 2020 12:18:06 +0200
>>> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>>
> 
> [...]
> 
>>>>>
>>>>> FYI, PAPR requires all vCPUs to be "stopped" by default. It is up to the
>>>>> guest to start them explicitly through an RTAS call. The hypervisor is
>>>>> only responsible to start a single vCPU (see spapr_cpu_set_entry_state()
>>>>> called from spapr_machine_reset()) to be able to boot the guest.
>>>>>
>>>>> So I'm not sure to see how that would depend on the accelerator...
>>>>
>>>> $ qemu-system-ppc64 -M pseries-5.0,accel=tcg -d in_asm
>>>> qemu-system-ppc64: warning: TCG doesn't support requested feature,
>>>> cap-cfpc=workaround
>>>> qemu-system-ppc64: warning: TCG doesn't support requested feature,
>>>> cap-sbbc=workaround
>>>> qemu-system-ppc64: warning: TCG doesn't support requested feature,
>>>> cap-ibs=workaround
>>>> qemu-system-ppc64: warning: TCG doesn't support requested feature,
>>>> cap-ccf-assist=on
>>>> ----------------
>>>> IN:
>>>> 0x00000100:  48003f00  b        0x4000
>>>>
>>>> ----------------
>>>> IN:
>>>> 0x00004000:  7c7f1b78  mr       r31, r3
>>>> 0x00004004:  7d6000a6  mfmsr    r11
>>>> 0x00004008:  3980a000  li       r12, 0xa000
>>>> 0x0000400c:  798c83c6  sldi     r12, r12, 0x30
>>>> 0x00004010:  7d6b6378  or       r11, r11, r12
>>>> 0x00004014:  7d600164  mtmsrd   r11
>>>> ...
>>>>
>>>> The vCPU doesn't seem stopped to me...
>>>>
>>>> Am I missing something?
>>>>
>>>
>>> Yeah this is the boot vCPU which is required to be started
>>> by the platform as explained above, but if you had more
>>> vCPUs the other ones would be stopped until the guest OS
>>> asks us to start them.
>>
>> Ah OK, so we are good :)
>>
>> The machine simply has to set the 'start-powered-off' flag on
>> all vCPUS except the 1st one.
>>
> 
> We only want the first vCPU to start when the platform is
> fully configured, so I'd rather put 'start-powered-off' on
> every body and explicitly power on the first one during
> machine reset as we do now.

I meant "we are good" in reference to the beginning of
this thread with Thiago and Eduardo:

- 'start-powered-off' is a CPU feature (not machine)
- machine set the 'start-powered-off' field

Re: [PATCH] cpu: Add starts_halted() method

[Peter Maydell]

On Thu, 9 Jul 2020 at 14:13, Greg Kurz <groug@kaod.org> wrote:
> On Thu, 9 Jul 2020 14:21:04 +0200
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> > The machine simply has to set the 'start-powered-off' flag on
> > all vCPUS except the 1st one.
> >
>
> We only want the first vCPU to start when the platform is
> fully configured, so I'd rather put 'start-powered-off' on
> every body and explicitly power on the first one during
> machine reset as we do now.

Nothing should ever be able to run before the first
machine reset has completed: that would be a pretty bad bug.

thanks
-- PMM