| Message ID | 20170306142458.8875-2-marc.zyngier@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/03/17 14:24, Marc Zyngier wrote: > Let's define a new stub hypercall that resets the HYP configuration > to its default: hyp-stub vectors, and MMU disabled. > > Of course, for the hyp-stub itself, this is a trivial no-op. > Hypervisors will have a bit more work to do. Reviewed-by: James Morse <james.morse@arm.com> Thanks, James
On Mon, Mar 06, 2017 at 02:24:34PM +0000, Marc Zyngier wrote: > Let's define a new stub hypercall that resets the HYP configuration > to its default: hyp-stub vectors, and MMU disabled. > > Of course, for the hyp-stub itself, this is a trivial no-op. > Hypervisors will have a bit more work to do. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > arch/arm64/include/asm/virt.h | 9 +++++++++ > arch/arm64/kernel/hyp-stub.S | 13 ++++++++++++- > 2 files changed, 21 insertions(+), 1 deletion(-) [...] > +ENTRY(__hyp_reset_vectors) > + str lr, [sp, #-16]! > + mov x0, #HVC_RESET_VECTORS > + hvc #0 > + ldr lr, [sp], #16 > + ret > +ENDPROC(__hyp_reset_vectors) Why do we need to specifically preserve lr across the hvc call? Is it corrupted by the EL2 code (if yes, are other caller-saved registers that need preserving)? I don't see something similar in the arch/arm code.
Hi Catalin, On 21/03/17 17:04, Catalin Marinas wrote: > On Mon, Mar 06, 2017 at 02:24:34PM +0000, Marc Zyngier wrote: >> Let's define a new stub hypercall that resets the HYP configuration >> to its default: hyp-stub vectors, and MMU disabled. >> >> Of course, for the hyp-stub itself, this is a trivial no-op. >> Hypervisors will have a bit more work to do. >> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >> --- >> arch/arm64/include/asm/virt.h | 9 +++++++++ >> arch/arm64/kernel/hyp-stub.S | 13 ++++++++++++- >> 2 files changed, 21 insertions(+), 1 deletion(-) > [...] >> +ENTRY(__hyp_reset_vectors) >> + str lr, [sp, #-16]! >> + mov x0, #HVC_RESET_VECTORS >> + hvc #0 >> + ldr lr, [sp], #16 >> + ret >> +ENDPROC(__hyp_reset_vectors) > > Why do we need to specifically preserve lr across the hvc call? Is it > corrupted by the EL2 code (if yes, are other caller-saved registers that > need preserving)? I don't see something similar in the arch/arm code. Kexec on arm64 needed a register to clobber in the hyp-stub's el1_sync code. We wanted to preserve all the registers so soft_restart() could look more like a function call. Thanks, James
On 21/03/17 17:04, Catalin Marinas wrote: > On Mon, Mar 06, 2017 at 02:24:34PM +0000, Marc Zyngier wrote: >> Let's define a new stub hypercall that resets the HYP configuration >> to its default: hyp-stub vectors, and MMU disabled. >> >> Of course, for the hyp-stub itself, this is a trivial no-op. >> Hypervisors will have a bit more work to do. >> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >> --- >> arch/arm64/include/asm/virt.h | 9 +++++++++ >> arch/arm64/kernel/hyp-stub.S | 13 ++++++++++++- >> 2 files changed, 21 insertions(+), 1 deletion(-) > [...] >> +ENTRY(__hyp_reset_vectors) >> + str lr, [sp, #-16]! >> + mov x0, #HVC_RESET_VECTORS >> + hvc #0 >> + ldr lr, [sp], #16 >> + ret >> +ENDPROC(__hyp_reset_vectors) > > Why do we need to specifically preserve lr across the hvc call? Is it > corrupted by the EL2 code (if yes, are other caller-saved registers that > need preserving)? I don't see something similar in the arch/arm code. Yeah, that's another oddity. The KVM code uses it as a temp register, but that feels quite wrong, now that you mention it. If should be saved there, and definitely not in the stubs. Let me grab a hammer, and I'll set that one straight. Thanks, M.
On 21/03/17 17:25, James Morse wrote: > Hi Catalin, > > On 21/03/17 17:04, Catalin Marinas wrote: >> On Mon, Mar 06, 2017 at 02:24:34PM +0000, Marc Zyngier wrote: >>> Let's define a new stub hypercall that resets the HYP configuration >>> to its default: hyp-stub vectors, and MMU disabled. >>> >>> Of course, for the hyp-stub itself, this is a trivial no-op. >>> Hypervisors will have a bit more work to do. >>> >>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >>> --- >>> arch/arm64/include/asm/virt.h | 9 +++++++++ >>> arch/arm64/kernel/hyp-stub.S | 13 ++++++++++++- >>> 2 files changed, 21 insertions(+), 1 deletion(-) >> [...] >>> +ENTRY(__hyp_reset_vectors) >>> + str lr, [sp, #-16]! >>> + mov x0, #HVC_RESET_VECTORS >>> + hvc #0 >>> + ldr lr, [sp], #16 >>> + ret >>> +ENDPROC(__hyp_reset_vectors) >> >> Why do we need to specifically preserve lr across the hvc call? Is it >> corrupted by the EL2 code (if yes, are other caller-saved registers that >> need preserving)? I don't see something similar in the arch/arm code. > > Kexec on arm64 needed a register to clobber in the hyp-stub's el1_sync code. We > wanted to preserve all the registers so soft_restart() could look more like a > function call. I don't think we need this anymore. Once we enter __cpu_soft_restart(), there is no turning back. Or am I missing something else? Thanks, M.
On 21/03/17 17:37, Marc Zyngier wrote: > On 21/03/17 17:25, James Morse wrote: >> On 21/03/17 17:04, Catalin Marinas wrote: >>> On Mon, Mar 06, 2017 at 02:24:34PM +0000, Marc Zyngier wrote: >>>> Let's define a new stub hypercall that resets the HYP configuration >>>> to its default: hyp-stub vectors, and MMU disabled. >>>> >>>> Of course, for the hyp-stub itself, this is a trivial no-op. >>>> Hypervisors will have a bit more work to do. >>>> >>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >>>> --- >>>> arch/arm64/include/asm/virt.h | 9 +++++++++ >>>> arch/arm64/kernel/hyp-stub.S | 13 ++++++++++++- >>>> 2 files changed, 21 insertions(+), 1 deletion(-) >>> [...] >>>> +ENTRY(__hyp_reset_vectors) >>>> + str lr, [sp, #-16]! >>>> + mov x0, #HVC_RESET_VECTORS >>>> + hvc #0 >>>> + ldr lr, [sp], #16 >>>> + ret >>>> +ENDPROC(__hyp_reset_vectors) >>> >>> Why do we need to specifically preserve lr across the hvc call? Is it >>> corrupted by the EL2 code (if yes, are other caller-saved registers that >>> need preserving)? I don't see something similar in the arch/arm code. >> >> Kexec on arm64 needed a register to clobber in the hyp-stub's el1_sync code. We >> wanted to preserve all the registers so soft_restart() could look more like a >> function call. > > I don't think we need this anymore. Once we enter __cpu_soft_restart(), > there is no turning back. Or am I missing something else? My recollection of the history may be wrong: but we needed to mess with esr_el2 before we know its a soft_restart() call, at which point we didn't want to clobber the registers. This was the strange use of x18 in kexec. James
diff --git a/arch/arm64/include/asm/virt.h b/arch/arm64/include/asm/virt.h index 439f6b5d31f6..f24f1eb72dc0 100644 --- a/arch/arm64/include/asm/virt.h +++ b/arch/arm64/include/asm/virt.h @@ -39,6 +39,14 @@ */ #define HVC_SOFT_RESTART 2 +/* + * HVC_RESET_VECTORS - Restore the vectors to the original HYP stubs + */ +#define HVC_RESET_VECTORS 3 + +/* Max number of HYP stub hypercalls */ +#define HVC_STUB_HCALL_NR 4 + #define BOOT_CPU_MODE_EL1 (0xe11) #define BOOT_CPU_MODE_EL2 (0xe12) @@ -62,6 +70,7 @@ extern u32 __boot_cpu_mode[2]; void __hyp_set_vectors(phys_addr_t phys_vector_base); phys_addr_t __hyp_get_vectors(void); +void __hyp_reset_vectors(void); /* Reports the availability of HYP mode */ static inline bool is_hyp_mode_available(void) diff --git a/arch/arm64/kernel/hyp-stub.S b/arch/arm64/kernel/hyp-stub.S index d3b5f75e652e..a162182d5662 100644 --- a/arch/arm64/kernel/hyp-stub.S +++ b/arch/arm64/kernel/hyp-stub.S @@ -79,8 +79,11 @@ el1_sync: mov x1, x3 br x4 // no return +3: cmp x0, #HVC_RESET_VECTORS + beq 9f // Nothing to reset! + /* Someone called kvm_call_hyp() against the hyp-stub... */ -3: mov x0, #ARM_EXCEPTION_HYP_GONE + mov x0, #ARM_EXCEPTION_HYP_GONE 9: eret ENDPROC(el1_sync) @@ -137,3 +140,11 @@ ENTRY(__hyp_set_vectors) ldr lr, [sp], #16 ret ENDPROC(__hyp_set_vectors) + +ENTRY(__hyp_reset_vectors) + str lr, [sp, #-16]! + mov x0, #HVC_RESET_VECTORS + hvc #0 + ldr lr, [sp], #16 + ret +ENDPROC(__hyp_reset_vectors)
Let's define a new stub hypercall that resets the HYP configuration to its default: hyp-stub vectors, and MMU disabled. Of course, for the hyp-stub itself, this is a trivial no-op. Hypervisors will have a bit more work to do. Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- arch/arm64/include/asm/virt.h | 9 +++++++++ arch/arm64/kernel/hyp-stub.S | 13 ++++++++++++- 2 files changed, 21 insertions(+), 1 deletion(-)