Re: [XEN][PATCH v10 03/20] xen/arm/device: Remove __init from function type

[Vikram Garhwal <vikram.garhwal@xxxxxxx>]

Hi Michal,
On Tue, Aug 29, 2023 at 09:17:37AM +0200, Michal Orzel wrote:
> 
> 
> On 25/08/2023 10:02, Vikram Garhwal wrote:
> > Remove __init from following function to access during runtime:
> >     1. map_irq_to_domain()
> >     2. handle_device_interrupts()
> >     3. map_range_to_domain()
> >     4. unflatten_dt_node()
> >     5. handle_device()
> >     6. map_device_children()
> >     7. map_dt_irq_to_domain()
> > Move map_irq_to_domain() prototype from domain_build.h to setup.h.
> > 
> > Above changes will create an error on build as non-init function are still
> > in domain_build.c file. So, to avoid build fails, following changes are 
> > done:
> > 1. Move map_irq_to_domain(), handle_device_interrupts(), 
> > map_range_to_domain(),
> >     handle_device(), map_device_children() and map_dt_irq_to_domain()
> >     to device.c. After removing __init type,  these functions are not 
> > specific
> >     to domain building, so moving them out of domain_build.c to device.c.
> > 2. Remove static type from handle_device_interrupts().
> > 
> > Overall, these changes are done to support the dynamic programming of a 
> > nodes
> > where an overlay node will be added to fdt and unflattened node will be 
> > added to
> > dt_host. Furthermore, IRQ and mmio mapping will be done for the added node.
> > 
> > Signed-off-by: Vikram Garhwal <vikram.garhwal@xxxxxxx>
> > 
> > ---
> > Changes from v9:
> >     Move handle_device(), map_device_children() and map_dt_irq_to_domain() 
> > out
> >         of domain_build.c
> > ---
> > ---
> >  xen/arch/arm/device.c                   | 293 ++++++++++++++++++++++++
> >  xen/arch/arm/domain_build.c             | 293 ------------------------
> >  xen/arch/arm/include/asm/domain_build.h |   2 -
> >  xen/arch/arm/include/asm/setup.h        |   9 +
> >  xen/common/device_tree.c                |  12 +-
> >  5 files changed, 308 insertions(+), 301 deletions(-)
> > 
> > diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
> > index ca8539dee5..857f171a27 100644
> > --- a/xen/arch/arm/device.c
> > +++ b/xen/arch/arm/device.c
> > @@ -9,8 +9,10 @@
> >   */
> >  
> >  #include <asm/device.h>
> > +#include <asm/setup.h>
> >  #include <xen/errno.h>
> >  #include <xen/init.h>
> > +#include <xen/iocap.h>
> >  #include <xen/lib.h>
> >  
> >  extern const struct device_desc _sdevice[], _edevice[];
> > @@ -75,6 +77,297 @@ enum device_class device_get_class(const struct 
> > dt_device_node *dev)
> >      return DEVICE_UNKNOWN;
> >  }
> >  
> > +int map_irq_to_domain(struct domain *d, unsigned int irq,
> > +                      bool need_mapping, const char *devname)
> > +{
> > +    int res;
> > +
> > +    res = irq_permit_access(d, irq);
> > +    if ( res )
> > +    {
> > +        printk(XENLOG_ERR "Unable to permit to %pd access to IRQ %u\n", d, 
> > irq);
> > +        return res;
> > +    }
> > +
> > +    if ( need_mapping )
> > +    {
> > +        /*
> > +         * Checking the return of vgic_reserve_virq is not
> > +         * necessary. It should not fail except when we try to map
> > +         * the IRQ twice. This can legitimately happen if the IRQ is shared
> > +         */
> > +        vgic_reserve_virq(d, irq);
> > +
> > +        res = route_irq_to_guest(d, irq, irq, devname);
> > +        if ( res < 0 )
> > +        {
> > +            printk(XENLOG_ERR "Unable to map IRQ%u to %pd\n", irq, d);
> > +            return res;
> > +        }
> > +    }
> > +
> > +    dt_dprintk("  - IRQ: %u\n", irq);
> > +    return 0;
> > +}
> > +
> > +int map_range_to_domain(const struct dt_device_node *dev,
> > +                        uint64_t addr, uint64_t len, void *data)
> > +{
> > +    struct map_range_data *mr_data = data;
> > +    struct domain *d = mr_data->d;
> > +    int res;
> > +
> > +    if ( (addr != (paddr_t)addr) || (((paddr_t)~0 - addr) < len) )
> > +    {
> > +        printk(XENLOG_ERR "%s: [0x%"PRIx64", 0x%"PRIx64"] exceeds the 
> > maximum allowed PA width (%u bits)",
> > +               dt_node_full_name(dev), addr, (addr + len), PADDR_BITS);
> > +        return -ERANGE;
> > +    }
> > +
> > +    /*
> > +     * reserved-memory regions are RAM carved out for a special purpose.
> > +     * They are not MMIO and therefore a domain should not be able to
> > +     * manage them via the IOMEM interface.
> > +     */
> > +    if ( strncasecmp(dt_node_full_name(dev), "/reserved-memory/",
> > +                     strlen("/reserved-memory/")) != 0 )
> > +    {
> > +        res = iomem_permit_access(d, paddr_to_pfn(addr),
> > +                paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
> > +        if ( res )
> > +        {
> > +            printk(XENLOG_ERR "Unable to permit to dom%d access to"
> > +                    " 0x%"PRIx64" - 0x%"PRIx64"\n",
> > +                    d->domain_id,
> > +                    addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
> > +            return res;
> > +        }
> > +    }
> > +
> > +    if ( !mr_data->skip_mapping )
> > +    {
> > +        res = map_regions_p2mt(d,
> > +                               gaddr_to_gfn(addr),
> > +                               PFN_UP(len),
> > +                               maddr_to_mfn(addr),
> > +                               mr_data->p2mt);
> > +
> > +        if ( res < 0 )
> > +        {
> > +            printk(XENLOG_ERR "Unable to map 0x%"PRIx64
> > +                   " - 0x%"PRIx64" in domain %d\n",
> > +                   addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1,
> > +                   d->domain_id);
> > +            return res;
> > +        }
> > +    }
> > +
> > +    dt_dprintk("  - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n",
> > +               addr, addr + len, mr_data->p2mt);
> > +
> > +    return 0;
> > +}
> > +
> > +/*
> > + * handle_device_interrupts retrieves the interrupts configuration from
> > + * a device tree node and maps those interrupts to the target domain.
> > + *
> > + * Returns:
> > + *   < 0 error
> > + *   0   success
> > + */
> > +int handle_device_interrupts(struct domain *d,
> This needs to be renamed. AFAIK you agreed on map_device_irqs_to_domain().
Yeah, i changed this in v11.
> 
> > +                             struct dt_device_node *dev,
> > +                             bool need_mapping)
> > +{
> > +    unsigned int i, nirq;
> > +    int res;
> > +    struct dt_raw_irq rirq;
> > +
> > +    nirq = dt_number_of_irq(dev);
> > +
> > +    /* Give permission and map IRQs */
> > +    for ( i = 0; i < nirq; i++ )
> > +    {
> > +        res = dt_device_get_raw_irq(dev, i, &rirq);
> > +        if ( res )
> > +        {
> > +            printk(XENLOG_ERR "Unable to retrieve irq %u for %s\n",
> > +                   i, dt_node_full_name(dev));
> > +            return res;
> > +        }
> > +
> > +        /*
> > +         * Don't map IRQ that have no physical meaning
> > +         * ie: IRQ whose controller is not the GIC
> > +         */
> > +        if ( rirq.controller != dt_interrupt_controller )
> > +        {
> > +            dt_dprintk("irq %u not connected to primary controller. 
> > Connected to %s\n",
> > +                      i, dt_node_full_name(rirq.controller));
> > +            continue;
> > +        }
> > +
> > +        res = platform_get_irq(dev, i);
> > +        if ( res < 0 )
> > +        {
> > +            printk(XENLOG_ERR "Unable to get irq %u for %s\n",
> > +                   i, dt_node_full_name(dev));
> > +            return res;
> > +        }
> > +
> > +        res = map_irq_to_domain(d, res, need_mapping, dt_node_name(dev));
> > +        if ( res )
> > +            return res;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static int map_dt_irq_to_domain(const struct dt_device_node *dev,
> > +                                       const struct dt_irq *dt_irq,
> > +                                       void *data)
> Parameters are not alligned. Should be:
> static int map_dt_irq_to_domain(const struct dt_device_node *dev,
>                                 const struct dt_irq *dt_irq,
>                                 void *data)
> 
> > +{
> > +    struct map_range_data *mr_data = data;
> > +    struct domain *d = mr_data->d;
> > +    unsigned int irq = dt_irq->irq;
> > +    int res;
> > +
> > +    if ( irq < NR_LOCAL_IRQS )
> > +    {
> > +        printk(XENLOG_ERR "%s: IRQ%u is not a SPI\n", dt_node_name(dev), 
> > irq);
> > +        return -EINVAL;
> > +    }
> > +
> > +    /* Setup the IRQ type */
> > +    res = irq_set_spi_type(irq, dt_irq->type);
> > +    if ( res )
> > +    {
> > +        printk(XENLOG_ERR "%s: Unable to setup IRQ%u to %pd\n",
> > +               dt_node_name(dev), irq, d);
> > +        return res;
> > +    }
> > +
> > +    res = map_irq_to_domain(d, irq, !mr_data->skip_mapping, 
> > dt_node_name(dev));
> > +
> > +    return res;
> > +}
> > +
> > +/*
> > + * For a node which describes a discoverable bus (such as a PCI bus)
> > + * then we may need to perform additional mappings in order to make
> > + * the child resources available to domain 0.
> > + */
> > +static int map_device_children(const struct dt_device_node *dev,
> > +                                      struct map_range_data *mr_data)
> Parameter is not aligned.
Fixed the style here.
> 
> [...]
> > diff --git a/xen/arch/arm/include/asm/setup.h 
> > b/xen/arch/arm/include/asm/setup.h
> > index 19dc637d55..1a052ed924 100644
> > --- a/xen/arch/arm/include/asm/setup.h
> > +++ b/xen/arch/arm/include/asm/setup.h
> > @@ -165,9 +165,18 @@ void device_tree_get_reg(const __be32 **cell, uint32_t 
> > address_cells,
> >  u32 device_tree_get_u32(const void *fdt, int node,
> >                          const char *prop_name, u32 dflt);
> >  
> > +int handle_device(struct domain *d, struct dt_device_node *dev, p2m_type_t 
> > p2mt,
> > +                  struct rangeset *iomem_ranges, struct rangeset 
> > *irq_ranges);
> Remove the rangeset parameters. AFAIK you'll introduce it later, so this is a 
> mistake
> causing the build to fail.
Fixed this.
> 
> > +
> > +int handle_device_interrupts(struct domain *d, struct dt_device_node *dev,
> > +                             bool need_mapping);
> Don't forget to rename.
> 
> 
> With all the remarks above addressed:
> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
> 
> ~Michal
>