On Mon, Dec 26, 2016 at 09:34:54PM +0100, Lluís Vilanova wrote: > @@ -847,6 +855,10 @@ int main(int argc, char **argv) > } else if (!strcmp(r, "trace")) { > g_free(trace_file); > trace_file = trace_opt_parse(optarg); > + } else if (!strcmp(r, "hypertrace")) { > + g_free(hypertrace_file);
This variable hasn't been declared yet. Perhaps it's in a later patch. Please reorder things to avoid the compilation error. Or was this supposed to be hypertrace_base? > +void hypertrace_init_config(struct hypertrace_config *config, > + unsigned int max_clients) > +{ > + config->max_clients = max_clients; > + config->client_args = CONFIG_HYPERTRACE_ARGS; > + config->client_data_size = config->client_args * sizeof(uint64_t); > + config->control_size = QEMU_ALIGN_UP( > + config->max_clients * sizeof(uint64_t), TARGET_PAGE_SIZE); This needs to be host page size aligned, too. Otherwise protect will affect bytes beyond the end of the control region. > +static void init_channel(const char *base, const char *suffix, size_t size, > + char **path, int *fd, uint64_t **addr) > +{ > + *path = g_malloc(strlen(base) + strlen(suffix) + 1); > + sprintf(*path, "%s%s", base, suffix); > + > + *fd = open(*path, O_CREAT | O_EXCL | O_RDWR, S_IRUSR | S_IWUSR); > + if (*fd == -1) { > + error_report("error: open(%s): %s", *path, strerror(errno)); > + abort(); > + } open() can fail for reasons outside QEMU's control. This isn't an internal error. Please exit cleanly instead of using abort(3). > +void hypertrace_init(const char *base, unsigned int max_clients) > +{ > + struct sigaction sigint; > + struct hypertrace_config *pconfig; > + > + if (base == NULL) { > + return; > + } > + > + memset(&sigint, 0, sizeof(sigint)); > + sigint.sa_sigaction = fini_handler; > + sigint.sa_flags = SA_SIGINFO | SA_RESTART; > + if (sigaction(SIGINT, &sigint, NULL) != 0) { > + error_report("error: sigaction(SIGINT): %s", strerror(errno)); > + abort(); > + } > + if (sigaction(SIGABRT, &sigint, NULL) != 0) { > + error_report("error: sigaction(SIGABRT): %s", strerror(errno)); > + abort(); > + } I don't know whether it's okay to set up signal handlers in user mode. Will this break guest code SIGINT/SIGABRT handling? > +bool hypertrace_guest_mmap_check(int fd, unsigned long len, > + unsigned long offset) > +{ > + struct stat s; > + if (fstat(fd, &s) < 0) { > + return true; Should this be return false? > + } > + > + if (s.st_dev != control_fd_stat.st_dev || > + s.st_ino != control_fd_stat.st_ino) { > + return true; Here too. > +static void segv_handler(int signum, siginfo_t *siginfo, void *sigctxt) > +{ > + CPUState *vcpu = current_cpu; > + void *control_0 = vcpu->hypertrace_control; > + void *control_1 = vcpu->hypertrace_control + config.control_size / 2; > + void *control_2 = control_1 + config.control_size / 2; > + > + if (control_0 <= siginfo->si_addr && siginfo->si_addr < control_1) { > + > + /* 1st fault (guest will write cmd) */ > + assert(((unsigned long)siginfo->si_addr % sizeof(uint64_t)) == 0); Please use uintptr_t instead of unsigned long. It's more portable because it doesn't assume sizeof(void*) == sizeof(unsigned long). On Windows the 64-bit data model is LLP64, not LP64: https://en.wikipedia.org/wiki/64-bit_computing#64-bit_data_models > + swap_control(control_0, control_1); > + > + } else if (control_1 <= siginfo->si_addr && siginfo->si_addr < > control_2) { > + size_t client = (siginfo->si_addr - control_1) / sizeof(uint64_t); > + uint64_t vcontrol = ((uint64_t *)control_0)[client]; > + uint64_t *data_ptr = &qemu_data[client * config.client_data_size]; Is byte swapping required? > + > + /* 2nd fault (invoke) */ > + assert(((unsigned long)siginfo->si_addr % sizeof(uint64_t)) == 0); > + hypertrace_emit(current_cpu, vcontrol, data_ptr); > + swap_control(control_1, control_0); I don't understand how this scheme works for multi-threaded programs. If two threads are both writing at the same time can we miss events due to swap_control() changing mprotect? > + > + } else { > + /* proxy to next handler */ > + if (segv_next.sa_sigaction != NULL) { > + segv_next.sa_sigaction(signum, siginfo, sigctxt); > + } else if (segv_next.sa_handler != NULL) { > + segv_next.sa_handler(signum); > + } Is there a case when no signal handler was installed (i.e. default action)?
signature.asc
Description: PGP signature