Re: [PATCH for-next 1/9] xen/arm: Use mfn_to_pdx instead of pfn_to_pdx when possible

[Julien Grall <julien.grall@arm.com>]

(+ Andrew and Jan)

On 15/04/2019 23:42, Julien Grall wrote:
> Hi,
> 
> On 4/15/19 11:25 PM, Stefano Stabellini wrote:
>> On Mon, 15 Apr 2019, Julien Grall wrote:
>>> Hi,
>>>
>>> On 4/15/19 10:55 PM, Stefano Stabellini wrote:
>>>> On Mon, 18 Feb 2019, Julien Grall wrote:
>>>>> mfn_to_pdx adds more safety than pfn_to_pdx. Replace all but on place in
>>>>> the Arm code to use the former.
>>>>
>>>> This is good but maybe we can go even further.
>>>>
>>>> You should also be able to replace one call site of pfn_to_pdx in
>>>> mfn_valid and the one in maddr_to_virt. Something like this:
>>>>
>>>>
>>>> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
>>>> index eafa26f..b3455ea 100644
>>>> --- a/xen/include/asm-arm/mm.h
>>>> +++ b/xen/include/asm-arm/mm.h
>>>> @@ -209,7 +209,7 @@ static inline void __iomem *ioremap_wc(paddr_t start,
>>>> size_t len)
>>>>    /* XXX -- account for base */
>>>>    #define mfn_valid(mfn)        ({
>>>> \
>>>>        unsigned long __m_f_n = mfn_x(mfn);
>>>> \
>>>> -    likely(pfn_to_pdx(__m_f_n) >= frametable_base_pdx &&
>>>> __mfn_valid(__m_f_n)); \
>>>> +    likely(mfn_to_pdx(mfn) >= frametable_base_pdx && __mfn_valid(__m_f_n));
>>>> \
>>>
>>> This is quite undesirable, you will end up to evaluate mfn twice here.

I have been looking at making __mfn_valid(...) typesafe. While it compiles
on Arm, I have a build error on x86:

In file included from /home/julieng/works/xen/xen/include/asm/x86_64/page.h:47:0,
                 from /home/julieng/works/xen/xen/include/asm/page.h:23,
                 from /home/julieng/works/xen/xen/include/asm/current.h:12,
                 from /home/julieng/works/xen/xen/include/asm/smp.h:10,
                 from /home/julieng/works/xen/xen/include/xen/smp.h:4,
                 from /home/julieng/works/xen/xen/include/xen/perfc.h:7,
                 from x86_64/asm-offsets.c:8:
/home/julieng/works/xen/xen/include/xen/pdx.h:24:18: error: unknown type name ‘mfn_t’
 bool __mfn_valid(mfn_t mfn);
                  ^~~~~
In file included from /home/julieng/works/xen/xen/include/xen/config.h:13:0,
                 from <command-line>:0:
/home/julieng/works/xen/xen/include/asm/mm.h: In function ‘get_page_from_mfn’:
/home/julieng/works/xen/xen/include/asm/page.h:262:29: error: implicit declaration of function ‘__mfn_valid’ [-Werror=implicit-function-declaration]
 #define mfn_valid(mfn)      __mfn_valid(mfn)
                             ^
/home/julieng/works/xen/xen/include/xen/compiler.h:11:43: note: in definition of macro ‘unlikely’
 #define unlikely(x)   __builtin_expect(!!(x),0)

We get away on Arm because mfn_valid is implemented in mm.h. On x86 it is implemented
in page.h. I am quite impressed we never had build failure in common code until now...

Anyway, it is not the first time I see build error when trying to make the code using
typesafe gfn/mfn. The headers dependency are quite messy in general.

Andrew, Jan do you have a suggestion how to process on the x86 side?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel