[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [BUGFIX][PATCH 3/4] hvm_save_one: return correct data.



On 12/15/13 13:11, Andrew Cooper wrote:
On 15/12/2013 17:42, Don Slutz wrote:
On 12/15/13 12:22, Andrew Cooper wrote:
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@xxxxxxxxxxx>
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@xxxxxxxxxxx>
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

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.

Not sure which one(s) you are referring to.

From the full dump:

xen-hvmctx 1| grep -i entry
Entry 0: type 1 instance 0, length 24
Entry 1: type 2 instance 0, length 1024
Entry 2: type 2 instance 2, length 1024
Entry 3: type 2 instance 3, length 1024
Entry 4: type 2 instance 4, length 1024
Entry 5: type 2 instance 5, length 1024
Entry 6: type 2 instance 6, length 1024
Entry 7: type 2 instance 7, length 1024
Entry 8: type 3 instance 0, length 8
Entry 9: type 3 instance 1, length 8
Entry 10: type 4 instance 0, length 400
Entry 11: type 5 instance 0, length 24
Entry 12: type 5 instance 1, length 24
Entry 13: type 5 instance 2, length 24
Entry 14: type 5 instance 3, length 24
Entry 15: type 5 instance 4, length 24
Entry 16: type 5 instance 5, length 24
Entry 17: type 5 instance 6, length 24
Entry 18: type 5 instance 7, length 24
Entry 19: type 6 instance 0, length 1024
Entry 20: type 6 instance 1, length 1024
Entry 21: type 6 instance 2, length 1024
Entry 22: type 6 instance 3, length 1024
Entry 23: type 6 instance 4, length 1024
Entry 24: type 6 instance 5, length 1024
Entry 25: type 6 instance 6, length 1024
Entry 26: type 6 instance 7, length 1024
Entry 27: type 7 instance 0, length 16
Entry 28: type 8 instance 0, length 8
Entry 29: type 9 instance 0, length 8
Entry 30: type 10 instance 0, length 56
Entry 31: type 11 instance 0, length 16
Entry 32: type 12 instance 0, length 1048
Entry 33: type 13 instance 0, length 8
Entry 34: type 14 instance 0, length 240
Entry 35: type 14 instance 1, length 240
Entry 36: type 14 instance 2, length 240
Entry 37: type 14 instance 3, length 240
Entry 38: type 14 instance 4, length 240
Entry 39: type 14 instance 5, length 240
Entry 40: type 14 instance 6, length 240
Entry 41: type 14 instance 7, length 240
Entry 42: type 16 instance 0, length 856
Entry 43: type 16 instance 1, length 856
Entry 44: type 16 instance 2, length 856
Entry 45: type 16 instance 3, length 856
Entry 46: type 16 instance 4, length 856
Entry 47: type 16 instance 5, length 856
Entry 48: type 16 instance 6, length 856
Entry 49: type 16 instance 7, length 856
Entry 50: type 18 instance 0, length 24
Entry 51: type 18 instance 1, length 24
Entry 52: type 18 instance 2, length 24
Entry 53: type 18 instance 3, length 24
Entry 54: type 18 instance 4, length 24
Entry 55: type 18 instance 5, length 24
Entry 56: type 18 instance 6, length 24
Entry 57: type 18 instance 7, length 24
Entry 58: type 19 instance 0, length 8
Entry 59: type 19 instance 1, length 8
Entry 60: type 19 instance 2, length 8
Entry 61: type 19 instance 3, length 8
Entry 62: type 19 instance 4, length 8
Entry 63: type 19 instance 5, length 8
Entry 64: type 19 instance 6, length 8
Entry 65: type 19 instance 7, length 8
Entry 66: type 0 instance 0, length 0

All typecode's appear to save the same amount per instance.

Most use hvm_save_entry:

...
        _hvm_write_entry((_h), (_src), HVM_SAVE_LENGTH(_x));    \

and

/* Syntactic sugar around that function: specify the max number of                                                     
 * saves, and this calculates the size of buffer needed */
#define HVM_REGISTER_SAVE_RESTORE(_x, _save, _load, _num, _k)             \
static int __init __hvm_register_##_x##_save_and_restore(void)            \
{                                                                         \
    hvm_register_savevm(HVM_SAVE_CODE(_x),                                \
                        #_x,                                              \
                        &_save,                                           \
                        &_load,                                           \
                        (_num) * (HVM_SAVE_LENGTH(_x)                     \
                                  + sizeof (struct hvm_save_descriptor)), \
                        _k);                                              \
    return 0;                                                             \
}                                                                         \

I do not find any that call on _hvm_write_entry directly.

The only special one I found: CPU_XSAVE_CODE

Still "writes" a full sized entry:

        if ( _hvm_init_entry(h, CPU_XSAVE_CODE, v->vcpu_id, HVM_CPU_XSAVE_SIZE) )
            return 1;
        ctxt = (struct hvm_hw_cpu_xsave *)&h->data[h->cur];
        h->cur += HVM_CPU_XSAVE_SIZE;
        memset(ctxt, 0, HVM_CPU_XSAVE_SIZE);

It then modifies the zeros conditionaly.

        if ( v->fpu_initialised )
            memcpy(&ctxt->save_area,
                v->arch.xsave_area, xsave_cntxt_size);

#define HVM_CPU_XSAVE_SIZE  (3 * sizeof(uint64_t) + xsave_cntxt_size)

is part of this.

/* We need variable length data chunk for xsave area, hence customized                                                 
 * declaration other than HVM_REGISTER_SAVE_RESTORE.                                                                   
 */
static int __init __hvm_register_CPU_XSAVE_save_and_restore(void)
{
    hvm_register_savevm(CPU_XSAVE_CODE,
                        "CPU_XSAVE",
                        hvm_save_cpu_xsave_states,
                        hvm_load_cpu_xsave_states,
                        HVM_CPU_XSAVE_SIZE + sizeof (struct hvm_save_descriptor),
                        HVMSR_PER_VCPU);
    return 0;
}
__initcall(__hvm_register_CPU_XSAVE_save_and_restore);

is the final part of this one.  So I do not find any code that does what you are wondering about.

   -Don


HVM_CPU_XSAVE_SIZE() changes depending on which xsave features have ever been enabled by a vcpu (size is proportional to the contents of v->arch.xcr0_accum).  It is not guaranteed to be the same for each vcpu in a domain, (although almost certainly will be the same for any recognisable OS)

Ah, I see.

Well, hvm_save_one, hvm_save_size, and hvm_save all expect that hvm_sr_handlers[typecode].size has the max size.  I do not see that being true for XSAVE.
Jan's new generic MSR save record will also write less than the maximum if it can.

This looks to be Jan's patch:

http://lists.xen.org/archives/html/xen-devel/2013-12/msg02061.html

Does look to set hvm_sr_handlers[typecode].size to the max size.

And it looks like the code I did in patch #4 would actually fix this issue.  Since it now uses the length stored in the save descriptor to find each instance.

Jan has some questions about patch #4; so what to do about it is still pending.

Clearly I can merge #3 and #4 into 1 patch.

   -Don Slutz
~Andrew



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.