KVM Archive on lore.kernel.org
 help / color / Atom feed
* What's with all of the hardcoded instruction lengths in svm.c?
@ 2019-06-12 20:17 Jim Mattson
  2019-06-13 13:55 ` Vitaly Kuznetsov
  0 siblings, 1 reply; 4+ messages in thread
From: Jim Mattson @ 2019-06-12 20:17 UTC (permalink / raw)
  To: kvm list

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.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: What's with all of the hardcoded instruction lengths in svm.c?
  2019-06-12 20:17 What's with all of the hardcoded instruction lengths in svm.c? Jim Mattson
@ 2019-06-13 13:55 ` Vitaly Kuznetsov
  2019-06-13 16:08   ` Jim Mattson
  0 siblings, 1 reply; 4+ messages in thread
From: Vitaly Kuznetsov @ 2019-06-13 13:55 UTC (permalink / raw)
  To: Jim Mattson; +Cc: kvm list

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: What's with all of the hardcoded instruction lengths in svm.c?
  2019-06-13 13:55 ` Vitaly Kuznetsov
@ 2019-06-13 16:08   ` Jim Mattson
  2019-06-14 17:01     ` Vitaly Kuznetsov
  0 siblings, 1 reply; 4+ messages in thread
From: Jim Mattson @ 2019-06-13 16:08 UTC (permalink / raw)
  To: Vitaly Kuznetsov; +Cc: kvm list

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!

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: What's with all of the hardcoded instruction lengths in svm.c?
  2019-06-13 16:08   ` Jim Mattson
@ 2019-06-14 17:01     ` Vitaly Kuznetsov
  0 siblings, 0 replies; 4+ messages in thread
From: Vitaly Kuznetsov @ 2019-06-14 17:01 UTC (permalink / raw)
  To: Jim Mattson; +Cc: kvm list

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-12 20:17 What's with all of the hardcoded instruction lengths in svm.c? Jim Mattson
2019-06-13 13:55 ` Vitaly Kuznetsov
2019-06-13 16:08   ` Jim Mattson
2019-06-14 17:01     ` Vitaly Kuznetsov

KVM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvm/0 kvm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kvm kvm/ https://lore.kernel.org/kvm \
		kvm@vger.kernel.org kvm@archiver.kernel.org
	public-inbox-index kvm


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.kvm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox