| Message ID | 1452110602-3570-3-git-send-email-boris.ostrovsky@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
El 06/01/16 a les 21.03, Boris Ostrovsky ha escrit: > With commit 8c45adec18e0 ("libxc: create unmapped initrd in domain > builder if supported") location of ramdisk may not be available to > HVMlite guests by the time alloc_magic_pages_hvm() is invoked if the > guest supports unmapped initrd. > > So let's move ramdisk info initialization (along with a few other > operations that are not directly related to allocating magic/special > pages) from alloc_magic_pages_hvm() to bootlate_hvm(). > > Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > Acked-by: Wei Liu <wei.liu2@citrix.com> > --- > tools/libxc/xc_dom_x86.c | 116 ++++++++++++++++++++++++++------------------- > 1 files changed, 67 insertions(+), 49 deletions(-) > > diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c > index b8d2904..e102bd2 100644 > --- a/tools/libxc/xc_dom_x86.c > +++ b/tools/libxc/xc_dom_x86.c > @@ -586,23 +586,12 @@ static void build_hvm_info(void *hvm_info_page, struct xc_dom_image *dom) > static int alloc_magic_pages_hvm(struct xc_dom_image *dom) > { > unsigned long i; > - void *hvm_info_page; > uint32_t *ident_pt, domid = dom->guest_domid; > int rc; > 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; > > - if ( dom->device_model ) > - { > - if ( (hvm_info_page = xc_map_foreign_range( > - xch, domid, PAGE_SIZE, PROT_READ | PROT_WRITE, > - HVM_INFO_PFN)) == NULL ) > - goto error_out; > - build_hvm_info(hvm_info_page, dom); > - munmap(hvm_info_page, PAGE_SIZE); > - } > - > /* Allocate and clear special pages. */ > for ( i = 0; i < X86_HVM_NR_SPECIAL_PAGES; i++ ) > special_array[i] = special_pfn(i); > @@ -637,12 +626,9 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom) > if ( !dom->device_model ) > { > struct xc_dom_seg seg; > - struct hvm_start_info *start_info; > - char *cmdline; > struct hvm_modlist_entry *modlist; This can be removed if the conditional below is changed to: if ( dom->ramdisk_blob ) - start_info_size += sizeof(*modlist); + start_info_size += sizeof(struct hvm_modlist_entry); Because AFAICT the variable itself is not used for anything else. > - void *start_page; > size_t cmdline_size = 0; > - size_t start_info_size = sizeof(*start_info); > + size_t start_info_size = sizeof(struct hvm_start_info); > > if ( dom->cmdline ) > { > @@ -661,39 +647,6 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom) > goto out; > } > > - start_page = xc_map_foreign_range(xch, domid, start_info_size, > - PROT_READ | PROT_WRITE, > - seg.pfn); > - if ( start_page == NULL ) > - { > - DOMPRINTF("Unable to map HVM start info page"); > - goto error_out; > - } > - > - start_info = start_page; > - cmdline = start_page + sizeof(*start_info); > - modlist = start_page + sizeof(*start_info) + cmdline_size; > - > - if ( dom->cmdline ) > - { > - strncpy(cmdline, dom->cmdline, cmdline_size); > - start_info->cmdline_paddr = (seg.pfn << PAGE_SHIFT) + > - ((uintptr_t)cmdline - (uintptr_t)start_info); > - } > - > - if ( dom->ramdisk_blob ) > - { > - 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 = (seg.pfn << PAGE_SHIFT) + > - ((uintptr_t)modlist - (uintptr_t)start_info); > - start_info->nr_modules = 1; > - } > - > - start_info->magic = HVM_START_MAGIC_VALUE; > - > - munmap(start_page, start_info_size); > - > dom->start_info_pfn = seg.pfn; > } > else > @@ -1783,7 +1736,72 @@ static int alloc_pgtables_hvm(struct xc_dom_image *dom) > > static int bootlate_hvm(struct xc_dom_image *dom) > { > - DOMPRINTF("%s: doing nothing", __func__); > + uint32_t domid = dom->guest_domid; > + xc_interface *xch = dom->xch; > + > + if ( !dom->device_model ) > + { > + struct hvm_start_info *start_info; > + size_t start_info_size = sizeof(*start_info); > + void *start_page; > + struct hvm_modlist_entry *modlist; > + size_t cmdline_size = 0; > + > + if ( dom->cmdline ) > + { > + cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 8); > + start_info_size += cmdline_size; > + } > + if ( dom->ramdisk_blob ) > + start_info_size += sizeof(*modlist); /* Limited to one module. */ The size calculations are duplicated, could you either stash start_info_size into xc_dom_image, or simply do the memory allocation (xc_dom_alloc_segment) inside of bootlate_hvm? (I think the latter would be better if possible). Roger.
El 06/01/16 a les 21.03, Boris Ostrovsky ha escrit: > With commit 8c45adec18e0 ("libxc: create unmapped initrd in domain > builder if supported") location of ramdisk may not be available to > HVMlite guests by the time alloc_magic_pages_hvm() is invoked if the > guest supports unmapped initrd. > > So let's move ramdisk info initialization (along with a few other > operations that are not directly related to allocating magic/special > pages) from alloc_magic_pages_hvm() to bootlate_hvm(). > > Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > Acked-by: Wei Liu <wei.liu2@citrix.com> > --- > tools/libxc/xc_dom_x86.c | 116 ++++++++++++++++++++++++++------------------- > 1 files changed, 67 insertions(+), 49 deletions(-) > > diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c > index b8d2904..e102bd2 100644 > --- a/tools/libxc/xc_dom_x86.c > +++ b/tools/libxc/xc_dom_x86.c > @@ -586,23 +586,12 @@ static void build_hvm_info(void *hvm_info_page, struct xc_dom_image *dom) > static int alloc_magic_pages_hvm(struct xc_dom_image *dom) > { > unsigned long i; > - void *hvm_info_page; > uint32_t *ident_pt, domid = dom->guest_domid; > int rc; > 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; > > - if ( dom->device_model ) > - { > - if ( (hvm_info_page = xc_map_foreign_range( > - xch, domid, PAGE_SIZE, PROT_READ | PROT_WRITE, > - HVM_INFO_PFN)) == NULL ) > - goto error_out; > - build_hvm_info(hvm_info_page, dom); > - munmap(hvm_info_page, PAGE_SIZE); > - } > - > /* Allocate and clear special pages. */ > for ( i = 0; i < X86_HVM_NR_SPECIAL_PAGES; i++ ) > special_array[i] = special_pfn(i); > @@ -637,12 +626,9 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom) > if ( !dom->device_model ) > { > struct xc_dom_seg seg; > - struct hvm_start_info *start_info; > - char *cmdline; > struct hvm_modlist_entry *modlist; This can be removed if the conditional below is changed to: if ( dom->ramdisk_blob ) - start_info_size += sizeof(*modlist); + start_info_size += sizeof(struct hvm_modlist_entry); Because AFAICT the variable itself is not used for anything else. > - void *start_page; > size_t cmdline_size = 0; > - size_t start_info_size = sizeof(*start_info); > + size_t start_info_size = sizeof(struct hvm_start_info); > > if ( dom->cmdline ) > { > @@ -661,39 +647,6 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom) > goto out; > } > > - start_page = xc_map_foreign_range(xch, domid, start_info_size, > - PROT_READ | PROT_WRITE, > - seg.pfn); > - if ( start_page == NULL ) > - { > - DOMPRINTF("Unable to map HVM start info page"); > - goto error_out; > - } > - > - start_info = start_page; > - cmdline = start_page + sizeof(*start_info); > - modlist = start_page + sizeof(*start_info) + cmdline_size; > - > - if ( dom->cmdline ) > - { > - strncpy(cmdline, dom->cmdline, cmdline_size); > - start_info->cmdline_paddr = (seg.pfn << PAGE_SHIFT) + > - ((uintptr_t)cmdline - (uintptr_t)start_info); > - } > - > - if ( dom->ramdisk_blob ) > - { > - 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 = (seg.pfn << PAGE_SHIFT) + > - ((uintptr_t)modlist - (uintptr_t)start_info); > - start_info->nr_modules = 1; > - } > - > - start_info->magic = HVM_START_MAGIC_VALUE; > - > - munmap(start_page, start_info_size); > - > dom->start_info_pfn = seg.pfn; > } > else > @@ -1783,7 +1736,72 @@ static int alloc_pgtables_hvm(struct xc_dom_image *dom) > > static int bootlate_hvm(struct xc_dom_image *dom) > { > - DOMPRINTF("%s: doing nothing", __func__); > + uint32_t domid = dom->guest_domid; > + xc_interface *xch = dom->xch; > + > + if ( !dom->device_model ) > + { > + struct hvm_start_info *start_info; > + size_t start_info_size = sizeof(*start_info); > + void *start_page; > + struct hvm_modlist_entry *modlist; > + size_t cmdline_size = 0; > + > + if ( dom->cmdline ) > + { > + cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 8); > + start_info_size += cmdline_size; > + } > + if ( dom->ramdisk_blob ) > + start_info_size += sizeof(*modlist); /* Limited to one module. */ The size calculations are duplicated, could you either stash start_info_size into xc_dom_image, or simply do the memory allocation (xc_dom_alloc_segment) inside of bootlate_hvm? (I think the latter would be better if possible). Roger.
On 01/07/2016 06:43 AM, Roger Pau Monné wrote: > El 06/01/16 a les 21.03, Boris Ostrovsky ha escrit: >> >> static int bootlate_hvm(struct xc_dom_image *dom) >> { >> - DOMPRINTF("%s: doing nothing", __func__); >> + uint32_t domid = dom->guest_domid; >> + xc_interface *xch = dom->xch; >> + >> + if ( !dom->device_model ) >> + { >> + struct hvm_start_info *start_info; >> + size_t start_info_size = sizeof(*start_info); >> + void *start_page; >> + struct hvm_modlist_entry *modlist; >> + size_t cmdline_size = 0; >> + >> + if ( dom->cmdline ) >> + { >> + cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 8); >> + start_info_size += cmdline_size; >> + } >> + if ( dom->ramdisk_blob ) >> + start_info_size += sizeof(*modlist); /* Limited to one module. */ > The size calculations are duplicated, could you either stash > start_info_size into xc_dom_image, or simply do the memory allocation > (xc_dom_alloc_segment) inside of bootlate_hvm? (I think the latter would > be better if possible). I didn't want to do the first because we'd use that information (two pieces --- we need to to know both the size of the extra chunk and where modlist starts) only once and it's not on a critical path. You can, of course, argue that we increase text size. The problem with the second approach is that while it does seem to work I don't know whether we can delay allocations until bootlate(): notice how we print dom->virt_alloc_end in xc_dom_build_image() which to me indicates some "finality" as far as allocations for domain are concerned. -boris
El 07/01/16 a les 15.47, Boris Ostrovsky ha escrit: > On 01/07/2016 06:43 AM, Roger Pau Monné wrote: >> El 06/01/16 a les 21.03, Boris Ostrovsky ha escrit: >>> static int bootlate_hvm(struct xc_dom_image *dom) >>> { >>> - DOMPRINTF("%s: doing nothing", __func__); >>> + uint32_t domid = dom->guest_domid; >>> + xc_interface *xch = dom->xch; >>> + >>> + if ( !dom->device_model ) >>> + { >>> + struct hvm_start_info *start_info; >>> + size_t start_info_size = sizeof(*start_info); >>> + void *start_page; >>> + struct hvm_modlist_entry *modlist; >>> + size_t cmdline_size = 0; >>> + >>> + if ( dom->cmdline ) >>> + { >>> + cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 8); >>> + start_info_size += cmdline_size; >>> + } >>> + if ( dom->ramdisk_blob ) >>> + start_info_size += sizeof(*modlist); /* Limited to one >>> module. */ >> The size calculations are duplicated, could you either stash >> start_info_size into xc_dom_image, or simply do the memory allocation >> (xc_dom_alloc_segment) inside of bootlate_hvm? (I think the latter would >> be better if possible). > > I didn't want to do the first because we'd use that information (two > pieces --- we need to to know both the size of the extra chunk and where > modlist starts) only once and it's not on a critical path. You can, of > course, argue that we increase text size. It's just that I don't want to get them out of synch, and that's what usually happens when you perform the same calculations in two different places, one might get out of synch with the other. > The problem with the second approach is that while it does seem to work > I don't know whether we can delay allocations until bootlate(): notice > how we print dom->virt_alloc_end in xc_dom_build_image() which to me > indicates some "finality" as far as allocations for domain are concerned. For PV guests it probably matters, because we have to build the page tables and the p2m, for HVMlite guests I don't think it matters at all (or at least I don't see any obvious reason). Another third option is to place the code that performs the size calculations inside of a function that's called by both (bootlate_hvm and alloc_magic_pages_hvm). Roger.
On Thu, 2016-01-07 at 17:54 +0100, Roger Pau Monné wrote: > El 07/01/16 a les 15.47, Boris Ostrovsky ha escrit: > > On 01/07/2016 06:43 AM, Roger Pau Monné wrote: > > > El 06/01/16 a les 21.03, Boris Ostrovsky ha escrit: > > > > static int bootlate_hvm(struct xc_dom_image *dom) > > > > { > > > > - DOMPRINTF("%s: doing nothing", __func__); > > > > + uint32_t domid = dom->guest_domid; > > > > + xc_interface *xch = dom->xch; > > > > + > > > > + if ( !dom->device_model ) > > > > + { > > > > + struct hvm_start_info *start_info; > > > > + size_t start_info_size = sizeof(*start_info); > > > > + void *start_page; > > > > + struct hvm_modlist_entry *modlist; > > > > + size_t cmdline_size = 0; > > > > + > > > > + if ( dom->cmdline ) > > > > + { > > > > + cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 8); > > > > + start_info_size += cmdline_size; > > > > + } > > > > + if ( dom->ramdisk_blob ) > > > > + start_info_size += sizeof(*modlist); /* Limited to one > > > > module. */ > > > The size calculations are duplicated, could you either stash > > > start_info_size into xc_dom_image, or simply do the memory allocation > > > (xc_dom_alloc_segment) inside of bootlate_hvm? (I think the latter > > > would > > > be better if possible). > > > > I didn't want to do the first because we'd use that information (two > > pieces --- we need to to know both the size of the extra chunk and > > where > > modlist starts) only once and it's not on a critical path. You can, of > > course, argue that we increase text size. > > It's just that I don't want to get them out of synch, and that's what > usually happens when you perform the same calculations in two different > places, one might get out of synch with the other. > > > The problem with the second approach is that while it does seem to work > > I don't know whether we can delay allocations until bootlate(): notice > > how we print dom->virt_alloc_end in xc_dom_build_image() which to me > > indicates some "finality" as far as allocations for domain are > > concerned. > > For PV guests it probably matters, because we have to build the page > tables and the p2m, for HVMlite guests I don't think it matters at all > (or at least I don't see any obvious reason). > > Another third option is to place the code that performs the size > calculations inside of a function that's called by both (bootlate_hvm > and alloc_magic_pages_hvm). The call to xc_dom_alloc_segment initialises a xc_dom_seg, which in this case alloc_magic just throws away. If the location/size of that segment is required elsewhere then the normal approach would be to add it to the handle struct (cf dom->{kernel,ramdisk}_seg et al) and to consume it in the other places. My concerns would be the size calculation and the pointer figuring out getting out of sync or the size of one of the inputs changing between the size calculation and the use (i.e. by a future patch moving something around). So the place which figures out the pointers would also need to bounds check against the mapped region's size. Ian. > > Roger. >
On 01/07/2016 12:06 PM, Ian Campbell wrote: > On Thu, 2016-01-07 at 17:54 +0100, Roger Pau Monné wrote: >> El 07/01/16 a les 15.47, Boris Ostrovsky ha escrit: >>> On 01/07/2016 06:43 AM, Roger Pau Monné wrote: >>>> El 06/01/16 a les 21.03, Boris Ostrovsky ha escrit: >>>>> static int bootlate_hvm(struct xc_dom_image *dom) >>>>> { >>>>> - DOMPRINTF("%s: doing nothing", __func__); >>>>> + uint32_t domid = dom->guest_domid; >>>>> + xc_interface *xch = dom->xch; >>>>> + >>>>> + if ( !dom->device_model ) >>>>> + { >>>>> + struct hvm_start_info *start_info; >>>>> + size_t start_info_size = sizeof(*start_info); >>>>> + void *start_page; >>>>> + struct hvm_modlist_entry *modlist; >>>>> + size_t cmdline_size = 0; >>>>> + >>>>> + if ( dom->cmdline ) >>>>> + { >>>>> + cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 8); >>>>> + start_info_size += cmdline_size; >>>>> + } >>>>> + if ( dom->ramdisk_blob ) >>>>> + start_info_size += sizeof(*modlist); /* Limited to one >>>>> module. */ >>>> The size calculations are duplicated, could you either stash >>>> start_info_size into xc_dom_image, or simply do the memory allocation >>>> (xc_dom_alloc_segment) inside of bootlate_hvm? (I think the latter >>>> would >>>> be better if possible). >>> I didn't want to do the first because we'd use that information (two >>> pieces --- we need to to know both the size of the extra chunk and >>> where >>> modlist starts) only once and it's not on a critical path. You can, of >>> course, argue that we increase text size. >> It's just that I don't want to get them out of synch, and that's what >> usually happens when you perform the same calculations in two different >> places, one might get out of synch with the other. >> >>> The problem with the second approach is that while it does seem to work >>> I don't know whether we can delay allocations until bootlate(): notice >>> how we print dom->virt_alloc_end in xc_dom_build_image() which to me >>> indicates some "finality" as far as allocations for domain are >>> concerned. >> For PV guests it probably matters, because we have to build the page >> tables and the p2m, for HVMlite guests I don't think it matters at all >> (or at least I don't see any obvious reason). >> >> Another third option is to place the code that performs the size >> calculations inside of a function that's called by both (bootlate_hvm >> and alloc_magic_pages_hvm). > The call to xc_dom_alloc_segment initialises a xc_dom_seg, which in this > case alloc_magic just throws away. If the location/size of that segment is > required elsewhere then the normal approach would be to add it to the > handle struct (cf dom->{kernel,ramdisk}_seg et al) and to consume it in the > other places. xc_dom_alloc_segment() also updates domain's pfn_alloc_end and virt_alloc_end, which is what I was thinking about (i.e. that updating those values bootlate() time may be too late). > > My concerns would be the size calculation and the pointer figuring out > getting out of sync or the size of one of the inputs changing between the > size calculation and the use (i.e. by a future patch moving something > around). So the place which figures out the pointers would also need to > bounds check against the mapped region's size. So then I think the safest is to stash the value of cmdline length in xc_dom_image (and use strncpy). For modlist we only have a single module (ramdisk) and I just realized that we can actually use ramdisk_seg. In fact, it looks like we are setting it up and then never use it for HVMlite. -boris
On Thu, 2016-01-07 at 12:33 -0500, Boris Ostrovsky wrote: > On 01/07/2016 12:06 PM, Ian Campbell wrote: > > On Thu, 2016-01-07 at 17:54 +0100, Roger Pau Monné wrote: > > > El 07/01/16 a les 15.47, Boris Ostrovsky ha escrit: > > > > On 01/07/2016 06:43 AM, Roger Pau Monné wrote: > > > > > El 06/01/16 a les 21.03, Boris Ostrovsky ha escrit: > > > > > > static int bootlate_hvm(struct xc_dom_image *dom) > > > > > > { > > > > > > - DOMPRINTF("%s: doing nothing", __func__); > > > > > > + uint32_t domid = dom->guest_domid; > > > > > > + xc_interface *xch = dom->xch; > > > > > > + > > > > > > + if ( !dom->device_model ) > > > > > > + { > > > > > > + struct hvm_start_info *start_info; > > > > > > + size_t start_info_size = sizeof(*start_info); > > > > > > + void *start_page; > > > > > > + struct hvm_modlist_entry *modlist; > > > > > > + size_t cmdline_size = 0; > > > > > > + > > > > > > + if ( dom->cmdline ) > > > > > > + { > > > > > > + cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, > > > > > > 8); > > > > > > + start_info_size += cmdline_size; > > > > > > + } > > > > > > + if ( dom->ramdisk_blob ) > > > > > > + start_info_size += sizeof(*modlist); /* Limited to > > > > > > one > > > > > > module. */ > > > > > The size calculations are duplicated, could you either stash > > > > > start_info_size into xc_dom_image, or simply do the memory > > > > > allocation > > > > > (xc_dom_alloc_segment) inside of bootlate_hvm? (I think the > > > > > latter > > > > > would > > > > > be better if possible). > > > > I didn't want to do the first because we'd use that information > > > > (two > > > > pieces --- we need to to know both the size of the extra chunk and > > > > where > > > > modlist starts) only once and it's not on a critical path. You can, > > > > of > > > > course, argue that we increase text size. > > > It's just that I don't want to get them out of synch, and that's what > > > usually happens when you perform the same calculations in two > > > different > > > places, one might get out of synch with the other. > > > > > > > The problem with the second approach is that while it does seem to > > > > work > > > > I don't know whether we can delay allocations until bootlate(): > > > > notice > > > > how we print dom->virt_alloc_end in xc_dom_build_image() which to > > > > me > > > > indicates some "finality" as far as allocations for domain are > > > > concerned. > > > For PV guests it probably matters, because we have to build the page > > > tables and the p2m, for HVMlite guests I don't think it matters at > > > all > > > (or at least I don't see any obvious reason). > > > > > > Another third option is to place the code that performs the size > > > calculations inside of a function that's called by both (bootlate_hvm > > > and alloc_magic_pages_hvm). > > The call to xc_dom_alloc_segment initialises a xc_dom_seg, which in > > this > > case alloc_magic just throws away. If the location/size of that segment > > is > > required elsewhere then the normal approach would be to add it to the > > handle struct (cf dom->{kernel,ramdisk}_seg et al) and to consume it in > > the > > other places. > > xc_dom_alloc_segment() also updates domain's pfn_alloc_end and > virt_alloc_end, which is what I was thinking about (i.e. that updating > those values bootlate() time may be too late). I was assuming the reason for calculating the size twice was that xc_dom_alloc_segment() was called in the earlier hook, which is why I mentioned stacking the xc_dom_seg. Ian.
diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c index b8d2904..e102bd2 100644 --- a/tools/libxc/xc_dom_x86.c +++ b/tools/libxc/xc_dom_x86.c @@ -586,23 +586,12 @@ static void build_hvm_info(void *hvm_info_page, struct xc_dom_image *dom) static int alloc_magic_pages_hvm(struct xc_dom_image *dom) { unsigned long i; - void *hvm_info_page; uint32_t *ident_pt, domid = dom->guest_domid; int rc; 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; - if ( dom->device_model ) - { - if ( (hvm_info_page = xc_map_foreign_range( - xch, domid, PAGE_SIZE, PROT_READ | PROT_WRITE, - HVM_INFO_PFN)) == NULL ) - goto error_out; - build_hvm_info(hvm_info_page, dom); - munmap(hvm_info_page, PAGE_SIZE); - } - /* Allocate and clear special pages. */ for ( i = 0; i < X86_HVM_NR_SPECIAL_PAGES; i++ ) special_array[i] = special_pfn(i); @@ -637,12 +626,9 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom) if ( !dom->device_model ) { struct xc_dom_seg seg; - struct hvm_start_info *start_info; - char *cmdline; struct hvm_modlist_entry *modlist; - void *start_page; size_t cmdline_size = 0; - size_t start_info_size = sizeof(*start_info); + size_t start_info_size = sizeof(struct hvm_start_info); if ( dom->cmdline ) { @@ -661,39 +647,6 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom) goto out; } - start_page = xc_map_foreign_range(xch, domid, start_info_size, - PROT_READ | PROT_WRITE, - seg.pfn); - if ( start_page == NULL ) - { - DOMPRINTF("Unable to map HVM start info page"); - goto error_out; - } - - start_info = start_page; - cmdline = start_page + sizeof(*start_info); - modlist = start_page + sizeof(*start_info) + cmdline_size; - - if ( dom->cmdline ) - { - strncpy(cmdline, dom->cmdline, cmdline_size); - start_info->cmdline_paddr = (seg.pfn << PAGE_SHIFT) + - ((uintptr_t)cmdline - (uintptr_t)start_info); - } - - if ( dom->ramdisk_blob ) - { - 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 = (seg.pfn << PAGE_SHIFT) + - ((uintptr_t)modlist - (uintptr_t)start_info); - start_info->nr_modules = 1; - } - - start_info->magic = HVM_START_MAGIC_VALUE; - - munmap(start_page, start_info_size); - dom->start_info_pfn = seg.pfn; } else @@ -1783,7 +1736,72 @@ static int alloc_pgtables_hvm(struct xc_dom_image *dom) static int bootlate_hvm(struct xc_dom_image *dom) { - DOMPRINTF("%s: doing nothing", __func__); + uint32_t domid = dom->guest_domid; + xc_interface *xch = dom->xch; + + if ( !dom->device_model ) + { + struct hvm_start_info *start_info; + size_t start_info_size = sizeof(*start_info); + void *start_page; + struct hvm_modlist_entry *modlist; + size_t cmdline_size = 0; + + if ( dom->cmdline ) + { + cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 8); + start_info_size += cmdline_size; + } + if ( dom->ramdisk_blob ) + start_info_size += sizeof(*modlist); /* Limited to one module. */ + + start_page = xc_map_foreign_range(xch, domid, start_info_size, + PROT_READ | PROT_WRITE, + dom->start_info_pfn); + if ( start_page == NULL ) + { + DOMPRINTF("Unable to map HVM start info page"); + return -1; + } + + start_info = start_page; + + if ( dom->cmdline ) + { + char *cmdline = start_page + sizeof(*start_info); + + strncpy(cmdline, dom->cmdline, cmdline_size); + start_info->cmdline_paddr = (dom->start_info_pfn << PAGE_SHIFT) + + ((uintptr_t)cmdline - (uintptr_t)start_info); + } + + if ( dom->ramdisk_blob ) + { + modlist = start_page + sizeof(*start_info) + 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_pfn << PAGE_SHIFT) + + ((uintptr_t)modlist - (uintptr_t)start_info); + start_info->nr_modules = 1; + } + + start_info->magic = HVM_START_MAGIC_VALUE; + + munmap(start_page, start_info_size); + } + else + { + void *hvm_info_page; + + if ( (hvm_info_page = xc_map_foreign_range( + xch, domid, PAGE_SIZE, PROT_READ | PROT_WRITE, + HVM_INFO_PFN)) == NULL ) + return -1; + build_hvm_info(hvm_info_page, dom); + munmap(hvm_info_page, PAGE_SIZE); + } + return 0; }
With commit 8c45adec18e0 ("libxc: create unmapped initrd in domain builder if supported") location of ramdisk may not be available to HVMlite guests by the time alloc_magic_pages_hvm() is invoked if the guest supports unmapped initrd. So let's move ramdisk info initialization (along with a few other operations that are not directly related to allocating magic/special pages) from alloc_magic_pages_hvm() to bootlate_hvm(). Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> Acked-by: Wei Liu <wei.liu2@citrix.com> --- tools/libxc/xc_dom_x86.c | 116 ++++++++++++++++++++++++++------------------- 1 files changed, 67 insertions(+), 49 deletions(-)