Re: [PATCH v2 06/21] xen/arm: do not pass dt_host to make_memory_node and make_hypervisor_node

[Stefano Stabellini <sstabellini@kernel.org>]

On Mon, 9 Jul 2018, Julien Grall wrote:
> Hi,
> 
> On 07/07/18 00:12, Stefano Stabellini wrote:
> > In order to make make_memory_node and make_hypervisor_node more
> > reusable, do not pass them dt_host. As they only use it to calculate
> > addrcells and sizecells, pass addrcells and sizecells directly.
> > 
> > In make_hypervisor_node, assume that evtchn_irq has already been
> > allocated. Move the evtchn_allocate call to handle_node.
> 
> This needs a bit more explanation why. But I think this belongs to a separate
> patch.

Sure


> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > 
> > ---
> > Changes in v2:
> > - add blank line
> > - move evtchn_allocate to handle_node
> > ---
> >   xen/arch/arm/domain_build.c   | 36 ++++++++++++++++++++----------------
> >   xen/common/device_tree.c      |  6 +++---
> >   xen/include/xen/device_tree.h |  2 +-
> >   3 files changed, 24 insertions(+), 20 deletions(-)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index ae3ebc5..c349ce4 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -558,11 +558,11 @@ static int fdt_property_interrupts(void *fdt,
> > gic_interrupt_t *intr,
> >     static int make_memory_node(const struct domain *d,
> >                               void *fdt,
> > -                            const struct dt_device_node *parent,
> > +                            int addrcells, int sizecells,
> >                               const struct kernel_info *kinfo)
> >   {
> >       int res, i;
> > -    int reg_size = dt_child_n_addr_cells(parent) +
> > dt_child_n_size_cells(parent);
> > +    int reg_size = addrcells + sizecells;
> >       int nr_cells = reg_size*kinfo->mem.nr_banks;
> >       __be32 reg[nr_cells];
> >       __be32 *cells;
> > @@ -588,7 +588,7 @@ static int make_memory_node(const struct domain *d,
> >           dt_dprintk("  Bank %d: %#"PRIx64"->%#"PRIx64"\n",
> >                      i, start, start + size);
> >   -        dt_child_set_range(&cells, parent, start, size);
> > +        dt_child_set_range(&cells, addrcells, sizecells, start, size);
> >       }
> >         res = fdt_property(fdt, "reg", reg, sizeof(reg));
> > @@ -604,7 +604,7 @@ static void evtchn_allocate(struct domain *d);
> >     static int make_hypervisor_node(struct domain *d,
> >                                   const struct kernel_info *kinfo,
> > -                                const struct dt_device_node *parent)
> > +                                int addrcells, int sizecells)
> >   {
> >       const char compat[] =
> >           "xen,xen-"__stringify(XEN_VERSION)"."__stringify(XEN_SUBVERSION)"\0"
> > @@ -613,9 +613,6 @@ static int make_hypervisor_node(struct domain *d,
> >       gic_interrupt_t intr;
> >       __be32 *cells;
> >       int res;
> > -    /* Convenience alias */
> > -    int addrcells = dt_child_n_addr_cells(parent);
> > -    int sizecells = dt_child_n_size_cells(parent);
> >       void *fdt = kinfo->fdt;
> >         dt_dprintk("Create hypervisor node\n");
> > @@ -640,19 +637,14 @@ static int make_hypervisor_node(struct domain *d,
> >         /* reg 0 is grant table space */
> >       cells = &reg[0];
> > -    dt_child_set_range(&cells, parent, kinfo->gnttab_start,
> > kinfo->gnttab_size);
> > +    dt_child_set_range(&cells, addrcells, sizecells,
> > +                       kinfo->gnttab_start, kinfo->gnttab_size);
> >       res = fdt_property(fdt, "reg", reg,
> >                          dt_cells_to_size(addrcells + sizecells));
> >       if ( res )
> >           return res;
> >         /*
> > -     * It is safe to allocate the event channel here because all the
> > -     * PPIs used by the hardware domain have been registered.
> > -     */
> > -    evtchn_allocate(d);
> 
> I would replace this with a BUG_ON(evtchn != 0).

I agree with the principle, but I think you meant
BUG_ON(d->arch.evtchn_irq <= 0) ?


> > -
> > -    /*
> >        * Interrupt event channel upcall:
> >        *  - Active-low level-sensitive
> >        *  - All CPUs
> > @@ -1317,11 +1309,23 @@ static int handle_node(struct domain *d, struct
> > kernel_info *kinfo,
> >         if ( node == dt_host )
> >       {
> > +        int addrcells = dt_child_n_addr_cells(node);
> > +        int sizecells = dt_child_n_size_cells(node);
> > +
> > +
> > +        /*
> > +         *      * It is safe to allocate the event channel here
> > +         *      because all the
> > +         *           * PPIs used by the hardware domain have been
> > +         *           registered.
> > +         *                */
> 
> Please fix the comment.

No idea what happened there :-)
I'll fix


> > +        evtchn_allocate(d);
> > +
> >           /*
> >            * The hypervisor node should always be created after all nodes
> >            * from the host DT have been parsed.
> >            */
> > -        res = make_hypervisor_node(d, kinfo, node);
> > +        res = make_hypervisor_node(d, kinfo, addrcells, sizecells);
> >           if ( res )
> >               return res;
> >   @@ -1333,7 +1337,7 @@ static int handle_node(struct domain *d, struct
> > kernel_info *kinfo,
> >           if ( res )
> >               return res;
> >   -        res = make_memory_node(d, kinfo->fdt, node, kinfo);
> > +        res = make_memory_node(d, kinfo->fdt, addrcells, sizecells, kinfo);
> >           if ( res )
> >               return res;
> >   diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> > index 7b009ea..8fc401d 100644
> > --- a/xen/common/device_tree.c
> > +++ b/xen/common/device_tree.c
> > @@ -112,11 +112,11 @@ void dt_set_range(__be32 **cellp, const struct
> > dt_device_node *np,
> >       dt_set_cell(cellp, dt_n_size_cells(np), size);
> >   }
> >   -void dt_child_set_range(__be32 **cellp, const struct dt_device_node
> > *parent,
> > +void dt_child_set_range(__be32 **cellp, int addrcells, int sizecells,
> >                           u64 address, u64 size)
> >   {
> > -    dt_set_cell(cellp, dt_child_n_addr_cells(parent), address);
> > -    dt_set_cell(cellp, dt_child_n_size_cells(parent), size);
> > +    dt_set_cell(cellp, addrcells, address);
> > +    dt_set_cell(cellp, sizecells, size);
> >   }
> >     static void __init *unflatten_dt_alloc(unsigned long *mem, unsigned long
> > size,
> > diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> > index 638b926..91fa0b6 100644
> > --- a/xen/include/xen/device_tree.h
> > +++ b/xen/include/xen/device_tree.h
> > @@ -674,7 +674,7 @@ void dt_set_range(__be32 **cellp, const struct
> > dt_device_node *np,
> >    * Write a range into a series of cells and update cellp to point to the
> >    * cell just after.
> >    */
> > -void dt_child_set_range(__be32 **cellp, const struct dt_device_node
> > *parent,
> > +void dt_child_set_range(__be32 **cellp, int addrcells, int sizecells,
> >                           u64 address, u64 size);
> >     /**
> > 
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel