Re: [PATCH RFC V4 5/5] xen: Handle resumed instruction based on previous mem_event reply

["Jan Beulich" <JBeulich@suse.com>]

>>> On 04.08.14 at 13:30, <rcojocaru@bitdefender.com> wrote:
> In a scenario where a page fault that triggered a mem_event occured,
> p2m_mem_access_check() will now be able to either 1) emulate the
> current instruction, or 2) emulate it, but don't allow it to perform
> any writes.
> 
> Changes since V1:
>  - Removed the 'skip' code which required computing the current
>    instruction length.
>  - Removed the set_ad_bits() code that attempted to modify the
>    'accessed' and 'dirty' bits for instructions that the emulator
>    can't handle at the moment.
> 
> Changes since V2:
>  - Moved the __vmread(EXIT_QUALIFICATION, &exit_qualification); code
>    in vmx.c, accessible via hvm_funcs.
>  - Incorporated changes by Andrew Cooper ("[PATCH 1/2] Xen/mem_event:
>    Validate the response vcpu_id before acting on it."
> 
> Changes since V3:
>  - Collapsed verbose lines into a single "else if()".
>  - Changed an int to bool_t.
>  - Fixed a minor coding style issue.
>  - Now computing the first parameter to hvm_emulate_one_full()
>    (replaced an "if" with a single call).
>  - Added code comments about eip and gla reset (clarity issue).
>  - Removed duplicate code by Andrew Cooper (introduced in V2,
>    since committed).
> 
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> ---
>  xen/arch/x86/domain.c          |    3 ++
>  xen/arch/x86/hvm/vmx/vmx.c     |   13 ++++++
>  xen/arch/x86/mm/p2m.c          |   85 
> ++++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-x86/domain.h   |    9 +++++
>  xen/include/asm-x86/hvm/hvm.h  |    2 +
>  xen/include/public/mem_event.h |   12 +++---
>  6 files changed, 119 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index e896210..af9b213 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -407,6 +407,9 @@ int vcpu_initialise(struct vcpu *v)
>  
>      v->arch.flags = TF_kernel_mode;
>  
> +    /* By default, do not emulate */
> +    v->arch.mem_event.emulate_flags = 0;
> +
>      rc = mapcache_vcpu_init(v);
>      if ( rc )
>          return rc;
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index c0e3d73..150fe9f 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1698,6 +1698,18 @@ static void vmx_enable_intro_msr_interception(struct 
> domain *d)
>      }
>  }
>  
> +static bool_t vmx_exited_by_pagefault(void)
> +{
> +    unsigned long exit_qualification;
> +
> +    __vmread(EXIT_QUALIFICATION, &exit_qualification);
> +
> +    if ( (exit_qualification & EPT_GLA_FAULT) == 0 )
> +        return 0;
> +
> +    return 1;
> +}
> +
>  static struct hvm_function_table __initdata vmx_function_table = {
>      .name                 = "VMX",
>      .cpu_up_prepare       = vmx_cpu_up_prepare,
> @@ -1757,6 +1769,7 @@ static struct hvm_function_table __initdata 
> vmx_function_table = {
>      .nhvm_hap_walk_L1_p2m = nvmx_hap_walk_L1_p2m,
>      .hypervisor_cpuid_leaf = vmx_hypervisor_cpuid_leaf,
>      .enable_intro_msr_interception = vmx_enable_intro_msr_interception,
> +    .exited_by_pagefault  = vmx_exited_by_pagefault,
>  };
>  
>  const struct hvm_function_table * __init start_vmx(void)
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 069e869..da1bc2d 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1391,6 +1391,7 @@ bool_t p2m_mem_access_check(paddr_t gpa, bool_t 
> gla_valid, unsigned long gla,
>      p2m_access_t p2ma;
>      mem_event_request_t *req;
>      int rc;
> +    unsigned long eip = guest_cpu_user_regs()->eip;
>  
>      /* First, handle rx2rw conversion automatically.
>       * These calls to p2m->set_entry() must succeed: we have the gfn
> @@ -1443,6 +1444,36 @@ bool_t p2m_mem_access_check(paddr_t gpa, bool_t 
> gla_valid, unsigned long gla,
>              return 1;
>          }
>      }
> +    else if ( hvm_funcs.exited_by_pagefault && !hvm_funcs.exited_by_pagefault() ) /* don't send a mem_event */

DYM

    else if ( !hvm_funcs.exited_by_pagefault || !hvm_funcs.exited_by_pagefault() )

Apart from that the line is too long and ...

> +    {
> +        if ( v->arch.mem_event.emulate_flags == 0 )

... the two if()s should again be combined.

> +        {
> +            v->arch.mem_event.emulate_flags = MEM_EVENT_FLAG_EMULATE;
> +            v->arch.mem_event.gpa = gpa;
> +            v->arch.mem_event.eip = eip;
> +        }
> +    }
> +
> +    /* The previous mem_event reply does not match the current state. */
> +    if ( v->arch.mem_event.gpa != gpa || v->arch.mem_event.eip != eip )
> +    {
> +        /* Don't emulate the current instruction, send a new mem_event. */
> +        v->arch.mem_event.emulate_flags = 0;
> +
> +        /* Make sure to mark the current state to match it again against
> +         * the new mem_event about to be sent. */
> +        v->arch.mem_event.gpa = gpa;
> +        v->arch.mem_event.eip = eip;
> +    }
> +
> +    if ( v->arch.mem_event.emulate_flags )
> +    {
> +        hvm_emulate_one_full((v->arch.mem_event.emulate_flags & MEM_EVENT_FLAG_EMULATE_NOWRITE) != 0,

Again too long a line.

> @@ -1495,6 +1526,60 @@ void p2m_mem_access_resume(struct domain *d)
>  
>          v = d->vcpu[rsp.vcpu_id];
>  
> +        /* Mark vcpu for skipping one instruction upon rescheduling */
> +        if ( rsp.flags & MEM_EVENT_FLAG_EMULATE )
> +        {
> +            xenmem_access_t access;
> +            bool_t violation = 1;
> +
> +            v->arch.mem_event.emulate_flags = 0;
> +
> +            if ( p2m_get_mem_access(d, rsp.gfn, &access) == 0 )
> +            {
> +                violation = 0;

This belongs ...

> +
> +                switch ( access )
> +                {
> +                case XENMEM_access_n:
> +                case XENMEM_access_n2rwx:
> +                default:
> +                    violation = rsp.access_r || rsp.access_w || rsp.access_x;
> +                    break;
> +
> +                case XENMEM_access_r:
> +                    violation = rsp.access_w || rsp.access_x;
> +                    break;
> +
> +                case XENMEM_access_w:
> +                    violation = rsp.access_r || rsp.access_x;
> +                    break;
> +
> +                case XENMEM_access_x:
> +                    violation = rsp.access_r || rsp.access_w;
> +                    break;
> +
> +                case XENMEM_access_rx:
> +                case XENMEM_access_rx2rw:
> +                    violation = rsp.access_w;
> +                    break;
> +
> +                case XENMEM_access_wx:
> +                    violation = rsp.access_r;
> +                    break;
> +
> +                case XENMEM_access_rw:
> +                    violation = rsp.access_x;
> +                    break;
> +
> +                case XENMEM_access_rwx:

... here, as all other cases set violation anyway.

> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -207,6 +207,8 @@ struct hvm_function_table {
>                                    uint32_t *ecx, uint32_t *edx);
>  
>      void (*enable_intro_msr_interception)(struct domain *d);
> +
> +    bool_t (*exited_by_pagefault)(void);

The naming needs improvement, since afaiu you're not caring about
ordinary page faults, but only nested ones.

Jan