Re: [Xen-devel] [PATCH v3 3/3] libxc/PV: save/restore data breakpoint extension registers

[George Dunlap <dunlapg@xxxxxxxxx>]

On Mon, Apr 7, 2014 at 10:39 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> Requiring the handling of the generic MSR extension to
> XEN_DOMCTL_[gs]et_ext_vcpucontext.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> 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 <stdlib.h>
>  #include <unistd.h>
> +#include <assert.h>
>
>  #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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel