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?
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;
> }
>
