Re: [PATCH v6 11/19] xen/x86: introduce "cpufreq=amd-cppc" xen cmdline and amd-cppc driver

[Jan Beulich <jbeulich@xxxxxxxx>]

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