diff mbox

[v4,08/14] hvmloader: Locate the BIOS blob

Message ID 1457978150-27201-9-git-send-email-anthony.perard@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Anthony PERARD March 14, 2016, 5:55 p.m. UTC
The BIOS can be found an entry called "bios" of the modlist of the
hvm_start_info struct.

The found BIOS blob is not loaded by this patch, but only passed as
argument to bios_load() function. It is going to be used by the next few
patches.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

---
Changes in V4:
- add more BUG_ON into get_module_entry(). Check that modules paddr and
  size are 32bits.

Changes in V3:
- fix some codying style
- use module.cmdline to look for a module name instead of the main cmdline
  from hvm_start_info.
---
 tools/firmware/hvmloader/config.h    |  2 +-
 tools/firmware/hvmloader/hvmloader.c | 42 ++++++++++++++++++++++++++++++++++--
 tools/firmware/hvmloader/ovmf.c      |  3 ++-
 tools/firmware/hvmloader/rombios.c   |  3 ++-
 tools/firmware/hvmloader/util.h      |  2 ++
 5 files changed, 47 insertions(+), 5 deletions(-)

Comments

Konrad Rzeszutek Wilk March 16, 2016, 1:14 a.m. UTC | #1
On Mon, Mar 14, 2016 at 05:55:43PM +0000, Anthony PERARD wrote:
> The BIOS can be found an entry called "bios" of the modlist of the

s/BIOS/BIOS blob/
> hvm_start_info struct.
> 
> The found BIOS blob is not loaded by this patch, but only passed as
> argument to bios_load() function. It is going to be used by the next few
> patches.

Please list the 'few patches' by name as this patch and others may not
always be committed in order.

And it also helps in future to figure out what those other ones are
when debugging or backporting.

See below.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> 
> ---
> Changes in V4:
> - add more BUG_ON into get_module_entry(). Check that modules paddr and
>   size are 32bits.
> 
> Changes in V3:
> - fix some codying style
> - use module.cmdline to look for a module name instead of the main cmdline
>   from hvm_start_info.
> ---
>  tools/firmware/hvmloader/config.h    |  2 +-
>  tools/firmware/hvmloader/hvmloader.c | 42 ++++++++++++++++++++++++++++++++++--
>  tools/firmware/hvmloader/ovmf.c      |  3 ++-
>  tools/firmware/hvmloader/rombios.c   |  3 ++-
>  tools/firmware/hvmloader/util.h      |  2 ++
>  5 files changed, 47 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/firmware/hvmloader/config.h b/tools/firmware/hvmloader/config.h
> index b838cf9..4c6d8ad 100644
> --- a/tools/firmware/hvmloader/config.h
> +++ b/tools/firmware/hvmloader/config.h
> @@ -22,7 +22,7 @@ struct bios_config {
>      /* ROMS */
>      void (*load_roms)(void);
>  
> -    void (*bios_load)(const struct bios_config *config);
> +    void (*bios_load)(const struct bios_config *config, void *addr, uint32_t size);
>  
>      void (*bios_info_setup)(void);
>      void (*bios_info_finish)(void);
> diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c
> index c45f367..460efb9 100644
> --- a/tools/firmware/hvmloader/hvmloader.c
> +++ b/tools/firmware/hvmloader/hvmloader.c
> @@ -253,10 +253,40 @@ static void acpi_enable_sci(void)
>      BUG_ON(!(pm1a_cnt_val & ACPI_PM1C_SCI_EN));
>  }
>  
> +const struct hvm_modlist_entry *get_module_entry(
> +    const struct hvm_start_info *info,
> +    const char *name)
> +{
> +    const struct hvm_modlist_entry *modlist =
> +        (struct hvm_modlist_entry *)info->modlist_paddr;
> +    unsigned int i;
> +
> +    if ( !modlist )
> +        return NULL;
> +
> +    for ( i = 0; i < info->nr_modules; i++ )
> +    {
> +        uint32_t module_name = modlist[i].cmdline_paddr;
> +
> +        BUG_ON(!modlist[i].cmdline_paddr ||

Having an zero value in cmdline_paddr is OK.

The spec says:

803  * NOTE: nothing will be loaded at physical address 0, so a 0 value in any
804  * of the address fields should be treated as not present.
805  *

> +               modlist[i].cmdline_paddr > UINT_MAX);
> +
> +        if ( !strcmp(name, (char*)module_name) )

Which could result in looking up a value at 0 address. 

Perhaps change your code to:
	if ( !modlist[i].cmdline_paddr )
		continue;
?


> +        {
> +            BUG_ON(!modlist[i].paddr || modlist[i].paddr > UINT_MAX ||
> +                   modlist[i].size > UINT_MAX);

To be fair all of those values are in spec..Perhaps you should mention
that the libxc code would never put those there and neermind the spec?


> +            return &modlist[i];
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
>  int main(void)
>  {
>      const struct bios_config *bios;
>      int acpi_enabled;
> +    const struct hvm_modlist_entry *bios_module;
>  
>      /* Initialise hypercall stubs with RET, rendering them no-ops. */
>      memset((void *)HYPERCALL_PHYSICAL_ADDRESS, 0xc3 /* RET */, PAGE_SIZE);
> @@ -292,8 +322,16 @@ int main(void)
>      }
>  
>      printf("Loading %s ...\n", bios->name);
> -    if ( bios->bios_load )
> -        bios->bios_load(bios);
> +    bios_module = get_module_entry(hvm_start_info, "bios");
> +    if ( bios_module && bios->bios_load )
> +    {
> +        uint32_t paddr = bios_module->paddr;
> +        bios->bios_load(bios, (void*)paddr, bios_module->size);
> +    }
> +    else if ( bios->bios_load )
> +    {
> +        bios->bios_load(bios, 0, 0);
> +    }
>      else
>      {
>          BUG_ON(bios->bios_address + bios->image_size >
> diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
> index db9fa7a..858a2d4 100644
> --- a/tools/firmware/hvmloader/ovmf.c
> +++ b/tools/firmware/hvmloader/ovmf.c
> @@ -93,7 +93,8 @@ static void ovmf_finish_bios_info(void)
>      info->checksum = -checksum;
>  }
>  
> -static void ovmf_load(const struct bios_config *config)
> +static void ovmf_load(const struct bios_config *config,
> +                      void *bios_addr, uint32_t bios_length)
>  {
>      xen_pfn_t mfn;
>      uint64_t addr = OVMF_BEGIN;
> diff --git a/tools/firmware/hvmloader/rombios.c b/tools/firmware/hvmloader/rombios.c
> index 1f15b94..2ded844 100644
> --- a/tools/firmware/hvmloader/rombios.c
> +++ b/tools/firmware/hvmloader/rombios.c
> @@ -121,7 +121,8 @@ static void rombios_load_roms(void)
>                 option_rom_phys_addr + option_rom_sz - 1);
>  }
>  
> -static void rombios_load(const struct bios_config *config)
> +static void rombios_load(const struct bios_config *config,
> +                         void *unused_addr, uint32_t unused_size)
>  {
>      uint32_t bioshigh;
>      struct rombios_info *info;
> diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
> index 9808016..003dc71 100644
> --- a/tools/firmware/hvmloader/util.h
> +++ b/tools/firmware/hvmloader/util.h
> @@ -34,6 +34,8 @@ enum {
>  #undef NULL
>  #define NULL ((void*)0)
>  
> +#define UINT_MAX (~0U)
> +
>  void __assert_failed(char *assertion, char *file, int line)
>      __attribute__((noreturn));
>  #define ASSERT(p) \
> -- 
> Anthony PERARD
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
Anthony PERARD March 17, 2016, 5:46 p.m. UTC | #2
On Tue, Mar 15, 2016 at 09:14:17PM -0400, Konrad Rzeszutek Wilk wrote:
> On Mon, Mar 14, 2016 at 05:55:43PM +0000, Anthony PERARD wrote:
> > The BIOS can be found an entry called "bios" of the modlist of the
> 
> s/BIOS/BIOS blob/
> > hvm_start_info struct.
> > 
> > The found BIOS blob is not loaded by this patch, but only passed as
> > argument to bios_load() function. It is going to be used by the next few
> > patches.
> 
> Please list the 'few patches' by name as this patch and others may not
> always be committed in order.

Yes, will do.

> And it also helps in future to figure out what those other ones are
> when debugging or backporting.
> 
> See below.
> > 
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > 
> > ---
> > Changes in V4:
> > - add more BUG_ON into get_module_entry(). Check that modules paddr and
> >   size are 32bits.
> > 
> > Changes in V3:
> > - fix some codying style
> > - use module.cmdline to look for a module name instead of the main cmdline
> >   from hvm_start_info.
> > ---
> >  tools/firmware/hvmloader/config.h    |  2 +-
> >  tools/firmware/hvmloader/hvmloader.c | 42 ++++++++++++++++++++++++++++++++++--
> >  tools/firmware/hvmloader/ovmf.c      |  3 ++-
> >  tools/firmware/hvmloader/rombios.c   |  3 ++-
> >  tools/firmware/hvmloader/util.h      |  2 ++
> >  5 files changed, 47 insertions(+), 5 deletions(-)
> > 
> > diff --git a/tools/firmware/hvmloader/config.h b/tools/firmware/hvmloader/config.h
> > index b838cf9..4c6d8ad 100644
> > --- a/tools/firmware/hvmloader/config.h
> > +++ b/tools/firmware/hvmloader/config.h
> > @@ -22,7 +22,7 @@ struct bios_config {
> >      /* ROMS */
> >      void (*load_roms)(void);
> >  
> > -    void (*bios_load)(const struct bios_config *config);
> > +    void (*bios_load)(const struct bios_config *config, void *addr, uint32_t size);
> >  
> >      void (*bios_info_setup)(void);
> >      void (*bios_info_finish)(void);
> > diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c
> > index c45f367..460efb9 100644
> > --- a/tools/firmware/hvmloader/hvmloader.c
> > +++ b/tools/firmware/hvmloader/hvmloader.c
> > @@ -253,10 +253,40 @@ static void acpi_enable_sci(void)
> >      BUG_ON(!(pm1a_cnt_val & ACPI_PM1C_SCI_EN));
> >  }
> >  
> > +const struct hvm_modlist_entry *get_module_entry(
> > +    const struct hvm_start_info *info,
> > +    const char *name)
> > +{
> > +    const struct hvm_modlist_entry *modlist =
> > +        (struct hvm_modlist_entry *)info->modlist_paddr;
> > +    unsigned int i;
> > +
> > +    if ( !modlist )
> > +        return NULL;
> > +
> > +    for ( i = 0; i < info->nr_modules; i++ )
> > +    {
> > +        uint32_t module_name = modlist[i].cmdline_paddr;
> > +
> > +        BUG_ON(!modlist[i].cmdline_paddr ||
> 
> Having an zero value in cmdline_paddr is OK.
> 
> The spec says:
> 
> 803  * NOTE: nothing will be loaded at physical address 0, so a 0 value in any
> 804  * of the address fields should be treated as not present.
> 805  *
> 
> > +               modlist[i].cmdline_paddr > UINT_MAX);
> > +
> > +        if ( !strcmp(name, (char*)module_name) )
> 
> Which could result in looking up a value at 0 address. 
> 
> Perhaps change your code to:
> 	if ( !modlist[i].cmdline_paddr )
> 		continue;
> ?

I'll do that. I'm using to many BUG_ON in this code.

> > +        {
> > +            BUG_ON(!modlist[i].paddr || modlist[i].paddr > UINT_MAX ||
> > +                   modlist[i].size > UINT_MAX);
> 
> To be fair all of those values are in spec..Perhaps you should mention
> that the libxc code would never put those there and neermind the spec?

Yes, there is just this mention in the spec: "However Xen will always try
to place all modules below the 4GiB boundary."

But it actualy would be a bug if the blob would be abrove 4GiB, since
hvmloader can not read that far, right? I can add a comment.
Konrad Rzeszutek Wilk March 17, 2016, 5:57 p.m. UTC | #3
> I'll do that. I'm using to many BUG_ON in this code.
> 
> > > +        {
> > > +            BUG_ON(!modlist[i].paddr || modlist[i].paddr > UINT_MAX ||
> > > +                   modlist[i].size > UINT_MAX);
> > 
> > To be fair all of those values are in spec..Perhaps you should mention
> > that the libxc code would never put those there and neermind the spec?
> 
> Yes, there is just this mention in the spec: "However Xen will always try
> to place all modules below the 4GiB boundary."
> 
> But it actualy would be a bug if the blob would be abrove 4GiB, since
> hvmloader can not read that far, right? I can add a comment.

Well no. It says that the hypervisor will 'try', not that it 'MUST'.
Jan Beulich March 18, 2016, 7:34 a.m. UTC | #4
>>> On 17.03.16 at 18:46, <anthony.perard@citrix.com> wrote:
> On Tue, Mar 15, 2016 at 09:14:17PM -0400, Konrad Rzeszutek Wilk wrote:
>> On Mon, Mar 14, 2016 at 05:55:43PM +0000, Anthony PERARD wrote:
>> > The BIOS can be found an entry called "bios" of the modlist of the
>> 
>> s/BIOS/BIOS blob/
>> > hvm_start_info struct.
>> > 
>> > The found BIOS blob is not loaded by this patch, but only passed as
>> > argument to bios_load() function. It is going to be used by the next few
>> > patches.
>> 
>> Please list the 'few patches' by name as this patch and others may not
>> always be committed in order.
> 
> Yes, will do.

Or maybe just s/next few/subsequent/ or some such? Listing
future dependencies in a too explicit manner may also not be
a good idea - who knows if their titles change by the time they
go in?

Jan
Jan Beulich April 5, 2016, 12:59 p.m. UTC | #5
>>> On 14.03.16 at 18:55, <anthony.perard@citrix.com> wrote:
> --- a/tools/firmware/hvmloader/hvmloader.c
> +++ b/tools/firmware/hvmloader/hvmloader.c
> @@ -253,10 +253,40 @@ static void acpi_enable_sci(void)
>      BUG_ON(!(pm1a_cnt_val & ACPI_PM1C_SCI_EN));
>  }
>  
> +const struct hvm_modlist_entry *get_module_entry(
> +    const struct hvm_start_info *info,
> +    const char *name)
> +{
> +    const struct hvm_modlist_entry *modlist =
> +        (struct hvm_modlist_entry *)info->modlist_paddr;

This cast puzzles me (as at the first glance I would expect it to
cause a compiler warning): Roger, how come cmdline_paddr,
modlist_paddr, and rsdp_paddr are 32-bit quantities? While on
x86 that _may_ be fine, what about other architectures we may
want to run Xen on?

> +    unsigned int i;
> +
> +    if ( !modlist )
> +        return NULL;
> +
> +    for ( i = 0; i < info->nr_modules; i++ )
> +    {
> +        uint32_t module_name = modlist[i].cmdline_paddr;
> +
> +        BUG_ON(!modlist[i].cmdline_paddr ||
> +               modlist[i].cmdline_paddr > UINT_MAX);

While you can't use the local variable for the latter check, you can
for the former.

> @@ -292,8 +322,16 @@ int main(void)
>      }
>  
>      printf("Loading %s ...\n", bios->name);
> -    if ( bios->bios_load )
> -        bios->bios_load(bios);
> +    bios_module = get_module_entry(hvm_start_info, "bios");

Isn't "bios" a bit vague, as there could be multiple (system, video,
add-on card)?

> +    if ( bios_module && bios->bios_load )
> +    {
> +        uint32_t paddr = bios_module->paddr;
> +        bios->bios_load(bios, (void*)paddr, bios_module->size);

Blank line between declaration(s) and statement(s) please.

> +    }
> +    else if ( bios->bios_load )
> +    {
> +        bios->bios_load(bios, 0, 0);

        bios->bios_load(bios, NULL, 0);

Jan
Roger Pau Monné April 5, 2016, 2:05 p.m. UTC | #6
On Tue, 5 Apr 2016, Jan Beulich wrote:
> >>> On 14.03.16 at 18:55, <anthony.perard@citrix.com> wrote:
> > --- a/tools/firmware/hvmloader/hvmloader.c
> > +++ b/tools/firmware/hvmloader/hvmloader.c
> > @@ -253,10 +253,40 @@ static void acpi_enable_sci(void)
> >      BUG_ON(!(pm1a_cnt_val & ACPI_PM1C_SCI_EN));
> >  }
> >  
> > +const struct hvm_modlist_entry *get_module_entry(
> > +    const struct hvm_start_info *info,
> > +    const char *name)
> > +{
> > +    const struct hvm_modlist_entry *modlist =
> > +        (struct hvm_modlist_entry *)info->modlist_paddr;
> 
> This cast puzzles me (as at the first glance I would expect it to
> cause a compiler warning): Roger, how come cmdline_paddr,
> modlist_paddr, and rsdp_paddr are 32-bit quantities? While on
> x86 that _may_ be fine, what about other architectures we may
> want to run Xen on?

I've always considered this protocol x86 specific TBH, and since guests 
are started in 32bit mode I have always considered mandatory to have all 
this information available below the 4GiB boundary.

I don't mind changing the sizes to be 64bits, it's a fairly easy change 
that could be done before the release.

Roger.
Jan Beulich April 5, 2016, 2:23 p.m. UTC | #7
>>> On 05.04.16 at 16:05, <roger.pau@citrix.com> wrote:
> On Tue, 5 Apr 2016, Jan Beulich wrote:
>> >>> On 14.03.16 at 18:55, <anthony.perard@citrix.com> wrote:
>> > --- a/tools/firmware/hvmloader/hvmloader.c
>> > +++ b/tools/firmware/hvmloader/hvmloader.c
>> > @@ -253,10 +253,40 @@ static void acpi_enable_sci(void)
>> >      BUG_ON(!(pm1a_cnt_val & ACPI_PM1C_SCI_EN));
>> >  }
>> >  
>> > +const struct hvm_modlist_entry *get_module_entry(
>> > +    const struct hvm_start_info *info,
>> > +    const char *name)
>> > +{
>> > +    const struct hvm_modlist_entry *modlist =
>> > +        (struct hvm_modlist_entry *)info->modlist_paddr;
>> 
>> This cast puzzles me (as at the first glance I would expect it to
>> cause a compiler warning): Roger, how come cmdline_paddr,
>> modlist_paddr, and rsdp_paddr are 32-bit quantities? While on
>> x86 that _may_ be fine, what about other architectures we may
>> want to run Xen on?
> 
> I've always considered this protocol x86 specific TBH, and since guests 
> are started in 32bit mode I have always considered mandatory to have all 
> this information available below the 4GiB boundary.

I could live with that, but then this should be made explicit, rather
than depending on only x86 defining XEN_HAVE_PV_GUEST_ENTRY.

And if that was to be done, then the other 64-bit address changes
likely could be undone again, too. We really (just) need to settle on
the intended scope of the interface, and then it should be made
internally consistent.

Jan
Anthony PERARD April 7, 2016, 3:10 p.m. UTC | #8
On Tue, Apr 05, 2016 at 06:59:03AM -0600, Jan Beulich wrote:
> >>> On 14.03.16 at 18:55, <anthony.perard@citrix.com> wrote:
> > --- a/tools/firmware/hvmloader/hvmloader.c
> > +++ b/tools/firmware/hvmloader/hvmloader.c
> > @@ -292,8 +322,16 @@ int main(void)
> >      }
> >  
> >      printf("Loading %s ...\n", bios->name);
> > -    if ( bios->bios_load )
> > -        bios->bios_load(bios);
> > +    bios_module = get_module_entry(hvm_start_info, "bios");
> 
> Isn't "bios" a bit vague, as there could be multiple (system, video,
> add-on card)?

Maybe a bit, also this can be use to load OVMF which is not technically a
BIOS. I guess a better name would be "firmware" or "system firmware".

On the other hand, the name "bios" is used in few places, in hvmloader, in
libxl (to choose between seabios/ovmf).
Jan Beulich April 7, 2016, 3:30 p.m. UTC | #9
>>> On 07.04.16 at 17:10, <anthony.perard@citrix.com> wrote:
> On Tue, Apr 05, 2016 at 06:59:03AM -0600, Jan Beulich wrote:
>> >>> On 14.03.16 at 18:55, <anthony.perard@citrix.com> wrote:
>> > --- a/tools/firmware/hvmloader/hvmloader.c
>> > +++ b/tools/firmware/hvmloader/hvmloader.c
>> > @@ -292,8 +322,16 @@ int main(void)
>> >      }
>> >  
>> >      printf("Loading %s ...\n", bios->name);
>> > -    if ( bios->bios_load )
>> > -        bios->bios_load(bios);
>> > +    bios_module = get_module_entry(hvm_start_info, "bios");
>> 
>> Isn't "bios" a bit vague, as there could be multiple (system, video,
>> add-on card)?
> 
> Maybe a bit, also this can be use to load OVMF which is not technically a
> BIOS. I guess a better name would be "firmware" or "system firmware".
> 
> On the other hand, the name "bios" is used in few places, in hvmloader, in
> libxl (to choose between seabios/ovmf).

Perhaps that's a result of how that code evolved? I'm not heavily
objecting to using "bios" here, but I think this being a newly
introduced name it could as well be made more correct.

Jan
diff mbox

Patch

diff --git a/tools/firmware/hvmloader/config.h b/tools/firmware/hvmloader/config.h
index b838cf9..4c6d8ad 100644
--- a/tools/firmware/hvmloader/config.h
+++ b/tools/firmware/hvmloader/config.h
@@ -22,7 +22,7 @@  struct bios_config {
     /* ROMS */
     void (*load_roms)(void);
 
-    void (*bios_load)(const struct bios_config *config);
+    void (*bios_load)(const struct bios_config *config, void *addr, uint32_t size);
 
     void (*bios_info_setup)(void);
     void (*bios_info_finish)(void);
diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c
index c45f367..460efb9 100644
--- a/tools/firmware/hvmloader/hvmloader.c
+++ b/tools/firmware/hvmloader/hvmloader.c
@@ -253,10 +253,40 @@  static void acpi_enable_sci(void)
     BUG_ON(!(pm1a_cnt_val & ACPI_PM1C_SCI_EN));
 }
 
+const struct hvm_modlist_entry *get_module_entry(
+    const struct hvm_start_info *info,
+    const char *name)
+{
+    const struct hvm_modlist_entry *modlist =
+        (struct hvm_modlist_entry *)info->modlist_paddr;
+    unsigned int i;
+
+    if ( !modlist )
+        return NULL;
+
+    for ( i = 0; i < info->nr_modules; i++ )
+    {
+        uint32_t module_name = modlist[i].cmdline_paddr;
+
+        BUG_ON(!modlist[i].cmdline_paddr ||
+               modlist[i].cmdline_paddr > UINT_MAX);
+
+        if ( !strcmp(name, (char*)module_name) )
+        {
+            BUG_ON(!modlist[i].paddr || modlist[i].paddr > UINT_MAX ||
+                   modlist[i].size > UINT_MAX);
+            return &modlist[i];
+        }
+    }
+
+    return NULL;
+}
+
 int main(void)
 {
     const struct bios_config *bios;
     int acpi_enabled;
+    const struct hvm_modlist_entry *bios_module;
 
     /* Initialise hypercall stubs with RET, rendering them no-ops. */
     memset((void *)HYPERCALL_PHYSICAL_ADDRESS, 0xc3 /* RET */, PAGE_SIZE);
@@ -292,8 +322,16 @@  int main(void)
     }
 
     printf("Loading %s ...\n", bios->name);
-    if ( bios->bios_load )
-        bios->bios_load(bios);
+    bios_module = get_module_entry(hvm_start_info, "bios");
+    if ( bios_module && bios->bios_load )
+    {
+        uint32_t paddr = bios_module->paddr;
+        bios->bios_load(bios, (void*)paddr, bios_module->size);
+    }
+    else if ( bios->bios_load )
+    {
+        bios->bios_load(bios, 0, 0);
+    }
     else
     {
         BUG_ON(bios->bios_address + bios->image_size >
diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
index db9fa7a..858a2d4 100644
--- a/tools/firmware/hvmloader/ovmf.c
+++ b/tools/firmware/hvmloader/ovmf.c
@@ -93,7 +93,8 @@  static void ovmf_finish_bios_info(void)
     info->checksum = -checksum;
 }
 
-static void ovmf_load(const struct bios_config *config)
+static void ovmf_load(const struct bios_config *config,
+                      void *bios_addr, uint32_t bios_length)
 {
     xen_pfn_t mfn;
     uint64_t addr = OVMF_BEGIN;
diff --git a/tools/firmware/hvmloader/rombios.c b/tools/firmware/hvmloader/rombios.c
index 1f15b94..2ded844 100644
--- a/tools/firmware/hvmloader/rombios.c
+++ b/tools/firmware/hvmloader/rombios.c
@@ -121,7 +121,8 @@  static void rombios_load_roms(void)
                option_rom_phys_addr + option_rom_sz - 1);
 }
 
-static void rombios_load(const struct bios_config *config)
+static void rombios_load(const struct bios_config *config,
+                         void *unused_addr, uint32_t unused_size)
 {
     uint32_t bioshigh;
     struct rombios_info *info;
diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
index 9808016..003dc71 100644
--- a/tools/firmware/hvmloader/util.h
+++ b/tools/firmware/hvmloader/util.h
@@ -34,6 +34,8 @@  enum {
 #undef NULL
 #define NULL ((void*)0)
 
+#define UINT_MAX (~0U)
+
 void __assert_failed(char *assertion, char *file, int line)
     __attribute__((noreturn));
 #define ASSERT(p) \