From: "Thomas Hellström (VMware)" <thomas_os@shipmail.org>
To: Chia-I Wu <olvaffe@gmail.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>,
ML dri-devel <dri-devel@lists.freedesktop.org>,
Guillaume Gardet <Guillaume.Gardet@arm.com>,
David Airlie <airlied@linux.ie>,
open list <linux-kernel@vger.kernel.org>,
stable@vger.kernel.org,
Gurchetan Singh <gurchetansingh@chromium.org>,
tzimmermann@suse.de
Subject: Re: [PATCH v5 1/3] drm/shmem: add support for per object caching flags.
Date: Thu, 27 Feb 2020 08:16:49 +0100 [thread overview]
Message-ID: <614ed528-8d6d-0179-6149-218566d4af06@shipmail.org> (raw)
In-Reply-To: <CAPaKu7R4VFYnk9UdpguZnkWeKk2ELVnoQ60=i72RN2GkME1ukw@mail.gmail.com>
On 2/27/20 1:02 AM, Chia-I Wu wrote:
> On Wed, Feb 26, 2020 at 10:25 AM Thomas Hellström (VMware)
> <thomas_os@shipmail.org> wrote:
>> Hi, Gerd,
>>
>>
>>
>> #define to_drm_gem_shmem_obj(obj) \
>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
>> index a421a2eed48a..aad9324dcf4f 100644
>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
>> @@ -254,11 +254,16 @@ static void *drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem)
>> if (ret)
>> goto err_zero_use;
>>
>> - if (obj->import_attach)
>> + if (obj->import_attach) {
>> shmem->vaddr = dma_buf_vmap(obj->import_attach->dmabuf);
>> - else
>> + } else {
>> + pgprot_t prot = PAGE_KERNEL;
>> +
>> + if (!shmem->map_cached)
>> + prot = pgprot_writecombine(prot);
>> shmem->vaddr = vmap(shmem->pages, obj->size >> PAGE_SHIFT,
>> - VM_MAP, pgprot_writecombine(PAGE_KERNEL));
>> + VM_MAP, prot)
>>
>> Wouldn't a vmap with pgprot_writecombine() create conflicting mappings
>> with the linear kernel map which is not write-combined? Or do you change
>> the linear kernel map of the shmem pages somewhere? vmap bypassess at
>> least the x86 PAT core mapping consistency check and this could
>> potentially cause spuriously overwritten memory.
> Yeah, I think this creates a conflicting alias. It seems a call to
> set_pages_array_wc here or changes elsewhere is needed..
>
> But this is a pre-existing issue in the shmem helper. There is also
> no universal fix (e.g., set_pages_array_wc is x86 only)? I would hope
> this series can be merged sooner to fix the regression first.
The problem is this isn't doable with shmem, since that would create
interesting problems when pages are swapped out.
So I would argue that the correct fix is to revert commit 0be895893607f
("drm/shmem: switch shmem helper to &drm_gem_object_funcs.mmap")
If some drivers can argue that in their particular environment it's safe
to use writecombine in this way, then modifying the page protection
should be moved out to those drivers documenting that assumption. Using
pgprot_decrypted() in this way could never be safe.
But IMO this should never be part of generic helpers.
/Thomas
next prev parent reply other threads:[~2020-02-27 7:16 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20200226154752.24328-1-kraxel@redhat.com>
2020-02-26 15:47 ` [PATCH v5 1/3] drm/shmem: add support for per object caching flags Gerd Hoffmann
2020-02-26 16:51 ` Guillaume Gardet
2020-02-26 18:24 ` Thomas Hellström (VMware)
2020-02-27 0:02 ` Chia-I Wu
2020-02-27 7:16 ` Thomas Hellström (VMware) [this message]
2020-02-27 7:53 ` Gerd Hoffmann
2020-02-27 8:10 ` Thomas Hellström (VMware)
2020-02-27 10:56 ` Gerd Hoffmann
2020-02-27 12:16 ` Thomas Hellström (VMware)
2020-02-27 13:21 ` Gerd Hoffmann
2020-02-27 13:44 ` Thomas Hellström (VMware)
2020-02-27 13:49 ` Thomas Hellström (VMware)
2020-02-28 9:49 ` Gerd Hoffmann
2020-02-28 9:54 ` Thomas Hellström (VMware)
2020-02-28 10:46 ` Gerd Hoffmann
2020-03-02 1:56 ` Qiang Yu
2020-02-26 15:47 ` [PATCH v5 2/3] drm/virtio: fix mmap page attributes Gerd Hoffmann
2020-02-26 16:52 ` Guillaume Gardet
2020-02-26 15:47 ` [PATCH v5 3/3] drm/udl: simplify gem object mapping Gerd Hoffmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=614ed528-8d6d-0179-6149-218566d4af06@shipmail.org \
--to=thomas_os@shipmail.org \
--cc=Guillaume.Gardet@arm.com \
--cc=airlied@linux.ie \
--cc=dri-devel@lists.freedesktop.org \
--cc=gurchetansingh@chromium.org \
--cc=kraxel@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=olvaffe@gmail.com \
--cc=stable@vger.kernel.org \
--cc=tzimmermann@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).