Re: [Xen-devel] [PATCH v3 2/2] libxc: Defer initialization of start_page for HVM guests

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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.
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel