[PATCH v2] platform/cpufreq: add public defines for CPUFREQ_SHARED_TYPE_

[PATCH v2] platform/cpufreq: add public defines for CPUFREQ_SHARED_TYPE_

[Roger Pau Monne]

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_CPUPERF_SHARED_TYPE_ prefix
for clarity.

Set the Xen internal defines for CPUFREQ_SHARED_TYPE_ using the newly
introduced XEN_CPUPERF_SHARED_TYPE_ public defines in order to avoid
unnecessary code churn.  While there also drop
CPUFREQ_SHARED_TYPE_NONE as it's unused.

Fixes: 2fa7bee0a0 ('Get ACPI Px from dom0 and choose Px controller')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Keep CPUFREQ_SHARED_TYPE_ and define them on top of
   XEN_CPUPERF_SHARED_TYPE_.
 - Use CPUPERF instead of plain PERF.
---
 xen/include/acpi/cpufreq/cpufreq.h | 7 +++----
 xen/include/public/platform.h      | 6 +++++-
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/xen/include/acpi/cpufreq/cpufreq.h b/xen/include/acpi/cpufreq/cpufreq.h
index e5e58c6c30..35dcf21e8f 100644
--- a/xen/include/acpi/cpufreq/cpufreq.h
+++ b/xen/include/acpi/cpufreq/cpufreq.h
@@ -78,10 +78,9 @@ 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*/
+#define CPUFREQ_SHARED_TYPE_HW   XEN_CPUPERF_SHARED_TYPE_HW
+#define CPUFREQ_SHARED_TYPE_ALL  XEN_CPUPERF_SHARED_TYPE_ALL
+#define CPUFREQ_SHARED_TYPE_ANY  XEN_CPUPERF_SHARED_TYPE_ANY
 
 /******************** cpufreq transition notifiers *******************/
 
diff --git a/xen/include/public/platform.h b/xen/include/public/platform.h
index a4c0eb6224..8100133509 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_CPUPERF_SHARED_TYPE_HW   1 /* HW does needed coordination */
+#define XEN_CPUPERF_SHARED_TYPE_ALL  2 /* All dependent CPUs should set freq */
+#define XEN_CPUPERF_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

Re: [PATCH v2] platform/cpufreq: add public defines for CPUFREQ_SHARED_TYPE_

[Jan Beulich]

On 07.04.2022 10:18, 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_CPUPERF_SHARED_TYPE_ prefix
> for clarity.
> 
> Set the Xen internal defines for CPUFREQ_SHARED_TYPE_ using the newly
> introduced XEN_CPUPERF_SHARED_TYPE_ public defines in order to avoid
> unnecessary code churn.  While there also drop
> CPUFREQ_SHARED_TYPE_NONE as it's unused.
> 
> Fixes: 2fa7bee0a0 ('Get ACPI Px from dom0 and choose Px controller')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with one remark:

> --- a/xen/include/acpi/cpufreq/cpufreq.h
> +++ b/xen/include/acpi/cpufreq/cpufreq.h
> @@ -78,10 +78,9 @@ 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 */

I realize this is unused, but do we really want/need to drop this?
I think it is used implicitly right now by assuming the value would
be zero; this could do with making explicit, in which case we'd
need the #define.

Jan

Re: [PATCH v2] platform/cpufreq: add public defines for CPUFREQ_SHARED_TYPE_

[Roger Pau Monné]

On Thu, Apr 07, 2022 at 10:48:50AM +0200, Jan Beulich wrote:
> On 07.04.2022 10:18, 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_CPUPERF_SHARED_TYPE_ prefix
> > for clarity.
> > 
> > Set the Xen internal defines for CPUFREQ_SHARED_TYPE_ using the newly
> > introduced XEN_CPUPERF_SHARED_TYPE_ public defines in order to avoid
> > unnecessary code churn.  While there also drop
> > CPUFREQ_SHARED_TYPE_NONE as it's unused.
> > 
> > Fixes: 2fa7bee0a0 ('Get ACPI Px from dom0 and choose Px controller')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with one remark:
> 
> > --- a/xen/include/acpi/cpufreq/cpufreq.h
> > +++ b/xen/include/acpi/cpufreq/cpufreq.h
> > @@ -78,10 +78,9 @@ 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 */
> 
> I realize this is unused, but do we really want/need to drop this?
> I think it is used implicitly right now by assuming the value would
> be zero; this could do with making explicit, in which case we'd
> need the #define.

I don't think Xen uses it explicitly, all checks of shared_type are
always against a specific CPUFREQ_SHARED_TYPE_{HW,ALL,ANY}.

I don't have a strong opinion about keeping it however, I've just
removed as a cleanup, if you don't think it's helpful I can resubmit
keeping the define.

Thanks, Roger.

Re: [PATCH v2] platform/cpufreq: add public defines for CPUFREQ_SHARED_TYPE_

[Jan Beulich]

On 07.04.2022 11:22, Roger Pau Monné wrote:
> On Thu, Apr 07, 2022 at 10:48:50AM +0200, Jan Beulich wrote:
>> On 07.04.2022 10:18, 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_CPUPERF_SHARED_TYPE_ prefix
>>> for clarity.
>>>
>>> Set the Xen internal defines for CPUFREQ_SHARED_TYPE_ using the newly
>>> introduced XEN_CPUPERF_SHARED_TYPE_ public defines in order to avoid
>>> unnecessary code churn.  While there also drop
>>> CPUFREQ_SHARED_TYPE_NONE as it's unused.
>>>
>>> Fixes: 2fa7bee0a0 ('Get ACPI Px from dom0 and choose Px controller')
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> with one remark:
>>
>>> --- a/xen/include/acpi/cpufreq/cpufreq.h
>>> +++ b/xen/include/acpi/cpufreq/cpufreq.h
>>> @@ -78,10 +78,9 @@ 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 */
>>
>> I realize this is unused, but do we really want/need to drop this?
>> I think it is used implicitly right now by assuming the value would
>> be zero; this could do with making explicit, in which case we'd
>> need the #define.
> 
> I don't think Xen uses it explicitly, all checks of shared_type are
> always against a specific CPUFREQ_SHARED_TYPE_{HW,ALL,ANY}.

Well, I said "implicitly"; if there was an explicit reference, you'd
have run into a build failure. But I did check now - all comparisons of
->shared_type are against explicit CPUFREQ_SHARED_TYPE_*. So I guess
dropping the value is fine.

Jan

Re: [PATCH v2] platform/cpufreq: add public defines for CPUFREQ_SHARED_TYPE_

[Roger Pau Monné]

On Thu, Apr 07, 2022 at 11:35:20AM +0200, Jan Beulich wrote:
> On 07.04.2022 11:22, Roger Pau Monné wrote:
> > On Thu, Apr 07, 2022 at 10:48:50AM +0200, Jan Beulich wrote:
> >> On 07.04.2022 10:18, 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_CPUPERF_SHARED_TYPE_ prefix
> >>> for clarity.
> >>>
> >>> Set the Xen internal defines for CPUFREQ_SHARED_TYPE_ using the newly
> >>> introduced XEN_CPUPERF_SHARED_TYPE_ public defines in order to avoid
> >>> unnecessary code churn.  While there also drop
> >>> CPUFREQ_SHARED_TYPE_NONE as it's unused.
> >>>
> >>> Fixes: 2fa7bee0a0 ('Get ACPI Px from dom0 and choose Px controller')
> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >>
> >> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> >> with one remark:
> >>
> >>> --- a/xen/include/acpi/cpufreq/cpufreq.h
> >>> +++ b/xen/include/acpi/cpufreq/cpufreq.h
> >>> @@ -78,10 +78,9 @@ 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 */
> >>
> >> I realize this is unused, but do we really want/need to drop this?
> >> I think it is used implicitly right now by assuming the value would
> >> be zero; this could do with making explicit, in which case we'd
> >> need the #define.
> > 
> > I don't think Xen uses it explicitly, all checks of shared_type are
> > always against a specific CPUFREQ_SHARED_TYPE_{HW,ALL,ANY}.
> 
> Well, I said "implicitly"; if there was an explicit reference, you'd
> have run into a build failure. But I did check now - all comparisons of
> ->shared_type are against explicit CPUFREQ_SHARED_TYPE_*. So I guess
> dropping the value is fine.

Yes, that's what I've tried to explain, unsuccessfully it seems :), on
my reply.  I should have used 'explicit' instead of 'specific'.

Thanks, Roger.