linux-kernel - Re: [PATCH v2 01/20] xen: Add Xen specific page definition

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <55A7C54B.5030603@citrix.com>
Date:	Thu, 16 Jul 2015 16:52:59 +0200
From:	Julien Grall <julien.grall@...rix.com>
To:	Stefano Stabellini <stefano.stabellini@...citrix.com>
CC:	<ian.campbell@...rix.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
	<linux-kernel@...r.kernel.org>,
	David Vrabel <david.vrabel@...rix.com>,
	<xen-devel@...ts.xenproject.org>,
	Boris Ostrovsky <boris.ostrovsky@...cle.com>,
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v2 01/20] xen: Add Xen specific page definition

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@...citrix.com>

Thank you,

-- 
Julien Grall
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/