From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kai Huang Subject: Re: [PATCH 6/6] KVM: VMX: Add PML support in VMX Date: Fri, 06 Feb 2015 08:28:06 +0800 Message-ID: <54D40A96.7020907@linux.intel.com> References: <1422413668-3509-1-git-send-email-kai.huang@linux.intel.com> <1422413668-3509-7-git-send-email-kai.huang@linux.intel.com> <20150203151833.GD19731@potion.redhat.com> <54D30C5D.1000402@linux.intel.com> <20150205150414.GA8786@potion.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: pbonzini@redhat.com, gleb@kernel.org, linux@arm.linux.org.uk, kvm@vger.kernel.org To: =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= Return-path: Received: from mga09.intel.com ([134.134.136.24]:36071 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754622AbbBFAgY (ORCPT ); Thu, 5 Feb 2015 19:36:24 -0500 In-Reply-To: <20150205150414.GA8786@potion.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 02/05/2015 11:04 PM, Radim Kr=C4=8Dm=C3=A1=C5=99 wrote: > 2015-02-05 14:23+0800, Kai Huang: >> On 02/03/2015 11:18 PM, Radim Kr=C4=8Dm=C3=A1=C5=99 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 f= ix 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, So far I see no other bug reported :) Thanks, -Kai > > Thanks. > >>>> + exec_control &=3D ~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_exe= c_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 &=3D ~SECONDARY_EXEC_ENABLE_PML > > here and no exec_control operations in vmx_create_vcpu(). > >> so it's better to enable it alo= ng 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_MA= SK) && >>>> + 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 (Informati= on >>> for VM Exits Due to Vectored Events), which makes me think that thi= s 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 nee= ded >>> 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 def= inition >> of bit 12 of exit_qualification (section 1.5), which explains above = code. >> The same definition of exit_qualification is in EPT violation part i= n >> Intel's SDM. > True ... IDT-vectoring doesn't set bit 12 because PML exit isn't a fa= ult > and 27.2.2: "=E2=80=94 For all other relevant VM exits, bit 12 is cle= ared to 0." > (This was humourously pasted to PML as well.) > >>>> + /* PML index always points to next available PML buffer entity *= / >>>> + if (pml_idx >=3D PML_ENTITY_NUM) >>>> + pml_idx =3D 0; >>>> + else >>>> + pml_idx++; >>> If the pml_idx is >=3D PML_ENTITY_NUM and < 0xffff, the log is empt= y, >> In this case, the log is full, actually. PML index is set to PML_ENT= ITY_NUM >> - 1 initially, and hardware decrease it after GPA is logged. >> >> Below is the pseudocode of PML hardware logic. >> >> IF (PML Index[15:9] =E2=89=A0 0) >> THEN VM exit; >> FI; > pml_idx >=3D 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] =E2=86=90 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 =3D> we shouldn't read it. > (I should have said "untouched" instead of "empty".) > >> FI;