[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 3/6] hypertrace: [*-user] Add QEMU-side proxy
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH v4 3/6] hypertrace: [*-user] Add QEMU-side proxy to "guest_hypertrace" event |
Date: |
Mon, 9 Jan 2017 15:44:39 +0000 |
User-agent: |
Mutt/1.7.1 (2016-10-04) |
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
- Re: [Qemu-devel] [PATCH v4 3/6] hypertrace: [*-user] Add QEMU-side proxy to "guest_hypertrace" event,
Stefan Hajnoczi <=