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

Re: [Xen-devel] [PATCH v5 21/22] xen/arm: ITS: Generate ITS node for Dom0





On 27/07/2015 04:12, vijay.kilari@xxxxxxxxx wrote:
From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>

Parse host dt and generate ITS node for Dom0.
ITS node resides inside GIC node so when GIC node
is encountered look for ITS node.

Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
---
v5: - Moved ITS dt node generation to ITS driver
v4: - Generate only one ITS node for Dom0
     - Replace msi-parent references to single its phandle
---
  xen/arch/arm/domain_build.c   |   17 ++++++++++
  xen/arch/arm/gic-v3-its.c     |   74 +++++++++++++++++++++++++++++++++++++++++
  xen/arch/arm/gic-v3.c         |   29 ++++++++++++++++
  xen/arch/arm/gic.c            |   18 ++++++++++
  xen/include/asm-arm/gic-its.h |    3 ++
  xen/include/asm-arm/gic.h     |    7 ++++
  6 files changed, 148 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 8556afd..6b6f013 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -469,6 +469,19 @@ static int write_properties(struct domain *d, struct 
kernel_info *kinfo,
              continue;
          }

+        /*
+         * Replace all msi-parent phandle references to single ITS node
+         * generated for Dom0
+         */
+        if ( dt_property_name_is_equal(prop, "msi-parent") )

I think this need more care than replacing every msi-parent without any checking.

You need to make sure that the msi-parent points to an ITS MSI just in case there is other possibility of MSI.

Furthermore, I would do this a ITS specific callback (gic_rewrite_node or smth similar) to avoid replacing msi-parent when it's not necessary. I have in mind GICv2M.

+        {
+            fdt32_t phandle = gic_get_msi_handle();
+            DPRINT(" Set msi-parent(ITS) phandle 0x%x\n",phandle);
+            fdt_property(kinfo->fdt, prop->name, (void *)&phandle,
+                         sizeof(phandle));
+            continue;
+        }
+
          res = fdt_property(kinfo->fdt, prop->name, prop_data, prop_len);

          xfree(new_data);
@@ -875,6 +888,10 @@ static int make_gic_node(const struct domain *d, void *fdt,
          return res;

      res = fdt_end_node(fdt);
+    if ( res )
+        return res;
+
+    res = gic_its_hwdom_dt_node(d, node, fdt);

Can you explain why you didn't follow my suggestion to plumb the ITS node creation in gic_hwdow_dt_node? IHMO there is no need of new callback.

Furthermore the call is misplaced. You will end up to have a DT looking like

gic {
}

gic-its {
}

rather than

gic {
  gic-its {
  }
}

      return res;
  }
diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index 99f6edc..042c70d 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c

+int its_make_dt_node(const struct domain *d,
+                     const struct dt_device_node *node, void *fdt)
+{
+    struct its_node *its;
+    const struct dt_device_node *gic;
+    const void *compatible = NULL;
+    u32 len;
+    __be32 *new_cells, *tmp;
+    int res = 0;
+
+    /* Will pass only first ITS node info */
+    its = list_first_entry(&its_nodes, struct its_node, entry);
+    if ( !its )
+    {
+        dprintk(XENLOG_ERR, "ITS node not found\n");
+        return -FDT_ERR_XEN(ENOENT);
+    }
+
+    gic = its->dt_node;
+
+    compatible = dt_get_property(gic, "compatible", &len);
+    if ( !compatible )
+    {
+        dprintk(XENLOG_ERR, "Can't find compatible property for the its 
node\n");
+        return -FDT_ERR_XEN(ENOENT);
+    }
+
+    res = fdt_begin_node(fdt, "gic-its");
+    if ( res )
+        return res;
+
+    res = fdt_property(fdt, "compatible", compatible, len);
+    if ( res )
+        return res;
+
+    res = fdt_property(fdt, "msi-controller", NULL, 0);
+    if ( res )
+        return res;
+
+    len = dt_cells_to_size(dt_n_addr_cells(node) + dt_n_size_cells(node));
+
+    new_cells = xzalloc_bytes(len);
+    if ( new_cells == NULL )
+        return -FDT_ERR_XEN(ENOMEM);
+    tmp = new_cells;
+
+    dt_set_range(&tmp, node, its->phys_base, its->phys_size);
+
+    res = fdt_property(fdt, "reg", new_cells, len);
+    xfree(new_cells);
+
+    if ( node->phandle )
+    {
+        res = fdt_property_cell(fdt, "phandle", node->phandle);
+        if ( res )
+            return res;
+
+        its_phandle = cpu_to_fdt32(node->phandle);

Why this is set in make_hwdom_dt_node? The ITS phandle may be used before we effectively parse the GIC node.

+    }
+
+    res = fdt_end_node(fdt);
+
+    return res;
+}
+
+
+fdt32_t its_get_lpi_handle(void)
+{
+    return its_phandle;
+}
+
  static int its_force_quiescent(void __iomem *base)
  {
      u32 count = 1000000;   /* 1s */
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 23eb47c..828bf27 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1143,6 +1143,34 @@ static int gicv3_make_hwdom_dt_node(const struct domain 
*d,
      return res;
  }

+static int gicv3_make_hwdom_its_dt_node(const struct domain *d,
+                                        const struct dt_device_node *node,
+                                        void *fdt)
+{
+    struct dt_device_node *gic_child;
+    int res = 0;
+
+    static const struct dt_device_match its_matches[] __initconst =
+    {
+        DT_MATCH_GIC_ITS,
+        { /* sentinel */ },
+    };
+
+    dt_for_each_child_node(node, gic_child)
+    {
+        if ( gic_child != NULL )
+        {
+            if ( dt_match_node(its_matches, gic_child) )
+            {
+                res = its_make_dt_node(d, gic_child, fdt);
+                break;
+            }
+        }
+    }

I already asked on v4, why do you need this loop? The GIC node for the DOM0 is recreating from scratch, there is no need of looping on the current GIC node...


+
+    return res;
+}
+
  static const hw_irq_controller gicv3_host_irq_type = {
      .typename     = "gic-v3",
      .startup      = gicv3_irq_startup,
@@ -1372,6 +1400,7 @@ static const struct gic_hw_operations gicv3_ops = {
      .read_apr            = gicv3_read_apr,
      .secondary_init      = gicv3_secondary_cpu_init,
      .make_hwdom_dt_node  = gicv3_make_hwdom_dt_node,
+    .make_hwdom_its_dt_node  = gicv3_make_hwdom_its_dt_node,
      .is_lpi              = gicv3_is_lpi,
  };

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index f6be0e9..88c1427 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -753,6 +753,15 @@ void __cpuinit init_maintenance_interrupt(void)
                  "irq-maintenance", NULL);
  }

+fdt32_t gic_get_msi_handle(void)
+{
+#ifdef HAS_GICV3
+    if ( gic_lpi_supported() )
+        return its_get_lpi_handle();
+#endif

If you need this introduce a new callback but don't add #ifdef in the common code unless you have a strong argument.

+    return 0;
+}
+

[..]

Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

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