[Xen-devel,RFC,13/14] xen/xsm: Add support for device tree

This patch adds a new module "xen,xsm-blob" to allow the user to load the XSM
policy when Xen is booting.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 docs/misc/arm/device-tree/booting.txt |    1 +
 xen/common/device_tree.c              |    2 ++
 xen/include/xen/device_tree.h         |    3 ++-
 xen/include/xsm/xsm.h                 |   12 +++++++++++
 xen/xsm/xsm_core.c                    |   37 +++++++++++++++++++++++++++++++++
 xen/xsm/xsm_policy.c                  |   37 +++++++++++++++++++++++++++++++++
 6 files changed, 91 insertions(+), 1 deletion(-)

On 03/12/2014 12:16 PM, Julien Grall wrote:
> This patch adds a new module "xen,xsm-blob" to allow the user to load the XSM
> policy when Xen is booting.
>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>

Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

> ---
>   docs/misc/arm/device-tree/booting.txt |    1 +
>   xen/common/device_tree.c              |    2 ++
>   xen/include/xen/device_tree.h         |    3 ++-
>   xen/include/xsm/xsm.h                 |   12 +++++++++++
>   xen/xsm/xsm_core.c                    |   37 +++++++++++++++++++++++++++++++++
>   xen/xsm/xsm_policy.c                  |   37 +++++++++++++++++++++++++++++++++
>   6 files changed, 91 insertions(+), 1 deletion(-)
>
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> index 07fde27..85988fb 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -16,6 +16,7 @@ Each node contains the following properties:
>
>   	- "linux-zimage" -- the dom0 kernel
>   	- "linux-initrd" -- the dom0 ramdisk
> +	- "xsm-blob"	 -- XSM policy blob
>
>   - reg
>
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 55716a8..91146fb 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -354,6 +354,8 @@ static void __init process_multiboot_node(const void *fdt, int node,
>           nr = MOD_KERNEL;
>       else if ( fdt_node_check_compatible(fdt, node, "xen,linux-initrd") == 0)
>           nr = MOD_INITRD;
> +    else if ( fdt_node_check_compatible(fdt, node, "xen,xsm-blob") == 0 )
> +        nr = MOD_XSM;
>       else
>           early_panic("%s not a known xen multiboot type\n", name);
>
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 9a8c3de..76faf11 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -24,7 +24,8 @@
>   #define MOD_FDT    1
>   #define MOD_KERNEL 2
>   #define MOD_INITRD 3
> -#define NR_MODULES 4
> +#define MOD_XSM    4
> +#define NR_MODULES 5
>
>   #define MOD_DISCARD_FIRST MOD_FDT
>
> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> index 4863e41..2cd3a3b 100644
> --- a/xen/include/xsm/xsm.h
> +++ b/xen/include/xsm/xsm.h
> @@ -652,6 +652,11 @@ extern int xsm_multiboot_policy_init(unsigned long *module_map,
>                                        void *(*bootstrap_map)(const module_t *));
>   #endif
>
> +#ifdef HAS_DEVICE_TREE
> +extern int xsm_dt_init(void);
> +extern int xsm_dt_policy_init(void);
> +#endif
> +
>   extern int register_xsm(struct xsm_operations *ops);
>   extern int unregister_xsm(struct xsm_operations *ops);
>
> @@ -671,6 +676,13 @@ static inline int xsm_multiboot_init (unsigned long *module_map,
>   }
>   #endif
>
> +#ifdef HAS_DEVICE_TREE
> +static inline int xsm_dt_init(void)
> +{
> +    return 0;
> +}
> +#endif
> +
>   #endif /* XSM_ENABLE */
>
>   #endif /* __XSM_H */
> diff --git a/xen/xsm/xsm_core.c b/xen/xsm/xsm_core.c
> index 11a9ca7..755a5dd 100644
> --- a/xen/xsm/xsm_core.c
> +++ b/xen/xsm/xsm_core.c
> @@ -79,6 +79,43 @@ int __init xsm_multiboot_init(unsigned long *module_map,
>   }
>   #endif
>
> +#ifdef HAS_DEVICE_TREE
> +int __init xsm_dt_init(void)
> +{
> +    int ret = 0;
> +
> +    printk("XSM Framework v" XSM_FRAMEWORK_VERSION " initialized\n");
> +
> +    if ( XSM_MAGIC )
> +    {
> +        ret = xsm_dt_policy_init();
> +        if ( ret )
> +        {
> +            printk("%s: Error initializing policy (rc = %d).\n",
> +                   __FUNCTION__, ret);
> +            return -EINVAL;
> +        }
> +    }
> +
> +    if ( verify(&dummy_xsm_ops) )
> +    {
> +        printk("%s could not verify dummy_xsm_ops structure.\n",
> +               __FUNCTION__);
> +        ret = -EIO;
> +        goto err;
> +    }
> +
> +    xsm_ops = &dummy_xsm_ops;
> +    do_xsm_initcalls();
> +
> +err:
> +    if ( policy_buffer )
> +        xfree(policy_buffer);
> +
> +    return ret;
> +}
> +#endif
> +
>   int register_xsm(struct xsm_operations *ops)
>   {
>       if ( verify(ops) )
> diff --git a/xen/xsm/xsm_policy.c b/xen/xsm/xsm_policy.c
> index 3d5f66a..a0dee09 100644
> --- a/xen/xsm/xsm_policy.c
> +++ b/xen/xsm/xsm_policy.c
> @@ -23,6 +23,10 @@
>   #include <xen/multiboot.h>
>   #endif
>   #include <xen/bitops.h>
> +#ifdef HAS_DEVICE_TREE
> +# include <asm/setup.h>
> +# include <xen/device_tree.h>
> +#endif
>
>   char *__initdata policy_buffer = NULL;
>   u32 __initdata policy_size = 0;
> @@ -69,3 +73,36 @@ int __init xsm_multiboot_policy_init(unsigned long *module_map,
>       return rc;
>   }
>   #endif
> +
> +#ifdef HAS_DEVICE_TREE
> +int __init xsm_dt_policy_init(void)
> +{
> +    paddr_t paddr = early_info.modules.module[MOD_XSM].start;
> +    paddr_t len = early_info.modules.module[MOD_XSM].size;
> +    xsm_magic_t magic;
> +
> +    if ( !len )
> +        return 0;
> +
> +    copy_from_paddr(&magic, paddr, sizeof(magic));
> +
> +    if ( magic != XSM_MAGIC )
> +    {
> +        printk(XENLOG_ERR "xsm: Invalid magic for XSM blob got 0x%x "
> +               "expected 0x%x\n", magic, XSM_MAGIC);
> +        return -EINVAL;
> +    }
> +
> +    printk("xsm: Policy len = 0x%"PRIpaddr" start at 0x%"PRIpaddr"\n",
> +           len, paddr);
> +
> +    policy_buffer = xmalloc_bytes(len);
> +    if ( !policy_buffer )
> +        return -ENOMEM;
> +
> +    copy_from_paddr(policy_buffer, paddr, len);
> +    policy_size = len;
> +
> +    return 0;
> +}
> +#endif
>

On Wed, 2014-03-12 at 16:16 +0000, Julien Grall wrote:
> This patch adds a new module "xen,xsm-blob" to allow the user to load the XSM
> policy when Xen is booting.

While xsm_dt_policy_init does seem DT specific the xem_dt_init is very
similar to the xsm_multiboot_init. Can they not be a common function
with #ifdef HAVE_MULTIBOOT / #ifdef HAVE_DEVICETREE around the relevant
specific bits (essentially just the call to the relevant policy_init)?
Or at least refactor the tail of xsm_init into xsm_core_init which both
cases can call into.

Hi Ian,

On 03/14/2014 05:34 PM, Ian Campbell wrote:
> On Wed, 2014-03-12 at 16:16 +0000, Julien Grall wrote:
>> This patch adds a new module "xen,xsm-blob" to allow the user to load the XSM
>> policy when Xen is booting.
> 
> While xsm_dt_policy_init does seem DT specific the xem_dt_init is very
> similar to the xsm_multiboot_init. Can they not be a common function
> with #ifdef HAVE_MULTIBOOT / #ifdef HAVE_DEVICETREE around the relevant
> specific bits (essentially just the call to the relevant policy_init)?
> Or at least refactor the tail of xsm_init into xsm_core_init which both
> cases can call into.

The parameters of the function is not the same and it seems stupid to
also ifdef the list of arguments :).

I can at least create a new function with

    if ( verify(&dummy_xsm_ops) )
    {
        printk("%s could not verify dummy_xsm_ops structure.\n",
               __FUNCTION__);
        ret = -EIO;
        goto err;
    }

    xsm_ops = &dummy_xsm_ops;
    do_xsm_initcalls();

Regards,

On Fri, 2014-03-14 at 18:24 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 03/14/2014 05:34 PM, Ian Campbell wrote:
> > On Wed, 2014-03-12 at 16:16 +0000, Julien Grall wrote:
> >> This patch adds a new module "xen,xsm-blob" to allow the user to load the XSM
> >> policy when Xen is booting.
> > 
> > While xsm_dt_policy_init does seem DT specific the xem_dt_init is very
> > similar to the xsm_multiboot_init. Can they not be a common function
> > with #ifdef HAVE_MULTIBOOT / #ifdef HAVE_DEVICETREE around the relevant
> > specific bits (essentially just the call to the relevant policy_init)?
> > Or at least refactor the tail of xsm_init into xsm_core_init which both
> > cases can call into.
> 
> The parameters of the function is not the same and it seems stupid to
> also ifdef the list of arguments :).
> 
> I can at least create a new function with
> 
>     if ( verify(&dummy_xsm_ops) )
>     {
>         printk("%s could not verify dummy_xsm_ops structure.\n",
>                __FUNCTION__);
>         ret = -EIO;
>         goto err;
>     }
> 
>     xsm_ops = &dummy_xsm_ops;
>     do_xsm_initcalls();

My main concern was new code getting added to the generic tail of the
init function, so putting that into a function sounds like a good idea
to me. 

Ian.