[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 01/40] hw/isa: check for current_cpu before generating IRQ
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH v4 01/40] hw/isa: check for current_cpu before generating IRQ |
Date: |
Wed, 1 Jul 2020 20:13:09 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 |
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é <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.
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.
>
- [PATCH v4 03/40] crypto/linux_keyring: fix 'secret_keyring' configure test, (continued)
- [PATCH v4 03/40] crypto/linux_keyring: fix 'secret_keyring' configure test, Alex Bennée, 2020/07/01
- [PATCH v4 02/40] iotests: Fix 051 output after qdev_init_nofail() removal, Alex Bennée, 2020/07/01
- [PATCH v4 01/40] hw/isa: check for current_cpu before generating IRQ, Alex Bennée, 2020/07/01
- Re: [PATCH v4 01/40] hw/isa: check for current_cpu before generating IRQ, Philippe Mathieu-Daudé, 2020/07/01
- Re: [PATCH v4 01/40] hw/isa: check for current_cpu before generating IRQ, Alex Bennée, 2020/07/01
- Re: [PATCH v4 01/40] hw/isa: check for current_cpu before generating IRQ, Philippe Mathieu-Daudé, 2020/07/01
- Re: [PATCH v4 01/40] hw/isa: check for current_cpu before generating IRQ, Alex Bennée, 2020/07/01
- Re: [PATCH v4 01/40] hw/isa: check for current_cpu before generating IRQ, Philippe Mathieu-Daudé, 2020/07/01
- Re: [PATCH v4 01/40] hw/isa: check for current_cpu before generating IRQ, Philippe Mathieu-Daudé, 2020/07/01
- Re: [PATCH v4 01/40] hw/isa: check for current_cpu before generating IRQ, Philippe Mathieu-Daudé, 2020/07/01
- Re: [PATCH v4 01/40] hw/isa: check for current_cpu before generating IRQ,
Philippe Mathieu-Daudé <=
[PATCH v4 04/40] util/coroutine: Cleanup start_switch_fiber_ for TSAN., Alex Bennée, 2020/07/01
[PATCH v4 12/40] python/qemu: Add ConsoleSocket for optional use in QEMUMachine, Alex Bennée, 2020/07/01
[PATCH v4 10/40] tests/vm: Added a new script for centos.aarch64., Alex Bennée, 2020/07/01
[PATCH v4 11/40] tests/vm: change scripts to use self._config, Alex Bennée, 2020/07/01
[PATCH v4 07/40] tests/vm: Added configuration file support, Alex Bennée, 2020/07/01
[PATCH v4 09/40] tests/vm: Added a new script for ubuntu.aarch64., Alex Bennée, 2020/07/01
[PATCH v4 08/40] tests/vm: Add common Ubuntu python module, Alex Bennée, 2020/07/01
[PATCH v4 05/40] tests/vm: pass args through to BaseVM's __init__, Alex Bennée, 2020/07/01
[PATCH v4 25/40] tests/docker: add packages needed for check-acceptance, Alex Bennée, 2020/07/01