[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





On 12/16/2013 09:08 AM, 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?

Spurious line from a previous version. I will remove it.


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

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

Jan


--
Julien Grall

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.