[PATCH] vm_event: Record FS_BASE/GS_BASE during events

[PATCH] vm_event: Record FS_BASE/GS_BASE during events

[Tamas K Lengyel]

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(-)

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)
-- 
2.1.4

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

[Razvan Cojocaru]

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

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

[Andrew Cooper]

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>

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

[Razvan Cojocaru]

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

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

[Andrew Cooper]

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

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

[Razvan Cojocaru]

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

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

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 2027 bytes --]

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

[-- Attachment #1.2: Type: text/html, Size: 3244 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Razvan Cojocaru]

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

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

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 3571 bytes --]

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:

[-- Attachment #1.2: Type: text/html, Size: 5753 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Tamas K Lengyel]

[-- Attachment #1.1: Type: text/plain, Size: 4010 bytes --]

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

[-- Attachment #1.2: Type: text/html, Size: 6754 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Razvan Cojocaru]

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