Upstream Dom0 DRM problems regarding swiotlb

Upstream Dom0 DRM problems regarding swiotlb

[Michael Labriola]

Konrad,

Starting w/ v4.17, I cannot log in to GNOME w/out getting the
following mess in dmesg and ending up back at the GDM login screen.

[   28.554259] radeon_dp_aux_transfer_native: 200 callbacks suppressed
[   31.219821] radeon 0000:01:00.0: swiotlb buffer is full (sz: 2097152 bytes)
[   31.220030] [drm:radeon_gem_object_create [radeon]] *ERROR* Failed
to allocate GEM object (16384000, 2, 4096, -14)
[   31.226109] radeon 0000:01:00.0: swiotlb buffer is full (sz: 2097152 bytes)
[   31.226300] [drm:radeon_gem_object_create [radeon]] *ERROR* Failed
to allocate GEM object (16384000, 2, 4096, -14)
[   31.300734] gnome-shell[1935]: segfault at 88 ip 00007f39151cd904
sp 00007ffc97611ad8 error 4 in libmutter-cogl.so[7f3915178000+aa000]
[   31.300745] Code: 5f c3 0f 1f 40 00 48 8b 47 78 48 8b 40 40 ff e0
66 0f 1f 44 00 00 48 8b 47 78 48 8b 40 48 ff e0 66 0f 1f 44 00 00 48
8b 47 78 <48> 8b 80 88 00 00 00 ff e0 0f 1f 00 48 8b 47 78 48 8b 40 68
ff e0
[   38.193302] radeon_dp_aux_transfer_native: 116 callbacks suppressed
[   40.009317] radeon 0000:01:00.0: swiotlb buffer is full (sz: 2097152 bytes)
[   40.009488] [drm:radeon_gem_object_create [radeon]] *ERROR* Failed
to allocate GEM object (16384000, 2, 4096, -14)
[   40.015114] radeon 0000:01:00.0: swiotlb buffer is full (sz: 2097152 bytes)
[   40.015297] [drm:radeon_gem_object_create [radeon]] *ERROR* Failed
to allocate GEM object (16384000, 2, 4096, -14)
[   40.028302] gnome-shell[2431]: segfault at 2dadf40 ip
0000000002dadf40 sp 00007ffcd24ea5f8 error 15
[   40.028306] Code: 20 6e 31 00 00 00 00 00 00 00 00 37 e3 3d 2d 7f
00 00 80 f4 e6 3d 2d 7f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 <00> 00 00 00 00 00 00 00 c1 00 00 00 00 00 00 00 80 e1 d2 03
00 00


This happens w/ both radeon and amdgpu.

I bisected down to the following range of commits, which basically add
conditional code to radeon and amdgpu to NOT use swiotlb if dma_bits
is smaller than the system's max iomem address...  but that very much
doesn't work on a Xen dom0.

82626363 drm: add func to get max iomem address v2
fd5fd480 drm/amdgpu: only enable swiotlb alloc when need v2
1bc3d3cc drm/radeon: only enable swiotlb path when need v2

Reverting the offending commits gives me a usable v4.20 dom0 kernel w/
working 3d support.  Not sure what the appropriate upstream fix for
this would be, as I don't 100% understand this.  Could you enlighten
me?  ;-)

FYI, this is on an x86_64 CentOS 7 machine w/ Xen 4.11.1 (installed by
me, from source).

Thanks!

-Mike

-- 
Michael D Labriola
21 Rip Van Winkle Cir
Warwick, RI 02886
401-316-9844 (cell)
401-848-8871 (work)
401-234-1306 (home)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Upstream Dom0 DRM problems regarding swiotlb

[Jan Beulich]

>>> On 12.02.19 at 19:46, <michael.d.labriola@gmail.com> wrote:
> Konrad,
> 
> Starting w/ v4.17, I cannot log in to GNOME w/out getting the
> following mess in dmesg and ending up back at the GDM login screen.
> 
> [   28.554259] radeon_dp_aux_transfer_native: 200 callbacks suppressed
> [   31.219821] radeon 0000:01:00.0: swiotlb buffer is full (sz: 2097152 
> bytes)
> [   31.220030] [drm:radeon_gem_object_create [radeon]] *ERROR* Failed
> to allocate GEM object (16384000, 2, 4096, -14)
> [   31.226109] radeon 0000:01:00.0: swiotlb buffer is full (sz: 2097152 
> bytes)
> [   31.226300] [drm:radeon_gem_object_create [radeon]] *ERROR* Failed
> to allocate GEM object (16384000, 2, 4096, -14)
> [   31.300734] gnome-shell[1935]: segfault at 88 ip 00007f39151cd904
> sp 00007ffc97611ad8 error 4 in libmutter-cogl.so[7f3915178000+aa000]
> [   31.300745] Code: 5f c3 0f 1f 40 00 48 8b 47 78 48 8b 40 40 ff e0
> 66 0f 1f 44 00 00 48 8b 47 78 48 8b 40 48 ff e0 66 0f 1f 44 00 00 48
> 8b 47 78 <48> 8b 80 88 00 00 00 ff e0 0f 1f 00 48 8b 47 78 48 8b 40 68
> ff e0
> [   38.193302] radeon_dp_aux_transfer_native: 116 callbacks suppressed
> [   40.009317] radeon 0000:01:00.0: swiotlb buffer is full (sz: 2097152 
> bytes)
> [   40.009488] [drm:radeon_gem_object_create [radeon]] *ERROR* Failed
> to allocate GEM object (16384000, 2, 4096, -14)
> [   40.015114] radeon 0000:01:00.0: swiotlb buffer is full (sz: 2097152 
> bytes)
> [   40.015297] [drm:radeon_gem_object_create [radeon]] *ERROR* Failed
> to allocate GEM object (16384000, 2, 4096, -14)
> [   40.028302] gnome-shell[2431]: segfault at 2dadf40 ip
> 0000000002dadf40 sp 00007ffcd24ea5f8 error 15
> [   40.028306] Code: 20 6e 31 00 00 00 00 00 00 00 00 37 e3 3d 2d 7f
> 00 00 80 f4 e6 3d 2d 7f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 <00> 00 00 00 00 00 00 00 c1 00 00 00 00 00 00 00 80 e1 d2 03
> 00 00
> 
> 
> This happens w/ both radeon and amdgpu.
> 
> I bisected down to the following range of commits, which basically add
> conditional code to radeon and amdgpu to NOT use swiotlb if dma_bits
> is smaller than the system's max iomem address...  but that very much
> doesn't work on a Xen dom0.

Well, not so much a Xen Dom0, but a Xen PV domain.

> 82626363 drm: add func to get max iomem address v2
> fd5fd480 drm/amdgpu: only enable swiotlb alloc when need v2
> 1bc3d3cc drm/radeon: only enable swiotlb path when need v2
> 
> Reverting the offending commits gives me a usable v4.20 dom0 kernel w/
> working 3d support.  Not sure what the appropriate upstream fix for
> this would be, as I don't 100% understand this.  Could you enlighten
> me?  ;-)

Well, this depends on how much abstraction we want, and how
much abstraction the maintainers of the DRM drivers demand.
It could be as simple as adding xen_swiotlb checks into the
conditionals setting ->need_swiotlb, but in an abstract sense
the issue of course exists for PV guests of any hypervisor.
(Altering drm_get_max_iomem() itself would seem wrong to me,
unless its name was also changed.)

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Upstream Dom0 DRM problems regarding swiotlb

[Michael Labriola]

On Wed, Feb 13, 2019 at 5:34 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 12.02.19 at 19:46, <michael.d.labriola@gmail.com> wrote:
> > Konrad,
> >
> > Starting w/ v4.17, I cannot log in to GNOME w/out getting the
> > following mess in dmesg and ending up back at the GDM login screen.
> >
> > [   28.554259] radeon_dp_aux_transfer_native: 200 callbacks suppressed
> > [   31.219821] radeon 0000:01:00.0: swiotlb buffer is full (sz: 2097152
> > bytes)
> > [   31.220030] [drm:radeon_gem_object_create [radeon]] *ERROR* Failed
> > to allocate GEM object (16384000, 2, 4096, -14)
> > [   31.226109] radeon 0000:01:00.0: swiotlb buffer is full (sz: 2097152
> > bytes)
> > [   31.226300] [drm:radeon_gem_object_create [radeon]] *ERROR* Failed
> > to allocate GEM object (16384000, 2, 4096, -14)
> > [   31.300734] gnome-shell[1935]: segfault at 88 ip 00007f39151cd904
> > sp 00007ffc97611ad8 error 4 in libmutter-cogl.so[7f3915178000+aa000]
> > [   31.300745] Code: 5f c3 0f 1f 40 00 48 8b 47 78 48 8b 40 40 ff e0
> > 66 0f 1f 44 00 00 48 8b 47 78 48 8b 40 48 ff e0 66 0f 1f 44 00 00 48
> > 8b 47 78 <48> 8b 80 88 00 00 00 ff e0 0f 1f 00 48 8b 47 78 48 8b 40 68
> > ff e0
> > [   38.193302] radeon_dp_aux_transfer_native: 116 callbacks suppressed
> > [   40.009317] radeon 0000:01:00.0: swiotlb buffer is full (sz: 2097152
> > bytes)
> > [   40.009488] [drm:radeon_gem_object_create [radeon]] *ERROR* Failed
> > to allocate GEM object (16384000, 2, 4096, -14)
> > [   40.015114] radeon 0000:01:00.0: swiotlb buffer is full (sz: 2097152
> > bytes)
> > [   40.015297] [drm:radeon_gem_object_create [radeon]] *ERROR* Failed
> > to allocate GEM object (16384000, 2, 4096, -14)
> > [   40.028302] gnome-shell[2431]: segfault at 2dadf40 ip
> > 0000000002dadf40 sp 00007ffcd24ea5f8 error 15
> > [   40.028306] Code: 20 6e 31 00 00 00 00 00 00 00 00 37 e3 3d 2d 7f
> > 00 00 80 f4 e6 3d 2d 7f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > 00 00 00 <00> 00 00 00 00 00 00 00 c1 00 00 00 00 00 00 00 80 e1 d2 03
> > 00 00
> >
> >
> > This happens w/ both radeon and amdgpu.
> >
> > I bisected down to the following range of commits, which basically add
> > conditional code to radeon and amdgpu to NOT use swiotlb if dma_bits
> > is smaller than the system's max iomem address...  but that very much
> > doesn't work on a Xen dom0.
>
> Well, not so much a Xen Dom0, but a Xen PV domain.
>
> > 82626363 drm: add func to get max iomem address v2
> > fd5fd480 drm/amdgpu: only enable swiotlb alloc when need v2
> > 1bc3d3cc drm/radeon: only enable swiotlb path when need v2
> >
> > Reverting the offending commits gives me a usable v4.20 dom0 kernel w/
> > working 3d support.  Not sure what the appropriate upstream fix for
> > this would be, as I don't 100% understand this.  Could you enlighten
> > me?  ;-)
>
> Well, this depends on how much abstraction we want, and how
> much abstraction the maintainers of the DRM drivers demand.
> It could be as simple as adding xen_swiotlb checks into the
> conditionals setting ->need_swiotlb, but in an abstract sense
> the issue of course exists for PV guests of any hypervisor.
> (Altering drm_get_max_iomem() itself would seem wrong to me,
> unless its name was also changed.)

Ah, so this isn't necessarily Xen-specific but rather any paravirtual
guest?  That hadn't crossed my mind.  Is there an easy way to find out
if we're a pv guest in the need_swiotlb conditionals?  If not, we
should at least add a module parameter to force swiotlb usage to both
radeon and amdgpu.  I'd be more than happy to gin up a patch to do
either and submit to upstream (dri-devel, I guess).

Thanks!

-Mike

-- 
Michael D Labriola
21 Rip Van Winkle Cir
Warwick, RI 02886
401-316-9844 (cell)
401-848-8871 (work)
401-234-1306 (home)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Upstream Dom0 DRM problems regarding swiotlb

[Jan Beulich]

>>> On 13.02.19 at 15:10, <michael.d.labriola@gmail.com> wrote:
> On Wed, Feb 13, 2019 at 5:34 AM Jan Beulich <JBeulich@suse.com> wrote:
>>
>> >>> On 12.02.19 at 19:46, <michael.d.labriola@gmail.com> wrote:
>> > Konrad,
>> >
>> > Starting w/ v4.17, I cannot log in to GNOME w/out getting the
>> > following mess in dmesg and ending up back at the GDM login screen.
>> >
>> > [   28.554259] radeon_dp_aux_transfer_native: 200 callbacks suppressed
>> > [   31.219821] radeon 0000:01:00.0: swiotlb buffer is full (sz: 2097152
>> > bytes)
>> > [   31.220030] [drm:radeon_gem_object_create [radeon]] *ERROR* Failed
>> > to allocate GEM object (16384000, 2, 4096, -14)
>> > [   31.226109] radeon 0000:01:00.0: swiotlb buffer is full (sz: 2097152
>> > bytes)
>> > [   31.226300] [drm:radeon_gem_object_create [radeon]] *ERROR* Failed
>> > to allocate GEM object (16384000, 2, 4096, -14)
>> > [   31.300734] gnome-shell[1935]: segfault at 88 ip 00007f39151cd904
>> > sp 00007ffc97611ad8 error 4 in libmutter-cogl.so[7f3915178000+aa000]
>> > [   31.300745] Code: 5f c3 0f 1f 40 00 48 8b 47 78 48 8b 40 40 ff e0
>> > 66 0f 1f 44 00 00 48 8b 47 78 48 8b 40 48 ff e0 66 0f 1f 44 00 00 48
>> > 8b 47 78 <48> 8b 80 88 00 00 00 ff e0 0f 1f 00 48 8b 47 78 48 8b 40 68
>> > ff e0
>> > [   38.193302] radeon_dp_aux_transfer_native: 116 callbacks suppressed
>> > [   40.009317] radeon 0000:01:00.0: swiotlb buffer is full (sz: 2097152
>> > bytes)
>> > [   40.009488] [drm:radeon_gem_object_create [radeon]] *ERROR* Failed
>> > to allocate GEM object (16384000, 2, 4096, -14)
>> > [   40.015114] radeon 0000:01:00.0: swiotlb buffer is full (sz: 2097152
>> > bytes)
>> > [   40.015297] [drm:radeon_gem_object_create [radeon]] *ERROR* Failed
>> > to allocate GEM object (16384000, 2, 4096, -14)
>> > [   40.028302] gnome-shell[2431]: segfault at 2dadf40 ip
>> > 0000000002dadf40 sp 00007ffcd24ea5f8 error 15
>> > [   40.028306] Code: 20 6e 31 00 00 00 00 00 00 00 00 37 e3 3d 2d 7f
>> > 00 00 80 f4 e6 3d 2d 7f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> > 00 00 00 <00> 00 00 00 00 00 00 00 c1 00 00 00 00 00 00 00 80 e1 d2 03
>> > 00 00
>> >
>> >
>> > This happens w/ both radeon and amdgpu.
>> >
>> > I bisected down to the following range of commits, which basically add
>> > conditional code to radeon and amdgpu to NOT use swiotlb if dma_bits
>> > is smaller than the system's max iomem address...  but that very much
>> > doesn't work on a Xen dom0.
>>
>> Well, not so much a Xen Dom0, but a Xen PV domain.
>>
>> > 82626363 drm: add func to get max iomem address v2
>> > fd5fd480 drm/amdgpu: only enable swiotlb alloc when need v2
>> > 1bc3d3cc drm/radeon: only enable swiotlb path when need v2
>> >
>> > Reverting the offending commits gives me a usable v4.20 dom0 kernel w/
>> > working 3d support.  Not sure what the appropriate upstream fix for
>> > this would be, as I don't 100% understand this.  Could you enlighten
>> > me?  ;-)
>>
>> Well, this depends on how much abstraction we want, and how
>> much abstraction the maintainers of the DRM drivers demand.
>> It could be as simple as adding xen_swiotlb checks into the
>> conditionals setting ->need_swiotlb, but in an abstract sense
>> the issue of course exists for PV guests of any hypervisor.
>> (Altering drm_get_max_iomem() itself would seem wrong to me,
>> unless its name was also changed.)
> 
> Ah, so this isn't necessarily Xen-specific but rather any paravirtual
> guest?  That hadn't crossed my mind.  Is there an easy way to find out
> if we're a pv guest in the need_swiotlb conditionals?

There's xen_pv_domain(), but I think xen_swiotlb would be more to
the point if the check is already to be Xen-specific. There's no generic
"is PV" predicate that I'm aware of.

>  If not, we
> should at least add a module parameter to force swiotlb usage to both
> radeon and amdgpu.  I'd be more than happy to gin up a patch to do
> either and submit to upstream (dri-devel, I guess).

I don't think module parameters are a good way forward here.
They may do as a temporary workaround, but not as a solution.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Upstream Dom0 DRM problems regarding swiotlb

[Michael Labriola]

On Wed, Feb 13, 2019 at 9:28 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 13.02.19 at 15:10, <michael.d.labriola@gmail.com> wrote:
> > On Wed, Feb 13, 2019 at 5:34 AM Jan Beulich <JBeulich@suse.com> wrote:
> >>
> >> >>> On 12.02.19 at 19:46, <michael.d.labriola@gmail.com> wrote:
> >> > Konrad,
> >> >
> >> > Starting w/ v4.17, I cannot log in to GNOME w/out getting the
> >> > following mess in dmesg and ending up back at the GDM login screen.
> >> >
> >> > [   28.554259] radeon_dp_aux_transfer_native: 200 callbacks suppressed
> >> > [   31.219821] radeon 0000:01:00.0: swiotlb buffer is full (sz: 2097152
> >> > bytes)
> >> > [   31.220030] [drm:radeon_gem_object_create [radeon]] *ERROR* Failed
> >> > to allocate GEM object (16384000, 2, 4096, -14)
> >> > [   31.226109] radeon 0000:01:00.0: swiotlb buffer is full (sz: 2097152
> >> > bytes)
> >> > [   31.226300] [drm:radeon_gem_object_create [radeon]] *ERROR* Failed
> >> > to allocate GEM object (16384000, 2, 4096, -14)
> >> > [   31.300734] gnome-shell[1935]: segfault at 88 ip 00007f39151cd904
> >> > sp 00007ffc97611ad8 error 4 in libmutter-cogl.so[7f3915178000+aa000]
> >> > [   31.300745] Code: 5f c3 0f 1f 40 00 48 8b 47 78 48 8b 40 40 ff e0
> >> > 66 0f 1f 44 00 00 48 8b 47 78 48 8b 40 48 ff e0 66 0f 1f 44 00 00 48
> >> > 8b 47 78 <48> 8b 80 88 00 00 00 ff e0 0f 1f 00 48 8b 47 78 48 8b 40 68
> >> > ff e0
> >> > [   38.193302] radeon_dp_aux_transfer_native: 116 callbacks suppressed
> >> > [   40.009317] radeon 0000:01:00.0: swiotlb buffer is full (sz: 2097152
> >> > bytes)
> >> > [   40.009488] [drm:radeon_gem_object_create [radeon]] *ERROR* Failed
> >> > to allocate GEM object (16384000, 2, 4096, -14)
> >> > [   40.015114] radeon 0000:01:00.0: swiotlb buffer is full (sz: 2097152
> >> > bytes)
> >> > [   40.015297] [drm:radeon_gem_object_create [radeon]] *ERROR* Failed
> >> > to allocate GEM object (16384000, 2, 4096, -14)
> >> > [   40.028302] gnome-shell[2431]: segfault at 2dadf40 ip
> >> > 0000000002dadf40 sp 00007ffcd24ea5f8 error 15
> >> > [   40.028306] Code: 20 6e 31 00 00 00 00 00 00 00 00 37 e3 3d 2d 7f
> >> > 00 00 80 f4 e6 3d 2d 7f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >> > 00 00 00 <00> 00 00 00 00 00 00 00 c1 00 00 00 00 00 00 00 80 e1 d2 03
> >> > 00 00
> >> >
> >> >
> >> > This happens w/ both radeon and amdgpu.
> >> >
> >> > I bisected down to the following range of commits, which basically add
> >> > conditional code to radeon and amdgpu to NOT use swiotlb if dma_bits
> >> > is smaller than the system's max iomem address...  but that very much
> >> > doesn't work on a Xen dom0.
> >>
> >> Well, not so much a Xen Dom0, but a Xen PV domain.
> >>
> >> > 82626363 drm: add func to get max iomem address v2
> >> > fd5fd480 drm/amdgpu: only enable swiotlb alloc when need v2
> >> > 1bc3d3cc drm/radeon: only enable swiotlb path when need v2
> >> >
> >> > Reverting the offending commits gives me a usable v4.20 dom0 kernel w/
> >> > working 3d support.  Not sure what the appropriate upstream fix for
> >> > this would be, as I don't 100% understand this.  Could you enlighten
> >> > me?  ;-)
> >>
> >> Well, this depends on how much abstraction we want, and how
> >> much abstraction the maintainers of the DRM drivers demand.
> >> It could be as simple as adding xen_swiotlb checks into the
> >> conditionals setting ->need_swiotlb, but in an abstract sense
> >> the issue of course exists for PV guests of any hypervisor.
> >> (Altering drm_get_max_iomem() itself would seem wrong to me,
> >> unless its name was also changed.)

So, the commit message for the patch that added drm_get_max_iomem()
specifically states that the function "will be used to check if the
driver needs swiotlb"...  Sounds like a logical place to put some type
of PV conditional to me.  I get that that seems a bit sneaky and
unrelated to the function's name... but it definitely plays directly
into the function's stated purpose.

> >
> > Ah, so this isn't necessarily Xen-specific but rather any paravirtual
> > guest?  That hadn't crossed my mind.  Is there an easy way to find out
> > if we're a pv guest in the need_swiotlb conditionals?
>
> There's xen_pv_domain(), but I think xen_swiotlb would be more to
> the point if the check is already to be Xen-specific. There's no generic
> "is PV" predicate that I'm aware of.

Well, that makes doing conditional code right more difficult.  I
assume since there isn't a generic predicate, and PV isn't new, that
it's absence is by design?  To reign in the temptation to sprinkle
conditional code all over the kernel?  ;-)

>
> >  If not, we
> > should at least add a module parameter to force swiotlb usage to both
> > radeon and amdgpu.  I'd be more than happy to gin up a patch to do
> > either and submit to upstream (dri-devel, I guess).
>
> I don't think module parameters are a good way forward here.
> They may do as a temporary workaround, but not as a solution.

Agreed.  I suppose not many people have bumped into this problem in
the wild, since it's been in mainline since 4.17.  Am I really the
only person running a development system in Xen w/ AMD video cards who
expects 3d to work?  Didn't really seem like a strange workload to
me... especially with so many desktop environments requiring GL
support nowadays.

-Mike

-- 
Michael D Labriola
21 Rip Van Winkle Cir
Warwick, RI 02886
401-316-9844 (cell)
401-848-8871 (work)
401-234-1306 (home)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Upstream Dom0 DRM problems regarding swiotlb

[Jan Beulich]

>>> On 13.02.19 at 17:00, <michael.d.labriola@gmail.com> wrote:
> On Wed, Feb 13, 2019 at 9:28 AM Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 13.02.19 at 15:10, <michael.d.labriola@gmail.com> wrote:
>> > Ah, so this isn't necessarily Xen-specific but rather any paravirtual
>> > guest?  That hadn't crossed my mind.  Is there an easy way to find out
>> > if we're a pv guest in the need_swiotlb conditionals?
>>
>> There's xen_pv_domain(), but I think xen_swiotlb would be more to
>> the point if the check is already to be Xen-specific. There's no generic
>> "is PV" predicate that I'm aware of.
> 
> Well, that makes doing conditional code right more difficult.  I
> assume since there isn't a generic predicate, and PV isn't new, that
> it's absence is by design?  To reign in the temptation to sprinkle
> conditional code all over the kernel?  ;-)

Well, with lguest gone, Xen is the only PV environment the kernel
can run in, afaik at least. I guess to decide between the suggested
options or the need for some abstracting macro (or yet something
else), you'll really need to ask the driver maintainers. Or simply
send a patch their way implementing one of them, and see what
their reaction is.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Upstream Dom0 DRM problems regarding swiotlb

[Konrad Rzeszutek Wilk]

On Wed, Feb 13, 2019 at 09:09:32AM -0700, Jan Beulich wrote:
> >>> On 13.02.19 at 17:00, <michael.d.labriola@gmail.com> wrote:
> > On Wed, Feb 13, 2019 at 9:28 AM Jan Beulich <JBeulich@suse.com> wrote:
> >> >>> On 13.02.19 at 15:10, <michael.d.labriola@gmail.com> wrote:
> >> > Ah, so this isn't necessarily Xen-specific but rather any paravirtual
> >> > guest?  That hadn't crossed my mind.  Is there an easy way to find out
> >> > if we're a pv guest in the need_swiotlb conditionals?
> >>
> >> There's xen_pv_domain(), but I think xen_swiotlb would be more to
> >> the point if the check is already to be Xen-specific. There's no generic
> >> "is PV" predicate that I'm aware of.
> > 
> > Well, that makes doing conditional code right more difficult.  I
> > assume since there isn't a generic predicate, and PV isn't new, that
> > it's absence is by design?  To reign in the temptation to sprinkle
> > conditional code all over the kernel?  ;-)
> 
> Well, with lguest gone, Xen is the only PV environment the kernel
> can run in, afaik at least. I guess to decide between the suggested
> options or the need for some abstracting macro (or yet something
> else), you'll really need to ask the driver maintainers. Or simply
> send a patch their way implementing one of them, and see what
> their reaction is.

Could you try this out and see if it works and I will send it out:



diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
index 9fc3296592fe..96bf1df0ed28 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
@@ -23,6 +23,7 @@
 #include <linux/firmware.h>
 #include <drm/drmP.h>
 #include <drm/drm_cache.h>
+#include <xen/xen.h>
 #include "amdgpu.h"
 #include "gmc_v6_0.h"
 #include "amdgpu_ucode.h"
@@ -887,6 +888,8 @@ static int gmc_v6_0_sw_init(void *handle)
 		dev_warn(adev->dev, "amdgpu: No coherent DMA available.\n");
 	}
 	adev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
+	if (xen_pv_domain())
+		adev->need_swiotlb = 1;
 
 	r = gmc_v6_0_init_microcode(adev);
 	if (r) {
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
index 761dcfb2fec0..710ac0ece1b0 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
@@ -23,6 +23,7 @@
 #include <linux/firmware.h>
 #include <drm/drmP.h>
 #include <drm/drm_cache.h>
+#include <xen/xen.h>
 #include "amdgpu.h"
 #include "cikd.h"
 #include "cik.h"
@@ -1031,6 +1032,8 @@ static int gmc_v7_0_sw_init(void *handle)
 		pr_warn("amdgpu: No coherent DMA available\n");
 	}
 	adev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
+	if (xen_pv_domain())
+		adev->need_swiotlb = 1;
 
 	r = gmc_v7_0_init_microcode(adev);
 	if (r) {
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
index 1ad7e6b8ed1d..c418a129bb32 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
@@ -23,6 +23,7 @@
 #include <linux/firmware.h>
 #include <drm/drmP.h>
 #include <drm/drm_cache.h>
+#include <xen/xen.h>
 #include "amdgpu.h"
 #include "gmc_v8_0.h"
 #include "amdgpu_ucode.h"
@@ -1156,6 +1157,8 @@ static int gmc_v8_0_sw_init(void *handle)
 		pr_warn("amdgpu: No coherent DMA available\n");
 	}
 	adev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
+	if (xen_pv_domain())
+		adev->need_swiotlb = 1;
 
 	r = gmc_v8_0_init_microcode(adev);
 	if (r) {
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
index bacdaef77b6c..85c0762c37ae 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
@@ -22,6 +22,7 @@
  */
 #include <linux/firmware.h>
 #include <drm/drm_cache.h>
+#include <xen/xen.h>
 #include "amdgpu.h"
 #include "gmc_v9_0.h"
 #include "amdgpu_atomfirmware.h"
@@ -1004,6 +1005,8 @@ static int gmc_v9_0_sw_init(void *handle)
 		printk(KERN_WARNING "amdgpu: No coherent DMA available.\n");
 	}
 	adev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
+	if (xen_pv_domain())
+		adev->need_swiotlb = 1;
 
 	if (adev->gmc.xgmi.supported) {
 		r = gfxhub_v1_1_get_xgmi_info(adev);
diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
index 59c8a6647ff2..02fba6829936 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -35,6 +35,7 @@
 #include <linux/vgaarb.h>
 #include <linux/vga_switcheroo.h>
 #include <linux/efi.h>
+#include <xen/xen.h>
 #include "radeon_reg.h"
 #include "radeon.h"
 #include "atom.h"
@@ -1388,6 +1389,8 @@ int radeon_device_init(struct radeon_device *rdev,
 		pr_warn("radeon: No coherent DMA available\n");
 	}
 	rdev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
+	if (xen_pv_domain())
+		rdev->need_swiotlb = 1;
 
 	/* Registers mapping */
 	/* TODO: block userspace mapping of io register */

> 
> Jan
> 
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Upstream Dom0 DRM problems regarding swiotlb

[Michael Labriola]

On Wed, Feb 13, 2019 at 11:57 AM Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
>
> On Wed, Feb 13, 2019 at 09:09:32AM -0700, Jan Beulich wrote:
> > >>> On 13.02.19 at 17:00, <michael.d.labriola@gmail.com> wrote:
> > > On Wed, Feb 13, 2019 at 9:28 AM Jan Beulich <JBeulich@suse.com> wrote:
> > >> >>> On 13.02.19 at 15:10, <michael.d.labriola@gmail.com> wrote:
> > >> > Ah, so this isn't necessarily Xen-specific but rather any paravirtual
> > >> > guest?  That hadn't crossed my mind.  Is there an easy way to find out
> > >> > if we're a pv guest in the need_swiotlb conditionals?
> > >>
> > >> There's xen_pv_domain(), but I think xen_swiotlb would be more to
> > >> the point if the check is already to be Xen-specific. There's no generic
> > >> "is PV" predicate that I'm aware of.
> > >
> > > Well, that makes doing conditional code right more difficult.  I
> > > assume since there isn't a generic predicate, and PV isn't new, that
> > > it's absence is by design?  To reign in the temptation to sprinkle
> > > conditional code all over the kernel?  ;-)
> >
> > Well, with lguest gone, Xen is the only PV environment the kernel
> > can run in, afaik at least. I guess to decide between the suggested
> > options or the need for some abstracting macro (or yet something
> > else), you'll really need to ask the driver maintainers. Or simply
> > send a patch their way implementing one of them, and see what
> > their reaction is.
>
> Could you try this out and see if it works and I will send it out:
>
>
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
> index 9fc3296592fe..96bf1df0ed28 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
> @@ -23,6 +23,7 @@
>  #include <linux/firmware.h>
>  #include <drm/drmP.h>
>  #include <drm/drm_cache.h>
> +#include <xen/xen.h>
>  #include "amdgpu.h"
>  #include "gmc_v6_0.h"
>  #include "amdgpu_ucode.h"
> @@ -887,6 +888,8 @@ static int gmc_v6_0_sw_init(void *handle)
>                 dev_warn(adev->dev, "amdgpu: No coherent DMA available.\n");
>         }
>         adev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
> +       if (xen_pv_domain())
> +               adev->need_swiotlb = 1;
>
>         r = gmc_v6_0_init_microcode(adev);
>         if (r) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
> index 761dcfb2fec0..710ac0ece1b0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
> @@ -23,6 +23,7 @@
>  #include <linux/firmware.h>
>  #include <drm/drmP.h>
>  #include <drm/drm_cache.h>
> +#include <xen/xen.h>
>  #include "amdgpu.h"
>  #include "cikd.h"
>  #include "cik.h"
> @@ -1031,6 +1032,8 @@ static int gmc_v7_0_sw_init(void *handle)
>                 pr_warn("amdgpu: No coherent DMA available\n");
>         }
>         adev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
> +       if (xen_pv_domain())
> +               adev->need_swiotlb = 1;
>
>         r = gmc_v7_0_init_microcode(adev);
>         if (r) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> index 1ad7e6b8ed1d..c418a129bb32 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> @@ -23,6 +23,7 @@
>  #include <linux/firmware.h>
>  #include <drm/drmP.h>
>  #include <drm/drm_cache.h>
> +#include <xen/xen.h>
>  #include "amdgpu.h"
>  #include "gmc_v8_0.h"
>  #include "amdgpu_ucode.h"
> @@ -1156,6 +1157,8 @@ static int gmc_v8_0_sw_init(void *handle)
>                 pr_warn("amdgpu: No coherent DMA available\n");
>         }
>         adev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
> +       if (xen_pv_domain())
> +               adev->need_swiotlb = 1;
>
>         r = gmc_v8_0_init_microcode(adev);
>         if (r) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index bacdaef77b6c..85c0762c37ae 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -22,6 +22,7 @@
>   */
>  #include <linux/firmware.h>
>  #include <drm/drm_cache.h>
> +#include <xen/xen.h>
>  #include "amdgpu.h"
>  #include "gmc_v9_0.h"
>  #include "amdgpu_atomfirmware.h"
> @@ -1004,6 +1005,8 @@ static int gmc_v9_0_sw_init(void *handle)
>                 printk(KERN_WARNING "amdgpu: No coherent DMA available.\n");
>         }
>         adev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
> +       if (xen_pv_domain())
> +               adev->need_swiotlb = 1;
>
>         if (adev->gmc.xgmi.supported) {
>                 r = gfxhub_v1_1_get_xgmi_info(adev);
> diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
> index 59c8a6647ff2..02fba6829936 100644
> --- a/drivers/gpu/drm/radeon/radeon_device.c
> +++ b/drivers/gpu/drm/radeon/radeon_device.c
> @@ -35,6 +35,7 @@
>  #include <linux/vgaarb.h>
>  #include <linux/vga_switcheroo.h>
>  #include <linux/efi.h>
> +#include <xen/xen.h>
>  #include "radeon_reg.h"
>  #include "radeon.h"
>  #include "atom.h"
> @@ -1388,6 +1389,8 @@ int radeon_device_init(struct radeon_device *rdev,
>                 pr_warn("radeon: No coherent DMA available\n");
>         }
>         rdev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
> +       if (xen_pv_domain())
> +               rdev->need_swiotlb = 1;
>
>         /* Registers mapping */
>         /* TODO: block userspace mapping of io register */
>
> >
> > Jan
> >
> >

Yes, that works for me.  However, I feel like the conditional should
be in drm_get_max_iomem() instead of directly after it everywhere it's
used...  and is just checking xen_pv_domain() enough?  Jan made it
sound like there were possibly other PV cases that would also still
need swiotlb.

-Mike

-- 
Michael D Labriola
21 Rip Van Winkle Cir
Warwick, RI 02886
401-316-9844 (cell)
401-848-8871 (work)
401-234-1306 (home)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Upstream Dom0 DRM problems regarding swiotlb

[Michael Labriola]

On Wed, Feb 13, 2019 at 1:16 PM Michael Labriola
<michael.d.labriola@gmail.com> wrote:
>
> On Wed, Feb 13, 2019 at 11:57 AM Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com> wrote:
> >
> > On Wed, Feb 13, 2019 at 09:09:32AM -0700, Jan Beulich wrote:
> > > >>> On 13.02.19 at 17:00, <michael.d.labriola@gmail.com> wrote:
> > > > On Wed, Feb 13, 2019 at 9:28 AM Jan Beulich <JBeulich@suse.com> wrote:
> > > >> >>> On 13.02.19 at 15:10, <michael.d.labriola@gmail.com> wrote:
> > > >> > Ah, so this isn't necessarily Xen-specific but rather any paravirtual
> > > >> > guest?  That hadn't crossed my mind.  Is there an easy way to find out
> > > >> > if we're a pv guest in the need_swiotlb conditionals?
> > > >>
> > > >> There's xen_pv_domain(), but I think xen_swiotlb would be more to
> > > >> the point if the check is already to be Xen-specific. There's no generic
> > > >> "is PV" predicate that I'm aware of.
> > > >
> > > > Well, that makes doing conditional code right more difficult.  I
> > > > assume since there isn't a generic predicate, and PV isn't new, that
> > > > it's absence is by design?  To reign in the temptation to sprinkle
> > > > conditional code all over the kernel?  ;-)
> > >
> > > Well, with lguest gone, Xen is the only PV environment the kernel
> > > can run in, afaik at least. I guess to decide between the suggested
> > > options or the need for some abstracting macro (or yet something
> > > else), you'll really need to ask the driver maintainers. Or simply
> > > send a patch their way implementing one of them, and see what
> > > their reaction is.
> >
> > Could you try this out and see if it works and I will send it out:
> >
*snip*
>
> Yes, that works for me.  However, I feel like the conditional should
> be in drm_get_max_iomem() instead of directly after it everywhere it's
> used...  and is just checking xen_pv_domain() enough?  Jan made it
> sound like there were possibly other PV cases that would also still
> need swiotlb.

How about this?  It strcmp's pv_info to see if we're bare metal, does
the comparison in a single place, moves the bit shifting comparison
into the function (simplifying the drm driver code), and renames the
function to more aptly describe what's going on.


diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
index 73ad02aea2b2..328d45b8b2ec 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
@@ -885,7 +885,7 @@ static int gmc_v6_0_sw_init(void *handle)
         pci_set_consistent_dma_mask(adev->pdev, DMA_BIT_MASK(32));
         dev_warn(adev->dev, "amdgpu: No coherent DMA available.\n");
     }
-    adev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
+    adev->need_swiotlb = drm_need_swiotlb_for_dma(dma_bits);

     r = gmc_v6_0_init_microcode(adev);
     if (r) {
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
index 910c4ce19cb3..3d49eff28448 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
@@ -1029,7 +1029,7 @@ static int gmc_v7_0_sw_init(void *handle)
         pci_set_consistent_dma_mask(adev->pdev, DMA_BIT_MASK(32));
         pr_warn("amdgpu: No coherent DMA available\n");
     }
-    adev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
+    adev->need_swiotlb = drm_need_swiotlb_for_dma(dma_bits);

     r = gmc_v7_0_init_microcode(adev);
     if (r) {
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
index 747c068379dc..9247dd6316f1 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
@@ -1155,7 +1155,7 @@ static int gmc_v8_0_sw_init(void *handle)
         pci_set_consistent_dma_mask(adev->pdev, DMA_BIT_MASK(32));
         pr_warn("amdgpu: No coherent DMA available\n");
     }
-    adev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
+    adev->need_swiotlb = drm_need_swiotlb_for_dma(dma_bits);

     r = gmc_v8_0_init_microcode(adev);
     if (r) {
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
index f35d7a554ad5..89f3fe981ac5 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
@@ -989,7 +989,7 @@ static int gmc_v9_0_sw_init(void *handle)
         pci_set_consistent_dma_mask(adev->pdev, DMA_BIT_MASK(32));
         printk(KERN_WARNING "amdgpu: No coherent DMA available.\n");
     }
-    adev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
+    adev->need_swiotlb = drm_need_swiotlb_for_dma(dma_bits);

     if (adev->asic_type == CHIP_VEGA20) {
         r = gfxhub_v1_1_get_xgmi_info(adev);
diff --git a/drivers/gpu/drm/drm_memory.c b/drivers/gpu/drm/drm_memory.c
index d69e4fc1ee77..f22f6a0d20b3 100644
--- a/drivers/gpu/drm/drm_memory.c
+++ b/drivers/gpu/drm/drm_memory.c
@@ -35,6 +35,7 @@

 #include <linux/highmem.h>
 #include <linux/export.h>
+#include <xen/xen.h>
 #include <drm/drmP.h>
 #include "drm_legacy.h"

@@ -150,15 +151,24 @@ void drm_legacy_ioremapfree(struct drm_local_map
*map, struct drm_device *dev)
 }
 EXPORT_SYMBOL(drm_legacy_ioremapfree);

-u64 drm_get_max_iomem(void)
+bool drm_need_swiotlb_for_dma(int dma_bits)
 {
     struct resource *tmp;
     resource_size_t max_iomem = 0;

+#ifdef CONFIG_PARAVIRT
+    /*
+     * Paravirtual hosts require swiotlb regardless of requested dma
+     * transfer size.
+     */
+    if (strcmp(pv_info.name, "bare hardware") != 0)
+        return true;
+#endif
+
     for (tmp = iomem_resource.child; tmp; tmp = tmp->sibling) {
         max_iomem = max(max_iomem,  tmp->end);
     }

-    return max_iomem;
+    return max_iomem > ((u64)1 << dma_bits);
 }
-EXPORT_SYMBOL(drm_get_max_iomem);
+EXPORT_SYMBOL(drm_need_swiotlb_for_dma);
diff --git a/drivers/gpu/drm/radeon/radeon_device.c
b/drivers/gpu/drm/radeon/radeon_device.c
index 59c8a6647ff2..7c8222d98bc3 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -1387,7 +1387,8 @@ int radeon_device_init(struct radeon_device *rdev,
         pci_set_consistent_dma_mask(rdev->pdev, DMA_BIT_MASK(32));
         pr_warn("radeon: No coherent DMA available\n");
     }
-    rdev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
+    rdev->need_swiotlb = drm_need_swiotlb_for_dma(dma_bits);
+    pr_info("%s: need_swiotlb: %d\n", __func__, rdev->need_swiotlb);

     /* Registers mapping */
     /* TODO: block userspace mapping of io register */
diff --git a/include/drm/drm_cache.h b/include/drm/drm_cache.h
index bfe1639df02d..070b44624ff9 100644
--- a/include/drm/drm_cache.h
+++ b/include/drm/drm_cache.h
@@ -38,7 +38,7 @@
 void drm_clflush_pages(struct page *pages[], unsigned long num_pages);
 void drm_clflush_sg(struct sg_table *st);
 void drm_clflush_virt_range(void *addr, unsigned long length);
-u64 drm_get_max_iomem(void);
+bool drm_need_swiotlb_for_dma(int dma_bits);


 static inline bool drm_arch_can_wc_memory(void)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Upstream Dom0 DRM problems regarding swiotlb

[Konrad Rzeszutek Wilk]

On Wed, Feb 13, 2019 at 01:38:21PM -0500, Michael Labriola wrote:
> On Wed, Feb 13, 2019 at 1:16 PM Michael Labriola
> <michael.d.labriola@gmail.com> wrote:
> >
> > On Wed, Feb 13, 2019 at 11:57 AM Konrad Rzeszutek Wilk
> > <konrad.wilk@oracle.com> wrote:
> > >
> > > On Wed, Feb 13, 2019 at 09:09:32AM -0700, Jan Beulich wrote:
> > > > >>> On 13.02.19 at 17:00, <michael.d.labriola@gmail.com> wrote:
> > > > > On Wed, Feb 13, 2019 at 9:28 AM Jan Beulich <JBeulich@suse.com> wrote:
> > > > >> >>> On 13.02.19 at 15:10, <michael.d.labriola@gmail.com> wrote:
> > > > >> > Ah, so this isn't necessarily Xen-specific but rather any paravirtual
> > > > >> > guest?  That hadn't crossed my mind.  Is there an easy way to find out
> > > > >> > if we're a pv guest in the need_swiotlb conditionals?
> > > > >>
> > > > >> There's xen_pv_domain(), but I think xen_swiotlb would be more to
> > > > >> the point if the check is already to be Xen-specific. There's no generic
> > > > >> "is PV" predicate that I'm aware of.
> > > > >
> > > > > Well, that makes doing conditional code right more difficult.  I
> > > > > assume since there isn't a generic predicate, and PV isn't new, that
> > > > > it's absence is by design?  To reign in the temptation to sprinkle
> > > > > conditional code all over the kernel?  ;-)
> > > >
> > > > Well, with lguest gone, Xen is the only PV environment the kernel
> > > > can run in, afaik at least. I guess to decide between the suggested
> > > > options or the need for some abstracting macro (or yet something
> > > > else), you'll really need to ask the driver maintainers. Or simply
> > > > send a patch their way implementing one of them, and see what
> > > > their reaction is.
> > >
> > > Could you try this out and see if it works and I will send it out:
> > >
> *snip*
> >
> > Yes, that works for me.  However, I feel like the conditional should
> > be in drm_get_max_iomem() instead of directly after it everywhere it's
> > used...  and is just checking xen_pv_domain() enough?  Jan made it
> > sound like there were possibly other PV cases that would also still
> > need swiotlb.
> 
> How about this?  It strcmp's pv_info to see if we're bare metal, does
> the comparison in a single place, moves the bit shifting comparison
> into the function (simplifying the drm driver code), and renames the
> function to more aptly describe what's going on.

<nods> That looks much better.

Would love to see this posted upstream!
> 
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
> b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
> index 73ad02aea2b2..328d45b8b2ec 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
> @@ -885,7 +885,7 @@ static int gmc_v6_0_sw_init(void *handle)
>          pci_set_consistent_dma_mask(adev->pdev, DMA_BIT_MASK(32));
>          dev_warn(adev->dev, "amdgpu: No coherent DMA available.\n");
>      }
> -    adev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
> +    adev->need_swiotlb = drm_need_swiotlb_for_dma(dma_bits);
> 
>      r = gmc_v6_0_init_microcode(adev);
>      if (r) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
> b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
> index 910c4ce19cb3..3d49eff28448 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
> @@ -1029,7 +1029,7 @@ static int gmc_v7_0_sw_init(void *handle)
>          pci_set_consistent_dma_mask(adev->pdev, DMA_BIT_MASK(32));
>          pr_warn("amdgpu: No coherent DMA available\n");
>      }
> -    adev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
> +    adev->need_swiotlb = drm_need_swiotlb_for_dma(dma_bits);
> 
>      r = gmc_v7_0_init_microcode(adev);
>      if (r) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> index 747c068379dc..9247dd6316f1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> @@ -1155,7 +1155,7 @@ static int gmc_v8_0_sw_init(void *handle)
>          pci_set_consistent_dma_mask(adev->pdev, DMA_BIT_MASK(32));
>          pr_warn("amdgpu: No coherent DMA available\n");
>      }
> -    adev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
> +    adev->need_swiotlb = drm_need_swiotlb_for_dma(dma_bits);
> 
>      r = gmc_v8_0_init_microcode(adev);
>      if (r) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index f35d7a554ad5..89f3fe981ac5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -989,7 +989,7 @@ static int gmc_v9_0_sw_init(void *handle)
>          pci_set_consistent_dma_mask(adev->pdev, DMA_BIT_MASK(32));
>          printk(KERN_WARNING "amdgpu: No coherent DMA available.\n");
>      }
> -    adev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
> +    adev->need_swiotlb = drm_need_swiotlb_for_dma(dma_bits);
> 
>      if (adev->asic_type == CHIP_VEGA20) {
>          r = gfxhub_v1_1_get_xgmi_info(adev);
> diff --git a/drivers/gpu/drm/drm_memory.c b/drivers/gpu/drm/drm_memory.c
> index d69e4fc1ee77..f22f6a0d20b3 100644
> --- a/drivers/gpu/drm/drm_memory.c
> +++ b/drivers/gpu/drm/drm_memory.c
> @@ -35,6 +35,7 @@
> 
>  #include <linux/highmem.h>
>  #include <linux/export.h>
> +#include <xen/xen.h>
>  #include <drm/drmP.h>
>  #include "drm_legacy.h"
> 
> @@ -150,15 +151,24 @@ void drm_legacy_ioremapfree(struct drm_local_map
> *map, struct drm_device *dev)
>  }
>  EXPORT_SYMBOL(drm_legacy_ioremapfree);
> 
> -u64 drm_get_max_iomem(void)
> +bool drm_need_swiotlb_for_dma(int dma_bits)
>  {
>      struct resource *tmp;
>      resource_size_t max_iomem = 0;
> 
> +#ifdef CONFIG_PARAVIRT
> +    /*
> +     * Paravirtual hosts require swiotlb regardless of requested dma
> +     * transfer size.
> +     */
> +    if (strcmp(pv_info.name, "bare hardware") != 0)
> +        return true;
> +#endif
> +
>      for (tmp = iomem_resource.child; tmp; tmp = tmp->sibling) {
>          max_iomem = max(max_iomem,  tmp->end);
>      }
> 
> -    return max_iomem;
> +    return max_iomem > ((u64)1 << dma_bits);
>  }
> -EXPORT_SYMBOL(drm_get_max_iomem);
> +EXPORT_SYMBOL(drm_need_swiotlb_for_dma);
> diff --git a/drivers/gpu/drm/radeon/radeon_device.c
> b/drivers/gpu/drm/radeon/radeon_device.c
> index 59c8a6647ff2..7c8222d98bc3 100644
> --- a/drivers/gpu/drm/radeon/radeon_device.c
> +++ b/drivers/gpu/drm/radeon/radeon_device.c
> @@ -1387,7 +1387,8 @@ int radeon_device_init(struct radeon_device *rdev,
>          pci_set_consistent_dma_mask(rdev->pdev, DMA_BIT_MASK(32));
>          pr_warn("radeon: No coherent DMA available\n");
>      }
> -    rdev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
> +    rdev->need_swiotlb = drm_need_swiotlb_for_dma(dma_bits);
> +    pr_info("%s: need_swiotlb: %d\n", __func__, rdev->need_swiotlb);
> 
>      /* Registers mapping */
>      /* TODO: block userspace mapping of io register */
> diff --git a/include/drm/drm_cache.h b/include/drm/drm_cache.h
> index bfe1639df02d..070b44624ff9 100644
> --- a/include/drm/drm_cache.h
> +++ b/include/drm/drm_cache.h
> @@ -38,7 +38,7 @@
>  void drm_clflush_pages(struct page *pages[], unsigned long num_pages);
>  void drm_clflush_sg(struct sg_table *st);
>  void drm_clflush_virt_range(void *addr, unsigned long length);
> -u64 drm_get_max_iomem(void);
> +bool drm_need_swiotlb_for_dma(int dma_bits);
> 
> 
>  static inline bool drm_arch_can_wc_memory(void)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Upstream Dom0 DRM problems regarding swiotlb

[Michael Labriola]

On Wed, Feb 13, 2019 at 2:16 PM Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
>
> On Wed, Feb 13, 2019 at 01:38:21PM -0500, Michael Labriola wrote:
> > On Wed, Feb 13, 2019 at 1:16 PM Michael Labriola
> > <michael.d.labriola@gmail.com> wrote:
> > >
> > > On Wed, Feb 13, 2019 at 11:57 AM Konrad Rzeszutek Wilk
> > > <konrad.wilk@oracle.com> wrote:
> > > >
> > > > On Wed, Feb 13, 2019 at 09:09:32AM -0700, Jan Beulich wrote:
> > > > > >>> On 13.02.19 at 17:00, <michael.d.labriola@gmail.com> wrote:
> > > > > > On Wed, Feb 13, 2019 at 9:28 AM Jan Beulich <JBeulich@suse.com> wrote:
> > > > > >> >>> On 13.02.19 at 15:10, <michael.d.labriola@gmail.com> wrote:
> > > > > >> > Ah, so this isn't necessarily Xen-specific but rather any paravirtual
> > > > > >> > guest?  That hadn't crossed my mind.  Is there an easy way to find out
> > > > > >> > if we're a pv guest in the need_swiotlb conditionals?
> > > > > >>
> > > > > >> There's xen_pv_domain(), but I think xen_swiotlb would be more to
> > > > > >> the point if the check is already to be Xen-specific. There's no generic
> > > > > >> "is PV" predicate that I'm aware of.
> > > > > >
> > > > > > Well, that makes doing conditional code right more difficult.  I
> > > > > > assume since there isn't a generic predicate, and PV isn't new, that
> > > > > > it's absence is by design?  To reign in the temptation to sprinkle
> > > > > > conditional code all over the kernel?  ;-)
> > > > >
> > > > > Well, with lguest gone, Xen is the only PV environment the kernel
> > > > > can run in, afaik at least. I guess to decide between the suggested
> > > > > options or the need for some abstracting macro (or yet something
> > > > > else), you'll really need to ask the driver maintainers. Or simply
> > > > > send a patch their way implementing one of them, and see what
> > > > > their reaction is.
> > > >
> > > > Could you try this out and see if it works and I will send it out:
> > > >
> > *snip*
> > >
> > > Yes, that works for me.  However, I feel like the conditional should
> > > be in drm_get_max_iomem() instead of directly after it everywhere it's
> > > used...  and is just checking xen_pv_domain() enough?  Jan made it
> > > sound like there were possibly other PV cases that would also still
> > > need swiotlb.
> >
> > How about this?  It strcmp's pv_info to see if we're bare metal, does
> > the comparison in a single place, moves the bit shifting comparison
> > into the function (simplifying the drm driver code), and renames the
> > function to more aptly describe what's going on.
>
> <nods> That looks much better.

Great!  Now the only question left is:  KVM, VMware, Xen PVH, Xen HVM,
and Xen PV all populate pv_info.  Do any of those other than Xen PV
*really* need swiotlb.  That's slightly over my head.  As written, my
patch would require swiotlb for all of them because I was attempting
to not be Xen-specific.

> Would love to see this posted upstream!

I'm assuming dri-devel is the appropriate place to post this?

> >
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
> > b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
> > index 73ad02aea2b2..328d45b8b2ec 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
> > @@ -885,7 +885,7 @@ static int gmc_v6_0_sw_init(void *handle)
> >          pci_set_consistent_dma_mask(adev->pdev, DMA_BIT_MASK(32));
> >          dev_warn(adev->dev, "amdgpu: No coherent DMA available.\n");
> >      }
> > -    adev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
> > +    adev->need_swiotlb = drm_need_swiotlb_for_dma(dma_bits);
> >
> >      r = gmc_v6_0_init_microcode(adev);
> >      if (r) {
> > diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
> > b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
> > index 910c4ce19cb3..3d49eff28448 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
> > @@ -1029,7 +1029,7 @@ static int gmc_v7_0_sw_init(void *handle)
> >          pci_set_consistent_dma_mask(adev->pdev, DMA_BIT_MASK(32));
> >          pr_warn("amdgpu: No coherent DMA available\n");
> >      }
> > -    adev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
> > +    adev->need_swiotlb = drm_need_swiotlb_for_dma(dma_bits);
> >
> >      r = gmc_v7_0_init_microcode(adev);
> >      if (r) {
> > diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> > b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> > index 747c068379dc..9247dd6316f1 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> > @@ -1155,7 +1155,7 @@ static int gmc_v8_0_sw_init(void *handle)
> >          pci_set_consistent_dma_mask(adev->pdev, DMA_BIT_MASK(32));
> >          pr_warn("amdgpu: No coherent DMA available\n");
> >      }
> > -    adev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
> > +    adev->need_swiotlb = drm_need_swiotlb_for_dma(dma_bits);
> >
> >      r = gmc_v8_0_init_microcode(adev);
> >      if (r) {
> > diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> > b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> > index f35d7a554ad5..89f3fe981ac5 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> > @@ -989,7 +989,7 @@ static int gmc_v9_0_sw_init(void *handle)
> >          pci_set_consistent_dma_mask(adev->pdev, DMA_BIT_MASK(32));
> >          printk(KERN_WARNING "amdgpu: No coherent DMA available.\n");
> >      }
> > -    adev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
> > +    adev->need_swiotlb = drm_need_swiotlb_for_dma(dma_bits);
> >
> >      if (adev->asic_type == CHIP_VEGA20) {
> >          r = gfxhub_v1_1_get_xgmi_info(adev);
> > diff --git a/drivers/gpu/drm/drm_memory.c b/drivers/gpu/drm/drm_memory.c
> > index d69e4fc1ee77..f22f6a0d20b3 100644
> > --- a/drivers/gpu/drm/drm_memory.c
> > +++ b/drivers/gpu/drm/drm_memory.c
> > @@ -35,6 +35,7 @@
> >
> >  #include <linux/highmem.h>
> >  #include <linux/export.h>
> > +#include <xen/xen.h>
> >  #include <drm/drmP.h>
> >  #include "drm_legacy.h"
> >
> > @@ -150,15 +151,24 @@ void drm_legacy_ioremapfree(struct drm_local_map
> > *map, struct drm_device *dev)
> >  }
> >  EXPORT_SYMBOL(drm_legacy_ioremapfree);
> >
> > -u64 drm_get_max_iomem(void)
> > +bool drm_need_swiotlb_for_dma(int dma_bits)
> >  {
> >      struct resource *tmp;
> >      resource_size_t max_iomem = 0;
> >
> > +#ifdef CONFIG_PARAVIRT
> > +    /*
> > +     * Paravirtual hosts require swiotlb regardless of requested dma
> > +     * transfer size.
> > +     */
> > +    if (strcmp(pv_info.name, "bare hardware") != 0)
> > +        return true;
> > +#endif
> > +
> >      for (tmp = iomem_resource.child; tmp; tmp = tmp->sibling) {
> >          max_iomem = max(max_iomem,  tmp->end);
> >      }
> >
> > -    return max_iomem;
> > +    return max_iomem > ((u64)1 << dma_bits);
> >  }
> > -EXPORT_SYMBOL(drm_get_max_iomem);
> > +EXPORT_SYMBOL(drm_need_swiotlb_for_dma);
> > diff --git a/drivers/gpu/drm/radeon/radeon_device.c
> > b/drivers/gpu/drm/radeon/radeon_device.c
> > index 59c8a6647ff2..7c8222d98bc3 100644
> > --- a/drivers/gpu/drm/radeon/radeon_device.c
> > +++ b/drivers/gpu/drm/radeon/radeon_device.c
> > @@ -1387,7 +1387,8 @@ int radeon_device_init(struct radeon_device *rdev,
> >          pci_set_consistent_dma_mask(rdev->pdev, DMA_BIT_MASK(32));
> >          pr_warn("radeon: No coherent DMA available\n");
> >      }
> > -    rdev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
> > +    rdev->need_swiotlb = drm_need_swiotlb_for_dma(dma_bits);
> > +    pr_info("%s: need_swiotlb: %d\n", __func__, rdev->need_swiotlb);
> >
> >      /* Registers mapping */
> >      /* TODO: block userspace mapping of io register */
> > diff --git a/include/drm/drm_cache.h b/include/drm/drm_cache.h
> > index bfe1639df02d..070b44624ff9 100644
> > --- a/include/drm/drm_cache.h
> > +++ b/include/drm/drm_cache.h
> > @@ -38,7 +38,7 @@
> >  void drm_clflush_pages(struct page *pages[], unsigned long num_pages);
> >  void drm_clflush_sg(struct sg_table *st);
> >  void drm_clflush_virt_range(void *addr, unsigned long length);
> > -u64 drm_get_max_iomem(void);
> > +bool drm_need_swiotlb_for_dma(int dma_bits);
> >
> >
> >  static inline bool drm_arch_can_wc_memory(void)



-- 
Michael D Labriola
21 Rip Van Winkle Cir
Warwick, RI 02886
401-316-9844 (cell)
401-848-8871 (work)
401-234-1306 (home)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Upstream Dom0 DRM problems regarding swiotlb

[Andrew Cooper]

On 13/02/2019 20:15, Michael Labriola wrote:
> On Wed, Feb 13, 2019 at 2:16 PM Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com> wrote:
>> On Wed, Feb 13, 2019 at 01:38:21PM -0500, Michael Labriola wrote:
>>> On Wed, Feb 13, 2019 at 1:16 PM Michael Labriola
>>> <michael.d.labriola@gmail.com> wrote:
>>>> On Wed, Feb 13, 2019 at 11:57 AM Konrad Rzeszutek Wilk
>>>> <konrad.wilk@oracle.com> wrote:
>>>>> On Wed, Feb 13, 2019 at 09:09:32AM -0700, Jan Beulich wrote:
>>>>>>>>> On 13.02.19 at 17:00, <michael.d.labriola@gmail.com> wrote:
>>>>>>> On Wed, Feb 13, 2019 at 9:28 AM Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>>>>> On 13.02.19 at 15:10, <michael.d.labriola@gmail.com> wrote:
>>>>>>>>> Ah, so this isn't necessarily Xen-specific but rather any paravirtual
>>>>>>>>> guest?  That hadn't crossed my mind.  Is there an easy way to find out
>>>>>>>>> if we're a pv guest in the need_swiotlb conditionals?
>>>>>>>> There's xen_pv_domain(), but I think xen_swiotlb would be more to
>>>>>>>> the point if the check is already to be Xen-specific. There's no generic
>>>>>>>> "is PV" predicate that I'm aware of.
>>>>>>> Well, that makes doing conditional code right more difficult.  I
>>>>>>> assume since there isn't a generic predicate, and PV isn't new, that
>>>>>>> it's absence is by design?  To reign in the temptation to sprinkle
>>>>>>> conditional code all over the kernel?  ;-)
>>>>>> Well, with lguest gone, Xen is the only PV environment the kernel
>>>>>> can run in, afaik at least. I guess to decide between the suggested
>>>>>> options or the need for some abstracting macro (or yet something
>>>>>> else), you'll really need to ask the driver maintainers. Or simply
>>>>>> send a patch their way implementing one of them, and see what
>>>>>> their reaction is.
>>>>> Could you try this out and see if it works and I will send it out:
>>>>>
>>> *snip*
>>>> Yes, that works for me.  However, I feel like the conditional should
>>>> be in drm_get_max_iomem() instead of directly after it everywhere it's
>>>> used...  and is just checking xen_pv_domain() enough?  Jan made it
>>>> sound like there were possibly other PV cases that would also still
>>>> need swiotlb.
>>> How about this?  It strcmp's pv_info to see if we're bare metal, does
>>> the comparison in a single place, moves the bit shifting comparison
>>> into the function (simplifying the drm driver code), and renames the
>>> function to more aptly describe what's going on.
>> <nods> That looks much better.
> Great!  Now the only question left is:  KVM, VMware, Xen PVH, Xen HVM,
> and Xen PV all populate pv_info.  Do any of those other than Xen PV
> *really* need swiotlb.  That's slightly over my head.  As written, my
> patch would require swiotlb for all of them because I was attempting
> to not be Xen-specific.

Its far more complicated that "Xen PV requires swiotlb".

I presume the underlying problem here is DRM being special and not
DMA-mapping its buffers, and trying to DMA to a buffer crossing a 4k
boundary?

Buffers sitting entirely within one 4k frame never need the swiotlb
unless you've only got a 32-bit capable graphics card, and there is
separate mode dma-mapping mode in the process of being upstreamed where
frames which most of Linux things are adjacent do appear adjacent in
device-virtual address space.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Upstream Dom0 DRM problems regarding swiotlb

[Michael Labriola]

On Wed, Feb 13, 2019 at 3:21 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 13/02/2019 20:15, Michael Labriola wrote:
> > On Wed, Feb 13, 2019 at 2:16 PM Konrad Rzeszutek Wilk
> > <konrad.wilk@oracle.com> wrote:
> >> On Wed, Feb 13, 2019 at 01:38:21PM -0500, Michael Labriola wrote:
> >>> On Wed, Feb 13, 2019 at 1:16 PM Michael Labriola
> >>> <michael.d.labriola@gmail.com> wrote:
> >>>> On Wed, Feb 13, 2019 at 11:57 AM Konrad Rzeszutek Wilk
> >>>> <konrad.wilk@oracle.com> wrote:
> >>>>> On Wed, Feb 13, 2019 at 09:09:32AM -0700, Jan Beulich wrote:
> >>>>>>>>> On 13.02.19 at 17:00, <michael.d.labriola@gmail.com> wrote:
> >>>>>>> On Wed, Feb 13, 2019 at 9:28 AM Jan Beulich <JBeulich@suse.com> wrote:
> >>>>>>>>>>> On 13.02.19 at 15:10, <michael.d.labriola@gmail.com> wrote:
> >>>>>>>>> Ah, so this isn't necessarily Xen-specific but rather any paravirtual
> >>>>>>>>> guest?  That hadn't crossed my mind.  Is there an easy way to find out
> >>>>>>>>> if we're a pv guest in the need_swiotlb conditionals?
> >>>>>>>> There's xen_pv_domain(), but I think xen_swiotlb would be more to
> >>>>>>>> the point if the check is already to be Xen-specific. There's no generic
> >>>>>>>> "is PV" predicate that I'm aware of.
> >>>>>>> Well, that makes doing conditional code right more difficult.  I
> >>>>>>> assume since there isn't a generic predicate, and PV isn't new, that
> >>>>>>> it's absence is by design?  To reign in the temptation to sprinkle
> >>>>>>> conditional code all over the kernel?  ;-)
> >>>>>> Well, with lguest gone, Xen is the only PV environment the kernel
> >>>>>> can run in, afaik at least. I guess to decide between the suggested
> >>>>>> options or the need for some abstracting macro (or yet something
> >>>>>> else), you'll really need to ask the driver maintainers. Or simply
> >>>>>> send a patch their way implementing one of them, and see what
> >>>>>> their reaction is.
> >>>>> Could you try this out and see if it works and I will send it out:
> >>>>>
> >>> *snip*
> >>>> Yes, that works for me.  However, I feel like the conditional should
> >>>> be in drm_get_max_iomem() instead of directly after it everywhere it's
> >>>> used...  and is just checking xen_pv_domain() enough?  Jan made it
> >>>> sound like there were possibly other PV cases that would also still
> >>>> need swiotlb.
> >>> How about this?  It strcmp's pv_info to see if we're bare metal, does
> >>> the comparison in a single place, moves the bit shifting comparison
> >>> into the function (simplifying the drm driver code), and renames the
> >>> function to more aptly describe what's going on.
> >> <nods> That looks much better.
> > Great!  Now the only question left is:  KVM, VMware, Xen PVH, Xen HVM,
> > and Xen PV all populate pv_info.  Do any of those other than Xen PV
> > *really* need swiotlb.  That's slightly over my head.  As written, my
> > patch would require swiotlb for all of them because I was attempting
> > to not be Xen-specific.
>
> Its far more complicated that "Xen PV requires swiotlb".
>
> I presume the underlying problem here is DRM being special and not
> DMA-mapping its buffers, and trying to DMA to a buffer crossing a 4k
> boundary?

Well, I don't 100% understand how all these things work...  but here's
what I do know.  There are a series of commits in v4.17 that try to
optimize the radeon and amdgpu drivers by skipping calls to
ttm_dma_populate() and ttm_dma_unpopulate() unless they're "really
needed".  The original commit determines if swiotlb is needed by
checking to see if the max io mapping address of system memory is over
the video card's accessing range.  I can no longer log into Gnome on a
Xen dom0 after upgrading my kernel to v4.20 because the code that's no
longer happening was actually needed in a paravirtualized environment.

So, I'm trying to get all my details straight so I can submit a patch
to fix it w/out saying anything factually incorrect.

> Buffers sitting entirely within one 4k frame never need the swiotlb
> unless you've only got a 32-bit capable graphics card, and there is
> separate mode dma-mapping mode in the process of being upstreamed where
> frames which most of Linux things are adjacent do appear adjacent in
> device-virtual address space.
>
> ~Andrew

-- 
Michael D Labriola
21 Rip Van Winkle Cir
Warwick, RI 02886
401-316-9844 (cell)
401-848-8871 (work)
401-234-1306 (home)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Upstream Dom0 DRM problems regarding swiotlb

[Andrew Cooper]

On 13/02/2019 21:08, Michael Labriola wrote:
> On Wed, Feb 13, 2019 at 3:21 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 13/02/2019 20:15, Michael Labriola wrote:
>>> On Wed, Feb 13, 2019 at 2:16 PM Konrad Rzeszutek Wilk
>>> <konrad.wilk@oracle.com> wrote:
>>>> On Wed, Feb 13, 2019 at 01:38:21PM -0500, Michael Labriola wrote:
>>>>> On Wed, Feb 13, 2019 at 1:16 PM Michael Labriola
>>>>> <michael.d.labriola@gmail.com> wrote:
>>>>>> On Wed, Feb 13, 2019 at 11:57 AM Konrad Rzeszutek Wilk
>>>>>> <konrad.wilk@oracle.com> wrote:
>>>>>>> On Wed, Feb 13, 2019 at 09:09:32AM -0700, Jan Beulich wrote:
>>>>>>>>>>> On 13.02.19 at 17:00, <michael.d.labriola@gmail.com> wrote:
>>>>>>>>> On Wed, Feb 13, 2019 at 9:28 AM Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>>>>>>> On 13.02.19 at 15:10, <michael.d.labriola@gmail.com> wrote:
>>>>>>>>>>> Ah, so this isn't necessarily Xen-specific but rather any paravirtual
>>>>>>>>>>> guest?  That hadn't crossed my mind.  Is there an easy way to find out
>>>>>>>>>>> if we're a pv guest in the need_swiotlb conditionals?
>>>>>>>>>> There's xen_pv_domain(), but I think xen_swiotlb would be more to
>>>>>>>>>> the point if the check is already to be Xen-specific. There's no generic
>>>>>>>>>> "is PV" predicate that I'm aware of.
>>>>>>>>> Well, that makes doing conditional code right more difficult.  I
>>>>>>>>> assume since there isn't a generic predicate, and PV isn't new, that
>>>>>>>>> it's absence is by design?  To reign in the temptation to sprinkle
>>>>>>>>> conditional code all over the kernel?  ;-)
>>>>>>>> Well, with lguest gone, Xen is the only PV environment the kernel
>>>>>>>> can run in, afaik at least. I guess to decide between the suggested
>>>>>>>> options or the need for some abstracting macro (or yet something
>>>>>>>> else), you'll really need to ask the driver maintainers. Or simply
>>>>>>>> send a patch their way implementing one of them, and see what
>>>>>>>> their reaction is.
>>>>>>> Could you try this out and see if it works and I will send it out:
>>>>>>>
>>>>> *snip*
>>>>>> Yes, that works for me.  However, I feel like the conditional should
>>>>>> be in drm_get_max_iomem() instead of directly after it everywhere it's
>>>>>> used...  and is just checking xen_pv_domain() enough?  Jan made it
>>>>>> sound like there were possibly other PV cases that would also still
>>>>>> need swiotlb.
>>>>> How about this?  It strcmp's pv_info to see if we're bare metal, does
>>>>> the comparison in a single place, moves the bit shifting comparison
>>>>> into the function (simplifying the drm driver code), and renames the
>>>>> function to more aptly describe what's going on.
>>>> <nods> That looks much better.
>>> Great!  Now the only question left is:  KVM, VMware, Xen PVH, Xen HVM,
>>> and Xen PV all populate pv_info.  Do any of those other than Xen PV
>>> *really* need swiotlb.  That's slightly over my head.  As written, my
>>> patch would require swiotlb for all of them because I was attempting
>>> to not be Xen-specific.
>> Its far more complicated that "Xen PV requires swiotlb".
>>
>> I presume the underlying problem here is DRM being special and not
>> DMA-mapping its buffers, and trying to DMA to a buffer crossing a 4k
>> boundary?
> Well, I don't 100% understand how all these things work...  but here's
> what I do know.  There are a series of commits in v4.17 that try to
> optimize the radeon and amdgpu drivers by skipping calls to
> ttm_dma_populate() and ttm_dma_unpopulate() unless they're "really
> needed".  The original commit determines if swiotlb is needed by
> checking to see if the max io mapping address of system memory is over
> the video card's accessing range.  I can no longer log into Gnome on a
> Xen dom0 after upgrading my kernel to v4.20 because the code that's no
> longer happening was actually needed in a paravirtualized environment.

But from the log you provided, your bug was space exhaustion in the
swiotlb, no?

> So, I'm trying to get all my details straight so I can submit a patch
> to fix it w/out saying anything factually incorrect.

The thing which is different between Xen PV guests and most others (all
others(?), now that Lguest and UML have been dropped) is that what Linux
thinks of as PFN $N isn't necessarily adjacent to PFN $N+1 in system
physical address space.

Therefore, code which has a buffer spanning a page boundary can't just
convert a pointer to the buffer into a physical address, and hand that
address to a device.  You generally end up with either memory corruption
(DMA hitting the wrong page allocated to the guest), or an IOMMU fault
(DMA hitting a pages which isn't allocated to the guest).

Xen PV is very good at finding DMA bugs in drivers.  The way to resolve
this is to fix the driver to use the proper DMA APIs - not to add even
more magic corner cases.

In general, a lot of devices can do 4k scatter/gather, or end up making
requests to buffers which fit within a single page, but the SWIOTLB does
act as a mechanism of last resort.  It has a massive performance penalty
(due to double buffering), and does have a tendency to fragment (due to
asymmetric size requests).

However, there is one DMA mode (in the process of getting properly
upstream, but has been used for several years by various downstreams)
where IOVA == Linux's idea of contiguous PFN space, so you can do odd
sized DMAs which cross page boundaries.

The point is that the DMA ops (and *only* the DMA ops, from a
correctness standpoint) know how to convert PFNs into IO-virtual
addresses for devices, because it may not be a 1:1 mapping.  Nothing
else in the kernel can legitimately be making decisions like this.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Upstream Dom0 DRM problems regarding swiotlb

[Juergen Gross]

On 14/02/2019 01:11, Andrew Cooper wrote:
> On 13/02/2019 21:08, Michael Labriola wrote:
>> On Wed, Feb 13, 2019 at 3:21 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>> On 13/02/2019 20:15, Michael Labriola wrote:
>>>> On Wed, Feb 13, 2019 at 2:16 PM Konrad Rzeszutek Wilk
>>>> <konrad.wilk@oracle.com> wrote:
>>>>> On Wed, Feb 13, 2019 at 01:38:21PM -0500, Michael Labriola wrote:
>>>>>> On Wed, Feb 13, 2019 at 1:16 PM Michael Labriola
>>>>>> <michael.d.labriola@gmail.com> wrote:
>>>>>>> On Wed, Feb 13, 2019 at 11:57 AM Konrad Rzeszutek Wilk
>>>>>>> <konrad.wilk@oracle.com> wrote:
>>>>>>>> On Wed, Feb 13, 2019 at 09:09:32AM -0700, Jan Beulich wrote:
>>>>>>>>>>>> On 13.02.19 at 17:00, <michael.d.labriola@gmail.com> wrote:
>>>>>>>>>> On Wed, Feb 13, 2019 at 9:28 AM Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>>>>>>>> On 13.02.19 at 15:10, <michael.d.labriola@gmail.com> wrote:
>>>>>>>>>>>> Ah, so this isn't necessarily Xen-specific but rather any paravirtual
>>>>>>>>>>>> guest?  That hadn't crossed my mind.  Is there an easy way to find out
>>>>>>>>>>>> if we're a pv guest in the need_swiotlb conditionals?
>>>>>>>>>>> There's xen_pv_domain(), but I think xen_swiotlb would be more to
>>>>>>>>>>> the point if the check is already to be Xen-specific. There's no generic
>>>>>>>>>>> "is PV" predicate that I'm aware of.
>>>>>>>>>> Well, that makes doing conditional code right more difficult.  I
>>>>>>>>>> assume since there isn't a generic predicate, and PV isn't new, that
>>>>>>>>>> it's absence is by design?  To reign in the temptation to sprinkle
>>>>>>>>>> conditional code all over the kernel?  ;-)
>>>>>>>>> Well, with lguest gone, Xen is the only PV environment the kernel
>>>>>>>>> can run in, afaik at least. I guess to decide between the suggested
>>>>>>>>> options or the need for some abstracting macro (or yet something
>>>>>>>>> else), you'll really need to ask the driver maintainers. Or simply
>>>>>>>>> send a patch their way implementing one of them, and see what
>>>>>>>>> their reaction is.
>>>>>>>> Could you try this out and see if it works and I will send it out:
>>>>>>>>
>>>>>> *snip*
>>>>>>> Yes, that works for me.  However, I feel like the conditional should
>>>>>>> be in drm_get_max_iomem() instead of directly after it everywhere it's
>>>>>>> used...  and is just checking xen_pv_domain() enough?  Jan made it
>>>>>>> sound like there were possibly other PV cases that would also still
>>>>>>> need swiotlb.
>>>>>> How about this?  It strcmp's pv_info to see if we're bare metal, does
>>>>>> the comparison in a single place, moves the bit shifting comparison
>>>>>> into the function (simplifying the drm driver code), and renames the
>>>>>> function to more aptly describe what's going on.
>>>>> <nods> That looks much better.
>>>> Great!  Now the only question left is:  KVM, VMware, Xen PVH, Xen HVM,
>>>> and Xen PV all populate pv_info.  Do any of those other than Xen PV
>>>> *really* need swiotlb.  That's slightly over my head.  As written, my
>>>> patch would require swiotlb for all of them because I was attempting
>>>> to not be Xen-specific.
>>> Its far more complicated that "Xen PV requires swiotlb".
>>>
>>> I presume the underlying problem here is DRM being special and not
>>> DMA-mapping its buffers, and trying to DMA to a buffer crossing a 4k
>>> boundary?
>> Well, I don't 100% understand how all these things work...  but here's
>> what I do know.  There are a series of commits in v4.17 that try to
>> optimize the radeon and amdgpu drivers by skipping calls to
>> ttm_dma_populate() and ttm_dma_unpopulate() unless they're "really
>> needed".  The original commit determines if swiotlb is needed by
>> checking to see if the max io mapping address of system memory is over
>> the video card's accessing range.  I can no longer log into Gnome on a
>> Xen dom0 after upgrading my kernel to v4.20 because the code that's no
>> longer happening was actually needed in a paravirtualized environment.
> 
> But from the log you provided, your bug was space exhaustion in the
> swiotlb, no?
> 
>> So, I'm trying to get all my details straight so I can submit a patch
>> to fix it w/out saying anything factually incorrect.
> 
> The thing which is different between Xen PV guests and most others (all
> others(?), now that Lguest and UML have been dropped) is that what Linux
> thinks of as PFN $N isn't necessarily adjacent to PFN $N+1 in system
> physical address space.
> 
> Therefore, code which has a buffer spanning a page boundary can't just
> convert a pointer to the buffer into a physical address, and hand that
> address to a device.  You generally end up with either memory corruption
> (DMA hitting the wrong page allocated to the guest), or an IOMMU fault
> (DMA hitting a pages which isn't allocated to the guest).
> 
> Xen PV is very good at finding DMA bugs in drivers.  The way to resolve
> this is to fix the driver to use the proper DMA APIs - not to add even
> more magic corner cases.
> 
> In general, a lot of devices can do 4k scatter/gather, or end up making
> requests to buffers which fit within a single page, but the SWIOTLB does
> act as a mechanism of last resort.  It has a massive performance penalty
> (due to double buffering), and does have a tendency to fragment (due to
> asymmetric size requests).
> 
> However, there is one DMA mode (in the process of getting properly
> upstream, but has been used for several years by various downstreams)
> where IOVA == Linux's idea of contiguous PFN space, so you can do odd
> sized DMAs which cross page boundaries.
> 
> The point is that the DMA ops (and *only* the DMA ops, from a
> correctness standpoint) know how to convert PFNs into IO-virtual
> addresses for devices, because it may not be a 1:1 mapping.  Nothing
> else in the kernel can legitimately be making decisions like this.

Correct. Adding Christoph who might want to add something.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Upstream Dom0 DRM problems regarding swiotlb

[Christoph Hellwig]

On Thu, Feb 14, 2019 at 07:03:38AM +0100, Juergen Gross wrote:
> > The thing which is different between Xen PV guests and most others (all
> > others(?), now that Lguest and UML have been dropped) is that what Linux
> > thinks of as PFN $N isn't necessarily adjacent to PFN $N+1 in system
> > physical address space.
> > 
> > Therefore, code which has a buffer spanning a page boundary can't just
> > convert a pointer to the buffer into a physical address, and hand that
> > address to a device.  You generally end up with either memory corruption
> > (DMA hitting the wrong page allocated to the guest), or an IOMMU fault
> > (DMA hitting a pages which isn't allocated to the guest).

The Linux DMA API allows for dma_map_page / dma_map_single calls to
spawn 4k boundaries.  If Xen doesn't support that it will have to bounce
buffer for that case (and get horrible performance).

But the latter text seems to agree with that.  So what is the actual
problem that started this discussion?

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Upstream Dom0 DRM problems regarding swiotlb

[Juergen Gross]

On 14/02/2019 18:57, Christoph Hellwig wrote:
> On Thu, Feb 14, 2019 at 07:03:38AM +0100, Juergen Gross wrote:
>>> The thing which is different between Xen PV guests and most others (all
>>> others(?), now that Lguest and UML have been dropped) is that what Linux
>>> thinks of as PFN $N isn't necessarily adjacent to PFN $N+1 in system
>>> physical address space.
>>>
>>> Therefore, code which has a buffer spanning a page boundary can't just
>>> convert a pointer to the buffer into a physical address, and hand that
>>> address to a device.  You generally end up with either memory corruption
>>> (DMA hitting the wrong page allocated to the guest), or an IOMMU fault
>>> (DMA hitting a pages which isn't allocated to the guest).
> 
> The Linux DMA API allows for dma_map_page / dma_map_single calls to
> spawn 4k boundaries.  If Xen doesn't support that it will have to bounce
> buffer for that case (and get horrible performance).
> 
> But the latter text seems to agree with that.  So what is the actual
> problem that started this discussion?
> 

See https://lists.xen.org/archives/html/xen-devel/2019-02/threads.html#00818


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Upstream Dom0 DRM problems regarding swiotlb

[Michael Labriola]

On Fri, Feb 15, 2019 at 12:57 AM Juergen Gross <jgross@suse.com> wrote:
>
> On 14/02/2019 18:57, Christoph Hellwig wrote:
> > On Thu, Feb 14, 2019 at 07:03:38AM +0100, Juergen Gross wrote:
> >>> The thing which is different between Xen PV guests and most others (all
> >>> others(?), now that Lguest and UML have been dropped) is that what Linux
> >>> thinks of as PFN $N isn't necessarily adjacent to PFN $N+1 in system
> >>> physical address space.
> >>>
> >>> Therefore, code which has a buffer spanning a page boundary can't just
> >>> convert a pointer to the buffer into a physical address, and hand that
> >>> address to a device.  You generally end up with either memory corruption
> >>> (DMA hitting the wrong page allocated to the guest), or an IOMMU fault
> >>> (DMA hitting a pages which isn't allocated to the guest).
> >
> > The Linux DMA API allows for dma_map_page / dma_map_single calls to
> > spawn 4k boundaries.  If Xen doesn't support that it will have to bounce
> > buffer for that case (and get horrible performance).
> >
> > But the latter text seems to agree with that.  So what is the actual
> > problem that started this discussion?
> >
>
> See https://lists.xen.org/archives/html/xen-devel/2019-02/threads.html#00818

I believe the actual problem is either:

1) The radeon/amdgpu drivers are calling ttm_populate_and_map_pages()
which *should* work on a Xen PV host, but doesn't and needs to be
fixed.

or

2) The radeon/amdgpu drivers are calling ttm_populate_and_map_pages()
which *cannot* work in Xen, and they should go back to calling
ttm_dma_populate() in that case.

I'm having a hard time figuring out which of those is correct.

-- 
Michael D Labriola
21 Rip Van Winkle Cir
Warwick, RI 02886
401-316-9844 (cell)
401-848-8871 (work)
401-234-1306 (home)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Upstream Dom0 DRM problems regarding swiotlb

[Konrad Rzeszutek Wilk]

On Fri, Feb 15, 2019 at 11:07:22AM -0500, Michael Labriola wrote:
> On Fri, Feb 15, 2019 at 12:57 AM Juergen Gross <jgross@suse.com> wrote:
> >
> > On 14/02/2019 18:57, Christoph Hellwig wrote:
> > > On Thu, Feb 14, 2019 at 07:03:38AM +0100, Juergen Gross wrote:
> > >>> The thing which is different between Xen PV guests and most others (all
> > >>> others(?), now that Lguest and UML have been dropped) is that what Linux
> > >>> thinks of as PFN $N isn't necessarily adjacent to PFN $N+1 in system
> > >>> physical address space.
> > >>>
> > >>> Therefore, code which has a buffer spanning a page boundary can't just
> > >>> convert a pointer to the buffer into a physical address, and hand that
> > >>> address to a device.  You generally end up with either memory corruption
> > >>> (DMA hitting the wrong page allocated to the guest), or an IOMMU fault
> > >>> (DMA hitting a pages which isn't allocated to the guest).
> > >
> > > The Linux DMA API allows for dma_map_page / dma_map_single calls to
> > > spawn 4k boundaries.  If Xen doesn't support that it will have to bounce
> > > buffer for that case (and get horrible performance).
> > >
> > > But the latter text seems to agree with that.  So what is the actual
> > > problem that started this discussion?
> > >
> >
> > See https://lists.xen.org/archives/html/xen-devel/2019-02/threads.html#00818
> 
> I believe the actual problem is either:
> 
> 1) The radeon/amdgpu drivers are calling ttm_populate_and_map_pages()
> which *should* work on a Xen PV host, but doesn't and needs to be
> fixed.
> 
> or
> 
> 2) The radeon/amdgpu drivers are calling ttm_populate_and_map_pages()
> which *cannot* work in Xen, and they should go back to calling
> ttm_dma_populate() in that case.

The Nvidia one has this (correct):

1583 #if IS_ENABLED(CONFIG_SWIOTLB) && IS_ENABLED(CONFIG_X86)                        
1584         if (swiotlb_nr_tbl()) {                                                 
1585                 return ttm_dma_populate((void *)ttm, dev, ctx);                 
1586         }                                                                       
1587 #endif  

The Radeon has this - where now it adds 'need_swiotlb':

695 #ifdef CONFIG_SWIOTLB                                                           
 696         if (rdev->need_swiotlb && swiotlb_nr_tbl()) {                           
 697                 return ttm_dma_populate(&gtt->ttm, rdev->dev, ctx);             
 698         }                                                                       
 699 #endif   

The problem is fairly simple - the platform _requires_ to use
DMA API.

But the driver's have their own 'need_swiotlb' which ignores the platform
and sets it based on the device's DMA width:

  rdev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);

There should be an extra check to see if the platform requires
to use DMA API.

> 
> I'm having a hard time figuring out which of those is correct.
> 
> -- 
> Michael D Labriola
> 21 Rip Van Winkle Cir
> Warwick, RI 02886
> 401-316-9844 (cell)
> 401-848-8871 (work)
> 401-234-1306 (home)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Upstream Dom0 DRM problems regarding swiotlb

[Konrad Rzeszutek Wilk via dri-devel]

On Fri, Feb 15, 2019 at 11:07:22AM -0500, Michael Labriola wrote:
> On Fri, Feb 15, 2019 at 12:57 AM Juergen Gross <jgross@suse.com> wrote:
> >
> > On 14/02/2019 18:57, Christoph Hellwig wrote:
> > > On Thu, Feb 14, 2019 at 07:03:38AM +0100, Juergen Gross wrote:
> > >>> The thing which is different between Xen PV guests and most others (all
> > >>> others(?), now that Lguest and UML have been dropped) is that what Linux
> > >>> thinks of as PFN $N isn't necessarily adjacent to PFN $N+1 in system
> > >>> physical address space.
> > >>>
> > >>> Therefore, code which has a buffer spanning a page boundary can't just
> > >>> convert a pointer to the buffer into a physical address, and hand that
> > >>> address to a device.  You generally end up with either memory corruption
> > >>> (DMA hitting the wrong page allocated to the guest), or an IOMMU fault
> > >>> (DMA hitting a pages which isn't allocated to the guest).
> > >
> > > The Linux DMA API allows for dma_map_page / dma_map_single calls to
> > > spawn 4k boundaries.  If Xen doesn't support that it will have to bounce
> > > buffer for that case (and get horrible performance).
> > >
> > > But the latter text seems to agree with that.  So what is the actual
> > > problem that started this discussion?
> > >
> >
> > See https://lists.xen.org/archives/html/xen-devel/2019-02/threads.html#00818
> 
> I believe the actual problem is either:
> 
> 1) The radeon/amdgpu drivers are calling ttm_populate_and_map_pages()
> which *should* work on a Xen PV host, but doesn't and needs to be
> fixed.
> 
> or
> 
> 2) The radeon/amdgpu drivers are calling ttm_populate_and_map_pages()
> which *cannot* work in Xen, and they should go back to calling
> ttm_dma_populate() in that case.

The Nvidia one has this (correct):

1583 #if IS_ENABLED(CONFIG_SWIOTLB) && IS_ENABLED(CONFIG_X86)                        
1584         if (swiotlb_nr_tbl()) {                                                 
1585                 return ttm_dma_populate((void *)ttm, dev, ctx);                 
1586         }                                                                       
1587 #endif  

The Radeon has this - where now it adds 'need_swiotlb':

695 #ifdef CONFIG_SWIOTLB                                                           
 696         if (rdev->need_swiotlb && swiotlb_nr_tbl()) {                           
 697                 return ttm_dma_populate(&gtt->ttm, rdev->dev, ctx);             
 698         }                                                                       
 699 #endif   

The problem is fairly simple - the platform _requires_ to use
DMA API.

But the driver's have their own 'need_swiotlb' which ignores the platform
and sets it based on the device's DMA width:

  rdev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);

There should be an extra check to see if the platform requires
to use DMA API.

> 
> I'm having a hard time figuring out which of those is correct.
> 
> -- 
> Michael D Labriola
> 21 Rip Van Winkle Cir
> Warwick, RI 02886
> 401-316-9844 (cell)
> 401-848-8871 (work)
> 401-234-1306 (home)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Upstream Dom0 DRM problems regarding swiotlb

[Christoph Hellwig]

On Fri, Feb 15, 2019 at 11:07:22AM -0500, Michael Labriola wrote:
> > > But the latter text seems to agree with that.  So what is the actual
> > > problem that started this discussion?
> > >
> >
> > See https://lists.xen.org/archives/html/xen-devel/2019-02/threads.html#00818
> 
> I believe the actual problem is either:
> 
> 1) The radeon/amdgpu drivers are calling ttm_populate_and_map_pages()
> which *should* work on a Xen PV host, but doesn't and needs to be
> fixed.
> 
> or
> 
> 2) The radeon/amdgpu drivers are calling ttm_populate_and_map_pages()
> which *cannot* work in Xen, and they should go back to calling
> ttm_dma_populate() in that case.
> 
> I'm having a hard time figuring out which of those is correct.

I think the answer is neither 1 or 2 (or a bit of both).

ttm_populate_and_map_pages uses dma_map_page to map GPU memory,
and from what I can tell from your report potentially lots of it.

So this does "properly" use the DMA API for some amount of "properly".
The problem here is that ttm_populate_and_map_pages first allocates
memory and then maps it in a way where it bounce buffers a lot,
leading to a swiotlb buffer exaustion, as seen in your report.

ttm_dma_populate also sort of "properly" uses the DMA API in that it
uses the dma_alloc_coherent allocator.  The benefit of that allocator is
that is always returns addressable memory without exhausing the swiotlb
buffer.  The dowside of ttm_dma_populate / dma_alloc_coherent is that
on architectures where PCIe is not cache coherent it pointlessly
up other resources, as coherent DMA memory can be a very finite resource
there.

So for a short term fix forcing the dma_alloc_coherent route on
Xen/x86 is the right thing.  On Xen/arm and Xen/arm64 is might already
be problemeatic due to the explanation above unfortunately.

The real fix is to finally get broadly available non-coherent device
memory allocator into mainlaine and with ttm_populate_and_map_pages
to use it.  Note that for non-coherent devices it seems like
ttm_populate_and_map_pages also seems to miss proper ownership
management but that is another issue.

My proposal for such an allocator here:

   https://lwn.net/Articles/774429/

unfortunately doesn't seem to go anywhere as the DRM folks generally
seem to prefer bolting band-aid ontop of band-aid instead of actually
fixing such problems.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Upstream Dom0 DRM problems regarding swiotlb

[Juergen Gross]

On 13/02/2019 19:38, Michael Labriola wrote:
> On Wed, Feb 13, 2019 at 1:16 PM Michael Labriola
> <michael.d.labriola@gmail.com> wrote:
>>
>> On Wed, Feb 13, 2019 at 11:57 AM Konrad Rzeszutek Wilk
>> <konrad.wilk@oracle.com> wrote:
>>>
>>> On Wed, Feb 13, 2019 at 09:09:32AM -0700, Jan Beulich wrote:
>>>>>>> On 13.02.19 at 17:00, <michael.d.labriola@gmail.com> wrote:
>>>>> On Wed, Feb 13, 2019 at 9:28 AM Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>>> On 13.02.19 at 15:10, <michael.d.labriola@gmail.com> wrote:
>>>>>>> Ah, so this isn't necessarily Xen-specific but rather any paravirtual
>>>>>>> guest?  That hadn't crossed my mind.  Is there an easy way to find out
>>>>>>> if we're a pv guest in the need_swiotlb conditionals?
>>>>>>
>>>>>> There's xen_pv_domain(), but I think xen_swiotlb would be more to
>>>>>> the point if the check is already to be Xen-specific. There's no generic
>>>>>> "is PV" predicate that I'm aware of.
>>>>>
>>>>> Well, that makes doing conditional code right more difficult.  I
>>>>> assume since there isn't a generic predicate, and PV isn't new, that
>>>>> it's absence is by design?  To reign in the temptation to sprinkle
>>>>> conditional code all over the kernel?  ;-)
>>>>
>>>> Well, with lguest gone, Xen is the only PV environment the kernel
>>>> can run in, afaik at least. I guess to decide between the suggested
>>>> options or the need for some abstracting macro (or yet something
>>>> else), you'll really need to ask the driver maintainers. Or simply
>>>> send a patch their way implementing one of them, and see what
>>>> their reaction is.
>>>
>>> Could you try this out and see if it works and I will send it out:
>>>
> *snip*
>>
>> Yes, that works for me.  However, I feel like the conditional should
>> be in drm_get_max_iomem() instead of directly after it everywhere it's
>> used...  and is just checking xen_pv_domain() enough?  Jan made it
>> sound like there were possibly other PV cases that would also still
>> need swiotlb.
> 
> How about this?  It strcmp's pv_info to see if we're bare metal, does
> the comparison in a single place, moves the bit shifting comparison
> into the function (simplifying the drm driver code), and renames the
> function to more aptly describe what's going on.

...

> diff --git a/drivers/gpu/drm/drm_memory.c b/drivers/gpu/drm/drm_memory.c
> index d69e4fc1ee77..f22f6a0d20b3 100644
> --- a/drivers/gpu/drm/drm_memory.c
> +++ b/drivers/gpu/drm/drm_memory.c
> @@ -35,6 +35,7 @@
> 
>  #include <linux/highmem.h>
>  #include <linux/export.h>
> +#include <xen/xen.h>
>  #include <drm/drmP.h>
>  #include "drm_legacy.h"
> 
> @@ -150,15 +151,24 @@ void drm_legacy_ioremapfree(struct drm_local_map
> *map, struct drm_device *dev)
>  }
>  EXPORT_SYMBOL(drm_legacy_ioremapfree);
> 
> -u64 drm_get_max_iomem(void)
> +bool drm_need_swiotlb_for_dma(int dma_bits)
>  {
>      struct resource *tmp;
>      resource_size_t max_iomem = 0;
> 
> +#ifdef CONFIG_PARAVIRT
> +    /*
> +     * Paravirtual hosts require swiotlb regardless of requested dma
> +     * transfer size.
> +     */
> +    if (strcmp(pv_info.name, "bare hardware") != 0)
> +        return true;
> +#endif
> +

No, this is really not acceptable.

Apart from Andrew's comments on using the DMA API (which I really
support) relying on the pv_info.name string is a very bad interface.
You'd need to add something like a "need_swiotlb" boolean to the
pv_info struct and set it for Xen PV and maybe others.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Upstream Dom0 DRM problems regarding swiotlb

[Jan Beulich]

>>> On 13.02.19 at 17:56, <konrad.wilk@oracle.com> wrote:
> @@ -887,6 +888,8 @@ static int gmc_v6_0_sw_init(void *handle)
>  		dev_warn(adev->dev, "amdgpu: No coherent DMA available.\n");
>  	}
>  	adev->need_swiotlb = drm_get_max_iomem() > ((u64)1 << dma_bits);
> +	if (xen_pv_domain())
> +		adev->need_swiotlb = 1;

But changes like this go too far imo, at least when taking into
account the logic in pci_xen_swiotlb_detect(). Hence my earlier
suggestion to use xen_swiotlb instead of xen_pv_domain().

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Upstream Dom0 DRM problems regarding swiotlb

[Michael Labriola]

On Wed, Feb 13, 2019 at 11:09 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 13.02.19 at 17:00, <michael.d.labriola@gmail.com> wrote:
> > On Wed, Feb 13, 2019 at 9:28 AM Jan Beulich <JBeulich@suse.com> wrote:
> >> >>> On 13.02.19 at 15:10, <michael.d.labriola@gmail.com> wrote:
> >> > Ah, so this isn't necessarily Xen-specific but rather any paravirtual
> >> > guest?  That hadn't crossed my mind.  Is there an easy way to find out
> >> > if we're a pv guest in the need_swiotlb conditionals?
> >>
> >> There's xen_pv_domain(), but I think xen_swiotlb would be more to
> >> the point if the check is already to be Xen-specific. There's no generic
> >> "is PV" predicate that I'm aware of.
> >
> > Well, that makes doing conditional code right more difficult.  I
> > assume since there isn't a generic predicate, and PV isn't new, that
> > it's absence is by design?  To reign in the temptation to sprinkle
> > conditional code all over the kernel?  ;-)
>
> Well, with lguest gone, Xen is the only PV environment the kernel
> can run in, afaik at least. I guess to decide between the suggested
> options or the need for some abstracting macro (or yet something
> else), you'll really need to ask the driver maintainers. Or simply
> send a patch their way implementing one of them, and see what
> their reaction is.

Thanks, I'll do that.

When you said any PV guest would need swiotlb, not just Xen, does that
mean anything that's using CONFIG_PARAVIRT?  That appears to include
KVM, VMware, Xen PVH, and Xen HVM in addition to Xen PV, all of which
populate the global pv_info structure at kernel bootup.  Is Xen PV the
only one of those that requires swiotlb?

-Mike

-- 
Michael D Labriola
21 Rip Van Winkle Cir
Warwick, RI 02886
401-316-9844 (cell)
401-848-8871 (work)
401-234-1306 (home)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Upstream Dom0 DRM problems regarding swiotlb

[Jan Beulich]

>>> On 13.02.19 at 18:45, <michael.d.labriola@gmail.com> wrote:
> On Wed, Feb 13, 2019 at 11:09 AM Jan Beulich <JBeulich@suse.com> wrote:
>>
>> >>> On 13.02.19 at 17:00, <michael.d.labriola@gmail.com> wrote:
>> > On Wed, Feb 13, 2019 at 9:28 AM Jan Beulich <JBeulich@suse.com> wrote:
>> >> >>> On 13.02.19 at 15:10, <michael.d.labriola@gmail.com> wrote:
>> >> > Ah, so this isn't necessarily Xen-specific but rather any paravirtual
>> >> > guest?  That hadn't crossed my mind.  Is there an easy way to find out
>> >> > if we're a pv guest in the need_swiotlb conditionals?
>> >>
>> >> There's xen_pv_domain(), but I think xen_swiotlb would be more to
>> >> the point if the check is already to be Xen-specific. There's no generic
>> >> "is PV" predicate that I'm aware of.
>> >
>> > Well, that makes doing conditional code right more difficult.  I
>> > assume since there isn't a generic predicate, and PV isn't new, that
>> > it's absence is by design?  To reign in the temptation to sprinkle
>> > conditional code all over the kernel?  ;-)
>>
>> Well, with lguest gone, Xen is the only PV environment the kernel
>> can run in, afaik at least. I guess to decide between the suggested
>> options or the need for some abstracting macro (or yet something
>> else), you'll really need to ask the driver maintainers. Or simply
>> send a patch their way implementing one of them, and see what
>> their reaction is.
> 
> Thanks, I'll do that.
> 
> When you said any PV guest would need swiotlb, not just Xen, does that
> mean anything that's using CONFIG_PARAVIRT?  That appears to include
> KVM, VMware, Xen PVH, and Xen HVM in addition to Xen PV, all of which
> populate the global pv_info structure at kernel bootup.  Is Xen PV the
> only one of those that requires swiotlb?

No - paravirtual interfaces can also be used by fully virtualized guests.
As said, to my knowledge Xen is the only environment in which the
Linux kernel supports running paravirtualized.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel