|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [BUGFIX][PATCH 4/4] hvm_save_one: allow the 2nd instance to be fetched for PIC.
On 12/13/13 09:38, Jan Beulich wrote: 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:On 12.12.13 at 01:56, Don Slutz <dslutz@xxxxxxxxxxx> wrote:
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; Not exactly. Using the following adjustment to check-hvmctx (patch #1): cb4d0e75a497f169c8419b30c1954f92bb8e29a8 Mon Sep 17 00:00:00 2001 From: Don Slutz <dslutz@xxxxxxxxxxx> 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@xxxxxxxxxxx> --- 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 *~ .*.dgcc -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=0cError: 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).
This is not 100% correct. The libxl code for xc_domain_hvm_getcontext_partial does take a length: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.
/* 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@xxxxxxxxxxxxx http://lists.xen.org/xen-devel _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |