acpi: arm: Add IORT Support

From: Manish Jaggi <[email protected]>

IORT has a hierarchical structure containing PCIRC nodes, IORT nodes and SMMU nodes. Each node has with it an array of ids and a mapping which maps a range of ids to another node's ids. PCIRC(requesterid)->SMMU(streamid)->ITS(devid) or PCIRC->ITS

IORT is parsed multiple times when streamid(sid) / deviceid(devid) is queried from requesterid (rid).

Xen needs to prepare IORT for hardware domain which might again require parsing. Thus it is prudent to parse IORT once and save mapping information into individual maps namely rid-sid rid-devid.

This patch provides API to add a new mapping and query sid/devid based on rid. Two lists are created rid-sid list, rid-devid list. rid-devid list forms the basis of hardware domains' IORT.

Signed-off-by Manish Jaggi <[email protected]>

---
  xen/arch/arm/acpi/Makefile        |   1 +
  xen/arch/arm/acpi/ridmap.c        | 126 ++++++++++++++++++++++++++++++++++++++
  xen/include/asm-arm/acpi/ridmap.h | 112 +++++++++++++++++++++++++++++++++
  3 files changed, 239 insertions(+)

xen/arch/arm/acpi/Makefile +1 -0

index 23963f8fa0..eb7e8ce4f7 100644
@@ -1,2 +1,3 @@
  obj-y += lib.o
  obj-y += boot.init.o
+obj-y += ridmap.o

xen/arch/arm/acpi/ridmap.c new file +19 -0

new file mode 100644
index 0000000000..daa137f625
@@ -0,0 +1,126 @@
+/*
+ * xen/drivers/acpi/arm/ridmap.c
+ *
+ * This file implements rid-sid rid-devid mapping API
+ *
+ * Manish Jaggi <[email protected]>
+ * Copyright (c) 2018 Linaro.
+ *
+ * Ths program is free software; you can redistribute it and/or
+ * modify it under the terms and conditions of the GNU General Public
+ * License, version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <asm/acpi/ridmap.h>

xen/arch/arm/acpi/ridmap.c new file +10 -0

+{
+    struct rid_sid_map *rid_map;
+
+    rid_map = xzalloc(struct rid_sid_map);
+    if ( !rid_map )
+        return -ENOMEM;
+
+    rid_map->idmap.input_base = input_base;
+    rid_map->idmap.output_base = output_base;
+    rid_map->idmap.id_count = id_count;

xen/arch/arm/acpi/ridmap.c new file +73 -0

+{
+    struct rid_devid_map *rid_map;
+
+    rid_map = xzalloc(struct rid_devid_map);
+    if ( !rid_map )
+        return -ENOMEM;
+
+    rid_map->idmap.input_base = input_base;
+    rid_map->idmap.output_base = output_base;
+    rid_map->idmap.id_count = id_count;
+    rid_map->pcirc_node = pcirc_node;
+    rid_map->its_node = its_node;
+
+    list_add_tail(&rid_map->entry, &rid_devid_list);
+
+    return 0;
+}
+
+bool query_sid(struct acpi_iort_node *pcirc_node, uint32_t rid,
+               uint32_t *sid, struct acpi_iort_node **smmu_node)
+{
+    struct rid_sid_map *rmap;
+
+    list_for_each_entry(rmap, &rid_sid_list, entry)
+    {
+        if ( rmap->pcirc_node == pcirc_node )
+        {
+            if ( (rid >= rmap->idmap.input_base) &&
+                 (rid < rmap->idmap.input_base + rmap->idmap.id_count) )
+            {
+                *sid = rid - rmap->idmap.input_base +
+                       rmap->idmap.output_base;
+                *smmu_node = rmap->smmu_node;
+
+                return 1;
+            }
+        }
+    }
+
+    return 0;
+}
+
+bool query_devid(struct acpi_iort_node *pcirc_node,
+                uint32_t rid, uint32_t *devid)
+{
+    struct rid_devid_map *rmap;
+
+    list_for_each_entry(rmap, &rid_devid_list, entry)
+    {
+        if ( rmap->pcirc_node == pcirc_node )
+        {
+            if ( (rid >= rmap->idmap.input_base) &&
+                 (rid < rmap->idmap.input_base + rmap->idmap.id_count) )
+            {
+                *devid = rid - rmap->idmap.input_base +
+                         rmap->idmap.output_base;
+
+                return 1;
+            }
+        }
+    }
+
+    return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */

xen/include/asm-arm/acpi/ridmap.h new file +22 -0

new file mode 100644
index 0000000000..5d12d86c3a
@@ -0,0 +1,112 @@
+/*
+ * xen/include/acpi/ridmap.h
+ *
+ * Manish Jaggi <[email protected]>
+ * Copyright (c) 2018 Linaro.
+ *
+ * Ths program is free software; you can redistribute it and/or
+ * modify it under the terms and conditions of the GNU General Public
+ * License, version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __ASM_ACPI_RIDMAP_H__
+#define __ASM_ACPI_RIDMAP_H__
+
+#include <xen/acpi.h>
+
+/*

xen/include/asm-arm/acpi/ridmap.h new file +26 -0

+ * Note: output_reference and flags members of acpi_iort_id_mapping
+ * are not used. This is done to avoid creating a new structure for
+ * same purpose.
+ *
+ * smmu node pointer is stored in this structure because, in some places
+ * smmu_node along with streamid is required based on rid and pcirc_node.
+ */
+struct rid_sid_map
+{
+    struct acpi_iort_node *pcirc_node;
+    struct acpi_iort_node *smmu_node;
+    struct acpi_iort_id_mapping idmap;
+    struct list_head entry;
+};
+
+/*
+ * API to add a rid-sid mapping
+ * This method should be called while parsing each entry in idmap array
+ * under the pcirc node in IORT.
+ */
+int add_rid_sid_map(struct acpi_iort_node *pcirc_node,
+                    struct acpi_iort_node *smmu_node,
+                    uint32_t input_base, uint32_t output_base,
+                    uint32_t id_count);
+/*
+ * API to query sid and smmu_node based on pcirc_node and rid.

xen/include/asm-arm/acpi/ridmap.h new file +3 -0

+bool query_devid(struct acpi_iort_node *pcirc_node, uint32_t rid,
+                 uint32_t *devid);
+

Hi Manish,

Quoted 14 lines

From: Manish Jaggi <[email protected]>

IORT has a hierarchical structure containing PCIRC nodes, IORT nodes and SMMU nodes. Each node has with it an array of ids and a mapping which maps a range of ids to another node's ids. PCIRC(requesterid)->SMMU(streamid)->ITS(devid) or PCIRC->ITS

IORT is parsed multiple times when streamid(sid) / deviceid(devid) is queried from requesterid (rid).

Xen needs to prepare IORT for hardware domain which might again require parsing. Thus it is prudent to parse IORT once and save mapping information into individual maps namely rid-sid rid-devid.

Quoted 3 lines

This patch provides API to add a new mapping and query sid/devid based on rid. Two lists are created rid-sid list, rid-devid list. rid-devid list forms the basis of hardware domains' IORT.

Thank you for updating the commit message. However, you stil don't give an idea often those function will get called and whether unsorted list will be fine.

Quoted 44 lines

Signed-off-by Manish Jaggi <[email protected]>

---
  xen/arch/arm/acpi/Makefile        |   1 +
  xen/arch/arm/acpi/ridmap.c        | 126
++++++++++++++++++++++++++++++++++++++
  xen/include/asm-arm/acpi/ridmap.h | 112
+++++++++++++++++++++++++++++++++

  3 files changed, 239 insertions(+)

xen/arch/arm/acpi/Makefile +0 -0

index 23963f8fa0..eb7e8ce4f7 100644
@@ -1,2 +1,3 @@

  obj-y += lib.o
  obj-y += boot.init.o

xen/arch/arm/acpi/Makefile +1 -0

+obj-y += ridmap.o

xen/arch/arm/acpi/ridmap.c new file +19 -0

new file mode 100644
index 0000000000..daa137f625
@@ -0,0 +1,126 @@
+/*
+ * xen/drivers/acpi/arm/ridmap.c
+ *
+ * This file implements rid-sid rid-devid mapping API
+ *
+ * Manish Jaggi <[email protected]>
+ * Copyright (c) 2018 Linaro.
+ *
+ * Ths program is free software; you can redistribute it and/or
+ * modify it under the terms and conditions of the GNU General Public
+ * License, version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <asm/acpi/ridmap.h>

xen/arch/arm/acpi/ridmap.c line 29

Quoted 10 lines

xen/arch/arm/acpi/ridmap.c new file +10 -0

+#include <xen/iommu.h>
+#include <xen/kernel.h>
+#include <xen/list.h>
+#include <xen/pci.h>
+
+LIST_HEAD(rid_sid_list);
+LIST_HEAD(rid_devid_list);
+
+int add_rid_sid_map(struct acpi_iort_node *pcirc_node,
+                    struct acpi_iort_node *smmu_node,

Is there any one that will modify pcirc_node and smmu_node afterwards? If not, then they should be const.

xen/arch/arm/acpi/ridmap.c line 31

Quoted 2 lines

xen/arch/arm/acpi/ridmap.c new file +2 -0

+                    uint32_t input_base, uint32_t output_base,
+                    uint32_t id_count)

I suggested to put __init in front of that function. But you dismissed it saying it might not be valid and you will add rationale. I don't see any rationale in that patch nor an answer to my question on the previous version.

Quoted 10 lines

xen/arch/arm/acpi/ridmap.c new file +10 -0

+{
+    struct rid_sid_map *rid_map;
+
+    rid_map = xzalloc(struct rid_sid_map);
+    if ( !rid_map )
+        return -ENOMEM;
+
+    rid_map->idmap.input_base = input_base;
+    rid_map->idmap.output_base = output_base;
+    rid_map->idmap.id_count = id_count;

xen/arch/arm/acpi/ridmap.c line 51

Quoted 10 lines

xen/arch/arm/acpi/ridmap.c new file +10 -0

+    rid_map->pcirc_node = pcirc_node;
+    rid_map->smmu_node = smmu_node;
+
+    list_add_tail(&rid_map->entry, &rid_sid_list);
+
+    return 0;
+}
+
+int add_rid_devid_map(struct acpi_iort_node *pcirc_node,
+                      struct acpi_iort_node *its_node,

Same here about const.

xen/arch/arm/acpi/ridmap.c line 53

Quoted 2 lines

xen/arch/arm/acpi/ridmap.c new file +2 -0

+                      uint32_t input_base, uint32_t output_base,
+                      uint32_t id_count)

Same here about __init.

Quoted 103 lines

xen/arch/arm/acpi/ridmap.c new file +29 -0

+{
+    struct rid_devid_map *rid_map;
+
+    rid_map = xzalloc(struct rid_devid_map);
+    if ( !rid_map )
+        return -ENOMEM;
+
+    rid_map->idmap.input_base = input_base;
+    rid_map->idmap.output_base = output_base;
+    rid_map->idmap.id_count = id_count;
+    rid_map->pcirc_node = pcirc_node;
+    rid_map->its_node = its_node;
+
+    list_add_tail(&rid_map->entry, &rid_devid_list);
+
+    return 0;
+}
+
+bool query_sid(struct acpi_iort_node *pcirc_node, uint32_t rid,
+               uint32_t *sid, struct acpi_iort_node **smmu_node)
+{
+    struct rid_sid_map *rmap;
+
+    list_for_each_entry(rmap, &rid_sid_list, entry)
+    {
+        if ( rmap->pcirc_node == pcirc_node )
+        {
+            if ( (rid >= rmap->idmap.input_base) &&
+                 (rid < rmap->idmap.input_base + 

rmap->idmap.id_count) )
+            {
+                *sid = rid - rmap->idmap.input_base + +                       rmap->idmap.output_base; +                *smmu_node = rmap->smmu_node;

xen/arch/arm/acpi/ridmap.c new file +20 -0

+
+                return 1;
+            }
+        }
+    }
+
+    return 0;
+}
+
+bool query_devid(struct acpi_iort_node *pcirc_node,
+                uint32_t rid, uint32_t *devid)
+{
+    struct rid_devid_map *rmap;
+
+    list_for_each_entry(rmap, &rid_devid_list, entry)
+    {
+        if ( rmap->pcirc_node == pcirc_node )
+        {
+            if ( (rid >= rmap->idmap.input_base) &&
+                 (rid < rmap->idmap.input_base + 

rmap->idmap.id_count) )
+            {
+                *devid = rid - rmap->idmap.input_base + +                         rmap->idmap.output_base;

xen/arch/arm/acpi/ridmap.c new file +17 -0

+
+                return 1;
+            }
+        }
+    }
+
+    return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */

xen/include/asm-arm/acpi/ridmap.h new file +22 -0

new file mode 100644
index 0000000000..5d12d86c3a
@@ -0,0 +1,112 @@
+/*
+ * xen/include/acpi/ridmap.h
+ *
+ * Manish Jaggi <[email protected]>
+ * Copyright (c) 2018 Linaro.
+ *
+ * Ths program is free software; you can redistribute it and/or
+ * modify it under the terms and conditions of the GNU General Public
+ * License, version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __ASM_ACPI_RIDMAP_H__
+#define __ASM_ACPI_RIDMAP_H__
+
+#include <xen/acpi.h>
+
+/*

xen/include/asm-arm/acpi/ridmap.h line 32

Quoted 10 lines

xen/include/asm-arm/acpi/ridmap.h new file +10 -0

+ * List holds requesterid (rid) - streamid (sid) mapping entries.
+ */
+extern struct list_head rid_sid_list;
+/*
+ * List holds requesterid (rid) - deviceid (devid) mapping entries.
+ */
+extern struct list_head rid_devid_list;
+
+/*
+ * structure to hold mapping between requresterid and streamid.

requesterid.

Quoted 33 lines

xen/include/asm-arm/acpi/ridmap.h new file +5 -0

+ * Note: output_reference and flags members of acpi_iort_id_mapping
+ * are not used. This is done to avoid creating a new structure for
+ * same purpose.
+ *
+ * smmu node pointer is stored in this structure because, in some 

places
+ * smmu_node along with streamid is required based on rid and pcirc_node.
+ */

xen/include/asm-arm/acpi/ridmap.h new file +22 -0

+struct rid_sid_map
+{
+    struct acpi_iort_node *pcirc_node;
+    struct acpi_iort_node *smmu_node;
+    struct acpi_iort_id_mapping idmap;
+    struct list_head entry;
+};
+
+/*
+ * API to add a rid-sid mapping
+ * This method should be called while parsing each entry in idmap array
+ * under the pcirc node in IORT.
+ */
+int add_rid_sid_map(struct acpi_iort_node *pcirc_node,
+                    struct acpi_iort_node *smmu_node,
+                    uint32_t input_base, uint32_t output_base,
+                    uint32_t id_count);
+/*
+ * API to query sid and smmu_node based on pcirc_node and rid.
+ *
+ * Example of usage:
+ *  int iort_pci_iommu_init(struct pci_dev *pdev, u16 alias, void 

*data)
+ *  {

Quoted 6 lines

+ *     struct iort_pci_alias_info *info = data;
+ *   ...
+ *      if ( query_sid(info->node, alias, &streamid, &smmu_node) )
+ *          return iort_iommu_xlate(info->dev, smmu_node, streamid);
+ *   ...
+ *   }

I don't see the benefits of the example usage. If the function is difficult to use, then it much better to describe each argument.

xen/include/asm-arm/acpi/ridmap.h line 75

Quoted 8 lines

+ *
+ */

xen/include/asm-arm/acpi/ridmap.h new file +5 -0

+bool query_sid(struct acpi_iort_node *pcirc_node, uint32_t rid,
+               uint32_t *sid, struct acpi_iort_node **smmu_node);
+
+/*
+ * structure to hold a mapping between requresterid and deviceid.

requesterid

xen/include/asm-arm/acpi/ridmap.h line 86

Quoted 11 lines

+ * Note: output_reference and flags members of acpi_iort_id_mapping
+ * are not used. This is done to avoid creating a new structure for
+ * same purpose.
+ */

xen/include/asm-arm/acpi/ridmap.h new file +7 -0

+struct rid_devid_map
+{
+    struct acpi_iort_node *pcirc_node;
+    struct acpi_iort_node *its_node;
+    struct acpi_iort_id_mapping idmap;
+    struct list_head entry;
+};

You could probably merge this structure and rid_sid_map. The only difference is smmu_node, so you could use an union for that.

xen/include/asm-arm/acpi/ridmap.h line 99

Quoted 13 lines

xen/include/asm-arm/acpi/ridmap.h new file +13 -0

+
+/*
+ * API to add a rid-devid mapping
+ * This method should be called while parsing each entry in idmap array
+ * under the pcirc node in IORT.
+ */
+int add_rid_devid_map(struct acpi_iort_node *pcirc_node,
+                      struct acpi_iort_node *its_node,
+                      uint32_t input_base, uint32_t output_base,
+                      uint32_t id_count);
+
+/*
+ * API to query devid based on pcirc_node and rid */

Comment coding style.

Quoted 4 lines

xen/include/asm-arm/acpi/ridmap.h new file +3 -0

+bool query_devid(struct acpi_iort_node *pcirc_node, uint32_t rid,
+                 uint32_t *devid);
+

xen/include/asm-arm/acpi/ridmap.h line 112

Quoted 10 lines

xen/include/asm-arm/acpi/ridmap.h new file +10 -0

+#endif
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */

Cheers,

Quoted 1 line · Julien Grall

Hi Manish,

Hi Julien,

Quoted 16 lines · On 03/13/2018 03:20 PM, [email protected] wrote:

Quoted 14 lines

From: Manish Jaggi <[email protected]>

IORT has a hierarchical structure containing PCIRC nodes, IORT nodes and SMMU nodes. Each node has with it an array of ids and a mapping which maps a range of ids to another node's ids. PCIRC(requesterid)->SMMU(streamid)->ITS(devid) or PCIRC->ITS

IORT is parsed multiple times when streamid(sid) / deviceid(devid) is queried from requesterid (rid).

Xen needs to prepare IORT for hardware domain which might again require parsing. Thus it is prudent to parse IORT once and save mapping information into individual maps namely rid-sid rid-devid.

Quoted 5 lines

Quoted 3 lines

This patch provides API to add a new mapping and query sid/devid based on rid. Two lists are created rid-sid list, rid-devid list. rid-devid list forms the basis of hardware domains' IORT.

Thank you for updating the commit message. However, you stil don't give an idea often those function will get called and whether unsorted list will be fine.

I have added that in code comments.

Quoted 14 lines

xen/arch/arm/acpi/ridmap.c line 29

Quoted 11 lines

xen/arch/arm/acpi/ridmap.c new file +11 -0

+#include <asm/acpi/ridmap.h>
+#include <xen/iommu.h>
+#include <xen/kernel.h>
+#include <xen/list.h>
+#include <xen/pci.h>
+
+LIST_HEAD(rid_sid_list);
+LIST_HEAD(rid_devid_list);
+
+int add_rid_sid_map(struct acpi_iort_node *pcirc_node,
+                    struct acpi_iort_node *smmu_node,

Is there any one that will modify pcirc_node and smmu_node afterwards? If not, then they should be const.

Quoted 5 lines

Quoted 2 lines

+                    uint32_t input_base, uint32_t output_base, +                    uint32_t id_count)

I suggested to put __init in front of that function. But you dismissed it saying it might not be valid and you will add rationale. I don't see any rationale in that patch nor an answer to my question on the previous version.

Should a public API function be __init ?

Quoted 16 lines

Quoted 16 lines

xen/include/asm-arm/acpi/ridmap.h new file +14 -0

+/*
+ * API to add a rid-sid mapping
+ * This method should be called while parsing each entry in idmap array
+ * under the pcirc node in IORT.
+ */
+int add_rid_sid_map(struct acpi_iort_node *pcirc_node,
+                    struct acpi_iort_node *smmu_node,
+                    uint32_t input_base, uint32_t output_base,
+                    uint32_t id_count);
+/*
+ * API to query sid and smmu_node based on pcirc_node and rid.
+ *
+ * Example of usage:
+ *  int iort_pci_iommu_init(struct pci_dev *pdev, u16 alias, void 

*data)
+ *  {

Quoted 8 lines

Quoted 6 lines

+ *     struct iort_pci_alias_info *info = data;
+ *   ...
+ *      if ( query_sid(info->node, alias, &streamid, &smmu_node) )
+ *          return iort_iommu_xlate(info->dev, smmu_node, streamid);
+ *   ...
+ *   }

I don't see the benefits of the example usage. If the function is difficult to use, then it much better to describe each argument.

It is not difficult, I was citing an example usecase, as you requested in earlier comments, What is the best place to add it ? commit message / code comments.

From: Manish Jaggi <[email protected]>

IORT has a hierarchical structure containing PCIRC nodes, IORT nodes and SMMU nodes. Each node has with it an array of ids and a mapping which maps a range of ids to another node's ids. PCIRC(requesterid)->SMMU(streamid)->ITS(devid) or PCIRC->ITS

IORT is parsed multiple times when streamid(sid) / deviceid(devid) is queried from requesterid (rid).

Xen needs to prepare IORT for hardware domain which might again require parsing. Thus it is prudent to parse IORT once and save mapping information into individual maps namely rid-sid rid-devid.

This patch provides API to add a new mapping and query sid/devid based on rid. Two lists are created rid-sid list, rid-devid list. rid-devid list forms the basis of hardware domains' IORT.

Signed-off-by Manish Jaggi <[email protected]>

---
  xen/arch/arm/acpi/Makefile        |   1 +
  xen/arch/arm/acpi/ridmap.c        | 126 ++++++++++++++++++++++++++++++++++++++
  xen/include/asm-arm/acpi/ridmap.h | 112 +++++++++++++++++++++++++++++++++
  3 files changed, 239 insertions(+)

xen/arch/arm/acpi/Makefile +1 -0

index 23963f8fa0..eb7e8ce4f7 100644
@@ -1,2 +1,3 @@
  obj-y += lib.o
  obj-y += boot.init.o
+obj-y += ridmap.o

xen/arch/arm/acpi/ridmap.c new file +126 -0

new file mode 100644
index 0000000000..daa137f625
@@ -0,0 +1,126 @@
+/*
+ * xen/drivers/acpi/arm/ridmap.c
+ *
+ * This file implements rid-sid rid-devid mapping API
+ *
+ * Manish Jaggi <[email protected]>
+ * Copyright (c) 2018 Linaro.
+ *
+ * Ths program is free software; you can redistribute it and/or
+ * modify it under the terms and conditions of the GNU General Public
+ * License, version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <asm/acpi/ridmap.h>
+#include <xen/iommu.h>
+#include <xen/kernel.h>
+#include <xen/list.h>
+#include <xen/pci.h>
+
+LIST_HEAD(rid_sid_list);
+LIST_HEAD(rid_devid_list);
+
+int add_rid_sid_map(struct acpi_iort_node *pcirc_node,
+                    struct acpi_iort_node *smmu_node,
+                    uint32_t input_base, uint32_t output_base,
+                    uint32_t id_count)
+{
+    struct rid_sid_map *rid_map;
+
+    rid_map = xzalloc(struct rid_sid_map);
+    if ( !rid_map )
+        return -ENOMEM;
+
+    rid_map->idmap.input_base = input_base;
+    rid_map->idmap.output_base = output_base;
+    rid_map->idmap.id_count = id_count;
+    rid_map->pcirc_node = pcirc_node;
+    rid_map->smmu_node = smmu_node;
+
+    list_add_tail(&rid_map->entry, &rid_sid_list);
+
+    return 0;
+}
+
+int add_rid_devid_map(struct acpi_iort_node *pcirc_node,
+                      struct acpi_iort_node *its_node,
+                      uint32_t input_base, uint32_t output_base,
+                      uint32_t id_count)
+{
+    struct rid_devid_map *rid_map;
+
+    rid_map = xzalloc(struct rid_devid_map);
+    if ( !rid_map )
+        return -ENOMEM;
+
+    rid_map->idmap.input_base = input_base;
+    rid_map->idmap.output_base = output_base;
+    rid_map->idmap.id_count = id_count;
+    rid_map->pcirc_node = pcirc_node;
+    rid_map->its_node = its_node;
+
+    list_add_tail(&rid_map->entry, &rid_devid_list);
+
+    return 0;
+}
+
+bool query_sid(struct acpi_iort_node *pcirc_node, uint32_t rid,
+               uint32_t *sid, struct acpi_iort_node **smmu_node)
+{
+    struct rid_sid_map *rmap;
+
+    list_for_each_entry(rmap, &rid_sid_list, entry)
+    {
+        if ( rmap->pcirc_node == pcirc_node )
+        {
+            if ( (rid >= rmap->idmap.input_base) &&
+                 (rid < rmap->idmap.input_base + rmap->idmap.id_count) )
+            {
+                *sid = rid - rmap->idmap.input_base +
+                       rmap->idmap.output_base;
+                *smmu_node = rmap->smmu_node;
+
+                return 1;
+            }
+        }
+    }
+
+    return 0;
+}
+
+bool query_devid(struct acpi_iort_node *pcirc_node,
+                uint32_t rid, uint32_t *devid)
+{
+    struct rid_devid_map *rmap;
+
+    list_for_each_entry(rmap, &rid_devid_list, entry)
+    {
+        if ( rmap->pcirc_node == pcirc_node )
+        {
+            if ( (rid >= rmap->idmap.input_base) &&
+                 (rid < rmap->idmap.input_base + rmap->idmap.id_count) )
+            {
+                *devid = rid - rmap->idmap.input_base +
+                         rmap->idmap.output_base;
+
+                return 1;
+            }
+        }
+    }
+
+    return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */

xen/include/asm-arm/acpi/ridmap.h new file +112 -0

new file mode 100644
index 0000000000..5d12d86c3a
@@ -0,0 +1,112 @@
+/*
+ * xen/include/acpi/ridmap.h
+ *
+ * Manish Jaggi <[email protected]>
+ * Copyright (c) 2018 Linaro.
+ *
+ * Ths program is free software; you can redistribute it and/or
+ * modify it under the terms and conditions of the GNU General Public
+ * License, version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __ASM_ACPI_RIDMAP_H__
+#define __ASM_ACPI_RIDMAP_H__
+
+#include <xen/acpi.h>
+
+/*
+ * List holds requesterid (rid) - streamid (sid) mapping entries.
+ */
+extern struct list_head rid_sid_list;
+/*
+ * List holds requesterid (rid) - deviceid (devid) mapping entries.
+ */
+extern struct list_head rid_devid_list;
+
+/*
+ * structure to hold mapping between requresterid and streamid.
+ * Note: output_reference and flags members of acpi_iort_id_mapping
+ * are not used. This is done to avoid creating a new structure for
+ * same purpose.
+ *
+ * smmu node pointer is stored in this structure because, in some places
+ * smmu_node along with streamid is required based on rid and pcirc_node.
+ */
+struct rid_sid_map
+{
+    struct acpi_iort_node *pcirc_node;
+    struct acpi_iort_node *smmu_node;
+    struct acpi_iort_id_mapping idmap;
+    struct list_head entry;
+};
+
+/*
+ * API to add a rid-sid mapping
+ * This method should be called while parsing each entry in idmap array
+ * under the pcirc node in IORT.
+ */
+int add_rid_sid_map(struct acpi_iort_node *pcirc_node,
+                    struct acpi_iort_node *smmu_node,
+                    uint32_t input_base, uint32_t output_base,
+                    uint32_t id_count);
+/*
+ * API to query sid and smmu_node based on pcirc_node and rid.
+ *
+ * Example of usage:
+ *  int iort_pci_iommu_init(struct pci_dev *pdev, u16 alias, void *data)
+ *  {
+ *     struct iort_pci_alias_info *info = data;
+ *   ...
+ *      if ( query_sid(info->node, alias, &streamid, &smmu_node) )
+ *          return iort_iommu_xlate(info->dev, smmu_node, streamid);
+ *   ...
+ *   }
+ *
+ */
+bool query_sid(struct acpi_iort_node *pcirc_node, uint32_t rid,
+               uint32_t *sid, struct acpi_iort_node **smmu_node);
+
+/*
+ * structure to hold a mapping between requresterid and deviceid.
+ * Note: output_reference and flags members of acpi_iort_id_mapping
+ * are not used. This is done to avoid creating a new structure for
+ * same purpose.
+ */
+struct rid_devid_map
+{
+    struct acpi_iort_node *pcirc_node;
+    struct acpi_iort_node *its_node;
+    struct acpi_iort_id_mapping idmap;
+    struct list_head entry;
+};
+
+/*
+ * API to add a rid-devid mapping
+ * This method should be called while parsing each entry in idmap array
+ * under the pcirc node in IORT.
+ */
+int add_rid_devid_map(struct acpi_iort_node *pcirc_node,
+                      struct acpi_iort_node *its_node,
+                      uint32_t input_base, uint32_t output_base,
+                      uint32_t id_count);
+
+/*
+ * API to query devid based on pcirc_node and rid */
+bool query_devid(struct acpi_iort_node *pcirc_node, uint32_t rid,
+                 uint32_t *devid);
+
+#endif
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */

From: Manish Jaggi <[email protected]>

Code to query estimated IORT size for hardware domain. IORT for hardware domain is generated using the requesterid and deviceid map. Xen code requires the size to be predeterminded.

Signed-off-by Manish Jaggi <[email protected]>

---
  xen/arch/arm/acpi/Makefile          |   1 +
  xen/arch/arm/acpi/gen-iort.c        | 101 ++++++++++++++++++++++++++++++++++++
  xen/arch/arm/domain_build.c         |  16 ++++--
  xen/include/asm-arm/acpi/gen-iort.h |  33 ++++++++++++
  4 files changed, 148 insertions(+), 3 deletions(-)

xen/arch/arm/acpi/Makefile +1 -0

index eb7e8ce4f7..073339603c 100644
@@ -1,3 +1,4 @@
  obj-y += lib.o
  obj-y += boot.init.o
  obj-y += ridmap.o
+obj-y += gen-iort.o

xen/arch/arm/acpi/gen-iort.c new file +0 -0

new file mode 100644
index 0000000000..687c4f18ee

xen/arch/arm/acpi/gen-iort.c new file +4 -0

+ * Copyright (c) 2018 Linaro.
+ *
+ * Ths program is free software; you can redistribute it and/or
+ * modify it under the terms and conditions of the GNU General Public

xen/arch/arm/acpi/gen-iort.c new file +17 -0

+
+        for ( i = 0; i <= pcirc_count; i++ )
+        {
+            if ( pcirc_array[i] == (uint64_t) rmap->pcirc_node )
+                break;
+            if ( i == pcirc_count )
+            {
+                pcirc_array[i] = (uint64_t) rmap->pcirc_node;
+                pcirc_count++;
+                break;
+            }
+        }
+
+        for ( i = 0; i <= itsg_count; i++ )
+        {
+            if ( itsg_array[i] == (uint64_t) rmap->its_node )
+                break;

xen/arch/arm/acpi/gen-iort.c new file +14 -0

+
+    xfree(itsg_array);
+    xfree(pcirc_array);
+
+    return 0;
+}
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */

xen/arch/arm/domain_build.c +1 -0

index 155c952349..33a46cab1e 100644
@@ -14,6 +14,7 @@
  #include <xen/acpi.h>
  #include <xen/warning.h>
  #include <acpi/actables.h>
+#include <asm/acpi/gen-iort.h>
  #include <asm/device.h>
  #include <asm/setup.h>
  #include <asm/platform.h>

xen/include/asm-arm/acpi/gen-iort.h new file +23 -0

@@ -0,0 +1,33 @@
+/*
+ * xen/include/asm-arm/acpi/gen-iort.h
+ *
+ * Manish Jaggi <[email protected]>
+ * Copyright (c) 2018 Linaro.
+ *
+ * Ths program is free software; you can redistribute it and/or
+ * modify it under the terms and conditions of the GNU General Public
+ * License, version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef _ACPI_GEN_IORT_H
+#define _ACPI_GEN_IORT_H
+
+/*
+ * Returns the size of hardware domains IORT
+ */
+int estimate_iort_size(size_t *iort_size);

Title: Please drop the full stop.

On 03/13/2018 03:20 PM, [email protected] wrote:
...

Quoted 1 line

+    struct rid_devid_map *rmap;

I am sorry but I still don't see any comment about my comment on the previous version. For reminder:

"A bit more documention of this function would be appreciated. For instance, the rationale between browsing the list twice for allocation.

I actually do think this might be avoidable by storing a bit more information from the IORT. From the table you can easily deduced the number of root complex and ITS group.

They could be store with the rest of information."

From: Manish Jaggi <[email protected]>

Structure of Hardware domain's (hwdom) IORT

hwdom's IORT will only have PCIRC nodes and ITS group nodes in the following order. SMMU nodes as they are hidden from hardware domain.

[IORT Header]
[ITS Group 1 ]
...
[ITS Group n ]
[PCIRC Node 1]
[PCIRC IDMAP entry 1]
...
[PCIRC IDMAP entry m]
...
[PCIRC Node p]
[PCIRC IDMAP entry 1]
...
[PCIRC IDMAP entry q]
...
*n,m,p are variable.

requesterid-deviceid mapping list (rid_devid_list) populated by parsing IORT is used to generate hwdom IORT.

As the rid_devid_list is populated from firmware IORT, IDMAP entry would have output references offsets based on firmware's IORT. It is required to fixup node offset of ITS Group Nodes in the PCIRC idmap (output_reference)

First write all the ITS group nodes in the hwdom's IORT. For this write_hwits_nodes is called, which parses the rid_devid_list and for each unique its_node in firmware IORT create a its_node in hwdom's IORT and also creates and entry in fwits_hwits_map.

fwits_hwits_map is a mapping between firmware IORT's its node and the node offset of the corresponding its_node stored in the hwdom's IORT.

This map can later be used to set output reference value in hwdom's pcirc node's idmap entries.

Signed-off-by Manish Jaggi <[email protected]>

---
  xen/arch/arm/acpi/gen-iort.c        | 299 ++++++++++++++++++++++++++++++++++++
  xen/arch/arm/domain_build.c         |  35 +++++
  xen/include/asm-arm/acpi.h          |   1 +
  xen/include/asm-arm/acpi/gen-iort.h |  11 ++
  4 files changed, 346 insertions(+)

xen/arch/arm/acpi/gen-iort.c +14 -0

index 687c4f18ee..251a9771e3 100644
@@ -19,6 +19,305 @@
 
  #include <asm/acpi/ridmap.h>
  #include <xen/acpi.h>
+#include <acpi/actables.h>
+
+/*
+ * Structure of Hardware domain's (hwdom) IORT
+ * -----------------------------------
+ *
+ * hwdom's IORT will only have PCIRC nodes and ITS group nodes
+ * in the following order.
+ *
+ * [IORT Header]
+ * [ITS Group 1 ]
+ * ...
+ * [ITS Group N ]
+ * [PCIRC Node 1]

xen/arch/arm/acpi/gen-iort.c +21 -0

+ * in the PCIRC idmap (output_reference)
+ *
+ * In rid_devid_map firmware IORT's ITS group node pointer in stored.
+ *
+ * We first write all the ITS group nodes in the hwdom's IORT. For this
+ * write_hwits_nodes is called, which parses the rid_devid_list and for
+ * each unique its_node in firmware IORT create a its_node in hwdom's IORT
+ * and also creates and entry in fwits_hwits_map.
+ *
+ * fwits_hwits_map is a mapping between firmware IORT's its node
+ * and the node offset of the corresponding its_node stored in the
+ * hwdom's IORT.
+ *
+ * This map can be later used to set output reference value in hwdom's
+ * pcirc node's idmap entries.
+ *
+ */
+
+/*
+ * Stores the mapping between firmware tables its group node
+ * to the offset of the equivalent its node to be stored in

xen/arch/arm/acpi/gen-iort.c +19 -0

+    if ( !map )
+        return -ENOMEM;
+
+    map->fwits_node = fwits_node;
+    map->hwitsnode_offset = offset;
+    list_add_tail(&map->entry, &fwits_hwits_list);
+
+    return 1;
+}
+
+/*
+ * Returns the offset of corresponding its node to fwits_node
+ * written in hwdom's IORT.
+ *
+ * This function would be used when write hwdoms pcirc nodes' idmap
+ * entries.
+ */
+static
+unsigned int hwitsnode_offset_from_map(struct acpi_iort_node *fwits_node)

xen/arch/arm/acpi/gen-iort.c +5 -0

+    int n = 0;
+
+    /*
+     * rid_devid_list is iterated to get unique its group nodes
+     * Each unique ITS group node is written in hardware domains IORT

xen/arch/arm/acpi/gen-iort.c +16 -0

+
+            of += node->length;
+            n++;
+        }
+    }
+    *offset = of;
+    *num_nodes = n;
+}
+
+static void write_hwpcirc_nodes(u8 *iort, unsigned int *pos,
+                                unsigned int *num_nodes)
+{
+    struct acpi_iort_node *opcirc_node, *pcirc_node;
+    struct acpi_iort_node *hwdom_pcirc_node = NULL;
+    struct rid_devid_map *rmap;
+    struct acpi_iort_id_mapping *idmap;

Hi Manish,

Quoted 81 lines · On 03/13/2018 03:20 PM, [email protected] wrote:

From: Manish Jaggi <[email protected]>

Structure of Hardware domain's (hwdom) IORT

hwdom's IORT will only have PCIRC nodes and ITS group nodes in the following order. SMMU nodes as they are hidden from hardware domain.

[IORT Header]
[ITS Group 1 ]
...
[ITS Group n ]
[PCIRC Node 1]
   [PCIRC IDMAP entry 1]
   ...
   [PCIRC IDMAP entry m]
...
[PCIRC Node p]
   [PCIRC IDMAP entry 1]
   ...
   [PCIRC IDMAP entry q]
...
*n,m,p are variable.

requesterid-deviceid mapping list (rid_devid_list) populated by parsing IORT is used to generate hwdom IORT.

As the rid_devid_list is populated from firmware IORT, IDMAP entry would have output references offsets based on firmware's IORT. It is required to fixup node offset of ITS Group Nodes in the PCIRC idmap (output_reference)

First write all the ITS group nodes in the hwdom's IORT. For this write_hwits_nodes is called, which parses the rid_devid_list and for each unique its_node in firmware IORT create a its_node in hwdom's IORT and also creates and entry in fwits_hwits_map.

fwits_hwits_map is a mapping between firmware IORT's its node and the node offset of the corresponding its_node stored in the hwdom's IORT.

This map can later be used to set output reference value in hwdom's pcirc node's idmap entries.

Signed-off-by Manish Jaggi <[email protected]>

---
  xen/arch/arm/acpi/gen-iort.c        | 299
++++++++++++++++++++++++++++++++++++
  xen/arch/arm/domain_build.c         |  35 +++++
  xen/include/asm-arm/acpi.h          |   1 +
  xen/include/asm-arm/acpi/gen-iort.h |  11 ++

  4 files changed, 346 insertions(+)

xen/arch/arm/acpi/gen-iort.c +21 -0

index 687c4f18ee..251a9771e3 100644
@@ -19,6 +19,305 @@
     #include <asm/acpi/ridmap.h>
   #include <xen/acpi.h>
+#include <acpi/actables.h>
+
+/*
+ * Structure of Hardware domain's (hwdom) IORT
+ * -----------------------------------
+ *
+ * hwdom's IORT will only have PCIRC nodes and ITS group nodes
+ * in the following order.
+ *
+ * [IORT Header]
+ * [ITS Group 1 ]
+ * ...
+ * [ITS Group N ]
+ * [PCIRC Node 1]
+ * [PCIRC IDMAP entry 1]
+ * ...
+ * [PCIRC IDMAP entry N]
+ * ...
+ * [PCIRC Node N]
+ *
+ * requesterid-deviceid mapping list (rid_devid_list) populated by 

Quoted 4 lines

parsing IORT
+ * is used to generate hwdom IORT.
+ *
+ * One of the challanges is to fixup node offset of ITS Group Nodes

s/challanges/challenges/

Quoted 22 lines

+ * in the PCIRC idmap (output_reference)
+ *
+ * In rid_devid_map firmware IORT's ITS group node pointer in stored.
+ *
+ * We first write all the ITS group nodes in the hwdom's IORT. For this
+ * write_hwits_nodes is called, which parses the rid_devid_list and for
+ * each unique its_node in firmware IORT create a its_node in hwdom's IORT
+ * and also creates and entry in fwits_hwits_map.
+ *
+ * fwits_hwits_map is a mapping between firmware IORT's its node
+ * and the node offset of the corresponding its_node stored in the
+ * hwdom's IORT.
+ *
+ * This map can be later used to set output reference value in hwdom's
+ * pcirc node's idmap entries.
+ *
+ */

xen/arch/arm/acpi/gen-iort.c +4 -0

+
+/*
+ * Stores the mapping between firmware tables its group node
+ * to the offset of the equivalent its node to be stored in

xen/arch/arm/acpi/gen-iort.c line 76

Quoted 10 lines

xen/arch/arm/acpi/gen-iort.c +10 -0

+ * hwdom's IORT.
+ */
+struct fwits_hwits_map
+{
+    struct acpi_iort_node *fwits_node;
+    unsigned int hwitsnode_offset;
+    struct list_head entry;
+};
+
+LIST_HEAD(fwits_hwits_list);

As said in the previous version, I think this should be static.

xen/arch/arm/acpi/gen-iort.c line 82

Quoted 6 lines

xen/arch/arm/acpi/gen-iort.c +6 -0

+
+/*
+ * is_uniq_fwits_node
+ *
+ * returns 1 - if fwits_node is not already in the its_map_list
+ *         0 - if it is present already

It also returns -ENOMEM when you can't allocate memory.

Quoted 6 lines

+ *
+ * fwits_node - ITS Node pointer in Firmware IORT
+ * offset     - offset of the equivalent its node to be stored in
+ *              hwdom's IORT
+ */
+static int is_uniq_fwits_node(struct acpi_iort_node *fwits_node,

The name is a bit odd given that you add the ITS node. On the previous version, I requested to document that behavior...

But you likely want to rename the function to add_fwits_node(...) or something similar.

xen/arch/arm/acpi/gen-iort.c line 99

Quoted 12 lines

+                              unsigned int offset)

xen/arch/arm/acpi/gen-iort.c +10 -0

+{
+    struct fwits_hwits_map *map;
+
+    list_for_each_entry(map, &fwits_hwits_list, entry)
+    {
+        if ( map->fwits_node == fwits_node )
+            return 0;
+    }
+
+    map = xzalloc(struct fwits_hwits_map);

Where this memory is going to be freed?

Quoted 24 lines

+    if ( !map )
+        return -ENOMEM;

xen/arch/arm/acpi/gen-iort.c +17 -0

+
+    map->fwits_node = fwits_node;
+    map->hwitsnode_offset = offset;
+    list_add_tail(&map->entry, &fwits_hwits_list);
+
+    return 1;
+}
+
+/*
+ * Returns the offset of corresponding its node to fwits_node
+ * written in hwdom's IORT.
+ *
+ * This function would be used when write hwdoms pcirc nodes' idmap
+ * entries.
+ */
+static
+unsigned int hwitsnode_offset_from_map(struct acpi_iort_node 

*fwits_node)

xen/arch/arm/acpi/gen-iort.c +4 -0

+{
+    struct fwits_hwits_map *map;
+
+    list_for_each_entry(map, &fwits_hwits_list, entry)

Quoted 6 lines

+    {
+        if ( map->fwits_node == fwits_node )
+            return map->hwitsnode_offset;
+    }
+
+    return 0;

0 could never be a valid offset, right?

xen/arch/arm/acpi/gen-iort.c line 131

Quoted 4 lines

xen/arch/arm/acpi/gen-iort.c +3 -0

+}
+
+static void write_hwits_nodes(u8 *iort, unsigned int *offset,

Please avoid using u* and use uint_* instead. I expect that you fix all of for the next version.

Here, I think you want to use void *.

Quoted 4 lines

+                              unsigned int *num_nodes) +{
+    struct rid_devid_map *rmap;
+    unsigned int of = *offset;

Please name it off. This is clearer that it is an offset. But as I said on the previous version, why can't you just re-use offset?

Quoted 6 lines

+    int n = 0;

xen/arch/arm/acpi/gen-iort.c +4 -0

+
+    /*
+     * rid_devid_list is iterated to get unique its group nodes
+     * Each unique ITS group node is written in hardware domains IORT

xen/arch/arm/acpi/gen-iort.c line 150

Quoted 10 lines

xen/arch/arm/acpi/gen-iort.c +10 -0

+     * by using some values from the firmware ITS group node.
+     */
+    list_for_each_entry(rmap, &rid_devid_list, entry)
+    {
+        struct acpi_iort_node *node;
+        struct acpi_iort_its_group *grp;
+        struct acpi_iort_its_group *fw_grp;
+
+        /* save its_node_offset_map in a list uniquely */
+        if ( is_uniq_fwits_node(rmap->its_node, of) == 1 )

If the function is returning -ENOMEM, then you will ignore the node without a warning. That's going to be a real pain to find out a ITS node is not present if that happen.

Here, you should propagate error if something wrong is going.

Quoted 9 lines

+        {
+            node = (struct acpi_iort_node *) &iort[of]; +            grp = (struct acpi_iort_its_group *)(&node->node_data); +
+            node->type = ACPI_IORT_NODE_ITS_GROUP;
+            node->length = sizeof(struct acpi_iort_node) + +                           sizeof(struct acpi_iort_its_group) - +                           sizeof(node->node_data);

While the substraction is good, this is odd enough to warrant a comment. But likely But likely you want to provide macros for defining the length. This will clean a lot the code.

Quoted 2 lines

+
+            node->revision = rmap->its_node->revision;

I am not sure this is right. You rewrite the IORT based on a given revision. Imagine the host IORT get updated to a newer spec but not Xen.

Then you would end up to have the wrong revision number here.

Quoted 11 lines

+            node->reserved = 0;
+            node->mapping_count = 0;
+            node->mapping_offset= 0;
+
+            fw_grp = (struct acpi_iort_its_group *)(&rmap->its_node->node_data);
+
+            /* Copy its_count and identifiers from firmware iort's its_node */
+            grp->its_count = fw_grp->its_count;
+            grp->identifiers[0] = fw_grp->identifiers[0];

Hmmm, here you will only copy the first identifier. What if you have multiple one?

It would also be good that somewhere (maybe at the top of the file) that you rely on the number of ITS and identifiers for the hwdom is the same as the host.

Quoted 16 lines

xen/arch/arm/acpi/gen-iort.c +16 -0

+
+            of += node->length;
+            n++;
+        }
+    }
+    *offset = of;
+    *num_nodes = n;
+}
+
+static void write_hwpcirc_nodes(u8 *iort, unsigned int *pos,
+                                unsigned int *num_nodes)
+{
+    struct acpi_iort_node *opcirc_node, *pcirc_node;
+    struct acpi_iort_node *hwdom_pcirc_node = NULL;
+    struct rid_devid_map *rmap;
+    struct acpi_iort_id_mapping *idmap;

xen/arch/arm/acpi/gen-iort.c line 195

Quoted 10 lines

xen/arch/arm/acpi/gen-iort.c +10 -0

+    int num_idmap = 0, n = 0;
+    unsigned int old_pos = *pos;
+
+    opcirc_node = NULL;
+    /* Iterate rid_map_devid list */
+    list_for_each_entry(rmap, &rid_devid_list, entry)
+    {
+        struct acpi_iort_root_complex *rc;
+        struct acpi_iort_root_complex *rc_fw;
+        int add_node = 0;

This should be bool.

I am going to stop the review here because I feel I am just repeating all my comments from the first version and new ones. So please go through *all* my e-mails from the previous version, answer to my question, and respin it.

Cheers,

Quoted 8 lines · Julien Grall

Quoted 6 lines · On 03/13/2018 03:20 PM, [email protected] wrote:

+ *
+ * fwits_node - ITS Node pointer in Firmware IORT
+ * offset     - offset of the equivalent its node to be stored in
+ *              hwdom's IORT
+ */
+static int is_uniq_fwits_node(struct acpi_iort_node *fwits_node,

Quoted 1 line

The name is a bit odd given that you add the ITS node. On the previous version, I requested to document that behavior...

I think the name is quite appropriate. Also in this patch I have added description of the flow so this should be fairly intuitive. Could you please let me know the specific point you dont understand, I can explain that.

Quoted 2 lines

But you likely want to rename the function to add_fwits_node(...) or something similar.

I think name is quite appropriate.

Quoted 15 lines

xen/arch/arm/acpi/gen-iort.c line 99

Quoted 12 lines

+                              unsigned int offset)

xen/arch/arm/acpi/gen-iort.c +10 -0

+{
+    struct fwits_hwits_map *map;
+
+    list_for_each_entry(map, &fwits_hwits_list, entry)
+    {
+        if ( map->fwits_node == fwits_node )
+            return 0;
+    }
+
+    map = xzalloc(struct fwits_hwits_map);

Where this memory is going to be freed?

Since this list can be used multiple times even after creation of IORT for dom0, say thinking ahead for domUs.

Quoted 24 lines

Quoted 24 lines

+    if ( !map )
+        return -ENOMEM;

xen/arch/arm/acpi/gen-iort.c +17 -0

+
+    map->fwits_node = fwits_node;
+    map->hwitsnode_offset = offset;
+    list_add_tail(&map->entry, &fwits_hwits_list);
+
+    return 1;
+}
+
+/*
+ * Returns the offset of corresponding its node to fwits_node
+ * written in hwdom's IORT.
+ *
+ * This function would be used when write hwdoms pcirc nodes' idmap
+ * entries.
+ */
+static
+unsigned int hwitsnode_offset_from_map(struct acpi_iort_node 

*fwits_node)

xen/arch/arm/acpi/gen-iort.c +4 -0

+{
+    struct fwits_hwits_map *map;
+
+    list_for_each_entry(map, &fwits_hwits_list, entry)

Quoted 8 lines

Quoted 6 lines

+    {
+        if ( map->fwits_node == fwits_node )
+            return map->hwitsnode_offset;
+    }
+
+    return 0;

0 could never be a valid offset, right?

Yes
See a bug_on when it is used.

Quoted 9 lines

xen/arch/arm/acpi/gen-iort.c line 131

Quoted 4 lines

xen/arch/arm/acpi/gen-iort.c +3 -0

+}
+
+static void write_hwits_nodes(u8 *iort, unsigned int *offset,

Please avoid using u* and use uint_* instead. I expect that you fix all of for the next version.

Here, I think you want to use void *.

Quoted 6 lines

Quoted 4 lines

+                              unsigned int *num_nodes) +{
+    struct rid_devid_map *rmap;
+    unsigned int of = *offset;

Please name it off. This is clearer that it is an offset. But as I said on the previous version, why can't you just re-use offset?

I will change it to off, will not break anything.

Quoted 19 lines

Quoted 6 lines

+    int n = 0;

xen/arch/arm/acpi/gen-iort.c +4 -0

+
+    /*
+     * rid_devid_list is iterated to get unique its group nodes
+     * Each unique ITS group node is written in hardware domains IORT

xen/arch/arm/acpi/gen-iort.c line 150

Quoted 10 lines

xen/arch/arm/acpi/gen-iort.c +10 -0

+     * by using some values from the firmware ITS group node.
+     */
+    list_for_each_entry(rmap, &rid_devid_list, entry)
+    {
+        struct acpi_iort_node *node;
+        struct acpi_iort_its_group *grp;
+        struct acpi_iort_its_group *fw_grp;
+
+        /* save its_node_offset_map in a list uniquely */
+        if ( is_uniq_fwits_node(rmap->its_node, of) == 1 )

If the function is returning -ENOMEM, then you will ignore the node without a warning. That's going to be a real pain to find out a ITS node is not present if that happen.

Quoted 1 line

Here, you should propagate error if something wrong is going.

ok.

Quoted 10 lines

Quoted 9 lines

+        {
+            node = (struct acpi_iort_node *) &iort[of]; +            grp = (struct acpi_iort_its_group *)(&node->node_data); +
+            node->type = ACPI_IORT_NODE_ITS_GROUP;
+            node->length = sizeof(struct acpi_iort_node) + +                           sizeof(struct acpi_iort_its_group) - +                           sizeof(node->node_data);

Quoted 6 lines

While the substraction is good, this is odd enough to warrant a comment. But likely But likely you want to provide macros for defining the length. This will clean a lot the code.

Quoted 2 lines

+
+            node->revision = rmap->its_node->revision;

I am not sure this is right. You rewrite the IORT based on a given revision. Imagine the host IORT get updated to a newer spec but not Xen.

Not sure if I follow your comment here.
Xen gets host IORT from firmware, how will it get updated?

Quoted 29 lines

Quoted 16 lines

xen/arch/arm/acpi/gen-iort.c +16 -0

+
+            of += node->length;
+            n++;
+        }
+    }
+    *offset = of;
+    *num_nodes = n;
+}
+
+static void write_hwpcirc_nodes(u8 *iort, unsigned int *pos,
+                                unsigned int *num_nodes)
+{
+    struct acpi_iort_node *opcirc_node, *pcirc_node;
+    struct acpi_iort_node *hwdom_pcirc_node = NULL;
+    struct rid_devid_map *rmap;
+    struct acpi_iort_id_mapping *idmap;

xen/arch/arm/acpi/gen-iort.c line 195

Quoted 10 lines

xen/arch/arm/acpi/gen-iort.c +10 -0

+    int num_idmap = 0, n = 0;
+    unsigned int old_pos = *pos;
+
+    opcirc_node = NULL;
+    /* Iterate rid_map_devid list */
+    list_for_each_entry(rmap, &rid_devid_list, entry)
+    {
+        struct acpi_iort_root_complex *rc;
+        struct acpi_iort_root_complex *rc_fw;
+        int add_node = 0;

This should be bool.

Quoted 2 lines

I am going to stop the review here because I feel I am just repeating all my comments from the first version and new ones. So please go through *all* my e-mails from the previous version, answer to my question, and respin it.

Could you please have a look at other patches and if there is functionality related change that I need to make, i can add that in my next rev.