| Message ID | 20210726201710.2432874-3-farosas@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: PPC: Book3S HV: Nested guest state sanitising changes | expand |
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 <farosas@linux.ibm.com> > Reviewed-by: Nicholas Piggin <npiggin@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? > + > + 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? > + > + /* Don't leak the cause field */ > + hr->hfscr &= ~HFSCR_INTR_CAUSE; This hunk also remains -- shouldn't change HFSCR for HEA, only HFAC. Thanks, Nick
Nicholas Piggin <npiggin@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 <farosas@linux.ibm.com> >> Reviewed-by: Nicholas Piggin <npiggin@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
Excerpts from Fabiano Rosas's message of July 28, 2021 12:36 am: > Nicholas Piggin <npiggin@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 <farosas@linux.ibm.com> >>> Reviewed-by: Nicholas Piggin <npiggin@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. Yeah I think it would be better to move this logic to the nested exit handler. Thanks, Nick
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; + + 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); + + /* Don't leak the cause field */ + hr->hfscr &= ~HFSCR_INTR_CAUSE; + + fallthrough; + } case BOOK3S_INTERRUPT_H_EMUL_ASSIST: hr->heir = vcpu->arch.emul_inst; break; @@ -368,7 +388,7 @@ long kvmhv_enter_nested_guest(struct kvm_vcpu *vcpu) delta_spurr = vcpu->arch.spurr - l2_hv.spurr; delta_ic = vcpu->arch.ic - l2_hv.ic; delta_vtb = vc->vtb - l2_hv.vtb; - save_hv_return_state(vcpu, vcpu->arch.trap, &l2_hv); + save_hv_return_state(vcpu, &l2_hv); /* restore L1 state */ vcpu->arch.nested = NULL;