| Message ID | 20200701182100.26930-1-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] cpus: Initialize current_cpu with the first vCPU created | expand |
On 200701 2021, Philippe Mathieu-Daudé wrote: > We can run I/O access with the 'i' or 'o' HMP commands in the > monitor. These commands are expected to run on a vCPU. The > monitor is not a vCPU thread. To avoid crashing, initialize > the 'current_cpu' variable with the first vCPU created. The > command executed on the monitor will end using it. > > This fixes: > > $ cat << EOF| qemu-system-i386 -M q35 -nographic -serial none -monitor stdio > o/4 0xcf8 0x8400f841 > o/4 0xcfc 0xaa215d6d > o/4 0x6d30 0x2ef8ffbe > o/1 0xb2 0x20 > EOF > Segmentation fault (core dumped) > > Thread 1 "qemu-system-i38" received signal SIGSEGV, Segmentation fault. > 0x00005555558946c7 in tcg_handle_interrupt (cpu=0x0, mask=64) at accel/tcg/tcg-all.c:57 > 57 old_mask = cpu->interrupt_request; > (gdb) bt > #0 0x00005555558946c7 in tcg_handle_interrupt (cpu=0x0, mask=64) at accel/tcg/tcg-all.c:57 > #1 0x00005555558ed7d2 in cpu_interrupt (cpu=0x0, mask=64) at include/hw/core/cpu.h:877 > #2 0x00005555558ee776 in ich9_apm_ctrl_changed (val=32, arg=0x555556e2ff50) at hw/isa/lpc_ich9.c:442 > #3 0x0000555555b47f96 in apm_ioport_writeb (opaque=0x555556e308c0, addr=0, val=32, size=1) at hw/isa/apm.c:44 > #4 0x0000555555879931 in memory_region_write_accessor (mr=0x555556e308e0, addr=0, value=0x7fffffffb9f8, size=1, shift=0, mask=255, attrs=...) at memory.c:483 > #5 0x0000555555879b5a in access_with_adjusted_size (addr=0, value=0x7fffffffb9f8, size=4, access_size_min=1, access_size_max=1, access_fn= > 0x55555587984e <memory_region_write_accessor>, mr=0x555556e308e0, attrs=...) at memory.c:544 > #6 0x000055555587ca32 in memory_region_dispatch_write (mr=0x555556e308e0, addr=0, data=32, op=MO_32, attrs=...) at memory.c:1465 > #7 0x000055555581b7e9 in flatview_write_continue (fv=0x55555698a790, addr=178, attrs=..., ptr=0x7fffffffbb84, len=4, addr1=0, l=4, mr=0x555556e308e0) at exec.c:3198 > #8 0x000055555581b92e in flatview_write (fv=0x55555698a790, addr=178, attrs=..., buf=0x7fffffffbb84, len=4) at exec.c:3238 > #9 0x000055555581bc81 in address_space_write (as=0x555556441220 <address_space_io>, addr=178, attrs=..., buf=0x7fffffffbb84, len=4) at exec.c:3329 > #10 0x0000555555873f08 in cpu_outl (addr=178, val=32) at ioport.c:80 > #11 0x000055555598a26a in hmp_ioport_write (mon=0x5555567621b0, qdict=0x555557702600) at monitor/misc.c:937 > #12 0x0000555555c9c5a5 in handle_hmp_command (mon=0x5555567621b0, cmdline=0x55555676aae1 "/1 0xb2 0x20") at monitor/hmp.c:1082 > #13 0x0000555555c99e02 in monitor_command_cb (opaque=0x5555567621b0, cmdline=0x55555676aae0 "o/1 0xb2 0x20", readline_opaque=0x0) at monitor/hmp.c:47 > ^ > HMP command from monitor > > Reported-by: Alexander Bulekov <alxndr@bu.edu> > Buglink: https://bugs.launchpad.net/qemu/+bug/1878645 > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > Cc: Bug 1878645 <1878645@bugs.launchpad.net> > > RFC because I believe the correct fix is to NOT use current_cpu > out of cpus.c, at least use qemu_get_cpu(0) to get the first vCPU. > --- > cpus.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/cpus.c b/cpus.c > index 41d1c5099f..1f6f43d221 100644 > --- a/cpus.c > +++ b/cpus.c > @@ -2106,6 +2106,9 @@ void qemu_init_vcpu(CPUState *cpu) > { > MachineState *ms = MACHINE(qdev_get_machine()); > > + if (!current_cpu) { > + current_cpu = cpu; > + } Seems like a neat solution. is it fair to assume that qemu_init_vcpu is called before any threads that can do I/O are created? I confirmed that the qtest and hmp crashes are avoided. -Alex > cpu->nr_cores = ms->smp.cores; > cpu->nr_threads = ms->smp.threads; > cpu->stopped = true; > -- > 2.21.3 >
On Wed, 1 Jul 2020 at 19:21, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > We can run I/O access with the 'i' or 'o' HMP commands in the > monitor. These commands are expected to run on a vCPU. The > monitor is not a vCPU thread. To avoid crashing, initialize > the 'current_cpu' variable with the first vCPU created. The > command executed on the monitor will end using it. > > RFC because I believe the correct fix is to NOT use current_cpu > out of cpus.c, at least use qemu_get_cpu(0) to get the first vCPU. Yes, I agree -- I don't think this is the correct fix. current_cpu is documented as "only valid inside cpu_exec()", ie if you're actually on a vcpu thread you can use it, but if you're not on a CPU thread, like the monitor, you need to say which vCPU you want to affect. For the monitor, that would be the current "default cpu" as set by the "cpu" command (cf monitor_set_cpu()). The bug here will be that somewhere along the line we are probably missing plumbing sufficient to pass down "which CPU do we want". https://bugs.launchpad.net/qemu/+bug/1602247 is a bug of a similar nature -- if the gdbstub does a memory access we know which CPU we're trying to do that memory access as, but it doesn't get plumbed through and so in the Arm GIC register read/write function that looks at current_cpu we get a segfault. One way to fix this would be to do something akin to how real hardware works -- encode into the MemTxAttrs an indication of what the transaction master was (eg the CPU number for CPUs); then the handful of devices that care about which CPU was doing the transaction can use that, rather than directly looking at current_cpu, and when gdbstub or monitor perform an access that should act like it's from a particular CPU they can indicate that by supplying appropriate transaction attributes. That would potentially be quite a bit of work though (but I think it would be a neat design if we want to try to fix this kind of "segfault if the user prods a device via the monitor" bug). + if (!current_cpu) { + current_cpu = cpu; + } Some more specific issues with this -- current_cpu is a thread-local variable, so this will set that for whatever thread happens to make this call, which might or might not be the one that ends up handling the monitor command. Also some code assumes that current_cpu is non-NULL only if this is a vCPU thread, eg sigbus_handler(). thanks -- PMM
On 7/1/20 10:35 PM, Peter Maydell wrote: > On Wed, 1 Jul 2020 at 19:21, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> >> We can run I/O access with the 'i' or 'o' HMP commands in the >> monitor. These commands are expected to run on a vCPU. The >> monitor is not a vCPU thread. To avoid crashing, initialize >> the 'current_cpu' variable with the first vCPU created. The >> command executed on the monitor will end using it. > >> >> RFC because I believe the correct fix is to NOT use current_cpu >> out of cpus.c, at least use qemu_get_cpu(0) to get the first vCPU. > > Yes, I agree -- I don't think this is the correct fix. > current_cpu is documented as "only valid inside cpu_exec()", > ie if you're actually on a vcpu thread you can use it, but if > you're not on a CPU thread, like the monitor, you need to > say which vCPU you want to affect. For the monitor, that > would be the current "default cpu" as set by the "cpu" > command (cf monitor_set_cpu()). The bug here will be that > somewhere along the line we are probably missing plumbing > sufficient to pass down "which CPU do we want". TIL mon_get_cpu() :) This is a bit different here, the monitor is not doing an access on a CPU address space, but directly on the I/O address space. The APM port is registered by the ICH9 south bridge, and this triggers a SMI exception on a CPU core, but I have no idea which one, a random one? Then this particular core will switch to SMM mode. Another way of seeing this problem, is imagining we start a PIT timer from the monitor. When the timer expires, the PIT will interrupt the CPU. Which CPU to interrupt? We are not in the context of the monitor. > https://bugs.launchpad.net/qemu/+bug/1602247 is a bug of > a similar nature -- if the gdbstub does a memory access > we know which CPU we're trying to do that memory access as, > but it doesn't get plumbed through and so in the Arm GIC > register read/write function that looks at current_cpu > we get a segfault. > > One way to fix this would be to do something akin to how > real hardware works -- encode into the MemTxAttrs an > indication of what the transaction master was (eg the > CPU number for CPUs); then the handful of devices that > care about which CPU was doing the transaction can use > that, rather than directly looking at current_cpu, and > when gdbstub or monitor perform an access that should > act like it's from a particular CPU they can indicate > that by supplying appropriate transaction attributes. > That would potentially be quite a bit of work though > (but I think it would be a neat design if we want to > try to fix this kind of "segfault if the user prods > a device via the monitor" bug). This is complex stuff. Too early for me to digest, I am keeping this for later (I am not ignoring your comment). > > + if (!current_cpu) { > + current_cpu = cpu; > + } > > Some more specific issues with this -- current_cpu is > a thread-local variable, so this will set that for > whatever thread happens to make this call, which > might or might not be the one that ends up handling > the monitor command. Also some code assumes that > current_cpu is non-NULL only if this is a vCPU thread, > eg sigbus_handler(). Yes, I agree. > > thanks > -- PMM >
On 01/07/20 22:35, Peter Maydell wrote: > For the monitor, that > would be the current "default cpu" as set by the "cpu" > command (cf monitor_set_cpu()). The bug here will be that > somewhere along the line we are probably missing plumbing > sufficient to pass down "which CPU do we want". Yeah, the fix is probably to add a functions that returns either current_cpu or the monitor CPU, and use it in device emulation if applicable. The problem with current_cpu is that it affects stuff like run_on_cpu, and that is guaranteed to cause havoc to code that expects to run on a given CPU and therefore doesn't use locks. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 01/07/20 22:35, Peter Maydell wrote: >> For the monitor, that >> would be the current "default cpu" as set by the "cpu" >> command (cf monitor_set_cpu()). The bug here will be that >> somewhere along the line we are probably missing plumbing >> sufficient to pass down "which CPU do we want". > > Yeah, the fix is probably to add a functions that returns either > current_cpu or the monitor CPU, and use it in device emulation if > applicable. > > The problem with current_cpu is that it affects stuff like run_on_cpu, > and that is guaranteed to cause havoc to code that expects to run on a > given CPU and therefore doesn't use locks. IIRC the original reported bug was in a APM callback which was triggered by an MMIO operation. Currently we don't expose the current cpu via the memory dispatch routines. Should we to ensure there is always something valid there? > > Paolo
On 200701 2135, Peter Maydell wrote: > On Wed, 1 Jul 2020 at 19:21, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > > > We can run I/O access with the 'i' or 'o' HMP commands in the > > monitor. These commands are expected to run on a vCPU. The > > monitor is not a vCPU thread. To avoid crashing, initialize > > the 'current_cpu' variable with the first vCPU created. The > > command executed on the monitor will end using it. > > > > > RFC because I believe the correct fix is to NOT use current_cpu > > out of cpus.c, at least use qemu_get_cpu(0) to get the first vCPU. > > Yes, I agree -- I don't think this is the correct fix. > current_cpu is documented as "only valid inside cpu_exec()", > ie if you're actually on a vcpu thread you can use it, but if > you're not on a CPU thread, like the monitor, you need to > say which vCPU you want to affect. For the monitor, that Following up on this old thread.. Does qtest count as a pseudo-vCPU? Since qtest already uses first_cpu for all of its address_space calls, would it be appropriate to set current_cpu to first_cpu in qtest's initialization? Thank you -Alex > would be the current "default cpu" as set by the "cpu" > command (cf monitor_set_cpu()). The bug here will be that > somewhere along the line we are probably missing plumbing > sufficient to pass down "which CPU do we want". > > https://bugs.launchpad.net/qemu/+bug/1602247 is a bug of > a similar nature -- if the gdbstub does a memory access > we know which CPU we're trying to do that memory access as, > but it doesn't get plumbed through and so in the Arm GIC > register read/write function that looks at current_cpu > we get a segfault. > > One way to fix this would be to do something akin to how > real hardware works -- encode into the MemTxAttrs an > indication of what the transaction master was (eg the > CPU number for CPUs); then the handful of devices that > care about which CPU was doing the transaction can use > that, rather than directly looking at current_cpu, and > when gdbstub or monitor perform an access that should > act like it's from a particular CPU they can indicate > that by supplying appropriate transaction attributes. > That would potentially be quite a bit of work though > (but I think it would be a neat design if we want to > try to fix this kind of "segfault if the user prods > a device via the monitor" bug). > > + if (!current_cpu) { > + current_cpu = cpu; > + } > > Some more specific issues with this -- current_cpu is > a thread-local variable, so this will set that for > whatever thread happens to make this call, which > might or might not be the one that ends up handling > the monitor command. Also some code assumes that > current_cpu is non-NULL only if this is a vCPU thread, > eg sigbus_handler(). > > thanks > -- PMM
On 07/09/20 22:35, Alexander Bulekov wrote: >>> RFC because I believe the correct fix is to NOT use current_cpu >>> out of cpus.c, at least use qemu_get_cpu(0) to get the first vCPU. >> Yes, I agree -- I don't think this is the correct fix. >> current_cpu is documented as "only valid inside cpu_exec()", >> ie if you're actually on a vcpu thread you can use it, but if >> you're not on a CPU thread, like the monitor, you need to >> say which vCPU you want to affect. For the monitor, that > Following up on this old thread.. Does qtest count as a pseudo-vCPU? > Since qtest already uses first_cpu for all of its address_space calls, > would it be appropriate to set current_cpu to first_cpu in qtest's > initialization? No, current_cpu should never be set on non-vCPU threads. Paolo
Le mar. 8 sept. 2020 08:33, Paolo Bonzini <pbonzini@redhat.com> a écrit : > On 07/09/20 22:35, Alexander Bulekov wrote: > >>> RFC because I believe the correct fix is to NOT use current_cpu > >>> out of cpus.c, at least use qemu_get_cpu(0) to get the first vCPU. > >> Yes, I agree -- I don't think this is the correct fix. > >> current_cpu is documented as "only valid inside cpu_exec()", > >> ie if you're actually on a vcpu thread you can use it, but if > >> you're not on a CPU thread, like the monitor, you need to > >> say which vCPU you want to affect. For the monitor, that > > Following up on this old thread.. Does qtest count as a pseudo-vCPU? > > Since qtest already uses first_cpu for all of its address_space calls, > > would it be appropriate to set current_cpu to first_cpu in qtest's > > initialization? > qtest doesn't provide vCPU so first_cpu = NULL, that wouldn't help. > No, current_cpu should never be set on non-vCPU threads. > > Paolo > > >
On 08/09/20 08:39, Philippe Mathieu-Daudé wrote: > Le mar. 8 sept. 2020 08:33, Paolo Bonzini <pbonzini@redhat.com > <mailto:pbonzini@redhat.com>> a écrit : > > On 07/09/20 22:35, Alexander Bulekov wrote: > >>> RFC because I believe the correct fix is to NOT use current_cpu > >>> out of cpus.c, at least use qemu_get_cpu(0) to get the first vCPU. > >> Yes, I agree -- I don't think this is the correct fix. > >> current_cpu is documented as "only valid inside cpu_exec()", > >> ie if you're actually on a vcpu thread you can use it, but if > >> you're not on a CPU thread, like the monitor, you need to > >> say which vCPU you want to affect. For the monitor, that > > Following up on this old thread.. Does qtest count as a pseudo-vCPU? > > Since qtest already uses first_cpu for all of its address_space calls, > > would it be appropriate to set current_cpu to first_cpu in qtest's > > initialization? > > > qtest doesn't provide vCPU so first_cpu = NULL, that wouldn't help. Doesn't qtest use qemu_dummy_start_vcpu? Paolo
diff --git a/cpus.c b/cpus.c index 41d1c5099f..1f6f43d221 100644 --- a/cpus.c +++ b/cpus.c @@ -2106,6 +2106,9 @@ void qemu_init_vcpu(CPUState *cpu) { MachineState *ms = MACHINE(qdev_get_machine()); + if (!current_cpu) { + current_cpu = cpu; + } cpu->nr_cores = ms->smp.cores; cpu->nr_threads = ms->smp.threads; cpu->stopped = true;
We can run I/O access with the 'i' or 'o' HMP commands in the monitor. These commands are expected to run on a vCPU. The monitor is not a vCPU thread. To avoid crashing, initialize the 'current_cpu' variable with the first vCPU created. The command executed on the monitor will end using it. This fixes: $ cat << EOF| qemu-system-i386 -M q35 -nographic -serial none -monitor stdio o/4 0xcf8 0x8400f841 o/4 0xcfc 0xaa215d6d o/4 0x6d30 0x2ef8ffbe o/1 0xb2 0x20 EOF Segmentation fault (core dumped) Thread 1 "qemu-system-i38" received signal SIGSEGV, Segmentation fault. 0x00005555558946c7 in tcg_handle_interrupt (cpu=0x0, mask=64) at accel/tcg/tcg-all.c:57 57 old_mask = cpu->interrupt_request; (gdb) bt #0 0x00005555558946c7 in tcg_handle_interrupt (cpu=0x0, mask=64) at accel/tcg/tcg-all.c:57 #1 0x00005555558ed7d2 in cpu_interrupt (cpu=0x0, mask=64) at include/hw/core/cpu.h:877 #2 0x00005555558ee776 in ich9_apm_ctrl_changed (val=32, arg=0x555556e2ff50) at hw/isa/lpc_ich9.c:442 #3 0x0000555555b47f96 in apm_ioport_writeb (opaque=0x555556e308c0, addr=0, val=32, size=1) at hw/isa/apm.c:44 #4 0x0000555555879931 in memory_region_write_accessor (mr=0x555556e308e0, addr=0, value=0x7fffffffb9f8, size=1, shift=0, mask=255, attrs=...) at memory.c:483 #5 0x0000555555879b5a in access_with_adjusted_size (addr=0, value=0x7fffffffb9f8, size=4, access_size_min=1, access_size_max=1, access_fn= 0x55555587984e <memory_region_write_accessor>, mr=0x555556e308e0, attrs=...) at memory.c:544 #6 0x000055555587ca32 in memory_region_dispatch_write (mr=0x555556e308e0, addr=0, data=32, op=MO_32, attrs=...) at memory.c:1465 #7 0x000055555581b7e9 in flatview_write_continue (fv=0x55555698a790, addr=178, attrs=..., ptr=0x7fffffffbb84, len=4, addr1=0, l=4, mr=0x555556e308e0) at exec.c:3198 #8 0x000055555581b92e in flatview_write (fv=0x55555698a790, addr=178, attrs=..., buf=0x7fffffffbb84, len=4) at exec.c:3238 #9 0x000055555581bc81 in address_space_write (as=0x555556441220 <address_space_io>, addr=178, attrs=..., buf=0x7fffffffbb84, len=4) at exec.c:3329 #10 0x0000555555873f08 in cpu_outl (addr=178, val=32) at ioport.c:80 #11 0x000055555598a26a in hmp_ioport_write (mon=0x5555567621b0, qdict=0x555557702600) at monitor/misc.c:937 #12 0x0000555555c9c5a5 in handle_hmp_command (mon=0x5555567621b0, cmdline=0x55555676aae1 "/1 0xb2 0x20") at monitor/hmp.c:1082 #13 0x0000555555c99e02 in monitor_command_cb (opaque=0x5555567621b0, cmdline=0x55555676aae0 "o/1 0xb2 0x20", readline_opaque=0x0) at monitor/hmp.c:47 ^ HMP command from monitor Reported-by: Alexander Bulekov <alxndr@bu.edu> Buglink: https://bugs.launchpad.net/qemu/+bug/1878645 Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- Cc: Bug 1878645 <1878645@bugs.launchpad.net> RFC because I believe the correct fix is to NOT use current_cpu out of cpus.c, at least use qemu_get_cpu(0) to get the first vCPU. --- cpus.c | 3 +++ 1 file changed, 3 insertions(+)