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

[Alexander Bulekov]

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