Excerpts from Daniel Axtens's message of March 12, 2021 3:45 pm:
> Nicholas Piggin <npig...@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 <npig...@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

Reply via email to