On 26.08.2025 07:53, Penny, Zheng wrote:
> [Public]
>
>> -----Original Message-----
>> From: Jan Beulich <jbeul...@suse.com>
>> Sent: Monday, August 25, 2025 11:02 PM
>> To: Penny, Zheng <penny.zh...@amd.com>
>> Cc: Huang, Ray <ray.hu...@amd.com>; Andrew Cooper
>> <andrew.coop...@citrix.com>; Roger Pau Monné <roger....@citrix.com>;
>> Anthony PERARD <anthony.per...@vates.tech>; Orzel, Michal
>> <michal.or...@amd.com>; Julien Grall <jul...@xen.org>; Stefano Stabellini
>> <sstabell...@kernel.org>; xen-devel@lists.xenproject.org
>> Subject: Re: [PATCH v7 06/13] xen/cpufreq: introduce new sub-hypercall to
>> propagate CPPC data
>>
>> On 22.08.2025 12:52, Penny Zheng wrote:
>>> --- a/xen/arch/x86/x86_64/cpufreq.c
>>> +++ b/xen/arch/x86/x86_64/cpufreq.c
>>> @@ -54,3 +54,22 @@ int compat_set_px_pminfo(uint32_t acpi_id,
>>>
>>> return set_px_pminfo(acpi_id, xen_perf); }
>>> +
>>> +int compat_set_cppc_pminfo(unsigned int acpi_id,
>>> + const struct compat_processor_cppc
>>> +*cppc_data)
>>> +
>>> +{
>>> + struct xen_processor_cppc *xen_cppc;
>>> + unsigned long xlat_page_current;
>>> +
>>> + xlat_malloc_init(xlat_page_current);
>>> +
>>> + xen_cppc = xlat_malloc_array(xlat_page_current,
>>> + struct xen_processor_cppc, 1);
>>> + if ( unlikely(xen_cppc == NULL) )
>>> + return -EFAULT;
>>
>> I think we want to avoid repeating the earlier mistake with using a wrong
>> error code.
>> It's ENOMEM or ENOSPC or some such.
>>
>
> Understood, I'll change it to -ENOMEM
>
>>> --- a/xen/drivers/acpi/pm-op.c
>>> +++ b/xen/drivers/acpi/pm-op.c
>>> @@ -91,7 +91,9 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
>>> pmpt = processor_pminfo[op->cpuid];
>>> policy = per_cpu(cpufreq_cpu_policy, op->cpuid);
>>>
>>> - if ( !pmpt || !pmpt->perf.states ||
>>> + if ( !pmpt ||
>>> + ((pmpt->init & XEN_PX_INIT) && !pmpt->perf.states) ||
>>> + ((pmpt->init & XEN_CPPC_INIT) && pmpt->perf.state_count) ||
>>
>> I fear I don't understand this: In the PX case we check whether necessary
>> data is
>> lacking. In the CPPC case you check that some data was provided that we don't
>> want to use? Why not similarly check that data we need was provided?
>>
>
> We are introducing another checking line for CPPC is actually to avoid NULL
> deref of state[i]:
> ```
> for ( i = 0; i < op->u.get_para.freq_num; i++ )
> data[i] = pmpt->perf.states[i].core_frequency * 1000;
> ```
> We want to ensure "op->u.get_para.freq_num" is always zero in CPPC mode,
> which is validated against pmpt->perf.state_count.
> We have similar discussion in here
> https://old-list-archives.xen.org/archives/html/xen-devel/2025-06/msg01160.html
Indeed I was thinking that we would have touched this before. As to your reply:
This explains the .state_count check (which imo wants a comment). It doesn't,
however, explain the absence of a "have we got the data we need" part. Unless
of course there simply isn't anything to check for.
Jan
