Re: [PATCH V4 4/6] libxl: arm: Split make_virtio_mmio_node()

From: Oleksandr <olekstysh@gmail.com>
To: Viresh Kumar <viresh.kumar@linaro.org>, xen-devel@lists.xen.org
Cc: "Vincent Guittot" <vincent.guittot@linaro.org>,
	stratos-dev@op-lists.linaro.org,
	"Alex Bennée" <alex.bennee@linaro.org>,
	"Stefano Stabellini" <stefano.stabellini@xilinx.com>,
	"Mathieu Poirier" <mathieu.poirier@linaro.com>,
	"Mike Holmes" <mike.holmes@linaro.org>, "Wei Liu" <wl@xen.org>,
	"Juergen Gross" <jgross@suse.com>,
	"Julien Grall" <julien@xen.org>
Subject: Re: [PATCH V4 4/6] libxl: arm: Split make_virtio_mmio_node()
Date: Thu, 18 Aug 2022 23:36:32 +0300	[thread overview]
Message-ID: <e353deb6-1de0-c575-df4b-02efa5972bc5@gmail.com> (raw)
In-Reply-To: <25fa5be0459f2f790f8420540840ea951a2f872e.1660023094.git.viresh.kumar@linaro.org>

On 09.08.22 08:34, Viresh Kumar wrote:

Hello Viresh

> make_virtio_mmio_node() creates the DT node for simple MMIO devices
> currently, i.e. the ones that don't require any additional properties.
>
> In order to allow using it for other complex device types, split the
> functionality into two, one where the fdt node isn't closed and the
> other one to create a simple DT node.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>   tools/libs/light/libxl_arm.c | 21 ++++++++++++++-------
>   1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 4a750852b671..891cb6ef2674 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -899,9 +899,8 @@ static int make_xen_iommu_node(libxl__gc *gc, void *fdt)
>       return 0;
>   }
>   
> -static int make_virtio_mmio_node(libxl__gc *gc, void *fdt,
> -                                 uint64_t base, uint32_t irq,
> -                                 uint32_t backend_domid)
> +static int make_virtio_mmio_node_common(libxl__gc *gc, void *fdt, uint64_t base,
> +                                        uint32_t irq, uint32_t backend_domid)

Nit: I would probably add a comment on top of this function that fdt 
node isn't completed (closed) after the function returns,

so the caller should take care of completing it.

>   {
>       int res;
>       gic_interrupt intr;
> @@ -934,10 +933,18 @@ static int make_virtio_mmio_node(libxl__gc *gc, void *fdt,
>           if (res) return res;
>       }
>   
> -    res = fdt_end_node(fdt);
> +    return res;
> +}
> +
> +static int make_virtio_mmio_node_simple(libxl__gc *gc, void *fdt, uint64_t base,
> +                                        uint32_t irq, uint32_t backend_domid)
> +{
> +    int res;
> +
> +    res = make_virtio_mmio_node_common(gc, fdt, base, irq, backend_domid);
>       if (res) return res;
>   
> -    return 0;
> +    return fdt_end_node(fdt);
>   }
>   
>   static const struct arch_info *get_arch_info(libxl__gc *gc,
> @@ -1260,8 +1267,8 @@ static int libxl__prepare_dtb(libxl__gc *gc, libxl_domain_config *d_config,
>                       iommu_created = true;
>                   }
>   
> -                FDT( make_virtio_mmio_node(gc, fdt, disk->base, disk->irq,
> -                                           disk->backend_domid) );
> +                FDT( make_virtio_mmio_node_simple(gc, fdt, disk->base,
> +                                            disk->irq, disk->backend_domid) );

Nit: Something wrong with the indentation

>               }
>           }

Preferably with above fixed:

Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

>   

-- 
Regards,

Oleksandr Tyshchenko