From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH] libxc: Defer initialization of start_page for HVM guests Date: Tue, 22 Dec 2015 09:12:03 -0500 Message-ID: <56795A33.4080407@oracle.com> References: <1450741524-27859-1-git-send-email-boris.ostrovsky@oracle.com> <56791AEB.2080409@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <56791AEB.2080409@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= , ian.jackson@eu.citrix.com, stefano.stabellini@eu.citrix.com, ian.campbell@citrix.com, wei.liu2@citrix.com Cc: jgross@suse.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 12/22/2015 04:42 AM, Roger Pau Monn=E9 wrote: > El 22/12/15 a les 0.45, 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. >> >> Since the only operation related to allocating magic pages in that >> routine is allocation of HVMlite start info we can move everything >> else to a later point such as xc_dom_arch.start_info(). >> >> Signed-off-by: Boris Ostrovsky >> --- >> I am not sure xc_dom_arch.start_info() is the right place neither since = we >> still are doing things that have nothing to do with start_info. >> >> tools/libxc/xc_dom_x86.c | 45 ++++++++++++++++++++++++++++++--------= ------- >> 1 files changed, 30 insertions(+), 15 deletions(-) >> >> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c >> index 3960875..f64079e 100644 >> --- a/tools/libxc/xc_dom_x86.c >> +++ b/tools/libxc/xc_dom_x86.c >> @@ -585,6 +585,32 @@ static void build_hvm_info(void *hvm_info_page, str= uct xc_dom_image *dom) >> = >> static int alloc_magic_pages_hvm(struct xc_dom_image *dom) >> { >> + struct xc_dom_seg seg; >> + size_t start_info_size =3D sizeof(struct hvm_start_info); >> + >> + if ( dom->device_model ) >> + return 0; >> + >> + if ( dom->cmdline ) >> + start_info_size +=3D ROUNDUP(strlen(dom->cmdline) + 1, 8); >> + if ( dom->ramdisk_blob ) >> + /* Limited to one module. */ >> + start_info_size +=3D sizeof( struct hvm_modlist_entry); > ^ extra space. >> + >> + if ( xc_dom_alloc_segment(dom, &seg, "HVMlite start info", 0, >> + start_info_size) ) >> + { >> + DOMPRINTF("Unable to reserve memory for the start info"); >> + return -1; >> + } >> + >> + dom->start_info_pfn =3D seg.pfn; >> + >> + return 0; >> +} >> + >> +static int start_info_hvm(struct xc_dom_image *dom) >> +{ >> unsigned long i; >> void *hvm_info_page; >> uint32_t *ident_pt, domid =3D dom->guest_domid; >> @@ -636,7 +662,6 @@ 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; >> @@ -653,17 +678,9 @@ static int alloc_magic_pages_hvm(struct xc_dom_imag= e *dom) >> if ( dom->ramdisk_blob ) >> start_info_size +=3D sizeof(*modlist); /* Limited to one m= odule. */ >> = >> - rc =3D xc_dom_alloc_segment(dom, &seg, "HVMlite start info", 0, >> - start_info_size); >> - if ( rc !=3D 0 ) >> - { >> - DOMPRINTF("Unable to reserve memory for the start info"); >> - goto out; >> - } >> - >> start_page =3D xc_map_foreign_range(xch, domid, start_info_siz= e, > IMHO, I would stash start_info_size somewhere inside xc_dom_image in > order to avoid calculating it twice. Yes, I should do this. > > Also, why is everything done inside of alloc_magic_pages_hvm moved to > start_info_hvm? AFAICT we should only need to move the code that fills > the hvm_start_info struct, but the rest of the code already present in > alloc_magic_pages_hvm could be left as-is. That's what I was referring to in the commit message (and in the comment = below it): most, if not all, of the stuff that alloc_magic_pages_hvm() = currently does really has nothing to do with magic pages allocation. = E.g. hvm_params settting, special pages initialization, etc. So I = figured I could move all of this to a later point. But, as I said in the = comment, I am not convinced that start_info() is the right place either. -boris