Xen devel - Sep 2012 - [PATCH] mem_event: fix regression affecting CR3, CR4 memory events

This is a patch repairing a regression in code previously functional in 4.1.x.
It appears that, during some refactoring work, calls to hvm_memory_event_cr3 and
hvm_memory_event_cr4 were lost.

These functions were originally called in mov_to_cr() of vmx.c, but the commit 
http://xenbits.xen.org/hg/xen-unstable.hg/rev/1276926e3795 abstracted the
original code into generic functions up a level in hvm.c, dropping these calls
in the process.

Signed-off-by: Steven Maresca <steve@zentific.com>

diff -r a64f4e107951 -r 4d31c1c86418 xen/arch/x86/hvm/hvm.c
--- a/xen/arch/x86/hvm/hvm.c	Fri Sep 07 11:09:46 2012 +0100
+++ b/xen/arch/x86/hvm/hvm.c	Mon Sep 10 16:48:15 2012 -0400
@@ -1758,6 +1758,7 @@ int hvm_set_cr3(unsigned long value)
 {
     struct vcpu *v = current;
     struct page_info *page;
+    unsigned long old;
 
     if ( hvm_paging_enabled(v) && !paging_mode_hap(v->domain)
&&
          (value != v->arch.hvm_vcpu.guest_cr[3]) )
@@ -1775,8 +1776,10 @@ int hvm_set_cr3(unsigned long value)
         HVM_DBG_LOG(DBG_LEVEL_VMMU, "Update CR3 value = %lx", value);
     }
 
+    old=v->arch.hvm_vcpu.guest_cr[3];
     v->arch.hvm_vcpu.guest_cr[3] = value;
     paging_update_cr3(v);
+    hvm_memory_event_cr3(value, old);
     return X86EMUL_OKAY;
 
  bad_cr3:
@@ -1818,6 +1821,7 @@ int hvm_set_cr4(unsigned long value)
 
     v->arch.hvm_vcpu.guest_cr[4] = value;
     hvm_update_guest_cr(v, 4);
+    hvm_memory_event_cr4(value, old_cr);
 
     /*
      * Modifying CR4.{PSE,PAE,PGE,SMEP}, or clearing CR4.PCIDE

On Mon, Sep 10, 2012 at 5:03 PM, Steven Maresca <steve@zentific.com>
wrote:
> This is a patch repairing a regression in code previously functional in
4.1.x. It appears that, during some refactoring work, calls to
hvm_memory_event_cr3 and hvm_memory_event_cr4 were lost.
>
> These functions were originally called in mov_to_cr() of vmx.c, but the
commit  http://xenbits.xen.org/hg/xen-unstable.hg/rev/1276926e3795 abstracted
the original code into generic functions up a level in hvm.c, dropping these
calls in the process.
>
> Signed-off-by: Steven Maresca <steve@zentific.com>
>
> diff -r a64f4e107951 -r 4d31c1c86418 xen/arch/x86/hvm/hvm.c
> --- a/xen/arch/x86/hvm/hvm.c    Fri Sep 07 11:09:46 2012 +0100
> +++ b/xen/arch/x86/hvm/hvm.c    Mon Sep 10 16:48:15 2012 -0400
> @@ -1758,6 +1758,7 @@ int hvm_set_cr3(unsigned long value)
>  {
>      struct vcpu *v = current;
>      struct page_info *page;
> +    unsigned long old;
>
>      if ( hvm_paging_enabled(v) && !paging_mode_hap(v->domain)
&&
>           (value != v->arch.hvm_vcpu.guest_cr[3]) )
> @@ -1775,8 +1776,10 @@ int hvm_set_cr3(unsigned long value)
>          HVM_DBG_LOG(DBG_LEVEL_VMMU, "Update CR3 value = %lx",
value);
>      }
>
> +    old=v->arch.hvm_vcpu.guest_cr[3];
>      v->arch.hvm_vcpu.guest_cr[3] = value;
>      paging_update_cr3(v);
> +    hvm_memory_event_cr3(value, old);
>      return X86EMUL_OKAY;
>
>   bad_cr3:
> @@ -1818,6 +1821,7 @@ int hvm_set_cr4(unsigned long value)
>
>      v->arch.hvm_vcpu.guest_cr[4] = value;
>      hvm_update_guest_cr(v, 4);
> +    hvm_memory_event_cr4(value, old_cr);
>
>      /*
>       * Modifying CR4.{PSE,PAE,PGE,SMEP}, or clearing CR4.PCIDE
Given the refactoring in the commit related to the regression
http://xenbits.xen.org/hg/xen-unstable.hg/rev/1276926e3795, it seemed
(to me anyway) that inserting calls as shown in the patch would be
cleaner, but I can definitely come up with some drawbacks.  However, I
wanted to get this fixed for 4.2 if at all possible, so I wanted to
send regardless.

In terms of drawbacks, this will require some ifdefs for x86_64, for example.

Any suggestions for the cleanest means of achieving the same in vmx.c?

Steve

On 10/09/2012 22:20, "Steven Maresca" <steve@zentific.com>
wrote:
> Given the refactoring in the commit related to the regression
> http://xenbits.xen.org/hg/xen-unstable.hg/rev/1276926e3795, it seemed
> (to me anyway) that inserting calls as shown in the patch would be
> cleaner, but I can definitely come up with some drawbacks.  However, I
> wanted to get this fixed for 4.2 if at all possible, so I wanted to
> send regardless.
> 
> In terms of drawbacks, this will require some ifdefs for x86_64, for
example.
> 
> Any suggestions for the cleanest means of achieving the same in vmx.c?
I don''t understand this at all. The original commit moved some CR
handling
to common HVM code. Your patch adds some mem_event calls into that common
HVM code. What would need to be "achieved" in vmx.c? What ifdefs would
be
required for x86_64?

 -- Keir

On Mon, Sep 10, 2012 at 5:39 PM, Keir Fraser <keir@xen.org>
wrote:
> On 10/09/2012 22:20, "Steven Maresca" <steve@zentific.com>
wrote:
>
>> Given the refactoring in the commit related to the regression
>> http://xenbits.xen.org/hg/xen-unstable.hg/rev/1276926e3795, it seemed
>> (to me anyway) that inserting calls as shown in the patch would be
>> cleaner, but I can definitely come up with some drawbacks.  However, I
>> wanted to get this fixed for 4.2 if at all possible, so I wanted to
>> send regardless.
>>
>> In terms of drawbacks, this will require some ifdefs for x86_64, for
example.
>>
>> Any suggestions for the cleanest means of achieving the same in vmx.c?
>
> I don''t understand this at all. The original commit moved some CR
handling
> to common HVM code. Your patch adds some mem_event calls into that common
> HVM code. What would need to be "achieved" in vmx.c? What ifdefs
would be
> required for x86_64?
>
>  -- Keir
>
>
Keir,

In the process of moving CR handling to common code, that commit
removed calls to hvm_memory_event_cr3() and hvm_memory_event_cr4().
Without some patch restoring the calls to those two functions, current
xen-unstable and xen-4.2-testing only reference them as function
prototypes and function definitions -- there are no calls whatsoever.

I only mentioned an ifdef relative to x86_64 because of the #ifdef
__x86_64__  around the function definitions. ( I know in the hvm.h
itself they''re just empty inline functions if not __x86_64__. )

Regarding vmx.c: hvm_memory_event_cr0 is called within vmx_cr_access.
The patch I sent restoring the calls to hvm_memory_event_cr4 and
hvm_memory_event_cr3 could be treated similarly, that''s all.

Steve

On Sep 10, 2012, at 6:06 PM, Steven Maresca wrote:
> On Mon, Sep 10, 2012 at 5:39 PM, Keir Fraser <keir@xen.org> wrote:
>> On 10/09/2012 22:20, "Steven Maresca"
<steve@zentific.com> wrote:
>> 
>>> Given the refactoring in the commit related to the regression
>>> http://xenbits.xen.org/hg/xen-unstable.hg/rev/1276926e3795, it
seemed
>>> (to me anyway) that inserting calls as shown in the patch would be
>>> cleaner, but I can definitely come up with some drawbacks. 
However, I
>>> wanted to get this fixed for 4.2 if at all possible, so I wanted to
>>> send regardless.
>>> 
>>> In terms of drawbacks, this will require some ifdefs for x86_64,
for example.
>>> 
>>> Any suggestions for the cleanest means of achieving the same in
vmx.c?
>> 
>> I don''t understand this at all. The original commit moved some
CR handling
>> to common HVM code. Your patch adds some mem_event calls into that
common
>> HVM code. What would need to be "achieved" in vmx.c? What
ifdefs would be
>> required for x86_64?
>> 
>> -- Keir
>> 
>> 
> 
> Keir,
> 
> In the process of moving CR handling to common code, that commit
> removed calls to hvm_memory_event_cr3() and hvm_memory_event_cr4().
> Without some patch restoring the calls to those two functions, current
> xen-unstable and xen-4.2-testing only reference them as function
> prototypes and function definitions -- there are no calls whatsoever.
> 
> I only mentioned an ifdef relative to x86_64 because of the #ifdef
> __x86_64__  around the function definitions. ( I know in the hvm.h
> itself they''re just empty inline functions if not __x86_64__. )
> 
> Regarding vmx.c: hvm_memory_event_cr0 is called within vmx_cr_access.
> The patch I sent restoring the calls to hvm_memory_event_cr4 and
> hvm_memory_event_cr3 could be treated similarly, that''s all.
The patch looks ok to me.
Acked-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

You don''t need to worry about x86_32. Is on its way out. You
don''t need to push this down into vmx either. Once AMD gains support
for mem-event, this will just work for svm as well.

Like Keir says, no need for us. Your patch is good as is. Best to cc Tim Deegan
<tim@xen.org>, the mm maintainer.

Cheers
Andres
> 
> Steve

On Mon, Sep 10, 2012 at 3:06 PM, Steven Maresca <steve@zentific.com>
wrote:
> On Mon, Sep 10, 2012 at 5:39 PM, Keir Fraser <keir@xen.org> wrote:
>> On 10/09/2012 22:20, "Steven Maresca"
<steve@zentific.com> wrote:
>>
>>> Given the refactoring in the commit related to the regression
>>> http://xenbits.xen.org/hg/xen-unstable.hg/rev/1276926e3795, it
seemed
>>> (to me anyway) that inserting calls as shown in the patch would be
>>> cleaner, but I can definitely come up with some drawbacks. 
However, I
>>> wanted to get this fixed for 4.2 if at all possible, so I wanted to
>>> send regardless.
>>>
>>> In terms of drawbacks, this will require some ifdefs for x86_64,
for example.
>>>
>>> Any suggestions for the cleanest means of achieving the same in
vmx.c?
>>
>> I don''t understand this at all. The original commit moved some
CR handling
>> to common HVM code. Your patch adds some mem_event calls into that
common
>> HVM code. What would need to be "achieved" in vmx.c? What
ifdefs would be
>> required for x86_64?
>>
>>  -- Keir
>>
>>
>
> Keir,
>
> In the process of moving CR handling to common code, that commit
> removed calls to hvm_memory_event_cr3() and hvm_memory_event_cr4().
> Without some patch restoring the calls to those two functions, current
> xen-unstable and xen-4.2-testing only reference them as function
> prototypes and function definitions -- there are no calls whatsoever.
>
> I only mentioned an ifdef relative to x86_64 because of the #ifdef
> __x86_64__  around the function definitions. ( I know in the hvm.h
> itself they''re just empty inline functions if not __x86_64__. )
>
> Regarding vmx.c: hvm_memory_event_cr0 is called within vmx_cr_access.
> The patch I sent restoring the calls to hvm_memory_event_cr4 and
> hvm_memory_event_cr3 could be treated similarly, that''s all.
Looks good to me.
Acked-by: Aravindh Puthiyaparambil <aravindh@virtuata.com>

Cheers,
Aravindh

On Mon, Sep 10, 2012 at 9:49 PM, Aravindh Puthiyaparambil
<aravindh@virtuata.com> wrote:
> On Mon, Sep 10, 2012 at 3:06 PM, Steven Maresca <steve@zentific.com>
wrote:
>> On Mon, Sep 10, 2012 at 5:39 PM, Keir Fraser <keir@xen.org>
wrote:
>>> On 10/09/2012 22:20, "Steven Maresca"
<steve@zentific.com> wrote:
>>>
>>>> Given the refactoring in the commit related to the regression
>>>> http://xenbits.xen.org/hg/xen-unstable.hg/rev/1276926e3795, it
seemed
>>>> (to me anyway) that inserting calls as shown in the patch would
be
>>>> cleaner, but I can definitely come up with some drawbacks. 
However, I
>>>> wanted to get this fixed for 4.2 if at all possible, so I
wanted to
>>>> send regardless.
>>>>
>>>> In terms of drawbacks, this will require some ifdefs for
x86_64, for example.
>>>>
>>>> Any suggestions for the cleanest means of achieving the same in
vmx.c?
>>>
>>> I don''t understand this at all. The original commit moved
some CR handling
>>> to common HVM code. Your patch adds some mem_event calls into that
common
>>> HVM code. What would need to be "achieved" in vmx.c? What
ifdefs would be
>>> required for x86_64?
>>>
>>>  -- Keir
>>>
>>>
>>
>> Keir,
>>
>> In the process of moving CR handling to common code, that commit
>> removed calls to hvm_memory_event_cr3() and hvm_memory_event_cr4().
>> Without some patch restoring the calls to those two functions, current
>> xen-unstable and xen-4.2-testing only reference them as function
>> prototypes and function definitions -- there are no calls whatsoever.
>>
>> I only mentioned an ifdef relative to x86_64 because of the #ifdef
>> __x86_64__  around the function definitions. ( I know in the hvm.h
>> itself they''re just empty inline functions if not __x86_64__.
)
>>
>> Regarding vmx.c: hvm_memory_event_cr0 is called within vmx_cr_access.
>> The patch I sent restoring the calls to hvm_memory_event_cr4 and
>> hvm_memory_event_cr3 could be treated similarly, that''s all.
>
> Looks good to me.
> Acked-by: Aravindh Puthiyaparambil <aravindh@virtuata.com>
>
> Cheers,
> Aravindh
Adding Tim Deegan as suggested.

Tim, this is a regression fix for CR3/CR4 memory events
unintentionally removed during some refactoring of HVM CR handling.
If at all possible, for 4.2 inclusion before we officially leave RC.
Acks received from Andres and Aravindh. Thanks to all for the quick
review.

Take care,
Steve

At 23:56 -0400 on 10 Sep (1347321409), Steven Maresca
wrote:
> On Mon, Sep 10, 2012 at 9:49 PM, Aravindh Puthiyaparambil
> <aravindh@virtuata.com> wrote:
> > On Mon, Sep 10, 2012 at 3:06 PM, Steven Maresca
<steve@zentific.com> wrote:
> >> On Mon, Sep 10, 2012 at 5:39 PM, Keir Fraser <keir@xen.org>
wrote:
> >>> On 10/09/2012 22:20, "Steven Maresca"
<steve@zentific.com> wrote:
> >>>
> >>>> Given the refactoring in the commit related to the
regression
> >>>>
http://xenbits.xen.org/hg/xen-unstable.hg/rev/1276926e3795, it seemed
> >>>> (to me anyway) that inserting calls as shown in the patch
would be
> >>>> cleaner, but I can definitely come up with some drawbacks.
However, I
> >>>> wanted to get this fixed for 4.2 if at all possible, so I
wanted to
> >>>> send regardless.
> >>>>
> >>>> In terms of drawbacks, this will require some ifdefs for
x86_64, for example.
> >>>>
> >>>> Any suggestions for the cleanest means of achieving the
same in vmx.c?
> >>>
> >>> I don''t understand this at all. The original commit
moved some CR handling
> >>> to common HVM code. Your patch adds some mem_event calls into
that common
> >>> HVM code. What would need to be "achieved" in vmx.c?
What ifdefs would be
> >>> required for x86_64?
> >>>
> >>>  -- Keir
> >>>
> >>>
> >>
> >> Keir,
> >>
> >> In the process of moving CR handling to common code, that commit
> >> removed calls to hvm_memory_event_cr3() and
hvm_memory_event_cr4().
> >> Without some patch restoring the calls to those two functions,
current
> >> xen-unstable and xen-4.2-testing only reference them as function
> >> prototypes and function definitions -- there are no calls
whatsoever.
> >>
> >> I only mentioned an ifdef relative to x86_64 because of the #ifdef
> >> __x86_64__  around the function definitions. ( I know in the hvm.h
> >> itself they''re just empty inline functions if not
__x86_64__. )
> >>
> >> Regarding vmx.c: hvm_memory_event_cr0 is called within
vmx_cr_access.
> >> The patch I sent restoring the calls to hvm_memory_event_cr4 and
> >> hvm_memory_event_cr3 could be treated similarly, that''s
all.
> >
> > Looks good to me.
> > Acked-by: Aravindh Puthiyaparambil <aravindh@virtuata.com>
> >
> > Cheers,
> > Aravindh
> 
> Adding Tim Deegan as suggested.
> 
> Tim, this is a regression fix for CR3/CR4 memory events
> unintentionally removed during some refactoring of HVM CR handling.
> If at all possible, for 4.2 inclusion before we officially leave RC.
I think it''s too late to get any more code changes in to 4.2.0, so this
will have to wait for 4.2.1.  Ian?

The patch looks OK, but why didn''t you put the memory_event calls back
in mov_to_cr(), which is where they were before that?

Cheers,

Tim.

On Tue, 2012-09-11 at 08:22 +0100, Tim Deegan wrote:
> I think it''s too late to get any more code changes in to 4.2.0, so
this
> will have to wait for 4.2.1.  Ian?
I''d defer to you h/v guys but IMHO only fixes for super critical issues
should be going in for 4.2.0 now.

4.2.1 is not that far away and these fixes will be in 4.2-testing
between 4.2.0 and then.
> 
> The patch looks OK, but why didn''t you put the memory_event calls
back
> in mov_to_cr(), which is where they were before that?
> 
> Cheers,
> 
> Tim.

On Tue, Sep 11, 2012 at 3:22 AM, Tim Deegan <tim@xen.org>
wrote:
> At 23:56 -0400 on 10 Sep (1347321409), Steven Maresca wrote:
>> On Mon, Sep 10, 2012 at 9:49 PM, Aravindh Puthiyaparambil
>> <aravindh@virtuata.com> wrote:
>> > On Mon, Sep 10, 2012 at 3:06 PM, Steven Maresca
<steve@zentific.com> wrote:
>> >> On Mon, Sep 10, 2012 at 5:39 PM, Keir Fraser
<keir@xen.org> wrote:
>> >>> On 10/09/2012 22:20, "Steven Maresca"
<steve@zentific.com> wrote:
>> >>>
>> >>>> Given the refactoring in the commit related to the
regression
>> >>>>
http://xenbits.xen.org/hg/xen-unstable.hg/rev/1276926e3795, it seemed
>> >>>> (to me anyway) that inserting calls as shown in the
patch would be
>> >>>> cleaner, but I can definitely come up with some
drawbacks.  However, I
>> >>>> wanted to get this fixed for 4.2 if at all possible,
so I wanted to
>> >>>> send regardless.
>> >>>>
>> >>>> In terms of drawbacks, this will require some ifdefs
for x86_64, for example.
>> >>>>
>> >>>> Any suggestions for the cleanest means of achieving
the same in vmx.c?
>> >>>
>> >>> I don''t understand this at all. The original
commit moved some CR handling
>> >>> to common HVM code. Your patch adds some mem_event calls
into that common
>> >>> HVM code. What would need to be "achieved" in
vmx.c? What ifdefs would be
>> >>> required for x86_64?
>> >>>
>> >>>  -- Keir
>> >>>
>> >>>
>> >>
>> >> Keir,
>> >>
>> >> In the process of moving CR handling to common code, that
commit
>> >> removed calls to hvm_memory_event_cr3() and
hvm_memory_event_cr4().
>> >> Without some patch restoring the calls to those two functions,
current
>> >> xen-unstable and xen-4.2-testing only reference them as
function
>> >> prototypes and function definitions -- there are no calls
whatsoever.
>> >>
>> >> I only mentioned an ifdef relative to x86_64 because of the
#ifdef
>> >> __x86_64__  around the function definitions. ( I know in the
hvm.h
>> >> itself they''re just empty inline functions if not
__x86_64__. )
>> >>
>> >> Regarding vmx.c: hvm_memory_event_cr0 is called within
vmx_cr_access.
>> >> The patch I sent restoring the calls to hvm_memory_event_cr4
and
>> >> hvm_memory_event_cr3 could be treated similarly,
that''s all.
>> >
>> > Looks good to me.
>> > Acked-by: Aravindh Puthiyaparambil <aravindh@virtuata.com>
>> >
>> > Cheers,
>> > Aravindh
>>
>> Adding Tim Deegan as suggested.
>>
>> Tim, this is a regression fix for CR3/CR4 memory events
>> unintentionally removed during some refactoring of HVM CR handling.
>> If at all possible, for 4.2 inclusion before we officially leave RC.
>
> I think it''s too late to get any more code changes in to 4.2.0, so
this
> will have to wait for 4.2.1.  Ian?
>
> The patch looks OK, but why didn''t you put the memory_event calls
back
> in mov_to_cr(), which is where they were before that?
>
> Cheers,
>
> Tim.
Tim,

This bugfix could definitely be placed in hvm_mov_to_cr() - it just
seemed more localized if placed in the hvm_set_cr*() functions. I''ll
happily defer to preference, if you have one.

Thanks,
Steve

At 21:21 -0400 on 10 Sep (1347312087), Andres Lagar-Cavilla
wrote:
> 
> On Sep 10, 2012, at 6:06 PM, Steven Maresca wrote:
> 
> > On Mon, Sep 10, 2012 at 5:39 PM, Keir Fraser <keir@xen.org>
wrote:
> >> On 10/09/2012 22:20, "Steven Maresca"
<steve@zentific.com> wrote:
> >> 
> >>> Given the refactoring in the commit related to the regression
> >>> http://xenbits.xen.org/hg/xen-unstable.hg/rev/1276926e3795, it
seemed
> >>> (to me anyway) that inserting calls as shown in the patch
would be
> >>> cleaner, but I can definitely come up with some drawbacks. 
However, I
> >>> wanted to get this fixed for 4.2 if at all possible, so I
wanted to
> >>> send regardless.
> >>> 
> >>> In terms of drawbacks, this will require some ifdefs for
x86_64, for example.
> >>> 
> >>> Any suggestions for the cleanest means of achieving the same
in vmx.c?
> >> 
> >> I don''t understand this at all. The original commit moved
some CR handling
> >> to common HVM code. Your patch adds some mem_event calls into that
common
> >> HVM code. What would need to be "achieved" in vmx.c?
What ifdefs would be
> >> required for x86_64?
> >> 
> >> -- Keir
> >> 
> >> 
> > 
> > Keir,
> > 
> > In the process of moving CR handling to common code, that commit
> > removed calls to hvm_memory_event_cr3() and hvm_memory_event_cr4().
> > Without some patch restoring the calls to those two functions, current
> > xen-unstable and xen-4.2-testing only reference them as function
> > prototypes and function definitions -- there are no calls whatsoever.
> > 
> > I only mentioned an ifdef relative to x86_64 because of the #ifdef
> > __x86_64__  around the function definitions. ( I know in the hvm.h
> > itself they''re just empty inline functions if not __x86_64__.
)
> > 
> > Regarding vmx.c: hvm_memory_event_cr0 is called within vmx_cr_access.
> > The patch I sent restoring the calls to hvm_memory_event_cr4 and
> > hvm_memory_event_cr3 could be treated similarly, that''s all.
> 
> The patch looks ok to me.
> Acked-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> 
Acked-by: Tim Deegan <tim@xen.org>

Tim.