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