1. Patchew
  2. Xen
  3. View series

[PATCH] x86/boot: Force error checking for reserve_e820_ram()

Andrew Cooper posted 1 patch 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20260417160828.526063-1-andrew.cooper3@citrix.com
xen/arch/x86/e820.c             |  8 +++++++-
xen/arch/x86/include/asm/e820.h |  2 +-
xen/arch/x86/setup.c            | 11 +++++++----
3 files changed, 15 insertions(+), 6 deletions(-)
[PATCH] x86/boot: Force error checking for reserve_e820_ram()
Posted by Andrew Cooper 2 weeks ago 
Failing to mark Xen as Reserved in the E820 is catastrophic; RAM regions get
handed to the physical memory allocator for general use.  Similarly, failure
to mark the boot modules as reserved is not going to result in a working
system.

Mark reserve_e820_ram() as __must_check, and panic() on failure.  To avoid
opencoding the range in every caller, print a general failure message in
reserve_e820_ram().

Reported-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <jbeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Teddy Astie <teddy.astie@vates.tech>

Slightly RFC; only compile tested so far.

There's no obvious fixes tag.  This has been many variations of broken since
forever.
---
 xen/arch/x86/e820.c             |  8 +++++++-
 xen/arch/x86/include/asm/e820.h |  2 +-
 xen/arch/x86/setup.c            | 11 +++++++----
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
index 872208ab3722..f09a01f0c50a 100644
--- a/xen/arch/x86/e820.c
+++ b/xen/arch/x86/e820.c
@@ -670,7 +670,13 @@ int __init e820_change_range_type(
 /* Set E820_RAM area (@s,@e) as RESERVED in specified e820 map. */
 int __init reserve_e820_ram(struct e820map *map, uint64_t s, uint64_t e)
 {
-    return e820_change_range_type(map, s, e, E820_RAM, E820_RESERVED);
+    int res = e820_change_range_type(map, s, e, E820_RAM, E820_RESERVED);
+
+    if ( !res )
+        printk("Failed to convert E820 RAM %"PRIx64"-%"PRIx64" to RESERVED\n",
+               s, e);
+
+    return res;
 }
 
 unsigned long __init init_e820(const char *str, struct e820map *raw)
diff --git a/xen/arch/x86/include/asm/e820.h b/xen/arch/x86/include/asm/e820.h
index 8e7644f8870b..a86d60ce3e77 100644
--- a/xen/arch/x86/include/asm/e820.h
+++ b/xen/arch/x86/include/asm/e820.h
@@ -25,7 +25,7 @@ struct e820map {
 
 extern int sanitize_e820_map(struct e820entry *biosmap, unsigned int *pnr_map);
 extern int e820_all_mapped(u64 start, u64 end, unsigned type);
-extern int reserve_e820_ram(struct e820map *map, uint64_t s, uint64_t e);
+extern int __must_check reserve_e820_ram(struct e820map *map, uint64_t s, uint64_t e);
 extern int e820_change_range_type(
     struct e820map *map, uint64_t s, uint64_t e,
     uint32_t orig_type, uint32_t new_type);
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index d041cbd5f6f1..9c1f1eafa0d7 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1639,7 +1639,8 @@ void asmlinkage __init noreturn __start_xen(void)
     {
         uint64_t s = bi->mods[i].start, l = bi->mods[i].size;
 
-        reserve_e820_ram(&boot_e820, s, s + PAGE_ALIGN(l));
+        if ( !reserve_e820_ram(&boot_e820, s, s + PAGE_ALIGN(l)) )
+            panic("Failed to reserve boot module %u in E820\n", i);
     }
 
     if ( !xen_phys_start )
@@ -1652,11 +1653,13 @@ void asmlinkage __init noreturn __start_xen(void)
     /* This needs to remain in sync with remove_xen_ranges(). */
     if ( efi_boot_mem_unused(&eb_start, &eb_end) )
     {
-        reserve_e820_ram(&boot_e820, __pa(_stext), __pa(eb_start));
-        reserve_e820_ram(&boot_e820, __pa(eb_end), __pa(__2M_rwdata_end));
+        if ( !reserve_e820_ram(&boot_e820, __pa(_stext), __pa(eb_start)) ||
+             !reserve_e820_ram(&boot_e820, __pa(eb_end), __pa(__2M_rwdata_end)) )
+            panic("Failed to reserve Xen in E820\n");
     }
     else
-        reserve_e820_ram(&boot_e820, __pa(_stext), __pa(__2M_rwdata_end));
+        if ( reserve_e820_ram(&boot_e820, __pa(_stext), __pa(__2M_rwdata_end)) )
+            panic("Failed to reserve Xen in E820\n");
 
     /* Late kexec reservation (dynamic start address). */
     kexec_reserve_area();

base-commit: 986707b461eb56d75f55581dd1c8e2633f814795
-- 
2.39.5
Re: [PATCH] x86/boot: Force error checking for reserve_e820_ram()
Posted by Jan Beulich 1 week, 4 days ago 
On 17.04.2026 18:08, Andrew Cooper wrote:
> --- a/xen/arch/x86/include/asm/e820.h
> +++ b/xen/arch/x86/include/asm/e820.h
> @@ -25,7 +25,7 @@ struct e820map {
>  
>  extern int sanitize_e820_map(struct e820entry *biosmap, unsigned int *pnr_map);
>  extern int e820_all_mapped(u64 start, u64 end, unsigned type);
> -extern int reserve_e820_ram(struct e820map *map, uint64_t s, uint64_t e);
> +extern int __must_check reserve_e820_ram(struct e820map *map, uint64_t s, uint64_t e);

Nit: This line has grown too long now. With this and the adjustments Ross
has asked for:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan
Re: [PATCH] x86/boot: Force error checking for reserve_e820_ram()
Posted by Ross Lagerwall 2 weeks ago 
On 4/17/26 5:08 PM, Andrew Cooper wrote:
> Failing to mark Xen as Reserved in the E820 is catastrophic; RAM regions get
> handed to the physical memory allocator for general use.  Similarly, failure
> to mark the boot modules as reserved is not going to result in a working
> system.
> 
> Mark reserve_e820_ram() as __must_check, and panic() on failure.  To avoid
> opencoding the range in every caller, print a general failure message in
> reserve_e820_ram().
> 
> Reported-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Teddy Astie <teddy.astie@vates.tech>
> 
> Slightly RFC; only compile tested so far.
> 
> There's no obvious fixes tag.  This has been many variations of broken since
> forever.
> ---
>   xen/arch/x86/e820.c             |  8 +++++++-
>   xen/arch/x86/include/asm/e820.h |  2 +-
>   xen/arch/x86/setup.c            | 11 +++++++----
>   3 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
> index 872208ab3722..f09a01f0c50a 100644
> --- a/xen/arch/x86/e820.c
> +++ b/xen/arch/x86/e820.c
> @@ -670,7 +670,13 @@ int __init e820_change_range_type(
>   /* Set E820_RAM area (@s,@e) as RESERVED in specified e820 map. */
>   int __init reserve_e820_ram(struct e820map *map, uint64_t s, uint64_t e)
>   {
> -    return e820_change_range_type(map, s, e, E820_RAM, E820_RESERVED);
> +    int res = e820_change_range_type(map, s, e, E820_RAM, E820_RESERVED);
> +
> +    if ( !res )
> +        printk("Failed to convert E820 RAM %"PRIx64"-%"PRIx64" to RESERVED\n",
> +               s, e);
> +
> +    return res;
>   }
>   
>   unsigned long __init init_e820(const char *str, struct e820map *raw)
> diff --git a/xen/arch/x86/include/asm/e820.h b/xen/arch/x86/include/asm/e820.h
> index 8e7644f8870b..a86d60ce3e77 100644
> --- a/xen/arch/x86/include/asm/e820.h
> +++ b/xen/arch/x86/include/asm/e820.h
> @@ -25,7 +25,7 @@ struct e820map {
>   
>   extern int sanitize_e820_map(struct e820entry *biosmap, unsigned int *pnr_map);
>   extern int e820_all_mapped(u64 start, u64 end, unsigned type);
> -extern int reserve_e820_ram(struct e820map *map, uint64_t s, uint64_t e);
> +extern int __must_check reserve_e820_ram(struct e820map *map, uint64_t s, uint64_t e);
>   extern int e820_change_range_type(
>       struct e820map *map, uint64_t s, uint64_t e,
>       uint32_t orig_type, uint32_t new_type);
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index d041cbd5f6f1..9c1f1eafa0d7 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1639,7 +1639,8 @@ void asmlinkage __init noreturn __start_xen(void)
>       {
>           uint64_t s = bi->mods[i].start, l = bi->mods[i].size;
>   
> -        reserve_e820_ram(&boot_e820, s, s + PAGE_ALIGN(l));
> +        if ( !reserve_e820_ram(&boot_e820, s, s + PAGE_ALIGN(l)) )
> +            panic("Failed to reserve boot module %u in E820\n", i);
>       }
>   
>       if ( !xen_phys_start )
> @@ -1652,11 +1653,13 @@ void asmlinkage __init noreturn __start_xen(void)
>       /* This needs to remain in sync with remove_xen_ranges(). */
>       if ( efi_boot_mem_unused(&eb_start, &eb_end) )
>       {
> -        reserve_e820_ram(&boot_e820, __pa(_stext), __pa(eb_start));
> -        reserve_e820_ram(&boot_e820, __pa(eb_end), __pa(__2M_rwdata_end));
> +        if ( !reserve_e820_ram(&boot_e820, __pa(_stext), __pa(eb_start)) ||
> +             !reserve_e820_ram(&boot_e820, __pa(eb_end), __pa(__2M_rwdata_end)) )
> +            panic("Failed to reserve Xen in E820\n");
>       }
>       else
> -        reserve_e820_ram(&boot_e820, __pa(_stext), __pa(__2M_rwdata_end));
> +        if ( reserve_e820_ram(&boot_e820, __pa(_stext), __pa(__2M_rwdata_end)) )
> +            panic("Failed to reserve Xen in E820\n");
>   

This condition is inverted.
Otherwise this looks like a sensible change.

Ross
Re: [PATCH] x86/boot: Force error checking for reserve_e820_ram()
Posted by Andrew Cooper 2 weeks ago 
On 17/04/2026 5:27 pm, Ross Lagerwall wrote:
> On 4/17/26 5:08 PM, Andrew Cooper wrote:
>> Failing to mark Xen as Reserved in the E820 is catastrophic; RAM
>> regions get
>> handed to the physical memory allocator for general use.  Similarly,
>> failure
>> to mark the boot modules as reserved is not going to result in a working
>> system.
>>
>> Mark reserve_e820_ram() as __must_check, and panic() on failure.  To
>> avoid
>> opencoding the range in every caller, print a general failure message in
>> reserve_e820_ram().
>>
>> Reported-by: Ross Lagerwall <ross.lagerwall@citrix.com>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <jbeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Teddy Astie <teddy.astie@vates.tech>
>>
>> Slightly RFC; only compile tested so far.
>>
>> There's no obvious fixes tag.  This has been many variations of
>> broken since
>> forever.
>> ---
>>   xen/arch/x86/e820.c             |  8 +++++++-
>>   xen/arch/x86/include/asm/e820.h |  2 +-
>>   xen/arch/x86/setup.c            | 11 +++++++----
>>   3 files changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
>> index 872208ab3722..f09a01f0c50a 100644
>> --- a/xen/arch/x86/e820.c
>> +++ b/xen/arch/x86/e820.c
>> @@ -670,7 +670,13 @@ int __init e820_change_range_type(
>>   /* Set E820_RAM area (@s,@e) as RESERVED in specified e820 map. */
>>   int __init reserve_e820_ram(struct e820map *map, uint64_t s,
>> uint64_t e)
>>   {
>> -    return e820_change_range_type(map, s, e, E820_RAM, E820_RESERVED);
>> +    int res = e820_change_range_type(map, s, e, E820_RAM,
>> E820_RESERVED);
>> +
>> +    if ( !res )
>> +        printk("Failed to convert E820 RAM %"PRIx64"-%"PRIx64" to
>> RESERVED\n",
>> +               s, e);
>> +
>> +    return res;
>>   }
>>     unsigned long __init init_e820(const char *str, struct e820map *raw)
>> diff --git a/xen/arch/x86/include/asm/e820.h
>> b/xen/arch/x86/include/asm/e820.h
>> index 8e7644f8870b..a86d60ce3e77 100644
>> --- a/xen/arch/x86/include/asm/e820.h
>> +++ b/xen/arch/x86/include/asm/e820.h
>> @@ -25,7 +25,7 @@ struct e820map {
>>     extern int sanitize_e820_map(struct e820entry *biosmap, unsigned
>> int *pnr_map);
>>   extern int e820_all_mapped(u64 start, u64 end, unsigned type);
>> -extern int reserve_e820_ram(struct e820map *map, uint64_t s,
>> uint64_t e);
>> +extern int __must_check reserve_e820_ram(struct e820map *map,
>> uint64_t s, uint64_t e);
>>   extern int e820_change_range_type(
>>       struct e820map *map, uint64_t s, uint64_t e,
>>       uint32_t orig_type, uint32_t new_type);
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index d041cbd5f6f1..9c1f1eafa0d7 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -1639,7 +1639,8 @@ void asmlinkage __init noreturn __start_xen(void)
>>       {
>>           uint64_t s = bi->mods[i].start, l = bi->mods[i].size;
>>   -        reserve_e820_ram(&boot_e820, s, s + PAGE_ALIGN(l));
>> +        if ( !reserve_e820_ram(&boot_e820, s, s + PAGE_ALIGN(l)) )
>> +            panic("Failed to reserve boot module %u in E820\n", i);
>>       }
>>         if ( !xen_phys_start )
>> @@ -1652,11 +1653,13 @@ void asmlinkage __init noreturn
>> __start_xen(void)
>>       /* This needs to remain in sync with remove_xen_ranges(). */
>>       if ( efi_boot_mem_unused(&eb_start, &eb_end) )
>>       {
>> -        reserve_e820_ram(&boot_e820, __pa(_stext), __pa(eb_start));
>> -        reserve_e820_ram(&boot_e820, __pa(eb_end),
>> __pa(__2M_rwdata_end));
>> +        if ( !reserve_e820_ram(&boot_e820, __pa(_stext),
>> __pa(eb_start)) ||
>> +             !reserve_e820_ram(&boot_e820, __pa(eb_end),
>> __pa(__2M_rwdata_end)) )
>> +            panic("Failed to reserve Xen in E820\n");
>>       }
>>       else
>> -        reserve_e820_ram(&boot_e820, __pa(_stext),
>> __pa(__2M_rwdata_end));
>> +        if ( reserve_e820_ram(&boot_e820, __pa(_stext),
>> __pa(__2M_rwdata_end)) )
>> +            panic("Failed to reserve Xen in E820\n");
>>   
>
> This condition is inverted.
> Otherwise this looks like a sensible change.

Oops, yes.  Fixed.

This is a horrible function of type int but returning a boolean
success.  Still, I'm not altering the error scheme in the same patch as
this fix.

~Andrew
Re: [PATCH] x86/boot: Force error checking for reserve_e820_ram()
Posted by Ross Lagerwall 1 week, 4 days ago 
On 4/17/26 6:05 PM, Andrew Cooper wrote:
> On 17/04/2026 5:27 pm, Ross Lagerwall wrote:
>> On 4/17/26 5:08 PM, Andrew Cooper wrote:
>>> Failing to mark Xen as Reserved in the E820 is catastrophic; RAM
>>> regions get
>>> handed to the physical memory allocator for general use.  Similarly,
>>> failure
>>> to mark the boot modules as reserved is not going to result in a working
>>> system.
>>>
>>> Mark reserve_e820_ram() as __must_check, and panic() on failure.  To
>>> avoid
>>> opencoding the range in every caller, print a general failure message in
>>> reserve_e820_ram().
>>>
>>> Reported-by: Ross Lagerwall <ross.lagerwall@citrix.com>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> CC: Jan Beulich <jbeulich@suse.com>
>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>> CC: Teddy Astie <teddy.astie@vates.tech>
>>>
>>> Slightly RFC; only compile tested so far.
>>>
>>> There's no obvious fixes tag.  This has been many variations of
>>> broken since
>>> forever.
>>> ---
>>>    xen/arch/x86/e820.c             |  8 +++++++-
>>>    xen/arch/x86/include/asm/e820.h |  2 +-
>>>    xen/arch/x86/setup.c            | 11 +++++++----
>>>    3 files changed, 15 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
>>> index 872208ab3722..f09a01f0c50a 100644
>>> --- a/xen/arch/x86/e820.c
>>> +++ b/xen/arch/x86/e820.c
>>> @@ -670,7 +670,13 @@ int __init e820_change_range_type(
>>>    /* Set E820_RAM area (@s,@e) as RESERVED in specified e820 map. */
>>>    int __init reserve_e820_ram(struct e820map *map, uint64_t s,
>>> uint64_t e)
>>>    {
>>> -    return e820_change_range_type(map, s, e, E820_RAM, E820_RESERVED);
>>> +    int res = e820_change_range_type(map, s, e, E820_RAM,
>>> E820_RESERVED);
>>> +
>>> +    if ( !res )
>>> +        printk("Failed to convert E820 RAM %"PRIx64"-%"PRIx64" to
>>> RESERVED\n",
>>> +               s, e);
>>> +
>>> +    return res;
>>>    }
>>>      unsigned long __init init_e820(const char *str, struct e820map *raw)
>>> diff --git a/xen/arch/x86/include/asm/e820.h
>>> b/xen/arch/x86/include/asm/e820.h
>>> index 8e7644f8870b..a86d60ce3e77 100644
>>> --- a/xen/arch/x86/include/asm/e820.h
>>> +++ b/xen/arch/x86/include/asm/e820.h
>>> @@ -25,7 +25,7 @@ struct e820map {
>>>      extern int sanitize_e820_map(struct e820entry *biosmap, unsigned
>>> int *pnr_map);
>>>    extern int e820_all_mapped(u64 start, u64 end, unsigned type);
>>> -extern int reserve_e820_ram(struct e820map *map, uint64_t s,
>>> uint64_t e);
>>> +extern int __must_check reserve_e820_ram(struct e820map *map,
>>> uint64_t s, uint64_t e);
>>>    extern int e820_change_range_type(
>>>        struct e820map *map, uint64_t s, uint64_t e,
>>>        uint32_t orig_type, uint32_t new_type);
>>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>>> index d041cbd5f6f1..9c1f1eafa0d7 100644
>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -1639,7 +1639,8 @@ void asmlinkage __init noreturn __start_xen(void)
>>>        {
>>>            uint64_t s = bi->mods[i].start, l = bi->mods[i].size;
>>>    -        reserve_e820_ram(&boot_e820, s, s + PAGE_ALIGN(l));
>>> +        if ( !reserve_e820_ram(&boot_e820, s, s + PAGE_ALIGN(l)) )
>>> +            panic("Failed to reserve boot module %u in E820\n", i);

i is an int, so it should be %d.

>>>        }
>>>          if ( !xen_phys_start )
>>> @@ -1652,11 +1653,13 @@ void asmlinkage __init noreturn
>>> __start_xen(void)
>>>        /* This needs to remain in sync with remove_xen_ranges(). */
>>>        if ( efi_boot_mem_unused(&eb_start, &eb_end) )
>>>        {
>>> -        reserve_e820_ram(&boot_e820, __pa(_stext), __pa(eb_start));
>>> -        reserve_e820_ram(&boot_e820, __pa(eb_end),
>>> __pa(__2M_rwdata_end));
>>> +        if ( !reserve_e820_ram(&boot_e820, __pa(_stext),
>>> __pa(eb_start)) ||
>>> +             !reserve_e820_ram(&boot_e820, __pa(eb_end),
>>> __pa(__2M_rwdata_end)) )
>>> +            panic("Failed to reserve Xen in E820\n");
>>>        }
>>>        else
>>> -        reserve_e820_ram(&boot_e820, __pa(_stext),
>>> __pa(__2M_rwdata_end));
>>> +        if ( reserve_e820_ram(&boot_e820, __pa(_stext),
>>> __pa(__2M_rwdata_end)) )
>>> +            panic("Failed to reserve Xen in E820\n");
>>>    
>>
>> This condition is inverted.
>> Otherwise this looks like a sensible change.
> 
> Oops, yes.  Fixed.

With those changes,

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.