| Message ID | 20160425084111.GA19515@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 25/04/16 09:41, AKASHI Takahiro wrote: > On Wed, Apr 20, 2016 at 12:19:45PM +0100, James Morse wrote: >> On 20/04/16 11:37, Marc Zyngier wrote: >>> On 19/04/16 18:37, James Morse wrote: >>>> It looks like x86 uses the extable to work around this, their vmx_vcpu_run() has: >>>>> __ex(ASM_VMX_VMLAUNCH) "\n\t" >>>> Where __ex ends up calling ____kvm_handle_fault_on_reboot(), with a nearby comment: >>>>> * Hardware virtualization extension instructions may fault if a >>>>> * reboot turns off virtualization while processes are running. >>>>> * Trap the fault and ignore the instruction if that happens. >>> >>> I very much like that approach, to be honest. Tearing down a CPU is >>> something exceptional, so let's make it an actual exception. >>> >>> It is now pretty easy to discriminate between KVM functions and stub >>> functions thanks to your earlier patch, so if we end up calling the >>> hyp-stub because we've torn down KVM's EL2, let's just return an >>> appropriate error code (ARM_EXCEPTION_HYP_GONE), and handle it at EL1. >> >> Okay. kexec uses kvm_call_hyp() against the hyp-stub to do the kernel-copy and >> hand over to purgatory, but we could change that to a new 'special' builtin >> call, something like HVC_KEXEC_CALL_HYP. It never calls it with kvm loaded, so >> there is no reason the calls have to be same. >> >> Given hibernate doesn't hit this issue, I will drop this hunk from this version >> of the patch, and repost hibernate incorporating the feedback so far. I will >> provide a patch for kexec to do the above. > > Thanks, but you don' have to. I was wrong with the 'hibernate doesn't hit this issue', with this patch we re-install the hyp-stub during system reboot, and race with the scheduler. ('reboot -f' while running a guest). > If the fix below is acceptable, we will merge it to our next kexec/kdump > patch series. I'm testing something that looks very similar at the moment. > diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c > index eba89e4..31b5224 100644 > --- a/arch/arm64/kvm/handle_exit.c > +++ b/arch/arm64/kvm/handle_exit.c > @@ -186,6 +186,10 @@ int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run, > exit_handler = kvm_get_exit_handler(vcpu); > > return exit_handler(vcpu, run); > + case ARM_EXCEPTION_HYP_GONE: > + /* due to kexec reboot */ > + run->exit_reason = KVM_EXIT_SHUTDOWN; > + return 0; Is it fair to throw this back out to user space? While the hypervisor doesn't have long to live, it may not be expecting this exit_reason. I couldn't see a value for 'suprise cpu removal', and it looks like the x86 code causes the vcpu to spin round enter guest... Thanks, James
On 25/04/16 10:16, James Morse wrote: > Hi, > > On 25/04/16 09:41, AKASHI Takahiro wrote: >> On Wed, Apr 20, 2016 at 12:19:45PM +0100, James Morse wrote: >>> On 20/04/16 11:37, Marc Zyngier wrote: >>>> On 19/04/16 18:37, James Morse wrote: >>>>> It looks like x86 uses the extable to work around this, their vmx_vcpu_run() has: >>>>>> __ex(ASM_VMX_VMLAUNCH) "\n\t" >>>>> Where __ex ends up calling ____kvm_handle_fault_on_reboot(), with a nearby comment: >>>>>> * Hardware virtualization extension instructions may fault if a >>>>>> * reboot turns off virtualization while processes are running. >>>>>> * Trap the fault and ignore the instruction if that happens. >>>> >>>> I very much like that approach, to be honest. Tearing down a CPU is >>>> something exceptional, so let's make it an actual exception. >>>> >>>> It is now pretty easy to discriminate between KVM functions and stub >>>> functions thanks to your earlier patch, so if we end up calling the >>>> hyp-stub because we've torn down KVM's EL2, let's just return an >>>> appropriate error code (ARM_EXCEPTION_HYP_GONE), and handle it at EL1. >>> >>> Okay. kexec uses kvm_call_hyp() against the hyp-stub to do the kernel-copy and >>> hand over to purgatory, but we could change that to a new 'special' builtin >>> call, something like HVC_KEXEC_CALL_HYP. It never calls it with kvm loaded, so >>> there is no reason the calls have to be same. >>> >>> Given hibernate doesn't hit this issue, I will drop this hunk from this version >>> of the patch, and repost hibernate incorporating the feedback so far. I will >>> provide a patch for kexec to do the above. >> >> Thanks, but you don' have to. > > I was wrong with the 'hibernate doesn't hit this issue', with this patch we > re-install the hyp-stub during system reboot, and race with the scheduler. > ('reboot -f' while running a guest). > > >> If the fix below is acceptable, we will merge it to our next kexec/kdump >> patch series. > > I'm testing something that looks very similar at the moment. > > >> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c >> index eba89e4..31b5224 100644 >> --- a/arch/arm64/kvm/handle_exit.c >> +++ b/arch/arm64/kvm/handle_exit.c >> @@ -186,6 +186,10 @@ int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run, >> exit_handler = kvm_get_exit_handler(vcpu); >> >> return exit_handler(vcpu, run); >> + case ARM_EXCEPTION_HYP_GONE: >> + /* due to kexec reboot */ >> + run->exit_reason = KVM_EXIT_SHUTDOWN; >> + return 0; > > Is it fair to throw this back out to user space? While the hypervisor doesn't > have long to live, it may not be expecting this exit_reason. I couldn't see a > value for 'suprise cpu removal', and it looks like the x86 code causes the vcpu > to spin round enter guest... Yeah, KVM_EXIT_SHUTDOWN is a vcpu reset (which on x86 is caused by a triple fault). KVM_EXIT_FAIL_ENTRY seems slightly better. As for getting back to userspace, I don't see that as a problem (though the documentation on that part of the API is... sparse). Thanks, M.
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c index 962904a..0e92787 100644 --- a/arch/arm/kvm/arm.c +++ b/arch/arm/kvm/arm.c @@ -587,13 +587,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) /* * Re-check atomic conditions */ - if (unlikely(!__this_cpu_read(kvm_arm_hardware_enabled))) { - /* cpu has been torn down */ - ret = 0; - run->exit_reason = KVM_EXIT_FAIL_ENTRY; - run->fail_entry.hardware_entry_failure_reason - = (u64)-ENOEXEC; - } else if (signal_pending(current)) { + if (signal_pending(current)) { ret = -EINTR; run->exit_reason = KVM_EXIT_INTR; } diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h index ebc8d0e..7f653ad 100644 --- a/arch/arm64/include/asm/kvm_asm.h +++ b/arch/arm64/include/asm/kvm_asm.h @@ -22,6 +22,7 @@ #define ARM_EXCEPTION_IRQ 0 #define ARM_EXCEPTION_TRAP 1 +#define ARM_EXCEPTION_HYP_GONE 2 #define KVM_ARM64_DEBUG_DIRTY_SHIFT 0 #define KVM_ARM64_DEBUG_DIRTY (1 << KVM_ARM64_DEBUG_DIRTY_SHIFT) diff --git a/arch/arm64/include/asm/virt.h b/arch/arm64/include/asm/virt.h index d406d2e..9079661 100644 --- a/arch/arm64/include/asm/virt.h +++ b/arch/arm64/include/asm/virt.h @@ -34,6 +34,11 @@ */ #define HVC_SET_VECTORS 1 +/* + * HVC_KEXEC_RESTART + */ +#define HVC_KEXEC_RESTART 2 + #define BOOT_CPU_MODE_EL1 (0xe11) #define BOOT_CPU_MODE_EL2 (0xe12) diff --git a/arch/arm64/kernel/cpu-reset.S b/arch/arm64/kernel/cpu-reset.S index 11a5bc6..4d35331 100644 --- a/arch/arm64/kernel/cpu-reset.S +++ b/arch/arm64/kernel/cpu-reset.S @@ -41,10 +41,7 @@ ENTRY(__cpu_soft_restart) isb cbz x0, 1f // el2_switch? - mov x0, x1 // entry - mov x1, x2 // arg0 - mov x2, x3 // arg1 - mov x3, x4 // arg2 + mov x0, #HVC_KEXEC_RESTART hvc #0 // no return 1: mov x18, x1 // entry diff --git a/arch/arm64/kernel/hyp-stub.S b/arch/arm64/kernel/hyp-stub.S index 6bba25c..7fad129 100644 --- a/arch/arm64/kernel/hyp-stub.S +++ b/arch/arm64/kernel/hyp-stub.S @@ -22,7 +22,8 @@ #include <linux/irqchip/arm-gic-v3.h> #include <asm/assembler.h> -#include <asm/kvm_arm.h> +#include <asm/esr.h> +#include <asm/kvm_asm.h> #include <asm/ptrace.h> #include <asm/virt.h> @@ -70,7 +71,16 @@ el1_sync: msr vbar_el2, x1 b 9f -2: do_el2_call +2: cmp x0, #HVC_KEXEC_RESTART + b.eq 3f + mov x0, #ARM_EXCEPTION_HYP_GONE + b 9f + +3: mov lr, x1 + mov x0, x2 + mov x1, x3 + mov x2, x4 + blr lr 9: eret ENDPROC(el1_sync) diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c index eba89e4..31b5224 100644 --- a/arch/arm64/kvm/handle_exit.c +++ b/arch/arm64/kvm/handle_exit.c @@ -186,6 +186,10 @@ int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run, exit_handler = kvm_get_exit_handler(vcpu); return exit_handler(vcpu, run); + case ARM_EXCEPTION_HYP_GONE: + /* due to kexec reboot */ + run->exit_reason = KVM_EXIT_SHUTDOWN; + return 0; default: kvm_pr_unimpl("Unsupported exception type: %d", exception_index);