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

[Philippe Mathieu-Daudé]

+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é <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...
> 
> 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.