On 23-03-2024 01:07, Rodrigo Vivi wrote: > On Fri, Mar 22, 2024 at 03:00:17PM +0530, Ghimiray, Himal Prasad wrote: >> On 21-03-2024 02:27, Rodrigo Vivi wrote: >> >> On Wed, Mar 20, 2024 at 03:48:35PM +0530, Himal Prasad Ghimiray wrote: >> >> >Cast to proper datatypes to avoid overflows. >> >> I'm afraid that the cast wont prevent the overflow, but mask it. >> probably safer to move the multiplication to some of the helpers >> in linux/math64.h ?! >> >> Hi Rodrigo, >> Thank you for your response. The modifications are inspired by the inline >> u64 mul_u32_u32(u32 a, u32 b) >> function defined in linux/math64.h. Initially, I considered using the same >> approach. >> However, I discovered an architecture-specific implementation for >> mul_u32_u32. >> To prevent ambiguity, I opted for casting, which I observed is a standard >> practice throughout Linux code. > afaik the cast just tells compilers and static analyzer tools that we know > what we are doing and we know that that math on the right won't exceed > the cast size. But it doesn't prevent overflow. You need to take care > and be sure that you are not overflowing the bits you have available. Always. Apologies for the oversight. I realize now that I should have been clearer in the commit message regarding the specific overflows I aimed to address. In this context, we are performing multiplication of two 32-bit operands and expecting the result as a |u64|. However, not all compilers or platforms are guaranteed to use a |u64| for the intermediate result of multiplication in this scenario. Some might opt for a |u32| for storing the intermediate result of multiplication before widening it to |u64|. By explicitly casting one of the operands to |u64|, we ensure that the multiplication will be carried out with a |u64| as the intermediate result. This casting mitigates the risk of overflow that might occur due to the multiplication of two lower precision (|u32|) operands before widening the result to higher precision (|u64|). This patch does not aim to address overflow if the result itself overflows |u64|, but rather focuses on addressing overflow that might occur due to the multiplication of two lower precision (|u32|) operands before widening the result to higher precision (|u64|). > > I doubled checked all the cases below and I'm sure we don't have any > overflow issue there. So you need to at least adjust the commit message. Will modify the commit message to: "Addressing potential overflow in result of  multiplication of two lower precision (|u32|) operands  before widening it to higher precision (|u64|)." > > > Btw, (map_ofs / XE_PAGE_SIZE - NUM_KERNEL_PDE) probably deserves a separate > patch to make it to use some of the variants of DIV_ROUND macros... To me this looks unnecessary.  But if you feel it is good to have or required it can be done. BR Himal Ghimiray > > >> BR >> Himal >> >> >> >> >Cc: Matthew Auld [1] >> >Cc: Matthew Brost [2] >> >Cc: Rodrigo Vivi [3] >> >Signed-off-by: Himal Prasad Ghimiray [4] >> >--- >> >These errors were highlighted by Coverity. I'm uncertain whether they >> >require attention or if it would be more appropriate to label them as >> >false positives within the tool. >> >> >I've submitted this patch in case addressing the issues is necessary. >> >However, if reviewers determine that these issues should be marked as >> >false positives or ignored within the tool, that option is also >> >available >> >> > drivers/gpu/drm/xe/xe_migrate.c | 8 ++++---- >> > 1 file changed, 4 insertions(+), 4 deletions(-) >> >> >diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c >> >index ee1bb938c493..2ba4fb9511f6 100644 >> >--- a/drivers/gpu/drm/xe/xe_migrate.c >> >+++ b/drivers/gpu/drm/xe/xe_migrate.c >> >@@ -227,7 +227,7 @@ static int xe_migrate_prepare_vm(struct xe_tile *tile, struct xe_migrate *m, >> > if (vm->flags & XE_VM_FLAG_64K && level == 1) >> > flags = XE_PDE_64K; >> > >> >- entry = vm->pt_ops->pde_encode_bo(bo, map_ofs + (level - 1) * >> >+ entry = vm->pt_ops->pde_encode_bo(bo, map_ofs + (u64)(level - 1) * >> > XE_PAGE_SIZE, pat_index); >> > xe_map_wr(xe, &bo->vmap, map_ofs + XE_PAGE_SIZE * level, u64, >> > entry | flags); >> >@@ -235,7 +235,7 @@ static int xe_migrate_prepare_vm(struct xe_tile *tile, struct xe_migrate *m, >> > >> > /* Write PDE's that point to our BO. */ >> > for (i = 0; i < num_entries - num_level; i++) { >> >- entry = vm->pt_ops->pde_encode_bo(bo, i * XE_PAGE_SIZE, >> >+ entry = vm->pt_ops->pde_encode_bo(bo, (u64)i * XE_PAGE_SIZE, >> > pat_index); >> > >> > xe_map_wr(xe, &bo->vmap, map_ofs + XE_PAGE_SIZE + >> >@@ -291,7 +291,7 @@ static int xe_migrate_prepare_vm(struct xe_tile *tile, struct xe_migrate *m, >> > #define VM_SA_UPDATE_UNIT_SIZE (XE_PAGE_SIZE / NUM_VMUSA_UNIT_PER_PAGE) >> > #define NUM_VMUSA_WRITES_PER_UNIT (VM_SA_UPDATE_UNIT_SIZE / sizeof(u64)) >> > drm_suballoc_manager_init(&m->vm_update_sa, >> >- (map_ofs / XE_PAGE_SIZE - NUM_KERNEL_PDE) * >> >+ (size_t)(map_ofs / XE_PAGE_SIZE - NUM_KERNEL_PDE) * >> > NUM_VMUSA_UNIT_PER_PAGE, 0); >> > >> > m->pt_bo = bo; >> >@@ -490,7 +490,7 @@ static void emit_pte(struct xe_migrate *m, >> > struct xe_vm *vm = m->q->vm; >> > u16 pat_index; >> > u32 ptes; >> >- u64 ofs = at_pt * XE_PAGE_SIZE; >> >+ u64 ofs = (u64)at_pt * XE_PAGE_SIZE; >> > u64 cur_ofs; >> > >> > /* Indirect access needs compression enabled uncached PAT index */ >> >-- >> >2.25.1 >> >> References >> >> Visible links >> 1.mailto:matthew.auld@intel.com >> 2.mailto:matthew.brost@intel.com >> 3.mailto:rodrigo.vivi@intel.com >> 4.mailto:himal.prasad.ghimiray@intel.com