Re: [PATCH] powerpc/kvm: Allow qemu hypercalls to set all regs

[Alexander Graf <agraf@suse.de>]

On 27.07.2012, at 08:18, Benjamin Herrenschmidt wrote:

> Right now, whenever we exit for an hcall, upon return, we fetch
> some register values from the structure that carries the hcall
> results and update the vcpu accordingly.
> 
> However, if the hypercall chooses instead to update the registers
> itself by calling set_regs, then we end up clobbering those values.
> 
> This is for example the case of the rtas calls used to reboot the
> machine where a new CPU state is established and must not be clobbered.
> 
> The simple fix is to always clear the hcall_needed flag when a
> set-regs happen. This fixes reboot problems.

I do understand the race you're running into. However, I don't understand why. QEMU's hcall return code fetches its GPRs from env->gprs. That should be perfectly fine if the values are 0 before you return from the hcall function.

Could you please try to manually set the relevant regs to 0 in QEMU and see if that also fixes reboot for you? That'd keep the interface a bit less complex.


Alex

> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> 
> Note: There are similar issues if the reboot is triggered by an MMIO
> emulation, or an OSI call, Alex, you might want to handle those cases
> as well. In any cases, I'd like this to still go into 3.6 but I can
> send it myself to Linus if you want.
> 
> diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
> index 3f2a836..9ab13d6 100644
> --- a/arch/powerpc/kvm/book3s.c
> +++ b/arch/powerpc/kvm/book3s.c
> @@ -463,6 +463,14 @@ int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
> 	for (i = 0; i < ARRAY_SIZE(regs->gpr); i++)
> 		kvmppc_set_gpr(vcpu, i, regs->gpr[i]);
> 
> +	/*
> +	 * If the hypercall decides to change register values
> +	 * explicitly, we must ensure that we do not override
> +	 * them upon return from that hypercall. Among others
> +	 * this happens on system reset
> +	 */
> +	vcpu->arch.hcall_needed = 0;
> +
> 	return 0;
> }
> 
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 87f4dc8..75e196d 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -592,6 +592,11 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
> 	} else if (vcpu->arch.hcall_needed) {
> 		int i;
> 
> +		/*
> +		 * Note: This might not be called if the hypervisor
> +		 * call has done a set_regs(). In this case hcall_needed
> +		 * is cleared. This is necessary for reset to work properly
> +		 */
> 		kvmppc_set_gpr(vcpu, 3, run->papr_hcall.ret);
> 		for (i = 0; i < 9; ++i)
> 			kvmppc_set_gpr(vcpu, 4 + i, run->papr_hcall.args[i]);
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html