Re: [PATCH] xsm: refactor and optimize policy loading

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.05.2022 17:40, Daniel P. Smith wrote:
> --- a/xen/xsm/xsm_core.c
> +++ b/xen/xsm/xsm_core.c
> @@ -55,19 +55,35 @@ static enum xsm_bootparam __initdata xsm_bootparam =
>      XSM_BOOTPARAM_DUMMY;
>  #endif
>  
> +static bool __initdata init_policy =
> +#ifdef CONFIG_XSM_FLASK_DEFAULT
> +    true;
> +#else
> +    false;
> +#endif

Simply IS_ENABLED(CONFIG_XSM_FLASK_DEFAULT) without any #ifdef-ary?

> @@ -148,11 +156,11 @@ int __init xsm_multiboot_init(
>  
>      printk("XSM Framework v" XSM_FRAMEWORK_VERSION " initialized\n");
>  
> -    if ( XSM_MAGIC )
> +    if ( init_policy && XSM_MAGIC )
>      {
>          ret = xsm_multiboot_policy_init(module_map, mbi, &policy_buffer,
>                                          &policy_size);
> -        if ( ret )
> +        if ( ret != 0 )

Nit: Stray change?

> @@ -36,10 +36,17 @@ int __init xsm_multiboot_policy_init(
>  {
>      int i;
>      module_t *mod = (module_t *)__va(mbi->mods_addr);
> -    int rc = 0;
> +    int rc = -ENOENT;

I'm afraid I can't easily convince myself that this and the other
-ENOENT is really relevant to this change and/or not breaking
anything which currently does work (i.e. not entirely benign).
Please can you extend the description accordingly or split off
this adjustment?

> @@ -79,7 +87,16 @@ int __init xsm_dt_policy_init(void **policy_buffer, size_t 
> *policy_size)
>      paddr_t paddr, len;
>  
>      if ( !mod || !mod->size )
> +#ifdef CONFIG_XSM_FLASK_POLICY
> +    {
> +        *policy_buffer = (void *)xsm_flask_init_policy;

I don't think we want a cast here, especially not when it discards
"const". Instead the local variables' types want adjusting in
xsm_{multiboot,dt}_init() as well as the types of the respective
parameters of xsm_{multiboot,dt}_policy_init().

> +        *policy_size = xsm_flask_init_policy_size;
>          return 0;
> +    }
> +#else
> +        /* No policy was loaded */
> +        return -ENOENT;
> +#endif

I think this is easier to read if you put the braces there
unconditionally and have the #if / #else inside. And if you wanted
to I think you could get away without any #else then.

Jan