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

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 20/08/2019 01:11, Stefano Stabellini wrote:
> 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
I don't think we should treat interrupts property differently depending on what 
was routed to.
As I pointed out before, you could decide to give an interrupt controller (and 
all the associated devices) to the guest. That controller will use a GIC 
interrupts but devices behind it will not.
With your suggestion here, all the devices will not have the 
"interrupts"/"interrupts-extended" property copied over.
[...]

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

--
Julien Grall

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