Re: [PATCH 1/2] spapr: Add SPAPR_CAP_AIL_MODE_3 for AIL mode 3 support for H_SET_MODE hcall

[Nicholas Piggin]

Excerpts from David Gibson's message of February 15, 2022 11:10 am:
> On Mon, Feb 14, 2022 at 09:17:48PM +1000, Nicholas Piggin wrote:
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index 222c1b6bbd..5dec056796 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -811,32 +811,35 @@ static target_ulong h_set_mode_resource_le(PowerPCCPU 
>> *cpu,
>>  }
>>  
>>  static target_ulong h_set_mode_resource_addr_trans_mode(PowerPCCPU *cpu,
>> +                                                        SpaprMachineState 
>> *spapr,
>>                                                          target_ulong mflags,
>>                                                          target_ulong value1,
>>                                                          target_ulong value2)
>>  {
>> -    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>> -
>> -    if (!(pcc->insns_flags2 & PPC2_ISA207S)) {
>> -        return H_P2;
>> -    }
>>      if (value1) {
>>          return H_P3;
>>      }
>> +
>>      if (value2) {
>>          return H_P4;
>>      }
>>  
>> -    if (mflags == 1) {
>> -        /* AIL=1 is reserved in POWER8/POWER9/POWER10 */
>> +    /* AIL-1 is not architected, and AIL-2 is not supported by QEMU. */
>> +    if (mflags == 1 || mflags == 2) {
> 
> This test is redundant with the one below, isn't it?

Ah, yes.

> 
>>          return H_UNSUPPORTED_FLAG;
>>      }
>>  
>> -    if (mflags == 2 && (pcc->insns_flags2 & PPC2_ISA310)) {
>> -        /* AIL=2 is reserved in POWER10 (ISA v3.1) */
>> +    /* Only 0 and 3 are possible */
>> +    if (mflags != 0 && mflags != 3) {
>>          return H_UNSUPPORTED_FLAG;
>>      }
>>  
>> +    if (mflags == 3) {
>> +        if (!spapr_get_cap(spapr, SPAPR_CAP_AIL_MODE_3)) {
>> +            return H_UNSUPPORTED_FLAG;
>> +        }
>> +    }
>> +
>>      spapr_set_all_lpcrs(mflags << LPCR_AIL_SHIFT, LPCR_AIL);
>>  
>>      return H_SUCCESS;
>> @@ -853,7 +856,7 @@ static target_ulong h_set_mode(PowerPCCPU *cpu, 
>> SpaprMachineState *spapr,
>>          ret = h_set_mode_resource_le(cpu, spapr, args[0], args[2], args[3]);
>>          break;
>>      case H_SET_MODE_RESOURCE_ADDR_TRANS_MODE:
>> -        ret = h_set_mode_resource_addr_trans_mode(cpu, args[0],
>> +        ret = h_set_mode_resource_addr_trans_mode(cpu, spapr, args[0],
>>                                                    args[2], args[3]);
>>          break;
>>      }
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index ee7504b976..edbf3eeed0 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -77,8 +77,10 @@ typedef enum {
>>  #define SPAPR_CAP_FWNMI                 0x0A
>>  /* Support H_RPT_INVALIDATE */
>>  #define SPAPR_CAP_RPT_INVALIDATE        0x0B
>> +/* Support for AIL modes */
>> +#define SPAPR_CAP_AIL_MODE_3            0x0C
>>  /* Num Caps */
>> -#define SPAPR_CAP_NUM                   (SPAPR_CAP_RPT_INVALIDATE + 1)
>> +#define SPAPR_CAP_NUM                   (SPAPR_CAP_AIL_MODE_3 + 1)
>>  
>>  /*
>>   * Capability Values
>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>> index dc93b99189..128bc530d4 100644
>> --- a/target/ppc/kvm.c
>> +++ b/target/ppc/kvm.c
>> @@ -2563,6 +2563,29 @@ int kvmppc_has_cap_rpt_invalidate(void)
>>      return cap_rpt_invalidate;
>>  }
>>  
>> +int kvmppc_supports_ail_3(void)
> 
> Returning bool would make more sense, no?

It would.

> 
>> +{
>> +    PowerPCCPUClass *pcc = kvm_ppc_get_host_cpu_class();
>> +
>> +    /*
>> +     * KVM PR only supports AIL-0
>> +     */
>> +    if (kvmppc_is_pr(kvm_state)) {
>> +        return 0;
>> +    }
>> +
>> +    /*
>> +     * KVM HV hosts support AIL-3 on POWER8 and above, except for radix
>> +     * mode on some early POWER9s, but it's not clear how to cover those
>> +     * without disabling the feature for many.
>> +     */
>> +    if (!(pcc->insns_flags2 & PPC2_ISA207S)) {
> 
> This effectively means that the pseries machine type simply won't
> start with a !PPC2_ISA207S cpu model.  I'm not sure if we support any
> such CPUs in any case.

Oh, would that at least give an error to disable cap-ail-mode-3?

> If we do, we should probably change the
> default value for this cap based on cpu model in
> default_caps_with_cpu().

We allegedly still support POWER7 KVM in Linux. I've never tested it
and I don't know how much it's used at all. Probably should keep it
working here if possible. I'll look into default_caps_with_cpu().

Thanks,
Nick