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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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@xxxxxxxxxx>
> > > ---
> > >  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.

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