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

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

[mjaggi]

From: Manish Jaggi <mjaggi@cavium.com>

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 v5
- fixed indentation
- fixed return value check of gicv3_its_deny_access
- moved GICV3_ITS_SIZE definition 

Changes since v4
- gic_hw_operations callback name changed to include "extra"
   gic_get_hwdom_extra_madt_size
- newline and ws issues fixed.
- updated commit message for patch 4.

Changes since v3
- Set GICV3_ITS_SIZE as 128K
- updated gicv2_get_hwdom_madt_size
- Removed offset from gicv3_its_make_hwdom_madt

Changes since v2:
- %s/u32/unsigned long
- %s/u64/paddr_t
- cleanup gicv3_its_make_hwdom_madt as per review comments
- remove gicv3_its_host_has_its() checks in patch 3
- removed gicv3_its_madt_generic_translator_size() 

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: Update Formula to compute MADT size using new callbacks in
    gic_hw_operations
  ARM: ITS: Expose ITS in the MADT table

 xen/arch/arm/domain_build.c      |  7 +---
 xen/arch/arm/gic-v2.c            |  6 +++
 xen/arch/arm/gic-v3-its.c        | 91 ++++++++++++++++++++++++++++++++++++----
 xen/arch/arm/gic-v3.c            | 27 ++++++++++++
 xen/arch/arm/gic.c               | 12 ++++++
 xen/include/asm-arm/gic.h        |  3 ++
 xen/include/asm-arm/gic_v3_its.h | 27 ++++++++++++
 7 files changed, 158 insertions(+), 15 deletions(-)

-- 
2.7.4


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

[PATCH v6 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_probe.

Signed-off-by: Manish Jaggi <mjaggi@cavium.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Acked-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/gic-v3-its.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index 2d36030..0610991 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -976,11 +976,29 @@ int gicv3_its_make_hwdom_dt_nodes(const struct domain *d,
     return res;
 }
 
+/* Common function for adding to host_its_list */
+static void add_to_host_its_list(paddr_t addr, paddr_t size,
+                                 const struct dt_device_node *node)
+{
+    struct host_its *its_data;
+
+    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 = node;
+
+    printk("GICv3: Found ITS @0x%lx\n", addr);
+
+    list_add_tail(&its_data->entry, &host_its_list);
+}
+
 /* 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
@@ -996,17 +1014,7 @@ 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);
+        add_to_host_its_list(addr, size, its);
     }
 }
 
-- 
2.7.4


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

[PATCH v6 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 structure stores dt_node as NULL.

Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Manish Jaggi <mjaggi@cavium.com>
---
 xen/arch/arm/gic-v3-its.c        | 24 ++++++++++++++++++++++++
 xen/arch/arm/gic-v3.c            |  2 ++
 xen/include/asm-arm/gic_v3_its.h | 10 ++++++++++
 3 files changed, 36 insertions(+)

diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index 0610991..3023ee5 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -18,6 +18,7 @@
  * along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <xen/acpi.h>
 #include <xen/lib.h>
 #include <xen/delay.h>
 #include <xen/libfdt/libfdt.h>
@@ -1018,6 +1019,29 @@ void gicv3_its_dt_init(const struct dt_device_node *node)
     }
 }
 
+#ifdef CONFIG_ACPI
+static int gicv3_its_acpi_probe(struct acpi_subtable_header *header,
+                                const unsigned long end)
+{
+    struct acpi_madt_generic_translator *its;
+
+    its = (struct acpi_madt_generic_translator *)header;
+    if ( BAD_MADT_ENTRY(its, end) )
+        return -EINVAL;
+
+    add_to_host_its_list(its->base_address, GICV3_ITS_SIZE, NULL);
+
+    return 0;
+}
+
+void gicv3_its_acpi_init(void)
+{
+    /* Parse ITS information */
+    acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
+                          gicv3_its_acpi_probe, 0);
+}
+#endif
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index f990eae..6f562f4 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1567,6 +1567,8 @@ static void __init gicv3_acpi_init(void)
 
     gicv3.rdist_stride = 0;
 
+    gicv3_its_acpi_init();
+
     /*
      * 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..73d1fd1 100644
--- a/xen/include/asm-arm/gic_v3_its.h
+++ b/xen/include/asm-arm/gic_v3_its.h
@@ -102,6 +102,7 @@
 #define GITS_CMD_DISCARD                0x0f
 
 #define ITS_DOORBELL_OFFSET             0x10040
+#define GICV3_ITS_SIZE                  SZ_128K
 
 #include <xen/device_tree.h>
 #include <xen/rbtree.h>
@@ -135,6 +136,9 @@ 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
+void gicv3_its_acpi_init(void);
+#endif
 bool gicv3_its_host_has_its(void);
 
 unsigned int vgic_v3_its_count(const struct domain *d);
@@ -196,6 +200,12 @@ static inline void gicv3_its_dt_init(const struct dt_device_node *node)
 {
 }
 
+#ifdef CONFIG_ACPI
+static inline void gicv3_its_acpi_init(void)
+{
+}
+#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 v6 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. Add function gicv3_its_deny_access.

Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Acked-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Manish Jaggi <mjaggi@cavium.com>
---
 xen/arch/arm/gic-v3-its.c        | 22 ++++++++++++++++++++++
 xen/arch/arm/gic-v3.c            |  4 ++++
 xen/include/asm-arm/gic_v3_its.h |  9 +++++++++
 3 files changed, 35 insertions(+)

diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index 3023ee5..bd94308 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -21,6 +21,7 @@
 #include <xen/acpi.h>
 #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,27 @@ 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(its_data->size);
+        rc = iomem_deny_access(d, mfn, mfn + nr);
+        if ( rc )
+        {
+            printk("iomem_deny_access failed for %lx:%lx \r\n", mfn, nr);
+            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 6f562f4..475e0d3 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1308,6 +1308,10 @@ static int gicv3_iomem_deny_access(const struct domain *d)
     if ( rc )
         return rc;
 
+    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 73d1fd1..73ee0ba 100644
--- a/xen/include/asm-arm/gic_v3_its.h
+++ b/xen/include/asm-arm/gic_v3_its.h
@@ -139,6 +139,10 @@ void gicv3_its_dt_init(const struct dt_device_node *node);
 #ifdef CONFIG_ACPI
 void gicv3_its_acpi_init(void);
 #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);
@@ -206,6 +210,11 @@ static inline void gicv3_its_acpi_init(void)
 }
 #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 v6 4/5] ARM: Update Formula to compute MADT size using new callbacks 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.

This patch updates the formula to compute extra MADT size, as per GICv2/3
by calling gic_get_hwdom_extra_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.c       | 19 +++++++++++++++++++
 xen/arch/arm/gic.c          | 12 ++++++++++++
 xen/include/asm-arm/gic.h   |  3 +++
 5 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index d6f9585..f17fcf1 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1808,12 +1808,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..0123ea4 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 unsigned long gicv2_get_hwdom_extra_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_extra_madt_size = gicv2_get_hwdom_extra_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.c b/xen/arch/arm/gic-v3.c
index 475e0d3..0289d1a 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1407,6 +1407,19 @@ static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
     return table_len;
 }
 
+static unsigned long gicv3_get_hwdom_extra_madt_size(const struct domain *d)
+{
+    unsigned long size;
+
+    size = sizeof(struct acpi_madt_generic_redistributor)
+           * d->arch.vgic.nr_regions;
+
+    size += sizeof(struct acpi_madt_generic_translator)
+            * vgic_v3_its_count(d);
+
+    return size;
+}
+
 static int __init
 gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
                         const unsigned long end)
@@ -1598,6 +1611,11 @@ static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
 {
     return 0;
 }
+
+static unsigned long gicv3_get_hwdom_extra_madt_size(const struct domain *d)
+{
+    return 0;
+}
 #endif
 
 /* Set up the GIC */
@@ -1699,6 +1717,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_extra_madt_size = gicv3_get_hwdom_extra_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..3c7b6df 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -851,6 +851,18 @@ int gic_make_hwdom_madt(const struct domain *d, u32 offset)
     return gic_hw_ops->make_hwdom_madt(d, offset);
 }
 
+unsigned long gic_get_hwdom_madt_size(const struct domain *d)
+{
+    unsigned long 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_extra_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..0612706 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 */
+    unsigned long (*get_hwdom_extra_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);
+unsigned long 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);
 
-- 
2.7.4


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

[PATCH v6 5/5] ARM: ITS: Expose ITS in the MADT table

[mjaggi]

From: Manish Jaggi <mjaggi@cavium.com>

Add gicv3_its_make_hwdom_madt to update hwdom MADT ITS information.

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

diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index bd94308..e57ae05 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -1062,6 +1062,25 @@ void gicv3_its_acpi_init(void)
     acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
                           gicv3_its_acpi_probe, 0);
 }
+
+unsigned long gicv3_its_make_hwdom_madt(const struct domain *d, void *base_ptr)
+{
+    unsigned int i;
+    void *fw_its;
+    struct acpi_madt_generic_translator *hwdom_its;
+
+    hwdom_its = base_ptr;
+
+    for ( i = 0; i < vgic_v3_its_count(d); i++ )
+    {
+        fw_its = acpi_table_get_entry_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
+                                           i);
+        memcpy(hwdom_its, fw_its, sizeof(struct acpi_madt_generic_translator));
+        hwdom_its++;
+    }
+
+    return sizeof(struct acpi_madt_generic_translator) * vgic_v3_its_count(d);
+}
 #endif
 
 /*
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 0289d1a..e9b9060 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1404,6 +1404,8 @@ static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
         table_len += size;
     }
 
+    table_len += gicv3_its_make_hwdom_madt(d, 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 73ee0ba..40dffdc 100644
--- a/xen/include/asm-arm/gic_v3_its.h
+++ b/xen/include/asm-arm/gic_v3_its.h
@@ -138,6 +138,8 @@ void gicv3_its_dt_init(const struct dt_device_node *node);
 
 #ifdef CONFIG_ACPI
 void gicv3_its_acpi_init(void);
+unsigned long gicv3_its_make_hwdom_madt(const struct domain *d,
+                                        void *base_ptr);
 #endif
 
 /* Deny iomem access for its */
@@ -208,6 +210,12 @@ static inline void gicv3_its_dt_init(const struct dt_device_node *node)
 static inline void gicv3_its_acpi_init(void)
 {
 }
+
+static inline unsigned long gicv3_its_make_hwdom_madt(const struct domain *d,
+                                                      void *base_ptr)
+{
+    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

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

[Julien Grall]

Hi Manish,

On 10/10/17 13:52, 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. Add function gicv3_its_deny_access.
> 
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> Acked-by: Julien Grall <julien.grall@arm.com>

Please state after "---" when you modified a patch and keep the tags to 
at least check if the reviewer is happy with it.

It is one of the reason I like the changelog in each patch. It helps to 
know what changed in a specific one. It helps me to decide whether I am 
happy with you keeping my tag and avoid to fully review yet another time 
the patch.

In that case, it is fine to keep it.

> Signed-off-by: Manish Jaggi <mjaggi@cavium.com> > ---
>   xen/arch/arm/gic-v3-its.c        | 22 ++++++++++++++++++++++
>   xen/arch/arm/gic-v3.c            |  4 ++++
>   xen/include/asm-arm/gic_v3_its.h |  9 +++++++++
>   3 files changed, 35 insertions(+)
> 
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 3023ee5..bd94308 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -21,6 +21,7 @@
>   #include <xen/acpi.h>
>   #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,27 @@ 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(its_data->size);
> +        rc = iomem_deny_access(d, mfn, mfn + nr);
> +        if ( rc )
> +        {
> +            printk("iomem_deny_access failed for %lx:%lx \r\n", mfn, nr);
> +            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 6f562f4..475e0d3 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1308,6 +1308,10 @@ static int gicv3_iomem_deny_access(const struct domain *d)
>       if ( rc )
>           return rc;
>   
> +    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 73d1fd1..73ee0ba 100644
> --- a/xen/include/asm-arm/gic_v3_its.h
> +++ b/xen/include/asm-arm/gic_v3_its.h
> @@ -139,6 +139,10 @@ void gicv3_its_dt_init(const struct dt_device_node *node);
>   #ifdef CONFIG_ACPI
>   void gicv3_its_acpi_init(void);
>   #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);
> @@ -206,6 +210,11 @@ static inline void gicv3_its_acpi_init(void)
>   }
>   #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 v6 2/5] ARM: ITS: Populate host_its_list from ACPI MADT Table

[Julien Grall]

On 10/10/17 13:52, 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 structure stores dt_node as NULL.
> 
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> Signed-off-by: Manish Jaggi <mjaggi@cavium.com>

Acked-by: Julien Grall <julien.grall@linaro.org>

Cheers,

-- 
Julien Grall

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

Re: [PATCH v6 4/5] ARM: Update Formula to compute MADT size using new callbacks in gic_hw_operations

[Julien Grall]

On 10/10/17 13:52, 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.
> 
> This patch updates the formula to compute extra MADT size, as per GICv2/3
> by calling gic_get_hwdom_extra_madt_size.
> 
> Signed-off-by: Manish Jaggi <mjaggi@cavium.com>

Acked-by: Julien Grall <julien.grall@linaro.org.

> ---
>   xen/arch/arm/domain_build.c |  7 +------
>   xen/arch/arm/gic-v2.c       |  6 ++++++
>   xen/arch/arm/gic-v3.c       | 19 +++++++++++++++++++
>   xen/arch/arm/gic.c          | 12 ++++++++++++
>   xen/include/asm-arm/gic.h   |  3 +++
>   5 files changed, 41 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index d6f9585..f17fcf1 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1808,12 +1808,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..0123ea4 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 unsigned long gicv2_get_hwdom_extra_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_extra_madt_size = gicv2_get_hwdom_extra_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.c b/xen/arch/arm/gic-v3.c
> index 475e0d3..0289d1a 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1407,6 +1407,19 @@ static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
>       return table_len;
>   }
>   
> +static unsigned long gicv3_get_hwdom_extra_madt_size(const struct domain *d)
> +{
> +    unsigned long size;
> +
> +    size = sizeof(struct acpi_madt_generic_redistributor)
> +           * d->arch.vgic.nr_regions;
> +
> +    size += sizeof(struct acpi_madt_generic_translator)
> +            * vgic_v3_its_count(d);
> +
> +    return size;
> +}
> +
>   static int __init
>   gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
>                           const unsigned long end)
> @@ -1598,6 +1611,11 @@ static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
>   {
>       return 0;
>   }
> +
> +static unsigned long gicv3_get_hwdom_extra_madt_size(const struct domain *d)
> +{
> +    return 0;
> +}
>   #endif
>   
>   /* Set up the GIC */
> @@ -1699,6 +1717,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_extra_madt_size = gicv3_get_hwdom_extra_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..3c7b6df 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -851,6 +851,18 @@ int gic_make_hwdom_madt(const struct domain *d, u32 offset)
>       return gic_hw_ops->make_hwdom_madt(d, offset);
>   }
>   
> +unsigned long gic_get_hwdom_madt_size(const struct domain *d)
> +{
> +    unsigned long 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_extra_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..0612706 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 */
> +    unsigned long (*get_hwdom_extra_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);
> +unsigned long 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);
>   
> 

-- 
Julien Grall

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

Re: [PATCH v6 5/5] ARM: ITS: Expose ITS in the MADT table

[Julien Grall]

On 10/10/17 13:52, mjaggi@caviumnetworks.com wrote:
> From: Manish Jaggi <mjaggi@cavium.com>
> 
> Add gicv3_its_make_hwdom_madt to update hwdom MADT ITS information.
> 
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Usually *-by are order from the oldest first to the earlier. This mean 
your Signed-off-by is first.

> Signed-off-by: Manish Jaggi <mjaggi@cavium.com>

Acked-by: Julien Grall <julien.grall@linaro.org>

Cheers,

> ---
>   xen/arch/arm/gic-v3-its.c        | 19 +++++++++++++++++++
>   xen/arch/arm/gic-v3.c            |  2 ++
>   xen/include/asm-arm/gic_v3_its.h |  8 ++++++++
>   3 files changed, 29 insertions(+)
> 
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index bd94308..e57ae05 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -1062,6 +1062,25 @@ void gicv3_its_acpi_init(void)
>       acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
>                             gicv3_its_acpi_probe, 0);
>   }
> +
> +unsigned long gicv3_its_make_hwdom_madt(const struct domain *d, void *base_ptr)
> +{
> +    unsigned int i;
> +    void *fw_its;
> +    struct acpi_madt_generic_translator *hwdom_its;
> +
> +    hwdom_its = base_ptr;
> +
> +    for ( i = 0; i < vgic_v3_its_count(d); i++ )
> +    {
> +        fw_its = acpi_table_get_entry_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
> +                                           i);
> +        memcpy(hwdom_its, fw_its, sizeof(struct acpi_madt_generic_translator));
> +        hwdom_its++;
> +    }
> +
> +    return sizeof(struct acpi_madt_generic_translator) * vgic_v3_its_count(d);
> +}
>   #endif
>   
>   /*
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 0289d1a..e9b9060 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1404,6 +1404,8 @@ static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
>           table_len += size;
>       }
>   
> +    table_len += gicv3_its_make_hwdom_madt(d, 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 73ee0ba..40dffdc 100644
> --- a/xen/include/asm-arm/gic_v3_its.h
> +++ b/xen/include/asm-arm/gic_v3_its.h
> @@ -138,6 +138,8 @@ void gicv3_its_dt_init(const struct dt_device_node *node);
>   
>   #ifdef CONFIG_ACPI
>   void gicv3_its_acpi_init(void);
> +unsigned long gicv3_its_make_hwdom_madt(const struct domain *d,
> +                                        void *base_ptr);
>   #endif
>   
>   /* Deny iomem access for its */
> @@ -208,6 +210,12 @@ static inline void gicv3_its_dt_init(const struct dt_device_node *node)
>   static inline void gicv3_its_acpi_init(void)
>   {
>   }
> +
> +static inline unsigned long gicv3_its_make_hwdom_madt(const struct domain *d,
> +                                                      void *base_ptr)
> +{
> +    return 0;
> +}
>   #endif
>   
>   static inline int gicv3_its_deny_access(const struct domain *d)
> 

-- 
Julien Grall

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

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

[Manish Jaggi]

Hi Julien,

On 10/10/2017 7:09 PM, Julien Grall wrote:
> Hi Manish,
>
> On 10/10/17 13:52, 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. Add function gicv3_its_deny_access.
>>
>> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>> Acked-by: Julien Grall <julien.grall@arm.com>
>
> Please state after "---" when you modified a patch and keep the tags 
> to at least check if the reviewer is happy with it.
>
> It is one of the reason I like the changelog in each patch. It helps 
> to know what changed in a specific one. It helps me to decide whether 
> I am happy with you keeping my tag and avoid to fully review yet 
> another time the patch.
>
> In that case, it is fine to keep it.
For this patch please ack it.
Changelog:
I have added
- a check on return value for gicv3_its_deny_access(d);
- used its_data->size in place of GICV3_ITS_SIZE
- remove extra space in printk

Thanks
manish
>
>> Signed-off-by: Manish Jaggi <mjaggi@cavium.com> > ---
>>   xen/arch/arm/gic-v3-its.c        | 22 ++++++++++++++++++++++
>>   xen/arch/arm/gic-v3.c            |  4 ++++
>>   xen/include/asm-arm/gic_v3_its.h |  9 +++++++++
>>   3 files changed, 35 insertions(+)
>>
>> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
>> index 3023ee5..bd94308 100644
>> --- a/xen/arch/arm/gic-v3-its.c
>> +++ b/xen/arch/arm/gic-v3-its.c
>> @@ -21,6 +21,7 @@
>>   #include <xen/acpi.h>
>>   #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,27 @@ 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(its_data->size);
>> +        rc = iomem_deny_access(d, mfn, mfn + nr);
>> +        if ( rc )
>> +        {
>> +            printk("iomem_deny_access failed for %lx:%lx \r\n", mfn, 
>> nr);
>> +            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 6f562f4..475e0d3 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -1308,6 +1308,10 @@ static int gicv3_iomem_deny_access(const 
>> struct domain *d)
>>       if ( rc )
>>           return rc;
>>   +    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 73d1fd1..73ee0ba 100644
>> --- a/xen/include/asm-arm/gic_v3_its.h
>> +++ b/xen/include/asm-arm/gic_v3_its.h
>> @@ -139,6 +139,10 @@ void gicv3_its_dt_init(const struct 
>> dt_device_node *node);
>>   #ifdef CONFIG_ACPI
>>   void gicv3_its_acpi_init(void);
>>   #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);
>> @@ -206,6 +210,11 @@ static inline void gicv3_its_acpi_init(void)
>>   }
>>   #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,
>


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

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

[Julien Grall]

On 10/10/17 14:53, Manish Jaggi wrote:
> Hi Julien,
> 
> On 10/10/2017 7:09 PM, Julien Grall wrote:
>> Hi Manish,
>>
>> On 10/10/17 13:52, 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. Add function gicv3_its_deny_access.
>>>
>>> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>>> Acked-by: Julien Grall <julien.grall@arm.com>
>>
>> Please state after "---" when you modified a patch and keep the tags 
>> to at least check if the reviewer is happy with it.
>>
>> It is one of the reason I like the changelog in each patch. It helps 
>> to know what changed in a specific one. It helps me to decide whether 
>> I am happy with you keeping my tag and avoid to fully review yet 
>> another time the patch.
>>
>> In that case, it is fine to keep it.
> For this patch please ack it.

I didn't ask to remove it :). Just pointed out that you should be more 
mindful when keeping an acked-by/reviewed-by.

> Changelog:
> I have added
> - a check on return value for gicv3_its_deny_access(d);
> - used its_data->size in place of GICV3_ITS_SIZE
> - remove extra space in printk

Thank you for the changelog!

Cheers,

-- 
Julien Grall

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

Re: [PATCH v6 0/5] ARM: ACPI: ITS: Add ITS Support for ACPI hardware domain

[Julien Grall]

Hi,

On 10/10/17 13:52, mjaggi@caviumnetworks.com wrote:
> From: Manish Jaggi <mjaggi@cavium.com>
> 
> 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.

Stefano, this series is fully acked and I have build test it with 
different configuration (GICv3, GICv2, with/without ACPI).

Can you commit it?

Cheers,

-- 
Julien Grall

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

Re: [PATCH v6 5/5] ARM: ITS: Expose ITS in the MADT table

[Stefano Stabellini]

On Tue, 10 Oct 2017, mjaggi@caviumnetworks.com wrote:
> From: Manish Jaggi <mjaggi@cavium.com>
> 
> Add gicv3_its_make_hwdom_madt to update hwdom MADT ITS information.
> 
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> Signed-off-by: Manish Jaggi <mjaggi@cavium.com>
> ---
>  xen/arch/arm/gic-v3-its.c        | 19 +++++++++++++++++++
>  xen/arch/arm/gic-v3.c            |  2 ++
>  xen/include/asm-arm/gic_v3_its.h |  8 ++++++++
>  3 files changed, 29 insertions(+)
> 
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index bd94308..e57ae05 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -1062,6 +1062,25 @@ void gicv3_its_acpi_init(void)
>      acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
>                            gicv3_its_acpi_probe, 0);
>  }
> +
> +unsigned long gicv3_its_make_hwdom_madt(const struct domain *d, void *base_ptr)
> +{
> +    unsigned int i;
> +    void *fw_its;
> +    struct acpi_madt_generic_translator *hwdom_its;
> +
> +    hwdom_its = base_ptr;
> +
> +    for ( i = 0; i < vgic_v3_its_count(d); i++ )
> +    {
> +        fw_its = acpi_table_get_entry_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
> +                                           i);
> +        memcpy(hwdom_its, fw_its, sizeof(struct acpi_madt_generic_translator));

I think we are supposed to use ACPI_MEMCPY for this kind of operations.
If that's OK for you, I'll fix on commit.


> +        hwdom_its++;
> +    }
> +
> +    return sizeof(struct acpi_madt_generic_translator) * vgic_v3_its_count(d);
> +}
>  #endif
>  
>  /*
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 0289d1a..e9b9060 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1404,6 +1404,8 @@ static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
>          table_len += size;
>      }
>  
> +    table_len += gicv3_its_make_hwdom_madt(d, 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 73ee0ba..40dffdc 100644
> --- a/xen/include/asm-arm/gic_v3_its.h
> +++ b/xen/include/asm-arm/gic_v3_its.h
> @@ -138,6 +138,8 @@ void gicv3_its_dt_init(const struct dt_device_node *node);
>  
>  #ifdef CONFIG_ACPI
>  void gicv3_its_acpi_init(void);
> +unsigned long gicv3_its_make_hwdom_madt(const struct domain *d,
> +                                        void *base_ptr);
>  #endif
>  
>  /* Deny iomem access for its */
> @@ -208,6 +210,12 @@ static inline void gicv3_its_dt_init(const struct dt_device_node *node)
>  static inline void gicv3_its_acpi_init(void)
>  {
>  }
> +
> +static inline unsigned long gicv3_its_make_hwdom_madt(const struct domain *d,
> +                                                      void *base_ptr)
> +{
> +    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

Re: [PATCH v6 5/5] ARM: ITS: Expose ITS in the MADT table

[Julien Grall]

Hi,

On 10/10/2017 20:15, Stefano Stabellini wrote:
> On Tue, 10 Oct 2017, mjaggi@caviumnetworks.com wrote:
>> From: Manish Jaggi <mjaggi@cavium.com>
>>
>> Add gicv3_its_make_hwdom_madt to update hwdom MADT ITS information.
>>
>> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>> Signed-off-by: Manish Jaggi <mjaggi@cavium.com>
>> ---
>>  xen/arch/arm/gic-v3-its.c        | 19 +++++++++++++++++++
>>  xen/arch/arm/gic-v3.c            |  2 ++
>>  xen/include/asm-arm/gic_v3_its.h |  8 ++++++++
>>  3 files changed, 29 insertions(+)
>>
>> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
>> index bd94308..e57ae05 100644
>> --- a/xen/arch/arm/gic-v3-its.c
>> +++ b/xen/arch/arm/gic-v3-its.c
>> @@ -1062,6 +1062,25 @@ void gicv3_its_acpi_init(void)
>>      acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
>>                            gicv3_its_acpi_probe, 0);
>>  }
>> +
>> +unsigned long gicv3_its_make_hwdom_madt(const struct domain *d, void *base_ptr)
>> +{
>> +    unsigned int i;
>> +    void *fw_its;
>> +    struct acpi_madt_generic_translator *hwdom_its;
>> +
>> +    hwdom_its = base_ptr;
>> +
>> +    for ( i = 0; i < vgic_v3_its_count(d); i++ )
>> +    {
>> +        fw_its = acpi_table_get_entry_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
>> +                                           i);
>> +        memcpy(hwdom_its, fw_its, sizeof(struct acpi_madt_generic_translator));
>
> I think we are supposed to use ACPI_MEMCPY for this kind of operations.
> If that's OK for you, I'll fix on commit.

I don't think we should use ACPI_MEMCPY. The macro is here because 
acpica (our drivers/acpi) is meant to be OS-agnostic. So you need a way 
to tell how your OS copies memory.

I had a look on the usage of ACPI_MEMCPY, it seems that only the 
arch/arm and drivers/acpi is using it. This seem to confirm that 
probably we used it by mistake in the Arm code.

Cheers,

-- 
Julien Grall

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

Re: [PATCH v6 5/5] ARM: ITS: Expose ITS in the MADT table

[Stefano Stabellini]

CC'ing Jan and Andrew, just in case they disagree on this.

On Tue, 10 Oct 2017, Julien Grall wrote:
> > > +unsigned long gicv3_its_make_hwdom_madt(const struct domain *d, void
> > > *base_ptr)
> > > +{
> > > +    unsigned int i;
> > > +    void *fw_its;
> > > +    struct acpi_madt_generic_translator *hwdom_its;
> > > +
> > > +    hwdom_its = base_ptr;
> > > +
> > > +    for ( i = 0; i < vgic_v3_its_count(d); i++ )
> > > +    {
> > > +        fw_its =
> > > acpi_table_get_entry_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
> > > +                                           i);
> > > +        memcpy(hwdom_its, fw_its, sizeof(struct
> > > acpi_madt_generic_translator));
> > 
> > I think we are supposed to use ACPI_MEMCPY for this kind of operations.
> > If that's OK for you, I'll fix on commit.
> 
> I don't think we should use ACPI_MEMCPY. The macro is here because acpica (our
> drivers/acpi) is meant to be OS-agnostic. So you need a way to tell how your
> OS copies memory.
> 
> I had a look on the usage of ACPI_MEMCPY, it seems that only the arch/arm and
> drivers/acpi is using it. This seem to confirm that probably we used it by
> mistake in the Arm code.

It looks like you are right, ACPI_MEMCPY does not make sense in Xen
code outside of drivers/acpi.

Consistency is important, so if we are not going to use ACPI_MEMCPY, then
I'll write a patch (or I'll take a patch if you volunteer in writing it)
to s/ACPI_MEMCPY/memcpy/g everywhere under arch/arm. The worst we could
end up with is a mixed environment.

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

Re: [PATCH v6 5/5] ARM: ITS: Expose ITS in the MADT table

[Julien Grall]

Hi,

On 10/10/2017 21:07, Stefano Stabellini wrote:
> CC'ing Jan and Andrew, just in case they disagree on this.
>
> On Tue, 10 Oct 2017, Julien Grall wrote:
>>>> +unsigned long gicv3_its_make_hwdom_madt(const struct domain *d, void
>>>> *base_ptr)
>>>> +{
>>>> +    unsigned int i;
>>>> +    void *fw_its;
>>>> +    struct acpi_madt_generic_translator *hwdom_its;
>>>> +
>>>> +    hwdom_its = base_ptr;
>>>> +
>>>> +    for ( i = 0; i < vgic_v3_its_count(d); i++ )
>>>> +    {
>>>> +        fw_its =
>>>> acpi_table_get_entry_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
>>>> +                                           i);
>>>> +        memcpy(hwdom_its, fw_its, sizeof(struct
>>>> acpi_madt_generic_translator));
>>>
>>> I think we are supposed to use ACPI_MEMCPY for this kind of operations.
>>> If that's OK for you, I'll fix on commit.
>>
>> I don't think we should use ACPI_MEMCPY. The macro is here because acpica (our
>> drivers/acpi) is meant to be OS-agnostic. So you need a way to tell how your
>> OS copies memory.
>>
>> I had a look on the usage of ACPI_MEMCPY, it seems that only the arch/arm and
>> drivers/acpi is using it. This seem to confirm that probably we used it by
>> mistake in the Arm code.
>
> It looks like you are right, ACPI_MEMCPY does not make sense in Xen
> code outside of drivers/acpi.
>
> Consistency is important, so if we are not going to use ACPI_MEMCPY, then
> I'll write a patch (or I'll take a patch if you volunteer in writing it)
> to s/ACPI_MEMCPY/memcpy/g everywhere under arch/arm. The worst we could
> end up with is a mixed environment.

Feel free to write a patch, but I don't think you should block this 
series for that.

Cheers,

-- 
Julien Grall

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