[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH RFC V5 3/5] xen: Force-enable relevant MSR events; optimize the number of sent MSR events



>>> On 06.08.14 at 17:58, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> @@ -695,11 +696,30 @@ 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) )
> +    {
> +        /* Filter out MSR-s needed for memory introspection */

I continue to be unconvinced that this code block's surrounding
conditional is as precise as possible: Your introspection code
surely isn't the only mem-event based mechanism. Yet you'd
impact guests in all other cases too.

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1682,6 +1682,22 @@ void vmx_hypervisor_cpuid_leaf(uint32_t sub_idx,
>          *eax |= XEN_HVM_CPUID_X2APIC_VIRT;
>  }
>  
> +static void vmx_enable_intro_msr_interception(struct domain *d)

The "intro" in the name is surely odd: For one, it implies that _only_
introspection might be interested in doing this. And then it may
(without reading the comments inside the function) well be an
abbreviation for something else, e.g. "introduction".

> +{
> +    struct vcpu *v;
> +
> +    /* Enable interception for MSRs needed for memory introspection. */
> +    for_each_vcpu ( d, v )
> +    {
> +        vmx_enable_intercept_for_msr(v, MSR_IA32_SYSENTER_EIP, MSR_TYPE_W);
> +        vmx_enable_intercept_for_msr(v, MSR_IA32_SYSENTER_ESP, MSR_TYPE_W);
> +        vmx_enable_intercept_for_msr(v, MSR_IA32_SYSENTER_CS, MSR_TYPE_W);
> +        vmx_enable_intercept_for_msr(v, MSR_IA32_MC0_CTL, MSR_TYPE_W);
> +        vmx_enable_intercept_for_msr(v, MSR_STAR, MSR_TYPE_W);
> +        vmx_enable_intercept_for_msr(v, MSR_LSTAR, MSR_TYPE_W);

I also wonder whether the redundant enumeration of all these
MSRs couldn't be abstracted to just a single place.

> --- a/xen/arch/x86/mm/mem_event.c
> +++ b/xen/arch/x86/mm/mem_event.c
> @@ -600,6 +600,9 @@ 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 ( rc == 0 && hvm_funcs.enable_intro_msr_interception )
> +                hvm_funcs.enable_intro_msr_interception(d);

Isn't the sequence of operations wrong here (leaving a window in
time where mem events are already enabled but the necessary MSRs
aren't being intercepted yet? Or was it that guests are being paused
while all this takes place?

Plus of course the same remark regarding the insufficient conditional
as above applies here.

Jan


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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.