[Xen-devel] [XEN PATCH for-4.13 v2 0/9] libxl memkb & pt defaulting

[Xen-devel] [XEN PATCH for-4.13 v2 0/9] libxl memkb & pt defaulting

[Ian Jackson]

This is v2 of my series to sort out the shadow/iommu memory and pci
passthrough situation.  I think this series will mask the bug which is
currently blocking staging propagating to master.

I had some difficulty testing this, as the test box under my desk
doesn't do PT and I didn't want to wait to book one out from the test
lab.  So I have executed only some of the new code in libxl.

I would really appreciate review by a hypervisor expert on the main
chunk of new code in libxl added in patch 9, which is where all the
logic related to passthrough enabling and mode now resides.

 am 1 libxl: Offer API versions 0x040700 and 0x040800
 r  2 xl: Pass libxl_domain_config to freemem(), instead of b_info
 r* 3 libxl: libxl__domain_config_setdefault: New function
 r* 4 libxl: libxl_domain_need_memory: Make it take a domain_config
  * 5 libxl: Move shadow_memkb and iommu_memkb defaulting into libxl
 a  6 libxl: Remove/deprecate libxl_get_required_*_memory from the API
  + 7 libxl: create: setdefault: Make libxl_physinfo info[1]
  + 8 libxl: create: setdefault: Move physinfo into config_setdefault
  + 9 libxl/xl: Overhaul passthrough setting logic

a=acked; r=reviewed; m=message changed; *=patch changed; +=new patch

 tools/libxl/libxl.h          |  24 +++++-
 tools/libxl/libxl_create.c   | 174 ++++++++++++++++++++++++++++++++++---------
 tools/libxl/libxl_dm.c       |   7 +-
 tools/libxl/libxl_dom.c      |   7 +-
 tools/libxl/libxl_internal.h |  13 +++-
 tools/libxl/libxl_mem.c      |  69 +++++++++++++----
 tools/libxl/libxl_types.idl  |   5 +-
 tools/libxl/libxl_utils.c    |  15 ----
 tools/libxl/libxl_utils.h    |   2 +-
 tools/xl/xl_parse.c          |  82 ++------------------
 tools/xl/xl_vmcontrol.c      |   6 +-
 11 files changed, 254 insertions(+), 150 deletions(-)

-- 
2.11.0


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

[Xen-devel] [XEN PATCH for-4.13 v2 1/9] libxl: Offer API versions 0x040700 and 0x040800

[Ian Jackson]

According to git log -G:

0x040700 was introduced in 304400459ef0 (aka 4.7.0-rc1~481)
  "tools/libxl: rename remus device to checkpoint device"

0x040800 was introduced in 57f8b13c7240 (aka 4.8.0-rc1~437)
  "libxl: memory size in kb requires 64 bit variable"

It is surprising that no-one noticed this.

Anyway, in the meantime, we should fix it.  Backporting this is
probably a good idea: it won't change the behaviour for existing
callers but it will avoid errors for some older correct uses.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
---
v2: Adjusted commit message slightly.
---
 tools/libxl/libxl.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 2555e9cd3b..518fc9e47f 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -655,6 +655,7 @@ typedef struct libxl__ctx libxl_ctx;
 #ifdef LIBXL_API_VERSION
 #if LIBXL_API_VERSION != 0x040200 && LIBXL_API_VERSION != 0x040300 && \
     LIBXL_API_VERSION != 0x040400 && LIBXL_API_VERSION != 0x040500 && \
+    LIBXL_API_VERSION != 0x040700 && LIBXL_API_VERSION != 0x040800 && \
     LIBXL_API_VERSION != 0x041300
 #error Unknown LIBXL_API_VERSION
 #endif
-- 
2.11.0


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

Re: [Xen-devel] [XEN PATCH for-4.13 v2 1/9] libxl: Offer API versions 0x040700 and 0x040800

[Jim Fehlig]

On 10/10/19 9:11 AM, Ian Jackson wrote:
> According to git log -G:
> 
> 0x040700 was introduced in 304400459ef0 (aka 4.7.0-rc1~481)
>    "tools/libxl: rename remus device to checkpoint device"
> 
> 0x040800 was introduced in 57f8b13c7240 (aka 4.8.0-rc1~437)
>    "libxl: memory size in kb requires 64 bit variable"
> 
> It is surprising that no-one noticed this.

I am now noticing it :-(.

As Anthony noted in V1, libvirt uses LIBXL_API_VERSION and currently has it set 
to 0x040500. I'm attempting to bump libvirt's minimum supported Xen version to 
4.9.0 and for that would use 0x040800, but it's not possible without this commit 
backported through 4.9 and picked up and released by all the downstreams.

Any ideas on how to use the APIs changes through 0x040800, but avoid the ones 
introduced in 0x041300 would be much appreciated.

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

[Xen-devel] [XEN PATCH for-4.13 v2 2/9] xl: Pass libxl_domain_config to freemem(), instead of b_info

[Ian Jackson]

We are going to change the libxl API in a moment and this change will
make it simpler.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/xl/xl_vmcontrol.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
index b20582e15b..d33c6b38c9 100644
--- a/tools/xl/xl_vmcontrol.c
+++ b/tools/xl/xl_vmcontrol.c
@@ -314,7 +314,7 @@ static int domain_wait_event(uint32_t domid, libxl_event **event_r)
  * Returns true in case there is already, or we manage to free it, enough
  * memory, but also if autoballoon is false.
  */
-static bool freemem(uint32_t domid, libxl_domain_build_info *b_info)
+static bool freemem(uint32_t domid, libxl_domain_config *d_config)
 {
     int rc, retries = 3;
     uint64_t need_memkb, free_memkb;
@@ -322,7 +322,7 @@ static bool freemem(uint32_t domid, libxl_domain_build_info *b_info)
     if (!autoballoon)
         return true;
 
-    rc = libxl_domain_need_memory(ctx, b_info, &need_memkb);
+    rc = libxl_domain_need_memory(ctx, &d_config->b_info, &need_memkb);
     if (rc < 0)
         return false;
 
@@ -879,7 +879,7 @@ start:
         goto error_out;
 
     if (domid_soft_reset == INVALID_DOMID) {
-        if (!freemem(domid, &d_config.b_info)) {
+        if (!freemem(domid, &d_config)) {
             fprintf(stderr, "failed to free memory for the domain\n");
             ret = ERROR_FAIL;
             goto error_out;
-- 
2.11.0


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

[Xen-devel] [XEN PATCH for-4.13 v2 3/9] libxl: libxl__domain_config_setdefault: New function

[Ian Jackson]

Break out this into a new function.  We are going to want to call it
from a new call site.

Unfortunately not all of the defaults can be moved into the new
function without changing the order in which things are done.  That
does not seem wise at this stage of the release.  The effect is that
additional calls to libxl__domain_config_setdefault (which are going
to be introduced) do not quite set everything.  But they will do what
is needed.  After Xen 4.13 is done, we should move those settings into
the right order.

No functional change.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
---
v2: Add missing error check
---
 tools/libxl/libxl_create.c   | 41 +++++++++++++++++++++++++++++------------
 tools/libxl/libxl_internal.h |  3 +++
 2 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 099761a2d7..fd8bb22be9 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -862,22 +862,14 @@ static void domcreate_destruction_cb(libxl__egc *egc,
                                      libxl__domain_destroy_state *dds,
                                      int rc);
 
-static void initiate_domain_create(libxl__egc *egc,
-                                   libxl__domain_create_state *dcs)
+int libxl__domain_config_setdefault(libxl__gc *gc,
+                                    libxl_domain_config *d_config,
+                                    uint32_t domid)
 {
-    STATE_AO_GC(dcs->ao);
     libxl_ctx *ctx = libxl__gc_owner(gc);
-    uint32_t domid;
-    int i, ret;
+    int ret;
     bool pod_enabled = false;
 
-    /* convenience aliases */
-    libxl_domain_config *const d_config = dcs->guest_config;
-    const int restore_fd = dcs->restore_fd;
-
-    domid = dcs->domid_soft_reset;
-    libxl__domain_build_state_init(&dcs->build_state);
-
     if (d_config->c_info.ssid_label) {
         char *s = d_config->c_info.ssid_label;
         ret = libxl_flask_context_to_sid(ctx, s, strlen(s),
@@ -1008,6 +1000,28 @@ static void initiate_domain_create(libxl__egc *egc,
         goto error_out;
     }
 
+    ret = 0;
+ error_out:
+    return ret;
+}
+
+static void initiate_domain_create(libxl__egc *egc,
+                                   libxl__domain_create_state *dcs)
+{
+    STATE_AO_GC(dcs->ao);
+    uint32_t domid;
+    int i, ret;
+
+    /* convenience aliases */
+    libxl_domain_config *const d_config = dcs->guest_config;
+    const int restore_fd = dcs->restore_fd;
+
+    domid = dcs->domid_soft_reset;
+    libxl__domain_build_state_init(&dcs->build_state);
+
+    ret = libxl__domain_config_setdefault(gc,d_config,domid);
+    if (ret) goto error_out;
+
     ret = libxl__domain_make(gc, d_config, &dcs->build_state, &domid);
     if (ret) {
         LOGD(ERROR, domid, "cannot make domain: %d", ret);
@@ -1019,6 +1033,9 @@ static void initiate_domain_create(libxl__egc *egc,
     dcs->guest_domid = domid;
     dcs->sdss.dm.guest_domid = 0; /* means we haven't spawned */
 
+    /* post-4.13 todo: move these next bits of defaulting to
+     * libxl__domain_config_setdefault */
+
     /*
      * Set the dm version quite early so that libxl doesn't have to pass the
      * build info around just to know if the domain has a device model or not.
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index d2d5af746b..50ac7b64ed 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1440,6 +1440,9 @@ _hidden int libxl__resolve_domid(libxl__gc *gc, const char *name,
  *     All libxl API functions are expected to have arranged for this
  *     to be called before using any values within these structures.
  */
+_hidden int libxl__domain_config_setdefault(libxl__gc *gc,
+                                            libxl_domain_config *d_config,
+                                            uint32_t domid /* logging only */);
 _hidden int libxl__domain_create_info_setdefault(libxl__gc *gc,
                                         libxl_domain_create_info *c_info);
 _hidden int libxl__domain_build_info_setdefault(libxl__gc *gc,
-- 
2.11.0


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

[Xen-devel] [XEN PATCH for-4.13 v2 4/9] libxl: libxl_domain_need_memory: Make it take a domain_config

[Ian Jackson]

This should calculate the extra memory needed for shadow and iommu,
the defaults for which depend on values in c_info.  So we need this to
have the complete domain config available.

And the defaults should actually be updated and stored.  So make it
non-const.

We provide the usual kind of compatibility function for callers
expecting 4.12 and earlier.  This function becomes responsible for the
clone-and-modify of the b_info.

No overall functional change for external libxl callers which use the
API version system to request a particular API version.

Other external libxl callers will need to update their calling code,
and will then find that the new version of this function fills in most
of the defaults in d_config.  Because libxl__domain_config_setdefault
doesn't quite do all of the defaults, that's only partial.  For
present purposes that doesn't matter because none of the missing
settings are used by the memory calculations.  It does mean we need to
document in the API spec that the defaulting is only partial.

This lack of functional change is despite the fact that
numa_place_domain now no longer calls
libxl__domain_build_info_setdefault (via libxl_domain_need_memory).
That is OK because it's idempotent and numa_place_domain's one call
site is libxl__build_pre which is called from libxl__domain_build
which is called from domcreate_bootloader_done, well after the
defaults are set by initiate_domain_create.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
---
v2: Drop now-erroneous GC_FREE as well as the corresponding GC_INIT.
---
 tools/libxl/libxl.h          | 23 +++++++++++++++-
 tools/libxl/libxl_dom.c      |  7 +++--
 tools/libxl/libxl_internal.h |  4 +++
 tools/libxl/libxl_mem.c      | 65 +++++++++++++++++++++++++++++++++++---------
 tools/xl/xl_vmcontrol.c      |  2 +-
 5 files changed, 83 insertions(+), 18 deletions(-)

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 518fc9e47f..49b56fa1a3 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1245,6 +1245,20 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, const libxl_mac *src);
  */
 #define LIBXL_HAVE_FN_USING_QMP_ASYNC 1
 
+/*
+ * LIBXL_HAVE_DOMAIN_NEED_MEMORY_CONFIG
+ *
+ * If this is set, libxl_domain_need_memory takes a
+ * libxl_domain_config* (non-const) and uint32_t domid_for_logging
+ * (instead of a const libxl_domain_build_info*).
+ *
+ * If this is set, there is no need to call
+ * libxl_get_required_shadow_memory and instead the caller should
+ * simply leave shadow_memkb set to LIBXL_MEMKB_DEFAULT and allow
+ * libxl to fill in a suitable default in the usual way.
+ */
+#define LIBXL_HAVE_DOMAIN_NEED_MEMORY_CONFIG
+
 typedef char **libxl_string_list;
 void libxl_string_list_dispose(libxl_string_list *sl);
 int libxl_string_list_length(const libxl_string_list *sl);
@@ -1723,8 +1737,13 @@ int libxl_get_memory_target_0x040700(libxl_ctx *ctx, uint32_t domid,
  */
 /* how much free memory in the system a domain needs to be built */
 int libxl_domain_need_memory(libxl_ctx *ctx,
-                             const libxl_domain_build_info *b_info_in,
+                             libxl_domain_config *config
+                             /* ^ will be partially defaulted */,
+                             uint32_t domid_for_logging /* INVALID_DOMID ok */,
                              uint64_t *need_memkb);
+int libxl_domain_need_memory_0x041200(libxl_ctx *ctx,
+                                      const libxl_domain_build_info *b_info_in,
+                                      uint64_t *need_memkb);
 int libxl_domain_need_memory_0x040700(libxl_ctx *ctx,
                                       const libxl_domain_build_info *b_info_in,
                                       uint32_t *need_memkb)
@@ -1754,6 +1773,8 @@ int libxl_wait_for_memory_target(libxl_ctx *ctx, uint32_t domid, int wait_secs);
 #define libxl_get_memory_target libxl_get_memory_target_0x040700
 #define libxl_domain_need_memory libxl_domain_need_memory_0x040700
 #define libxl_get_free_memory libxl_get_free_memory_0x040700
+#elif defined(LIBXL_API_VERSION) && LIBXL_API_VERSION < 0x041300
+#define libxl_domain_need_memory libxl_domain_need_memory_0x041200
 #endif
 
 int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass);
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index c5685b061c..cdb294ab8d 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -140,8 +140,9 @@ static int numa_cmpf(const libxl__numa_candidate *c1,
 
 /* The actual automatic NUMA placement routine */
 static int numa_place_domain(libxl__gc *gc, uint32_t domid,
-                             libxl_domain_build_info *info)
+                             libxl_domain_config *d_config)
 {
+    libxl_domain_build_info *info = &d_config->b_info;
     int found;
     libxl__numa_candidate candidate;
     libxl_bitmap cpumap, cpupool_nodemap, *map;
@@ -195,7 +196,7 @@ static int numa_place_domain(libxl__gc *gc, uint32_t domid,
         }
     }
 
-    rc = libxl_domain_need_memory(CTX, info, &memkb);
+    rc = libxl__domain_need_memory_calculate(gc, info, &memkb);
     if (rc)
         goto out;
     if (libxl_node_bitmap_alloc(CTX, &cpupool_nodemap, 0)) {
@@ -432,7 +433,7 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
             if (rc)
                 return rc;
 
-            rc = numa_place_domain(gc, domid, info);
+            rc = numa_place_domain(gc, domid, d_config);
             if (rc) {
                 libxl_bitmap_dispose(&cpumap_soft);
                 return rc;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 50ac7b64ed..01de5576d9 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1450,6 +1450,10 @@ _hidden int libxl__domain_build_info_setdefault(libxl__gc *gc,
 _hidden void libxl__rdm_setdefault(libxl__gc *gc,
                                    libxl_domain_build_info *b_info);
 
+_hidden int libxl__domain_need_memory_calculate(libxl__gc *gc,
+                                      libxl_domain_build_info *b_info,
+                                      uint64_t *need_memkb);
+
 _hidden const char *libxl__device_nic_devname(libxl__gc *gc,
                                               uint32_t domid,
                                               uint32_t devid,
diff --git a/tools/libxl/libxl_mem.c b/tools/libxl/libxl_mem.c
index fd6f33312e..6042299393 100644
--- a/tools/libxl/libxl_mem.c
+++ b/tools/libxl/libxl_mem.c
@@ -446,20 +446,12 @@ int libxl_get_memory_target_0x040700(
     return libxl__memkb_64to32(ctx, rc, my_out_target, out_target);
 }
 
-int libxl_domain_need_memory(libxl_ctx *ctx,
-                             const libxl_domain_build_info *b_info_in,
-                             uint64_t *need_memkb)
+int libxl__domain_need_memory_calculate(libxl__gc *gc,
+                              libxl_domain_build_info *b_info,
+                              uint64_t *need_memkb)
 {
-    GC_INIT(ctx);
-    libxl_domain_build_info b_info[1];
     int rc;
 
-    libxl_domain_build_info_init(b_info);
-    libxl_domain_build_info_copy(ctx, b_info, b_info_in);
-
-    rc = libxl__domain_build_info_setdefault(gc, b_info);
-    if (rc) goto out;
-
     *need_memkb = b_info->target_memkb;
     *need_memkb += b_info->shadow_memkb + b_info->iommu_memkb;
 
@@ -481,10 +473,57 @@ int libxl_domain_need_memory(libxl_ctx *ctx,
         *need_memkb += (2 * 1024) - (*need_memkb % (2 * 1024));
     rc = 0;
 out:
+    return rc;
+}
+
+int libxl_domain_need_memory(libxl_ctx *ctx,
+                             libxl_domain_config *d_config,
+                             uint32_t domid_for_logging,
+                             uint64_t *need_memkb)
+{
+    GC_INIT(ctx);
+    int rc;
+
+    rc = libxl__domain_config_setdefault(gc,
+                                         d_config,
+                                         domid_for_logging);
+    if (rc) goto out;
+
+    rc = libxl__domain_need_memory_calculate(gc,
+                                   &d_config->b_info,
+                                   need_memkb);
+    if (rc) goto out;
+
+    rc = 0;
+ out:
     GC_FREE;
-    libxl_domain_build_info_dispose(b_info);
     return rc;
+}
+
+int libxl_domain_need_memory_0x041200(libxl_ctx *ctx,
+                                      const libxl_domain_build_info *b_info_in,
+                                      uint64_t *need_memkb)
+{
+    GC_INIT(ctx);
+    int rc;
+
+    libxl_domain_build_info b_info[1];
+    libxl_domain_build_info_init(b_info);
+    libxl_domain_build_info_copy(ctx, b_info, b_info_in);
+
+    rc = libxl__domain_build_info_setdefault(gc, b_info);
+    if (rc) goto out;
 
+    rc = libxl__domain_need_memory_calculate(gc,
+                                   b_info,
+                                   need_memkb);
+    if (rc) goto out;
+
+    rc = 0;
+ out:
+    libxl_domain_build_info_dispose(b_info);
+    GC_FREE;
+    return rc;
 }
 
 int libxl_domain_need_memory_0x040700(libxl_ctx *ctx,
@@ -494,7 +533,7 @@ int libxl_domain_need_memory_0x040700(libxl_ctx *ctx,
     uint64_t my_need_memkb;
     int rc;
 
-    rc = libxl_domain_need_memory(ctx, b_info_in, &my_need_memkb);
+    rc = libxl_domain_need_memory_0x041200(ctx, b_info_in, &my_need_memkb);
     return libxl__memkb_64to32(ctx, rc, my_need_memkb, need_memkb);
 }
 
diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
index d33c6b38c9..e520b1da79 100644
--- a/tools/xl/xl_vmcontrol.c
+++ b/tools/xl/xl_vmcontrol.c
@@ -322,7 +322,7 @@ static bool freemem(uint32_t domid, libxl_domain_config *d_config)
     if (!autoballoon)
         return true;
 
-    rc = libxl_domain_need_memory(ctx, &d_config->b_info, &need_memkb);
+    rc = libxl_domain_need_memory(ctx, d_config, domid, &need_memkb);
     if (rc < 0)
         return false;
 
-- 
2.11.0


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

[Xen-devel] [XEN PATCH for-4.13 v2 5/9] libxl: Move shadow_memkb and iommu_memkb defaulting into libxl

[Ian Jackson]

Defaulting is supposed to be done by libxl.  So these calculations
should be here in libxl.  libxl__domain_config_setdefault has all the
necessary information including the values of max_memkb and max_vcpus.

The overall functional effect depends on the caller:

For xl, no change.  The code moves from xl to libxl.

For callers who set one or both shadow_memkb and iommu_memkb (whether
from libxl_get_required_shadow_memory or otherwise) before calling
libxl_domain_need_memory (any version): the new code will leave their
setting(s) unchanged.

For callers who do not call libxl_domain_need_memory at all, and who
fail to set one of these memory values: now they are both are properly
set.  The shadow and iommu memory to be properly accounted for as
intended.

For callers which call libxl_domain_need_memory and request the
current API (4.13) or which track libxl, the default values are also
now right and everything works as intended.

For callers which call libxl_domain_need_memory, and request an old
pre-4.13 libxl API, and which leave one of these memkb settings unset,
we take special measures to preserve the old behaviour.

This means that they don't get the additional iommu memory and are at
risk of the domain running out of memory as a result of f89f555827a6
"remove late (on-demand) construction of IOMMU page tables".  But this
is no worse than the state just after f89f555827a6, which already
broke such callers in that way.  This is perhaps justifiable because
of the API stability warning next to libxl_domain_need_memory.

An alternative would be to drop the special-casing of these callers.
That would cause a discrepancy between libxl_domain_need_memory and
libxl_domain_create: the former would not include the iommu memory and
the latter would.  That seems worse, but it's debateable.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
v2: Replace _Bool with bool
v2: Fix logic sense in ok_to_default_memkb_in_create
---
 tools/libxl/libxl_create.c   | 40 ++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_internal.h |  3 +++
 tools/libxl/libxl_mem.c      |  4 ++++
 tools/xl/xl_parse.c          | 15 ++-------------
 4 files changed, 49 insertions(+), 13 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index fd8bb22be9..a1b00a8aef 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -250,6 +250,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
     switch (b_info->type) {
     case LIBXL_DOMAIN_TYPE_HVM:
         if (b_info->shadow_memkb == LIBXL_MEMKB_DEFAULT)
+            /* Normally defaulted in libxl__domain_create_info_setdefault */
             b_info->shadow_memkb = 0;
         if (b_info->u.hvm.mmio_hole_memkb == LIBXL_MEMKB_DEFAULT)
             b_info->u.hvm.mmio_hole_memkb = 0;
@@ -395,6 +396,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
         if (b_info->video_memkb == LIBXL_MEMKB_DEFAULT)
             b_info->video_memkb = 0;
         if (b_info->shadow_memkb == LIBXL_MEMKB_DEFAULT)
+            /* Normally defaulted in libxl__domain_create_info_setdefault */
             b_info->shadow_memkb = 0;
         if (b_info->u.pv.slack_memkb == LIBXL_MEMKB_DEFAULT)
             b_info->u.pv.slack_memkb = 0;
@@ -862,6 +864,30 @@ static void domcreate_destruction_cb(libxl__egc *egc,
                                      libxl__domain_destroy_state *dds,
                                      int rc);
 
+static bool ok_to_default_memkb_in_create(libxl__gc *gc)
+{
+    /*
+     * This is a fudge.  We are trying to find whether the caller
+     * calls the old version of libxl_domain_need_memory.  If they do
+     * then, because it only gets the b_info, and because it can't
+     * update the b_info (because it's const), it will base its
+     * calculations on defaulting shadow_memkb and iommu_memkb to 0
+     * In that case we probably shouldn't default them differently
+     * during libxl_domain_create.
+     *
+     * The result is that the behaviour with old callers is the same
+     * as in 4.13: no additional memory is allocated for shadow and
+     * iommu (unless the caller set shadow_memkb, eg from a call to
+     * libxl_get_required_shadow_memory).
+     */
+    return !CTX->libxl_domain_need_memory_0x041200_called ||
+            CTX->libxl_domain_need_memory_called;
+    /*
+     * Treat mixed callers as new callers.  Presumably they know what
+     * they are doing.
+     */
+}
+
 int libxl__domain_config_setdefault(libxl__gc *gc,
                                     libxl_domain_config *d_config,
                                     uint32_t domid)
@@ -974,6 +1000,20 @@ int libxl__domain_config_setdefault(libxl__gc *gc,
         goto error_out;
     }
 
+    if (d_config->b_info.shadow_memkb == LIBXL_MEMKB_DEFAULT
+        && ok_to_default_memkb_in_create(gc))
+        d_config->b_info.shadow_memkb =
+            libxl_get_required_shadow_memory(d_config->b_info.max_memkb,
+                                             d_config->b_info.max_vcpus);
+
+    /* No IOMMU reservation is needed if passthrough mode is not 'sync_pt' */
+    if (d_config->b_info.iommu_memkb == LIBXL_MEMKB_DEFAULT
+        && ok_to_default_memkb_in_create(gc))
+        d_config->b_info.iommu_memkb =
+            (d_config->c_info.passthrough == LIBXL_PASSTHROUGH_SYNC_PT)
+            ? libxl_get_required_iommu_memory(d_config->b_info.max_memkb)
+            : 0;
+
     ret = libxl__domain_build_info_setdefault(gc, &d_config->b_info);
     if (ret) {
         LOGD(ERROR, domid, "Unable to set domain build info defaults");
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 01de5576d9..0185b8ff01 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -637,6 +637,9 @@ struct libxl__ctx {
     LIBXL_LIST_ENTRY(libxl_ctx) sigchld_users_entry;
 
     libxl_version_info version_info;
+
+    bool libxl_domain_need_memory_0x041200_called,
+         libxl_domain_need_memory_called;
 };
 
 /*
diff --git a/tools/libxl/libxl_mem.c b/tools/libxl/libxl_mem.c
index 6042299393..7c01fac7e5 100644
--- a/tools/libxl/libxl_mem.c
+++ b/tools/libxl/libxl_mem.c
@@ -484,6 +484,8 @@ int libxl_domain_need_memory(libxl_ctx *ctx,
     GC_INIT(ctx);
     int rc;
 
+    ctx->libxl_domain_need_memory_called = 1;
+
     rc = libxl__domain_config_setdefault(gc,
                                          d_config,
                                          domid_for_logging);
@@ -507,6 +509,8 @@ int libxl_domain_need_memory_0x041200(libxl_ctx *ctx,
     GC_INIT(ctx);
     int rc;
 
+    ctx->libxl_domain_need_memory_0x041200_called = 1;
+
     libxl_domain_build_info b_info[1];
     libxl_domain_build_info_init(b_info);
     libxl_domain_build_info_copy(ctx, b_info, b_info_in);
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 03a2c54dd2..79871c22d0 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1572,19 +1572,8 @@ void parse_config_data(const char *config_source,
         exit(-ERROR_FAIL);
     }
 
-    /* libxl_get_required_shadow_memory() and
-     * libxl_get_required_iommu_memory() must be called after final values
-     * (default or specified) for vcpus and memory are set, because the
-     * calculations depend on those values. */
-    b_info->shadow_memkb = !xlu_cfg_get_long(config, "shadow_memory", &l, 0)
-        ? l * 1024
-        : libxl_get_required_shadow_memory(b_info->max_memkb,
-                                           b_info->max_vcpus);
-
-    /* No IOMMU reservation is needed if passthrough mode is not 'sync_pt' */
-    b_info->iommu_memkb = (c_info->passthrough == LIBXL_PASSTHROUGH_SYNC_PT)
-        ? libxl_get_required_iommu_memory(b_info->max_memkb)
-        : 0;
+    if (!xlu_cfg_get_long(config, "shadow_memory", &l, 0))
+        b_info->shadow_memkb = l * 1024;
 
     xlu_cfg_get_defbool(config, "nomigrate", &b_info->disable_migrate, 0);
 
-- 
2.11.0


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

[Xen-devel] [XEN PATCH for-4.13 v2 6/9] libxl: Remove/deprecate libxl_get_required_*_memory from the API

[Ian Jackson]

These are now redundant because shadow_memkb and iommu_memkb are now
defaulted automatically by libxl_domain_need_memory and
libxl_domain_create etc.  Callers should not now call these; instead,
they should just let libxl take care of it.

libxl_get_required_shadow_memory was introduced in f89f555827a6
  "remove late (on-demand) construction of IOMMU page tables"
We can freely remove it because it was never in any release.

libxl_get_required_shadow_memory has been in libxl approximately
forever.  It should probably not have survived the creation of
libxl_domain_create, but it seems the API awkwardnesses we see in
recent commits prevented this.  So we have to keep it.  It remains
functional but we can deprecate it.  Hopefully we can get rid of it
completely before we find the need to change the calculation to use
additional information which its arguments do not currently supply.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_create.c | 17 ++++++++++++++++-
 tools/libxl/libxl_utils.c  | 15 ---------------
 tools/libxl/libxl_utils.h  |  2 +-
 3 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index a1b00a8aef..b394312d98 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -888,6 +888,21 @@ static bool ok_to_default_memkb_in_create(libxl__gc *gc)
      */
 }
 
+static unsigned long libxl__get_required_iommu_memory(unsigned long maxmem_kb)
+{
+    unsigned long iommu_pages = 0, mem_pages = maxmem_kb / 4;
+    unsigned int level;
+
+    /* Assume a 4 level page table with 512 entries per level */
+    for (level = 0; level < 4; level++)
+    {
+        mem_pages = DIV_ROUNDUP(mem_pages, 512);
+        iommu_pages += mem_pages;
+    }
+
+    return iommu_pages * 4;
+}
+
 int libxl__domain_config_setdefault(libxl__gc *gc,
                                     libxl_domain_config *d_config,
                                     uint32_t domid)
@@ -1011,7 +1026,7 @@ int libxl__domain_config_setdefault(libxl__gc *gc,
         && ok_to_default_memkb_in_create(gc))
         d_config->b_info.iommu_memkb =
             (d_config->c_info.passthrough == LIBXL_PASSTHROUGH_SYNC_PT)
-            ? libxl_get_required_iommu_memory(d_config->b_info.max_memkb)
+            ? libxl__get_required_iommu_memory(d_config->b_info.max_memkb)
             : 0;
 
     ret = libxl__domain_build_info_setdefault(gc, &d_config->b_info);
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index 405733b7e1..f360f5e228 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -48,21 +48,6 @@ unsigned long libxl_get_required_shadow_memory(unsigned long maxmem_kb, unsigned
     return 4 * (256 * smp_cpus + 2 * (maxmem_kb / 1024));
 }
 
-unsigned long libxl_get_required_iommu_memory(unsigned long maxmem_kb)
-{
-    unsigned long iommu_pages = 0, mem_pages = maxmem_kb / 4;
-    unsigned int level;
-
-    /* Assume a 4 level page table with 512 entries per level */
-    for (level = 0; level < 4; level++)
-    {
-        mem_pages = DIV_ROUNDUP(mem_pages, 512);
-        iommu_pages += mem_pages;
-    }
-
-    return iommu_pages * 4;
-}
-
 char *libxl_domid_to_name(libxl_ctx *ctx, uint32_t domid)
 {
     unsigned int len;
diff --git a/tools/libxl/libxl_utils.h b/tools/libxl/libxl_utils.h
index 630ccbe28a..46918aea84 100644
--- a/tools/libxl/libxl_utils.h
+++ b/tools/libxl/libxl_utils.h
@@ -24,7 +24,7 @@ const
 char *libxl_basename(const char *name); /* returns string from strdup */
 
 unsigned long libxl_get_required_shadow_memory(unsigned long maxmem_kb, unsigned int smp_cpus);
-unsigned long libxl_get_required_iommu_memory(unsigned long maxmem_kb);
+  /* deprecated; see LIBXL_HAVE_DOMAIN_NEED_MEMORY_CONFIG in libxl.h */
 int libxl_name_to_domid(libxl_ctx *ctx, const char *name, uint32_t *domid);
 int libxl_domain_qualifier_to_domid(libxl_ctx *ctx, const char *name, uint32_t *domid);
 char *libxl_domid_to_name(libxl_ctx *ctx, uint32_t domid);
-- 
2.11.0


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

[Xen-devel] [XEN PATCH for-4.13 v2 7/9] libxl: create: setdefault: Make libxl_physinfo info[1]

[Ian Jackson]

No functional change.  This will let us make it into a pointer without
textual change other than to the definition.

While we are here, fix some style errors (missing { }).

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
v2: New patch in this version of the series.
---
 tools/libxl/libxl_create.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index b394312d98..9c56a914ca 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -30,10 +30,10 @@
 int libxl__domain_create_info_setdefault(libxl__gc *gc,
                                          libxl_domain_create_info *c_info)
 {
-    libxl_physinfo info;
+    libxl_physinfo info[1];
     int rc;
 
-    rc = libxl_get_physinfo(CTX, &info);
+    rc = libxl_get_physinfo(CTX, info);
     if (rc)
         return rc;
 
@@ -45,11 +45,11 @@ int libxl__domain_create_info_setdefault(libxl__gc *gc,
     libxl__arch_domain_create_info_setdefault(gc, c_info);
 
     if (c_info->type != LIBXL_DOMAIN_TYPE_PV) {
-        if (info.cap_hap)
+        if (info->cap_hap) {
             libxl_defbool_setdefault(&c_info->hap, true);
-        else if (info.cap_shadow)
+        } else if (info->cap_shadow) {
             libxl_defbool_setdefault(&c_info->hap, false);
-        else {
+        } else {
             LOG(ERROR, "neither hap nor shadow paging available");
             return ERROR_INVAL;
         }
@@ -63,12 +63,12 @@ int libxl__domain_create_info_setdefault(libxl__gc *gc,
     if (!c_info->ssidref)
         c_info->ssidref = SECINITSID_DOMU;
 
-    if (info.cap_hvm_directio &&
+    if (info->cap_hvm_directio &&
         (c_info->passthrough == LIBXL_PASSTHROUGH_UNKNOWN)) {
         c_info->passthrough = ((c_info->type == LIBXL_DOMAIN_TYPE_PV) ||
-                               !info.cap_iommu_hap_pt_share) ?
+                               !info->cap_iommu_hap_pt_share) ?
             LIBXL_PASSTHROUGH_SYNC_PT : LIBXL_PASSTHROUGH_SHARE_PT;
-    } else if (!info.cap_hvm_directio) {
+    } else if (!info->cap_hvm_directio) {
         c_info->passthrough = LIBXL_PASSTHROUGH_DISABLED;
     }
 
-- 
2.11.0


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

Re: [Xen-devel] [XEN PATCH for-4.13 v2 7/9] libxl: create: setdefault: Make libxl_physinfo info[1]

[Wei Liu]

On Thu, Oct 10, 2019 at 04:11:09PM +0100, Ian Jackson wrote:
> No functional change.  This will let us make it into a pointer without
> textual change other than to the definition.
> 
> While we are here, fix some style errors (missing { }).
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>


Acked-by: Wei Liu <wl@xen.org>

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

[Xen-devel] [XEN PATCH for-4.13 v2 8/9] libxl: create: setdefault: Move physinfo into config_setdefault

[Ian Jackson]

No functional change.  This will let us refer to it in code we are
about to add to this function.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
v2: New patch in this version of the series.
---
 tools/libxl/libxl_create.c   | 17 ++++++++---------
 tools/libxl/libxl_dm.c       |  7 ++++++-
 tools/libxl/libxl_internal.h |  3 ++-
 3 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 9c56a914ca..69971c97b6 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -28,15 +28,9 @@
 #include <xen-xsm/flask/flask.h>
 
 int libxl__domain_create_info_setdefault(libxl__gc *gc,
-                                         libxl_domain_create_info *c_info)
+                                         libxl_domain_create_info *c_info,
+                                         const libxl_physinfo *info)
 {
-    libxl_physinfo info[1];
-    int rc;
-
-    rc = libxl_get_physinfo(CTX, info);
-    if (rc)
-        return rc;
-
     if (!c_info->type) {
         LOG(ERROR, "domain type unspecified");
         return ERROR_INVAL;
@@ -911,6 +905,10 @@ int libxl__domain_config_setdefault(libxl__gc *gc,
     int ret;
     bool pod_enabled = false;
 
+    libxl_physinfo physinfo;
+    ret = libxl_get_physinfo(CTX, &physinfo);
+    if (ret) goto error_out;
+
     if (d_config->c_info.ssid_label) {
         char *s = d_config->c_info.ssid_label;
         ret = libxl_flask_context_to_sid(ctx, s, strlen(s),
@@ -1009,7 +1007,8 @@ int libxl__domain_config_setdefault(libxl__gc *gc,
         goto error_out;
     }
 
-    ret = libxl__domain_create_info_setdefault(gc, &d_config->c_info);
+    ret = libxl__domain_create_info_setdefault(gc, &d_config->c_info,
+                                               &physinfo);
     if (ret) {
         LOGD(ERROR, domid, "Unable to set domain create info defaults");
         goto error_out;
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index c00356a2f1..e6a48974f8 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2167,7 +2167,12 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
     dm_config->c_info.run_hotplug_scripts =
         guest_config->c_info.run_hotplug_scripts;
 
-    ret = libxl__domain_create_info_setdefault(gc, &dm_config->c_info);
+    libxl_physinfo physinfo;
+    ret = libxl_get_physinfo(CTX, &physinfo);
+    if (ret) goto out;
+
+    ret = libxl__domain_create_info_setdefault(gc, &dm_config->c_info,
+                                               &physinfo);
     if (ret) goto out;
     ret = libxl__domain_build_info_setdefault(gc, &dm_config->b_info);
     if (ret) goto out;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 0185b8ff01..6a614658c2 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1447,7 +1447,8 @@ _hidden int libxl__domain_config_setdefault(libxl__gc *gc,
                                             libxl_domain_config *d_config,
                                             uint32_t domid /* logging only */);
 _hidden int libxl__domain_create_info_setdefault(libxl__gc *gc,
-                                        libxl_domain_create_info *c_info);
+                                        libxl_domain_create_info *c_info,
+                                        const libxl_physinfo *info);
 _hidden int libxl__domain_build_info_setdefault(libxl__gc *gc,
                                         libxl_domain_build_info *b_info);
 _hidden void libxl__rdm_setdefault(libxl__gc *gc,
-- 
2.11.0


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

Re: [Xen-devel] [XEN PATCH for-4.13 v2 8/9] libxl: create: setdefault: Move physinfo into config_setdefault

[Wei Liu]

On Thu, Oct 10, 2019 at 04:11:10PM +0100, Ian Jackson wrote:
> No functional change.  This will let us refer to it in code we are
> about to add to this function.
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>

Acked-by: Wei Liu <wl@xen.org>

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

[Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic

[Ian Jackson]

LIBXL_PASSTHROUGH_UNKNOWN (aka "ENABLED" in an earlier uncommitted
version of this code) is doing double duty.  We actually need all of
the following to be specificable:
  * default ("unknown"): enable PT iff we have devices to
    pass through specified in the initial config file.
  * "enabled" (and fail if the platform doesn't support it).
  * "disabled" (and reject future PT hotplug).
  * "share_pt"/"sync_pt": enable PT and set a specific PT mode.

Defaulting and error checking should be done in libxl.  So, we make
several changes here.

We introduce "enabled".  (And we document "unknown".)

We move all of the error checking and defaulting code from xl into
libxl.  Now, libxl__domain_config_setdefault has all of the necessary
information to get this right.  So we can do it all there, in one
place.

We can also arrange to have only one place each which calculates
(i) whether passthrough needs to be enabled because pt devices were
specified (ii) whether pt_share can be used.

xl now only has to parse the enum in the same way as it parses all
other enums.

This change fixes a regression from earlier 4.13-pre: until recent
changes, passthrough was only enabled by default if passthrough
devices were specified.  We restore this behaviour.

This will hide, from the point of view of libvirt tests in osstest, a
separate hypervisor regression which prevents migration of a domain
with passthrough enabled but without actual PT devices.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
v2: New patch in this version of the series.
---
 tools/libxl/libxl_create.c  | 57 ++++++++++++++++++++++++++++++--------
 tools/libxl/libxl_types.idl |  5 ++--
 tools/xl/xl_parse.c         | 67 ++++-----------------------------------------
 3 files changed, 53 insertions(+), 76 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 69971c97b6..fccb6a6271 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -57,18 +57,6 @@ int libxl__domain_create_info_setdefault(libxl__gc *gc,
     if (!c_info->ssidref)
         c_info->ssidref = SECINITSID_DOMU;
 
-    if (info->cap_hvm_directio &&
-        (c_info->passthrough == LIBXL_PASSTHROUGH_UNKNOWN)) {
-        c_info->passthrough = ((c_info->type == LIBXL_DOMAIN_TYPE_PV) ||
-                               !info->cap_iommu_hap_pt_share) ?
-            LIBXL_PASSTHROUGH_SYNC_PT : LIBXL_PASSTHROUGH_SHARE_PT;
-    } else if (!info->cap_hvm_directio) {
-        c_info->passthrough = LIBXL_PASSTHROUGH_DISABLED;
-    }
-
-    /* An explicit setting should now have been chosen */
-    assert(c_info->passthrough != LIBXL_PASSTHROUGH_UNKNOWN);
-
     return 0;
 }
 
@@ -904,6 +892,7 @@ int libxl__domain_config_setdefault(libxl__gc *gc,
     libxl_ctx *ctx = libxl__gc_owner(gc);
     int ret;
     bool pod_enabled = false;
+    libxl_domain_create_info *c_info = &d_config->c_info;
 
     libxl_physinfo physinfo;
     ret = libxl_get_physinfo(CTX, &physinfo);
@@ -968,6 +957,50 @@ int libxl__domain_config_setdefault(libxl__gc *gc,
         goto error_out;
     }
 
+    bool need_pt = d_config->num_pcidevs || d_config->num_dtdevs;
+    if (c_info->passthrough == LIBXL_PASSTHROUGH_UNKNOWN) {
+        c_info->passthrough = need_pt
+            ? LIBXL_PASSTHROUGH_ENABLED : LIBXL_PASSTHROUGH_DISABLED;
+    }
+
+    bool iommu_enabled = physinfo.cap_hvm_directio;
+    if (c_info->passthrough != LIBXL_PASSTHROUGH_DISABLED && !iommu_enabled) {
+        LOGD(ERROR, domid,
+             "ERROR: passthrough not supported on this platform\n");
+        ret = ERROR_INVAL;
+        goto error_out;
+    }
+
+    if (c_info->passthrough == LIBXL_PASSTHROUGH_DISABLED && need_pt) {
+        LOGD(ERROR, domid,
+             "passthrough disabled but devices are specified");
+        ret = ERROR_INVAL;
+        goto error_out;
+    }
+
+    const char *whynot_pt_share =
+        c_info->type == LIBXL_DOMAIN_TYPE_PV ? "not valid for PV domain" :
+        !physinfo.cap_iommu_hap_pt_share ? "not supported on this platform" :
+        NULL;
+
+    if (c_info->passthrough == LIBXL_PASSTHROUGH_ENABLED) {
+        assert(iommu_enabled);
+        c_info->passthrough = whynot_pt_share
+            ? LIBXL_PASSTHROUGH_SYNC_PT : LIBXL_PASSTHROUGH_SHARE_PT;
+    }
+
+    if (c_info->passthrough == LIBXL_PASSTHROUGH_SHARE_PT && whynot_pt_share) {
+        LOGD(ERROR, domid,
+             "ERROR: passthrough=\"share_pt\" %s\n",
+             whynot_pt_share);
+        ret = ERROR_INVAL;
+        goto error_out;
+    }
+
+    /* An explicit setting should now have been chosen */
+    assert(c_info->passthrough != LIBXL_PASSTHROUGH_UNKNOWN);
+    assert(c_info->passthrough != LIBXL_PASSTHROUGH_ENABLED);
+
     /* If target_memkb is smaller than max_memkb, the subsequent call
      * to libxc when building HVM domain will enable PoD mode.
      */
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 3ac9494b80..2441c0c233 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -266,8 +266,9 @@ libxl_vkb_backend = Enumeration("vkb_backend", [
 libxl_passthrough = Enumeration("passthrough", [
     (0, "unknown"),
     (1, "disabled"),
-    (2, "sync_pt"),
-    (3, "share_pt"),
+    (2, "enabled"),
+    (3, "sync_pt"),
+    (4, "share_pt"),
     ])
 
 #
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 79871c22d0..112f8ee026 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1222,7 +1222,6 @@ void parse_config_data(const char *config_source,
     int pci_seize = 0;
     int i, e;
     char *kernel_basename;
-    bool iommu_enabled, iommu_hap_pt_share;
 
     libxl_domain_create_info *c_info = &d_config->c_info;
     libxl_domain_build_info *b_info = &d_config->b_info;
@@ -1234,8 +1233,6 @@ void parse_config_data(const char *config_source,
         exit(EXIT_FAILURE);
     }
 
-    iommu_enabled = physinfo.cap_hvm_directio;
-    iommu_hap_pt_share = physinfo.cap_iommu_hap_pt_share;
     libxl_physinfo_dispose(&physinfo);
 
     config= xlu_cfg_init(stderr, config_source);
@@ -1509,67 +1506,13 @@ void parse_config_data(const char *config_source,
         }
     }
 
-    if (xlu_cfg_get_string(config, "passthrough", &buf, 0)) {
-        c_info->passthrough =
-            (d_config->num_pcidevs || d_config->num_dtdevs)
-            ? LIBXL_PASSTHROUGH_UNKNOWN : LIBXL_PASSTHROUGH_DISABLED;
-    } else {
-        if (!strcasecmp("enabled", buf))
-            c_info->passthrough = LIBXL_PASSTHROUGH_UNKNOWN;
-        else {
-            libxl_passthrough o;
-
-            e = libxl_passthrough_from_string(buf, &o);
-            if (e || !strcasecmp("unknown", buf)) {
-                fprintf(stderr,
-                        "ERROR: unknown passthrough option '%s'\n",
-                        buf);
-                exit(-ERROR_FAIL);
-            }
-
-            c_info->passthrough = o;
-        }
-    }
-
-    switch (c_info->passthrough) {
-    case LIBXL_PASSTHROUGH_UNKNOWN:
-        /*
-         * Choose a suitable default. libxl would also do this but
-         * choosing here allows the code calculating 'iommu_memkb'
-         * below make an informed decision.
-         */
-        c_info->passthrough =
-            (c_info->type == LIBXL_DOMAIN_TYPE_PV) || !iommu_hap_pt_share
-            ? LIBXL_PASSTHROUGH_SYNC_PT : LIBXL_PASSTHROUGH_SHARE_PT;
-        break;
-
-    case LIBXL_PASSTHROUGH_DISABLED:
-        if (d_config->num_pcidevs || d_config->num_dtdevs) {
+    if (!xlu_cfg_get_string(config, "passthrough", &buf, 0)) {
+        if (libxl_passthrough_from_string(buf, &c_info->passthrough)) {
             fprintf(stderr,
-                    "ERROR: passthrough disabled but devices are specified\n");
-            exit(-ERROR_FAIL);
-        }
-        break;
-    case LIBXL_PASSTHROUGH_SHARE_PT:
-        if (c_info->type == LIBXL_DOMAIN_TYPE_PV) {
-            fprintf(stderr,
-                    "ERROR: passthrough=\"share_pt\" not valid for PV domain\n");
-            exit(-ERROR_FAIL);
-        } else if (!iommu_hap_pt_share) {
-            fprintf(stderr,
-                    "ERROR: passthrough=\"share_pt\" not supported on this platform\n");
-            exit(-ERROR_FAIL);
+                    "ERROR: unknown passthrough option '%s'\n",
+                    buf);
+            exit(1);
         }
-        break;
-    case LIBXL_PASSTHROUGH_SYNC_PT:
-        break;
-    }
-
-    if ((c_info->passthrough != LIBXL_PASSTHROUGH_DISABLED) &&
-        !iommu_enabled) {
-        fprintf(stderr,
-                "ERROR: passthrough not supported on this platform\n");
-        exit(-ERROR_FAIL);
     }
 
     if (!xlu_cfg_get_long(config, "shadow_memory", &l, 0))
-- 
2.11.0


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

Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic

[Wei Liu]

On Thu, Oct 10, 2019 at 04:11:11PM +0100, Ian Jackson wrote:
> LIBXL_PASSTHROUGH_UNKNOWN (aka "ENABLED" in an earlier uncommitted
> version of this code) is doing double duty.  We actually need all of
> the following to be specificable:
>   * default ("unknown"): enable PT iff we have devices to
>     pass through specified in the initial config file.
>   * "enabled" (and fail if the platform doesn't support it).
>   * "disabled" (and reject future PT hotplug).
>   * "share_pt"/"sync_pt": enable PT and set a specific PT mode.
> 
> Defaulting and error checking should be done in libxl.  So, we make
> several changes here.
> 
> We introduce "enabled".  (And we document "unknown".)
> 
> We move all of the error checking and defaulting code from xl into
> libxl.  Now, libxl__domain_config_setdefault has all of the necessary
> information to get this right.  So we can do it all there, in one
> place.
> 
> We can also arrange to have only one place each which calculates
> (i) whether passthrough needs to be enabled because pt devices were
> specified (ii) whether pt_share can be used.
> 
> xl now only has to parse the enum in the same way as it parses all
> other enums.
> 
> This change fixes a regression from earlier 4.13-pre: until recent
> changes, passthrough was only enabled by default if passthrough
> devices were specified.  We restore this behaviour.
> 
> This will hide, from the point of view of libvirt tests in osstest, a
> separate hypervisor regression which prevents migration of a domain
> with passthrough enabled but without actual PT devices.

I think Jan committed a patch to fix that, so this may need deleting.

I will let Jan and Paul confirm this.

Wei.

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

Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic

[Andrew Cooper]

On 10/10/2019 16:11, Ian Jackson wrote:
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 69971c97b6..fccb6a6271 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -968,6 +957,50 @@ int libxl__domain_config_setdefault(libxl__gc *gc,
>          goto error_out;
>      }
>  
> +    bool need_pt = d_config->num_pcidevs || d_config->num_dtdevs;
> +    if (c_info->passthrough == LIBXL_PASSTHROUGH_UNKNOWN) {
> +        c_info->passthrough = need_pt
> +            ? LIBXL_PASSTHROUGH_ENABLED : LIBXL_PASSTHROUGH_DISABLED;
> +    }
> +
> +    bool iommu_enabled = physinfo.cap_hvm_directio;
> +    if (c_info->passthrough != LIBXL_PASSTHROUGH_DISABLED && !iommu_enabled) {
> +        LOGD(ERROR, domid,
> +             "ERROR: passthrough not supported on this platform\n");
> +        ret = ERROR_INVAL;
> +        goto error_out;
> +    }
> +
> +    if (c_info->passthrough == LIBXL_PASSTHROUGH_DISABLED && need_pt) {
> +        LOGD(ERROR, domid,
> +             "passthrough disabled but devices are specified");

This is the only log message which isn't prefixed with ERROR:

> +        ret = ERROR_INVAL;
> +        goto error_out;
> +    }
> +
> +    const char *whynot_pt_share =
> +        c_info->type == LIBXL_DOMAIN_TYPE_PV ? "not valid for PV domain" :
> +        !physinfo.cap_iommu_hap_pt_share ? "not supported on this platform" :
> +        NULL;

This is a little more complicated.

For ARM, doesn't libxl treat guests as PV, or has that been fixed now? 
ARM's only passthrough mode is PT_SHARE.

On x86 for PVH, passthrough doesn't work yet.  This may not be an
argument against constructing the guest suitably, but we should check
that things don't explode in new and interesting ways from this change.

For x86 HVM, PT_SHARE is only available for HAP guests, so shadow guests
must use PT_SYNC.


> +
> +    if (c_info->passthrough == LIBXL_PASSTHROUGH_ENABLED) {
> +        assert(iommu_enabled);
> +        c_info->passthrough = whynot_pt_share
> +            ? LIBXL_PASSTHROUGH_SYNC_PT : LIBXL_PASSTHROUGH_SHARE_PT;
> +    }
> +
> +    if (c_info->passthrough == LIBXL_PASSTHROUGH_SHARE_PT && whynot_pt_share) {
> +        LOGD(ERROR, domid,
> +             "ERROR: passthrough=\"share_pt\" %s\n",
> +             whynot_pt_share);
> +        ret = ERROR_INVAL;
> +        goto error_out;
> +    }
> +
> +    /* An explicit setting should now have been chosen */
> +    assert(c_info->passthrough != LIBXL_PASSTHROUGH_UNKNOWN);
> +    assert(c_info->passthrough != LIBXL_PASSTHROUGH_ENABLED);

This is confusing.  I think it would help if ...

> +
>      /* If target_memkb is smaller than max_memkb, the subsequent call
>       * to libxc when building HVM domain will enable PoD mode.
>       */
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 3ac9494b80..2441c0c233 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -266,8 +266,9 @@ libxl_vkb_backend = Enumeration("vkb_backend", [
>  libxl_passthrough = Enumeration("passthrough", [
>      (0, "unknown"),
>      (1, "disabled"),
> -    (2, "sync_pt"),
> -    (3, "share_pt"),
> +    (2, "enabled"),
> +    (3, "sync_pt"),
> +    (4, "share_pt"),

... this had a comment explaining enabled is just interim value.

(2, "enabled"), # becomes {sync,share}_pt once defaults are evaluated

~Andrew

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

Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic

[Ian Jackson]

Andrew Cooper writes ("Re: [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic"):
> On 10/10/2019 16:11, Ian Jackson wrote:
> > +    if (c_info->passthrough == LIBXL_PASSTHROUGH_DISABLED && need_pt) {
> > +        LOGD(ERROR, domid,
> > +             "passthrough disabled but devices are specified");
> 
> This is the only log message which isn't prefixed with ERROR:

I will strip the ERROR: out of the others.  I think I inherited them
from when this code was in xl, where it's just fprintf stderr.  Here
the priority is an argument to LOGD.

> > +    const char *whynot_pt_share =
> > +        c_info->type == LIBXL_DOMAIN_TYPE_PV ? "not valid for PV domain" :
> > +        !physinfo.cap_iommu_hap_pt_share ? "not supported on this platform" :
> > +        NULL;
> 
> This is a little more complicated.

I aimed to replicate the logic prior to my series.  FTAOD I think this
means this was already broken in xl ?  Anyway:

> For ARM, doesn't libxl treat guests as PV, or has that been fixed now? 
> ARM's only passthrough mode is PT_SHARE.

I think this means that I need to move the calculation of
whynot_pt_share into arch-specific code.  I'll wait and see what ARM
folks say.

> On x86 for PVH, passthrough doesn't work yet.  This may not be an
> argument against constructing the guest suitably, but we should check
> that things don't explode in new and interesting ways from this change.

If we know it doesn't work, it's not a good idea to accept it.  I will
arrange to reject it.

> For x86 HVM, PT_SHARE is only available for HAP guests, so shadow guests
> must use PT_SYNC.

I will add a check for c_info->hap.

> > +    /* An explicit setting should now have been chosen */
> > +    assert(c_info->passthrough != LIBXL_PASSTHROUGH_UNKNOWN);
> > +    assert(c_info->passthrough != LIBXL_PASSTHROUGH_ENABLED);
> 
> This is confusing.  I think it would help if ...
...
> ... this had a comment explaining enabled is just interim value.
> 
> (2, "enabled"), # becomes {sync,share}_pt once defaults are evaluated

Good idea, thanks.

Ian.

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

Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic

[Julien Grall]

Hi,

On 11/10/2019 10:47, Andrew Cooper wrote:
> On 10/10/2019 16:11, Ian Jackson wrote:
>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
>> index 69971c97b6..fccb6a6271 100644
>> --- a/tools/libxl/libxl_create.c
>> +++ b/tools/libxl/libxl_create.c
>> @@ -968,6 +957,50 @@ int libxl__domain_config_setdefault(libxl__gc *gc,
>>           goto error_out;
>>       }
>>   
>> +    bool need_pt = d_config->num_pcidevs || d_config->num_dtdevs;
>> +    if (c_info->passthrough == LIBXL_PASSTHROUGH_UNKNOWN) {
>> +        c_info->passthrough = need_pt
>> +            ? LIBXL_PASSTHROUGH_ENABLED : LIBXL_PASSTHROUGH_DISABLED;
>> +    }
>> +
>> +    bool iommu_enabled = physinfo.cap_hvm_directio;
>> +    if (c_info->passthrough != LIBXL_PASSTHROUGH_DISABLED && !iommu_enabled) {
>> +        LOGD(ERROR, domid,
>> +             "ERROR: passthrough not supported on this platform\n");
>> +        ret = ERROR_INVAL;
>> +        goto error_out;
>> +    }
>> +
>> +    if (c_info->passthrough == LIBXL_PASSTHROUGH_DISABLED && need_pt) {
>> +        LOGD(ERROR, domid,
>> +             "passthrough disabled but devices are specified");
> 
> This is the only log message which isn't prefixed with ERROR:
> 
>> +        ret = ERROR_INVAL;
>> +        goto error_out;
>> +    }
>> +
>> +    const char *whynot_pt_share =
>> +        c_info->type == LIBXL_DOMAIN_TYPE_PV ? "not valid for PV domain" :
>> +        !physinfo.cap_iommu_hap_pt_share ? "not supported on this platform" :
>> +        NULL;
> 
> This is a little more complicated.
> 
> For ARM, doesn't libxl treat guests as PV, or has that been fixed now?

libxl treats Arm guest as PVH now. Note that we seamlessly convert PV to PVH in 
libxl__arch_domain_{build, create}_info_setdefault().

So as long as this is called after any of those calls, then we should be fine.

> ARM's only passthrough mode is PT_SHARE.

Correct.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic

[Ian Jackson]

Julien Grall writes ("Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic"):
> libxl treats Arm guest as PVH now. Note that we seamlessly convert
> PV to PVH in libxl__arch_domain_{build, create}_info_setdefault().
> 
> So as long as this is called after any of those calls, then we
> should be fine.

I'll check this.

This means that the algorithm is quite different on ARM to on x86:
ARM guests are always PVH and need PT_SHARE; on x86, PVH is not
compatible with passthrough at all.

So I need to put this logic in an arch-specific function.

Ian.

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

Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic

[George Dunlap]

On Thu, Oct 10, 2019 at 4:12 PM Ian Jackson <ian.jackson@eu.citrix.com> wrote:
>
> LIBXL_PASSTHROUGH_UNKNOWN (aka "ENABLED" in an earlier uncommitted
> version of this code) is doing double duty.  We actually need all of
> the following to be specificable:
>   * default ("unknown"): enable PT iff we have devices to
>     pass through specified in the initial config file.

I realize this may be a bit late, but I find "unknown" to be a very
strange word to use to indicate, "please choose the best option for
me".  For USB device type I used "auto", meaning, "automatically
choose the best option for me".  Paul didn't like "auto", which is
fair enough, but I really don't see how "unknown" is better.

Anyway, not meaning to block, just a suggestion.

 -George

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

Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic

[Ian Jackson]

George Dunlap writes ("Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic"):
> On Thu, Oct 10, 2019 at 4:12 PM Ian Jackson <ian.jackson@eu.citrix.com> wrote:
> > LIBXL_PASSTHROUGH_UNKNOWN (aka "ENABLED" in an earlier uncommitted
> > version of this code) is doing double duty.  We actually need all of
> > the following to be specificable:
> >   * default ("unknown"): enable PT iff we have devices to
> >     pass through specified in the initial config file.
> 
> I realize this may be a bit late, but I find "unknown" to be a very
> strange word to use to indicate, "please choose the best option for
> me".  For USB device type I used "auto", meaning, "automatically
> choose the best option for me".  Paul didn't like "auto", which is
> fair enough, but I really don't see how "unknown" is better.
> 
> Anyway, not meaning to block, just a suggestion.

I do not have a strong opinion about this.  I would be happy with
"auto" (or "default" maybe).

None of this was in 4.12 so we are still free to change it.  Now would
be the time!

Ian.

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

Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic

[Jürgen Groß]

On 11.10.19 15:31, Ian Jackson wrote:
> George Dunlap writes ("Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic"):
>> On Thu, Oct 10, 2019 at 4:12 PM Ian Jackson <ian.jackson@eu.citrix.com> wrote:
>>> LIBXL_PASSTHROUGH_UNKNOWN (aka "ENABLED" in an earlier uncommitted
>>> version of this code) is doing double duty.  We actually need all of
>>> the following to be specificable:
>>>    * default ("unknown"): enable PT iff we have devices to
>>>      pass through specified in the initial config file.
>>
>> I realize this may be a bit late, but I find "unknown" to be a very
>> strange word to use to indicate, "please choose the best option for
>> me".  For USB device type I used "auto", meaning, "automatically
>> choose the best option for me".  Paul didn't like "auto", which is
>> fair enough, but I really don't see how "unknown" is better.
>>
>> Anyway, not meaning to block, just a suggestion.
> 
> I do not have a strong opinion about this.  I would be happy with
> "auto" (or "default" maybe).

"unspecified"?


Juergen

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

Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic

[Ian Jackson]

Jürgen Groß writes ("Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic"):
> On 11.10.19 15:31, Ian Jackson wrote:
> > I do not have a strong opinion about this.  I would be happy with
> > "auto" (or "default" maybe).
> 
> "unspecified"?

That is IMO the best suggestion so far so I will go with that in my
v3.

Ian.

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

Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic

[Paul Durrant]

On Fri, 11 Oct 2019 at 17:34, Ian Jackson <ian.jackson@citrix.com> wrote:
>
> Jürgen Groß writes ("Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic"):
> > On 11.10.19 15:31, Ian Jackson wrote:
> > > I do not have a strong opinion about this.  I would be happy with
> > > "auto" (or "default" maybe).
> >
> > "unspecified"?
>
> That is IMO the best suggestion so far so I will go with that in my
> v3.

Seems odd to specify a parameter with a value of 'unspecified' ;-)

  Paul

>
> Ian.

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

Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic

[Ian Jackson]

Paul Durrant writes ("Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic"):
> On Fri, 11 Oct 2019 at 17:34, Ian Jackson <ian.jackson@citrix.com> wrote:
> >
> > Jürgen Groß writes ("Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic"):
> > > On 11.10.19 15:31, Ian Jackson wrote:
> > > > I do not have a strong opinion about this.  I would be happy with
> > > > "auto" (or "default" maybe).
> > >
> > > "unspecified"?
> >
> > That is IMO the best suggestion so far so I will go with that in my
> > v3.
> 
> Seems odd to specify a parameter with a value of 'unspecified' ;-)

I have tried to infer +1/-1/0 numbers from the mail thread.  I have
also looked at libxl_types.idl to see how many times we are using
what name to represent roughly this concept:

 Bikeshed colour  Paul Juergen George Ian Anthony Wei #already

 unknown           ?      ?     -1    +2    ?     ?    17
 default           ?      ?     ?      0    ?     ?     2
 auto              -1     ?     +1     0    ?     ?     1
 unspecified       -1     +1    ?      0    ?     ?     0

                                      ^^^^^^^^^^^^^^^^
                                      libxl maintainers

On this basis IMO clearly this should go back to "unknown".
I will do that in a respin (or maybe on commit) but right now I think
I am still awaiting a review for this patch.

Thanks,
Ian.

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

Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic

[Wei Liu]

On Mon, Oct 14, 2019 at 05:09:24PM +0100, Ian Jackson wrote:
> Paul Durrant writes ("Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic"):
> > On Fri, 11 Oct 2019 at 17:34, Ian Jackson <ian.jackson@citrix.com> wrote:
> > >
> > > Jürgen Groß writes ("Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic"):
> > > > On 11.10.19 15:31, Ian Jackson wrote:
> > > > > I do not have a strong opinion about this.  I would be happy with
> > > > > "auto" (or "default" maybe).
> > > >
> > > > "unspecified"?
> > >
> > > That is IMO the best suggestion so far so I will go with that in my
> > > v3.
> > 
> > Seems odd to specify a parameter with a value of 'unspecified' ;-)
> 
> I have tried to infer +1/-1/0 numbers from the mail thread.  I have
> also looked at libxl_types.idl to see how many times we are using
> what name to represent roughly this concept:
> 
>  Bikeshed colour  Paul Juergen George Ian Anthony Wei #already
> 
>  unknown           ?      ?     -1    +2    ?     ?    17
>  default           ?      ?     ?      0    ?     ?     2
>  auto              -1     ?     +1     0    ?     ?     1
>  unspecified       -1     +1    ?      0    ?     ?     0
> 
>                                       ^^^^^^^^^^^^^^^^
>                                       libxl maintainers

+1 to "unknown". I prefer consistency.

0 to all others.


> 
> On this basis IMO clearly this should go back to "unknown".
> I will do that in a respin (or maybe on commit) but right now I think
> I am still awaiting a review for this patch.
> 

I think a respin is required -- in one of the mails your said you would
need to put some logic into arch-specific function.

Wei.

> Thanks,
> Ian.

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

Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic

[Ian Jackson]

Wei Liu writes ("Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic"):
> On Mon, Oct 14, 2019 at 05:09:24PM +0100, Ian Jackson wrote:
> >  Bikeshed colour  Paul Juergen George Ian Anthony Wei #already
> > 
> >  unknown           ?      ?     -1    +2    ?     ?    17
> >  default           ?      ?     ?      0    ?     ?     2
> >  auto              -1     ?     +1     0    ?     ?     1
> >  unspecified       -1     +1    ?      0    ?     ?     0
> > 
> >                                       ^^^^^^^^^^^^^^^^
> >                                       libxl maintainers
> 
> +1 to "unknown". I prefer consistency.
> 
> 0 to all others.

Thanks.

> > On this basis IMO clearly this should go back to "unknown".
> > I will do that in a respin (or maybe on commit) but right now I think
> > I am still awaiting a review for this patch.
> 
> I think a respin is required -- in one of the mails your said you would
> need to put some logic into arch-specific function.

Yes.  Already done.

This is confusing because we are in the thread re v2, which is where
this bikeshed conversation is happening.

But there is also a v3, see:

  Subject: Re: [Xen-devel] [XEN PATCH for-4.13 v3 10/10] libxl/xl: Overhaul
           passthrough setting logic

That's been R-B Anthony but I have now changed "unspecified" back to
"unknown"...

Ian.

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

[Xen-devel] [XEN PATCH v4 for-4.13 10/10] libxl/xl: Overhaul passthrough setting logic passthrough setting logic

[Ian Jackson]

Here's what I hope is the final version...

From b60195e857d3699eaa55d9174a69bf91902cddb5 Mon Sep 17 00:00:00 2001
From: Ian Jackson <ian.jackson@eu.citrix.com>
Date: Mon, 7 Oct 2019 17:59:15 +0100
Subject: [XEN PATCH v4 for-4.13 10/10] libxl/xl: Overhaul passthrough setting
 logic

LIBXL_PASSTHROUGH_UNKNOWN (aka "ENABLED" in an earlier uncommitted
version of this code) is doing double duty.  We actually need all of
the following to be specificable:
  * default ("unknown"): enable PT iff we have devices to
    pass through specified in the initial config file.
  * "enabled" (and fail if the platform doesn't support it).
  * "disabled" (and reject future PT hotplug).
  * "share_pt"/"sync_pt": enable PT and set a specific PT mode.

Defaulting and error checking should be done in libxl.  So, we make
several changes here.

We introduce "enabled".  (And we document "unknown".)

We move all of the error checking and defaulting code from xl into
libxl.  Now, libxl__domain_config_setdefault has all of the necessary
information to get this right.  So we can do it all there.  Choosing
the specific mode is arch-specific.

We can also arrange to have only one place each which calculates
(i) whether passthrough needs to be enabled because pt devices were
specified (ii) whether pt_share can be used (for each arch).

xl now only has to parse the enum in the same way as it parses all
other enums.

This change fixes a regression from earlier 4.13-pre: until recent
changes, passthrough was only enabled by default if passthrough
devices were specified.  We restore this behaviour.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Andrew Cooper <Andrew.Cooper3@citrix.com>
CC: Paul Durrant <pdurrant@gmail.com>
CC: Jan Beulich <jbeulich@suse.com>

---
v4: Fix trailing whitespace
    No longer change "unknown" to "unspecified".

v3: Drop paragraph about masking another osstest regression,
     as that's now fixed.
    Drop redundant "ERROR:" in two log messages.
    Add a comment about the way "enabled" gets changed to a specific value.
    Split passthrough mode defaulting into arch specific functions.
    On ARM, always choose (and insist on) share_pt.
    Reject share_pt for non-HAP guests.
    Reject passthrough for PVH guests.
    Actually document "unspecified" option in xl.cfg(5)
    Rename "unknown" to "unspecified"

v2: New patch in this version of the series.
---
 docs/man/xl.cfg.5.pod.in    |  6 ++++
 tools/libxl/libxl_arch.h    |  6 ++++
 tools/libxl/libxl_arm.c     | 24 ++++++++++++++++
 tools/libxl/libxl_create.c  | 41 +++++++++++++++++++--------
 tools/libxl/libxl_types.idl |  5 ++--
 tools/libxl/libxl_x86.c     | 41 +++++++++++++++++++++++++++
 tools/xl/xl_parse.c         | 67 ++++-----------------------------------------
 7 files changed, 114 insertions(+), 76 deletions(-)

diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index 64bed30bce..7e220d0c20 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -660,6 +660,12 @@ in preference. However, the availability of this option is hardware
 specific. If B<xl info> reports B<virt_caps> containing
 B<iommu_hap_pt_share> then this option may be used.
 
+=item B<unspecified>
+
+The default, which chooses between B<disabled> and B<enabled>
+according to whether passthrough devices are enabled in the config
+file.
+
 =back
 
 =back
diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
index d624159e53..ee6641b3e6 100644
--- a/tools/libxl/libxl_arch.h
+++ b/tools/libxl/libxl_arch.h
@@ -73,6 +73,12 @@ void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
                                               libxl_domain_build_info *b_info);
 
 _hidden
+int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
+                                            uint32_t domid,
+                                            libxl_domain_config *d_config,
+                                            const libxl_physinfo *physinfo);
+
+_hidden
 int libxl__arch_extra_memory(libxl__gc *gc,
                              const libxl_domain_build_info *info,
                              uint64_t *out);
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index bf31b9b3ca..2f1ca69431 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -1191,6 +1191,30 @@ void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
     libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_PVH);
 }
 
+int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
+                                            uint32_t domid,
+                                            libxl_domain_config *d_config,
+                                            const libxl_physinfo *physinfo)
+{
+    int rc;
+    libxl_domain_create_info *const c_info = &d_config->c_info;
+
+    if (c_info->passthrough == LIBXL_PASSTHROUGH_ENABLED) {
+        c_info->passthrough = LIBXL_PASSTHROUGH_SHARE_PT;
+    }
+
+    if (c_info->passthrough == LIBXL_PASSTHROUGH_SYNC_PT) {
+        LOGD(ERROR, domid,
+             "passthrough=\"sync_pt\" not supported on ARM\n");
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    rc = 0;
+ out:
+    return rc;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 3d2cb52bf7..7423bedf7d 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -57,18 +57,6 @@ int libxl__domain_create_info_setdefault(libxl__gc *gc,
     if (!c_info->ssidref)
         c_info->ssidref = SECINITSID_DOMU;
 
-    if (info->cap_hvm_directio &&
-        (c_info->passthrough == LIBXL_PASSTHROUGH_UNKNOWN)) {
-        c_info->passthrough = ((c_info->type == LIBXL_DOMAIN_TYPE_PV) ||
-                               !info->cap_iommu_hap_pt_share) ?
-            LIBXL_PASSTHROUGH_SYNC_PT : LIBXL_PASSTHROUGH_SHARE_PT;
-    } else if (!info->cap_hvm_directio) {
-        c_info->passthrough = LIBXL_PASSTHROUGH_DISABLED;
-    }
-
-    /* An explicit setting should now have been chosen */
-    assert(c_info->passthrough != LIBXL_PASSTHROUGH_UNKNOWN);
-
     return 0;
 }
 
@@ -904,6 +892,7 @@ int libxl__domain_config_setdefault(libxl__gc *gc,
     libxl_ctx *ctx = libxl__gc_owner(gc);
     int ret;
     bool pod_enabled = false;
+    libxl_domain_create_info *c_info = &d_config->c_info;
 
     libxl_physinfo physinfo;
     ret = libxl_get_physinfo(CTX, &physinfo);
@@ -975,6 +964,34 @@ int libxl__domain_config_setdefault(libxl__gc *gc,
         goto error_out;
     }
 
+    bool need_pt = d_config->num_pcidevs || d_config->num_dtdevs;
+    if (c_info->passthrough == LIBXL_PASSTHROUGH_UNKNOWN) {
+        c_info->passthrough = need_pt
+            ? LIBXL_PASSTHROUGH_ENABLED : LIBXL_PASSTHROUGH_DISABLED;
+    }
+
+    bool iommu_enabled = physinfo.cap_hvm_directio;
+    if (c_info->passthrough != LIBXL_PASSTHROUGH_DISABLED && !iommu_enabled) {
+        LOGD(ERROR, domid,
+             "passthrough not supported on this platform\n");
+        ret = ERROR_INVAL;
+        goto error_out;
+    }
+
+    if (c_info->passthrough == LIBXL_PASSTHROUGH_DISABLED && need_pt) {
+        LOGD(ERROR, domid,
+             "passthrough disabled but devices are specified");
+        ret = ERROR_INVAL;
+        goto error_out;
+    }
+
+    ret = libxl__arch_passthrough_mode_setdefault(gc,domid,d_config,&physinfo);
+    if (ret) goto error_out;
+
+    /* An explicit setting should now have been chosen */
+    assert(c_info->passthrough != LIBXL_PASSTHROUGH_UNKNOWN);
+    assert(c_info->passthrough != LIBXL_PASSTHROUGH_ENABLED);
+
     /* If target_memkb is smaller than max_memkb, the subsequent call
      * to libxc when building HVM domain will enable PoD mode.
      */
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 3ac9494b80..ca47db2961 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -266,8 +266,9 @@ libxl_vkb_backend = Enumeration("vkb_backend", [
 libxl_passthrough = Enumeration("passthrough", [
     (0, "unknown"),
     (1, "disabled"),
-    (2, "sync_pt"),
-    (3, "share_pt"),
+    (2, "enabled"), # becomes {sync,share}_pt once defaults are evaluated
+    (3, "sync_pt"),
+    (4, "share_pt"),
     ])
 
 #
diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index c0f88a7eaa..8b804537ba 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -631,6 +631,47 @@ void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
     libxl_defbool_setdefault(&b_info->acpi, true);
 }
 
+int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
+                                            uint32_t domid,
+                                            libxl_domain_config *d_config,
+                                            const libxl_physinfo *physinfo)
+{
+    int rc;
+    libxl_domain_create_info *const c_info = &d_config->c_info;
+
+    if (c_info->passthrough != LIBXL_PASSTHROUGH_DISABLED &&
+        c_info->type == LIBXL_DOMAIN_TYPE_PVH) {
+        LOGD(ERROR, domid,
+             "passthrough not yet supported for x86 PVH guests\n");
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    const char *whynot_pt_share =
+        c_info->type == LIBXL_DOMAIN_TYPE_PV ? "not valid for PV domain" :
+        !physinfo->cap_iommu_hap_pt_share ? "not supported on this platform" :
+        !libxl_defbool_val(d_config->c_info.hap) ?"only valid for HAP guests":
+        NULL;
+
+    if (c_info->passthrough == LIBXL_PASSTHROUGH_ENABLED) {
+        c_info->passthrough = whynot_pt_share
+            ? LIBXL_PASSTHROUGH_SYNC_PT : LIBXL_PASSTHROUGH_SHARE_PT;
+    }
+
+    if (c_info->passthrough == LIBXL_PASSTHROUGH_SHARE_PT && whynot_pt_share) {
+        LOGD(ERROR, domid,
+             "passthrough=\"share_pt\" %s\n",
+             whynot_pt_share);
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    rc = 0;
+ out:
+    return rc;
+}
+
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 79871c22d0..112f8ee026 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1222,7 +1222,6 @@ void parse_config_data(const char *config_source,
     int pci_seize = 0;
     int i, e;
     char *kernel_basename;
-    bool iommu_enabled, iommu_hap_pt_share;
 
     libxl_domain_create_info *c_info = &d_config->c_info;
     libxl_domain_build_info *b_info = &d_config->b_info;
@@ -1234,8 +1233,6 @@ void parse_config_data(const char *config_source,
         exit(EXIT_FAILURE);
     }
 
-    iommu_enabled = physinfo.cap_hvm_directio;
-    iommu_hap_pt_share = physinfo.cap_iommu_hap_pt_share;
     libxl_physinfo_dispose(&physinfo);
 
     config= xlu_cfg_init(stderr, config_source);
@@ -1509,67 +1506,13 @@ void parse_config_data(const char *config_source,
         }
     }
 
-    if (xlu_cfg_get_string(config, "passthrough", &buf, 0)) {
-        c_info->passthrough =
-            (d_config->num_pcidevs || d_config->num_dtdevs)
-            ? LIBXL_PASSTHROUGH_UNKNOWN : LIBXL_PASSTHROUGH_DISABLED;
-    } else {
-        if (!strcasecmp("enabled", buf))
-            c_info->passthrough = LIBXL_PASSTHROUGH_UNKNOWN;
-        else {
-            libxl_passthrough o;
-
-            e = libxl_passthrough_from_string(buf, &o);
-            if (e || !strcasecmp("unknown", buf)) {
-                fprintf(stderr,
-                        "ERROR: unknown passthrough option '%s'\n",
-                        buf);
-                exit(-ERROR_FAIL);
-            }
-
-            c_info->passthrough = o;
-        }
-    }
-
-    switch (c_info->passthrough) {
-    case LIBXL_PASSTHROUGH_UNKNOWN:
-        /*
-         * Choose a suitable default. libxl would also do this but
-         * choosing here allows the code calculating 'iommu_memkb'
-         * below make an informed decision.
-         */
-        c_info->passthrough =
-            (c_info->type == LIBXL_DOMAIN_TYPE_PV) || !iommu_hap_pt_share
-            ? LIBXL_PASSTHROUGH_SYNC_PT : LIBXL_PASSTHROUGH_SHARE_PT;
-        break;
-
-    case LIBXL_PASSTHROUGH_DISABLED:
-        if (d_config->num_pcidevs || d_config->num_dtdevs) {
+    if (!xlu_cfg_get_string(config, "passthrough", &buf, 0)) {
+        if (libxl_passthrough_from_string(buf, &c_info->passthrough)) {
             fprintf(stderr,
-                    "ERROR: passthrough disabled but devices are specified\n");
-            exit(-ERROR_FAIL);
-        }
-        break;
-    case LIBXL_PASSTHROUGH_SHARE_PT:
-        if (c_info->type == LIBXL_DOMAIN_TYPE_PV) {
-            fprintf(stderr,
-                    "ERROR: passthrough=\"share_pt\" not valid for PV domain\n");
-            exit(-ERROR_FAIL);
-        } else if (!iommu_hap_pt_share) {
-            fprintf(stderr,
-                    "ERROR: passthrough=\"share_pt\" not supported on this platform\n");
-            exit(-ERROR_FAIL);
+                    "ERROR: unknown passthrough option '%s'\n",
+                    buf);
+            exit(1);
         }
-        break;
-    case LIBXL_PASSTHROUGH_SYNC_PT:
-        break;
-    }
-
-    if ((c_info->passthrough != LIBXL_PASSTHROUGH_DISABLED) &&
-        !iommu_enabled) {
-        fprintf(stderr,
-                "ERROR: passthrough not supported on this platform\n");
-        exit(-ERROR_FAIL);
     }
 
     if (!xlu_cfg_get_long(config, "shadow_memory", &l, 0))
-- 
2.11.0


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

Re: [Xen-devel] [XEN PATCH v4 for-4.13 10/10] libxl/xl: Overhaul passthrough setting logic passthrough setting logic

[Anthony PERARD]

On Mon, Oct 14, 2019 at 05:51:04PM +0100, Ian Jackson wrote:
> ---
> v4: Fix trailing whitespace
>     No longer change "unknown" to "unspecified".

> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> index 64bed30bce..7e220d0c20 100644
> --- a/docs/man/xl.cfg.5.pod.in
> +++ b/docs/man/xl.cfg.5.pod.in
> @@ -660,6 +660,12 @@ in preference. However, the availability of this option is hardware
>  specific. If B<xl info> reports B<virt_caps> containing
>  B<iommu_hap_pt_share> then this option may be used.
>  
> +=item B<unspecified>

Don't forget to change the man page as well ;-).

-- 
Anthony PERARD

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

Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic

[Anthony PERARD]

On Mon, Oct 14, 2019 at 05:09:24PM +0100, Ian Jackson wrote:
> Paul Durrant writes ("Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic"):
> > On Fri, 11 Oct 2019 at 17:34, Ian Jackson <ian.jackson@citrix.com> wrote:
> > >
> > > Jürgen Groß writes ("Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic"):
> > > > On 11.10.19 15:31, Ian Jackson wrote:
> > > > > I do not have a strong opinion about this.  I would be happy with
> > > > > "auto" (or "default" maybe).
> > > >
> > > > "unspecified"?
> > >
> > > That is IMO the best suggestion so far so I will go with that in my
> > > v3.
> > 
> > Seems odd to specify a parameter with a value of 'unspecified' ;-)
> 
> I have tried to infer +1/-1/0 numbers from the mail thread.  I have
> also looked at libxl_types.idl to see how many times we are using
> what name to represent roughly this concept:
> 
>  Bikeshed colour  Paul Juergen George Ian Anthony Wei #already
> 
>  unknown           ?      ?     -1    +2    ?     ?    17
>  default           ?      ?     ?      0    ?     ?     2
>  auto              -1     ?     +1     0    ?     ?     1
>  unspecified       -1     +1    ?      0    ?     ?     0
> 
>                                       ^^^^^^^^^^^^^^^^
>                                       libxl maintainers

Maybe "unknown" is more used in the API, but when I look at the manpage
"unknown" value as never been used before. On the other hand "default"
as been used twice in the man page. (and one "defaults" and two other
"default" that I'm not sure if they can be in the config file.)

So I would say +2 for default and +1 for unknown.

-- 
Anthony PERARD

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

Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic

[Wei Liu]

On Mon, Oct 14, 2019 at 05:59:26PM +0100, Anthony PERARD wrote:
> On Mon, Oct 14, 2019 at 05:09:24PM +0100, Ian Jackson wrote:
> > Paul Durrant writes ("Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic"):
> > > On Fri, 11 Oct 2019 at 17:34, Ian Jackson <ian.jackson@citrix.com> wrote:
> > > >
> > > > Jürgen Groß writes ("Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic"):
> > > > > On 11.10.19 15:31, Ian Jackson wrote:
> > > > > > I do not have a strong opinion about this.  I would be happy with
> > > > > > "auto" (or "default" maybe).
> > > > >
> > > > > "unspecified"?
> > > >
> > > > That is IMO the best suggestion so far so I will go with that in my
> > > > v3.
> > > 
> > > Seems odd to specify a parameter with a value of 'unspecified' ;-)
> > 
> > I have tried to infer +1/-1/0 numbers from the mail thread.  I have
> > also looked at libxl_types.idl to see how many times we are using
> > what name to represent roughly this concept:
> > 
> >  Bikeshed colour  Paul Juergen George Ian Anthony Wei #already
> > 
> >  unknown           ?      ?     -1    +2    ?     ?    17
> >  default           ?      ?     ?      0    ?     ?     2
> >  auto              -1     ?     +1     0    ?     ?     1
> >  unspecified       -1     +1    ?      0    ?     ?     0
> > 
> >                                       ^^^^^^^^^^^^^^^^
> >                                       libxl maintainers
> 
> Maybe "unknown" is more used in the API, but when I look at the manpage
> "unknown" value as never been used before. On the other hand "default"
> as been used twice in the man page. (and one "defaults" and two other
> "default" that I'm not sure if they can be in the config file.)
> 
> So I would say +2 for default and +1 for unknown.

Either is fine as long as it is clearly documented. If you feel strongly
about "default", so be it. :-)

Wei.

> 
> -- 
> Anthony PERARD

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

Re: [Xen-devel] [XEN PATCH for-4.13 v2 0/9] libxl memkb & pt defaulting

[Ian Jackson]

Ian Jackson writes ("[XEN PATCH for-4.13 v2 0/9] libxl memkb & pt defaulting"):
> This is v2 of my series to sort out the shadow/iommu memory and pci
> passthrough situation.  I think this series will mask the bug which is
> currently blocking staging propagating to master.

I have pushed this to:
   https://xenbits.xen.org/gitweb/?p=people/iwj/xen.git;a=summary
   wip.libxl-memkb-ptcfg.v2

Ian.

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