Re: [PATCH v1 01/12] KVM: PPC: Book3S HV P9: Restore host CTRL SPR after guest exit

[Fabiano Rosas <farosas@linux.ibm.com>]

Nicholas Piggin <npiggin@gmail.com> writes:

> The host CTRL (runlatch) value is not restored after guest exit. The
> host CTRL should always be 1 except in CPU idle code, so this can result
> in the host running with runlatch clear, and potentially switching to
> a different vCPU which then runs with runlatch clear as well.
>
> This has little effect on P9 machines, CTRL is only responsible for some
> PMU counter logic in the host and so other than corner cases of software
> relying on that, or explicitly reading the runlatch value (Linux does
> not appear to be affected but it's possible non-Linux guests could be),
> there should be no execution correctness problem, though it could be
> used as a covert channel between guests.
>
> There may be microcontrollers, firmware or monitoring tools that sample
> the runlatch value out-of-band, however since the register is writable
> by guests, these values would (should) not be relied upon for correct
> operation of the host, so suboptimal performance or incorrect reporting
> should be the worst problem.
>
> Fixes: 95a6432ce9038 ("KVM: PPC: Book3S HV: Streamlined guest entry/exit path on P9 for radix guests")
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  arch/powerpc/kvm/book3s_hv.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 13bad6bf4c95..208a053c9adf 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -3728,7 +3728,10 @@ static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
>  	vcpu->arch.dec_expires = dec + tb;
>  	vcpu->cpu = -1;
>  	vcpu->arch.thread_cpu = -1;
> +	/* Save guest CTRL register, set runlatch to 1 */
>  	vcpu->arch.ctrl = mfspr(SPRN_CTRLF);
> +	if (!(vcpu->arch.ctrl & 1))
> +		mtspr(SPRN_CTRLT, vcpu->arch.ctrl | 1);

Maybe ditch the comment and use the already defined CTRL_RUNLATCH?

>
>  	vcpu->arch.iamr = mfspr(SPRN_IAMR);
>  	vcpu->arch.pspb = mfspr(SPRN_PSPB);