linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/8] ACPI/IORT: Support for IORT RMR node
@ 2021-05-24 11:02 Shameer Kolothum
  2021-05-24 11:02 ` [PATCH v5 1/8] ACPI/IORT: Add support for RMR node parsing Shameer Kolothum
                   ` (8 more replies)
  0 siblings, 9 replies; 40+ messages in thread
From: Shameer Kolothum @ 2021-05-24 11:02 UTC (permalink / raw)
  To: linux-arm-kernel, linux-acpi, iommu
  Cc: linuxarm, lorenzo.pieralisi, joro, robin.murphy, wanghuiqiang,
	guohanjun, steven.price, Sami.Mujawar, jon, eric.auger,
	yangyicong

Hi,

v4 --> v5
 -Added a fw_data union to struct iommu_resv_region and removed
  struct iommu_rmr (Based on comments from Joerg/Robin).
 -Added iommu_put_rmrs() to release mem.
 -Thanks to Steve for verifying v4 on SMMUv2, but not added the
  Tested-by yet because of the above changes.

v3 -->v4
-Included the SMMUv2 SMR bypass install changes suggested by
 Steve(patch #7)
-As per Robin's comments, RMR reserve implementation is now
 more generic  (patch #8) and dropped v3 patches 8 and 10.
-Rebase to 5.13-rc1 

RFC v2 --> v3
 -Dropped RFC tag as the ACPICA header changes are now ready to be
  part of 5.13[0]. But this series still has a dependency on that patch.
 -Added IORT E.b related changes(node flags, _DSM function 5 checks for
  PCIe).
 -Changed RMR to stream id mapping from M:N to M:1 as per the spec and
  discussion here[1].
 -Last two patches add support for SMMUv2(Thanks to Jon Nettleton!) 

Sanity tested on a HiSilicon D06. Further testing and feedback is greatly
appreciated.

The whole series can be found here,
https://github.com/hisilicon/kernel-dev/tree/private-v5.12-rc8-rmr-v3

Thanks,
Shameer

[0] https://lore.kernel.org/linux-acpi/20210406213028.718796-22-erik.kaneda@intel.com/
[1] https://op-lists.linaro.org/pipermail/linaro-open-discussions/2021-April/000150.html

RFC v1 --> v2:
 - Added a generic interface for IOMMU drivers to retrieve all the 
   RMR info associated with a given IOMMU.
 - SMMUv3 driver gets the RMR list during probe() and installs
   bypass STEs for all the SIDs in the RMR list. This is to keep
   the ongoing traffic alive(if any) during SMMUv3 reset. This is
   based on the suggestions received for v1 to take care of the
   EFI framebuffer use case. Only sanity tested for now.
 - During the probe/attach device, SMMUv3 driver reserves any
   RMR region associated with the device such that there is a unity
   mapping for them in SMMU.
---    

From RFC v1:
-------------
The series adds support to IORT RMR nodes specified in IORT
Revision E -ARM DEN 0049E[0]. RMR nodes are used to describe memory
ranges that are used by endpoints and require a unity mapping
in SMMU.

We have faced issues with 3408iMR RAID controller cards which
fail to boot when SMMU is enabled. This is because these controllers
make use of host memory for various caching related purposes and when
SMMU is enabled the iMR firmware fails to access these memory regions
as there is no mapping for them. IORT RMR provides a way for UEFI to
describe and report these memory regions so that the kernel can make
a unity mapping for these in SMMU.

Tests:

With a UEFI, that reports the RMR for the dev,
....
[16F0h 5872   1]                         Type : 06
[16F1h 5873   2]                       Length : 007C
[16F3h 5875   1]                     Revision : 00
[1038h 0056   2]                     Reserved : 00000000
[1038h 0056   2]                   Identifier : 00000000
[16F8h 5880   4]                Mapping Count : 00000001
[16FCh 5884   4]               Mapping Offset : 00000040

[1700h 5888   4]    Number of RMR Descriptors : 00000002
[1704h 5892   4]        RMR Descriptor Offset : 00000018

[1708h 5896   8]          Base Address of RMR : 0000E6400000
[1710h 5904   8]                Length of RMR : 000000100000
[1718h 5912   4]                     Reserved : 00000000

[171Ch 5916   8]          Base Address of RMR : 0000000027B00000
[1724h 5924   8]                Length of RMR : 0000000000C00000
[172Ch 5932   4]                     Reserved : 00000000

[1730h 5936   4]                   Input base : 00000000
[1734h 5940   4]                     ID Count : 00000001
[1738h 5944   4]                  Output Base : 00000003
[173Ch 5948   4]             Output Reference : 00000064
[1740h 5952   4]        Flags (decoded below) : 00000001
                               Single Mapping : 1
...

Without the series the RAID controller initialization fails as
below,

...
[   12.631117] megaraid_sas 0000:03:00.0: FW supports sync cache        : Yes   
[   12.637360] megaraid_sas 0000:03:00.0: megasas_disable_intr_fusion is called outbound_intr_mask:0x40000009                                                   
[   18.776377] megaraid_sas 0000:03:00.0: Init cmd return status FAILED for SCSI host 0                                                                         
[   23.019383] megaraid_sas 0000:03:00.0: Waiting for FW to come to ready state 
[  106.684281] megaraid_sas 0000:03:00.0: FW in FAULT state, Fault code:0x10000 subcode:0x0 func:megasas_transition_to_ready                                    
[  106.695186] megaraid_sas 0000:03:00.0: System Register set:                  
[  106.889787] megaraid_sas 0000:03:00.0: Failed to transition controller to ready for scsi0.                                                                   
[  106.910475] megaraid_sas 0000:03:00.0: Failed from megasas_init_fw 6407      
estuary:/$

With the series, now the kernel has direct mapping for the dev as
below,

estuary:/$ cat /sys/kernel/iommu_groups/0/reserved_regions                      
0x0000000008000000 0x00000000080fffff msi                                       
0x0000000027b00000 0x00000000286fffff direct                                    
0x00000000e6400000 0x00000000e64fffff direct                                    
estuary:/$

....
[   12.254318] megaraid_sas 0000:03:00.0: megasas_disable_intr_fusion is called outbound_intr_mask:0x40000009                                                   
[   12.739089] megaraid_sas 0000:03:00.0: FW provided supportMaxExtLDs: 0      max_lds: 32                                                                      
[   12.746628] megaraid_sas 0000:03:00.0: controller type       : iMR(0MB)      
[   12.752694] megaraid_sas 0000:03:00.0: Online Controller Reset(OCR)  : Enabled                                                                               
[   12.759798] megaraid_sas 0000:03:00.0: Secure JBOD support   : Yes           
[   12.765778] megaraid_sas 0000:03:00.0: NVMe passthru support : Yes           
[   12.771931] megaraid_sas 0000:03:00.0: FW provided TM TaskAbort/Reset timeou: 6 secs/60 secs                                                                 
[   12.780503] megaraid_sas 0000:03:00.0: JBOD sequence map support     : Yes   
[   12.787000] megaraid_sas 0000:03:00.0: PCI Lane Margining support    : No    
[   12.819179] megaraid_sas 0000:03:00.0: NVME page size        : (4096)        
[   12.825672] megaraid_sas 0000:03:00.0: megasas_enable_intr_fusion is called outbound_intr_mask:0x40000000                                                    
[   12.835199] megaraid_sas 0000:03:00.0: INIT adapter done                     
[   12.873932] megaraid_sas 0000:03:00.0: pci id                : (0x1000)/(0x0017)/(0x19e5)/(0xd213)                                                           
[   12.881644] megaraid_sas 0000:03:00.0: unevenspan support    : no            
[   12.887451] megaraid_sas 0000:03:00.0: firmware crash dump   : no            
[   12.893344] megaraid_sas 0000:03:00.0: JBOD sequence map     : enabled       

RAID controller init is now success and can detect the drives
attached as well.

Jon Nettleton (1):
  iommu/arm-smmu: Get associated RMR info and install bypass SMR

Shameer Kolothum (7):
  ACPI/IORT: Add support for RMR node parsing
  iommu/dma: Introduce generic helper to retrieve RMR info
  ACPI/IORT: Add a helper to retrieve RMR memory regions
  iommu/arm-smmu-v3: Introduce strtab init helper
  iommu/arm-smmu-v3: Add bypass flag to arm_smmu_write_strtab_ent()
  iommu/arm-smmu-v3: Get associated RMR info and install
  iommu/dma: Reserve any RMR regions associated with a dev

 drivers/acpi/arm64/iort.c                   | 154 +++++++++++++++++++-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |  72 ++++++---
 drivers/iommu/arm/arm-smmu/arm-smmu.c       |  65 +++++++++
 drivers/iommu/dma-iommu.c                   |  89 ++++++++++-
 include/linux/acpi_iort.h                   |   7 +
 include/linux/dma-iommu.h                   |  13 ++
 include/linux/iommu.h                       |  10 ++
 7 files changed, 387 insertions(+), 23 deletions(-)

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH v5 1/8] ACPI/IORT: Add support for RMR node parsing
  2021-05-24 11:02 [PATCH v5 0/8] ACPI/IORT: Support for IORT RMR node Shameer Kolothum
@ 2021-05-24 11:02 ` Shameer Kolothum
  2021-06-14 11:14   ` Robin Murphy
  2021-05-24 11:02 ` [PATCH v5 2/8] iommu/dma: Introduce generic helper to retrieve RMR info Shameer Kolothum
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 40+ messages in thread
From: Shameer Kolothum @ 2021-05-24 11:02 UTC (permalink / raw)
  To: linux-arm-kernel, linux-acpi, iommu
  Cc: linuxarm, lorenzo.pieralisi, joro, robin.murphy, wanghuiqiang,
	guohanjun, steven.price, Sami.Mujawar, jon, eric.auger,
	yangyicong

Add support for parsing RMR node information from ACPI.
Find associated stream id and smmu node info from the
RMR node and populate a linked list with RMR memory
descriptors.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 drivers/acpi/arm64/iort.c | 104 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 103 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 3912a1f6058e..fea1ffaedf3b 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -40,6 +40,19 @@ struct iort_fwnode {
 static LIST_HEAD(iort_fwnode_list);
 static DEFINE_SPINLOCK(iort_fwnode_lock);
 
+/*
+ * One entry for IORT RMR.
+ */
+struct iort_rmr_entry {
+	struct list_head list;
+	u32 sid;
+	struct acpi_iort_node *smmu;
+	struct acpi_iort_rmr_desc *rmr_desc;
+	u32 flags;
+};
+
+static LIST_HEAD(iort_rmr_list);         /* list of RMR regions from ACPI */
+
 /**
  * iort_set_fwnode() - Create iort_fwnode and use it to register
  *		       iommu data in the iort_fwnode_list
@@ -393,7 +406,8 @@ static struct acpi_iort_node *iort_node_get_id(struct acpi_iort_node *node,
 		if (node->type == ACPI_IORT_NODE_NAMED_COMPONENT ||
 		    node->type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX ||
 		    node->type == ACPI_IORT_NODE_SMMU_V3 ||
-		    node->type == ACPI_IORT_NODE_PMCG) {
+		    node->type == ACPI_IORT_NODE_PMCG ||
+		    node->type == ACPI_IORT_NODE_RMR) {
 			*id_out = map->output_base;
 			return parent;
 		}
@@ -1660,6 +1674,91 @@ static void __init iort_enable_acs(struct acpi_iort_node *iort_node)
 #else
 static inline void iort_enable_acs(struct acpi_iort_node *iort_node) { }
 #endif
+static int iort_rmr_desc_valid(struct acpi_iort_rmr_desc *desc, u32 count)
+{
+	int i, j;
+
+	for (i = 0; i < count; i++) {
+		u64 end, start = desc[i].base_address, length = desc[i].length;
+
+		if (!IS_ALIGNED(start, SZ_64K) || !IS_ALIGNED(length, SZ_64K))
+			return -EINVAL;
+
+		end = start + length - 1;
+
+		/* Check for address overlap */
+		for (j = i + 1; j < count; j++) {
+			u64 e_start = desc[j].base_address;
+			u64 e_end = e_start + desc[j].length - 1;
+
+			if (start <= e_end && end >= e_start)
+				return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
+static int __init iort_parse_rmr(struct acpi_iort_node *iort_node)
+{
+	struct acpi_iort_node *smmu;
+	struct iort_rmr_entry *e;
+	struct acpi_iort_rmr *rmr;
+	struct acpi_iort_rmr_desc *rmr_desc;
+	u32 map_count = iort_node->mapping_count;
+	u32  sid;
+	int i, ret = 0;
+
+	if (iort_node->type != ACPI_IORT_NODE_RMR)
+		return 0;
+
+	if (!iort_node->mapping_offset || map_count != 1) {
+		pr_err(FW_BUG "Invalid ID mapping, skipping RMR node %p\n",
+		       iort_node);
+		return -EINVAL;
+	}
+
+	/* Retrieve associated smmu and stream id */
+	smmu = iort_node_get_id(iort_node, &sid, 0);
+	if (!smmu) {
+		pr_err(FW_BUG "Invalid SMMU reference, skipping RMR node %p\n",
+		       iort_node);
+		return -EINVAL;
+	}
+
+	/* Retrieve RMR data */
+	rmr = (struct acpi_iort_rmr *)iort_node->node_data;
+	if (!rmr->rmr_offset || !rmr->rmr_count) {
+		pr_err(FW_BUG "Invalid RMR descriptor array, skipping RMR node %p\n",
+		       iort_node);
+		return -EINVAL;
+	}
+
+	rmr_desc = ACPI_ADD_PTR(struct acpi_iort_rmr_desc, iort_node,
+				rmr->rmr_offset);
+
+	ret = iort_rmr_desc_valid(rmr_desc, rmr->rmr_count);
+	if (ret) {
+		pr_err(FW_BUG "Invalid RMR descriptor[%d] for node %p, skipping...\n",
+		       i, iort_node);
+		return ret;
+	}
+
+	for (i = 0; i < rmr->rmr_count; i++, rmr_desc++) {
+		e = kmalloc(sizeof(*e), GFP_KERNEL);
+		if (!e)
+			return -ENOMEM;
+
+		e->sid = sid;
+		e->smmu = smmu;
+		e->rmr_desc = rmr_desc;
+		e->flags = rmr->flags;
+
+		list_add_tail(&e->list, &iort_rmr_list);
+	}
+
+	return 0;
+}
 
 static void __init iort_init_platform_devices(void)
 {
@@ -1689,6 +1788,9 @@ static void __init iort_init_platform_devices(void)
 
 		iort_enable_acs(iort_node);
 
+		if (iort_table->revision == 3)
+			iort_parse_rmr(iort_node);
+
 		ops = iort_get_dev_cfg(iort_node);
 		if (ops) {
 			fwnode = acpi_alloc_fwnode_static();
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [PATCH v5 2/8] iommu/dma: Introduce generic helper to retrieve RMR info
  2021-05-24 11:02 [PATCH v5 0/8] ACPI/IORT: Support for IORT RMR node Shameer Kolothum
  2021-05-24 11:02 ` [PATCH v5 1/8] ACPI/IORT: Add support for RMR node parsing Shameer Kolothum
@ 2021-05-24 11:02 ` Shameer Kolothum
  2021-05-24 15:35   ` kernel test robot
  2021-05-24 11:02 ` [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory regions Shameer Kolothum
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 40+ messages in thread
From: Shameer Kolothum @ 2021-05-24 11:02 UTC (permalink / raw)
  To: linux-arm-kernel, linux-acpi, iommu
  Cc: linuxarm, lorenzo.pieralisi, joro, robin.murphy, wanghuiqiang,
	guohanjun, steven.price, Sami.Mujawar, jon, eric.auger,
	yangyicong

Reserved Memory Regions(RMR) associated with an IOMMU can be
described through ACPI IORT tables in systems with devices
that require a unity mapping or bypass for those
regions.

Introduce a generic interface so that IOMMU drivers can retrieve
and set up necessary mappings. Also introduce a union to struct
iommu_resv_region to hold any firmware specific data(eg:RMR
specific info).

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 drivers/iommu/dma-iommu.c | 29 +++++++++++++++++++++++++++++
 include/linux/dma-iommu.h | 13 +++++++++++++
 include/linux/iommu.h     | 10 ++++++++++
 3 files changed, 52 insertions(+)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 7bcdd1205535..229ec65d98be 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -174,6 +174,35 @@ void iommu_put_dma_cookie(struct iommu_domain *domain)
 }
 EXPORT_SYMBOL(iommu_put_dma_cookie);
 
+/**
+ *
+ * iommu_dma_get_rmrs - Retrieve Reserved Memory Regions(RMRs) associated
+ *                      with a given IOMMU
+ * @iommu_fwnode: fwnode associated with IOMMU
+ * @list: RMR list to be populated
+ *
+ */
+int iommu_dma_get_rmrs(struct fwnode_handle *iommu_fwnode,
+		       struct list_head *list)
+{
+	return -EINVAL;
+}
+EXPORT_SYMBOL(iommu_dma_get_rmrs);
+
+/**
+ *
+ * iommu_dma_put_rmrs - Release Reserved Memory Regions(RMRs) associated
+ *                      with a given IOMMU
+ * @iommu_fwnode: fwnode associated with IOMMU
+ * @list: RMR list
+ *
+ */
+void iommu_dma_put_rmrs(struct fwnode_handle *iommu_fwnode,
+			struct list_head *list)
+{
+}
+EXPORT_SYMBOL(iommu_dma_put_rmrs);
+
 /**
  * iommu_dma_get_resv_regions - Reserved region driver helper
  * @dev: Device from iommu_get_resv_regions()
diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
index 6e75a2d689b4..a83e6c09444c 100644
--- a/include/linux/dma-iommu.h
+++ b/include/linux/dma-iommu.h
@@ -42,12 +42,16 @@ void iommu_dma_free_cpu_cached_iovas(unsigned int cpu,
 
 extern bool iommu_dma_forcedac;
 
+int iommu_dma_get_rmrs(struct fwnode_handle *iommu, struct list_head *list);
+void iommu_dma_put_rmrs(struct fwnode_handle *iommu, struct list_head *list);
+
 #else /* CONFIG_IOMMU_DMA */
 
 struct iommu_domain;
 struct msi_desc;
 struct msi_msg;
 struct device;
+struct fwnode_handle;
 
 static inline void iommu_setup_dma_ops(struct device *dev, u64 dma_base,
 		u64 size)
@@ -83,5 +87,14 @@ static inline void iommu_dma_get_resv_regions(struct device *dev, struct list_he
 {
 }
 
+int iommu_dma_get_rmrs(struct fwnode_handle *iommu, struct list_head *list)
+{
+	return -ENODEV;
+}
+
+void iommu_dma_put_rmrs(struct fwnode_handle *iommu, struct list_head *list)
+{
+}
+
 #endif	/* CONFIG_IOMMU_DMA */
 #endif	/* __DMA_IOMMU_H */
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 32d448050bf7..856a1736c744 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -114,6 +114,12 @@ enum iommu_resv_type {
 	IOMMU_RESV_SW_MSI,
 };
 
+struct iommu_iort_rmr_data {
+#define IOMMU_RMR_REMAP_PERMITTED	(1 << 0)
+	u32 flags;
+	u32 sid;	/* Stream Id associated with RMR entry */
+};
+
 /**
  * struct iommu_resv_region - descriptor for a reserved memory region
  * @list: Linked list pointers
@@ -121,6 +127,7 @@ enum iommu_resv_type {
  * @length: Length of the region in bytes
  * @prot: IOMMU Protection flags (READ/WRITE/...)
  * @type: Type of the reserved region
+ * @rmr: ACPI IORT RMR specific data
  */
 struct iommu_resv_region {
 	struct list_head	list;
@@ -128,6 +135,9 @@ struct iommu_resv_region {
 	size_t			length;
 	int			prot;
 	enum iommu_resv_type	type;
+	union {
+		struct iommu_iort_rmr_data rmr;
+	} fw_data;
 };
 
 /**
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory regions
  2021-05-24 11:02 [PATCH v5 0/8] ACPI/IORT: Support for IORT RMR node Shameer Kolothum
  2021-05-24 11:02 ` [PATCH v5 1/8] ACPI/IORT: Add support for RMR node parsing Shameer Kolothum
  2021-05-24 11:02 ` [PATCH v5 2/8] iommu/dma: Introduce generic helper to retrieve RMR info Shameer Kolothum
@ 2021-05-24 11:02 ` Shameer Kolothum
  2021-05-26  7:53   ` Laurentiu Tudor
  2021-06-14 11:23   ` Robin Murphy
  2021-05-24 11:02 ` [PATCH v5 4/8] iommu/arm-smmu-v3: Introduce strtab init helper Shameer Kolothum
                   ` (5 subsequent siblings)
  8 siblings, 2 replies; 40+ messages in thread
From: Shameer Kolothum @ 2021-05-24 11:02 UTC (permalink / raw)
  To: linux-arm-kernel, linux-acpi, iommu
  Cc: linuxarm, lorenzo.pieralisi, joro, robin.murphy, wanghuiqiang,
	guohanjun, steven.price, Sami.Mujawar, jon, eric.auger,
	yangyicong

Add a helper function that retrieves RMR memory descriptors
associated with a given IOMMU. This will be used by IOMMU
drivers to setup necessary mappings.

Now that we have this, invoke it from the generic helper
interface.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 drivers/acpi/arm64/iort.c | 50 +++++++++++++++++++++++++++++++++++++++
 drivers/iommu/dma-iommu.c |  4 ++++
 include/linux/acpi_iort.h |  7 ++++++
 3 files changed, 61 insertions(+)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index fea1ffaedf3b..01917caf58de 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -12,6 +12,7 @@
 
 #include <linux/acpi_iort.h>
 #include <linux/bitfield.h>
+#include <linux/dma-iommu.h>
 #include <linux/iommu.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
@@ -837,6 +838,53 @@ static inline int iort_add_device_replay(struct device *dev)
 	return err;
 }
 
+/**
+ * iort_iommu_get_rmrs - Helper to retrieve RMR info associated with IOMMU
+ * @iommu: fwnode for the IOMMU
+ * @head: RMR list head to be populated
+ *
+ * Returns: 0 on success, <0 failure
+ */
+int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
+			struct list_head *head)
+{
+	struct iort_rmr_entry *e;
+	struct acpi_iort_node *iommu;
+	int rmrs = 0;
+
+	iommu = iort_get_iort_node(iommu_fwnode);
+	if (!iommu || list_empty(&iort_rmr_list))
+		return -ENODEV;
+
+	list_for_each_entry(e, &iort_rmr_list, list) {
+		int prot = IOMMU_READ | IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
+		struct iommu_resv_region *region;
+		enum iommu_resv_type type;
+		struct acpi_iort_rmr_desc *rmr_desc;
+
+		if (e->smmu != iommu)
+			continue;
+
+		rmr_desc = e->rmr_desc;
+		if (e->flags & IOMMU_RMR_REMAP_PERMITTED)
+			type = IOMMU_RESV_DIRECT_RELAXABLE;
+		else
+			type = IOMMU_RESV_DIRECT;
+
+		region = iommu_alloc_resv_region(rmr_desc->base_address,
+						 rmr_desc->length,
+						 prot, type);
+		if (region) {
+			region->fw_data.rmr.flags = e->flags;
+			region->fw_data.rmr.sid = e->sid;
+			list_add_tail(&region->list, head);
+			rmrs++;
+		}
+	}
+
+	return (rmrs == 0) ? -ENODEV : 0;
+}
+
 /**
  * iort_iommu_msi_get_resv_regions - Reserved region driver helper
  * @dev: Device from iommu_get_resv_regions()
@@ -1108,6 +1156,8 @@ int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head)
 const struct iommu_ops *iort_iommu_configure_id(struct device *dev,
 						const u32 *input_id)
 { return NULL; }
+int iort_iommu_get_rmrs(struct fwnode_handle *fwnode, struct list_head *head)
+{ return -ENODEV; }
 #endif
 
 static int nc_dma_get_range(struct device *dev, u64 *size)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 229ec65d98be..f893d460cfa4 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -185,6 +185,9 @@ EXPORT_SYMBOL(iommu_put_dma_cookie);
 int iommu_dma_get_rmrs(struct fwnode_handle *iommu_fwnode,
 		       struct list_head *list)
 {
+	if (!is_of_node(iommu_fwnode))
+		return iort_iommu_get_rmrs(iommu_fwnode, list);
+
 	return -EINVAL;
 }
 EXPORT_SYMBOL(iommu_dma_get_rmrs);
@@ -200,6 +203,7 @@ EXPORT_SYMBOL(iommu_dma_get_rmrs);
 void iommu_dma_put_rmrs(struct fwnode_handle *iommu_fwnode,
 			struct list_head *list)
 {
+	generic_iommu_put_resv_regions(iommu_fwnode->dev, list);
 }
 EXPORT_SYMBOL(iommu_dma_put_rmrs);
 
diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
index 1a12baa58e40..e8c45fa59531 100644
--- a/include/linux/acpi_iort.h
+++ b/include/linux/acpi_iort.h
@@ -39,6 +39,8 @@ const struct iommu_ops *iort_iommu_configure_id(struct device *dev,
 						const u32 *id_in);
 int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head);
 phys_addr_t acpi_iort_dma_get_max_cpu_address(void);
+int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
+			struct list_head *list);
 #else
 static inline void acpi_iort_init(void) { }
 static inline u32 iort_msi_map_id(struct device *dev, u32 id)
@@ -59,6 +61,11 @@ int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head)
 
 static inline phys_addr_t acpi_iort_dma_get_max_cpu_address(void)
 { return PHYS_ADDR_MAX; }
+
+static inline
+int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
+			struct list_head *list)
+{ return -ENODEV; }
 #endif
 
 #endif /* __ACPI_IORT_H__ */
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [PATCH v5 4/8] iommu/arm-smmu-v3: Introduce strtab init helper
  2021-05-24 11:02 [PATCH v5 0/8] ACPI/IORT: Support for IORT RMR node Shameer Kolothum
                   ` (2 preceding siblings ...)
  2021-05-24 11:02 ` [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory regions Shameer Kolothum
@ 2021-05-24 11:02 ` Shameer Kolothum
  2021-05-24 11:02 ` [PATCH v5 5/8] iommu/arm-smmu-v3: Add bypass flag to arm_smmu_write_strtab_ent() Shameer Kolothum
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 40+ messages in thread
From: Shameer Kolothum @ 2021-05-24 11:02 UTC (permalink / raw)
  To: linux-arm-kernel, linux-acpi, iommu
  Cc: linuxarm, lorenzo.pieralisi, joro, robin.murphy, wanghuiqiang,
	guohanjun, steven.price, Sami.Mujawar, jon, eric.auger,
	yangyicong

Introduce a helper to check the sid range and to init the l2 strtab
entries(bypass). This will be useful when we have to initialize the
l2 strtab with bypass for RMR SIDs.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 28 +++++++++++----------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 54b2f27b81d4..754bad6092c1 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2369,6 +2369,19 @@ static bool arm_smmu_sid_in_range(struct arm_smmu_device *smmu, u32 sid)
 	return sid < limit;
 }
 
+static int arm_smmu_init_sid_strtab(struct arm_smmu_device *smmu, u32 sid)
+{
+	/* Check the SIDs are in range of the SMMU and our stream table */
+	if (!arm_smmu_sid_in_range(smmu, sid))
+		return -ERANGE;
+
+	/* Ensure l2 strtab is initialised */
+	if (smmu->features & ARM_SMMU_FEAT_2_LVL_STRTAB)
+		return arm_smmu_init_l2_strtab(smmu, sid);
+
+	return 0;
+}
+
 static int arm_smmu_insert_master(struct arm_smmu_device *smmu,
 				  struct arm_smmu_master *master)
 {
@@ -2392,20 +2405,9 @@ static int arm_smmu_insert_master(struct arm_smmu_device *smmu,
 		new_stream->id = sid;
 		new_stream->master = master;
 
-		/*
-		 * Check the SIDs are in range of the SMMU and our stream table
-		 */
-		if (!arm_smmu_sid_in_range(smmu, sid)) {
-			ret = -ERANGE;
+		ret = arm_smmu_init_sid_strtab(smmu, sid);
+		if (ret)
 			break;
-		}
-
-		/* Ensure l2 strtab is initialised */
-		if (smmu->features & ARM_SMMU_FEAT_2_LVL_STRTAB) {
-			ret = arm_smmu_init_l2_strtab(smmu, sid);
-			if (ret)
-				break;
-		}
 
 		/* Insert into SID tree */
 		new_node = &(smmu->streams.rb_node);
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [PATCH v5 5/8] iommu/arm-smmu-v3: Add bypass flag to arm_smmu_write_strtab_ent()
  2021-05-24 11:02 [PATCH v5 0/8] ACPI/IORT: Support for IORT RMR node Shameer Kolothum
                   ` (3 preceding siblings ...)
  2021-05-24 11:02 ` [PATCH v5 4/8] iommu/arm-smmu-v3: Introduce strtab init helper Shameer Kolothum
@ 2021-05-24 11:02 ` Shameer Kolothum
  2021-06-14 10:23   ` Robin Murphy
  2021-05-24 11:02 ` [PATCH v5 6/8] iommu/arm-smmu-v3: Get associated RMR info and install Shameer Kolothum
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 40+ messages in thread
From: Shameer Kolothum @ 2021-05-24 11:02 UTC (permalink / raw)
  To: linux-arm-kernel, linux-acpi, iommu
  Cc: linuxarm, lorenzo.pieralisi, joro, robin.murphy, wanghuiqiang,
	guohanjun, steven.price, Sami.Mujawar, jon, eric.auger,
	yangyicong

By default, disable_bypass is set and any dev without an iommu domain
installs STE with CFG_ABORT during arm_smmu_init_bypass_stes(). Introduce
a "bypass" flag to arm_smmu_write_strtab_ent() so that we can force it to
install CFG_BYPASS STE for specific SIDs. This will be useful in follow
up patch to install bypass for IORT RMR SIDs.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 754bad6092c1..f9195b740f48 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1174,7 +1174,7 @@ static void arm_smmu_sync_ste_for_sid(struct arm_smmu_device *smmu, u32 sid)
 }
 
 static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
-				      __le64 *dst)
+				      __le64 *dst, bool bypass)
 {
 	/*
 	 * This is hideously complicated, but we only really care about
@@ -1245,7 +1245,7 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
 
 	/* Bypass/fault */
 	if (!smmu_domain || !(s1_cfg || s2_cfg)) {
-		if (!smmu_domain && disable_bypass)
+		if (!smmu_domain && disable_bypass && !bypass)
 			val |= FIELD_PREP(STRTAB_STE_0_CFG, STRTAB_STE_0_CFG_ABORT);
 		else
 			val |= FIELD_PREP(STRTAB_STE_0_CFG, STRTAB_STE_0_CFG_BYPASS);
@@ -1320,7 +1320,7 @@ static void arm_smmu_init_bypass_stes(__le64 *strtab, unsigned int nent)
 	unsigned int i;
 
 	for (i = 0; i < nent; ++i) {
-		arm_smmu_write_strtab_ent(NULL, -1, strtab);
+		arm_smmu_write_strtab_ent(NULL, -1, strtab, false);
 		strtab += STRTAB_STE_DWORDS;
 	}
 }
@@ -2097,7 +2097,7 @@ static void arm_smmu_install_ste_for_dev(struct arm_smmu_master *master)
 		if (j < i)
 			continue;
 
-		arm_smmu_write_strtab_ent(master, sid, step);
+		arm_smmu_write_strtab_ent(master, sid, step, false);
 	}
 }
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [PATCH v5 6/8] iommu/arm-smmu-v3: Get associated RMR info and install
  2021-05-24 11:02 [PATCH v5 0/8] ACPI/IORT: Support for IORT RMR node Shameer Kolothum
                   ` (4 preceding siblings ...)
  2021-05-24 11:02 ` [PATCH v5 5/8] iommu/arm-smmu-v3: Add bypass flag to arm_smmu_write_strtab_ent() Shameer Kolothum
@ 2021-05-24 11:02 ` Shameer Kolothum
  2021-06-14 10:15   ` Robin Murphy
  2021-05-24 11:02 ` [PATCH v5 7/8] iommu/arm-smmu: Get associated RMR info and install bypass SMR Shameer Kolothum
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 40+ messages in thread
From: Shameer Kolothum @ 2021-05-24 11:02 UTC (permalink / raw)
  To: linux-arm-kernel, linux-acpi, iommu
  Cc: linuxarm, lorenzo.pieralisi, joro, robin.murphy, wanghuiqiang,
	guohanjun, steven.price, Sami.Mujawar, jon, eric.auger,
	yangyicong

Check if there is any RMR info associated with the devices behind
the SMMUv3 and if any, install bypass STEs for them. This is to
keep any ongoing traffic associated with these devices alive
when we enable/reset SMMUv3 during probe().

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 36 +++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index f9195b740f48..be1563e06732 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -3574,6 +3574,39 @@ static void __iomem *arm_smmu_ioremap(struct device *dev, resource_size_t start,
 	return devm_ioremap_resource(dev, &res);
 }
 
+static void arm_smmu_rmr_install_bypass_ste(struct arm_smmu_device *smmu)
+{
+	struct list_head rmr_list;
+	struct iommu_resv_region *e;
+	int ret;
+
+	INIT_LIST_HEAD(&rmr_list);
+	if (iommu_dma_get_rmrs(dev_fwnode(smmu->dev), &rmr_list))
+		return;
+
+	/*
+	 * Since, we don't have a mechanism to differentiate the RMR
+	 * SIDs that has an ongoing live stream, install bypass STEs
+	 * for all the reported ones. 
+	 */
+	list_for_each_entry(e, &rmr_list, list) {
+		__le64 *step;
+		u32 sid = e->fw_data.rmr.sid;
+
+		ret = arm_smmu_init_sid_strtab(smmu, sid);
+		if (ret) {
+			dev_err(smmu->dev, "RMR bypass(0x%x) failed\n",
+				sid);
+			continue;
+		}
+
+		step = arm_smmu_get_step_for_sid(smmu, sid);
+		arm_smmu_write_strtab_ent(NULL, sid, step, true);
+	}
+
+	iommu_dma_put_rmrs(dev_fwnode(smmu->dev), &rmr_list);
+}
+
 static int arm_smmu_device_probe(struct platform_device *pdev)
 {
 	int irq, ret;
@@ -3657,6 +3690,9 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 	/* Record our private device structure */
 	platform_set_drvdata(pdev, smmu);
 
+	/* Check for RMRs and install bypass STEs if any */
+	arm_smmu_rmr_install_bypass_ste(smmu);
+
 	/* Reset the device */
 	ret = arm_smmu_device_reset(smmu, bypass);
 	if (ret)
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [PATCH v5 7/8] iommu/arm-smmu: Get associated RMR info and install bypass SMR
  2021-05-24 11:02 [PATCH v5 0/8] ACPI/IORT: Support for IORT RMR node Shameer Kolothum
                   ` (5 preceding siblings ...)
  2021-05-24 11:02 ` [PATCH v5 6/8] iommu/arm-smmu-v3: Get associated RMR info and install Shameer Kolothum
@ 2021-05-24 11:02 ` Shameer Kolothum
  2021-06-03  8:52   ` Jon Nettleton
  2021-06-14 10:06   ` Robin Murphy
  2021-05-24 11:02 ` [PATCH v5 8/8] iommu/dma: Reserve any RMR regions associated with a dev Shameer Kolothum
  2021-05-24 15:18 ` [PATCH v5 0/8] ACPI/IORT: Support for IORT RMR node Steven Price
  8 siblings, 2 replies; 40+ messages in thread
From: Shameer Kolothum @ 2021-05-24 11:02 UTC (permalink / raw)
  To: linux-arm-kernel, linux-acpi, iommu
  Cc: linuxarm, lorenzo.pieralisi, joro, robin.murphy, wanghuiqiang,
	guohanjun, steven.price, Sami.Mujawar, jon, eric.auger,
	yangyicong

From: Jon Nettleton <jon@solid-run.com>

Check if there is any RMR info associated with the devices behind
the SMMU and if any, install bypass SMRs for them. This is to
keep any ongoing traffic associated with these devices alive
when we enable/reset SMMU during probe().

Signed-off-by: Jon Nettleton <jon@solid-run.com>
Signed-off-by: Steven Price <steven.price@arm.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 drivers/iommu/arm/arm-smmu/arm-smmu.c | 65 +++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index 6f72c4d208ca..56db3d3238fc 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -2042,6 +2042,67 @@ err_reset_platform_ops: __maybe_unused;
 	return err;
 }
 
+static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)
+{
+	struct list_head rmr_list;
+	struct iommu_resv_region *e;
+	int i, cnt = 0;
+	u32 smr;
+	u32 reg;
+
+	INIT_LIST_HEAD(&rmr_list);
+	if (iommu_dma_get_rmrs(dev_fwnode(smmu->dev), &rmr_list))
+		return;
+
+	reg = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_sCR0);
+
+	if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg & ARM_SMMU_sCR0_CLIENTPD)) {
+		/*
+		 * SMMU is already enabled and disallowing bypass, so preserve
+		 * the existing SMRs
+		 */
+		for (i = 0; i < smmu->num_mapping_groups; i++) {
+			smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
+			if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))
+				continue;
+			smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
+			smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK, smr);
+			smmu->smrs[i].valid = true;
+		}
+	}
+
+	list_for_each_entry(e, &rmr_list, list) {
+		u32 sid = e->fw_data.rmr.sid;
+
+		i = arm_smmu_find_sme(smmu, sid, ~0);
+		if (i < 0)
+			continue;
+		if (smmu->s2crs[i].count == 0) {
+			smmu->smrs[i].id = sid;
+			smmu->smrs[i].mask = ~0;
+			smmu->smrs[i].valid = true;
+		}
+		smmu->s2crs[i].count++;
+		smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
+		smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
+		smmu->s2crs[i].cbndx = 0xff;
+
+		cnt++;
+	}
+
+	if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg & ARM_SMMU_sCR0_CLIENTPD)) {
+		/* Remove the valid bit for unused SMRs */
+		for (i = 0; i < smmu->num_mapping_groups; i++) {
+			if (smmu->s2crs[i].count == 0)
+				smmu->smrs[i].valid = false;
+		}
+	}
+
+	dev_notice(smmu->dev, "\tpreserved %d boot mapping%s\n", cnt,
+		   cnt == 1 ? "" : "s");
+	iommu_dma_put_rmrs(dev_fwnode(smmu->dev), &rmr_list);
+}
+
 static int arm_smmu_device_probe(struct platform_device *pdev)
 {
 	struct resource *res;
@@ -2168,6 +2229,10 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 	}
 
 	platform_set_drvdata(pdev, smmu);
+
+	/* Check for RMRs and install bypass SMRs if any */
+	arm_smmu_rmr_install_bypass_smr(smmu);
+
 	arm_smmu_device_reset(smmu);
 	arm_smmu_test_smr_masks(smmu);
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [PATCH v5 8/8] iommu/dma: Reserve any RMR regions associated with a dev
  2021-05-24 11:02 [PATCH v5 0/8] ACPI/IORT: Support for IORT RMR node Shameer Kolothum
                   ` (6 preceding siblings ...)
  2021-05-24 11:02 ` [PATCH v5 7/8] iommu/arm-smmu: Get associated RMR info and install bypass SMR Shameer Kolothum
@ 2021-05-24 11:02 ` Shameer Kolothum
  2021-05-24 15:18 ` [PATCH v5 0/8] ACPI/IORT: Support for IORT RMR node Steven Price
  8 siblings, 0 replies; 40+ messages in thread
From: Shameer Kolothum @ 2021-05-24 11:02 UTC (permalink / raw)
  To: linux-arm-kernel, linux-acpi, iommu
  Cc: linuxarm, lorenzo.pieralisi, joro, robin.murphy, wanghuiqiang,
	guohanjun, steven.price, Sami.Mujawar, jon, eric.auger,
	yangyicong

Get ACPI IORT RMR regions associated with a dev reserved
so that there is a unity mapping for them in SMMU.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 drivers/iommu/dma-iommu.c | 56 +++++++++++++++++++++++++++++++++++----
 1 file changed, 51 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index f893d460cfa4..c68093f48816 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -207,22 +207,68 @@ void iommu_dma_put_rmrs(struct fwnode_handle *iommu_fwnode,
 }
 EXPORT_SYMBOL(iommu_dma_put_rmrs);
 
+static bool iommu_dma_dev_has_rmr(struct iommu_fwspec *fwspec,
+				  struct iommu_resv_region *e)
+{
+	int i;
+
+	for (i = 0; i < fwspec->num_ids; i++) {
+		if (e->fw_data.rmr.sid == fwspec->ids[i])
+			return true;
+	}
+
+	return false;
+}
+
+static void iommu_dma_get_rmr_resv_regions(struct device *dev,
+					   struct list_head *list)
+{
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+	struct list_head rmr_list;
+	struct iommu_resv_region *rmr, *tmp;
+
+	INIT_LIST_HEAD(&rmr_list);
+	if (iommu_dma_get_rmrs(fwspec->iommu_fwnode, &rmr_list))
+		return;
+
+	if (dev_is_pci(dev)) {
+		struct pci_dev *pdev = to_pci_dev(dev);
+		struct pci_host_bridge *host = pci_find_host_bridge(pdev->bus);
+
+		if (!host->preserve_config)
+			return;
+	}
+
+	list_for_each_entry_safe(rmr, tmp, &rmr_list, list) {
+		if (!iommu_dma_dev_has_rmr(fwspec, rmr))
+			continue;
+
+		/* Remove from iommu RMR list and add to dev resv_regions */
+		list_del_init(&rmr->list);
+		list_add_tail(&rmr->list, list);
+	}
+
+	iommu_dma_put_rmrs(fwspec->iommu_fwnode, &rmr_list);
+}
+
 /**
  * iommu_dma_get_resv_regions - Reserved region driver helper
  * @dev: Device from iommu_get_resv_regions()
  * @list: Reserved region list from iommu_get_resv_regions()
  *
  * IOMMU drivers can use this to implement their .get_resv_regions callback
- * for general non-IOMMU-specific reservations. Currently, this covers GICv3
- * ITS region reservation on ACPI based ARM platforms that may require HW MSI
- * reservation.
+ * for general non-IOMMU-specific reservations. Currently this covers,
+ *  -GICv3 ITS region reservation on ACPI based ARM platforms that may
+ *   require HW MSI reservation.
+ *  -Any ACPI IORT RMR memory range reservations (IORT spec rev E.b)
  */
 void iommu_dma_get_resv_regions(struct device *dev, struct list_head *list)
 {
 
-	if (!is_of_node(dev_iommu_fwspec_get(dev)->iommu_fwnode))
+	if (!is_of_node(dev_iommu_fwspec_get(dev)->iommu_fwnode)) {
 		iort_iommu_msi_get_resv_regions(dev, list);
-
+		iommu_dma_get_rmr_resv_regions(dev, list);
+	}
 }
 EXPORT_SYMBOL(iommu_dma_get_resv_regions);
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 0/8] ACPI/IORT: Support for IORT RMR node
  2021-05-24 11:02 [PATCH v5 0/8] ACPI/IORT: Support for IORT RMR node Shameer Kolothum
                   ` (7 preceding siblings ...)
  2021-05-24 11:02 ` [PATCH v5 8/8] iommu/dma: Reserve any RMR regions associated with a dev Shameer Kolothum
@ 2021-05-24 15:18 ` Steven Price
  8 siblings, 0 replies; 40+ messages in thread
From: Steven Price @ 2021-05-24 15:18 UTC (permalink / raw)
  To: Shameer Kolothum, linux-arm-kernel, linux-acpi, iommu
  Cc: linuxarm, lorenzo.pieralisi, joro, robin.murphy, wanghuiqiang,
	guohanjun, Sami.Mujawar, jon, eric.auger, yangyicong

On 24/05/2021 12:02, Shameer Kolothum wrote:
> Hi,
> 
> v4 --> v5
>  -Added a fw_data union to struct iommu_resv_region and removed
>   struct iommu_rmr (Based on comments from Joerg/Robin).
>  -Added iommu_put_rmrs() to release mem.
>  -Thanks to Steve for verifying v4 on SMMUv2, but not added the
>   Tested-by yet because of the above changes.

I've retested with this series (Juno with SMMU in front of display
controller and EFI framebuffer), and it still works, so:

Tested-by: Steven Price <steven.price@arm.com>

Thanks,

Steve

> 
> v3 -->v4
> -Included the SMMUv2 SMR bypass install changes suggested by
>  Steve(patch #7)
> -As per Robin's comments, RMR reserve implementation is now
>  more generic  (patch #8) and dropped v3 patches 8 and 10.
> -Rebase to 5.13-rc1 
> 
> RFC v2 --> v3
>  -Dropped RFC tag as the ACPICA header changes are now ready to be
>   part of 5.13[0]. But this series still has a dependency on that patch.
>  -Added IORT E.b related changes(node flags, _DSM function 5 checks for
>   PCIe).
>  -Changed RMR to stream id mapping from M:N to M:1 as per the spec and
>   discussion here[1].
>  -Last two patches add support for SMMUv2(Thanks to Jon Nettleton!) 
> 
> Sanity tested on a HiSilicon D06. Further testing and feedback is greatly
> appreciated.
> 
> The whole series can be found here,
> https://github.com/hisilicon/kernel-dev/tree/private-v5.12-rc8-rmr-v3
> 
> Thanks,
> Shameer
> 
> [0] https://lore.kernel.org/linux-acpi/20210406213028.718796-22-erik.kaneda@intel.com/
> [1] https://op-lists.linaro.org/pipermail/linaro-open-discussions/2021-April/000150.html
> 
> RFC v1 --> v2:
>  - Added a generic interface for IOMMU drivers to retrieve all the 
>    RMR info associated with a given IOMMU.
>  - SMMUv3 driver gets the RMR list during probe() and installs
>    bypass STEs for all the SIDs in the RMR list. This is to keep
>    the ongoing traffic alive(if any) during SMMUv3 reset. This is
>    based on the suggestions received for v1 to take care of the
>    EFI framebuffer use case. Only sanity tested for now.
>  - During the probe/attach device, SMMUv3 driver reserves any
>    RMR region associated with the device such that there is a unity
>    mapping for them in SMMU.
> ---    
> 
> From RFC v1:
> -------------
> The series adds support to IORT RMR nodes specified in IORT
> Revision E -ARM DEN 0049E[0]. RMR nodes are used to describe memory
> ranges that are used by endpoints and require a unity mapping
> in SMMU.
> 
> We have faced issues with 3408iMR RAID controller cards which
> fail to boot when SMMU is enabled. This is because these controllers
> make use of host memory for various caching related purposes and when
> SMMU is enabled the iMR firmware fails to access these memory regions
> as there is no mapping for them. IORT RMR provides a way for UEFI to
> describe and report these memory regions so that the kernel can make
> a unity mapping for these in SMMU.
> 
> Tests:
> 
> With a UEFI, that reports the RMR for the dev,
> ....
> [16F0h 5872   1]                         Type : 06
> [16F1h 5873   2]                       Length : 007C
> [16F3h 5875   1]                     Revision : 00
> [1038h 0056   2]                     Reserved : 00000000
> [1038h 0056   2]                   Identifier : 00000000
> [16F8h 5880   4]                Mapping Count : 00000001
> [16FCh 5884   4]               Mapping Offset : 00000040
> 
> [1700h 5888   4]    Number of RMR Descriptors : 00000002
> [1704h 5892   4]        RMR Descriptor Offset : 00000018
> 
> [1708h 5896   8]          Base Address of RMR : 0000E6400000
> [1710h 5904   8]                Length of RMR : 000000100000
> [1718h 5912   4]                     Reserved : 00000000
> 
> [171Ch 5916   8]          Base Address of RMR : 0000000027B00000
> [1724h 5924   8]                Length of RMR : 0000000000C00000
> [172Ch 5932   4]                     Reserved : 00000000
> 
> [1730h 5936   4]                   Input base : 00000000
> [1734h 5940   4]                     ID Count : 00000001
> [1738h 5944   4]                  Output Base : 00000003
> [173Ch 5948   4]             Output Reference : 00000064
> [1740h 5952   4]        Flags (decoded below) : 00000001
>                                Single Mapping : 1
> ...
> 
> Without the series the RAID controller initialization fails as
> below,
> 
> ...
> [   12.631117] megaraid_sas 0000:03:00.0: FW supports sync cache        : Yes   
> [   12.637360] megaraid_sas 0000:03:00.0: megasas_disable_intr_fusion is called outbound_intr_mask:0x40000009                                                   
> [   18.776377] megaraid_sas 0000:03:00.0: Init cmd return status FAILED for SCSI host 0                                                                         
> [   23.019383] megaraid_sas 0000:03:00.0: Waiting for FW to come to ready state 
> [  106.684281] megaraid_sas 0000:03:00.0: FW in FAULT state, Fault code:0x10000 subcode:0x0 func:megasas_transition_to_ready                                    
> [  106.695186] megaraid_sas 0000:03:00.0: System Register set:                  
> [  106.889787] megaraid_sas 0000:03:00.0: Failed to transition controller to ready for scsi0.                                                                   
> [  106.910475] megaraid_sas 0000:03:00.0: Failed from megasas_init_fw 6407      
> estuary:/$
> 
> With the series, now the kernel has direct mapping for the dev as
> below,
> 
> estuary:/$ cat /sys/kernel/iommu_groups/0/reserved_regions                      
> 0x0000000008000000 0x00000000080fffff msi                                       
> 0x0000000027b00000 0x00000000286fffff direct                                    
> 0x00000000e6400000 0x00000000e64fffff direct                                    
> estuary:/$
> 
> ....
> [   12.254318] megaraid_sas 0000:03:00.0: megasas_disable_intr_fusion is called outbound_intr_mask:0x40000009                                                   
> [   12.739089] megaraid_sas 0000:03:00.0: FW provided supportMaxExtLDs: 0      max_lds: 32                                                                      
> [   12.746628] megaraid_sas 0000:03:00.0: controller type       : iMR(0MB)      
> [   12.752694] megaraid_sas 0000:03:00.0: Online Controller Reset(OCR)  : Enabled                                                                               
> [   12.759798] megaraid_sas 0000:03:00.0: Secure JBOD support   : Yes           
> [   12.765778] megaraid_sas 0000:03:00.0: NVMe passthru support : Yes           
> [   12.771931] megaraid_sas 0000:03:00.0: FW provided TM TaskAbort/Reset timeou: 6 secs/60 secs                                                                 
> [   12.780503] megaraid_sas 0000:03:00.0: JBOD sequence map support     : Yes   
> [   12.787000] megaraid_sas 0000:03:00.0: PCI Lane Margining support    : No    
> [   12.819179] megaraid_sas 0000:03:00.0: NVME page size        : (4096)        
> [   12.825672] megaraid_sas 0000:03:00.0: megasas_enable_intr_fusion is called outbound_intr_mask:0x40000000                                                    
> [   12.835199] megaraid_sas 0000:03:00.0: INIT adapter done                     
> [   12.873932] megaraid_sas 0000:03:00.0: pci id                : (0x1000)/(0x0017)/(0x19e5)/(0xd213)                                                           
> [   12.881644] megaraid_sas 0000:03:00.0: unevenspan support    : no            
> [   12.887451] megaraid_sas 0000:03:00.0: firmware crash dump   : no            
> [   12.893344] megaraid_sas 0000:03:00.0: JBOD sequence map     : enabled       
> 
> RAID controller init is now success and can detect the drives
> attached as well.
> 
> Jon Nettleton (1):
>   iommu/arm-smmu: Get associated RMR info and install bypass SMR
> 
> Shameer Kolothum (7):
>   ACPI/IORT: Add support for RMR node parsing
>   iommu/dma: Introduce generic helper to retrieve RMR info
>   ACPI/IORT: Add a helper to retrieve RMR memory regions
>   iommu/arm-smmu-v3: Introduce strtab init helper
>   iommu/arm-smmu-v3: Add bypass flag to arm_smmu_write_strtab_ent()
>   iommu/arm-smmu-v3: Get associated RMR info and install
>   iommu/dma: Reserve any RMR regions associated with a dev
> 
>  drivers/acpi/arm64/iort.c                   | 154 +++++++++++++++++++-
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |  72 ++++++---
>  drivers/iommu/arm/arm-smmu/arm-smmu.c       |  65 +++++++++
>  drivers/iommu/dma-iommu.c                   |  89 ++++++++++-
>  include/linux/acpi_iort.h                   |   7 +
>  include/linux/dma-iommu.h                   |  13 ++
>  include/linux/iommu.h                       |  10 ++
>  7 files changed, 387 insertions(+), 23 deletions(-)
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 2/8] iommu/dma: Introduce generic helper to retrieve RMR info
  2021-05-24 11:02 ` [PATCH v5 2/8] iommu/dma: Introduce generic helper to retrieve RMR info Shameer Kolothum
@ 2021-05-24 15:35   ` kernel test robot
  0 siblings, 0 replies; 40+ messages in thread
From: kernel test robot @ 2021-05-24 15:35 UTC (permalink / raw)
  To: Shameer Kolothum, linux-arm-kernel, linux-acpi, iommu
  Cc: kbuild-all, clang-built-linux, linuxarm, lorenzo.pieralisi, joro,
	robin.murphy, wanghuiqiang, guohanjun, steven.price

[-- Attachment #1: Type: text/plain, Size: 3118 bytes --]

Hi Shameer,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on iommu/next]
[also build test WARNING on pm/linux-next arm64/for-next/core linus/master v5.13-rc3 next-20210524]
[cannot apply to xlnx/master arm/for-next soc/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Shameer-Kolothum/ACPI-IORT-Support-for-IORT-RMR-node/20210524-190633
base:   https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git next
config: x86_64-randconfig-a013-20210524 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 93d1e5822ed64abd777eb94ea9899e96c4c39fbe)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/2c046e03beb7d64e0f7182d01a7dc63dce79c882
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Shameer-Kolothum/ACPI-IORT-Support-for-IORT-RMR-node/20210524-190633
        git checkout 2c046e03beb7d64e0f7182d01a7dc63dce79c882
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from drivers/vfio/vfio_iommu_type1.c:41:
>> include/linux/dma-iommu.h:90:5: warning: no previous prototype for function 'iommu_dma_get_rmrs' [-Wmissing-prototypes]
   int iommu_dma_get_rmrs(struct fwnode_handle *iommu, struct list_head *list)
       ^
   include/linux/dma-iommu.h:90:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int iommu_dma_get_rmrs(struct fwnode_handle *iommu, struct list_head *list)
   ^
   static 
>> include/linux/dma-iommu.h:95:6: warning: no previous prototype for function 'iommu_dma_put_rmrs' [-Wmissing-prototypes]
   void iommu_dma_put_rmrs(struct fwnode_handle *iommu, struct list_head *list)
        ^
   include/linux/dma-iommu.h:95:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void iommu_dma_put_rmrs(struct fwnode_handle *iommu, struct list_head *list)
   ^
   static 
   2 warnings generated.


vim +/iommu_dma_get_rmrs +90 include/linux/dma-iommu.h

    89	
  > 90	int iommu_dma_get_rmrs(struct fwnode_handle *iommu, struct list_head *list)
    91	{
    92		return -ENODEV;
    93	}
    94	
  > 95	void iommu_dma_put_rmrs(struct fwnode_handle *iommu, struct list_head *list)
    96	{
    97	}
    98	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 38444 bytes --]

[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory regions
  2021-05-24 11:02 ` [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory regions Shameer Kolothum
@ 2021-05-26  7:53   ` Laurentiu Tudor
  2021-05-26 16:36     ` Shameerali Kolothum Thodi
  2021-06-14 11:23   ` Robin Murphy
  1 sibling, 1 reply; 40+ messages in thread
From: Laurentiu Tudor @ 2021-05-26  7:53 UTC (permalink / raw)
  To: Shameer Kolothum, linux-arm-kernel, linux-acpi, iommu
  Cc: jon, linuxarm, steven.price, guohanjun, yangyicong, Sami.Mujawar,
	robin.murphy, wanghuiqiang

Hi Shameer,

On 5/24/2021 2:02 PM, Shameer Kolothum wrote:
> Add a helper function that retrieves RMR memory descriptors
> associated with a given IOMMU. This will be used by IOMMU
> drivers to setup necessary mappings.
> 
> Now that we have this, invoke it from the generic helper
> interface.
> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  drivers/acpi/arm64/iort.c | 50 +++++++++++++++++++++++++++++++++++++++
>  drivers/iommu/dma-iommu.c |  4 ++++
>  include/linux/acpi_iort.h |  7 ++++++
>  3 files changed, 61 insertions(+)
> 
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index fea1ffaedf3b..01917caf58de 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -12,6 +12,7 @@
>  
>  #include <linux/acpi_iort.h>
>  #include <linux/bitfield.h>
> +#include <linux/dma-iommu.h>
>  #include <linux/iommu.h>
>  #include <linux/kernel.h>
>  #include <linux/list.h>
> @@ -837,6 +838,53 @@ static inline int iort_add_device_replay(struct device *dev)
>  	return err;
>  }
>  
> +/**
> + * iort_iommu_get_rmrs - Helper to retrieve RMR info associated with IOMMU
> + * @iommu: fwnode for the IOMMU
> + * @head: RMR list head to be populated
> + *
> + * Returns: 0 on success, <0 failure
> + */
> +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> +			struct list_head *head)
> +{
> +	struct iort_rmr_entry *e;
> +	struct acpi_iort_node *iommu;
> +	int rmrs = 0;
> +
> +	iommu = iort_get_iort_node(iommu_fwnode);
> +	if (!iommu || list_empty(&iort_rmr_list))
> +		return -ENODEV;
> +
> +	list_for_each_entry(e, &iort_rmr_list, list) {
> +		int prot = IOMMU_READ | IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;

We have a case with an IP block that needs EXEC rights on its reserved
memory, so could you please drop the IOMMU_NOEXEC flag?

---
Thanks & Best Regards, Laurentiu

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 40+ messages in thread

* RE: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory regions
  2021-05-26  7:53   ` Laurentiu Tudor
@ 2021-05-26 16:36     ` Shameerali Kolothum Thodi
  2021-05-26 17:11       ` Laurentiu Tudor
                         ` (2 more replies)
  0 siblings, 3 replies; 40+ messages in thread
From: Shameerali Kolothum Thodi @ 2021-05-26 16:36 UTC (permalink / raw)
  To: Laurentiu Tudor, linux-arm-kernel, linux-acpi, iommu
  Cc: jon, Linuxarm, steven.price, Guohanjun (Hanjun Guo),
	yangyicong, Sami.Mujawar, robin.murphy, wanghuiqiang



> -----Original Message-----
> From: Laurentiu Tudor [mailto:laurentiu.tudor@nxp.com]
> Sent: 26 May 2021 08:53
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> linux-arm-kernel@lists.infradead.org; linux-acpi@vger.kernel.org;
> iommu@lists.linux-foundation.org
> Cc: jon@solid-run.com; Linuxarm <linuxarm@huawei.com>;
> steven.price@arm.com; Guohanjun (Hanjun Guo) <guohanjun@huawei.com>;
> yangyicong <yangyicong@huawei.com>; Sami.Mujawar@arm.com;
> robin.murphy@arm.com; wanghuiqiang <wanghuiqiang@huawei.com>
> Subject: Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory
> regions
> 
> Hi Shameer,
> 
> On 5/24/2021 2:02 PM, Shameer Kolothum wrote:
> > Add a helper function that retrieves RMR memory descriptors
> > associated with a given IOMMU. This will be used by IOMMU
> > drivers to setup necessary mappings.
> >
> > Now that we have this, invoke it from the generic helper
> > interface.
> >
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> > ---
> >  drivers/acpi/arm64/iort.c | 50
> +++++++++++++++++++++++++++++++++++++++
> >  drivers/iommu/dma-iommu.c |  4 ++++
> >  include/linux/acpi_iort.h |  7 ++++++
> >  3 files changed, 61 insertions(+)
> >
> > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > index fea1ffaedf3b..01917caf58de 100644
> > --- a/drivers/acpi/arm64/iort.c
> > +++ b/drivers/acpi/arm64/iort.c
> > @@ -12,6 +12,7 @@
> >
> >  #include <linux/acpi_iort.h>
> >  #include <linux/bitfield.h>
> > +#include <linux/dma-iommu.h>
> >  #include <linux/iommu.h>
> >  #include <linux/kernel.h>
> >  #include <linux/list.h>
> > @@ -837,6 +838,53 @@ static inline int iort_add_device_replay(struct
> device *dev)
> >  	return err;
> >  }
> >
> > +/**
> > + * iort_iommu_get_rmrs - Helper to retrieve RMR info associated with
> IOMMU
> > + * @iommu: fwnode for the IOMMU
> > + * @head: RMR list head to be populated
> > + *
> > + * Returns: 0 on success, <0 failure
> > + */
> > +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> > +			struct list_head *head)
> > +{
> > +	struct iort_rmr_entry *e;
> > +	struct acpi_iort_node *iommu;
> > +	int rmrs = 0;
> > +
> > +	iommu = iort_get_iort_node(iommu_fwnode);
> > +	if (!iommu || list_empty(&iort_rmr_list))
> > +		return -ENODEV;
> > +
> > +	list_for_each_entry(e, &iort_rmr_list, list) {
> > +		int prot = IOMMU_READ | IOMMU_WRITE | IOMMU_NOEXEC |
> IOMMU_MMIO;
> 
> We have a case with an IP block that needs EXEC rights on its reserved
> memory, so could you please drop the IOMMU_NOEXEC flag?

Ok, I think I can drop that one if there are no other concerns. I was not quite
sure what to include here in the first place as the IORT spec is not giving any
further details about the RMR regions(May be the flags field can be extended to
describe these details).

Thanks,
Shameer

   
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory regions
  2021-05-26 16:36     ` Shameerali Kolothum Thodi
@ 2021-05-26 17:11       ` Laurentiu Tudor
  2021-06-03 12:27         ` Jon Nettleton
  2021-05-27  4:25       ` Jon Nettleton
  2021-06-14 10:35       ` Robin Murphy
  2 siblings, 1 reply; 40+ messages in thread
From: Laurentiu Tudor @ 2021-05-26 17:11 UTC (permalink / raw)
  To: Shameerali Kolothum Thodi, linux-arm-kernel, linux-acpi, iommu
  Cc: jon, Linuxarm, steven.price, Guohanjun (Hanjun Guo),
	yangyicong, Sami.Mujawar, robin.murphy, wanghuiqiang



On 5/26/2021 7:36 PM, Shameerali Kolothum Thodi wrote:
> 
> 
>> -----Original Message-----
>> From: Laurentiu Tudor [mailto:laurentiu.tudor@nxp.com]
>> Sent: 26 May 2021 08:53
>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
>> linux-arm-kernel@lists.infradead.org; linux-acpi@vger.kernel.org;
>> iommu@lists.linux-foundation.org
>> Cc: jon@solid-run.com; Linuxarm <linuxarm@huawei.com>;
>> steven.price@arm.com; Guohanjun (Hanjun Guo) <guohanjun@huawei.com>;
>> yangyicong <yangyicong@huawei.com>; Sami.Mujawar@arm.com;
>> robin.murphy@arm.com; wanghuiqiang <wanghuiqiang@huawei.com>
>> Subject: Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory
>> regions
>>
>> Hi Shameer,
>>
>> On 5/24/2021 2:02 PM, Shameer Kolothum wrote:
>>> Add a helper function that retrieves RMR memory descriptors
>>> associated with a given IOMMU. This will be used by IOMMU
>>> drivers to setup necessary mappings.
>>>
>>> Now that we have this, invoke it from the generic helper
>>> interface.
>>>
>>> Signed-off-by: Shameer Kolothum
>> <shameerali.kolothum.thodi@huawei.com>
>>> ---
>>>  drivers/acpi/arm64/iort.c | 50
>> +++++++++++++++++++++++++++++++++++++++
>>>  drivers/iommu/dma-iommu.c |  4 ++++
>>>  include/linux/acpi_iort.h |  7 ++++++
>>>  3 files changed, 61 insertions(+)
>>>
>>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
>>> index fea1ffaedf3b..01917caf58de 100644
>>> --- a/drivers/acpi/arm64/iort.c
>>> +++ b/drivers/acpi/arm64/iort.c
>>> @@ -12,6 +12,7 @@
>>>
>>>  #include <linux/acpi_iort.h>
>>>  #include <linux/bitfield.h>
>>> +#include <linux/dma-iommu.h>
>>>  #include <linux/iommu.h>
>>>  #include <linux/kernel.h>
>>>  #include <linux/list.h>
>>> @@ -837,6 +838,53 @@ static inline int iort_add_device_replay(struct
>> device *dev)
>>>  	return err;
>>>  }
>>>
>>> +/**
>>> + * iort_iommu_get_rmrs - Helper to retrieve RMR info associated with
>> IOMMU
>>> + * @iommu: fwnode for the IOMMU
>>> + * @head: RMR list head to be populated
>>> + *
>>> + * Returns: 0 on success, <0 failure
>>> + */
>>> +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
>>> +			struct list_head *head)
>>> +{
>>> +	struct iort_rmr_entry *e;
>>> +	struct acpi_iort_node *iommu;
>>> +	int rmrs = 0;
>>> +
>>> +	iommu = iort_get_iort_node(iommu_fwnode);
>>> +	if (!iommu || list_empty(&iort_rmr_list))
>>> +		return -ENODEV;
>>> +
>>> +	list_for_each_entry(e, &iort_rmr_list, list) {
>>> +		int prot = IOMMU_READ | IOMMU_WRITE | IOMMU_NOEXEC |
>> IOMMU_MMIO;
>>
>> We have a case with an IP block that needs EXEC rights on its reserved
>> memory, so could you please drop the IOMMU_NOEXEC flag?
> 
> Ok, I think I can drop that one if there are no other concerns. I was not quite
> sure what to include here in the first place as the IORT spec is not giving any
> further details about the RMR regions(May be the flags field can be extended to
> describe these details).
> 

That would be great, given that some preliminary investigations on my
side revealed that our IP block seems to be quite sensitive to memory
attributes. I need to spend some more time on this but at first sight
looks like it needs cacheable, normal memory (not mmio mapping).

---
Thanks & Best Regards, Laurentiu

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory regions
  2021-05-26 16:36     ` Shameerali Kolothum Thodi
  2021-05-26 17:11       ` Laurentiu Tudor
@ 2021-05-27  4:25       ` Jon Nettleton
  2021-06-14 10:35       ` Robin Murphy
  2 siblings, 0 replies; 40+ messages in thread
From: Jon Nettleton @ 2021-05-27  4:25 UTC (permalink / raw)
  To: Shameerali Kolothum Thodi
  Cc: Laurentiu Tudor, linux-arm-kernel, linux-acpi, iommu, Linuxarm,
	steven.price, Guohanjun (Hanjun Guo),
	yangyicong, Sami.Mujawar, robin.murphy, wanghuiqiang

On Wed, May 26, 2021 at 6:36 PM Shameerali Kolothum Thodi
<shameerali.kolothum.thodi@huawei.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Laurentiu Tudor [mailto:laurentiu.tudor@nxp.com]
> > Sent: 26 May 2021 08:53
> > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> > linux-arm-kernel@lists.infradead.org; linux-acpi@vger.kernel.org;
> > iommu@lists.linux-foundation.org
> > Cc: jon@solid-run.com; Linuxarm <linuxarm@huawei.com>;
> > steven.price@arm.com; Guohanjun (Hanjun Guo) <guohanjun@huawei.com>;
> > yangyicong <yangyicong@huawei.com>; Sami.Mujawar@arm.com;
> > robin.murphy@arm.com; wanghuiqiang <wanghuiqiang@huawei.com>
> > Subject: Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory
> > regions
> >
> > Hi Shameer,
> >
> > On 5/24/2021 2:02 PM, Shameer Kolothum wrote:
> > > Add a helper function that retrieves RMR memory descriptors
> > > associated with a given IOMMU. This will be used by IOMMU
> > > drivers to setup necessary mappings.
> > >
> > > Now that we have this, invoke it from the generic helper
> > > interface.
> > >
> > > Signed-off-by: Shameer Kolothum
> > <shameerali.kolothum.thodi@huawei.com>
> > > ---
> > >  drivers/acpi/arm64/iort.c | 50
> > +++++++++++++++++++++++++++++++++++++++
> > >  drivers/iommu/dma-iommu.c |  4 ++++
> > >  include/linux/acpi_iort.h |  7 ++++++
> > >  3 files changed, 61 insertions(+)
> > >
> > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > > index fea1ffaedf3b..01917caf58de 100644
> > > --- a/drivers/acpi/arm64/iort.c
> > > +++ b/drivers/acpi/arm64/iort.c
> > > @@ -12,6 +12,7 @@
> > >
> > >  #include <linux/acpi_iort.h>
> > >  #include <linux/bitfield.h>
> > > +#include <linux/dma-iommu.h>
> > >  #include <linux/iommu.h>
> > >  #include <linux/kernel.h>
> > >  #include <linux/list.h>
> > > @@ -837,6 +838,53 @@ static inline int iort_add_device_replay(struct
> > device *dev)
> > >     return err;
> > >  }
> > >
> > > +/**
> > > + * iort_iommu_get_rmrs - Helper to retrieve RMR info associated with
> > IOMMU
> > > + * @iommu: fwnode for the IOMMU
> > > + * @head: RMR list head to be populated
> > > + *
> > > + * Returns: 0 on success, <0 failure
> > > + */
> > > +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> > > +                   struct list_head *head)
> > > +{
> > > +   struct iort_rmr_entry *e;
> > > +   struct acpi_iort_node *iommu;
> > > +   int rmrs = 0;
> > > +
> > > +   iommu = iort_get_iort_node(iommu_fwnode);
> > > +   if (!iommu || list_empty(&iort_rmr_list))
> > > +           return -ENODEV;
> > > +
> > > +   list_for_each_entry(e, &iort_rmr_list, list) {
> > > +           int prot = IOMMU_READ | IOMMU_WRITE | IOMMU_NOEXEC |
> > IOMMU_MMIO;
> >
> > We have a case with an IP block that needs EXEC rights on its reserved
> > memory, so could you please drop the IOMMU_NOEXEC flag?
>
> Ok, I think I can drop that one if there are no other concerns. I was not quite
> sure what to include here in the first place as the IORT spec is not giving any
> further details about the RMR regions(May be the flags field can be extended to
> describe these details).

We probably need to bring this back up to the ACPI forums. This is probably
something that should be attached to the memory range node which does have
4 extra reserved bytes.

-Jon

>
> Thanks,
> Shameer
>
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 7/8] iommu/arm-smmu: Get associated RMR info and install bypass SMR
  2021-05-24 11:02 ` [PATCH v5 7/8] iommu/arm-smmu: Get associated RMR info and install bypass SMR Shameer Kolothum
@ 2021-06-03  8:52   ` Jon Nettleton
  2021-06-03 11:27     ` Steven Price
  2021-06-14 10:06   ` Robin Murphy
  1 sibling, 1 reply; 40+ messages in thread
From: Jon Nettleton @ 2021-06-03  8:52 UTC (permalink / raw)
  To: Shameer Kolothum
  Cc: linux-arm-kernel, ACPI Devel Maling List, iommu, Linuxarm,
	Lorenzo Pieralisi, joro, Robin Murphy, wanghuiqiang, Hanjun Guo,
	Steven Price, Sami.Mujawar, eric.auger, yangyicong

On Mon, May 24, 2021 at 1:04 PM Shameer Kolothum
<shameerali.kolothum.thodi@huawei.com> wrote:
>
> From: Jon Nettleton <jon@solid-run.com>
>
> Check if there is any RMR info associated with the devices behind
> the SMMU and if any, install bypass SMRs for them. This is to
> keep any ongoing traffic associated with these devices alive
> when we enable/reset SMMU during probe().
>
> Signed-off-by: Jon Nettleton <jon@solid-run.com>
> Signed-off-by: Steven Price <steven.price@arm.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  drivers/iommu/arm/arm-smmu/arm-smmu.c | 65 +++++++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
>
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index 6f72c4d208ca..56db3d3238fc 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -2042,6 +2042,67 @@ err_reset_platform_ops: __maybe_unused;
>         return err;
>  }
>
> +static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)
> +{
> +       struct list_head rmr_list;
> +       struct iommu_resv_region *e;
> +       int i, cnt = 0;
> +       u32 smr;
> +       u32 reg;
> +
> +       INIT_LIST_HEAD(&rmr_list);
> +       if (iommu_dma_get_rmrs(dev_fwnode(smmu->dev), &rmr_list))
> +               return;
> +
> +       reg = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_sCR0);
> +
> +       if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg & ARM_SMMU_sCR0_CLIENTPD)) {
> +               /*
> +                * SMMU is already enabled and disallowing bypass, so preserve
> +                * the existing SMRs
> +                */
> +               for (i = 0; i < smmu->num_mapping_groups; i++) {
> +                       smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
> +                       if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))
> +                               continue;
> +                       smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
> +                       smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK, smr);
> +                       smmu->smrs[i].valid = true;
> +               }
> +       }
> +
> +       list_for_each_entry(e, &rmr_list, list) {
> +               u32 sid = e->fw_data.rmr.sid;
> +
> +               i = arm_smmu_find_sme(smmu, sid, ~0);
> +               if (i < 0)
> +                       continue;
> +               if (smmu->s2crs[i].count == 0) {
> +                       smmu->smrs[i].id = sid;
> +                       smmu->smrs[i].mask = ~0;
> +                       smmu->smrs[i].valid = true;
> +               }
> +               smmu->s2crs[i].count++;
> +               smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
> +               smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
> +               smmu->s2crs[i].cbndx = 0xff;
> +
> +               cnt++;
> +       }
> +
> +       if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg & ARM_SMMU_sCR0_CLIENTPD)) {
> +               /* Remove the valid bit for unused SMRs */
> +               for (i = 0; i < smmu->num_mapping_groups; i++) {
> +                       if (smmu->s2crs[i].count == 0)
> +                               smmu->smrs[i].valid = false;
> +               }
> +       }
> +
> +       dev_notice(smmu->dev, "\tpreserved %d boot mapping%s\n", cnt,
> +                  cnt == 1 ? "" : "s");
> +       iommu_dma_put_rmrs(dev_fwnode(smmu->dev), &rmr_list);
> +}
> +
>  static int arm_smmu_device_probe(struct platform_device *pdev)
>  {
>         struct resource *res;
> @@ -2168,6 +2229,10 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>         }
>
>         platform_set_drvdata(pdev, smmu);
> +
> +       /* Check for RMRs and install bypass SMRs if any */
> +       arm_smmu_rmr_install_bypass_smr(smmu);
> +
>         arm_smmu_device_reset(smmu);
>         arm_smmu_test_smr_masks(smmu);
>
> --
> 2.17.1
>

Shameer and Robin

I finally got around to updating edk2 and the HoneyComb IORT tables to
reflect the new standards.
Out of the box the new patchset was generating errors immediatly after
the smmu bringup.

arm-smmu arm-smmu.0.auto: Unhandled context fault: fsr=0x402, iova=0x2080000140,
fsynr=0x1d0040, cbfrsynra=0x4000, cb=0

These errors were generated even with disable_bypass=0

I tracked down the issue to

This code is skipped as Robin said would be correct

> +       if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg & ARM_SMMU_sCR0_CLIENTPD)) {
> +               /*
> +                * SMMU is already enabled and disallowing bypass, so preserve
> +                * the existing SMRs
> +                */
> +               for (i = 0; i < smmu->num_mapping_groups; i++) {
> +                       smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
> +                       if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))
> +                               continue;
> +                       smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
> +                       smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK, smr);
> +                       smmu->smrs[i].valid = true;
> +               }[    2.707729] arm-smmu: setting up rmr list on 0x4000
[    2.712598] arm-smmu: s2crs count is 0 smrs index 0x0
[    2.717638] arm-smmu: s2crs count is 0 smrs id is 0x4000
[    2.722939] arm-smmu: s2crs count is 0 smrs mask is 0x8000
[    2.728417] arm-smmu arm-smmu.0.auto:        preserved 1 boot mapping

> +       }

Then this code block was hit which is correct

> +               if (smmu->s2crs[i].count == 0) {
> +                       smmu->smrs[i].id = sid;
> +                       smmu->smrs[i].mask = ~0;
> +                       smmu->smrs[i].valid = true;
> +               }

The mask was causing the issue.  If I think ammended that segment to read
the mask as setup by the hardware everything was back to functioning both
with and without disable_bypass set.

Some debug from that section when it is working

[    2.712598] arm-smmu: s2crs count is 0 smrs index 0x0
[    2.717638] arm-smmu: s2crs count is 0 smrs id is 0x4000
[    2.722939] arm-smmu: s2crs count is 0 smrs mask is 0x8000
[    2.728417] arm-smmu arm-smmu.0.auto:        preserved 1 boot mapping

Robin if anything jumps out at you let me know, otherwise I will debug further.

-Jon

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 7/8] iommu/arm-smmu: Get associated RMR info and install bypass SMR
  2021-06-03  8:52   ` Jon Nettleton
@ 2021-06-03 11:27     ` Steven Price
  2021-06-03 11:51       ` Jon Nettleton
  0 siblings, 1 reply; 40+ messages in thread
From: Steven Price @ 2021-06-03 11:27 UTC (permalink / raw)
  To: Jon Nettleton, Shameer Kolothum
  Cc: linux-arm-kernel, ACPI Devel Maling List, iommu, Linuxarm,
	Lorenzo Pieralisi, joro, Robin Murphy, wanghuiqiang, Hanjun Guo,
	Sami.Mujawar, eric.auger, yangyicong

On 03/06/2021 09:52, Jon Nettleton wrote:
> On Mon, May 24, 2021 at 1:04 PM Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com> wrote:
>>
>> From: Jon Nettleton <jon@solid-run.com>
>>
>> Check if there is any RMR info associated with the devices behind
>> the SMMU and if any, install bypass SMRs for them. This is to
>> keep any ongoing traffic associated with these devices alive
>> when we enable/reset SMMU during probe().
>>
>> Signed-off-by: Jon Nettleton <jon@solid-run.com>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>> ---
>>  drivers/iommu/arm/arm-smmu/arm-smmu.c | 65 +++++++++++++++++++++++++++
>>  1 file changed, 65 insertions(+)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
>> index 6f72c4d208ca..56db3d3238fc 100644
>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
>> @@ -2042,6 +2042,67 @@ err_reset_platform_ops: __maybe_unused;
>>         return err;
>>  }
>>
>> +static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)
>> +{
>> +       struct list_head rmr_list;
>> +       struct iommu_resv_region *e;
>> +       int i, cnt = 0;
>> +       u32 smr;
>> +       u32 reg;
>> +
>> +       INIT_LIST_HEAD(&rmr_list);
>> +       if (iommu_dma_get_rmrs(dev_fwnode(smmu->dev), &rmr_list))
>> +               return;
>> +
>> +       reg = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_sCR0);
>> +
>> +       if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg & ARM_SMMU_sCR0_CLIENTPD)) {
>> +               /*
>> +                * SMMU is already enabled and disallowing bypass, so preserve
>> +                * the existing SMRs
>> +                */
>> +               for (i = 0; i < smmu->num_mapping_groups; i++) {
>> +                       smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
>> +                       if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))
>> +                               continue;
>> +                       smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
>> +                       smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK, smr);
>> +                       smmu->smrs[i].valid = true;
>> +               }
>> +       }
>> +
>> +       list_for_each_entry(e, &rmr_list, list) {
>> +               u32 sid = e->fw_data.rmr.sid;
>> +
>> +               i = arm_smmu_find_sme(smmu, sid, ~0);
>> +               if (i < 0)
>> +                       continue;
>> +               if (smmu->s2crs[i].count == 0) {
>> +                       smmu->smrs[i].id = sid;
>> +                       smmu->smrs[i].mask = ~0;

Looking at this code again, that mask looks wrong! According to the SMMU
spec MASK[i]==1 means ID[i] is ignored. I.e. this means completely
ignore the ID...

I'm not at all sure why they designed the hardware that way round.

>> +                       smmu->smrs[i].valid = true;
>> +               }
>> +               smmu->s2crs[i].count++;
>> +               smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
>> +               smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
>> +               smmu->s2crs[i].cbndx = 0xff;
>> +
>> +               cnt++;
>> +       }
>> +
>> +       if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg & ARM_SMMU_sCR0_CLIENTPD)) {
>> +               /* Remove the valid bit for unused SMRs */
>> +               for (i = 0; i < smmu->num_mapping_groups; i++) {
>> +                       if (smmu->s2crs[i].count == 0)
>> +                               smmu->smrs[i].valid = false;
>> +               }
>> +       }
>> +
>> +       dev_notice(smmu->dev, "\tpreserved %d boot mapping%s\n", cnt,
>> +                  cnt == 1 ? "" : "s");
>> +       iommu_dma_put_rmrs(dev_fwnode(smmu->dev), &rmr_list);
>> +}
>> +
>>  static int arm_smmu_device_probe(struct platform_device *pdev)
>>  {
>>         struct resource *res;
>> @@ -2168,6 +2229,10 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>>         }
>>
>>         platform_set_drvdata(pdev, smmu);
>> +
>> +       /* Check for RMRs and install bypass SMRs if any */
>> +       arm_smmu_rmr_install_bypass_smr(smmu);
>> +
>>         arm_smmu_device_reset(smmu);
>>         arm_smmu_test_smr_masks(smmu);
>>
>> --
>> 2.17.1
>>
> 
> Shameer and Robin
> 
> I finally got around to updating edk2 and the HoneyComb IORT tables to
> reflect the new standards.
> Out of the box the new patchset was generating errors immediatly after
> the smmu bringup.
> 
> arm-smmu arm-smmu.0.auto: Unhandled context fault: fsr=0x402, iova=0x2080000140,
> fsynr=0x1d0040, cbfrsynra=0x4000, cb=0
> 
> These errors were generated even with disable_bypass=0
> 
> I tracked down the issue to
> 
> This code is skipped as Robin said would be correct

If you're skipping the first bit of code, then that (hopefully) means
that the SMMU is disabled...

>> +       if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg & ARM_SMMU_sCR0_CLIENTPD)) {
>> +               /*
>> +                * SMMU is already enabled and disallowing bypass, so preserve
>> +                * the existing SMRs
>> +                */
>> +               for (i = 0; i < smmu->num_mapping_groups; i++) {
>> +                       smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
>> +                       if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))
>> +                               continue;
>> +                       smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
>> +                       smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK, smr);
>> +                       smmu->smrs[i].valid = true;
>> +               }[    2.707729] arm-smmu: setting up rmr list on 0x4000
> [    2.712598] arm-smmu: s2crs count is 0 smrs index 0x0
> [    2.717638] arm-smmu: s2crs count is 0 smrs id is 0x4000
> [    2.722939] arm-smmu: s2crs count is 0 smrs mask is 0x8000
> [    2.728417] arm-smmu arm-smmu.0.auto:        preserved 1 boot mapping
> 
>> +       }
> 
> Then this code block was hit which is correct
> 
>> +               if (smmu->s2crs[i].count == 0) {
>> +                       smmu->smrs[i].id = sid;
>> +                       smmu->smrs[i].mask = ~0;
>> +                       smmu->smrs[i].valid = true;
>> +               }
> 
> The mask was causing the issue.  If I think ammended that segment to read
> the mask as setup by the hardware everything was back to functioning both
> with and without disable_bypass set.

...so reading a mask from it doesn't sounds quite right.

Can you have a go with a corrected mask of '0' rather than all-1s and
see if that helps? My guess is that the mask of ~0 was causing multiple
matches in the SMRs which is 'UNPREDICTABLE'.

Sadly in my test setup there's only the one device behind the SMMU so
I didn't spot this (below patch works for me, but that's not saying
much).

Steve

--->8---
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index 56db3d3238fc..44831d0c78e4 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -2079,7 +2079,7 @@ static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)
                        continue;
                if (smmu->s2crs[i].count == 0) {
                        smmu->smrs[i].id = sid;
-                       smmu->smrs[i].mask = ~0;
+                       smmu->smrs[i].mask = 0;
                        smmu->smrs[i].valid = true;
                }
                smmu->s2crs[i].count++;

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 7/8] iommu/arm-smmu: Get associated RMR info and install bypass SMR
  2021-06-03 11:27     ` Steven Price
@ 2021-06-03 11:51       ` Jon Nettleton
  2021-06-13  7:40         ` Jon Nettleton
  0 siblings, 1 reply; 40+ messages in thread
From: Jon Nettleton @ 2021-06-03 11:51 UTC (permalink / raw)
  To: Steven Price
  Cc: Shameer Kolothum, linux-arm-kernel, ACPI Devel Maling List,
	iommu, Linuxarm, Lorenzo Pieralisi, joro, Robin Murphy,
	wanghuiqiang, Hanjun Guo, Sami.Mujawar, eric.auger, yangyicong

On Thu, Jun 3, 2021 at 1:27 PM Steven Price <steven.price@arm.com> wrote:
>
> On 03/06/2021 09:52, Jon Nettleton wrote:
> > On Mon, May 24, 2021 at 1:04 PM Shameer Kolothum
> > <shameerali.kolothum.thodi@huawei.com> wrote:
> >>
> >> From: Jon Nettleton <jon@solid-run.com>
> >>
> >> Check if there is any RMR info associated with the devices behind
> >> the SMMU and if any, install bypass SMRs for them. This is to
> >> keep any ongoing traffic associated with these devices alive
> >> when we enable/reset SMMU during probe().
> >>
> >> Signed-off-by: Jon Nettleton <jon@solid-run.com>
> >> Signed-off-by: Steven Price <steven.price@arm.com>
> >> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> >> ---
> >>  drivers/iommu/arm/arm-smmu/arm-smmu.c | 65 +++++++++++++++++++++++++++
> >>  1 file changed, 65 insertions(+)
> >>
> >> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> >> index 6f72c4d208ca..56db3d3238fc 100644
> >> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> >> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> >> @@ -2042,6 +2042,67 @@ err_reset_platform_ops: __maybe_unused;
> >>         return err;
> >>  }
> >>
> >> +static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)
> >> +{
> >> +       struct list_head rmr_list;
> >> +       struct iommu_resv_region *e;
> >> +       int i, cnt = 0;
> >> +       u32 smr;
> >> +       u32 reg;
> >> +
> >> +       INIT_LIST_HEAD(&rmr_list);
> >> +       if (iommu_dma_get_rmrs(dev_fwnode(smmu->dev), &rmr_list))
> >> +               return;
> >> +
> >> +       reg = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_sCR0);
> >> +
> >> +       if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg & ARM_SMMU_sCR0_CLIENTPD)) {
> >> +               /*
> >> +                * SMMU is already enabled and disallowing bypass, so preserve
> >> +                * the existing SMRs
> >> +                */
> >> +               for (i = 0; i < smmu->num_mapping_groups; i++) {
> >> +                       smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
> >> +                       if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))
> >> +                               continue;
> >> +                       smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
> >> +                       smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK, smr);
> >> +                       smmu->smrs[i].valid = true;
> >> +               }
> >> +       }
> >> +
> >> +       list_for_each_entry(e, &rmr_list, list) {
> >> +               u32 sid = e->fw_data.rmr.sid;
> >> +
> >> +               i = arm_smmu_find_sme(smmu, sid, ~0);
> >> +               if (i < 0)
> >> +                       continue;
> >> +               if (smmu->s2crs[i].count == 0) {
> >> +                       smmu->smrs[i].id = sid;
> >> +                       smmu->smrs[i].mask = ~0;
>
> Looking at this code again, that mask looks wrong! According to the SMMU
> spec MASK[i]==1 means ID[i] is ignored. I.e. this means completely
> ignore the ID...
>
> I'm not at all sure why they designed the hardware that way round.
>
> >> +                       smmu->smrs[i].valid = true;
> >> +               }
> >> +               smmu->s2crs[i].count++;
> >> +               smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
> >> +               smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
> >> +               smmu->s2crs[i].cbndx = 0xff;
> >> +
> >> +               cnt++;
> >> +       }
> >> +
> >> +       if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg & ARM_SMMU_sCR0_CLIENTPD)) {
> >> +               /* Remove the valid bit for unused SMRs */
> >> +               for (i = 0; i < smmu->num_mapping_groups; i++) {
> >> +                       if (smmu->s2crs[i].count == 0)
> >> +                               smmu->smrs[i].valid = false;
> >> +               }
> >> +       }
> >> +
> >> +       dev_notice(smmu->dev, "\tpreserved %d boot mapping%s\n", cnt,
> >> +                  cnt == 1 ? "" : "s");
> >> +       iommu_dma_put_rmrs(dev_fwnode(smmu->dev), &rmr_list);
> >> +}
> >> +
> >>  static int arm_smmu_device_probe(struct platform_device *pdev)
> >>  {
> >>         struct resource *res;
> >> @@ -2168,6 +2229,10 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
> >>         }
> >>
> >>         platform_set_drvdata(pdev, smmu);
> >> +
> >> +       /* Check for RMRs and install bypass SMRs if any */
> >> +       arm_smmu_rmr_install_bypass_smr(smmu);
> >> +
> >>         arm_smmu_device_reset(smmu);
> >>         arm_smmu_test_smr_masks(smmu);
> >>
> >> --
> >> 2.17.1
> >>
> >
> > Shameer and Robin
> >
> > I finally got around to updating edk2 and the HoneyComb IORT tables to
> > reflect the new standards.
> > Out of the box the new patchset was generating errors immediatly after
> > the smmu bringup.
> >
> > arm-smmu arm-smmu.0.auto: Unhandled context fault: fsr=0x402, iova=0x2080000140,
> > fsynr=0x1d0040, cbfrsynra=0x4000, cb=0
> >
> > These errors were generated even with disable_bypass=0
> >
> > I tracked down the issue to
> >
> > This code is skipped as Robin said would be correct
>
> If you're skipping the first bit of code, then that (hopefully) means
> that the SMMU is disabled...
>
> >> +       if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg & ARM_SMMU_sCR0_CLIENTPD)) {
> >> +               /*
> >> +                * SMMU is already enabled and disallowing bypass, so preserve
> >> +                * the existing SMRs
> >> +                */
> >> +               for (i = 0; i < smmu->num_mapping_groups; i++) {
> >> +                       smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
> >> +                       if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))
> >> +                               continue;
> >> +                       smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
> >> +                       smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK, smr);
> >> +                       smmu->smrs[i].valid = true;
> >> +               }[    2.707729] arm-smmu: setting up rmr list on 0x4000
> > [    2.712598] arm-smmu: s2crs count is 0 smrs index 0x0
> > [    2.717638] arm-smmu: s2crs count is 0 smrs id is 0x4000
> > [    2.722939] arm-smmu: s2crs count is 0 smrs mask is 0x8000
> > [    2.728417] arm-smmu arm-smmu.0.auto:        preserved 1 boot mapping
> >
> >> +       }
> >
> > Then this code block was hit which is correct
> >
> >> +               if (smmu->s2crs[i].count == 0) {
> >> +                       smmu->smrs[i].id = sid;
> >> +                       smmu->smrs[i].mask = ~0;
> >> +                       smmu->smrs[i].valid = true;
> >> +               }
> >
> > The mask was causing the issue.  If I think ammended that segment to read
> > the mask as setup by the hardware everything was back to functioning both
> > with and without disable_bypass set.
>
> ...so reading a mask from it doesn't sounds quite right.
>
> Can you have a go with a corrected mask of '0' rather than all-1s and
> see if that helps? My guess is that the mask of ~0 was causing multiple
> matches in the SMRs which is 'UNPREDICTABLE'.
>
> Sadly in my test setup there's only the one device behind the SMMU so
> I didn't spot this (below patch works for me, but that's not saying
> much).
>
> Steve
>
> --->8---
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index 56db3d3238fc..44831d0c78e4 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -2079,7 +2079,7 @@ static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)
>                         continue;
>                 if (smmu->s2crs[i].count == 0) {
>                         smmu->smrs[i].id = sid;
> -                       smmu->smrs[i].mask = ~0;
> +                       smmu->smrs[i].mask = 0;
>                         smmu->smrs[i].valid = true;
>                 }
>                 smmu->s2crs[i].count++;

Yes this works fine. Thanks

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory regions
  2021-05-26 17:11       ` Laurentiu Tudor
@ 2021-06-03 12:27         ` Jon Nettleton
  2021-06-03 12:32           ` Laurentiu Tudor
  0 siblings, 1 reply; 40+ messages in thread
From: Jon Nettleton @ 2021-06-03 12:27 UTC (permalink / raw)
  To: Laurentiu Tudor
  Cc: Shameerali Kolothum Thodi, linux-arm-kernel, linux-acpi, iommu,
	Linuxarm, steven.price, Guohanjun (Hanjun Guo),
	yangyicong, Sami.Mujawar, robin.murphy, wanghuiqiang

On Wed, May 26, 2021 at 7:11 PM Laurentiu Tudor <laurentiu.tudor@nxp.com> wrote:
>
>
>
> On 5/26/2021 7:36 PM, Shameerali Kolothum Thodi wrote:
> >
> >
> >> -----Original Message-----
> >> From: Laurentiu Tudor [mailto:laurentiu.tudor@nxp.com]
> >> Sent: 26 May 2021 08:53
> >> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> >> linux-arm-kernel@lists.infradead.org; linux-acpi@vger.kernel.org;
> >> iommu@lists.linux-foundation.org
> >> Cc: jon@solid-run.com; Linuxarm <linuxarm@huawei.com>;
> >> steven.price@arm.com; Guohanjun (Hanjun Guo) <guohanjun@huawei.com>;
> >> yangyicong <yangyicong@huawei.com>; Sami.Mujawar@arm.com;
> >> robin.murphy@arm.com; wanghuiqiang <wanghuiqiang@huawei.com>
> >> Subject: Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory
> >> regions
> >>
> >> Hi Shameer,
> >>
> >> On 5/24/2021 2:02 PM, Shameer Kolothum wrote:
> >>> Add a helper function that retrieves RMR memory descriptors
> >>> associated with a given IOMMU. This will be used by IOMMU
> >>> drivers to setup necessary mappings.
> >>>
> >>> Now that we have this, invoke it from the generic helper
> >>> interface.
> >>>
> >>> Signed-off-by: Shameer Kolothum
> >> <shameerali.kolothum.thodi@huawei.com>
> >>> ---
> >>>  drivers/acpi/arm64/iort.c | 50
> >> +++++++++++++++++++++++++++++++++++++++
> >>>  drivers/iommu/dma-iommu.c |  4 ++++
> >>>  include/linux/acpi_iort.h |  7 ++++++
> >>>  3 files changed, 61 insertions(+)
> >>>
> >>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> >>> index fea1ffaedf3b..01917caf58de 100644
> >>> --- a/drivers/acpi/arm64/iort.c
> >>> +++ b/drivers/acpi/arm64/iort.c
> >>> @@ -12,6 +12,7 @@
> >>>
> >>>  #include <linux/acpi_iort.h>
> >>>  #include <linux/bitfield.h>
> >>> +#include <linux/dma-iommu.h>
> >>>  #include <linux/iommu.h>
> >>>  #include <linux/kernel.h>
> >>>  #include <linux/list.h>
> >>> @@ -837,6 +838,53 @@ static inline int iort_add_device_replay(struct
> >> device *dev)
> >>>     return err;
> >>>  }
> >>>
> >>> +/**
> >>> + * iort_iommu_get_rmrs - Helper to retrieve RMR info associated with
> >> IOMMU
> >>> + * @iommu: fwnode for the IOMMU
> >>> + * @head: RMR list head to be populated
> >>> + *
> >>> + * Returns: 0 on success, <0 failure
> >>> + */
> >>> +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> >>> +                   struct list_head *head)
> >>> +{
> >>> +   struct iort_rmr_entry *e;
> >>> +   struct acpi_iort_node *iommu;
> >>> +   int rmrs = 0;
> >>> +
> >>> +   iommu = iort_get_iort_node(iommu_fwnode);
> >>> +   if (!iommu || list_empty(&iort_rmr_list))
> >>> +           return -ENODEV;
> >>> +
> >>> +   list_for_each_entry(e, &iort_rmr_list, list) {
> >>> +           int prot = IOMMU_READ | IOMMU_WRITE | IOMMU_NOEXEC |
> >> IOMMU_MMIO;
> >>
> >> We have a case with an IP block that needs EXEC rights on its reserved
> >> memory, so could you please drop the IOMMU_NOEXEC flag?
> >
> > Ok, I think I can drop that one if there are no other concerns. I was not quite
> > sure what to include here in the first place as the IORT spec is not giving any
> > further details about the RMR regions(May be the flags field can be extended to
> > describe these details).
> >
>
> That would be great, given that some preliminary investigations on my
> side revealed that our IP block seems to be quite sensitive to memory
> attributes. I need to spend some more time on this but at first sight
> looks like it needs cacheable, normal memory (not mmio mapping).
>
> ---
> Thanks & Best Regards, Laurentiu

Laurentiu,

Is this regarding the mc-bin memory block or another IP?  I am currently
running this patchset with IOMMU_NOEXEC under ACPI without any problems.

If so maybe we can touch base off list and align on the implementation.

-Jon

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory regions
  2021-06-03 12:27         ` Jon Nettleton
@ 2021-06-03 12:32           ` Laurentiu Tudor
  0 siblings, 0 replies; 40+ messages in thread
From: Laurentiu Tudor @ 2021-06-03 12:32 UTC (permalink / raw)
  To: Jon Nettleton
  Cc: Shameerali Kolothum Thodi, linux-arm-kernel, linux-acpi, iommu,
	Linuxarm, steven.price, Guohanjun (Hanjun Guo),
	yangyicong, Sami.Mujawar, robin.murphy, wanghuiqiang

Hi Jon,

On 6/3/2021 3:27 PM, Jon Nettleton wrote:
> On Wed, May 26, 2021 at 7:11 PM Laurentiu Tudor <laurentiu.tudor@nxp.com> wrote:
>>
>>
>>
>> On 5/26/2021 7:36 PM, Shameerali Kolothum Thodi wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Laurentiu Tudor [mailto:laurentiu.tudor@nxp.com]
>>>> Sent: 26 May 2021 08:53
>>>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
>>>> linux-arm-kernel@lists.infradead.org; linux-acpi@vger.kernel.org;
>>>> iommu@lists.linux-foundation.org
>>>> Cc: jon@solid-run.com; Linuxarm <linuxarm@huawei.com>;
>>>> steven.price@arm.com; Guohanjun (Hanjun Guo) <guohanjun@huawei.com>;
>>>> yangyicong <yangyicong@huawei.com>; Sami.Mujawar@arm.com;
>>>> robin.murphy@arm.com; wanghuiqiang <wanghuiqiang@huawei.com>
>>>> Subject: Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory
>>>> regions
>>>>
>>>> Hi Shameer,
>>>>
>>>> On 5/24/2021 2:02 PM, Shameer Kolothum wrote:
>>>>> Add a helper function that retrieves RMR memory descriptors
>>>>> associated with a given IOMMU. This will be used by IOMMU
>>>>> drivers to setup necessary mappings.
>>>>>
>>>>> Now that we have this, invoke it from the generic helper
>>>>> interface.
>>>>>
>>>>> Signed-off-by: Shameer Kolothum
>>>> <shameerali.kolothum.thodi@huawei.com>
>>>>> ---
>>>>>  drivers/acpi/arm64/iort.c | 50
>>>> +++++++++++++++++++++++++++++++++++++++
>>>>>  drivers/iommu/dma-iommu.c |  4 ++++
>>>>>  include/linux/acpi_iort.h |  7 ++++++
>>>>>  3 files changed, 61 insertions(+)
>>>>>
>>>>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
>>>>> index fea1ffaedf3b..01917caf58de 100644
>>>>> --- a/drivers/acpi/arm64/iort.c
>>>>> +++ b/drivers/acpi/arm64/iort.c
>>>>> @@ -12,6 +12,7 @@
>>>>>
>>>>>  #include <linux/acpi_iort.h>
>>>>>  #include <linux/bitfield.h>
>>>>> +#include <linux/dma-iommu.h>
>>>>>  #include <linux/iommu.h>
>>>>>  #include <linux/kernel.h>
>>>>>  #include <linux/list.h>
>>>>> @@ -837,6 +838,53 @@ static inline int iort_add_device_replay(struct
>>>> device *dev)
>>>>>     return err;
>>>>>  }
>>>>>
>>>>> +/**
>>>>> + * iort_iommu_get_rmrs - Helper to retrieve RMR info associated with
>>>> IOMMU
>>>>> + * @iommu: fwnode for the IOMMU
>>>>> + * @head: RMR list head to be populated
>>>>> + *
>>>>> + * Returns: 0 on success, <0 failure
>>>>> + */
>>>>> +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
>>>>> +                   struct list_head *head)
>>>>> +{
>>>>> +   struct iort_rmr_entry *e;
>>>>> +   struct acpi_iort_node *iommu;
>>>>> +   int rmrs = 0;
>>>>> +
>>>>> +   iommu = iort_get_iort_node(iommu_fwnode);
>>>>> +   if (!iommu || list_empty(&iort_rmr_list))
>>>>> +           return -ENODEV;
>>>>> +
>>>>> +   list_for_each_entry(e, &iort_rmr_list, list) {
>>>>> +           int prot = IOMMU_READ | IOMMU_WRITE | IOMMU_NOEXEC |
>>>> IOMMU_MMIO;
>>>>
>>>> We have a case with an IP block that needs EXEC rights on its reserved
>>>> memory, so could you please drop the IOMMU_NOEXEC flag?
>>>
>>> Ok, I think I can drop that one if there are no other concerns. I was not quite
>>> sure what to include here in the first place as the IORT spec is not giving any
>>> further details about the RMR regions(May be the flags field can be extended to
>>> describe these details).
>>>
>>
>> That would be great, given that some preliminary investigations on my
>> side revealed that our IP block seems to be quite sensitive to memory
>> attributes. I need to spend some more time on this but at first sight
>> looks like it needs cacheable, normal memory (not mmio mapping).
>>
>> ---
>> Thanks & Best Regards, Laurentiu
> 
> Laurentiu,
> 
> Is this regarding the mc-bin memory block or another IP?  I am currently
> running this patchset with IOMMU_NOEXEC under ACPI without any problems.

It's the MC firmware needing EXEC rights on its reserved memory. On my
side, with IOMMU_NOEXEC, as soon as the direct mappings are created I
get SMMU faults triggered by the FW.

> If so maybe we can touch base off list and align on the implementation.

Sure, just let me know when you have the time.

---
Best Regards, Laurentiu

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 7/8] iommu/arm-smmu: Get associated RMR info and install bypass SMR
  2021-06-03 11:51       ` Jon Nettleton
@ 2021-06-13  7:40         ` Jon Nettleton
  2021-06-14  9:23           ` Robin Murphy
  0 siblings, 1 reply; 40+ messages in thread
From: Jon Nettleton @ 2021-06-13  7:40 UTC (permalink / raw)
  To: Steven Price
  Cc: Shameer Kolothum, linux-arm-kernel, ACPI Devel Maling List,
	iommu, Linuxarm, Lorenzo Pieralisi, joro, Robin Murphy,
	wanghuiqiang, Hanjun Guo, Sami.Mujawar, eric.auger, yangyicong

On Thu, Jun 3, 2021 at 1:51 PM Jon Nettleton <jon@solid-run.com> wrote:
>
> On Thu, Jun 3, 2021 at 1:27 PM Steven Price <steven.price@arm.com> wrote:
> >
> > On 03/06/2021 09:52, Jon Nettleton wrote:
> > > On Mon, May 24, 2021 at 1:04 PM Shameer Kolothum
> > > <shameerali.kolothum.thodi@huawei.com> wrote:
> > >>
> > >> From: Jon Nettleton <jon@solid-run.com>
> > >>
> > >> Check if there is any RMR info associated with the devices behind
> > >> the SMMU and if any, install bypass SMRs for them. This is to
> > >> keep any ongoing traffic associated with these devices alive
> > >> when we enable/reset SMMU during probe().
> > >>
> > >> Signed-off-by: Jon Nettleton <jon@solid-run.com>
> > >> Signed-off-by: Steven Price <steven.price@arm.com>
> > >> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > >> ---
> > >>  drivers/iommu/arm/arm-smmu/arm-smmu.c | 65 +++++++++++++++++++++++++++
> > >>  1 file changed, 65 insertions(+)
> > >>
> > >> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > >> index 6f72c4d208ca..56db3d3238fc 100644
> > >> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > >> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > >> @@ -2042,6 +2042,67 @@ err_reset_platform_ops: __maybe_unused;
> > >>         return err;
> > >>  }
> > >>
> > >> +static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)
> > >> +{
> > >> +       struct list_head rmr_list;
> > >> +       struct iommu_resv_region *e;
> > >> +       int i, cnt = 0;
> > >> +       u32 smr;
> > >> +       u32 reg;
> > >> +
> > >> +       INIT_LIST_HEAD(&rmr_list);
> > >> +       if (iommu_dma_get_rmrs(dev_fwnode(smmu->dev), &rmr_list))
> > >> +               return;
> > >> +
> > >> +       reg = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_sCR0);
> > >> +
> > >> +       if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg & ARM_SMMU_sCR0_CLIENTPD)) {
> > >> +               /*
> > >> +                * SMMU is already enabled and disallowing bypass, so preserve
> > >> +                * the existing SMRs
> > >> +                */
> > >> +               for (i = 0; i < smmu->num_mapping_groups; i++) {
> > >> +                       smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
> > >> +                       if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))
> > >> +                               continue;
> > >> +                       smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
> > >> +                       smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK, smr);
> > >> +                       smmu->smrs[i].valid = true;
> > >> +               }
> > >> +       }
> > >> +
> > >> +       list_for_each_entry(e, &rmr_list, list) {
> > >> +               u32 sid = e->fw_data.rmr.sid;
> > >> +
> > >> +               i = arm_smmu_find_sme(smmu, sid, ~0);
> > >> +               if (i < 0)
> > >> +                       continue;
> > >> +               if (smmu->s2crs[i].count == 0) {
> > >> +                       smmu->smrs[i].id = sid;
> > >> +                       smmu->smrs[i].mask = ~0;
> >
> > Looking at this code again, that mask looks wrong! According to the SMMU
> > spec MASK[i]==1 means ID[i] is ignored. I.e. this means completely
> > ignore the ID...
> >
> > I'm not at all sure why they designed the hardware that way round.
> >
> > >> +                       smmu->smrs[i].valid = true;
> > >> +               }
> > >> +               smmu->s2crs[i].count++;
> > >> +               smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
> > >> +               smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
> > >> +               smmu->s2crs[i].cbndx = 0xff;
> > >> +
> > >> +               cnt++;
> > >> +       }
> > >> +
> > >> +       if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg & ARM_SMMU_sCR0_CLIENTPD)) {
> > >> +               /* Remove the valid bit for unused SMRs */
> > >> +               for (i = 0; i < smmu->num_mapping_groups; i++) {
> > >> +                       if (smmu->s2crs[i].count == 0)
> > >> +                               smmu->smrs[i].valid = false;
> > >> +               }
> > >> +       }
> > >> +
> > >> +       dev_notice(smmu->dev, "\tpreserved %d boot mapping%s\n", cnt,
> > >> +                  cnt == 1 ? "" : "s");
> > >> +       iommu_dma_put_rmrs(dev_fwnode(smmu->dev), &rmr_list);
> > >> +}
> > >> +
> > >>  static int arm_smmu_device_probe(struct platform_device *pdev)
> > >>  {
> > >>         struct resource *res;
> > >> @@ -2168,6 +2229,10 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
> > >>         }
> > >>
> > >>         platform_set_drvdata(pdev, smmu);
> > >> +
> > >> +       /* Check for RMRs and install bypass SMRs if any */
> > >> +       arm_smmu_rmr_install_bypass_smr(smmu);
> > >> +
> > >>         arm_smmu_device_reset(smmu);
> > >>         arm_smmu_test_smr_masks(smmu);
> > >>
> > >> --
> > >> 2.17.1
> > >>
> > >
> > > Shameer and Robin
> > >
> > > I finally got around to updating edk2 and the HoneyComb IORT tables to
> > > reflect the new standards.
> > > Out of the box the new patchset was generating errors immediatly after
> > > the smmu bringup.
> > >
> > > arm-smmu arm-smmu.0.auto: Unhandled context fault: fsr=0x402, iova=0x2080000140,
> > > fsynr=0x1d0040, cbfrsynra=0x4000, cb=0
> > >
> > > These errors were generated even with disable_bypass=0
> > >
> > > I tracked down the issue to
> > >
> > > This code is skipped as Robin said would be correct
> >
> > If you're skipping the first bit of code, then that (hopefully) means
> > that the SMMU is disabled...
> >
> > >> +       if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg & ARM_SMMU_sCR0_CLIENTPD)) {
> > >> +               /*
> > >> +                * SMMU is already enabled and disallowing bypass, so preserve
> > >> +                * the existing SMRs
> > >> +                */
> > >> +               for (i = 0; i < smmu->num_mapping_groups; i++) {
> > >> +                       smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
> > >> +                       if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))
> > >> +                               continue;
> > >> +                       smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
> > >> +                       smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK, smr);
> > >> +                       smmu->smrs[i].valid = true;
> > >> +               }[    2.707729] arm-smmu: setting up rmr list on 0x4000
> > > [    2.712598] arm-smmu: s2crs count is 0 smrs index 0x0
> > > [    2.717638] arm-smmu: s2crs count is 0 smrs id is 0x4000
> > > [    2.722939] arm-smmu: s2crs count is 0 smrs mask is 0x8000
> > > [    2.728417] arm-smmu arm-smmu.0.auto:        preserved 1 boot mapping
> > >
> > >> +       }
> > >
> > > Then this code block was hit which is correct
> > >
> > >> +               if (smmu->s2crs[i].count == 0) {
> > >> +                       smmu->smrs[i].id = sid;
> > >> +                       smmu->smrs[i].mask = ~0;
> > >> +                       smmu->smrs[i].valid = true;
> > >> +               }
> > >
> > > The mask was causing the issue.  If I think ammended that segment to read
> > > the mask as setup by the hardware everything was back to functioning both
> > > with and without disable_bypass set.
> >
> > ...so reading a mask from it doesn't sounds quite right.
> >
> > Can you have a go with a corrected mask of '0' rather than all-1s and
> > see if that helps? My guess is that the mask of ~0 was causing multiple
> > matches in the SMRs which is 'UNPREDICTABLE'.
> >
> > Sadly in my test setup there's only the one device behind the SMMU so
> > I didn't spot this (below patch works for me, but that's not saying
> > much).
> >
> > Steve
> >
> > --->8---
> > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > index 56db3d3238fc..44831d0c78e4 100644
> > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > @@ -2079,7 +2079,7 @@ static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)
> >                         continue;
> >                 if (smmu->s2crs[i].count == 0) {
> >                         smmu->smrs[i].id = sid;
> > -                       smmu->smrs[i].mask = ~0;
> > +                       smmu->smrs[i].mask = 0;
> >                         smmu->smrs[i].valid = true;
> >                 }
> >                 smmu->s2crs[i].count++;
>
> Yes this works fine. Thanks

Shameer,

Can you pick up this change into your next patch set?  Also are there
any objections to adding this to the SMMUv2 code from the maintainers?

-Jon

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 7/8] iommu/arm-smmu: Get associated RMR info and install bypass SMR
  2021-06-13  7:40         ` Jon Nettleton
@ 2021-06-14  9:23           ` Robin Murphy
  0 siblings, 0 replies; 40+ messages in thread
From: Robin Murphy @ 2021-06-14  9:23 UTC (permalink / raw)
  To: Jon Nettleton, Steven Price
  Cc: Shameer Kolothum, linux-arm-kernel, ACPI Devel Maling List,
	iommu, Linuxarm, Lorenzo Pieralisi, joro, wanghuiqiang,
	Hanjun Guo, Sami.Mujawar, eric.auger, yangyicong

On 2021-06-13 08:40, Jon Nettleton wrote:
> On Thu, Jun 3, 2021 at 1:51 PM Jon Nettleton <jon@solid-run.com> wrote:
>>
>> On Thu, Jun 3, 2021 at 1:27 PM Steven Price <steven.price@arm.com> wrote:
>>>
>>> On 03/06/2021 09:52, Jon Nettleton wrote:
>>>> On Mon, May 24, 2021 at 1:04 PM Shameer Kolothum
>>>> <shameerali.kolothum.thodi@huawei.com> wrote:
>>>>>
>>>>> From: Jon Nettleton <jon@solid-run.com>
>>>>>
>>>>> Check if there is any RMR info associated with the devices behind
>>>>> the SMMU and if any, install bypass SMRs for them. This is to
>>>>> keep any ongoing traffic associated with these devices alive
>>>>> when we enable/reset SMMU during probe().
>>>>>
>>>>> Signed-off-by: Jon Nettleton <jon@solid-run.com>
>>>>> Signed-off-by: Steven Price <steven.price@arm.com>
>>>>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>>>>> ---
>>>>>   drivers/iommu/arm/arm-smmu/arm-smmu.c | 65 +++++++++++++++++++++++++++
>>>>>   1 file changed, 65 insertions(+)
>>>>>
>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
>>>>> index 6f72c4d208ca..56db3d3238fc 100644
>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
>>>>> @@ -2042,6 +2042,67 @@ err_reset_platform_ops: __maybe_unused;
>>>>>          return err;
>>>>>   }
>>>>>
>>>>> +static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)
>>>>> +{
>>>>> +       struct list_head rmr_list;
>>>>> +       struct iommu_resv_region *e;
>>>>> +       int i, cnt = 0;
>>>>> +       u32 smr;
>>>>> +       u32 reg;
>>>>> +
>>>>> +       INIT_LIST_HEAD(&rmr_list);
>>>>> +       if (iommu_dma_get_rmrs(dev_fwnode(smmu->dev), &rmr_list))
>>>>> +               return;
>>>>> +
>>>>> +       reg = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_sCR0);
>>>>> +
>>>>> +       if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg & ARM_SMMU_sCR0_CLIENTPD)) {
>>>>> +               /*
>>>>> +                * SMMU is already enabled and disallowing bypass, so preserve
>>>>> +                * the existing SMRs
>>>>> +                */
>>>>> +               for (i = 0; i < smmu->num_mapping_groups; i++) {
>>>>> +                       smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
>>>>> +                       if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))
>>>>> +                               continue;
>>>>> +                       smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
>>>>> +                       smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK, smr);
>>>>> +                       smmu->smrs[i].valid = true;
>>>>> +               }
>>>>> +       }
>>>>> +
>>>>> +       list_for_each_entry(e, &rmr_list, list) {
>>>>> +               u32 sid = e->fw_data.rmr.sid;
>>>>> +
>>>>> +               i = arm_smmu_find_sme(smmu, sid, ~0);
>>>>> +               if (i < 0)
>>>>> +                       continue;
>>>>> +               if (smmu->s2crs[i].count == 0) {
>>>>> +                       smmu->smrs[i].id = sid;
>>>>> +                       smmu->smrs[i].mask = ~0;
>>>
>>> Looking at this code again, that mask looks wrong! According to the SMMU
>>> spec MASK[i]==1 means ID[i] is ignored. I.e. this means completely
>>> ignore the ID...
>>>
>>> I'm not at all sure why they designed the hardware that way round.
>>>
>>>>> +                       smmu->smrs[i].valid = true;
>>>>> +               }
>>>>> +               smmu->s2crs[i].count++;
>>>>> +               smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
>>>>> +               smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
>>>>> +               smmu->s2crs[i].cbndx = 0xff;
>>>>> +
>>>>> +               cnt++;
>>>>> +       }
>>>>> +
>>>>> +       if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg & ARM_SMMU_sCR0_CLIENTPD)) {
>>>>> +               /* Remove the valid bit for unused SMRs */
>>>>> +               for (i = 0; i < smmu->num_mapping_groups; i++) {
>>>>> +                       if (smmu->s2crs[i].count == 0)
>>>>> +                               smmu->smrs[i].valid = false;
>>>>> +               }
>>>>> +       }
>>>>> +
>>>>> +       dev_notice(smmu->dev, "\tpreserved %d boot mapping%s\n", cnt,
>>>>> +                  cnt == 1 ? "" : "s");
>>>>> +       iommu_dma_put_rmrs(dev_fwnode(smmu->dev), &rmr_list);
>>>>> +}
>>>>> +
>>>>>   static int arm_smmu_device_probe(struct platform_device *pdev)
>>>>>   {
>>>>>          struct resource *res;
>>>>> @@ -2168,6 +2229,10 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>>>>>          }
>>>>>
>>>>>          platform_set_drvdata(pdev, smmu);
>>>>> +
>>>>> +       /* Check for RMRs and install bypass SMRs if any */
>>>>> +       arm_smmu_rmr_install_bypass_smr(smmu);
>>>>> +
>>>>>          arm_smmu_device_reset(smmu);
>>>>>          arm_smmu_test_smr_masks(smmu);
>>>>>
>>>>> --
>>>>> 2.17.1
>>>>>
>>>>
>>>> Shameer and Robin
>>>>
>>>> I finally got around to updating edk2 and the HoneyComb IORT tables to
>>>> reflect the new standards.
>>>> Out of the box the new patchset was generating errors immediatly after
>>>> the smmu bringup.
>>>>
>>>> arm-smmu arm-smmu.0.auto: Unhandled context fault: fsr=0x402, iova=0x2080000140,
>>>> fsynr=0x1d0040, cbfrsynra=0x4000, cb=0
>>>>
>>>> These errors were generated even with disable_bypass=0
>>>>
>>>> I tracked down the issue to
>>>>
>>>> This code is skipped as Robin said would be correct
>>>
>>> If you're skipping the first bit of code, then that (hopefully) means
>>> that the SMMU is disabled...
>>>
>>>>> +       if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg & ARM_SMMU_sCR0_CLIENTPD)) {
>>>>> +               /*
>>>>> +                * SMMU is already enabled and disallowing bypass, so preserve
>>>>> +                * the existing SMRs
>>>>> +                */
>>>>> +               for (i = 0; i < smmu->num_mapping_groups; i++) {
>>>>> +                       smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
>>>>> +                       if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))
>>>>> +                               continue;
>>>>> +                       smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
>>>>> +                       smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK, smr);
>>>>> +                       smmu->smrs[i].valid = true;
>>>>> +               }[    2.707729] arm-smmu: setting up rmr list on 0x4000
>>>> [    2.712598] arm-smmu: s2crs count is 0 smrs index 0x0
>>>> [    2.717638] arm-smmu: s2crs count is 0 smrs id is 0x4000
>>>> [    2.722939] arm-smmu: s2crs count is 0 smrs mask is 0x8000
>>>> [    2.728417] arm-smmu arm-smmu.0.auto:        preserved 1 boot mapping
>>>>
>>>>> +       }
>>>>
>>>> Then this code block was hit which is correct
>>>>
>>>>> +               if (smmu->s2crs[i].count == 0) {
>>>>> +                       smmu->smrs[i].id = sid;
>>>>> +                       smmu->smrs[i].mask = ~0;
>>>>> +                       smmu->smrs[i].valid = true;
>>>>> +               }
>>>>
>>>> The mask was causing the issue.  If I think ammended that segment to read
>>>> the mask as setup by the hardware everything was back to functioning both
>>>> with and without disable_bypass set.
>>>
>>> ...so reading a mask from it doesn't sounds quite right.
>>>
>>> Can you have a go with a corrected mask of '0' rather than all-1s and
>>> see if that helps? My guess is that the mask of ~0 was causing multiple
>>> matches in the SMRs which is 'UNPREDICTABLE'.
>>>
>>> Sadly in my test setup there's only the one device behind the SMMU so
>>> I didn't spot this (below patch works for me, but that's not saying
>>> much).
>>>
>>> Steve
>>>
>>> --->8---
>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
>>> index 56db3d3238fc..44831d0c78e4 100644
>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
>>> @@ -2079,7 +2079,7 @@ static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)
>>>                          continue;
>>>                  if (smmu->s2crs[i].count == 0) {
>>>                          smmu->smrs[i].id = sid;
>>> -                       smmu->smrs[i].mask = ~0;
>>> +                       smmu->smrs[i].mask = 0;
>>>                          smmu->smrs[i].valid = true;
>>>                  }
>>>                  smmu->s2crs[i].count++;
>>
>> Yes this works fine. Thanks
> 
> Shameer,
> 
> Can you pick up this change into your next patch set?  Also are there
> any objections to adding this to the SMMUv2 code from the maintainers?

Urgh, I was rather confused here since I knew I'd already written a 
review of an earlier version pointing this out along with a couple of 
other issues... then I found it still sat in my drafts folder :(

Let me just "rebase" those comments to v5...

Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 7/8] iommu/arm-smmu: Get associated RMR info and install bypass SMR
  2021-05-24 11:02 ` [PATCH v5 7/8] iommu/arm-smmu: Get associated RMR info and install bypass SMR Shameer Kolothum
  2021-06-03  8:52   ` Jon Nettleton
@ 2021-06-14 10:06   ` Robin Murphy
  2021-06-14 16:51     ` Shameerali Kolothum Thodi
  2021-06-29  7:03     ` Jon Nettleton
  1 sibling, 2 replies; 40+ messages in thread
From: Robin Murphy @ 2021-06-14 10:06 UTC (permalink / raw)
  To: Shameer Kolothum, linux-arm-kernel, linux-acpi, iommu
  Cc: jon, linuxarm, steven.price, guohanjun, yangyicong, Sami.Mujawar,
	wanghuiqiang

On 2021-05-24 12:02, Shameer Kolothum wrote:
> From: Jon Nettleton <jon@solid-run.com>
> 
> Check if there is any RMR info associated with the devices behind
> the SMMU and if any, install bypass SMRs for them. This is to
> keep any ongoing traffic associated with these devices alive
> when we enable/reset SMMU during probe().
> 
> Signed-off-by: Jon Nettleton <jon@solid-run.com>
> Signed-off-by: Steven Price <steven.price@arm.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>   drivers/iommu/arm/arm-smmu/arm-smmu.c | 65 +++++++++++++++++++++++++++
>   1 file changed, 65 insertions(+)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index 6f72c4d208ca..56db3d3238fc 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -2042,6 +2042,67 @@ err_reset_platform_ops: __maybe_unused;
>   	return err;
>   }
>   
> +static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)
> +{
> +	struct list_head rmr_list;
> +	struct iommu_resv_region *e;
> +	int i, cnt = 0;
> +	u32 smr;
> +	u32 reg;
> +
> +	INIT_LIST_HEAD(&rmr_list);
> +	if (iommu_dma_get_rmrs(dev_fwnode(smmu->dev), &rmr_list))
> +		return;
> +
> +	reg = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_sCR0);
> +
> +	if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg & ARM_SMMU_sCR0_CLIENTPD)) {
> +		/*
> +		 * SMMU is already enabled and disallowing bypass, so preserve
> +		 * the existing SMRs
> +		 */
> +		for (i = 0; i < smmu->num_mapping_groups; i++) {
> +			smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));

To reiterate, just because a bootloader/crashed kernel/whatever may have 
left some configuration behind doesn't mean that it matters (e.g. what 
if these SMRs are pointing at translation contexts?). All we should be 
doing here is setting the relevant RMR accommodations in our "clean 
slate" software state before the reset routine applies it to the 
hardware, just like patch #5 does for SMMUv3.

Trying to safely reset an SMMU when we discover it with CLIENTPD=0 is 
really another issue entirely, and I'd say is beyond the scope of this 
series

> +			if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))
> +				continue;

Note that that's not even necessarily correct (thanks to EXIDS).

> +			smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
> +			smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK, smr);
> +			smmu->smrs[i].valid = true;
> +		}
> +	}
> +
> +	list_for_each_entry(e, &rmr_list, list) {
> +		u32 sid = e->fw_data.rmr.sid;
> +
> +		i = arm_smmu_find_sme(smmu, sid, ~0);
> +		if (i < 0)
> +			continue;
> +		if (smmu->s2crs[i].count == 0) {
> +			smmu->smrs[i].id = sid;
> +			smmu->smrs[i].mask = ~0;

This is very wrong (as has now already been pointed out).

> +			smmu->smrs[i].valid = true;
> +		}
> +		smmu->s2crs[i].count++;
> +		smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
> +		smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
> +		smmu->s2crs[i].cbndx = 0xff;

Nit: cbndx is left uninitialised for bypass/fault entries elsewhere, so 
there's little point touching it here.

> +
> +		cnt++;
> +	}
> +
> +	if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg & ARM_SMMU_sCR0_CLIENTPD)) {
> +		/* Remove the valid bit for unused SMRs */
> +		for (i = 0; i < smmu->num_mapping_groups; i++) {
> +			if (smmu->s2crs[i].count == 0)
> +				smmu->smrs[i].valid = false;
> +		}

If this dance is only about avoiding stream match conflicts when trying 
to reprogram live SMRs, simply turning the SMMU off beforehand would be 
a lot simpler.

Robin.

> +	}
> +
> +	dev_notice(smmu->dev, "\tpreserved %d boot mapping%s\n", cnt,
> +		   cnt == 1 ? "" : "s");
> +	iommu_dma_put_rmrs(dev_fwnode(smmu->dev), &rmr_list);
> +}
> +
>   static int arm_smmu_device_probe(struct platform_device *pdev)
>   {
>   	struct resource *res;
> @@ -2168,6 +2229,10 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>   	}
>   
>   	platform_set_drvdata(pdev, smmu);
> +
> +	/* Check for RMRs and install bypass SMRs if any */
> +	arm_smmu_rmr_install_bypass_smr(smmu);
> +
>   	arm_smmu_device_reset(smmu);
>   	arm_smmu_test_smr_masks(smmu);
>   
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 6/8] iommu/arm-smmu-v3: Get associated RMR info and install
  2021-05-24 11:02 ` [PATCH v5 6/8] iommu/arm-smmu-v3: Get associated RMR info and install Shameer Kolothum
@ 2021-06-14 10:15   ` Robin Murphy
  0 siblings, 0 replies; 40+ messages in thread
From: Robin Murphy @ 2021-06-14 10:15 UTC (permalink / raw)
  To: Shameer Kolothum, linux-arm-kernel, linux-acpi, iommu
  Cc: jon, linuxarm, steven.price, guohanjun, yangyicong, Sami.Mujawar,
	wanghuiqiang

On 2021-05-24 12:02, Shameer Kolothum wrote:
> Check if there is any RMR info associated with the devices behind
> the SMMUv3 and if any, install bypass STEs for them. This is to
> keep any ongoing traffic associated with these devices alive
> when we enable/reset SMMUv3 during probe().
> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 36 +++++++++++++++++++++
>   1 file changed, 36 insertions(+)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index f9195b740f48..be1563e06732 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -3574,6 +3574,39 @@ static void __iomem *arm_smmu_ioremap(struct device *dev, resource_size_t start,
>   	return devm_ioremap_resource(dev, &res);
>   }
>   
> +static void arm_smmu_rmr_install_bypass_ste(struct arm_smmu_device *smmu)
> +{
> +	struct list_head rmr_list;
> +	struct iommu_resv_region *e;
> +	int ret;
> +
> +	INIT_LIST_HEAD(&rmr_list);
> +	if (iommu_dma_get_rmrs(dev_fwnode(smmu->dev), &rmr_list))
> +		return;
> +
> +	/*
> +	 * Since, we don't have a mechanism to differentiate the RMR
> +	 * SIDs that has an ongoing live stream, install bypass STEs
> +	 * for all the reported ones.
> +	 */

I don't really follow that comment. The point of being reserved is that 
we don't know how and why the device is using them, and we have little 
need to care - we are simply required to preserve an effective unity 
mapping at all times. I don't see any value in trying to second-guess 
things beyond that, as this appears to suggest.

Robin.

> +	list_for_each_entry(e, &rmr_list, list) {
> +		__le64 *step;
> +		u32 sid = e->fw_data.rmr.sid;
> +
> +		ret = arm_smmu_init_sid_strtab(smmu, sid);
> +		if (ret) {
> +			dev_err(smmu->dev, "RMR bypass(0x%x) failed\n",
> +				sid);
> +			continue;
> +		}
> +
> +		step = arm_smmu_get_step_for_sid(smmu, sid);
> +		arm_smmu_write_strtab_ent(NULL, sid, step, true);
> +	}
> +
> +	iommu_dma_put_rmrs(dev_fwnode(smmu->dev), &rmr_list);
> +}
> +
>   static int arm_smmu_device_probe(struct platform_device *pdev)
>   {
>   	int irq, ret;
> @@ -3657,6 +3690,9 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>   	/* Record our private device structure */
>   	platform_set_drvdata(pdev, smmu);
>   
> +	/* Check for RMRs and install bypass STEs if any */
> +	arm_smmu_rmr_install_bypass_ste(smmu);
> +
>   	/* Reset the device */
>   	ret = arm_smmu_device_reset(smmu, bypass);
>   	if (ret)
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 5/8] iommu/arm-smmu-v3: Add bypass flag to arm_smmu_write_strtab_ent()
  2021-05-24 11:02 ` [PATCH v5 5/8] iommu/arm-smmu-v3: Add bypass flag to arm_smmu_write_strtab_ent() Shameer Kolothum
@ 2021-06-14 10:23   ` Robin Murphy
  2021-06-14 12:51     ` Shameerali Kolothum Thodi
  0 siblings, 1 reply; 40+ messages in thread
From: Robin Murphy @ 2021-06-14 10:23 UTC (permalink / raw)
  To: Shameer Kolothum, linux-arm-kernel, linux-acpi, iommu
  Cc: linuxarm, lorenzo.pieralisi, joro, wanghuiqiang, guohanjun,
	steven.price, Sami.Mujawar, jon, eric.auger, yangyicong

On 2021-05-24 12:02, Shameer Kolothum wrote:
> By default, disable_bypass is set and any dev without an iommu domain
> installs STE with CFG_ABORT during arm_smmu_init_bypass_stes(). Introduce
> a "bypass" flag to arm_smmu_write_strtab_ent() so that we can force it to
> install CFG_BYPASS STE for specific SIDs. This will be useful in follow
> up patch to install bypass for IORT RMR SIDs.
> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 754bad6092c1..f9195b740f48 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1174,7 +1174,7 @@ static void arm_smmu_sync_ste_for_sid(struct arm_smmu_device *smmu, u32 sid)
>   }
>   
>   static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
> -				      __le64 *dst)
> +				      __le64 *dst, bool bypass)
>   {
>   	/*
>   	 * This is hideously complicated, but we only really care about
> @@ -1245,7 +1245,7 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
>   
>   	/* Bypass/fault */
>   	if (!smmu_domain || !(s1_cfg || s2_cfg)) {
> -		if (!smmu_domain && disable_bypass)
> +		if (!smmu_domain && disable_bypass && !bypass)

Umm...

>   			val |= FIELD_PREP(STRTAB_STE_0_CFG, STRTAB_STE_0_CFG_ABORT);
>   		else
>   			val |= FIELD_PREP(STRTAB_STE_0_CFG, STRTAB_STE_0_CFG_BYPASS);
> @@ -1320,7 +1320,7 @@ static void arm_smmu_init_bypass_stes(__le64 *strtab, unsigned int nent)
>   	unsigned int i;
>   
>   	for (i = 0; i < nent; ++i) {
> -		arm_smmu_write_strtab_ent(NULL, -1, strtab);
> +		arm_smmu_write_strtab_ent(NULL, -1, strtab, false);

...and in particular, an operation named "init_bypass_stes" passing 
bypass=false is just breaking my brain. Can we pull the logic for 
default bypass/fault out to here as part of the refactoring so that it 
actually makes sense?

Robin.

>   		strtab += STRTAB_STE_DWORDS;
>   	}
>   }
> @@ -2097,7 +2097,7 @@ static void arm_smmu_install_ste_for_dev(struct arm_smmu_master *master)
>   		if (j < i)
>   			continue;
>   
> -		arm_smmu_write_strtab_ent(master, sid, step);
> +		arm_smmu_write_strtab_ent(master, sid, step, false);
>   	}
>   }
>   
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory regions
  2021-05-26 16:36     ` Shameerali Kolothum Thodi
  2021-05-26 17:11       ` Laurentiu Tudor
  2021-05-27  4:25       ` Jon Nettleton
@ 2021-06-14 10:35       ` Robin Murphy
  2 siblings, 0 replies; 40+ messages in thread
From: Robin Murphy @ 2021-06-14 10:35 UTC (permalink / raw)
  To: Shameerali Kolothum Thodi, Laurentiu Tudor, linux-arm-kernel,
	linux-acpi, iommu
  Cc: jon, Linuxarm, steven.price, Guohanjun (Hanjun Guo),
	yangyicong, Sami.Mujawar, wanghuiqiang

On 2021-05-26 17:36, Shameerali Kolothum Thodi wrote:
> 
> 
>> -----Original Message-----
>> From: Laurentiu Tudor [mailto:laurentiu.tudor@nxp.com]
>> Sent: 26 May 2021 08:53
>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
>> linux-arm-kernel@lists.infradead.org; linux-acpi@vger.kernel.org;
>> iommu@lists.linux-foundation.org
>> Cc: jon@solid-run.com; Linuxarm <linuxarm@huawei.com>;
>> steven.price@arm.com; Guohanjun (Hanjun Guo) <guohanjun@huawei.com>;
>> yangyicong <yangyicong@huawei.com>; Sami.Mujawar@arm.com;
>> robin.murphy@arm.com; wanghuiqiang <wanghuiqiang@huawei.com>
>> Subject: Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory
>> regions
>>
>> Hi Shameer,
>>
>> On 5/24/2021 2:02 PM, Shameer Kolothum wrote:
>>> Add a helper function that retrieves RMR memory descriptors
>>> associated with a given IOMMU. This will be used by IOMMU
>>> drivers to setup necessary mappings.
>>>
>>> Now that we have this, invoke it from the generic helper
>>> interface.
>>>
>>> Signed-off-by: Shameer Kolothum
>> <shameerali.kolothum.thodi@huawei.com>
>>> ---
>>>   drivers/acpi/arm64/iort.c | 50
>> +++++++++++++++++++++++++++++++++++++++
>>>   drivers/iommu/dma-iommu.c |  4 ++++
>>>   include/linux/acpi_iort.h |  7 ++++++
>>>   3 files changed, 61 insertions(+)
>>>
>>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
>>> index fea1ffaedf3b..01917caf58de 100644
>>> --- a/drivers/acpi/arm64/iort.c
>>> +++ b/drivers/acpi/arm64/iort.c
>>> @@ -12,6 +12,7 @@
>>>
>>>   #include <linux/acpi_iort.h>
>>>   #include <linux/bitfield.h>
>>> +#include <linux/dma-iommu.h>
>>>   #include <linux/iommu.h>
>>>   #include <linux/kernel.h>
>>>   #include <linux/list.h>
>>> @@ -837,6 +838,53 @@ static inline int iort_add_device_replay(struct
>> device *dev)
>>>   	return err;
>>>   }
>>>
>>> +/**
>>> + * iort_iommu_get_rmrs - Helper to retrieve RMR info associated with
>> IOMMU
>>> + * @iommu: fwnode for the IOMMU
>>> + * @head: RMR list head to be populated
>>> + *
>>> + * Returns: 0 on success, <0 failure
>>> + */
>>> +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
>>> +			struct list_head *head)
>>> +{
>>> +	struct iort_rmr_entry *e;
>>> +	struct acpi_iort_node *iommu;
>>> +	int rmrs = 0;
>>> +
>>> +	iommu = iort_get_iort_node(iommu_fwnode);
>>> +	if (!iommu || list_empty(&iort_rmr_list))
>>> +		return -ENODEV;
>>> +
>>> +	list_for_each_entry(e, &iort_rmr_list, list) {
>>> +		int prot = IOMMU_READ | IOMMU_WRITE | IOMMU_NOEXEC |
>> IOMMU_MMIO;
>>
>> We have a case with an IP block that needs EXEC rights on its reserved
>> memory, so could you please drop the IOMMU_NOEXEC flag?
> 
> Ok, I think I can drop that one if there are no other concerns. I was not quite
> sure what to include here in the first place as the IORT spec is not giving any
> further details about the RMR regions(May be the flags field can be extended to
> describe these details).

Right, it's reserved for the device to use *somehow* - that's all we 
know, so it's not our place to try and enforce any restrictive 
permissions. All it could possibly achieve is the ability to log an 
error to say "a thing did an unknown thing in a way we didn't expect!", 
and there's hardly much value in that.

Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 1/8] ACPI/IORT: Add support for RMR node parsing
  2021-05-24 11:02 ` [PATCH v5 1/8] ACPI/IORT: Add support for RMR node parsing Shameer Kolothum
@ 2021-06-14 11:14   ` Robin Murphy
  2021-06-14 12:37     ` Shameerali Kolothum Thodi
  0 siblings, 1 reply; 40+ messages in thread
From: Robin Murphy @ 2021-06-14 11:14 UTC (permalink / raw)
  To: Shameer Kolothum, linux-arm-kernel, linux-acpi, iommu
  Cc: linuxarm, lorenzo.pieralisi, joro, wanghuiqiang, guohanjun,
	steven.price, Sami.Mujawar, jon, eric.auger, yangyicong

On 2021-05-24 12:02, Shameer Kolothum wrote:
> Add support for parsing RMR node information from ACPI.
> Find associated stream id and smmu node info from the
> RMR node and populate a linked list with RMR memory
> descriptors.
> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>   drivers/acpi/arm64/iort.c | 104 +++++++++++++++++++++++++++++++++++++-
>   1 file changed, 103 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 3912a1f6058e..fea1ffaedf3b 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -40,6 +40,19 @@ struct iort_fwnode {
>   static LIST_HEAD(iort_fwnode_list);
>   static DEFINE_SPINLOCK(iort_fwnode_lock);
>   
> +/*
> + * One entry for IORT RMR.
> + */
> +struct iort_rmr_entry {
> +	struct list_head list;
> +	u32 sid;
> +	struct acpi_iort_node *smmu;
> +	struct acpi_iort_rmr_desc *rmr_desc;
> +	u32 flags;
> +};
> +
> +static LIST_HEAD(iort_rmr_list);         /* list of RMR regions from ACPI */
> +
>   /**
>    * iort_set_fwnode() - Create iort_fwnode and use it to register
>    *		       iommu data in the iort_fwnode_list
> @@ -393,7 +406,8 @@ static struct acpi_iort_node *iort_node_get_id(struct acpi_iort_node *node,
>   		if (node->type == ACPI_IORT_NODE_NAMED_COMPONENT ||
>   		    node->type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX ||
>   		    node->type == ACPI_IORT_NODE_SMMU_V3 ||
> -		    node->type == ACPI_IORT_NODE_PMCG) {
> +		    node->type == ACPI_IORT_NODE_PMCG ||
> +		    node->type == ACPI_IORT_NODE_RMR) {
>   			*id_out = map->output_base;
>   			return parent;
>   		}
> @@ -1660,6 +1674,91 @@ static void __init iort_enable_acs(struct acpi_iort_node *iort_node)
>   #else
>   static inline void iort_enable_acs(struct acpi_iort_node *iort_node) { }
>   #endif
> +static int iort_rmr_desc_valid(struct acpi_iort_rmr_desc *desc, u32 count)
> +{
> +	int i, j;
> +
> +	for (i = 0; i < count; i++) {
> +		u64 end, start = desc[i].base_address, length = desc[i].length;
> +
> +		if (!IS_ALIGNED(start, SZ_64K) || !IS_ALIGNED(length, SZ_64K))
> +			return -EINVAL;

We should certainly FW_BUG for this, but maybe it's OK to continue, 
since all we should need to do is round our iommu_resv_regions to at 
least PAGE_SIZE. That seems possibly better than ignoring them and 
having things potentially blow up later (especially if an end user 
exercises the system more thoroughly than the firmware developer tested 
it, which in at least one case I've seen may be "at all"...)

> +		end = start + length - 1;
> +
> +		/* Check for address overlap */
> +		for (j = i + 1; j < count; j++) {
> +			u64 e_start = desc[j].base_address;
> +			u64 e_end = e_start + desc[j].length - 1;
> +
> +			if (start <= e_end && end >= e_start)
> +				return -EINVAL;

Similarly it's not *too* hard to trim overlaps; I guess it's really a 
question of whether we want to bother :/

> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int __init iort_parse_rmr(struct acpi_iort_node *iort_node)
> +{
> +	struct acpi_iort_node *smmu;
> +	struct iort_rmr_entry *e;
> +	struct acpi_iort_rmr *rmr;
> +	struct acpi_iort_rmr_desc *rmr_desc;
> +	u32 map_count = iort_node->mapping_count;
> +	u32  sid;

Nit: extra space.

> +	int i, ret = 0;
> +
> +	if (iort_node->type != ACPI_IORT_NODE_RMR)
> +		return 0;
> +
> +	if (!iort_node->mapping_offset || map_count != 1) {

Beware that there was some discussion about allowing multiple SIDs to 
share an RMR descriptor, since there are potential use-cases which would 
otherwise lead to excessive duplication (e.g. an MSI doorbell carveout 
in a VM which would effectively apply to all possible PCI RIDs). I think 
the conclusion we reached was that disallowing that was fairly 
arbitrary, and could possibly be relaxed in future. It looks like the 
design of things here could grow to fit that fairly easily though, so I 
don't think it's a major concern.

Robin.

> +		pr_err(FW_BUG "Invalid ID mapping, skipping RMR node %p\n",
> +		       iort_node);
> +		return -EINVAL;
> +	}
> +
> +	/* Retrieve associated smmu and stream id */
> +	smmu = iort_node_get_id(iort_node, &sid, 0);
> +	if (!smmu) {
> +		pr_err(FW_BUG "Invalid SMMU reference, skipping RMR node %p\n",
> +		       iort_node);
> +		return -EINVAL;
> +	}
> +
> +	/* Retrieve RMR data */
> +	rmr = (struct acpi_iort_rmr *)iort_node->node_data;
> +	if (!rmr->rmr_offset || !rmr->rmr_count) {
> +		pr_err(FW_BUG "Invalid RMR descriptor array, skipping RMR node %p\n",
> +		       iort_node);
> +		return -EINVAL;
> +	}
> +
> +	rmr_desc = ACPI_ADD_PTR(struct acpi_iort_rmr_desc, iort_node,
> +				rmr->rmr_offset);
> +
> +	ret = iort_rmr_desc_valid(rmr_desc, rmr->rmr_count);
> +	if (ret) {
> +		pr_err(FW_BUG "Invalid RMR descriptor[%d] for node %p, skipping...\n",
> +		       i, iort_node);
> +		return ret;
> +	}
> +
> +	for (i = 0; i < rmr->rmr_count; i++, rmr_desc++) {
> +		e = kmalloc(sizeof(*e), GFP_KERNEL);
> +		if (!e)
> +			return -ENOMEM;
> +
> +		e->sid = sid;
> +		e->smmu = smmu;
> +		e->rmr_desc = rmr_desc;
> +		e->flags = rmr->flags;
> +
> +		list_add_tail(&e->list, &iort_rmr_list);
> +	}
> +
> +	return 0;
> +}
>   
>   static void __init iort_init_platform_devices(void)
>   {
> @@ -1689,6 +1788,9 @@ static void __init iort_init_platform_devices(void)
>   
>   		iort_enable_acs(iort_node);
>   
> +		if (iort_table->revision == 3)
> +			iort_parse_rmr(iort_node);
> +
>   		ops = iort_get_dev_cfg(iort_node);
>   		if (ops) {
>   			fwnode = acpi_alloc_fwnode_static();
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory regions
  2021-05-24 11:02 ` [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory regions Shameer Kolothum
  2021-05-26  7:53   ` Laurentiu Tudor
@ 2021-06-14 11:23   ` Robin Murphy
  2021-06-14 12:49     ` Shameerali Kolothum Thodi
  1 sibling, 1 reply; 40+ messages in thread
From: Robin Murphy @ 2021-06-14 11:23 UTC (permalink / raw)
  To: Shameer Kolothum, linux-arm-kernel, linux-acpi, iommu
  Cc: jon, linuxarm, steven.price, guohanjun, yangyicong, Sami.Mujawar,
	wanghuiqiang

On 2021-05-24 12:02, Shameer Kolothum wrote:
> Add a helper function that retrieves RMR memory descriptors
> associated with a given IOMMU. This will be used by IOMMU
> drivers to setup necessary mappings.
> 
> Now that we have this, invoke it from the generic helper
> interface.
> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>   drivers/acpi/arm64/iort.c | 50 +++++++++++++++++++++++++++++++++++++++
>   drivers/iommu/dma-iommu.c |  4 ++++
>   include/linux/acpi_iort.h |  7 ++++++
>   3 files changed, 61 insertions(+)
> 
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index fea1ffaedf3b..01917caf58de 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -12,6 +12,7 @@
>   
>   #include <linux/acpi_iort.h>
>   #include <linux/bitfield.h>
> +#include <linux/dma-iommu.h>
>   #include <linux/iommu.h>
>   #include <linux/kernel.h>
>   #include <linux/list.h>
> @@ -837,6 +838,53 @@ static inline int iort_add_device_replay(struct device *dev)
>   	return err;
>   }
>   
> +/**
> + * iort_iommu_get_rmrs - Helper to retrieve RMR info associated with IOMMU
> + * @iommu: fwnode for the IOMMU
> + * @head: RMR list head to be populated
> + *
> + * Returns: 0 on success, <0 failure
> + */
> +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> +			struct list_head *head)
> +{
> +	struct iort_rmr_entry *e;
> +	struct acpi_iort_node *iommu;
> +	int rmrs = 0;
> +
> +	iommu = iort_get_iort_node(iommu_fwnode);
> +	if (!iommu || list_empty(&iort_rmr_list))
> +		return -ENODEV;
> +
> +	list_for_each_entry(e, &iort_rmr_list, list) {
> +		int prot = IOMMU_READ | IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> +		struct iommu_resv_region *region;
> +		enum iommu_resv_type type;
> +		struct acpi_iort_rmr_desc *rmr_desc;
> +
> +		if (e->smmu != iommu)
> +			continue;
> +
> +		rmr_desc = e->rmr_desc;
> +		if (e->flags & IOMMU_RMR_REMAP_PERMITTED)
> +			type = IOMMU_RESV_DIRECT_RELAXABLE;
> +		else
> +			type = IOMMU_RESV_DIRECT;

Wasn't the idea that we can do all this during the initial parsing, i.e. 
we don't even have iort_rmr_entry, we store them as iommu_resv_regions 
and simply kmemdup() or whatever at this point?

Robin.

> +
> +		region = iommu_alloc_resv_region(rmr_desc->base_address,
> +						 rmr_desc->length,
> +						 prot, type);
> +		if (region) {
> +			region->fw_data.rmr.flags = e->flags;
> +			region->fw_data.rmr.sid = e->sid;
> +			list_add_tail(&region->list, head);
> +			rmrs++;
> +		}
> +	}
> +
> +	return (rmrs == 0) ? -ENODEV : 0;
> +}
> +
>   /**
>    * iort_iommu_msi_get_resv_regions - Reserved region driver helper
>    * @dev: Device from iommu_get_resv_regions()
> @@ -1108,6 +1156,8 @@ int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head)
>   const struct iommu_ops *iort_iommu_configure_id(struct device *dev,
>   						const u32 *input_id)
>   { return NULL; }
> +int iort_iommu_get_rmrs(struct fwnode_handle *fwnode, struct list_head *head)
> +{ return -ENODEV; }
>   #endif
>   
>   static int nc_dma_get_range(struct device *dev, u64 *size)
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 229ec65d98be..f893d460cfa4 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -185,6 +185,9 @@ EXPORT_SYMBOL(iommu_put_dma_cookie);
>   int iommu_dma_get_rmrs(struct fwnode_handle *iommu_fwnode,
>   		       struct list_head *list)
>   {
> +	if (!is_of_node(iommu_fwnode))
> +		return iort_iommu_get_rmrs(iommu_fwnode, list);
> +
>   	return -EINVAL;
>   }
>   EXPORT_SYMBOL(iommu_dma_get_rmrs);
> @@ -200,6 +203,7 @@ EXPORT_SYMBOL(iommu_dma_get_rmrs);
>   void iommu_dma_put_rmrs(struct fwnode_handle *iommu_fwnode,
>   			struct list_head *list)
>   {
> +	generic_iommu_put_resv_regions(iommu_fwnode->dev, list);
>   }
>   EXPORT_SYMBOL(iommu_dma_put_rmrs);
>   
> diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
> index 1a12baa58e40..e8c45fa59531 100644
> --- a/include/linux/acpi_iort.h
> +++ b/include/linux/acpi_iort.h
> @@ -39,6 +39,8 @@ const struct iommu_ops *iort_iommu_configure_id(struct device *dev,
>   						const u32 *id_in);
>   int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head);
>   phys_addr_t acpi_iort_dma_get_max_cpu_address(void);
> +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> +			struct list_head *list);
>   #else
>   static inline void acpi_iort_init(void) { }
>   static inline u32 iort_msi_map_id(struct device *dev, u32 id)
> @@ -59,6 +61,11 @@ int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head *head)
>   
>   static inline phys_addr_t acpi_iort_dma_get_max_cpu_address(void)
>   { return PHYS_ADDR_MAX; }
> +
> +static inline
> +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> +			struct list_head *list)
> +{ return -ENODEV; }
>   #endif
>   
>   #endif /* __ACPI_IORT_H__ */
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 40+ messages in thread

* RE: [PATCH v5 1/8] ACPI/IORT: Add support for RMR node parsing
  2021-06-14 11:14   ` Robin Murphy
@ 2021-06-14 12:37     ` Shameerali Kolothum Thodi
  0 siblings, 0 replies; 40+ messages in thread
From: Shameerali Kolothum Thodi @ 2021-06-14 12:37 UTC (permalink / raw)
  To: Robin Murphy, linux-arm-kernel, linux-acpi, iommu
  Cc: Linuxarm, lorenzo.pieralisi, joro, wanghuiqiang,
	Guohanjun (Hanjun Guo),
	steven.price, Sami.Mujawar, jon, eric.auger, yangyicong

Hi Robin,

> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy@arm.com]
> Sent: 14 June 2021 12:15
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> linux-arm-kernel@lists.infradead.org; linux-acpi@vger.kernel.org;
> iommu@lists.linux-foundation.org
> Cc: Linuxarm <linuxarm@huawei.com>; lorenzo.pieralisi@arm.com;
> joro@8bytes.org; wanghuiqiang <wanghuiqiang@huawei.com>; Guohanjun
> (Hanjun Guo) <guohanjun@huawei.com>; steven.price@arm.com;
> Sami.Mujawar@arm.com; jon@solid-run.com; eric.auger@redhat.com;
> yangyicong <yangyicong@huawei.com>
> Subject: Re: [PATCH v5 1/8] ACPI/IORT: Add support for RMR node parsing
> 
> On 2021-05-24 12:02, Shameer Kolothum wrote:
> > Add support for parsing RMR node information from ACPI.
> > Find associated stream id and smmu node info from the
> > RMR node and populate a linked list with RMR memory
> > descriptors.
> >
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> > ---
> >   drivers/acpi/arm64/iort.c | 104
> +++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 103 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > index 3912a1f6058e..fea1ffaedf3b 100644
> > --- a/drivers/acpi/arm64/iort.c
> > +++ b/drivers/acpi/arm64/iort.c
> > @@ -40,6 +40,19 @@ struct iort_fwnode {
> >   static LIST_HEAD(iort_fwnode_list);
> >   static DEFINE_SPINLOCK(iort_fwnode_lock);
> >
> > +/*
> > + * One entry for IORT RMR.
> > + */
> > +struct iort_rmr_entry {
> > +	struct list_head list;
> > +	u32 sid;
> > +	struct acpi_iort_node *smmu;
> > +	struct acpi_iort_rmr_desc *rmr_desc;
> > +	u32 flags;
> > +};
> > +
> > +static LIST_HEAD(iort_rmr_list);         /* list of RMR regions from ACPI
> */
> > +
> >   /**
> >    * iort_set_fwnode() - Create iort_fwnode and use it to register
> >    *		       iommu data in the iort_fwnode_list
> > @@ -393,7 +406,8 @@ static struct acpi_iort_node
> *iort_node_get_id(struct acpi_iort_node *node,
> >   		if (node->type == ACPI_IORT_NODE_NAMED_COMPONENT ||
> >   		    node->type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX ||
> >   		    node->type == ACPI_IORT_NODE_SMMU_V3 ||
> > -		    node->type == ACPI_IORT_NODE_PMCG) {
> > +		    node->type == ACPI_IORT_NODE_PMCG ||
> > +		    node->type == ACPI_IORT_NODE_RMR) {
> >   			*id_out = map->output_base;
> >   			return parent;
> >   		}
> > @@ -1660,6 +1674,91 @@ static void __init iort_enable_acs(struct
> acpi_iort_node *iort_node)
> >   #else
> >   static inline void iort_enable_acs(struct acpi_iort_node *iort_node) { }
> >   #endif
> > +static int iort_rmr_desc_valid(struct acpi_iort_rmr_desc *desc, u32 count)
> > +{
> > +	int i, j;
> > +
> > +	for (i = 0; i < count; i++) {
> > +		u64 end, start = desc[i].base_address, length = desc[i].length;
> > +
> > +		if (!IS_ALIGNED(start, SZ_64K) || !IS_ALIGNED(length, SZ_64K))
> > +			return -EINVAL;
> 
> We should certainly FW_BUG for this, but maybe it's OK to continue,
> since all we should need to do is round our iommu_resv_regions to at
> least PAGE_SIZE. That seems possibly better than ignoring them and
> having things potentially blow up later (especially if an end user
> exercises the system more thoroughly than the firmware developer tested
> it, which in at least one case I've seen may be "at all"...)

Ok. Will report FW_BUG but continue with a rounded addr/size.

> 
> > +		end = start + length - 1;
> > +
> > +		/* Check for address overlap */
> > +		for (j = i + 1; j < count; j++) {
> > +			u64 e_start = desc[j].base_address;
> > +			u64 e_end = e_start + desc[j].length - 1;
> > +
> > +			if (start <= e_end && end >= e_start)
> > +				return -EINVAL;
> 
> Similarly it's not *too* hard to trim overlaps; I guess it's really a
> question of whether we want to bother :/

I guess then, we can just report it and ignore,
   pr_warn(FW_BUG, "Found overlapping rmr desc @...., but ignoring...\n"

Thanks
Shameer

> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int __init iort_parse_rmr(struct acpi_iort_node *iort_node)
> > +{
> > +	struct acpi_iort_node *smmu;
> > +	struct iort_rmr_entry *e;
> > +	struct acpi_iort_rmr *rmr;
> > +	struct acpi_iort_rmr_desc *rmr_desc;
> > +	u32 map_count = iort_node->mapping_count;
> > +	u32  sid;
> 
> Nit: extra space.
> 
> > +	int i, ret = 0;
> > +
> > +	if (iort_node->type != ACPI_IORT_NODE_RMR)
> > +		return 0;
> > +
> > +	if (!iort_node->mapping_offset || map_count != 1) {
> 
> Beware that there was some discussion about allowing multiple SIDs to
> share an RMR descriptor, since there are potential use-cases which would
> otherwise lead to excessive duplication (e.g. an MSI doorbell carveout
> in a VM which would effectively apply to all possible PCI RIDs). I think
> the conclusion we reached was that disallowing that was fairly
> arbitrary, and could possibly be relaxed in future. It looks like the
> design of things here could grow to fit that fairly easily though, so I
> don't think it's a major concern.
> 
> Robin.
> 
> > +		pr_err(FW_BUG "Invalid ID mapping, skipping RMR node %p\n",
> > +		       iort_node);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Retrieve associated smmu and stream id */
> > +	smmu = iort_node_get_id(iort_node, &sid, 0);
> > +	if (!smmu) {
> > +		pr_err(FW_BUG "Invalid SMMU reference, skipping RMR
> node %p\n",
> > +		       iort_node);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Retrieve RMR data */
> > +	rmr = (struct acpi_iort_rmr *)iort_node->node_data;
> > +	if (!rmr->rmr_offset || !rmr->rmr_count) {
> > +		pr_err(FW_BUG "Invalid RMR descriptor array, skipping RMR
> node %p\n",
> > +		       iort_node);
> > +		return -EINVAL;
> > +	}
> > +
> > +	rmr_desc = ACPI_ADD_PTR(struct acpi_iort_rmr_desc, iort_node,
> > +				rmr->rmr_offset);
> > +
> > +	ret = iort_rmr_desc_valid(rmr_desc, rmr->rmr_count);
> > +	if (ret) {
> > +		pr_err(FW_BUG "Invalid RMR descriptor[%d] for node %p,
> skipping...\n",
> > +		       i, iort_node);
> > +		return ret;
> > +	}
> > +
> > +	for (i = 0; i < rmr->rmr_count; i++, rmr_desc++) {
> > +		e = kmalloc(sizeof(*e), GFP_KERNEL);
> > +		if (!e)
> > +			return -ENOMEM;
> > +
> > +		e->sid = sid;
> > +		e->smmu = smmu;
> > +		e->rmr_desc = rmr_desc;
> > +		e->flags = rmr->flags;
> > +
> > +		list_add_tail(&e->list, &iort_rmr_list);
> > +	}
> > +
> > +	return 0;
> > +}
> >
> >   static void __init iort_init_platform_devices(void)
> >   {
> > @@ -1689,6 +1788,9 @@ static void __init iort_init_platform_devices(void)
> >
> >   		iort_enable_acs(iort_node);
> >
> > +		if (iort_table->revision == 3)
> > +			iort_parse_rmr(iort_node);
> > +
> >   		ops = iort_get_dev_cfg(iort_node);
> >   		if (ops) {
> >   			fwnode = acpi_alloc_fwnode_static();
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 40+ messages in thread

* RE: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory regions
  2021-06-14 11:23   ` Robin Murphy
@ 2021-06-14 12:49     ` Shameerali Kolothum Thodi
  2021-06-29 17:34       ` Jon Nettleton
  0 siblings, 1 reply; 40+ messages in thread
From: Shameerali Kolothum Thodi @ 2021-06-14 12:49 UTC (permalink / raw)
  To: Robin Murphy, linux-arm-kernel, linux-acpi, iommu
  Cc: jon, Linuxarm, steven.price, Guohanjun (Hanjun Guo),
	yangyicong, Sami.Mujawar, wanghuiqiang



> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy@arm.com]
> Sent: 14 June 2021 12:23
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> linux-arm-kernel@lists.infradead.org; linux-acpi@vger.kernel.org;
> iommu@lists.linux-foundation.org
> Cc: jon@solid-run.com; Linuxarm <linuxarm@huawei.com>;
> steven.price@arm.com; Guohanjun (Hanjun Guo) <guohanjun@huawei.com>;
> yangyicong <yangyicong@huawei.com>; Sami.Mujawar@arm.com;
> wanghuiqiang <wanghuiqiang@huawei.com>
> Subject: Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory
> regions
> 
> On 2021-05-24 12:02, Shameer Kolothum wrote:
> > Add a helper function that retrieves RMR memory descriptors
> > associated with a given IOMMU. This will be used by IOMMU
> > drivers to setup necessary mappings.
> >
> > Now that we have this, invoke it from the generic helper
> > interface.
> >
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> > ---
> >   drivers/acpi/arm64/iort.c | 50
> +++++++++++++++++++++++++++++++++++++++
> >   drivers/iommu/dma-iommu.c |  4 ++++
> >   include/linux/acpi_iort.h |  7 ++++++
> >   3 files changed, 61 insertions(+)
> >
> > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > index fea1ffaedf3b..01917caf58de 100644
> > --- a/drivers/acpi/arm64/iort.c
> > +++ b/drivers/acpi/arm64/iort.c
> > @@ -12,6 +12,7 @@
> >
> >   #include <linux/acpi_iort.h>
> >   #include <linux/bitfield.h>
> > +#include <linux/dma-iommu.h>
> >   #include <linux/iommu.h>
> >   #include <linux/kernel.h>
> >   #include <linux/list.h>
> > @@ -837,6 +838,53 @@ static inline int iort_add_device_replay(struct
> device *dev)
> >   	return err;
> >   }
> >
> > +/**
> > + * iort_iommu_get_rmrs - Helper to retrieve RMR info associated with
> IOMMU
> > + * @iommu: fwnode for the IOMMU
> > + * @head: RMR list head to be populated
> > + *
> > + * Returns: 0 on success, <0 failure
> > + */
> > +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> > +			struct list_head *head)
> > +{
> > +	struct iort_rmr_entry *e;
> > +	struct acpi_iort_node *iommu;
> > +	int rmrs = 0;
> > +
> > +	iommu = iort_get_iort_node(iommu_fwnode);
> > +	if (!iommu || list_empty(&iort_rmr_list))
> > +		return -ENODEV;
> > +
> > +	list_for_each_entry(e, &iort_rmr_list, list) {
> > +		int prot = IOMMU_READ | IOMMU_WRITE | IOMMU_NOEXEC |
> IOMMU_MMIO;
> > +		struct iommu_resv_region *region;
> > +		enum iommu_resv_type type;
> > +		struct acpi_iort_rmr_desc *rmr_desc;
> > +
> > +		if (e->smmu != iommu)
> > +			continue;
> > +
> > +		rmr_desc = e->rmr_desc;
> > +		if (e->flags & IOMMU_RMR_REMAP_PERMITTED)
> > +			type = IOMMU_RESV_DIRECT_RELAXABLE;
> > +		else
> > +			type = IOMMU_RESV_DIRECT;
> 
> Wasn't the idea that we can do all this during the initial parsing, i.e.
> we don't even have iort_rmr_entry, we store them as iommu_resv_regions
> and simply kmemdup() or whatever at this point?


Hmm... Not yet. I removed struct iommu_rmr() from v4. But yes, it looks like
we can get rid of iort_rmr_entry as well. Will give it a go in next.

Thanks,
Shameer

> Robin.
> 
> > +
> > +		region = iommu_alloc_resv_region(rmr_desc->base_address,
> > +						 rmr_desc->length,
> > +						 prot, type);
> > +		if (region) {
> > +			region->fw_data.rmr.flags = e->flags;
> > +			region->fw_data.rmr.sid = e->sid;
> > +			list_add_tail(&region->list, head);
> > +			rmrs++;
> > +		}
> > +	}
> > +
> > +	return (rmrs == 0) ? -ENODEV : 0;
> > +}
> > +
> >   /**
> >    * iort_iommu_msi_get_resv_regions - Reserved region driver helper
> >    * @dev: Device from iommu_get_resv_regions()
> > @@ -1108,6 +1156,8 @@ int iort_iommu_msi_get_resv_regions(struct
> device *dev, struct list_head *head)
> >   const struct iommu_ops *iort_iommu_configure_id(struct device *dev,
> >   						const u32 *input_id)
> >   { return NULL; }
> > +int iort_iommu_get_rmrs(struct fwnode_handle *fwnode, struct list_head
> *head)
> > +{ return -ENODEV; }
> >   #endif
> >
> >   static int nc_dma_get_range(struct device *dev, u64 *size)
> > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> > index 229ec65d98be..f893d460cfa4 100644
> > --- a/drivers/iommu/dma-iommu.c
> > +++ b/drivers/iommu/dma-iommu.c
> > @@ -185,6 +185,9 @@ EXPORT_SYMBOL(iommu_put_dma_cookie);
> >   int iommu_dma_get_rmrs(struct fwnode_handle *iommu_fwnode,
> >   		       struct list_head *list)
> >   {
> > +	if (!is_of_node(iommu_fwnode))
> > +		return iort_iommu_get_rmrs(iommu_fwnode, list);
> > +
> >   	return -EINVAL;
> >   }
> >   EXPORT_SYMBOL(iommu_dma_get_rmrs);
> > @@ -200,6 +203,7 @@ EXPORT_SYMBOL(iommu_dma_get_rmrs);
> >   void iommu_dma_put_rmrs(struct fwnode_handle *iommu_fwnode,
> >   			struct list_head *list)
> >   {
> > +	generic_iommu_put_resv_regions(iommu_fwnode->dev, list);
> >   }
> >   EXPORT_SYMBOL(iommu_dma_put_rmrs);
> >
> > diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
> > index 1a12baa58e40..e8c45fa59531 100644
> > --- a/include/linux/acpi_iort.h
> > +++ b/include/linux/acpi_iort.h
> > @@ -39,6 +39,8 @@ const struct iommu_ops
> *iort_iommu_configure_id(struct device *dev,
> >   						const u32 *id_in);
> >   int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head
> *head);
> >   phys_addr_t acpi_iort_dma_get_max_cpu_address(void);
> > +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> > +			struct list_head *list);
> >   #else
> >   static inline void acpi_iort_init(void) { }
> >   static inline u32 iort_msi_map_id(struct device *dev, u32 id)
> > @@ -59,6 +61,11 @@ int iort_iommu_msi_get_resv_regions(struct device
> *dev, struct list_head *head)
> >
> >   static inline phys_addr_t acpi_iort_dma_get_max_cpu_address(void)
> >   { return PHYS_ADDR_MAX; }
> > +
> > +static inline
> > +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> > +			struct list_head *list)
> > +{ return -ENODEV; }
> >   #endif
> >
> >   #endif /* __ACPI_IORT_H__ */
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 40+ messages in thread

* RE: [PATCH v5 5/8] iommu/arm-smmu-v3: Add bypass flag to arm_smmu_write_strtab_ent()
  2021-06-14 10:23   ` Robin Murphy
@ 2021-06-14 12:51     ` Shameerali Kolothum Thodi
  0 siblings, 0 replies; 40+ messages in thread
From: Shameerali Kolothum Thodi @ 2021-06-14 12:51 UTC (permalink / raw)
  To: Robin Murphy, linux-arm-kernel, linux-acpi, iommu
  Cc: Linuxarm, lorenzo.pieralisi, joro, wanghuiqiang,
	Guohanjun (Hanjun Guo),
	steven.price, Sami.Mujawar, jon, eric.auger, yangyicong



> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy@arm.com]
> Sent: 14 June 2021 11:23
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> linux-arm-kernel@lists.infradead.org; linux-acpi@vger.kernel.org;
> iommu@lists.linux-foundation.org
> Cc: Linuxarm <linuxarm@huawei.com>; lorenzo.pieralisi@arm.com;
> joro@8bytes.org; wanghuiqiang <wanghuiqiang@huawei.com>; Guohanjun
> (Hanjun Guo) <guohanjun@huawei.com>; steven.price@arm.com;
> Sami.Mujawar@arm.com; jon@solid-run.com; eric.auger@redhat.com;
> yangyicong <yangyicong@huawei.com>
> Subject: Re: [PATCH v5 5/8] iommu/arm-smmu-v3: Add bypass flag
> to arm_smmu_write_strtab_ent()
> 
> On 2021-05-24 12:02, Shameer Kolothum wrote:
> > By default, disable_bypass is set and any dev without an iommu domain
> > installs STE with CFG_ABORT during arm_smmu_init_bypass_stes().
> Introduce
> > a "bypass" flag to arm_smmu_write_strtab_ent() so that we can force it to
> > install CFG_BYPASS STE for specific SIDs. This will be useful in follow
> > up patch to install bypass for IORT RMR SIDs.
> >
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> > ---
> >   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 8 ++++----
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > index 754bad6092c1..f9195b740f48 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -1174,7 +1174,7 @@ static void arm_smmu_sync_ste_for_sid(struct
> arm_smmu_device *smmu, u32 sid)
> >   }
> >
> >   static void arm_smmu_write_strtab_ent(struct arm_smmu_master
> *master, u32 sid,
> > -				      __le64 *dst)
> > +				      __le64 *dst, bool bypass)
> >   {
> >   	/*
> >   	 * This is hideously complicated, but we only really care about
> > @@ -1245,7 +1245,7 @@ static void arm_smmu_write_strtab_ent(struct
> arm_smmu_master *master, u32 sid,
> >
> >   	/* Bypass/fault */
> >   	if (!smmu_domain || !(s1_cfg || s2_cfg)) {
> > -		if (!smmu_domain && disable_bypass)
> > +		if (!smmu_domain && disable_bypass && !bypass)
> 
> Umm...
> 
> >   			val |= FIELD_PREP(STRTAB_STE_0_CFG,
> STRTAB_STE_0_CFG_ABORT);
> >   		else
> >   			val |= FIELD_PREP(STRTAB_STE_0_CFG,
> STRTAB_STE_0_CFG_BYPASS);
> > @@ -1320,7 +1320,7 @@ static void arm_smmu_init_bypass_stes(__le64
> *strtab, unsigned int nent)
> >   	unsigned int i;
> >
> >   	for (i = 0; i < nent; ++i) {
> > -		arm_smmu_write_strtab_ent(NULL, -1, strtab);
> > +		arm_smmu_write_strtab_ent(NULL, -1, strtab, false);
> 
> ...and in particular, an operation named "init_bypass_stes" passing
> bypass=false is just breaking my brain. Can we pull the logic for
> default bypass/fault out to here as part of the refactoring so that it
> actually makes sense?

Agree, it is confusing. Will add the default bypass/fault logic here.

Thanks,
Shameer

> 
> Robin.
> 
> >   		strtab += STRTAB_STE_DWORDS;
> >   	}
> >   }
> > @@ -2097,7 +2097,7 @@ static void arm_smmu_install_ste_for_dev(struct
> arm_smmu_master *master)
> >   		if (j < i)
> >   			continue;
> >
> > -		arm_smmu_write_strtab_ent(master, sid, step);
> > +		arm_smmu_write_strtab_ent(master, sid, step, false);
> >   	}
> >   }
> >
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 40+ messages in thread

* RE: [PATCH v5 7/8] iommu/arm-smmu: Get associated RMR info and install bypass SMR
  2021-06-14 10:06   ` Robin Murphy
@ 2021-06-14 16:51     ` Shameerali Kolothum Thodi
  2021-06-15  8:02       ` Jon Nettleton
  2021-06-29  7:03     ` Jon Nettleton
  1 sibling, 1 reply; 40+ messages in thread
From: Shameerali Kolothum Thodi @ 2021-06-14 16:51 UTC (permalink / raw)
  To: Robin Murphy, linux-arm-kernel, linux-acpi, iommu
  Cc: jon, Linuxarm, steven.price, Guohanjun (Hanjun Guo),
	yangyicong, Sami.Mujawar, wanghuiqiang



> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy@arm.com]
> Sent: 14 June 2021 11:06
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> linux-arm-kernel@lists.infradead.org; linux-acpi@vger.kernel.org;
> iommu@lists.linux-foundation.org
> Cc: jon@solid-run.com; Linuxarm <linuxarm@huawei.com>;
> steven.price@arm.com; Guohanjun (Hanjun Guo) <guohanjun@huawei.com>;
> yangyicong <yangyicong@huawei.com>; Sami.Mujawar@arm.com;
> wanghuiqiang <wanghuiqiang@huawei.com>
> Subject: Re: [PATCH v5 7/8] iommu/arm-smmu: Get associated RMR info and
> install bypass SMR
> 
> On 2021-05-24 12:02, Shameer Kolothum wrote:
> > From: Jon Nettleton <jon@solid-run.com>
> >
> > Check if there is any RMR info associated with the devices behind
> > the SMMU and if any, install bypass SMRs for them. This is to
> > keep any ongoing traffic associated with these devices alive
> > when we enable/reset SMMU during probe().
> >
> > Signed-off-by: Jon Nettleton <jon@solid-run.com>
> > Signed-off-by: Steven Price <steven.price@arm.com>
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> > ---
> >   drivers/iommu/arm/arm-smmu/arm-smmu.c | 65
> +++++++++++++++++++++++++++
> >   1 file changed, 65 insertions(+)
> >
> > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > index 6f72c4d208ca..56db3d3238fc 100644
> > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > @@ -2042,6 +2042,67 @@ err_reset_platform_ops: __maybe_unused;
> >   	return err;
> >   }
> >
> > +static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device
> *smmu)
> > +{
> > +	struct list_head rmr_list;
> > +	struct iommu_resv_region *e;
> > +	int i, cnt = 0;
> > +	u32 smr;
> > +	u32 reg;
> > +
> > +	INIT_LIST_HEAD(&rmr_list);
> > +	if (iommu_dma_get_rmrs(dev_fwnode(smmu->dev), &rmr_list))
> > +		return;
> > +
> > +	reg = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_sCR0);
> > +
> > +	if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg &
> ARM_SMMU_sCR0_CLIENTPD)) {
> > +		/*
> > +		 * SMMU is already enabled and disallowing bypass, so preserve
> > +		 * the existing SMRs
> > +		 */
> > +		for (i = 0; i < smmu->num_mapping_groups; i++) {
> > +			smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
> 
> To reiterate, just because a bootloader/crashed kernel/whatever may have
> left some configuration behind doesn't mean that it matters (e.g. what
> if these SMRs are pointing at translation contexts?). All we should be
> doing here is setting the relevant RMR accommodations in our "clean
> slate" software state before the reset routine applies it to the
> hardware, just like patch #5 does for SMMUv3.
> 
> Trying to safely reset an SMMU when we discover it with CLIENTPD=0 is
> really another issue entirely, and I'd say is beyond the scope of this
> series
> 
> > +			if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))
> > +				continue;
> 
> Note that that's not even necessarily correct (thanks to EXIDS).
> 
> > +			smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
> > +			smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK,
> smr);
> > +			smmu->smrs[i].valid = true;
> > +		}
> > +	}
> > +
> > +	list_for_each_entry(e, &rmr_list, list) {
> > +		u32 sid = e->fw_data.rmr.sid;
> > +
> > +		i = arm_smmu_find_sme(smmu, sid, ~0);
> > +		if (i < 0)
> > +			continue;
> > +		if (smmu->s2crs[i].count == 0) {
> > +			smmu->smrs[i].id = sid;
> > +			smmu->smrs[i].mask = ~0;
> 
> This is very wrong (as has now already been pointed out).
> 
> > +			smmu->smrs[i].valid = true;
> > +		}
> > +		smmu->s2crs[i].count++;
> > +		smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
> > +		smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
> > +		smmu->s2crs[i].cbndx = 0xff;
> 
> Nit: cbndx is left uninitialised for bypass/fault entries elsewhere, so
> there's little point touching it here.
> 
> > +
> > +		cnt++;
> > +	}
> > +
> > +	if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg &
> ARM_SMMU_sCR0_CLIENTPD)) {
> > +		/* Remove the valid bit for unused SMRs */
> > +		for (i = 0; i < smmu->num_mapping_groups; i++) {
> > +			if (smmu->s2crs[i].count == 0)
> > +				smmu->smrs[i].valid = false;
> > +		}
> 
> If this dance is only about avoiding stream match conflicts when trying
> to reprogram live SMRs, simply turning the SMMU off beforehand would be
> a lot simpler.

Hi Steve/Jon,

Since I don’t have access to an SMMUv2 setup, really appreciate if one of you
could please take a look at the above comments and provide me with a tested
code so that I can include it in the v6 that I am planning to send out soon. 

Thanks,
Shameer

> Robin.
> 
> > +	}
> > +
> > +	dev_notice(smmu->dev, "\tpreserved %d boot mapping%s\n", cnt,
> > +		   cnt == 1 ? "" : "s");
> > +	iommu_dma_put_rmrs(dev_fwnode(smmu->dev), &rmr_list);
> > +}
> > +
> >   static int arm_smmu_device_probe(struct platform_device *pdev)
> >   {
> >   	struct resource *res;
> > @@ -2168,6 +2229,10 @@ static int arm_smmu_device_probe(struct
> platform_device *pdev)
> >   	}
> >
> >   	platform_set_drvdata(pdev, smmu);
> > +
> > +	/* Check for RMRs and install bypass SMRs if any */
> > +	arm_smmu_rmr_install_bypass_smr(smmu);
> > +
> >   	arm_smmu_device_reset(smmu);
> >   	arm_smmu_test_smr_masks(smmu);
> >
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 7/8] iommu/arm-smmu: Get associated RMR info and install bypass SMR
  2021-06-14 16:51     ` Shameerali Kolothum Thodi
@ 2021-06-15  8:02       ` Jon Nettleton
  0 siblings, 0 replies; 40+ messages in thread
From: Jon Nettleton @ 2021-06-15  8:02 UTC (permalink / raw)
  To: Shameerali Kolothum Thodi
  Cc: Robin Murphy, linux-arm-kernel, linux-acpi, iommu, Linuxarm,
	steven.price, Guohanjun (Hanjun Guo),
	yangyicong, Sami.Mujawar, wanghuiqiang

On Mon, Jun 14, 2021 at 6:51 PM Shameerali Kolothum Thodi
<shameerali.kolothum.thodi@huawei.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Robin Murphy [mailto:robin.murphy@arm.com]
> > Sent: 14 June 2021 11:06
> > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> > linux-arm-kernel@lists.infradead.org; linux-acpi@vger.kernel.org;
> > iommu@lists.linux-foundation.org
> > Cc: jon@solid-run.com; Linuxarm <linuxarm@huawei.com>;
> > steven.price@arm.com; Guohanjun (Hanjun Guo) <guohanjun@huawei.com>;
> > yangyicong <yangyicong@huawei.com>; Sami.Mujawar@arm.com;
> > wanghuiqiang <wanghuiqiang@huawei.com>
> > Subject: Re: [PATCH v5 7/8] iommu/arm-smmu: Get associated RMR info and
> > install bypass SMR
> >
> > On 2021-05-24 12:02, Shameer Kolothum wrote:
> > > From: Jon Nettleton <jon@solid-run.com>
> > >
> > > Check if there is any RMR info associated with the devices behind
> > > the SMMU and if any, install bypass SMRs for them. This is to
> > > keep any ongoing traffic associated with these devices alive
> > > when we enable/reset SMMU during probe().
> > >
> > > Signed-off-by: Jon Nettleton <jon@solid-run.com>
> > > Signed-off-by: Steven Price <steven.price@arm.com>
> > > Signed-off-by: Shameer Kolothum
> > <shameerali.kolothum.thodi@huawei.com>
> > > ---
> > >   drivers/iommu/arm/arm-smmu/arm-smmu.c | 65
> > +++++++++++++++++++++++++++
> > >   1 file changed, 65 insertions(+)
> > >
> > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > > index 6f72c4d208ca..56db3d3238fc 100644
> > > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > > @@ -2042,6 +2042,67 @@ err_reset_platform_ops: __maybe_unused;
> > >     return err;
> > >   }
> > >
> > > +static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device
> > *smmu)
> > > +{
> > > +   struct list_head rmr_list;
> > > +   struct iommu_resv_region *e;
> > > +   int i, cnt = 0;
> > > +   u32 smr;
> > > +   u32 reg;
> > > +
> > > +   INIT_LIST_HEAD(&rmr_list);
> > > +   if (iommu_dma_get_rmrs(dev_fwnode(smmu->dev), &rmr_list))
> > > +           return;
> > > +
> > > +   reg = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_sCR0);
> > > +
> > > +   if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg &
> > ARM_SMMU_sCR0_CLIENTPD)) {
> > > +           /*
> > > +            * SMMU is already enabled and disallowing bypass, so preserve
> > > +            * the existing SMRs
> > > +            */
> > > +           for (i = 0; i < smmu->num_mapping_groups; i++) {
> > > +                   smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
> >
> > To reiterate, just because a bootloader/crashed kernel/whatever may have
> > left some configuration behind doesn't mean that it matters (e.g. what
> > if these SMRs are pointing at translation contexts?). All we should be
> > doing here is setting the relevant RMR accommodations in our "clean
> > slate" software state before the reset routine applies it to the
> > hardware, just like patch #5 does for SMMUv3.
> >
> > Trying to safely reset an SMMU when we discover it with CLIENTPD=0 is
> > really another issue entirely, and I'd say is beyond the scope of this
> > series
> >
> > > +                   if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))
> > > +                           continue;
> >
> > Note that that's not even necessarily correct (thanks to EXIDS).
> >
> > > +                   smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
> > > +                   smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK,
> > smr);
> > > +                   smmu->smrs[i].valid = true;
> > > +           }
> > > +   }
> > > +
> > > +   list_for_each_entry(e, &rmr_list, list) {
> > > +           u32 sid = e->fw_data.rmr.sid;
> > > +
> > > +           i = arm_smmu_find_sme(smmu, sid, ~0);
> > > +           if (i < 0)
> > > +                   continue;
> > > +           if (smmu->s2crs[i].count == 0) {
> > > +                   smmu->smrs[i].id = sid;
> > > +                   smmu->smrs[i].mask = ~0;
> >
> > This is very wrong (as has now already been pointed out).
> >
> > > +                   smmu->smrs[i].valid = true;
> > > +           }
> > > +           smmu->s2crs[i].count++;
> > > +           smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
> > > +           smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
> > > +           smmu->s2crs[i].cbndx = 0xff;
> >
> > Nit: cbndx is left uninitialised for bypass/fault entries elsewhere, so
> > there's little point touching it here.
> >
> > > +
> > > +           cnt++;
> > > +   }
> > > +
> > > +   if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg &
> > ARM_SMMU_sCR0_CLIENTPD)) {
> > > +           /* Remove the valid bit for unused SMRs */
> > > +           for (i = 0; i < smmu->num_mapping_groups; i++) {
> > > +                   if (smmu->s2crs[i].count == 0)
> > > +                           smmu->smrs[i].valid = false;
> > > +           }
> >
> > If this dance is only about avoiding stream match conflicts when trying
> > to reprogram live SMRs, simply turning the SMMU off beforehand would be
> > a lot simpler.
>
> Hi Steve/Jon,
>
> Since I don’t have access to an SMMUv2 setup, really appreciate if one of you
> could please take a look at the above comments and provide me with a tested
> code so that I can include it in the v6 that I am planning to send out soon.

Will do.  Thanks.
Jon

>
> Thanks,
> Shameer
>
> > Robin.
> >
> > > +   }
> > > +
> > > +   dev_notice(smmu->dev, "\tpreserved %d boot mapping%s\n", cnt,
> > > +              cnt == 1 ? "" : "s");
> > > +   iommu_dma_put_rmrs(dev_fwnode(smmu->dev), &rmr_list);
> > > +}
> > > +
> > >   static int arm_smmu_device_probe(struct platform_device *pdev)
> > >   {
> > >     struct resource *res;
> > > @@ -2168,6 +2229,10 @@ static int arm_smmu_device_probe(struct
> > platform_device *pdev)
> > >     }
> > >
> > >     platform_set_drvdata(pdev, smmu);
> > > +
> > > +   /* Check for RMRs and install bypass SMRs if any */
> > > +   arm_smmu_rmr_install_bypass_smr(smmu);
> > > +
> > >     arm_smmu_device_reset(smmu);
> > >     arm_smmu_test_smr_masks(smmu);
> > >
> > >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 7/8] iommu/arm-smmu: Get associated RMR info and install bypass SMR
  2021-06-14 10:06   ` Robin Murphy
  2021-06-14 16:51     ` Shameerali Kolothum Thodi
@ 2021-06-29  7:03     ` Jon Nettleton
  2021-06-29 13:22       ` Robin Murphy
  1 sibling, 1 reply; 40+ messages in thread
From: Jon Nettleton @ 2021-06-29  7:03 UTC (permalink / raw)
  To: Robin Murphy
  Cc: Shameer Kolothum, linux-arm-kernel, ACPI Devel Maling List,
	iommu, Linuxarm, Steven Price, Hanjun Guo, yangyicong,
	Sami.Mujawar, wanghuiqiang

On Mon, Jun 14, 2021 at 12:06 PM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2021-05-24 12:02, Shameer Kolothum wrote:
> > From: Jon Nettleton <jon@solid-run.com>
> >
> > Check if there is any RMR info associated with the devices behind
> > the SMMU and if any, install bypass SMRs for them. This is to
> > keep any ongoing traffic associated with these devices alive
> > when we enable/reset SMMU during probe().
> >
> > Signed-off-by: Jon Nettleton <jon@solid-run.com>
> > Signed-off-by: Steven Price <steven.price@arm.com>
> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > ---
> >   drivers/iommu/arm/arm-smmu/arm-smmu.c | 65 +++++++++++++++++++++++++++
> >   1 file changed, 65 insertions(+)
> >
> > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > index 6f72c4d208ca..56db3d3238fc 100644
> > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > @@ -2042,6 +2042,67 @@ err_reset_platform_ops: __maybe_unused;
> >       return err;
> >   }
> >
> > +static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)
> > +{
> > +     struct list_head rmr_list;
> > +     struct iommu_resv_region *e;
> > +     int i, cnt = 0;
> > +     u32 smr;
> > +     u32 reg;
> > +
> > +     INIT_LIST_HEAD(&rmr_list);
> > +     if (iommu_dma_get_rmrs(dev_fwnode(smmu->dev), &rmr_list))
> > +             return;
> > +
> > +     reg = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_sCR0);
> > +
> > +     if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg & ARM_SMMU_sCR0_CLIENTPD)) {
> > +             /*
> > +              * SMMU is already enabled and disallowing bypass, so preserve
> > +              * the existing SMRs
> > +              */
> > +             for (i = 0; i < smmu->num_mapping_groups; i++) {
> > +                     smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
>
> To reiterate, just because a bootloader/crashed kernel/whatever may have
> left some configuration behind doesn't mean that it matters (e.g. what
> if these SMRs are pointing at translation contexts?). All we should be
> doing here is setting the relevant RMR accommodations in our "clean
> slate" software state before the reset routine applies it to the
> hardware, just like patch #5 does for SMMUv3.
>
> Trying to safely reset an SMMU when we discover it with CLIENTPD=0 is
> really another issue entirely, and I'd say is beyond the scope of this
> series
>
> > +                     if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))
> > +                             continue;
>
> Note that that's not even necessarily correct (thanks to EXIDS).
>
> > +                     smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
> > +                     smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK, smr);
> > +                     smmu->smrs[i].valid = true;
> > +             }
> > +     }
> > +
> > +     list_for_each_entry(e, &rmr_list, list) {
> > +             u32 sid = e->fw_data.rmr.sid;
> > +
> > +             i = arm_smmu_find_sme(smmu, sid, ~0);
> > +             if (i < 0)
> > +                     continue;
> > +             if (smmu->s2crs[i].count == 0) {
> > +                     smmu->smrs[i].id = sid;
> > +                     smmu->smrs[i].mask = ~0;
>
> This is very wrong (as has now already been pointed out).
>
> > +                     smmu->smrs[i].valid = true;
> > +             }
> > +             smmu->s2crs[i].count++;
> > +             smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
> > +             smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
> > +             smmu->s2crs[i].cbndx = 0xff;
>
> Nit: cbndx is left uninitialised for bypass/fault entries elsewhere, so
> there's little point touching it here.
>
> > +
> > +             cnt++;
> > +     }
> > +
> > +     if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg & ARM_SMMU_sCR0_CLIENTPD)) {
> > +             /* Remove the valid bit for unused SMRs */
> > +             for (i = 0; i < smmu->num_mapping_groups; i++) {
> > +                     if (smmu->s2crs[i].count == 0)
> > +                             smmu->smrs[i].valid = false;
> > +             }
>
> If this dance is only about avoiding stream match conflicts when trying
> to reprogram live SMRs, simply turning the SMMU off beforehand would be
> a lot simpler.

Robin,

I am not sure what you mean here, and maybe Steve wants to jump in and
help clarify.

My understanding is that "dance" is required for regions that need to
continue to work
throughout the boot process.  I think the example I have seen the most
is for GOP drivers that
use system memory rather than dedicated VRAM.

-Jon

>
> Robin.
>
> > +     }
> > +
> > +     dev_notice(smmu->dev, "\tpreserved %d boot mapping%s\n", cnt,
> > +                cnt == 1 ? "" : "s");
> > +     iommu_dma_put_rmrs(dev_fwnode(smmu->dev), &rmr_list);
> > +}
> > +
> >   static int arm_smmu_device_probe(struct platform_device *pdev)
> >   {
> >       struct resource *res;
> > @@ -2168,6 +2229,10 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
> >       }
> >
> >       platform_set_drvdata(pdev, smmu);
> > +
> > +     /* Check for RMRs and install bypass SMRs if any */
> > +     arm_smmu_rmr_install_bypass_smr(smmu);
> > +
> >       arm_smmu_device_reset(smmu);
> >       arm_smmu_test_smr_masks(smmu);
> >
> >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 7/8] iommu/arm-smmu: Get associated RMR info and install bypass SMR
  2021-06-29  7:03     ` Jon Nettleton
@ 2021-06-29 13:22       ` Robin Murphy
  2021-06-29 16:25         ` Jon Nettleton
  0 siblings, 1 reply; 40+ messages in thread
From: Robin Murphy @ 2021-06-29 13:22 UTC (permalink / raw)
  To: Jon Nettleton
  Cc: Shameer Kolothum, linux-arm-kernel, ACPI Devel Maling List,
	iommu, Linuxarm, Steven Price, Hanjun Guo, yangyicong,
	Sami.Mujawar, wanghuiqiang

On 2021-06-29 08:03, Jon Nettleton wrote:
> On Mon, Jun 14, 2021 at 12:06 PM Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> On 2021-05-24 12:02, Shameer Kolothum wrote:
>>> From: Jon Nettleton <jon@solid-run.com>
>>>
>>> Check if there is any RMR info associated with the devices behind
>>> the SMMU and if any, install bypass SMRs for them. This is to
>>> keep any ongoing traffic associated with these devices alive
>>> when we enable/reset SMMU during probe().
>>>
>>> Signed-off-by: Jon Nettleton <jon@solid-run.com>
>>> Signed-off-by: Steven Price <steven.price@arm.com>
>>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>>> ---
>>>    drivers/iommu/arm/arm-smmu/arm-smmu.c | 65 +++++++++++++++++++++++++++
>>>    1 file changed, 65 insertions(+)
>>>
>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
>>> index 6f72c4d208ca..56db3d3238fc 100644
>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
>>> @@ -2042,6 +2042,67 @@ err_reset_platform_ops: __maybe_unused;
>>>        return err;
>>>    }
>>>
>>> +static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)
>>> +{
>>> +     struct list_head rmr_list;
>>> +     struct iommu_resv_region *e;
>>> +     int i, cnt = 0;
>>> +     u32 smr;
>>> +     u32 reg;
>>> +
>>> +     INIT_LIST_HEAD(&rmr_list);
>>> +     if (iommu_dma_get_rmrs(dev_fwnode(smmu->dev), &rmr_list))
>>> +             return;
>>> +
>>> +     reg = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_sCR0);
>>> +
>>> +     if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg & ARM_SMMU_sCR0_CLIENTPD)) {
>>> +             /*
>>> +              * SMMU is already enabled and disallowing bypass, so preserve
>>> +              * the existing SMRs
>>> +              */
>>> +             for (i = 0; i < smmu->num_mapping_groups; i++) {
>>> +                     smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
>>
>> To reiterate, just because a bootloader/crashed kernel/whatever may have
>> left some configuration behind doesn't mean that it matters (e.g. what
>> if these SMRs are pointing at translation contexts?). All we should be
>> doing here is setting the relevant RMR accommodations in our "clean
>> slate" software state before the reset routine applies it to the
>> hardware, just like patch #5 does for SMMUv3.
>>
>> Trying to safely reset an SMMU when we discover it with CLIENTPD=0 is
>> really another issue entirely, and I'd say is beyond the scope of this
>> series
>>
>>> +                     if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))
>>> +                             continue;
>>
>> Note that that's not even necessarily correct (thanks to EXIDS).
>>
>>> +                     smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
>>> +                     smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK, smr);
>>> +                     smmu->smrs[i].valid = true;
>>> +             }
>>> +     }
>>> +
>>> +     list_for_each_entry(e, &rmr_list, list) {
>>> +             u32 sid = e->fw_data.rmr.sid;
>>> +
>>> +             i = arm_smmu_find_sme(smmu, sid, ~0);
>>> +             if (i < 0)
>>> +                     continue;
>>> +             if (smmu->s2crs[i].count == 0) {
>>> +                     smmu->smrs[i].id = sid;
>>> +                     smmu->smrs[i].mask = ~0;
>>
>> This is very wrong (as has now already been pointed out).
>>
>>> +                     smmu->smrs[i].valid = true;
>>> +             }
>>> +             smmu->s2crs[i].count++;
>>> +             smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
>>> +             smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
>>> +             smmu->s2crs[i].cbndx = 0xff;
>>
>> Nit: cbndx is left uninitialised for bypass/fault entries elsewhere, so
>> there's little point touching it here.
>>
>>> +
>>> +             cnt++;
>>> +     }
>>> +
>>> +     if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg & ARM_SMMU_sCR0_CLIENTPD)) {
>>> +             /* Remove the valid bit for unused SMRs */
>>> +             for (i = 0; i < smmu->num_mapping_groups; i++) {
>>> +                     if (smmu->s2crs[i].count == 0)
>>> +                             smmu->smrs[i].valid = false;
>>> +             }
>>
>> If this dance is only about avoiding stream match conflicts when trying
>> to reprogram live SMRs, simply turning the SMMU off beforehand would be
>> a lot simpler.
> 
> Robin,
> 
> I am not sure what you mean here, and maybe Steve wants to jump in and
> help clarify.
> 
> My understanding is that "dance" is required for regions that need to
> continue to work
> throughout the boot process.  I think the example I have seen the most
> is for GOP drivers that
> use system memory rather than dedicated VRAM.

All that is required is to install bypass SMEs for the relevant streams 
before enabling the SMMU. That much is achieved by the 
list_for_each_entry(e, &rmr_list, list) loop setting up initial state 
followed by arm_smmu_device_reset(). The "dance" I'm referring to is the 
additional reading out of (possibly nonsense) SMR state beforehand to 
pre-bias the SMR allocator, then trying to clean up the remnants afterwards.

If we're going to pretend to be robust against finding the SMMU already 
enabled *with* live traffic ongoing, there's frankly an awful lot more 
care we'd need to take beyond here (and for some aspects there's still 
no right answer). If on the other hand we're implicitly assuming that 
any boot-time enabled SMRs exactly match the RMR configuration anyway, 
then simply disabling the SMMU until we enable it again in the reset 
routine still preserves the necessary bypass behaviour for RMR streams 
while sidestepping any issues related to reprogramming live SMMU state.

Robin.

>>> +     }
>>> +
>>> +     dev_notice(smmu->dev, "\tpreserved %d boot mapping%s\n", cnt,
>>> +                cnt == 1 ? "" : "s");
>>> +     iommu_dma_put_rmrs(dev_fwnode(smmu->dev), &rmr_list);
>>> +}
>>> +
>>>    static int arm_smmu_device_probe(struct platform_device *pdev)
>>>    {
>>>        struct resource *res;
>>> @@ -2168,6 +2229,10 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>>>        }
>>>
>>>        platform_set_drvdata(pdev, smmu);
>>> +
>>> +     /* Check for RMRs and install bypass SMRs if any */
>>> +     arm_smmu_rmr_install_bypass_smr(smmu);
>>> +
>>>        arm_smmu_device_reset(smmu);
>>>        arm_smmu_test_smr_masks(smmu);
>>>
>>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 7/8] iommu/arm-smmu: Get associated RMR info and install bypass SMR
  2021-06-29 13:22       ` Robin Murphy
@ 2021-06-29 16:25         ` Jon Nettleton
  2021-06-30  8:50           ` Shameerali Kolothum Thodi
  0 siblings, 1 reply; 40+ messages in thread
From: Jon Nettleton @ 2021-06-29 16:25 UTC (permalink / raw)
  To: Robin Murphy
  Cc: Shameer Kolothum, linux-arm-kernel, ACPI Devel Maling List,
	iommu, Linuxarm, Steven Price, Hanjun Guo, yangyicong,
	Sami.Mujawar, wanghuiqiang

On Tue, Jun 29, 2021 at 3:23 PM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2021-06-29 08:03, Jon Nettleton wrote:
> > On Mon, Jun 14, 2021 at 12:06 PM Robin Murphy <robin.murphy@arm.com> wrote:
> >>
> >> On 2021-05-24 12:02, Shameer Kolothum wrote:
> >>> From: Jon Nettleton <jon@solid-run.com>
> >>>
> >>> Check if there is any RMR info associated with the devices behind
> >>> the SMMU and if any, install bypass SMRs for them. This is to
> >>> keep any ongoing traffic associated with these devices alive
> >>> when we enable/reset SMMU during probe().
> >>>
> >>> Signed-off-by: Jon Nettleton <jon@solid-run.com>
> >>> Signed-off-by: Steven Price <steven.price@arm.com>
> >>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> >>> ---
> >>>    drivers/iommu/arm/arm-smmu/arm-smmu.c | 65 +++++++++++++++++++++++++++
> >>>    1 file changed, 65 insertions(+)
> >>>
> >>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> >>> index 6f72c4d208ca..56db3d3238fc 100644
> >>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> >>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> >>> @@ -2042,6 +2042,67 @@ err_reset_platform_ops: __maybe_unused;
> >>>        return err;
> >>>    }
> >>>
> >>> +static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)
> >>> +{
> >>> +     struct list_head rmr_list;
> >>> +     struct iommu_resv_region *e;
> >>> +     int i, cnt = 0;
> >>> +     u32 smr;
> >>> +     u32 reg;
> >>> +
> >>> +     INIT_LIST_HEAD(&rmr_list);
> >>> +     if (iommu_dma_get_rmrs(dev_fwnode(smmu->dev), &rmr_list))
> >>> +             return;
> >>> +
> >>> +     reg = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_sCR0);
> >>> +
> >>> +     if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg & ARM_SMMU_sCR0_CLIENTPD)) {
> >>> +             /*
> >>> +              * SMMU is already enabled and disallowing bypass, so preserve
> >>> +              * the existing SMRs
> >>> +              */
> >>> +             for (i = 0; i < smmu->num_mapping_groups; i++) {
> >>> +                     smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
> >>
> >> To reiterate, just because a bootloader/crashed kernel/whatever may have
> >> left some configuration behind doesn't mean that it matters (e.g. what
> >> if these SMRs are pointing at translation contexts?). All we should be
> >> doing here is setting the relevant RMR accommodations in our "clean
> >> slate" software state before the reset routine applies it to the
> >> hardware, just like patch #5 does for SMMUv3.
> >>
> >> Trying to safely reset an SMMU when we discover it with CLIENTPD=0 is
> >> really another issue entirely, and I'd say is beyond the scope of this
> >> series
> >>
> >>> +                     if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))
> >>> +                             continue;
> >>
> >> Note that that's not even necessarily correct (thanks to EXIDS).
> >>
> >>> +                     smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
> >>> +                     smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK, smr);
> >>> +                     smmu->smrs[i].valid = true;
> >>> +             }
> >>> +     }
> >>> +
> >>> +     list_for_each_entry(e, &rmr_list, list) {
> >>> +             u32 sid = e->fw_data.rmr.sid;
> >>> +
> >>> +             i = arm_smmu_find_sme(smmu, sid, ~0);
> >>> +             if (i < 0)
> >>> +                     continue;
> >>> +             if (smmu->s2crs[i].count == 0) {
> >>> +                     smmu->smrs[i].id = sid;
> >>> +                     smmu->smrs[i].mask = ~0;
> >>
> >> This is very wrong (as has now already been pointed out).
> >>
> >>> +                     smmu->smrs[i].valid = true;
> >>> +             }
> >>> +             smmu->s2crs[i].count++;
> >>> +             smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
> >>> +             smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
> >>> +             smmu->s2crs[i].cbndx = 0xff;
> >>
> >> Nit: cbndx is left uninitialised for bypass/fault entries elsewhere, so
> >> there's little point touching it here.
> >>
> >>> +
> >>> +             cnt++;
> >>> +     }
> >>> +
> >>> +     if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg & ARM_SMMU_sCR0_CLIENTPD)) {
> >>> +             /* Remove the valid bit for unused SMRs */
> >>> +             for (i = 0; i < smmu->num_mapping_groups; i++) {
> >>> +                     if (smmu->s2crs[i].count == 0)
> >>> +                             smmu->smrs[i].valid = false;
> >>> +             }
> >>
> >> If this dance is only about avoiding stream match conflicts when trying
> >> to reprogram live SMRs, simply turning the SMMU off beforehand would be
> >> a lot simpler.
> >
> > Robin,
> >
> > I am not sure what you mean here, and maybe Steve wants to jump in and
> > help clarify.
> >
> > My understanding is that "dance" is required for regions that need to
> > continue to work
> > throughout the boot process.  I think the example I have seen the most
> > is for GOP drivers that
> > use system memory rather than dedicated VRAM.
>
> All that is required is to install bypass SMEs for the relevant streams
> before enabling the SMMU. That much is achieved by the
> list_for_each_entry(e, &rmr_list, list) loop setting up initial state
> followed by arm_smmu_device_reset(). The "dance" I'm referring to is the
> additional reading out of (possibly nonsense) SMR state beforehand to
> pre-bias the SMR allocator, then trying to clean up the remnants afterwards.
>
> If we're going to pretend to be robust against finding the SMMU already
> enabled *with* live traffic ongoing, there's frankly an awful lot more
> care we'd need to take beyond here (and for some aspects there's still
> no right answer). If on the other hand we're implicitly assuming that
> any boot-time enabled SMRs exactly match the RMR configuration anyway,
> then simply disabling the SMMU until we enable it again in the reset
> routine still preserves the necessary bypass behaviour for RMR streams
> while sidestepping any issues related to reprogramming live SMMU state.
>
> Robin.
>
> >>> +     }
> >>> +
> >>> +     dev_notice(smmu->dev, "\tpreserved %d boot mapping%s\n", cnt,
> >>> +                cnt == 1 ? "" : "s");
> >>> +     iommu_dma_put_rmrs(dev_fwnode(smmu->dev), &rmr_list);
> >>> +}
> >>> +
> >>>    static int arm_smmu_device_probe(struct platform_device *pdev)
> >>>    {
> >>>        struct resource *res;
> >>> @@ -2168,6 +2229,10 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
> >>>        }
> >>>
> >>>        platform_set_drvdata(pdev, smmu);
> >>> +
> >>> +     /* Check for RMRs and install bypass SMRs if any */
> >>> +     arm_smmu_rmr_install_bypass_smr(smmu);
> >>> +
> >>>        arm_smmu_device_reset(smmu);
> >>>        arm_smmu_test_smr_masks(smmu);
> >>>
> >>>

Shameer,

Sorry for the delays.  Here is a diff of the changes that should
address the issues pointed out by Robin,
I have tested that this works as expected on my HoneyComb LX2160A.

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c
b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index ab7b9db77625..a358bd326d0b 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -2068,29 +2068,21 @@ static void
arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)
        struct list_head rmr_list;
        struct iommu_resv_region *e;
        int i, cnt = 0;
-       u32 smr;
        u32 reg;

        INIT_LIST_HEAD(&rmr_list);
        if (iommu_dma_get_rmrs(dev_fwnode(smmu->dev), &rmr_list))
                return;

-       reg = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_sCR0);
+       /* Rather than trying to look at existing mappings that
+        * are setup by the firmware and then invalidate the ones
+        * that do no have matching RMR entries, just disable the
+        * SMMU until it gets enabled again in the reset routine.
+        */

-       if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg & ARM_SMMU_sCR0_CLIENTPD)) {
-               /*
-                * SMMU is already enabled and disallowing bypass, so preserve
-                * the existing SMRs
-                */
-               for (i = 0; i < smmu->num_mapping_groups; i++) {
-                       smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
-                       if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))
-                               continue;
-                       smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
-                       smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK, smr);
-                       smmu->smrs[i].valid = true;
-               }
-       }
+       reg = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_sCR0);
+       reg &= ~ARM_SMMU_sCR0_CLIENTPD;
+       arm_smmu_gr0_write(smmu, ARM_SMMU_GR0_sCR0, reg);

        list_for_each_entry(e, &rmr_list, list) {
                u32 sid = e->fw_data.rmr.sid;
@@ -2100,25 +2092,16 @@ static void
arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)
                        continue;
                if (smmu->s2crs[i].count == 0) {
                        smmu->smrs[i].id = sid;
-                       smmu->smrs[i].mask = ~0;
+                       smmu->smrs[i].mask = 0;
                        smmu->smrs[i].valid = true;
                }
                smmu->s2crs[i].count++;
                smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
                smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
-               smmu->s2crs[i].cbndx = 0xff;

                cnt++;
        }

-       if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg & ARM_SMMU_sCR0_CLIENTPD)) {
-               /* Remove the valid bit for unused SMRs */
-               for (i = 0; i < smmu->num_mapping_groups; i++) {
-                       if (smmu->s2crs[i].count == 0)
-                               smmu->smrs[i].valid = false;
-               }
-       }
-
        dev_notice(smmu->dev, "\tpreserved %d boot mapping%s\n", cnt,
                   cnt == 1 ? "" : "s");
        iommu_dma_put_rmrs(dev_fwnode(smmu->dev), &rmr_list);

Please include that in your next patch series.  Let me know if you
want me to send you the patch direct
off the list.

Thanks,
Jon

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory regions
  2021-06-14 12:49     ` Shameerali Kolothum Thodi
@ 2021-06-29 17:34       ` Jon Nettleton
  2021-07-04  7:38         ` Jon Nettleton
  0 siblings, 1 reply; 40+ messages in thread
From: Jon Nettleton @ 2021-06-29 17:34 UTC (permalink / raw)
  To: Shameerali Kolothum Thodi
  Cc: Robin Murphy, linux-arm-kernel, linux-acpi, iommu, Linuxarm,
	steven.price, Guohanjun (Hanjun Guo),
	yangyicong, Sami.Mujawar, wanghuiqiang

On Mon, Jun 14, 2021 at 2:49 PM Shameerali Kolothum Thodi
<shameerali.kolothum.thodi@huawei.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Robin Murphy [mailto:robin.murphy@arm.com]
> > Sent: 14 June 2021 12:23
> > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> > linux-arm-kernel@lists.infradead.org; linux-acpi@vger.kernel.org;
> > iommu@lists.linux-foundation.org
> > Cc: jon@solid-run.com; Linuxarm <linuxarm@huawei.com>;
> > steven.price@arm.com; Guohanjun (Hanjun Guo) <guohanjun@huawei.com>;
> > yangyicong <yangyicong@huawei.com>; Sami.Mujawar@arm.com;
> > wanghuiqiang <wanghuiqiang@huawei.com>
> > Subject: Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory
> > regions
> >
> > On 2021-05-24 12:02, Shameer Kolothum wrote:
> > > Add a helper function that retrieves RMR memory descriptors
> > > associated with a given IOMMU. This will be used by IOMMU
> > > drivers to setup necessary mappings.
> > >
> > > Now that we have this, invoke it from the generic helper
> > > interface.
> > >
> > > Signed-off-by: Shameer Kolothum
> > <shameerali.kolothum.thodi@huawei.com>
> > > ---
> > >   drivers/acpi/arm64/iort.c | 50
> > +++++++++++++++++++++++++++++++++++++++
> > >   drivers/iommu/dma-iommu.c |  4 ++++
> > >   include/linux/acpi_iort.h |  7 ++++++
> > >   3 files changed, 61 insertions(+)
> > >
> > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > > index fea1ffaedf3b..01917caf58de 100644
> > > --- a/drivers/acpi/arm64/iort.c
> > > +++ b/drivers/acpi/arm64/iort.c
> > > @@ -12,6 +12,7 @@
> > >
> > >   #include <linux/acpi_iort.h>
> > >   #include <linux/bitfield.h>
> > > +#include <linux/dma-iommu.h>
> > >   #include <linux/iommu.h>
> > >   #include <linux/kernel.h>
> > >   #include <linux/list.h>
> > > @@ -837,6 +838,53 @@ static inline int iort_add_device_replay(struct
> > device *dev)
> > >     return err;
> > >   }
> > >
> > > +/**
> > > + * iort_iommu_get_rmrs - Helper to retrieve RMR info associated with
> > IOMMU
> > > + * @iommu: fwnode for the IOMMU
> > > + * @head: RMR list head to be populated
> > > + *
> > > + * Returns: 0 on success, <0 failure
> > > + */
> > > +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> > > +                   struct list_head *head)
> > > +{
> > > +   struct iort_rmr_entry *e;
> > > +   struct acpi_iort_node *iommu;
> > > +   int rmrs = 0;
> > > +
> > > +   iommu = iort_get_iort_node(iommu_fwnode);
> > > +   if (!iommu || list_empty(&iort_rmr_list))
> > > +           return -ENODEV;
> > > +
> > > +   list_for_each_entry(e, &iort_rmr_list, list) {
> > > +           int prot = IOMMU_READ | IOMMU_WRITE | IOMMU_NOEXEC |
> > IOMMU_MMIO;
> > > +           struct iommu_resv_region *region;
> > > +           enum iommu_resv_type type;
> > > +           struct acpi_iort_rmr_desc *rmr_desc;
> > > +
> > > +           if (e->smmu != iommu)
> > > +                   continue;
> > > +
> > > +           rmr_desc = e->rmr_desc;
> > > +           if (e->flags & IOMMU_RMR_REMAP_PERMITTED)
> > > +                   type = IOMMU_RESV_DIRECT_RELAXABLE;
> > > +           else
> > > +                   type = IOMMU_RESV_DIRECT;
> >

I have been looking at this a bit more and looking at the history of
RMR_REMAP_PERMITTED.  Based on the history I have found it
seems to me like this would be the better options for prot.

@@ -840,7 +840,7 @@ int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
                return -ENODEV;

        list_for_each_entry(e, &iort_rmr_list, list) {
-               int prot = IOMMU_READ | IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
+               int prot = IOMMU_READ | IOMMU_WRITE;
                struct iommu_resv_region *region;
                enum iommu_resv_type type;
                struct acpi_iort_rmr_desc *rmr_desc;
@@ -849,10 +849,13 @@ int iort_iommu_get_rmrs(struct fwnode_handle
*iommu_fwnode,
                        continue;

                rmr_desc = e->rmr_desc;
-               if (e->flags & IOMMU_RMR_REMAP_PERMITTED)
+               if (e->flags & IOMMU_RMR_REMAP_PERMITTED) {
                        type = IOMMU_RESV_DIRECT_RELAXABLE;
-               else
+                       prot |= IOMMU_CACHE;
+               } else {
                        type = IOMMU_RESV_DIRECT;
+                       prot |= IOMMU_MMIO;
+               }

                region = iommu_alloc_resv_region(rmr_desc->base_address,
                                                 rmr_desc->length,

any input on this?  My reasoning is that IOMMU_RESV_DIRECT is specifically
referenced for things like MSI doorbell and in all the examples needs
IOMMU_MMIO, while REMAP_PERMITTED seems to be used for allocated
system memory that is then used for device specific reserved regions which
commits say would be like a GPU or USB device.

-Jon

> > Wasn't the idea that we can do all this during the initial parsing, i.e.
> > we don't even have iort_rmr_entry, we store them as iommu_resv_regions
> > and simply kmemdup() or whatever at this point?
>
>
> Hmm... Not yet. I removed struct iommu_rmr() from v4. But yes, it looks like
> we can get rid of iort_rmr_entry as well. Will give it a go in next.
>
> Thanks,
> Shameer
>
> > Robin.
> >
> > > +
> > > +           region = iommu_alloc_resv_region(rmr_desc->base_address,
> > > +                                            rmr_desc->length,
> > > +                                            prot, type);
> > > +           if (region) {
> > > +                   region->fw_data.rmr.flags = e->flags;
> > > +                   region->fw_data.rmr.sid = e->sid;
> > > +                   list_add_tail(&region->list, head);
> > > +                   rmrs++;
> > > +           }
> > > +   }
> > > +
> > > +   return (rmrs == 0) ? -ENODEV : 0;
> > > +}
> > > +
> > >   /**
> > >    * iort_iommu_msi_get_resv_regions - Reserved region driver helper
> > >    * @dev: Device from iommu_get_resv_regions()
> > > @@ -1108,6 +1156,8 @@ int iort_iommu_msi_get_resv_regions(struct
> > device *dev, struct list_head *head)
> > >   const struct iommu_ops *iort_iommu_configure_id(struct device *dev,
> > >                                             const u32 *input_id)
> > >   { return NULL; }
> > > +int iort_iommu_get_rmrs(struct fwnode_handle *fwnode, struct list_head
> > *head)
> > > +{ return -ENODEV; }
> > >   #endif
> > >
> > >   static int nc_dma_get_range(struct device *dev, u64 *size)
> > > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> > > index 229ec65d98be..f893d460cfa4 100644
> > > --- a/drivers/iommu/dma-iommu.c
> > > +++ b/drivers/iommu/dma-iommu.c
> > > @@ -185,6 +185,9 @@ EXPORT_SYMBOL(iommu_put_dma_cookie);
> > >   int iommu_dma_get_rmrs(struct fwnode_handle *iommu_fwnode,
> > >                    struct list_head *list)
> > >   {
> > > +   if (!is_of_node(iommu_fwnode))
> > > +           return iort_iommu_get_rmrs(iommu_fwnode, list);
> > > +
> > >     return -EINVAL;
> > >   }
> > >   EXPORT_SYMBOL(iommu_dma_get_rmrs);
> > > @@ -200,6 +203,7 @@ EXPORT_SYMBOL(iommu_dma_get_rmrs);
> > >   void iommu_dma_put_rmrs(struct fwnode_handle *iommu_fwnode,
> > >                     struct list_head *list)
> > >   {
> > > +   generic_iommu_put_resv_regions(iommu_fwnode->dev, list);
> > >   }
> > >   EXPORT_SYMBOL(iommu_dma_put_rmrs);
> > >
> > > diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
> > > index 1a12baa58e40..e8c45fa59531 100644
> > > --- a/include/linux/acpi_iort.h
> > > +++ b/include/linux/acpi_iort.h
> > > @@ -39,6 +39,8 @@ const struct iommu_ops
> > *iort_iommu_configure_id(struct device *dev,
> > >                                             const u32 *id_in);
> > >   int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head
> > *head);
> > >   phys_addr_t acpi_iort_dma_get_max_cpu_address(void);
> > > +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> > > +                   struct list_head *list);
> > >   #else
> > >   static inline void acpi_iort_init(void) { }
> > >   static inline u32 iort_msi_map_id(struct device *dev, u32 id)
> > > @@ -59,6 +61,11 @@ int iort_iommu_msi_get_resv_regions(struct device
> > *dev, struct list_head *head)
> > >
> > >   static inline phys_addr_t acpi_iort_dma_get_max_cpu_address(void)
> > >   { return PHYS_ADDR_MAX; }
> > > +
> > > +static inline
> > > +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> > > +                   struct list_head *list)
> > > +{ return -ENODEV; }
> > >   #endif
> > >
> > >   #endif /* __ACPI_IORT_H__ */
> > >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 40+ messages in thread

* RE: [PATCH v5 7/8] iommu/arm-smmu: Get associated RMR info and install bypass SMR
  2021-06-29 16:25         ` Jon Nettleton
@ 2021-06-30  8:50           ` Shameerali Kolothum Thodi
  0 siblings, 0 replies; 40+ messages in thread
From: Shameerali Kolothum Thodi @ 2021-06-30  8:50 UTC (permalink / raw)
  To: Jon Nettleton, Robin Murphy
  Cc: linux-arm-kernel, ACPI Devel Maling List, iommu, Linuxarm,
	Steven Price, Guohanjun (Hanjun Guo),
	yangyicong, Sami.Mujawar, wanghuiqiang



> -----Original Message-----
> From: Jon Nettleton [mailto:jon@solid-run.com]
> Sent: 29 June 2021 17:26
> To: Robin Murphy <robin.murphy@arm.com>
> Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> linux-arm-kernel <linux-arm-kernel@lists.infradead.org>; ACPI Devel Maling
> List <linux-acpi@vger.kernel.org>; iommu@lists.linux-foundation.org; Linuxarm
> <linuxarm@huawei.com>; Steven Price <steven.price@arm.com>; Guohanjun
> (Hanjun Guo) <guohanjun@huawei.com>; yangyicong
> <yangyicong@huawei.com>; Sami.Mujawar@arm.com; wanghuiqiang
> <wanghuiqiang@huawei.com>
> Subject: Re: [PATCH v5 7/8] iommu/arm-smmu: Get associated RMR info and
> install bypass SMR
> 

[...]
 
> Shameer,
> 
> Sorry for the delays.  Here is a diff of the changes that should
> address the issues pointed out by Robin,
> I have tested that this works as expected on my HoneyComb LX2160A.

Ok. Thanks for that.

> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index ab7b9db77625..a358bd326d0b 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -2068,29 +2068,21 @@ static void
> arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)
>         struct list_head rmr_list;
>         struct iommu_resv_region *e;
>         int i, cnt = 0;
> -       u32 smr;
>         u32 reg;
> 
>         INIT_LIST_HEAD(&rmr_list);
>         if (iommu_dma_get_rmrs(dev_fwnode(smmu->dev), &rmr_list))
>                 return;
> 
> -       reg = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_sCR0);
> +       /* Rather than trying to look at existing mappings that
> +        * are setup by the firmware and then invalidate the ones
> +        * that do no have matching RMR entries, just disable the
> +        * SMMU until it gets enabled again in the reset routine.
> +        */
> 
> -       if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg &
> ARM_SMMU_sCR0_CLIENTPD)) {
> -               /*
> -                * SMMU is already enabled and disallowing bypass, so
> preserve
> -                * the existing SMRs
> -                */
> -               for (i = 0; i < smmu->num_mapping_groups; i++) {
> -                       smr = arm_smmu_gr0_read(smmu,
> ARM_SMMU_GR0_SMR(i));
> -                       if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))
> -                               continue;
> -                       smmu->smrs[i].id =
> FIELD_GET(ARM_SMMU_SMR_ID, smr);
> -                       smmu->smrs[i].mask =
> FIELD_GET(ARM_SMMU_SMR_MASK, smr);
> -                       smmu->smrs[i].valid = true;
> -               }
> -       }
> +       reg = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_sCR0);
> +       reg &= ~ARM_SMMU_sCR0_CLIENTPD;
> +       arm_smmu_gr0_write(smmu, ARM_SMMU_GR0_sCR0, reg);
> 
>         list_for_each_entry(e, &rmr_list, list) {
>                 u32 sid = e->fw_data.rmr.sid;
> @@ -2100,25 +2092,16 @@ static void
> arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)
>                         continue;
>                 if (smmu->s2crs[i].count == 0) {
>                         smmu->smrs[i].id = sid;
> -                       smmu->smrs[i].mask = ~0;
> +                       smmu->smrs[i].mask = 0;
>                         smmu->smrs[i].valid = true;
>                 }
>                 smmu->s2crs[i].count++;
>                 smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
>                 smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
> -               smmu->s2crs[i].cbndx = 0xff;
> 
>                 cnt++;
>         }
> 
> -       if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg &
> ARM_SMMU_sCR0_CLIENTPD)) {
> -               /* Remove the valid bit for unused SMRs */
> -               for (i = 0; i < smmu->num_mapping_groups; i++) {
> -                       if (smmu->s2crs[i].count == 0)
> -                               smmu->smrs[i].valid = false;
> -               }
> -       }
> -
>         dev_notice(smmu->dev, "\tpreserved %d boot mapping%s\n", cnt,
>                    cnt == 1 ? "" : "s");
>         iommu_dma_put_rmrs(dev_fwnode(smmu->dev), &rmr_list);
> 
> Please include that in your next patch series.  Let me know if you
> want me to send you the patch direct
> off the list.

No problem, I will take this in next.

Thanks,
Shameer
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory regions
  2021-06-29 17:34       ` Jon Nettleton
@ 2021-07-04  7:38         ` Jon Nettleton
  2021-07-05  9:10           ` Shameerali Kolothum Thodi
  0 siblings, 1 reply; 40+ messages in thread
From: Jon Nettleton @ 2021-07-04  7:38 UTC (permalink / raw)
  To: Shameerali Kolothum Thodi
  Cc: Robin Murphy, linux-arm-kernel, linux-acpi, iommu, Linuxarm,
	steven.price, Guohanjun (Hanjun Guo),
	yangyicong, Sami.Mujawar, wanghuiqiang

On Tue, Jun 29, 2021 at 7:34 PM Jon Nettleton <jon@solid-run.com> wrote:
>
> On Mon, Jun 14, 2021 at 2:49 PM Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Robin Murphy [mailto:robin.murphy@arm.com]
> > > Sent: 14 June 2021 12:23
> > > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> > > linux-arm-kernel@lists.infradead.org; linux-acpi@vger.kernel.org;
> > > iommu@lists.linux-foundation.org
> > > Cc: jon@solid-run.com; Linuxarm <linuxarm@huawei.com>;
> > > steven.price@arm.com; Guohanjun (Hanjun Guo) <guohanjun@huawei.com>;
> > > yangyicong <yangyicong@huawei.com>; Sami.Mujawar@arm.com;
> > > wanghuiqiang <wanghuiqiang@huawei.com>
> > > Subject: Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory
> > > regions
> > >
> > > On 2021-05-24 12:02, Shameer Kolothum wrote:
> > > > Add a helper function that retrieves RMR memory descriptors
> > > > associated with a given IOMMU. This will be used by IOMMU
> > > > drivers to setup necessary mappings.
> > > >
> > > > Now that we have this, invoke it from the generic helper
> > > > interface.
> > > >
> > > > Signed-off-by: Shameer Kolothum
> > > <shameerali.kolothum.thodi@huawei.com>
> > > > ---
> > > >   drivers/acpi/arm64/iort.c | 50
> > > +++++++++++++++++++++++++++++++++++++++
> > > >   drivers/iommu/dma-iommu.c |  4 ++++
> > > >   include/linux/acpi_iort.h |  7 ++++++
> > > >   3 files changed, 61 insertions(+)
> > > >
> > > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > > > index fea1ffaedf3b..01917caf58de 100644
> > > > --- a/drivers/acpi/arm64/iort.c
> > > > +++ b/drivers/acpi/arm64/iort.c
> > > > @@ -12,6 +12,7 @@
> > > >
> > > >   #include <linux/acpi_iort.h>
> > > >   #include <linux/bitfield.h>
> > > > +#include <linux/dma-iommu.h>
> > > >   #include <linux/iommu.h>
> > > >   #include <linux/kernel.h>
> > > >   #include <linux/list.h>
> > > > @@ -837,6 +838,53 @@ static inline int iort_add_device_replay(struct
> > > device *dev)
> > > >     return err;
> > > >   }
> > > >
> > > > +/**
> > > > + * iort_iommu_get_rmrs - Helper to retrieve RMR info associated with
> > > IOMMU
> > > > + * @iommu: fwnode for the IOMMU
> > > > + * @head: RMR list head to be populated
> > > > + *
> > > > + * Returns: 0 on success, <0 failure
> > > > + */
> > > > +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> > > > +                   struct list_head *head)
> > > > +{
> > > > +   struct iort_rmr_entry *e;
> > > > +   struct acpi_iort_node *iommu;
> > > > +   int rmrs = 0;
> > > > +
> > > > +   iommu = iort_get_iort_node(iommu_fwnode);
> > > > +   if (!iommu || list_empty(&iort_rmr_list))
> > > > +           return -ENODEV;
> > > > +
> > > > +   list_for_each_entry(e, &iort_rmr_list, list) {
> > > > +           int prot = IOMMU_READ | IOMMU_WRITE | IOMMU_NOEXEC |
> > > IOMMU_MMIO;
> > > > +           struct iommu_resv_region *region;
> > > > +           enum iommu_resv_type type;
> > > > +           struct acpi_iort_rmr_desc *rmr_desc;
> > > > +
> > > > +           if (e->smmu != iommu)
> > > > +                   continue;
> > > > +
> > > > +           rmr_desc = e->rmr_desc;
> > > > +           if (e->flags & IOMMU_RMR_REMAP_PERMITTED)
> > > > +                   type = IOMMU_RESV_DIRECT_RELAXABLE;
> > > > +           else
> > > > +                   type = IOMMU_RESV_DIRECT;
> > >
>
> I have been looking at this a bit more and looking at the history of
> RMR_REMAP_PERMITTED.  Based on the history I have found it
> seems to me like this would be the better options for prot.
>
> @@ -840,7 +840,7 @@ int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
>                 return -ENODEV;
>
>         list_for_each_entry(e, &iort_rmr_list, list) {
> -               int prot = IOMMU_READ | IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> +               int prot = IOMMU_READ | IOMMU_WRITE;
>                 struct iommu_resv_region *region;
>                 enum iommu_resv_type type;
>                 struct acpi_iort_rmr_desc *rmr_desc;
> @@ -849,10 +849,13 @@ int iort_iommu_get_rmrs(struct fwnode_handle
> *iommu_fwnode,
>                         continue;
>
>                 rmr_desc = e->rmr_desc;
> -               if (e->flags & IOMMU_RMR_REMAP_PERMITTED)
> +               if (e->flags & IOMMU_RMR_REMAP_PERMITTED) {
>                         type = IOMMU_RESV_DIRECT_RELAXABLE;
> -               else
> +                       prot |= IOMMU_CACHE;
> +               } else {
>                         type = IOMMU_RESV_DIRECT;
> +                       prot |= IOMMU_MMIO;
> +               }
>
>                 region = iommu_alloc_resv_region(rmr_desc->base_address,
>                                                  rmr_desc->length,
>
> any input on this?  My reasoning is that IOMMU_RESV_DIRECT is specifically
> referenced for things like MSI doorbell and in all the examples needs
> IOMMU_MMIO, while REMAP_PERMITTED seems to be used for allocated
> system memory that is then used for device specific reserved regions which
> commits say would be like a GPU or USB device.
>
> -Jon
>
> > > Wasn't the idea that we can do all this during the initial parsing, i.e.
> > > we don't even have iort_rmr_entry, we store them as iommu_resv_regions
> > > and simply kmemdup() or whatever at this point?
> >
> >
> > Hmm... Not yet. I removed struct iommu_rmr() from v4. But yes, it looks like
> > we can get rid of iort_rmr_entry as well. Will give it a go in next.
> >
> > Thanks,
> > Shameer
> >
> > > Robin.
> > >
> > > > +
> > > > +           region = iommu_alloc_resv_region(rmr_desc->base_address,
> > > > +                                            rmr_desc->length,
> > > > +                                            prot, type);
> > > > +           if (region) {
> > > > +                   region->fw_data.rmr.flags = e->flags;
> > > > +                   region->fw_data.rmr.sid = e->sid;
> > > > +                   list_add_tail(&region->list, head);
> > > > +                   rmrs++;
> > > > +           }
> > > > +   }
> > > > +
> > > > +   return (rmrs == 0) ? -ENODEV : 0;
> > > > +}
> > > > +
> > > >   /**
> > > >    * iort_iommu_msi_get_resv_regions - Reserved region driver helper
> > > >    * @dev: Device from iommu_get_resv_regions()
> > > > @@ -1108,6 +1156,8 @@ int iort_iommu_msi_get_resv_regions(struct
> > > device *dev, struct list_head *head)
> > > >   const struct iommu_ops *iort_iommu_configure_id(struct device *dev,
> > > >                                             const u32 *input_id)
> > > >   { return NULL; }
> > > > +int iort_iommu_get_rmrs(struct fwnode_handle *fwnode, struct list_head
> > > *head)
> > > > +{ return -ENODEV; }
> > > >   #endif
> > > >
> > > >   static int nc_dma_get_range(struct device *dev, u64 *size)
> > > > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> > > > index 229ec65d98be..f893d460cfa4 100644
> > > > --- a/drivers/iommu/dma-iommu.c
> > > > +++ b/drivers/iommu/dma-iommu.c
> > > > @@ -185,6 +185,9 @@ EXPORT_SYMBOL(iommu_put_dma_cookie);
> > > >   int iommu_dma_get_rmrs(struct fwnode_handle *iommu_fwnode,
> > > >                    struct list_head *list)
> > > >   {
> > > > +   if (!is_of_node(iommu_fwnode))
> > > > +           return iort_iommu_get_rmrs(iommu_fwnode, list);
> > > > +
> > > >     return -EINVAL;
> > > >   }
> > > >   EXPORT_SYMBOL(iommu_dma_get_rmrs);
> > > > @@ -200,6 +203,7 @@ EXPORT_SYMBOL(iommu_dma_get_rmrs);
> > > >   void iommu_dma_put_rmrs(struct fwnode_handle *iommu_fwnode,
> > > >                     struct list_head *list)
> > > >   {
> > > > +   generic_iommu_put_resv_regions(iommu_fwnode->dev, list);
> > > >   }
> > > >   EXPORT_SYMBOL(iommu_dma_put_rmrs);
> > > >
> > > > diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
> > > > index 1a12baa58e40..e8c45fa59531 100644
> > > > --- a/include/linux/acpi_iort.h
> > > > +++ b/include/linux/acpi_iort.h
> > > > @@ -39,6 +39,8 @@ const struct iommu_ops
> > > *iort_iommu_configure_id(struct device *dev,
> > > >                                             const u32 *id_in);
> > > >   int iort_iommu_msi_get_resv_regions(struct device *dev, struct list_head
> > > *head);
> > > >   phys_addr_t acpi_iort_dma_get_max_cpu_address(void);
> > > > +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> > > > +                   struct list_head *list);
> > > >   #else
> > > >   static inline void acpi_iort_init(void) { }
> > > >   static inline u32 iort_msi_map_id(struct device *dev, u32 id)
> > > > @@ -59,6 +61,11 @@ int iort_iommu_msi_get_resv_regions(struct device
> > > *dev, struct list_head *head)
> > > >
> > > >   static inline phys_addr_t acpi_iort_dma_get_max_cpu_address(void)
> > > >   { return PHYS_ADDR_MAX; }
> > > > +
> > > > +static inline
> > > > +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> > > > +                   struct list_head *list)
> > > > +{ return -ENODEV; }
> > > >   #endif
> > > >
> > > >   #endif /* __ACPI_IORT_H__ */
> > > >

Ping.  Any comments on this?

-Jon

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 40+ messages in thread

* RE: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory regions
  2021-07-04  7:38         ` Jon Nettleton
@ 2021-07-05  9:10           ` Shameerali Kolothum Thodi
  0 siblings, 0 replies; 40+ messages in thread
From: Shameerali Kolothum Thodi @ 2021-07-05  9:10 UTC (permalink / raw)
  To: Jon Nettleton, Robin Murphy
  Cc: linux-arm-kernel, linux-acpi, iommu, Linuxarm, steven.price,
	Guohanjun (Hanjun Guo),
	yangyicong, Sami.Mujawar, wanghuiqiang



> -----Original Message-----
> From: Jon Nettleton [mailto:jon@solid-run.com]
> Sent: 04 July 2021 08:39
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: Robin Murphy <robin.murphy@arm.com>;
> linux-arm-kernel@lists.infradead.org; linux-acpi@vger.kernel.org;
> iommu@lists.linux-foundation.org; Linuxarm <linuxarm@huawei.com>;
> steven.price@arm.com; Guohanjun (Hanjun Guo) <guohanjun@huawei.com>;
> yangyicong <yangyicong@huawei.com>; Sami.Mujawar@arm.com;
> wanghuiqiang <wanghuiqiang@huawei.com>
> Subject: Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory
> regions
> 
> On Tue, Jun 29, 2021 at 7:34 PM Jon Nettleton <jon@solid-run.com> wrote:
> >
> > On Mon, Jun 14, 2021 at 2:49 PM Shameerali Kolothum Thodi
> > <shameerali.kolothum.thodi@huawei.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Robin Murphy [mailto:robin.murphy@arm.com]
> > > > Sent: 14 June 2021 12:23
> > > > To: Shameerali Kolothum Thodi
> > > > <shameerali.kolothum.thodi@huawei.com>;
> > > > linux-arm-kernel@lists.infradead.org; linux-acpi@vger.kernel.org;
> > > > iommu@lists.linux-foundation.org
> > > > Cc: jon@solid-run.com; Linuxarm <linuxarm@huawei.com>;
> > > > steven.price@arm.com; Guohanjun (Hanjun Guo)
> > > > <guohanjun@huawei.com>; yangyicong <yangyicong@huawei.com>;
> > > > Sami.Mujawar@arm.com; wanghuiqiang <wanghuiqiang@huawei.com>
> > > > Subject: Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve
> > > > RMR memory regions
> > > >
> > > > On 2021-05-24 12:02, Shameer Kolothum wrote:
> > > > > Add a helper function that retrieves RMR memory descriptors
> > > > > associated with a given IOMMU. This will be used by IOMMU
> > > > > drivers to setup necessary mappings.
> > > > >
> > > > > Now that we have this, invoke it from the generic helper
> > > > > interface.
> > > > >
> > > > > Signed-off-by: Shameer Kolothum
> > > > <shameerali.kolothum.thodi@huawei.com>
> > > > > ---
> > > > >   drivers/acpi/arm64/iort.c | 50
> > > > +++++++++++++++++++++++++++++++++++++++
> > > > >   drivers/iommu/dma-iommu.c |  4 ++++
> > > > >   include/linux/acpi_iort.h |  7 ++++++
> > > > >   3 files changed, 61 insertions(+)
> > > > >
> > > > > diff --git a/drivers/acpi/arm64/iort.c
> > > > > b/drivers/acpi/arm64/iort.c index fea1ffaedf3b..01917caf58de
> > > > > 100644
> > > > > --- a/drivers/acpi/arm64/iort.c
> > > > > +++ b/drivers/acpi/arm64/iort.c
> > > > > @@ -12,6 +12,7 @@
> > > > >
> > > > >   #include <linux/acpi_iort.h>
> > > > >   #include <linux/bitfield.h>
> > > > > +#include <linux/dma-iommu.h>
> > > > >   #include <linux/iommu.h>
> > > > >   #include <linux/kernel.h>
> > > > >   #include <linux/list.h>
> > > > > @@ -837,6 +838,53 @@ static inline int
> > > > > iort_add_device_replay(struct
> > > > device *dev)
> > > > >     return err;
> > > > >   }
> > > > >
> > > > > +/**
> > > > > + * iort_iommu_get_rmrs - Helper to retrieve RMR info associated
> > > > > +with
> > > > IOMMU
> > > > > + * @iommu: fwnode for the IOMMU
> > > > > + * @head: RMR list head to be populated
> > > > > + *
> > > > > + * Returns: 0 on success, <0 failure  */ int
> > > > > +iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> > > > > +                   struct list_head *head) {
> > > > > +   struct iort_rmr_entry *e;
> > > > > +   struct acpi_iort_node *iommu;
> > > > > +   int rmrs = 0;
> > > > > +
> > > > > +   iommu = iort_get_iort_node(iommu_fwnode);
> > > > > +   if (!iommu || list_empty(&iort_rmr_list))
> > > > > +           return -ENODEV;
> > > > > +
> > > > > +   list_for_each_entry(e, &iort_rmr_list, list) {
> > > > > +           int prot = IOMMU_READ | IOMMU_WRITE |
> IOMMU_NOEXEC |
> > > > IOMMU_MMIO;
> > > > > +           struct iommu_resv_region *region;
> > > > > +           enum iommu_resv_type type;
> > > > > +           struct acpi_iort_rmr_desc *rmr_desc;
> > > > > +
> > > > > +           if (e->smmu != iommu)
> > > > > +                   continue;
> > > > > +
> > > > > +           rmr_desc = e->rmr_desc;
> > > > > +           if (e->flags & IOMMU_RMR_REMAP_PERMITTED)
> > > > > +                   type = IOMMU_RESV_DIRECT_RELAXABLE;
> > > > > +           else
> > > > > +                   type = IOMMU_RESV_DIRECT;
> > > >
> >
> > I have been looking at this a bit more and looking at the history of
> > RMR_REMAP_PERMITTED.  Based on the history I have found it seems to
> me
> > like this would be the better options for prot.
> >
> > @@ -840,7 +840,7 @@ int iort_iommu_get_rmrs(struct fwnode_handle
> *iommu_fwnode,
> >                 return -ENODEV;
> >
> >         list_for_each_entry(e, &iort_rmr_list, list) {
> > -               int prot = IOMMU_READ | IOMMU_WRITE |
> IOMMU_NOEXEC | IOMMU_MMIO;
> > +               int prot = IOMMU_READ | IOMMU_WRITE;
> >                 struct iommu_resv_region *region;
> >                 enum iommu_resv_type type;
> >                 struct acpi_iort_rmr_desc *rmr_desc; @@ -849,10
> > +849,13 @@ int iort_iommu_get_rmrs(struct fwnode_handle
> *iommu_fwnode,
> >                         continue;
> >
> >                 rmr_desc = e->rmr_desc;
> > -               if (e->flags & IOMMU_RMR_REMAP_PERMITTED)
> > +               if (e->flags & IOMMU_RMR_REMAP_PERMITTED) {
> >                         type = IOMMU_RESV_DIRECT_RELAXABLE;
> > -               else
> > +                       prot |= IOMMU_CACHE;
> > +               } else {
> >                         type = IOMMU_RESV_DIRECT;
> > +                       prot |= IOMMU_MMIO;
> > +               }
> >
> >                 region =
> iommu_alloc_resv_region(rmr_desc->base_address,
> >                                                  rmr_desc->length,
> >
> > any input on this?  My reasoning is that IOMMU_RESV_DIRECT is
> > specifically referenced for things like MSI doorbell and in all the
> > examples needs IOMMU_MMIO, while REMAP_PERMITTED seems to be used
> for
> > allocated system memory that is then used for device specific reserved
> > regions which commits say would be like a GPU or USB device.

I am Ok to make those changes but not sure we can make the above assumptions
based on the way it is currently used. 

Hi Robin,

Any thoughts?

Thanks,
Shameer

> >
> > -Jon
> >
> > > > Wasn't the idea that we can do all this during the initial parsing, i.e.
> > > > we don't even have iort_rmr_entry, we store them as
> > > > iommu_resv_regions and simply kmemdup() or whatever at this point?
> > >
> > >
> > > Hmm... Not yet. I removed struct iommu_rmr() from v4. But yes, it
> > > looks like we can get rid of iort_rmr_entry as well. Will give it a go in next.
> > >
> > > Thanks,
> > > Shameer
> > >
> > > > Robin.
> > > >
> > > > > +
> > > > > +           region =
> iommu_alloc_resv_region(rmr_desc->base_address,
> > > > > +                                            rmr_desc->length,
> > > > > +                                            prot, type);
> > > > > +           if (region) {
> > > > > +                   region->fw_data.rmr.flags = e->flags;
> > > > > +                   region->fw_data.rmr.sid = e->sid;
> > > > > +                   list_add_tail(&region->list, head);
> > > > > +                   rmrs++;
> > > > > +           }
> > > > > +   }
> > > > > +
> > > > > +   return (rmrs == 0) ? -ENODEV : 0; }
> > > > > +
> > > > >   /**
> > > > >    * iort_iommu_msi_get_resv_regions - Reserved region driver helper
> > > > >    * @dev: Device from iommu_get_resv_regions() @@ -1108,6
> > > > > +1156,8 @@ int iort_iommu_msi_get_resv_regions(struct
> > > > device *dev, struct list_head *head)
> > > > >   const struct iommu_ops *iort_iommu_configure_id(struct device
> *dev,
> > > > >                                             const u32
> *input_id)
> > > > >   { return NULL; }
> > > > > +int iort_iommu_get_rmrs(struct fwnode_handle *fwnode, struct
> > > > > +list_head
> > > > *head)
> > > > > +{ return -ENODEV; }
> > > > >   #endif
> > > > >
> > > > >   static int nc_dma_get_range(struct device *dev, u64 *size)
> > > > > diff --git a/drivers/iommu/dma-iommu.c
> > > > > b/drivers/iommu/dma-iommu.c index 229ec65d98be..f893d460cfa4
> > > > > 100644
> > > > > --- a/drivers/iommu/dma-iommu.c
> > > > > +++ b/drivers/iommu/dma-iommu.c
> > > > > @@ -185,6 +185,9 @@ EXPORT_SYMBOL(iommu_put_dma_cookie);
> > > > >   int iommu_dma_get_rmrs(struct fwnode_handle *iommu_fwnode,
> > > > >                    struct list_head *list)
> > > > >   {
> > > > > +   if (!is_of_node(iommu_fwnode))
> > > > > +           return iort_iommu_get_rmrs(iommu_fwnode, list);
> > > > > +
> > > > >     return -EINVAL;
> > > > >   }
> > > > >   EXPORT_SYMBOL(iommu_dma_get_rmrs);
> > > > > @@ -200,6 +203,7 @@ EXPORT_SYMBOL(iommu_dma_get_rmrs);
> > > > >   void iommu_dma_put_rmrs(struct fwnode_handle *iommu_fwnode,
> > > > >                     struct list_head *list)
> > > > >   {
> > > > > +   generic_iommu_put_resv_regions(iommu_fwnode->dev, list);
> > > > >   }
> > > > >   EXPORT_SYMBOL(iommu_dma_put_rmrs);
> > > > >
> > > > > diff --git a/include/linux/acpi_iort.h
> > > > > b/include/linux/acpi_iort.h index 1a12baa58e40..e8c45fa59531
> > > > > 100644
> > > > > --- a/include/linux/acpi_iort.h
> > > > > +++ b/include/linux/acpi_iort.h
> > > > > @@ -39,6 +39,8 @@ const struct iommu_ops
> > > > *iort_iommu_configure_id(struct device *dev,
> > > > >                                             const u32 *id_in);
> > > > >   int iort_iommu_msi_get_resv_regions(struct device *dev, struct
> > > > > list_head
> > > > *head);
> > > > >   phys_addr_t acpi_iort_dma_get_max_cpu_address(void);
> > > > > +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> > > > > +                   struct list_head *list);
> > > > >   #else
> > > > >   static inline void acpi_iort_init(void) { }
> > > > >   static inline u32 iort_msi_map_id(struct device *dev, u32 id)
> > > > > @@ -59,6 +61,11 @@ int iort_iommu_msi_get_resv_regions(struct
> > > > > device
> > > > *dev, struct list_head *head)
> > > > >
> > > > >   static inline phys_addr_t acpi_iort_dma_get_max_cpu_address(void)
> > > > >   { return PHYS_ADDR_MAX; }
> > > > > +
> > > > > +static inline
> > > > > +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> > > > > +                   struct list_head *list) { return -ENODEV; }
> > > > >   #endif
> > > > >
> > > > >   #endif /* __ACPI_IORT_H__ */
> > > > >
> 
> Ping.  Any comments on this?
> 
> -Jon
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 40+ messages in thread

end of thread, other threads:[~2021-07-05  9:12 UTC | newest]

Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-24 11:02 [PATCH v5 0/8] ACPI/IORT: Support for IORT RMR node Shameer Kolothum
2021-05-24 11:02 ` [PATCH v5 1/8] ACPI/IORT: Add support for RMR node parsing Shameer Kolothum
2021-06-14 11:14   ` Robin Murphy
2021-06-14 12:37     ` Shameerali Kolothum Thodi
2021-05-24 11:02 ` [PATCH v5 2/8] iommu/dma: Introduce generic helper to retrieve RMR info Shameer Kolothum
2021-05-24 15:35   ` kernel test robot
2021-05-24 11:02 ` [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory regions Shameer Kolothum
2021-05-26  7:53   ` Laurentiu Tudor
2021-05-26 16:36     ` Shameerali Kolothum Thodi
2021-05-26 17:11       ` Laurentiu Tudor
2021-06-03 12:27         ` Jon Nettleton
2021-06-03 12:32           ` Laurentiu Tudor
2021-05-27  4:25       ` Jon Nettleton
2021-06-14 10:35       ` Robin Murphy
2021-06-14 11:23   ` Robin Murphy
2021-06-14 12:49     ` Shameerali Kolothum Thodi
2021-06-29 17:34       ` Jon Nettleton
2021-07-04  7:38         ` Jon Nettleton
2021-07-05  9:10           ` Shameerali Kolothum Thodi
2021-05-24 11:02 ` [PATCH v5 4/8] iommu/arm-smmu-v3: Introduce strtab init helper Shameer Kolothum
2021-05-24 11:02 ` [PATCH v5 5/8] iommu/arm-smmu-v3: Add bypass flag to arm_smmu_write_strtab_ent() Shameer Kolothum
2021-06-14 10:23   ` Robin Murphy
2021-06-14 12:51     ` Shameerali Kolothum Thodi
2021-05-24 11:02 ` [PATCH v5 6/8] iommu/arm-smmu-v3: Get associated RMR info and install Shameer Kolothum
2021-06-14 10:15   ` Robin Murphy
2021-05-24 11:02 ` [PATCH v5 7/8] iommu/arm-smmu: Get associated RMR info and install bypass SMR Shameer Kolothum
2021-06-03  8:52   ` Jon Nettleton
2021-06-03 11:27     ` Steven Price
2021-06-03 11:51       ` Jon Nettleton
2021-06-13  7:40         ` Jon Nettleton
2021-06-14  9:23           ` Robin Murphy
2021-06-14 10:06   ` Robin Murphy
2021-06-14 16:51     ` Shameerali Kolothum Thodi
2021-06-15  8:02       ` Jon Nettleton
2021-06-29  7:03     ` Jon Nettleton
2021-06-29 13:22       ` Robin Murphy
2021-06-29 16:25         ` Jon Nettleton
2021-06-30  8:50           ` Shameerali Kolothum Thodi
2021-05-24 11:02 ` [PATCH v5 8/8] iommu/dma: Reserve any RMR regions associated with a dev Shameer Kolothum
2021-05-24 15:18 ` [PATCH v5 0/8] ACPI/IORT: Support for IORT RMR node Steven Price

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).