* [PATCH resend 00/13] acpi: arm: Add IORT Support
@ 2018-03-13 15:20 mjaggi
2018-03-13 15:20 ` [PATCH resend 01/13] acpi: arm: API: Populate/query rid-devid rid-sid map mjaggi
` (12 more replies)
0 siblings, 13 replies; 23+ messages in thread
From: mjaggi @ 2018-03-13 15:20 UTC (permalink / raw)
To: manish.jaggi, julien.grall, xen-devel, sameer.goel, jgross, sstabellini
Cc: Manish Jaggi, manish.jaggi
From: Manish Jaggi <mjaggi@caviumnetworks.com>
This patch aims to add the support of IORT in Xen. Below is the list
of major components which this patchset provides.
a. Add support for parsing the IORT
b. Provides API to populate/query requesterid - streamID mappings and
reuqesterid - deviceid mappings
c. The requesterid - deviceid mappings is used to create the IORT for
hardware domain (which hides smmu nodes from IORT)
d. iort.c fwnode.h fwspec code is imported from linux and modified.
Changes since RFC
- Added more documentation
- Moved code to arch/arm/acpi/ folder
- Lot of fixes for review comments
This patch works with [1] but few modifiations were made
(a) path of acpi_iort.h has changed so fixed to asm/acpi/acpi_iort.h
(b) Macro #define alloc_io_pgtable_ops(f, c, o) in Patch 5 of [1] results in
-ENOMEM and need to be fixed.
-[1] https://lists.xenproject.org/archives/html/xen-devel/2018-02/msg00713.html
Manish Jaggi (13):
acpi: arm: API: Populate/query rid-devid rid-sid map.
acpi: arm: query estimated size of hardware domain's IORT.
acpi: arm: Code to generate Hardware Domains IORT
acpi: arm: Copy fwnode / iommu_fwspec code from Linux 4.14
acpi: arm: Import acpi_iort.h verbatim from linux 4.14
acpi: arm: Update acpi_iort.h with xen specific changes
arm: Adding ACPI_IORT in arm Kconfig
asm: arm: pci: Fix the #include label in asm-arm/pci.h
asm: arm: to_pci_dev
asm: arm: add dev_is_pci
asm: arm: add pci_domain_nr
acpi: arm: Provide support for iort iommu configuration hooks
acpi: arm: Add code to parse IORT and prepare rid maps.
xen/arch/arm/Kconfig | 4 +
xen/arch/arm/acpi/Makefile | 3 +
xen/arch/arm/acpi/gen-iort.c | 400 +++++++++++++++++++++++
xen/arch/arm/acpi/iort.c | 608 +++++++++++++++++++++++++++++++++++
xen/arch/arm/acpi/ridmap.c | 126 ++++++++
xen/arch/arm/domain_build.c | 51 ++-
xen/drivers/passthrough/arm/iommu.c | 85 +++++
xen/drivers/passthrough/arm/smmu.c | 3 +-
xen/include/asm-arm/acpi.h | 1 +
xen/include/asm-arm/acpi/acpi_iort.h | 51 +++
xen/include/asm-arm/acpi/gen-iort.h | 44 +++
xen/include/asm-arm/acpi/ridmap.h | 112 +++++++
xen/include/asm-arm/device.h | 17 +-
xen/include/asm-arm/fwnode.h | 128 ++++++++
xen/include/asm-arm/fwspec.h | 38 +++
xen/include/asm-arm/pci.h | 12 +-
xen/include/xen/pci.h | 2 +
17 files changed, 1675 insertions(+), 10 deletions(-)
create mode 100644 xen/arch/arm/acpi/gen-iort.c
create mode 100644 xen/arch/arm/acpi/iort.c
create mode 100644 xen/arch/arm/acpi/ridmap.c
create mode 100644 xen/include/asm-arm/acpi/acpi_iort.h
create mode 100644 xen/include/asm-arm/acpi/gen-iort.h
create mode 100644 xen/include/asm-arm/acpi/ridmap.h
create mode 100644 xen/include/asm-arm/fwnode.h
create mode 100644 xen/include/asm-arm/fwspec.h
--
2.14.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH resend 01/13] acpi: arm: API: Populate/query rid-devid rid-sid map.
2018-03-13 15:20 [PATCH resend 00/13] acpi: arm: Add IORT Support mjaggi
@ 2018-03-13 15:20 ` mjaggi
2018-03-21 9:29 ` Julien Grall
2018-03-21 9:29 ` Julien Grall
2018-03-13 15:20 ` [PATCH resend 02/13] acpi: arm: query estimated size of hardware domain's IORT mjaggi
` (11 subsequent siblings)
12 siblings, 2 replies; 23+ messages in thread
From: mjaggi @ 2018-03-13 15:20 UTC (permalink / raw)
To: manish.jaggi, julien.grall, xen-devel, sameer.goel, jgross, sstabellini
Cc: Manish Jaggi, manish.jaggi
From: Manish Jaggi <mjaggi@caviumnetworks.com>
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 <manish.jaggi@linaro.org>
---
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@linaro.org>
+ * 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:
+ */
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@linaro.org>
+ * 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:
+ */
--
2.14.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH resend 02/13] acpi: arm: query estimated size of hardware domain's IORT.
2018-03-13 15:20 [PATCH resend 00/13] acpi: arm: Add IORT Support mjaggi
2018-03-13 15:20 ` [PATCH resend 01/13] acpi: arm: API: Populate/query rid-devid rid-sid map mjaggi
@ 2018-03-13 15:20 ` mjaggi
2018-03-21 10:12 ` Julien Grall
2018-03-13 15:20 ` [PATCH resend 03/13] acpi: arm: Code to generate Hardware Domains IORT mjaggi
` (10 subsequent siblings)
12 siblings, 1 reply; 23+ messages in thread
From: mjaggi @ 2018-03-13 15:20 UTC (permalink / raw)
To: manish.jaggi, julien.grall, xen-devel, sameer.goel, jgross, sstabellini
Cc: Manish Jaggi, Manish Jaggi, manish.jaggi
From: Manish Jaggi <mjaggi@caviumnetworks.com>
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 <manish.jaggi@linaro.com>
---
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(-)
diff --git a/xen/arch/arm/acpi/Makefile b/xen/arch/arm/acpi/Makefile
index eb7e8ce4f7..073339603c 100644
--- a/xen/arch/arm/acpi/Makefile
+++ b/xen/arch/arm/acpi/Makefile
@@ -1,3 +1,4 @@
obj-y += lib.o
obj-y += boot.init.o
obj-y += ridmap.o
+obj-y += gen-iort.o
diff --git a/xen/arch/arm/acpi/gen-iort.c b/xen/arch/arm/acpi/gen-iort.c
new file mode 100644
index 0000000000..687c4f18ee
--- /dev/null
+++ b/xen/arch/arm/acpi/gen-iort.c
@@ -0,0 +1,101 @@
+/*
+ * xen/arch/arm/acpi/gen-iort.c
+ *
+ * Code to generate IORT for hardware domain using the requesterId
+ * and deviceId map.
+ *
+ * Manish Jaggi <manish.jaggi@linaro.com>
+ * 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/acpi.h>
+
+/*
+ * Size of hardware domains' IORT is calculated based on the number of
+ * mappings in the requesterid - deviceid mapping list.
+ * Return value 0: Success
+ */
+int estimate_iort_size(size_t *iort_size)
+{
+ int count = 0;
+ int pcirc_count = 0;
+ int itsg_count = 0;
+ uint64_t *pcirc_array;
+ uint64_t *itsg_array;
+ struct rid_devid_map *rmap;
+
+ list_for_each_entry(rmap, &rid_devid_list, entry)
+ count++;
+
+ pcirc_array = xzalloc_bytes(sizeof(uint64_t)*count);
+ if ( !pcirc_array )
+ return -ENOMEM;
+
+ itsg_array = xzalloc_bytes(sizeof(uint64_t)*count);
+ if ( !itsg_array )
+ return -ENOMEM;
+
+ list_for_each_entry(rmap, &rid_devid_list, entry)
+ {
+ int i = 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;
+ if ( i == itsg_count )
+ {
+ itsg_array[i] = (uint64_t) rmap->its_node;
+ itsg_count++;
+ break;
+ }
+ }
+ }
+
+ /* Size of IORT
+ * = Size of IORT Table Header + Size of PCIRC Header Nodes +
+ * Size of PCIRC nodes + Size of ITS Header nodes + Size of ITS Nodes
+ * + Size of idmap nodes
+ */
+ *iort_size = sizeof(struct acpi_table_iort) +
+ pcirc_count*( (sizeof(struct acpi_iort_node) -1) +
+ sizeof(struct acpi_iort_root_complex) ) +
+ itsg_count*( (sizeof(struct acpi_iort_node) -1) +
+ sizeof(struct acpi_iort_its_group) ) +
+ count*( sizeof(struct acpi_iort_id_mapping) );
+
+ 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:
+ */
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 155c952349..33a46cab1e 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -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>
@@ -1801,7 +1802,7 @@ static int acpi_create_fadt(struct domain *d, struct membank tbl_add[])
static int estimate_acpi_efi_size(struct domain *d, struct kernel_info *kinfo)
{
- size_t efi_size, acpi_size, madt_size;
+ size_t efi_size, acpi_size, table_size;
u64 addr;
struct acpi_table_rsdp *rsdp_tbl;
struct acpi_table_header *table;
@@ -1811,8 +1812,8 @@ 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 = gic_get_hwdom_madt_size(d);
- acpi_size += ROUNDUP(madt_size, 8);
+ table_size = gic_get_hwdom_madt_size(d);
+ acpi_size += ROUNDUP(table_size, 8);
addr = acpi_os_get_root_pointer();
if ( !addr )
@@ -1842,6 +1843,15 @@ static int estimate_acpi_efi_size(struct domain *d, struct kernel_info *kinfo)
acpi_os_unmap_memory(table, sizeof(struct acpi_table_header));
acpi_size += ROUNDUP(sizeof(struct acpi_table_rsdp), 8);
+
+ if ( estimate_iort_size(&table_size) )
+ {
+ printk("Unable to get hwdom iort size\n");
+ return -EINVAL;
+ }
+
+ acpi_size += ROUNDUP(table_size, 8);
+
d->arch.efi_acpi_len = PAGE_ALIGN(ROUNDUP(efi_size, 8)
+ ROUNDUP(acpi_size, 8));
diff --git a/xen/include/asm-arm/acpi/gen-iort.h b/xen/include/asm-arm/acpi/gen-iort.h
new file mode 100644
index 0000000000..3b2af1e871
--- /dev/null
+++ b/xen/include/asm-arm/acpi/gen-iort.h
@@ -0,0 +1,33 @@
+/*
+ * xen/include/asm-arm/acpi/gen-iort.h
+ *
+ * Manish Jaggi <manish.jaggi@linaro.org>
+ * 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);
+
+#endif
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.14.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH resend 03/13] acpi: arm: Code to generate Hardware Domains IORT
2018-03-13 15:20 [PATCH resend 00/13] acpi: arm: Add IORT Support mjaggi
2018-03-13 15:20 ` [PATCH resend 01/13] acpi: arm: API: Populate/query rid-devid rid-sid map mjaggi
2018-03-13 15:20 ` [PATCH resend 02/13] acpi: arm: query estimated size of hardware domain's IORT mjaggi
@ 2018-03-13 15:20 ` mjaggi
2018-03-23 1:28 ` Julien Grall
2018-03-13 15:20 ` [PATCH resend 04/13] acpi: arm: Copy fwnode / iommu_fwspec code from Linux 4.14 mjaggi
` (9 subsequent siblings)
12 siblings, 1 reply; 23+ messages in thread
From: mjaggi @ 2018-03-13 15:20 UTC (permalink / raw)
To: manish.jaggi, julien.grall, xen-devel, sameer.goel, jgross, sstabellini
Cc: Manish Jaggi, manish.jaggi
From: Manish Jaggi <mjaggi@caviumnetworks.com>
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 <manish.jaggi@linaro.org>
---
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(+)
diff --git a/xen/arch/arm/acpi/gen-iort.c b/xen/arch/arm/acpi/gen-iort.c
index 687c4f18ee..251a9771e3 100644
--- a/xen/arch/arm/acpi/gen-iort.c
+++ b/xen/arch/arm/acpi/gen-iort.c
@@ -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 parsing IORT
+ * is used to generate hwdom IORT.
+ *
+ * One of the challanges is to fixup node offset of ITS Group Nodes
+ * 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
+ * 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);
+
+/*
+ * is_uniq_fwits_node
+ *
+ * returns 1 - if fwits_node is not already in the its_map_list
+ * 0 - if it is present already
+ *
+ * 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,
+ unsigned int offset)
+{
+ 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);
+ 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)
+{
+ struct fwits_hwits_map *map;
+
+ list_for_each_entry(map, &fwits_hwits_list, entry)
+ {
+ if ( map->fwits_node == fwits_node )
+ return map->hwitsnode_offset;
+ }
+
+ return 0;
+}
+
+static void write_hwits_nodes(u8 *iort, unsigned int *offset,
+ unsigned int *num_nodes)
+{
+ struct rid_devid_map *rmap;
+ unsigned int of = *offset;
+ 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
+ * 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 )
+ {
+ 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);
+
+ node->revision = rmap->its_node->revision;
+ 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];
+
+ 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;
+ 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;
+
+ pcirc_node = rmap->pcirc_node;
+
+ if ( opcirc_node == NULL ) /* First entry */
+ {
+ add_node = 1;
+ }
+ else if ( opcirc_node != pcirc_node ) /* another pci_rc_node found*/
+ {
+ /* All the idmaps of a pcirc are written, now update node info*/
+ hwdom_pcirc_node->length = num_idmap *
+ sizeof(struct acpi_iort_id_mapping) +
+ sizeof(struct acpi_iort_node) +
+ sizeof(struct acpi_iort_root_complex) -
+ sizeof(pcirc_node->node_data);
+
+ hwdom_pcirc_node->mapping_count = num_idmap;
+ hwdom_pcirc_node->mapping_offset = sizeof(struct acpi_iort_node) +
+ sizeof(struct acpi_iort_root_complex) -
+ sizeof(pcirc_node->node_data);
+ old_pos += hwdom_pcirc_node->length;
+ add_node = 1;
+ }
+
+ if ( add_node ) /* create the pcirc node */
+ {
+ opcirc_node = pcirc_node;
+ hwdom_pcirc_node = (struct acpi_iort_node *)&iort[old_pos];
+ hwdom_pcirc_node->type = ACPI_IORT_NODE_PCI_ROOT_COMPLEX;
+ hwdom_pcirc_node->mapping_offset = sizeof(struct acpi_iort_node) +
+ sizeof(struct acpi_iort_root_complex) -
+ sizeof(hwdom_pcirc_node->node_data);
+
+ rc = (struct acpi_iort_root_complex *)
+ &hwdom_pcirc_node->node_data;
+
+ rc_fw = (struct acpi_iort_root_complex *)
+ &pcirc_node->node_data;
+
+ rc->pci_segment_number = rc_fw->pci_segment_number;
+ rc->ats_attribute = rc_fw->ats_attribute;
+ rc->memory_properties = rc_fw->memory_properties;
+
+ idmap = ACPI_ADD_PTR(struct acpi_iort_id_mapping,
+ hwdom_pcirc_node,
+ hwdom_pcirc_node->mapping_offset);
+ n++;
+ num_idmap = 0;
+ }
+
+ idmap->input_base = rmap->idmap.input_base;
+ idmap->id_count = rmap->idmap.id_count;
+ idmap->output_base = rmap->idmap.output_base;
+ idmap->output_reference = hwitsnode_offset_from_map(rmap->its_node);
+ BUG_ON(!idmap->output_reference);
+
+ idmap->flags = 0;
+ idmap++;
+ num_idmap++;
+ }
+
+ if ( hwdom_pcirc_node ) /* if no further PCIRC nodes found */
+ {
+ /* All the idmaps of a pcirc are written, now update node info*/
+ hwdom_pcirc_node->length = num_idmap *
+ sizeof(struct acpi_iort_id_mapping) +
+ sizeof(struct acpi_iort_node) +
+ sizeof(struct acpi_iort_root_complex) -1;
+
+ hwdom_pcirc_node->mapping_count = num_idmap;
+ old_pos += hwdom_pcirc_node->length;
+ }
+
+ *pos = old_pos;
+ *num_nodes = n;
+}
+
+bool is_iort_available(void)
+{
+ struct acpi_table_iort *iort;
+
+ if ( acpi_get_table(ACPI_SIG_IORT, 0,
+ (struct acpi_table_header **)&iort) )
+ return 0;
+
+ return 1;
+}
+
+int prepare_hwdom_iort(struct acpi_table_iort *hwdom_iort, unsigned int *iort_size)
+{
+ struct acpi_table_iort *fw_iort;
+ unsigned int num_nodes = 0;
+ unsigned int pos;
+
+ pos = sizeof(struct acpi_table_iort);
+
+ if ( acpi_get_table(ACPI_SIG_IORT, 0,
+ (struct acpi_table_header **)&fw_iort) )
+ {
+ printk("Failed to get IORT table\n");
+ return -ENODEV;
+ }
+
+ /* Write IORT header */
+ ACPI_MEMCPY(hwdom_iort, fw_iort, sizeof(struct acpi_table_iort));
+ hwdom_iort->node_offset = pos;
+ hwdom_iort->node_count = 0;
+
+ /* Write its group nodes */
+ write_hwits_nodes((u8*)hwdom_iort, &pos, &num_nodes);
+ hwdom_iort->node_count = num_nodes;
+
+ /* Write pcirc_nodes */
+ write_hwpcirc_nodes((u8*)hwdom_iort, &pos, &num_nodes);
+
+ /* Update IORT Size in IORT header */
+ hwdom_iort->node_count += num_nodes;
+ hwdom_iort->header.length = pos;
+ hwdom_iort->header.checksum = acpi_tb_checksum(
+ ACPI_CAST_PTR(u8, hwdom_iort),
+ pos);
+ *iort_size = hwdom_iort->header.length;
+
+ return 0;
+}
/*
* Size of hardware domains' IORT is calculated based on the number of
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 33a46cab1e..6c6dfad38c 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1656,6 +1656,8 @@ static int acpi_create_xsdt(struct domain *d, struct membank tbl_add[])
ACPI_SIG_FADT, tbl_add[TBL_FADT].start);
acpi_xsdt_modify_entry(xsdt->table_offset_entry, entry_count,
ACPI_SIG_MADT, tbl_add[TBL_MADT].start);
+ acpi_xsdt_modify_entry(xsdt->table_offset_entry, entry_count,
+ ACPI_SIG_IORT, tbl_add[TBL_IORT].start);
xsdt->table_offset_entry[entry_count] = tbl_add[TBL_STAO].start;
xsdt->header.length = table_size;
@@ -1706,6 +1708,35 @@ static int acpi_create_stao(struct domain *d, struct membank tbl_add[])
return 0;
}
+static int acpi_create_iort(struct domain *d, struct membank tbl_add[])
+{
+ struct acpi_table_iort *hwdom_table;
+ unsigned int size = 0;
+
+ if ( is_iort_available() )
+ {
+ tbl_add[TBL_IORT].start = d->arch.efi_acpi_gpa +
+ acpi_get_table_offset(tbl_add, TBL_IORT);
+ hwdom_table = d->arch.efi_acpi_table +
+ acpi_get_table_offset(tbl_add, TBL_IORT);
+
+ if ( prepare_hwdom_iort(hwdom_table, &size) )
+ {
+ printk("Failed to write IORT table\n");
+ return -EINVAL;
+ }
+
+ tbl_add[TBL_IORT].size = size;
+ }
+ else
+ {
+ tbl_add[TBL_IORT].start = 0;
+ tbl_add[TBL_IORT].size = 0;
+ }
+
+ return 0;
+}
+
static int acpi_create_madt(struct domain *d, struct membank tbl_add[])
{
struct acpi_table_header *table = NULL;
@@ -1901,6 +1932,10 @@ static int prepare_acpi(struct domain *d, struct kernel_info *kinfo)
if ( rc != 0 )
return rc;
+ rc = acpi_create_iort(d, tbl_add);
+ if ( rc != 0 )
+ return rc;
+
rc = acpi_create_xsdt(d, tbl_add);
if ( rc != 0 )
return rc;
diff --git a/xen/include/asm-arm/acpi.h b/xen/include/asm-arm/acpi.h
index c183b6bb6e..f8b5254621 100644
--- a/xen/include/asm-arm/acpi.h
+++ b/xen/include/asm-arm/acpi.h
@@ -36,6 +36,7 @@ typedef enum {
TBL_FADT,
TBL_MADT,
TBL_STAO,
+ TBL_IORT,
TBL_XSDT,
TBL_RSDP,
TBL_EFIT,
diff --git a/xen/include/asm-arm/acpi/gen-iort.h b/xen/include/asm-arm/acpi/gen-iort.h
index 3b2af1e871..52974f6052 100644
--- a/xen/include/asm-arm/acpi/gen-iort.h
+++ b/xen/include/asm-arm/acpi/gen-iort.h
@@ -22,6 +22,17 @@
*/
int estimate_iort_size(size_t *iort_size);
+/*
+ * Checks if IORT is present in ACPI tables.
+ */
+bool is_iort_available(void);
+
+/*
+ * Prepares IORT in buffer hwdom_iort and updates iort_size
+ */
+int prepare_hwdom_iort(struct acpi_table_iort *hwdom_iort,
+ unsigned int *iort_size);
+
#endif
/*
* Local variables:
--
2.14.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH resend 04/13] acpi: arm: Copy fwnode / iommu_fwspec code from Linux 4.14
2018-03-13 15:20 [PATCH resend 00/13] acpi: arm: Add IORT Support mjaggi
` (2 preceding siblings ...)
2018-03-13 15:20 ` [PATCH resend 03/13] acpi: arm: Code to generate Hardware Domains IORT mjaggi
@ 2018-03-13 15:20 ` mjaggi
2018-03-13 15:20 ` [PATCH resend 05/13] acpi: arm: Import acpi_iort.h verbatim from linux 4.14 mjaggi
` (8 subsequent siblings)
12 siblings, 0 replies; 23+ messages in thread
From: mjaggi @ 2018-03-13 15:20 UTC (permalink / raw)
To: manish.jaggi, julien.grall, xen-devel, sameer.goel, jgross, sstabellini
Cc: Manish Jaggi, manish.jaggi
From: Manish Jaggi <mjaggi@caviumnetworks.com>
fwnode is firmware device node object handle type definition. This can
be used either for device tree node or ACPI table node.
However in the context of this patchset it is used mainly for ACPI
nodes.
iommu_fwspec defines set of opeations associated with fwnode.
This patch does not directly imports the code files from linux,
rahter copies only the relavant parts of code required for Xen.
Signed-off-by: Manish Jaggi <manish.jaggi@linaro.org>
---
xen/drivers/passthrough/arm/iommu.c | 85 ++++++++++++++++++++++++
xen/include/asm-arm/device.h | 1 +
xen/include/asm-arm/fwnode.h | 128 ++++++++++++++++++++++++++++++++++++
xen/include/asm-arm/fwspec.h | 38 +++++++++++
4 files changed, 252 insertions(+)
diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c
index 95b1abb972..6e3960e4af 100644
--- a/xen/drivers/passthrough/arm/iommu.c
+++ b/xen/drivers/passthrough/arm/iommu.c
@@ -19,6 +19,8 @@
#include <xen/iommu.h>
#include <xen/device_tree.h>
#include <asm/device.h>
+#include <asm/fwnode.h>
+#include <asm/fwspec.h>
static const struct iommu_ops *iommu_ops;
@@ -73,3 +75,86 @@ int arch_iommu_populate_page_table(struct domain *d)
/* The IOMMU shares the p2m with the CPU */
return -ENOSYS;
}
+
+/**
+ * fwnode_handle_put - Drop reference to a device node
+ * @fwnode: Pointer to the device node to drop the reference to.
+ *
+ * To be used when terminating device_for_each_child_node() iteration with
+ * break / return to prevent stale device node references being left behind
+ */
+void fwnode_handle_put(struct fwnode_handle *fwnode)
+{
+ fwnode_call_void_op(fwnode, put);
+}
+
+const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
+{
+ return iommu_get_ops();
+}
+
+int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
+ const struct iommu_ops *ops)
+{
+ struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+
+ if ( fwspec )
+ return ops == fwspec->ops ? 0 : -EINVAL;
+
+ fwspec = xzalloc_bytes(sizeof(*fwspec));
+ if ( !fwspec )
+ return -ENOMEM;
+#if 0
+ of_node_get(to_of_node(iommu_fwnode)); /* TODO */
+#endif
+ fwspec->iommu_fwnode = iommu_fwnode;
+ fwspec->ops = ops;
+ dev->iommu_fwspec = fwspec;
+
+ return 0;
+}
+
+void iommu_fwspec_free(struct device *dev)
+{
+ struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+
+ if ( fwspec )
+ {
+ fwnode_handle_put(fwspec->iommu_fwnode);
+ xfree(fwspec);
+ dev->iommu_fwspec = NULL;
+ }
+}
+
+int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids)
+{
+ struct iommu_fwspec *fwspec_n;
+ struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+ size_t size, size_n;
+ int i;
+
+ if ( !fwspec )
+ return -EINVAL;
+
+ size = offsetof(struct iommu_fwspec, ids[fwspec->num_ids]);
+ size_n = offsetof(struct iommu_fwspec, ids[fwspec->num_ids + num_ids]);
+ if ( size_n > size )
+ {
+ fwspec_n = _xzalloc(size_n, sizeof(void*));
+ if ( !fwspec_n )
+ return -ENOMEM;
+
+ memcpy(fwspec_n, fwspec, size);
+ xfree(fwspec);
+ fwspec = fwspec_n;
+ }
+
+ for ( i = 0; i < num_ids; i++ )
+ fwspec->ids[fwspec->num_ids + i] = ids[i];
+
+ fwspec->num_ids += num_ids;
+ dev->iommu_fwspec = fwspec;
+
+ return 0;
+}
+
diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
index 6734ae8efd..7f2d8d367e 100644
--- a/xen/include/asm-arm/device.h
+++ b/xen/include/asm-arm/device.h
@@ -20,6 +20,7 @@ struct device
struct dt_device_node *of_node; /* Used by drivers imported from Linux */
#endif
struct dev_archdata archdata;
+ struct iommu_fwspec *iommu_fwspec;
};
typedef struct device device_t;
diff --git a/xen/include/asm-arm/fwnode.h b/xen/include/asm-arm/fwnode.h
new file mode 100644
index 0000000000..d74c7776ea
--- /dev/null
+++ b/xen/include/asm-arm/fwnode.h
@@ -0,0 +1,128 @@
+/*
+ * fwnode.h - Firmware device node object handle type definition.
+ *
+ * Copyright (C) 2015, Intel Corporation
+ * Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Xen Modifications: Manish Jaggi
+ * Coding Style: Linux
+ */
+
+#ifndef _ASM_ARM_FWNODE_H_
+#define _ASM_ARM_FWNODE_H_
+
+#include <xen/types.h>
+
+struct fwnode_operations;
+
+struct fwnode_handle {
+ struct fwnode_handle *secondary;
+ const struct fwnode_operations *ops;
+};
+
+/**
+ * struct fwnode_endpoint - Fwnode graph endpoint
+ * @port: Port number
+ * @id: Endpoint id
+ * @local_fwnode: reference to the related fwnode
+ */
+struct fwnode_endpoint {
+ unsigned int port;
+ unsigned int id;
+ const struct fwnode_handle *local_fwnode;
+};
+
+#define NR_FWNODE_REFERENCE_ARGS 8
+
+/**
+ * struct fwnode_reference_args - Fwnode reference with additional arguments
+ * @fwnode:- A reference to the base fwnode
+ * @nargs: Number of elements in @args array
+ * @args: Integer arguments on the fwnode
+ */
+struct fwnode_reference_args {
+ struct fwnode_handle *fwnode;
+ unsigned int nargs;
+ unsigned int args[NR_FWNODE_REFERENCE_ARGS];
+};
+
+/**
+ * struct fwnode_operations - Operations for fwnode interface
+ * @get: Get a reference to an fwnode.
+ * @put: Put a reference to an fwnode.
+ * @property_present: Return true if a property is present.
+ * @property_read_integer_array: Read an array of integer properties. Return
+ * zero on success, a negative error code
+ * otherwise.
+ * @property_read_string_array: Read an array of string properties. Return zero
+ * on success, a negative error code otherwise.
+ * @get_parent: Return the parent of an fwnode.
+ * @get_next_child_node: Return the next child node in an iteration.
+ * @get_named_child_node: Return a child node with a given name.
+ * @get_reference_args: Return a reference pointed to by a property, with args
+ * @graph_get_next_endpoint: Return an endpoint node in an iteration.
+ * @graph_get_remote_endpoint: Return the remote endpoint node of a local
+ * endpoint node.
+ * @graph_get_port_parent: Return the parent node of a port node.
+ * @graph_parse_endpoint: Parse endpoint for port and endpoint id.
+ */
+struct fwnode_operations {
+ void (*get)(struct fwnode_handle *fwnode);
+ void (*put)(struct fwnode_handle *fwnode);
+ bool (*device_is_available)(const struct fwnode_handle *fwnode);
+ bool (*property_present)(const struct fwnode_handle *fwnode,
+ const char *propname);
+ int (*property_read_int_array)(const struct fwnode_handle *fwnode,
+ const char *propname,
+ unsigned int elem_size, void *val,
+ size_t nval);
+ int
+ (*property_read_string_array)(const struct fwnode_handle *fwnode_handle,
+ const char *propname, const char **val,
+ size_t nval);
+ struct fwnode_handle *(*get_parent)(const struct fwnode_handle *fwnode);
+ struct fwnode_handle *
+ (*get_next_child_node)(const struct fwnode_handle *fwnode,
+ struct fwnode_handle *child);
+ struct fwnode_handle *
+ (*get_named_child_node)(const struct fwnode_handle *fwnode,
+ const char *name);
+ int (*get_reference_args)(const struct fwnode_handle *fwnode,
+ const char *prop, const char *nargs_prop,
+ unsigned int nargs, unsigned int index,
+ struct fwnode_reference_args *args);
+ struct fwnode_handle *
+ (*graph_get_next_endpoint)(const struct fwnode_handle *fwnode,
+ struct fwnode_handle *prev);
+ struct fwnode_handle *
+ (*graph_get_remote_endpoint)(const struct fwnode_handle *fwnode);
+ struct fwnode_handle *
+ (*graph_get_port_parent)(struct fwnode_handle *fwnode);
+ int (*graph_parse_endpoint)(const struct fwnode_handle *fwnode,
+ struct fwnode_endpoint *endpoint);
+};
+
+#define fwnode_has_op(fwnode, op) \
+ ((fwnode) && (fwnode)->ops && (fwnode)->ops->op)
+#define fwnode_call_int_op(fwnode, op, ...) \
+ (fwnode ? (fwnode_has_op(fwnode, op) ? \
+ (fwnode)->ops->op(fwnode, ## __VA_ARGS__) : -ENXIO) : \
+ -EINVAL)
+#define fwnode_call_bool_op(fwnode, op, ...) \
+ (fwnode ? (fwnode_has_op(fwnode, op) ? \
+ (fwnode)->ops->op(fwnode, ## __VA_ARGS__) : false) : \
+ false)
+#define fwnode_call_ptr_op(fwnode, op, ...) \
+ (fwnode_has_op(fwnode, op) ? \
+ (fwnode)->ops->op(fwnode, ## __VA_ARGS__) : NULL)
+#define fwnode_call_void_op(fwnode, op, ...) \
+ do { \
+ if (fwnode_has_op(fwnode, op)) \
+ (fwnode)->ops->op(fwnode, ## __VA_ARGS__); \
+ } while (false)
+
+#endif
diff --git a/xen/include/asm-arm/fwspec.h b/xen/include/asm-arm/fwspec.h
new file mode 100644
index 0000000000..e67d2bb678
--- /dev/null
+++ b/xen/include/asm-arm/fwspec.h
@@ -0,0 +1,38 @@
+/*
+ *
+ * Copyright (C) 2015, Intel Corporation
+ * Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Xen Modifications: Manish Jaggi
+ */
+
+#ifndef _ASM_FWSPEC_H
+#define _ASM_FWSPEC_H
+
+/**
+ * struct iommu_fwspec - per-device IOMMU instance data
+ * @ops: ops for this device's IOMMU
+ * @iommu_fwnode: firmware handle for this device's IOMMU
+ * @iommu_priv: IOMMU driver private data for this device
+ * @num_ids: number of associated device IDs
+ * @ids: IDs which this device may present to the IOMMU
+ */
+struct iommu_fwspec {
+ const struct iommu_ops *ops;
+ struct fwnode_handle *iommu_fwnode;
+ void *iommu_priv;
+ unsigned int num_ids;
+ u32 ids[1];
+};
+
+int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
+ const struct iommu_ops *ops);
+void iommu_fwspec_free(struct device *dev);
+int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
+const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode);
+
+#endif
--
2.14.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH resend 05/13] acpi: arm: Import acpi_iort.h verbatim from linux 4.14
2018-03-13 15:20 [PATCH resend 00/13] acpi: arm: Add IORT Support mjaggi
` (3 preceding siblings ...)
2018-03-13 15:20 ` [PATCH resend 04/13] acpi: arm: Copy fwnode / iommu_fwspec code from Linux 4.14 mjaggi
@ 2018-03-13 15:20 ` mjaggi
2018-03-13 15:20 ` [PATCH resend 06/13] acpi: arm: Update acpi_iort.h with xen specific changes mjaggi
` (7 subsequent siblings)
12 siblings, 0 replies; 23+ messages in thread
From: mjaggi @ 2018-03-13 15:20 UTC (permalink / raw)
To: manish.jaggi, julien.grall, xen-devel, sameer.goel, jgross, sstabellini
Cc: Manish Jaggi, manish.jaggi
From: Manish Jaggi <mjaggi@caviumnetworks.com>
Import acpi_iort.h verbatim from linux 4.14
Signed-off-by: Manish Jaggi <manish.jaggi@linaro.org>
---
xen/include/asm-arm/acpi/acpi_iort.h | 57 ++++++++++++++++++++++++++++++++++++
1 file changed, 57 insertions(+)
diff --git a/xen/include/asm-arm/acpi/acpi_iort.h b/xen/include/asm-arm/acpi/acpi_iort.h
new file mode 100644
index 0000000000..8d3f0bf803
--- /dev/null
+++ b/xen/include/asm-arm/acpi/acpi_iort.h
@@ -0,0 +1,57 @@
+/*
+ * Copyright (C) 2016, Semihalf
+ * Author: Tomasz Nowicki <tn@semihalf.com>
+ *
+ * This 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 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.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc., 59 Temple
+ * Place - Suite 330, Boston, MA 02111-1307 USA.
+ */
+
+#ifndef __ACPI_IORT_H__
+#define __ACPI_IORT_H__
+
+#include <linux/acpi.h>
+#include <linux/fwnode.h>
+#include <linux/irqdomain.h>
+
+#define IORT_IRQ_MASK(irq) (irq & 0xffffffffULL)
+#define IORT_IRQ_TRIGGER_MASK(irq) ((irq >> 32) & 0xffffffffULL)
+
+int iort_register_domain_token(int trans_id, struct fwnode_handle *fw_node);
+void iort_deregister_domain_token(int trans_id);
+struct fwnode_handle *iort_find_domain_token(int trans_id);
+#ifdef CONFIG_ACPI_IORT
+void acpi_iort_init(void);
+u32 iort_msi_map_rid(struct device *dev, u32 req_id);
+struct irq_domain *iort_get_device_domain(struct device *dev, u32 req_id);
+void acpi_configure_pmsi_domain(struct device *dev);
+int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id);
+/* IOMMU interface */
+void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *size);
+const struct iommu_ops *iort_iommu_configure(struct device *dev);
+#else
+static inline void acpi_iort_init(void) { }
+static inline u32 iort_msi_map_rid(struct device *dev, u32 req_id)
+{ return req_id; }
+static inline struct irq_domain *iort_get_device_domain(struct device *dev,
+ u32 req_id)
+{ return NULL; }
+static inline void acpi_configure_pmsi_domain(struct device *dev) { }
+/* IOMMU interface */
+static inline void iort_dma_setup(struct device *dev, u64 *dma_addr,
+ u64 *size) { }
+static inline
+const struct iommu_ops *iort_iommu_configure(struct device *dev)
+{ return NULL; }
+#endif
+
+#endif /* __ACPI_IORT_H__ */
--
2.14.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH resend 06/13] acpi: arm: Update acpi_iort.h with xen specific changes
2018-03-13 15:20 [PATCH resend 00/13] acpi: arm: Add IORT Support mjaggi
` (4 preceding siblings ...)
2018-03-13 15:20 ` [PATCH resend 05/13] acpi: arm: Import acpi_iort.h verbatim from linux 4.14 mjaggi
@ 2018-03-13 15:20 ` mjaggi
2018-03-13 15:20 ` [PATCH resend 07/13] arm: Adding ACPI_IORT in arm Kconfig mjaggi
` (6 subsequent siblings)
12 siblings, 0 replies; 23+ messages in thread
From: mjaggi @ 2018-03-13 15:20 UTC (permalink / raw)
To: manish.jaggi, julien.grall, xen-devel, sameer.goel, jgross, sstabellini
Cc: Manish Jaggi, manish.jaggi
From: Manish Jaggi <mjaggi@caviumnetworks.com>
Remove the parts of acpi_iort.h which are not required for Xen.
Signed-off-by: Manish Jaggi <manish.jaggi@linaro.org>
---
xen/include/asm-arm/acpi/acpi_iort.h | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/xen/include/asm-arm/acpi/acpi_iort.h b/xen/include/asm-arm/acpi/acpi_iort.h
index 8d3f0bf803..dcffb23773 100644
--- a/xen/include/asm-arm/acpi/acpi_iort.h
+++ b/xen/include/asm-arm/acpi/acpi_iort.h
@@ -19,18 +19,13 @@
#ifndef __ACPI_IORT_H__
#define __ACPI_IORT_H__
-#include <linux/acpi.h>
-#include <linux/fwnode.h>
-#include <linux/irqdomain.h>
+#include <asm/device.h>
+#include <xen/acpi.h>
#define IORT_IRQ_MASK(irq) (irq & 0xffffffffULL)
#define IORT_IRQ_TRIGGER_MASK(irq) ((irq >> 32) & 0xffffffffULL)
-int iort_register_domain_token(int trans_id, struct fwnode_handle *fw_node);
-void iort_deregister_domain_token(int trans_id);
-struct fwnode_handle *iort_find_domain_token(int trans_id);
#ifdef CONFIG_ACPI_IORT
-void acpi_iort_init(void);
u32 iort_msi_map_rid(struct device *dev, u32 req_id);
struct irq_domain *iort_get_device_domain(struct device *dev, u32 req_id);
void acpi_configure_pmsi_domain(struct device *dev);
@@ -39,7 +34,6 @@ int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id);
void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *size);
const struct iommu_ops *iort_iommu_configure(struct device *dev);
#else
-static inline void acpi_iort_init(void) { }
static inline u32 iort_msi_map_rid(struct device *dev, u32 req_id)
{ return req_id; }
static inline struct irq_domain *iort_get_device_domain(struct device *dev,
--
2.14.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH resend 07/13] arm: Adding ACPI_IORT in arm Kconfig
2018-03-13 15:20 [PATCH resend 00/13] acpi: arm: Add IORT Support mjaggi
` (5 preceding siblings ...)
2018-03-13 15:20 ` [PATCH resend 06/13] acpi: arm: Update acpi_iort.h with xen specific changes mjaggi
@ 2018-03-13 15:20 ` mjaggi
2018-03-13 15:20 ` [PATCH resend 08/13] asm: arm: pci: Fix the #include label in asm-arm/pci.h mjaggi
` (5 subsequent siblings)
12 siblings, 0 replies; 23+ messages in thread
From: mjaggi @ 2018-03-13 15:20 UTC (permalink / raw)
To: manish.jaggi, julien.grall, xen-devel, sameer.goel, jgross, sstabellini
Cc: Manish Jaggi, manish.jaggi
From: Manish Jaggi <mjaggi@caviumnetworks.com>
Signed-off-by: Manish Jaggi <manish.jaggi@linaro.org>
---
xen/arch/arm/Kconfig | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 2782ee6589..d3fbcbcc6f 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -59,6 +59,10 @@ config SBSA_VUART_CONSOLE
endmenu
+config ACPI_IORT
+ depends on ACPI
+ def_bool y
+
menu "ARM errata workaround via the alternative framework"
depends on HAS_ALTERNATIVE
--
2.14.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH resend 08/13] asm: arm: pci: Fix the #include label in asm-arm/pci.h
2018-03-13 15:20 [PATCH resend 00/13] acpi: arm: Add IORT Support mjaggi
` (6 preceding siblings ...)
2018-03-13 15:20 ` [PATCH resend 07/13] arm: Adding ACPI_IORT in arm Kconfig mjaggi
@ 2018-03-13 15:20 ` mjaggi
2018-03-13 15:20 ` [PATCH resend 09/13] asm: arm: to_pci_dev mjaggi
` (4 subsequent siblings)
12 siblings, 0 replies; 23+ messages in thread
From: mjaggi @ 2018-03-13 15:20 UTC (permalink / raw)
To: manish.jaggi, julien.grall, xen-devel, sameer.goel, jgross, sstabellini
Cc: Manish Jaggi, manish.jaggi
From: Manish Jaggi <mjaggi@caviumnetworks.com>
asm-arm/pci.h has an incorrect #ifndef label.
Fixing it to ARM
Signed-off-by: Manish Jaggi <manish.jaggi@linaro.org>
---
xen/include/asm-arm/pci.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
index de13359f65..3145ed505c 100644
--- a/xen/include/asm-arm/pci.h
+++ b/xen/include/asm-arm/pci.h
@@ -1,7 +1,7 @@
-#ifndef __X86_PCI_H__
-#define __X86_PCI_H__
+#ifndef __ASM_ARM_PCI_H__
+#define __ASM_ARM_PCI_H__
struct arch_pci_dev {
};
-#endif /* __X86_PCI_H__ */
+#endif /* __ASM_ARM_PCI_H__ */
--
2.14.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH resend 09/13] asm: arm: to_pci_dev
2018-03-13 15:20 [PATCH resend 00/13] acpi: arm: Add IORT Support mjaggi
` (7 preceding siblings ...)
2018-03-13 15:20 ` [PATCH resend 08/13] asm: arm: pci: Fix the #include label in asm-arm/pci.h mjaggi
@ 2018-03-13 15:20 ` mjaggi
2018-03-13 15:20 ` [PATCH resend 10/13] asm: arm: add dev_is_pci mjaggi
` (3 subsequent siblings)
12 siblings, 0 replies; 23+ messages in thread
From: mjaggi @ 2018-03-13 15:20 UTC (permalink / raw)
To: manish.jaggi, julien.grall, xen-devel, sameer.goel, jgross, sstabellini
Cc: Manish Jaggi, manish.jaggi
From: Manish Jaggi <mjaggi@caviumnetworks.com>
to_pci_dev for ARM was todo till now.
Provide definition for this macro.
Signed-off-by: Manish Jaggi <manish.jaggi@linaro.org>
---
xen/drivers/passthrough/arm/smmu.c | 3 ++-
xen/include/asm-arm/pci.h | 6 ++++++
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 74c09b0991..81629a695c 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -46,10 +46,12 @@
#include <xen/rbtree.h>
#include <xen/sched.h>
#include <xen/sizes.h>
+#include <xen/pci.h>
#include <asm/atomic.h>
#include <asm/device.h>
#include <asm/io.h>
#include <asm/platform.h>
+#include <asm/pci.h>
/* Xen: The below defines are redefined within the file. Undef it */
#undef SCTLR_AFE
@@ -181,7 +183,6 @@ static void __iomem *devm_ioremap_resource(struct device *dev,
* Xen: PCI functions
* TODO: It should be implemented when PCI will be supported
*/
-#define to_pci_dev(dev) (NULL)
static inline int pci_for_each_dma_alias(struct pci_dev *pdev,
int (*fn) (struct pci_dev *pdev,
u16 alias, void *data),
diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
index 3145ed505c..d3de409927 100644
--- a/xen/include/asm-arm/pci.h
+++ b/xen/include/asm-arm/pci.h
@@ -2,6 +2,12 @@
#define __ASM_ARM_PCI_H__
struct arch_pci_dev {
+ struct device dev;
};
+#define to_pci_dev(d) container_of( \
+ container_of(d, struct arch_pci_dev, dev), \
+ struct pci_dev,\
+ arch\
+ )
#endif /* __ASM_ARM_PCI_H__ */
--
2.14.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH resend 10/13] asm: arm: add dev_is_pci
2018-03-13 15:20 [PATCH resend 00/13] acpi: arm: Add IORT Support mjaggi
` (8 preceding siblings ...)
2018-03-13 15:20 ` [PATCH resend 09/13] asm: arm: to_pci_dev mjaggi
@ 2018-03-13 15:20 ` mjaggi
2018-03-13 15:20 ` [PATCH resend 11/13] asm: arm: add pci_domain_nr mjaggi
` (2 subsequent siblings)
12 siblings, 0 replies; 23+ messages in thread
From: mjaggi @ 2018-03-13 15:20 UTC (permalink / raw)
To: manish.jaggi, julien.grall, xen-devel, sameer.goel, jgross, sstabellini
Cc: Manish Jaggi, manish.jaggi
From: Manish Jaggi <mjaggi@caviumnetworks.com>
dev_is_pci for ARM was todo till now.
Provide definition for this macro.
Signed-off-by: Manish Jaggi <manish.jaggi@linaro.org>
---
xen/include/asm-arm/device.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
index 7f2d8d367e..b75d79b793 100644
--- a/xen/include/asm-arm/device.h
+++ b/xen/include/asm-arm/device.h
@@ -6,6 +6,7 @@
enum device_type
{
DEV_DT,
+ DEV_PCI,
};
struct dev_archdata {
@@ -27,8 +28,7 @@ typedef struct device device_t;
#include <xen/device_tree.h>
-/* TODO: Correctly implement dev_is_pci when PCI is supported on ARM */
-#define dev_is_pci(dev) ((void)(dev), 0)
+#define dev_is_pci(dev) ((dev->type == DEV_PCI))
#define dev_is_dt(dev) ((dev->type == DEV_DT)
enum device_class
--
2.14.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH resend 11/13] asm: arm: add pci_domain_nr
2018-03-13 15:20 [PATCH resend 00/13] acpi: arm: Add IORT Support mjaggi
` (9 preceding siblings ...)
2018-03-13 15:20 ` [PATCH resend 10/13] asm: arm: add dev_is_pci mjaggi
@ 2018-03-13 15:20 ` mjaggi
2018-03-13 15:20 ` [PATCH resend 12/13] acpi: arm: Provide support for iort iommu configuration hooks mjaggi
2018-03-13 15:20 ` [PATCH resend 13/13] Add code to parse IORT and prepare rid maps mjaggi
12 siblings, 0 replies; 23+ messages in thread
From: mjaggi @ 2018-03-13 15:20 UTC (permalink / raw)
To: manish.jaggi, julien.grall, xen-devel, sameer.goel, jgross, sstabellini
Cc: Manish Jaggi, manish.jaggi
From: Manish Jaggi <mjaggi@caviumnetworks.com>
Provide definition for this macro.
Signed-off-by: Manish Jaggi <manish.jaggi@linaro.org>
---
xen/include/xen/pci.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index dd5ec43a70..ee1d4dbf93 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -114,6 +114,8 @@ struct pci_dev {
u64 vf_rlen[6];
};
+#define pci_domain_nr(pdev) pdev->seg
+
#define for_each_pdev(domain, pdev) \
list_for_each_entry(pdev, &(domain->arch.pdev_list), domain_list)
--
2.14.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH resend 12/13] acpi: arm: Provide support for iort iommu configuration hooks
2018-03-13 15:20 [PATCH resend 00/13] acpi: arm: Add IORT Support mjaggi
` (10 preceding siblings ...)
2018-03-13 15:20 ` [PATCH resend 11/13] asm: arm: add pci_domain_nr mjaggi
@ 2018-03-13 15:20 ` mjaggi
2018-03-13 15:20 ` [PATCH resend 13/13] Add code to parse IORT and prepare rid maps mjaggi
12 siblings, 0 replies; 23+ messages in thread
From: mjaggi @ 2018-03-13 15:20 UTC (permalink / raw)
To: manish.jaggi, julien.grall, xen-devel, sameer.goel, jgross, sstabellini
Cc: Manish Jaggi, manish.jaggi
From: Manish Jaggi <mjaggi@caviumnetworks.com>
This patch copies the basic code from linux (iort.c) required to parse
IORT and
hooks for iommu configuration and initializes smmu device.
Provides a top level initcall acpi_iort_init which would call
parse_iort (next patch) to parse and prepare the rid-devid and rid-sid
maps.
This patch is dependent on next patch. The code is cleanly split into
two patches
Signed-off-by: Manish Jaggi <manish.jaggi@linaro.org>
---
xen/arch/arm/acpi/Makefile | 1 +
xen/arch/arm/acpi/iort.c | 401 +++++++++++++++++++++++++++++++++++++++++++
xen/include/asm-arm/device.h | 12 +-
3 files changed, 413 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/acpi/Makefile b/xen/arch/arm/acpi/Makefile
index 073339603c..a596559205 100644
--- a/xen/arch/arm/acpi/Makefile
+++ b/xen/arch/arm/acpi/Makefile
@@ -2,3 +2,4 @@ obj-y += lib.o
obj-y += boot.init.o
obj-y += ridmap.o
obj-y += gen-iort.o
+obj-y += iort.o
diff --git a/xen/arch/arm/acpi/iort.c b/xen/arch/arm/acpi/iort.c
new file mode 100644
index 0000000000..4238611ef7
--- /dev/null
+++ b/xen/arch/arm/acpi/iort.c
@@ -0,0 +1,401 @@
+/*
+ * Copyright (C) 2016, Semihalf
+ * Author: Tomasz Nowicki <tn@semihalf.com>
+ *
+ * This 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 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.
+ *
+ * This file implements early detection/parsing of I/O mapping
+ * reported to OS through firmware via I/O Remapping Table (IORT)
+ * IORT document number: ARM DEN 0049A
+ *
+ * Based on Linux 4.14.0
+ * Xen Modifications : Manish Jaggi <manish.jaggi@linaro.org>
+ * Coding Style: Xen
+ */
+
+#define pr_fmt(fmt) "ACPI: IORT: " fmt
+
+#include <asm/acpi/ridmap.h>
+#include <asm/acpi/acpi_iort.h>
+#include <asm/fwnode.h>
+#include <asm/fwspec.h>
+#include <xen/iommu.h>
+#include <xen/kernel.h>
+#include <xen/list.h>
+#include <xen/lib.h>
+#include <xen/pci.h>
+
+#define IORT_TYPE_MASK(type) (1 << (type))
+#define IORT_MSI_TYPE (1 << ACPI_IORT_NODE_ITS_GROUP)
+#define IORT_IOMMU_TYPE ((1 << ACPI_IORT_NODE_SMMU) | \
+ (1 << ACPI_IORT_NODE_SMMU_V3))
+
+/* Until ACPICA headers cover IORT rev. C */
+#ifndef ACPI_IORT_SMMU_V3_CAVIUM_CN99XX
+#define ACPI_IORT_SMMU_V3_CAVIUM_CN99XX 0x2
+#endif
+
+/* Redefine WARN macros */
+#undef WARN
+#undef WARN_ON
+#define WARN(condition, format...) ({ \
+ int __ret_warn_on = !!(condition); \
+ if (unlikely(__ret_warn_on)) \
+ printk(format); \
+ unlikely(__ret_warn_on); \
+})
+#define WARN_TAINT(cond, taint, format...) WARN(cond, format)
+#define WARN_ON(cond) (!!cond)
+
+#define MAX_ERRNO 4095
+#define IS_ERR_VALUE(x) unlikely((unsigned long)(void *)(x) >= (unsigned long)-MAX_ERRNO)
+
+struct iort_its_msi_chip {
+ struct list_head list;
+ struct fwnode_handle *fw_node;
+ u32 translation_id;
+};
+
+struct iort_fwnode {
+ struct list_head list;
+ struct acpi_iort_node *iort_node;
+ struct fwnode_handle *fwnode;
+};
+
+static LIST_HEAD(iort_fwnode_list);
+static DEFINE_SPINLOCK(iort_fwnode_lock);
+const struct fwnode_operations acpi_static_fwnode_ops;
+
+/**
+ * iort_set_fwnode() - Create iort_fwnode and use it to register
+ * iommu data in the iort_fwnode_list
+ *
+ * @node: IORT table node associated with the IOMMU
+ * @fwnode: fwnode associated with the IORT node
+ *
+ * Returns: 0 on success
+ * <0 on failure
+ */
+static inline int iort_set_fwnode(struct acpi_iort_node *iort_node,
+ struct fwnode_handle *fwnode)
+{
+ struct iort_fwnode *np;
+
+ np = xzalloc(struct iort_fwnode);
+
+ if ( WARN_ON(!np) )
+ return -ENOMEM;
+
+ INIT_LIST_HEAD(&np->list);
+ np->iort_node = iort_node;
+ np->fwnode = fwnode;
+ spin_lock(&iort_fwnode_lock);
+ list_add_tail(&np->list, &iort_fwnode_list);
+ spin_unlock(&iort_fwnode_lock);
+
+ return 0;
+}
+
+/**
+ * iort_get_fwnode() - Retrieve fwnode associated with an IORT node
+ *
+ * @node: IORT table node to be looked-up
+ *
+ * Returns: fwnode_handle pointer on success, NULL on failure
+ */
+static inline
+struct fwnode_handle *iort_get_fwnode(struct acpi_iort_node *node)
+{
+ struct iort_fwnode *curr;
+ struct fwnode_handle *fwnode = NULL;
+ spin_lock(&iort_fwnode_lock);
+ list_for_each_entry(curr, &iort_fwnode_list, list)
+ {
+ if ( curr->iort_node == node )
+ {
+ fwnode = curr->fwnode;
+ break;
+ }
+ }
+ spin_unlock(&iort_fwnode_lock);
+
+ return fwnode;
+}
+
+/**
+ * iort_delete_fwnode() - Delete fwnode associated with an IORT node
+ *
+ * @node: IORT table node associated with fwnode to delete
+ */
+static inline void iort_delete_fwnode(struct acpi_iort_node *node)
+{
+ struct iort_fwnode *curr, *tmp;
+
+ spin_lock(&iort_fwnode_lock);
+ list_for_each_entry_safe(curr, tmp, &iort_fwnode_list, list)
+ {
+ if ( curr->iort_node == node )
+ {
+ list_del(&curr->list);
+ xfree(curr);
+ break;
+ }
+ }
+ spin_unlock(&iort_fwnode_lock);
+}
+
+typedef acpi_status (*iort_find_node_callback)
+ (struct acpi_iort_node *node, void *context);
+
+/* Root pointer to the mapped IORT table */
+static struct acpi_table_header *iort_table;
+
+static struct acpi_iort_node *iort_scan_node(enum acpi_iort_node_type type,
+ iort_find_node_callback callback,
+ void *context)
+{
+ struct acpi_iort_node *iort_node, *iort_end;
+ struct acpi_table_iort *iort;
+ int i;
+
+ if ( !iort_table )
+ return NULL;
+
+ /* Get the first IORT node */
+ iort = (struct acpi_table_iort *)iort_table;
+ iort_node = ACPI_ADD_PTR(struct acpi_iort_node, iort,
+ iort->node_offset);
+ iort_end = ACPI_ADD_PTR(struct acpi_iort_node, iort_table,
+ iort_table->length);
+
+ for ( i = 0; i < iort->node_count; i++ )
+ {
+ if ( WARN_TAINT(iort_node >= iort_end, TAINT_FIRMWARE_WORKAROUND,
+ "IORT node pointer overflows, bad table!\n") )
+ return NULL;
+
+ if ( iort_node->type == type &&
+ ACPI_SUCCESS(callback(iort_node, context)) )
+ return iort_node;
+
+ iort_node = ACPI_ADD_PTR(struct acpi_iort_node, iort_node,
+ iort_node->length);
+ }
+
+ return NULL;
+}
+
+static acpi_status iort_match_node_callback(struct acpi_iort_node *node,
+ void *context)
+{
+ struct device *dev = context;
+ acpi_status status = AE_NOT_FOUND;
+
+ if ( node->type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX )
+ {
+ struct acpi_iort_root_complex *pci_rc;
+ struct pci_dev *pdev;
+
+ pdev = to_pci_dev(dev);
+ pci_rc = (struct acpi_iort_root_complex *)node->node_data;
+
+ /*
+ * It is assumed that PCI segment numbers maps one-to-one
+ * with root complexes. Each segment number can represent only
+ * one root complex.
+ */
+ status = pci_rc->pci_segment_number == pci_domain_nr(pdev) ?
+ AE_OK : AE_NOT_FOUND;
+ }
+
+ return status;
+}
+
+static int arm_smmu_iort_xlate(struct device *dev, u32 streamid,
+ struct fwnode_handle *fwnode,
+ const struct iommu_ops *ops)
+{
+ int ret;
+ ret = iommu_fwspec_init(dev, fwnode, ops);
+
+ if ( !ret )
+ ret = iommu_fwspec_add_ids(dev, &streamid, 1);
+
+ return ret;
+}
+
+static inline
+const struct iommu_ops *iort_fwspec_iommu_ops(struct iommu_fwspec *fwspec)
+{
+ return (fwspec && fwspec->ops) ? fwspec->ops : NULL;
+}
+
+static int iort_iommu_xlate(struct device *dev, struct acpi_iort_node *node,
+ u32 streamid)
+{
+ const struct iommu_ops *ops;
+ struct fwnode_handle *iort_fwnode;
+
+ if ( !node )
+ return -ENODEV;
+
+ iort_fwnode = iort_get_fwnode(node);
+ if ( !iort_fwnode )
+ return -ENODEV;
+
+ /*
+ * If the ops look-up fails, this means that either
+ * the SMMU drivers have not been probed yet or that
+ * the SMMU drivers are not built in the kernel;
+ * Depending on whether the SMMU drivers are built-in
+ * in the kernel or not, defer the IOMMU configuration
+ * or just abort it.
+ */
+ ops = iommu_ops_from_fwnode(iort_fwnode);
+ if (!ops)
+ return -1;
+
+ return arm_smmu_iort_xlate(dev, streamid, iort_fwnode, ops);
+}
+
+struct iort_pci_alias_info {
+ struct device *dev;
+ struct acpi_iort_node *node;
+};
+
+static int iort_pci_iommu_init(struct pci_dev *pdev, u16 alias, void *data)
+{
+ struct iort_pci_alias_info *info = data;
+ struct acpi_iort_node *smmu_node;
+ u32 streamid;
+
+ if ( query_sid(info->node, alias, &streamid, &smmu_node) )
+ return iort_iommu_xlate(info->dev, smmu_node, streamid);
+
+ return -1;
+}
+
+int pci_for_each_dma_alias(struct pci_dev *pdev,
+ int (*fn)(struct pci_dev *pdev,
+ u16 alias, void *data), void *data)
+{
+ return fn(pdev, PCI_BDF2(pdev->bus, pdev->devfn), data);
+}
+
+/**
+ * iort_iommu_configure - Set-up IOMMU configuration for a device.
+ *
+ * @dev: device to configure
+ *
+ * Returns: iommu_ops pointer on configuration success
+ * NULL on configuration failure
+ */
+const struct iommu_ops *iort_iommu_configure(struct device *dev)
+{
+ struct acpi_iort_node *node;
+ const struct iommu_ops *ops;
+ int err = -ENODEV;
+
+ /*
+ * If we already translated the fwspec there
+ * is nothing left to do, return the iommu_ops.
+ */
+ ops = iort_fwspec_iommu_ops(dev->iommu_fwspec);
+ if ( ops )
+ return ops;
+
+ if ( dev_is_pci(dev) )
+ {
+ struct iort_pci_alias_info info = { .dev = dev };
+ node = iort_scan_node(
+ ACPI_IORT_NODE_PCI_ROOT_COMPLEX,
+ iort_match_node_callback,
+ dev);
+ if ( !node )
+ return NULL;
+
+ info.node = node;
+ err = pci_for_each_dma_alias(
+ to_pci_dev(dev),
+ iort_pci_iommu_init, &info);
+ }
+
+ return ops;
+}
+
+int iort_add_smmu_platform_device(struct acpi_iort_node *node)
+{
+ struct device *dev;
+ struct fwnode_handle *fwnode;
+ int ret;
+
+ dev = xzalloc(struct device);
+ if ( !dev )
+ return -ENOMEM;
+
+ dev->type = DEV_ACPI;
+ dev->acpi_node = node;
+
+ fwnode = iort_get_fwnode(node);
+
+ if ( !fwnode )
+ {
+ ret = -ENODEV;
+ goto end;
+ }
+
+ dev->fwnode = fwnode;
+ dev->iommu_fwspec = xzalloc(struct iommu_fwspec);
+
+ if ( !dev->iommu_fwspec )
+ {
+ ret = -ENOMEM;
+ goto end;
+ }
+
+ /* Call the acpi init functions for IOMMU devices */
+ ret = acpi_device_init(DEVICE_IOMMU, (void *) dev, node->type);
+end:
+ return ret;
+}
+
+static inline struct fwnode_handle *acpi_alloc_fwnode_static(void)
+{
+ struct fwnode_handle *fwnode;
+
+ fwnode = xzalloc(struct fwnode_handle);
+ if ( !fwnode )
+ return NULL;
+
+ fwnode->ops = &acpi_static_fwnode_ops;
+
+ return fwnode;
+}
+
+int __init acpi_iort_init(void)
+{
+ acpi_status status;
+
+ status = acpi_get_table(ACPI_SIG_IORT, 0, &iort_table);
+
+ if ( ACPI_FAILURE(status) )
+ {
+ if ( status != AE_NOT_FOUND )
+ {
+ const char *msg = acpi_format_exception(status);
+ printk("Failed to get table, %s\n", msg);
+ }
+
+ return -1;
+ }
+
+ return 0;
+}
+__initcall(acpi_iort_init);
diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
index b75d79b793..19a76f59d6 100644
--- a/xen/include/asm-arm/device.h
+++ b/xen/include/asm-arm/device.h
@@ -7,6 +7,7 @@ enum device_type
{
DEV_DT,
DEV_PCI,
+ DEV_ACPI,
};
struct dev_archdata {
@@ -18,9 +19,18 @@ struct device
{
enum device_type type;
#ifdef CONFIG_HAS_DEVICE_TREE
- struct dt_device_node *of_node; /* Used by drivers imported from Linux */
+ /*
+ * TODO: of_node is redundant by addition of fwnode.
+ * Will be cleaned in future, kept here for compatability
+ * with smmuv2 driver
+ */
+ struct dt_device_node *of_node;
+#endif
+#ifdef CONFIG_ACPI
+ void *acpi_node;
#endif
struct dev_archdata archdata;
+ struct fwnode_handle *fwnode;
struct iommu_fwspec *iommu_fwspec;
};
--
2.14.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH resend 13/13] Add code to parse IORT and prepare rid maps.
2018-03-13 15:20 [PATCH resend 00/13] acpi: arm: Add IORT Support mjaggi
` (11 preceding siblings ...)
2018-03-13 15:20 ` [PATCH resend 12/13] acpi: arm: Provide support for iort iommu configuration hooks mjaggi
@ 2018-03-13 15:20 ` mjaggi
12 siblings, 0 replies; 23+ messages in thread
From: mjaggi @ 2018-03-13 15:20 UTC (permalink / raw)
To: manish.jaggi, julien.grall, xen-devel, sameer.goel, jgross, sstabellini
Cc: Manish Jaggi, manish.jaggi
From: Manish Jaggi <mjaggi@caviumnetworks.com>
This patch adds code to parse the pci_rc, idmaps and smmu nodes to
- prepare idmaps (rid-sid rid-devid)
While resolving the ipmap from pcirc->smmu->its some fixup is required
this is done by fixup_rid_devid_map.
Signed-off-by: Manish Jaggi <manish.jaggi@linaro.org>
---
xen/arch/arm/acpi/iort.c | 207 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 207 insertions(+)
diff --git a/xen/arch/arm/acpi/iort.c b/xen/arch/arm/acpi/iort.c
index 4238611ef7..e7a7c027e5 100644
--- a/xen/arch/arm/acpi/iort.c
+++ b/xen/arch/arm/acpi/iort.c
@@ -379,6 +379,212 @@ static inline struct fwnode_handle *acpi_alloc_fwnode_static(void)
return fwnode;
}
+static inline bool __must_check IS_ERR_OR_NULL(__force const void *ptr)
+{
+ return unlikely(!ptr) || IS_ERR_VALUE((unsigned long)ptr);
+}
+
+static inline bool is_acpi_static_node(const struct fwnode_handle *fwnode)
+{
+ return !IS_ERR_OR_NULL(fwnode) &&
+ fwnode->ops == &acpi_static_fwnode_ops;
+}
+
+static inline void acpi_free_fwnode_static(struct fwnode_handle *fwnode)
+{
+ if (WARN_ON(!is_acpi_static_node(fwnode)))
+ return;
+
+ xfree(fwnode);
+}
+
+int fixup_rid_devid_map(struct acpi_iort_node *inode,
+ struct acpi_iort_id_mapping *pci_idmap,
+ struct acpi_iort_node *smmu_node)
+{
+
+ unsigned int p_input_base, p_output_base, p_id_count;
+ unsigned int s_input_base, s_output_base, s_id_count;
+ unsigned int delta, i;
+ int ret = 0;
+ struct acpi_iort_id_mapping *smmu_idmap = NULL;
+ struct acpi_iort_node *its_node;
+ struct acpi_table_iort *iort;
+
+ iort = (struct acpi_table_iort*) iort_table;
+
+ p_input_base = pci_idmap->input_base;
+ p_output_base = pci_idmap->output_base;
+ p_id_count = pci_idmap->id_count;
+
+ smmu_idmap = (struct acpi_iort_id_mapping*) ((u8*) smmu_node +
+ smmu_node->mapping_offset);
+
+ for ( i = 0; i < smmu_node->mapping_count; i++, smmu_idmap++ )
+ {
+ s_input_base = smmu_idmap->input_base;
+ s_output_base = smmu_idmap->output_base;
+ s_id_count = smmu_idmap->id_count;
+ its_node = ACPI_ADD_PTR(struct acpi_iort_node, iort,
+ smmu_idmap->output_reference);
+
+ if (s_input_base <= p_output_base)
+ {
+ int count;
+ if (s_input_base + s_id_count < p_output_base)
+ continue;
+
+ delta = p_output_base - s_input_base;
+ count = s_input_base + s_id_count <= p_output_base +
+ p_id_count ? s_id_count - delta : p_id_count;
+
+ ret = add_rid_devid_map (inode, its_node,
+ p_input_base,
+ s_output_base + delta,
+ count);
+ if (ret)
+ return ret;
+ }
+ else
+ {
+ int count;
+ if ( p_output_base + p_id_count < s_input_base )
+ continue;
+
+ delta = s_input_base - p_output_base;
+ count = s_input_base + s_id_count < p_output_base +
+ p_id_count ? s_id_count : p_id_count - delta;
+
+ ret = add_rid_devid_map (inode, its_node,
+ p_input_base + delta,
+ s_output_base, count);
+
+ if ( ret )
+ return ret;
+ }
+ }
+
+ return ret;
+}
+
+void parse_pcirc_node(struct acpi_iort_node *iort_node)
+{
+ int j, ret;
+ struct acpi_iort_id_mapping *idmap;
+ struct acpi_iort_node *onode;
+ struct acpi_table_iort *iort;
+
+ iort = (struct acpi_table_iort*) iort_table;
+ idmap = ACPI_ADD_PTR(struct acpi_iort_id_mapping, iort_node,
+ iort_node->mapping_offset);
+
+ /* iterate over idmap */
+ for ( j = 0; j < iort_node->mapping_count; j++ )
+ {
+ struct acpi_iort_node *its_node;
+ struct acpi_iort_node *smmu_node;
+ onode = ACPI_ADD_PTR(struct acpi_iort_node, iort,
+ idmap->output_reference);
+
+ switch (onode->type)
+ {
+ case ACPI_IORT_NODE_ITS_GROUP:
+
+ its_node = ACPI_ADD_PTR(struct acpi_iort_node, iort,
+ idmap->output_reference);
+
+ ret = add_rid_devid_map(iort_node, its_node,
+ idmap->input_base,
+ idmap->output_base,
+ idmap->id_count);
+ if ( ret )
+ {
+ printk("%s: add_rid_devid_map"
+ "failed with ret=%d \r\n",
+ __func__, ret);
+ break;
+ }
+ break;
+
+ case ACPI_IORT_NODE_SMMU:
+ case ACPI_IORT_NODE_SMMU_V3:
+
+ smmu_node = ACPI_ADD_PTR(struct acpi_iort_node,
+ iort_table,
+ idmap->output_reference);
+
+ ret = add_rid_sid_map(iort_node,
+ smmu_node,
+ idmap->input_base,
+ idmap->output_base,
+ idmap->id_count);
+ if ( ret )
+ {
+ printk("%s: add_rid_sid_map"
+ "failed with ret=%d \r\n",
+ __func__, ret);
+ break;
+ }
+
+ ret = fixup_rid_devid_map(iort_node, idmap, onode);
+ if ( ret )
+ {
+ printk("%s: fixup_rid_devid_map"
+ "failed with ret=%d \r\n",
+ __func__, ret);
+ break;
+ }
+ break;
+ }
+ idmap++;
+ }
+}
+
+void parse_smmu_node(struct acpi_iort_node *iort_node)
+{
+ int ret;
+ struct fwnode_handle *fwnode;
+ fwnode = acpi_alloc_fwnode_static();
+ if ( !fwnode )
+ return;
+
+ iort_set_fwnode(iort_node, fwnode);
+ ret = iort_add_smmu_platform_device(iort_node);
+ if ( ret )
+ acpi_free_fwnode_static(fwnode);
+}
+
+void parse_iort(void)
+{
+ struct acpi_iort_node *iort_node, *iort_end;
+ struct acpi_table_iort *iort;
+ int i;
+
+ iort = (struct acpi_table_iort*) iort_table;
+ iort_node = ACPI_ADD_PTR(struct acpi_iort_node, iort,
+ iort->node_offset);
+ iort_end = ACPI_ADD_PTR(struct acpi_iort_node, iort,
+ iort->header.length);
+
+ for (i = 0; i < iort->node_count; i++)
+ {
+ if ( iort_node >= iort_end )
+ {
+ printk("iort node pointer overflows, bad table\n");
+ return;
+ }
+
+ if ( iort_node->type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX )
+ parse_pcirc_node(iort_node);
+ else if ( (iort_node->type == ACPI_IORT_NODE_SMMU ||
+ iort_node->type == ACPI_IORT_NODE_SMMU_V3) )
+ parse_smmu_node(iort_node);
+
+ iort_node = ACPI_ADD_PTR(struct acpi_iort_node, iort_node,
+ iort_node->length);
+ }
+}
+
int __init acpi_iort_init(void)
{
acpi_status status;
@@ -396,6 +602,7 @@ int __init acpi_iort_init(void)
return -1;
}
+ parse_iort();
return 0;
}
__initcall(acpi_iort_init);
--
2.14.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH resend 01/13] acpi: arm: API: Populate/query rid-devid rid-sid map.
2018-03-13 15:20 ` [PATCH resend 01/13] acpi: arm: API: Populate/query rid-devid rid-sid map mjaggi
@ 2018-03-21 9:29 ` Julien Grall
2018-03-21 9:34 ` Manish Jaggi
2018-03-21 9:29 ` Julien Grall
1 sibling, 1 reply; 23+ messages in thread
From: Julien Grall @ 2018-03-21 9:29 UTC (permalink / raw)
To: mjaggi, manish.jaggi, xen-devel, sameer.goel, jgross, sstabellini
Cc: manish.jaggi
Hi Manish,
On 03/13/2018 03:20 PM, mjaggi@caviumnetworks.com wrote:
> From: Manish Jaggi <mjaggi@caviumnetworks.com>
>
> 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.
>
> Signed-off-by: Manish Jaggi <manish.jaggi@linaro.org>
> ---
> 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@linaro.org>
> + * 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.
> +{
> + 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@linaro.org>
> + * 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.
> + *
> + */
> +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,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH resend 01/13] acpi: arm: API: Populate/query rid-devid rid-sid map.
2018-03-13 15:20 ` [PATCH resend 01/13] acpi: arm: API: Populate/query rid-devid rid-sid map mjaggi
2018-03-21 9:29 ` Julien Grall
@ 2018-03-21 9:29 ` Julien Grall
1 sibling, 0 replies; 23+ messages in thread
From: Julien Grall @ 2018-03-21 9:29 UTC (permalink / raw)
To: mjaggi, manish.jaggi, xen-devel, sameer.goel, jgross, sstabellini
Cc: manish.jaggi
Title: Please drop the full stop.
On 03/13/2018 03:20 PM, mjaggi@caviumnetworks.com wrote:
> From: Manish Jaggi <mjaggi@caviumnetworks.com>
>
> 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 <manish.jaggi@linaro.org>
> ---
> 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@linaro.org>
> + * 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:
> + */
> 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@linaro.org>
> + * 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:
> + */
>
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH resend 01/13] acpi: arm: API: Populate/query rid-devid rid-sid map.
2018-03-21 9:29 ` Julien Grall
@ 2018-03-21 9:34 ` Manish Jaggi
2018-03-21 9:41 ` Julien Grall
0 siblings, 1 reply; 23+ messages in thread
From: Manish Jaggi @ 2018-03-21 9:34 UTC (permalink / raw)
To: Julien Grall, manish.jaggi, xen-devel, sameer.goel, jgross, sstabellini
Cc: manish.jaggi
On 03/21/2018 02:59 PM, Julien Grall wrote:
> Hi Manish,
Hi Julien,
>
> On 03/13/2018 03:20 PM, mjaggi@caviumnetworks.com wrote:
>> From: Manish Jaggi <mjaggi@caviumnetworks.com>
>>
>> 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@linaro.org>
>> ---
>> 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@linaro.org>
>> + * 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@linaro.org>
>> + * 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH resend 01/13] acpi: arm: API: Populate/query rid-devid rid-sid map.
2018-03-21 9:34 ` Manish Jaggi
@ 2018-03-21 9:41 ` Julien Grall
0 siblings, 0 replies; 23+ messages in thread
From: Julien Grall @ 2018-03-21 9:41 UTC (permalink / raw)
To: Manish Jaggi, manish.jaggi, xen-devel, sameer.goel, jgross, sstabellini
Cc: manish.jaggi
Hi,
On 03/21/2018 09:34 AM, Manish Jaggi wrote:
>
> On 03/21/2018 02:59 PM, Julien Grall wrote:
>> Hi Manish,
> Hi Julien,
>>
>> On 03/13/2018 03:20 PM, mjaggi@caviumnetworks.com wrote:
>>> From: Manish Jaggi <mjaggi@caviumnetworks.com>
>>>
>>> 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.
Mind to show where?
[...]
>>> +#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 ?
The question is do you expect this function to be called after Xen has
booted. Likely not, so this should be public to shrink down the size of
Xen after boot.
[...]
>>> +/*
>>> + * 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.
Use case belongs to the commit message. The comment code should only
describe the expected usage or a couple of words for the use case (but
that usually part of the description of the function).
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH resend 02/13] acpi: arm: query estimated size of hardware domain's IORT.
2018-03-13 15:20 ` [PATCH resend 02/13] acpi: arm: query estimated size of hardware domain's IORT mjaggi
@ 2018-03-21 10:12 ` Julien Grall
2018-03-28 11:24 ` Manish Jaggi
0 siblings, 1 reply; 23+ messages in thread
From: Julien Grall @ 2018-03-21 10:12 UTC (permalink / raw)
To: mjaggi, manish.jaggi, xen-devel, sameer.goel, jgross, sstabellini
Cc: Manish Jaggi, manish.jaggi
Title: Please drop the full stop.
On 03/13/2018 03:20 PM, mjaggi@caviumnetworks.com wrote:
> From: Manish Jaggi <mjaggi@caviumnetworks.com>
>
> 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.
Please expand: "Xen code requires the size to be predeterminded".
>
> Signed-off-by: Manish Jaggi <manish.jaggi@linaro.com>
> ---
> 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(-)
>
> diff --git a/xen/arch/arm/acpi/Makefile b/xen/arch/arm/acpi/Makefile
> index eb7e8ce4f7..073339603c 100644
> --- a/xen/arch/arm/acpi/Makefile
> +++ b/xen/arch/arm/acpi/Makefile
> @@ -1,3 +1,4 @@
> obj-y += lib.o
> obj-y += boot.init.o
> obj-y += ridmap.o
> +obj-y += gen-iort.o
> diff --git a/xen/arch/arm/acpi/gen-iort.c b/xen/arch/arm/acpi/gen-iort.c
> new file mode 100644
> index 0000000000..687c4f18ee
> --- /dev/null
> +++ b/xen/arch/arm/acpi/gen-iort.c
> @@ -0,0 +1,101 @@
> +/*
> + * xen/arch/arm/acpi/gen-iort.c
> + *
> + * Code to generate IORT for hardware domain using the requesterId
> + * and deviceId map.
Here we go. You again mix upper case and lower case for the same name.
To be honest, I would much prefer if you stick with deviceId. I.e first
letter of the second word is uppercase.
> + *
> + * Manish Jaggi <manish.jaggi@linaro.com>
It is linaro.org.
> + * 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/acpi.h>
I am a bit surprised that only those 2 headers are necessary.
> +
> +/*
> + * Size of hardware domains' IORT is calculated based on the number of
> + * mappings in the requesterid - deviceid mapping list.
> + * Return value 0: Success
> + */
> +int estimate_iort_size(size_t *iort_size)
__init.
> +{
> + int count = 0;
> + int pcirc_count = 0;
> + int itsg_count = 0;
All the 3 variables above should be unsigned int.
> + uint64_t *pcirc_array;
> + uint64_t *itsg_array;
Again, what is the rationale to store the address directly rather than
"void *"? This would avoid the cast in the code.
> + 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."
> +
> + list_for_each_entry(rmap, &rid_devid_list, entry)
> + count++;
> +
> + pcirc_array = xzalloc_bytes(sizeof(uint64_t)*count);
> + if ( !pcirc_array )
> + return -ENOMEM;
> +
> + itsg_array = xzalloc_bytes(sizeof(uint64_t)*count);
> + if ( !itsg_array )
> + return -ENOMEM;
If the allocation fail, you will leak pcirc_array.
> +
> + list_for_each_entry(rmap, &rid_devid_list, entry)
> + {
> + int i = 0;
unsigned.
> +
> + 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;
> + if ( i == itsg_count )
> + {
> + itsg_array[i] = (uint64_t) rmap->its_node;
> + itsg_count++;
> + break;
> + }
> + }
> + }
> +
> + /* Size of IORT
Coding style.
> + * = Size of IORT Table Header + Size of PCIRC Header Nodes +
> + * Size of PCIRC nodes + Size of ITS Header nodes + Size of ITS Nodes
> + * + Size of idmap nodes
> + */
> + *iort_size = sizeof(struct acpi_table_iort) +
> + pcirc_count*( (sizeof(struct acpi_iort_node) -1) +
> + sizeof(struct acpi_iort_root_complex) ) +
> + itsg_count*( (sizeof(struct acpi_iort_node) -1) +
> + sizeof(struct acpi_iort_its_group) ) +
> + count*( sizeof(struct acpi_iort_id_mapping) );
Coding style in general:
- No space needed after ( in that cases
- Space before and after +*-
> +
> + 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:
> + */
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 155c952349..33a46cab1e 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -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>
> @@ -1801,7 +1802,7 @@ static int acpi_create_fadt(struct domain *d, struct membank tbl_add[])
>
> static int estimate_acpi_efi_size(struct domain *d, struct kernel_info *kinfo)
> {
> - size_t efi_size, acpi_size, madt_size;
> + size_t efi_size, acpi_size, table_size;
I am ok if you keep the renaming in that patch. But you should at least
mention it in the commit message.
> u64 addr;
> struct acpi_table_rsdp *rsdp_tbl;
> struct acpi_table_header *table;
> @@ -1811,8 +1812,8 @@ 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 = gic_get_hwdom_madt_size(d);
> - acpi_size += ROUNDUP(madt_size, 8);
> + table_size = gic_get_hwdom_madt_size(d);
> + acpi_size += ROUNDUP(table_size, 8);
>
> addr = acpi_os_get_root_pointer();
> if ( !addr )
> @@ -1842,6 +1843,15 @@ static int estimate_acpi_efi_size(struct domain *d, struct kernel_info *kinfo)
> acpi_os_unmap_memory(table, sizeof(struct acpi_table_header));
>
> acpi_size += ROUNDUP(sizeof(struct acpi_table_rsdp), 8);
> +
> + if ( estimate_iort_size(&table_size) )
> + {
> + printk("Unable to get hwdom iort size\n");
> + return -EINVAL;
> + }
> +
> + acpi_size += ROUNDUP(table_size, 8);
> +
> d->arch.efi_acpi_len = PAGE_ALIGN(ROUNDUP(efi_size, 8)
> + ROUNDUP(acpi_size, 8));
>
> diff --git a/xen/include/asm-arm/acpi/gen-iort.h b/xen/include/asm-arm/acpi/gen-iort.h
> new file mode 100644
> index 0000000000..3b2af1e871
> --- /dev/null
> +++ b/xen/include/asm-arm/acpi/gen-iort.h
Is it worth it to have a separate header for gen-iort? How about using
acpi.h?
> @@ -0,0 +1,33 @@
> +/*
> + * xen/include/asm-arm/acpi/gen-iort.h
> + *
> + * Manish Jaggi <manish.jaggi@linaro.org>
> + * 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);
> +
> +#endif
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH resend 03/13] acpi: arm: Code to generate Hardware Domains IORT
2018-03-13 15:20 ` [PATCH resend 03/13] acpi: arm: Code to generate Hardware Domains IORT mjaggi
@ 2018-03-23 1:28 ` Julien Grall
2018-03-23 4:17 ` Manish Jaggi
0 siblings, 1 reply; 23+ messages in thread
From: Julien Grall @ 2018-03-23 1:28 UTC (permalink / raw)
To: mjaggi, manish.jaggi, xen-devel, sameer.goel, jgross, sstabellini
Cc: manish.jaggi
Hi Manish,
On 03/13/2018 03:20 PM, mjaggi@caviumnetworks.com wrote:
> From: Manish Jaggi <mjaggi@caviumnetworks.com>
>
> 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 <manish.jaggi@linaro.org>
> ---
> 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(+)
>
> diff --git a/xen/arch/arm/acpi/gen-iort.c b/xen/arch/arm/acpi/gen-iort.c
> index 687c4f18ee..251a9771e3 100644
> --- a/xen/arch/arm/acpi/gen-iort.c
> +++ b/xen/arch/arm/acpi/gen-iort.c
> @@ -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 parsing IORT
> + * is used to generate hwdom IORT.
> + *
> + * One of the challanges is to fixup node offset of ITS Group Nodes
s/challanges/challenges/
> + * 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
> + * 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.
> +
> +/*
> + * 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.
> + *
> + * 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.
> + unsigned int offset)
> +{
> + 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?
> + 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)
> +{
> + struct fwits_hwits_map *map;
> +
> + list_for_each_entry(map, &fwits_hwits_list, entry)
> + {
> + if ( map->fwits_node == fwits_node )
> + return map->hwitsnode_offset;
> + }
> +
> + return 0;
0 could never be a valid offset, right?
> +}
> +
> +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 *.
> + 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?
> + 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
> + * 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.
> + {
> + 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.
> +
> + 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.
> + 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.
> +
> + 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;
> + 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,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH resend 03/13] acpi: arm: Code to generate Hardware Domains IORT
2018-03-23 1:28 ` Julien Grall
@ 2018-03-23 4:17 ` Manish Jaggi
2018-03-23 4:55 ` Julien Grall
0 siblings, 1 reply; 23+ messages in thread
From: Manish Jaggi @ 2018-03-23 4:17 UTC (permalink / raw)
To: Julien Grall, manish.jaggi, xen-devel, sameer.goel, jgross, sstabellini
Cc: manish.jaggi
On 03/23/2018 06:58 AM, Julien Grall wrote:
> Hi Manish,
>
> On 03/13/2018 03:20 PM, mjaggi@caviumnetworks.com wrote:
>> From: Manish Jaggi <mjaggi@caviumnetworks.com>
>>
>> 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 <manish.jaggi@linaro.org>
>> ---
>> 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(+)
>>
>> diff --git a/xen/arch/arm/acpi/gen-iort.c b/xen/arch/arm/acpi/gen-iort.c
>> index 687c4f18ee..251a9771e3 100644
>> --- a/xen/arch/arm/acpi/gen-iort.c
>> +++ b/xen/arch/arm/acpi/gen-iort.c
>> @@ -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
>> parsing IORT
>> + * is used to generate hwdom IORT.
>> + *
>> + * One of the challanges is to fixup node offset of ITS Group Nodes
>
> s/challanges/challenges/
>
>> + * 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
>> + * 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.
>
>> +
>> +/*
>> + * 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.
>
>> + *
>> + * 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...
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.
>
> But you likely want to rename the function to add_fwits_node(...) or
> something similar.
I think name is quite appropriate.
>
>> + unsigned int offset)
>> +{
>> + 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.
>> + 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)
>> +{
>> + struct fwits_hwits_map *map;
>> +
>> + list_for_each_entry(map, &fwits_hwits_list, entry)
>> + {
>> + 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.
>
>> +}
>> +
>> +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 *.
>
>> + 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.
>
>> + 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
>> + * 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.
ok.
>
>> + {
>> + 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.
>
>> +
>> + 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?
> Then you would end up to have the wrong revision number here.
>
>> + 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.
>
ok
>> +
>> + 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;
>> + 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.
>
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.
> Cheers,
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH resend 03/13] acpi: arm: Code to generate Hardware Domains IORT
2018-03-23 4:17 ` Manish Jaggi
@ 2018-03-23 4:55 ` Julien Grall
0 siblings, 0 replies; 23+ messages in thread
From: Julien Grall @ 2018-03-23 4:55 UTC (permalink / raw)
To: Manish Jaggi, manish.jaggi, xen-devel, sameer.goel, jgross, sstabellini
Cc: manish.jaggi
Hi,
On 03/23/2018 04:17 AM, Manish Jaggi wrote:
> On 03/23/2018 06:58 AM, Julien Grall wrote:
>> On 03/13/2018 03:20 PM, mjaggi@caviumnetworks.com 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,
>>
>> 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.
The fact that a function calling is_* will add the element to the list.
An is_* function should only check the element is in the list.
So yes, it is not intuitive for me.
>>
>> But you likely want to rename the function to add_fwits_node(...) or
>> something similar.
> I think name is quite appropriate.
See above.
>>
>>> + unsigned int offset)
>>> +{
>>> + 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.
IORT for DomU will not rely on the host firmware and will be created by
the toolstack. It does not make sense to keep that around.
>>> + 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)
>>> +{
>>> + struct fwits_hwits_map *map;
>>> +
>>> + list_for_each_entry(map, &fwits_hwits_list, entry)
>>> + {
>>> + 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.
I don't much care about the BUG_ON()... I was only checking what the
spec says here.
But... you document the return really well on the previous function and
here it is seems to be forgotten.
>>
>>> +}
>>> +
>>> +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 *.
>>
>>> + 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.
It does not answer my question.
>>
>>> + 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
>>> + * 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.
> ok.
>>
>>> + {
>>> + 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.
>>
>>> +
>>> + 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?
The host IORT will be built on top of a revision X. For the hwdom IORT,
at the moment, you are always building on top of revision 0.
While today X == 0, this may change in the future and will prevent
proper booting.
>>> +
>>> + 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;
>>> + 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.
>>
> 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.
I am afraid I am not going to have time reviewing for the next couple of
weeks.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH resend 02/13] acpi: arm: query estimated size of hardware domain's IORT.
2018-03-21 10:12 ` Julien Grall
@ 2018-03-28 11:24 ` Manish Jaggi
0 siblings, 0 replies; 23+ messages in thread
From: Manish Jaggi @ 2018-03-28 11:24 UTC (permalink / raw)
To: Julien Grall, manish.jaggi, xen-devel, sameer.goel, jgross, sstabellini
Cc: Manish Jaggi, manish.jaggi
Hi Julien,
On 03/21/2018 03:42 PM, Julien Grall wrote:
> Title: Please drop the full stop.
>
> On 03/13/2018 03:20 PM, mjaggi@caviumnetworks.com wrote:
> ...
>
>> + 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.
That will still require parsing of all IORT nodes.
I am add one more api to return total mappings, which would basically
return a counter which is updated when add_rid_deviceid_map is called.
This would replace
+ list_for_each_entry(rmap, &rid_devid_list, entry)
+ count++;
+
Sounds good?
> They could be store with the rest of information."
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2018-03-28 11:24 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-13 15:20 [PATCH resend 00/13] acpi: arm: Add IORT Support mjaggi
2018-03-13 15:20 ` [PATCH resend 01/13] acpi: arm: API: Populate/query rid-devid rid-sid map mjaggi
2018-03-21 9:29 ` Julien Grall
2018-03-21 9:34 ` Manish Jaggi
2018-03-21 9:41 ` Julien Grall
2018-03-21 9:29 ` Julien Grall
2018-03-13 15:20 ` [PATCH resend 02/13] acpi: arm: query estimated size of hardware domain's IORT mjaggi
2018-03-21 10:12 ` Julien Grall
2018-03-28 11:24 ` Manish Jaggi
2018-03-13 15:20 ` [PATCH resend 03/13] acpi: arm: Code to generate Hardware Domains IORT mjaggi
2018-03-23 1:28 ` Julien Grall
2018-03-23 4:17 ` Manish Jaggi
2018-03-23 4:55 ` Julien Grall
2018-03-13 15:20 ` [PATCH resend 04/13] acpi: arm: Copy fwnode / iommu_fwspec code from Linux 4.14 mjaggi
2018-03-13 15:20 ` [PATCH resend 05/13] acpi: arm: Import acpi_iort.h verbatim from linux 4.14 mjaggi
2018-03-13 15:20 ` [PATCH resend 06/13] acpi: arm: Update acpi_iort.h with xen specific changes mjaggi
2018-03-13 15:20 ` [PATCH resend 07/13] arm: Adding ACPI_IORT in arm Kconfig mjaggi
2018-03-13 15:20 ` [PATCH resend 08/13] asm: arm: pci: Fix the #include label in asm-arm/pci.h mjaggi
2018-03-13 15:20 ` [PATCH resend 09/13] asm: arm: to_pci_dev mjaggi
2018-03-13 15:20 ` [PATCH resend 10/13] asm: arm: add dev_is_pci mjaggi
2018-03-13 15:20 ` [PATCH resend 11/13] asm: arm: add pci_domain_nr mjaggi
2018-03-13 15:20 ` [PATCH resend 12/13] acpi: arm: Provide support for iort iommu configuration hooks mjaggi
2018-03-13 15:20 ` [PATCH resend 13/13] Add code to parse IORT and prepare rid maps mjaggi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.