Re: [RFC v2 3/8] xen/arm: Export host device-tree to hypfs

[Julien Grall <julien@xxxxxxx>]

Hi Oleksii,

On 08/02/2022 18:00, Oleksii Moisieiev wrote:
> If enabled, host device-tree will be exported to hypfs and can be
> accessed through /devicetree path.
> Exported device-tree has the same format, as the device-tree
> exported to the sysfs by the Linux kernel.
> This is useful when XEN toolstack needs an access to the host device-tree.
>
> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx>
> ---
>   xen/arch/arm/Kconfig           |   8 +
>   xen/arch/arm/Makefile          |   1 +
>   xen/arch/arm/host_dtb_export.c | 307 +++++++++++++++++++++++++++++++++
There is nothing specific in this file. So can this be moved in common/?

>   3 files changed, 316 insertions(+)
>   create mode 100644 xen/arch/arm/host_dtb_export.c
>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index ecfa6822e4..895016b21e 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -33,6 +33,14 @@ config ACPI
>           Advanced Configuration and Power Interface (ACPI) support for Xen is
>           an alternative to device tree on ARM64.
>   
> +config HOST_DTB_EXPORT
> +       bool "Export host device tree to hypfs if enabled"
> +       depends on ARM && HYPFS && !ACPI
A Xen built with ACPI enabled will still be able to boot on a host using 
Device-Tree. So I don't think should depend on ACPI.
Also, I think this should depend on HAS_DEVICE_TREE rather than ARM.

> +       ---help---
> +
> +         Export host device-tree to hypfs so toolstack can have an access for 
> the
> +         host device tree from Dom0. If you unsure say N.
> +
>   config GICV3
>         bool "GICv3 driver"
>         depends on ARM_64 && !NEW_VGIC
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 07f634508e..0a41f68f8c 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -8,6 +8,7 @@ obj-y += platforms/
>   endif
>   obj-$(CONFIG_TEE) += tee/
>   obj-$(CONFIG_HAS_VPCI) += vpci.o
> +obj-$(CONFIG_HOST_DTB_EXPORT) += host_dtb_export.o
>   
>   obj-$(CONFIG_HAS_ALTERNATIVE) += alternative.o
>   obj-y += bootfdt.init.o
> diff --git a/xen/arch/arm/host_dtb_export.c b/xen/arch/arm/host_dtb_export.c
> new file mode 100644
> index 0000000000..794395683c
> --- /dev/null
> +++ b/xen/arch/arm/host_dtb_export.c
This is mostly hypfs related. So CCing Juergen for his input on the code.

> @@ -0,0 +1,307 @@
> +/*
> + * xen/arch/arm/host_dtb_export.c
> + *
> + * Export host device-tree to the hypfs so toolstack can access
> + * host device-tree from Dom0
> + *
> + * Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx>
> + * Copyright (C) 2021, EPAM Systems.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <xen/device_tree.h>
> +#include <xen/err.h>
> +#include <xen/guest_access.h>
> +#include <xen/hypfs.h>
> +#include <xen/init.h>
> +
> +#define HOST_DT_DIR "devicetree"
> +
> +static int host_dt_dir_read(const struct hypfs_entry *entry,
> +                            XEN_GUEST_HANDLE_PARAM(void) uaddr);
> +static unsigned int host_dt_dir_getsize(const struct hypfs_entry *entry);
> +
> +static const struct hypfs_entry *host_dt_dir_enter(
> +    const struct hypfs_entry *entry);
> +static void host_dt_dir_exit(const struct hypfs_entry *entry);
> +
> +static struct hypfs_entry *host_dt_dir_findentry(
> +    const struct hypfs_entry_dir *dir, const char *name, unsigned int 
> name_len);
This is new code. So can you please make sure to avoid forward 
declaration by re-ordering the code.

[...]

> +static char *get_root_from_path(const char *path, char *name)
> +{
> +    const char *nm = strchr(path, '/');
> +    if ( !nm )
> +        nm = path + strlen(path);
> +    else
> +    {
> +        if ( !*nm )
> +            nm--;
> +    }
> +
> +    return memcpy(name, path, nm - path);
What does guaranteee that name will be big enough for the new value?

> +}
> +
> +static int host_dt_dir_read(const struct hypfs_entry *entry,
> +                            XEN_GUEST_HANDLE_PARAM(void) uaddr)
> +{
> +    int ret = 0;
> +    struct dt_device_node *node;
> +    struct dt_device_node *child;
The hypfs should not modify the device-tree. So can this be const?

> +    const struct dt_property *prop;
> +    struct hypfs_dyndir_id *data;
> +
> +    data = hypfs_get_dyndata();
> +    if ( !data )
> +        return -EINVAL;
[...]

> +static struct hypfs_entry *host_dt_dir_findentry(
> +    const struct hypfs_entry_dir *dir, const char *name, unsigned int name_len)
> +{
> +    struct dt_device_node *node;
> +    char root_name[HYPFS_DYNDIR_ID_NAMELEN];
> +    struct dt_device_node *child;
> +    struct hypfs_dyndir_id *data;
> +    struct dt_property *prop;
> +
> +    data = hypfs_get_dyndata();
> +    if ( !data )
> +        return ERR_PTR(-EINVAL);
> +
> +    node = data->data;
> +    if ( !node )
> +        return ERR_PTR(-EINVAL);
> +
> +    memset(root_name, 0, sizeof(root_name));
> +    get_root_from_path(name, root_name);
> +
> +    for ( child = node->child; child != NULL; child = child->sibling )
> +    {
> +        if ( strcmp(get_name_from_path(child->full_name), root_name) == 0 )
> +            return hypfs_gen_dyndir_entry(&dt_dir.e,
> +                                  get_name_from_path(child->full_name), child);
> +    }
> +
> +    dt_for_each_property_node( node, prop )
> +    {
> +
> +        if ( dt_property_name_is_equal(prop, root_name) )
> +            return hypfs_gen_dyndir_entry(&dt_prop.e, prop->name, prop);
> +    }
> +
> +    return ERR_PTR(-ENOENT);
[...]

> +static HYPFS_DIR_INIT_FUNC(host_dt_dir, HOST_DT_DIR, &host_dt_dir_funcs);
> +
> +static int __init host_dtb_export_init(void)
> +{
> +    ASSERT(dt_host && (dt_host->sibling == NULL));
dt_host can be NULL when booting on ACPI platform. So I think this wants 
to be turned to a normal check and return directly.
Also could you explain why you need to check dt_host->sibling?

Cheers,

--
Julien Grall