From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH v2 4/5] kvm: x86: mmu: Lockless access tracking for Intel CPUs without EPT A bits. Date: Tue, 29 Nov 2016 03:09:31 -0500 (EST) Message-ID: <634179352.518636.1480406971725.JavaMail.zimbra@redhat.com> References: <41116680.VFaKihePAT@js-desktop.mtv.corp.google.com> <1857180.zXFPSRIdpL@js-desktop.mtv.corp.google.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, andreslc@google.com, pfeiner@google.com, guangrong xiao To: Junaid Shahid Return-path: Received: from mx4-phx2.redhat.com ([209.132.183.25]:60020 "EHLO mx4-phx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756132AbcK2IJe (ORCPT ); Tue, 29 Nov 2016 03:09:34 -0500 In-Reply-To: <1857180.zXFPSRIdpL@js-desktop.mtv.corp.google.com> Sender: kvm-owner@vger.kernel.org List-ID: > > >> Let's separate the three conditions (R/W/X): > > >> > > >> if ((error_code & PFERR_FETCH_MASK) { > > >> if ((spte & (shadow_x_mask|shadow_nx_mask)) > > >> == shadow_x_mask) { > > >> fault_handled = true; > > >> break; > > >> } > > >> } > > >> if (error_code & PFERR_WRITE_MASK) { > > >> if (is_writable_pte(spte)) { > > >> fault_handled = true; > > >> break; > > >> } > > >> remove_write_prot = > > >> spte_can_locklessly_be_made_writable(spte); > > >> } > > >> if (!(error_code & PFERR_PRESENT_MASK)) { > > >> if (!is_access_track_spte(spte)) { > > >> fault_handled = true; > > >> break; > > >> } > > >> remove_acc_track = true; > > >> } > > > > > > I think the third block is incorrect e.g. it will set fault_handled = > > > true even > > > for a completely zero PTE. > > > > A completely zero PTE would have been filtered before by the > > is_shadow_present_pte check, wouldn't it? > > Oh, the is_shadow_present_pte check was actually removed in the patch. We could > add it back, minus the ret = true statement, and then it would filter the zero > PTE case. But I still think that the other form: > > if ((error_code & PFERR_USER_MASK) && > (spte & PT_PRESENT_MASK)) { > fault_handled = true; > break; > } > > is simpler as it is directly analogous to the cases for fetch and write. > Please let me know if you think otherwise. Fair enough, but add a comment to explain the error_code check because I don't get it. :) Paolo