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

Re: [Xen-devel] [PATCH v4 1/2] xen/arm: extend fdt_property_interrupts



Hi,

You should have enough characters in the title to explain what you are extending. Something like:

xen/arm: Extend fdt_property_interrupts to support domU

On 31/07/2019 11:28, Viktor Mitin wrote:
Extend fdt_property_interrupts to deal with other domain than the hwdom.

The prototype of fdt_property_interrupts() has been modified
to support both hwdom and domU in one function.

This is a preparatory for the patch "xen/arm: merge make_timer_node and
make_timer_domU_node". Original goal is to merge make_timer_node and
make_timer_domU_node functions. See discussion in e-mail, the Message-ID is:
<20190620103805.927-1-viktor.mitin.19@xxxxxxxxx>

The commit message should not point to discussion but summarize it. If you want to point to the discussion, then please do it after ---.

Also, this is a bit risky to write down the title of a patch that does not preceded it (or been committed). Image we decide to rename it... Instead, it is common to say "A follow-up patch will need to create the interrupts for either dom0 or domU".


Note: there is no functional changes introduced by this patch.

You are expanding the function to this is technically a functional changes.


Suggested-by: Julien Grall <julien.grall@xxxxxxx>
Signed-off-by: Viktor Mitin <viktor_mitin@xxxxxxxx>
---
  xen/arch/arm/domain_build.c | 22 +++++++++++++---------
  1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 4c8404155a..d04a1c3aec 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -621,17 +621,19 @@ static void __init set_interrupt(gic_interrupt_t 
interrupt,
   *  "interrupts": contains the list of interrupts
   *  "interrupt-parent": link to the GIC
   */
-static int __init fdt_property_interrupts(void *fdt, gic_interrupt_t *intr,
-                                          unsigned num_irq)
+static int __init fdt_property_interrupts(const struct kernel_info *kinfo,
+                            gic_interrupt_t *intr, unsigned num_irq)

Please align the section line with the first argument.

  {
      int res;
+    uint32_t phandle = is_hardware_domain(kinfo->d) ?
+                       dt_interrupt_controller->phandle : GUEST_PHANDLE_GIC;
- res = fdt_property(fdt, "interrupts", intr, sizeof (intr[0]) * num_irq);
+    res = fdt_property(kinfo->fdt, "interrupts",
+                       intr, sizeof (intr[0]) * num_irq);
      if ( res )
          return res;
- res = fdt_property_cell(fdt, "interrupt-parent",
-                            dt_interrupt_controller->phandle);
+    res = fdt_property_cell(kinfo->fdt, "interrupt-parent", phandle);
return res;
  }
@@ -733,7 +735,7 @@ static int __init make_hypervisor_node(struct domain *d,
       *  TODO: Handle properly the cpumask;
       */
      set_interrupt(intr, d->arch.evtchn_irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
-    res = fdt_property_interrupts(fdt, &intr, 1);
+    res = fdt_property_interrupts(kinfo, &intr, 1);
      if ( res )
          return res;
@@ -960,8 +962,10 @@ static int __init make_gic_node(const struct domain *d, void *fdt,
      return res;
  }
-static int __init make_timer_node(const struct domain *d, void *fdt)
+static int __init make_timer_node(const struct kernel_info *kinfo)

This change is still not explained in the commit message.

  {
+    void *fdt = kinfo->fdt;
+

You add the newline here but drop it in the next patch. In general, it is strongly recommended to not add code this is removed in the same series unless there are a reason to.

      static const struct dt_device_match timer_ids[] __initconst =
      {
          DT_MATCH_COMPATIBLE("arm,armv7-timer"),
@@ -1016,7 +1020,7 @@ static int __init make_timer_node(const struct domain *d, 
void *fdt)
      dt_dprintk("  Virt interrupt %u\n", irq);
      set_interrupt(intrs[2], irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
- res = fdt_property_interrupts(fdt, intrs, 3);
+    res = fdt_property_interrupts(kinfo, intrs, 3);
      if ( res )
          return res;
@@ -1377,7 +1381,7 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
      if ( device_get_class(node) == DEVICE_GIC )
          return make_gic_node(d, kinfo->fdt, node);
      if ( dt_match_node(timer_matches, node) )
-        return make_timer_node(d, kinfo->fdt);
+        return make_timer_node(kinfo);
/* Skip nodes used by Xen */
      if ( dt_device_used_by(node) == DOMID_XEN )


Cheers,

--
Julien Grall

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

 


Rackspace

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