From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH v3 3/3] libxc/PV: save/restore data breakpoint extension registers Date: Fri, 11 Apr 2014 17:37:17 +0100 Message-ID: References: <53428C8B020000780000606B@nat28.tlf.novell.com> <53428E770200007800006089@nat28.tlf.novell.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 1WYeS9-0004MN-8L for xen-devel@lists.xenproject.org; Fri, 11 Apr 2014 16:37:21 +0000 Received: by mail-we0-f181.google.com with SMTP id q58so5481468wes.26 for ; Fri, 11 Apr 2014 09:37:17 -0700 (PDT) In-Reply-To: <53428E770200007800006089@nat28.tlf.novell.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: Jan Beulich Cc: Keir Fraser , Ian Jackson , Ian Campbell , Aravind Gopalakrishnan , Suravee Suthikulpanit , xen-devel List-Id: xen-devel@lists.xenproject.org On Mon, Apr 7, 2014 at 10:39 AM, Jan Beulich wrote: > Requiring the handling of the generic MSR extension to > XEN_DOMCTL_[gs]et_ext_vcpucontext. > > Signed-off-by: Jan Beulich > --- > v3: Split off of hypervisor patch and address review comments: > - check XEN_DOMCTL_get_ext_vcpucontext size output before looking > at msr_count field > - scrub xen_domctl_ext_vcpucontext_t's msrs field on the sending > side > - clear xen_domctl_ext_vcpucontext_t's msr_count field on the > restore side if the size field doesn't cover the msrs one > - make use of hypercall buffer bouncing interfaces on the restore > side (on the save side this would seem to complicate the code > rather than simplifying it) > - add documenting note to tools/libxc/xg_save_restore.h > > --- a/tools/libxc/xc_domain_restore.c > +++ b/tools/libxc/xc_domain_restore.c > @@ -38,6 +38,7 @@ > > #include > #include > +#include > > #include "xg_private.h" > #include "xg_save_restore.h" > @@ -590,8 +591,13 @@ static int buffer_tail_pv(xc_interface * > uint32_t vcpuextstate_size) > { > unsigned int i; > - size_t pfnlen, vcpulen; > + size_t pfnlen, vcpulen, total; > + int alloc = 0; > struct domain_info_context *dinfo = &ctx->dinfo; > + union { > + const unsigned char *raw; > + xen_domctl_ext_vcpucontext_t *evc; > + } ptr; > > /* TODO: handle changing pfntab and vcpu counts */ > /* PFN tab */ > @@ -634,11 +640,42 @@ static int buffer_tail_pv(xc_interface * > ERROR("Error allocating VCPU ctxt tail buffer"); > goto free_pfntab; > } > + alloc = 1; > } > // DPRINTF("Reading VCPUS: %d bytes\n", vcpulen); > - if ( RDEXACT(fd, buf->vcpubuf, vcpulen) ) { > - PERROR("Error when reading ctxt"); > - goto free_vcpus; > + for ( total = i = 0, ptr.raw = buf->vcpubuf; ext_vcpucontext; ) { This isn't actually a for loop. Would it really be so terrible to do if(ext_vcpucontext) { while(1) {}}? > + if ( RDEXACT(fd, buf->vcpubuf + total, vcpulen) ) { > + PERROR("Error when reading ctxt"); > + goto free_vcpus; > + } > + total += vcpulen; > + for ( vcpulen = 0; i < buf->vcpucount; ++i ) { > + size_t msrlen; > + > + if ( (const unsigned char *)(ptr.evc + 1) > buf->vcpubuf + total ) > + break; > + if ( ptr.evc->size < > + (offsetof(xen_domctl_ext_vcpucontext_t, msrs) + > + sizeof(ptr.evc->msrs)) ) > + ptr.evc->msr_count = 0; > + msrlen = ptr.evc->msr_count * sizeof(xen_domctl_ext_vcpu_msr_t); > + vcpulen += msrlen; > + ptr.raw += 128 + msrlen + vcpuextstate_size; > + } > + if ( !vcpulen ) { > + assert(i == buf->vcpucount); > + break; > + } If you're going to write such twisted logic, you absolutely must comment it -- at very least you need to give a high level description of what it's doing. I've been staring at this for an hour now, and I only have a vague idea what it's supposed to be doing. Seriously, is this any better than spaghetti code? > --- a/tools/libxc/xc_domain_save.c > +++ b/tools/libxc/xc_domain_save.c [snip] > @@ -1960,16 +1963,42 @@ int xc_domain_save(xc_interface *xch, in > domctl.domain = dom; > memset(&domctl.u, 0, sizeof(domctl.u)); > domctl.u.ext_vcpucontext.vcpu = i; > - if ( xc_domctl(xch, &domctl) < 0 ) > + frc = xc_domctl(xch, &domctl); > + if ( frc < 0 && errno == ENOBUFS && > + domctl.u.ext_vcpucontext.size >= > + (offsetof(xen_domctl_ext_vcpucontext_t, msrs) + > + sizeof(domctl.u.ext_vcpucontext.msrs)) && > + domctl.u.ext_vcpucontext.msr_count ) > + { > + msrs = xc_hypercall_buffer_alloc(xch, msrs, > + domctl.u.ext_vcpucontext.msr_count * > + sizeof(*msrs)); > + set_xen_guest_handle(domctl.u.ext_vcpucontext.msrs, msrs); > + frc = msrs ? xc_domctl(xch, &domctl) : -1; > + /* Don't save the actual pointer. */ > + set_xen_guest_handle_raw(domctl.u.ext_vcpucontext.msrs, NULL); > + } I think this could use a comment as well. -George