[PATCH v2] x86/Intel: virtualize support for cpuid faulting

[PATCH v2] x86/Intel: virtualize support for cpuid faulting

[Kyle Huey]

rr (http://rr-project.org/), a Linux userspace record-and-replay reverse-
execution debugger, would like to trap and emulate the CPUID instruction.
This would allow us to a) mask away certain hardware features that rr does
not support (e.g. RDRAND) and b) enable trace portability across machines
by providing constant results. Patches for support in the Linux kernel are in
flight, and we'd like to be able to use this feature on virtualized Linux
instances as well.

Changes	since the previous version:
- Rebased.
- Exposed cpuid_faulting_enabled outside of cpu/intel.c, as suggested by
  Andrew Cooper. This is now patch 1, and the original changes are in
  patch 2.
- Various style nits from Andrew Cooper and Jan Beulich.
- Additional style changes not suggested (primarily replacing ternaries with
  if (condition) value |= BIT for futureproofing).
- Check guest_kernel_mode instead of ring_0 in emulate_privileged_op.
- Check cpuid_fault and guest_kernel_mode in emulate_forced_invalid_op.


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

[PATCH v2 1/2] x86/Intel: Expose cpuid_faulting_enabled so it can be used elsewhere

[Kyle Huey]

---
 xen/arch/x86/cpu/intel.c    | 3 ++-
 xen/include/asm-x86/cpuid.h | 3 +++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/cpu/intel.c b/xen/arch/x86/cpu/intel.c
index 7b60aaa..95c8e14 100644
--- a/xen/arch/x86/cpu/intel.c
+++ b/xen/arch/x86/cpu/intel.c
@@ -27,19 +27,20 @@ static bool_t __init probe_intel_cpuid_faulting(void)
 		return 0;
 
 	expected_levelling_cap |= LCAP_faulting;
 	levelling_caps |=  LCAP_faulting;
 	__set_bit(X86_FEATURE_CPUID_FAULTING, boot_cpu_data.x86_capability);
 	return 1;
 }
 
+DEFINE_PER_CPU(bool_t, cpuid_faulting_enabled);
+
 static void set_cpuid_faulting(bool_t enable)
 {
-	static DEFINE_PER_CPU(bool_t, cpuid_faulting_enabled);
 	bool_t *this_enabled = &this_cpu(cpuid_faulting_enabled);
 	uint32_t hi, lo;
 
 	ASSERT(cpu_has_cpuid_faulting);
 
 	if (*this_enabled == enable)
 		return;
 
diff --git a/xen/include/asm-x86/cpuid.h b/xen/include/asm-x86/cpuid.h
index 8e3f639..af8eb9d 100644
--- a/xen/include/asm-x86/cpuid.h
+++ b/xen/include/asm-x86/cpuid.h
@@ -59,16 +59,19 @@ struct cpuidmasks
 };
 
 /* Per CPU shadows of masking MSR values, for lazy context switching. */
 DECLARE_PER_CPU(struct cpuidmasks, cpuidmasks);
 
 /* Default masking MSR values, calculated at boot. */
 extern struct cpuidmasks cpuidmask_defaults;
 
+/* Whether or not cpuid faulting is available for the current domain. */
+DECLARE_PER_CPU(bool_t, cpuid_faulting_enabled);
+
 #endif /* __ASSEMBLY__ */
 #endif /* !__X86_CPUID_H__ */
 
 /*
  * Local variables:
  * mode: C
  * c-file-style: "BSD"
  * c-basic-offset: 4

base-commit: 71b8b46111219a2f83f4f9ae06ac5409744ea86e
-- 
2.10.1


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

[PATCH v2 2/2] x86/Intel: virtualize support for cpuid faulting

[Kyle Huey]

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 <khuey@kylehuey.com>
---
 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.cpuid_fault )
+            *val |= MSR_MISC_FEATURES_CPUID_FAULTING;
         return X86EMUL_OKAY;
 
     case MSR_P6_PERFCTR(0)...MSR_P6_PERFCTR(7):
     case MSR_P6_EVNTSEL(0)...MSR_P6_EVNTSEL(3):
     case MSR_CORE_PERF_FIXED_CTR0...MSR_CORE_PERF_FIXED_CTR2:
     case MSR_CORE_PERF_FIXED_CTR_CTRL...MSR_CORE_PERF_GLOBAL_OVF_CTRL:
         if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
         {
@@ -2677,16 +2692,27 @@ static int priv_op_write_msr(unsigned int reg, uint64_t val,
         return X86EMUL_OKAY;
 
     case MSR_INTEL_PLATFORM_INFO:
         if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL ||
              val || rdmsr_safe(MSR_INTEL_PLATFORM_INFO, val) )
             break;
         return X86EMUL_OKAY;
 
+    case MSR_INTEL_MISC_FEATURES_ENABLES:
+        if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL ||
+             (val & ~MSR_MISC_FEATURES_CPUID_FAULTING) ||
+             rdmsr_safe(MSR_INTEL_MISC_FEATURES_ENABLES, temp) )
+            break;
+        if ( (val & MSR_MISC_FEATURES_CPUID_FAULTING) &&
+             !this_cpu(cpuid_faulting_enabled) )
+            break;
+        curr->arch.cpuid_fault = !!(val & MSR_MISC_FEATURES_CPUID_FAULTING);
+        return X86EMUL_OKAY;
+
     case MSR_P6_PERFCTR(0)...MSR_P6_PERFCTR(7):
     case MSR_P6_EVNTSEL(0)...MSR_P6_EVNTSEL(3):
     case MSR_CORE_PERF_FIXED_CTR0...MSR_CORE_PERF_FIXED_CTR2:
     case MSR_CORE_PERF_FIXED_CTR_CTRL...MSR_CORE_PERF_GLOBAL_OVF_CTRL:
         if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
         {
             vpmu_msr = true;
     case MSR_AMD_FAM15H_EVNTSEL0...MSR_AMD_FAM15H_PERFCTR5:
@@ -3186,16 +3212,20 @@ static int emulate_privileged_op(struct cpu_user_regs *regs)
         if ( priv_op_read_msr(regs->_ecx, &val, NULL) != X86EMUL_OKAY )
             goto fail;
  rdmsr_writeback:
         regs->eax = (uint32_t)val;
         regs->edx = (uint32_t)(val >> 32);
         break;
 
     case 0xa2: /* CPUID */
+        /* Let the guest have this one */
+        if ( v->arch.cpuid_fault && !guest_kernel_mode(v, regs) )
+            goto fail;
+
         pv_cpuid(regs);
         break;
 
     default:
         goto fail;
     }
 
 #undef wr_ad
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 5807a1f..8ebc03b 100644
--- 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;
 };
 
 smap_check_policy_t smap_policy_change(struct vcpu *v,
                                        smap_check_policy_t new_policy);
 
 /* Shorthands to improve code legibility. */
 #define hvm_vmx         hvm_vcpu.u.vmx
 #define hvm_svm         hvm_vcpu.u.svm
-- 
2.10.1


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

Re: [PATCH v2 1/2] x86/Intel: Expose cpuid_faulting_enabled so it can be used elsewhere

[Jan Beulich]

>>> On 13.10.16 at 23:09, <me@kylehuey.com> wrote:
> ---

This is lacking an S-o-b. And it would also be nice if you switched the
bool_t instances to bool as you're touching them anyway.

Jan

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

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

[Jan Beulich]

>>> On 13.10.16 at 23:09, <me@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?

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

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

[Andrew Cooper]

On 14/10/16 13:04, Jan Beulich wrote:
>>>> On 13.10.16 at 23:09, <me@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.

~Andrew

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

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

[Kyle Huey]

On Fri, Oct 14, 2016 at 7:46 AM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 14/10/16 13:04, Jan Beulich wrote:
>>>>> On 13.10.16 at 23:09, <me@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).  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: [PATCH v2 2/2] x86/Intel: virtualize support for cpuid faulting

[Andrew Cooper]

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

~Andrew

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

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

[Kyle Huey]

> :) 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: [PATCH v2 2/2] x86/Intel: virtualize support for cpuid faulting

[Kyle Huey]

On Fri, Oct 14, 2016 at 10:18 AM, Andrew Cooper
<andrew.cooper3@citrix.com> 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: [PATCH v2 2/2] x86/Intel: virtualize support for cpuid faulting

[Andrew Cooper]

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: [PATCH v2 2/2] x86/Intel: virtualize support for cpuid faulting

[Andrew Cooper]

On 14/10/16 20:36, Kyle Huey wrote:
> On Fri, Oct 14, 2016 at 10:18 AM, Andrew Cooper
> <andrew.cooper3@citrix.com> 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: [PATCH v2 2/2] x86/Intel: virtualize support for cpuid faulting

[Kyle Huey]

On Mon, Oct 17, 2016 at 5:34 AM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 14/10/16 20:36, Kyle Huey wrote:
>> On Fri, Oct 14, 2016 at 10:18 AM, Andrew Cooper
>> <andrew.cooper3@citrix.com> 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: [PATCH v2 2/2] x86/Intel: virtualize support for cpuid faulting

[Andrew Cooper]

On 17/10/16 17:28, Kyle Huey wrote:
> On Mon, Oct 17, 2016 at 5:34 AM, Andrew Cooper
> <andrew.cooper3@citrix.com> wrote:
>> On 14/10/16 20:36, Kyle Huey wrote:
>>> On Fri, Oct 14, 2016 at 10:18 AM, Andrew Cooper
>>> <andrew.cooper3@citrix.com> 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: [PATCH v2 2/2] x86/Intel: virtualize support for cpuid faulting

[Kyle Huey]

On Mon, Oct 17, 2016 at 9:39 AM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 17/10/16 17:28, Kyle Huey wrote:
>> On Mon, Oct 17, 2016 at 5:34 AM, Andrew Cooper
>> <andrew.cooper3@citrix.com> wrote:
>>> On 14/10/16 20:36, Kyle Huey wrote:
>>>> On Fri, Oct 14, 2016 at 10:18 AM, Andrew Cooper
>>>> <andrew.cooper3@citrix.com> 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