[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH resend 01/13] acpi: arm: API: Populate/query rid-devid rid-sid map.




On 03/21/2018 02:59 PM, Julien Grall wrote:
Hi Manish,
Hi Julien,

On 03/13/2018 03:20 PM, mjaggi@xxxxxxxxxxxxxxxxxx wrote:
From: Manish Jaggi <mjaggi@xxxxxxxxxxxxxxxxxx>

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.

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.


Signed-off-by: Manish Jaggi <manish.jaggi@xxxxxxxxxx>
---
  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(+)

diff --git a/xen/arch/arm/acpi/Makefile b/xen/arch/arm/acpi/Makefile
index 23963f8fa0..eb7e8ce4f7 100644
--- a/xen/arch/arm/acpi/Makefile
+++ b/xen/arch/arm/acpi/Makefile
@@ -1,2 +1,3 @@
  obj-y += lib.o
  obj-y += boot.init.o
+obj-y += ridmap.o
diff --git a/xen/arch/arm/acpi/ridmap.c b/xen/arch/arm/acpi/ridmap.c
new file mode 100644
index 0000000000..daa137f625
--- /dev/null
+++ b/xen/arch/arm/acpi/ridmap.c
@@ -0,0 +1,126 @@
+/*
+ * xen/drivers/acpi/arm/ridmap.c
+ *
+ * This file implements rid-sid rid-devid mapping API
+ *
+ * Manish Jaggi <manish.jaggi@xxxxxxxxxx>
+ * 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,

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

+                    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 ?
+{
+    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,

Same here about const.

+                      uint32_t input_base, uint32_t output_base,
+                      uint32_t id_count)

Same here about __init.

+{
+    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:
+ */
diff --git a/xen/include/asm-arm/acpi/ridmap.h b/xen/include/asm-arm/acpi/ridmap.h
new file mode 100644
index 0000000000..5d12d86c3a
--- /dev/null
+++ b/xen/include/asm-arm/acpi/ridmap.h
@@ -0,0 +1,112 @@
+/*
+ * xen/include/acpi/ridmap.h
+ *
+ * Manish Jaggi <manish.jaggi@xxxxxxxxxx>
+ * 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.

requesterid.

+ * 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);
+ *   ...
+ *   }

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.

+ *
+ */
+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

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

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

+
+/*
+ * 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.

+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:
+ */


Cheers,



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.