Re: [PATCH v3 3/8] ppc/spapr: Use start-powered-off CPUState property
Greg Kurz writes: > On Wed, 22 Jul 2020 23:56:52 -0300 > Thiago Jung Bauermann 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 >> Signed-off-by: Thiago Jung Bauermann >> --- > > 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 Thanks for confirming the issue, and for reviewing the code. -- Thiago Jung Bauermann IBM Linux Technology Center
Re: [PATCH v3 3/8] ppc/spapr: Use start-powered-off CPUState property
On 7/23/20 4:56 AM, Thiago Jung Bauermann 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
> Signed-off-by: Thiago Jung Bauermann
> ---
> hw/ppc/spapr_cpu_core.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> NB: Tested on ppc64le pseries KVM guest with two threads per core.
> Hot-plugging additional cores doesn't cause the bug described above
> anymore.
>
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index c4f47dcc04..2125fdac34 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -36,11 +36,6 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
>
> cpu_reset(cs);
>
> -/* All CPUs start halted. CPU0 is unhalted from the machine level
> - * reset code and the rest are explicitly started up by the guest
> - * using an RTAS call */
> -cs->halted = 1;
> -
> env->spr[SPR_HIOR] = 0;
>
> lpcr = env->spr[SPR_LPCR];
> @@ -274,6 +269,11 @@ static PowerPCCPU *spapr_create_vcpu(SpaprCpuCore *sc,
> int i, Error **errp)
>
> cs = CPU(obj);
> cpu = POWERPC_CPU(obj);
> +/*
> + * All CPUs start halted. CPU0 is unhalted from the machine level reset
> code
> + * and the rest are explicitly started up by the guest using an RTAS
> call.
> + */
> +cs->start_powered_off = true;
> cs->cpu_index = cc->core_id + i;
> spapr_set_vcpu_id(cpu, cs->cpu_index, _err);
> if (local_err) {
>
Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH v3 3/8] ppc/spapr: Use start-powered-off CPUState property
On Wed, 22 Jul 2020 23:56:52 -0300
Thiago Jung Bauermann 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
> Signed-off-by: Thiago Jung Bauermann
> ---
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
> hw/ppc/spapr_cpu_core.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> NB: Tested on ppc64le pseries KVM guest with two threads per core.
> Hot-plugging additional cores doesn't cause the bug described above
> anymore.
>
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index c4f47dcc04..2125fdac34 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -36,11 +36,6 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
>
> cpu_reset(cs);
>
> -/* All CPUs start halted. CPU0 is unhalted from the machine level
> - * reset code and the rest are explicitly started up by the guest
> - * using an RTAS call */
> -cs->halted = 1;
> -
> env->spr[SPR_HIOR] = 0;
>
> lpcr = env->spr[SPR_LPCR];
> @@ -274,6 +269,11 @@ static PowerPCCPU *spapr_create_vcpu(SpaprCpuCore *sc,
> int i, Error **errp)
>
> cs = CPU(obj);
> cpu = POWERPC_CPU(obj);
> +/*
> + * All CPUs start halted. CPU0 is unhalted from the machine level reset
> code
> + * and the rest are explicitly started up by the guest using an RTAS
> call.
> + */
> +cs->start_powered_off = true;
> cs->cpu_index = cc->core_id + i;
> spapr_set_vcpu_id(cpu, cs->cpu_index, _err);
> if (local_err) {
Re: [PATCH v3 3/8] ppc/spapr: Use start-powered-off CPUState property
On Wed, Jul 22, 2020 at 11:56:52PM -0300, Thiago Jung Bauermann wrote:
65;6003;1c> 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
> Signed-off-by: Thiago Jung Bauermann
Acked-by: David Gibson
> ---
> hw/ppc/spapr_cpu_core.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> NB: Tested on ppc64le pseries KVM guest with two threads per core.
> Hot-plugging additional cores doesn't cause the bug described above
> anymore.
>
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index c4f47dcc04..2125fdac34 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -36,11 +36,6 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
>
> cpu_reset(cs);
>
> -/* All CPUs start halted. CPU0 is unhalted from the machine level
> - * reset code and the rest are explicitly started up by the guest
> - * using an RTAS call */
> -cs->halted = 1;
> -
> env->spr[SPR_HIOR] = 0;
>
> lpcr = env->spr[SPR_LPCR];
> @@ -274,6 +269,11 @@ static PowerPCCPU *spapr_create_vcpu(SpaprCpuCore *sc,
> int i, Error **errp)
>
> cs = CPU(obj);
> cpu = POWERPC_CPU(obj);
> +/*
> + * All CPUs start halted. CPU0 is unhalted from the machine level reset
> code
> + * and the rest are explicitly started up by the guest using an RTAS
> call.
> + */
> +cs->start_powered_off = true;
> cs->cpu_index = cc->core_id + i;
> spapr_set_vcpu_id(cpu, cs->cpu_index, _err);
> if (local_err) {
>
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
