| Message ID | 20210305150638.2675513-13-npiggin@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | KVM: PPC: Book3S: C-ify the P9 entry/exit code | expand |
| Related | show |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/merge (91966823812efbd175f904599e5cf2a854b39809) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/next (fe07bfda2fb9cdef8a4d4008a409bb02f35f1bd8) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch linus/master (280d542f6ffac0e6d65dc267f92191d509b13b64) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/fixes (5c88a17e15795226b56d83f579cbb9b7a4864f79) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch linux-next (7a7fd0de4a9804299793e564a555a49c1fc924cb) |
| snowpatch_ozlabs/apply_patch | fail | Failed to apply to any branch |
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. > #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? > + 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? > + ld r10,PACA_EXGEN+EX_R10(r13) > Otherwise, this looks good to me so far. Kind regards, Daniel > .global kvmppc_interrupt > .balign IFETCH_ALIGN_BYTES > -- > 2.23.0
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
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 */ #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) + HMT_MEDIUM + mfcr r10 + std r12,HSTATE_SCRATCH0(r13) + sldi r12,r10,32 + ori r12,r12,0xc00 + ld r10,PACA_EXGEN+EX_R10(r13) .global kvmppc_interrupt .balign IFETCH_ALIGN_BYTES
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(-)