| Message ID | 20220407081828.38747-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] platform/cpufreq: add public defines for CPUFREQ_SHARED_TYPE_ | expand |
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
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.
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
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.
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);
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(-)