[Xen-devel] [PATCH v4 0/7] purge free_shared_domheap_page()

[Xen-devel] [PATCH v4 0/7] purge free_shared_domheap_page()

[Paul Durrant]

The last patch is unrelated cleanup for something I happened to notice along the way. 

Paul Durrant (7):
  x86 / vmx: make apic_access_mfn type-safe
  x86 / hvm: add domain_relinquish_resources() method
  x86 / hvm: make domain_destroy() method optional
  x86 / vmx: move teardown from domain_destroy()...
  mm: make MEMF_no_refcount pages safe to assign
  x86 / vmx: use a MEMF_no_refcount domheap page for
    APIC_DEFAULT_PHYS_BASE
  mm: remove donate_page()

 xen/arch/arm/mm.c                  |  6 ----
 xen/arch/x86/hvm/hvm.c             |  7 +++-
 xen/arch/x86/hvm/mtrr.c            |  2 +-
 xen/arch/x86/hvm/svm/svm.c         |  5 ---
 xen/arch/x86/hvm/vmx/vmx.c         | 37 +++++++++++++++-------
 xen/arch/x86/mm.c                  | 51 ------------------------------
 xen/common/page_alloc.c            | 39 ++++++++++++++++-------
 xen/include/asm-arm/mm.h           |  5 ++-
 xen/include/asm-x86/hvm/hvm.h      |  1 +
 xen/include/asm-x86/hvm/vmx/vmcs.h |  2 +-
 xen/include/asm-x86/mm.h           |  9 +++---
 xen/include/xen/mm.h               |  2 --
 12 files changed, 72 insertions(+), 94 deletions(-)

-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v4 1/7] x86 / vmx: make apic_access_mfn type-safe

[Paul Durrant]

Use mfn_t rather than unsigned long and change previous tests against 0 to
tests against INVALID_MFN (also introducing initialization to that value).

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Acked-by: Kevin Tian <kevin.tian@intel.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>

v3:
 - Use _mfn(0) instead of INVALID_MFN

v2:
 - Set apic_access_mfn to INVALID_MFN in vmx_free_vlapic_mapping() to make
   the function idempotent
---
 xen/arch/x86/hvm/mtrr.c            |  2 +-
 xen/arch/x86/hvm/vmx/vmx.c         | 14 +++++++-------
 xen/include/asm-x86/hvm/vmx/vmcs.h |  2 +-
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c
index 5ad15eafe0..8356e8de3d 100644
--- a/xen/arch/x86/hvm/mtrr.c
+++ b/xen/arch/x86/hvm/mtrr.c
@@ -818,7 +818,7 @@ int epte_get_entry_emt(struct domain *d, unsigned long gfn, mfn_t mfn,
 
     if ( direct_mmio )
     {
-        if ( (mfn_x(mfn) ^ d->arch.hvm.vmx.apic_access_mfn) >> order )
+        if ( (mfn_x(mfn) ^ mfn_x(d->arch.hvm.vmx.apic_access_mfn)) >> order )
             return MTRR_TYPE_UNCACHABLE;
         if ( order )
             return -1;
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index f83f102638..b262d38a7c 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3034,7 +3034,7 @@ static int vmx_alloc_vlapic_mapping(struct domain *d)
     mfn = page_to_mfn(pg);
     clear_domain_page(mfn);
     share_xen_page_with_guest(pg, d, SHARE_rw);
-    d->arch.hvm.vmx.apic_access_mfn = mfn_x(mfn);
+    d->arch.hvm.vmx.apic_access_mfn = mfn;
 
     return set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE), mfn,
                               PAGE_ORDER_4K,
@@ -3043,24 +3043,24 @@ static int vmx_alloc_vlapic_mapping(struct domain *d)
 
 static void vmx_free_vlapic_mapping(struct domain *d)
 {
-    unsigned long mfn = d->arch.hvm.vmx.apic_access_mfn;
+    mfn_t mfn = d->arch.hvm.vmx.apic_access_mfn;
 
-    if ( mfn != 0 )
-        free_shared_domheap_page(mfn_to_page(_mfn(mfn)));
+    d->arch.hvm.vmx.apic_access_mfn = _mfn(0);
+    if ( !mfn_eq(mfn, _mfn(0)) )
+        free_shared_domheap_page(mfn_to_page(mfn));
 }
 
 static void vmx_install_vlapic_mapping(struct vcpu *v)
 {
     paddr_t virt_page_ma, apic_page_ma;
 
-    if ( v->domain->arch.hvm.vmx.apic_access_mfn == 0 )
+    if ( mfn_eq(v->domain->arch.hvm.vmx.apic_access_mfn, _mfn(0)) )
         return;
 
     ASSERT(cpu_has_vmx_virtualize_apic_accesses);
 
     virt_page_ma = page_to_maddr(vcpu_vlapic(v)->regs_page);
-    apic_page_ma = v->domain->arch.hvm.vmx.apic_access_mfn;
-    apic_page_ma <<= PAGE_SHIFT;
+    apic_page_ma = mfn_to_maddr(v->domain->arch.hvm.vmx.apic_access_mfn);
 
     vmx_vmcs_enter(v);
     __vmwrite(VIRTUAL_APIC_PAGE_ADDR, virt_page_ma);
diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
index a514299144..be4661a929 100644
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -59,7 +59,7 @@ struct ept_data {
 #define _VMX_DOMAIN_PML_ENABLED    0
 #define VMX_DOMAIN_PML_ENABLED     (1ul << _VMX_DOMAIN_PML_ENABLED)
 struct vmx_domain {
-    unsigned long apic_access_mfn;
+    mfn_t apic_access_mfn;
     /* VMX_DOMAIN_* */
     unsigned int status;
 
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v4 2/7] x86 / hvm: add domain_relinquish_resources() method

[Paul Durrant]

There are two functions in hvm.c to deal with tear-down and a domain:
hvm_domain_relinquish_resources() and hvm_domain_destroy(). However, only
the latter has an associated method in 'hvm_funcs'. This patch adds
a method for the former.

A subsequent patch will define a VMX implementation.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>

v2:
 - Make the new method optional and make it an alternative_vcall
---
 xen/arch/x86/hvm/hvm.c        | 3 +++
 xen/include/asm-x86/hvm/hvm.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index d899594888..6333ae6aba 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -716,6 +716,9 @@ int hvm_domain_initialise(struct domain *d)
 
 void hvm_domain_relinquish_resources(struct domain *d)
 {
+    if ( hvm_funcs.domain_relinquish_resources )
+        alternative_vcall(hvm_funcs.domain_relinquish_resources, d);
+
     if ( hvm_funcs.nhvm_domain_relinquish_resources )
         hvm_funcs.nhvm_domain_relinquish_resources(d);
 
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 09793c12e9..9eab1d7493 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -107,6 +107,7 @@ struct hvm_function_table {
      * Initialise/destroy HVM domain/vcpu resources
      */
     int  (*domain_initialise)(struct domain *d);
+    void (*domain_relinquish_resources)(struct domain *d);
     void (*domain_destroy)(struct domain *d);
     int  (*vcpu_initialise)(struct vcpu *v);
     void (*vcpu_destroy)(struct vcpu *v);
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v4 3/7] x86 / hvm: make domain_destroy() method optional

[Paul Durrant]

This method is currently empty for SVM so make it optional and, while in
the neighbourhood, make it an alternative_vcall().

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>

v4:
 - New in v4 (disaggregated from v3 patch #3)
---
 xen/arch/x86/hvm/hvm.c     | 4 +++-
 xen/arch/x86/hvm/svm/svm.c | 5 -----
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 6333ae6aba..0b93609a82 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -747,7 +747,9 @@ void hvm_domain_destroy(struct domain *d)
 
     hvm_destroy_cacheattr_region_list(d);
 
-    hvm_funcs.domain_destroy(d);
+    if ( hvm_funcs.domain_destroy )
+        alternative_vcall(hvm_funcs.domain_destroy, d);
+
     rtc_deinit(d);
     stdvga_deinit(d);
     vioapic_deinit(d);
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index b1c376d455..b7f67f9f03 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1155,10 +1155,6 @@ static int svm_domain_initialise(struct domain *d)
     return 0;
 }
 
-static void svm_domain_destroy(struct domain *d)
-{
-}
-
 static int svm_vcpu_initialise(struct vcpu *v)
 {
     int rc;
@@ -2425,7 +2421,6 @@ static struct hvm_function_table __initdata svm_function_table = {
     .cpu_up               = svm_cpu_up,
     .cpu_down             = svm_cpu_down,
     .domain_initialise    = svm_domain_initialise,
-    .domain_destroy       = svm_domain_destroy,
     .vcpu_initialise      = svm_vcpu_initialise,
     .vcpu_destroy         = svm_vcpu_destroy,
     .save_cpu_ctxt        = svm_save_vmcb_ctxt,
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v4 4/7] x86 / vmx: move teardown from domain_destroy()...

[Paul Durrant]

... to domain_relinquish_resources().

The teardown code frees the APICv page. This does not need to be done late
so do it in domain_relinquish_resources() rather than domain_destroy().

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>

v4:
  - New in v4 (disaggregated from v3 patch #3)
---
 xen/arch/x86/hvm/vmx/vmx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index b262d38a7c..606f3dc2eb 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -419,7 +419,7 @@ static int vmx_domain_initialise(struct domain *d)
     return 0;
 }
 
-static void vmx_domain_destroy(struct domain *d)
+static void vmx_domain_relinquish_resources(struct domain *d)
 {
     if ( !has_vlapic(d) )
         return;
@@ -2240,7 +2240,7 @@ static struct hvm_function_table __initdata vmx_function_table = {
     .cpu_up_prepare       = vmx_cpu_up_prepare,
     .cpu_dead             = vmx_cpu_dead,
     .domain_initialise    = vmx_domain_initialise,
-    .domain_destroy       = vmx_domain_destroy,
+    .domain_relinquish_resources = vmx_domain_relinquish_resources,
     .vcpu_initialise      = vmx_vcpu_initialise,
     .vcpu_destroy         = vmx_vcpu_destroy,
     .save_cpu_ctxt        = vmx_save_vmcs_ctxt,
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v4 5/7] mm: make MEMF_no_refcount pages safe to assign

[Paul Durrant]

Currently it is unsafe to assign a domheap page allocated with
MEMF_no_refcount to a domain because the domain't 'tot_pages' will not
be incremented, but will be decrement when the page is freed (since
free_domheap_pages() has no way of telling that the increment was skipped).

This patch allocates a new 'count_info' bit for a PGC_no_refcount flag
which is then used to mark domheap pages allocated with MEMF_no_refcount.
This then allows free_domheap_pages() to skip decrementing tot_pages when
appropriate and hence makes the pages safe to assign.

NOTE: The patch sets MEMF_no_refcount directly in alloc_domheap_pages()
      rather than in assign_pages() because the latter is called with
      MEMF_no_refcount by memory_exchange() as an optimization, to avoid
      too many calls to domain_adjust_tot_pages() (which acquires and
      releases the global 'heap_lock').

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien@xen.org>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Wei Liu <wl@xen.org>
Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>

v4:
 - New in v4
---
 xen/common/page_alloc.c  | 39 ++++++++++++++++++++++++++++-----------
 xen/include/asm-arm/mm.h |  5 ++++-
 xen/include/asm-x86/mm.h |  7 +++++--
 3 files changed, 37 insertions(+), 14 deletions(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 919a270587..6103f14a4e 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -460,6 +460,9 @@ unsigned long domain_adjust_tot_pages(struct domain *d, long pages)
 {
     long dom_before, dom_after, dom_claimed, sys_before, sys_after;
 
+    if ( !pages )
+        goto out;
+
     ASSERT(spin_is_locked(&d->page_alloc_lock));
     d->tot_pages += pages;
 
@@ -1389,6 +1392,8 @@ static void free_heap_pages(
 
     for ( i = 0; i < (1 << order); i++ )
     {
+        ASSERT(!(pg[i].count_info & PGC_no_refcount));
+
         /*
          * Cannot assume that count_info == 0, as there are some corner cases
          * where it isn't the case and yet it isn't a bug:
@@ -2314,11 +2319,6 @@ struct page_info *alloc_domheap_pages(
 
     if ( memflags & MEMF_no_owner )
         memflags |= MEMF_no_refcount;
-    else if ( (memflags & MEMF_no_refcount) && d )
-    {
-        ASSERT(!(memflags & MEMF_no_refcount));
-        return NULL;
-    }
 
     if ( !dma_bitsize )
         memflags &= ~MEMF_no_dma;
@@ -2331,11 +2331,20 @@ struct page_info *alloc_domheap_pages(
                                   memflags, d)) == NULL)) )
          return NULL;
 
-    if ( d && !(memflags & MEMF_no_owner) &&
-         assign_pages(d, pg, order, memflags) )
+    if ( d && !(memflags & MEMF_no_owner) )
     {
-        free_heap_pages(pg, order, memflags & MEMF_no_scrub);
-        return NULL;
+        if ( assign_pages(d, pg, order, memflags) )
+        {
+            free_heap_pages(pg, order, memflags & MEMF_no_scrub);
+            return NULL;
+        }
+        if ( memflags & MEMF_no_refcount )
+        {
+            unsigned long i;
+
+            for ( i = 0; i < (1 << order); i++ )
+                pg[i].count_info |= PGC_no_refcount;
+        }
     }
 
     return pg;
@@ -2368,24 +2377,32 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
 
         if ( likely(d) && likely(d != dom_cow) )
         {
+            long pages = 0;
+
             /* NB. May recursively lock from relinquish_memory(). */
             spin_lock_recursive(&d->page_alloc_lock);
 
             for ( i = 0; i < (1 << order); i++ )
             {
+                unsigned long count_info = pg[i].count_info;
+
                 if ( pg[i].u.inuse.type_info & PGT_count_mask )
                 {
                     printk(XENLOG_ERR
                            "pg[%u] MFN %"PRI_mfn" c=%#lx o=%u v=%#lx t=%#x\n",
                            i, mfn_x(page_to_mfn(pg + i)),
-                           pg[i].count_info, pg[i].v.free.order,
+                           count_info, pg[i].v.free.order,
                            pg[i].u.free.val, pg[i].tlbflush_timestamp);
                     BUG();
                 }
                 arch_free_heap_page(d, &pg[i]);
+                if ( count_info & PGC_no_refcount )
+                    pg[i].count_info &= ~PGC_no_refcount;
+                else
+                    pages--;
             }
 
-            drop_dom_ref = !domain_adjust_tot_pages(d, -(1 << order));
+            drop_dom_ref = !domain_adjust_tot_pages(d, pages);
 
             spin_unlock_recursive(&d->page_alloc_lock);
 
diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index 333efd3a60..1076cc9713 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -119,9 +119,12 @@ struct page_info
 #define PGC_state_offlined PG_mask(2, 9)
 #define PGC_state_free    PG_mask(3, 9)
 #define page_state_is(pg, st) (((pg)->count_info&PGC_state) == PGC_state_##st)
+/* Page is not reference counted */
+#define _PGC_no_refcount  PG_shift(10)
+#define PGC_no_refcount   PG_mask(1, 10)
 
 /* Count of references to this frame. */
-#define PGC_count_width   PG_shift(9)
+#define PGC_count_width   PG_shift(10)
 #define PGC_count_mask    ((1UL<<PGC_count_width)-1)
 
 /*
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index 2ca8882ad0..e75feea15e 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -77,9 +77,12 @@
 #define PGC_state_offlined PG_mask(2, 9)
 #define PGC_state_free    PG_mask(3, 9)
 #define page_state_is(pg, st) (((pg)->count_info&PGC_state) == PGC_state_##st)
+/* Page is not reference counted */
+#define _PGC_no_refcount  PG_shift(10)
+#define PGC_no_refcount   PG_mask(1, 10)
 
- /* Count of references to this frame. */
-#define PGC_count_width   PG_shift(9)
+/* Count of references to this frame. */
+#define PGC_count_width   PG_shift(10)
 #define PGC_count_mask    ((1UL<<PGC_count_width)-1)
 
 /*
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v4 6/7] x86 / vmx: use a MEMF_no_refcount domheap page for APIC_DEFAULT_PHYS_BASE

[Paul Durrant]

vmx_alloc_vlapic_mapping() currently contains some very odd looking code
that allocates a MEMF_no_owner domheap page and then shares with the guest
as if it were a xenheap page. This then requires vmx_free_vlapic_mapping()
to call a special function in the mm code: free_shared_domheap_page().

By using a MEMF_no_refcount domheap page instead, the odd looking code in
vmx_alloc_vlapic_mapping() can simply use get_page_and_type() to set up a
writable mapping before insertion in the P2M and vmx_free_vlapic_mapping()
can simply release the page using put_page_alloc_ref() followed by
put_page_and_type(). This then allows free_shared_domheap_page() to be
purged.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>

v4:
 - Use a MEMF_no_refcount page rather than a 'normal' page

v2:
 - Set an initial value for max_pages rather than avoiding the check in
   assign_pages()
 - Make domain_destroy() optional
---
 xen/arch/x86/hvm/vmx/vmx.c | 21 ++++++++++++++++++---
 xen/arch/x86/mm.c          | 10 ----------
 xen/include/asm-x86/mm.h   |  2 --
 3 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 606f3dc2eb..7423d2421b 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3028,12 +3028,22 @@ static int vmx_alloc_vlapic_mapping(struct domain *d)
     if ( !cpu_has_vmx_virtualize_apic_accesses )
         return 0;
 
-    pg = alloc_domheap_page(d, MEMF_no_owner);
+    pg = alloc_domheap_page(d, MEMF_no_refcount);
     if ( !pg )
         return -ENOMEM;
+
+    if ( !get_page_and_type(pg, d, PGT_writable_page) )
+    {
+        /*
+         * The domain can't possibly know about this page yet, so failure
+         * here is a clear indication of something fishy going on.
+         */
+        domain_crash(d);
+        return -ENODATA;
+    }
+
     mfn = page_to_mfn(pg);
     clear_domain_page(mfn);
-    share_xen_page_with_guest(pg, d, SHARE_rw);
     d->arch.hvm.vmx.apic_access_mfn = mfn;
 
     return set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE), mfn,
@@ -3047,7 +3057,12 @@ static void vmx_free_vlapic_mapping(struct domain *d)
 
     d->arch.hvm.vmx.apic_access_mfn = _mfn(0);
     if ( !mfn_eq(mfn, _mfn(0)) )
-        free_shared_domheap_page(mfn_to_page(mfn));
+    {
+        struct page_info *pg = mfn_to_page(mfn);
+
+        put_page_alloc_ref(pg);
+        put_page_and_type(pg);
+    }
 }
 
 static void vmx_install_vlapic_mapping(struct vcpu *v)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 654190e9e9..2a6d2e8af9 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -496,16 +496,6 @@ void share_xen_page_with_guest(struct page_info *page, struct domain *d,
     spin_unlock(&d->page_alloc_lock);
 }
 
-void free_shared_domheap_page(struct page_info *page)
-{
-    put_page_alloc_ref(page);
-    if ( !test_and_clear_bit(_PGC_xen_heap, &page->count_info) )
-        ASSERT_UNREACHABLE();
-    page->u.inuse.type_info = 0;
-    page_set_owner(page, NULL);
-    free_domheap_page(page);
-}
-
 void make_cr3(struct vcpu *v, mfn_t mfn)
 {
     struct domain *d = v->domain;
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index e75feea15e..036d7ac22f 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -320,8 +320,6 @@ struct page_info
 
 #define maddr_get_owner(ma)   (page_get_owner(maddr_to_page((ma))))
 
-extern void free_shared_domheap_page(struct page_info *page);
-
 #define frame_table ((struct page_info *)FRAMETABLE_VIRT_START)
 extern unsigned long max_page;
 extern unsigned long total_pages;
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v4 7/7] mm: remove donate_page()

[Paul Durrant]

This function appears to have no callers.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien@xen.org>
Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Wei Liu <wl@xen.org>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>

v4:
 - New in v4
---
 xen/arch/arm/mm.c    |  6 ------
 xen/arch/x86/mm.c    | 41 -----------------------------------------
 xen/include/xen/mm.h |  2 --
 3 files changed, 49 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 4d6c971f37..727107eefa 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1353,12 +1353,6 @@ void arch_dump_shared_mem_info(void)
 {
 }
 
-int donate_page(struct domain *d, struct page_info *page, unsigned int memflags)
-{
-    ASSERT_UNREACHABLE();
-    return -ENOSYS;
-}
-
 int steal_page(
     struct domain *d, struct page_info *page, unsigned int memflags)
 {
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 2a6d2e8af9..67351798c7 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4174,47 +4174,6 @@ long do_mmu_update(
 }
 #endif /* CONFIG_PV */
 
-int donate_page(
-    struct domain *d, struct page_info *page, unsigned int memflags)
-{
-    const struct domain *owner = dom_xen;
-
-    spin_lock(&d->page_alloc_lock);
-
-    if ( is_xen_heap_page(page) || ((owner = page_get_owner(page)) != NULL) )
-        goto fail;
-
-    if ( d->is_dying )
-        goto fail;
-
-    if ( page->count_info & ~(PGC_allocated | 1) )
-        goto fail;
-
-    if ( !(memflags & MEMF_no_refcount) )
-    {
-        if ( d->tot_pages >= d->max_pages )
-            goto fail;
-        if ( unlikely(domain_adjust_tot_pages(d, 1) == 1) )
-            get_knownalive_domain(d);
-    }
-
-    page->count_info = PGC_allocated | 1;
-    page_set_owner(page, d);
-    page_list_add_tail(page,&d->page_list);
-
-    spin_unlock(&d->page_alloc_lock);
-    return 0;
-
- fail:
-    spin_unlock(&d->page_alloc_lock);
-    gdprintk(XENLOG_WARNING, "Bad donate mfn %" PRI_mfn
-             " to d%d (owner d%d) caf=%08lx taf=%" PRtype_info "\n",
-             mfn_x(page_to_mfn(page)), d->domain_id,
-             owner ? owner->domain_id : DOMID_INVALID,
-             page->count_info, page->u.inuse.type_info);
-    return -EINVAL;
-}
-
 /*
  * Steal page will attempt to remove `page` from domain `d`.  Upon
  * return, `page` will be in a state similar to the state of a page
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 8d0ddfb60c..d0d095d9c7 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -599,8 +599,6 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,
 int __must_check guest_remove_page(struct domain *d, unsigned long gmfn);
 int __must_check steal_page(struct domain *d, struct page_info *page,
                             unsigned int memflags);
-int __must_check donate_page(struct domain *d, struct page_info *page,
-                             unsigned int memflags);
 
 #define RAM_TYPE_CONVENTIONAL 0x00000001
 #define RAM_TYPE_RESERVED     0x00000002
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 3/7] x86 / hvm: make domain_destroy() method optional

[Jan Beulich]

On 24.01.2020 16:30, Paul Durrant wrote:
> This method is currently empty for SVM so make it optional and, while in
> the neighbourhood, make it an alternative_vcall().
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 7/7] mm: remove donate_page()

[George Dunlap]

On 1/24/20 3:31 PM, Paul Durrant wrote:
> This function appears to have no callers.
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>

Acked-by: George Dunlap <george.dunlap@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 7/7] mm: remove donate_page()

[Julien Grall]

Hi Paul,

On 24/01/2020 15:31, Paul Durrant wrote:
> This function appears to have no callers.
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>

Acked-by: Julien Grall <julien@xen.org>

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 7/7] mm: remove donate_page()

[Andrew Cooper]

On 24/01/2020 15:31, Paul Durrant wrote:
> This function appears to have no callers.
>
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>

Looks like tmem was the sole user.  donate_page() was introduced by
6009f4ddb2 and the last caller was dropped by c492e19fdd.

This patch is standalone, and I can tweak the commit message on commit,
if you're happy with that.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 7/7] mm: remove donate_page()

[Durrant, Paul]

[-- Attachment #1.1: Type: text/plain, Size: 547 bytes --]



Sent from Workspace ONE Boxer
On 24 Jan 2020 17:57, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 24/01/2020 15:31, Paul Durrant wrote:
> > This function appears to have no callers.
> >
> > Signed-off-by: Paul Durrant <pdurrant@amazon.com>
>
> Looks like tmem was the sole user.  donate_page() was introduced by
> 6009f4ddb2 and the last caller was dropped by c492e19fdd.
>
> This patch is standalone, and I can tweak the commit message on commit,
> if you're happy with that.
>

Fine with me.

  Paul
> ~Andrew


[-- Attachment #1.2: Type: text/html, Size: 1127 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 4/7] x86 / vmx: move teardown from domain_destroy()...

[Jan Beulich]

On 24.01.2020 16:31, Paul Durrant wrote:
> ... to domain_relinquish_resources().
> 
> The teardown code frees the APICv page. This does not need to be done late
> so do it in domain_relinquish_resources() rather than domain_destroy().

For the normal domain destruction path this is fine. For the error path
of domain_create(), however, this will leak the page, as in this case
hvm_domain_relinquish_resources() won't be called. I'm afraid there
already is a similar issue with e.g. viridian_domain_deinit(). I guess
I'll make a patch.

Jan

> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> ---
> Cc: Jun Nakajima <jun.nakajima@intel.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Wei Liu <wl@xen.org>
> Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> Cc: George Dunlap <george.dunlap@citrix.com>
> 
> v4:
>   - New in v4 (disaggregated from v3 patch #3)
> ---
>  xen/arch/x86/hvm/vmx/vmx.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index b262d38a7c..606f3dc2eb 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -419,7 +419,7 @@ static int vmx_domain_initialise(struct domain *d)
>      return 0;
>  }
>  
> -static void vmx_domain_destroy(struct domain *d)
> +static void vmx_domain_relinquish_resources(struct domain *d)
>  {
>      if ( !has_vlapic(d) )
>          return;
> @@ -2240,7 +2240,7 @@ static struct hvm_function_table __initdata vmx_function_table = {
>      .cpu_up_prepare       = vmx_cpu_up_prepare,
>      .cpu_dead             = vmx_cpu_dead,
>      .domain_initialise    = vmx_domain_initialise,
> -    .domain_destroy       = vmx_domain_destroy,
> +    .domain_relinquish_resources = vmx_domain_relinquish_resources,
>      .vcpu_initialise      = vmx_vcpu_initialise,
>      .vcpu_destroy         = vmx_vcpu_destroy,
>      .save_cpu_ctxt        = vmx_save_vmcs_ctxt,
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 4/7] x86 / vmx: move teardown from domain_destroy()...

[Durrant, Paul]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 28 January 2020 08:15
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: xen-devel@lists.xenproject.org; Jun Nakajima <jun.nakajima@intel.com>;
> Kevin Tian <kevin.tian@intel.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné
> <roger.pau@citrix.com>; George Dunlap <george.dunlap@citrix.com>
> Subject: Re: [PATCH v4 4/7] x86 / vmx: move teardown from
> domain_destroy()...
> 
> On 24.01.2020 16:31, Paul Durrant wrote:
> > ... to domain_relinquish_resources().
> >
> > The teardown code frees the APICv page. This does not need to be done
> late
> > so do it in domain_relinquish_resources() rather than domain_destroy().
> 
> For the normal domain destruction path this is fine. For the error path
> of domain_create(), however, this will leak the page, as in this case
> hvm_domain_relinquish_resources() won't be called.

Well it's really arch_domain_create() that's at fault but, yes that needs fixing.

> I'm afraid there
> already is a similar issue with e.g. viridian_domain_deinit(). I guess
> I'll make a patch.
> 

Ok, thanks.

  Paul

> Jan
> 
> > Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> > ---
> > Cc: Jun Nakajima <jun.nakajima@intel.com>
> > Cc: Kevin Tian <kevin.tian@intel.com>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: Wei Liu <wl@xen.org>
> > Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> > Cc: George Dunlap <george.dunlap@citrix.com>
> >
> > v4:
> >   - New in v4 (disaggregated from v3 patch #3)
> > ---
> >  xen/arch/x86/hvm/vmx/vmx.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> > index b262d38a7c..606f3dc2eb 100644
> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -419,7 +419,7 @@ static int vmx_domain_initialise(struct domain *d)
> >      return 0;
> >  }
> >
> > -static void vmx_domain_destroy(struct domain *d)
> > +static void vmx_domain_relinquish_resources(struct domain *d)
> >  {
> >      if ( !has_vlapic(d) )
> >          return;
> > @@ -2240,7 +2240,7 @@ static struct hvm_function_table __initdata
> vmx_function_table = {
> >      .cpu_up_prepare       = vmx_cpu_up_prepare,
> >      .cpu_dead             = vmx_cpu_dead,
> >      .domain_initialise    = vmx_domain_initialise,
> > -    .domain_destroy       = vmx_domain_destroy,
> > +    .domain_relinquish_resources = vmx_domain_relinquish_resources,
> >      .vcpu_initialise      = vmx_vcpu_initialise,
> >      .vcpu_destroy         = vmx_vcpu_destroy,
> >      .save_cpu_ctxt        = vmx_save_vmcs_ctxt,
> >

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 4/7] x86 / vmx: move teardown from domain_destroy()...

[Jan Beulich]

On 28.01.2020 09:22, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 28 January 2020 08:15
>> To: Durrant, Paul <pdurrant@amazon.co.uk>
>> Cc: xen-devel@lists.xenproject.org; Jun Nakajima <jun.nakajima@intel.com>;
>> Kevin Tian <kevin.tian@intel.com>; Andrew Cooper
>> <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné
>> <roger.pau@citrix.com>; George Dunlap <george.dunlap@citrix.com>
>> Subject: Re: [PATCH v4 4/7] x86 / vmx: move teardown from
>> domain_destroy()...
>>
>> On 24.01.2020 16:31, Paul Durrant wrote:
>>> ... to domain_relinquish_resources().
>>>
>>> The teardown code frees the APICv page. This does not need to be done
>> late
>>> so do it in domain_relinquish_resources() rather than domain_destroy().
>>
>> For the normal domain destruction path this is fine. For the error path
>> of domain_create(), however, this will leak the page, as in this case
>> hvm_domain_relinquish_resources() won't be called.
> 
> Well it's really arch_domain_create() that's at fault but, yes that needs fixing.

Why arch_domain_create()? For HVM domains hvm_domain_initialise()
is the last thing tried, and hence no further cleanup is needed
(assuming hvm_domain_initialise() cleans up in case of failure
after itself). It's failures encountered by domain_create() after
arch_domain_create() has succeeded which are a problem here. In
this case arch_domain_destroy() will be called, but nothing down
from there has called hvm_domain_relinquish_resources() so far.

>> I'm afraid there
>> already is a similar issue with e.g. viridian_domain_deinit(). I guess
>> I'll make a patch.
> 
> Ok, thanks.

Which turned out to take more time because of other issues I've
found in the course, but I think I now have something I can
actually test.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 5/7] mm: make MEMF_no_refcount pages safe to assign

[Jan Beulich]

On 24.01.2020 16:31, Paul Durrant wrote:
> Currently it is unsafe to assign a domheap page allocated with
> MEMF_no_refcount to a domain because the domain't 'tot_pages' will not
> be incremented, but will be decrement when the page is freed (since
> free_domheap_pages() has no way of telling that the increment was skipped).
> 
> This patch allocates a new 'count_info' bit for a PGC_no_refcount flag
> which is then used to mark domheap pages allocated with MEMF_no_refcount.
> This then allows free_domheap_pages() to skip decrementing tot_pages when
> appropriate and hence makes the pages safe to assign.
> 
> NOTE: The patch sets MEMF_no_refcount directly in alloc_domheap_pages()
>       rather than in assign_pages() because the latter is called with
>       MEMF_no_refcount by memory_exchange() as an optimization, to avoid
>       too many calls to domain_adjust_tot_pages() (which acquires and
>       releases the global 'heap_lock').

I don't think there were any optimization thoughts with this. The
MEMF_no_refcount use is because otherwise for a domain with
tot_pages == max_pages the assignment would fail.

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -460,6 +460,9 @@ unsigned long domain_adjust_tot_pages(struct domain *d, long pages)
>  {
>      long dom_before, dom_after, dom_claimed, sys_before, sys_after;
>  
> +    if ( !pages )
> +        goto out;

Unrelated change? Are there, in fact, any callers passing in 0?
Oh, further down you add one which may do so, but then perhaps
better to make the caller not call here (as is done e.g. in
memory_exchange())?

> @@ -2331,11 +2331,20 @@ struct page_info *alloc_domheap_pages(
>                                    memflags, d)) == NULL)) )
>           return NULL;
>  
> -    if ( d && !(memflags & MEMF_no_owner) &&
> -         assign_pages(d, pg, order, memflags) )
> +    if ( d && !(memflags & MEMF_no_owner) )
>      {
> -        free_heap_pages(pg, order, memflags & MEMF_no_scrub);
> -        return NULL;
> +        if ( assign_pages(d, pg, order, memflags) )
> +        {
> +            free_heap_pages(pg, order, memflags & MEMF_no_scrub);
> +            return NULL;
> +        }
> +        if ( memflags & MEMF_no_refcount )
> +        {
> +            unsigned long i;
> +
> +            for ( i = 0; i < (1 << order); i++ )
> +                pg[i].count_info |= PGC_no_refcount;
> +        }

I would seem to me that this needs doing the other way around:
First set PGC_no_refcount, then assign_pages(). After all, the
moment assign_pages() drops its lock, the domain could also
decide to get rid of (some of) the pages again. For this (and
also to slightly simplify things in free_domheap_pages())
perhaps it would be better not to add that ASSERT() to
free_heap_pages(). The function shouldn't really be concerned
of any refcounting, and hence could as well be ignorant to
PGC_no_refcount being set on a page.

> @@ -2368,24 +2377,32 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
>  
>          if ( likely(d) && likely(d != dom_cow) )
>          {
> +            long pages = 0;
> +
>              /* NB. May recursively lock from relinquish_memory(). */
>              spin_lock_recursive(&d->page_alloc_lock);
>  
>              for ( i = 0; i < (1 << order); i++ )
>              {
> +                unsigned long count_info = pg[i].count_info;
> +
>                  if ( pg[i].u.inuse.type_info & PGT_count_mask )
>                  {
>                      printk(XENLOG_ERR
>                             "pg[%u] MFN %"PRI_mfn" c=%#lx o=%u v=%#lx t=%#x\n",
>                             i, mfn_x(page_to_mfn(pg + i)),
> -                           pg[i].count_info, pg[i].v.free.order,
> +                           count_info, pg[i].v.free.order,
>                             pg[i].u.free.val, pg[i].tlbflush_timestamp);
>                      BUG();
>                  }
>                  arch_free_heap_page(d, &pg[i]);
> +                if ( count_info & PGC_no_refcount )
> +                    pg[i].count_info &= ~PGC_no_refcount;
> +                else
> +                    pages--;

Not only to reduce code churn, may I recommend to avoid introducing
the local variable? There's no strict rule preventing
arch_free_heap_page() from possibly playing with the field you
latch up front.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 6/7] x86 / vmx: use a MEMF_no_refcount domheap page for APIC_DEFAULT_PHYS_BASE

[Jan Beulich]

On 24.01.2020 16:31, Paul Durrant wrote:
> vmx_alloc_vlapic_mapping() currently contains some very odd looking code
> that allocates a MEMF_no_owner domheap page and then shares with the guest
> as if it were a xenheap page. This then requires vmx_free_vlapic_mapping()
> to call a special function in the mm code: free_shared_domheap_page().
> 
> By using a MEMF_no_refcount domheap page instead, the odd looking code in
> vmx_alloc_vlapic_mapping() can simply use get_page_and_type() to set up a
> writable mapping before insertion in the P2M and vmx_free_vlapic_mapping()
> can simply release the page using put_page_alloc_ref() followed by
> put_page_and_type(). This then allows free_shared_domheap_page() to be
> purged.
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 5/7] mm: make MEMF_no_refcount pages safe to assign

[Durrant, Paul]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 28 January 2020 15:23
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: xen-devel@lists.xenproject.org; Andrew Cooper
> <andrew.cooper3@citrix.com>; George Dunlap <George.Dunlap@eu.citrix.com>;
> Ian Jackson <ian.jackson@eu.citrix.com>; Julien Grall <julien@xen.org>;
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com>; Roger Pau Monné <roger.pau@citrix.com>
> Subject: Re: [PATCH v4 5/7] mm: make MEMF_no_refcount pages safe to assign
> 
> On 24.01.2020 16:31, Paul Durrant wrote:
> > Currently it is unsafe to assign a domheap page allocated with
> > MEMF_no_refcount to a domain because the domain't 'tot_pages' will not
> > be incremented, but will be decrement when the page is freed (since
> > free_domheap_pages() has no way of telling that the increment was
> skipped).
> >
> > This patch allocates a new 'count_info' bit for a PGC_no_refcount flag
> > which is then used to mark domheap pages allocated with
> MEMF_no_refcount.
> > This then allows free_domheap_pages() to skip decrementing tot_pages
> when
> > appropriate and hence makes the pages safe to assign.
> >
> > NOTE: The patch sets MEMF_no_refcount directly in alloc_domheap_pages()
> >       rather than in assign_pages() because the latter is called with
> >       MEMF_no_refcount by memory_exchange() as an optimization, to avoid
> >       too many calls to domain_adjust_tot_pages() (which acquires and
> >       releases the global 'heap_lock').
> 
> I don't think there were any optimization thoughts with this. The
> MEMF_no_refcount use is because otherwise for a domain with
> tot_pages == max_pages the assignment would fail.
> 

That would not be the case if the calls to steal_page() further up didn't pass MEMF_no_refcount (which would be the correct thing to do if not passing it to assign_pages(). I had originally considered doing that because I think it allows the somewhat complex error path after assign_pages() to be dropped. But avoiding thrashing the global lock seemed a good reason to leave memory_exchange() the way it is.

> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -460,6 +460,9 @@ unsigned long domain_adjust_tot_pages(struct domain
> *d, long pages)
> >  {
> >      long dom_before, dom_after, dom_claimed, sys_before, sys_after;
> >
> > +    if ( !pages )
> > +        goto out;
> 
> Unrelated change? Are there, in fact, any callers passing in 0?
> Oh, further down you add one which may do so, but then perhaps
> better to make the caller not call here (as is done e.g. in
> memory_exchange())?

I think it's preferable for domain_adjust_tot_pages() to handle zero gracefully.

> 
> > @@ -2331,11 +2331,20 @@ struct page_info *alloc_domheap_pages(
> >                                    memflags, d)) == NULL)) )
> >           return NULL;
> >
> > -    if ( d && !(memflags & MEMF_no_owner) &&
> > -         assign_pages(d, pg, order, memflags) )
> > +    if ( d && !(memflags & MEMF_no_owner) )
> >      {
> > -        free_heap_pages(pg, order, memflags & MEMF_no_scrub);
> > -        return NULL;
> > +        if ( assign_pages(d, pg, order, memflags) )
> > +        {
> > +            free_heap_pages(pg, order, memflags & MEMF_no_scrub);
> > +            return NULL;
> > +        }
> > +        if ( memflags & MEMF_no_refcount )
> > +        {
> > +            unsigned long i;
> > +
> > +            for ( i = 0; i < (1 << order); i++ )
> > +                pg[i].count_info |= PGC_no_refcount;
> > +        }
> 
> I would seem to me that this needs doing the other way around:
> First set PGC_no_refcount, then assign_pages(). After all, the
> moment assign_pages() drops its lock, the domain could also
> decide to get rid of (some of) the pages again.

True. Yes, this needs to be swapped.

> For this (and
> also to slightly simplify things in free_domheap_pages())
> perhaps it would be better not to add that ASSERT() to
> free_heap_pages(). The function shouldn't really be concerned
> of any refcounting, and hence could as well be ignorant to
> PGC_no_refcount being set on a page.
> 

Not sure I understand here. What would you like to see free_heap_pages() assert?

> > @@ -2368,24 +2377,32 @@ void free_domheap_pages(struct page_info *pg,
> unsigned int order)
> >
> >          if ( likely(d) && likely(d != dom_cow) )
> >          {
> > +            long pages = 0;
> > +
> >              /* NB. May recursively lock from relinquish_memory(). */
> >              spin_lock_recursive(&d->page_alloc_lock);
> >
> >              for ( i = 0; i < (1 << order); i++ )
> >              {
> > +                unsigned long count_info = pg[i].count_info;
> > +
> >                  if ( pg[i].u.inuse.type_info & PGT_count_mask )
> >                  {
> >                      printk(XENLOG_ERR
> >                             "pg[%u] MFN %"PRI_mfn" c=%#lx o=%u v=%#lx
> t=%#x\n",
> >                             i, mfn_x(page_to_mfn(pg + i)),
> > -                           pg[i].count_info, pg[i].v.free.order,
> > +                           count_info, pg[i].v.free.order,
> >                             pg[i].u.free.val, pg[i].tlbflush_timestamp);
> >                      BUG();
> >                  }
> >                  arch_free_heap_page(d, &pg[i]);
> > +                if ( count_info & PGC_no_refcount )
> > +                    pg[i].count_info &= ~PGC_no_refcount;
> > +                else
> > +                    pages--;
> 
> Not only to reduce code churn, may I recommend to avoid introducing
> the local variable? There's no strict rule preventing
> arch_free_heap_page() from possibly playing with the field you
> latch up front.

Ok.

  Paul

> 
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 5/7] mm: make MEMF_no_refcount pages safe to assign

[Durrant, Paul]

> -----Original Message-----
[snip]
> > > @@ -2331,11 +2331,20 @@ struct page_info *alloc_domheap_pages(
> > >                                    memflags, d)) == NULL)) )
> > >           return NULL;
> > >
> > > -    if ( d && !(memflags & MEMF_no_owner) &&
> > > -         assign_pages(d, pg, order, memflags) )
> > > +    if ( d && !(memflags & MEMF_no_owner) )
> > >      {
> > > -        free_heap_pages(pg, order, memflags & MEMF_no_scrub);
> > > -        return NULL;
> > > +        if ( assign_pages(d, pg, order, memflags) )
> > > +        {
> > > +            free_heap_pages(pg, order, memflags & MEMF_no_scrub);
> > > +            return NULL;
> > > +        }
> > > +        if ( memflags & MEMF_no_refcount )
> > > +        {
> > > +            unsigned long i;
> > > +
> > > +            for ( i = 0; i < (1 << order); i++ )
> > > +                pg[i].count_info |= PGC_no_refcount;
> > > +        }
> >
> > I would seem to me that this needs doing the other way around:
> > First set PGC_no_refcount, then assign_pages(). After all, the
> > moment assign_pages() drops its lock, the domain could also
> > decide to get rid of (some of) the pages again.
> 
> True. Yes, this needs to be swapped.
> 
> > For this (and
> > also to slightly simplify things in free_domheap_pages())
> > perhaps it would be better not to add that ASSERT() to
> > free_heap_pages(). The function shouldn't really be concerned
> > of any refcounting, and hence could as well be ignorant to
> > PGC_no_refcount being set on a page.
> >
> 
> Not sure I understand here. What would you like to see free_heap_pages()
> assert?
> 

Oh, I misread. You want me to remove the ASSERT that I added... that's fine.

  Paul
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 5/7] mm: make MEMF_no_refcount pages safe to assign

[Jan Beulich]

On 28.01.2020 18:01, Durrant, Paul wrote:
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 28 January 2020 15:23
>>
>> On 24.01.2020 16:31, Paul Durrant wrote:
>>> Currently it is unsafe to assign a domheap page allocated with
>>> MEMF_no_refcount to a domain because the domain't 'tot_pages' will not
>>> be incremented, but will be decrement when the page is freed (since
>>> free_domheap_pages() has no way of telling that the increment was
>> skipped).
>>>
>>> This patch allocates a new 'count_info' bit for a PGC_no_refcount flag
>>> which is then used to mark domheap pages allocated with
>> MEMF_no_refcount.
>>> This then allows free_domheap_pages() to skip decrementing tot_pages
>> when
>>> appropriate and hence makes the pages safe to assign.
>>>
>>> NOTE: The patch sets MEMF_no_refcount directly in alloc_domheap_pages()
>>>       rather than in assign_pages() because the latter is called with
>>>       MEMF_no_refcount by memory_exchange() as an optimization, to avoid
>>>       too many calls to domain_adjust_tot_pages() (which acquires and
>>>       releases the global 'heap_lock').
>>
>> I don't think there were any optimization thoughts with this. The
>> MEMF_no_refcount use is because otherwise for a domain with
>> tot_pages == max_pages the assignment would fail.
>>
> 
> That would not be the case if the calls to steal_page() further up didn't
> pass MEMF_no_refcount (which would be the correct thing to do if not
> passing it to assign_pages().

No, that's not an option either: steal_page() would otherwise decrement
->tot_pages, allowing the domain to allocate new memory on another vCPU.
This would again result in the exchange failing for no reason. (And no,
I don't think a guest should be required to serialize e.g. ballooning
operations with exchanges.)

>>> --- a/xen/common/page_alloc.c
>>> +++ b/xen/common/page_alloc.c
>>> @@ -460,6 +460,9 @@ unsigned long domain_adjust_tot_pages(struct domain
>> *d, long pages)
>>>  {
>>>      long dom_before, dom_after, dom_claimed, sys_before, sys_after;
>>>
>>> +    if ( !pages )
>>> +        goto out;
>>
>> Unrelated change? Are there, in fact, any callers passing in 0?
>> Oh, further down you add one which may do so, but then perhaps
>> better to make the caller not call here (as is done e.g. in
>> memory_exchange())?
> 
> I think it's preferable for domain_adjust_tot_pages() to handle zero
> gracefully.

That's an option, but imo would then better be a separate change (to
also drop present guards of calls to the function).

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 5/7] mm: make MEMF_no_refcount pages safe to assign

[Durrant, Paul]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 29 January 2020 08:22
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: xen-devel@lists.xenproject.org; Andrew Cooper
> <andrew.cooper3@citrix.com>; George Dunlap <George.Dunlap@eu.citrix.com>;
> Ian Jackson <ian.jackson@eu.citrix.com>; Julien Grall <julien@xen.org>;
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com>; Roger Pau Monné <roger.pau@citrix.com>
> Subject: Re: [PATCH v4 5/7] mm: make MEMF_no_refcount pages safe to assign
> 
> On 28.01.2020 18:01, Durrant, Paul wrote:
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: 28 January 2020 15:23
> >>
> >> On 24.01.2020 16:31, Paul Durrant wrote:
> >>> Currently it is unsafe to assign a domheap page allocated with
> >>> MEMF_no_refcount to a domain because the domain't 'tot_pages' will not
> >>> be incremented, but will be decrement when the page is freed (since
> >>> free_domheap_pages() has no way of telling that the increment was
> >> skipped).
> >>>
> >>> This patch allocates a new 'count_info' bit for a PGC_no_refcount flag
> >>> which is then used to mark domheap pages allocated with
> >> MEMF_no_refcount.
> >>> This then allows free_domheap_pages() to skip decrementing tot_pages
> >> when
> >>> appropriate and hence makes the pages safe to assign.
> >>>
> >>> NOTE: The patch sets MEMF_no_refcount directly in
> alloc_domheap_pages()
> >>>       rather than in assign_pages() because the latter is called with
> >>>       MEMF_no_refcount by memory_exchange() as an optimization, to
> avoid
> >>>       too many calls to domain_adjust_tot_pages() (which acquires and
> >>>       releases the global 'heap_lock').
> >>
> >> I don't think there were any optimization thoughts with this. The
> >> MEMF_no_refcount use is because otherwise for a domain with
> >> tot_pages == max_pages the assignment would fail.
> >>
> >
> > That would not be the case if the calls to steal_page() further up
> didn't
> > pass MEMF_no_refcount (which would be the correct thing to do if not
> > passing it to assign_pages().
> 
> No, that's not an option either: steal_page() would otherwise decrement
> ->tot_pages, allowing the domain to allocate new memory on another vCPU.
> This would again result in the exchange failing for no reason. (And no,
> I don't think a guest should be required to serialize e.g. ballooning
> operations with exchanges.)
> 

Ok, yes it does make it non-atomic but my view would be that the guest should not be simultaneously ballooning; however, we clearly differ there.

> >>> --- a/xen/common/page_alloc.c
> >>> +++ b/xen/common/page_alloc.c
> >>> @@ -460,6 +460,9 @@ unsigned long domain_adjust_tot_pages(struct
> domain
> >> *d, long pages)
> >>>  {
> >>>      long dom_before, dom_after, dom_claimed, sys_before, sys_after;
> >>>
> >>> +    if ( !pages )
> >>> +        goto out;
> >>
> >> Unrelated change? Are there, in fact, any callers passing in 0?
> >> Oh, further down you add one which may do so, but then perhaps
> >> better to make the caller not call here (as is done e.g. in
> >> memory_exchange())?
> >
> > I think it's preferable for domain_adjust_tot_pages() to handle zero
> > gracefully.
> 
> That's an option, but imo would then better be a separate change (to
> also drop present guards of calls to the function).

Ok, I'll split it out into a separate patch.

  Paul

> 
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel