[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


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

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

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

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

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

See above.

Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.