Re: What's with all of the hardcoded instruction lengths in svm.c?

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Jim Mattson <jmattson@google.com> writes:

> Take the following code in rdmsr_interception, for example.
>
> svm->next_rip = kvm_rip_read(&svm->vcpu) + 2;
>
> Yes, the canonical rdmsr instruction is two bytes. However, there is
> nothing in the architectural specification prohibiting useless or
> redundant prefixes. So, for instance, 65 66 67 67 67 0f 32 is a
> perfectly valid 7-byte rdmsr instruction.

(I don't know much about why this was added but nobody else commented
so in case I'm not terribly mistaken):

This looks ugly, it is likely an over-optimization: we seem to only
advance svm->next_rip to be able to avoid doing
kvm_emulate_instruction() in skip_emulated_instruction(). With NRIP_SAVE
feature (appeared long ago) we don't use the advanced value as we
already know the next RIP:

	if (svm->vmcb->control.next_rip != 0) {
		WARN_ON_ONCE(!static_cpu_has(X86_FEATURE_NRIPS));
		svm->next_rip = svm->vmcb->control.next_rip;
	}

IMO, always doing kvm_emulate_instruction(vcpu, EMULTYPE_SKIP) in !NRIPS
case would be the correct way. I tried throwing away these advancements
and nothing broke, with and without NRIPS.

I can try sending a patch removing the manual advancement to see if
anyone has any objections.

-- 
Vitaly