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: 2717 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]