Nicholas Piggin <npig...@gmail.com> writes: > Excerpts from Fabiano Rosas's message of July 27, 2021 6:17 am: >> If the nested hypervisor has no access to a facility because it has >> been disabled by the host, it should also not be able to see the >> Hypervisor Facility Unavailable that arises from one of its guests >> trying to access the facility. >> >> This patch turns a HFU that happened in L2 into a Hypervisor Emulation >> Assistance interrupt and forwards it to L1 for handling. The ones that >> happened because L1 explicitly disabled the facility for L2 are still >> let through, along with the corresponding Cause bits in the HFSCR. >> >> Signed-off-by: Fabiano Rosas <faro...@linux.ibm.com> >> Reviewed-by: Nicholas Piggin <npig...@gmail.com> >> --- >> arch/powerpc/kvm/book3s_hv_nested.c | 32 +++++++++++++++++++++++------ >> 1 file changed, 26 insertions(+), 6 deletions(-) >> >> diff --git a/arch/powerpc/kvm/book3s_hv_nested.c >> b/arch/powerpc/kvm/book3s_hv_nested.c >> index 8215dbd4be9a..d544b092b49a 100644 >> --- a/arch/powerpc/kvm/book3s_hv_nested.c >> +++ b/arch/powerpc/kvm/book3s_hv_nested.c >> @@ -99,7 +99,7 @@ static void byteswap_hv_regs(struct hv_guest_state *hr) >> hr->dawrx1 = swab64(hr->dawrx1); >> } >> >> -static void save_hv_return_state(struct kvm_vcpu *vcpu, int trap, >> +static void save_hv_return_state(struct kvm_vcpu *vcpu, >> struct hv_guest_state *hr) >> { >> struct kvmppc_vcore *vc = vcpu->arch.vcore; >> @@ -118,7 +118,7 @@ static void save_hv_return_state(struct kvm_vcpu *vcpu, >> int trap, >> hr->pidr = vcpu->arch.pid; >> hr->cfar = vcpu->arch.cfar; >> hr->ppr = vcpu->arch.ppr; >> - switch (trap) { >> + switch (vcpu->arch.trap) { >> case BOOK3S_INTERRUPT_H_DATA_STORAGE: >> hr->hdar = vcpu->arch.fault_dar; >> hr->hdsisr = vcpu->arch.fault_dsisr; >> @@ -128,9 +128,29 @@ static void save_hv_return_state(struct kvm_vcpu *vcpu, >> int trap, >> hr->asdr = vcpu->arch.fault_gpa; >> break; >> case BOOK3S_INTERRUPT_H_FAC_UNAVAIL: >> - hr->hfscr = ((~HFSCR_INTR_CAUSE & hr->hfscr) | >> - (HFSCR_INTR_CAUSE & vcpu->arch.hfscr)); >> - break; >> + { >> + u8 cause = vcpu->arch.hfscr >> 56; > > Can this be u64 just to help gcc? >
Yes. >> + >> + WARN_ON_ONCE(cause >= BITS_PER_LONG); >> + >> + if (!(hr->hfscr & (1UL << cause))) { >> + hr->hfscr = ((~HFSCR_INTR_CAUSE & hr->hfscr) | >> + (HFSCR_INTR_CAUSE & vcpu->arch.hfscr)); >> + break; >> + } >> + >> + /* >> + * We have disabled this facility, so it does not >> + * exist from L1's perspective. Turn it into a HEAI. >> + */ >> + vcpu->arch.trap = BOOK3S_INTERRUPT_H_EMUL_ASSIST; >> + kvmppc_load_last_inst(vcpu, INST_GENERIC, >> &vcpu->arch.emul_inst); > > Hmm, this doesn't handle kvmpc_load_last_inst failure. Other code tends > to just resume guest and retry in this case. Can we do that here? > Not at this point. The other code does that inside kvmppc_handle_exit_hv, which is called from kvmhv_run_single_vcpu. And since we're changing the interrupt, I cannot load the last instruction at kvmppc_handle_nested_exit because at that point this is still an HFU. Unless I do it anyway at the HFU handler and put a comment explaining the situation. Or I could check for failure and clear vcpu->arch.emul_inst and therefore also hr->heir if we couldn't load the instruction. >> + >> + /* Don't leak the cause field */ >> + hr->hfscr &= ~HFSCR_INTR_CAUSE; > > This hunk also remains -- shouldn't change HFSCR for HEA, only HFAC. Ah of course, thanks. > > Thanks, > Nick