On 30/03/2023 16:34, Luben Tuikov wrote:
On 2023-03-29 11:47, Shashank Sharma wrote:This patch: - updates start/end values for each of the doorbell object created. - adds a function which validates that the kernel doorbell read/write is within this range. - uses this function during doorbell writes from kernel. Cc: Alex Deucher <alexander.deucher@xxxxxxx> Cc: Christian Koenig <christian.koenig@xxxxxxx> Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxx> Signed-off-by: Arvind Yadav <arvind.yadav@xxxxxxx> --- .../gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c | 29 ++++++++++++++++--- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c index b46fe8b1378d..81713b2c28e1 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell_mgr.c @@ -22,6 +22,25 @@ */#include "amdgpu.h"+#include "kfd_priv.h" + +static inline +bool amdgpu_doorbell_valid(struct amdgpu_device *adev, u32 index) +{ + if (index >= adev->doorbell.kernel_doorbells.start && + index < adev->doorbell.kernel_doorbells.end) + return true; + + if (index >= adev->mes.kernel_doorbells.start && + index < adev->mes.kernel_doorbells.end) + return true; + + if (index >= adev->kfd.dev->kernel_doorbells.start && + index < adev->kfd.dev->kernel_doorbells.end) + return true; + + return false; +}Here you're excluding "end". In patch 7 we see this:+ /* Last index in this object */ + uint32_t end;Which implies that "end" is included, but in this patch, the code excludes it. Perhaps you intended to use "index <= ...end" here?
No, this is intended, same as array object calculation. end = start + size; max = start + size - 1 so (< end) not (<= end) end says last index in this doorbell range; - Shashank
Since this isn't RTL, wouldn't it be better to describe the doorbell instance, with a "start" and "size"? This is traditionally used in memory management, and it makes comparisons and checks easy. Regards, Luben/*** amdgpu_mm_rdoorbell - read a doorbell dword @@ -37,7 +56,7 @@ u32 amdgpu_mm_rdoorbell(struct amdgpu_device *adev, u32 index) if (amdgpu_device_skip_hw_access(adev)) return 0;- if (index < adev->doorbell.num_kernel_doorbells) {+ if (amdgpu_doorbell_valid(adev, index)) { return readl(adev->doorbell.ptr + index); } else { DRM_ERROR("reading beyond doorbell aperture: 0x%08x!\n", index); @@ -60,7 +79,7 @@ void amdgpu_mm_wdoorbell(struct amdgpu_device *adev, u32 index, u32 v) if (amdgpu_device_skip_hw_access(adev)) return;- if (index < adev->doorbell.num_kernel_doorbells) {+ if (amdgpu_doorbell_valid(adev, index)) { writel(v, adev->doorbell.ptr + index); } else { DRM_ERROR("writing beyond doorbell aperture: 0x%08x!\n", index); @@ -81,7 +100,7 @@ u64 amdgpu_mm_rdoorbell64(struct amdgpu_device *adev, u32 index) if (amdgpu_device_skip_hw_access(adev)) return 0;- if (index < adev->doorbell.num_kernel_doorbells) {+ if (amdgpu_doorbell_valid(adev, index)) { return atomic64_read((atomic64_t *)(adev->doorbell.ptr + index)); } else { DRM_ERROR("reading beyond doorbell aperture: 0x%08x!\n", index); @@ -104,7 +123,7 @@ void amdgpu_mm_wdoorbell64(struct amdgpu_device *adev, u32 index, u64 v) if (amdgpu_device_skip_hw_access(adev)) return;- if (index < adev->doorbell.num_kernel_doorbells) {+ if (amdgpu_doorbell_valid(adev, index)) { atomic64_set((atomic64_t *)(adev->doorbell.ptr + index), v); } else { DRM_ERROR("writing beyond doorbell aperture: 0x%08x!\n", index); @@ -157,6 +176,8 @@ int amdgpu_doorbell_alloc_page(struct amdgpu_device *adev, return r; }+ db_obj->start = amdgpu_doorbell_index_on_bar(adev, db_obj->bo, 0);+ db_obj->end = db_obj->start + db_obj->size / sizeof(u32); return 0; }
