Re: [PATCH] svm: implement NEXTRIPsave SVM feature

Andre Przywara
2010-04-11T22:14:12Z

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