Re: [PATCH v2 1/3] xen/dom0less: introduce next_phandle in struct kernel_info

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 22/04/2026 11:44, Oleksii Kurochko wrote:
> There are cases where it is necessary to know the next available phandle
> number in order to generate phandles for guest device nodes.
> 
> When a partial FDT (pfdt) is provided, special care is needed during
> initialization of next_phandle, as the pfdt may already contain a dummy
> interrupt controller node with a phandle assigned to it. next_phandle
> must therefore be initialized to one past the highest phandle already
> present in the pfdt, to avoid collisions.
> 
> Since next_phandle may be needed for the very first guest node generated,
> domain_handle_dtb_boot_module() is moved earlier in prepare_dtb_domU().
> The new call site also aligns better with the existing comment stating
> that domain_handle_dtb_boot_module() must be called before the rest of
> the device tree is generated.
> 
> Introduce alloc_phandle() to ensure that phandles allocated for guest
> nodes do not overlap the Xen-reserved phandle range.  This helper will
> be used by subsequent patches (by RISC-V at the moment).
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
> Here is an example of generated guest DTB:
>     cpus {
>     ...
>       cpu@0 {
>     ...
>         interrupt-controller {
>           compatible = "riscv,cpu-intc";
>           #interrupt-cells = <0x1>;
>           interrupt-controller;
>           phandle = <0xfdea>;
>         };
>       };
>     };
> 
>     /soc/imsics@28000000 {
> 
>       interrupts-extended = <0xfdea 0x9 >;
> 
>       phandle = <0xfdeb>;
>     };
> 
>     /soc/aplic@d000000 {
>     ...
>       msi-parent = <0xfdeb>;
>       phandle = <0x1>;
>     };
> 
> Note that phandle is generated in this example not by get_next_free_phandle().
> 
> For non RISC-V people, APLIC is an interrupt controller (something like GIC in
> Arm), IMSIC it is interrupt controller which provides MSI and connects to
> each CPU.
> 
> [1] 
> https://www.kernel.org/doc/Documentation/devicetree/bindings/interrupt-controller/riscv%2Ccpu-intc.txt
> ---
> Changes in v2:
>  - s/free_phandle/next_phandle.
>  - s/get_next_free_phandle/alloc_phandle.
> ---
>  xen/common/device-tree/dom0less-build.c | 44 ++++++++++++++++++-------
>  xen/include/xen/fdt-domain-build.h      |  6 ++++
>  xen/include/xen/fdt-kernel.h            |  3 ++
>  3 files changed, 41 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/common/device-tree/dom0less-build.c 
> b/xen/common/device-tree/dom0less-build.c
> index 840d14419da2..ca3ac84a3ef3 100644
> --- a/xen/common/device-tree/dom0less-build.c
> +++ b/xen/common/device-tree/dom0less-build.c
> @@ -389,6 +389,24 @@ static int __init domain_handle_dtb_boot_module(struct 
> domain *d,
>      if ( res < 0 )
>          goto out;
>  
> +    /*
> +     * Find the highest phandle in the partial FDT so next_phandle starts
> +     * above it, avoiding collisions with pfdt's own phandle assignments.
> +     */
> +    res = fdt_generate_phandle(pfdt, &kinfo->next_phandle);
> +    if ( res )
> +    {
> +        res = (res == -FDT_ERR_NOPHANDLES) ? -EOVERFLOW : -EINVAL;
> +        goto out;
> +    }
> +
> +    if ( kinfo->next_phandle >= GUEST_PHANDLE_GIC )
> +    {
> +        dprintk(XENLOG_ERR, "Phandle allocation overlaps Xen reserved 
> range\n");
> +        res = -EOVERFLOW;
> +        goto out;
> +    }
> +
>      for ( node_next = fdt_first_subnode(pfdt, 0);
>            node_next > 0;
>            node_next = fdt_next_subnode(pfdt, node_next) )
> @@ -459,6 +477,8 @@ static int __init prepare_dtb_domU(struct domain *d, 
> struct kernel_info *kinfo)
>      BUILD_BUG_ON(DOMU_DTB_SIZE > SZ_2M);
>  
>      kinfo->phandle_intc = GUEST_PHANDLE_GIC;
> +    kinfo->next_phandle = 1;
> +    BUILD_BUG_ON(GUEST_PHANDLE_GIC == 1);
I'm not sure that we need this. It does not seem to be useful. If you want to
keep it though, I think you want to compare to next_phandle, not opencoding it's
initial value.

>  
>  #ifdef CONFIG_GRANT_TABLE
>      kinfo->gnttab_start = GUEST_GNTTAB_BASE;
> @@ -499,6 +519,18 @@ static int __init prepare_dtb_domU(struct domain *d, 
> struct kernel_info *kinfo)
>      if ( ret )
>          goto err;
>  
> +    /*
> +     * domain_handle_dtb_boot_module() must be called before the rest of the
> +     * device tree is generated because it sets phandle_intc and 
> next_phandle,
> +     * which subsequent node generation depends on.
> +     */
> +    if ( kinfo->dtb )
> +    {
> +        ret = domain_handle_dtb_boot_module(d, kinfo);
> +        if ( ret )
> +            goto err;
> +    }
> +
>      ret = make_chosen_node(kinfo);
>      if ( ret )
>          goto err;
> @@ -516,18 +548,6 @@ static int __init prepare_dtb_domU(struct domain *d, 
> struct kernel_info *kinfo)
>      if ( ret )
>          goto err;
>  
> -    /*
> -     * domain_handle_dtb_boot_module has to be called before the rest of
> -     * the device tree is generated because it depends on the value of
> -     * the field phandle_intc.
> -     */
> -    if ( kinfo->dtb )
> -    {
> -        ret = domain_handle_dtb_boot_module(d, kinfo);
> -        if ( ret )
> -            goto err;
> -    }
> -
>      ret = make_intc_domU_node(kinfo);
>      if ( ret )
>          goto err;
> diff --git a/xen/include/xen/fdt-domain-build.h 
> b/xen/include/xen/fdt-domain-build.h
> index 1d9e77df0eb3..a604f3983fe6 100644
> --- a/xen/include/xen/fdt-domain-build.h
> +++ b/xen/include/xen/fdt-domain-build.h
> @@ -63,6 +63,12 @@ int find_unallocated_memory(const struct kernel_info 
> *kinfo,
>                                        unsigned long e_gfn,
>                                        void *data));
>  
> +/* Return 0 (invalid phandle) if the Xen-reserved range has been reached */
> +static inline uint32_t alloc_phandle(struct kernel_info *kinfo)
> +{
> +    return kinfo->next_phandle >= GUEST_PHANDLE_GIC ? 0 : 
> kinfo->next_phandle++;
> +}
> +
>  #endif /* __XEN_FDT_DOMAIN_BUILD_H__ */
>  
>  /*
> diff --git a/xen/include/xen/fdt-kernel.h b/xen/include/xen/fdt-kernel.h
> index aa977a50f4fc..438adfe3855b 100644
> --- a/xen/include/xen/fdt-kernel.h
> +++ b/xen/include/xen/fdt-kernel.h
> @@ -44,6 +44,9 @@ struct kernel_info {
>      /* Interrupt controller phandle */
>      uint32_t phandle_intc;
>  
> +    /* Next free phandle available for assigning to guest device nodes */
I would mention not to use this value directly but rather obtain from
alloc_phandle. This value should only really be used by alloc_phandle.

~Michal

> +    uint32_t next_phandle;
> +
>      /* loader to use for this kernel */
>      void (*load)(struct kernel_info *info);
>