Re: [PATCH] drm/xe/ggtt: clear reserved area at GUC_GGTT_TOP.

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Jun 03, 2026 at 02:57:48PM +0200, Maarten Lankhorst wrote:
> Hello,
> 
> On 5/27/26 16:45, Rodrigo Vivi wrote:
> > The PF GGTT allocator was initialised over a relative [0, usable_size)
> > range, with ggtt->start added on every address conversion to get the
> > actual hardware address.  Two consequences of that model were considered
> > "horrible hacks":
> > 
> >   - ggtt->start (the WOPCM offset) had to be carried around and added
> >     to every drm_mm result.
> This is still the case, only we add a separate variable now that is usually 0.
> We always have to ensure that this is correctly accounted
> for, otherwise we get subtle bugs, so there's no reason not to use it,
> if it's already used everywhere.
> 
> Whether it's 0 or wopcm_start doesn't change the result.
> 
> >   - The GUC_GGTT_TOP ceiling silently truncated the GGTT range instead
> >     of being made explicit, leaving PTEs in [GUC_GGTT_TOP, total_size)
> >     untouched during the initial clear.
> This is a valid concern, but it's very easily be fixed inside the initial clear
> by calling xe_ggtt_clear once more with a comment for !VF case.
> 
> Something like below is enough to fix it. Adding a reserved region at the end/start
> is not the way to do so and just adds more complications.
> ---8<---
> When initialising GGTT, we never clear the part above GUC_GGTT_TOP.
> Add a member to &xe_ggtt that holds the full GGTT size, and clear it
> during the pass at xe_ggtt_initial_clear.
> 
> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
> ---
> diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
> index 8ec23862477fc..497b99a932f0d 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.c
> +++ b/drivers/gpu/drm/xe/xe_ggtt.c
> @@ -115,6 +115,8 @@ struct xe_ggtt {
>  	u64 start;
>  	/** @size: Total usable size of this GGTT */
>  	u64 size;
> +	/** @entire_size: complete size of the accessible GGTT, reserved regions inclusive */
> +	u64 entire_size;
>  	/**
>  	 * @flags: Flags for this GGTT.
>  	 * Acceptable flags:
> @@ -406,7 +408,8 @@ int xe_ggtt_init_early(struct xe_ggtt *ggtt)
>  			return -ENOMEM;
>  		}
>  		ggtt_start = wopcm;
> -		ggtt_size = (gsm_size / 8) * (u64)XE_PAGE_SIZE - ggtt_start;
> +		ggtt->entire_size = (gsm_size / 8) * (u64)XE_PAGE_SIZE;
> +		ggtt_size = ggtt->entire_size - ggtt_start;
>  	} else {
>  		ggtt_start = xe_tile_sriov_vf_ggtt_base(ggtt->tile);
>  		ggtt_size = xe_tile_sriov_vf_ggtt(ggtt->tile);
> @@ -417,6 +420,7 @@ int xe_ggtt_init_early(struct xe_ggtt *ggtt)
>  				    ggtt_start, ggtt_start + ggtt_size - 1);
>  			return -ERANGE;
>  		}
> +		ggtt->entire_size = ggtt_size;
>  	}
>  
>  	ggtt->gsm = ggtt->tile->mmio.regs + SZ_8M;
> @@ -461,6 +465,10 @@ static void xe_ggtt_initial_clear(struct xe_ggtt *ggtt)
>  	drm_mm_for_each_hole(hole, &ggtt->mm, start, end)
>  		xe_ggtt_clear(ggtt, ggtt->start + start, end - start);
>  
> +	end = ggtt->start + ggtt->size;
> +	if (ggtt->entire_size > end)
> +		xe_ggtt_clear(ggtt, end, ggtt->entire_size - end);

You'll need the same thing for the 0 to ggtt->start range.

> +
>  	xe_ggtt_invalidate(ggtt);
>  	mutex_unlock(&ggtt->lock);
>  }

-- 
Ville Syrjälä
Intel