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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 4/22/26 1:42 PM, Orzel, Michal wrote:
> 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.
Agree, alloc_phandle() will catch that. I will drop that.

> >   
> > +/* 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.
    /*
     * Next free phandle for guest device nodes; do not access 
directly, use
     * alloc_phandle().
     */
    uint32_t next_phandle;

Thanks.

~ Oleksii