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

[Keir Fraser <keir.xen@xxxxxxxxx>]

On 20/06/2013 11:33, "Tim Deegan" <tim@xxxxxxx> wrote:

> (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@xxxxxxx>
>>> 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@xxxxxxxxxxxx>
>>>     Signed-off-by: Tim Deegan <tim@xxxxxxx>
>>> 
>>> 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?

By preference, and even for 4.3 since it is still obviously correct, I would
change the handling of inject_trap.vector != -1 to be the same as previous
error checks, moving the actual work out of an else block. But maybe that
should be a later cleanup. Either way:
Acked-by: Keir Fraser <keir@xxxxxxx>

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

Agreed, this is really a no-brainer for 4.3. It's fixing an obvio bug.

 -- Keir

> 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);



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