Re: [Xen-devel] [PATCH V4] vm_event: Allow subscribing to write events for specific MSR-s

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 04/22/16 21:07, Andrew Cooper wrote:
> On 17/04/16 20:15, Razvan Cojocaru wrote:
>> diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
>> index 56c5514..9c17f37 100644
>> --- a/xen/arch/x86/hvm/event.c
>> +++ b/xen/arch/x86/hvm/event.c
>> @@ -57,9 +57,8 @@ bool_t hvm_event_cr(unsigned int index, unsigned long 
>> value, unsigned long old)
>>  void hvm_event_msr(unsigned int msr, uint64_t value)
>>  {
>>      struct vcpu *curr = current;
>> -    struct arch_domain *ad = &curr->domain->arch;
>>  
>> -    if ( ad->monitor.mov_to_msr_enabled )
>> +    if ( monitor_is_msr_enabled(curr->domain, msr) )
> 
> "enabled" can take several meanings with MSRs.  This function name would
> be clearer as is_msr_monitored(), or perhaps monitored_msr() if you want
> to keep the monitor prefix.

I'll pick the latter if nobody minds.

>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
>> index 8284281..24ad906 100644
>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>> @@ -37,6 +37,7 @@
>>  #include <asm/hvm/vmx/vvmx.h>
>>  #include <asm/hvm/vmx/vmcs.h>
>>  #include <asm/flushtlb.h>
>> +#include <asm/monitor.h>
>>  #include <asm/shadow.h>
>>  #include <asm/tboot.h>
>>  #include <asm/apic.h>
>> @@ -108,18 +109,6 @@ u64 vmx_ept_vpid_cap __read_mostly;
>>  u64 vmx_vmfunc __read_mostly;
>>  bool_t vmx_virt_exception __read_mostly;
>>  
>> -const u32 vmx_introspection_force_enabled_msrs[] = {
>> -    MSR_IA32_SYSENTER_EIP,
>> -    MSR_IA32_SYSENTER_ESP,
>> -    MSR_IA32_SYSENTER_CS,
>> -    MSR_IA32_MC0_CTL,
>> -    MSR_STAR,
>> -    MSR_LSTAR
>> -};
> 
> What takes care of enabling monitoring for these MSRs, or are you
> expecting the introspection engine to now explicitly register for each
> MSR it wants?  Either is fine, but I suspect the latter, which is a
> behavioural change in the interface.

Yes, that's the idea - the consumer now needs to explicitly register for
each MSR it wants write events for (and is able to unsubscribe
individually to MSRs that are no longer interesting). This provides
fine-grained control over MSR vm_events and can be a performance
optimization for an introspection application, since no vm_event exits
will occur for MSRs that are no longer interesting.

>> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
>> index 1fec412..2bc05ed 100644
>> --- a/xen/arch/x86/monitor.c
>> +++ b/xen/arch/x86/monitor.c
>> @@ -22,6 +22,74 @@
>>  #include <asm/monitor.h>
>>  #include <public/vm_event.h>
>>  
>> +static int monitor_enable_msr(struct domain *d, u32 msr)
>> +{
>> +    if ( !d->arch.monitor_msr_bitmap )
>> +        return -ENXIO;
>> +
>> +    if ( msr <= 0x1fff )
>> +        __set_bit(msr, &d->arch.monitor_msr_bitmap->low);
>> +    else if ( (msr >= 0x40000000) && (msr <= 0x40001fff) )
>> +    {
>> +        msr &= 0x1fff;
>> +        __set_bit(msr, &d->arch.monitor_msr_bitmap->hypervisor);
>> +    }
>> +    else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) )
>> +    {
>> +        msr &= 0x1fff;
>> +        __set_bit(msr, &d->arch.monitor_msr_bitmap->high);
>> +    }
>> +
>> +    hvm_enable_msr_interception(d, msr);
>> +
>> +    return 0;
>> +}
>> +
>> +static int monitor_disable_msr(struct domain *d, u32 msr)
>> +{
>> +    if ( !d->arch.monitor_msr_bitmap )
>> +        return -ENXIO;
>> +
>> +    if ( msr <= 0x1fff )
>> +        clear_bit(msr, &d->arch.monitor_msr_bitmap->low);
>> +    else if ( (msr >= 0x40000000) && (msr <= 0x40001fff) )
>> +    {
>> +        msr &= 0x1fff;
>> +        clear_bit(msr, &d->arch.monitor_msr_bitmap->hypervisor);
>> +    }
>> +    else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) )
>> +    {
>> +        msr &= 0x1fff;
>> +        clear_bit(msr, &d->arch.monitor_msr_bitmap->high);
> 
> __clear_bit()

Fair enough, will change it.

>> +    }
>> +
> 
> What about disabling MSR interception, to mirror the enable side?  (This
> is starting to get complicated - should we start refcounting how many
> entities want an MSR intercepted?  This sounds suboptimal).

Yes, that's why I've just left them on. This would indeed require
reference counting for each MSR to work properly, which would again
require more memory to store the refcount.

>> +
>> +    return 0;
>> +}
> 
> You should also return -EINVAL for a bad MSR index, and BUILD_BUG_ON()
> if the size of the bitmaps change.  You can also combine these
> functions, to reduce the code duplication and risk of divergence.  e.g.
> make a helper which looks like this
> 
> static void *monitor_bitmap_for_msr(struct domain *d, u32 *msr)
> {
>     ASSERT(d->arch.monitor_msr_bitmap);
> 
>     switch ( msr )
>     {
>     case 0 ... 0x1fff:
>         BUILD_BUG_ON(ARRAY_SIZE(d->arch.monitor_msr_bitmap->low) * 8 <
> 0x1fff));
>         return &d->arch.monitor_msr_bitmap->low;
> 
>     case 0x40000000 ... 0x40001fff:
>         BUILD_BUG_ON(ARRAY_SIZE(d->arch.monitor_msr_bitmap->hypervisor)
> * 8 < 0x1fff));
>         *msr &= 0x1fff;
>         return &d->arch.monitor_msr_bitmap->hypervisor;
> 
>     case 0xc0000000 ... 0xc0001fff:
>         BUILD_BUG_ON(ARRAY_SIZE(d->arch.monitor_msr_bitmap->high) * 8 <
> 0x1fff));
>         *msr &= 0x1fff;
>         return = &d->arch.monitor_msr_bitmap->high;
> 
>     default:
>         return NULL;
>     }
> }
> 
> To reduce the complexity of the other functions.

That's a great suggestion, I'll make that change.

>> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
>> index 5635603..22819c5 100644
>> --- a/xen/arch/x86/vm_event.c
>> +++ b/xen/arch/x86/vm_event.c
>> @@ -20,6 +20,7 @@
>>  
>>  #include <xen/sched.h>
>>  #include <asm/hvm/hvm.h>
>> +#include <asm/monitor.h>
>>  #include <asm/vm_event.h>
>>  
>>  /* Implicitly serialized by the domctl lock. */
>> @@ -27,6 +28,11 @@ int vm_event_init_domain(struct domain *d)
>>  {
>>      struct vcpu *v;
>>  
>> +    d->arch.monitor_msr_bitmap = xzalloc(struct monitor_msr_bitmap);
>> +
>> +    if ( !d->arch.monitor_msr_bitmap )
>> +        return -ENOMEM;
> 
> Shouldn't we in principle have a monitor_domain_init() function for
> this, or are monitor and vm_event too intertwined in practice?

I think they are (it doesn't really make sense to try to monitor MSR
writes without initializing the vm_event subsystem). Maybe Tamas has
different ideas?


Thanks,
Razvan

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