xen/arch/x86/acpi/cpufreq/cpufreq.c | 2 +- xen/arch/x86/acpi/cpufreq/powernow.c | 8 ++++---- xen/drivers/cpufreq/cpufreq.c | 10 +++++----- xen/drivers/cpufreq/cpufreq_ondemand.c | 2 +- xen/include/acpi/cpufreq/cpufreq.h | 5 ----- xen/include/public/platform.h | 6 +++++- 6 files changed, 16 insertions(+), 17 deletions(-)
The values set in the shared_type field of xen_processor_performance
have so far relied on Xen and Linux having the same
CPUFREQ_SHARED_TYPE_ defines, as those have never been part of the
public interface.
Formalize by adding the defines for the allowed values in the public
header, while renaming them to use the XEN_PERF_SHARED_TYPE_ prefix
for clarity.
Fixes: 2fa7bee0a0 ('Get ACPI Px from dom0 and choose Px controller')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
I wonder if we want to keep the CPUFREQ_SHARED_TYPE_ defines for
internal usage (and define them based on XEN_PERF_SHARED_TYPE_) in
case we need to pick up changes from Linux.
---
xen/arch/x86/acpi/cpufreq/cpufreq.c | 2 +-
xen/arch/x86/acpi/cpufreq/powernow.c | 8 ++++----
xen/drivers/cpufreq/cpufreq.c | 10 +++++-----
xen/drivers/cpufreq/cpufreq_ondemand.c | 2 +-
xen/include/acpi/cpufreq/cpufreq.h | 5 -----
xen/include/public/platform.h | 6 +++++-
6 files changed, 16 insertions(+), 17 deletions(-)
diff --git a/xen/arch/x86/acpi/cpufreq/cpufreq.c b/xen/arch/x86/acpi/cpufreq/cpufreq.c
index c27cbb2304..200c6ac851 100644
--- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
+++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
@@ -426,7 +426,7 @@ static int cf_check acpi_cpufreq_target(
return -ENODEV;
}
- if (policy->shared_type != CPUFREQ_SHARED_TYPE_ANY)
+ if (policy->shared_type != XEN_PERF_SHARED_TYPE_ANY)
cmd.mask = &online_policy_cpus;
else
cmd.mask = cpumask_of(policy->cpu);
diff --git a/xen/arch/x86/acpi/cpufreq/powernow.c b/xen/arch/x86/acpi/cpufreq/powernow.c
index d4c7dcd5d9..e03a079ecb 100644
--- a/xen/arch/x86/acpi/cpufreq/powernow.c
+++ b/xen/arch/x86/acpi/cpufreq/powernow.c
@@ -109,7 +109,7 @@ static int cf_check powernow_cpufreq_target(
return 0;
}
- if (policy->shared_type == CPUFREQ_SHARED_TYPE_HW &&
+ if (policy->shared_type == XEN_PERF_SHARED_TYPE_HW &&
likely(policy->cpu == smp_processor_id())) {
transition_pstate(&next_perf_state);
cpufreq_statistic_update(policy->cpu, perf->state, next_perf_state);
@@ -119,7 +119,7 @@ static int cf_check powernow_cpufreq_target(
cpumask_and(&online_policy_cpus, policy->cpus, &cpu_online_map);
- if (policy->shared_type == CPUFREQ_SHARED_TYPE_ALL ||
+ if (policy->shared_type == XEN_PERF_SHARED_TYPE_ALL ||
unlikely(policy->cpu != smp_processor_id()))
on_selected_cpus(&online_policy_cpus, transition_pstate,
&next_perf_state, 1);
@@ -220,8 +220,8 @@ static int cf_check powernow_cpufreq_cpu_init(struct cpufreq_policy *policy)
info.perf = perf = data->acpi_data;
policy->shared_type = perf->shared_type;
- if (policy->shared_type == CPUFREQ_SHARED_TYPE_ALL ||
- policy->shared_type == CPUFREQ_SHARED_TYPE_ANY) {
+ if (policy->shared_type == XEN_PERF_SHARED_TYPE_ALL ||
+ policy->shared_type == XEN_PERF_SHARED_TYPE_ANY) {
cpumask_set_cpu(cpu, policy->cpus);
if (cpumask_weight(policy->cpus) != 1) {
printk(XENLOG_WARNING "Unsupported sharing type %d (%u CPUs)\n",
diff --git a/xen/drivers/cpufreq/cpufreq.c b/xen/drivers/cpufreq/cpufreq.c
index a94520ee57..64ed8ab1d3 100644
--- a/xen/drivers/cpufreq/cpufreq.c
+++ b/xen/drivers/cpufreq/cpufreq.c
@@ -179,7 +179,7 @@ int cpufreq_add_cpu(unsigned int cpu)
if (per_cpu(cpufreq_cpu_policy, cpu))
return 0;
- if (perf->shared_type == CPUFREQ_SHARED_TYPE_HW)
+ if (perf->shared_type == XEN_PERF_SHARED_TYPE_HW)
hw_all = 1;
dom = perf->domain_info.domain;
@@ -334,7 +334,7 @@ int cpufreq_del_cpu(unsigned int cpu)
if (!per_cpu(cpufreq_cpu_policy, cpu))
return 0;
- if (perf->shared_type == CPUFREQ_SHARED_TYPE_HW)
+ if (perf->shared_type == XEN_PERF_SHARED_TYPE_HW)
hw_all = 1;
dom = perf->domain_info.domain;
@@ -504,9 +504,9 @@ int set_px_pminfo(uint32_t acpi_id, struct xen_processor_performance *dom0_px_in
if ( dom0_px_info->flags & XEN_PX_PSD )
{
/* check domain coordination */
- if (dom0_px_info->shared_type != CPUFREQ_SHARED_TYPE_ALL &&
- dom0_px_info->shared_type != CPUFREQ_SHARED_TYPE_ANY &&
- dom0_px_info->shared_type != CPUFREQ_SHARED_TYPE_HW)
+ if (dom0_px_info->shared_type != XEN_PERF_SHARED_TYPE_ALL &&
+ dom0_px_info->shared_type != XEN_PERF_SHARED_TYPE_ANY &&
+ dom0_px_info->shared_type != XEN_PERF_SHARED_TYPE_HW)
{
ret = -EINVAL;
goto out;
diff --git a/xen/drivers/cpufreq/cpufreq_ondemand.c b/xen/drivers/cpufreq/cpufreq_ondemand.c
index fbcd14d6c3..ece90cb45c 100644
--- a/xen/drivers/cpufreq/cpufreq_ondemand.c
+++ b/xen/drivers/cpufreq/cpufreq_ondemand.c
@@ -195,7 +195,7 @@ static void dbs_timer_init(struct cpu_dbs_info_s *dbs_info)
set_timer(&per_cpu(dbs_timer, dbs_info->cpu), NOW()+dbs_tuners_ins.sampling_rate);
if ( processor_pminfo[dbs_info->cpu]->perf.shared_type
- == CPUFREQ_SHARED_TYPE_HW )
+ == XEN_PERF_SHARED_TYPE_HW )
{
dbs_info->stoppable = 1;
}
diff --git a/xen/include/acpi/cpufreq/cpufreq.h b/xen/include/acpi/cpufreq/cpufreq.h
index e5e58c6c30..6e4dcc99c5 100644
--- a/xen/include/acpi/cpufreq/cpufreq.h
+++ b/xen/include/acpi/cpufreq/cpufreq.h
@@ -78,11 +78,6 @@ DECLARE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_policy);
extern int __cpufreq_set_policy(struct cpufreq_policy *data,
struct cpufreq_policy *policy);
-#define CPUFREQ_SHARED_TYPE_NONE (0) /* None */
-#define CPUFREQ_SHARED_TYPE_HW (1) /* HW does needed coordination */
-#define CPUFREQ_SHARED_TYPE_ALL (2) /* All dependent CPUs should set freq */
-#define CPUFREQ_SHARED_TYPE_ANY (3) /* Freq can be set from any dependent CPU*/
-
/******************** cpufreq transition notifiers *******************/
struct cpufreq_freqs {
diff --git a/xen/include/public/platform.h b/xen/include/public/platform.h
index a4c0eb6224..0198d249ef 100644
--- a/xen/include/public/platform.h
+++ b/xen/include/public/platform.h
@@ -465,7 +465,11 @@ struct xen_processor_performance {
uint32_t state_count; /* total available performance states */
XEN_GUEST_HANDLE(xen_processor_px_t) states;
struct xen_psd_package domain_info;
- uint32_t shared_type; /* coordination type of this processor */
+ /* Coordination type of this processor */
+#define XEN_PERF_SHARED_TYPE_HW 1 /* HW does needed coordination */
+#define XEN_PERF_SHARED_TYPE_ALL 2 /* All dependent CPUs should set freq */
+#define XEN_PERF_SHARED_TYPE_ANY 3 /* Freq can be set from any dependent CPU */
+ uint32_t shared_type;
};
typedef struct xen_processor_performance xen_processor_performance_t;
DEFINE_XEN_GUEST_HANDLE(xen_processor_performance_t);
--
2.35.1
On 06.04.2022 17:16, Roger Pau Monne wrote:
> The values set in the shared_type field of xen_processor_performance
> have so far relied on Xen and Linux having the same
> CPUFREQ_SHARED_TYPE_ defines, as those have never been part of the
> public interface.
>
> Formalize by adding the defines for the allowed values in the public
> header, while renaming them to use the XEN_PERF_SHARED_TYPE_ prefix
> for clarity.
>
> Fixes: 2fa7bee0a0 ('Get ACPI Px from dom0 and choose Px controller')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> I wonder if we want to keep the CPUFREQ_SHARED_TYPE_ defines for
> internal usage (and define them based on XEN_PERF_SHARED_TYPE_) in
> case we need to pick up changes from Linux.
I think that would be desirable, also to limit code churn by this change.
> --- a/xen/include/public/platform.h
> +++ b/xen/include/public/platform.h
> @@ -465,7 +465,11 @@ struct xen_processor_performance {
> uint32_t state_count; /* total available performance states */
> XEN_GUEST_HANDLE(xen_processor_px_t) states;
> struct xen_psd_package domain_info;
> - uint32_t shared_type; /* coordination type of this processor */
> + /* Coordination type of this processor */
> +#define XEN_PERF_SHARED_TYPE_HW 1 /* HW does needed coordination */
> +#define XEN_PERF_SHARED_TYPE_ALL 2 /* All dependent CPUs should set freq */
> +#define XEN_PERF_SHARED_TYPE_ANY 3 /* Freq can be set from any dependent CPU */
While the names may then become a little long, I think it would be
relevant to have "processor" (or maybe "CPU") in the names, to have
a better match with struct xen_processor_performance. Also I'm not
sure you want to define these inside the struct - they're
supposedly suitable for Px, Cx, and Tx aiui (just like the underlying
ACPI constants are).
Jan
On Wed, Apr 06, 2022 at 05:31:22PM +0200, Jan Beulich wrote:
> On 06.04.2022 17:16, Roger Pau Monne wrote:
> > The values set in the shared_type field of xen_processor_performance
> > have so far relied on Xen and Linux having the same
> > CPUFREQ_SHARED_TYPE_ defines, as those have never been part of the
> > public interface.
> >
> > Formalize by adding the defines for the allowed values in the public
> > header, while renaming them to use the XEN_PERF_SHARED_TYPE_ prefix
> > for clarity.
> >
> > Fixes: 2fa7bee0a0 ('Get ACPI Px from dom0 and choose Px controller')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > I wonder if we want to keep the CPUFREQ_SHARED_TYPE_ defines for
> > internal usage (and define them based on XEN_PERF_SHARED_TYPE_) in
> > case we need to pick up changes from Linux.
>
> I think that would be desirable, also to limit code churn by this change.
>
> > --- a/xen/include/public/platform.h
> > +++ b/xen/include/public/platform.h
> > @@ -465,7 +465,11 @@ struct xen_processor_performance {
> > uint32_t state_count; /* total available performance states */
> > XEN_GUEST_HANDLE(xen_processor_px_t) states;
> > struct xen_psd_package domain_info;
> > - uint32_t shared_type; /* coordination type of this processor */
> > + /* Coordination type of this processor */
> > +#define XEN_PERF_SHARED_TYPE_HW 1 /* HW does needed coordination */
> > +#define XEN_PERF_SHARED_TYPE_ALL 2 /* All dependent CPUs should set freq */
> > +#define XEN_PERF_SHARED_TYPE_ANY 3 /* Freq can be set from any dependent CPU */
>
> While the names may then become a little long, I think it would be
> relevant to have "processor" (or maybe "CPU") in the names, to have
> a better match with struct xen_processor_performance. Also I'm not
> sure you want to define these inside the struct - they're
> supposedly suitable for Px, Cx, and Tx aiui (just like the underlying
> ACPI constants are).
But those defines are specific to CPUFREQ code, the raw values from
the ACPI tables for the different 'coordination' fields found in the
Cx and Px states use different values, ie:
#define ACPI_DOMAIN_COORD_TYPE_SW_ALL 0xfc
#define ACPI_DOMAIN_COORD_TYPE_SW_ANY 0xfd
#define ACPI_DOMAIN_COORD_TYPE_HW_ALL 0xfe
And hence the values exposed here should be limited to the existing
usage by the xen_processor_performance struct. Otherwise it would be
preferred to use the native ACPI values, which don't need exposing in
the Xen headers then because are already part of a different
specification. IOW I think it was a mistake for the shared_type field
to use the CPUFREQ defines instead of the ACPI values.
Note the coord_type field in xen_psd_package and xen_processor_csd do
expect the ACPI values instead of the CPUFREQ ones.
Thanks, Roger.
On 06.04.2022 17:51, Roger Pau Monné wrote:
> On Wed, Apr 06, 2022 at 05:31:22PM +0200, Jan Beulich wrote:
>> On 06.04.2022 17:16, Roger Pau Monne wrote:
>>> The values set in the shared_type field of xen_processor_performance
>>> have so far relied on Xen and Linux having the same
>>> CPUFREQ_SHARED_TYPE_ defines, as those have never been part of the
>>> public interface.
>>>
>>> Formalize by adding the defines for the allowed values in the public
>>> header, while renaming them to use the XEN_PERF_SHARED_TYPE_ prefix
>>> for clarity.
>>>
>>> Fixes: 2fa7bee0a0 ('Get ACPI Px from dom0 and choose Px controller')
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> ---
>>> I wonder if we want to keep the CPUFREQ_SHARED_TYPE_ defines for
>>> internal usage (and define them based on XEN_PERF_SHARED_TYPE_) in
>>> case we need to pick up changes from Linux.
>>
>> I think that would be desirable, also to limit code churn by this change.
>>
>>> --- a/xen/include/public/platform.h
>>> +++ b/xen/include/public/platform.h
>>> @@ -465,7 +465,11 @@ struct xen_processor_performance {
>>> uint32_t state_count; /* total available performance states */
>>> XEN_GUEST_HANDLE(xen_processor_px_t) states;
>>> struct xen_psd_package domain_info;
>>> - uint32_t shared_type; /* coordination type of this processor */
>>> + /* Coordination type of this processor */
>>> +#define XEN_PERF_SHARED_TYPE_HW 1 /* HW does needed coordination */
>>> +#define XEN_PERF_SHARED_TYPE_ALL 2 /* All dependent CPUs should set freq */
>>> +#define XEN_PERF_SHARED_TYPE_ANY 3 /* Freq can be set from any dependent CPU */
>>
>> While the names may then become a little long, I think it would be
>> relevant to have "processor" (or maybe "CPU") in the names, to have
>> a better match with struct xen_processor_performance. Also I'm not
>> sure you want to define these inside the struct - they're
>> supposedly suitable for Px, Cx, and Tx aiui (just like the underlying
>> ACPI constants are).
>
> But those defines are specific to CPUFREQ code, the raw values from
> the ACPI tables for the different 'coordination' fields found in the
> Cx and Px states use different values, ie:
>
> #define ACPI_DOMAIN_COORD_TYPE_SW_ALL 0xfc
> #define ACPI_DOMAIN_COORD_TYPE_SW_ANY 0xfd
> #define ACPI_DOMAIN_COORD_TYPE_HW_ALL 0xfe
>
> And hence the values exposed here should be limited to the existing
> usage by the xen_processor_performance struct.
Oh, yes, sorry.
> Otherwise it would be
> preferred to use the native ACPI values, which don't need exposing in
> the Xen headers then because are already part of a different
> specification. IOW I think it was a mistake for the shared_type field
> to use the CPUFREQ defines instead of the ACPI values.
I agree, but we can't do anything about this now.
Jan
© 2016 - 2024 Red Hat, Inc.