Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1323098 > unrolled thread

Re: [PATCH v2 06/21] arm64: KVM: VHE: Patch out use of HVC

Started byChristoffer Dall <christoffer.dall@linaro.org>
First post2016-02-01 14:20 +0100
Last post2016-02-01 16:40 +0100
Articles 8 — 4 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 06/21] arm64: KVM: VHE: Patch out use of HVC Christoffer Dall <christoffer.dall@linaro.org> - 2016-02-01 14:20 +0100
    Re: [PATCH v2 06/21] arm64: KVM: VHE: Patch out use of HVC Marc Zyngier <marc.zyngier@arm.com> - 2016-02-01 14:40 +0100
      Re: [PATCH v2 06/21] arm64: KVM: VHE: Patch out use of HVC Catalin Marinas <catalin.marinas@arm.com> - 2016-02-01 16:40 +0100
        Re: [PATCH v2 06/21] arm64: KVM: VHE: Patch out use of HVC Marc Zyngier <marc.zyngier@arm.com> - 2016-02-01 17:30 +0100
          Re: [PATCH v2 06/21] arm64: KVM: VHE: Patch out use of HVC Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-02-01 18:10 +0100
            Re: [PATCH v2 06/21] arm64: KVM: VHE: Patch out use of HVC Marc Zyngier <marc.zyngier@arm.com> - 2016-02-01 18:30 +0100
              Re: [PATCH v2 06/21] arm64: KVM: VHE: Patch out use of HVC Christoffer Dall <christoffer.dall@linaro.org> - 2016-02-02 16:50 +0100
      Re: [PATCH v2 06/21] arm64: KVM: VHE: Patch out use of HVC Christoffer Dall <christoffer.dall@linaro.org> - 2016-02-01 16:40 +0100

#1323098 — Re: [PATCH v2 06/21] arm64: KVM: VHE: Patch out use of HVC

FromChristoffer Dall <christoffer.dall@linaro.org>
Date2016-02-01 14:20 +0100
SubjectRe: [PATCH v2 06/21] arm64: KVM: VHE: Patch out use of HVC
Message-ID<qXmyE-3iQ-25@gated-at.bofh.it>
On Mon, Jan 25, 2016 at 03:53:40PM +0000, Marc Zyngier wrote:
> With VHE, the host never issues an HVC instruction to get into the
> KVM code, as we can simply branch there.
> 
> Use runtime code patching to simplify things a bit.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/kvm/hyp.S           |  7 +++++++
>  arch/arm64/kvm/hyp/hyp-entry.S | 38 +++++++++++++++++++++++++++++---------
>  2 files changed, 36 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
> index 0ccdcbb..0689a74 100644
> --- a/arch/arm64/kvm/hyp.S
> +++ b/arch/arm64/kvm/hyp.S
> @@ -17,7 +17,9 @@
>  
>  #include <linux/linkage.h>
>  
> +#include <asm/alternative.h>
>  #include <asm/assembler.h>
> +#include <asm/cpufeature.h>
>  
>  /*
>   * u64 kvm_call_hyp(void *hypfn, ...);
> @@ -38,6 +40,11 @@
>   * arch/arm64/kernel/hyp_stub.S.
>   */
>  ENTRY(kvm_call_hyp)
> +alternative_if_not ARM64_HAS_VIRT_HOST_EXTN	
>  	hvc	#0
>  	ret
> +alternative_else
> +	b	__vhe_hyp_call
> +	nop
> +alternative_endif
>  ENDPROC(kvm_call_hyp)
> diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
> index 93e8d983..9e0683f 100644
> --- a/arch/arm64/kvm/hyp/hyp-entry.S
> +++ b/arch/arm64/kvm/hyp/hyp-entry.S
> @@ -38,6 +38,32 @@
>  	ldp	x0, x1, [sp], #16
>  .endm
>  
> +.macro do_el2_call
> +	/*
> +	 * Shuffle the parameters before calling the function
> +	 * pointed to in x0. Assumes parameters in x[1,2,3].
> +	 */
> +	stp	lr, xzr, [sp, #-16]!

remind me why this pair isn't just doing "str" instead of "stp" with the
xzr ?

> +	mov	lr, x0
> +	mov	x0, x1
> +	mov	x1, x2
> +	mov	x2, x3
> +	blr	lr
> +	ldp	lr, xzr, [sp], #16
> +.endm
> +
> +ENTRY(__vhe_hyp_call)
> +	do_el2_call
> +	/*
> +	 * We used to rely on having an exception return to get
> +	 * an implicit isb. In the E2H case, we don't have it anymore.
> +	 * rather than changing all the leaf functions, just do it here
> +	 * before returning to the rest of the kernel.
> +	 */

why is this not the case with an ISB before do_el2_call then?

> +	isb
> +	ret
> +ENDPROC(__vhe_hyp_call)
> +	
>  el1_sync:				// Guest trapped into EL2
>  	save_x0_to_x3
>  
> @@ -58,19 +84,13 @@ el1_sync:				// Guest trapped into EL2
>  	mrs	x0, vbar_el2
>  	b	2f
>  
> -1:	stp	lr, xzr, [sp, #-16]!
> -
> +1:
>  	/*
> -	 * Compute the function address in EL2, and shuffle the parameters.
> +	 * Perform the EL2 call
>  	 */
>  	kern_hyp_va	x0
> -	mov	lr, x0
> -	mov	x0, x1
> -	mov	x1, x2
> -	mov	x2, x3
> -	blr	lr
> +	do_el2_call
>  
> -	ldp	lr, xzr, [sp], #16
>  2:	eret
>  
>  el1_trap:
> -- 
> 2.1.4
>

[toc] | [next] | [standalone]

#1323118

FromMarc Zyngier <marc.zyngier@arm.com>
Date2016-02-01 14:40 +0100
Message-ID<qXmS0-3vq-33@gated-at.bofh.it>
In reply to#1323098
On 01/02/16 13:16, Christoffer Dall wrote:
> On Mon, Jan 25, 2016 at 03:53:40PM +0000, Marc Zyngier wrote:
>> With VHE, the host never issues an HVC instruction to get into the
>> KVM code, as we can simply branch there.
>>
>> Use runtime code patching to simplify things a bit.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  arch/arm64/kvm/hyp.S           |  7 +++++++
>>  arch/arm64/kvm/hyp/hyp-entry.S | 38 +++++++++++++++++++++++++++++---------
>>  2 files changed, 36 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
>> index 0ccdcbb..0689a74 100644
>> --- a/arch/arm64/kvm/hyp.S
>> +++ b/arch/arm64/kvm/hyp.S
>> @@ -17,7 +17,9 @@
>>  
>>  #include <linux/linkage.h>
>>  
>> +#include <asm/alternative.h>
>>  #include <asm/assembler.h>
>> +#include <asm/cpufeature.h>
>>  
>>  /*
>>   * u64 kvm_call_hyp(void *hypfn, ...);
>> @@ -38,6 +40,11 @@
>>   * arch/arm64/kernel/hyp_stub.S.
>>   */
>>  ENTRY(kvm_call_hyp)
>> +alternative_if_not ARM64_HAS_VIRT_HOST_EXTN	
>>  	hvc	#0
>>  	ret
>> +alternative_else
>> +	b	__vhe_hyp_call
>> +	nop
>> +alternative_endif
>>  ENDPROC(kvm_call_hyp)
>> diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
>> index 93e8d983..9e0683f 100644
>> --- a/arch/arm64/kvm/hyp/hyp-entry.S
>> +++ b/arch/arm64/kvm/hyp/hyp-entry.S
>> @@ -38,6 +38,32 @@
>>  	ldp	x0, x1, [sp], #16
>>  .endm
>>  
>> +.macro do_el2_call
>> +	/*
>> +	 * Shuffle the parameters before calling the function
>> +	 * pointed to in x0. Assumes parameters in x[1,2,3].
>> +	 */
>> +	stp	lr, xzr, [sp, #-16]!
> 
> remind me why this pair isn't just doing "str" instead of "stp" with the
> xzr ?

Because SP has to be aligned on a 16 bytes boundary at all times.

> 
>> +	mov	lr, x0
>> +	mov	x0, x1
>> +	mov	x1, x2
>> +	mov	x2, x3
>> +	blr	lr
>> +	ldp	lr, xzr, [sp], #16
>> +.endm
>> +
>> +ENTRY(__vhe_hyp_call)
>> +	do_el2_call
>> +	/*
>> +	 * We used to rely on having an exception return to get
>> +	 * an implicit isb. In the E2H case, we don't have it anymore.
>> +	 * rather than changing all the leaf functions, just do it here
>> +	 * before returning to the rest of the kernel.
>> +	 */
> 
> why is this not the case with an ISB before do_el2_call then?

That's a good point. I guess the safest thing to do would be to add one,
but looking at the various functions we call, I don't see any that could
go wrong by not having a ISB in their prologue.

Or maybe you've identified such a case?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1323234

FromCatalin Marinas <catalin.marinas@arm.com>
Date2016-02-01 16:40 +0100
Message-ID<qXoK5-4Sy-3@gated-at.bofh.it>
In reply to#1323118
On Mon, Feb 01, 2016 at 01:34:16PM +0000, Marc Zyngier wrote:
> On 01/02/16 13:16, Christoffer Dall wrote:
> > On Mon, Jan 25, 2016 at 03:53:40PM +0000, Marc Zyngier wrote:
> >> diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
> >> index 93e8d983..9e0683f 100644
> >> --- a/arch/arm64/kvm/hyp/hyp-entry.S
> >> +++ b/arch/arm64/kvm/hyp/hyp-entry.S
> >> @@ -38,6 +38,32 @@
> >>  	ldp	x0, x1, [sp], #16
> >>  .endm
> >>  
> >> +.macro do_el2_call
> >> +	/*
> >> +	 * Shuffle the parameters before calling the function
> >> +	 * pointed to in x0. Assumes parameters in x[1,2,3].
> >> +	 */
> >> +	stp	lr, xzr, [sp, #-16]!
> > 
> > remind me why this pair isn't just doing "str" instead of "stp" with the
> > xzr ?
> 
> Because SP has to be aligned on a 16 bytes boundary at all times.

You could do something like:

	sub	sp, sp, #16
	str	lr, [sp]

-- 
Catalin

[toc] | [prev] | [next] | [standalone]

#1323298

FromMarc Zyngier <marc.zyngier@arm.com>
Date2016-02-01 17:30 +0100
Message-ID<qXpwt-5xp-1@gated-at.bofh.it>
In reply to#1323234
On 01/02/16 15:36, Catalin Marinas wrote:
> On Mon, Feb 01, 2016 at 01:34:16PM +0000, Marc Zyngier wrote:
>> On 01/02/16 13:16, Christoffer Dall wrote:
>>> On Mon, Jan 25, 2016 at 03:53:40PM +0000, Marc Zyngier wrote:
>>>> diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
>>>> index 93e8d983..9e0683f 100644
>>>> --- a/arch/arm64/kvm/hyp/hyp-entry.S
>>>> +++ b/arch/arm64/kvm/hyp/hyp-entry.S
>>>> @@ -38,6 +38,32 @@
>>>>  	ldp	x0, x1, [sp], #16
>>>>  .endm
>>>>  
>>>> +.macro do_el2_call
>>>> +	/*
>>>> +	 * Shuffle the parameters before calling the function
>>>> +	 * pointed to in x0. Assumes parameters in x[1,2,3].
>>>> +	 */
>>>> +	stp	lr, xzr, [sp, #-16]!
>>>
>>> remind me why this pair isn't just doing "str" instead of "stp" with the
>>> xzr ?
>>
>> Because SP has to be aligned on a 16 bytes boundary at all times.
> 
> You could do something like:
> 
> 	sub	sp, sp, #16
> 	str	lr, [sp]
> 

Ah, fair enough. I'll fold that in.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1323324

FromArd Biesheuvel <ard.biesheuvel@linaro.org>
Date2016-02-01 18:10 +0100
Message-ID<qXq9b-62X-1@gated-at.bofh.it>
In reply to#1323298
On 1 February 2016 at 17:20, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 01/02/16 15:36, Catalin Marinas wrote:
>> On Mon, Feb 01, 2016 at 01:34:16PM +0000, Marc Zyngier wrote:
>>> On 01/02/16 13:16, Christoffer Dall wrote:
>>>> On Mon, Jan 25, 2016 at 03:53:40PM +0000, Marc Zyngier wrote:
>>>>> diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
>>>>> index 93e8d983..9e0683f 100644
>>>>> --- a/arch/arm64/kvm/hyp/hyp-entry.S
>>>>> +++ b/arch/arm64/kvm/hyp/hyp-entry.S
>>>>> @@ -38,6 +38,32 @@
>>>>>    ldp     x0, x1, [sp], #16
>>>>>  .endm
>>>>>
>>>>> +.macro do_el2_call
>>>>> +  /*
>>>>> +   * Shuffle the parameters before calling the function
>>>>> +   * pointed to in x0. Assumes parameters in x[1,2,3].
>>>>> +   */
>>>>> +  stp     lr, xzr, [sp, #-16]!
>>>>
>>>> remind me why this pair isn't just doing "str" instead of "stp" with the
>>>> xzr ?
>>>
>>> Because SP has to be aligned on a 16 bytes boundary at all times.
>>
>> You could do something like:
>>
>>       sub     sp, sp, #16
>>       str     lr, [sp]
>>
>
> Ah, fair enough. I'll fold that in.
>

Since we're micro-reviewing: what's wrong with

str lr, [sp, #-16]!

?

[toc] | [prev] | [next] | [standalone]

#1323335

FromMarc Zyngier <marc.zyngier@arm.com>
Date2016-02-01 18:30 +0100
Message-ID<qXqsx-6bL-1@gated-at.bofh.it>
In reply to#1323324
On 01/02/16 17:08, Ard Biesheuvel wrote:
> On 1 February 2016 at 17:20, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> On 01/02/16 15:36, Catalin Marinas wrote:
>>> On Mon, Feb 01, 2016 at 01:34:16PM +0000, Marc Zyngier wrote:
>>>> On 01/02/16 13:16, Christoffer Dall wrote:
>>>>> On Mon, Jan 25, 2016 at 03:53:40PM +0000, Marc Zyngier wrote:
>>>>>> diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
>>>>>> index 93e8d983..9e0683f 100644
>>>>>> --- a/arch/arm64/kvm/hyp/hyp-entry.S
>>>>>> +++ b/arch/arm64/kvm/hyp/hyp-entry.S
>>>>>> @@ -38,6 +38,32 @@
>>>>>>    ldp     x0, x1, [sp], #16
>>>>>>  .endm
>>>>>>
>>>>>> +.macro do_el2_call
>>>>>> +  /*
>>>>>> +   * Shuffle the parameters before calling the function
>>>>>> +   * pointed to in x0. Assumes parameters in x[1,2,3].
>>>>>> +   */
>>>>>> +  stp     lr, xzr, [sp, #-16]!
>>>>>
>>>>> remind me why this pair isn't just doing "str" instead of "stp" with the
>>>>> xzr ?
>>>>
>>>> Because SP has to be aligned on a 16 bytes boundary at all times.
>>>
>>> You could do something like:
>>>
>>>       sub     sp, sp, #16
>>>       str     lr, [sp]
>>>
>>
>> Ah, fair enough. I'll fold that in.
>>
> 
> Since we're micro-reviewing: what's wrong with
> 
> str lr, [sp, #-16]!
> 
> ?
> 

I suspect that on most micro-architectures, a register writeback is
going to be slower than doing the sub independently.

I may be wrong, though.

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1324214

FromChristoffer Dall <christoffer.dall@linaro.org>
Date2016-02-02 16:50 +0100
Message-ID<qXLnl-55B-35@gated-at.bofh.it>
In reply to#1323335
On Mon, Feb 01, 2016 at 05:28:32PM +0000, Marc Zyngier wrote:
> On 01/02/16 17:08, Ard Biesheuvel wrote:
> > On 1 February 2016 at 17:20, Marc Zyngier <marc.zyngier@arm.com> wrote:
> >> On 01/02/16 15:36, Catalin Marinas wrote:
> >>> On Mon, Feb 01, 2016 at 01:34:16PM +0000, Marc Zyngier wrote:
> >>>> On 01/02/16 13:16, Christoffer Dall wrote:
> >>>>> On Mon, Jan 25, 2016 at 03:53:40PM +0000, Marc Zyngier wrote:
> >>>>>> diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
> >>>>>> index 93e8d983..9e0683f 100644
> >>>>>> --- a/arch/arm64/kvm/hyp/hyp-entry.S
> >>>>>> +++ b/arch/arm64/kvm/hyp/hyp-entry.S
> >>>>>> @@ -38,6 +38,32 @@
> >>>>>>    ldp     x0, x1, [sp], #16
> >>>>>>  .endm
> >>>>>>
> >>>>>> +.macro do_el2_call
> >>>>>> +  /*
> >>>>>> +   * Shuffle the parameters before calling the function
> >>>>>> +   * pointed to in x0. Assumes parameters in x[1,2,3].
> >>>>>> +   */
> >>>>>> +  stp     lr, xzr, [sp, #-16]!
> >>>>>
> >>>>> remind me why this pair isn't just doing "str" instead of "stp" with the
> >>>>> xzr ?
> >>>>
> >>>> Because SP has to be aligned on a 16 bytes boundary at all times.
> >>>
> >>> You could do something like:
> >>>
> >>>       sub     sp, sp, #16
> >>>       str     lr, [sp]
> >>>
> >>
> >> Ah, fair enough. I'll fold that in.
> >>
> > 
> > Since we're micro-reviewing: what's wrong with
> > 
> > str lr, [sp, #-16]!
> > 
> > ?
> > 
> 
> I suspect that on most micro-architectures, a register writeback is
> going to be slower than doing the sub independently.
> 
> I may be wrong, though.
> 
For the record, I don't mind it the way it was in the original patch
either, I was just curious about the store of xzr and had forgottten the
stack alignment thing.

-Christoffer

[toc] | [prev] | [next] | [standalone]

#1323239

FromChristoffer Dall <christoffer.dall@linaro.org>
Date2016-02-01 16:40 +0100
Message-ID<qXoK7-4Sy-33@gated-at.bofh.it>
In reply to#1323118
On Mon, Feb 01, 2016 at 01:34:16PM +0000, Marc Zyngier wrote:
> On 01/02/16 13:16, Christoffer Dall wrote:
> > On Mon, Jan 25, 2016 at 03:53:40PM +0000, Marc Zyngier wrote:
> >> With VHE, the host never issues an HVC instruction to get into the
> >> KVM code, as we can simply branch there.
> >>
> >> Use runtime code patching to simplify things a bit.
> >>
> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >> ---
> >>  arch/arm64/kvm/hyp.S           |  7 +++++++
> >>  arch/arm64/kvm/hyp/hyp-entry.S | 38 +++++++++++++++++++++++++++++---------
> >>  2 files changed, 36 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
> >> index 0ccdcbb..0689a74 100644
> >> --- a/arch/arm64/kvm/hyp.S
> >> +++ b/arch/arm64/kvm/hyp.S
> >> @@ -17,7 +17,9 @@
> >>  
> >>  #include <linux/linkage.h>
> >>  
> >> +#include <asm/alternative.h>
> >>  #include <asm/assembler.h>
> >> +#include <asm/cpufeature.h>
> >>  
> >>  /*
> >>   * u64 kvm_call_hyp(void *hypfn, ...);
> >> @@ -38,6 +40,11 @@
> >>   * arch/arm64/kernel/hyp_stub.S.
> >>   */
> >>  ENTRY(kvm_call_hyp)
> >> +alternative_if_not ARM64_HAS_VIRT_HOST_EXTN	
> >>  	hvc	#0
> >>  	ret
> >> +alternative_else
> >> +	b	__vhe_hyp_call
> >> +	nop
> >> +alternative_endif
> >>  ENDPROC(kvm_call_hyp)
> >> diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
> >> index 93e8d983..9e0683f 100644
> >> --- a/arch/arm64/kvm/hyp/hyp-entry.S
> >> +++ b/arch/arm64/kvm/hyp/hyp-entry.S
> >> @@ -38,6 +38,32 @@
> >>  	ldp	x0, x1, [sp], #16
> >>  .endm
> >>  
> >> +.macro do_el2_call
> >> +	/*
> >> +	 * Shuffle the parameters before calling the function
> >> +	 * pointed to in x0. Assumes parameters in x[1,2,3].
> >> +	 */
> >> +	stp	lr, xzr, [sp, #-16]!
> > 
> > remind me why this pair isn't just doing "str" instead of "stp" with the
> > xzr ?
> 
> Because SP has to be aligned on a 16 bytes boundary at all times.
> 

right, duh.

> > 
> >> +	mov	lr, x0
> >> +	mov	x0, x1
> >> +	mov	x1, x2
> >> +	mov	x2, x3
> >> +	blr	lr
> >> +	ldp	lr, xzr, [sp], #16
> >> +.endm
> >> +
> >> +ENTRY(__vhe_hyp_call)
> >> +	do_el2_call
> >> +	/*
> >> +	 * We used to rely on having an exception return to get
> >> +	 * an implicit isb. In the E2H case, we don't have it anymore.
> >> +	 * rather than changing all the leaf functions, just do it here
> >> +	 * before returning to the rest of the kernel.
> >> +	 */
> > 
> > why is this not the case with an ISB before do_el2_call then?
> 
> That's a good point. I guess the safest thing to do would be to add one,
> but looking at the various functions we call, I don't see any that could
> go wrong by not having a ISB in their prologue.
> 
> Or maybe you've identified such a case?
> 
I think I argued on Mario's VFP patches that we could rely on an ISB
before the hyp call, but they're not merged yet, so, hey...

-Christoffer

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web