Re: [PATCH 03/10] accel/ivpu: Add debug prints for MMU map/unmap operations
On 05.01.2024 16:32, Jeffrey Hugo wrote: > On 1/5/2024 4:22 AM, Jacek Lawrynowicz wrote: >> From: "Wachowski, Karol" >> >> It is common need to be able to see IOVA/physical to VPU addresses > > Errant double space between "to" and "see" Sure >> mappings. Especially when debugging different kind of memory related >> issues. Lack of such logs forces user to modify and recompile KMD manually. >> >> This commit adds those logs under MMU debug mask which can be turned on >> dynamically with module param during KMD load. > As far as I understand, the preference is to not expose any kind of raw > addresses as it is seen as a security issue, and usually the addresses don't > have any real value to someone reading logs, etc. I beleive I picked this up > from GregKH. > > However, this commit text suggests there is value, and I see that one needs > to be root to enable this which could probably be considered a sufficent gate > to avoiding the data getting into the wrong hands. > > Is it possible to provide more details as a justification for this? Perhaps > an example of a past issue where this data was necessary for debug? There were multiple occasions were these messages were useful: - Debugging IOMMU issues on pre-production hardware - Enabling DevTLB cache on a functional simulator - Debugging performance issues with fragmented buffers There is always some security tradeoff when enabling debug features but in this case it seems to be worth it.
Re: [PATCH 03/10] accel/ivpu: Add debug prints for MMU map/unmap operations
On 1/5/2024 4:22 AM, Jacek Lawrynowicz wrote:
From: "Wachowski, Karol"
It is common need to be able to see IOVA/physical to VPU addresses
Errant double space between "to" and "see"
mappings. Especially when debugging different kind of memory related
issues. Lack of such logs forces user to modify and recompile KMD manually.
This commit adds those logs under MMU debug mask which can be turned on
dynamically with module param during KMD load.
As far as I understand, the preference is to not expose any kind of raw
addresses as it is seen as a security issue, and usually the addresses
don't have any real value to someone reading logs, etc. I beleive I
picked this up from GregKH.
However, this commit text suggests there is value, and I see that one
needs to be root to enable this which could probably be considered a
sufficent gate to avoiding the data getting into the wrong hands.
Is it possible to provide more details as a justification for this?
Perhaps an example of a past issue where this data was necessary for debug?
Signed-off-by: Wachowski, Karol
Signed-off-by: Jacek Lawrynowicz
---
drivers/accel/ivpu/ivpu_drv.h | 1 +
drivers/accel/ivpu/ivpu_mmu_context.c | 9 +
2 files changed, 10 insertions(+)
diff --git a/drivers/accel/ivpu/ivpu_drv.h b/drivers/accel/ivpu/ivpu_drv.h
index ebc4b84f27b2..9b6e336626e3 100644
--- a/drivers/accel/ivpu/ivpu_drv.h
+++ b/drivers/accel/ivpu/ivpu_drv.h
@@ -56,6 +56,7 @@
#define IVPU_DBG_JSM BIT(10)
#define IVPU_DBG_KREF BIT(11)
#define IVPU_DBG_RPM BIT(12)
+#define IVPU_DBG_MMU_MAP BIT(13)
#define ivpu_err(vdev, fmt, ...) \
drm_err(&(vdev)->drm, "%s(): " fmt, __func__, ##__VA_ARGS__)
diff --git a/drivers/accel/ivpu/ivpu_mmu_context.c
b/drivers/accel/ivpu/ivpu_mmu_context.c
index 12a8c09d4547..fe6161299236 100644
--- a/drivers/accel/ivpu/ivpu_mmu_context.c
+++ b/drivers/accel/ivpu/ivpu_mmu_context.c
@@ -355,6 +355,9 @@ ivpu_mmu_context_map_sgt(struct ivpu_device *vdev, struct
ivpu_mmu_context *ctx,
dma_addr_t dma_addr = sg_dma_address(sg) - sg->offset;
size_t size = sg_dma_len(sg) + sg->offset;
+ ivpu_dbg(vdev, MMU_MAP, "Map ctx: %u dma_addr: 0x%llx vpu_addr: 0x%llx size: %lu\n",
+ctx->id, dma_addr, vpu_addr, size);
+
ret = ivpu_mmu_context_map_pages(vdev, ctx, vpu_addr, dma_addr,
size, prot);
if (ret) {
ivpu_err(vdev, "Failed to map context pages\n");
@@ -366,6 +369,7 @@ ivpu_mmu_context_map_sgt(struct ivpu_device *vdev, struct
ivpu_mmu_context *ctx,
/* Ensure page table modifications are flushed from wc buffers to memory */
wmb();
+
This looks like an unrelated whitespace change (although I see it pairs
with the whitespace change below).
mutex_unlock(>lock);
ret = ivpu_mmu_invalidate_tlb(vdev, ctx->id);
@@ -388,14 +392,19 @@ ivpu_mmu_context_unmap_sgt(struct ivpu_device *vdev,
struct ivpu_mmu_context *ct
mutex_lock(>lock);
for_each_sgtable_dma_sg(sgt, sg, i) {
+ dma_addr_t dma_addr = sg_dma_address(sg) - sg->offset;
size_t size = sg_dma_len(sg) + sg->offset;
+ ivpu_dbg(vdev, MMU_MAP, "Unmap ctx: %u dma_addr: 0x%llx vpu_addr: 0x%llx size: %lu\n",
+ctx->id, dma_addr, vpu_addr, size);
+
ivpu_mmu_context_unmap_pages(ctx, vpu_addr, size);
vpu_addr += size;
}
/* Ensure page table modifications are flushed from wc buffers to memory */
wmb();
+
This looks like an unrelated whitespace change.
mutex_unlock(>lock);
ret = ivpu_mmu_invalidate_tlb(vdev, ctx->id);
[PATCH 03/10] accel/ivpu: Add debug prints for MMU map/unmap operations
From: "Wachowski, Karol"
It is common need to be able to see IOVA/physical to VPU addresses
mappings. Especially when debugging different kind of memory related
issues. Lack of such logs forces user to modify and recompile KMD manually.
This commit adds those logs under MMU debug mask which can be turned on
dynamically with module param during KMD load.
Signed-off-by: Wachowski, Karol
Signed-off-by: Jacek Lawrynowicz
---
drivers/accel/ivpu/ivpu_drv.h | 1 +
drivers/accel/ivpu/ivpu_mmu_context.c | 9 +
2 files changed, 10 insertions(+)
diff --git a/drivers/accel/ivpu/ivpu_drv.h b/drivers/accel/ivpu/ivpu_drv.h
index ebc4b84f27b2..9b6e336626e3 100644
--- a/drivers/accel/ivpu/ivpu_drv.h
+++ b/drivers/accel/ivpu/ivpu_drv.h
@@ -56,6 +56,7 @@
#define IVPU_DBG_JSMBIT(10)
#define IVPU_DBG_KREF BIT(11)
#define IVPU_DBG_RPMBIT(12)
+#define IVPU_DBG_MMU_MAP BIT(13)
#define ivpu_err(vdev, fmt, ...) \
drm_err(&(vdev)->drm, "%s(): " fmt, __func__, ##__VA_ARGS__)
diff --git a/drivers/accel/ivpu/ivpu_mmu_context.c
b/drivers/accel/ivpu/ivpu_mmu_context.c
index 12a8c09d4547..fe6161299236 100644
--- a/drivers/accel/ivpu/ivpu_mmu_context.c
+++ b/drivers/accel/ivpu/ivpu_mmu_context.c
@@ -355,6 +355,9 @@ ivpu_mmu_context_map_sgt(struct ivpu_device *vdev, struct
ivpu_mmu_context *ctx,
dma_addr_t dma_addr = sg_dma_address(sg) - sg->offset;
size_t size = sg_dma_len(sg) + sg->offset;
+ ivpu_dbg(vdev, MMU_MAP, "Map ctx: %u dma_addr: 0x%llx vpu_addr:
0x%llx size: %lu\n",
+ctx->id, dma_addr, vpu_addr, size);
+
ret = ivpu_mmu_context_map_pages(vdev, ctx, vpu_addr, dma_addr,
size, prot);
if (ret) {
ivpu_err(vdev, "Failed to map context pages\n");
@@ -366,6 +369,7 @@ ivpu_mmu_context_map_sgt(struct ivpu_device *vdev, struct
ivpu_mmu_context *ctx,
/* Ensure page table modifications are flushed from wc buffers to
memory */
wmb();
+
mutex_unlock(>lock);
ret = ivpu_mmu_invalidate_tlb(vdev, ctx->id);
@@ -388,14 +392,19 @@ ivpu_mmu_context_unmap_sgt(struct ivpu_device *vdev,
struct ivpu_mmu_context *ct
mutex_lock(>lock);
for_each_sgtable_dma_sg(sgt, sg, i) {
+ dma_addr_t dma_addr = sg_dma_address(sg) - sg->offset;
size_t size = sg_dma_len(sg) + sg->offset;
+ ivpu_dbg(vdev, MMU_MAP, "Unmap ctx: %u dma_addr: 0x%llx
vpu_addr: 0x%llx size: %lu\n",
+ctx->id, dma_addr, vpu_addr, size);
+
ivpu_mmu_context_unmap_pages(ctx, vpu_addr, size);
vpu_addr += size;
}
/* Ensure page table modifications are flushed from wc buffers to
memory */
wmb();
+
mutex_unlock(>lock);
ret = ivpu_mmu_invalidate_tlb(vdev, ctx->id);
--
2.43.0
