Re: [Qemu-devel] [PATCH v4 3/6] hypertrace: [*-user] Add QEMU-side proxy to "guest_hypertrace" event

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4860 bytes --]

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)?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]