[PATCH] Fix BSR to allow mmap of small BSR on 64k kernel

[PATCH] Fix BSR to allow mmap of small BSR on 64k kernel

[Sonny Rao]

Fix the BSR driver to allow small BSR devices, which are limited to a
single 4k space, on a 64k page kernel.  Previously the driver would
reject the mmap since the size was smaller than PAGESIZE (or because
the size was greater than the size of the device).  Now, we check for
this case use remap_4k_pfn(). Also, take out code to set vm_flags,
as the remap_pfn functions will do this for us.


Signed-off-by: Sonny Rao <sonnyrao@us.ibm.com>

Index: common/drivers/char/bsr.c
===================================================================
--- common.orig/drivers/char/bsr.c	2008-11-06 16:43:58.000000000 -0600
+++ common/drivers/char/bsr.c	2008-11-06 18:30:41.000000000 -0600
@@ -27,6 +27,7 @@
 #include <linux/cdev.h>
 #include <linux/list.h>
 #include <linux/mm.h>
+#include <asm/pgtable.h>
 #include <asm/io.h>
 
 /*
@@ -115,15 +116,23 @@
 {
 	unsigned long size   = vma->vm_end - vma->vm_start;
 	struct bsr_dev *dev = filp->private_data;
+	int ret;
 
-	if (size > dev->bsr_len || (size & (PAGE_SIZE-1)))
-		return -EINVAL;
+	/* This is legal where we have a BSR on a 4k page but a 64k kernel */
+	if (size > dev->bsr_len)
+		size = dev->bsr_len;
 
-	vma->vm_flags |= (VM_IO | VM_DONTEXPAND);
 	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
 
-	if (io_remap_pfn_range(vma, vma->vm_start, dev->bsr_addr >> PAGE_SHIFT,
-			       size, vma->vm_page_prot))
+	if (dev->bsr_len < PAGE_SIZE)
+		ret = remap_4k_pfn(vma, vma->vm_start, dev->bsr_addr >> 12,
+				   vma->vm_page_prot);
+	else
+		ret = io_remap_pfn_range(vma, vma->vm_start,
+					 dev->bsr_addr >> PAGE_SHIFT,
+					 size, vma->vm_page_prot);
+
+	if (ret)
 		return -EAGAIN;
 
 	return 0;

Re: [PATCH] Fix BSR to allow mmap of small BSR on 64k kernel

[Paul Mackerras]

Sonny Rao writes:

> Fix the BSR driver to allow small BSR devices, which are limited to a
> single 4k space, on a 64k page kernel.  Previously the driver would
> reject the mmap since the size was smaller than PAGESIZE (or because
> the size was greater than the size of the device).  Now, we check for
> this case use remap_4k_pfn(). Also, take out code to set vm_flags,
> as the remap_pfn functions will do this for us.

Thanks.

Do we know that the BSR size will always be 4k if it's not a multiple
of 64k?  Is it possible that we could get 8k, 16k or 32k or BSRs?
If it is possible, what does the user need to be able to do?  Do they
just want to map 4k, or might then want to map the whole thing?

Paul.

Re: [PATCH] Fix BSR to allow mmap of small BSR on 64k kernel

[Sonny Rao]

On Fri, Nov 07, 2008 at 04:28:29PM +1100, Paul Mackerras wrote:
> Sonny Rao writes:
> 
> > Fix the BSR driver to allow small BSR devices, which are limited to a
> > single 4k space, on a 64k page kernel.  Previously the driver would
> > reject the mmap since the size was smaller than PAGESIZE (or because
> > the size was greater than the size of the device).  Now, we check for
> > this case use remap_4k_pfn(). Also, take out code to set vm_flags,
> > as the remap_pfn functions will do this for us.
> 
> Thanks.
> 
> Do we know that the BSR size will always be 4k if it's not a multiple
> of 64k?  Is it possible that we could get 8k, 16k or 32k or BSRs?
> If it is possible, what does the user need to be able to do?  Do they
> just want to map 4k, or might then want to map the whole thing?

Hi Paul, the BSR comes in 4 different sizes, 8, 16, 64, 128.

The 8 byte BSR registers are always contained to 4k pages and are
always representing a piece of a larger BSR, but can be assigned to
individual LPARs.  

The 16 byte BSR is contained in two 4k pages, and so the code as
patched would not allow both 4k pages to be mapped.  However, I don't
see any reason for the user to need both 4k pages.  

To give some background as to why the BSR exists in multiple pages at
all I'll say that one proposed way to use the BSR is to have each
participating cpu "own" a cache-line sized piece of the BSR mapped
page and write only to that piece.  The reasoning is that using this approach,
software could use either a BSR or regular cachable memory for the
barrier operation, and I would not need to know which it is actually
using.  So in this type of scenario, there should be enough
cache-lines sized pieces mappable. In the case of the 16 byte BSR, one
4k page contains 32 128byte cache-line pieces.  So this is enough to
still use the BSR in this way.

The 64 byte BSR is contained in 16 4k-pages -- so one 64k page works
there, and the 128 byte BSR is contained in 32 4k pages.

The case of the 16 byte BSR is the only one where it matters, I can
change the code to map both 4k pages if the user requests it, but I
don't see any extra utility.  For consistency though, maybe we should
reject a request to map more than 4k but less than 64k on a 64k kernel?

Sonny

Re: [PATCH] Fix BSR to allow mmap of small BSR on 64k kernel

[Sonny Rao]

On Fri, Nov 07, 2008 at 04:28:29PM +1100, Paul Mackerras wrote:
> Sonny Rao writes:
> 
> > Fix the BSR driver to allow small BSR devices, which are limited to a
> > single 4k space, on a 64k page kernel.  Previously the driver would
> > reject the mmap since the size was smaller than PAGESIZE (or because
> > the size was greater than the size of the device).  Now, we check for
> > this case use remap_4k_pfn(). Also, take out code to set vm_flags,
> > as the remap_pfn functions will do this for us.
> 
> Thanks.
> 
> Do we know that the BSR size will always be 4k if it's not a multiple
> of 64k?  Is it possible that we could get 8k, 16k or 32k or BSRs?
> If it is possible, what does the user need to be able to do?  Do they
> just want to map 4k, or might then want to map the whole thing?


Hi Paul, I took a look at changing the driver to reject a request for
mapping more than a single 4k page, however the only indication we get
of the requested size in the mmap function is the vma size, and this
is always one page at minimum.  So, it's not possible to determine if
the user wants one 4k page or more.  As I noted in my first response,
there is only one case where this is even possible and I don't think
it is a significant concern.

I did notice that I left out the check to see if the user is trying to
map more than the device length, so I fixed that.  Here's the revised
patch.

--------

Fix the BSR driver to allow small BSR devices on a 64k page kernel.  
Previously the driver would reject the mmap since the size was smaller
than PAGESIZE. This patch adds a  check for this case and uses remap_4k_pfn().

Also, take out code to set vm_flags, as the remap_pfn functions will
do this for us.

Signed-off-by: Sonny Rao <sonnyrao@us.ibm.com>

Index: linux/drivers/char/bsr.c
===================================================================
--- linux.orig/drivers/char/bsr.c	2008-11-17 00:29:23.000000000 -0600
+++ linux/drivers/char/bsr.c	2008-11-17 00:59:57.000000000 -0600
@@ -27,6 +27,7 @@
 #include <linux/cdev.h>
 #include <linux/list.h>
 #include <linux/mm.h>
+#include <asm/pgtable.h>
 #include <asm/io.h>
 
 /*
@@ -115,15 +116,22 @@
 {
 	unsigned long size   = vma->vm_end - vma->vm_start;
 	struct bsr_dev *dev = filp->private_data;
+	int ret;
 
-	if (size > dev->bsr_len || (size & (PAGE_SIZE-1)))
-		return -EINVAL;
-
-	vma->vm_flags |= (VM_IO | VM_DONTEXPAND);
 	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
 
-	if (io_remap_pfn_range(vma, vma->vm_start, dev->bsr_addr >> PAGE_SHIFT,
-			       size, vma->vm_page_prot))
+	/* check for the case of a small BSR device and map one 4k page for it*/
+	if (dev->bsr_len < PAGE_SIZE && size == PAGE_SIZE)
+		ret = remap_4k_pfn(vma, vma->vm_start, dev->bsr_addr >> 12,
+				   vma->vm_page_prot);
+	else if (size <= dev->bsr_len)
+		ret = io_remap_pfn_range(vma, vma->vm_start,
+					 dev->bsr_addr >> PAGE_SHIFT,
+					 size, vma->vm_page_prot);
+	else
+		return -EINVAL;
+
+	if (ret)
 		return -EAGAIN;
 
 	return 0;

Re: [PATCH] Fix BSR to allow mmap of small BSR on 64k kernel

[Paul Mackerras]

Sonny Rao writes:

> -	if (io_remap_pfn_range(vma, vma->vm_start, dev->bsr_addr >> PAGE_SHIFT,
> -			       size, vma->vm_page_prot))
> +	/* check for the case of a small BSR device and map one 4k page for it*/
> +	if (dev->bsr_len < PAGE_SIZE && size == PAGE_SIZE)
> +		ret = remap_4k_pfn(vma, vma->vm_start, dev->bsr_addr >> 12,
> +				   vma->vm_page_prot);

I think we should be checking that dev->bsr_len == 4096 here.

Paul.

Re: [PATCH] Fix BSR to allow mmap of small BSR on 64k kernel

[Sonny Rao]

On Wed, Nov 19, 2008 at 03:07:04PM +1100, Paul Mackerras wrote:
> Sonny Rao writes:
> 
> > -	if (io_remap_pfn_range(vma, vma->vm_start, dev->bsr_addr >> PAGE_SHIFT,
> > -			       size, vma->vm_page_prot))
> > +	/* check for the case of a small BSR device and map one 4k page for it*/
> > +	if (dev->bsr_len < PAGE_SIZE && size == PAGE_SIZE)
> > +		ret = remap_4k_pfn(vma, vma->vm_start, dev->bsr_addr >> 12,
> > +				   vma->vm_page_prot);
> 
> I think we should be checking that dev->bsr_len == 4096 here.
> 
> Paul.

Well, dev->bsr_len could be 4096 or 8192

-- 
Sonny Rao, LTC OzLabs, BML team

Re: [PATCH] Fix BSR to allow mmap of small BSR on 64k kernel

[Paul Mackerras]

Sonny Rao writes:

> On Wed, Nov 19, 2008 at 03:07:04PM +1100, Paul Mackerras wrote:
> > I think we should be checking that dev->bsr_len == 4096 here.
> > 
> > Paul.
> 
> Well, dev->bsr_len could be 4096 or 8192

Isn't the dev->bsr_len == 8192 case the one where we'll only map 4096
bytes and therefore not do what the user expected?  Sounds to me like
we want to return an error for that case.

Paul.

Re: [PATCH] Fix BSR to allow mmap of small BSR on 64k kernel

[Sonny Rao]

On Thu, Nov 20, 2008 at 09:54:21AM +1100, Paul Mackerras wrote:
> Sonny Rao writes:
> 
> > On Wed, Nov 19, 2008 at 03:07:04PM +1100, Paul Mackerras wrote:
> > > I think we should be checking that dev->bsr_len == 4096 here.
> > > 
> > > Paul.
> > 
> > Well, dev->bsr_len could be 4096 or 8192
> 
> Isn't the dev->bsr_len == 8192 case the one where we'll only map 4096
> bytes and therefore not do what the user expected?  Sounds to me like
> we want to return an error for that case.

Well, the problem is that we can't tell if the user asked for 4k or
8k (since we only know the size of the VMA).  

If we fail whenever dev->bsr_len is 8k then the user could never
map that device on a 64k page kernel. Is that what we want?


-- 
Sonny Rao, LTC OzLabs, BML team

Re: [PATCH] Fix BSR to allow mmap of small BSR on 64k kernel

[Sonny Rao]

On Mon, Nov 17, 2008 at 01:26:13AM -0600, Sonny Rao wrote:
> On Fri, Nov 07, 2008 at 04:28:29PM +1100, Paul Mackerras wrote:
> > Sonny Rao writes:
> > 
> > > Fix the BSR driver to allow small BSR devices, which are limited to a
> > > single 4k space, on a 64k page kernel.  Previously the driver would
> > > reject the mmap since the size was smaller than PAGESIZE (or because
> > > the size was greater than the size of the device).  Now, we check for
> > > this case use remap_4k_pfn(). Also, take out code to set vm_flags,
> > > as the remap_pfn functions will do this for us.
> > 
> > Thanks.
> > 
> > Do we know that the BSR size will always be 4k if it's not a multiple
> > of 64k?  Is it possible that we could get 8k, 16k or 32k or BSRs?
> > If it is possible, what does the user need to be able to do?  Do they
> > just want to map 4k, or might then want to map the whole thing?
> 
> 
> Hi Paul, I took a look at changing the driver to reject a request for
> mapping more than a single 4k page, however the only indication we get
> of the requested size in the mmap function is the vma size, and this
> is always one page at minimum.  So, it's not possible to determine if
> the user wants one 4k page or more.  As I noted in my first response,
> there is only one case where this is even possible and I don't think
> it is a significant concern.
> 
> I did notice that I left out the check to see if the user is trying to
> map more than the device length, so I fixed that.  Here's the revised
> patch.

Alright, I've reworked this now so that if we get one of these cases
where there's a bsr that's > 4k and < 64k on a 64k kernel we'll only
advertise that it is a 4k BSR to userspace.  I think this is the best
solution since user programs are only supposed to look at sysfs to
determine how much can be mapped, and libbsr does this as well.

Please consider for 2.6.31 as a fix, thanks.

---

Fix the BSR driver to allow small BSR devices on a 64k page kernel.  
Previously the driver would reject the mmap since the size was smaller
than PAGESIZE. This patch adds a check for this case and uses remap_4k_pfn().

There are also casees where we have a size that is greater than 4k but
smaller than 64k, and in that case we would only map the first 4k using
remap_4k_pfn, so we also change the length that we advertise in sysfs, 
so the user knows they can only map 4k. 

Also, take out code to set vm_flags, as the remap_pfn functions will
do this for us.

Signed-off-by: Sonny Rao <sonnyrao@us.ibm.com>

Index: linux-2.6.30/drivers/char/bsr.c
===================================================================
--- linux-2.6.30.orig/drivers/char/bsr.c	2009-06-18 13:02:16.000000000 -0500
+++ linux-2.6.30/drivers/char/bsr.c	2009-06-18 18:18:29.000000000 -0500
@@ -27,6 +27,7 @@
 #include <linux/cdev.h>
 #include <linux/list.h>
 #include <linux/mm.h>
+#include <asm/pgtable.h>
 #include <asm/io.h>
 
 /*
@@ -117,15 +118,22 @@
 {
 	unsigned long size   = vma->vm_end - vma->vm_start;
 	struct bsr_dev *dev = filp->private_data;
+	int ret;
 
-	if (size > dev->bsr_len || (size & (PAGE_SIZE-1)))
-		return -EINVAL;
-
-	vma->vm_flags |= (VM_IO | VM_DONTEXPAND);
 	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
 
-	if (io_remap_pfn_range(vma, vma->vm_start, dev->bsr_addr >> PAGE_SHIFT,
-			       size, vma->vm_page_prot))
+	/* check for the case of a small BSR device and map one 4k page for it*/
+	if (dev->bsr_len < PAGE_SIZE && size == PAGE_SIZE)
+		ret = remap_4k_pfn(vma, vma->vm_start, dev->bsr_addr >> 12,
+				   vma->vm_page_prot);
+	else if (size <= dev->bsr_len)
+		ret = io_remap_pfn_range(vma, vma->vm_start,
+					 dev->bsr_addr >> PAGE_SHIFT,
+					 size, vma->vm_page_prot);
+	else
+		return -EINVAL;
+
+	if (ret)
 		return -EAGAIN;
 
 	return 0;
@@ -205,6 +213,11 @@
 		cur->bsr_stride = bsr_stride[i];
 		cur->bsr_dev    = MKDEV(bsr_major, i + total_bsr_devs);
 
+		/* if we have a bsr_len of > 4k and less then PAGE_SIZE (64k pages) */
+		/* we can only map 4k of it, so only advertise the 4k in sysfs */
+		if (cur->bsr_len > 4096 && cur->bsr_len < PAGE_SIZE)
+			cur->bsr_len = 4096;
+
 		switch(cur->bsr_bytes) {
 		case 8:
 			cur->bsr_type = BSR_8;