Re: [Xen-devel] [PATCH v3 1/6] xen/arm: introduce handle_interrupts

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hello Stefano,

Stefano Stabellini writes:

> Move the interrupt handling code out of handle_device to a new function
> so that it can be reused for dom0less VMs later.
>
> Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> ---
> Changes in v3:
> - add patch
>
> The diff is hard to read but I just moved the interrupts related code
> from handle_devices to a new function handle_interrupts, and very little
> else.
> ---
>  xen/arch/arm/domain_build.c | 79 +++++++++++++++++++++++--------------
>  1 file changed, 49 insertions(+), 30 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 4c8404155a..00ddb3b05d 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1220,41 +1220,19 @@ static int __init map_device_children(struct domain 
> *d,
>  }
>
>  /*
> - * For a given device node:
> - *  - Give permission to the guest to manage IRQ and MMIO range
> - *  - Retrieve the IRQ configuration (i.e edge/level) from device tree
> - * When the device is not marked for guest passthrough:
> - *  - Assign the device to the guest if it's protected by an IOMMU
> - *  - Map the IRQs and iomem regions to DOM0
> + * Return:
> + *   < 0 on error
> + *   0   on no mapping required
> + *   1   IRQ mapping done
>   */
Are this such returns values really needed? I don't see any code that
depends on return value 0 or 1.

> -static int __init handle_device(struct domain *d, struct dt_device_node *dev,
> -                                p2m_type_t p2mt)
> +static int __init handle_interrupts(struct domain *d,
> +                                    struct dt_device_node *dev,
> +                                    bool need_mapping)
>  {
> -    unsigned int nirq;
> -    unsigned int naddr;
> -    unsigned int i;
> -    int res;
> +    int i, nirq, res;
>      struct dt_raw_irq rirq;
> -    u64 addr, size;
> -    bool need_mapping = !dt_device_for_passthrough(dev);
>
>      nirq = dt_number_of_irq(dev);
> -    naddr = dt_number_of_address(dev);
> -
> -    dt_dprintk("%s passthrough = %d nirq = %d naddr = %u\n",
> -               dt_node_full_name(dev), need_mapping, nirq, naddr);
> -
> -    if ( dt_device_is_protected(dev) && need_mapping )
> -    {
> -        dt_dprintk("%s setup iommu\n", dt_node_full_name(dev));
> -        res = iommu_assign_dt_device(d, dev);
> -        if ( res )
> -        {
> -            printk(XENLOG_ERR "Failed to setup the IOMMU for %s\n",
> -                   dt_node_full_name(dev));
> -            return res;
> -        }
> -    }
>
>      /* Give permission and map IRQs */
>      for ( i = 0; i < nirq; i++ )
> @@ -1291,6 +1269,47 @@ static int __init handle_device(struct domain *d, 
> struct dt_device_node *dev,
>              return res;
>      }
>
> +    return !!(need_mapping && res == 0);
> +}
> +
> +/*
> + * For a given device node:
> + *  - Give permission to the guest to manage IRQ and MMIO range
> + *  - Retrieve the IRQ configuration (i.e edge/level) from device tree
> + * When the device is not marked for guest passthrough:
> + *  - Assign the device to the guest if it's protected by an IOMMU
> + *  - Map the IRQs and iomem regions to DOM0
> + */
> +static int __init handle_device(struct domain *d, struct dt_device_node *dev,
> +                                p2m_type_t p2mt)
> +{
> +    unsigned int naddr;
> +    unsigned int i;
> +    int res;
> +    u64 addr, size;
> +    bool need_mapping = !dt_device_for_passthrough(dev);
> +
> +    naddr = dt_number_of_address(dev);
> +
> +    dt_dprintk("%s passthrough = %d naddr = %u\n",
> +               dt_node_full_name(dev), need_mapping, naddr);
> +
> +    if ( dt_device_is_protected(dev) && need_mapping )
> +    {
> +        dt_dprintk("%s setup iommu\n", dt_node_full_name(dev));
> +        res = iommu_assign_dt_device(d, dev);
> +        if ( res )
> +        {
> +            printk(XENLOG_ERR "Failed to setup the IOMMU for %s\n",
> +                   dt_node_full_name(dev));
> +            return res;
> +        }
> +    }
> +
> +    res = handle_interrupts(d, dev, need_mapping);
> +    if ( res < 0 )
> +        return res;
> +
>      /* Give permission and map MMIOs */
>      for ( i = 0; i < naddr; i++ )
>      {


--
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel