[PATCH v3 2/2] vhost-vdpa: fix page pinning leakage in error path

2020-10-02 Thread Si-Wei Liu 
Pinned pages are not properly accounted particularly when
mapping error occurs on IOTLB update. Clean up dangling
pinned pages for the error path. As the inflight pinned
pages, specifically for memory region that strides across
multiple chunks, would need more than one free page for
book keeping and accounting. For simplicity, pin pages
for all memory in the IOVA range in one go rather than
have multiple pin_user_pages calls to make up the entire
region. This way it's easier to track and account the
pages already mapped, particularly for clean-up in the
error path.

Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
Signed-off-by: Si-Wei Liu 
---
Changes in v3:
- Factor out vhost_vdpa_map() change to a separate patch

Changes in v2:
- Fix incorrect target SHA1 referenced

 drivers/vhost/vdpa.c | 119 ++-
 1 file changed, 71 insertions(+), 48 deletions(-)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 0f27919..dad41dae 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -595,21 +595,19 @@ static int vhost_vdpa_process_iotlb_update(struct 
vhost_vdpa *v,
struct vhost_dev *dev = &v->vdev;
struct vhost_iotlb *iotlb = dev->iotlb;
struct page **page_list;
-   unsigned long list_size = PAGE_SIZE / sizeof(struct page *);
+   struct vm_area_struct **vmas;
unsigned int gup_flags = FOLL_LONGTERM;
-   unsigned long npages, cur_base, map_pfn, last_pfn = 0;
-   unsigned long locked, lock_limit, pinned, i;
+   unsigned long map_pfn, last_pfn = 0;
+   unsigned long npages, lock_limit;
+   unsigned long i, nmap = 0;
u64 iova = msg->iova;
+   long pinned;
int ret = 0;
 
if (vhost_iotlb_itree_first(iotlb, msg->iova,
msg->iova + msg->size - 1))
return -EEXIST;
 
-   page_list = (struct page **) __get_free_page(GFP_KERNEL);
-   if (!page_list)
-   return -ENOMEM;
-
if (msg->perm & VHOST_ACCESS_WO)
gup_flags |= FOLL_WRITE;
 
@@ -617,61 +615,86 @@ static int vhost_vdpa_process_iotlb_update(struct 
vhost_vdpa *v,
if (!npages)
return -EINVAL;
 
+   page_list = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
+   vmas = kvmalloc_array(npages, sizeof(struct vm_area_struct *),
+ GFP_KERNEL);
+   if (!page_list || !vmas) {
+   ret = -ENOMEM;
+   goto free;
+   }
+
mmap_read_lock(dev->mm);
 
-   locked = atomic64_add_return(npages, &dev->mm->pinned_vm);
lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
-
-   if (locked > lock_limit) {
+   if (npages + atomic64_read(&dev->mm->pinned_vm) > lock_limit) {
ret = -ENOMEM;
-   goto out;
+   goto unlock;
}
 
-   cur_base = msg->uaddr & PAGE_MASK;
-   iova &= PAGE_MASK;
+   pinned = pin_user_pages(msg->uaddr & PAGE_MASK, npages, gup_flags,
+   page_list, vmas);
+   if (npages != pinned) {
+   if (pinned < 0) {
+   ret = pinned;
+   } else {
+   unpin_user_pages(page_list, pinned);
+   ret = -ENOMEM;
+   }
+   goto unlock;
+   }
 
-   while (npages) {
-   pinned = min_t(unsigned long, npages, list_size);
-   ret = pin_user_pages(cur_base, pinned,
-gup_flags, page_list, NULL);
-   if (ret != pinned)
-   goto out;
-
-   if (!last_pfn)
-   map_pfn = page_to_pfn(page_list[0]);
-
-   for (i = 0; i < ret; i++) {
-   unsigned long this_pfn = page_to_pfn(page_list[i]);
-   u64 csize;
-
-   if (last_pfn && (this_pfn != last_pfn + 1)) {
-   /* Pin a contiguous chunk of memory */
-   csize = (last_pfn - map_pfn + 1) << PAGE_SHIFT;
-   if (vhost_vdpa_map(v, iova, csize,
-  map_pfn << PAGE_SHIFT,
-  msg->perm))
-   goto out;
-   map_pfn = this_pfn;
-   iova += csize;
+   iova &= PAGE_MASK;
+   map_pfn = page_to_pfn(page_list[0]);
+
+   /* One more iteration to avoid extra vdpa_map() call out of loop. */
+   for (i = 0; i <= npages; i++) {
+   unsigned long this_pfn;
+   u64 csize;
+
+   /* The last chunk may have no valid PFN next to it */
+   this_pfn = i < npages ? page_to_pfn(page_list[i]) : -1UL;
+
+   if (last_pfn && (this_pfn == -1UL ||
+thi

[PATCH v3 1/2] vhost-vdpa: fix vhost_vdpa_map() on error condition

2020-10-02 Thread Si-Wei Liu 
vhost_vdpa_map() should remove the iotlb entry just added
if the corresponding mapping fails to set up properly.

Fixes: 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
Signed-off-by: Si-Wei Liu 
---
 drivers/vhost/vdpa.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 796fe97..0f27919 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -565,6 +565,9 @@ static int vhost_vdpa_map(struct vhost_vdpa *v,
  perm_to_iommu_flags(perm));
}
 
+   if (r)
+   vhost_iotlb_del_range(dev->iotlb, iova, iova + size - 1);
+
return r;
 }
 
-- 
1.8.3.1

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v3 0/2] vhost-vdpa mapping error path fixes

2020-10-02 Thread Si-Wei Liu 
Commit 4c8cf31885f6 ("vhost: introduce vDPA-based backend")
has following issues in the failure path of IOTLB update:

1) vhost_vdpa_map() does not clean up dangling iotlb entry
   upon mapping failure

2) vhost_vdpa_process_iotlb_update() has leakage of pinned
   pages in case of vhost_vdpa_map() failure

This patchset attempts to address the above issues.

Changes in v3:
- Factor out changes in vhost_vdpa_map() and the fix for
  page pinning leak to separate patches (Jason)

---
Si-Wei Liu (2):
  vhost-vdpa: fix vhost_vdpa_map() on error condition
  vhost-vdpa: fix page pinning leakage in error path

 drivers/vhost/vdpa.c | 122 +++
 1 file changed, 74 insertions(+), 48 deletions(-)

-- 
1.8.3.1

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [Linux-kernel-mentees][PATCH 0/2] reorder members of structures in virtio_net for optimization

2020-10-02 Thread David Miller 
From: Anant Thazhemadam 
Date: Wed, 30 Sep 2020 10:47:20 +0530

> The structures virtnet_info and receive_queue have byte holes in 
> middle, and their members could do with some rearranging 
> (order-of-declaration wise) in order to overcome this.
> 
> Rearranging the members helps in:
>   * elimination the byte holes in the middle of the structures
>   * reduce the size of the structure (virtnet_info)
>   * have more members stored in one cache line (as opposed to 
> unnecessarily crossing the cacheline boundary and spanning
> different cachelines)
> 
> The analysis was performed using pahole.
> 
> These patches may be applied in any order.

What effects do these changes have on performance?

The cache locality for various TX and RX paths could be effected.

I'm not applying these patches without some data on the performance
impact.

Thank you.

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] vhost-vdpa: fix page pinning leakage in error path

2020-10-02 Thread Jason Wang 


On 2020/10/2 上午4:23, Si-Wei Liu wrote:

Pinned pages are not properly accounted particularly when
mapping error occurs on IOTLB update. Clean up dangling
pinned pages for the error path. As the inflight pinned
pages, specifically for memory region that strides across
multiple chunks, would need more than one free page for
book keeping and accounting. For simplicity, pin pages
for all memory in the IOVA range in one go rather than
have multiple pin_user_pages calls to make up the entire
region. This way it's easier to track and account the
pages already mapped, particularly for clean-up in the
error path.

Fixes: 20453a45fb06 ("vhost: introduce vDPA-based backend")
Signed-off-by: Si-Wei Liu 
---
  drivers/vhost/vdpa.c | 121 +++
  1 file changed, 73 insertions(+), 48 deletions(-)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 796fe97..abc4aa2 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -565,6 +565,8 @@ static int vhost_vdpa_map(struct vhost_vdpa *v,
  perm_to_iommu_flags(perm));
}
  
+	if (r)

+   vhost_iotlb_del_range(dev->iotlb, iova, iova + size - 1);
return r;
  }



Please use a separate patch for this fix.


  
@@ -592,21 +594,19 @@ static int vhost_vdpa_process_iotlb_update(struct vhost_vdpa *v,

struct vhost_dev *dev = &v->vdev;
struct vhost_iotlb *iotlb = dev->iotlb;
struct page **page_list;
-   unsigned long list_size = PAGE_SIZE / sizeof(struct page *);
+   struct vm_area_struct **vmas;
unsigned int gup_flags = FOLL_LONGTERM;
-   unsigned long npages, cur_base, map_pfn, last_pfn = 0;
-   unsigned long locked, lock_limit, pinned, i;
+   unsigned long map_pfn, last_pfn = 0;
+   unsigned long npages, lock_limit;
+   unsigned long i, nmap = 0;
u64 iova = msg->iova;
+   long pinned;
int ret = 0;
  
  	if (vhost_iotlb_itree_first(iotlb, msg->iova,

msg->iova + msg->size - 1))
return -EEXIST;
  
-	page_list = (struct page **) __get_free_page(GFP_KERNEL);

-   if (!page_list)
-   return -ENOMEM;
-
if (msg->perm & VHOST_ACCESS_WO)
gup_flags |= FOLL_WRITE;
  
@@ -614,61 +614,86 @@ static int vhost_vdpa_process_iotlb_update(struct vhost_vdpa *v,

if (!npages)
return -EINVAL;
  
+	page_list = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);

+   vmas = kvmalloc_array(npages, sizeof(struct vm_area_struct *),
+ GFP_KERNEL);
+   if (!page_list || !vmas) {
+   ret = -ENOMEM;
+   goto free;
+   }
+
mmap_read_lock(dev->mm);
  
-	locked = atomic64_add_return(npages, &dev->mm->pinned_vm);

lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
-
-   if (locked > lock_limit) {
+   if (npages + atomic64_read(&dev->mm->pinned_vm) > lock_limit) {
ret = -ENOMEM;
-   goto out;
+   goto unlock;
}
  
-	cur_base = msg->uaddr & PAGE_MASK;

-   iova &= PAGE_MASK;
+   pinned = pin_user_pages(msg->uaddr & PAGE_MASK, npages, gup_flags,
+   page_list, vmas);
+   if (npages != pinned) {
+   if (pinned < 0) {
+   ret = pinned;
+   } else {
+   unpin_user_pages(page_list, pinned);
+   ret = -ENOMEM;
+   }
+   goto unlock;
+   }
  
-	while (npages) {

-   pinned = min_t(unsigned long, npages, list_size);
-   ret = pin_user_pages(cur_base, pinned,
-gup_flags, page_list, NULL);
-   if (ret != pinned)
-   goto out;
-
-   if (!last_pfn)
-   map_pfn = page_to_pfn(page_list[0]);
-
-   for (i = 0; i < ret; i++) {
-   unsigned long this_pfn = page_to_pfn(page_list[i]);
-   u64 csize;
-
-   if (last_pfn && (this_pfn != last_pfn + 1)) {
-   /* Pin a contiguous chunk of memory */
-   csize = (last_pfn - map_pfn + 1) << PAGE_SHIFT;
-   if (vhost_vdpa_map(v, iova, csize,
-  map_pfn << PAGE_SHIFT,
-  msg->perm))
-   goto out;
-   map_pfn = this_pfn;
-   iova += csize;
+   iova &= PAGE_MASK;
+   map_pfn = page_to_pfn(page_list[0]);
+
+   /* One more iteration to avoid extra vdpa_map() call out of loop. */
+   for (i = 0; i <= npages; i++) {
+   unsigned long this_pfn;
+   u64 csize;
+
+   /* The last chunk may have no v

Re: [PATCH v2 2/2] vhost: Don't call log_access_ok() when using IOTLB

2020-10-02 Thread Jason Wang 


On 2020/9/30 上午12:30, Greg Kurz wrote:

When the IOTLB device is enabled, the log_guest_addr that is passed by
userspace to the VHOST_SET_VRING_ADDR ioctl, and which is then written
to vq->log_addr, is a GIOVA. All writes to this address are translated
by log_user() to writes to an HVA, and then ultimately logged through
the corresponding GPAs in log_write_hva(). No logging will ever occur
with vq->log_addr in this case. It is thus wrong to pass vq->log_addr
and log_guest_addr to log_access_vq() which assumes they are actual
GPAs.

Introduce a new vq_log_used_access_ok() helper that only checks accesses
to the log for the used structure when there isn't an IOTLB device around.

Signed-off-by: Greg Kurz 
---
  drivers/vhost/vhost.c |   23 +++
  1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index c3b49975dc28..5996e32fa818 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1370,6 +1370,20 @@ bool vhost_log_access_ok(struct vhost_dev *dev)
  }
  EXPORT_SYMBOL_GPL(vhost_log_access_ok);
  
+static bool vq_log_used_access_ok(struct vhost_virtqueue *vq,

+ void __user *log_base,
+ bool log_used,
+ u64 log_addr,
+ size_t log_size)
+{
+   /* If an IOTLB device is present, log_addr is a GIOVA that
+* will never be logged by log_used(). */
+   if (vq->iotlb)
+   return true;
+
+   return !log_used || log_access_ok(log_base, log_addr, log_size);
+}
+
  /* Verify access for write logging. */
  /* Caller should have vq mutex and device mutex */
  static bool vq_log_access_ok(struct vhost_virtqueue *vq,
@@ -1377,8 +1391,8 @@ static bool vq_log_access_ok(struct vhost_virtqueue *vq,
  {
return vq_memory_access_ok(log_base, vq->umem,
   vhost_has_feature(vq, VHOST_F_LOG_ALL)) &&
-   (!vq->log_used || log_access_ok(log_base, vq->log_addr,
- vhost_get_used_size(vq, vq->num)));
+   vq_log_used_access_ok(vq, log_base, vq->log_used, vq->log_addr,
+ vhost_get_used_size(vq, vq->num));
  }
  
  /* Can we start vq? */

@@ -1517,8 +1531,9 @@ static long vhost_vring_set_addr(struct vhost_dev *d,
return -EINVAL;
  
  		/* Also validate log access for used ring if enabled. */

-   if ((a.flags & (0x1 << VHOST_VRING_F_LOG)) &&
-   !log_access_ok(vq->log_base, a.log_guest_addr,
+   if (!vq_log_used_access_ok(vq, vq->log_base,
+   a.flags & (0x1 << VHOST_VRING_F_LOG),
+   a.log_guest_addr,
sizeof *vq->used +
vq->num * sizeof *vq->used->ring))



It looks to me that we should use vhost_get_used_size() which takes 
event into account.


Any reason that we can't reuse vq_log_access_ok() here?

Thanks



return -EINVAL;




___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 1/2] vhost: Don't call access_ok() when using IOTLB

2020-10-02 Thread Jason Wang 


On 2020/9/30 上午12:30, Greg Kurz wrote:

When the IOTLB device is enabled, the vring addresses we get
from userspace are GIOVAs. It is thus wrong to pass them down
to access_ok() which only takes HVAs.

Access validation is done at prefetch time with IOTLB. Teach
vq_access_ok() about that by moving the (vq->iotlb) check
from vhost_vq_access_ok() to vq_access_ok(). This prevents
vhost_vring_set_addr() to fail when verifying the accesses.
No behavior change for vhost_vq_access_ok().

BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1883084
Fixes: 6b1e6cc7855b ("vhost: new device IOTLB API")
Cc: jasow...@redhat.com
CC: sta...@vger.kernel.org # 4.14+
Signed-off-by: Greg Kurz 
---
  drivers/vhost/vhost.c |9 +
  1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index b45519ca66a7..c3b49975dc28 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1290,6 +1290,11 @@ static bool vq_access_ok(struct vhost_virtqueue *vq, 
unsigned int num,
 vring_used_t __user *used)
  
  {

+   /* If an IOTLB device is present, the vring addresses are
+* GIOVAs. Access validation occurs at prefetch time. */
+   if (vq->iotlb)
+   return true;
+
return access_ok(desc, vhost_get_desc_size(vq, num)) &&
   access_ok(avail, vhost_get_avail_size(vq, num)) &&
   access_ok(used, vhost_get_used_size(vq, num));
@@ -1383,10 +1388,6 @@ bool vhost_vq_access_ok(struct vhost_virtqueue *vq)
if (!vq_log_access_ok(vq, vq->log_base))
return false;
  
-	/* Access validation occurs at prefetch time with IOTLB */

-   if (vq->iotlb)
-   return true;
-
return vq_access_ok(vq, vq->num, vq->desc, vq->avail, vq->used);
  }
  EXPORT_SYMBOL_GPL(vhost_vq_access_ok);



Acked-by: Jason Wang 

Thanks


___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 6/7] drm/fb_helper: Support framebuffers in I/O memory

2020-10-02 Thread Daniel Vetter 
On Fri, Oct 2, 2020 at 8:05 PM Daniel Vetter  wrote:
>
> On Tue, Sep 29, 2020 at 05:14:36PM +0200, Thomas Zimmermann wrote:
> > At least sparc64 requires I/O-specific access to framebuffers. This
> > patch updates the fbdev console accordingly.
> >
> > For drivers with direct access to the framebuffer memory, the callback
> > functions in struct fb_ops test for the type of memory and call the rsp
> > fb_sys_ of fb_cfb_ functions.
> >
> > For drivers that employ a shadow buffer, fbdev's blit function retrieves
> > the framebuffer address as struct dma_buf_map, and uses dma_buf_map
> > interfaces to access the buffer.
> >
> > The bochs driver on sparc64 uses a workaround to flag the framebuffer as
> > I/O memory and avoid a HW exception. With the introduction of struct
> > dma_buf_map, this is not required any longer. The patch removes the rsp
> > code from both, bochs and fbdev.
> >
> > Signed-off-by: Thomas Zimmermann 

Argh, I accidentally hit send before finishing this ...

> > ---
> >  drivers/gpu/drm/bochs/bochs_kms.c |   1 -
> >  drivers/gpu/drm/drm_fb_helper.c   | 217 --
> >  include/drm/drm_mode_config.h |  12 --
> >  include/linux/dma-buf-map.h   |  72 --
> >  4 files changed, 265 insertions(+), 37 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bochs/bochs_kms.c 
> > b/drivers/gpu/drm/bochs/bochs_kms.c
> > index 13d0d04c4457..853081d186d5 100644
> > --- a/drivers/gpu/drm/bochs/bochs_kms.c
> > +++ b/drivers/gpu/drm/bochs/bochs_kms.c
> > @@ -151,7 +151,6 @@ int bochs_kms_init(struct bochs_device *bochs)
> >   bochs->dev->mode_config.preferred_depth = 24;
> >   bochs->dev->mode_config.prefer_shadow = 0;
> >   bochs->dev->mode_config.prefer_shadow_fbdev = 1;
> > - bochs->dev->mode_config.fbdev_use_iomem = true;
> >   bochs->dev->mode_config.quirk_addfb_prefer_host_byte_order = true;
> >
> >   bochs->dev->mode_config.funcs = &bochs_mode_funcs;
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c 
> > b/drivers/gpu/drm/drm_fb_helper.c
> > index 343a292f2c7c..f345a314a437 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -388,24 +388,22 @@ static void drm_fb_helper_resume_worker(struct 
> > work_struct *work)
> >  }
> >
> >  static void drm_fb_helper_dirty_blit_real(struct drm_fb_helper *fb_helper,
> > -   struct drm_clip_rect *clip)
> > +   struct drm_clip_rect *clip,
> > +   struct dma_buf_map *dst)
> >  {
> >   struct drm_framebuffer *fb = fb_helper->fb;
> >   unsigned int cpp = fb->format->cpp[0];
> >   size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp;
> >   void *src = fb_helper->fbdev->screen_buffer + offset;
> > - void *dst = fb_helper->buffer->map.vaddr + offset;
> >   size_t len = (clip->x2 - clip->x1) * cpp;
> >   unsigned int y;
> >
> > - for (y = clip->y1; y < clip->y2; y++) {
> > - if (!fb_helper->dev->mode_config.fbdev_use_iomem)
> > - memcpy(dst, src, len);
> > - else
> > - memcpy_toio((void __iomem *)dst, src, len);
> > + dma_buf_map_incr(dst, offset); /* go to first pixel within clip rect 
> > */
> >
> > + for (y = clip->y1; y < clip->y2; y++) {
> > + dma_buf_map_memcpy_to(dst, src, len);
> > + dma_buf_map_incr(dst, fb->pitches[0]);
> >   src += fb->pitches[0];
> > - dst += fb->pitches[0];
> >   }
> >  }
> >
> > @@ -433,8 +431,9 @@ static void drm_fb_helper_dirty_work(struct work_struct 
> > *work)
> >   ret = drm_client_buffer_vmap(helper->buffer, &map);
> >   if (ret)
> >   return;
> > - drm_fb_helper_dirty_blit_real(helper, &clip_copy);
> > + drm_fb_helper_dirty_blit_real(helper, &clip_copy, 
> > &map);
> >   }
> > +
> >   if (helper->fb->funcs->dirty)
> >   helper->fb->funcs->dirty(helper->fb, NULL, 0, 0,
> >&clip_copy, 1);
> > @@ -771,6 +770,136 @@ void drm_fb_helper_sys_imageblit(struct fb_info *info,
> >  }
> >  EXPORT_SYMBOL(drm_fb_helper_sys_imageblit);
> >
> > +static ssize_t drm_fb_helper_cfb_read(struct fb_info *info, char __user 
> > *buf,
> > +   size_t count, loff_t *ppos)
> > +{
> > + unsigned long p = *ppos;
> > + u8 *dst;
> > + u8 __iomem *src;
> > + int c, err = 0;
> > + unsigned long total_size;
> > + unsigned long alloc_size;
> > + ssize_t ret = 0;
> > +
> > + if (info->state != FBINFO_STATE_RUNNING)
> > + return -EPERM;
> > +
> > + total_size = info->screen_size;
> > +
> > + if (total_size == 0)
> > + total_size = info->fix.smem_len;
> > +
> > + if (p >= total_size)
> > + return

Re: [PATCH v3 7/7] drm/todo: Update entries around struct dma_buf_map

2020-10-02 Thread Daniel Vetter 
On Tue, Sep 29, 2020 at 05:14:37PM +0200, Thomas Zimmermann wrote:
> Instances of struct dma_buf_map should be useful throughout DRM's
> memory management code. Furthermore, several drivers can now use
> generic fbdev emulation.
> 
> Signed-off-by: Thomas Zimmermann 

Acked-by: Daniel Vetter 

> ---
>  Documentation/gpu/todo.rst | 24 ++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index 3751ac976c3e..023626c1837b 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -197,8 +197,10 @@ Convert drivers to use drm_fbdev_generic_setup()
>  
>  
>  Most drivers can use drm_fbdev_generic_setup(). Driver have to implement
> -atomic modesetting and GEM vmap support. Current generic fbdev emulation
> -expects the framebuffer in system memory (or system-like memory).
> +atomic modesetting and GEM vmap support. Historically, generic fbdev 
> emulation
> +expected the framebuffer in system memory or system-like memory. By employing
> +struct dma_buf_map, drivers with frambuffers in I/O memory can be supported
> +as well.
>  
>  Contact: Maintainer of the driver you plan to convert
>  
> @@ -446,6 +448,24 @@ Contact: Ville Syrjälä, Daniel Vetter
>  
>  Level: Intermediate
>  
> +Use struct dma_buf_map throughout codebase
> +--
> +
> +Pointers to shared device memory are stored in struct dma_buf_map. Each
> +instance knows whether it refers to system or I/O memory. Most of the 
> DRM-wide
> +interface have been converted to use struct dma_buf_map, but implementations
> +often still use raw pointers.
> +
> +The task is to use struct dma_buf_map where it makes sense.
> +
> +* Memory managers should use struct dma_buf_map for dma-buf-imported buffers.
> +* TTM might benefit from using struct dma_buf_map internally.
> +* Framebuffer copying and blitting helpers should operate on struct 
> dma_buf_map.
> +
> +Contact: Thomas Zimmermann , Christian König, Daniel 
> Vetter
> +
> +Level: Intermediate
> +
>  
>  Core refactorings
>  =
> -- 
> 2.28.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 6/7] drm/fb_helper: Support framebuffers in I/O memory

2020-10-02 Thread Daniel Vetter 
On Tue, Sep 29, 2020 at 05:14:36PM +0200, Thomas Zimmermann wrote:
> At least sparc64 requires I/O-specific access to framebuffers. This
> patch updates the fbdev console accordingly.
> 
> For drivers with direct access to the framebuffer memory, the callback
> functions in struct fb_ops test for the type of memory and call the rsp
> fb_sys_ of fb_cfb_ functions.
> 
> For drivers that employ a shadow buffer, fbdev's blit function retrieves
> the framebuffer address as struct dma_buf_map, and uses dma_buf_map
> interfaces to access the buffer.
> 
> The bochs driver on sparc64 uses a workaround to flag the framebuffer as
> I/O memory and avoid a HW exception. With the introduction of struct
> dma_buf_map, this is not required any longer. The patch removes the rsp
> code from both, bochs and fbdev.
> 
> Signed-off-by: Thomas Zimmermann 
> ---
>  drivers/gpu/drm/bochs/bochs_kms.c |   1 -
>  drivers/gpu/drm/drm_fb_helper.c   | 217 --
>  include/drm/drm_mode_config.h |  12 --
>  include/linux/dma-buf-map.h   |  72 --
>  4 files changed, 265 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bochs/bochs_kms.c 
> b/drivers/gpu/drm/bochs/bochs_kms.c
> index 13d0d04c4457..853081d186d5 100644
> --- a/drivers/gpu/drm/bochs/bochs_kms.c
> +++ b/drivers/gpu/drm/bochs/bochs_kms.c
> @@ -151,7 +151,6 @@ int bochs_kms_init(struct bochs_device *bochs)
>   bochs->dev->mode_config.preferred_depth = 24;
>   bochs->dev->mode_config.prefer_shadow = 0;
>   bochs->dev->mode_config.prefer_shadow_fbdev = 1;
> - bochs->dev->mode_config.fbdev_use_iomem = true;
>   bochs->dev->mode_config.quirk_addfb_prefer_host_byte_order = true;
>  
>   bochs->dev->mode_config.funcs = &bochs_mode_funcs;
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 343a292f2c7c..f345a314a437 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -388,24 +388,22 @@ static void drm_fb_helper_resume_worker(struct 
> work_struct *work)
>  }
>  
>  static void drm_fb_helper_dirty_blit_real(struct drm_fb_helper *fb_helper,
> -   struct drm_clip_rect *clip)
> +   struct drm_clip_rect *clip,
> +   struct dma_buf_map *dst)
>  {
>   struct drm_framebuffer *fb = fb_helper->fb;
>   unsigned int cpp = fb->format->cpp[0];
>   size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp;
>   void *src = fb_helper->fbdev->screen_buffer + offset;
> - void *dst = fb_helper->buffer->map.vaddr + offset;
>   size_t len = (clip->x2 - clip->x1) * cpp;
>   unsigned int y;
>  
> - for (y = clip->y1; y < clip->y2; y++) {
> - if (!fb_helper->dev->mode_config.fbdev_use_iomem)
> - memcpy(dst, src, len);
> - else
> - memcpy_toio((void __iomem *)dst, src, len);
> + dma_buf_map_incr(dst, offset); /* go to first pixel within clip rect */
>  
> + for (y = clip->y1; y < clip->y2; y++) {
> + dma_buf_map_memcpy_to(dst, src, len);
> + dma_buf_map_incr(dst, fb->pitches[0]);
>   src += fb->pitches[0];
> - dst += fb->pitches[0];
>   }
>  }
>  
> @@ -433,8 +431,9 @@ static void drm_fb_helper_dirty_work(struct work_struct 
> *work)
>   ret = drm_client_buffer_vmap(helper->buffer, &map);
>   if (ret)
>   return;
> - drm_fb_helper_dirty_blit_real(helper, &clip_copy);
> + drm_fb_helper_dirty_blit_real(helper, &clip_copy, &map);
>   }
> +
>   if (helper->fb->funcs->dirty)
>   helper->fb->funcs->dirty(helper->fb, NULL, 0, 0,
>&clip_copy, 1);
> @@ -771,6 +770,136 @@ void drm_fb_helper_sys_imageblit(struct fb_info *info,
>  }
>  EXPORT_SYMBOL(drm_fb_helper_sys_imageblit);
>  
> +static ssize_t drm_fb_helper_cfb_read(struct fb_info *info, char __user *buf,
> +   size_t count, loff_t *ppos)
> +{
> + unsigned long p = *ppos;
> + u8 *dst;
> + u8 __iomem *src;
> + int c, err = 0;
> + unsigned long total_size;
> + unsigned long alloc_size;
> + ssize_t ret = 0;
> +
> + if (info->state != FBINFO_STATE_RUNNING)
> + return -EPERM;
> +
> + total_size = info->screen_size;
> +
> + if (total_size == 0)
> + total_size = info->fix.smem_len;
> +
> + if (p >= total_size)
> + return 0;
> +
> + if (count >= total_size)
> + count = total_size;
> +
> + if (count + p > total_size)
> + count = total_size - p;
> +
> + src = (u8 __iomem *)(info->screen_base + p);
> +
> + alloc_size = min(count, PAGE_SIZE);
> +
> + dst = kmalloc(alloc_size, GFP_KERNEL);
> + if (!dst)
> +

Re: [PATCH v3 5/7] drm/gem: Store client buffer mappings as struct dma_buf_map

2020-10-02 Thread Daniel Vetter 
On Tue, Sep 29, 2020 at 05:14:35PM +0200, Thomas Zimmermann wrote:
> Kernel DRM clients now store their framebuffer address in an instance
> of struct dma_buf_map. Depending on the buffer's location, the address
> refers to system or I/O memory.
> 
> Callers of drm_client_buffer_vmap() receive a copy of the value in
> the call's supplied arguments. It can be accessed and modified with
> dma_buf_map interfaces.
> 
> Signed-off-by: Thomas Zimmermann 
> ---
>  drivers/gpu/drm/drm_client.c| 34 +++--
>  drivers/gpu/drm/drm_fb_helper.c | 23 +-
>  include/drm/drm_client.h|  7 ---
>  3 files changed, 38 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
> index ac0082bed966..fe573acf1067 100644
> --- a/drivers/gpu/drm/drm_client.c
> +++ b/drivers/gpu/drm/drm_client.c
> @@ -235,7 +235,7 @@ static void drm_client_buffer_delete(struct 
> drm_client_buffer *buffer)
>  {
>   struct drm_device *dev = buffer->client->dev;
>  
> - drm_gem_vunmap(buffer->gem, buffer->vaddr);
> + drm_gem_vunmap(buffer->gem, &buffer->map);
>  
>   if (buffer->gem)
>   drm_gem_object_put(buffer->gem);
> @@ -291,25 +291,31 @@ drm_client_buffer_create(struct drm_client_dev *client, 
> u32 width, u32 height, u
>  /**
>   * drm_client_buffer_vmap - Map DRM client buffer into address space
>   * @buffer: DRM client buffer
> + * @map_copy: Returns the mapped memory's address
>   *
>   * This function maps a client buffer into kernel address space. If the
> - * buffer is already mapped, it returns the mapping's address.
> + * buffer is already mapped, it returns the existing mapping's address.
>   *
>   * Client buffer mappings are not ref'counted. Each call to
>   * drm_client_buffer_vmap() should be followed by a call to
>   * drm_client_buffer_vunmap(); or the client buffer should be mapped
>   * throughout its lifetime.
>   *
> + * The returned address is a copy of the internal value. In contrast to
> + * other vmap interfaces, you don't need it for the client's vunmap
> + * function. So you can modify it at will during blit and draw operations.
> + *
>   * Returns:
> - *   The mapped memory's address
> + *   0 on success, or a negative errno code otherwise.
>   */
> -void *drm_client_buffer_vmap(struct drm_client_buffer *buffer)
> +int
> +drm_client_buffer_vmap(struct drm_client_buffer *buffer, struct dma_buf_map 
> *map_copy)
>  {
> - struct dma_buf_map map;
> + struct dma_buf_map *map = &buffer->map;
>   int ret;
>  
> - if (buffer->vaddr)
> - return buffer->vaddr;
> + if (dma_buf_map_is_set(map))
> + goto out;
>  
>   /*
>* FIXME: The dependency on GEM here isn't required, we could
> @@ -319,13 +325,14 @@ void *drm_client_buffer_vmap(struct drm_client_buffer 
> *buffer)
>* fd_install step out of the driver backend hooks, to make that
>* final step optional for internal users.
>*/
> - ret = drm_gem_vmap(buffer->gem, &map);
> + ret = drm_gem_vmap(buffer->gem, map);
>   if (ret)
> - return ERR_PTR(ret);
> + return ret;
>  
> - buffer->vaddr = map.vaddr;
> +out:
> + *map_copy = *map;
>  
> - return map.vaddr;
> + return 0;
>  }
>  EXPORT_SYMBOL(drm_client_buffer_vmap);
>  
> @@ -339,10 +346,9 @@ EXPORT_SYMBOL(drm_client_buffer_vmap);
>   */
>  void drm_client_buffer_vunmap(struct drm_client_buffer *buffer)
>  {
> - struct dma_buf_map map = DMA_BUF_MAP_INIT_VADDR(buffer->vaddr);
> + struct dma_buf_map *map = &buffer->map;
>  
> - drm_gem_vunmap(buffer->gem, &map);
> - buffer->vaddr = NULL;
> + drm_gem_vunmap(buffer->gem, map);
>  }
>  EXPORT_SYMBOL(drm_client_buffer_vunmap);
>  
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 8697554ccd41..343a292f2c7c 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -394,7 +394,7 @@ static void drm_fb_helper_dirty_blit_real(struct 
> drm_fb_helper *fb_helper,
>   unsigned int cpp = fb->format->cpp[0];
>   size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp;
>   void *src = fb_helper->fbdev->screen_buffer + offset;
> - void *dst = fb_helper->buffer->vaddr + offset;
> + void *dst = fb_helper->buffer->map.vaddr + offset;
>   size_t len = (clip->x2 - clip->x1) * cpp;
>   unsigned int y;
>  
> @@ -416,7 +416,8 @@ static void drm_fb_helper_dirty_work(struct work_struct 
> *work)
>   struct drm_clip_rect *clip = &helper->dirty_clip;
>   struct drm_clip_rect clip_copy;
>   unsigned long flags;
> - void *vaddr;
> + struct dma_buf_map map;
> + int ret;
>  
>   spin_lock_irqsave(&helper->dirty_lock, flags);
>   clip_copy = *clip;
> @@ -429,8 +430,8 @@ static void drm_fb_helper_dirty_work(struct work_struct 
> *work)
>  
>   /* Generic fbdev uses a shadow buffer */
>

Re: [PATCH v3 4/7] drm/gem: Update internal GEM vmap/vunmap interfaces to use struct dma_buf_map

2020-10-02 Thread Daniel Vetter 
On Tue, Sep 29, 2020 at 05:14:34PM +0200, Thomas Zimmermann wrote:
> GEM's vmap and vunmap interfaces now wrap memory pointers in struct
> dma_buf_map.
> 
> Signed-off-by: Thomas Zimmermann 
> ---
>  drivers/gpu/drm/drm_client.c   | 18 +++---
>  drivers/gpu/drm/drm_gem.c  | 28 ++--
>  drivers/gpu/drm/drm_internal.h |  5 +++--
>  drivers/gpu/drm/drm_prime.c| 14 --
>  4 files changed, 32 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
> index 495f47d23d87..ac0082bed966 100644
> --- a/drivers/gpu/drm/drm_client.c
> +++ b/drivers/gpu/drm/drm_client.c
> @@ -3,6 +3,7 @@
>   * Copyright 2018 Noralf Trønnes
>   */
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -304,7 +305,8 @@ drm_client_buffer_create(struct drm_client_dev *client, 
> u32 width, u32 height, u
>   */
>  void *drm_client_buffer_vmap(struct drm_client_buffer *buffer)
>  {
> - void *vaddr;
> + struct dma_buf_map map;
> + int ret;
>  
>   if (buffer->vaddr)
>   return buffer->vaddr;
> @@ -317,13 +319,13 @@ void *drm_client_buffer_vmap(struct drm_client_buffer 
> *buffer)
>* fd_install step out of the driver backend hooks, to make that
>* final step optional for internal users.
>*/
> - vaddr = drm_gem_vmap(buffer->gem);
> - if (IS_ERR(vaddr))
> - return vaddr;
> + ret = drm_gem_vmap(buffer->gem, &map);
> + if (ret)
> + return ERR_PTR(ret);
>  
> - buffer->vaddr = vaddr;
> + buffer->vaddr = map.vaddr;
>  
> - return vaddr;
> + return map.vaddr;
>  }
>  EXPORT_SYMBOL(drm_client_buffer_vmap);
>  
> @@ -337,7 +339,9 @@ EXPORT_SYMBOL(drm_client_buffer_vmap);
>   */
>  void drm_client_buffer_vunmap(struct drm_client_buffer *buffer)
>  {
> - drm_gem_vunmap(buffer->gem, buffer->vaddr);
> + struct dma_buf_map map = DMA_BUF_MAP_INIT_VADDR(buffer->vaddr);
> +
> + drm_gem_vunmap(buffer->gem, &map);
>   buffer->vaddr = NULL;
>  }
>  EXPORT_SYMBOL(drm_client_buffer_vunmap);
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 0c4a66dea5c2..f2b2f37d41c4 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1205,32 +1205,32 @@ void drm_gem_unpin(struct drm_gem_object *obj)
>   obj->funcs->unpin(obj);
>  }
>  
> -void *drm_gem_vmap(struct drm_gem_object *obj)
> +int drm_gem_vmap(struct drm_gem_object *obj, struct dma_buf_map *map)
>  {
> - struct dma_buf_map map;
>   int ret;
>  
> - if (!obj->funcs->vmap) {
> - return ERR_PTR(-EOPNOTSUPP);
> + if (!obj->funcs->vmap)
> + return -EOPNOTSUPP;
>  
> - ret = obj->funcs->vmap(obj, &map);
> + ret = obj->funcs->vmap(obj, map);
>   if (ret)
> - return ERR_PTR(ret);
> - else if (dma_buf_map_is_null(&map))
> - return ERR_PTR(-ENOMEM);
> + return ret;
> + else if (dma_buf_map_is_null(map))
> + return -ENOMEM;
>  
> - return map.vaddr;
> + return 0;
>  }
>  
> -void drm_gem_vunmap(struct drm_gem_object *obj, void *vaddr)
> +void drm_gem_vunmap(struct drm_gem_object *obj, struct dma_buf_map *map)
>  {
> - struct dma_buf_map map = DMA_BUF_MAP_INIT_VADDR(vaddr);
> -
> - if (!vaddr)
> + if (dma_buf_map_is_null(map))
>   return;
>  
>   if (obj->funcs->vunmap)
> - obj->funcs->vunmap(obj, &map);
> + obj->funcs->vunmap(obj, map);
> +
> + /* Always set the mapping to NULL. Callers may rely on this. */
> + dma_buf_map_clear(map);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index b65865c630b0..58832d75a9bd 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -33,6 +33,7 @@
>  
>  struct dentry;
>  struct dma_buf;
> +struct dma_buf_map;
>  struct drm_connector;
>  struct drm_crtc;
>  struct drm_framebuffer;
> @@ -187,8 +188,8 @@ void drm_gem_print_info(struct drm_printer *p, unsigned 
> int indent,
>  
>  int drm_gem_pin(struct drm_gem_object *obj);
>  void drm_gem_unpin(struct drm_gem_object *obj);
> -void *drm_gem_vmap(struct drm_gem_object *obj);
> -void drm_gem_vunmap(struct drm_gem_object *obj, void *vaddr);
> +int drm_gem_vmap(struct drm_gem_object *obj, struct dma_buf_map *map);
> +void drm_gem_vunmap(struct drm_gem_object *obj, struct dma_buf_map *map);
>  
>  /* drm_debugfs.c drm_debugfs_crc.c */
>  #if defined(CONFIG_DEBUG_FS)
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 89e2a2496734..cb8fbeeb731b 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -667,21 +667,15 @@ EXPORT_SYMBOL(drm_gem_unmap_dma_buf);
>   *
>   * Sets up a kernel virtual mapping. This can be used as the 
> &dma_buf_ops.vmap
>   * callback. Calls into &drm_gem_object_funcs.vmap for device specific 
> handling.
> + * The kern

Re: [PATCH v3 3/7] drm/gem: Use struct dma_buf_map in GEM vmap ops and convert GEM backends

2020-10-02 Thread Daniel Vetter 
On Tue, Sep 29, 2020 at 05:14:33PM +0200, Thomas Zimmermann wrote:
> This patch replaces the vmap/vunmap's use of raw pointers in GEM object
> functions with instances of struct dma_buf_map. GEM backends are
> converted as well.
> 
> For most GEM backends, this simply change the returned type. GEM VRAM
> helpers are also updated to indicate whether the returned framebuffer
> address is in system or I/O memory.
> 
> Signed-off-by: Thomas Zimmermann 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 14 ++--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h |  4 +-
>  drivers/gpu/drm/ast/ast_cursor.c| 29 +++
>  drivers/gpu/drm/ast/ast_drv.h   |  7 +-
>  drivers/gpu/drm/drm_gem.c   | 22 ++---
>  drivers/gpu/drm/drm_gem_cma_helper.c| 14 ++--
>  drivers/gpu/drm/drm_gem_shmem_helper.c  | 48 ++-
>  drivers/gpu/drm/drm_gem_vram_helper.c   | 90 +++--
>  drivers/gpu/drm/etnaviv/etnaviv_drv.h   |  4 +-
>  drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 11 ++-
>  drivers/gpu/drm/exynos/exynos_drm_gem.c |  6 +-
>  drivers/gpu/drm/exynos/exynos_drm_gem.h |  4 +-
>  drivers/gpu/drm/lima/lima_gem.c |  6 +-
>  drivers/gpu/drm/lima/lima_sched.c   | 11 ++-
>  drivers/gpu/drm/mgag200/mgag200_mode.c  | 12 +--
>  drivers/gpu/drm/nouveau/nouveau_gem.h   |  4 +-
>  drivers/gpu/drm/nouveau/nouveau_prime.c |  9 ++-
>  drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 14 ++--
>  drivers/gpu/drm/qxl/qxl_display.c   | 13 +--
>  drivers/gpu/drm/qxl/qxl_draw.c  | 16 ++--
>  drivers/gpu/drm/qxl/qxl_drv.h   |  8 +-
>  drivers/gpu/drm/qxl/qxl_object.c| 23 +++---
>  drivers/gpu/drm/qxl/qxl_object.h|  2 +-
>  drivers/gpu/drm/qxl/qxl_prime.c | 12 +--
>  drivers/gpu/drm/radeon/radeon_gem.c |  4 +-
>  drivers/gpu/drm/radeon/radeon_prime.c   |  9 ++-
>  drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 22 +++--
>  drivers/gpu/drm/rockchip/rockchip_drm_gem.h |  4 +-
>  drivers/gpu/drm/tiny/cirrus.c   | 10 ++-
>  drivers/gpu/drm/tiny/gm12u320.c | 10 ++-
>  drivers/gpu/drm/udl/udl_modeset.c   |  8 +-
>  drivers/gpu/drm/vboxvideo/vbox_mode.c   | 11 ++-
>  drivers/gpu/drm/vc4/vc4_bo.c|  6 +-
>  drivers/gpu/drm/vc4/vc4_drv.h   |  2 +-
>  drivers/gpu/drm/vgem/vgem_drv.c | 16 ++--
>  drivers/gpu/drm/xen/xen_drm_front_gem.c | 18 +++--
>  drivers/gpu/drm/xen/xen_drm_front_gem.h |  6 +-
>  include/drm/drm_gem.h   |  5 +-
>  include/drm/drm_gem_cma_helper.h|  4 +-
>  include/drm/drm_gem_shmem_helper.h  |  4 +-
>  include/drm/drm_gem_vram_helper.h   |  4 +-
>  41 files changed, 304 insertions(+), 222 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> index 5b465ab774d1..de7d0cfe1b93 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> @@ -44,13 +44,14 @@
>  /**
>   * amdgpu_gem_prime_vmap - &dma_buf_ops.vmap implementation
>   * @obj: GEM BO
> + * @map: The virtual address of the mapping.
>   *
>   * Sets up an in-kernel virtual mapping of the BO's memory.
>   *
>   * Returns:
> - * The virtual address of the mapping or an error pointer.
> + * 0 on success, or a negative errno code otherwise.
>   */
> -void *amdgpu_gem_prime_vmap(struct drm_gem_object *obj)
> +int amdgpu_gem_prime_vmap(struct drm_gem_object *obj, struct dma_buf_map 
> *map)
>  {
>   struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
>   int ret;
> @@ -58,19 +59,20 @@ void *amdgpu_gem_prime_vmap(struct drm_gem_object *obj)
>   ret = ttm_bo_kmap(&bo->tbo, 0, bo->tbo.num_pages,
> &bo->dma_buf_vmap);
>   if (ret)
> - return ERR_PTR(ret);
> + return ret;
> + ttm_kmap_obj_to_dma_buf_map(&bo->dma_buf_vmap, map);

I guess with the ttm_bo_vmap idea all the ttm changes here will look a bit
different.

>  
> - return bo->dma_buf_vmap.virtual;
> + return 0;
>  }
>  
>  /**
>   * amdgpu_gem_prime_vunmap - &dma_buf_ops.vunmap implementation
>   * @obj: GEM BO
> - * @vaddr: Virtual address (unused)
> + * @map: Virtual address (unused)
>   *
>   * Tears down the in-kernel virtual mapping of the BO's memory.
>   */
> -void amdgpu_gem_prime_vunmap(struct drm_gem_object *obj, void *vaddr)
> +void amdgpu_gem_prime_vunmap(struct drm_gem_object *obj, struct dma_buf_map 
> *map)
>  {
>   struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h
> index 2c5c84a06bb9..622642793064 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h
> @@ -31,8 +31,8 @@ struct drm_gem_object *amdgpu_gem_prime_import(struct 
> drm_device *dev,
>

Re: [PATCH v3 2/7] drm/ttm: Add ttm_kmap_obj_to_dma_buf_map() for type conversion

2020-10-02 Thread Daniel Vetter 
On Fri, Oct 2, 2020 at 1:30 PM Christian König
 wrote:
>
> Am 02.10.20 um 11:58 schrieb Daniel Vetter:
> > On Wed, Sep 30, 2020 at 02:51:46PM +0200, Daniel Vetter wrote:
> >> On Wed, Sep 30, 2020 at 2:34 PM Christian König
> >>  wrote:
> >>> Am 30.09.20 um 11:47 schrieb Daniel Vetter:
>  On Wed, Sep 30, 2020 at 10:34:31AM +0200, Christian König wrote:
> > Am 30.09.20 um 10:19 schrieb Thomas Zimmermann:
> >> Hi
> >>
> >> Am 30.09.20 um 10:05 schrieb Christian König:
> >>> Am 29.09.20 um 19:49 schrieb Thomas Zimmermann:
>  Hi Christian
> 
>  Am 29.09.20 um 17:35 schrieb Christian König:
> > Am 29.09.20 um 17:14 schrieb Thomas Zimmermann:
> >> The new helper ttm_kmap_obj_to_dma_buf() extracts address and 
> >> location
> >> from and instance of TTM's kmap_obj and initializes struct 
> >> dma_buf_map
> >> with these values. Helpful for TTM-based drivers.
> > We could completely drop that if we use the same structure inside 
> > TTM as
> > well.
> >
> > Additional to that which driver is going to use this?
>  As Daniel mentioned, it's in patch 3. The TTM-based drivers will
>  retrieve the pointer via this function.
> 
>  I do want to see all that being more tightly integrated into TTM, but
>  not in this series. This one is about fixing the bochs-on-sparc64
>  problem for good. Patch 7 adds an update to TTM to the DRM TODO list.
> >>> I should have asked which driver you try to fix here :)
> >>>
> >>> In this case just keep the function inside bochs and only fix it 
> >>> there.
> >>>
> >>> All other drivers can be fixed when we generally pump this through 
> >>> TTM.
> >> Did you take a look at patch 3? This function will be used by VRAM
> >> helpers, nouveau, radeon, amdgpu and qxl. If we don't put it here, we
> >> have to duplicate the functionality in each if these drivers. Bochs
> >> itself uses VRAM helpers and doesn't touch the function directly.
> > Ah, ok can we have that then only in the VRAM helpers?
> >
> > Alternative you could go ahead and use dma_buf_map in ttm_bo_kmap_obj
> > directly and drop the hack with the TTM_BO_MAP_IOMEM_MASK.
> >
> > What I want to avoid is to have another conversion function in TTM 
> > because
> > what happens here is that we already convert from ttm_bus_placement to
> > ttm_bo_kmap_obj and then to dma_buf_map.
>  Hm I'm not really seeing how that helps with a gradual conversion of
>  everything over to dma_buf_map and assorted helpers for access? There's
>  too many places in ttm drivers where is_iomem and related stuff is used 
>  to
>  be able to convert it all in one go. An intermediate state with a bunch 
>  of
>  conversions seems fairly unavoidable to me.
> >>> Fair enough. I would just have started bottom up and not top down.
> >>>
> >>> Anyway feel free to go ahead with this approach as long as we can remove
> >>> the new function again when we clean that stuff up for good.
> >> Yeah I guess bottom up would make more sense as a refactoring. But the
> >> main motivation to land this here is to fix the __mmio vs normal
> >> memory confusion in the fbdev emulation helpers for sparc (and
> >> anything else that needs this). Hence the top down approach for
> >> rolling this out.
> > Ok I started reviewing this a bit more in-depth, and I think this is a bit
> > too much of a de-tour.
> >
> > Looking through all the callers of ttm_bo_kmap almost everyone maps the
> > entire object. Only vmwgfx uses to map less than that. Also, everyone just
> > immediately follows up with converting that full object map into a
> > pointer.
> >
> > So I think what we really want here is:
> > - new function
> >
> > int ttm_bo_vmap(struct ttm_buffer_object *bo, struct dma_buf_map *map);
> >
> >_vmap name since that's consistent with both dma_buf functions and
> >what's usually used to implement this. Outside of the ttm world kmap
> >usually just means single-page mappings using kmap() or it's iomem
> >sibling io_mapping_map* so rather confusing name for a function which
> >usually is just used to set up a vmap of the entire buffer.
> >
> > - a helper which can be used for the drm_gem_object_funcs vmap/vunmap
> >functions for all ttm drivers. We should be able to make this fully
> >generic because a) we now have dma_buf_map and b) drm_gem_object is
> >embedded in the ttm_bo, so we can upcast for everyone who's both a ttm
> >and gem driver.
> >
> >This is maybe a good follow-up, since it should allow us to ditch quite
> >a bit of the vram helper code for this more generic stuff. I also might
> >have missed some special-cases here, but from a quick look everything
> >just pins the buffer to the current location and that's it.
> >
> >Also this obvio

Re: [PATCH v3 2/7] drm/ttm: Add ttm_kmap_obj_to_dma_buf_map() for type conversion

2020-10-02 Thread Christian König 

Am 02.10.20 um 11:58 schrieb Daniel Vetter:

On Wed, Sep 30, 2020 at 02:51:46PM +0200, Daniel Vetter wrote:

On Wed, Sep 30, 2020 at 2:34 PM Christian König
 wrote:

Am 30.09.20 um 11:47 schrieb Daniel Vetter:

On Wed, Sep 30, 2020 at 10:34:31AM +0200, Christian König wrote:

Am 30.09.20 um 10:19 schrieb Thomas Zimmermann:

Hi

Am 30.09.20 um 10:05 schrieb Christian König:

Am 29.09.20 um 19:49 schrieb Thomas Zimmermann:

Hi Christian

Am 29.09.20 um 17:35 schrieb Christian König:

Am 29.09.20 um 17:14 schrieb Thomas Zimmermann:

The new helper ttm_kmap_obj_to_dma_buf() extracts address and location
from and instance of TTM's kmap_obj and initializes struct dma_buf_map
with these values. Helpful for TTM-based drivers.

We could completely drop that if we use the same structure inside TTM as
well.

Additional to that which driver is going to use this?

As Daniel mentioned, it's in patch 3. The TTM-based drivers will
retrieve the pointer via this function.

I do want to see all that being more tightly integrated into TTM, but
not in this series. This one is about fixing the bochs-on-sparc64
problem for good. Patch 7 adds an update to TTM to the DRM TODO list.

I should have asked which driver you try to fix here :)

In this case just keep the function inside bochs and only fix it there.

All other drivers can be fixed when we generally pump this through TTM.

Did you take a look at patch 3? This function will be used by VRAM
helpers, nouveau, radeon, amdgpu and qxl. If we don't put it here, we
have to duplicate the functionality in each if these drivers. Bochs
itself uses VRAM helpers and doesn't touch the function directly.

Ah, ok can we have that then only in the VRAM helpers?

Alternative you could go ahead and use dma_buf_map in ttm_bo_kmap_obj
directly and drop the hack with the TTM_BO_MAP_IOMEM_MASK.

What I want to avoid is to have another conversion function in TTM because
what happens here is that we already convert from ttm_bus_placement to
ttm_bo_kmap_obj and then to dma_buf_map.

Hm I'm not really seeing how that helps with a gradual conversion of
everything over to dma_buf_map and assorted helpers for access? There's
too many places in ttm drivers where is_iomem and related stuff is used to
be able to convert it all in one go. An intermediate state with a bunch of
conversions seems fairly unavoidable to me.

Fair enough. I would just have started bottom up and not top down.

Anyway feel free to go ahead with this approach as long as we can remove
the new function again when we clean that stuff up for good.

Yeah I guess bottom up would make more sense as a refactoring. But the
main motivation to land this here is to fix the __mmio vs normal
memory confusion in the fbdev emulation helpers for sparc (and
anything else that needs this). Hence the top down approach for
rolling this out.

Ok I started reviewing this a bit more in-depth, and I think this is a bit
too much of a de-tour.

Looking through all the callers of ttm_bo_kmap almost everyone maps the
entire object. Only vmwgfx uses to map less than that. Also, everyone just
immediately follows up with converting that full object map into a
pointer.

So I think what we really want here is:
- new function

int ttm_bo_vmap(struct ttm_buffer_object *bo, struct dma_buf_map *map);

   _vmap name since that's consistent with both dma_buf functions and
   what's usually used to implement this. Outside of the ttm world kmap
   usually just means single-page mappings using kmap() or it's iomem
   sibling io_mapping_map* so rather confusing name for a function which
   usually is just used to set up a vmap of the entire buffer.

- a helper which can be used for the drm_gem_object_funcs vmap/vunmap
   functions for all ttm drivers. We should be able to make this fully
   generic because a) we now have dma_buf_map and b) drm_gem_object is
   embedded in the ttm_bo, so we can upcast for everyone who's both a ttm
   and gem driver.

   This is maybe a good follow-up, since it should allow us to ditch quite
   a bit of the vram helper code for this more generic stuff. I also might
   have missed some special-cases here, but from a quick look everything
   just pins the buffer to the current location and that's it.

   Also this obviously requires Christian's generic ttm_bo_pin rework
   first.

- roll the above out to drivers.

Christian/Thomas, thoughts on this?


Calling this vmap instead of kmap certainly makes sense.

Not 100% sure about the generic helpers, but it sounds like this should 
indeed look rather clean in the end.


Christian.



I think for the immediate need of rolling this out for vram helpers and
fbdev code we should be able to do this, but just postpone the driver wide
roll-out for now.

Cheers, Daniel


-Daniel


Christian.


-Daniel


Thanks,
Christian.


Best regards
Thomas


Regards,
Christian.


Best regards
Thomas


Regards,
Christian.


Signed-off-by: Thomas Zimmermann 
---
 include/drm/ttm/ttm_bo_api.h | 24 ++

Re: [PATCH v3 2/7] drm/ttm: Add ttm_kmap_obj_to_dma_buf_map() for type conversion

2020-10-02 Thread Daniel Vetter 
On Wed, Sep 30, 2020 at 02:51:46PM +0200, Daniel Vetter wrote:
> On Wed, Sep 30, 2020 at 2:34 PM Christian König
>  wrote:
> >
> > Am 30.09.20 um 11:47 schrieb Daniel Vetter:
> > > On Wed, Sep 30, 2020 at 10:34:31AM +0200, Christian König wrote:
> > >> Am 30.09.20 um 10:19 schrieb Thomas Zimmermann:
> > >>> Hi
> > >>>
> > >>> Am 30.09.20 um 10:05 schrieb Christian König:
> >  Am 29.09.20 um 19:49 schrieb Thomas Zimmermann:
> > > Hi Christian
> > >
> > > Am 29.09.20 um 17:35 schrieb Christian König:
> > >> Am 29.09.20 um 17:14 schrieb Thomas Zimmermann:
> > >>> The new helper ttm_kmap_obj_to_dma_buf() extracts address and 
> > >>> location
> > >>> from and instance of TTM's kmap_obj and initializes struct 
> > >>> dma_buf_map
> > >>> with these values. Helpful for TTM-based drivers.
> > >> We could completely drop that if we use the same structure inside 
> > >> TTM as
> > >> well.
> > >>
> > >> Additional to that which driver is going to use this?
> > > As Daniel mentioned, it's in patch 3. The TTM-based drivers will
> > > retrieve the pointer via this function.
> > >
> > > I do want to see all that being more tightly integrated into TTM, but
> > > not in this series. This one is about fixing the bochs-on-sparc64
> > > problem for good. Patch 7 adds an update to TTM to the DRM TODO list.
> >  I should have asked which driver you try to fix here :)
> > 
> >  In this case just keep the function inside bochs and only fix it there.
> > 
> >  All other drivers can be fixed when we generally pump this through TTM.
> > >>> Did you take a look at patch 3? This function will be used by VRAM
> > >>> helpers, nouveau, radeon, amdgpu and qxl. If we don't put it here, we
> > >>> have to duplicate the functionality in each if these drivers. Bochs
> > >>> itself uses VRAM helpers and doesn't touch the function directly.
> > >> Ah, ok can we have that then only in the VRAM helpers?
> > >>
> > >> Alternative you could go ahead and use dma_buf_map in ttm_bo_kmap_obj
> > >> directly and drop the hack with the TTM_BO_MAP_IOMEM_MASK.
> > >>
> > >> What I want to avoid is to have another conversion function in TTM 
> > >> because
> > >> what happens here is that we already convert from ttm_bus_placement to
> > >> ttm_bo_kmap_obj and then to dma_buf_map.
> > > Hm I'm not really seeing how that helps with a gradual conversion of
> > > everything over to dma_buf_map and assorted helpers for access? There's
> > > too many places in ttm drivers where is_iomem and related stuff is used to
> > > be able to convert it all in one go. An intermediate state with a bunch of
> > > conversions seems fairly unavoidable to me.
> >
> > Fair enough. I would just have started bottom up and not top down.
> >
> > Anyway feel free to go ahead with this approach as long as we can remove
> > the new function again when we clean that stuff up for good.
> 
> Yeah I guess bottom up would make more sense as a refactoring. But the
> main motivation to land this here is to fix the __mmio vs normal
> memory confusion in the fbdev emulation helpers for sparc (and
> anything else that needs this). Hence the top down approach for
> rolling this out.

Ok I started reviewing this a bit more in-depth, and I think this is a bit
too much of a de-tour.

Looking through all the callers of ttm_bo_kmap almost everyone maps the
entire object. Only vmwgfx uses to map less than that. Also, everyone just
immediately follows up with converting that full object map into a
pointer.

So I think what we really want here is:
- new function

int ttm_bo_vmap(struct ttm_buffer_object *bo, struct dma_buf_map *map);

  _vmap name since that's consistent with both dma_buf functions and
  what's usually used to implement this. Outside of the ttm world kmap
  usually just means single-page mappings using kmap() or it's iomem
  sibling io_mapping_map* so rather confusing name for a function which
  usually is just used to set up a vmap of the entire buffer.

- a helper which can be used for the drm_gem_object_funcs vmap/vunmap
  functions for all ttm drivers. We should be able to make this fully
  generic because a) we now have dma_buf_map and b) drm_gem_object is
  embedded in the ttm_bo, so we can upcast for everyone who's both a ttm
  and gem driver.

  This is maybe a good follow-up, since it should allow us to ditch quite
  a bit of the vram helper code for this more generic stuff. I also might
  have missed some special-cases here, but from a quick look everything
  just pins the buffer to the current location and that's it.

  Also this obviously requires Christian's generic ttm_bo_pin rework
  first.

- roll the above out to drivers.

Christian/Thomas, thoughts on this?

I think for the immediate need of rolling this out for vram helpers and
fbdev code we should be able to do this, but just postpone the driver wide
roll-out for now.

Cheers, Daniel

> -Daniel
> 
> >
> > Christia

Re: [PATCH v3 1/7] drm/vram-helper: Remove invariant parameters from internal kmap function

2020-10-02 Thread Daniel Vetter 
On Tue, Sep 29, 2020 at 05:14:31PM +0200, Thomas Zimmermann wrote:
> The parameters map and is_iomem are always of the same value. Removed them
> to prepares the function for conversion to struct dma_buf_map.
> 
> Signed-off-by: Thomas Zimmermann 

Reviewed-by: Daniel Vetter 

> ---
>  drivers/gpu/drm/drm_gem_vram_helper.c | 17 ++---
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c 
> b/drivers/gpu/drm/drm_gem_vram_helper.c
> index 3fe4b326e18e..256b346664f2 100644
> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
> @@ -382,16 +382,16 @@ int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo)
>  }
>  EXPORT_SYMBOL(drm_gem_vram_unpin);
>  
> -static void *drm_gem_vram_kmap_locked(struct drm_gem_vram_object *gbo,
> -   bool map, bool *is_iomem)
> +static void *drm_gem_vram_kmap_locked(struct drm_gem_vram_object *gbo)
>  {
>   int ret;
>   struct ttm_bo_kmap_obj *kmap = &gbo->kmap;
> + bool is_iomem;
>  
>   if (gbo->kmap_use_count > 0)
>   goto out;
>  
> - if (kmap->virtual || !map)
> + if (kmap->virtual)
>   goto out;
>  
>   ret = ttm_bo_kmap(&gbo->bo, 0, gbo->bo.num_pages, kmap);
> @@ -399,15 +399,10 @@ static void *drm_gem_vram_kmap_locked(struct 
> drm_gem_vram_object *gbo,
>   return ERR_PTR(ret);
>  
>  out:
> - if (!kmap->virtual) {
> - if (is_iomem)
> - *is_iomem = false;
> + if (!kmap->virtual)
>   return NULL; /* not mapped; don't increment ref */
> - }
>   ++gbo->kmap_use_count;
> - if (is_iomem)
> - return ttm_kmap_obj_virtual(kmap, is_iomem);
> - return kmap->virtual;
> + return ttm_kmap_obj_virtual(kmap, &is_iomem);
>  }
>  
>  static void drm_gem_vram_kunmap_locked(struct drm_gem_vram_object *gbo)
> @@ -452,7 +447,7 @@ void *drm_gem_vram_vmap(struct drm_gem_vram_object *gbo)
>   ret = drm_gem_vram_pin_locked(gbo, 0);
>   if (ret)
>   goto err_ttm_bo_unreserve;
> - base = drm_gem_vram_kmap_locked(gbo, true, NULL);
> + base = drm_gem_vram_kmap_locked(gbo);
>   if (IS_ERR(base)) {
>   ret = PTR_ERR(base);
>   goto err_drm_gem_vram_unpin_locked;
> -- 
> 2.28.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization