Re: [Xen-devel] [PATCH-for-4.13 v5] Rationalize max_grant_frames and max_maptrack_frames handling

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Ian Jackson writes ("RE: [PATCH-for-4.13 v5] Rationalize max_grant_frames and 
max_maptrack_frames handling"):
> I think our proposal is to drop the type change, continue to use
> uint32_t everwhere for these values, and specify the "use default"
> value to be all-bits-set.

Following discussion on IRC:

I think we toolstack maintainers do not think it right to change the
type anywhere, but the hypervisor API/ABI is outside our bailiwick.

Changing the type in the libxl API is definitely undesirable in this
patch, especially as we will want to backport it.  Changing the type
can cause compile errors in callers (who are out of tree), or even
wrong behaviours.

We (Anthony and I, at least, having discussed it) feel that the
libxl_types.idl type should remain unchanged.

Furthermore, in the existing libxl API, LIBXL_MAX_GRANT_FRAMES_DEFAULT
and LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT have concrete values.  Callers
may have developed code which does something like
LIBXL_MAX_GRANT_FRAMES_DEFAULT*4 to avoid the max_grant_frames issue.
We need to keep those callers working.

I think the best way to do this is to retain those #defines with their
existing values, but to decouple them.  Instead, we provide a new
#define for the sentinel value and change the init_val in the IDL to
refer to that.

The effect is that callers who use _DEFAULT explicitly will see no
change (and the only reason to do that would probably be to increase
it as a bug workaround).  Callers who do not set the value at all will
get the sentinel value from init, which gets passed to the hypervisor
and used as the default.

(In principle callers might call _init on the domain config and then
modify the value they get, but I think that is both not entirely
reasonable and quite unlikely.  I think fixing this issue for others
callers is more important.)

To demonstrate exactly what I mean here is a delta which can be
squashed into v6.

Ian.

From 86d6e3b6e5434f0d872a195ed457a4af25a00208 Mon Sep 17 00:00:00 2001
From: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Date: Fri, 29 Nov 2019 16:51:45 +0000
Subject: [PATCH] squash! Rationalize max_grant_frames and max_maptrack_frames
 handling

---
v7: Do not change type of libxl fields.
    Do not change values of LIBXL_MAX_GRANT_FRAMES_DEFAULT etc.
    in case someone has done LIBXL_MAX_GRANT_FRAMES_DEFAULT*4.
    Deprecate those and provide LIBXL_MAX_GRANT_DEFAULT instead.
---
 tools/libxl/libxl.h         | 16 +++++++++-------
 tools/libxl/libxl_types.idl |  4 ++--
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index b829c1bbce..54abb9db1f 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -364,15 +364,17 @@
  */
 #define LIBXL_HAVE_BUILDINFO_GRANT_LIMITS 1
 
+#define LIBXL_MAX_GRANT_DEFAULT (~(uint32_t)0)
+#define LIBXL_MAX_GRANT_FRAMES_DEFAULT 32 /* deprecated */
+#define LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT 1024 /* deprecated */
 /*
- * LIBXL_HAVE_BUILDINFO_SIGNED_GRANT_LIMITS indicates that the
- * signed max_grant_frames and max_maptrack_frames fields in
- * libxl_domain_build_info are signed quantities.
+ * LIBXL_HAVE_BUILDINFO_GRANT_DEFAULT indicates that the default
+ * values of max_grant_frames and max_maptrack_frames fields in
+ * libxl_domain_build_info are the special sentinel value
+ * LIBXL_MAX_GRANT_DEFAULT rather than the fixed values above.
+ * This means to use the hypervisor's default.
  */
-#define LIBXL_HAVE_BUILDINFO_SIGNED_GRANT_LIMITS 1
-
-#define LIBXL_MAX_GRANT_FRAMES_DEFAULT -1
-#define LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT -1
+#define LIBXL_HAVE_BUILDINFO_GRANT_DEFAULT 1
 
 /*
  * LIBXL_HAVE_BUILDINFO_* indicates that libxl_domain_build_info has
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 63e29bb2fb..7921950f6a 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -511,8 +511,8 @@ libxl_domain_build_info = Struct("domain_build_info",[
 
     ("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")),
 
-    ("max_grant_frames",    integer, {'init_val': 
'LIBXL_MAX_GRANT_FRAMES_DEFAULT'}),
-    ("max_maptrack_frames", integer, {'init_val': 
'LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT'}),
+    ("max_grant_frames",    uint32, {'init_val': 'LIBXL_MAX_GRANT_DEFAULT'}),
+    ("max_maptrack_frames", uint32, {'init_val': 'LIBXL_MAX_GRANT_DEFAULT'}),
     
     ("device_model_version", libxl_device_model_version),
     ("device_model_stubdomain", libxl_defbool),
-- 
2.11.0


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