[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


  • To: Jan Beulich <JBeulich@xxxxxxxx>
  • From: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
  • Date: Fri, 08 Aug 2014 17:47:43 +0300
  • Cc: kevin.tian@xxxxxxxxx, ian.campbell@xxxxxxxxxx, stefano.stabellini@xxxxxxxxxxxxx, andrew.cooper3@xxxxxxxxxx, eddie.dong@xxxxxxxxx, xen-devel@xxxxxxxxxxxxx, jun.nakajima@xxxxxxxxx, ian.jackson@xxxxxxxxxxxxx
  • Comment: DomainKeys? See http://domainkeys.sourceforge.net/
  • Delivery-date: Fri, 08 Aug 2014 14:47:58 +0000
  • Domainkey-signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=bitdefender.com; b=tUiX2ETkAXj7h5C6SsfIzRLHfKgFzBlLRgWoacx7rwpxJYubLSeXFJ+0uyyiRmW4VjEB38g7bV5LPfScdVoe/NGxs73DdwxoCLH1aAFY0JUJdnusbs+v8YaAiaeIMrwsjCkFdUU7UUG1bWlCS0eV3t6G0NuUbR7cTFd89hiXAeDPUcEF5YFLgzG+oxmCZh6/4f5TUjRCtwwdI8gihSH+OeqINr/+l+Zchuw5UTU2sq1PHPe28aaVB+PHjwXJ/irJ0hVpGJB6GeaNqTJRT0AXyPn4Df+IgDnvW5qjuinAKJu//hOF38HYbBKR53uAEzID+UcSbbmQEwNV9OFfY9Bv4A==; h=Received:Received:Received:Received:Message-ID:Date:From:User-Agent:MIME-Version:To:CC:Subject:References:In-Reply-To:Content-Type:Content-Transfer-Encoding:X-BitDefender-Scanner:X-BitDefender-Spam:X-BitDefender-SpamStamp:X-BitDefender-CF-Stamp;
  • List-id: Xen developer discussion <xen-devel.lists.xen.org>

On 08/08/2014 05:34 PM, Jan Beulich wrote:
>>>> 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.

I agree, however I can't think of a way to be more specific without
introducing a special new parameter / bit when enabling mem_access.
If you feel that that would not be a problem, I'll add one.

>> --- 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".

It's no problem to either drop "intro" or expand it into
"introspection". Would one be preferable to the other?

>> +{
>> +    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.

I'll add them to a const array and iterate through that.

>> --- 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?

The guest is paused, but that's a fair point. I'll look into it.


Thanks,
Razvan Cojocaru

_______________________________________________
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®.