Re: [PATCH v7 3/4] KVM: PPC: Book3S HV: Add support for compat CPU capabilities for KVM on PowerNV

[Amit Machhiwal]

On 2026/08/07 10:24 AM, Ritesh Harjani wrote:
> Amit Machhiwal <amachhiw@xxxxxxxxxxxxx> writes:

<snip>

> >  
> >  static int kvmppc_get_compat_caps(struct kvm_ppc_compat_caps *host_caps)
> >  {
> > +	struct device_node *np;
> >  	unsigned long capabilities = 0;
> >  	long rc = -EINVAL;
> > +	u32 cpu_version = 0;
> >  
> >  	if (kvmhv_on_pseries()) {
> >  		if (kvmhv_is_nestedv2()) {
> >  			WARN_ON_ONCE(!nested_capabilities);
> >  			capabilities = nested_capabilities;
> >  			rc = 0;
> > +		} else {
> > +			for_each_node_by_type(np, "cpu") {
> > +				if (!of_property_read_u32(np, "cpu-version",
> > +							  &cpu_version)) {
> > +					of_node_put(np);
> > +					break;
> > +				}
> > +			}
> 
> What happens when we don't have "cpu-version" DT property?
> Check this commit
> 5a61ef74f269f2 ("powerpc/64s: Support new device tree binding for discovering CPU features")
> 
> And also why do we need to parse the DT properties again?
> Shouldn't we check something like this?
> 
> 	if (cpu_has_feature(CPU_FTR_P11_PVR))
> 		capabilities |= KVM_PPC_COMPAT_CAP_POWER11;
> 	if (cpu_has_feature(CPU_FTR_ARCH_31))
> 		capabilities |= KVM_PPC_COMPAT_CAP_POWER10;
> 	if (cpu_has_feature(CPU_FTR_ARCH_300))
> 		capabilities |= KVM_PPC_COMPAT_CAP_POWER9;
> 	

Thanks for the suggestion, Ritesh! After closer analysis, cpu_has_feature()
would give the same result on pseries, but I believe that using 'cpu-version'
directly is both more correct and more explicit for this context.

Here's why:

1. 'ibm,powerpc-cpu-features' / dt-cpu-ftrs is baremetal (powernv/OPAL) only.
   SLOF firmware used by pseries guests never provides that node. On pseries,
   dt_cpu_ftrs_in_use() is always false, and CPU feature bits including
   CPU_FTR_ARCH_300, CPU_FTR_ARCH_31, CPU_FTR_P11_PVR are set by identify_cpu()
   from the cpu-version DT property at prom.c:423:

     if (!dt_cpu_ftrs_in_use()) {
         prop = of_get_flat_dt_prop(node, "cpu-version", NULL);
         if (prop && (be32_to_cpup(prop) & 0xff000000) == 0x0f000000) {
             identify_cpu(0, be32_to_cpup(prop));

   So cpu_has_feature() on pseries is just an indirect readback of what
   identify_cpu() already derived from 'cpu-version' — the source of truth is
   still cpu-version.

2. Commit e4de1b9cb3b5 ("powerpc/dt_cpu_ftrs: Set CPU_FTR_P11_PVR for Power11
   and later processors") explicitly documents this split:

     "This issue does not affect pseries guests, where SLOF firmware
      does not provide this node, causing the kernel to fall back to
      the traditional cputable path (identify_cpu) which correctly
      sets CPU_FTR_P11_PVR during PVR-based CPU identification."

   That fix was needed on powernv only — and our code is in the
   kvmhv_on_pseries() branch, so the dt-cpu-ftrs path is never taken.

3. 'cpu-version' is the PAPR-defined compat level indicator — it is what PHYP
   and QEMU explicitly set to communicate the negotiated compat mode. Reading it
   directly is semantically correct: we are reporting the compat level the
   hypervisor advertised, not a kernel-internal feature bit derived from it.

I also responded to a similar concern from Sashiko covering points 1 and 3 here:

  https://lore.kernel.org/all/20260806214117.8a2ca150-97-amachhiw@xxxxxxxxxxxxx/

So I'd prefer to keep the direct cpu-version lookup.

Thanks,
Amit