Re: [Xen-devel] [PATCH v4 08/14] hvmloader: Locate the BIOS blob

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

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@xxxxxxxxxx>
> 
> ---
> 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@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel