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

[Anthony PERARD <anthony.perard@citrix.com>]

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.

-- 
Anthony PERARD

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