Re: [PATCH 1/3] spapr: Ignore nested KVM hypercalls when not running TCG

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3125 bytes --]

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;
> >>      }
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]