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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Razvan Cojocaru <rcojocaru@bitdefender.com>, xen-devel@lists.xen.org
Cc: george.dunlap@eu.citrix.com,
	Tamas K Lengyel <tamas.lengyel@zentific.com>,
	keir@xen.org, jbeulich@suse.com
Subject: Re: [PATCH] x86/hvm: simplify emulation triggered by vm_event response
Date: Thu, 4 Feb 2016 12:36:33 +0000	[thread overview]
Message-ID: <56B345D1.5090200@citrix.com> (raw)
In-Reply-To: <1454588852-5389-1-git-send-email-rcojocaru@bitdefender.com>

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.