[PATCH 0/6] gnttab: add per-domain controls

[PATCH 0/6] gnttab: add per-domain controls

[Roger Pau Monne]

Hello,

The first two patches of this series allows setting the preisoutly host
wide command line `gnttab` option on a per domain basis. That means
selecting the max allowed grant table version and whether transitive
grants are allowed.

The last 4 patches attempt to implement support for creating guests
without grant table support at all. This requires some changes to
xenstore in order to map shared ring using foreign memory instead of
grant table.

Note that patch 5 will break the save format for xenstore records, and
should not be applied.

Thanks, Roger.

Roger Pau Monne (6):
  gnttab: allow setting max version per-domain
  grant: allow per-domain control over transitive grants
  tools/console: use xenforeigmemory to map console ring
  tools/xenstored: use atexit to close interfaces
  tools/xenstored: restore support for mapping ring as foreign memory
  gnttab: allow disabling grant table per-domain

 docs/man/xl.cfg.5.pod.in                |  12 +++
 docs/man/xl.conf.5.pod.in               |  14 +++
 tools/console/Makefile                  |   4 +-
 tools/console/daemon/io.c               |  25 ++++-
 tools/helpers/init-xenstore-domain.c    |   1 +
 tools/include/libxl.h                   |  14 +++
 tools/libs/light/libxl_create.c         |   4 +
 tools/libs/light/libxl_dm.c             |   2 +
 tools/libs/light/libxl_dom.c            |   2 +-
 tools/libs/light/libxl_types.idl        |   2 +
 tools/ocaml/libs/xc/xenctrl.ml          |   5 +
 tools/ocaml/libs/xc/xenctrl.mli         |   5 +
 tools/ocaml/libs/xc/xenctrl_stubs.c     |  12 ++-
 tools/xenstore/Makefile                 |   4 +-
 tools/xenstore/include/xenstore_state.h |   1 +
 tools/xenstore/xenstored_core.h         |   2 +-
 tools/xenstore/xenstored_domain.c       | 120 +++++++++++++++---------
 tools/xl/xl.c                           |  15 +++
 tools/xl/xl.h                           |   2 +
 tools/xl/xl_parse.c                     |  13 +++
 xen/arch/arm/domain_build.c             |   4 +
 xen/arch/x86/setup.c                    |   2 +
 xen/common/domain.c                     |   4 +-
 xen/common/grant_table.c                | 119 ++++++++++++++++++++++-
 xen/include/public/domctl.h             |  13 ++-
 xen/include/xen/grant_table.h           |   7 +-
 26 files changed, 341 insertions(+), 67 deletions(-)

-- 
2.33.0

[PATCH 1/6] gnttab: allow setting max version per-domain

[Roger Pau Monne]

Introduce a new domain create field so that toolstack can specify the
maximum grant table version usable by the domain. This is plumbed into
xl and settable by the user as max_grant_version.

Previously this was only settable on a per host basis using the
gnttab command line option.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
NB: the stubdom max grant version is cloned from the domain one. Not
sure whether long term we might want to use different options for the
stubdom and the domain. In any case the attack surface will always be
max(stubdom, domain), so maybe it's just pointless to allow more fine
setting.
---
 docs/man/xl.cfg.5.pod.in             |  6 ++++++
 docs/man/xl.conf.5.pod.in            |  7 +++++++
 tools/helpers/init-xenstore-domain.c |  1 +
 tools/include/libxl.h                |  7 +++++++
 tools/libs/light/libxl_create.c      |  1 +
 tools/libs/light/libxl_dm.c          |  1 +
 tools/libs/light/libxl_types.idl     |  1 +
 tools/ocaml/libs/xc/xenctrl.ml       |  1 +
 tools/ocaml/libs/xc/xenctrl.mli      |  1 +
 tools/ocaml/libs/xc/xenctrl_stubs.c  |  5 ++++-
 tools/xl/xl.c                        |  8 ++++++++
 tools/xl/xl.h                        |  1 +
 tools/xl/xl_parse.c                  |  9 +++++++++
 xen/arch/arm/domain_build.c          |  2 ++
 xen/arch/x86/setup.c                 |  1 +
 xen/common/domain.c                  |  3 ++-
 xen/common/grant_table.c             | 21 +++++++++++++++++++--
 xen/include/public/domctl.h          |  5 +++--
 xen/include/xen/grant_table.h        |  5 +++--
 19 files changed, 78 insertions(+), 8 deletions(-)

diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index 4b1e3028d2..a4bf2caafa 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -580,6 +580,12 @@ to have. This value controls how many pages of foreign domains can be accessed
 via the grant mechanism by this domain. The default value is settable via
 L<xl.conf(5)>.
 
+=item B<max_grant_version=NUMBER>
+
+Specify the maximum grant table version the domain is allowed to use. Current
+supported versions are 1 and 2. The default value is settable via
+L<xl.conf(5)>.
+
 =item B<nomigrate=BOOLEAN>
 
 Disable migration of this domain.  This enables certain other features
diff --git a/docs/man/xl.conf.5.pod.in b/docs/man/xl.conf.5.pod.in
index b48e99131a..0a70698a7c 100644
--- a/docs/man/xl.conf.5.pod.in
+++ b/docs/man/xl.conf.5.pod.in
@@ -101,6 +101,13 @@ Sets the default value for the C<max_maptrack_frames> domain config value.
 Default: value of Xen command line B<gnttab_max_maptrack_frames>
 parameter (or its default value if unspecified).
 
+=item B<max_grant_version=NUMBER>
+
+Sets the default value for the C<max_grant_version> domain config value.
+
+Default: value of Xen command line B<gnttab> parameter (or its default value if
+unspecified).
+
 =item B<vif.default.script="PATH">
 
 Configures the default hotplug script used by virtual network devices.
diff --git a/tools/helpers/init-xenstore-domain.c b/tools/helpers/init-xenstore-domain.c
index 6836002f0b..dae2b68f27 100644
--- a/tools/helpers/init-xenstore-domain.c
+++ b/tools/helpers/init-xenstore-domain.c
@@ -88,6 +88,7 @@ static int build(xc_interface *xch)
          */
         .max_grant_frames = 4,
         .max_maptrack_frames = 128,
+        .max_grant_version = 1,
     };
 
     xs_fd = open("/dev/xen/xenbus_backend", O_RDWR);
diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index b9ba16d698..7a35833312 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -502,6 +502,13 @@
  */
 #define LIBXL_HAVE_X86_MSR_RELAXED 1
 
+/*
+ * LIBXL_HAVE_MAX_GRANT_VERSION indicates libxl_domain_build_info has a
+ * max_grant_version field for setting the max grant table version per
+ * domain.
+ */
+#define LIBXL_HAVE_MAX_GRANT_VERSION 1
+
 /*
  * libxl ABI compatibility
  *
diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index e356b2106d..69b5419416 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -606,6 +606,7 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
             .max_evtchn_port = b_info->event_channels,
             .max_grant_frames = b_info->max_grant_frames,
             .max_maptrack_frames = b_info->max_maptrack_frames,
+            .max_grant_version = b_info->max_grant_version,
             .vmtrace_size = ROUNDUP(b_info->vmtrace_buf_kb << 10, XC_PAGE_SHIFT),
         };
 
diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
index 9d93056b5c..9a8ddbe188 100644
--- a/tools/libs/light/libxl_dm.c
+++ b/tools/libs/light/libxl_dm.c
@@ -2320,6 +2320,7 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
 
     dm_config->b_info.max_grant_frames = guest_config->b_info.max_grant_frames;
     dm_config->b_info.max_maptrack_frames = guest_config->b_info.max_maptrack_frames;
+    dm_config->b_info.max_grant_version = guest_config->b_info.max_grant_version;
 
     dm_config->b_info.u.pv.features = "";
 
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 3f9fff653a..37789a568c 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -518,6 +518,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
 
     ("max_grant_frames",    uint32, {'init_val': 'LIBXL_MAX_GRANT_DEFAULT'}),
     ("max_maptrack_frames", uint32, {'init_val': 'LIBXL_MAX_GRANT_DEFAULT'}),
+    ("max_grant_version",   integer, {'init_val': '-1'}),
     
     ("device_model_version", libxl_device_model_version),
     ("device_model_stubdomain", libxl_defbool),
diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index a5588c643f..6a8658bfec 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -83,6 +83,7 @@ type domctl_create_config =
 	max_evtchn_port: int;
 	max_grant_frames: int;
 	max_maptrack_frames: int;
+	max_grant_version: int;
 	arch: arch_domainconfig;
 }
 
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index 6e94940a8a..5933d32c70 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -75,6 +75,7 @@ type domctl_create_config = {
   max_evtchn_port: int;
   max_grant_frames: int;
   max_maptrack_frames: int;
+  max_grant_version: int;
   arch: arch_domainconfig;
 }
 
diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index ad953d36bd..aad3c6a726 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -188,7 +188,8 @@ CAMLprim value stub_xc_domain_create(value xch, value wanted_domid, value config
 #define VAL_MAX_EVTCHN_PORT     Field(config, 5)
 #define VAL_MAX_GRANT_FRAMES    Field(config, 6)
 #define VAL_MAX_MAPTRACK_FRAMES Field(config, 7)
-#define VAL_ARCH                Field(config, 8)
+#define VAL_MAX_GRANT_VERSION   Field(config, 8)
+#define VAL_ARCH                Field(config, 9)
 
 	uint32_t domid = Int_val(wanted_domid);
 	int result;
@@ -198,6 +199,7 @@ CAMLprim value stub_xc_domain_create(value xch, value wanted_domid, value config
 		.max_evtchn_port = Int_val(VAL_MAX_EVTCHN_PORT),
 		.max_grant_frames = Int_val(VAL_MAX_GRANT_FRAMES),
 		.max_maptrack_frames = Int_val(VAL_MAX_MAPTRACK_FRAMES),
+		.max_grant_version = Int_val(VAL_MAX_GRANT_VERSION),
 	};
 
 	domain_handle_of_uuid_string(cfg.handle, String_val(VAL_HANDLE));
@@ -251,6 +253,7 @@ CAMLprim value stub_xc_domain_create(value xch, value wanted_domid, value config
 	}
 
 #undef VAL_ARCH
+#undef VAL_MAX_GRANT_VERSION
 #undef VAL_MAX_MAPTRACK_FRAMES
 #undef VAL_MAX_GRANT_FRAMES
 #undef VAL_MAX_EVTCHN_PORT
diff --git a/tools/xl/xl.c b/tools/xl/xl.c
index 4107d10fd4..0fde879cf4 100644
--- a/tools/xl/xl.c
+++ b/tools/xl/xl.c
@@ -55,6 +55,7 @@ bool progress_use_cr = 0;
 bool timestamps = 0;
 int max_grant_frames = -1;
 int max_maptrack_frames = -1;
+int max_grant_version = -1;
 libxl_domid domid_policy = INVALID_DOMID;
 
 xentoollog_level minmsglevel = minmsglevel_default;
@@ -213,6 +214,13 @@ static void parse_global_config(const char *configfile,
     else if (e != ESRCH)
         exit(1);
 
+    e = xlu_cfg_get_bounded_long (config, "max_grant_version", 0,
+                                  INT_MAX, &l, 1);
+    if (!e)
+        max_grant_version = l;
+    else if (e != ESRCH)
+        exit(1);
+
     libxl_cpu_bitmap_alloc(ctx, &global_vm_affinity_mask, 0);
     libxl_cpu_bitmap_alloc(ctx, &global_hvm_affinity_mask, 0);
     libxl_cpu_bitmap_alloc(ctx, &global_pv_affinity_mask, 0);
diff --git a/tools/xl/xl.h b/tools/xl/xl.h
index 7e23f30192..cf12c79a9b 100644
--- a/tools/xl/xl.h
+++ b/tools/xl/xl.h
@@ -282,6 +282,7 @@ extern char *default_colo_proxy_script;
 extern char *blkdev_start;
 extern int max_grant_frames;
 extern int max_maptrack_frames;
+extern int max_grant_version;
 extern libxl_bitmap global_vm_affinity_mask;
 extern libxl_bitmap global_hvm_affinity_mask;
 extern libxl_bitmap global_pv_affinity_mask;
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 17dddb4cd5..1206d7ea51 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1431,6 +1431,15 @@ void parse_config_data(const char *config_source,
     else
         exit(1);
 
+    e = xlu_cfg_get_bounded_long (config, "max_grant_version", 0,
+                                  INT_MAX, &l, 1);
+    if (e == ESRCH) /* not specified */
+        b_info->max_grant_version = max_grant_version;
+    else if (!e)
+        b_info->max_grant_version = l;
+    else
+        exit(1);
+
     libxl_defbool_set(&b_info->claim_mode, claim_mode);
 
     if (xlu_cfg_get_string (config, "on_poweroff", &buf, 0))
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 206038d1c0..e9a34f2f8d 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2484,6 +2484,7 @@ void __init create_domUs(void)
             .max_evtchn_port = -1,
             .max_grant_frames = -1,
             .max_maptrack_frames = -1,
+            .max_grant_version = -1,
         };
 
         if ( !dt_device_is_compatible(node, "xen,domain") )
@@ -2591,6 +2592,7 @@ void __init create_dom0(void)
         .max_evtchn_port = -1,
         .max_grant_frames = gnttab_dom0_frames(),
         .max_maptrack_frames = -1,
+        .max_grant_version = -1,
     };
 
     /* The vGIC for DOM0 is exactly emulating the hardware GIC */
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 8105dc36bb..af69e20029 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -750,6 +750,7 @@ static struct domain *__init create_dom0(const module_t *image,
         .max_evtchn_port = -1,
         .max_grant_frames = -1,
         .max_maptrack_frames = -1,
+        .max_grant_version = -1,
         .max_vcpus = dom0_max_vcpus(),
         .arch = {
             .misc_flags = opt_dom0_msr_relaxed ? XEN_X86_MSR_RELAXED : 0,
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 6ee5d033b0..0c85d89419 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -669,7 +669,8 @@ struct domain *domain_create(domid_t domid,
         init_status |= INIT_evtchn;
 
         if ( (err = grant_table_init(d, config->max_grant_frames,
-                                     config->max_maptrack_frames)) != 0 )
+                                     config->max_maptrack_frames,
+                                     config->max_grant_version)) != 0 )
             goto fail;
         init_status |= INIT_gnttab;
 
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index e80f8d044d..d01c6813d1 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -53,6 +53,7 @@ struct grant_table {
     percpu_rwlock_t       lock;
     /* Lock protecting the maptrack limit */
     spinlock_t            maptrack_lock;
+    unsigned int          max_grant_version;
     /*
      * Defaults to v1.  May be changed with GNTTABOP_set_version.  All other
      * values are invalid.
@@ -1917,11 +1918,26 @@ active_alloc_failed:
 }
 
 int grant_table_init(struct domain *d, int max_grant_frames,
-                     int max_maptrack_frames)
+                     int max_maptrack_frames, int max_grant_version)
 {
     struct grant_table *gt;
     int ret = -ENOMEM;
 
+    if ( max_grant_version < 0 )
+        max_grant_version = opt_gnttab_max_version;
+    if ( !max_grant_version )
+    {
+        dprintk(XENLOG_INFO, "Invalid grant table version 0 requested\n");
+        return -EINVAL;
+    }
+    if ( max_grant_version > opt_gnttab_max_version )
+    {
+        dprintk(XENLOG_INFO,
+                "Requested grant version (%u) greater than supported (%u)\n",
+                max_grant_version, opt_gnttab_max_version);
+        return -EINVAL;
+    }
+
     /* Default to maximum value if no value was specified */
     if ( max_grant_frames < 0 )
         max_grant_frames = opt_max_grant_frames;
@@ -1947,6 +1963,7 @@ int grant_table_init(struct domain *d, int max_grant_frames,
     gt->gt_version = 1;
     gt->max_grant_frames = max_grant_frames;
     gt->max_maptrack_frames = max_maptrack_frames;
+    gt->max_grant_version = max_grant_version;
 
     /* Install the structure early to simplify the error path. */
     gt->domain = d;
@@ -3076,7 +3093,7 @@ gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)
         goto out;
 
     res = -ENOSYS;
-    if ( op.version == 2 && opt_gnttab_max_version == 1 )
+    if ( op.version == 2 && gt->max_grant_version == 1 )
         goto out; /* Behave as before set_version was introduced. */
 
     res = 0;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 96696e3842..7f8456c50e 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -87,13 +87,14 @@ struct xen_domctl_createdomain {
     /*
      * Various domain limits, which impact the quantity of resources
      * (global mapping space, xenheap, etc) a guest may consume.  For
-     * max_grant_frames and max_maptrack_frames, < 0 means "use the
-     * default maximum value in the hypervisor".
+     * max_grant_frames, max_maptrack_frames and max_gnttab_version < 0
+     * means "use the default maximum value in the hypervisor".
      */
     uint32_t max_vcpus;
     uint32_t max_evtchn_port;
     int32_t max_grant_frames;
     int32_t max_maptrack_frames;
+    int32_t max_grant_version;
 
     /* Per-vCPU buffer size in bytes.  0 to disable. */
     uint32_t vmtrace_size;
diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index 41713e2726..d6da067fc1 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -36,7 +36,7 @@ extern unsigned int opt_max_grant_frames;
 
 /* Create/destroy per-domain grant table context. */
 int grant_table_init(struct domain *d, int max_grant_frames,
-                     int max_maptrack_frames);
+                     int max_maptrack_frames, int max_grant_version);
 void grant_table_destroy(
     struct domain *d);
 void grant_table_init_vcpu(struct vcpu *v);
@@ -67,7 +67,8 @@ int gnttab_acquire_resource(
 
 static inline int grant_table_init(struct domain *d,
                                    int max_grant_frames,
-                                   int max_maptrack_frames)
+                                   int max_maptrack_frames,
+                                   int max_grant_version)
 {
     return 0;
 }
-- 
2.33.0

[PATCH 2/6] gnttab: allow per-domain control over transitive grants

[Roger Pau Monne]

Introduce a new grant options flags field in domain create and use it
to signal whether transitive grants are allowed on the domain. This is
settable from xl using the transitive_grants option.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 docs/man/xl.cfg.5.pod.in            |  6 ++++++
 docs/man/xl.conf.5.pod.in           |  7 +++++++
 tools/include/libxl.h               |  7 +++++++
 tools/libs/light/libxl_create.c     |  3 +++
 tools/libs/light/libxl_dm.c         |  1 +
 tools/libs/light/libxl_types.idl    |  1 +
 tools/ocaml/libs/xc/xenctrl.ml      |  4 ++++
 tools/ocaml/libs/xc/xenctrl.mli     |  4 ++++
 tools/ocaml/libs/xc/xenctrl_stubs.c |  9 ++++++++-
 tools/xl/xl.c                       |  7 +++++++
 tools/xl/xl.h                       |  1 +
 tools/xl/xl_parse.c                 |  4 ++++
 xen/arch/arm/domain_build.c         |  2 ++
 xen/arch/x86/setup.c                |  1 +
 xen/common/domain.c                 |  3 ++-
 xen/common/grant_table.c            | 11 +++++++++--
 xen/include/public/domctl.h         |  8 ++++++++
 xen/include/xen/grant_table.h       |  6 ++++--
 18 files changed, 79 insertions(+), 6 deletions(-)

diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index a4bf2caafa..c5a447dfcd 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -586,6 +586,12 @@ Specify the maximum grant table version the domain is allowed to use. Current
 supported versions are 1 and 2. The default value is settable via
 L<xl.conf(5)>.
 
+=item B<transitive_grants=BOOLEAN>
+
+Specify whether transitive grants should be available to the domain. Note such
+functionality only applies to grant table version 2. The default value is
+settable via L<xl.conf(5)>.
+
 =item B<nomigrate=BOOLEAN>
 
 Disable migration of this domain.  This enables certain other features
diff --git a/docs/man/xl.conf.5.pod.in b/docs/man/xl.conf.5.pod.in
index 0a70698a7c..88efbee5fe 100644
--- a/docs/man/xl.conf.5.pod.in
+++ b/docs/man/xl.conf.5.pod.in
@@ -108,6 +108,13 @@ Sets the default value for the C<max_grant_version> domain config value.
 Default: value of Xen command line B<gnttab> parameter (or its default value if
 unspecified).
 
+=item B<transitive_grants=BOOLEAN>
+
+Sets the default value for the C<transitive_grants> domain config value.
+
+Default: value of Xen command line B<gnttab> parameter (or its default value if
+unspecified).
+
 =item B<vif.default.script="PATH">
 
 Configures the default hotplug script used by virtual network devices.
diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index 7a35833312..d23586f2cc 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -509,6 +509,13 @@
  */
 #define LIBXL_HAVE_MAX_GRANT_VERSION 1
 
+/*
+ * LIBXL_HAVE_TRANSITIVE_GRANTS indicates libxl_domain_build_info has a
+ * transitive_grants field for setting whether such functionality should be
+ * available to the domain.
+ */
+#define LIBXL_HAVE_TRANSITIVE_GRANTS 1
+
 /*
  * libxl ABI compatibility
  *
diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index 69b5419416..61d65d1342 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -633,6 +633,9 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
         if (info->passthrough == LIBXL_PASSTHROUGH_SYNC_PT)
             create.iommu_opts |= XEN_DOMCTL_IOMMU_no_sharept;
 
+        if (libxl_defbool_val(b_info->transitive_grants))
+            create.grant_opts |= XEN_DOMCTL_GRANT_transitive;
+
         /* Ultimately, handle is an array of 16 uint8_t, same as uuid */
         libxl_uuid_copy(ctx, (libxl_uuid *)&create.handle, &info->uuid);
 
diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
index 9a8ddbe188..4ade437257 100644
--- a/tools/libs/light/libxl_dm.c
+++ b/tools/libs/light/libxl_dm.c
@@ -2321,6 +2321,7 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
     dm_config->b_info.max_grant_frames = guest_config->b_info.max_grant_frames;
     dm_config->b_info.max_maptrack_frames = guest_config->b_info.max_maptrack_frames;
     dm_config->b_info.max_grant_version = guest_config->b_info.max_grant_version;
+    dm_config->b_info.transitive_grants = guest_config->b_info.transitive_grants;
 
     dm_config->b_info.u.pv.features = "";
 
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 37789a568c..d05b5d2abc 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -519,6 +519,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
     ("max_grant_frames",    uint32, {'init_val': 'LIBXL_MAX_GRANT_DEFAULT'}),
     ("max_maptrack_frames", uint32, {'init_val': 'LIBXL_MAX_GRANT_DEFAULT'}),
     ("max_grant_version",   integer, {'init_val': '-1'}),
+    ("transitive_grants",   libxl_defbool),
     
     ("device_model_version", libxl_device_model_version),
     ("device_model_stubdomain", libxl_defbool),
diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index 6a8658bfec..ec7a296776 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -73,6 +73,9 @@ type domain_create_flag =
 type domain_create_iommu_opts =
 	| IOMMU_NO_SHAREPT
 
+type domain_create_grant_opts =
+	| GRANT_TRANSITIVE
+
 type domctl_create_config =
 {
 	ssidref: int32;
@@ -84,6 +87,7 @@ type domctl_create_config =
 	max_grant_frames: int;
 	max_maptrack_frames: int;
 	max_grant_version: int;
+	grant_opts: domain_create_grant_opts list;
 	arch: arch_domainconfig;
 }
 
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index 5933d32c70..e47fd1947f 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -66,6 +66,9 @@ type domain_create_flag =
 type domain_create_iommu_opts =
   | IOMMU_NO_SHAREPT
 
+type domain_create_grant_opts =
+  | GRANT_TRANSITIVE
+
 type domctl_create_config = {
   ssidref: int32;
   handle: string;
@@ -76,6 +79,7 @@ type domctl_create_config = {
   max_grant_frames: int;
   max_maptrack_frames: int;
   max_grant_version: int;
+  grant_opts: domain_create_grant_opts list;
   arch: arch_domainconfig;
 }
 
diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index aad3c6a726..772704759d 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -189,7 +189,8 @@ CAMLprim value stub_xc_domain_create(value xch, value wanted_domid, value config
 #define VAL_MAX_GRANT_FRAMES    Field(config, 6)
 #define VAL_MAX_MAPTRACK_FRAMES Field(config, 7)
 #define VAL_MAX_GRANT_VERSION   Field(config, 8)
-#define VAL_ARCH                Field(config, 9)
+#define VAL_GRANT_OPTS          Field(config, 9)
+#define VAL_ARCH                Field(config, 10)
 
 	uint32_t domid = Int_val(wanted_domid);
 	int result;
@@ -214,6 +215,11 @@ CAMLprim value stub_xc_domain_create(value xch, value wanted_domid, value config
 		/* ! XEN_DOMCTL_IOMMU_ XEN_DOMCTL_IOMMU_MAX max */
 		(VAL_IOMMU_OPTS);
 
+	cfg.grant_opts = ocaml_list_to_c_bitmap
+		/* ! domain_create_grant_opts GRANT_ lc */
+		/* ! XEN_DOMCTL_GRANT_ XEN_DOMCTL_GRANT_MAX max */
+		(VAL_GRANT_OPTS);
+
 	arch_domconfig = Field(VAL_ARCH, 0);
 	switch ( Tag_val(VAL_ARCH) )
 	{
@@ -253,6 +259,7 @@ CAMLprim value stub_xc_domain_create(value xch, value wanted_domid, value config
 	}
 
 #undef VAL_ARCH
+#undef VAL_GRANT_OPTS
 #undef VAL_MAX_GRANT_VERSION
 #undef VAL_MAX_MAPTRACK_FRAMES
 #undef VAL_MAX_GRANT_FRAMES
diff --git a/tools/xl/xl.c b/tools/xl/xl.c
index 0fde879cf4..9bd398f8c9 100644
--- a/tools/xl/xl.c
+++ b/tools/xl/xl.c
@@ -56,6 +56,7 @@ bool timestamps = 0;
 int max_grant_frames = -1;
 int max_maptrack_frames = -1;
 int max_grant_version = -1;
+bool transitive_grants = true;
 libxl_domid domid_policy = INVALID_DOMID;
 
 xentoollog_level minmsglevel = minmsglevel_default;
@@ -221,6 +222,12 @@ static void parse_global_config(const char *configfile,
     else if (e != ESRCH)
         exit(1);
 
+    e = xlu_cfg_get_long (config, "transitive_grants", &l, 0);
+    if (!e)
+        transitive_grants = l;
+    else if (e != ESRCH)
+        exit(1);
+
     libxl_cpu_bitmap_alloc(ctx, &global_vm_affinity_mask, 0);
     libxl_cpu_bitmap_alloc(ctx, &global_hvm_affinity_mask, 0);
     libxl_cpu_bitmap_alloc(ctx, &global_pv_affinity_mask, 0);
diff --git a/tools/xl/xl.h b/tools/xl/xl.h
index cf12c79a9b..d7f83c9abd 100644
--- a/tools/xl/xl.h
+++ b/tools/xl/xl.h
@@ -283,6 +283,7 @@ extern char *blkdev_start;
 extern int max_grant_frames;
 extern int max_maptrack_frames;
 extern int max_grant_version;
+extern bool transitive_grants;
 extern libxl_bitmap global_vm_affinity_mask;
 extern libxl_bitmap global_hvm_affinity_mask;
 extern libxl_bitmap global_pv_affinity_mask;
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 1206d7ea51..8e4e561df8 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1440,6 +1440,10 @@ void parse_config_data(const char *config_source,
     else
         exit(1);
 
+    xlu_cfg_get_defbool(config, "transitive_grants", &b_info->transitive_grants,
+                        0);
+    libxl_defbool_setdefault(&b_info->transitive_grants, transitive_grants);
+
     libxl_defbool_set(&b_info->claim_mode, claim_mode);
 
     if (xlu_cfg_get_string (config, "on_poweroff", &buf, 0))
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index e9a34f2f8d..b4763b5ec6 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2485,6 +2485,7 @@ void __init create_domUs(void)
             .max_grant_frames = -1,
             .max_maptrack_frames = -1,
             .max_grant_version = -1,
+            .grant_opts = XEN_DOMCTL_GRANT_transitive,
         };
 
         if ( !dt_device_is_compatible(node, "xen,domain") )
@@ -2593,6 +2594,7 @@ void __init create_dom0(void)
         .max_grant_frames = gnttab_dom0_frames(),
         .max_maptrack_frames = -1,
         .max_grant_version = -1,
+        .grant_opts = XEN_DOMCTL_GRANT_transitive,
     };
 
     /* The vGIC for DOM0 is exactly emulating the hardware GIC */
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index af69e20029..c743a88592 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -751,6 +751,7 @@ static struct domain *__init create_dom0(const module_t *image,
         .max_grant_frames = -1,
         .max_maptrack_frames = -1,
         .max_grant_version = -1,
+        .grant_opts = XEN_DOMCTL_GRANT_transitive,
         .max_vcpus = dom0_max_vcpus(),
         .arch = {
             .misc_flags = opt_dom0_msr_relaxed ? XEN_X86_MSR_RELAXED : 0,
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 0c85d89419..ab16c422f7 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -670,7 +670,8 @@ struct domain *domain_create(domid_t domid,
 
         if ( (err = grant_table_init(d, config->max_grant_frames,
                                      config->max_maptrack_frames,
-                                     config->max_grant_version)) != 0 )
+                                     config->max_grant_version,
+                                     config->grant_opts)) != 0 )
             goto fail;
         init_status |= INIT_gnttab;
 
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index d01c6813d1..280dbc850a 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -74,6 +74,10 @@ struct grant_table {
      * progress.
      */
     unsigned int          maptrack_limit;
+
+    /* Allow transitive grants. Only applies to grant v2. */
+    bool                  allow_transitive:1;
+
     /* Shared grant table (see include/public/grant_table.h). */
     union {
         void **shared_raw;
@@ -1918,7 +1922,8 @@ active_alloc_failed:
 }
 
 int grant_table_init(struct domain *d, int max_grant_frames,
-                     int max_maptrack_frames, int max_grant_version)
+                     int max_maptrack_frames, int max_grant_version,
+                     unsigned int options)
 {
     struct grant_table *gt;
     int ret = -ENOMEM;
@@ -1964,6 +1969,8 @@ int grant_table_init(struct domain *d, int max_grant_frames,
     gt->max_grant_frames = max_grant_frames;
     gt->max_maptrack_frames = max_maptrack_frames;
     gt->max_grant_version = max_grant_version;
+    gt->allow_transitive = !!(options & XEN_DOMCTL_GRANT_transitive) &&
+                           opt_transitive_grants;
 
     /* Install the structure early to simplify the error path. */
     gt->domain = d;
@@ -2886,7 +2893,7 @@ static int gnttab_copy_claim_buf(const struct gnttab_copy *op,
                                     buf->read_only,
                                     &buf->mfn, &buf->page,
                                     &buf->ptr.offset, &buf->len,
-                                    opt_transitive_grants);
+                                    buf->domain->grant_table->allow_transitive);
         if ( rc != GNTST_okay )
             goto out;
         buf->ptr.u.ref = ptr->u.ref;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 7f8456c50e..fe2201fca1 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -96,6 +96,14 @@ struct xen_domctl_createdomain {
     int32_t max_maptrack_frames;
     int32_t max_grant_version;
 
+/* Allow transitive grants. */
+#define _XEN_DOMCTL_GRANT_transitive  0
+#define XEN_DOMCTL_GRANT_transitive   (1U << _XEN_DOMCTL_GRANT_transitive)
+
+#define XEN_DOMCTL_GRANT_MAX XEN_DOMCTL_GRANT_transitive
+
+    uint32_t grant_opts;
+
     /* Per-vCPU buffer size in bytes.  0 to disable. */
     uint32_t vmtrace_size;
 
diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index d6da067fc1..f264b1c3fc 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -36,7 +36,8 @@ extern unsigned int opt_max_grant_frames;
 
 /* Create/destroy per-domain grant table context. */
 int grant_table_init(struct domain *d, int max_grant_frames,
-                     int max_maptrack_frames, int max_grant_version);
+                     int max_maptrack_frames, int max_grant_version,
+                     unsigned int options);
 void grant_table_destroy(
     struct domain *d);
 void grant_table_init_vcpu(struct vcpu *v);
@@ -68,7 +69,8 @@ int gnttab_acquire_resource(
 static inline int grant_table_init(struct domain *d,
                                    int max_grant_frames,
                                    int max_maptrack_frames,
-                                   int max_grant_version)
+                                   int max_grant_version,
+                                   unsigned int options)
 {
     return 0;
 }
-- 
2.33.0

Re: [PATCH 2/6] gnttab: allow per-domain control over transitive grants

[Andrew Cooper]

On 17/09/2021 16:46, Roger Pau Monne wrote:
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 7f8456c50e..fe2201fca1 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -96,6 +96,14 @@ struct xen_domctl_createdomain {
>      int32_t max_maptrack_frames;
>      int32_t max_grant_version;
>  
> +/* Allow transitive grants. */
> +#define _XEN_DOMCTL_GRANT_transitive  0
> +#define XEN_DOMCTL_GRANT_transitive   (1U << _XEN_DOMCTL_GRANT_transitive)

There's no need for bit position variables.

> +
> +#define XEN_DOMCTL_GRANT_MAX XEN_DOMCTL_GRANT_transitive
> +
> +    uint32_t grant_opts;

So far, we've got 3 bits of information, v1, v2 and transitive, and
we're tight on space in the structure with loads more to fit in.

I was thinking grant_flags or equiv to contain these 3 settings, and any
further which might appear.


One thing which is missing however is the enumeration of which settings
are available, and rejection of bad settings.  If v2 is disabled
globally, trying to create a VM with v2 needs to fail.

~Andrew

Re: [PATCH 2/6] gnttab: allow per-domain control over transitive grants

[Roger Pau Monné]

On Mon, Sep 20, 2021 at 10:32:24AM +0100, Andrew Cooper wrote:
> On 17/09/2021 16:46, Roger Pau Monne wrote:
> > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> > index 7f8456c50e..fe2201fca1 100644
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -96,6 +96,14 @@ struct xen_domctl_createdomain {
> >      int32_t max_maptrack_frames;
> >      int32_t max_grant_version;
> >  
> > +/* Allow transitive grants. */
> > +#define _XEN_DOMCTL_GRANT_transitive  0
> > +#define XEN_DOMCTL_GRANT_transitive   (1U << _XEN_DOMCTL_GRANT_transitive)
> 
> There's no need for bit position variables.
> 
> > +
> > +#define XEN_DOMCTL_GRANT_MAX XEN_DOMCTL_GRANT_transitive
> > +
> > +    uint32_t grant_opts;
> 
> So far, we've got 3 bits of information, v1, v2 and transitive, and
> we're tight on space in the structure with loads more to fit in.
> 
> I was thinking grant_flags or equiv to contain these 3 settings, and any
> further which might appear.

What about using something like the below?

We also need to consider selecting the default version (whatever is
set on the hypervisor) and no grant table at all.

/* Grant version, use low 4 bits. */
#define XEN_DOMCTL_GRANT_disable         0
#define XEN_DOMCTL_GRANT_version_v1      1
#define XEN_DOMCTL_GRANT_version_v2      2
#define XEN_DOMCTL_GRANT_version_default 0xf
/* Allow transitive grants. */
#define _XEN_DOMCTL_GRANT_transitive  4
#define XEN_DOMCTL_GRANT_transitive   (1U << _XEN_DOMCTL_GRANT_transitive)

#define XEN_DOMCTLGRANT_MAX XEN_DOMCTL_GRANT_transitive

    uint32_t grant_opts;

> 
> 
> One thing which is missing however is the enumeration of which settings
> are available, and rejection of bad settings.  If v2 is disabled
> globally, trying to create a VM with v2 needs to fail.

Right, I think this is already the case with the current
implementation. This doesn't happen however with the transitive
option, as I implemented it and'ing the hypervisor selection to the
tools provided one, partially due to the lack of a 'use hypervisor
default' option.

Thanks, Roger.

[PATCH 3/6] tools/console: use xenforeigmemory to map console ring

[Roger Pau Monne]

This patch replaces the usage of xc_map_foreign_range with
xenforeignmemory_map from the stable xenforeignmemory library. Note
there are still other uses of libxc functions which prevents removing
the dependency.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/console/Makefile    |  4 ++--
 tools/console/daemon/io.c | 25 ++++++++++++++++++++-----
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/tools/console/Makefile b/tools/console/Makefile
index 84796eac8f..3f4cddab03 100644
--- a/tools/console/Makefile
+++ b/tools/console/Makefile
@@ -29,9 +29,9 @@ clean:
 distclean: clean
 
 daemon/main.o: daemon/_paths.h
-daemon/io.o: CFLAGS += $(CFLAGS_libxenevtchn) $(CFLAGS_libxengnttab) $(CONSOLE_CFLAGS-y)
+daemon/io.o: CFLAGS += $(CFLAGS_libxenevtchn) $(CFLAGS_libxengnttab) $(CFLAGS_libxenforeignmemory) $(CONSOLE_CFLAGS-y)
 xenconsoled: $(patsubst %.c,%.o,$(wildcard daemon/*.c))
-	$(CC) $(LDFLAGS) $^ -o $@ $(LDLIBS) $(LDLIBS_libxenevtchn) $(LDLIBS_libxengnttab) $(LDLIBS_xenconsoled) $(APPEND_LDFLAGS)
+	$(CC) $(LDFLAGS) $^ -o $@ $(LDLIBS) $(LDLIBS_libxenevtchn) $(LDLIBS_libxengnttab) $(LDLIBS_libxenforeignmemory) $(LDLIBS_xenconsoled) $(APPEND_LDFLAGS)
 
 client/main.o: client/_paths.h
 xenconsole: $(patsubst %.c,%.o,$(wildcard client/*.c))
diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index 200b575d76..682c1f4008 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
@@ -22,6 +22,7 @@
 #include "utils.h"
 #include "io.h"
 #include <xenevtchn.h>
+#include <xenforeignmemory.h>
 #include <xengnttab.h>
 #include <xenstore.h>
 #include <xen/io/console.h>
@@ -73,6 +74,7 @@ static int log_time_guest_needts = 1;
 static int log_hv_fd = -1;
 
 static xengnttab_handle *xgt_handle = NULL;
+static xenforeignmemory_handle *xfm_handle;
 
 static struct pollfd  *fds;
 static unsigned int current_array_size;
@@ -675,7 +677,7 @@ static void console_unmap_interface(struct console *con)
 	if (xgt_handle && con->ring_ref == -1)
 		xengnttab_unmap(xgt_handle, con->interface, 1);
 	else
-		munmap(con->interface, XC_PAGE_SIZE);
+		xenforeignmemory_unmap(xfm_handle, con->interface, 1);
 	con->interface = NULL;
 	con->ring_ref = -1;
 }
@@ -722,11 +724,12 @@ static int console_create_ring(struct console *con)
 		con->ring_ref = -1;
 	}
 	if (!con->interface) {
+		xen_pfn_t pfn = ring_ref;
+
 		/* Fall back to xc_map_foreign_range */
-		con->interface = xc_map_foreign_range(
-			xc, dom->domid, XC_PAGE_SIZE,
-			PROT_READ|PROT_WRITE,
-			(unsigned long)ring_ref);
+		con->interface = xenforeignmemory_map(
+			xfm_handle, dom->domid,	PROT_READ|PROT_WRITE, 1,
+			&pfn, NULL);
 		if (con->interface == NULL) {
 			err = EINVAL;
 			goto out;
@@ -1341,6 +1344,14 @@ void handle_io(void)
 		      errno, strerror(errno));
 	}
 
+	xfm_handle = xenforeignmemory_open(NULL, 0);
+	if (xfm_handle == NULL) {
+		dolog(LOG_ERR,
+		      "Failed to open xen foreign memory handle: %d (%s)",
+		      errno, strerror(errno));
+		goto out;
+	}
+
 	enum_domains();
 
 	for (;;) {
@@ -1462,6 +1473,10 @@ void handle_io(void)
 		xengnttab_close(xgt_handle);
 		xgt_handle = NULL;
 	}
+	if (xfm_handle != NULL) {
+		xenforeignmemory_close(xfm_handle);
+		xfm_handle = NULL;
+	}
 	log_hv_evtchn = -1;
 }
 
-- 
2.33.0

Re: [PATCH 3/6] tools/console: use xenforeigmemory to map console ring

[Ian Jackson]

Roger Pau Monne writes ("[PATCH 3/6] tools/console: use xenforeigmemory to map console ring"):
> This patch replaces the usage of xc_map_foreign_range with
> xenforeignmemory_map from the stable xenforeignmemory library. Note
> there are still other uses of libxc functions which prevents removing
> the dependency.
> 
> No functional change intended.

Reviewed-by: Ian Jackson <iwj@xenproject.org>

[PATCH 4/6] tools/xenstored: use atexit to close interfaces

[Roger Pau Monne]

Exploiting the talloc clean up routines to close the Xen interfaces
is cumbersome, specially when atexit can be used to the same effect.

Convert xc and gnttab to use atexit which allows to drop one
indirection from the storing variables.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/xenstore/xenstored_core.h   |  2 +-
 tools/xenstore/xenstored_domain.c | 57 +++++++++++++------------------
 2 files changed, 25 insertions(+), 34 deletions(-)

diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index 258f6ff382..a813bc5a8c 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -266,7 +266,7 @@ void init_pipe(int reopen_log_pipe[2]);
 #ifndef NO_SOCKETS
 extern const struct interface_funcs socket_funcs;
 #endif
-extern xengnttab_handle **xgt_handle;
+extern xengnttab_handle *xgt_handle;
 
 int remember_string(struct hashtable *hash, const char *str);
 
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index 47e9107c14..8930303773 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -35,8 +35,8 @@
 #include <xenctrl.h>
 #include <xen/grant_table.h>
 
-static xc_interface **xc_handle;
-xengnttab_handle **xgt_handle;
+static xc_interface *xc_handle;
+xengnttab_handle *xgt_handle;
 static evtchn_port_t virq_port;
 
 xenevtchn_handle *xce_handle = NULL;
@@ -198,14 +198,14 @@ static const struct interface_funcs domain_funcs = {
 
 static void *map_interface(domid_t domid)
 {
-	return xengnttab_map_grant_ref(*xgt_handle, domid,
+	return xengnttab_map_grant_ref(xgt_handle, domid,
 				       GNTTAB_RESERVED_XENSTORE,
 				       PROT_READ|PROT_WRITE);
 }
 
 static void unmap_interface(void *interface)
 {
-	xengnttab_unmap(*xgt_handle, interface, 1);
+	xengnttab_unmap(xgt_handle, interface, 1);
 }
 
 static int destroy_domain(void *_domain)
@@ -240,7 +240,7 @@ static int destroy_domain(void *_domain)
 
 static bool get_domain_info(unsigned int domid, xc_dominfo_t *dominfo)
 {
-	return xc_domain_getinfo(*xc_handle, domid, 1, dominfo) == 1 &&
+	return xc_domain_getinfo(xc_handle, domid, 1, dominfo) == 1 &&
 	       dominfo->domid == domid;
 }
 
@@ -648,18 +648,6 @@ int do_reset_watches(struct connection *conn, struct buffered_data *in)
 	return 0;
 }
 
-static int close_xc_handle(void *_handle)
-{
-	xc_interface_close(*(xc_interface**)_handle);
-	return 0;
-}
-
-static int close_xgt_handle(void *_handle)
-{
-	xengnttab_close(*(xengnttab_handle **)_handle);
-	return 0;
-}
-
 /* Returns the implicit path of a connection (only domains have this) */
 const char *get_implicit_path(const struct connection *conn)
 {
@@ -737,35 +725,38 @@ void dom0_init(void)
 	xenevtchn_notify(xce_handle, dom0->port);
 }
 
+void interface_cleanup(void)
+{
+	if (xc_handle) {
+		xc_interface_close(xc_handle);
+		xc_handle = NULL;
+	}
+	if (xgt_handle) {
+		xengnttab_close(xgt_handle);
+		xgt_handle = NULL;
+	}
+}
+
 void domain_init(int evtfd)
 {
-	int rc;
+	int rc = atexit(interface_cleanup);
 
-	xc_handle = talloc(talloc_autofree_context(), xc_interface*);
-	if (!xc_handle)
-		barf_perror("Failed to allocate domain handle");
+	if (rc)
+		barf_perror("Unable to register cleanup handler");
 
-	*xc_handle = xc_interface_open(0,0,0);
-	if (!*xc_handle)
+	xc_handle = xc_interface_open(0,0,0);
+	if (!xc_handle)
 		barf_perror("Failed to open connection to hypervisor");
 
-	talloc_set_destructor(xc_handle, close_xc_handle);
-
-	xgt_handle = talloc(talloc_autofree_context(), xengnttab_handle*);
+	xgt_handle = xengnttab_open(NULL, 0);
 	if (!xgt_handle)
-		barf_perror("Failed to allocate domain gnttab handle");
-
-	*xgt_handle = xengnttab_open(NULL, 0);
-	if (*xgt_handle == NULL)
 		barf_perror("Failed to open connection to gnttab");
 
 	/*
 	 * Allow max number of domains for mappings. We allow one grant per
 	 * domain so the theoretical maximum is DOMID_FIRST_RESERVED.
 	 */
-	xengnttab_set_max_grants(*xgt_handle, DOMID_FIRST_RESERVED);
-
-	talloc_set_destructor(xgt_handle, close_xgt_handle);
+	xengnttab_set_max_grants(xgt_handle, DOMID_FIRST_RESERVED);
 
 	if (evtfd < 0)
 		xce_handle = xenevtchn_open(NULL, XENEVTCHN_NO_CLOEXEC);
-- 
2.33.0

Re: [PATCH 4/6] tools/xenstored: use atexit to close interfaces

[Roger Pau Monné]

On Fri, Sep 17, 2021 at 05:46:23PM +0200, Roger Pau Monne wrote:
> Exploiting the talloc clean up routines to close the Xen interfaces
> is cumbersome, specially when atexit can be used to the same effect.
> 
> Convert xc and gnttab to use atexit which allows to drop one
> indirection from the storing variables.
> 
> No functional change intended.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

This patch is missing the following chunk:

diff --git a/tools/xenstore/xenstored_minios.c b/tools/xenstore/xenstored_minios.c
index c94493e52a..9b050c7e02 100644
--- a/tools/xenstore/xenstored_minios.c
+++ b/tools/xenstore/xenstored_minios.c
@@ -49,12 +49,12 @@ evtchn_port_t xenbus_evtchn(void)
 
 void *xenbus_map(void)
 {
-	return xengnttab_map_grant_ref(*xgt_handle, xenbus_master_domid(),
+	return xengnttab_map_grant_ref(xgt_handle, xenbus_master_domid(),
 			GNTTAB_RESERVED_XENSTORE, PROT_READ|PROT_WRITE);
 }
 
 void unmap_xenbus(void *interface)
 {
-	xengnttab_unmap(*xgt_handle, interface, 1);
+	xengnttab_unmap(xgt_handle, interface, 1);
 }
 

Re: [PATCH 4/6] tools/xenstored: use atexit to close interfaces

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1191 bytes --]

On 17.09.21 17:46, Roger Pau Monne wrote:
> Exploiting the talloc clean up routines to close the Xen interfaces
> is cumbersome, specially when atexit can be used to the same effect.
> 
> Convert xc and gnttab to use atexit which allows to drop one
> indirection from the storing variables.
> 
> No functional change intended.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>   tools/xenstore/xenstored_core.h   |  2 +-
>   tools/xenstore/xenstored_domain.c | 57 +++++++++++++------------------
>   2 files changed, 25 insertions(+), 34 deletions(-)
> 
> diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
> index 47e9107c14..8930303773 100644
> --- a/tools/xenstore/xenstored_domain.c
> +++ b/tools/xenstore/xenstored_domain.c
> @@ -737,35 +725,38 @@ void dom0_init(void)
>   	xenevtchn_notify(xce_handle, dom0->port);
>   }
>   
> +void interface_cleanup(void)
> +{
> +	if (xc_handle) {
> +		xc_interface_close(xc_handle);
> +		xc_handle = NULL;
> +	}
> +	if (xgt_handle) {
> +		xengnttab_close(xgt_handle);
> +		xgt_handle = NULL;
> +	}

Could you please add closing of xce_handle(), too?


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 4/6] tools/xenstored: use atexit to close interfaces

[Roger Pau Monné]

On Mon, Sep 20, 2021 at 11:22:15AM +0200, Juergen Gross wrote:
> On 17.09.21 17:46, Roger Pau Monne wrote:
> > Exploiting the talloc clean up routines to close the Xen interfaces
> > is cumbersome, specially when atexit can be used to the same effect.
> > 
> > Convert xc and gnttab to use atexit which allows to drop one
> > indirection from the storing variables.
> > 
> > No functional change intended.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> >   tools/xenstore/xenstored_core.h   |  2 +-
> >   tools/xenstore/xenstored_domain.c | 57 +++++++++++++------------------
> >   2 files changed, 25 insertions(+), 34 deletions(-)
> > 
> > diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
> > index 47e9107c14..8930303773 100644
> > --- a/tools/xenstore/xenstored_domain.c
> > +++ b/tools/xenstore/xenstored_domain.c
> > @@ -737,35 +725,38 @@ void dom0_init(void)
> >   	xenevtchn_notify(xce_handle, dom0->port);
> >   }
> > +void interface_cleanup(void)
> > +{
> > +	if (xc_handle) {
> > +		xc_interface_close(xc_handle);
> > +		xc_handle = NULL;
> > +	}
> > +	if (xgt_handle) {
> > +		xengnttab_close(xgt_handle);
> > +		xgt_handle = NULL;
> > +	}
> 
> Could you please add closing of xce_handle(), too?

Sure, I somehow assumed there was a reason for not closing it related
to live update, but I see that's not the case as you use exec to
launch the new image and atexit handlers are not called in that case.

Thanks, Roger.

Re: [PATCH 4/6] tools/xenstored: use atexit to close interfaces

[Ian Jackson]

Roger Pau Monné writes ("Re: [PATCH 4/6] tools/xenstored: use atexit to close interfaces"):
> On Mon, Sep 20, 2021 at 11:22:15AM +0200, Juergen Gross wrote:
> > Could you please add closing of xce_handle(), too?
> 
> Sure, I somehow assumed there was a reason for not closing it related
> to live update, but I see that's not the case as you use exec to
> launch the new image and atexit handlers are not called in that case.

Are these fds marked CLOEXEC ?  I don't think they are BICBW.
It would probably be good idea to make them so.  An fd leak might
result in one fd being consumed per live update operation.

Ian.

Re: [PATCH 4/6] tools/xenstored: use atexit to close interfaces

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 794 bytes --]

On 20.09.21 12:57, Ian Jackson wrote:
> Roger Pau Monné writes ("Re: [PATCH 4/6] tools/xenstored: use atexit to close interfaces"):
>> On Mon, Sep 20, 2021 at 11:22:15AM +0200, Juergen Gross wrote:
>>> Could you please add closing of xce_handle(), too?
>>
>> Sure, I somehow assumed there was a reason for not closing it related
>> to live update, but I see that's not the case as you use exec to
>> launch the new image and atexit handlers are not called in that case.
> 
> Are these fds marked CLOEXEC ?  I don't think they are BICBW.

They are. I checked when implementing LU and just rechecked.

The event-channel fd is opened explicitly without CLOEXEC in order
to support LU (the new xenstored won't open it again, but gets its
fd via the migration stream).


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 4/6] tools/xenstored: use atexit to close interfaces

[Ian Jackson]

Juergen Gross writes ("Re: [PATCH 4/6] tools/xenstored: use atexit to close interfaces"):
> They are. I checked when implementing LU and just rechecked.
> 
> The event-channel fd is opened explicitly without CLOEXEC in order
> to support LU (the new xenstored won't open it again, but gets its
> fd via the migration stream).

Oh, excellent.  Thanks.

Ian.

Re: [PATCH 4/6] tools/xenstored: use atexit to close interfaces

[Ian Jackson]

Roger Pau Monne writes ("[PATCH 4/6] tools/xenstored: use atexit to close interfaces"):
> Exploiting the talloc clean up routines to close the Xen interfaces
> is cumbersome, specially when atexit can be used to the same effect.

Wait, what ?  Why do we need to do this at all ?  xenstored can't be
restarted so if it exits we might as well leave whatever wreckage.

AIUI the live update doesn't end up calling atexit ?

Ian.

Re: [PATCH 4/6] tools/xenstored: use atexit to close interfaces

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 707 bytes --]

On 20.09.21 12:34, Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH 4/6] tools/xenstored: use atexit to close interfaces"):
>> Exploiting the talloc clean up routines to close the Xen interfaces
>> is cumbersome, specially when atexit can be used to the same effect.
> 
> Wait, what ?  Why do we need to do this at all ?  xenstored can't be
> restarted so if it exits we might as well leave whatever wreckage.

In practice, yes. But having those cleanups might make it easier to
find memory leaks. Why do we have cleanups in normal Xen tools before
exiting? The system is doing cleanup anyway... ;-)

> AIUI the live update doesn't end up calling atexit ?

That's correct.


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[PATCH DNA 5/6] tools/xenstored: restore support for mapping ring as foreign memory

[Roger Pau Monne]

Restore the previous way of mapping the xenstore ring using foreign
memory. Use xenforeignmemory instead of libxc in order to avoid adding
another dependency on a unstable interface.

This in turn requires storing the gfn into xs_state_connection for
resume purposes, which breaks the current format.

Note this is a preparatory patch in order to support the usage of
xenstore on domains without grant table. This not only allows xenstore
usage, but also makes things like "@introduceDomain" events work when
using such guests, otherwise the mapping done in introduce_domain
fails and the "@introduceDomain" event won't be signaled.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/xenstore/Makefile                 |  4 +-
 tools/xenstore/include/xenstore_state.h |  1 +
 tools/xenstore/xenstored_domain.c       | 69 +++++++++++++++++++------
 3 files changed, 56 insertions(+), 18 deletions(-)

diff --git a/tools/xenstore/Makefile b/tools/xenstore/Makefile
index 292b478fa1..9a9f7be5cb 100644
--- a/tools/xenstore/Makefile
+++ b/tools/xenstore/Makefile
@@ -67,10 +67,10 @@ $(XENSTORED_OBJS): CFLAGS += $(SYSTEMD_CFLAGS)
 xenstored: LDFLAGS += $(SYSTEMD_LIBS)
 endif
 
-$(XENSTORED_OBJS): CFLAGS += $(CFLAGS_libxengnttab)
+$(XENSTORED_OBJS): CFLAGS += $(CFLAGS_libxengnttab) $(CFLAGS_libxenforeignmemory)
 
 xenstored: $(XENSTORED_OBJS)
-	$(CC) $^ $(LDFLAGS) $(LDLIBS_libxenevtchn) $(LDLIBS_libxengnttab) $(LDLIBS_libxenctrl) $(LDLIBS_xenstored) $(SOCKET_LIBS) -o $@ $(APPEND_LDFLAGS)
+	$(CC) $^ $(LDFLAGS) $(LDLIBS_libxenevtchn) $(LDLIBS_libxengnttab) $(LDLIBS_libxenforeignmemory) $(LDLIBS_libxenctrl) $(LDLIBS_xenstored) $(SOCKET_LIBS) -o $@ $(APPEND_LDFLAGS)
 
 xenstored.a: $(XENSTORED_OBJS)
 	$(AR) cr $@ $^
diff --git a/tools/xenstore/include/xenstore_state.h b/tools/xenstore/include/xenstore_state.h
index ae0d053c8f..8dcc8d9d8b 100644
--- a/tools/xenstore/include/xenstore_state.h
+++ b/tools/xenstore/include/xenstore_state.h
@@ -80,6 +80,7 @@ struct xs_state_connection {
             uint16_t domid;  /* Domain-Id. */
             uint16_t tdomid; /* Id of target domain or DOMID_INVALID. */
             uint32_t evtchn; /* Event channel port. */
+            uint64_t gfn;    /* Store GFN. */
         } ring;
         int32_t socket_fd;   /* File descriptor for socket connections. */
     } spec;
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index 8930303773..f3563e47aa 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -33,10 +33,12 @@
 
 #include <xenevtchn.h>
 #include <xenctrl.h>
+#include <xenforeignmemory.h>
 #include <xen/grant_table.h>
 
 static xc_interface *xc_handle;
 xengnttab_handle *xgt_handle;
+static xenforeignmemory_handle *xfm_handle;
 static evtchn_port_t virq_port;
 
 xenevtchn_handle *xce_handle = NULL;
@@ -66,12 +68,18 @@ struct domain
 	/* Generation count at domain introduction time. */
 	uint64_t generation;
 
+	/* Store GFN (if using a ring connection). */
+	xen_pfn_t gfn;
+
 	/* Have we noticed that this domain is shutdown? */
 	bool shutdown;
 
 	/* Has domain been officially introduced? */
 	bool introduced;
 
+	/* Is the ring memory foreign mapped? */
+	bool foreign_mapped;
+
 	/* number of entry from this domain in the store */
 	int nbentry;
 
@@ -196,16 +204,29 @@ static const struct interface_funcs domain_funcs = {
 	.can_read = domain_can_read,
 };
 
-static void *map_interface(domid_t domid)
+static void *map_interface(domid_t domid, xen_pfn_t gfn, bool *foreign_mapped)
 {
-	return xengnttab_map_grant_ref(xgt_handle, domid,
-				       GNTTAB_RESERVED_XENSTORE,
-				       PROT_READ|PROT_WRITE);
+	void *map = xengnttab_map_grant_ref(xgt_handle, domid,
+					    GNTTAB_RESERVED_XENSTORE,
+					    PROT_READ|PROT_WRITE);
+
+	if (!map)
+	{
+		map = xenforeignmemory_map(xfm_handle, domid,
+					   PROT_READ|PROT_WRITE, 1,
+					   &gfn, NULL);
+		*foreign_mapped = !!map;
+	}
+
+	return map;
 }
 
-static void unmap_interface(void *interface)
+static void unmap_interface(void *interface, bool foreign_mapped)
 {
-	xengnttab_unmap(xgt_handle, interface, 1);
+	if (foreign_mapped)
+		xenforeignmemory_unmap(xfm_handle, interface, 1);
+	else
+		xengnttab_unmap(xgt_handle, interface, 1);
 }
 
 static int destroy_domain(void *_domain)
@@ -228,7 +249,8 @@ static int destroy_domain(void *_domain)
 		if (domain->domid == 0)
 			unmap_xenbus(domain->interface);
 		else
-			unmap_interface(domain->interface);
+			unmap_interface(domain->interface,
+					domain->foreign_mapped);
 	}
 
 	fire_watches(NULL, domain, "@releaseDomain", NULL, false, NULL);
@@ -363,12 +385,15 @@ static struct domain *find_or_alloc_domain(const void *ctx, unsigned int domid)
 	return domain ? : alloc_domain(ctx, domid);
 }
 
-static int new_domain(struct domain *domain, int port, bool restore)
+static int new_domain(struct domain *domain, int port, xen_pfn_t gfn,
+		      bool foreign_mapped, bool restore)
 {
 	int rc;
 
 	domain->port = 0;
 	domain->shutdown = false;
+	domain->gfn = gfn;
+	domain->foreign_mapped = foreign_mapped;
 	domain->path = talloc_domain_path(domain, domain->domid);
 	if (!domain->path) {
 		errno = ENOMEM;
@@ -436,7 +461,8 @@ static void domain_conn_reset(struct domain *domain)
 
 static struct domain *introduce_domain(const void *ctx,
 				       unsigned int domid,
-				       evtchn_port_t port, bool restore)
+				       evtchn_port_t port, xen_pfn_t gfn,
+				       bool restore)
 {
 	struct domain *domain;
 	int rc;
@@ -448,17 +474,21 @@ static struct domain *introduce_domain(const void *ctx,
 		return NULL;
 
 	if (!domain->introduced) {
+		bool foreign_mapped = false;
+
 		interface = is_master_domain ? xenbus_map()
-					     : map_interface(domid);
+					     : map_interface(domid, gfn,
+							     &foreign_mapped);
 		if (!interface && !restore)
 			return NULL;
-		if (new_domain(domain, port, restore)) {
+		if (new_domain(domain, port, gfn, foreign_mapped, restore)) {
 			rc = errno;
 			if (interface) {
 				if (is_master_domain)
 					unmap_xenbus(interface);
 				else
-					unmap_interface(interface);
+					unmap_interface(interface,
+							foreign_mapped);
 			}
 			errno = rc;
 			return NULL;
@@ -489,19 +519,20 @@ int do_introduce(struct connection *conn, struct buffered_data *in)
 	char *vec[3];
 	unsigned int domid;
 	evtchn_port_t port;
+	xen_pfn_t gfn;
 
 	if (get_strings(in, vec, ARRAY_SIZE(vec)) < ARRAY_SIZE(vec))
 		return EINVAL;
 
 	domid = atoi(vec[0]);
-	/* Ignore the gfn, we don't need it. */
+	gfn = atol(vec[1]);
 	port = atoi(vec[2]);
 
 	/* Sanity check args. */
 	if (port <= 0)
 		return EINVAL;
 
-	domain = introduce_domain(in, domid, port, false);
+	domain = introduce_domain(in, domid, port, gfn, false);
 	if (!domain)
 		return errno;
 
@@ -718,7 +749,7 @@ void dom0_init(void)
 	if (port == -1)
 		barf_perror("Failed to initialize dom0 port");
 
-	dom0 = introduce_domain(NULL, xenbus_master_domid(), port, false);
+	dom0 = introduce_domain(NULL, xenbus_master_domid(), port, 0, false);
 	if (!dom0)
 		barf_perror("Failed to initialize dom0");
 
@@ -758,6 +789,10 @@ void domain_init(int evtfd)
 	 */
 	xengnttab_set_max_grants(xgt_handle, DOMID_FIRST_RESERVED);
 
+	xfm_handle = xenforeignmemory_open(NULL, 0);
+	if (!xfm_handle)
+		barf_perror("Failed to create handle for foreign memory");
+
 	if (evtfd < 0)
 		xce_handle = xenevtchn_open(NULL, XENEVTCHN_NO_CLOEXEC);
 	else
@@ -1189,6 +1224,7 @@ const char *dump_state_connections(FILE *fp)
 			sc.spec.ring.tdomid = c->target ? c->target->id
 						: DOMID_INVALID;
 			sc.spec.ring.evtchn = c->domain->port;
+			sc.spec.ring.gfn = c->domain->gfn;
 		} else {
 			sc.conn_type = XS_STATE_CONN_TYPE_SOCKET;
 			sc.spec.socket_fd = c->fd;
@@ -1290,7 +1326,8 @@ void read_state_connection(const void *ctx, const void *state)
 #endif
 	} else {
 		domain = introduce_domain(ctx, sc->spec.ring.domid,
-					  sc->spec.ring.evtchn, true);
+					  sc->spec.ring.evtchn,
+					  sc->spec.ring.gfn, true);
 		if (!domain)
 			barf("domain allocation error");
 
-- 
2.33.0

Re: [PATCH DNA 5/6] tools/xenstored: restore support for mapping ring as foreign memory

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 9529 bytes --]

On 17.09.21 17:46, Roger Pau Monne wrote:
> Restore the previous way of mapping the xenstore ring using foreign
> memory. Use xenforeignmemory instead of libxc in order to avoid adding
> another dependency on a unstable interface.

Mapping a guest page via xenforeignmemory is no good move IMO. A guest
not supporting a grant table for security reasons is a rather strange
idea, as it needs to trade that for a general memory access by any
backend without a way to restrict such accesses. This contradicts one
of the main concepts of the Xen security architecture.

At the same time this will either kill the use of xenstore-stubdom, or
it will require an extended XSM configuration allowing xenstore-stubdom
to establish arbitrary page mappings of those guests.


Juergen

> 
> This in turn requires storing the gfn into xs_state_connection for
> resume purposes, which breaks the current format.
> 
> Note this is a preparatory patch in order to support the usage of
> xenstore on domains without grant table. This not only allows xenstore
> usage, but also makes things like "@introduceDomain" events work when
> using such guests, otherwise the mapping done in introduce_domain
> fails and the "@introduceDomain" event won't be signaled.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>   tools/xenstore/Makefile                 |  4 +-
>   tools/xenstore/include/xenstore_state.h |  1 +
>   tools/xenstore/xenstored_domain.c       | 69 +++++++++++++++++++------
>   3 files changed, 56 insertions(+), 18 deletions(-)
> 
> diff --git a/tools/xenstore/Makefile b/tools/xenstore/Makefile
> index 292b478fa1..9a9f7be5cb 100644
> --- a/tools/xenstore/Makefile
> +++ b/tools/xenstore/Makefile
> @@ -67,10 +67,10 @@ $(XENSTORED_OBJS): CFLAGS += $(SYSTEMD_CFLAGS)
>   xenstored: LDFLAGS += $(SYSTEMD_LIBS)
>   endif
>   
> -$(XENSTORED_OBJS): CFLAGS += $(CFLAGS_libxengnttab)
> +$(XENSTORED_OBJS): CFLAGS += $(CFLAGS_libxengnttab) $(CFLAGS_libxenforeignmemory)
>   
>   xenstored: $(XENSTORED_OBJS)
> -	$(CC) $^ $(LDFLAGS) $(LDLIBS_libxenevtchn) $(LDLIBS_libxengnttab) $(LDLIBS_libxenctrl) $(LDLIBS_xenstored) $(SOCKET_LIBS) -o $@ $(APPEND_LDFLAGS)
> +	$(CC) $^ $(LDFLAGS) $(LDLIBS_libxenevtchn) $(LDLIBS_libxengnttab) $(LDLIBS_libxenforeignmemory) $(LDLIBS_libxenctrl) $(LDLIBS_xenstored) $(SOCKET_LIBS) -o $@ $(APPEND_LDFLAGS)
>   
>   xenstored.a: $(XENSTORED_OBJS)
>   	$(AR) cr $@ $^
> diff --git a/tools/xenstore/include/xenstore_state.h b/tools/xenstore/include/xenstore_state.h
> index ae0d053c8f..8dcc8d9d8b 100644
> --- a/tools/xenstore/include/xenstore_state.h
> +++ b/tools/xenstore/include/xenstore_state.h
> @@ -80,6 +80,7 @@ struct xs_state_connection {
>               uint16_t domid;  /* Domain-Id. */
>               uint16_t tdomid; /* Id of target domain or DOMID_INVALID. */
>               uint32_t evtchn; /* Event channel port. */
> +            uint64_t gfn;    /* Store GFN. */
>           } ring;
>           int32_t socket_fd;   /* File descriptor for socket connections. */
>       } spec;
> diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
> index 8930303773..f3563e47aa 100644
> --- a/tools/xenstore/xenstored_domain.c
> +++ b/tools/xenstore/xenstored_domain.c
> @@ -33,10 +33,12 @@
>   
>   #include <xenevtchn.h>
>   #include <xenctrl.h>
> +#include <xenforeignmemory.h>
>   #include <xen/grant_table.h>
>   
>   static xc_interface *xc_handle;
>   xengnttab_handle *xgt_handle;
> +static xenforeignmemory_handle *xfm_handle;
>   static evtchn_port_t virq_port;
>   
>   xenevtchn_handle *xce_handle = NULL;
> @@ -66,12 +68,18 @@ struct domain
>   	/* Generation count at domain introduction time. */
>   	uint64_t generation;
>   
> +	/* Store GFN (if using a ring connection). */
> +	xen_pfn_t gfn;
> +
>   	/* Have we noticed that this domain is shutdown? */
>   	bool shutdown;
>   
>   	/* Has domain been officially introduced? */
>   	bool introduced;
>   
> +	/* Is the ring memory foreign mapped? */
> +	bool foreign_mapped;
> +
>   	/* number of entry from this domain in the store */
>   	int nbentry;
>   
> @@ -196,16 +204,29 @@ static const struct interface_funcs domain_funcs = {
>   	.can_read = domain_can_read,
>   };
>   
> -static void *map_interface(domid_t domid)
> +static void *map_interface(domid_t domid, xen_pfn_t gfn, bool *foreign_mapped)
>   {
> -	return xengnttab_map_grant_ref(xgt_handle, domid,
> -				       GNTTAB_RESERVED_XENSTORE,
> -				       PROT_READ|PROT_WRITE);
> +	void *map = xengnttab_map_grant_ref(xgt_handle, domid,
> +					    GNTTAB_RESERVED_XENSTORE,
> +					    PROT_READ|PROT_WRITE);
> +
> +	if (!map)
> +	{
> +		map = xenforeignmemory_map(xfm_handle, domid,
> +					   PROT_READ|PROT_WRITE, 1,
> +					   &gfn, NULL);
> +		*foreign_mapped = !!map;
> +	}
> +
> +	return map;
>   }
>   
> -static void unmap_interface(void *interface)
> +static void unmap_interface(void *interface, bool foreign_mapped)
>   {
> -	xengnttab_unmap(xgt_handle, interface, 1);
> +	if (foreign_mapped)
> +		xenforeignmemory_unmap(xfm_handle, interface, 1);
> +	else
> +		xengnttab_unmap(xgt_handle, interface, 1);
>   }
>   
>   static int destroy_domain(void *_domain)
> @@ -228,7 +249,8 @@ static int destroy_domain(void *_domain)
>   		if (domain->domid == 0)
>   			unmap_xenbus(domain->interface);
>   		else
> -			unmap_interface(domain->interface);
> +			unmap_interface(domain->interface,
> +					domain->foreign_mapped);
>   	}
>   
>   	fire_watches(NULL, domain, "@releaseDomain", NULL, false, NULL);
> @@ -363,12 +385,15 @@ static struct domain *find_or_alloc_domain(const void *ctx, unsigned int domid)
>   	return domain ? : alloc_domain(ctx, domid);
>   }
>   
> -static int new_domain(struct domain *domain, int port, bool restore)
> +static int new_domain(struct domain *domain, int port, xen_pfn_t gfn,
> +		      bool foreign_mapped, bool restore)
>   {
>   	int rc;
>   
>   	domain->port = 0;
>   	domain->shutdown = false;
> +	domain->gfn = gfn;
> +	domain->foreign_mapped = foreign_mapped;
>   	domain->path = talloc_domain_path(domain, domain->domid);
>   	if (!domain->path) {
>   		errno = ENOMEM;
> @@ -436,7 +461,8 @@ static void domain_conn_reset(struct domain *domain)
>   
>   static struct domain *introduce_domain(const void *ctx,
>   				       unsigned int domid,
> -				       evtchn_port_t port, bool restore)
> +				       evtchn_port_t port, xen_pfn_t gfn,
> +				       bool restore)
>   {
>   	struct domain *domain;
>   	int rc;
> @@ -448,17 +474,21 @@ static struct domain *introduce_domain(const void *ctx,
>   		return NULL;
>   
>   	if (!domain->introduced) {
> +		bool foreign_mapped = false;
> +
>   		interface = is_master_domain ? xenbus_map()
> -					     : map_interface(domid);
> +					     : map_interface(domid, gfn,
> +							     &foreign_mapped);
>   		if (!interface && !restore)
>   			return NULL;
> -		if (new_domain(domain, port, restore)) {
> +		if (new_domain(domain, port, gfn, foreign_mapped, restore)) {
>   			rc = errno;
>   			if (interface) {
>   				if (is_master_domain)
>   					unmap_xenbus(interface);
>   				else
> -					unmap_interface(interface);
> +					unmap_interface(interface,
> +							foreign_mapped);
>   			}
>   			errno = rc;
>   			return NULL;
> @@ -489,19 +519,20 @@ int do_introduce(struct connection *conn, struct buffered_data *in)
>   	char *vec[3];
>   	unsigned int domid;
>   	evtchn_port_t port;
> +	xen_pfn_t gfn;
>   
>   	if (get_strings(in, vec, ARRAY_SIZE(vec)) < ARRAY_SIZE(vec))
>   		return EINVAL;
>   
>   	domid = atoi(vec[0]);
> -	/* Ignore the gfn, we don't need it. */
> +	gfn = atol(vec[1]);
>   	port = atoi(vec[2]);
>   
>   	/* Sanity check args. */
>   	if (port <= 0)
>   		return EINVAL;
>   
> -	domain = introduce_domain(in, domid, port, false);
> +	domain = introduce_domain(in, domid, port, gfn, false);
>   	if (!domain)
>   		return errno;
>   
> @@ -718,7 +749,7 @@ void dom0_init(void)
>   	if (port == -1)
>   		barf_perror("Failed to initialize dom0 port");
>   
> -	dom0 = introduce_domain(NULL, xenbus_master_domid(), port, false);
> +	dom0 = introduce_domain(NULL, xenbus_master_domid(), port, 0, false);
>   	if (!dom0)
>   		barf_perror("Failed to initialize dom0");
>   
> @@ -758,6 +789,10 @@ void domain_init(int evtfd)
>   	 */
>   	xengnttab_set_max_grants(xgt_handle, DOMID_FIRST_RESERVED);
>   
> +	xfm_handle = xenforeignmemory_open(NULL, 0);
> +	if (!xfm_handle)
> +		barf_perror("Failed to create handle for foreign memory");
> +
>   	if (evtfd < 0)
>   		xce_handle = xenevtchn_open(NULL, XENEVTCHN_NO_CLOEXEC);
>   	else
> @@ -1189,6 +1224,7 @@ const char *dump_state_connections(FILE *fp)
>   			sc.spec.ring.tdomid = c->target ? c->target->id
>   						: DOMID_INVALID;
>   			sc.spec.ring.evtchn = c->domain->port;
> +			sc.spec.ring.gfn = c->domain->gfn;
>   		} else {
>   			sc.conn_type = XS_STATE_CONN_TYPE_SOCKET;
>   			sc.spec.socket_fd = c->fd;
> @@ -1290,7 +1326,8 @@ void read_state_connection(const void *ctx, const void *state)
>   #endif
>   	} else {
>   		domain = introduce_domain(ctx, sc->spec.ring.domid,
> -					  sc->spec.ring.evtchn, true);
> +					  sc->spec.ring.evtchn,
> +					  sc->spec.ring.gfn, true);
>   		if (!domain)
>   			barf("domain allocation error");
>   
> 


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH DNA 5/6] tools/xenstored: restore support for mapping ring as foreign memory

[Roger Pau Monné]

On Mon, Sep 20, 2021 at 10:24:45AM +0200, Juergen Gross wrote:
> On 17.09.21 17:46, Roger Pau Monne wrote:
> > Restore the previous way of mapping the xenstore ring using foreign
> > memory. Use xenforeignmemory instead of libxc in order to avoid adding
> > another dependency on a unstable interface.
> 
> Mapping a guest page via xenforeignmemory is no good move IMO. A guest
> not supporting a grant table for security reasons is a rather strange
> idea, as it needs to trade that for a general memory access by any
> backend without a way to restrict such accesses. This contradicts one
> of the main concepts of the Xen security architecture.

I've done this in order to be able to assert that the switch to
disable grant tables was working correctly, I don't intended this
specific mode to be something that is desirable or that should be used
in production, but I do think it's useful to be able to create such
guests in order to assert that the option is taking effect.

The main problem of xenstore not being correctly initialized on a
domain is that the "@introduceDomain" watch doesn't fire, and thus
other components don't get notified of the newly created domain.

This seems to be a limitation of the current design, where the only
way to get notifications of new domain creation is using
"@introduceDomain", even when the caller doesn't care of whether the
created domain as a working xenstore connection.

Maybe I can workaround this differently in xenstore, so that the watch
fires even when the shared xenstore ring cannot be initialized.

Thanks, Roger.

Re: [PATCH DNA 5/6] tools/xenstored: restore support for mapping ring as foreign memory

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1732 bytes --]

On 20.09.21 12:42, Roger Pau Monné wrote:
> On Mon, Sep 20, 2021 at 10:24:45AM +0200, Juergen Gross wrote:
>> On 17.09.21 17:46, Roger Pau Monne wrote:
>>> Restore the previous way of mapping the xenstore ring using foreign
>>> memory. Use xenforeignmemory instead of libxc in order to avoid adding
>>> another dependency on a unstable interface.
>>
>> Mapping a guest page via xenforeignmemory is no good move IMO. A guest
>> not supporting a grant table for security reasons is a rather strange
>> idea, as it needs to trade that for a general memory access by any
>> backend without a way to restrict such accesses. This contradicts one
>> of the main concepts of the Xen security architecture.
> 
> I've done this in order to be able to assert that the switch to
> disable grant tables was working correctly, I don't intended this
> specific mode to be something that is desirable or that should be used
> in production, but I do think it's useful to be able to create such
> guests in order to assert that the option is taking effect.
> 
> The main problem of xenstore not being correctly initialized on a
> domain is that the "@introduceDomain" watch doesn't fire, and thus
> other components don't get notified of the newly created domain.
> 
> This seems to be a limitation of the current design, where the only
> way to get notifications of new domain creation is using
> "@introduceDomain", even when the caller doesn't care of whether the
> created domain as a working xenstore connection.
> 
> Maybe I can workaround this differently in xenstore, so that the watch
> fires even when the shared xenstore ring cannot be initialized.

Yes, I think this would be the way to go.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH DNA 5/6] tools/xenstored: restore support for mapping ring as foreign memory

[Ian Jackson]

Roger Pau Monne writes ("[PATCH DNA 5/6] tools/xenstored: restore support for mapping ring as foreign memory"):
> Restore the previous way of mapping the xenstore ring using foreign
> memory. Use xenforeignmemory instead of libxc in order to avoid adding
> another dependency on a unstable interface.

AIUI this therefore partially reverts something - can you point us to
the specific commits ?  In the commit message, ideally.

Thanks,
Ian.

[PATCH 6/6] gnttab: allow disabling grant table per-domain

[Roger Pau Monne]

Allow setting max_grant_version to 0 in order to disable grant table
usage by a domain. This prevents allocating the grant-table structure
inside of Xen and requires guards to be added in several functions in
order to prevent dereferencing the structure.

Note that a domain without a grant table could still use some of the
grant related hypercalls, it could for example issue a GNTTABOP_copy
of a grant reference from a remote domain into a local frame.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 docs/man/xl.cfg.5.pod.in     |  4 +-
 tools/libs/light/libxl_dom.c |  2 +-
 xen/common/grant_table.c     | 97 ++++++++++++++++++++++++++++++++++--
 3 files changed, 95 insertions(+), 8 deletions(-)

diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index c5a447dfcd..d507540c2c 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -583,8 +583,8 @@ L<xl.conf(5)>.
 =item B<max_grant_version=NUMBER>
 
 Specify the maximum grant table version the domain is allowed to use. Current
-supported versions are 1 and 2. The default value is settable via
-L<xl.conf(5)>.
+supported versions are 1 and 2. Setting to 0 disables the grant table for the
+domain. The default value is settable via L<xl.conf(5)>.
 
 =item B<transitive_grants=BOOLEAN>
 
diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
index e9f58ee4b2..afc8b88497 100644
--- a/tools/libs/light/libxl_dom.c
+++ b/tools/libs/light/libxl_dom.c
@@ -598,7 +598,7 @@ static int libxl__build_dom(libxl__gc *gc, uint32_t domid,
         LOGE(ERROR, "xc_dom_boot_image failed");
         goto out;
     }
-    if ( (ret = xc_dom_gnttab_init(dom)) != 0 ) {
+    if ( info->max_grant_version && (ret = xc_dom_gnttab_init(dom)) != 0 ) {
         LOGE(ERROR, "xc_dom_gnttab_init failed");
         goto out;
     }
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 280dbc850a..68ea742498 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1027,6 +1027,12 @@ map_grant_ref(
     }
 
     lgt = ld->grant_table;
+    if ( !lgt )
+    {
+        gdprintk(XENLOG_INFO, "%pd has no grant table\n", ld);
+        op->status = GNTST_bad_domain;
+        return;
+    }
     handle = get_maptrack_handle(lgt);
     if ( unlikely(handle == INVALID_MAPTRACK_HANDLE) )
     {
@@ -1037,6 +1043,14 @@ map_grant_ref(
     }
 
     rgt = rd->grant_table;
+    if ( !rgt )
+    {
+        put_maptrack_handle(lgt, handle);
+        rcu_unlock_domain(rd);
+        gdprintk(XENLOG_INFO, "%pd has no grant table\n", rd);
+        op->status = GNTST_bad_domain;
+        return;
+    }
     grant_read_lock(rgt);
 
     /* Bounds check on the grant ref */
@@ -1367,6 +1381,13 @@ unmap_common(
     ld = current->domain;
     lgt = ld->grant_table;
 
+    if ( !lgt )
+    {
+        gdprintk(XENLOG_INFO, "%pd has no grant table\n", ld);
+        op->status = GNTST_bad_domain;
+        return;
+    }
+
     if ( unlikely(op->handle >= lgt->maptrack_limit) )
     {
         gdprintk(XENLOG_INFO, "Bad d%d handle %#x\n",
@@ -1406,6 +1427,13 @@ unmap_common(
     TRACE_1D(TRC_MEM_PAGE_GRANT_UNMAP, dom);
 
     rgt = rd->grant_table;
+    if ( !rgt )
+    {
+        rcu_unlock_domain(rd);
+        gdprintk(XENLOG_INFO, "%pd has no grant table\n", rd);
+        op->status = GNTST_bad_domain;
+        return;
+    }
 
     grant_read_lock(rgt);
 
@@ -1556,6 +1584,12 @@ unmap_common_complete(struct gnttab_unmap_common *op)
 
     rcu_lock_domain(rd);
     rgt = rd->grant_table;
+    if ( !rgt )
+    {
+        rcu_unlock_domain(rd);
+        op->status = GNTST_bad_domain;
+        return;
+    }
 
     grant_read_lock(rgt);
 
@@ -1931,10 +1965,7 @@ int grant_table_init(struct domain *d, int max_grant_frames,
     if ( max_grant_version < 0 )
         max_grant_version = opt_gnttab_max_version;
     if ( !max_grant_version )
-    {
-        dprintk(XENLOG_INFO, "Invalid grant table version 0 requested\n");
-        return -EINVAL;
-    }
+        return 0;
     if ( max_grant_version > opt_gnttab_max_version )
     {
         dprintk(XENLOG_INFO,
@@ -2056,6 +2087,11 @@ gnttab_setup_table(
     }
 
     gt = d->grant_table;
+    if ( !gt )
+    {
+        op.status = GNTST_bad_domain;
+        goto out;
+    }
     grant_write_lock(gt);
 
     if ( unlikely(op.nr_frames > gt->max_grant_frames) )
@@ -2138,6 +2174,11 @@ gnttab_query_size(
     }
 
     gt = d->grant_table;
+    if ( !gt )
+    {
+        op.status = GNTST_bad_domain;
+        goto out;
+    }
 
     grant_read_lock(gt);
 
@@ -2302,6 +2343,13 @@ gnttab_transfer(
             goto put_gfn_and_copyback;
         }
 
+        if ( unlikely(!e->grant_table) )
+        {
+            gdprintk(XENLOG_INFO, "%pd has no grant table\n", e);
+            gop.status = GNTST_bad_domain;
+            goto unlock_and_copyback;
+        }
+
         if ( xsm_grant_transfer(XSM_HOOK, d, e) )
         {
             gop.status = GNTST_permission_denied;
@@ -2888,6 +2936,12 @@ static int gnttab_copy_claim_buf(const struct gnttab_copy *op,
 
     if ( op->flags & gref_flag )
     {
+        if ( !buf->domain->grant_table )
+        {
+            rc = GNTST_bad_domain;
+            goto out;
+        }
+
         rc = acquire_grant_for_copy(buf->domain, ptr->u.ref,
                                     current->domain->domain_id,
                                     buf->read_only,
@@ -3092,6 +3146,9 @@ gnttab_set_version(XEN_GUEST_HANDLE_PARAM(gnttab_set_version_t) uop)
     int res;
     unsigned int i, nr_ents;
 
+    if ( !gt )
+        return -ENODEV;
+
     if ( copy_from_guest(&op, uop, 1) )
         return -EFAULT;
 
@@ -3270,6 +3327,11 @@ gnttab_get_status_frames(XEN_GUEST_HANDLE_PARAM(gnttab_get_status_frames_t) uop,
     }
 
     gt = d->grant_table;
+    if ( !gt )
+    {
+        op.status = GNTST_bad_domain;
+        goto out2;
+    }
 
     op.status = GNTST_okay;
 
@@ -3332,7 +3394,11 @@ gnttab_get_version(XEN_GUEST_HANDLE_PARAM(gnttab_get_version_t) uop)
         return rc;
     }
 
-    op.version = d->grant_table->gt_version;
+    if ( d->grant_table )
+        op.version = d->grant_table->gt_version;
+    else
+        /* Use 0 to signal no grant table. */
+        op.version = 0;
 
     rcu_unlock_domain(d);
 
@@ -3351,6 +3417,12 @@ swap_grant_ref(grant_ref_t ref_a, grant_ref_t ref_b)
     struct active_grant_entry *act_b = NULL;
     s16 rc = GNTST_okay;
 
+    if ( !gt )
+    {
+        rcu_unlock_domain(d);
+        return GNTST_bad_domain;
+    }
+
     grant_write_lock(gt);
 
     /* Bounds check on the grant refs */
@@ -3872,6 +3944,9 @@ void grant_table_warn_active_grants(struct domain *d)
 
 #define WARN_GRANT_MAX 10
 
+    if ( !gt )
+        return;
+
     grant_read_lock(gt);
 
     nr_ents = nr_grant_entries(gt);
@@ -3953,6 +4028,9 @@ int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref,
     int rc = 0;
     uint16_t flags = 0;
 
+    if ( !gt )
+        return -ENODEV;
+
     grant_read_lock(gt);
 
     if ( gt->gt_version < 1 )
@@ -4069,6 +4147,9 @@ unsigned int gnttab_resource_max_frames(const struct domain *d, unsigned int id)
     const struct grant_table *gt = d->grant_table;
     unsigned int nr = 0;
 
+    if ( !gt )
+        return 0;
+
     /* Don't need the grant lock.  This limit is fixed at domain create time. */
     switch ( id )
     {
@@ -4100,6 +4181,9 @@ int gnttab_acquire_resource(
     if ( !nr_frames )
         return rc;
 
+    if ( !gt )
+        return -ENODEV;
+
     final_frame = frame + nr_frames - 1;
 
     /* Grow table if necessary. */
@@ -4156,6 +4240,9 @@ int gnttab_map_frame(struct domain *d, unsigned long idx, gfn_t gfn, mfn_t *mfn)
     struct grant_table *gt = d->grant_table;
     bool status = false;
 
+    if ( !gt )
+        return -ENODEV;
+
     grant_write_lock(gt);
 
     if ( evaluate_nospec(gt->gt_version == 2) && (idx & XENMAPIDX_grant_table_status) )
-- 
2.33.0

Re: [PATCH 0/6] gnttab: add per-domain controls

[Christian Lindig]

> On 17 Sep 2021, at 16:46, Roger Pau Monne <roger.pau@citrix.com> wrote:
> 
> Hello,
> 
> The first two patches of this series allows setting the preisoutly host
> wide command line `gnttab` option on a per domain basis. That means
> selecting the max allowed grant table version and whether transitive
> grants are allowed.
> 
> The last 4 patches attempt to implement support for creating guests
> without grant table support at all. This requires some changes to
> xenstore in order to map shared ring using foreign memory instead of
> grant table.
> 
> Note that patch 5 will break the save format for xenstore records, and
> should not be applied.

Has this relevance for the format used by oxenstored?

> 
> Thanks, Roger.
> 
> Roger Pau Monne (6):
>  gnttab: allow setting max version per-domain
>  grant: allow per-domain control over transitive grants
>  tools/console: use xenforeigmemory to map console ring
>  tools/xenstored: use atexit to close interfaces
>  tools/xenstored: restore support for mapping ring as foreign memory
>  gnttab: allow disabling grant table per-domain
> 
> docs/man/xl.cfg.5.pod.in                |  12 +++
> docs/man/xl.conf.5.pod.in               |  14 +++
> tools/console/Makefile                  |   4 +-
> tools/console/daemon/io.c               |  25 ++++-
> tools/helpers/init-xenstore-domain.c    |   1 +
> tools/include/libxl.h                   |  14 +++
> tools/libs/light/libxl_create.c         |   4 +
> tools/libs/light/libxl_dm.c             |   2 +
> tools/libs/light/libxl_dom.c            |   2 +-
> tools/libs/light/libxl_types.idl        |   2 +
> tools/ocaml/libs/xc/xenctrl.ml          |   5 +
> tools/ocaml/libs/xc/xenctrl.mli         |   5 +
> tools/ocaml/libs/xc/xenctrl_stubs.c     |  12 ++-
> tools/xenstore/Makefile                 |   4 +-
> tools/xenstore/include/xenstore_state.h |   1 +
> tools/xenstore/xenstored_core.h         |   2 +-
> tools/xenstore/xenstored_domain.c       | 120 +++++++++++++++---------
> tools/xl/xl.c                           |  15 +++
> tools/xl/xl.h                           |   2 +
> tools/xl/xl_parse.c                     |  13 +++
> xen/arch/arm/domain_build.c             |   4 +
> xen/arch/x86/setup.c                    |   2 +
> xen/common/domain.c                     |   4 +-
> xen/common/grant_table.c                | 119 ++++++++++++++++++++++-
> xen/include/public/domctl.h             |  13 ++-
> xen/include/xen/grant_table.h           |   7 +-
> 26 files changed, 341 insertions(+), 67 deletions(-)
> 
> -- 
> 2.33.0
> 

Re: [PATCH 0/6] gnttab: add per-domain controls

[Roger Pau Monné]

On Fri, Sep 17, 2021 at 06:06:42PM +0200, Christian Lindig wrote:
> 
> 
> > On 17 Sep 2021, at 16:46, Roger Pau Monne <roger.pau@citrix.com> wrote:
> > 
> > Hello,
> > 
> > The first two patches of this series allows setting the preisoutly host
> > wide command line `gnttab` option on a per domain basis. That means
> > selecting the max allowed grant table version and whether transitive
> > grants are allowed.
> > 
> > The last 4 patches attempt to implement support for creating guests
> > without grant table support at all. This requires some changes to
> > xenstore in order to map shared ring using foreign memory instead of
> > grant table.
> > 
> > Note that patch 5 will break the save format for xenstore records, and
> > should not be applied.
> 
> Has this relevance for the format used by oxenstored?

I'm no expert on oxenstored, but I think it has always mapped the
shared ring as foreign memory, and hence no changes are needed there.
AFAICT it also stores the mfn on the save format, so I think this is
all fine.

Should have mentioned it on the cover letter.

Thanks, Roger.

Re: [PATCH 0/6] gnttab: add per-domain controls

[Edwin Torok]

> On 20 Sep 2021, at 08:26, Roger Pau Monne <roger.pau@citrix.com> wrote:
> 
> On Fri, Sep 17, 2021 at 06:06:42PM +0200, Christian Lindig wrote:
>> 
>> 
>>> On 17 Sep 2021, at 16:46, Roger Pau Monne <roger.pau@citrix.com> wrote:
>>> 
>>> Hello,
>>> 
>>> The first two patches of this series allows setting the preisoutly host
>>> wide command line `gnttab` option on a per domain basis. That means
>>> selecting the max allowed grant table version and whether transitive
>>> grants are allowed.
>>> 
>>> The last 4 patches attempt to implement support for creating guests
>>> without grant table support at all. This requires some changes to
>>> xenstore in order to map shared ring using foreign memory instead of
>>> grant table.
>>> 
>>> Note that patch 5 will break the save format for xenstore records, and
>>> should not be applied.
>> 
>> Has this relevance for the format used by oxenstored?
> 
> I'm no expert on oxenstored, but I think it has always mapped the
> shared ring as foreign memory, and hence no changes are needed there.
> AFAICT it also stores the mfn on the save format, so I think this is
> all fine.
> 
> Should have mentioned it on the cover letter.
>  


There is a patch series from last year to make oxenstored use gnttab instead of map_foreign_range.
https://patchwork.kernel.org/project/xen-devel/cover/cover.1598548832.git.edvin.torok@citrix.com/
This got lost/forgotten amid all the oxenstored XSA work.

Later on I discovered and fixed some bugs in it, and is part of this refreshed patch series (part of which got committed, part of which didn't):
https://patchwork.kernel.org/project/xen-devel/list/?series=480623
https://github.com/edwintorok/xen/pull/2

I think the current status is:
* there was an objection that the commit vendoring the external dependencies for the unit tests was too big, and should be replaced by just an opam and lockfile telling 'opam' or 'opam monorepo' where to download it from
* I've discovered some bugs while testing this code together with other code, and need to retest with just this code alone to check that the bug was not in this code


As for the save format, that is part of this patch series too, and we don't store the mfn anymore. Do we need to go back to storing the mfn?

What do I need to change here? The reason to move away from foreign memory was that we could avoid relying on xenctrl for that function (and thus having one less unstable interface to link to). If we need to conditionally use foreign memory mapping then we're back to using unstable interfaces, unless there is a stable interface equivalent to mapping foreign pages?
I see there is a libs/foreignmemory (it has no OCaml bindings though). If we wrote OCaml bindings would the API/ABI of libs/foreignmemory be stable?
In which case we should probably replace the commit introducing the use of gnttab with the one using foreignmemory and always use foreignmemory instead of gnttab libs.

What do you think?

Best regards,
--Edwin