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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 11.04.14 at 18:37, <dunlapg@umich.edu> wrote:
> On Mon, Apr 7, 2014 at 10:39 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> @@ -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) {}}?

Nothing really terrible, but it would increase indentation by one level
for no real reason.

>> +        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?

Now do you have any better suggestion? I was just trying to extend
what is there with as little churn on the code as possible. I'm not even
certain the model chose is desirable - I could imagine that introducing
another "extended info block", e.g. tagged "msrs", might be preferable.

In any event, this isn't code I'm feeling comfortable changing, so of all
possible options I'd prefer to have this whole aspect of the intended
functionality done by someone else who is. And the patch here then
might serve as no more than a baseline reference, to be thrown away
entirely if so desired.

Perhaps the best thing really is to leave the save/restore aspect out
for now, and get it implemented once Andrew's rewrite is in place.

>> @@ -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.

Not sure what you refer to here - the one non-obvious thing (clearing
out the handle) is being commented. Are you trying to tell me that this
isn't sufficient of an explanation, or are you referring to other parts of
this code block?

Jan