Re: [PATCH v4 05/11] xsm: refactor xsm_ops handling

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 03/09/2021 20:06, Daniel P. Smith wrote:
> This renames the `struct xsm_operations` to the shorter `struct xsm_ops` and
> converts the global xsm_ops from being a pointer to an explicit instance. As
> part of this conversion, it reworks the XSM modules init function to return
> their xsm_ops struct which is copied in to the global xsm_ops instance.
>
> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>

Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

However, some suggestions...

> diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c
> index 55483292c5..859af3fe9a 100644
> --- a/xen/xsm/xsm_core.c
> +++ b/xen/xsm/xsm_core.c
> @@ -28,9 +28,17 @@
>  #include <asm/setup.h>
>  #endif
>  
> -#define XSM_FRAMEWORK_VERSION    "1.0.0"
> +#define XSM_FRAMEWORK_VERSION    "1.0.1"
>  
> -struct xsm_operations *xsm_ops;
> +struct xsm_ops __read_mostly xsm_ops;
> +
> +enum xsm_ops_state {
> +    XSM_OPS_UNREGISTERED,
> +    XSM_OPS_REG_FAILED,
> +    XSM_OPS_REGISTERED,
> +};
> +
> +static enum xsm_ops_state xsm_ops_registered = XSM_OPS_UNREGISTERED;

__read_mostly, or can this be __initdata ?

> @@ -87,25 +88,35 @@ static int __init xsm_core_init(const void 
> *policy_buffer, size_t policy_size)
>      }
>  #endif
>  
> -    if ( verify(&dummy_xsm_ops) )
> +    if ( xsm_ops_registered != XSM_OPS_UNREGISTERED )
>      {
> -        printk(XENLOG_ERR "Could not verify dummy_xsm_ops structure\n");
> +        printk(XENLOG_ERR
> +               "Could not init XSM, xsm_ops register already attempted\n");
>          return -EIO;
>      }
>  
> -    xsm_ops = &dummy_xsm_ops;
> -
>      switch ( xsm_bootparam )
>      {
>      case XSM_BOOTPARAM_DUMMY:
> +        xsm_ops_registered = XSM_OPS_REGISTERED;
>          break;
>  
>      case XSM_BOOTPARAM_FLASK:
> -        flask_init(policy_buffer, policy_size);
> +        ops = flask_init(policy_buffer, policy_size);
> +        if ( ops )
> +        {
> +            xsm_ops_registered = XSM_OPS_REGISTERED;
> +            xsm_ops = *ops;
> +        }
>          break;
>  
>      case XSM_BOOTPARAM_SILO:
> -        silo_init();
> +        ops = silo_init();
> +        if ( ops )
> +        {
> +            xsm_ops_registered = XSM_OPS_REGISTERED;
> +            xsm_ops = *ops;
> +        }

This if ( ops ) block can be deduplicated by moving after the switch()
statement.  It's going to be common to all everything except dummy.

~Andrew