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@xxxxxxxxxxxxxxxxxxx>
> ---
>
> 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
