Re: [Xen-devel] [PATCH v5 6/9] libxc: create unmapped initrd in domain builder if supported

[Juergen Gross <jgross@xxxxxxxx>]

On 30/11/15 12:23, Ian Campbell wrote:
> On Mon, 2015-11-30 at 12:03 +0100, Juergen Gross wrote:
>> On 30/11/15 11:52, Ian Campbell wrote:
>>> On Mon, 2015-11-30 at 10:51 +0000, Ian Campbell wrote:
>>>> On Mon, 2015-11-30 at 11:47 +0100, Juergen Gross wrote:
>>>>> On 30/11/15 11:34, Ian Campbell wrote:
>>>>>> On Mon, 2015-11-30 at 11:23 +0100, Juergen Gross wrote:
>>>>>>> On 30/11/15 11:20, Wei Liu wrote:
>>>>>>>> On Thu, Nov 26, 2015 at 08:35:02AM +0100, Juergen Gross
>>>>>>>> wrote:
>>>>>>>>>  
>>>>>>>>>      /* initrd parameters as specified in start_info page
>>>>>>>>> */
>>>>>>>>> -    unsigned long initrd_start;
>>>>>>>>> -    unsigned long initrd_len;
>>>>>>>>> +    uint64_t initrd_start;
>>>>>>>>> +    uint64_t initrd_len;
>>>>>>>>>  
>>>>>>>>
>>>>>>>> I think these should be of type xen_vaddr_t. Doesn't make a
>>>>>>>> difference
>>>>>>>> in the end though.
>>>>>>>
>>>>>>> xen_vaddr_t seems not to be appropriate. It can be either a
>>>>>>> virtual
>>>>>>> address or a pfn.
>>>>>>
>>>>>> Did you mean a virtual address or a physical _address_?
>>>>>> Potentially
>>>>>> mixing
>>>>>> addresses and frame numbers in a single variable seems liable to
>>>>>> be
>>>>>> confusing, at best.
>>>>>
>>>>> No, it's really a pfn. And this is part of the stable interface
>>>>> between
>>>>> hypervisor and the pv-domU since more than 5 years now.
>>>>
>>>> Including the virtual address bit?
>>>>
>>>> That's a shame.
>>>
>>> ... and that being the case would you mind adding a comment here
>>> explaining
>>> the two forms of these variables and the flag which indicates which one
>>> is
>>> "in force" at a given moment.
>>
>> The comment in the struct already tells us that initrd_start and
>> initrd_len are in the very same format as in the start_info page.
>> Both fields are meant to be opaque to most of the domain builder
>> parts.
>>
>> The only function dealing with the differences is xc_dom_build_image()
>> which already contains the appropriate flag. I added this on your
>> request. You acked the resulting patch. So why do you want to add
>> another comment now?
> 
> I hadn't realised at the time that the semantics of these fields was so,
> uh, interesting.

:-)

I guess due to the lack of a comment? ;-)

Okay, I'll add one when submitting the patch after (hopefully) Boris
confirmed it is fixing his problem.


Juergen


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