| Message ID | 1460710927-4138-1-git-send-email-rcojocaru@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 15, 2016 at 12:02:07PM +0300, 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 <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 +-- Subject to an ack from hypervisor maintainer: Acked-by: Wei Liu <wei.liu2@citrix.com>
On 04/15/16 12:02, Razvan Cojocaru wrote: > + else if ( (msr >= 0x80000000) && (msr <= 0x80001fff) ) > + { > + msr &= 0x1fff; > + __set_bit(msr, &d->arch.monitor_msr_bitmap->high); > + } This of course should be 0xc0000000 and 0xc0001fff. Sorry for the typo. Will fix it in V4 (but waiting for other comments in the meantime). Thanks, Razvan
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? Tamas
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
On 04/15/16 20: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? 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. Thanks, Razvan
On Fri, Apr 15, 2016 at 11:18 AM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 15/04/16 18:12, Tamas K Lengyel wrote: > > > > On Fri, Apr 15, 2016 at 3:02 AM, Razvan Cojocaru < > <rcojocaru@bitdefender.com>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> >> 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 > Fair enough. Tamas
On Fri, Apr 15, 2016 at 11:19 AM, Razvan Cojocaru <rcojocaru@bitdefender.com > wrote: > On 04/15/16 20: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? > > 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
On 04/15/16 20:38, Tamas K Lengyel wrote: > > > On Fri, Apr 15, 2016 at 11:19 AM, Razvan Cojocaru > <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> wrote: > > On 04/15/16 20:12, Tamas K Lengyel wrote: > > > > > > On Fri, Apr 15, 2016 at 3:02 AM, Razvan Cojocaru > > <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com> > <mailto: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> > > <mailto: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? > > 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
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); int xc_monitor_singlestep(xc_interface *xch, domid_t domain_id, bool enable); int xc_monitor_software_breakpoint(xc_interface *xch, domid_t domain_id, bool enable); diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c index b1705dd..78131b2 100644 --- a/tools/libxc/xc_monitor.c +++ b/tools/libxc/xc_monitor.c @@ -86,8 +86,8 @@ int xc_monitor_write_ctrlreg(xc_interface *xch, domid_t domain_id, return do_domctl(xch, &domctl); } -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) { DECLARE_DOMCTL; @@ -96,7 +96,7 @@ int xc_monitor_mov_to_msr(xc_interface *xch, domid_t domain_id, bool enable, domctl.u.monitor_op.op = enable ? XEN_DOMCTL_MONITOR_OP_ENABLE : XEN_DOMCTL_MONITOR_OP_DISABLE; domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR; - domctl.u.monitor_op.u.mov_to_msr.extended_capture = extended_capture; + domctl.u.monitor_op.u.mov_to_msr.msr = msr; return do_domctl(xch, &domctl); } diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c index 56c5514..9c17f37 100644 --- a/xen/arch/x86/hvm/event.c +++ b/xen/arch/x86/hvm/event.c @@ -57,9 +57,8 @@ bool_t hvm_event_cr(unsigned int index, unsigned long value, unsigned long old) void hvm_event_msr(unsigned int msr, uint64_t value) { struct vcpu *curr = current; - struct arch_domain *ad = &curr->domain->arch; - if ( ad->monitor.mov_to_msr_enabled ) + if ( monitor_is_msr_enabled(curr->domain, msr) ) { vm_event_request_t req = { .reason = VM_EVENT_REASON_MOV_TO_MSR, diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index f24126d..447e3c2 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -3695,7 +3695,6 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content, bool_t mtrr; unsigned int edx, index; int ret = X86EMUL_OKAY; - struct arch_domain *currad = ¤t->domain->arch; HVMTRACE_3D(MSR_WRITE, msr, (uint32_t)msr_content, (uint32_t)(msr_content >> 32)); @@ -3703,7 +3702,7 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content, hvm_cpuid(1, NULL, NULL, NULL, &edx); mtrr = !!(edx & cpufeat_mask(X86_FEATURE_MTRR)); - if ( may_defer && unlikely(currad->monitor.mov_to_msr_enabled) ) + if ( may_defer && unlikely(monitor_is_msr_enabled(v->domain, msr)) ) { ASSERT(v->arch.vm_event); diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c index 8284281..24ad906 100644 --- a/xen/arch/x86/hvm/vmx/vmcs.c +++ b/xen/arch/x86/hvm/vmx/vmcs.c @@ -37,6 +37,7 @@ #include <asm/hvm/vmx/vvmx.h> #include <asm/hvm/vmx/vmcs.h> #include <asm/flushtlb.h> +#include <asm/monitor.h> #include <asm/shadow.h> #include <asm/tboot.h> #include <asm/apic.h> @@ -108,18 +109,6 @@ u64 vmx_ept_vpid_cap __read_mostly; u64 vmx_vmfunc __read_mostly; bool_t vmx_virt_exception __read_mostly; -const u32 vmx_introspection_force_enabled_msrs[] = { - MSR_IA32_SYSENTER_EIP, - MSR_IA32_SYSENTER_ESP, - MSR_IA32_SYSENTER_CS, - MSR_IA32_MC0_CTL, - MSR_STAR, - MSR_LSTAR -}; - -const unsigned int vmx_introspection_force_enabled_msrs_size = - ARRAY_SIZE(vmx_introspection_force_enabled_msrs); - static DEFINE_PER_CPU_READ_MOSTLY(paddr_t, vmxon_region); static DEFINE_PER_CPU(paddr_t, current_vmcs); static DEFINE_PER_CPU(struct list_head, active_vmcs_list); @@ -810,17 +799,8 @@ void vmx_disable_intercept_for_msr(struct vcpu *v, u32 msr, int type) if ( msr_bitmap == NULL ) return; - if ( unlikely(d->arch.monitor.mov_to_msr_enabled && - d->arch.monitor.mov_to_msr_extended) && - vm_event_check_ring(&d->vm_event->monitor) ) - { - unsigned int i; - - /* Filter out MSR-s needed for memory introspection */ - for ( i = 0; i < vmx_introspection_force_enabled_msrs_size; i++ ) - if ( msr == vmx_introspection_force_enabled_msrs[i] ) - return; - } + if ( unlikely(monitor_is_msr_enabled(d, msr)) ) + return; /* * See Intel PRM Vol. 3, 20.6.9 (MSR-Bitmap Address). Early manuals diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index bc4410f..9135441 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -1958,16 +1958,12 @@ void vmx_hypervisor_cpuid_leaf(uint32_t sub_idx, *eax |= XEN_HVM_CPUID_X2APIC_VIRT; } -static void vmx_enable_msr_exit_interception(struct domain *d) +static void vmx_enable_msr_interception(struct domain *d, uint32_t msr) { struct vcpu *v; - unsigned int i; - /* Enable interception for MSRs needed for memory introspection. */ for_each_vcpu ( d, v ) - for ( i = 0; i < vmx_introspection_force_enabled_msrs_size; i++ ) - vmx_enable_intercept_for_msr(v, vmx_introspection_force_enabled_msrs[i], - MSR_TYPE_W); + vmx_enable_intercept_for_msr(v, msr, MSR_TYPE_W); } static bool_t vmx_is_singlestep_supported(void) @@ -2166,7 +2162,7 @@ static struct hvm_function_table __initdata vmx_function_table = { .handle_eoi = vmx_handle_eoi, .nhvm_hap_walk_L1_p2m = nvmx_hap_walk_L1_p2m, .hypervisor_cpuid_leaf = vmx_hypervisor_cpuid_leaf, - .enable_msr_exit_interception = vmx_enable_msr_exit_interception, + .enable_msr_interception = vmx_enable_msr_interception, .is_singlestep_supported = vmx_is_singlestep_supported, .set_mode = vmx_set_mode, .altp2m_vcpu_update_p2m = vmx_vcpu_update_eptp, diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c index 1fec412..2b0ff08 100644 --- a/xen/arch/x86/monitor.c +++ b/xen/arch/x86/monitor.c @@ -22,6 +22,74 @@ #include <asm/monitor.h> #include <public/vm_event.h> +static int monitor_enable_msr(struct domain *d, u32 msr) +{ + if ( !d->arch.monitor_msr_bitmap ) + return -ENXIO; + + if ( msr <= 0x1fff ) + __set_bit(msr, &d->arch.monitor_msr_bitmap->low); + else if ( (msr >= 0x40000000) && (msr <= 0x40001fff) ) + { + msr &= 0x1fff; + __set_bit(msr, &d->arch.monitor_msr_bitmap->hypervisor); + } + else if ( (msr >= 0x80000000) && (msr <= 0x80001fff) ) + { + msr &= 0x1fff; + __set_bit(msr, &d->arch.monitor_msr_bitmap->high); + } + + hvm_enable_msr_interception(d, msr); + + return 0; +} + +static int monitor_disable_msr(struct domain *d, u32 msr) +{ + if ( !d->arch.monitor_msr_bitmap ) + return -ENXIO; + + if ( msr <= 0x1fff ) + clear_bit(msr, &d->arch.monitor_msr_bitmap->low); + else if ( (msr >= 0x40000000) && (msr <= 0x40001fff) ) + { + msr &= 0x1fff; + clear_bit(msr, &d->arch.monitor_msr_bitmap->hypervisor); + } + else if ( (msr >= 0x80000000) && (msr <= 0x80001fff) ) + { + msr &= 0x1fff; + clear_bit(msr, &d->arch.monitor_msr_bitmap->high); + } + + + return 0; +} + +bool_t monitor_is_msr_enabled(const struct domain *d, u32 msr) +{ + bool_t rc = 0; + + if ( !d->arch.monitor_msr_bitmap ) + return 0; + + if ( msr <= 0x1fff ) + rc = test_bit(msr, &d->arch.monitor_msr_bitmap->low); + else if ( (msr >= 0x40000000) && (msr <= 0x40001fff) ) + { + msr &= 0x1fff; + rc = test_bit(msr, &d->arch.monitor_msr_bitmap->hypervisor); + } + else if ( (msr >= 0x80000000) && (msr <= 0x80001fff) ) + { + msr &= 0x1fff; + rc = test_bit(msr, &d->arch.monitor_msr_bitmap->high); + } + + return rc; +} + int arch_monitor_domctl_event(struct domain *d, struct xen_domctl_monitor_op *mop) { @@ -77,25 +145,28 @@ int arch_monitor_domctl_event(struct domain *d, case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR: { - bool_t old_status = ad->monitor.mov_to_msr_enabled; + bool_t old_status; + int rc; + u32 msr = mop->u.mov_to_msr.msr; - if ( unlikely(old_status == requested_status) ) - return -EEXIST; + domain_pause(d); - if ( requested_status && mop->u.mov_to_msr.extended_capture && - !hvm_enable_msr_exit_interception(d) ) - return -EOPNOTSUPP; + old_status = monitor_is_msr_enabled(d, msr); - domain_pause(d); + if ( unlikely(old_status == requested_status) ) + { + domain_unpause(d); + return -EEXIST; + } - if ( requested_status && mop->u.mov_to_msr.extended_capture ) - ad->monitor.mov_to_msr_extended = 1; + if ( requested_status ) + rc = monitor_enable_msr(d, msr); else - ad->monitor.mov_to_msr_extended = 0; + rc = monitor_disable_msr(d, msr); - ad->monitor.mov_to_msr_enabled = requested_status; domain_unpause(d); - break; + + return rc; } case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP: diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c index 5635603..22819c5 100644 --- a/xen/arch/x86/vm_event.c +++ b/xen/arch/x86/vm_event.c @@ -20,6 +20,7 @@ #include <xen/sched.h> #include <asm/hvm/hvm.h> +#include <asm/monitor.h> #include <asm/vm_event.h> /* Implicitly serialized by the domctl lock. */ @@ -27,6 +28,11 @@ int vm_event_init_domain(struct domain *d) { struct vcpu *v; + d->arch.monitor_msr_bitmap = xzalloc(struct monitor_msr_bitmap); + + if ( !d->arch.monitor_msr_bitmap ) + return -ENOMEM; + for_each_vcpu ( d, v ) { if ( v->arch.vm_event ) @@ -55,6 +61,9 @@ void vm_event_cleanup_domain(struct domain *d) v->arch.vm_event = NULL; } + xfree(d->arch.monitor_msr_bitmap); + d->arch.monitor_msr_bitmap = NULL; + d->arch.mem_access_emulate_each_rep = 0; memset(&d->arch.monitor, 0, sizeof(d->arch.monitor)); memset(&d->monitor, 0, sizeof(d->monitor)); diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h index d393ed2..591ef26 100644 --- a/xen/include/asm-x86/domain.h +++ b/xen/include/asm-x86/domain.h @@ -398,12 +398,12 @@ struct arch_domain unsigned int write_ctrlreg_enabled : 4; unsigned int write_ctrlreg_sync : 4; unsigned int write_ctrlreg_onchangeonly : 4; - unsigned int mov_to_msr_enabled : 1; - unsigned int mov_to_msr_extended : 1; unsigned int singlestep_enabled : 1; unsigned int software_breakpoint_enabled : 1; } monitor; + struct monitor_msr_bitmap *monitor_msr_bitmap; + /* Mem_access emulation control */ bool_t mem_access_emulate_each_rep; diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h index 7b7ff3f..9d1c0ef 100644 --- a/xen/include/asm-x86/hvm/hvm.h +++ b/xen/include/asm-x86/hvm/hvm.h @@ -211,7 +211,7 @@ struct hvm_function_table { uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx); - void (*enable_msr_exit_interception)(struct domain *d); + void (*enable_msr_interception)(struct domain *d, uint32_t msr); bool_t (*is_singlestep_supported)(void); int (*set_mode)(struct vcpu *v, int mode); @@ -565,11 +565,11 @@ static inline enum hvm_intblk nhvm_interrupt_blocked(struct vcpu *v) return hvm_funcs.nhvm_intr_blocked(v); } -static inline bool_t hvm_enable_msr_exit_interception(struct domain *d) +static inline bool_t hvm_enable_msr_interception(struct domain *d, uint32_t msr) { - if ( hvm_funcs.enable_msr_exit_interception ) + if ( hvm_funcs.enable_msr_interception ) { - hvm_funcs.enable_msr_exit_interception(d); + hvm_funcs.enable_msr_interception(d, msr); return 1; } diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h index b54f52f..7bf5326 100644 --- a/xen/include/asm-x86/hvm/vmx/vmcs.h +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h @@ -562,13 +562,6 @@ enum vmcs_field { HOST_RIP = 0x00006c16, }; -/* - * A set of MSR-s that need to be enabled for memory introspection - * to work. - */ -extern const u32 vmx_introspection_force_enabled_msrs[]; -extern const unsigned int vmx_introspection_force_enabled_msrs_size; - #define VMCS_VPID_WIDTH 16 #define MSR_TYPE_R 1 diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h index 0954b59..5056452 100644 --- a/xen/include/asm-x86/monitor.h +++ b/xen/include/asm-x86/monitor.h @@ -29,6 +29,12 @@ #define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index)) +struct monitor_msr_bitmap { + uint8_t low[1024]; + uint8_t hypervisor[1024]; + uint8_t high[1024]; +}; + static inline int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop) { @@ -50,4 +56,6 @@ int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop) int arch_monitor_domctl_event(struct domain *d, struct xen_domctl_monitor_op *mop); +bool_t monitor_is_msr_enabled(const struct domain *d, u32 msr); + #endif /* __ASM_X86_MONITOR_H__ */ diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index 2457698..7be3924 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -37,7 +37,7 @@ #include "hvm/save.h" #include "memory.h" -#define XEN_DOMCTL_INTERFACE_VERSION 0x0000000b +#define XEN_DOMCTL_INTERFACE_VERSION 0x0000000c /* * NB. xen_domctl.domain is an IN/OUT parameter for this operation. @@ -1107,8 +1107,7 @@ struct xen_domctl_monitor_op { } mov_to_cr; struct { - /* Enable the capture of an extended set of MSRs */ - uint8_t extended_capture; + uint32_t msr; } mov_to_msr; struct {
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(-)