| Message ID | 1457978150-27201-3-git-send-email-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 14, 2016 at 05:55:37PM +0000, Anthony PERARD wrote: > ... and load BIOS into guest memory. > > This adds a new firmware module, bios_module. It is > loaded in the guest memory and final location is provided to hvmloader > via the hvm_start_info struct. > > This patch create the hvm_start_info struct for HVM guest that have a > device model, so this is now common code with HVM guest without device > model. CC-ing Boris and Roger since this impact the PVH code. And therefore not snipping the code so they can see it in its full glory. > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > --- > Change in V4: > - change title to suggest the change of beavior > - remove code to load acpi tables (dsdt) > - Update public/xen.h about hvm_start_info available on other HVM guest > in %ebx. > > Change in V3: > - rename acpi_table_module to full_acpi_module. > - factorise module loading, using new function to load existing optinal > module, this should not change anything > - should now use the same code to loads modules as for HVMlite VMs. > this avoid duplication of code. > - no more generic cmdline with a list of modules, each module have its name > in the module specific cmdline. > - scope change for common code between hvmlite and hvmloader > --- > tools/libxc/include/xc_dom.h | 3 + > tools/libxc/xc_dom_hvmloader.c | 2 + > tools/libxc/xc_dom_x86.c | 132 ++++++++++++++++++++++++++++------------- > xen/include/public/xen.h | 2 +- > 4 files changed, 96 insertions(+), 43 deletions(-) > > diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h > index 6ebe946..93f894c 100644 > --- a/tools/libxc/include/xc_dom.h > +++ b/tools/libxc/include/xc_dom.h > @@ -209,6 +209,9 @@ struct xc_dom_image { > /* If unset disables the setup of the IOREQ pages. */ > bool device_model; > > + /* BIOS passed to HVMLOADER */ > + struct xc_hvm_firmware_module bios_module; > + > /* Extra ACPI tables passed to HVMLOADER */ > struct xc_hvm_firmware_module acpi_module; > > diff --git a/tools/libxc/xc_dom_hvmloader.c b/tools/libxc/xc_dom_hvmloader.c > index 7cf5854..606351a 100644 > --- a/tools/libxc/xc_dom_hvmloader.c > +++ b/tools/libxc/xc_dom_hvmloader.c > @@ -167,6 +167,8 @@ static int modules_init(struct xc_dom_image *dom) > { > int rc; > > + rc = module_init_one(dom, &dom->bios_module, "bios module"); > + if ( rc ) goto err; > rc = module_init_one(dom, &dom->acpi_module, "acpi module"); > if ( rc ) goto err; > rc = module_init_one(dom, &dom->smbios_module, "smbios module"); > diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c > index bdec40a..9c56d55 100644 > --- a/tools/libxc/xc_dom_x86.c > +++ b/tools/libxc/xc_dom_x86.c > @@ -69,6 +69,9 @@ > #define round_up(addr, mask) ((addr) | (mask)) > #define round_pg_up(addr) (((addr) + PAGE_SIZE_X86 - 1) & ~(PAGE_SIZE_X86 - 1)) > > +#define HVMLOADER_MODULE_MAX_COUNT 1 > +#define HVMLOADER_MODULE_NAME_SIZE 10 > + > struct xc_dom_params { > unsigned levels; > xen_vaddr_t vaddr_mask; > @@ -590,6 +593,7 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom) > xen_pfn_t special_array[X86_HVM_NR_SPECIAL_PAGES]; > xen_pfn_t ioreq_server_array[NR_IOREQ_SERVER_PAGES]; > xc_interface *xch = dom->xch; > + size_t start_info_size = sizeof(struct hvm_start_info); > > /* Allocate and clear special pages. */ > for ( i = 0; i < X86_HVM_NR_SPECIAL_PAGES; i++ ) > @@ -624,8 +628,6 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom) > > if ( !dom->device_model ) > { > - size_t start_info_size = sizeof(struct hvm_start_info); > - > if ( dom->cmdline ) > { > dom->cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 8); > @@ -635,17 +637,26 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom) > /* Limited to one module. */ > if ( dom->ramdisk_blob ) > start_info_size += sizeof(struct hvm_modlist_entry); > - > - rc = xc_dom_alloc_segment(dom, &dom->start_info_seg, > - "HVMlite start info", 0, start_info_size); > - if ( rc != 0 ) > - { > - DOMPRINTF("Unable to reserve memory for the start info"); > - goto out; > - } > } > else > { > + start_info_size += > + sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT; > + /* Add extra space to write modules name */ > + start_info_size += > + HVMLOADER_MODULE_NAME_SIZE * HVMLOADER_MODULE_MAX_COUNT; What about \0 ? Ah, the strncpy we use adds \0 byte. But it would be nice to mention that somewhere. Perhaps mention: The HVMLOADER_MODULE_NAME_SIZE accounts for NUL byte? > + } > + > + rc = xc_dom_alloc_segment(dom, &dom->start_info_seg, > + "HVMlite start info", 0, start_info_size); > + if ( rc != 0 ) > + { > + DOMPRINTF("Unable to reserve memory for the start info"); > + goto out; > + } > + > + if ( dom->device_model ) > + { > /* > * Allocate and clear additional ioreq server pages. The default > * server will use the IOREQ and BUFIOREQ special pages above. > @@ -1696,39 +1707,68 @@ static int alloc_pgtables_hvm(struct xc_dom_image *dom) > return 0; > } > > +static void add_module_to_list(struct xc_dom_image *dom, > + struct xc_hvm_firmware_module *module, > + const char *name, > + struct hvm_modlist_entry *modlist, > + struct hvm_start_info *start_info) > +{ > + uint32_t index = start_info->nr_modules; > + if ( module->length == 0 ) > + return; > + > + assert(start_info->nr_modules < HVMLOADER_MODULE_MAX_COUNT); > + assert(strnlen(name, HVMLOADER_MODULE_NAME_SIZE) > + < HVMLOADER_MODULE_NAME_SIZE); > + > + modlist[index].paddr = module->guest_addr_out; > + modlist[index].size = module->length; > + strncpy((char*)(modlist + HVMLOADER_MODULE_MAX_COUNT) > + + HVMLOADER_MODULE_NAME_SIZE * index, > + name, HVMLOADER_MODULE_NAME_SIZE); > + modlist[index].cmdline_paddr = > + (dom->start_info_seg.pfn << PAGE_SHIFT) + > + ((uintptr_t)modlist - (uintptr_t)start_info) + > + sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT + > + HVMLOADER_MODULE_NAME_SIZE * index; That looks right, but boy it takes a bit of thinking to make sure it is right. Perhaps put a comment outlining where it ought to be (so folks reading the first time can feel OK they got it right?) > + > + start_info->nr_modules++; > +} > + > static int bootlate_hvm(struct xc_dom_image *dom) > { > uint32_t domid = dom->guest_domid; > xc_interface *xch = dom->xch; > + struct hvm_start_info *start_info; > + size_t start_info_size; > + void *start_page; > + struct hvm_modlist_entry *modlist; > > - if ( !dom->device_model ) > - { > - struct hvm_start_info *start_info; > - size_t start_info_size; > - void *start_page; > - > - start_info_size = sizeof(*start_info) + dom->cmdline_size; > - if ( dom->ramdisk_blob ) > - start_info_size += sizeof(struct hvm_modlist_entry); > + start_info_size = sizeof(*start_info) + dom->cmdline_size; > + if ( dom->ramdisk_blob ) > + start_info_size += sizeof(struct hvm_modlist_entry); > > - if ( start_info_size > > - dom->start_info_seg.pages << XC_DOM_PAGE_SHIFT(dom) ) > - { > - DOMPRINTF("Trying to map beyond start_info_seg"); > - return -1; > - } > + if ( start_info_size > > + dom->start_info_seg.pages << XC_DOM_PAGE_SHIFT(dom) ) > + { > + DOMPRINTF("Trying to map beyond start_info_seg"); > + return -1; > + } > > - start_page = xc_map_foreign_range(xch, domid, start_info_size, > - PROT_READ | PROT_WRITE, > - dom->start_info_seg.pfn); > - if ( start_page == NULL ) > - { > - DOMPRINTF("Unable to map HVM start info page"); > - return -1; > - } > + start_page = xc_map_foreign_range(xch, domid, start_info_size, > + PROT_READ | PROT_WRITE, > + dom->start_info_seg.pfn); > + if ( start_page == NULL ) > + { > + DOMPRINTF("Unable to map HVM start info page"); > + return -1; > + } > > - start_info = start_page; > + start_info = start_page; > + modlist = start_page + sizeof(*start_info) + dom->cmdline_size; > > + if ( !dom->device_model ) > + { > if ( dom->cmdline ) > { > char *cmdline = start_page + sizeof(*start_info); > @@ -1740,22 +1780,30 @@ static int bootlate_hvm(struct xc_dom_image *dom) > > if ( dom->ramdisk_blob ) > { > - struct hvm_modlist_entry *modlist = > - start_page + sizeof(*start_info) + dom->cmdline_size; > > modlist[0].paddr = dom->ramdisk_seg.vstart - dom->parms.virt_base; > modlist[0].size = dom->ramdisk_seg.vend - dom->ramdisk_seg.vstart; > - start_info->modlist_paddr = (dom->start_info_seg.pfn << PAGE_SHIFT) + > - ((uintptr_t)modlist - (uintptr_t)start_info); > start_info->nr_modules = 1; > } > - > - start_info->magic = XEN_HVM_START_MAGIC_VALUE; > - > - munmap(start_page, start_info_size); > } > else > { > + add_module_to_list(dom, &dom->bios_module, "bios", > + modlist, start_info); > + } > + > + if ( start_info->nr_modules ) > + { > + start_info->modlist_paddr = (dom->start_info_seg.pfn << PAGE_SHIFT) + > + ((uintptr_t)modlist - (uintptr_t)start_info); > + } > + > + start_info->magic = XEN_HVM_START_MAGIC_VALUE; > + > + munmap(start_page, start_info_size); > + > + if ( dom->device_model ) > + { > void *hvm_info_page; > > if ( (hvm_info_page = xc_map_foreign_range( > diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h > index 64ba7ab..1cfec5c 100644 > --- a/xen/include/public/xen.h > +++ b/xen/include/public/xen.h > @@ -798,7 +798,7 @@ struct start_info { > typedef struct start_info start_info_t; > > /* > - * Start of day structure passed to PVH guests in %ebx. > + * Start of day structure passed to PVH guests and to HVM guests in %ebx. > * > * NOTE: nothing will be loaded at physical address 0, so a 0 value in any > * of the address fields should be treated as not present. > -- > Anthony PERARD > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
On 03/15/2016 08:18 PM, Konrad Rzeszutek Wilk wrote: > On Mon, Mar 14, 2016 at 05:55:37PM +0000, Anthony PERARD wrote: > >> @@ -624,8 +628,6 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom) >> >> if ( !dom->device_model ) >> { >> - size_t start_info_size = sizeof(struct hvm_start_info); >> - >> if ( dom->cmdline ) >> { >> dom->cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 8); >> @@ -635,17 +637,26 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom) >> /* Limited to one module. */ >> if ( dom->ramdisk_blob ) >> start_info_size += sizeof(struct hvm_modlist_entry); >> - >> - rc = xc_dom_alloc_segment(dom, &dom->start_info_seg, >> - "HVMlite start info", 0, start_info_size); >> - if ( rc != 0 ) >> - { >> - DOMPRINTF("Unable to reserve memory for the start info"); >> - goto out; >> - } >> } >> else >> { >> + start_info_size += >> + sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT; >> + /* Add extra space to write modules name */ >> + start_info_size += >> + HVMLOADER_MODULE_NAME_SIZE * HVMLOADER_MODULE_MAX_COUNT; > What about \0 ? Ah, the strncpy we use adds \0 byte. But it would be nice > to mention that somewhere. Perhaps mention: > > The HVMLOADER_MODULE_NAME_SIZE accounts for NUL byte? > >> + } >> + >> + rc = xc_dom_alloc_segment(dom, &dom->start_info_seg, >> + "HVMlite start info", 0, start_info_size); >> + if ( rc != 0 ) >> + { >> + DOMPRINTF("Unable to reserve memory for the start info"); >> + goto out; >> + } >> + >> + if ( dom->device_model ) >> + { Can you fold this into the 'else' clause above and move xc_dom_alloc_segment() down? >> + >> static int bootlate_hvm(struct xc_dom_image *dom) >> { >> uint32_t domid = dom->guest_domid; >> xc_interface *xch = dom->xch; >> + struct hvm_start_info *start_info; >> + size_t start_info_size; >> + void *start_page; >> + struct hvm_modlist_entry *modlist; >> >> - if ( !dom->device_model ) >> - { >> - struct hvm_start_info *start_info; >> - size_t start_info_size; >> - void *start_page; >> - >> - start_info_size = sizeof(*start_info) + dom->cmdline_size; >> - if ( dom->ramdisk_blob ) >> - start_info_size += sizeof(struct hvm_modlist_entry); >> + start_info_size = sizeof(*start_info) + dom->cmdline_size; >> + if ( dom->ramdisk_blob ) >> + start_info_size += sizeof(struct hvm_modlist_entry); >> >> - if ( start_info_size > >> - dom->start_info_seg.pages << XC_DOM_PAGE_SHIFT(dom) ) >> - { >> - DOMPRINTF("Trying to map beyond start_info_seg"); >> - return -1; >> - } >> + if ( start_info_size > >> + dom->start_info_seg.pages << XC_DOM_PAGE_SHIFT(dom) ) >> + { >> + DOMPRINTF("Trying to map beyond start_info_seg"); >> + return -1; >> + } >> >> - start_page = xc_map_foreign_range(xch, domid, start_info_size, >> - PROT_READ | PROT_WRITE, >> - dom->start_info_seg.pfn); >> - if ( start_page == NULL ) >> - { >> - DOMPRINTF("Unable to map HVM start info page"); >> - return -1; >> - } >> + start_page = xc_map_foreign_range(xch, domid, start_info_size, >> + PROT_READ | PROT_WRITE, >> + dom->start_info_seg.pfn); >> + if ( start_page == NULL ) >> + { >> + DOMPRINTF("Unable to map HVM start info page"); >> + return -1; >> + } >> >> - start_info = start_page; >> + start_info = start_page; >> + modlist = start_page + sizeof(*start_info) + dom->cmdline_size; I think we can drop start_page and use start_info only. They are the same, aren't they? -boris
> > diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c > > index bdec40a..9c56d55 100644 > > --- a/tools/libxc/xc_dom_x86.c > > +++ b/tools/libxc/xc_dom_x86.c > > @@ -69,6 +69,9 @@ > > #define round_up(addr, mask) ((addr) | (mask)) > > #define round_pg_up(addr) (((addr) + PAGE_SIZE_X86 - 1) & ~(PAGE_SIZE_X86 - 1)) > > > > +#define HVMLOADER_MODULE_MAX_COUNT 1 > > +#define HVMLOADER_MODULE_NAME_SIZE 10 > > + > > struct xc_dom_params { > > unsigned levels; > > xen_vaddr_t vaddr_mask; > > @@ -590,6 +593,7 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom) > > xen_pfn_t special_array[X86_HVM_NR_SPECIAL_PAGES]; > > xen_pfn_t ioreq_server_array[NR_IOREQ_SERVER_PAGES]; > > xc_interface *xch = dom->xch; > > + size_t start_info_size = sizeof(struct hvm_start_info); > > > > /* Allocate and clear special pages. */ > > for ( i = 0; i < X86_HVM_NR_SPECIAL_PAGES; i++ ) > > @@ -624,8 +628,6 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom) > > > > if ( !dom->device_model ) > > { > > - size_t start_info_size = sizeof(struct hvm_start_info); > > - > > if ( dom->cmdline ) > > { > > dom->cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 8); > > @@ -635,17 +637,26 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom) > > /* Limited to one module. */ > > if ( dom->ramdisk_blob ) > > start_info_size += sizeof(struct hvm_modlist_entry); > > - > > - rc = xc_dom_alloc_segment(dom, &dom->start_info_seg, > > - "HVMlite start info", 0, start_info_size); > > - if ( rc != 0 ) > > - { > > - DOMPRINTF("Unable to reserve memory for the start info"); > > - goto out; > > - } > > } > > else > > { > > + start_info_size += > > + sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT; > > + /* Add extra space to write modules name */ > > + start_info_size += > > + HVMLOADER_MODULE_NAME_SIZE * HVMLOADER_MODULE_MAX_COUNT; > > What about \0 ? Ah, the strncpy we use adds \0 byte. But it would be nice > to mention that somewhere. Perhaps mention: > > The HVMLOADER_MODULE_NAME_SIZE accounts for NUL byte? Yes, I can add a comment about it. > > + } > > + > > + rc = xc_dom_alloc_segment(dom, &dom->start_info_seg, > > + "HVMlite start info", 0, start_info_size); > > + if ( rc != 0 ) > > + { > > + DOMPRINTF("Unable to reserve memory for the start info"); > > + goto out; > > + } > > + > > + if ( dom->device_model ) > > + { > > /* > > * Allocate and clear additional ioreq server pages. The default > > * server will use the IOREQ and BUFIOREQ special pages above. > > @@ -1696,39 +1707,68 @@ static int alloc_pgtables_hvm(struct xc_dom_image *dom) > > return 0; > > } > > > > +static void add_module_to_list(struct xc_dom_image *dom, > > + struct xc_hvm_firmware_module *module, > > + const char *name, > > + struct hvm_modlist_entry *modlist, > > + struct hvm_start_info *start_info) > > +{ > > + uint32_t index = start_info->nr_modules; > > + if ( module->length == 0 ) > > + return; > > + > > + assert(start_info->nr_modules < HVMLOADER_MODULE_MAX_COUNT); > > + assert(strnlen(name, HVMLOADER_MODULE_NAME_SIZE) > > + < HVMLOADER_MODULE_NAME_SIZE); > > + > > + modlist[index].paddr = module->guest_addr_out; > > + modlist[index].size = module->length; > > + strncpy((char*)(modlist + HVMLOADER_MODULE_MAX_COUNT) > > + + HVMLOADER_MODULE_NAME_SIZE * index, > > + name, HVMLOADER_MODULE_NAME_SIZE); > > + modlist[index].cmdline_paddr = > > + (dom->start_info_seg.pfn << PAGE_SHIFT) + > > + ((uintptr_t)modlist - (uintptr_t)start_info) + > > + sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT + > > + HVMLOADER_MODULE_NAME_SIZE * index; > > That looks right, but boy it takes a bit of thinking to > make sure it is right. Perhaps put a comment outlining where > it ought to be (so folks reading the first time can feel > OK they got it right?) Yes, I can add comments, and maybe try to simplify it a bit.
On Wed, Mar 16, 2016 at 02:01:38PM -0400, Boris Ostrovsky wrote: > On 03/15/2016 08:18 PM, Konrad Rzeszutek Wilk wrote: > >On Mon, Mar 14, 2016 at 05:55:37PM +0000, Anthony PERARD wrote: > > > > > >>@@ -624,8 +628,6 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom) > >> if ( !dom->device_model ) > >> { > >>- size_t start_info_size = sizeof(struct hvm_start_info); > >>- > >> if ( dom->cmdline ) > >> { > >> dom->cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 8); > >>@@ -635,17 +637,26 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom) > >> /* Limited to one module. */ > >> if ( dom->ramdisk_blob ) > >> start_info_size += sizeof(struct hvm_modlist_entry); > >>- > >>- rc = xc_dom_alloc_segment(dom, &dom->start_info_seg, > >>- "HVMlite start info", 0, start_info_size); > >>- if ( rc != 0 ) > >>- { > >>- DOMPRINTF("Unable to reserve memory for the start info"); > >>- goto out; > >>- } > >> } > >> else > >> { > >>+ start_info_size += > >>+ sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT; > >>+ /* Add extra space to write modules name */ > >>+ start_info_size += > >>+ HVMLOADER_MODULE_NAME_SIZE * HVMLOADER_MODULE_MAX_COUNT; > >What about \0 ? Ah, the strncpy we use adds \0 byte. But it would be nice > >to mention that somewhere. Perhaps mention: > > > >The HVMLOADER_MODULE_NAME_SIZE accounts for NUL byte? > > > >>+ } > >>+ > >>+ rc = xc_dom_alloc_segment(dom, &dom->start_info_seg, > >>+ "HVMlite start info", 0, start_info_size); > >>+ if ( rc != 0 ) > >>+ { > >>+ DOMPRINTF("Unable to reserve memory for the start info"); > >>+ goto out; > >>+ } > >>+ > >>+ if ( dom->device_model ) > >>+ { > > > Can you fold this into the 'else' clause above and move > xc_dom_alloc_segment() down? Yes, I will. > >>+ > >> static int bootlate_hvm(struct xc_dom_image *dom) > >> { > >> uint32_t domid = dom->guest_domid; > >> xc_interface *xch = dom->xch; > >>+ struct hvm_start_info *start_info; > >>+ size_t start_info_size; > >>+ void *start_page; > >>+ struct hvm_modlist_entry *modlist; > >>- if ( !dom->device_model ) > >>- { > >>- struct hvm_start_info *start_info; > >>- size_t start_info_size; > >>- void *start_page; > >>- > >>- start_info_size = sizeof(*start_info) + dom->cmdline_size; > >>- if ( dom->ramdisk_blob ) > >>- start_info_size += sizeof(struct hvm_modlist_entry); > >>+ start_info_size = sizeof(*start_info) + dom->cmdline_size; > >>+ if ( dom->ramdisk_blob ) > >>+ start_info_size += sizeof(struct hvm_modlist_entry); > >>- if ( start_info_size > > >>- dom->start_info_seg.pages << XC_DOM_PAGE_SHIFT(dom) ) > >>- { > >>- DOMPRINTF("Trying to map beyond start_info_seg"); > >>- return -1; > >>- } > >>+ if ( start_info_size > > >>+ dom->start_info_seg.pages << XC_DOM_PAGE_SHIFT(dom) ) > >>+ { > >>+ DOMPRINTF("Trying to map beyond start_info_seg"); > >>+ return -1; > >>+ } > >>- start_page = xc_map_foreign_range(xch, domid, start_info_size, > >>- PROT_READ | PROT_WRITE, > >>- dom->start_info_seg.pfn); > >>- if ( start_page == NULL ) > >>- { > >>- DOMPRINTF("Unable to map HVM start info page"); > >>- return -1; > >>- } > >>+ start_page = xc_map_foreign_range(xch, domid, start_info_size, > >>+ PROT_READ | PROT_WRITE, > >>+ dom->start_info_seg.pfn); > >>+ if ( start_page == NULL ) > >>+ { > >>+ DOMPRINTF("Unable to map HVM start info page"); > >>+ return -1; > >>+ } > >>- start_info = start_page; > >>+ start_info = start_page; > >>+ modlist = start_page + sizeof(*start_info) + dom->cmdline_size; > > I think we can drop start_page and use start_info only. They are the same, > aren't they? Yes, there are pointer to the same part of memory, with different types. By droping start_page, I could replace few "start_page+sizeof(*start_info)" by "start_info + 1" :), and have to use a cast for the line abrove.
diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h index 6ebe946..93f894c 100644 --- a/tools/libxc/include/xc_dom.h +++ b/tools/libxc/include/xc_dom.h @@ -209,6 +209,9 @@ struct xc_dom_image { /* If unset disables the setup of the IOREQ pages. */ bool device_model; + /* BIOS passed to HVMLOADER */ + struct xc_hvm_firmware_module bios_module; + /* Extra ACPI tables passed to HVMLOADER */ struct xc_hvm_firmware_module acpi_module; diff --git a/tools/libxc/xc_dom_hvmloader.c b/tools/libxc/xc_dom_hvmloader.c index 7cf5854..606351a 100644 --- a/tools/libxc/xc_dom_hvmloader.c +++ b/tools/libxc/xc_dom_hvmloader.c @@ -167,6 +167,8 @@ static int modules_init(struct xc_dom_image *dom) { int rc; + rc = module_init_one(dom, &dom->bios_module, "bios module"); + if ( rc ) goto err; rc = module_init_one(dom, &dom->acpi_module, "acpi module"); if ( rc ) goto err; rc = module_init_one(dom, &dom->smbios_module, "smbios module"); diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c index bdec40a..9c56d55 100644 --- a/tools/libxc/xc_dom_x86.c +++ b/tools/libxc/xc_dom_x86.c @@ -69,6 +69,9 @@ #define round_up(addr, mask) ((addr) | (mask)) #define round_pg_up(addr) (((addr) + PAGE_SIZE_X86 - 1) & ~(PAGE_SIZE_X86 - 1)) +#define HVMLOADER_MODULE_MAX_COUNT 1 +#define HVMLOADER_MODULE_NAME_SIZE 10 + struct xc_dom_params { unsigned levels; xen_vaddr_t vaddr_mask; @@ -590,6 +593,7 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom) xen_pfn_t special_array[X86_HVM_NR_SPECIAL_PAGES]; xen_pfn_t ioreq_server_array[NR_IOREQ_SERVER_PAGES]; xc_interface *xch = dom->xch; + size_t start_info_size = sizeof(struct hvm_start_info); /* Allocate and clear special pages. */ for ( i = 0; i < X86_HVM_NR_SPECIAL_PAGES; i++ ) @@ -624,8 +628,6 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom) if ( !dom->device_model ) { - size_t start_info_size = sizeof(struct hvm_start_info); - if ( dom->cmdline ) { dom->cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 8); @@ -635,17 +637,26 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom) /* Limited to one module. */ if ( dom->ramdisk_blob ) start_info_size += sizeof(struct hvm_modlist_entry); - - rc = xc_dom_alloc_segment(dom, &dom->start_info_seg, - "HVMlite start info", 0, start_info_size); - if ( rc != 0 ) - { - DOMPRINTF("Unable to reserve memory for the start info"); - goto out; - } } else { + start_info_size += + sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT; + /* Add extra space to write modules name */ + start_info_size += + HVMLOADER_MODULE_NAME_SIZE * HVMLOADER_MODULE_MAX_COUNT; + } + + rc = xc_dom_alloc_segment(dom, &dom->start_info_seg, + "HVMlite start info", 0, start_info_size); + if ( rc != 0 ) + { + DOMPRINTF("Unable to reserve memory for the start info"); + goto out; + } + + if ( dom->device_model ) + { /* * Allocate and clear additional ioreq server pages. The default * server will use the IOREQ and BUFIOREQ special pages above. @@ -1696,39 +1707,68 @@ static int alloc_pgtables_hvm(struct xc_dom_image *dom) return 0; } +static void add_module_to_list(struct xc_dom_image *dom, + struct xc_hvm_firmware_module *module, + const char *name, + struct hvm_modlist_entry *modlist, + struct hvm_start_info *start_info) +{ + uint32_t index = start_info->nr_modules; + if ( module->length == 0 ) + return; + + assert(start_info->nr_modules < HVMLOADER_MODULE_MAX_COUNT); + assert(strnlen(name, HVMLOADER_MODULE_NAME_SIZE) + < HVMLOADER_MODULE_NAME_SIZE); + + modlist[index].paddr = module->guest_addr_out; + modlist[index].size = module->length; + strncpy((char*)(modlist + HVMLOADER_MODULE_MAX_COUNT) + + HVMLOADER_MODULE_NAME_SIZE * index, + name, HVMLOADER_MODULE_NAME_SIZE); + modlist[index].cmdline_paddr = + (dom->start_info_seg.pfn << PAGE_SHIFT) + + ((uintptr_t)modlist - (uintptr_t)start_info) + + sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT + + HVMLOADER_MODULE_NAME_SIZE * index; + + start_info->nr_modules++; +} + static int bootlate_hvm(struct xc_dom_image *dom) { uint32_t domid = dom->guest_domid; xc_interface *xch = dom->xch; + struct hvm_start_info *start_info; + size_t start_info_size; + void *start_page; + struct hvm_modlist_entry *modlist; - if ( !dom->device_model ) - { - struct hvm_start_info *start_info; - size_t start_info_size; - void *start_page; - - start_info_size = sizeof(*start_info) + dom->cmdline_size; - if ( dom->ramdisk_blob ) - start_info_size += sizeof(struct hvm_modlist_entry); + start_info_size = sizeof(*start_info) + dom->cmdline_size; + if ( dom->ramdisk_blob ) + start_info_size += sizeof(struct hvm_modlist_entry); - if ( start_info_size > - dom->start_info_seg.pages << XC_DOM_PAGE_SHIFT(dom) ) - { - DOMPRINTF("Trying to map beyond start_info_seg"); - return -1; - } + if ( start_info_size > + dom->start_info_seg.pages << XC_DOM_PAGE_SHIFT(dom) ) + { + DOMPRINTF("Trying to map beyond start_info_seg"); + return -1; + } - start_page = xc_map_foreign_range(xch, domid, start_info_size, - PROT_READ | PROT_WRITE, - dom->start_info_seg.pfn); - if ( start_page == NULL ) - { - DOMPRINTF("Unable to map HVM start info page"); - return -1; - } + start_page = xc_map_foreign_range(xch, domid, start_info_size, + PROT_READ | PROT_WRITE, + dom->start_info_seg.pfn); + if ( start_page == NULL ) + { + DOMPRINTF("Unable to map HVM start info page"); + return -1; + } - start_info = start_page; + start_info = start_page; + modlist = start_page + sizeof(*start_info) + dom->cmdline_size; + if ( !dom->device_model ) + { if ( dom->cmdline ) { char *cmdline = start_page + sizeof(*start_info); @@ -1740,22 +1780,30 @@ static int bootlate_hvm(struct xc_dom_image *dom) if ( dom->ramdisk_blob ) { - struct hvm_modlist_entry *modlist = - start_page + sizeof(*start_info) + dom->cmdline_size; modlist[0].paddr = dom->ramdisk_seg.vstart - dom->parms.virt_base; modlist[0].size = dom->ramdisk_seg.vend - dom->ramdisk_seg.vstart; - start_info->modlist_paddr = (dom->start_info_seg.pfn << PAGE_SHIFT) + - ((uintptr_t)modlist - (uintptr_t)start_info); start_info->nr_modules = 1; } - - start_info->magic = XEN_HVM_START_MAGIC_VALUE; - - munmap(start_page, start_info_size); } else { + add_module_to_list(dom, &dom->bios_module, "bios", + modlist, start_info); + } + + if ( start_info->nr_modules ) + { + start_info->modlist_paddr = (dom->start_info_seg.pfn << PAGE_SHIFT) + + ((uintptr_t)modlist - (uintptr_t)start_info); + } + + start_info->magic = XEN_HVM_START_MAGIC_VALUE; + + munmap(start_page, start_info_size); + + if ( dom->device_model ) + { void *hvm_info_page; if ( (hvm_info_page = xc_map_foreign_range( diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h index 64ba7ab..1cfec5c 100644 --- a/xen/include/public/xen.h +++ b/xen/include/public/xen.h @@ -798,7 +798,7 @@ struct start_info { typedef struct start_info start_info_t; /* - * Start of day structure passed to PVH guests in %ebx. + * Start of day structure passed to PVH guests and to HVM guests in %ebx. * * NOTE: nothing will be loaded at physical address 0, so a 0 value in any * of the address fields should be treated as not present.
... and load BIOS into guest memory. This adds a new firmware module, bios_module. It is loaded in the guest memory and final location is provided to hvmloader via the hvm_start_info struct. This patch create the hvm_start_info struct for HVM guest that have a device model, so this is now common code with HVM guest without device model. Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- Change in V4: - change title to suggest the change of beavior - remove code to load acpi tables (dsdt) - Update public/xen.h about hvm_start_info available on other HVM guest in %ebx. Change in V3: - rename acpi_table_module to full_acpi_module. - factorise module loading, using new function to load existing optinal module, this should not change anything - should now use the same code to loads modules as for HVMlite VMs. this avoid duplication of code. - no more generic cmdline with a list of modules, each module have its name in the module specific cmdline. - scope change for common code between hvmlite and hvmloader --- tools/libxc/include/xc_dom.h | 3 + tools/libxc/xc_dom_hvmloader.c | 2 + tools/libxc/xc_dom_x86.c | 132 ++++++++++++++++++++++++++++------------- xen/include/public/xen.h | 2 +- 4 files changed, 96 insertions(+), 43 deletions(-)