Re: [Xen-devel] [PATCH v2 2/2] x86/Intel: virtualize support for cpuid faulting
On Mon, Oct 17, 2016 at 9:39 AM, Andrew Cooper wrote: > On 17/10/16 17:28, Kyle Huey wrote: >> On Mon, Oct 17, 2016 at 5:34 AM, Andrew Cooper >> wrote: >>> On 14/10/16 20:36, Kyle Huey wrote: On Fri, Oct 14, 2016 at 10:18 AM, Andrew Cooper wrote: > 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. >>> FWIW, I have just done some library improvements and rebased the test. >>> It's pretty slick. Much better than what Linux has ;) I do think it's a bit confusing that xtf_has_fep is false on PV guests. >>> Now you point it out, I can see how it would be confusing. This is due >>> to the history of FEP. >>> >>> The sequence `ud2; .ascii 'xen'; cpuid` has been around ages (it >>> predates faulting and hardware with mask/override MSRs), and is used by >>> PV guests to specifically request Xen's CPUID information, rather than >>> getting the real hardware information. >>> >>> There is also an rdtscp variant for PV guests, used for virtual TSC modes. >>> >>> In Xen 4.5, I introduced the same prefix to HVM guests, but for >>> arbitrary instructions. This was for the express purpose of testing the >>> x86 instruction emulator. >>> >>> As a result, CPUID in PV guests is the odd case out. >>> It might also be nice to (at least optionally) have xtf_assert(cond, message) so instead of if ( cond ) xtf_failure(message); you can write xtf_assert(!cond, message); A bonus of doing this is that the framework could actually count how many checks were run. So for the HVM tests (which don't run the FEP bits) instead of getting "Test result: SKIP" you could say "Test result: 9 PASS 1 SKIP" or something similar. >>> Boot with "hvm_fep" on the command line and the tests should end up >>> reporting success. >> They do not, because the hvm_fep code calls vmx_cpuid_intercept (not >> vmx_do_cpuid) so it skips the faulting check. The reason I did this >> in vmx_do_cpuid originally is that hvm_efer_valid also calls >> vmx_cpuid_intercept and that should not fault. >> >> I could push the cpuid faulting code down into vmx_cpuid_intercept, >> give it a non-void return value so it can tell its callees not to >> advance the IP in this situation, and make hvm_efer_valid save, clear, >> and restore the cpuid_fault flag on the vcpu to call >> vmx_cpuid_intercept. Though it's not immediately obvious to me that >> hvm_efer_valid is always called with v == current. Do you think it's >> worth it for this testing code? > > This isn't just for testing code. It also means that cpuid faulting > support won't work with introspected domains, which can also end up > emulating cpuid instructions because of restricted execute permissions > on a page. > > The hvm_efer_valid() tangle can't be untangled at the moment; the use of > vmx_cpuid_intercept() is deliberate to provide accurate behaviour with > the handling on EFER_SCE. > > Your best bet here is to put a faulting check in hvmemul_cpuid() as well. That's not quite what we want either, because hvmemul_cpuid will also be called when clzero is emulated. - Kyle ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 2/2] x86/Intel: virtualize support for cpuid faulting
On 17/10/16 17:28, Kyle Huey wrote: > On Mon, Oct 17, 2016 at 5:34 AM, Andrew Cooper > wrote: >> On 14/10/16 20:36, Kyle Huey wrote: >>> On Fri, Oct 14, 2016 at 10:18 AM, Andrew Cooper >>> wrote: 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. >> FWIW, I have just done some library improvements and rebased the test. >> >>> It's pretty slick. Much better than what Linux has ;) >>> >>> I do think it's a bit confusing that xtf_has_fep is false on PV guests. >> Now you point it out, I can see how it would be confusing. This is due >> to the history of FEP. >> >> The sequence `ud2; .ascii 'xen'; cpuid` has been around ages (it >> predates faulting and hardware with mask/override MSRs), and is used by >> PV guests to specifically request Xen's CPUID information, rather than >> getting the real hardware information. >> >> There is also an rdtscp variant for PV guests, used for virtual TSC modes. >> >> In Xen 4.5, I introduced the same prefix to HVM guests, but for >> arbitrary instructions. This was for the express purpose of testing the >> x86 instruction emulator. >> >> As a result, CPUID in PV guests is the odd case out. >> >>> It might also be nice to (at least optionally) have xtf_assert(cond, >>> message) so instead of >>> >>> if ( cond ) >>> xtf_failure(message); >>> >>> you can write >>> >>> xtf_assert(!cond, message); >>> >>> A bonus of doing this is that the framework could actually count how >>> many checks were run. So for the HVM tests (which don't run the FEP >>> bits) instead of getting "Test result: SKIP" you could say "Test >>> result: 9 PASS 1 SKIP" or something similar. >> Boot with "hvm_fep" on the command line and the tests should end up >> reporting success. > They do not, because the hvm_fep code calls vmx_cpuid_intercept (not > vmx_do_cpuid) so it skips the faulting check. The reason I did this > in vmx_do_cpuid originally is that hvm_efer_valid also calls > vmx_cpuid_intercept and that should not fault. > > I could push the cpuid faulting code down into vmx_cpuid_intercept, > give it a non-void return value so it can tell its callees not to > advance the IP in this situation, and make hvm_efer_valid save, clear, > and restore the cpuid_fault flag on the vcpu to call > vmx_cpuid_intercept. Though it's not immediately obvious to me that > hvm_efer_valid is always called with v == current. Do you think it's > worth it for this testing code? This isn't just for testing code. It also means that cpuid faulting support won't work with introspected domains, which can also end up emulating cpuid instructions because of restricted execute permissions on a page. The hvm_efer_valid() tangle can't be untangled at the moment; the use of vmx_cpuid_intercept() is deliberate to provide accurate behaviour with the handling on EFER_SCE. Your best bet here is to put a faulting check in hvmemul_cpuid() as well. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 2/2] x86/Intel: virtualize support for cpuid faulting
On Mon, Oct 17, 2016 at 5:34 AM, Andrew Cooper wrote: > On 14/10/16 20:36, Kyle Huey wrote: >> On Fri, Oct 14, 2016 at 10:18 AM, Andrew Cooper >> wrote: >>> 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. > > FWIW, I have just done some library improvements and rebased the test. > >> It's pretty slick. Much better than what Linux has ;) >> >> I do think it's a bit confusing that xtf_has_fep is false on PV guests. > > Now you point it out, I can see how it would be confusing. This is due > to the history of FEP. > > The sequence `ud2; .ascii 'xen'; cpuid` has been around ages (it > predates faulting and hardware with mask/override MSRs), and is used by > PV guests to specifically request Xen's CPUID information, rather than > getting the real hardware information. > > There is also an rdtscp variant for PV guests, used for virtual TSC modes. > > In Xen 4.5, I introduced the same prefix to HVM guests, but for > arbitrary instructions. This was for the express purpose of testing the > x86 instruction emulator. > > As a result, CPUID in PV guests is the odd case out. > >> >> It might also be nice to (at least optionally) have xtf_assert(cond, >> message) so instead of >> >> if ( cond ) >> xtf_failure(message); >> >> you can write >> >> xtf_assert(!cond, message); >> >> A bonus of doing this is that the framework could actually count how >> many checks were run. So for the HVM tests (which don't run the FEP >> bits) instead of getting "Test result: SKIP" you could say "Test >> result: 9 PASS 1 SKIP" or something similar. > > Boot with "hvm_fep" on the command line and the tests should end up > reporting success. They do not, because the hvm_fep code calls vmx_cpuid_intercept (not vmx_do_cpuid) so it skips the faulting check. The reason I did this in vmx_do_cpuid originally is that hvm_efer_valid also calls vmx_cpuid_intercept and that should not fault. I could push the cpuid faulting code down into vmx_cpuid_intercept, give it a non-void return value so it can tell its callees not to advance the IP in this situation, and make hvm_efer_valid save, clear, and restore the cpuid_fault flag on the vcpu to call vmx_cpuid_intercept. Though it's not immediately obvious to me that hvm_efer_valid is always called with v == current. Do you think it's worth it for this testing code? - Kyle ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 2/2] x86/Intel: virtualize support for cpuid faulting
On 14/10/16 20:36, Kyle Huey wrote: > On Fri, Oct 14, 2016 at 10:18 AM, Andrew Cooper > wrote: >> 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. FWIW, I have just done some library improvements and rebased the test. > It's pretty slick. Much better than what Linux has ;) > > I do think it's a bit confusing that xtf_has_fep is false on PV guests. Now you point it out, I can see how it would be confusing. This is due to the history of FEP. The sequence `ud2; .ascii 'xen'; cpuid` has been around ages (it predates faulting and hardware with mask/override MSRs), and is used by PV guests to specifically request Xen's CPUID information, rather than getting the real hardware information. There is also an rdtscp variant for PV guests, used for virtual TSC modes. In Xen 4.5, I introduced the same prefix to HVM guests, but for arbitrary instructions. This was for the express purpose of testing the x86 instruction emulator. As a result, CPUID in PV guests is the odd case out. > > It might also be nice to (at least optionally) have xtf_assert(cond, > message) so instead of > > if ( cond ) > xtf_failure(message); > > you can write > > xtf_assert(!cond, message); > > A bonus of doing this is that the framework could actually count how > many checks were run. So for the HVM tests (which don't run the FEP > bits) instead of getting "Test result: SKIP" you could say "Test > result: 9 PASS 1 SKIP" or something similar. Boot with "hvm_fep" on the command line and the tests should end up reporting success. It is a deliberate design choice to have a single result from a single run of the test kernel, and is a design inherited from XenServer's internal testing system (which runs ~10k machine hours of testing every single day). During development, I can see the attraction of having a unittest style report, but it becomes counterproductive at larger scale. Subdividing the specific failures is not helpful for the high level view, so is omitted to simply status reporting. Once a test is developed and stable, it should always be reporting SUCCESS; anything else indicates an issue to be fixed. When investigating failures, the actual text of the failure message is the important bit of information. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 2/2] x86/Intel: virtualize support for cpuid faulting
On 14/10/16 20:28, Kyle Huey wrote:
>> :) I am now curious as to which bit I missed.
> I made these changes.
>
> - Kyle
>
> ---
> tests/cpuid-faulting/main.c | 9 ++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/tests/cpuid-faulting/main.c b/tests/cpuid-faulting/main.c
> index 3e782a2..221567d 100644
> --- a/tests/cpuid-faulting/main.c
> +++ b/tests/cpuid-faulting/main.c
> @@ -37,36 +37,39 @@ bool ex_record_fault_ebx(struct cpu_regs *regs,
> }
>
> unsigned int stub_rdmsr(uint32_t idx, uint64_t *val)
> {
> unsigned int fault = 0;
> uint32_t lo, hi;
>
> *val = 0;
> -asm volatile("1: rdmsr; 2:"
> +asm volatile("1: rdmsr;"
> + "xor %%ebx, %%ebx; 2:"
> _ASM_EXTABLE_HANDLER(1b, 2b, ex_record_fault_ebx)
> : "=a" (lo), "=d" (hi), "=&b" (fault)
Ah - this should be +b rather than =&b, which avoids modifying the assembly.
> : "c" (idx));
>
> if ( !fault )
> *val = (((uint64_t)hi) << 32) | lo;
>
> return fault;
> }
>
> unsigned int stub_wrmsr(uint32_t idx, uint64_t val)
> {
> unsigned int fault = 0;
>
> -asm volatile("1: rdmsr; 2:"
> +asm volatile("1: wrmsr;"
Oops.
~Andrew
> "xor %%ebx, %%ebx; 2:"
> _ASM_EXTABLE_HANDLER(1b, 2b, ex_record_fault_ebx)
> : "=&b" (fault)
> : "c" (idx), "a" ((uint32_t)val),
> - "d" ((uint32_t)(val >> 32)));
> + "d" ((uint32_t)(val >> 32))
> + : "memory");
>
> return fault;
> }
>
> unsigned long stub_cpuid(void)
> {
> unsigned int fault = 0;
>
>
> base-commit: 0342fd279b05d0c2e31c8418fcffcdc9e48eb42f
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 2/2] x86/Intel: virtualize support for cpuid faulting
On Fri, Oct 14, 2016 at 10:18 AM, Andrew Cooper wrote: > 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. It's pretty slick. Much better than what Linux has ;) I do think it's a bit confusing that xtf_has_fep is false on PV guests. It might also be nice to (at least optionally) have xtf_assert(cond, message) so instead of if ( cond ) xtf_failure(message); you can write xtf_assert(!cond, message); A bonus of doing this is that the framework could actually count how many checks were run. So for the HVM tests (which don't run the FEP bits) instead of getting "Test result: SKIP" you could say "Test result: 9 PASS 1 SKIP" or something similar. - Kyle ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 2/2] x86/Intel: virtualize support for cpuid faulting
> :) I am now curious as to which bit I missed.
I made these changes.
- Kyle
---
tests/cpuid-faulting/main.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/tests/cpuid-faulting/main.c b/tests/cpuid-faulting/main.c
index 3e782a2..221567d 100644
--- a/tests/cpuid-faulting/main.c
+++ b/tests/cpuid-faulting/main.c
@@ -37,36 +37,39 @@ bool ex_record_fault_ebx(struct cpu_regs *regs,
}
unsigned int stub_rdmsr(uint32_t idx, uint64_t *val)
{
unsigned int fault = 0;
uint32_t lo, hi;
*val = 0;
-asm volatile("1: rdmsr; 2:"
+asm volatile("1: rdmsr;"
+ "xor %%ebx, %%ebx; 2:"
_ASM_EXTABLE_HANDLER(1b, 2b, ex_record_fault_ebx)
: "=a" (lo), "=d" (hi), "=&b" (fault)
: "c" (idx));
if ( !fault )
*val = (((uint64_t)hi) << 32) | lo;
return fault;
}
unsigned int stub_wrmsr(uint32_t idx, uint64_t val)
{
unsigned int fault = 0;
-asm volatile("1: rdmsr; 2:"
+asm volatile("1: wrmsr;"
+ "xor %%ebx, %%ebx; 2:"
_ASM_EXTABLE_HANDLER(1b, 2b, ex_record_fault_ebx)
: "=&b" (fault)
: "c" (idx), "a" ((uint32_t)val),
- "d" ((uint32_t)(val >> 32)));
+ "d" ((uint32_t)(val >> 32))
+ : "memory");
return fault;
}
unsigned long stub_cpuid(void)
{
unsigned int fault = 0;
base-commit: 0342fd279b05d0c2e31c8418fcffcdc9e48eb42f
--
2.10.1
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 2/2] x86/Intel: virtualize support for cpuid faulting
On 14/10/16 18:05, Kyle Huey wrote:
> On Fri, Oct 14, 2016 at 7:46 AM, Andrew Cooper
> wrote:
>> On 14/10/16 13:04, Jan Beulich wrote:
>> On 13.10.16 at 23:09, 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@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 2/2] x86/Intel: virtualize support for cpuid faulting
On Fri, Oct 14, 2016 at 7:46 AM, Andrew Cooper
wrote:
> On 14/10/16 13:04, Jan Beulich wrote:
> On 13.10.16 at 23:09, 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@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 2/2] x86/Intel: virtualize support for cpuid faulting
On 14/10/16 13:04, Jan Beulich wrote:
On 13.10.16 at 23:09, 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.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 2/2] x86/Intel: virtualize support for cpuid faulting
>>> On 13.10.16 at 23:09, 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?
> @@ -2474,16 +2478,27 @@ static int priv_op_read_msr(unsigned int reg,
> uint64_t *val,
> *val = 0;
> return X86EMUL_OKAY;
>
> case MSR_INTEL_PLATFORM_INFO:
> if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL ||
> rdmsr_safe(MSR_INTEL_PLATFORM_INFO, *val) )
> break;
> *val = 0;
> +if ( this_cpu(cpuid_faulting_enabled) )
> +*val |= MSR_PLATFORM_INFO_CPUID_FAULTING;
> +return X86EMUL_OKAY;
> +
> +case MSR_INTEL_MISC_FEATURES_ENABLES:
> +if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL ||
> + rdmsr_safe(MSR_INTEL_MISC_FEATURES_ENABLES, *val))
Missing blank before the final closing parenthesis.
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -568,16 +568,19 @@ struct arch_vcpu
> struct paging_vcpu paging;
>
> uint32_t gdbsx_vcpu_event;
>
> /* A secondary copy of the vcpu time info. */
> XEN_GUEST_HANDLE(vcpu_time_info_t) time_info_guest;
>
> struct arch_vm_event *vm_event;
> +
> +/* Has the guest enabled CPUID faulting? */
> +bool cpuid_fault;
> };
This should be moved up into one of the available holes: Preferably
next to the other boolean, but aside gdbsx_vcpu_event would also
be better than putting it here.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2 2/2] x86/Intel: virtualize support for cpuid faulting
On HVM guests, the cpuid triggers a vm exit, so we can check the emulated
faulting state in vmx_do_cpuid and inject a GP(0) if CPL > 0. Notably no
hardware support for faulting on cpuid is necessary to emulate support with an
HVM guest.
On PV guests, hardware support is required so that userspace cpuid will trap
to xen. Xen already enables cpuid faulting on supported CPUs for pv guests (that
aren't the control domain, see the comment in intel_ctxt_switch_levelling).
Every PV guest cpuid will trap via a GP(0) to emulate_privileged_op (via
do_general_protection). Once there we simply decline to emulate cpuid if the
CPL > 0 and faulting is enabled, leaving the GP(0) for the guest kernel to
handle.
Signed-off-by: Kyle Huey
---
xen/arch/x86/hvm/vmx/vmx.c | 24 ++--
xen/arch/x86/traps.c | 30 ++
xen/include/asm-x86/domain.h | 3 +++
3 files changed, 55 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index b9102ce..55201c1 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2427,16 +2427,25 @@ static void vmx_cpuid_intercept(
HVMTRACE_5D (CPUID, input, *eax, *ebx, *ecx, *edx);
}
static int vmx_do_cpuid(struct cpu_user_regs *regs)
{
unsigned int eax, ebx, ecx, edx;
unsigned int leaf, subleaf;
+struct segment_register sreg;
+struct vcpu *v = current;
+
+hvm_get_segment_register(v, x86_seg_ss, &sreg);
+if ( v->arch.cpuid_fault && sreg.attr.fields.dpl > 0 )
+{
+hvm_inject_hw_exception(TRAP_gp_fault, 0);
+return 0;
+}
eax = regs->eax;
ebx = regs->ebx;
ecx = regs->ecx;
edx = regs->edx;
leaf = regs->eax;
subleaf = regs->ecx;
@@ -2694,19 +2703,23 @@ static int vmx_msr_read_intercept(unsigned int msr,
uint64_t *msr_content)
case MSR_CORE_PERF_FIXED_CTR_CTRL...MSR_CORE_PERF_GLOBAL_OVF_CTRL:
case MSR_IA32_PEBS_ENABLE:
case MSR_IA32_DS_AREA:
if ( vpmu_do_rdmsr(msr, msr_content) )
goto gp_fault;
break;
case MSR_INTEL_PLATFORM_INFO:
-if ( rdmsr_safe(MSR_INTEL_PLATFORM_INFO, *msr_content) )
-goto gp_fault;
+*msr_content = MSR_PLATFORM_INFO_CPUID_FAULTING;
+break;
+
+case MSR_INTEL_MISC_FEATURES_ENABLES:
*msr_content = 0;
+if ( current->arch.cpuid_fault )
+*msr_content |= MSR_MISC_FEATURES_CPUID_FAULTING;
break;
default:
if ( passive_domain_do_rdmsr(msr, msr_content) )
goto done;
switch ( long_mode_do_msr_read(msr, msr_content) )
{
case HNDL_unhandled:
@@ -2925,16 +2938,23 @@ static int vmx_msr_write_intercept(unsigned int msr,
uint64_t msr_content)
break;
case MSR_INTEL_PLATFORM_INFO:
if ( msr_content ||
rdmsr_safe(MSR_INTEL_PLATFORM_INFO, msr_content) )
goto gp_fault;
break;
+case MSR_INTEL_MISC_FEATURES_ENABLES:
+if ( msr_content & ~MSR_MISC_FEATURES_CPUID_FAULTING )
+goto gp_fault;
+v->arch.cpuid_fault =
+!!(msr_content & MSR_MISC_FEATURES_CPUID_FAULTING);
+break;
+
default:
if ( passive_domain_do_wrmsr(msr, msr_content) )
return X86EMUL_OKAY;
if ( wrmsr_viridian_regs(msr, msr_content) )
break;
switch ( long_mode_do_msr_write(msr, msr_content) )
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 293ff8d..6d1c1ef 100644
--- 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;
+
eip += sizeof(instr);
pv_cpuid(regs);
instruction_done(regs, eip, 0);
trace_trap_one_addr(TRC_PV_FORCED_INVALID_OP, regs->eip);
@@ -2474,16 +2478,27 @@ static int priv_op_read_msr(unsigned int reg, uint64_t
*val,
*val = 0;
return X86EMUL_OKAY;
case MSR_INTEL_PLATFORM_INFO:
if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL ||
rdmsr_safe(MSR_INTEL_PLATFORM_INFO, *val) )
break;
*val = 0;
+if ( this_cpu(cpuid_faulting_enabled) )
+*val |= MSR_PLATFORM_INFO_CPUID_FAULTING;
+return X86EMUL_OKAY;
+
+case MSR_INTEL_MISC_FEATURES_ENABLES:
+if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL ||
+ rdmsr_safe(MSR_INTEL_MISC_FEATURES_ENABLES, *val))
+break;
+*val = 0;
+if ( curr->arch.
