Re: [Xen-devel] [PATCH v2 2/2] x86/Intel: virtualize support for cpuid faulting

[Kyle Huey <me@xxxxxxxxxxxx>]

On Fri, Oct 14, 2016 at 7:46 AM, Andrew Cooper
<andrew.cooper3@xxxxxxxxxx> wrote:
> On 14/10/16 13:04, Jan Beulich wrote:
>>>>> On 13.10.16 at 23:09, <me@xxxxxxxxxxxx> wrote:
>>> --- a/xen/arch/x86/traps.c
>>> +++ b/xen/arch/x86/traps.c
>>> @@ -1315,16 +1315,20 @@ static int emulate_forced_invalid_op(struct 
>>> cpu_user_regs *regs)
>>>      /* We only emulate CPUID. */
>>>      if ( ( rc = copy_from_user(instr, (char *)eip, sizeof(instr))) != 0 )
>>>      {
>>>          propagate_page_fault(eip + sizeof(instr) - rc, 0);
>>>          return EXCRET_fault_fixed;
>>>      }
>>>      if ( memcmp(instr, "\xf\xa2", sizeof(instr)) )
>>>          return 0;
>>> +    /* Let the guest have this one */
>>> +    if ( current->arch.cpuid_fault && !guest_kernel_mode(current, regs) )
>>> +        return 0;
>>> +
>> I think another blank line ahead of the addition would be nice. I also
>> think the comment should include the conditional aspect of what is
>> says (same on the second instance below).
>>
>> And then there is the question of state here: There's no rIP update
>> ahead of here, yet I'm uncertain whether we can expect the guest
>> kernel to handle both bare and Xen-prefixed CPUID instructions.
>> Andrew, what do you think?
>
> The #GP fault handed back to the guest kernel should point at the cpuid
> instruction, not the ud2 of the FEP.
>
> In reality, the only real use of this path from userspace is the
> `xen-detect` utility.  Regular userspace shouldn't know or care.
> Therefore, I am not concerned about the fact that it causes an implicit
> change of information source.
>
>
> I have taken the liberty of writing a CPUID Faulting XTF test, which
> should cover all the intended guest-side behaviour (although I did code
> it without a hypervisor side implementation, so I don't guarantee it is
> bug-free).
>
> The test can be found
> http://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen-test-framework.git;a=shortlog;h=refs/heads/cpuid-faulting
> and general docs on using XTF can be found
> http://xenbits.xen.org/docs/xtf/index.html  After cloning and building,
> use "./xtf-runner cpuid-faulting" to run the test in all types of VM.
>
> In particular, it will catch this specific issue when the exception
> frame from an emulated cpuid doesn't point at the cpuid instruction.
>
> Would you mind trying out this test?  Ideally, we would look to putting
> it (or a bugfixed version of it) into our CI system at the same time as
> the hypervisor support gets accepted.

Thanks for the test.  It almost works out of the box (will post a diff
later).  It also found another bug: my patches currently advance the
ip when cpuid faults in an HVM guest.  Will fix that too.

- Kyle

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