|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH V5] vm_event: Allow subscribing to write events for specific MSR-s
On 04/26/2016 11:49 AM, Razvan Cojocaru wrote:
> Previously, subscribing to MSR write events was an all-or-none
> approach, with special cases for introspection MSR-s. This patch
> allows the vm_event consumer to specify exactly what MSR-s it is
> interested in, and as a side-effect gets rid of the
> vmx_introspection_force_enabled_msrs[] special case.
> This replaces the previously posted "xen: Filter out MSR write
> events" patch.
>
> Signed-off-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
>
> ---
> Changes since V4:
> - Added arch_monitor_init_domain() and arch_monitor_cleanup_domain()
> as suggested by Tamas Lengyel and Andrew Cooper.
> - Factored out common MSR range code in monitor_bitmap_for_msr(),
> as suggested by Andrew Cooper.
> - Renamed monitor_is_msr_enabled() to monitored_msr(), as
> suggested by Andrew Cooper.
> - Replaced clear_bit() with __clear_bit().
> - Now returning -EINVAL when monitor_bitmap_for_msr() returns NULL
> (i.e. when the MSR is out of range).
> - Since these are slightly more than minor changes, removed all
> previous Acks.
> ---
> tools/libxc/include/xenctrl.h | 9 ++-
> tools/libxc/xc_monitor.c | 6 +-
> xen/arch/x86/hvm/event.c | 3 +-
> xen/arch/x86/hvm/hvm.c | 3 +-
> xen/arch/x86/hvm/vmx/vmcs.c | 26 +-------
> xen/arch/x86/hvm/vmx/vmx.c | 10 +---
> xen/arch/x86/monitor.c | 120
> +++++++++++++++++++++++++++++++++----
> xen/arch/x86/vm_event.c | 9 +++
> xen/common/vm_event.c | 5 ++
> xen/include/asm-arm/monitor.h | 13 ++++
> xen/include/asm-x86/domain.h | 4 +-
> xen/include/asm-x86/hvm/hvm.h | 8 +--
> xen/include/asm-x86/hvm/vmx/vmcs.h | 7 ---
> xen/include/asm-x86/monitor.h | 12 ++++
> xen/include/public/domctl.h | 5 +-
> 15 files changed, 173 insertions(+), 67 deletions(-)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index f5a034a..3216e48 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2183,8 +2183,13 @@ int xc_monitor_get_capabilities(xc_interface *xch,
> domid_t domain_id,
> int xc_monitor_write_ctrlreg(xc_interface *xch, domid_t domain_id,
> uint16_t index, bool enable, bool sync,
> bool onchangeonly);
> -int xc_monitor_mov_to_msr(xc_interface *xch, domid_t domain_id, bool enable,
> - bool extended_capture);
> +/*
> + * A list of MSR indices can usually be found in
> /usr/include/asm/msr-index.h.
> + * Please consult the Intel/AMD manuals for more information on
> + * non-architectural indices.
> + */
> +int xc_monitor_mov_to_msr(xc_interface *xch, domid_t domain_id, uint32_t msr,
> + bool enable);
> int xc_monitor_singlestep(xc_interface *xch, domid_t domain_id, bool enable);
> int xc_monitor_software_breakpoint(xc_interface *xch, domid_t domain_id,
> bool enable);
> diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
> index b1705dd..78131b2 100644
> --- a/tools/libxc/xc_monitor.c
> +++ b/tools/libxc/xc_monitor.c
> @@ -86,8 +86,8 @@ int xc_monitor_write_ctrlreg(xc_interface *xch, domid_t
> domain_id,
> return do_domctl(xch, &domctl);
> }
>
> -int xc_monitor_mov_to_msr(xc_interface *xch, domid_t domain_id, bool enable,
> - bool extended_capture)
> +int xc_monitor_mov_to_msr(xc_interface *xch, domid_t domain_id, uint32_t msr,
> + bool enable)
> {
> DECLARE_DOMCTL;
>
> @@ -96,7 +96,7 @@ int xc_monitor_mov_to_msr(xc_interface *xch, domid_t
> domain_id, bool enable,
> domctl.u.monitor_op.op = enable ? XEN_DOMCTL_MONITOR_OP_ENABLE
> : XEN_DOMCTL_MONITOR_OP_DISABLE;
> domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR;
> - domctl.u.monitor_op.u.mov_to_msr.extended_capture = extended_capture;
> + domctl.u.monitor_op.u.mov_to_msr.msr = msr;
>
> return do_domctl(xch, &domctl);
> }
> diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
> index 56c5514..8fdb6f5 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 ( monitored_msr(curr->domain, msr) )
> {
> vm_event_request_t req = {
> .reason = VM_EVENT_REASON_MOV_TO_MSR,
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index e9d4c6b..7495820 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3694,7 +3694,6 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t
> msr_content,
> bool_t mtrr;
> unsigned int edx, index;
> int ret = X86EMUL_OKAY;
> - struct arch_domain *currad = ¤t->domain->arch;
>
> HVMTRACE_3D(MSR_WRITE, msr,
> (uint32_t)msr_content, (uint32_t)(msr_content >> 32));
> @@ -3702,7 +3701,7 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t
> msr_content,
> hvm_cpuid(1, NULL, NULL, NULL, &edx);
> mtrr = !!(edx & cpufeat_mask(X86_FEATURE_MTRR));
>
> - if ( may_defer && unlikely(currad->monitor.mov_to_msr_enabled) )
> + if ( may_defer && unlikely(monitored_msr(v->domain, msr)) )
> {
> ASSERT(v->arch.vm_event);
>
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index 8284281..f8421e8 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
> -};
> -
> -const unsigned int vmx_introspection_force_enabled_msrs_size =
> - ARRAY_SIZE(vmx_introspection_force_enabled_msrs);
> -
> static DEFINE_PER_CPU_READ_MOSTLY(paddr_t, vmxon_region);
> static DEFINE_PER_CPU(paddr_t, current_vmcs);
> static DEFINE_PER_CPU(struct list_head, active_vmcs_list);
> @@ -810,17 +799,8 @@ void vmx_disable_intercept_for_msr(struct vcpu *v, u32
> msr, int type)
> if ( msr_bitmap == NULL )
> return;
>
> - if ( unlikely(d->arch.monitor.mov_to_msr_enabled &&
> - d->arch.monitor.mov_to_msr_extended) &&
> - vm_event_check_ring(&d->vm_event->monitor) )
> - {
> - unsigned int i;
> -
> - /* Filter out MSR-s needed for memory introspection */
> - for ( i = 0; i < vmx_introspection_force_enabled_msrs_size; i++ )
> - if ( msr == vmx_introspection_force_enabled_msrs[i] )
> - return;
> - }
> + if ( unlikely(monitored_msr(d, msr)) )
> + return;
>
> /*
> * See Intel PRM Vol. 3, 20.6.9 (MSR-Bitmap Address). Early manuals
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index bc4410f..9135441 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1958,16 +1958,12 @@ 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)
> +static void vmx_enable_msr_interception(struct domain *d, uint32_t msr)
> {
> struct vcpu *v;
> - unsigned int i;
>
> - /* Enable interception for MSRs needed for memory introspection. */
> for_each_vcpu ( d, v )
> - for ( i = 0; i < vmx_introspection_force_enabled_msrs_size; i++ )
> - vmx_enable_intercept_for_msr(v,
> vmx_introspection_force_enabled_msrs[i],
> - MSR_TYPE_W);
> + vmx_enable_intercept_for_msr(v, msr, MSR_TYPE_W);
> }
>
> static bool_t vmx_is_singlestep_supported(void)
> @@ -2166,7 +2162,7 @@ static struct hvm_function_table __initdata
> vmx_function_table = {
> .handle_eoi = vmx_handle_eoi,
> .nhvm_hap_walk_L1_p2m = nvmx_hap_walk_L1_p2m,
> .hypervisor_cpuid_leaf = vmx_hypervisor_cpuid_leaf,
> - .enable_msr_exit_interception = vmx_enable_msr_exit_interception,
> + .enable_msr_interception = vmx_enable_msr_interception,
> .is_singlestep_supported = vmx_is_singlestep_supported,
> .set_mode = vmx_set_mode,
> .altp2m_vcpu_update_p2m = vmx_vcpu_update_eptp,
> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
> index 1fec412..df56981 100644
> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c
> @@ -22,6 +22,99 @@
> #include <asm/monitor.h>
> #include <public/vm_event.h>
>
> +int arch_monitor_init_domain(struct domain *d)
> +{
> + d->arch.monitor_msr_bitmap = xzalloc(struct monitor_msr_bitmap);
> +
> + if ( !d->arch.monitor_msr_bitmap )
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +void arch_monitor_cleanup_domain(struct domain *d)
> +{
> + xfree(d->arch.monitor_msr_bitmap);
> + d->arch.monitor_msr_bitmap = NULL;
> +}
> +
> +static void *monitor_bitmap_for_msr(struct domain *d, u32 *msr)
> +{
> + ASSERT(d->arch.monitor_msr_bitmap && msr);
> +
> + 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;
> + }
> +}
> +
> +static int monitor_enable_msr(struct domain *d, u32 msr)
> +{
> + u32 *bitmap;
> +
> + if ( !d->arch.monitor_msr_bitmap )
> + return -ENXIO;
> +
> + bitmap = monitor_bitmap_for_msr(d, &msr);
> +
> + if ( !bitmap )
> + return -EINVAL;
> +
> + __set_bit(msr, bitmap);
> +
> + hvm_enable_msr_interception(d, msr);
> +
> + return 0;
> +}
> +
> +static int monitor_disable_msr(struct domain *d, u32 msr)
> +{
> + u32 *bitmap;
> +
> + if ( !d->arch.monitor_msr_bitmap )
> + return -ENXIO;
> +
> + bitmap = monitor_bitmap_for_msr(d, &msr);
> +
> + if ( !bitmap )
> + return -EINVAL;
> +
> + __clear_bit(msr, bitmap);
> +
> + return 0;
> +}
> +
> +bool_t monitored_msr(struct domain *d, u32 msr)
> +{
> + u32 *bitmap;
> +
> + if ( !d->arch.monitor_msr_bitmap )
> + return 0;
> +
> + bitmap = monitor_bitmap_for_msr(d, &msr);
> +
> + if ( !bitmap )
> + return 0;
> +
> + return test_bit(msr, bitmap);
> +}
> +
> int arch_monitor_domctl_event(struct domain *d,
> struct xen_domctl_monitor_op *mop)
> {
> @@ -77,25 +170,28 @@ int arch_monitor_domctl_event(struct domain *d,
>
> case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR:
> {
> - bool_t old_status = ad->monitor.mov_to_msr_enabled;
> + bool_t old_status;
> + int rc;
> + u32 msr = mop->u.mov_to_msr.msr;
>
> - if ( unlikely(old_status == requested_status) )
> - return -EEXIST;
> + domain_pause(d);
>
> - if ( requested_status && mop->u.mov_to_msr.extended_capture &&
> - !hvm_enable_msr_exit_interception(d) )
> - return -EOPNOTSUPP;
> + old_status = monitored_msr(d, msr);
>
> - domain_pause(d);
> + if ( unlikely(old_status == requested_status) )
> + {
> + domain_unpause(d);
> + return -EEXIST;
> + }
>
> - if ( requested_status && mop->u.mov_to_msr.extended_capture )
> - ad->monitor.mov_to_msr_extended = 1;
> + if ( requested_status )
> + rc = monitor_enable_msr(d, msr);
> else
> - ad->monitor.mov_to_msr_extended = 0;
> + rc = monitor_disable_msr(d, msr);
>
> - ad->monitor.mov_to_msr_enabled = requested_status;
> domain_unpause(d);
> - break;
> +
> + return rc;
> }
>
> case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP:
> 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;
> +
> for_each_vcpu ( d, v )
> {
> if ( v->arch.vm_event )
> @@ -55,6 +61,9 @@ void vm_event_cleanup_domain(struct domain *d)
> v->arch.vm_event = NULL;
> }
>
> + xfree(d->arch.monitor_msr_bitmap);
> + d->arch.monitor_msr_bitmap = NULL;
> +
> d->arch.mem_access_emulate_each_rep = 0;
> memset(&d->arch.monitor, 0, sizeof(d->arch.monitor));
> memset(&d->monitor, 0, sizeof(d->monitor));
Aside from accidentally duplicating the alloc() / free() code here, I
should have probably moved the following monitor-related memset()s in
arch_monitor_init_domain() as well. I'll do that in V6.
Thanks,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |