[XEN,v2] x86/intel: optional build of PSR support

Commit Message

Sergiy Kibrik Aug. 1, 2024, 8:44 a.m. UTC

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 <[e-mail]>
CC: Jan Beulich <[e-mail]>
---
v1 patch here:
https://lore.kernel.org/xen-devel/[e-mail]/

changes in v2:
 - split outer switch cases for {XEN_SYSCTL,XEN_DOMCTL}_psr_cmt_op and
   {XEN_SYSCTL,XEN_DOMCTL}_psr_alloc so that return codes in default switch
   cases are not mangled (as Jan suggested)
---
 xen/arch/x86/Makefile          |  2 +-
 xen/arch/x86/domctl.c          |  8 ++++++++
 xen/arch/x86/include/asm/psr.h |  9 +++++++++
 xen/arch/x86/sysctl.c          | 12 +++++++++++-
 4 files changed, 29 insertions(+), 2 deletions(-)

Comments

Jan Beulich Aug. 8, 2024, 7:11 a.m. UTC | #1

On 01.08.2024 10:44, Sergiy Kibrik wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -1162,6 +1162,7 @@ long arch_do_domctl(
>      }
>  
>      case XEN_DOMCTL_psr_cmt_op:
> +#ifdef CONFIG_INTEL
>          if ( !psr_cmt_enabled() )
>          {
>              ret = -ENODEV;
> @@ -1190,11 +1191,16 @@ long arch_do_domctl(
>              ret = -ENOSYS;

This pre-existing use of -ENOSYS is imo wrong; should be -EINVAL or -EOPNOTSUPP.

>              break;
>          }
> +
> +#else /* CONFIG_INTEL */
> +        ret = -ENOSYS;

This use therefore is imo wrong, too. However, can't we avoid #ifdef-ary here
altogether by supplying a stub psr_cmt_enabled() (plus keeping the decls
visible for psr_{alloc,free}_rmid())? Similarly for the respective code in
sysctl.c then.

> +#endif
>          break;
>  
>      case XEN_DOMCTL_psr_alloc:
>          switch ( domctl->u.psr_alloc.cmd )
>          {
> +#ifdef CONFIG_INTEL
>          case XEN_DOMCTL_PSR_SET_L3_CBM:
>              ret = psr_set_val(d, domctl->u.psr_alloc.target,
>                                domctl->u.psr_alloc.data,
> @@ -1257,6 +1263,8 @@ long arch_do_domctl(
>  
>  #undef domctl_psr_get_val
>  
> +#endif /* CONFIG_INTEL */
> +
>          default:
>              ret = -EOPNOTSUPP;
>              break;

Here (and again for the respective sysctl code) otoh I'm okay with the #ifdef.

> @@ -225,10 +233,11 @@ long arch_do_sysctl(
>  
>      case XEN_SYSCTL_psr_alloc:
>      {
> -        uint32_t data[PSR_INFO_ARRAY_SIZE] = { };
> +        uint32_t __maybe_unused data[PSR_INFO_ARRAY_SIZE] = { };

Remark to Andrew: Leaving aside the fact that the initializer here stands in
the way of doing so, the need for this (imo ugly) attribute is one of the
reasons why generally I'd prefer such declarations to live ...

>          switch ( sysctl->u.psr_alloc.cmd )
>          {
> +#ifdef CONFIG_INTEL

... immediately inside the switch() accessing them.

Jan

Patch

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 68b5b46d1a..bccd3852a7 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1162,6 +1162,7 @@  long arch_do_domctl(
     }
 
     case XEN_DOMCTL_psr_cmt_op:
+#ifdef CONFIG_INTEL
         if ( !psr_cmt_enabled() )
         {
             ret = -ENODEV;
@@ -1190,11 +1191,16 @@  long arch_do_domctl(
             ret = -ENOSYS;
             break;
         }
+
+#else /* CONFIG_INTEL */
+        ret = -ENOSYS;
+#endif
         break;
 
     case XEN_DOMCTL_psr_alloc:
         switch ( domctl->u.psr_alloc.cmd )
         {
+#ifdef CONFIG_INTEL
         case XEN_DOMCTL_PSR_SET_L3_CBM:
             ret = psr_set_val(d, domctl->u.psr_alloc.target,
                               domctl->u.psr_alloc.data,
@@ -1257,6 +1263,8 @@  long arch_do_domctl(
 
 #undef domctl_psr_get_val
 
+#endif /* CONFIG_INTEL */
+
         default:
             ret = -EOPNOTSUPP;
             break;
diff --git a/xen/arch/x86/include/asm/psr.h b/xen/arch/x86/include/asm/psr.h
index 51df78794c..8a8c2b13d0 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,13 @@  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..b39b7f68de 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)
 {
@@ -170,6 +172,7 @@  long arch_do_sysctl(
     break;
 
     case XEN_SYSCTL_psr_cmt_op:
+#ifdef CONFIG_INTEL
         if ( !psr_cmt_enabled() )
             return -ENODEV;
 
@@ -218,6 +221,11 @@  long arch_do_sysctl(
             break;
         }
 
+#else /* CONFIG_INTEL*/
+        sysctl->u.psr_cmt_op.u.data = 0;
+        ret = -ENOSYS;
+#endif
+
         if ( __copy_to_guest(u_sysctl, sysctl, 1) )
             ret = -EFAULT;
 
@@ -225,10 +233,11 @@  long arch_do_sysctl(
 
     case XEN_SYSCTL_psr_alloc:
     {
-        uint32_t data[PSR_INFO_ARRAY_SIZE] = { };
+        uint32_t __maybe_unused data[PSR_INFO_ARRAY_SIZE] = { };
 
         switch ( sysctl->u.psr_alloc.cmd )
         {
+#ifdef CONFIG_INTEL
         case XEN_SYSCTL_PSR_get_l3_info:
             ret = psr_get_info(sysctl->u.psr_alloc.target,
                                PSR_TYPE_L3_CBM, data, ARRAY_SIZE(data));
@@ -279,6 +288,7 @@  long arch_do_sysctl(
             if ( __copy_field_to_guest(u_sysctl, sysctl, u.psr_alloc) )
                 ret = -EFAULT;
             break;
+#endif /* CONFIG_INTEL */
 
         default:
             ret = -EOPNOTSUPP;