Re: [PATCH 18/24] Exiting from L2 to L1

["Nadav Har'El" <nyh@math.technion.ac.il>]

Hi,

Continuing to work on the nested VMX patches,

On Mon, Jun 14, 2010, Avi Kivity wrote about "Re: [PATCH 18/24] Exiting from L2 to L1":
> On 06/13/2010 03:31 PM, Nadav Har'El wrote:
>...
> >+/* prepare_vmcs_12 is called when the nested L2 guest exits and we want to
> >+ * prepare to run its L1 parent. L1 keeps a vmcs for L2 (vmcs12), and this
> >+ * function updates it to reflect the state of the registers during the 
> >exit,
>...
> >+	vmcs12->tsc_offset = vmcs_read64(TSC_OFFSET);
> >   
> 
> TSC_OFFSET cannot have changed.

Right. I cleaned up this function now, to only copy the fields that could
have changed, namely fields listed as guest-state or exit-information fields
in the spec. Control fields like this TSC_OFFSET and more examples you found
below, indeed could not have changed while L2 was running or during the exit.

> >+	vmcs12->guest_ia32_debugctl = vmcs_read64(GUEST_IA32_DEBUGCTL);
> >   
> Without msr bitmaps, cannot change.

I added a TODO before this (and a couple of others) for future optimization.
I'm not even convinced how much quicker it is to check the MSR bitmap before
doing vmcs_read64, vs just to going ahead and vmreading it in any case.

> >+    vmcs12->vmcs_link_pointer = vmcs_read64(VMCS_LINK_POINTER);
>
> Can this change?

Well, according to the spec, (SDM vol 3B), VMCS link pointer is a guest-state
field, but it is listed as being for "future expansion". I guess that with
current hardware, it cannot change, but for future hardware it might. I'm
not sure if it's wiser to ignore this field for now (and shave a bit off
the l2->l1 switch time), or just copy it anyway, as I do now.
What would you prefer?

> >+	if (vmcs_config.vmentry_ctrl&  VM_ENTRY_LOAD_IA32_PAT)
> >+		vmcs12->guest_ia32_pat = vmcs_read64(GUEST_IA32_PAT);
> >   
> 
> Should check for VM_EXIT_SAVE_IA32_PAT, no?

You're absolutely right. Fixed.

> >+	vmcs12->vm_entry_intr_info_field =
> >+		vmcs_read32(VM_ENTRY_INTR_INFO_FIELD);
> >   
> 
> Autocleared, no need to read.

Well, we need to clear the "valid bit" on exit, so we don't mistakenly inject
the same interrupt twice. There were two ways to do it: 1. clear it ourselves,
or 2. copy the value from vmcs02 where the processor already cleared it.
There are pros and cons for each approach, but I'll change like you suggest,
to clear it ourselves:

	vmcs12->vm_entry_intr_info_field &= ~INTR_INFO_VALID_MASK;

> >+	vmcs12->vm_instruction_error = vmcs_read32(VM_INSTRUCTION_ERROR);
>
> We don't want to pass this to the guest?

I didn't quite understand your question, but now that I look at it, I see
that VM_INSTRUCTION_ERROR has nothing to do with exits, but is only modified
when running VMX instructions, and our emulation of VMX instructions already
sets it appropriately, so no sense of copying it here.

> 
> >+	vmcs12->vm_exit_reason  = vmcs_read32(VM_EXIT_REASON);
> >+	vmcs12->vm_exit_intr_info = vmcs_read32(VM_EXIT_INTR_INFO);
> >+	vmcs12->vm_exit_intr_error_code = 
> >vmcs_read32(VM_EXIT_INTR_ERROR_CODE);
> >+	vmcs12->idt_vectoring_info_field =
> >+		vmcs_read32(IDT_VECTORING_INFO_FIELD);
> >+	vmcs12->idt_vectoring_error_code =
> >+		vmcs_read32(IDT_VECTORING_ERROR_CODE);
> >+	vmcs12->vm_exit_instruction_len = 
> >vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
> >+	vmcs12->vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO);
> >   
> 
> For the above, if the host handles the exit, we must not clobber guest 
> fields.  A subsequent guest vmread will see the changed values even 
> though from its point of view a vmexit has not occurred.
> 
> But no, that can't happen, since a vmread needs to have a vmexit first 
> to happen.  Still, best to delay this.+

All this code is in prepare_vmcs_12, which only gets called if we exit from
L2 to L1 - it doesn't get called when we exit from L2 to L0 (when the host
handles the exit).

As long as a certain field gets written to on *every* exit, and not just
on some of them, I believe we can safely copy the current values in vmcs02
to vmcs12, knowing these are current values from the current exit, and not
some old values we shouldn't be copying.

You may have a point (if that was your point?) that some fields are not
always written to - e.g., for most exits vm_exit_intr_info doesn't get
written to and just one "valid" bit is cleared. As the code is now, we copy
vmcs02's field, which might have been written earlier (e.g., during
an exit to L0) and not now, and an observant L1 might notice this value
it should not have seen. However, I don't see any problem with that, because
the "valid" bit would be correctly turned off, and the spec says that all
other bits are undefined (for irrelevant exits) and copying-old-values is one
legal setting for undefined bits...

> >+	/* If any of the CRO_GUEST_HOST_MASK bits are off, the L2 guest may
> >+	 * have changed some cr0 bits without us ever saving them in the 
> >shadow
> >+	 * vmcs. So we need to save these changes now.
>...
> >+
> >+	vmcs12->guest_cr4 = vmcs_readl(GUEST_CR4);
> 
> Can't we have the same issue with cr4?

I guess we can. I didn't think this (giving guest full control over cr4 bits)
was happening in KVM, but maybe I was wrong, or maybe this will happen in the
future, so no reason not to do it for cr4 as well. So I'll do it for cr4 as
well.

> Better to have some helpers to do the common magic, and not encode the 
> special knowledge about TS into it (make it generic).

I thought that since in current KVM code the only cr0_guest_owned_bits bit
that could possibly be turned on was TS, then I should only deal with that
bit. But you're right, no reason not to make it more general, to look for
any bits which L0 traps but L1 didn't think it was trapping. As in:

	long bits;
	bits = vcpu->arch.cr0_guest_owned_bits | vmcs12->cr0_guest_host_mask;
	vmcs12->guest_cr0 = (vmcs_readl(GUEST_CR0) & bits) |
				(vmcs_readl(CR0_READ_SHADOW) & ~bits);

(the "bits" lists all the bits which are already fine in guest_cr0, i.e.,
either guest_owned_bit (so L2 wrote to it directly) or guest_host_mask
(so L1 didn't expect them to be updated in guest_cr0 anyway). All other
bits need to be copied from the read_shadow).

I don't know how to put this into a helper function, because these two
statements have so many dependencies on the word "cr0" that making one
that would work for either cr0 or cr4 seems too difficult to be worth the
trouble.

This reply is getting long, so I'll leave it about prepare_vmcs_12 and
will reply to your comments about the rest of this patch in a separate mail.

Thanks,
Nadav.

-- 
Nadav Har'El                        |       Sunday, Sep 12 2010, 4 Tishri 5771
nyh@math.technion.ac.il             |-----------------------------------------
Phone +972-523-790466, ICQ 13349191 |An Apple a day, keeps Windows away.
http://nadav.harel.org.il           |