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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.09.14 at 12:28, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> Changes since V5:
>  - Now synchronizing access to the trap injection data.
>  - Only enforcing the CR3 match for cr3 != ~0.
>  - Updated the xen-access.c test.

Leaving aside the question of whether this is the right mechanism,
still a couple of comments:

> --- 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_can_inject_domain_pf(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) )

If you take off the masking here ...

> @@ -450,12 +475,30 @@ void hvm_do_resume(struct vcpu *v)
>          }
>      }
>  
> +    spin_lock(&d->arch.hvm_domain.inject_trap_lock);
> +
>      /* Inject pending hw/sw trap */
> -    if ( v->arch.hvm_vcpu.inject_trap.vector != -1 ) 
> +    if ( v->arch.hvm_vcpu.inject_trap.vector != -1 )
>      {
>          hvm_inject_trap(&v->arch.hvm_vcpu.inject_trap);
>          v->arch.hvm_vcpu.inject_trap.vector = -1;
>      }
> +    /*
> +     * Inject per-domain pending hw/sw trap (this will most likely
> +     * be a page fault injected by memory introspection code).
> +     */
> +    else if ( d->arch.hvm_domain.inject_trap.vector != -1 )
> +    {
> +        if ( d->arch.hvm_domain.inject_trap.vector != TRAP_page_fault ||
> +             d->arch.hvm_domain.inject_trap.cr3 == ~0 ||

... you can avoid this check (which is suspicious anyway due to the
hard coded number - it'd be better if that value got a manifest
constant assigned in the public header).

> @@ -1473,9 +1516,11 @@ 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;
> +    spin_lock_init(&d->arch.hvm_domain.inject_trap_lock);
> +
>      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,20 +6131,58 @@ 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 
> +
> +        spin_lock(&d->arch.hvm_domain.inject_trap_lock);
> +
> +        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;
> +            unsigned int i;
> +
> +            for ( i = 0; i < d->max_vcpus; i++ )
> +                if ( (v = d->vcpu[i]) != NULL &&
> +                     v->arch.hvm_vcpu.inject_trap.vector != -1 )
> +                {
> +                    rc = -EBUSY;
> +                    break;
> +                }
> +
> +            if ( d->arch.hvm_domain.inject_trap.vector != -1 )
> +                rc = -EBUSY;
> +
> +            if ( rc != -EBUSY )
> +            {
> +                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 /* Specific VCPU. */
> +        {
> +            if ( tr.vcpuid >= d->max_vcpus || (v = d->vcpu[tr.vcpuid]) == 
> NULL )
> +                goto param_fail8;

You can't do this anymore with a spin lock held.

> +
> +            if ( v->arch.hvm_vcpu.inject_trap.cr3 != ~0 )
> +                rc = -EOPNOTSUPP;
> +            if ( v->arch.hvm_vcpu.inject_trap.vector != -1 ||

This ought to be "else if".

> --- a/xen/include/asm-x86/hvm/domain.h
> +++ b/xen/include/asm-x86/hvm/domain.h
> @@ -146,6 +146,10 @@ 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;
> +    spinlock_t          inject_trap_lock;

Are you sure you want a spin lock here, not an rw one?

Jan

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