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 <stefa...@xilinx.com>
---
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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
