Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [Medium] The refactoring inadvertently removes the SYSTEM memory fallback for Display Page Tables (DPTs) on discrete graphics (DGFX). Pre-existing issues: - [High] Integer truncation of the GEM object size leads to an undersized Display Page Table (DPT) allocation, allowing the display engine to read out-of-bounds physical memory. -- commit 45b977ae39cb9d3ad37f33363c230cce698f7958 Author: Maarten Lankhorst <[email protected]> drm/xe: Avoid using stolen memory for DPT. On systems with media GT, extra latency is added when accessing stolen memory when the GT is in MC6. Since we additionally aren't counting how much memory is used for stolen and we could in theory fill up the entire stolen area with DPT's, avoid using stolen and only use the default memory region. Additionally, this may also result in random system hangs under load. Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/work_items/7513 > diff --git a/drivers/gpu/drm/xe/display/xe_fb_pin.c > b/drivers/gpu/drm/xe/display/xe_fb_pin.c > index f93c98bec5b5f..86013ef532fd7 100644 > --- a/drivers/gpu/drm/xe/display/xe_fb_pin.c > +++ b/drivers/gpu/drm/xe/display/xe_fb_pin.c > @@ -146,8 +146,8 @@ static int __xe_pin_fb_vma_dpt(struct drm_gem_object *obj, > const struct intel_fb_pin_params *pin_params, > struct i915_vma *vma) > { > struct xe_device *xe = to_xe_device(obj->dev); > struct xe_tile *tile0 = xe_device_get_root_tile(xe); > struct xe_ggtt *ggtt = tile0->mem.ggtt; > const struct i915_gtt_view *view = pin_params->view; > struct xe_bo *bo = gem_to_xe_bo(obj), *dpt; > u32 dpt_size, size = bo->ttm.base.size; [Severity: High] This is a pre-existing issue, but does assigning the 64-bit bo->ttm.base.size to a 32-bit size variable in __xe_pin_fb_vma_dpt() cause integer truncation? If a BO larger than 4GB is allocated by a userspace attacker and used as a normal framebuffer, the size could be truncated (e.g., 5GB becomes 1GB), leading to an undersized DPT allocation. When the display engine scans out the framebuffer, could this cause it to read past the undersized DPT and potentially leak arbitrary physical memory contents to the screen? [ ... ] > @@ -164,31 +164,14 @@ static int __xe_pin_fb_vma_dpt(struct drm_gem_object > *obj, > dpt_size = ALIGN(intel_rotation_info_size(&view->rotated) * 8, > XE_PAGE_SIZE); > > - if (IS_DGFX(xe)) > - dpt = xe_bo_create_pin_map_at_novm(xe, tile0, > - dpt_size, ~0ull, > - ttm_bo_type_kernel, > - XE_BO_FLAG_VRAM0 | > - XE_BO_FLAG_GGTT | > - XE_BO_FLAG_PAGETABLE, > - pin_params->alignment, > false); > - else > - dpt = xe_bo_create_pin_map_at_novm(xe, tile0, > - dpt_size, ~0ull, > - ttm_bo_type_kernel, > - XE_BO_FLAG_STOLEN | > - XE_BO_FLAG_GGTT | > - XE_BO_FLAG_PAGETABLE, > - pin_params->alignment, > false); > - if (IS_ERR(dpt)) > - dpt = xe_bo_create_pin_map_at_novm(xe, tile0, > - dpt_size, ~0ull, > - ttm_bo_type_kernel, > - XE_BO_FLAG_SYSTEM | > - XE_BO_FLAG_GGTT | > - XE_BO_FLAG_PAGETABLE | > - XE_BO_FLAG_FORCE_WC, > - pin_params->alignment, > false); > + dpt = xe_bo_create_pin_map_at_novm(xe, tile0, > + dpt_size, ~0ull, > + ttm_bo_type_kernel, > + XE_BO_FLAG_VRAM_IF_DGFX(tile0) | > + XE_BO_FLAG_GGTT | > + XE_BO_FLAG_PAGETABLE | > + XE_BO_FLAG_FORCE_WC, > + pin_params->alignment, false); > if (IS_ERR(dpt)) > return PTR_ERR(dpt); [Severity: Medium] Does removing the if (IS_ERR(dpt)) fallback to XE_BO_FLAG_SYSTEM inadvertently break DPT allocations on discrete graphics (DGFX) under memory pressure? If VRAM is full during normal operations, it seems the allocation would now just fail rather than falling back to SYSTEM memory, which could cause modesets to fail on DGFX. The commit message only mentions avoiding STOLEN memory on integrated graphics. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
