Re: [PATCH 6/6] KVM: VMX: Add PML support in VMX

[Kai Huang <kai.huang@linux.intel.com>]

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;