[PATCH 1/2] xen/privcmd: Corrected error handling path and mark pages dirty

[PATCH 1/2] xen/privcmd: Corrected error handling path and mark pages dirty

[Souptick Joarder]

Previously, if lock_pages() end up partially mapping pages, it used
to return -ERRNO due to which unlock_pages() have to go through
each pages[i] till *nr_pages* to validate them. This can be avoided
by passing correct number of partially mapped pages & -ERRNO separately,
while returning from lock_pages() due to error.

With this fix unlock_pages() doesn't need to validate pages[i] till
*nr_pages* for error scenario and few condition checks can be ignored.

As discussed, pages need to be marked as dirty before unpinned it in
unlock_pages() which was oversight.

Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
Cc: John Hubbard <jhubbard@nvidia.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Paul Durrant <xadimgnik@gmail.com>
---
Hi,

I'm compile tested this, but unable to run-time test, so any testing
help is much appriciated.

 drivers/xen/privcmd.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index a250d11..0da417c 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -580,43 +580,44 @@ static long privcmd_ioctl_mmap_batch(
 
 static int lock_pages(
 	struct privcmd_dm_op_buf kbufs[], unsigned int num,
-	struct page *pages[], unsigned int nr_pages)
+	struct page *pages[], unsigned int nr_pages, int *pinned)
 {
 	unsigned int i;
+	int errno = 0, page_count = 0;
 
 	for (i = 0; i < num; i++) {
 		unsigned int requested;
-		int pinned;
 
+		*pinned += page_count;
 		requested = DIV_ROUND_UP(
 			offset_in_page(kbufs[i].uptr) + kbufs[i].size,
 			PAGE_SIZE);
 		if (requested > nr_pages)
 			return -ENOSPC;
 
-		pinned = get_user_pages_fast(
+		page_count = get_user_pages_fast(
 			(unsigned long) kbufs[i].uptr,
 			requested, FOLL_WRITE, pages);
-		if (pinned < 0)
-			return pinned;
+		if (page_count < 0) {
+			errno = page_count;
+			return errno;
+		}
 
-		nr_pages -= pinned;
-		pages += pinned;
+		nr_pages -= page_count;
+		pages += page_count;
 	}
 
-	return 0;
+	return errno;
 }
 
 static void unlock_pages(struct page *pages[], unsigned int nr_pages)
 {
 	unsigned int i;
 
-	if (!pages)
-		return;
-
 	for (i = 0; i < nr_pages; i++) {
-		if (pages[i])
-			put_page(pages[i]);
+		if (!PageDirty(page))
+			set_page_dirty_lock(page);
+		put_page(pages[i]);
 	}
 }
 
@@ -630,6 +631,7 @@ static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
 	struct xen_dm_op_buf *xbufs = NULL;
 	unsigned int i;
 	long rc;
+	int pinned = 0;
 
 	if (copy_from_user(&kdata, udata, sizeof(kdata)))
 		return -EFAULT;
@@ -683,9 +685,11 @@ static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
 		goto out;
 	}
 
-	rc = lock_pages(kbufs, kdata.num, pages, nr_pages);
-	if (rc)
+	rc = lock_pages(kbufs, kdata.num, pages, nr_pages, &pinned);
+	if (rc < 0) {
+		nr_pages = pinned;
 		goto out;
+	}
 
 	for (i = 0; i < kdata.num; i++) {
 		set_xen_guest_handle(xbufs[i].h, kbufs[i].uptr);
-- 
1.9.1

[PATCH 2/2] xen/privcmd: Convert get_user_pages*() to pin_user_pages*()

[Souptick Joarder]

In 2019, we introduced pin_user_pages*() and now we are converting
get_user_pages*() to the new API as appropriate. [1] & [2] could
be referred for more information. This is case 5 as per document [1].

[1] Documentation/core-api/pin_user_pages.rst

[2] "Explicit pinning of user-space pages":
        https://lwn.net/Articles/807108/

Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
Cc: John Hubbard <jhubbard@nvidia.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Paul Durrant <xadimgnik@gmail.com>
---
Hi,

I'm compile tested this, but unable to run-time test, so any testing
help is much appriciated.

 drivers/xen/privcmd.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 0da417c..eb05254 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -595,7 +595,7 @@ static int lock_pages(
 		if (requested > nr_pages)
 			return -ENOSPC;
 
-		page_count = get_user_pages_fast(
+		page_count = pin_user_pages_fast(
 			(unsigned long) kbufs[i].uptr,
 			requested, FOLL_WRITE, pages);
 		if (page_count < 0) {
@@ -612,13 +612,7 @@ static int lock_pages(
 
 static void unlock_pages(struct page *pages[], unsigned int nr_pages)
 {
-	unsigned int i;
-
-	for (i = 0; i < nr_pages; i++) {
-		if (!PageDirty(page))
-			set_page_dirty_lock(page);
-		put_page(pages[i]);
-	}
+	unpin_user_pages_dirty_lock(pages, nr_pages, 1);
 }
 
 static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
-- 
1.9.1

Re: [PATCH 2/2] xen/privcmd: Convert get_user_pages*() to pin_user_pages*()

[John Hubbard]

On 2020-06-24 20:02, Souptick Joarder wrote:
> In 2019, we introduced pin_user_pages*() and now we are converting
> get_user_pages*() to the new API as appropriate. [1] & [2] could
> be referred for more information. This is case 5 as per document [1].
> 
> [1] Documentation/core-api/pin_user_pages.rst
> 
> [2] "Explicit pinning of user-space pages":
>          https://lwn.net/Articles/807108/
> 
> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> Cc: John Hubbard <jhubbard@nvidia.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Paul Durrant <xadimgnik@gmail.com>
> ---
> Hi,
> 
> I'm compile tested this, but unable to run-time test, so any testing
> help is much appriciated.
> 
>   drivers/xen/privcmd.c | 10 ++--------
>   1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index 0da417c..eb05254 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -595,7 +595,7 @@ static int lock_pages(
>   		if (requested > nr_pages)
>   			return -ENOSPC;
>   
> -		page_count = get_user_pages_fast(
> +		page_count = pin_user_pages_fast(
>   			(unsigned long) kbufs[i].uptr,
>   			requested, FOLL_WRITE, pages);
>   		if (page_count < 0) {
> @@ -612,13 +612,7 @@ static int lock_pages(
>   
>   static void unlock_pages(struct page *pages[], unsigned int nr_pages)
>   {
> -	unsigned int i;
> -
> -	for (i = 0; i < nr_pages; i++) {
> -		if (!PageDirty(page))
> -			set_page_dirty_lock(page);
> -		put_page(pages[i]);
> -	}
> +	unpin_user_pages_dirty_lock(pages, nr_pages, 1);

"true", not "1", is the correct way to call that function.

Also, this approach changes the behavior slightly, but I think it's
reasonable to just set_page_dirty_lock() on the whole range--hard to
see much benefit in checking PageDirty first.


>   }
>   
>   static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
> 

thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH 1/2] xen/privcmd: Corrected error handling path and mark pages dirty

[Boris Ostrovsky]

On 6/24/20 11:02 PM, Souptick Joarder wrote:
> Previously, if lock_pages() end up partially mapping pages, it used
> to return -ERRNO due to which unlock_pages() have to go through
> each pages[i] till *nr_pages* to validate them. This can be avoided
> by passing correct number of partially mapped pages & -ERRNO separately,
> while returning from lock_pages() due to error.
>
> With this fix unlock_pages() doesn't need to validate pages[i] till
> *nr_pages* for error scenario and few condition checks can be ignored.
>
> As discussed, pages need to be marked as dirty before unpinned it in
> unlock_pages() which was oversight.


There are two unrelated changes here (improving error path and marking
pages dirty), they should be handled by separate patches.


(I assume marking pages dirty is something you want to go to stable tree
since otherwise there is no reason for making this change)


>
> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> Cc: John Hubbard <jhubbard@nvidia.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Paul Durrant <xadimgnik@gmail.com>
> ---
> Hi,
>
> I'm compile tested this,


I don't think so.


>  but unable to run-time test, so any testing
> help is much appriciated.
>
>  drivers/xen/privcmd.c | 34 +++++++++++++++++++---------------
>  1 file changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index a250d11..0da417c 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -580,43 +580,44 @@ static long privcmd_ioctl_mmap_batch(
>  
>  static int lock_pages(
>  	struct privcmd_dm_op_buf kbufs[], unsigned int num,
> -	struct page *pages[], unsigned int nr_pages)
> +	struct page *pages[], unsigned int nr_pages, int *pinned)
>  {
>  	unsigned int i;
> +	int errno = 0, page_count = 0;


No need for error, really --- you can return the value immediately.


>  
>  	for (i = 0; i < num; i++) {
>  		unsigned int requested;
> -		int pinned;
>  
> +		*pinned += page_count;


I'd move this lower, after a successful call to get_user_pages_fast()
(and then you won't need to initialize it)


>  		requested = DIV_ROUND_UP(
>  			offset_in_page(kbufs[i].uptr) + kbufs[i].size,
>  			PAGE_SIZE);
>  		if (requested > nr_pages)
>  			return -ENOSPC;
>  
> -		pinned = get_user_pages_fast(
> +		page_count = get_user_pages_fast(
>  			(unsigned long) kbufs[i].uptr,
>  			requested, FOLL_WRITE, pages);
> -		if (pinned < 0)
> -			return pinned;
> +		if (page_count < 0) {
> +			errno = page_count;
> +			return errno;
> +		}
>  
> -		nr_pages -= pinned;
> -		pages += pinned;
> +		nr_pages -= page_count;
> +		pages += page_count;
>  	}
>  
> -	return 0;
> +	return errno;
>  }
>  
>  static void unlock_pages(struct page *pages[], unsigned int nr_pages)
>  {
>  	unsigned int i;
>  
> -	if (!pages)
> -		return;
> -
>  	for (i = 0; i < nr_pages; i++) {
> -		if (pages[i])
> -			put_page(pages[i]);
> +		if (!PageDirty(page))
> +			set_page_dirty_lock(page);
> +		put_page(pages[i]);
>  	}


This won't compile.


-boris

Re: [PATCH 2/2] xen/privcmd: Convert get_user_pages*() to pin_user_pages*()

[Souptick Joarder]

On Thu, Jun 25, 2020 at 11:19 AM John Hubbard <jhubbard@nvidia.com> wrote:
>
> On 2020-06-24 20:02, Souptick Joarder wrote:
> > In 2019, we introduced pin_user_pages*() and now we are converting
> > get_user_pages*() to the new API as appropriate. [1] & [2] could
> > be referred for more information. This is case 5 as per document [1].
> >
> > [1] Documentation/core-api/pin_user_pages.rst
> >
> > [2] "Explicit pinning of user-space pages":
> >          https://lwn.net/Articles/807108/
> >
> > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> > Cc: John Hubbard <jhubbard@nvidia.com>
> > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > Cc: Paul Durrant <xadimgnik@gmail.com>
> > ---
> > Hi,
> >
> > I'm compile tested this, but unable to run-time test, so any testing
> > help is much appriciated.
> >
> >   drivers/xen/privcmd.c | 10 ++--------
> >   1 file changed, 2 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> > index 0da417c..eb05254 100644
> > --- a/drivers/xen/privcmd.c
> > +++ b/drivers/xen/privcmd.c
> > @@ -595,7 +595,7 @@ static int lock_pages(
> >               if (requested > nr_pages)
> >                       return -ENOSPC;
> >
> > -             page_count = get_user_pages_fast(
> > +             page_count = pin_user_pages_fast(
> >                       (unsigned long) kbufs[i].uptr,
> >                       requested, FOLL_WRITE, pages);
> >               if (page_count < 0) {
> > @@ -612,13 +612,7 @@ static int lock_pages(
> >
> >   static void unlock_pages(struct page *pages[], unsigned int nr_pages)
> >   {
> > -     unsigned int i;
> > -
> > -     for (i = 0; i < nr_pages; i++) {
> > -             if (!PageDirty(page))
> > -                     set_page_dirty_lock(page);
> > -             put_page(pages[i]);
> > -     }
> > +     unpin_user_pages_dirty_lock(pages, nr_pages, 1);
>
> "true", not "1", is the correct way to call that function.

Ok.

>
> Also, this approach changes the behavior slightly, but I think it's
> reasonable to just set_page_dirty_lock() on the whole range--hard to
> see much benefit in checking PageDirty first.

unpin_user_pages_dirty_lock() internally will do the same check after
patch [2/2]
So I thought to keep old and new code in sync. Shall we avoid this check ?


>
>
> >   }
> >
> >   static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
> >
>
> thanks,
> --
> John Hubbard
> NVIDIA

Re: [PATCH 1/2] xen/privcmd: Corrected error handling path and mark pages dirty

[Souptick Joarder]

On Fri, Jun 26, 2020 at 5:01 AM Boris Ostrovsky
<boris.ostrovsky@oracle.com> wrote:
>
> On 6/24/20 11:02 PM, Souptick Joarder wrote:
> > Previously, if lock_pages() end up partially mapping pages, it used
> > to return -ERRNO due to which unlock_pages() have to go through
> > each pages[i] till *nr_pages* to validate them. This can be avoided
> > by passing correct number of partially mapped pages & -ERRNO separately,
> > while returning from lock_pages() due to error.
> >
> > With this fix unlock_pages() doesn't need to validate pages[i] till
> > *nr_pages* for error scenario and few condition checks can be ignored.
> >
> > As discussed, pages need to be marked as dirty before unpinned it in
> > unlock_pages() which was oversight.
>
>
> There are two unrelated changes here (improving error path and marking
> pages dirty), they should be handled by separate patches.

Sure, will do it in v2.

>
>
> (I assume marking pages dirty is something you want to go to stable tree
> since otherwise there is no reason for making this change)
>
>
> >
> > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> > Cc: John Hubbard <jhubbard@nvidia.com>
> > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > Cc: Paul Durrant <xadimgnik@gmail.com>
> > ---
> > Hi,
> >
> > I'm compile tested this,
>
>
> I don't think so.

I compile test it against ARM and it was fine.
Against which ARCH is it failing ?

>
>
> >  but unable to run-time test, so any testing
> > help is much appriciated.
> >
> >  drivers/xen/privcmd.c | 34 +++++++++++++++++++---------------
> >  1 file changed, 19 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> > index a250d11..0da417c 100644
> > --- a/drivers/xen/privcmd.c
> > +++ b/drivers/xen/privcmd.c
> > @@ -580,43 +580,44 @@ static long privcmd_ioctl_mmap_batch(
> >
> >  static int lock_pages(
> >       struct privcmd_dm_op_buf kbufs[], unsigned int num,
> > -     struct page *pages[], unsigned int nr_pages)
> > +     struct page *pages[], unsigned int nr_pages, int *pinned)
> >  {
> >       unsigned int i;
> > +     int errno = 0, page_count = 0;
>
>
> No need for error, really --- you can return the value immediately.

yes, this is unnecessary.

>
>
> >
> >       for (i = 0; i < num; i++) {
> >               unsigned int requested;
> > -             int pinned;
> >
> > +             *pinned += page_count;
>
>
> I'd move this lower, after a successful call to get_user_pages_fast()
> (and then you won't need to initialize it)

Ok.

>
>
> >               requested = DIV_ROUND_UP(
> >                       offset_in_page(kbufs[i].uptr) + kbufs[i].size,
> >                       PAGE_SIZE);
> >               if (requested > nr_pages)
> >                       return -ENOSPC;
> >
> > -             pinned = get_user_pages_fast(
> > +             page_count = get_user_pages_fast(
> >                       (unsigned long) kbufs[i].uptr,
> >                       requested, FOLL_WRITE, pages);
> > -             if (pinned < 0)
> > -                     return pinned;
> > +             if (page_count < 0) {
> > +                     errno = page_count;
> > +                     return errno;
> > +             }
> >
> > -             nr_pages -= pinned;
> > -             pages += pinned;
> > +             nr_pages -= page_count;
> > +             pages += page_count;
> >       }
> >
> > -     return 0;
> > +     return errno;
> >  }
> >
> >  static void unlock_pages(struct page *pages[], unsigned int nr_pages)
> >  {
> >       unsigned int i;
> >
> > -     if (!pages)
> > -             return;
> > -
> >       for (i = 0; i < nr_pages; i++) {
> > -             if (pages[i])
> > -                     put_page(pages[i]);
> > +             if (!PageDirty(page))
> > +                     set_page_dirty_lock(page);
> > +             put_page(pages[i]);
> >       }
>
>
> This won't compile.
>
>
> -boris
>
>
>

Re: [PATCH 1/2] xen/privcmd: Corrected error handling path and mark pages dirty

[Jürgen Groß]

On 25.06.20 05:02, Souptick Joarder wrote:
> Previously, if lock_pages() end up partially mapping pages, it used
> to return -ERRNO due to which unlock_pages() have to go through
> each pages[i] till *nr_pages* to validate them. This can be avoided
> by passing correct number of partially mapped pages & -ERRNO separately,
> while returning from lock_pages() due to error.
> 
> With this fix unlock_pages() doesn't need to validate pages[i] till
> *nr_pages* for error scenario and few condition checks can be ignored.
> 
> As discussed, pages need to be marked as dirty before unpinned it in
> unlock_pages() which was oversight.
> 
> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> Cc: John Hubbard <jhubbard@nvidia.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Paul Durrant <xadimgnik@gmail.com>
> ---
> Hi,
> 
> I'm compile tested this, but unable to run-time test, so any testing
> help is much appriciated.
> 
>   drivers/xen/privcmd.c | 34 +++++++++++++++++++---------------
>   1 file changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index a250d11..0da417c 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -580,43 +580,44 @@ static long privcmd_ioctl_mmap_batch(
>   
>   static int lock_pages(
>   	struct privcmd_dm_op_buf kbufs[], unsigned int num,
> -	struct page *pages[], unsigned int nr_pages)
> +	struct page *pages[], unsigned int nr_pages, int *pinned)

unsigned int *pinned, please.

>   {
>   	unsigned int i;
> +	int errno = 0, page_count = 0;

Please drop the errno variable. It is misnamed (you never assign an
errno value to it) and not needed, as you can ...

>   
>   	for (i = 0; i < num; i++) {
>   		unsigned int requested;
> -		int pinned;
>   
> +		*pinned += page_count;
>   		requested = DIV_ROUND_UP(
>   			offset_in_page(kbufs[i].uptr) + kbufs[i].size,
>   			PAGE_SIZE);
>   		if (requested > nr_pages)
>   			return -ENOSPC;
>   
> -		pinned = get_user_pages_fast(
> +		page_count = get_user_pages_fast(
>   			(unsigned long) kbufs[i].uptr,
>   			requested, FOLL_WRITE, pages);
> -		if (pinned < 0)
> -			return pinned;
> +		if (page_count < 0) {
> +			errno = page_count;
> +			return errno;

... just return page_count her, and ...

> +		}
>   
> -		nr_pages -= pinned;
> -		pages += pinned;
> +		nr_pages -= page_count;
> +		pages += page_count;
>   	}
>   
> -	return 0;
> +	return errno;

... leave this as it was.

>   }
>   
>   static void unlock_pages(struct page *pages[], unsigned int nr_pages)
>   {
>   	unsigned int i;
>   
> -	if (!pages)
> -		return;
> -
>   	for (i = 0; i < nr_pages; i++) {
> -		if (pages[i])
> -			put_page(pages[i]);
> +		if (!PageDirty(page))
> +			set_page_dirty_lock(page);

page? Not pages[i]?

> +		put_page(pages[i]);
>   	}
>   }
>   
> @@ -630,6 +631,7 @@ static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
>   	struct xen_dm_op_buf *xbufs = NULL;
>   	unsigned int i;
>   	long rc;
> +	int pinned = 0;
>   
>   	if (copy_from_user(&kdata, udata, sizeof(kdata)))
>   		return -EFAULT;
> @@ -683,9 +685,11 @@ static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
>   		goto out;
>   	}
>   
> -	rc = lock_pages(kbufs, kdata.num, pages, nr_pages);
> -	if (rc)
> +	rc = lock_pages(kbufs, kdata.num, pages, nr_pages, &pinned);
> +	if (rc < 0) {
> +		nr_pages = pinned;
>   		goto out;
> +	}
>   
>   	for (i = 0; i < kdata.num; i++) {
>   		set_xen_guest_handle(xbufs[i].h, kbufs[i].uptr);
> 


Juergen

Re: [PATCH 1/2] xen/privcmd: Corrected error handling path and mark pages dirty

[Souptick Joarder]

On Fri, Jun 26, 2020 at 11:22 AM Jürgen Groß <jgross@suse.com> wrote:
>
> On 25.06.20 05:02, Souptick Joarder wrote:
> > Previously, if lock_pages() end up partially mapping pages, it used
> > to return -ERRNO due to which unlock_pages() have to go through
> > each pages[i] till *nr_pages* to validate them. This can be avoided
> > by passing correct number of partially mapped pages & -ERRNO separately,
> > while returning from lock_pages() due to error.
> >
> > With this fix unlock_pages() doesn't need to validate pages[i] till
> > *nr_pages* for error scenario and few condition checks can be ignored.
> >
> > As discussed, pages need to be marked as dirty before unpinned it in
> > unlock_pages() which was oversight.
> >
> > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> > Cc: John Hubbard <jhubbard@nvidia.com>
> > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > Cc: Paul Durrant <xadimgnik@gmail.com>
> > ---
> > Hi,
> >
> > I'm compile tested this, but unable to run-time test, so any testing
> > help is much appriciated.
> >
> >   drivers/xen/privcmd.c | 34 +++++++++++++++++++---------------
> >   1 file changed, 19 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> > index a250d11..0da417c 100644
> > --- a/drivers/xen/privcmd.c
> > +++ b/drivers/xen/privcmd.c
> > @@ -580,43 +580,44 @@ static long privcmd_ioctl_mmap_batch(
> >
> >   static int lock_pages(
> >       struct privcmd_dm_op_buf kbufs[], unsigned int num,
> > -     struct page *pages[], unsigned int nr_pages)
> > +     struct page *pages[], unsigned int nr_pages, int *pinned)
>
> unsigned int *pinned, please.
>
> >   {
> >       unsigned int i;
> > +     int errno = 0, page_count = 0;
>
> Please drop the errno variable. It is misnamed (you never assign an
> errno value to it) and not needed, as you can ...
>
> >
> >       for (i = 0; i < num; i++) {
> >               unsigned int requested;
> > -             int pinned;
> >
> > +             *pinned += page_count;
> >               requested = DIV_ROUND_UP(
> >                       offset_in_page(kbufs[i].uptr) + kbufs[i].size,
> >                       PAGE_SIZE);
> >               if (requested > nr_pages)
> >                       return -ENOSPC;
> >
> > -             pinned = get_user_pages_fast(
> > +             page_count = get_user_pages_fast(
> >                       (unsigned long) kbufs[i].uptr,
> >                       requested, FOLL_WRITE, pages);
> > -             if (pinned < 0)
> > -                     return pinned;
> > +             if (page_count < 0) {
> > +                     errno = page_count;
> > +                     return errno;
>
> ... just return page_count her, and ...
>
> > +             }
> >
> > -             nr_pages -= pinned;
> > -             pages += pinned;
> > +             nr_pages -= page_count;
> > +             pages += page_count;
> >       }
> >
> > -     return 0;
> > +     return errno;
>
> ... leave this as it was.
>
> >   }
> >
> >   static void unlock_pages(struct page *pages[], unsigned int nr_pages)
> >   {
> >       unsigned int i;
> >
> > -     if (!pages)
> > -             return;
> > -
> >       for (i = 0; i < nr_pages; i++) {
> > -             if (pages[i])
> > -                     put_page(pages[i]);
> > +             if (!PageDirty(page))
> > +                     set_page_dirty_lock(page);
>
> page? Not pages[i]?

I fixed it in compile branch, but missed it in patch creation work
space at the time of posting.
I think, this is the compile error Boris was pointing to.
Sorry about it. I will fix it and post the v2.

>
> > +             put_page(pages[i]);
> >       }
> >   }
> >
> > @@ -630,6 +631,7 @@ static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
> >       struct xen_dm_op_buf *xbufs = NULL;
> >       unsigned int i;
> >       long rc;
> > +     int pinned = 0;
> >
> >       if (copy_from_user(&kdata, udata, sizeof(kdata)))
> >               return -EFAULT;
> > @@ -683,9 +685,11 @@ static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
> >               goto out;
> >       }
> >
> > -     rc = lock_pages(kbufs, kdata.num, pages, nr_pages);
> > -     if (rc)
> > +     rc = lock_pages(kbufs, kdata.num, pages, nr_pages, &pinned);
> > +     if (rc < 0) {
> > +             nr_pages = pinned;
> >               goto out;
> > +     }
> >
> >       for (i = 0; i < kdata.num; i++) {
> >               set_xen_guest_handle(xbufs[i].h, kbufs[i].uptr);
> >
>
>
> Juergen

Re: [PATCH 2/2] xen/privcmd: Convert get_user_pages*() to pin_user_pages*()

[John Hubbard]

On 2020-06-25 22:26, Souptick Joarder wrote:
> On Thu, Jun 25, 2020 at 11:19 AM John Hubbard <jhubbard@nvidia.com> wrote:
>> On 2020-06-24 20:02, Souptick Joarder wrote:
...
>>> @@ -612,13 +612,7 @@ static int lock_pages(
>>>
>>>    static void unlock_pages(struct page *pages[], unsigned int nr_pages)
>>>    {
>>> -     unsigned int i;
>>> -
>>> -     for (i = 0; i < nr_pages; i++) {
>>> -             if (!PageDirty(page))
>>> -                     set_page_dirty_lock(page);
>>> -             put_page(pages[i]);
>>> -     }
>>> +     unpin_user_pages_dirty_lock(pages, nr_pages, 1);
>>
>> "true", not "1", is the correct way to call that function.
> 
> Ok.
> 
>>
>> Also, this approach changes the behavior slightly, but I think it's

Correction, I forgot that I put that same if(!PageDirty(page)) check into
unpin_user_pages_dirty_lock(). So it doesn't change behavior. That's good.

>> reasonable to just set_page_dirty_lock() on the whole range--hard to
>> see much benefit in checking PageDirty first.
> 
> unpin_user_pages_dirty_lock() internally will do the same check after
> patch [2/2]
> So I thought to keep old and new code in sync. Shall we avoid this check ?
> 
Just leave it as you have it, but of course use "true" instead of 1, please.


thanks,
-- 
John Hubbard
NVIDIA