From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH RESEND v5 6/6] xen/arm: Implement toolstack for xl restore/save and migrate Date: Wed, 13 Nov 2013 11:09:02 +0000 Message-ID: <1384340942.5406.68.camel@kazak.uk.xensource.com> References: <1383897048-12528-1-git-send-email-jaeyong.yoo@samsung.com> <1383897048-12528-7-git-send-email-jaeyong.yoo@samsung.com> <1384276945.10204.103.camel@kazak.uk.xensource.com> <52834D24.7010209@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <52834D24.7010209@samsung.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Eugene Fedotov Cc: xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On Wed, 2013-11-13 at 13:57 +0400, Eugene Fedotov wrote: > >> diff --git a/tools/libxc/xc_arm_migrate.c b/tools/libxc/xc_arm_migrate.c > >> new file mode 100644 > >> index 0000000..461e339 > >> --- /dev/null > >> +++ b/tools/libxc/xc_arm_migrate.c > >> @@ -0,0 +1,712 @@ > > Is this implementing the exact protocol as described in > > tools/libxc/xg_save_restore.h or is it a variant? Are there any docs of > > the specifics of the ARM protocol? > This implements a quite different from tools/libxc/xg_save_restore.h > protocol, it is much more simplified because we do not need some things > that implemented for x86. So you're right, it has to be documented. > Should we use a different header to place documentation to this (and > place some definitions), for example xc_arm_migrate.h? I think xg_arm_save_restore.h would be the consistent name. At some point we should rename some of the x86 specific stuff, but you don't need to worry about that. > > We will eventually need to make a statement about the stability of the > > protocol, i.e on x86 we support X->X+1 migrations across Xen versions. I > > think we'd need to make similar guarantees on ARM before we would remove > > the "tech preview" label from the migration feature. > So, should you believe our results (and where should we place this > statement) or should you make tests from your side? By stability I mean the stability of the migration ABI across version changes, not stability as in the bugginess or otherwise of the code. IOW a commitment to supporting migration from version X to version X+1 in the future. WRT "tech preview" I don't think we are going to be able to honestly call this production ready for the general case in 4.4, I expect it'll need a bit longer to "bed in" before we are happy with that statement. (this has no bearing on whether you want to fully support it as production ready in your particular application). > > > >> +/* ============ Memory ============= */ > >> +static int save_memory(xc_interface *xch, int io_fd, uint32_t dom, > >> + struct save_callbacks *callbacks, > >> + uint32_t max_iters, uint32_t max_factor, > >> + guest_params_t *params) > >> +{ > >> + int live = !!(params->flags & XCFLAGS_LIVE); > >> + int debug = !!(params->flags & XCFLAGS_DEBUG); > >> + xen_pfn_t i; > >> + char reportbuf[80]; > >> + int iter = 0; > >> + int last_iter = !live; > >> + int total_dirty_pages_num = 0; > >> + int dirty_pages_on_prev_iter_num = 0; > >> + int count = 0; > >> + char *page = 0; > >> + xen_pfn_t *busy_pages = 0; > >> + int busy_pages_count = 0; > >> + int busy_pages_max = 256; > >> + > >> + DECLARE_HYPERCALL_BUFFER(unsigned long, to_send); > >> + > >> + xen_pfn_t start = params->start_gpfn; > >> + const xen_pfn_t end = params->max_gpfn; > >> + const xen_pfn_t mem_size = end - start; > >> + > >> + if ( debug ) > >> + { > >> + IPRINTF("(save mem) start=%llx end=%llx!\n", start, end); > >> + } > > FYI you don't need the {}'s for cases like this. > Actually we don't need {}, this has been done because we was not sure if > this macro can be empty-substituted. > > > > is if ( debug ) IPRINTF(...) not the equivalent of DPRINTF? > This equivalence is not obvious for me, because in current code we > obtain debug flag with XCFLAGS_DEBUG mask (when --debug option passed). > If it is equivalent I'll use DPRINTF. No you are right, these are different. Actually DPRINTF is "detail level", there is a DBGPRINTF which is "debug level" (levels here being in terms of XTL_FOO from xentoollog.h). So you probably do want to use if (debug), although you may separately want to consider which level the resulting messages are printed at when they are printed... > > > > [...] > >> + > >> +static int restore_guest_params(xc_interface *xch, int io_fd, > >> + uint32_t dom, guest_params_t *params) > >> +{ > >> [...] > >> + if ( xc_domain_setmaxmem(xch, dom, maxmemkb) ) > >> + { > >> + ERROR("Can't set memory map"); > >> + return -1; > >> + } > >> + > >> + /* Set max. number of vcpus as max_vcpu_id + 1 */ > >> + if ( xc_domain_max_vcpus(xch, dom, params->max_vcpu_id + 1) ) > > Does the higher level toolstack not take care of vcpus and maxmem? I > > thought so. I think this is how it shoud be. > > For my tests guest config information is not transferred for ARM case > from high-level stack. At the migration receiver side toolstack always > create a new domain with vcpus=1 and default max. mem. So we have to > send guest information as our local guest_params structure (at the > beginning of migration). > It is easy way to work "xl save" or "xl migrate" without modification of > libxl level, but you may have another idea? > Also, toolstack_restore callback is not set (NULL) for ARM case. So you aren't using xl to do the migration? This is what we should ultimately be aiming for. It is almost certainly going to require fixes at the libxl level though. If you need to send additional information for your usecase then it should be done at the layer above libxc. Ian.