From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [PATCH 1/1] xc_domain_restore: Allow QEMU to
 increase memory
Date: Tue, 14 Apr 2015 10:28:59 +0100
Message-ID: <552CDDDB.5050806@citrix.com>
References: <1428941353-18673-1-git-send-email-dslutz@verizon.com>
	<552BEDF1.7070209@citrix.com> <552C5683.1040903@one.verizon.com>
	<552CD5A7.4080803@citrix.com> <552CDC57.1020107@cn.fujitsu.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <552CDC57.1020107@cn.fujitsu.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Hongyang Yang <yanghy@cn.fujitsu.com>, Don Slutz <dslutz@verizon.com>, "xen-devel@lists.xen.org" <xen-devel@lists.xen.org>
Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>, Ian Jackson <ian.jackson@eu.citrix.com>, Wei Liu <wei.liu2@citrix.com>, Ian Campbell <ian.campbell@citrix.com>, Stefano Stabellini <stefano.stabellini@eu.citrix.com>
List-Id: xen-devel@lists.xenproject.org

On 14/04/15 10:22, Hongyang Yang wrote:
>
>
> On 04/14/2015 04:53 PM, Andrew Cooper wrote:
>> On 14/04/15 00:51, Don Slutz wrote:
>>> On 04/13/15 12:25, Andrew Cooper wrote:
>>>> On 13/04/15 17:09, Don Slutz wrote:
>>>>> If QEMU has called on xc_domain_setmaxmem to add more memory for
>>>>> option ROMs, domain restore needs to also increase the memory.
>>>>>
>>>>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>>>> hvmloader has no interaction with xc_domain_restore().
>>> I did not mean to imply that it did.  Somehow I lost the fact that this
>>> is a bug in master:
>>>
>>> [root@dcs-xen-52 don]# xl cre -p /home/don/e1000x8.xfg
>>> Parsing config from /home/don/e1000x8.xfg
>>> got a tsc mode string: "native_paravirt"
>>> [root@dcs-xen-52 don]# xl save e1000x8 e1000x8.save
>>> Saving to e1000x8.save new xl format (info 0x1/0x0/3506)
>>> xc: Saving memory: iter 0 (last sent 0 skipped 0): 1044481/1044481 
>>> 100%
>>> [root@dcs-xen-52 don]# xl restore e1000x8.save
>>> Loading new save file e1000x8.save (new xl fmt info 0x1/0x0/3506)
>>>   Savefile contains xl domain config in JSON format
>>> Parsing config from <saved>
>>> xc: error: Failed to allocate memory for batch.!: Internal error
>>> libxl: error: libxl_create.c:1057:libxl__xc_domain_restore_done:
>>> restoring domain: Cannot allocate memory
>>> libxl: error: libxl_create.c:1129:domcreate_rebuild_done: cannot
>>> (re-)build domain: -3
>>> libxl: error: libxl.c:1576:libxl__destroy_domid: non-existant domain 2
>>> libxl: error: libxl.c:1540:domain_destroy_callback: unable to destroy
>>> guest with domid 2
>>> libxl: error: libxl_create.c:1490:domcreate_destruction_cb: unable to
>>> destroy domain 2 following failed creation
>>> [root@dcs-xen-52 don]#
>>>
>>> The hvmloader part turns out to be a "warning message" that is ok to
>>> ignore.  However "xl save" followed by "xl restore" is currently broken
>>> without some fix.
>>>
>>>> It is xl's job to propagate the current memory as part of
>>>> migration.  If
>>>> qemu has had to bump it up, this should be reflected in the domain
>>>> config file.
>>> Not at all sure how QEMU (either in dom0 or a driver domain) is
>>> expected
>>> to change a file (the domain config file).  My guess is that you are
>>> saying that "xl save" (before xc_domain_save) is the correct place to
>>> record (or add extra info).   However it looks to me that all the data
>>> needed is in the save file, but
>>> I could be wrong.
>>>
>>> The page data is correctly saved into the save file (migration stream).
>>> However when
>>> the new domain is created, it's size is "wrong".  You cannot adjust any
>>> of the config info to fix the size, because the option ROM space to
>>> reserve is not an existing config option.   So if I am following you
>>> correctly, libxl needs to add and process more info to handle this
>>> case.
>>>
>>>> The migration path should not be hacked up to fix a higher level
>>>> toolstack bug.
>>> I do not see how QEMU is part of the "higher level toolstack".  If you
>>> mean xl vs xc, then
>>> I can see "xl save" some how doing the work.  This patch works for me,
>>> but I am happy to
>>> explore other ways to fix this bug.
>>
>> If I understand correctly, the steps are this:
>>
>> * 'xl create' makes a VM of size $FOO
>> * qemu bumps the size to $FOO+$N
>> * 'xl save' writes $FOO+$N of page data, but the xl config file at the
>> start of the image still says $FOO
>> * 'xl restore' creates a vm of size $FOO, then instructs
>> xc_domain_restore() to put $FOO+$N pages into it.
>>
>> I would argue first, that qemu should not play in this area to start
>> with.
>>
>> However, the real bug here is that the domain configuration written by
>> xl save is inaccurate.
>
> There's a case like COLO:
> 1. Both Primary/Secondary VM are created first with the same config file
>    which makes a VM of size $FOO
> 2. qemu bumps the size to $FOO+$N
> 3. 'save' writes $FOO+$N of page data
> 4. 'restore' put $FOO+$N pages into $FOO pages which will cause error
>
> Even if you fix the configuration, the bug still exists because the
> config
> only been transferred from Primary to Secondary once at the very
> beginning
> when you execute 'xl remus' command. The problem is how to let the
> secondary
> (restore) side knows the size change? Through a migration
> command(which is
> easier in v2 migration) or some other solution?
> Form this point of view, I think Don's solution is one way to solve the
> problem.

Funny you should ask that.  Migrationv2 for libxl moves the JSON config
blob into the libxl stream, rather than being a singleshot action at the
very start.  From that point, it becomes plausible to send a new JSON
blob when an update on the source side occurs.

However, I am still firmly of the opinion that is an xl/libxl bug to be
fixed at that level, not hacked around in the restore code.

~Andrew