On 12.04.2010, at 00:13, Andre Przywara wrote:
> 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.
I remember that this was the XBox hack. So it's not as constructed as it sounds :-). It's probably not real-world relevant, but worth keeping in mind that we're not 100% compatible then.
>
>>> 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 see. No need to go that far. The current code works just fine as is and is fast.
> > 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?
Hrm. I don't think it's too much work to go through the individual next_rip setters and convert them to call an inline function. In fact, it's probably easier than writing mails in this thread :-). If you like I can do it for you?
Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
