Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752051AbdINLaC (ORCPT ); Thu, 14 Sep 2017 07:30:02 -0400 Received: from mx2.suse.de ([195.135.220.15]:52142 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751876AbdINLaB (ORCPT ); Thu, 14 Sep 2017 07:30:01 -0400 Subject: Re: [PATCH v6 05/11] arm64/mm: Add support for XPFO To: Julien Grall , Tycho Andersen , Christoph Hellwig Cc: Marco Benatto , kernel-hardening@lists.openwall.com, linux-kernel@vger.kernel.org, linux-mm@kvack.org, Juerg Haefliger , xen-devel@lists.xenproject.org, linux-arm-kernel@lists.infradead.org, Boris Ostrovsky , Stefano Stabellini References: <20170907173609.22696-1-tycho@docker.com> <20170907173609.22696-6-tycho@docker.com> <20170908075347.GC4957@infradead.org> <20170908172422.rxmhwd2vl6eye2or@docker> From: Juergen Gross Message-ID: Date: Thu, 14 Sep 2017 13:29:57 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: de-DE Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2828 Lines: 72 On 14/09/17 12:41, Julien Grall wrote: > Hi, > > CC Juergen, Boris and Stefano. > > On 08/09/17 18:24, Tycho Andersen wrote: >> On Fri, Sep 08, 2017 at 12:53:47AM -0700, Christoph Hellwig wrote: >>>> +/* >>>> + * Lookup the page table entry for a virtual address and return a >>>> pointer to >>>> + * the entry. Based on x86 tree. >>>> + */ >>>> +static pte_t *lookup_address(unsigned long addr) >>> >>> Seems like this should be moved to common arm64 mm code and used by >>> kernel_page_present. >> >> Sounds good, I'll include something like the patch below in the next >> series. >> >> Unfortunately, adding an implementation of lookup_address seems to be >> slightly more complicated than necessary, because of the xen piece. We >> have to define lookup_address() with the level parameter, but it's not >> obvious to me to name the page levels. So for now I've just left it as >> a WARN() if someone supplies it. >> >> It seems like xen still does need this to be defined, because if I >> define it without level: >> >> drivers/xen/xenbus/xenbus_client.c: In function >> ‘xenbus_unmap_ring_vfree_pv’: >> drivers/xen/xenbus/xenbus_client.c:760:4: error: too many arguments to >> function ‘lookup_address’ >>      lookup_address(addr, &level)).maddr; >>      ^~~~~~~~~~~~~~ >> In file included from ./arch/arm64/include/asm/page.h:37:0, >>                   from ./include/linux/mmzone.h:20, >>                   from ./include/linux/gfp.h:5, >>                   from ./include/linux/mm.h:9, >>                   from drivers/xen/xenbus/xenbus_client.c:33: >> ./arch/arm64/include/asm/pgtable-types.h:67:15: note: declared here >>   extern pte_t *lookup_address(unsigned long addr); >>                 ^~~~~~~~~~~~~~ >> >> I've cc-d the xen folks, maybe they can suggest a way to untangle it? >> Alternatively, if someone can suggest a good naming scheme for the >> page levels, I can just do that. > > The implementation of lookup_address(...) on ARM for Xen (see > include/xen/arm/page.h) is just a BUG(). This is because this code > should never be called (only used for x86 PV code). > > Furthermore, xenbus client does not use at all the level. It is just to > cope with the x86 version of lookup_address. > > So one way to solve the problem would be to introduce > xen_lookup_address(addr) that would be implemented as: >     - on x86 >         unsigned int level; > >         return lookup_address(addr, &level).maddr; >     - on ARM >         BUG(); > > With that there would be no prototype clash and avoid introducing a > level parameter. I'd rather add some #ifdef CONFIG_XEN_PV and remove the *_pv functions from ARM completely this way. I'm sending a patch soon... Juergen