On 15/12/2013 17:19, Don Slutz wrote:
On 12/15/13 11:51, Andrew Cooper wrote:
On 15/12/2013 00:29, Don Slutz wrote:

I think I have corrected all coding errors (please check again). And done all requested changes.  I did add the reviewed by (not sure if I should since this changes a large part of the patch, but they are all what Jan said).

I have unit tested it and it appears to work the same as the previous version (as expected).

Here is the new version, also attached.

From e0e8f5246ba492b153884cea93bfe753f1b0782e Mon Sep 17 00:00:00 2001
From: Don Slutz <dslutz@verizon.com>
Date: Tue, 12 Nov 2013 08:22:53 -0500
Subject: [PATCH v2 3/4] hvm_save_one: return correct data.

It is possible that hvm_sr_handlers[typecode].save does not use all
the provided room.  In that case, using:

   instance * hvm_sr_handlers[typecode].size

does not select the correct instance.  Add code to search for the
correct instance.

Signed-off-by: Don Slutz <dslutz@verizon.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>

but this fairs no better at selecting the correct subset in the case that less data than hvm_sr_handlers[typecode].size is written by hvm_sr_handlers[typecode].save.

True, but the inverse is the case here; .save writes 'n' 'size' blocks.  Form the loop above:

    if ( hvm_sr_handlers[typecode].kind == HVMSR_PER_VCPU )
        for_each_vcpu(d, v)
            sz += hvm_sr_handlers[typecode].size;
    else
        sz = hvm_sr_handlers[typecode].size;

so sz is in multiples of 'size'.  Normally sz == ctxt.cur.  With some offline vcpus it write fewer 'size' blocks.
It always increments by 'size' bytes, and will only copy the data back if the bytes under desc->instance happen to match the instance we are looking for.

The only time it does not find one is for an offline vcpu.  Try out the unit test code in patch #1 on an unchanged xen.  It should not display anything.  Then offline a cpu in a domU (echo 0 > /sys/devices/system/cpu/cpu1/online).  And with 3 vcpus, it will report an error.

   -Don Slutz

Ah - so there are actually two problems.  I see now the one you are trying to solve, and would agree that your code does solve it.

However, some of the save handlers are themselves variable length, and will write records shorter than hvm_sr_handlers[typecode].size if they can get away with doing so.  In this case, the new logic still wont get the correct instance.

~Andrew