Re: i915 "GPU HANG", bisected to a2daa27c0c61 "swiotlb: simplify swiotlb_max_segment"

Re: i915 "GPU HANG", bisected to a2daa27c0c61 "swiotlb: simplify swiotlb_max_segment"

[Jan Beulich]

On 18.10.2022 10:24, Christoph Hellwig wrote:
> @@ -127,19 +128,22 @@ static inline unsigned int i915_sg_dma_sizes(struct scatterlist *sg)
>  	return page_sizes;
>  }
>  
> -static inline unsigned int i915_sg_segment_size(void)
> +static inline unsigned int i915_sg_segment_size(struct device *dev)
>  {
> -	unsigned int size = swiotlb_max_segment();
> -
> -	if (size == 0)
> -		size = UINT_MAX;
> -
> -	size = rounddown(size, PAGE_SIZE);
> -	/* swiotlb_max_segment_size can return 1 byte when it means one page. */
> -	if (size < PAGE_SIZE)
> -		size = PAGE_SIZE;
> -
> -	return size;
> +	size_t max = min_t(size_t, UINT_MAX, dma_max_mapping_size(dev));
> +
> +	/*
> +	 * Xen on x86 can reshuffle pages under us.  The DMA API takes
> +	 * care of that both in dma_alloc_* (by calling into the hypervisor
> +	 * to make the pages contigous) and in dma_map_* (by bounce buffering).
> +	 * But i915 abuses ignores the coherency aspects of the DMA API and
> +	 * thus can't cope with bounce buffering actually happening, so add
> +	 * a hack here to force small allocations and mapping when running on
> +	 * Xen.  (good luck with TDX, btw --hch)
> +	 */
> +	if (IS_ENABLED(CONFIG_X86) && xen_domain())
> +		max = PAGE_SIZE;
> +	return round_down(max, PAGE_SIZE);
>  }

Shouldn't this then be xen_pv_domain() that you use here, and - if you
really want IS_ENABLED() in addition - CONFIG_XEN_PV?

Jan

Re: i915 "GPU HANG", bisected to a2daa27c0c61 "swiotlb: simplify swiotlb_max_segment"

[Jan Beulich]

On 18.10.2022 13:02, Christoph Hellwig wrote:
> On Tue, Oct 18, 2022 at 10:57:37AM +0200, Jan Beulich wrote:
>> Shouldn't this then be xen_pv_domain() that you use here, and - if you
>> really want IS_ENABLED() in addition - CONFIG_XEN_PV?
> 
> I'll need help from people that understand Xen better than me what
> the exact conditions (and maybe also comments are).

Leaving the "i915 abuses" part aside (because I can't tell what exactly the
abuse is), but assuming that "can't cope with bounce buffering" means they
don't actually use the allocated buffers, I'd suggest this:

	/*
	 * For Xen PV guests pages aren't contiguous in DMA (machine) address
	 * space.  The DMA API takes care of that both in dma_alloc_* (by
	 * calling into the hypervisor to make the pages contiguous) and in
	 * dma_map_* (by bounce buffering).  But i915 abuses ignores the
	 * coherency aspects of the DMA API and thus can't cope with bounce
	 * buffering actually happening, so add a hack here to force small
	 * allocations and mappings when running in PV mode on Xen.
	 */
	if (IS_ENABLED(CONFIG_XEN_PV) && xen_pv_domain())
		max = PAGE_SIZE;

I've dropped the TDX related remark because I don't think it's meaningful
for PV guests. Otoh I've left the "abuses ignores" word sequence as is, no
matter that it reads odd to me. Plus, as hinted at before, I'm not
convinced the IS_ENABLED() use is actually necessary or warranted here.

Jan

Re: i915 "GPU HANG", bisected to a2daa27c0c61 "swiotlb: simplify swiotlb_max_segment"

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1386 bytes --]

On 18.10.22 16:33, Christoph Hellwig wrote:
> On Tue, Oct 18, 2022 at 04:21:43PM +0200, Jan Beulich wrote:
>> Leaving the "i915 abuses" part aside (because I can't tell what exactly the
>> abuse is), but assuming that "can't cope with bounce buffering" means they
>> don't actually use the allocated buffers, I'd suggest this:
> 
> Except for one odd place i915 never uses dma_alloc_* but always allocates
> memory itself and then maps it, but then treats it as if it was a
> dma_alloc_coherent allocations, that is never does ownership changes.
> 
>> I've dropped the TDX related remark because I don't think it's meaningful
>> for PV guests.
> 
> This remark is for TDX in general, not Xen related.  With TDX and other
> confidentatial computing schemes, all DMA must be bounce buffered, and
> all drivers skipping dma_sync* calls are broken.
> 
>> Otoh I've left the "abuses ignores" word sequence as is, no
>> matter that it reads odd to me. Plus, as hinted at before, I'm not
>> convinced the IS_ENABLED() use is actually necessary or warranted here.
> 
> If we don't need the IS_ENABLED is not needed I'm all for dropping it.
> But unless I misread the code, on arm/arm64 even PV guests are 1:1
> mapped so that all Linux physically contigous memory also is Xen
> contigous, so we don't need the hack.

There are no PV guests on arm/arm64.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: i915 "GPU HANG", bisected to a2daa27c0c61 "swiotlb: simplify swiotlb_max_segment"

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 1217 bytes --]

On Tue, Oct 18, 2022 at 10:24:13AM +0200, Christoph Hellwig wrote:
> On Tue, Oct 18, 2022 at 05:52:16AM +0200, Marek Marczykowski-Górecki wrote:
> > not only) when using IGD in Xen PV dom0. After not very long time Xorg
> > crashes, and dmesg contain messages like this:
> > 
> >     i915 0000:00:02.0: [drm] GPU HANG: ecode 7:1:01fffbfe, in Xorg [5337]
> >     i915 0000:00:02.0: [drm] Resetting rcs0 for stopped heartbeat on rcs0
> >     i915 0000:00:02.0: [drm] Xorg[5337] context reset due to GPU hang
> 
> <snip>
> 
> > I tried reverting just this commit on top of 6.0.x, but the context
> > changed significantly in subsequent commits, so after trying reverting
> > it together with 3 or 4 more commits I gave up.
> > 
> > What may be an important detail, the system heavily uses cross-VM shared
> > memory (gntdev) to map window contents from VMs. This is Qubes OS, and
> > it uses Xen 4.14.
> 
> Can you try the patch below?

Yes, this seems to help.

And since this is a PV domain, with Jan proposed change it should work
too. Intuitively this indeed should be PV-only thing (as was forcing
swiotlb before).

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]