Few Qualcomm platforms such as, sdm845 have an additional outer
cache called as System cache, aka. Last level cache (LLC) that
allows non-coherent devices to upgrade to using caching.
This cache sits right before the DDR, and is tightly coupled
with the memory controller. The clients using this cache request
their slices from this system cache, make it active, and can then
start using it.
There is a fundamental assumption that non-coherent devices can't
access caches. This change adds an exception where they *can* use
some level of cache despite still being non-coherent overall.
The coherent devices that use cacheable memory, and CPU make use of
this system cache by default.
Looking at memory types, we have following -
a) Normal uncached :- MAIR 0x44, inner non-cacheable,
outer non-cacheable;
b) Normal cached :- MAIR 0xff, inner read write-back non-transient,
outer read write-back non-transient;
attribute setting for coherenet I/O devices.
and, for non-coherent i/o devices that can allocate in system cache
another type gets added -
c) Normal sys-cached :- MAIR 0xf4, inner non-cacheable,
outer read write-back non-transient
Coherent I/O devices use system cache by marking the memory as
normal cached.
Non-coherent I/O devices should mark the memory as normal
sys-cached in page tables to use system cache.
Signed-off-by: Vivek Gautam <[email protected]>
---
V3 version of this patch and related series can be found at [1].
This change is a realisation of following changes from downstream msm-4.9:
iommu: io-pgtable-arm: Implement IOMMU_USE_UPSTREAM_HINT[2]
Changes since v3:
- Dropping support to cache i/o page tables to system cache. Getting support
for data buffers is the first step.
Removed io-pgtable quirk and related change to add domain attribute.
Glmark2 numbers on SDM845 based cheza board:
S.No.| with LLC support | without LLC support
| for data buffers |
---------------------------------------------------
1 | 4480; 72.3fps | 4042; 65.2fps
2 | 4500; 72.6fps | 4039; 65.1fps
3 | 4523; 72.9fps | 4106; 66.2fps
4 | 4489; 72.4fps | 4104; 66.2fps
5 | 4518; 72.9fps | 4072; 65.7fps
[1] https://patchwork.kernel.org/cover/10772629/
[2] https://source.codeaurora.org/quic/la/kernel/msm-4.9/commit/?h=msm-4.9&id=d4c72c413ea27c43f60825193d4de9cb8ffd9602
drivers/iommu/io-pgtable-arm.c | 9 ++++++++-
include/linux/iommu.h | 1 +
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index d3700ec15cbd..2dbafe697531 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -167,10 +167,12 @@
#define ARM_LPAE_MAIR_ATTR_MASK 0xff
#define ARM_LPAE_MAIR_ATTR_DEVICE 0x04
#define ARM_LPAE_MAIR_ATTR_NC 0x44
+#define ARM_LPAE_MAIR_ATTR_QCOM_SYS_CACHE 0xf4
#define ARM_LPAE_MAIR_ATTR_WBRWA 0xff
#define ARM_LPAE_MAIR_ATTR_IDX_NC 0
#define ARM_LPAE_MAIR_ATTR_IDX_CACHE 1
#define ARM_LPAE_MAIR_ATTR_IDX_DEV 2
+#define ARM_LPAE_MAIR_ATTR_IDX_QCOM_SYS_CACHE 3
/* IOPTE accessors */
#define iopte_deref(pte,d) __va(iopte_to_paddr(pte, d))
@@ -442,6 +444,9 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
else if (prot & IOMMU_CACHE)
pte |= (ARM_LPAE_MAIR_ATTR_IDX_CACHE
<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
+ else if (prot & IOMMU_QCOM_SYS_CACHE)
+ pte |= (ARM_LPAE_MAIR_ATTR_IDX_QCOM_SYS_CACHE
+ << ARM_LPAE_PTE_ATTRINDX_SHIFT);
} else {
pte = ARM_LPAE_PTE_HAP_FAULT;
if (prot & IOMMU_READ)
@@ -841,7 +846,9 @@ arm_64_lpae_alloc_pgtable_s1(struct io_pgtable_cfg *cfg, void *cookie)
(ARM_LPAE_MAIR_ATTR_WBRWA
<< ARM_LPAE_MAIR_ATTR_SHIFT(ARM_LPAE_MAIR_ATTR_IDX_CACHE)) |
(ARM_LPAE_MAIR_ATTR_DEVICE
- << ARM_LPAE_MAIR_ATTR_SHIFT(ARM_LPAE_MAIR_ATTR_IDX_DEV));
+ << ARM_LPAE_MAIR_ATTR_SHIFT(ARM_LPAE_MAIR_ATTR_IDX_DEV)) |
+ (ARM_LPAE_MAIR_ATTR_QCOM_SYS_CACHE
+ << ARM_LPAE_MAIR_ATTR_SHIFT(ARM_LPAE_MAIR_ATTR_IDX_QCOM_SYS_CACHE));
cfg->arm_lpae_s1_cfg.mair[0] = reg;
cfg->arm_lpae_s1_cfg.mair[1] = 0;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index a815cf6f6f47..29dd2c624348 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -31,6 +31,7 @@
#define IOMMU_CACHE (1 << 2) /* DMA cache coherency */
#define IOMMU_NOEXEC (1 << 3)
#define IOMMU_MMIO (1 << 4) /* e.g. things like MSI doorbells */
+#define IOMMU_QCOM_SYS_CACHE (1 << 6)
/*
* Where the bus hardware includes a privilege level as part of its access type
* markings, and certain devices are capable of issuing transactions marked as
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
On 13/05/2019 11:04, Vivek Gautam wrote:
> Few Qualcomm platforms such as, sdm845 have an additional outer
> cache called as System cache, aka. Last level cache (LLC) that
> allows non-coherent devices to upgrade to using caching.
> This cache sits right before the DDR, and is tightly coupled
> with the memory controller. The clients using this cache request
> their slices from this system cache, make it active, and can then
> start using it.
>
> There is a fundamental assumption that non-coherent devices can't
> access caches. This change adds an exception where they *can* use
> some level of cache despite still being non-coherent overall.
> The coherent devices that use cacheable memory, and CPU make use of
> this system cache by default.
>
> Looking at memory types, we have following -
> a) Normal uncached :- MAIR 0x44, inner non-cacheable,
> outer non-cacheable;
> b) Normal cached :- MAIR 0xff, inner read write-back non-transient,
> outer read write-back non-transient;
> attribute setting for coherenet I/O devices.
> and, for non-coherent i/o devices that can allocate in system cache
> another type gets added -
> c) Normal sys-cached :- MAIR 0xf4, inner non-cacheable,
> outer read write-back non-transient
>
> Coherent I/O devices use system cache by marking the memory as
> normal cached.
> Non-coherent I/O devices should mark the memory as normal
> sys-cached in page tables to use system cache.
>
> Signed-off-by: Vivek Gautam <[email protected]>
> ---
>
> V3 version of this patch and related series can be found at [1].
>
> This change is a realisation of following changes from downstream msm-4.9:
> iommu: io-pgtable-arm: Implement IOMMU_USE_UPSTREAM_HINT[2]
>
> Changes since v3:
> - Dropping support to cache i/o page tables to system cache. Getting support
> for data buffers is the first step.
> Removed io-pgtable quirk and related change to add domain attribute.
>
> Glmark2 numbers on SDM845 based cheza board:
>
> S.No.| with LLC support | without LLC support
> | for data buffers |
> ---------------------------------------------------
> 1 | 4480; 72.3fps | 4042; 65.2fps
> 2 | 4500; 72.6fps | 4039; 65.1fps
> 3 | 4523; 72.9fps | 4106; 66.2fps
> 4 | 4489; 72.4fps | 4104; 66.2fps
> 5 | 4518; 72.9fps | 4072; 65.7fps
>
> [1] https://patchwork.kernel.org/cover/10772629/
> [2] https://source.codeaurora.org/quic/la/kernel/msm-4.9/commit/?h=msm-4.9&id=d4c72c413ea27c43f60825193d4de9cb8ffd9602
>
> drivers/iommu/io-pgtable-arm.c | 9 ++++++++-
> include/linux/iommu.h | 1 +
> 2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index d3700ec15cbd..2dbafe697531 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -167,10 +167,12 @@
> #define ARM_LPAE_MAIR_ATTR_MASK 0xff
> #define ARM_LPAE_MAIR_ATTR_DEVICE 0x04
> #define ARM_LPAE_MAIR_ATTR_NC 0x44
> +#define ARM_LPAE_MAIR_ATTR_QCOM_SYS_CACHE 0xf4
> #define ARM_LPAE_MAIR_ATTR_WBRWA 0xff
> #define ARM_LPAE_MAIR_ATTR_IDX_NC 0
> #define ARM_LPAE_MAIR_ATTR_IDX_CACHE 1
> #define ARM_LPAE_MAIR_ATTR_IDX_DEV 2
> +#define ARM_LPAE_MAIR_ATTR_IDX_QCOM_SYS_CACHE 3
Here at the implementation level, I'd rather just call these what they
are, i.e. s/QCOM_SYS_CACHE/INC_OWBRWA/.
>
> /* IOPTE accessors */
> #define iopte_deref(pte,d) __va(iopte_to_paddr(pte, d))
> @@ -442,6 +444,9 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
> else if (prot & IOMMU_CACHE)
> pte |= (ARM_LPAE_MAIR_ATTR_IDX_CACHE
> << ARM_LPAE_PTE_ATTRINDX_SHIFT);
> + else if (prot & IOMMU_QCOM_SYS_CACHE)
Where in the call stack is this going to be decided? (I don't recall the
previous discussions ever really reaching a solid conclusion on how to
separate responsibilities).
> + pte |= (ARM_LPAE_MAIR_ATTR_IDX_QCOM_SYS_CACHE
> + << ARM_LPAE_PTE_ATTRINDX_SHIFT);
> } else {
> pte = ARM_LPAE_PTE_HAP_FAULT;
> if (prot & IOMMU_READ)
> @@ -841,7 +846,9 @@ arm_64_lpae_alloc_pgtable_s1(struct io_pgtable_cfg *cfg, void *cookie)
> (ARM_LPAE_MAIR_ATTR_WBRWA
> << ARM_LPAE_MAIR_ATTR_SHIFT(ARM_LPAE_MAIR_ATTR_IDX_CACHE)) |
> (ARM_LPAE_MAIR_ATTR_DEVICE
> - << ARM_LPAE_MAIR_ATTR_SHIFT(ARM_LPAE_MAIR_ATTR_IDX_DEV));
> + << ARM_LPAE_MAIR_ATTR_SHIFT(ARM_LPAE_MAIR_ATTR_IDX_DEV)) |
> + (ARM_LPAE_MAIR_ATTR_QCOM_SYS_CACHE
> + << ARM_LPAE_MAIR_ATTR_SHIFT(ARM_LPAE_MAIR_ATTR_IDX_QCOM_SYS_CACHE));
>
> cfg->arm_lpae_s1_cfg.mair[0] = reg;
> cfg->arm_lpae_s1_cfg.mair[1] = 0;
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index a815cf6f6f47..29dd2c624348 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -31,6 +31,7 @@
> #define IOMMU_CACHE (1 << 2) /* DMA cache coherency */
> #define IOMMU_NOEXEC (1 << 3)
> #define IOMMU_MMIO (1 << 4) /* e.g. things like MSI doorbells */
> +#define IOMMU_QCOM_SYS_CACHE (1 << 6)
Nit: 6 usually comes *after* 5 ;)
Plus although it's fairly self-evident that this value has *something*
to do with Qcom system caches and isn't as generic as, say, IOMMU_PRIV,
it probably still warrants some degree of comment.
Robin.
> /*
> * Where the bus hardware includes a privilege level as part of its access type
> * markings, and certain devices are capable of issuing transactions marked as
>
Hi Robin,
On Mon, May 13, 2019 at 5:02 PM Robin Murphy <[email protected]> wrote:
>
> On 13/05/2019 11:04, Vivek Gautam wrote:
> > Few Qualcomm platforms such as, sdm845 have an additional outer
> > cache called as System cache, aka. Last level cache (LLC) that
> > allows non-coherent devices to upgrade to using caching.
> > This cache sits right before the DDR, and is tightly coupled
> > with the memory controller. The clients using this cache request
> > their slices from this system cache, make it active, and can then
> > start using it.
> >
> > There is a fundamental assumption that non-coherent devices can't
> > access caches. This change adds an exception where they *can* use
> > some level of cache despite still being non-coherent overall.
> > The coherent devices that use cacheable memory, and CPU make use of
> > this system cache by default.
> >
> > Looking at memory types, we have following -
> > a) Normal uncached :- MAIR 0x44, inner non-cacheable,
> > outer non-cacheable;
> > b) Normal cached :- MAIR 0xff, inner read write-back non-transient,
> > outer read write-back non-transient;
> > attribute setting for coherenet I/O devices.
> > and, for non-coherent i/o devices that can allocate in system cache
> > another type gets added -
> > c) Normal sys-cached :- MAIR 0xf4, inner non-cacheable,
> > outer read write-back non-transient
> >
> > Coherent I/O devices use system cache by marking the memory as
> > normal cached.
> > Non-coherent I/O devices should mark the memory as normal
> > sys-cached in page tables to use system cache.
> >
> > Signed-off-by: Vivek Gautam <[email protected]>
> > ---
> >
> > V3 version of this patch and related series can be found at [1].
> >
> > This change is a realisation of following changes from downstream msm-4.9:
> > iommu: io-pgtable-arm: Implement IOMMU_USE_UPSTREAM_HINT[2]
> >
> > Changes since v3:
> > - Dropping support to cache i/o page tables to system cache. Getting support
> > for data buffers is the first step.
> > Removed io-pgtable quirk and related change to add domain attribute.
> >
> > Glmark2 numbers on SDM845 based cheza board:
> >
> > S.No.| with LLC support | without LLC support
> > | for data buffers |
> > ---------------------------------------------------
> > 1 | 4480; 72.3fps | 4042; 65.2fps
> > 2 | 4500; 72.6fps | 4039; 65.1fps
> > 3 | 4523; 72.9fps | 4106; 66.2fps
> > 4 | 4489; 72.4fps | 4104; 66.2fps
> > 5 | 4518; 72.9fps | 4072; 65.7fps
> >
> > [1] https://patchwork.kernel.org/cover/10772629/
> > [2] https://source.codeaurora.org/quic/la/kernel/msm-4.9/commit/?h=msm-4.9&id=d4c72c413ea27c43f60825193d4de9cb8ffd9602
> >
> > drivers/iommu/io-pgtable-arm.c | 9 ++++++++-
> > include/linux/iommu.h | 1 +
> > 2 files changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> > index d3700ec15cbd..2dbafe697531 100644
> > --- a/drivers/iommu/io-pgtable-arm.c
> > +++ b/drivers/iommu/io-pgtable-arm.c
> > @@ -167,10 +167,12 @@
> > #define ARM_LPAE_MAIR_ATTR_MASK 0xff
> > #define ARM_LPAE_MAIR_ATTR_DEVICE 0x04
> > #define ARM_LPAE_MAIR_ATTR_NC 0x44
> > +#define ARM_LPAE_MAIR_ATTR_QCOM_SYS_CACHE 0xf4
> > #define ARM_LPAE_MAIR_ATTR_WBRWA 0xff
> > #define ARM_LPAE_MAIR_ATTR_IDX_NC 0
> > #define ARM_LPAE_MAIR_ATTR_IDX_CACHE 1
> > #define ARM_LPAE_MAIR_ATTR_IDX_DEV 2
> > +#define ARM_LPAE_MAIR_ATTR_IDX_QCOM_SYS_CACHE 3
>
> Here at the implementation level, I'd rather just call these what they
> are, i.e. s/QCOM_SYS_CACHE/INC_OWBRWA/.
>
Thanks for the review.
Sure, will change this as suggested.
> >
> > /* IOPTE accessors */
> > #define iopte_deref(pte,d) __va(iopte_to_paddr(pte, d))
> > @@ -442,6 +444,9 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
> > else if (prot & IOMMU_CACHE)
> > pte |= (ARM_LPAE_MAIR_ATTR_IDX_CACHE
> > << ARM_LPAE_PTE_ATTRINDX_SHIFT);
> > + else if (prot & IOMMU_QCOM_SYS_CACHE)
>
> Where in the call stack is this going to be decided? (I don't recall the
> previous discussions ever really reaching a solid conclusion on how to
> separate responsibilities).
>
Based on the last discussion [1], I understood that we may not want to expose
these cache protections to DMA APIs. So such control would lie with the masters
that are creating the individual domains. An example [2] of this is
graphics on sdm845.
Please ignore the change in naming at [2] IOMMU_UPSTREAM_HINT in [2] is same as
IOMMU_QCOM_SYS_CACHE here.
At that point [1] I also pointed to the fact that video that uses DMA
APIs to handle
buffers too uses system cache on sdm845. In this case shouldn't we expose the
protection controls to DMA APIs? Or would you suggest that such devices get
iommu domains in the driver, and then update these protection flags?
[1] https://lkml.org/lkml/2018/12/4/790
[2] https://patchwork.kernel.org/patch/10302791/
> > + pte |= (ARM_LPAE_MAIR_ATTR_IDX_QCOM_SYS_CACHE
> > + << ARM_LPAE_PTE_ATTRINDX_SHIFT);
> > } else {
> > pte = ARM_LPAE_PTE_HAP_FAULT;
> > if (prot & IOMMU_READ)
> > @@ -841,7 +846,9 @@ arm_64_lpae_alloc_pgtable_s1(struct io_pgtable_cfg *cfg, void *cookie)
> > (ARM_LPAE_MAIR_ATTR_WBRWA
> > << ARM_LPAE_MAIR_ATTR_SHIFT(ARM_LPAE_MAIR_ATTR_IDX_CACHE)) |
> > (ARM_LPAE_MAIR_ATTR_DEVICE
> > - << ARM_LPAE_MAIR_ATTR_SHIFT(ARM_LPAE_MAIR_ATTR_IDX_DEV));
> > + << ARM_LPAE_MAIR_ATTR_SHIFT(ARM_LPAE_MAIR_ATTR_IDX_DEV)) |
> > + (ARM_LPAE_MAIR_ATTR_QCOM_SYS_CACHE
> > + << ARM_LPAE_MAIR_ATTR_SHIFT(ARM_LPAE_MAIR_ATTR_IDX_QCOM_SYS_CACHE));
> >
> > cfg->arm_lpae_s1_cfg.mair[0] = reg;
> > cfg->arm_lpae_s1_cfg.mair[1] = 0;
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index a815cf6f6f47..29dd2c624348 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -31,6 +31,7 @@
> > #define IOMMU_CACHE (1 << 2) /* DMA cache coherency */
> > #define IOMMU_NOEXEC (1 << 3)
> > #define IOMMU_MMIO (1 << 4) /* e.g. things like MSI doorbells */
> > +#define IOMMU_QCOM_SYS_CACHE (1 << 6)
>
> Nit: 6 usually comes *after* 5 ;)
Sorry, pasting mistake.
>
> Plus although it's fairly self-evident that this value has *something*
> to do with Qcom system caches and isn't as generic as, say, IOMMU_PRIV,
> it probably still warrants some degree of comment.
I will add the necessary comments.
Best regards
Vivek
>
> Robin.
>
> > /*
> > * Where the bus hardware includes a privilege level as part of its access type
> > * markings, and certain devices are capable of issuing transactions marked as
> >
> _______________________________________________
> iommu mailing list
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
On Tue, May 14, 2019 at 12:26 PM Vivek Gautam
<[email protected]> wrote:
>
> Hi Robin,
>
>
> On Mon, May 13, 2019 at 5:02 PM Robin Murphy <[email protected]> wrote:
> >
> > On 13/05/2019 11:04, Vivek Gautam wrote:
> > > Few Qualcomm platforms such as, sdm845 have an additional outer
> > > cache called as System cache, aka. Last level cache (LLC) that
> > > allows non-coherent devices to upgrade to using caching.
> > > This cache sits right before the DDR, and is tightly coupled
> > > with the memory controller. The clients using this cache request
> > > their slices from this system cache, make it active, and can then
> > > start using it.
> > >
> > > There is a fundamental assumption that non-coherent devices can't
> > > access caches. This change adds an exception where they *can* use
> > > some level of cache despite still being non-coherent overall.
> > > The coherent devices that use cacheable memory, and CPU make use of
> > > this system cache by default.
> > >
> > > Looking at memory types, we have following -
> > > a) Normal uncached :- MAIR 0x44, inner non-cacheable,
> > > outer non-cacheable;
> > > b) Normal cached :- MAIR 0xff, inner read write-back non-transient,
> > > outer read write-back non-transient;
> > > attribute setting for coherenet I/O devices.
> > > and, for non-coherent i/o devices that can allocate in system cache
> > > another type gets added -
> > > c) Normal sys-cached :- MAIR 0xf4, inner non-cacheable,
> > > outer read write-back non-transient
> > >
> > > Coherent I/O devices use system cache by marking the memory as
> > > normal cached.
> > > Non-coherent I/O devices should mark the memory as normal
> > > sys-cached in page tables to use system cache.
> > >
> > > Signed-off-by: Vivek Gautam <[email protected]>
> > > ---
> > >
> > > V3 version of this patch and related series can be found at [1].
> > >
> > > This change is a realisation of following changes from downstream msm-4.9:
> > > iommu: io-pgtable-arm: Implement IOMMU_USE_UPSTREAM_HINT[2]
> > >
> > > Changes since v3:
> > > - Dropping support to cache i/o page tables to system cache. Getting support
> > > for data buffers is the first step.
> > > Removed io-pgtable quirk and related change to add domain attribute.
> > >
> > > Glmark2 numbers on SDM845 based cheza board:
> > >
> > > S.No.| with LLC support | without LLC support
> > > | for data buffers |
> > > ---------------------------------------------------
> > > 1 | 4480; 72.3fps | 4042; 65.2fps
> > > 2 | 4500; 72.6fps | 4039; 65.1fps
> > > 3 | 4523; 72.9fps | 4106; 66.2fps
> > > 4 | 4489; 72.4fps | 4104; 66.2fps
> > > 5 | 4518; 72.9fps | 4072; 65.7fps
> > >
> > > [1] https://patchwork.kernel.org/cover/10772629/
> > > [2] https://source.codeaurora.org/quic/la/kernel/msm-4.9/commit/?h=msm-4.9&id=d4c72c413ea27c43f60825193d4de9cb8ffd9602
> > >
> > > drivers/iommu/io-pgtable-arm.c | 9 ++++++++-
> > > include/linux/iommu.h | 1 +
> > > 2 files changed, 9 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> > > index d3700ec15cbd..2dbafe697531 100644
> > > --- a/drivers/iommu/io-pgtable-arm.c
> > > +++ b/drivers/iommu/io-pgtable-arm.c
> > > @@ -167,10 +167,12 @@
> > > #define ARM_LPAE_MAIR_ATTR_MASK 0xff
> > > #define ARM_LPAE_MAIR_ATTR_DEVICE 0x04
> > > #define ARM_LPAE_MAIR_ATTR_NC 0x44
> > > +#define ARM_LPAE_MAIR_ATTR_QCOM_SYS_CACHE 0xf4
s/QCOM_SYS_CACHE/INC_OWBRWA/ looks more appropriate here.
> > > #define ARM_LPAE_MAIR_ATTR_WBRWA 0xff
> > > #define ARM_LPAE_MAIR_ATTR_IDX_NC 0
> > > #define ARM_LPAE_MAIR_ATTR_IDX_CACHE 1
> > > #define ARM_LPAE_MAIR_ATTR_IDX_DEV 2
> > > +#define ARM_LPAE_MAIR_ATTR_IDX_QCOM_SYS_CACHE 3
> >
> > Here at the implementation level, I'd rather just call these what they
> > are, i.e. s/QCOM_SYS_CACHE/INC_OWBRWA/.
Allow me to change this to - s/QCOM_SYS_CACHE/INC_OC, or
s/QCOM_SYS_CACHE/INC_OCACHE to go inline with IDX_NC and IDX_CACHE.
Sounds okay?
> >
>
> Thanks for the review.
> Sure, will change this as suggested.
>
> > >
> > > /* IOPTE accessors */
> > > #define iopte_deref(pte,d) __va(iopte_to_paddr(pte, d))
> > > @@ -442,6 +444,9 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
> > > else if (prot & IOMMU_CACHE)
> > > pte |= (ARM_LPAE_MAIR_ATTR_IDX_CACHE
> > > << ARM_LPAE_PTE_ATTRINDX_SHIFT);
> > > + else if (prot & IOMMU_QCOM_SYS_CACHE)
> >
> > Where in the call stack is this going to be decided? (I don't recall the
> > previous discussions ever really reaching a solid conclusion on how to
> > separate responsibilities).
> >
>
> Based on the last discussion [1], I understood that we may not want to expose
> these cache protections to DMA APIs. So such control would lie with the masters
> that are creating the individual domains. An example [2] of this is
> graphics on sdm845.
> Please ignore the change in naming at [2] IOMMU_UPSTREAM_HINT in [2] is same as
> IOMMU_QCOM_SYS_CACHE here.
>
> At that point [1] I also pointed to the fact that video that uses DMA
> APIs to handle
> buffers too uses system cache on sdm845. In this case shouldn't we expose the
> protection controls to DMA APIs? Or would you suggest that such devices get
> iommu domains in the driver, and then update these protection flags?
>
> [1] https://lkml.org/lkml/2018/12/4/790
> [2] https://patchwork.kernel.org/patch/10302791/
>
> > > + pte |= (ARM_LPAE_MAIR_ATTR_IDX_QCOM_SYS_CACHE
> > > + << ARM_LPAE_PTE_ATTRINDX_SHIFT);
> > > } else {
> > > pte = ARM_LPAE_PTE_HAP_FAULT;
> > > if (prot & IOMMU_READ)
> > > @@ -841,7 +846,9 @@ arm_64_lpae_alloc_pgtable_s1(struct io_pgtable_cfg *cfg, void *cookie)
> > > (ARM_LPAE_MAIR_ATTR_WBRWA
> > > << ARM_LPAE_MAIR_ATTR_SHIFT(ARM_LPAE_MAIR_ATTR_IDX_CACHE)) |
> > > (ARM_LPAE_MAIR_ATTR_DEVICE
> > > - << ARM_LPAE_MAIR_ATTR_SHIFT(ARM_LPAE_MAIR_ATTR_IDX_DEV));
> > > + << ARM_LPAE_MAIR_ATTR_SHIFT(ARM_LPAE_MAIR_ATTR_IDX_DEV)) |
> > > + (ARM_LPAE_MAIR_ATTR_QCOM_SYS_CACHE
> > > + << ARM_LPAE_MAIR_ATTR_SHIFT(ARM_LPAE_MAIR_ATTR_IDX_QCOM_SYS_CACHE));
> > >
> > > cfg->arm_lpae_s1_cfg.mair[0] = reg;
> > > cfg->arm_lpae_s1_cfg.mair[1] = 0;
> > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > > index a815cf6f6f47..29dd2c624348 100644
> > > --- a/include/linux/iommu.h
> > > +++ b/include/linux/iommu.h
> > > @@ -31,6 +31,7 @@
> > > #define IOMMU_CACHE (1 << 2) /* DMA cache coherency */
> > > #define IOMMU_NOEXEC (1 << 3)
> > > #define IOMMU_MMIO (1 << 4) /* e.g. things like MSI doorbells */
> > > +#define IOMMU_QCOM_SYS_CACHE (1 << 6)
> >
> > Nit: 6 usually comes *after* 5 ;)
>
> Sorry, pasting mistake.
>
> >
> > Plus although it's fairly self-evident that this value has *something*
> > to do with Qcom system caches and isn't as generic as, say, IOMMU_PRIV,
> > it probably still warrants some degree of comment.
>
> I will add the necessary comments.
>
> Best regards
> Vivek
> >
> > Robin.
> >
> > > /*
> > > * Where the bus hardware includes a privilege level as part of its access type
> > > * markings, and certain devices are capable of issuing transactions marked as
> > >
> > _______________________________________________
> > iommu mailing list
> > [email protected]
> > https://lists.linuxfoundation.org/mailman/listinfo/iommu
>
>
>
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation