Re: [PATCH v4 01/40] hw/isa: check for current_cpu before generating IRQ

[Philippe Mathieu-Daudé]

+Paolo

On 7/1/20 7:09 PM, Alex Bennée wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>> On 7/1/20 6:40 PM, Alex Bennée wrote:
>>> Philippe Mathieu-Daudé <philmd@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.bennee@linaro.org>
>>>>> Cc: Bug 1878645 <1878645@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...