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

[PATCH v5 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 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            | 26 ++++++++++++
 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, 157 insertions(+), 15 deletions(-)

-- 
2.7.4


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

[PATCH v5 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 v5 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..e1be33c 100644
--- a/xen/include/asm-arm/gic_v3_its.h
+++ b/xen/include/asm-arm/gic_v3_its.h
@@ -20,6 +20,7 @@
 #ifndef __ASM_ARM_ITS_H__
 #define __ASM_ARM_ITS_H__
 
+#define GICV3_ITS_SIZE                  SZ_128K
 #define GITS_CTLR                       0x000
 #define GITS_IIDR                       0x004
 #define GITS_TYPER                      0x008
@@ -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 v5 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            |  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 3023ee5..7746ae8 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(GICV3_ITS_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 e1be33c..31fca66 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 v5 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 b3d605d..447998d 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1406,6 +1406,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  += 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 +1610,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 */
@@ -1698,6 +1716,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 v5 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 7746ae8..3fa592c 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 long i = 0;
+    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 447998d..90385bf 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1403,6 +1403,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 31fca66..539aa30 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 v5 4/5] ARM: Update Formula to compute MADT size using new callbacks in gic_hw_operations

[Andre Przywara]

Hi Manish,

On 10/10/17 07:16, 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>

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

Thanks!
Andre

> ---
>  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 b3d605d..447998d 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1406,6 +1406,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  += 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 +1610,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 */
> @@ -1698,6 +1716,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);
>  
> 

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

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

[Julien Grall]

Hi Manish,

On 10/10/17 07:16, mjaggi@caviumnetworks.com wrote:
> diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h
> index 1fac1c7..e1be33c 100644
> --- a/xen/include/asm-arm/gic_v3_its.h
> +++ b/xen/include/asm-arm/gic_v3_its.h
> @@ -20,6 +20,7 @@
>   #ifndef __ASM_ARM_ITS_H__
>   #define __ASM_ARM_ITS_H__
>   
> +#define GICV3_ITS_SIZE                  SZ_128K

You still didn't address my comment here... This *must* be moved close
to ITS_DOORBELL_OFFSET as the current place is just random.

>   #define GITS_CTLR                       0x000
>   #define GITS_IIDR                       0x004
>   #define GITS_TYPER                      0x008

Cheers,

-- 
Julien Grall

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

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

[Julien Grall]

Hi Manish,

I just spotted an issue in this patch.

On 10/10/17 07:16, 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>
> 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 3023ee5..7746ae8 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(GICV3_ITS_SIZE);

While you fix the bug (see below), please use its_data->size here.

> +        rc = iomem_deny_access(d, mfn, mfn + nr);
> +        if ( rc )
> +        {
> +            printk( "iomem_deny_access failed for %lx:%lx \r\n", mfn, nr);

And here the spurious space between ( and ".

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

you return rc here in case of an error, however you don't set it before. 
So this will always return 0 and Xen will continue to boot as nothing 
happen.

The best way to fix it would be:

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 e1be33c..31fca66 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 v5 4/5] ARM: Update Formula to compute MADT size using new callbacks in gic_hw_operations

[Julien Grall]

Hi Manish,

On 10/10/17 07:16, 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

Missing full stop.

> 
> 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 b3d605d..447998d 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1406,6 +1406,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;

Here you align the * with struct. But below, you align with sizeof. 
Please stay consistent and always align with sizeof.

> +
> +    size  += vgic_v3_its_count(d)
> +                    * sizeof(struct acpi_madt_generic_translator);

Same here.

> +
> +    return size;
> +}
> +
>   static int __init
>   gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
>                           const unsigned long end)
> @@ -1597,6 +1610,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 */
> @@ -1698,6 +1716,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);
>   
> 

Cheers,

-- 
Julien Grall

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

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

[Julien Grall]

Hi Manish,

On 10/10/17 07:16, 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 7746ae8..3fa592c 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 long i = 0;

vgic_v3_its_count(..) return an unsigned int. So no need to use unsigned 
long here.

With that fixed:

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 v5 4/5] ARM: Update Formula to compute MADT size using new callbacks in gic_hw_operations

[Manish Jaggi]

On 10/10/2017 3:44 PM, Julien Grall wrote:
> Hi Manish,
>
> On 10/10/17 07:16, 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
>
> Missing full stop.
oh i missed it.
>
>>
>> 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 b3d605d..447998d 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -1406,6 +1406,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;
>
> Here you align the * with struct. But below, you align with sizeof. 
> Please stay consistent and always align with sizeof.
>
>> +
>> +    size  += vgic_v3_its_count(d)
>> +                    * sizeof(struct acpi_madt_generic_translator);
>
> Same here.
>
Could you please help with the specific section on coding style 
guidelines on xen for indentation when line over 80 chars which I am not 
following for this case.
>> +
>> +    return size;
>> +}
>> +
>>   static int __init
>>   gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
>>                           const unsigned long end)
>> @@ -1597,6 +1610,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 */
>> @@ -1698,6 +1716,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);
>>
>
> Cheers,
>


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

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

[Julien Grall]

Hi,

On 10/10/17 11:33, Manish Jaggi wrote:
> 
> 
> On 10/10/2017 3:44 PM, Julien Grall wrote:
>> Hi Manish,
>>
>> On 10/10/17 07:16, 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
>>
>> Missing full stop.
> oh i missed it.
>>
>>>
>>> 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 b3d605d..447998d 100644
>>> --- a/xen/arch/arm/gic-v3.c
>>> +++ b/xen/arch/arm/gic-v3.c
>>> @@ -1406,6 +1406,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;
>>
>> Here you align the * with struct. But below, you align with sizeof. 
>> Please stay consistent and always align with sizeof.
>>
>>> +
>>> +    size  += vgic_v3_its_count(d)
>>> +                    * sizeof(struct acpi_madt_generic_translator);
>>
>> Same here.
>>
> Could you please help with the specific section on coding style 
> guidelines on xen for indentation when line over 80 chars which I am not 
> following for this case.

The best guideline is the existing code around in the file, in doubt ask.

The code you copied was aligned with sizeof (see in domain_build.c) and 
now it does not have the same alignment.

Furthermore, you used double tab and not one tab (Xen is using 4 spaces).

Cheers

-- 
Julien Grall

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