add new classifier result

1 files changed, 1074 insertions, 0 deletions

diff --git a/results/classifier/105/KVM/1878645 b/results/classifier/105/KVM/1878645
new file mode 100644
index 00000000..68ec4f69
--- /dev/null
+++ b/results/classifier/105/KVM/1878645
@@ -0,0 +1,1074 @@
+KVM: 0.832
+other: 0.823
+vnc: 0.766
+graphic: 0.692
+instruction: 0.677
+device: 0.668
+semantic: 0.650
+mistranslation: 0.650
+assembly: 0.616
+socket: 0.591
+boot: 0.579
+network: 0.564
+
+null-ptr dereference in ich9_apm_ctrl_changed
+
+Hello,
+While fuzzing, I found an input which triggers a NULL pointer dereference in
+tcg_handle_interrupt. It seems the culprint is a "cpu" pointer - maybe this bug
+is specific to QTest?
+
+==23862==ERROR: AddressSanitizer: SEGV on unknown address 0x0000000000b4 (pc 0x55b9dc7c9dce bp 0x7ffc346a0900 sp 0x7ffc346a0880 T0)
+==23862==The signal is caused by a READ memory access.
+==23862==Hint: address points to the zero page.
+    #0 0x55b9dc7c9dce in tcg_handle_interrupt /home/alxndr/Development/qemu/accel/tcg/tcg-all.c:57:21
+    #1 0x55b9dc904799 in cpu_interrupt /home/alxndr/Development/qemu/include/hw/core/cpu.h:872:5
+    #2 0x55b9dc9085e8 in ich9_apm_ctrl_changed /home/alxndr/Development/qemu/hw/isa/lpc_ich9.c:442:13
+    #3 0x55b9dd19cdc8 in apm_ioport_writeb /home/alxndr/Development/qemu/hw/isa/apm.c:50:13
+    #4 0x55b9dc73f8b4 in memory_region_write_accessor /home/alxndr/Development/qemu/memory.c:483:5
+    #5 0x55b9dc73f289 in access_with_adjusted_size /home/alxndr/Development/qemu/memory.c:544:18
+    #6 0x55b9dc73ddf5 in memory_region_dispatch_write /home/alxndr/Development/qemu/memory.c:1476:16
+    #7 0x55b9dc577bf3 in flatview_write_continue /home/alxndr/Development/qemu/exec.c:3137:23
+    #8 0x55b9dc567ad8 in flatview_write /home/alxndr/Development/qemu/exec.c:3177:14
+    #9 0x55b9dc567608 in address_space_write /home/alxndr/Development/qemu/exec.c:3268:18
+    #10 0x55b9dc723fe7 in cpu_outb /home/alxndr/Development/qemu/ioport.c:60:5
+    #11 0x55b9dc72d3c0 in qtest_process_command /home/alxndr/Development/qemu/qtest.c:392:13
+    #12 0x55b9dc72b186 in qtest_process_inbuf /home/alxndr/Development/qemu/qtest.c:710:9
+    #13 0x55b9dc72a8b3 in qtest_read /home/alxndr/Development/qemu/qtest.c:722:5
+    #14 0x55b9ddc6e60b in qemu_chr_be_write_impl /home/alxndr/Development/qemu/chardev/char.c:183:9
+    #15 0x55b9ddc6e75a in qemu_chr_be_write /home/alxndr/Development/qemu/chardev/char.c:195:9
+    #16 0x55b9ddc77979 in fd_chr_read /home/alxndr/Development/qemu/chardev/char-fd.c:68:9
+    #17 0x55b9ddcff0e9 in qio_channel_fd_source_dispatch /home/alxndr/Development/qemu/io/channel-watch.c:84:12
+    #18 0x7f7161eac897 in g_main_context_dispatch (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4e897)
+    #19 0x55b9ddebcb84 in glib_pollfds_poll /home/alxndr/Development/qemu/util/main-loop.c:219:9
+    #20 0x55b9ddebb57d in os_host_main_loop_wait /home/alxndr/Development/qemu/util/main-loop.c:242:5
+    #21 0x55b9ddebb176 in main_loop_wait /home/alxndr/Development/qemu/util/main-loop.c:518:11
+    #22 0x55b9dcb4bd1d in qemu_main_loop /home/alxndr/Development/qemu/softmmu/vl.c:1664:9
+    #23 0x55b9ddd1629c in main /home/alxndr/Development/qemu/softmmu/main.c:49:5
+    #24 0x7f7160a5ce0a in __libc_start_main /build/glibc-GwnBeO/glibc-2.30/csu/../csu/libc-start.c:308:16
+    #25 0x55b9dc49c819 in _start (/home/alxndr/Development/qemu/build/i386-softmmu/qemu-system-i386+0xc9c819)
+
+
+I can reproduce this in qemu 5.0 built with AddressSanitizer using these qtest commands:
+
+cat << EOF | ./qemu-system-i386 \
+-qtest stdio -nographic -monitor none -serial none \
+-M pc-q35-5.0
+outl 0xcf8 0x8400f841
+outl 0xcfc 0xaa215d6d
+outl 0x6d30 0x2ef8ffbe
+outb 0xb2 0x20
+EOF
+
+Please let me know if I can provide any further info.
+-Alex
+
+I don't think this is a qtest-specific error: 
+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
+
+
+
+Alexander Bulekov <email address hidden> writes:
+
+> I don't think this is a qtest-specific error: 
+> 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
+
+Both this and the qtest have the same problem of depending on
+current_cpu which is a TLS variable which will never be correct from the
+qtest or monitor context. There are only a few other cases.
+
+sun4m:cpu_halt_signal does:
+
+    if (level && current_cpu) {
+        cpu_interrupt(current_cpu, CPU_INTERRUPT_HALT);
+    }
+
+pxa2xx:pxa2xx_pwrmode_write does a bare:
+
+    /* Suspend */
+    cpu_interrupt(current_cpu, CPU_INTERRUPT_HALT);
+
+but given the context has a CPUARMState *env it could arguably use that
+to derive current_cpu but as it's only triggered by a system register
+write you can't actually trigger from a monitor/qtest command.
+
+I would suggest either:
+
+        } else if (current_cpu) {
+            cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
+        }
+
+or possibly:
+
+        } else {
+            cpu_interrupt(current_cpu ? current_cpu : first_cpu, CPU_INTERRUPT_SMI);
+        }
+
+if you really care about triggering a real IRQ from outside the CPU context.
+
+-- 
+Alex Bennée
+
+
+On 200629 2000, Alex Bennée wrote:
+> 
+> Alexander Bulekov <email address hidden> writes:
+> 
+> > I don't think this is a qtest-specific error: 
+> > 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
+> 
+> Both this and the qtest have the same problem of depending on
+> current_cpu which is a TLS variable which will never be correct from the
+> qtest or monitor context. There are only a few other cases.
+
+Ah that makes sense. It probably isn't a real issue, but I'll send
+patches with the changes you suggested below.
+Thank you
+
+> sun4m:cpu_halt_signal does:
+> 
+>     if (level && current_cpu) {
+>         cpu_interrupt(current_cpu, CPU_INTERRUPT_HALT);
+>     }
+> 
+> pxa2xx:pxa2xx_pwrmode_write does a bare:
+> 
+>     /* Suspend */
+>     cpu_interrupt(current_cpu, CPU_INTERRUPT_HALT);
+> 
+> but given the context has a CPUARMState *env it could arguably use that
+> to derive current_cpu but as it's only triggered by a system register
+> write you can't actually trigger from a monitor/qtest command.
+> 
+> I would suggest either:
+> 
+>         } else if (current_cpu) {
+>             cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
+>         }
+> 
+> or possibly:
+> 
+>         } else {
+>             cpu_interrupt(current_cpu ? current_cpu : first_cpu, CPU_INTERRUPT_SMI);
+>         }
+> 
+> if you really care about triggering a real IRQ from outside the CPU context.
+> 
+> -- 
+> Alex Bennée
+> 
+
+
+It's possible to trigger this function from qtest/monitor at which
+point current_cpu won't point at the right place. Check it and
+fall back to first_cpu if it's NULL.
+
+Signed-off-by: Alex Bennée <email address hidden>
+Cc: Bug 1878645 <email address hidden>
+---
+ hw/isa/lpc_ich9.c | 2 +-
+ 1 file changed, 1 insertion(+), 1 deletion(-)
+
+diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
+index cd6e169d47a..791c878eb0b 100644
+--- a/hw/isa/lpc_ich9.c
++++ b/hw/isa/lpc_ich9.c
+@@ -439,7 +439,7 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
+                 cpu_interrupt(cs, CPU_INTERRUPT_SMI);
+             }
+         } else {
+-            cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
++            cpu_interrupt(current_cpu ? current_cpu : first_cpu, CPU_INTERRUPT_SMI);
+         }
+     }
+ }
+-- 
+2.20.1
+
+
+
+On 7/1/20 3:56 PM, Alex Bennée wrote:
+> It's possible to trigger this function from qtest/monitor at which
+> point current_cpu won't point at the right place. Check it and
+> fall back to first_cpu if it's NULL.
+> 
+> Signed-off-by: Alex Bennée <email address hidden>
+> Cc: Bug 1878645 <email address hidden>
+> ---
+>  hw/isa/lpc_ich9.c | 2 +-
+>  1 file changed, 1 insertion(+), 1 deletion(-)
+> 
+> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
+> index cd6e169d47a..791c878eb0b 100644
+> --- a/hw/isa/lpc_ich9.c
+> +++ b/hw/isa/lpc_ich9.c
+> @@ -439,7 +439,7 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
+>                  cpu_interrupt(cs, CPU_INTERRUPT_SMI);
+>              }
+>          } else {
+> -            cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
+> +            cpu_interrupt(current_cpu ? current_cpu : first_cpu, CPU_INTERRUPT_SMI);
+
+I'm not sure this change anything, as first_cpu is NULL when using
+qtest accelerator or none-machine, see 508b4ecc39 ("gdbstub.c: fix
+GDB connection segfault caused by empty machines").
+
+>          }
+>      }
+>  }
+> 
+
+
+
+
+Philippe Mathieu-Daudé <email address hidden> writes:
+
+> On 7/1/20 3:56 PM, Alex Bennée wrote:
+>> It's possible to trigger this function from qtest/monitor at which
+>> point current_cpu won't point at the right place. Check it and
+>> fall back to first_cpu if it's NULL.
+>> 
+>> Signed-off-by: Alex Bennée <email address hidden>
+>> Cc: Bug 1878645 <email address hidden>
+>> ---
+>>  hw/isa/lpc_ich9.c | 2 +-
+>>  1 file changed, 1 insertion(+), 1 deletion(-)
+>> 
+>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
+>> index cd6e169d47a..791c878eb0b 100644
+>> --- a/hw/isa/lpc_ich9.c
+>> +++ b/hw/isa/lpc_ich9.c
+>> @@ -439,7 +439,7 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
+>>                  cpu_interrupt(cs, CPU_INTERRUPT_SMI);
+>>              }
+>>          } else {
+>> -            cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
+>> +            cpu_interrupt(current_cpu ? current_cpu : first_cpu, CPU_INTERRUPT_SMI);
+>
+> I'm not sure this change anything, as first_cpu is NULL when using
+> qtest accelerator or none-machine, see 508b4ecc39 ("gdbstub.c: fix
+> GDB connection segfault caused by empty machines").
+
+Good point - anyway feel free to ignore - it shouldn't have been in this
+series. It was just some random experimentation I was doing when looking
+at that bug.
+
+-- 
+Alex Bennée
+
+
+On 7/1/20 6:40 PM, Alex Bennée wrote:
+> 
+> Philippe Mathieu-Daudé <email address hidden> writes:
+> 
+>> On 7/1/20 3:56 PM, Alex Bennée wrote:
+>>> It's possible to trigger this function from qtest/monitor at which
+>>> point current_cpu won't point at the right place. Check it and
+>>> fall back to first_cpu if it's NULL.
+>>>
+>>> Signed-off-by: Alex Bennée <email address hidden>
+>>> Cc: Bug 1878645 <email address hidden>
+>>> ---
+>>>  hw/isa/lpc_ich9.c | 2 +-
+>>>  1 file changed, 1 insertion(+), 1 deletion(-)
+>>>
+>>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
+>>> index cd6e169d47a..791c878eb0b 100644
+>>> --- a/hw/isa/lpc_ich9.c
+>>> +++ b/hw/isa/lpc_ich9.c
+>>> @@ -439,7 +439,7 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
+>>>                  cpu_interrupt(cs, CPU_INTERRUPT_SMI);
+>>>              }
+>>>          } else {
+>>> -            cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
+>>> +            cpu_interrupt(current_cpu ? current_cpu : first_cpu, CPU_INTERRUPT_SMI);
+>>
+>> I'm not sure this change anything, as first_cpu is NULL when using
+>> qtest accelerator or none-machine, see 508b4ecc39 ("gdbstub.c: fix
+>> GDB connection segfault caused by empty machines").
+> 
+> Good point - anyway feel free to ignore - it shouldn't have been in this
+> series. It was just some random experimentation I was doing when looking
+> at that bug.
+
+See commit c781a2cc42 ("hw/i386/vmport: Allow QTest use without
+crashing") for a similar approach, but here I was thinking about
+a more generic fix, not very intrusive:
+
+-- >8 --
+diff --git a/hw/isa/apm.c b/hw/isa/apm.c
+index bce266b957..809afeb3e4 100644
+--- a/hw/isa/apm.c
++++ b/hw/isa/apm.c
+@@ -40,7 +40,7 @@ static void apm_ioport_writeb(void *opaque, hwaddr
+addr, uint64_t val,
+     if (addr == 0) {
+         apm->apmc = val;
+
+-        if (apm->callback) {
++        if (apm->callback && !qtest_enabled()) {
+             (apm->callback)(val, apm->arg);
+         }
+     } else {
+---
+
+
+
+
+Philippe Mathieu-Daudé <email address hidden> writes:
+
+> On 7/1/20 6:40 PM, Alex Bennée wrote:
+>> 
+>> Philippe Mathieu-Daudé <email address hidden> writes:
+>> 
+>>> On 7/1/20 3:56 PM, Alex Bennée wrote:
+>>>> It's possible to trigger this function from qtest/monitor at which
+>>>> point current_cpu won't point at the right place. Check it and
+>>>> fall back to first_cpu if it's NULL.
+>>>>
+>>>> Signed-off-by: Alex Bennée <email address hidden>
+>>>> Cc: Bug 1878645 <email address hidden>
+>>>> ---
+>>>>  hw/isa/lpc_ich9.c | 2 +-
+>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
+>>>>
+>>>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
+>>>> index cd6e169d47a..791c878eb0b 100644
+>>>> --- a/hw/isa/lpc_ich9.c
+>>>> +++ b/hw/isa/lpc_ich9.c
+>>>> @@ -439,7 +439,7 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
+>>>>                  cpu_interrupt(cs, CPU_INTERRUPT_SMI);
+>>>>              }
+>>>>          } else {
+>>>> -            cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
+>>>> +            cpu_interrupt(current_cpu ? current_cpu : first_cpu, CPU_INTERRUPT_SMI);
+>>>
+>>> I'm not sure this change anything, as first_cpu is NULL when using
+>>> qtest accelerator or none-machine, see 508b4ecc39 ("gdbstub.c: fix
+>>> GDB connection segfault caused by empty machines").
+>> 
+>> Good point - anyway feel free to ignore - it shouldn't have been in this
+>> series. It was just some random experimentation I was doing when looking
+>> at that bug.
+>
+> See commit c781a2cc42 ("hw/i386/vmport: Allow QTest use without
+> crashing") for a similar approach, but here I was thinking about
+> a more generic fix, not very intrusive:
+>
+> -- >8 --
+> diff --git a/hw/isa/apm.c b/hw/isa/apm.c
+> index bce266b957..809afeb3e4 100644
+> --- a/hw/isa/apm.c
+> +++ b/hw/isa/apm.c
+> @@ -40,7 +40,7 @@ static void apm_ioport_writeb(void *opaque, hwaddr
+> addr, uint64_t val,
+>      if (addr == 0) {
+>          apm->apmc = val;
+>
+> -        if (apm->callback) {
+> +        if (apm->callback && !qtest_enabled()) {
+>              (apm->callback)(val, apm->arg);
+>          }
+
+But the other failure mode reported on the bug thread was via the
+monitor - so I'm not sure just checking for qtest catches that.
+
+>      } else {
+> ---
+
+
+-- 
+Alex Bennée
+
+
++Paolo
+
+On 7/1/20 7:09 PM, Alex Bennée wrote:
+> Philippe Mathieu-Daudé <email address hidden> writes:
+>> On 7/1/20 6:40 PM, Alex Bennée wrote:
+>>> Philippe Mathieu-Daudé <email address hidden> writes:
+>>>
+>>>> On 7/1/20 3:56 PM, Alex Bennée wrote:
+>>>>> It's possible to trigger this function from qtest/monitor at which
+>>>>> point current_cpu won't point at the right place. Check it and
+>>>>> fall back to first_cpu if it's NULL.
+>>>>>
+>>>>> Signed-off-by: Alex Bennée <email address hidden>
+>>>>> Cc: Bug 1878645 <email address hidden>
+>>>>> ---
+>>>>>  hw/isa/lpc_ich9.c | 2 +-
+>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
+>>>>>
+>>>>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
+>>>>> index cd6e169d47a..791c878eb0b 100644
+>>>>> --- a/hw/isa/lpc_ich9.c
+>>>>> +++ b/hw/isa/lpc_ich9.c
+>>>>> @@ -439,7 +439,7 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
+>>>>>                  cpu_interrupt(cs, CPU_INTERRUPT_SMI);
+>>>>>              }
+>>>>>          } else {
+>>>>> -            cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
+>>>>> +            cpu_interrupt(current_cpu ? current_cpu : first_cpu, CPU_INTERRUPT_SMI);
+>>>>
+>>>> I'm not sure this change anything, as first_cpu is NULL when using
+>>>> qtest accelerator or none-machine, see 508b4ecc39 ("gdbstub.c: fix
+>>>> GDB connection segfault caused by empty machines").
+>>>
+>>> Good point - anyway feel free to ignore - it shouldn't have been in this
+>>> series. It was just some random experimentation I was doing when looking
+>>> at that bug.
+>>
+>> See commit c781a2cc42 ("hw/i386/vmport: Allow QTest use without
+>> crashing") for a similar approach, but here I was thinking about
+>> a more generic fix, not very intrusive:
+>>
+>> -- >8 --
+>> diff --git a/hw/isa/apm.c b/hw/isa/apm.c
+>> index bce266b957..809afeb3e4 100644
+>> --- a/hw/isa/apm.c
+>> +++ b/hw/isa/apm.c
+>> @@ -40,7 +40,7 @@ static void apm_ioport_writeb(void *opaque, hwaddr
+>> addr, uint64_t val,
+>>      if (addr == 0) {
+>>          apm->apmc = val;
+>>
+>> -        if (apm->callback) {
+>> +        if (apm->callback && !qtest_enabled()) {
+>>              (apm->callback)(val, apm->arg);
+>>          }
+> 
+> But the other failure mode reported on the bug thread was via the
+> monitor - so I'm not sure just checking for qtest catches that.
+
+Ah indeed.
+
+in exec.c:
+
+/* current CPU in the current thread. It is only valid inside
+   cpu_exec() */
+__thread CPUState *current_cpu;
+
+Maybe we shouldn't use current_cpu out of exec.c...
+
+
+
+On 7/1/20 7:34 PM, Philippe Mathieu-Daudé wrote:
+> +Paolo
+> 
+> On 7/1/20 7:09 PM, Alex Bennée wrote:
+>> Philippe Mathieu-Daudé <email address hidden> writes:
+>>> On 7/1/20 6:40 PM, Alex Bennée wrote:
+>>>> Philippe Mathieu-Daudé <email address hidden> writes:
+>>>>
+>>>>> On 7/1/20 3:56 PM, Alex Bennée wrote:
+>>>>>> It's possible to trigger this function from qtest/monitor at which
+>>>>>> point current_cpu won't point at the right place. Check it and
+>>>>>> fall back to first_cpu if it's NULL.
+>>>>>>
+>>>>>> Signed-off-by: Alex Bennée <email address hidden>
+>>>>>> Cc: Bug 1878645 <email address hidden>
+>>>>>> ---
+>>>>>>  hw/isa/lpc_ich9.c | 2 +-
+>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
+>>>>>>
+>>>>>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
+>>>>>> index cd6e169d47a..791c878eb0b 100644
+>>>>>> --- a/hw/isa/lpc_ich9.c
+>>>>>> +++ b/hw/isa/lpc_ich9.c
+>>>>>> @@ -439,7 +439,7 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
+>>>>>>                  cpu_interrupt(cs, CPU_INTERRUPT_SMI);
+>>>>>>              }
+>>>>>>          } else {
+>>>>>> -            cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
+>>>>>> +            cpu_interrupt(current_cpu ? current_cpu : first_cpu, CPU_INTERRUPT_SMI);
+>>>>>
+>>>>> I'm not sure this change anything, as first_cpu is NULL when using
+>>>>> qtest accelerator or none-machine, see 508b4ecc39 ("gdbstub.c: fix
+>>>>> GDB connection segfault caused by empty machines").
+>>>>
+>>>> Good point - anyway feel free to ignore - it shouldn't have been in this
+>>>> series. It was just some random experimentation I was doing when looking
+>>>> at that bug.
+>>>
+>>> See commit c781a2cc42 ("hw/i386/vmport: Allow QTest use without
+>>> crashing") for a similar approach, but here I was thinking about
+>>> a more generic fix, not very intrusive:
+>>>
+>>> -- >8 --
+>>> diff --git a/hw/isa/apm.c b/hw/isa/apm.c
+>>> index bce266b957..809afeb3e4 100644
+>>> --- a/hw/isa/apm.c
+>>> +++ b/hw/isa/apm.c
+>>> @@ -40,7 +40,7 @@ static void apm_ioport_writeb(void *opaque, hwaddr
+>>> addr, uint64_t val,
+>>>      if (addr == 0) {
+>>>          apm->apmc = val;
+>>>
+>>> -        if (apm->callback) {
+>>> +        if (apm->callback && !qtest_enabled()) {
+>>>              (apm->callback)(val, apm->arg);
+>>>          }
+>>
+>> But the other failure mode reported on the bug thread was via the
+>> monitor - so I'm not sure just checking for qtest catches that.
+> 
+> Ah indeed.
+> 
+> in exec.c:
+> 
+> /* current CPU in the current thread. It is only valid inside
+>    cpu_exec() */
+> __thread CPUState *current_cpu;
+> 
+> Maybe we shouldn't use current_cpu out of exec.c...
+
+I meant, out of cpu_exec(), a cpu thread. Here we access it
+from an I/O thread.
+
+
+
++MST/Igor for ICH9
+
+On 7/1/20 7:37 PM, Philippe Mathieu-Daudé wrote:
+> On 7/1/20 7:34 PM, Philippe Mathieu-Daudé wrote:
+>> +Paolo
+>>
+>> On 7/1/20 7:09 PM, Alex Bennée wrote:
+>>> Philippe Mathieu-Daudé <email address hidden> writes:
+>>>> On 7/1/20 6:40 PM, Alex Bennée wrote:
+>>>>> Philippe Mathieu-Daudé <email address hidden> writes:
+>>>>>
+>>>>>> On 7/1/20 3:56 PM, Alex Bennée wrote:
+>>>>>>> It's possible to trigger this function from qtest/monitor at which
+>>>>>>> point current_cpu won't point at the right place. Check it and
+>>>>>>> fall back to first_cpu if it's NULL.
+>>>>>>>
+>>>>>>> Signed-off-by: Alex Bennée <email address hidden>
+>>>>>>> Cc: Bug 1878645 <email address hidden>
+>>>>>>> ---
+>>>>>>>  hw/isa/lpc_ich9.c | 2 +-
+>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
+>>>>>>>
+>>>>>>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
+>>>>>>> index cd6e169d47a..791c878eb0b 100644
+>>>>>>> --- a/hw/isa/lpc_ich9.c
+>>>>>>> +++ b/hw/isa/lpc_ich9.c
+>>>>>>> @@ -439,7 +439,7 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
+>>>>>>>                  cpu_interrupt(cs, CPU_INTERRUPT_SMI);
+>>>>>>>              }
+>>>>>>>          } else {
+>>>>>>> -            cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
+>>>>>>> +            cpu_interrupt(current_cpu ? current_cpu : first_cpu, CPU_INTERRUPT_SMI);
+>>>>>>
+>>>>>> I'm not sure this change anything, as first_cpu is NULL when using
+>>>>>> qtest accelerator or none-machine, see 508b4ecc39 ("gdbstub.c: fix
+>>>>>> GDB connection segfault caused by empty machines").
+>>>>>
+>>>>> Good point - anyway feel free to ignore - it shouldn't have been in this
+>>>>> series. It was just some random experimentation I was doing when looking
+>>>>> at that bug.
+>>>>
+>>>> See commit c781a2cc42 ("hw/i386/vmport: Allow QTest use without
+>>>> crashing") for a similar approach, but here I was thinking about
+>>>> a more generic fix, not very intrusive:
+>>>>
+>>>> -- >8 --
+>>>> diff --git a/hw/isa/apm.c b/hw/isa/apm.c
+>>>> index bce266b957..809afeb3e4 100644
+>>>> --- a/hw/isa/apm.c
+>>>> +++ b/hw/isa/apm.c
+>>>> @@ -40,7 +40,7 @@ static void apm_ioport_writeb(void *opaque, hwaddr
+>>>> addr, uint64_t val,
+>>>>      if (addr == 0) {
+>>>>          apm->apmc = val;
+>>>>
+>>>> -        if (apm->callback) {
+>>>> +        if (apm->callback && !qtest_enabled()) {
+>>>>              (apm->callback)(val, apm->arg);
+>>>>          }
+>>>
+>>> But the other failure mode reported on the bug thread was via the
+>>> monitor - so I'm not sure just checking for qtest catches that.
+>>
+>> Ah indeed.
+>>
+>> in exec.c:
+>>
+>> /* current CPU in the current thread. It is only valid inside
+>>    cpu_exec() */
+>> __thread CPUState *current_cpu;
+>>
+>> Maybe we shouldn't use current_cpu out of exec.c...
+> 
+> I meant, out of cpu_exec(), a cpu thread. Here we access it
+> from an I/O thread.
+
+ARM and S390X use:
+
+hw/arm/boot.c:460:    ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(0));
+hw/arm/virt.c:331:    armcpu = ARM_CPU(qemu_get_cpu(0));
+hw/arm/virt.c:549:    armcpu = ARM_CPU(qemu_get_cpu(0));
+hw/cpu/a15mpcore.c:69:        cpuobj = OBJECT(qemu_get_cpu(0));
+hw/cpu/a9mpcore.c:76:    cpuobj = OBJECT(qemu_get_cpu(0));
+target/s390x/cpu_models.c:155:        cpu = S390_CPU(qemu_get_cpu(0));
+target/s390x/cpu_models.c:169:        cpu = S390_CPU(qemu_get_cpu(0));
+target/s390x/cpu_models.c:184:        cpu = S390_CPU(qemu_get_cpu(0));
+target/s390x/cpu_models.c:204:        cpu = S390_CPU(qemu_get_cpu(0));
+target/s390x/cpu_models.c:218:        cpu = S390_CPU(qemu_get_cpu(0));
+
+It seems odd that the ICH9 delivers the SMI on a random core.
+Usually the IRQ lines are wired to a particular unit.
+
+
+
+On 7/1/20 7:48 PM, Philippe Mathieu-Daudé wrote:
+> +MST/Igor for ICH9
+> 
+> On 7/1/20 7:37 PM, Philippe Mathieu-Daudé wrote:
+>> On 7/1/20 7:34 PM, Philippe Mathieu-Daudé wrote:
+>>> +Paolo
+>>>
+>>> On 7/1/20 7:09 PM, Alex Bennée wrote:
+>>>> Philippe Mathieu-Daudé <email address hidden> writes:
+>>>>> On 7/1/20 6:40 PM, Alex Bennée wrote:
+>>>>>> Philippe Mathieu-Daudé <email address hidden> writes:
+>>>>>>
+>>>>>>> On 7/1/20 3:56 PM, Alex Bennée wrote:
+>>>>>>>> It's possible to trigger this function from qtest/monitor at which
+>>>>>>>> point current_cpu won't point at the right place. Check it and
+>>>>>>>> fall back to first_cpu if it's NULL.
+>>>>>>>>
+>>>>>>>> Signed-off-by: Alex Bennée <email address hidden>
+>>>>>>>> Cc: Bug 1878645 <email address hidden>
+>>>>>>>> ---
+>>>>>>>>  hw/isa/lpc_ich9.c | 2 +-
+>>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
+>>>>>>>>
+>>>>>>>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
+>>>>>>>> index cd6e169d47a..791c878eb0b 100644
+>>>>>>>> --- a/hw/isa/lpc_ich9.c
+>>>>>>>> +++ b/hw/isa/lpc_ich9.c
+>>>>>>>> @@ -439,7 +439,7 @@ static void ich9_apm_ctrl_changed(uint32_t val, void *arg)
+>>>>>>>>                  cpu_interrupt(cs, CPU_INTERRUPT_SMI);
+>>>>>>>>              }
+>>>>>>>>          } else {
+>>>>>>>> -            cpu_interrupt(current_cpu, CPU_INTERRUPT_SMI);
+>>>>>>>> +            cpu_interrupt(current_cpu ? current_cpu : first_cpu, CPU_INTERRUPT_SMI);
+>>>>>>>
+>>>>>>> I'm not sure this change anything, as first_cpu is NULL when using
+>>>>>>> qtest accelerator or none-machine, see 508b4ecc39 ("gdbstub.c: fix
+>>>>>>> GDB connection segfault caused by empty machines").
+>>>>>>
+>>>>>> Good point - anyway feel free to ignore - it shouldn't have been in this
+>>>>>> series. It was just some random experimentation I was doing when looking
+>>>>>> at that bug.
+>>>>>
+>>>>> See commit c781a2cc42 ("hw/i386/vmport: Allow QTest use without
+>>>>> crashing") for a similar approach, but here I was thinking about
+>>>>> a more generic fix, not very intrusive:
+>>>>>
+>>>>> -- >8 --
+>>>>> diff --git a/hw/isa/apm.c b/hw/isa/apm.c
+>>>>> index bce266b957..809afeb3e4 100644
+>>>>> --- a/hw/isa/apm.c
+>>>>> +++ b/hw/isa/apm.c
+>>>>> @@ -40,7 +40,7 @@ static void apm_ioport_writeb(void *opaque, hwaddr
+>>>>> addr, uint64_t val,
+>>>>>      if (addr == 0) {
+>>>>>          apm->apmc = val;
+>>>>>
+>>>>> -        if (apm->callback) {
+>>>>> +        if (apm->callback && !qtest_enabled()) {
+>>>>>              (apm->callback)(val, apm->arg);
+>>>>>          }
+>>>>
+>>>> But the other failure mode reported on the bug thread was via the
+>>>> monitor - so I'm not sure just checking for qtest catches that.
+>>>
+>>> Ah indeed.
+>>>
+>>> in exec.c:
+>>>
+>>> /* current CPU in the current thread. It is only valid inside
+>>>    cpu_exec() */
+>>> __thread CPUState *current_cpu;
+>>>
+>>> Maybe we shouldn't use current_cpu out of exec.c...
+>>
+>> I meant, out of cpu_exec(), a cpu thread. Here we access it
+>> from an I/O thread.
+
+Ah! we are in the monitor thread... It makes sense there is not
+current_cpu assigned :)
+
+> 
+> ARM and S390X use:
+> 
+> hw/arm/boot.c:460:    ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(0));
+> hw/arm/virt.c:331:    armcpu = ARM_CPU(qemu_get_cpu(0));
+> hw/arm/virt.c:549:    armcpu = ARM_CPU(qemu_get_cpu(0));
+> hw/cpu/a15mpcore.c:69:        cpuobj = OBJECT(qemu_get_cpu(0));
+> hw/cpu/a9mpcore.c:76:    cpuobj = OBJECT(qemu_get_cpu(0));
+> target/s390x/cpu_models.c:155:        cpu = S390_CPU(qemu_get_cpu(0));
+> target/s390x/cpu_models.c:169:        cpu = S390_CPU(qemu_get_cpu(0));
+> target/s390x/cpu_models.c:184:        cpu = S390_CPU(qemu_get_cpu(0));
+> target/s390x/cpu_models.c:204:        cpu = S390_CPU(qemu_get_cpu(0));
+> target/s390x/cpu_models.c:218:        cpu = S390_CPU(qemu_get_cpu(0));
+> 
+> It seems odd that the ICH9 delivers the SMI on a random core.
+> Usually the IRQ lines are wired to a particular unit.
+> 
+
+
+
+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 <email address hidden>
+Buglink: https://bugs.launchpad.net/qemu/+bug/1878645
+Signed-off-by: Philippe Mathieu-Daudé <email address hidden>
+---
+Cc: Bug 1878645 <email address hidden>
+
+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;
++    }
+     cpu->nr_cores = ms->smp.cores;
+     cpu->nr_threads =  ms->smp.threads;
+     cpu->stopped = true;
+-- 
+2.21.3
+
+
+
+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 <email address hidden>
+> Buglink: https://bugs.launchpad.net/qemu/+bug/1878645
+> Signed-off-by: Philippe Mathieu-Daudé <email address hidden>
+> ---
+> Cc: Bug 1878645 <email address hidden>
+> 
+> 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
+> 
+
+
+On Wed, 1 Jul 2020 at 19:21, Philippe Mathieu-Daudé <email address hidden> 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
+
+
+On 7/1/20 10:35 PM, Peter Maydell wrote:
+> On Wed, 1 Jul 2020 at 19:21, Philippe Mathieu-Daudé <email address hidden> 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
+> 
+
+
+
+
+Paolo Bonzini <email address hidden> writes:
+
+> On 01/07/20 22:35, Peter Maydell wrote:
+>> 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".
+>
+> Yeah, the fix is probably to add a functions that returns either
+> current_cpu or the monitor CPU, and use it in device emulation if
+> applicable.
+>
+> The problem with current_cpu is that it affects stuff like run_on_cpu,
+> and that is guaranteed to cause havoc to code that expects to run on a
+> given CPU and therefore doesn't use locks.
+
+IIRC the original reported bug was in a APM callback which was triggered
+by an MMIO operation. Currently we don't expose the current cpu via the
+memory dispatch routines. Should we to ensure there is always something
+valid there?
+
+>
+> Paolo
+
+
+-- 
+Alex Bennée
+
+
+I moved this report over to QEMU's new bug tracker on gitlab.com.
+Please continue with the discussion here:
+
+https://gitlab.com/qemu-project/qemu/-/issues/536
+
+Thanks for moving it over! ... let's close this one here on Launchpad now.
+
+