From mboxrd@z Thu Jan 1 00:00:00 1970 From: Razvan Cojocaru Subject: Re: [PATCH] x86/hvm: simplify emulation triggered by vm_event response Date: Mon, 8 Feb 2016 11:12:32 +0200 Message-ID: <56B85C00.80006@bitdefender.com> References: <1454588852-5389-1-git-send-email-rcojocaru@bitdefender.com> <56B345D1.5090200@citrix.com> <56B349A0.90802@bitdefender.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <56B349A0.90802@bitdefender.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper , xen-devel@lists.xen.org Cc: george.dunlap@eu.citrix.com, "Lengyel, Tamas" , keir@xen.org, jbeulich@suse.com List-Id: xen-devel@lists.xenproject.org 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 >>> --- >>> 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 >> 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.