Re: [PATCH 3/6] xsm: enabling xsm to always be included

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.06.2021 01:39, Daniel P. Smith wrote:
> The only difference between !CONFIG_XSM and CONFIG_XSM with !CONFIG_XSM_SILO 
> and !CONFIG_XSM_FLASK
> is whether the XSM hooks in dummy.h are called as static inline functions or 
> as function
> pointers to static functions. As such this commit,
>  * eliminates CONFIG_XSM

Following from what Andrew has said (including him mentioning your
changing of certain Kconfig option defaults), I'm not convinced this is
a good move. This still ought to serve as the overall XSM-yes-or-no
setting. If internally you make said two variants match in behavior,
that's a different thing.

>  * introduces CONFIG_XSM_EVTCHN_LABELING as replacement for enabling event 
> channel labels

Is this mode needed as separate functionality at all? Nothing defines
XSM_NEED_GENERIC_EVTCHN_SSID anywhere. _If_ XSM went away as a separate
setting, then imo this one should go away as well.

> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -197,22 +197,33 @@ config XENOPROF
>  
>         If unsure, say Y.
>  
> -config XSM
> -     bool "Xen Security Modules support"
> -     default ARM
> -     ---help---
> -       Enables the security framework known as Xen Security Modules which
> -       allows administrators fine-grained control over a Xen domain and
> -       its capabilities by defining permissible interactions between domains,
> -       the hypervisor itself, and related resources such as memory and
> -       devices.
> +menu "Xen Security Modules"
>  
> -       If unsure, say N.
> +choice
> +     prompt "Default XSM module"
> +     default XSM_SILO_DEFAULT if XSM_SILO && ARM
> +     default XSM_FLASK_DEFAULT if XSM_FLASK
> +     default XSM_SILO_DEFAULT if XSM_SILO
> +     default XSM_DUMMY_DEFAULT
> +     config XSM_DUMMY_DEFAULT
> +             bool "Match non-XSM behavior"
> +     config XSM_FLASK_DEFAULT
> +             bool "FLux Advanced Security Kernel" if XSM_FLASK
> +     config XSM_SILO_DEFAULT
> +             bool "SILO" if XSM_SILO
> +endchoice

This did live after the individual options it depends on for a reason,
and you don't say anywhere why you need to move it up. The way you
have it, with the default command line kconfig tool, users will be
presented with dependent options before having chosen the settings of
the dependency ones. That's because this tool, to a degree, moves
linearly through the options it has parsed.

> @@ -261,25 +271,12 @@ config XSM_SILO
>  
>         If unsure, say Y.
>  
> -choice
> -     prompt "Default XSM implementation"
> -     depends on XSM
> -     default XSM_SILO_DEFAULT if XSM_SILO && ARM
> -     default XSM_FLASK_DEFAULT if XSM_FLASK
> -     default XSM_SILO_DEFAULT if XSM_SILO
> -     default XSM_DUMMY_DEFAULT
> -     config XSM_DUMMY_DEFAULT
> -             bool "Match non-XSM behavior"
> -     config XSM_FLASK_DEFAULT
> -             bool "FLux Advanced Security Kernel" if XSM_FLASK
> -     config XSM_SILO_DEFAULT
> -             bool "SILO" if XSM_SILO
> -endchoice
> +endmenu
>  
>  config LATE_HWDOM
>       bool "Dedicated hardware domain"
>       default n
> -     depends on XSM && X86
> +     depends on XSM_FLASK && X86

I don't think this is a compatible change. I'm not going to exclude that
this is how it was meant, but as it stands LATE_HWDOM right now doesn't
really require FLASK, and could e.g. also go with SILO or dummy. If you
_mean_ to change this, then your description needs to say so (and ideally
it would then be split out, so - if this is actually a bug - it could
also be backported).

Jan