| Message ID | 20240627180342.110238-2-gautam@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix doorbell emulation for nested KVM guests in V1 API | expand |
On Fri Jun 28, 2024 at 4:03 AM AEST, Gautam Menghani wrote: > This reverts commit 7c3ded5735141ff4d049747c9f76672a8b737c49. > > On PowerNV, when a nested guest tries to use a feature prohibited by > HFSCR, the nested hypervisor (L1) should get a H_FAC_UNAVAILABLE trap > and then L1 can emulate the feature. But with the change introduced by > commit 7c3ded573514 ("KVM: PPC: Book3S HV Nested: Stop forwarding all HFUs to L1") > the L1 ends up getting a H_EMUL_ASSIST because of which, the L1 ends up > injecting a SIGILL when L2 (nested guest) tries to use doorbells. Yeah, we struggled to come up with a coherent story for this kind of compatibility and mismatched feature handling between L0 and L1. The L1 doorbell emulation shows a legitimate case the L1 wants to see the HFAC to emulate it and the L0 does not permit the L1 to set it for the L2. Actually the L0 could just permit it (even if the L0 wanted to emulate doorbells for the L1, it could still allow the L2 to run with doorbells if that's what the L1 asked for). That would also solve this problem, but there is a potential future hardware change where doorbells will be able to address any thread in the core even in "LPAR-per-thread" mode and the hypervisor *must* disable the doorbell HFSCR to the guest if it runs in KVM style that schedules LPARs on a per-thread basis instead of per-core. In that case the L0 must not permit the L2 to run with HFSCR set. So this approach actually works better there. In other cases where the L0 might deliberately prohibit some facility in a way that we don't want the L1 to see HFAC. I think we just cross that bridge when it comes. I'm sure the L0 would really need to advertise that to the L1 properly via device-tree or similar, and we could special case the HFAC->HEAI if necessary then. Reviewed-by: Nicholas Piggin <npiggin@gmail.com> > > Signed-off-by: Gautam Menghani <gautam@linux.ibm.com> > --- > arch/powerpc/kvm/book3s_hv.c | 31 ++----------------------------- > 1 file changed, 2 insertions(+), 29 deletions(-) > > diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c > index daaf7faf21a5..cea28ac05923 100644 > --- a/arch/powerpc/kvm/book3s_hv.c > +++ b/arch/powerpc/kvm/book3s_hv.c > @@ -2052,36 +2052,9 @@ static int kvmppc_handle_nested_exit(struct kvm_vcpu *vcpu) > fallthrough; /* go to facility unavailable handler */ > #endif > > - case BOOK3S_INTERRUPT_H_FAC_UNAVAIL: { > - u64 cause = vcpu->arch.hfscr >> 56; > - > - /* > - * Only pass HFU interrupts to the L1 if the facility is > - * permitted but disabled by the L1's HFSCR, otherwise > - * the interrupt does not make sense to the L1 so turn > - * it into a HEAI. > - */ > - if (!(vcpu->arch.hfscr_permitted & (1UL << cause)) || > - (vcpu->arch.nested_hfscr & (1UL << cause))) { > - ppc_inst_t pinst; > - vcpu->arch.trap = BOOK3S_INTERRUPT_H_EMUL_ASSIST; > - > - /* > - * If the fetch failed, return to guest and > - * try executing it again. > - */ > - r = kvmppc_get_last_inst(vcpu, INST_GENERIC, &pinst); > - vcpu->arch.emul_inst = ppc_inst_val(pinst); > - if (r != EMULATE_DONE) > - r = RESUME_GUEST; > - else > - r = RESUME_HOST; > - } else { > - r = RESUME_HOST; > - } > - > + case BOOK3S_INTERRUPT_H_FAC_UNAVAIL: > + r = RESUME_HOST; > break; > - } > > case BOOK3S_INTERRUPT_HV_RM_HARD: > vcpu->arch.trap = 0;
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c index daaf7faf21a5..cea28ac05923 100644 --- a/arch/powerpc/kvm/book3s_hv.c +++ b/arch/powerpc/kvm/book3s_hv.c @@ -2052,36 +2052,9 @@ static int kvmppc_handle_nested_exit(struct kvm_vcpu *vcpu) fallthrough; /* go to facility unavailable handler */ #endif - case BOOK3S_INTERRUPT_H_FAC_UNAVAIL: { - u64 cause = vcpu->arch.hfscr >> 56; - - /* - * Only pass HFU interrupts to the L1 if the facility is - * permitted but disabled by the L1's HFSCR, otherwise - * the interrupt does not make sense to the L1 so turn - * it into a HEAI. - */ - if (!(vcpu->arch.hfscr_permitted & (1UL << cause)) || - (vcpu->arch.nested_hfscr & (1UL << cause))) { - ppc_inst_t pinst; - vcpu->arch.trap = BOOK3S_INTERRUPT_H_EMUL_ASSIST; - - /* - * If the fetch failed, return to guest and - * try executing it again. - */ - r = kvmppc_get_last_inst(vcpu, INST_GENERIC, &pinst); - vcpu->arch.emul_inst = ppc_inst_val(pinst); - if (r != EMULATE_DONE) - r = RESUME_GUEST; - else - r = RESUME_HOST; - } else { - r = RESUME_HOST; - } - + case BOOK3S_INTERRUPT_H_FAC_UNAVAIL: + r = RESUME_HOST; break; - } case BOOK3S_INTERRUPT_HV_RM_HARD: vcpu->arch.trap = 0;
This reverts commit 7c3ded5735141ff4d049747c9f76672a8b737c49. On PowerNV, when a nested guest tries to use a feature prohibited by HFSCR, the nested hypervisor (L1) should get a H_FAC_UNAVAILABLE trap and then L1 can emulate the feature. But with the change introduced by commit 7c3ded573514 ("KVM: PPC: Book3S HV Nested: Stop forwarding all HFUs to L1") the L1 ends up getting a H_EMUL_ASSIST because of which, the L1 ends up injecting a SIGILL when L2 (nested guest) tries to use doorbells. Signed-off-by: Gautam Menghani <gautam@linux.ibm.com> --- arch/powerpc/kvm/book3s_hv.c | 31 ++----------------------------- 1 file changed, 2 insertions(+), 29 deletions(-)