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

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 08/27/2014 11:25 PM, Tian, Kevin wrote:
>> From: Razvan Cojocaru [mailto:rcojocaru@bitdefender.com]
>> Sent: Wednesday, August 27, 2014 7:02 AM
>>
>> Vmx_disable_intercept_for_msr() will now refuse to disable interception of
>> MSRs needed for memory introspection. It is not possible to gate this on
>> mem_access being active for the domain, since by the time mem_access does
>> become active the interception for the interesting MSRs has already been
>> disabled (vmx_disable_intercept_for_msr() runs very early on).
>>
>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
>> index 4a4f4e1..44aca73 100644
>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>> @@ -39,6 +39,7 @@
>>  #include <xen/keyhandler.h>
>>  #include <asm/shadow.h>
>>  #include <asm/tboot.h>
>> +#include <asm/mem_event.h>
>>
>>  static bool_t __read_mostly opt_vpid_enabled = 1;
>>  boolean_param("vpid", opt_vpid_enabled);
>> @@ -71,6 +72,18 @@ u32 vmx_vmexit_control __read_mostly;
>>  u32 vmx_vmentry_control __read_mostly;
>>  u64 vmx_ept_vpid_cap __read_mostly;
>>
>> +const u32 vmx_msrs_exit_array[] = {
>> +    MSR_IA32_SYSENTER_EIP,
>> +    MSR_IA32_SYSENTER_ESP,
>> +    MSR_IA32_SYSENTER_CS,
>> +    MSR_IA32_MC0_CTL,
>> +    MSR_STAR,
>> +    MSR_LSTAR
>> +};
>> +
>> +const unsigned int vmx_msrs_exit_array_size =
>> +    ARRAY_SIZE(vmx_msrs_exit_array);
>> +
> 
> prefer a name including 'force' purpose or at least have a comment
> to describe the intention, so its meaning is explained as early as possible.

How would vmx_force_enabled_msrs_array (with a comment added) sound to
you and Jan?

>>      /*
>>       * See Intel PRM Vol. 3, 20.6.9 (MSR-Bitmap Address). Early manuals
>>       * have the write-low and read-high bitmap offsets the wrong way
>> round.
>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>> index dc18a72..40e6c2c 100644
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -1682,6 +1682,17 @@ void vmx_hypervisor_cpuid_leaf(uint32_t sub_idx,
>>          *eax |= XEN_HVM_CPUID_X2APIC_VIRT;
>>  }
>>
>> +static void vmx_enable_msr_exit_interception(struct domain *d)
>> +{
>> +    struct vcpu *v;
>> +    unsigned int i;
>> +
>> +    /* Enable interception for MSRs needed for memory introspection. */
>> +    for_each_vcpu ( d, v )
>> +        for ( i = 0; i < vmx_msrs_exit_array_size; i++ )
>> +            vmx_enable_intercept_for_msr(v, vmx_msrs_exit_array[i],
>> MSR_TYPE_W);
>> +}
>> +
> 
> so you need same conditional check as you added for vmx_disable_intercept_for_msr

The check part is in this code:

diff --git a/xen/arch/x86/mm/mem_event.c b/xen/arch/x86/mm/mem_event.c
index ba7e71e..fdd5ff6 100644
--- a/xen/arch/x86/mm/mem_event.c
+++ b/xen/arch/x86/mm/mem_event.c
@@ -587,6 +587,7 @@ int mem_event_domctl(struct domain *d,
xen_domctl_mem_event_op_t *mec,
         switch( mec->op )
         {
         case XEN_DOMCTL_MEM_EVENT_OP_ACCESS_ENABLE:
+        case XEN_DOMCTL_MEM_EVENT_OP_ACCESS_ENABLE_INTROSPECTION:
         {
             rc = -ENODEV;
             /* Only HAP is supported */
@@ -600,13 +601,23 @@ 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 &&
+                 rc == 0 && hvm_funcs.enable_msr_exit_interception )
+            {
+                d->arch.hvm_domain.introspection_enabled = 1;
+                hvm_funcs.enable_msr_exit_interception(d);
+            }
         }
         break;

         case XEN_DOMCTL_MEM_EVENT_OP_ACCESS_DISABLE:
         {
             if ( med->ring_page )
+            {
                 rc = mem_event_disable(d, med);
+                d->arch.hvm_domain.introspection_enabled = 0;
+            }
         }
         break;

So if we do a domctl with
XEN_DOMCTL_MEM_EVENT_OP_ACCESS_ENABLE_INTROSPECTION, then we must make
sure that those MSRs are being intercepted (so only then do we call
hvm_funcs.enable_msr_exit_interception(d)).

The previous check (the one you've mentioned) only makes sure that if
d->arch.hvm_domain.introspection_enabled is true, interception for the
interesting MSRs cannot be disabled.


Thanks,
Razvan Cojocaru