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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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.

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

> 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()

> +    }
> +

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

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

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

~Andrew

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