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

[Lluís Vilanova]

Stefan Hajnoczi writes:

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

It's hypertrace_base, yes.


>> +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.

Ummm, so right. Although I think only host page alignment is required (there's
no soft TLB in user-mode, right?).


>> +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).

By cleanly you mean exit with a non-zero code, right? It still is an error that
cannot be recovered.

Also, if this goes with exit() what about the abort()s I have added in other
places? (e.g., on a failed call to sigaction)


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

Yes, I should reflect the signal back to the guest.


>> +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.

Yes, that's so embarrassing.


>> +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

Got it.


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

Ummmm, these are values passed to the trace emitter, so either is "correct".

But for the sake of people's sanity when looking at traces, it might be better
to always swap to host endianness.


>> +
>> +        /* 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?

Dang, with the version changes I forgot to add the per-client padding to place
each on a separate page.


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

Yes, before calling hypertrace_init() or if it is called without a
"hypertrace_base" argument set (i.e., the user has not enabled hypertrace in the
command line).


Thanks,
  Lluis