Re: [PATCH v5 14/14] spapr: nested: Introduce cap-nested-papr for Nested PAPR API

[Harsh Prateek Bora <harshpb@linux.ibm.com>]

Hi Nick,

Updated incremental fix below:

On 3/12/24 18:21, Harsh Prateek Bora wrote:
> 
> 
> On 3/12/24 18:17, Harsh Prateek Bora wrote:
>> Hi Nick,
>>
>> On 3/12/24 17:41, Harsh Prateek Bora wrote:
>>> Hi Nick,
>>>
>>> On 3/12/24 17:21, Nicholas Piggin wrote:
>>>> On Fri Mar 8, 2024 at 9:19 PM AEST, Harsh Prateek Bora wrote:
>>>>> Introduce a SPAPR capability cap-nested-papr which enables nested PAPR
>>>>> API for nested guests. This new API is to enable support for KVM on 
>>>>> PowerVM
>>>>> and the support in Linux kernel has already merged upstream.
>>>>>
>>>>> Signed-off-by: Michael Neuling <mikey@neuling.org>
>>>>> Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
>>>>> ---
>>>>>   include/hw/ppc/spapr.h |  6 +++-
>>>>>   hw/ppc/spapr.c         |  2 ++
>>>>>   hw/ppc/spapr_caps.c    | 62 
>>>>> ++++++++++++++++++++++++++++++++++++++++++
>>>>>   hw/ppc/spapr_nested.c  |  8 ++++--
>>>>>   4 files changed, 74 insertions(+), 4 deletions(-)
>>>>>

<snip>

>>>>> +static void cap_nested_papr_apply(SpaprMachineState *spapr,
>>>>> +                                    uint8_t val, Error **errp)
>>>>> +{
>>>>> +    ERRP_GUARD();
>>>>> +    PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
>>>>> +    CPUPPCState *env = &cpu->env;
>>>>> +
>>>>> +    if (!val) {
>>>>> +        /* capability disabled by default */
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    if (tcg_enabled()) {
>>>>> +        if (!(env->insns_flags2 & PPC2_ISA300)) {
>>>>> +            error_setg(errp, "Nested-PAPR only supported on POWER9 
>>>>> and later");
>>>>> +            error_append_hint(errp,
>>>>> +                              "Try appending -machine 
>>>>> cap-nested-papr=off\n");
>>>>> +            return;
>>>>> +        }
>>>>> +        if (spapr_nested_api(spapr) &&
>>>>> +            spapr_nested_api(spapr) != NESTED_API_PAPR) {
>>>>> +            error_setg(errp, "Nested-HV APIs are mutually 
>>>>> exclusive/incompatible");
>>>>> +            error_append_hint(errp, "Please use either 
>>>>> cap-nested-hv or "
>>>>> +                                    "cap-nested-papr to proceed.\n");
>>>>> +            return;
>>>>> +        } else {
>>>>> +            spapr->nested.api = NESTED_API_PAPR;
>>>>> +        }
>>>>> +
>>>>> +    } else if (kvm_enabled()) {
>>>>> +        /*
>>>>> +         * this gets executed in L1 qemu when L2 is launched,
>>>>> +         * needs kvm-hv support in L1 kernel.
>>>>> +         */
>>>>> +        if (!kvmppc_has_cap_nested_kvm_hv()) {
>>>>> +            error_setg(errp,
>>>>> +                       "KVM implementation does not support 
>>>>> Nested-HV");
>>>>> +        } else if (kvmppc_set_cap_nested_kvm_hv(val) < 0) {
>>>>> +            error_setg(errp, "Error enabling Nested-HV with KVM");
>>>>> +        }
>>>>
>>>> I'll just disable this on KVM for now. With that changed,
>>>>
>>>> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>>>>
>>>
>>> AFAIK, v2 api also expects this capability to be enabled on L1 kernel.
>>> I guess the reason is the L1 implementation has used the same capab and
>>> extended to be used with v2 api. So, this check is needed in L1 Qemu for
>>> now. We may revisit L1 implementation later to see if a change is
>>> appropriate.
>>
>> Please ignore above response. I think my observation was based on 
>> older version of L1 implementation. This doesnt seem to be an issue 
>> with upstream L1. You may disable the kvm_enabled() path for now. I 
>> just tested and it works fine.
> 
> Here's the incremental fix:
> 

Updated to keep error_setg for kvm_enabled() case:

diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index d6d5a6b8df..92d8966d60 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -527,18 +527,9 @@ static void cap_nested_papr_apply(SpaprMachineState 
*spapr,
          } else {
              spapr->nested.api = NESTED_API_PAPR;
          }
-
      } else if (kvm_enabled()) {
-        /*
-         * this gets executed in L1 qemu when L2 is launched,
-         * needs kvm-hv support in L1 kernel.
-         */
-        if (!kvmppc_has_cap_nested_kvm_hv()) {
              error_setg(errp,
-                       "KVM implementation does not support Nested-HV");
-        } else if (kvmppc_set_cap_nested_kvm_hv(val) < 0) {
-            error_setg(errp, "Error enabling Nested-HV with KVM");
-        }
+                       "KVM implementation does not support Nested-PAPR");
      }
  }



> 
> regards,
> Harsh
>>
>> regards,
>> Harsh
>>
>>>
>>> regards,
>>> Harsh
>>>
>>>>>       }
>>>>>   }
>>>>> @@ -735,6 +787,15 @@ SpaprCapabilityInfo 
>>>>> capability_table[SPAPR_CAP_NUM] = {
>>>>>           .type = "bool",
>>>>>           .apply = cap_nested_kvm_hv_apply,
>>>>>       },
>>>>> +    [SPAPR_CAP_NESTED_PAPR] = {
>>>>> +        .name = "nested-papr",
>>>>> +        .description = "Allow Nested HV (PAPR API)",
>>>>> +        .index = SPAPR_CAP_NESTED_PAPR,
>>>>> +        .get = spapr_cap_get_bool,
>>>>> +        .set = spapr_cap_set_bool,
>>>>> +        .type = "bool",
>>>>> +        .apply = cap_nested_papr_apply,
>>>>> +    },
>>>>>       [SPAPR_CAP_LARGE_DECREMENTER] = {
>>>>>           .name = "large-decr",
>>>>>           .description = "Allow Large Decrementer",
>>>>> @@ -919,6 +980,7 @@ SPAPR_CAP_MIG_STATE(sbbc, SPAPR_CAP_SBBC);
>>>>>   SPAPR_CAP_MIG_STATE(ibs, SPAPR_CAP_IBS);
>>>>>   SPAPR_CAP_MIG_STATE(hpt_maxpagesize, SPAPR_CAP_HPT_MAXPAGESIZE);
>>>>>   SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV);
>>>>> +SPAPR_CAP_MIG_STATE(nested_papr, SPAPR_CAP_NESTED_PAPR);
>>>>>   SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER);
>>>>>   SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST);
>>>>>   SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI);
>>>>> diff --git a/hw/ppc/spapr_nested.c b/hw/ppc/spapr_nested.c
>>>>> index 597dba7fdc..8db9dc19e3 100644
>>>>> --- a/hw/ppc/spapr_nested.c
>>>>> +++ b/hw/ppc/spapr_nested.c
>>>>> @@ -13,13 +13,15 @@
>>>>>   void spapr_nested_reset(SpaprMachineState *spapr)
>>>>>   {
>>>>>       if (spapr_get_cap(spapr, SPAPR_CAP_NESTED_KVM_HV)) {
>>>>> -        spapr->nested.api = NESTED_API_KVM_HV;
>>>>>           spapr_unregister_nested_hv();
>>>>>           spapr_register_nested_hv();
>>>>> -    } else {
>>>>> -        spapr->nested.api = 0;
>>>>> +    } else if (spapr_get_cap(spapr, SPAPR_CAP_NESTED_PAPR)) {
>>>>>           spapr->nested.capabilities_set = false;
>>>>> +        spapr_unregister_nested_papr();
>>>>> +        spapr_register_nested_papr();
>>>>>           spapr_nested_gsb_init();
>>>>> +    } else {
>>>>> +        spapr->nested.api = 0;
>>>>>       }
>>>>>   }
>>>>
>