Re: [Xen-users] xc_hvm_inject_trap() failing for int3 traps under Xen 4.2.2

[Tim Deegan <tim@xen.org>]

(Cc-ing a few more people, moving xen-users to Bcc)

At 14:51 +0000 on 15 Jun (1371307878), Antony Saba wrote:
> >> 2) xc_hvm_inject_trap() always returns a negative value, even when there
> >> is not a problem and the guest receives the trap as expected.  There
> >> hasn't been a clarification as to whether it's supposed to return
> >> non-negative, but one would assume that it should because of the way the
> >> xen-access.c example checks for it.
> > 
> > That looks like a hypervisor bug to me: does this (untested) patch fix
> > it for you?
> > 
> > commit 67b9272fcedcb5dc73cc77a2adf580f2572117d7
> > Author: Tim Deegan <tim@xen.org>
> > Date:   Mon Jun 10 19:35:34 2013 +0100
> > 
> >     x86/hvm: Fix HVMOP_inject_trap return value on success.
> >     
> >     Reported-by: Antony Saba <Antony.Saba@mandiant.com>
> >     Signed-off-by: Tim Deegan <tim@xen.org>
> > 
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index ce44bff..6c86fc2 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -4430,6 +4430,7 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
> >              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;
> >          }
> >  
> >      param_fail8:
> > 
> > 
> > 
> 
> This works

Thanks.  I'll take that as a Tested-by:.

Keir, Jan, can I get an Ack?

George, this is a clean bug-fix for something seen in the field, in a
path that doesn't affect any other features.  OK before 4.3?

Jan, this is a candidate for backporting to 4.1.

> but the instruction size must be set to 1, at least on 4.2.2
> to work for me.  Here is the patch against RELEASE-4.2.2.

Sorry, I wasn't clear: setting it to 1 is certainly an improvement over
zero (which is always wrong), but if you're relying on this to be
correct you should also handle cases where prefixes make the instruction
longer than 1 byte.

In any case, this tools-side change needs to be acked/nacked by a tools
maintainer, and probably Aravindh too.

Cheers,

Tim.

> diff --git a/tools/tests/xen-access/xen-access.c
> b/tools/tests/xen-access/xen-access.c
> index 9ec7332..8bcd88b 100644
> --- a/tools/tests/xen-access/xen-access.c
> +++ b/tools/tests/xen-access/xen-access.c
> @@ -664,7 +664,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, 1, 0);
>                  if (rc < 0)
>                  {
>                      ERROR("Error %d injecting int3\n", rc);