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

[Don Slutz <dslutz@verizon.com>]

On 12/13/13 09:38, Jan Beulich wrote:
>>>> On 12.12.13 at 01:56, Don Slutz <dslutz@verizon.com> 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).

>>   
>>           rv = -EBADSLT;
>> -        for (off = 0; off < ctxt.cur; off += hvm_sr_handlers[typecode].size) {
>> +        for (off = 0; off < ctxt.cur; off += add + sizeof (struct hvm_save_descriptor)) {
>>               struct hvm_save_descriptor *desc
>>                      = (struct hvm_save_descriptor *)&ctxt.data[off];
>> +            add = desc->length;
>>               if (instance == desc->instance) {
>>                   rv = 0;
>>                   if ( copy_to_guest(handle,
>>                                      ctxt.data
>>                                      + off
>>                                      + sizeof (struct hvm_save_descriptor),
>> -                                   hvm_sr_handlers[typecode].size
>> -                                   - sizeof (struct hvm_save_descriptor)) )
>> +                                   add) )
> Further, the way this was done before means that for multi-
> instance records all records would get copied out, but the
> caller would have no way of knowing that (except by implying
> that behavior, e.g. "known to be the case for PIC").
Not exactly.  Using the following adjustment to check-hvmctx (patch #1):

cb4d0e75a497f169c8419b30c1954f92bb8e29a8 Mon Sep 17 00:00:00 2001
From: Don Slutz <dslutz@verizon.com>
Date: Sat, 14 Dec 2013 19:31:16 -0500
Subject: [PATCH] check-hvmctx: add check for extra data under debug 8

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
  tools/tests/check-hvmctx/check-hvmctx.c | 19 ++++++++++++++++++-
  1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/tools/tests/check-hvmctx/check-hvmctx.c 
b/tools/tests/check-hvmctx/check-hvmctx.c
index 9b5c3aa..668b77a 100644
--- a/tools/tests/check-hvmctx/check-hvmctx.c
+++ b/tools/tests/check-hvmctx/check-hvmctx.c
@@ -574,7 +574,7 @@ int main(int argc, char **argv)
          default:
              if (xc_domain_hvm_getcontext_partial(
                      xch, domid, desc.typecode, desc.instance,
-                    tbuf, desc.length) != 0) {
+                    tbuf, len) != 0) {
                  fprintf(stderr, "Error: 
xc_domain_hvm_getcontext_partial: entry %i: type %u instance %u, length 
%u: ",
                          entry, (unsigned) desc.typecode,
                          (unsigned) desc.instance, (unsigned) desc.length);
@@ -582,6 +582,23 @@ int main(int argc, char **argv)
                  retval = 42;
                  memset(tbuf, 0xee, desc.length);
              }
+            if (debug & 0x08) {
+                int i;
+                int header = 1;
+
+                for (i = desc.length; i < len; i++) {
+                    if (tbuf[i]) {
+                        if (header) {
+                            header = 0;
+                            printf("Error: entry %i: type %u instance 
%u, length %u extra data!\n",
+                                   entry, (unsigned) desc.typecode,
+                                   (unsigned) desc.instance, (unsigned) 
desc.length);
+                        }
+                        printf("[%03x] unexpected data=%02x\n",
+                               i, tbuf[i]);
+                    }
+                }
+            }
              ret = desc.length;
  #ifndef NOTCLEAN
              if (desc.typecode == HVM_SAVE_CODE(CPU))
-- 
1.7.11.7

and before this patch:

dcs-xen-51:~/xen/tools/tests/check-hvmctx>make clean;make 
CHECK_HVMCTX="-DDOPIC"
rm -f *.o check-hvmctx *~ .*.d
gcc  -O1 -fno-omit-frame-pointer -m64 -g -fno-strict-aliasing -std=gnu99 
-Wall -Wstrict-prototypes -Wdeclaration-after-statement 
-Wno-unused-but-set-variable -Wno-unused-local-typedefs -D__XEN_TOOLS__ 
-MMD -MF .check-hvmctx.o.d -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE 
-fno-optimize-sibling-calls  -Werror 
-I/home/don/xen/tools/tests/check-hvmctx/../../../tools/libxc 
-I/home/don/xen/tools/tests/check-hvmctx/../../../tools/include 
-I/home/don/xen/tools/tests/check-hvmctx/../../../tools/include 
-I/home/don/xen/tools/tests/check-hvmctx/../../../tools/xenstore 
-I/home/don/xen/tools/tests/check-hvmctx/../../../tools/include 
-I/home/don/xen/tools/tests/check-hvmctx/../../../tools -DDOPIC  -c -o 
check-hvmctx.o check-hvmctx.c
gcc    -o check-hvmctx check-hvmctx.o 
/home/don/xen/tools/tests/check-hvmctx/../../../tools/libxc/libxenctrl.so
dcs-xen-51:~/xen/tools/tests/check-hvmctx>sudo ./check-hvmctx 1 8
Check HVM save record vs partial for domain 1
Error: entry 8: type 3 instance 0, length 8 extra data!
[008] unexpected data=03
[00a] unexpected data=01
[00c] unexpected data=08
[010] unexpected data=01
[011] unexpected data=ff
[013] unexpected data=28
[016] unexpected data=0c
Error: xc_domain_hvm_getcontext_partial: entry 9: type 3 instance 1, 
length 8: Invalid argument
Error: entry 9: type 3 instance 1, length 8 mismatch!
     PIC: IRQ base 0x28, irr 0x1, imr 0xff, isr 0


I see that after the expected length (8), there is a struct 
hvm_save_descriptor (type 3 instance 1, length 8) followed by another 
HVM_SAVE_TYPE(PIC).
> 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)
{

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.

> Hence I think rather than complicating the logic here, we should
> change the interface to pass a size in and back out, which will
> not only avoid corrupting memory, but also allow the guest to
> recognize multi-instance data being returned.
The size is passed in, just not passed out.  And the fact that the data is:

HVM_SAVE_TYPE(PIC)
struct hvm_save_descriptor
HVM_SAVE_TYPE(PIC)

Is strange to me. One descriptor for two entries.  This was the primary 
reason I went the way I did.  I am not saying that the interface should 
not change; I just do not see that as a bug fix type change.

      -Don Slutz
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel