From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radim =?utf-8?B?S3LEjW3DocWZ?= Subject: Re: [PATCH 6/6] KVM: VMX: Add PML support in VMX Date: Thu, 5 Feb 2015 16:04:15 +0100 Message-ID: <20150205150414.GA8786@potion.redhat.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> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: pbonzini@redhat.com, gleb@kernel.org, linux@arm.linux.org.uk, kvm@vger.kernel.org To: Kai Huang Return-path: Received: from mx1.redhat.com ([209.132.183.28]:33459 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751439AbbBEPEa (ORCPT ); Thu, 5 Feb 2015 10:04:30 -0500 Content-Disposition: inline In-Reply-To: <54D30C5D.1000402@linux.intel.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 fi= x 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 &=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_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 &=3D ~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_MAS= K) && > >>+ 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 (Informatio= n > >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 need= ed > > just because of a hardware error?) > This needs to be handled in both EPT violation and PML. If you look a= t the > PML specification (the link is in cover letter), you can see the defi= nition > of bit 12 of exit_qualification (section 1.5), which explains above c= ode. > 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 faul= t and 27.2.2: "=E2=80=94 For all other relevant VM exits, bit 12 is clear= ed 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 empty= , > In this case, the log is full, actually. PML index is set to PML_ENTI= TY_NUM > - 1 initially, and hardware decrease it after GPA is logged. >=20 > Below is the pseudocode of PML hardware logic. >=20 > 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 a= ddress; > 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;