userspace firmware loader, vmap, and nommu

userspace firmware loader, vmap, and nommu

[Mike Frysinger]

semi-recently (9 Apr 2009), the userspace firmware code was rewritten
to use vmap().  this causes problems for nommu systems as it isnt
possible to create a virtually contiguous map with physically
discontiguous pages.  the firmware loader used to work before this
change because it would handle the realloc steps itself (allocate
larger contiguous memory, copy over older data, release older memory)
and vmalloc() on nommu is simply kmalloc().

this could be handled transparently on nommu systems by moving this
scatter gathering of pages into vmap:
void *vmap(struct page **pages, unsigned int count, unsigned long
flags, pgprot_t prot)
{
    unsigned int i;
    void *new_map, *page_data;

    new_map = kmalloc(count << PAGE_SHIFT, GFP_KERNEL);
    if (!new_map)
        return NULL;

    for (i = 0; i < count; ++i) {
        page_data = kmap(pages[i]);
        memcpy(new_map + (i << PAGE_SHIFT), page_data, PAGE_SIZE);
        kunmap(page_data);
    }

    return new_map;
}
void vunmap(const void *addr)
{
    kfree(addr);
}

or we could add nommu-specific code to the firmware loader to not use
vmap().  how would you like to go David (Howells) ?

there is a possibility for the semi-common case of vmap-ing only one
page.  but i'm not familiar enough with the mm code to figure that
case out.
-mike

Re: userspace firmware loader, vmap, and nommu

[David Woodhouse]

On Tue, 2009-10-06 at 00:25 -0400, Mike Frysinger wrote:
> semi-recently (9 Apr 2009), the userspace firmware code was rewritten
> to use vmap().  this causes problems for nommu systems as it isnt
> possible to create a virtually contiguous map with physically
> discontiguous pages. 

Oops, sorry.

>  the firmware loader used to work before this
> change because it would handle the realloc steps itself (allocate
> larger contiguous memory, copy over older data, release older memory)
> and vmalloc() on nommu is simply kmalloc().
> 
> this could be handled transparently on nommu systems by moving this
> scatter gathering of pages into vmap:
> void *vmap(struct page **pages, unsigned int count, unsigned long
> flags, pgprot_t prot)
> {
>     unsigned int i;
>     void *new_map, *page_data;
> 
>     new_map = kmalloc(count << PAGE_SHIFT, GFP_KERNEL);
>     if (!new_map)
>         return NULL;
> 
>     for (i = 0; i < count; ++i) {
>         page_data = kmap(pages[i]);
>         memcpy(new_map + (i << PAGE_SHIFT), page_data, PAGE_SIZE);
>         kunmap(page_data);
>     }
> 
>     return new_map;

I wouldn't necessarily want to do that for _all_ vmap() calls, but doing
it just for the firmware loader might make some sense. It does mean you
have to have _twice_ as much memory available as the size of the
firmware in question. And you have to have a contiguous chunk even
_after_ allocating it once piecemeal.

> void vunmap(const void *addr)
> {
>     kfree(addr);
> }
> 
> or we could add nommu-specific code to the firmware loader to not use
> vmap().  how would you like to go David (Howells) ?

Or we could add _generic_ code not to use vmap(). Just teach the users
that you don't get a virtually contiguous blob back from
request_firmware(); you get an array of pages instead.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: userspace firmware loader, vmap, and nommu

[Mike Frysinger]

On Tue, Oct 6, 2009 at 00:38, David Woodhouse wrote:
> On Tue, 2009-10-06 at 00:25 -0400, Mike Frysinger wrote:
>>  the firmware loader used to work before this
>> change because it would handle the realloc steps itself (allocate
>> larger contiguous memory, copy over older data, release older memory)
>> and vmalloc() on nommu is simply kmalloc().
>>
>> this could be handled transparently on nommu systems by moving this
>> scatter gathering of pages into vmap:
>> void *vmap(struct page **pages, unsigned int count, unsigned long
>> flags, pgprot_t prot)
>> {
>>     unsigned int i;
>>     void *new_map, *page_data;
>>
>>     new_map = kmalloc(count << PAGE_SHIFT, GFP_KERNEL);
>>     if (!new_map)
>>         return NULL;
>>
>>     for (i = 0; i < count; ++i) {
>>         page_data = kmap(pages[i]);
>>         memcpy(new_map + (i << PAGE_SHIFT), page_data, PAGE_SIZE);
>>         kunmap(page_data);
>>     }
>>
>>     return new_map;
>
> I wouldn't necessarily want to do that for _all_ vmap() calls,

there arent any vmap() callers currently on nommu systems since the
functions currently BUG().  looking at lxr for 2.6.31 indicates that
there are very few relevant vmap() callers in general.

> but doing
> it just for the firmware loader might make some sense. It does mean you
> have to have _twice_ as much memory available as the size of the
> firmware in question. And you have to have a contiguous chunk even
> _after_ allocating it once piecemeal.

yes, but this is how it worked before and no one complained ;).
firmware files after all tend to be on the "small" side, so getting a
small physically contiguous mapping isnt that hard.

>> void vunmap(const void *addr)
>> {
>>     kfree(addr);
>> }
>>
>> or we could add nommu-specific code to the firmware loader to not use
>> vmap().  how would you like to go David (Howells) ?
>
> Or we could add _generic_ code not to use vmap(). Just teach the users
> that you don't get a virtually contiguous blob back from
> request_firmware(); you get an array of pages instead.

wouldnt that non-trivially increase the code work for callers of the
firmware functions ?  seems like a hefty penalty for a minority
(nommu) to impose on the majority (mmu).
-mike

[PATCH] nommu: implement vmap/vunmap with kmalloc

[Mike Frysinger]

git-svn-id: svn://localhost/svn/linux-kernel/trunk@7693 526b6c2d-f592-4532-a319-5dd88ccb003d
---
 mm/nommu.c |   18 +++++++++++++++---
 1 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/mm/nommu.c b/mm/nommu.c
index 8687973..d28ab94 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -360,14 +360,26 @@ EXPORT_SYMBOL(vmalloc_32_user);
 
 void *vmap(struct page **pages, unsigned int count, unsigned long flags, pgprot_t prot)
 {
-	BUG();
-	return NULL;
+	unsigned int i;
+	void *new_map, *page_data;
+
+	new_map = kmalloc(count << PAGE_SHIFT, GFP_KERNEL);
+	if (!new_map)
+		return NULL;
+
+	for (i = 0; i < count; ++i) {
+		page_data = kmap(pages[i]);
+		memcpy(new_map + (i << PAGE_SHIFT), page_data, PAGE_SIZE);
+		kunmap(page_data);
+	}
+
+	return new_map;
 }
 EXPORT_SYMBOL(vmap);
 
 void vunmap(const void *addr)
 {
-	BUG();
+	kfree(addr);
 }
 EXPORT_SYMBOL(vunmap);
 
-- 
1.6.5.4

Re: [uClinux-dev] [PATCH] nommu: implement vmap/vunmap with kmalloc

[Mike Frysinger]

On Mon, Dec 21, 2009 at 11:44, Mike Frysinger wrote:
> git-svn-id: svn://localhost/svn/linux-kernel/trunk@7693 526b6c2d-f592-4532-a319-5dd88ccb003d

blah, didnt realize i still had this crap.  drop the changelog and add:
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
-mike

Re: [PATCH] nommu: implement vmap/vunmap with kmalloc

[David Howells]

Seems a reasonable idea.

It could be done more efficiently if vunmap() was given the page count given
to vmap().

David

[PATCH v2] NOMMU: implement vmap/vunmap with kmalloc

[Mike Frysinger]

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
v2
	- fix up changelog

 mm/nommu.c |   18 +++++++++++++++---
 1 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/mm/nommu.c b/mm/nommu.c
index b9b5cce..61a68d5 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -360,14 +360,26 @@ EXPORT_SYMBOL(vmalloc_32_user);
 
 void *vmap(struct page **pages, unsigned int count, unsigned long flags, pgprot_t prot)
 {
-	BUG();
-	return NULL;
+	unsigned int i;
+	void *new_map, *page_data;
+
+	new_map = kmalloc(count << PAGE_SHIFT, GFP_KERNEL);
+	if (!new_map)
+		return NULL;
+
+	for (i = 0; i < count; ++i) {
+		page_data = kmap(pages[i]);
+		memcpy(new_map + (i << PAGE_SHIFT), page_data, PAGE_SIZE);
+		kunmap(page_data);
+	}
+
+	return new_map;
 }
 EXPORT_SYMBOL(vmap);
 
 void vunmap(const void *addr)
 {
-	BUG();
+	kfree(addr);
 }
 EXPORT_SYMBOL(vunmap);
 
-- 
1.7.0.2

Re: [PATCH v2] NOMMU: implement vmap/vunmap with kmalloc

[David Howells]

How about the attached patch instead?  I'd rather not make vmap() generally
available in NOMMU mode since it can't be implemented in NOMMU mode.  Yes,
vmap() can take a copy of the pages it is given, but you can't guarantee
that's the right thing to do.  It's like a shared-writable mmap.

Instead, why not just override vmap() in firmware_class.c for the one instance
where we know we're happy with this behaviour?

David
---
From: David Howells <dhowells@redhat.com>
Subject: [PATCH] NOMMU: Work around the lack of vmap()/vunmap() in firmware_loading_store()

Work around the lack of vmap()/vunmap() in firmware_loading_store() when
operating in NOMMU mode.  vmap() cannot be implemented as there's no virtual
mapping available.

Instead, in NOMMU mode, coalesce the data into one big buffer and store as the
address vmap() would've returned.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 drivers/base/firmware_class.c |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)


diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 18518ba..e33c2cb 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -59,6 +59,33 @@ static struct builtin_fw *__start_builtin_fw;
 static struct builtin_fw *__end_builtin_fw;
 #endif
 
+/*
+ * NOMMU mode can't provide vmap() as there's no MMU to do the virtual mapping.
+ * Coalesce the data into a big buffer instead.
+ */
+#ifndef CONFIG_MMU
+static void *__pretend_vmap(struct page **pages, unsigned int count,
+			    unsigned long flags, pgprot_t prot)
+{
+	unsigned int i;
+	void *new_map, *page_data;
+
+	new_map = kmalloc(count << PAGE_SHIFT, GFP_KERNEL);
+	if (!new_map)
+		return NULL;
+
+	for (i = 0; i < count; ++i) {
+		page_data = kmap(pages[i]);
+		memcpy(new_map + (i << PAGE_SHIFT), page_data, PAGE_SIZE);
+		kunmap(page_data);
+	}
+
+	return new_map;
+}
+
+#define vmap(pg, c, f, pr)	__pretend_vmap(pg, c, f, pr)
+#endif /* !CONFIG_MMU */
+
 static void
 fw_load_abort(struct firmware_priv *fw_priv)
 {

Re: [PATCH v2] NOMMU: implement vmap/vunmap with kmalloc

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 667 bytes --]

On Wednesday 31 March 2010 13:11:24 David Howells wrote:
> How about the attached patch instead?  I'd rather not make vmap() generally
> available in NOMMU mode since it can't be implemented in NOMMU mode.  Yes,
> vmap() can take a copy of the pages it is given, but you can't guarantee
> that's the right thing to do.  It's like a shared-writable mmap.
> 
> Instead, why not just override vmap() in firmware_class.c for the one
> instance where we know we're happy with this behaviour?

how about putting this implementation into like vmap_nommu() and only 
rewriting vmap() to vmap_nommu() when we know it's safe ?  such as this 
firmware case ?
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2] NOMMU: implement vmap/vunmap with kmalloc

[David Howells]

Mike Frysinger <vapier@gentoo.org> wrote:

> how about putting this implementation into like vmap_nommu() and only 
> rewriting vmap() to vmap_nommu() when we know it's safe ?  such as this 
> firmware case ?

Well, I'd argue it's _not_ vmap(), so it doesn't really make sense to call it
such.  Perhaps some better name?  Perhaps vcopy()?

David

Re: [PATCH v2] NOMMU: implement vmap/vunmap with kmalloc

[David Howells]

David Howells <dhowells@redhat.com> wrote:

> > how about putting this implementation into like vmap_nommu() and only 
> > rewriting vmap() to vmap_nommu() when we know it's safe ?  such as this 
> > firmware case ?
> 
> Well, I'd argue it's _not_ vmap(), so it doesn't really make sense to call it
> such.  Perhaps some better name?  Perhaps vcopy()?

How about vcoalesce()?

David

Re: [PATCH v2] NOMMU: implement vmap/vunmap with kmalloc

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 499 bytes --]

On Wednesday 31 March 2010 19:07:54 David Howells wrote:
> David Howells wrote:
> > > how about putting this implementation into like vmap_nommu() and only
> > > rewriting vmap() to vmap_nommu() when we know it's safe ?  such as this
> > > firmware case ?
> > 
> > Well, I'd argue it's _not_ vmap(), so it doesn't really make sense to
> > call it such.  Perhaps some better name?  Perhaps vcopy()?

fair point

> How about vcoalesce()?

WFM, as does something like vmerge() ...
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]