[PATCH] arm64: KVM: Turn kvm_ksym_ref into a NOP on VHE

[PATCH] arm64: KVM: Turn kvm_ksym_ref into a NOP on VHE

[Marc Zyngier]

When running with VHE, there is no need to translate kernel pointers
to the EL2 memory space, since we're already there (and we have a much
saner memory map to start with).

Unfortunately, kvm_ksym_ref is getting in the way, and the first
call into the "hypervisor" section is going to end up in fireworks,
since we're now branching into nowhereland. Meh.

A potential solution is to test if VHE is engaged or not, and only
perform the translation in the negative case. With this in place,
VHE is able to run again.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/kvm_asm.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
index 226f49d..282f907 100644
--- a/arch/arm64/include/asm/kvm_asm.h
+++ b/arch/arm64/include/asm/kvm_asm.h
@@ -26,7 +26,13 @@
 #define KVM_ARM64_DEBUG_DIRTY_SHIFT	0
 #define KVM_ARM64_DEBUG_DIRTY		(1 << KVM_ARM64_DEBUG_DIRTY_SHIFT)
 
-#define kvm_ksym_ref(sym)		phys_to_virt((u64)&sym - kimage_voffset)
+#define kvm_ksym_ref(sym)						\
+	({								\
+		void *val = sym;					\
+		if (!is_kernel_in_hyp_mode())				\
+			val = phys_to_virt((u64)&sym - kimage_voffset);	\
+		val;							\
+	 })
 
 #ifndef __ASSEMBLY__
 struct kvm;
-- 
2.1.4

[PATCH] arm64: KVM: Turn kvm_ksym_ref into a NOP on VHE

[Robin Murphy]

Hi Marc,

On 18/03/16 17:25, Marc Zyngier wrote:
> When running with VHE, there is no need to translate kernel pointers
> to the EL2 memory space, since we're already there (and we have a much
> saner memory map to start with).
>
> Unfortunately, kvm_ksym_ref is getting in the way, and the first
> call into the "hypervisor" section is going to end up in fireworks,
> since we're now branching into nowhereland. Meh.
>
> A potential solution is to test if VHE is engaged or not, and only
> perform the translation in the negative case. With this in place,
> VHE is able to run again.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>   arch/arm64/include/asm/kvm_asm.h | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> index 226f49d..282f907 100644
> --- a/arch/arm64/include/asm/kvm_asm.h
> +++ b/arch/arm64/include/asm/kvm_asm.h
> @@ -26,7 +26,13 @@
>   #define KVM_ARM64_DEBUG_DIRTY_SHIFT	0
>   #define KVM_ARM64_DEBUG_DIRTY		(1 << KVM_ARM64_DEBUG_DIRTY_SHIFT)
>
> -#define kvm_ksym_ref(sym)		phys_to_virt((u64)&sym - kimage_voffset)
> +#define kvm_ksym_ref(sym)						\
> +	({								\
> +		void *val = sym;					\
> +		if (!is_kernel_in_hyp_mode())				\
> +			val = phys_to_virt((u64)&sym - kimage_voffset);	\

Is it definitely OK to evaluate sym twice here?

Robin.

> +		val;							\
> +	 })
>
>   #ifndef __ASSEMBLY__
>   struct kvm;
>

[PATCH] arm64: KVM: Turn kvm_ksym_ref into a NOP on VHE

[Catalin Marinas]

On Fri, Mar 18, 2016 at 05:25:59PM +0000, Marc Zyngier wrote:
> When running with VHE, there is no need to translate kernel pointers
> to the EL2 memory space, since we're already there (and we have a much
> saner memory map to start with).
> 
> Unfortunately, kvm_ksym_ref is getting in the way, and the first
> call into the "hypervisor" section is going to end up in fireworks,
> since we're now branching into nowhereland. Meh.
> 
> A potential solution is to test if VHE is engaged or not, and only
> perform the translation in the negative case. With this in place,
> VHE is able to run again.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/include/asm/kvm_asm.h | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> index 226f49d..282f907 100644
> --- a/arch/arm64/include/asm/kvm_asm.h
> +++ b/arch/arm64/include/asm/kvm_asm.h
> @@ -26,7 +26,13 @@
>  #define KVM_ARM64_DEBUG_DIRTY_SHIFT	0
>  #define KVM_ARM64_DEBUG_DIRTY		(1 << KVM_ARM64_DEBUG_DIRTY_SHIFT)
>  
> -#define kvm_ksym_ref(sym)		phys_to_virt((u64)&sym - kimage_voffset)
> +#define kvm_ksym_ref(sym)						\
> +	({								\
> +		void *val = sym;					\

Should we not use &sym, for consistency and just in case we ever pass
something other than a function.

> +		if (!is_kernel_in_hyp_mode())				\
> +			val = phys_to_virt((u64)&sym - kimage_voffset);	\
> +		val;							\
> +	 })

Otherwise:

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

[PATCH] arm64: KVM: Turn kvm_ksym_ref into a NOP on VHE

[Christoffer Dall]

On Fri, Mar 18, 2016 at 06:07:09PM +0000, Catalin Marinas wrote:
> On Fri, Mar 18, 2016 at 05:25:59PM +0000, Marc Zyngier wrote:
> > When running with VHE, there is no need to translate kernel pointers
> > to the EL2 memory space, since we're already there (and we have a much
> > saner memory map to start with).
> > 
> > Unfortunately, kvm_ksym_ref is getting in the way, and the first
> > call into the "hypervisor" section is going to end up in fireworks,
> > since we're now branching into nowhereland. Meh.
> > 
> > A potential solution is to test if VHE is engaged or not, and only
> > perform the translation in the negative case. With this in place,
> > VHE is able to run again.
> > 
> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > ---
> >  arch/arm64/include/asm/kvm_asm.h | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> > index 226f49d..282f907 100644
> > --- a/arch/arm64/include/asm/kvm_asm.h
> > +++ b/arch/arm64/include/asm/kvm_asm.h
> > @@ -26,7 +26,13 @@
> >  #define KVM_ARM64_DEBUG_DIRTY_SHIFT	0
> >  #define KVM_ARM64_DEBUG_DIRTY		(1 << KVM_ARM64_DEBUG_DIRTY_SHIFT)
> >  
> > -#define kvm_ksym_ref(sym)		phys_to_virt((u64)&sym - kimage_voffset)
> > +#define kvm_ksym_ref(sym)						\
> > +	({								\
> > +		void *val = sym;					\
> 
> Should we not use &sym, for consistency and just in case we ever pass
> something other than a function.
> 

we already do, we pass the symbox for an extern char _foo[] array, but
that should work just the same with sym and &sym.

Given the effort put in to avoid these kinds of branches for the VHE
code, it may be worth optimizing this later on with the use of
instruction patching or static keys to avoid the branch on every vm
entry/exit?

> > +		if (!is_kernel_in_hyp_mode())				\
> > +			val = phys_to_virt((u64)&sym - kimage_voffset);	\
> > +		val;							\
> > +	 })
> 
> Otherwise:
> 
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Thanks, I can change to &sym and queue this patch (not assuming you plan
on taking it directly via the arm64 tree).

-Christoffer

[PATCH] arm64: KVM: Turn kvm_ksym_ref into a NOP on VHE

[Catalin Marinas]

On Sun, Mar 20, 2016 at 10:07:04PM +0100, Christoffer Dall wrote:
> On Fri, Mar 18, 2016 at 06:07:09PM +0000, Catalin Marinas wrote:
> > On Fri, Mar 18, 2016 at 05:25:59PM +0000, Marc Zyngier wrote:
> > > When running with VHE, there is no need to translate kernel pointers
> > > to the EL2 memory space, since we're already there (and we have a much
> > > saner memory map to start with).
> > > 
> > > Unfortunately, kvm_ksym_ref is getting in the way, and the first
> > > call into the "hypervisor" section is going to end up in fireworks,
> > > since we're now branching into nowhereland. Meh.
> > > 
> > > A potential solution is to test if VHE is engaged or not, and only
> > > perform the translation in the negative case. With this in place,
> > > VHE is able to run again.
> > > 
> > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > > ---
> > >  arch/arm64/include/asm/kvm_asm.h | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> > > index 226f49d..282f907 100644
> > > --- a/arch/arm64/include/asm/kvm_asm.h
> > > +++ b/arch/arm64/include/asm/kvm_asm.h
> > > @@ -26,7 +26,13 @@
> > >  #define KVM_ARM64_DEBUG_DIRTY_SHIFT	0
> > >  #define KVM_ARM64_DEBUG_DIRTY		(1 << KVM_ARM64_DEBUG_DIRTY_SHIFT)
> > >  
> > > -#define kvm_ksym_ref(sym)		phys_to_virt((u64)&sym - kimage_voffset)
> > > +#define kvm_ksym_ref(sym)						\
> > > +	({								\
> > > +		void *val = sym;					\
> > 
> > Should we not use &sym, for consistency and just in case we ever pass
> > something other than a function.
> 
> we already do, we pass the symbox for an extern char _foo[] array, but
> that should work just the same with sym and &sym.

Just for consistency between "val = sym" and "phys_to_virt((u64)&sym..."

> Given the effort put in to avoid these kinds of branches for the VHE
> code, it may be worth optimizing this later on with the use of
> instruction patching or static keys to avoid the branch on every vm
> entry/exit?

Marc tried with run-time patching first but the code was still reading
kimage_voffset even with VHE. Anyway, it can be optimised later.

> > > +		if (!is_kernel_in_hyp_mode())				\
> > > +			val = phys_to_virt((u64)&sym - kimage_voffset);	\
> > > +		val;							\
> > > +	 })
> > 
> > Otherwise:
> > 
> > Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> 
> Thanks, I can change to &sym and queue this patch (not assuming you plan
> on taking it directly via the arm64 tree).

Please feel free to merge this via the kvm tree.

-- 
Catalin

[PATCH] arm64: KVM: Turn kvm_ksym_ref into a NOP on VHE

[Ard Biesheuvel]

On 18 March 2016 at 18:25, Marc Zyngier <marc.zyngier@arm.com> wrote:
> When running with VHE, there is no need to translate kernel pointers
> to the EL2 memory space, since we're already there (and we have a much
> saner memory map to start with).
>
> Unfortunately, kvm_ksym_ref is getting in the way, and the first
> call into the "hypervisor" section is going to end up in fireworks,
> since we're now branching into nowhereland. Meh.
>
> A potential solution is to test if VHE is engaged or not, and only
> perform the translation in the negative case. With this in place,
> VHE is able to run again.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

I think you need the & when initializing val, otherwise, it will
silently refer to the value rather than the address of a void* symbol
if we ever end up using this macro on one.

That was the whoie point of the opaque struct type in the original
patch that introduced this macro, to disallow references lacking the
&, but unfortunately, that was incompatible with the other VHE
changes.

With that fixed

Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>  arch/arm64/include/asm/kvm_asm.h | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> index 226f49d..282f907 100644
> --- a/arch/arm64/include/asm/kvm_asm.h
> +++ b/arch/arm64/include/asm/kvm_asm.h
> @@ -26,7 +26,13 @@
>  #define KVM_ARM64_DEBUG_DIRTY_SHIFT    0
>  #define KVM_ARM64_DEBUG_DIRTY          (1 << KVM_ARM64_DEBUG_DIRTY_SHIFT)
>
> -#define kvm_ksym_ref(sym)              phys_to_virt((u64)&sym - kimage_voffset)
> +#define kvm_ksym_ref(sym)                                              \
> +       ({                                                              \
> +               void *val = sym;                                        \
> +               if (!is_kernel_in_hyp_mode())                           \
> +                       val = phys_to_virt((u64)&sym - kimage_voffset); \
> +               val;                                                    \
> +        })
>
>  #ifndef __ASSEMBLY__
>  struct kvm;
> --
> 2.1.4
>

[PATCH] arm64: KVM: Turn kvm_ksym_ref into a NOP on VHE

[Paolo Bonzini]

On 21/03/2016 09:47, Ard Biesheuvel wrote:
> On 18 March 2016 at 18:25, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> When running with VHE, there is no need to translate kernel pointers
>> to the EL2 memory space, since we're already there (and we have a much
>> saner memory map to start with).
>>
>> Unfortunately, kvm_ksym_ref is getting in the way, and the first
>> call into the "hypervisor" section is going to end up in fireworks,
>> since we're now branching into nowhereland. Meh.
>>
>> A potential solution is to test if VHE is engaged or not, and only
>> perform the translation in the negative case. With this in place,
>> VHE is able to run again.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> 
> I think you need the & when initializing val, otherwise, it will
> silently refer to the value rather than the address of a void* symbol
> if we ever end up using this macro on one.
> 
> That was the whoie point of the opaque struct type in the original
> patch that introduced this macro, to disallow references lacking the
> &, but unfortunately, that was incompatible with the other VHE
> changes.
> 
> With that fixed
> 
> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Marc, if you send a v2 I can merge it directly without a pull request.

Paolo

[PATCH] arm64: KVM: Turn kvm_ksym_ref into a NOP on VHE

[Christoffer Dall]

On Mon, Mar 21, 2016 at 01:36:10PM +0100, Paolo Bonzini wrote:
> 
> 
> On 21/03/2016 09:47, Ard Biesheuvel wrote:
> > On 18 March 2016 at 18:25, Marc Zyngier <marc.zyngier@arm.com> wrote:
> >> When running with VHE, there is no need to translate kernel pointers
> >> to the EL2 memory space, since we're already there (and we have a much
> >> saner memory map to start with).
> >>
> >> Unfortunately, kvm_ksym_ref is getting in the way, and the first
> >> call into the "hypervisor" section is going to end up in fireworks,
> >> since we're now branching into nowhereland. Meh.
> >>
> >> A potential solution is to test if VHE is engaged or not, and only
> >> perform the translation in the negative case. With this in place,
> >> VHE is able to run again.
> >>
> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > 
> > I think you need the & when initializing val, otherwise, it will
> > silently refer to the value rather than the address of a void* symbol
> > if we ever end up using this macro on one.
> > 
> > That was the whoie point of the opaque struct type in the original
> > patch that introduced this macro, to disallow references lacking the
> > &, but unfortunately, that was incompatible with the other VHE
> > changes.
> > 
> > With that fixed
> > 
> > Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> Marc, if you send a v2 I can merge it directly without a pull request.
> 
Marc is on holiday this week, but I put this (with the fix) on top of
Linus' master in the kvmarm/master branch:

git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git master

Do you want to just grab it from there?

Thanks,
-Christoffer

[PATCH] arm64: KVM: Turn kvm_ksym_ref into a NOP on VHE

[Paolo Bonzini]

On 21/03/2016 13:42, Christoffer Dall wrote:
> Marc is on holiday this week, but I put this (with the fix) on top of
> Linus' master in the kvmarm/master branch:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git master
> 
> Do you want to just grab it from there?

Since you have two more patches, I guess you can just send a pull
request for all three.  I plan to send mine to Linus on Wednesday.

Thanks,

Paolo

[PATCH] arm64: KVM: Turn kvm_ksym_ref into a NOP on VHE

[Christoffer Dall]

On Mon, Mar 21, 2016 at 03:02:19PM +0100, Paolo Bonzini wrote:
> 
> 
> On 21/03/2016 13:42, Christoffer Dall wrote:
> > Marc is on holiday this week, but I put this (with the fix) on top of
> > Linus' master in the kvmarm/master branch:
> > 
> > git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git master
> > 
> > Do you want to just grab it from there?
> 
> Since you have two more patches, I guess you can just send a pull
> request for all three.  I plan to send mine to Linus on Wednesday.
> 
Sounds good, I'll send you one tomorrow (based on Linus' master branch
of today - if that works?)

-Christoffer

[PATCH] arm64: KVM: Turn kvm_ksym_ref into a NOP on VHE

[Paolo Bonzini]

On 21/03/2016 19:55, Christoffer Dall wrote:
> > Since you have two more patches, I guess you can just send a pull
> > request for all three.  I plan to send mine to Linus on Wednesday.
> 
> Sounds good, I'll send you one tomorrow (based on Linus' master branch
> of today - if that works?)

Anything that you can test with works.  Even basing it on your last pull
request is okay for me, and it's better for future bisection if there's
some known breakage in Linus's master branch.

Paolo

[PATCH] arm64: KVM: Turn kvm_ksym_ref into a NOP on VHE

[Christoffer Dall]

On Mon, Mar 21, 2016 at 09:55:51PM +0100, Paolo Bonzini wrote:
> 
> 
> On 21/03/2016 19:55, Christoffer Dall wrote:
> > > Since you have two more patches, I guess you can just send a pull
> > > request for all three.  I plan to send mine to Linus on Wednesday.
> > 
> > Sounds good, I'll send you one tomorrow (based on Linus' master branch
> > of today - if that works?)
> 
> Anything that you can test with works.  Even basing it on your last pull
> request is okay for me, and it's better for future bisection if there's
> some known breakage in Linus's master branch.
> 
The changes to kvm_ksym_ref() didn't apply to anything in the kvmarm
side of things but needed stuff from Catalin, so I just applied it on
top of Linus' master.

I'll send you the pull request later and just holler if I need to change
it somehow.

Thanks,
-Christoffer