diff mbox series

[v3,1/6] domain: introduce alloc/free_shared_info() helpers...

Message ID 20200305124504.3564-2-pdurrant@amzn.com (mailing list archive)
State New, archived
Headers show
Series remove one more shared xenheap page: shared_info | expand

Commit Message

pdurrant@amzn.com March 5, 2020, 12:44 p.m. UTC 
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(-)

Comments

Xia, Hongyan March 5, 2020, 1:23 p.m. UTC | #1 
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
Xia, Hongyan March 5, 2020, 1:25 p.m. UTC | #2 
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
Jan Beulich March 6, 2020, 11:41 a.m. UTC | #3 
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
diff mbox series

Patch

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__ */