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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 04/15/16 20:38, Tamas K Lengyel wrote:
> 
> 
> On Fri, Apr 15, 2016 at 11:19 AM, Razvan Cojocaru
> <rcojocaru@xxxxxxxxxxxxxxx <mailto:rcojocaru@xxxxxxxxxxxxxxx>> wrote:
> 
>     On 04/15/16 20:12, Tamas K Lengyel wrote:
>     >
>     >
>     > On Fri, Apr 15, 2016 at 3:02 AM, Razvan Cojocaru
>     > <rcojocaru@xxxxxxxxxxxxxxx <mailto:rcojocaru@xxxxxxxxxxxxxxx>
>     <mailto:rcojocaru@xxxxxxxxxxxxxxx
>     <mailto:rcojocaru@xxxxxxxxxxxxxxx>>> 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 
> <mailto:rcojocaru@xxxxxxxxxxxxxxx>
>     >     <mailto:rcojocaru@xxxxxxxxxxxxxxx
>     <mailto:rcojocaru@xxxxxxxxxxxxxxx>>>
>     >
>     >     ---
>     >     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.
> 
> 
> I just checked on Debian and got the same header so I'm OK with that.
> Adding a comment might then be enough specifying that the most common
> MSR indices can usually be found there (with a note saying
> non-architectural MSRs should be gathered from the manuals).

That sounds fair, I'll add the comment.


Thanks,
Razvan


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