Re: [PATCH] vm_event: Record FS_BASE/GS_BASE during events

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 02/11/2016 11:12 PM, Tamas K Lengyel wrote:
> 
> 
> On Thu, Feb 11, 2016 at 2:11 PM, Tamas K Lengyel
> <tamas.k.lengyel@gmail.com <mailto:tamas.k.lengyel@gmail.com>> wrote:
> 
> 
> 
>     On Thu, Feb 11, 2016 at 1:59 PM, Razvan Cojocaru
>     <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> wrote:
> 
>         On 02/11/2016 10:38 PM, Tamas K Lengyel wrote:
>         >
>         >
>         > On Thu, Feb 11, 2016 at 1:13 PM, Razvan Cojocaru
>         > <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>
>         <mailto:rcojocaru@bitdefender.com
>         <mailto:rcojocaru@bitdefender.com>>> wrote:
>         >
>         >     On 02/11/2016 10:04 PM, Andrew Cooper wrote:
>         >     > On 11/02/16 20:00, Razvan Cojocaru wrote:
>         >     >> On 02/11/2016 09:55 PM, Andrew Cooper wrote:
>         >     >>> On 11/02/16 19:54, Razvan Cojocaru wrote:
>         >     >>>> On 02/11/2016 09:51 PM, Tamas K Lengyel wrote:
>         >     >>>>> While the public vm_event header specifies fs_base/gs_base as
>         >     registers that
>         >     >>>>> should be recorded for each event, that hasn't actually been
>         >     the case. In
>         >     >>>>> this patch we remedy the issue.
>         >     >>>>>
>         >     >>>>> Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com <mailto:tlengyel@novetta.com>
>         >     <mailto:tlengyel@novetta.com <mailto:tlengyel@novetta.com>>>
>         >     >>>>> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com
>         <mailto:rcojocaru@bitdefender.com>
>         >     <mailto:rcojocaru@bitdefender.com
>         <mailto:rcojocaru@bitdefender.com>>>
>         >     >>>>> Cc: Keir Fraser <keir@xen.org <mailto:keir@xen.org>
>         <mailto:keir@xen.org <mailto:keir@xen.org>>>
>         >     >>>>> Cc: Jan Beulich <jbeulich@suse.com
>         <mailto:jbeulich@suse.com> <mailto:jbeulich@suse.com
>         <mailto:jbeulich@suse.com>>>
>         >     >>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com
>         <mailto:andrew.cooper3@citrix.com>
>         >     <mailto:andrew.cooper3@citrix.com
>         <mailto:andrew.cooper3@citrix.com>>>
>         >     >>>>> ---
>         >     >>>>>  xen/arch/x86/hvm/event.c | 9 ++++++++-
>         >     >>>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>         >     >>>> Fair enough.
>         >     >>>>
>         >     >>>> Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>
>         >     <mailto:rcojocaru@bitdefender.com
>         <mailto:rcojocaru@bitdefender.com>>>
>         >     >>> Oops.
>         >     >>>
>         >     >>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com <mailto:andrew.cooper3@citrix.com>
>         >     <mailto:andrew.cooper3@citrix.com
>         <mailto:andrew.cooper3@citrix.com>>>
>         >     >> This has actually been intentional, in that we've only needed those
>         >     >> fields for EPT events, and thought that not filling what's not needed
>         >     >> until it's needed would save a tiny bit of hypervisor processing
>         >     time.
>         >     >> They are being filled in only for page fault events at the moment.
>         >     >>
>         >     >> I believe it's been discussed at the time. We still don't need those
>         >     >> coming with the events that use hvm_event_fill_regs(), but if Tamas
>         >     >> needs them then by all means.
>         >     >
>         >     > The public header file does suggest that all of vm_event_regs_x86 will
>         >     > be complete.  Are there any other fields currently missing?
>         >
>         >     There are. p2m_vm_event_fill_regs() fills everything in (in
>         >     xen/arch/x86/mm/p2m.c). hvm_event_fill_regs() still does not, even after
>         >     Tamas' patch.
>         >
>         >
>         > Ah, that makes sense. Yea, I would prefer if all registers would get
>         > filled in for all events so I'll just consolidate these two functions
>         > into one.
> 
>         Right, but please be careful and test that you get correct
>         values with
>         all events (page fault events + the others), I remember that for
>         some
>         reason I needed to use different ways to get at the same values in
>         p2m_vm_event_fill_regs() and hvm_event_fill_regs().
> 
>         For example, p2m_vm_event_fill_regs() does:
> 
>         hvm_funcs.save_cpu_ctxt(curr, &ctxt);
>         req->data.regs.x86.cr0 = ctxt.cr0;
> 
>         and hvm_event_fill_regs() does:
> 
>         req->data.regs.x86.cr0 = curr->arch.hvm_vcpu.guest_cr[0];
> 
>         I don't remember exactly why I had to do that at the time, but I do
>         recall it being necessary.
> 
> 
>     That sounds odd to me. As far as I can tell everything works just
>     right with the other patch I just sent. I looked into what
>     hvm_funcs.save_cpu_ctxt does on Intel and it calls
>     vmx_save_vmcs_ctxt which calls vmx_vmcs_save. That has:
> 
> 
> (continued)
> 
>     c->cr0 = v->arch.hvm_vcpu.guest_cr[0];
>     c->cr2 = v->arch.hvm_vcpu.guest_cr[2];
>     c->cr3 = v->arch.hvm_vcpu.guest_cr[3];
>     c->cr4 = v->arch.hvm_vcpu.guest_cr[4];
> 
> So there shouldn't really be any difference here.

Fair enough, it's possible that the code was different when I first
wrote that (roughly 2012) so there was something else going on. Thanks
for checking!


Cheers,
Razvan