Xen devel - Oct 2012 - [PATCH 09/10] xen: balloon: use correct type for frame_list

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

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.

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.

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




>