Re: [Xen-devel] [PATCH RFC V7 3/5] xen, libxc: Force-enable relevant MSR events

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.08.14 at 17:28, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> Changes since V6:
>  - Moved the array of interesting MSRs to common header.

Urgh (see below).

> @@ -695,11 +696,23 @@ static void vmx_set_host_env(struct vcpu *v)
>  void vmx_disable_intercept_for_msr(struct vcpu *v, u32 msr, int type)
>  {
>      unsigned long *msr_bitmap = v->arch.hvm_vmx.msr_bitmap;
> +    struct domain *d = v->domain;
>  
>      /* VMX MSR bitmap supported? */
>      if ( msr_bitmap == NULL )
>          return;
>  
> +    if ( mem_event_check_ring(&d->mem_event->access) &&
> +         d->arch.hvm_domain.introspection_enabled )

Reverse the two sides of the && to do the obviously cheap check
first (and maybe wrap that in unlikely())?

> @@ -600,13 +601,25 @@ int mem_event_domctl(struct domain *d, 
> xen_domctl_mem_event_op_t *mec,
>              rc = mem_event_enable(d, mec, med, _VPF_mem_access, 
>                                      HVM_PARAM_ACCESS_RING_PFN,
>                                      mem_access_notification);
> +
> +            if (  mec->op == XEN_DOMCTL_MEM_EVENT_OP_ACCESS_ENABLE )
> +                break;
> +
> +            if ( rc == 0 && hvm_funcs.enable_msr_exit_interception )

No reason not to combine the two if()s afaict.

> --- a/xen/include/asm-x86/hvm/domain.h
> +++ b/xen/include/asm-x86/hvm/domain.h
> @@ -145,6 +145,8 @@ struct hvm_domain {
>          struct vmx_domain vmx;
>          struct svm_domain svm;
>      };
> +
> +    bool_t introspection_enabled;

Please put this adjacent to the four other bool_t-s a few lines above,
thus filling (rather than creating) a gap.

> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> @@ -471,6 +471,15 @@ enum vmcs_field {
>      HOST_RIP                        = 0x00006c16,
>  };
>  
> +static const u32 msrs_exit_array[] = {
> +    MSR_IA32_SYSENTER_EIP,
> +    MSR_IA32_SYSENTER_ESP,
> +    MSR_IA32_SYSENTER_CS,
> +    MSR_IA32_MC0_CTL,
> +    MSR_STAR,
> +    MSR_LSTAR
> +};

So why do you need more than one instance of this in the hypervisor?
This should just be a declaration here, and a definition in one of the
source files. Furthermore its name should start with vmx_.

Jan


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