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

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 04/15/16 20:12, Tamas K Lengyel wrote:
> 
> 
> On Fri, Apr 15, 2016 at 3:02 AM, Razvan Cojocaru
> <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> 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@bitdefender.com
>     <mailto:rcojocaru@bitdefender.com>>
> 
>     ---
>     Changes since V2:
>      - Bumped XEN_DOMCTL_INTERFACE_VERSION.
>      - Introduced struct monitor_msr_bitmap as recommended by Andrew
>        Cooper, which allowed removing some pointer arithmetic magic.
>      - Removed arch_ prefix from monitor functions, as recommended
>        by Tamas Lengyel.
>      - Replaced the page allocation code with xzalloc() / xfree() for
>        struct monitor_msr_bitmap.
>      - Now returning -ENXIO instead of -EINVAL from the monitor
>        functions, as recommended by Konrad Rzeszutek Wilk.
>     ---
>      tools/libxc/include/xenctrl.h      |  4 +-
>      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             | 95
>     +++++++++++++++++++++++++++++++++-----
>      xen/arch/x86/vm_event.c            |  9 ++++
>      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      |  8 ++++
>      xen/include/public/domctl.h        |  5 +-
>      13 files changed, 121 insertions(+), 67 deletions(-)
> 
>     diff --git a/tools/libxc/include/xenctrl.h
>     b/tools/libxc/include/xenctrl.h
>     index f5a034a..9698d46 100644
>     --- a/tools/libxc/include/xenctrl.h
>     +++ b/tools/libxc/include/xenctrl.h
>     @@ -2183,8 +2183,8 @@ 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);
>     +int xc_monitor_mov_to_msr(xc_interface *xch, domid_t domain_id,
>     uint32_t msr,
>     +                          bool enable);
> 
>  
> So my only concern with this approach here is that the MSR index
> definitions that are supposed to be passed are never exported via a
> public header, are only defined in asm-x86/msr-index.h. Should that also
> be moved to be a public header as part of this patch?

There's usually an OS header defining those constants, at least with
Linux. I've just checked on my Arch Linux now and I have
/usr/include/asm/msr-index.h, so I would say that's not necessary.

Having said that, if you'd prefer that the Xen header file be made
public I'm happy to do that.


Thanks,
Razvan

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