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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/10/16 18:05, Kyle Huey wrote:
> 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).

:) I am now curious as to which bit I missed.

> It also found another bug: my patches currently advance the
> ip when cpuid faults in an HVM guest.  Will fix that too.

Ah of course.  Looks like you need to return 1 avoid the eip update, but
I have to admit that this is quite a poor interface.

In some copious free time, I will see about updating it to use the
X86EMUL_* interface.


On a slightly separate note, as you have just been a successful
guinea-pig for XTF, how did you find it?  It is a very new (still
somewhat in development) system but the project is looking to try and
improve regression testing in this way, especially for new features.  I
welcome any feedback.

~Andrew

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