Nested VMX, v5

The SVM module had a "nested" option, on by default, which controls whether to allow nested virtualization. Now that VMX also supports nested virtualization, we can move this option to x86.c, for both SVM and VMX.

The "nested" option takes three possible values. 0 disables nested virtualization on both SVM and VMX, and 1 enables it on both. The value 2, which is the default when this module option is not explicitly set, asks each of SVM or VMX to choose its own default; Currently, VMX disables nested virtualization in this case, while SVM leaves it enabled.

When nested VMX becomes more mature, this default should probably be changed to enable nested virtualization on both architectures.

arch/x86/kvm/x86.c +2 -0

@@ -95,6 +95,17 @@ EXPORT_SYMBOL_GPL(kvm_x86_ops);
  int ignore_msrs = 0;
  module_param_named(ignore_msrs, ignore_msrs, bool, S_IRUGO | S_IWUSR);
 
+/* If nested=1, nested virtualization is supported. I.e., the guest may use
+ * VMX or SVM (as appropriate) and be a hypervisor for its own guests.

A global variable names 'nested' is not a good idea. I recommend having a kvm-intel scope module parameter instead, that also avoids the 0/1/2 values.

Add the "VMX" CPU feature to the list of CPU featuress KVM advertises with the KVM_GET_SUPPORTED_CPUID ioctl (unless the "nested" module option is off).

Qemu uses this ioctl, and intersects KVM's list with its own list of desired cpu features (depending on the -cpu option given to qemu) to determine the final list of features presented to the guest.
This patch also does the same for SVM: KVM now advertises it supports SVM, unless the "nested" module option is off.

Signed-off-by Nadav Har'El<[email protected]>

---

Review

Quoted 8 lines

(quoted diff) +1 -1

 	const u32 kvm_supported_word4_x86_features =
 		F(XMM3) | 0 /* Reserved, DTES64, MONITOR */ |
-		0 /* DS-CPL, VMX, SMX, EST */ |
+		0 /* DS-CPL */ | (nested ? F(VMX) : 0) | 0 /* SMX, EST */ |
 		0 /* TM2 */ | F(SSSE3) | 0 /* CNXT-ID */ | 0 /* Reserved */ |
 		0 /* Reserved */ | F(CX16) | 0 /* xTPR Update, PDCM */ |
 		0 /* Reserved, DCA */ | F(XMM4_1) |

You can use kvm_x86_ops->set_supported_cpuid() to alter features.

This patch allows a guest to use the VMXON and VMXOFF instructions, and emulates them accordingly. Basically this amounts to checking some prerequisites, and then remembering whether the guest has enabled or disabled VMX operation.

+/* The nested_vmx structure is part of vcpu_vmx, and holds information we need
+ * for correct emulation of VMX (i.e., nested VMX) on this vcpu. For example,
+ * the current VMCS set by L1, a list of the VMCSs used to run the active
+ * L2 guests on the hardware, and more.
+ */

arch/x86/kvm/vmx.c +30 -0

+/* Emulate the VMXON instruction.
+ * Currently, we just remember that VMX is active, and do not save or even
+ * inspect the argument to VMXON (the so-called "VMXON pointer") because we
+ * do not currently need to store anything in that guest-allocated memory
+ * region. Consequently, VMCLEAR and VMPTRLD also do not verify that the their
+ * argument is different from the VMXON pointer (which the spec says they do).
+ */
+static int handle_vmon(struct kvm_vcpu *vcpu)
+{
+	struct kvm_segment cs;
+	struct vcpu_vmx *vmx = to_vmx(vcpu);
+
+	/* The Intel VMX Instruction Reference lists a bunch of bits that
+	 * are prerequisite to running VMXON, most notably CR4.VMXE must be
+	 * set to 1. Otherwise, we should fail with #UD. We test these now:
+	 */
+	if (!nested) {
+		kvm_queue_exception(vcpu, UD_VECTOR);
+		return 1;
+	}
+
+	if (!(vcpu->arch.cr4&  X86_CR4_VMXE) ||
+	    !(vcpu->arch.cr0&  X86_CR0_PE) ||
+	    (vmx_get_rflags(vcpu)&  X86_EFLAGS_VM)) {
+		kvm_queue_exception(vcpu, UD_VECTOR);
+		return 1;
+	}
+
+	vmx_get_segment(vcpu,&cs, VCPU_SREG_CS);
+	if (is_long_mode(vcpu)&&  !cs.l) {

Quoted 5 lines

This patch allows a guest to use the VMXON and VMXOFF instructions, and emulates them accordingly. Basically this amounts to checking some prerequisites, and then remembering whether the guest has enabled or disabled
VMX operation.

Should probably reorder with next patch.

Please (here and elsewhere) use the standard kernel style for multiline comments - start with /* on a line by itself.

Review

Quoted 2 lines

(quoted diff) +1 -0

+	vmx->nested.vmxon = 1;

= true

Need to block INIT signals in the local apic as well (fine for a separate patch).

Hi,

On Mon, Jun 14, 2010, Avi Kivity wrote about "Re: [PATCH 3/24] Implement VMXON and VMXOFF":

Quoted 8 lines · Nadav Har'El

Quoted 7 lines

This patch allows a guest to use the VMXON and VMXOFF instructions, and emulates them accordingly. Basically this amounts to checking some prerequisites, and then remembering whether the guest has enabled or disabled
VMX operation.

Quoted 2 lines

Should probably reorder with next patch.

I can't do this if I want the code to compile after each patch, because the next patch (controlling when setting cr4.VMXE can be set) needs to check whether VMXON was done.

Quoted 3 lines

Need to block INIT signals in the local apic as well (fine for a separate patch).

I've been looking into how I might best go about achieving this.

The APIC_DM_INIT handler is in lapic.c, which is not aware of VMX or (obviously) nested VMX. So I need to add some sort of generic "block INIT" flag which that code will check. Is this the sort of fix you had in mind?

This patch allows a guest to use the VMXON and VMXOFF instructions, and emulates them accordingly. Basically this amounts to checking some prerequisites, and then remembering whether the guest has enabled or disabled VMX operation.

Signed-off-by Nadav Har'El <[email protected]>

---

arch/x86/kvm/vmx.c +26 -0

@@ -117,6 +117,16 @@ struct shared_msr_entry {
 	u64 mask;
 };
 
+/* The nested_vmx structure is part of vcpu_vmx, and holds information we need
+ * for correct emulation of VMX (i.e., nested VMX) on this vcpu. For example,
+ * the current VMCS set by L1, a list of the VMCSs used to run the active
+ * L2 guests on the hardware, and more.
+ */
+struct nested_vmx {
+	/* Has the level1 guest done vmxon? */
+	bool vmxon;
+};
+
 struct vcpu_vmx {
 	struct kvm_vcpu       vcpu;
 	struct list_head      local_vcpus_link;

@@ -168,6 +178,9 @@ struct vcpu_vmx {
 	u32 exit_reason;
 
 	bool rdtscp_enabled;
+
+	/* Support for guest hypervisors (nested VMX) */
+	struct nested_vmx nested;
 };
 
 static inline struct vcpu_vmx *to_vmx(struct kvm_vcpu *vcpu)

@@ -3353,6 +3366,93 @@ static int handle_vmx_insn(struct kvm_vc
 	return 1;
 }
 
+/* Emulate the VMXON instruction.
+ * Currently, we just remember that VMX is active, and do not save or even
+ * inspect the argument to VMXON (the so-called "VMXON pointer") because we
+ * do not currently need to store anything in that guest-allocated memory
+ * region. Consequently, VMCLEAR and VMPTRLD also do not verify that the their
+ * argument is different from the VMXON pointer (which the spec says they do).
+ */
+static int handle_vmon(struct kvm_vcpu *vcpu)
+{
+	struct kvm_segment cs;
+	struct vcpu_vmx *vmx = to_vmx(vcpu);
+
+	/* The Intel VMX Instruction Reference lists a bunch of bits that

arch/x86/kvm/vmx.c static int handle_vmx_insn(struct kvm_vc · line 3391

Quoted 2 lines

arch/x86/kvm/vmx.c +2 -0

+	if (!(vcpu->arch.cr4 & X86_CR4_VMXE) ||
+	    !(vcpu->arch.cr0 & X86_CR0_PE) ||

kvm_read_cr0_bits, kvm_read_cr4_bits.

This patch allows the guest to enable the VMXE bit in CR4, which is a prerequisite to running VMXON.

Signed-off-by Nadav Har'El <[email protected]>

---

arch/x86/kvm/x86.c +0 -1

 	kvm_x86_ops->set_cr4(vcpu, cr4);
--

To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 2 lines

This patch allows the guest to enable the VMXE bit in CR4, which is a prerequisite to running VMXON.

..

Quoted 1 line

--- .before/arch/x86/kvm/x86.c 2010-06-13 15:01:28.000000000 +0300

..

Review

Quoted 2 lines

(quoted diff) +1 -0

+	if (cr4 & x86_cr4_vmxe && !nested)
 		return 1;

We shouldn't be able to clear X86_CR4_VMXE after VMXON.

An implementation of VMX needs to define a VMCS structure. This structure is kept in guest memory, but is opaque to the guest (who can only read or write it with VMX instructions).

This patch starts to define the VMCS structure which our nested VMX implementation will present to L1. We call it "vmcs12", as it is the VMCS that L1 keeps for its L2 guests.

This patch also adds the notion (as required by the VMX spec) of the "current VMCS", and finally includes utility functions for mapping the guest-allocated VMCSs in host memory.

+#define VMCS12_REVISION 0x11e57ed0

arch/x86/kvm/vmx.c +5 -0

+
+/*
+ * struct vmcs12 describes the state that our guest hypervisor (L1) keeps for a
+ * single nested guest (L2), hence the name vmcs12. Any VMX implementation has
+ * a VMCS structure (which is opaque to the guest), and vmcs12 is our emulated

arch/x86/kvm/vmx.c +12 -0

+		kvm_release_page_clean(vmcs_page);
+		return NULL;
+	}
+	return vmcs_page;
+}
+
+static void nested_unmap_current(struct kvm_vcpu *vcpu)
+{
+	struct page *page;
+	struct vcpu_vmx *vmx = to_vmx(vcpu);
+
+	if (!vmx->nested.current_l2_page) {

Quoted 2 lines

+#define VMCS12_REVISION 0x11e57ed0

Where did this number come from? It's not from real hardware, yes?

On Mon, Jun 14, 2010, Avi Kivity wrote about "Re: [PATCH 5/24] Introduce vmcs12: a VMCS structure for L1":

Quoted 5 lines · Nadav Har'El

Quoted 4 lines

+#define VMCS12_REVISION 0x11e57ed0

Quoted 2 lines

Where did this number come from? It's not from real hardware, yes?

Basically, we are presenting emulated VMX for the L1 guest, complete with its own VMCS structure. This structure needs to have some VMCS revision id, which should be an arbitrary number that we invent - it is not related to any revision id that any real hardware uses. If you look closely, you can see that the number I used is leetspeak for "Nested0" ;-)

As you can see in the following patches, MSR_IA32_VMX_BASIC will return this arbitrary VMCS revision id, and and VMPTRLD will verify that the VMCS region that L1 is trying to load contains this revision id.

Quoted 6 lines

arch/x86/kvm/vmx.c +6 -0

+struct __attribute__ ((__packed__)) vmcs12 {
+	/* According to the Intel spec, a VMCS region must start with the
+	 * following two fields. Then follow implementation-specific data.
+	 */
+	u32 revision_id;
+	u32 abort;

Quoted 2 lines

+};

Note that this structure becomes an ABI, it cannot change except in a backward compatible way due to the need for live migration. So I'd like a documentation patch that adds a description of the content to Documentation/kvm/. It can be as simple as listing the structure definition.

On Mon, Jun 14, 2010, Avi Kivity wrote about "Re: [PATCH 5/24] Introduce vmcs12: a VMCS structure for L1":

Quoted 6 lines

Quoted 6 lines

arch/x86/kvm/vmx.c +6 -0

+struct __attribute__ ((__packed__)) vmcs12 {
+	/* According to the Intel spec, a VMCS region must start with the
+	 * following two fields. Then follow implementation-specific data.
+	 */
+	u32 revision_id;
+	u32 abort;

Quoted 9 lines

Quoted 3 lines

+};

Note that this structure becomes an ABI, it cannot change except in a backward compatible way due to the need for live migration. So I'd like a documentation patch that adds a description of the content to Documentation/kvm/. It can be as simple as listing the structure definition.

I agree that if struct vmcs12 is changed, this will cause problems for live migration, but why does this mean that the struct's fields or layout an ABI worth documenting?

After all, isn't the idea of VMCS that its internal content and layout is opaque for the L1 guest - he can only read/write it with VMREAD/VMWRITE, and those two instructions are the ABI (which is of course documented in the Intel spec) - not the content of the vmcs12 structure. Even if the guest knew the exact layout of this structure, he's not supposed to use it.

By the way, we have not actually checked that live migration is working as expected with nested virtualization running. I expect there to be more pitfalls and bugs even before we consider migration between two different versions. We would indeed like to allow live migration of different kinds (of L1 with all its L2 guests; Of all L2 guests of a L1; Of a single L2 guest), but we're trying to finish the more basic functionality first.

Quoted 2 lines

+#define VMCS12_REVISION 0x11e57ed0

Where did this number come from? It's not from real hardware, yes?

Quoted 2 lines

+struct __attribute__ ((__packed__)) vmcs12 {

__packed is a convenient define for this.

Quoted 5 lines

(quoted diff) +5 -0

+	/* According to the Intel spec, a VMCS region must start with the
+	 * following two fields. Then follow implementation-specific data.
+	 */
+	u32 revision_id;
+	u32 abort;

Quoted 2 lines

+};

Note that this structure becomes an ABI, it cannot change except in a backward compatible way due to the need for live migration. So I'd like a documentation patch that adds a description of the content to Documentation/kvm/. It can be as simple as listing the structure definition.

Review

Quoted 9 lines

(quoted diff) +8 -0

+static struct page *nested_get_page(struct kvm_vcpu *vcpu, u64 vmcs_addr)
+{
+	struct page *vmcs_page =
+		gfn_to_page(vcpu->kvm, vmcs_addr>>  PAGE_SHIFT);
+
+	if (is_error_page(vmcs_page)) {
+		printk(KERN_ERR "%s error allocating page 0x%llx\n",
+		       __func__, vmcs_addr);

Those printks can be used by a malicious guest to span the host logs. Please wrap them with something that is conditional on a debug flag.

I'm not sure what we need to do with vmcs that is not in RAM. It may simplify things to return the error_page to the caller and set KVM_REQ_TRIPLE_FAULT, so we don't have to deal with error handling later on.

Review

Quoted 2 lines

(quoted diff) +1 -0

+	kvm_release_page_dirty(page);

Do we always dirty the page?

I guess it is no big deal even if we don't.

Quoted 5 lines

Note that this structure becomes an ABI, it cannot change except in a backward compatible way due to the need for live migration. So I'd like a documentation patch that adds a description of the content to Documentation/kvm/. It can be as simple as listing the structure definition.

On Tue, Jun 22, 2010, Nadav Har'El wrote about "Re: [PATCH 5/24] Introduce vmcs12: a VMCS structure for L1":

Quoted 6 lines

Quoted 6 lines

Note that this structure becomes an ABI, it cannot change except in a backward compatible way due to the need for live migration. So I'd like a documentation patch that adds a description of the content to Documentation/kvm/. It can be as simple as listing the structure definition.

I decided that if I add a file in Documentation/kvm, it would be very useful for it to describe the nested vmx feature in general, in addition to the structure that you asked documented. So here is the new patch I propose:

----
Subject: [PATCH 25/25] Documentation

This patch includes a brief introduction to the nested vmx feature in the Documentation/kvm directory. The document also includes a copy of the vmcs12 structure, as requested by Avi Kivity.

Documentation/kvm/nested-vmx.txt +12 -0

+ABIs
+----
+
+Nested VMX aims to present a standard and (eventually) fully-functional VMX
+implementation for the a guest hypervisor to use. As such, the official
+specification of the ABI that it provides is Intel's VMX specification,
+namely volume 3B of their "Intel 64 and IA-32 Architectures Software
+Developer's Manual". Not all of VMX's features are currently fully supported,
+but the goal is to eventually support them all, starting with the VMX features
+which are used in practice by popular hypervisors (KVM and others).
+
+As a VMX implementation, nested VMX presents a VMCS structure to L1.

Documentation/kvm/nested-vmx.txt +4 -0

+
+struct __packed vmcs12 {
+	/* According to the Intel spec, a VMCS region must start with the
+	 * following two fields. Then follow implementation-specific data.

Documentation/kvm/nested-vmx.txt +32 -0

+	unsigned long cr0_guest_host_mask;
+	unsigned long cr4_guest_host_mask;
+	unsigned long cr0_read_shadow;
+	unsigned long cr4_read_shadow;
+	unsigned long cr3_target_value0;
+	unsigned long cr3_target_value1;
+	unsigned long cr3_target_value2;
+	unsigned long cr3_target_value3;
+	unsigned long exit_qualification;
+	unsigned long guest_linear_address;
+	unsigned long guest_cr0;
+	unsigned long guest_cr3;
+	unsigned long guest_cr4;
+	unsigned long guest_es_base;
+	unsigned long guest_cs_base;
+	unsigned long guest_ss_base;
+	unsigned long guest_ds_base;
+	unsigned long guest_fs_base;
+	unsigned long guest_gs_base;
+	unsigned long guest_ldtr_base;
+	unsigned long guest_tr_base;
+	unsigned long guest_gdtr_base;
+	unsigned long guest_idtr_base;
+	unsigned long guest_dr7;
+	unsigned long guest_rsp;
+	unsigned long guest_rip;
+	unsigned long guest_rflags;
+	unsigned long guest_pending_dbg_exceptions;
+	unsigned long guest_sysenter_esp;
+	unsigned long guest_sysenter_eip;
+	unsigned long host_cr0;
+	unsigned long host_cr3;

Documentation/kvm/nested-vmx.txt +3 -0

+};
+
+

Documentation/kvm/nested-vmx.txt line 22

Quoted 9 lines

(quoted diff) +2 -0

+
+We describe in much greater detail the theory behind the nested VMX 

feature,
+its implementation and its performance characteristics, in IBM Research report

Documentation/kvm/nested-vmx.txt +4 -0

+H-0282, "The Turtles Project: Design and Implementation of Nested
+Virtualization", available at:
+
+        http://bit.ly/a0o9te

Please put the true url in here.

Review

Quoted 2 lines

(quoted diff) +2 -0

+The current code support running Linux under a nested KVM using shadow
+page table (with bypass_guest_pf disabled).

Might as well remove this, since nvmx will not be merged with such a gaping hole.

In theory I ought to reject anything that doesn't comply with the spec. In practice I'll accept deviations from the spec, so long as

- those features aren't used by common guests
- when the features are attempted to be used, kvm will issue a warning

I don't think PFEC matching ought to present any implementation difficulty.

Quoted 10 lines

Quoted 2 lines

+page table (with bypass_guest_pf disabled).

Might as well remove this, since nvmx will not be merged with such a gaping hole.

In theory I ought to reject anything that doesn't comply with the spec. In practice I'll accept deviations from the spec, so long as

- those features aren't used by common guests
- when the features are attempted to be used, kvm will issue a warning

Ok, I plugged the big gaping hole and left a small invisible hole ;-)

The situation now is that you no longer have to run kvm with bypass_guest_pf, not on L0 and not on L1. L1 guests will run normally, possibly with bypass_guest_pf enabled. However, when L2 guests run every page-fault will cause an exit - regardless of what L0 or L1 tried to define via PFEC_MASK, PFEC_MATCH and EB[pf].

The reason why I said there is a "small hole" left is that now there is the possibility that we inject L1 with a page fault that it didn't expect to get. But in practice, this does not seem to cause any problems for neither KVM nor VMWare Server.

Quoted 1 line

I don't think PFEC matching ought to present any implementation difficulty.

Well, it is more complicated than it first appeared (at least to me). One problem is that there is no real way (at least none that I thought of) to "or" the pf-trapping desires of L0 and L1.

I solved this by traping all
page faults, which is unfortunate. The second problem, related to the first one, when L0 gets a page fault while running L2, it is now quite diffcult to figure out whether it should be injected into L1, i.e., whether L1 asked for this specific page-fault trap to happen. We need check whether the page_fault_error_code match's the L1-specified pfec_mask and pfec_match (and eb.pf), but it's actually more complicated, because the page_fault_error_code we got from the processor refers to the shadow page tables, and we need to translate it back to what it would mean for L1's page tables.

Doing this correctly would require me to spend quite a bit more time to understand exactly how the shadow page tables code works, and I hesitate whether I should do that now, when I know that common guest hypervisors work perfectly without fixing this issue, and when most people would rather use EPT and not shadow page tables anyway.

In any case, I left a TODO in the code about this, so it won't be forgotten.

Quoted 5 lines

I'm not sure what we need to do with vmcs that is not in RAM. It may simplify things to return the error_page to the caller and set KVM_REQ_TRIPLE_FAULT, so we don't have to deal with error handling later on.

This is a very good point. The approach in the patches I sent was to pin the L1-specified VMCS page and map it every time it was needed, and unpin and unmap it immediately afterward. This indeed will not work correctly if called with interrupts disabled and for some reason the vmcs12 page is swapped out...

I decided to reconsider this whole approach. In the new approach, we pin and kmap the guest vmcs12 page on VMPTRLD time (when sleeping is fine), and unpin and unmap it when a different VMPTRLD is done (or VMCLEAR, or cleanup). Whenever we want to use this vmcs12 in the code - we can do so without needing to swap in or map anything.

The code should now handle the rare situation when the vmcs12 gets swapped out, and is cleaner (with almost 100 lines of ugly nested_map_current()/ nested_unmap_current() calls were eliminated). The only downside I see is that now when nested vmx is being used, a single page, the current vmcs of L1, is always pinned and kmaped. I believe that pinning and mapping one single page (no matter how many guests there are) is a small price to pay - we already spend more than that in other places (e.g., one vmcs02 page per .

Does this sound reasonable to you?

Quoted 25 lines · Nadav Har'El

Quoted 5 lines

I'm not sure what we need to do with vmcs that is not in RAM. It may simplify things to return the error_page to the caller and set KVM_REQ_TRIPLE_FAULT, so we don't have to deal with error handling later on.

This is a very good point. The approach in the patches I sent was to pin the L1-specified VMCS page and map it every time it was needed, and unpin and unmap it immediately afterward. This indeed will not work correctly if called with interrupts disabled and for some reason the vmcs12 page is swapped out...

I decided to reconsider this whole approach. In the new approach, we pin and kmap the guest vmcs12 page on VMPTRLD time (when sleeping is fine), and unpin and unmap it when a different VMPTRLD is done (or VMCLEAR, or cleanup). Whenever we want to use this vmcs12 in the code - we can do so without needing to swap in or map anything.

The code should now handle the rare situation when the vmcs12 gets swapped out, and is cleaner (with almost 100 lines of ugly nested_map_current()/ nested_unmap_current() calls were eliminated). The only downside I see is that now when nested vmx is being used, a single page, the current vmcs of L1, is always pinned and kmaped. I believe that pinning and mapping one single page (no matter how many guests there are) is a small price to pay - we already spend more than that in other places (e.g., one vmcs02 page per .

Quoted 2 lines

Does this sound reasonable to you?

kmap() really should be avoided when possible. It is for when we don't have a pte pointing to the page (for example, accessing a user page from outside its process context).

Really, the correct API is kvm_read_guest() and kvm_write_guest(). They can easily be wrapped in with something that takes a vmcs12 field and automatically references the vmptr:

kvm_set_cr0(vcpu, gvmcs_read64(vcpu, guest_cr0));

This will take care of mark_page_dirty() etc.

kvm_*_guest() is a slow API since it needs to search the memory slot list but that can be optimized easily by caching the memory slot (and invalidating the cache when memory mapping changes). The optimized APIs can end up as doing a pointer fetch and a get/put_user(), which is very efficient.

Now the only problem is access from atomic contexts, as far as I can tell there are two areas where this is needed:

1) interrupt injection
2) optimized VMREAD/VMWRITE emulation

There are other reasons to move interrupt injection out of atomic context. If that's the only thing in the way of using kvm_read/write_guest(), I'll be happy to prioritize that work.

Alex, Joerg, well gvmcb_{read,write}{32,64}() work for nsvm? All that kmapping is incredibly annoying.

Quoted 3 lines

Alex, Joerg, well gvmcb_{read,write}{32,64}() work for nsvm? All that kmapping is incredibly annoying.

I'm sceptical that we can actually get that to be as fast as a direct kmap, but apart from that I don't see an obvious reason why it wouldn't.

kmap() really should be avoided when possible. It is for when we don't have a pte pointing to the page (for example, accessing a user page from outside its process context).

Really, the correct API is kvm_read_guest() and kvm_write_guest(). They can easily be wrapped in with something that takes a vmcs12 field and automatically references the vmptr:

  kvm_set_cr0(vcpu, gvmcs_read64(vcpu, guest_cr0));

I guess it's fine to start with the kmap() based implementation and change it later.

Note: you need to kunmap() and kvm_release_page_dirty() not only on vmxoff and vmptrld/vmclear, but also when ioctl(KVM_VCPU_RUN) exits to userspace. That ensures that live migration sees the page dirtied and is able to copy it.

On Wed, Jun 23, 2010, Avi Kivity wrote about "Re: [PATCH 5/24] Introduce vmcs12: a VMCS structure for L1":

Quoted 4 lines

kmap() really should be avoided when possible. It is for when we don't have a pte pointing to the page (for example, accessing a user page from outside its process context).

I'm afraid I do not follow. kmap() is needed for when you have 32-bit pointers but more than 4GB of RAM.

Why do we need to avoid using it?
I understand why one would like to avoid leaving many kmaped pages for long durations, but here we're talking about just one page (per vcpu).

Quoted 6 lines

Really, the correct API is kvm_read_guest() and kvm_write_guest(). They can easily be wrapped in with something that takes a vmcs12 field and automatically references the vmptr:

   kvm_set_cr0(vcpu, gvmcs_read64(vcpu, guest_cr0));

But as you also said, this doesn't solve the original problem which you found, (which was what happens when the vmcs12 page is swapped out and it is needed when sleeping is not allowed) unless we make changes to the injection logic, as you analyzed. I agree that we should consider it for a future fix.

If I understand the nested-SVM code correctly, they took a similar approach to mine - except they pin and unpin the page on every entry and exit, instead of on VMPTRLD (SVM doesn't have a notion of one current vmcb). But they still don't (if I understand correctly) call any special function on every access.

When the guest can use VMX instructions (when the "nested" module option is on), it should also be able to read and write VMX MSRs, e.g., to query about VMX capabilities. This patch adds this support.

Signed-off-by Nadav Har'El<[email protected]>

---

arch/x86/kvm/x86.c +0 -0

@@ -702,7 +702,11 @@ static u32 msrs_to_save[] = {
  #ifdef CONFIG_X86_64

arch/x86/kvm/x86.c +18 -0

  /*
+ * If we allow our guest to use VMX instructions, we should also let it use
+ * VMX-specific MSRs.
+ */
+static int nested_vmx_get_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata)
+{
+	u64 vmx_msr = 0;
+	u32 vmx_msr_high, vmx_msr_low;
+
+	switch (msr_index) {
+	case MSR_IA32_FEATURE_CONTROL:
+		*pdata = 0;
+		break;
+	case MSR_IA32_VMX_BASIC:
+		/*
+		 * This MSR reports some information about VMX support of the
+		 * processor. We should return information about the VMX we
+		 * emulate for the guest, and the VMCS structure we give it -
+		 * not about the VMX support of the underlying hardware. Some

Quoted 4 lines

When the guest can use VMX instructions (when the "nested" module option is on), it should also be able to read and write VMX MSRs, e.g., to query about
VMX capabilities. This patch adds this support.

...

Quoted 1 line

(quoted diff) +0 -0

@@ -702,7 +702,11 @@ static u32 msrs_to_save[] = {

...

arch/x86/kvm/x86.c static u32 msrs_to_save[] = { · line 706

Quoted 1 line

arch/x86/kvm/x86.c +1 -0

+	MSR_IA32_FEATURE_CONTROL,  MSR_IA32_VMX_BASIC,

...
These are read only from the guest point of view, but we need write support from the host to allow for tuning the features exposed to the guest.

On Mon, Jun 14, 2010, Avi Kivity wrote about "Re: [PATCH 6/24] Implement reading and writing of VMX MSRs":

Quoted 13 lines · Nadav Har'El

Quoted 6 lines

When the guest can use VMX instructions (when the "nested" module option is on), it should also be able to read and write VMX MSRs, e.g., to query about
VMX capabilities. This patch adds this support.

...

Quoted 2 lines

(quoted diff) +0 -0

@@ -702,7 +702,11 @@ static u32 msrs_to_save[] = {
 

...

Quoted 6 lines

arch/x86/kvm/x86.c static u32 msrs_to_save[] = { · line 706

Quoted 2 lines

arch/x86/kvm/x86.c +1 -0

+	MSR_IA32_FEATURE_CONTROL,  MSR_IA32_VMX_BASIC,

...
These are read only from the guest point of view, but we need write support from the host to allow for tuning the features exposed to the guest.

Hi,

I'm afraid I did not understand what you meant.
There is a KVM_SET_MSRS ioctl, but it appears to do the same thing that a guest's WRMSR would do, i.e., eventually call nested_vmx_set_msr(). In some of these MSRs (like VMX_BASIC) there's no point, if I understand correctly, for the guest or the host to change anything (e.g., what would it mean to change the VMCS length or revision id?). In others like VMX_PROCBASED_CTLS I guess that we can allow it to be set if it doesn't try to enable something not supported - is that what you had in mind? Or did you mean something else?

This patch includes a couple of utility functions for extracting pointer operands of VMX instructions issued by L1 (a guest hypervisor), and translating guest-given vmcs12 virtual addresses to guest-physical addresses.

arch/x86/kvm/vmx.c +16 -0

+/*
+ * Decode the memory-address operand of a vmx instruction, according to the
+ * Intel spec.
+ */
+#define VMX_OPERAND_SCALING(vii)	((vii)&  3)
+#define VMX_OPERAND_ADDR_SIZE(vii)	(((vii)>>  7)&  7)
+#define VMX_OPERAND_IS_REG(vii)		((vii)&  (1u<<  10))
+#define VMX_OPERAND_SEG_REG(vii)	(((vii)>>  15)&  7)
+#define VMX_OPERAND_INDEX_REG(vii)	(((vii)>>  18)&  0xf)
+#define VMX_OPERAND_INDEX_INVALID(vii)	((vii)&  (1u<<  22))
+#define VMX_OPERAND_BASE_REG(vii)	(((vii)>>  23)&  0xf)
+#define VMX_OPERAND_BASE_INVALID(vii)	((vii)&  (1u<<  27))
+#define VMX_OPERAND_REG(vii)		(((vii)>>  3)&  0xf)
+#define VMX_OPERAND_REG2(vii)		(((vii)>>  28)&  0xf)
+static gva_t get_vmx_mem_address(struct kvm_vcpu *vcpu,
+				 unsigned long exit_qualification,

arch/x86/kvm/vmx.c +15 -0

+	gva_t addr;
+
+	if (is_reg) {
+		kvm_queue_exception(vcpu, UD_VECTOR);
+		return 0;
+	}
+
+	switch (addr_size) {
+	case 1: /* 32 bit. high bits are undefined according to the spec: */
+		exit_qualification&= 0xffffffff;
+		break;
+	case 2: /* 64 bit */
+		break;
+	default: /* addr_size=0 means 16 bit */
+		return 0;

arch/x86/kvm/vmx.c +4 -0

+
+	return addr;
+}
+

Quoted 4 lines

This patch includes a couple of utility functions for extracting pointer operands of VMX instructions issued by L1 (a guest hypervisor), and translating guest-given vmcs12 virtual addresses to guest-physical addresses.

...

Quoted 1 line

+#define VMX_OPERAND_IS_REG(vii) ((vii)& (1u<< 10))

...
Since those defines are used just ones, you can fold them into their uses. It doesn't add much to repeat the variable name.

arch/x86/kvm/vmx.c static int handle_vmoff(struct kvm_vcpu · line 3707

Quoted 8 lines

arch/x86/kvm/vmx.c +7 -0

+	/* offfset = Base + [Index * Scale] + Displacement */
+	addr = vmx_get_segment_base(vcpu, seg_reg);
+	if (base_is_valid)
+		addr += kvm_register_read(vcpu, base_reg);
+	if (index_is_valid)
+		addr += kvm_register_read(vcpu, index_reg)<<scaling;
+	addr += exit_qualification; /* holds the displacement */

Do we need a segment limit and access rights check?

Quoted 11 lines

arch/x86/kvm/vmx.c static int handle_vmoff(struct kvm_vcpu · line 3707

Quoted 10 lines

arch/x86/kvm/vmx.c +7 -0

+	/* offfset = Base + [Index * Scale] + Displacement */
+	addr = vmx_get_segment_base(vcpu, seg_reg);
+	if (base_is_valid)
+		addr += kvm_register_read(vcpu, base_reg);
+	if (index_is_valid)
+		addr += kvm_register_read(vcpu, index_reg)<<scaling;
+	addr += exit_qualification; /* holds the displacement */

Do we need a segment limit and access rights check?

You are absolutely right. The instructions we're emulating (VMREAD, VMWRITE, VMPTRLD, etc.) should throw a #GP in a bunch of segmentation errors, including segment limit, execute-only segments, non-canonical 64-bit addresses, and a bunch of other unlikely error cases.

To achieve 100% accurate emulation in the error path, it will require quite a bit new code (here, and in many other places throughout the nested VMX code) that isn't necessary for running a correctly-written guest hypervisor (such as KVM or VMware). At worst, not accurately emulating the error path correctly might allow a broken L1 to do bad things to itself, but it doesn't allow it to do anything bad to L0 or other L1's.

Would you accept that I'll add a TODO in the code here (and in similar cases) and leave perfecting the error path to a later path?

This patch includes a couple of utility functions for extracting pointer operands of VMX instructions issued by L1 (a guest hypervisor), and translating guest-given vmcs12 virtual addresses to guest-physical addresses.

Signed-off-by Nadav Har'El <[email protected]>

---

arch/x86/kvm/x86.c +2 -1

@@ -3286,13 +3286,14 @@ static int kvm_fetch_guest_virt(gva_t ad
 					  access | PFERR_FETCH_MASK, error);
 }
 
-static int kvm_read_guest_virt(gva_t addr, void *val, unsigned int bytes,
+int kvm_read_guest_virt(gva_t addr, void *val, unsigned int bytes,
 			       struct kvm_vcpu *vcpu, u32 *error)
 {
 	u32 access = (kvm_x86_ops->get_cpl(vcpu) == 3) ? PFERR_USER_MASK : 0;
 	return kvm_read_guest_virt_helper(addr, val, bytes, vcpu, access,
 					  error);
 }
+EXPORT_SYMBOL_GPL(kvm_read_guest_virt);
 
 static int kvm_read_guest_virt_system(gva_t addr, void *val, unsigned int bytes,
 			       struct kvm_vcpu *vcpu, u32 *error)

arch/x86/kvm/x86.h +3 -0

@@ -75,6 +75,9 @@ static inline struct kvm_mem_aliases *kv
 void kvm_before_handle_nmi(struct kvm_vcpu *vcpu);
 void kvm_after_handle_nmi(struct kvm_vcpu *vcpu);
 
+int kvm_read_guest_virt(gva_t addr, void *val, unsigned int bytes,
+			struct kvm_vcpu *vcpu, u32 *error);
+
 extern int nested;
 
 #endif

arch/x86/kvm/vmx.c +21 -0

@@ -3654,6 +3654,86 @@ static int handle_vmoff(struct kvm_vcpu 
 	return 1;
 }
 
+/*
+ * Decode the memory-address operand of a vmx instruction, according to the
+ * Intel spec.
+ */
+#define VMX_OPERAND_SCALING(vii)	((vii) & 3)
+#define VMX_OPERAND_ADDR_SIZE(vii)	(((vii) >> 7) & 7)
+#define VMX_OPERAND_IS_REG(vii)		((vii) & (1u << 10))
+#define VMX_OPERAND_SEG_REG(vii)	(((vii) >> 15) & 7)
+#define VMX_OPERAND_INDEX_REG(vii)	(((vii) >> 18) & 0xf)
+#define VMX_OPERAND_INDEX_INVALID(vii)	((vii) & (1u << 22))
+#define VMX_OPERAND_BASE_REG(vii)	(((vii) >> 23) & 0xf)
+#define VMX_OPERAND_BASE_INVALID(vii)	((vii) & (1u << 27))
+#define VMX_OPERAND_REG(vii)		(((vii) >> 3) & 0xf)
+#define VMX_OPERAND_REG2(vii)		(((vii) >> 28) & 0xf)
+static gva_t get_vmx_mem_address(struct kvm_vcpu *vcpu,
+				 unsigned long exit_qualification,
+				 u32 vmx_instruction_info)
+{
+	int  scaling = VMX_OPERAND_SCALING(vmx_instruction_info);
+	int  addr_size = VMX_OPERAND_ADDR_SIZE(vmx_instruction_info);
+	bool is_reg = VMX_OPERAND_IS_REG(vmx_instruction_info);

arch/x86/kvm/vmx.c +49 -1

+	}
+
+	switch (addr_size) {
+	case 1: /* 32 bit. high bits are undefined according to the spec: */
+		exit_qualification &= 0xffffffff;
+		break;
+	case 2: /* 64 bit */
+		break;
+	default: /* addr_size=0 means 16 bit */
+		return 0;
+	}
+
+	/* Addr = segment_base + offset */
+	/* offfset = Base + [Index * Scale] + Displacement */
+	addr = vmx_get_segment_base(vcpu, seg_reg);
+	if (base_is_valid)
+		addr += kvm_register_read(vcpu, base_reg);
+	if (index_is_valid)
+		addr += kvm_register_read(vcpu, index_reg)<<scaling;
+	addr += exit_qualification; /* holds the displacement */
+
+	return addr;
+}
+
+static int read_guest_vmcs_gpa(struct kvm_vcpu *vcpu, gpa_t *gpap)
+{
+	int r;
+	gva_t gva = get_vmx_mem_address(vcpu,
+		vmcs_readl(EXIT_QUALIFICATION),
+		vmcs_read32(VMX_INSTRUCTION_INFO));
+	if (gva == 0)
+		return 1;
+	*gpap = 0;
+	r = kvm_read_guest_virt(gva, gpap, sizeof(*gpap), vcpu, NULL);
+	if (r) {
+		printk(KERN_ERR "%s cannot read guest vmcs addr %lx : %d\n",
+		       __func__, gva, r);
+		return r;
+	}
+	/* According to the spec, VMCS addresses must be 4K aligned */
+	if (!IS_ALIGNED(*gpap, PAGE_SIZE)) {
+		printk(KERN_DEBUG "%s addr %llx not aligned\n",
+		       __func__, *gpap);
+		return 1;
+	}
+
+	return 0;
+}
+
 static int handle_invlpg(struct kvm_vcpu *vcpu)
 {
 	unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
--

To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 4 lines

+/*
+ * Decode the memory-address operand of a vmx instruction, according to the
+ * Intel spec.
+ */

...

Quoted 4 lines

arch/x86/kvm/vmx.c +4 -0

+static gva_t get_vmx_mem_address(struct kvm_vcpu *vcpu,
+				 unsigned long exit_qualification,
+				 u32 vmx_instruction_info)
+{

...

arch/x86/kvm/vmx.c static int handle_vmoff(struct kvm_vcpu · line 3687

Quoted 3 lines

arch/x86/kvm/vmx.c +3 -0

+	if (is_reg) {
+		kvm_queue_exception(vcpu, UD_VECTOR);
+		return 0;

Isn't zero a legitimate address for vmx operation?

On Tue, Jun 15, 2010, Gleb Natapov wrote about "Re: [PATCH 7/24] Understanding guest pointers to vmcs12 structures":

Quoted 10 lines

Quoted 4 lines

+/*
+ * Decode the memory-address operand of a vmx instruction, according to the
+ * Intel spec.
+ */

...

Quoted 4 lines

arch/x86/kvm/vmx.c +4 -0

+static gva_t get_vmx_mem_address(struct kvm_vcpu *vcpu,
+				 unsigned long exit_qualification,
+				 u32 vmx_instruction_info)
+{

...

Quoted 4 lines

arch/x86/kvm/vmx.c static int handle_vmoff(struct kvm_vcpu · line 3687

Quoted 3 lines

arch/x86/kvm/vmx.c +3 -0

+	if (is_reg) {
+		kvm_queue_exception(vcpu, UD_VECTOR);
+		return 0;

Isn't zero a legitimate address for vmx operation?

Thanks. Please excuse my naivity, but is address 0 actually considered a usable guest virtual address? If it is, do we have any possible value which is considered invalid? Perhaps -1ull? I see that -1ull is used in a few places in vmx.c, for example.

If all gva_t turn out to actually be valid addresses, I'll need to move to a more complex (and uglier) success flag approach :(

Signature · 4 lines

Nadav Har'El                        |          Sunday, Aug  1 2010, 22 Av 5770
[email protected]             |-----------------------------------------
Phone +972-523-790466, ICQ 13349191 |The only "intuitive" interface is the
http://nadav.harel.org.il           |nipple. After that, it's all learned.

Guest can use any valid virtual address. There is UNMAPPED_GVA (~(gpa_t)0) which at least cannot be valid if address that your function returns have to be page aligned.

And not all virtual addresses are valid BTW. For 32 bit guest virt address cannot be bigger then 32 bit and for 64 bit guest virtual address should be in canonical form.

In this patch we add a list of L0 (hardware) VMCSs, which we'll use to hold a hardware VMCS for each active L1 VMCS (i.e., for each L2 guest).

We call each of these L0 VMCSs a "vmcs02", as it is the VMCS that L0 uses to run its nested guest L2.

arch/x86/kvm/vmx.c +6 -0

+
+/* Free the current L2 VMCS, and remove it from l2_vmcs_list */
+static void nested_free_current_vmcs(struct kvm_vcpu *vcpu)
+{
+	struct vcpu_vmx *vmx = to_vmx(vcpu);
+	struct vmcs_list *list_item, *n;

+/* Allocate an L0 VMCS (vmcs02) for the current L1 VMCS (vmcs12), if one + * does not already exist. The allocation is done in L0 memory, so to avoid + * denial-of-service attack by guests, we limit the number of concurrently- + * allocated vmcss. A well-behaving L1 will VMCLEAR unused vmcs12s and not + * trigger this limit.
+ */

arch/x86/kvm/vmx.c +8 -0

+static const int NESTED_MAX_VMCS = 256;
+static int nested_create_current_vmcs(struct kvm_vcpu *vcpu)
+{
+	struct vmcs_list *new_l2_guest;
+	struct vmcs *l2_vmcs;
+
+	if (nested_get_current_vmcs(vcpu))
+		return 0; /* nothing to do - we already have a VMCS */

arch/x86/kvm/vmx.c +8 -0

+	if (!l2_vmcs) {
+		kfree(new_l2_guest);
+		return -ENOMEM;
+	}
+
+	new_l2_guest->vmcs_addr = to_vmx(vcpu)->nested.current_vmptr;
+	new_l2_guest->l2_vmcs = l2_vmcs;
+	list_add(&(new_l2_guest->list),

&(to_vmx(vcpu)->nested.l2_vmcs_list));

arch/x86/kvm/vmx.c +3 -0

+	to_vmx(vcpu)->nested.l2_vmcs_num++; +	return 0;
+}
+

Quoted 2 lines

+/* Allocate an L0 VMCS (vmcs02) for the current L1 VMCS (vmcs12), if one + * does not already exist. The allocation is done in L0 memory,

...

Quoted 11 lines

arch/x86/kvm/vmx.c +11 -0

+static int nested_create_current_vmcs(struct kvm_vcpu *vcpu)
+{
+	struct vmcs_list *new_l2_guest;
+	struct vmcs *l2_vmcs;
+
+	if (nested_get_current_vmcs(vcpu))
+		return 0; /* nothing to do - we already have a VMCS */
+
+	if (to_vmx(vcpu)->nested.l2_vmcs_num >= NESTED_MAX_VMCS)
+		return -ENOMEM;
+

arch/x86/kvm/vmx.c static void free_vmcs(struct vmcs *vmcs) · line 1802

Quoted 6 lines

arch/x86/kvm/vmx.c +6 -0

+	new_l2_guest = (struct vmcs_list *)
+		kmalloc(sizeof(struct vmcs_list), GFP_KERNEL);
+	if (!new_l2_guest)
+		return -ENOMEM;
+
+	l2_vmcs = alloc_vmcs();

I didn't see where it was used. Hints on the usage?

Quoted 11 lines

arch/x86/kvm/vmx.c +8 -0

+	if (!l2_vmcs) {
+		kfree(new_l2_guest);
+		return -ENOMEM;
+	}
+
+	new_l2_guest->vmcs_addr = to_vmx(vcpu)->nested.current_vmptr;
+	new_l2_guest->l2_vmcs = l2_vmcs;
+	list_add(&(new_l2_guest->list),

&(to_vmx(vcpu)->nested.l2_vmcs_list));

(quoted diff) +2 -0

+	to_vmx(vcpu)->nested.l2_vmcs_num++; +	return 0;
+}

This patch implements the VMCLEAR instruction.

arch/x86/kvm/vmx.c +8 -0

+
+/* Emulate the VMCLEAR instruction */
+static int handle_vmclear(struct kvm_vcpu *vcpu)
+{
+	struct vcpu_vmx *vmx = to_vmx(vcpu);
+	gpa_t guest_vmcs_addr, save_current_vmptr;
+
+	if (!nested_vmx_check_permission(vcpu))

arch/x86/kvm/vmx.c +15 -0

+	vmx->nested.current_l2_page->launch_state = 0;
+	nested_unmap_current(vcpu);
+
+	nested_free_current_vmcs(vcpu);
+
+	if (save_current_vmptr == guest_vmcs_addr)
+		vmx->nested.current_vmptr = -1ull;
+	else
+		vmx->nested.current_vmptr = save_current_vmptr;
+
+	skip_emulated_instruction(vcpu);
+	clear_rflags_cf_zf(vcpu);
+	return 1;
+}
+

This patch implements the VMCLEAR instruction.

Signed-off-by Nadav Har'El <[email protected]>

---

arch/x86/kvm/vmx.c +32 -0

@@ -138,6 +138,8 @@ struct __attribute__ ((__packed__)) vmcs
 	 */
 	u32 revision_id;
 	u32 abort;
+
+	bool launch_state; /* set to 0 by VMCLEAR, to 1 by VMLAUNCH */
 };
 
 struct vmcs_list {

@@ -3827,6 +3829,46 @@ static int read_guest_vmcs_gpa(struct kv
 	return 0;
 }
 
+static void clear_rflags_cf_zf(struct kvm_vcpu *vcpu)
+{
+	unsigned long rflags;
+	rflags = vmx_get_rflags(vcpu);
+	rflags &= ~(X86_EFLAGS_CF | X86_EFLAGS_ZF);
+	vmx_set_rflags(vcpu, rflags);
+}
+
+/* Emulate the VMCLEAR instruction */
+static int handle_vmclear(struct kvm_vcpu *vcpu)
+{
+	struct vcpu_vmx *vmx = to_vmx(vcpu);
+	gpa_t guest_vmcs_addr, save_current_vmptr;
+
+	if (!nested_vmx_check_permission(vcpu))
+		return 1;
+
+	if (read_guest_vmcs_gpa(vcpu, &guest_vmcs_addr))
+		return 1;
+
+	save_current_vmptr = vmx->nested.current_vmptr;
+
+	vmx->nested.current_vmptr = guest_vmcs_addr;
+	if (!nested_map_current(vcpu))
+		return 1;
+	vmx->nested.current_l2_page->launch_state = 0;
+	nested_unmap_current(vcpu);
+
+	nested_free_current_vmcs(vcpu);
+

arch/x86/kvm/vmx.c +1 -2

 static int handle_invlpg(struct kvm_vcpu *vcpu)
 {
 	unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION);

@@ -4109,7 +4151,7 @@ static int (*kvm_vmx_exit_handlers[])(st
 	[EXIT_REASON_HLT]                     = handle_halt,
 	[EXIT_REASON_INVLPG]		      = handle_invlpg,
 	[EXIT_REASON_VMCALL]                  = handle_vmcall,
-	[EXIT_REASON_VMCLEAR]	              = handle_vmx_insn,
+	[EXIT_REASON_VMCLEAR]	              = handle_vmclear,
 	[EXIT_REASON_VMLAUNCH]                = handle_vmx_insn,
 	[EXIT_REASON_VMPTRLD]                 = handle_vmx_insn,
 	[EXIT_REASON_VMPTRST]                 = handle_vmx_insn,
--

To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 53 lines · Nadav Har'El

This patch implements the VMCLEAR instruction.

Signed-off-by Nadav Har'El<[email protected]>

---

arch/x86/kvm/vmx.c +36 -0

@@ -138,6 +138,8 @@ struct __attribute__ ((__packed__)) vmcs
  	 */
  	u32 revision_id;
  	u32 abort;
+
+	bool launch_state; /* set to 0 by VMCLEAR, to 1 by VMLAUNCH */
  };
 
  struct vmcs_list {

@@ -3827,6 +3829,46 @@ static int read_guest_vmcs_gpa(struct kv
  	return 0;
  }
 
+static void clear_rflags_cf_zf(struct kvm_vcpu *vcpu)
+{
+	unsigned long rflags;
+	rflags = vmx_get_rflags(vcpu);
+	rflags&= ~(X86_EFLAGS_CF | X86_EFLAGS_ZF);
+	vmx_set_rflags(vcpu, rflags);
+}
+
+/* Emulate the VMCLEAR instruction */
+static int handle_vmclear(struct kvm_vcpu *vcpu)
+{
+	struct vcpu_vmx *vmx = to_vmx(vcpu);
+	gpa_t guest_vmcs_addr, save_current_vmptr;
+
+	if (!nested_vmx_check_permission(vcpu))
+		return 1;
+
+	if (read_guest_vmcs_gpa(vcpu,&guest_vmcs_addr))
+		return 1;
+
+	save_current_vmptr = vmx->nested.current_vmptr;
+
+	vmx->nested.current_vmptr = guest_vmcs_addr;
+	if (!nested_map_current(vcpu))
+		return 1;
+	vmx->nested.current_l2_page->launch_state = 0;
+	nested_unmap_current(vcpu);
+
+	nested_free_current_vmcs(vcpu);
+
+	if (save_current_vmptr == guest_vmcs_addr)
+		vmx->nested.current_vmptr = -1ull;
+	else
+		vmx->nested.current_vmptr = save_current_vmptr;

arch/x86/kvm/vmx.c static int read_guest_vmcs_gpa(struct kv

Quoted 7 lines

arch/x86/kvm/vmx.c +6 -0

+
+	skip_emulated_instruction(vcpu);
+	clear_rflags_cf_zf(vcpu);
+	return 1;
+}
+
 

Shouldn't error cases update flags too?

Quoted 53 lines · Gleb Natapov

Quoted 52 lines · Nadav Har'El

This patch implements the VMCLEAR instruction.

Signed-off-by Nadav Har'El<[email protected]>

---

arch/x86/kvm/vmx.c +35 -0

@@ -138,6 +138,8 @@ struct __attribute__ ((__packed__)) vmcs
 	 */
 	u32 revision_id;
 	u32 abort;
+
+	bool launch_state; /* set to 0 by VMCLEAR, to 1 by VMLAUNCH */
 };
 
 struct vmcs_list {

@@ -3827,6 +3829,46 @@ static int read_guest_vmcs_gpa(struct kv
 	return 0;
 }
 
+static void clear_rflags_cf_zf(struct kvm_vcpu *vcpu)
+{
+	unsigned long rflags;
+	rflags = vmx_get_rflags(vcpu);
+	rflags&= ~(X86_EFLAGS_CF | X86_EFLAGS_ZF);
+	vmx_set_rflags(vcpu, rflags);
+}
+
+/* Emulate the VMCLEAR instruction */
+static int handle_vmclear(struct kvm_vcpu *vcpu)
+{
+	struct vcpu_vmx *vmx = to_vmx(vcpu);
+	gpa_t guest_vmcs_addr, save_current_vmptr;
+
+	if (!nested_vmx_check_permission(vcpu))
+		return 1;
+
+	if (read_guest_vmcs_gpa(vcpu,&guest_vmcs_addr))
+		return 1;
+
+	save_current_vmptr = vmx->nested.current_vmptr;
+
+	vmx->nested.current_vmptr = guest_vmcs_addr;
+	if (!nested_map_current(vcpu))
+		return 1;
+	vmx->nested.current_l2_page->launch_state = 0;
+	nested_unmap_current(vcpu);
+
+	nested_free_current_vmcs(vcpu);
+
+	if (save_current_vmptr == guest_vmcs_addr)
+		vmx->nested.current_vmptr = -1ull;
+	else

Quoted 8 lines

arch/x86/kvm/vmx.c static int read_guest_vmcs_gpa(struct kv · line 3871

Quoted 7 lines

arch/x86/kvm/vmx.c +7 -0

+		vmx->nested.current_vmptr = save_current_vmptr;
+
+	skip_emulated_instruction(vcpu);
+	clear_rflags_cf_zf(vcpu);
+	return 1;
+}
+

Shouldn't error cases update flags too?

Architectural errors (bad alignment) should update flags. Internal errors (ENOMEM, vpmtr pointing outside of RAM) should not.

Quoted 4 lines · Gleb Natapov

Architectural errors (bad alignment) should update flags. Internal errors (ENOMEM, vpmtr pointing outside of RAM) should not.

vpmtr pointing outside of RAM is architectural error (or not?). SDM says "The operand of this instruction is always 64 bits and is always in memory", but may be they mean "not in register". Anyway internal errors should generate error exit to userspace which this patch is also missing.

On Tue, Jun 15, 2010, Gleb Natapov wrote about "Re: [PATCH 9/24] Implement VMCLEAR":

Quoted 10 lines · Avi Kivity

Quoted 4 lines · Gleb Natapov

Architectural errors (bad alignment) should update flags. Internal errors (ENOMEM, vpmtr pointing outside of RAM) should not.

vpmtr pointing outside of RAM is architectural error (or not?). SDM says "The operand of this instruction is always 64 bits and is always in memory", but may be they mean "not in register". Anyway internal errors should generate error exit to userspace which this patch is also missing.

I'm a bit puzzled what I am supposed to do when the guest-physical address I get as a parameter to VMCLEAR (after I read this address from guest virtual memory) is beyond the guest's actual memory, i.e., gfn_to_page fails on this address. Is this a normal "architectural error" and I should VMfail(VMCLEAR with invalid physical address)? #GP? Or something else? The SMD says
"ensure that data for VMCS referenced by the operand is in memory"
but it doesn't appear to say what to do if that is not the case. When the address itself is faulty (e.g., more than 32 bits in 32 bit mode) the SDM says VMfail(VMCLEAR with invalid physical address) - but it doesn't say to do that when the physical address is "simply" beyond the amount of available memory.

In any case, I don't think this should be considered an internal error, or that we have a reason to exit to user space in this case.

Quoted 3 lines

In any case, I don't think this should be considered an internal error, or that we have a reason to exit to user space in this case.

But you can't emulate this either, no?

On Thu, Aug 05, 2010, Gleb Natapov wrote about "Re: [PATCH 9/24] Implement VMCLEAR":

Quoted 4 lines

Quoted 3 lines

In any case, I don't think this should be considered an internal error, or that we have a reason to exit to user space in this case.

But you can't emulate this either, no?

I could, if I knew what to emulate ;-) Does anybody know what a real processor with VMX does when you give it VMCLEAR with a physical address which is beyond the amount of available memory? If the answer was "it throws #GP" or "it does VMFail(Invalid Physical Address) or something of this sort, I could easily do this in the emulation too - I just don't know yet what it does...

As far as the processor is concerned, there is no end to physical memory. The VMCLEAR will write some stuff out, and the chipset will throw it away.

However, eventually the guest will crash and burn,
better to take it out in a controlled way.

On Thu, Aug 05, 2010, Avi Kivity wrote about "Re: [PATCH 9/24] Implement VMCLEAR":

Quoted 3 lines

As far as the processor is concerned, there is no end to physical memory. The VMCLEAR will write some stuff out, and the chipset will throw it away.

So isn't what you're saying that for accurate VMCLEAR emulation, I should simply ignore this error and have VMCLEAR do nothing in this case?

Quoted 2 lines

However, eventually the guest will crash and burn,
better to take it out in a controlled way.

Isn't it concievable that some poorly-written hypervisor actually VMCLEARs something beyond available memory, and doesn't crash because it never tries to actually use that memory later?

Do you think it's a good idea to kill
the guest immediately, in anticipation of a crash that will most likely come later?

Quoted 4 lines

Do you think it's a good idea to kill
the guest immediately, in anticipation of a crash that will most likely come
later?

At this stage, the problem is likely to be in the nvmx implementation. So it's best to report the problem as early as possible.