Re: [Xen-devel] [BUGFIX][PATCH 4/4] hvm_save_one: allow the 2nd instance to be fetched for PIC.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.12.13 at 02:38, Don Slutz <dslutz@xxxxxxxxxxx> wrote:
> On 12/13/13 09:38, Jan Beulich wrote:
>>>>> On 12.12.13 at 01:56, Don Slutz <dslutz@xxxxxxxxxxx> wrote:
>>> @@ -114,20 +111,20 @@ int hvm_save_one(struct domain *d, uint16_t typecode, 
> uint16_t instance,
>>>       }
>>>       else
>>>       {
>>> -        uint32_t off;
>>> +        uint32_t off, add;
>> "add" is a pretty odd name for what this is being used for. Why
>> don't you use desc->length directly?
> I picked the name when the 3rd part of the "for" was "off += add". 
> During unit
> testing that did not work and so did not try and pick a new one.  I 
> could have added add2:
> 
>      const uint32_t add2 = sizeof (struct hvm_save_descriptor);
> 
> And then the last part of the for becomes "off += add + add2".
> 
> I can not use desc->length because:
> 
> save.c: In function 'hvm_save_one':
> save.c:120:47: error: 'desc' undeclared (first use in this function)
> save.c:120:47: note: each undeclared identifier is reported only once 
> for each function it appears in
> 
> (It is only defined in the body of the for).

Right - so simply move it into the next surrounding scope.

>> Which shows another shortcoming of the interface: There's no
>> buffer size being passed in from the caller, yet we have variable
>> size records. Which means latent data corruption in the caller.
> This is not 100% correct.  The libxl code for 
> xc_domain_hvm_getcontext_partial does take a length:
> 
> /* Get just one element of the HVM guest context.
>   * size must be >= HVM_SAVE_LENGTH(type) */
> int xc_domain_hvm_getcontext_partial(xc_interface *xch,
>                                       uint32_t domid,
>                                       uint16_t typecode,
>                                       uint16_t instance,
>                                       void *ctxt_buf,
>                                       uint32_t size)
> {

Which doesn't mean anything for the hypervisor interface. Yet
that's the one I said has the limitation.

> and it gets embeded in
> 
> 
>      DECLARE_HYPERCALL_BOUNCE(ctxt_buf, size, 
> XC_HYPERCALL_BUFFER_BOUNCE_OUT);
> 
> which is handle.  I do not know that much about 
> "XEN_GUEST_HANDLE_64(uint8) handle" and DECLARE_HYPERCALL_BOUNCE, but 
> what my unit testing has shown me is that copy_to_guest(handle, <ptr>, 
> <nr>) does only copy up to the size stored in handle.  It looks to zero 
> an unknown amount more (looks to be page sized).  So there is more 
> needed here.

Again, you need to split the way libxc works from the actual
hypercall: A handle _never_ tells you to how many items it
refers, there's always a second parameter required to pass
the element count (unless known by other means).

Jan


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