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:18 AM, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:

On 15/04/16 18:12, Tamas K Lengyel wrote:

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

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

Fair enough.

Tamas

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