| Message ID | 20220317172049.2681740-2-farosas@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spapr: Nested TCG is TCG only | expand |
On Thu, Mar 17, 2022 at 02:20:47PM -0300, Fabiano Rosas wrote: > It is possible that nested KVM hypercalls reach QEMU while we're > running KVM. The spapr virtual hypervisor implementation of the nested > KVM API only works when the L1 is running under TCG. So return > H_FUNCTION if we are under KVM. > > Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> > --- > hw/ppc/spapr_hcall.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > index f008290787..119baa1d2d 100644 > --- a/hw/ppc/spapr_hcall.c > +++ b/hw/ppc/spapr_hcall.c > @@ -1508,7 +1508,7 @@ static target_ulong h_set_ptbl(PowerPCCPU *cpu, > { > target_ulong ptcr = args[0]; > > - if (!spapr_get_cap(spapr, SPAPR_CAP_NESTED_KVM_HV)) { > + if (!spapr_get_cap(spapr, SPAPR_CAP_NESTED_KVM_HV) || !tcg_enabled()) { I was about to nack this on the grounds that it changes guest visible behaviour based on host properties. Then I realized that's not the case, because in the KVM + SPAPR_CAP_NESTED_KVM_HV case the hypercall should be caught by KVM first and never reach here. So at the very least I think this needs a comment explaining that. However, I'm still kind of confused how we would get here in the first place. If SPAPR_CAP_NESTED_KVM_HV is set, but KVM doesn't support it, we should fail outright in cap_nested_kvm_hv_apply(). So how *do* we get here? Is the kernel not doing what we expect of it? If so, we should probably abort, rather than just returning H_FUNCTION. > return H_FUNCTION; > } > > @@ -1532,6 +1532,10 @@ static target_ulong h_tlb_invalidate(PowerPCCPU *cpu, > * across L1<->L2 transitions, so nothing is required here. > */ > > + if (!tcg_enabled()) { > + return H_FUNCTION; > + } > + > return H_SUCCESS; > } > > @@ -1572,6 +1576,10 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu, > uint64_t cr; > int i; > > + if (!tcg_enabled()) { > + return H_FUNCTION; > + } > + > if (spapr->nested_ptcr == 0) { > return H_NOT_AVAILABLE; > }
David Gibson <david@gibson.dropbear.id.au> writes: > On Thu, Mar 17, 2022 at 02:20:47PM -0300, Fabiano Rosas wrote: >> It is possible that nested KVM hypercalls reach QEMU while we're >> running KVM. The spapr virtual hypervisor implementation of the nested >> KVM API only works when the L1 is running under TCG. So return >> H_FUNCTION if we are under KVM. >> >> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> >> --- >> hw/ppc/spapr_hcall.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c >> index f008290787..119baa1d2d 100644 >> --- a/hw/ppc/spapr_hcall.c >> +++ b/hw/ppc/spapr_hcall.c >> @@ -1508,7 +1508,7 @@ static target_ulong h_set_ptbl(PowerPCCPU *cpu, >> { >> target_ulong ptcr = args[0]; >> >> - if (!spapr_get_cap(spapr, SPAPR_CAP_NESTED_KVM_HV)) { >> + if (!spapr_get_cap(spapr, SPAPR_CAP_NESTED_KVM_HV) || !tcg_enabled()) { > > I was about to nack this on the grounds that it changes guest visible > behaviour based on host properties. Then I realized that's not the > case, because in the KVM + SPAPR_CAP_NESTED_KVM_HV case the hypercall > should be caught by KVM first and never reach here. > > So at the very least I think this needs a comment explaining that. Ok. > However, I'm still kind of confused how we would get here in the first > place. If SPAPR_CAP_NESTED_KVM_HV is set, but KVM doesn't support it, > we should fail outright in cap_nested_kvm_hv_apply(). So how *do* we > get here? Is the kernel not doing what we expect of it? If so, we > should probably abort, rather than just returning H_FUNCTION. Indeed, If all parts are functioning this should never happen. I was hacking in L0 and accidentally let some hcalls through. So I'm just being overly cautions with this patch. If that will end up causing too much confusion, we could drop this one. >> return H_FUNCTION; >> } >> >> @@ -1532,6 +1532,10 @@ static target_ulong h_tlb_invalidate(PowerPCCPU *cpu, >> * across L1<->L2 transitions, so nothing is required here. >> */ >> >> + if (!tcg_enabled()) { >> + return H_FUNCTION; >> + } >> + >> return H_SUCCESS; >> } >> >> @@ -1572,6 +1576,10 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu, >> uint64_t cr; >> int i; >> >> + if (!tcg_enabled()) { >> + return H_FUNCTION; >> + } >> + >> if (spapr->nested_ptcr == 0) { >> return H_NOT_AVAILABLE; >> }
On Fri, Mar 18, 2022 at 10:41:19AM -0300, Fabiano Rosas wrote: > David Gibson <david@gibson.dropbear.id.au> writes: > > > On Thu, Mar 17, 2022 at 02:20:47PM -0300, Fabiano Rosas wrote: > >> It is possible that nested KVM hypercalls reach QEMU while we're > >> running KVM. The spapr virtual hypervisor implementation of the nested > >> KVM API only works when the L1 is running under TCG. So return > >> H_FUNCTION if we are under KVM. > >> > >> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> > >> --- > >> hw/ppc/spapr_hcall.c | 10 +++++++++- > >> 1 file changed, 9 insertions(+), 1 deletion(-) > >> > >> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > >> index f008290787..119baa1d2d 100644 > >> --- a/hw/ppc/spapr_hcall.c > >> +++ b/hw/ppc/spapr_hcall.c > >> @@ -1508,7 +1508,7 @@ static target_ulong h_set_ptbl(PowerPCCPU *cpu, > >> { > >> target_ulong ptcr = args[0]; > >> > >> - if (!spapr_get_cap(spapr, SPAPR_CAP_NESTED_KVM_HV)) { > >> + if (!spapr_get_cap(spapr, SPAPR_CAP_NESTED_KVM_HV) || !tcg_enabled()) { > > > > I was about to nack this on the grounds that it changes guest visible > > behaviour based on host properties. Then I realized that's not the > > case, because in the KVM + SPAPR_CAP_NESTED_KVM_HV case the hypercall > > should be caught by KVM first and never reach here. > > > > So at the very least I think this needs a comment explaining that. > > Ok. > > > However, I'm still kind of confused how we would get here in the first > > place. If SPAPR_CAP_NESTED_KVM_HV is set, but KVM doesn't support it, > > we should fail outright in cap_nested_kvm_hv_apply(). So how *do* we > > get here? Is the kernel not doing what we expect of it? If so, we > > should probably abort, rather than just returning H_FUNCTION. > > Indeed, If all parts are functioning this should never happen. I was > hacking in L0 and accidentally let some hcalls through. So I'm just > being overly cautions with this patch. If that will end up causing too > much confusion, we could drop this one. Ok, having something check that case is reasonable - but as a "can't happen" it should abort, rather than returning something sensible to the guest. > > >> return H_FUNCTION; > >> } > >> > >> @@ -1532,6 +1532,10 @@ static target_ulong h_tlb_invalidate(PowerPCCPU *cpu, > >> * across L1<->L2 transitions, so nothing is required here. > >> */ > >> > >> + if (!tcg_enabled()) { > >> + return H_FUNCTION; > >> + } > >> + > >> return H_SUCCESS; > >> } > >> > >> @@ -1572,6 +1576,10 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu, > >> uint64_t cr; > >> int i; > >> > >> + if (!tcg_enabled()) { > >> + return H_FUNCTION; > >> + } > >> + > >> if (spapr->nested_ptcr == 0) { > >> return H_NOT_AVAILABLE; > >> } >
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c index f008290787..119baa1d2d 100644 --- a/hw/ppc/spapr_hcall.c +++ b/hw/ppc/spapr_hcall.c @@ -1508,7 +1508,7 @@ static target_ulong h_set_ptbl(PowerPCCPU *cpu, { target_ulong ptcr = args[0]; - if (!spapr_get_cap(spapr, SPAPR_CAP_NESTED_KVM_HV)) { + if (!spapr_get_cap(spapr, SPAPR_CAP_NESTED_KVM_HV) || !tcg_enabled()) { return H_FUNCTION; } @@ -1532,6 +1532,10 @@ static target_ulong h_tlb_invalidate(PowerPCCPU *cpu, * across L1<->L2 transitions, so nothing is required here. */ + if (!tcg_enabled()) { + return H_FUNCTION; + } + return H_SUCCESS; } @@ -1572,6 +1576,10 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu, uint64_t cr; int i; + if (!tcg_enabled()) { + return H_FUNCTION; + } + if (spapr->nested_ptcr == 0) { return H_NOT_AVAILABLE; }
It is possible that nested KVM hypercalls reach QEMU while we're running KVM. The spapr virtual hypervisor implementation of the nested KVM API only works when the L1 is running under TCG. So return H_FUNCTION if we are under KVM. Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> --- hw/ppc/spapr_hcall.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)