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 05:53 PM, Razvan Cojocaru wrote:
> 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     }
Actually, on further talking with Andrew, he is right - this
hvm_inject_event() call should _not_ trigger a vm_event if it is a
result of calling xc_hvm_inject_trap() - which is the current behaviour.

The "simplest" solution I can think of is to somehow record that this
injection has been caused by xc_hvm_inject_trap() (i.e. deliberately
caused by an external agent).


Thanks,
Razvan

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