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

From: Razvan Cojocaru <rcojocaru@bitdefender.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>, xen-devel@lists.xen.org
Cc: george.dunlap@eu.citrix.com, "Lengyel,
	Tamas" <tlengyel@novetta.com>,
	keir@xen.org, jbeulich@suse.com
Subject: Re: [PATCH] x86/hvm: simplify emulation triggered by vm_event response
Date: Mon, 8 Feb 2016 11:12:32 +0200	[thread overview]
Message-ID: <56B85C00.80006@bitdefender.com> (raw)
In-Reply-To: <56B349A0.90802@bitdefender.com>

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.