Re: [Xen-devel] [PATCH 08/11] x86/hvm: RFC - PROBABLY BROKEN - Defer all debugging/monitor actions to {svm, vmx}_inject_event()

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 06/04/2018 04:59 PM, Andrew Cooper wrote:
> Currently, there is a lot of functionality in the #DB intercepts, and some
> repeated functionality in the *_inject_event() logic.
> 
> The gdbsx code is implemented at both levels (albeit differently for #BP,
> which is presumably due to the fact that the old emulator behaviour used to be
> to move %rip forwards for traps), while the monitor behaviour only exists at
> the intercept level.
> 
> Updating of %dr6 is implemented (buggily) at both levels, but having it at
> both levels is problematic to implement correctly.
> 
> Rearrange the logic to have nothing interesting at the intercept level, and
> everything implemented at the injection level.  Amongst other things, this
> means that the monitor subsystem will pick up debug actions from emulated
> events.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Jun Nakajima <jun.nakajima@xxxxxxxxx>
> CC: Kevin Tian <kevin.tian@xxxxxxxxx>
> CC: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> CC: Brian Woods <brian.woods@xxxxxxx>
> CC: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
> CC: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
> 
> This is RFC because it probably breaks introspection, as injection replies
> from the introspection engine will (probably, but I haven't confirmed) trigger
> new monitor events.
> 
> First of all, monitoring emulated debug events is a change in behaviour,
> although IMO it is more of a bugfix than a new feature.  Also, similar changes
> will happen to other monitored events as we try to unify the
> intercept/emulation paths.
> 
> As for the recursive triggering of monitor events, I was considering extending
> the monitor infrastructure to have a "in monitor reply" boolean which causes
> hvm_monitor_debug() to ignore the recursive request.
> 
> Does this plan sound ok, or have I missed something more subtle with monitor
> handling?

The plan does sound OK, but I'm not convinced the problem is real: the
only way an introspection agent can inject something that I'm aware of
is via xc_hvm_inject_trap() (which admittedly we do use).

But calling xc_hvm_inject_trap() does not lead to an immediate
injection. Instead, the information is only recorded, and the action is
taken if possible only in hvm_do_resume() - which gets called only after
a VCPU-pause-causing vm_event has been handled:

 509 void hvm_do_resume(struct vcpu *v)
 510 {
 511     check_wakeup_from_wait();
 512
 513     pt_restore_timer(v);
 514
 515     if ( !handle_hvm_io_completion(v) )
 516         return;
 517
 518     if ( unlikely(v->arch.vm_event) )
 519         hvm_vm_event_do_resume(v);
 520
 521     /* Inject pending hw/sw event */
 522     if ( v->arch.hvm_vcpu.inject_event.vector >= 0 )
 523     {
 524         smp_rmb();
 525
 526         if ( !hvm_event_pending(v) )
 527             hvm_inject_event(&v->arch.hvm_vcpu.inject_event);
 528
 529         v->arch.hvm_vcpu.inject_event.vector = HVM_EVENT_VECTOR_UNSET;
 530     }

An example of this is the breakpoint events test in xen-access.c:

762             case VM_EVENT_REASON_SOFTWARE_BREAKPOINT:
763                 printf("Breakpoint: rip=%016"PRIx64", gfn=%"PRIx64"
(vcpu %d)\n",
764                        req.data.regs.x86.rip,
765                        req.u.software_breakpoint.gfn,
766                        req.vcpu_id);
767
768                 /* Reinject */
769                 rc = xc_hvm_inject_trap(xch, domain_id, req.vcpu_id,
770                                         X86_TRAP_INT3,
771
req.u.software_breakpoint.type, -1,
772
req.u.software_breakpoint.insn_length, 0);
773                 if (rc < 0)
774                 {
775                     ERROR("Error %d injecting breakpoint\n", rc);
776                     interrupted = -1;
777                     continue;
778                 }
779                 break;

I did try to apply your series and test it as much as I can think of
(several times), but for reasons which escape me I found that it doesn't
apply cleanly on either the current staging or master (starting with
patch 6). Maybe something's going wrong in my email client.


Thanks,
Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel