diff mbox series

[v9,2/6] x86/boot: introduce module release

Message ID 20241115131204.32135-3-dpsmith@apertussolutions.com (mailing list archive)
State New
Headers show
Series Boot modules for Hyperlaunch | expand

Commit Message

Daniel P. Smith Nov. 15, 2024, 1:12 p.m. UTC
A precarious approach was used to release the pages used to hold a boot module.
The precariousness stemmed from the fact that in the case of PV dom0, the
initrd module pages may be either mapped or copied into the dom0 address space.
In the former case, the PV dom0 construction code will set the size of the
module to zero, relying on discard_initial_images() to skip any modules with a
size of zero. In the latter case, the pages are freed by the PV dom0
construction code. This freeing of pages is done so that in either case, the
initrd variable can be reused for tracking the initrd location in dom0 memory
through the remaining dom0 construction code.

To encapsulate the logical action of releasing a boot module, the function
release_boot_module() is introduced along with the `released` flag added to
boot module. The boot module flag `released` allows the tracking of when a boot
module has been released by release_boot_module().

As part of adopting release_boot_module() the function discard_initial_images()
is renamed to free_boot_modules(), a name that better reflects the functions
actions.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
Changes since v8:
- completely reworked the commit
  - switch backed to a releasing all but pv initrd approach
  - renamed discard_initial_images to free_boot_modules
---
 xen/arch/x86/hvm/dom0_build.c       |  2 +-
 xen/arch/x86/include/asm/bootinfo.h |  2 ++
 xen/arch/x86/include/asm/setup.h    |  4 +++-
 xen/arch/x86/pv/dom0_build.c        | 27 +++++++++++++--------------
 xen/arch/x86/setup.c                | 27 +++++++++++++++------------
 5 files changed, 34 insertions(+), 28 deletions(-)

Comments

Jason Andryuk Nov. 15, 2024, 4:50 p.m. UTC | #1
On 2024-11-15 08:12, Daniel P. Smith wrote:
> A precarious approach was used to release the pages used to hold a boot module.
> The precariousness stemmed from the fact that in the case of PV dom0, the
> initrd module pages may be either mapped or copied into the dom0 address space.
> In the former case, the PV dom0 construction code will set the size of the
> module to zero, relying on discard_initial_images() to skip any modules with a
> size of zero. In the latter case, the pages are freed by the PV dom0
> construction code. This freeing of pages is done so that in either case, the
> initrd variable can be reused for tracking the initrd location in dom0 memory
> through the remaining dom0 construction code.
> 
> To encapsulate the logical action of releasing a boot module, the function
> release_boot_module() is introduced along with the `released` flag added to
> boot module. The boot module flag `released` allows the tracking of when a boot
> module has been released by release_boot_module().
> 
> As part of adopting release_boot_module() the function discard_initial_images()
> is renamed to free_boot_modules(), a name that better reflects the functions
> actions.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> ---
> Changes since v8:
> - completely reworked the commit
>    - switch backed to a releasing all but pv initrd approach
>    - renamed discard_initial_images to free_boot_modules
> ---
>   xen/arch/x86/hvm/dom0_build.c       |  2 +-
>   xen/arch/x86/include/asm/bootinfo.h |  2 ++
>   xen/arch/x86/include/asm/setup.h    |  4 +++-
>   xen/arch/x86/pv/dom0_build.c        | 27 +++++++++++++--------------
>   xen/arch/x86/setup.c                | 27 +++++++++++++++------------
>   5 files changed, 34 insertions(+), 28 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> index d1bdf1b14601..d1410e1a02b0 100644
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -755,7 +755,7 @@ static int __init pvh_load_kernel(
>       }
>   
>       /* Free temporary buffers. */
> -    discard_initial_images();
> +    free_boot_modules();

This...

>       if ( cmdline != NULL )
>       {

> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
> index 6be3d7745fab..2580162f3df4 100644
> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c

> @@ -875,7 +874,7 @@ static int __init dom0_construct(struct boot_info *bi, struct domain *d)
>       }
>   
>       /* Free temporary buffers. */
> -    discard_initial_images();
> +    free_boot_modules();

...and this.  I think Andrew requested/suggested moving to a single 
free_boot_modules call:
     They're both right at the end of construction, so it would
     make far more sense for __start_xen() to do this after
     create_dom0().   That also avoids needing to export the function.

>   
>       /* Set up start info area. */
>       si = (start_info_t *)vstartinfo_start;
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 495e90a7e132..0bda1326a485 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c

> +void __init free_boot_modules(void)
>   {
>       struct boot_info *bi = &xen_boot_info;
>       unsigned int i;
>   
>       for ( i = 0; i < bi->nr_modules; ++i )
>       {
> -        uint64_t start = pfn_to_paddr(bi->mods[i].mod->mod_start);
> -        uint64_t size  = bi->mods[i].mod->mod_end;
> -
> -        /*
> -         * Sometimes the initrd is mapped, rather than copied, into dom0.
> -         * Size being 0 is how we're instructed to leave the module alone.
> -         */
> -        if ( size == 0 )
> +        if ( bi->mods[i].released )
>               continue;
>   
> -        init_domheap_pages(start, start + PAGE_ALIGN(size));
> +        release_boot_module(&bi->mods[i]);
>       }
> -
> -    bi->nr_modules = 0;

IIUC, zero-ing here was a safety feature to ensure boot modules could 
not be used after this point.  Should it be retained?

Regards,
Jason

>   }
>   
>   static void __init init_idle_domain(void)
Andrew Cooper Nov. 15, 2024, 5:09 p.m. UTC | #2
On 15/11/2024 4:50 pm, Jason Andryuk wrote:
> On 2024-11-15 08:12, Daniel P. Smith wrote:
>> A precarious approach was used to release the pages used to hold a
>> boot module.
>> The precariousness stemmed from the fact that in the case of PV dom0,
>> the
>> initrd module pages may be either mapped or copied into the dom0
>> address space.
>> In the former case, the PV dom0 construction code will set the size
>> of the
>> module to zero, relying on discard_initial_images() to skip any
>> modules with a
>> size of zero. In the latter case, the pages are freed by the PV dom0
>> construction code. This freeing of pages is done so that in either
>> case, the
>> initrd variable can be reused for tracking the initrd location in
>> dom0 memory
>> through the remaining dom0 construction code.
>>
>> To encapsulate the logical action of releasing a boot module, the
>> function
>> release_boot_module() is introduced along with the `released` flag
>> added to
>> boot module. The boot module flag `released` allows the tracking of
>> when a boot
>> module has been released by release_boot_module().
>>
>> As part of adopting release_boot_module() the function
>> discard_initial_images()
>> is renamed to free_boot_modules(), a name that better reflects the
>> functions
>> actions.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> ---
>> Changes since v8:
>> - completely reworked the commit
>>    - switch backed to a releasing all but pv initrd approach
>>    - renamed discard_initial_images to free_boot_modules
>> ---
>>   xen/arch/x86/hvm/dom0_build.c       |  2 +-
>>   xen/arch/x86/include/asm/bootinfo.h |  2 ++
>>   xen/arch/x86/include/asm/setup.h    |  4 +++-
>>   xen/arch/x86/pv/dom0_build.c        | 27 +++++++++++++--------------
>>   xen/arch/x86/setup.c                | 27 +++++++++++++++------------
>>   5 files changed, 34 insertions(+), 28 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/dom0_build.c
>> b/xen/arch/x86/hvm/dom0_build.c
>> index d1bdf1b14601..d1410e1a02b0 100644
>> --- a/xen/arch/x86/hvm/dom0_build.c
>> +++ b/xen/arch/x86/hvm/dom0_build.c
>> @@ -755,7 +755,7 @@ static int __init pvh_load_kernel(
>>       }
>>         /* Free temporary buffers. */
>> -    discard_initial_images();
>> +    free_boot_modules();
>
> This...
>
>>       if ( cmdline != NULL )
>>       {
>
>> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
>> index 6be3d7745fab..2580162f3df4 100644
>> --- a/xen/arch/x86/pv/dom0_build.c
>> +++ b/xen/arch/x86/pv/dom0_build.c
>
>> @@ -875,7 +874,7 @@ static int __init dom0_construct(struct boot_info
>> *bi, struct domain *d)
>>       }
>>         /* Free temporary buffers. */
>> -    discard_initial_images();
>> +    free_boot_modules();
>
> ...and this.  I think Andrew requested/suggested moving to a single
> free_boot_modules call:
>     They're both right at the end of construction, so it would
>     make far more sense for __start_xen() to do this after
>     create_dom0().   That also avoids needing to export the function.

Yeah...  It turns out that also breaks PVH Boot in Gitlab, for reasons
we still don't understand.

I'd still like to clean it up, but it wants to be detached from the
mechanics of changing the data-structures.

>
>>         /* Set up start info area. */
>>       si = (start_info_t *)vstartinfo_start;
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index 495e90a7e132..0bda1326a485 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>
>> +void __init free_boot_modules(void)
>>   {
>>       struct boot_info *bi = &xen_boot_info;
>>       unsigned int i;
>>         for ( i = 0; i < bi->nr_modules; ++i )
>>       {
>> -        uint64_t start = pfn_to_paddr(bi->mods[i].mod->mod_start);
>> -        uint64_t size  = bi->mods[i].mod->mod_end;
>> -
>> -        /*
>> -         * Sometimes the initrd is mapped, rather than copied, into
>> dom0.
>> -         * Size being 0 is how we're instructed to leave the module
>> alone.
>> -         */
>> -        if ( size == 0 )
>> +        if ( bi->mods[i].released )
>>               continue;
>>   -        init_domheap_pages(start, start + PAGE_ALIGN(size));
>> +        release_boot_module(&bi->mods[i]);
>>       }
>> -
>> -    bi->nr_modules = 0;
>
> IIUC, zero-ing here was a safety feature to ensure boot modules could
> not be used after this point.  Should it be retained?

Clobbering this prevents the loop constructs from working.

Safety is now based on the .released field, which is better IMO.

~Andrew
Daniel P. Smith Nov. 15, 2024, 5:16 p.m. UTC | #3
On 11/15/24 11:50, Jason Andryuk wrote:
> On 2024-11-15 08:12, Daniel P. Smith wrote:
>> A precarious approach was used to release the pages used to hold a 
>> boot module.
>> The precariousness stemmed from the fact that in the case of PV dom0, the
>> initrd module pages may be either mapped or copied into the dom0 
>> address space.
>> In the former case, the PV dom0 construction code will set the size of 
>> the
>> module to zero, relying on discard_initial_images() to skip any 
>> modules with a
>> size of zero. In the latter case, the pages are freed by the PV dom0
>> construction code. This freeing of pages is done so that in either 
>> case, the
>> initrd variable can be reused for tracking the initrd location in dom0 
>> memory
>> through the remaining dom0 construction code.
>>
>> To encapsulate the logical action of releasing a boot module, the 
>> function
>> release_boot_module() is introduced along with the `released` flag 
>> added to
>> boot module. The boot module flag `released` allows the tracking of 
>> when a boot
>> module has been released by release_boot_module().
>>
>> As part of adopting release_boot_module() the function 
>> discard_initial_images()
>> is renamed to free_boot_modules(), a name that better reflects the 
>> functions
>> actions.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> ---
>> Changes since v8:
>> - completely reworked the commit
>>    - switch backed to a releasing all but pv initrd approach
>>    - renamed discard_initial_images to free_boot_modules
>> ---
>>   xen/arch/x86/hvm/dom0_build.c       |  2 +-
>>   xen/arch/x86/include/asm/bootinfo.h |  2 ++
>>   xen/arch/x86/include/asm/setup.h    |  4 +++-
>>   xen/arch/x86/pv/dom0_build.c        | 27 +++++++++++++--------------
>>   xen/arch/x86/setup.c                | 27 +++++++++++++++------------
>>   5 files changed, 34 insertions(+), 28 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/ 
>> dom0_build.c
>> index d1bdf1b14601..d1410e1a02b0 100644
>> --- a/xen/arch/x86/hvm/dom0_build.c
>> +++ b/xen/arch/x86/hvm/dom0_build.c
>> @@ -755,7 +755,7 @@ static int __init pvh_load_kernel(
>>       }
>>       /* Free temporary buffers. */
>> -    discard_initial_images();
>> +    free_boot_modules();
> 
> This...
> 
>>       if ( cmdline != NULL )
>>       {
> 
>> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
>> index 6be3d7745fab..2580162f3df4 100644
>> --- a/xen/arch/x86/pv/dom0_build.c
>> +++ b/xen/arch/x86/pv/dom0_build.c
> 
>> @@ -875,7 +874,7 @@ static int __init dom0_construct(struct boot_info 
>> *bi, struct domain *d)
>>       }
>>       /* Free temporary buffers. */
>> -    discard_initial_images();
>> +    free_boot_modules();
> 
> ...and this.  I think Andrew requested/suggested moving to a single 
> free_boot_modules call:
>      They're both right at the end of construction, so it would
>      make far more sense for __start_xen() to do this after
>      create_dom0().   That also avoids needing to export the function.

I wanted to do this and had it written this way. Then I started testing 
it and the pvhshim test failed due to not enough ram to build the domU 
inside pvshim. I started splitting this commit to see where it broke the 
test case, and for an unknown reason, replacing these two calls with a 
single call in __start_xen() just after create_dom0() is the cause. 
Instead of trying to tear apart the construction logic to determine why, 
I backed this part of the change out for the time being.

>>       /* Set up start info area. */
>>       si = (start_info_t *)vstartinfo_start;
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index 495e90a7e132..0bda1326a485 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
> 
>> +void __init free_boot_modules(void)
>>   {
>>       struct boot_info *bi = &xen_boot_info;
>>       unsigned int i;
>>       for ( i = 0; i < bi->nr_modules; ++i )
>>       {
>> -        uint64_t start = pfn_to_paddr(bi->mods[i].mod->mod_start);
>> -        uint64_t size  = bi->mods[i].mod->mod_end;
>> -
>> -        /*
>> -         * Sometimes the initrd is mapped, rather than copied, into 
>> dom0.
>> -         * Size being 0 is how we're instructed to leave the module 
>> alone.
>> -         */
>> -        if ( size == 0 )
>> +        if ( bi->mods[i].released )
>>               continue;
>> -        init_domheap_pages(start, start + PAGE_ALIGN(size));
>> +        release_boot_module(&bi->mods[i]);
>>       }
>> -
>> -    bi->nr_modules = 0;
> 
> IIUC, zero-ing here was a safety feature to ensure boot modules could 
> not be used after this point.  Should it be retained?

The released flag displaced the need for this, but I realized it would 
make it stronger if in bootstrap_map_bm() we add a check that the 
released flag is not set before mapping. I think this is a stronger 
approach without loosing information like the number of boot modules 
were passed.

v/r,
dps
Jason Andryuk Nov. 15, 2024, 5:18 p.m. UTC | #4
On 2024-11-15 12:16, Daniel P. Smith wrote:
> On 11/15/24 11:50, Jason Andryuk wrote:
>> On 2024-11-15 08:12, Daniel P. Smith wrote:
>>> A precarious approach was used to release the pages used to hold a 
>>> boot module.
>>> The precariousness stemmed from the fact that in the case of PV dom0, 
>>> the
>>> initrd module pages may be either mapped or copied into the dom0 
>>> address space.
>>> In the former case, the PV dom0 construction code will set the size 
>>> of the
>>> module to zero, relying on discard_initial_images() to skip any 
>>> modules with a
>>> size of zero. In the latter case, the pages are freed by the PV dom0
>>> construction code. This freeing of pages is done so that in either 
>>> case, the
>>> initrd variable can be reused for tracking the initrd location in 
>>> dom0 memory
>>> through the remaining dom0 construction code.
>>>
>>> To encapsulate the logical action of releasing a boot module, the 
>>> function
>>> release_boot_module() is introduced along with the `released` flag 
>>> added to
>>> boot module. The boot module flag `released` allows the tracking of 
>>> when a boot
>>> module has been released by release_boot_module().
>>>
>>> As part of adopting release_boot_module() the function 
>>> discard_initial_images()
>>> is renamed to free_boot_modules(), a name that better reflects the 
>>> functions
>>> actions.
>>>
>>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>>> ---
>>> Changes since v8:
>>> - completely reworked the commit
>>>    - switch backed to a releasing all but pv initrd approach
>>>    - renamed discard_initial_images to free_boot_modules
>>> ---
>>>   xen/arch/x86/hvm/dom0_build.c       |  2 +-
>>>   xen/arch/x86/include/asm/bootinfo.h |  2 ++
>>>   xen/arch/x86/include/asm/setup.h    |  4 +++-
>>>   xen/arch/x86/pv/dom0_build.c        | 27 +++++++++++++--------------
>>>   xen/arch/x86/setup.c                | 27 +++++++++++++++------------
>>>   5 files changed, 34 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/ 
>>> dom0_build.c
>>> index d1bdf1b14601..d1410e1a02b0 100644
>>> --- a/xen/arch/x86/hvm/dom0_build.c
>>> +++ b/xen/arch/x86/hvm/dom0_build.c
>>> @@ -755,7 +755,7 @@ static int __init pvh_load_kernel(
>>>       }
>>>       /* Free temporary buffers. */
>>> -    discard_initial_images();
>>> +    free_boot_modules();
>>
>> This...
>>
>>>       if ( cmdline != NULL )
>>>       {
>>
>>> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
>>> index 6be3d7745fab..2580162f3df4 100644
>>> --- a/xen/arch/x86/pv/dom0_build.c
>>> +++ b/xen/arch/x86/pv/dom0_build.c
>>
>>> @@ -875,7 +874,7 @@ static int __init dom0_construct(struct boot_info 
>>> *bi, struct domain *d)
>>>       }
>>>       /* Free temporary buffers. */
>>> -    discard_initial_images();
>>> +    free_boot_modules();
>>
>> ...and this.  I think Andrew requested/suggested moving to a single 
>> free_boot_modules call:
>>      They're both right at the end of construction, so it would
>>      make far more sense for __start_xen() to do this after
>>      create_dom0().   That also avoids needing to export the function.
> 
> I wanted to do this and had it written this way. Then I started testing 
> it and the pvhshim test failed due to not enough ram to build the domU 
> inside pvshim. I started splitting this commit to see where it broke the 
> test case, and for an unknown reason, replacing these two calls with a 
> single call in __start_xen() just after create_dom0() is the cause. 
> Instead of trying to tear apart the construction logic to determine why, 
> I backed this part of the change out for the time being.

Ah, ok.  Thanks for the info.

>>>       /* Set up start info area. */
>>>       si = (start_info_t *)vstartinfo_start;
>>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>>> index 495e90a7e132..0bda1326a485 100644
>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>
>>> +void __init free_boot_modules(void)
>>>   {
>>>       struct boot_info *bi = &xen_boot_info;
>>>       unsigned int i;
>>>       for ( i = 0; i < bi->nr_modules; ++i )
>>>       {
>>> -        uint64_t start = pfn_to_paddr(bi->mods[i].mod->mod_start);
>>> -        uint64_t size  = bi->mods[i].mod->mod_end;
>>> -
>>> -        /*
>>> -         * Sometimes the initrd is mapped, rather than copied, into 
>>> dom0.
>>> -         * Size being 0 is how we're instructed to leave the module 
>>> alone.
>>> -         */
>>> -        if ( size == 0 )
>>> +        if ( bi->mods[i].released )
>>>               continue;
>>> -        init_domheap_pages(start, start + PAGE_ALIGN(size));
>>> +        release_boot_module(&bi->mods[i]);
>>>       }
>>> -
>>> -    bi->nr_modules = 0;
>>
>> IIUC, zero-ing here was a safety feature to ensure boot modules could 
>> not be used after this point.  Should it be retained?
> 
> The released flag displaced the need for this, but I realized it would 
> make it stronger if in bootstrap_map_bm() we add a check that the 
> released flag is not set before mapping. I think this is a stronger 
> approach without loosing information like the number of boot modules 
> were passed.

Andrew> Clobbering this prevents the loop constructs from working.

I thought the boot modules are unusable after free_boot_modules() is 
called, so I'm not clear on the utility of keeping the boot modules 
around and/or keeping the loop constructs working.  I wondered about, 
but didn't write, clearing the boot_module info in release_boot_module() 
to eliminate stale data hanging around.

Yes, a bootstrap_map_bm() check is a good idea.  Having said that, there 
is a lack of checking the return value of bootstrap_map_bm(), so would 
you panic?

Regards,
Jason
diff mbox series

Patch

diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index d1bdf1b14601..d1410e1a02b0 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -755,7 +755,7 @@  static int __init pvh_load_kernel(
     }
 
     /* Free temporary buffers. */
-    discard_initial_images();
+    free_boot_modules();
 
     if ( cmdline != NULL )
     {
diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
index b9c94b370d57..f76876386763 100644
--- a/xen/arch/x86/include/asm/bootinfo.h
+++ b/xen/arch/x86/include/asm/bootinfo.h
@@ -34,8 +34,10 @@  struct boot_module {
     /*
      * Module State Flags:
      *   relocated: indicates module has been relocated in memory.
+     *   released:  indicates module's pages have been freed.
      */
     bool relocated:1;
+    bool released:1;
 
     /*
      * A boot module may need decompressing by Xen.  Headroom is an estimate of
diff --git a/xen/arch/x86/include/asm/setup.h b/xen/arch/x86/include/asm/setup.h
index 8a415087e9a4..4ad493637892 100644
--- a/xen/arch/x86/include/asm/setup.h
+++ b/xen/arch/x86/include/asm/setup.h
@@ -34,13 +34,15 @@  void setup_io_bitmap(struct domain *d);
 extern struct boot_info xen_boot_info;
 
 unsigned long initial_images_nrpages(nodeid_t node);
-void discard_initial_images(void);
+void free_boot_modules(void);
 
 struct boot_module;
 void *bootstrap_map_bm(const struct boot_module *bm);
 void *bootstrap_map(const module_t *mod);
 void bootstrap_unmap(void);
 
+void release_boot_module(struct boot_module *bm);
+
 struct rangeset;
 int remove_xen_ranges(struct rangeset *r);
 
diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index 6be3d7745fab..2580162f3df4 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -649,9 +649,12 @@  static int __init dom0_construct(struct boot_info *bi, struct domain *d)
                 }
             memcpy(page_to_virt(page), mfn_to_virt(initrd->mod->mod_start),
                    initrd_len);
-            mpt_alloc = pfn_to_paddr(initrd->mod->mod_start);
-            init_domheap_pages(mpt_alloc,
-                               mpt_alloc + PAGE_ALIGN(initrd_len));
+            /*
+             * The initrd was copied but the initrd variable is reused in the
+             * calculations below. As to not leak the memory used for the
+             * module free at this time.
+             */
+            release_boot_module(initrd);
             initrd_mfn = mfn_x(page_to_mfn(page));
             initrd->mod->mod_start = initrd_mfn;
         }
@@ -660,18 +663,14 @@  static int __init dom0_construct(struct boot_info *bi, struct domain *d)
             while ( count-- )
                 if ( assign_pages(mfn_to_page(_mfn(mfn++)), 1, d, 0) )
                     BUG();
+            /*
+             * We have mapped the initrd directly into dom0, and assigned the
+             * pages. Tell the boot_module handling that we've freed it, so the
+             * memory is left alone.
+             */
+            initrd->released = true;
         }
 
-        /*
-         * We have either:
-         * - Mapped the initrd directly into dom0, or
-         * - Copied it and freed the module.
-         *
-         * Either way, tell discard_initial_images() to not free it a second
-         * time.
-         */
-        initrd->mod->mod_end = 0;
-
         iommu_memory_setup(d, "initrd", mfn_to_page(_mfn(initrd_mfn)),
                            PFN_UP(initrd_len), &flush_flags);
     }
@@ -875,7 +874,7 @@  static int __init dom0_construct(struct boot_info *bi, struct domain *d)
     }
 
     /* Free temporary buffers. */
-    discard_initial_images();
+    free_boot_modules();
 
     /* Set up start info area. */
     si = (start_info_t *)vstartinfo_start;
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 495e90a7e132..0bda1326a485 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -346,27 +346,30 @@  unsigned long __init initial_images_nrpages(nodeid_t node)
     return nr;
 }
 
-void __init discard_initial_images(void) /* a.k.a. Free boot modules */
+void __init release_boot_module(struct boot_module *bm)
+{
+    uint64_t start = pfn_to_paddr(bm->mod->mod_start);
+    uint64_t size  = bm->mod->mod_end;
+
+    ASSERT(!bm->released);
+
+    init_domheap_pages(start, start + PAGE_ALIGN(size));
+
+    bm->released = true;
+}
+
+void __init free_boot_modules(void)
 {
     struct boot_info *bi = &xen_boot_info;
     unsigned int i;
 
     for ( i = 0; i < bi->nr_modules; ++i )
     {
-        uint64_t start = pfn_to_paddr(bi->mods[i].mod->mod_start);
-        uint64_t size  = bi->mods[i].mod->mod_end;
-
-        /*
-         * Sometimes the initrd is mapped, rather than copied, into dom0.
-         * Size being 0 is how we're instructed to leave the module alone.
-         */
-        if ( size == 0 )
+        if ( bi->mods[i].released )
             continue;
 
-        init_domheap_pages(start, start + PAGE_ALIGN(size));
+        release_boot_module(&bi->mods[i]);
     }
-
-    bi->nr_modules = 0;
 }
 
 static void __init init_idle_domain(void)