[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Julien Grall <julien@xxxxxxx>
  • From: Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx>
  • Date: Wed, 9 Feb 2022 10:20:38 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=zgXPjBiGwaJIK/z8yCxnzELuL+zHuRsre0yutMwKAlw=; b=Nh/Fv2jUtHu8kCUArV6jWqm6sAdc7gLDfdubKdsquOOeC7L929FjxefjYjyaGJeo/EqnjBClA1IOKhXSIixZrc6XU6ijtKL8fiUwbmj292EoN7bNEzvX9fuezpSwTThPf2PpmuZ8Pza86nxXKC/gwxIjALMVyiAW+LDMh1/hc+r0CnqJ+BxOHI2rvXDH0QC1GOs8Vb9NIrpn9KAtvEGoS1Y7a3vZRs3kcwnG2uBKsUT9Wd+Jpo62XhXR2L4DFCxsMjbWzGonrH57fEca9IuaUM/l5jcUuM0Uje9S64JxdlDDJs6Y4yVTyCrNEfsgtJZxS6MYy/W2TxSoLulMgUZ9mA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=WyPVVPfboBBS6PjiiOO4JxbmvtIuUPLonLHglvMtJqCW1U0Hp4kLMpXm1cvok7p258O1GA1gEQc4bLhs2luYVhY4A5UqyyJzMiEurYlRPIgBDgYTQowfLK/csUfR9ZSsKDkZSDrBCZ2xUpeWlycbmFekAB0RVSMTFCHuXHtEG6CK+h3ItCE47Fjpl986yq0Xrv9HflxZJzuBR0R5PI5QXDQ8JhJ/iNkpGqjvwoMZzWcZQjeJIg46KWypP0NGid/zuzv6/0DHJUlHheGN4vWZqm3AznJN7mL6qLtnoezb53l6TYWV9N1noWYTVF9w+GcyQ8h1QF0kO+Q0IOKPCdoYiA==
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>, Juergen Gross <jgross@xxxxxxxx>
  • Delivery-date: Wed, 09 Feb 2022 10:20:54 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHYHRW1S717SraA0Ui+K6wc2/TNAKyJ+LkAgAEKd4A=
  • Thread-topic: [RFC v2 3/8] xen/arm: Export host device-tree to hypfs

Hi Julien,

On Tue, Feb 08, 2022 at 06:26:54PM +0000, Julien Grall wrote:
> 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/?

You're right. I will move it to 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.

I agree. Thank you.

> 
> > +   ---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.

Thank you.

> 
> > @@ -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.
> 

I can't avoid forward declaration here because all those functions
should be passed as callbacks for node template dt_dir. And dt_dir is
used in read and findentry functions.

> 
> [...]
> 
> > +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?

I will refactor that.

> 
> > +}
> > +
> > +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?

That's a good point.
Unfortunatelly child can't be const because it is going to be passed to
data->data pointer, but node can be const I think. In any case I will go
through the file and see where const for the device_node can be set.

> 
> > +    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.
> 

I will replace if with
if ( !acpi_disabled )
    return -ENODEV;

> Also could you explain why you need to check dt_host->sibling?
> 

This is my way to check if dt_host points to the top of the device-tree.
In any case I will replace it with !acpi_disabled as I mentioned
earlier.

Best regards,
Oleksii


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.