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

[Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx>]

Hi Julien,

On Wed, Feb 09, 2022 at 12:17:17PM +0000, Julien Grall wrote:
> 
> 
> On 09/02/2022 10:20, Oleksii Moisieiev wrote:
> > Hi Julien,
> 
> Hi,
> 
> > 
> > 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.
> 
> You can avoid most of those forward declarations if you define the static
> variable now but fill them up after (see [1]). I don't think we can avoid
> the static variable forward declaration without reworking the API.
> 
> BTW, I could not fully apply the series on the staging tree:
> 
> Applying: xen/hypfs: support fo nested dynamic hypfs nodes
> Applying: libs: libxenhypfs - handle blob properties
> Applying: xen/arm: Export host device-tree to hypfs
> Applying: xen/arm: add generic SCI mediator framework
> error: patch failed: MAINTAINERS:512
> error: MAINTAINERS: patch does not apply
> error: patch failed: xen/arch/arm/domain_build.c:1894
> error: xen/arch/arm/domain_build.c: patch does not apply
> error: xen/include/asm-arm/domain.h: does not exist in index
> Patch failed at 0004 xen/arm: add generic SCI mediator framework
> hint: Use 'git am --show-current-patch=diff' to see the failed patch
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
> 
> From the errors, it sounds like your baseline is from a couple of months
> ago. Please make sure to send your series based on the latest staging (at
> the time you send it).
> 

I'm sorry for that. I will fix all comments, mentioned above, make a
rebase and post v3 shortly.

> > > > +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.
> 
> Can you explain why that data->data is not const?

I reused struct hypfs_dyndir_id, added by @Juergen Gross in
4f1e5ed49c2292d3dd18160b7e728b1aa9453206 hope he will help to answer
this question.

> > > > +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.
> 
> dt_host will always points to the root of the host device-tree. I don't
> think it is the job of hypfs to enforce it unless you expect the code to be
> buggy if this happens. But then I would argue the code should be hardened.
> 

I will refactor this check.

Best regards,
Oleksii.