On 7/1/20 10:35 PM, Peter Maydell wrote: > 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".
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 >
