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

[Steven Maresca <steve@xxxxxxxxxxxx>]

On Tue, Sep 11, 2012 at 3:22 AM, Tim Deegan <tim@xxxxxxx> wrote:
> At 23:56 -0400 on 10 Sep (1347321409), Steven Maresca wrote:
>> On Mon, Sep 10, 2012 at 9:49 PM, Aravindh Puthiyaparambil
>> <aravindh@xxxxxxxxxxxx> wrote:
>> > On Mon, Sep 10, 2012 at 3:06 PM, Steven Maresca <steve@xxxxxxxxxxxx> wrote:
>> >> On Mon, Sep 10, 2012 at 5:39 PM, Keir Fraser <keir@xxxxxxx> wrote:
>> >>> On 10/09/2012 22:20, "Steven Maresca" <steve@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
>> >
>> > 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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel