diff mbox series

[v7,1/2] xen/arm: extend fdt_property_interrupts to support DomU

Message ID 20190807101028.28778-2-viktor.mitin.19@gmail.com (mailing list archive)
State Superseded
Headers show
Series xen/arm: Consolidate make_timer_node and make_timer_domU_node | expand

Commit Message

Viktor Mitin Aug. 7, 2019, 10:10 a.m. UTC 
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 next patch which consolidates
make_timer_node and make_timer_domU_node".
Original goal is to consolidate make_timer functions.

Suggested-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Viktor Mitin <viktor_mitin@epam.com>
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
v7: removed extra space after sizeof in fdt_property_interrupts()
---
 xen/arch/arm/domain_build.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

Comments

Julien Grall Aug. 7, 2019, 12:26 p.m. UTC | #1 
Hi Viktor,

On 07/08/2019 11:10, 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.

To be pedantic, the current prototype for fdt_property_interrupts() can already 
deal with either hwdom and DomU. What your patch does is passing kinfo, so you 
only use parameter rather than two. So how about:

"The domain and fdt can be found in the structure kinfo. Rather than adding a an 
extra argument for the domain, pass directly kinfo. This also requires to adapt 
fdt_property_interrupts() prototype."

> 
> This is a preparatory for the next patch which consolidates
> make_timer_node and make_timer_domU_node".

In v3, I pointed out that it is a bit risky to write down the title of a patch 
that does not preceded it (or been committed). Imagine we can decide to rename 
it. Furthermore, "next patch" implies they are committed one after the other.

It is fairly common to apply part of the series if the rest needs some rework. 
So a better (and safer wording) is "A follow-up patch will need to create the 
interrupts for either Dom0 or DomU".

> Original goal is to consolidate make_timer functions.

With my suggestion above, this sentence can be dropped.

The rest of the patch looks good to me. I am happy to update the commit message 
while committing it.

Let me know your preference.

Cheers,
diff mbox series

Patch

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 4c8404155a..26cd0ae12c 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -621,17 +621,20 @@  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,
+static int __init fdt_property_interrupts(const struct kernel_info *kinfo,
+                                          gic_interrupt_t *intr,
                                           unsigned num_irq)
 {
     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 +736,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 +963,9 @@  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)
 {
+    void *fdt = kinfo->fdt;
     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 )