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

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

[Roger Pau Monne]

Hello,

First patch on the series is a trivial change to xenconsoled in order to
use xenforeignmemory stable library in order to map the shared console
ring instead of the unstable libxc interface. It's reviewed and ready to
go in.

Patches 2 and 3 allow setting the 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 3 patches attempt to implement support for creating guests
without a grant table. This requires some changes to xenstored in order
to partially support guests without a valid ring interface, as the lack
of grant table will prevent C xenstored from mapping the shared ring.
Note this is not an issue for Ocaml xenstored, as it still uses the
foreign memory interface to map the shared ring, and thus won't notice
the lack of grant table support on the domain.

Thanks, Roger.

Roger Pau Monne (6):
  tools/console: use xenforeigmemory to map console ring
  gnttab: allow setting max version per-domain
  gnttab: allow per-domain control over transitive grants
  tools/xenstored: use atexit to close interfaces
  tools/xenstored: partially handle domains without a shared ring
  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      |   6 ++
 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       |   2 +
 tools/ocaml/libs/xc/xenctrl.mli      |   2 +
 tools/ocaml/libs/xc/xenctrl_stubs.c  |  12 ++-
 tools/xenstore/xenstored_core.h      |   2 +-
 tools/xenstore/xenstored_domain.c    |  91 ++++++++++----------
 tools/xenstore/xenstored_minios.c    |   4 +-
 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                  |   3 +-
 xen/common/grant_table.c             | 122 ++++++++++++++++++++++++++-
 xen/include/public/domctl.h          |  15 +++-
 xen/include/xen/grant_table.h        |   5 +-
 25 files changed, 310 insertions(+), 66 deletions(-)

-- 
2.33.0

[PATCH v2 1/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>
Reviewed-by: Ian Jackson <iwj@xenproject.org>
---
 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

[PATCH v2 2/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.

Note the version is specified using 4 bits, which leaves room to
specify up to grant table version 14 (15 is used to signal default max
version). Given that we only have 2 grant table versions right now,
and a new version is unlikely in the near future using 4 bits seems
more than enough.

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.
---
Changes since v1:
 - Introduce a grant_opts field and use the low 4 bits to specify the
   version. Remaining bits will be used for other purposes.
---
 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      |  3 +++
 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  |  7 ++++++-
 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             | 22 ++++++++++++++++++++--
 xen/include/public/domctl.h          | 12 ++++++++++--
 xen/include/xen/grant_table.h        |  5 +++--
 19 files changed, 90 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..7cd1aa8f7c 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,
+        .grant_opts = 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..1ee86602ae 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -606,6 +606,9 @@ 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,
+            .grant_opts = b_info->max_grant_version == -1
+                          ? XEN_DOMCTL_GRANT_version_default
+                          : 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..1e60925069 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,9 @@ 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),
+		.grant_opts = Int_val(VAL_MAX_GRANT_VERSION) == -1
+			      ? XEN_DOMCTL_GRANT_version_default
+			      : Int_val(VAL_MAX_GRANT_VERSION),
 	};
 
 	domain_handle_of_uuid_string(cfg.handle, String_val(VAL_HANDLE));
@@ -251,6 +255,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 d233d634c1..3beb1cbb41 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2649,6 +2649,7 @@ void __init create_domUs(void)
             .max_evtchn_port = -1,
             .max_grant_frames = -1,
             .max_maptrack_frames = -1,
+            .grant_opts = XEN_DOMCTL_GRANT_version_default,
         };
 
         if ( !dt_device_is_compatible(node, "xen,domain") )
@@ -2756,6 +2757,7 @@ void __init create_dom0(void)
         .max_evtchn_port = -1,
         .max_grant_frames = gnttab_dom0_frames(),
         .max_maptrack_frames = -1,
+        .grant_opts = XEN_DOMCTL_GRANT_version_default,
     };
 
     /* 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 b101565f14..b5b6c75447 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,
+        .grant_opts = XEN_DOMCTL_GRANT_version_default,
         .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..6519272c47 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->grant_opts) != 0 ) )
             goto fail;
         init_status |= INIT_gnttab;
 
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index fe1fc11b22..c43e9d5ee4 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,27 @@ active_alloc_failed:
 }
 
 int grant_table_init(struct domain *d, int max_grant_frames,
-                     int max_maptrack_frames)
+                     int max_maptrack_frames, unsigned int options)
 {
     struct grant_table *gt;
+    unsigned int max_grant_version = options & XEN_DOMCTL_GRANT_version_mask;
     int ret = -ENOMEM;
 
+    if ( max_grant_version == XEN_DOMCTL_GRANT_version_default )
+        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 +1964,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 +3094,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..e2b47184a0 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -87,14 +87,22 @@ 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;
 
+/* Grant version, use low 4 bits. */
+#define XEN_DOMCTL_GRANT_version_mask    0xf
+#define XEN_DOMCTL_GRANT_version_default 0xf
+
+#define XEN_DOMCTLGRANT_MAX XEN_DOMCTL_GRANT_version_mask
+
+    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 41713e2726..fe6225346f 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, unsigned int options);
 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,
+                                   unsigned int options)
 {
     return 0;
 }
-- 
2.33.0

[PATCH v2 3/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      | 1 +
 tools/ocaml/libs/xc/xenctrl.mli     | 1 +
 tools/ocaml/libs/xc/xenctrl_stubs.c | 7 ++++++-
 tools/xl/xl.c                       | 7 +++++++
 tools/xl/xl.h                       | 1 +
 tools/xl/xl_parse.c                 | 4 ++++
 xen/arch/arm/domain_build.c         | 6 ++++--
 xen/arch/x86/setup.c                | 3 ++-
 xen/common/grant_table.c            | 8 +++++++-
 xen/include/public/domctl.h         | 5 ++++-
 16 files changed, 62 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 1ee86602ae..fa527923e4 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -635,6 +635,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..da12b67baf 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -84,6 +84,7 @@ type domctl_create_config =
 	max_grant_frames: int;
 	max_maptrack_frames: int;
 	max_grant_version: int;
+	transitive_grants: bool;
 	arch: arch_domainconfig;
 }
 
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index 5933d32c70..83ca554eb7 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -76,6 +76,7 @@ type domctl_create_config = {
   max_grant_frames: int;
   max_maptrack_frames: int;
   max_grant_version: int;
+  transitive_grants: bool;
   arch: arch_domainconfig;
 }
 
diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index 1e60925069..5697a8bd8b 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_TRANSITIVE_GRANTS   Field(config, 9)
+#define VAL_ARCH                Field(config, 10)
 
 	uint32_t domid = Int_val(wanted_domid);
 	int result;
@@ -216,6 +217,9 @@ 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 |= Bool_val(VAL_TRANSITIVE_GRANTS)
+			  ? XEN_DOMCTL_GRANT_transitive : 0;
+
 	arch_domconfig = Field(VAL_ARCH, 0);
 	switch ( Tag_val(VAL_ARCH) )
 	{
@@ -255,6 +259,7 @@ CAMLprim value stub_xc_domain_create(value xch, value wanted_domid, value config
 	}
 
 #undef VAL_ARCH
+#undef VAL_TRANSITIVE_GRANTS
 #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 3beb1cbb41..b9048aa9cc 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2649,7 +2649,8 @@ void __init create_domUs(void)
             .max_evtchn_port = -1,
             .max_grant_frames = -1,
             .max_maptrack_frames = -1,
-            .grant_opts = XEN_DOMCTL_GRANT_version_default,
+            .grant_opts = XEN_DOMCTL_GRANT_version_default |
+                          XEN_DOMCTL_GRANT_transitive,
         };
 
         if ( !dt_device_is_compatible(node, "xen,domain") )
@@ -2757,7 +2758,8 @@ void __init create_dom0(void)
         .max_evtchn_port = -1,
         .max_grant_frames = gnttab_dom0_frames(),
         .max_maptrack_frames = -1,
-        .grant_opts = XEN_DOMCTL_GRANT_version_default,
+        .grant_opts = XEN_DOMCTL_GRANT_version_default |
+                      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 b5b6c75447..7ce54a5a57 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -750,7 +750,8 @@ static struct domain *__init create_dom0(const module_t *image,
         .max_evtchn_port = -1,
         .max_grant_frames = -1,
         .max_maptrack_frames = -1,
-        .grant_opts = XEN_DOMCTL_GRANT_version_default,
+        .grant_opts = XEN_DOMCTL_GRANT_version_default |
+                      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/grant_table.c b/xen/common/grant_table.c
index c43e9d5ee4..f50e9f6a06 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;
@@ -1965,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;
@@ -2887,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 e2b47184a0..2f90e4c3f8 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -98,8 +98,11 @@ struct xen_domctl_createdomain {
 /* Grant version, use low 4 bits. */
 #define XEN_DOMCTL_GRANT_version_mask    0xf
 #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_version_mask
+#define XEN_DOMCTLGRANT_MAX XEN_DOMCTL_GRANT_transitive
 
     uint32_t grant_opts;
 
-- 
2.33.0

[PATCH v2 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. While there also close the
event channel interface.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Fix minios xenstored build.
 - Also close event channel handle
---
 tools/xenstore/xenstored_core.h   |  2 +-
 tools/xenstore/xenstored_domain.c | 61 ++++++++++++++-----------------
 tools/xenstore/xenstored_minios.c |  4 +-
 3 files changed, 31 insertions(+), 36 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..9fb78d5772 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,42 @@ 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;
+	}
+	if (xce_handle) {
+		xenevtchn_close(xce_handle);
+		xce_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);
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);
 }
 
-- 
2.33.0

[PATCH v2 5/6] tools/xenstored: partially handle domains without a shared ring

[Roger Pau Monne]

Failure to map the shared ring and thus establish a xenstore
connection with a domain shouldn't prevent the "@introduceDomain"
watch from firing, likewise with "@releaseDomain".

In order to handle such events properly xenstored should keep track of
the domains even if the shared communication ring cannot be mapped.
This was partially the case due to the restore mode, which needs to
handle domains that have been destroyed between the save and restore
period. This patch extends on the previous limited support of
temporary adding a domain without a valid interface ring, and modifies
check_domains to keep domains without an interface on the list.

Handling domains without a valid shared ring is required in order to
support domain without a grant table, since the lack of grant table
will prevent the mapping of the xenstore ring grant reference.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
oxenstored will need a similar treatment once grant mapping is used
there. For the time being it still works correctly because it uses
foreign memory to map the shared ring, and that will work in the
absence of grant tables on the domain.
---
Changes since v1:
 - New in this version.
---
 tools/xenstore/xenstored_domain.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index 9fb78d5772..150c6f082e 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -119,6 +119,11 @@ static int writechn(struct connection *conn,
 	struct xenstore_domain_interface *intf = conn->domain->interface;
 	XENSTORE_RING_IDX cons, prod;
 
+	if (!intf) {
+		errno = ENODEV;
+		return -1;
+	}
+
 	/* Must read indexes once, and before anything else, and verified. */
 	cons = intf->rsp_cons;
 	prod = intf->rsp_prod;
@@ -149,6 +154,11 @@ static int readchn(struct connection *conn, void *data, unsigned int len)
 	struct xenstore_domain_interface *intf = conn->domain->interface;
 	XENSTORE_RING_IDX cons, prod;
 
+	if (!intf) {
+		errno = ENODEV;
+		return -1;
+	}
+
 	/* Must read indexes once, and before anything else, and verified. */
 	cons = intf->req_cons;
 	prod = intf->req_prod;
@@ -176,6 +186,9 @@ static bool domain_can_write(struct connection *conn)
 {
 	struct xenstore_domain_interface *intf = conn->domain->interface;
 
+	if (!intf)
+		return false;
+
 	return ((intf->rsp_prod - intf->rsp_cons) != XENSTORE_RING_SIZE);
 }
 
@@ -183,7 +196,8 @@ static bool domain_can_read(struct connection *conn)
 {
 	struct xenstore_domain_interface *intf = conn->domain->interface;
 
-	if (domain_is_unprivileged(conn) && conn->domain->wrl_credit < 0)
+	if ((domain_is_unprivileged(conn) && conn->domain->wrl_credit < 0) ||
+	    !intf)
 		return false;
 
 	return (intf->req_cons != intf->req_prod);
@@ -268,14 +282,7 @@ void check_domains(void)
 				domain->shutdown = true;
 				notify = 1;
 			}
-			/*
-			 * On Restore, we may have been unable to remap the
-			 * interface and the port. As we don't know whether
-			 * this was because of a dying domain, we need to
-			 * check if the interface and port are still valid.
-			 */
-			if (!dominfo.dying && domain->port &&
-			    domain->interface)
+			if (!dominfo.dying)
 				continue;
 		}
 		if (domain->conn) {
@@ -450,8 +457,6 @@ static struct domain *introduce_domain(const void *ctx,
 	if (!domain->introduced) {
 		interface = is_master_domain ? xenbus_map()
 					     : map_interface(domid);
-		if (!interface && !restore)
-			return NULL;
 		if (new_domain(domain, port, restore)) {
 			rc = errno;
 			if (interface) {
@@ -505,7 +510,8 @@ int do_introduce(struct connection *conn, struct buffered_data *in)
 	if (!domain)
 		return errno;
 
-	domain_conn_reset(domain);
+	if (domain->interface)
+		domain_conn_reset(domain);
 
 	send_ack(conn, XS_INTRODUCE);
 
-- 
2.33.0

[PATCH v2 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     | 100 +++++++++++++++++++++++++++++++++--
 3 files changed, 98 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 f50e9f6a06..df01d03ce4 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 == XEN_DOMCTL_GRANT_version_default )
         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) )
@@ -4200,6 +4287,9 @@ static void gnttab_usage_print(struct domain *rd)
     struct grant_table *gt = rd->grant_table;
     unsigned int nr_ents;
 
+    if ( !gt )
+        return;
+
     printk("      -------- active --------       -------- shared --------\n");
     printk("[ref] localdom mfn      pin          localdom gmfn     flags\n");
 
-- 
2.33.0

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

[Julien Grall]

On 22/09/2021 13:21, Roger Pau Monne wrote:
> Hello,

Hi Roger,

> First patch on the series is a trivial change to xenconsoled in order to
> use xenforeignmemory stable library in order to map the shared console
> ring instead of the unstable libxc interface. It's reviewed and ready to
> go in.
> 
> Patches 2 and 3 allow setting the 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 3 patches attempt to implement support for creating guests
> without a grant table. This requires some changes to xenstored in order
> to partially support guests without a valid ring interface, as the lack
> of grant table will prevent C xenstored from mapping the shared ring.
> Note this is not an issue for Ocaml xenstored, as it still uses the
> foreign memory interface to map the shared ring, and thus won't notice
> the lack of grant table support on the domain.

I find a bit odd that the Xenstore support is conditional to whether 
grant table is available. Are you expecting domains with no grant table 
to have no PV drivers (including PV shutdown)?

Cheers,

-- 
Julien Grall

Re: [PATCH v2 5/6] tools/xenstored: partially handle domains without a shared ring

[Julien Grall]

Hi Roger,

On 22/09/2021 13:21, Roger Pau Monne wrote:
> Failure to map the shared ring and thus establish a xenstore
> connection with a domain shouldn't prevent the "@introduceDomain"
> watch from firing, likewise with "@releaseDomain".
> 
> In order to handle such events properly xenstored should keep track of
> the domains even if the shared communication ring cannot be mapped.
> This was partially the case due to the restore mode, which needs to
> handle domains that have been destroyed between the save and restore
> period. This patch extends on the previous limited support of
> temporary adding a domain without a valid interface ring, and modifies
> check_domains to keep domains without an interface on the list.
> 
> Handling domains without a valid shared ring is required in order to
> support domain without a grant table, since the lack of grant table
> will prevent the mapping of the xenstore ring grant reference.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> oxenstored will need a similar treatment once grant mapping is used
> there. For the time being it still works correctly because it uses
> foreign memory to map the shared ring, and that will work in the
> absence of grant tables on the domain.
> ---
> Changes since v1:
>   - New in this version.
> ---
>   tools/xenstore/xenstored_domain.c | 30 ++++++++++++++++++------------
>   1 file changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
> index 9fb78d5772..150c6f082e 100644
> --- a/tools/xenstore/xenstored_domain.c
> +++ b/tools/xenstore/xenstored_domain.c
> @@ -119,6 +119,11 @@ static int writechn(struct connection *conn,
>   	struct xenstore_domain_interface *intf = conn->domain->interface;
>   	XENSTORE_RING_IDX cons, prod;
>   
> +	if (!intf) {
> +		errno = ENODEV;
> +		return -1;
> +	}
> +
>   	/* Must read indexes once, and before anything else, and verified. */
>   	cons = intf->rsp_cons;
>   	prod = intf->rsp_prod;
> @@ -149,6 +154,11 @@ static int readchn(struct connection *conn, void *data, unsigned int len)
>   	struct xenstore_domain_interface *intf = conn->domain->interface;
>   	XENSTORE_RING_IDX cons, prod;
>   
> +	if (!intf) {
> +		errno = ENODEV;
> +		return -1;
> +	}
> +
>   	/* Must read indexes once, and before anything else, and verified. */
>   	cons = intf->req_cons;
>   	prod = intf->req_prod;
> @@ -176,6 +186,9 @@ static bool domain_can_write(struct connection *conn)
>   {
>   	struct xenstore_domain_interface *intf = conn->domain->interface;
>   
> +	if (!intf)
> +		return false;
> +

Rather than adding an extra check, how about taking advantage of is_ignore?

>   	return ((intf->rsp_prod - intf->rsp_cons) != XENSTORE_RING_SIZE);
>   }
>   
> @@ -183,7 +196,8 @@ static bool domain_can_read(struct connection *conn)
>   {
>   	struct xenstore_domain_interface *intf = conn->domain->interface;
>   
> -	if (domain_is_unprivileged(conn) && conn->domain->wrl_credit < 0)
> +	if ((domain_is_unprivileged(conn) && conn->domain->wrl_credit < 0) ||
> +	    !intf)
>   		return false;
>   
>   	return (intf->req_cons != intf->req_prod);
> @@ -268,14 +282,7 @@ void check_domains(void)
>   				domain->shutdown = true;
>   				notify = 1;
>   			}
> -			/*
> -			 * On Restore, we may have been unable to remap the
> -			 * interface and the port. As we don't know whether
> -			 * this was because of a dying domain, we need to
> -			 * check if the interface and port are still valid.
> -			 */
> -			if (!dominfo.dying && domain->port &&
> -			    domain->interface)
> +			if (!dominfo.dying)
>   				continue;

This is mostly a revert on "tools/xenstore: handle dying domains in live 
update". However, IIRC, this check was necessary to release the 
connection if the domain has died in the middle of Live-Update.

So I think this check should stick here. Instead, I think, we should 
mark the "fake domain" so we can ignore them here.

>   		}
>   		if (domain->conn) {
> @@ -450,8 +457,6 @@ static struct domain *introduce_domain(const void *ctx,
>   	if (!domain->introduced) {
>   		interface = is_master_domain ? xenbus_map()
>   					     : map_interface(domid);
> -		if (!interface && !restore)
> -			return NULL;
>   		if (new_domain(domain, port, restore)) {
>   			rc = errno;
>   			if (interface) {
> @@ -505,7 +510,8 @@ int do_introduce(struct connection *conn, struct buffered_data *in)
>   	if (!domain)
>   		return errno;
>   
> -	domain_conn_reset(domain);
> +	if (domain->interface)
> +		domain_conn_reset(domain);
>   
>   	send_ack(conn, XS_INTRODUCE);
>   
> 

Cheers,

-- 
Julien Grall

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

[Julien Grall]

Hi Roger,

On 22/09/2021 13:21, Roger Pau Monne wrote:
> 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     | 100 +++++++++++++++++++++++++++++++++--
>   3 files changed, 98 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)>.

Technically, the version only applies to format of the table for 
granting page. The mapping itself is version agnostic. So this feels a 
bit wrong to use max_grant_version to not allocate d->grant_table.

I also can see use-cases where we may want to allow a domain to grant 
page but not map grant (for instance, a further hardening of XSA-380). 
Therefore, I think we want to keep max_grant_version for the table 
itself and manage the mappings separately (possibly by letting the admin 
to select the max number of entries in the maptrack).

Cheers,

-- 
Julien Grall

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

[Christian Lindig]

> On 22 Sep 2021, at 09:21, Roger Pau Monne <roger.pau@citrix.com> wrote:
> 
> 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      | 1 +
> tools/ocaml/libs/xc/xenctrl.mli     | 1 +
> tools/ocaml/libs/xc/xenctrl_stubs.c | 7 ++++++-
> tools/xl/xl.c                       | 7 +++++++
> tools/xl/xl.h                       | 1 +
> tools/xl/xl_parse.c                 | 4 ++++
> xen/arch/arm/domain_build.c         | 6 ++++--
> xen/arch/x86/setup.c                | 3 ++-
> xen/common/grant_table.c            | 8 +++++++-
> xen/include/public/domctl.h         | 5 ++++-
> 16 files changed, 62 insertions(+), 6 deletions(-)

Acked-by: Christian Lindig <christian.lindig@citrix.com>

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

[Juergen Gross]

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

On 22.09.21 11:19, Julien Grall wrote:
> Hi Roger,
> 
> On 22/09/2021 13:21, Roger Pau Monne wrote:
>> 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     | 100 +++++++++++++++++++++++++++++++++--
>>   3 files changed, 98 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)>.
> 
> Technically, the version only applies to format of the table for 
> granting page. The mapping itself is version agnostic. So this feels a 
> bit wrong to use max_grant_version to not allocate d->grant_table.
> 
> I also can see use-cases where we may want to allow a domain to grant 
> page but not map grant (for instance, a further hardening of XSA-380). 
> Therefore, I think we want to keep max_grant_version for the table 
> itself and manage the mappings separately (possibly by letting the admin 
> to select the max number of entries in the maptrack).

You mean the already existing domain config option max_maptrack_frames?


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 v2 0/6] gnttab: add per-domain controls

[Roger Pau Monné]

On Wed, Sep 22, 2021 at 01:57:02PM +0500, Julien Grall wrote:
> 
> 
> On 22/09/2021 13:21, Roger Pau Monne wrote:
> > Hello,
> 
> Hi Roger,
> 
> > First patch on the series is a trivial change to xenconsoled in order to
> > use xenforeignmemory stable library in order to map the shared console
> > ring instead of the unstable libxc interface. It's reviewed and ready to
> > go in.
> > 
> > Patches 2 and 3 allow setting the 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 3 patches attempt to implement support for creating guests
> > without a grant table. This requires some changes to xenstored in order
> > to partially support guests without a valid ring interface, as the lack
> > of grant table will prevent C xenstored from mapping the shared ring.
> > Note this is not an issue for Ocaml xenstored, as it still uses the
> > foreign memory interface to map the shared ring, and thus won't notice
> > the lack of grant table support on the domain.
> 
> I find a bit odd that the Xenstore support is conditional to whether grant
> table is available. Are you expecting domains with no grant table to have no
> PV drivers (including PV shutdown)?

I don't really expect much, as having guests without grant table is a
developer option right now, if someone wants to make use of them for
any reason it would need some thought.

The other option would be my first proposal to restore foreign mapping
of the xenstore ring on that case:

https://lore.kernel.org/xen-devel/20210917154625.89315-6-roger.pau@citrix.com/

But it's also arguable that a guest not having a grant table should
also likely prevent foreign mapping attempts. Plus such foreign
mapping won't work from stubdomains.

I'm fine with dropping those patches if they turn out to be too
controversial, I think it's an interesting option to be able to
disable the grant table, but I don't have a full picture of how that
could be used in practice. Maybe others have and would be willing to
pick this up.

The xenstored patch is mostly so that I can boot guests without a
grant table using xl and test it's disabled using XTF.

Regards, Roger.

Re: [PATCH v2 5/6] tools/xenstored: partially handle domains without a shared ring

[Roger Pau Monné]

On Wed, Sep 22, 2021 at 02:07:44PM +0500, Julien Grall wrote:
> Hi Roger,
> 
> On 22/09/2021 13:21, Roger Pau Monne wrote:
> > Failure to map the shared ring and thus establish a xenstore
> > connection with a domain shouldn't prevent the "@introduceDomain"
> > watch from firing, likewise with "@releaseDomain".
> > 
> > In order to handle such events properly xenstored should keep track of
> > the domains even if the shared communication ring cannot be mapped.
> > This was partially the case due to the restore mode, which needs to
> > handle domains that have been destroyed between the save and restore
> > period. This patch extends on the previous limited support of
> > temporary adding a domain without a valid interface ring, and modifies
> > check_domains to keep domains without an interface on the list.
> > 
> > Handling domains without a valid shared ring is required in order to
> > support domain without a grant table, since the lack of grant table
> > will prevent the mapping of the xenstore ring grant reference.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > oxenstored will need a similar treatment once grant mapping is used
> > there. For the time being it still works correctly because it uses
> > foreign memory to map the shared ring, and that will work in the
> > absence of grant tables on the domain.
> > ---
> > Changes since v1:
> >   - New in this version.
> > ---
> >   tools/xenstore/xenstored_domain.c | 30 ++++++++++++++++++------------
> >   1 file changed, 18 insertions(+), 12 deletions(-)
> > 
> > diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
> > index 9fb78d5772..150c6f082e 100644
> > --- a/tools/xenstore/xenstored_domain.c
> > +++ b/tools/xenstore/xenstored_domain.c
> > @@ -119,6 +119,11 @@ static int writechn(struct connection *conn,
> >   	struct xenstore_domain_interface *intf = conn->domain->interface;
> >   	XENSTORE_RING_IDX cons, prod;
> > +	if (!intf) {
> > +		errno = ENODEV;
> > +		return -1;
> > +	}
> > +
> >   	/* Must read indexes once, and before anything else, and verified. */
> >   	cons = intf->rsp_cons;
> >   	prod = intf->rsp_prod;
> > @@ -149,6 +154,11 @@ static int readchn(struct connection *conn, void *data, unsigned int len)
> >   	struct xenstore_domain_interface *intf = conn->domain->interface;
> >   	XENSTORE_RING_IDX cons, prod;
> > +	if (!intf) {
> > +		errno = ENODEV;
> > +		return -1;
> > +	}
> > +
> >   	/* Must read indexes once, and before anything else, and verified. */
> >   	cons = intf->req_cons;
> >   	prod = intf->req_prod;
> > @@ -176,6 +186,9 @@ static bool domain_can_write(struct connection *conn)
> >   {
> >   	struct xenstore_domain_interface *intf = conn->domain->interface;
> > +	if (!intf)
> > +		return false;
> > +
> 
> Rather than adding an extra check, how about taking advantage of is_ignore?

Right, I just need to change the order in conn_can_read and
conn_can_write so that the is_ignored check is performed before the
can_{read,write} handler is called.

> >   	return ((intf->rsp_prod - intf->rsp_cons) != XENSTORE_RING_SIZE);
> >   }
> > @@ -183,7 +196,8 @@ static bool domain_can_read(struct connection *conn)
> >   {
> >   	struct xenstore_domain_interface *intf = conn->domain->interface;
> > -	if (domain_is_unprivileged(conn) && conn->domain->wrl_credit < 0)
> > +	if ((domain_is_unprivileged(conn) && conn->domain->wrl_credit < 0) ||
> > +	    !intf)
> >   		return false;
> >   	return (intf->req_cons != intf->req_prod);
> > @@ -268,14 +282,7 @@ void check_domains(void)
> >   				domain->shutdown = true;
> >   				notify = 1;
> >   			}
> > -			/*
> > -			 * On Restore, we may have been unable to remap the
> > -			 * interface and the port. As we don't know whether
> > -			 * this was because of a dying domain, we need to
> > -			 * check if the interface and port are still valid.
> > -			 */
> > -			if (!dominfo.dying && domain->port &&
> > -			    domain->interface)
> > +			if (!dominfo.dying)
> >   				continue;
> 
> This is mostly a revert on "tools/xenstore: handle dying domains in live
> update". However, IIRC, this check was necessary to release the connection
> if the domain has died in the middle of Live-Update.

But if the domain has died in the middle of live update
get_domain_info will return false, and thus the code won't get here.

If the domain is in the process of being removed (ie: dominfo.shutdown
being set for example), it would eventually get into dominfo.dying and
thus be removed normally.

I assumed those checks where needed in order to prevent having a
domain without a valid interface on the tracked list while it was on
the process of being removed. With the other changes on this patch
tracking a domain without a valid interface should be fine, and it
will eventually be removed as part of the normal flow.

Thanks, Roger.

Re: [PATCH v2 5/6] tools/xenstored: partially handle domains without a shared ring

[Julien Grall]

Hi Roger,

On 22/09/2021 14:58, Roger Pau Monné wrote:
> On Wed, Sep 22, 2021 at 02:07:44PM +0500, Julien Grall wrote:
>> Hi Roger,
>>
>> On 22/09/2021 13:21, Roger Pau Monne wrote:
>>> Failure to map the shared ring and thus establish a xenstore
>>> connection with a domain shouldn't prevent the "@introduceDomain"
>>> watch from firing, likewise with "@releaseDomain".
>>>
>>> In order to handle such events properly xenstored should keep track of
>>> the domains even if the shared communication ring cannot be mapped.
>>> This was partially the case due to the restore mode, which needs to
>>> handle domains that have been destroyed between the save and restore
>>> period. This patch extends on the previous limited support of
>>> temporary adding a domain without a valid interface ring, and modifies
>>> check_domains to keep domains without an interface on the list.
>>>
>>> Handling domains without a valid shared ring is required in order to
>>> support domain without a grant table, since the lack of grant table
>>> will prevent the mapping of the xenstore ring grant reference.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> ---
>>> oxenstored will need a similar treatment once grant mapping is used
>>> there. For the time being it still works correctly because it uses
>>> foreign memory to map the shared ring, and that will work in the
>>> absence of grant tables on the domain.
>>> ---
>>> Changes since v1:
>>>    - New in this version.
>>> ---
>>>    tools/xenstore/xenstored_domain.c | 30 ++++++++++++++++++------------
>>>    1 file changed, 18 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
>>> index 9fb78d5772..150c6f082e 100644
>>> --- a/tools/xenstore/xenstored_domain.c
>>> +++ b/tools/xenstore/xenstored_domain.c
>>> @@ -119,6 +119,11 @@ static int writechn(struct connection *conn,
>>>    	struct xenstore_domain_interface *intf = conn->domain->interface;
>>>    	XENSTORE_RING_IDX cons, prod;
>>> +	if (!intf) {
>>> +		errno = ENODEV;
>>> +		return -1;
>>> +	}
>>> +
>>>    	/* Must read indexes once, and before anything else, and verified. */
>>>    	cons = intf->rsp_cons;
>>>    	prod = intf->rsp_prod;
>>> @@ -149,6 +154,11 @@ static int readchn(struct connection *conn, void *data, unsigned int len)
>>>    	struct xenstore_domain_interface *intf = conn->domain->interface;
>>>    	XENSTORE_RING_IDX cons, prod;
>>> +	if (!intf) {
>>> +		errno = ENODEV;
>>> +		return -1;
>>> +	}
>>> +
>>>    	/* Must read indexes once, and before anything else, and verified. */
>>>    	cons = intf->req_cons;
>>>    	prod = intf->req_prod;
>>> @@ -176,6 +186,9 @@ static bool domain_can_write(struct connection *conn)
>>>    {
>>>    	struct xenstore_domain_interface *intf = conn->domain->interface;
>>> +	if (!intf)
>>> +		return false;
>>> +
>>
>> Rather than adding an extra check, how about taking advantage of is_ignore?
> 
> Right, I just need to change the order in conn_can_read and
> conn_can_write so that the is_ignored check is performed before the
> can_{read,write} handler is called.
> 
>>>    	return ((intf->rsp_prod - intf->rsp_cons) != XENSTORE_RING_SIZE);
>>>    }
>>> @@ -183,7 +196,8 @@ static bool domain_can_read(struct connection *conn)
>>>    {
>>>    	struct xenstore_domain_interface *intf = conn->domain->interface;
>>> -	if (domain_is_unprivileged(conn) && conn->domain->wrl_credit < 0)
>>> +	if ((domain_is_unprivileged(conn) && conn->domain->wrl_credit < 0) ||
>>> +	    !intf)
>>>    		return false;
>>>    	return (intf->req_cons != intf->req_prod);
>>> @@ -268,14 +282,7 @@ void check_domains(void)
>>>    				domain->shutdown = true;
>>>    				notify = 1;
>>>    			}
>>> -			/*
>>> -			 * On Restore, we may have been unable to remap the
>>> -			 * interface and the port. As we don't know whether
>>> -			 * this was because of a dying domain, we need to
>>> -			 * check if the interface and port are still valid.
>>> -			 */
>>> -			if (!dominfo.dying && domain->port &&
>>> -			    domain->interface)
>>> +			if (!dominfo.dying)
>>>    				continue;
>>
>> This is mostly a revert on "tools/xenstore: handle dying domains in live
>> update". However, IIRC, this check was necessary to release the connection
>> if the domain has died in the middle of Live-Update.
> 
> But if the domain has died in the middle of live update
> get_domain_info will return false, and thus the code won't get here.

Hmmm... I think I am mixing up a few things... I went through the 
original discussion (it was on the security ML) to find out why I wrote 
the patch like that. When going through the archives, I noticed that I 
provided a different version of this patch (see [1]) because there was 
some issue with the check here (I wrote that it would lead to zombie 
domain, but don't have the rationale :().

Juergen, I don't seem to find the reason why the patch was not replaced 
as we discussed on the security ML. Do you remember why?

Assuming this was a mistake, could someone take care of sending an 
update? If not, I could do it when I am back in October.

For the archives, the issues would appear when shutting down a domain 
during Live-Update.

[1]

commit 3d1f5b71f8565787c0cf305e5571884d6aec6865
Author: Julien Grall <jgrall@amazon.com>
Date:   Thu Jun 11 16:13:10 2020 +0200

      tools/xenstore: handle dying domains in live update

      A domain could just be dying when live updating Xenstore, so the case
      of not being able to map the ring page or to connect to the event
      channel must be handled gracefully.

      Signed-off-by: Julien Grall <jgrall@amazon.com>

diff --git a/tools/xenstore/xenstored_control.c
b/tools/xenstore/xenstored_control.c
index d1187a4346b4..92dae6be6195 100644
--- a/tools/xenstore/xenstored_control.c
+++ b/tools/xenstore/xenstored_control.c
@@ -533,6 +533,13 @@ void lu_read_state(void)
         lu_close_dump_state(&state);

         talloc_free(ctx);
+
+       /*
+        * We may have missed the VIRQ_DOM_EXC notification and a domain may
+        * have died while we were live-updating. So check all the 
domains are
+        * still alive.
+        */
+       check_domains();
   }

   static const char *lu_activate_binary(const void *ctx)
diff --git a/tools/xenstore/xenstored_domain.c
b/tools/xenstore/xenstored_domain.c
index 4976ae420800..0519e88eb819 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -224,7 +224,7 @@ static bool get_domain_info(unsigned int domid,
xc_dominfo_t *dominfo)
                dominfo->domid == domid;
   }

-static void domain_cleanup(void)
+void check_domains()
   {
         xc_dominfo_t dominfo;
         struct domain *domain;
@@ -248,6 +248,7 @@ static void domain_cleanup(void)
                                 domain->shutdown = true;
                                 notify = 1;
                         }
+
                         if (!dominfo.dying)
                                 continue;
                 }
@@ -274,7 +275,7 @@ void handle_event(void)
                 barf_perror("Failed to read from event fd");

         if (port == virq_port)
-               domain_cleanup();
+               check_domains();

         if (xenevtchn_unmask(xce_handle, port) == -1)
                 barf_perror("Failed to write to event fd");
@@ -359,7 +360,7 @@ 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)
+static int new_domain(struct domain *domain, int port, bool restore)
   {
         int rc;

@@ -375,9 +376,10 @@ static int new_domain(struct domain *domain, int port)

         /* Tell kernel we're interested in this event. */
         rc = xenevtchn_bind_interdomain(xce_handle, domain->domid, port);
-       if (rc == -1)
+       if (rc != -1)
+               domain->port = rc;
+       else if (!restore)
                 return errno;
-       domain->port = rc;

         domain->introduced = true;

@@ -428,7 +430,7 @@ static void domain_conn_reset(struct domain *domain)

   static struct domain *introduce_domain(const void *ctx,
                                        unsigned int domid,
-                                      evtchn_port_t port, bool fire_watch)
+                                      evtchn_port_t port, bool restore)
   {
         struct domain *domain;
         int rc;
@@ -440,11 +442,12 @@ static struct domain *introduce_domain(const void
*ctx,

         if (!domain->introduced) {
                 interface = map_interface(domid);
-               if (!interface)
+               if (!interface && !restore)
                         return NULL;
-               if (new_domain(domain, port)) {
+               if (new_domain(domain, port, restore)) {
                         rc = errno;
-                       unmap_interface(interface);
+                       if (interface)
+                               unmap_interface(interface);
                         errno = rc;
                         return NULL;
                 }
@@ -453,7 +456,7 @@ static struct domain *introduce_domain(const void *ctx,
                 /* Now domain belongs to its connection. */
                 talloc_steal(domain->conn, domain);

-               if (fire_watch)
+               if (!restore)
                         fire_watches(NULL, ctx, "@introduceDomain", NULL,
                                     false, NULL);
         } else {
@@ -490,7 +493,7 @@ int do_introduce(struct connection *conn, struct
buffered_data *in)
         if (port <= 0)
                 return EINVAL;

-       domain = introduce_domain(in, domid, port, true);
+       domain = introduce_domain(in, domid, port, false);
         if (!domain)
                 return errno;

@@ -730,7 +733,7 @@ static int dom0_init(void)
         dom0 = alloc_domain(NULL, xenbus_master_domid());
         if (!dom0)
                 return -1;
-       if (new_domain(dom0, port))
+       if (new_domain(dom0, port, false))
                 return -1;

         dom0->interface = xenbus_map();
@@ -1298,10 +1301,20 @@ void read_state_connection(const void *ctx,
const void *state)
                 conn = domain->conn;
         } else {
                 domain = introduce_domain(ctx, sc->spec.ring.domid,
-                                         sc->spec.ring.evtchn, false);
+                                         sc->spec.ring.evtchn, true);
                 if (!domain)
                         barf("domain allocation error");

+               conn = domain->conn;
+
+               /*
+                * The domain is about to die if we didn't manage to
+                * map the interface or the port. Mark the domain as
+                * ignored so it will be cleaned up when the domain
+                * is dead.
+                */
+               conn->is_ignored = !(domain->port && domain->interface);
+
                 if (sc->spec.ring.tdomid != DOMID_INVALID) {
                         tdomain = find_or_alloc_domain(ctx,
 
sc->spec.ring.tdomid);
@@ -1310,7 +1323,6 @@ void read_state_connection(const void *ctx, const
void *state)
                         talloc_reference(domain->conn, tdomain->conn);
                         domain->conn->target = tdomain->conn;
                 }
-               conn = domain->conn;
         }

         conn->conn_id = sc->conn_id;
diff --git a/tools/xenstore/xenstored_domain.h
b/tools/xenstore/xenstored_domain.h
index b71ab6d8a140..ec3b95a97195 100644
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -21,6 +21,8 @@

   void handle_event(void);

+void check_domains(void);
+
   /* domid, mfn, eventchn, path */
   int do_introduce(struct connection *conn, struct buffered_data *in);

Cheers,

-- 
Julien Grall

Re: [PATCH v2 5/6] tools/xenstored: partially handle domains without a shared ring

[Juergen Gross]

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

On 22.09.21 12:23, Julien Grall wrote:
> Hi Roger,
> 
> On 22/09/2021 14:58, Roger Pau Monné wrote:
>> On Wed, Sep 22, 2021 at 02:07:44PM +0500, Julien Grall wrote:
>>> Hi Roger,
>>>
>>> On 22/09/2021 13:21, Roger Pau Monne wrote:
>>>> Failure to map the shared ring and thus establish a xenstore
>>>> connection with a domain shouldn't prevent the "@introduceDomain"
>>>> watch from firing, likewise with "@releaseDomain".
>>>>
>>>> In order to handle such events properly xenstored should keep track of
>>>> the domains even if the shared communication ring cannot be mapped.
>>>> This was partially the case due to the restore mode, which needs to
>>>> handle domains that have been destroyed between the save and restore
>>>> period. This patch extends on the previous limited support of
>>>> temporary adding a domain without a valid interface ring, and modifies
>>>> check_domains to keep domains without an interface on the list.
>>>>
>>>> Handling domains without a valid shared ring is required in order to
>>>> support domain without a grant table, since the lack of grant table
>>>> will prevent the mapping of the xenstore ring grant reference.
>>>>
>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>> ---
>>>> oxenstored will need a similar treatment once grant mapping is used
>>>> there. For the time being it still works correctly because it uses
>>>> foreign memory to map the shared ring, and that will work in the
>>>> absence of grant tables on the domain.
>>>> ---
>>>> Changes since v1:
>>>>    - New in this version.
>>>> ---
>>>>    tools/xenstore/xenstored_domain.c | 30 
>>>> ++++++++++++++++++------------
>>>>    1 file changed, 18 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/tools/xenstore/xenstored_domain.c 
>>>> b/tools/xenstore/xenstored_domain.c
>>>> index 9fb78d5772..150c6f082e 100644
>>>> --- a/tools/xenstore/xenstored_domain.c
>>>> +++ b/tools/xenstore/xenstored_domain.c
>>>> @@ -119,6 +119,11 @@ static int writechn(struct connection *conn,
>>>>        struct xenstore_domain_interface *intf = 
>>>> conn->domain->interface;
>>>>        XENSTORE_RING_IDX cons, prod;
>>>> +    if (!intf) {
>>>> +        errno = ENODEV;
>>>> +        return -1;
>>>> +    }
>>>> +
>>>>        /* Must read indexes once, and before anything else, and 
>>>> verified. */
>>>>        cons = intf->rsp_cons;
>>>>        prod = intf->rsp_prod;
>>>> @@ -149,6 +154,11 @@ static int readchn(struct connection *conn, 
>>>> void *data, unsigned int len)
>>>>        struct xenstore_domain_interface *intf = 
>>>> conn->domain->interface;
>>>>        XENSTORE_RING_IDX cons, prod;
>>>> +    if (!intf) {
>>>> +        errno = ENODEV;
>>>> +        return -1;
>>>> +    }
>>>> +
>>>>        /* Must read indexes once, and before anything else, and 
>>>> verified. */
>>>>        cons = intf->req_cons;
>>>>        prod = intf->req_prod;
>>>> @@ -176,6 +186,9 @@ static bool domain_can_write(struct connection 
>>>> *conn)
>>>>    {
>>>>        struct xenstore_domain_interface *intf = 
>>>> conn->domain->interface;
>>>> +    if (!intf)
>>>> +        return false;
>>>> +
>>>
>>> Rather than adding an extra check, how about taking advantage of 
>>> is_ignore?
>>
>> Right, I just need to change the order in conn_can_read and
>> conn_can_write so that the is_ignored check is performed before the
>> can_{read,write} handler is called.
>>
>>>>        return ((intf->rsp_prod - intf->rsp_cons) != 
>>>> XENSTORE_RING_SIZE);
>>>>    }
>>>> @@ -183,7 +196,8 @@ static bool domain_can_read(struct connection 
>>>> *conn)
>>>>    {
>>>>        struct xenstore_domain_interface *intf = 
>>>> conn->domain->interface;
>>>> -    if (domain_is_unprivileged(conn) && conn->domain->wrl_credit < 0)
>>>> +    if ((domain_is_unprivileged(conn) && conn->domain->wrl_credit < 
>>>> 0) ||
>>>> +        !intf)
>>>>            return false;
>>>>        return (intf->req_cons != intf->req_prod);
>>>> @@ -268,14 +282,7 @@ void check_domains(void)
>>>>                    domain->shutdown = true;
>>>>                    notify = 1;
>>>>                }
>>>> -            /*
>>>> -             * On Restore, we may have been unable to remap the
>>>> -             * interface and the port. As we don't know whether
>>>> -             * this was because of a dying domain, we need to
>>>> -             * check if the interface and port are still valid.
>>>> -             */
>>>> -            if (!dominfo.dying && domain->port &&
>>>> -                domain->interface)
>>>> +            if (!dominfo.dying)
>>>>                    continue;
>>>
>>> This is mostly a revert on "tools/xenstore: handle dying domains in live
>>> update". However, IIRC, this check was necessary to release the 
>>> connection
>>> if the domain has died in the middle of Live-Update.
>>
>> But if the domain has died in the middle of live update
>> get_domain_info will return false, and thus the code won't get here.
> 
> Hmmm... I think I am mixing up a few things... I went through the 
> original discussion (it was on the security ML) to find out why I wrote 
> the patch like that. When going through the archives, I noticed that I 
> provided a different version of this patch (see [1]) because there was 
> some issue with the check here (I wrote that it would lead to zombie 
> domain, but don't have the rationale :().
> 
> Juergen, I don't seem to find the reason why the patch was not replaced 
> as we discussed on the security ML. Do you remember why?

Sorry, no, I don't.

You did send the new version for V6 of the LU series, but it seems at
least in V9 you commented on the patch requesting that a comment just
in the section being different between the two variants to be removed.

So either we both overlooked the new variant not having gone in, or we
agreed to use the old version (e.g. in a security meeting). In my IRC
logs I couldn't find anything either (the only mentioning of that patch
was before V6 of the series was sent, and that was before you sending
the new one as a reply to V6).

> Assuming this was a mistake, could someone take care of sending an 
> update? If not, I could do it when I am back in October.
> 
> For the archives, the issues would appear when shutting down a domain 
> during Live-Update.

Hmm, IIRC you did quite some extensive testing of LU and didn't find
any problem in the final version.

Are you sure there really is a problem?


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 v2 5/6] tools/xenstored: partially handle domains without a shared ring

[Julien Grall]

(+ Some AWS folks)

Hi Juergen,

On 22/09/2021 17:34, Juergen Gross wrote:
> On 22.09.21 12:23, Julien Grall wrote:
>> Hi Roger,
>>
>> On 22/09/2021 14:58, Roger Pau Monné wrote:
>>> On Wed, Sep 22, 2021 at 02:07:44PM +0500, Julien Grall wrote:
>>>> Hi Roger,
>>>>
>>>> On 22/09/2021 13:21, Roger Pau Monne wrote:
>>>>> Failure to map the shared ring and thus establish a xenstore
>>>>> connection with a domain shouldn't prevent the "@introduceDomain"
>>>>> watch from firing, likewise with "@releaseDomain".
>>>>>
>>>>> In order to handle such events properly xenstored should keep track of
>>>>> the domains even if the shared communication ring cannot be mapped.
>>>>> This was partially the case due to the restore mode, which needs to
>>>>> handle domains that have been destroyed between the save and restore
>>>>> period. This patch extends on the previous limited support of
>>>>> temporary adding a domain without a valid interface ring, and modifies
>>>>> check_domains to keep domains without an interface on the list.
>>>>>
>>>>> Handling domains without a valid shared ring is required in order to
>>>>> support domain without a grant table, since the lack of grant table
>>>>> will prevent the mapping of the xenstore ring grant reference.
>>>>>
>>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>>> ---
>>>>> oxenstored will need a similar treatment once grant mapping is used
>>>>> there. For the time being it still works correctly because it uses
>>>>> foreign memory to map the shared ring, and that will work in the
>>>>> absence of grant tables on the domain.
>>>>> ---
>>>>> Changes since v1:
>>>>>    - New in this version.
>>>>> ---
>>>>>    tools/xenstore/xenstored_domain.c | 30 
>>>>> ++++++++++++++++++------------
>>>>>    1 file changed, 18 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/tools/xenstore/xenstored_domain.c 
>>>>> b/tools/xenstore/xenstored_domain.c
>>>>> index 9fb78d5772..150c6f082e 100644
>>>>> --- a/tools/xenstore/xenstored_domain.c
>>>>> +++ b/tools/xenstore/xenstored_domain.c
>>>>> @@ -119,6 +119,11 @@ static int writechn(struct connection *conn,
>>>>>        struct xenstore_domain_interface *intf = 
>>>>> conn->domain->interface;
>>>>>        XENSTORE_RING_IDX cons, prod;
>>>>> +    if (!intf) {
>>>>> +        errno = ENODEV;
>>>>> +        return -1;
>>>>> +    }
>>>>> +
>>>>>        /* Must read indexes once, and before anything else, and 
>>>>> verified. */
>>>>>        cons = intf->rsp_cons;
>>>>>        prod = intf->rsp_prod;
>>>>> @@ -149,6 +154,11 @@ static int readchn(struct connection *conn, 
>>>>> void *data, unsigned int len)
>>>>>        struct xenstore_domain_interface *intf = 
>>>>> conn->domain->interface;
>>>>>        XENSTORE_RING_IDX cons, prod;
>>>>> +    if (!intf) {
>>>>> +        errno = ENODEV;
>>>>> +        return -1;
>>>>> +    }
>>>>> +
>>>>>        /* Must read indexes once, and before anything else, and 
>>>>> verified. */
>>>>>        cons = intf->req_cons;
>>>>>        prod = intf->req_prod;
>>>>> @@ -176,6 +186,9 @@ static bool domain_can_write(struct connection 
>>>>> *conn)
>>>>>    {
>>>>>        struct xenstore_domain_interface *intf = 
>>>>> conn->domain->interface;
>>>>> +    if (!intf)
>>>>> +        return false;
>>>>> +
>>>>
>>>> Rather than adding an extra check, how about taking advantage of 
>>>> is_ignore?
>>>
>>> Right, I just need to change the order in conn_can_read and
>>> conn_can_write so that the is_ignored check is performed before the
>>> can_{read,write} handler is called.
>>>
>>>>>        return ((intf->rsp_prod - intf->rsp_cons) != 
>>>>> XENSTORE_RING_SIZE);
>>>>>    }
>>>>> @@ -183,7 +196,8 @@ static bool domain_can_read(struct connection 
>>>>> *conn)
>>>>>    {
>>>>>        struct xenstore_domain_interface *intf = 
>>>>> conn->domain->interface;
>>>>> -    if (domain_is_unprivileged(conn) && conn->domain->wrl_credit < 0)
>>>>> +    if ((domain_is_unprivileged(conn) && conn->domain->wrl_credit 
>>>>> < 0) ||
>>>>> +        !intf)
>>>>>            return false;
>>>>>        return (intf->req_cons != intf->req_prod);
>>>>> @@ -268,14 +282,7 @@ void check_domains(void)
>>>>>                    domain->shutdown = true;
>>>>>                    notify = 1;
>>>>>                }
>>>>> -            /*
>>>>> -             * On Restore, we may have been unable to remap the
>>>>> -             * interface and the port. As we don't know whether
>>>>> -             * this was because of a dying domain, we need to
>>>>> -             * check if the interface and port are still valid.
>>>>> -             */
>>>>> -            if (!dominfo.dying && domain->port &&
>>>>> -                domain->interface)
>>>>> +            if (!dominfo.dying)
>>>>>                    continue;
>>>>
>>>> This is mostly a revert on "tools/xenstore: handle dying domains in 
>>>> live
>>>> update". However, IIRC, this check was necessary to release the 
>>>> connection
>>>> if the domain has died in the middle of Live-Update.
>>>
>>> But if the domain has died in the middle of live update
>>> get_domain_info will return false, and thus the code won't get here.
>>
>> Hmmm... I think I am mixing up a few things... I went through the 
>> original discussion (it was on the security ML) to find out why I 
>> wrote the patch like that. When going through the archives, I noticed 
>> that I provided a different version of this patch (see [1]) because 
>> there was some issue with the check here (I wrote that it would lead 
>> to zombie domain, but don't have the rationale :().
>>
>> Juergen, I don't seem to find the reason why the patch was not 
>> replaced as we discussed on the security ML. Do you remember why?
> 
> Sorry, no, I don't.
> 
> You did send the new version for V6 of the LU series, but it seems at
> least in V9 you commented on the patch requesting that a comment just
> in the section being different between the two variants to be removed.
> 
> So either we both overlooked the new variant not having gone in, or we
> agreed to use the old version (e.g. in a security meeting). In my IRC
> logs I couldn't find anything either (the only mentioning of that patch
> was before V6 of the series was sent, and that was before you sending
> the new one as a reply to V6).
> 
>> Assuming this was a mistake, could someone take care of sending an 
>> update? If not, I could do it when I am back in October.
>>
>> For the archives, the issues would appear when shutting down a domain 
>> during Live-Update.
> 
> Hmm, IIRC you did quite some extensive testing of LU and didn't find
> any problem in the final version.

I did extensive testing with early series but I can't remember whether I 
tested the shutdown reproducer with the latest series.

> 
> Are you sure there really is a problem?

I thought a bit more and looked at the code (I don't have access to a 
test machine at the moment). I think there is indeed a problem.

Some watchers of @releaseDomain (such as xenconsoled) will only remove a 
domain from their internal state when the domain is actually dead.

This is based on dominfo.dying which is only set when all the resources 
are relinquished and waiting for the other domains to release any 
resources for that domain.

The problem is Xenstore may fail to map the interface or the event 
channel long before the domain is actually dead. With the current check, 
we would free the allocated structure and therefore send @releaseDomain 
too early. So daemon like xenconsoled, would never cleanup for the 
domain and leave a zombie domain. Well... until the next @releaseDomain 
(or @introduceDomain for Xenconsoled) AFAICT.

The revised patch is meant to solve it by just ignoring the connection. 
With that approach, we would correctly notify watches when the domain is 
dead.

Cheers,

-- 
Julien Grall

Re: [PATCH v2 5/6] tools/xenstored: partially handle domains without a shared ring

[Roger Pau Monné]

On Wed, Sep 22, 2021 at 06:46:25PM +0500, Julien Grall wrote:
> (+ Some AWS folks)
> 
> Hi Juergen,
> 
> On 22/09/2021 17:34, Juergen Gross wrote:
> > On 22.09.21 12:23, Julien Grall wrote:
> > > Hi Roger,
> > > 
> > > On 22/09/2021 14:58, Roger Pau Monné wrote:
> > > > On Wed, Sep 22, 2021 at 02:07:44PM +0500, Julien Grall wrote:
> > > > > Hi Roger,
> > > > > 
> > > > > On 22/09/2021 13:21, Roger Pau Monne wrote:
> > > > > > Failure to map the shared ring and thus establish a xenstore
> > > > > > connection with a domain shouldn't prevent the "@introduceDomain"
> > > > > > watch from firing, likewise with "@releaseDomain".
> > > > > > 
> > > > > > In order to handle such events properly xenstored should keep track of
> > > > > > the domains even if the shared communication ring cannot be mapped.
> > > > > > This was partially the case due to the restore mode, which needs to
> > > > > > handle domains that have been destroyed between the save and restore
> > > > > > period. This patch extends on the previous limited support of
> > > > > > temporary adding a domain without a valid interface ring, and modifies
> > > > > > check_domains to keep domains without an interface on the list.
> > > > > > 
> > > > > > Handling domains without a valid shared ring is required in order to
> > > > > > support domain without a grant table, since the lack of grant table
> > > > > > will prevent the mapping of the xenstore ring grant reference.
> > > > > > 
> > > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > > > > > ---
> > > > > > oxenstored will need a similar treatment once grant mapping is used
> > > > > > there. For the time being it still works correctly because it uses
> > > > > > foreign memory to map the shared ring, and that will work in the
> > > > > > absence of grant tables on the domain.
> > > > > > ---
> > > > > > Changes since v1:
> > > > > >    - New in this version.
> > > > > > ---
> > > > > >    tools/xenstore/xenstored_domain.c | 30
> > > > > > ++++++++++++++++++------------
> > > > > >    1 file changed, 18 insertions(+), 12 deletions(-)
> > > > > > 
> > > > > > diff --git a/tools/xenstore/xenstored_domain.c
> > > > > > b/tools/xenstore/xenstored_domain.c
> > > > > > index 9fb78d5772..150c6f082e 100644
> > > > > > --- a/tools/xenstore/xenstored_domain.c
> > > > > > +++ b/tools/xenstore/xenstored_domain.c
> > > > > > @@ -119,6 +119,11 @@ static int writechn(struct connection *conn,
> > > > > >        struct xenstore_domain_interface *intf =
> > > > > > conn->domain->interface;
> > > > > >        XENSTORE_RING_IDX cons, prod;
> > > > > > +    if (!intf) {
> > > > > > +        errno = ENODEV;
> > > > > > +        return -1;
> > > > > > +    }
> > > > > > +
> > > > > >        /* Must read indexes once, and before anything
> > > > > > else, and verified. */
> > > > > >        cons = intf->rsp_cons;
> > > > > >        prod = intf->rsp_prod;
> > > > > > @@ -149,6 +154,11 @@ static int readchn(struct
> > > > > > connection *conn, void *data, unsigned int len)
> > > > > >        struct xenstore_domain_interface *intf =
> > > > > > conn->domain->interface;
> > > > > >        XENSTORE_RING_IDX cons, prod;
> > > > > > +    if (!intf) {
> > > > > > +        errno = ENODEV;
> > > > > > +        return -1;
> > > > > > +    }
> > > > > > +
> > > > > >        /* Must read indexes once, and before anything
> > > > > > else, and verified. */
> > > > > >        cons = intf->req_cons;
> > > > > >        prod = intf->req_prod;
> > > > > > @@ -176,6 +186,9 @@ static bool domain_can_write(struct
> > > > > > connection *conn)
> > > > > >    {
> > > > > >        struct xenstore_domain_interface *intf =
> > > > > > conn->domain->interface;
> > > > > > +    if (!intf)
> > > > > > +        return false;
> > > > > > +
> > > > > 
> > > > > Rather than adding an extra check, how about taking
> > > > > advantage of is_ignore?
> > > > 
> > > > Right, I just need to change the order in conn_can_read and
> > > > conn_can_write so that the is_ignored check is performed before the
> > > > can_{read,write} handler is called.
> > > > 
> > > > > >        return ((intf->rsp_prod - intf->rsp_cons) !=
> > > > > > XENSTORE_RING_SIZE);
> > > > > >    }
> > > > > > @@ -183,7 +196,8 @@ static bool domain_can_read(struct
> > > > > > connection *conn)
> > > > > >    {
> > > > > >        struct xenstore_domain_interface *intf =
> > > > > > conn->domain->interface;
> > > > > > -    if (domain_is_unprivileged(conn) && conn->domain->wrl_credit < 0)
> > > > > > +    if ((domain_is_unprivileged(conn) &&
> > > > > > conn->domain->wrl_credit < 0) ||
> > > > > > +        !intf)
> > > > > >            return false;
> > > > > >        return (intf->req_cons != intf->req_prod);
> > > > > > @@ -268,14 +282,7 @@ void check_domains(void)
> > > > > >                    domain->shutdown = true;
> > > > > >                    notify = 1;
> > > > > >                }
> > > > > > -            /*
> > > > > > -             * On Restore, we may have been unable to remap the
> > > > > > -             * interface and the port. As we don't know whether
> > > > > > -             * this was because of a dying domain, we need to
> > > > > > -             * check if the interface and port are still valid.
> > > > > > -             */
> > > > > > -            if (!dominfo.dying && domain->port &&
> > > > > > -                domain->interface)
> > > > > > +            if (!dominfo.dying)
> > > > > >                    continue;
> > > > > 
> > > > > This is mostly a revert on "tools/xenstore: handle dying
> > > > > domains in live
> > > > > update". However, IIRC, this check was necessary to release
> > > > > the connection
> > > > > if the domain has died in the middle of Live-Update.
> > > > 
> > > > But if the domain has died in the middle of live update
> > > > get_domain_info will return false, and thus the code won't get here.
> > > 
> > > Hmmm... I think I am mixing up a few things... I went through the
> > > original discussion (it was on the security ML) to find out why I
> > > wrote the patch like that. When going through the archives, I
> > > noticed that I provided a different version of this patch (see [1])
> > > because there was some issue with the check here (I wrote that it
> > > would lead to zombie domain, but don't have the rationale :().
> > > 
> > > Juergen, I don't seem to find the reason why the patch was not
> > > replaced as we discussed on the security ML. Do you remember why?
> > 
> > Sorry, no, I don't.
> > 
> > You did send the new version for V6 of the LU series, but it seems at
> > least in V9 you commented on the patch requesting that a comment just
> > in the section being different between the two variants to be removed.
> > 
> > So either we both overlooked the new variant not having gone in, or we
> > agreed to use the old version (e.g. in a security meeting). In my IRC
> > logs I couldn't find anything either (the only mentioning of that patch
> > was before V6 of the series was sent, and that was before you sending
> > the new one as a reply to V6).
> > 
> > > Assuming this was a mistake, could someone take care of sending an
> > > update? If not, I could do it when I am back in October.
> > > 
> > > For the archives, the issues would appear when shutting down a
> > > domain during Live-Update.
> > 
> > Hmm, IIRC you did quite some extensive testing of LU and didn't find
> > any problem in the final version.
> 
> I did extensive testing with early series but I can't remember whether I
> tested the shutdown reproducer with the latest series.
> 
> > 
> > Are you sure there really is a problem?
> 
> I thought a bit more and looked at the code (I don't have access to a test
> machine at the moment). I think there is indeed a problem.
> 
> Some watchers of @releaseDomain (such as xenconsoled) will only remove a
> domain from their internal state when the domain is actually dead.
> 
> This is based on dominfo.dying which is only set when all the resources are
> relinquished and waiting for the other domains to release any resources for
> that domain.
> 
> The problem is Xenstore may fail to map the interface or the event channel
> long before the domain is actually dead. With the current check, we would
> free the allocated structure and therefore send @releaseDomain too early. So
> daemon like xenconsoled, would never cleanup for the domain and leave a
> zombie domain. Well... until the next @releaseDomain (or @introduceDomain
> for Xenconsoled) AFAICT.
> 
> The revised patch is meant to solve it by just ignoring the connection. With
> that approach, we would correctly notify watches when the domain is dead.

I think the patch provided by Julien is indeed better than the current
code, or else you will miss @releaseDomain events in corner cases for
dominfo.dying.

I think however the patch is missing a change in the order of the
checks in conn_can_{read,write}, so that the is_ignored check is
performed before calling can_{read,write} which will try to poke at
the interface and trigger a fault if not mapped.

Thanks, Roger.

Re: [PATCH v2 5/6] tools/xenstored: partially handle domains without a shared ring

[Julien Grall]

Hi Roger,

On 23/09/2021 12:23, Roger Pau Monné wrote:
> On Wed, Sep 22, 2021 at 06:46:25PM +0500, Julien Grall wrote:
>> I thought a bit more and looked at the code (I don't have access to a test
>> machine at the moment). I think there is indeed a problem.
>>
>> Some watchers of @releaseDomain (such as xenconsoled) will only remove a
>> domain from their internal state when the domain is actually dead.
>>
>> This is based on dominfo.dying which is only set when all the resources are
>> relinquished and waiting for the other domains to release any resources for
>> that domain.
>>
>> The problem is Xenstore may fail to map the interface or the event channel
>> long before the domain is actually dead. With the current check, we would
>> free the allocated structure and therefore send @releaseDomain too early. So
>> daemon like xenconsoled, would never cleanup for the domain and leave a
>> zombie domain. Well... until the next @releaseDomain (or @introduceDomain
>> for Xenconsoled) AFAICT.
>>
>> The revised patch is meant to solve it by just ignoring the connection. With
>> that approach, we would correctly notify watches when the domain is dead.
> 
> I think the patch provided by Julien is indeed better than the current
> code, or else you will miss @releaseDomain events in corner cases for
> dominfo.dying.
> 
> I think however the patch is missing a change in the order of the
> checks in conn_can_{read,write}, so that the is_ignored check is
> performed before calling can_{read,write} which will try to poke at
> the interface and trigger a fault if not mapped.

Ah good point. I originally moved the is_ignored check after the 
callback because the socket connection can in theory be closed from 
can_{read, write}.

However, in pratice, is_ignored is only set for socket from 
ignore_connection() that will also close the socket.

The new use will only set is_ignored for the domain connection. So I am 
guessing moving the check early on ought to be fine.

The alternative would be to call ignore_connection() but this feels a 
bit weird because most of it would be a NOP as we are introducing the 
domain.

Cheers,


-- 
Julien Grall

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

[Julien Grall]

Hi Juergen,

On 22/09/2021 14:38, Juergen Gross wrote:
> On 22.09.21 11:19, Julien Grall wrote:
>> Hi Roger,
>>
>> On 22/09/2021 13:21, Roger Pau Monne wrote:
>>> 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     | 100 +++++++++++++++++++++++++++++++++--
>>>   3 files changed, 98 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)>.
>>
>> Technically, the version only applies to format of the table for 
>> granting page. The mapping itself is version agnostic. So this feels a 
>> bit wrong to use max_grant_version to not allocate d->grant_table.
>>
>> I also can see use-cases where we may want to allow a domain to grant 
>> page but not map grant (for instance, a further hardening of XSA-380). 
>> Therefore, I think we want to keep max_grant_version for the table 
>> itself and manage the mappings separately (possibly by letting the 
>> admin to select the max number of entries in the maptrack).
> 
> You mean the already existing domain config option max_maptrack_frames?

Yes. I didn't realize we already had an option for that :).

Cheers,

-- 
Julien Grall

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

[Julien Grall]

Hi Roger,

On 22/09/2021 14:39, Roger Pau Monné wrote:
> On Wed, Sep 22, 2021 at 01:57:02PM +0500, Julien Grall wrote:
>>
>>
>> On 22/09/2021 13:21, Roger Pau Monne wrote:
>>> Hello,
>>
>> Hi Roger,
>>
>>> First patch on the series is a trivial change to xenconsoled in order to
>>> use xenforeignmemory stable library in order to map the shared console
>>> ring instead of the unstable libxc interface. It's reviewed and ready to
>>> go in.
>>>
>>> Patches 2 and 3 allow setting the 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 3 patches attempt to implement support for creating guests
>>> without a grant table. This requires some changes to xenstored in order
>>> to partially support guests without a valid ring interface, as the lack
>>> of grant table will prevent C xenstored from mapping the shared ring.
>>> Note this is not an issue for Ocaml xenstored, as it still uses the
>>> foreign memory interface to map the shared ring, and thus won't notice
>>> the lack of grant table support on the domain.
>>
>> I find a bit odd that the Xenstore support is conditional to whether grant
>> table is available. Are you expecting domains with no grant table to have no
>> PV drivers (including PV shutdown)?
> 
> I don't really expect much, as having guests without grant table is a
> developer option right now, if someone wants to make use of them for
> any reason it would need some thought.
> 
> The other option would be my first proposal to restore foreign mapping
> of the xenstore ring on that case:
> 
> https://lore.kernel.org/xen-devel/20210917154625.89315-6-roger.pau@citrix.com/
> 
> But it's also arguable that a guest not having a grant table should
> also likely prevent foreign mapping attempts. Plus such foreign
> mapping won't work from stubdomains.

There is another option: extend the acquire hypercall to allow xenstored 
domain to map the xenstore interface. This would require more work, but 
at least it would avoid the interesting dependency on the grant table.

> 
> I'm fine with dropping those patches if they turn out to be too
> controversial, I think it's an interesting option to be able to
> disable the grant table, but I don't have a full picture of how that
> could be used in practice. Maybe others have and would be willing to
> pick this up.

I think the current approach is probably OK as a developper option. 
However, we should at least document in the option that disabling the 
grant-table will also disable Xenstore (anything else?) support when 
using C Xenstored.

It might also be worth to clearly state in the doc that this is only 
intended for developer use and not supported.

Cheers,

-- 
Julien Grall

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

[Roger Pau Monné]

On Thu, Sep 23, 2021 at 01:47:37PM +0500, Julien Grall wrote:
> Hi Roger,
> 
> On 22/09/2021 14:39, Roger Pau Monné wrote:
> > On Wed, Sep 22, 2021 at 01:57:02PM +0500, Julien Grall wrote:
> > > 
> > > 
> > > On 22/09/2021 13:21, Roger Pau Monne wrote:
> > > > Hello,
> > > 
> > > Hi Roger,
> > > 
> > > > First patch on the series is a trivial change to xenconsoled in order to
> > > > use xenforeignmemory stable library in order to map the shared console
> > > > ring instead of the unstable libxc interface. It's reviewed and ready to
> > > > go in.
> > > > 
> > > > Patches 2 and 3 allow setting the 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 3 patches attempt to implement support for creating guests
> > > > without a grant table. This requires some changes to xenstored in order
> > > > to partially support guests without a valid ring interface, as the lack
> > > > of grant table will prevent C xenstored from mapping the shared ring.
> > > > Note this is not an issue for Ocaml xenstored, as it still uses the
> > > > foreign memory interface to map the shared ring, and thus won't notice
> > > > the lack of grant table support on the domain.
> > > 
> > > I find a bit odd that the Xenstore support is conditional to whether grant
> > > table is available. Are you expecting domains with no grant table to have no
> > > PV drivers (including PV shutdown)?
> > 
> > I don't really expect much, as having guests without grant table is a
> > developer option right now, if someone wants to make use of them for
> > any reason it would need some thought.
> > 
> > The other option would be my first proposal to restore foreign mapping
> > of the xenstore ring on that case:
> > 
> > https://lore.kernel.org/xen-devel/20210917154625.89315-6-roger.pau@citrix.com/
> > 
> > But it's also arguable that a guest not having a grant table should
> > also likely prevent foreign mapping attempts. Plus such foreign
> > mapping won't work from stubdomains.
> 
> There is another option: extend the acquire hypercall to allow xenstored
> domain to map the xenstore interface. This would require more work, but at
> least it would avoid the interesting dependency on the grant table.

Xen isn't aware of the shared xenstore ring page currently, so that
would mean introducing more knowledge to the hypervisor that what's
strictly required IMO, as Xen has no business in knowing such details.

The grant table slot used by the xenstore shared page is just an
agreement at the toolstack level, but not known to the hypervisor so
far.

> > 
> > I'm fine with dropping those patches if they turn out to be too
> > controversial, I think it's an interesting option to be able to
> > disable the grant table, but I don't have a full picture of how that
> > could be used in practice. Maybe others have and would be willing to
> > pick this up.
> 
> I think the current approach is probably OK as a developper option. However,
> we should at least document in the option that disabling the grant-table
> will also disable Xenstore (anything else?) support when using C Xenstored.
> 
> It might also be worth to clearly state in the doc that this is only
> intended for developer use and not supported.

Sure, adding it to xl.cfg man page is likely the best place. Will do
when updating the patches.

Thanks, Roger.

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

[Julien Grall]

[-- Attachment #1: Type: text/plain, Size: 4292 bytes --]

Hi Roger,

On Thu, 23 Sep 2021, 16:20 Roger Pau Monné, <roger.pau@citrix.com> wrote:

> On Thu, Sep 23, 2021 at 01:47:37PM +0500, Julien Grall wrote:
> > Hi Roger,
> >
> > On 22/09/2021 14:39, Roger Pau Monné wrote:
> > > On Wed, Sep 22, 2021 at 01:57:02PM +0500, Julien Grall wrote:
> > > >
> > > >
> > > > On 22/09/2021 13:21, Roger Pau Monne wrote:
> > > > > Hello,
> > > >
> > > > Hi Roger,
> > > >
> > > > > First patch on the series is a trivial change to xenconsoled in
> order to
> > > > > use xenforeignmemory stable library in order to map the shared
> console
> > > > > ring instead of the unstable libxc interface. It's reviewed and
> ready to
> > > > > go in.
> > > > >
> > > > > Patches 2 and 3 allow setting the 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 3 patches attempt to implement support for creating guests
> > > > > without a grant table. This requires some changes to xenstored in
> order
> > > > > to partially support guests without a valid ring interface, as the
> lack
> > > > > of grant table will prevent C xenstored from mapping the shared
> ring.
> > > > > Note this is not an issue for Ocaml xenstored, as it still uses the
> > > > > foreign memory interface to map the shared ring, and thus won't
> notice
> > > > > the lack of grant table support on the domain.
> > > >
> > > > I find a bit odd that the Xenstore support is conditional to whether
> grant
> > > > table is available. Are you expecting domains with no grant table to
> have no
> > > > PV drivers (including PV shutdown)?
> > >
> > > I don't really expect much, as having guests without grant table is a
> > > developer option right now, if someone wants to make use of them for
> > > any reason it would need some thought.
> > >
> > > The other option would be my first proposal to restore foreign mapping
> > > of the xenstore ring on that case:
> > >
> > >
> https://lore.kernel.org/xen-devel/20210917154625.89315-6-roger.pau@citrix.com/
> > >
> > > But it's also arguable that a guest not having a grant table should
> > > also likely prevent foreign mapping attempts. Plus such foreign
> > > mapping won't work from stubdomains.
> >
> > There is another option: extend the acquire hypercall to allow xenstored
> > domain to map the xenstore interface. This would require more work, but
> at
> > least it would avoid the interesting dependency on the grant table.
>
> Xen isn't aware of the shared xenstore ring page currently, so that
> would mean introducing more knowledge to the hypervisor that what's
> strictly required IMO, as Xen has no business in knowing such details.
>

Well Xen already knows the page for HVM/PVH because the guest retrieve it
through an HMV param.

We only miss (?) the PV part.


> The grant table slot used by the xenstore shared page is just an
> agreement at the toolstack level, but not known to the hypervisor so
> far.
>

Right, we need to find a different way to provide/map the shared page if
the grant table is not present.

To me the acquire hypercall is the best way to resolve it as Xen knows
whether the domain will run Xenstored (at least we used to have a flag) and
we can do the permission control easily.

Do you have another alternative?


> > >
> > > I'm fine with dropping those patches if they turn out to be too
> > > controversial, I think it's an interesting option to be able to
> > > disable the grant table, but I don't have a full picture of how that
> > > could be used in practice. Maybe others have and would be willing to
> > > pick this up.
> >
> > I think the current approach is probably OK as a developper option.
> However,
> > we should at least document in the option that disabling the grant-table
> > will also disable Xenstore (anything else?) support when using C
> Xenstored.
> >
> > It might also be worth to clearly state in the doc that this is only
> > intended for developer use and not supported.
>
> Sure, adding it to xl.cfg man page is likely the best place. Will do
> when updating the patches.
>
> Thanks, Roger.
>

[-- Attachment #2: Type: text/html, Size: 5919 bytes --]

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

[Jan Beulich]

On 24.09.2021 04:30, Julien Grall wrote:
> Hi Roger,
> 
> On Thu, 23 Sep 2021, 16:20 Roger Pau Monné, <roger.pau@citrix.com> wrote:
> 
>> On Thu, Sep 23, 2021 at 01:47:37PM +0500, Julien Grall wrote:
>>> Hi Roger,
>>>
>>> On 22/09/2021 14:39, Roger Pau Monné wrote:
>>>> On Wed, Sep 22, 2021 at 01:57:02PM +0500, Julien Grall wrote:
>>>>>
>>>>>
>>>>> On 22/09/2021 13:21, Roger Pau Monne wrote:
>>>>>> Hello,
>>>>>
>>>>> Hi Roger,
>>>>>
>>>>>> First patch on the series is a trivial change to xenconsoled in
>> order to
>>>>>> use xenforeignmemory stable library in order to map the shared
>> console
>>>>>> ring instead of the unstable libxc interface. It's reviewed and
>> ready to
>>>>>> go in.
>>>>>>
>>>>>> Patches 2 and 3 allow setting the 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 3 patches attempt to implement support for creating guests
>>>>>> without a grant table. This requires some changes to xenstored in
>> order
>>>>>> to partially support guests without a valid ring interface, as the
>> lack
>>>>>> of grant table will prevent C xenstored from mapping the shared
>> ring.
>>>>>> Note this is not an issue for Ocaml xenstored, as it still uses the
>>>>>> foreign memory interface to map the shared ring, and thus won't
>> notice
>>>>>> the lack of grant table support on the domain.
>>>>>
>>>>> I find a bit odd that the Xenstore support is conditional to whether
>> grant
>>>>> table is available. Are you expecting domains with no grant table to
>> have no
>>>>> PV drivers (including PV shutdown)?
>>>>
>>>> I don't really expect much, as having guests without grant table is a
>>>> developer option right now, if someone wants to make use of them for
>>>> any reason it would need some thought.
>>>>
>>>> The other option would be my first proposal to restore foreign mapping
>>>> of the xenstore ring on that case:
>>>>
>>>>
>> https://lore.kernel.org/xen-devel/20210917154625.89315-6-roger.pau@citrix.com/
>>>>
>>>> But it's also arguable that a guest not having a grant table should
>>>> also likely prevent foreign mapping attempts. Plus such foreign
>>>> mapping won't work from stubdomains.
>>>
>>> There is another option: extend the acquire hypercall to allow xenstored
>>> domain to map the xenstore interface. This would require more work, but
>> at
>>> least it would avoid the interesting dependency on the grant table.
>>
>> Xen isn't aware of the shared xenstore ring page currently, so that
>> would mean introducing more knowledge to the hypervisor that what's
>> strictly required IMO, as Xen has no business in knowing such details.
>>
> 
> Well Xen already knows the page for HVM/PVH because the guest retrieve it
> through an HMV param.

To be honest using this in such a way would feel like an abuse / layering
violation to me.

Jan

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

[Julien Grall]

[-- Attachment #1: Type: text/plain, Size: 3423 bytes --]

On Fri, 24 Sep 2021, 11:21 Jan Beulich, <jbeulich@suse.com> wrote:

> On 24.09.2021 04:30, Julien Grall wrote:
> > Hi Roger,
> >
> > On Thu, 23 Sep 2021, 16:20 Roger Pau Monné, <roger.pau@citrix.com>
> wrote:
> >
> >> On Thu, Sep 23, 2021 at 01:47:37PM +0500, Julien Grall wrote:
> >>> Hi Roger,
> >>>
> >>> On 22/09/2021 14:39, Roger Pau Monné wrote:
> >>>> On Wed, Sep 22, 2021 at 01:57:02PM +0500, Julien Grall wrote:
> >>>>>
> >>>>>
> >>>>> On 22/09/2021 13:21, Roger Pau Monne wrote:
> >>>>>> Hello,
> >>>>>
> >>>>> Hi Roger,
> >>>>>
> >>>>>> First patch on the series is a trivial change to xenconsoled in
> >> order to
> >>>>>> use xenforeignmemory stable library in order to map the shared
> >> console
> >>>>>> ring instead of the unstable libxc interface. It's reviewed and
> >> ready to
> >>>>>> go in.
> >>>>>>
> >>>>>> Patches 2 and 3 allow setting the 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 3 patches attempt to implement support for creating guests
> >>>>>> without a grant table. This requires some changes to xenstored in
> >> order
> >>>>>> to partially support guests without a valid ring interface, as the
> >> lack
> >>>>>> of grant table will prevent C xenstored from mapping the shared
> >> ring.
> >>>>>> Note this is not an issue for Ocaml xenstored, as it still uses the
> >>>>>> foreign memory interface to map the shared ring, and thus won't
> >> notice
> >>>>>> the lack of grant table support on the domain.
> >>>>>
> >>>>> I find a bit odd that the Xenstore support is conditional to whether
> >> grant
> >>>>> table is available. Are you expecting domains with no grant table to
> >> have no
> >>>>> PV drivers (including PV shutdown)?
> >>>>
> >>>> I don't really expect much, as having guests without grant table is a
> >>>> developer option right now, if someone wants to make use of them for
> >>>> any reason it would need some thought.
> >>>>
> >>>> The other option would be my first proposal to restore foreign mapping
> >>>> of the xenstore ring on that case:
> >>>>
> >>>>
> >>
> https://lore.kernel.org/xen-devel/20210917154625.89315-6-roger.pau@citrix.com/
> >>>>
> >>>> But it's also arguable that a guest not having a grant table should
> >>>> also likely prevent foreign mapping attempts. Plus such foreign
> >>>> mapping won't work from stubdomains.
> >>>
> >>> There is another option: extend the acquire hypercall to allow
> xenstored
> >>> domain to map the xenstore interface. This would require more work, but
> >> at
> >>> least it would avoid the interesting dependency on the grant table.
> >>
> >> Xen isn't aware of the shared xenstore ring page currently, so that
> >> would mean introducing more knowledge to the hypervisor that what's
> >> strictly required IMO, as Xen has no business in knowing such details.
> >>
> >
> > Well Xen already knows the page for HVM/PVH because the guest retrieve it
> > through an HMV param.
>
> To be honest using this in such a way would feel like an abuse / layering
> violation to me.
>

I can see how it can be seen like this. Do you have a better suggestion to
be able to map mapping without the foreign mapping interface and the grant
table?

Jan
>
>

[-- Attachment #2: Type: text/html, Size: 5151 bytes --]

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

[Roger Pau Monné]

On Fri, Sep 24, 2021 at 07:30:51AM +0500, Julien Grall wrote:
> Hi Roger,
> 
> On Thu, 23 Sep 2021, 16:20 Roger Pau Monné, <roger.pau@citrix.com> wrote:
> 
> > On Thu, Sep 23, 2021 at 01:47:37PM +0500, Julien Grall wrote:
> > > Hi Roger,
> > >
> > > On 22/09/2021 14:39, Roger Pau Monné wrote:
> > > > On Wed, Sep 22, 2021 at 01:57:02PM +0500, Julien Grall wrote:
> > > > >
> > > > >
> > > > > On 22/09/2021 13:21, Roger Pau Monne wrote:
> > > > > > Hello,
> > > > >
> > > > > Hi Roger,
> > > > >
> > > > > > First patch on the series is a trivial change to xenconsoled in
> > order to
> > > > > > use xenforeignmemory stable library in order to map the shared
> > console
> > > > > > ring instead of the unstable libxc interface. It's reviewed and
> > ready to
> > > > > > go in.
> > > > > >
> > > > > > Patches 2 and 3 allow setting the 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 3 patches attempt to implement support for creating guests
> > > > > > without a grant table. This requires some changes to xenstored in
> > order
> > > > > > to partially support guests without a valid ring interface, as the
> > lack
> > > > > > of grant table will prevent C xenstored from mapping the shared
> > ring.
> > > > > > Note this is not an issue for Ocaml xenstored, as it still uses the
> > > > > > foreign memory interface to map the shared ring, and thus won't
> > notice
> > > > > > the lack of grant table support on the domain.
> > > > >
> > > > > I find a bit odd that the Xenstore support is conditional to whether
> > grant
> > > > > table is available. Are you expecting domains with no grant table to
> > have no
> > > > > PV drivers (including PV shutdown)?
> > > >
> > > > I don't really expect much, as having guests without grant table is a
> > > > developer option right now, if someone wants to make use of them for
> > > > any reason it would need some thought.
> > > >
> > > > The other option would be my first proposal to restore foreign mapping
> > > > of the xenstore ring on that case:
> > > >
> > > >
> > https://lore.kernel.org/xen-devel/20210917154625.89315-6-roger.pau@citrix.com/
> > > >
> > > > But it's also arguable that a guest not having a grant table should
> > > > also likely prevent foreign mapping attempts. Plus such foreign
> > > > mapping won't work from stubdomains.
> > >
> > > There is another option: extend the acquire hypercall to allow xenstored
> > > domain to map the xenstore interface. This would require more work, but
> > at
> > > least it would avoid the interesting dependency on the grant table.
> >
> > Xen isn't aware of the shared xenstore ring page currently, so that
> > would mean introducing more knowledge to the hypervisor that what's
> > strictly required IMO, as Xen has no business in knowing such details.
> >
> 
> Well Xen already knows the page for HVM/PVH because the guest retrieve it
> through an HMV param.

HVM params has always been a bit weird IMO, as some are known and used
by Xen (like the identity page tables) while others (like the xenstore
or console ones) are merely used as a way to pass information from
the toolstack into the domain.

> We only miss (?) the PV part.

Right - ideally we should use the same mechanism for PV and HVM, which
would rule out he piggyback on HVM params.

> 
> > The grant table slot used by the xenstore shared page is just an
> > agreement at the toolstack level, but not known to the hypervisor so
> > far.
> >
> 
> Right, we need to find a different way to provide/map the shared page if
> the grant table is not present.
> 
> To me the acquire hypercall is the best way to resolve it as Xen knows
> whether the domain will run Xenstored (at least we used to have a flag) and
> we can do the permission control easily.
> 
> Do you have another alternative?

As said before, I didn't give much through about a practical use case
for this. My main focus where patches 1 and 2 (which sadly seem to be
shadowed by this no grant-table option) in order to have more fine
grained control over the grant table support on a per-domain basis.
The no grant-table mode was mostly the cherry on the cake.

I could see people using no grant-table also likely wanting
no-xenstore and no-console as to prevent mappings from other domains
into it's private memory altogether. Then using Argo or a similar no
memory sharing mechanism in order to interact with other entities. So
I think we shouldn't over engineer this xenstore usage without grant
tables, as it might not even be relevant as a use-case itself.

Using the acquire hypercall could be a solution, but I think we would
also need to introduce a new hypercall then in order for user space to
be able to tell Xen about resource areas, so that the xenstore address
can be provided to Xen without (ab)using HVM params.

Thanks, Roger.

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

[Jan Beulich]

On 24.09.2021 09:30, Julien Grall wrote:
> On Fri, 24 Sep 2021, 11:21 Jan Beulich, <jbeulich@suse.com> wrote:
>> On 24.09.2021 04:30, Julien Grall wrote:
>>> On Thu, 23 Sep 2021, 16:20 Roger Pau Monné, <roger.pau@citrix.com>
>> wrote:
>>>> On Thu, Sep 23, 2021 at 01:47:37PM +0500, Julien Grall wrote:
>>>>> On 22/09/2021 14:39, Roger Pau Monné wrote:
>>>>>> On Wed, Sep 22, 2021 at 01:57:02PM +0500, Julien Grall wrote:
>>>>>>> On 22/09/2021 13:21, Roger Pau Monne wrote:
>>>>>> But it's also arguable that a guest not having a grant table should
>>>>>> also likely prevent foreign mapping attempts. Plus such foreign
>>>>>> mapping won't work from stubdomains.
>>>>>
>>>>> There is another option: extend the acquire hypercall to allow
>> xenstored
>>>>> domain to map the xenstore interface. This would require more work, but
>>>> at
>>>>> least it would avoid the interesting dependency on the grant table.
>>>>
>>>> Xen isn't aware of the shared xenstore ring page currently, so that
>>>> would mean introducing more knowledge to the hypervisor that what's
>>>> strictly required IMO, as Xen has no business in knowing such details.
>>>>
>>>
>>> Well Xen already knows the page for HVM/PVH because the guest retrieve it
>>> through an HMV param.
>>
>> To be honest using this in such a way would feel like an abuse / layering
>> violation to me.
>>
> 
> I can see how it can be seen like this. Do you have a better suggestion to
> be able to map mapping without the foreign mapping interface and the grant
> table?

Well, as was mentioned, PV would need covering anyway. And I think just
like with grants the guest should consent with such foreign mappings
outside of the "can map everything anyway" category. Hence I think if
such a capability is indeed needed/wanted, it ought to be the guest to
announce this page to Xen.

Jan

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

[Roger Pau Monné]

On Wed, Sep 22, 2021 at 10:21:17AM +0200, Roger Pau Monne wrote:
> Hello,
> 
> First patch on the series is a trivial change to xenconsoled in order to
> use xenforeignmemory stable library in order to map the shared console
> ring instead of the unstable libxc interface. It's reviewed and ready to
> go in.
> 
> Patches 2 and 3 allow setting the 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 3 patches attempt to implement support for creating guests
> without a grant table. This requires some changes to xenstored in order
> to partially support guests without a valid ring interface, as the lack
> of grant table will prevent C xenstored from mapping the shared ring.
> Note this is not an issue for Ocaml xenstored, as it still uses the
> foreign memory interface to map the shared ring, and thus won't notice
> the lack of grant table support on the domain.
> 
> Thanks, Roger.
> 
> Roger Pau Monne (6):
>   tools/console: use xenforeigmemory to map console ring
>   gnttab: allow setting max version per-domain
>   gnttab: allow per-domain control over transitive grants

Ping? The two patches above didn't get any review in either v1 or v2.

Patch #1 should be ready to go in AFAICT.

Thanks, Roger.

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

[Christian Lindig]

[-- Attachment #1: Type: text/plain, Size: 320 bytes --]



On 11 Oct 2021, at 10:36, Roger Pau Monne <roger.pau@citrix.com<mailto:roger.pau@citrix.com>> wrote:

Ping? The two patches above didn't get any review in either v1 or v2.

Patch #1 should be ready to go in AFAICT.

Acked-by: Christian Lindig <christian.lindig@citrix.com<mailto:christian.lindig@citrix.com>>

[-- Attachment #2: Type: text/html, Size: 2490 bytes --]

Re: [PATCH v2 2/6] gnttab: allow setting max version per-domain

[Jan Beulich]

On 22.09.2021 10:21, Roger Pau Monne wrote:
> --- 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;

Nit: I realize the version field also has "gt" in its name, but just
like I consider that superfluous, I don't think "grant" needs to be
in the field name here.

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -87,14 +87,22 @@ 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;
>  
> +/* Grant version, use low 4 bits. */
> +#define XEN_DOMCTL_GRANT_version_mask    0xf
> +#define XEN_DOMCTL_GRANT_version_default 0xf
> +
> +#define XEN_DOMCTLGRANT_MAX XEN_DOMCTL_GRANT_version_mask
> +
> +    uint32_t grant_opts;

As it now seems unlikely that this will make 4.16, please don't forget
to bump the interface version for 4.17. With that and preferably with
the nit above addressed, hypervisor parts:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v2 2/6] gnttab: allow setting max version per-domain

[Jan Beulich]

On 15.10.2021 11:39, Jan Beulich wrote:
> On 22.09.2021 10:21, Roger Pau Monne wrote:
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -87,14 +87,22 @@ 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;
>>  
>> +/* Grant version, use low 4 bits. */
>> +#define XEN_DOMCTL_GRANT_version_mask    0xf
>> +#define XEN_DOMCTL_GRANT_version_default 0xf
>> +
>> +#define XEN_DOMCTLGRANT_MAX XEN_DOMCTL_GRANT_version_mask
>> +
>> +    uint32_t grant_opts;
> 
> As it now seems unlikely that this will make 4.16, please don't forget
> to bump the interface version for 4.17. With that and preferably with
> the nit above addressed, hypervisor parts:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Actually no, I'm afraid there is an issue with migration: If the tool
stack remembers the "use default" setting and hands this to the new
host, that host's default may be different from the source host's. It
is the effective max-version that needs passing on in this case, which
in turn requires a means to obtain the value.

Jan

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

[Jan Beulich]

On 22.09.2021 10:21, Roger Pau Monne wrote:
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2649,7 +2649,8 @@ void __init create_domUs(void)
>              .max_evtchn_port = -1,
>              .max_grant_frames = -1,
>              .max_maptrack_frames = -1,
> -            .grant_opts = XEN_DOMCTL_GRANT_version_default,
> +            .grant_opts = XEN_DOMCTL_GRANT_version_default |
> +                          XEN_DOMCTL_GRANT_transitive,
>          };
>  
>          if ( !dt_device_is_compatible(node, "xen,domain") )
> @@ -2757,7 +2758,8 @@ void __init create_dom0(void)
>          .max_evtchn_port = -1,
>          .max_grant_frames = gnttab_dom0_frames(),
>          .max_maptrack_frames = -1,
> -        .grant_opts = XEN_DOMCTL_GRANT_version_default,
> +        .grant_opts = XEN_DOMCTL_GRANT_version_default |
> +                      XEN_DOMCTL_GRANT_transitive,
>      };
>  
>      /* The vGIC for DOM0 is exactly emulating the hardware GIC */
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -750,7 +750,8 @@ static struct domain *__init create_dom0(const module_t *image,
>          .max_evtchn_port = -1,
>          .max_grant_frames = -1,
>          .max_maptrack_frames = -1,
> -        .grant_opts = XEN_DOMCTL_GRANT_version_default,
> +        .grant_opts = XEN_DOMCTL_GRANT_version_default |
> +                      XEN_DOMCTL_GRANT_transitive,
>          .max_vcpus = dom0_max_vcpus(),
>          .arch = {
>              .misc_flags = opt_dom0_msr_relaxed ? XEN_X86_MSR_RELAXED : 0,

While I can see that you make these adjustments for retaining backwards
compatibility, I wonder if we need to, at least for Dom0. Dom0 doesn't
normally grant anything anyway and hence would even less so use
transitive grants. Of course then there's need to be a command line
control to re-enable that, just in case.

> @@ -1965,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;

No need for !! here afaics. Not even if there weren't the &&.

As to combining with the global option: I think if an admin requested
them for a domain, this should overrule the command line option. Which
in turn suggests that the command line option could go away at this
point. Otherwise, if you AND both together and the guest is known to
not work without this functionality, domain creation would instead
better fail (or at the very least it should be logged by the tool
stack that the request wasn't satisfied, which would require a means
to retrieve the effective setting). IOW I would see the command line
turning this off to trump the per-guest enabling request.

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -98,8 +98,11 @@ struct xen_domctl_createdomain {
>  /* Grant version, use low 4 bits. */
>  #define XEN_DOMCTL_GRANT_version_mask    0xf
>  #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)

Omit the former and have the latter be 0x10 or (1U << 4)?

> -#define XEN_DOMCTLGRANT_MAX XEN_DOMCTL_GRANT_version_mask
> +#define XEN_DOMCTLGRANT_MAX XEN_DOMCTL_GRANT_transitive

I didn't even spot this in patch 2 - what is this intended to be used
for? Neither there nor here I can spot any use.

Jan

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

[Jan Beulich]

On 22.09.2021 10:21, Roger Pau Monne wrote:
> @@ -1965,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;

Btw, should you perhaps reject the flag being set when max version isn't 2?

Jan

Re: [PATCH v2 2/6] gnttab: allow setting max version per-domain

[Jan Beulich]

On 22.09.2021 10:21, Roger Pau Monne wrote:
> @@ -1917,11 +1918,27 @@ active_alloc_failed:
>  }
>  
>  int grant_table_init(struct domain *d, int max_grant_frames,
> -                     int max_maptrack_frames)
> +                     int max_maptrack_frames, unsigned int options)
>  {
>      struct grant_table *gt;
> +    unsigned int max_grant_version = options & XEN_DOMCTL_GRANT_version_mask;
>      int ret = -ENOMEM;
>  
> +    if ( max_grant_version == XEN_DOMCTL_GRANT_version_default )
> +        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;
> +    }

To aid diagnosis of issues, maybe also log the affected domain in
both cases?

Jan

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

[Jan Beulich]

On 22.09.2021 11:19, Julien Grall wrote:
> On 22/09/2021 13:21, Roger Pau Monne wrote:
>> --- 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)>.
> 
> Technically, the version only applies to format of the table for 
> granting page. The mapping itself is version agnostic. So this feels a 
> bit wrong to use max_grant_version to not allocate d->grant_table.
> 
> I also can see use-cases where we may want to allow a domain to grant 
> page but not map grant (for instance, a further hardening of XSA-380). 

Or the other way around: A typical Dom0 may not have a need to grant
anything, but will likely want to be able to map grants.

Nevertheless I think an overall "no grant operations at all" switch
is good; both of the sub-aspects already have controls.

Jan

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

[Jan Beulich]

On 22.09.2021 10:21, Roger Pau Monne wrote:
> 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.

I guess I'd consider this wrong - no grant table should imo mean no
grant operations at all. Disabling granting can be done by setting
the frame count to zero, while disabling the mapping of grants can
be done by forcing no maptrack table.

That way the number of places where checks need adding would reduce
quite a bit.

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

I would pull this check earlier, to simplify error cleanup. It
could live right after having established rd.

> @@ -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;
> +    }

While this is necessary, ...

> @@ -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;
> +    }

.. this looks to simply be a bug check, i.e. may want to be BUG_ON().
There's can't be anything to unmap if the mapping of a grant of that
domain can't have succeeded.

> @@ -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;
> +    }

Same here, I think.

> @@ -2138,6 +2174,11 @@ gnttab_query_size(
>      }
>  
>      gt = d->grant_table;
> +    if ( !gt )
> +    {
> +        op.status = GNTST_bad_domain;
> +        goto out;
> +    }

I'm not sure here - I could also see this report zero (and success).

> @@ -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;
> +    }

While not simplifying error cleanup here, I think this might still
benefit from getting moved ahead of the XSM hook. There's no point
querying XSM in this case.

Jan

Re: [PATCH v2 2/6] gnttab: allow setting max version per-domain

[Roger Pau Monné]

On Fri, Oct 15, 2021 at 11:48:33AM +0200, Jan Beulich wrote:
> On 15.10.2021 11:39, Jan Beulich wrote:
> > On 22.09.2021 10:21, Roger Pau Monne wrote:
> >> --- a/xen/include/public/domctl.h
> >> +++ b/xen/include/public/domctl.h
> >> @@ -87,14 +87,22 @@ 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;
> >>  
> >> +/* Grant version, use low 4 bits. */
> >> +#define XEN_DOMCTL_GRANT_version_mask    0xf
> >> +#define XEN_DOMCTL_GRANT_version_default 0xf
> >> +
> >> +#define XEN_DOMCTLGRANT_MAX XEN_DOMCTL_GRANT_version_mask
> >> +
> >> +    uint32_t grant_opts;
> > 
> > As it now seems unlikely that this will make 4.16, please don't forget
> > to bump the interface version for 4.17. With that and preferably with
> > the nit above addressed, hypervisor parts:
> > Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Actually no, I'm afraid there is an issue with migration: If the tool
> stack remembers the "use default" setting and hands this to the new
> host, that host's default may be different from the source host's. It
> is the effective max-version that needs passing on in this case, which
> in turn requires a means to obtain the value.

Hm, my thinking was that the admin (or a higer level orchestration
tool) would already have performed the necessary adjustments to assert
the environments are compatible.

This problem already exist today where you can migrate a VM from a
host having the max default grant version to one having
gnttab=max-ver:1 without complains.

Note that adding such a check would then effectively prevent us from
lowering the default max grant version, as any incoming migration from
a previous hypervisor using the default parameters would be rejected.

Thanks, Roger.

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

[Roger Pau Monné]

On Fri, Oct 15, 2021 at 12:05:06PM +0200, Jan Beulich wrote:
> On 22.09.2021 10:21, Roger Pau Monne wrote:
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -2649,7 +2649,8 @@ void __init create_domUs(void)
> >              .max_evtchn_port = -1,
> >              .max_grant_frames = -1,
> >              .max_maptrack_frames = -1,
> > -            .grant_opts = XEN_DOMCTL_GRANT_version_default,
> > +            .grant_opts = XEN_DOMCTL_GRANT_version_default |
> > +                          XEN_DOMCTL_GRANT_transitive,
> >          };
> >  
> >          if ( !dt_device_is_compatible(node, "xen,domain") )
> > @@ -2757,7 +2758,8 @@ void __init create_dom0(void)
> >          .max_evtchn_port = -1,
> >          .max_grant_frames = gnttab_dom0_frames(),
> >          .max_maptrack_frames = -1,
> > -        .grant_opts = XEN_DOMCTL_GRANT_version_default,
> > +        .grant_opts = XEN_DOMCTL_GRANT_version_default |
> > +                      XEN_DOMCTL_GRANT_transitive,
> >      };
> >  
> >      /* The vGIC for DOM0 is exactly emulating the hardware GIC */
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -750,7 +750,8 @@ static struct domain *__init create_dom0(const module_t *image,
> >          .max_evtchn_port = -1,
> >          .max_grant_frames = -1,
> >          .max_maptrack_frames = -1,
> > -        .grant_opts = XEN_DOMCTL_GRANT_version_default,
> > +        .grant_opts = XEN_DOMCTL_GRANT_version_default |
> > +                      XEN_DOMCTL_GRANT_transitive,
> >          .max_vcpus = dom0_max_vcpus(),
> >          .arch = {
> >              .misc_flags = opt_dom0_msr_relaxed ? XEN_X86_MSR_RELAXED : 0,
> 
> While I can see that you make these adjustments for retaining backwards
> compatibility, I wonder if we need to, at least for Dom0. Dom0 doesn't
> normally grant anything anyway and hence would even less so use
> transitive grants. Of course then there's need to be a command line
> control to re-enable that, just in case.

dom0=gnttab-transitive, or should it be placed somewhere else?

> > @@ -1965,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;
> 
> No need for !! here afaics. Not even if there weren't the &&.
> 
> As to combining with the global option: I think if an admin requested
> them for a domain, this should overrule the command line option. Which
> in turn suggests that the command line option could go away at this
> point. Otherwise, if you AND both together and the guest is known to
> not work without this functionality, domain creation would instead
> better fail (or at the very least it should be logged by the tool
> stack that the request wasn't satisfied, which would require a means
> to retrieve the effective setting). IOW I would see the command line
> turning this off to trump the per-guest enabling request.

How should we go about deprecating it? It would be a bit antisocial
IMO to just drop the option, since people using it would then be
exposed to guests using transient grants if they didn't realize it
should be set in xl.conf or xl.cfg now.

> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -98,8 +98,11 @@ struct xen_domctl_createdomain {
> >  /* Grant version, use low 4 bits. */
> >  #define XEN_DOMCTL_GRANT_version_mask    0xf
> >  #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)
> 
> Omit the former and have the latter be 0x10 or (1U << 4)?
> 
> > -#define XEN_DOMCTLGRANT_MAX XEN_DOMCTL_GRANT_version_mask
> > +#define XEN_DOMCTLGRANT_MAX XEN_DOMCTL_GRANT_transitive
> 
> I didn't even spot this in patch 2 - what is this intended to be used
> for? Neither there nor here I can spot any use.

Yeah, AFAICT those _MAX definitions are used by the ocaml stubs to
assert the max option available. Given how these new options are handled
in ocaml the _MAX check is not implemented, so I could like drop those
(unless there's some other tool that also depends on them).

Thanks, Roger.

Re: [PATCH v2 2/6] gnttab: allow setting max version per-domain

[Jan Beulich]

On 20.10.2021 10:04, Roger Pau Monné wrote:
> On Fri, Oct 15, 2021 at 11:48:33AM +0200, Jan Beulich wrote:
>> On 15.10.2021 11:39, Jan Beulich wrote:
>>> On 22.09.2021 10:21, Roger Pau Monne wrote:
>>>> --- a/xen/include/public/domctl.h
>>>> +++ b/xen/include/public/domctl.h
>>>> @@ -87,14 +87,22 @@ 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;
>>>>  
>>>> +/* Grant version, use low 4 bits. */
>>>> +#define XEN_DOMCTL_GRANT_version_mask    0xf
>>>> +#define XEN_DOMCTL_GRANT_version_default 0xf
>>>> +
>>>> +#define XEN_DOMCTLGRANT_MAX XEN_DOMCTL_GRANT_version_mask
>>>> +
>>>> +    uint32_t grant_opts;
>>>
>>> As it now seems unlikely that this will make 4.16, please don't forget
>>> to bump the interface version for 4.17. With that and preferably with
>>> the nit above addressed, hypervisor parts:
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>
>> Actually no, I'm afraid there is an issue with migration: If the tool
>> stack remembers the "use default" setting and hands this to the new
>> host, that host's default may be different from the source host's. It
>> is the effective max-version that needs passing on in this case, which
>> in turn requires a means to obtain the value.
> 
> Hm, my thinking was that the admin (or a higer level orchestration
> tool) would already have performed the necessary adjustments to assert
> the environments are compatible.

I don't think we can rely on this in the hypervisor. We may take this
as a prereq for proper working, but I think we ought to detect
violations and report errors in such a case.

> This problem already exist today where you can migrate a VM from a
> host having the max default grant version to one having
> gnttab=max-ver:1 without complains.

Are you sure about "without complaints"? What would a guest do if it
expects to be able to use v2, since it was able to on the prior host?

> Note that adding such a check would then effectively prevent us from
> lowering the default max grant version, as any incoming migration from
> a previous hypervisor using the default parameters would be rejected.

Right, guests would need booting anew on a such restricted host.

Jan

Re: [PATCH v2 2/6] gnttab: allow setting max version per-domain

[Juergen Gross]

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

On 20.10.21 12:57, Jan Beulich wrote:
> On 20.10.2021 10:04, Roger Pau Monné wrote:
>> On Fri, Oct 15, 2021 at 11:48:33AM +0200, Jan Beulich wrote:
>>> On 15.10.2021 11:39, Jan Beulich wrote:
>>>> On 22.09.2021 10:21, Roger Pau Monne wrote:
>>>>> --- a/xen/include/public/domctl.h
>>>>> +++ b/xen/include/public/domctl.h
>>>>> @@ -87,14 +87,22 @@ 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;
>>>>>   
>>>>> +/* Grant version, use low 4 bits. */
>>>>> +#define XEN_DOMCTL_GRANT_version_mask    0xf
>>>>> +#define XEN_DOMCTL_GRANT_version_default 0xf
>>>>> +
>>>>> +#define XEN_DOMCTLGRANT_MAX XEN_DOMCTL_GRANT_version_mask
>>>>> +
>>>>> +    uint32_t grant_opts;
>>>>
>>>> As it now seems unlikely that this will make 4.16, please don't forget
>>>> to bump the interface version for 4.17. With that and preferably with
>>>> the nit above addressed, hypervisor parts:
>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> Actually no, I'm afraid there is an issue with migration: If the tool
>>> stack remembers the "use default" setting and hands this to the new
>>> host, that host's default may be different from the source host's. It
>>> is the effective max-version that needs passing on in this case, which
>>> in turn requires a means to obtain the value.
>>
>> Hm, my thinking was that the admin (or a higer level orchestration
>> tool) would already have performed the necessary adjustments to assert
>> the environments are compatible.
> 
> I don't think we can rely on this in the hypervisor. We may take this
> as a prereq for proper working, but I think we ought to detect
> violations and report errors in such a case.
> 
>> This problem already exist today where you can migrate a VM from a
>> host having the max default grant version to one having
>> gnttab=max-ver:1 without complains.
> 
> Are you sure about "without complaints"? What would a guest do if it
> expects to be able to use v2, since it was able to on the prior host?

A Linux guest should "just work". On resume it is setting up the grant
interface again like on boot (in fact there is one difference: the
number of grant frames is kept from before suspending).

Guest transparent migration wouldn't work in such a case, and I have no
idea how other OS's will react.


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 v2 3/6] gnttab: allow per-domain control over transitive grants

[Jan Beulich]

On 20.10.2021 12:14, Roger Pau Monné wrote:
> On Fri, Oct 15, 2021 at 12:05:06PM +0200, Jan Beulich wrote:
>> On 22.09.2021 10:21, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -750,7 +750,8 @@ static struct domain *__init create_dom0(const module_t *image,
>>>          .max_evtchn_port = -1,
>>>          .max_grant_frames = -1,
>>>          .max_maptrack_frames = -1,
>>> -        .grant_opts = XEN_DOMCTL_GRANT_version_default,
>>> +        .grant_opts = XEN_DOMCTL_GRANT_version_default |
>>> +                      XEN_DOMCTL_GRANT_transitive,
>>>          .max_vcpus = dom0_max_vcpus(),
>>>          .arch = {
>>>              .misc_flags = opt_dom0_msr_relaxed ? XEN_X86_MSR_RELAXED : 0,
>>
>> While I can see that you make these adjustments for retaining backwards
>> compatibility, I wonder if we need to, at least for Dom0. Dom0 doesn't
>> normally grant anything anyway and hence would even less so use
>> transitive grants. Of course then there's need to be a command line
>> control to re-enable that, just in case.
> 
> dom0=gnttab-transitive, or should it be placed somewhere else?

That sounds like the place to have it at; at least that's where I would
have suggested to put it. One question of course is how it ought to
interact with v2 being unavailable.

>>> @@ -1965,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;
>>
>> No need for !! here afaics. Not even if there weren't the &&.
>>
>> As to combining with the global option: I think if an admin requested
>> them for a domain, this should overrule the command line option. Which
>> in turn suggests that the command line option could go away at this
>> point. Otherwise, if you AND both together and the guest is known to
>> not work without this functionality, domain creation would instead
>> better fail (or at the very least it should be logged by the tool
>> stack that the request wasn't satisfied, which would require a means
>> to retrieve the effective setting). IOW I would see the command line
>> turning this off to trump the per-guest enabling request.
> 
> How should we go about deprecating it? It would be a bit antisocial
> IMO to just drop the option, since people using it would then be
> exposed to guests using transient grants if they didn't realize it
> should be set in xl.conf or xl.cfg now.

So perhaps for a transitional phase fail if the command line option
says no and the request for the guest says yes? Accompanied by a
log message warning that the command line control is going to go
away, so that people will know to adjust their guest configs?

Jan

Re: [PATCH v2 2/6] gnttab: allow setting max version per-domain

[Jan Beulich]

On 22.09.2021 10:21, Roger Pau Monne wrote:
> 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.

One further remark: Already with the command line control is was
questionable in how far people are aware that PV guests may need to
use v2, if the host has memory beyond the 16Tb address boundary. I
wonder whether permitting to restrict to v1 on such hosts isn't
counterproductive. I guess at the very least a warning may want
logging.

Jan

Re: [PATCH v2 2/6] gnttab: allow setting max version per-domain

[Roger Pau Monné]

On Wed, Oct 20, 2021 at 12:57:11PM +0200, Jan Beulich wrote:
> On 20.10.2021 10:04, Roger Pau Monné wrote:
> > On Fri, Oct 15, 2021 at 11:48:33AM +0200, Jan Beulich wrote:
> >> On 15.10.2021 11:39, Jan Beulich wrote:
> >>> On 22.09.2021 10:21, Roger Pau Monne wrote:
> >>>> --- a/xen/include/public/domctl.h
> >>>> +++ b/xen/include/public/domctl.h
> >>>> @@ -87,14 +87,22 @@ 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;
> >>>>  
> >>>> +/* Grant version, use low 4 bits. */
> >>>> +#define XEN_DOMCTL_GRANT_version_mask    0xf
> >>>> +#define XEN_DOMCTL_GRANT_version_default 0xf
> >>>> +
> >>>> +#define XEN_DOMCTLGRANT_MAX XEN_DOMCTL_GRANT_version_mask
> >>>> +
> >>>> +    uint32_t grant_opts;
> >>>
> >>> As it now seems unlikely that this will make 4.16, please don't forget
> >>> to bump the interface version for 4.17. With that and preferably with
> >>> the nit above addressed, hypervisor parts:
> >>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> >>
> >> Actually no, I'm afraid there is an issue with migration: If the tool
> >> stack remembers the "use default" setting and hands this to the new
> >> host, that host's default may be different from the source host's. It
> >> is the effective max-version that needs passing on in this case, which
> >> in turn requires a means to obtain the value.
> > 
> > Hm, my thinking was that the admin (or a higer level orchestration
> > tool) would already have performed the necessary adjustments to assert
> > the environments are compatible.
> 
> I don't think we can rely on this in the hypervisor. We may take this
> as a prereq for proper working, but I think we ought to detect
> violations and report errors in such a case.
> 
> > This problem already exist today where you can migrate a VM from a
> > host having the max default grant version to one having
> > gnttab=max-ver:1 without complains.
> 
> Are you sure about "without complaints"?

Without hypervisor complaining AFAICT, as the max grant version is
not migrated or checked in any way ATM.

> What would a guest do if it
> expects to be able to use v2, since it was able to on the prior host?

IMO any well behaved guest should be capable of handling both v1 and
v2, and lacking v2 a guest should fallback to v1 on resume, as the
grant table needs to be re-initialized anyway. I think it would be
wrong (ie: a bug) for guests to assume v2 to be present on resume
based on the fact it was present previously, as it would be wrong for
a block frontend to assume the same features to be present on resume
for example.

If a guest only supports grant v2 then I think it's up to the
administrator to set the max version explicitly in the config file and
that would be acknowledged on the destination end and migration
aborted if not supported. I think the behavior after this patch
is more user friendly that the current one, as no checks are
performed currently.

Thanks, Roger.

Re: [PATCH v2 5/6] tools/xenstored: partially handle domains without a shared ring

[Julien Grall]

Hi all,

On 23/09/2021 08:56, Julien Grall wrote:
> On 23/09/2021 12:23, Roger Pau Monné wrote:
>> On Wed, Sep 22, 2021 at 06:46:25PM +0500, Julien Grall wrote:
>>> I thought a bit more and looked at the code (I don't have access to a 
>>> test
>>> machine at the moment). I think there is indeed a problem.
>>>
>>> Some watchers of @releaseDomain (such as xenconsoled) will only remove a
>>> domain from their internal state when the domain is actually dead.
>>>
>>> This is based on dominfo.dying which is only set when all the 
>>> resources are
>>> relinquished and waiting for the other domains to release any 
>>> resources for
>>> that domain.
>>>
>>> The problem is Xenstore may fail to map the interface or the event 
>>> channel
>>> long before the domain is actually dead. With the current check, we 
>>> would
>>> free the allocated structure and therefore send @releaseDomain too 
>>> early. So
>>> daemon like xenconsoled, would never cleanup for the domain and leave a
>>> zombie domain. Well... until the next @releaseDomain (or 
>>> @introduceDomain
>>> for Xenconsoled) AFAICT.
>>>
>>> The revised patch is meant to solve it by just ignoring the 
>>> connection. With
>>> that approach, we would correctly notify watches when the domain is 
>>> dead.
>>
>> I think the patch provided by Julien is indeed better than the current
>> code, or else you will miss @releaseDomain events in corner cases for
>> dominfo.dying.
>>
>> I think however the patch is missing a change in the order of the
>> checks in conn_can_{read,write}, so that the is_ignored check is
>> performed before calling can_{read,write} which will try to poke at
>> the interface and trigger a fault if not mapped.
> 
> Ah good point. I originally moved the is_ignored check after the 
> callback because the socket connection can in theory be closed from 
> can_{read, write}.
> 
> However, in pratice, is_ignored is only set for socket from 
> ignore_connection() that will also close the socket.
> 
> The new use will only set is_ignored for the domain connection. So I am 
> guessing moving the check early on ought to be fine.
> 
> The alternative would be to call ignore_connection() but this feels a 
> bit weird because most of it would be a NOP as we are introducing the 
> domain.

At the end I went with re-using ignore_connection() and posted a patch 
for discussion:

https://lore.kernel.org/xen-devel/20211020144519.10362-1-julien@xen.org/T/#u

Cheers,

-- 
Julien Grall