From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paul Mackerras Date: Thu, 10 Oct 2013 10:33:02 +0000 Subject: Re: [PATCH 3/4] kvm: powerpc: define a linux pte lookup function Message-Id: <20131010103302.GC9906@iris.ozlabs.ibm.com> List-Id: References: <1381212212-29641-1-git-send-email-Bharat.Bhushan@freescale.com> <1381212212-29641-4-git-send-email-Bharat.Bhushan@freescale.com> In-Reply-To: <1381212212-29641-4-git-send-email-Bharat.Bhushan@freescale.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Bharat Bhushan Cc: agraf@suse.de, scottwood@freescale.com, stuart.yoder@freescale.com, kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, Bharat Bhushan On Tue, Oct 08, 2013 at 11:33:31AM +0530, Bharat Bhushan wrote: > We need to search linux "pte" to get "pte" attributes for > setting TLB in KVM. > This patch defines a linux_pte_lookup() function for same. > [snip] > + /* wait until _PAGE_BUSY is clear */ > + while (1) { > + pte = pte_val(*ptep); > + if (unlikely(pte & _PAGE_BUSY)) { > + cpu_relax(); > + continue; > + } > + } > + > + /* If pte is not present return None */ > + if (unlikely(!(pte & _PAGE_PRESENT))) > + return __pte(0); First, this looks racy to me, since nothing stops the compiler refetching pte from memory, and it might have become busy again in the meantime. However, I don't think you need to do any of this, since the caller in your next patch checks for pte_present(pte) anyway. On book E systems we don't use _PAGE_BUSY, so you don't need the loop for your intended application. I suggest you remove the entire section quoted above. Paul. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paul Mackerras Subject: Re: [PATCH 3/4] kvm: powerpc: define a linux pte lookup function Date: Thu, 10 Oct 2013 21:33:02 +1100 Message-ID: <20131010103302.GC9906@iris.ozlabs.ibm.com> References: <1381212212-29641-1-git-send-email-Bharat.Bhushan@freescale.com> <1381212212-29641-4-git-send-email-Bharat.Bhushan@freescale.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: agraf@suse.de, scottwood@freescale.com, stuart.yoder@freescale.com, kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, Bharat Bhushan To: Bharat Bhushan Return-path: Content-Disposition: inline In-Reply-To: <1381212212-29641-4-git-send-email-Bharat.Bhushan@freescale.com> Sender: kvm-ppc-owner@vger.kernel.org List-Id: kvm.vger.kernel.org On Tue, Oct 08, 2013 at 11:33:31AM +0530, Bharat Bhushan wrote: > We need to search linux "pte" to get "pte" attributes for > setting TLB in KVM. > This patch defines a linux_pte_lookup() function for same. > [snip] > + /* wait until _PAGE_BUSY is clear */ > + while (1) { > + pte = pte_val(*ptep); > + if (unlikely(pte & _PAGE_BUSY)) { > + cpu_relax(); > + continue; > + } > + } > + > + /* If pte is not present return None */ > + if (unlikely(!(pte & _PAGE_PRESENT))) > + return __pte(0); First, this looks racy to me, since nothing stops the compiler refetching pte from memory, and it might have become busy again in the meantime. However, I don't think you need to do any of this, since the caller in your next patch checks for pte_present(pte) anyway. On book E systems we don't use _PAGE_BUSY, so you don't need the loop for your intended application. I suggest you remove the entire section quoted above. Paul.