On 14/10/16 18:05, Kyle Huey wrote:
> On Fri, Oct 14, 2016 at 7:46 AM, Andrew Cooper
> <andrew.coop...@citrix.com> wrote:
>> On 14/10/16 13:04, Jan Beulich wrote:
>>>>>> On 13.10.16 at 23:09, <m...@kylehuey.com> 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.


Xen-devel mailing list

Reply via email to