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
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
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
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
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 */
>
>
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.
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
On Thu, Dec 3, 2009 at 11:31 PM, Konrad Rzeszutek Wilk
<[email protected]> 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 <[email protected]>
On Thu, Dec 3, 2009 at 11:31 PM, Konrad Rzeszutek Wilk
<[email protected]> 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 <[email protected]>
On 12/03/09 16:35, Jaya Kumar wrote:
> On Thu, Dec 3, 2009 at 11:31 PM, Konrad Rzeszutek Wilk
> <[email protected]> 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<[email protected]>
>
Thanks. Are you OK with me sending to Linus, or do you have a tree
you'd prefer it to go by?
J
On Fri, Dec 4, 2009 at 8:52 AM, Jeremy Fitzhardinge <[email protected]> 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.
On Fri, Dec 4, 2009 at 12:31 AM, Konrad Rzeszutek Wilk
<[email protected]> 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 <[email protected]>
/ magnus