[PATCH] Always use 64 bit addresses for the firmware memory map

[PATCH] Always use 64 bit addresses for the firmware memory map

[Bernhard Walle]

I had a problem that on i386 without PAE enabled the firmware memory map was
wrong because a 64 bit address has been truncated:

        0000000000000000-000000000009f400 (System RAM)
        000000000009f400-00000000000a0000 (reserved)
        00000000fec10000-00000000fec11000 (reserved)
        00000000fec20000-00000000fec21000 (reserved)
        00000000fee00000-00000000fee10000 (reserved)
        00000000ff800000-0000000100000000 (reserved)
 --->   0000000000000000-00000000fffff000 (System RAM)  <---
        00000000000f0000-0000000000100000 (reserved)
        0000000000100000-00000000f57fa000 (System RAM)
        00000000f57fa000-00000000f5800000 (ACPI Tables)
        00000000fdc00000-00000000fdc01000 (reserved)
        00000000fdc10000-00000000fdc11000 (reserved)
        00000000fdc20000-00000000fdc21000 (reserved)
        00000000fdc30000-00000000fdc31000 (reserved)
        00000000fec00000-00000000fec01000 (reserved)

Just always using 64 bit is the most sane approach in my opinion.


Signed-off-by: Bernhard Walle <bwalle@suse.de>
---
 drivers/firmware/memmap.c    |   17 +++++++----------
 include/linux/firmware-map.h |   12 +++++-------
 2 files changed, 12 insertions(+), 17 deletions(-)

diff --git a/drivers/firmware/memmap.c b/drivers/firmware/memmap.c
index 3bf8ee1..010afde 100644
--- a/drivers/firmware/memmap.c
+++ b/drivers/firmware/memmap.c
@@ -31,8 +31,8 @@
  * information is necessary as for the resource tree.
  */
 struct firmware_map_entry {
-	resource_size_t		start;	/* start of the memory range */
-	resource_size_t		end;	/* end of the memory range (incl.) */
+	uint64_t		start;	/* start of the memory range */
+	uint64_t		end;	/* end of the memory range (incl.) */
 	const char		*type;	/* type of the memory range */
 	struct list_head	list;	/* entry for the linked list */
 	struct kobject		kobj;   /* kobject for each entry */
@@ -101,7 +101,7 @@ static LIST_HEAD(map_entries);
  * Common implementation of firmware_map_add() and firmware_map_add_early()
  * which expects a pre-allocated struct firmware_map_entry.
  **/
-static int firmware_map_add_entry(resource_size_t start, resource_size_t end,
+static int firmware_map_add_entry(uint64_t start, uint64_t end,
 				  const char *type,
 				  struct firmware_map_entry *entry)
 {
@@ -132,8 +132,7 @@ static int firmware_map_add_entry(resource_size_t start, resource_size_t end,
  *
  * Returns 0 on success, or -ENOMEM if no memory could be allocated.
  **/
-int firmware_map_add(resource_size_t start, resource_size_t end,
-		     const char *type)
+int firmware_map_add(uint64_t start, uint64_t end, const char *type)
 {
 	struct firmware_map_entry *entry;
 
@@ -157,7 +156,7 @@ int firmware_map_add(resource_size_t start, resource_size_t end,
  *
  * Returns 0 on success, or -ENOMEM if no memory could be allocated.
  **/
-int __init firmware_map_add_early(resource_size_t start, resource_size_t end,
+int __init firmware_map_add_early(uint64_t start, uint64_t end,
 				  const char *type)
 {
 	struct firmware_map_entry *entry;
@@ -175,14 +174,12 @@ int __init firmware_map_add_early(resource_size_t start, resource_size_t end,
 
 static ssize_t start_show(struct firmware_map_entry *entry, char *buf)
 {
-	return snprintf(buf, PAGE_SIZE, "0x%llx\n",
-		(unsigned long long)entry->start);
+	return snprintf(buf, PAGE_SIZE, "0x%llx\n", entry->start);
 }
 
 static ssize_t end_show(struct firmware_map_entry *entry, char *buf)
 {
-	return snprintf(buf, PAGE_SIZE, "0x%llx\n",
-		(unsigned long long)entry->end);
+	return snprintf(buf, PAGE_SIZE, "0x%llx\n", entry->end);
 }
 
 static ssize_t type_show(struct firmware_map_entry *entry, char *buf)
diff --git a/include/linux/firmware-map.h b/include/linux/firmware-map.h
index 6e199c8..209a27a 100644
--- a/include/linux/firmware-map.h
+++ b/include/linux/firmware-map.h
@@ -24,21 +24,19 @@
  */
 #ifdef CONFIG_FIRMWARE_MEMMAP
 
-int firmware_map_add(resource_size_t start, resource_size_t end,
-		     const char *type);
-int firmware_map_add_early(resource_size_t start, resource_size_t end,
-			   const char *type);
+int firmware_map_add(uint64_t start, uint64_t end, const char *type);
+int firmware_map_add_early(uint64_t start, uint64_t end, const char *type);
 
 #else /* CONFIG_FIRMWARE_MEMMAP */
 
-static inline int firmware_map_add(resource_size_t start, resource_size_t end,
+static inline int firmware_map_add(uint64_t start, uint64_t end,
 				   const char *type)
 {
 	return 0;
 }
 
-static inline int firmware_map_add_early(resource_size_t start,
-					 resource_size_t end, const char *type)
+static inline int firmware_map_add_early(uint64_t start, uint64_t end,
+					 const char *type)
 {
 	return 0;
 }
-- 
1.6.0.2

Re: [PATCH] Always use 64 bit addresses for the firmware memory map

[H. Peter Anvin]

Bernhard Walle wrote:
> I had a problem that on i386 without PAE enabled the firmware memory map was
> wrong because a 64 bit address has been truncated:
> 
>         0000000000000000-000000000009f400 (System RAM)
>         000000000009f400-00000000000a0000 (reserved)
>         00000000fec10000-00000000fec11000 (reserved)
>         00000000fec20000-00000000fec21000 (reserved)
>         00000000fee00000-00000000fee10000 (reserved)
>         00000000ff800000-0000000100000000 (reserved)
>  --->   0000000000000000-00000000fffff000 (System RAM)  <---
>         00000000000f0000-0000000000100000 (reserved)
>         0000000000100000-00000000f57fa000 (System RAM)
>         00000000f57fa000-00000000f5800000 (ACPI Tables)
>         00000000fdc00000-00000000fdc01000 (reserved)
>         00000000fdc10000-00000000fdc11000 (reserved)
>         00000000fdc20000-00000000fdc21000 (reserved)
>         00000000fdc30000-00000000fdc31000 (reserved)
>         00000000fec00000-00000000fec01000 (reserved)
> 
> Just always using 64 bit is the most sane approach in my opinion.
> 
> 
> Signed-off-by: Bernhard Walle <bwalle@suse.de>

There are two options: either filter addresses outside the
resource_size_t range (since we don't manage that space and therefore
don't care about it) or, as you do, enforce 64-bitness.

I want to make sure, though, that we don't just end up pushing the
truncation further down in the code.

	-hpa

Re: [PATCH] Always use 64 bit addresses for the firmware memory map

[Jeremy Fitzhardinge]

Bernhard Walle wrote:
> I had a problem that on i386 without PAE enabled the firmware memory map was
> wrong because a 64 bit address has been truncated:
>
>         0000000000000000-000000000009f400 (System RAM)
>         000000000009f400-00000000000a0000 (reserved)
>         00000000fec10000-00000000fec11000 (reserved)
>         00000000fec20000-00000000fec21000 (reserved)
>         00000000fee00000-00000000fee10000 (reserved)
>         00000000ff800000-0000000100000000 (reserved)
>  --->   0000000000000000-00000000fffff000 (System RAM)  <---
>         00000000000f0000-0000000000100000 (reserved)
>         0000000000100000-00000000f57fa000 (System RAM)
>         00000000f57fa000-00000000f5800000 (ACPI Tables)
>         00000000fdc00000-00000000fdc01000 (reserved)
>         00000000fdc10000-00000000fdc11000 (reserved)
>         00000000fdc20000-00000000fdc21000 (reserved)
>         00000000fdc30000-00000000fdc31000 (reserved)
>         00000000fec00000-00000000fec01000 (reserved)
>
> Just always using 64 bit is the most sane approach in my opinion.
>
>
> Signed-off-by: Bernhard Walle <bwalle@suse.de>
> ---
>  drivers/firmware/memmap.c    |   17 +++++++----------
>  include/linux/firmware-map.h |   12 +++++-------
>  2 files changed, 12 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/firmware/memmap.c b/drivers/firmware/memmap.c
> index 3bf8ee1..010afde 100644
> --- a/drivers/firmware/memmap.c
> +++ b/drivers/firmware/memmap.c
> @@ -31,8 +31,8 @@
>   * information is necessary as for the resource tree.
>   */
>  struct firmware_map_entry {
> -	resource_size_t		start;	/* start of the memory range */
> -	resource_size_t		end;	/* end of the memory range (incl.) */
>   

resource_size_t should always be 64-bit on PAE now.

    J

> +	uint64_t		start;	/* start of the memory range */
> +	uint64_t		end;	/* end of the memory range (incl.) */
>  	const char		*type;	/* type of the memory range */
>  	struct list_head	list;	/* entry for the linked list */
>  	struct kobject		kobj;   /* kobject for each entry */
> @@ -101,7 +101,7 @@ static LIST_HEAD(map_entries);
>   * Common implementation of firmware_map_add() and firmware_map_add_early()
>   * which expects a pre-allocated struct firmware_map_entry.
>   **/
> -static int firmware_map_add_entry(resource_size_t start, resource_size_t end,
> +static int firmware_map_add_entry(uint64_t start, uint64_t end,
>  				  const char *type,
>  				  struct firmware_map_entry *entry)
>  {
> @@ -132,8 +132,7 @@ static int firmware_map_add_entry(resource_size_t start, resource_size_t end,
>   *
>   * Returns 0 on success, or -ENOMEM if no memory could be allocated.
>   **/
> -int firmware_map_add(resource_size_t start, resource_size_t end,
> -		     const char *type)
> +int firmware_map_add(uint64_t start, uint64_t end, const char *type)
>  {
>  	struct firmware_map_entry *entry;
>  
> @@ -157,7 +156,7 @@ int firmware_map_add(resource_size_t start, resource_size_t end,
>   *
>   * Returns 0 on success, or -ENOMEM if no memory could be allocated.
>   **/
> -int __init firmware_map_add_early(resource_size_t start, resource_size_t end,
> +int __init firmware_map_add_early(uint64_t start, uint64_t end,
>  				  const char *type)
>  {
>  	struct firmware_map_entry *entry;
> @@ -175,14 +174,12 @@ int __init firmware_map_add_early(resource_size_t start, resource_size_t end,
>  
>  static ssize_t start_show(struct firmware_map_entry *entry, char *buf)
>  {
> -	return snprintf(buf, PAGE_SIZE, "0x%llx\n",
> -		(unsigned long long)entry->start);
> +	return snprintf(buf, PAGE_SIZE, "0x%llx\n", entry->start);
>  }
>  
>  static ssize_t end_show(struct firmware_map_entry *entry, char *buf)
>  {
> -	return snprintf(buf, PAGE_SIZE, "0x%llx\n",
> -		(unsigned long long)entry->end);
> +	return snprintf(buf, PAGE_SIZE, "0x%llx\n", entry->end);
>  }
>  
>  static ssize_t type_show(struct firmware_map_entry *entry, char *buf)
> diff --git a/include/linux/firmware-map.h b/include/linux/firmware-map.h
> index 6e199c8..209a27a 100644
> --- a/include/linux/firmware-map.h
> +++ b/include/linux/firmware-map.h
> @@ -24,21 +24,19 @@
>   */
>  #ifdef CONFIG_FIRMWARE_MEMMAP
>  
> -int firmware_map_add(resource_size_t start, resource_size_t end,
> -		     const char *type);
> -int firmware_map_add_early(resource_size_t start, resource_size_t end,
> -			   const char *type);
> +int firmware_map_add(uint64_t start, uint64_t end, const char *type);
> +int firmware_map_add_early(uint64_t start, uint64_t end, const char *type);
>  
>  #else /* CONFIG_FIRMWARE_MEMMAP */
>  
> -static inline int firmware_map_add(resource_size_t start, resource_size_t end,
> +static inline int firmware_map_add(uint64_t start, uint64_t end,
>  				   const char *type)
>  {
>  	return 0;
>  }
>  
> -static inline int firmware_map_add_early(resource_size_t start,
> -					 resource_size_t end, const char *type)
> +static inline int firmware_map_add_early(uint64_t start, uint64_t end,
> +					 const char *type)
>  {
>  	return 0;
>  }
>   

Re: [PATCH] Always use 64 bit addresses for the firmware memory map

[Bernhard Walle]

* H. Peter Anvin [2008-11-12 11:59]:
> 
> I want to make sure, though, that we don't just end up pushing the
> truncation further down in the code.

Well, I think that interface should export the BIOS memmap as provided.
Since E820 does provide 64 bit addresses, that should get exported.

It should even possible to kexec a PAE kernel from a non PAE kernel ...
I didn't test, but it could work. But only if the E820 map is correctly
written in the zero page, which is only the case if we get it correctly.


Regards,
Bernhard
-- 
Bernhard Walle, SUSE LINUX Products GmbH, Architecture Development

Re: [PATCH] Always use 64 bit addresses for the firmware memory map

[H. Peter Anvin]

Jeremy Fitzhardinge wrote:
> 
> resource_size_t should always be 64-bit on PAE now.
> 

Yes, but this would affect non-PAE kernels too.

	-hpa

Re: [PATCH] Always use 64 bit addresses for the firmware memory map

[H. Peter Anvin]

Bernhard Walle wrote:
> * H. Peter Anvin [2008-11-12 11:59]:
>> I want to make sure, though, that we don't just end up pushing the
>> truncation further down in the code.
> 
> Well, I think that interface should export the BIOS memmap as provided.
> Since E820 does provide 64 bit addresses, that should get exported.
> 
> It should even possible to kexec a PAE kernel from a non PAE kernel ...
> I didn't test, but it could work. But only if the E820 map is correctly
> written in the zero page, which is only the case if we get it correctly.
> 

That's fine, but we do have to check that we don't truncate elsewhere.

	-hpa

Re: [PATCH] Always use 64 bit addresses for the firmware memory map

[Jeremy Fitzhardinge]

H. Peter Anvin wrote:
> Jeremy Fitzhardinge wrote:
>   
>> resource_size_t should always be 64-bit on PAE now.
>>
>>     
>
> Yes, but this would affect non-PAE kernels too.
>   

Yeah, I overlooked the comment about non-PAE.  Should we just make 
resource_size_t 64 bits all the time?  Or ignore inaccessible resources?

    J

Re: [PATCH] Always use 64 bit addresses for the firmware memory map

[H. Peter Anvin]

Jeremy Fitzhardinge wrote:
> H. Peter Anvin wrote:
>> Jeremy Fitzhardinge wrote:
>>  
>>> resource_size_t should always be 64-bit on PAE now.
>>>
>>>     
>>
>> Yes, but this would affect non-PAE kernels too.
>>   
> 
> Yeah, I overlooked the comment about non-PAE.  Should we just make
> resource_size_t 64 bits all the time?  Or ignore inaccessible resources?
> 

Ignore them, presumably (keeping in mind some of them may need to be
truncated.)  We simply don't manage that space.

	-hpa

Re: [PATCH] Always use 64 bit addresses for the firmware memory map

[Bernhard Walle]

* H. Peter Anvin [2008-11-12 15:35]:
>
> Bernhard Walle wrote:
> > * H. Peter Anvin [2008-11-12 11:59]:
> >> I want to make sure, though, that we don't just end up pushing the
> >> truncation further down in the code.
> > 
> > Well, I think that interface should export the BIOS memmap as provided.
> > Since E820 does provide 64 bit addresses, that should get exported.
> > 
> > It should even possible to kexec a PAE kernel from a non PAE kernel ...
> > I didn't test, but it could work. But only if the E820 map is correctly
> > written in the zero page, which is only the case if we get it correctly.
> 
> That's fine, but we do have to check that we don't truncate elsewhere.

What do you mean? That my patch doesn't fix all problems that might
exist but are not yet fixed or that my patch introduces new problems?

Well, for example in the resource reservation code [e820.c,
e820_reserve_resource()] that is handled at line 1285:

		if (end != (resource_size_t)end) {
			res++;
			continue;
		}

My patch only changes the firmware interface, not the architecture
specific code. However, I cannot guarantee that it doesn't break
something, but what action do you expect from me that the patch is
taken?


Regards,
Bernhard