On Wed, 1 Jul 2020 at 19:21, Philippe Mathieu-Daudé <phi...@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
