| Message ID | 20200707204333.261506-1-bauerman@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | cpu: Add starts_halted() method | expand |
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; > +} > +
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
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 >
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.
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
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?
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
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.
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.
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
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).
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
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.
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 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
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 ()
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.
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... >
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?
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 () >
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.
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.
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.
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
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
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
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
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.
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.
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"; }
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(-)