Re: [PATCH v6 05/11] arm64/mm: Add support for XPFO

[Juergen Gross]

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