From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nicholas Piggin Date: Thu, 04 Mar 2021 11:02:49 +0000 Subject: Re: [PATCH v2 28/37] KVM: PPC: Book3S HV P9: Add helpers for OS SPR handling Message-Id: <1614855554.y6ukh6cl5v.astroid@bobo.none> List-Id: References: <20210225134652.2127648-1-npiggin@gmail.com> <20210225134652.2127648-29-npiggin@gmail.com> <87pn0hwq9f.fsf@linux.ibm.com> In-Reply-To: <87pn0hwq9f.fsf@linux.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Fabiano Rosas , kvm-ppc@vger.kernel.org Cc: linuxppc-dev@lists.ozlabs.org Excerpts from Fabiano Rosas's message of March 3, 2021 1:04 am: > Nicholas Piggin writes: > >> This is a first step to wrapping supervisor and user SPR saving and >> loading up into helpers, which will then be called independently in >> bare metal and nested HV cases in order to optimise SPR access. >> >> Signed-off-by: Nicholas Piggin >> --- > > > >> +/* vcpu guest regs must already be saved */ >> +static void restore_p9_host_os_sprs(struct kvm_vcpu *vcpu, >> + struct p9_host_os_sprs *host_os_sprs) >> +{ >> + mtspr(SPRN_PSPB, 0); >> + mtspr(SPRN_WORT, 0); >> + mtspr(SPRN_UAMOR, 0); >> + mtspr(SPRN_PSPB, 0); > > Not your fault, but PSPB is set twice here. Yeah you're right. >> + >> + mtspr(SPRN_DSCR, host_os_sprs->dscr); >> + mtspr(SPRN_TIDR, host_os_sprs->tidr); >> + mtspr(SPRN_IAMR, host_os_sprs->iamr); >> + >> + if (host_os_sprs->amr != vcpu->arch.amr) >> + mtspr(SPRN_AMR, host_os_sprs->amr); >> + >> + if (host_os_sprs->fscr != vcpu->arch.fscr) >> + mtspr(SPRN_FSCR, host_os_sprs->fscr); >> +} >> + > > > >> @@ -3605,34 +3666,10 @@ static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit, >> vcpu->arch.dec_expires = dec + tb; >> vcpu->cpu = -1; >> vcpu->arch.thread_cpu = -1; >> - vcpu->arch.ctrl = mfspr(SPRN_CTRLF); >> - >> - vcpu->arch.iamr = mfspr(SPRN_IAMR); >> - vcpu->arch.pspb = mfspr(SPRN_PSPB); >> - vcpu->arch.fscr = mfspr(SPRN_FSCR); >> - vcpu->arch.tar = mfspr(SPRN_TAR); >> - vcpu->arch.ebbhr = mfspr(SPRN_EBBHR); >> - vcpu->arch.ebbrr = mfspr(SPRN_EBBRR); >> - vcpu->arch.bescr = mfspr(SPRN_BESCR); >> - vcpu->arch.wort = mfspr(SPRN_WORT); >> - vcpu->arch.tid = mfspr(SPRN_TIDR); >> - vcpu->arch.amr = mfspr(SPRN_AMR); >> - vcpu->arch.uamor = mfspr(SPRN_UAMOR); >> - vcpu->arch.dscr = mfspr(SPRN_DSCR); >> - >> - mtspr(SPRN_PSPB, 0); >> - mtspr(SPRN_WORT, 0); >> - mtspr(SPRN_UAMOR, 0); >> - mtspr(SPRN_DSCR, host_dscr); >> - mtspr(SPRN_TIDR, host_tidr); >> - mtspr(SPRN_IAMR, host_iamr); >> - mtspr(SPRN_PSPB, 0); >> >> - if (host_amr != vcpu->arch.amr) >> - mtspr(SPRN_AMR, host_amr); >> + restore_p9_host_os_sprs(vcpu, &host_os_sprs); >> >> - if (host_fscr != vcpu->arch.fscr) >> - mtspr(SPRN_FSCR, host_fscr); >> + store_spr_state(vcpu); > > store_spr_state should come first, right? We want to save the guest > state before restoring the host state. Yes good catch. I switched that back around later but looks like I never brought the fix back to the right patch. Interestingly, things pretty much work like this if the guest or host doesn't do anything much with the SPRs! Thanks, Nick