Re: [RFC PATCH V2 2/2] iommu: add Unisoc iommu basic driver

2021-01-20 Thread Chunyan Zhang
On Wed, 20 Jan 2021 at 20:29, Robin Murphy  wrote:
>
> On 2021-01-20 11:40, Chunyan Zhang wrote:
> [...]
> >>> + pgt_base_iova = dom->pgt_va +
> >>> + ((iova - mdata->iova_start) >> SPRD_IOMMU_PAGE_SHIFT);
> >>> +
> >>> + spin_lock_irqsave(>pgtlock, flags);
> >>> + for (i = 0; i < page_num; i++) {
> >>> + pgt_base_iova[i] = pabase >> SPRD_IOMMU_PAGE_SHIFT;
> >>
> >> Out of curiosity, is the pagetable walker cache-coherent, or is this
> >> currently managing to work by pure chance and natural cache churn?
> >
> > ->iotlb_sync_map() was implemented in this driver, I guess that has
> > done what you say here?
>
> No, sync_map only ensures that the previous (invalid) PTE isn't held in
> the IOMMU's TLB. If pgt_va is a regular page allocation then you're
> writing the new PTE to normal kernel memory, with nothing to guarantee
> that write goes any further than the CPU's L1 cache. Thus either the
> IOMMU has capable of snooping the CPU caches in order to see the updated
> PTE value (rather than refetching the stale value from DRAM), or you're
> just incredibly lucky that by the time the IOMMU *does* go to fetch the
> PTE for that address, that updated cache line has already been evicted
> out to DRAM naturally.
>

Got it, thanks for the detailed explanation.
In order to make clear why this code can work, I made a test, and
found that if I wrote more than 1024 PTEs, the value would be updated
to DRAM immediately, otherwise the cache line seems not updated even
if I wrote 1023 PTEs.

> This is not an issue if you use the proper DMA allocator, since that
> will ensure you get a non-cacheable buffer if you need one.
>

I will switch to use dma_alloc_coherent().

Thanks again.

Chunyan

> Robin.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [RFC PATCH V2 2/2] iommu: add Unisoc iommu basic driver

2021-01-20 Thread Robin Murphy

On 2021-01-20 11:40, Chunyan Zhang wrote:
[...]

+ pgt_base_iova = dom->pgt_va +
+ ((iova - mdata->iova_start) >> SPRD_IOMMU_PAGE_SHIFT);
+
+ spin_lock_irqsave(>pgtlock, flags);
+ for (i = 0; i < page_num; i++) {
+ pgt_base_iova[i] = pabase >> SPRD_IOMMU_PAGE_SHIFT;


Out of curiosity, is the pagetable walker cache-coherent, or is this
currently managing to work by pure chance and natural cache churn?


->iotlb_sync_map() was implemented in this driver, I guess that has
done what you say here?


No, sync_map only ensures that the previous (invalid) PTE isn't held in 
the IOMMU's TLB. If pgt_va is a regular page allocation then you're 
writing the new PTE to normal kernel memory, with nothing to guarantee 
that write goes any further than the CPU's L1 cache. Thus either the 
IOMMU has capable of snooping the CPU caches in order to see the updated 
PTE value (rather than refetching the stale value from DRAM), or you're 
just incredibly lucky that by the time the IOMMU *does* go to fetch the 
PTE for that address, that updated cache line has already been evicted 
out to DRAM naturally.


This is not an issue if you use the proper DMA allocator, since that 
will ensure you get a non-cacheable buffer if you need one.


Robin.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [RFC PATCH V2 2/2] iommu: add Unisoc iommu basic driver

2021-01-20 Thread Chunyan Zhang
Hi Robin,

On Wed, 13 Jan 2021 at 03:10, Robin Murphy  wrote:
>
> On 2021-01-08 11:38, Chunyan Zhang wrote:
> > From: Chunyan Zhang 
> >
> > This patch only adds display iommu support, the driver was tested with sprd
> > dpu.
> >
> > The iommu support for others would be added once finished tests with those
> > devices, such as Image codec(jpeg) processor, a few signal processors,
> > including VSP(video), GSP(graphic), ISP(image), and camera CPP, etc.
> >
> > Signed-off-by: Chunyan Zhang 
> > ---
> >   drivers/iommu/Kconfig  |  12 +
> >   drivers/iommu/Makefile |   1 +
> >   drivers/iommu/sprd-iommu.c | 546 +
> >   3 files changed, 559 insertions(+)
> >   create mode 100644 drivers/iommu/sprd-iommu.c
> >
> > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> > index 192ef8f61310..3f8dcf070442 100644
> > --- a/drivers/iommu/Kconfig
> > +++ b/drivers/iommu/Kconfig
> > @@ -408,4 +408,16 @@ config VIRTIO_IOMMU
> >
> > Say Y here if you intend to run this kernel as a guest.
> >
> > +config SPRD_IOMMU
> > + tristate "Unisoc IOMMU Support"
> > + depends on ARCH_SPRD
>
> Any chance of COMPILE_TEST support too?

Yes, just forgot to add that.

>
> > + select IOMMU_API
> > + help
> > +   Support for IOMMU on Unisoc's SoCs on which multi-media subsystems
> > +   need IOMMU, such as DPU, Image codec(jpeg) processor, and a few
> > +   signal processors, including VSP(video), GSP(graphic), ISP(image), 
> > and
> > +   CPP, etc.
> > +
> > +   Say Y here if you want multi-media functions.
> > +
> >   endif # IOMMU_SUPPORT
> > diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
> > index 61bd30cd8369..5925b6af2123 100644
> > --- a/drivers/iommu/Makefile
> > +++ b/drivers/iommu/Makefile
> > @@ -28,3 +28,4 @@ obj-$(CONFIG_S390_IOMMU) += s390-iommu.o
> >   obj-$(CONFIG_HYPERV_IOMMU) += hyperv-iommu.o
> >   obj-$(CONFIG_VIRTIO_IOMMU) += virtio-iommu.o
> >   obj-$(CONFIG_IOMMU_SVA_LIB) += iommu-sva-lib.o
> > +obj-$(CONFIG_SPRD_IOMMU) += sprd-iommu.o
> > diff --git a/drivers/iommu/sprd-iommu.c b/drivers/iommu/sprd-iommu.c
> > new file mode 100644
> > index ..a112b4d3cc23
> > --- /dev/null
> > +++ b/drivers/iommu/sprd-iommu.c
> > @@ -0,0 +1,546 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Unisoc IOMMU driver
> > + *
> > + * Copyright (C) 2020 Unisoc, Inc.
> > + * Author: Chunyan Zhang 
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +/* SPRD IOMMU page is 4K size alignment */
> > +#define SPRD_IOMMU_PAGE_SHIFT12
> > +#define SPRD_IOMMU_PAGE_SIZE SZ_4K
> > +
> > +#define SPRD_IOMMU_REG_OFFSET0x800
> > +#define SPRD_EX_CFG  (SPRD_IOMMU_REG_OFFSET + 0x0)
> > +#define SPRD_IOMMU_VAOR_BYPASS   BIT(4)
> > +#define SPRD_IOMMU_GATE_EN   BIT(1)
> > +#define SPRD_IOMMU_ENBIT(0)
> > +#define SPRD_EX_UPDATE   (SPRD_IOMMU_REG_OFFSET + 0x4)
> > +#define SPRD_EX_FIRST_VPN(SPRD_IOMMU_REG_OFFSET + 0x8)
> > +#define SPRD_EX_VPN_RANGE(SPRD_IOMMU_REG_OFFSET + 0xc)
> > +#define SPRD_EX_FIRST_PPN(SPRD_IOMMU_REG_OFFSET + 0x10)
> > +#define SPRD_EX_DEFAULT_PPN  (SPRD_IOMMU_REG_OFFSET + 0x14)
> > +
> > +#define SPRD_IOMMU_VERSION   (SPRD_IOMMU_REG_OFFSET + 0x0)
> > +#define SPRD_VERSION_MASKGENMASK(15, 8)
> > +#define SPRD_VERSION_SHIFT   8
> > +#define SPRD_VAU_CFG (SPRD_IOMMU_REG_OFFSET + 0x4)
> > +#define SPRD_VAU_UPDATE  (SPRD_IOMMU_REG_OFFSET + 0x8)
> > +#define SPRD_VAU_AUTH_CFG(SPRD_IOMMU_REG_OFFSET + 0xc)
> > +#define SPRD_VAU_FIRST_PPN   (SPRD_IOMMU_REG_OFFSET + 0x10)
> > +#define SPRD_VAU_DEFAULT_PPN_RD  (SPRD_IOMMU_REG_OFFSET + 0x14)
> > +#define SPRD_VAU_DEFAULT_PPN_WR  (SPRD_IOMMU_REG_OFFSET + 0x18)
> > +#define SPRD_VAU_FIRST_VPN   (SPRD_IOMMU_REG_OFFSET + 0x1c)
> > +#define SPRD_VAU_VPN_RANGE   (SPRD_IOMMU_REG_OFFSET + 0x20)
> > +
> > +enum sprd_iommu_version {
> > + SPRD_IOMMU_EX,
> > + SPRD_IOMMU_VAU,
> > +};
> > +
> > +/*
> > + * struct sprd_iommu_match_data - sprd iommu configurations which serves
> > + * for different master devices
> > + *
> > + * @iova_start:  the first address that can be mapped
> > + * @iova_size:   the largest address range that can be mapped
> > + *
> > + * iova_start and iova_size are designed for debug purpose, that says 
> > different
> > + * masters use different ranges of virtual address.
> > + */
> > +struct sprd_iommu_match_data {
> > + unsigned long iova_start;
> > + unsigned long iova_size;
> > +};
> > +
> > +/*
> > + * struct sprd_iommu_device - high-level sprd iommu device representation,
> > + * including hardware information and configuration, also driver data, etc
> > + *
> > + * @mdata:   hardware configuration and information
> > + * @ver: sprd iommu device version
> > + * @prot_page:   protect page base address, data 

Re: [RFC PATCH V2 2/2] iommu: add Unisoc iommu basic driver

2021-01-12 Thread Robin Murphy

On 2021-01-08 11:38, Chunyan Zhang wrote:

From: Chunyan Zhang 

This patch only adds display iommu support, the driver was tested with sprd
dpu.

The iommu support for others would be added once finished tests with those
devices, such as Image codec(jpeg) processor, a few signal processors,
including VSP(video), GSP(graphic), ISP(image), and camera CPP, etc.

Signed-off-by: Chunyan Zhang 
---
  drivers/iommu/Kconfig  |  12 +
  drivers/iommu/Makefile |   1 +
  drivers/iommu/sprd-iommu.c | 546 +
  3 files changed, 559 insertions(+)
  create mode 100644 drivers/iommu/sprd-iommu.c

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 192ef8f61310..3f8dcf070442 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -408,4 +408,16 @@ config VIRTIO_IOMMU
  
  	  Say Y here if you intend to run this kernel as a guest.
  
+config SPRD_IOMMU

+   tristate "Unisoc IOMMU Support"
+   depends on ARCH_SPRD


Any chance of COMPILE_TEST support too?


+   select IOMMU_API
+   help
+ Support for IOMMU on Unisoc's SoCs on which multi-media subsystems
+ need IOMMU, such as DPU, Image codec(jpeg) processor, and a few
+ signal processors, including VSP(video), GSP(graphic), ISP(image), and
+ CPP, etc.
+
+ Say Y here if you want multi-media functions.
+
  endif # IOMMU_SUPPORT
diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index 61bd30cd8369..5925b6af2123 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -28,3 +28,4 @@ obj-$(CONFIG_S390_IOMMU) += s390-iommu.o
  obj-$(CONFIG_HYPERV_IOMMU) += hyperv-iommu.o
  obj-$(CONFIG_VIRTIO_IOMMU) += virtio-iommu.o
  obj-$(CONFIG_IOMMU_SVA_LIB) += iommu-sva-lib.o
+obj-$(CONFIG_SPRD_IOMMU) += sprd-iommu.o
diff --git a/drivers/iommu/sprd-iommu.c b/drivers/iommu/sprd-iommu.c
new file mode 100644
index ..a112b4d3cc23
--- /dev/null
+++ b/drivers/iommu/sprd-iommu.c
@@ -0,0 +1,546 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Unisoc IOMMU driver
+ *
+ * Copyright (C) 2020 Unisoc, Inc.
+ * Author: Chunyan Zhang 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/* SPRD IOMMU page is 4K size alignment */
+#define SPRD_IOMMU_PAGE_SHIFT  12
+#define SPRD_IOMMU_PAGE_SIZE   SZ_4K
+
+#define SPRD_IOMMU_REG_OFFSET  0x800
+#define SPRD_EX_CFG(SPRD_IOMMU_REG_OFFSET + 0x0)
+#define SPRD_IOMMU_VAOR_BYPASS BIT(4)
+#define SPRD_IOMMU_GATE_EN BIT(1)
+#define SPRD_IOMMU_EN  BIT(0)
+#define SPRD_EX_UPDATE (SPRD_IOMMU_REG_OFFSET + 0x4)
+#define SPRD_EX_FIRST_VPN  (SPRD_IOMMU_REG_OFFSET + 0x8)
+#define SPRD_EX_VPN_RANGE  (SPRD_IOMMU_REG_OFFSET + 0xc)
+#define SPRD_EX_FIRST_PPN  (SPRD_IOMMU_REG_OFFSET + 0x10)
+#define SPRD_EX_DEFAULT_PPN(SPRD_IOMMU_REG_OFFSET + 0x14)
+
+#define SPRD_IOMMU_VERSION (SPRD_IOMMU_REG_OFFSET + 0x0)
+#define SPRD_VERSION_MASK  GENMASK(15, 8)
+#define SPRD_VERSION_SHIFT 8
+#define SPRD_VAU_CFG   (SPRD_IOMMU_REG_OFFSET + 0x4)
+#define SPRD_VAU_UPDATE(SPRD_IOMMU_REG_OFFSET + 0x8)
+#define SPRD_VAU_AUTH_CFG  (SPRD_IOMMU_REG_OFFSET + 0xc)
+#define SPRD_VAU_FIRST_PPN (SPRD_IOMMU_REG_OFFSET + 0x10)
+#define SPRD_VAU_DEFAULT_PPN_RD(SPRD_IOMMU_REG_OFFSET + 0x14)
+#define SPRD_VAU_DEFAULT_PPN_WR(SPRD_IOMMU_REG_OFFSET + 0x18)
+#define SPRD_VAU_FIRST_VPN (SPRD_IOMMU_REG_OFFSET + 0x1c)
+#define SPRD_VAU_VPN_RANGE (SPRD_IOMMU_REG_OFFSET + 0x20)
+
+enum sprd_iommu_version {
+   SPRD_IOMMU_EX,
+   SPRD_IOMMU_VAU,
+};
+
+/*
+ * struct sprd_iommu_match_data - sprd iommu configurations which serves
+ *   for different master devices
+ *
+ * @iova_start:the first address that can be mapped
+ * @iova_size: the largest address range that can be mapped
+ *
+ * iova_start and iova_size are designed for debug purpose, that says different
+ * masters use different ranges of virtual address.
+ */
+struct sprd_iommu_match_data {
+   unsigned long iova_start;
+   unsigned long iova_size;
+};
+
+/*
+ * struct sprd_iommu_device - high-level sprd iommu device representation,
+ * including hardware information and configuration, also driver data, etc
+ *
+ * @mdata: hardware configuration and information
+ * @ver:   sprd iommu device version
+ * @prot_page: protect page base address, data would be written to here
+ * while translation fault
+ * @base:  mapped base address for accessing registers
+ * @dev:   pointer to basic device structure
+ * @iommu: IOMMU core representation
+ * @group: IOMMU group
+ */
+struct sprd_iommu_device {
+   const struct sprd_iommu_match_data *mdata;
+   enum sprd_iommu_version ver;
+   phys_addr_t prot_page;
+   void __iomem*base;
+   struct device   *dev;
+   struct iommu_device iommu;
+   struct iommu_group