| Message ID | 20240606083908.2510396-1-Sergiy_Kibrik@epam.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [XEN,v1] x86/intel: optional build of PSR support | expand |
On 06.06.2024 10:39, Sergiy Kibrik wrote: > --- a/xen/arch/x86/domctl.c > +++ b/xen/arch/x86/domctl.c > @@ -1160,6 +1160,7 @@ long arch_do_domctl( > break; > } > > +#ifdef CONFIG_INTEL > case XEN_DOMCTL_psr_cmt_op: > if ( !psr_cmt_enabled() ) > { > @@ -1262,6 +1263,7 @@ long arch_do_domctl( > } > > break; > +#endif Imo the result thereof shouldn't be -ENOSYS, but -EOPNOTSUPP (at least for XEN_DOMCTL_psr_alloc; for XEN_DOMCTL_psr_cmt_op it shouldn't change, even if I consider this wrong, but that's a separate topic). Wouldn't it be possible here to reduce the #ifdef scope a little to just most of the inner switch()es (i.e. requiring it to be split into two regions), whose "default" case is already (kind of) doing what we want? > --- a/xen/arch/x86/include/asm/psr.h > +++ b/xen/arch/x86/include/asm/psr.h > @@ -72,6 +72,8 @@ static inline bool psr_cmt_enabled(void) > return !!psr_cmt; > } > > +#ifdef CONFIG_INTEL > + > int psr_alloc_rmid(struct domain *d); > void psr_free_rmid(struct domain *d); > void psr_ctxt_switch_to(struct domain *d); > @@ -86,6 +88,19 @@ int psr_set_val(struct domain *d, unsigned int socket, > void psr_domain_init(struct domain *d); > void psr_domain_free(struct domain *d); > > +#else > + > +static inline void psr_ctxt_switch_to(struct domain *d) > +{ > +} > +static inline void psr_domain_init(struct domain *d) > +{ > +} > +static inline void psr_domain_free(struct domain *d) > +{ > +} > +#endif /* CONFIG_INTEL */ As I think I did mention elsewhere, such stubs can have the braces on the same line the the function specifier. > @@ -169,6 +171,7 @@ long arch_do_sysctl( > } > break; > > +#ifdef CONFIG_INTEL > case XEN_SYSCTL_psr_cmt_op: > if ( !psr_cmt_enabled() ) > return -ENODEV; > @@ -286,6 +289,7 @@ long arch_do_sysctl( > } > break; > } > +#endif Like for domctl I think you want to reduce the #ifdef scope some, even if for XEN_SYSCTL_psr_cmt_op that'll still result in -ENOSYS. At least we'll then be consistent in clearing sysctl->u.psr_cmt_op.u.data there. Jan
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index d902fb7acc..02218d32c5 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -57,7 +57,7 @@ obj-y += pci.o obj-y += percpu.o obj-y += physdev.o obj-$(CONFIG_COMPAT) += x86_64/physdev.o -obj-y += psr.o +obj-$(CONFIG_INTEL) += psr.o obj-y += setup.o obj-y += shutdown.o obj-y += smp.o diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c index 9a72d57333..cccf71f745 100644 --- a/xen/arch/x86/domctl.c +++ b/xen/arch/x86/domctl.c @@ -1160,6 +1160,7 @@ long arch_do_domctl( break; } +#ifdef CONFIG_INTEL case XEN_DOMCTL_psr_cmt_op: if ( !psr_cmt_enabled() ) { @@ -1262,6 +1263,7 @@ long arch_do_domctl( } break; +#endif case XEN_DOMCTL_get_cpu_policy: /* Process the CPUID leaves. */ diff --git a/xen/arch/x86/include/asm/psr.h b/xen/arch/x86/include/asm/psr.h index 51df78794c..14d5d33970 100644 --- a/xen/arch/x86/include/asm/psr.h +++ b/xen/arch/x86/include/asm/psr.h @@ -72,6 +72,8 @@ static inline bool psr_cmt_enabled(void) return !!psr_cmt; } +#ifdef CONFIG_INTEL + int psr_alloc_rmid(struct domain *d); void psr_free_rmid(struct domain *d); void psr_ctxt_switch_to(struct domain *d); @@ -86,6 +88,19 @@ int psr_set_val(struct domain *d, unsigned int socket, void psr_domain_init(struct domain *d); void psr_domain_free(struct domain *d); +#else + +static inline void psr_ctxt_switch_to(struct domain *d) +{ +} +static inline void psr_domain_init(struct domain *d) +{ +} +static inline void psr_domain_free(struct domain *d) +{ +} +#endif /* CONFIG_INTEL */ + #endif /* __ASM_PSR_H__ */ /* diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c index 1d40d82c5a..947c954b42 100644 --- a/xen/arch/x86/sysctl.c +++ b/xen/arch/x86/sysctl.c @@ -32,6 +32,7 @@ #include <asm/psr.h> #include <asm/cpu-policy.h> +#ifdef CONFIG_INTEL struct l3_cache_info { int ret; unsigned long size; @@ -46,6 +47,7 @@ static void cf_check l3_cache_get(void *arg) if ( !l3_info->ret ) l3_info->size = info.size / 1024; /* in KB unit */ } +#endif static long cf_check smt_up_down_helper(void *data) { @@ -169,6 +171,7 @@ long arch_do_sysctl( } break; +#ifdef CONFIG_INTEL case XEN_SYSCTL_psr_cmt_op: if ( !psr_cmt_enabled() ) return -ENODEV; @@ -286,6 +289,7 @@ long arch_do_sysctl( } break; } +#endif case XEN_SYSCTL_get_cpu_levelling_caps: sysctl->u.cpu_levelling_caps.caps = levelling_caps;
Platform Shared Resource feature is available for certain Intel's CPUs only, hence can be put under CONFIG_INTEL build option. When !INTEL then PSR-related sysctls XEN_SYSCTL_psr_cmt_op & XEN_SYSCTL_psr_alloc are off as well. Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com> --- xen/arch/x86/Makefile | 2 +- xen/arch/x86/domctl.c | 2 ++ xen/arch/x86/include/asm/psr.h | 15 +++++++++++++++ xen/arch/x86/sysctl.c | 4 ++++ 4 files changed, 22 insertions(+), 1 deletion(-)