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

[Julien Grall <julien.grall@xxxxxxxxxx>]

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@xxxxxxxxxxxxx>
Thank you,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel