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

[IBM]

Amit Machhiwal <amachhiw@xxxxxxxxxxxxx> writes:

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

Arghh. That make sense. Thanks for checking.
The design with "cpu-version" sounds good in that case.

Please feel free to add:
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@xxxxxxxxx>