| Message ID | 1457978150-27201-9-git-send-email-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
> 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'.
>>> 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
>>> 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
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.
>>> 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
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).
>>> 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 --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) \
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(-)