Re: [Xen-devel] [PATCH v2] xen/arm: Allow balooning working with 1:1 memory mapping

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Mon, 16 Dec 2013, Jan Beulich wrote:
> >>> On 13.12.13 at 21:18, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -90,7 +90,7 @@ static void increase_reservation(struct memop_args *a)
> >  
> >  static void populate_physmap(struct memop_args *a)
> >  {
> > -    struct page_info *page;
> > +    struct page_info *page = NULL;
> 
> Why?
> 
> > @@ -122,7 +122,29 @@ static void populate_physmap(struct memop_args *a)
> >          }
> >          else
> >          {
> > -            page = alloc_domheap_pages(d, a->extent_order, a->memflags);
> > +            if ( d == dom0 && is_dom0_mapped_11() )
> > +            {
> > +                mfn = gpfn;
> > +                if (!mfn_valid(mfn))
> 
> Coding style.
> 
> > +                {
> > +                    gdprintk(XENLOG_INFO, "Invalid mfn 0x%"PRI_xen_pfn"\n",
> > +                             mfn);
> > +                    goto out;
> > +                }
> > +
> > +                page = mfn_to_page(mfn);
> > +                if ( !get_page(page, d) )
> > +                {
> > +                    gdprintk(XENLOG_INFO,
> > +                             "mfn 0x%"PRI_xen_pfn" doesn't belong to 
> > dom0\n",
> > +                             mfn);
> > +                    goto out;
> > +                }
> > +                put_page(page);
> > +            }
> 
> I think this hack doesn't belong into a common file. Rather than
> having the (anyway oddly named) is_dom0_mapped_11(), it
> would seem more clean to have this implemented by an inline
> function for ARM, returning the struct page_info * (and getting
> #define-d to NULL for all other cases, perhaps not even in an
> x86 header, but right in the source file).

Considering that having a 1:1 mapping for dom0 is conceivable even on
x86 (if you want PVH dom0 but your platform doesn't come with an IOMMU),
I would prefer the approach taken here. However I find your alternative
suggestion acceptable too.


> And even if we were to stay with the implementation here, for
> being at least half way sane the checking macro should take a
> domain argument rather than requiring the extra "d == dom0"
> check up front. Plus the "11" there is bogus - I first read this as
> "eleven" rather than "1:1", wondering what eleven here was
> about. I'd suggest something like is_domain_direct_mapped() or
> is_domain_identity_mapped().

I agree.

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