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

[PATCH v3 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 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: Introduce get_hwdom_madt_size 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        | 97 ++++++++++++++++++++++++++++++++++++----
 xen/arch/arm/gic-v3.c            | 24 ++++++++++
 xen/arch/arm/gic.c               | 11 +++++
 xen/include/asm-arm/gic.h        |  3 ++
 xen/include/asm-arm/gic_v3_its.h | 26 +++++++++++
 7 files changed, 159 insertions(+), 15 deletions(-)

-- 
2.7.4


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

[PATCH v3 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>
---
 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..61a6452 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 v3 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.

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

diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index 61a6452..536b48d 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -25,6 +25,7 @@
 #include <xen/rbtree.h>
 #include <xen/sched.h>
 #include <xen/sizes.h>
+#include <xen/acpi.h>
 #include <asm/gic.h>
 #include <asm/gic_v3_defs.h>
 #include <asm/gic_v3_its.h>
@@ -32,6 +33,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
@@ -1018,6 +1020,30 @@ void gicv3_its_dt_init(const struct dt_device_node *node)
     }
 }
 
+#ifdef CONFIG_ACPI
+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,
+                        ACPI_GICV3_ITS_MEM_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..993819a 100644
--- a/xen/include/asm-arm/gic_v3_its.h
+++ b/xen/include/asm-arm/gic_v3_its.h
@@ -135,6 +135,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 +199,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 v3 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.

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

diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index 536b48d..0ab1466 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>
@@ -906,6 +907,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(ACPI_GICV3_ITS_MEM_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..b3d605d 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1308,6 +1308,9 @@ static int gicv3_iomem_deny_access(const struct domain *d)
     if ( rc )
         return rc;
 
+    if ( gicv3_its_deny_access(d) )
+        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 993819a..9cf18da 100644
--- a/xen/include/asm-arm/gic_v3_its.h
+++ b/xen/include/asm-arm/gic_v3_its.h
@@ -138,6 +138,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);
@@ -205,6 +209,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 v3 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.c       | 18 ++++++++++++++++++
 xen/arch/arm/gic.c          | 11 +++++++++++
 xen/include/asm-arm/gic.h   |  3 +++
 5 files changed, 39 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..737c50a 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_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.c b/xen/arch/arm/gic-v3.c
index b3d605d..3eb67f2 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1406,6 +1406,18 @@ static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
     return table_len;
 }
 
+static unsigned long gicv3_get_hwdom_madt_size(const struct domain *d)
+{
+    unsigned long size;
+    size  = sizeof(struct acpi_madt_generic_redistributor)
+                    * d->arch.vgic.nr_regions;
+
+    size  += vgic_v3_its_count(d)
+                    * sizeof(struct acpi_madt_generic_translator);
+
+    return size;
+}
+
 static int __init
 gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
                         const unsigned long end)
@@ -1597,6 +1609,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 */
@@ -1698,6 +1715,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..9ffd33a 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);
 }
 
+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_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..3acdd6d 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_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 v3 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.

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

diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index 0ab1466..bf84db8 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -1064,6 +1064,29 @@ 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, u8 *base_ptr,
+                                        unsigned long offset)
+{
+    unsigned long i;
+    struct acpi_madt_generic_translator *fw_its;
+    struct acpi_madt_generic_translator *hwdom_its;
+
+    hwdom_its = (struct acpi_madt_generic_translator *)(base_ptr
+                   + offset);
+
+    for ( i = 0; i < vgic_v3_its_count(d); i++ )
+    {
+        fw_its = (struct acpi_madt_generic_translator *)
+                    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 (offset + 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 3eb67f2..0392795 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1403,6 +1403,7 @@ 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 9cf18da..ae8a494 100644
--- a/xen/include/asm-arm/gic_v3_its.h
+++ b/xen/include/asm-arm/gic_v3_its.h
@@ -137,6 +137,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, u8 *base_ptr,
+                                        unsigned long offset);
 #endif
 
 /* Deny iomem access for its */
@@ -207,6 +209,12 @@ static inline void gicv3_its_dt_init(const struct dt_device_node *node)
 static inline void gicv3_its_acpi_init(void)
 {
 }
+
+unsigned long gicv3_its_make_hwdom_madt(struct domain *d, u8 *base_ptr,
+                                        unsigned long offset)
+{
+    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 v3 1/5] ARM: ITS: Introduce common function add_to_host_its_list

[Andre Przywara]

Hi,

On 05/09/17 18:14, 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_probe.
> 
> Signed-off-by: Manish Jaggi <mjaggi@cavium.com>

Makes sense.

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

> ---
>  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..61a6452 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);
>      }
>  }
>  
> 

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

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

[Andre Przywara]

Hi,

On 05/09/17 18:14, 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.
> 
> Signed-off-by: Manish Jaggi <mjaggi@cavium.com>
> ---
>  xen/arch/arm/gic-v3-its.c        | 26 ++++++++++++++++++++++++++
>  xen/arch/arm/gic-v3.c            |  2 ++
>  xen/include/asm-arm/gic_v3_its.h |  9 +++++++++
>  3 files changed, 37 insertions(+)
> 
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 61a6452..536b48d 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -25,6 +25,7 @@
>  #include <xen/rbtree.h>
>  #include <xen/sched.h>
>  #include <xen/sizes.h>
> +#include <xen/acpi.h>
>  #include <asm/gic.h>
>  #include <asm/gic_v3_defs.h>
>  #include <asm/gic_v3_its.h>
> @@ -32,6 +33,7 @@
>  #include <asm/page.h>
>  
>  #define ITS_CMD_QUEUE_SZ                SZ_1M
> +#define ACPI_GICV3_ITS_MEM_SIZE         SZ_64K

Although this is used for ACPI only, this size is really the architected
size for the ITS register frame and thus should be named like this,
possibly GUEST_GICV3_ITS_SIZE or so (in xen/include/public/arch-arm.h).
Which actually makes me wonder why we would need to store this size in
the data structure in the first place ...

>  /*
>   * No lock here, as this list gets only populated upon boot while scanning
> @@ -1018,6 +1020,30 @@ void gicv3_its_dt_init(const struct dt_device_node *node)
>      }
>  }
>  
> +#ifdef CONFIG_ACPI
> +int gicv3_its_acpi_probe(struct acpi_subtable_header *header,
> +                        const unsigned long end)

                          w/s?
> +{
> +    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,
> +                        ACPI_GICV3_ITS_MEM_SIZE, NULL);

                          w/s?

> +
> +    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);

                            w/s?

Cheers,
Andre.

> +}
> +#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..993819a 100644
> --- a/xen/include/asm-arm/gic_v3_its.h
> +++ b/xen/include/asm-arm/gic_v3_its.h
> @@ -135,6 +135,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 +199,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;
> 

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

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

[Andre Przywara]

Hi,

On 05/09/17 18:14, 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.
> 
> Signed-off-by: Manish Jaggi <mjaggi@cavium.com>
> ---
>  xen/arch/arm/gic-v3-its.c        | 22 ++++++++++++++++++++++
>  xen/arch/arm/gic-v3.c            |  3 +++
>  xen/include/asm-arm/gic_v3_its.h |  9 +++++++++
>  3 files changed, 34 insertions(+)
> 
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 536b48d..0ab1466 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>
> @@ -906,6 +907,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(ACPI_GICV3_ITS_MEM_SIZE);

Shouldn't this not only cover the ITS register frame, but also the
following 64K page containing the doorbell address? Otherwise we leave
the doorbell address open, which seems to be asking for trouble ...

Cheers,
Andre.

> +        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..b3d605d 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1308,6 +1308,9 @@ static int gicv3_iomem_deny_access(const struct domain *d)
>      if ( rc )
>          return rc;
>  
> +    if ( gicv3_its_deny_access(d) )
> +        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 993819a..9cf18da 100644
> --- a/xen/include/asm-arm/gic_v3_its.h
> +++ b/xen/include/asm-arm/gic_v3_its.h
> @@ -138,6 +138,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);
> @@ -205,6 +209,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;
> 

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

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

[Andre Przywara]

Hi,

On 05/09/17 18:14, 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.c       | 18 ++++++++++++++++++
>  xen/arch/arm/gic.c          | 11 +++++++++++
>  xen/include/asm-arm/gic.h   |  3 +++
>  5 files changed, 39 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..737c50a 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_madt_size(const struct domain *d)
> +{
> +    return 0;
> +}

Nothing too critical, but this looks a bit confusing, as the size of the
GIC part of the MADT isn't 0 even for GICv2. So either you rename it to
something containing "additional" or the like or you do what it says on
the tin and return the per-VCPU size and the size for the distributor
here (at the cost of copying this to the GICv3 code).

Cheers,
Andre.

> +
>  #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.c b/xen/arch/arm/gic-v3.c
> index b3d605d..3eb67f2 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1406,6 +1406,18 @@ static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
>      return table_len;
>  }
>  
> +static unsigned long gicv3_get_hwdom_madt_size(const struct domain *d)
> +{
> +    unsigned long size;
> +    size  = sizeof(struct acpi_madt_generic_redistributor)
> +                    * d->arch.vgic.nr_regions;
> +
> +    size  += vgic_v3_its_count(d)
> +                    * sizeof(struct acpi_madt_generic_translator);
> +
> +    return size;
> +}
> +
>  static int __init
>  gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
>                          const unsigned long end)
> @@ -1597,6 +1609,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 */
> @@ -1698,6 +1715,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..9ffd33a 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);
>  }
>  
> +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_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..3acdd6d 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_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);
>  
> 

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

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

[Andre Przywara]

Hi,

On 05/09/17 18:15, mjaggi@caviumnetworks.com wrote:
> From: Manish Jaggi <mjaggi@cavium.com>
> 
> Add 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        | 23 +++++++++++++++++++++++
>  xen/arch/arm/gic-v3.c            |  1 +
>  xen/include/asm-arm/gic_v3_its.h |  8 ++++++++
>  3 files changed, 32 insertions(+)
> 
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 0ab1466..bf84db8 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -1064,6 +1064,29 @@ 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, u8 *base_ptr,
> +                                        unsigned long offset)

What about we drop offset here and add it at the caller, then return
just the size of the ITS MADT size? Also base_ptr could be a void* then.

> +{
> +    unsigned long i;
> +    struct acpi_madt_generic_translator *fw_its;

If you make this either a "void *" or a "struct acpi_subtable_header *"
then you can save the rather ugly cast in the assignment below.

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

If you drop offset as mentioned above and make base_ptr a void*, you can
save the cast.

> +
> +    for ( i = 0; i < vgic_v3_its_count(d); i++ )
> +    {
> +        fw_its = (struct acpi_madt_generic_translator *)
> +                    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 (offset + 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 3eb67f2..0392795 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1403,6 +1403,7 @@ 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);

... and here you could mimic the other calls then:
	table_len += gicv3_its_make_hwdom_madt(d, base_ptr + table_len);

(or directly return).

Cheers,
Andre.


>      return table_len;
>  }
>  
> diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h
> index 9cf18da..ae8a494 100644
> --- a/xen/include/asm-arm/gic_v3_its.h
> +++ b/xen/include/asm-arm/gic_v3_its.h
> @@ -137,6 +137,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, u8 *base_ptr,
> +                                        unsigned long offset);
>  #endif
>  
>  /* Deny iomem access for its */
> @@ -207,6 +209,12 @@ static inline void gicv3_its_dt_init(const struct dt_device_node *node)
>  static inline void gicv3_its_acpi_init(void)
>  {
>  }
> +
> +unsigned long gicv3_its_make_hwdom_madt(struct domain *d, u8 *base_ptr,
> +                                        unsigned long offset)
> +{
> +    return 0;
> +}
>  #endif
>  
>  static inline int gicv3_its_deny_access(const struct domain *d)
> 

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

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

[Julien Grall]

Hi Manish,

On 05/09/17 18:14, 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_probe.
> 
> Signed-off-by: Manish Jaggi <mjaggi@cavium.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..61a6452 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)

The indentation looks wrong here.

With that fixed:

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

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

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

[Julien Grall]

On 07/09/17 17:56, Andre Przywara wrote:
> Hi,

Hi Andre,

> On 05/09/17 18:14, 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.
>>
>> Signed-off-by: Manish Jaggi <mjaggi@cavium.com>
>> ---
>>   xen/arch/arm/gic-v3-its.c        | 26 ++++++++++++++++++++++++++
>>   xen/arch/arm/gic-v3.c            |  2 ++
>>   xen/include/asm-arm/gic_v3_its.h |  9 +++++++++
>>   3 files changed, 37 insertions(+)
>>
>> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
>> index 61a6452..536b48d 100644
>> --- a/xen/arch/arm/gic-v3-its.c
>> +++ b/xen/arch/arm/gic-v3-its.c
>> @@ -25,6 +25,7 @@
>>   #include <xen/rbtree.h>
>>   #include <xen/sched.h>
>>   #include <xen/sizes.h>
>> +#include <xen/acpi.h>
>>   #include <asm/gic.h>
>>   #include <asm/gic_v3_defs.h>
>>   #include <asm/gic_v3_its.h>
>> @@ -32,6 +33,7 @@
>>   #include <asm/page.h>
>>   
>>   #define ITS_CMD_QUEUE_SZ                SZ_1M
>> +#define ACPI_GICV3_ITS_MEM_SIZE         SZ_64K
> 
> Although this is used for ACPI only, this size is really the architected
> size for the ITS register frame and thus should be named like this,
> possibly GUEST_GICV3_ITS_SIZE or so (in xen/include/public/arch-arm.h).
> Which actually makes me wonder why we would need to store this size in
> the data structure in the first place ...

I guess it because the size is also present on the DT :). But yes, we 
should not need as the size of the ITS MMIO is fixed.

But using GUEST_GICV3_ITS_SIZE would be a bit odd as we are talking 
about the host its here. So a better name would be GICV3_ITS_SIZE.

And no inclusion in xen/include/public/arch-arm.h please. gic_v3_its.h 
is a more suitable place for that.

Cheers,

-- 
Julien Grall

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

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

[Julien Grall]

Hi Manish,

On 05/09/17 18:14, 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.
> 
> Signed-off-by: Manish Jaggi <mjaggi@cavium.com>
> ---
>   xen/arch/arm/gic-v3-its.c        | 26 ++++++++++++++++++++++++++
>   xen/arch/arm/gic-v3.c            |  2 ++
>   xen/include/asm-arm/gic_v3_its.h |  9 +++++++++
>   3 files changed, 37 insertions(+)
> 
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 61a6452..536b48d 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -25,6 +25,7 @@
>   #include <xen/rbtree.h>
>   #include <xen/sched.h>
>   #include <xen/sizes.h>
> +#include <xen/acpi.h>

Headers are ordered alphabetically, so please move this to the right place.

>   #include <asm/gic.h>
>   #include <asm/gic_v3_defs.h>
>   #include <asm/gic_v3_its.h>
> @@ -32,6 +33,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
> @@ -1018,6 +1020,30 @@ void gicv3_its_dt_init(const struct dt_device_node *node)
>       }
>   }
>   
> +#ifdef CONFIG_ACPI
> +int gicv3_its_acpi_probe(struct acpi_subtable_header *header,

This should be static int...

> +                        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,
> +                        ACPI_GICV3_ITS_MEM_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..993819a 100644
> --- a/xen/include/asm-arm/gic_v3_its.h
> +++ b/xen/include/asm-arm/gic_v3_its.h
> @@ -135,6 +135,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 +199,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;
> 

Cheers,

-- 
Julien Grall

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

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

[Julien Grall]

On 07/09/17 17:57, Andre Przywara wrote:
> Hi,

Hi,

> On 05/09/17 18:14, 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.
>>
>> Signed-off-by: Manish Jaggi <mjaggi@cavium.com>
>> ---
>>   xen/arch/arm/gic-v3-its.c        | 22 ++++++++++++++++++++++
>>   xen/arch/arm/gic-v3.c            |  3 +++
>>   xen/include/asm-arm/gic_v3_its.h |  9 +++++++++
>>   3 files changed, 34 insertions(+)
>>
>> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
>> index 536b48d..0ab1466 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>
>> @@ -906,6 +907,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(ACPI_GICV3_ITS_MEM_SIZE);
> 
> Shouldn't this not only cover the ITS register frame, but also the
> following 64K page containing the doorbell address? Otherwise we leave
> the doorbell address open, which seems to be asking for trouble ...

I think you are right. We don't want to allow the hardware domain to map 
the doorbell itself. This should only be done by Xen.

Cheers,

-- 
Julien Grall

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

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

[Julien Grall]

Hi,

On 05/09/17 18:14, 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.c       | 18 ++++++++++++++++++
>   xen/arch/arm/gic.c          | 11 +++++++++++
>   xen/include/asm-arm/gic.h   |  3 +++
>   5 files changed, 39 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..737c50a 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_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.c b/xen/arch/arm/gic-v3.c
> index b3d605d..3eb67f2 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1406,6 +1406,18 @@ static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
>       return table_len;
>   }
>   
> +static unsigned long gicv3_get_hwdom_madt_size(const struct domain *d)
> +{
> +    unsigned long size;

As already requested on v2, you need to add a newline here.

> +    size  = sizeof(struct acpi_madt_generic_redistributor)
> +                    * d->arch.vgic.nr_regions;
> +
> +    size  += vgic_v3_its_count(d)
> +                    * sizeof(struct acpi_madt_generic_translator);
> +
> +    return size;
> +}
> +
>   static int __init
>   gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
>                           const unsigned long end)
> @@ -1597,6 +1609,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 */
> @@ -1698,6 +1715,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..9ffd33a 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.cMissing newline here.
> @@ -851,6 +851,17 @@ 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;

Same here.

> +    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..3acdd6d 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_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);
>   
> 

Cheers,

-- 
Julien Grall

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

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

[Manish Jaggi]

On 9/7/2017 10:27 PM, Andre Przywara wrote:
> Hi,
>
> On 05/09/17 18:14, 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.
>>
>> Signed-off-by: Manish Jaggi <mjaggi@cavium.com>
>> ---
>>   xen/arch/arm/gic-v3-its.c        | 22 ++++++++++++++++++++++
>>   xen/arch/arm/gic-v3.c            |  3 +++
>>   xen/include/asm-arm/gic_v3_its.h |  9 +++++++++
>>   3 files changed, 34 insertions(+)
>>
>> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
>> index 536b48d..0ab1466 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>
>> @@ -906,6 +907,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(ACPI_GICV3_ITS_MEM_SIZE);
> Shouldn't this not only cover the ITS register frame, but also the
> following 64K page containing the doorbell address? Otherwise we leave
> the doorbell address open, which seems to be asking for trouble ...
>
> Cheers,
> Andre.
ok,  I will fix in patch 2 the size as 128K, same a linux.
If no other change required in this patch can you please ack it.
>
>> +        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..b3d605d 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -1308,6 +1308,9 @@ static int gicv3_iomem_deny_access(const struct domain *d)
>>       if ( rc )
>>           return rc;
>>   
>> +    if ( gicv3_its_deny_access(d) )
>> +        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 993819a..9cf18da 100644
>> --- a/xen/include/asm-arm/gic_v3_its.h
>> +++ b/xen/include/asm-arm/gic_v3_its.h
>> @@ -138,6 +138,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);
>> @@ -205,6 +209,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;
>>


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