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

[Andrew Cooper <andrew.cooper3@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3574 bytes --]

On 15/04/16 18: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?

The MSRs are specified by the Intel/AMD manuals.  Furthermore, for the
non-architectural ones, it is quite possible that the same index maps to
different MSRs on different hardware.  It is definitely the case that
different hadware has the same MSR at different indices.  (The Intel
cpuid masking MSRs have this propery across different CPU generations).

I expect the monitoring application to know the current hardware, and
which MSRs are applicable.

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 5746 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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