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

[Alexander Bulekov]

On 200701 2021, Philippe Mathieu-Daudé 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.
> 
> This fixes:
> 
>   $ cat << EOF| qemu-system-i386 -M q35 -nographic -serial none -monitor stdio
>   o/4 0xcf8 0x8400f841
>   o/4 0xcfc 0xaa215d6d
>   o/4 0x6d30 0x2ef8ffbe
>   o/1 0xb2 0x20
>   EOF
>   Segmentation fault (core dumped)
> 
>   Thread 1 "qemu-system-i38" received signal SIGSEGV, Segmentation fault.
>   0x00005555558946c7 in tcg_handle_interrupt (cpu=0x0, mask=64) at 
> accel/tcg/tcg-all.c:57
>   57          old_mask = cpu->interrupt_request;
>   (gdb) bt
>   #0  0x00005555558946c7 in tcg_handle_interrupt (cpu=0x0, mask=64) at 
> accel/tcg/tcg-all.c:57
>   #1  0x00005555558ed7d2 in cpu_interrupt (cpu=0x0, mask=64) at 
> include/hw/core/cpu.h:877
>   #2  0x00005555558ee776 in ich9_apm_ctrl_changed (val=32, 
> arg=0x555556e2ff50) at hw/isa/lpc_ich9.c:442
>   #3  0x0000555555b47f96 in apm_ioport_writeb (opaque=0x555556e308c0, addr=0, 
> val=32, size=1) at hw/isa/apm.c:44
>   #4  0x0000555555879931 in memory_region_write_accessor (mr=0x555556e308e0, 
> addr=0, value=0x7fffffffb9f8, size=1, shift=0, mask=255, attrs=...) at 
> memory.c:483
>   #5  0x0000555555879b5a in access_with_adjusted_size (addr=0, 
> value=0x7fffffffb9f8, size=4, access_size_min=1, access_size_max=1, access_fn=
>       0x55555587984e <memory_region_write_accessor>, mr=0x555556e308e0, 
> attrs=...) at memory.c:544
>   #6  0x000055555587ca32 in memory_region_dispatch_write (mr=0x555556e308e0, 
> addr=0, data=32, op=MO_32, attrs=...) at memory.c:1465
>   #7  0x000055555581b7e9 in flatview_write_continue (fv=0x55555698a790, 
> addr=178, attrs=..., ptr=0x7fffffffbb84, len=4, addr1=0, l=4, 
> mr=0x555556e308e0) at exec.c:3198
>   #8  0x000055555581b92e in flatview_write (fv=0x55555698a790, addr=178, 
> attrs=..., buf=0x7fffffffbb84, len=4) at exec.c:3238
>   #9  0x000055555581bc81 in address_space_write (as=0x555556441220 
> <address_space_io>, addr=178, attrs=..., buf=0x7fffffffbb84, len=4) at 
> exec.c:3329
>   #10 0x0000555555873f08 in cpu_outl (addr=178, val=32) at ioport.c:80
>   #11 0x000055555598a26a in hmp_ioport_write (mon=0x5555567621b0, 
> qdict=0x555557702600) at monitor/misc.c:937
>   #12 0x0000555555c9c5a5 in handle_hmp_command (mon=0x5555567621b0, 
> cmdline=0x55555676aae1 "/1 0xb2 0x20") at monitor/hmp.c:1082
>   #13 0x0000555555c99e02 in monitor_command_cb (opaque=0x5555567621b0, 
> cmdline=0x55555676aae0 "o/1 0xb2 0x20", readline_opaque=0x0) at 
> monitor/hmp.c:47
>                             ^
>     HMP command from monitor
> 
> Reported-by: Alexander Bulekov <alxndr@bu.edu>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1878645
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Cc: Bug 1878645 <1878645@bugs.launchpad.net>
> 
> 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.
> ---
>  cpus.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/cpus.c b/cpus.c
> index 41d1c5099f..1f6f43d221 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -2106,6 +2106,9 @@ void qemu_init_vcpu(CPUState *cpu)
>  {
>      MachineState *ms = MACHINE(qdev_get_machine());
>  
> +    if (!current_cpu) {
> +        current_cpu = cpu;
> +    }

Seems like a neat solution.
is it fair to assume that qemu_init_vcpu is called before any threads
that can do I/O are created? I confirmed that the qtest and hmp crashes
are avoided.
-Alex

>      cpu->nr_cores = ms->smp.cores;
>      cpu->nr_threads =  ms->smp.threads;
>      cpu->stopped = true;
> -- 
> 2.21.3
>