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

[Lluís Vilanova]

Stefan Hajnoczi writes:

> On Fri, Aug 05, 2016 at 06:59:39PM +0200, Lluís Vilanova wrote:
>> +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);

> Use g_strdup_printf() instead.

>> +static void swap_control(void *from, void *to)
>> +{
>> +    if (mprotect(from, getpagesize(), PROT_READ | PROT_WRITE) == -1) {
>> +        error_report("error: mprotect(from): %s", strerror(errno));
>> +        abort();
>> +    }
>> +    if (mprotect(to, getpagesize(), PROT_READ) == -1) {
>> +        error_report("error: mprotect(to): %s", strerror(errno));
>> +        abort();
>> +    }
>> +}
>> +
>> +#include "hypertrace/emit.c"
>> +
>> +static void segv_handler(int signum, siginfo_t *siginfo, void *sigctxt)
>> +{
>> +    if (qemu_control_0 <= siginfo->si_addr &&
>> +        siginfo->si_addr < qemu_control_1) {
>> +
>> +        /* 1st fault (guest will write cmd) */
>> +        assert(((unsigned long)siginfo->si_addr % getpagesize()) == 
>> sizeof(uint64_t));
>> +        swap_control(qemu_control_0, qemu_control_1);
>> +
>> +    } else if (qemu_control_1 <= siginfo->si_addr &&
>> +               siginfo->si_addr < qemu_control_1 + getpagesize()) {
>> +        uint64_t vcontrol = ((uint64_t*)qemu_control_0)[2];
>> +        uint64_t *data_ptr = &qemu_data[vcontrol * CONFIG_HYPERTRACE_ARGS * 
>> sizeof(uint64_t)];
>> +
>> +        /* 2nd fault (invoke) */
>> +        assert(((unsigned long)siginfo->si_addr % getpagesize()) == 
>> sizeof(uint64_t));
>> +        hypertrace_emit(current_cpu, data_ptr);
>> +        swap_control(qemu_control_1, qemu_control_0);
>> +
>> +    } 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);
>> +        }
>> +    }
>> +}

> Can this approach be made thread-safe?

> If not then it would be good to consider the problem right away and
> switch to something that is thread-safe, even if it depends on the
> target architecture.

Kind of. The easiest solution is to have each thread have an mmap of its own of
the device (and moving qemu_control_{0,1} into CPUState). This would be
completely explicit and easy to understand.

Cheers,
  Lluis