On 11.07.2025 05:50, Penny Zheng wrote:
> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
> @@ -128,12 +128,14 @@ static int __init cf_check cpufreq_driver_init(void)
>
> if ( cpufreq_controller == FREQCTL_xen )
> {
> + unsigned int i = 0;
Pointless initializer; both for() loops set i to 0. But also see further
down.
> @@ -157,9 +164,70 @@ static int __init cf_check cpufreq_driver_init(void)
>
> case X86_VENDOR_AMD:
> case X86_VENDOR_HYGON:
> - ret = IS_ENABLED(CONFIG_AMD) ? powernow_register_driver() :
> -ENODEV;
> + if ( !IS_ENABLED(CONFIG_AMD) )
> + {
> + ret = -ENODEV;
> + break;
> + }
> + ret = -ENOENT;
The code structure is sufficiently different from the Intel counterpart for
this to perhaps better move ...
> + for ( i = 0; i < cpufreq_xen_cnt; i++ )
> + {
> + switch ( cpufreq_xen_opts[i] )
> + {
> + case CPUFREQ_xen:
> + ret = powernow_register_driver();
> + break;
> +
> + case CPUFREQ_amd_cppc:
> + ret = amd_cppc_register_driver();
> + break;
> +
> + case CPUFREQ_none:
> + ret = 0;
> + break;
> +
> + default:
> + printk(XENLOG_WARNING
> + "Unsupported cpufreq driver for vendor AMD or
> Hygon\n");
> + break;
... here.
> + }
> +
> + if ( !ret || ret == -EBUSY )
> + break;
> + }
> +
> break;
> }
> +
> + /*
> + * After successful cpufreq driver registeration,
> XEN_PROCESSOR_PM_CPPC
> + * and XEN_PROCESSOR_PM_PX shall become exclusive flags.
> + */
> + if ( !ret )
> + {
> + ASSERT(i < cpufreq_xen_cnt);
> + switch ( cpufreq_xen_opts[i] )
Hmm, this is using the the initializer of i that I commented on. I think there's
another default: case missing, where you simply "return 0" (to retain prior
behavior). But again see also yet further down.
> + {
> + case CPUFREQ_amd_cppc:
> + xen_processor_pmbits &= ~XEN_PROCESSOR_PM_PX;
> + break;
> +
> + case CPUFREQ_hwp:
> + case CPUFREQ_xen:
> + xen_processor_pmbits &= ~XEN_PROCESSOR_PM_CPPC;
> + break;
> +
> + default:
> + break;
> + }
> + } else if ( ret != -EBUSY )
Nit (style): Closing brace wants to be on its own line.
> + /*
> + * No cpufreq driver gets registered, clear both
> + * XEN_PROCESSOR_PM_CPPC and XEN_PROCESSOR_PM_PX
> + */
> + xen_processor_pmbits &= ~(XEN_PROCESSOR_PM_CPPC |
> + XEN_PROCESSOR_PM_PX);
Yet more hmm - this path you want to get through for the case mentioned above.
But only this code; specifically not the "switch ( cpufreq_xen_opts[i] )",
which really is "switch ( cpufreq_xen_opts[0] )" in that case, and that's
pretty clearly wrong to evaluate in then.
> --- a/xen/drivers/acpi/pmstat.c
> +++ b/xen/drivers/acpi/pmstat.c
> @@ -107,6 +107,9 @@ int cpufreq_statistic_init(unsigned int cpu)
> if ( !pmpt )
> return -EINVAL;
>
> + if ( !(pmpt->init & XEN_PX_INIT) )
> + return 0;
> +
> spin_lock(cpufreq_statistic_lock);
>
> pxpt = per_cpu(cpufreq_statistic_data, cpu);
This change could do with a code comment, I think.
> --- a/xen/drivers/cpufreq/cpufreq.c
> +++ b/xen/drivers/cpufreq/cpufreq.c
> @@ -98,6 +98,10 @@ static int __init handle_cpufreq_cmdline(enum
> cpufreq_xen_opt option)
> cpufreq_xen_opts[cpufreq_xen_cnt++] = option;
> switch ( option )
> {
> + case CPUFREQ_amd_cppc:
> + xen_processor_pmbits |= XEN_PROCESSOR_PM_CPPC;
> + break;
> +
> case CPUFREQ_hwp:
> case CPUFREQ_xen:
> xen_processor_pmbits |= XEN_PROCESSOR_PM_PX;
Unless they're clearly "more important" (tm), please can insertions like
this not be done at the top of a switch() (or whatever else it is)? You
don't do so ...
> @@ -166,6 +170,13 @@ static int __init cf_check setup_cpufreq_option(const
> char *str)
> if ( !ret && arg[0] && arg[1] )
> ret = hwp_cmdline_parse(arg + 1, end);
> }
> + else if ( IS_ENABLED(CONFIG_AMD) && choice < 0 &&
> + !cmdline_strcmp(str, "amd-cppc") )
> + {
> + ret = handle_cpufreq_cmdline(CPUFREQ_amd_cppc);
> + if ( !ret && arg[0] && arg[1] )
> + ret = amd_cppc_cmdline_parse(arg + 1, end);
> + }
> else
> ret = -EINVAL;
... here, for example.
Jan
