From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andre Przywara Subject: Re: [PATCH] svm: implement NEXTRIPsave SVM feature Date: Mon, 12 Apr 2010 00:13:06 +0200 Message-ID: <4BC24972.3070301@amd.com> References: <1271020048-10083-1-git-send-email-andre.przywara@amd.com> <98EA2068-1878-4698-945B-10BACCB0631F@suse.de> <4BC24478.5060803@amd.com> <9837F692-5DB6-4E81-9CFD-8405312DE542@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Cc: Avi Kivity , kvm-devel list To: Alexander Graf Return-path: Received: from va3ehsobe001.messaging.microsoft.com ([216.32.180.11]:26026 "EHLO VA3EHSOBE001.bigfish.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753081Ab0DKWOM (ORCPT ); Sun, 11 Apr 2010 18:14:12 -0400 In-Reply-To: <9837F692-5DB6-4E81-9CFD-8405312DE542@suse.de> Sender: kvm-owner@vger.kernel.org List-ID: Alexander Graf wrote: > On 11.04.2010, at 23:51, Andre Przywara wrote: > >> Alexander Graf wrote: >>> On 11.04.2010, at 23:40, Alexander Graf wrote: >>>> /* Either adds offset n to the instruction counter or takes the next >>>> instruction pointer from the vmcb if the CPU supports it */ >>>> >>>> static u64 svm_next_rip(struct kvm_vcpu *vcpu, int add) >>>> { >>>> if (svm->vmcb->control.next_rip != 0) >>> In fact, that if should probably be: >>> if (svm_has(SVM_FEATURE_NRIP)) >> This is not sufficient. The next RIP is only provided for some >> intercepts (namely instruction intercepts), so one would need to >> check the validity of this field anyway. By definition reserved >> VMCB fields are 0, and as 0 is never a valid _next_ RIP, this >> is a quick and correct check. > It's not? If you're at -1 which is hlt in our imaginary case. What would the next instruction be? A wrap-around to zero? From kernel space to user space? Come on, that sounds a bit constructed (A20, someone?). I dimly remember reading in our internal docs that 0 is a safe indicator for a missing NEXTRIP. I will do some research tomorrow. >> P.S. I don't have a strong opinion about your proposed refactoring, >> if Avi agrees I will rework it. I only found the current fix small >> and easy, and the mentioned patch for older CPUs removed the add >> line anyway, so the concerns you rose did not apply to the original >> version of the patch. > > What patch for older CPUs? The one that'd be expensive? Yes. It removes the "guessed" value lines entirely and triggers a decode if NEXTRIP is not available. > I was more concerned about readability here - it's great to > be able to follow code on what it does :-). Maybe a comment about the overriding behavior of the NEXTRIP line would appease you? Andre. -- Andre Przywara AMD-Operating System Research Center (OSRC), Dresden, Germany Tel: +49 351 488-3567-12