Date: Thu, 14 Sep 2017 13:29:57 +0200 From: Juergen Gross <jgross@...e.com> To: Julien Grall <julien.grall@....com>, Tycho Andersen <tycho@...ker.com>, Christoph Hellwig <hch@...radead.org> Cc: Marco Benatto <marco.antonio.780@...il.com>, kernel-hardening@...ts.openwall.com, linux-kernel@...r.kernel.org, linux-mm@...ck.org, Juerg Haefliger <juerg.haefliger@...onical.com>, xen-devel@...ts.xenproject.org, linux-arm-kernel@...ts.infradead.org, Boris Ostrovsky <boris.ostrovsky@...cle.com>, Stefano Stabellini <sstabellini@...nel.org> Subject: Re: [PATCH v6 05/11] arm64/mm: Add support for XPFO 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
Powered by blists - more mailing lists
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.