From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v7 07/10] libxc: introduce soft reset for HVM domains Date: Tue, 2 Jun 2015 16:09:25 +0100 Message-ID: <1433257765.15036.310.camel@citrix.com> References: <1432740346-7887-1-git-send-email-vkuznets@redhat.com> <1432740346-7887-8-git-send-email-vkuznets@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Yznrl-0007QV-RE for xen-devel@lists.xenproject.org; Tue, 02 Jun 2015 15:12:34 +0000 In-Reply-To: <1432740346-7887-8-git-send-email-vkuznets@redhat.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: Vitaly Kuznetsov Cc: Wei Liu , Andrew Jones , Keir Fraser , Stefano Stabellini , Andrew Cooper , Julien Grall , Ian Jackson , Olaf Hering , Tim Deegan , David Vrabel , Jan Beulich , xen-devel@lists.xenproject.org, Daniel De Graaf List-Id: xen-devel@lists.xenproject.org On Wed, 2015-05-27 at 17:25 +0200, Vitaly Kuznetsov wrote: > Add new xc_soft_reset() function which performs so-called 'soft reset' > for an HVM domain. It is being performed in the following way: > - Save HVM context and all HVM params of the source domain; > - Transfer all the source domain's memory to the destinatio domain with "destination" > XEN_DOMCTL_soft_reset; > - Restore HVM context, HVM params, seed grant table for the new domain. > When the operation succeeds the source domain is being destroyed and the This is sounding a lot like a migration, with the refactoring done for migration v2 is there any possibility we might be able to reuse any of that? (e.g. the list of hvmparams to be dealt with seems like something which needs to be the same everywhere) > + if ( xc_domain_get_pod_target(xch, source_dom, &pod_info[0], &pod_info[1], > + &pod_info[2]) ) I don't like all the open coded [0], [1] and [2] in the following code which this implies. You could define symbolic names TOT_PAGES=1, etc but TBH I think you would be better off just having 3 appropriately named variables instead of the array. > + { > + PERROR("Could not get old domain's PoD info"); > + return 1; > + } > + > + if ( xc_domain_getinfo(xch, dest_dom, 1, &new_info) != 1 || > + new_info.domid != dest_dom ) > + { > + PERROR("Could not get new domain's info"); > + return 1; > + } > + > + if ( !old_info.hvm || !new_info.hvm ) > + { > + PERROR("Soft reset is supported for HVM only"); > + return 1; > + } Should do these sorts of checks before real work, like getting the PoD info. > + > + sharedinfo_pfn = old_info.shared_info_frame; > + if ( xc_get_pfn_type_batch(xch, source_dom, 1, &sharedinfo_pfn) ) > + { > + PERROR("xc_get_pfn_type_batch failed"); > + goto out; > + } > + > + hvm_buf_size = xc_domain_hvm_getcontext(xch, source_dom, 0, 0); > + if ( hvm_buf_size == -1 ) > + { > + PERROR("Couldn't get HVM context size from Xen"); > + goto out; > + } > + > + hvm_buf = malloc(hvm_buf_size); > + if ( !hvm_buf ) > + { > + ERROR("Couldn't allocate memory"); > + goto out; > + } > + > + if ( xc_domain_hvm_getcontext(xch, source_dom, hvm_buf, > + hvm_buf_size) == -1 ) > + { > + PERROR("HVM:Could not get hvm buffer"); > + goto out; > + } > + > + xc_hvm_param_get(xch, source_dom, HVM_PARAM_STORE_PFN, > + &hvm_params[HVM_PARAM_STORE_PFN]); > + store_pfn = hvm_params[HVM_PARAM_STORE_PFN]; > + *store_mfn = store_pfn; > + > + xc_hvm_param_get(xch, source_dom, > + HVM_PARAM_CONSOLE_PFN, > + &hvm_params[HVM_PARAM_CONSOLE_PFN]); > + console_pfn = hvm_params[HVM_PARAM_CONSOLE_PFN]; > + *console_mfn = console_pfn; > + > + xc_hvm_param_get(xch, source_dom, HVM_PARAM_BUFIOREQ_PFN, > + &hvm_params[HVM_PARAM_BUFIOREQ_PFN]); > + buffio_pfn = hvm_params[HVM_PARAM_BUFIOREQ_PFN]; > + > + xc_hvm_param_get(xch, source_dom, HVM_PARAM_IOREQ_PFN, > + &hvm_params[HVM_PARAM_IOREQ_PFN]); > + io_pfn = hvm_params[HVM_PARAM_IOREQ_PFN]; > + > + xc_hvm_param_get(xch, source_dom, HVM_PARAM_IDENT_PT, > + &hvm_params[HVM_PARAM_IDENT_PT]); > + > + xc_hvm_param_get(xch, source_dom, HVM_PARAM_PAGING_RING_PFN, > + &hvm_params[HVM_PARAM_PAGING_RING_PFN]); > + > + xc_hvm_param_get(xch, source_dom, HVM_PARAM_VM86_TSS, > + &hvm_params[HVM_PARAM_VM86_TSS]); > + > + xc_hvm_param_get(xch, source_dom, HVM_PARAM_ACPI_IOPORTS_LOCATION, > + &hvm_params[HVM_PARAM_ACPI_IOPORTS_LOCATION]); > + > + xc_hvm_param_get(xch, source_dom, HVM_PARAM_VIRIDIAN, > + &hvm_params[HVM_PARAM_VIRIDIAN]); > + > + xc_hvm_param_get(xch, source_dom, HVM_PARAM_PAE_ENABLED, > + &hvm_params[HVM_PARAM_PAE_ENABLED]); > + > + xc_hvm_param_get(xch, source_dom, HVM_PARAM_STORE_EVTCHN, > + &hvm_params[HVM_PARAM_STORE_EVTCHN]); > + > + xc_hvm_param_get(xch, source_dom, HVM_PARAM_IOREQ_SERVER_PFN, > + &hvm_params[HVM_PARAM_IOREQ_SERVER_PFN]); > + > + xc_hvm_param_get(xch, source_dom, HVM_PARAM_NR_IOREQ_SERVER_PAGES, > + &hvm_params[HVM_PARAM_NR_IOREQ_SERVER_PAGES]); > + > + xc_hvm_param_get(xch, source_dom, HVM_PARAM_VM_GENERATION_ID_ADDR, > + &hvm_params[HVM_PARAM_VM_GENERATION_ID_ADDR]); Even if this can't be shared with the migration please at least make it table driven (i.e. an array of HVM_PARAM_* to wander over) so that get/set can use the same table and not get out of sync. Does the set have to follow the xc_domain_soft_reset and/or the get precede the xc_domain_soft_reset? Or could a simple helper implement get +set in one "move" operation? That would also remove the possibility of forgetting to do both halves of one hvm param. Ian.