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

[David Gibson]

On Wed, Feb 16, 2022 at 11:50:34AM +1000, Nicholas Piggin wrote:
> 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?

Yes, it should.  Which for something as obscure as POWER7 may be acceptable.

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

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

signature.asc
Description: PGP signature