From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH V3] vm_event: Allow subscribing to write events for specific MSR-s Date: Fri, 15 Apr 2016 18:18:11 +0100 Message-ID: <57112253.7040308@citrix.com> References: <1460710927-4138-1-git-send-email-rcojocaru@bitdefender.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3449102163691789518==" Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Tamas K Lengyel , Razvan Cojocaru Cc: Kevin Tian , "wei.liu2@citrix.com" , Jan Beulich , Ian Jackson , Xen-devel , Jun Nakajima , Keir Fraser List-Id: xen-devel@lists.xenproject.org --===============3449102163691789518== Content-Type: multipart/alternative; boundary="------------050503060005000707070606" --------------050503060005000707070606 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit On 15/04/16 18:12, Tamas K Lengyel wrote: > > > On Fri, Apr 15, 2016 at 3:02 AM, Razvan Cojocaru > > 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 > > > --- > 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 --------------050503060005000707070606 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 8bit
On 15/04/16 18:12, Tamas K Lengyel wrote:


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

---
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
--------------050503060005000707070606-- --===============3449102163691789518== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwOi8vbGlzdHMueGVuLm9y Zy94ZW4tZGV2ZWwK --===============3449102163691789518==--