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@xxxxxxxxxxxxxxxxxxx |----------------------------------------- Phone +972-523-790466, ICQ 13349191 |An Apple a day, keeps Windows away. http://nadav.harel.org.il | -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
