[PATCH] x86/hvm: simplify emulation triggered by vm_event response

[PATCH] x86/hvm: simplify emulation triggered by vm_event response

[Razvan Cojocaru]

Currently, after receiving a vm_event reply requesting emulation,
the actual emulation is triggered in p2m_mem_access_check(),
which means that we're waiting for the page fault to occur again
before emulating. Aside from the performance impact, this
complicates the code since between hvm_do_resume() and the second
page fault it is possible that the latter becomes a completely
new page fault - hence checking that EIP and the GPA match with
the ones in the original page fault. If they don't, duplicate
EPT fault vm_events will occur, of which a monitoring application
needs to be aware.
This patch makes struct arch_vm_event smaller (since we no longer
need to track eip and gpa), removes the checking code from
p2m_mem_access_check(), and moves the emulation in hvm_do_resume().

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
---
 xen/arch/x86/hvm/hvm.c         | 17 +++++++++++++++++
 xen/arch/x86/mm/p2m.c          | 34 ----------------------------------
 xen/include/asm-x86/vm_event.h |  2 --
 3 files changed, 17 insertions(+), 36 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 35ec6c9..930d0e3 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -552,6 +552,23 @@ void hvm_do_resume(struct vcpu *v)
     {
         struct monitor_write_data *w = &v->arch.vm_event->write_data;
 
+        if ( v->arch.vm_event->emulate_flags )
+        {
+            enum emul_kind kind = EMUL_KIND_NORMAL;
+
+            if ( v->arch.vm_event->emulate_flags &
+                 VM_EVENT_FLAG_SET_EMUL_READ_DATA )
+                kind = EMUL_KIND_SET_CONTEXT;
+            else if ( v->arch.vm_event->emulate_flags &
+                      VM_EVENT_FLAG_EMULATE_NOWRITE )
+                kind = EMUL_KIND_NOWRITE;
+
+            hvm_mem_access_emulate_one(kind, TRAP_invalid_op,
+                                       HVM_DELIVER_NO_ERROR_CODE);
+
+            v->arch.vm_event->emulate_flags = 0;
+        }
+
         if ( w->do_write.msr )
         {
             hvm_msr_write_intercept(w->msr, w->value, 0);
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index a45ee35..47e7fad 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1639,7 +1639,6 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla,
     p2m_access_t p2ma;
     vm_event_request_t *req;
     int rc;
-    unsigned long eip = guest_cpu_user_regs()->eip;
 
     if ( altp2m_active(d) )
         p2m = p2m_get_altp2m(v);
@@ -1698,39 +1697,6 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla,
         }
     }
 
-    /* The previous vm_event reply does not match the current state. */
-    if ( unlikely(v->arch.vm_event) &&
-         (v->arch.vm_event->gpa != gpa || v->arch.vm_event->eip != eip) )
-    {
-        /* Don't emulate the current instruction, send a new vm_event. */
-        v->arch.vm_event->emulate_flags = 0;
-
-        /*
-         * Make sure to mark the current state to match it again against
-         * the new vm_event about to be sent.
-         */
-        v->arch.vm_event->gpa = gpa;
-        v->arch.vm_event->eip = eip;
-    }
-
-    if ( unlikely(v->arch.vm_event) && v->arch.vm_event->emulate_flags )
-    {
-        enum emul_kind kind = EMUL_KIND_NORMAL;
-
-        if ( v->arch.vm_event->emulate_flags &
-             VM_EVENT_FLAG_SET_EMUL_READ_DATA )
-            kind = EMUL_KIND_SET_CONTEXT;
-        else if ( v->arch.vm_event->emulate_flags &
-                  VM_EVENT_FLAG_EMULATE_NOWRITE )
-            kind = EMUL_KIND_NOWRITE;
-
-        hvm_mem_access_emulate_one(kind, TRAP_invalid_op,
-                                   HVM_DELIVER_NO_ERROR_CODE);
-
-        v->arch.vm_event->emulate_flags = 0;
-        return 1;
-    }
-
     *req_ptr = NULL;
     req = xzalloc(vm_event_request_t);
     if ( req )
diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h
index 5aff834..fff8326 100644
--- a/xen/include/asm-x86/vm_event.h
+++ b/xen/include/asm-x86/vm_event.h
@@ -28,8 +28,6 @@
  */
 struct arch_vm_event {
     uint32_t emulate_flags;
-    unsigned long gpa;
-    unsigned long eip;
     struct vm_event_emul_read_data emul_read_data;
     struct monitor_write_data write_data;
 };
-- 
1.9.1

Re: [PATCH] x86/hvm: simplify emulation triggered by vm_event response

[Andrew Cooper]

On 04/02/16 12:27, Razvan Cojocaru wrote:
> Currently, after receiving a vm_event reply requesting emulation,
> the actual emulation is triggered in p2m_mem_access_check(),
> which means that we're waiting for the page fault to occur again
> before emulating.

Presumably this means that we re-enter the guest and exit immediately
for (hopefully) the same violation?

>  Aside from the performance impact, this
> complicates the code since between hvm_do_resume() and the second
> page fault it is possible that the latter becomes a completely
> new page fault - hence checking that EIP and the GPA match with
> the ones in the original page fault.

Presumably this occurs when we injected an event on the vmentry?

>  If they don't, duplicate
> EPT fault vm_events will occur, of which a monitoring application
> needs to be aware.
> This patch makes struct arch_vm_event smaller (since we no longer
> need to track eip and gpa), removes the checking code from
> p2m_mem_access_check(), and moves the emulation in hvm_do_resume().
>
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> ---
>  xen/arch/x86/hvm/hvm.c         | 17 +++++++++++++++++
>  xen/arch/x86/mm/p2m.c          | 34 ----------------------------------
>  xen/include/asm-x86/vm_event.h |  2 --
>  3 files changed, 17 insertions(+), 36 deletions(-)

Gotta love that diffstat!

The logic makes sense, so Acked-by: Andrew Cooper
<andrew.cooper3@citrix.com> for the x86-related nature, but it would be
nice to have a review from Tamas for the vm_event side of things.

Re: [PATCH] x86/hvm: simplify emulation triggered by vm_event response

[Razvan Cojocaru]

On 02/04/2016 02:36 PM, Andrew Cooper wrote:
> On 04/02/16 12:27, Razvan Cojocaru wrote:
>> Currently, after receiving a vm_event reply requesting emulation,
>> the actual emulation is triggered in p2m_mem_access_check(),
>> which means that we're waiting for the page fault to occur again
>> before emulating.
> 
> Presumably this means that we re-enter the guest and exit immediately
> for (hopefully) the same violation?

Yes, something along those lines: the original page fault occurs, a
vm_event is being sent to the application, which replies with the
EMULATE flag set (but does not lift the page restrictions). Now we're
hoping that the same instruction that has caused the first page fault
runs, which triggers a new page fault (thus a new call of
p2m_mem_access_check()). But in p2m_mem_access_check() we check to see
if emulate flags are set for the current VCPU, and if they are, we check
to see that the instruction is really the one that has caused the
original page fault, and if it is (i.e. both EIP and GPA match), we emulate.

The ideal case is the one where the second page fault is being caused by
the same instruction hitting the same page, and that happens most of the
time, but it unfortunately does not happen all of the time.

So when the second page fault is _not_ caused by the same instruction,
we just reset the emulate flags and carry on with regular processing,
which means that a new vm_event will be sent out about this new page
fault. But even though the application has reuquested that the page
fault that has triggered the last page fault be emulated, it wasn't (as
a design limitation). So now, when / if the old instruction hits the
page again, it will be received by the monitoring application as a new
hit, not still the old, unemulated one.

There are safeguards possible for this in the monitoring application,
but they too have limitations, and it is ultimately less efficient and
more error-prone that the alternative hopefully is.

>>  Aside from the performance impact, this
>> complicates the code since between hvm_do_resume() and the second
>> page fault it is possible that the latter becomes a completely
>> new page fault - hence checking that EIP and the GPA match with
>> the ones in the original page fault.
> 
> Presumably this occurs when we injected an event on the vmentry?

Any asynchronous-type cause of a page fault will do, hopefully I've been
able to explain somewhat above.

>>  If they don't, duplicate
>> EPT fault vm_events will occur, of which a monitoring application
>> needs to be aware.
>> This patch makes struct arch_vm_event smaller (since we no longer
>> need to track eip and gpa), removes the checking code from
>> p2m_mem_access_check(), and moves the emulation in hvm_do_resume().
>>
>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>> ---
>>  xen/arch/x86/hvm/hvm.c         | 17 +++++++++++++++++
>>  xen/arch/x86/mm/p2m.c          | 34 ----------------------------------
>>  xen/include/asm-x86/vm_event.h |  2 --
>>  3 files changed, 17 insertions(+), 36 deletions(-)
> 
> Gotta love that diffstat!
> 
> The logic makes sense, so Acked-by: Andrew Cooper
> <andrew.cooper3@citrix.com> for the x86-related nature, but it would be
> nice to have a review from Tamas for the vm_event side of things.

Thanks! Of course. Hopefully Tamas will like this, based on a
conversation we've had a few days ago here.


Thanks,
Razvan

Re: [PATCH] x86/hvm: simplify emulation triggered by vm_event response

[Razvan Cojocaru]

On 02/04/2016 02:52 PM, Razvan Cojocaru wrote:
> On 02/04/2016 02:36 PM, Andrew Cooper wrote:
>> On 04/02/16 12:27, Razvan Cojocaru wrote:
>>> Currently, after receiving a vm_event reply requesting emulation,
>>> the actual emulation is triggered in p2m_mem_access_check(),
>>> which means that we're waiting for the page fault to occur again
>>> before emulating.
>>
>> Presumably this means that we re-enter the guest and exit immediately
>> for (hopefully) the same violation?
> 
> Yes, something along those lines: the original page fault occurs, a
> vm_event is being sent to the application, which replies with the
> EMULATE flag set (but does not lift the page restrictions). Now we're
> hoping that the same instruction that has caused the first page fault
> runs, which triggers a new page fault (thus a new call of
> p2m_mem_access_check()). But in p2m_mem_access_check() we check to see
> if emulate flags are set for the current VCPU, and if they are, we check
> to see that the instruction is really the one that has caused the
> original page fault, and if it is (i.e. both EIP and GPA match), we emulate.
> 
> The ideal case is the one where the second page fault is being caused by
> the same instruction hitting the same page, and that happens most of the
> time, but it unfortunately does not happen all of the time.
> 
> So when the second page fault is _not_ caused by the same instruction,
> we just reset the emulate flags and carry on with regular processing,
> which means that a new vm_event will be sent out about this new page
> fault. But even though the application has reuquested that the page
> fault that has triggered the last page fault be emulated, it wasn't (as
> a design limitation). So now, when / if the old instruction hits the
> page again, it will be received by the monitoring application as a new
> hit, not still the old, unemulated one.
> 
> There are safeguards possible for this in the monitoring application,
> but they too have limitations, and it is ultimately less efficient and
> more error-prone that the alternative hopefully is.
> 
>>>  Aside from the performance impact, this
>>> complicates the code since between hvm_do_resume() and the second
>>> page fault it is possible that the latter becomes a completely
>>> new page fault - hence checking that EIP and the GPA match with
>>> the ones in the original page fault.
>>
>> Presumably this occurs when we injected an event on the vmentry?
> 
> Any asynchronous-type cause of a page fault will do, hopefully I've been
> able to explain somewhat above.
> 
>>>  If they don't, duplicate
>>> EPT fault vm_events will occur, of which a monitoring application
>>> needs to be aware.
>>> This patch makes struct arch_vm_event smaller (since we no longer
>>> need to track eip and gpa), removes the checking code from
>>> p2m_mem_access_check(), and moves the emulation in hvm_do_resume().
>>>
>>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>> ---
>>>  xen/arch/x86/hvm/hvm.c         | 17 +++++++++++++++++
>>>  xen/arch/x86/mm/p2m.c          | 34 ----------------------------------
>>>  xen/include/asm-x86/vm_event.h |  2 --
>>>  3 files changed, 17 insertions(+), 36 deletions(-)
>>
>> Gotta love that diffstat!
>>
>> The logic makes sense, so Acked-by: Andrew Cooper
>> <andrew.cooper3@citrix.com> for the x86-related nature, but it would be
>> nice to have a review from Tamas for the vm_event side of things.
> 
> Thanks! Of course. Hopefully Tamas will like this, based on a
> conversation we've had a few days ago here.

I've changed Tamas' address to the @novetta one, I think he might not be
using the older @zentific one.

Re: [PATCH] x86/hvm: simplify emulation triggered by vm_event response

[Tamas K Lengyel]

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

> >> The logic makes sense, so Acked-by: Andrew Cooper
> >> <andrew.cooper3@citrix.com> for the x86-related nature, but it would be
> >> nice to have a review from Tamas for the vm_event side of things.
> >
> > Thanks! Of course. Hopefully Tamas will like this, based on a
> > conversation we've had a few days ago here.
>

Yes, it's a step in the right direction!


>
> I've changed Tamas' address to the @novetta one, I think he might not be
> using the older @zentific one.
>

Thanks, either of these emails is actually fine but for Xen related things
tamas@tklengyel.com (as in the Maintainers file) works best as that's where
I have the proper filtering rules setup as to avoid getting mails like this
lost in the inbox.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 1404 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] x86/hvm: simplify emulation triggered by vm_event response

[Tamas K Lengyel]

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

On Thu, Feb 4, 2016 at 5:27 AM, Razvan Cojocaru <rcojocaru@bitdefender.com>
wrote:

> Currently, after receiving a vm_event reply requesting emulation,
> the actual emulation is triggered in p2m_mem_access_check(),
> which means that we're waiting for the page fault to occur again
> before emulating. Aside from the performance impact, this
> complicates the code since between hvm_do_resume() and the second
> page fault it is possible that the latter becomes a completely
> new page fault - hence checking that EIP and the GPA match with
> the ones in the original page fault. If they don't, duplicate
> EPT fault vm_events will occur, of which a monitoring application
> needs to be aware.
> This patch makes struct arch_vm_event smaller (since we no longer
> need to track eip and gpa), removes the checking code from
> p2m_mem_access_check(), and moves the emulation in hvm_do_resume().
>
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>

Acked-by: Tamas K Lengyel <tamas@tklengyel.com>

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

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

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