[PATCH 0/2] Improving calls to kvmppc_hv_entry

[PATCH 0/2] Improving calls to kvmppc_hv_entry

[Kautuk Consul]

- remove .global scope of kvmppc_hv_entry
- remove r4 argument to kvmppc_hv_entry as it is not required

Kautuk Consul (2):
  arch/powerpc/kvm: kvmppc_hv_entry: remove .global scope
  arch/powerpc/kvm: kvmppc_hv_entry: remove r4 argument

 arch/powerpc/kvm/book3s_hv_rmhandlers.S | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

-- 
2.31.1

[PATCH 1/2] arch/powerpc/kvm: kvmppc_hv_entry: remove .global scope

[Kautuk Consul]

kvmppc_hv_entry isn't called from anywhere other than
book3s_hv_rmhandlers.S itself. Removing .global scope for
this function.

Signed-off-by: Kautuk Consul <kconsul@linux.vnet.ibm.com>
---
 arch/powerpc/kvm/book3s_hv_rmhandlers.S | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
index acf80915f406..7e063fde7adc 100644
--- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
+++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
@@ -502,7 +502,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
  *                                                                            *
  *****************************************************************************/
 
-.global kvmppc_hv_entry
 kvmppc_hv_entry:
 
 	/* Required state:
-- 
2.31.1

[PATCH 2/2] arch/powerpc/kvm: kvmppc_hv_entry: remove r4 argument

[Kautuk Consul]

kvmppc_hv_entry is called from only 2 locations within
book3s_hv_rmhandlers.S. Both of those locations set r4
as HSTATE_KVM_VCPU(r13) before calling kvmppc_hv_entry.
So, shift the r4 load instruction to kvmppc_hv_entry and
thus modify the calling convention of this function.

Signed-off-by: Kautuk Consul <kconsul@linux.vnet.ibm.com>
---
 arch/powerpc/kvm/book3s_hv_rmhandlers.S | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
index 7e063fde7adc..922667b09168 100644
--- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
+++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
@@ -85,7 +85,7 @@ _GLOBAL_TOC(kvmppc_hv_entry_trampoline)
 	RFI_TO_KERNEL
 
 kvmppc_call_hv_entry:
-	ld	r4, HSTATE_KVM_VCPU(r13)
+	/* Enter guest. */
 	bl	kvmppc_hv_entry
 
 	/* Back from guest - restore host state and return to caller */
@@ -352,9 +352,7 @@ kvm_secondary_got_guest:
 	mtspr	SPRN_LDBAR, r0
 	isync
 63:
-	/* Order load of vcpu after load of vcore */
-	lwsync
-	ld	r4, HSTATE_KVM_VCPU(r13)
+	/* Enter guest. */
 	bl	kvmppc_hv_entry
 
 	/* Back from the guest, go back to nap */
@@ -506,7 +504,6 @@ kvmppc_hv_entry:
 
 	/* Required state:
 	 *
-	 * R4 = vcpu pointer (or NULL)
 	 * MSR = ~IR|DR
 	 * R13 = PACA
 	 * R1 = host R1
@@ -524,6 +521,8 @@ kvmppc_hv_entry:
 	li	r6, KVM_GUEST_MODE_HOST_HV
 	stb	r6, HSTATE_IN_GUEST(r13)
 
+	ld	r4, HSTATE_KVM_VCPU(r13)
+
 #ifdef CONFIG_KVM_BOOK3S_HV_P8_TIMING
 	/* Store initial timestamp */
 	cmpdi	r4, 0
-- 
2.31.1

Re: [PATCH 1/2] arch/powerpc/kvm: kvmppc_hv_entry: remove .global scope

[Sathvika Vasireddy]

Hi Kautuk,

On 20/02/23 10:53, Kautuk Consul wrote:
> kvmppc_hv_entry isn't called from anywhere other than
> book3s_hv_rmhandlers.S itself. Removing .global scope for
> this function.
>
> Signed-off-by: Kautuk Consul <kconsul@linux.vnet.ibm.com>
> ---
>   arch/powerpc/kvm/book3s_hv_rmhandlers.S | 1 -
>   1 file changed, 1 deletion(-)
>
> diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> index acf80915f406..7e063fde7adc 100644
> --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> @@ -502,7 +502,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
>    *                                                                            *
>    *****************************************************************************/
>   
> -.global kvmppc_hv_entry
>   kvmppc_hv_entry:
>   
>   	/* Required state:
I see the following objtool warning with this patch applied.
arch/powerpc/kvm/book3s_hv_rmhandlers.o: warning: objtool: .text+0x48: 
unannotated intra-function call

Annotating kvmppc_hv_entry symbol with SYM_FUNC_START_LOCAL and 
SYM_FUNC_END macros should help fix this warning.

Thanks,
Sathvika

Re: [PATCH 1/2] arch/powerpc/kvm: kvmppc_hv_entry: remove .global scope

[Sathvika Vasireddy]

Hi Kautuk,

On 20/02/23 10:53, Kautuk Consul wrote:
> kvmppc_hv_entry isn't called from anywhere other than
> book3s_hv_rmhandlers.S itself. Removing .global scope for
> this function.
>
> Signed-off-by: Kautuk Consul <kconsul@linux.vnet.ibm.com>
> ---
>   arch/powerpc/kvm/book3s_hv_rmhandlers.S | 1 -
>   1 file changed, 1 deletion(-)
>
> diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> index acf80915f406..7e063fde7adc 100644
> --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> @@ -502,7 +502,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
>    *                                                                            *
>    *****************************************************************************/
>   
> -.global kvmppc_hv_entry
>   kvmppc_hv_entry:
>   
>   	/* Required state:
I see the following objtool warning with this patch applied.
arch/powerpc/kvm/book3s_hv_rmhandlers.o: warning: objtool: .text+0x48: 
unannotated intra-function call

Annotating kvmppc_hv_entry symbol with SYM_FUNC_START_LOCAL and 
SYM_FUNC_END macros should help fix this warning.

Thanks,
Sathvika

Re: [PATCH 1/2] arch/powerpc/kvm: kvmppc_hv_entry: remove .global scope

[Kautuk Consul]

Hi Sathvika,

(Sorry didn't include list in earlier email.)

On Mon, Feb 20, 2023 at 12:35:09PM +0530, Sathvika Vasireddy wrote:
> Hi Kautuk,
> 
> On 20/02/23 10:53, Kautuk Consul wrote:
> > kvmppc_hv_entry isn't called from anywhere other than
> > book3s_hv_rmhandlers.S itself. Removing .global scope for
> > this function.
> > 
> > Signed-off-by: Kautuk Consul <kconsul@linux.vnet.ibm.com>
> > ---
> >   arch/powerpc/kvm/book3s_hv_rmhandlers.S | 1 -
> >   1 file changed, 1 deletion(-)
> > 
> > diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> > index acf80915f406..7e063fde7adc 100644
> > --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> > +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> > @@ -502,7 +502,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
> >    *                                                                            *
> >    *****************************************************************************/
> > -.global kvmppc_hv_entry
> >   kvmppc_hv_entry:
> >   	/* Required state:
> I see the following objtool warning with this patch applied.
> arch/powerpc/kvm/book3s_hv_rmhandlers.o: warning: objtool: .text+0x48:
> unannotated intra-function call
> 
> Annotating kvmppc_hv_entry symbol with SYM_FUNC_START_LOCAL and SYM_FUNC_END
> macros should help fix this warning.

Not sure where to put the SYM_FUNC_END annotation.
Will the following do:
<snip>
    ld  r0, VCPU_GPR(R0)(r4)
    ld  r2, VCPU_GPR(R2)(r4)
    ld  r3, VCPU_GPR(R3)(r4)
    ld  r4, VCPU_GPR(R4)(r4)
    HRFI_TO_GUEST
    b   .

SYM_FUNC_END(kvmppc_hv_entry)

secondary_too_late:
    li  r12, 0

?

Thanks.

> 
> Thanks,
> Sathvika
> 
> 

Re: [PATCH 1/2] arch/powerpc/kvm: kvmppc_hv_entry: remove .global scope

[Sathvika Vasireddy]

On 20/02/23 12:58, Kautuk Consul wrote:
> Hi Sathvika,
>
> (Sorry didn't include list in earlier email.)
>
> On Mon, Feb 20, 2023 at 12:35:09PM +0530, Sathvika Vasireddy wrote:
>> Hi Kautuk,
>>
>> On 20/02/23 10:53, Kautuk Consul wrote:
>>> kvmppc_hv_entry isn't called from anywhere other than
>>> book3s_hv_rmhandlers.S itself. Removing .global scope for
>>> this function.
>>>
>>> Signed-off-by: Kautuk Consul <kconsul@linux.vnet.ibm.com>
>>> ---
>>>    arch/powerpc/kvm/book3s_hv_rmhandlers.S | 1 -
>>>    1 file changed, 1 deletion(-)
>>>
>>> diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
>>> index acf80915f406..7e063fde7adc 100644
>>> --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
>>> +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
>>> @@ -502,7 +502,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
>>>     *                                                                            *
>>>     *****************************************************************************/
>>> -.global kvmppc_hv_entry
>>>    kvmppc_hv_entry:
>>>    	/* Required state:
>> I see the following objtool warning with this patch applied.
>> arch/powerpc/kvm/book3s_hv_rmhandlers.o: warning: objtool: .text+0x48:
>> unannotated intra-function call
>>
>> Annotating kvmppc_hv_entry symbol with SYM_FUNC_START_LOCAL and SYM_FUNC_END
>> macros should help fix this warning.
> Not sure where to put the SYM_FUNC_END annotation.
> Will the following do:
> <snip>
>      ld  r0, VCPU_GPR(R0)(r4)
>      ld  r2, VCPU_GPR(R2)(r4)
>      ld  r3, VCPU_GPR(R3)(r4)
>      ld  r4, VCPU_GPR(R4)(r4)
>      HRFI_TO_GUEST
>      b   .
>
> SYM_FUNC_END(kvmppc_hv_entry)
>
> secondary_too_late:
>      li  r12, 0
>
> ?
>
> Thanks.

Placing SYM_FUNC_END(kvmppc_hv_entry) before kvmppc_got_guest() should do:

@@ -502,12 +500,10 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
* *
*****************************************************************************/

-.global kvmppc_hv_entry
-kvmppc_hv_entry:
+SYM_FUNC_START_LOCAL(kvmppc_hv_entry)

         /* Required state:
          *
-        * R4 = vcpu pointer (or NULL)
          * MSR = ~IR|DR
          * R13 = PACA
          * R1 = host R1
@@ -525,6 +521,8 @@ kvmppc_hv_entry:
         li      r6, KVM_GUEST_MODE_HOST_HV
         stb     r6, HSTATE_IN_GUEST(r13)

+       ld      r4, HSTATE_KVM_VCPU(r13)
+
  #ifdef CONFIG_KVM_BOOK3S_HV_P8_TIMING
         /* Store initial timestamp */
         cmpdi   r4, 0
@@ -619,6 +617,8 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
         /* Do we have a guest vcpu to run? */
  10:    cmpdi   r4, 0
         beq     kvmppc_primary_no_guest
+SYM_FUNC_END(kvmppc_hv_entry)
+
  kvmppc_got_guest:
         /* Increment yield count if they have a VPA */
         ld      r3, VCPU_VPA(r4)


Thanks,
Sathvika

Re: [PATCH 1/2] arch/powerpc/kvm: kvmppc_hv_entry: remove .global scope

[Kautuk Consul]

On Mon, Feb 20, 2023 at 01:31:40PM +0530, Sathvika Vasireddy wrote:
> Placing SYM_FUNC_END(kvmppc_hv_entry) before kvmppc_got_guest() should do:
> 
> @@ -502,12 +500,10 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
> * *
> *****************************************************************************/
> 
> -.global kvmppc_hv_entry
> -kvmppc_hv_entry:
> +SYM_FUNC_START_LOCAL(kvmppc_hv_entry)
> 
>         /* Required state:
>          *
> -        * R4 = vcpu pointer (or NULL)
>          * MSR = ~IR|DR
>          * R13 = PACA
>          * R1 = host R1
> @@ -525,6 +521,8 @@ kvmppc_hv_entry:
>         li      r6, KVM_GUEST_MODE_HOST_HV
>         stb     r6, HSTATE_IN_GUEST(r13)
> 
> +       ld      r4, HSTATE_KVM_VCPU(r13)
> +
>  #ifdef CONFIG_KVM_BOOK3S_HV_P8_TIMING
>         /* Store initial timestamp */
>         cmpdi   r4, 0
> @@ -619,6 +617,8 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
>         /* Do we have a guest vcpu to run? */
>  10:    cmpdi   r4, 0
>         beq     kvmppc_primary_no_guest
> +SYM_FUNC_END(kvmppc_hv_entry)
> +
>  kvmppc_got_guest:
>         /* Increment yield count if they have a VPA */
>         ld      r3, VCPU_VPA(r4)
> 

Thanks! Will send out a v2 after I get some response for
PATCH 2/2 with comments.
> 
> Thanks,
> Sathvika

Re: [PATCH 1/2] arch/powerpc/kvm: kvmppc_hv_entry: remove .global scope

[Kautuk Consul]

On Mon, Feb 20, 2023 at 01:41:38PM +0530, Kautuk Consul wrote:
> On Mon, Feb 20, 2023 at 01:31:40PM +0530, Sathvika Vasireddy wrote:
> > Placing SYM_FUNC_END(kvmppc_hv_entry) before kvmppc_got_guest() should do:
> > 
> > @@ -502,12 +500,10 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
> > * *
> > *****************************************************************************/
> > 
> > -.global kvmppc_hv_entry
> > -kvmppc_hv_entry:
> > +SYM_FUNC_START_LOCAL(kvmppc_hv_entry)
> > 
> >         /* Required state:
> >          *
> > -        * R4 = vcpu pointer (or NULL)
> >          * MSR = ~IR|DR
> >          * R13 = PACA
> >          * R1 = host R1
> > @@ -525,6 +521,8 @@ kvmppc_hv_entry:
> >         li      r6, KVM_GUEST_MODE_HOST_HV
> >         stb     r6, HSTATE_IN_GUEST(r13)
> > 
> > +       ld      r4, HSTATE_KVM_VCPU(r13)
> > +
> >  #ifdef CONFIG_KVM_BOOK3S_HV_P8_TIMING
> >         /* Store initial timestamp */
> >         cmpdi   r4, 0
> > @@ -619,6 +617,8 @@ END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
> >         /* Do we have a guest vcpu to run? */
> >  10:    cmpdi   r4, 0
> >         beq     kvmppc_primary_no_guest
> > +SYM_FUNC_END(kvmppc_hv_entry)

Just one question though. Went through the code again and I think
that this place shouldn't be proper to insert a SYM_FUNC_END
because we haven't entered the guest at this point and the name
of the function is kvmppc_hv_entry which  I think implies that
this SYM_FUNC_END should be at some place after the HRFI_TO_GUEST.

What do you think ?

> > +
> >  kvmppc_got_guest:
> >         /* Increment yield count if they have a VPA */
> >         ld      r3, VCPU_VPA(r4)
> > 
> 
> Thanks! Will send out a v2 after I get some response for
> PATCH 2/2 with comments.
> > 
> > Thanks,
> > Sathvika

Re: [PATCH 1/2] arch/powerpc/kvm: kvmppc_hv_entry: remove .global scope

[Kautuk Consul]

Hi Sathvika,
> 
> Just one question though. Went through the code again and I think
> that this place shouldn't be proper to insert a SYM_FUNC_END
> because we haven't entered the guest at this point and the name
> of the function is kvmppc_hv_entry which  I think implies that
> this SYM_FUNC_END should be at some place after the HRFI_TO_GUEST.
> 
> What do you think ?
Any updates on this ? Is there any other way to avoid this warning ?

Re: [PATCH 1/2] arch/powerpc/kvm: kvmppc_hv_entry: remove .global scope

[Sathvika Vasireddy]

On 23/02/23 10:39, Kautuk Consul wrote:

> Hi Sathvika,
>> Just one question though. Went through the code again and I think
>> that this place shouldn't be proper to insert a SYM_FUNC_END
>> because we haven't entered the guest at this point and the name
>> of the function is kvmppc_hv_entry which  I think implies that
>> this SYM_FUNC_END should be at some place after the HRFI_TO_GUEST.
>>
>> What do you think ?
> Any updates on this ? Is there any other way to avoid this warning ?
Hmm, to mark the end of the kvmppc_hv_entry function, I think 
SYM_FUNC_END(kvmppc_hv_entry) should be placed before the next symbol, 
which is kvmppc_got_guest() in this case.

However, if you think it needs to be put at a different place, then it 
does not make sense to have any other symbols before that. You may want 
to consider checking if other macros like SYM_INNER_LABEL() can be used.

- Sathvika

Re: [PATCH 1/2] arch/powerpc/kvm: kvmppc_hv_entry: remove .global scope

[Kautuk Consul]

On 2023-02-24 16:45:45, Sathvika Vasireddy wrote:
> On 23/02/23 10:39, Kautuk Consul wrote:
> 
> > Hi Sathvika,
> > > Just one question though. Went through the code again and I think
> > > that this place shouldn't be proper to insert a SYM_FUNC_END
> > > because we haven't entered the guest at this point and the name
> > > of the function is kvmppc_hv_entry which  I think implies that
> > > this SYM_FUNC_END should be at some place after the HRFI_TO_GUEST.
> > > 
> > > What do you think ?
> > Any updates on this ? Is there any other way to avoid this warning ?
> Hmm, to mark the end of the kvmppc_hv_entry function, I think
> SYM_FUNC_END(kvmppc_hv_entry) should be placed before the next symbol, which
> is kvmppc_got_guest() in this case.
> 
> However, if you think it needs to be put at a different place, then it does
> not make sense to have any other symbols before that. You may want to
> consider checking if other macros like SYM_INNER_LABEL() can be used.

SYM_INNER_LABEL works fine for me. I will post a v2 with this change.

Thanks! :-)
> 
> - Sathvika
> 

Re: [PATCH 2/2] arch/powerpc/kvm: kvmppc_hv_entry: remove r4 argument

[Kautuk Consul]

Hi,

On 2023-02-20 10:53:55, Kautuk Consul wrote:
> kvmppc_hv_entry is called from only 2 locations within
> book3s_hv_rmhandlers.S. Both of those locations set r4
> as HSTATE_KVM_VCPU(r13) before calling kvmppc_hv_entry.
> So, shift the r4 load instruction to kvmppc_hv_entry and
> thus modify the calling convention of this function.
> 

I am posting v2 of this patch-set now.
I have tested this on POWER8 and it works fine.
Can anyone review the v2 for this patch ? 
I didn't receive any review comments for this patch.

> Signed-off-by: Kautuk Consul <kconsul@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kvm/book3s_hv_rmhandlers.S | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> index 7e063fde7adc..922667b09168 100644
> --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> @@ -85,7 +85,7 @@ _GLOBAL_TOC(kvmppc_hv_entry_trampoline)
>  	RFI_TO_KERNEL
> 
>  kvmppc_call_hv_entry:
> -	ld	r4, HSTATE_KVM_VCPU(r13)
> +	/* Enter guest. */
>  	bl	kvmppc_hv_entry
> 
>  	/* Back from guest - restore host state and return to caller */
> @@ -352,9 +352,7 @@ kvm_secondary_got_guest:
>  	mtspr	SPRN_LDBAR, r0
>  	isync
>  63:
> -	/* Order load of vcpu after load of vcore */
> -	lwsync
> -	ld	r4, HSTATE_KVM_VCPU(r13)
> +	/* Enter guest. */
>  	bl	kvmppc_hv_entry
> 
>  	/* Back from the guest, go back to nap */
> @@ -506,7 +504,6 @@ kvmppc_hv_entry:
> 
>  	/* Required state:
>  	 *
> -	 * R4 = vcpu pointer (or NULL)
>  	 * MSR = ~IR|DR
>  	 * R13 = PACA
>  	 * R1 = host R1
> @@ -524,6 +521,8 @@ kvmppc_hv_entry:
>  	li	r6, KVM_GUEST_MODE_HOST_HV
>  	stb	r6, HSTATE_IN_GUEST(r13)
> 
> +	ld	r4, HSTATE_KVM_VCPU(r13)
> +
>  #ifdef CONFIG_KVM_BOOK3S_HV_P8_TIMING
>  	/* Store initial timestamp */
>  	cmpdi	r4, 0
> -- 
> 2.31.1
>