On Thu, Feb 11, 2016 at 2:11 PM, Tamas K Lengyel <tamas.k.leng...@gmail.com> wrote:
> > > On Thu, Feb 11, 2016 at 1:59 PM, Razvan Cojocaru < > rcojoc...@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 >> > <rcojoc...@bitdefender.com <mailto:rcojoc...@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 <tleng...@novetta.com >> > <mailto:tleng...@novetta.com>> >> > >>>>> Cc: Razvan Cojocaru <rcojoc...@bitdefender.com >> > <mailto:rcojoc...@bitdefender.com>> >> > >>>>> Cc: Keir Fraser <k...@xen.org <mailto:k...@xen.org>> >> > >>>>> Cc: Jan Beulich <jbeul...@suse.com <mailto:jbeul...@suse.com >> >> >> > >>>>> Cc: Andrew Cooper <andrew.coop...@citrix.com >> > <mailto:andrew.coop...@citrix.com>> >> > >>>>> --- >> > >>>>> xen/arch/x86/hvm/event.c | 9 ++++++++- >> > >>>>> 1 file changed, 8 insertions(+), 1 deletion(-) >> > >>>> Fair enough. >> > >>>> >> > >>>> Acked-by: Razvan Cojocaru <rcojoc...@bitdefender.com >> > <mailto:rcojoc...@bitdefender.com>> >> > >>> Oops. >> > >>> >> > >>> Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com >> > <mailto:andrew.coop...@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. Tamas
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
