[PATCH 00/10] xen: build fixes and interface tweaks

[PATCH 00/10] xen: build fixes and interface tweaks

[Ian Campbell]

The following is based on v3.7-rc1 and fixes various build fallout which
I discovered when build for Xen on ARM.

It also cleans up a final few s/unsigned long/xen_{pfn,ulong}_t/
changes. These have no impact on x86 (since those types are typedefs of
unsigned long) but change the ARM port to use the interface we have
settled on and which I shall be committing to the h/v shortly.

I've sent a few of these out individually as I've gone along too, but
they are mostly independent so I figure there's no harm in resending.

Ian.

[PATCH 01/10] xen: xenbus: quirk uses x86 specific cpuid

[Ian Campbell]

This breaks on ARM. This quirk is not necessary on ARM because no
hypervisors of that vintage exist for that architecture (port is too
new).

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

diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c
index 48220e1..b46ad11 100644
--- a/drivers/xen/xenbus/xenbus_xs.c
+++ b/drivers/xen/xenbus/xenbus_xs.c
@@ -619,6 +619,8 @@ static struct xenbus_watch *find_watch(const char *token)
 
 	return NULL;
 }
+
+#ifdef CONFIG_X86
 /*
  * Certain older XenBus toolstack cannot handle reading values that are
  * not populated. Some Xen 3.4 installation are incapable of doing this
@@ -637,6 +639,10 @@ static bool xen_strict_xenbus_quirk()
 	return false;
 
 }
+#else
+static bool xen_strict_xenbus_quirk(void) { return false; }
+#endif
+
 static void xs_reset_watches(void)
 {
 	int err, supported = 0;
-- 
1.7.2.5

Re: [PATCH 01/10] xen: xenbus: quirk uses x86 specific cpuid

[Ian Campbell]

I botched the LKML address in my git send-email cmdline so these didn't
go there. This is Xen specific enough that I don't think it is worth
resending/spamming you guys with a repeat to fix that.

On Wed, 2012-10-17 at 09:39 +0100, Ian Campbell wrote:
> This breaks on ARM. This quirk is not necessary on ARM because no
> hypervisors of that vintage exist for that architecture (port is too
> new).
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  drivers/xen/xenbus/xenbus_xs.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c
> index 48220e1..b46ad11 100644
> --- a/drivers/xen/xenbus/xenbus_xs.c
> +++ b/drivers/xen/xenbus/xenbus_xs.c
> @@ -619,6 +619,8 @@ static struct xenbus_watch *find_watch(const char *token)
>  
>  	return NULL;
>  }
> +
> +#ifdef CONFIG_X86
>  /*
>   * Certain older XenBus toolstack cannot handle reading values that are
>   * not populated. Some Xen 3.4 installation are incapable of doing this
> @@ -637,6 +639,10 @@ static bool xen_strict_xenbus_quirk()
>  	return false;
>  
>  }
> +#else
> +static bool xen_strict_xenbus_quirk(void) { return false; }
> +#endif
> +
>  static void xs_reset_watches(void)
>  {
>  	int err, supported = 0;

Re: [PATCH 01/10] xen: xenbus: quirk uses x86 specific cpuid

[Jan Beulich]

>>> On 17.10.12 at 10:39, Ian Campbell <ian.campbell@citrix.com> wrote:
> --- a/drivers/xen/xenbus/xenbus_xs.c
> +++ b/drivers/xen/xenbus/xenbus_xs.c
> @@ -619,6 +619,8 @@ static struct xenbus_watch *find_watch(const char *token)
>  
>  	return NULL;
>  }
> +
> +#ifdef CONFIG_X86
>  /*
>   * Certain older XenBus toolstack cannot handle reading values that are
>   * not populated. Some Xen 3.4 installation are incapable of doing this
> @@ -637,6 +639,10 @@ static bool xen_strict_xenbus_quirk()
>  	return false;
>  
>  }
> +#else
> +static bool xen_strict_xenbus_quirk(void) { return false; }
> +#endif

Wouldn't it reduce redundancy if the #ifdef block was inserted
inside the existing function?

Jan

> +
>  static void xs_reset_watches(void)
>  {
>  	int err, supported = 0;

Re: [PATCH 01/10] xen: xenbus: quirk uses x86 specific cpuid

[Konrad Rzeszutek Wilk]

On Wed, Oct 17, 2012 at 10:09:26AM +0100, Jan Beulich wrote:
> >>> On 17.10.12 at 10:39, Ian Campbell <ian.campbell@citrix.com> wrote:
> > --- a/drivers/xen/xenbus/xenbus_xs.c
> > +++ b/drivers/xen/xenbus/xenbus_xs.c
> > @@ -619,6 +619,8 @@ static struct xenbus_watch *find_watch(const char *token)
> >  
> >  	return NULL;
> >  }
> > +
> > +#ifdef CONFIG_X86
> >  /*
> >   * Certain older XenBus toolstack cannot handle reading values that are
> >   * not populated. Some Xen 3.4 installation are incapable of doing this
> > @@ -637,6 +639,10 @@ static bool xen_strict_xenbus_quirk()
> >  	return false;
> >  
> >  }
> > +#else
> > +static bool xen_strict_xenbus_quirk(void) { return false; }
> > +#endif
> 
> Wouldn't it reduce redundancy if the #ifdef block was inserted
> inside the existing function?

Applied with Jan's suggestions.
> 
> Jan
> 
> > +
> >  static void xs_reset_watches(void)
> >  {
> >  	int err, supported = 0;
> 

[PATCH 02/10] xen: sysfs: include err.h for PTR_ERR etc

[Ian Campbell]

Fixes build error on ARM:
drivers/xen/sys-hypervisor.c: In function 'uuid_show_fallback':
drivers/xen/sys-hypervisor.c:127:2: error: implicit declaration of function 'IS_ERR' [-Werror=implicit-function-declaration]
drivers/xen/sys-hypervisor.c:128:3: error: implicit declaration of function 'PTR_ERR' [-Werror=implicit-function-declaration]

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

diff --git a/drivers/xen/sys-hypervisor.c b/drivers/xen/sys-hypervisor.c
index 5e5ad7e..66a0a14 100644
--- a/drivers/xen/sys-hypervisor.c
+++ b/drivers/xen/sys-hypervisor.c
@@ -11,6 +11,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/kobject.h>
+#include <linux/err.h>
 
 #include <asm/xen/hypervisor.h>
 #include <asm/xen/hypercall.h>
-- 
1.7.2.5

Re: [PATCH 02/10] xen: sysfs: include err.h for PTR_ERR etc

[Stefano Stabellini]

On Wed, 17 Oct 2012, Ian Campbell wrote:
> Fixes build error on ARM:
> drivers/xen/sys-hypervisor.c: In function 'uuid_show_fallback':
> drivers/xen/sys-hypervisor.c:127:2: error: implicit declaration of function 'IS_ERR' [-Werror=implicit-function-declaration]
> drivers/xen/sys-hypervisor.c:128:3: error: implicit declaration of function 'PTR_ERR' [-Werror=implicit-function-declaration]
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

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


>  drivers/xen/sys-hypervisor.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/xen/sys-hypervisor.c b/drivers/xen/sys-hypervisor.c
> index 5e5ad7e..66a0a14 100644
> --- a/drivers/xen/sys-hypervisor.c
> +++ b/drivers/xen/sys-hypervisor.c
> @@ -11,6 +11,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/kobject.h>
> +#include <linux/err.h>
>  
>  #include <asm/xen/hypervisor.h>
>  #include <asm/xen/hypercall.h>
> -- 
> 1.7.2.5
> 

[PATCH 03/10] xen: sysfs: fix build warning.

[Ian Campbell]

Define PRI macros for xen_ulong_t and xen_pfn_t and use to fix:
drivers/xen/sys-hypervisor.c:288:4: warning: format '%lx' expects argument of type 'long unsigned int', but argument 3 has type 'xen_ulong_t' [-Wformat]

Ideally this would use PRIx64 on ARM but these (or equivalent) don't
seem to be available in the kernel.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 arch/arm/include/asm/xen/interface.h |    2 ++
 arch/x86/include/asm/xen/interface.h |    2 ++
 drivers/xen/sys-hypervisor.c         |    3 ++-
 3 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/arch/arm/include/asm/xen/interface.h b/arch/arm/include/asm/xen/interface.h
index ae05e56..62160f2 100644
--- a/arch/arm/include/asm/xen/interface.h
+++ b/arch/arm/include/asm/xen/interface.h
@@ -31,7 +31,9 @@
 /* Explicitly size integers that represent pfns in the interface with
  * Xen so that we can have one ABI that works for 32 and 64 bit guests. */
 typedef uint64_t xen_pfn_t;
+#define PRI_xen_pfn "llx"
 typedef uint64_t xen_ulong_t;
+#define PRI_xen_ulong "llx"
 /* Guest handles for primitive C types. */
 __DEFINE_GUEST_HANDLE(uchar, unsigned char);
 __DEFINE_GUEST_HANDLE(uint,  unsigned int);
diff --git a/arch/x86/include/asm/xen/interface.h b/arch/x86/include/asm/xen/interface.h
index 6d2f75a..ab3c67c 100644
--- a/arch/x86/include/asm/xen/interface.h
+++ b/arch/x86/include/asm/xen/interface.h
@@ -51,7 +51,9 @@
  * with Xen so that on ARM we can have one ABI that works for 32 and 64
  * bit guests. */
 typedef unsigned long xen_pfn_t;
+#define PRI_xen_pfn "lx"
 typedef unsigned long xen_ulong_t;
+#define PRI_xen_ulong "lx"
 /* Guest handles for primitive C types. */
 __DEFINE_GUEST_HANDLE(uchar, unsigned char);
 __DEFINE_GUEST_HANDLE(uint,  unsigned int);
diff --git a/drivers/xen/sys-hypervisor.c b/drivers/xen/sys-hypervisor.c
index 66a0a14..96453f8 100644
--- a/drivers/xen/sys-hypervisor.c
+++ b/drivers/xen/sys-hypervisor.c
@@ -285,7 +285,8 @@ static ssize_t virtual_start_show(struct hyp_sysfs_attr *attr, char *buffer)
 		ret = HYPERVISOR_xen_version(XENVER_platform_parameters,
 					     parms);
 		if (!ret)
-			ret = sprintf(buffer, "%lx\n", parms->virt_start);
+			ret = sprintf(buffer, "%"PRI_xen_ulong"\n",
+				      parms->virt_start);
 		kfree(parms);
 	}
 
-- 
1.7.2.5

Re: [PATCH 03/10] xen: sysfs: fix build warning.

[Stefano Stabellini]

On Wed, 17 Oct 2012, Ian Campbell wrote:
> Define PRI macros for xen_ulong_t and xen_pfn_t and use to fix:
> drivers/xen/sys-hypervisor.c:288:4: warning: format '%lx' expects argument of type 'long unsigned int', but argument 3 has type 'xen_ulong_t' [-Wformat]
> 
> Ideally this would use PRIx64 on ARM but these (or equivalent) don't
> seem to be available in the kernel.

I don't think that the PRI macros are used at all in the kernel, so you
can actually name and define these ones the way you like.

These definitions seem good to me.


> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  arch/arm/include/asm/xen/interface.h |    2 ++
>  arch/x86/include/asm/xen/interface.h |    2 ++
>  drivers/xen/sys-hypervisor.c         |    3 ++-
>  3 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/include/asm/xen/interface.h b/arch/arm/include/asm/xen/interface.h
> index ae05e56..62160f2 100644
> --- a/arch/arm/include/asm/xen/interface.h
> +++ b/arch/arm/include/asm/xen/interface.h
> @@ -31,7 +31,9 @@
>  /* Explicitly size integers that represent pfns in the interface with
>   * Xen so that we can have one ABI that works for 32 and 64 bit guests. */
>  typedef uint64_t xen_pfn_t;
> +#define PRI_xen_pfn "llx"
>  typedef uint64_t xen_ulong_t;
> +#define PRI_xen_ulong "llx"
>  /* Guest handles for primitive C types. */
>  __DEFINE_GUEST_HANDLE(uchar, unsigned char);
>  __DEFINE_GUEST_HANDLE(uint,  unsigned int);
> diff --git a/arch/x86/include/asm/xen/interface.h b/arch/x86/include/asm/xen/interface.h
> index 6d2f75a..ab3c67c 100644
> --- a/arch/x86/include/asm/xen/interface.h
> +++ b/arch/x86/include/asm/xen/interface.h
> @@ -51,7 +51,9 @@
>   * with Xen so that on ARM we can have one ABI that works for 32 and 64
>   * bit guests. */
>  typedef unsigned long xen_pfn_t;
> +#define PRI_xen_pfn "lx"
>  typedef unsigned long xen_ulong_t;
> +#define PRI_xen_ulong "lx"
>  /* Guest handles for primitive C types. */
>  __DEFINE_GUEST_HANDLE(uchar, unsigned char);
>  __DEFINE_GUEST_HANDLE(uint,  unsigned int);
> diff --git a/drivers/xen/sys-hypervisor.c b/drivers/xen/sys-hypervisor.c
> index 66a0a14..96453f8 100644
> --- a/drivers/xen/sys-hypervisor.c
> +++ b/drivers/xen/sys-hypervisor.c
> @@ -285,7 +285,8 @@ static ssize_t virtual_start_show(struct hyp_sysfs_attr *attr, char *buffer)
>  		ret = HYPERVISOR_xen_version(XENVER_platform_parameters,
>  					     parms);
>  		if (!ret)
> -			ret = sprintf(buffer, "%lx\n", parms->virt_start);
> +			ret = sprintf(buffer, "%"PRI_xen_ulong"\n",
> +				      parms->virt_start);
>  		kfree(parms);
>  	}
>  
> -- 
> 1.7.2.5
> 

Re: [PATCH 03/10] xen: sysfs: fix build warning.

[Ian Campbell]

On Wed, 2012-10-17 at 18:02 +0100, Stefano Stabellini wrote:
> On Wed, 17 Oct 2012, Ian Campbell wrote:
> > Define PRI macros for xen_ulong_t and xen_pfn_t and use to fix:
> > drivers/xen/sys-hypervisor.c:288:4: warning: format '%lx' expects argument of type 'long unsigned int', but argument 3 has type 'xen_ulong_t' [-Wformat]
> > 
> > Ideally this would use PRIx64 on ARM but these (or equivalent) don't
> > seem to be available in the kernel.
> 
> I don't think that the PRI macros are used at all in the kernel, so you
> can actually name and define these ones the way you like.

Sure, it's just a bit annoying to have the disconnect between "uint64_t"
and to pair it with "llx" instead of PRIx64. It might almost be better
to have typedef unsigned long long xen_pfn_t et al.

Anyway, lets stick with this for now.

> These definitions seem good to me.
> 
> 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  arch/arm/include/asm/xen/interface.h |    2 ++
> >  arch/x86/include/asm/xen/interface.h |    2 ++
> >  drivers/xen/sys-hypervisor.c         |    3 ++-
> >  3 files changed, 6 insertions(+), 1 deletions(-)
> > 
> > diff --git a/arch/arm/include/asm/xen/interface.h b/arch/arm/include/asm/xen/interface.h
> > index ae05e56..62160f2 100644
> > --- a/arch/arm/include/asm/xen/interface.h
> > +++ b/arch/arm/include/asm/xen/interface.h
> > @@ -31,7 +31,9 @@
> >  /* Explicitly size integers that represent pfns in the interface with
> >   * Xen so that we can have one ABI that works for 32 and 64 bit guests. */
> >  typedef uint64_t xen_pfn_t;
> > +#define PRI_xen_pfn "llx"
> >  typedef uint64_t xen_ulong_t;
> > +#define PRI_xen_ulong "llx"
> >  /* Guest handles for primitive C types. */
> >  __DEFINE_GUEST_HANDLE(uchar, unsigned char);
> >  __DEFINE_GUEST_HANDLE(uint,  unsigned int);
> > diff --git a/arch/x86/include/asm/xen/interface.h b/arch/x86/include/asm/xen/interface.h
> > index 6d2f75a..ab3c67c 100644
> > --- a/arch/x86/include/asm/xen/interface.h
> > +++ b/arch/x86/include/asm/xen/interface.h
> > @@ -51,7 +51,9 @@
> >   * with Xen so that on ARM we can have one ABI that works for 32 and 64
> >   * bit guests. */
> >  typedef unsigned long xen_pfn_t;
> > +#define PRI_xen_pfn "lx"
> >  typedef unsigned long xen_ulong_t;
> > +#define PRI_xen_ulong "lx"
> >  /* Guest handles for primitive C types. */
> >  __DEFINE_GUEST_HANDLE(uchar, unsigned char);
> >  __DEFINE_GUEST_HANDLE(uint,  unsigned int);
> > diff --git a/drivers/xen/sys-hypervisor.c b/drivers/xen/sys-hypervisor.c
> > index 66a0a14..96453f8 100644
> > --- a/drivers/xen/sys-hypervisor.c
> > +++ b/drivers/xen/sys-hypervisor.c
> > @@ -285,7 +285,8 @@ static ssize_t virtual_start_show(struct hyp_sysfs_attr *attr, char *buffer)
> >  		ret = HYPERVISOR_xen_version(XENVER_platform_parameters,
> >  					     parms);
> >  		if (!ret)
> > -			ret = sprintf(buffer, "%lx\n", parms->virt_start);
> > +			ret = sprintf(buffer, "%"PRI_xen_ulong"\n",
> > +				      parms->virt_start);
> >  		kfree(parms);
> >  	}
> >  
> > -- 
> > 1.7.2.5
> > 

[PATCH 04/10] xen: dbgp: Fix warning when CONFIG_PCI is not enabled.

[Ian Campbell]

I saw this on ARM:
linux/drivers/xen/dbgp.c:11:23: warning: unused variable 'ctrlr' [-Wunused-variable]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
---
 drivers/xen/dbgp.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/xen/dbgp.c b/drivers/xen/dbgp.c
index 42569c7..f3ccc80 100644
--- a/drivers/xen/dbgp.c
+++ b/drivers/xen/dbgp.c
@@ -8,7 +8,9 @@
 
 static int xen_dbgp_op(struct usb_hcd *hcd, int op)
 {
+#ifdef CONFIG_PCI
 	const struct device *ctrlr = hcd_to_bus(hcd)->controller;
+#endif
 	struct physdev_dbgp_op dbgp;
 
 	if (!xen_initial_domain())
-- 
1.7.2.5

Re: [PATCH 04/10] xen: dbgp: Fix warning when CONFIG_PCI is not enabled.

[Jan Beulich]

>>> On 17.10.12 at 10:39, Ian Campbell <ian.campbell@citrix.com> wrote:
> I saw this on ARM:
> linux/drivers/xen/dbgp.c:11:23: warning: unused variable 'ctrlr' 
> [-Wunused-variable]
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Jan Beulich <JBeulich@suse.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

> ---
>  drivers/xen/dbgp.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/xen/dbgp.c b/drivers/xen/dbgp.c
> index 42569c7..f3ccc80 100644
> --- a/drivers/xen/dbgp.c
> +++ b/drivers/xen/dbgp.c
> @@ -8,7 +8,9 @@
>  
>  static int xen_dbgp_op(struct usb_hcd *hcd, int op)
>  {
> +#ifdef CONFIG_PCI
>  	const struct device *ctrlr = hcd_to_bus(hcd)->controller;
> +#endif
>  	struct physdev_dbgp_op dbgp;
>  
>  	if (!xen_initial_domain())
> -- 
> 1.7.2.5

Re: [PATCH 04/10] xen: dbgp: Fix warning when CONFIG_PCI is not enabled.

[Stefano Stabellini]

On Wed, 17 Oct 2012, Ian Campbell wrote:
> I saw this on ARM:
> linux/drivers/xen/dbgp.c:11:23: warning: unused variable 'ctrlr' [-Wunused-variable]
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Jan Beulich <JBeulich@suse.com>


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

>  drivers/xen/dbgp.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/xen/dbgp.c b/drivers/xen/dbgp.c
> index 42569c7..f3ccc80 100644
> --- a/drivers/xen/dbgp.c
> +++ b/drivers/xen/dbgp.c
> @@ -8,7 +8,9 @@
>  
>  static int xen_dbgp_op(struct usb_hcd *hcd, int op)
>  {
> +#ifdef CONFIG_PCI
>  	const struct device *ctrlr = hcd_to_bus(hcd)->controller;
> +#endif
>  	struct physdev_dbgp_op dbgp;
>  
>  	if (!xen_initial_domain())
> -- 
> 1.7.2.5
> 

[PATCH 05/10] xen: events: pirq_check_eoi_map is X86 specific

[Ian Campbell]

On ARM I see:
drivers/xen/events.c:280:13: warning: 'pirq_check_eoi_map' defined but not used
[-Wunused-function]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 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 59e10a1..912ac81 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -115,7 +115,9 @@ struct irq_info {
 #define PIRQ_SHAREABLE	(1 << 1)
 
 static int *evtchn_to_irq;
+#ifdef CONFIG_X86
 static unsigned long *pirq_eoi_map;
+#endif
 static bool (*pirq_needs_eoi)(unsigned irq);
 
 static DEFINE_PER_CPU(unsigned long [NR_EVENT_CHANNELS/BITS_PER_LONG],
@@ -277,10 +279,12 @@ static unsigned int cpu_from_evtchn(unsigned int evtchn)
 	return ret;
 }
 
+#ifdef CONFIG_X86
 static bool pirq_check_eoi_map(unsigned irq)
 {
 	return test_bit(pirq_from_irq(irq), pirq_eoi_map);
 }
+#endif
 
 static bool pirq_needs_eoi_flag(unsigned irq)
 {
-- 
1.7.2.5

Re: [PATCH 05/10] xen: events: pirq_check_eoi_map is X86 specific

[Stefano Stabellini]

On Wed, 17 Oct 2012, Ian Campbell wrote:
> On ARM I see:
> drivers/xen/events.c:280:13: warning: 'pirq_check_eoi_map' defined but not used
> [-Wunused-function]
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

I hate the proliferation of #ifdefs but in this case it might be the
only thing to do


>  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 59e10a1..912ac81 100644
> --- a/drivers/xen/events.c
> +++ b/drivers/xen/events.c
> @@ -115,7 +115,9 @@ struct irq_info {
>  #define PIRQ_SHAREABLE	(1 << 1)
>  
>  static int *evtchn_to_irq;
> +#ifdef CONFIG_X86
>  static unsigned long *pirq_eoi_map;
> +#endif
>  static bool (*pirq_needs_eoi)(unsigned irq);
>  
>  static DEFINE_PER_CPU(unsigned long [NR_EVENT_CHANNELS/BITS_PER_LONG],
> @@ -277,10 +279,12 @@ static unsigned int cpu_from_evtchn(unsigned int evtchn)
>  	return ret;
>  }
>  
> +#ifdef CONFIG_X86
>  static bool pirq_check_eoi_map(unsigned irq)
>  {
>  	return test_bit(pirq_from_irq(irq), pirq_eoi_map);
>  }
> +#endif
>  
>  static bool pirq_needs_eoi_flag(unsigned irq)
>  {
> -- 
> 1.7.2.5
> 

Re: [PATCH 05/10] xen: events: pirq_check_eoi_map is X86 specific

[Ian Campbell]

On Wed, 2012-10-17 at 18:06 +0100, Stefano Stabellini wrote:
> On Wed, 17 Oct 2012, Ian Campbell wrote:
> > On ARM I see:
> > drivers/xen/events.c:280:13: warning: 'pirq_check_eoi_map' defined but not used
> > [-Wunused-function]
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> I hate the proliferation of #ifdefs but in this case it might be the
> only thing to do

I wondered if there was any chance that ARM might want need an eoi map
in the future? I suppose the real question is whether any future port
might want it -- to which the answer is unknowable until one shows up
but probably "yes" given enough architectures ;-)

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 59e10a1..912ac81 100644
> > --- a/drivers/xen/events.c
> > +++ b/drivers/xen/events.c
> > @@ -115,7 +115,9 @@ struct irq_info {
> >  #define PIRQ_SHAREABLE	(1 << 1)
> >  
> >  static int *evtchn_to_irq;
> > +#ifdef CONFIG_X86
> >  static unsigned long *pirq_eoi_map;
> > +#endif
> >  static bool (*pirq_needs_eoi)(unsigned irq);
> >  
> >  static DEFINE_PER_CPU(unsigned long [NR_EVENT_CHANNELS/BITS_PER_LONG],
> > @@ -277,10 +279,12 @@ static unsigned int cpu_from_evtchn(unsigned int evtchn)
> >  	return ret;
> >  }
> >  
> > +#ifdef CONFIG_X86
> >  static bool pirq_check_eoi_map(unsigned irq)
> >  {
> >  	return test_bit(pirq_from_irq(irq), pirq_eoi_map);
> >  }
> > +#endif
> >  
> >  static bool pirq_needs_eoi_flag(unsigned irq)
> >  {
> > -- 
> > 1.7.2.5
> > 

[PATCH 06/10] xen: XENMEM_translate_gpfn_list was remove ages ago and is unused.

[Ian Campbell]

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

diff --git a/include/xen/interface/memory.h b/include/xen/interface/memory.h
index b66d04c..90712e2 100644
--- a/include/xen/interface/memory.h
+++ b/include/xen/interface/memory.h
@@ -179,28 +179,8 @@ struct xen_add_to_physmap {
 };
 DEFINE_GUEST_HANDLE_STRUCT(xen_add_to_physmap);
 
-/*
- * Translates a list of domain-specific GPFNs into MFNs. Returns a -ve error
- * code on failure. This call only works for auto-translated guests.
- */
-#define XENMEM_translate_gpfn_list  8
-struct xen_translate_gpfn_list {
-    /* Which domain to translate for? */
-    domid_t domid;
-
-    /* Length of list. */
-    xen_ulong_t nr_gpfns;
-
-    /* List of GPFNs to translate. */
-    GUEST_HANDLE(ulong) gpfn_list;
-
-    /*
-     * Output list to contain MFN translations. May be the same as the input
-     * list (in which case each input GPFN is overwritten with the output MFN).
-     */
-    GUEST_HANDLE(ulong) mfn_list;
-};
-DEFINE_GUEST_HANDLE_STRUCT(xen_translate_gpfn_list);
+/*** REMOVED ***/
+/*#define XENMEM_translate_gpfn_list  8*/
 
 /*
  * Returns the pseudo-physical memory map as it was when the domain
-- 
1.7.2.5

[PATCH 07/10] xen: grant: use xen_pfn_t type for frame_list.

[Ian Campbell]

This correctly sizes it as 64 bit on ARM but leaves it as unsigned
long on x86 (therefore no intended change on x86).

The long and ulong guest handles are now unused (and a bit dangerous)
so remove them.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 arch/arm/include/asm/xen/interface.h |    2 --
 arch/arm/xen/grant-table.c           |    2 +-
 arch/x86/include/asm/xen/interface.h |    2 --
 drivers/xen/grant-table.c            |    8 ++++----
 include/xen/grant_table.h            |    2 +-
 include/xen/interface/grant_table.h  |    2 +-
 6 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/arch/arm/include/asm/xen/interface.h b/arch/arm/include/asm/xen/interface.h
index 62160f2..1d6ef9c 100644
--- a/arch/arm/include/asm/xen/interface.h
+++ b/arch/arm/include/asm/xen/interface.h
@@ -37,10 +37,8 @@ typedef uint64_t xen_ulong_t;
 /* Guest handles for primitive C types. */
 __DEFINE_GUEST_HANDLE(uchar, unsigned char);
 __DEFINE_GUEST_HANDLE(uint,  unsigned int);
-__DEFINE_GUEST_HANDLE(ulong, unsigned long);
 DEFINE_GUEST_HANDLE(char);
 DEFINE_GUEST_HANDLE(int);
-DEFINE_GUEST_HANDLE(long);
 DEFINE_GUEST_HANDLE(void);
 DEFINE_GUEST_HANDLE(uint64_t);
 DEFINE_GUEST_HANDLE(uint32_t);
diff --git a/arch/arm/xen/grant-table.c b/arch/arm/xen/grant-table.c
index dbd1330..859a9bb 100644
--- a/arch/arm/xen/grant-table.c
+++ b/arch/arm/xen/grant-table.c
@@ -33,7 +33,7 @@
 #include <xen/page.h>
 #include <xen/grant_table.h>
 
-int arch_gnttab_map_shared(unsigned long *frames, unsigned long nr_gframes,
+int arch_gnttab_map_shared(xen_pfn_t *frames, unsigned long nr_gframes,
 			   unsigned long max_nr_gframes,
 			   void **__shared)
 {
diff --git a/arch/x86/include/asm/xen/interface.h b/arch/x86/include/asm/xen/interface.h
index ab3c67c..54d52ff 100644
--- a/arch/x86/include/asm/xen/interface.h
+++ b/arch/x86/include/asm/xen/interface.h
@@ -57,10 +57,8 @@ typedef unsigned long xen_ulong_t;
 /* Guest handles for primitive C types. */
 __DEFINE_GUEST_HANDLE(uchar, unsigned char);
 __DEFINE_GUEST_HANDLE(uint,  unsigned int);
-__DEFINE_GUEST_HANDLE(ulong, unsigned long);
 DEFINE_GUEST_HANDLE(char);
 DEFINE_GUEST_HANDLE(int);
-DEFINE_GUEST_HANDLE(long);
 DEFINE_GUEST_HANDLE(void);
 DEFINE_GUEST_HANDLE(uint64_t);
 DEFINE_GUEST_HANDLE(uint32_t);
diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index b2b0a37..b91f14e 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -84,7 +84,7 @@ struct gnttab_ops {
 	 * nr_gframes is the number of frames to map grant table. Returning
 	 * GNTST_okay means success and negative value means failure.
 	 */
-	int (*map_frames)(unsigned long *frames, unsigned int nr_gframes);
+	int (*map_frames)(xen_pfn_t *frames, unsigned int nr_gframes);
 	/*
 	 * Release a list of frames which are mapped in map_frames for grant
 	 * entry status.
@@ -960,7 +960,7 @@ static unsigned nr_status_frames(unsigned nr_grant_frames)
 	return (nr_grant_frames * GREFS_PER_GRANT_FRAME + SPP - 1) / SPP;
 }
 
-static int gnttab_map_frames_v1(unsigned long *frames, unsigned int nr_gframes)
+static int gnttab_map_frames_v1(xen_pfn_t *frames, unsigned int nr_gframes)
 {
 	int rc;
 
@@ -977,7 +977,7 @@ static void gnttab_unmap_frames_v1(void)
 	arch_gnttab_unmap(gnttab_shared.addr, nr_grant_frames);
 }
 
-static int gnttab_map_frames_v2(unsigned long *frames, unsigned int nr_gframes)
+static int gnttab_map_frames_v2(xen_pfn_t *frames, unsigned int nr_gframes)
 {
 	uint64_t *sframes;
 	unsigned int nr_sframes;
@@ -1029,7 +1029,7 @@ static void gnttab_unmap_frames_v2(void)
 static int gnttab_map(unsigned int start_idx, unsigned int end_idx)
 {
 	struct gnttab_setup_table setup;
-	unsigned long *frames;
+	xen_pfn_t *frames;
 	unsigned int nr_gframes = end_idx + 1;
 	int rc;
 
diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
index aecee9d..694dcaf 100644
--- a/include/xen/grant_table.h
+++ b/include/xen/grant_table.h
@@ -170,7 +170,7 @@ gnttab_set_unmap_op(struct gnttab_unmap_grant_ref *unmap, phys_addr_t addr,
 	unmap->dev_bus_addr = 0;
 }
 
-int arch_gnttab_map_shared(unsigned long *frames, unsigned long nr_gframes,
+int arch_gnttab_map_shared(xen_pfn_t *frames, unsigned long nr_gframes,
 			   unsigned long max_nr_gframes,
 			   void **__shared);
 int arch_gnttab_map_status(uint64_t *frames, unsigned long nr_gframes,
diff --git a/include/xen/interface/grant_table.h b/include/xen/interface/grant_table.h
index f9f8b97..e40fae9 100644
--- a/include/xen/interface/grant_table.h
+++ b/include/xen/interface/grant_table.h
@@ -310,7 +310,7 @@ struct gnttab_setup_table {
     uint32_t nr_frames;
     /* OUT parameters. */
     int16_t  status;              /* GNTST_* */
-    GUEST_HANDLE(ulong) frame_list;
+    GUEST_HANDLE(xen_pfn_t) frame_list;
 };
 DEFINE_GUEST_HANDLE_STRUCT(gnttab_setup_table);
 
-- 
1.7.2.5

Re: [PATCH 07/10] xen: grant: use xen_pfn_t type for frame_list.

[Stefano Stabellini]

On Wed, 17 Oct 2012, Ian Campbell wrote:
> This correctly sizes it as 64 bit on ARM but leaves it as unsigned
> long on x86 (therefore no intended change on x86).
> 
> The long and ulong guest handles are now unused (and a bit dangerous)
> so remove them.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>


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

>  arch/arm/include/asm/xen/interface.h |    2 --
>  arch/arm/xen/grant-table.c           |    2 +-
>  arch/x86/include/asm/xen/interface.h |    2 --
>  drivers/xen/grant-table.c            |    8 ++++----
>  include/xen/grant_table.h            |    2 +-
>  include/xen/interface/grant_table.h  |    2 +-
>  6 files changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm/include/asm/xen/interface.h b/arch/arm/include/asm/xen/interface.h
> index 62160f2..1d6ef9c 100644
> --- a/arch/arm/include/asm/xen/interface.h
> +++ b/arch/arm/include/asm/xen/interface.h
> @@ -37,10 +37,8 @@ typedef uint64_t xen_ulong_t;
>  /* Guest handles for primitive C types. */
>  __DEFINE_GUEST_HANDLE(uchar, unsigned char);
>  __DEFINE_GUEST_HANDLE(uint,  unsigned int);
> -__DEFINE_GUEST_HANDLE(ulong, unsigned long);
>  DEFINE_GUEST_HANDLE(char);
>  DEFINE_GUEST_HANDLE(int);
> -DEFINE_GUEST_HANDLE(long);
>  DEFINE_GUEST_HANDLE(void);
>  DEFINE_GUEST_HANDLE(uint64_t);
>  DEFINE_GUEST_HANDLE(uint32_t);
> diff --git a/arch/arm/xen/grant-table.c b/arch/arm/xen/grant-table.c
> index dbd1330..859a9bb 100644
> --- a/arch/arm/xen/grant-table.c
> +++ b/arch/arm/xen/grant-table.c
> @@ -33,7 +33,7 @@
>  #include <xen/page.h>
>  #include <xen/grant_table.h>
>  
> -int arch_gnttab_map_shared(unsigned long *frames, unsigned long nr_gframes,
> +int arch_gnttab_map_shared(xen_pfn_t *frames, unsigned long nr_gframes,
>  			   unsigned long max_nr_gframes,
>  			   void **__shared)
>  {
> diff --git a/arch/x86/include/asm/xen/interface.h b/arch/x86/include/asm/xen/interface.h
> index ab3c67c..54d52ff 100644
> --- a/arch/x86/include/asm/xen/interface.h
> +++ b/arch/x86/include/asm/xen/interface.h
> @@ -57,10 +57,8 @@ typedef unsigned long xen_ulong_t;
>  /* Guest handles for primitive C types. */
>  __DEFINE_GUEST_HANDLE(uchar, unsigned char);
>  __DEFINE_GUEST_HANDLE(uint,  unsigned int);
> -__DEFINE_GUEST_HANDLE(ulong, unsigned long);
>  DEFINE_GUEST_HANDLE(char);
>  DEFINE_GUEST_HANDLE(int);
> -DEFINE_GUEST_HANDLE(long);
>  DEFINE_GUEST_HANDLE(void);
>  DEFINE_GUEST_HANDLE(uint64_t);
>  DEFINE_GUEST_HANDLE(uint32_t);
> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> index b2b0a37..b91f14e 100644
> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -84,7 +84,7 @@ struct gnttab_ops {
>  	 * nr_gframes is the number of frames to map grant table. Returning
>  	 * GNTST_okay means success and negative value means failure.
>  	 */
> -	int (*map_frames)(unsigned long *frames, unsigned int nr_gframes);
> +	int (*map_frames)(xen_pfn_t *frames, unsigned int nr_gframes);
>  	/*
>  	 * Release a list of frames which are mapped in map_frames for grant
>  	 * entry status.
> @@ -960,7 +960,7 @@ static unsigned nr_status_frames(unsigned nr_grant_frames)
>  	return (nr_grant_frames * GREFS_PER_GRANT_FRAME + SPP - 1) / SPP;
>  }
>  
> -static int gnttab_map_frames_v1(unsigned long *frames, unsigned int nr_gframes)
> +static int gnttab_map_frames_v1(xen_pfn_t *frames, unsigned int nr_gframes)
>  {
>  	int rc;
>  
> @@ -977,7 +977,7 @@ static void gnttab_unmap_frames_v1(void)
>  	arch_gnttab_unmap(gnttab_shared.addr, nr_grant_frames);
>  }
>  
> -static int gnttab_map_frames_v2(unsigned long *frames, unsigned int nr_gframes)
> +static int gnttab_map_frames_v2(xen_pfn_t *frames, unsigned int nr_gframes)
>  {
>  	uint64_t *sframes;
>  	unsigned int nr_sframes;
> @@ -1029,7 +1029,7 @@ static void gnttab_unmap_frames_v2(void)
>  static int gnttab_map(unsigned int start_idx, unsigned int end_idx)
>  {
>  	struct gnttab_setup_table setup;
> -	unsigned long *frames;
> +	xen_pfn_t *frames;
>  	unsigned int nr_gframes = end_idx + 1;
>  	int rc;
>  
> diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
> index aecee9d..694dcaf 100644
> --- a/include/xen/grant_table.h
> +++ b/include/xen/grant_table.h
> @@ -170,7 +170,7 @@ gnttab_set_unmap_op(struct gnttab_unmap_grant_ref *unmap, phys_addr_t addr,
>  	unmap->dev_bus_addr = 0;
>  }
>  
> -int arch_gnttab_map_shared(unsigned long *frames, unsigned long nr_gframes,
> +int arch_gnttab_map_shared(xen_pfn_t *frames, unsigned long nr_gframes,
>  			   unsigned long max_nr_gframes,
>  			   void **__shared);
>  int arch_gnttab_map_status(uint64_t *frames, unsigned long nr_gframes,
> diff --git a/include/xen/interface/grant_table.h b/include/xen/interface/grant_table.h
> index f9f8b97..e40fae9 100644
> --- a/include/xen/interface/grant_table.h
> +++ b/include/xen/interface/grant_table.h
> @@ -310,7 +310,7 @@ struct gnttab_setup_table {
>      uint32_t nr_frames;
>      /* OUT parameters. */
>      int16_t  status;              /* GNTST_* */
> -    GUEST_HANDLE(ulong) frame_list;
> +    GUEST_HANDLE(xen_pfn_t) frame_list;
>  };
>  DEFINE_GUEST_HANDLE_STRUCT(gnttab_setup_table);
>  
> -- 
> 1.7.2.5
> 

[PATCH 08/10] 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 31ab82f..d7bd1b3 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

Re: [PATCH 08/10] xen: balloon: don't include e820.h

[Stefano Stabellini]

On Wed, 17 Oct 2012, Ian Campbell wrote:
> This breaks on !X86 and AFAICT is not required on X86 either.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.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 31ab82f..d7bd1b3 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 09/10] 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 d7bd1b3..d6886d9 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 09/10] xen: balloon: use correct type for frame_list

[Stefano Stabellini]

On Wed, 17 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 d7bd1b3..d6886d9 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++)

I know that we agreed to this change but it still gives me the creeps.
I would love a comment either in the code or in the commit message,
explaining why it is safe to pass a xen_pfn_t (potentially 64 bit) to 
set_phys_to_machine.

Re: [PATCH 09/10] xen: balloon: use correct type for frame_list

[Konrad Rzeszutek Wilk]

On Wed, Oct 17, 2012 at 06:28:35PM +0100, Stefano Stabellini wrote:
> On Wed, 17 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 d7bd1b3..d6886d9 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++)
> 
> I know that we agreed to this change but it still gives me the creeps.
> I would love a comment either in the code or in the commit message,
> explaining why it is safe to pass a xen_pfn_t (potentially 64 bit) to 
> set_phys_to_machine.


I applied all the patches in this patchset except this one. Will
apply once a new version is out.

Re: [PATCH 09/10] xen: balloon: use correct type for frame_list

[Ian Campbell]

On Wed, 2012-10-17 at 18:32 +0100, Konrad Rzeszutek Wilk wrote:
> On Wed, Oct 17, 2012 at 06:28:35PM +0100, Stefano Stabellini wrote:
> > On Wed, 17 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 d7bd1b3..d6886d9 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++)
> > 
> > I know that we agreed to this change but it still gives me the creeps.
> > I would love a comment either in the code or in the commit message,
> > explaining why it is safe to pass a xen_pfn_t (potentially 64 bit) to 
> > set_phys_to_machine.
> 
> 
> I applied all the patches in this patchset except this one. Will
> apply once a new version is out.

The patch is better as an incremental one I think since it doesn't
really have much to do with this change in particular, really its a more
generic facet of Linux's (totally reasonable) choice of unsigned long
for pfns and not anything to do with Xen or p2ms or anything like that.
The comment below could just as well be in a generic location and read
"hardware might support more PFNs than the kernel is prepared to deal
with in its unsigned long pfns".

But whatever, here is a comment.

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

>From 7bbf07e67b3b93680ee229a4a8e8edabdbea072e Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@citrix.com>
Date: Thu, 18 Oct 2012 08:22:03 +0100
Subject: [PATCH] xen: arm: comment on why 64-bit xen_pfn_t is safe even on 32 bit

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

diff --git a/arch/arm/include/asm/xen/interface.h b/arch/arm/include/asm/xen/interface.h
index 007e6da..1151188 100644
--- a/arch/arm/include/asm/xen/interface.h
+++ b/arch/arm/include/asm/xen/interface.h
@@ -29,7 +29,13 @@
 
 #ifndef __ASSEMBLY__
 /* Explicitly size integers that represent pfns in the interface with
- * Xen so that we can have one ABI that works for 32 and 64 bit guests. */
+ * Xen so that we can have one ABI that works for 32 and 64 bit guests.
+ * Note that this means that the xen_pfn_t type may be capable of
+ * representing pfn's which the guest cannot represent in its own pfn
+ * type. However since pfn space is controlled by the guest this is
+ * fine since it simply wouldn't be able to create any sure pfns in
+ * the first place.
+ */
 typedef uint64_t xen_pfn_t;
 #define PRI_xen_pfn "llx"
 typedef uint64_t xen_ulong_t;
-- 
1.7.2.5





> 

[PATCH 10/10] xen: 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 ++++++++++---
 1 files changed, 10 insertions(+), 3 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 */
-- 
1.7.2.5

Re: [PATCH 10/10] xen: arm: make p2m operations NOPs

[Stefano Stabellini]

On Wed, 17 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 ++++++++++---
>  1 files changed, 10 insertions(+), 3 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 */
> -- 
> 1.7.2.5
>