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

Re: [Xen-devel] [PATCH 2/5] ARM: ITS: Populate host_its_list from ACPI MADT Table


On 13/08/17 22:30, mjaggi@xxxxxxxxxxxxxxxxxx wrote:
From: Manish Jaggi <mjaggi@xxxxxxxxxx>

Added gicv3_its_acpi_init to update host_its_list from MADT table.
For ACPI, host_its sturcture  stores dt_node as NULL.


Future TOD0:
Cleanup :(1) Remove from host_its dt_node as it is required only for ACPI
Enhancement :(2) Provide a method to access translation_id and
other fields of madt generic translator.

I don't get those TODOs. This is not related to this patch and does not really hence the commit message.

Signed-off-by: Manish Jaggi <mjaggi@xxxxxxxxxx>
 xen/arch/arm/gic-v3-its.c        | 14 ++++++++++++++
 xen/arch/arm/gic-v3.c            |  8 ++++++++
 xen/include/asm-arm/gic_v3_its.h | 13 +++++++++++++
 3 files changed, 35 insertions(+)

diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index f844a0d..c4f1288 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -32,6 +32,7 @@
 #include <asm/page.h>

 #define ITS_CMD_QUEUE_SZ                SZ_1M
+#define ACPI_GICV3_ITS_MEM_SIZE        (SZ_64K)

The (...) are not necessary.

  * No lock here, as this list gets only populated upon boot while scanning
@@ -1020,6 +1021,19 @@ void gicv3_its_dt_init(const struct dt_device_node *node)

+int gicv3_its_acpi_init(struct acpi_subtable_header *header,
+                        const unsigned long end)

I don't much like the idea of providing a callback that will be called by gic-v3.c. I would much prefer to follow the same pattern as the DT where gic-v3.c will call a function in the gic-v3-its.c that will iterate on all possible ITS.

This will make a more sane interface.

Also, it would make sense to call it gicv3_its_acpi_probe.

+    struct acpi_madt_generic_translator *its;
+    its = (struct acpi_madt_generic_translator *)header;

You probably want to add check BAD_MADT_ENTRY(its, end).

+    return add_to_host_its_list(its->base_address,
+                        ACPI_GICV3_ITS_MEM_SIZE, NULL);

If you follow my suggestion in patch #1 regarding the return of add_to_host_its_list, then you would need to check true/false and return a correct errno.

Even if you don't follow it, please return an appropriate errno rather than -1.

  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index f990eae..0be8942 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1567,6 +1567,14 @@ static void __init gicv3_acpi_init(void)

     gicv3.rdist_stride = 0;

+    /* Parse ITS information */
+    count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
+                                  gicv3_its_acpi_init, 0);

See my comment above.

+    if ( count <= 0 )

Hardware without ITS support will return 0 and therefore panic. You don't want this to happen.

+        panic("GICv3: Can't get ITS entry");
      * In ACPI, 0 is considered as the invalid address. However the rest
      * of the initialization rely on the invalid address to be
diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h
index 1fac1c7..2b7493d 100644
--- a/xen/include/asm-arm/gic_v3_its.h
+++ b/xen/include/asm-arm/gic_v3_its.h
@@ -105,6 +105,7 @@

 #include <xen/device_tree.h>
 #include <xen/rbtree.h>
+#include <xen/acpi.h>

With the suggestion suggested above, you will not need to include <xen/acpi.h> in gic_v3_its.h.

 #define HOST_ITS_FLUSH_CMD_QUEUE        (1U << 0)
 #define HOST_ITS_USES_PTA               (1U << 1)
@@ -135,6 +136,10 @@ extern struct list_head host_its_list;
 /* Parse the host DT and pick up all host ITSes. */
 void gicv3_its_dt_init(const struct dt_device_node *node);

+int gicv3_its_acpi_init(struct acpi_subtable_header *header,
+                                    const unsigned long end);

Newline here please.

 bool gicv3_its_host_has_its(void);

 unsigned int vgic_v3_its_count(const struct domain *d);
@@ -196,6 +201,14 @@ static inline void gicv3_its_dt_init(const struct 
dt_device_node *node)

+static inline int gicv3_its_acpi_init(struct acpi_subtable_header *header,
+                                    const unsigned long end)
+    return false;

gicv3_its_acpi_init return an int not a bool. Please modify this accordingly.

 static inline bool gicv3_its_host_has_its(void)
     return false;


Julien Grall

Xen-devel mailing list



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