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

["Radim Krčmář" <rkrcmar@redhat.com>]

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().

>                                     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.

> >>+	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,

> 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".)

> FI;