Re: [RFC PATCH] cpus: Initialize current_cpu with the first vCPU created

[Peter Maydell]

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