|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

I'll review this DPDK patch series focusing on correctness, coding style, and guideline compliance.

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

This patch series replaces lcore_id >= RTE_MAX_LCORE checks with
rte_lcore_is_enabled() across power management drivers and introduces a
common validation macro.

> 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 rte_lcore_is_enabled() properly handle lcore_id >= RTE_MAX_LCORE?
The original code validated the range before use. Does the replacement
function include this bounds check, or could this introduce an
out-of-bounds access in the enabled lcore bitmap?

[ ... ]

> 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 need to verify that lcore_id is less than RTE_MAX_LCORE
before calling rte_lcore_is_enabled()? If rte_lcore_is_enabled() does
not validate the range internally, this could be unsafe.

The macro uses 'return', which means it can only be used in functions
returning the appropriate type. Is this limitation documented, or should
the macro be named more clearly to indicate this restriction?

> +
>  /* check if scaling driver matches one we want */
>  __rte_internal
>  int cpufreq_check_scaling_driver(const char *driver);

[ ... ]

> 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 this include actually used anywhere in the patch? No errno values
appear to be referenced in the changes.

[ ... ]

> @@ -147,6 +149,7 @@ RTE_EXPORT_SYMBOL(rte_power_exit)
>  int
>  rte_power_exit(unsigned int lcore_id)
>  {
> +	RTE_POWER_VALID_LCOREID_OR_ERR_RET(lcore_id, -1);
>  	if (global_default_env != PM_ENV_NOT_SET)
>  		return global_cpufreq_ops->exit(lcore_id);

In power_kvm_vm_init(), power_acpi_cpufreq_init(), and similar
functions, after patches 11-15 remove the driver-level checks, does
this create a situation where init functions can be called on disabled
lcores and partially initialize state? The state machine uses atomic
operations with POWER_IDLE as the expected state - could this cause
issues if an init is attempted on a disabled lcore that was previously
used?

Does the removal of driver-level validation in patches 11-15 rely on
the framework checks added in patch 10? If patches are applied
out-of-order or selectively, could this create a gap where no
validation occurs?