diff mbox series

[v3,4/4] x86/mem_sharing: compile mem_sharing subsystem only when kconfig is enabled

Message ID 20190426172138.14669-4-tamas@tklengyel.com (mailing list archive)
State Superseded
Headers show
Series [v3,1/4] x86/mem_sharing: reorder when pages are unlocked and released | expand

Commit Message

Tamas K Lengyel April 26, 2019, 5:21 p.m. UTC 
Disable it by default as it is only an experimental subsystem.

Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Roger Pau Monne <roger.pau@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
---
 xen/arch/x86/Kconfig              |  8 +++++++-
 xen/arch/x86/domctl.c             |  2 ++
 xen/arch/x86/mm.c                 |  4 ++--
 xen/arch/x86/mm/Makefile          |  2 +-
 xen/arch/x86/x86_64/compat/mm.c   |  2 ++
 xen/arch/x86/x86_64/mm.c          |  2 ++
 xen/common/Kconfig                |  3 ---
 xen/common/domain.c               |  2 +-
 xen/common/grant_table.c          |  2 +-
 xen/common/memory.c               |  2 +-
 xen/common/vm_event.c             |  6 +++---
 xen/include/asm-x86/mem_sharing.h | 31 +++++++++++++++++++++++++++++++
 xen/include/asm-x86/mm.h          |  3 +++
 xen/include/xen/sched.h           |  2 +-
 xen/include/xsm/dummy.h           |  2 +-
 xen/include/xsm/xsm.h             |  4 ++--
 xen/xsm/dummy.c                   |  2 +-
 xen/xsm/flask/hooks.c             |  4 ++--
 18 files changed, 63 insertions(+), 20 deletions(-)

Comments

Jan Beulich April 29, 2019, 3:32 p.m. UTC | #1 
>>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -17,7 +17,6 @@ config X86
>  	select HAS_KEXEC
>  	select MEM_ACCESS_ALWAYS_ON
>  	select HAS_MEM_PAGING
> -	select HAS_MEM_SHARING
>  	select HAS_NS16550
>  	select HAS_PASSTHROUGH
>  	select HAS_PCI
> @@ -198,6 +197,13 @@ config PV_SHIM_EXCLUSIVE
>  	  firmware, and will not function correctly in other scenarios.
>  
>  	  If unsure, say N.
> +
> +config MEM_SHARING
> +    bool
> +    default n
> +    depends on HVM
> +    prompt "Xen memory sharing support" if EXPERT = "y"
> +

As per all the context above, please use proper (tab) indentation.
Also please omit the pointless "default n". And then the "bool"
and "prompt ..." can (and hence should) be combined into a
single line.

I also wonder whether this shouldn't be in common/Kconfig, but
then again it can be moved there if Arm would ever gain
mem-sharing support.

> @@ -98,4 +100,33 @@ void mem_sharing_init(void);
>   */
>  int relinquish_shared_pages(struct domain *d);
>  
> +#else
> +
> +static inline unsigned int mem_sharing_get_nr_saved_mfns(void)
> +{
> +    return 0;
> +}
> +static inline unsigned int mem_sharing_get_nr_shared_mfns(void)
> +{
> +    return 0;
> +}

I follow you for these.

> +static inline int mem_sharing_unshare_page(struct domain *d,
> +                                           unsigned long gfn,
> +                                           uint16_t flags)
> +{
> +    return 0;
> +}

But shouldn't this one (just as an example, there may be more
below) return -EOPNOTSUPP?

And in general - if these inline functions are needed, shouldn't
all of them (or at least some) gain ASSERT_UNREACHABLE(), as we
do elsewhere?

Jan
Tamas K Lengyel April 29, 2019, 3:49 p.m. UTC | #2 
On Mon, Apr 29, 2019 at 9:32 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
> > --- a/xen/arch/x86/Kconfig
> > +++ b/xen/arch/x86/Kconfig
> > @@ -17,7 +17,6 @@ config X86
> >       select HAS_KEXEC
> >       select MEM_ACCESS_ALWAYS_ON
> >       select HAS_MEM_PAGING
> > -     select HAS_MEM_SHARING
> >       select HAS_NS16550
> >       select HAS_PASSTHROUGH
> >       select HAS_PCI
> > @@ -198,6 +197,13 @@ config PV_SHIM_EXCLUSIVE
> >         firmware, and will not function correctly in other scenarios.
> >
> >         If unsure, say N.
> > +
> > +config MEM_SHARING
> > +    bool
> > +    default n
> > +    depends on HVM
> > +    prompt "Xen memory sharing support" if EXPERT = "y"
> > +
>
> As per all the context above, please use proper (tab) indentation.
> Also please omit the pointless "default n". And then the "bool"
> and "prompt ..." can (and hence should) be combined into a
> single line.

Sure.

>
> I also wonder whether this shouldn't be in common/Kconfig, but
> then again it can be moved there if Arm would ever gain
> mem-sharing support.

It is currently only supported for x86 HVM guests. There are no plans
for adding ARM support. If and when that happens, it could be moved to
common. I don't expect that will ever happen.

>
> > @@ -98,4 +100,33 @@ void mem_sharing_init(void);
> >   */
> >  int relinquish_shared_pages(struct domain *d);
> >
> > +#else
> > +
> > +static inline unsigned int mem_sharing_get_nr_saved_mfns(void)
> > +{
> > +    return 0;
> > +}
> > +static inline unsigned int mem_sharing_get_nr_shared_mfns(void)
> > +{
> > +    return 0;
> > +}
>
> I follow you for these.
>
> > +static inline int mem_sharing_unshare_page(struct domain *d,
> > +                                           unsigned long gfn,
> > +                                           uint16_t flags)
> > +{
> > +    return 0;
> > +}
>
> But shouldn't this one (just as an example, there may be more
> below) return -EOPNOTSUPP?

It really doesn't matter. No caller ever reaches this function when
mem_sharing is compiled out since it's gated on the page type being
p2m_ram_shared. You can't set that page type when the memop/domctl to
set it is gone.

>
> And in general - if these inline functions are needed, shouldn't
> all of them (or at least some) gain ASSERT_UNREACHABLE(), as we
> do elsewhere?

Yes, since unshare_page is unreachable adding that assert would be
appropriate. It would probably be appropriate for the others too
(except get_nr_saved/shared_mfns).

Tamas
diff mbox series

Patch

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 4b8b07b549..af7c25543f 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -17,7 +17,6 @@  config X86
 	select HAS_KEXEC
 	select MEM_ACCESS_ALWAYS_ON
 	select HAS_MEM_PAGING
-	select HAS_MEM_SHARING
 	select HAS_NS16550
 	select HAS_PASSTHROUGH
 	select HAS_PCI
@@ -198,6 +197,13 @@  config PV_SHIM_EXCLUSIVE
 	  firmware, and will not function correctly in other scenarios.
 
 	  If unsure, say N.
+
+config MEM_SHARING
+    bool
+    default n
+    depends on HVM
+    prompt "Xen memory sharing support" if EXPERT = "y"
+
 endmenu
 
 source "common/Kconfig"
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 9bf2d0820f..bc9e024ccc 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1231,9 +1231,11 @@  long arch_do_domctl(
         break;
     }
 
+#ifdef CONFIG_MEM_SHARING
     case XEN_DOMCTL_mem_sharing_op:
         ret = mem_sharing_domctl(d, &domctl->u.mem_sharing_op);
         break;
+#endif
 
 #if P2M_AUDIT && defined(CONFIG_HVM)
     case XEN_DOMCTL_audit_p2m:
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index c2c92a96ac..c3b0f3115c 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2030,7 +2030,7 @@  static inline bool current_locked_page_ne_check(struct page_info *page) {
 #define current_locked_page_ne_check(x) true
 #endif
 
-#if defined(CONFIG_PV) || defined(CONFIG_HAS_MEM_SHARING)
+#if defined(CONFIG_PV) || defined(CONFIG_MEM_SHARING)
 static int _page_lock(struct page_info *page)
 {
     unsigned long x, nx;
@@ -2063,7 +2063,7 @@  static void _page_unlock(struct page_info *page)
 }
 #endif
 
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
 int page_lock_memshr(struct page_info *page)
 {
     return _page_lock(page);
diff --git a/xen/arch/x86/mm/Makefile b/xen/arch/x86/mm/Makefile
index 5a17646f98..5010a29d6c 100644
--- a/xen/arch/x86/mm/Makefile
+++ b/xen/arch/x86/mm/Makefile
@@ -6,7 +6,7 @@  obj-$(CONFIG_HVM) += guest_walk_2.o guest_walk_3.o guest_walk_4.o
 obj-$(CONFIG_SHADOW_PAGING) += guest_walk_2.o guest_walk_3.o guest_walk_4.o
 obj-$(CONFIG_MEM_ACCESS) += mem_access.o
 obj-y += mem_paging.o
-obj-y += mem_sharing.o
+obj-$(CONFIG_MEM_SHARING) += mem_sharing.o
 obj-y += p2m.o p2m-pt.o
 obj-$(CONFIG_HVM) += p2m-ept.o p2m-pod.o
 obj-y += paging.o
diff --git a/xen/arch/x86/x86_64/compat/mm.c b/xen/arch/x86/x86_64/compat/mm.c
index 32410ed273..d4c6be3032 100644
--- a/xen/arch/x86/x86_64/compat/mm.c
+++ b/xen/arch/x86/x86_64/compat/mm.c
@@ -152,8 +152,10 @@  int compat_arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     case XENMEM_paging_op:
         return mem_paging_memop(guest_handle_cast(arg, xen_mem_paging_op_t));
 
+#ifdef CONFIG_MEM_SHARING
     case XENMEM_sharing_op:
         return mem_sharing_memop(guest_handle_cast(arg, xen_mem_sharing_op_t));
+#endif
 
     default:
         rc = -ENOSYS;
diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index d8f558bc3a..51d1d511f2 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -993,8 +993,10 @@  long subarch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     case XENMEM_paging_op:
         return mem_paging_memop(guest_handle_cast(arg, xen_mem_paging_op_t));
 
+#ifdef CONFIG_MEM_SHARING
     case XENMEM_sharing_op:
         return mem_sharing_memop(guest_handle_cast(arg, xen_mem_sharing_op_t));
+#endif
 
     default:
         rc = -ENOSYS;
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index c838506241..80575cac10 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -45,9 +45,6 @@  config MEM_ACCESS
 config HAS_MEM_PAGING
 	bool
 
-config HAS_MEM_SHARING
-	bool
-
 config HAS_PDX
 	bool
 
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 88bbe984bc..bb072cf93f 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -926,7 +926,7 @@  static void complete_domain_destroy(struct rcu_head *head)
     xfree(d->vm_event_paging);
 #endif
     xfree(d->vm_event_monitor);
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
     xfree(d->vm_event_share);
 #endif
 
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 80728ea57d..6c40dccae9 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -3760,7 +3760,7 @@  void grant_table_init_vcpu(struct vcpu *v)
     v->maptrack_tail = MAPTRACK_TAIL;
 }
 
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
 int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref,
                             gfn_t *gfn, uint16_t *status)
 {
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 86567e6117..915f2cee1a 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -1676,7 +1676,7 @@  int check_get_page_from_gfn(struct domain *d, gfn_t gfn, bool readonly,
         return -EAGAIN;
     }
 #endif
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
     if ( (q & P2M_UNSHARE) && p2m_is_shared(p2mt) )
     {
         if ( page )
diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
index 6e68be47bc..163a671cea 100644
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -544,7 +544,7 @@  static void monitor_notification(struct vcpu *v, unsigned int port)
     vm_event_resume(v->domain, v->domain->vm_event_monitor);
 }
 
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
 /* Registered with Xen-bound event channel for incoming notifications. */
 static void mem_sharing_notification(struct vcpu *v, unsigned int port)
 {
@@ -574,7 +574,7 @@  void vm_event_cleanup(struct domain *d)
         destroy_waitqueue_head(&d->vm_event_monitor->wq);
         (void)vm_event_disable(d, &d->vm_event_monitor);
     }
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
     if ( vm_event_check_ring(d->vm_event_share) )
     {
         destroy_waitqueue_head(&d->vm_event_share->wq);
@@ -720,7 +720,7 @@  int vm_event_domctl(struct domain *d, struct xen_domctl_vm_event_op *vec,
     }
     break;
 
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
     case XEN_DOMCTL_VM_EVENT_OP_SHARING:
     {
         rc = -EINVAL;
diff --git a/xen/include/asm-x86/mem_sharing.h b/xen/include/asm-x86/mem_sharing.h
index 52ea91efa0..e0f8792f52 100644
--- a/xen/include/asm-x86/mem_sharing.h
+++ b/xen/include/asm-x86/mem_sharing.h
@@ -24,6 +24,8 @@ 
 #include <public/domctl.h>
 #include <public/memory.h>
 
+#ifdef CONFIG_MEM_SHARING
+
 /* Auditing of memory sharing code? */
 #ifndef NDEBUG
 #define MEM_SHARING_AUDIT 1
@@ -98,4 +100,33 @@  void mem_sharing_init(void);
  */
 int relinquish_shared_pages(struct domain *d);
 
+#else
+
+static inline unsigned int mem_sharing_get_nr_saved_mfns(void)
+{
+    return 0;
+}
+static inline unsigned int mem_sharing_get_nr_shared_mfns(void)
+{
+    return 0;
+}
+static inline int mem_sharing_unshare_page(struct domain *d,
+                                           unsigned long gfn,
+                                           uint16_t flags)
+{
+    return 0;
+}
+static inline int mem_sharing_notify_enomem(struct domain *d, unsigned long gfn,
+                              bool allow_sleep)
+{
+    return 0;
+}
+static inline int relinquish_shared_pages(struct domain *d)
+{
+    return 0;
+}
+static inline void mem_sharing_init(void) {}
+
+#endif
+
 #endif /* __MEM_SHARING_H__ */
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index ba49eee24d..a3c754cb2f 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -127,6 +127,8 @@  struct page_info
         /* For non-pinnable single-page shadows, a higher entry that points
          * at us. */
         paddr_t up;
+
+#ifdef CONFIG_MEM_SHARING
         /* For shared/sharable pages, we use a doubly-linked list
          * of all the {pfn,domain} pairs that map this page. We also include
          * an opaque handle, which is effectively a version, so that clients
@@ -134,6 +136,7 @@  struct page_info
          * This list is allocated and freed when a page is shared/unshared.
          */
         struct page_sharing_info *sharing;
+#endif
     };
 
     /* Reference count and various PGC_xxx flags and fields. */
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 748bb0f2f9..17cf8785fb 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -462,7 +462,7 @@  struct domain
     /* Various vm_events */
 
     /* Memory sharing support */
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
     struct vm_event_domain *vm_event_share;
 #endif
     /* Memory paging support */
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index e628b1c6af..8afdec9fe8 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -603,7 +603,7 @@  static XSM_INLINE int xsm_mem_paging(XSM_DEFAULT_ARG struct domain *d)
 }
 #endif
 
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
 static XSM_INLINE int xsm_mem_sharing(XSM_DEFAULT_ARG struct domain *d)
 {
     XSM_ASSERT_ACTION(XSM_DM_PRIV);
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 8a78d8abd3..8ec6b1a6e8 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -151,7 +151,7 @@  struct xsm_operations {
     int (*mem_paging) (struct domain *d);
 #endif
 
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
     int (*mem_sharing) (struct domain *d);
 #endif
 
@@ -603,7 +603,7 @@  static inline int xsm_mem_paging (xsm_default_t def, struct domain *d)
 }
 #endif
 
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
 static inline int xsm_mem_sharing (xsm_default_t def, struct domain *d)
 {
     return xsm_ops->mem_sharing(d);
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index 1fe0e746fa..6158dce814 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -129,7 +129,7 @@  void __init xsm_fixup_ops (struct xsm_operations *ops)
     set_to_dummy_if_null(ops, mem_paging);
 #endif
 
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
     set_to_dummy_if_null(ops, mem_sharing);
 #endif
 
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 3d00c747f6..f5f3b42e6e 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1270,7 +1270,7 @@  static int flask_mem_paging(struct domain *d)
 }
 #endif
 
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
 static int flask_mem_sharing(struct domain *d)
 {
     return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__MEM_SHARING);
@@ -1838,7 +1838,7 @@  static struct xsm_operations flask_ops = {
     .mem_paging = flask_mem_paging,
 #endif
 
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
     .mem_sharing = flask_mem_sharing,
 #endif