1. Patchew
  2. Xen
  3. View series

[XEN PATCH v3] x86/intel: optional build of PSR support

Sergiy Kibrik posted 1 patch 2 months, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20240829090559.149249-1-Sergiy._5FKibrik@epam.com
There is a newer version of this series
xen/arch/x86/Makefile          |  2 +-
xen/arch/x86/domain.c          | 12 ++++++++----
xen/arch/x86/domctl.c          |  3 +++
xen/arch/x86/include/asm/psr.h |  7 +++++++
xen/arch/x86/sysctl.c          |  4 +++-
5 files changed, 22 insertions(+), 6 deletions(-)
[XEN PATCH v3] x86/intel: optional build of PSR support
Posted by Sergiy Kibrik 2 months, 3 weeks ago 
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>
CC: Jan Beulich <jbeulich@suse.com>
---
v2 patch here:
https://lore.kernel.org/xen-devel/20240801084453.1163506-1-Sergiy_Kibrik@epam.com/

changes in v3:
 - drop stubs for psr_domain_{init,free} & psr_ctxt_switch_to() and guard these
   routines at call sites
 - add stub for psr_cmt_enabled()
 - drop some of #ifdef-s from arch_do_{domctl,sysctl}
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/domain.c          | 12 ++++++++----
 xen/arch/x86/domctl.c          |  3 +++
 xen/arch/x86/include/asm/psr.h |  7 +++++++
 xen/arch/x86/sysctl.c          |  4 +++-
 5 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 286c003ec3..4d1ba2a3a1 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/domain.c b/xen/arch/x86/domain.c
index 89aad7e897..ebe648ef80 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -861,7 +861,8 @@ int arch_domain_create(struct domain *d,
     if ( (rc = iommu_domain_init(d, config->iommu_opts)) != 0 )
         goto fail;
 
-    psr_domain_init(d);
+    if ( IS_ENABLED(CONFIG_INTEL) )
+        psr_domain_init(d);
 
     if ( is_hvm_domain(d) )
     {
@@ -902,7 +903,8 @@ int arch_domain_create(struct domain *d,
 
  fail:
     d->is_dying = DOMDYING_dead;
-    psr_domain_free(d);
+    if ( IS_ENABLED(CONFIG_INTEL) )
+        psr_domain_free(d);
     iommu_domain_destroy(d);
     cleanup_domain_irq_mapping(d);
     free_xenheap_page(d->shared_info);
@@ -940,7 +942,8 @@ void arch_domain_destroy(struct domain *d)
     free_xenheap_page(d->shared_info);
     cleanup_domain_irq_mapping(d);
 
-    psr_domain_free(d);
+    if ( IS_ENABLED(CONFIG_INTEL) )
+        psr_domain_free(d);
 }
 
 void arch_domain_shutdown(struct domain *d)
@@ -2024,7 +2027,8 @@ static void __context_switch(void)
         nd->arch.ctxt_switch->to(n);
     }
 
-    psr_ctxt_switch_to(nd);
+    if ( IS_ENABLED(CONFIG_INTEL) )
+        psr_ctxt_switch_to(nd);
 
     if ( need_full_gdt(nd) )
         update_xen_slot_in_full_gdt(n, cpu);
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 68b5b46d1a..bd8baaae20 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1195,6 +1195,7 @@ long arch_do_domctl(
     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 +1258,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..d42c7f1580 100644
--- a/xen/arch/x86/include/asm/psr.h
+++ b/xen/arch/x86/include/asm/psr.h
@@ -67,10 +67,17 @@ enum psr_type {
 
 extern struct psr_cmt *psr_cmt;
 
+#ifdef CONFIG_INTEL
 static inline bool psr_cmt_enabled(void)
 {
     return !!psr_cmt;
 }
+#else
+static inline bool psr_cmt_enabled(void)
+{
+    return false;
+}
+#endif /* CONFIG_INTEL */
 
 int psr_alloc_rmid(struct domain *d);
 void psr_free_rmid(struct domain *d);
diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index 1d40d82c5a..8b0d1f1e32 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -225,10 +225,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 +280,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;
-- 
2.25.1
Re: [XEN PATCH v3] x86/intel: optional build of PSR support
Posted by Andrew Cooper 2 months, 3 weeks ago 
On 29/08/2024 10:05 am, Sergiy Kibrik wrote:
> Platform Shared Resource feature is available for certain Intel's CPUs only,
> hence can be put under CONFIG_INTEL build option.

AMD implement PSR too, and even some extensions over what Intel implement.

Furthermore it is likely that the eventual automotive system will want
to make use of it to reduce interference between criticial and
non-critical VMs.

What is currently true is "Xen's implementation of PSR only supports
Intel CPUs right now".

But - I think it is wrong to tie this to CONFIG_INTEL.

Perhaps CONFIG_PSR which is selected by CONFIG_INTEL for now, which can
eventually become user selectable 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>
> CC: Jan Beulich <jbeulich@suse.com>
> ---
> v2 patch here:
> https://lore.kernel.org/xen-devel/20240801084453.1163506-1-Sergiy_Kibrik@epam.com/
>
> changes in v3:
>  - drop stubs for psr_domain_{init,free} & psr_ctxt_switch_to() and guard these
>    routines at call sites

Why?  That's error prone and contrary to how other subsystems work.

> diff --git a/xen/arch/x86/include/asm/psr.h b/xen/arch/x86/include/asm/psr.h
> index 51df78794c..d42c7f1580 100644
> --- a/xen/arch/x86/include/asm/psr.h
> +++ b/xen/arch/x86/include/asm/psr.h
> @@ -67,10 +67,17 @@ enum psr_type {
>  
>  extern struct psr_cmt *psr_cmt;
>  
> +#ifdef CONFIG_INTEL
>  static inline bool psr_cmt_enabled(void)
>  {
>      return !!psr_cmt;
>  }
> +#else
> +static inline bool psr_cmt_enabled(void)
> +{
> +    return false;
> +}

This would be better as simply:

static inline bool psr_cmt_enabled(void)
{
    return IS_ENABLED(CONFIG_blah) ? !!psr_cmt : false;
}

or if the worst comes to the worst,

static inline bool psr_cmt_enabled(void)
{
#ifdef CONFIG_blah
    return !!psr_cmt;
#else
    return false;
#endif
}

Both cases leave you with only a single function declaration, which
helps grep/tags/cscope-ability.

~Andrew

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.