RE: [PATCH v4 05/15] xen/x86: introduce "cpufreq=amd-cppc" xen cmdline

["Penny, Zheng" <penny.zheng@xxxxxxx>]

[Public]

Hi,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Tuesday, April 29, 2025 8:52 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Anthony PERARD <anthony.perard@xxxxxxxxxx>;
> Orzel, Michal <Michal.Orzel@xxxxxxx>; Julien Grall <julien@xxxxxxx>; Roger
> Pau Monné <roger.pau@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>;
> xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v4 05/15] xen/x86: introduce "cpufreq=amd-cppc" xen
> cmdline
>
> On 14.04.2025 09:40, Penny Zheng wrote:
> > @@ -514,5 +515,16 @@ acpi_cpufreq_driver = {
> >
> >  int __init acpi_cpufreq_register(void)  {
> > -    return cpufreq_register_driver(&acpi_cpufreq_driver);
> > +    int ret;
> > +
> > +    ret = cpufreq_register_driver(&acpi_cpufreq_driver);
> > +    if ( ret )
> > +        return ret;
> > +    /*
> > +     * After cpufreq driver registeration, XEN_PROCESSOR_PM_CPPC
> > +     * and XEN_PROCESSOR_PM_PX shall become exclusive flags
> > +     */
> > +    xen_processor_pmbits &= ~XEN_PROCESSOR_PM_CPPC;
> > +
> > +    return ret;
> >  }
>
> Why is no similar adjustment needed in powernow_register_driver()? In 
> principle I
> would have expected that it's not each individual driver which needs to care 
> about
> this aspect, but that the framework is taking care of this.
>

Then maybe we shall add this here, to extract the codes from each specific 
driver:
```
diff --git a/xen/arch/x86/acpi/cpufreq/cpufreq.c 
b/xen/arch/x86/acpi/cpufreq/cpufreq.c
index eac1c125a3..9276241291 100644
--- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
+++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
@@ -190,6 +190,25 @@ static int __init cf_check cpufreq_driver_init(void)
                 if ( ret != -ENODEV )
                     break;
             }
+
+            if ( !ret && i < cpufreq_xen_cnt )
+            {
+                switch ( cpufreq_xen_opts[i] )
+                {
+                case CPUFREQ_amd_cppc:
+                    xen_processor_pmbits &= ~XEN_PROCESSOR_PM_XEN;
+                    break;
+
+                case CPUFREQ_xen:
+                    xen_processor_pmbits &= ~XEN_PROCESSOR_PM_CPPC;
+                    break;
+
+                case CPUFREQ_none:
+                default:
+                    break;
+                }
+            }
+
             break;
         }
     }
```

> > @@ -573,6 +576,14 @@ ret_t do_platform_op(
> >          }
> >
> >          case XEN_PM_CPPC:
> > +            if ( !(xen_processor_pmbits & XEN_PROCESSOR_PM_CPPC) )
> > +            {
> > +                ret = -EOPNOTSUPP;
> > +                break;
> > +            }
>
> While at least you no longer use -ENOSYS here, I question this behavior, 
> including
> that for the pre-existing cases: How is the caller supposed to know whether to
> invoke this sub-op? Ignoring errors is generally not a good idea, so it would 
> be
> better if the caller could blindly issue this request, getting back success 
> unless
> there really was an issue with the data provided.
>

Understood.
I will change it to ret = 0. Do you think we shall add warning info here?
Dom0 will send the CPPC data whenever it could. XEN_PROCESSOR_PM_CPPC
is not set could largely be users choosing not to. In such case, silently 
getting back success
shall be enough.


> Jan