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

[Ian Campbell <ian.campbell@citrix.com>]

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.

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