[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [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_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_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@xxxxxxxxxxxxx>

Thank you,

Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.