Re: [PATCH v3 3/8] ppc/spapr: Use start-powered-off CPUState property

[Thiago Jung Bauermann]

Greg Kurz <groug@kaod.org> writes:

> On Wed, 22 Jul 2020 23:56:52 -0300
> Thiago Jung Bauermann <bauerman@linux.ibm.com> wrote:
>
>> PowerPC sPAPR CPUs start in the halted state, and spapr_reset_vcpu()
>> attempts to implement this by setting CPUState::halted to 1. But that's too
>> late for the case of hotplugged CPUs in a machine configure with 2 or more
>> threads per core.
>> 
>> By then, other parts of QEMU have already caused the vCPU to run in an
>> unitialized state a couple of times. For example, ppc_cpu_reset() calls
>> ppc_tlb_invalidate_all(), which ends up calling async_run_on_cpu(). This
>> kicks the new vCPU while it has CPUState::halted = 0, causing QEMU to issue
>> a KVM_RUN ioctl on the new vCPU before the guest is able to make the
>> start-cpu RTAS call to initialize its register state.
>> 
>> This problem doesn't seem to cause visible issues for regular guests, but
>> on a secure guest running under the Ultravisor it does. The Ultravisor
>> relies on being able to snoop on the start-cpu RTAS call to map vCPUs to
>> guests, and this issue causes it to see a stray vCPU that doesn't belong to
>> any guest.
>> 
>> Fix by setting the start-powered-off CPUState property in
>> spapr_create_vcpu(), which makes cpu_common_reset() initialize
>> CPUState::halted to 1 at an earlier moment.
>> 
>> Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
>> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
>> ---
>
> Thanks for doing this ! I remember seeing partly initialized CPUs being
> kicked in the past, which is clearly wrong but I never got time to find
> a proper fix (especially since it didn't seem to break anything).
>
> Reviewed-by: Greg Kurz <groug@kaod.org>

Thanks for confirming the issue, and for reviewing the code.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center