Re: [Xen-devel] [PATCH 1/2] x86/monitor: Introduce a boolean to suppress nested monitoring events

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 10/23/18 5:35 PM, Andrew Cooper wrote:
> When applying vm_event actions, monitoring events can nest and effectively
> livelock the vcpu.  Introduce a boolean which is set for a specific portion of
> the hvm_do_resume() path, which causes the hvm_monitor_* helpers to exit
> early.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
> CC: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> 
> RFC - This probably wants wiring up on ARM as well, but all I see is
> monitor_smc() and no equivalent parts to hvm_do_resume() where we may inject
> an exception from a vm_event.  Should this be included in non-x86 monitor
> functions for consistency at this point?
> ---
>  xen/arch/x86/hvm/hvm.c     |  8 ++++++++
>  xen/arch/x86/hvm/monitor.c | 24 ++++++++++++++++++++++--
>  xen/include/xen/sched.h    |  8 ++++++++
>  3 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 9bc8078..4b4d9d6 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -512,6 +512,12 @@ void hvm_do_resume(struct vcpu *v)
>      if ( !handle_hvm_io_completion(v) )
>          return;
>  
> +    /*
> +     * We are about to apply actions requested by the introspection
> +     * agent.  Don't trigger further monitoring.
> +     */
> +    v->monitor.suppress = true;
> +
>      if ( unlikely(v->arch.vm_event) )
>          hvm_vm_event_do_resume(v);
>  
> @@ -526,6 +532,8 @@ void hvm_do_resume(struct vcpu *v)
>          v->arch.hvm.inject_event.vector = HVM_EVENT_VECTOR_UNSET;
>      }
>  
> +    v->monitor.suppress = false;
> +
>      if ( unlikely(v->arch.vm_event) && 
> v->arch.monitor.next_interrupt_enabled )
>      {
>          struct x86_event info;
> diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
> index 2a41ccc..f1a196f 100644
> --- a/xen/arch/x86/hvm/monitor.c
> +++ b/xen/arch/x86/hvm/monitor.c
> @@ -36,6 +36,9 @@ bool hvm_monitor_cr(unsigned int index, unsigned long 
> value, unsigned long old)
>      struct arch_domain *ad = &curr->domain->arch;
>      unsigned int ctrlreg_bitmask = monitor_ctrlreg_bitmask(index);
>  
> +    if ( curr->monitor.suppress )
> +        return 0;
> +
>      if ( index == VM_EVENT_X86_CR3 && hvm_pcid_enabled(curr) )
>          value &= ~X86_CR3_NOFLUSH; /* Clear the noflush bit. */
>  
> @@ -73,6 +76,9 @@ bool hvm_monitor_emul_unimplemented(void)
>          .vcpu_id  = curr->vcpu_id,
>      };
>  
> +    if ( curr->monitor.suppress )
> +        return false;

Rather than doing this for each event, I think we may be able to do it
only in monitor_traps(). Am I missing something?


Thanks,
Razvan

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