Excerpts from Fabiano Rosas's message of March 3, 2021 1:04 am: > Nicholas Piggin <npiggin@xxxxxxxxx> 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 <npiggin@xxxxxxxxx> >> --- > > <snip> > >> +/* 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); >> +} >> + > > <snip> > >> @@ -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