Re: [PATCH] svm: implement NEXTRIPsave SVM feature

[Andre Przywara <andre.przywara@amd.com>]

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