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

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

[Jim Mattson]

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.

It looks like this code was checked in with commit 6aa8b732ca01c
("kvm: userspace interface"), with nary a word of explanation.

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

[Vitaly Kuznetsov]

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

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

[Jim Mattson]

On Thu, Jun 13, 2019 at 6:55 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote:

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

That would be great!

Thanks!

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

[Vitaly Kuznetsov]

Jim Mattson <jmattson@google.com> writes:

> On Thu, Jun 13, 2019 at 6:55 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>
>> I can try sending a patch removing the manual advancement to see if
>> anyone has any objections.
>
> That would be great!
>

Turns out this is harder than I initially thought, in the emulator we
don't emulate everything (e.g. XSETVB) and emulating some instructions
(even with EMULTYPE_SKIP) gives us some unintended side-effects,
e.g. I'm currently observing a hang when trying to apply
kvm_emulate_instruction() to HLT.  

Overall, I still think this is the right approach, we just need to make
EMULTYPE_SKIP skip correctly. Stay tuned...

-- 
Vitaly