[Xen-devel] [PATCH v3 0/3] purge free_shared_domheap_page()

[Xen-devel] [PATCH v3 0/3] purge free_shared_domheap_page()

[Paul Durrant]

Paul Durrant (3):
  x86 / vmx: make apic_access_mfn type-safe
  x86 / hvm: add domain_relinquish_resources() method
  x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE

 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                  | 10 --------
 xen/common/domain.c                |  8 +++++++
 xen/include/asm-x86/hvm/hvm.h      |  1 +
 xen/include/asm-x86/hvm/vmx/vmcs.h |  2 +-
 xen/include/asm-x86/mm.h           |  2 --
 9 files changed, 43 insertions(+), 31 deletions(-)

-- 
2.20.1


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

[Xen-devel] [PATCH v3 1/3] 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 v3 2/3] 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>
---
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: 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 4723f5d09c..e51c077269 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -715,6 +715,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 v3 3/3] x86 / vmx: use a 'normal' 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 'normal' domheap page (i.e. by not passing MEMF_no_owner to
alloc_domheap_page()), 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.

There is, however, some fall-out from this simplification:

- alloc_domheap_page() will now call assign_pages() and run into the fact
  that 'max_pages' is not set until some time after domain_create(). To
  avoid an allocation failure, domain_create() is modified to set
  max_pages to an initial value, sufficient to cover any domheap
  allocations required to complete domain creation. The value will be
  set to the 'real' max_pages when the tool-stack later performs the
  XEN_DOMCTL_max_mem operation, thus allowing the rest of the domain's
  memory to be allocated.

- Because the domheap page is no longer a pseudo-xenheap page, the
  reference counting will prevent the domain from being destroyed. Thus
  the call to vmx_free_vlapic_mapping() is moved from the
  domain_destroy() method into the domain_relinquish_resources() method.
  Whilst in the area, make the domain_destroy() method an optional
  alternative_vcall() (since it will no longer peform any function in VMX
  and is stubbed in SVM anyway).

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@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Julien Grall <julien@xen.org>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>

v2:
 - Set an initial value for max_pages rather than avoiding the check in
   assign_pages()
 - Make domain_destroy() optional
---
 xen/arch/x86/hvm/hvm.c     |  4 +++-
 xen/arch/x86/hvm/svm/svm.c |  5 -----
 xen/arch/x86/hvm/vmx/vmx.c | 25 ++++++++++++++++++++-----
 xen/arch/x86/mm.c          | 10 ----------
 xen/common/domain.c        |  8 ++++++++
 xen/include/asm-x86/mm.h   |  2 --
 6 files changed, 31 insertions(+), 23 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index e51c077269..d2610f5f01 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -746,7 +746,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,
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index b262d38a7c..c3a06b54a8 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,
@@ -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, 0);
     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/common/domain.c b/xen/common/domain.c
index ee3f9ffd3e..30c777acb8 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -339,6 +339,8 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
     return arch_sanitise_domain_config(config);
 }
 
+#define DOMAIN_INIT_PAGES 1
+
 struct domain *domain_create(domid_t domid,
                              struct xen_domctl_createdomain *config,
                              bool is_priv)
@@ -441,6 +443,12 @@ struct domain *domain_create(domid_t domid,
         radix_tree_init(&d->pirq_tree);
     }
 
+    /*
+     * Allow a limited number of special pages to be allocated for the
+     * domain
+     */
+    d->max_pages = DOMAIN_INIT_PAGES;
+
     if ( (err = arch_domain_create(d, config)) != 0 )
         goto fail;
     init_status |= INIT_arch;
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index 2ca8882ad0..e429f38228 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -317,8 +317,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

Re: [Xen-devel] [PATCH v3 3/3] x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE

[George Dunlap]

On 1/23/20 2:03 PM, 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 'normal' domheap page (i.e. by not passing MEMF_no_owner to
> alloc_domheap_page()), 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.
> 
> There is, however, some fall-out from this simplification:
> 
> - alloc_domheap_page() will now call assign_pages() and run into the fact
>   that 'max_pages' is not set until some time after domain_create(). To
>   avoid an allocation failure, domain_create() is modified to set
>   max_pages to an initial value, sufficient to cover any domheap
>   allocations required to complete domain creation. The value will be
>   set to the 'real' max_pages when the tool-stack later performs the
>   XEN_DOMCTL_max_mem operation, thus allowing the rest of the domain's
>   memory to be allocated.
> 
> - Because the domheap page is no longer a pseudo-xenheap page, the
>   reference counting will prevent the domain from being destroyed. Thus
>   the call to vmx_free_vlapic_mapping() is moved from the
>   domain_destroy() method into the domain_relinquish_resources() method.
>   Whilst in the area, make the domain_destroy() method an optional
>   alternative_vcall() (since it will no longer peform any function in VMX
>   and is stubbed in SVM anyway).
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>

This is an excellent change, thank you:

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

My only comment is that this would have been a bit easier to review
broken down into probably three patches: 1) making domain_destroy
optional, 2) moving vmx teardown to a relinquish_resources call 3) using
"normal" pages".  But I don't think it's worth a re-send just for that.

 -George

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

Re: [Xen-devel] [PATCH v3 2/3] x86 / hvm: add domain_relinquish_resources() method

[Jan Beulich]

On 23.01.2020 15:03, Paul Durrant wrote:
> 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>

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

Re: [Xen-devel] [PATCH v3 3/3] x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE

[Julien Grall]

On 23/01/2020 14:03, Paul Durrant wrote:
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index ee3f9ffd3e..30c777acb8 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -339,6 +339,8 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
>       return arch_sanitise_domain_config(config);
>   }
>   
> +#define DOMAIN_INIT_PAGES 1

Would it make sense to make this a per-arch define? This would allow 
each arch to define a different number of init pages (and catch any misuse).

> +
>   struct domain *domain_create(domid_t domid,
>                                struct xen_domctl_createdomain *config,
>                                bool is_priv)
> @@ -441,6 +443,12 @@ struct domain *domain_create(domid_t domid,
>           radix_tree_init(&d->pirq_tree);
>       }
>   
> +    /*
> +     * Allow a limited number of special pages to be allocated for the
> +     * domain
> +     */
> +    d->max_pages = DOMAIN_INIT_PAGES;
> +
>       if ( (err = arch_domain_create(d, config)) != 0 )
>           goto fail;
>       init_status |= INIT_arch;
> diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
> index 2ca8882ad0..e429f38228 100644
> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -317,8 +317,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;
> 

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v3 1/3] x86 / vmx: make apic_access_mfn type-safe

[Andrew Cooper]

On 23/01/2020 14:03, Paul Durrant wrote:
> 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

Thanks.

For the create/destroy paths to function safely, it is necessary that:

struct domain d = {};

destroy_domain(&d);

works correctly, because it will be used to clean up from any arbitrary
early exit from create_domain() which managed to allocate a domain
struct in the first place.

This places a restriction that a bitpattern of 0 is the only detail
which can be relied upon, in the worst corner cases.

~Andrew

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

Re: [Xen-devel] [PATCH v3 3/3] x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE

[Jan Beulich]

On 23.01.2020 15:03, 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 'normal' domheap page (i.e. by not passing MEMF_no_owner to
> alloc_domheap_page()), 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.
> 
> There is, however, some fall-out from this simplification:
> 
> - alloc_domheap_page() will now call assign_pages() and run into the fact
>   that 'max_pages' is not set until some time after domain_create(). To
>   avoid an allocation failure, domain_create() is modified to set
>   max_pages to an initial value, sufficient to cover any domheap
>   allocations required to complete domain creation. The value will be
>   set to the 'real' max_pages when the tool-stack later performs the
>   XEN_DOMCTL_max_mem operation, thus allowing the rest of the domain's
>   memory to be allocated.

I continue to disagree with this approach, and I don't think I've
heard back on the alternative suggestion of using MEMF_no_refcount
instead of MEMF_no_owner. As said before, the page (and any other
ones up to DOMAIN_INIT_PAGES, which I find rather fragile to be
set to 1) will now get accounted against the amount allowed for
the domain to allocate.

It also looks to me as if this will break e.g.
p2m_pod_set_mem_target(), which at the very top has

    /* P == B: Nothing to do (unless the guest is being created). */
    populated = d->tot_pages - p2m->pod.count;
    if ( populated > 0 && p2m->pod.entry_count == 0 )
        goto out;

This code assumes that during domain creation all pages recorded
in ->tot_pages have also been recorded in ->pod.count.

There may be other uses of ->tot_pages which this change conflicts
with.

> - Because the domheap page is no longer a pseudo-xenheap page, the
>   reference counting will prevent the domain from being destroyed. Thus
>   the call to vmx_free_vlapic_mapping() is moved from the
>   domain_destroy() method into the domain_relinquish_resources() method.
>   Whilst in the area, make the domain_destroy() method an optional
>   alternative_vcall() (since it will no longer peform any function in VMX
>   and is stubbed in SVM anyway).

All of this would, afaict, become irrelevant when using MEMF_no_refcount.

There looks to be one issue (which I think we have been discussing
before): By not bumping ->tot_pages, its decrementing upon freeing
the page will be a problem. I can see two possible solutions to this:
- Introduce a flag indicating there should also be no accounting
  upon freeing of the page.
- Instead of avoiding to increment ->tot_pages, _also_ increment
  ->max_pages. The interaction with XEN_DOMCTL_max_mem will then of
  course be, well, interesting.

Jan

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

Re: [Xen-devel] [PATCH v3 3/3] x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE

[George Dunlap]

On 1/23/20 3:32 PM, Jan Beulich wrote:
> On 23.01.2020 15:03, 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 'normal' domheap page (i.e. by not passing MEMF_no_owner to
>> alloc_domheap_page()), 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.
>>
>> There is, however, some fall-out from this simplification:
>>
>> - alloc_domheap_page() will now call assign_pages() and run into the fact
>>   that 'max_pages' is not set until some time after domain_create(). To
>>   avoid an allocation failure, domain_create() is modified to set
>>   max_pages to an initial value, sufficient to cover any domheap
>>   allocations required to complete domain creation. The value will be
>>   set to the 'real' max_pages when the tool-stack later performs the
>>   XEN_DOMCTL_max_mem operation, thus allowing the rest of the domain's
>>   memory to be allocated.
> 
> I continue to disagree with this approach, and I don't think I've
> heard back on the alternative suggestion of using MEMF_no_refcount
> instead of MEMF_no_owner. As said before, the page (and any other
> ones up to DOMAIN_INIT_PAGES, which I find rather fragile to be
> set to 1) will now get accounted against the amount allowed for
> the domain to allocate.
> 
> It also looks to me as if this will break e.g.
> p2m_pod_set_mem_target(), which at the very top has
> 
>     /* P == B: Nothing to do (unless the guest is being created). */
>     populated = d->tot_pages - p2m->pod.count;
>     if ( populated > 0 && p2m->pod.entry_count == 0 )
>         goto out;
> 
> This code assumes that during domain creation all pages recorded
> in ->tot_pages have also been recorded in ->pod.count.
> 
> There may be other uses of ->tot_pages which this change conflicts
> with.

This basically boils down to the "memory accounting swamp", where
various random features need to allocate domain pages to function.

>> - Because the domheap page is no longer a pseudo-xenheap page, the
>>   reference counting will prevent the domain from being destroyed. Thus
>>   the call to vmx_free_vlapic_mapping() is moved from the
>>   domain_destroy() method into the domain_relinquish_resources() method.
>>   Whilst in the area, make the domain_destroy() method an optional
>>   alternative_vcall() (since it will no longer peform any function in VMX
>>   and is stubbed in SVM anyway).
> 
> All of this would, afaict, become irrelevant when using MEMF_no_refcount.
> 
> There looks to be one issue (which I think we have been discussing
> before): By not bumping ->tot_pages, its decrementing upon freeing
> the page will be a problem. I can see two possible solutions to this:
> - Introduce a flag indicating there should also be no accounting
>   upon freeing of the page.

This seems like a reasonable approach to look into.

 -George

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

Re: [Xen-devel] [PATCH v3 3/3] x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE

[Durrant, Paul]

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: 23 January 2020 15:26
> To: Durrant, Paul <pdurrant@amazon.co.uk>; xen-devel@lists.xenproject.org
> Cc: Jan Beulich <jbeulich@suse.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné
> <roger.pau@citrix.com>; George Dunlap <George.Dunlap@eu.citrix.com>; Ian
> Jackson <ian.jackson@eu.citrix.com>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>; Jun
> Nakajima <jun.nakajima@intel.com>; Kevin Tian <kevin.tian@intel.com>
> Subject: Re: [PATCH v3 3/3] x86 / vmx: use a 'normal' domheap page for
> APIC_DEFAULT_PHYS_BASE
> 
> 
> 
> On 23/01/2020 14:03, Paul Durrant wrote:
> > diff --git a/xen/common/domain.c b/xen/common/domain.c
> > index ee3f9ffd3e..30c777acb8 100644
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -339,6 +339,8 @@ static int sanitise_domain_config(struct
> xen_domctl_createdomain *config)
> >       return arch_sanitise_domain_config(config);
> >   }
> >
> > +#define DOMAIN_INIT_PAGES 1
> 
> Would it make sense to make this a per-arch define? This would allow
> each arch to define a different number of init pages (and catch any
> misuse).
>

I thought about that and decided against it. The arch code may want to increase (which may be a bad idea) but I think it should be set early. Ultimately it should come in from the toolstack via the domctl anyway.

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

Re: [Xen-devel] [PATCH v3 3/3] x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE

[George Dunlap]

On 1/23/20 3:46 PM, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: 23 January 2020 15:26
>> To: Durrant, Paul <pdurrant@amazon.co.uk>; xen-devel@lists.xenproject.org
>> Cc: Jan Beulich <jbeulich@suse.com>; Andrew Cooper
>> <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné
>> <roger.pau@citrix.com>; George Dunlap <George.Dunlap@eu.citrix.com>; Ian
>> Jackson <ian.jackson@eu.citrix.com>; Konrad Rzeszutek Wilk
>> <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>; Jun
>> Nakajima <jun.nakajima@intel.com>; Kevin Tian <kevin.tian@intel.com>
>> Subject: Re: [PATCH v3 3/3] x86 / vmx: use a 'normal' domheap page for
>> APIC_DEFAULT_PHYS_BASE
>>
>>
>>
>> On 23/01/2020 14:03, Paul Durrant wrote:
>>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>>> index ee3f9ffd3e..30c777acb8 100644
>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
>>> @@ -339,6 +339,8 @@ static int sanitise_domain_config(struct
>> xen_domctl_createdomain *config)
>>>       return arch_sanitise_domain_config(config);
>>>   }
>>>
>>> +#define DOMAIN_INIT_PAGES 1
>>
>> Would it make sense to make this a per-arch define? This would allow
>> each arch to define a different number of init pages (and catch any
>> misuse).
>>
> 
> I thought about that and decided against it. The arch code may want to increase (which may be a bad idea) but I think it should be set early. Ultimately it should come in from the toolstack via the domctl anyway.

I understood Julien to be saying that maybe Arm might want this to be
hard-coded to 5 (or 0) rather than 1.

 -George

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

Re: [Xen-devel] [PATCH v3 3/3] x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE

[Julien Grall]

On 23/01/2020 15:46, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: 23 January 2020 15:26
>> To: Durrant, Paul <pdurrant@amazon.co.uk>; xen-devel@lists.xenproject.org
>> Cc: Jan Beulich <jbeulich@suse.com>; Andrew Cooper
>> <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné
>> <roger.pau@citrix.com>; George Dunlap <George.Dunlap@eu.citrix.com>; Ian
>> Jackson <ian.jackson@eu.citrix.com>; Konrad Rzeszutek Wilk
>> <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>; Jun
>> Nakajima <jun.nakajima@intel.com>; Kevin Tian <kevin.tian@intel.com>
>> Subject: Re: [PATCH v3 3/3] x86 / vmx: use a 'normal' domheap page for
>> APIC_DEFAULT_PHYS_BASE
>>
>>
>>
>> On 23/01/2020 14:03, Paul Durrant wrote:
>>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>>> index ee3f9ffd3e..30c777acb8 100644
>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
>>> @@ -339,6 +339,8 @@ static int sanitise_domain_config(struct
>> xen_domctl_createdomain *config)
>>>        return arch_sanitise_domain_config(config);
>>>    }
>>>
>>> +#define DOMAIN_INIT_PAGES 1
>>
>> Would it make sense to make this a per-arch define? This would allow
>> each arch to define a different number of init pages (and catch any
>> misuse).
>>
> 
> I thought about that and decided against it. The arch code may want to increase (which may be a bad idea) but I think it should be set early. Ultimately it should come in from the toolstack via the domctl anyway.
The value is already arch specific because on Arm we have 0 pages 
requires this trick yet.

While I agree, it should be set early, there is nothing to prevent this 
to be in a arch header. Am I correct?

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v3 3/3] x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE

[Durrant, Paul]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 23 January 2020 15:32
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: xen-devel@lists.xenproject.org; Andrew Cooper
> <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné
> <roger.pau@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>; Jun Nakajima <jun.nakajima@intel.com>; Kevin
> Tian <kevin.tian@intel.com>
> Subject: Re: [PATCH v3 3/3] x86 / vmx: use a 'normal' domheap page for
> APIC_DEFAULT_PHYS_BASE
> 
> On 23.01.2020 15:03, 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 'normal' domheap page (i.e. by not passing MEMF_no_owner to
> > alloc_domheap_page()), 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.
> >
> > There is, however, some fall-out from this simplification:
> >
> > - alloc_domheap_page() will now call assign_pages() and run into the
> fact
> >   that 'max_pages' is not set until some time after domain_create(). To
> >   avoid an allocation failure, domain_create() is modified to set
> >   max_pages to an initial value, sufficient to cover any domheap
> >   allocations required to complete domain creation. The value will be
> >   set to the 'real' max_pages when the tool-stack later performs the
> >   XEN_DOMCTL_max_mem operation, thus allowing the rest of the domain's
> >   memory to be allocated.
> 
> I continue to disagree with this approach, and I don't think I've
> heard back on the alternative suggestion of using MEMF_no_refcount
> instead of MEMF_no_owner.

I responded in v1:

"
> Did you consider passing MEMF_no_refcount here, to avoid the
> fiddling with assign_pages()? That'll in particular also
> avoid ...
> 

You remember what happened last time we did that (with the ioreq server page), right? That's why assign_pages() vetoes non-refcounted pages.
"

> As said before, the page (and any other
> ones up to DOMAIN_INIT_PAGES, which I find rather fragile to be
> set to 1) will now get accounted against the amount allowed for
> the domain to allocate.
> 
> It also looks to me as if this will break e.g.
> p2m_pod_set_mem_target(), which at the very top has
> 
>     /* P == B: Nothing to do (unless the guest is being created). */
>     populated = d->tot_pages - p2m->pod.count;
>     if ( populated > 0 && p2m->pod.entry_count == 0 )
>         goto out;
> 
> This code assumes that during domain creation all pages recorded
> in ->tot_pages have also been recorded in ->pod.count.
> 
> There may be other uses of ->tot_pages which this change conflicts
> with.
> 
> > - Because the domheap page is no longer a pseudo-xenheap page, the
> >   reference counting will prevent the domain from being destroyed. Thus
> >   the call to vmx_free_vlapic_mapping() is moved from the
> >   domain_destroy() method into the domain_relinquish_resources() method.
> >   Whilst in the area, make the domain_destroy() method an optional
> >   alternative_vcall() (since it will no longer peform any function in
> VMX
> >   and is stubbed in SVM anyway).
> 
> All of this would, afaict, become irrelevant when using MEMF_no_refcount.
> 
> There looks to be one issue (which I think we have been discussing
> before): By not bumping ->tot_pages, its decrementing upon freeing
> the page will be a problem.

Yes, that's exactly the problem with assigning MEMF_no_refcount pages, which is way it is outlawed.

> I can see two possible solutions to this:
> - Introduce a flag indicating there should also be no accounting
>   upon freeing of the page.

What sort of flag did you have in mind? Do we have space anywhere in type-info or count-info to put it? If we can make assigning non-refcounted pages safe then it's certainly an attractive option.

> - Instead of avoiding to increment ->tot_pages, _also_ increment
>   ->max_pages. The interaction with XEN_DOMCTL_max_mem will then of
>   course be, well, interesting.
> 

Indeed, which is why I opted for the simple approach. As I've said in other responses, max_pages ought be set by the toolstack when the domain is created so I wanted to come up with something that's not too invasive until that change is made so if the pages do need to be ref-counted then I guess I'll have to figure out how to make the initial allocation co-exist with PoD.

  Paul 

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

Re: [Xen-devel] [PATCH v3 3/3] x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE

[Jan Beulich]

On 23.01.2020 16:56, Durrant, Paul wrote:
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 23 January 2020 15:32
>>
>> On 23.01.2020 15:03, 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 'normal' domheap page (i.e. by not passing MEMF_no_owner to
>>> alloc_domheap_page()), 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.
>>>
>>> There is, however, some fall-out from this simplification:
>>>
>>> - alloc_domheap_page() will now call assign_pages() and run into the
>> fact
>>>   that 'max_pages' is not set until some time after domain_create(). To
>>>   avoid an allocation failure, domain_create() is modified to set
>>>   max_pages to an initial value, sufficient to cover any domheap
>>>   allocations required to complete domain creation. The value will be
>>>   set to the 'real' max_pages when the tool-stack later performs the
>>>   XEN_DOMCTL_max_mem operation, thus allowing the rest of the domain's
>>>   memory to be allocated.
>>
>> I continue to disagree with this approach, and I don't think I've
>> heard back on the alternative suggestion of using MEMF_no_refcount
>> instead of MEMF_no_owner.
> 
> I responded in v1:
> 
> "
>> Did you consider passing MEMF_no_refcount here, to avoid the
>> fiddling with assign_pages()? That'll in particular also
>> avoid ...
>>
> 
> You remember what happened last time we did that (with the ioreq
> server page), right? That's why assign_pages() vetoes non-refcounted
> pages.
> "

Interesting - this mail appears to never have reached me. I'm
sorry for implying you didn't reply.

>> As said before, the page (and any other
>> ones up to DOMAIN_INIT_PAGES, which I find rather fragile to be
>> set to 1) will now get accounted against the amount allowed for
>> the domain to allocate.
>>
>> It also looks to me as if this will break e.g.
>> p2m_pod_set_mem_target(), which at the very top has
>>
>>     /* P == B: Nothing to do (unless the guest is being created). */
>>     populated = d->tot_pages - p2m->pod.count;
>>     if ( populated > 0 && p2m->pod.entry_count == 0 )
>>         goto out;
>>
>> This code assumes that during domain creation all pages recorded
>> in ->tot_pages have also been recorded in ->pod.count.
>>
>> There may be other uses of ->tot_pages which this change conflicts
>> with.
>>
>>> - Because the domheap page is no longer a pseudo-xenheap page, the
>>>   reference counting will prevent the domain from being destroyed. Thus
>>>   the call to vmx_free_vlapic_mapping() is moved from the
>>>   domain_destroy() method into the domain_relinquish_resources() method.
>>>   Whilst in the area, make the domain_destroy() method an optional
>>>   alternative_vcall() (since it will no longer peform any function in
>> VMX
>>>   and is stubbed in SVM anyway).
>>
>> All of this would, afaict, become irrelevant when using MEMF_no_refcount.
>>
>> There looks to be one issue (which I think we have been discussing
>> before): By not bumping ->tot_pages, its decrementing upon freeing
>> the page will be a problem.
> 
> Yes, that's exactly the problem with assigning MEMF_no_refcount
> pages, which is way it is outlawed.

Outlawed? It's being special treated, but nothing more/worse.

>> I can see two possible solutions to this:
>> - Introduce a flag indicating there should also be no accounting
>>   upon freeing of the page.
> 
> What sort of flag did you have in mind? Do we have space anywhere
> in type-info or count-info to put it? If we can make assigning
> non-refcounted pages safe then it's certainly an attractive option.

Stealing a bit from PGC_count_mask would likely be the way to go,
unless we can figure a PGC_* bit which can safely be overloaded.
Type-info wouldn't be the right place, I guess.

>> - Instead of avoiding to increment ->tot_pages, _also_ increment
>>   ->max_pages. The interaction with XEN_DOMCTL_max_mem will then of
>>   course be, well, interesting.
>>
> 
> Indeed, which is why I opted for the simple approach. As I've said in
> other responses, max_pages ought be set by the toolstack when the
> domain is created so I wanted to come up with something that's not
> too invasive until that change is made so if the pages do need to be
> ref-counted then I guess I'll have to figure out how to make the
> initial allocation co-exist with PoD.

I'd suggest you don't even try to. The interaction with PoD was just
the example I could easily think of because of having touched PoD
code recently. The general accounting issue that I've also explained
in my previous reply is enough reason to not want to go this route:
The amount of memory given / givable to a domain should not change
as a (silent) side effect of this patch.

And let's face it - the reason why the VMX code does what it does
is because there are no (obvious) easy alternatives.

Jan

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

Re: [Xen-devel] [PATCH v3 3/3] x86 / vmx: use a 'normal' domheap page for APIC_DEFAULT_PHYS_BASE

[Durrant, Paul]

> -----Original Message-----
> From: George Dunlap [mailto:george.dunlap@citrix.com]
> Sent: 23 January 2020 14:46
> To: Durrant, Paul <pdurrant@amazon.co.uk>; xen-devel@lists.xenproject.org
> Cc: Jan Beulich <jbeulich@suse.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné
> <roger.pau@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>; Jun Nakajima <jun.nakajima@intel.com>; Kevin
> Tian <kevin.tian@intel.com>
> Subject: Re: [PATCH v3 3/3] x86 / vmx: use a 'normal' domheap page for
> APIC_DEFAULT_PHYS_BASE
> 
> On 1/23/20 2:03 PM, 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 'normal' domheap page (i.e. by not passing MEMF_no_owner to
> > alloc_domheap_page()), 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.
> >
> > There is, however, some fall-out from this simplification:
> >
> > - alloc_domheap_page() will now call assign_pages() and run into the
> fact
> >   that 'max_pages' is not set until some time after domain_create(). To
> >   avoid an allocation failure, domain_create() is modified to set
> >   max_pages to an initial value, sufficient to cover any domheap
> >   allocations required to complete domain creation. The value will be
> >   set to the 'real' max_pages when the tool-stack later performs the
> >   XEN_DOMCTL_max_mem operation, thus allowing the rest of the domain's
> >   memory to be allocated.
> >
> > - Because the domheap page is no longer a pseudo-xenheap page, the
> >   reference counting will prevent the domain from being destroyed. Thus
> >   the call to vmx_free_vlapic_mapping() is moved from the
> >   domain_destroy() method into the domain_relinquish_resources() method.
> >   Whilst in the area, make the domain_destroy() method an optional
> >   alternative_vcall() (since it will no longer peform any function in
> VMX
> >   and is stubbed in SVM anyway).
> >
> > Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> 
> This is an excellent change, thank you:
> 
> Reviewed-by: George Dunlap <george.dunlap@citrix.com>
> 
> My only comment is that this would have been a bit easier to review broken
> down into probably three patches: 1) making domain_destroy optional, 2)
> moving vmx teardown to a relinquish_resources call 3) using "normal"
> pages".  But I don't think it's worth a re-send just for that.

Since I appear to need to do a v4 to re-work the allocation (assuming I can steal another PGC bit) I'll split things as you suggest.

  Paul

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