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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.10.16 at 23:09, <me@xxxxxxxxxxxx> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel