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

[Philippe Mathieu-Daudé]

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
>