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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Fri, Apr 15, 2016 at 11:19 AM, Razvan Cojocaru <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>> 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>>

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

Tamas

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