[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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.
