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

[Joe Epstein <jepstein98@xxxxxxxxx>]

One question in line.

Thanks

On Wed, Jan 5, 2011 at 6:39 AM, Tim Deegan <Tim.Deegan@xxxxxxxxxx> wrote:

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.

Hmm...it doesn't seem like there can be two different injected events, as this will overwrite whatever else was set.  If the guest was trying to service a trap and took a VM exit, then injection should be the same as if it happened natively.  So I think I'm missing something here.

Also, because the injection only takes effect on the next entry, the odds are good that the caller knows the VCPU was paused, and might have a better sense of the state.  The hypercall is most useful on a memory event handler registered for INT3, having to inject the INT3 to whatever debugger is running in the guest that needs to handle it. 

>  }
>
>  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