[PATCH 0/3] xen/arm: Rework the way to allocate event channel IRQ for hwdom

[PATCH 0/3] xen/arm: Rework the way to allocate event channel IRQ for hwdom

[julien.grall]

From: Julien Grall <julien.grall@arm.com>

Hi all,

This patch series was triggered by commit 11e7dd958d "xen/arm: domain_builder:
irq sanity check logic fix" that is valid but break some assumption in the
current Xen. Rather than removing a useful BUG_ON(), this series rework
the event channel IRQ allocation for the hardware domain.

The last patch of the series will reinstance commit 11e7dd958d.

Cheers,

Julien Grall (2):
  xen/arm: domain_build: Prepare DTB/ACPI tables after specific mappings
  xen/arm: domain_build: Rework the way to allocate the event channel
    interrupt

Stewart Hildebrand (1):
  xen/arm: domain_builder: irq sanity check logic fix

 xen/arch/arm/domain_build.c | 95 ++++++++++++++++++++++-----------------------
 1 file changed, 46 insertions(+), 49 deletions(-)

-- 
2.11.0


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

[PATCH 1/3] xen/arm: domain_build: Prepare DTB/ACPI tables after specific mappings

[julien.grall]

From: Julien Grall <julien.grall@arm.com>

A follow-up patch will require to have all interrupts routed to the
hardware registered before calling prepare_dtb/prepare_acpi.

At the moment, it is not necessary to call platform specific mappings
(gic and platform) after, so it is fine to move them.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/domain_build.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 941688a2ce..a5e5c82355 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2145,14 +2145,6 @@ int construct_dom0(struct domain *d)
     allocate_memory(d, &kinfo);
     find_gnttab_region(d, &kinfo);
 
-    if ( acpi_disabled )
-        rc = prepare_dtb(d, &kinfo);
-    else
-        rc = prepare_acpi(d, &kinfo);
-
-    if ( rc < 0 )
-        return rc;
-
     /* Map extra GIC MMIO, irqs and other hw stuffs to dom0. */
     rc = gic_map_hwdom_extra_mappings(d);
     if ( rc < 0 )
@@ -2162,6 +2154,14 @@ int construct_dom0(struct domain *d)
     if ( rc < 0 )
         return rc;
 
+    if ( acpi_disabled )
+        rc = prepare_dtb(d, &kinfo);
+    else
+        rc = prepare_acpi(d, &kinfo);
+
+    if ( rc < 0 )
+        return rc;
+
     /*
      * The following loads use the domain's p2m and require current to
      * be a vcpu of the domain, temporarily switch
-- 
2.11.0


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

Re: [PATCH 1/3] xen/arm: domain_build: Prepare DTB/ACPI tables after specific mappings

[Stefano Stabellini]

On Tue, 27 Feb 2018, julien.grall@arm.com wrote:
> From: Julien Grall <julien.grall@arm.com>
> 
> A follow-up patch will require to have all interrupts routed to the
> hardware registered before calling prepare_dtb/prepare_acpi.
> 
> At the moment, it is not necessary to call platform specific mappings
> (gic and platform) after, so it is fine to move them.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/arch/arm/domain_build.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 941688a2ce..a5e5c82355 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2145,14 +2145,6 @@ int construct_dom0(struct domain *d)
>      allocate_memory(d, &kinfo);
>      find_gnttab_region(d, &kinfo);
>  
> -    if ( acpi_disabled )
> -        rc = prepare_dtb(d, &kinfo);
> -    else
> -        rc = prepare_acpi(d, &kinfo);
> -
> -    if ( rc < 0 )
> -        return rc;
> -
>      /* Map extra GIC MMIO, irqs and other hw stuffs to dom0. */
>      rc = gic_map_hwdom_extra_mappings(d);
>      if ( rc < 0 )
> @@ -2162,6 +2154,14 @@ int construct_dom0(struct domain *d)
>      if ( rc < 0 )
>          return rc;
>  
> +    if ( acpi_disabled )
> +        rc = prepare_dtb(d, &kinfo);
> +    else
> +        rc = prepare_acpi(d, &kinfo);
> +
> +    if ( rc < 0 )
> +        return rc;
> +
>      /*
>       * The following loads use the domain's p2m and require current to
>       * be a vcpu of the domain, temporarily switch
> -- 
> 2.11.0
> 

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

[PATCH 2/3] xen/arm: domain_build: Rework the way to allocate the event channel interrupt

[julien.grall]

From: Julien Grall <julien.grall@arm.com>

At the moment, a placeholder will be created in the device-tree for the
event channel information. Later in the domain construction, the
interrupt for the event channel upcall will be allocated the device-tree
fixed up.

Looking at the code, the current split is not necessary because all the
PPIs used by the hardware domain will by the time we create the node in
the device-tree.

From now, mandate that all interrupts are registered before
acpi_prepare() and dtb_prepare(). This allows us to rework the event
channel code and remove one placeholder.

Note, this will also help to fix the BUG(...) condition in set_interrupt_ppi
which is completely wrong. See in a follow-up patch.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/domain_build.c | 74 +++++++++++++++++++++------------------------
 1 file changed, 35 insertions(+), 39 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index a5e5c82355..ed1a393bb5 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -576,7 +576,10 @@ static int make_memory_node(const struct domain *d,
     return res;
 }
 
-static int make_hypervisor_node(const struct kernel_info *kinfo,
+static void evtchn_allocate(struct domain *d);
+
+static int make_hypervisor_node(struct domain *d,
+                                const struct kernel_info *kinfo,
                                 const struct dt_device_node *parent)
 {
     const char compat[] =
@@ -620,10 +623,18 @@ static int make_hypervisor_node(const struct kernel_info *kinfo,
         return res;
 
     /*
-     * Placeholder for the event channel interrupt.  The values will be
-     * replaced later.
+     * It is safe to allocate the event channel here because all the
+     * PPIs used by the hardware domain have been registered.
+     */
+    evtchn_allocate(d);
+
+    /*
+     * Interrupt event channel upcall:
+     *  - Active-low level-sensitive
+     *  - All CPUs
+     *  TODO: Handle properly the cpumask;
      */
-    set_interrupt_ppi(intr, ~0, 0xf, IRQ_TYPE_INVALID);
+    set_interrupt_ppi(intr, d->arch.evtchn_irq, 0xf, IRQ_TYPE_LEVEL_LOW);
     res = fdt_property_interrupts(fdt, &intr, 1);
     if ( res )
         return res;
@@ -1282,7 +1293,11 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
 
     if ( node == dt_host )
     {
-        res = make_hypervisor_node(kinfo, node);
+        /*
+         * The hypervisor node should always be created after all nodes
+         * from the host DT have been parsed.
+         */
+        res = make_hypervisor_node(d, kinfo, node);
         if ( res )
             return res;
 
@@ -1939,6 +1954,12 @@ static int prepare_acpi(struct domain *d, struct kernel_info *kinfo)
     if ( rc != 0 )
         return rc;
 
+    /*
+     * All PPIs have been registered, allocate the event channel
+     * interrupts.
+     */
+    evtchn_allocate(d);
+
     return 0;
 }
 #else
@@ -2014,16 +2035,18 @@ static void initrd_load(struct kernel_info *kinfo)
         panic("Unable to copy the initrd in the hwdom memory");
 }
 
-static void evtchn_fixup(struct domain *d, struct kernel_info *kinfo)
+/*
+ * Allocate the event channel PPIs and setup the HVM_PARAM_CALLBACK_IRQ.
+ * The allocated IRQ will be found in d->arch.evtchn_irq.
+ *
+ * Note that this should only be called once all PPIs used by the
+ * hardware domain have been registered.
+ */
+static void evtchn_allocate(struct domain *d)
 {
-    int res, node;
+    int res;
     u64 val;
-    gic_interrupt_t intr;
 
-    /*
-     * The allocation of the event channel IRQ has been deferred until
-     * now. At this time, all PPIs used by DOM0 have been registered.
-     */
     res = vgic_allocate_ppi(d);
     if ( res < 0 )
         panic("Unable to allocate a PPI for the event channel interrupt\n");
@@ -2041,31 +2064,6 @@ static void evtchn_fixup(struct domain *d, struct kernel_info *kinfo)
                      HVM_PARAM_CALLBACK_TYPE_PPI_FLAG_MASK);
     val |= d->arch.evtchn_irq;
     d->arch.hvm_domain.params[HVM_PARAM_CALLBACK_IRQ] = val;
-
-    /*
-     * When booting Dom0 using ACPI, Dom0 can only get the event channel
-     * interrupt via hypercall.
-     */
-    if ( !acpi_disabled )
-        return;
-
-    /* Fix up "interrupts" in /hypervisor node */
-    node = fdt_path_offset(kinfo->fdt, "/hypervisor");
-    if ( node < 0 )
-        panic("Cannot find the /hypervisor node");
-
-    /* Interrupt event channel upcall:
-     *  - Active-low level-sensitive
-     *  - All CPUs
-     *
-     *  TODO: Handle properly the cpumask
-     */
-    set_interrupt_ppi(intr, d->arch.evtchn_irq, 0xf,
-                      IRQ_TYPE_LEVEL_LOW);
-    res = fdt_setprop_inplace(kinfo->fdt, node, "interrupts",
-                              &intr, sizeof(intr));
-    if ( res )
-        panic("Cannot fix up \"interrupts\" property of the hypervisor node");
 }
 
 static void __init find_gnttab_region(struct domain *d,
@@ -2177,8 +2175,6 @@ int construct_dom0(struct domain *d)
     kernel_load(&kinfo);
     /* initrd_load will fix up the fdt, so call it before dtb_load */
     initrd_load(&kinfo);
-    /* Allocate the event channel IRQ and fix up the device tree */
-    evtchn_fixup(d, &kinfo);
     dtb_load(&kinfo);
 
     /* Now that we are done restore the original p2m and current. */
-- 
2.11.0


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

Re: [PATCH 2/3] xen/arm: domain_build: Rework the way to allocate the event channel interrupt

[Stefano Stabellini]

On Tue, 27 Feb 2018, julien.grall@arm.com wrote:
> From: Julien Grall <julien.grall@arm.com>
> 
> At the moment, a placeholder will be created in the device-tree for the
> event channel information. Later in the domain construction, the
> interrupt for the event channel upcall will be allocated the device-tree
> fixed up.
> 
> Looking at the code, the current split is not necessary because all the
> PPIs used by the hardware domain will by the time we create the node in
> the device-tree.
> 
> >From now, mandate that all interrupts are registered before
> acpi_prepare() and dtb_prepare(). This allows us to rework the event
> channel code and remove one placeholder.
> 
> Note, this will also help to fix the BUG(...) condition in set_interrupt_ppi
> which is completely wrong. See in a follow-up patch.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/arch/arm/domain_build.c | 74 +++++++++++++++++++++------------------------
>  1 file changed, 35 insertions(+), 39 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index a5e5c82355..ed1a393bb5 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -576,7 +576,10 @@ static int make_memory_node(const struct domain *d,
>      return res;
>  }
>  
> -static int make_hypervisor_node(const struct kernel_info *kinfo,
> +static void evtchn_allocate(struct domain *d);
> +
> +static int make_hypervisor_node(struct domain *d,
> +                                const struct kernel_info *kinfo,
>                                  const struct dt_device_node *parent)
>  {
>      const char compat[] =
> @@ -620,10 +623,18 @@ static int make_hypervisor_node(const struct kernel_info *kinfo,
>          return res;
>  
>      /*
> -     * Placeholder for the event channel interrupt.  The values will be
> -     * replaced later.
> +     * It is safe to allocate the event channel here because all the
> +     * PPIs used by the hardware domain have been registered.
> +     */
> +    evtchn_allocate(d);
> +
> +    /*
> +     * Interrupt event channel upcall:
> +     *  - Active-low level-sensitive
> +     *  - All CPUs
> +     *  TODO: Handle properly the cpumask;
>       */
> -    set_interrupt_ppi(intr, ~0, 0xf, IRQ_TYPE_INVALID);
> +    set_interrupt_ppi(intr, d->arch.evtchn_irq, 0xf, IRQ_TYPE_LEVEL_LOW);
>      res = fdt_property_interrupts(fdt, &intr, 1);
>      if ( res )
>          return res;
> @@ -1282,7 +1293,11 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
>  
>      if ( node == dt_host )
>      {
> -        res = make_hypervisor_node(kinfo, node);
> +        /*
> +         * The hypervisor node should always be created after all nodes
> +         * from the host DT have been parsed.
> +         */
> +        res = make_hypervisor_node(d, kinfo, node);
>          if ( res )
>              return res;
>  
> @@ -1939,6 +1954,12 @@ static int prepare_acpi(struct domain *d, struct kernel_info *kinfo)
>      if ( rc != 0 )
>          return rc;
>  
> +    /*
> +     * All PPIs have been registered, allocate the event channel
> +     * interrupts.
> +     */
> +    evtchn_allocate(d);
> +
>      return 0;
>  }
>  #else
> @@ -2014,16 +2035,18 @@ static void initrd_load(struct kernel_info *kinfo)
>          panic("Unable to copy the initrd in the hwdom memory");
>  }
>  
> -static void evtchn_fixup(struct domain *d, struct kernel_info *kinfo)
> +/*
> + * Allocate the event channel PPIs and setup the HVM_PARAM_CALLBACK_IRQ.
> + * The allocated IRQ will be found in d->arch.evtchn_irq.
> + *
> + * Note that this should only be called once all PPIs used by the
> + * hardware domain have been registered.
> + */
> +static void evtchn_allocate(struct domain *d)
>  {
> -    int res, node;
> +    int res;
>      u64 val;
> -    gic_interrupt_t intr;
>  
> -    /*
> -     * The allocation of the event channel IRQ has been deferred until
> -     * now. At this time, all PPIs used by DOM0 have been registered.
> -     */
>      res = vgic_allocate_ppi(d);
>      if ( res < 0 )
>          panic("Unable to allocate a PPI for the event channel interrupt\n");
> @@ -2041,31 +2064,6 @@ static void evtchn_fixup(struct domain *d, struct kernel_info *kinfo)
>                       HVM_PARAM_CALLBACK_TYPE_PPI_FLAG_MASK);
>      val |= d->arch.evtchn_irq;
>      d->arch.hvm_domain.params[HVM_PARAM_CALLBACK_IRQ] = val;
> -
> -    /*
> -     * When booting Dom0 using ACPI, Dom0 can only get the event channel
> -     * interrupt via hypercall.
> -     */
> -    if ( !acpi_disabled )
> -        return;
> -
> -    /* Fix up "interrupts" in /hypervisor node */
> -    node = fdt_path_offset(kinfo->fdt, "/hypervisor");
> -    if ( node < 0 )
> -        panic("Cannot find the /hypervisor node");
> -
> -    /* Interrupt event channel upcall:
> -     *  - Active-low level-sensitive
> -     *  - All CPUs
> -     *
> -     *  TODO: Handle properly the cpumask
> -     */
> -    set_interrupt_ppi(intr, d->arch.evtchn_irq, 0xf,
> -                      IRQ_TYPE_LEVEL_LOW);
> -    res = fdt_setprop_inplace(kinfo->fdt, node, "interrupts",
> -                              &intr, sizeof(intr));
> -    if ( res )
> -        panic("Cannot fix up \"interrupts\" property of the hypervisor node");
>  }
>  
>  static void __init find_gnttab_region(struct domain *d,
> @@ -2177,8 +2175,6 @@ int construct_dom0(struct domain *d)
>      kernel_load(&kinfo);
>      /* initrd_load will fix up the fdt, so call it before dtb_load */
>      initrd_load(&kinfo);
> -    /* Allocate the event channel IRQ and fix up the device tree */
> -    evtchn_fixup(d, &kinfo);
>      dtb_load(&kinfo);
>  
>      /* Now that we are done restore the original p2m and current. */
> -- 
> 2.11.0
> 

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

[PATCH 3/3] xen/arm: domain_builder: irq sanity check logic fix

[julien.grall]

From: Stewart Hildebrand <Stewart.Hildebrand@dornerworks.com>

Since commit "xen/arm: domain_build: Rework the way to allocate the
event channel interrupt", it is not possible for an irq to be both below 16
and greater/equal than 32.

Also fix the reference to linux documentation while we're at it.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com>
Signed-off-by: Julien Grall <julien.grall@arm.com>
[Slightly rework the commit message]
---
 xen/arch/arm/domain_build.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index ed1a393bb5..28ee876b92 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -503,9 +503,10 @@ static void set_interrupt_ppi(gic_interrupt_t interrupt, unsigned int irq,
 {
     __be32 *cells = interrupt;
 
-    BUG_ON(irq < 16 && irq >= 32);
+    BUG_ON(irq < 16);
+    BUG_ON(irq >= 32);
 
-    /* See linux Documentation/devictree/bindings/arm/gic.txt */
+    /* See linux Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt */
     dt_set_cell(&cells, 1, 1); /* is a PPI */
     dt_set_cell(&cells, 1, irq - 16); /* PPIs start at 16 */
     dt_set_cell(&cells, 1, (cpumask << 8) | level);
-- 
2.11.0


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

Re: [PATCH 3/3] xen/arm: domain_builder: irq sanity check logic fix

[Stefano Stabellini]

On Tue, 27 Feb 2018, julien.grall@arm.com wrote:
> From: Stewart Hildebrand <Stewart.Hildebrand@dornerworks.com>
> 
> Since commit "xen/arm: domain_build: Rework the way to allocate the
> event channel interrupt", it is not possible for an irq to be both below 16
> and greater/equal than 32.
> 
> Also fix the reference to linux documentation while we're at it.
> 
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@dornerworks.com>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> [Slightly rework the commit message]

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/arch/arm/domain_build.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index ed1a393bb5..28ee876b92 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -503,9 +503,10 @@ static void set_interrupt_ppi(gic_interrupt_t interrupt, unsigned int irq,
>  {
>      __be32 *cells = interrupt;
>  
> -    BUG_ON(irq < 16 && irq >= 32);
> +    BUG_ON(irq < 16);
> +    BUG_ON(irq >= 32);
>  
> -    /* See linux Documentation/devictree/bindings/arm/gic.txt */
> +    /* See linux Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt */
>      dt_set_cell(&cells, 1, 1); /* is a PPI */
>      dt_set_cell(&cells, 1, irq - 16); /* PPIs start at 16 */
>      dt_set_cell(&cells, 1, (cpumask << 8) | level);
> -- 
> 2.11.0
> 

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