Xen devel - Dec 2012 - [PATCH V2] mem_event: Add support for MEM_EVENT_REASON_MSR

Add the new MEM_EVENT_REASON_MSR event type. Works similarly
to the other register events, except event.gla always contains
the MSR type (in addition to event.gfn, which holds the value).

Signed-off-by: Razvan Cojocaru <rzvncj@gmail.com>
Acked-by: Tim Deegan <tim@xen.org>

diff -r b04de677de31 -r e33d3d37dfbf xen/arch/x86/hvm/hvm.c
--- a/xen/arch/x86/hvm/hvm.c	Tue Dec 18 18:16:52 2012 +0000
+++ b/xen/arch/x86/hvm/hvm.c	Thu Dec 20 14:52:52 2012 +0200
@@ -2927,6 +2927,8 @@ int hvm_msr_write_intercept(unsigned int
     hvm_cpuid(1, &cpuid[0], &cpuid[1], &cpuid[2], &cpuid[3]);
     mtrr = !!(cpuid[3] & cpufeat_mask(X86_FEATURE_MTRR));
 
+    hvm_memory_event_msr(msr, msr_content);
+
     switch ( msr )
     {
     case MSR_EFER:
@@ -3857,6 +3859,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
             case HVM_PARAM_MEMORY_EVENT_CR0:
             case HVM_PARAM_MEMORY_EVENT_CR3:
             case HVM_PARAM_MEMORY_EVENT_CR4:
+            case HVM_PARAM_MEMORY_EVENT_MSR:
                 if ( d == current->domain )
                     rc = -EPERM;
                 break;
@@ -4485,6 +4488,14 @@ void hvm_memory_event_cr4(unsigned long 
                            value, old, 0, 0);
 }
 
+void hvm_memory_event_msr(unsigned long msr, unsigned long value)
+{
+    hvm_memory_event_traps(current->domain->arch.hvm_domain
+                             .params[HVM_PARAM_MEMORY_EVENT_MSR],
+                           MEM_EVENT_REASON_MSR,
+                           value, ~value, 1, msr);
+}
+
 int hvm_memory_event_int3(unsigned long gla) 
 {
     uint32_t pfec = PFEC_page_present;
diff -r b04de677de31 -r e33d3d37dfbf xen/include/asm-x86/hvm/hvm.h
--- a/xen/include/asm-x86/hvm/hvm.h	Tue Dec 18 18:16:52 2012 +0000
+++ b/xen/include/asm-x86/hvm/hvm.h	Thu Dec 20 14:52:52 2012 +0200
@@ -448,6 +448,7 @@ int hvm_x2apic_msr_write(struct vcpu *v,
 void hvm_memory_event_cr0(unsigned long value, unsigned long old);
 void hvm_memory_event_cr3(unsigned long value, unsigned long old);
 void hvm_memory_event_cr4(unsigned long value, unsigned long old);
+void hvm_memory_event_msr(unsigned long msr, unsigned long value);
 /* Called for current VCPU on int3: returns -1 if no listener */
 int hvm_memory_event_int3(unsigned long gla);
 
diff -r b04de677de31 -r e33d3d37dfbf xen/include/public/hvm/params.h
--- a/xen/include/public/hvm/params.h	Tue Dec 18 18:16:52 2012 +0000
+++ b/xen/include/public/hvm/params.h	Thu Dec 20 14:52:52 2012 +0200
@@ -126,6 +126,7 @@
 #define HVM_PARAM_MEMORY_EVENT_CR4          22
 #define HVM_PARAM_MEMORY_EVENT_INT3         23
 #define HVM_PARAM_MEMORY_EVENT_SINGLE_STEP  25
+#define HVM_PARAM_MEMORY_EVENT_MSR          30
 
 #define HVMPME_MODE_MASK       (3 << 0)
 #define HVMPME_mode_disabled   0
@@ -141,6 +142,6 @@
 #define HVM_PARAM_ACCESS_RING_PFN   28
 #define HVM_PARAM_SHARING_RING_PFN  29
 
-#define HVM_NR_PARAMS          30
+#define HVM_NR_PARAMS          31
 
 #endif /* __XEN_PUBLIC_HVM_PARAMS_H__ */
diff -r b04de677de31 -r e33d3d37dfbf xen/include/public/mem_event.h
--- a/xen/include/public/mem_event.h	Tue Dec 18 18:16:52 2012 +0000
+++ b/xen/include/public/mem_event.h	Thu Dec 20 14:52:52 2012 +0200
@@ -45,6 +45,8 @@
 #define MEM_EVENT_REASON_CR4         4    /* CR4 was hit: gfn is CR4 value */
 #define MEM_EVENT_REASON_INT3        5    /* int3 was hit: gla/gfn are RIP */
 #define MEM_EVENT_REASON_SINGLESTEP  6    /* single step was invoked: gla/gfn
are RIP */
+#define MEM_EVENT_REASON_MSR         7    /* MSR was hit: gfn is MSR value, gla
is MSR type;
+                                             does NOT honour
HVMPME_onchangeonly */
 
 typedef struct mem_event_st {
     uint32_t flags;

On Thu, 2012-12-20 at 12:55 +0000, Razvan Cojocaru
wrote:
> 
> diff -r b04de677de31 -r e33d3d37dfbf xen/include/public/mem_event.h
> --- a/xen/include/public/mem_event.h    Tue Dec 18 18:16:52 2012 +0000
> +++ b/xen/include/public/mem_event.h    Thu Dec 20 14:52:52 2012 +0200
> @@ -45,6 +45,8 @@
>  #define MEM_EVENT_REASON_CR4         4    /* CR4 was hit: gfn is CR4 value
*/
>  #define MEM_EVENT_REASON_INT3        5    /* int3 was hit: gla/gfn are RIP
*/
>  #define MEM_EVENT_REASON_SINGLESTEP  6    /* single step was invoked:
gla/gfn are RIP */
> +#define MEM_EVENT_REASON_MSR         7    /* MSR was hit: gfn is MSR
value, gla is MSR type;
> +                                             does NOT honour
HVMPME_onchangeonly */
Why doesn''t/shouldn''t it support onchangeonly?

HVM_PARAM_MEMORY_EVENT_INT3 and HVM_PARAM_MEMORY_EVENT_SINGLE_STEP seem
not to support it either but they reject attempts to use them with it
(in do_hvm_op). I think this new value should do the same.

By "MSR type" do you mean the address?

Ian.

>> +#define MEM_EVENT_REASON_MSR         7    /* MSR was hit: gfn is MSR
value, gla is MSR type;
>> +                                             does NOT honour
HVMPME_onchangeonly */
> 
> Why doesn''t/shouldn''t it support onchangeonly?
It doesn''t support onchangeonly because it is unreasonably complicated
to get the old value (which we need to compare to the new one, and thus
establish if onchangeonly applies or not).

If you look at hvm_msr_write_intercept() (it''s at line 2917 in
xen/arch/x86/hvm/hvm.c), depending on the value of the "msr" parameter
there are several ways of setting the value: hvm_set_efer() if msr is
MSR_EFER, hvm_set_guest_tsc() if msr is MSR_IA32_TSC, and so on. So
rather than having a getter call for the current value for each of these
MSR types, I found it cleaner to just notify the event channel that a
value is being written, regardless of whether it''s different from the
currently stored value or not, and regardless of how the actual write is
being handle depending on the MSR type.
> HVM_PARAM_MEMORY_EVENT_INT3 and HVM_PARAM_MEMORY_EVENT_SINGLE_STEP seem
> not to support it either but they reject attempts to use them with it
> (in do_hvm_op). I think this new value should do the same.
Understood, I agree. Will add this new modification and submit V3 of the
patch tomorrow (no access to my development machine now).
> By "MSR type" do you mean the address?
I mean one of the following: MSR_EFER, MSR_IA32_TSC, etc.
Would you prefer that I change the "MSR type" expression in the
comment
to something else? What would make more sense?

Thanks,
Razvan Cojocaru

On Wed, 2013-01-02 at 13:56 +0000, Razvan Cojocaru
wrote:
> >> +#define MEM_EVENT_REASON_MSR         7    /* MSR was hit: gfn is
MSR value, gla is MSR type;
> >> +                                             does NOT honour
HVMPME_onchangeonly */
> > 
> > Why doesn''t/shouldn''t it support onchangeonly?
> 
> It doesn''t support onchangeonly because it is unreasonably
complicated
> to get the old value (which we need to compare to the new one, and thus
> establish if onchangeonly applies or not).
> 
> If you look at hvm_msr_write_intercept() (it''s at line 2917 in
> xen/arch/x86/hvm/hvm.c), depending on the value of the "msr"
parameter
> there are several ways of setting the value: hvm_set_efer() if msr is
> MSR_EFER, hvm_set_guest_tsc() if msr is MSR_IA32_TSC, and so on. So
> rather than having a getter call for the current value for each of these
> MSR types, I found it cleaner to just notify the event channel that a
> value is being written, regardless of whether it''s different from
the
> currently stored value or not, and regardless of how the actual write is
> being handle depending on the MSR type.
That seems reasonable, thanks. Might be nice to include some of this
description in the changelog.
> > HVM_PARAM_MEMORY_EVENT_INT3 and HVM_PARAM_MEMORY_EVENT_SINGLE_STEP
seem
> > not to support it either but they reject attempts to use them with it
> > (in do_hvm_op). I think this new value should do the same.
> 
> Understood, I agree. Will add this new modification and submit V3 of the
> patch tomorrow (no access to my development machine now).
Thanks.
> > By "MSR type" do you mean the address?
> 
> I mean one of the following: MSR_EFER, MSR_IA32_TSC, etc.
> Would you prefer that I change the "MSR type" expression in the
comment
> to something else? What would make more sense?
Those names are #defines for the MSR address (see
xen/include/asm-x86/msr-index.h).

"MSR address" or "MSR number" would be better than "MSR
type" (which
implies to me membership of some sort of class of MSRs)`

Ian.

> That seems reasonable, thanks. Might be nice to include some of this
> description in the changelog.
No problem. I''ll add that information to the changelog before
re-submitting the patch.
> Those names are #defines for the MSR address (see
> xen/include/asm-x86/msr-index.h).
> 
> "MSR address" or "MSR number" would be better than
"MSR type" (which
> implies to me membership of some sort of class of MSRs)`
I''ll change "MSR type" to "MSR address".

Thank you for your comments,
Razvan Cojocaru