[PATCH v2 0/5] [resend] ARM: ACPI: ITS: Add ITS Support for ACPI hardware domain

[PATCH v2 0/5] [resend] ARM: ACPI: ITS: Add ITS Support for ACPI hardware domain

[mjaggi]

From: Manish Jaggi <mjaggi@cavium.com>

*resending the patch, patch 2/5 had incorrect index 2/2,
Modified patch description for the same patch

This patch is split into 5 patches. First two add support for updating
host_its_list from ACPI MADT table.
The rest patches provide support to update the hardware domain MADT table
with ITS information.

Changes since v1:
- split patches into smaller ones
- removed translation_id



Manish Jaggi (5):
  ARM: ITS: Introduce common function add_to_host_its_list
  ARM: ITS: Populate host_its_list from ACPI MADT Table
  ARM: ITS: Deny hardware domain access to its
  ARM: Introduce get_hwdom_madt_size in gic_hw_operations
  ARM: ITS: Pass ITS in Hardware Domain MADT

 xen/arch/arm/domain_build.c      |   7 +--
 xen/arch/arm/gic-v2.c            |   6 +++
 xen/arch/arm/gic-v3-its.c        | 101 ++++++++++++++++++++++++++++++++++-----
 xen/arch/arm/gic-v3.c            |  33 +++++++++++++
 xen/arch/arm/gic.c               |  11 +++++
 xen/include/asm-arm/gic.h        |   3 ++
 xen/include/asm-arm/gic_v3_its.h |  29 ++++++++++-
 7 files changed, 172 insertions(+), 18 deletions(-)

-- 
2.7.4


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

[PATCH 1/5] ARM: ITS: Introduce common function add_to_host_its_list

[mjaggi]

From: Manish Jaggi <mjaggi@cavium.com>

add_to_host_its_list will update the host_its_list. This common function to
be invoked from gicv3_its_dt_init and gic_v3_its_acpi_init.

Signed-off-by: Manish Jaggi <mjaggi@cavium.com>
---
 xen/arch/arm/gic-v3-its.c | 36 +++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index 2d36030..f844a0d 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -976,12 +976,31 @@ int gicv3_its_make_hwdom_dt_nodes(const struct domain *d,
     return res;
 }
 
+/* Common function for adding to host_its_list */
+static int add_to_host_its_list(u64 addr, u64 size, const void *node)
+{
+    struct host_its *its_data;
+    its_data = xzalloc(struct host_its);
+
+    if ( !its_data )
+        return -1;
+
+    its_data->addr = addr;
+    its_data->size = size;
+    if ( node )
+        its_data->dt_node = node;
+
+    printk("GICv3: Found ITS @0x%lx\n", addr);
+
+    list_add_tail(&its_data->entry, &host_its_list);
+
+    return 0;
+}
+
 /* Scan the DT for any ITS nodes and create a list of host ITSes out of it. */
 void gicv3_its_dt_init(const struct dt_device_node *node)
 {
     const struct dt_device_node *its = NULL;
-    struct host_its *its_data;
-
     /*
      * Check for ITS MSI subnodes. If any, add the ITS register
      * frames to the ITS list.
@@ -996,17 +1015,8 @@ void gicv3_its_dt_init(const struct dt_device_node *node)
         if ( dt_device_get_address(its, 0, &addr, &size) )
             panic("GICv3: Cannot find a valid ITS frame address");
 
-        its_data = xzalloc(struct host_its);
-        if ( !its_data )
-            panic("GICv3: Cannot allocate memory for ITS frame");
-
-        its_data->addr = addr;
-        its_data->size = size;
-        its_data->dt_node = its;
-
-        printk("GICv3: Found ITS @0x%lx\n", addr);
-
-        list_add_tail(&its_data->entry, &host_its_list);
+        if ( add_to_host_its_list(addr, size, its) )
+            panic("GICV3: Adding Host ITS failed ");
     }
 }
 
-- 
2.7.4


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

[PATCH 2/5] ARM: ITS: Populate host_its_list from ACPI MADT Table

[mjaggi]

From: Manish Jaggi <mjaggi@cavium.com>

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.

Signed-off-by: Manish Jaggi <mjaggi@cavium.com>
---
 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)
 
 /*
  * 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)
     }
 }
 
+#ifdef CONFIG_ACPI
+int gicv3_its_acpi_init(struct acpi_subtable_header *header,
+                        const unsigned long end)
+{
+    struct acpi_madt_generic_translator *its;
+
+    its = (struct acpi_madt_generic_translator *)header;
+
+    return add_to_host_its_list(its->base_address,
+                        ACPI_GICV3_ITS_MEM_SIZE, NULL);
+}
+#endif
+
 /*
  * 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);
+
+    if ( count <= 0 )
+        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>
 
 #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);
 
+#ifdef CONFIG_ACPI
+int gicv3_its_acpi_init(struct acpi_subtable_header *header,
+                                    const unsigned long end);
+#endif
 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)
 {
 }
 
+#ifdef CONFIG_ACPI
+static inline int gicv3_its_acpi_init(struct acpi_subtable_header *header,
+                                    const unsigned long end)
+{
+    return false;
+}
+#endif
+
 static inline bool gicv3_its_host_has_its(void)
 {
     return false;
-- 
2.7.4


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

[PATCH 3/5] ARM: ITS: Deny hardware domain access to its

[mjaggi]

From: Manish Jaggi <mjaggi@cavium.com>

This patch extends the gicv3_iomem_deny_access functionality by adding support
for its region as well. Added function gicv3_its_deny_access.

Signed-off-by: Manish Jaggi <mjaggi@cavium.com>
---
 xen/arch/arm/gic-v3-its.c        | 19 +++++++++++++++++++
 xen/arch/arm/gic-v3.c            |  7 +++++++
 xen/include/asm-arm/gic_v3_its.h |  8 ++++++++
 3 files changed, 34 insertions(+)

diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index c4f1288..f584d33 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -20,6 +20,7 @@
 
 #include <xen/lib.h>
 #include <xen/delay.h>
+#include <xen/iocap.h>
 #include <xen/libfdt/libfdt.h>
 #include <xen/mm.h>
 #include <xen/rbtree.h>
@@ -905,6 +906,24 @@ struct pending_irq *gicv3_assign_guest_event(struct domain *d,
     return pirq;
 }
 
+int gicv3_its_deny_access(const struct domain *d)
+{
+    int rc = 0;
+    unsigned long mfn, nr;
+    const struct host_its *its_data;
+
+    list_for_each_entry(its_data, &host_its_list, entry)
+    {
+        mfn = paddr_to_pfn(its_data->addr);
+        nr = PFN_UP(ACPI_GICV3_ITS_MEM_SIZE);
+        rc = iomem_deny_access(d, mfn, mfn + nr);
+        if ( rc )
+            break;
+    }
+
+    return rc;
+}
+
 /*
  * Create the respective guest DT nodes from a list of host ITSes.
  * This copies the reg property, so the guest sees the ITS at the same address
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 0be8942..045d20d 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1308,6 +1308,13 @@ static int gicv3_iomem_deny_access(const struct domain *d)
     if ( rc )
         return rc;
 
+    if ( gicv3_its_host_has_its() )
+    {
+        rc = gicv3_its_deny_access(d);
+        if ( rc )
+            return rc;
+    }
+
     for ( i = 0; i < gicv3.rdist_count; i++ )
     {
         mfn = gicv3.rdist_regions[i].base >> PAGE_SHIFT;
diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h
index b9d8957..a673fba 100644
--- a/xen/include/asm-arm/gic_v3_its.h
+++ b/xen/include/asm-arm/gic_v3_its.h
@@ -139,6 +139,9 @@ 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);
 #endif
+/* Deny iomem access for its */
+int gicv3_its_deny_access(const struct domain *d);
+
 bool gicv3_its_host_has_its(void);
 
 unsigned int vgic_v3_its_count(const struct domain *d);
@@ -208,6 +211,11 @@ static inline int gicv3_its_acpi_init(struct acpi_subtable_header *header,
 }
 #endif
 
+static inline int gicv3_its_deny_access(const struct domain *d)
+{
+    return 0;
+}
+
 static inline bool gicv3_its_host_has_its(void)
 {
     return false;
-- 
2.7.4


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

[PATCH 4/5] ARM: Introduce get_hwdom_madt_size in gic_hw_operations

[mjaggi]

From: Manish Jaggi <mjaggi@cavium.com>

estimate_acpi_efi_size needs to be updated to provide correct size of
hardware domains MADT, which now adds ITS information as well.

Introducing gic_get_hwdom_madt_size.

Signed-off-by: Manish Jaggi <mjaggi@cavium.com>
---
 xen/arch/arm/domain_build.c      |  7 +------
 xen/arch/arm/gic-v2.c            |  6 ++++++
 xen/arch/arm/gic-v3-its.c        | 12 ++++++++++++
 xen/arch/arm/gic-v3.c            | 17 +++++++++++++++++
 xen/arch/arm/gic.c               | 11 +++++++++++
 xen/include/asm-arm/gic.h        |  3 +++
 xen/include/asm-arm/gic_v3_its.h |  6 ++++++
 7 files changed, 56 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 1bec4fa..5739ea4 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1806,12 +1806,7 @@ static int estimate_acpi_efi_size(struct domain *d, struct kernel_info *kinfo)
     acpi_size = ROUNDUP(sizeof(struct acpi_table_fadt), 8);
     acpi_size += ROUNDUP(sizeof(struct acpi_table_stao), 8);
 
-    madt_size = sizeof(struct acpi_table_madt)
-                + sizeof(struct acpi_madt_generic_interrupt) * d->max_vcpus
-                + sizeof(struct acpi_madt_generic_distributor);
-    if ( d->arch.vgic.version == GIC_V3 )
-        madt_size += sizeof(struct acpi_madt_generic_redistributor)
-                     * d->arch.vgic.nr_regions;
+    madt_size = gic_get_hwdom_madt_size(d);
     acpi_size += ROUNDUP(madt_size, 8);
 
     addr = acpi_os_get_root_pointer();
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index cbe71a9..f5ca227 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -1012,6 +1012,11 @@ static int gicv2_iomem_deny_access(const struct domain *d)
     return iomem_deny_access(d, mfn, mfn + nr);
 }
 
+static u32 gicv2_get_hwdom_madt_size(const struct domain *d)
+{
+    return 0;
+}
+
 #ifdef CONFIG_ACPI
 static int gicv2_make_hwdom_madt(const struct domain *d, u32 offset)
 {
@@ -1248,6 +1253,7 @@ const static struct gic_hw_operations gicv2_ops = {
     .read_apr            = gicv2_read_apr,
     .make_hwdom_dt_node  = gicv2_make_hwdom_dt_node,
     .make_hwdom_madt     = gicv2_make_hwdom_madt,
+    .get_hwdom_madt_size = gicv2_get_hwdom_madt_size,
     .map_hwdom_extra_mappings = gicv2_map_hwdown_extra_mappings,
     .iomem_deny_access   = gicv2_iomem_deny_access,
     .do_LPI              = gicv2_do_LPI,
diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index f584d33..82e025e 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -924,6 +924,18 @@ int gicv3_its_deny_access(const struct domain *d)
     return rc;
 }
 
+#ifdef CONFIG_ACPI
+u32 gicv3_its_madt_generic_translator_size(void)
+{
+    const struct host_its *its_data;
+    u32 size = 0;
+
+    list_for_each_entry(its_data, &host_its_list, entry)
+        size += sizeof(struct acpi_madt_generic_translator);
+
+    return size;
+}
+#endif
 /*
  * Create the respective guest DT nodes from a list of host ITSes.
  * This copies the reg property, so the guest sees the ITS at the same address
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 045d20d..6c2b562 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1410,6 +1410,17 @@ static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
     return table_len;
 }
 
+static u32 gicv3_get_hwdom_madt_size(const struct domain *d)
+{
+    u32 size;
+    size  = sizeof(struct acpi_madt_generic_redistributor)
+                     * d->arch.vgic.nr_regions;
+    if ( gicv3_its_host_has_its() )
+        size  += gicv3_its_madt_generic_translator_size();
+
+    return size;
+}
+
 static int __init
 gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
                         const unsigned long end)
@@ -1607,6 +1618,11 @@ static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
 {
     return 0;
 }
+
+static u32 gicv3_get_hwdom_madt_size(const struct domain *d)
+{
+    return 0;
+}
 #endif
 
 /* Set up the GIC */
@@ -1708,6 +1724,7 @@ static const struct gic_hw_operations gicv3_ops = {
     .secondary_init      = gicv3_secondary_cpu_init,
     .make_hwdom_dt_node  = gicv3_make_hwdom_dt_node,
     .make_hwdom_madt     = gicv3_make_hwdom_madt,
+    .get_hwdom_madt_size = gicv3_get_hwdom_madt_size,
     .iomem_deny_access   = gicv3_iomem_deny_access,
     .do_LPI              = gicv3_do_LPI,
 };
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 6c803bf..7bdb603 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -851,6 +851,17 @@ int gic_make_hwdom_madt(const struct domain *d, u32 offset)
     return gic_hw_ops->make_hwdom_madt(d, offset);
 }
 
+u32 gic_get_hwdom_madt_size(const struct domain *d)
+{
+    u32 madt_size;
+    madt_size = sizeof(struct acpi_table_madt)
+                + sizeof(struct acpi_madt_generic_interrupt) * d->max_vcpus
+                + sizeof(struct acpi_madt_generic_distributor)
+                + gic_hw_ops->get_hwdom_madt_size(d);
+
+    return madt_size;
+}
+
 int gic_iomem_deny_access(const struct domain *d)
 {
     return gic_hw_ops->iomem_deny_access(d);
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 6203dc5..a766e42 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -365,6 +365,8 @@ struct gic_hw_operations {
     int (*make_hwdom_madt)(const struct domain *d, u32 offset);
     /* Map extra GIC MMIO, irqs and other hw stuffs to the hardware domain. */
     int (*map_hwdom_extra_mappings)(struct domain *d);
+    /* Query the size of hardware domain madt table */
+    u32 (*get_hwdom_madt_size)(const struct domain *d);
     /* Deny access to GIC regions */
     int (*iomem_deny_access)(const struct domain *d);
     /* Handle LPIs, which require special handling */
@@ -376,6 +378,7 @@ int gic_make_hwdom_dt_node(const struct domain *d,
                            const struct dt_device_node *gic,
                            void *fdt);
 int gic_make_hwdom_madt(const struct domain *d, u32 offset);
+u32 gic_get_hwdom_madt_size(const struct domain *d);
 int gic_map_hwdom_extra_mappings(struct domain *d);
 int gic_iomem_deny_access(const struct domain *d);
 
diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h
index a673fba..b849b16 100644
--- a/xen/include/asm-arm/gic_v3_its.h
+++ b/xen/include/asm-arm/gic_v3_its.h
@@ -138,6 +138,7 @@ void gicv3_its_dt_init(const struct dt_device_node *node);
 #ifdef CONFIG_ACPI
 int gicv3_its_acpi_init(struct acpi_subtable_header *header,
                                     const unsigned long end);
+u32 gicv3_its_madt_generic_translator_size(void);
 #endif
 /* Deny iomem access for its */
 int gicv3_its_deny_access(const struct domain *d);
@@ -209,6 +210,11 @@ static inline int gicv3_its_acpi_init(struct acpi_subtable_header *header,
 {
     return false;
 }
+
+static inline u32 gicv3_its_madt_generic_translator_size(void)
+{
+    return 0;
+}
 #endif
 
 static inline int gicv3_its_deny_access(const struct domain *d)
-- 
2.7.4


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

[PATCH 5/5] ARM: ITS: Pass ITS in Hardware Domain MADT

[mjaggi]

From: Manish Jaggi <mjaggi@cavium.com>

Adds gicv3_its_make_hwdom_madt to update hwdom MADT ITS information.

Signed-off-by: Manish Jaggi <mjaggi@cavium.com>
---
 xen/arch/arm/gic-v3-its.c        | 24 ++++++++++++++++++++++++
 xen/arch/arm/gic-v3.c            |  1 +
 xen/include/asm-arm/gic_v3_its.h |  1 +
 3 files changed, 26 insertions(+)

diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index 82e025e..6e0a701 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -935,6 +935,30 @@ u32 gicv3_its_madt_generic_translator_size(void)
 
     return size;
 }
+
+u32 gicv3_its_make_hwdom_madt(u8 *base_ptr, u32 offset)
+{
+    const struct host_its *its_data;
+    u32 table_len = offset, i = 0, size;
+    struct acpi_madt_generic_translator *fw_its;
+    struct acpi_madt_generic_translator *hwdom_its;
+
+    size = sizeof(struct acpi_madt_generic_translator);
+
+    /* Update GIC ITS information in hardware domain's MADT */
+    list_for_each_entry(its_data, &host_its_list, entry)
+    {
+        hwdom_its = (struct acpi_madt_generic_translator *)(base_ptr
+                   + table_len);
+        fw_its = (struct acpi_madt_generic_translator *)
+                    acpi_table_get_entry_madt(
+                        ACPI_MADT_TYPE_GENERIC_TRANSLATOR, i++);
+        memcpy(hwdom_its, fw_its, size);
+        table_len +=  size;
+    }
+
+    return table_len;
+}
 #endif
 /*
  * Create the respective guest DT nodes from a list of host ITSes.
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 6c2b562..30b29c9 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1407,6 +1407,7 @@ static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
         table_len += size;
     }
 
+    table_len = gicv3_its_make_hwdom_madt(base_ptr, table_len);
     return table_len;
 }
 
diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h
index b849b16..8955451 100644
--- a/xen/include/asm-arm/gic_v3_its.h
+++ b/xen/include/asm-arm/gic_v3_its.h
@@ -139,6 +139,7 @@ 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);
 u32 gicv3_its_madt_generic_translator_size(void);
+u32 gicv3_its_make_hwdom_madt(u8 *base_ptr, u32 offset);
 #endif
 /* Deny iomem access for its */
 int gicv3_its_deny_access(const struct domain *d);
-- 
2.7.4


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

Re: [PATCH 1/5] ARM: ITS: Introduce common function add_to_host_its_list

[Julien Grall]

Hello,

On 13/08/17 22:30, mjaggi@caviumnetworks.com wrote:
> From: Manish Jaggi <mjaggi@cavium.com>
>
> add_to_host_its_list will update the host_its_list. This common function to
> be invoked from gicv3_its_dt_init and gic_v3_its_acpi_init.
>
> Signed-off-by: Manish Jaggi <mjaggi@cavium.com>
> ---
>  xen/arch/arm/gic-v3-its.c | 36 +++++++++++++++++++++++-------------
>  1 file changed, 23 insertions(+), 13 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 2d36030..f844a0d 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -976,12 +976,31 @@ int gicv3_its_make_hwdom_dt_nodes(const struct domain *d,
>      return res;
>  }
>
> +/* Common function for adding to host_its_list */
> +static int add_to_host_its_list(u64 addr, u64 size, const void *node)

Why void *? node will be always assigned to dt_node and we should keep 
some type safety.

Also this function only return -1 or 0. Please use boolean.

> +{
> +    struct host_its *its_data;

Missing newline between the declaration and the code.

> +    its_data = xzalloc(struct host_its);
> +
> +    if ( !its_data )
> +        return -1;
> +
> +    its_data->addr = addr;
> +    its_data->size = size;
> +    if ( node )

This check is pointless. If it is NULL then dt_node will be NULL and 
this is what we want.

> +        its_data->dt_node = node;
> +
> +    printk("GICv3: Found ITS @0x%lx\n", addr);
> +
> +    list_add_tail(&its_data->entry, &host_its_list);
> +
> +    return 0;
> +}
> +
>  /* Scan the DT for any ITS nodes and create a list of host ITSes out of it. */
>  void gicv3_its_dt_init(const struct dt_device_node *node)
>  {
>      const struct dt_device_node *its = NULL;
> -    struct host_its *its_data;
> -

Why this newline is dropped?

>      /*
>       * Check for ITS MSI subnodes. If any, add the ITS register
>       * frames to the ITS list.
> @@ -996,17 +1015,8 @@ void gicv3_its_dt_init(const struct dt_device_node *node)
>          if ( dt_device_get_address(its, 0, &addr, &size) )
>              panic("GICv3: Cannot find a valid ITS frame address");
>
> -        its_data = xzalloc(struct host_its);
> -        if ( !its_data )
> -            panic("GICv3: Cannot allocate memory for ITS frame");
> -
> -        its_data->addr = addr;
> -        its_data->size = size;
> -        its_data->dt_node = its;
> -
> -        printk("GICv3: Found ITS @0x%lx\n", addr);
> -
> -        list_add_tail(&its_data->entry, &host_its_list);
> +        if ( add_to_host_its_list(addr, size, its) )
> +            panic("GICV3: Adding Host ITS failed ");
>      }
>  }
>
>

Cheers,

-- 
Julien Grall

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

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

[Julien Grall]

Hello,

On 13/08/17 22:30, mjaggi@caviumnetworks.com wrote:
> From: Manish Jaggi <mjaggi@cavium.com>
>
> Added gicv3_its_acpi_init to update host_its_list from MADT table.
> For ACPI, host_its sturcture  stores dt_node as NULL.

s/sturcture/structure/

>
> 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@cavium.com>
> ---
>  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)
>      }
>  }
>
> +#ifdef CONFIG_ACPI
> +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.

> +}
> +#endif
> +
>  /*
>   * 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);
>
> +#ifdef CONFIG_ACPI
> +int gicv3_its_acpi_init(struct acpi_subtable_header *header,
> +                                    const unsigned long end);
> +#endif

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)
>  {
>  }
>
> +#ifdef CONFIG_ACPI
> +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.

> +}
> +#endif
> +
>  static inline bool gicv3_its_host_has_its(void)
>  {
>      return false;
>

Cheers,

-- 
Julien Grall

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

Re: [PATCH 3/5] ARM: ITS: Deny hardware domain access to its

[Julien Grall]

Hello,

On 13/08/17 22:30, mjaggi@caviumnetworks.com wrote:
> From: Manish Jaggi <mjaggi@cavium.com>
>
> This patch extends the gicv3_iomem_deny_access functionality by adding support
> for its region as well. Added function gicv3_its_deny_access.

s/its/ITS/ making clearer the commit message.

s/Added/Add/

>
> Signed-off-by: Manish Jaggi <mjaggi@cavium.com>
> ---
>  xen/arch/arm/gic-v3-its.c        | 19 +++++++++++++++++++
>  xen/arch/arm/gic-v3.c            |  7 +++++++
>  xen/include/asm-arm/gic_v3_its.h |  8 ++++++++
>  3 files changed, 34 insertions(+)
>
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index c4f1288..f584d33 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -20,6 +20,7 @@
>
>  #include <xen/lib.h>
>  #include <xen/delay.h>
> +#include <xen/iocap.h>
>  #include <xen/libfdt/libfdt.h>
>  #include <xen/mm.h>
>  #include <xen/rbtree.h>
> @@ -905,6 +906,24 @@ struct pending_irq *gicv3_assign_guest_event(struct domain *d,
>      return pirq;
>  }
>
> +int gicv3_its_deny_access(const struct domain *d)
> +{
> +    int rc = 0;
> +    unsigned long mfn, nr;
> +    const struct host_its *its_data;
> +
> +    list_for_each_entry(its_data, &host_its_list, entry)
> +    {
> +        mfn = paddr_to_pfn(its_data->addr);
> +        nr = PFN_UP(ACPI_GICV3_ITS_MEM_SIZE);
> +        rc = iomem_deny_access(d, mfn, mfn + nr);
> +        if ( rc )
> +            break;
> +    }
> +
> +    return rc;
> +}
> +
>  /*
>   * Create the respective guest DT nodes from a list of host ITSes.
>   * This copies the reg property, so the guest sees the ITS at the same address
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 0be8942..045d20d 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1308,6 +1308,13 @@ static int gicv3_iomem_deny_access(const struct domain *d)
>      if ( rc )
>          return rc;
>
> +    if ( gicv3_its_host_has_its() )

gicv3_its_deny_access will do nothing and return 0 when there are no ITS 
present. Same when Xen does not support ITS. So please drop this 
pointless check.

> +    {
> +        rc = gicv3_its_deny_access(d);
> +        if ( rc )
> +            return rc;
> +    }
> +
>      for ( i = 0; i < gicv3.rdist_count; i++ )
>      {
>          mfn = gicv3.rdist_regions[i].base >> PAGE_SHIFT;
> diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h
> index b9d8957..a673fba 100644
> --- a/xen/include/asm-arm/gic_v3_its.h
> +++ b/xen/include/asm-arm/gic_v3_its.h
> @@ -139,6 +139,9 @@ 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);
>  #endif

Newline here please.

> +/* Deny iomem access for its */
> +int gicv3_its_deny_access(const struct domain *d);
> +
>  bool gicv3_its_host_has_its(void);
>
>  unsigned int vgic_v3_its_count(const struct domain *d);
> @@ -208,6 +211,11 @@ static inline int gicv3_its_acpi_init(struct acpi_subtable_header *header,
>  }
>  #endif
>
> +static inline int gicv3_its_deny_access(const struct domain *d)
> +{
> +    return 0;
> +}
> +
>  static inline bool gicv3_its_host_has_its(void)
>  {
>      return false;
>

Cheers,

-- 
Julien Grall

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

Re: [PATCH 1/5] ARM: ITS: Introduce common function add_to_host_its_list

[Julien Grall]

On 13/08/17 22:30, mjaggi@caviumnetworks.com wrote:
> From: Manish Jaggi <mjaggi@cavium.com>
>
> add_to_host_its_list will update the host_its_list. This common function to
> be invoked from gicv3_its_dt_init and gic_v3_its_acpi_init.
>
> Signed-off-by: Manish Jaggi <mjaggi@cavium.com>
> ---
>  xen/arch/arm/gic-v3-its.c | 36 +++++++++++++++++++++++-------------
>  1 file changed, 23 insertions(+), 13 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 2d36030..f844a0d 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -976,12 +976,31 @@ int gicv3_its_make_hwdom_dt_nodes(const struct domain *d,
>      return res;
>  }
>
> +/* Common function for adding to host_its_list */
> +static int add_to_host_its_list(u64 addr, u64 size, const void *node)

BTW this should be paddr_t and not u64 for both.

> +{
> +    struct host_its *its_data;
> +    its_data = xzalloc(struct host_its);
> +
> +    if ( !its_data )
> +        return -1;
> +
> +    its_data->addr = addr;
> +    its_data->size = size;
> +    if ( node )
> +        its_data->dt_node = node;
> +
> +    printk("GICv3: Found ITS @0x%lx\n", addr);
> +
> +    list_add_tail(&its_data->entry, &host_its_list);
> +
> +    return 0;
> +}
> +
>  /* Scan the DT for any ITS nodes and create a list of host ITSes out of it. */
>  void gicv3_its_dt_init(const struct dt_device_node *node)
>  {
>      const struct dt_device_node *its = NULL;
> -    struct host_its *its_data;
> -
>      /*
>       * Check for ITS MSI subnodes. If any, add the ITS register
>       * frames to the ITS list.
> @@ -996,17 +1015,8 @@ void gicv3_its_dt_init(const struct dt_device_node *node)
>          if ( dt_device_get_address(its, 0, &addr, &size) )
>              panic("GICv3: Cannot find a valid ITS frame address");
>
> -        its_data = xzalloc(struct host_its);
> -        if ( !its_data )
> -            panic("GICv3: Cannot allocate memory for ITS frame");
> -
> -        its_data->addr = addr;
> -        its_data->size = size;
> -        its_data->dt_node = its;
> -
> -        printk("GICv3: Found ITS @0x%lx\n", addr);
> -
> -        list_add_tail(&its_data->entry, &host_its_list);
> +        if ( add_to_host_its_list(addr, size, its) )
> +            panic("GICV3: Adding Host ITS failed ");
>      }
>  }
>
>

-- 
Julien Grall

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

Re: [PATCH 4/5] ARM: Introduce get_hwdom_madt_size in gic_hw_operations

[Julien Grall]

Hello,

On 13/08/17 22:30, mjaggi@caviumnetworks.com wrote:
> From: Manish Jaggi <mjaggi@cavium.com>
>
> estimate_acpi_efi_size needs to be updated to provide correct size of
> hardware domains MADT, which now adds ITS information as well.
>
> Introducing gic_get_hwdom_madt_size.
>
> Signed-off-by: Manish Jaggi <mjaggi@cavium.com>
> ---
>  xen/arch/arm/domain_build.c      |  7 +------
>  xen/arch/arm/gic-v2.c            |  6 ++++++
>  xen/arch/arm/gic-v3-its.c        | 12 ++++++++++++
>  xen/arch/arm/gic-v3.c            | 17 +++++++++++++++++
>  xen/arch/arm/gic.c               | 11 +++++++++++
>  xen/include/asm-arm/gic.h        |  3 +++
>  xen/include/asm-arm/gic_v3_its.h |  6 ++++++
>  7 files changed, 56 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 1bec4fa..5739ea4 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1806,12 +1806,7 @@ static int estimate_acpi_efi_size(struct domain *d, struct kernel_info *kinfo)
>      acpi_size = ROUNDUP(sizeof(struct acpi_table_fadt), 8);
>      acpi_size += ROUNDUP(sizeof(struct acpi_table_stao), 8);
>
> -    madt_size = sizeof(struct acpi_table_madt)
> -                + sizeof(struct acpi_madt_generic_interrupt) * d->max_vcpus
> -                + sizeof(struct acpi_madt_generic_distributor);
> -    if ( d->arch.vgic.version == GIC_V3 )
> -        madt_size += sizeof(struct acpi_madt_generic_redistributor)
> -                     * d->arch.vgic.nr_regions;
> +    madt_size = gic_get_hwdom_madt_size(d);
>      acpi_size += ROUNDUP(madt_size, 8);
>
>      addr = acpi_os_get_root_pointer();
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index cbe71a9..f5ca227 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -1012,6 +1012,11 @@ static int gicv2_iomem_deny_access(const struct domain *d)
>      return iomem_deny_access(d, mfn, mfn + nr);
>  }
>
> +static u32 gicv2_get_hwdom_madt_size(const struct domain *d)

Please no more use of u32, use uint32_t. But, the return is a bit weird. 
Why 32-bit when the code is using size_t.

I think this should return unsigned long given you use them in 
combination with _xmalloc.

> +{
> +    return 0;
> +}
> +
>  #ifdef CONFIG_ACPI
>  static int gicv2_make_hwdom_madt(const struct domain *d, u32 offset)
>  {
> @@ -1248,6 +1253,7 @@ const static struct gic_hw_operations gicv2_ops = {
>      .read_apr            = gicv2_read_apr,
>      .make_hwdom_dt_node  = gicv2_make_hwdom_dt_node,
>      .make_hwdom_madt     = gicv2_make_hwdom_madt,
> +    .get_hwdom_madt_size = gicv2_get_hwdom_madt_size,
>      .map_hwdom_extra_mappings = gicv2_map_hwdown_extra_mappings,
>      .iomem_deny_access   = gicv2_iomem_deny_access,
>      .do_LPI              = gicv2_do_LPI,
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index f584d33..82e025e 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -924,6 +924,18 @@ int gicv3_its_deny_access(const struct domain *d)
>      return rc;
>  }
>
> +#ifdef CONFIG_ACPI
> +u32 gicv3_its_madt_generic_translator_size(void)

See my comment above.

> +{
> +    const struct host_its *its_data;
> +    u32 size = 0;

Same here.

> +
> +    list_for_each_entry(its_data, &host_its_list, entry)
> +        size += sizeof(struct acpi_madt_generic_translator);
> +
> +    return size;
> +}

Overall, I don't think this function is necessary. Instead you should 
use vgic_v3_its_count given that the ACPI table will for the hardware 
domain.

> +#endif
>  /*
>   * Create the respective guest DT nodes from a list of host ITSes.
>   * This copies the reg property, so the guest sees the ITS at the same address
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 045d20d..6c2b562 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1410,6 +1410,17 @@ static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
>      return table_len;
>  }
>
> +static u32 gicv3_get_hwdom_madt_size(const struct domain *d)

Ditto

> +{
> +    u32 size;

Ditto + newline here.

> +    size  = sizeof(struct acpi_madt_generic_redistributor)
> +                     * d->arch.vgic.nr_regions;
> +    if ( gicv3_its_host_has_its() )
> +        size  += gicv3_its_madt_generic_translator_size();

size += vgic_v3_its_count(d);

> +
> +    return size;
> +}
> +
>  static int __init
>  gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
>                          const unsigned long end)
> @@ -1607,6 +1618,11 @@ static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
>  {
>      return 0;
>  }
> +
> +static u32 gicv3_get_hwdom_madt_size(const struct domain *d)
> +{
> +    return 0;
> +}
>  #endif
>
>  /* Set up the GIC */
> @@ -1708,6 +1724,7 @@ static const struct gic_hw_operations gicv3_ops = {
>      .secondary_init      = gicv3_secondary_cpu_init,
>      .make_hwdom_dt_node  = gicv3_make_hwdom_dt_node,
>      .make_hwdom_madt     = gicv3_make_hwdom_madt,
> +    .get_hwdom_madt_size = gicv3_get_hwdom_madt_size,
>      .iomem_deny_access   = gicv3_iomem_deny_access,
>      .do_LPI              = gicv3_do_LPI,
>  };
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 6c803bf..7bdb603 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -851,6 +851,17 @@ int gic_make_hwdom_madt(const struct domain *d, u32 offset)
>      return gic_hw_ops->make_hwdom_madt(d, offset);
>  }
>
> +u32 gic_get_hwdom_madt_size(const struct domain *d)

See above for u32.

> +{
> +    u32 madt_size;

Ditto + newline.

> +    madt_size = sizeof(struct acpi_table_madt)
> +                + sizeof(struct acpi_madt_generic_interrupt) * d->max_vcpus
> +                + sizeof(struct acpi_madt_generic_distributor)
> +                + gic_hw_ops->get_hwdom_madt_size(d);
> +
> +    return madt_size;
> +}
> +
>  int gic_iomem_deny_access(const struct domain *d)
>  {
>      return gic_hw_ops->iomem_deny_access(d);
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 6203dc5..a766e42 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -365,6 +365,8 @@ struct gic_hw_operations {
>      int (*make_hwdom_madt)(const struct domain *d, u32 offset);
>      /* Map extra GIC MMIO, irqs and other hw stuffs to the hardware domain. */
>      int (*map_hwdom_extra_mappings)(struct domain *d);
> +    /* Query the size of hardware domain madt table */
> +    u32 (*get_hwdom_madt_size)(const struct domain *d);

See above for u32.

>      /* Deny access to GIC regions */
>      int (*iomem_deny_access)(const struct domain *d);
>      /* Handle LPIs, which require special handling */
> @@ -376,6 +378,7 @@ int gic_make_hwdom_dt_node(const struct domain *d,
>                             const struct dt_device_node *gic,
>                             void *fdt);
>  int gic_make_hwdom_madt(const struct domain *d, u32 offset);
> +u32 gic_get_hwdom_madt_size(const struct domain *d);
>  int gic_map_hwdom_extra_mappings(struct domain *d);
>  int gic_iomem_deny_access(const struct domain *d);
>
> diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h
> index a673fba..b849b16 100644
> --- a/xen/include/asm-arm/gic_v3_its.h
> +++ b/xen/include/asm-arm/gic_v3_its.h
> @@ -138,6 +138,7 @@ void gicv3_its_dt_init(const struct dt_device_node *node);
>  #ifdef CONFIG_ACPI
>  int gicv3_its_acpi_init(struct acpi_subtable_header *header,
>                                      const unsigned long end);
> +u32 gicv3_its_madt_generic_translator_size(void);

Ditto.

>  #endif
>  /* Deny iomem access for its */
>  int gicv3_its_deny_access(const struct domain *d);
> @@ -209,6 +210,11 @@ static inline int gicv3_its_acpi_init(struct acpi_subtable_header *header,
>  {
>      return false;
>  }
> +
> +static inline u32 gicv3_its_madt_generic_translator_size(void)
> +{
> +    return 0;
> +}
>  #endif
>
>  static inline int gicv3_its_deny_access(const struct domain *d)
>

Cheers,

-- 
Julien Grall

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

Re: [PATCH 5/5] ARM: ITS: Pass ITS in Hardware Domain MADT

[Julien Grall]

Hello,

Title: xen/arm: ITS: Expose ITS in the MADT table

On 13/08/17 22:30, mjaggi@caviumnetworks.com wrote:
> From: Manish Jaggi <mjaggi@cavium.com>
>
> Adds gicv3_its_make_hwdom_madt to update hwdom MADT ITS information.

s/Adds/Add/

>
> Signed-off-by: Manish Jaggi <mjaggi@cavium.com>
> ---
>  xen/arch/arm/gic-v3-its.c        | 24 ++++++++++++++++++++++++
>  xen/arch/arm/gic-v3.c            |  1 +
>  xen/include/asm-arm/gic_v3_its.h |  1 +
>  3 files changed, 26 insertions(+)
>
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 82e025e..6e0a701 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -935,6 +935,30 @@ u32 gicv3_its_madt_generic_translator_size(void)
>
>      return size;
>  }
> +
> +u32 gicv3_its_make_hwdom_madt(u8 *base_ptr, u32 offset)

Pretty much all my remarks in the previous patch for u* are valid here 
too. I will not repeat them and for the whole the patch.

> +{
> +    const struct host_its *its_data;
> +    u32 table_len = offset, i = 0, size;
> +    struct acpi_madt_generic_translator *fw_its;
> +    struct acpi_madt_generic_translator *hwdom_its;
> +
> +    size = sizeof(struct acpi_madt_generic_translator);
> +
> +    /* Update GIC ITS information in hardware domain's MADT */
> +    list_for_each_entry(its_data, &host_its_list, entry)

Please look at vgic_v3_its_count to avoid introduce dummy variables:

for ( i = 0; i < vgic_v3_its_count(...); i++ )

> +    {
> +        hwdom_its = (struct acpi_madt_generic_translator *)(base_ptr
> +                   + table_len);
> +        fw_its = (struct acpi_madt_generic_translator *)
> +                    acpi_table_get_entry_madt(
> +                        ACPI_MADT_TYPE_GENERIC_TRANSLATOR, i++);

Please check the return here + panic if it does not work.

> +        memcpy(hwdom_its, fw_its, size);
> +        table_len +=  size;

This code is too complicate for not much reason. If you do:

hwdom_its = (struct acpi_madt_generic_translator)(base_ptr + offset);

for ( i = 0; i < vgic_v3_its_count(...); i++ )
{
	fw_its = (struct .... *)acpi_table_get_...

	hwdom_its++;
}

return (offset + sizeof(...) * vgic_v3_its_count());

That would be much clear.

> +    }
> +
> +    return table_len;
> +}
>  #endif
>  /*
>   * Create the respective guest DT nodes from a list of host ITSes.
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 6c2b562..30b29c9 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1407,6 +1407,7 @@ static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
>          table_len += size;
>      }
>
> +    table_len = gicv3_its_make_hwdom_madt(base_ptr, table_len);

Newline here.

>      return table_len;
>  }
>
> diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h
> index b849b16..8955451 100644
> --- a/xen/include/asm-arm/gic_v3_its.h
> +++ b/xen/include/asm-arm/gic_v3_its.h
> @@ -139,6 +139,7 @@ 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);
>  u32 gicv3_its_madt_generic_translator_size(void);
> +u32 gicv3_its_make_hwdom_madt(u8 *base_ptr, u32 offset);
>  #endif
>  /* Deny iomem access for its */
>  int gicv3_its_deny_access(const struct domain *d);
>

Cheers,

-- 
Julien Grall

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