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

[Juergen Gross <jgross@suse.com>]

On 30/11/15 13:35, Ian Campbell wrote:
> On Mon, 2015-11-30 at 13:20 +0100, Juergen Gross wrote:
>> 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.
> 
> Thanks!
> 
> FYI attempting to upgrade osstest to use Debian Jessie in the guest seems
> to have exposed another issue here.
> 
> http://logs.test-lab.xenproject.org/osstest/logs/65172/test-amd64-amd64-amd64-pvgrub/info.html
> 
>       Booting 'Debian GNU/Linux, kernel 3.16.0-4-amd64'
> 
>     root  (hd0,0)
>      Filesystem type is ext2fs, partition type 0x83
>     kernel  /boot/vmlinuz-3.16.0-4-amd64 root=UUID=12447529-e85a-4b41-86b6-3e83ccfc
>     1377 ro 
>     initrd  /boot/initrd.img-3.16.0-4-amd64
> 
>     ============= Init TPM Front ================
>     Tpmfront:Error Unable to read device/vtpm/0/backend-id during tpmfront initialization! error = ENOENT
>     Tpmfront:Info Shutting down tpmfront
>     pin_table(x) returned 1357193
>     close(3)
> 
>     Error 9: Unknown boot failure
> 
>     Press any key to continue...
> 
> xen.git 6853c9bf9ff0 is OK, whereas 713b7e4ef2aa is not. Adding your two
> outstanding patches:
>     libxc: correct domain builder for 64 bit guest with 32 bit tools
>     libxc: use correct return type for do_memory_op()
> 
> Doesn't appear to have helped. Anyway, I was in the process of
> investigating/bisecting etc but since I was mailing you any way I thought
> I'd mention it. I'll start a fresh thread once I have some more to go on.

When something was wrong with pvgrub in my tests of the patches it died
right away and didn't show random errors later. I don't think the
problem you are seeing is related to my recent changes. OTOH I have been
wrong before. :-(


Juergen