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

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

[Manish Jaggi]

This patch series adds the support of ITS for ACPI hardware domain.
It is tested on staging branch with has ITS v12 patchset by Andre.

I have tried to incorporate the review comments on the RFC v1/v2 patch.
The single patch in RFC is now split into 4 patches. 

Patch1: ARM: ITS: Add translation_id to host_its
 Adds translation_id in host_its data structure, which is populated from 
 translation_id read from firmwar MADT. This value is then programmed into
 local MADT created for hardware domain in patch 4.

Patch2: ARM: ITS: ACPI: Introduce gicv3_its_acpi_init
 Introduces function for its_acpi_init, which calls add_to_host_its_list
 which is a common function also called from _dt variant.

Patch3: ARM: ITS: Deny hardware domain access to its
 Extends the gicv3_iomem_deny to include its regions as well

Patch4: ARM: ACPI: Add ITS to hardware domain MADT
 This patch adds ITS information in hardware domain's MADT table. 
 Also this patch interoduces .get_hwdom_madt_size in gic_hw_operations,
 to return the complete size of MADT table for hardware domain.


Manish Jaggi (4):
  ARM: ITS: Add translation_id to host_its
  ARM: ITS: ACPI: Introduce gicv3_its_acpi_init
  ARM: ITS: Deny hardware domain access to its
  ARM: ACPI: Add ITS to hardware domain MADT

 xen/arch/arm/domain_build.c      |   7 +--
 xen/arch/arm/gic-v2.c            |   6 +++
 xen/arch/arm/gic-v3-its.c        | 102 +++++++++++++++++++++++++++++++++++----
 xen/arch/arm/gic-v3.c            |  31 ++++++++++++
 xen/arch/arm/gic.c               |  11 +++++
 xen/include/asm-arm/gic.h        |   3 ++
 xen/include/asm-arm/gic_v3_its.h |  36 ++++++++++++++
 7 files changed, 180 insertions(+), 16 deletions(-)

-- 
2.7.4


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

[PATCH 1/4] ARM: ITS: Add translation_id to host_its

[Manish Jaggi]

This patch adds a translation_id to host_its data structure.
Value stored in this id should be copied over to hardware domains
MADT table.

Signed-off-by: Manish Jaggi <mjaggi@cavium.com>
---
 xen/include/asm-arm/gic_v3_its.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h
index 1fac1c7..96b910b 100644
--- a/xen/include/asm-arm/gic_v3_its.h
+++ b/xen/include/asm-arm/gic_v3_its.h
@@ -118,6 +118,8 @@ struct host_its {
     const struct dt_device_node *dt_node;
     paddr_t addr;
     paddr_t size;
+    /* A unique value to identify each ITS */
+    u32 translation_id;
     void __iomem *its_base;
     unsigned int devid_bits;
     unsigned int evid_bits;
-- 
2.7.4


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

[PATCH 2/4] ARM: ITS: ACPI: Introduce gicv3_its_acpi_init

[Manish Jaggi]

This patch adds gicv3_its_acpi_init. To avoid duplicate code for
initializing and adding to host_its_list a common function
add_to_host_its_list is added which is called by both _dt_init and _acpi_init.

Signed-off-by: Manish Jaggi <mjaggi@cavium.com>
---
 xen/arch/arm/gic-v3-its.c        | 49 ++++++++++++++++++++++++++++++++--------
 xen/arch/arm/gic-v3.c            |  6 +++++
 xen/include/asm-arm/gic_v3_its.h | 14 ++++++++++++
 3 files changed, 59 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index 2d36030..e11f29a 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -33,6 +33,7 @@
 
 #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
  * firmware tables for all host ITSes, and only gets iterated afterwards.
@@ -976,11 +977,35 @@ int gicv3_its_make_hwdom_dt_nodes(const struct domain *d,
     return res;
 }
 
+/* Common function for addind to host_its_list
+*/
+static int add_to_host_its_list(u64 addr, u64 size,
+                      u32 translation_id, const void *node)
+{
+    struct host_its *its_data;
+    its_data = xzalloc(struct host_its);
+
+    if ( !its_data )
+        return -1;
+
+    if ( node )
+        its_data->dt_node = node;
+
+    its_data->addr = addr;
+    its_data->size = size;
+    its_data->translation_id = translation_id;
+    printk("GICv3: Found ITS @0x%lx\n", addr);
+
+    list_add_tail(&its_data->entry, &host_its_list);
+
+    return 0;
+}
+
 /* Scan the DT for any ITS nodes and create a list of host ITSes out of it. */
 void gicv3_its_dt_init(const struct dt_device_node *node)
 {
     const struct dt_device_node *its = NULL;
-    struct host_its *its_data;
+    static int its_id = 1;
 
     /*
      * Check for ITS MSI subnodes. If any, add the ITS register
@@ -996,19 +1021,23 @@ 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");
+        if ( add_to_host_its_list(addr, size, its_id++, its) )
+            panic("GICV3: Adding Host ITS failed ");
+    }
+}
 
-        its_data->addr = addr;
-        its_data->size = size;
-        its_data->dt_node = its;
+#ifdef CONFIG_ACPI
+int gicv3_its_acpi_init(struct acpi_subtable_header *header, const unsigned long end)
+{
+    struct acpi_madt_generic_translator *its_entry;
 
-        printk("GICv3: Found ITS @0x%lx\n", addr);
+    its_entry = (struct acpi_madt_generic_translator *)header;
 
-        list_add_tail(&its_data->entry, &host_its_list);
-    }
+    return add_to_host_its_list(its_entry->base_address,
+                        ACPI_GICV3_ITS_MEM_SIZE,
+                        its_entry->translation_id, NULL);
 }
+#endif
 
 /*
  * Local variables:
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index c927306..558b32c 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1567,6 +1567,12 @@ static void __init gicv3_acpi_init(void)
 
     gicv3.rdist_stride = 0;
 
+    count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
+                                  gicv3_its_acpi_init, 0);
+
+    if ( count <= 0 )
+        panic("GICv3: Can't get ITS entry");
+
     /*
      * In ACPI, 0 is considered as the invalid address. However the rest
      * of the initialization rely on the invalid address to be
diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h
index 96b910b..bcfa181 100644
--- a/xen/include/asm-arm/gic_v3_its.h
+++ b/xen/include/asm-arm/gic_v3_its.h
@@ -105,6 +105,7 @@
 
 #include <xen/device_tree.h>
 #include <xen/rbtree.h>
+#include <xen/acpi.h>
 
 #define HOST_ITS_FLUSH_CMD_QUEUE        (1U << 0)
 #define HOST_ITS_USES_PTA               (1U << 1)
@@ -137,6 +138,11 @@ extern struct list_head host_its_list;
 /* Parse the host DT and pick up all host ITSes. */
 void gicv3_its_dt_init(const struct dt_device_node *node);
 
+#ifdef CONFIG_ACPI
+int gicv3_its_acpi_init(struct acpi_subtable_header *header,
+                                    const unsigned long end);
+#endif
+
 bool gicv3_its_host_has_its(void);
 
 unsigned int vgic_v3_its_count(const struct domain *d);
@@ -198,6 +204,14 @@ static inline void gicv3_its_dt_init(const struct dt_device_node *node)
 {
 }
 
+#ifdef CONFIG_ACPI
+static inline int gicv3_its_acpi_init(struct acpi_subtable_header *header,
+                                    const unsigned long end)
+{
+    return false;
+}
+#endif
+
 static inline bool gicv3_its_host_has_its(void)
 {
     return false;
-- 
2.7.4


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

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

[Manish Jaggi]

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

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

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


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

[PATCH 4/4] ARM: ACPI: Add ITS to hardware domain MADT

[Manish Jaggi]

This patch adds ITS information in hardware domain's MADT table.
Also this patch interoduces .get_hwdom_madt_size in gic_hw_operations,
to return the complete size of MADT table for hardware domain.

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

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 3abacc0..15c7f9b 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1802,12 +1802,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 ffbe47c..e92dc3d 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -1012,6 +1012,11 @@ static int gicv2_iomem_deny_access(const struct domain *d)
     return iomem_deny_access(d, mfn, mfn + nr);
 }
 
+static u32 gicv2_get_hwdom_madt_size(const struct domain *d)
+{
+    return 0;
+}
+
 #ifdef CONFIG_ACPI
 static int gicv2_make_hwdom_madt(const struct domain *d, u32 offset)
 {
@@ -1248,6 +1253,7 @@ const static struct gic_hw_operations gicv2_ops = {
     .read_apr            = gicv2_read_apr,
     .make_hwdom_dt_node  = gicv2_make_hwdom_dt_node,
     .make_hwdom_madt     = gicv2_make_hwdom_madt,
+    .get_hwdom_madt_size = gicv2_get_hwdom_madt_size,
     .map_hwdom_extra_mappings = gicv2_map_hwdown_extra_mappings,
     .iomem_deny_access   = gicv2_iomem_deny_access,
     .do_LPI              = gicv2_do_LPI,
diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index 98c8f46..7f8ff34 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -924,6 +924,40 @@ int gicv3_its_deny_access(const struct domain *d)
     return rc;
 }
 
+#ifdef CONFIG_ACPI
+u32 gicv3_its_madt_generic_translator_size(void)
+{
+    const struct host_its *its_data;
+    u32 size = 0;
+
+    list_for_each_entry(its_data, &host_its_list, entry)
+        size += sizeof(struct acpi_madt_generic_translator);
+
+    return size;
+}
+
+u32 gicv3_its_make_hwdom_madt(u8 *base_ptr, u32 offset)
+{
+    struct acpi_madt_generic_translator *gic_its;
+    const struct host_its *its_data;
+    u32 table_len = offset, size;
+
+    /* Update GIC ITS information in hardware domain's MADT */
+    list_for_each_entry(its_data, &host_its_list, entry)
+    {
+        size = sizeof(struct acpi_madt_generic_translator);
+        gic_its = (struct acpi_madt_generic_translator *)(base_ptr
+                   + table_len);
+        gic_its->header.type = ACPI_MADT_TYPE_GENERIC_TRANSLATOR;
+        gic_its->header.length = size;
+        gic_its->base_address = its_data->addr;
+        gic_its->translation_id = its_data->translation_id;
+        table_len +=  size;
+    }
+
+    return table_len;
+}
+#endif
 /*
  * Create the respective guest DT nodes from a list of host ITSes.
  * This copies the reg property, so the guest sees the ITS at the same address
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index f6fbf2f..c7a8c1c 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1407,9 +1407,21 @@ static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
         table_len += size;
     }
 
+    table_len = gicv3_its_make_hwdom_madt(base_ptr, table_len);
     return table_len;
 }
 
+static u32 gicv3_get_hwdom_madt_size(const struct domain *d)
+{
+    u32 size;
+    size  = sizeof(struct acpi_madt_generic_redistributor)
+                     * d->arch.vgic.nr_regions;
+    if ( gicv3_its_host_has_its() )
+        size  += gicv3_its_madt_generic_translator_size();
+
+    return size;
+}
+
 static int __init
 gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
                         const unsigned long end)
@@ -1605,6 +1617,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 */
@@ -1706,6 +1723,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 288e740..0bfb877 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -847,6 +847,17 @@ int gic_make_hwdom_madt(const struct domain *d, u32 offset)
     return gic_hw_ops->make_hwdom_madt(d, offset);
 }
 
+u32 gic_get_hwdom_madt_size(const struct domain *d)
+{
+    u32 madt_size;
+    madt_size = sizeof(struct acpi_table_madt)
+                + sizeof(struct acpi_madt_generic_interrupt) * d->max_vcpus
+                + sizeof(struct acpi_madt_generic_distributor)
+                + gic_hw_ops->get_hwdom_madt_size(d);
+
+    return madt_size;
+}
+
 int gic_iomem_deny_access(const struct domain *d)
 {
     return gic_hw_ops->iomem_deny_access(d);
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 6203dc5..a766e42 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -365,6 +365,8 @@ struct gic_hw_operations {
     int (*make_hwdom_madt)(const struct domain *d, u32 offset);
     /* Map extra GIC MMIO, irqs and other hw stuffs to the hardware domain. */
     int (*map_hwdom_extra_mappings)(struct domain *d);
+    /* Query the size of hardware domain madt table */
+    u32 (*get_hwdom_madt_size)(const struct domain *d);
     /* Deny access to GIC regions */
     int (*iomem_deny_access)(const struct domain *d);
     /* Handle LPIs, which require special handling */
@@ -376,6 +378,7 @@ int gic_make_hwdom_dt_node(const struct domain *d,
                            const struct dt_device_node *gic,
                            void *fdt);
 int gic_make_hwdom_madt(const struct domain *d, u32 offset);
+u32 gic_get_hwdom_madt_size(const struct domain *d);
 int gic_map_hwdom_extra_mappings(struct domain *d);
 int gic_iomem_deny_access(const struct domain *d);
 
diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h
index 84dbb9c..a629dbe 100644
--- a/xen/include/asm-arm/gic_v3_its.h
+++ b/xen/include/asm-arm/gic_v3_its.h
@@ -141,6 +141,8 @@ void gicv3_its_dt_init(const struct dt_device_node *node);
 #ifdef CONFIG_ACPI
 int gicv3_its_acpi_init(struct acpi_subtable_header *header,
                                     const unsigned long end);
+u32 gicv3_its_madt_generic_translator_size(void);
+u32 gicv3_its_make_hwdom_madt(u8 *base_ptr, u32 offset);
 #endif
 
 /* Deny iomem access for its */
@@ -213,6 +215,16 @@ static inline int gicv3_its_acpi_init(struct acpi_subtable_header *header,
 {
     return false;
 }
+
+static inline u32 gicv3_its_madt_generic_translator_size(void)
+{
+    return 0;
+}
+
+static inline u32 gicv3_its_make_hwdom_madt(u8 *base_ptr, u32 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 0/4] ARM: ACPI: ITS: Add ITS Support for ACPI hardware domain

[Julien Grall]

Hi Manish,

On 21/06/17 02:01, Manish Jaggi wrote:
> This patch series adds the support of ITS for ACPI hardware domain.
> It is tested on staging branch with has ITS v12 patchset by Andre.
>
> I have tried to incorporate the review comments on the RFC v1/v2 patch.
> The single patch in RFC is now split into 4 patches.

I will comment here rather than on each patches.

>
> Patch1: ARM: ITS: Add translation_id to host_its
>  Adds translation_id in host_its data structure, which is populated from
>  translation_id read from firmwar MADT. This value is then programmed into
>  local MADT created for hardware domain in patch 4.

I don't see any reason to store value that will only be used for 
generating the MADT which BTW is just a copy for the ITS. Instead we 
should copy over the MADT entries.

This would also avoid to introduce a fake ID for DT as you currently do 
in patch #2.

>
> Patch2: ARM: ITS: ACPI: Introduce gicv3_its_acpi_init
>  Introduces function for its_acpi_init, which calls add_to_host_its_list
>  which is a common function also called from _dt variant.

Just reading at the description, there are a call for splitting this 
patch... Looking at the code, you mix code movement and code addition.

Have a look at [1] to see how to break patches.

>
> Patch3: ARM: ITS: Deny hardware domain access to its
>  Extends the gicv3_iomem_deny to include its regions as well
>
> Patch4: ARM: ACPI: Add ITS to hardware domain MADT
>  This patch adds ITS information in hardware domain's MADT table.
>  Also this patch interoduces .get_hwdom_madt_size in gic_hw_operations,
>  to return the complete size of MADT table for hardware domain.

Same here.

>
>
> Manish Jaggi (4):
>   ARM: ITS: Add translation_id to host_its
>   ARM: ITS: ACPI: Introduce gicv3_its_acpi_init
>   ARM: ITS: Deny hardware domain access to its
>   ARM: ACPI: Add ITS to hardware domain MADT
>
>  xen/arch/arm/domain_build.c      |   7 +--
>  xen/arch/arm/gic-v2.c            |   6 +++
>  xen/arch/arm/gic-v3-its.c        | 102 +++++++++++++++++++++++++++++++++++----
>  xen/arch/arm/gic-v3.c            |  31 ++++++++++++
>  xen/arch/arm/gic.c               |  11 +++++
>  xen/include/asm-arm/gic.h        |   3 ++
>  xen/include/asm-arm/gic_v3_its.h |  36 ++++++++++++++
>  7 files changed, 180 insertions(+), 16 deletions(-)
>

Cheers,

[1] 
https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Making_good_patches

-- 
Julien Grall

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

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

[Manish Jaggi]

Hi Julien,

On 6/21/2017 6:53 PM, Julien Grall wrote:
> Hi Manish,
>
> On 21/06/17 02:01, Manish Jaggi wrote:
>> This patch series adds the support of ITS for ACPI hardware domain.
>> It is tested on staging branch with has ITS v12 patchset by Andre.
>>
>> I have tried to incorporate the review comments on the RFC v1/v2 patch.
>> The single patch in RFC is now split into 4 patches.
>
> I will comment here rather than on each patches.
>
>>
>> Patch1: ARM: ITS: Add translation_id to host_its
>>  Adds translation_id in host_its data structure, which is populated from
>>  translation_id read from firmwar MADT. This value is then programmed 
>> into
>>  local MADT created for hardware domain in patch 4.
>
> I don't see any reason to store value that will only be used for 
> generating the MADT which BTW is just a copy for the ITS. Instead we 
> should copy over the MADT entries.
>
There are two approaches,

If I use the standard API  acpi_table_parse_madt which would iterate 
over ACPI_MADT_TYPE_GENERIC_TRANSLATOR entries, I have to maintain the 
addr and translation_id in some data structure, to be filled later in 
the hwdomain copy of madt generic translator.

If I don't use the standard API I have to add code to manually parse all 
the translator entries.
Which of the two you find cleaner?
> This would also avoid to introduce a fake ID for DT as you currently 
> do in patch #2.
>
This can be avoided by storing translator_id only for acpi.

+static int add_to_host_its_list(u64 addr, u64 size,
+                      u32 translation_id, const void *node)
+{
+    struct host_its *its_data;
+    its_data = xzalloc(struct host_its);
+
+    if ( !its_data )
+        return -1;
+
+    if ( node )
+        its_data->dt_node = node;
+    else
+        its_data->translation_id = translation_id;
+
+    its_data->addr = addr;
+    its_data->size = size;
+    printk("GICv3: Found ITS @0x%lx\n", addr);
+
+    list_add_tail(&its_data->entry, &host_its_list);
+
+    return 0;

What do you think?
>>
>> Patch2: ARM: ITS: ACPI: Introduce gicv3_its_acpi_init
>>  Introduces function for its_acpi_init, which calls add_to_host_its_list
>>  which is a common function also called from _dt variant.
>
> Just reading at the description, there are a call for splitting this 
> patch... Looking at the code, you mix code movement and code addition.
>
> Have a look at [1] to see how to break patches.
>
Yes I will break into multiple patches patch 2 and 4.
>>
>> Patch3: ARM: ITS: Deny hardware domain access to its
>>  Extends the gicv3_iomem_deny to include its regions as well
>>
>> Patch4: ARM: ACPI: Add ITS to hardware domain MADT
>>  This patch adds ITS information in hardware domain's MADT table.
>>  Also this patch interoduces .get_hwdom_madt_size in gic_hw_operations,
>>  to return the complete size of MADT table for hardware domain.
>
> Same here.
Yes.
>
>>
>>
>> Manish Jaggi (4):
>>   ARM: ITS: Add translation_id to host_its
>>   ARM: ITS: ACPI: Introduce gicv3_its_acpi_init
>>   ARM: ITS: Deny hardware domain access to its
>>   ARM: ACPI: Add ITS to hardware domain MADT
>>
>>  xen/arch/arm/domain_build.c      |   7 +--
>>  xen/arch/arm/gic-v2.c            |   6 +++
>>  xen/arch/arm/gic-v3-its.c        | 102 
>> +++++++++++++++++++++++++++++++++++----
>>  xen/arch/arm/gic-v3.c            |  31 ++++++++++++
>>  xen/arch/arm/gic.c               |  11 +++++
>>  xen/include/asm-arm/gic.h        |   3 ++
>>  xen/include/asm-arm/gic_v3_its.h |  36 ++++++++++++++
>>  7 files changed, 180 insertions(+), 16 deletions(-)
>>
>
> Cheers,
>
> [1] 
> https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Making_good_patches
>


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

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

[Julien Grall]

On 10/08/17 12:21, Manish Jaggi wrote:
> Hi Julien,
>
> On 6/21/2017 6:53 PM, Julien Grall wrote:
>> Hi Manish,
>>
>> On 21/06/17 02:01, Manish Jaggi wrote:
>>> This patch series adds the support of ITS for ACPI hardware domain.
>>> It is tested on staging branch with has ITS v12 patchset by Andre.
>>>
>>> I have tried to incorporate the review comments on the RFC v1/v2 patch.
>>> The single patch in RFC is now split into 4 patches.
>>
>> I will comment here rather than on each patches.
>>
>>>
>>> Patch1: ARM: ITS: Add translation_id to host_its
>>>  Adds translation_id in host_its data structure, which is populated from
>>>  translation_id read from firmwar MADT. This value is then programmed
>>> into
>>>  local MADT created for hardware domain in patch 4.
>>
>> I don't see any reason to store value that will only be used for
>> generating the MADT which BTW is just a copy for the ITS. Instead we
>> should copy over the MADT entries.
>>
> There are two approaches,
>
> If I use the standard API  acpi_table_parse_madt which would iterate
> over ACPI_MADT_TYPE_GENERIC_TRANSLATOR entries, I have to maintain the
> addr and translation_id in some data structure, to be filled later in
> the hwdomain copy of madt generic translator.
>
> If I don't use the standard API I have to add code to manually parse all
> the translator entries.

There are a 3rd approach I suggested and ignored... The ITS entries for 
Dom0 is exactly the same as the host entries. So you only need to do a 
verbatim copy of the entry...

> Which of the two you find cleaner?
>> This would also avoid to introduce a fake ID for DT as you currently
>> do in patch #2.
>>
> This can be avoided by storing translator_id only for acpi.
>
> +static int add_to_host_its_list(u64 addr, u64 size,
> +                      u32 translation_id, const void *node)
> +{
> +    struct host_its *its_data;
> +    its_data = xzalloc(struct host_its);
> +
> +    if ( !its_data )
> +        return -1;
> +
> +    if ( node )
> +        its_data->dt_node = node;
> +    else
> +        its_data->translation_id = translation_id;
> +
> +    its_data->addr = addr;
> +    its_data->size = size;
> +    printk("GICv3: Found ITS @0x%lx\n", addr);
> +
> +    list_add_tail(&its_data->entry, &host_its_list);
> +
> +    return 0;
>
> What do you think?

I don't want to see the translation_id stored for no use at all but 
creating the DOM0 ACPI tables. Is that clearer?

Cheers,

-- 
Julien Grall

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

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

[Manish Jaggi]

Hi Julien,

On 8/10/2017 4:58 PM, Julien Grall wrote:
>
>
> On 10/08/17 12:21, Manish Jaggi wrote:
>> Hi Julien,
>>
>> On 6/21/2017 6:53 PM, Julien Grall wrote:
>>> Hi Manish,
>>>
>>> On 21/06/17 02:01, Manish Jaggi wrote:
>>>> This patch series adds the support of ITS for ACPI hardware domain.
>>>> It is tested on staging branch with has ITS v12 patchset by Andre.
>>>>
>>>> I have tried to incorporate the review comments on the RFC v1/v2 
>>>> patch.
>>>> The single patch in RFC is now split into 4 patches.
>>>
>>> I will comment here rather than on each patches.
>>>
>>>>
>>>> Patch1: ARM: ITS: Add translation_id to host_its
>>>>  Adds translation_id in host_its data structure, which is populated 
>>>> from
>>>>  translation_id read from firmwar MADT. This value is then programmed
>>>> into
>>>>  local MADT created for hardware domain in patch 4.
>>>
>>> I don't see any reason to store value that will only be used for
>>> generating the MADT which BTW is just a copy for the ITS. Instead we
>>> should copy over the MADT entries.
>>>
>> There are two approaches,
>>
>> If I use the standard API  acpi_table_parse_madt which would iterate
>> over ACPI_MADT_TYPE_GENERIC_TRANSLATOR entries, I have to maintain the
>> addr and translation_id in some data structure, to be filled later in
>> the hwdomain copy of madt generic translator.
>>
>> If I don't use the standard API I have to add code to manually parse all
>> the translator entries.
>
> There are a 3rd approach I suggested and ignored... The ITS entries 
> for Dom0 is exactly the same as the host entries.
Yes, and if not passed properly dom0 wont get device interrupts...
> So you only need to do a verbatim copy of the entry...
>
Can you please check patch 4/2, the translation_id and address are 
passed verbatim, the other values are reserved in 
acpi_madt_generic_translator.

Could you please detail 3rd approach and how different it is from 
approach 2.
>> Which of the two you find cleaner?
>>> This would also avoid to introduce a fake ID for DT as you currently
>>> do in patch #2.
>>>
>> This can be avoided by storing translator_id only for acpi.
>>
>> +static int add_to_host_its_list(u64 addr, u64 size,
>> +                      u32 translation_id, const void *node)
>> +{
>> +    struct host_its *its_data;
>> +    its_data = xzalloc(struct host_its);
>> +
>> +    if ( !its_data )
>> +        return -1;
>> +
>> +    if ( node )
>> +        its_data->dt_node = node;
>> +    else
>> +        its_data->translation_id = translation_id;
>> +
>> +    its_data->addr = addr;
>> +    its_data->size = size;
>> +    printk("GICv3: Found ITS @0x%lx\n", addr);
>> +
>> +    list_add_tail(&its_data->entry, &host_its_list);
>> +
>> +    return 0;
>>
>> What do you think?
>
> I don't want to see the translation_id stored for no use at all but 
> creating the DOM0 ACPI tables. Is that clearer?
ok, I will remove it.
>
> Cheers,
>


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

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

[Julien Grall]

On 10/08/17 13:00, Manish Jaggi wrote:
> Hi Julien,
>
> On 8/10/2017 4:58 PM, Julien Grall wrote:
>>
>>
>> On 10/08/17 12:21, Manish Jaggi wrote:
>>> Hi Julien,
>>>
>>> On 6/21/2017 6:53 PM, Julien Grall wrote:
>>>> Hi Manish,
>>>>
>>>> On 21/06/17 02:01, Manish Jaggi wrote:
>>>>> This patch series adds the support of ITS for ACPI hardware domain.
>>>>> It is tested on staging branch with has ITS v12 patchset by Andre.
>>>>>
>>>>> I have tried to incorporate the review comments on the RFC v1/v2
>>>>> patch.
>>>>> The single patch in RFC is now split into 4 patches.
>>>>
>>>> I will comment here rather than on each patches.
>>>>
>>>>>
>>>>> Patch1: ARM: ITS: Add translation_id to host_its
>>>>>  Adds translation_id in host_its data structure, which is populated
>>>>> from
>>>>>  translation_id read from firmwar MADT. This value is then programmed
>>>>> into
>>>>>  local MADT created for hardware domain in patch 4.
>>>>
>>>> I don't see any reason to store value that will only be used for
>>>> generating the MADT which BTW is just a copy for the ITS. Instead we
>>>> should copy over the MADT entries.
>>>>
>>> There are two approaches,
>>>
>>> If I use the standard API  acpi_table_parse_madt which would iterate
>>> over ACPI_MADT_TYPE_GENERIC_TRANSLATOR entries, I have to maintain the
>>> addr and translation_id in some data structure, to be filled later in
>>> the hwdomain copy of madt generic translator.
>>>
>>> If I don't use the standard API I have to add code to manually parse all
>>> the translator entries.
>>
>> There are a 3rd approach I suggested and ignored... The ITS entries
>> for Dom0 is exactly the same as the host entries.
> Yes, and if not passed properly dom0 wont get device interrupts...
>> So you only need to do a verbatim copy of the entry...
>>
> Can you please check patch 4/2, the translation_id and address are
> passed verbatim, the other values are reserved in
> acpi_madt_generic_translator.

For ACPI, we took the approach to only rewrite what's necessary and give 
the rest to Dom0 as it is. If newer version of ACPI re-used those 
fields, then they will be copied over to Dom0. I don't consider it as an 
issue because the problem would be the same if those fields have an 
important meaning for the platform.

>
> Could you please detail 3rd approach and how different it is from
> approach 2.

ACPI_MEMCPY(its, host_its, size);

Cheers,

-- 
Julien Grall

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

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

[Manish Jaggi]

HI Julien,

On 8/10/2017 5:43 PM, Julien Grall wrote:
>
>
> On 10/08/17 13:00, Manish Jaggi wrote:
>> Hi Julien,
>>
>> On 8/10/2017 4:58 PM, Julien Grall wrote:
>>>
>>>
>>> On 10/08/17 12:21, Manish Jaggi wrote:
>>>> Hi Julien,
>>>>
>>>> On 6/21/2017 6:53 PM, Julien Grall wrote:
>>>>> Hi Manish,
>>>>>
>>>>> On 21/06/17 02:01, Manish Jaggi wrote:
>>>>>> This patch series adds the support of ITS for ACPI hardware domain.
>>>>>> It is tested on staging branch with has ITS v12 patchset by Andre.
>>>>>>
>>>>>> I have tried to incorporate the review comments on the RFC v1/v2
>>>>>> patch.
>>>>>> The single patch in RFC is now split into 4 patches.
>>>>>
>>>>> I will comment here rather than on each patches.
>>>>>
>>>>>>
>>>>>> Patch1: ARM: ITS: Add translation_id to host_its
>>>>>>  Adds translation_id in host_its data structure, which is populated
>>>>>> from
>>>>>>  translation_id read from firmwar MADT. This value is then 
>>>>>> programmed
>>>>>> into
>>>>>>  local MADT created for hardware domain in patch 4.
>>>>>
>>>>> I don't see any reason to store value that will only be used for
>>>>> generating the MADT which BTW is just a copy for the ITS. Instead we
>>>>> should copy over the MADT entries.
>>>>>
>>>> There are two approaches,
>>>>
>>>> If I use the standard API  acpi_table_parse_madt which would iterate
>>>> over ACPI_MADT_TYPE_GENERIC_TRANSLATOR entries, I have to maintain the
>>>> addr and translation_id in some data structure, to be filled later in
>>>> the hwdomain copy of madt generic translator.
>>>>
>>>> If I don't use the standard API I have to add code to manually 
>>>> parse all
>>>> the translator entries.
>>>
>>> There are a 3rd approach I suggested and ignored... The ITS entries
>>> for Dom0 is exactly the same as the host entries.
>> Yes, and if not passed properly dom0 wont get device interrupts...
>>> So you only need to do a verbatim copy of the entry...
>>>
>> Can you please check patch 4/2, the translation_id and address are
>> passed verbatim, the other values are reserved in
>> acpi_madt_generic_translator.
>
> For ACPI, we took the approach to only rewrite what's necessary and 
> give the rest to Dom0 as it is. If newer version of ACPI re-used those 
> fields, then they will be copied over to Dom0. I don't consider it as 
> an issue because the problem would be the same if those fields have an 
> important meaning for the platform.
Few thoughts...
If we follow this approach, few points needs to be considered
- If ACPI may use the reserved information later it could be equally 
important for dom0 and Xen,
  so it might be useful to keep reserved in xen as well.

- For platforms which use dt, translation_id is not required to be 
stored in struct host_its, similarly for platforms which use acpi
dt_node pointer might be of no use.

So we can have struct host_its having a union with dt_device_node * for 
dt and acpi_madt_generic_translator * for acpi.
IMHO this could be an approach we can take.

struct host_its {
      struct list_head entry;
-    const struct dt_device_node *dt_node;
+   union {
+    const struct dt_device_node *dt_node;
+    const struct acpi_madt_generic_translator *acpi_its_entry;
+    };
     paddr_t addr;


>
>>
>> Could you please detail 3rd approach and how different it is from
>> approach 2.
>
> ACPI_MEMCPY(its, host_its, size);
>
> Cheers,
>


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

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

[Julien Grall]

On 08/10/2017 02:00 PM, Manish Jaggi wrote:
> HI Julien,
> 
> On 8/10/2017 5:43 PM, Julien Grall wrote:
>>
>>
>> On 10/08/17 13:00, Manish Jaggi wrote:
>>> Hi Julien,
>>>
>>> On 8/10/2017 4:58 PM, Julien Grall wrote:
>>>>
>>>>
>>>> On 10/08/17 12:21, Manish Jaggi wrote:
>>>>> Hi Julien,
>>>>>
>>>>> On 6/21/2017 6:53 PM, Julien Grall wrote:
>>>>>> Hi Manish,
>>>>>>
>>>>>> On 21/06/17 02:01, Manish Jaggi wrote:
>>>>>>> This patch series adds the support of ITS for ACPI hardware domain.
>>>>>>> It is tested on staging branch with has ITS v12 patchset by Andre.
>>>>>>>
>>>>>>> I have tried to incorporate the review comments on the RFC v1/v2
>>>>>>> patch.
>>>>>>> The single patch in RFC is now split into 4 patches.
>>>>>>
>>>>>> I will comment here rather than on each patches.
>>>>>>
>>>>>>>
>>>>>>> Patch1: ARM: ITS: Add translation_id to host_its
>>>>>>>  Adds translation_id in host_its data structure, which is populated
>>>>>>> from
>>>>>>>  translation_id read from firmwar MADT. This value is then 
>>>>>>> programmed
>>>>>>> into
>>>>>>>  local MADT created for hardware domain in patch 4.
>>>>>>
>>>>>> I don't see any reason to store value that will only be used for
>>>>>> generating the MADT which BTW is just a copy for the ITS. Instead we
>>>>>> should copy over the MADT entries.
>>>>>>
>>>>> There are two approaches,
>>>>>
>>>>> If I use the standard API  acpi_table_parse_madt which would iterate
>>>>> over ACPI_MADT_TYPE_GENERIC_TRANSLATOR entries, I have to maintain the
>>>>> addr and translation_id in some data structure, to be filled later in
>>>>> the hwdomain copy of madt generic translator.
>>>>>
>>>>> If I don't use the standard API I have to add code to manually 
>>>>> parse all
>>>>> the translator entries.
>>>>
>>>> There are a 3rd approach I suggested and ignored... The ITS entries
>>>> for Dom0 is exactly the same as the host entries.
>>> Yes, and if not passed properly dom0 wont get device interrupts...
>>>> So you only need to do a verbatim copy of the entry...
>>>>
>>> Can you please check patch 4/2, the translation_id and address are
>>> passed verbatim, the other values are reserved in
>>> acpi_madt_generic_translator.
>>
>> For ACPI, we took the approach to only rewrite what's necessary and 
>> give the rest to Dom0 as it is. If newer version of ACPI re-used those 
>> fields, then they will be copied over to Dom0. I don't consider it as 
>> an issue because the problem would be the same if those fields have an 
>> important meaning for the platform.
> Few thoughts...
> If we follow this approach, few points needs to be considered
> - If ACPI may use the reserved information later it could be equally 
> important for dom0 and Xen,
>   so it might be useful to keep reserved in xen as well.

I already covered that in my previous e-mail.

> 
> - For platforms which use dt, translation_id is not required to be 
> stored in struct host_its, similarly for platforms which use acpi
> dt_node pointer might be of no use.
> 
> So we can have struct host_its having a union with dt_device_node * for 
> dt and acpi_madt_generic_translator * for acpi.
> IMHO this could be an approach we can take.
> 
> struct host_its {
>       struct list_head entry;
> -    const struct dt_device_node *dt_node;
> +   union {
> +    const struct dt_device_node *dt_node;
> +    const struct acpi_madt_generic_translator *acpi_its_entry;
> +    };
>      paddr_t addr;

What don't you get in my previous e-mail? A no is a no, full stop.

Just do what we do in *_make_hwdom_madt. That will work here with no 
need of a union or anything else. Even the DT code can be reworked to 
avoid storing the node.

We could even have the common code go through the MADT entry one by one 
and let the specific driver updating what's necessary avoid ACPI_MEMCPY 
duplication in each bit. But this is a longer term thoughts than for.

Cheers,

-- 
Julien Grall

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

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

[Manish Jaggi]

On 8/10/2017 6:44 PM, Julien Grall wrote:
>
>
> On 08/10/2017 02:00 PM, Manish Jaggi wrote:
>> HI Julien,
>>
>> On 8/10/2017 5:43 PM, Julien Grall wrote:
>>>
>>>
>>> On 10/08/17 13:00, Manish Jaggi wrote:
>>>> Hi Julien,
>>>>
>>>> On 8/10/2017 4:58 PM, Julien Grall wrote:
>>>>>
>>>>>
>>>>> On 10/08/17 12:21, Manish Jaggi wrote:
>>>>>> Hi Julien,
>>>>>>
>>>>>> On 6/21/2017 6:53 PM, Julien Grall wrote:
>>>>>>> Hi Manish,
>>>>>>>
>>>>>>> On 21/06/17 02:01, Manish Jaggi wrote:
>>>>>>>> This patch series adds the support of ITS for ACPI hardware 
>>>>>>>> domain.
>>>>>>>> It is tested on staging branch with has ITS v12 patchset by Andre.
>>>>>>>>
>>>>>>>> I have tried to incorporate the review comments on the RFC v1/v2
>>>>>>>> patch.
>>>>>>>> The single patch in RFC is now split into 4 patches.
>>>>>>>
>>>>>>> I will comment here rather than on each patches.
>>>>>>>
>>>>>>>>
>>>>>>>> Patch1: ARM: ITS: Add translation_id to host_its
>>>>>>>>  Adds translation_id in host_its data structure, which is 
>>>>>>>> populated
>>>>>>>> from
>>>>>>>>  translation_id read from firmwar MADT. This value is then 
>>>>>>>> programmed
>>>>>>>> into
>>>>>>>>  local MADT created for hardware domain in patch 4.
>>>>>>>
>>>>>>> I don't see any reason to store value that will only be used for
>>>>>>> generating the MADT which BTW is just a copy for the ITS. 
>>>>>>> Instead we
>>>>>>> should copy over the MADT entries.
>>>>>>>
>>>>>> There are two approaches,
>>>>>>
>>>>>> If I use the standard API  acpi_table_parse_madt which would iterate
>>>>>> over ACPI_MADT_TYPE_GENERIC_TRANSLATOR entries, I have to 
>>>>>> maintain the
>>>>>> addr and translation_id in some data structure, to be filled 
>>>>>> later in
>>>>>> the hwdomain copy of madt generic translator.
>>>>>>
>>>>>> If I don't use the standard API I have to add code to manually 
>>>>>> parse all
>>>>>> the translator entries.
>>>>>
>>>>> There are a 3rd approach I suggested and ignored... The ITS entries
>>>>> for Dom0 is exactly the same as the host entries.
>>>> Yes, and if not passed properly dom0 wont get device interrupts...
>>>>> So you only need to do a verbatim copy of the entry...
>>>>>
>>>> Can you please check patch 4/2, the translation_id and address are
>>>> passed verbatim, the other values are reserved in
>>>> acpi_madt_generic_translator.
>>>
>>> For ACPI, we took the approach to only rewrite what's necessary and 
>>> give the rest to Dom0 as it is. If newer version of ACPI re-used 
>>> those fields, then they will be copied over to Dom0. I don't 
>>> consider it as an issue because the problem would be the same if 
>>> those fields have an important meaning for the platform.
>> Few thoughts...
>> If we follow this approach, few points needs to be considered
>> - If ACPI may use the reserved information later it could be equally 
>> important for dom0 and Xen,
>>   so it might be useful to keep reserved in xen as well.
>
> I already covered that in my previous e-mail.
>
Yes, I am just stating it again for xen.
>>
>> - For platforms which use dt, translation_id is not required to be 
>> stored in struct host_its, similarly for platforms which use acpi
>> dt_node pointer might be of no use.
>>
>> So we can have struct host_its having a union with dt_device_node * 
>> for dt and acpi_madt_generic_translator * for acpi.
>> IMHO this could be an approach we can take.
>>
>> struct host_its {
>>       struct list_head entry;
>> -    const struct dt_device_node *dt_node;
>> +   union {
>> +    const struct dt_device_node *dt_node;
>> +    const struct acpi_madt_generic_translator *acpi_its_entry;
>> +    };
>>      paddr_t addr;
>
> What don't you get in my previous e-mail? A no is a no, full stop.
This is not helping.
>
>
> Just do what we do in *_make_hwdom_madt. That will work here with no 
> need of a union or anything else.
The patchset provides two features
  (a) populates host_its list from ACPI tables, so ACPI xen can use ITS
  (b) provides a MADT with ITS information to dom0.

What I am focusing with union is for (a) ,
and (b) code would be simpler if we use the union in (a).

You seem to be discounting (a) in comments so far.

why union? as I have mentioned before...
  It will make the host_its structure accommodate dt node and 
acpi_madt_generic_translator, both has same purpose.
If one is valid why not other.

please provide a technical reason for not doing it.

> Even the DT code can be reworked to avoid storing the node.
>
we can have a separate patch for that.


> Cheers,
>
Cheers!
Sending next rev shortly.

-manish

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

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

[Julien Grall]

Hi,

On 11/08/17 05:54, Manish Jaggi wrote:
>
>
> On 8/10/2017 6:44 PM, Julien Grall wrote:
>>
>>
>> On 08/10/2017 02:00 PM, Manish Jaggi wrote:
>>> HI Julien,
>>>
>>> On 8/10/2017 5:43 PM, Julien Grall wrote:
>>>>
>>>>
>>>> On 10/08/17 13:00, Manish Jaggi wrote:
>>>>> Hi Julien,
>>>>>
>>>>> On 8/10/2017 4:58 PM, Julien Grall wrote:
>>>>>>
>>>>>>
>>>>>> On 10/08/17 12:21, Manish Jaggi wrote:
>>>>>>> Hi Julien,
>>>>>>>
>>>>>>> On 6/21/2017 6:53 PM, Julien Grall wrote:
>>>>>>>> Hi Manish,
>>>>>>>>
>>>>>>>> On 21/06/17 02:01, Manish Jaggi wrote:
>>>>>>>>> This patch series adds the support of ITS for ACPI hardware
>>>>>>>>> domain.
>>>>>>>>> It is tested on staging branch with has ITS v12 patchset by Andre.
>>>>>>>>>
>>>>>>>>> I have tried to incorporate the review comments on the RFC v1/v2
>>>>>>>>> patch.
>>>>>>>>> The single patch in RFC is now split into 4 patches.
>>>>>>>>
>>>>>>>> I will comment here rather than on each patches.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Patch1: ARM: ITS: Add translation_id to host_its
>>>>>>>>>  Adds translation_id in host_its data structure, which is
>>>>>>>>> populated
>>>>>>>>> from
>>>>>>>>>  translation_id read from firmwar MADT. This value is then
>>>>>>>>> programmed
>>>>>>>>> into
>>>>>>>>>  local MADT created for hardware domain in patch 4.
>>>>>>>>
>>>>>>>> I don't see any reason to store value that will only be used for
>>>>>>>> generating the MADT which BTW is just a copy for the ITS.
>>>>>>>> Instead we
>>>>>>>> should copy over the MADT entries.
>>>>>>>>
>>>>>>> There are two approaches,
>>>>>>>
>>>>>>> If I use the standard API  acpi_table_parse_madt which would iterate
>>>>>>> over ACPI_MADT_TYPE_GENERIC_TRANSLATOR entries, I have to
>>>>>>> maintain the
>>>>>>> addr and translation_id in some data structure, to be filled
>>>>>>> later in
>>>>>>> the hwdomain copy of madt generic translator.
>>>>>>>
>>>>>>> If I don't use the standard API I have to add code to manually
>>>>>>> parse all
>>>>>>> the translator entries.
>>>>>>
>>>>>> There are a 3rd approach I suggested and ignored... The ITS entries
>>>>>> for Dom0 is exactly the same as the host entries.
>>>>> Yes, and if not passed properly dom0 wont get device interrupts...
>>>>>> So you only need to do a verbatim copy of the entry...
>>>>>>
>>>>> Can you please check patch 4/2, the translation_id and address are
>>>>> passed verbatim, the other values are reserved in
>>>>> acpi_madt_generic_translator.
>>>>
>>>> For ACPI, we took the approach to only rewrite what's necessary and
>>>> give the rest to Dom0 as it is. If newer version of ACPI re-used
>>>> those fields, then they will be copied over to Dom0. I don't
>>>> consider it as an issue because the problem would be the same if
>>>> those fields have an important meaning for the platform.
>>> Few thoughts...
>>> If we follow this approach, few points needs to be considered
>>> - If ACPI may use the reserved information later it could be equally
>>> important for dom0 and Xen,
>>>   so it might be useful to keep reserved in xen as well.
>>
>> I already covered that in my previous e-mail.
>>
> Yes, I am just stating it again for xen.
>>>
>>> - For platforms which use dt, translation_id is not required to be
>>> stored in struct host_its, similarly for platforms which use acpi
>>> dt_node pointer might be of no use.
>>>
>>> So we can have struct host_its having a union with dt_device_node *
>>> for dt and acpi_madt_generic_translator * for acpi.
>>> IMHO this could be an approach we can take.
>>>
>>> struct host_its {
>>>       struct list_head entry;
>>> -    const struct dt_device_node *dt_node;
>>> +   union {
>>> +    const struct dt_device_node *dt_node;
>>> +    const struct acpi_madt_generic_translator *acpi_its_entry;
>>> +    };
>>>      paddr_t addr;
>>
>> What don't you get in my previous e-mail? A no is a no, full stop.
> This is not helping.
>>
>>
>> Just do what we do in *_make_hwdom_madt. That will work here with no
>> need of a union or anything else.
> The patchset provides two features
>  (a) populates host_its list from ACPI tables, so ACPI xen can use ITS
>  (b) provides a MADT with ITS information to dom0.
>
> What I am focusing with union is for (a) ,
> and (b) code would be simpler if we use the union in (a).
>
> You seem to be discounting (a) in comments so far.
>
> why union? as I have mentioned before...
>  It will make the host_its structure accommodate dt node and
> acpi_madt_generic_translator, both has same purpose.
> If one is valid why not other.

You commented on "Even the DT code can be reworked to avoid storing the 
node.", so I guess you can easily deduce by yourself why I don't think 
it should be used.

To repeat for the 4th time, there are need to keep around both DT and 
ACPI pointer in the structure. Maybe some code will help you to understand:

for (i = 0; i < nr_its; i++)
{
     struct acpi_madt_.... *entry;

     entry = acpi_table_get_entry_madt(ACPI_MADT_TYPE_ITS, i);
     BUG_ON(entry);

     ACPI_MEMCPY(..., ...);
}

Job done.

>
> please provide a technical reason for not doing it.

I would appreciate to not repeat 4 times (including on IRC) the same 
things... I don't have spare time for that. So as I said either you 
address my comment or I am going to ignore this until I find spare time 
to deal with it.

Cheers,

-- 
Julien Grall

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

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

[Manish Jaggi]

Hi,

This patch series adds the support of ITS for ACPI hardware domain.
It is tested on staging branch with has ITS v12 patchset by Andre.

I have tried to incorporate the review comments on the RFC v1/v2 patch.
The single patch in RFC is now split into 4 patches.

Patch1: ARM: ITS: Add translation_id to host_its
  Adds translation_id in host_its data structure, which is populated from
  translation_id read from firmware MADT. This value is then programmed into
  local MADT created for hardware domain in patch 4.

Patch2: ARM: ITS: ACPI: Introduce gicv3_its_acpi_init
  Introduces function for its_acpi_init, which calls add_to_host_its_list
  which is a common function also called from _dt variant.

Patch3: ARM: ITS: Deny hardware domain access to its
  Extends the gicv3_iomem_deny to include its regions as well

Patch4: ARM: ACPI: Add ITS to hardware domain MADT
  This patch adds ITS information in hardware domain's MADT table.
  Also this patch introduces .get_hwdom_madt_size in gic_hw_operations,
  to return the complete size of MADT table for hardware domain.

Manish Jaggi (4):
   ARM: ITS: Add translation_id to host_its
   ARM: ITS: ACPI: Introduce gicv3_its_acpi_init
   ARM: ITS: Deny hardware domain access to its
   ARM: ACPI: Add ITS to hardware domain MADT

  xen/arch/arm/domain_build.c      |   7 +--
  xen/arch/arm/gic-v2.c            |   6 +++
  xen/arch/arm/gic-v3-its.c        | 102 
+++++++++++++++++++++++++++++++++++----
  xen/arch/arm/gic-v3.c            |  31 ++++++++++++
  xen/arch/arm/gic.c               |  11 +++++
  xen/include/asm-arm/gic.h        |   3 ++
  xen/include/asm-arm/gic_v3_its.h |  36 ++++++++++++++
  7 files changed, 180 insertions(+), 16 deletions(-)

-- 
2.7.4


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