|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH V2] vm_event: Allow subscribing to write events for specific MSR-s
> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
> index 1fec412..4c96968 100644
> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c
> @@ -22,6 +22,58 @@
> #include <asm/monitor.h>
> #include <public/vm_event.h>
>
> +static int arch_monitor_enable_msr(struct domain *d, u32 msr)
> +{
> + if ( !d->arch.monitor_msr_bitmap )
> + return -EINVAL;
I this was not set wouldn't we fail in vm_event_enable with -ENOMEM?
I presume the user can still make this hypercall.. Ah yes.
Perhaps -ENXIO?
> +
> + if ( msr <= 0x1fff )
> + set_bit(msr, d->arch.monitor_msr_bitmap + 0x000/BYTES_PER_LONG);
The 0x000/BYTER_PER_LONG looks odd. Is it even needed?
> + else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) )
> + {
> + msr &= 0x1fff;
> + set_bit(msr, d->arch.monitor_msr_bitmap + 0x400/BYTES_PER_LONG);
> + }
> +
> + hvm_enable_msr_interception(d, msr);
And for MSRs above 0xc0001fff it is OK to enable the interception?
Or between 0x1fff and 0xc0000000?
No need to filter them out? Or error on them?
> +
> + return 0;
> +}
> +
> +static int arch_monitor_disable_msr(struct domain *d, u32 msr)
> +{
> + if ( !d->arch.monitor_msr_bitmap )
> + return -EINVAL;
> +
> + if ( msr <= 0x1fff )
> + clear_bit(msr, d->arch.monitor_msr_bitmap + 0x000/BYTES_PER_LONG);
> + else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) )
> + {
> + msr &= 0x1fff;
> + clear_bit(msr, d->arch.monitor_msr_bitmap + 0x400/BYTES_PER_LONG);
> + }
> +
> + return 0;
> +}
> +
> +bool_t arch_monitor_is_msr_enabled(const struct domain *d, u32 msr)
> +{
> + bool_t rc = 0;
> +
> + if ( !d->arch.monitor_msr_bitmap )
> + return 0;
> +
> + if ( msr <= 0x1fff )
> + rc = test_bit(msr, d->arch.monitor_msr_bitmap +
> 0x000/BYTES_PER_LONG);
> + else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) )
> + {
> + msr &= 0x1fff;
> + rc = test_bit(msr, d->arch.monitor_msr_bitmap +
> 0x400/BYTES_PER_LONG);
> + }
And what if msr requested is above 0xc0001fff ? What then?
> +
> + return rc;
> +}
> +
> int arch_monitor_domctl_event(struct domain *d,
> struct xen_domctl_monitor_op *mop)
> {
> @@ -77,25 +129,28 @@ int arch_monitor_domctl_event(struct domain *d,
>
> case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR:
Should this be renamed?
> {
> - 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 = arch_monitor_is_msr_enabled(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 = arch_monitor_enable_msr(d, msr);
> else
> - ad->monitor.mov_to_msr_extended = 0;
> + rc = arch_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..9b4267e 100644
> --- a/xen/arch/x86/vm_event.c
> +++ b/xen/arch/x86/vm_event.c
> @@ -27,6 +27,13 @@ int vm_event_init_domain(struct domain *d)
> {
> struct vcpu *v;
>
> + d->arch.monitor_msr_bitmap = alloc_xenheap_page();
How about using vzalloc?
> +
> + if ( !d->arch.monitor_msr_bitmap )
> + return -ENOMEM;
> +
> + memset(d->arch.monitor_msr_bitmap, 0, PAGE_SIZE);
Then you don't have to do that.
> +
> for_each_vcpu ( d, v )
> {
> if ( v->arch.vm_event )
> @@ -55,6 +62,9 @@ void vm_event_cleanup_domain(struct domain *d)
> v->arch.vm_event = NULL;
> }
>
> + free_xenheap_page(d->arch.monitor_msr_bitmap);
And this would be vfree.
> + 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));
> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index d393ed2..d8d91c2 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -398,12 +398,12 @@ struct arch_domain
> unsigned int write_ctrlreg_enabled : 4;
> unsigned int write_ctrlreg_sync : 4;
> unsigned int write_ctrlreg_onchangeonly : 4;
> - unsigned int mov_to_msr_enabled : 1;
> - unsigned int mov_to_msr_extended : 1;
> unsigned int singlestep_enabled : 1;
> unsigned int software_breakpoint_enabled : 1;
> } monitor;
>
> + unsigned long *monitor_msr_bitmap;
> +
> /* Mem_access emulation control */
> bool_t mem_access_emulate_each_rep;
>
> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
> index 7b7ff3f..9d1c0ef 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -211,7 +211,7 @@ struct hvm_function_table {
> uint32_t *eax, uint32_t *ebx,
> uint32_t *ecx, uint32_t *edx);
>
> - void (*enable_msr_exit_interception)(struct domain *d);
> + void (*enable_msr_interception)(struct domain *d, uint32_t msr);
> bool_t (*is_singlestep_supported)(void);
> int (*set_mode)(struct vcpu *v, int mode);
>
> @@ -565,11 +565,11 @@ static inline enum hvm_intblk
> nhvm_interrupt_blocked(struct vcpu *v)
> return hvm_funcs.nhvm_intr_blocked(v);
> }
>
> -static inline bool_t hvm_enable_msr_exit_interception(struct domain *d)
> +static inline bool_t hvm_enable_msr_interception(struct domain *d, uint32_t
> msr)
> {
> - if ( hvm_funcs.enable_msr_exit_interception )
> + if ( hvm_funcs.enable_msr_interception )
> {
> - hvm_funcs.enable_msr_exit_interception(d);
> + hvm_funcs.enable_msr_interception(d, msr);
> return 1;
> }
>
> diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h
> b/xen/include/asm-x86/hvm/vmx/vmcs.h
> index b54f52f..7bf5326 100644
> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
> @@ -562,13 +562,6 @@ enum vmcs_field {
> HOST_RIP = 0x00006c16,
> };
>
> -/*
> - * A set of MSR-s that need to be enabled for memory introspection
> - * to work.
> - */
> -extern const u32 vmx_introspection_force_enabled_msrs[];
> -extern const unsigned int vmx_introspection_force_enabled_msrs_size;
> -
> #define VMCS_VPID_WIDTH 16
>
> #define MSR_TYPE_R 1
> diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
> index 0954b59..74e5b1b 100644
> --- a/xen/include/asm-x86/monitor.h
> +++ b/xen/include/asm-x86/monitor.h
> @@ -50,4 +50,6 @@ int arch_monitor_domctl_op(struct domain *d, struct
> xen_domctl_monitor_op *mop)
> int arch_monitor_domctl_event(struct domain *d,
> struct xen_domctl_monitor_op *mop);
>
> +bool_t arch_monitor_is_msr_enabled(const struct domain *d, u32 msr);
> +
> #endif /* __ASM_X86_MONITOR_H__ */
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 2457698..875c09a 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -1107,8 +1107,7 @@ struct xen_domctl_monitor_op {
> } mov_to_cr;
>
> struct {
> - /* Enable the capture of an extended set of MSRs */
> - uint8_t extended_capture;
> + uint32_t msr;
Whoa there. Isn't it expanding the structure? Will this be backwards
compatible? What if somebody is using an older version of xen-access
against this hypervisor? Will they work?
Perhaps this should have a new struct / sub-ops? And the old
'mov_to_msr' will just re-use this new fangled code?
> } mov_to_msr;
>
> struct {
> --
> 1.9.1
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |