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

[Jan Beulich <jbeulich@xxxxxxxx>]

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