Re: [Xen-devel] [PATCH V2 4/5] xen, libxc: Request page fault injection via libxc

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Razvan Cojocaru [mailto:rcojocaru@xxxxxxxxxxxxxxx]
> Sent: Wednesday, September 03, 2014 4:21 AM
> 
> Extended HVMOP_inject_trap to allow asking for trap injection done
> by the first available CPU, when it's in user mode and its CR3
> matches the one for an interesting application inside the guest.
> This mechanism allows bringing in swapped-out pages for inspection.
> 
> Signed-off-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
> ---
>  tools/libxc/xc_misc.c               |    5 ++-
>  tools/libxc/xenctrl.h               |    3 +-
>  tools/tests/xen-access/xen-access.c |    2 +-
>  xen/arch/x86/hvm/hvm.c              |   81
> +++++++++++++++++++++++++++++------
>  xen/include/asm-x86/hvm/domain.h    |    3 ++
>  xen/include/asm-x86/hvm/hvm.h       |    1 +
>  xen/include/public/hvm/hvm_op.h     |    2 +
>  7 files changed, 80 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
> index e253a58..6773446 100644
> --- a/tools/libxc/xc_misc.c
> +++ b/tools/libxc/xc_misc.c
> @@ -597,7 +597,7 @@ int xc_hvm_set_mem_type(
>  int xc_hvm_inject_trap(
>      xc_interface *xch, domid_t dom, int vcpu, uint32_t vector,
>      uint32_t type, uint32_t error_code, uint32_t insn_len,
> -    uint64_t cr2)
> +    uint64_t cr2, uint64_t cr3)
>  {
>      DECLARE_HYPERCALL;
>      DECLARE_HYPERCALL_BUFFER(struct xen_hvm_inject_trap, arg);
> @@ -611,12 +611,13 @@ int xc_hvm_inject_trap(
>      }
> 
>      arg->domid       = dom;
> -    arg->vcpuid      = vcpu;
> +    arg->vcpuid      = (vcpu == -1 ? (uint32_t)~0 : vcpu);
>      arg->vector      = vector;
>      arg->type        = type;
>      arg->error_code  = error_code;
>      arg->insn_len    = insn_len;
>      arg->cr2         = cr2;
> +    arg->cr3         = cr3;
> 
>      hypercall.op     = __HYPERVISOR_hvm_op;
>      hypercall.arg[0] = HVMOP_inject_trap;
> diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
> index 28b5562..5bf0173 100644
> --- a/tools/libxc/xenctrl.h
> +++ b/tools/libxc/xenctrl.h
> @@ -1816,11 +1816,12 @@ int xc_hvm_set_mem_type(
>  /*
>   * Injects a hardware/software CPU trap, to take effect the next time the
> HVM
>   * resumes.
> + * Cr3 is only taken into account if vcpu == -1 (wildcard for "any vcpu").
>   */
>  int xc_hvm_inject_trap(
>      xc_interface *xch, domid_t dom, int vcpu, uint32_t vector,
>      uint32_t type, uint32_t error_code, uint32_t insn_len,
> -    uint64_t cr2);
> +    uint64_t cr2, uint64_t cr3);
> 
>  /*
>   *  LOGGING AND ERROR REPORTING
> diff --git a/tools/tests/xen-access/xen-access.c
> b/tools/tests/xen-access/xen-access.c
> index 090df5f..34c53d2 100644
> --- a/tools/tests/xen-access/xen-access.c
> +++ b/tools/tests/xen-access/xen-access.c
> @@ -601,7 +601,7 @@ int main(int argc, char *argv[])
>                  /* Reinject */
>                  rc = xc_hvm_inject_trap(
>                      xch, domain_id, req.vcpu_id, 3,
> -                    HVMOP_TRAP_sw_exc, -1, 0, 0);
> +                    HVMOP_TRAP_sw_exc, -1, 0, 0, 0);
>                  if (rc < 0)
>                  {
>                      ERROR("Error %d injecting int3\n", rc);
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 5761ff9..5d3e4d4 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -420,6 +420,31 @@ static bool_t hvm_wait_for_io(struct
> hvm_ioreq_vcpu *sv, ioreq_t *p)
>      return 1;
>  }
> 
> +static bool_t hvm_is_pf_requested(struct vcpu *v)
> +{
> +    const struct domain *d = v->domain;
> +    struct segment_register seg;
> +    uint64_t mask;
> +
> +    hvm_get_segment_register(v, x86_seg_ss, &seg);
> +
> +    if ( seg.attr.fields.dpl != 3 ) /* Guest is not in user mode */
> +        return 0;
> +
> +    if ( hvm_long_mode_enabled(v) )
> +        mask = PADDR_MASK & PAGE_MASK; /* Bits 51:12. */
> +    else if ( hvm_pae_enabled(v) )
> +        mask = 0x00000000ffffffe0; /* Bits 31:5. */
> +    else
> +        mask = (uint32_t)PAGE_MASK; /* Bits 31:12. */
> +
> +    if ( (v->arch.hvm_vcpu.guest_cr[3] & mask) !=
> +         (d->arch.hvm_domain.inject_trap.cr3 & mask) )
> +        return 0;
> +
> +    return 1;
> +}
> +
>  void hvm_do_resume(struct vcpu *v)
>  {
>      struct domain *d = v->domain;
> @@ -451,6 +476,15 @@ void hvm_do_resume(struct vcpu *v)
>      }
> 
>      /* Inject pending hw/sw trap */

you want to make comment more introspection related.

> +    if ( d->arch.hvm_domain.inject_trap.vector != -1 &&
> +         v->arch.hvm_vcpu.inject_trap.vector == -1 &&
> +         hvm_is_pf_requested(v) )
> +    {
> +        hvm_inject_trap(&d->arch.hvm_domain.inject_trap);
> +        d->arch.hvm_domain.inject_trap.vector = -1;
> +    }
> +
> +    /* Inject pending hw/sw trap */
>      if ( v->arch.hvm_vcpu.inject_trap.vector != -1 )
>      {
>          hvm_inject_trap(&v->arch.hvm_vcpu.inject_trap);
> @@ -1473,9 +1507,10 @@ int hvm_domain_initialise(struct domain *d)
>              printk(XENLOG_G_INFO "PVH guest must have HAP on\n");
>              return -EINVAL;
>          }
> -
>      }
> 
> +    d->arch.hvm_domain.inject_trap.vector = -1;
> +

A question here. With new design, now you have two places which may cache
fault injection information. If unfortunately two consecutive hypercalls with
one having vcpuid=-1 and the other having vcpuid=n, it's possible two injections
will be handled together if same vcpu is chosen for two pending injections.

I think the hypercall needs a check of all previous pending requests, not only
in domain specific structure as what you did in this version.

>      spin_lock_init(&d->arch.hvm_domain.ioreq_server.lock);
>      INIT_LIST_HEAD(&d->arch.hvm_domain.ioreq_server.list);
>      spin_lock_init(&d->arch.hvm_domain.irq_lock);
> @@ -6086,19 +6121,39 @@ long do_hvm_op(unsigned long op,
> XEN_GUEST_HANDLE_PARAM(void) arg)
>              goto param_fail8;
> 
>          rc = -ENOENT;
> -        if ( tr.vcpuid >= d->max_vcpus || (v = d->vcpu[tr.vcpuid]) == NULL )
> -            goto param_fail8;
> -
> -        if ( v->arch.hvm_vcpu.inject_trap.vector != -1 )
> -            rc = -EBUSY;
> -        else
> +
> +        if ( tr.vcpuid == (uint32_t)~0 ) /* Any VCPU. */
>          {
> -            v->arch.hvm_vcpu.inject_trap.vector = tr.vector;
> -            v->arch.hvm_vcpu.inject_trap.type = tr.type;
> -            v->arch.hvm_vcpu.inject_trap.error_code = tr.error_code;
> -            v->arch.hvm_vcpu.inject_trap.insn_len = tr.insn_len;
> -            v->arch.hvm_vcpu.inject_trap.cr2 = tr.cr2;
> -            rc = 0;
> +            if ( d->arch.hvm_domain.inject_trap.vector != -1 )
> +                rc = -EBUSY;
> +            else
> +            {
> +                d->arch.hvm_domain.inject_trap.vector = tr.vector;
> +                d->arch.hvm_domain.inject_trap.type = tr.type;
> +                d->arch.hvm_domain.inject_trap.error_code =
> tr.error_code;
> +                d->arch.hvm_domain.inject_trap.insn_len = tr.insn_len;
> +                d->arch.hvm_domain.inject_trap.cr2 = tr.cr2;
> +                d->arch.hvm_domain.inject_trap.cr3 = tr.cr3;
> +                rc = 0;
> +            }
> +        }
> +        else
> +        {
> +            if ( tr.vcpuid >= d->max_vcpus || (v = d->vcpu[tr.vcpuid]) ==
> NULL )
> +                goto param_fail8;
> +
> +            if ( v->arch.hvm_vcpu.inject_trap.vector != -1 )
> +                rc = -EBUSY;
> +            else
> +            {
> +                v->arch.hvm_vcpu.inject_trap.vector = tr.vector;
> +                v->arch.hvm_vcpu.inject_trap.type = tr.type;
> +                v->arch.hvm_vcpu.inject_trap.error_code = tr.error_code;
> +                v->arch.hvm_vcpu.inject_trap.insn_len = tr.insn_len;
> +                v->arch.hvm_vcpu.inject_trap.cr2 = tr.cr2;
> +                v->arch.hvm_vcpu.inject_trap.cr3 = tr.cr3;
> +                rc = 0;
> +            }
>          }
> 
>      param_fail8:
> diff --git a/xen/include/asm-x86/hvm/domain.h
> b/xen/include/asm-x86/hvm/domain.h
> index 30d4aa3..b432874 100644
> --- a/xen/include/asm-x86/hvm/domain.h
> +++ b/xen/include/asm-x86/hvm/domain.h
> @@ -146,6 +146,9 @@ struct hvm_domain {
>          struct vmx_domain vmx;
>          struct svm_domain svm;
>      };
> +
> +    /* Pending hw/sw interrupt (.vector = -1 means nothing pending). */
> +    struct hvm_trap     inject_trap;
>  };
> 
>  #define hap_enabled(d)  ((d)->arch.hvm_domain.hap_enabled)
> diff --git a/xen/include/asm-x86/hvm/hvm.h
> b/xen/include/asm-x86/hvm/hvm.h
> index 121d053..3b0bde9 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -78,6 +78,7 @@ struct hvm_trap {
>      int           error_code;   /* HVM_DELIVER_NO_ERROR_CODE if
> n/a */
>      int           insn_len;     /* Instruction length */
>      unsigned long cr2;          /* Only for TRAP_page_fault h/w
> exception */
> +    unsigned long cr3;          /* Only for TRAP_page_fault h/w
> exception */
>  };
> 
>  /*
> diff --git a/xen/include/public/hvm/hvm_op.h
> b/xen/include/public/hvm/hvm_op.h
> index eeb0a60..5c229b7 100644
> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -197,6 +197,8 @@ struct xen_hvm_inject_trap {
>      uint32_t insn_len;
>      /* CR2 for page faults */
>      uint64_aligned_t cr2;
> +    /* If vcpuid == -1, any CPU with a matching CR3 will inject. */

the comment looks a bit abrupt when you mention 'vcpuid' here.

> +    uint64_aligned_t cr3;
>  };
>  typedef struct xen_hvm_inject_trap xen_hvm_inject_trap_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_hvm_inject_trap_t);
> --
> 1.7.9.5


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel