[PATCH 00/12] xen: support per-cpupool scheduling granularity

[PATCH 00/12] xen: support per-cpupool scheduling granularity

[Juergen Gross]

Support scheduling granularity per cpupool. Setting the granularity is
done via hypfs, which needed to gain dynamical entries for that
purpose.

Apart from the hypfs related additional functionality the main change
for cpupools was the support for moving a domain to a new granularity,
as this requires to modify the scheduling unit/vcpu relationship.

I have tried to do the hypfs modifications in a rather generic way in
order to be able to use the same infrastructure in other cases, too
(e.g. for per-domain entries).

The complete series has been tested by creating cpupools with different
granularities and moving busy and idle domains between those.

Juergen Gross (12):
  xen/cpupool: add cpu to sched_res_mask when removing it from cpupool
  xen/cpupool: add missing bits for per-cpupool scheduling granularity
  xen/sched: support moving a domain between cpupools with different
    granularity
  xen/sched: sort included headers in cpupool.c
  docs: fix hypfs path documentation
  xen/hypfs: move per-node function pointers into a dedicated struct
  xen/hypfs: pass real failure reason up from hypfs_get_entry()
  xen/hypfs: support dynamic hypfs nodes
  xen/hypfs: add support for id-based dynamic directories
  xen/hypfs: add cpupool directories
  xen/hypfs: add scheduling granularity entry to cpupool entries
  xen/cpupool: make per-cpupool sched-gran hypfs node writable

 docs/misc/hypfs-paths.pandoc |  18 ++-
 xen/common/hypfs.c           | 233 +++++++++++++++++++++++++++--------
 xen/common/sched/core.c      | 122 +++++++++++++-----
 xen/common/sched/cpupool.c   | 213 +++++++++++++++++++++++++++++---
 xen/common/sched/private.h   |   1 +
 xen/include/xen/hypfs.h      | 106 +++++++++++-----
 xen/include/xen/param.h      |  15 +--
 7 files changed, 567 insertions(+), 141 deletions(-)

-- 
2.26.2

[PATCH 01/12] xen/cpupool: add cpu to sched_res_mask when removing it from cpupool

[Juergen Gross]

When a cpu is removed from a cpupool and added to the free cpus it
should be added to sched_res_mask, too.

The related removal from sched_res_mask in case of core scheduling
is already done in schedule_cpu_add().

As long as all cpupools share the same scheduling granularity there
is nothing going wrong with the missing removal, but this will change
when per-cpupool granularity is fully supported.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/sched/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index ed973e90ec..f8c81592af 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -3189,6 +3189,7 @@ int schedule_cpu_rm(unsigned int cpu)
             /* Adjust cpu masks of resources (old and new). */
             cpumask_clear_cpu(cpu_iter, sr->cpus);
             cpumask_set_cpu(cpu_iter, sr_new[idx]->cpus);
+            cpumask_set_cpu(cpu_iter, &sched_res_mask);
 
             /* Init timer. */
             init_timer(&sr_new[idx]->s_timer, s_timer_fn, NULL, cpu_iter);
-- 
2.26.2

[PATCH 02/12] xen/cpupool: add missing bits for per-cpupool scheduling granularity

[Juergen Gross]

Even with storing the scheduling granularity in struct cpupool there
are still a few bits missing for being able to have cpupools with
different granularity (apart from the missing interface for setting
the individual granularities): the number of cpus in a scheduling
unit is always taken from the global sched_granularity variable.

So store the value in struct cpupool and use that instead of
sched_granularity.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/sched/cpupool.c | 3 ++-
 xen/common/sched/private.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/common/sched/cpupool.c b/xen/common/sched/cpupool.c
index 7ea641ca26..6429c8f7b5 100644
--- a/xen/common/sched/cpupool.c
+++ b/xen/common/sched/cpupool.c
@@ -151,7 +151,7 @@ static void __init cpupool_gran_init(void)
 
 unsigned int cpupool_get_granularity(const struct cpupool *c)
 {
-    return c ? sched_granularity : 1;
+    return c ? c->sched_gran : 1;
 }
 
 static void free_cpupool_struct(struct cpupool *c)
@@ -289,6 +289,7 @@ static struct cpupool *cpupool_create(
     }
     c->sched->cpupool = c;
     c->gran = opt_sched_granularity;
+    c->sched_gran = sched_granularity;
 
     *q = c;
 
diff --git a/xen/common/sched/private.h b/xen/common/sched/private.h
index df50976eb2..685992cab9 100644
--- a/xen/common/sched/private.h
+++ b/xen/common/sched/private.h
@@ -514,6 +514,7 @@ struct cpupool
     struct scheduler *sched;
     atomic_t         refcnt;
     enum sched_gran  gran;
+    unsigned int     sched_gran;     /* Number of cpus per sched-item. */
 };
 
 static inline cpumask_t *cpupool_domain_master_cpumask(const struct domain *d)
-- 
2.26.2

[PATCH 03/12] xen/sched: support moving a domain between cpupools with different granularity

[Juergen Gross]

When moving a domain between cpupools with different scheduling
granularity the sched_units of the domain need to be adjusted.

Do that by allocating new sched_units and throwing away the old ones
in sched_move_domain().

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/sched/core.c | 121 ++++++++++++++++++++++++++++++----------
 1 file changed, 90 insertions(+), 31 deletions(-)

diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index f8c81592af..8f1db88593 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -613,17 +613,45 @@ static void sched_move_irqs(const struct sched_unit *unit)
         vcpu_move_irqs(v);
 }
 
+/*
+ * Move a domain from one cpupool to another.
+ *
+ * A domain with any vcpu having temporary affinity settings will be denied
+ * to move. Hard and soft affinities will be reset.
+ *
+ * In order to support cpupools with different scheduling granularities all
+ * scheduling units are replaced by new ones.
+ *
+ * The complete move is done in the following steps:
+ * - check prerequisites (no vcpu with temporary affinities)
+ * - allocate all new data structures (scheduler specific domain data, unit
+ *   memory, scheduler specific unit data)
+ * - pause domain
+ * - temporarily move all (old) units to the same scheduling resource (this
+ *   makes the final resource assignment easier in case the new cpupool has
+ *   a larger granularity than the old one, as the scheduling locks for all
+ *   vcpus must be held for that operation)
+ * - remove old units from scheduling
+ * - set new cpupool and scheduler domain data pointers in struct domain
+ * - switch all vcpus to new units, still assigned to the old scheduling
+ *   resource
+ * - migrate all new units to scheduling resources of the new cpupool
+ * - unpause the domain
+ * - free the old memory (scheduler specific domain data, unit memory,
+ *   scheduler specific unit data)
+ */
 int sched_move_domain(struct domain *d, struct cpupool *c)
 {
     struct vcpu *v;
-    struct sched_unit *unit;
+    struct sched_unit *unit, *old_unit;
+    struct sched_unit *new_units = NULL, *old_units;
+    struct sched_unit **unit_ptr = &new_units;
     unsigned int new_p, unit_idx;
-    void **unit_priv;
     void *domdata;
-    void *unitdata;
-    struct scheduler *old_ops;
+    struct scheduler *old_ops = dom_scheduler(d);
     void *old_domdata;
     unsigned int gran = cpupool_get_granularity(c);
+    unsigned int n_units = DIV_ROUND_UP(d->max_vcpus, gran);
     int ret = 0;
 
     for_each_vcpu ( d, v )
@@ -641,53 +669,78 @@ int sched_move_domain(struct domain *d, struct cpupool *c)
         goto out;
     }
 
-    unit_priv = xzalloc_array(void *, DIV_ROUND_UP(d->max_vcpus, gran));
-    if ( unit_priv == NULL )
+    for ( unit_idx = 0; unit_idx < n_units; unit_idx++ )
     {
-        sched_free_domdata(c->sched, domdata);
-        ret = -ENOMEM;
-        goto out;
-    }
+        unit = sched_alloc_unit_mem();
+        if ( unit )
+        {
+            /* Initialize unit for sched_alloc_udata() to work. */
+            unit->domain = d;
+            unit->unit_id = unit_idx * gran;
+            unit->vcpu_list = d->vcpu[unit->unit_id];
+            unit->priv = sched_alloc_udata(c->sched, unit, domdata);
+            *unit_ptr = unit;
+        }
 
-    unit_idx = 0;
-    for_each_sched_unit ( d, unit )
-    {
-        unit_priv[unit_idx] = sched_alloc_udata(c->sched, unit, domdata);
-        if ( unit_priv[unit_idx] == NULL )
+        if ( !unit || !unit->priv )
         {
-            for ( unit_idx = 0; unit_priv[unit_idx]; unit_idx++ )
-                sched_free_udata(c->sched, unit_priv[unit_idx]);
-            xfree(unit_priv);
-            sched_free_domdata(c->sched, domdata);
+            old_units = new_units;
+            old_domdata = domdata;
             ret = -ENOMEM;
-            goto out;
+            goto out_free;
         }
-        unit_idx++;
+
+        unit_ptr = &unit->next_in_list;
     }
 
     domain_pause(d);
 
-    old_ops = dom_scheduler(d);
     old_domdata = d->sched_priv;
 
+    new_p = cpumask_first(d->cpupool->cpu_valid);
     for_each_sched_unit ( d, unit )
     {
+        spinlock_t *lock;
+
+        /*
+         * Temporarily move all units to same processor to make locking
+         * easier when moving the new units to the new processors.
+         */
+        lock = unit_schedule_lock_irq(unit);
+        sched_set_res(unit, get_sched_res(new_p));
+        spin_unlock_irq(lock);
+
         sched_remove_unit(old_ops, unit);
     }
 
+    old_units = d->sched_unit_list;
+
     d->cpupool = c;
     d->sched_priv = domdata;
 
+    unit = new_units;
+    for_each_vcpu ( d, v )
+    {
+        old_unit = v->sched_unit;
+        if ( unit->unit_id + gran == v->vcpu_id )
+            unit = unit->next_in_list;
+
+        unit->state_entry_time = old_unit->state_entry_time;
+        unit->runstate_cnt[v->runstate.state]++;
+        /* Temporarily use old resource assignment */
+        unit->res = get_sched_res(new_p);
+
+        v->sched_unit = unit;
+    }
+
+    d->sched_unit_list = new_units;
+
     new_p = cpumask_first(c->cpu_valid);
-    unit_idx = 0;
     for_each_sched_unit ( d, unit )
     {
         spinlock_t *lock;
         unsigned int unit_p = new_p;
 
-        unitdata = unit->priv;
-        unit->priv = unit_priv[unit_idx];
-
         for_each_sched_unit_vcpu ( unit, v )
         {
             migrate_timer(&v->periodic_timer, new_p);
@@ -713,8 +766,6 @@ int sched_move_domain(struct domain *d, struct cpupool *c)
 
         sched_insert_unit(c->sched, unit);
 
-        sched_free_udata(old_ops, unitdata);
-
         unit_idx++;
     }
 
@@ -722,11 +773,19 @@ int sched_move_domain(struct domain *d, struct cpupool *c)
 
     domain_unpause(d);
 
-    sched_free_domdata(old_ops, old_domdata);
+ out_free:
+    for ( unit = old_units; unit; )
+    {
+        if ( unit->priv )
+            sched_free_udata(c->sched, unit->priv);
+        old_unit = unit;
+        unit = unit->next_in_list;
+        xfree(old_unit);
+    }
 
-    xfree(unit_priv);
+    sched_free_domdata(old_ops, old_domdata);
 
-out:
+ out:
     rcu_read_unlock(&sched_res_rculock);
 
     return ret;
-- 
2.26.2

[PATCH 04/12] xen/sched: sort included headers in cpupool.c

[Juergen Gross]

Common style is to include header files in alphabetical order. Sort the
#include statements in cpupool.c accordingly.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/sched/cpupool.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/common/sched/cpupool.c b/xen/common/sched/cpupool.c
index 6429c8f7b5..84f326ea63 100644
--- a/xen/common/sched/cpupool.c
+++ b/xen/common/sched/cpupool.c
@@ -11,15 +11,15 @@
  * (C) 2009, Juergen Gross, Fujitsu Technology Solutions
  */
 
-#include <xen/lib.h>
-#include <xen/init.h>
+#include <xen/cpu.h>
 #include <xen/cpumask.h>
+#include <xen/init.h>
+#include <xen/keyhandler.h>
+#include <xen/lib.h>
 #include <xen/param.h>
 #include <xen/percpu.h>
 #include <xen/sched.h>
 #include <xen/warning.h>
-#include <xen/keyhandler.h>
-#include <xen/cpu.h>
 
 #include "private.h"
 
-- 
2.26.2

[PATCH 05/12] docs: fix hypfs path documentation

[Juergen Gross]

The /params/* entry is missing a writable tag.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 docs/misc/hypfs-paths.pandoc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/misc/hypfs-paths.pandoc b/docs/misc/hypfs-paths.pandoc
index dddb592bc5..6c7b2f7ee3 100644
--- a/docs/misc/hypfs-paths.pandoc
+++ b/docs/misc/hypfs-paths.pandoc
@@ -179,7 +179,7 @@ The minor version of Xen.
 
 A directory of runtime parameters.
 
-#### /params/*
+#### /params/* [w]
 
 The individual parameters. The description of the different parameters can be
 found in `docs/misc/xen-command-line.pandoc`.
-- 
2.26.2

[PATCH 06/12] xen/hypfs: move per-node function pointers into a dedicated struct

[Juergen Gross]

Move the function pointers currently stored in each hypfs node into a
dedicated structure in order to save some space for each node. This
will save even more space with additional callbacks added in future.

Provide some standard function vectors.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/hypfs.c      | 25 +++++++++++++++---
 xen/include/xen/hypfs.h | 57 +++++++++++++++++++++++++----------------
 xen/include/xen/param.h | 15 ++++-------
 3 files changed, 62 insertions(+), 35 deletions(-)

diff --git a/xen/common/hypfs.c b/xen/common/hypfs.c
index 8e932b5cf9..e655e8cfc7 100644
--- a/xen/common/hypfs.c
+++ b/xen/common/hypfs.c
@@ -24,6 +24,25 @@ CHECK_hypfs_dirlistentry;
     (DIRENTRY_NAME_OFF +        \
      ROUNDUP((name_len) + 1, alignof(struct xen_hypfs_direntry)))
 
+struct hypfs_funcs hypfs_dir_funcs = {
+    .read = hypfs_read_dir,
+};
+struct hypfs_funcs hypfs_leaf_ro_funcs = {
+    .read = hypfs_read_leaf,
+};
+struct hypfs_funcs hypfs_leaf_wr_funcs = {
+    .read = hypfs_read_leaf,
+    .write = hypfs_write_leaf,
+};
+struct hypfs_funcs hypfs_bool_wr_funcs = {
+    .read = hypfs_read_leaf,
+    .write = hypfs_write_bool,
+};
+struct hypfs_funcs hypfs_custom_wr_funcs = {
+    .read = hypfs_read_leaf,
+    .write = hypfs_write_custom,
+};
+
 static DEFINE_RWLOCK(hypfs_lock);
 enum hypfs_lock_state {
     hypfs_unlocked,
@@ -284,7 +303,7 @@ static int hypfs_read(const struct hypfs_entry *entry,
 
     guest_handle_add_offset(uaddr, sizeof(e));
 
-    ret = entry->read(entry, uaddr);
+    ret = entry->funcs->read(entry, uaddr);
 
  out:
     return ret;
@@ -387,14 +406,14 @@ static int hypfs_write(struct hypfs_entry *entry,
 {
     struct hypfs_entry_leaf *l;
 
-    if ( !entry->write )
+    if ( !entry->funcs->write )
         return -EACCES;
 
     ASSERT(entry->max_size);
 
     l = container_of(entry, struct hypfs_entry_leaf, e);
 
-    return entry->write(l, uaddr, ulen);
+    return entry->funcs->write(l, uaddr, ulen);
 }
 
 long do_hypfs_op(unsigned int cmd,
diff --git a/xen/include/xen/hypfs.h b/xen/include/xen/hypfs.h
index 5ad99cb558..77916ebb58 100644
--- a/xen/include/xen/hypfs.h
+++ b/xen/include/xen/hypfs.h
@@ -7,6 +7,20 @@
 #include <public/hypfs.h>
 
 struct hypfs_entry_leaf;
+struct hypfs_entry;
+
+struct hypfs_funcs {
+    int (*read)(const struct hypfs_entry *entry,
+                XEN_GUEST_HANDLE_PARAM(void) uaddr);
+    int (*write)(struct hypfs_entry_leaf *leaf,
+                 XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned int ulen);
+};
+
+extern struct hypfs_funcs hypfs_dir_funcs;
+extern struct hypfs_funcs hypfs_leaf_ro_funcs;
+extern struct hypfs_funcs hypfs_leaf_wr_funcs;
+extern struct hypfs_funcs hypfs_bool_wr_funcs;
+extern struct hypfs_funcs hypfs_custom_wr_funcs;
 
 struct hypfs_entry {
     unsigned short type;
@@ -15,10 +29,7 @@ struct hypfs_entry {
     unsigned int max_size;
     const char *name;
     struct list_head list;
-    int (*read)(const struct hypfs_entry *entry,
-                XEN_GUEST_HANDLE_PARAM(void) uaddr);
-    int (*write)(struct hypfs_entry_leaf *leaf,
-                 XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned int ulen);
+    struct hypfs_funcs *funcs;
 };
 
 struct hypfs_entry_leaf {
@@ -42,7 +53,7 @@ struct hypfs_entry_dir {
         .e.size = 0,                              \
         .e.max_size = 0,                          \
         .e.list = LIST_HEAD_INIT(var.e.list),     \
-        .e.read = hypfs_read_dir,                 \
+        .e.funcs = &hypfs_dir_funcs,              \
         .dirlist = LIST_HEAD_INIT(var.dirlist),   \
     }
 
@@ -52,7 +63,7 @@ struct hypfs_entry_dir {
         .e.encoding = XEN_HYPFS_ENC_PLAIN,        \
         .e.name = (nam),                          \
         .e.max_size = (msz),                      \
-        .e.read = hypfs_read_leaf,                \
+        .e.funcs = &hypfs_leaf_ro_funcs,          \
     }
 
 /* Content and size need to be set via hypfs_string_set_reference(). */
@@ -72,35 +83,37 @@ static inline void hypfs_string_set_reference(struct hypfs_entry_leaf *leaf,
     leaf->e.size = strlen(str) + 1;
 }
 
-#define HYPFS_FIXEDSIZE_INIT(var, typ, nam, contvar, wr) \
-    struct hypfs_entry_leaf __read_mostly var = {        \
-        .e.type = (typ),                                 \
-        .e.encoding = XEN_HYPFS_ENC_PLAIN,               \
-        .e.name = (nam),                                 \
-        .e.size = sizeof(contvar),                       \
-        .e.max_size = (wr) ? sizeof(contvar) : 0,        \
-        .e.read = hypfs_read_leaf,                       \
-        .e.write = (wr),                                 \
-        .u.content = &(contvar),                         \
+#define HYPFS_FIXEDSIZE_INIT(var, typ, nam, contvar, fn, wr) \
+    struct hypfs_entry_leaf __read_mostly var = {            \
+        .e.type = (typ),                                     \
+        .e.encoding = XEN_HYPFS_ENC_PLAIN,                   \
+        .e.name = (nam),                                     \
+        .e.size = sizeof(contvar),                           \
+        .e.max_size = (wr) ? sizeof(contvar) : 0,            \
+        .e.funcs = (fn),                                     \
+        .u.content = &(contvar),                             \
     }
 
 #define HYPFS_UINT_INIT(var, nam, contvar)                       \
-    HYPFS_FIXEDSIZE_INIT(var, XEN_HYPFS_TYPE_UINT, nam, contvar, NULL)
+    HYPFS_FIXEDSIZE_INIT(var, XEN_HYPFS_TYPE_UINT, nam, contvar, \
+                         &hypfs_leaf_ro_funcs, 0)
 #define HYPFS_UINT_INIT_WRITABLE(var, nam, contvar)              \
     HYPFS_FIXEDSIZE_INIT(var, XEN_HYPFS_TYPE_UINT, nam, contvar, \
-                         hypfs_write_leaf)
+                         &hypfs_leaf_wr_funcs, 1)
 
 #define HYPFS_INT_INIT(var, nam, contvar)                        \
-    HYPFS_FIXEDSIZE_INIT(var, XEN_HYPFS_TYPE_INT, nam, contvar, NULL)
+    HYPFS_FIXEDSIZE_INIT(var, XEN_HYPFS_TYPE_INT, nam, contvar,  \
+                         &hypfs_leaf_ro_funcs, 0)
 #define HYPFS_INT_INIT_WRITABLE(var, nam, contvar)               \
     HYPFS_FIXEDSIZE_INIT(var, XEN_HYPFS_TYPE_INT, nam, contvar, \
-                         hypfs_write_leaf)
+                         &hypfs_leaf_wr_funcs, 1)
 
 #define HYPFS_BOOL_INIT(var, nam, contvar)                       \
-    HYPFS_FIXEDSIZE_INIT(var, XEN_HYPFS_TYPE_BOOL, nam, contvar, NULL)
+    HYPFS_FIXEDSIZE_INIT(var, XEN_HYPFS_TYPE_BOOL, nam, contvar, \
+                         &hypfs_leaf_ro_funcs, 0)
 #define HYPFS_BOOL_INIT_WRITABLE(var, nam, contvar)              \
     HYPFS_FIXEDSIZE_INIT(var, XEN_HYPFS_TYPE_BOOL, nam, contvar, \
-                         hypfs_write_bool)
+                         &hypfs_bool_wr_funcs, 1)
 
 extern struct hypfs_entry_dir hypfs_root;
 
diff --git a/xen/include/xen/param.h b/xen/include/xen/param.h
index d0409d3a0e..1b2c7db954 100644
--- a/xen/include/xen/param.h
+++ b/xen/include/xen/param.h
@@ -116,8 +116,7 @@ extern struct param_hypfs __paramhypfs_start[], __paramhypfs_end[];
         { .hypfs.e.type = XEN_HYPFS_TYPE_STRING, \
           .hypfs.e.encoding = XEN_HYPFS_ENC_PLAIN, \
           .hypfs.e.name = (nam), \
-          .hypfs.e.read = hypfs_read_leaf, \
-          .hypfs.e.write = hypfs_write_custom, \
+          .hypfs.e.funcs = &hypfs_custom_wr_funcs, \
           .init_leaf = (initfunc), \
           .func = (variable) }
 #define boolean_runtime_only_param(nam, variable) \
@@ -127,8 +126,7 @@ extern struct param_hypfs __paramhypfs_start[], __paramhypfs_end[];
           .hypfs.e.name = (nam), \
           .hypfs.e.size = sizeof(variable), \
           .hypfs.e.max_size = sizeof(variable), \
-          .hypfs.e.read = hypfs_read_leaf, \
-          .hypfs.e.write = hypfs_write_bool, \
+          .hypfs.e.funcs = &hypfs_bool_wr_funcs, \
           .hypfs.u.content = &(variable) }
 #define integer_runtime_only_param(nam, variable) \
     __paramfs __parfs_##variable = \
@@ -137,8 +135,7 @@ extern struct param_hypfs __paramhypfs_start[], __paramhypfs_end[];
           .hypfs.e.name = (nam), \
           .hypfs.e.size = sizeof(variable), \
           .hypfs.e.max_size = sizeof(variable), \
-          .hypfs.e.read = hypfs_read_leaf, \
-          .hypfs.e.write = hypfs_write_leaf, \
+          .hypfs.e.funcs = &hypfs_leaf_wr_funcs, \
           .hypfs.u.content = &(variable) }
 #define size_runtime_only_param(nam, variable) \
     __paramfs __parfs_##variable = \
@@ -147,8 +144,7 @@ extern struct param_hypfs __paramhypfs_start[], __paramhypfs_end[];
           .hypfs.e.name = (nam), \
           .hypfs.e.size = sizeof(variable), \
           .hypfs.e.max_size = sizeof(variable), \
-          .hypfs.e.read = hypfs_read_leaf, \
-          .hypfs.e.write = hypfs_write_leaf, \
+          .hypfs.e.funcs = &hypfs_leaf_wr_funcs, \
           .hypfs.u.content = &(variable) }
 #define string_runtime_only_param(nam, variable) \
     __paramfs __parfs_##variable = \
@@ -157,8 +153,7 @@ extern struct param_hypfs __paramhypfs_start[], __paramhypfs_end[];
           .hypfs.e.name = (nam), \
           .hypfs.e.size = 0, \
           .hypfs.e.max_size = sizeof(variable), \
-          .hypfs.e.read = hypfs_read_leaf, \
-          .hypfs.e.write = hypfs_write_leaf, \
+          .hypfs.e.funcs = &hypfs_leaf_wr_funcs, \
           .hypfs.u.content = &(variable) }
 
 #else
-- 
2.26.2

[PATCH 07/12] xen/hypfs: pass real failure reason up from hypfs_get_entry()

[Juergen Gross]

Instead of handling all errors from hypfs_get_entry() as ENOENT pass
up the real error value via ERR_PTR().

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/hypfs.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/xen/common/hypfs.c b/xen/common/hypfs.c
index e655e8cfc7..97260bd4a3 100644
--- a/xen/common/hypfs.c
+++ b/xen/common/hypfs.c
@@ -182,7 +182,7 @@ static struct hypfs_entry *hypfs_get_entry_rel(struct hypfs_entry_dir *dir,
     while ( again )
     {
         if ( dir->e.type != XEN_HYPFS_TYPE_DIR )
-            return NULL;
+            return ERR_PTR(-ENOENT);
 
         if ( !*path )
             return &dir->e;
@@ -201,7 +201,7 @@ static struct hypfs_entry *hypfs_get_entry_rel(struct hypfs_entry_dir *dir,
                                                      struct hypfs_entry_dir, e);
 
             if ( cmp < 0 )
-                return NULL;
+                return ERR_PTR(-ENOENT);
             if ( !cmp && strlen(entry->name) == name_len )
             {
                 if ( !*end )
@@ -216,13 +216,13 @@ static struct hypfs_entry *hypfs_get_entry_rel(struct hypfs_entry_dir *dir,
         }
     }
 
-    return NULL;
+    return ERR_PTR(-ENOENT);
 }
 
 static struct hypfs_entry *hypfs_get_entry(const char *path)
 {
     if ( path[0] != '/' )
-        return NULL;
+        return ERR_PTR(-EINVAL);
 
     return hypfs_get_entry_rel(&hypfs_root, path + 1);
 }
@@ -446,9 +446,9 @@ long do_hypfs_op(unsigned int cmd,
         goto out;
 
     entry = hypfs_get_entry(path);
-    if ( !entry )
+    if ( IS_ERR(entry) )
     {
-        ret = -ENOENT;
+        ret = PTR_ERR(entry);
         goto out;
     }
 
-- 
2.26.2

[PATCH 08/12] xen/hypfs: support dynamic hypfs nodes

[Juergen Gross]

Add a getsize() function pointer to struct hypfs_funcs for being able
to have dynamically filled entries without the need to take the hypfs
lock each time the contents are being generated.

For directories add a findentry callback to the vector and modify
hypfs_get_entry_rel() to use it.

Add a HYPFS_VARDIR_INIT() macro for intializing such a directory
statically, taking a struct hypfs_funcs pointer as parameter additional
to those of HYPFS_DIR_INIT().

Modify HYPFS_VARSIZE_INIT() to take the function vector pointer as an
additional parameter as this will be needed for dynamical entries.

Move DIRENTRY_SIZE() macro to hypfs.h as it will be needed by the read
function of a directory with dynamically generated entries.

For being able to let the generic hypfs coding continue to work on
normal struct hypfs_entry entities even for dynamical nodes add some
infrastructure for allocating a working area for the current hypfs
request in order to store needed information for traversing the tree.
This area is anchored in a percpu pointer and can be retrieved by any
level of the dynamic entries. It will be freed automatically when
dropping the hypfs lock.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/hypfs.c      | 124 +++++++++++++++++++++++++---------------
 xen/include/xen/hypfs.h |  39 +++++++++----
 2 files changed, 108 insertions(+), 55 deletions(-)

diff --git a/xen/common/hypfs.c b/xen/common/hypfs.c
index 97260bd4a3..4c226a06b4 100644
--- a/xen/common/hypfs.c
+++ b/xen/common/hypfs.c
@@ -19,28 +19,29 @@
 CHECK_hypfs_dirlistentry;
 #endif
 
-#define DIRENTRY_NAME_OFF offsetof(struct xen_hypfs_dirlistentry, name)
-#define DIRENTRY_SIZE(name_len) \
-    (DIRENTRY_NAME_OFF +        \
-     ROUNDUP((name_len) + 1, alignof(struct xen_hypfs_direntry)))
-
 struct hypfs_funcs hypfs_dir_funcs = {
     .read = hypfs_read_dir,
+    .getsize = hypfs_getsize,
+    .findentry = hypfs_dir_findentry,
 };
 struct hypfs_funcs hypfs_leaf_ro_funcs = {
     .read = hypfs_read_leaf,
+    .getsize = hypfs_getsize,
 };
 struct hypfs_funcs hypfs_leaf_wr_funcs = {
     .read = hypfs_read_leaf,
     .write = hypfs_write_leaf,
+    .getsize = hypfs_getsize,
 };
 struct hypfs_funcs hypfs_bool_wr_funcs = {
     .read = hypfs_read_leaf,
     .write = hypfs_write_bool,
+    .getsize = hypfs_getsize,
 };
 struct hypfs_funcs hypfs_custom_wr_funcs = {
     .read = hypfs_read_leaf,
     .write = hypfs_write_custom,
+    .getsize = hypfs_getsize,
 };
 
 static DEFINE_RWLOCK(hypfs_lock);
@@ -50,6 +51,7 @@ enum hypfs_lock_state {
     hypfs_write_locked
 };
 static DEFINE_PER_CPU(enum hypfs_lock_state, hypfs_locked);
+static DEFINE_PER_CPU(void *, hypfs_dyndata);
 
 HYPFS_DIR_INIT(hypfs_root, "");
 
@@ -71,9 +73,12 @@ static void hypfs_write_lock(void)
 
 static void hypfs_unlock(void)
 {
-    enum hypfs_lock_state locked = this_cpu(hypfs_locked);
+    unsigned int cpu = smp_processor_id();
+    enum hypfs_lock_state locked = per_cpu(hypfs_locked, cpu);
+
+    XFREE(per_cpu(hypfs_dyndata, cpu));
 
-    this_cpu(hypfs_locked) = hypfs_unlocked;
+    per_cpu(hypfs_locked, cpu) = hypfs_unlocked;
 
     switch ( locked )
     {
@@ -88,6 +93,23 @@ static void hypfs_unlock(void)
     }
 }
 
+void *hypfs_alloc_dyndata(unsigned long size, unsigned long align)
+{
+    unsigned int cpu = smp_processor_id();
+
+    ASSERT(per_cpu(hypfs_locked, cpu) != hypfs_unlocked);
+    ASSERT(per_cpu(hypfs_dyndata, cpu) == NULL);
+
+    per_cpu(hypfs_dyndata, cpu) = _xzalloc(size, align);
+
+    return per_cpu(hypfs_dyndata, cpu);
+}
+
+void *hypfs_get_dyndata(void)
+{
+    return this_cpu(hypfs_dyndata);
+}
+
 static int add_entry(struct hypfs_entry_dir *parent, struct hypfs_entry *new)
 {
     int ret = -ENOENT;
@@ -122,7 +144,7 @@ static int add_entry(struct hypfs_entry_dir *parent, struct hypfs_entry *new)
     {
         unsigned int sz = strlen(new->name);
 
-        parent->e.size += DIRENTRY_SIZE(sz);
+        parent->e.size += HYPFS_DIRENTRY_SIZE(sz);
     }
 
     hypfs_unlock();
@@ -171,15 +193,34 @@ static int hypfs_get_path_user(char *buf,
     return 0;
 }
 
+struct hypfs_entry *hypfs_dir_findentry(struct hypfs_entry_dir *dir,
+                                        const char *name,
+                                        unsigned int name_len)
+{
+    struct hypfs_entry *entry;
+
+    list_for_each_entry ( entry, &dir->dirlist, list )
+    {
+        int cmp = strncmp(name, entry->name, name_len);
+
+        if ( cmp < 0 )
+            return ERR_PTR(-ENOENT);
+
+        if ( !cmp && strlen(entry->name) == name_len )
+            return entry;
+    }
+
+    return ERR_PTR(-ENOENT);
+}
+
 static struct hypfs_entry *hypfs_get_entry_rel(struct hypfs_entry_dir *dir,
                                                const char *path)
 {
     const char *end;
     struct hypfs_entry *entry;
     unsigned int name_len;
-    bool again = true;
 
-    while ( again )
+    for ( ;; )
     {
         if ( dir->e.type != XEN_HYPFS_TYPE_DIR )
             return ERR_PTR(-ENOENT);
@@ -192,28 +233,12 @@ static struct hypfs_entry *hypfs_get_entry_rel(struct hypfs_entry_dir *dir,
             end = strchr(path, '\0');
         name_len = end - path;
 
-        again = false;
+        entry = dir->e.funcs->findentry(dir, path, name_len);
+        if ( IS_ERR(entry) || !*end )
+            return entry;
 
-        list_for_each_entry ( entry, &dir->dirlist, list )
-        {
-            int cmp = strncmp(path, entry->name, name_len);
-            struct hypfs_entry_dir *d = container_of(entry,
-                                                     struct hypfs_entry_dir, e);
-
-            if ( cmp < 0 )
-                return ERR_PTR(-ENOENT);
-            if ( !cmp && strlen(entry->name) == name_len )
-            {
-                if ( !*end )
-                    return entry;
-
-                again = true;
-                dir = d;
-                path = end + 1;
-
-                break;
-            }
-        }
+        path = end + 1;
+        dir = container_of(entry, struct hypfs_entry_dir, e);
     }
 
     return ERR_PTR(-ENOENT);
@@ -227,12 +252,17 @@ static struct hypfs_entry *hypfs_get_entry(const char *path)
     return hypfs_get_entry_rel(&hypfs_root, path + 1);
 }
 
+unsigned int hypfs_getsize(const struct hypfs_entry *entry)
+{
+    return entry->size;
+}
+
 int hypfs_read_dir(const struct hypfs_entry *entry,
                    XEN_GUEST_HANDLE_PARAM(void) uaddr)
 {
     const struct hypfs_entry_dir *d;
     const struct hypfs_entry *e;
-    unsigned int size = entry->size;
+    unsigned int size = entry->funcs->getsize(entry);
 
     ASSERT(this_cpu(hypfs_locked) != hypfs_unlocked);
 
@@ -242,18 +272,18 @@ int hypfs_read_dir(const struct hypfs_entry *entry,
     {
         struct xen_hypfs_dirlistentry direntry;
         unsigned int e_namelen = strlen(e->name);
-        unsigned int e_len = DIRENTRY_SIZE(e_namelen);
+        unsigned int e_len = HYPFS_DIRENTRY_SIZE(e_namelen);
 
         direntry.e.pad = 0;
         direntry.e.type = e->type;
         direntry.e.encoding = e->encoding;
-        direntry.e.content_len = e->size;
+        direntry.e.content_len = e->funcs->getsize(e);
         direntry.e.max_write_len = e->max_size;
         direntry.off_next = list_is_last(&e->list, &d->dirlist) ? 0 : e_len;
         if ( copy_to_guest(uaddr, &direntry, 1) )
             return -EFAULT;
 
-        if ( copy_to_guest_offset(uaddr, DIRENTRY_NAME_OFF,
+        if ( copy_to_guest_offset(uaddr, HYPFS_DIRENTRY_NAME_OFF,
                                   e->name, e_namelen + 1) )
             return -EFAULT;
 
@@ -275,22 +305,25 @@ int hypfs_read_leaf(const struct hypfs_entry *entry,
 
     l = container_of(entry, const struct hypfs_entry_leaf, e);
 
-    return copy_to_guest(uaddr, l->u.content, entry->size) ? -EFAULT: 0;
+    return copy_to_guest(uaddr, l->u.content, entry->funcs->getsize(entry)) ?
+                                              -EFAULT : 0;
 }
 
 static int hypfs_read(const struct hypfs_entry *entry,
                       XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen)
 {
     struct xen_hypfs_direntry e;
+    unsigned int size;
     long ret = -EINVAL;
 
     if ( ulen < sizeof(e) )
         goto out;
 
+    size = entry->funcs->getsize(entry);
     e.pad = 0;
     e.type = entry->type;
     e.encoding = entry->encoding;
-    e.content_len = entry->size;
+    e.content_len = size;
     e.max_write_len = entry->max_size;
 
     ret = -EFAULT;
@@ -298,7 +331,7 @@ static int hypfs_read(const struct hypfs_entry *entry,
         goto out;
 
     ret = -ENOBUFS;
-    if ( ulen < entry->size + sizeof(e) )
+    if ( ulen < size + sizeof(e) )
         goto out;
 
     guest_handle_add_offset(uaddr, sizeof(e));
@@ -314,14 +347,15 @@ int hypfs_write_leaf(struct hypfs_entry_leaf *leaf,
 {
     char *buf;
     int ret;
+    struct hypfs_entry *e = &leaf->e;
 
     ASSERT(this_cpu(hypfs_locked) == hypfs_write_locked);
 
-    if ( ulen > leaf->e.max_size )
+    if ( ulen > e->max_size )
         return -ENOSPC;
 
-    if ( leaf->e.type != XEN_HYPFS_TYPE_STRING &&
-         leaf->e.type != XEN_HYPFS_TYPE_BLOB && ulen != leaf->e.size )
+    if ( e->type != XEN_HYPFS_TYPE_STRING &&
+         e->type != XEN_HYPFS_TYPE_BLOB && ulen != e->funcs->getsize(e) )
         return -EDOM;
 
     buf = xmalloc_array(char, ulen);
@@ -333,14 +367,14 @@ int hypfs_write_leaf(struct hypfs_entry_leaf *leaf,
         goto out;
 
     ret = -EINVAL;
-    if ( leaf->e.type == XEN_HYPFS_TYPE_STRING &&
-         leaf->e.encoding == XEN_HYPFS_ENC_PLAIN &&
+    if ( e->type == XEN_HYPFS_TYPE_STRING &&
+         e->encoding == XEN_HYPFS_ENC_PLAIN &&
          memchr(buf, 0, ulen) != (buf + ulen - 1) )
         goto out;
 
     ret = 0;
     memcpy(leaf->u.write_ptr, buf, ulen);
-    leaf->e.size = ulen;
+    e->size = ulen;
 
  out:
     xfree(buf);
@@ -354,7 +388,7 @@ int hypfs_write_bool(struct hypfs_entry_leaf *leaf,
 
     ASSERT(this_cpu(hypfs_locked) == hypfs_write_locked);
     ASSERT(leaf->e.type == XEN_HYPFS_TYPE_BOOL &&
-           leaf->e.size == sizeof(bool) &&
+           leaf->e.funcs->getsize(&leaf->e) == sizeof(bool) &&
            leaf->e.max_size == sizeof(bool) );
 
     if ( ulen != leaf->e.max_size )
diff --git a/xen/include/xen/hypfs.h b/xen/include/xen/hypfs.h
index 77916ebb58..c8999b5381 100644
--- a/xen/include/xen/hypfs.h
+++ b/xen/include/xen/hypfs.h
@@ -2,11 +2,13 @@
 #define __XEN_HYPFS_H__
 
 #ifdef CONFIG_HYPFS
+#include <xen/lib.h>
 #include <xen/list.h>
 #include <xen/string.h>
 #include <public/hypfs.h>
 
 struct hypfs_entry_leaf;
+struct hypfs_entry_dir;
 struct hypfs_entry;
 
 struct hypfs_funcs {
@@ -14,6 +16,9 @@ struct hypfs_funcs {
                 XEN_GUEST_HANDLE_PARAM(void) uaddr);
     int (*write)(struct hypfs_entry_leaf *leaf,
                  XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned int ulen);
+    unsigned int (*getsize)(const struct hypfs_entry *entry);
+    struct hypfs_entry *(*findentry)(struct hypfs_entry_dir *dir,
+                                     const char *name, unsigned int name_len);
 };
 
 extern struct hypfs_funcs hypfs_dir_funcs;
@@ -45,7 +50,12 @@ struct hypfs_entry_dir {
     struct list_head dirlist;
 };
 
-#define HYPFS_DIR_INIT(var, nam)                  \
+#define HYPFS_DIRENTRY_NAME_OFF offsetof(struct xen_hypfs_dirlistentry, name)
+#define HYPFS_DIRENTRY_SIZE(name_len) \
+    (HYPFS_DIRENTRY_NAME_OFF +        \
+     ROUNDUP((name_len) + 1, alignof(struct xen_hypfs_direntry)))
+
+#define HYPFS_VARDIR_INIT(var, nam, fn)           \
     struct hypfs_entry_dir __read_mostly var = {  \
         .e.type = XEN_HYPFS_TYPE_DIR,             \
         .e.encoding = XEN_HYPFS_ENC_PLAIN,        \
@@ -53,22 +63,25 @@ struct hypfs_entry_dir {
         .e.size = 0,                              \
         .e.max_size = 0,                          \
         .e.list = LIST_HEAD_INIT(var.e.list),     \
-        .e.funcs = &hypfs_dir_funcs,              \
+        .e.funcs = (fn),                          \
         .dirlist = LIST_HEAD_INIT(var.dirlist),   \
     }
 
-#define HYPFS_VARSIZE_INIT(var, typ, nam, msz)    \
-    struct hypfs_entry_leaf __read_mostly var = { \
-        .e.type = (typ),                          \
-        .e.encoding = XEN_HYPFS_ENC_PLAIN,        \
-        .e.name = (nam),                          \
-        .e.max_size = (msz),                      \
-        .e.funcs = &hypfs_leaf_ro_funcs,          \
+#define HYPFS_DIR_INIT(var, nam)                  \
+    HYPFS_VARDIR_INIT(var, nam, &hypfs_dir_funcs)
+
+#define HYPFS_VARSIZE_INIT(var, typ, nam, msz, fn) \
+    struct hypfs_entry_leaf __read_mostly var = {  \
+        .e.type = (typ),                           \
+        .e.encoding = XEN_HYPFS_ENC_PLAIN,         \
+        .e.name = (nam),                           \
+        .e.max_size = (msz),                       \
+        .e.funcs = (fn),                           \
     }
 
 /* Content and size need to be set via hypfs_string_set_reference(). */
 #define HYPFS_STRING_INIT(var, nam)               \
-    HYPFS_VARSIZE_INIT(var, XEN_HYPFS_TYPE_STRING, nam, 0)
+    HYPFS_VARSIZE_INIT(var, XEN_HYPFS_TYPE_STRING, nam, 0, &hypfs_leaf_ro_funcs)
 
 /*
  * Set content and size of a XEN_HYPFS_TYPE_STRING node. The node will point
@@ -131,6 +144,12 @@ int hypfs_write_bool(struct hypfs_entry_leaf *leaf,
                      XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned int ulen);
 int hypfs_write_custom(struct hypfs_entry_leaf *leaf,
                        XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned int ulen);
+unsigned int hypfs_getsize(const struct hypfs_entry *entry);
+struct hypfs_entry *hypfs_dir_findentry(struct hypfs_entry_dir *dir,
+                                        const char *name,
+                                        unsigned int name_len);
+void *hypfs_alloc_dyndata(unsigned long size, unsigned long align);
+void *hypfs_get_dyndata(void);
 #endif
 
 #endif /* __XEN_HYPFS_H__ */
-- 
2.26.2

[PATCH 09/12] xen/hypfs: add support for id-based dynamic directories

[Juergen Gross]

Add some helpers to hypfs.c to support dynamic directories with a
numerical id as name.

The dynamic directory is based on a template specified by the user
allowing to use specific access functions and having a predefined
set of entries in the directory.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/hypfs.c      | 76 +++++++++++++++++++++++++++++++++++++++++
 xen/include/xen/hypfs.h | 14 ++++++++
 2 files changed, 90 insertions(+)

diff --git a/xen/common/hypfs.c b/xen/common/hypfs.c
index 4c226a06b4..12be2f6d16 100644
--- a/xen/common/hypfs.c
+++ b/xen/common/hypfs.c
@@ -257,6 +257,82 @@ unsigned int hypfs_getsize(const struct hypfs_entry *entry)
     return entry->size;
 }
 
+int hypfs_read_dyndir_id_entry(struct hypfs_entry_dir *template,
+                               unsigned int id, bool is_last,
+                               XEN_GUEST_HANDLE_PARAM(void) *uaddr)
+{
+    struct xen_hypfs_dirlistentry direntry;
+    char name[12];
+    unsigned int e_namelen, e_len;
+
+    e_namelen = snprintf(name, sizeof(name), "%u", id);
+    e_len = HYPFS_DIRENTRY_SIZE(e_namelen);
+    direntry.e.pad = 0;
+    direntry.e.type = template->e.type;
+    direntry.e.encoding = template->e.encoding;
+    direntry.e.content_len = template->e.funcs->getsize(&template->e);
+    direntry.e.max_write_len = template->e.max_size;
+    direntry.off_next = is_last ? 0 : e_len;
+    if ( copy_to_guest(*uaddr, &direntry, 1) )
+        return -EFAULT;
+    if ( copy_to_guest_offset(*uaddr, HYPFS_DIRENTRY_NAME_OFF, name,
+                              e_namelen + 1) )
+        return -EFAULT;
+
+    guest_handle_add_offset(*uaddr, e_len);
+
+    return 0;
+}
+
+static struct hypfs_entry *hypfs_dyndir_findentry(struct hypfs_entry_dir *dir,
+                                                  const char *name,
+                                                  unsigned int name_len)
+{
+    struct hypfs_dyndir_id *data;
+
+    data = hypfs_get_dyndata();
+    if ( !data )
+        return ERR_PTR(-ENOENT);
+
+    /* Use template with original findentry function. */
+    return data->template->e.funcs->findentry(data->template, name, name_len);
+}
+
+static int hypfs_read_dyndir(const struct hypfs_entry *entry,
+                             XEN_GUEST_HANDLE_PARAM(void) uaddr)
+{
+    struct hypfs_dyndir_id *data;
+
+    data = hypfs_get_dyndata();
+    if ( !data )
+        return -ENOENT;
+
+    /* Use template with original read function. */
+    return data->template->e.funcs->read(&data->template->e, uaddr);
+}
+
+struct hypfs_entry *hypfs_gen_dyndir_entry_id(struct hypfs_entry_dir *template,
+                                              unsigned int id)
+{
+    struct hypfs_dyndir_id *data;
+
+    data = hypfs_alloc_dyndata(sizeof(*data), alignof(*data));
+    if ( !data )
+        return ERR_PTR(-ENOMEM);
+
+    data->template = template;
+    data->id = id;
+    snprintf(data->name, sizeof(data->name), "%u", id);
+    data->dir = *template;
+    data->dir.e.name = data->name;
+    data->dir.e.funcs = &data->funcs;
+    data->funcs = *template->e.funcs;
+    data->funcs.findentry = hypfs_dyndir_findentry;
+    data->funcs.read = hypfs_read_dyndir;
+
+    return &data->dir.e;
+}
+
 int hypfs_read_dir(const struct hypfs_entry *entry,
                    XEN_GUEST_HANDLE_PARAM(void) uaddr)
 {
diff --git a/xen/include/xen/hypfs.h b/xen/include/xen/hypfs.h
index c8999b5381..adfb522496 100644
--- a/xen/include/xen/hypfs.h
+++ b/xen/include/xen/hypfs.h
@@ -50,6 +50,15 @@ struct hypfs_entry_dir {
     struct list_head dirlist;
 };
 
+struct hypfs_dyndir_id {
+    struct hypfs_entry_dir dir;       /* Modified copy of template. */
+    struct hypfs_funcs funcs;         /* Dynamic functions. */
+    struct hypfs_entry_dir *template; /* Template used. */
+    char name[12];                    /* Name of hypfs entry. */
+
+    unsigned int id;                  /* Numerical id. */
+};
+
 #define HYPFS_DIRENTRY_NAME_OFF offsetof(struct xen_hypfs_dirlistentry, name)
 #define HYPFS_DIRENTRY_SIZE(name_len) \
     (HYPFS_DIRENTRY_NAME_OFF +        \
@@ -150,6 +159,11 @@ struct hypfs_entry *hypfs_dir_findentry(struct hypfs_entry_dir *dir,
                                         unsigned int name_len);
 void *hypfs_alloc_dyndata(unsigned long size, unsigned long align);
 void *hypfs_get_dyndata(void);
+int hypfs_read_dyndir_id_entry(struct hypfs_entry_dir *template,
+                               unsigned int id, bool is_last,
+                               XEN_GUEST_HANDLE_PARAM(void) *uaddr);
+struct hypfs_entry *hypfs_gen_dyndir_entry_id(struct hypfs_entry_dir *template,
+                                              unsigned int id);
 #endif
 
 #endif /* __XEN_HYPFS_H__ */
-- 
2.26.2

[PATCH 10/12] xen/hypfs: add cpupool directories

[Juergen Gross]

Add /cpupool/<cpupool-id> directories to hypfs. Those are completely
dynamic, so the related hypfs access functions need to be implemented.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 docs/misc/hypfs-paths.pandoc |  9 +++++
 xen/common/sched/cpupool.c   | 78 ++++++++++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+)

diff --git a/docs/misc/hypfs-paths.pandoc b/docs/misc/hypfs-paths.pandoc
index 6c7b2f7ee3..aaca1cdf92 100644
--- a/docs/misc/hypfs-paths.pandoc
+++ b/docs/misc/hypfs-paths.pandoc
@@ -175,6 +175,15 @@ The major version of Xen.
 
 The minor version of Xen.
 
+#### /cpupool/
+
+A directory of all current cpupools.
+
+#### /cpupool/*/
+
+The individual cpupools. Each entry is a directory with the name being the
+cpupool-id (e.g. /cpupool/0/).
+
 #### /params/
 
 A directory of runtime parameters.
diff --git a/xen/common/sched/cpupool.c b/xen/common/sched/cpupool.c
index 84f326ea63..8612ee5cf6 100644
--- a/xen/common/sched/cpupool.c
+++ b/xen/common/sched/cpupool.c
@@ -13,6 +13,8 @@
 
 #include <xen/cpu.h>
 #include <xen/cpumask.h>
+#include <xen/guest_access.h>
+#include <xen/hypfs.h>
 #include <xen/init.h>
 #include <xen/keyhandler.h>
 #include <xen/lib.h>
@@ -992,6 +994,78 @@ static struct notifier_block cpu_nfb = {
     .notifier_call = cpu_callback
 };
 
+#ifdef CONFIG_HYPFS
+static HYPFS_DIR_INIT(cpupool_pooldir, "id");
+
+static int cpupool_dir_read(const struct hypfs_entry *entry,
+                            XEN_GUEST_HANDLE_PARAM(void) uaddr)
+{
+    int ret = 0;
+    struct cpupool **q;
+
+    spin_lock(&cpupool_lock);
+
+    for_each_cpupool(q)
+    {
+        ret = hypfs_read_dyndir_id_entry(&cpupool_pooldir, (*q)->cpupool_id,
+                                         !(*q)->next, &uaddr);
+        if ( ret )
+            break;
+    }
+
+    spin_unlock(&cpupool_lock);
+
+    return ret;
+}
+
+static unsigned int cpupool_dir_getsize(const struct hypfs_entry *entry)
+{
+    struct cpupool **q;
+    unsigned int size = 0;
+
+    spin_lock(&cpupool_lock);
+
+    for_each_cpupool(q)
+        size += HYPFS_DIRENTRY_SIZE(snprintf(NULL, 0, "%d", (*q)->cpupool_id));
+
+    spin_unlock(&cpupool_lock);
+
+    return size;
+}
+
+static struct hypfs_entry *cpupool_dir_findentry(struct hypfs_entry_dir *dir,
+                                                 const char *name,
+                                                 unsigned int name_len)
+{
+    unsigned long id;
+    const char *end;
+    struct cpupool *cpupool;
+
+    id = simple_strtoul(name, &end, 10);
+    if ( id > INT_MAX || end != name + name_len )
+        return ERR_PTR(-ENOENT);
+
+    spin_lock(&cpupool_lock);
+
+    cpupool = __cpupool_find_by_id(id, true);
+
+    spin_unlock(&cpupool_lock);
+
+    if ( !cpupool )
+        return ERR_PTR(-ENOENT);
+
+    return hypfs_gen_dyndir_entry_id(&cpupool_pooldir, id);
+}
+
+static struct hypfs_funcs cpupool_dir_funcs = {
+    .read = cpupool_dir_read,
+    .getsize = cpupool_dir_getsize,
+    .findentry = cpupool_dir_findentry,
+};
+
+static HYPFS_VARDIR_INIT(cpupool_dir, "cpupool", &cpupool_dir_funcs);
+#endif
+
 static int __init cpupool_init(void)
 {
     unsigned int cpu;
@@ -999,6 +1073,10 @@ static int __init cpupool_init(void)
 
     cpupool_gran_init();
 
+#ifdef CONFIG_HYPFS
+    hypfs_add_dir(&hypfs_root, &cpupool_dir, true);
+#endif
+
     cpupool0 = cpupool_create(0, 0, &err);
     BUG_ON(cpupool0 == NULL);
     cpupool_put(cpupool0);
-- 
2.26.2

[PATCH 11/12] xen/hypfs: add scheduling granularity entry to cpupool entries

[Juergen Gross]

Add a "sched-gran" entry to the per-cpupool hypfs directories.

For now make this entry read-only and let it contain one of the
strings "cpu", "core" or "socket".

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 docs/misc/hypfs-paths.pandoc |  4 +++
 xen/common/sched/cpupool.c   | 51 +++++++++++++++++++++++++++++++++---
 2 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/docs/misc/hypfs-paths.pandoc b/docs/misc/hypfs-paths.pandoc
index aaca1cdf92..f1ce24d7fe 100644
--- a/docs/misc/hypfs-paths.pandoc
+++ b/docs/misc/hypfs-paths.pandoc
@@ -184,6 +184,10 @@ A directory of all current cpupools.
 The individual cpupools. Each entry is a directory with the name being the
 cpupool-id (e.g. /cpupool/0/).
 
+#### /cpupool/*/sched-gran = ("cpu" | "core" | "socket")
+
+The scheduling granularity of a cpupool.
+
 #### /params/
 
 A directory of runtime parameters.
diff --git a/xen/common/sched/cpupool.c b/xen/common/sched/cpupool.c
index 8612ee5cf6..8674ac0fdd 100644
--- a/xen/common/sched/cpupool.c
+++ b/xen/common/sched/cpupool.c
@@ -42,9 +42,10 @@ static DEFINE_SPINLOCK(cpupool_lock);
 static enum sched_gran __read_mostly opt_sched_granularity = SCHED_GRAN_cpu;
 static unsigned int __read_mostly sched_granularity = 1;
 
+#define SCHED_GRAN_NAME_LEN  8
 struct sched_gran_name {
     enum sched_gran mode;
-    char name[8];
+    char name[SCHED_GRAN_NAME_LEN];
 };
 
 static const struct sched_gran_name sg_name[] = {
@@ -53,7 +54,7 @@ static const struct sched_gran_name sg_name[] = {
     {SCHED_GRAN_socket, "socket"},
 };
 
-static void sched_gran_print(enum sched_gran mode, unsigned int gran)
+static const char *sched_gran_get_name(enum sched_gran mode)
 {
     const char *name = "";
     unsigned int i;
@@ -67,8 +68,13 @@ static void sched_gran_print(enum sched_gran mode, unsigned int gran)
         }
     }
 
+    return name;
+}
+
+static void sched_gran_print(enum sched_gran mode, unsigned int gran)
+{
     printk("Scheduling granularity: %s, %u CPU%s per sched-resource\n",
-           name, gran, gran == 1 ? "" : "s");
+           sched_gran_get_name(mode), gran, gran == 1 ? "" : "s");
 }
 
 #ifdef CONFIG_HAS_SCHED_GRANULARITY
@@ -1057,6 +1063,43 @@ static struct hypfs_entry *cpupool_dir_findentry(struct hypfs_entry_dir *dir,
     return hypfs_gen_dyndir_entry_id(&cpupool_pooldir, id);
 }
 
+static int cpupool_gran_read(const struct hypfs_entry *entry,
+                             XEN_GUEST_HANDLE_PARAM(void) uaddr)
+{
+    const struct hypfs_dyndir_id *data;
+    struct cpupool *cpupool;
+    const char *name = "";
+
+    data = hypfs_get_dyndata();
+    if ( !data )
+        return -ENOENT;
+
+    spin_lock(&cpupool_lock);
+
+    cpupool = __cpupool_find_by_id(data->id, true);
+    if ( cpupool )
+        name = sched_gran_get_name(cpupool->gran);
+
+    spin_unlock(&cpupool_lock);
+
+    if ( !cpupool )
+        return -ENOENT;
+
+    return copy_to_guest(uaddr, name, strlen(name) + 1) ? -EFAULT : 0;
+}
+
+static struct hypfs_funcs cpupool_gran_funcs = {
+    .read = cpupool_gran_read,
+    .getsize = hypfs_getsize,
+};
+
+static HYPFS_VARSIZE_INIT(cpupool_gran, XEN_HYPFS_TYPE_STRING, "sched-gran",
+                          0, &cpupool_gran_funcs);
+static char granstr[SCHED_GRAN_NAME_LEN] = {
+    [0 ... SCHED_GRAN_NAME_LEN - 2] = '?',
+    [SCHED_GRAN_NAME_LEN - 1] = 0
+};
+
 static struct hypfs_funcs cpupool_dir_funcs = {
     .read = cpupool_dir_read,
     .getsize = cpupool_dir_getsize,
@@ -1075,6 +1118,8 @@ static int __init cpupool_init(void)
 
 #ifdef CONFIG_HYPFS
     hypfs_add_dir(&hypfs_root, &cpupool_dir, true);
+    hypfs_string_set_reference(&cpupool_gran, granstr);
+    hypfs_add_leaf(&cpupool_pooldir, &cpupool_gran, true);
 #endif
 
     cpupool0 = cpupool_create(0, 0, &err);
-- 
2.26.2

[PATCH 12/12] xen/cpupool: make per-cpupool sched-gran hypfs node writable

[Juergen Gross]

Make /cpupool/<id>/sched-gran in hypfs writable. This will enable per
cpupool selectable scheduling granularity.

Writing this node is allowed only with no cpu assigned to the cpupool.
Allowed are values "cpu", "core" and "socket".

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 docs/misc/hypfs-paths.pandoc |  5 ++-
 xen/common/sched/cpupool.c   | 75 +++++++++++++++++++++++++++++++-----
 2 files changed, 69 insertions(+), 11 deletions(-)

diff --git a/docs/misc/hypfs-paths.pandoc b/docs/misc/hypfs-paths.pandoc
index f1ce24d7fe..e86f7d0dbe 100644
--- a/docs/misc/hypfs-paths.pandoc
+++ b/docs/misc/hypfs-paths.pandoc
@@ -184,10 +184,13 @@ A directory of all current cpupools.
 The individual cpupools. Each entry is a directory with the name being the
 cpupool-id (e.g. /cpupool/0/).
 
-#### /cpupool/*/sched-gran = ("cpu" | "core" | "socket")
+#### /cpupool/*/sched-gran = ("cpu" | "core" | "socket") [w]
 
 The scheduling granularity of a cpupool.
 
+Writing a value is allowed only for cpupools with no cpu assigned and if the
+architecture is supporting different scheduling granularities.
+
 #### /params/
 
 A directory of runtime parameters.
diff --git a/xen/common/sched/cpupool.c b/xen/common/sched/cpupool.c
index 8674ac0fdd..d0c61fb720 100644
--- a/xen/common/sched/cpupool.c
+++ b/xen/common/sched/cpupool.c
@@ -78,7 +78,7 @@ static void sched_gran_print(enum sched_gran mode, unsigned int gran)
 }
 
 #ifdef CONFIG_HAS_SCHED_GRANULARITY
-static int __init sched_select_granularity(const char *str)
+static int sched_gran_get(const char *str, enum sched_gran *mode)
 {
     unsigned int i;
 
@@ -86,36 +86,43 @@ static int __init sched_select_granularity(const char *str)
     {
         if ( strcmp(sg_name[i].name, str) == 0 )
         {
-            opt_sched_granularity = sg_name[i].mode;
+            *mode = sg_name[i].mode;
             return 0;
         }
     }
 
     return -EINVAL;
 }
+
+static int __init sched_select_granularity(const char *str)
+{
+    return sched_gran_get(str, &opt_sched_granularity);
+}
 custom_param("sched-gran", sched_select_granularity);
+#else
+static int sched_gran_get(const char *str, enum sched_gran *mode)
+{
+    return -EINVAL;
+}
 #endif
 
-static unsigned int __init cpupool_check_granularity(void)
+static unsigned int cpupool_check_granularity(enum sched_gran mode)
 {
     unsigned int cpu;
     unsigned int siblings, gran = 0;
 
-    if ( opt_sched_granularity == SCHED_GRAN_cpu )
+    if ( mode == SCHED_GRAN_cpu )
         return 1;
 
     for_each_online_cpu ( cpu )
     {
-        siblings = cpumask_weight(sched_get_opt_cpumask(opt_sched_granularity,
-                                                        cpu));
+        siblings = cpumask_weight(sched_get_opt_cpumask(mode, cpu));
         if ( gran == 0 )
             gran = siblings;
         else if ( gran != siblings )
             return 0;
     }
 
-    sched_disable_smt_switching = true;
-
     return gran;
 }
 
@@ -127,7 +134,7 @@ static void __init cpupool_gran_init(void)
 
     while ( gran == 0 )
     {
-        gran = cpupool_check_granularity();
+        gran = cpupool_check_granularity(opt_sched_granularity);
 
         if ( gran == 0 )
         {
@@ -153,6 +160,9 @@ static void __init cpupool_gran_init(void)
     if ( fallback )
         warning_add(fallback);
 
+    if ( opt_sched_granularity != SCHED_GRAN_cpu )
+        sched_disable_smt_switching = true;
+
     sched_granularity = gran;
     sched_gran_print(opt_sched_granularity, sched_granularity);
 }
@@ -1088,13 +1098,58 @@ static int cpupool_gran_read(const struct hypfs_entry *entry,
     return copy_to_guest(uaddr, name, strlen(name) + 1) ? -EFAULT : 0;
 }
 
+static int cpupool_gran_write(struct hypfs_entry_leaf *leaf,
+                              XEN_GUEST_HANDLE_PARAM(void) uaddr,
+                              unsigned int ulen)
+{
+    const struct hypfs_dyndir_id *data;
+    struct cpupool *cpupool;
+    enum sched_gran gran;
+    unsigned int sched_gran;
+    char name[SCHED_GRAN_NAME_LEN];
+    int ret = 0;
+
+    if ( ulen > SCHED_GRAN_NAME_LEN )
+        return -ENOSPC;
+
+    if ( copy_from_guest(name, uaddr, ulen) )
+        return -EFAULT;
+
+    sched_gran = sched_gran_get(name, &gran) ? 0
+                                             : cpupool_check_granularity(gran);
+    if ( memchr(name, 0, ulen) != (name + ulen - 1) || sched_gran == 0 )
+        return -EINVAL;
+
+    data = hypfs_get_dyndata();
+    if ( !data )
+        return -ENOENT;
+
+    spin_lock(&cpupool_lock);
+
+    cpupool = __cpupool_find_by_id(data->id, true);
+    if ( !cpupool )
+        ret = -ENOENT;
+    else if ( !cpumask_empty(cpupool->cpu_valid) )
+        ret = -EBUSY;
+    else
+    {
+        cpupool->gran = gran;
+        cpupool->sched_gran = sched_gran;
+    }
+
+    spin_unlock(&cpupool_lock);
+
+    return ret;
+}
+
 static struct hypfs_funcs cpupool_gran_funcs = {
     .read = cpupool_gran_read,
+    .write = cpupool_gran_write,
     .getsize = hypfs_getsize,
 };
 
 static HYPFS_VARSIZE_INIT(cpupool_gran, XEN_HYPFS_TYPE_STRING, "sched-gran",
-                          0, &cpupool_gran_funcs);
+                          SCHED_GRAN_NAME_LEN, &cpupool_gran_funcs);
 static char granstr[SCHED_GRAN_NAME_LEN] = {
     [0 ... SCHED_GRAN_NAME_LEN - 2] = '?',
     [SCHED_GRAN_NAME_LEN - 1] = 0
-- 
2.26.2

Re: [PATCH 05/12] docs: fix hypfs path documentation

[Jan Beulich]

On 26.10.2020 10:13, Juergen Gross wrote:
> The /params/* entry is missing a writable tag.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH 12/12] xen/cpupool: make per-cpupool sched-gran hypfs node writable

[Jan Beulich]

On 26.10.2020 10:13, Juergen Gross wrote:
> @@ -1088,13 +1098,58 @@ static int cpupool_gran_read(const struct hypfs_entry *entry,
>      return copy_to_guest(uaddr, name, strlen(name) + 1) ? -EFAULT : 0;
>  }
>  
> +static int cpupool_gran_write(struct hypfs_entry_leaf *leaf,
> +                              XEN_GUEST_HANDLE_PARAM(void) uaddr,
> +                              unsigned int ulen)
> +{
> +    const struct hypfs_dyndir_id *data;
> +    struct cpupool *cpupool;
> +    enum sched_gran gran;
> +    unsigned int sched_gran;
> +    char name[SCHED_GRAN_NAME_LEN];
> +    int ret = 0;
> +
> +    if ( ulen > SCHED_GRAN_NAME_LEN )
> +        return -ENOSPC;
> +
> +    if ( copy_from_guest(name, uaddr, ulen) )
> +        return -EFAULT;
> +
> +    sched_gran = sched_gran_get(name, &gran) ? 0
> +                                             : cpupool_check_granularity(gran);
> +    if ( memchr(name, 0, ulen) != (name + ulen - 1) || sched_gran == 0 )
> +        return -EINVAL;

I guess the memchr() check wants to happen before the call to
sched_gran_get()?

Jan

Re: [PATCH 12/12] xen/cpupool: make per-cpupool sched-gran hypfs node writable

[Jürgen Groß]

On 29.10.20 15:58, Jan Beulich wrote:
> On 26.10.2020 10:13, Juergen Gross wrote:
>> @@ -1088,13 +1098,58 @@ static int cpupool_gran_read(const struct hypfs_entry *entry,
>>       return copy_to_guest(uaddr, name, strlen(name) + 1) ? -EFAULT : 0;
>>   }
>>   
>> +static int cpupool_gran_write(struct hypfs_entry_leaf *leaf,
>> +                              XEN_GUEST_HANDLE_PARAM(void) uaddr,
>> +                              unsigned int ulen)
>> +{
>> +    const struct hypfs_dyndir_id *data;
>> +    struct cpupool *cpupool;
>> +    enum sched_gran gran;
>> +    unsigned int sched_gran;
>> +    char name[SCHED_GRAN_NAME_LEN];
>> +    int ret = 0;
>> +
>> +    if ( ulen > SCHED_GRAN_NAME_LEN )
>> +        return -ENOSPC;
>> +
>> +    if ( copy_from_guest(name, uaddr, ulen) )
>> +        return -EFAULT;
>> +
>> +    sched_gran = sched_gran_get(name, &gran) ? 0
>> +                                             : cpupool_check_granularity(gran);
>> +    if ( memchr(name, 0, ulen) != (name + ulen - 1) || sched_gran == 0 )
>> +        return -EINVAL;
> 
> I guess the memchr() check wants to happen before the call to
> sched_gran_get()?

Yes.


Juergen

Re: [PATCH 01/12] xen/cpupool: add cpu to sched_res_mask when removing it from cpupool

[Dario Faggioli]

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

On Mon, 2020-10-26 at 10:13 +0100, Juergen Gross wrote:
> When a cpu is removed from a cpupool and added to the free cpus it
> should be added to sched_res_mask, too.
> 
> The related removal from sched_res_mask in case of core scheduling
> is already done in schedule_cpu_add().
> 
> As long as all cpupools share the same scheduling granularity there
> is nothing going wrong with the missing removal, 
>
This patch is adding an addition of the CPU to sched_res_mask, which
was missing... So isn't the above "there is nothing going wrong with
the missing addition", or something like that?

Or, if it's an actual missing removal that we are referring to here,
then it must be clarified which one.

> but this will change
> when per-cpupool granularity is fully supported.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
>
With the above fixed or clarified:

Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 02/12] xen/cpupool: add missing bits for per-cpupool scheduling granularity

[Dario Faggioli]

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

On Mon, 2020-10-26 at 10:13 +0100, Juergen Gross wrote:
> Even with storing the scheduling granularity in struct cpupool there
> are still a few bits missing for being able to have cpupools with
> different granularity (apart from the missing interface for setting
> the individual granularities): the number of cpus in a scheduling
> unit is always taken from the global sched_granularity variable.
> 
> So store the value in struct cpupool and use that instead of
> sched_granularity.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 04/12] xen/sched: sort included headers in cpupool.c

[Dario Faggioli]

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

On Mon, 2020-10-26 at 10:13 +0100, Juergen Gross wrote:
> Common style is to include header files in alphabetical order. Sort
> the
> #include statements in cpupool.c accordingly.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
>
Acked-by: Dario Faggioli <dfaggioli@suse.com>

Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 01/12] xen/cpupool: add cpu to sched_res_mask when removing it from cpupool

[Jürgen Groß]

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

On 11.11.20 15:32, Dario Faggioli wrote:
> On Mon, 2020-10-26 at 10:13 +0100, Juergen Gross wrote:
>> When a cpu is removed from a cpupool and added to the free cpus it
>> should be added to sched_res_mask, too.
>>
>> The related removal from sched_res_mask in case of core scheduling
>> is already done in schedule_cpu_add().
>>
>> As long as all cpupools share the same scheduling granularity there
>> is nothing going wrong with the missing removal,
>>
> This patch is adding an addition of the CPU to sched_res_mask, which
> was missing... So isn't the above "there is nothing going wrong with
> the missing addition", or something like that?

Oh yes, of course.

Will fix that.

> 
> Or, if it's an actual missing removal that we are referring to here,
> then it must be clarified which one.
> 
>> but this will change
>> when per-cpupool granularity is fully supported.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>
> With the above fixed or clarified:
> 
> Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

Thanks,


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

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

Re: [PATCH 10/12] xen/hypfs: add cpupool directories

[Dario Faggioli]

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

On Mon, 2020-10-26 at 10:13 +0100, Juergen Gross wrote:
> Add /cpupool/<cpupool-id> directories to hypfs. Those are completely
> dynamic, so the related hypfs access functions need to be
> implemented.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
>
So, I'm almost sold... Just one comment:

> --- a/xen/common/sched/cpupool.c
> +++ b/xen/common/sched/cpupool.c
> @@ -999,6 +1073,10 @@ static int __init cpupool_init(void)
>  
>      cpupool_gran_init();
>  
> +#ifdef CONFIG_HYPFS
> +    hypfs_add_dir(&hypfs_root, &cpupool_dir, true);
> +#endif
> +
What would you think about doing this in an helper function
(hypfs_cpupool_init() ?), implemented inside the above #ifdef and as an
empty stub if !CONFIG_HYPFS ?

That will save us from having the #ifdef-s again here.

I'm asking because it's certainly not critical and I don't have a too
strong opinion about it. But I do think the code would look better.

>      cpupool0 = cpupool_create(0, 0, &err);
>      BUG_ON(cpupool0 == NULL);
>      cpupool_put(cpupool0);

-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 10/12] xen/hypfs: add cpupool directories

[Jan Beulich]

On 11.11.2020 15:51, Dario Faggioli wrote:
> On Mon, 2020-10-26 at 10:13 +0100, Juergen Gross wrote:
>> Add /cpupool/<cpupool-id> directories to hypfs. Those are completely
>> dynamic, so the related hypfs access functions need to be
>> implemented.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>
> So, I'm almost sold... Just one comment:
> 
>> --- a/xen/common/sched/cpupool.c
>> +++ b/xen/common/sched/cpupool.c
>> @@ -999,6 +1073,10 @@ static int __init cpupool_init(void)
>>  
>>      cpupool_gran_init();
>>  
>> +#ifdef CONFIG_HYPFS
>> +    hypfs_add_dir(&hypfs_root, &cpupool_dir, true);
>> +#endif
>> +
> What would you think about doing this in an helper function
> (hypfs_cpupool_init() ?), implemented inside the above #ifdef and as an
> empty stub if !CONFIG_HYPFS ?
> 
> That will save us from having the #ifdef-s again here.

Having a hypfs_add_dir() stub would also allow to achieve this, and
then, going forward, perhaps also elsewhere.

Jan

Re: [PATCH 10/12] xen/hypfs: add cpupool directories

[Jürgen Groß]

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

On 11.11.20 15:51, Dario Faggioli wrote:
> On Mon, 2020-10-26 at 10:13 +0100, Juergen Gross wrote:
>> Add /cpupool/<cpupool-id> directories to hypfs. Those are completely
>> dynamic, so the related hypfs access functions need to be
>> implemented.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>
> So, I'm almost sold... Just one comment:
> 
>> --- a/xen/common/sched/cpupool.c
>> +++ b/xen/common/sched/cpupool.c
>> @@ -999,6 +1073,10 @@ static int __init cpupool_init(void)
>>   
>>       cpupool_gran_init();
>>   
>> +#ifdef CONFIG_HYPFS
>> +    hypfs_add_dir(&hypfs_root, &cpupool_dir, true);
>> +#endif
>> +
> What would you think about doing this in an helper function
> (hypfs_cpupool_init() ?), implemented inside the above #ifdef and as an
> empty stub if !CONFIG_HYPFS ?
> 
> That will save us from having the #ifdef-s again here.
> 
> I'm asking because it's certainly not critical and I don't have a too
> strong opinion about it. But I do think the code would look better.

I'm fine either way.

In case nobody objects I'll change it.


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

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

Re: [PATCH 10/12] xen/hypfs: add cpupool directories

[Dario Faggioli]

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

On Wed, 2020-11-11 at 15:56 +0100, Jürgen Groß wrote:
> On 11.11.20 15:51, Dario Faggioli wrote:
> > 
> > What would you think about doing this in an helper function
> > (hypfs_cpupool_init() ?), implemented inside the above #ifdef and
> > as an
> > empty stub if !CONFIG_HYPFS ?
> > 
> > That will save us from having the #ifdef-s again here.
> > 
> > I'm asking because it's certainly not critical and I don't have a
> > too
> > strong opinion about it. But I do think the code would look better.
> 
> I'm fine either way.
> 
> In case nobody objects I'll change it.
> 
Ok, cool. If you do that and resend, and that's the only change, you
can add:

Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

Thanks and Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 10/12] xen/hypfs: add cpupool directories

[Jürgen Groß]

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

On 11.11.20 15:56, Jan Beulich wrote:
> On 11.11.2020 15:51, Dario Faggioli wrote:
>> On Mon, 2020-10-26 at 10:13 +0100, Juergen Gross wrote:
>>> Add /cpupool/<cpupool-id> directories to hypfs. Those are completely
>>> dynamic, so the related hypfs access functions need to be
>>> implemented.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>
>> So, I'm almost sold... Just one comment:
>>
>>> --- a/xen/common/sched/cpupool.c
>>> +++ b/xen/common/sched/cpupool.c
>>> @@ -999,6 +1073,10 @@ static int __init cpupool_init(void)
>>>   
>>>       cpupool_gran_init();
>>>   
>>> +#ifdef CONFIG_HYPFS
>>> +    hypfs_add_dir(&hypfs_root, &cpupool_dir, true);
>>> +#endif
>>> +
>> What would you think about doing this in an helper function
>> (hypfs_cpupool_init() ?), implemented inside the above #ifdef and as an
>> empty stub if !CONFIG_HYPFS ?
>>
>> That will save us from having the #ifdef-s again here.
> 
> Having a hypfs_add_dir() stub would also allow to achieve this, and
> then, going forward, perhaps also elsewhere.

I thought about that. This would require to be macro for the stub case,
but I don't think this is a major problem.

Currently there are no other places requiring a stub, but in future this
might change.


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

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

Re: [PATCH 10/12] xen/hypfs: add cpupool directories

[Dario Faggioli]

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

On Wed, 2020-11-11 at 16:00 +0100, Jürgen Groß wrote:
> On 11.11.20 15:56, Jan Beulich wrote:
> > On 11.11.2020 15:51, Dario Faggioli wrote:
> > 
> > 
> > Having a hypfs_add_dir() stub would also allow to achieve this, and
> > then, going forward, perhaps also elsewhere.
> 
> I thought about that. This would require to be macro for the stub
> case,
> but I don't think this is a major problem.
> 
Yes, I thought about "stub-bing" at the hypfs_add_dir() level, but we
need a macro for that, for dealing with the parameters.

Also, it's not like having that stub would prevent having #ifdef-s at
all in this file. So that's why I thought about a local stub.

But sure, if you go for the generic macro stub, I'm fine with that too.

Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 11/12] xen/hypfs: add scheduling granularity entry to cpupool entries

[Dario Faggioli]

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

On Mon, 2020-10-26 at 10:13 +0100, Juergen Gross wrote:
> Add a "sched-gran" entry to the per-cpupool hypfs directories.
> 
> For now make this entry read-only and let it contain one of the
> strings "cpu", "core" or "socket".
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
>
Acked-by: Dario Faggioli <dfaggioli@suse.com>

Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 06/12] xen/hypfs: move per-node function pointers into a dedicated struct

[Jan Beulich]

On 26.10.2020 10:13, Juergen Gross wrote:
> @@ -15,10 +29,7 @@ struct hypfs_entry {
>      unsigned int max_size;
>      const char *name;
>      struct list_head list;
> -    int (*read)(const struct hypfs_entry *entry,
> -                XEN_GUEST_HANDLE_PARAM(void) uaddr);
> -    int (*write)(struct hypfs_entry_leaf *leaf,
> -                 XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned int ulen);
> +    struct hypfs_funcs *funcs;

const (with all the cascade changes necessary)?

Jan

Re: [PATCH 07/12] xen/hypfs: pass real failure reason up from hypfs_get_entry()

[Jan Beulich]

On 26.10.2020 10:13, Juergen Gross wrote:
> Instead of handling all errors from hypfs_get_entry() as ENOENT pass
> up the real error value via ERR_PTR().
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH 08/12] xen/hypfs: support dynamic hypfs nodes

[Jan Beulich]

On 26.10.2020 10:13, Juergen Gross wrote:
> Add a getsize() function pointer to struct hypfs_funcs for being able
> to have dynamically filled entries without the need to take the hypfs
> lock each time the contents are being generated.

But a dynamic update causing a change in size will require _some_
lock, won't it?

> --- a/xen/common/hypfs.c
> +++ b/xen/common/hypfs.c
> @@ -19,28 +19,29 @@
>  CHECK_hypfs_dirlistentry;
>  #endif
>  
> -#define DIRENTRY_NAME_OFF offsetof(struct xen_hypfs_dirlistentry, name)
> -#define DIRENTRY_SIZE(name_len) \
> -    (DIRENTRY_NAME_OFF +        \
> -     ROUNDUP((name_len) + 1, alignof(struct xen_hypfs_direntry)))
> -
>  struct hypfs_funcs hypfs_dir_funcs = {
>      .read = hypfs_read_dir,
> +    .getsize = hypfs_getsize,
> +    .findentry = hypfs_dir_findentry,
>  };
>  struct hypfs_funcs hypfs_leaf_ro_funcs = {
>      .read = hypfs_read_leaf,
> +    .getsize = hypfs_getsize,
>  };
>  struct hypfs_funcs hypfs_leaf_wr_funcs = {
>      .read = hypfs_read_leaf,
>      .write = hypfs_write_leaf,
> +    .getsize = hypfs_getsize,
>  };
>  struct hypfs_funcs hypfs_bool_wr_funcs = {
>      .read = hypfs_read_leaf,
>      .write = hypfs_write_bool,
> +    .getsize = hypfs_getsize,
>  };
>  struct hypfs_funcs hypfs_custom_wr_funcs = {
>      .read = hypfs_read_leaf,
>      .write = hypfs_write_custom,
> +    .getsize = hypfs_getsize,
>  };

With the increasing number of fields that may (deliberately or
by mistake) be NULL, should we gain some form of proactive
guarding against calls through such pointers?

> @@ -88,6 +93,23 @@ static void hypfs_unlock(void)
>      }
>  }
>  
> +void *hypfs_alloc_dyndata(unsigned long size, unsigned long align)

Will callers really need to specify (high) alignment values? IOW ...

> +{
> +    unsigned int cpu = smp_processor_id();
> +
> +    ASSERT(per_cpu(hypfs_locked, cpu) != hypfs_unlocked);
> +    ASSERT(per_cpu(hypfs_dyndata, cpu) == NULL);
> +
> +    per_cpu(hypfs_dyndata, cpu) = _xzalloc(size, align);

... is xzalloc_bytes() not suitable for use here?

> @@ -171,15 +193,34 @@ static int hypfs_get_path_user(char *buf,
>      return 0;
>  }
>  
> +struct hypfs_entry *hypfs_dir_findentry(struct hypfs_entry_dir *dir,
> +                                        const char *name,
> +                                        unsigned int name_len)
> +{
> +    struct hypfs_entry *entry;
> +
> +    list_for_each_entry ( entry, &dir->dirlist, list )
> +    {
> +        int cmp = strncmp(name, entry->name, name_len);
> +
> +        if ( cmp < 0 )
> +            return ERR_PTR(-ENOENT);
> +
> +        if ( !cmp && strlen(entry->name) == name_len )
> +            return entry;
> +    }
> +
> +    return ERR_PTR(-ENOENT);
> +}
> +
>  static struct hypfs_entry *hypfs_get_entry_rel(struct hypfs_entry_dir *dir,
>                                                 const char *path)
>  {
>      const char *end;
>      struct hypfs_entry *entry;
>      unsigned int name_len;
> -    bool again = true;
>  
> -    while ( again )
> +    for ( ;; )

Nit: Strictly speaking another blank is needed between the two
semicolons.

> @@ -275,22 +305,25 @@ int hypfs_read_leaf(const struct hypfs_entry *entry,
>  
>      l = container_of(entry, const struct hypfs_entry_leaf, e);
>  
> -    return copy_to_guest(uaddr, l->u.content, entry->size) ? -EFAULT: 0;
> +    return copy_to_guest(uaddr, l->u.content, entry->funcs->getsize(entry)) ?
> +                                              -EFAULT : 0;

With the intended avoiding of locking, how is this ->getsize()
guaranteed to not ...

> @@ -298,7 +331,7 @@ static int hypfs_read(const struct hypfs_entry *entry,
>          goto out;
>  
>      ret = -ENOBUFS;
> -    if ( ulen < entry->size + sizeof(e) )
> +    if ( ulen < size + sizeof(e) )
>          goto out;

... invalidate the checking done here? (A similar risk looks to
exist on the write path, albeit there we have at least the
->max_size checks, where I hope that field isn't mean to become
dynamic as well.)

Jan

Re: [PATCH 09/12] xen/hypfs: add support for id-based dynamic directories

[Jan Beulich]

On 26.10.2020 10:13, Juergen Gross wrote:
> --- a/xen/common/hypfs.c
> +++ b/xen/common/hypfs.c
> @@ -257,6 +257,82 @@ unsigned int hypfs_getsize(const struct hypfs_entry *entry)
>      return entry->size;
>  }
>  
> +int hypfs_read_dyndir_id_entry(struct hypfs_entry_dir *template,
> +                               unsigned int id, bool is_last,
> +                               XEN_GUEST_HANDLE_PARAM(void) *uaddr)
> +{
> +    struct xen_hypfs_dirlistentry direntry;
> +    char name[12];

Perhaps better tie this literal 12 to the one used for declaring
struct hypfs_dyndir_id's name[] field, such that an eventual
change will need making in exactly one place?

> +    unsigned int e_namelen, e_len;
> +
> +    e_namelen = snprintf(name, sizeof(name), "%u", id);
> +    e_len = HYPFS_DIRENTRY_SIZE(e_namelen);
> +    direntry.e.pad = 0;
> +    direntry.e.type = template->e.type;
> +    direntry.e.encoding = template->e.encoding;
> +    direntry.e.content_len = template->e.funcs->getsize(&template->e);
> +    direntry.e.max_write_len = template->e.max_size;
> +    direntry.off_next = is_last ? 0 : e_len;
> +    if ( copy_to_guest(*uaddr, &direntry, 1) )
> +        return -EFAULT;
> +    if ( copy_to_guest_offset(*uaddr, HYPFS_DIRENTRY_NAME_OFF, name,
> +                              e_namelen + 1) )
> +        return -EFAULT;
> +
> +    guest_handle_add_offset(*uaddr, e_len);
> +
> +    return 0;
> +}
> +
> +static struct hypfs_entry *hypfs_dyndir_findentry(struct hypfs_entry_dir *dir,
> +                                                  const char *name,
> +                                                  unsigned int name_len)
> +{
> +    struct hypfs_dyndir_id *data;

const? (also in read_dyndir below)

> +    data = hypfs_get_dyndata();
> +    if ( !data )
> +        return ERR_PTR(-ENOENT);
> +
> +    /* Use template with original findentry function. */
> +    return data->template->e.funcs->findentry(data->template, name, name_len);

Why does this pass the address of the template? If it truly is
(just) a template, then its dirlist ought to be empty at all
times? If otoh the "template" indeed gets used as a node in the
tree then perhaps it wants naming differently? "Stem" would come
to mind, but likely there are better alternatives. I've also
considered the German "Statthalter", but its English translations
don't seem reasonable to me here. And "placeholder" has kind of a
negative touch. (Also in this case some of my "const?" remarks
may be irrelevant.)

Further this and ...

> +static int hypfs_read_dyndir(const struct hypfs_entry *entry,
> +                             XEN_GUEST_HANDLE_PARAM(void) uaddr)
> +{
> +    struct hypfs_dyndir_id *data;
> +
> +    data = hypfs_get_dyndata();
> +    if ( !data )
> +        return -ENOENT;
> +
> +    /* Use template with original read function. */
> +    return data->template->e.funcs->read(&data->template->e, uaddr);

... this using the template's funcs is somewhat unexpected, but
with the functions acting as the entry's .findentry() / .read()
hooks is obviously the right thing (and if the template is more
that what the word says, the consideration may become
inapplicable anyway). The implication is that the hooks
themselves can't be replaced, if need be down the road.

> +struct hypfs_entry *hypfs_gen_dyndir_entry_id(struct hypfs_entry_dir *template,
> +                                              unsigned int id)
> +{
> +    struct hypfs_dyndir_id *data;
> +
> +    data = hypfs_alloc_dyndata(sizeof(*data), alignof(*data));
> +    if ( !data )
> +        return ERR_PTR(-ENOMEM);
> +
> +    data->template = template;
> +    data->id = id;

I can't seem to spot any consumer of this field: Is it really
needed?

> --- a/xen/include/xen/hypfs.h
> +++ b/xen/include/xen/hypfs.h
> @@ -50,6 +50,15 @@ struct hypfs_entry_dir {
>      struct list_head dirlist;
>  };
>  
> +struct hypfs_dyndir_id {
> +    struct hypfs_entry_dir dir;       /* Modified copy of template. */
> +    struct hypfs_funcs funcs;         /* Dynamic functions. */
> +    struct hypfs_entry_dir *template; /* Template used. */

const?

> @@ -150,6 +159,11 @@ struct hypfs_entry *hypfs_dir_findentry(struct hypfs_entry_dir *dir,
>                                          unsigned int name_len);
>  void *hypfs_alloc_dyndata(unsigned long size, unsigned long align);
>  void *hypfs_get_dyndata(void);
> +int hypfs_read_dyndir_id_entry(struct hypfs_entry_dir *template,

const?

> +                               unsigned int id, bool is_last,
> +                               XEN_GUEST_HANDLE_PARAM(void) *uaddr);
> +struct hypfs_entry *hypfs_gen_dyndir_entry_id(struct hypfs_entry_dir *template,

const?

Jan

Re: [PATCH 10/12] xen/hypfs: add cpupool directories

[Jan Beulich]

On 26.10.2020 10:13, Juergen Gross wrote:
> @@ -992,6 +994,78 @@ static struct notifier_block cpu_nfb = {
>      .notifier_call = cpu_callback
>  };
>  
> +#ifdef CONFIG_HYPFS
> +static HYPFS_DIR_INIT(cpupool_pooldir, "id");

This "id" string won't appear anywhere, will it? I would have
expected this to act as the format string used when generating
names of the dynamic entries. This would e.g. allow CPU pools
to have decimal numbered names, but other entries also hex
ones, and then if so desired also e.g. with leading zeros.

> +static int cpupool_dir_read(const struct hypfs_entry *entry,
> +                            XEN_GUEST_HANDLE_PARAM(void) uaddr)
> +{
> +    int ret = 0;
> +    struct cpupool **q;

I was going to ask for const here, but the way for_each_cpupool()
works looks to prohibit this. Nevertheless I wonder whether the
extra level of indirection there wouldn't better be dropped. Of
the users, only cpupool_destroy() looks to need it, so open-
coding the loop there (or introducing an auxiliary variable)
would allow improvements here and elsewhere. (Actually I notice
there's also a similar use in cpupool_create(), but the general
consideration remains.)

> +    spin_lock(&cpupool_lock);
> +
> +    for_each_cpupool(q)
> +    {
> +        ret = hypfs_read_dyndir_id_entry(&cpupool_pooldir, (*q)->cpupool_id,
> +                                         !(*q)->next, &uaddr);
> +        if ( ret )
> +            break;
> +    }
> +
> +    spin_unlock(&cpupool_lock);
> +
> +    return ret;
> +}
> +
> +static unsigned int cpupool_dir_getsize(const struct hypfs_entry *entry)
> +{
> +    struct cpupool **q;
> +    unsigned int size = 0;
> +
> +    spin_lock(&cpupool_lock);
> +
> +    for_each_cpupool(q)
> +        size += HYPFS_DIRENTRY_SIZE(snprintf(NULL, 0, "%d", (*q)->cpupool_id));

Beyond the remark above I consider this problematic: If the pool
ID was negative, the use of %d here would get things out of sync
with the %u uses in hypfs.c. I guess exposing
HYPFS_DIRENTRY_SIZE() isn't the right approach, and you instead
need another hypfs library function.

> +static struct hypfs_entry *cpupool_dir_findentry(struct hypfs_entry_dir *dir,
> +                                                 const char *name,
> +                                                 unsigned int name_len)
> +{
> +    unsigned long id;
> +    const char *end;
> +    struct cpupool *cpupool;
> +
> +    id = simple_strtoul(name, &end, 10);
> +    if ( id > INT_MAX || end != name + name_len )

What does this INT_MAX match up with? Afaics
XEN_SYSCTL_CPUPOOL_OP_CREATE is fine to have an effectively
negative pool ID passed in (the public interface struct uses
uint32_t, but this gets converted to plain int first thing in
the sysctl handler).

> +        return ERR_PTR(-ENOENT);
> +
> +    spin_lock(&cpupool_lock);
> +
> +    cpupool = __cpupool_find_by_id(id, true);
> +
> +    spin_unlock(&cpupool_lock);
> +
> +    if ( !cpupool )
> +        return ERR_PTR(-ENOENT);
> +
> +    return hypfs_gen_dyndir_entry_id(&cpupool_pooldir, id);

The latest this one makes clear that cpupool_lock nests inside
the hypfs one. I think this wants spelling out next to the
definition of the former, as it implies that there are
restrictions on what can be done from inside cpupool-locked
regions. hypfs_read_dyndir_id_entry(), for example, has to
remain lock free for this reason.

Jan

Re: [PATCH 06/12] xen/hypfs: move per-node function pointers into a dedicated struct

[Jürgen Groß]

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

On 17.11.20 12:18, Jan Beulich wrote:
> On 26.10.2020 10:13, Juergen Gross wrote:
>> @@ -15,10 +29,7 @@ struct hypfs_entry {
>>       unsigned int max_size;
>>       const char *name;
>>       struct list_head list;
>> -    int (*read)(const struct hypfs_entry *entry,
>> -                XEN_GUEST_HANDLE_PARAM(void) uaddr);
>> -    int (*write)(struct hypfs_entry_leaf *leaf,
>> -                 XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned int ulen);
>> +    struct hypfs_funcs *funcs;
> 
> const (with all the cascade changes necessary)?

Yes, probably possible.


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

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

[PATCH 08/12] xen/hypfs: support dynamic hypfs nodes

[Jürgen Groß]

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

On 17.11.20 13:37, Jan Beulich wrote:
> On 26.10.2020 10:13, Juergen Gross wrote:
>> Add a getsize() function pointer to struct hypfs_funcs for being able
>> to have dynamically filled entries without the need to take the hypfs
>> lock each time the contents are being generated.
> 
> But a dynamic update causing a change in size will require _some_
> lock, won't it?

Yes, of course.

e.g. the getsize() function returning the size of a directory holding an
entry for each cpupool will need to take the cpupool lock in order to
avoid a cpupool being created or deleted in parallel.

But the cpupool create/destroy functions don't need to take the hypfs
lock.

> 
>> --- a/xen/common/hypfs.c
>> +++ b/xen/common/hypfs.c
>> @@ -19,28 +19,29 @@
>>   CHECK_hypfs_dirlistentry;
>>   #endif
>>   
>> -#define DIRENTRY_NAME_OFF offsetof(struct xen_hypfs_dirlistentry, name)
>> -#define DIRENTRY_SIZE(name_len) \
>> -    (DIRENTRY_NAME_OFF +        \
>> -     ROUNDUP((name_len) + 1, alignof(struct xen_hypfs_direntry)))
>> -
>>   struct hypfs_funcs hypfs_dir_funcs = {
>>       .read = hypfs_read_dir,
>> +    .getsize = hypfs_getsize,
>> +    .findentry = hypfs_dir_findentry,
>>   };
>>   struct hypfs_funcs hypfs_leaf_ro_funcs = {
>>       .read = hypfs_read_leaf,
>> +    .getsize = hypfs_getsize,
>>   };
>>   struct hypfs_funcs hypfs_leaf_wr_funcs = {
>>       .read = hypfs_read_leaf,
>>       .write = hypfs_write_leaf,
>> +    .getsize = hypfs_getsize,
>>   };
>>   struct hypfs_funcs hypfs_bool_wr_funcs = {
>>       .read = hypfs_read_leaf,
>>       .write = hypfs_write_bool,
>> +    .getsize = hypfs_getsize,
>>   };
>>   struct hypfs_funcs hypfs_custom_wr_funcs = {
>>       .read = hypfs_read_leaf,
>>       .write = hypfs_write_custom,
>> +    .getsize = hypfs_getsize,
>>   };
> 
> With the increasing number of fields that may (deliberately or
> by mistake) be NULL, should we gain some form of proactive
> guarding against calls through such pointers?

Hmm, up to now I think such a bug would be detected rather fast.

I can add some ASSERT()s for mandatory functions not being NULL when
a node is added dynamically or during hypfs initialization for the
static nodes.

> 
>> @@ -88,6 +93,23 @@ static void hypfs_unlock(void)
>>       }
>>   }
>>   
>> +void *hypfs_alloc_dyndata(unsigned long size, unsigned long align)
> 
> Will callers really need to specify (high) alignment values? IOW ...
> 
>> +{
>> +    unsigned int cpu = smp_processor_id();
>> +
>> +    ASSERT(per_cpu(hypfs_locked, cpu) != hypfs_unlocked);
>> +    ASSERT(per_cpu(hypfs_dyndata, cpu) == NULL);
>> +
>> +    per_cpu(hypfs_dyndata, cpu) = _xzalloc(size, align);
> 
> ... is xzalloc_bytes() not suitable for use here?

Good question.

Up to now I think we could get away without specific alignment.

I can drop that parameter for now if you'd like that better.

> 
>> @@ -171,15 +193,34 @@ static int hypfs_get_path_user(char *buf,
>>       return 0;
>>   }
>>   
>> +struct hypfs_entry *hypfs_dir_findentry(struct hypfs_entry_dir *dir,
>> +                                        const char *name,
>> +                                        unsigned int name_len)
>> +{
>> +    struct hypfs_entry *entry;
>> +
>> +    list_for_each_entry ( entry, &dir->dirlist, list )
>> +    {
>> +        int cmp = strncmp(name, entry->name, name_len);
>> +
>> +        if ( cmp < 0 )
>> +            return ERR_PTR(-ENOENT);
>> +
>> +        if ( !cmp && strlen(entry->name) == name_len )
>> +            return entry;
>> +    }
>> +
>> +    return ERR_PTR(-ENOENT);
>> +}
>> +
>>   static struct hypfs_entry *hypfs_get_entry_rel(struct hypfs_entry_dir *dir,
>>                                                  const char *path)
>>   {
>>       const char *end;
>>       struct hypfs_entry *entry;
>>       unsigned int name_len;
>> -    bool again = true;
>>   
>> -    while ( again )
>> +    for ( ;; )
> 
> Nit: Strictly speaking another blank is needed between the two
> semicolons.

Okay.

> 
>> @@ -275,22 +305,25 @@ int hypfs_read_leaf(const struct hypfs_entry *entry,
>>   
>>       l = container_of(entry, const struct hypfs_entry_leaf, e);
>>   
>> -    return copy_to_guest(uaddr, l->u.content, entry->size) ? -EFAULT: 0;
>> +    return copy_to_guest(uaddr, l->u.content, entry->funcs->getsize(entry)) ?
>> +                                              -EFAULT : 0;
> 
> With the intended avoiding of locking, how is this ->getsize()
> guaranteed to not ...
> 
>> @@ -298,7 +331,7 @@ static int hypfs_read(const struct hypfs_entry *entry,
>>           goto out;
>>   
>>       ret = -ENOBUFS;
>> -    if ( ulen < entry->size + sizeof(e) )
>> +    if ( ulen < size + sizeof(e) )
>>           goto out;
> 
> ... invalidate the checking done here? (A similar risk looks to
> exist on the write path, albeit there we have at least the
> ->max_size checks, where I hope that field isn't mean to become
> dynamic as well.)

I think you are right. I should add the size value as a parameter to the
read and write functions.

And no, max_size should not be dynamic.


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

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

Re: [PATCH 09/12] xen/hypfs: add support for id-based dynamic directories

[Jürgen Groß]

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

On 17.11.20 14:33, Jan Beulich wrote:
> On 26.10.2020 10:13, Juergen Gross wrote:
>> --- a/xen/common/hypfs.c
>> +++ b/xen/common/hypfs.c
>> @@ -257,6 +257,82 @@ unsigned int hypfs_getsize(const struct hypfs_entry *entry)
>>       return entry->size;
>>   }
>>   
>> +int hypfs_read_dyndir_id_entry(struct hypfs_entry_dir *template,
>> +                               unsigned int id, bool is_last,
>> +                               XEN_GUEST_HANDLE_PARAM(void) *uaddr)
>> +{
>> +    struct xen_hypfs_dirlistentry direntry;
>> +    char name[12];
> 
> Perhaps better tie this literal 12 to the one used for declaring
> struct hypfs_dyndir_id's name[] field, such that an eventual
> change will need making in exactly one place?

Yes.

> 
>> +    unsigned int e_namelen, e_len;
>> +
>> +    e_namelen = snprintf(name, sizeof(name), "%u", id);
>> +    e_len = HYPFS_DIRENTRY_SIZE(e_namelen);
>> +    direntry.e.pad = 0;
>> +    direntry.e.type = template->e.type;
>> +    direntry.e.encoding = template->e.encoding;
>> +    direntry.e.content_len = template->e.funcs->getsize(&template->e);
>> +    direntry.e.max_write_len = template->e.max_size;
>> +    direntry.off_next = is_last ? 0 : e_len;
>> +    if ( copy_to_guest(*uaddr, &direntry, 1) )
>> +        return -EFAULT;
>> +    if ( copy_to_guest_offset(*uaddr, HYPFS_DIRENTRY_NAME_OFF, name,
>> +                              e_namelen + 1) )
>> +        return -EFAULT;
>> +
>> +    guest_handle_add_offset(*uaddr, e_len);
>> +
>> +    return 0;
>> +}
>> +
>> +static struct hypfs_entry *hypfs_dyndir_findentry(struct hypfs_entry_dir *dir,
>> +                                                  const char *name,
>> +                                                  unsigned int name_len)
>> +{
>> +    struct hypfs_dyndir_id *data;
> 
> const? (also in read_dyndir below)

Okay.

> 
>> +    data = hypfs_get_dyndata();
>> +    if ( !data )
>> +        return ERR_PTR(-ENOENT);
>> +
>> +    /* Use template with original findentry function. */
>> +    return data->template->e.funcs->findentry(data->template, name, name_len);
> 
> Why does this pass the address of the template? If it truly is
> (just) a template, then its dirlist ought to be empty at all
> times? If otoh the "template" indeed gets used as a node in the
> tree then perhaps it wants naming differently? "Stem" would come
> to mind, but likely there are better alternatives. I've also
> considered the German "Statthalter", but its English translations
> don't seem reasonable to me here. And "placeholder" has kind of a
> negative touch. (Also in this case some of my "const?" remarks
> may be irrelevant.)

It is basically a template tree.

In the current use case (cpupool/<id>/sched-gran) the template is
<id> with the leaf "sched-gran" which is the template for the per
cpupool incarnation.

If you like it better I can use stem.

> 
> Further this and ...
> 
>> +static int hypfs_read_dyndir(const struct hypfs_entry *entry,
>> +                             XEN_GUEST_HANDLE_PARAM(void) uaddr)
>> +{
>> +    struct hypfs_dyndir_id *data;
>> +
>> +    data = hypfs_get_dyndata();
>> +    if ( !data )
>> +        return -ENOENT;
>> +
>> +    /* Use template with original read function. */
>> +    return data->template->e.funcs->read(&data->template->e, uaddr);
> 
> ... this using the template's funcs is somewhat unexpected, but
> with the functions acting as the entry's .findentry() / .read()
> hooks is obviously the right thing (and if the template is more
> that what the word says, the consideration may become
> inapplicable anyway). The implication is that the hooks
> themselves can't be replaced, if need be down the road.

Correct. In case this is needed the related node must be really
completely dynamical instead.

> 
>> +struct hypfs_entry *hypfs_gen_dyndir_entry_id(struct hypfs_entry_dir *template,
>> +                                              unsigned int id)
>> +{
>> +    struct hypfs_dyndir_id *data;
>> +
>> +    data = hypfs_alloc_dyndata(sizeof(*data), alignof(*data));
>> +    if ( !data )
>> +        return ERR_PTR(-ENOMEM);
>> +
>> +    data->template = template;
>> +    data->id = id;
> 
> I can't seem to spot any consumer of this field: Is it really
> needed?

Yes. It will be used by the specific read/write functions, e.g.
cpupool_gran_read().

> 
>> --- a/xen/include/xen/hypfs.h
>> +++ b/xen/include/xen/hypfs.h
>> @@ -50,6 +50,15 @@ struct hypfs_entry_dir {
>>       struct list_head dirlist;
>>   };
>>   
>> +struct hypfs_dyndir_id {
>> +    struct hypfs_entry_dir dir;       /* Modified copy of template. */
>> +    struct hypfs_funcs funcs;         /* Dynamic functions. */
>> +    struct hypfs_entry_dir *template; /* Template used. */
> 
> const?

Yes.

> 
>> @@ -150,6 +159,11 @@ struct hypfs_entry *hypfs_dir_findentry(struct hypfs_entry_dir *dir,
>>                                           unsigned int name_len);
>>   void *hypfs_alloc_dyndata(unsigned long size, unsigned long align);
>>   void *hypfs_get_dyndata(void);
>> +int hypfs_read_dyndir_id_entry(struct hypfs_entry_dir *template,
> 
> const?

Yes.

> 
>> +                               unsigned int id, bool is_last,
>> +                               XEN_GUEST_HANDLE_PARAM(void) *uaddr);
>> +struct hypfs_entry *hypfs_gen_dyndir_entry_id(struct hypfs_entry_dir *template,
> 
> const?

Yes.


Juergen



[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

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

Re: [PATCH 08/12] xen/hypfs: support dynamic hypfs nodes

[Jan Beulich]

On 17.11.2020 15:29, Jürgen Groß wrote:
> On 17.11.20 13:37, Jan Beulich wrote:
>> On 26.10.2020 10:13, Juergen Gross wrote:
>>> --- a/xen/common/hypfs.c
>>> +++ b/xen/common/hypfs.c
>>> @@ -19,28 +19,29 @@
>>>   CHECK_hypfs_dirlistentry;
>>>   #endif
>>>   
>>> -#define DIRENTRY_NAME_OFF offsetof(struct xen_hypfs_dirlistentry, name)
>>> -#define DIRENTRY_SIZE(name_len) \
>>> -    (DIRENTRY_NAME_OFF +        \
>>> -     ROUNDUP((name_len) + 1, alignof(struct xen_hypfs_direntry)))
>>> -
>>>   struct hypfs_funcs hypfs_dir_funcs = {
>>>       .read = hypfs_read_dir,
>>> +    .getsize = hypfs_getsize,
>>> +    .findentry = hypfs_dir_findentry,
>>>   };
>>>   struct hypfs_funcs hypfs_leaf_ro_funcs = {
>>>       .read = hypfs_read_leaf,
>>> +    .getsize = hypfs_getsize,
>>>   };
>>>   struct hypfs_funcs hypfs_leaf_wr_funcs = {
>>>       .read = hypfs_read_leaf,
>>>       .write = hypfs_write_leaf,
>>> +    .getsize = hypfs_getsize,
>>>   };
>>>   struct hypfs_funcs hypfs_bool_wr_funcs = {
>>>       .read = hypfs_read_leaf,
>>>       .write = hypfs_write_bool,
>>> +    .getsize = hypfs_getsize,
>>>   };
>>>   struct hypfs_funcs hypfs_custom_wr_funcs = {
>>>       .read = hypfs_read_leaf,
>>>       .write = hypfs_write_custom,
>>> +    .getsize = hypfs_getsize,
>>>   };
>>
>> With the increasing number of fields that may (deliberately or
>> by mistake) be NULL, should we gain some form of proactive
>> guarding against calls through such pointers?
> 
> Hmm, up to now I think such a bug would be detected rather fast.

Not sure: Are there any unavoidable uses of all affected code
paths?

> I can add some ASSERT()s for mandatory functions not being NULL when
> a node is added dynamically or during hypfs initialization for the
> static nodes.

I'm not sure ASSERT()s alone are enough. I'd definitely prefer
something which at least avoids the obvious x86 PV privilege
escalation attack in case a call through NULL has gone
unnoticed earlier on. E.g. rather than storing NULL in unused
entries, store a non-canonical pointer so that the effect will
"just" be a DoS.

>>> @@ -88,6 +93,23 @@ static void hypfs_unlock(void)
>>>       }
>>>   }
>>>   
>>> +void *hypfs_alloc_dyndata(unsigned long size, unsigned long align)
>>
>> Will callers really need to specify (high) alignment values? IOW ...
>>
>>> +{
>>> +    unsigned int cpu = smp_processor_id();
>>> +
>>> +    ASSERT(per_cpu(hypfs_locked, cpu) != hypfs_unlocked);
>>> +    ASSERT(per_cpu(hypfs_dyndata, cpu) == NULL);
>>> +
>>> +    per_cpu(hypfs_dyndata, cpu) = _xzalloc(size, align);
>>
>> ... is xzalloc_bytes() not suitable for use here?
> 
> Good question.
> 
> Up to now I think we could get away without specific alignment.
> 
> I can drop that parameter for now if you'd like that better.

I think control over alignment should be limited to those
special cases really needing it.

>>> @@ -275,22 +305,25 @@ int hypfs_read_leaf(const struct hypfs_entry *entry,
>>>   
>>>       l = container_of(entry, const struct hypfs_entry_leaf, e);
>>>   
>>> -    return copy_to_guest(uaddr, l->u.content, entry->size) ? -EFAULT: 0;
>>> +    return copy_to_guest(uaddr, l->u.content, entry->funcs->getsize(entry)) ?
>>> +                                              -EFAULT : 0;
>>
>> With the intended avoiding of locking, how is this ->getsize()
>> guaranteed to not ...
>>
>>> @@ -298,7 +331,7 @@ static int hypfs_read(const struct hypfs_entry *entry,
>>>           goto out;
>>>   
>>>       ret = -ENOBUFS;
>>> -    if ( ulen < entry->size + sizeof(e) )
>>> +    if ( ulen < size + sizeof(e) )
>>>           goto out;
>>
>> ... invalidate the checking done here? (A similar risk looks to
>> exist on the write path, albeit there we have at least the
>> ->max_size checks, where I hope that field isn't mean to become
>> dynamic as well.)
> 
> I think you are right. I should add the size value as a parameter to the
> read and write functions.

Except that a function like hypfs_read_leaf() shouldn't really
require its caller to pass in the size of the leaf's payload.

Jan

Re: [PATCH 09/12] xen/hypfs: add support for id-based dynamic directories

[Jan Beulich]

On 17.11.2020 15:38, Jürgen Groß wrote:
> On 17.11.20 14:33, Jan Beulich wrote:
>> On 26.10.2020 10:13, Juergen Gross wrote:
>>> +static struct hypfs_entry *hypfs_dyndir_findentry(struct hypfs_entry_dir *dir,
>>> +                                                  const char *name,
>>> +                                                  unsigned int name_len)
>>> +{
>>> +    struct hypfs_dyndir_id *data;
>>> +
>>> +    data = hypfs_get_dyndata();
>>> +    if ( !data )
>>> +        return ERR_PTR(-ENOENT);
>>> +
>>> +    /* Use template with original findentry function. */
>>> +    return data->template->e.funcs->findentry(data->template, name, name_len);
>>
>> Why does this pass the address of the template? If it truly is
>> (just) a template, then its dirlist ought to be empty at all
>> times? If otoh the "template" indeed gets used as a node in the
>> tree then perhaps it wants naming differently? "Stem" would come
>> to mind, but likely there are better alternatives. I've also
>> considered the German "Statthalter", but its English translations
>> don't seem reasonable to me here. And "placeholder" has kind of a
>> negative touch. (Also in this case some of my "const?" remarks
>> may be irrelevant.)
> 
> It is basically a template tree.
> 
> In the current use case (cpupool/<id>/sched-gran) the template is
> <id> with the leaf "sched-gran" which is the template for the per
> cpupool incarnation.

I can see sched-gran being a template, albeit even that will get
dirtied as soon as a second leaf appears, as then these entries
again need linking together. I think anything called template
should be strictly r/o.

It's also not clear to me how adding a 2nd level in the hierarchy
would end up working: <component>/<id1>/<id2>/<leaf>. How would
<leaf> know all the higher level IDs it's associated with? While
I don't think this needs making work right away, the underlying
model at least shouldn't prohibit it.

Jan

Re: [PATCH 10/12] xen/hypfs: add cpupool directories

[Jürgen Groß]

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

On 17.11.20 15:13, Jan Beulich wrote:
> On 26.10.2020 10:13, Juergen Gross wrote:
>> @@ -992,6 +994,78 @@ static struct notifier_block cpu_nfb = {
>>       .notifier_call = cpu_callback
>>   };
>>   
>> +#ifdef CONFIG_HYPFS
>> +static HYPFS_DIR_INIT(cpupool_pooldir, "id");
> 
> This "id" string won't appear anywhere, will it? I would have
> expected this to act as the format string used when generating
> names of the dynamic entries. This would e.g. allow CPU pools
> to have decimal numbered names, but other entries also hex
> ones, and then if so desired also e.g. with leading zeros.

I like that idea.

> 
>> +static int cpupool_dir_read(const struct hypfs_entry *entry,
>> +                            XEN_GUEST_HANDLE_PARAM(void) uaddr)
>> +{
>> +    int ret = 0;
>> +    struct cpupool **q;
> 
> I was going to ask for const here, but the way for_each_cpupool()
> works looks to prohibit this. Nevertheless I wonder whether the
> extra level of indirection there wouldn't better be dropped. Of
> the users, only cpupool_destroy() looks to need it, so open-
> coding the loop there (or introducing an auxiliary variable)
> would allow improvements here and elsewhere. (Actually I notice
> there's also a similar use in cpupool_create(), but the general
> consideration remains.)

I'll have a look.

> 
>> +    spin_lock(&cpupool_lock);
>> +
>> +    for_each_cpupool(q)
>> +    {
>> +        ret = hypfs_read_dyndir_id_entry(&cpupool_pooldir, (*q)->cpupool_id,
>> +                                         !(*q)->next, &uaddr);
>> +        if ( ret )
>> +            break;
>> +    }
>> +
>> +    spin_unlock(&cpupool_lock);
>> +
>> +    return ret;
>> +}
>> +
>> +static unsigned int cpupool_dir_getsize(const struct hypfs_entry *entry)
>> +{
>> +    struct cpupool **q;
>> +    unsigned int size = 0;
>> +
>> +    spin_lock(&cpupool_lock);
>> +
>> +    for_each_cpupool(q)
>> +        size += HYPFS_DIRENTRY_SIZE(snprintf(NULL, 0, "%d", (*q)->cpupool_id));
> 
> Beyond the remark above I consider this problematic: If the pool
> ID was negative, the use of %d here would get things out of sync
> with the %u uses in hypfs.c. I guess exposing
> HYPFS_DIRENTRY_SIZE() isn't the right approach, and you instead
> need another hypfs library function.

Fine with me.

> 
>> +static struct hypfs_entry *cpupool_dir_findentry(struct hypfs_entry_dir *dir,
>> +                                                 const char *name,
>> +                                                 unsigned int name_len)
>> +{
>> +    unsigned long id;
>> +    const char *end;
>> +    struct cpupool *cpupool;
>> +
>> +    id = simple_strtoul(name, &end, 10);
>> +    if ( id > INT_MAX || end != name + name_len )
> 
> What does this INT_MAX match up with? Afaics
> XEN_SYSCTL_CPUPOOL_OP_CREATE is fine to have an effectively
> negative pool ID passed in (the public interface struct uses
> uint32_t, but this gets converted to plain int first thing in
> the sysctl handler).

Oh, this wants to be fixed.

> 
>> +        return ERR_PTR(-ENOENT);
>> +
>> +    spin_lock(&cpupool_lock);
>> +
>> +    cpupool = __cpupool_find_by_id(id, true);
>> +
>> +    spin_unlock(&cpupool_lock);
>> +
>> +    if ( !cpupool )
>> +        return ERR_PTR(-ENOENT);
>> +
>> +    return hypfs_gen_dyndir_entry_id(&cpupool_pooldir, id);
> 
> The latest this one makes clear that cpupool_lock nests inside
> the hypfs one. I think this wants spelling out next to the
> definition of the former, as it implies that there are
> restrictions on what can be done from inside cpupool-locked
> regions. hypfs_read_dyndir_id_entry(), for example, has to
> remain lock free for this reason.

Okay, I'll add a comment.


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

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

Re: [PATCH 08/12] xen/hypfs: support dynamic hypfs nodes

[Jürgen Groß]

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

On 17.11.20 15:40, Jan Beulich wrote:
> On 17.11.2020 15:29, Jürgen Groß wrote:
>> On 17.11.20 13:37, Jan Beulich wrote:
>>> On 26.10.2020 10:13, Juergen Gross wrote:
>>>> --- a/xen/common/hypfs.c
>>>> +++ b/xen/common/hypfs.c
>>>> @@ -19,28 +19,29 @@
>>>>    CHECK_hypfs_dirlistentry;
>>>>    #endif
>>>>    
>>>> -#define DIRENTRY_NAME_OFF offsetof(struct xen_hypfs_dirlistentry, name)
>>>> -#define DIRENTRY_SIZE(name_len) \
>>>> -    (DIRENTRY_NAME_OFF +        \
>>>> -     ROUNDUP((name_len) + 1, alignof(struct xen_hypfs_direntry)))
>>>> -
>>>>    struct hypfs_funcs hypfs_dir_funcs = {
>>>>        .read = hypfs_read_dir,
>>>> +    .getsize = hypfs_getsize,
>>>> +    .findentry = hypfs_dir_findentry,
>>>>    };
>>>>    struct hypfs_funcs hypfs_leaf_ro_funcs = {
>>>>        .read = hypfs_read_leaf,
>>>> +    .getsize = hypfs_getsize,
>>>>    };
>>>>    struct hypfs_funcs hypfs_leaf_wr_funcs = {
>>>>        .read = hypfs_read_leaf,
>>>>        .write = hypfs_write_leaf,
>>>> +    .getsize = hypfs_getsize,
>>>>    };
>>>>    struct hypfs_funcs hypfs_bool_wr_funcs = {
>>>>        .read = hypfs_read_leaf,
>>>>        .write = hypfs_write_bool,
>>>> +    .getsize = hypfs_getsize,
>>>>    };
>>>>    struct hypfs_funcs hypfs_custom_wr_funcs = {
>>>>        .read = hypfs_read_leaf,
>>>>        .write = hypfs_write_custom,
>>>> +    .getsize = hypfs_getsize,
>>>>    };
>>>
>>> With the increasing number of fields that may (deliberately or
>>> by mistake) be NULL, should we gain some form of proactive
>>> guarding against calls through such pointers?
>>
>> Hmm, up to now I think such a bug would be detected rather fast.
> 
> Not sure: Are there any unavoidable uses of all affected code
> paths?

Assuming that any new node implementation is tested at least once,
yes. But in general you are right, of course.

> 
>> I can add some ASSERT()s for mandatory functions not being NULL when
>> a node is added dynamically or during hypfs initialization for the
>> static nodes.
> 
> I'm not sure ASSERT()s alone are enough. I'd definitely prefer
> something which at least avoids the obvious x86 PV privilege
> escalation attack in case a call through NULL has gone
> unnoticed earlier on. E.g. rather than storing NULL in unused
> entries, store a non-canonical pointer so that the effect will
> "just" be a DoS.

Hmm, either this, or a dummy function doing no harm?

> 
>>>> @@ -88,6 +93,23 @@ static void hypfs_unlock(void)
>>>>        }
>>>>    }
>>>>    
>>>> +void *hypfs_alloc_dyndata(unsigned long size, unsigned long align)
>>>
>>> Will callers really need to specify (high) alignment values? IOW ...
>>>
>>>> +{
>>>> +    unsigned int cpu = smp_processor_id();
>>>> +
>>>> +    ASSERT(per_cpu(hypfs_locked, cpu) != hypfs_unlocked);
>>>> +    ASSERT(per_cpu(hypfs_dyndata, cpu) == NULL);
>>>> +
>>>> +    per_cpu(hypfs_dyndata, cpu) = _xzalloc(size, align);
>>>
>>> ... is xzalloc_bytes() not suitable for use here?
>>
>> Good question.
>>
>> Up to now I think we could get away without specific alignment.
>>
>> I can drop that parameter for now if you'd like that better.
> 
> I think control over alignment should be limited to those
> special cases really needing it.

Okay, I'll drop the align parameter then.

> 
>>>> @@ -275,22 +305,25 @@ int hypfs_read_leaf(const struct hypfs_entry *entry,
>>>>    
>>>>        l = container_of(entry, const struct hypfs_entry_leaf, e);
>>>>    
>>>> -    return copy_to_guest(uaddr, l->u.content, entry->size) ? -EFAULT: 0;
>>>> +    return copy_to_guest(uaddr, l->u.content, entry->funcs->getsize(entry)) ?
>>>> +                                              -EFAULT : 0;
>>>
>>> With the intended avoiding of locking, how is this ->getsize()
>>> guaranteed to not ...
>>>
>>>> @@ -298,7 +331,7 @@ static int hypfs_read(const struct hypfs_entry *entry,
>>>>            goto out;
>>>>    
>>>>        ret = -ENOBUFS;
>>>> -    if ( ulen < entry->size + sizeof(e) )
>>>> +    if ( ulen < size + sizeof(e) )
>>>>            goto out;
>>>
>>> ... invalidate the checking done here? (A similar risk looks to
>>> exist on the write path, albeit there we have at least the
>>> ->max_size checks, where I hope that field isn't mean to become
>>> dynamic as well.)
>>
>> I think you are right. I should add the size value as a parameter to the
>> read and write functions.
> 
> Except that a function like hypfs_read_leaf() shouldn't really
> require its caller to pass in the size of the leaf's payload.

Its not the leaf's payload size, but the size the user buffer has been
tested against.


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

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

Re: [PATCH 09/12] xen/hypfs: add support for id-based dynamic directories

[Jürgen Groß]

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

On 17.11.20 15:50, Jan Beulich wrote:
> On 17.11.2020 15:38, Jürgen Groß wrote:
>> On 17.11.20 14:33, Jan Beulich wrote:
>>> On 26.10.2020 10:13, Juergen Gross wrote:
>>>> +static struct hypfs_entry *hypfs_dyndir_findentry(struct hypfs_entry_dir *dir,
>>>> +                                                  const char *name,
>>>> +                                                  unsigned int name_len)
>>>> +{
>>>> +    struct hypfs_dyndir_id *data;
>>>> +
>>>> +    data = hypfs_get_dyndata();
>>>> +    if ( !data )
>>>> +        return ERR_PTR(-ENOENT);
>>>> +
>>>> +    /* Use template with original findentry function. */
>>>> +    return data->template->e.funcs->findentry(data->template, name, name_len);
>>>
>>> Why does this pass the address of the template? If it truly is
>>> (just) a template, then its dirlist ought to be empty at all
>>> times? If otoh the "template" indeed gets used as a node in the
>>> tree then perhaps it wants naming differently? "Stem" would come
>>> to mind, but likely there are better alternatives. I've also
>>> considered the German "Statthalter", but its English translations
>>> don't seem reasonable to me here. And "placeholder" has kind of a
>>> negative touch. (Also in this case some of my "const?" remarks
>>> may be irrelevant.)
>>
>> It is basically a template tree.
>>
>> In the current use case (cpupool/<id>/sched-gran) the template is
>> <id> with the leaf "sched-gran" which is the template for the per
>> cpupool incarnation.
> 
> I can see sched-gran being a template, albeit even that will get
> dirtied as soon as a second leaf appears, as then these entries
> again need linking together. I think anything called template
> should be strictly r/o.

After boot the template will be constant, just like the hypfs tree
created at boot time.

In theory it could be setup completely static, but this would be
a rather awful mess of macros.

> It's also not clear to me how adding a 2nd level in the hierarchy
> would end up working: <component>/<id1>/<id2>/<leaf>. How would
> <leaf> know all the higher level IDs it's associated with? While
> I don't think this needs making work right away, the underlying
> model at least shouldn't prohibit it.

This is the purpose of hypfs_alloc_dyndata(). You'd need something
like struct hypfs_dyndir_id, but with two ids in it.


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

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

Re: [PATCH 11/12] xen/hypfs: add scheduling granularity entry to cpupool entries

[Jan Beulich]

On 26.10.2020 10:13, Juergen Gross wrote:
> @@ -1057,6 +1063,43 @@ static struct hypfs_entry *cpupool_dir_findentry(struct hypfs_entry_dir *dir,
>      return hypfs_gen_dyndir_entry_id(&cpupool_pooldir, id);
>  }
>  
> +static int cpupool_gran_read(const struct hypfs_entry *entry,
> +                             XEN_GUEST_HANDLE_PARAM(void) uaddr)
> +{
> +    const struct hypfs_dyndir_id *data;
> +    struct cpupool *cpupool;

const?

> +    const char *name = "";
> +
> +    data = hypfs_get_dyndata();
> +    if ( !data )
> +        return -ENOENT;
> +
> +    spin_lock(&cpupool_lock);
> +
> +    cpupool = __cpupool_find_by_id(data->id, true);
> +    if ( cpupool )
> +        name = sched_gran_get_name(cpupool->gran);
> +
> +    spin_unlock(&cpupool_lock);
> +
> +    if ( !cpupool )

May I suggest to check !*name here, to avoid giving the impression
of ...

> +        return -ENOENT;
> +
> +    return copy_to_guest(uaddr, name, strlen(name) + 1) ? -EFAULT : 0;

... success (but an empty name) in this admittedly unlikely event?

Jan

Re: [PATCH 11/12] xen/hypfs: add scheduling granularity entry to cpupool entries

[Jürgen Groß]

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

On 17.11.20 17:49, Jan Beulich wrote:
> On 26.10.2020 10:13, Juergen Gross wrote:
>> @@ -1057,6 +1063,43 @@ static struct hypfs_entry *cpupool_dir_findentry(struct hypfs_entry_dir *dir,
>>       return hypfs_gen_dyndir_entry_id(&cpupool_pooldir, id);
>>   }
>>   
>> +static int cpupool_gran_read(const struct hypfs_entry *entry,
>> +                             XEN_GUEST_HANDLE_PARAM(void) uaddr)
>> +{
>> +    const struct hypfs_dyndir_id *data;
>> +    struct cpupool *cpupool;
> 
> const?

Yes.

> 
>> +    const char *name = "";
>> +
>> +    data = hypfs_get_dyndata();
>> +    if ( !data )
>> +        return -ENOENT;
>> +
>> +    spin_lock(&cpupool_lock);
>> +
>> +    cpupool = __cpupool_find_by_id(data->id, true);
>> +    if ( cpupool )
>> +        name = sched_gran_get_name(cpupool->gran);
>> +
>> +    spin_unlock(&cpupool_lock);
>> +
>> +    if ( !cpupool )
> 
> May I suggest to check !*name here, to avoid giving the impression
> of ...
> 
>> +        return -ENOENT;
>> +
>> +    return copy_to_guest(uaddr, name, strlen(name) + 1) ? -EFAULT : 0;
> 
> ... success (but an empty name) in this admittedly unlikely event?

Fine with me.


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

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