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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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

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.



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