| Message ID | 1455220260-5987-1-git-send-email-tlengyel@novetta.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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> > Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> > Cc: Keir Fraser <keir@xen.org> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <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> Thanks, Razvan
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> >> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> >> Cc: Keir Fraser <keir@xen.org> >> Cc: Jan Beulich <jbeulich@suse.com> >> Cc: Andrew Cooper <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> Oops. Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
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> >>> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> >>> Cc: Keir Fraser <keir@xen.org> >>> Cc: Jan Beulich <jbeulich@suse.com> >>> Cc: Andrew Cooper <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> > > Oops. > > Reviewed-by: Andrew Cooper <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. Thanks, Razvan
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> >>>> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> >>>> Cc: Keir Fraser <keir@xen.org> >>>> Cc: Jan Beulich <jbeulich@suse.com> >>>> Cc: Andrew Cooper <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> >> Oops. >> >> Reviewed-by: Andrew Cooper <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? Given the overhead of the vmexit and communication on the vm_event channel, a few extra cycles reading state is lost in the overhead. ~Andrew
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> >>>>> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> >>>>> Cc: Keir Fraser <keir@xen.org> >>>>> Cc: Jan Beulich <jbeulich@suse.com> >>>>> Cc: Andrew Cooper <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> >>> Oops. >>> >>> Reviewed-by: Andrew Cooper <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. Thanks, Razvan
On Thu, Feb 11, 2016 at 1:13 PM, Razvan Cojocaru <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> > >>>>> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> > >>>>> Cc: Keir Fraser <keir@xen.org> > >>>>> Cc: Jan Beulich <jbeulich@suse.com> > >>>>> Cc: Andrew Cooper <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> > >>> Oops. > >>> > >>> Reviewed-by: Andrew Cooper <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. Tamas
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>> 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>> > >>>>> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com > <mailto:rcojocaru@bitdefender.com>> > >>>>> Cc: Keir Fraser <keir@xen.org <mailto:keir@xen.org>> > >>>>> Cc: Jan Beulich <jbeulich@suse.com <mailto:jbeulich@suse.com>> > >>>>> Cc: Andrew Cooper <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>> > >>> Oops. > >>> > >>> Reviewed-by: Andrew Cooper <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. Thanks, Razvan
On Thu, Feb 11, 2016 at 1:59 PM, Razvan Cojocaru <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>> 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>> > > >>>>> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com > > <mailto:rcojocaru@bitdefender.com>> > > >>>>> Cc: Keir Fraser <keir@xen.org <mailto:keir@xen.org>> > > >>>>> Cc: Jan Beulich <jbeulich@suse.com <mailto:jbeulich@suse.com>> > > >>>>> Cc: Andrew Cooper <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>> > > >>> Oops. > > >>> > > >>> Reviewed-by: Andrew Cooper <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:
On Thu, Feb 11, 2016 at 2:11 PM, Tamas K Lengyel <tamas.k.lengyel@gmail.com> wrote: > > > On Thu, Feb 11, 2016 at 1:59 PM, Razvan Cojocaru < > 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>> 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>> >> > >>>>> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com >> > <mailto:rcojocaru@bitdefender.com>> >> > >>>>> Cc: Keir Fraser <keir@xen.org <mailto:keir@xen.org>> >> > >>>>> Cc: Jan Beulich <jbeulich@suse.com <mailto:jbeulich@suse.com >> >> >> > >>>>> Cc: Andrew Cooper <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>> >> > >>> Oops. >> > >>> >> > >>> Reviewed-by: Andrew Cooper <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. Tamas
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
diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c index a3d4892..c218f12 100644 --- a/xen/arch/x86/hvm/event.c +++ b/xen/arch/x86/hvm/event.c @@ -28,7 +28,8 @@ static void hvm_event_fill_regs(vm_event_request_t *req) { const struct cpu_user_regs *regs = guest_cpu_user_regs(); - const struct vcpu *curr = current; + struct vcpu *curr = current; + struct segment_register seg; req->data.regs.x86.rax = regs->eax; req->data.regs.x86.rcx = regs->ecx; @@ -55,6 +56,12 @@ static void hvm_event_fill_regs(vm_event_request_t *req) req->data.regs.x86.cr0 = curr->arch.hvm_vcpu.guest_cr[0]; req->data.regs.x86.cr3 = curr->arch.hvm_vcpu.guest_cr[3]; req->data.regs.x86.cr4 = curr->arch.hvm_vcpu.guest_cr[4]; + + hvm_get_segment_register(curr, x86_seg_fs, &seg); + req->data.regs.x86.fs_base = seg.base; + + hvm_get_segment_register(curr, x86_seg_gs, &seg); + req->data.regs.x86.gs_base = seg.base; } static int hvm_event_traps(uint8_t sync, vm_event_request_t *req)
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> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> Cc: Keir Fraser <keir@xen.org> Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> --- xen/arch/x86/hvm/event.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)