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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 9 Aug 2019, Julien Grall wrote:
> 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.

Fair enough, I'll add a few words.


> 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.

Yes, the return statement of handle_interrupts, the way I wrote it:

  return !!(need_mapping && res == 0);

is wrong. I'll fix it (also see below).

Stepping back from this specific error, the reason to distinguish
whether a mapping was done or not is to figure out whether we need to
add an interrupt property to the guest device tree. The idea is the
following:

- call handle_interrupts to do any required interrupt mappings
- if any mappings are done, copy over the interrupts property to the guest
  device tree


> >    */
> > -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?

OK


> > +                                    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.

Well spotted!


> >       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.

Yes, I'll remove it


> 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?

That is a mistake: it should return 1 only when mappings are actually
done.

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