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

[Fabiano Rosas]

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