* [PATCH v5 1/3] drm/shmem: add support for per object caching flags.
[not found] <20200226154752.24328-1-kraxel@redhat.com>
@ 2020-02-26 15:47 ` Gerd Hoffmann
2020-02-26 16:51 ` Guillaume Gardet
2020-02-26 18:24 ` Thomas Hellström (VMware)
2020-02-26 15:47 ` [PATCH v5 2/3] drm/virtio: fix mmap page attributes Gerd Hoffmann
2020-02-26 15:47 ` [PATCH v5 3/3] drm/udl: simplify gem object mapping Gerd Hoffmann
2 siblings, 2 replies; 19+ messages in thread
From: Gerd Hoffmann @ 2020-02-26 15:47 UTC (permalink / raw)
To: dri-devel
Cc: tzimmermann, gurchetansingh, olvaffe, Guillaume.Gardet,
Gerd Hoffmann, stable, Maarten Lankhorst, Maxime Ripard,
David Airlie, Daniel Vetter, open list
Add map_cached bool to drm_gem_shmem_object, to request cached mappings
on a per-object base. Check the flag before adding writecombine to
pgprot bits.
Cc: stable@vger.kernel.org
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
include/drm/drm_gem_shmem_helper.h | 5 +++++
drivers/gpu/drm/drm_gem_shmem_helper.c | 15 +++++++++++----
2 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
index e34a7b7f848a..294b2931c4cc 100644
--- a/include/drm/drm_gem_shmem_helper.h
+++ b/include/drm/drm_gem_shmem_helper.h
@@ -96,6 +96,11 @@ struct drm_gem_shmem_object {
* The address are un-mapped when the count reaches zero.
*/
unsigned int vmap_use_count;
+
+ /**
+ * @map_cached: map object cached (instead of using writecombine).
+ */
+ bool map_cached;
};
#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);
+ }
if (!shmem->vaddr) {
DRM_DEBUG_KMS("Failed to vmap pages\n");
@@ -540,7 +545,9 @@ int drm_gem_shmem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
}
vma->vm_flags |= VM_MIXEDMAP | VM_DONTEXPAND;
- vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
+ vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
+ if (!shmem->map_cached)
+ vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
vma->vm_ops = &drm_gem_shmem_vm_ops;
--
2.18.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* RE: [PATCH v5 1/3] drm/shmem: add support for per object caching flags.
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)
1 sibling, 0 replies; 19+ messages in thread
From: Guillaume Gardet @ 2020-02-26 16:51 UTC (permalink / raw)
To: Gerd Hoffmann, dri-devel
Cc: tzimmermann, gurchetansingh, olvaffe, stable, Maarten Lankhorst,
Maxime Ripard, David Airlie, Daniel Vetter, open list
> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: 26 February 2020 16:48
> To: dri-devel@lists.freedesktop.org
> Cc: tzimmermann@suse.de; gurchetansingh@chromium.org; olvaffe@gmail.com;
> Guillaume Gardet <Guillaume.Gardet@arm.com>; Gerd Hoffmann
> <kraxel@redhat.com>; stable@vger.kernel.org; Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com>; Maxime Ripard <mripard@kernel.org>;
> David Airlie <airlied@linux.ie>; Daniel Vetter <daniel@ffwll.ch>; open list <linux-
> kernel@vger.kernel.org>
> Subject: [PATCH v5 1/3] drm/shmem: add support for per object caching flags.
>
> Add map_cached bool to drm_gem_shmem_object, to request cached mappings
> on a per-object base. Check the flag before adding writecombine to pgprot bits.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Tested-by: Guillaume Gardet <Guillaume.Gardet@arm.com>
> ---
> include/drm/drm_gem_shmem_helper.h | 5 +++++
> drivers/gpu/drm/drm_gem_shmem_helper.c | 15 +++++++++++----
> 2 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/include/drm/drm_gem_shmem_helper.h
> b/include/drm/drm_gem_shmem_helper.h
> index e34a7b7f848a..294b2931c4cc 100644
> --- a/include/drm/drm_gem_shmem_helper.h
> +++ b/include/drm/drm_gem_shmem_helper.h
> @@ -96,6 +96,11 @@ struct drm_gem_shmem_object {
> * The address are un-mapped when the count reaches zero.
> */
> unsigned int vmap_use_count;
> +
> +/**
> + * @map_cached: map object cached (instead of using writecombine).
> + */
> +bool map_cached;
> };
>
> #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);
> +}
>
> if (!shmem->vaddr) {
> DRM_DEBUG_KMS("Failed to vmap pages\n"); @@ -540,7 +545,9
> @@ int drm_gem_shmem_mmap(struct drm_gem_object *obj, struct
> vm_area_struct *vma)
> }
>
> vma->vm_flags |= VM_MIXEDMAP | VM_DONTEXPAND;
> -vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma-
> >vm_flags));
> +vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> +if (!shmem->map_cached)
> +vma->vm_page_prot = pgprot_writecombine(vma-
> >vm_page_prot);
> vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
> vma->vm_ops = &drm_gem_shmem_vm_ops;
>
> --
> 2.18.2
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v5 1/3] drm/shmem: add support for per object caching flags.
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:53 ` Gerd Hoffmann
1 sibling, 2 replies; 19+ messages in thread
From: Thomas Hellström (VMware) @ 2020-02-26 18:24 UTC (permalink / raw)
To: Gerd Hoffmann, dri-devel
Cc: Guillaume.Gardet, David Airlie, open list, stable,
gurchetansingh, tzimmermann
Hi, Gerd,
While looking at this patchset I came across some stuff that seems
strange but that was merged in a previous patchset.
(please refer to
https://lists.freedesktop.org/archives/dri-devel/2018-September/190001.html.
Forgive me if I've missed any discussion leading up to this).
On 2/26/20 4:47 PM, Gerd Hoffmann wrote:
> Add map_cached bool to drm_gem_shmem_object, to request cached mappings
> on a per-object base. Check the flag before adding writecombine to
> pgprot bits.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> include/drm/drm_gem_shmem_helper.h | 5 +++++
> drivers/gpu/drm/drm_gem_shmem_helper.c | 15 +++++++++++----
> 2 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
> index e34a7b7f848a..294b2931c4cc 100644
> --- a/include/drm/drm_gem_shmem_helper.h
> +++ b/include/drm/drm_gem_shmem_helper.h
> @@ -96,6 +96,11 @@ struct drm_gem_shmem_object {
> * The address are un-mapped when the count reaches zero.
> */
> unsigned int vmap_use_count;
> +
> + /**
> + * @map_cached: map object cached (instead of using writecombine).
> + */
> + bool map_cached;
> };
>
> #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.
> + }
>
> if (!shmem->vaddr) {
> DRM_DEBUG_KMS("Failed to vmap pages\n");
> @@ -540,7 +545,9 @@ int drm_gem_shmem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
> }
>
> vma->vm_flags |= VM_MIXEDMAP | VM_DONTEXPAND;
> - vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
> + vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> + if (!shmem->map_cached)
> + vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
Same thing here. Note that vmf_insert_page() which is used by the fault
handler also bypasses the x86 PAT consistency check, whereas
vmf_insert_mixed() doesn't.
> vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
At least with SME or SEV encryption, where shmem memory has its kernel
map set to encrypted, creating conflicting mappings is explicitly
disallowed.
BTW, why is mmap mapping decrypted while vmap isn't?
> vma->vm_ops = &drm_gem_shmem_vm_ops;
>
Thanks,
Thomas
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v5 1/3] drm/shmem: add support for per object caching flags.
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)
2020-02-27 7:53 ` Gerd Hoffmann
1 sibling, 1 reply; 19+ messages in thread
From: Chia-I Wu @ 2020-02-27 0:02 UTC (permalink / raw)
To: Thomas Hellström (VMware)
Cc: Gerd Hoffmann, ML dri-devel, Guillaume Gardet, David Airlie,
open list, stable, Gurchetan Singh, tzimmermann
On Wed, Feb 26, 2020 at 10:25 AM Thomas Hellström (VMware)
<thomas_os@shipmail.org> wrote:
>
> Hi, Gerd,
>
> While looking at this patchset I came across some stuff that seems
> strange but that was merged in a previous patchset.
>
> (please refer to
> https://lists.freedesktop.org/archives/dri-devel/2018-September/190001.html.
> Forgive me if I've missed any discussion leading up to this).
>
>
> On 2/26/20 4:47 PM, Gerd Hoffmann wrote:
> > Add map_cached bool to drm_gem_shmem_object, to request cached mappings
> > on a per-object base. Check the flag before adding writecombine to
> > pgprot bits.
> >
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> > include/drm/drm_gem_shmem_helper.h | 5 +++++
> > drivers/gpu/drm/drm_gem_shmem_helper.c | 15 +++++++++++----
> > 2 files changed, 16 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
> > index e34a7b7f848a..294b2931c4cc 100644
> > --- a/include/drm/drm_gem_shmem_helper.h
> > +++ b/include/drm/drm_gem_shmem_helper.h
> > @@ -96,6 +96,11 @@ struct drm_gem_shmem_object {
> > * The address are un-mapped when the count reaches zero.
> > */
> > unsigned int vmap_use_count;
> > +
> > + /**
> > + * @map_cached: map object cached (instead of using writecombine).
> > + */
> > + bool map_cached;
> > };
> >
> > #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.
>
>
> > + }
> >
> > if (!shmem->vaddr) {
> > DRM_DEBUG_KMS("Failed to vmap pages\n");
> > @@ -540,7 +545,9 @@ int drm_gem_shmem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
> > }
> >
> > vma->vm_flags |= VM_MIXEDMAP | VM_DONTEXPAND;
> > - vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
> > + vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> > + if (!shmem->map_cached)
> > + vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
>
> Same thing here. Note that vmf_insert_page() which is used by the fault
> handler also bypasses the x86 PAT consistency check, whereas
> vmf_insert_mixed() doesn't.
>
> > vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
>
> At least with SME or SEV encryption, where shmem memory has its kernel
> map set to encrypted, creating conflicting mappings is explicitly
> disallowed.
> BTW, why is mmap mapping decrypted while vmap isn't?
>
> > vma->vm_ops = &drm_gem_shmem_vm_ops;
> >
>
> Thanks,
> Thomas
>
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v5 1/3] drm/shmem: add support for per object caching flags.
2020-02-27 0:02 ` Chia-I Wu
@ 2020-02-27 7:16 ` Thomas Hellström (VMware)
0 siblings, 0 replies; 19+ messages in thread
From: Thomas Hellström (VMware) @ 2020-02-27 7:16 UTC (permalink / raw)
To: Chia-I Wu
Cc: Gerd Hoffmann, ML dri-devel, Guillaume Gardet, David Airlie,
open list, stable, Gurchetan Singh, tzimmermann
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v5 1/3] drm/shmem: add support for per object caching flags.
2020-02-26 18:24 ` Thomas Hellström (VMware)
2020-02-27 0:02 ` Chia-I Wu
@ 2020-02-27 7:53 ` Gerd Hoffmann
2020-02-27 8:10 ` Thomas Hellström (VMware)
1 sibling, 1 reply; 19+ messages in thread
From: Gerd Hoffmann @ 2020-02-27 7:53 UTC (permalink / raw)
To: Thomas Hellström (VMware)
Cc: dri-devel, Guillaume.Gardet, David Airlie, open list, stable,
gurchetansingh, tzimmermann
Hi,
> > + 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?
I think so, yes.
> Or do you change the linear kernel map of the shmem pages somewhere?
Havn't seen anything doing so while browsing the code.
> vmap bypassess at least the
> x86 PAT core mapping consistency check and this could potentially cause
> spuriously overwritten memory.
Well, I don't think the linear kernel map is ever used to access the
shmem gem objects. So while this isn't exactly clean it shouldn't
cause problems in practice.
Suggestions how to fix that?
The reason I need cachable gem object mappings for virtio-gpu is because
we have a inconsistency between host (cached) and guest (wc) otherwise.
> > + }
> > if (!shmem->vaddr) {
> > DRM_DEBUG_KMS("Failed to vmap pages\n");
> > @@ -540,7 +545,9 @@ int drm_gem_shmem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
> > }
> > vma->vm_flags |= VM_MIXEDMAP | VM_DONTEXPAND;
> > - vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
> > + vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> > + if (!shmem->map_cached)
> > + vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
>
> Same thing here. Note that vmf_insert_page() which is used by the fault
> handler also bypasses the x86 PAT consistency check, whereas
> vmf_insert_mixed() doesn't.
vmap + mmap are consistent though, so this likewise shouldn't cause
issues in practice.
> > vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
>
> At least with SME or SEV encryption, where shmem memory has its kernel map
> set to encrypted, creating conflicting mappings is explicitly disallowed.
> BTW, why is mmap mapping decrypted while vmap isn't?
Ok, that sounds like a real problem. Have to check.
cheers,
Gerd
PS: Given we are discussing pre-existing issues in the code I think the
series can be merged nevertheless.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v5 1/3] drm/shmem: add support for per object caching flags.
2020-02-27 7:53 ` Gerd Hoffmann
@ 2020-02-27 8:10 ` Thomas Hellström (VMware)
2020-02-27 10:56 ` Gerd Hoffmann
0 siblings, 1 reply; 19+ messages in thread
From: Thomas Hellström (VMware) @ 2020-02-27 8:10 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: dri-devel, Guillaume.Gardet, David Airlie, open list, stable,
gurchetansingh, tzimmermann
On 2/27/20 8:53 AM, Gerd Hoffmann wrote:
> Hi,
>
>>> + 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?
> I think so, yes.
>
>> Or do you change the linear kernel map of the shmem pages somewhere?
> Havn't seen anything doing so while browsing the code.
>
>> vmap bypassess at least the
>> x86 PAT core mapping consistency check and this could potentially cause
>> spuriously overwritten memory.
> Well, I don't think the linear kernel map is ever used to access the
> shmem gem objects. So while this isn't exactly clean it shouldn't
> cause problems in practice.
>
> Suggestions how to fix that?
>
So this has historically caused problems since the linear kernel map has
been accessed while prefetching, even if it's never used. Some
processors like AMD athlon actually even wrote back the prefetched
contents without ever using it.
Also the linear kernel map could be cached somewhere because of the
page's previous usage. (hibernation for example?)
I think it might be safe for some integrated graphics where the driver
maintainers can guarantee that it's safe on all particular processors
used with that driver, but then IMO it should be moved out to those drivers.
Other drivers needing write-combine shouldn't really use shmem.
So again, to fix the regression, could we revert 0be895893607f
("drm/shmem: switch shmem helper to &drm_gem_object_funcs.mmap") or does
that have other implications?
/Thomas
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v5 1/3] drm/shmem: add support for per object caching flags.
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)
0 siblings, 1 reply; 19+ messages in thread
From: Gerd Hoffmann @ 2020-02-27 10:56 UTC (permalink / raw)
To: Thomas Hellström (VMware)
Cc: dri-devel, Guillaume.Gardet, David Airlie, open list, stable,
gurchetansingh, tzimmermann
Hi,
> I think it might be safe for some integrated graphics where the driver
> maintainers can guarantee that it's safe on all particular processors used
> with that driver, but then IMO it should be moved out to those drivers.
>
> Other drivers needing write-combine shouldn't really use shmem.
>
> So again, to fix the regression, could we revert 0be895893607f ("drm/shmem:
> switch shmem helper to &drm_gem_object_funcs.mmap") or does that have other
> implications?
This patch isn't a regression. The old code path has the
pgprot_writecombine() call in drm_gem_mmap_obj(), so the behavior
is the same before and afterwards.
But with the patch in place we can easily have shmem helpers do their
own thing instead of depending on whatever drm_gem_mmap_obj() is doing.
Just using cached mappings unconditionally would be perfectly fine for
virtio-gpu.
Not sure about the other users though. I'd like to fix the virtio-gpu
regression (coming from ttm -> shmem switch) asap, and I don't feel like
changing the behavior for other drivers in 5.6-rc is a good idea.
So I'd like to push patches 1+2 to -fixes and sort everything else later
in -next. OK?
cheers,
Gerd
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v5 1/3] drm/shmem: add support for per object caching flags.
2020-02-27 10:56 ` Gerd Hoffmann
@ 2020-02-27 12:16 ` Thomas Hellström (VMware)
2020-02-27 13:21 ` Gerd Hoffmann
0 siblings, 1 reply; 19+ messages in thread
From: Thomas Hellström (VMware) @ 2020-02-27 12:16 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: dri-devel, Guillaume.Gardet, David Airlie, open list, stable,
gurchetansingh, tzimmermann
On 2/27/20 11:56 AM, Gerd Hoffmann wrote:
> Hi,
>
>> I think it might be safe for some integrated graphics where the driver
>> maintainers can guarantee that it's safe on all particular processors used
>> with that driver, but then IMO it should be moved out to those drivers.
>>
>> Other drivers needing write-combine shouldn't really use shmem.
>>
>> So again, to fix the regression, could we revert 0be895893607f ("drm/shmem:
>> switch shmem helper to &drm_gem_object_funcs.mmap") or does that have other
>> implications?
> This patch isn't a regression. The old code path has the
> pgprot_writecombine() call in drm_gem_mmap_obj(), so the behavior
> is the same before and afterwards.
OK. I wasn't checking where this all came from from the start...
> But with the patch in place we can easily have shmem helpers do their
> own thing instead of depending on whatever drm_gem_mmap_obj() is doing.
> Just using cached mappings unconditionally would be perfectly fine for
> virtio-gpu.
>
> Not sure about the other users though. I'd like to fix the virtio-gpu
> regression (coming from ttm -> shmem switch) asap, and I don't feel like
> changing the behavior for other drivers in 5.6-rc is a good idea.
>
> So I'd like to push patches 1+2 to -fixes and sort everything else later
> in -next. OK?
OK with me. Do we have any idea what drivers are actually using
write-combine and decrypted?
/Thomas
>
> cheers,
> Gerd
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v5 1/3] drm/shmem: add support for per object caching flags.
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-03-02 1:56 ` Qiang Yu
0 siblings, 2 replies; 19+ messages in thread
From: Gerd Hoffmann @ 2020-02-27 13:21 UTC (permalink / raw)
To: Thomas Hellström (VMware)
Cc: dri-devel, Guillaume.Gardet, David Airlie, open list,
gurchetansingh, tzimmermann, yuq825, noralf, robh
Hi,
> > So I'd like to push patches 1+2 to -fixes and sort everything else later
> > in -next. OK?
>
> OK with me.
Done.
>> [ context: why shmem helpers use pgprot_writecombine + pgprot_decrypted?
>> we get conflicting mappings because of that, linear kernel
>> map vs. gem object vmap/mmap ]
> Do we have any idea what drivers are actually using
> write-combine and decrypted?
drivers/gpu/drm# find -name Kconfig* -print | xargs grep -l DRM_GEM_SHMEM_HELPER
./lima/Kconfig
./tiny/Kconfig
./cirrus/Kconfig
./Kconfig
./panfrost/Kconfig
./udl/Kconfig
./v3d/Kconfig
./virtio/Kconfig
virtio needs cached.
cirrus+udl should be ok with cached too.
Not clue about the others (lima, tiny, panfrost, v3d). Maybe they use
write-combine just because this is what they got by default from
drm_gem_mmap_obj(). Maybe they actually need that. Trying to Cc:
maintainters (and drop stable@).
On decrypted: I guess that is only relevant for virtual machines, i.e.
virtio-gpu and cirrus?
virtio-gpu needs it, otherwise the host can't show the virtual display.
cirrus bounces everything via blits to vram, so it should be ok without
decrypted. I guess that implies we should make decrypted configurable.
cheers,
Gerd
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v5 1/3] drm/shmem: add support for per object caching flags.
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-03-02 1:56 ` Qiang Yu
1 sibling, 2 replies; 19+ messages in thread
From: Thomas Hellström (VMware) @ 2020-02-27 13:44 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: dri-devel, Guillaume.Gardet, David Airlie, open list,
gurchetansingh, tzimmermann, yuq825, noralf, robh
Hi,
On 2/27/20 2:21 PM, Gerd Hoffmann wrote:
> Hi,
>
>>> So I'd like to push patches 1+2 to -fixes and sort everything else later
>>> in -next. OK?
>> OK with me.
> Done.
>
>>> [ context: why shmem helpers use pgprot_writecombine + pgprot_decrypted?
>>> we get conflicting mappings because of that, linear kernel
>>> map vs. gem object vmap/mmap ]
>> Do we have any idea what drivers are actually using
>> write-combine and decrypted?
> drivers/gpu/drm# find -name Kconfig* -print | xargs grep -l DRM_GEM_SHMEM_HELPER
> ./lima/Kconfig
> ./tiny/Kconfig
> ./cirrus/Kconfig
> ./Kconfig
> ./panfrost/Kconfig
> ./udl/Kconfig
> ./v3d/Kconfig
> ./virtio/Kconfig
>
> virtio needs cached.
> cirrus+udl should be ok with cached too.
>
> Not clue about the others (lima, tiny, panfrost, v3d). Maybe they use
> write-combine just because this is what they got by default from
> drm_gem_mmap_obj(). Maybe they actually need that. Trying to Cc:
> maintainters (and drop stable@).
>
> On decrypted: I guess that is only relevant for virtual machines, i.e.
> virtio-gpu and cirrus?
>
> virtio-gpu needs it, otherwise the host can't show the virtual display.
> cirrus bounces everything via blits to vram, so it should be ok without
> decrypted. I guess that implies we should make decrypted configurable.
Decrypted here is clearly incorrect and violates the SEV spec,
regardless of a config option.
The only correct way is currently to use dma_alloc_coherent() and
mmap_coherent() to allocate decrypted memory and then use the
pgprot_decrypted flag.
Since the same page is aliased with two physical addresses (one
encrypted and one decrypted) switching memory from one to the other
needs extensive handling even to flush stale vmaps...
So if virtio-gpu needs it for some bos, it should move away from shmem
for those bos.
/Thomas
>
> cheers,
> Gerd
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v5 1/3] drm/shmem: add support for per object caching flags.
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
1 sibling, 0 replies; 19+ messages in thread
From: Thomas Hellström (VMware) @ 2020-02-27 13:49 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: dri-devel, Guillaume.Gardet, David Airlie, open list,
gurchetansingh, tzimmermann, yuq825, noralf, robh
On 2/27/20 2:44 PM, Thomas Hellström (VMware) wrote:
> Hi,
>
> On 2/27/20 2:21 PM, Gerd Hoffmann wrote:
>> Hi,
>>
>>>> So I'd like to push patches 1+2 to -fixes and sort everything else
>>>> later
>>>> in -next. OK?
>>> OK with me.
>> Done.
>>
>>>> [ context: why shmem helpers use pgprot_writecombine +
>>>> pgprot_decrypted?
>>>> we get conflicting mappings because of that, linear kernel
>>>> map vs. gem object vmap/mmap ]
>>> Do we have any idea what drivers are actually using
>>> write-combine and decrypted?
>> drivers/gpu/drm# find -name Kconfig* -print | xargs grep -l
>> DRM_GEM_SHMEM_HELPER
>> ./lima/Kconfig
>> ./tiny/Kconfig
>> ./cirrus/Kconfig
>> ./Kconfig
>> ./panfrost/Kconfig
>> ./udl/Kconfig
>> ./v3d/Kconfig
>> ./virtio/Kconfig
>>
>> virtio needs cached.
>> cirrus+udl should be ok with cached too.
>>
>> Not clue about the others (lima, tiny, panfrost, v3d). Maybe they use
>> write-combine just because this is what they got by default from
>> drm_gem_mmap_obj(). Maybe they actually need that. Trying to Cc:
>> maintainters (and drop stable@).
>>
>> On decrypted: I guess that is only relevant for virtual machines, i.e.
>> virtio-gpu and cirrus?
>>
>> virtio-gpu needs it, otherwise the host can't show the virtual display.
>> cirrus bounces everything via blits to vram, so it should be ok without
>> decrypted. I guess that implies we should make decrypted configurable.
>
> Decrypted here is clearly incorrect and violates the SEV spec,
> regardless of a config option.
> The only correct way is currently to use dma_alloc_coherent() and
> mmap_coherent() to allocate decrypted memory and then use the
> pgprot_decrypted flag.
>
> Since the same page is aliased with two physical addresses (one
> encrypted and one decrypted) switching memory from one to the other
> needs extensive handling even to flush stale vmaps...
>
> So if virtio-gpu needs it for some bos, it should move away from shmem
> for those bos.
(But this is of course up to the virtio-gpu and drm maintainers), but
IMO, again, pgprot_decrypted() shouldn't be part of generic helpers.
/Thomas
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v5 1/3] drm/shmem: add support for per object caching flags.
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)
1 sibling, 1 reply; 19+ messages in thread
From: Gerd Hoffmann @ 2020-02-28 9:49 UTC (permalink / raw)
To: Thomas Hellström (VMware)
Cc: dri-devel, Guillaume.Gardet, David Airlie, open list,
gurchetansingh, tzimmermann, yuq825, noralf, robh
Hi,
> > Not clue about the others (lima, tiny, panfrost, v3d). Maybe they use
> > write-combine just because this is what they got by default from
> > drm_gem_mmap_obj(). Maybe they actually need that. Trying to Cc:
> > maintainters (and drop stable@).
> > virtio-gpu needs it, otherwise the host can't show the virtual display.
> > cirrus bounces everything via blits to vram, so it should be ok without
> > decrypted. I guess that implies we should make decrypted configurable.
>
> Decrypted here is clearly incorrect and violates the SEV spec, regardless of
> a config option.
>
> The only correct way is currently to use dma_alloc_coherent() and
> mmap_coherent() to allocate decrypted memory and then use the
> pgprot_decrypted flag.
Hmm, virtio-gpu uses the dma api to allow the host access the gem
object. So I think I have to correct the statement above, if I
understands things correctly the dma api will use (properly allocated)
decrypted bounce buffers and the virtio-gpu shmem objects don't need
pgprot_decrypted mappings.
That leaves the question what to do about pgprot_writecombine(). Any
comments from the driver maintainers (see first paragraph)?
cheers,
Gerd
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v5 1/3] drm/shmem: add support for per object caching flags.
2020-02-28 9:49 ` Gerd Hoffmann
@ 2020-02-28 9:54 ` Thomas Hellström (VMware)
2020-02-28 10:46 ` Gerd Hoffmann
0 siblings, 1 reply; 19+ messages in thread
From: Thomas Hellström (VMware) @ 2020-02-28 9:54 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: dri-devel, Guillaume.Gardet, David Airlie, open list,
gurchetansingh, tzimmermann, yuq825, noralf, robh
On 2/28/20 10:49 AM, Gerd Hoffmann wrote:
> Hi,
>
>>> Not clue about the others (lima, tiny, panfrost, v3d). Maybe they use
>>> write-combine just because this is what they got by default from
>>> drm_gem_mmap_obj(). Maybe they actually need that. Trying to Cc:
>>> maintainters (and drop stable@).
>>> virtio-gpu needs it, otherwise the host can't show the virtual display.
>>> cirrus bounces everything via blits to vram, so it should be ok without
>>> decrypted. I guess that implies we should make decrypted configurable.
>> Decrypted here is clearly incorrect and violates the SEV spec, regardless of
>> a config option.
>>
>> The only correct way is currently to use dma_alloc_coherent() and
>> mmap_coherent() to allocate decrypted memory and then use the
>> pgprot_decrypted flag.
> Hmm, virtio-gpu uses the dma api to allow the host access the gem
> object. So I think I have to correct the statement above, if I
> understands things correctly the dma api will use (properly allocated)
> decrypted bounce buffers and the virtio-gpu shmem objects don't need
> pgprot_decrypted mappings.
Yes, that sounds more correct. I wonder whether the "pgprot_decrypted()"
perhaps remains from mapping VRAM gem buffers...
/Thomas
>
> That leaves the question what to do about pgprot_writecombine(). Any
> comments from the driver maintainers (see first paragraph)?
>
> cheers,
> Gerd
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v5 1/3] drm/shmem: add support for per object caching flags.
2020-02-28 9:54 ` Thomas Hellström (VMware)
@ 2020-02-28 10:46 ` Gerd Hoffmann
0 siblings, 0 replies; 19+ messages in thread
From: Gerd Hoffmann @ 2020-02-28 10:46 UTC (permalink / raw)
To: Thomas Hellström (VMware)
Cc: dri-devel, Guillaume.Gardet, David Airlie, open list,
gurchetansingh, tzimmermann, yuq825, noralf, robh
On Fri, Feb 28, 2020 at 10:54:54AM +0100, Thomas Hellström (VMware) wrote:
> On 2/28/20 10:49 AM, Gerd Hoffmann wrote:
> > Hi,
> >
> > > > Not clue about the others (lima, tiny, panfrost, v3d). Maybe they use
> > > > write-combine just because this is what they got by default from
> > > > drm_gem_mmap_obj(). Maybe they actually need that. Trying to Cc:
> > > > maintainters (and drop stable@).
> > > > virtio-gpu needs it, otherwise the host can't show the virtual display.
> > > > cirrus bounces everything via blits to vram, so it should be ok without
> > > > decrypted. I guess that implies we should make decrypted configurable.
> > > Decrypted here is clearly incorrect and violates the SEV spec, regardless of
> > > a config option.
> > >
> > > The only correct way is currently to use dma_alloc_coherent() and
> > > mmap_coherent() to allocate decrypted memory and then use the
> > > pgprot_decrypted flag.
> > Hmm, virtio-gpu uses the dma api to allow the host access the gem
> > object. So I think I have to correct the statement above, if I
> > understands things correctly the dma api will use (properly allocated)
> > decrypted bounce buffers and the virtio-gpu shmem objects don't need
> > pgprot_decrypted mappings.
>
> Yes, that sounds more correct. I wonder whether the "pgprot_decrypted()"
> perhaps remains from mapping VRAM gem buffers...
Commit 95cf9264d5f3 ("x86, drm, fbdev: Do not specify encrypted memory
for video mappings") added it, then it was kept through various changes.
cheers,
Gerd
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v5 1/3] drm/shmem: add support for per object caching flags.
2020-02-27 13:21 ` Gerd Hoffmann
2020-02-27 13:44 ` Thomas Hellström (VMware)
@ 2020-03-02 1:56 ` Qiang Yu
1 sibling, 0 replies; 19+ messages in thread
From: Qiang Yu @ 2020-03-02 1:56 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Thomas Hellström (VMware),
dri-devel, Guillaume.Gardet, David Airlie, open list,
gurchetansingh, tzimmermann, noralf, Rob Herring
On Thu, Feb 27, 2020 at 9:21 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> Hi,
>
> > > So I'd like to push patches 1+2 to -fixes and sort everything else later
> > > in -next. OK?
> >
> > OK with me.
>
> Done.
>
> >> [ context: why shmem helpers use pgprot_writecombine + pgprot_decrypted?
> >> we get conflicting mappings because of that, linear kernel
> >> map vs. gem object vmap/mmap ]
>
> > Do we have any idea what drivers are actually using
> > write-combine and decrypted?
>
> drivers/gpu/drm# find -name Kconfig* -print | xargs grep -l DRM_GEM_SHMEM_HELPER
> ./lima/Kconfig
> ./tiny/Kconfig
> ./cirrus/Kconfig
> ./Kconfig
> ./panfrost/Kconfig
> ./udl/Kconfig
> ./v3d/Kconfig
> ./virtio/Kconfig
>
> virtio needs cached.
> cirrus+udl should be ok with cached too.
>
> Not clue about the others (lima, tiny, panfrost, v3d). Maybe they use
> write-combine just because this is what they got by default from
> drm_gem_mmap_obj(). Maybe they actually need that. Trying to Cc:
> maintainters (and drop stable@).
>
lima driver needs writecombine mapped buffer for GPU hardware
working properly due to CPU-GPU not cache coherent. Although we
haven't meet problems caused by kernel/user map conflict, but I
do admit it's a problem as I met with amdgpu+arm before.
With TTM we can control page allocated and create a pool for
writecombine pages, but seems shmem is not friendly with
writecombine pages.
Thanks,
Qiang
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v5 2/3] drm/virtio: fix mmap page attributes
[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 15:47 ` 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
2 siblings, 1 reply; 19+ messages in thread
From: Gerd Hoffmann @ 2020-02-26 15:47 UTC (permalink / raw)
To: dri-devel
Cc: tzimmermann, gurchetansingh, olvaffe, Guillaume.Gardet,
Gerd Hoffmann, stable, David Airlie, Daniel Vetter,
open list:VIRTIO GPU DRIVER, open list
virtio-gpu uses cached mappings, set
drm_gem_shmem_object.map_cached accordingly.
Cc: stable@vger.kernel.org
Fixes: c66df701e783 ("drm/virtio: switch from ttm to gem shmem helpers")
Reported-by: Gurchetan Singh <gurchetansingh@chromium.org>
Reported-by: Guillaume Gardet <Guillaume.Gardet@arm.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
drivers/gpu/drm/virtio/virtgpu_object.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c
index 3d2a6d489bfc..59319435218f 100644
--- a/drivers/gpu/drm/virtio/virtgpu_object.c
+++ b/drivers/gpu/drm/virtio/virtgpu_object.c
@@ -119,6 +119,7 @@ struct drm_gem_object *virtio_gpu_create_object(struct drm_device *dev,
return NULL;
bo->base.base.funcs = &virtio_gpu_gem_funcs;
+ bo->base.map_cached = true;
return &bo->base.base;
}
--
2.18.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* RE: [PATCH v5 2/3] drm/virtio: fix mmap page attributes
2020-02-26 15:47 ` [PATCH v5 2/3] drm/virtio: fix mmap page attributes Gerd Hoffmann
@ 2020-02-26 16:52 ` Guillaume Gardet
0 siblings, 0 replies; 19+ messages in thread
From: Guillaume Gardet @ 2020-02-26 16:52 UTC (permalink / raw)
To: Gerd Hoffmann, dri-devel
Cc: tzimmermann, gurchetansingh, olvaffe, stable, David Airlie,
Daniel Vetter, open list:VIRTIO GPU DRIVER, open list
> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: 26 February 2020 16:48
> To: dri-devel@lists.freedesktop.org
> Cc: tzimmermann@suse.de; gurchetansingh@chromium.org; olvaffe@gmail.com;
> Guillaume Gardet <Guillaume.Gardet@arm.com>; Gerd Hoffmann
> <kraxel@redhat.com>; stable@vger.kernel.org; David Airlie <airlied@linux.ie>;
> Daniel Vetter <daniel.vetter@ffwll.ch>; open list:VIRTIO GPU DRIVER
> <virtualization@lists.linux-foundation.org>; open list <linux-
> kernel@vger.kernel.org>
> Subject: [PATCH v5 2/3] drm/virtio: fix mmap page attributes
>
> virtio-gpu uses cached mappings, set
> drm_gem_shmem_object.map_cached accordingly.
>
> Cc: stable@vger.kernel.org
> Fixes: c66df701e783 ("drm/virtio: switch from ttm to gem shmem helpers")
> Reported-by: Gurchetan Singh <gurchetansingh@chromium.org>
> Reported-by: Guillaume Gardet <Guillaume.Gardet@arm.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Tested-by: Guillaume Gardet <Guillaume.Gardet@arm.com>
> ---
> drivers/gpu/drm/virtio/virtgpu_object.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c
> b/drivers/gpu/drm/virtio/virtgpu_object.c
> index 3d2a6d489bfc..59319435218f 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_object.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_object.c
> @@ -119,6 +119,7 @@ struct drm_gem_object *virtio_gpu_create_object(struct
> drm_device *dev,
> return NULL;
>
> bo->base.base.funcs = &virtio_gpu_gem_funcs;
> +bo->base.map_cached = true;
> return &bo->base.base;
> }
>
> --
> 2.18.2
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v5 3/3] drm/udl: simplify gem object mapping.
[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 15:47 ` [PATCH v5 2/3] drm/virtio: fix mmap page attributes Gerd Hoffmann
@ 2020-02-26 15:47 ` Gerd Hoffmann
2 siblings, 0 replies; 19+ messages in thread
From: Gerd Hoffmann @ 2020-02-26 15:47 UTC (permalink / raw)
To: dri-devel
Cc: tzimmermann, gurchetansingh, olvaffe, Guillaume.Gardet,
Gerd Hoffmann, Dave Airlie, Sean Paul, David Airlie,
Daniel Vetter, Sumit Semwal, open list,
open list:DMA BUFFER SHARING FRAMEWORK,
moderated list:DMA BUFFER SHARING FRAMEWORK
With shmem helpers allowing to update pgprot caching flags via
drm_gem_shmem_object.map_cached we can just use that and ditch
our own implementations of mmap() and vmap().
We also don't need a special case for imported objects, any map
requests are handled by the exporter not udl.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
drivers/gpu/drm/udl/udl_gem.c | 62 ++---------------------------------
1 file changed, 3 insertions(+), 59 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
index b6e26f98aa0a..7e3a88b25b6b 100644
--- a/drivers/gpu/drm/udl/udl_gem.c
+++ b/drivers/gpu/drm/udl/udl_gem.c
@@ -17,72 +17,15 @@
* GEM object funcs
*/
-static int udl_gem_object_mmap(struct drm_gem_object *obj,
- struct vm_area_struct *vma)
-{
- int ret;
-
- ret = drm_gem_shmem_mmap(obj, vma);
- if (ret)
- return ret;
-
- vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
- if (obj->import_attach)
- vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
- vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
-
- return 0;
-}
-
-static void *udl_gem_object_vmap(struct drm_gem_object *obj)
-{
- struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
- int ret;
-
- ret = mutex_lock_interruptible(&shmem->vmap_lock);
- if (ret)
- return ERR_PTR(ret);
-
- if (shmem->vmap_use_count++ > 0)
- goto out;
-
- ret = drm_gem_shmem_get_pages(shmem);
- if (ret)
- goto err_zero_use;
-
- if (obj->import_attach)
- shmem->vaddr = dma_buf_vmap(obj->import_attach->dmabuf);
- else
- shmem->vaddr = vmap(shmem->pages, obj->size >> PAGE_SHIFT,
- VM_MAP, PAGE_KERNEL);
-
- if (!shmem->vaddr) {
- DRM_DEBUG_KMS("Failed to vmap pages\n");
- ret = -ENOMEM;
- goto err_put_pages;
- }
-
-out:
- mutex_unlock(&shmem->vmap_lock);
- return shmem->vaddr;
-
-err_put_pages:
- drm_gem_shmem_put_pages(shmem);
-err_zero_use:
- shmem->vmap_use_count = 0;
- mutex_unlock(&shmem->vmap_lock);
- return ERR_PTR(ret);
-}
-
static const struct drm_gem_object_funcs udl_gem_object_funcs = {
.free = drm_gem_shmem_free_object,
.print_info = drm_gem_shmem_print_info,
.pin = drm_gem_shmem_pin,
.unpin = drm_gem_shmem_unpin,
.get_sg_table = drm_gem_shmem_get_sg_table,
- .vmap = udl_gem_object_vmap,
+ .vmap = drm_gem_shmem_vmap,
.vunmap = drm_gem_shmem_vunmap,
- .mmap = udl_gem_object_mmap,
+ .mmap = drm_gem_shmem_mmap,
};
/*
@@ -101,6 +44,7 @@ struct drm_gem_object *udl_driver_gem_create_object(struct drm_device *dev,
obj = &shmem->base;
obj->funcs = &udl_gem_object_funcs;
+ shmem->map_cached = true;
return obj;
}
--
2.18.2
^ permalink raw reply related [flat|nested] 19+ messages in thread