[PATCH v3 12/41] KVM: PPC: Book3S 64: Move hcall early register setup to KVM
Nicholas Piggin
npiggin at gmail.com
Tue Mar 16 14:43:40 AEDT 2021
Excerpts from Daniel Axtens's message of March 12, 2021 3:45 pm:
> Nicholas Piggin <npiggin at 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 at 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
More information about the Linuxppc-dev
mailing list