[PATCH] fb-defio: Inhibit setting VM_IO flag if FBINFO_VIRTFB is set.

[PATCH] fb-defio: Inhibit setting VM_IO flag if FBINFO_VIRTFB is set.

[Konrad Rzeszutek Wilk]

Hey Jaya and Magnus,

I was wondering if I could get your opinion and hopefully an ACK on these
following patches?  They fix an issue when Linux is running in a Xen paravirt
environment with the framebuffer enabled. This bug looks to have been in
existence for a long time and finally has been tracked down.

Essentially the VM_IO flag, that is set in fb_deferred_io_mmap, has a special
meaning for Xen-paravirt Linux. It is used to signify pages that are backed by
PCI devices memory (BARs, and such). Having other types of memory (System RAM)
marked with this flag, throws a monkey wrench in the accounting and we end
up with infinite General Protection Fault occurring during boot-up.

The first two patches fix the problem. The last one is a cleanup of the
other users of fb_deferred that use a framebuffer allocated from System RAM.

Thank you for taking the time to review these patches.

Sincerely,

Konrad Rzeszutek Wilk

[PATCH 1/3] fb-defio: If FBINFO_VIRTFB is defined, do not set VM_IO flag.

[Konrad Rzeszutek Wilk]

Most users (except sh_mobile_lcdcfb.c) get their framebuffer from
vmalloc. Setting VM_IO is not necessary as the memory obtained
from vmalloc is System RAM type and is not susceptible to PCI memory
constraints.
---
 drivers/video/fb_defio.c |    4 +++-
 include/linux/fb.h       |    1 +
 2 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/video/fb_defio.c b/drivers/video/fb_defio.c
index c27ab1e..94414fc 100644
--- a/drivers/video/fb_defio.c
+++ b/drivers/video/fb_defio.c
@@ -144,7 +144,9 @@ static const struct address_space_operations fb_deferred_io_aops = {
 static int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
 {
 	vma->vm_ops = &fb_deferred_io_vm_ops;
-	vma->vm_flags |= ( VM_IO | VM_RESERVED | VM_DONTEXPAND );
+	vma->vm_flags |= ( VM_RESERVED | VM_DONTEXPAND );
+	if (!(info->flags & FBINFO_VIRTFB))
+		vma->vm_flags |= VM_IO;
 	vma->vm_private_data = info;
 	return 0;
 }
diff --git a/include/linux/fb.h b/include/linux/fb.h
index de9c722..369767b 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -763,6 +763,7 @@ struct fb_tile_ops {
 	 *  takes over; acceleration engine should be in a quiescent state */
 
 /* hints */
+#define FBINFO_VIRTFB		0x0004 /* FB is System RAM, not device. */
 #define FBINFO_PARTIAL_PAN_OK	0x0040 /* otw use pan only for double-buffering */
 #define FBINFO_READS_FAST	0x0080 /* soft-copy faster than rendering */
 
-- 
1.6.2.5

[PATCH 2/3] xen pvfb: Inhibit VM_IO flag to be set on vmalloc-ed framebuffers.

[Konrad Rzeszutek Wilk]

In Xen-paravirt mode, VM_IO flag signifies that the page frame number (PFN)
is actually a machine frame number (MFN). This is correct for memory backed by
PCI devices, but wrong for memory allocated from System RAM where the PFN
!= MFN.

During page faults, pages with VM_IO, get assigned to special domain I/O
domain and as said, the PFN is interpreted as MFN. When Xen hypervisor
modifies the PTE it interprets the PFN as the MFN, complains and
fails the PTE modification.

The end result is an infinitive page fault in the domain.
---
 drivers/video/xen-fbfront.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/video/xen-fbfront.c b/drivers/video/xen-fbfront.c
index 54cd916..91a68e9 100644
--- a/drivers/video/xen-fbfront.c
+++ b/drivers/video/xen-fbfront.c
@@ -440,7 +440,7 @@ static int __devinit xenfb_probe(struct xenbus_device *dev,
 	fb_info->fix.type = FB_TYPE_PACKED_PIXELS;
 	fb_info->fix.accel = FB_ACCEL_NONE;
 
-	fb_info->flags = FBINFO_FLAG_DEFAULT;
+	fb_info->flags = FBINFO_FLAG_DEFAULT | FBINFO_VIRTFB;
 
 	ret = fb_alloc_cmap(&fb_info->cmap, 256, 0);
 	if (ret < 0) {
-- 
1.6.2.5

[PATCH 3/3] fb-defio: Inhibit VM_IO flag to be set on vmalloc-ed framebuffers.

[Konrad Rzeszutek Wilk]

The framebuffers (screenbase) these drivers present are actually
vmalloc-ed pages. There is no need for them to have the VM_IO flag set.
---
 drivers/video/broadsheetfb.c |    2 +-
 drivers/video/hecubafb.c     |    2 +-
 drivers/video/metronomefb.c  |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/video/broadsheetfb.c b/drivers/video/broadsheetfb.c
index 509cb92..df9ccb9 100644
--- a/drivers/video/broadsheetfb.c
+++ b/drivers/video/broadsheetfb.c
@@ -470,7 +470,7 @@ static int __devinit broadsheetfb_probe(struct platform_device *dev)
 	par->read_reg = broadsheet_read_reg;
 	init_waitqueue_head(&par->waitq);
 
-	info->flags = FBINFO_FLAG_DEFAULT;
+	info->flags = FBINFO_FLAG_DEFAULT | FBINFO_VIRTFB;
 
 	info->fbdefio = &broadsheetfb_defio;
 	fb_deferred_io_init(info);
diff --git a/drivers/video/hecubafb.c b/drivers/video/hecubafb.c
index 0b4bffb..f9d77ad 100644
--- a/drivers/video/hecubafb.c
+++ b/drivers/video/hecubafb.c
@@ -253,7 +253,7 @@ static int __devinit hecubafb_probe(struct platform_device *dev)
 	par->send_command = apollo_send_command;
 	par->send_data = apollo_send_data;
 
-	info->flags = FBINFO_FLAG_DEFAULT;
+	info->flags = FBINFO_FLAG_DEFAULT | FBINFO_VIRTFB;
 
 	info->fbdefio = &hecubafb_defio;
 	fb_deferred_io_init(info);
diff --git a/drivers/video/metronomefb.c b/drivers/video/metronomefb.c
index df1f757..661bfd2 100644
--- a/drivers/video/metronomefb.c
+++ b/drivers/video/metronomefb.c
@@ -700,7 +700,7 @@ static int __devinit metronomefb_probe(struct platform_device *dev)
 	if (retval < 0)
 		goto err_free_irq;
 
-	info->flags = FBINFO_FLAG_DEFAULT;
+	info->flags = FBINFO_FLAG_DEFAULT | FBINFO_VIRTFB;
 
 	info->fbdefio = &metronomefb_defio;
 	fb_deferred_io_init(info);
-- 
1.6.2.5

Re: [PATCH 1/3] fb-defio: If FBINFO_VIRTFB is defined, do not set VM_IO flag.

[Jeremy Fitzhardinge]

On 12/03/09 07:31, Konrad Rzeszutek Wilk wrote:
> Most users (except sh_mobile_lcdcfb.c) get their framebuffer from
> vmalloc. Setting VM_IO is not necessary as the memory obtained
> from vmalloc is System RAM type and is not susceptible to PCI memory
> constraints.
>   

Looks good, but you forgot signoffs on these.  Do you want me to add them?

Thanks,
    J
> ---
>  drivers/video/fb_defio.c |    4 +++-
>  include/linux/fb.h       |    1 +
>  2 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/video/fb_defio.c b/drivers/video/fb_defio.c
> index c27ab1e..94414fc 100644
> --- a/drivers/video/fb_defio.c
> +++ b/drivers/video/fb_defio.c
> @@ -144,7 +144,9 @@ static const struct address_space_operations fb_deferred_io_aops = {
>  static int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
>  {
>  	vma->vm_ops = &fb_deferred_io_vm_ops;
> -	vma->vm_flags |= ( VM_IO | VM_RESERVED | VM_DONTEXPAND );
> +	vma->vm_flags |= ( VM_RESERVED | VM_DONTEXPAND );
> +	if (!(info->flags & FBINFO_VIRTFB))
> +		vma->vm_flags |= VM_IO;
>  	vma->vm_private_data = info;
>  	return 0;
>  }
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index de9c722..369767b 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -763,6 +763,7 @@ struct fb_tile_ops {
>  	 *  takes over; acceleration engine should be in a quiescent state */
>  
>  /* hints */
> +#define FBINFO_VIRTFB		0x0004 /* FB is System RAM, not device. */
>  #define FBINFO_PARTIAL_PAN_OK	0x0040 /* otw use pan only for double-buffering */
>  #define FBINFO_READS_FAST	0x0080 /* soft-copy faster than rendering */
>  
>   

Re: [PATCH 1/3] fb-defio: If FBINFO_VIRTFB is defined, do not set VM_IO flag.

[Konrad Rzeszutek Wilk]

On Thu, Dec 03, 2009 at 10:50:15AM -0800, Jeremy Fitzhardinge wrote:
> On 12/03/09 07:31, Konrad Rzeszutek Wilk wrote:
> > Most users (except sh_mobile_lcdcfb.c) get their framebuffer from
> > vmalloc. Setting VM_IO is not necessary as the memory obtained
> > from vmalloc is System RAM type and is not susceptible to PCI memory
> > constraints.
> >   
> 
> Looks good, but you forgot signoffs on these.  Do you want me to add them?

Duh!! Please do. Thank you for noticing it.

Re: [PATCH 1/3] fb-defio: If FBINFO_VIRTFB is defined, do not set VM_IO flag.

[Jeremy Fitzhardinge]

On 12/03/09 10:56, Konrad Rzeszutek Wilk wrote:
> On Thu, Dec 03, 2009 at 10:50:15AM -0800, Jeremy Fitzhardinge wrote:
>   
>> On 12/03/09 07:31, Konrad Rzeszutek Wilk wrote:
>>     
>>> Most users (except sh_mobile_lcdcfb.c) get their framebuffer from
>>> vmalloc. Setting VM_IO is not necessary as the memory obtained
>>> from vmalloc is System RAM type and is not susceptible to PCI memory
>>> constraints.
>>>   
>>>       
>> Looks good, but you forgot signoffs on these.  Do you want me to add them?
>>     
> Duh!! Please do. Thank you for noticing it.
>   

Already done ;)  Will merge into xen/master shortly.  I'd like to queue
this up for this merge window if Jaya is OK with it, and also mark for
stable.  Though I think fbdev overall is currently considered
maintainerless...

    J

Re: [PATCH 1/3] fb-defio: If FBINFO_VIRTFB is defined, do not set VM_IO flag.

[Jaya Kumar]

On Thu, Dec 3, 2009 at 11:31 PM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> Most users (except sh_mobile_lcdcfb.c) get their framebuffer from
> vmalloc. Setting VM_IO is not necessary as the memory obtained
> from vmalloc is System RAM type and is not susceptible to PCI memory
> constraints.
> ---
>  drivers/video/fb_defio.c |    4 +++-
>  include/linux/fb.h       |    1 +
>  2 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/video/fb_defio.c b/drivers/video/fb_defio.c
> index c27ab1e..94414fc 100644
> --- a/drivers/video/fb_defio.c
> +++ b/drivers/video/fb_defio.c
> @@ -144,7 +144,9 @@ static const struct address_space_operations fb_deferred_io_aops = {
>  static int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
>  {
>        vma->vm_ops = &fb_deferred_io_vm_ops;
> -       vma->vm_flags |= ( VM_IO | VM_RESERVED | VM_DONTEXPAND );
> +       vma->vm_flags |= ( VM_RESERVED | VM_DONTEXPAND );
> +       if (!(info->flags & FBINFO_VIRTFB))
> +               vma->vm_flags |= VM_IO;
>        vma->vm_private_data = info;
>        return 0;
>  }
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index de9c722..369767b 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -763,6 +763,7 @@ struct fb_tile_ops {
>         *  takes over; acceleration engine should be in a quiescent state */
>
>  /* hints */
> +#define FBINFO_VIRTFB          0x0004 /* FB is System RAM, not device. */
>  #define FBINFO_PARTIAL_PAN_OK  0x0040 /* otw use pan only for double-buffering */
>  #define FBINFO_READS_FAST      0x0080 /* soft-copy faster than rendering */
>
> --
> 1.6.2.5
>
>

Looks good to me. Thanks.

Acked-by: Jaya Kumar <jayakumar.lkml@gmail.com>

Re: [PATCH 3/3] fb-defio: Inhibit VM_IO flag to be set on vmalloc-ed framebuffers.

[Jaya Kumar]

On Thu, Dec 3, 2009 at 11:31 PM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> The framebuffers (screenbase) these drivers present are actually
> vmalloc-ed pages. There is no need for them to have the VM_IO flag set.
> ---
>  drivers/video/broadsheetfb.c |    2 +-
>  drivers/video/hecubafb.c     |    2 +-
>  drivers/video/metronomefb.c  |    2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/video/broadsheetfb.c b/drivers/video/broadsheetfb.c
> index 509cb92..df9ccb9 100644
> --- a/drivers/video/broadsheetfb.c
> +++ b/drivers/video/broadsheetfb.c
> @@ -470,7 +470,7 @@ static int __devinit broadsheetfb_probe(struct platform_device *dev)
>        par->read_reg = broadsheet_read_reg;
>        init_waitqueue_head(&par->waitq);
>
> -       info->flags = FBINFO_FLAG_DEFAULT;
> +       info->flags = FBINFO_FLAG_DEFAULT | FBINFO_VIRTFB;
>
>        info->fbdefio = &broadsheetfb_defio;
>        fb_deferred_io_init(info);
> diff --git a/drivers/video/hecubafb.c b/drivers/video/hecubafb.c
> index 0b4bffb..f9d77ad 100644
> --- a/drivers/video/hecubafb.c
> +++ b/drivers/video/hecubafb.c
> @@ -253,7 +253,7 @@ static int __devinit hecubafb_probe(struct platform_device *dev)
>        par->send_command = apollo_send_command;
>        par->send_data = apollo_send_data;
>
> -       info->flags = FBINFO_FLAG_DEFAULT;
> +       info->flags = FBINFO_FLAG_DEFAULT | FBINFO_VIRTFB;
>
>        info->fbdefio = &hecubafb_defio;
>        fb_deferred_io_init(info);
> diff --git a/drivers/video/metronomefb.c b/drivers/video/metronomefb.c
> index df1f757..661bfd2 100644
> --- a/drivers/video/metronomefb.c
> +++ b/drivers/video/metronomefb.c
> @@ -700,7 +700,7 @@ static int __devinit metronomefb_probe(struct platform_device *dev)
>        if (retval < 0)
>                goto err_free_irq;
>
> -       info->flags = FBINFO_FLAG_DEFAULT;
> +       info->flags = FBINFO_FLAG_DEFAULT | FBINFO_VIRTFB;
>
>        info->fbdefio = &metronomefb_defio;
>        fb_deferred_io_init(info);
> --
> 1.6.2.5
>
>


Looks good to me. Thanks.

Acked-by: Jaya Kumar <jayakumar.lkml@gmail.com>

Re: [PATCH 3/3] fb-defio: Inhibit VM_IO flag to be set on vmalloc-ed framebuffers.

[Jeremy Fitzhardinge]

On 12/03/09 16:35, Jaya Kumar wrote:
> On Thu, Dec 3, 2009 at 11:31 PM, Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>  wrote:
>    
>> The framebuffers (screenbase) these drivers present are actually
>> vmalloc-ed pages. There is no need for them to have the VM_IO flag set.
>> ---
>>   drivers/video/broadsheetfb.c |    2 +-
>>   drivers/video/hecubafb.c     |    2 +-
>>   drivers/video/metronomefb.c  |    2 +-
>>   3 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/video/broadsheetfb.c b/drivers/video/broadsheetfb.c
>> index 509cb92..df9ccb9 100644
>> --- a/drivers/video/broadsheetfb.c
>> +++ b/drivers/video/broadsheetfb.c
>> @@ -470,7 +470,7 @@ static int __devinit broadsheetfb_probe(struct platform_device *dev)
>>         par->read_reg = broadsheet_read_reg;
>>         init_waitqueue_head(&par->waitq);
>>
>> -       info->flags = FBINFO_FLAG_DEFAULT;
>> +       info->flags = FBINFO_FLAG_DEFAULT | FBINFO_VIRTFB;
>>
>>         info->fbdefio =&broadsheetfb_defio;
>>         fb_deferred_io_init(info);
>> diff --git a/drivers/video/hecubafb.c b/drivers/video/hecubafb.c
>> index 0b4bffb..f9d77ad 100644
>> --- a/drivers/video/hecubafb.c
>> +++ b/drivers/video/hecubafb.c
>> @@ -253,7 +253,7 @@ static int __devinit hecubafb_probe(struct platform_device *dev)
>>         par->send_command = apollo_send_command;
>>         par->send_data = apollo_send_data;
>>
>> -       info->flags = FBINFO_FLAG_DEFAULT;
>> +       info->flags = FBINFO_FLAG_DEFAULT | FBINFO_VIRTFB;
>>
>>         info->fbdefio =&hecubafb_defio;
>>         fb_deferred_io_init(info);
>> diff --git a/drivers/video/metronomefb.c b/drivers/video/metronomefb.c
>> index df1f757..661bfd2 100644
>> --- a/drivers/video/metronomefb.c
>> +++ b/drivers/video/metronomefb.c
>> @@ -700,7 +700,7 @@ static int __devinit metronomefb_probe(struct platform_device *dev)
>>         if (retval<  0)
>>                 goto err_free_irq;
>>
>> -       info->flags = FBINFO_FLAG_DEFAULT;
>> +       info->flags = FBINFO_FLAG_DEFAULT | FBINFO_VIRTFB;
>>
>>         info->fbdefio =&metronomefb_defio;
>>         fb_deferred_io_init(info);
>> --
>> 1.6.2.5
>>
>>
>>      
>
> Looks good to me. Thanks.
>
> Acked-by: Jaya Kumar<jayakumar.lkml@gmail.com>
>    

Thanks.  Are you OK with me sending to Linus, or do you have a tree 
you'd prefer it to go by?

     J

Re: [PATCH 3/3] fb-defio: Inhibit VM_IO flag to be set on vmalloc-ed framebuffers.

[Jaya Kumar]

On Fri, Dec 4, 2009 at 8:52 AM, Jeremy Fitzhardinge <jeremy@goop.org> wrote:
>
> Thanks.  Are you OK with me sending to Linus, or do you have a tree you'd
> prefer it to go by?
>

Thanks Jeremy, yes, please feel free to send.

Re: [PATCH 1/3] fb-defio: If FBINFO_VIRTFB is defined, do not set VM_IO flag.

[Magnus Damm]

On Fri, Dec 4, 2009 at 12:31 AM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> Most users (except sh_mobile_lcdcfb.c) get their framebuffer from
> vmalloc. Setting VM_IO is not necessary as the memory obtained
> from vmalloc is System RAM type and is not susceptible to PCI memory
> constraints.
> ---
>  drivers/video/fb_defio.c |    4 +++-
>  include/linux/fb.h       |    1 +
>  2 files changed, 4 insertions(+), 1 deletions(-)

I just tested this on a Migo-R board which is using sh_mobile_lcdcfb.c
in deferred io mode. No problem.

Tested-by: Magnus Damm <damm@opensource.se>

/ magnus