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
signature.asc
Description: PGP signature
