[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] cpu: Add starts_halted() method
From: |
Alex Bennée |
Subject: |
Re: [PATCH] cpu: Add starts_halted() method |
Date: |
Sat, 11 Jul 2020 18:55:42 +0100 |
User-agent: |
mu4e 1.5.4; emacs 28.0.50 |
Thiago Jung Bauermann <bauerman@linux.ibm.com> writes:
> Alex Bennée <alex.bennee@linaro.org> writes:
>
>> Thiago Jung Bauermann <bauerman@linux.ibm.com> writes:
>>
>>> Eduardo Habkost <ehabkost@redhat.com> writes:
>>>
>>>> On Wed, Jul 08, 2020 at 09:11:55PM +0100, Peter Maydell wrote:
>>>>> On Wed, 8 Jul 2020 at 18:36, Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>>> >
>>>>> > On Wed, Jul 08, 2020 at 06:09:49PM +0100, Peter Maydell wrote:
>>>>> > > Exactly. It appears that there's a bug in our mechanisms,
>>>>> > > which is why I'm suggesting that the right thing is
>>>>> > > to fix that bug rather than marking the CPU as halted
>>>>> > > earlier in the reset process so that the KVM_RUN happens
>>>>> > > to do nothing...
>>>>> >
>>>>> > I agree this is necessary, but it doesn't seem sufficient.
>>>>> >
>>>>> > Having cpu_reset() set halted=0 on spapr (and probably other
>>>>> > machines) is also a bug, as it could still trigger unwanted
>>>>> > KVM_RUN when cpu_reset() returns (and before machine code sets
>>>>> > halted=1).
>>>>>
>>>>> The Arm handling of starting-halted sets halted=1 within cpu_reset,
>>>>> based on whether the CPU object was created with a
>>>>> "start-powered-off" property.
>>>>
>>>> Making this mechanism generic sounds like a good idea.
>>>
>>> I'll take a stab at doing that and using it for the spapr machine.
>>>
>>>>> I'm not sure in practice that anything can get in asynchronously
>>>>> and cause a KVM_RUN in between spapr_reset_vcpu() calling
>>>>> cpu_reset() and it setting cs->halted (and the other stuff),
>>>>> though. This function ought to be called with the iothread
>>>>> lock held, so KVM_RUN will only happen if it calls some
>>>>> other function which incorrectly lets the CPU run.
>>>>
>>>> Yeah, maybe it won't happen in practice. It just seems fragile.
>>>> The same way ppc_cpu_reset() kicked the CPU by accident, code
>>>> outside cpu_reset() might one day kick the CPU by accident before
>>>> setting halted=1.
>>>
>>> I'm seeing the vcpu being KVM_RUN'd too early twice during hotplug.
>>> Both of them are before cpu_reset() and ppc_cpu_reset().
>>>
>>> Here's the backtrace for the first of them (redacted for clarity):
>>>
>>> #0 in cpu_resume ()
>>> #1 in cpu_common_realizefn ()
>>> #2 in ppc_cpu_realize ()
>>> #3 in device_set_realized ()
>>> #4 in property_set_bool ()
>>> #5 in object_property_set ()
>>> #6 in object_property_set_qobject ()
>>> #7 in object_property_set_bool ()
>>> #8 in qdev_realize ()
>> <snip>
>>> #18 in qmp_device_add ()
>>
>> Is this a hotplug event?
>
> Yes, the way I reproduce the problem is starting a pseries guest with
> `-smp 2,maxcpus=32,sockets=1,cores=16,threads=2` and then use qmp-shell to
> send the command:
>
> device_add id=device-2 driver=host-spapr-cpu-core core-id=2 node-id=0
>
>>> Here's the second:
>>>
>>> #0 in qemu_cpu_kick_thread ()
>>> #1 in qemu_cpu_kick ()
>>> #2 in queue_work_on_cpu ()
>>> #3 in async_run_on_cpu ()
>>> #4 in tlb_flush_by_mmuidx ()
>>> #5 in tlb_flush ()
>>> #6 in ppc_tlb_invalidate_all ()
>>
>> FWIW tcg_flush_softmmu_tlb handles a tlb_flush in the common reset code.
>
> Ok, maybe KVM should be doing that too? Or maybe it does but pseries
> isn't relying on it. I'll dig further.
No tlb flush is a softmmu only thing.
--
Alex Bennée
- Re: [PATCH] cpu: Add starts_halted() method, (continued)
- Re: [PATCH] cpu: Add starts_halted() method, Peter Maydell, 2020/07/08
- Re: [PATCH] cpu: Add starts_halted() method, Eduardo Habkost, 2020/07/08
- Re: [PATCH] cpu: Add starts_halted() method, Peter Maydell, 2020/07/08
- Re: [PATCH] cpu: Add starts_halted() method, Eduardo Habkost, 2020/07/08
- Re: [PATCH] cpu: Add starts_halted() method, Thiago Jung Bauermann, 2020/07/08
- Re: [PATCH] cpu: Add starts_halted() method, Thiago Jung Bauermann, 2020/07/08
- Re: [PATCH] cpu: Add starts_halted() method, Philippe Mathieu-Daudé, 2020/07/09
- Re: [PATCH] cpu: Add starts_halted() method, Thiago Jung Bauermann, 2020/07/10
- Re: [PATCH] cpu: Add starts_halted() method, Eduardo Habkost, 2020/07/10
- Message not available
- Re: [PATCH] cpu: Add starts_halted() method, Thiago Jung Bauermann, 2020/07/10
- Re: [PATCH] cpu: Add starts_halted() method,
Alex Bennée <=
- Re: [PATCH] cpu: Add starts_halted() method, Philippe Mathieu-Daudé, 2020/07/08
- Re: [PATCH] cpu: Add starts_halted() method, Eduardo Habkost, 2020/07/08
- Re: [PATCH] cpu: Add starts_halted() method, Philippe Mathieu-Daudé, 2020/07/09
- Re: [PATCH] cpu: Add starts_halted() method, Greg Kurz, 2020/07/09
- Re: [PATCH] cpu: Add starts_halted() method, Philippe Mathieu-Daudé, 2020/07/09
- Re: [PATCH] cpu: Add starts_halted() method, Greg Kurz, 2020/07/09
- Re: [PATCH] cpu: Add starts_halted() method, Philippe Mathieu-Daudé, 2020/07/09
- Re: [PATCH] cpu: Add starts_halted() method, Greg Kurz, 2020/07/09
- Re: [PATCH] cpu: Add starts_halted() method, Philippe Mathieu-Daudé, 2020/07/09
- Re: [PATCH] cpu: Add starts_halted() method, Peter Maydell, 2020/07/09