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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

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

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