diff mbox series

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

Message ID 838d4d01-cd9e-d74a-5cd2-b23644172c9f@redhat.com (mailing list archive)
State New, archived
Headers show
Series [Bug,1878645] Re: [PATCH v4 01/40] hw/isa: check for current_cpu before generating IRQ | expand

Commit Message

Philippe Mathieu-Daudé July 1, 2020, 4:47 p.m. UTC 
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 --
     } else {
---

Comments

Alex Bennée July 1, 2020, 5:09 p.m. UTC | #1 
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.

>      } else {
> ---
Philippe Mathieu-Daudé July 1, 2020, 5:34 p.m. UTC | #2 
+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...
Philippe Mathieu-Daudé July 1, 2020, 5:37 p.m. UTC | #3 
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.
Philippe Mathieu-Daudé July 1, 2020, 5:48 p.m. UTC | #4 
+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.
Philippe Mathieu-Daudé July 1, 2020, 6:13 p.m. UTC | #5 
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.
>
diff mbox series

Patch

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);
         }