1. Patchew
  2. Xen
  3. View series

[XEN PATCH v1] x86/cpufreq: separate powernow/hwp cpufreq code

Sergiy Kibrik posted 1 patch 5 months, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20240604093406.2448552-1-Sergiy._5FKibrik@epam.com
xen/arch/x86/acpi/cpufreq/Makefile  |  4 ++--
xen/arch/x86/acpi/cpufreq/cpufreq.c |  2 +-
xen/include/acpi/cpufreq/cpufreq.h  | 32 +++++++++++++++++++++++++++++
3 files changed, 35 insertions(+), 3 deletions(-)
[XEN PATCH v1] x86/cpufreq: separate powernow/hwp cpufreq code
Posted by Sergiy Kibrik 5 months, 3 weeks ago 
Build AMD Architectural P-state driver when CONFIG_AMD is on, and
Intel Hardware P-States driver when CONFIG_INTEL is on respectively,
allowing for a plaftorm-specific build.

Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
CC: Jason Andryuk <jason.andryuk@amd.com>
---
 xen/arch/x86/acpi/cpufreq/Makefile  |  4 ++--
 xen/arch/x86/acpi/cpufreq/cpufreq.c |  2 +-
 xen/include/acpi/cpufreq/cpufreq.h  | 32 +++++++++++++++++++++++++++++
 3 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/acpi/cpufreq/Makefile b/xen/arch/x86/acpi/cpufreq/Makefile
index db83aa6b14..527ff20f5a 100644
--- a/xen/arch/x86/acpi/cpufreq/Makefile
+++ b/xen/arch/x86/acpi/cpufreq/Makefile
@@ -1,3 +1,3 @@
 obj-y += cpufreq.o
-obj-y += hwp.o
-obj-y += powernow.o
+obj-$(CONFIG_INTEL) += hwp.o
+obj-$(CONFIG_AMD) += powernow.o
diff --git a/xen/arch/x86/acpi/cpufreq/cpufreq.c b/xen/arch/x86/acpi/cpufreq/cpufreq.c
index a341f2f020..a89f3ed03a 100644
--- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
+++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
@@ -657,7 +657,7 @@ static int __init cf_check cpufreq_driver_init(void)
 
         case X86_VENDOR_AMD:
         case X86_VENDOR_HYGON:
-            ret = powernow_register_driver();
+            ret = IS_ENABLED(CONFIG_AMD) ? powernow_register_driver() : -ENODEV;
             break;
         }
     }
diff --git a/xen/include/acpi/cpufreq/cpufreq.h b/xen/include/acpi/cpufreq/cpufreq.h
index 443427153b..bc0c9a2b9f 100644
--- a/xen/include/acpi/cpufreq/cpufreq.h
+++ b/xen/include/acpi/cpufreq/cpufreq.h
@@ -252,6 +252,7 @@ void cpufreq_dbs_timer_resume(void);
 
 void intel_feature_detect(struct cpufreq_policy *policy);
 
+#ifdef CONFIG_INTEL
 int hwp_cmdline_parse(const char *s, const char *e);
 int hwp_register_driver(void);
 bool hwp_active(void);
@@ -260,4 +261,35 @@ int get_hwp_para(unsigned int cpu,
 int set_hwp_para(struct cpufreq_policy *policy,
                  struct xen_set_cppc_para *set_cppc);
 
+#else
+
+static inline int hwp_cmdline_parse(const char *s, const char *e)
+{
+    return -EINVAL;
+}
+
+static inline int hwp_register_driver(void)
+{
+    return -ENODEV;
+}
+
+static inline bool hwp_active(void)
+{
+    return false;
+}
+
+static inline int get_hwp_para(unsigned int cpu,
+                               struct xen_cppc_para *cppc_para)
+{
+    return -EINVAL;
+}
+
+static inline int set_hwp_para(struct cpufreq_policy *policy,
+                               struct xen_set_cppc_para *set_cppc)
+{
+    return -EINVAL;
+}
+
+#endif /* CONFIG_INTEL */
+
 #endif /* __XEN_CPUFREQ_PM_H__ */
-- 
2.25.1
Re: [XEN PATCH v1] x86/cpufreq: separate powernow/hwp cpufreq code
Posted by Jan Beulich 5 months, 3 weeks ago 
On 04.06.2024 11:34, Sergiy Kibrik wrote:
> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
> @@ -657,7 +657,7 @@ static int __init cf_check cpufreq_driver_init(void)
>  
>          case X86_VENDOR_AMD:
>          case X86_VENDOR_HYGON:
> -            ret = powernow_register_driver();
> +            ret = IS_ENABLED(CONFIG_AMD) ? powernow_register_driver() : -ENODEV;
>              break;
>          }

What about the Intel-specific code immediately up from here?
Dealing with that as well may likely permit to reduce ...

> --- a/xen/include/acpi/cpufreq/cpufreq.h
> +++ b/xen/include/acpi/cpufreq/cpufreq.h
> @@ -252,6 +252,7 @@ void cpufreq_dbs_timer_resume(void);
>  
>  void intel_feature_detect(struct cpufreq_policy *policy);
>  
> +#ifdef CONFIG_INTEL
>  int hwp_cmdline_parse(const char *s, const char *e);
>  int hwp_register_driver(void);
>  bool hwp_active(void);
> @@ -260,4 +261,35 @@ int get_hwp_para(unsigned int cpu,
>  int set_hwp_para(struct cpufreq_policy *policy,
>                   struct xen_set_cppc_para *set_cppc);
>  
> +#else
> +
> +static inline int hwp_cmdline_parse(const char *s, const char *e)
> +{
> +    return -EINVAL;
> +}
> +
> +static inline int hwp_register_driver(void)
> +{
> +    return -ENODEV;
> +}
> +
> +static inline bool hwp_active(void)
> +{
> +    return false;
> +}
> +
> +static inline int get_hwp_para(unsigned int cpu,
> +                               struct xen_cppc_para *cppc_para)
> +{
> +    return -EINVAL;
> +}
> +
> +static inline int set_hwp_para(struct cpufreq_policy *policy,
> +                               struct xen_set_cppc_para *set_cppc)
> +{
> +    return -EINVAL;
> +}
> +
> +#endif /* CONFIG_INTEL */

... the number of stubs you're adding here.

Jan
Re: [XEN PATCH v1] x86/cpufreq: separate powernow/hwp cpufreq code
Posted by Sergiy Kibrik 5 months, 3 weeks ago 
06.06.24 10:08, Jan Beulich:
> On 04.06.2024 11:34, Sergiy Kibrik wrote:
>> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
>> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
>> @@ -657,7 +657,7 @@ static int __init cf_check cpufreq_driver_init(void)
>>   
>>           case X86_VENDOR_AMD:
>>           case X86_VENDOR_HYGON:
>> -            ret = powernow_register_driver();
>> +            ret = IS_ENABLED(CONFIG_AMD) ? powernow_register_driver() : -ENODEV;
>>               break;
>>           }
> 
> What about the Intel-specific code immediately up from here?
> Dealing with that as well may likely permit to reduce ...
>

you mean to guard a call to hwp_register_driver() the same way as for 
powernow_register_driver(), and save one stub? ?

   -Sergiy
Re: [XEN PATCH v1] x86/cpufreq: separate powernow/hwp cpufreq code
Posted by Jan Beulich 5 months, 3 weeks ago 
On 06.06.2024 09:30, Sergiy Kibrik wrote:
> 06.06.24 10:08, Jan Beulich:
>> On 04.06.2024 11:34, Sergiy Kibrik wrote:
>>> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
>>> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
>>> @@ -657,7 +657,7 @@ static int __init cf_check cpufreq_driver_init(void)
>>>   
>>>           case X86_VENDOR_AMD:
>>>           case X86_VENDOR_HYGON:
>>> -            ret = powernow_register_driver();
>>> +            ret = IS_ENABLED(CONFIG_AMD) ? powernow_register_driver() : -ENODEV;
>>>               break;
>>>           }
>>
>> What about the Intel-specific code immediately up from here?
>> Dealing with that as well may likely permit to reduce ...
> 
> you mean to guard a call to hwp_register_driver() the same way as for 
> powernow_register_driver(), and save one stub? ?

Yes, and perhaps more. Maybe more stubs can be avoided? And
acpi_cpufreq_driver doesn't need registering either, and hence
would presumably be left unreferenced when !INTEL?

Jan
Re: [XEN PATCH v1] x86/cpufreq: separate powernow/hwp cpufreq code
Posted by Sergiy Kibrik 5 months, 2 weeks ago 
06.06.24 10:54, Jan Beulich:
> On 06.06.2024 09:30, Sergiy Kibrik wrote:
>> 06.06.24 10:08, Jan Beulich:
>>> On 04.06.2024 11:34, Sergiy Kibrik wrote:
>>>> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
>>>> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
>>>> @@ -657,7 +657,7 @@ static int __init cf_check cpufreq_driver_init(void)
>>>>    
>>>>            case X86_VENDOR_AMD:
>>>>            case X86_VENDOR_HYGON:
>>>> -            ret = powernow_register_driver();
>>>> +            ret = IS_ENABLED(CONFIG_AMD) ? powernow_register_driver() : -ENODEV;
>>>>                break;
>>>>            }
>>>
>>> What about the Intel-specific code immediately up from here?
>>> Dealing with that as well may likely permit to reduce ...
>>
>> you mean to guard a call to hwp_register_driver() the same way as for
>> powernow_register_driver(), and save one stub? ?
> 
> Yes, and perhaps more. Maybe more stubs can be avoided? And
> acpi_cpufreq_driver doesn't need registering either, and hence
> would presumably be left unreferenced when !INTEL?
> 

{get,set}_hwp_para() can be avoided, as they're being called just once 
and may be guarded by IS_ENABLED(CONFIG_INTEL).
The same for hwp_cmdline_parse().
As for hwp_active() it's being used many times by generic cpufreq code 
and even outside of cpufreq, so probably it has to be either a stub, or 
be moved outside of hwp.c and become smth, like this:

  bool hwp_active(void)
  {
     return IS_ENABLED(CONFIG_INTEL) && hwp_in_use;
  }

Though I'm not sure such movement would be any better than a stub.

acpi_cpufreq_driver, i.e. the most of code in cpufreq.c file, can 
probably be separated into acpi.c and put under CONFIG_INTEL as well. 
What you think of this?

  -Sergiy
Re: [XEN PATCH v1] x86/cpufreq: separate powernow/hwp cpufreq code
Posted by Jan Beulich 5 months, 2 weeks ago 
On 07.06.2024 11:14, Sergiy Kibrik wrote:
> 06.06.24 10:54, Jan Beulich:
>> On 06.06.2024 09:30, Sergiy Kibrik wrote:
>>> 06.06.24 10:08, Jan Beulich:
>>>> On 04.06.2024 11:34, Sergiy Kibrik wrote:
>>>>> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
>>>>> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
>>>>> @@ -657,7 +657,7 @@ static int __init cf_check cpufreq_driver_init(void)
>>>>>    
>>>>>            case X86_VENDOR_AMD:
>>>>>            case X86_VENDOR_HYGON:
>>>>> -            ret = powernow_register_driver();
>>>>> +            ret = IS_ENABLED(CONFIG_AMD) ? powernow_register_driver() : -ENODEV;
>>>>>                break;
>>>>>            }
>>>>
>>>> What about the Intel-specific code immediately up from here?
>>>> Dealing with that as well may likely permit to reduce ...
>>>
>>> you mean to guard a call to hwp_register_driver() the same way as for
>>> powernow_register_driver(), and save one stub? ?
>>
>> Yes, and perhaps more. Maybe more stubs can be avoided? And
>> acpi_cpufreq_driver doesn't need registering either, and hence
>> would presumably be left unreferenced when !INTEL?
>>
> 
> {get,set}_hwp_para() can be avoided, as they're being called just once 
> and may be guarded by IS_ENABLED(CONFIG_INTEL).
> The same for hwp_cmdline_parse().
> As for hwp_active() it's being used many times by generic cpufreq code 
> and even outside of cpufreq, so probably it has to be either a stub, or 
> be moved outside of hwp.c and become smth, like this:
> 
>   bool hwp_active(void)
>   {
>      return IS_ENABLED(CONFIG_INTEL) && hwp_in_use;
>   }
> 
> Though I'm not sure such movement would be any better than a stub.
> 
> acpi_cpufreq_driver, i.e. the most of code in cpufreq.c file, can 
> probably be separated into acpi.c and put under CONFIG_INTEL as well. 
> What you think of this?

Sounds like the direction I think we want to be following.

Jan
Re: [XEN PATCH v1] x86/cpufreq: separate powernow/hwp cpufreq code
Posted by Sergiy Kibrik 5 months, 2 weeks ago 
>> Though I'm not sure such movement would be any better than a stub.
>>
>> acpi_cpufreq_driver, i.e. the most of code in cpufreq.c file, can
>> probably be separated into acpi.c and put under CONFIG_INTEL as well.
>> What you think of this?
> 
> Sounds like the direction I think we want to be following.
> 

Sure, then I'll make a series for that.

   -Sergiy

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.