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é <phi...@redhat.com> writes: >>>>> On 7/1/20 6:40 PM, Alex Bennée wrote: >>>>>> Philippe Mathieu-Daudé <phi...@redhat.com> 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 <alex.ben...@linaro.org> >>>>>>>> Cc: Bug 1878645 <1878...@bugs.launchpad.net> >>>>>>>> --- >>>>>>>> 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. >