Re: [Xen-devel] [PATCH 5 of 6] REDO: mem_access & mem_access 2: added INT3/CRx capture

[Tim Deegan <Tim.Deegan@xxxxxxxxxx>]

Hi, 

At 22:07 +0000 on 04 Jan (1294178845), Joe Epstein wrote:
> diff -r 3500724e8052 -r c1866aff7a5f xen/arch/x86/hvm/hvm.c
> --- a/xen/arch/x86/hvm/hvm.c    Tue Jan 04 12:34:06 2011 -0800
> +++ b/xen/arch/x86/hvm/hvm.c    Tue Jan 04 12:35:49 2011 -0800
> @@ -309,6 +309,15 @@
>              return; /* bail */
>          }
>      }
> +
> +    /* Inject pending hw/sw trap */
> +    if (v->arch.hvm_vcpu.inject_trap != -1)
> +    {
> +        hvm_inject_exception(v->arch.hvm_vcpu.inject_trap,
> +                             v->arch.hvm_vcpu.inject_error_code,
> +                             v->arch.hvm_vcpu.inject_cr2);
> +        v->arch.hvm_vcpu.inject_trap = -1;
> +    }

What if the guest already has a trap pending for some reason?  This
could turn it into a double-fault, which is probably not what the caller
wanted. 

>  }
> 
>  static void hvm_init_ioreq_page(
> @@ -949,6 +958,8 @@
>      spin_lock_init(&v->arch.hvm_vcpu.tm_lock);
>      INIT_LIST_HEAD(&v->arch.hvm_vcpu.tm_list);
> 
> +    v->arch.hvm_vcpu.inject_trap = -1;
> +
>  #ifdef CONFIG_COMPAT
>      rc = setup_compat_arg_xlat(v);
>      if ( rc != 0 )
> @@ -3216,10 +3227,34 @@
>              case HVM_PARAM_ACPI_IOPORTS_LOCATION:
>                  rc = pmtimer_change_ioport(d, a.value);
>                  break;
> +            case HVM_PARAM_MEMORY_EVENT_INT3:
> +                if ( a.value & HVMPME_onchangeonly )
> +                    rc = -EINVAL;
> +                break;
>              }
> 
> -            if ( rc == 0 )
> +            if ( rc == 0 )
> +            {
>                  d->arch.hvm_domain.params[a.index] = a.value;
> +
> +                switch( a.index )
> +                {
> +                case HVM_PARAM_MEMORY_EVENT_INT3:
> +                {
> +                    domain_pause(d);

You need to make sure a VM can't set this param on itself, or else this
domain_pause() can deadlock Xen.   Also, you probably don't want the
daomin turning these intercepts off anyway. :)

Now that I think of it, it's worth checking your other hypercalls to
check that a VM can't change its own p2m mapping types.

[...]

> @@ -1589,13 +1601,25 @@
>      switch ( cr )
>      {
>      case 0:
> -        return !hvm_set_cr0(value);
> +        old = v->arch.hvm_vcpu.guest_cr[0];
> +        rc = !hvm_set_cr0(value);
> +        if (rc)
> +            hvm_memory_event_cr0(value, old);

You defined the hvm_memory_event_foo() functions to return error codes
but you don't handle them in the caller.  Please either handle them or
arrange for the functions to return void. 

Cheers,

Tim.

-- 
Tim Deegan <Tim.Deegan@xxxxxxxxxx>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel