[Xen-devel] [PATCH v3 0/6] remove one more shared xenheap page: shared_info

[Xen-devel] [PATCH v3 0/6] remove one more shared xenheap page: shared_info

[pdurrant]

From: Paul Durrant <pdurrant@amzn.com>

Paul Durrant (6):
  domain: introduce alloc/free_shared_info() helpers...
  x86 / p2m: remove page_list check in p2m_alloc_table
  x86 / pv: do not treat PGC_extra pages as RAM when constructing dom0
  x86 / ioreq: use a MEMF_no_refcount allocation for server pages...
  mm: add 'is_special_page' macro...
  domain: use PGC_extra domheap page for shared_info

 xen/arch/arm/domain.c           | 10 +++----
 xen/arch/arm/mm.c               |  2 +-
 xen/arch/x86/domain.c           | 12 ++++-----
 xen/arch/x86/domctl.c           |  2 +-
 xen/arch/x86/hvm/ioreq.c        |  2 +-
 xen/arch/x86/mm.c               | 11 ++++----
 xen/arch/x86/mm/altp2m.c        |  2 +-
 xen/arch/x86/mm/mem_sharing.c   |  3 +--
 xen/arch/x86/mm/p2m.c           |  8 ------
 xen/arch/x86/mm/shadow/common.c | 13 ++++++----
 xen/arch/x86/pv/dom0_build.c    |  6 ++++-
 xen/arch/x86/pv/shim.c          |  2 +-
 xen/common/domain.c             | 46 +++++++++++++++++++++++++++++++++
 xen/common/domctl.c             |  2 +-
 xen/common/event_channel.c      |  3 +++
 xen/common/time.c               | 19 ++++++++++++--
 xen/include/asm-x86/shared.h    | 15 ++++++-----
 xen/include/xen/domain.h        |  3 +++
 xen/include/xen/mm.h            |  3 +++
 xen/include/xen/sched.h         |  5 +++-
 xen/include/xen/shared.h        |  2 +-
 21 files changed, 119 insertions(+), 52 deletions(-)
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@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: Paul Durrant <pdurrant@amazon.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tamas K Lengyel <tamas@tklengyel.com>
Cc: Tim Deegan <tim@xen.org>
Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Cc: Wei Liu <wl@xen.org>
-- 
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/6] domain: introduce alloc/free_shared_info() helpers...

[pdurrant]

From: Paul Durrant <pdurrant@amazon.com>

... and save the MFN.

This patch modifies the 'shared_info' field of struct domain to be
a structure comprising an MFN and a virtual address. Allocations are
still done from xenheap, so the virtual address still equates to
virt_to_mfn() called on the MFN but subsequent patch will change this.
Hence the need to save the MFN.

NOTE: Whist defining the new helpers, virt_to_mfn() in common/domain.c
      is made type safe.
      The definition of nmi_reason() in asm-x86/shared.h is also re-
      flowed to avoid overly long lines.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Reviewed-by: Julien Grall <julien@xen.org>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@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>
---
 xen/arch/arm/domain.c        |  8 ++------
 xen/arch/arm/mm.c            |  2 +-
 xen/arch/x86/domain.c        | 11 ++++-------
 xen/arch/x86/mm.c            |  2 +-
 xen/arch/x86/pv/dom0_build.c |  2 +-
 xen/arch/x86/pv/shim.c       |  2 +-
 xen/common/domain.c          | 26 ++++++++++++++++++++++++++
 xen/common/domctl.c          |  2 +-
 xen/common/time.c            |  4 ++--
 xen/include/asm-x86/shared.h | 15 ++++++++-------
 xen/include/xen/domain.h     |  3 +++
 xen/include/xen/sched.h      |  5 ++++-
 xen/include/xen/shared.h     |  2 +-
 13 files changed, 55 insertions(+), 29 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 6627be2922..5298d80bd2 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -689,13 +689,9 @@ int arch_domain_create(struct domain *d,
     if ( (rc = p2m_init(d)) != 0 )
         goto fail;
 
-    rc = -ENOMEM;
-    if ( (d->shared_info = alloc_xenheap_pages(0, 0)) == NULL )
+    if ( (rc = alloc_shared_info(d, 0)) != 0 )
         goto fail;
 
-    clear_page(d->shared_info);
-    share_xen_page_with_guest(virt_to_page(d->shared_info), d, SHARE_rw);
-
     switch ( config->arch.gic_version )
     {
     case XEN_DOMCTL_CONFIG_GIC_V2:
@@ -766,7 +762,7 @@ void arch_domain_destroy(struct domain *d)
     p2m_teardown(d);
     domain_vgic_free(d);
     domain_vuart_free(d);
-    free_xenheap_page(d->shared_info);
+    free_shared_info(d);
 #ifdef CONFIG_ACPI
     free_xenheap_pages(d->arch.efi_acpi_table,
                        get_order_from_bytes(d->arch.efi_acpi_len));
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 727107eefa..2bb592101d 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1424,7 +1424,7 @@ int xenmem_add_to_physmap_one(
         if ( idx != 0 )
             return -EINVAL;
 
-        mfn = virt_to_mfn(d->shared_info);
+        mfn = d->shared_info.mfn;
         t = p2m_ram_rw;
 
         break;
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index caf2ecad7e..bdcc0d972a 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -611,12 +611,9 @@ int arch_domain_create(struct domain *d,
      * The shared_info machine address must fit in a 32-bit field within a
      * 32-bit guest's start_info structure. Hence we specify MEMF_bits(32).
      */
-    if ( (d->shared_info = alloc_xenheap_pages(0, MEMF_bits(32))) == NULL )
+    if ( (rc = alloc_shared_info(d, MEMF_bits(32))) != 0 )
         goto fail;
 
-    clear_page(d->shared_info);
-    share_xen_page_with_guest(virt_to_page(d->shared_info), d, SHARE_rw);
-
     if ( (rc = init_domain_irq_mapping(d)) != 0 )
         goto fail;
 
@@ -664,7 +661,7 @@ int arch_domain_create(struct domain *d,
     psr_domain_free(d);
     iommu_domain_destroy(d);
     cleanup_domain_irq_mapping(d);
-    free_xenheap_page(d->shared_info);
+    free_shared_info(d);
     xfree(d->arch.cpuid);
     xfree(d->arch.msr);
     if ( paging_initialised )
@@ -693,7 +690,7 @@ void arch_domain_destroy(struct domain *d)
         pv_domain_destroy(d);
     free_perdomain_mappings(d);
 
-    free_xenheap_page(d->shared_info);
+    free_shared_info(d);
     cleanup_domain_irq_mapping(d);
 
     psr_domain_free(d);
@@ -719,7 +716,7 @@ void arch_domain_unpause(struct domain *d)
 
 int arch_domain_soft_reset(struct domain *d)
 {
-    struct page_info *page = virt_to_page(d->shared_info), *new_page;
+    struct page_info *page = mfn_to_page(d->shared_info.mfn), *new_page;
     int ret = 0;
     struct domain *owner;
     mfn_t mfn;
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 62507ca651..ba7563ed3c 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4540,7 +4540,7 @@ int xenmem_add_to_physmap_one(
     {
         case XENMAPSPACE_shared_info:
             if ( idx == 0 )
-                mfn = virt_to_mfn(d->shared_info);
+                mfn = d->shared_info.mfn;
             break;
         case XENMAPSPACE_grant_table:
             rc = gnttab_map_frame(d, idx, gpfn, &mfn);
diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index 5678da782d..dc16ef2e79 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -743,7 +743,7 @@ int __init dom0_construct_pv(struct domain *d,
     clear_page(si);
     si->nr_pages = nr_pages;
 
-    si->shared_info = virt_to_maddr(d->shared_info);
+    si->shared_info = mfn_to_maddr(d->shared_info.mfn);
 
     if ( !pv_shim )
         si->flags    = SIF_PRIVILEGED | SIF_INITDOMAIN;
diff --git a/xen/arch/x86/pv/shim.c b/xen/arch/x86/pv/shim.c
index d86e2de118..f512809dad 100644
--- a/xen/arch/x86/pv/shim.c
+++ b/xen/arch/x86/pv/shim.c
@@ -282,7 +282,7 @@ static void write_start_info(struct domain *d)
     snprintf(si->magic, sizeof(si->magic), "xen-3.0-x86_%s",
              is_pv_32bit_domain(d) ? "32p" : "64");
     si->nr_pages = domain_tot_pages(d);
-    si->shared_info = virt_to_maddr(d->shared_info);
+    si->shared_info = mfn_to_maddr(d->shared_info.mfn);
     si->flags = 0;
     BUG_ON(xen_hypercall_hvm_get_param(HVM_PARAM_STORE_PFN, &si->store_mfn));
     BUG_ON(xen_hypercall_hvm_get_param(HVM_PARAM_STORE_EVTCHN, &param));
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 6ad458fa6b..ba7a905258 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -47,6 +47,10 @@
 #include <asm/guest.h>
 #endif
 
+/* Override macros from asm/page.h to make them work with mfn_t */
+#undef virt_to_mfn
+#define virt_to_mfn(v) _mfn(__virt_to_mfn(v))
+
 /* Linux config option: propageted to domain0 */
 /* xen_processor_pmbits: xen control Cx, Px, ... */
 unsigned int xen_processor_pmbits = XEN_PROCESSOR_PM_PX;
@@ -1644,6 +1648,28 @@ int continue_hypercall_on_cpu(
     return 0;
 }
 
+int alloc_shared_info(struct domain *d, unsigned int memflags)
+{
+    if ( (d->shared_info.virt = alloc_xenheap_pages(0, memflags)) == NULL )
+        return -ENOMEM;
+
+    d->shared_info.mfn = virt_to_mfn(d->shared_info.virt);
+
+    clear_page(d->shared_info.virt);
+    share_xen_page_with_guest(mfn_to_page(d->shared_info.mfn), d, SHARE_rw);
+
+    return 0;
+}
+
+void free_shared_info(struct domain *d)
+{
+    if ( !d->shared_info.virt )
+        return;
+
+    free_xenheap_page(d->shared_info.virt);
+    d->shared_info.virt = NULL;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index a69b3b59a8..81f18e63a7 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -196,7 +196,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
     info->outstanding_pages = d->outstanding_pages;
     info->shr_pages         = atomic_read(&d->shr_pages);
     info->paged_pages       = atomic_read(&d->paged_pages);
-    info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info));
+    info->shared_info_frame = mfn_to_gmfn(d, mfn_x(d->shared_info.mfn));
     BUG_ON(SHARED_M2P(info->shared_info_frame));
 
     info->cpupool = cpupool_get_id(d);
diff --git a/xen/common/time.c b/xen/common/time.c
index 82336e2d5a..58fa9abc40 100644
--- a/xen/common/time.c
+++ b/xen/common/time.c
@@ -110,9 +110,9 @@ void update_domain_wallclock_time(struct domain *d)
     shared_info(d, wc_nsec)   = wc_nsec;
 #ifdef CONFIG_X86
     if ( likely(!has_32bit_shinfo(d)) )
-        d->shared_info->native.wc_sec_hi = sec >> 32;
+        d->shared_info.virt->native.wc_sec_hi = sec >> 32;
     else
-        d->shared_info->compat.arch.wc_sec_hi = sec >> 32;
+        d->shared_info.virt->compat.arch.wc_sec_hi = sec >> 32;
 #else
     shared_info(d, wc_sec_hi) = sec >> 32;
 #endif
diff --git a/xen/include/asm-x86/shared.h b/xen/include/asm-x86/shared.h
index af5d959d04..d4588e08a6 100644
--- a/xen/include/asm-x86/shared.h
+++ b/xen/include/asm-x86/shared.h
@@ -1,24 +1,25 @@
 #ifndef __XEN_X86_SHARED_H__
 #define __XEN_X86_SHARED_H__
 
-#define nmi_reason(d) (!has_32bit_shinfo(d) ?                             \
-                       (u32 *)&(d)->shared_info->native.arch.nmi_reason : \
-                       (u32 *)&(d)->shared_info->compat.arch.nmi_reason)
+#define nmi_reason(d)                                           \
+    (!has_32bit_shinfo(d) ?                                     \
+     (u32 *)&(d)->shared_info.virt->native.arch.nmi_reason :    \
+     (u32 *)&(d)->shared_info.virt->compat.arch.nmi_reason)
 
 #define GET_SET_SHARED(type, field)                             \
 static inline type arch_get_##field(const struct domain *d)     \
 {                                                               \
     return !has_32bit_shinfo(d) ?                               \
-           d->shared_info->native.arch.field :                  \
-           d->shared_info->compat.arch.field;                   \
+           d->shared_info.virt->native.arch.field :             \
+           d->shared_info.virt->compat.arch.field;              \
 }                                                               \
 static inline void arch_set_##field(struct domain *d,           \
                                     type val)                   \
 {                                                               \
     if ( !has_32bit_shinfo(d) )                                 \
-        d->shared_info->native.arch.field = val;                \
+        d->shared_info.virt->native.arch.field = val;           \
     else                                                        \
-        d->shared_info->compat.arch.field = val;                \
+        d->shared_info.virt->compat.arch.field = val;           \
 }
 
 #define GET_SET_VCPU(type, field)                               \
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index 7e51d361de..740e2032ad 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -130,4 +130,7 @@ struct vnuma_info {
 
 void vnuma_destroy(struct vnuma_info *vnuma);
 
+int alloc_shared_info(struct domain *d, unsigned int memflags);
+void free_shared_info(struct domain *d);
+
 #endif /* __XEN_DOMAIN_H__ */
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 3a4f43098c..f41d0ad2a0 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -346,7 +346,10 @@ struct domain
     unsigned int     max_vcpus;
     struct vcpu    **vcpu;
 
-    shared_info_t   *shared_info;     /* shared data area */
+    struct {
+        mfn_t mfn;
+        shared_info_t *virt;
+    } shared_info; /* shared data area */
 
     spinlock_t       domain_lock;
 
diff --git a/xen/include/xen/shared.h b/xen/include/xen/shared.h
index a411a8a3e3..57b2ff1e34 100644
--- a/xen/include/xen/shared.h
+++ b/xen/include/xen/shared.h
@@ -43,7 +43,7 @@ typedef struct vcpu_info vcpu_info_t;
 
 extern vcpu_info_t dummy_vcpu_info;
 
-#define shared_info(d, field)      __shared_info(d, (d)->shared_info, field)
+#define shared_info(d, field)      __shared_info(d, (d)->shared_info.virt, field)
 #define vcpu_info(v, field)        __vcpu_info(v, (v)->vcpu_info, field)
 
 #endif /* __XEN_SHARED_H__ */
-- 
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 1/6] domain: introduce alloc/free_shared_info() helpers...

[Xia, Hongyan]

On Thu, 2020-03-05 at 12:44 +0000, pdurrant@amzn.com wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> ... and save the MFN.
> 
> This patch modifies the 'shared_info' field of struct domain to be
> a structure comprising an MFN and a virtual address. Allocations are
> still done from xenheap, so the virtual address still equates to
> virt_to_mfn() called on the MFN but subsequent patch will change
> this.
> Hence the need to save the MFN.
> 
> NOTE: Whist defining the new helpers, virt_to_mfn() in
> common/domain.c
>       is made type safe.
>       The definition of nmi_reason() in asm-x86/shared.h is also re-
>       flowed to avoid overly long lines.
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> Reviewed-by: Julien Grall <julien@xen.org>
> ...
>  
> +int alloc_shared_info(struct domain *d, unsigned int memflags)
> +{
> +    if ( (d->shared_info.virt = alloc_xenheap_pages(0, memflags)) ==
> NULL )
> +        return -ENOMEM;
> +
> +    d->shared_info.mfn = virt_to_mfn(d->shared_info.virt);
> +
> +    clear_page(d->shared_info.virt);
> +    share_xen_page_with_guest(mfn_to_page(d->shared_info.mfn), d,
> SHARE_rw);
> +
> +    return 0;
> +}
> +
> +void free_shared_info(struct domain *d)
> +{
> +    if ( !d->shared_info.virt )
> +        return;
> +
> +    free_xenheap_page(d->shared_info.virt);
> +    d->shared_info.virt = NULL;
> +}
> +

I was going to say that free_xenheap_page() can deal with NULL so we do
not have to return on NULL. But then I found that the final patch needs
to unmap it which cannot deal with NULL anyway, so I no longer have a
strong opinion to do clean-ups here.

Other than that,
Reviewed-by: Hongyan Xia <hongyxia@amazon.org>

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

Re: [Xen-devel] [PATCH v3 1/6] domain: introduce alloc/free_shared_info() helpers...

[Xia, Hongyan]

On Thu, 2020-03-05 at 13:23 +0000, Hongyan Xia wrote:
> On Thu, 2020-03-05 at 12:44 +0000, pdurrant@amzn.com wrote:
> > ...
> 
> Other than that,
> Reviewed-by: Hongyan Xia <hongyxia@amazon.org>

Sorry, I meant hongyxia@amazon.com
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/6] domain: introduce alloc/free_shared_info() helpers...

[Jan Beulich]

On 05.03.2020 13:44, pdurrant@amzn.com wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> ... and save the MFN.
> 
> This patch modifies the 'shared_info' field of struct domain to be
> a structure comprising an MFN and a virtual address. Allocations are
> still done from xenheap, so the virtual address still equates to
> virt_to_mfn() called on the MFN but subsequent patch will change this.
> Hence the need to save the MFN.
> 
> NOTE: Whist defining the new helpers, virt_to_mfn() in common/domain.c
>       is made type safe.
>       The definition of nmi_reason() in asm-x86/shared.h is also re-
>       flowed to avoid overly long lines.
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> Reviewed-by: Julien Grall <julien@xen.org>

This patch by itself looks okay, but of course increases storage
requirements a little. Therefore
Acked-by: Jan Beulich <jbeulich@suse.com>
only if we reach agreement that the final patch in this series is
also to go in, which I'm yet to be convinced of.

Jan

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

[Xen-devel] [PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table

[pdurrant]

From: Paul Durrant <pdurrant@amazon.com>

There does not seem to be any justification for refusing to create the
domain's p2m table simply because it may have assigned pages. Particularly
it prevents the prior allocation of PGC_extra pages.

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

v2:
 - New in v2
---
 xen/arch/x86/mm/p2m.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 3719deae77..9fd4b115be 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -695,14 +695,6 @@ int p2m_alloc_table(struct p2m_domain *p2m)
 
     p2m_lock(p2m);
 
-    if ( p2m_is_hostp2m(p2m)
-         && !page_list_empty(&d->page_list) )
-    {
-        P2M_ERROR("dom %d already has memory allocated\n", d->domain_id);
-        p2m_unlock(p2m);
-        return -EINVAL;
-    }
-
     if ( pagetable_get_pfn(p2m_get_pagetable(p2m)) != 0 )
     {
         P2M_ERROR("p2m already allocated for this domain\n");
-- 
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 2/6] x86 / p2m: remove page_list check in p2m_alloc_table

[Jan Beulich]

On 05.03.2020 13:45, pdurrant@amzn.com wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> There does not seem to be any justification for refusing to create the
> domain's p2m table simply because it may have assigned pages.

I think there is: If any such allocation had happened before, how
would it be represented in the domain's p2m?

> Particularly
> it prevents the prior allocation of PGC_extra pages.

That's unfortunate, but will need taking care of differently then:

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -695,14 +695,6 @@ int p2m_alloc_table(struct p2m_domain *p2m)
>  
>      p2m_lock(p2m);
>  
> -    if ( p2m_is_hostp2m(p2m)
> -         && !page_list_empty(&d->page_list) )
> -    {
> -        P2M_ERROR("dom %d already has memory allocated\n", d->domain_id);
> -        p2m_unlock(p2m);
> -        return -EINVAL;
> -    }

Instead of checking the list to be empty, how about checking
domain_tot_pages() to return zero?

Jan

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

Re: [Xen-devel] [PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table

[Durrant, Paul]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 06 March 2020 11:46
> To: pdurrant@amzn.com
> Cc: xen-devel@lists.xenproject.org; Durrant, Paul <pdurrant@amazon.co.uk>; Andrew Cooper
> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau
> Monné <roger.pau@citrix.com>
> Subject: Re: [PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table
> 
> On 05.03.2020 13:45, pdurrant@amzn.com wrote:
> > From: Paul Durrant <pdurrant@amazon.com>
> >
> > There does not seem to be any justification for refusing to create the
> > domain's p2m table simply because it may have assigned pages.
> 
> I think there is: If any such allocation had happened before, how
> would it be represented in the domain's p2m?

Insertion into the p2m is a separate action from page allocation. Why should they be linked?

> 
> > Particularly
> > it prevents the prior allocation of PGC_extra pages.
> 
> That's unfortunate, but will need taking care of differently then:
> 
> > --- a/xen/arch/x86/mm/p2m.c
> > +++ b/xen/arch/x86/mm/p2m.c
> > @@ -695,14 +695,6 @@ int p2m_alloc_table(struct p2m_domain *p2m)
> >
> >      p2m_lock(p2m);
> >
> > -    if ( p2m_is_hostp2m(p2m)
> > -         && !page_list_empty(&d->page_list) )
> > -    {
> > -        P2M_ERROR("dom %d already has memory allocated\n", d->domain_id);
> > -        p2m_unlock(p2m);
> > -        return -EINVAL;
> > -    }
> 
> Instead of checking the list to be empty, how about checking
> domain_tot_pages() to return zero?

I could do that, and in fact my original code did, but with more consideration the whole test just didn't make sense to me. Yes, clearly the p2m has to be there before pages can be added into it, but I can't see any reason why you couldn't even allocate the entire guest RAM, then create the p2m and then add the pages into it.

  Paul

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

Re: [Xen-devel] [PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table

[Jan Beulich]

On 06.03.2020 13:07, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 06 March 2020 11:46
>> To: pdurrant@amzn.com
>> Cc: xen-devel@lists.xenproject.org; Durrant, Paul <pdurrant@amazon.co.uk>; Andrew Cooper
>> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau
>> Monné <roger.pau@citrix.com>
>> Subject: Re: [PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table
>>
>> On 05.03.2020 13:45, pdurrant@amzn.com wrote:
>>> From: Paul Durrant <pdurrant@amazon.com>
>>>
>>> There does not seem to be any justification for refusing to create the
>>> domain's p2m table simply because it may have assigned pages.
>>
>> I think there is: If any such allocation had happened before, how
>> would it be represented in the domain's p2m?
> 
> Insertion into the p2m is a separate action from page allocation. Why should they be linked?

They are, because of how XENMEM_populate_physmap works. Yes,
they _could_ be separate steps, but that's only a theoretical
consideration.

>>> Particularly
>>> it prevents the prior allocation of PGC_extra pages.
>>
>> That's unfortunate, but will need taking care of differently then:
>>
>>> --- a/xen/arch/x86/mm/p2m.c
>>> +++ b/xen/arch/x86/mm/p2m.c
>>> @@ -695,14 +695,6 @@ int p2m_alloc_table(struct p2m_domain *p2m)
>>>
>>>      p2m_lock(p2m);
>>>
>>> -    if ( p2m_is_hostp2m(p2m)
>>> -         && !page_list_empty(&d->page_list) )
>>> -    {
>>> -        P2M_ERROR("dom %d already has memory allocated\n", d->domain_id);
>>> -        p2m_unlock(p2m);
>>> -        return -EINVAL;
>>> -    }
>>
>> Instead of checking the list to be empty, how about checking
>> domain_tot_pages() to return zero?
> 
> I could do that, and in fact my original code did, but with more
> consideration the whole test just didn't make sense to me. Yes,
> clearly the p2m has to be there before pages can be added into it,
> but I can't see any reason why you couldn't even allocate the
> entire guest RAM, then create the p2m and then add the pages into
> it.

Right - more hypercalls (XENMEM_increase_reservation + operations
like XENMAPSPACE_gmfn), and hence slower overall domain creation.
Plus - XENMEM_increase_reservation is not very useful for
translated domains, as it won't return the MFNs allocated, and
there's no way to specify where they should appear in GFN space.
Hence in practice I don't see how this whole operation could
work without XENMEM_populate_physmap.

Jan

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

Re: [Xen-devel] [PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table

[Durrant, Paul]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 06 March 2020 12:47
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: pdurrant@amzn.com; xen-devel@lists.xenproject.org; Andrew Cooper <andrew.cooper3@citrix.com>;
> George Dunlap <george.dunlap@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné <roger.pau@citrix.com>
> Subject: Re: [PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table
> 
> On 06.03.2020 13:07, Durrant, Paul wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: 06 March 2020 11:46
> >> To: pdurrant@amzn.com
> >> Cc: xen-devel@lists.xenproject.org; Durrant, Paul <pdurrant@amazon.co.uk>; Andrew Cooper
> >> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>; Wei Liu <wl@xen.org>; Roger
> Pau
> >> Monné <roger.pau@citrix.com>
> >> Subject: Re: [PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table
> >>
> >> On 05.03.2020 13:45, pdurrant@amzn.com wrote:
> >>> From: Paul Durrant <pdurrant@amazon.com>
> >>>
> >>> There does not seem to be any justification for refusing to create the
> >>> domain's p2m table simply because it may have assigned pages.
> >>
> >> I think there is: If any such allocation had happened before, how
> >> would it be represented in the domain's p2m?
> >
> > Insertion into the p2m is a separate action from page allocation. Why should they be linked?
> 
> They are, because of how XENMEM_populate_physmap works. Yes,
> they _could_ be separate steps, but that's only a theoretical
> consideration.

Then surely the check should be in the XENMEM_populate_physmap code?

> 
> >>> Particularly
> >>> it prevents the prior allocation of PGC_extra pages.
> >>
> >> That's unfortunate, but will need taking care of differently then:
> >>
> >>> --- a/xen/arch/x86/mm/p2m.c
> >>> +++ b/xen/arch/x86/mm/p2m.c
> >>> @@ -695,14 +695,6 @@ int p2m_alloc_table(struct p2m_domain *p2m)
> >>>
> >>>      p2m_lock(p2m);
> >>>
> >>> -    if ( p2m_is_hostp2m(p2m)
> >>> -         && !page_list_empty(&d->page_list) )
> >>> -    {
> >>> -        P2M_ERROR("dom %d already has memory allocated\n", d->domain_id);
> >>> -        p2m_unlock(p2m);
> >>> -        return -EINVAL;
> >>> -    }
> >>
> >> Instead of checking the list to be empty, how about checking
> >> domain_tot_pages() to return zero?
> >
> > I could do that, and in fact my original code did, but with more
> > consideration the whole test just didn't make sense to me. Yes,
> > clearly the p2m has to be there before pages can be added into it,
> > but I can't see any reason why you couldn't even allocate the
> > entire guest RAM, then create the p2m and then add the pages into
> > it.
> 
> Right - more hypercalls (XENMEM_increase_reservation + operations
> like XENMAPSPACE_gmfn), and hence slower overall domain creation.
> Plus - XENMEM_increase_reservation is not very useful for
> translated domains, as it won't return the MFNs allocated, and
> there's no way to specify where they should appear in GFN space.
> Hence in practice I don't see how this whole operation could
> work without XENMEM_populate_physmap.
> 

Oh, it would mean a big change in the tools etc. so I'm not saying it's a good idea or even possible at the moment. I was just pointing out that, as far as the lower layers of code in Xen go, page allocation and p2m insertion are distinct actions.

  Paul

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

Re: [Xen-devel] [PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table

[Jan Beulich]

On 06.03.2020 13:50, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 06 March 2020 12:47
>> To: Durrant, Paul <pdurrant@amazon.co.uk>
>> Cc: pdurrant@amzn.com; xen-devel@lists.xenproject.org; Andrew Cooper <andrew.cooper3@citrix.com>;
>> George Dunlap <george.dunlap@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné <roger.pau@citrix.com>
>> Subject: Re: [PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table
>>
>> On 06.03.2020 13:07, Durrant, Paul wrote:
>>>> -----Original Message-----
>>>> From: Jan Beulich <jbeulich@suse.com>
>>>> Sent: 06 March 2020 11:46
>>>> To: pdurrant@amzn.com
>>>> Cc: xen-devel@lists.xenproject.org; Durrant, Paul <pdurrant@amazon.co.uk>; Andrew Cooper
>>>> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>; Wei Liu <wl@xen.org>; Roger
>> Pau
>>>> Monné <roger.pau@citrix.com>
>>>> Subject: Re: [PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table
>>>>
>>>> On 05.03.2020 13:45, pdurrant@amzn.com wrote:
>>>>> From: Paul Durrant <pdurrant@amazon.com>
>>>>>
>>>>> There does not seem to be any justification for refusing to create the
>>>>> domain's p2m table simply because it may have assigned pages.
>>>>
>>>> I think there is: If any such allocation had happened before, how
>>>> would it be represented in the domain's p2m?
>>>
>>> Insertion into the p2m is a separate action from page allocation. Why should they be linked?
>>
>> They are, because of how XENMEM_populate_physmap works. Yes,
>> they _could_ be separate steps, but that's only a theoretical
>> consideration.
> 
> Then surely the check should be in the XENMEM_populate_physmap code?

How that? populate-physmap can be called any number of times. We
can't refuse a 2nd call there just because a 1st one had happened
already. Or did you mean the inverse check (i.e. that there
already is a p2m)? This surely wouldn't be a bad idea, as
otherwise both ept_get_entry() and p2m_pt_get_entry() would
blindly map MFN 0. But adding such a check wouldn't eliminate
the reason to also have the check that you're proposing to drop.

Jan

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

Re: [Xen-devel] [EXTERNAL][PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 06 March 2020 13:07
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: pdurrant@amzn.com; xen-devel@lists.xenproject.org; Andrew Cooper <andrew.cooper3@citrix.com>;
> George Dunlap <george.dunlap@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné <roger.pau@citrix.com>
> Subject: RE: [EXTERNAL][PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table
> 
> CAUTION: This email originated from outside of the organization. Do not click links or open
> attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On 06.03.2020 13:50, Durrant, Paul wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: 06 March 2020 12:47
> >> To: Durrant, Paul <pdurrant@amazon.co.uk>
> >> Cc: pdurrant@amzn.com; xen-devel@lists.xenproject.org; Andrew Cooper <andrew.cooper3@citrix.com>;
> >> George Dunlap <george.dunlap@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné
> <roger.pau@citrix.com>
> >> Subject: Re: [PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table
> >>
> >> On 06.03.2020 13:07, Durrant, Paul wrote:
> >>>> -----Original Message-----
> >>>> From: Jan Beulich <jbeulich@suse.com>
> >>>> Sent: 06 March 2020 11:46
> >>>> To: pdurrant@amzn.com
> >>>> Cc: xen-devel@lists.xenproject.org; Durrant, Paul <pdurrant@amazon.co.uk>; Andrew Cooper
> >>>> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>; Wei Liu <wl@xen.org>;
> Roger
> >> Pau
> >>>> Monné <roger.pau@citrix.com>
> >>>> Subject: Re: [PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table
> >>>>
> >>>> On 05.03.2020 13:45, pdurrant@amzn.com wrote:
> >>>>> From: Paul Durrant <pdurrant@amazon.com>
> >>>>>
> >>>>> There does not seem to be any justification for refusing to create the
> >>>>> domain's p2m table simply because it may have assigned pages.
> >>>>
> >>>> I think there is: If any such allocation had happened before, how
> >>>> would it be represented in the domain's p2m?
> >>>
> >>> Insertion into the p2m is a separate action from page allocation. Why should they be linked?
> >>
> >> They are, because of how XENMEM_populate_physmap works. Yes,
> >> they _could_ be separate steps, but that's only a theoretical
> >> consideration.
> >
> > Then surely the check should be in the XENMEM_populate_physmap code?
> 
> How that? populate-physmap can be called any number of times. We
> can't refuse a 2nd call there just because a 1st one had happened
> already. Or did you mean the inverse check (i.e. that there
> already is a p2m)?

Yes, I mean check the p2m has been initialized there.

> This surely wouldn't be a bad idea, as
> otherwise both ept_get_entry() and p2m_pt_get_entry() would
> blindly map MFN 0. But adding such a check wouldn't eliminate
> the reason to also have the check that you're proposing to drop.
> 

Why not? Anywhere assuming the existence of a p2m ought to check for it; I still can't see why initialising the p2m after having allocated pages (PGC_extra or otherwise) is inherently wrong.

  Paul

> Jan


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

Re: [Xen-devel] [EXTERNAL][PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table

[Jan Beulich]

On 06.03.2020 14:11, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 06 March 2020 13:07
>> To: Durrant, Paul <pdurrant@amazon.co.uk>
>> Cc: pdurrant@amzn.com; xen-devel@lists.xenproject.org; Andrew Cooper <andrew.cooper3@citrix.com>;
>> George Dunlap <george.dunlap@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné <roger.pau@citrix.com>
>> Subject: RE: [EXTERNAL][PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table
>>
>> CAUTION: This email originated from outside of the organization. Do not click links or open
>> attachments unless you can confirm the sender and know the content is safe.
>>
>>
>>
>> On 06.03.2020 13:50, Durrant, Paul wrote:
>>>> -----Original Message-----
>>>> From: Jan Beulich <jbeulich@suse.com>
>>>> Sent: 06 March 2020 12:47
>>>> To: Durrant, Paul <pdurrant@amazon.co.uk>
>>>> Cc: pdurrant@amzn.com; xen-devel@lists.xenproject.org; Andrew Cooper <andrew.cooper3@citrix.com>;
>>>> George Dunlap <george.dunlap@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné
>> <roger.pau@citrix.com>
>>>> Subject: Re: [PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table
>>>>
>>>> On 06.03.2020 13:07, Durrant, Paul wrote:
>>>>>> -----Original Message-----
>>>>>> From: Jan Beulich <jbeulich@suse.com>
>>>>>> Sent: 06 March 2020 11:46
>>>>>> To: pdurrant@amzn.com
>>>>>> Cc: xen-devel@lists.xenproject.org; Durrant, Paul <pdurrant@amazon.co.uk>; Andrew Cooper
>>>>>> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>; Wei Liu <wl@xen.org>;
>> Roger
>>>> Pau
>>>>>> Monné <roger.pau@citrix.com>
>>>>>> Subject: Re: [PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table
>>>>>>
>>>>>> On 05.03.2020 13:45, pdurrant@amzn.com wrote:
>>>>>>> From: Paul Durrant <pdurrant@amazon.com>
>>>>>>>
>>>>>>> There does not seem to be any justification for refusing to create the
>>>>>>> domain's p2m table simply because it may have assigned pages.
>>>>>>
>>>>>> I think there is: If any such allocation had happened before, how
>>>>>> would it be represented in the domain's p2m?
>>>>>
>>>>> Insertion into the p2m is a separate action from page allocation. Why should they be linked?
>>>>
>>>> They are, because of how XENMEM_populate_physmap works. Yes,
>>>> they _could_ be separate steps, but that's only a theoretical
>>>> consideration.
>>>
>>> Then surely the check should be in the XENMEM_populate_physmap code?
>>
>> How that? populate-physmap can be called any number of times. We
>> can't refuse a 2nd call there just because a 1st one had happened
>> already. Or did you mean the inverse check (i.e. that there
>> already is a p2m)?
> 
> Yes, I mean check the p2m has been initialized there.
> 
>> This surely wouldn't be a bad idea, as
>> otherwise both ept_get_entry() and p2m_pt_get_entry() would
>> blindly map MFN 0. But adding such a check wouldn't eliminate
>> the reason to also have the check that you're proposing to drop.
>>
> 
> Why not? Anywhere assuming the existence of a p2m ought to check
> for it;

As said - I agree this wouldn't be a bad thing to do. It would
be a requirement if paging_enable() wasn't called from
hvm_domain_initialise(), but via a distinct domctl. But since
it is, there's no way to invoke populate-physmap on a domain
without its p2m root table already allocated.

> I still can't see why initialising the p2m after having allocated
> pages (PGC_extra or otherwise) is inherently wrong.

"inherently" as in "from an abstract pov" - yes. But within the
constraints of the hypercalls available - no. Yet what gets
checked has to be of practical use, not just of theoretical one.
I.e. I'd be fine to see the check go away when a viable
alternative mechanism to allocate and _then_ populate p2m gets
introduced.

Jan

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

Re: [Xen-devel] [EXTERNAL][PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 06 March 2020 13:19
> To: Paul Durrant <xadimgnik@gmail.com>
> Cc: pdurrant@amzn.com; xen-devel@lists.xenproject.org; 'Andrew Cooper' <andrew.cooper3@citrix.com>;
> 'George Dunlap' <george.dunlap@citrix.com>; 'Wei Liu' <wl@xen.org>; 'Roger Pau Monné'
> <roger.pau@citrix.com>
> Subject: Re: [EXTERNAL][PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table
> 
> On 06.03.2020 14:11, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: 06 March 2020 13:07
> >> To: Durrant, Paul <pdurrant@amazon.co.uk>
> >> Cc: pdurrant@amzn.com; xen-devel@lists.xenproject.org; Andrew Cooper <andrew.cooper3@citrix.com>;
> >> George Dunlap <george.dunlap@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné
> <roger.pau@citrix.com>
> >> Subject: RE: [EXTERNAL][PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table
> >>
> >> CAUTION: This email originated from outside of the organization. Do not click links or open
> >> attachments unless you can confirm the sender and know the content is safe.
> >>
> >>
> >>
> >> On 06.03.2020 13:50, Durrant, Paul wrote:
> >>>> -----Original Message-----
> >>>> From: Jan Beulich <jbeulich@suse.com>
> >>>> Sent: 06 March 2020 12:47
> >>>> To: Durrant, Paul <pdurrant@amazon.co.uk>
> >>>> Cc: pdurrant@amzn.com; xen-devel@lists.xenproject.org; Andrew Cooper <andrew.cooper3@citrix.com>;
> >>>> George Dunlap <george.dunlap@citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné
> >> <roger.pau@citrix.com>
> >>>> Subject: Re: [PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table
> >>>>
> >>>> On 06.03.2020 13:07, Durrant, Paul wrote:
> >>>>>> -----Original Message-----
> >>>>>> From: Jan Beulich <jbeulich@suse.com>
> >>>>>> Sent: 06 March 2020 11:46
> >>>>>> To: pdurrant@amzn.com
> >>>>>> Cc: xen-devel@lists.xenproject.org; Durrant, Paul <pdurrant@amazon.co.uk>; Andrew Cooper
> >>>>>> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>; Wei Liu <wl@xen.org>;
> >> Roger
> >>>> Pau
> >>>>>> Monné <roger.pau@citrix.com>
> >>>>>> Subject: Re: [PATCH v3 2/6] x86 / p2m: remove page_list check in p2m_alloc_table
> >>>>>>
> >>>>>> On 05.03.2020 13:45, pdurrant@amzn.com wrote:
> >>>>>>> From: Paul Durrant <pdurrant@amazon.com>
> >>>>>>>
> >>>>>>> There does not seem to be any justification for refusing to create the
> >>>>>>> domain's p2m table simply because it may have assigned pages.
> >>>>>>
> >>>>>> I think there is: If any such allocation had happened before, how
> >>>>>> would it be represented in the domain's p2m?
> >>>>>
> >>>>> Insertion into the p2m is a separate action from page allocation. Why should they be linked?
> >>>>
> >>>> They are, because of how XENMEM_populate_physmap works. Yes,
> >>>> they _could_ be separate steps, but that's only a theoretical
> >>>> consideration.
> >>>
> >>> Then surely the check should be in the XENMEM_populate_physmap code?
> >>
> >> How that? populate-physmap can be called any number of times. We
> >> can't refuse a 2nd call there just because a 1st one had happened
> >> already. Or did you mean the inverse check (i.e. that there
> >> already is a p2m)?
> >
> > Yes, I mean check the p2m has been initialized there.
> >
> >> This surely wouldn't be a bad idea, as
> >> otherwise both ept_get_entry() and p2m_pt_get_entry() would
> >> blindly map MFN 0. But adding such a check wouldn't eliminate
> >> the reason to also have the check that you're proposing to drop.
> >>
> >
> > Why not? Anywhere assuming the existence of a p2m ought to check
> > for it;
> 
> As said - I agree this wouldn't be a bad thing to do. It would
> be a requirement if paging_enable() wasn't called from
> hvm_domain_initialise(), but via a distinct domctl. But since
> it is, there's no way to invoke populate-physmap on a domain
> without its p2m root table already allocated.
> 
> > I still can't see why initialising the p2m after having allocated
> > pages (PGC_extra or otherwise) is inherently wrong.
> 
> "inherently" as in "from an abstract pov" - yes. But within the
> constraints of the hypercalls available - no. Yet what gets
> checked has to be of practical use, not just of theoretical one.
> I.e. I'd be fine to see the check go away when a viable
> alternative mechanism to allocate and _then_ populate p2m gets
> introduced.
> 

OK... it still seems like the wrong place to me, but I'll leave the check and simply exclude PGG_extra pages.

  Paul


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

[Xen-devel] [PATCH v3 3/6] x86 / pv: do not treat PGC_extra pages as RAM when constructing dom0

[pdurrant]

From: Paul Durrant <pdurrant@amazon.com>

The walk of page_list in dom0_construct_pv() should ignore PGC_extra pages
as they are only created for special purposes and, if mapped, will be mapped
explicitly for whatever purpose is relevant.

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>

v2:
 - New in v2
---
 xen/arch/x86/pv/dom0_build.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index dc16ef2e79..f8f1bbe2f4 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -792,6 +792,10 @@ int __init dom0_construct_pv(struct domain *d,
     {
         mfn = mfn_x(page_to_mfn(page));
         BUG_ON(SHARED_M2P(get_gpfn_from_mfn(mfn)));
+
+        if ( page->count_info & PGC_extra )
+            continue;
+
         if ( get_gpfn_from_mfn(mfn) >= count )
         {
             BUG_ON(is_pv_32bit_domain(d));
-- 
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/6] x86 / pv: do not treat PGC_extra pages as RAM when constructing dom0

[Jan Beulich]

On 05.03.2020 13:45, pdurrant@amzn.com wrote:
> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -792,6 +792,10 @@ int __init dom0_construct_pv(struct domain *d,
>      {
>          mfn = mfn_x(page_to_mfn(page));
>          BUG_ON(SHARED_M2P(get_gpfn_from_mfn(mfn)));
> +
> +        if ( page->count_info & PGC_extra )
> +            continue;

This surely is a pattern, i.e. there are more similar changes to
make: tboot_gen_domain_integrity() e.g. ignores d->xenpage_list,
and hence with the goal of converting the shared info page would
also want adjustment. For dump_numa() it may be less important,
but it would still look more correct if it too got changed.
audit_p2m() might apparently complain about such pages (and
hence might be a problem with the one PGC_extra page VMX domains
now have). And this is only from me looking at
page_list_for_each(..., &d->page_list) constructs; who knows
what else there is.

Jan

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

Re: [Xen-devel] [PATCH v3 3/6] x86 / pv: do not treat PGC_extra pages as RAM when constructing dom0

[Durrant, Paul]

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich
> Sent: 06 March 2020 11:56
> To: pdurrant@amzn.com
> Cc: xen-devel@lists.xenproject.org; Durrant, Paul <pdurrant@amazon.co.uk>; Roger Pau Monné
> <roger.pau@citrix.com>; Wei Liu <wl@xen.org>; Andrew Cooper <andrew.cooper3@citrix.com>
> Subject: Re: [Xen-devel] [PATCH v3 3/6] x86 / pv: do not treat PGC_extra pages as RAM when
> constructing dom0
> 
> On 05.03.2020 13:45, pdurrant@amzn.com wrote:
> > --- a/xen/arch/x86/pv/dom0_build.c
> > +++ b/xen/arch/x86/pv/dom0_build.c
> > @@ -792,6 +792,10 @@ int __init dom0_construct_pv(struct domain *d,
> >      {
> >          mfn = mfn_x(page_to_mfn(page));
> >          BUG_ON(SHARED_M2P(get_gpfn_from_mfn(mfn)));
> > +
> > +        if ( page->count_info & PGC_extra )
> > +            continue;
> 
> This surely is a pattern, i.e. there are more similar changes to
> make: tboot_gen_domain_integrity() e.g. ignores d->xenpage_list,
> and hence with the goal of converting the shared info page would
> also want adjustment. For dump_numa() it may be less important,
> but it would still look more correct if it too got changed.
> audit_p2m() might apparently complain about such pages (and
> hence might be a problem with the one PGC_extra page VMX domains
> now have). And this is only from me looking at
> page_list_for_each(..., &d->page_list) constructs; who knows
> what else there is.
> 

Those are dealt with by the is_special_page() patch later on I think. It didn't seem appropriate to use that macro here though since we know pages on the page list cannot be xenheap pages.

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

Re: [Xen-devel] [PATCH v3 3/6] x86 / pv: do not treat PGC_extra pages as RAM when constructing dom0

[Jan Beulich]

On 06.03.2020 13:03, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich
>> Sent: 06 March 2020 11:56
>> To: pdurrant@amzn.com
>> Cc: xen-devel@lists.xenproject.org; Durrant, Paul <pdurrant@amazon.co.uk>; Roger Pau Monné
>> <roger.pau@citrix.com>; Wei Liu <wl@xen.org>; Andrew Cooper <andrew.cooper3@citrix.com>
>> Subject: Re: [Xen-devel] [PATCH v3 3/6] x86 / pv: do not treat PGC_extra pages as RAM when
>> constructing dom0
>>
>> On 05.03.2020 13:45, pdurrant@amzn.com wrote:
>>> --- a/xen/arch/x86/pv/dom0_build.c
>>> +++ b/xen/arch/x86/pv/dom0_build.c
>>> @@ -792,6 +792,10 @@ int __init dom0_construct_pv(struct domain *d,
>>>      {
>>>          mfn = mfn_x(page_to_mfn(page));
>>>          BUG_ON(SHARED_M2P(get_gpfn_from_mfn(mfn)));
>>> +
>>> +        if ( page->count_info & PGC_extra )
>>> +            continue;
>>
>> This surely is a pattern, i.e. there are more similar changes to
>> make: tboot_gen_domain_integrity() e.g. ignores d->xenpage_list,
>> and hence with the goal of converting the shared info page would
>> also want adjustment. For dump_numa() it may be less important,
>> but it would still look more correct if it too got changed.
>> audit_p2m() might apparently complain about such pages (and
>> hence might be a problem with the one PGC_extra page VMX domains
>> now have). And this is only from me looking at
>> page_list_for_each(..., &d->page_list) constructs; who knows
>> what else there is.
>>
> 
> Those are dealt with by the is_special_page() patch later on I think.

Having already looked at that patch as well - I don't think so, no.
That one only replaces uses of is_xen_heap_page(), but doesn't add
any checks where such uses simply aren't needed because code is
looking at ->page_list only.

Jan

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

Re: [Xen-devel] [PATCH v3 3/6] x86 / pv: do not treat PGC_extra pages as RAM when constructing dom0

[Paul Durrant]

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich
> Sent: 06 March 2020 13:39
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: xen-devel@lists.xenproject.org; Andrew Cooper <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>;
> pdurrant@amzn.com; Roger Pau Monné <roger.pau@citrix.com>
> Subject: Re: [Xen-devel] [PATCH v3 3/6] x86 / pv: do not treat PGC_extra pages as RAM when
> constructing dom0
> 
> On 06.03.2020 13:03, Durrant, Paul wrote:
> >> -----Original Message-----
> >> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich
> >> Sent: 06 March 2020 11:56
> >> To: pdurrant@amzn.com
> >> Cc: xen-devel@lists.xenproject.org; Durrant, Paul <pdurrant@amazon.co.uk>; Roger Pau Monné
> >> <roger.pau@citrix.com>; Wei Liu <wl@xen.org>; Andrew Cooper <andrew.cooper3@citrix.com>
> >> Subject: Re: [Xen-devel] [PATCH v3 3/6] x86 / pv: do not treat PGC_extra pages as RAM when
> >> constructing dom0
> >>
> >> On 05.03.2020 13:45, pdurrant@amzn.com wrote:
> >>> --- a/xen/arch/x86/pv/dom0_build.c
> >>> +++ b/xen/arch/x86/pv/dom0_build.c
> >>> @@ -792,6 +792,10 @@ int __init dom0_construct_pv(struct domain *d,
> >>>      {
> >>>          mfn = mfn_x(page_to_mfn(page));
> >>>          BUG_ON(SHARED_M2P(get_gpfn_from_mfn(mfn)));
> >>> +
> >>> +        if ( page->count_info & PGC_extra )
> >>> +            continue;
> >>
> >> This surely is a pattern, i.e. there are more similar changes to
> >> make: tboot_gen_domain_integrity() e.g. ignores d->xenpage_list,
> >> and hence with the goal of converting the shared info page would
> >> also want adjustment. For dump_numa() it may be less important,
> >> but it would still look more correct if it too got changed.
> >> audit_p2m() might apparently complain about such pages (and
> >> hence might be a problem with the one PGC_extra page VMX domains
> >> now have). And this is only from me looking at
> >> page_list_for_each(..., &d->page_list) constructs; who knows
> >> what else there is.
> >>
> >
> > Those are dealt with by the is_special_page() patch later on I think.
> 
> Having already looked at that patch as well - I don't think so, no.
> That one only replaces uses of is_xen_heap_page(), but doesn't add
> any checks where such uses simply aren't needed because code is
> looking at ->page_list only.

Well, I did say:

"It didn't seem appropriate to use that macro here though since we know pages on the page list cannot be xenheap pages."

i.e. an open coded check here seems like the right thing to do. If I've missed other places where I need to account for pages which are specifically PGC_extra pages then I'll need to fix them similarly.

  Paul



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


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

Re: [Xen-devel] [PATCH v3 3/6] x86 / pv: do not treat PGC_extra pages as RAM when constructing dom0

[Jan Beulich]

On 06.03.2020 14:45, Paul Durrant wrote:
>> -----Original Message-----
>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich
>> Sent: 06 March 2020 13:39
>> To: Durrant, Paul <pdurrant@amazon.co.uk>
>> Cc: xen-devel@lists.xenproject.org; Andrew Cooper <andrew.cooper3@citrix.com>; Wei Liu <wl@xen.org>;
>> pdurrant@amzn.com; Roger Pau Monné <roger.pau@citrix.com>
>> Subject: Re: [Xen-devel] [PATCH v3 3/6] x86 / pv: do not treat PGC_extra pages as RAM when
>> constructing dom0
>>
>> On 06.03.2020 13:03, Durrant, Paul wrote:
>>>> -----Original Message-----
>>>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich
>>>> Sent: 06 March 2020 11:56
>>>> To: pdurrant@amzn.com
>>>> Cc: xen-devel@lists.xenproject.org; Durrant, Paul <pdurrant@amazon.co.uk>; Roger Pau Monné
>>>> <roger.pau@citrix.com>; Wei Liu <wl@xen.org>; Andrew Cooper <andrew.cooper3@citrix.com>
>>>> Subject: Re: [Xen-devel] [PATCH v3 3/6] x86 / pv: do not treat PGC_extra pages as RAM when
>>>> constructing dom0
>>>>
>>>> On 05.03.2020 13:45, pdurrant@amzn.com wrote:
>>>>> --- a/xen/arch/x86/pv/dom0_build.c
>>>>> +++ b/xen/arch/x86/pv/dom0_build.c
>>>>> @@ -792,6 +792,10 @@ int __init dom0_construct_pv(struct domain *d,
>>>>>      {
>>>>>          mfn = mfn_x(page_to_mfn(page));
>>>>>          BUG_ON(SHARED_M2P(get_gpfn_from_mfn(mfn)));
>>>>> +
>>>>> +        if ( page->count_info & PGC_extra )
>>>>> +            continue;
>>>>
>>>> This surely is a pattern, i.e. there are more similar changes to
>>>> make: tboot_gen_domain_integrity() e.g. ignores d->xenpage_list,
>>>> and hence with the goal of converting the shared info page would
>>>> also want adjustment. For dump_numa() it may be less important,
>>>> but it would still look more correct if it too got changed.
>>>> audit_p2m() might apparently complain about such pages (and
>>>> hence might be a problem with the one PGC_extra page VMX domains
>>>> now have). And this is only from me looking at
>>>> page_list_for_each(..., &d->page_list) constructs; who knows
>>>> what else there is.
>>>>
>>>
>>> Those are dealt with by the is_special_page() patch later on I think.
>>
>> Having already looked at that patch as well - I don't think so, no.
>> That one only replaces uses of is_xen_heap_page(), but doesn't add
>> any checks where such uses simply aren't needed because code is
>> looking at ->page_list only.
> 
> Well, I did say:
> 
> "It didn't seem appropriate to use that macro here though since we
> know pages on the page list cannot be xenheap pages."

And I agree and understand.

> i.e. an open coded check here seems like the right thing to do.

Indeed.

> If I've missed other places where I need to account for pages which
> are specifically PGC_extra pages then I'll need to fix them similarly.

Yes please. Thanks.

Jan

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

[Xen-devel] [PATCH v3 4/6] x86 / ioreq: use a MEMF_no_refcount allocation for server pages...

[pdurrant]

From: Paul Durrant <pdurrant@amazon.com>

... now that it is safe to assign them.

This avoids relying on libxl (or whatever toolstack is in use) setting
max_pages up with sufficient 'slop' to allow all necessary ioreq server
pages to be allocated.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: 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>

v2:
 - New in v2
---
 xen/arch/x86/hvm/ioreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
index f8a5c81546..648ef9137f 100644
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -375,7 +375,7 @@ static int hvm_alloc_ioreq_mfn(struct hvm_ioreq_server *s, bool buf)
         return 0;
     }
 
-    page = alloc_domheap_page(s->target, 0);
+    page = alloc_domheap_page(s->target, MEMF_no_refcount);
 
     if ( !page )
         return -ENOMEM;
-- 
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 4/6] x86 / ioreq: use a MEMF_no_refcount allocation for server pages...

[Jan Beulich]

On 05.03.2020 13:45, pdurrant@amzn.com wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> ... now that it is safe to assign them.
> 
> This avoids relying on libxl (or whatever toolstack is in use) setting
> max_pages up with sufficient 'slop' to allow all necessary ioreq server
> pages to be allocated.
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>

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

While this one looks to be independent of earlier patches in this
series (and hence could be considered a candidate for committing
early), I don't think we want this committed ahead of (to be
extended) patch 3, to avoid having more pages which may get mis-
handled in a few places. It would be nice if in v4 you could add
a respective post-commit-message remark.

Jan

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

[Xen-devel] [PATCH v3 5/6] mm: add 'is_special_page' macro...

[pdurrant]

From: Paul Durrant <pdurrant@amazon.com>

... to cover xenheap and PGC_extra pages.

PGC_extra pages are intended to hold data structures that are associated
with a domain and my be mapped by that domain. They should not be treated
as 'normal' guest pages (i.e. RAM or page tables). Hence, in many cases
where code currently tests is_xen_heap_page() it should also check for
the PGC_extra bit in 'count_info'.

This patch therefore defines is_special_page() to cover both cases and
converts tests if is_xen_heap_page() to is_special_page() where
appropriate.

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>
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: Tamas K Lengyel <tamas@tklengyel.com>
Cc: Tim Deegan <tim@xen.org>

v3:
 - Delete obsolete comment.

v2:
 - New in v2
---
 xen/arch/x86/domctl.c           |  2 +-
 xen/arch/x86/mm.c               |  9 ++++-----
 xen/arch/x86/mm/altp2m.c        |  2 +-
 xen/arch/x86/mm/mem_sharing.c   |  3 +--
 xen/arch/x86/mm/shadow/common.c | 13 ++++++++-----
 xen/include/xen/mm.h            |  3 +++
 6 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index ed86762fa6..add70126b9 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -394,7 +394,7 @@ long arch_do_domctl(
             page = get_page_from_gfn(d, gfn, &t, P2M_ALLOC);
 
             if ( unlikely(!page) ||
-                 unlikely(is_xen_heap_page(page)) )
+                 unlikely(is_special_page(page)) )
             {
                 if ( unlikely(p2m_is_broken(t)) )
                     type = XEN_DOMCTL_PFINFO_BROKEN;
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index ba7563ed3c..353bde5c2c 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1014,7 +1014,7 @@ get_page_from_l1e(
         unsigned long cacheattr = pte_flags_to_cacheattr(l1f);
         int err;
 
-        if ( is_xen_heap_page(page) )
+        if ( is_special_page(page) )
         {
             if ( write )
                 put_page_type(page);
@@ -2447,7 +2447,7 @@ static int cleanup_page_mappings(struct page_info *page)
     {
         page->count_info &= ~PGC_cacheattr_mask;
 
-        BUG_ON(is_xen_heap_page(page));
+        BUG_ON(is_special_page(page));
 
         rc = update_xen_mappings(mfn, 0);
     }
@@ -2477,7 +2477,7 @@ static int cleanup_page_mappings(struct page_info *page)
                 rc = rc2;
         }
 
-        if ( likely(!is_xen_heap_page(page)) )
+        if ( likely(!is_special_page(page)) )
         {
             ASSERT((page->u.inuse.type_info &
                     (PGT_type_mask | PGT_count_mask)) == PGT_writable_page);
@@ -4216,8 +4216,7 @@ int steal_page(
     if ( !(owner = page_get_owner_and_reference(page)) )
         goto fail;
 
-    if ( owner != d || is_xen_heap_page(page) ||
-         (page->count_info & PGC_extra) )
+    if ( owner != d || is_special_page(page) )
         goto fail_put;
 
     /*
diff --git a/xen/arch/x86/mm/altp2m.c b/xen/arch/x86/mm/altp2m.c
index 50768f2547..c091b03ea3 100644
--- a/xen/arch/x86/mm/altp2m.c
+++ b/xen/arch/x86/mm/altp2m.c
@@ -77,7 +77,7 @@ int altp2m_vcpu_enable_ve(struct vcpu *v, gfn_t gfn)
      * pageable() predicate for this, due to it having the same properties
      * that we want.
      */
-    if ( !p2m_is_pageable(p2mt) || is_xen_heap_page(pg) )
+    if ( !p2m_is_pageable(p2mt) || is_special_page(pg) )
     {
         rc = -EINVAL;
         goto err;
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index 3835bc928f..f49f27a3ef 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -840,9 +840,8 @@ static int nominate_page(struct domain *d, gfn_t gfn,
     if ( !p2m_is_sharable(p2mt) )
         goto out;
 
-    /* Skip xen heap pages */
     page = mfn_to_page(mfn);
-    if ( !page || is_xen_heap_page(page) )
+    if ( !page || is_special_page(page) )
         goto out;
 
     /* Check if there are mem_access/remapped altp2m entries for this page */
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index cba3ab1eba..e835940d86 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -2087,19 +2087,22 @@ static int sh_remove_all_mappings(struct domain *d, mfn_t gmfn, gfn_t gfn)
          * The qemu helper process has an untyped mapping of this dom's RAM
          * and the HVM restore program takes another.
          * Also allow one typed refcount for
-         * - Xen heap pages, to match share_xen_page_with_guest(),
-         * - ioreq server pages, to match prepare_ring_for_helper().
+         * - special pages, which are explicitly referenced and mapped by
+         *   Xen.
+         * - ioreq server pages, which may be special pages or normal
+         *   guest pages with an extra reference taken by
+         *   prepare_ring_for_helper().
          */
         if ( !(shadow_mode_external(d)
                && (page->count_info & PGC_count_mask) <= 3
                && ((page->u.inuse.type_info & PGT_count_mask)
-                   == (is_xen_heap_page(page) ||
+                   == (is_special_page(page) ||
                        (is_hvm_domain(d) && is_ioreq_server_page(d, page))))) )
             printk(XENLOG_G_ERR "can't find all mappings of mfn %"PRI_mfn
-                   " (gfn %"PRI_gfn"): c=%lx t=%lx x=%d i=%d\n",
+                   " (gfn %"PRI_gfn"): c=%lx t=%lx s=%d i=%d\n",
                    mfn_x(gmfn), gfn_x(gfn),
                    page->count_info, page->u.inuse.type_info,
-                   !!is_xen_heap_page(page),
+                   !!is_special_page(page),
                    (is_hvm_domain(d) && is_ioreq_server_page(d, page)));
     }
 
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index d0d095d9c7..3a57c9177f 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -285,6 +285,9 @@ extern struct domain *dom_cow;
 
 #include <asm/mm.h>
 
+#define is_special_page(page) \
+    (is_xen_heap_page(page) || ((page)->count_info & PGC_extra))
+
 #ifndef page_list_entry
 struct page_list_head
 {
-- 
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 5/6] mm: add 'is_special_page' macro...

[Tamas K Lengyel]

On Thu, Mar 5, 2020 at 5:45 AM <pdurrant@amzn.com> wrote:
>
> From: Paul Durrant <pdurrant@amazon.com>
>
> ... to cover xenheap and PGC_extra pages.
>
> PGC_extra pages are intended to hold data structures that are associated
> with a domain and my be mapped by that domain. They should not be treated
> as 'normal' guest pages (i.e. RAM or page tables). Hence, in many cases
> where code currently tests is_xen_heap_page() it should also check for
> the PGC_extra bit in 'count_info'.
>
> This patch therefore defines is_special_page() to cover both cases and
> converts tests if is_xen_heap_page() to is_special_page() where
> appropriate.

In context of VM forking, are these pages only used by some type of PV
mechanism? If not, would we need to get them copied somehow or are
these setup during the regular createdomain routine? Can they be
copied on-demand, ie. do these pages pass a p2m_is_ram() check?

Thanks,
Tamas

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

Re: [Xen-devel] [PATCH v3 5/6] mm: add 'is_special_page' macro...

[Durrant, Paul]

> -----Original Message-----
> From: Tamas K Lengyel <tamas@tklengyel.com>
> Sent: 05 March 2020 15:10
> To: pdurrant@amzn.com
> Cc: Xen-devel <xen-devel@lists.xenproject.org>; Durrant, Paul <pdurrant@amazon.co.uk>; 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@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>; Tim Deegan <tim@xen.org>
> Subject: RE: [EXTERNAL][PATCH v3 5/6] mm: add 'is_special_page' macro...
> 
> CAUTION: This email originated from outside of the organization. Do not click links or open
> attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On Thu, Mar 5, 2020 at 5:45 AM <pdurrant@amzn.com> wrote:
> >
> > From: Paul Durrant <pdurrant@amazon.com>
> >
> > ... to cover xenheap and PGC_extra pages.
> >
> > PGC_extra pages are intended to hold data structures that are associated
> > with a domain and my be mapped by that domain. They should not be treated
> > as 'normal' guest pages (i.e. RAM or page tables). Hence, in many cases
> > where code currently tests is_xen_heap_page() it should also check for
> > the PGC_extra bit in 'count_info'.
> >
> > This patch therefore defines is_special_page() to cover both cases and
> > converts tests if is_xen_heap_page() to is_special_page() where
> > appropriate.
> 
> In context of VM forking, are these pages only used by some type of PV
> mechanism? If not, would we need to get them copied somehow or are
> these setup during the regular createdomain routine? Can they be
> copied on-demand, ie. do these pages pass a p2m_is_ram() check?

PGC_extra domheap pages are intended as direct replacements for shared xenheap pages and should be treated the same way. Thus they do not form part of the migration stream. Their p2m type depends entirely on how they are added to the p2m, as it is for any other page.

  Paul

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

Re: [Xen-devel] [PATCH v3 5/6] mm: add 'is_special_page' macro...

[Tamas K Lengyel]

On Thu, Mar 5, 2020 at 8:38 AM Durrant, Paul <pdurrant@amazon.co.uk> wrote:
>
> > -----Original Message-----
> > From: Tamas K Lengyel <tamas@tklengyel.com>
> > Sent: 05 March 2020 15:10
> > To: pdurrant@amzn.com
> > Cc: Xen-devel <xen-devel@lists.xenproject.org>; Durrant, Paul <pdurrant@amazon.co.uk>; 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@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>; Tim Deegan <tim@xen.org>
> > Subject: RE: [EXTERNAL][PATCH v3 5/6] mm: add 'is_special_page' macro...
> >
> > CAUTION: This email originated from outside of the organization. Do not click links or open
> > attachments unless you can confirm the sender and know the content is safe.
> >
> >
> >
> > On Thu, Mar 5, 2020 at 5:45 AM <pdurrant@amzn.com> wrote:
> > >
> > > From: Paul Durrant <pdurrant@amazon.com>
> > >
> > > ... to cover xenheap and PGC_extra pages.
> > >
> > > PGC_extra pages are intended to hold data structures that are associated
> > > with a domain and my be mapped by that domain. They should not be treated
> > > as 'normal' guest pages (i.e. RAM or page tables). Hence, in many cases
> > > where code currently tests is_xen_heap_page() it should also check for
> > > the PGC_extra bit in 'count_info'.
> > >
> > > This patch therefore defines is_special_page() to cover both cases and
> > > converts tests if is_xen_heap_page() to is_special_page() where
> > > appropriate.
> >
> > In context of VM forking, are these pages only used by some type of PV
> > mechanism? If not, would we need to get them copied somehow or are
> > these setup during the regular createdomain routine? Can they be
> > copied on-demand, ie. do these pages pass a p2m_is_ram() check?
>
> PGC_extra domheap pages are intended as direct replacements for shared xenheap pages and should be treated the same way. Thus they do not form part of the migration stream. Their p2m type depends entirely on how they are added to the p2m, as it is for any other page.

OK, thanks. For the mem_sharing bits:

Acked-by: Tamas K Lengyel <tamas@tklengyel.com>

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

Re: [Xen-devel] [PATCH v3 5/6] mm: add 'is_special_page' macro...

[Alan Robinson]

A typo...

On Thu, Mar 05, 2020 at 01:45:03PM +0100, pdurrant@amzn.com wrote:
> 
> PGC_extra pages are intended to hold data structures that are associated
> with a domain and my be mapped by that domain. They should not be treated

s/my/may/

> as 'normal' guest pages (i.e. RAM or page tables). Hence, in many cases
> where code currently tests is_xen_heap_page() it should also check for
> the PGC_extra bit in 'count_info'.
> 
> This patch therefore defines is_special_page() to cover both cases and
> converts tests if is_xen_heap_page() to is_special_page() where
> appropriate.
> 

Alan

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

Re: [Xen-devel] [PATCH v3 5/6] mm: add 'is_special_page' macro...

[Durrant, Paul]

> -----Original Message-----
> From: detected-as-spam@amazon.com <detected-as-spam@amazon.com> On Behalf Of Alan Robinson
> Sent: 06 March 2020 07:02
> To: pdurrant@amzn.com
> Cc: xen-devel@lists.xenproject.org; Stefano Stabellini <sstabellini@kernel.org>; Julien Grall
> <julien@xen.org>; Wei Liu <wl@xen.org>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Durrant, Paul <pdurrant@amazon.co.uk>; Ian Jackson
> <ian.jackson@eu.citrix.com>; George Dunlap <george.dunlap@citrix.com>; Tim Deegan <tim@xen.org>; Tamas
> K Lengyel <tamas@tklengyel.com>; Jan Beulich <jbeulich@suse.com>; Roger Pau Monné
> <roger.pau@citrix.com>
> Subject: RE: [EXTERNAL][Xen-devel] [PATCH v3 5/6] mm: add 'is_special_page' macro...
> 
> CAUTION: This email originated from outside of the organization. Do not click links or open
> attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> A typo...
> 
> On Thu, Mar 05, 2020 at 01:45:03PM +0100, pdurrant@amzn.com wrote:
> >
> > PGC_extra pages are intended to hold data structures that are associated
> > with a domain and my be mapped by that domain. They should not be treated
> 
> s/my/may/

Good spot. Will fix. Thanks,

  Paul

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

Re: [Xen-devel] [PATCH v3 5/6] mm: add 'is_special_page' macro...

[Jan Beulich]

On 05.03.2020 13:45, pdurrant@amzn.com wrote:
> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -2087,19 +2087,22 @@ static int sh_remove_all_mappings(struct domain *d, mfn_t gmfn, gfn_t gfn)
>           * The qemu helper process has an untyped mapping of this dom's RAM
>           * and the HVM restore program takes another.
>           * Also allow one typed refcount for
> -         * - Xen heap pages, to match share_xen_page_with_guest(),
> -         * - ioreq server pages, to match prepare_ring_for_helper().
> +         * - special pages, which are explicitly referenced and mapped by
> +         *   Xen.
> +         * - ioreq server pages, which may be special pages or normal
> +         *   guest pages with an extra reference taken by
> +         *   prepare_ring_for_helper().
>           */
>          if ( !(shadow_mode_external(d)
>                 && (page->count_info & PGC_count_mask) <= 3
>                 && ((page->u.inuse.type_info & PGT_count_mask)
> -                   == (is_xen_heap_page(page) ||
> +                   == (is_special_page(page) ||
>                         (is_hvm_domain(d) && is_ioreq_server_page(d, page))))) )

Shouldn't you delete most of this line, after the previous patch
converted the ioreq server pages to PGC_extra ones?

>              printk(XENLOG_G_ERR "can't find all mappings of mfn %"PRI_mfn
> -                   " (gfn %"PRI_gfn"): c=%lx t=%lx x=%d i=%d\n",
> +                   " (gfn %"PRI_gfn"): c=%lx t=%lx s=%d i=%d\n",
>                     mfn_x(gmfn), gfn_x(gfn),
>                     page->count_info, page->u.inuse.type_info,
> -                   !!is_xen_heap_page(page),
> +                   !!is_special_page(page),

The !! would be nice to go away at this occasion:

> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -285,6 +285,9 @@ extern struct domain *dom_cow;
>  
>  #include <asm/mm.h>
>  
> +#define is_special_page(page) \
> +    (is_xen_heap_page(page) || ((page)->count_info & PGC_extra))

Can this become an inline function returning bool?

Also I notice this construct is used by x86 code only - is there
a particular reason it doesn't get placed in an x86 header (at
least for the time being)?

Further I notice you neither take care of is_xen_heap_mfn(), nor
does the description explain why that would not also need at
least considering conversion. _sh_propagate(), for example, has
an instance that I think would need changing.

Finally I notice there are two is_xen_heap_page() uses in tboot.c,
both of which look like they also want converting.

Jan

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

Re: [Xen-devel] [PATCH v3 5/6] mm: add 'is_special_page' macro...

[Paul Durrant]

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich
> Sent: 06 March 2020 12:20
> To: pdurrant@amzn.com
> Cc: Stefano Stabellini <sstabellini@kernel.org>; Julien Grall <julien@xen.org>; Wei Liu <wl@xen.org>;
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Andrew Cooper <andrew.cooper3@citrix.com>; Paul
> Durrant <pdurrant@amazon.com>; Ian Jackson <ian.jackson@eu.citrix.com>; George Dunlap
> <george.dunlap@citrix.com>; Tim Deegan <tim@xen.org>; Tamas K Lengyel <tamas@tklengyel.com>; xen-
> devel@lists.xenproject.org; Roger Pau Monné <roger.pau@citrix.com>
> Subject: Re: [Xen-devel] [PATCH v3 5/6] mm: add 'is_special_page' macro...
> 
> On 05.03.2020 13:45, pdurrant@amzn.com wrote:
> > --- a/xen/arch/x86/mm/shadow/common.c
> > +++ b/xen/arch/x86/mm/shadow/common.c
> > @@ -2087,19 +2087,22 @@ static int sh_remove_all_mappings(struct domain *d, mfn_t gmfn, gfn_t gfn)
> >           * The qemu helper process has an untyped mapping of this dom's RAM
> >           * and the HVM restore program takes another.
> >           * Also allow one typed refcount for
> > -         * - Xen heap pages, to match share_xen_page_with_guest(),
> > -         * - ioreq server pages, to match prepare_ring_for_helper().
> > +         * - special pages, which are explicitly referenced and mapped by
> > +         *   Xen.
> > +         * - ioreq server pages, which may be special pages or normal
> > +         *   guest pages with an extra reference taken by
> > +         *   prepare_ring_for_helper().
> >           */
> >          if ( !(shadow_mode_external(d)
> >                 && (page->count_info & PGC_count_mask) <= 3
> >                 && ((page->u.inuse.type_info & PGT_count_mask)
> > -                   == (is_xen_heap_page(page) ||
> > +                   == (is_special_page(page) ||
> >                         (is_hvm_domain(d) && is_ioreq_server_page(d, page))))) )
> 
> Shouldn't you delete most of this line, after the previous patch
> converted the ioreq server pages to PGC_extra ones?

I thought that too originally but then I realise we still have to cater for the 'legacy' emulators that still require IOREQ server pages to be mapped through the p2m, in which case they will not be PGC_extra pages.

> 
> >              printk(XENLOG_G_ERR "can't find all mappings of mfn %"PRI_mfn
> > -                   " (gfn %"PRI_gfn"): c=%lx t=%lx x=%d i=%d\n",
> > +                   " (gfn %"PRI_gfn"): c=%lx t=%lx s=%d i=%d\n",
> >                     mfn_x(gmfn), gfn_x(gfn),
> >                     page->count_info, page->u.inuse.type_info,
> > -                   !!is_xen_heap_page(page),
> > +                   !!is_special_page(page),
> 
> The !! would be nice to go away at this occasion:
> 

Ok.

> > --- a/xen/include/xen/mm.h
> > +++ b/xen/include/xen/mm.h
> > @@ -285,6 +285,9 @@ extern struct domain *dom_cow;
> >
> >  #include <asm/mm.h>
> >
> > +#define is_special_page(page) \
> > +    (is_xen_heap_page(page) || ((page)->count_info & PGC_extra))
> 
> Can this become an inline function returning bool?
> 

I guess so. Hopefully there are no header inclusion orderings that would bite.

> Also I notice this construct is used by x86 code only - is there
> a particular reason it doesn't get placed in an x86 header (at
> least for the time being)?
> 

PGC_extra pages are common so maybe it is better off defined here so it is available to ARM code?

> Further I notice you neither take care of is_xen_heap_mfn(), nor
> does the description explain why that would not also need at
> least considering conversion. _sh_propagate(), for example, has
> an instance that I think would need changing.
> 

Ok; I'd simply not spotted any users that were vulnerable... I'll fix that one and re-check.

> Finally I notice there are two is_xen_heap_page() uses in tboot.c,
> both of which look like they also want converting.
> 

OK; I'd seen those so no idea why I didn't do the conversion. I'll fix.

  Paul

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


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

Re: [Xen-devel] [PATCH v3 5/6] mm: add 'is_special_page' macro...

[Jan Beulich]

On 06.03.2020 13:35, Paul Durrant wrote:
>> -----Original Message-----
>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich
>> Sent: 06 March 2020 12:20
>> To: pdurrant@amzn.com
>> Cc: Stefano Stabellini <sstabellini@kernel.org>; Julien Grall <julien@xen.org>; Wei Liu <wl@xen.org>;
>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Andrew Cooper <andrew.cooper3@citrix.com>; Paul
>> Durrant <pdurrant@amazon.com>; Ian Jackson <ian.jackson@eu.citrix.com>; George Dunlap
>> <george.dunlap@citrix.com>; Tim Deegan <tim@xen.org>; Tamas K Lengyel <tamas@tklengyel.com>; xen-
>> devel@lists.xenproject.org; Roger Pau Monné <roger.pau@citrix.com>
>> Subject: Re: [Xen-devel] [PATCH v3 5/6] mm: add 'is_special_page' macro...
>>
>> On 05.03.2020 13:45, pdurrant@amzn.com wrote:
>>> --- a/xen/arch/x86/mm/shadow/common.c
>>> +++ b/xen/arch/x86/mm/shadow/common.c
>>> @@ -2087,19 +2087,22 @@ static int sh_remove_all_mappings(struct domain *d, mfn_t gmfn, gfn_t gfn)
>>>           * The qemu helper process has an untyped mapping of this dom's RAM
>>>           * and the HVM restore program takes another.
>>>           * Also allow one typed refcount for
>>> -         * - Xen heap pages, to match share_xen_page_with_guest(),
>>> -         * - ioreq server pages, to match prepare_ring_for_helper().
>>> +         * - special pages, which are explicitly referenced and mapped by
>>> +         *   Xen.
>>> +         * - ioreq server pages, which may be special pages or normal
>>> +         *   guest pages with an extra reference taken by
>>> +         *   prepare_ring_for_helper().
>>>           */
>>>          if ( !(shadow_mode_external(d)
>>>                 && (page->count_info & PGC_count_mask) <= 3
>>>                 && ((page->u.inuse.type_info & PGT_count_mask)
>>> -                   == (is_xen_heap_page(page) ||
>>> +                   == (is_special_page(page) ||
>>>                         (is_hvm_domain(d) && is_ioreq_server_page(d, page))))) )
>>
>> Shouldn't you delete most of this line, after the previous patch
>> converted the ioreq server pages to PGC_extra ones?
> 
> I thought that too originally but then I realise we still have to
> cater for the 'legacy' emulators that still require IOREQ server
> pages to be mapped through the p2m, in which case they will not
> be PGC_extra pages.

Oh, indeed. (I don't suppose we can ever do away with this legacy
mechanism?)

>> Also I notice this construct is used by x86 code only - is there
>> a particular reason it doesn't get placed in an x86 header (at
>> least for the time being)?
> 
> PGC_extra pages are common so maybe it is better off defined here
> so it is available to ARM code?

To be honest, my question was mainly based on me being puzzled that
Arm (or common) code doesn't need any such adjustment. As a result
I'm wondering whether that's just "luck" (in which case I'd agree
the placement could remain as is), or whether there's a deeper
reason behind that, largely guaranteeing Arm would also never need
it.

Jan

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

Re: [Xen-devel] [PATCH v3 5/6] mm: add 'is_special_page' macro...

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 06 March 2020 13:44
> To: Paul Durrant <xadimgnik@gmail.com>
> Cc: pdurrant@amzn.com; 'Stefano Stabellini' <sstabellini@kernel.org>; 'Julien Grall' <julien@xen.org>;
> 'Wei Liu' <wl@xen.org>; 'Konrad Rzeszutek Wilk' <konrad.wilk@oracle.com>; 'Andrew Cooper'
> <andrew.cooper3@citrix.com>; Durrant, Paul <pdurrant@amazon.co.uk>; 'Ian Jackson'
> <ian.jackson@eu.citrix.com>; 'George Dunlap' <george.dunlap@citrix.com>; 'Tim Deegan' <tim@xen.org>;
> 'Tamas K Lengyel' <tamas@tklengyel.com>; xen-devel@lists.xenproject.org; 'Roger Pau Monné'
> <roger.pau@citrix.com>
> Subject: Re: [PATCH v3 5/6] mm: add 'is_special_page' macro...
> 
> On 06.03.2020 13:35, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich
> >> Sent: 06 March 2020 12:20
> >> To: pdurrant@amzn.com
> >> Cc: Stefano Stabellini <sstabellini@kernel.org>; Julien Grall <julien@xen.org>; Wei Liu
> <wl@xen.org>;
> >> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Andrew Cooper <andrew.cooper3@citrix.com>; Paul
> >> Durrant <pdurrant@amazon.com>; Ian Jackson <ian.jackson@eu.citrix.com>; George Dunlap
> >> <george.dunlap@citrix.com>; Tim Deegan <tim@xen.org>; Tamas K Lengyel <tamas@tklengyel.com>; xen-
> >> devel@lists.xenproject.org; Roger Pau Monné <roger.pau@citrix.com>
> >> Subject: Re: [Xen-devel] [PATCH v3 5/6] mm: add 'is_special_page' macro...
> >>
> >> On 05.03.2020 13:45, pdurrant@amzn.com wrote:
> >>> --- a/xen/arch/x86/mm/shadow/common.c
> >>> +++ b/xen/arch/x86/mm/shadow/common.c
> >>> @@ -2087,19 +2087,22 @@ static int sh_remove_all_mappings(struct domain *d, mfn_t gmfn, gfn_t gfn)
> >>>           * The qemu helper process has an untyped mapping of this dom's RAM
> >>>           * and the HVM restore program takes another.
> >>>           * Also allow one typed refcount for
> >>> -         * - Xen heap pages, to match share_xen_page_with_guest(),
> >>> -         * - ioreq server pages, to match prepare_ring_for_helper().
> >>> +         * - special pages, which are explicitly referenced and mapped by
> >>> +         *   Xen.
> >>> +         * - ioreq server pages, which may be special pages or normal
> >>> +         *   guest pages with an extra reference taken by
> >>> +         *   prepare_ring_for_helper().
> >>>           */
> >>>          if ( !(shadow_mode_external(d)
> >>>                 && (page->count_info & PGC_count_mask) <= 3
> >>>                 && ((page->u.inuse.type_info & PGT_count_mask)
> >>> -                   == (is_xen_heap_page(page) ||
> >>> +                   == (is_special_page(page) ||
> >>>                         (is_hvm_domain(d) && is_ioreq_server_page(d, page))))) )
> >>
> >> Shouldn't you delete most of this line, after the previous patch
> >> converted the ioreq server pages to PGC_extra ones?
> >
> > I thought that too originally but then I realise we still have to
> > cater for the 'legacy' emulators that still require IOREQ server
> > pages to be mapped through the p2m, in which case they will not
> > be PGC_extra pages.
> 
> Oh, indeed. (I don't suppose we can ever do away with this legacy
> mechanism?)

It's tricky because it would either mean breaking older (pre resource-mapping) QEMUs, or allowing the toolstack to allocate the 'special' pages with an extra flag to make them PGC_extra.

> 
> >> Also I notice this construct is used by x86 code only - is there
> >> a particular reason it doesn't get placed in an x86 header (at
> >> least for the time being)?
> >
> > PGC_extra pages are common so maybe it is better off defined here
> > so it is available to ARM code?
> 
> To be honest, my question was mainly based on me being puzzled that
> Arm (or common) code doesn't need any such adjustment. As a result
> I'm wondering whether that's just "luck" (in which case I'd agree
> the placement could remain as is), or whether there's a deeper
> reason behind that, largely guaranteeing Arm would also never need
> it.
> 

I'll have a chat with Julien and see what he thinks.

  Paul



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

Re: [Xen-devel] [PATCH v3 5/6] mm: add 'is_special_page' macro...

[Jan Beulich]

On 06.03.2020 14:48, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 06 March 2020 13:44
>> To: Paul Durrant <xadimgnik@gmail.com>
>> Cc: pdurrant@amzn.com; 'Stefano Stabellini' <sstabellini@kernel.org>; 'Julien Grall' <julien@xen.org>;
>> 'Wei Liu' <wl@xen.org>; 'Konrad Rzeszutek Wilk' <konrad.wilk@oracle.com>; 'Andrew Cooper'
>> <andrew.cooper3@citrix.com>; Durrant, Paul <pdurrant@amazon.co.uk>; 'Ian Jackson'
>> <ian.jackson@eu.citrix.com>; 'George Dunlap' <george.dunlap@citrix.com>; 'Tim Deegan' <tim@xen.org>;
>> 'Tamas K Lengyel' <tamas@tklengyel.com>; xen-devel@lists.xenproject.org; 'Roger Pau Monné'
>> <roger.pau@citrix.com>
>> Subject: Re: [PATCH v3 5/6] mm: add 'is_special_page' macro...
>>
>> On 06.03.2020 13:35, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich
>>>> Sent: 06 March 2020 12:20
>>>> To: pdurrant@amzn.com
>>>> Cc: Stefano Stabellini <sstabellini@kernel.org>; Julien Grall <julien@xen.org>; Wei Liu
>> <wl@xen.org>;
>>>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Andrew Cooper <andrew.cooper3@citrix.com>; Paul
>>>> Durrant <pdurrant@amazon.com>; Ian Jackson <ian.jackson@eu.citrix.com>; George Dunlap
>>>> <george.dunlap@citrix.com>; Tim Deegan <tim@xen.org>; Tamas K Lengyel <tamas@tklengyel.com>; xen-
>>>> devel@lists.xenproject.org; Roger Pau Monné <roger.pau@citrix.com>
>>>> Subject: Re: [Xen-devel] [PATCH v3 5/6] mm: add 'is_special_page' macro...
>>>>
>>>> On 05.03.2020 13:45, pdurrant@amzn.com wrote:
>>>>> --- a/xen/arch/x86/mm/shadow/common.c
>>>>> +++ b/xen/arch/x86/mm/shadow/common.c
>>>>> @@ -2087,19 +2087,22 @@ static int sh_remove_all_mappings(struct domain *d, mfn_t gmfn, gfn_t gfn)
>>>>>           * The qemu helper process has an untyped mapping of this dom's RAM
>>>>>           * and the HVM restore program takes another.
>>>>>           * Also allow one typed refcount for
>>>>> -         * - Xen heap pages, to match share_xen_page_with_guest(),
>>>>> -         * - ioreq server pages, to match prepare_ring_for_helper().
>>>>> +         * - special pages, which are explicitly referenced and mapped by
>>>>> +         *   Xen.
>>>>> +         * - ioreq server pages, which may be special pages or normal
>>>>> +         *   guest pages with an extra reference taken by
>>>>> +         *   prepare_ring_for_helper().
>>>>>           */
>>>>>          if ( !(shadow_mode_external(d)
>>>>>                 && (page->count_info & PGC_count_mask) <= 3
>>>>>                 && ((page->u.inuse.type_info & PGT_count_mask)
>>>>> -                   == (is_xen_heap_page(page) ||
>>>>> +                   == (is_special_page(page) ||
>>>>>                         (is_hvm_domain(d) && is_ioreq_server_page(d, page))))) )
>>>>
>>>> Shouldn't you delete most of this line, after the previous patch
>>>> converted the ioreq server pages to PGC_extra ones?
>>>
>>> I thought that too originally but then I realise we still have to
>>> cater for the 'legacy' emulators that still require IOREQ server
>>> pages to be mapped through the p2m, in which case they will not
>>> be PGC_extra pages.
>>
>> Oh, indeed. (I don't suppose we can ever do away with this legacy
>> mechanism?)
> 
> It's tricky because it would either mean breaking older (pre
> resource-mapping) QEMUs,

Didn't even qemu-trad get switched? (Anyway, not a big deal here,
just would have been nice if this large conditional could have
been shrunk a little in size.)

> or allowing the toolstack to allocate the 'special' pages with
> an extra flag to make them PGC_extra.

Doesn't sound impossible, but also not something we want to eagerly
go for.

Jan

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

Re: [Xen-devel] [PATCH v3 5/6] mm: add 'is_special_page' macro...

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 06 March 2020 13:52
> To: Paul Durrant <xadimgnik@gmail.com>
> Cc: pdurrant@amzn.com; 'Stefano Stabellini' <sstabellini@kernel.org>; 'Julien Grall' <julien@xen.org>;
> 'Wei Liu' <wl@xen.org>; 'Konrad Rzeszutek Wilk' <konrad.wilk@oracle.com>; 'Andrew Cooper'
> <andrew.cooper3@citrix.com>; 'Ian Jackson' <ian.jackson@eu.citrix.com>; 'George Dunlap'
> <george.dunlap@citrix.com>; 'Tim Deegan' <tim@xen.org>; 'Tamas K Lengyel' <tamas@tklengyel.com>; xen-
> devel@lists.xenproject.org; 'Roger Pau Monné' <roger.pau@citrix.com>
> Subject: Re: [PATCH v3 5/6] mm: add 'is_special_page' macro...
> 
> On 06.03.2020 14:48, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: 06 March 2020 13:44
> >> To: Paul Durrant <xadimgnik@gmail.com>
> >> Cc: pdurrant@amzn.com; 'Stefano Stabellini' <sstabellini@kernel.org>; 'Julien Grall'
> <julien@xen.org>;
> >> 'Wei Liu' <wl@xen.org>; 'Konrad Rzeszutek Wilk' <konrad.wilk@oracle.com>; 'Andrew Cooper'
> >> <andrew.cooper3@citrix.com>; Durrant, Paul <pdurrant@amazon.co.uk>; 'Ian Jackson'
> >> <ian.jackson@eu.citrix.com>; 'George Dunlap' <george.dunlap@citrix.com>; 'Tim Deegan'
> <tim@xen.org>;
> >> 'Tamas K Lengyel' <tamas@tklengyel.com>; xen-devel@lists.xenproject.org; 'Roger Pau Monné'
> >> <roger.pau@citrix.com>
> >> Subject: Re: [PATCH v3 5/6] mm: add 'is_special_page' macro...
> >>
> >> On 06.03.2020 13:35, Paul Durrant wrote:
> >>>> -----Original Message-----
> >>>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich
> >>>> Sent: 06 March 2020 12:20
> >>>> To: pdurrant@amzn.com
> >>>> Cc: Stefano Stabellini <sstabellini@kernel.org>; Julien Grall <julien@xen.org>; Wei Liu
> >> <wl@xen.org>;
> >>>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Andrew Cooper <andrew.cooper3@citrix.com>; Paul
> >>>> Durrant <pdurrant@amazon.com>; Ian Jackson <ian.jackson@eu.citrix.com>; George Dunlap
> >>>> <george.dunlap@citrix.com>; Tim Deegan <tim@xen.org>; Tamas K Lengyel <tamas@tklengyel.com>; xen-
> >>>> devel@lists.xenproject.org; Roger Pau Monné <roger.pau@citrix.com>
> >>>> Subject: Re: [Xen-devel] [PATCH v3 5/6] mm: add 'is_special_page' macro...
> >>>>
> >>>> On 05.03.2020 13:45, pdurrant@amzn.com wrote:
> >>>>> --- a/xen/arch/x86/mm/shadow/common.c
> >>>>> +++ b/xen/arch/x86/mm/shadow/common.c
> >>>>> @@ -2087,19 +2087,22 @@ static int sh_remove_all_mappings(struct domain *d, mfn_t gmfn, gfn_t
> gfn)
> >>>>>           * The qemu helper process has an untyped mapping of this dom's RAM
> >>>>>           * and the HVM restore program takes another.
> >>>>>           * Also allow one typed refcount for
> >>>>> -         * - Xen heap pages, to match share_xen_page_with_guest(),
> >>>>> -         * - ioreq server pages, to match prepare_ring_for_helper().
> >>>>> +         * - special pages, which are explicitly referenced and mapped by
> >>>>> +         *   Xen.
> >>>>> +         * - ioreq server pages, which may be special pages or normal
> >>>>> +         *   guest pages with an extra reference taken by
> >>>>> +         *   prepare_ring_for_helper().
> >>>>>           */
> >>>>>          if ( !(shadow_mode_external(d)
> >>>>>                 && (page->count_info & PGC_count_mask) <= 3
> >>>>>                 && ((page->u.inuse.type_info & PGT_count_mask)
> >>>>> -                   == (is_xen_heap_page(page) ||
> >>>>> +                   == (is_special_page(page) ||
> >>>>>                         (is_hvm_domain(d) && is_ioreq_server_page(d, page))))) )
> >>>>
> >>>> Shouldn't you delete most of this line, after the previous patch
> >>>> converted the ioreq server pages to PGC_extra ones?
> >>>
> >>> I thought that too originally but then I realise we still have to
> >>> cater for the 'legacy' emulators that still require IOREQ server
> >>> pages to be mapped through the p2m, in which case they will not
> >>> be PGC_extra pages.
> >>
> >> Oh, indeed. (I don't suppose we can ever do away with this legacy
> >> mechanism?)
> >
> > It's tricky because it would either mean breaking older (pre
> > resource-mapping) QEMUs,
> 
> Didn't even qemu-trad get switched? (Anyway, not a big deal here,
> just would have been nice if this large conditional could have
> been shrunk a little in size.)

Yes, trad is switched but I thought our position was that we supported use of arbitrary distro versions of QEMU in which case any compat code really does have to stick around for a very long time :-(

> 
> > or allowing the toolstack to allocate the 'special' pages with
> > an extra flag to make them PGC_extra.
> 
> Doesn't sound impossible, but also not something we want to eagerly
> go for.
> 

No, probably not worth it just to save doing this test.

  Paul



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

Re: [Xen-devel] [PATCH v3 5/6] mm: add 'is_special_page' macro...

[Julien Grall]

Hi Jan,

On 06/03/2020 13:44, Jan Beulich wrote:
> On 06.03.2020 13:35, Paul Durrant wrote:
>>> -----Original Message-----
>>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich
>>> Sent: 06 March 2020 12:20
>>> To: pdurrant@amzn.com
>>> Cc: Stefano Stabellini <sstabellini@kernel.org>; Julien Grall <julien@xen.org>; Wei Liu <wl@xen.org>;
>>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Andrew Cooper <andrew.cooper3@citrix.com>; Paul
>>> Durrant <pdurrant@amazon.com>; Ian Jackson <ian.jackson@eu.citrix.com>; George Dunlap
>>> <george.dunlap@citrix.com>; Tim Deegan <tim@xen.org>; Tamas K Lengyel <tamas@tklengyel.com>; xen-
>>> devel@lists.xenproject.org; Roger Pau Monné <roger.pau@citrix.com>
>>> Subject: Re: [Xen-devel] [PATCH v3 5/6] mm: add 'is_special_page' macro...
>>>
>>> On 05.03.2020 13:45, pdurrant@amzn.com wrote:
>>>> --- a/xen/arch/x86/mm/shadow/common.c
>>>> +++ b/xen/arch/x86/mm/shadow/common.c
>>>> @@ -2087,19 +2087,22 @@ static int sh_remove_all_mappings(struct domain *d, mfn_t gmfn, gfn_t gfn)
>>>>            * The qemu helper process has an untyped mapping of this dom's RAM
>>>>            * and the HVM restore program takes another.
>>>>            * Also allow one typed refcount for
>>>> -         * - Xen heap pages, to match share_xen_page_with_guest(),
>>>> -         * - ioreq server pages, to match prepare_ring_for_helper().
>>>> +         * - special pages, which are explicitly referenced and mapped by
>>>> +         *   Xen.
>>>> +         * - ioreq server pages, which may be special pages or normal
>>>> +         *   guest pages with an extra reference taken by
>>>> +         *   prepare_ring_for_helper().
>>>>            */
>>>>           if ( !(shadow_mode_external(d)
>>>>                  && (page->count_info & PGC_count_mask) <= 3
>>>>                  && ((page->u.inuse.type_info & PGT_count_mask)
>>>> -                   == (is_xen_heap_page(page) ||
>>>> +                   == (is_special_page(page) ||
>>>>                          (is_hvm_domain(d) && is_ioreq_server_page(d, page))))) )
>>>
>>> Shouldn't you delete most of this line, after the previous patch
>>> converted the ioreq server pages to PGC_extra ones?
>>
>> I thought that too originally but then I realise we still have to
>> cater for the 'legacy' emulators that still require IOREQ server
>> pages to be mapped through the p2m, in which case they will not
>> be PGC_extra pages.
> 
> Oh, indeed. (I don't suppose we can ever do away with this legacy
> mechanism?)
> 
>>> Also I notice this construct is used by x86 code only - is there
>>> a particular reason it doesn't get placed in an x86 header (at
>>> least for the time being)?
>>
>> PGC_extra pages are common so maybe it is better off defined here
>> so it is available to ARM code?
> 
> To be honest, my question was mainly based on me being puzzled that
> Arm (or common) code doesn't need any such adjustment. As a result
> I'm wondering whether that's just "luck" (in which case I'd agree
> the placement could remain as is), or whether there's a deeper
> reason behind that, largely guaranteeing Arm would also never need
> it.

is_special_page() is used in features that are not supported on Arm yet 
(e.g migration). So we will need it in the future and therefore the 
helper should be defined in common header.

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 5/6] mm: add 'is_special_page' macro...

[Jan Beulich]

On 06.03.2020 15:26, Julien Grall wrote:
> On 06/03/2020 13:44, Jan Beulich wrote:
>> On 06.03.2020 13:35, Paul Durrant wrote:
>>>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich
>>>> Sent: 06 March 2020 12:20
>>>>
>>>> On 05.03.2020 13:45, pdurrant@amzn.com wrote:
>>>>> --- a/xen/arch/x86/mm/shadow/common.c
>>>>> +++ b/xen/arch/x86/mm/shadow/common.c
>>>>> @@ -2087,19 +2087,22 @@ static int sh_remove_all_mappings(struct domain *d, mfn_t gmfn, gfn_t gfn)
>>>>>            * The qemu helper process has an untyped mapping of this dom's RAM
>>>>>            * and the HVM restore program takes another.
>>>>>            * Also allow one typed refcount for
>>>>> -         * - Xen heap pages, to match share_xen_page_with_guest(),
>>>>> -         * - ioreq server pages, to match prepare_ring_for_helper().
>>>>> +         * - special pages, which are explicitly referenced and mapped by
>>>>> +         *   Xen.
>>>>> +         * - ioreq server pages, which may be special pages or normal
>>>>> +         *   guest pages with an extra reference taken by
>>>>> +         *   prepare_ring_for_helper().
>>>>>            */
>>>>>           if ( !(shadow_mode_external(d)
>>>>>                  && (page->count_info & PGC_count_mask) <= 3
>>>>>                  && ((page->u.inuse.type_info & PGT_count_mask)
>>>>> -                   == (is_xen_heap_page(page) ||
>>>>> +                   == (is_special_page(page) ||
>>>>>                          (is_hvm_domain(d) && is_ioreq_server_page(d, page))))) )
>>>>
>>>> Shouldn't you delete most of this line, after the previous patch
>>>> converted the ioreq server pages to PGC_extra ones?
>>>
>>> I thought that too originally but then I realise we still have to
>>> cater for the 'legacy' emulators that still require IOREQ server
>>> pages to be mapped through the p2m, in which case they will not
>>> be PGC_extra pages.
>>
>> Oh, indeed. (I don't suppose we can ever do away with this legacy
>> mechanism?)
>>
>>>> Also I notice this construct is used by x86 code only - is there
>>>> a particular reason it doesn't get placed in an x86 header (at
>>>> least for the time being)?
>>>
>>> PGC_extra pages are common so maybe it is better off defined here
>>> so it is available to ARM code?
>>
>> To be honest, my question was mainly based on me being puzzled that
>> Arm (or common) code doesn't need any such adjustment. As a result
>> I'm wondering whether that's just "luck" (in which case I'd agree
>> the placement could remain as is), or whether there's a deeper
>> reason behind that, largely guaranteeing Arm would also never need
>> it.
> 
> is_special_page() is used in features that are not supported on Arm yet 
> (e.g migration). So we will need it in the future and therefore the 
> helper should be defined in common header.

Okay then, thanks for clarifying.

Jan

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

[Xen-devel] [PATCH v3 6/6] domain: use PGC_extra domheap page for shared_info

[pdurrant]

From: Paul Durrant <pdurrant@amazon.com>

Currently shared_info is a shared xenheap page but shared xenheap pages
complicate future plans for live-update of Xen so it is desirable to,
where possible, not use them [1]. This patch therefore converts shared_info
into a PGC_extra domheap page. This does entail freeing shared_info during
domain_relinquish_resources() rather than domain_destroy() so care is
needed to avoid de-referencing a NULL shared_info pointer hence some
extra checks of 'is_dying' are needed.

NOTE: For Arm, the call to free_shared_info() in arch_domain_destroy() is
      left in place since it is idempotent and called in the error path for
      arch_domain_create().

[1] See https://lists.xenproject.org/archives/html/xen-devel/2020-02/msg02018.html

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

v2:
 - Addressed comments from Julien
 - Expanded the commit comment to explain why this patch is wanted
---
 xen/arch/arm/domain.c      |  2 ++
 xen/arch/x86/domain.c      |  3 ++-
 xen/common/domain.c        | 28 ++++++++++++++++++++++++----
 xen/common/event_channel.c |  3 +++
 xen/common/time.c          | 15 +++++++++++++++
 5 files changed, 46 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 5298d80bd2..741f6dd444 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -1005,6 +1005,8 @@ int domain_relinquish_resources(struct domain *d)
         BUG();
     }
 
+    free_shared_info(d);
+
     return 0;
 }
 
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index bdcc0d972a..15db476646 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -690,7 +690,6 @@ void arch_domain_destroy(struct domain *d)
         pv_domain_destroy(d);
     free_perdomain_mappings(d);
 
-    free_shared_info(d);
     cleanup_domain_irq_mapping(d);
 
     psr_domain_free(d);
@@ -2245,6 +2244,8 @@ int domain_relinquish_resources(struct domain *d)
     if ( is_hvm_domain(d) )
         hvm_domain_relinquish_resources(d);
 
+    free_shared_info(d);
+
     return 0;
 }
 
diff --git a/xen/common/domain.c b/xen/common/domain.c
index ba7a905258..886206f648 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1650,24 +1650,44 @@ int continue_hypercall_on_cpu(
 
 int alloc_shared_info(struct domain *d, unsigned int memflags)
 {
-    if ( (d->shared_info.virt = alloc_xenheap_pages(0, memflags)) == NULL )
+    struct page_info *pg;
+
+    pg = alloc_domheap_page(d, MEMF_no_refcount | memflags);
+    if ( !pg )
         return -ENOMEM;
 
-    d->shared_info.mfn = virt_to_mfn(d->shared_info.virt);
+    if ( !get_page_and_type(pg, d, PGT_writable_page) )
+    {
+        /*
+         * The domain should not be running at this point so there is
+         * no way we should reach this error path.
+         */
+        ASSERT_UNREACHABLE();
+        return -ENODATA;
+    }
+
+    d->shared_info.mfn = page_to_mfn(pg);
+    d->shared_info.virt = __map_domain_page_global(pg);
 
     clear_page(d->shared_info.virt);
-    share_xen_page_with_guest(mfn_to_page(d->shared_info.mfn), d, SHARE_rw);
 
     return 0;
 }
 
 void free_shared_info(struct domain *d)
 {
+    struct page_info *pg;
+
     if ( !d->shared_info.virt )
         return;
 
-    free_xenheap_page(d->shared_info.virt);
+    unmap_domain_page_global(d->shared_info.virt);
     d->shared_info.virt = NULL;
+
+    pg = mfn_to_page(d->shared_info.mfn);
+
+    put_page_alloc_ref(pg);
+    put_page_and_type(pg);
 }
 
 /*
diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index e86e2bfab0..a17422284d 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -1325,6 +1325,9 @@ void evtchn_destroy(struct domain *d)
 {
     unsigned int i;
 
+    /* This must be done before shared_info is freed */
+    BUG_ON(!d->shared_info.virt);
+
     /* After this barrier no new event-channel allocations can occur. */
     BUG_ON(!d->is_dying);
     spin_barrier(&d->event_lock);
diff --git a/xen/common/time.c b/xen/common/time.c
index 58fa9abc40..ada02faf07 100644
--- a/xen/common/time.c
+++ b/xen/common/time.c
@@ -99,6 +99,18 @@ void update_domain_wallclock_time(struct domain *d)
     uint32_t *wc_version;
     uint64_t sec;
 
+    if ( d != current->domain )
+    {
+        /*
+         * We need to check is_dying here as, if it is set, the
+         * shared_info may have been freed. To do this safely we need
+         * hold the domain lock.
+         */
+        domain_lock(d);
+        if ( d->is_dying )
+            goto unlock;
+    }
+
     spin_lock(&wc_lock);
 
     wc_version = &shared_info(d, wc_version);
@@ -121,6 +133,9 @@ void update_domain_wallclock_time(struct domain *d)
     *wc_version = version_update_end(*wc_version);
 
     spin_unlock(&wc_lock);
+ unlock:
+    if ( d != current->domain )
+        domain_unlock(d);
 }
 
 /* Set clock to <secs,usecs> after 00:00:00 UTC, 1 January, 1970. */
-- 
2.20.1


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