From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [BUGFIX][PATCH 4/4] hvm_save_one: allow the 2nd instance to be fetched for PIC. Date: Sat, 14 Dec 2013 20:38:38 -0500 Message-ID: <52AD081E.4070600@terremark.com> References: <1386809777-12898-1-git-send-email-dslutz@terremark.com> <1386809777-12898-5-git-send-email-dslutz@terremark.com> <52AB29E4020000780010D0EE@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Vs0fQ-00021f-Mq for xen-devel@lists.xenproject.org; Sun, 15 Dec 2013 01:38:48 +0000 In-Reply-To: <52AB29E4020000780010D0EE@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Keir Fraser , Ian Campbell , Stefano Stabellini , Ian Jackson , Don Slutz , xen-devel List-Id: xen-devel@lists.xenproject.org On 12/13/13 09:38, Jan Beulich wrote: >>>> On 12.12.13 at 01:56, Don Slutz 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 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 --- 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, , ) 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