From mboxrd@z Thu Jan 1 00:00:00 1970 From: Razvan Cojocaru Subject: Re: [PATCH] vm_event: Record FS_BASE/GS_BASE during events Date: Thu, 11 Feb 2016 23:34:20 +0200 Message-ID: <56BCFE5C.4080409@bitdefender.com> References: <1455220260-5987-1-git-send-email-tlengyel@novetta.com> <56BCE6EA.5070501@bitdefender.com> <56BCE71B.1090003@citrix.com> <56BCE85B.2060005@bitdefender.com> <56BCE969.9010307@citrix.com> <56BCEB85.6000802@bitdefender.com> <56BCF616.80309@bitdefender.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1aTysb-0008NJ-7q for xen-devel@lists.xenproject.org; Thu, 11 Feb 2016 21:34:25 +0000 Received: from smtp02.buh.bitdefender.net (unknown [10.17.80.76]) by mx-sr.buh.bitdefender.com (Postfix) with ESMTP id 21A567FBDC for ; Thu, 11 Feb 2016 23:34:22 +0200 (EET) In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tamas K Lengyel Cc: Tamas K Lengyel , Andrew Cooper , Keir Fraser , Jan Beulich , Xen-devel List-Id: xen-devel@lists.xenproject.org On 02/11/2016 11:12 PM, Tamas K Lengyel wrote: > > > On Thu, Feb 11, 2016 at 2:11 PM, Tamas K Lengyel > > wrote: > > > > On Thu, Feb 11, 2016 at 1:59 PM, Razvan Cojocaru > > wrote: > > On 02/11/2016 10:38 PM, Tamas K Lengyel wrote: > > > > > > On Thu, Feb 11, 2016 at 1:13 PM, Razvan Cojocaru > > > >> 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 > > >> > > >>>>> Cc: Razvan Cojocaru > > >> > > >>>>> Cc: Keir Fraser > >> > > >>>>> Cc: Jan Beulich >> > > >>>>> Cc: Andrew Cooper > > >> > > >>>>> --- > > >>>>> xen/arch/x86/hvm/event.c | 9 ++++++++- > > >>>>> 1 file changed, 8 insertions(+), 1 deletion(-) > > >>>> Fair enough. > > >>>> > > >>>> Acked-by: Razvan Cojocaru > > >> > > >>> Oops. > > >>> > > >>> Reviewed-by: Andrew Cooper > > >> > > >> 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