Re: [PATCH v3 12/41] KVM: PPC: Book3S 64: Move hcall early register setup to KVM

[Nicholas Piggin <npiggin@gmail.com>]

Excerpts from Daniel Axtens's message of March 12, 2021 3:45 pm:
> Nicholas Piggin <npiggin@gmail.com> writes:
> 
>> System calls / hcalls have a different calling convention than
>> other interrupts, so there is code in the KVMTEST to massage these
>> into the same form as other interrupt handlers.
>>
>> Move this work into the KVM hcall handler. This means teaching KVM
>> a little more about the low level interrupt handler setup, PACA save
>> areas, etc., although that's not obviously worse than the current
>> approach of coming up with an entirely different interrupt register
>> / save convention.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>  arch/powerpc/include/asm/exception-64s.h | 13 ++++++++
>>  arch/powerpc/kernel/exceptions-64s.S     | 42 +-----------------------
>>  arch/powerpc/kvm/book3s_64_entry.S       | 17 ++++++++++
>>  3 files changed, 31 insertions(+), 41 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/exception-64s.h b/arch/powerpc/include/asm/exception-64s.h
>> index c1a8aac01cf9..bb6f78fcf981 100644
>> --- a/arch/powerpc/include/asm/exception-64s.h
>> +++ b/arch/powerpc/include/asm/exception-64s.h
>> @@ -35,6 +35,19 @@
>>  /* PACA save area size in u64 units (exgen, exmc, etc) */
>>  #define EX_SIZE		10
>>  
>> +/* PACA save area offsets */
>> +#define EX_R9		0
>> +#define EX_R10		8
>> +#define EX_R11		16
>> +#define EX_R12		24
>> +#define EX_R13		32
>> +#define EX_DAR		40
>> +#define EX_DSISR	48
>> +#define EX_CCR		52
>> +#define EX_CFAR		56
>> +#define EX_PPR		64
>> +#define EX_CTR		72
>> +
>>  /*
>>   * maximum recursive depth of MCE exceptions
>>   */
>> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
>> index 292435bd80f0..b7092ba87da8 100644
>> --- a/arch/powerpc/kernel/exceptions-64s.S
>> +++ b/arch/powerpc/kernel/exceptions-64s.S
>> @@ -21,22 +21,6 @@
>>  #include <asm/feature-fixups.h>
>>  #include <asm/kup.h>
>>  
>> -/* PACA save area offsets (exgen, exmc, etc) */
>> -#define EX_R9		0
>> -#define EX_R10		8
>> -#define EX_R11		16
>> -#define EX_R12		24
>> -#define EX_R13		32
>> -#define EX_DAR		40
>> -#define EX_DSISR	48
>> -#define EX_CCR		52
>> -#define EX_CFAR		56
>> -#define EX_PPR		64
>> -#define EX_CTR		72
>> -.if EX_SIZE != 10
>> -	.error "EX_SIZE is wrong"
>> -.endif
>> -
>>  /*
>>   * Following are fixed section helper macros.
>>   *
>> @@ -1964,29 +1948,8 @@ EXC_VIRT_END(system_call, 0x4c00, 0x100)
>>  
>>  #ifdef CONFIG_KVM_BOOK3S_64_HANDLER
>>  TRAMP_REAL_BEGIN(system_call_kvm)
>> -	/*
>> -	 * This is a hcall, so register convention is as above, with these
>> -	 * differences:
>> -	 * r13 = PACA
>> -	 * ctr = orig r13
>> -	 * orig r10 saved in PACA
>> -	 */
>> -	 /*
>> -	  * Save the PPR (on systems that support it) before changing to
>> -	  * HMT_MEDIUM. That allows the KVM code to save that value into the
>> -	  * guest state (it is the guest's PPR value).
>> -	  */
>> -BEGIN_FTR_SECTION
>> -	mfspr	r10,SPRN_PPR
>> -	std	r10,HSTATE_PPR(r13)
>> -END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>> -	HMT_MEDIUM
>>  	mfctr	r10
>> -	SET_SCRATCH0(r10)
>> -	mfcr	r10
>> -	std	r12,HSTATE_SCRATCH0(r13)
>> -	sldi	r12,r10,32
>> -	ori	r12,r12,0xc00
>> +	SET_SCRATCH0(r10) /* Save r13 in SCRATCH0 */
> 
> If I've understood correctly, you've saved the _original_/guest r13 in
> SCRATCH0. That makes sense - it just took me a while to follow the
> logic, especially because the parameter to SET_SCRATCH0 is r10, not r13.
> 
> I would probably expand the comment to say the original or guest r13 (as
> you do in the comment at the start of kvmppc_hcall), but if there's a
> convention here that I've missed that might not be necessary.

There is a convention which is that all kvm interrupts including system
call come in with r13 saved in SCRATCH0, although that's all in a state
of flux throughput this series of course.

I added the comment because I moved the bigger comment here, I think 
that's okay because you're always referring to interrupted context 
(i.e., guest in this case) when talking about saved registers.

> 
>>  #ifdef CONFIG_RELOCATABLE
>>  	/*
>>  	 * Requires __LOAD_FAR_HANDLER beause kvmppc_hcall lives
>> @@ -1994,15 +1957,12 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>>  	 */
>>  	__LOAD_FAR_HANDLER(r10, kvmppc_hcall)
>>  	mtctr   r10
>> -	ld	r10,PACA_EXGEN+EX_R10(r13)
>>  	bctr
>>  #else
>> -	ld	r10,PACA_EXGEN+EX_R10(r13)
>>  	b       kvmppc_hcall
>>  #endif
>>  #endif
>>  
>> -
>>  /**
>>   * Interrupt 0xd00 - Trace Interrupt.
>>   * This is a synchronous interrupt in response to instruction step or
>> diff --git a/arch/powerpc/kvm/book3s_64_entry.S b/arch/powerpc/kvm/book3s_64_entry.S
>> index 8cf5e24a81eb..a7b6edd18bc8 100644
>> --- a/arch/powerpc/kvm/book3s_64_entry.S
>> +++ b/arch/powerpc/kvm/book3s_64_entry.S
>> @@ -14,6 +14,23 @@
>>  .global	kvmppc_hcall
>>  .balign IFETCH_ALIGN_BYTES
>>  kvmppc_hcall:
>> +	/*
>> +	 * This is a hcall, so register convention is as
>> +	 * Documentation/powerpc/papr_hcalls.rst, with these additions:
>> +	 * R13		= PACA
>> +	 * guest R13 saved in SPRN_SCRATCH0
>> +	 * R10		= free
>> +	 */
>> +BEGIN_FTR_SECTION
>> +	mfspr	r10,SPRN_PPR
>> +	std	r10,HSTATE_PPR(r13)
>> +END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> 
> Do we want to preserve the comment about why we save the PPR?

Wouldn't hurt. I think the reason the comment is there is because it's a 
difference with system calls. Hcalls preserve the PPR, system calls do not.

Should probably leave the "orig r10 saved in the PACA" comment too.

> 
>> +	HMT_MEDIUM
>> +	mfcr	r10
>> +	std	r12,HSTATE_SCRATCH0(r13)
>> +	sldi	r12,r10,32
>> +	ori	r12,r12,0xc00
> 
> I see that this is a direct copy from the earlier code, but it confuses
> me a bit. Looking at exceptions-64s.S, there's the following comment:
> 
>  * In HPT, sc 1 always goes to 0xc00 real mode. In RADIX, sc 1 can go to
>  * 0x4c00 virtual mode.
> 
> However, this code uncondionally sets the low bits to be c00, even if
> the exception came in via 4c00. Is this right? Do we need to pass
> that through somehow?

We don't need to. The "trap" numbers are always the real-mode vectors
(except scv which is a bit weird) by convention.

> 
>> +	ld	r10,PACA_EXGEN+EX_R10(r13)
>>
> 
> Otherwise, this looks good to me so far.

Thanks,
Nick