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.
