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

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

[Benjamin Herrenschmidt]

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.

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]);

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

[Alexander Graf]

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