On 02/05/2015 11:04 PM, Radim Krčmář wrote:
2015-02-05 14:23+0800, Kai Huang:
On 02/03/2015 11:18 PM, Radim Krčmář wrote:
You have it protected by CONFIG_X86_64, but use it unconditionally.
Thanks for catching. This has been fixed by another patch, and the fix has
also been merged by Paolo.
(And I haven't noticed the followup either ...)
I don't know of any present bugs in this patch and all questions have
been cleared,
Thanks.
+ exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
What is the harm of enabling it here?
(SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES seems similar and does it.)
Because the PML feature detection is unconditional (meaning
SECONDARY_EXEC_ENABLE_PML is always in vmcs_config.cpu_based_2nd_exec_ctrl),
but the PML buffer is only created when vcpu is created, and it is
controlled by 'enable_pml' module parameter, if we always enable
SECONDARY_EXEC_ENABLE_PML here, no PML buffer will be created if PML is
disabled by 'enable_pml' parameter,
I meant
if (!enable_pml)
exec_control &= ~SECONDARY_EXEC_ENABLE_PML
here and no exec_control operations in vmx_create_vcpu().
This also works. I originally thought put the enabling part and buffer
allocation together is more straightforward.
so it's better to enable it along with
creating PML buffer.
I think the reason why KVM split the setup into vmx_create_vcpu() and
vmx_secondary_exec_control() is to simplify nested virtualization.
This is a good point and I'll think about it :)
+ if (!(to_vmx(vcpu)->idt_vectoring_info & VECTORING_INFO_VALID_MASK) &&
+ cpu_has_virtual_nmis() &&
+ (exit_qualification & INTR_INFO_UNBLOCK_NMI))
+ vmcs_set_bits(GUEST_INTERRUPTIBILITY_INFO,
+ GUEST_INTR_STATE_NMI);
Relevant part of the specification is pasted from 27.2.2 (Information
for VM Exits Due to Vectored Events), which makes me think that this bit
is mirrored to IDT-vectoring information field ...
Isn't vmx_recover_nmi_blocking() enough?
(I see the same code in handle_ept_violation(), but wasn't that needed
just because of a hardware error?)
This needs to be handled in both EPT violation and PML. If you look at the
PML specification (the link is in cover letter), you can see the definition
of bit 12 of exit_qualification (section 1.5), which explains above code.
The same definition of exit_qualification is in EPT violation part in
Intel's SDM.
True ... IDT-vectoring doesn't set bit 12 because PML exit isn't a fault
and 27.2.2: "— For all other relevant VM exits, bit 12 is cleared to 0."
(This was humourously pasted to PML as well.)
+ /* PML index always points to next available PML buffer entity */
+ if (pml_idx >= PML_ENTITY_NUM)
+ pml_idx = 0;
+ else
+ pml_idx++;
If the pml_idx is >= PML_ENTITY_NUM and < 0xffff, the log is empty,
In this case, the log is full, actually. PML index is set to PML_ENTITY_NUM
- 1 initially, and hardware decrease it after GPA is logged.
Below is the pseudocode of PML hardware logic.
IF (PML Index[15:9] ≠ 0)
THEN VM exit;
FI;
pml_idx >= PML_ENTITY_NUM exits without modifying the buffer,
Yes.
set accessed and dirty flags for EPT;
IF (a dirty flag was updated from 0 to 1)
THEN
PML address[PML index] ← 4-KByte-aligned guest-physical address;
PML index is decremented;
0xffff is the only value that specifies full buffer, the rest means
that we incorrectly set up the initial index and VMX exited without
changing the buffer => we shouldn't read it.
(I should have said "untouched" instead of "empty".)
Yes theoretically, according to the pseudocode, 0xffff is the only
possible value that specifies full buffer. Probably a better way is to
check if pml_idx is in range [-1, 511] at the beginning and give a
warning if it's not, or just ASSERT it. Then we can simply to do a
++pml_idx (with changing pml_idx to signed short type). But the current
code should also work anyway.
Thanks,
-Kai
FI;
--
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
