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

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 09/08/2019 00:12, Stefano Stabellini wrote:
> 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
This feels a bit odd to describe the return value and not what the function 
does.

But I don't understand why you need to tell the caller whether mapping were done 
or not. This is already conveyed by "need_mapping" provided by the caller.
Looking at the only place where you make the distinction between 0 and 1 (patch 
#3), you have
+            r = handle_interrupts(d, node, true);
+            if ( r < 0 )
+                return r;
+            if ( r > 0 )
+            {
               /* do something */
+            }


Not looking at the code below (which looks wrong), as you always pass true here, 
r can either be an error 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,
How about handle_device_interrupts? Or map_device_interrupts?

> +                                    struct dt_device_node *dev,
> +                                    bool need_mapping)
>   {
> -    unsigned int nirq;
> -    unsigned int naddr;
> -    unsigned int i;
> -    int res;
> +    int i, nirq, res;
res will be used unitialized if the device has no interrupts.

>       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);
Why do you need the !! here? (a && b) is already a boolean. But this looks 
pretty wrong as you would return 0 when res is non-zero (i.e an error) and 
need_mapping is true.
But looking at the code, res cannot be 0 here... So why are you checking "res" 
here?

> +}
> +
> +/*
> + * 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++ )
>       {
Cheers,

--
Julien Grall

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