[PATCH v2 01/20] xen: Add Xen specific page definition

[Julien Grall]

Hi Stefano,

On 16/07/2015 16:19, Stefano Stabellini wrote:
>> diff --git a/include/xen/page.h b/include/xen/page.h
>> index c5ed20b..8ebd37b 100644
>> --- a/include/xen/page.h
>> +++ b/include/xen/page.h
>> @@ -1,11 +1,30 @@
>>   #ifndef _XEN_PAGE_H
>>   #define _XEN_PAGE_H
>>
>> +#include <asm/page.h>
>> +
>> +/* The hypercall interface supports only 4KB page */
>> +#define XEN_PAGE_SHIFT	12
>> +#define XEN_PAGE_SIZE	(_AC(1,UL) << XEN_PAGE_SHIFT)
>> +#define XEN_PAGE_MASK	(~(XEN_PAGE_SIZE-1))
>> +#define xen_offset_in_page(p)	((unsigned long)(p) & ~XEN_PAGE_MASK)
>> +#define xen_pfn_to_page(pfn)	\
>
> I think it would be clearer if you called the parameter "xen_pfn"
> instead of "pfn".

Good idea, I will do it in the next version.

>
>> +	((pfn_to_page(((unsigned long)(pfn) << XEN_PAGE_SHIFT) >> PAGE_SHIFT)))
>> +#define xen_page_to_pfn(page)	\
>> +	(((page_to_pfn(page)) << PAGE_SHIFT) >> XEN_PAGE_SHIFT)
>
>
> It would be nice to have a comment:
>
> /* assume PAGE_SIZE is a multiple of XEN_PAGE_SIZE */

Ok. FWIW, there is already a BUILD_BUG_ON the privcmd driver to check 
this assumption (see patch #19).

I could move the BUILD_BUG_ON in page.h. Maybe inside xen_page_to_pfn?

>
>
>> +#define XEN_PFN_PER_PAGE	(PAGE_SIZE / XEN_PAGE_SIZE)
>> +
>> +#define XEN_PFN_DOWN(x)	((x) >> XEN_PAGE_SHIFT)
>> +#define XEN_PFN_UP(x)	(((x) + XEN_PAGE_SIZE-1) >> XEN_PAGE_SHIFT)
>> +#define XEN_PFN_PHYS(x)	((phys_addr_t)(x) << XEN_PAGE_SHIFT)
>> +
>>   #include <asm/xen/page.h>
>>
>> +/* Return the MFN associated to the first 4KB of the page */
>>   static inline unsigned long page_to_mfn(struct page *page)
>>   {
>> -	return pfn_to_mfn(page_to_pfn(page));
>> +	return pfn_to_mfn(xen_page_to_pfn(page));
>>   }
>>
>>   struct xen_memory_region {
>
>
> Aside from the two minor suggestions:
>
> Reviewed-by: Stefano Stabellini <stefano.stabellini at eu.citrix.com>

Thank you,

-- 
Julien Grall