Re: [PATCH v7 2/9] xen/common: Move Arm's bootfdt.c to common

[Julien Grall <julien@xxxxxxx>]

On 05/08/2024 11:04, oleksii.kurochko@xxxxxxxxx wrote:
> Hi Julien,
Hi Oleksii,

> On Mon, 2024-08-05 at 10:31 +0100, Julien Grall wrote:
> > Hi Oleksii,
> >
> > On 24/07/2024 16:31, Oleksii Kurochko wrote:
> > > From: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
> > >
> > > Move Arm's bootfdt.c to xen/common so that it can be used by other
> > > device tree architectures like PPC and RISCV.
> > >
> > > Suggested-by: Julien Grall <julien@xxxxxxx>
> > > Signed-off-by: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
> > > Acked-by: Julien Grall <julien@xxxxxxx>
> > On Matrix you asked me to review this patch again. This wasn't
> > obvious
> > given you kept my ack. If you think a review is needed, then please
> > either drop the ack or explain why you keep it and ask if it is fine.
> >
> > Also, I tend to list in the changes where this was acked. In this
> > case,
> > you said I acked v4.
> >
> > Anyway, before confirming my ack, I would like to ask some
> > clarification.
> >
> > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > > ---
> > > Changes in V7:
> > >    - Nothing changed. Only rebase.
> > > ---
> > > Changes in V6:
> > >    - update the version of the patch to v6.
> > > ---
> > > Changes in V5:
> > >    - add xen/include/xen/bootfdt.h to MAINTAINERS file.
> > I don't see any change in MAINTAINERS within this patch. Did you
> > happen
> > to copy/paste all the changes made in the series?
> This change should be mentioned in this patch. It is part of the
> previous patch (
> https://lore.kernel.org/xen-devel/102f8b60c55cdf2db5890b9fb1c2fb66e61c4a67.1721834549.git.oleksii.kurochko@xxxxxxxxx/
> )
>
> > In fact the only change related to this patch doesn't seem to be
> > listed.
> >
> > [...]
> >
> > > +#ifndef CONFIG_STATIC_SHM
> > > +static inline int process_shm_node(const void *fdt, int node,
> > > +                                   uint32_t address_cells,
> > > uint32_t size_cells)
> > > +{
> > > +    printk("CONFIG_STATIC_SHM must be enabled for parsing static
> > > shared"
> > > +            " memory nodes\n");
> > > +    return -EINVAL;
> > > +}
> > > +#endif
> > I see you duplicated the stub from arch/arm/include/static-shmem.h.
> > But
> > the one in static-shmem.h will now be unreachable. I think it needs
> > to
> > be removed.
> Overlooked that. Originally I added that to make Xen RISC-V and PPC
> build happy as early_scan_node() code uses process_shm_node():
>     static int __init early_scan_node(const void *fdt,
>                                       int node, const char *name, int
>     depth,
>                                       u32 address_cells, u32 size_cells,
>                                       void *data)
>     {
>     ...
>         else if ( depth == 2 && device_tree_node_compatible(fdt, node,
>     "xen,domain") )
>             rc = process_domain_node(fdt, node, name, address_cells,
>     size_cells);
>         else if ( depth <= 3 && device_tree_node_compatible(fdt, node,
>     "xen,domain-shared-memory-v1") )
>             rc = process_shm_node(fdt, node, address_cells, size_cells);
>     
>         if ( rc < 0 )
>             printk("fdt: node `%s': parsing failed\n", name);
>         return rc;
>     }
>
> Instead of introducing stub for process_shm_node() when
> CONFIG_STATIC_SHM I think it would be better to add "#ifdef
> CONFIG_STATIC_SHM" to early_scan_node():
>
>     static int __init early_scan_node(const void *fdt,
>                                       int node, const char *name, int
>     depth,
>                                       u32 address_cells, u32 size_cells,
>                                       void *data)
>     {
>     ...
>         else if ( depth == 2 && device_tree_node_compatible(fdt, node,
>     "xen,domain") )
>             rc = process_domain_node(fdt, node, name, address_cells,
>     size_cells);
>     #ifdef CONFIG_STATIC_SHM
>         else if ( depth <= 3 && device_tree_node_compatible(fdt, node,
>     "xen,domain-shared-memory-v1") )
>             rc = process_shm_node(fdt, node, address_cells, size_cells);
>     #endif
With this proposal, you would not throw an error if the user specify 
"xen,domain-shared-memory-v1" but Xen is not able to support it. This 
will be a change of behavior for Arm.
So my preferred approach is to stick with the existing patch and an 
explanation in the commit message.
Cheers,

--
Julien Grall