Re: [PATCH] cpu: Add starts_halted() method
Thiago Jung Bauermann writes: > Alex Bennée writes: > >> Thiago Jung Bauermann writes: >> >>> Eduardo Habkost writes: >>> On Wed, Jul 08, 2020 at 09:11:55PM +0100, Peter Maydell wrote: > On Wed, 8 Jul 2020 at 18:36, Eduardo Habkost 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 () >> >>> #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
On Fri, Jul 10, 2020 at 05:02:38PM -0300, Thiago Jung Bauermann wrote: > > Philippe Mathieu-Daudé writes: > > > On 7/9/20 5:26 AM, Thiago Jung Bauermann wrote: > >> > >> Thiago Jung Bauermann 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
Alex Bennée writes: > Thiago Jung Bauermann writes: > >> Eduardo Habkost writes: >> >>> On Wed, Jul 08, 2020 at 09:11:55PM +0100, Peter Maydell wrote: On Wed, 8 Jul 2020 at 18:36, Eduardo Habkost 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 () > >> #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
Philippe Mathieu-Daudé writes: > On 7/9/20 5:26 AM, Thiago Jung Bauermann wrote: >> >> Thiago Jung Bauermann 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
On Thu, 9 Jul 2020 at 14:13, Greg Kurz wrote: > On Thu, 9 Jul 2020 14:21:04 +0200 > Philippe Mathieu-Daudé 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
Re: [PATCH] cpu: Add starts_halted() method
On 7/9/20 3:13 PM, Greg Kurz wrote: > On Thu, 9 Jul 2020 14:21:04 +0200 > Philippe Mathieu-Daudé wrote: > >> On 7/9/20 12:55 PM, Greg Kurz wrote: >>> On Thu, 9 Jul 2020 12:18:06 +0200 >>> Philippe Mathieu-Daudé 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: 0x0100: 48003f00 b0x4000 IN: 0x4000: 7c7f1b78 mr r31, r3 0x4004: 7d6000a6 mfmsrr11 0x4008: 3980a000 li r12, 0xa000 0x400c: 798c83c6 sldi r12, r12, 0x30 0x4010: 7d6b6378 or r11, r11, r12 0x4014: 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
On Thu, 9 Jul 2020 14:21:04 +0200 Philippe Mathieu-Daudé wrote: > On 7/9/20 12:55 PM, Greg Kurz wrote: > > On Thu, 9 Jul 2020 12:18:06 +0200 > > Philippe Mathieu-Daudé 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: > >> 0x0100: 48003f00 b0x4000 > >> > >> > >> IN: > >> 0x4000: 7c7f1b78 mr r31, r3 > >> 0x4004: 7d6000a6 mfmsrr11 > >> 0x4008: 3980a000 li r12, 0xa000 > >> 0x400c: 798c83c6 sldi r12, r12, 0x30 > >> 0x4010: 7d6b6378 or r11, r11, r12 > >> 0x4014: 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
On 7/9/20 12:55 PM, Greg Kurz wrote: > On Thu, 9 Jul 2020 12:18:06 +0200 > Philippe Mathieu-Daudé wrote: > >> On 7/9/20 11:54 AM, Greg Kurz wrote: >>> On Thu, 9 Jul 2020 07:11:09 +0200 >>> Philippe Mathieu-Daudé 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 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: >> 0x0100: 48003f00 b0x4000 >> >> >> IN: >> 0x4000: 7c7f1b78 mr r31, r3 >> 0x4004: 7d6000a6 mfmsrr11 >> 0x4008: 3980a000 li r12, 0xa000 >> 0x400c: 798c83c6 sldi r12, r12, 0x30 >> 0x4010: 7d6b6378 or r11, r11, r12 >> 0x4014:
Re: [PATCH] cpu: Add starts_halted() method
On Thu, 9 Jul 2020 12:18:06 +0200 Philippe Mathieu-Daudé wrote: > On 7/9/20 11:54 AM, Greg Kurz wrote: > > On Thu, 9 Jul 2020 07:11:09 +0200 > > Philippe Mathieu-Daudé 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 > >> 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: > 0x0100: 48003f00 b0x4000 > > > IN: > 0x4000: 7c7f1b78 mr r31, r3 > 0x4004: 7d6000a6 mfmsrr11 > 0x4008: 3980a000 li r12, 0xa000 > 0x400c: 798c83c6 sldi r12, r12, 0x30 > 0x4010: 7d6b6378 or r11, r11, r12 > 0x4014: 7d600164 mtmsrd r11 > ... > > The vCPU doesn't seem stopped to
Re: [PATCH] cpu: Add starts_halted() method
On 7/9/20 5:26 AM, Thiago Jung Bauermann wrote: > > Thiago Jung Bauermann 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
On 7/9/20 11:54 AM, Greg Kurz wrote: > On Thu, 9 Jul 2020 07:11:09 +0200 > Philippe Mathieu-Daudé 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 >> 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: 0x0100: 48003f00 b0x4000 IN: 0x4000: 7c7f1b78 mr r31, r3 0x4004: 7d6000a6 mfmsrr11 0x4008: 3980a000 li r12, 0xa000 0x400c: 798c83c6 sldi r12, r12, 0x30 0x4010: 7d6b6378 or r11, r11, r12 0x4014: 7d600164 mtmsrd r11 ... The vCPU doesn't seem stopped to me... Am I missing something?
Re: [PATCH] cpu: Add starts_halted() method
On Thu, 9 Jul 2020 07:11:09 +0200 Philippe Mathieu-Daudé 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 > 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
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 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
Thiago Jung Bauermann 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
Eduardo Habkost writes: > On Wed, Jul 08, 2020 at 09:11:55PM +0100, Peter Maydell wrote: >> On Wed, 8 Jul 2020 at 18:36, Eduardo Habkost 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
Hi Thiago,
On 7/8/20 1:28 AM, Thiago Jung Bauermann wrote:
>
> Hello Eduardo,
>
> Eduardo Habkost 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
>> [...]
>>> +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
>
> 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
>
> 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(>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
On Wed, Jul 08, 2020 at 04:32:51PM +0100, Peter Maydell wrote: > On Wed, 8 Jul 2020 at 16:25, Eduardo Habkost 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
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
[...]
> +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
---
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(>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
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 >> 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
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 > >> 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
On Wed, Jul 08, 2020 at 06:09:49PM +0100, Peter Maydell wrote: > On Wed, 8 Jul 2020 at 17:03, Eduardo Habkost wrote: > > > > On Wed, Jul 08, 2020 at 04:32:51PM +0100, Peter Maydell wrote: > > > On Wed, 8 Jul 2020 at 16:25, Eduardo Habkost 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
On Wed, Jul 08, 2020 at 09:11:55PM +0100, Peter Maydell wrote: > On Wed, 8 Jul 2020 at 18:36, Eduardo Habkost 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
On Wed, Jul 08, 2020 at 02:14:03PM +0100, Peter Maydell wrote: > On Wed, 8 Jul 2020 at 12:12, David Gibson 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
On Wed, 8 Jul 2020 at 18:36, Eduardo Habkost 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
On Wed, 8 Jul 2020 at 17:03, Eduardo Habkost wrote: > > On Wed, Jul 08, 2020 at 04:32:51PM +0100, Peter Maydell wrote: > > On Wed, 8 Jul 2020 at 16:25, Eduardo Habkost 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
On Wed, 8 Jul 2020 at 16:25, Eduardo Habkost 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
On Wed, 8 Jul 2020 at 12:12, David Gibson 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
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 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
> >> [...]
> >>> +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
signature.asc
Description: PGP signature
Re: [PATCH] cpu: Add starts_halted() method
Hello Eduardo,
Eduardo Habkost 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
> [...]
>> +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
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
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(>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
