* [PATCH V9 7/9] dmaengine: pl330: Make sure microcode is privileged
From: Sricharan R @ 2017-01-06 13:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1483709296-32761-1-git-send-email-sricharan@codeaurora.org>
From: Mitchel Humpherys <mitchelh@codeaurora.org>
The PL330 is hard-wired such that instruction fetches on both the
manager and channel threads go out onto the bus with the "privileged"
bit set. This can become troublesome once there is an IOMMU or other
form of memory protection downstream, since those will typically be
programmed by the DMA mapping subsystem in the expectation of normal
unprivileged transactions (such as the PL330 channel threads' own data
accesses as currently configured by this driver).
To avoid the case of, say, an IOMMU blocking an unexpected privileged
transaction with a permission fault, use the newly-introduced
DMA_ATTR_PRIVILEGED attribute for the mapping of our microcode buffer.
That way the DMA layer can do whatever it needs to do to make things
continue to work as expected on more complex systems.
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Vinod Koul <vinod.koul@intel.com>
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
Tested-by: Robin Murphy <robin.murphy@arm.com>
Acked-by: Will Deacon <will.deacon@arm.com>
Acked-by: Vinod Koul <vinod.koul@intel.com>
Signed-off-by: Mitchel Humpherys <mitchelh@codeaurora.org>
[rm: remove now-redundant local variable, clarify commit message]
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
drivers/dma/pl330.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 87fd015..5a90d0c 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -1864,9 +1864,10 @@ static int dmac_alloc_resources(struct pl330_dmac *pl330)
* Alloc MicroCode buffer for 'chans' Channel threads.
* A channel's buffer offset is (Channel_Id * MCODE_BUFF_PERCHAN)
*/
- pl330->mcode_cpu = dma_alloc_coherent(pl330->ddma.dev,
+ pl330->mcode_cpu = dma_alloc_attrs(pl330->ddma.dev,
chans * pl330->mcbufsz,
- &pl330->mcode_bus, GFP_KERNEL);
+ &pl330->mcode_bus, GFP_KERNEL,
+ DMA_ATTR_PRIVILEGED);
if (!pl330->mcode_cpu) {
dev_err(pl330->ddma.dev, "%s:%d Can't allocate memory!\n",
__func__, __LINE__);
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related
* [PATCH V9 6/9] arm/dma-mapping: Implement DMA_ATTR_PRIVILEGED
From: Sricharan R @ 2017-01-06 13:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1483709296-32761-1-git-send-email-sricharan@codeaurora.org>
The newly added DMA_ATTR_PRIVILEGED is useful for creating mappings that
are only accessible to privileged DMA engines. Adding it to the
arm dma-mapping.c so that the ARM32 DMA IOMMU mapper can make use of it.
Signed-off-by: Sricharan R <sricharan@codeaurora.org>
Reviewed-by: Will Deacon <will.deacon@arm.com>
---
arch/arm/mm/dma-mapping.c | 60 +++++++++++++++++++++++------------------------
1 file changed, 30 insertions(+), 30 deletions(-)
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index ab77100..82d3e79 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1171,6 +1171,25 @@ static int __init dma_debug_do_init(void)
#ifdef CONFIG_ARM_DMA_USE_IOMMU
+static int __dma_info_to_prot(enum dma_data_direction dir, unsigned long attrs)
+{
+ int prot = 0;
+
+ if (attrs & DMA_ATTR_PRIVILEGED)
+ prot |= IOMMU_PRIV;
+
+ switch (dir) {
+ case DMA_BIDIRECTIONAL:
+ return prot | IOMMU_READ | IOMMU_WRITE;
+ case DMA_TO_DEVICE:
+ return prot | IOMMU_READ;
+ case DMA_FROM_DEVICE:
+ return prot | IOMMU_WRITE;
+ default:
+ return prot;
+ }
+}
+
/* IOMMU */
static int extend_iommu_mapping(struct dma_iommu_mapping *mapping);
@@ -1394,7 +1413,8 @@ static int __iommu_free_buffer(struct device *dev, struct page **pages,
* Create a mapping in device IO address space for specified pages
*/
static dma_addr_t
-__iommu_create_mapping(struct device *dev, struct page **pages, size_t size)
+__iommu_create_mapping(struct device *dev, struct page **pages, size_t size,
+ unsigned long attrs)
{
struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
unsigned int count = PAGE_ALIGN(size) >> PAGE_SHIFT;
@@ -1419,7 +1439,7 @@ static int __iommu_free_buffer(struct device *dev, struct page **pages,
len = (j - i) << PAGE_SHIFT;
ret = iommu_map(mapping->domain, iova, phys, len,
- IOMMU_READ|IOMMU_WRITE);
+ __dma_info_to_prot(DMA_BIDIRECTIONAL, attrs));
if (ret < 0)
goto fail;
iova += len;
@@ -1476,7 +1496,8 @@ static struct page **__iommu_get_pages(void *cpu_addr, unsigned long attrs)
}
static void *__iommu_alloc_simple(struct device *dev, size_t size, gfp_t gfp,
- dma_addr_t *handle, int coherent_flag)
+ dma_addr_t *handle, int coherent_flag,
+ unsigned long attrs)
{
struct page *page;
void *addr;
@@ -1488,7 +1509,7 @@ static void *__iommu_alloc_simple(struct device *dev, size_t size, gfp_t gfp,
if (!addr)
return NULL;
- *handle = __iommu_create_mapping(dev, &page, size);
+ *handle = __iommu_create_mapping(dev, &page, size, attrs);
if (*handle == DMA_ERROR_CODE)
goto err_mapping;
@@ -1522,7 +1543,7 @@ static void *__arm_iommu_alloc_attrs(struct device *dev, size_t size,
if (coherent_flag == COHERENT || !gfpflags_allow_blocking(gfp))
return __iommu_alloc_simple(dev, size, gfp, handle,
- coherent_flag);
+ coherent_flag, attrs);
/*
* Following is a work-around (a.k.a. hack) to prevent pages
@@ -1537,7 +1558,7 @@ static void *__arm_iommu_alloc_attrs(struct device *dev, size_t size,
if (!pages)
return NULL;
- *handle = __iommu_create_mapping(dev, pages, size);
+ *handle = __iommu_create_mapping(dev, pages, size, attrs);
if (*handle == DMA_ERROR_CODE)
goto err_buffer;
@@ -1672,27 +1693,6 @@ static int arm_iommu_get_sgtable(struct device *dev, struct sg_table *sgt,
GFP_KERNEL);
}
-static int __dma_direction_to_prot(enum dma_data_direction dir)
-{
- int prot;
-
- switch (dir) {
- case DMA_BIDIRECTIONAL:
- prot = IOMMU_READ | IOMMU_WRITE;
- break;
- case DMA_TO_DEVICE:
- prot = IOMMU_READ;
- break;
- case DMA_FROM_DEVICE:
- prot = IOMMU_WRITE;
- break;
- default:
- prot = 0;
- }
-
- return prot;
-}
-
/*
* Map a part of the scatter-gather list into contiguous io address space
*/
@@ -1722,7 +1722,7 @@ static int __map_sg_chunk(struct device *dev, struct scatterlist *sg,
if (!is_coherent && (attrs & DMA_ATTR_SKIP_CPU_SYNC) == 0)
__dma_page_cpu_to_dev(sg_page(s), s->offset, s->length, dir);
- prot = __dma_direction_to_prot(dir);
+ prot = __dma_info_to_prot(dir, attrs);
ret = iommu_map(mapping->domain, iova, phys, len, prot);
if (ret < 0)
@@ -1930,7 +1930,7 @@ static dma_addr_t arm_coherent_iommu_map_page(struct device *dev, struct page *p
if (dma_addr == DMA_ERROR_CODE)
return dma_addr;
- prot = __dma_direction_to_prot(dir);
+ prot = __dma_info_to_prot(dir, attrs);
ret = iommu_map(mapping->domain, dma_addr, page_to_phys(page), len, prot);
if (ret < 0)
@@ -2036,7 +2036,7 @@ static dma_addr_t arm_iommu_map_resource(struct device *dev,
if (dma_addr == DMA_ERROR_CODE)
return dma_addr;
- prot = __dma_direction_to_prot(dir) | IOMMU_MMIO;
+ prot = __dma_info_to_prot(dir, attrs) | IOMMU_MMIO;
ret = iommu_map(mapping->domain, dma_addr, addr, len, prot);
if (ret < 0)
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related
* [PATCH V9 5/9] arm64/dma-mapping: Implement DMA_ATTR_PRIVILEGED
From: Sricharan R @ 2017-01-06 13:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1483709296-32761-1-git-send-email-sricharan@codeaurora.org>
From: Mitchel Humpherys <mitchelh@codeaurora.org>
The newly added DMA_ATTR_PRIVILEGED is useful for creating mappings that
are only accessible to privileged DMA engines. Implement it in
dma-iommu.c so that the ARM64 DMA IOMMU mapper can make use of it.
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
Tested-by: Robin Murphy <robin.murphy@arm.com>
Acked-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Mitchel Humpherys <mitchelh@codeaurora.org>
---
arch/arm64/mm/dma-mapping.c | 6 +++---
drivers/iommu/dma-iommu.c | 12 +++++++++---
include/linux/dma-iommu.h | 3 ++-
3 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 290a84f..49c6f75 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -557,7 +557,7 @@ static void *__iommu_alloc_attrs(struct device *dev, size_t size,
unsigned long attrs)
{
bool coherent = is_device_dma_coherent(dev);
- int ioprot = dma_direction_to_prot(DMA_BIDIRECTIONAL, coherent);
+ int ioprot = dma_info_to_prot(DMA_BIDIRECTIONAL, coherent, attrs);
size_t iosize = size;
void *addr;
@@ -711,7 +711,7 @@ static dma_addr_t __iommu_map_page(struct device *dev, struct page *page,
unsigned long attrs)
{
bool coherent = is_device_dma_coherent(dev);
- int prot = dma_direction_to_prot(dir, coherent);
+ int prot = dma_info_to_prot(dir, coherent, attrs);
dma_addr_t dev_addr = iommu_dma_map_page(dev, page, offset, size, prot);
if (!iommu_dma_mapping_error(dev, dev_addr) &&
@@ -769,7 +769,7 @@ static int __iommu_map_sg_attrs(struct device *dev, struct scatterlist *sgl,
__iommu_sync_sg_for_device(dev, sgl, nelems, dir);
return iommu_dma_map_sg(dev, sgl, nelems,
- dma_direction_to_prot(dir, coherent));
+ dma_info_to_prot(dir, coherent, attrs));
}
static void __iommu_unmap_sg_attrs(struct device *dev,
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 2db0d64..3006eee 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -181,16 +181,22 @@ int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
EXPORT_SYMBOL(iommu_dma_init_domain);
/**
- * dma_direction_to_prot - Translate DMA API directions to IOMMU API page flags
+ * dma_info_to_prot - Translate DMA API directions and attributes to IOMMU API
+ * page flags.
* @dir: Direction of DMA transfer
* @coherent: Is the DMA master cache-coherent?
+ * @attrs: DMA attributes for the mapping
*
* Return: corresponding IOMMU API page protection flags
*/
-int dma_direction_to_prot(enum dma_data_direction dir, bool coherent)
+int dma_info_to_prot(enum dma_data_direction dir, bool coherent,
+ unsigned long attrs)
{
int prot = coherent ? IOMMU_CACHE : 0;
+ if (attrs & DMA_ATTR_PRIVILEGED)
+ prot |= IOMMU_PRIV;
+
switch (dir) {
case DMA_BIDIRECTIONAL:
return prot | IOMMU_READ | IOMMU_WRITE;
@@ -633,7 +639,7 @@ dma_addr_t iommu_dma_map_resource(struct device *dev, phys_addr_t phys,
size_t size, enum dma_data_direction dir, unsigned long attrs)
{
return __iommu_dma_map(dev, phys, size,
- dma_direction_to_prot(dir, false) | IOMMU_MMIO);
+ dma_info_to_prot(dir, false, attrs) | IOMMU_MMIO);
}
void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle,
diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
index 7f7e9a7..c5511e1 100644
--- a/include/linux/dma-iommu.h
+++ b/include/linux/dma-iommu.h
@@ -34,7 +34,8 @@ int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
u64 size, struct device *dev);
/* General helpers for DMA-API <-> IOMMU-API interaction */
-int dma_direction_to_prot(enum dma_data_direction dir, bool coherent);
+int dma_info_to_prot(enum dma_data_direction dir, bool coherent,
+ unsigned long attrs);
/*
* These implement the bulk of the relevant DMA mapping callbacks, but require
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related
* [PATCH V9 4/9] common: DMA-mapping: add DMA_ATTR_PRIVILEGED attribute
From: Sricharan R @ 2017-01-06 13:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1483709296-32761-1-git-send-email-sricharan@codeaurora.org>
From: Mitchel Humpherys <mitchelh@codeaurora.org>
This patch adds the DMA_ATTR_PRIVILEGED attribute to the DMA-mapping
subsystem.
Some advanced peripherals such as remote processors and GPUs perform
accesses to DMA buffers in both privileged "supervisor" and unprivileged
"user" modes. This attribute is used to indicate to the DMA-mapping
subsystem that the buffer is fully accessible at the elevated privilege
level (and ideally inaccessible or at least read-only at the
lesser-privileged levels).
Cc: linux-doc at vger.kernel.org
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
Tested-by: Robin Murphy <robin.murphy@arm.com>
Acked-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Mitchel Humpherys <mitchelh@codeaurora.org>
---
Documentation/DMA-attributes.txt | 10 ++++++++++
include/linux/dma-mapping.h | 7 +++++++
2 files changed, 17 insertions(+)
diff --git a/Documentation/DMA-attributes.txt b/Documentation/DMA-attributes.txt
index 98bf7ac..44c6bc4 100644
--- a/Documentation/DMA-attributes.txt
+++ b/Documentation/DMA-attributes.txt
@@ -143,3 +143,13 @@ So, this provides a way for drivers to avoid those error messages on calls
where allocation failures are not a problem, and shouldn't bother the logs.
NOTE: At the moment DMA_ATTR_NO_WARN is only implemented on PowerPC.
+
+DMA_ATTR_PRIVILEGED
+------------------------------
+
+Some advanced peripherals such as remote processors and GPUs perform
+accesses to DMA buffers in both privileged "supervisor" and unprivileged
+"user" modes. This attribute is used to indicate to the DMA-mapping
+subsystem that the buffer is fully accessible at the elevated privilege
+level (and ideally inaccessible or at least read-only at the
+lesser-privileged levels).
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 10c5a17b1..c24721a 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -63,6 +63,13 @@
#define DMA_ATTR_NO_WARN (1UL << 8)
/*
+ * DMA_ATTR_PRIVILEGED: used to indicate that the buffer is fully
+ * accessible at an elevated privilege level (and ideally inaccessible or
+ * at least read-only@lesser-privileged levels).
+ */
+#define DMA_ATTR_PRIVILEGED (1UL << 9)
+
+/*
* A dma_addr_t can hold any valid DMA or bus address for the platform.
* It can be given to a device to use as a DMA source or target. A CPU cannot
* reference a dma_addr_t directly because there may be translation between
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related
* [PATCH V9 3/9] iommu/io-pgtable-arm-v7s: Add support for the IOMMU_PRIV flag
From: Sricharan R @ 2017-01-06 13:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1483709296-32761-1-git-send-email-sricharan@codeaurora.org>
From: Robin Murphy <robin.murphy@arm.com>
The short-descriptor format also allows privileged-only mappings, so
let's wire it up.
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
Tested-by: Sricharan R <sricharan@codeaurora.org>
---
drivers/iommu/io-pgtable-arm-v7s.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/io-pgtable-arm-v7s.c b/drivers/iommu/io-pgtable-arm-v7s.c
index 0769276..1c049e2 100644
--- a/drivers/iommu/io-pgtable-arm-v7s.c
+++ b/drivers/iommu/io-pgtable-arm-v7s.c
@@ -265,7 +265,9 @@ static arm_v7s_iopte arm_v7s_prot_to_pte(int prot, int lvl,
if (!(prot & IOMMU_MMIO))
pte |= ARM_V7S_ATTR_TEX(1);
if (ap) {
- pte |= ARM_V7S_PTE_AF | ARM_V7S_PTE_AP_UNPRIV;
+ pte |= ARM_V7S_PTE_AF;
+ if (!(prot & IOMMU_PRIV))
+ pte |= ARM_V7S_PTE_AP_UNPRIV;
if (!(prot & IOMMU_WRITE))
pte |= ARM_V7S_PTE_AP_RDONLY;
}
@@ -288,6 +290,8 @@ static int arm_v7s_pte_to_prot(arm_v7s_iopte pte, int lvl)
if (!(attr & ARM_V7S_PTE_AP_RDONLY))
prot |= IOMMU_WRITE;
+ if (!(attr & ARM_V7S_PTE_AP_UNPRIV))
+ prot |= IOMMU_PRIV;
if ((attr & (ARM_V7S_TEX_MASK << ARM_V7S_TEX_SHIFT)) == 0)
prot |= IOMMU_MMIO;
else if (pte & ARM_V7S_ATTR_C)
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related
* [PATCH V9 2/9] iommu/io-pgtable-arm: add support for the IOMMU_PRIV flag
From: Sricharan R @ 2017-01-06 13:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1483709296-32761-1-git-send-email-sricharan@codeaurora.org>
From: Jeremy Gebben <jgebben@codeaurora.org>
Allow the creation of privileged mode mappings, for stage 1 only.
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
Tested-by: Robin Murphy <robin.murphy@arm.com>
Acked-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Jeremy Gebben <jgebben@codeaurora.org>
---
drivers/iommu/io-pgtable-arm.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index a40ce34..feacc54 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -350,11 +350,14 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
if (data->iop.fmt == ARM_64_LPAE_S1 ||
data->iop.fmt == ARM_32_LPAE_S1) {
- pte = ARM_LPAE_PTE_AP_UNPRIV | ARM_LPAE_PTE_nG;
+ pte = ARM_LPAE_PTE_nG;
if (!(prot & IOMMU_WRITE) && (prot & IOMMU_READ))
pte |= ARM_LPAE_PTE_AP_RDONLY;
+ if (!(prot & IOMMU_PRIV))
+ pte |= ARM_LPAE_PTE_AP_UNPRIV;
+
if (prot & IOMMU_MMIO)
pte |= (ARM_LPAE_MAIR_ATTR_IDX_DEV
<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related
* [PATCH V9 1/9] iommu: add IOMMU_PRIV attribute
From: Sricharan R @ 2017-01-06 13:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1483709296-32761-1-git-send-email-sricharan@codeaurora.org>
From: Mitchel Humpherys <mitchelh@codeaurora.org>
Add the IOMMU_PRIV attribute, which is used to indicate privileged
mappings.
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
Tested-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Mitchel Humpherys <mitchelh@codeaurora.org>
Acked-by: Will Deacon <will.deacon@arm.com>
---
include/linux/iommu.h | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 0ff5111..69e2417 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -31,6 +31,13 @@
#define IOMMU_CACHE (1 << 2) /* DMA cache coherency */
#define IOMMU_NOEXEC (1 << 3)
#define IOMMU_MMIO (1 << 4) /* e.g. things like MSI doorbells */
+/*
+ * This is to make the IOMMU API setup privileged
+ * mapppings accessible by the master only@higher
+ * privileged execution level and inaccessible at
+ * less privileged levels.
+ */
+#define IOMMU_PRIV (1 << 5)
struct iommu_ops;
struct iommu_group;
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related
* [PATCH V9 0/9] Add support for privileged mappings
From: Sricharan R @ 2017-01-06 13:28 UTC (permalink / raw)
To: linux-arm-kernel
This series is a resend of the V5 that Mitch sent sometime back [2]
All the patches are the same and i have just rebased. Redid patch [3],
as it does not apply in this code base. Added a couple of more patches
[4], [5] from Robin for adding the privileged attributes to armv7s format
and arm-smmuv3 revert. Added a patch for passing in the privileged
attributes from arm32 dma-mapping apis as well.
The following patch to the ARM SMMU driver:
commit d346180e70b91b3d5a1ae7e5603e65593d4622bc
Author: Robin Murphy <robin.murphy@arm.com>
Date: Tue Jan 26 18:06:34 2016 +0000
iommu/arm-smmu: Treat all device transactions as unprivileged
started forcing all SMMU transactions to come through as "unprivileged".
The rationale given was that:
(1) There is no way in the IOMMU API to even request privileged
mappings.
(2) It's difficult to implement a DMA mapper that correctly models the
ARM VMSAv8 behavior of unprivileged-writeable =>
privileged-execute-never.
This series rectifies (1) by introducing an IOMMU API for privileged
mappings and implements it in io-pgtable-arm.
This series rectifies (2) by introducing a new dma attribute
(DMA_ATTR_PRIVILEGED) for users of the DMA API that need privileged
mappings which are inaccessible to lesser-privileged execution levels, and
implements it in the arm64 IOMMU DMA mapper. The one known user (pl330.c)
is converted over to the new attribute.
Jordan and Jeremy can provide more info on the use case if needed, but the
high level is that it's a security feature to prevent attacks such as [1].
Note that, i tested this on arm64 with arm-smmuv2, short descriptor changes,
tested this on arm32 platform as well and do not have an platform to test
this with arm-smmuv3.
[1] https://github.com/robclark/kilroy
[2] https://lkml.org/lkml/2016/7/27/590
[3] https://patchwork.kernel.org/patch/9250493/
[4] http://www.linux-arm.org/git?p=linux-rm.git;a=commit;h=1291bd74f05d31da1dab3df02987cba5bd25849b
[5] http://www.linux-arm.org/git?p=linux-rm.git;a=commit;h=a79c1c6333f26849dba418cd92de26b60f5954f3
Changelog:
v8..v9
- Added additional comment in patch 1 and added tag for patch 6.
v7..v8
- Added a patch for passing in the privileged attributes from arm32
dma-mapping apis as well.
v6..v7
- Added couple of more patches, picked up acks, updated commit log
v5..v6
- Rebased all the patches and redid 6/6 as it does not apply in
this code base.
v4..v5
- Simplified patch 4/6 (suggested by Robin Murphy).
v3..v4
- Rebased and reworked on linux next due to the dma attrs rework going
on over there. Patches changed: 3/6, 4/6, and 5/6.
v2..v3
- Incorporated feedback from Robin:
* Various comments and re-wordings.
* Use existing bit definitions for IOMMU_PRIV implementation
in io-pgtable-arm.
* Renamed and redocumented dma_direction_to_prot.
* Don't worry about executability in new DMA attr.
v1..v2
- Added a new DMA attribute to make executable privileged mappings
work, and use that in the pl330 driver (suggested by Will).
Jeremy Gebben (1):
iommu/io-pgtable-arm: add support for the IOMMU_PRIV flag
Mitchel Humpherys (4):
iommu: add IOMMU_PRIV attribute
common: DMA-mapping: add DMA_ATTR_PRIVILEGED attribute
arm64/dma-mapping: Implement DMA_ATTR_PRIVILEGED
dmaengine: pl330: Make sure microcode is privileged
Robin Murphy (2):
iommu/io-pgtable-arm-v7s: Add support for the IOMMU_PRIV flag
Revert "iommu/arm-smmu: Set PRIVCFG in stage 1 STEs"
Sricharan R (2):
arm/dma-mapping: Implement DMA_ATTR_PRIVILEGED
iommu/arm-smmu: Set privileged attribute to 'default' instead of
'unprivileged'
Documentation/DMA-attributes.txt | 10 +++++++
arch/arm/mm/dma-mapping.c | 60 +++++++++++++++++++-------------------
arch/arm64/mm/dma-mapping.c | 6 ++--
drivers/dma/pl330.c | 5 ++--
drivers/iommu/arm-smmu-v3.c | 7 +----
drivers/iommu/arm-smmu.c | 2 +-
drivers/iommu/dma-iommu.c | 12 ++++++--
drivers/iommu/io-pgtable-arm-v7s.c | 6 +++-
drivers/iommu/io-pgtable-arm.c | 5 +++-
include/linux/dma-iommu.h | 3 +-
include/linux/dma-mapping.h | 7 +++++
include/linux/iommu.h | 7 +++++
12 files changed, 82 insertions(+), 48 deletions(-)
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply
* [PATCH V9 3/3] irqchip: qcom: Add IRQ combiner driver
From: Agustin Vega-Frias @ 2017-01-06 13:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <ded278ef-a0a2-811a-f362-70555f74d6fc@arm.com>
Hey Marc,
On 2017-01-05 11:48, Marc Zyngier wrote:
> Hi Agustin,
>
> On 14/12/16 22:10, Agustin Vega-Frias wrote:
>> Driver for interrupt combiners in the Top-level Control and Status
>> Registers (TCSR) hardware block in Qualcomm Technologies chips.
>>
>> An interrupt combiner in this block combines a set of interrupts by
>> OR'ing the individual interrupt signals into a summary interrupt
>> signal routed to a parent interrupt controller, and provides read-
>> only, 32-bit registers to query the status of individual interrupts.
>> The status bit for IRQ n is bit (n % 32) within register (n / 32)
>> of the given combiner. Thus, each combiner can be described as a set
>> of register offsets and the number of IRQs managed.
>>
>> Signed-off-by: Agustin Vega-Frias <agustinv@codeaurora.org>
>> ---
>> drivers/irqchip/Kconfig | 9 +
>> drivers/irqchip/Makefile | 1 +
>> drivers/irqchip/qcom-irq-combiner.c | 322
>> ++++++++++++++++++++++++++++++++++++
>> 3 files changed, 332 insertions(+)
>> create mode 100644 drivers/irqchip/qcom-irq-combiner.c
>>
>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>> index bc0af33..3e3430c 100644
>> --- a/drivers/irqchip/Kconfig
>> +++ b/drivers/irqchip/Kconfig
>> @@ -279,3 +279,12 @@ config EZNPS_GIC
>> config STM32_EXTI
>> bool
>> select IRQ_DOMAIN
>> +
>> +config QCOM_IRQ_COMBINER
>> + bool "QCOM IRQ combiner support"
>> + depends on ARCH_QCOM && ACPI
>> + select IRQ_DOMAIN
>> + select IRQ_DOMAIN_HIERARCHY
>> + help
>> + Say yes here to add support for the IRQ combiner devices embedded
>> + in Qualcomm Technologies chips.
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index e4dbfc8..1818a0b 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -74,3 +74,4 @@ obj-$(CONFIG_LS_SCFG_MSI) += irq-ls-scfg-msi.o
>> obj-$(CONFIG_EZNPS_GIC) += irq-eznps.o
>> obj-$(CONFIG_ARCH_ASPEED) += irq-aspeed-vic.o
>> obj-$(CONFIG_STM32_EXTI) += irq-stm32-exti.o
>> +obj-$(CONFIG_QCOM_IRQ_COMBINER) += qcom-irq-combiner.o
>> diff --git a/drivers/irqchip/qcom-irq-combiner.c
>> b/drivers/irqchip/qcom-irq-combiner.c
>> new file mode 100644
>> index 0000000..0055e08
>> --- /dev/null
>> +++ b/drivers/irqchip/qcom-irq-combiner.c
>> @@ -0,0 +1,322 @@
>> +/* Copyright (c) 2015-2016, The Linux Foundation. All rights
>> reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +/*
>> + * Driver for interrupt combiners in the Top-level Control and Status
>> + * Registers (TCSR) hardware block in Qualcomm Technologies chips.
>> + * An interrupt combiner in this block combines a set of interrupts
>> by
>> + * OR'ing the individual interrupt signals into a summary interrupt
>> + * signal routed to a parent interrupt controller, and provides read-
>> + * only, 32-bit registers to query the status of individual
>> interrupts.
>> + * The status bit for IRQ n is bit (n % 32) within register (n / 32)
>> + * of the given combiner. Thus, each combiner can be described as a
>> set
>> + * of register offsets and the number of IRQs managed.
>> + */
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/irqchip/chained_irq.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/platform_device.h>
>> +
>> +#define REG_SIZE 32
>> +
>> +struct combiner_reg {
>> + void __iomem *addr;
>> + unsigned long mask;
>> +};
>> +
>> +struct combiner {
>> + struct irq_domain *domain;
>> + int parent_irq;
>> + u32 nirqs;
>> + u32 nregs;
>> + struct combiner_reg regs[0];
>> +};
>> +
>> +static inline u32 irq_register(int irq)
>> +{
>> + return irq / REG_SIZE;
>> +}
>> +
>> +static inline u32 irq_bit(int irq)
>> +{
>> + return irq % REG_SIZE;
>> +
>> +}
>> +
>> +static inline int irq_nr(u32 reg, u32 bit)
>> +{
>> + return reg * REG_SIZE + bit;
>> +}
>> +
>> +/*
>> + * Handler for the cascaded IRQ.
>> + */
>> +static void combiner_handle_irq(struct irq_desc *desc)
>> +{
>> + struct combiner *combiner = irq_desc_get_handler_data(desc);
>> + struct irq_chip *chip = irq_desc_get_chip(desc);
>> + u32 reg;
>> +
>> + chained_irq_enter(chip, desc);
>> +
>> + for (reg = 0; reg < combiner->nregs; reg++) {
>> + int virq;
>> + int hwirq;
>> + u32 bit;
>> + u32 status;
>> +
>> + if (combiner->regs[reg].mask == 0)
>> + continue;
>
> I'm a bit worried by this. If I understand it well, this is a pure
> software construct (controlled from combiner_irq_chip_{un,}mask_irq)
> and
> there is nothing that actually masks the interrupt at the HW level.
>
> So if a device asserts its interrupt line, what mechanism do we have to
> make sure that we don't end-up with the CPU pegged in interrupt
> context?
>
Yes, unfortunately this is a dumb hardware combiner; however, the real
use
of mask here is to optimize IRQ handling if the combiner instance has
its
IRQ statuses across more than one register. Currently all active
instances
only use one register, but we are getting close to 32 in one case, so I
wanted to accommodate a general case where an instance can combine more
than 32 IRQs.
Having said that, what I'm inclined to do is to remove the use of mask
on the status read form the register on the next two lines, and then let
irq_find_mapping fail if we are getting an unexpected IRQ, then call
handle_bad_irq(desc).
Do you have any other suggestions to handle the scenario? E.g., can we
safely disable the parent IRQ from this context if we see too many
spurious interrupts?
>> +
>> + status = readl_relaxed(combiner->regs[reg].addr);
>> + status &= combiner->regs[reg].mask;
>> +
>> + while (status) {
>> + bit = __ffs(status);
>> + status &= ~(1 << bit);
>> + hwirq = irq_nr(reg, bit);
>> + virq = irq_find_mapping(combiner->domain, hwirq);
>> + if (virq >= 0)
>
> Small bug: virq == 0 shouldn't happen, since this would be an
> indication
> that the hwirq doesn't have a mapping.
>
Will fix this on V10.
>> + generic_handle_irq(virq);
>> +
>> + }
>> + }
>> +
>> + chained_irq_exit(chip, desc);
>> +}
>> +
>> +/*
>> + * irqchip callbacks
>> + */
>> +
>> +static void combiner_irq_chip_mask_irq(struct irq_data *data)
>> +{
>> + struct combiner *combiner = irq_data_get_irq_chip_data(data);
>> + struct combiner_reg *reg = combiner->regs +
>> irq_register(data->hwirq);
>> +
>> + clear_bit(irq_bit(data->hwirq), ®->mask);
>> +}
>> +
>> +static void combiner_irq_chip_unmask_irq(struct irq_data *data)
>> +{
>> + struct combiner *combiner = irq_data_get_irq_chip_data(data);
>> + struct combiner_reg *reg = combiner->regs +
>> irq_register(data->hwirq);
>> +
>> + set_bit(irq_bit(data->hwirq), ®->mask);
>> +}
>> +
>> +static struct irq_chip irq_chip = {
>> + .irq_mask = combiner_irq_chip_mask_irq,
>> + .irq_unmask = combiner_irq_chip_unmask_irq,
>> + .name = "qcom-irq-combiner"
>> +};
>> +
>> +/*
>> + * irq_domain_ops callbacks
>> + */
>> +
>> +static int combiner_irq_map(struct irq_domain *domain, unsigned int
>> irq,
>> + irq_hw_number_t hwirq)
>> +{
>> + struct combiner *combiner = domain->host_data;
>> +
>> + if (hwirq >= combiner->nirqs)
>> + return -EINVAL;
>
> Given that this should have come from the translate function, can we
> move the check there instead, so that we validate everything early?
>
Will fix this on V10.
>> +
>> + irq_set_chip_and_handler(irq, &irq_chip, handle_level_irq);
>> + irq_set_chip_data(irq, combiner);
>> + irq_set_noprobe(irq);
>> + return 0;
>> +}
>> +
>> +static void combiner_irq_unmap(struct irq_domain *domain, unsigned
>> int irq)
>> +{
>> + struct irq_data *data = irq_get_irq_data(irq);
>> +
>> + if (WARN_ON(!data))
>> + return;
>
> Can this happen?
>
I will remove this check on V10 given that irq_domain_disassociate
already
has a check for a NULL irq_data.
>> + irq_domain_reset_irq_data(data);
>> +}
>> +
>> +static int combiner_irq_translate(struct irq_domain *d, struct
>> irq_fwspec *fws,
>> + unsigned long *hwirq, unsigned int *type)
>> +{
>> + if (is_acpi_node(fws->fwnode)) {
>> + if (WARN_ON((fws->param_count != 2) ||
>> + (fws->param[1] & IORESOURCE_IRQ_LOWEDGE) ||
>> + (fws->param[1] & IORESOURCE_IRQ_HIGHEDGE)))
>> + return -EINVAL;
>> +
>> + *hwirq = fws->param[0];
>> + *type = fws->param[1];
>> + return 0;
>> + }
>> +
>> + return -EINVAL;
>> +}
>> +
>> +static const struct irq_domain_ops domain_ops = {
>> + .map = combiner_irq_map,
>> + .unmap = combiner_irq_unmap,
>> + .translate = combiner_irq_translate
>> +};
>> +
>> +/*
>> + * Device probing
>> + */
>> +
>> +static acpi_status count_registers_cb(struct acpi_resource *ares,
>> void *context)
>> +{
>> + int *count = context;
>> +
>> + if (ares->type == ACPI_RESOURCE_TYPE_GENERIC_REGISTER)
>> + ++(*count);
>> + return AE_OK;
>> +}
>> +
>> +static int count_registers(struct platform_device *pdev)
>> +{
>> + struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
>> + acpi_status status;
>> + int count = 0;
>> +
>> + if (!acpi_has_method(adev->handle, METHOD_NAME__CRS))
>> + return -EINVAL;
>> +
>> + status = acpi_walk_resources(adev->handle, METHOD_NAME__CRS,
>> + count_registers_cb, &count);
>> + if (ACPI_FAILURE(status))
>> + return -EINVAL;
>> + return count;
>> +}
>> +
>> +struct get_registers_context {
>> + struct device *dev;
>> + struct combiner *combiner;
>> + int err;
>> +};
>> +
>> +static acpi_status get_registers_cb(struct acpi_resource *ares, void
>> *context)
>> +{
>> + struct get_registers_context *ctx = context;
>> + struct acpi_resource_generic_register *reg;
>> + phys_addr_t paddr;
>> + void __iomem *vaddr;
>> +
>> + if (ares->type != ACPI_RESOURCE_TYPE_GENERIC_REGISTER)
>> + return AE_OK;
>> +
>> + reg = &ares->data.generic_reg;
>> + paddr = reg->address;
>> + if ((reg->space_id != ACPI_SPACE_MEM) ||
>> + (reg->bit_offset != 0) ||
>> + (reg->bit_width > REG_SIZE)) {
>> + dev_err(ctx->dev, "Bad register resource @%pa\n", &paddr);
>> + ctx->err = -EINVAL;
>> + return AE_ERROR;
>> + }
>> +
>> + vaddr = devm_ioremap(ctx->dev, reg->address, REG_SIZE);
>> + if (IS_ERR(vaddr)) {
>> + dev_err(ctx->dev, "Can't map register @%pa\n", &paddr);
>> + ctx->err = PTR_ERR(vaddr);
>> + return AE_ERROR;
>> + }
>> +
>> + ctx->combiner->regs[ctx->combiner->nregs].addr = vaddr;
>> + ctx->combiner->nirqs += reg->bit_width;
>> + ctx->combiner->nregs++;
>> + return AE_OK;
>> +}
>> +
>> +static int get_registers(struct platform_device *pdev, struct
>> combiner *comb)
>> +{
>> + struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
>> + acpi_status status;
>> + struct get_registers_context ctx;
>> +
>> + if (!acpi_has_method(adev->handle, METHOD_NAME__CRS))
>> + return -EINVAL;
>> +
>> + ctx.dev = &pdev->dev;
>> + ctx.combiner = comb;
>> + ctx.err = 0;
>> +
>> + status = acpi_walk_resources(adev->handle, METHOD_NAME__CRS,
>> + get_registers_cb, &ctx);
>> + if (ACPI_FAILURE(status))
>> + return ctx.err;
>> + return 0;
>> +}
>> +
>> +static int __init combiner_probe(struct platform_device *pdev)
>> +{
>> + struct combiner *combiner;
>> + size_t alloc_sz;
>> + u32 nregs;
>> + int err;
>> +
>> + nregs = count_registers(pdev);
>> + if (nregs <= 0) {
>> + dev_err(&pdev->dev, "Error reading register resources\n");
>> + return -EINVAL;
>> + }
>> +
>> + alloc_sz = sizeof(*combiner) + sizeof(struct combiner_reg) * nregs;
>> + combiner = devm_kzalloc(&pdev->dev, alloc_sz, GFP_KERNEL);
>> + if (!combiner)
>> + return -ENOMEM;
>> +
>> + err = get_registers(pdev, combiner);
>> + if (err < 0)
>> + return err;
>> +
>> + combiner->parent_irq = platform_get_irq(pdev, 0);
>> + if (combiner->parent_irq <= 0) {
>> + dev_err(&pdev->dev, "Error getting IRQ resource\n");
>> + return -EPROBE_DEFER;
>> + }
>> +
>> + combiner->domain = irq_domain_create_linear(pdev->dev.fwnode,
>> combiner->nirqs,
>> + &domain_ops, combiner);
>> + if (!combiner->domain)
>> + /* Errors printed by irq_domain_create_linear */
>> + return -ENODEV;
>> +
>> + irq_set_chained_handler_and_data(combiner->parent_irq,
>> + combiner_handle_irq, combiner);
>> +
>> + dev_info(&pdev->dev, "Initialized with [p=%d,n=%d,r=%p]\n",
>> + combiner->parent_irq, combiner->nirqs, combiner->regs[0].addr);
>> + return 0;
>> +}
>> +
>> +static const struct acpi_device_id qcom_irq_combiner_acpi_match[] = {
>> + { "QCOM80B1", },
>> + { }
>> +};
>> +
>> +static struct platform_driver qcom_irq_combiner_probe = {
>> + .driver = {
>> + .name = "qcom-irq-combiner",
>> + .owner = THIS_MODULE,
>> + .acpi_match_table = ACPI_PTR(qcom_irq_combiner_acpi_match),
>> + },
>> + .probe = combiner_probe,
>> +};
>> +
>> +static int __init register_qcom_irq_combiner(void)
>> +{
>> + return platform_driver_register(&qcom_irq_combiner_probe);
>> +}
>> +device_initcall(register_qcom_irq_combiner);
>>
>
> Other than the questions I have above, this now looks in a much better
> shape. Hopefully Rafael will soon come back will his conclusions on the
> first two patches, as I'd like to see this code to get some -next for a
> while.
Yes, hopefully we can get to that in the following weeks.
Thanks,
Agustin
>
> Thanks,
>
> M.
> --
> Jazz is not dead. It just smells funny...
--
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a
Linux Foundation Collaborative Project.
^ permalink raw reply
* [PATCH] ARM64: dts: meson-gxbb-odroidc2: Disable SCPI DVFS
From: Michał Zegan @ 2017-01-06 13:12 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <83fd9b61-dbff-68e0-82e5-8e01058d04a9@arm.com>
Yes, I meant what you think I meant. :) thanks
W dniu 06.01.2017 o 12:54, Sudeep Holla pisze:
> Hi Micha?,
>
> On 05/01/17 19:04, Micha? Zegan wrote:
>> Hello.
>>
>> The patch causes cpufreq module (scpi-cpufreq) not to detect cpufreq, so
>> it actually works, but...
>> Loading the module causes few errors because of not found frequencies or
>> something, then it is all okay. However after loading scpi-cpufreq you
>> cannot actually power the cpu off and on. You will power it off
>> successfully, but when trying to power it on, the cpufreq driver will
>> error out,
>
> Yes I had noticed this in past, this needs to be fixed. I had a patch
> and seems like it slipped through the cracks. I will fins and post it.
>
>> and then after it happens, the cpu that was trying to go
>> online will be offline again, and that is a little... unfortunate. The
>
> IIUC, you mean the cpufreq drive spits error on every hotplug event ?
> If so yes, otherwise I think I didn't understand you concern above.
>
>> question is, and I cannot really test that: will the module actually
>> autoload after this change?
>>
>
> It should work, I had tested this in past.
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 525 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170106/37f9d743/attachment.sig>
^ permalink raw reply
* [PATCHv2 net-next 09/11] net: mvpp2: simplify MVPP2_PRS_RI_* definitions
From: Russell King - ARM Linux @ 2017-01-06 13:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1482943567-12483-10-git-send-email-thomas.petazzoni@free-electrons.com>
On Wed, Dec 28, 2016 at 05:46:05PM +0100, Thomas Petazzoni wrote:
> Some of the MVPP2_PRS_RI_* definitions use the ~(value) syntax, which
> doesn't compile nicely on 64-bit. Moreover, those definitions are in
> fact unneeded, since they are always used in combination with a bit
> mask that ensures only the appropriate bits are modified.
>
> Therefore, such definitions should just be set to 0x0. For example:
>
> #define MVPP2_PRS_RI_L2_CAST_MASK 0x600
> #define MVPP2_PRS_RI_L2_UCAST ~(BIT(9) | BIT(10))
> #define MVPP2_PRS_RI_L2_MCAST BIT(9)
> #define MVPP2_PRS_RI_L2_BCAST BIT(10)
>
> becomes
>
> #define MVPP2_PRS_RI_L2_CAST_MASK 0x600
> #define MVPP2_PRS_RI_L2_UCAST 0x0
> #define MVPP2_PRS_RI_L2_MCAST BIT(9)
> #define MVPP2_PRS_RI_L2_BCAST BIT(10)
So this is a two-bit field in a register with three defined states - I'm
not sure that using BIT() here is really a good idea. BIT() is fine for
single-bit controls, but I think it adds an additional level of confusion
for multi-bit controls.
Also, the combination of the mask being defined as hex and the controls
using BIT() is particularly not nice. I think either use one style or
the other, don't mix them. So either:
#define MVPP2_PRS_RI_L2_CAST_MASK 0x600
#define MVPP2_PRS_RI_L2_UCAST 0x000
#define MVPP2_PRS_RI_L2_MCAST 0x200
#define MVPP2_PRS_RI_L2_BCAST 0x400
or:
#define MVPP2_PRS_RI_L2_CAST_MASK (BIT(10) | BIT(9))
#define MVPP2_PRS_RI_L2_UCAST 0
#define MVPP2_PRS_RI_L2_MCAST BIT(9)
#define MVPP2_PRS_RI_L2_BCAST BIT(10)
It then becomes obvious that the mask and the settings are changing the
same bits.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* [PATCH v6 01/18] iommu/dma: Allow MSI-only cookies
From: Auger Eric @ 2017-01-06 13:05 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <ae222579-6939-b89a-80c2-9ee8f147299e@arm.com>
Hi Robin,
On 06/01/2017 13:12, Robin Murphy wrote:
> On 06/01/17 11:46, Auger Eric wrote:
>>
>>
>> On 06/01/2017 11:59, Joerg Roedel wrote:
>>> On Thu, Jan 05, 2017 at 07:04:29PM +0000, Eric Auger wrote:
>>>> struct iommu_dma_cookie {
>>>> - struct iova_domain iovad;
>>>> - struct list_head msi_page_list;
>>>> - spinlock_t msi_lock;
>>>> + union {
>>>> + struct iova_domain iovad;
>>>> + dma_addr_t msi_iova;
>>>> + };
>>>> + struct list_head msi_page_list;
>>>> + spinlock_t msi_lock;
>>>> + enum iommu_dma_cookie_type type;
>>>
>>> Please move the type to the beginning of the struct and add a comment
>>> how the type relates to the union.
>>
>> Sure
>>
>> Thank you for the review.
>
> FWIW I already had a cleaned up version of this patch, I just hadn't
> mentioned it. I've pushed out an update with that change added too[1].
>
> Robin.
>
> [1]:http://linux-arm.org/git?p=linux-rm.git;a=shortlog;h=refs/heads/iommu/misc
Great, thanks!
Eric
>
>>
>> Best regards
>>
>> Eric
>>>
>>>
>>>
>>> Joerg
>>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* [PATCH v6 07/18] iommu: Implement reserved_regions iommu-group sysfs file
From: Auger Eric @ 2017-01-06 13:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170106124816.GQ17255@8bytes.org>
Hi,
On 06/01/2017 13:48, Joerg Roedel wrote:
> On Fri, Jan 06, 2017 at 12:46:05PM +0100, Auger Eric wrote:
>> On 06/01/2017 12:00, Joerg Roedel wrote:
>
>>> I think it also makes sense to report the type of the reserved region.
>>
>> What is the best practice in that case? Shall we put the type enum
>> values as strings such as:
>> - direct
>> - nomap
>> - msi
>>
>> and document that in Documentation/ABI/testing/sysfs-kernel-iommu_groups
>
> Yes, a string would be good. An probably 'reserved' is a better name
> than nomap?
OK that's equal to me.
Thanks
Eric
>
>
> Joerg
>
^ permalink raw reply
* [PATCH v6 08/18] iommu/vt-d: Implement reserved region get/put callbacks
From: Auger Eric @ 2017-01-06 13:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170106124631.GP17255@8bytes.org>
Hi Joerg,
On 06/01/2017 13:46, Joerg Roedel wrote:
> On Fri, Jan 06, 2017 at 12:45:54PM +0100, Auger Eric wrote:
>> On 06/01/2017 12:01, Joerg Roedel wrote:
>>> On Thu, Jan 05, 2017 at 07:04:36PM +0000, Eric Auger wrote:
>
>>> That is different from what AMD does, can you also report the RMRR
>>> regions for the device here (as direct-map regions)?
>>
>> if I return RMRR regions as direct mapped regions,
>> iommu_group_create_direct_mappings will perform the 1-1 mapping.
>
> No, this will not happen until the Intel IOMMU driver returns valid
> IOMMU_DOMAIN_DMA type domains.
Hum OK thanks!
Best Regards
Eric
>
>> I am not familiar with the intel-iommu code but I guess this job
>> currently is done in the intel driver:
>> iommu_prepare_rmrr_dev -> iommu_prepare_identity_map
>> ->domain_prepare_identity_map -> iommu_domain_identity_map?
>
> Right, this is done in the Intel driver atm.
>
>
>
> Joerg
>
^ permalink raw reply
* [PATCHv2 net-next 02/11] net: mvpp2: handle too large value in mvpp2_rx_time_coal_set()
From: Russell King - ARM Linux @ 2017-01-06 12:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1482943567-12483-3-git-send-email-thomas.petazzoni@free-electrons.com>
On Wed, Dec 28, 2016 at 05:45:58PM +0100, Thomas Petazzoni wrote:
> When configuring the MVPP2_ISR_RX_THRESHOLD_REG with the RX coalescing
> time threshold, we do not check for the maximum allowed value supported
> by the driver, which means we might overflow and use a bogus value. This
> commit adds a check for this situation, and if a value higher than what
> is supported by the hardware is provided, then we use the maximum value
> supported by the hardware.
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
> drivers/net/ethernet/marvell/mvpp2.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/net/ethernet/marvell/mvpp2.c b/drivers/net/ethernet/marvell/mvpp2.c
> index 02d91e4..a1ba89f 100644
> --- a/drivers/net/ethernet/marvell/mvpp2.c
> +++ b/drivers/net/ethernet/marvell/mvpp2.c
> @@ -154,6 +154,7 @@
>
> /* Interrupt Cause and Mask registers */
> #define MVPP2_ISR_RX_THRESHOLD_REG(rxq) (0x5200 + 4 * (rxq))
> +#define MVPP2_MAX_ISR_RX_THRESHOLD 0xfffff0
> #define MVPP2_ISR_RXQ_GROUP_REG(rxq) (0x5400 + 4 * (rxq))
> #define MVPP2_ISR_ENABLE_REG(port) (0x5420 + 4 * (port))
> #define MVPP2_ISR_ENABLE_INTERRUPT(mask) ((mask) & 0xffff)
> @@ -4397,6 +4398,12 @@ static void mvpp2_rx_time_coal_set(struct mvpp2_port *port,
> u32 val;
>
> val = (port->priv->tclk / USEC_PER_SEC) * usec;
> +
> + if (val > MVPP2_MAX_ISR_RX_THRESHOLD) {
> + val = MVPP2_MAX_ISR_RX_THRESHOLD;
> + usec = (val * USEC_PER_SEC) / port->priv->tclk;
> + }
> +
Beware of rounding and overflow errors. usec and val are u32's.
MVPP2_MAX_ISR_RX_THRESHOLD = 16777200
USEC_PER_SEC = 1000000
This equates to 0xF423F0BDC00 for the multiplication, which is a little
larger than 32-bit. Assuming tclk is 166.666666MHz (as it was on Dove
- I don't know what it would be here) and 64-bit arithmetic, the maximum
value gives 100663us.
Passing that back into the function gives... 16710058, so the second time
around, we end up with a different setting (even though a change wasn't
requested.)
However, 100664 won't trigger your check, neither will values all the way
up to 101067 - the reason being that you're losing so much precision by
dividling the clock by USEC_PER_SEC first. Only if it's a whole number
of MHz will you get away with that.
So, I'd suggest you switch to using 64 bit math here - it's not a fast
path. Using bc to evaluate val = port->priv->tclk * usec / USEC_PER_SEC
gives:
(166666666 * 100663 / 1000000)
16777166
which is as close as you can come to the limit.
So, I'd suggest (these variants round down, which is acceptable for
this implementation):
static u32 usec_to_cycles(u32 usec, unsigned long clock_rate_hz)
{
u64 tmp = clock_rate_hz * usec;
do_div(tmp, USEC_PER_SEC);
return tmp > 0xffffffff ? 0xffffffff : tmp;
}
static u32 cycles_to_usec(u32 cycles, unsigned long clock_rate_hz)
{
u64 tmp = cycles * USEC_PER_SEC;
do_div(tmp, clock_rate_hz);
return tmp > 0xffffffff ? 0xffffffff : tmp;
}
and:
u32 val = usec_to_cycles(usec, port->priv->tclk);
if (val > MVPP2_MAX_ISR_RX_THRESHOLD) {
usec = cycles_to_usec(MVPP2_MAX_ISR_RX_THRESHOLD,
port->priv->tclk);
/* re-evaluate to get actual register value for usec */
val = usec_to_cycles(usec, port->priv->tclk);
}
> mvpp2_write(port->priv, MVPP2_ISR_RX_THRESHOLD_REG(rxq->id), val);
>
> rxq->time_coal = usec;
This function appears to be called from two places:
mvpp2_rxq_init():
mvpp2_rx_time_coal_set(port, rxq, rxq->time_coal);
mvpp2_ethtool_set_coalesce():
rxq->time_coal = c->rx_coalesce_usecs;
mvpp2_rx_time_coal_set(port, rxq, rxq->time_coal);
It seems rather pointless to pass in rxq->time_coal when you're also
passing in rxq.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* [PATCH v6 07/18] iommu: Implement reserved_regions iommu-group sysfs file
From: Joerg Roedel @ 2017-01-06 12:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <888d3750-817c-77d1-8154-e77cf8c3ad4b@redhat.com>
On Fri, Jan 06, 2017 at 12:46:05PM +0100, Auger Eric wrote:
> On 06/01/2017 12:00, Joerg Roedel wrote:
> > I think it also makes sense to report the type of the reserved region.
>
> What is the best practice in that case? Shall we put the type enum
> values as strings such as:
> - direct
> - nomap
> - msi
>
> and document that in Documentation/ABI/testing/sysfs-kernel-iommu_groups
Yes, a string would be good. An probably 'reserved' is a better name
than nomap?
Joerg
^ permalink raw reply
* [PATCH v6 08/18] iommu/vt-d: Implement reserved region get/put callbacks
From: Joerg Roedel @ 2017-01-06 12:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <36bac0a9-89a6-ff7b-1870-a795a4f57700@redhat.com>
On Fri, Jan 06, 2017 at 12:45:54PM +0100, Auger Eric wrote:
> On 06/01/2017 12:01, Joerg Roedel wrote:
> > On Thu, Jan 05, 2017 at 07:04:36PM +0000, Eric Auger wrote:
> > That is different from what AMD does, can you also report the RMRR
> > regions for the device here (as direct-map regions)?
>
> if I return RMRR regions as direct mapped regions,
> iommu_group_create_direct_mappings will perform the 1-1 mapping.
No, this will not happen until the Intel IOMMU driver returns valid
IOMMU_DOMAIN_DMA type domains.
> I am not familiar with the intel-iommu code but I guess this job
> currently is done in the intel driver:
> iommu_prepare_rmrr_dev -> iommu_prepare_identity_map
> ->domain_prepare_identity_map -> iommu_domain_identity_map?
Right, this is done in the Intel driver atm.
Joerg
^ permalink raw reply
* [PATCH v4 2/5] arm64: dts: exynos: make tm2 and tm2e independent from each other
From: Javier Martinez Canillas @ 2017-01-06 12:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170106114114.19321-3-andi.shyti@samsung.com>
Hello Andi,
On 01/06/2017 08:41 AM, Andi Shyti wrote:
> Currently tm2e dts includes tm2 but there are some differences
> between the two boards and tm2 has some properties that tm2e
> doesn't have.
>
> That's why it's important to keep the two dts files independent
> and put all the commonalities in a tm2-common.dtsi file.
>
> At the current status the only two differences between the two
> dts files (besides the board name) are ldo31 and ldo38.
>
> Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
> ---
Patch looks good to me.
Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>
I've some comments though:
> diff --git a/arch/arm64/boot/dts/exynos/exynos5433-tm2.dts b/arch/arm64/boot/dts/exynos/exynos5433-tm2.dts
> dissimilarity index 98%
> index e8971f4a5977..d30b45a9c0d4 100644
> --- a/arch/arm64/boot/dts/exynos/exynos5433-tm2.dts
> +++ b/arch/arm64/boot/dts/exynos/exynos5433-tm2.dts
> @@ -1,1120 +1,33 @@
[snip]
> -
> -/ {
> - model = "Samsung TM2 board";
> - compatible = "samsung,tm2", "samsung,exynos5433";
> -
[snip]
> +
> +/ {
> + model = "Samsung TM2E board";
> + compatible = "samsung,tm2e", "samsung,exynos5433";
> +};
> +
You ended with the wrong model and compatible for TM2.
Speaking about these, I noticed that the common .dtsi has the TM2 model and
compatible. I think those don't belong to .dtsi files and instead should be
defined on each .dts file.
Best regards,
--
Javier Martinez Canillas
Open Source Group
Samsung Research America
^ permalink raw reply
* [PATCH] clk: scpi: don't add cpufreq device if the scpi dvfs node is disabled
From: Sudeep Holla @ 2017-01-06 12:34 UTC (permalink / raw)
To: linux-arm-kernel
Currently we add the virtual cpufreq device unconditionally even when
the SCPI DVFS clock provider node is disabled. This will cause cpufreq
driver to throw errors when it gets initailised on boot/modprobe and
also when the CPUs are hot-plugged back in.
This patch fixes the issue by adding the virtual cpufreq device only if
the SCPI DVFS clock provider is available and registered.
Fixes: 9490f01e2471 ("clk: scpi: add support for cpufreq virtual device")
Reported-by: Micha? Zegan <webczat_200@poczta.onet.pl>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
drivers/clk/clk-scpi.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/clk/clk-scpi.c b/drivers/clk/clk-scpi.c
index 2a3e9d8e88b0..96d37175d0ad 100644
--- a/drivers/clk/clk-scpi.c
+++ b/drivers/clk/clk-scpi.c
@@ -290,13 +290,15 @@ static int scpi_clocks_probe(struct platform_device *pdev)
of_node_put(child);
return ret;
}
- }
- /* Add the virtual cpufreq device */
- cpufreq_dev = platform_device_register_simple("scpi-cpufreq",
- -1, NULL, 0);
- if (IS_ERR(cpufreq_dev))
- pr_warn("unable to register cpufreq device");
+ if (match->data != &scpi_dvfs_ops)
+ continue;
+ /* Add the virtual cpufreq device if it's DVFS clock provider */
+ cpufreq_dev = platform_device_register_simple("scpi-cpufreq",
+ -1, NULL, 0);
+ if (IS_ERR(cpufreq_dev))
+ pr_warn("unable to register cpufreq device");
+ }
return 0;
}
--
2.7.4
^ permalink raw reply related
* [PATCH v3 4/9] arm64: cpufeature: Document the rules of safe value for features
From: Catalin Marinas @ 2017-01-06 12:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1483552147-9605-5-git-send-email-suzuki.poulose@arm.com>
On Wed, Jan 04, 2017 at 05:49:02PM +0000, Suzuki K. Poulose wrote:
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -29,7 +29,21 @@
> #include <linux/jump_label.h>
> #include <linux/kernel.h>
>
> -/* CPU feature register tracking */
> +/*
> + * CPU feature register tracking
> + *
> + * The safe value of a CPUID feature field is dependent on the implications
> + * of the values assigned to it by the architecture. Based on the relationship
> + * between the values, the features are classified into 3 types.
> + *
> + * a) LOWER_SAFE - The value 'n+1' indicates, value 'n' and some
> + * additional features. (where n >= 0). The smaller value (n) is
> + * considered safer in this case.
> + * b) HIGHER_SAFE - The value 'n+1' is safer than 'n' (for n>= 0).
> + * c) EXACT - If the values of the feature don't have any relationship,
> + * a predefined safe value is used.
> + */
I don't think this text fully describes what is actually compared. You
could say something that the lowest value of all the CPUs is chosen for
LOWER_SAFE, highest for HIGHER_SAFE and it is expected that all CPUs
have the same value for a field when EXACT is specified.
--
Catalin
^ permalink raw reply
* [PATCH v6 0/4] arm64: arch_timer: Add workaround for hisilicon-161601 erratum
From: Will Deacon @ 2017-01-06 12:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <e518ab78-4093-0bc9-fe68-e7da25fd5644@huawei.com>
On Fri, Jan 06, 2017 at 11:41:28AM +0800, Ding Tianhong wrote:
> Please help reviewing the new version, I think it is nearly close to the
> final solution and it is very important to our chip or something else chip
> has similar erratum. If there is any dissatisfaction, please inform me
> and I will fix it. :)
It looks fine to me, but I don't maintain the arch timer driver!
Will
^ permalink raw reply
* [PATCH] ARM: hw_breakpoint: blacklist Scorpion CPUs
From: Mark Rutland @ 2017-01-06 12:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170106112125.GB15333@arm.com>
On Fri, Jan 06, 2017 at 11:21:25AM +0000, Will Deacon wrote:
> On Thu, Jan 05, 2017 at 05:32:36PM +0000, Mark Rutland wrote:
> > + if (read_cpuid_part() == ARM_CPU_PART_SCORPION) {
> > + pr_info("Scorpion CPU detected. Breakpoints and watchpoints disabled\n");
>
> nit: we're disabling *hardware* breakpoints and watchpoints, so it's worth
> mentioning that in the print.
True. I've fixed up the message.
> With that:
>
> Acked-by: Will Deacon <will.deacon@arm.com>
>
> Please put this into Russell's patch system.
Cheers.
I've dropped this in the patch system as 8634/1.
Thanks,
Mark.
^ permalink raw reply
* [PATCH 2/2] arm64: mm: enable CONFIG_HOLES_IN_ZONE for NUMA
From: Ard Biesheuvel @ 2017-01-06 12:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170106120339.GA20726@arm.com>
On 6 January 2017 at 12:03, Will Deacon <will.deacon@arm.com> wrote:
> On Thu, Jan 05, 2017 at 08:49:44PM +0100, Robert Richter wrote:
>> On 05.01.17 13:22:00, Robert Richter wrote:
>> > On 05.01.17 12:08:20, Will Deacon wrote:
>> > > I really can't see how the fix causes a crash, and I couldn't reproduce
>> > > it on any of my boards, nor could any of the Linaro folk afaik. Are you
>> > > definitely running mainline with just these two patches from Ard?
>> >
>> > Yes, just both patches applied. Various other solutions were working.
>>
>> I have retested the same kernel (v4.9 based) as before and now it
>> boots fine including rtc-efi device registration (it was crashing
>> there):
>>
>> rtc-efi rtc-efi: rtc core: registered rtc-efi as rtc0
>>
>> There could be a difference in firmware and mem setup, though I also
>> downgraded the firmware to test it, but can't reproduce it anymore. I
>> could reliable trigger the crash the first time.
>>
>> FTR the oops.
>
> Hmm, I just can't help but think you were accidentally running with
> additional patches when you saw this oops previously. For example,
> your log looks very similar to this one:
>
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-December/473666.html
>
> but then again, these crashes probably often look alike.
>
These are quite different, in fact. In James's case, the UEFI memory
map was missing some entries, so not all memory regions that the
firmware expected to be there were actually mapped, hence the all-zero
*pte. In Robert's case, it looks like the UEFI runtime services page
tables are corrupted, i.e., *pte has RES0 bits set.
^ permalink raw reply
* [PATCH v3 2/3] DT: bingdings: power: reset: add linkstation-reset doc
From: Roger Shimizu @ 2017-01-06 12:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170103170952.7svgaa247ewlht7f@rob-hp-laptop>
Dear Rob,
Thanks for your comments!
On Wed, Jan 4, 2017 at 2:09 AM, Rob Herring <robh@kernel.org> wrote:
>
> This needs to model the uC connected to the UART rather than some node
> that defines only some portion of the functionality. I'm working on
> bindings and proper bus support for this[1], but it's not done yet.
> Though, the binding side is pretty simple.
>
> Rob
>
> [1] https://git.kernel.org/cgit/linux/kernel/git/robh/linux.git/log/?h=serial-bus-v2
Actually I have limited knowledge on DT binding.
I think it should be OK to remove the 2/3 patch (this thread) in my
next series, and I'll do my homework when you finish it.
Thank you!
Cheers,
--
Roger Shimizu, GMT +9 Tokyo
PGP/GPG: 4096R/6C6ACD6417B3ACB1
^ permalink raw reply
* [PATCH v3 9/9] arm64: Documentation - Expose CPU feature registers
From: Catalin Marinas @ 2017-01-06 12:16 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1483552147-9605-10-git-send-email-suzuki.poulose@arm.com>
On Wed, Jan 04, 2017 at 05:49:07PM +0000, Suzuki K. Poulose wrote:
> +The following rules are applied to the value returned by the
> +infrastructure:
> +
> + a) The value of an 'IMPLEMENTATION DEFINED' field is set to 0.
> + b) The value of a reserved field is populated with the reserved
> + value as defined by the architecture.
> + c) The value of a field marked as not 'visible', is set to indicate
> + the feature is missing (as defined by the architecture).
> + d) The value of a 'visible' field holds the system wide safe value
> + for the particular feature(except for MIDR_EL1, see section 4).
> + See Appendix I for more information on safe value.
> +
> +There are only a few registers visible to the userspace. See Section 4,
> +for the list of 'visible' registers.
> +
> +All others are emulated as having 'invisible' features.
BTW, we don't have any statement about whether a visible field may
become invisible but I guess this wouldn't be a problem as long as the
feature is reported as missing. I'm thinking about currently RES0 fields
that are listed as visible but they may report something in the future
that we don't want exposed to user. At that point, we'll change the
field to "invisible" while reporting RES0 to user. I don't see an issue
with this, just I thought worth flagging.
Anyway:
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox