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

Re: [PATCH v4 4/5] xen/memory, tools: Avoid hardcoding GUEST_MAGIC_BASE in init-dom0less


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Henry Wang <xin.wang2@xxxxxxx>
  • Date: Fri, 19 Apr 2024 10:31:38 +0800
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass (sender ip is 165.204.84.17) smtp.rcpttodomain=suse.com smtp.mailfrom=amd.com; dmarc=pass (p=quarantine sp=quarantine pct=100) action=none header.from=amd.com; dkim=none (message not signed); arc=none (0)
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=tUZRukvAfZ5neDu0ABl0uR5S6MvEatH59LzEPtV7y58=; b=iQZwV9leuiQ9vqg3IiDwfnGhJI05RE7oMw3Q7rRxjVkwZkWe8RxnVRisyFN8N6ZO6rNIfAcCXC9hWpX12WpchdPLbzjSdGhNFddc8BdWeN5xZ+E64CVNz+HtixEj/g4PCz5jKYce5j43QhA6Oz2yQCXXnfatq4K4wrbmUff8+HcHJazf+pZP6Ve/Rc9lf082l8DCvqdOkI7RoDLMVyIS/GoglHEO3hpkx8a1iOH6/HUIZ4K7XpKwCynz8AZ3/WTMGXPhpnB4xye3EQdKIH1JDBIrM6J63lJw6yDbBh48YlyC18w9HYVeJq0vDBY5SPRuSgfpnmyzGxfCU6HmVXzcGQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=etTvsyyd48O5GVegNMEQ+4xUOaTFT5kThmtI0TXIUJJX1MufdSAB9PMY18biFqioBBqF50OkLJEvILx9fJ4Ol7isjYxKQ1ecyZlh/e8dADveMRwXIh5O79Jk6n8QVTuG7p6kerCbOYPbIKEv1uztwzX0uw0Uv0avE2e5z+NB4aMVxy4NvofpR3zUWGP52k4KmfP0ZIw2s66DEAq9KqJOP21o92MOYMpH1f22UBe5SYofbhWr1XkDt/5KOqzrxcuAyvrp5sSSn3o/+HYvkKLZHnS0F6rSRGh/4LOEectTu+3YO57Z+L3IrhsfXN9rFFHLvlBjU+3oF+hLXEU7FNgsTA==
  • Cc: Anthony PERARD <anthony.perard@xxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, "Julien Grall" <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, "Juergen Gross" <jgross@xxxxxxxx>, Alec Kwapis <alec.kwapis@xxxxxxxxxxxxx>, <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Daniel Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Fri, 19 Apr 2024 02:31:51 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

Hi Jan,

On 4/18/2024 8:54 PM, Jan Beulich wrote:
On 09.04.2024 06:53, Henry Wang wrote:
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -155,6 +155,14 @@ static void increase_reservation(struct memop_args *a)
      a->nr_done = i;
  }
+/*
+ * Alias of _MEMF_no_refcount to avoid introduction of a new, single-use flag.
+ * This flag should be used for populate_physmap() only as a re-purposing of
+ * _MEMF_no_refcount to force a non-1:1 allocation from domheap.
+ */
+#define _MEMF_force_heap_alloc _MEMF_no_refcount
+#define  MEMF_force_heap_alloc (1U<<_MEMF_force_heap_alloc)
Nit (style): Blanks around << please.

Also do you really need both constants? I dont think so.

Plus please make sure to #undef the constant once no longer needed, to
help spotting / avoiding misuses.

Sounds good, I will fix the NIT, drop the first #define and properly add #undef.

@@ -219,7 +227,8 @@ static void populate_physmap(struct memop_args *a)
          }
          else
          {
-            if ( is_domain_direct_mapped(d) )
+            if ( is_domain_direct_mapped(d) &&
+                 !(a->memflags & MEMF_force_heap_alloc) )
              {
                  mfn = _mfn(gpfn);
@@ -246,7 +255,8 @@ static void populate_physmap(struct memop_args *a) mfn = _mfn(gpfn);
              }
-            else if ( is_domain_using_staticmem(d) )
+            else if ( is_domain_using_staticmem(d) &&
+                      !(a->memflags & MEMF_force_heap_alloc) )
              {
                  /*
                   * No easy way to guarantee the retrieved pages are 
contiguous,
@@ -271,6 +281,14 @@ static void populate_physmap(struct memop_args *a)
              }
              else
              {
+                /*
+                 * Avoid passing MEMF_force_heap_alloc down to
+                 * alloc_domheap_pages() where the meaning would be the
+                 * original MEMF_no_refcount.
+                 */
+                if ( unlikely(a->memflags & MEMF_force_heap_alloc) )
+                    a->memflags &= ~MEMF_force_heap_alloc;
As asked before: Why the if()?

I think there is no need to clear the flag if it is not set. But you are correct, the if is not needed. I can drop it.

@@ -1404,6 +1422,7 @@ long do_memory_op(unsigned long cmd, 
XEN_GUEST_HANDLE_PARAM(void) arg)
      {
      case XENMEM_increase_reservation:
      case XENMEM_decrease_reservation:
+    case XENMEM_populate_physmap_heap_alloc:
      case XENMEM_populate_physmap:
          if ( copy_from_guest(&reservation, arg, 1) )
              return start_extent;
Nit or not: Please insert the new case label last.

Sure.

@@ -1433,6 +1452,11 @@ long do_memory_op(unsigned long cmd, 
XEN_GUEST_HANDLE_PARAM(void) arg)
               && (reservation.mem_flags & XENMEMF_populate_on_demand) )
              args.memflags |= MEMF_populate_on_demand;
+ /* Assert flag is not set from construct_memop_from_reservation(). */
+        ASSERT(!(args.memflags & MEMF_force_heap_alloc));
+        if ( op == XENMEM_populate_physmap_heap_alloc )
+            args.memflags |= MEMF_force_heap_alloc;
Wouldn't this more logically live ...

          if ( xsm_memory_adjust_reservation(XSM_TARGET, curr_d, d) )
          {
              rcu_unlock_domain(d);
@@ -1453,7 +1477,7 @@ long do_memory_op(unsigned long cmd, 
XEN_GUEST_HANDLE_PARAM(void) arg)
          case XENMEM_decrease_reservation:
              decrease_reservation(&args);
              break;
here, as

           case XENMEM_populate_physmap_heap_alloc:
               ...
               fallthrough;

Ok.

-        default: /* XENMEM_populate_physmap */
+        default: /* XENMEM_populate_{physmap, physmap_heap_alloc} */
Otherwise: Just XENMEM_populate_physmap{,_heap_alloc} perhaps?

Sounds good, thanks for the suggestion.

--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -21,6 +21,7 @@
  #define XENMEM_increase_reservation 0
  #define XENMEM_decrease_reservation 1
  #define XENMEM_populate_physmap     6
+#define XENMEM_populate_physmap_heap_alloc 29
Without a comment, how is one supposed to know what the difference is of
this new sub-op compared to the "normal" one? I actually wonder whether
referring to a Xen internal (allocation requested to come from the heap)
is actually a good idea here. I'm inclined to suggest to name this after
the purpose it has from the guest or tool stack perspective.

Speaking of which: Is this supposed to be guest-accessible, or is it
intended for tool-stack use only (I have to admit I don't even know where
init-dom0less actually runs)? In the latter case that also wants enforcing.
This may require an adjustment to the XSM hook in use here. Cc-ing Daniel
for possible advice.

This sub-op should be called by the init-dom0less application (toolstack side), which runs in Dom0. Daniel has proposed an alternative solution which is based on the hypfs. If we decide to go that route, I think I will rewrite the series. I will wait for the discussion settled. Thanks for looping in Daniel!

Kind regards,
Henry


Jan




 


Rackspace

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