| Message ID | 20160728105013.22310-9-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 28/07/16 11:50, Anthony PERARD wrote: > @@ -293,8 +340,17 @@ 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, "firmware"); > + 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, NULL, 0); This is an unnecessary change in behaviour. Currently, 'bios' is never NULL, and would never pass bogus information to bios_load. As this is the new way of providing firmware, it should be a hard error if get_module_entry(, "firmware") fails, at which point this logic can collapse down quite a lot. ~Andrew > + } > else > { > BUG_ON(bios->bios_address + bios->image_size > >
On Thu, Jul 28, 2016 at 02:44:24PM +0100, Andrew Cooper wrote: > On 28/07/16 11:50, Anthony PERARD wrote: > > @@ -293,8 +340,17 @@ 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, "firmware"); > > + 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, NULL, 0); > > This is an unnecessary change in behaviour. Currently, 'bios' is never > NULL, and would never pass bogus information to bios_load. > > As this is the new way of providing firmware, it should be a hard error > if get_module_entry(, "firmware") fails, at which point this logic can > collapse down quite a lot. At this point in the patch series, the module is not used yet, and the seabios loader does not have a bios_load function. Also I've change the logic again in "hvmloader: bios->bios_load() now needs to be defined". Also, I've left ROMBIOS embedded in hvmloader, because it comes with VGABIOS and Etherboot, so it would be a bit more complicated.
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 77c70e0..5c57ab3 100644 --- a/tools/firmware/hvmloader/hvmloader.c +++ b/tools/firmware/hvmloader/hvmloader.c @@ -254,10 +254,57 @@ 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 *)(uintptr_t)info->modlist_paddr; + unsigned int i; + + if ( !modlist || + info->modlist_paddr > UINTPTR_MAX || + (info->modlist_paddr + info->nr_modules * sizeof(*modlist) - 1) + > UINTPTR_MAX + ) + return NULL; + + for ( i = 0; i < info->nr_modules; i++ ) + { + char *module_name = (char*)(uintptr_t)modlist[i].cmdline_paddr; + + /* Skip if the module or its cmdline is missing. */ + if ( !module_name || !modlist[i].paddr ) + continue; + + /* Skip if the cmdline can not be read. */ + if ( modlist[i].cmdline_paddr > UINTPTR_MAX || + (modlist[i].cmdline_paddr + strlen(name)) > UINTPTR_MAX ) + continue; + + if ( !strcmp(name, module_name) ) + { + if ( modlist[i].paddr > UINTPTR_MAX || + modlist[i].size > UINTPTR_MAX || + (modlist[i].paddr + modlist[i].size - 1) > UINTPTR_MAX ) + { + printf("Can not load \"%s\" from 0x"PRIllx" (0x"PRIllx")\n", + name, PRIllx_arg(modlist[i].paddr), + PRIllx_arg(modlist[i].size)); + BUG(); + } + 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); @@ -293,8 +340,17 @@ 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, "firmware"); + 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, NULL, 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;
The BIOS blob can be found an entry called "firmware" 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. Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- Changes in V6: - cast addresses to uintptr_t instead of uint32_t. - use UINTPTR_MAX for the upper boundary checks. - Do a full check of every things that are used, check that modlist, cmdlines, modules lives below 4GB and does not cross the boundary. Changes in V5: - don't BUG() on module's paddr having value 0, and just skip. - fix some coding style - rename module name to "firmware" (was "bios") - less use of BUG_ON in get_module_entry() and skip entries instead. Only BUG() if the module which match name is not accessible. 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 | 60 ++++++++++++++++++++++++++++++++++-- tools/firmware/hvmloader/ovmf.c | 3 +- tools/firmware/hvmloader/rombios.c | 3 +- 4 files changed, 63 insertions(+), 5 deletions(-)