qemu-devel
[Top][All Lists]
Advanced

[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: Tue, 10 Jan 2017 14:47:54 +0000
User-agent: Mutt/1.7.1 (2016-10-04)

On Mon, Jan 09, 2017 at 07:20:07PM +0100, Lluís Vilanova wrote:
> Stefan Hajnoczi writes:
> 
> > On Mon, Dec 26, 2016 at 09:34:54PM +0100, Lluís Vilanova wrote:
> >> +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);
> >> +        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];
> >> +
> >> +        /* 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);
> 
> > A simpler and faster approach is to permanently mprotect just one region
> > and load all arguments from data[] (including the first argument).  Then
> > swapping isn't necessary.
> 
> I'm don't understand what you propose.
> 
> With a single protected region, you don't know when to restore protection of 
> it
> so that later accesses will be detected too. That could be solved if we used
> single-stepping (maybe that's what you meant):
> 
> * trap access
> * unprotect memory region
> * single-step guest
> * read written data and emit event
> * protect memory region again
> * resume guest
> 
> If the single-stepping can be done without too much complexity, that'd be a
> faster option, and that piece of code might be cleaner too.
> 
> We could only avoid the protect/unprotect sequence if we added target-specific
> code to "skip" the failed instruction (assuming all useful writes go to the 
> data
> channel), but I wanted to make all code target-agnostic.

Okay, I didn't realize the instruction would be restarted.  I thought
swapping was solely to allow the guest to write vcontrol =
control_0[client].

Assuming for a second that the instruction isn't restarted, my
suggestion was to use si_addr to identify which client and then load all
args out of the (read/write) data region.  This eliminates vcontrol.

Stefan

Attachment: signature.asc
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]