| Message ID | 1436474552-31789-2-git-send-email-julien.grall@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 9 Jul 2015, Julien Grall wrote: > The Xen hypercall interface is always using 4K page granularity on ARM > and x86 architecture. > > With the incoming support of 64K page granularity for ARM64 guest, it > won't be possible to re-use the Linux page definition in Xen drivers. > > Introduce Xen page definition helpers based on the Linux page > definition. They have exactly the same name but prefixed with > XEN_/xen_ prefix. > > Also modify page_to_pfn to use new Xen page definition. > > Signed-off-by: Julien Grall <julien.grall@citrix.com> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> > Cc: David Vrabel <david.vrabel@citrix.com> > --- > I'm wondering if we should drop page_to_pfn has the macro will likely > misuse when Linux is using 64KB page granularity. > > Changes in v2: > - Add XEN_PFN_UP > - Add a comment describing the behavior of page_to_pfn > --- > include/xen/page.h | 21 ++++++++++++++++++++- > 1 file changed, 20 insertions(+), 1 deletion(-) > > 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". > + ((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 */ > +#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@eu.citrix.com>
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@eu.citrix.com> Thank you,
On 09/07/15 21:42, Julien Grall wrote: > The Xen hypercall interface is always using 4K page granularity on ARM > and x86 architecture. > > With the incoming support of 64K page granularity for ARM64 guest, it > won't be possible to re-use the Linux page definition in Xen drivers. > > Introduce Xen page definition helpers based on the Linux page > definition. They have exactly the same name but prefixed with > XEN_/xen_ prefix. > > Also modify page_to_pfn to use new Xen page definition. > > Signed-off-by: Julien Grall <julien.grall@citrix.com> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> > Cc: David Vrabel <david.vrabel@citrix.com> > --- > I'm wondering if we should drop page_to_pfn has the macro will likely > misuse when Linux is using 64KB page granularity. I think we want xen_gfn_to_page() and xen_page_to_gfn() and Xen front/back drivers never deal with PFNs only GFNs. David
Hi David, On 24/07/15 10:28, David Vrabel wrote: > On 09/07/15 21:42, Julien Grall wrote: >> The Xen hypercall interface is always using 4K page granularity on ARM >> and x86 architecture. >> >> With the incoming support of 64K page granularity for ARM64 guest, it >> won't be possible to re-use the Linux page definition in Xen drivers. >> >> Introduce Xen page definition helpers based on the Linux page >> definition. They have exactly the same name but prefixed with >> XEN_/xen_ prefix. >> >> Also modify page_to_pfn to use new Xen page definition. >> >> Signed-off-by: Julien Grall <julien.grall@citrix.com> >> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> >> Cc: David Vrabel <david.vrabel@citrix.com> >> --- >> I'm wondering if we should drop page_to_pfn has the macro will likely >> misuse when Linux is using 64KB page granularity. > > I think we want xen_gfn_to_page() and xen_page_to_gfn() and Xen > front/back drivers never deal with PFNs only GFNs. What is xen_gfn_to_page and xen_page_to_gfn? Neither Linux, nor my series have them. Regards,
On 24/07/15 10:39, Julien Grall wrote: > Hi David, > > On 24/07/15 10:28, David Vrabel wrote: >> On 09/07/15 21:42, Julien Grall wrote: >>> The Xen hypercall interface is always using 4K page granularity on ARM >>> and x86 architecture. >>> >>> With the incoming support of 64K page granularity for ARM64 guest, it >>> won't be possible to re-use the Linux page definition in Xen drivers. >>> >>> Introduce Xen page definition helpers based on the Linux page >>> definition. They have exactly the same name but prefixed with >>> XEN_/xen_ prefix. >>> >>> Also modify page_to_pfn to use new Xen page definition. >>> >>> Signed-off-by: Julien Grall <julien.grall@citrix.com> >>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> >>> Cc: David Vrabel <david.vrabel@citrix.com> >>> --- >>> I'm wondering if we should drop page_to_pfn has the macro will likely >>> misuse when Linux is using 64KB page granularity. >> >> I think we want xen_gfn_to_page() and xen_page_to_gfn() and Xen >> front/back drivers never deal with PFNs only GFNs. > > What is xen_gfn_to_page and xen_page_to_gfn? Neither Linux, nor my > series have them. I suggesting that you introduce these. David
On 24/07/15 10:48, David Vrabel wrote: > On 24/07/15 10:39, Julien Grall wrote: >> Hi David, >> >> On 24/07/15 10:28, David Vrabel wrote: >>> On 09/07/15 21:42, Julien Grall wrote: >>>> The Xen hypercall interface is always using 4K page granularity on ARM >>>> and x86 architecture. >>>> >>>> With the incoming support of 64K page granularity for ARM64 guest, it >>>> won't be possible to re-use the Linux page definition in Xen drivers. >>>> >>>> Introduce Xen page definition helpers based on the Linux page >>>> definition. They have exactly the same name but prefixed with >>>> XEN_/xen_ prefix. >>>> >>>> Also modify page_to_pfn to use new Xen page definition. >>>> >>>> Signed-off-by: Julien Grall <julien.grall@citrix.com> >>>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >>>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> >>>> Cc: David Vrabel <david.vrabel@citrix.com> >>>> --- >>>> I'm wondering if we should drop page_to_pfn has the macro will likely >>>> misuse when Linux is using 64KB page granularity. >>> >>> I think we want xen_gfn_to_page() and xen_page_to_gfn() and Xen >>> front/back drivers never deal with PFNs only GFNs. >> >> What is xen_gfn_to_page and xen_page_to_gfn? Neither Linux, nor my >> series have them. > > I suggesting that you introduce these. It's still not clear to me what you are suggesting here... Do you suggest to rename xen_pfn_to_page and xen_page_to_pfn by xen_gfn_to_page and xen_page_to_gfn? Regards,
On 24/07/15 10:51, Julien Grall wrote: > On 24/07/15 10:48, David Vrabel wrote: >> On 24/07/15 10:39, Julien Grall wrote: >>> Hi David, >>> >>> On 24/07/15 10:28, David Vrabel wrote: >>>> On 09/07/15 21:42, Julien Grall wrote: >>>>> The Xen hypercall interface is always using 4K page granularity on ARM >>>>> and x86 architecture. >>>>> >>>>> With the incoming support of 64K page granularity for ARM64 guest, it >>>>> won't be possible to re-use the Linux page definition in Xen drivers. >>>>> >>>>> Introduce Xen page definition helpers based on the Linux page >>>>> definition. They have exactly the same name but prefixed with >>>>> XEN_/xen_ prefix. >>>>> >>>>> Also modify page_to_pfn to use new Xen page definition. >>>>> >>>>> Signed-off-by: Julien Grall <julien.grall@citrix.com> >>>>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >>>>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> >>>>> Cc: David Vrabel <david.vrabel@citrix.com> >>>>> --- >>>>> I'm wondering if we should drop page_to_pfn has the macro will likely >>>>> misuse when Linux is using 64KB page granularity. >>>> >>>> I think we want xen_gfn_to_page() and xen_page_to_gfn() and Xen >>>> front/back drivers never deal with PFNs only GFNs. >>> >>> What is xen_gfn_to_page and xen_page_to_gfn? Neither Linux, nor my >>> series have them. >> >> I suggesting that you introduce these. > > It's still not clear to me what you are suggesting here... Do you > suggest to rename xen_pfn_to_page and xen_page_to_pfn by xen_gfn_to_page > and xen_page_to_gfn? Effectively, yes but it would be better to think that: PFNs index guest-sized pages (which may be 64 KiB). GFNs index Xen-sized pages (which is always 4 KiB). David
On Fri, 2015-07-24 at 11:34 +0100, David Vrabel wrote: > it would be better to think that: > > PFNs index guest-sized pages (which may be 64 KiB). > > GFNs index Xen-sized pages (which is always 4 KiB). This concept could be usefully added to the comment in xen/include/xen/mm.h IMHO. > > David
On 24/07/15 11:34, David Vrabel wrote: > On 24/07/15 10:51, Julien Grall wrote: >> On 24/07/15 10:48, David Vrabel wrote: >>> On 24/07/15 10:39, Julien Grall wrote: >>>> Hi David, >>>> >>>> On 24/07/15 10:28, David Vrabel wrote: >>>>> On 09/07/15 21:42, Julien Grall wrote: >>>>>> The Xen hypercall interface is always using 4K page granularity on ARM >>>>>> and x86 architecture. >>>>>> >>>>>> With the incoming support of 64K page granularity for ARM64 guest, it >>>>>> won't be possible to re-use the Linux page definition in Xen drivers. >>>>>> >>>>>> Introduce Xen page definition helpers based on the Linux page >>>>>> definition. They have exactly the same name but prefixed with >>>>>> XEN_/xen_ prefix. >>>>>> >>>>>> Also modify page_to_pfn to use new Xen page definition. >>>>>> >>>>>> Signed-off-by: Julien Grall <julien.grall@citrix.com> >>>>>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >>>>>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> >>>>>> Cc: David Vrabel <david.vrabel@citrix.com> >>>>>> --- >>>>>> I'm wondering if we should drop page_to_pfn has the macro will likely >>>>>> misuse when Linux is using 64KB page granularity. >>>>> >>>>> I think we want xen_gfn_to_page() and xen_page_to_gfn() and Xen >>>>> front/back drivers never deal with PFNs only GFNs. >>>> >>>> What is xen_gfn_to_page and xen_page_to_gfn? Neither Linux, nor my >>>> series have them. >>> >>> I suggesting that you introduce these. >> >> It's still not clear to me what you are suggesting here... Do you >> suggest to rename xen_pfn_to_page and xen_page_to_pfn by xen_gfn_to_page >> and xen_page_to_gfn? > > Effectively, yes but it would be better to think that: > > PFNs index guest-sized pages (which may be 64 KiB). > > GFNs index Xen-sized pages (which is always 4 KiB). If I'm understanding correctly you mean: #define xen_page_to_gfn(page) \ ((page_to_pfn(page) << PAGE_SHIFT) >> XEN_PAGE_SHIFT)) static page_to_mfn(struct page *page) { return pfn_to_mfn(xen_page_to_gfn(page)); } Although in some place you are suggesting to use: xen_page_to_gfn(virt_to_page(info->intf)) (see patch #11) where it suggests to rename page_to_mfn in xen_page_to_gfn. I think it would make more sense to use the latter one. We would also need to name to describe a PFN (pseudo-physical frame number based on xen/include/xen/mm.h) but with 4K granularity and not the Linux granularity. It's useful to have it in some place in order to iter on the 4K pfn (see gnttab_foreach_grant and xen_apply_to_page). Maybe xpfn for Xen pseudo-physical frame number? I will preprend some patches into this serie to rename the function with their correct naming. I have in mind pfn_to_mfn which should be name into pfn_to_gfn given the usage. Similarly, this function is mis-used on ARM because the function may return an MFN where we expect a GFN. Regards,
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) \ + ((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) + +#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 {
The Xen hypercall interface is always using 4K page granularity on ARM and x86 architecture. With the incoming support of 64K page granularity for ARM64 guest, it won't be possible to re-use the Linux page definition in Xen drivers. Introduce Xen page definition helpers based on the Linux page definition. They have exactly the same name but prefixed with XEN_/xen_ prefix. Also modify page_to_pfn to use new Xen page definition. Signed-off-by: Julien Grall <julien.grall@citrix.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> Cc: David Vrabel <david.vrabel@citrix.com> --- I'm wondering if we should drop page_to_pfn has the macro will likely misuse when Linux is using 64KB page granularity. Changes in v2: - Add XEN_PFN_UP - Add a comment describing the behavior of page_to_pfn --- include/xen/page.h | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)