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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.10.16 at 00:38, <me@xxxxxxxxxxxx> wrote:
> 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.
> 
> 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.

Why for CPL > 0 only? I don't think hardware CPUID faulting is CPL
sensitive.

> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1095,6 +1095,8 @@ int arch_set_info_guest(
>      for ( i = 0; i < 8; i++ )
>          (void)set_debugreg(v, i, c(debugreg[i]));
>  
> +    v->arch.cpuid_fault = 0;

What's the reason for clearing this here?

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2430,11 +2430,16 @@ static void vmx_cpuid_intercept(
>      HVMTRACE_5D (CPUID, input, *eax, *ebx, *ecx, *edx);
>  }
>  
> -static int vmx_do_cpuid(struct cpu_user_regs *regs)
> +static int vmx_do_cpuid(struct vcpu *v, struct cpu_user_regs *regs)

Not sure this is worthwhile - you could easily use current instead
of v.

>  {
>      unsigned int eax, ebx, ecx, edx;
>      unsigned int leaf, subleaf;
>  
> +    if ( v->arch.cpuid_fault && !ring_0(regs) ) {

Coding style.

> @@ -2701,9 +2706,13 @@ static int vmx_msr_read_intercept(unsigned int msr, 
> uint64_t *msr_content)
>          break;
>  
>      case MSR_INTEL_PLATFORM_INFO:
> -        if ( rdmsr_safe(MSR_INTEL_PLATFORM_INFO, *msr_content) )
> -            goto gp_fault;
> -        *msr_content = 0;
> +        if ( is_pvh_vcpu(current) && !cpu_has_cpuid_faulting )

Any specific reason to treat PVHv1 differently? Also don't you mean
|| instead of && ?

> +            *msr_content = 0;
> +        else
> +            *msr_content = 0x80000000ULL;

Please use MSR_PLATFORM_INFO_CPUID_FAULTING here.

> +        break;
> +    case MSR_INTEL_MISC_FEATURES_ENABLES:
> +        *msr_content = current->arch.cpuid_fault ? 1ULL : 0;

And MSR_MISC_FEATURES_CPUID_FAULTING here. And please
(here and elsewhere) maintain surrounding code style, i.e. blank
lines between non-fall-through case statements.

> @@ -2931,6 +2940,11 @@ static int vmx_msr_write_intercept(unsigned int msr, 
> uint64_t msr_content)
>               rdmsr_safe(MSR_INTEL_PLATFORM_INFO, msr_content) )
>              goto gp_fault;
>          break;
> +    case MSR_INTEL_MISC_FEATURES_ENABLES:
> +        if ( msr_content > 1 )

This wants to be "msr_content & ~MSR_MISC_FEATURES_CPUID_FAULTING".

> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -573,6 +573,9 @@ struct arch_vcpu
>      XEN_GUEST_HANDLE(vcpu_time_info_t) time_info_guest;
>  
>      struct arch_vm_event *vm_event;
> +
> +    /* Has the guest enabled CPUID faulting? */
> +    bool_t cpuid_fault;

Plain bool, true, and false please.

Jan


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