| Message ID | 1460392914-26033-1-git-send-email-rcojocaru@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 11, 2016 at 07:41:54PM +0300, Razvan Cojocaru wrote: > This patch only allows introspection-related MSR write events to > be sent out, improving performance. Should additional events be > required, they can then simply be added to the list of > vmx_introspection_force_enabled_msrs[]. > > Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Thought should there be some .. dynamic mechanism to update the MSR list? Or remove entries (or temporarily blacklist the built-one ins), etc? > --- > xen/arch/x86/hvm/hvm.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > index f24126d..21ba611 100644 > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -3688,6 +3688,17 @@ int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content) > goto out; > } > > +static bool_t is_introspection_msr(unsigned int msr) > +{ > + unsigned int i; > + > + for ( i = 0; i < vmx_introspection_force_enabled_msrs_size; i++ ) > + if ( msr == vmx_introspection_force_enabled_msrs[i] ) > + return 1; > + > + return 0; > +} > + > int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content, > bool_t may_defer) > { > @@ -3703,7 +3714,8 @@ 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(currad->monitor.mov_to_msr_enabled) && > + is_introspection_msr(msr) ) > { > ASSERT(v->arch.vm_event); > > -- > 2.8.0 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
On 04/11/16 22:18, Konrad Rzeszutek Wilk wrote: > On Mon, Apr 11, 2016 at 07:41:54PM +0300, Razvan Cojocaru wrote: >> This patch only allows introspection-related MSR write events to >> be sent out, improving performance. Should additional events be >> required, they can then simply be added to the list of >> vmx_introspection_force_enabled_msrs[]. >> >> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com> > > Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > Thought should there be some .. dynamic mechanism to update > the MSR list? Or remove entries (or temporarily blacklist > the built-one ins), etc? While this should be enough for now (the introspection MSR list is very small, and we're probably the only consumers), that's indeed the way for the future, I think. We should have a set-like container of unique elements that can grow and shrink and can be searched quickly, and add / remove MSRs we're interested in there. The CR write events bitmap approach clearly doesn't work, as there are too many MSRs to be able to select them all with a bitmap in the future. Thanks, Razvan
On 12/04/16 05:26, Razvan Cojocaru wrote: > On 04/11/16 22:18, Konrad Rzeszutek Wilk wrote: >> On Mon, Apr 11, 2016 at 07:41:54PM +0300, Razvan Cojocaru wrote: >>> This patch only allows introspection-related MSR write events to >>> be sent out, improving performance. Should additional events be >>> required, they can then simply be added to the list of >>> vmx_introspection_force_enabled_msrs[]. >>> >>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com> >> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >> >> Thought should there be some .. dynamic mechanism to update >> the MSR list? Or remove entries (or temporarily blacklist >> the built-one ins), etc? > While this should be enough for now (the introspection MSR list is very > small, and we're probably the only consumers), that's indeed the way for > the future, I think. We should have a set-like container of unique > elements that can grow and shrink and can be searched quickly, and add / > remove MSRs we're interested in there. > > The CR write events bitmap approach clearly doesn't work, as there are > too many MSRs to be able to select them all with a bitmap in the future. The current intercept bitmaps have 4x1K bitmaps, a read and a write bit for the low and high MSR indices. For introspection purposes, you will also want a bitmap covering the hypervisor range. For now, 3x1K bitmaps should be plenty to cover all potentially interesting MSRs. ~Andrew
On 04/12/2016 12:38 PM, Andrew Cooper wrote: > On 12/04/16 05:26, Razvan Cojocaru wrote: >> On 04/11/16 22:18, Konrad Rzeszutek Wilk wrote: >>> On Mon, Apr 11, 2016 at 07:41:54PM +0300, Razvan Cojocaru wrote: >>>> This patch only allows introspection-related MSR write events to >>>> be sent out, improving performance. Should additional events be >>>> required, they can then simply be added to the list of >>>> vmx_introspection_force_enabled_msrs[]. >>>> >>>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com> >>> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >>> >>> Thought should there be some .. dynamic mechanism to update >>> the MSR list? Or remove entries (or temporarily blacklist >>> the built-one ins), etc? >> While this should be enough for now (the introspection MSR list is very >> small, and we're probably the only consumers), that's indeed the way for >> the future, I think. We should have a set-like container of unique >> elements that can grow and shrink and can be searched quickly, and add / >> remove MSRs we're interested in there. >> >> The CR write events bitmap approach clearly doesn't work, as there are >> too many MSRs to be able to select them all with a bitmap in the future. > > The current intercept bitmaps have 4x1K bitmaps, a read and a write bit > for the low and high MSR indices. > > For introspection purposes, you will also want a bitmap covering the > hypervisor range. For now, 3x1K bitmaps should be plenty to cover all > potentially interesting MSRs. I see, so for future reference, add a pointer member allocated with alloc_xenheap_page() and memset() to 0 to struct arch_domain, maybe named monitor_msrs_bitmap (while removing mov_to_msr_enabled and mov_to_msr_extended from struct monitor)? This could get allocated on vm_event_init_domain() and de-allocated on vm_event_cleanup_domain(). Thanks, Razvan
On 12/04/16 11:17, Razvan Cojocaru wrote: > On 04/12/2016 12:38 PM, Andrew Cooper wrote: >> On 12/04/16 05:26, Razvan Cojocaru wrote: >>> On 04/11/16 22:18, Konrad Rzeszutek Wilk wrote: >>>> On Mon, Apr 11, 2016 at 07:41:54PM +0300, Razvan Cojocaru wrote: >>>>> This patch only allows introspection-related MSR write events to >>>>> be sent out, improving performance. Should additional events be >>>>> required, they can then simply be added to the list of >>>>> vmx_introspection_force_enabled_msrs[]. >>>>> >>>>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com> >>>> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >>>> >>>> Thought should there be some .. dynamic mechanism to update >>>> the MSR list? Or remove entries (or temporarily blacklist >>>> the built-one ins), etc? >>> While this should be enough for now (the introspection MSR list is very >>> small, and we're probably the only consumers), that's indeed the way for >>> the future, I think. We should have a set-like container of unique >>> elements that can grow and shrink and can be searched quickly, and add / >>> remove MSRs we're interested in there. >>> >>> The CR write events bitmap approach clearly doesn't work, as there are >>> too many MSRs to be able to select them all with a bitmap in the future. >> The current intercept bitmaps have 4x1K bitmaps, a read and a write bit >> for the low and high MSR indices. >> >> For introspection purposes, you will also want a bitmap covering the >> hypervisor range. For now, 3x1K bitmaps should be plenty to cover all >> potentially interesting MSRs. > I see, so for future reference, add a pointer member allocated with > alloc_xenheap_page() and memset() to 0 to struct arch_domain, maybe > named monitor_msrs_bitmap (while removing mov_to_msr_enabled and > mov_to_msr_extended from struct monitor)? This could get allocated on > vm_event_init_domain() and de-allocated on vm_event_cleanup_domain(). Looks suitable. ~Andrew
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index f24126d..21ba611 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -3688,6 +3688,17 @@ int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content) goto out; } +static bool_t is_introspection_msr(unsigned int msr) +{ + unsigned int i; + + for ( i = 0; i < vmx_introspection_force_enabled_msrs_size; i++ ) + if ( msr == vmx_introspection_force_enabled_msrs[i] ) + return 1; + + return 0; +} + int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content, bool_t may_defer) { @@ -3703,7 +3714,8 @@ 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(currad->monitor.mov_to_msr_enabled) && + is_introspection_msr(msr) ) { ASSERT(v->arch.vm_event);
This patch only allows introspection-related MSR write events to be sent out, improving performance. Should additional events be required, they can then simply be added to the list of vmx_introspection_force_enabled_msrs[]. Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com> --- xen/arch/x86/hvm/hvm.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)