|SUCCESS| [v1, 15/15] power/kvm_vm: remove the verification of lcore ID

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163111

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-16

git sha: 20260416030612.2379407
Author: Huisong Li <lihuisong at huawei.com>
Subject: [PATCH v1 01/15] power/kvm_vm: enforce enabled lcore ID check

This series enforces enabled lcore ID checking across multiple power
drivers and the power framework. The patches replace the current
RTE_MAX_LCORE checks with rte_lcore_is_enabled() and add a common macro
for this validation.

> diff --git a/drivers/power/kvm_vm/kvm_vm.c b/drivers/power/kvm_vm/kvm_vm.c
> index 5754a441cd..816863da7a 100644
> --- a/drivers/power/kvm_vm/kvm_vm.c
> +++ b/drivers/power/kvm_vm/kvm_vm.c
> @@ -24,11 +24,11 @@ power_kvm_vm_check_supported(void)
>  int
>  power_kvm_vm_init(unsigned int lcore_id)
>  {
> -	if (lcore_id >= RTE_MAX_LCORE) {
> -		POWER_LOG(ERR, "Core(%u) is out of range 0...%d",
> -				lcore_id, RTE_MAX_LCORE-1);
> +	if (!rte_lcore_is_enabled(lcore_id)) {
> +		POWER_LOG(ERR, "lcore id %u is not enabled", lcore_id);
>  		return -1;
>  	}
> +

Does this preserve the semantics of the original check? The old code
rejected any lcore_id >= RTE_MAX_LCORE. Does rte_lcore_is_enabled()
handle the case when lcore_id >= RTE_MAX_LCORE correctly? Can callers
still detect when they pass invalid lcore IDs that exceed the maximum,
versus valid IDs that are simply not enabled?

[ ... ]

> diff --git a/lib/power/power_common.h b/lib/power/power_common.h
> index 3f56b1103d..5f13068ad6 100644
> --- a/lib/power/power_common.h
> +++ b/lib/power/power_common.h
> @@ -23,6 +23,13 @@ extern int rte_power_logtype;
>  #define POWER_DEBUG_LOG(...)
>  #endif
>  
> +#define RTE_POWER_VALID_LCOREID_OR_ERR_RET(lcore_id, retval) do { \
> +	if (!rte_lcore_is_enabled(lcore_id)) { \
> +		POWER_LOG(ERR, "lcore id %u is not enabled", lcore_id); \
> +		return retval; \
> +	} \
> +} while (0)
> +

Does this macro handle all lcore_id values correctly? When lcore_id is
>= RTE_MAX_LCORE, does rte_lcore_is_enabled() return false as expected?
The macro silently changes error detection behavior from explicit range
checking to enabled-status checking.

[ ... ]

> diff --git a/lib/power/rte_power_cpufreq.c b/lib/power/rte_power_cpufreq.c
> index f63e976dc2..c51dc8364e 100644
> --- a/lib/power/rte_power_cpufreq.c
> +++ b/lib/power/rte_power_cpufreq.c
> @@ -1,6 +1,7 @@
>  /* SPDX-License-Identifier: BSD-3-Clause
>   * Copyright(c) 2010-2014 Intel Corporation
>   */
> +#include <errno.h>

Is errno.h included for a reason? Does this patch use any errno symbols?

[ ... ]

> @@ -116,6 +117,7 @@ rte_power_init(unsigned int lcore_id)
>  	struct rte_power_cpufreq_ops *ops;
>  	uint8_t env;
>  
> +	RTE_POWER_VALID_LCOREID_OR_ERR_RET(lcore_id, -1);
>  	if (global_default_env != PM_ENV_NOT_SET)
>  		return global_cpufreq_ops->init(lcore_id);

Does this introduce duplicate checking? After this series removes checks
from individual drivers, does calling rte_power_init()->driver->init()
result in the lcore_id being validated twice (once in the framework,
once in the driver that still has the check)?

[ ... ]

> diff --git a/drivers/power/acpi/acpi_cpufreq.c b/drivers/power/acpi/acpi_cpufreq.c
> index d22e0eb627..9c78b4cb1a 100644
> --- a/drivers/power/acpi/acpi_cpufreq.c
> +++ b/drivers/power/acpi/acpi_cpufreq.c
> @@ -242,11 +242,6 @@ power_acpi_cpufreq_init(unsigned int lcore_id)
>  		return -1;
>  	}
>  
> -	if (!rte_lcore_is_enabled(lcore_id)) {
> -		POWER_LOG(ERR, "lcore id %u is not enabled", lcore_id);
> -		return -1;
> -	}
> -

Can this create a TOCTOU race? Between the framework check in
rte_power_init() and this driver function executing, can an lcore's
enabled status change? Does removing the driver-level check make the
window larger?

[ ... ]

> @@ -372,11 +362,6 @@ power_acpi_cpufreq_freqs(unsigned int lcore_id, uint32_t *freqs, uint32_t num)
>  {
>  	struct acpi_power_info *pi;
>  
> -	if (!rte_lcore_is_enabled(lcore_id)) {
> -		POWER_LOG(ERR, "lcore id %u is not enabled", lcore_id);
> -		return 0;
> -	}
> -
>  	if (freqs == NULL) {
>  		POWER_LOG(ERR, "NULL buffer supplied");
>  		return 0;

Does this code now index into lcore_power_info[] without validating that
lcore_id is within bounds? When lcore_id >= RTE_MAX_LCORE, can this code
overflow the lcore_power_info array?

Similar concern in power_acpi_cpufreq_get_freq():

> @@ -395,22 +380,12 @@ power_acpi_cpufreq_freqs(unsigned int lcore_id, uint32_t *freqs, uint32_t num)
>  uint32_t
>  power_acpi_cpufreq_get_freq(unsigned int lcore_id)
>  {
> -	if (!rte_lcore_is_enabled(lcore_id)) {
> -		POWER_LOG(ERR, "lcore id %u is not enabled", lcore_id);
> -		return RTE_POWER_INVALID_FREQ_INDEX;
> -	}
> -
>  	return lcore_power_info[lcore_id].curr_idx;

Does lcore_power_info[lcore_id] overflow when lcore_id is out of bounds?

[ ... ]