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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxx>
> CC: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v2 patch here:
> https://lore.kernel.org/xen-devel/20240801084453.1163506-1-Sergiy_Kibrik@xxxxxxxx/
>
> 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