[RFC 00/14] arm: implement ballooning and privcmd foreign mappings based on x86 PVH

[RFC 00/14] arm: implement ballooning and privcmd foreign mappings based on x86 PVH

[Ian Campbell]

This series implements ballooning for Xen on ARM and builds and Mukesh's
PVH privcmd stuff to implement foreign page mapping on ARM, replacing
the old "HACK: initial (very hacky) XENMAPSPACE_gmfn_foreign" patch.

The baseline is a bit complex, it is basically Stefano's xenarm-forlinus
branch (commit bbd6eb29214e) merged with Konrad's linux-next-pvh branch
(commit 7e6e6f589de8). I suspect I might need to rebase it shortly. I
know there's a bunch of stuff in here which Mukesh has also changed, but
which I haven't seen yet, I'll deal with that when I rebase (RFC mainly
for that reason only).

This lets me start a guest without any of those nasty patches with
"HACK" in the title ;-0

Ian.

[PATCH 01/14] Build fixes which I beleive others have already fixed properly elsewhere

[Ian Campbell]

Do not apply, you have better versions of all these somewhere else!
---
 drivers/xen/Makefile           |    2 +-
 include/xen/interface/memory.h |    4 +---
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
index cd28aae..275abfc 100644
--- a/drivers/xen/Makefile
+++ b/drivers/xen/Makefile
@@ -8,10 +8,10 @@ obj-y	+= xenbus/
 nostackp := $(call cc-option, -fno-stack-protector)
 CFLAGS_features.o			:= $(nostackp)
 
-obj-$(CONFIG_XEN_DOM0)			+= $(dom0-y)
 dom0-$(CONFIG_PCI) += pci.o
 dom0-$(CONFIG_ACPI) += acpi.o
 dom0-$(CONFIG_X86) += pcpu.o
+obj-$(CONFIG_XEN_DOM0)			+= $(dom0-y)
 obj-$(CONFIG_BLOCK)			+= biomerge.o
 obj-$(CONFIG_XEN_XENCOMM)		+= xencomm.o
 obj-$(CONFIG_XEN_BALLOON)		+= xen-balloon.o
diff --git a/include/xen/interface/memory.h b/include/xen/interface/memory.h
index 9953914..6d74c47 100644
--- a/include/xen/interface/memory.h
+++ b/include/xen/interface/memory.h
@@ -163,15 +163,13 @@ struct xen_add_to_physmap {
     /* Which domain to change the mapping for. */
     domid_t domid;
 
-    /* Number of pages to go through for gmfn_range */
-    uint16_t    size;
-
     union {
         /* Number of pages to go through for gmfn_range */
         uint16_t    size;
 	/* IFF XENMAPSPACE_gmfn_foreign */
         domid_t foreign_domid;
     } u;
+
     /* Source mapping space. */
 #define XENMAPSPACE_shared_info 0 /* shared info page */
 #define XENMAPSPACE_grant_table 1 /* grant table page */
-- 
1.7.2.5

Re: [PATCH 01/14] Build fixes which I beleive others have already fixed properly elsewhere

[Konrad Rzeszutek Wilk]

On Thu, Oct 04, 2012 at 04:11:42PM +0100, Ian Campbell wrote:
> Do not apply, you have better versions of all these somewhere else!

<nods>.
> ---
>  drivers/xen/Makefile           |    2 +-
>  include/xen/interface/memory.h |    4 +---
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
> index cd28aae..275abfc 100644
> --- a/drivers/xen/Makefile
> +++ b/drivers/xen/Makefile
> @@ -8,10 +8,10 @@ obj-y	+= xenbus/
>  nostackp := $(call cc-option, -fno-stack-protector)
>  CFLAGS_features.o			:= $(nostackp)
>  
> -obj-$(CONFIG_XEN_DOM0)			+= $(dom0-y)
>  dom0-$(CONFIG_PCI) += pci.o
>  dom0-$(CONFIG_ACPI) += acpi.o
>  dom0-$(CONFIG_X86) += pcpu.o
> +obj-$(CONFIG_XEN_DOM0)			+= $(dom0-y)
>  obj-$(CONFIG_BLOCK)			+= biomerge.o
>  obj-$(CONFIG_XEN_XENCOMM)		+= xencomm.o
>  obj-$(CONFIG_XEN_BALLOON)		+= xen-balloon.o
> diff --git a/include/xen/interface/memory.h b/include/xen/interface/memory.h
> index 9953914..6d74c47 100644
> --- a/include/xen/interface/memory.h
> +++ b/include/xen/interface/memory.h
> @@ -163,15 +163,13 @@ struct xen_add_to_physmap {
>      /* Which domain to change the mapping for. */
>      domid_t domid;
>  
> -    /* Number of pages to go through for gmfn_range */
> -    uint16_t    size;
> -
>      union {
>          /* Number of pages to go through for gmfn_range */
>          uint16_t    size;
>  	/* IFF XENMAPSPACE_gmfn_foreign */
>          domid_t foreign_domid;
>      } u;
> +
>      /* Source mapping space. */
>  #define XENMAPSPACE_shared_info 0 /* shared info page */
>  #define XENMAPSPACE_grant_table 1 /* grant table page */
> -- 
> 1.7.2.5

[PATCH 02/14] xenbus: include features header.

[Ian Campbell]

Fixes:
drivers/xen/xenbus/xenbus_probe.c: In function 'xenbus_init':
drivers/xen/xenbus/xenbus_probe.c:760:3: error: implicit declaration of function 'xen_feature' [-Werror=implicit-function-declaration]
drivers/xen/xenbus/xenbus_probe.c:760:19: error: 'XENFEAT_auto_translated_physmap' undeclared (first use in this function)
drivers/xen/xenbus/xenbus_probe.c:760:19: note: each undeclared identifier is reported only once for each function it appears in
cc1: some warnings being treated as errors

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/xen/xenbus/xenbus_probe.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
index b92c024..974bea0 100644
--- a/drivers/xen/xenbus/xenbus_probe.c
+++ b/drivers/xen/xenbus/xenbus_probe.c
@@ -56,6 +56,7 @@
 #include <xen/xenbus.h>
 #include <xen/events.h>
 #include <xen/page.h>
+#include <xen/features.h>
 
 #include <xen/hvm.h>
 
-- 
1.7.2.5

[PATCH 03/14] xen events: xen_callback_vector is x86 specific

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
Should instead move somewhere arch specific?
---
 drivers/xen/events.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 6f55ef2..2508981 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -1775,6 +1775,7 @@ int xen_set_callback_via(uint64_t via)
 EXPORT_SYMBOL_GPL(xen_set_callback_via);
 
 
+#ifdef CONFIG_X86
 /* Vector callbacks are better than PCI interrupts to receive event
  * channel notifications because we can receive vector callbacks on any
  * vcpu and we don't need PCI support or APIC interactions. */
@@ -1798,6 +1799,9 @@ void xen_callback_vector(void)
 			alloc_intr_gate(XEN_HVM_EVTCHN_CALLBACK, xen_hvm_callback_vector);
 	}
 }
+#else
+void xen_callback_vector(void) {}
+#endif
 
 void xen_init_IRQ(void)
 {
-- 
1.7.2.5

Re: [PATCH 03/14] xen events: xen_callback_vector is x86 specific

[Stefano Stabellini]

On Thu, 4 Oct 2012, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> Should instead move somewhere arch specific?

This shouldn't be needed: it should be already protected by
CONFIG_XEN_PVHVM (that is x86 specific).


>  drivers/xen/events.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> index 6f55ef2..2508981 100644
> --- a/drivers/xen/events.c
> +++ b/drivers/xen/events.c
> @@ -1775,6 +1775,7 @@ int xen_set_callback_via(uint64_t via)
>  EXPORT_SYMBOL_GPL(xen_set_callback_via);
>  
>  
> +#ifdef CONFIG_X86
>  /* Vector callbacks are better than PCI interrupts to receive event
>   * channel notifications because we can receive vector callbacks on any
>   * vcpu and we don't need PCI support or APIC interactions. */
> @@ -1798,6 +1799,9 @@ void xen_callback_vector(void)
>  			alloc_intr_gate(XEN_HVM_EVTCHN_CALLBACK, xen_hvm_callback_vector);
>  	}
>  }
> +#else
> +void xen_callback_vector(void) {}
> +#endif
>  
>  void xen_init_IRQ(void)
>  {
> -- 
> 1.7.2.5
> 

Re: [PATCH 03/14] xen events: xen_callback_vector is x86 specific

[Ian Campbell]

On Fri, 2012-10-05 at 12:43 +0100, Stefano Stabellini wrote:
> On Thu, 4 Oct 2012, Ian Campbell wrote:
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> > Should instead move somewhere arch specific?
> 
> This shouldn't be needed: it should be already protected by
> CONFIG_XEN_PVHVM (that is x86 specific).

Mukesh removed that ifdef because PVH also wants this function.

Ian.

> 
> 
> >  drivers/xen/events.c |    4 ++++
> >  1 files changed, 4 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> > index 6f55ef2..2508981 100644
> > --- a/drivers/xen/events.c
> > +++ b/drivers/xen/events.c
> > @@ -1775,6 +1775,7 @@ int xen_set_callback_via(uint64_t via)
> >  EXPORT_SYMBOL_GPL(xen_set_callback_via);
> >  
> >  
> > +#ifdef CONFIG_X86
> >  /* Vector callbacks are better than PCI interrupts to receive event
> >   * channel notifications because we can receive vector callbacks on any
> >   * vcpu and we don't need PCI support or APIC interactions. */
> > @@ -1798,6 +1799,9 @@ void xen_callback_vector(void)
> >  			alloc_intr_gate(XEN_HVM_EVTCHN_CALLBACK, xen_hvm_callback_vector);
> >  	}
> >  }
> > +#else
> > +void xen_callback_vector(void) {}
> > +#endif
> >  
> >  void xen_init_IRQ(void)
> >  {
> > -- 
> > 1.7.2.5
> > 

Re: [PATCH 03/14] xen events: xen_callback_vector is x86 specific

[Mukesh Rathor]

On Fri, 5 Oct 2012 12:47:25 +0100
Ian Campbell <Ian.Campbell@citrix.com> wrote:

> On Fri, 2012-10-05 at 12:43 +0100, Stefano Stabellini wrote:
> > On Thu, 4 Oct 2012, Ian Campbell wrote:
> > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > > ---
> > > Should instead move somewhere arch specific?
> > 
> > This shouldn't be needed: it should be already protected by
> > CONFIG_XEN_PVHVM (that is x86 specific).
> 
> Mukesh removed that ifdef because PVH also wants this function.
> 

I'm putting #ifdef CONFIG_X86 around it.

[PATCH 04/14] xen: balloon: don't include e820.h

[Ian Campbell]

This breaks on !X86 and AFAICT is not required on X86 either.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/xen/balloon.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index 85a6917..85ef7e7 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -55,7 +55,6 @@
 #include <asm/pgalloc.h>
 #include <asm/pgtable.h>
 #include <asm/tlb.h>
-#include <asm/e820.h>
 
 #include <asm/xen/hypervisor.h>
 #include <asm/xen/hypercall.h>
-- 
1.7.2.5

[PATCH 05/14] xen: balloon: use correct type for frame_list

[Ian Campbell]

This is now a xen_pfn_t.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/xen/balloon.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index 85ef7e7..4625560 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -87,7 +87,7 @@ struct balloon_stats balloon_stats;
 EXPORT_SYMBOL_GPL(balloon_stats);
 
 /* We increase/decrease in batches which fit in a page */
-static unsigned long frame_list[PAGE_SIZE / sizeof(unsigned long)];
+static xen_pfn_t frame_list[PAGE_SIZE / sizeof(unsigned long)];
 
 #ifdef CONFIG_HIGHMEM
 #define inc_totalhigh_pages() (totalhigh_pages++)
-- 
1.7.2.5

Re: [PATCH 05/14] xen: balloon: use correct type for frame_list

[Stefano Stabellini]

On Thu, 4 Oct 2012, Ian Campbell wrote:
> This is now a xen_pfn_t.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  drivers/xen/balloon.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index 85ef7e7..4625560 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -87,7 +87,7 @@ struct balloon_stats balloon_stats;
>  EXPORT_SYMBOL_GPL(balloon_stats);
>  
>  /* We increase/decrease in batches which fit in a page */
> -static unsigned long frame_list[PAGE_SIZE / sizeof(unsigned long)];
> +static xen_pfn_t frame_list[PAGE_SIZE / sizeof(unsigned long)];
>  
>  #ifdef CONFIG_HIGHMEM
>  #define inc_totalhigh_pages() (totalhigh_pages++)
 
While I think is a good change, frame_list[i] is used as an argument to
set_phys_to_machine, that takes an unsigned long. Also unsigned long are
assigned to members of the array via page_to_pfn.
I think that we should take care of these cases, either introducing
explicit casts or changing the argument types.

Re: [PATCH 05/14] xen: balloon: use correct type for frame_list

[Ian Campbell]

On Fri, 2012-10-05 at 12:48 +0100, Stefano Stabellini wrote:
> On Thu, 4 Oct 2012, Ian Campbell wrote:
> > This is now a xen_pfn_t.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  drivers/xen/balloon.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> > index 85ef7e7..4625560 100644
> > --- a/drivers/xen/balloon.c
> > +++ b/drivers/xen/balloon.c
> > @@ -87,7 +87,7 @@ struct balloon_stats balloon_stats;
> >  EXPORT_SYMBOL_GPL(balloon_stats);
> >  
> >  /* We increase/decrease in batches which fit in a page */
> > -static unsigned long frame_list[PAGE_SIZE / sizeof(unsigned long)];
> > +static xen_pfn_t frame_list[PAGE_SIZE / sizeof(unsigned long)];
> >  
> >  #ifdef CONFIG_HIGHMEM
> >  #define inc_totalhigh_pages() (totalhigh_pages++)
>  
> While I think is a good change, frame_list[i] is used as an argument to
> set_phys_to_machine, that takes an unsigned long. Also unsigned long are
> assigned to members of the array via page_to_pfn.
> I think that we should take care of these cases, either introducing
> explicit casts or changing the argument types.

frame_list is used at the Xen interface, and so the pfn type has to be
sized for the largest pfn the hypervisor will use (aka xen_pfn_t). Those
unsigned longs are really "linux_pfn_t", that is they are the size of
the largest pfn the guest is itself prepared to deal with.

So long as sizeof(unsigned long) <= sizeof(xen_pfn_t) (which it is) then
we are ok.

If and when Linux wants to use pfn's which are not unsigned longs then
the uses of unsigned long will need to be audited (globally, not just
here) to become whatever type Linux then defines to contain a pfn. In
the absence of that type being defined in the core Linux code I think it
is correct for us to keep using unsigned long in those contexts.

Konrad, now I think about it the same argument applies to the discussion
of fgmfn in <20121004155400.GA12128@phenom.dumpdata.com>. So I'll leave
that as unsigned long as well.

Ian.

Re: [PATCH 05/14] xen: balloon: use correct type for frame_list

[Stefano Stabellini]

On Fri, 5 Oct 2012, Ian Campbell wrote:
> On Fri, 2012-10-05 at 12:48 +0100, Stefano Stabellini wrote:
> > On Thu, 4 Oct 2012, Ian Campbell wrote:
> > > This is now a xen_pfn_t.
> > > 
> > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > > ---
> > >  drivers/xen/balloon.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> > > index 85ef7e7..4625560 100644
> > > --- a/drivers/xen/balloon.c
> > > +++ b/drivers/xen/balloon.c
> > > @@ -87,7 +87,7 @@ struct balloon_stats balloon_stats;
> > >  EXPORT_SYMBOL_GPL(balloon_stats);
> > >  
> > >  /* We increase/decrease in batches which fit in a page */
> > > -static unsigned long frame_list[PAGE_SIZE / sizeof(unsigned long)];
> > > +static xen_pfn_t frame_list[PAGE_SIZE / sizeof(unsigned long)];
> > >  
> > >  #ifdef CONFIG_HIGHMEM
> > >  #define inc_totalhigh_pages() (totalhigh_pages++)
> >  
> > While I think is a good change, frame_list[i] is used as an argument to
> > set_phys_to_machine, that takes an unsigned long. Also unsigned long are
> > assigned to members of the array via page_to_pfn.
> > I think that we should take care of these cases, either introducing
> > explicit casts or changing the argument types.
> 
> frame_list is used at the Xen interface, and so the pfn type has to be
> sized for the largest pfn the hypervisor will use (aka xen_pfn_t). Those
> unsigned longs are really "linux_pfn_t", that is they are the size of
> the largest pfn the guest is itself prepared to deal with.
> 
> So long as sizeof(unsigned long) <= sizeof(xen_pfn_t) (which it is) then
> we are ok.

I think that we are playing with fire here.

Let's imaging a future where physical addresses are actually 64 bit.
Let's imaging that Xen is supporting them perfectly fine and returns to
this balloon driver a pfn > ULONG_MAX (already possible on ARM).

That is a perfectly valid value for Xen to give us and we should be able
to handle it. If we are not we should return an error.
With this change we would trimmer the pfn returned by Xen to 32 bit so we
would actually have an incorrect behaviour instead.

If we assume sizeof(unsigned long) <= sizeof(xen_pfn_t), we only need a
macro like this:

#define LINUX_PFN_MAX ULONG_MAX
#define linux_pfn_t unsigned long
#define xen_pfn_to_linux_pfn(pfn)    ({BUG_ON(pfn > LINUX_PFN_MAX); (linux_pfn_t)pfn;})

that is called in the right places.


> If and when Linux wants to use pfn's which are not unsigned longs then
> the uses of unsigned long will need to be audited (globally, not just
> here) to become whatever type Linux then defines to contain a pfn. In
> the absence of that type being defined in the core Linux code I think it
> is correct for us to keep using unsigned long in those contexts.

I think is OK using unsigned long for linux_pfn, the problem is the
conversion between what Xen gives us and linux_pfns.

Re: [PATCH 05/14] xen: balloon: use correct type for frame_list

[Ian Campbell]

On Fri, 2012-10-05 at 15:02 +0100, Stefano Stabellini wrote:
> On Fri, 5 Oct 2012, Ian Campbell wrote:
> > On Fri, 2012-10-05 at 12:48 +0100, Stefano Stabellini wrote:
> > > On Thu, 4 Oct 2012, Ian Campbell wrote:
> > > > This is now a xen_pfn_t.
> > > > 
> > > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > > > ---
> > > >  drivers/xen/balloon.c |    2 +-
> > > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> > > > index 85ef7e7..4625560 100644
> > > > --- a/drivers/xen/balloon.c
> > > > +++ b/drivers/xen/balloon.c
> > > > @@ -87,7 +87,7 @@ struct balloon_stats balloon_stats;
> > > >  EXPORT_SYMBOL_GPL(balloon_stats);
> > > >  
> > > >  /* We increase/decrease in batches which fit in a page */
> > > > -static unsigned long frame_list[PAGE_SIZE / sizeof(unsigned long)];
> > > > +static xen_pfn_t frame_list[PAGE_SIZE / sizeof(unsigned long)];
> > > >  
> > > >  #ifdef CONFIG_HIGHMEM
> > > >  #define inc_totalhigh_pages() (totalhigh_pages++)
> > >  
> > > While I think is a good change, frame_list[i] is used as an argument to
> > > set_phys_to_machine, that takes an unsigned long. Also unsigned long are
> > > assigned to members of the array via page_to_pfn.
> > > I think that we should take care of these cases, either introducing
> > > explicit casts or changing the argument types.
> > 
> > frame_list is used at the Xen interface, and so the pfn type has to be
> > sized for the largest pfn the hypervisor will use (aka xen_pfn_t). Those
> > unsigned longs are really "linux_pfn_t", that is they are the size of
> > the largest pfn the guest is itself prepared to deal with.
> > 
> > So long as sizeof(unsigned long) <= sizeof(xen_pfn_t) (which it is) then
> > we are ok.
> 
> I think that we are playing with fire here.
> 
> Let's imaging a future where physical addresses are actually 64 bit.
> Let's imaging that Xen is supporting them perfectly fine and returns to
> this balloon driver a pfn > ULONG_MAX (already possible on ARM).

It might give us an *mfn* larger than ULONG_MAX but the guest would
never see that, because ARM guests (and x86 PVHVM, PVH ones etc) never
see real mfns, they are hypervisor internal with HAP and the interfaces
are all in terms of pfns.

The issue you describe could only happen for a 32 bit HAP guest if the
guests was given > 16GB (2^(32+PAGE_SHIFT) bytes) of RAM and it was
explicitly trying to balloon memory over that limit, but in order for
that to even be possible it would already need to have made its concept
of a pfn larger than 32 bits.

In theory there might be a problem for a PV guest, but in the only case
which matters:
        arch/x86/include/asm/xen/interface.h:typedef unsigned long xen_pfn_t;
and furthermore 32 bit PV guests are limited to 160G of MFN space (which
is less than 2^32) for other reasons already.

> That is a perfectly valid value for Xen to give us and we should be able
> to handle it. If we are not we should return an error.
> With this change we would trimmer the pfn returned by Xen to 32 bit so we
> would actually have an incorrect behaviour instead.
> 
> If we assume sizeof(unsigned long) <= sizeof(xen_pfn_t), we only need a
> macro like this:
> 
> #define LINUX_PFN_MAX ULONG_MAX
> #define linux_pfn_t unsigned long
> #define xen_pfn_to_linux_pfn(pfn)    ({BUG_ON(pfn > LINUX_PFN_MAX); (linux_pfn_t)pfn;})
> 
> that is called in the right places.
> 
> 
> > If and when Linux wants to use pfn's which are not unsigned longs then
> > the uses of unsigned long will need to be audited (globally, not just
> > here) to become whatever type Linux then defines to contain a pfn. In
> > the absence of that type being defined in the core Linux code I think it
> > is correct for us to keep using unsigned long in those contexts.
> 
> I think is OK using unsigned long for linux_pfn, the problem is the
> conversion between what Xen gives us and linux_pfns.

Xen never gives us PFNs, they are always the guest's choice.

Ian.

Re: [PATCH 05/14] xen: balloon: use correct type for frame_list

[Stefano Stabellini]

On Fri, 5 Oct 2012, Ian Campbell wrote:
> In theory there might be a problem for a PV guest, but in the only case
> which matters:
>         arch/x86/include/asm/xen/interface.h:typedef unsigned long xen_pfn_t;
> and furthermore 32 bit PV guests are limited to 160G of MFN space (which
> is less than 2^32) for other reasons already.

Well, we should at least write that in a comment

Re: [PATCH 05/14] xen: balloon: use correct type for frame_list

[Ian Campbell]

On Fri, 2012-10-05 at 15:33 +0100, Ian Campbell wrote:
> The issue you describe could only happen for a 32 bit HAP guest if the
> guests was given > 16GB (2^(32+PAGE_SHIFT) bytes) of RAM and it was
> explicitly trying to balloon memory over that limit, but in order for
> that to even be possible it would already need to have made its concept
> of a pfn larger than 32 bits.

The one place this might matter is in the privcmd
IOCTL_PRIVCMD_MMAPBATCH interface for the *foreign* pfn (since a small
dom0 needs to be able to build a big domU). Luckily that interface
already uses xen_pfn_t, we just need to be a bit careful in the
xen_remap_domain_mfn_range case, which Konrad tried to tell me already
and he was right...

On ARM that meant the following (built but not executed) patch, I
suspect the PVH variant needs similar treatment.

8<--------------------------

diff --git a/arch/arm/include/asm/xen/interface.h b/arch/arm/include/asm/xen/interface.h
index ae05e56..ad87917 100644
--- a/arch/arm/include/asm/xen/interface.h
+++ b/arch/arm/include/asm/xen/interface.h
@@ -43,6 +43,7 @@ DEFINE_GUEST_HANDLE(void);
 DEFINE_GUEST_HANDLE(uint64_t);
 DEFINE_GUEST_HANDLE(uint32_t);
 DEFINE_GUEST_HANDLE(xen_pfn_t);
+DEFINE_GUEST_HANDLE(xen_ulong_t);
 
 /* Maximum number of virtual CPUs in multi-processor guests. */
 #define MAX_VIRT_CPUS 1
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index a9946aa..1d64c02 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -57,7 +57,7 @@ static int map_foreign_page(unsigned long lpfn, unsigned long fgmfn,
 		.size = 1,
 		.space = XENMAPSPACE_gmfn_foreign,
 	};
-	unsigned long idx = fgmfn;
+	xen_ulong_t idx = fgmfn;
 	xen_pfn_t gpfn = lpfn;
 
 	set_xen_guest_handle(xatp.idxs, &idx);
@@ -73,7 +73,7 @@ static int map_foreign_page(unsigned long lpfn, unsigned long fgmfn,
 }
 
 struct remap_data {
-	unsigned long fgmfn; /* foreign domain's gmfn */
+	xen_pfn_t fgmfn; /* foreign domain's gmfn */
 	pgprot_t prot;
 	domid_t  domid;
 	struct vm_area_struct *vma;
@@ -98,7 +98,7 @@ static int remap_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr,
 
 int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
 			       unsigned long addr,
-			       unsigned long mfn, int nr,
+			       xen_pfn_t mfn, int nr,
 			       pgprot_t prot, unsigned domid,
 			       struct xen_remap_mfn_info *info)
 {
diff --git a/arch/x86/include/asm/xen/interface.h b/arch/x86/include/asm/xen/interface.h
index 250c254..d67f3c6 100644
--- a/arch/x86/include/asm/xen/interface.h
+++ b/arch/x86/include/asm/xen/interface.h
@@ -63,6 +63,7 @@ DEFINE_GUEST_HANDLE(void);
 DEFINE_GUEST_HANDLE(uint64_t);
 DEFINE_GUEST_HANDLE(uint32_t);
 DEFINE_GUEST_HANDLE(xen_pfn_t);
+DEFINE_GUEST_HANDLE(xen_ulong_t);
 #endif
 
 #ifndef HYPERVISOR_VIRT_START
diff --git a/include/xen/interface/memory.h b/include/xen/interface/memory.h
index e5675bc..24e5731 100644
--- a/include/xen/interface/memory.h
+++ b/include/xen/interface/memory.h
@@ -199,7 +199,7 @@ struct xen_add_to_physmap_range {
     domid_t foreign_domid; /* IFF gmfn_foreign */
 
     /* Indexes into space being mapped. */
-    GUEST_HANDLE(ulong) idxs;
+    GUEST_HANDLE(xen_ulong_t) idxs;
 
     /* GPFN in domid where the source mapping page should appear. */
     GUEST_HANDLE(xen_pfn_t) gpfns;
diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
index 2f3cb06..59309f3 100644
--- a/include/xen/xen-ops.h
+++ b/include/xen/xen-ops.h
@@ -27,7 +27,7 @@ struct vm_area_struct;
 struct xen_remap_mfn_info;
 int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
 			       unsigned long addr,
-			       unsigned long mfn, int nr,
+			       xen_pfn_t mfn, int nr,
 			       pgprot_t prot, unsigned domid,
 			       struct xen_remap_mfn_info *pvhp);
 int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,

Re: [PATCH 05/14] xen: balloon: use correct type for frame_list

[Ian Campbell]

On Fri, 2012-10-05 at 17:11 +0100, Ian Campbell wrote:
> On Fri, 2012-10-05 at 15:33 +0100, Ian Campbell wrote:
> > The issue you describe could only happen for a 32 bit HAP guest if the
> > guests was given > 16GB (2^(32+PAGE_SHIFT) bytes) of RAM and it was
> > explicitly trying to balloon memory over that limit, but in order for
> > that to even be possible it would already need to have made its concept
> > of a pfn larger than 32 bits.
> 
> The one place this might matter is in the privcmd
> IOCTL_PRIVCMD_MMAPBATCH interface for the *foreign* pfn (since a small
> dom0 needs to be able to build a big domU). Luckily that interface
> already uses xen_pfn_t, we just need to be a bit careful in the
> xen_remap_domain_mfn_range case, which Konrad tried to tell me already
> and he was right...
> 
> On ARM that meant the following (built but not executed) patch, I
> suspect the PVH variant needs similar treatment.

NB, this is mostly just a bug fix to "arm: implement foreign mapping
using XENMEM_add_to_physmap_range" and/or "arm: implement remap
interfaces needed for privcmd mappings."

> 8<--------------------------
> 
> diff --git a/arch/arm/include/asm/xen/interface.h b/arch/arm/include/asm/xen/interface.h
> index ae05e56..ad87917 100644
> --- a/arch/arm/include/asm/xen/interface.h
> +++ b/arch/arm/include/asm/xen/interface.h
> @@ -43,6 +43,7 @@ DEFINE_GUEST_HANDLE(void);
>  DEFINE_GUEST_HANDLE(uint64_t);
>  DEFINE_GUEST_HANDLE(uint32_t);
>  DEFINE_GUEST_HANDLE(xen_pfn_t);
> +DEFINE_GUEST_HANDLE(xen_ulong_t);
>  
>  /* Maximum number of virtual CPUs in multi-processor guests. */
>  #define MAX_VIRT_CPUS 1
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index a9946aa..1d64c02 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -57,7 +57,7 @@ static int map_foreign_page(unsigned long lpfn, unsigned long fgmfn,
>  		.size = 1,
>  		.space = XENMAPSPACE_gmfn_foreign,
>  	};
> -	unsigned long idx = fgmfn;
> +	xen_ulong_t idx = fgmfn;
>  	xen_pfn_t gpfn = lpfn;
>  
>  	set_xen_guest_handle(xatp.idxs, &idx);
> @@ -73,7 +73,7 @@ static int map_foreign_page(unsigned long lpfn, unsigned long fgmfn,
>  }
>  
>  struct remap_data {
> -	unsigned long fgmfn; /* foreign domain's gmfn */
> +	xen_pfn_t fgmfn; /* foreign domain's gmfn */
>  	pgprot_t prot;
>  	domid_t  domid;
>  	struct vm_area_struct *vma;
> @@ -98,7 +98,7 @@ static int remap_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr,
>  
>  int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
>  			       unsigned long addr,
> -			       unsigned long mfn, int nr,
> +			       xen_pfn_t mfn, int nr,
>  			       pgprot_t prot, unsigned domid,
>  			       struct xen_remap_mfn_info *info)
>  {
> diff --git a/arch/x86/include/asm/xen/interface.h b/arch/x86/include/asm/xen/interface.h
> index 250c254..d67f3c6 100644
> --- a/arch/x86/include/asm/xen/interface.h
> +++ b/arch/x86/include/asm/xen/interface.h
> @@ -63,6 +63,7 @@ DEFINE_GUEST_HANDLE(void);
>  DEFINE_GUEST_HANDLE(uint64_t);
>  DEFINE_GUEST_HANDLE(uint32_t);
>  DEFINE_GUEST_HANDLE(xen_pfn_t);
> +DEFINE_GUEST_HANDLE(xen_ulong_t);
>  #endif
>  
>  #ifndef HYPERVISOR_VIRT_START
> diff --git a/include/xen/interface/memory.h b/include/xen/interface/memory.h
> index e5675bc..24e5731 100644
> --- a/include/xen/interface/memory.h
> +++ b/include/xen/interface/memory.h
> @@ -199,7 +199,7 @@ struct xen_add_to_physmap_range {
>      domid_t foreign_domid; /* IFF gmfn_foreign */
>  
>      /* Indexes into space being mapped. */
> -    GUEST_HANDLE(ulong) idxs;
> +    GUEST_HANDLE(xen_ulong_t) idxs;
>  
>      /* GPFN in domid where the source mapping page should appear. */
>      GUEST_HANDLE(xen_pfn_t) gpfns;
> diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
> index 2f3cb06..59309f3 100644
> --- a/include/xen/xen-ops.h
> +++ b/include/xen/xen-ops.h
> @@ -27,7 +27,7 @@ struct vm_area_struct;
>  struct xen_remap_mfn_info;
>  int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
>  			       unsigned long addr,
> -			       unsigned long mfn, int nr,
> +			       xen_pfn_t mfn, int nr,
>  			       pgprot_t prot, unsigned domid,
>  			       struct xen_remap_mfn_info *pvhp);
>  int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 05/14] xen: balloon: use correct type for frame_list

[Stefano Stabellini]

On Fri, 5 Oct 2012, Ian Campbell wrote:
> On Fri, 2012-10-05 at 15:33 +0100, Ian Campbell wrote:
> > The issue you describe could only happen for a 32 bit HAP guest if the
> > guests was given > 16GB (2^(32+PAGE_SHIFT) bytes) of RAM and it was
> > explicitly trying to balloon memory over that limit, but in order for
> > that to even be possible it would already need to have made its concept
> > of a pfn larger than 32 bits.
> 
> The one place this might matter is in the privcmd
> IOCTL_PRIVCMD_MMAPBATCH interface for the *foreign* pfn (since a small
> dom0 needs to be able to build a big domU). Luckily that interface
> already uses xen_pfn_t, we just need to be a bit careful in the
> xen_remap_domain_mfn_range case, which Konrad tried to tell me already
> and he was right...
> 
> On ARM that meant the following (built but not executed) patch, I
> suspect the PVH variant needs similar treatment.

I think you are right


> 
> diff --git a/arch/arm/include/asm/xen/interface.h b/arch/arm/include/asm/xen/interface.h
> index ae05e56..ad87917 100644
> --- a/arch/arm/include/asm/xen/interface.h
> +++ b/arch/arm/include/asm/xen/interface.h
> @@ -43,6 +43,7 @@ DEFINE_GUEST_HANDLE(void);
>  DEFINE_GUEST_HANDLE(uint64_t);
>  DEFINE_GUEST_HANDLE(uint32_t);
>  DEFINE_GUEST_HANDLE(xen_pfn_t);
> +DEFINE_GUEST_HANDLE(xen_ulong_t);
>  
>  /* Maximum number of virtual CPUs in multi-processor guests. */
>  #define MAX_VIRT_CPUS 1
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index a9946aa..1d64c02 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -57,7 +57,7 @@ static int map_foreign_page(unsigned long lpfn, unsigned long fgmfn,
>  		.size = 1,
>  		.space = XENMAPSPACE_gmfn_foreign,
>  	};
> -	unsigned long idx = fgmfn;
> +	xen_ulong_t idx = fgmfn;
>  	xen_pfn_t gpfn = lpfn;
>  
>  	set_xen_guest_handle(xatp.idxs, &idx);
> @@ -73,7 +73,7 @@ static int map_foreign_page(unsigned long lpfn, unsigned long fgmfn,
>  }
>  
>  struct remap_data {
> -	unsigned long fgmfn; /* foreign domain's gmfn */
> +	xen_pfn_t fgmfn; /* foreign domain's gmfn */
>  	pgprot_t prot;
>  	domid_t  domid;
>  	struct vm_area_struct *vma;
> @@ -98,7 +98,7 @@ static int remap_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr,
>  
>  int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
>  			       unsigned long addr,
> -			       unsigned long mfn, int nr,
> +			       xen_pfn_t mfn, int nr,
>  			       pgprot_t prot, unsigned domid,
>  			       struct xen_remap_mfn_info *info)
>  {
> diff --git a/arch/x86/include/asm/xen/interface.h b/arch/x86/include/asm/xen/interface.h
> index 250c254..d67f3c6 100644
> --- a/arch/x86/include/asm/xen/interface.h
> +++ b/arch/x86/include/asm/xen/interface.h
> @@ -63,6 +63,7 @@ DEFINE_GUEST_HANDLE(void);
>  DEFINE_GUEST_HANDLE(uint64_t);
>  DEFINE_GUEST_HANDLE(uint32_t);
>  DEFINE_GUEST_HANDLE(xen_pfn_t);
> +DEFINE_GUEST_HANDLE(xen_ulong_t);
>  #endif
>  
>  #ifndef HYPERVISOR_VIRT_START
> diff --git a/include/xen/interface/memory.h b/include/xen/interface/memory.h
> index e5675bc..24e5731 100644
> --- a/include/xen/interface/memory.h
> +++ b/include/xen/interface/memory.h
> @@ -199,7 +199,7 @@ struct xen_add_to_physmap_range {
>      domid_t foreign_domid; /* IFF gmfn_foreign */
>  
>      /* Indexes into space being mapped. */
> -    GUEST_HANDLE(ulong) idxs;
> +    GUEST_HANDLE(xen_ulong_t) idxs;
>  
>      /* GPFN in domid where the source mapping page should appear. */
>      GUEST_HANDLE(xen_pfn_t) gpfns;
> diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
> index 2f3cb06..59309f3 100644
> --- a/include/xen/xen-ops.h
> +++ b/include/xen/xen-ops.h
> @@ -27,7 +27,7 @@ struct vm_area_struct;
>  struct xen_remap_mfn_info;
>  int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
>  			       unsigned long addr,
> -			       unsigned long mfn, int nr,
> +			       xen_pfn_t mfn, int nr,
>  			       pgprot_t prot, unsigned domid,
>  			       struct xen_remap_mfn_info *pvhp);
>  int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
> 
> 
> 

[PATCH 06/14] arm: make p2m operations NOPs

[Ian Campbell]

This makes common code less ifdef-y and is consistent with PVHVM on
x86.

Also note that phys_to_machine_mapping_valid should take a pfn
argument and make it do so.

Add __set_phys_to_machine, make set_phys_to_machine a simple wrapper
(on systems with non-nop implementations the outer one can allocate
new p2m pages).

Make __set_phys_to_machine check for identity mapping or invalid only.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 arch/arm/include/asm/xen/page.h |   13 ++++++++++---
 drivers/xen/balloon.c           |    2 +-
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/arch/arm/include/asm/xen/page.h b/arch/arm/include/asm/xen/page.h
index 1742023..c6b9096 100644
--- a/arch/arm/include/asm/xen/page.h
+++ b/arch/arm/include/asm/xen/page.h
@@ -10,7 +10,7 @@
 #include <xen/interface/grant_table.h>
 
 #define pfn_to_mfn(pfn)			(pfn)
-#define phys_to_machine_mapping_valid	(1)
+#define phys_to_machine_mapping_valid(pfn) (1)
 #define mfn_to_pfn(mfn)			(mfn)
 #define mfn_to_virt(m)			(__va(mfn_to_pfn(m) << PAGE_SHIFT))
 
@@ -30,6 +30,8 @@ typedef struct xpaddr {
 #define XMADDR(x)	((xmaddr_t) { .maddr = (x) })
 #define XPADDR(x)	((xpaddr_t) { .paddr = (x) })
 
+#define INVALID_P2M_ENTRY      (~0UL)
+
 static inline xmaddr_t phys_to_machine(xpaddr_t phys)
 {
 	unsigned offset = phys.paddr & ~PAGE_MASK;
@@ -74,9 +76,14 @@ static inline int m2p_remove_override(struct page *page, bool clear_pte)
 	return 0;
 }
 
+static inline bool __set_phys_to_machine(unsigned long pfn, unsigned long mfn)
+{
+	BUG_ON(pfn != mfn && mfn != INVALID_P2M_ENTRY);
+	return true;
+}
+
 static inline bool set_phys_to_machine(unsigned long pfn, unsigned long mfn)
 {
-	BUG();
-	return false;
+	return __set_phys_to_machine(pfn, mfn);
 }
 #endif /* _ASM_ARM_XEN_PAGE_H */
diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index 4625560..7885a19 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -452,7 +452,7 @@ static enum bp_state decrease_reservation(unsigned long nr_pages, gfp_t gfp)
 	/* No more mappings: invalidate P2M and add to balloon. */
 	for (i = 0; i < nr_pages; i++) {
 		pfn = mfn_to_pfn(frame_list[i]);
-		/* PVH note: following will noop for auto translated */
+		/* The following is a noop for auto translated */
 		__set_phys_to_machine(pfn, INVALID_P2M_ENTRY);
 		balloon_append(pfn_to_page(pfn));
 	}
-- 
1.7.2.5

Re: [PATCH 06/14] arm: make p2m operations NOPs

[Stefano Stabellini]

On Thu, 4 Oct 2012, Ian Campbell wrote:
> This makes common code less ifdef-y and is consistent with PVHVM on
> x86.
> 
> Also note that phys_to_machine_mapping_valid should take a pfn
> argument and make it do so.
> 
> Add __set_phys_to_machine, make set_phys_to_machine a simple wrapper
> (on systems with non-nop implementations the outer one can allocate
> new p2m pages).
> 
> Make __set_phys_to_machine check for identity mapping or invalid only.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>


Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

>  arch/arm/include/asm/xen/page.h |   13 ++++++++++---
>  drivers/xen/balloon.c           |    2 +-
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/include/asm/xen/page.h b/arch/arm/include/asm/xen/page.h
> index 1742023..c6b9096 100644
> --- a/arch/arm/include/asm/xen/page.h
> +++ b/arch/arm/include/asm/xen/page.h
> @@ -10,7 +10,7 @@
>  #include <xen/interface/grant_table.h>
>  
>  #define pfn_to_mfn(pfn)			(pfn)
> -#define phys_to_machine_mapping_valid	(1)
> +#define phys_to_machine_mapping_valid(pfn) (1)
>  #define mfn_to_pfn(mfn)			(mfn)
>  #define mfn_to_virt(m)			(__va(mfn_to_pfn(m) << PAGE_SHIFT))
>  
> @@ -30,6 +30,8 @@ typedef struct xpaddr {
>  #define XMADDR(x)	((xmaddr_t) { .maddr = (x) })
>  #define XPADDR(x)	((xpaddr_t) { .paddr = (x) })
>  
> +#define INVALID_P2M_ENTRY      (~0UL)
> +
>  static inline xmaddr_t phys_to_machine(xpaddr_t phys)
>  {
>  	unsigned offset = phys.paddr & ~PAGE_MASK;
> @@ -74,9 +76,14 @@ static inline int m2p_remove_override(struct page *page, bool clear_pte)
>  	return 0;
>  }
>  
> +static inline bool __set_phys_to_machine(unsigned long pfn, unsigned long mfn)
> +{
> +	BUG_ON(pfn != mfn && mfn != INVALID_P2M_ENTRY);
> +	return true;
> +}
> +
>  static inline bool set_phys_to_machine(unsigned long pfn, unsigned long mfn)
>  {
> -	BUG();
> -	return false;
> +	return __set_phys_to_machine(pfn, mfn);
>  }
>  #endif /* _ASM_ARM_XEN_PAGE_H */
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index 4625560..7885a19 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -452,7 +452,7 @@ static enum bp_state decrease_reservation(unsigned long nr_pages, gfp_t gfp)
>  	/* No more mappings: invalidate P2M and add to balloon. */
>  	for (i = 0; i < nr_pages; i++) {
>  		pfn = mfn_to_pfn(frame_list[i]);
> -		/* PVH note: following will noop for auto translated */
> +		/* The following is a noop for auto translated */
>  		__set_phys_to_machine(pfn, INVALID_P2M_ENTRY);
>  		balloon_append(pfn_to_page(pfn));
>  	}
> -- 
> 1.7.2.5
> 

[PATCH 07/14] xen: balloon: do not update stage 1 pagetable for autotranslated guests

[Ian Campbell]

This is unnecessary (since the page will be removed from the p2m) and
can be tricky if the page is in the middle of a superpage, as it is on
ARM.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
Also effects PVH but in a benign way I think.
---
 drivers/xen/balloon.c |   23 +++--------------------
 1 files changed, 3 insertions(+), 20 deletions(-)

diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index 7885a19..1b56ae0 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -357,15 +357,7 @@ static enum bp_state increase_reservation(unsigned long nr_pages)
 		BUG_ON(!xen_feature(XENFEAT_auto_translated_physmap) &&
 		       phys_to_machine_mapping_valid(pfn));
 
-		if (xen_pv_domain() && 
-		    xen_feature(XENFEAT_auto_translated_physmap)) {
-			/* PVH: we just need to update native page table */
-			pte_t *ptep;
-			unsigned int level;
-			void *addr = __va(pfn << PAGE_SHIFT);
-			ptep = lookup_address((unsigned long)addr, &level);
-			set_pte(ptep, pfn_pte(pfn, PAGE_KERNEL));
-		} else
+		if (!xen_feature(XENFEAT_auto_translated_physmap))
 			set_phys_to_machine(pfn, frame_list[i]);
 
 		/* Link back into the page tables if not highmem. */
@@ -427,21 +419,12 @@ static enum bp_state decrease_reservation(unsigned long nr_pages, gfp_t gfp)
 
 		scrub_page(page);
 
-		if (xen_pv_domain() && !PageHighMem(page)) {
-			if (xen_feature(XENFEAT_auto_translated_physmap)) {
-				unsigned int level;
-				pte_t *ptep;
-				void *addr = __va(pfn << PAGE_SHIFT);
-				ptep = lookup_address((unsigned long)addr, 
-							&level);
-				set_pte(ptep, __pte(0));
-
-			} else {
+		if (xen_pv_domain() && !PageHighMem(page) &&
+		    !xen_feature(XENFEAT_auto_translated_physmap)) {
 				ret = HYPERVISOR_update_va_mapping(
 					(unsigned long)__va(pfn << PAGE_SHIFT),
 					__pte_ma(0), 0);
 				BUG_ON(ret);
-			}
 		}
 	}
 
-- 
1.7.2.5

Re: [PATCH 07/14] xen: balloon: do not update stage 1 pagetable for autotranslated guests

[Stefano Stabellini]

On Thu, 4 Oct 2012, Ian Campbell wrote:
> This is unnecessary (since the page will be removed from the p2m) and
> can be tricky if the page is in the middle of a superpage, as it is on
> ARM.
> 

I think that this patch is correct. I'll leave to Konrad whether we
should carry the original incorrect PVH patch and this separate fix or
just merge the two of them.


> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> Also effects PVH but in a benign way I think.
>
>  drivers/xen/balloon.c |   23 +++--------------------
>  1 files changed, 3 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index 7885a19..1b56ae0 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -357,15 +357,7 @@ static enum bp_state increase_reservation(unsigned long nr_pages)
>  		BUG_ON(!xen_feature(XENFEAT_auto_translated_physmap) &&
>  		       phys_to_machine_mapping_valid(pfn));
>  
> -		if (xen_pv_domain() && 
> -		    xen_feature(XENFEAT_auto_translated_physmap)) {
> -			/* PVH: we just need to update native page table */
> -			pte_t *ptep;
> -			unsigned int level;
> -			void *addr = __va(pfn << PAGE_SHIFT);
> -			ptep = lookup_address((unsigned long)addr, &level);
> -			set_pte(ptep, pfn_pte(pfn, PAGE_KERNEL));
> -		} else
> +		if (!xen_feature(XENFEAT_auto_translated_physmap))
>  			set_phys_to_machine(pfn, frame_list[i]);
>  
>  		/* Link back into the page tables if not highmem. */
> @@ -427,21 +419,12 @@ static enum bp_state decrease_reservation(unsigned long nr_pages, gfp_t gfp)
>  
>  		scrub_page(page);
>  
> -		if (xen_pv_domain() && !PageHighMem(page)) {
> -			if (xen_feature(XENFEAT_auto_translated_physmap)) {
> -				unsigned int level;
> -				pte_t *ptep;
> -				void *addr = __va(pfn << PAGE_SHIFT);
> -				ptep = lookup_address((unsigned long)addr, 
> -							&level);
> -				set_pte(ptep, __pte(0));
> -
> -			} else {
> +		if (xen_pv_domain() && !PageHighMem(page) &&
> +		    !xen_feature(XENFEAT_auto_translated_physmap)) {
>  				ret = HYPERVISOR_update_va_mapping(
>  					(unsigned long)__va(pfn << PAGE_SHIFT),
>  					__pte_ma(0), 0);
>  				BUG_ON(ret);
> -			}
>  		}
>  	}
>  
> -- 
> 1.7.2.5
> 

Re: [PATCH 07/14] xen: balloon: do not update stage 1 pagetable for autotranslated guests

[Mukesh Rathor]

On Fri, 5 Oct 2012 13:05:01 +0100
Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:

> On Thu, 4 Oct 2012, Ian Campbell wrote:
> > This is unnecessary (since the page will be removed from the p2m)
> > and can be tricky if the page is in the middle of a superpage, as
> > it is on ARM.
> > 
> 
> I think that this patch is correct. I'll leave to Konrad whether we
> should carry the original incorrect PVH patch and this separate fix or
> just merge the two of them.
> 
> 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> > Also effects PVH but in a benign way I think.
> >
> >  drivers/xen/balloon.c |   23 +++--------------------
> >  1 files changed, 3 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> > index 7885a19..1b56ae0 100644
> > --- a/drivers/xen/balloon.c
> > +++ b/drivers/xen/balloon.c
> > @@ -357,15 +357,7 @@ static enum bp_state
> > increase_reservation(unsigned long nr_pages)
> > BUG_ON(!xen_feature(XENFEAT_auto_translated_physmap) &&
> > phys_to_machine_mapping_valid(pfn)); 
> > -		if (xen_pv_domain() && 
> > -		    xen_feature(XENFEAT_auto_translated_physmap)) {
> > -			/* PVH: we just need to update native page
> > table */
> > -			pte_t *ptep;
> > -			unsigned int level;
> > -			void *addr = __va(pfn << PAGE_SHIFT);
> > -			ptep = lookup_address((unsigned long)addr,
> > &level);
> > -			set_pte(ptep, pfn_pte(pfn, PAGE_KERNEL));
> > -		} else
> > +		if (!xen_feature(XENFEAT_auto_translated_physmap))
> >  			set_phys_to_machine(pfn, frame_list[i]);
> >  
> >  		/* Link back into the page tables if not highmem.
> > */ @@ -427,21 +419,12 @@ static enum bp_state
> > decrease_reservation(unsigned long nr_pages, gfp_t gfp) 
> >  		scrub_page(page);
> >  
> > -		if (xen_pv_domain() && !PageHighMem(page)) {
> > -			if
> > (xen_feature(XENFEAT_auto_translated_physmap)) {
> > -				unsigned int level;
> > -				pte_t *ptep;
> > -				void *addr = __va(pfn <<
> > PAGE_SHIFT);
> > -				ptep = lookup_address((unsigned
> > long)addr, 
> > -							&level);
> > -				set_pte(ptep, __pte(0));
> > -
> > -			} else {
> > +		if (xen_pv_domain() && !PageHighMem(page) &&
> > +		    !xen_feature(XENFEAT_auto_translated_physmap))
> > { ret = HYPERVISOR_update_va_mapping(
> >  					(unsigned long)__va(pfn <<
> > PAGE_SHIFT), __pte_ma(0), 0);
> >  				BUG_ON(ret);
> > -			}
> >  		}
> >  	}
> >  
> > -- 
> > 1.7.2.5
> > 


I've made this change in my tree also. So, its good for PVH also. 

thanks
Mukesh

[PATCH 08/14] xen: balloon: allow PVMMU interfaces to be compiled out

[Ian Campbell]

The ARM platform has no concept of PVMMU and therefor no
HYPERVISOR_update_va_mapping et al. Allow this code to be compiled out
when not required.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 arch/x86/xen/Kconfig  |    1 +
 drivers/xen/Kconfig   |    3 +++
 drivers/xen/balloon.c |    4 ++++
 3 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/arch/x86/xen/Kconfig b/arch/x86/xen/Kconfig
index fdce49c..c31ee77 100644
--- a/arch/x86/xen/Kconfig
+++ b/arch/x86/xen/Kconfig
@@ -6,6 +6,7 @@ config XEN
 	bool "Xen guest support"
 	select PARAVIRT
 	select PARAVIRT_CLOCK
+	select XEN_HAVE_PVMMU
 	depends on X86_64 || (X86_32 && X86_PAE && !X86_VISWS)
 	depends on X86_CMPXCHG && X86_TSC
 	help
diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
index d4dffcd..9c00652 100644
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -204,4 +204,7 @@ config XEN_MCE_LOG
 	  Allow kernel fetching MCE error from Xen platform and
 	  converting it into Linux mcelog format for mcelog tools
 
+config XEN_HAVE_PVMMU
+       bool
+
 endmenu
diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index 1b56ae0..cfe47dae 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -360,6 +360,7 @@ static enum bp_state increase_reservation(unsigned long nr_pages)
 		if (!xen_feature(XENFEAT_auto_translated_physmap))
 			set_phys_to_machine(pfn, frame_list[i]);
 
+#ifdef CONFIG_XEN_HAVE_PVMMU
 		/* Link back into the page tables if not highmem. */
 		if (xen_pv_domain() && !PageHighMem(page) && 
 		    !xen_feature(XENFEAT_auto_translated_physmap)) {
@@ -371,6 +372,7 @@ static enum bp_state increase_reservation(unsigned long nr_pages)
 				0);
 			BUG_ON(ret);
 		}
+#endif
 
 		/* Relinquish the page back to the allocator. */
 		ClearPageReserved(page);
@@ -419,6 +421,7 @@ static enum bp_state decrease_reservation(unsigned long nr_pages, gfp_t gfp)
 
 		scrub_page(page);
 
+#ifdef CONFIG_XEN_HAVE_PVMMU
 		if (xen_pv_domain() && !PageHighMem(page) &&
 		    !xen_feature(XENFEAT_auto_translated_physmap)) {
 				ret = HYPERVISOR_update_va_mapping(
@@ -426,6 +429,7 @@ static enum bp_state decrease_reservation(unsigned long nr_pages, gfp_t gfp)
 					__pte_ma(0), 0);
 				BUG_ON(ret);
 		}
+#endif
 	}
 
 	/* Ensure that ballooned highmem pages don't have kmaps. */
-- 
1.7.2.5

Re: [PATCH 08/14] xen: balloon: allow PVMMU interfaces to be compiled out

[Stefano Stabellini]

On Thu, 4 Oct 2012, Ian Campbell wrote:
> The ARM platform has no concept of PVMMU and therefor no
> HYPERVISOR_update_va_mapping et al. Allow this code to be compiled out
> when not required.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

I am unsure whether it is worth a new symbol and two ifdef's


>  arch/x86/xen/Kconfig  |    1 +
>  drivers/xen/Kconfig   |    3 +++
>  drivers/xen/balloon.c |    4 ++++
>  3 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/xen/Kconfig b/arch/x86/xen/Kconfig
> index fdce49c..c31ee77 100644
> --- a/arch/x86/xen/Kconfig
> +++ b/arch/x86/xen/Kconfig
> @@ -6,6 +6,7 @@ config XEN
>  	bool "Xen guest support"
>  	select PARAVIRT
>  	select PARAVIRT_CLOCK
> +	select XEN_HAVE_PVMMU
>  	depends on X86_64 || (X86_32 && X86_PAE && !X86_VISWS)
>  	depends on X86_CMPXCHG && X86_TSC
>  	help
> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> index d4dffcd..9c00652 100644
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -204,4 +204,7 @@ config XEN_MCE_LOG
>  	  Allow kernel fetching MCE error from Xen platform and
>  	  converting it into Linux mcelog format for mcelog tools
>  
> +config XEN_HAVE_PVMMU
> +       bool
> +
>  endmenu
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index 1b56ae0..cfe47dae 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -360,6 +360,7 @@ static enum bp_state increase_reservation(unsigned long nr_pages)
>  		if (!xen_feature(XENFEAT_auto_translated_physmap))
>  			set_phys_to_machine(pfn, frame_list[i]);
>  
> +#ifdef CONFIG_XEN_HAVE_PVMMU
>  		/* Link back into the page tables if not highmem. */
>  		if (xen_pv_domain() && !PageHighMem(page) && 
>  		    !xen_feature(XENFEAT_auto_translated_physmap)) {
> @@ -371,6 +372,7 @@ static enum bp_state increase_reservation(unsigned long nr_pages)
>  				0);
>  			BUG_ON(ret);
>  		}
> +#endif
>  
>  		/* Relinquish the page back to the allocator. */
>  		ClearPageReserved(page);
> @@ -419,6 +421,7 @@ static enum bp_state decrease_reservation(unsigned long nr_pages, gfp_t gfp)
>  
>  		scrub_page(page);
>  
> +#ifdef CONFIG_XEN_HAVE_PVMMU
>  		if (xen_pv_domain() && !PageHighMem(page) &&
>  		    !xen_feature(XENFEAT_auto_translated_physmap)) {
>  				ret = HYPERVISOR_update_va_mapping(
> @@ -426,6 +429,7 @@ static enum bp_state decrease_reservation(unsigned long nr_pages, gfp_t gfp)
>  					__pte_ma(0), 0);
>  				BUG_ON(ret);
>  		}
> +#endif
>  	}
>  
>  	/* Ensure that ballooned highmem pages don't have kmaps. */
> -- 
> 1.7.2.5
> 

Re: [PATCH 08/14] xen: balloon: allow PVMMU interfaces to be compiled out

[Ian Campbell]

On Fri, 2012-10-05 at 14:19 +0100, Stefano Stabellini wrote:
> On Thu, 4 Oct 2012, Ian Campbell wrote:
> > The ARM platform has no concept of PVMMU and therefor no
> > HYPERVISOR_update_va_mapping et al. Allow this code to be compiled out
> > when not required.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> I am unsure whether it is worth a new symbol and two ifdef's

ARM doesn't compile without it, since HYPERVISOR_update_va_mapping
doesn't exist. Do you have a preferable alternative?

I'm not sure how much more of this sort of thing there will be as we
enable more features on the ARM port.

> 
> 
> >  arch/x86/xen/Kconfig  |    1 +
> >  drivers/xen/Kconfig   |    3 +++
> >  drivers/xen/balloon.c |    4 ++++
> >  3 files changed, 8 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/x86/xen/Kconfig b/arch/x86/xen/Kconfig
> > index fdce49c..c31ee77 100644
> > --- a/arch/x86/xen/Kconfig
> > +++ b/arch/x86/xen/Kconfig
> > @@ -6,6 +6,7 @@ config XEN
> >  	bool "Xen guest support"
> >  	select PARAVIRT
> >  	select PARAVIRT_CLOCK
> > +	select XEN_HAVE_PVMMU
> >  	depends on X86_64 || (X86_32 && X86_PAE && !X86_VISWS)
> >  	depends on X86_CMPXCHG && X86_TSC
> >  	help
> > diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> > index d4dffcd..9c00652 100644
> > --- a/drivers/xen/Kconfig
> > +++ b/drivers/xen/Kconfig
> > @@ -204,4 +204,7 @@ config XEN_MCE_LOG
> >  	  Allow kernel fetching MCE error from Xen platform and
> >  	  converting it into Linux mcelog format for mcelog tools
> >  
> > +config XEN_HAVE_PVMMU
> > +       bool
> > +
> >  endmenu
> > diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> > index 1b56ae0..cfe47dae 100644
> > --- a/drivers/xen/balloon.c
> > +++ b/drivers/xen/balloon.c
> > @@ -360,6 +360,7 @@ static enum bp_state increase_reservation(unsigned long nr_pages)
> >  		if (!xen_feature(XENFEAT_auto_translated_physmap))
> >  			set_phys_to_machine(pfn, frame_list[i]);
> >  
> > +#ifdef CONFIG_XEN_HAVE_PVMMU
> >  		/* Link back into the page tables if not highmem. */
> >  		if (xen_pv_domain() && !PageHighMem(page) && 
> >  		    !xen_feature(XENFEAT_auto_translated_physmap)) {
> > @@ -371,6 +372,7 @@ static enum bp_state increase_reservation(unsigned long nr_pages)
> >  				0);
> >  			BUG_ON(ret);
> >  		}
> > +#endif
> >  
> >  		/* Relinquish the page back to the allocator. */
> >  		ClearPageReserved(page);
> > @@ -419,6 +421,7 @@ static enum bp_state decrease_reservation(unsigned long nr_pages, gfp_t gfp)
> >  
> >  		scrub_page(page);
> >  
> > +#ifdef CONFIG_XEN_HAVE_PVMMU
> >  		if (xen_pv_domain() && !PageHighMem(page) &&
> >  		    !xen_feature(XENFEAT_auto_translated_physmap)) {
> >  				ret = HYPERVISOR_update_va_mapping(
> > @@ -426,6 +429,7 @@ static enum bp_state decrease_reservation(unsigned long nr_pages, gfp_t gfp)
> >  					__pte_ma(0), 0);
> >  				BUG_ON(ret);
> >  		}
> > +#endif
> >  	}
> >  
> >  	/* Ensure that ballooned highmem pages don't have kmaps. */
> > -- 
> > 1.7.2.5
> > 

Re: [PATCH 08/14] xen: balloon: allow PVMMU interfaces to be compiled out

[Stefano Stabellini]

On Fri, 5 Oct 2012, Ian Campbell wrote:
> On Fri, 2012-10-05 at 14:19 +0100, Stefano Stabellini wrote:
> > On Thu, 4 Oct 2012, Ian Campbell wrote:
> > > The ARM platform has no concept of PVMMU and therefor no
> > > HYPERVISOR_update_va_mapping et al. Allow this code to be compiled out
> > > when not required.
> > > 
> > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > I am unsure whether it is worth a new symbol and two ifdef's
> 
> ARM doesn't compile without it, since HYPERVISOR_update_va_mapping
> doesn't exist. Do you have a preferable alternative?
> 
> I'm not sure how much more of this sort of thing there will be as we
> enable more features on the ARM port.

#define HYPERVISOR_update_va_mapping(va, new_val, flags) (0)

Re: [PATCH 08/14] xen: balloon: allow PVMMU interfaces to be compiled out

[Stefano Stabellini]

On Fri, 5 Oct 2012, Stefano Stabellini wrote:
> On Fri, 5 Oct 2012, Ian Campbell wrote:
> > On Fri, 2012-10-05 at 14:19 +0100, Stefano Stabellini wrote:
> > > On Thu, 4 Oct 2012, Ian Campbell wrote:
> > > > The ARM platform has no concept of PVMMU and therefor no
> > > > HYPERVISOR_update_va_mapping et al. Allow this code to be compiled out
> > > > when not required.
> > > > 
> > > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > > 
> > > I am unsure whether it is worth a new symbol and two ifdef's
> > 
> > ARM doesn't compile without it, since HYPERVISOR_update_va_mapping
> > doesn't exist. Do you have a preferable alternative?
> > 
> > I'm not sure how much more of this sort of thing there will be as we
> > enable more features on the ARM port.
> 
> #define HYPERVISOR_update_va_mapping(va, new_val, flags) (0)
> 

actually a proper static line with a BUG would be better

Re: [PATCH 08/14] xen: balloon: allow PVMMU interfaces to be compiled out

[Ian Campbell]

On Fri, 2012-10-05 at 15:04 +0100, Stefano Stabellini wrote:
> On Fri, 5 Oct 2012, Ian Campbell wrote:
> > On Fri, 2012-10-05 at 14:19 +0100, Stefano Stabellini wrote:
> > > On Thu, 4 Oct 2012, Ian Campbell wrote:
> > > > The ARM platform has no concept of PVMMU and therefor no
> > > > HYPERVISOR_update_va_mapping et al. Allow this code to be compiled out
> > > > when not required.
> > > > 
> > > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > > 
> > > I am unsure whether it is worth a new symbol and two ifdef's
> > 
> > ARM doesn't compile without it, since HYPERVISOR_update_va_mapping
> > doesn't exist. Do you have a preferable alternative?
> > 
> > I'm not sure how much more of this sort of thing there will be as we
> > enable more features on the ARM port.
> 
> #define HYPERVISOR_update_va_mapping(va, new_val, flags) (0)

This would mean we need to start defining things like mfn_pte() and
__pte_ma() too though, I think. How far do we want to take that?

Ian.

[PATCH 09/14] xen: arm: enable balloon driver

[Ian Campbell]

Drop the *_xenballloned_pages duplicates since these are now supplied
by the balloon code.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 arch/arm/xen/enlighten.c |   23 +++++------------------
 drivers/xen/Makefile     |    4 ++--
 drivers/xen/privcmd.c    |    9 ++++-----
 3 files changed, 11 insertions(+), 25 deletions(-)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 59bcb96..ba5cc13 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -8,6 +8,7 @@
 #include <xen/features.h>
 #include <xen/platform_pci.h>
 #include <xen/xenbus.h>
+#include <xen/page.h>
 #include <asm/xen/hypervisor.h>
 #include <asm/xen/hypercall.h>
 #include <linux/interrupt.h>
@@ -29,6 +30,10 @@ struct shared_info *HYPERVISOR_shared_info = (void *)&xen_dummy_shared_info;
 
 DEFINE_PER_CPU(struct vcpu_info *, xen_vcpu);
 
+/* These are unused until we support booting "pre-ballooned" */
+unsigned long xen_released_pages;
+struct xen_memory_region xen_extra_mem[XEN_EXTRA_MEM_MAX_REGIONS] __initdata;
+
 /* TODO: to be removed */
 __read_mostly int xen_have_vector_callback;
 EXPORT_SYMBOL_GPL(xen_have_vector_callback);
@@ -148,21 +153,3 @@ static int __init xen_init_events(void)
 	return 0;
 }
 postcore_initcall(xen_init_events);
-
-/* XXX: only until balloon is properly working */
-int alloc_xenballooned_pages(int nr_pages, struct page **pages, bool highmem)
-{
-	*pages = alloc_pages(highmem ? GFP_HIGHUSER : GFP_KERNEL,
-			get_order(nr_pages));
-	if (*pages == NULL)
-		return -ENOMEM;
-	return 0;
-}
-EXPORT_SYMBOL_GPL(alloc_xenballooned_pages);
-
-void free_xenballooned_pages(int nr_pages, struct page **pages)
-{
-	kfree(*pages);
-	*pages = NULL;
-}
-EXPORT_SYMBOL_GPL(free_xenballooned_pages);
diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
index 275abfc..9a7896f 100644
--- a/drivers/xen/Makefile
+++ b/drivers/xen/Makefile
@@ -1,8 +1,8 @@
 ifneq ($(CONFIG_ARM),y)
-obj-y	+= manage.o balloon.o
+obj-y	+= manage.o
 obj-$(CONFIG_HOTPLUG_CPU)		+= cpu_hotplug.o
 endif
-obj-y	+= grant-table.o features.o events.o
+obj-y	+= grant-table.o features.o events.o balloon.o
 obj-y	+= xenbus/
 
 nostackp := $(call cc-option, -fno-stack-protector)
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 1010bf7..bf4d62a 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -200,8 +200,8 @@ static long privcmd_ioctl_mmap(void __user *udata)
 	if (!xen_initial_domain())
 		return -EPERM;
 
-	/* PVH: TBD/FIXME. For now we only support privcmd_ioctl_mmap_batch */
-	if (xen_pv_domain() && xen_feature(XENFEAT_auto_translated_physmap))
+	/* We only support privcmd_ioctl_mmap_batch for auto translated. */
+	if (xen_feature(XENFEAT_auto_translated_physmap))
 		return -ENOSYS;
 
 	if (copy_from_user(&mmapcmd, udata, sizeof(mmapcmd)))
@@ -413,7 +413,7 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
 		up_write(&mm->mmap_sem);
 		goto out;
 	}
-	if (xen_pv_domain() && xen_feature(XENFEAT_auto_translated_physmap)) {
+	if (xen_feature(XENFEAT_auto_translated_physmap)) {
 		if ((ret = pvh_privcmd_resv_pfns(vma, m.num))) {
 			up_write(&mm->mmap_sem);
 			goto out;
@@ -492,8 +492,7 @@ static void privcmd_close(struct vm_area_struct *vma)
 	int count;
 	struct xen_pvh_pfn_info *pvhp = vma ? vma->vm_private_data : NULL;
 
-	if (!xen_pv_domain()  || !pvhp ||
-	    !xen_feature(XENFEAT_auto_translated_physmap))
+	if (!pvhp || !xen_feature(XENFEAT_auto_translated_physmap))
 		return;
 
 	count = xen_unmap_domain_mfn_range(vma, pvhp);
-- 
1.7.2.5

Re: [PATCH 09/14] xen: arm: enable balloon driver

[Stefano Stabellini]

On Thu, 4 Oct 2012, Ian Campbell wrote:
> Drop the *_xenballloned_pages duplicates since these are now supplied
> by the balloon code.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>


Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

>  arch/arm/xen/enlighten.c |   23 +++++------------------
>  drivers/xen/Makefile     |    4 ++--
>  drivers/xen/privcmd.c    |    9 ++++-----
>  3 files changed, 11 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index 59bcb96..ba5cc13 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -8,6 +8,7 @@
>  #include <xen/features.h>
>  #include <xen/platform_pci.h>
>  #include <xen/xenbus.h>
> +#include <xen/page.h>
>  #include <asm/xen/hypervisor.h>
>  #include <asm/xen/hypercall.h>
>  #include <linux/interrupt.h>
> @@ -29,6 +30,10 @@ struct shared_info *HYPERVISOR_shared_info = (void *)&xen_dummy_shared_info;
>  
>  DEFINE_PER_CPU(struct vcpu_info *, xen_vcpu);
>  
> +/* These are unused until we support booting "pre-ballooned" */
> +unsigned long xen_released_pages;
> +struct xen_memory_region xen_extra_mem[XEN_EXTRA_MEM_MAX_REGIONS] __initdata;
> +
>  /* TODO: to be removed */
>  __read_mostly int xen_have_vector_callback;
>  EXPORT_SYMBOL_GPL(xen_have_vector_callback);
> @@ -148,21 +153,3 @@ static int __init xen_init_events(void)
>  	return 0;
>  }
>  postcore_initcall(xen_init_events);
> -
> -/* XXX: only until balloon is properly working */
> -int alloc_xenballooned_pages(int nr_pages, struct page **pages, bool highmem)
> -{
> -	*pages = alloc_pages(highmem ? GFP_HIGHUSER : GFP_KERNEL,
> -			get_order(nr_pages));
> -	if (*pages == NULL)
> -		return -ENOMEM;
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(alloc_xenballooned_pages);
> -
> -void free_xenballooned_pages(int nr_pages, struct page **pages)
> -{
> -	kfree(*pages);
> -	*pages = NULL;
> -}
> -EXPORT_SYMBOL_GPL(free_xenballooned_pages);
> diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
> index 275abfc..9a7896f 100644
> --- a/drivers/xen/Makefile
> +++ b/drivers/xen/Makefile
> @@ -1,8 +1,8 @@
>  ifneq ($(CONFIG_ARM),y)
> -obj-y	+= manage.o balloon.o
> +obj-y	+= manage.o
>  obj-$(CONFIG_HOTPLUG_CPU)		+= cpu_hotplug.o
>  endif
> -obj-y	+= grant-table.o features.o events.o
> +obj-y	+= grant-table.o features.o events.o balloon.o
>  obj-y	+= xenbus/
>  
>  nostackp := $(call cc-option, -fno-stack-protector)
> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index 1010bf7..bf4d62a 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -200,8 +200,8 @@ static long privcmd_ioctl_mmap(void __user *udata)
>  	if (!xen_initial_domain())
>  		return -EPERM;
>  
> -	/* PVH: TBD/FIXME. For now we only support privcmd_ioctl_mmap_batch */
> -	if (xen_pv_domain() && xen_feature(XENFEAT_auto_translated_physmap))
> +	/* We only support privcmd_ioctl_mmap_batch for auto translated. */
> +	if (xen_feature(XENFEAT_auto_translated_physmap))
>  		return -ENOSYS;
>  
>  	if (copy_from_user(&mmapcmd, udata, sizeof(mmapcmd)))
> @@ -413,7 +413,7 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
>  		up_write(&mm->mmap_sem);
>  		goto out;
>  	}
> -	if (xen_pv_domain() && xen_feature(XENFEAT_auto_translated_physmap)) {
> +	if (xen_feature(XENFEAT_auto_translated_physmap)) {
>  		if ((ret = pvh_privcmd_resv_pfns(vma, m.num))) {
>  			up_write(&mm->mmap_sem);
>  			goto out;
> @@ -492,8 +492,7 @@ static void privcmd_close(struct vm_area_struct *vma)
>  	int count;
>  	struct xen_pvh_pfn_info *pvhp = vma ? vma->vm_private_data : NULL;
>  
> -	if (!xen_pv_domain()  || !pvhp ||
> -	    !xen_feature(XENFEAT_auto_translated_physmap))
> +	if (!pvhp || !xen_feature(XENFEAT_auto_translated_physmap))
>  		return;
>  
>  	count = xen_unmap_domain_mfn_range(vma, pvhp);
> -- 
> 1.7.2.5
> 

[PATCH 10/14] privcmd: refer to autotranslate not PVH in arch interfaces / comments.

[Ian Campbell]

PVH is X86 specific while this functionality is also used on ARM.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 arch/x86/xen/mmu.c    |   10 +++++-----
 drivers/xen/privcmd.c |   46 ++++++++++++++++++++++------------------------
 include/xen/xen-ops.h |    8 ++++----
 3 files changed, 31 insertions(+), 33 deletions(-)

diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
index 26097cb..3e781f9 100644
--- a/arch/x86/xen/mmu.c
+++ b/arch/x86/xen/mmu.c
@@ -2506,7 +2506,7 @@ struct pvh_remap_data {
 	unsigned long fgmfn;		/* foreign domain's gmfn */
 	pgprot_t prot;
 	domid_t  domid;
-	struct xen_pvh_pfn_info *pvhinfop;
+	struct xen_remap_mfn_info *pvhinfop;
 };
 
 static int pvh_map_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr, 
@@ -2514,7 +2514,7 @@ static int pvh_map_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr,
 {
 	int rc;
 	struct pvh_remap_data *remapp = data;
-	struct xen_pvh_pfn_info *pvhp = remapp->pvhinfop;
+	struct xen_remap_mfn_info *pvhp = remapp->pvhinfop;
 	unsigned long pfn = page_to_pfn(pvhp->pi_paga[pvhp->pi_next_todo++]);
 	pte_t pteval = pte_mkspecial(pfn_pte(pfn, remapp->prot));
 
@@ -2531,7 +2531,7 @@ static int pvh_map_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr,
 static int pvh_remap_gmfn_range(struct vm_area_struct *vma,
 				unsigned long addr, unsigned long mfn, int nr,
 				pgprot_t prot, unsigned domid,
-				struct xen_pvh_pfn_info *pvhp)
+				struct xen_remap_mfn_info *pvhp)
 {
 	int err;
 	struct pvh_remap_data pvhdata;
@@ -2574,7 +2574,7 @@ int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
 			       unsigned long addr,
 			       unsigned long mfn, int nr,
 			       pgprot_t prot, unsigned domid,
-			       struct xen_pvh_pfn_info *pvhp)
+			       struct xen_remap_mfn_info *pvhp)
 
 {
 	struct remap_data rmd;
@@ -2629,7 +2629,7 @@ EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);
 
 /* Returns: Number of pages unmapped */
 int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
-			       struct xen_pvh_pfn_info *pvhp)
+			       struct xen_remap_mfn_info *pvhp)
 {
 	int count = 0;
 
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index bf4d62a..ebf3c8d 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -265,18 +265,16 @@ struct mmap_batch_state {
 	xen_pfn_t __user *user_mfn;
 };
 
-/* PVH dom0 fyi: if domU being created is PV, then mfn is mfn(addr on bus). If
- * it's PVH then mfn is pfn (input to HAP). */
 static int mmap_batch_fn(void *data, void *state)
 {
 	xen_pfn_t *mfnp = data;
 	struct mmap_batch_state *st = state;
 	struct vm_area_struct *vma = st->vma;
-	struct xen_pvh_pfn_info *pvhp = vma ? vma->vm_private_data : NULL;
+	struct xen_remap_mfn_info *info = vma ? vma->vm_private_data : NULL;
 	int ret;
 
 	ret = xen_remap_domain_mfn_range(st->vma, st->va & PAGE_MASK, *mfnp, 1,
-					 st->vma->vm_page_prot, st->domain, pvhp);
+					 st->vma->vm_page_prot, st->domain, info);
 
 	/* Store error code for second pass. */
 	*(st->err++) = ret;
@@ -315,33 +313,33 @@ static int mmap_return_errors_v1(void *data, void *state)
 /* Allocate pfns that are then mapped with gmfns from foreign domid. Update
  * the vma with the page info to use later.
  * Returns: 0 if success, otherwise -errno
- */ 
-static int pvh_privcmd_resv_pfns(struct vm_area_struct *vma, int numpgs)
+ */
+static int alloc_empty_pages(struct vm_area_struct *vma, int numpgs)
 {
 	int rc;
-	struct xen_pvh_pfn_info *pvhp;
+	struct xen_remap_mfn_info *info;
 
-	pvhp = kzalloc(sizeof(struct xen_pvh_pfn_info), GFP_KERNEL);
-	if (pvhp == NULL)
+	info = kzalloc(sizeof(struct xen_remap_mfn_info), GFP_KERNEL);
+	if (info == NULL)
 		return -ENOMEM;
 
-	pvhp->pi_paga = kcalloc(numpgs, sizeof(pvhp->pi_paga[0]), GFP_KERNEL);
-	if (pvhp->pi_paga == NULL) {
-		kfree(pvhp);
+	info->pi_paga = kcalloc(numpgs, sizeof(info->pi_paga[0]), GFP_KERNEL);
+	if (info->pi_paga == NULL) {
+		kfree(info);
 		return -ENOMEM;
 	}
 
-	rc = alloc_xenballooned_pages(numpgs, pvhp->pi_paga, 0);
+	rc = alloc_xenballooned_pages(numpgs, info->pi_paga, 0);
 	if (rc != 0) {
 		pr_warn("%s Could not alloc %d pfns rc:%d\n", __FUNCTION__, 
 			numpgs, rc);
-		kfree(pvhp->pi_paga);
-		kfree(pvhp);
+		kfree(info->pi_paga);
+		kfree(info);
 		return -ENOMEM;
 	}
-	pvhp->pi_num_pgs = numpgs;
+	info->pi_num_pgs = numpgs;
 	BUG_ON(vma->vm_private_data != (void *)1);
-	vma->vm_private_data = pvhp;
+	vma->vm_private_data = info;
 
 	return 0;
 }
@@ -414,7 +412,7 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
 		goto out;
 	}
 	if (xen_feature(XENFEAT_auto_translated_physmap)) {
-		if ((ret = pvh_privcmd_resv_pfns(vma, m.num))) {
+		if ((ret = alloc_empty_pages(vma, m.num))) {
 			up_write(&mm->mmap_sem);
 			goto out;
 		}
@@ -490,16 +488,16 @@ static long privcmd_ioctl(struct file *file,
 static void privcmd_close(struct vm_area_struct *vma)
 {
 	int count;
-	struct xen_pvh_pfn_info *pvhp = vma ? vma->vm_private_data : NULL;
+	struct xen_remap_mfn_info *info = vma ? vma->vm_private_data : NULL;
 
-	if (!pvhp || !xen_feature(XENFEAT_auto_translated_physmap))
+	if (!info || !xen_feature(XENFEAT_auto_translated_physmap))
 		return;
 
-	count = xen_unmap_domain_mfn_range(vma, pvhp);
+	count = xen_unmap_domain_mfn_range(vma, info);
 	while (count--)
-		free_xenballooned_pages(1, &pvhp->pi_paga[count]);
-	kfree(pvhp->pi_paga);
-	kfree(pvhp);
+		free_xenballooned_pages(1, &info->pi_paga[count]);
+	kfree(info->pi_paga);
+	kfree(info);
 }
 
 static int privcmd_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
index 6c5ad83..2f3cb06 100644
--- a/include/xen/xen-ops.h
+++ b/include/xen/xen-ops.h
@@ -24,16 +24,16 @@ int xen_create_contiguous_region(unsigned long vstart, unsigned int order,
 void xen_destroy_contiguous_region(unsigned long vstart, unsigned int order);
 
 struct vm_area_struct;
-struct xen_pvh_pfn_info;
+struct xen_remap_mfn_info;
 int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
 			       unsigned long addr,
 			       unsigned long mfn, int nr,
 			       pgprot_t prot, unsigned domid,
-			       struct xen_pvh_pfn_info *pvhp);
+			       struct xen_remap_mfn_info *pvhp);
 int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
-			       struct xen_pvh_pfn_info *pvhp);
+			       struct xen_remap_mfn_info *pvhp);
 
-struct xen_pvh_pfn_info {
+struct xen_remap_mfn_info {
 	struct page **pi_paga;		/* pfn info page array */
 	int 	      pi_num_pgs;
 	int 	      pi_next_todo;
-- 
1.7.2.5

Re: [PATCH 10/14] privcmd: refer to autotranslate not PVH in arch interfaces / comments.

[Stefano Stabellini]

On Thu, 4 Oct 2012, Ian Campbell wrote:
> PVH is X86 specific while this functionality is also used on ARM.

I really think that this should be merged with the orignal PVH patch


> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  arch/x86/xen/mmu.c    |   10 +++++-----
>  drivers/xen/privcmd.c |   46 ++++++++++++++++++++++------------------------
>  include/xen/xen-ops.h |    8 ++++----
>  3 files changed, 31 insertions(+), 33 deletions(-)
> 
> diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
> index 26097cb..3e781f9 100644
> --- a/arch/x86/xen/mmu.c
> +++ b/arch/x86/xen/mmu.c
> @@ -2506,7 +2506,7 @@ struct pvh_remap_data {
>  	unsigned long fgmfn;		/* foreign domain's gmfn */
>  	pgprot_t prot;
>  	domid_t  domid;
> -	struct xen_pvh_pfn_info *pvhinfop;
> +	struct xen_remap_mfn_info *pvhinfop;
>  };
>  
>  static int pvh_map_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr, 
> @@ -2514,7 +2514,7 @@ static int pvh_map_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr,
>  {
>  	int rc;
>  	struct pvh_remap_data *remapp = data;
> -	struct xen_pvh_pfn_info *pvhp = remapp->pvhinfop;
> +	struct xen_remap_mfn_info *pvhp = remapp->pvhinfop;
>  	unsigned long pfn = page_to_pfn(pvhp->pi_paga[pvhp->pi_next_todo++]);
>  	pte_t pteval = pte_mkspecial(pfn_pte(pfn, remapp->prot));
>  
> @@ -2531,7 +2531,7 @@ static int pvh_map_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr,
>  static int pvh_remap_gmfn_range(struct vm_area_struct *vma,
>  				unsigned long addr, unsigned long mfn, int nr,
>  				pgprot_t prot, unsigned domid,
> -				struct xen_pvh_pfn_info *pvhp)
> +				struct xen_remap_mfn_info *pvhp)
>  {
>  	int err;
>  	struct pvh_remap_data pvhdata;
> @@ -2574,7 +2574,7 @@ int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
>  			       unsigned long addr,
>  			       unsigned long mfn, int nr,
>  			       pgprot_t prot, unsigned domid,
> -			       struct xen_pvh_pfn_info *pvhp)
> +			       struct xen_remap_mfn_info *pvhp)
>  
>  {
>  	struct remap_data rmd;
> @@ -2629,7 +2629,7 @@ EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);
>  
>  /* Returns: Number of pages unmapped */
>  int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
> -			       struct xen_pvh_pfn_info *pvhp)
> +			       struct xen_remap_mfn_info *pvhp)
>  {
>  	int count = 0;
>  
> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index bf4d62a..ebf3c8d 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -265,18 +265,16 @@ struct mmap_batch_state {
>  	xen_pfn_t __user *user_mfn;
>  };
>  
> -/* PVH dom0 fyi: if domU being created is PV, then mfn is mfn(addr on bus). If
> - * it's PVH then mfn is pfn (input to HAP). */
>  static int mmap_batch_fn(void *data, void *state)
>  {
>  	xen_pfn_t *mfnp = data;
>  	struct mmap_batch_state *st = state;
>  	struct vm_area_struct *vma = st->vma;
> -	struct xen_pvh_pfn_info *pvhp = vma ? vma->vm_private_data : NULL;
> +	struct xen_remap_mfn_info *info = vma ? vma->vm_private_data : NULL;
>  	int ret;
>  
>  	ret = xen_remap_domain_mfn_range(st->vma, st->va & PAGE_MASK, *mfnp, 1,
> -					 st->vma->vm_page_prot, st->domain, pvhp);
> +					 st->vma->vm_page_prot, st->domain, info);
>  
>  	/* Store error code for second pass. */
>  	*(st->err++) = ret;
> @@ -315,33 +313,33 @@ static int mmap_return_errors_v1(void *data, void *state)
>  /* Allocate pfns that are then mapped with gmfns from foreign domid. Update
>   * the vma with the page info to use later.
>   * Returns: 0 if success, otherwise -errno
> - */ 
> -static int pvh_privcmd_resv_pfns(struct vm_area_struct *vma, int numpgs)
> + */
> +static int alloc_empty_pages(struct vm_area_struct *vma, int numpgs)
>  {
>  	int rc;
> -	struct xen_pvh_pfn_info *pvhp;
> +	struct xen_remap_mfn_info *info;
>  
> -	pvhp = kzalloc(sizeof(struct xen_pvh_pfn_info), GFP_KERNEL);
> -	if (pvhp == NULL)
> +	info = kzalloc(sizeof(struct xen_remap_mfn_info), GFP_KERNEL);
> +	if (info == NULL)
>  		return -ENOMEM;
>  
> -	pvhp->pi_paga = kcalloc(numpgs, sizeof(pvhp->pi_paga[0]), GFP_KERNEL);
> -	if (pvhp->pi_paga == NULL) {
> -		kfree(pvhp);
> +	info->pi_paga = kcalloc(numpgs, sizeof(info->pi_paga[0]), GFP_KERNEL);
> +	if (info->pi_paga == NULL) {
> +		kfree(info);
>  		return -ENOMEM;
>  	}
>  
> -	rc = alloc_xenballooned_pages(numpgs, pvhp->pi_paga, 0);
> +	rc = alloc_xenballooned_pages(numpgs, info->pi_paga, 0);
>  	if (rc != 0) {
>  		pr_warn("%s Could not alloc %d pfns rc:%d\n", __FUNCTION__, 
>  			numpgs, rc);
> -		kfree(pvhp->pi_paga);
> -		kfree(pvhp);
> +		kfree(info->pi_paga);
> +		kfree(info);
>  		return -ENOMEM;
>  	}
> -	pvhp->pi_num_pgs = numpgs;
> +	info->pi_num_pgs = numpgs;
>  	BUG_ON(vma->vm_private_data != (void *)1);
> -	vma->vm_private_data = pvhp;
> +	vma->vm_private_data = info;
>  
>  	return 0;
>  }
> @@ -414,7 +412,7 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
>  		goto out;
>  	}
>  	if (xen_feature(XENFEAT_auto_translated_physmap)) {
> -		if ((ret = pvh_privcmd_resv_pfns(vma, m.num))) {
> +		if ((ret = alloc_empty_pages(vma, m.num))) {
>  			up_write(&mm->mmap_sem);
>  			goto out;
>  		}
> @@ -490,16 +488,16 @@ static long privcmd_ioctl(struct file *file,
>  static void privcmd_close(struct vm_area_struct *vma)
>  {
>  	int count;
> -	struct xen_pvh_pfn_info *pvhp = vma ? vma->vm_private_data : NULL;
> +	struct xen_remap_mfn_info *info = vma ? vma->vm_private_data : NULL;
>  
> -	if (!pvhp || !xen_feature(XENFEAT_auto_translated_physmap))
> +	if (!info || !xen_feature(XENFEAT_auto_translated_physmap))
>  		return;
>  
> -	count = xen_unmap_domain_mfn_range(vma, pvhp);
> +	count = xen_unmap_domain_mfn_range(vma, info);
>  	while (count--)
> -		free_xenballooned_pages(1, &pvhp->pi_paga[count]);
> -	kfree(pvhp->pi_paga);
> -	kfree(pvhp);
> +		free_xenballooned_pages(1, &info->pi_paga[count]);
> +	kfree(info->pi_paga);
> +	kfree(info);
>  }
>  
>  static int privcmd_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
> index 6c5ad83..2f3cb06 100644
> --- a/include/xen/xen-ops.h
> +++ b/include/xen/xen-ops.h
> @@ -24,16 +24,16 @@ int xen_create_contiguous_region(unsigned long vstart, unsigned int order,
>  void xen_destroy_contiguous_region(unsigned long vstart, unsigned int order);
>  
>  struct vm_area_struct;
> -struct xen_pvh_pfn_info;
> +struct xen_remap_mfn_info;
>  int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
>  			       unsigned long addr,
>  			       unsigned long mfn, int nr,
>  			       pgprot_t prot, unsigned domid,
> -			       struct xen_pvh_pfn_info *pvhp);
> +			       struct xen_remap_mfn_info *pvhp);
>  int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
> -			       struct xen_pvh_pfn_info *pvhp);
> +			       struct xen_remap_mfn_info *pvhp);
>  
> -struct xen_pvh_pfn_info {
> +struct xen_remap_mfn_info {
>  	struct page **pi_paga;		/* pfn info page array */
>  	int 	      pi_num_pgs;
>  	int 	      pi_next_todo;
> -- 
> 1.7.2.5
> 

Re: [PATCH 10/14] privcmd: refer to autotranslate not PVH in arch interfaces / comments.

[Ian Campbell]

On Fri, 2012-10-05 at 14:37 +0100, Stefano Stabellini wrote:
> On Thu, 4 Oct 2012, Ian Campbell wrote:
> > PVH is X86 specific while this functionality is also used on ARM.
> 
> I really think that this should be merged with the orignal PVH patch

Agreed, I should have said as much.

[PATCH 11/14] arm: implement remap interfaces needed for privcmd mappings.

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 arch/arm/xen/enlighten.c |   94 +++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 92 insertions(+), 2 deletions(-)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index ba5cc13..9956af5 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -9,6 +9,7 @@
 #include <xen/platform_pci.h>
 #include <xen/xenbus.h>
 #include <xen/page.h>
+#include <xen/xen-ops.h>
 #include <asm/xen/hypervisor.h>
 #include <asm/xen/hypercall.h>
 #include <linux/interrupt.h>
@@ -18,6 +19,8 @@
 #include <linux/of_irq.h>
 #include <linux/of_address.h>
 
+#include <linux/mm.h>
+
 struct start_info _xen_start_info;
 struct start_info *xen_start_info = &_xen_start_info;
 EXPORT_SYMBOL_GPL(xen_start_info);
@@ -43,15 +46,102 @@ EXPORT_SYMBOL_GPL(xen_platform_pci_unplug);
 
 static __read_mostly int xen_events_irq = -1;
 
+/* map fgmfn of domid to lpfn in the current domain */
+static int map_foreign_page(unsigned long lpfn, unsigned long fgmfn,
+			    unsigned int domid)
+{
+	int rc;
+	struct xen_add_to_physmap xatp = {
+		.domid = DOMID_SELF,
+		.u.foreign_domid = domid,
+		.space = XENMAPSPACE_gmfn_foreign,
+		.idx = fgmfn,
+		.gpfn = lpfn,
+	};
+
+	rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp);
+	if (rc) {
+		pr_warn("Failed to map pfn to mfn rc:%d pfn:%lx mfn:%lx\n",
+			rc, lpfn, fgmfn);
+		return 1;
+	}
+	return 0;
+}
+
+struct remap_data {
+	unsigned long fgmfn; /* foreign domain's gmfn */
+	pgprot_t prot;
+	domid_t  domid;
+	struct vm_area_struct *vma;
+	struct xen_remap_mfn_info *info;
+};
+
+static int remap_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr,
+			void *data)
+{
+	struct remap_data *info = data;
+	struct xen_remap_mfn_info *savp = info->info;
+	struct page *page = savp->pi_paga[savp->pi_next_todo++];
+	unsigned long pfn = page_to_pfn(page);
+	pte_t pte = pfn_pte(pfn, info->prot);
+
+	if (map_foreign_page(pfn, info->fgmfn, info->domid))
+		return -EFAULT;
+	set_pte_at(info->vma->vm_mm, addr, ptep, pte);
+
+	return 0;
+}
+
 int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
 			       unsigned long addr,
 			       unsigned long mfn, int nr,
-			       pgprot_t prot, unsigned domid)
+			       pgprot_t prot, unsigned domid,
+			       struct xen_remap_mfn_info *info)
 {
-	return -ENOSYS;
+	int err;
+	struct remap_data data;
+
+	/* TBD: Batching, current sole caller only does page at a time */
+	if (nr > 1)
+		return -EINVAL;
+
+	data.fgmfn = mfn;
+	data.prot = prot;
+	data.domid = domid;
+	data.vma = vma;
+	data.info = info;
+	err = apply_to_page_range(vma->vm_mm, addr, nr << PAGE_SHIFT,
+				  remap_pte_fn, &data);
+	return err;
 }
 EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);
 
+/* Returns: Number of pages unmapped */
+int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
+			       struct xen_remap_mfn_info *info)
+{
+	int count = 0;
+
+	while (info->pi_next_todo--) {
+		struct xen_remove_from_physmap xrp;
+		unsigned long rc, pfn;
+
+		pfn = page_to_pfn(info->pi_paga[info->pi_next_todo]);
+
+		xrp.domid = DOMID_SELF;
+		xrp.gpfn = pfn;
+		rc = HYPERVISOR_memory_op(XENMEM_remove_from_physmap, &xrp);
+		if (rc) {
+			pr_warn("Failed to unmap pfn:%lx rc:%ld\n",
+				pfn, rc);
+			return count;
+		}
+		count++;
+	}
+	return count;
+}
+EXPORT_SYMBOL_GPL(xen_unmap_domain_mfn_range);
+
 /*
  * see Documentation/devicetree/bindings/arm/xen.txt for the
  * documentation of the Xen Device Tree format.
-- 
1.7.2.5

Re: [PATCH 11/14] arm: implement remap interfaces needed for privcmd mappings.

[Konrad Rzeszutek Wilk]

On Thu, Oct 04, 2012 at 04:11:52PM +0100, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  arch/arm/xen/enlighten.c |   94 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 92 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index ba5cc13..9956af5 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -9,6 +9,7 @@
>  #include <xen/platform_pci.h>
>  #include <xen/xenbus.h>
>  #include <xen/page.h>
> +#include <xen/xen-ops.h>
>  #include <asm/xen/hypervisor.h>
>  #include <asm/xen/hypercall.h>
>  #include <linux/interrupt.h>
> @@ -18,6 +19,8 @@
>  #include <linux/of_irq.h>
>  #include <linux/of_address.h>
>  
> +#include <linux/mm.h>
> +
>  struct start_info _xen_start_info;
>  struct start_info *xen_start_info = &_xen_start_info;
>  EXPORT_SYMBOL_GPL(xen_start_info);
> @@ -43,15 +46,102 @@ EXPORT_SYMBOL_GPL(xen_platform_pci_unplug);
>  
>  static __read_mostly int xen_events_irq = -1;
>  
> +/* map fgmfn of domid to lpfn in the current domain */

<laughs> Say that really fast a couple of times :-)

Any way we can explain it a bit more of what each acronym means?

> +static int map_foreign_page(unsigned long lpfn, unsigned long fgmfn,
> +			    unsigned int domid)
> +{
> +	int rc;
> +	struct xen_add_to_physmap xatp = {
> +		.domid = DOMID_SELF,
> +		.u.foreign_domid = domid,
> +		.space = XENMAPSPACE_gmfn_foreign,
> +		.idx = fgmfn,
> +		.gpfn = lpfn,
> +	};
> +
> +	rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp);
> +	if (rc) {
> +		pr_warn("Failed to map pfn to mfn rc:%d pfn:%lx mfn:%lx\n",

How about 'Failed to map pfn (0x%lx) to mfn (0x%lx), rc: %d\n" ?

> +			rc, lpfn, fgmfn);
> +		return 1;
> +	}
> +	return 0;
> +}
> +
> +struct remap_data {
> +	unsigned long fgmfn; /* foreign domain's gmfn */

Shouldn't this be called now 'xen_pfn_t' or something.

> +	pgprot_t prot;
> +	domid_t  domid;
> +	struct vm_area_struct *vma;
> +	struct xen_remap_mfn_info *info;
> +};
> +
> +static int remap_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr,
> +			void *data)
> +{
> +	struct remap_data *info = data;
> +	struct xen_remap_mfn_info *savp = info->info;
> +	struct page *page = savp->pi_paga[savp->pi_next_todo++];
> +	unsigned long pfn = page_to_pfn(page);
> +	pte_t pte = pfn_pte(pfn, info->prot);
> +
> +	if (map_foreign_page(pfn, info->fgmfn, info->domid))
> +		return -EFAULT;
> +	set_pte_at(info->vma->vm_mm, addr, ptep, pte);
> +
> +	return 0;
> +}
> +
>  int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
>  			       unsigned long addr,
>  			       unsigned long mfn, int nr,
> -			       pgprot_t prot, unsigned domid)
> +			       pgprot_t prot, unsigned domid,
> +			       struct xen_remap_mfn_info *info)
>  {
> -	return -ENOSYS;
> +	int err;
> +	struct remap_data data;
> +
> +	/* TBD: Batching, current sole caller only does page at a time */
> +	if (nr > 1)

Lets also wrap it with WARN_ON?

> +		return -EINVAL;
> +
> +	data.fgmfn = mfn;
> +	data.prot = prot;
> +	data.domid = domid;
> +	data.vma = vma;
> +	data.info = info;
> +	err = apply_to_page_range(vma->vm_mm, addr, nr << PAGE_SHIFT,
> +				  remap_pte_fn, &data);
> +	return err;
>  }
>  EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);
>  
> +/* Returns: Number of pages unmapped */
> +int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
> +			       struct xen_remap_mfn_info *info)
> +{
> +	int count = 0;
> +
> +	while (info->pi_next_todo--) {
> +		struct xen_remove_from_physmap xrp;
> +		unsigned long rc, pfn;
> +
> +		pfn = page_to_pfn(info->pi_paga[info->pi_next_todo]);

Won't this miss the first pi_next_todo? You did the 'pi_next_todo--' so
will the compiler decrement it here in this operation or will it do
when it gets to the 'do' logic of the loop?

> +
> +		xrp.domid = DOMID_SELF;
> +		xrp.gpfn = pfn;
> +		rc = HYPERVISOR_memory_op(XENMEM_remove_from_physmap, &xrp);

'rc' is 'unsigned long'. Is that right? You don't want it to be 'int'?

> +		if (rc) {
> +			pr_warn("Failed to unmap pfn:%lx rc:%ld\n",
> +				pfn, rc);
> +			return count;
> +		}
> +		count++;
> +	}
> +	return count;
> +}
> +EXPORT_SYMBOL_GPL(xen_unmap_domain_mfn_range);
> +
>  /*
>   * see Documentation/devicetree/bindings/arm/xen.txt for the
>   * documentation of the Xen Device Tree format.
> -- 
> 1.7.2.5

Re: [PATCH 11/14] arm: implement remap interfaces needed for privcmd mappings.

[Ian Campbell]

On Thu, 2012-10-04 at 16:24 +0100, Konrad Rzeszutek Wilk wrote:
> On Thu, Oct 04, 2012 at 04:11:52PM +0100, Ian Campbell wrote:
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  arch/arm/xen/enlighten.c |   94 +++++++++++++++++++++++++++++++++++++++++++++-
> >  1 files changed, 92 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> > index ba5cc13..9956af5 100644
> > --- a/arch/arm/xen/enlighten.c
> > +++ b/arch/arm/xen/enlighten.c
> > @@ -9,6 +9,7 @@
> >  #include <xen/platform_pci.h>
> >  #include <xen/xenbus.h>
> >  #include <xen/page.h>
> > +#include <xen/xen-ops.h>
> >  #include <asm/xen/hypervisor.h>
> >  #include <asm/xen/hypercall.h>
> >  #include <linux/interrupt.h>
> > @@ -18,6 +19,8 @@
> >  #include <linux/of_irq.h>
> >  #include <linux/of_address.h>
> >  
> > +#include <linux/mm.h>
> > +
> >  struct start_info _xen_start_info;
> >  struct start_info *xen_start_info = &_xen_start_info;
> >  EXPORT_SYMBOL_GPL(xen_start_info);
> > @@ -43,15 +46,102 @@ EXPORT_SYMBOL_GPL(xen_platform_pci_unplug);
> >  
> >  static __read_mostly int xen_events_irq = -1;
> >  
> > +/* map fgmfn of domid to lpfn in the current domain */
> 
> <laughs> Say that really fast a couple of times :-)
> 
> Any way we can explain it a bit more of what each acronym means?

The names come from the x86 PVH version, which has the comment:
/* Map foreign gmfn, fgmfn, to local pfn, lpfn. This for the user space 
 * creating new guest on PVH dom0 and needs to map domU pages. 
 */
Is that preferable? (modulo removing the PVH bit)

> 
> > +static int map_foreign_page(unsigned long lpfn, unsigned long fgmfn,
> > +			    unsigned int domid)
> > +{
> > +	int rc;
> > +	struct xen_add_to_physmap xatp = {
> > +		.domid = DOMID_SELF,
> > +		.u.foreign_domid = domid,
> > +		.space = XENMAPSPACE_gmfn_foreign,
> > +		.idx = fgmfn,
> > +		.gpfn = lpfn,
> > +	};
> > +
> > +	rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp);
> > +	if (rc) {
> > +		pr_warn("Failed to map pfn to mfn rc:%d pfn:%lx mfn:%lx\n",
> 
> How about 'Failed to map pfn (0x%lx) to mfn (0x%lx), rc: %d\n" ?

Sure, need to slip foreign domid in there somewhere now I look at it.

x86 PVH has basically the same print BTW.

> 
> > +			rc, lpfn, fgmfn);
> > +		return 1;
> > +	}
> > +	return 0;
> > +}
> > +
> > +struct remap_data {
> > +	unsigned long fgmfn; /* foreign domain's gmfn */
> 
> Shouldn't this be called now 'xen_pfn_t' or something.

xen_pfn_t is needed at the hypervisor interface layer, I'm not so sure
about kernel internal use. I guess it can't hurt?

> 
> > +	pgprot_t prot;
> > +	domid_t  domid;
> > +	struct vm_area_struct *vma;
> > +	struct xen_remap_mfn_info *info;
> > +};
> > +
> > +static int remap_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr,
> > +			void *data)
> > +{
> > +	struct remap_data *info = data;
> > +	struct xen_remap_mfn_info *savp = info->info;
> > +	struct page *page = savp->pi_paga[savp->pi_next_todo++];
> > +	unsigned long pfn = page_to_pfn(page);
> > +	pte_t pte = pfn_pte(pfn, info->prot);
> > +
> > +	if (map_foreign_page(pfn, info->fgmfn, info->domid))
> > +		return -EFAULT;
> > +	set_pte_at(info->vma->vm_mm, addr, ptep, pte);
> > +
> > +	return 0;
> > +}
> > +
> >  int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
> >  			       unsigned long addr,
> >  			       unsigned long mfn, int nr,
> > -			       pgprot_t prot, unsigned domid)
> > +			       pgprot_t prot, unsigned domid,
> > +			       struct xen_remap_mfn_info *info)
> >  {
> > -	return -ENOSYS;
> > +	int err;
> > +	struct remap_data data;
> > +
> > +	/* TBD: Batching, current sole caller only does page at a time */
> > +	if (nr > 1)
> 
> Lets also wrap it with WARN_ON?

ACK, needs doing on x86 PVH too then.

> 
> > +		return -EINVAL;
> > +
> > +	data.fgmfn = mfn;
> > +	data.prot = prot;
> > +	data.domid = domid;
> > +	data.vma = vma;
> > +	data.info = info;
> > +	err = apply_to_page_range(vma->vm_mm, addr, nr << PAGE_SHIFT,
> > +				  remap_pte_fn, &data);
> > +	return err;
> >  }
> >  EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);
> >  
> > +/* Returns: Number of pages unmapped */
> > +int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
> > +			       struct xen_remap_mfn_info *info)
> > +{
> > +	int count = 0;
> > +
> > +	while (info->pi_next_todo--) {
> > +		struct xen_remove_from_physmap xrp;
> > +		unsigned long rc, pfn;
> > +
> > +		pfn = page_to_pfn(info->pi_paga[info->pi_next_todo]);
> 
> Won't this miss the first pi_next_todo? You did the 'pi_next_todo--' so
> will the compiler decrement it here in this operation or will it do
> when it gets to the 'do' logic of the loop?

It's a post decrement so if pi_next_todo == 1 then the expression in the
while will see 1 (true) but inside the loop we see zero. So we end up
processing elements N-1..0 of the array which is correct.

This is the same as on x86 PVH, so if I'm wrong then that is too.

I mentioned in the PVH thread this morning that I think this interface
should drop pi_next_todo and have a simple for loop based on the number
of pages between vm_start...vm_end here.

> 
> > +
> > +		xrp.domid = DOMID_SELF;
> > +		xrp.gpfn = pfn;
> > +		rc = HYPERVISOR_memory_op(XENMEM_remove_from_physmap, &xrp);
> 
> 'rc' is 'unsigned long'. Is that right? You don't want it to be 'int'?

Hypercalls return unsigned long these days, I think it was considered a
mistake that some didn't. See <25744:47080c965937> in the hypervisor
tree. 

Oh, wait, we are both wrong -- it's a long. I'll fix that...

> 
> > +		if (rc) {
> > +			pr_warn("Failed to unmap pfn:%lx rc:%ld\n",
> > +				pfn, rc);
> > +			return count;
> > +		}
> > +		count++;
> > +	}
> > +	return count;
> > +}
> > +EXPORT_SYMBOL_GPL(xen_unmap_domain_mfn_range);
> > +
> >  /*
> >   * see Documentation/devicetree/bindings/arm/xen.txt for the
> >   * documentation of the Xen Device Tree format.
> > -- 
> > 1.7.2.5

Re: [PATCH 11/14] arm: implement remap interfaces needed for privcmd mappings.

[Konrad Rzeszutek Wilk]

On Thu, Oct 04, 2012 at 04:48:38PM +0100, Ian Campbell wrote:
> On Thu, 2012-10-04 at 16:24 +0100, Konrad Rzeszutek Wilk wrote:
> > On Thu, Oct 04, 2012 at 04:11:52PM +0100, Ian Campbell wrote:
> > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > > ---
> > >  arch/arm/xen/enlighten.c |   94 +++++++++++++++++++++++++++++++++++++++++++++-
> > >  1 files changed, 92 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> > > index ba5cc13..9956af5 100644
> > > --- a/arch/arm/xen/enlighten.c
> > > +++ b/arch/arm/xen/enlighten.c
> > > @@ -9,6 +9,7 @@
> > >  #include <xen/platform_pci.h>
> > >  #include <xen/xenbus.h>
> > >  #include <xen/page.h>
> > > +#include <xen/xen-ops.h>
> > >  #include <asm/xen/hypervisor.h>
> > >  #include <asm/xen/hypercall.h>
> > >  #include <linux/interrupt.h>
> > > @@ -18,6 +19,8 @@
> > >  #include <linux/of_irq.h>
> > >  #include <linux/of_address.h>
> > >  
> > > +#include <linux/mm.h>
> > > +
> > >  struct start_info _xen_start_info;
> > >  struct start_info *xen_start_info = &_xen_start_info;
> > >  EXPORT_SYMBOL_GPL(xen_start_info);
> > > @@ -43,15 +46,102 @@ EXPORT_SYMBOL_GPL(xen_platform_pci_unplug);
> > >  
> > >  static __read_mostly int xen_events_irq = -1;
> > >  
> > > +/* map fgmfn of domid to lpfn in the current domain */
> > 
> > <laughs> Say that really fast a couple of times :-)
> > 
> > Any way we can explain it a bit more of what each acronym means?
> 
> The names come from the x86 PVH version, which has the comment:
> /* Map foreign gmfn, fgmfn, to local pfn, lpfn. This for the user space 
>  * creating new guest on PVH dom0 and needs to map domU pages. 
>  */
> Is that preferable? (modulo removing the PVH bit)

<nods>
> 
> > 
> > > +static int map_foreign_page(unsigned long lpfn, unsigned long fgmfn,
> > > +			    unsigned int domid)
> > > +{
> > > +	int rc;
> > > +	struct xen_add_to_physmap xatp = {
> > > +		.domid = DOMID_SELF,
> > > +		.u.foreign_domid = domid,
> > > +		.space = XENMAPSPACE_gmfn_foreign,
> > > +		.idx = fgmfn,
> > > +		.gpfn = lpfn,
> > > +	};
> > > +
> > > +	rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp);
> > > +	if (rc) {
> > > +		pr_warn("Failed to map pfn to mfn rc:%d pfn:%lx mfn:%lx\n",
> > 
> > How about 'Failed to map pfn (0x%lx) to mfn (0x%lx), rc: %d\n" ?
> 
> Sure, need to slip foreign domid in there somewhere now I look at it.
> 
> x86 PVH has basically the same print BTW.

OK, lets address that as well in the next review of the patchset.
> 
> > 
> > > +			rc, lpfn, fgmfn);
> > > +		return 1;
> > > +	}
> > > +	return 0;
> > > +}
> > > +
> > > +struct remap_data {
> > > +	unsigned long fgmfn; /* foreign domain's gmfn */
> > 
> > Shouldn't this be called now 'xen_pfn_t' or something.
> 
> xen_pfn_t is needed at the hypervisor interface layer, I'm not so sure
> about kernel internal use. I guess it can't hurt?

My thoughts.. as somebody might be wondering why here is unsigned long
but other places it is not.
> 
> > 
> > > +	pgprot_t prot;
> > > +	domid_t  domid;
> > > +	struct vm_area_struct *vma;
> > > +	struct xen_remap_mfn_info *info;
> > > +};
> > > +
> > > +static int remap_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr,
> > > +			void *data)
> > > +{
> > > +	struct remap_data *info = data;
> > > +	struct xen_remap_mfn_info *savp = info->info;
> > > +	struct page *page = savp->pi_paga[savp->pi_next_todo++];
> > > +	unsigned long pfn = page_to_pfn(page);
> > > +	pte_t pte = pfn_pte(pfn, info->prot);
> > > +
> > > +	if (map_foreign_page(pfn, info->fgmfn, info->domid))
> > > +		return -EFAULT;
> > > +	set_pte_at(info->vma->vm_mm, addr, ptep, pte);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
> > >  			       unsigned long addr,
> > >  			       unsigned long mfn, int nr,
> > > -			       pgprot_t prot, unsigned domid)
> > > +			       pgprot_t prot, unsigned domid,
> > > +			       struct xen_remap_mfn_info *info)
> > >  {
> > > -	return -ENOSYS;
> > > +	int err;
> > > +	struct remap_data data;
> > > +
> > > +	/* TBD: Batching, current sole caller only does page at a time */
> > > +	if (nr > 1)
> > 
> > Lets also wrap it with WARN_ON?
> 
> ACK, needs doing on x86 PVH too then.
> 
> > 
> > > +		return -EINVAL;
> > > +
> > > +	data.fgmfn = mfn;
> > > +	data.prot = prot;
> > > +	data.domid = domid;
> > > +	data.vma = vma;
> > > +	data.info = info;
> > > +	err = apply_to_page_range(vma->vm_mm, addr, nr << PAGE_SHIFT,
> > > +				  remap_pte_fn, &data);
> > > +	return err;
> > >  }
> > >  EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);
> > >  
> > > +/* Returns: Number of pages unmapped */
> > > +int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
> > > +			       struct xen_remap_mfn_info *info)
> > > +{
> > > +	int count = 0;
> > > +
> > > +	while (info->pi_next_todo--) {
> > > +		struct xen_remove_from_physmap xrp;
> > > +		unsigned long rc, pfn;
> > > +
> > > +		pfn = page_to_pfn(info->pi_paga[info->pi_next_todo]);
> > 
> > Won't this miss the first pi_next_todo? You did the 'pi_next_todo--' so
> > will the compiler decrement it here in this operation or will it do
> > when it gets to the 'do' logic of the loop?
> 
> It's a post decrement so if pi_next_todo == 1 then the expression in the
> while will see 1 (true) but inside the loop we see zero. So we end up
> processing elements N-1..0 of the array which is correct.

OK. That is what I wanted to make sure about.
> 
> This is the same as on x86 PVH, so if I'm wrong then that is too.
> 
> I mentioned in the PVH thread this morning that I think this interface
> should drop pi_next_todo and have a simple for loop based on the number
> of pages between vm_start...vm_end here.

Yeah, I think we need to do that. I understand Mukesh's desire to have
an easy searchable name for variables, but that 'pi' just makes me
think of bathroom, then of 3.1415, and then I have to actually recall
really hard why it is called 'pi' .. and I still can't remember why.
> 
> > 
> > > +
> > > +		xrp.domid = DOMID_SELF;
> > > +		xrp.gpfn = pfn;
> > > +		rc = HYPERVISOR_memory_op(XENMEM_remove_from_physmap, &xrp);
> > 
> > 'rc' is 'unsigned long'. Is that right? You don't want it to be 'int'?
> 
> Hypercalls return unsigned long these days, I think it was considered a
> mistake that some didn't. See <25744:47080c965937> in the hypervisor
> tree. 
> 
> Oh, wait, we are both wrong -- it's a long. I'll fix that...
> 
> > 
> > > +		if (rc) {
> > > +			pr_warn("Failed to unmap pfn:%lx rc:%ld\n",
> > > +				pfn, rc);
> > > +			return count;
> > > +		}
> > > +		count++;
> > > +	}
> > > +	return count;
> > > +}
> > > +EXPORT_SYMBOL_GPL(xen_unmap_domain_mfn_range);
> > > +
> > >  /*
> > >   * see Documentation/devicetree/bindings/arm/xen.txt for the
> > >   * documentation of the Xen Device Tree format.
> > > -- 
> > > 1.7.2.5
> 

Re: [PATCH 11/14] arm: implement remap interfaces needed for privcmd mappings.

[Stefano Stabellini]

On Thu, 4 Oct 2012, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  arch/arm/xen/enlighten.c |   94 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 92 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index ba5cc13..9956af5 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -9,6 +9,7 @@
>  #include <xen/platform_pci.h>
>  #include <xen/xenbus.h>
>  #include <xen/page.h>
> +#include <xen/xen-ops.h>
>  #include <asm/xen/hypervisor.h>
>  #include <asm/xen/hypercall.h>
>  #include <linux/interrupt.h>
> @@ -18,6 +19,8 @@
>  #include <linux/of_irq.h>
>  #include <linux/of_address.h>
>  
> +#include <linux/mm.h>
> +
>  struct start_info _xen_start_info;
>  struct start_info *xen_start_info = &_xen_start_info;
>  EXPORT_SYMBOL_GPL(xen_start_info);
> @@ -43,15 +46,102 @@ EXPORT_SYMBOL_GPL(xen_platform_pci_unplug);
>  
>  static __read_mostly int xen_events_irq = -1;
>  
> +/* map fgmfn of domid to lpfn in the current domain */
> +static int map_foreign_page(unsigned long lpfn, unsigned long fgmfn,
> +			    unsigned int domid)
> +{
> +	int rc;
> +	struct xen_add_to_physmap xatp = {
> +		.domid = DOMID_SELF,
> +		.u.foreign_domid = domid,
> +		.space = XENMAPSPACE_gmfn_foreign,
> +		.idx = fgmfn,
> +		.gpfn = lpfn,
> +	};
> +
> +	rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp);
> +	if (rc) {
> +		pr_warn("Failed to map pfn to mfn rc:%d pfn:%lx mfn:%lx\n",
> +			rc, lpfn, fgmfn);
> +		return 1;
> +	}
> +	return 0;
> +}
> +
> +struct remap_data {
> +	unsigned long fgmfn; /* foreign domain's gmfn */
> +	pgprot_t prot;
> +	domid_t  domid;
> +	struct vm_area_struct *vma;
> +	struct xen_remap_mfn_info *info;
> +};
> +
> +static int remap_pte_fn(pte_t *ptep, pgtable_t token, unsigned long addr,
> +			void *data)
> +{
> +	struct remap_data *info = data;
> +	struct xen_remap_mfn_info *savp = info->info;
> +	struct page *page = savp->pi_paga[savp->pi_next_todo++];
> +	unsigned long pfn = page_to_pfn(page);
> +	pte_t pte = pfn_pte(pfn, info->prot);
> +
> +	if (map_foreign_page(pfn, info->fgmfn, info->domid))
> +		return -EFAULT;
> +	set_pte_at(info->vma->vm_mm, addr, ptep, pte);
> +
> +	return 0;
> +}
> +
>  int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
>  			       unsigned long addr,
>  			       unsigned long mfn, int nr,
> -			       pgprot_t prot, unsigned domid)
> +			       pgprot_t prot, unsigned domid,
> +			       struct xen_remap_mfn_info *info)
>  {
> -	return -ENOSYS;
> +	int err;
> +	struct remap_data data;
> +
> +	/* TBD: Batching, current sole caller only does page at a time */
> +	if (nr > 1)
> +		return -EINVAL;

It looks like that this implementation of xen_remap_domain_mfn_range is
capable of handling nr > 1, so why this return -EINVAL?


> +	data.fgmfn = mfn;
> +	data.prot = prot;
> +	data.domid = domid;
> +	data.vma = vma;
> +	data.info = info;
> +	err = apply_to_page_range(vma->vm_mm, addr, nr << PAGE_SHIFT,
> +				  remap_pte_fn, &data);
> +	return err;
>  }
>  EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);
>  
> +/* Returns: Number of pages unmapped */
> +int xen_unmap_domain_mfn_range(struct vm_area_struct *vma,
> +			       struct xen_remap_mfn_info *info)
> +{
> +	int count = 0;
> +
> +	while (info->pi_next_todo--) {
> +		struct xen_remove_from_physmap xrp;
> +		unsigned long rc, pfn;
> +
> +		pfn = page_to_pfn(info->pi_paga[info->pi_next_todo]);
> +
> +		xrp.domid = DOMID_SELF;
> +		xrp.gpfn = pfn;
> +		rc = HYPERVISOR_memory_op(XENMEM_remove_from_physmap, &xrp);
> +		if (rc) {
> +			pr_warn("Failed to unmap pfn:%lx rc:%ld\n",
> +				pfn, rc);
> +			return count;
> +		}
> +		count++;
> +	}
> +	return count;
> +}
> +EXPORT_SYMBOL_GPL(xen_unmap_domain_mfn_range);
> +
>  /*
>   * see Documentation/devicetree/bindings/arm/xen.txt for the
>   * documentation of the Xen Device Tree format.
> -- 
> 1.7.2.5
> 

Re: [PATCH 11/14] arm: implement remap interfaces needed for privcmd mappings.

[Ian Campbell]

On Fri, 2012-10-05 at 14:36 +0100, Stefano Stabellini wrote:
[... snip dozens of pointless lines -- please trim your quotes...]

> > -	return -ENOSYS;
> > +	int err;
> > +	struct remap_data data;
> > +
> > +	/* TBD: Batching, current sole caller only does page at a time */
> > +	if (nr > 1)
> > +		return -EINVAL;
> 
> It looks like that this implementation of xen_remap_domain_mfn_range is
> capable of handling nr > 1, so why this return -EINVAL?

Hrm, yes. I think I just blindly copied that from the pvh
implementation.

[...]

Re: [PATCH 11/14] arm: implement remap interfaces needed for privcmd mappings.

[Mukesh Rathor]

On Fri, 5 Oct 2012 14:42:30 +0100
Ian Campbell <Ian.Campbell@citrix.com> wrote:

> On Fri, 2012-10-05 at 14:36 +0100, Stefano Stabellini wrote:
> [... snip dozens of pointless lines -- please trim your quotes...]
> 
> > > -	return -ENOSYS;
> > > +	int err;
> > > +	struct remap_data data;
> > > +
> > > +	/* TBD: Batching, current sole caller only does page at
> > > a time */
> > > +	if (nr > 1)
> > > +		return -EINVAL;
> > 
> > It looks like that this implementation of
> > xen_remap_domain_mfn_range is capable of handling nr > 1, so why
> > this return -EINVAL?
> 
> Hrm, yes. I think I just blindly copied that from the pvh
> implementation.
> 
> [...]

PVH was using a different mechanism earlier, pfn from high up memory
address space. So it was doing one at a time. It can be removed now.

[PATCH 12/14] arm: xen: explain the EXPERIMENTAL dependency in the Kconfig help

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 arch/arm/Kconfig |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 3361466..b171c46 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1907,6 +1907,14 @@ config XEN
 	help
 	  Say Y if you want to run Linux in a Virtual Machine on Xen on ARM.
 
+
+	  This option is EXPERIMETNAL because the hypervisor
+	  interfaces which it uses are not yet considered stable
+	  therefore backwards and forwards compatibility is not yet
+	  guaranteed.
+
+	  If unsure, say N.
+
 endmenu
 
 menu "Boot options"
-- 
1.7.2.5

Re: [PATCH 12/14] arm: xen: explain the EXPERIMENTAL dependency in the Kconfig help

[Stefano Stabellini]

On Thu, 4 Oct 2012, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  arch/arm/Kconfig |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 3361466..b171c46 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1907,6 +1907,14 @@ config XEN
>  	help
>  	  Say Y if you want to run Linux in a Virtual Machine on Xen on ARM.
>  
> +
> +	  This option is EXPERIMETNAL because the hypervisor
                         ^ EXPERIMENTAL
> +	  interfaces which it uses are not yet considered stable
> +	  therefore backwards and forwards compatibility is not yet
> +	  guaranteed.
> +
> +	  If unsure, say N.
> +
>  endmenu
>  
>  menu "Boot options"
> -- 
> 1.7.2.5
> 

Re: [PATCH 12/14] arm: xen: explain the EXPERIMENTAL dependency in the Kconfig help

[Ian Campbell]

On Fri, 2012-10-05 at 14:21 +0100, Stefano Stabellini wrote:
> On Thu, 4 Oct 2012, Ian Campbell wrote:
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  arch/arm/Kconfig |    8 ++++++++
> >  1 files changed, 8 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > index 3361466..b171c46 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -1907,6 +1907,14 @@ config XEN
> >  	help
> >  	  Say Y if you want to run Linux in a Virtual Machine on Xen on ARM.
> >  
> > +
> > +	  This option is EXPERIMETNAL because the hypervisor
>                          ^ EXPERIMENTAL

Fixed, thanks.

> > +	  interfaces which it uses are not yet considered stable
> > +	  therefore backwards and forwards compatibility is not yet
> > +	  guaranteed.
> > +
> > +	  If unsure, say N.
> > +
> >  endmenu
> >  
> >  menu "Boot options"
> > -- 
> > 1.7.2.5
> > 

[PATCH 13/14] xen: use xen_pft_t in struct xen_remove_from_physmap

[Ian Campbell]

4a6c2b4 "PVH basic and hader file changes" and bd3f79b "xen: Introduce
xen_pfn_t for pfn and mfn types" passed like ships in the night.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 include/xen/interface/memory.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/xen/interface/memory.h b/include/xen/interface/memory.h
index 6d74c47..d38bdc1 100644
--- a/include/xen/interface/memory.h
+++ b/include/xen/interface/memory.h
@@ -258,7 +258,7 @@ struct xen_remove_from_physmap {
     domid_t domid;
 
     /* GPFN of the current mapping of the page. */
-    unsigned long     gpfn;
+    xen_pfn_t gpfn;
 };
 DEFINE_GUEST_HANDLE_STRUCT(xen_remove_from_physmap);
 
-- 
1.7.2.5

Re: [PATCH 13/14] xen: use xen_pft_t in struct xen_remove_from_physmap

[Stefano Stabellini]

On Thu, 4 Oct 2012, Ian Campbell wrote:
> 4a6c2b4 "PVH basic and hader file changes" and bd3f79b "xen: Introduce
> xen_pfn_t for pfn and mfn types" passed like ships in the night.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>


Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

>  include/xen/interface/memory.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/include/xen/interface/memory.h b/include/xen/interface/memory.h
> index 6d74c47..d38bdc1 100644
> --- a/include/xen/interface/memory.h
> +++ b/include/xen/interface/memory.h
> @@ -258,7 +258,7 @@ struct xen_remove_from_physmap {
>      domid_t domid;
>  
>      /* GPFN of the current mapping of the page. */
> -    unsigned long     gpfn;
> +    xen_pfn_t gpfn;
>  };
>  DEFINE_GUEST_HANDLE_STRUCT(xen_remove_from_physmap);
>  
> -- 
> 1.7.2.5
> 

[PATCH 14/14] arm: implement foreign mapping using XENMEM_add_to_physmap_range

[Ian Campbell]

This interface is prefered for foreign mappings.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 arch/arm/xen/enlighten.c       |   14 +++++++++-----
 include/xen/interface/memory.h |   18 ++++++++++++++++++
 2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 9956af5..a9946aa 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -51,15 +51,19 @@ static int map_foreign_page(unsigned long lpfn, unsigned long fgmfn,
 			    unsigned int domid)
 {
 	int rc;
-	struct xen_add_to_physmap xatp = {
+	struct xen_add_to_physmap_range xatp = {
 		.domid = DOMID_SELF,
-		.u.foreign_domid = domid,
+		.foreign_domid = domid,
+		.size = 1,
 		.space = XENMAPSPACE_gmfn_foreign,
-		.idx = fgmfn,
-		.gpfn = lpfn,
 	};
+	unsigned long idx = fgmfn;
+	xen_pfn_t gpfn = lpfn;
 
-	rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp);
+	set_xen_guest_handle(xatp.idxs, &idx);
+	set_xen_guest_handle(xatp.gpfns, &gpfn);
+
+	rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap_range, &xatp);
 	if (rc) {
 		pr_warn("Failed to map pfn to mfn rc:%d pfn:%lx mfn:%lx\n",
 			rc, lpfn, fgmfn);
diff --git a/include/xen/interface/memory.h b/include/xen/interface/memory.h
index d38bdc1..e5675bc 100644
--- a/include/xen/interface/memory.h
+++ b/include/xen/interface/memory.h
@@ -188,6 +188,24 @@ struct xen_add_to_physmap {
 };
 DEFINE_GUEST_HANDLE_STRUCT(xen_add_to_physmap);
 
+#define XENMEM_add_to_physmap_range 23
+struct xen_add_to_physmap_range {
+    /* Which domain to change the mapping for. */
+    domid_t domid;
+    uint16_t space; /* => enum phys_map_space */
+
+    /* Number of pages to go through */
+    uint16_t size;
+    domid_t foreign_domid; /* IFF gmfn_foreign */
+
+    /* Indexes into space being mapped. */
+    GUEST_HANDLE(ulong) idxs;
+
+    /* GPFN in domid where the source mapping page should appear. */
+    GUEST_HANDLE(xen_pfn_t) gpfns;
+};
+DEFINE_GUEST_HANDLE_STRUCT(xen_add_to_physmap_range);
+
 /*
  * Translates a list of domain-specific GPFNs into MFNs. Returns a -ve error
  * code on failure. This call only works for auto-translated guests.
-- 
1.7.2.5

Re: [PATCH 14/14] arm: implement foreign mapping using XENMEM_add_to_physmap_range

[Stefano Stabellini]

On Thu, 4 Oct 2012, Ian Campbell wrote:
> This interface is prefered for foreign mappings.

So now we have XENMEM_add_to_physmap_range but we only have
XENMEM_remove_from_physmap. Would it be worth to introduce a
XENMEM_remove_from_physmap_range too?


> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  arch/arm/xen/enlighten.c       |   14 +++++++++-----
>  include/xen/interface/memory.h |   18 ++++++++++++++++++
>  2 files changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index 9956af5..a9946aa 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -51,15 +51,19 @@ static int map_foreign_page(unsigned long lpfn, unsigned long fgmfn,
>  			    unsigned int domid)
>  {
>  	int rc;
> -	struct xen_add_to_physmap xatp = {
> +	struct xen_add_to_physmap_range xatp = {
>  		.domid = DOMID_SELF,
> -		.u.foreign_domid = domid,
> +		.foreign_domid = domid,
> +		.size = 1,
>  		.space = XENMAPSPACE_gmfn_foreign,
> -		.idx = fgmfn,
> -		.gpfn = lpfn,
>  	};
> +	unsigned long idx = fgmfn;
> +	xen_pfn_t gpfn = lpfn;
>  
> -	rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp);
> +	set_xen_guest_handle(xatp.idxs, &idx);
> +	set_xen_guest_handle(xatp.gpfns, &gpfn);
> +
> +	rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap_range, &xatp);
>  	if (rc) {
>  		pr_warn("Failed to map pfn to mfn rc:%d pfn:%lx mfn:%lx\n",
>  			rc, lpfn, fgmfn);

Wouldn't it make sense to call XENMEM_add_to_physmap_range only once for
the whole range, rather than once per page?


> diff --git a/include/xen/interface/memory.h b/include/xen/interface/memory.h
> index d38bdc1..e5675bc 100644
> --- a/include/xen/interface/memory.h
> +++ b/include/xen/interface/memory.h
> @@ -188,6 +188,24 @@ struct xen_add_to_physmap {
>  };
>  DEFINE_GUEST_HANDLE_STRUCT(xen_add_to_physmap);
>  
> +#define XENMEM_add_to_physmap_range 23
> +struct xen_add_to_physmap_range {
> +    /* Which domain to change the mapping for. */
> +    domid_t domid;
> +    uint16_t space; /* => enum phys_map_space */
> +
> +    /* Number of pages to go through */
> +    uint16_t size;
> +    domid_t foreign_domid; /* IFF gmfn_foreign */
> +
> +    /* Indexes into space being mapped. */
> +    GUEST_HANDLE(ulong) idxs;
> +
> +    /* GPFN in domid where the source mapping page should appear. */
> +    GUEST_HANDLE(xen_pfn_t) gpfns;
> +};
> +DEFINE_GUEST_HANDLE_STRUCT(xen_add_to_physmap_range);
> +
>  /*
>   * Translates a list of domain-specific GPFNs into MFNs. Returns a -ve error
>   * code on failure. This call only works for auto-translated guests.
> -- 
> 1.7.2.5
> 

Re: [PATCH 14/14] arm: implement foreign mapping using XENMEM_add_to_physmap_range

[Ian Campbell]

On Fri, 2012-10-05 at 14:36 +0100, Stefano Stabellini wrote:
> On Thu, 4 Oct 2012, Ian Campbell wrote:
> > This interface is prefered for foreign mappings.
> 
> So now we have XENMEM_add_to_physmap_range but we only have
> XENMEM_remove_from_physmap. Would it be worth to introduce a
> XENMEM_remove_from_physmap_range too?

Worth considering but the need isn't so urgent since you wouldn't need
to coexist with XENMAPSPACE_gmfn_range's size parameter and the
foreign_domid one isn't needed either.

I wonder if XENMEM_add_to_physmap_range ought to reject
XENMAPSPACE_gmfn_range, just for the sake of sanity.

> 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  arch/arm/xen/enlighten.c       |   14 +++++++++-----
> >  include/xen/interface/memory.h |   18 ++++++++++++++++++
> >  2 files changed, 27 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> > index 9956af5..a9946aa 100644
> > --- a/arch/arm/xen/enlighten.c
> > +++ b/arch/arm/xen/enlighten.c
> > @@ -51,15 +51,19 @@ static int map_foreign_page(unsigned long lpfn, unsigned long fgmfn,
> >  			    unsigned int domid)
> >  {
> >  	int rc;
> > -	struct xen_add_to_physmap xatp = {
> > +	struct xen_add_to_physmap_range xatp = {
> >  		.domid = DOMID_SELF,
> > -		.u.foreign_domid = domid,
> > +		.foreign_domid = domid,
> > +		.size = 1,
> >  		.space = XENMAPSPACE_gmfn_foreign,
> > -		.idx = fgmfn,
> > -		.gpfn = lpfn,
> >  	};
> > +	unsigned long idx = fgmfn;
> > +	xen_pfn_t gpfn = lpfn;
> >  
> > -	rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp);
> > +	set_xen_guest_handle(xatp.idxs, &idx);
> > +	set_xen_guest_handle(xatp.gpfns, &gpfn);
> > +
> > +	rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap_range, &xatp);
> >  	if (rc) {
> >  		pr_warn("Failed to map pfn to mfn rc:%d pfn:%lx mfn:%lx\n",
> >  			rc, lpfn, fgmfn);
> 
> Wouldn't it make sense to call XENMEM_add_to_physmap_range only once for
> the whole range, rather than once per page?
> 
> 
> > diff --git a/include/xen/interface/memory.h b/include/xen/interface/memory.h
> > index d38bdc1..e5675bc 100644
> > --- a/include/xen/interface/memory.h
> > +++ b/include/xen/interface/memory.h
> > @@ -188,6 +188,24 @@ struct xen_add_to_physmap {
> >  };
> >  DEFINE_GUEST_HANDLE_STRUCT(xen_add_to_physmap);
> >  
> > +#define XENMEM_add_to_physmap_range 23
> > +struct xen_add_to_physmap_range {
> > +    /* Which domain to change the mapping for. */
> > +    domid_t domid;
> > +    uint16_t space; /* => enum phys_map_space */
> > +
> > +    /* Number of pages to go through */
> > +    uint16_t size;
> > +    domid_t foreign_domid; /* IFF gmfn_foreign */
> > +
> > +    /* Indexes into space being mapped. */
> > +    GUEST_HANDLE(ulong) idxs;
> > +
> > +    /* GPFN in domid where the source mapping page should appear. */
> > +    GUEST_HANDLE(xen_pfn_t) gpfns;
> > +};
> > +DEFINE_GUEST_HANDLE_STRUCT(xen_add_to_physmap_range);
> > +
> >  /*
> >   * Translates a list of domain-specific GPFNs into MFNs. Returns a -ve error
> >   * code on failure. This call only works for auto-translated guests.
> > -- 
> > 1.7.2.5
> > 

Re: [RFC 00/14] arm: implement ballooning and privcmd foreign mappings based on x86 PVH

[Konrad Rzeszutek Wilk]

On Thu, Oct 04, 2012 at 04:11:36PM +0100, Ian Campbell wrote:
> This series implements ballooning for Xen on ARM and builds and Mukesh's
> PVH privcmd stuff to implement foreign page mapping on ARM, replacing
> the old "HACK: initial (very hacky) XENMAPSPACE_gmfn_foreign" patch.

Great. Thanks for doing it.
> 
> The baseline is a bit complex, it is basically Stefano's xenarm-forlinus
> branch (commit bbd6eb29214e) merged with Konrad's linux-next-pvh branch
> (commit 7e6e6f589de8). I suspect I might need to rebase it shortly. I

Yes. I am hoping that on Monday Mukesh will have had sent out his revised
patch and I can rebase the whole thing on Linus's tree (which by that time
should have the Xen ARM's pulled in).


> know there's a bunch of stuff in here which Mukesh has also changed, but
> which I haven't seen yet, I'll deal with that when I rebase (RFC mainly
> for that reason only).

They look good to me. I've provided some feedback on some of them.

> 
> This lets me start a guest without any of those nasty patches with
> "HACK" in the title ;-0
> 
> Ian.
> 
> 
> 
>