# HG changeset patch # User Horms # Node ID 76218624fb4ad5f2ce5831ec0dcee1f0e24d7f41 # Parent 64f11b0e2e7d0c53320c4dc6e983fcb144258c43 libxc: xc_ptrace cleanups General Cleanups * Use { after if consistently in xc_ptrace.c and xc_ptrace_core.c (But not in xc_ptrace_core() which should be removed shortly) * Remove duplicate code and centralise around xc_ptrace.h * Avoid ifing values covered by case in xc_ptrace() - PTRACE_GETREGS, PTRACE_GETFPREGS and PTRACE_GETFPXREGS are grouped into a single case, and then with the exception of a call to FETCH_REGS(), different code is executed based on ifing the values covered by the case. The PTRACE_GETFPREGS and PTRACE_GETFPXREGS code is actually a duplicate. This patch breaks the code out to two different cases. Error Handling * Eliminate FETCH_REGS macro as it forces several functions to have an otherwise uneeded error_out label, mittigating any code savins. * Rework error handling in xc_ptrace(). - Remove FETCH_REGS as above - Make sure that all dom0 errors are caught - Make sure errno is always set on error * Eliminate gotos in xc_ptrace_core.c that do nothing but return Signed-Off-By: Horms diff -r 64f11b0e2e7d tools/libxc/xc_ptrace.c --- a/tools/libxc/xc_ptrace.c Sat Mar 4 19:16:36 2006 +0100 +++ b/tools/libxc/xc_ptrace.c Mon Mar 6 14:42:37 2006 +0900 @@ -7,9 +7,35 @@ #include "xc_private.h" #include "xg_private.h" -#include #include "xc_ptrace.h" +const char const * ptrace_names[] = { + "PTRACE_TRACEME", + "PTRACE_PEEKTEXT", + "PTRACE_PEEKDATA", + "PTRACE_PEEKUSER", + "PTRACE_POKETEXT", + "PTRACE_POKEDATA", + "PTRACE_POKEUSER", + "PTRACE_CONT", + "PTRACE_KILL", + "PTRACE_SINGLESTEP", + "PTRACE_INVALID", + "PTRACE_INVALID", + "PTRACE_GETREGS", + "PTRACE_SETREGS", + "PTRACE_GETFPREGS", + "PTRACE_SETFPREGS", + "PTRACE_ATTACH", + "PTRACE_DETACH", + "PTRACE_GETFPXREGS", + "PTRACE_SETFPXREGS", + "PTRACE_INVALID", + "PTRACE_INVALID", + "PTRACE_INVALID", + "PTRACE_INVALID", + "PTRACE_SYSCALL", +}; /* XXX application state */ static long nr_pages = 0; @@ -32,7 +58,8 @@ fetch_regs(int xc_handle, int cpu, int * if (online) *online = 0; - if ( !(regs_valid & (1 << cpu)) ) { + if ( !(regs_valid & (1 << cpu)) ) + { retval = xc_vcpu_getcontext(xc_handle, current_domid, cpu, &ctxt[cpu]); if ( retval ) @@ -50,9 +77,6 @@ fetch_regs(int xc_handle, int cpu, int * return retval; } -#define FETCH_REGS(cpu) if (fetch_regs(xc_handle, cpu, NULL)) goto error_out; - - static struct thr_ev_handlers { thr_ev_handler_t td_create; thr_ev_handler_t td_death; @@ -95,14 +119,12 @@ get_online_cpumap(int xc_handle, dom0_ge *cpumap = 0; for (i = 0; i <= d->max_vcpu_id; i++) { if ((retval = fetch_regs(xc_handle, i, &online))) - goto error_out; + return retval; if (online) *cpumap |= (1 << i); } return 0; - error_out: - return retval; } /* @@ -118,7 +140,8 @@ online_vcpus_changed(cpumap_t cpumap) int index; while ( (index = ffsll(changed_cpumap)) ) { - if ( cpumap & (1 << (index - 1)) ) { + if ( cpumap & (1 << (index - 1)) ) + { if (handlers.td_create) handlers.td_create(index - 1); } else { printf("thread death: %d\n", index - 1); @@ -143,34 +166,32 @@ map_domain_va_pae( uint64_t *l3, *l2, *l1; static void *v; - FETCH_REGS(cpu); + if (fetch_regs(xc_handle, cpu, NULL)) + return NULL; l3 = xc_map_foreign_range( xc_handle, current_domid, PAGE_SIZE, PROT_READ, ctxt[cpu].ctrlreg[3] >> PAGE_SHIFT); if ( l3 == NULL ) - goto error_out; + return NULL; l2p = l3[l3_table_offset_pae(va)] >> PAGE_SHIFT; l2 = xc_map_foreign_range(xc_handle, current_domid, PAGE_SIZE, PROT_READ, l2p); if ( l2 == NULL ) - goto error_out; + return NULL; l1p = l2[l2_table_offset_pae(va)] >> PAGE_SHIFT; l1 = xc_map_foreign_range(xc_handle, current_domid, PAGE_SIZE, perm, l1p); if ( l1 == NULL ) - goto error_out; + return NULL; p = l1[l1_table_offset_pae(va)] >> PAGE_SHIFT; if ( v != NULL ) munmap(v, PAGE_SIZE); v = xc_map_foreign_range(xc_handle, current_domid, PAGE_SIZE, perm, p); if ( v == NULL ) - goto error_out; + return NULL; return (void *)((unsigned long)v | (va & (PAGE_SIZE - 1))); - - error_out: - return NULL; } static void * @@ -215,17 +236,18 @@ map_domain_va( if ( (page_array = malloc(nr_pages * sizeof(unsigned long))) == NULL ) { printf("Could not allocate memory\n"); - goto error_out; + return NULL; } if ( xc_get_pfn_list(xc_handle, current_domid, page_array, nr_pages) != nr_pages ) { printf("Could not get the page frame list\n"); - goto error_out; - } - } - - FETCH_REGS(cpu); + return NULL; + } + } + + if (fetch_regs(xc_handle, cpu, NULL)) + return NULL; if ( ctxt[cpu].ctrlreg[3] != cr3_phys[cpu] ) { @@ -236,10 +258,10 @@ map_domain_va( xc_handle, current_domid, PAGE_SIZE, PROT_READ, cr3_phys[cpu] >> PAGE_SHIFT); if ( cr3_virt[cpu] == NULL ) - goto error_out; + return NULL; } if ( (pde = cr3_virt[cpu][vtopdi(va)]) == 0 ) - goto error_out; + return NULL; if ( (ctxt[cpu].flags & VGCF_HVM_GUEST) && paging_enabled(&ctxt[cpu]) ) pde = page_array[pde >> PAGE_SHIFT] << PAGE_SHIFT; if ( pde != pde_phys[cpu] ) @@ -251,10 +273,10 @@ map_domain_va( xc_handle, current_domid, PAGE_SIZE, PROT_READ, pde_phys[cpu] >> PAGE_SHIFT); if ( pde_virt[cpu] == NULL ) - goto error_out; + return NULL; } if ( (page = pde_virt[cpu][vtopti(va)]) == 0 ) - goto error_out; + return NULL; if ( (ctxt[cpu].flags & VGCF_HVM_GUEST) && paging_enabled(&ctxt[cpu]) ) page = page_array[page >> PAGE_SHIFT] << PAGE_SHIFT; if ( (page != page_phys[cpu]) || (perm != prev_perm[cpu]) ) @@ -268,15 +290,12 @@ map_domain_va( if ( page_virt[cpu] == NULL ) { page_phys[cpu] = 0; - goto error_out; + return NULL; } prev_perm[cpu] = perm; } return (void *)(((unsigned long)page_virt[cpu]) | (va & BSD_PAGE_MASK)); - - error_out: - return NULL; } int @@ -335,7 +354,6 @@ xc_ptrace( long edata) { DECLARE_DOM0_OP; - int status = 0; struct gdb_regs pt; long retval = 0; unsigned long *guest_va; @@ -353,10 +371,7 @@ xc_ptrace( guest_va = (unsigned long *)map_domain_va( xc_handle, cpu, addr, PROT_READ); if ( guest_va == NULL ) - { - status = EFAULT; - goto error_out; - } + goto out_error; retval = *guest_va; break; @@ -365,38 +380,30 @@ xc_ptrace( /* XXX assume that all CPUs have the same address space */ guest_va = (unsigned long *)map_domain_va( xc_handle, cpu, addr, PROT_READ|PROT_WRITE); - if ( guest_va == NULL ) { - status = EFAULT; - goto error_out; - } + if ( guest_va == NULL ) + goto out_error; *guest_va = (unsigned long)data; break; case PTRACE_GETREGS: + if (fetch_regs(xc_handle, cpu, NULL)) + goto out_error; + SET_PT_REGS(pt, ctxt[cpu].user_regs); + memcpy(data, &pt, sizeof(struct gdb_regs)); + break; + case PTRACE_GETFPREGS: case PTRACE_GETFPXREGS: - - FETCH_REGS(cpu); - if ( request == PTRACE_GETREGS ) - { - SET_PT_REGS(pt, ctxt[cpu].user_regs); - memcpy(data, &pt, sizeof(struct gdb_regs)); - } - else if (request == PTRACE_GETFPREGS) - { - memcpy(data, &ctxt[cpu].fpu_ctxt, sizeof(ctxt[cpu].fpu_ctxt)); - } - else /*if (request == PTRACE_GETFPXREGS)*/ - { - memcpy(data, &ctxt[cpu].fpu_ctxt, sizeof(ctxt[cpu].fpu_ctxt)); - } + if (fetch_regs(xc_handle, cpu, NULL)) + goto out_error; + memcpy(data, &ctxt[cpu].fpu_ctxt, sizeof(ctxt[cpu].fpu_ctxt)); break; case PTRACE_SETREGS: SET_XC_REGS(((struct gdb_regs *)data), ctxt[cpu].user_regs); - retval = xc_vcpu_setcontext(xc_handle, current_domid, cpu, &ctxt[cpu]); - if (retval) - goto error_out; + if ((retval = xc_vcpu_setcontext(xc_handle, current_domid, cpu, + &ctxt[cpu]))) + goto out_error_dom0; break; case PTRACE_SINGLESTEP: @@ -404,12 +411,9 @@ xc_ptrace( * during single-stepping - but that just seems retarded */ ctxt[cpu].user_regs.eflags |= PSL_T; - retval = xc_vcpu_setcontext(xc_handle, current_domid, cpu, &ctxt[cpu]); - if ( retval ) - { - perror("dom0 op failed"); - goto error_out; - } + if ((retval = xc_vcpu_setcontext(xc_handle, current_domid, cpu, + &ctxt[cpu]))) + goto out_error_dom0; /* FALLTHROUGH */ case PTRACE_CONT: @@ -418,16 +422,15 @@ xc_ptrace( { FOREACH_CPU(cpumap, index) { cpu = index - 1; - FETCH_REGS(cpu); + if (fetch_regs(xc_handle, cpu, NULL)) + goto out_error; /* Clear trace flag */ - if ( ctxt[cpu].user_regs.eflags & PSL_T ) { + if ( ctxt[cpu].user_regs.eflags & PSL_T ) + { ctxt[cpu].user_regs.eflags &= ~PSL_T; - retval = xc_vcpu_setcontext(xc_handle, current_domid, - cpu, &ctxt[cpu]); - if ( retval ) { - perror("dom0 op failed"); - goto error_out; - } + if ((retval = xc_vcpu_setcontext(xc_handle, current_domid, + cpu, &ctxt[cpu]))) + goto out_error_dom0; } } } @@ -436,10 +439,13 @@ xc_ptrace( op.cmd = DOM0_SETDEBUGGING; op.u.setdebugging.domain = current_domid; op.u.setdebugging.enable = 0; - retval = do_dom0_op(xc_handle, &op); + if ((retval = do_dom0_op(xc_handle, &op))) + goto out_error_dom0; } regs_valid = 0; - xc_domain_unpause(xc_handle, current_domid > 0 ? current_domid : -current_domid); + if ((retval = xc_domain_unpause(xc_handle, current_domid > 0 ? + current_domid : -current_domid))) + goto out_error_dom0; break; case PTRACE_ATTACH: @@ -448,19 +454,16 @@ xc_ptrace( op.u.getdomaininfo.domain = current_domid; retval = do_dom0_op(xc_handle, &op); if ( retval || (op.u.getdomaininfo.domain != current_domid) ) - { - perror("dom0 op failed"); - goto error_out; - } + goto out_error_dom0; if ( op.u.getdomaininfo.flags & DOMFLAGS_PAUSED ) - { printf("domain currently paused\n"); - } else - retval = xc_domain_pause(xc_handle, current_domid); + else if ((retval = xc_domain_pause(xc_handle, current_domid))) + goto out_error_dom0; op.cmd = DOM0_SETDEBUGGING; op.u.setdebugging.domain = current_domid; op.u.setdebugging.enable = 1; - retval = do_dom0_op(xc_handle, &op); + if ((retval = do_dom0_op(xc_handle, &op))) + goto out_error_dom0; if (get_online_cpumap(xc_handle, &op.u.getdomaininfo, &cpumap)) printf("get_online_cpumap failed\n"); @@ -478,21 +481,20 @@ xc_ptrace( printf("unsupported xc_ptrace request %s\n", ptrace_names[request]); #endif /* XXX not yet supported */ - status = ENOSYS; - break; + errno = ENOSYS; + return -1; case PTRACE_TRACEME: printf("PTRACE_TRACEME is an invalid request under Xen\n"); - status = EINVAL; - } - - if ( status ) - { - errno = status; - retval = -1; - } - - error_out: + goto out_error; + } + + return retval; + + out_error_dom0: + perror("dom0 op failed"); + out_error: + errno = EINVAL; return retval; } diff -r 64f11b0e2e7d tools/libxc/xc_ptrace.h --- a/tools/libxc/xc_ptrace.h Sat Mar 4 19:16:36 2006 +0100 +++ b/tools/libxc/xc_ptrace.h Mon Mar 6 14:42:37 2006 +0900 @@ -1,5 +1,7 @@ #ifndef XC_PTRACE_ #define XC_PTRACE_ + +#include #ifdef XC_PTRACE_PRIVATE #define X86_CR0_PE 0x00000001 /* Enable Protected Mode (RW) */ @@ -8,33 +10,7 @@ #define PDRSHIFT 22 #define PSL_T 0x00000100 /* trace enable bit */ -char * ptrace_names[] = { - "PTRACE_TRACEME", - "PTRACE_PEEKTEXT", - "PTRACE_PEEKDATA", - "PTRACE_PEEKUSER", - "PTRACE_POKETEXT", - "PTRACE_POKEDATA", - "PTRACE_POKEUSER", - "PTRACE_CONT", - "PTRACE_KILL", - "PTRACE_SINGLESTEP", - "PTRACE_INVALID", - "PTRACE_INVALID", - "PTRACE_GETREGS", - "PTRACE_SETREGS", - "PTRACE_GETFPREGS", - "PTRACE_SETFPREGS", - "PTRACE_ATTACH", - "PTRACE_DETACH", - "PTRACE_GETFPXREGS", - "PTRACE_SETFPXREGS", - "PTRACE_INVALID", - "PTRACE_INVALID", - "PTRACE_INVALID", - "PTRACE_INVALID", - "PTRACE_SYSCALL", -}; +extern const char const * ptrace_names[]; struct gdb_regs { long ebx; /* 0 */ diff -r 64f11b0e2e7d tools/libxc/xc_ptrace_core.c --- a/tools/libxc/xc_ptrace_core.c Sat Mar 4 19:16:36 2006 +0100 +++ b/tools/libxc/xc_ptrace_core.c Mon Mar 6 14:42:37 2006 +0900 @@ -1,81 +1,12 @@ +#define XC_PTRACE_PRIVATE + #include #include #include "xc_private.h" +#include "xc_ptrace.h" #include -#define BSD_PAGE_MASK (PAGE_SIZE-1) -#define PDRSHIFT 22 #define VCPU 0 /* XXX */ - -/* - * long - * ptrace(enum __ptrace_request request, pid_t pid, void *addr, void *data); - */ - -struct gdb_regs { - long ebx; /* 0 */ - long ecx; /* 4 */ - long edx; /* 8 */ - long esi; /* 12 */ - long edi; /* 16 */ - long ebp; /* 20 */ - long eax; /* 24 */ - int xds; /* 28 */ - int xes; /* 32 */ - int xfs; /* 36 */ - int xgs; /* 40 */ - long orig_eax; /* 44 */ - long eip; /* 48 */ - int xcs; /* 52 */ - long eflags; /* 56 */ - long esp; /* 60 */ - int xss; /* 64 */ -}; - -#define printval(x) printf("%s = %lx\n", #x, (long)x); -#define SET_PT_REGS(pt, xc) \ -{ \ - pt.ebx = xc.ebx; \ - pt.ecx = xc.ecx; \ - pt.edx = xc.edx; \ - pt.esi = xc.esi; \ - pt.edi = xc.edi; \ - pt.ebp = xc.ebp; \ - pt.eax = xc.eax; \ - pt.eip = xc.eip; \ - pt.xcs = xc.cs; \ - pt.eflags = xc.eflags; \ - pt.esp = xc.esp; \ - pt.xss = xc.ss; \ - pt.xes = xc.es; \ - pt.xds = xc.ds; \ - pt.xfs = xc.fs; \ - pt.xgs = xc.gs; \ -} - -#define SET_XC_REGS(pt, xc) \ -{ \ - xc.ebx = pt->ebx; \ - xc.ecx = pt->ecx; \ - xc.edx = pt->edx; \ - xc.esi = pt->esi; \ - xc.edi = pt->edi; \ - xc.ebp = pt->ebp; \ - xc.eax = pt->eax; \ - xc.eip = pt->eip; \ - xc.cs = pt->xcs; \ - xc.eflags = pt->eflags; \ - xc.esp = pt->esp; \ - xc.ss = pt->xss; \ - xc.es = pt->xes; \ - xc.ds = pt->xds; \ - xc.fs = pt->xfs; \ - xc.gs = pt->xgs; \ -} - - -#define vtopdi(va) ((va) >> PDRSHIFT) -#define vtopti(va) (((va) >> PAGE_SHIFT) & 0x3ff) /* XXX application state */ @@ -120,12 +51,12 @@ map_domain_va(unsigned long domfd, int c if (v == MAP_FAILED) { perror("mmap failed"); - goto error_out; + return NULL; } cr3_virt[cpu] = v; } if ((pde = cr3_virt[cpu][vtopdi(va)]) == 0) /* logical address */ - goto error_out; + return NULL; if (ctxt[cpu].flags & VGCF_HVM_GUEST) pde = p2m_array[pde >> PAGE_SHIFT] << PAGE_SHIFT; if (pde != pde_phys[cpu]) @@ -137,11 +68,11 @@ map_domain_va(unsigned long domfd, int c NULL, PAGE_SIZE, PROT_READ, MAP_PRIVATE, domfd, map_mtop_offset(pde_phys[cpu])); if (v == MAP_FAILED) - goto error_out; + return NULL; pde_virt[cpu] = v; } if ((page = pde_virt[cpu][vtopti(va)]) == 0) /* logical address */ - goto error_out; + return NULL; if (ctxt[cpu].flags & VGCF_HVM_GUEST) page = p2m_array[page >> PAGE_SHIFT] << PAGE_SHIFT; if (page != page_phys[cpu]) @@ -152,17 +83,15 @@ map_domain_va(unsigned long domfd, int c v = mmap( NULL, PAGE_SIZE, PROT_READ, MAP_PRIVATE, domfd, map_mtop_offset(page_phys[cpu])); - if (v == MAP_FAILED) { + if (v == MAP_FAILED) + { printf("cr3 %lx pde %lx page %lx pti %lx\n", cr3[cpu], pde, page, vtopti(va)); page_phys[cpu] = 0; - goto error_out; + return NULL; } page_virt[cpu] = v; } return (void *)(((unsigned long)page_virt[cpu]) | (va & BSD_PAGE_MASK)); - - error_out: - return 0; } int @@ -172,12 +101,12 @@ xc_waitdomain_core( int *status, int options) { - int retval = -1; int nr_vcpus; int i; xc_core_header_t header; - if (nr_pages == 0) { + if (nr_pages == 0) + { if (read(domfd, &header, sizeof(header)) != sizeof(header)) return -1; @@ -193,17 +122,19 @@ xc_waitdomain_core( for (i = 0; i < nr_vcpus; i++) { cr3[i] = ctxt[i].ctrlreg[3]; } - if ((p2m_array = malloc(nr_pages * sizeof(unsigned long))) == NULL) { + if ((p2m_array = malloc(nr_pages * sizeof(unsigned long))) == NULL) + { printf("Could not allocate p2m_array\n"); - goto error_out; + return -1; } if (read(domfd, p2m_array, sizeof(unsigned long)*nr_pages) != sizeof(unsigned long)*nr_pages) return -1; - if ((m2p_array = malloc((1<<20) * sizeof(unsigned long))) == NULL) { + if ((m2p_array = malloc((1<<20) * sizeof(unsigned long))) == NULL) + { printf("Could not allocate m2p array\n"); - goto error_out; + return -1; } bzero(m2p_array, sizeof(unsigned long)* 1 << 20); @@ -212,10 +143,7 @@ xc_waitdomain_core( } } - retval = 0; - error_out: - return retval; - + return 0; } long