Re: [PATCH v3 13/33] iommu/mediatek: Remove the power status checking in tlb flush all
On Mon, 2021-10-25 at 12:03 +0800, Yong Wu wrote: > On Fri, 2021-10-22 at 16:03 +0200, Dafna Hirschfeld wrote: > > Hi > > > > > > On 23.09.21 13:58, Yong Wu wrote: > > > To simplify the code, Remove the power status checking in the > > > tlb_flush_all, remove this: > > > if (pm_runtime_get_if_in_use(data->dev) <= 0) > > > continue; > > > > > > After this patch, the mtk_iommu_tlb_flush_all will be called from > > > a) isr > > > b) pm runtime resume callback > > > c) tlb flush range fail case > > > d) iommu_create_device_direct_mappings > > > -> iommu_flush_iotlb_all > > > In first three cases, the power and clock always are enabled; d) > > > is > > > direct > > > > Regarding case "c) tlb flush range fail case", I found out that > > this > > often happens when the iommu is used while it is runtime > > suspended. > > Which SoC and branch are you testing on? > > > For example the mtk-vcodec encoder driver calls > > "pm_runtime_resume_and_get" only when it starts > > streaming but > > buffers allocation is done in 'v4l2_reqbufs' before > > "pm_runtime_resume_and_get" is called > > This is the case I tried to fix in [14/33]. > pm_runtime_get_if_in_use should return when the iommu device's pm is > not active when vcodec allocate buffer before pm_runtime_resume_and > get. > > Do you have the devicelink patchset in your branch? if not, the > vcodec > should call mtk_smi_larb_get to enable the power/clock for larbs, > then > the iommu's device is active via devicelink between smi and iommu > device. > > > and then I see the warning "Partial TLB flush timed out, falling > > back > > to full flush" > > I am not sure how to fix that issue, but it seems that case 'c)' Have your issue been fixed? or more information about it. Thanks. > > might indicate that > > power and clock are actually not enabled. > > > > > mapping, the tlb flush is unnecessay since we already have > > > tlb_flush_all > > > in the pm_runtime_resume callback. When the iommu's power status > > > is > > > changed to active, the tlb always is clean. > > > > > > In addition, there still are 2 reasons that don't add PM status > > > checking > > > in the tlb flush all: > > > a) Write tlb flush all register also is ok even though the HW has > > > no > > > power and clocks. Write ignore. > > > b) pm_runtime_get_if_in_use(m4udev) is 0 when the tlb_flush_all > > > is called frm pm_runtime_resume cb. From this point, we can not > > > add > > > this code above in this tlb_flush_all. > > > > > > Signed-off-by: Yong Wu > > > --- > > > drivers/iommu/mtk_iommu.c | 20 +++- > > > 1 file changed, 7 insertions(+), 13 deletions(-) ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2] dma-direct: improve DMA_ATTR_NO_KERNEL_MAPPING
When the allocated buffers use dma coherent memory with
DMA_ATTR_NO_KERNEL_MAPPING, then its kernel mapping is exist.
The caller use that DMA_ATTR_NO_KERNEL_MAPPING mean they can't
rely on kernel mapping, but removing kernel mapping have
some improvements.
The improvements are:
a) Security improvement. In some cases, we don't hope the allocated
buffer to be read by cpu speculative execution. Therefore, it
need to remove kernel mapping, this patch improve
DMA_ATTR_NO_KERNEL_MAPPING to remove a page from kernel mapping
in order that cpu doesn't read it.
b) Debugging improvement. If the allocated buffer map into user space,
only access it in user space, nobody can access it in kernel space,
so we can use this patch to see who try to access it in kernel space.
This patch only works if the memory is mapping at page granularity
in the linear region, so that current only support for ARM64.
Signed-off-by: Walter Wu
Suggested-by: Christoph Hellwig
Suggested-by: Ard Biesheuvel
Cc: Christoph Hellwig
Cc: Marek Szyprowski
Cc: Robin Murphy
Cc: Matthias Brugger
Cc: Ard Biesheuvel
Cc: Andrew Morton
---
v2:
1. modify commit message and fix the removing mapping for arm64
2. fix build error for x86
---
include/linux/set_memory.h | 5 +
kernel/dma/direct.c| 13 +
2 files changed, 18 insertions(+)
diff --git a/include/linux/set_memory.h b/include/linux/set_memory.h
index f36be5166c19..6c7d1683339c 100644
--- a/include/linux/set_memory.h
+++ b/include/linux/set_memory.h
@@ -7,11 +7,16 @@
#ifdef CONFIG_ARCH_HAS_SET_MEMORY
#include
+
+#ifndef CONFIG_RODATA_FULL_DEFAULT_ENABLED
+static inline int set_memory_valid(unsigned long addr, int numpages, int
enable) { return 0; }
+#endif
#else
static inline int set_memory_ro(unsigned long addr, int numpages) { return 0; }
static inline int set_memory_rw(unsigned long addr, int numpages) { return 0; }
static inline int set_memory_x(unsigned long addr, int numpages) { return 0; }
static inline int set_memory_nx(unsigned long addr, int numpages) { return 0; }
+static inline int set_memory_valid(unsigned long addr, int numpages, int
enable) { return 0; }
#endif
#ifndef CONFIG_ARCH_HAS_SET_DIRECT_MAP
diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
index 4c6c5e0635e3..d5d03b51b708 100644
--- a/kernel/dma/direct.c
+++ b/kernel/dma/direct.c
@@ -155,6 +155,7 @@ void *dma_direct_alloc(struct device *dev, size_t size,
struct page *page;
void *ret;
int err;
+ unsigned long kaddr;
size = PAGE_ALIGN(size);
if (attrs & DMA_ATTR_NO_WARN)
@@ -169,6 +170,11 @@ void *dma_direct_alloc(struct device *dev, size_t size,
if (!PageHighMem(page))
arch_dma_prep_coherent(page, size);
*dma_handle = phys_to_dma_direct(dev, page_to_phys(page));
+ if (IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED)) {
+ kaddr = (unsigned long)phys_to_virt(dma_to_phys(dev,
*dma_handle));
+ /* page remove kernel mapping for arm64 */
+ set_memory_valid(kaddr, size >> PAGE_SHIFT, 0);
+ }
/* return the page pointer as the opaque cookie */
return page;
}
@@ -275,9 +281,16 @@ void dma_direct_free(struct device *dev, size_t size,
void *cpu_addr, dma_addr_t dma_addr, unsigned long attrs)
{
unsigned int page_order = get_order(size);
+ unsigned long kaddr;
if ((attrs & DMA_ATTR_NO_KERNEL_MAPPING) &&
!force_dma_unencrypted(dev) && !is_swiotlb_for_alloc(dev)) {
+ if (IS_ENABLED(CONFIG_RODATA_FULL_DEFAULT_ENABLED)) {
+ size = PAGE_ALIGN(size);
+ kaddr = (unsigned long)phys_to_virt(dma_to_phys(dev,
dma_addr));
+ /* page create kernel mapping for arm64 */
+ set_memory_valid(kaddr, size >> PAGE_SHIFT, 1);
+ }
/* cpu_addr is a struct page cookie, not a kernel address */
dma_free_contiguous(dev, cpu_addr, size);
return;
--
2.18.0
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC 02/20] vfio: Add device class for /dev/vfio/devices
On Tue, Nov 02, 2021 at 09:53:29AM +, Liu, Yi L wrote:
> > vfio_uninit_group_dev(_state->vdev);
> > kfree(mdev_state->pages);
> > kfree(mdev_state->vconfig);
> > kfree(mdev_state);
> >
> > pages/vconfig would logically be in a release function
>
> I see. So the criteria is: the pointer fields pointing to a memory buffer
> allocated by the device driver should be logically be free in a release
> function. right?
Often yes, that is usually a good idea
>I can see there are such fields in struct vfio_pci_core_device
> and mdev_state (both mbochs and mdpy). So we may go with your option #2.
> Is it? otherwise, needs to add release callback for all the related drivers.
Yes, that is the approx trade off
> > On the other hand ccw needs to rcu free the vfio_device, so that would
> > have to be global overhead with this api design.
>
> not quite get. why ccw is special here? could you elaborate?
I added a rcu usage to it in order to fix a race
+static inline struct vfio_ccw_private *vfio_ccw_get_priv(struct subchannel
*sch)
+{
+ struct vfio_ccw_private *private;
+
+ rcu_read_lock();
+ private = dev_get_drvdata(>dev);
+ if (private && !vfio_device_try_get(>vdev))
+ private = NULL;
+ rcu_read_unlock();
+ return private;
+}
Jason
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
