On 09/12/2010 04:05 PM, Nadav Har'El wrote:
Hi,
Continuing to work on the nested VMX patches,
Great. Hopefully you can commit some time to it or you'll spend a lot
of cycles just catching up.
Joerg just merged nested npt; as far as I can tell it is 100% in line
with nested ept, but please take a look as well.
+ 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.
IIRC we don't support msr bitmaps now, so no need to check anything.
In general, avoid vmcs reads as much as possible. Just think of your
code running in a guest!
+ 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 it changes in the future, it can only be under the influence of some
control or at least guest-discoverable capability. Since we don't
expose such control or capability, there's no need to copy it.
+ 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.
I don't think so. We write this field as part of guest entry (that is,
after the decision re which vmcs to use, yes?), so guest entry will
always follow writing this field. Since guest entry clears the field,
reading it after an exit will necessarily return 0.
What can happen is that the contents of the field is transferred to the
IDT_VECTORING_INFO field or VM_EXIT_INTR_INFO field.
(question: on a failed vmentry, is this field cleared?)
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;
That's really a temporary variable, I don't think you need to touch it.
+ 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).
Ok. That answers my concern.
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...
No, I wasn't worried about that, simply misunderstood the code.
+ /* 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.
We give the guest partial control over cr4:
#define KVM_CR4_GUEST_OWNED_BITS \
(X86_CR4_PVI | X86_CR4_DE | X86_CR4_PCE |
X86_CR4_OSFXSR \
| X86_CR4_OSXMMEXCPT)
Plus PGE if EPT is enabled.
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.
I didn't mean register independent helper; one function for cr0 and one
function for cr4 so the reader can just see the name and pretend to
understand what it does, instead of seeing a bunch of incomprehensible
bitwise operations.
(well, reading what I wrote, maybe I did mean a cr independent helper,
but don't do it if it results in more complication)
--
error compiling committee.c: too many arguments to function
--
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
