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

[Kyle Huey <me@xxxxxxxxxxxx>]

On Tue, Oct 18, 2016 at 1:29 AM, Tian, Kevin <kevin.tian@xxxxxxxxx> wrote:
>> From: Kyle Huey [mailto:me@xxxxxxxxxxxx]
>> Sent: Tuesday, October 18, 2016 2:51 AM
>>
>> On HVM guests, the cpuid triggers a vm exit, so we can check the emulated
>> faulting state in vmx_do_cpuid and hvmemul_cpuid. A new function,
>> hvm_check_cpuid_fault will check if cpuid faulting is enabled and the CPL > 
>> 0.
>> When it returns true, the cpuid handling functions will inject a GP(0). 
>> Notably
>> no hardware support for faulting on cpuid is necessary to emulate support 
>> with
>> an HVM guest.
>
> last sentence is confusing. Isn't "the cpuid triggers a vm exit" sort of
> hardware support?

Yeah, that's fair.  I'll reword this as "Notably explicit hardware
support for faulting on cpuid is not necessary to emulate support for
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@xxxxxxxxxxxx>
>> ---
>>  xen/arch/x86/hvm/emulate.c    | 19 +++++++++++++++++++
>>  xen/arch/x86/hvm/hvm.c        | 14 ++++++++++++++
>>  xen/arch/x86/hvm/vmx/vmx.c    | 20 ++++++++++++++++++--
>>  xen/arch/x86/traps.c          | 34
>> ++++++++++++++++++++++++++++++++++
>>  xen/include/asm-x86/domain.h  |  3 +++
>>  xen/include/asm-x86/hvm/hvm.h |  1 +
>>  6 files changed, 89 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
>> index 6ed7486..a713ff3 100644
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -1544,16 +1544,35 @@ static int hvmemul_wbinvd(
>>
>>  static int hvmemul_cpuid(
>>      unsigned int *eax,
>>      unsigned int *ebx,
>>      unsigned int *ecx,
>>      unsigned int *edx,
>>      struct x86_emulate_ctxt *ctxt)
>>  {
>> +    /*
>> +     * x86_emulate uses this function to query CPU features for its own 
>> internal
>> +     * use. Make sure we're actually emulating CPUID before emulating CPUID
>> +     * faulting.
>> +     */
>> +    if ( ctxt->opcode == X86EMUL_OPC(0x0f, 0xa2) &&
>> +         hvm_check_cpuid_fault(current) ) {
>> +        struct hvm_emulate_ctxt *hvmemul_ctxt =
>> +            container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
>> +
>> +        hvmemul_ctxt->exn_pending = 1;
>> +        hvmemul_ctxt->trap.vector = TRAP_gp_fault;
>> +        hvmemul_ctxt->trap.type = X86_EVENTTYPE_HW_EXCEPTION;
>> +        hvmemul_ctxt->trap.error_code = 0;
>> +        hvmemul_ctxt->trap.insn_len = 0;
>> +
>> +        return X86EMUL_EXCEPTION;
>
> I'm unclear about this change. So once guest enables CPUID faulting,
> emulation of other instructions which require internal cpuid query in
> x86_emulate will lead to a GP(0) injected to guest... Is this behavior
> change expected? Sorry if I overlooked something here.

This is the situation the 'ctxt->opcode == X86EMUL_OPC(0x0f, 0xa2)'
handles.  If the emulator is querying feature support the opcode will
be something else.

>> +    }
>> +
>>      hvm_funcs.cpuid_intercept(eax, ebx, ecx, edx);
>>      return X86EMUL_OKAY;
>>  }
>>
>>  static int hvmemul_inject_hw_exception(
>>      uint8_t vector,
>>      int32_t error_code,
>>      struct x86_emulate_ctxt *ctxt)
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index 3c90ecd..cf81e64 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -3675,16 +3675,30 @@ void hvm_cpuid(unsigned int input, unsigned int *eax,
>> unsigned int *ebx,
>>          hvm_cpuid(0x80000001, NULL, NULL, NULL, &_edx);
>>          *eax |= (_edx & cpufeat_mask(X86_FEATURE_LM) ? vaddr_bits : 32) << 
>> 8;
>>
>>          *ebx &= hvm_featureset[FEATURESET_e8b];
>>          break;
>>      }
>>  }
>>
>> +bool hvm_check_cpuid_fault(struct vcpu *v)
>
> to align with SDM, better to use full "cpuid_faulting". same for other
> places.

Ok.

>> +{
>> +    struct segment_register sreg;
>> +
>> +    if ( !v->arch.cpuid_fault )
>> +        return false;
>> +
>> +    hvm_get_segment_register(v, x86_seg_ss, &sreg);
>> +    if ( sreg.attr.fields.dpl == 0 )
>> +        return false;
>> +
>> +    return true;
>> +}
>> +
>>  static uint64_t _hvm_rdtsc_intercept(void)
>>  {
>>      struct vcpu *curr = current;
>>  #if !defined(NDEBUG) || defined(CONFIG_PERF_COUNTERS)
>>      struct domain *currd = curr->domain;
>>
>>      if ( currd->arch.vtsc )
>>          switch ( hvm_guest_x86_mode(curr) )
>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>> index b9102ce..228c1b9 100644
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -2428,16 +2428,21 @@ 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;
>>
>> +    if ( hvm_check_cpuid_fault(current) ) {
>> +        hvm_inject_hw_exception(TRAP_gp_fault, 0);
>> +        return 1;  /* Don't advance the guest IP! */
>> +    }
>> +
>>      eax = regs->eax;
>>      ebx = regs->ebx;
>>      ecx = regs->ecx;
>>      edx = regs->edx;
>>
>>      leaf = regs->eax;
>>      subleaf = regs->ecx;
>>
>> @@ -2694,19 +2699,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;
>> +
>
> So you plan to always export virtual cpuid faulting regardless of whether
> physical capability exists... not a technical problem since CPUID-induced
> VM-exit is a superset, but want to confirm the intention here. :-)

Yes.

- Kyle

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