[PATCH 0/7] Replace jailhouse-cell-list by native C implementation

[Ralf Ramsauer]

Hi,

this is a short series that does not introduce any functional changes.

I'd like to replace jailhouse-cell-list. It's a rudimentary function and
requires python. So I reimplemented cell-list natively in C and did some
cleanup work on tools/jailhouse.c.

First three patches are only housekeeping work. tools/jailhouse.c didn't respect
the return values of close(). Though it's very unlikely that close() fails,
return values should be checked. I introduced a global file descriptor to minimise
the occurence of open()s and close()s.

Fourth patch introduces some preparatory changes on read_file() as we need to read
files from sysfs.

Fifth patch is discussible. I don't know if it's a good idea to change the output
format of /sys/devices/jailhouse/cells/*/cpus_*. On the other hand -- who's gonna
parse it? I'm open for comments on that.

The last patch only does some cosmetic work.

Tested on x86 qemu and Jetson TK1. Applies against master and next.

Ralf

Ralf Ramsauer (7):
tools: jailhouse: reduce redundant usage of "/dev/jailhouse"
tools: jailhouse: introduce global jailhouse file descriptor
tools: jailhouse: add additional sanity checks to read_file()
tools: jailhouse: prepare read_file() to read files from sysfs
driver: sysfs: print human readable output for cpus_{assigned,failed}
tools: jailhouse: replace jailhouse-cell-list by native implementation
tools: jailhouse: introduce STR_(N)EQ macros

driver/sysfs.c | 8 +-
tools/jailhouse-cell-list | 71 ------------
tools/jailhouse.c | 280 ++++++++++++++++++++++++++++++++++++----------
3 files changed, 228 insertions(+), 131 deletions(-)
delete mode 100755 tools/jailhouse-cell-list

--
2.9.2

[Ralf Ramsauer]

The file descriptor of /dev/jailhouse is used in almost every path of
jailhouse.c. open_dev() is used to open and return the descriptor and
every path closes it on its own while ignoring the return value of
close().

This patch makes the file descriptor globally available and implements a
counterpart to open_dev(): close_dev(). close_dev() is called when
leaving the main routine and it will only close the descriptor if it is
valid.

Additionally, the return value of close() was ignored before. This
is fixed by checking its return value in close_dev().

Not all commands necessarily need an open file descriptor (e.g. config
create, config collect). The descriptor is only opened when needed, so
the jailhouse tool will run on systems without the module being loaded.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>
---
tools/jailhouse.c | 63 +++++++++++++++++++++++++++++--------------------------
1 file changed, 33 insertions(+), 30 deletions(-)

diff --git a/tools/jailhouse.c b/tools/jailhouse.c
index be5b833..5bf9b0f 100644
--- a/tools/jailhouse.c
+++ b/tools/jailhouse.c
@@ -29,6 +29,7 @@
#define JAILHOUSE_DEVICE "/dev/jailhouse"

enum shutdown_load_mode {LOAD, SHUTDOWN};
+static int jailhouse_fd = -1;

struct extension {
char *cmd, *subcmd, *help;
@@ -98,16 +99,29 @@ static void call_extension_script(const char *cmd, int argc, char *argv[])
}
}

-static int open_dev()
+static void open_dev()
{
- int fd;
+ if (jailhouse_fd != -1)
+ return;

- fd = open(JAILHOUSE_DEVICE, O_RDWR);
- if (fd < 0) {
+ jailhouse_fd = open(JAILHOUSE_DEVICE, O_RDWR);
+ if (jailhouse_fd < 0) {
perror("opening " JAILHOUSE_DEVICE);
exit(1);
}
- return fd;
+}
+
+static void close_dev()
+{
+ if (jailhouse_fd == -1)
+ return;
+
+ if (close(jailhouse_fd)) {
+ perror("closing " JAILHOUSE_DEVICE);
+ exit(1);
+ }
+
+ jailhouse_fd = -1;
}

static void *read_string(const char *string, size_t *size)
@@ -164,20 +178,17 @@ static void *read_file(const char *name, size_t *size)
static int enable(int argc, char *argv[])
{
void *config;
- int err, fd;
+ int err;

if (argc != 3)
help(argv[0], 1);

config = read_file(argv[2], NULL);

- fd = open_dev();
-
- err = ioctl(fd, JAILHOUSE_ENABLE, config);
+ err = ioctl(jailhouse_fd, JAILHOUSE_ENABLE, config);
if (err)
perror("JAILHOUSE_ENABLE");

- close(fd);
free(config);

return err;
@@ -187,7 +198,7 @@ static int cell_create(int argc, char *argv[])
{
struct jailhouse_cell_create cell_create;
size_t size;
- int err, fd;
+ int err;

if (argc != 4)
help(argv[0], 1);
@@ -195,13 +206,10 @@ static int cell_create(int argc, char *argv[])
cell_create.config_address = (unsigned long)read_file(argv[3], &size);
cell_create.config_size = size;

- fd = open_dev();
-
- err = ioctl(fd, JAILHOUSE_CELL_CREATE, &cell_create);
+ err = ioctl(jailhouse_fd, JAILHOUSE_CELL_CREATE, &cell_create);
if (err)
perror("JAILHOUSE_CELL_CREATE");

- close(fd);
free((void *)(unsigned long)cell_create.config_address);

return err;
@@ -255,7 +263,7 @@ static int cell_shutdown_load(int argc, char *argv[],
struct jailhouse_preload_image *image;
struct jailhouse_cell_load *cell_load;
struct jailhouse_cell_id cell_id;
- int err, fd, id_args, arg_num;
+ int err, id_args, arg_num;
unsigned int images, n;
size_t size;
char *endp;
@@ -320,13 +328,10 @@ static int cell_shutdown_load(int argc, char *argv[],
}
}

- fd = open_dev();
-
- err = ioctl(fd, JAILHOUSE_CELL_LOAD, cell_load);
+ err = ioctl(jailhouse_fd, JAILHOUSE_CELL_LOAD, cell_load);
if (err)
perror("JAILHOUSE_CELL_LOAD");

- close(fd);
for (n = 0, image = cell_load->image; n < images; n++, image++)
free((void *)(unsigned long)image->source_address);
free(cell_load);
@@ -337,15 +342,13 @@ static int cell_shutdown_load(int argc, char *argv[],
static int cell_simple_cmd(int argc, char *argv[], unsigned int command)
{
struct jailhouse_cell_id cell_id;
- int id_args, err, fd;
+ int id_args, err;

id_args = parse_cell_id(&cell_id, argc - 3, &argv[3]);
if (id_args == 0 || 3 + id_args != argc)
help(argv[0], 1);

- fd = open_dev();
-
- err = ioctl(fd, command, &cell_id);
+ err = ioctl(jailhouse_fd, command, &cell_id);
if (err)
perror(command == JAILHOUSE_CELL_START ?
"JAILHOUSE_CELL_START" :
@@ -353,8 +356,6 @@ static int cell_simple_cmd(int argc, char *argv[], unsigned int command)
"JAILHOUSE_CELL_DESTROY" :
"<unknown command>");

- close(fd);
-
return err;
}

@@ -385,21 +386,21 @@ static int cell_management(int argc, char *argv[])

int main(int argc, char *argv[])
{
- int fd;
int err;

if (argc < 2)
help(argv[0], 1);

if (strcmp(argv[1], "enable") == 0) {
+ open_dev();
err = enable(argc, argv);
} else if (strcmp(argv[1], "disable") == 0) {
- fd = open_dev();
- err = ioctl(fd, JAILHOUSE_DISABLE);
+ open_dev();
+ err = ioctl(jailhouse_fd, JAILHOUSE_DISABLE);
if (err)
perror("JAILHOUSE_DISABLE");
- close(fd);
} else if (strcmp(argv[1], "cell") == 0) {
+ open_dev();
err = cell_management(argc, argv);
} else if (strcmp(argv[1], "config") == 0 ||
strcmp(argv[1], "hardware") == 0) {
@@ -414,5 +415,7 @@ int main(int argc, char *argv[])
help(argv[0], 1);
}

+ close_dev();
+
return err ? 1 : 0;
}
--
2.9.2

[Ralf Ramsauer]

The device file "/dev/jailhouse" appears in several locations. Reducing
the occurence of this string makes code more flexible.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>
---

tools/jailhouse.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/jailhouse.c b/tools/jailhouse.c
index ba2af66..be5b833 100644
--- a/tools/jailhouse.c
+++ b/tools/jailhouse.c
@@ -26,6 +26,7 @@
#include <jailhouse.h>

#define JAILHOUSE_EXEC_DIR LIBEXECDIR "/jailhouse"
+#define JAILHOUSE_DEVICE "/dev/jailhouse"

enum shutdown_load_mode {LOAD, SHUTDOWN};

@@ -101,9 +102,9 @@ static int open_dev()
{
int fd;

- fd = open("/dev/jailhouse", O_RDWR);
+ fd = open(JAILHOUSE_DEVICE, O_RDWR);
if (fd < 0) {
- perror("opening /dev/jailhouse");
+ perror("opening " JAILHOUSE_DEVICE);
exit(1);
}
return fd;
--
2.9.2

[Ralf Ramsauer]

This makes output more readable. Kernel provides an interface for pretty
printed output. Use it.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>
---

driver/sysfs.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/driver/sysfs.c b/driver/sysfs.c
index 4959b56..b40da13 100644
--- a/driver/sysfs.c
+++ b/driver/sysfs.c
@@ -173,10 +173,10 @@ static ssize_t cpus_assigned_show(struct kobject *kobj,
int written;

#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,0,0)
- written = scnprintf(buf, PAGE_SIZE, "%*pb\n",
+ written = scnprintf(buf, PAGE_SIZE, "%*pbl\n",
cpumask_pr_args(&cell->cpus_assigned));
#else
- written = cpumask_scnprintf(buf, PAGE_SIZE, &cell->cpus_assigned);
+ written = cpulist_scnprintf(buf, PAGE_SIZE, &cell->cpus_assigned);
written += scnprintf(buf + written, PAGE_SIZE - written, "\n");
#endif
return written;
@@ -200,10 +200,10 @@ static ssize_t cpus_failed_show(struct kobject *kobj,
cpumask_set_cpu(cpu, cpus_failed);

#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,0,0)
- written = scnprintf(buf, PAGE_SIZE, "%*pb\n",
+ written = scnprintf(buf, PAGE_SIZE, "%*pbl\n",
cpumask_pr_args(cpus_failed));
#else
- written = cpumask_scnprintf(buf, PAGE_SIZE, cpus_failed);
+ written = cpulist_scnprintf(buf, PAGE_SIZE, cpus_failed);
written += scnprintf(buf + written, PAGE_SIZE - written, "\n");
#endif

--
2.9.2

[Ralf Ramsauer]

Check if file size is not zero and check the return value of close() as
it might fail, though it's very unlikely.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>
---

tools/jailhouse.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/tools/jailhouse.c b/tools/jailhouse.c
index 5bf9b0f..8c7783c 100644
--- a/tools/jailhouse.c
+++ b/tools/jailhouse.c
@@ -156,6 +156,11 @@ static void *read_file(const char *name, size_t *size)
exit(1);
}

+ if (stat.st_size == 0) {
+ fprintf(stderr, "reading empty file: %s\n", name);
+ exit(1);
+ }
+
buffer = malloc(stat.st_size);
if (!buffer) {
fprintf(stderr, "insufficient memory\n");
@@ -167,7 +172,10 @@ static void *read_file(const char *name, size_t *size)
exit(1);
}

- close(fd);
+ if (close(fd)) {
+ fprintf(stderr, "closing %s: %s\n", name, strerror(errno));
+ exit(1);
+ }

if (size)
*size = stat.st_size;
--
2.9.2

[Ralf Ramsauer]

Use a native implementation of the jailhouse-cell-list interface. Though
this implementation requires more lines of code, it is nice to be able
to run jailhouse cell list without a dependency on python.

Keep the output format of the former python script.

Also Add add simple interface to read sysfs entries.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>
---

tools/jailhouse-cell-list | 71 ---------------------
tools/jailhouse.c | 153 +++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 152 insertions(+), 72 deletions(-)
delete mode 100755 tools/jailhouse-cell-list

diff --git a/tools/jailhouse-cell-list b/tools/jailhouse-cell-list
deleted file mode 100755
index 3b5bc20..0000000
--- a/tools/jailhouse-cell-list
+++ /dev/null
@@ -1,71 +0,0 @@
-#!/usr/bin/env python
-
-# Jailhouse, a Linux-based partitioning hypervisor
-#
-# Copyright (c) Siemens AG, 2014
-#
-# Authors:
-# Jan Kiszka <jan.k...@siemens.com>
-#
-# This work is licensed under the terms of the GNU GPL, version 2. See
-# the COPYING file in the top-level directory.
-
-import glob
-import os
-import sys
-
-
-def read_cpus(path):
- return int(open(path).readline().strip().replace(",", ""), 16)
-
-
-def finish_cpu_list(last_cpu, cpu):
- if last_cpu is not None:
- if cpu-last_cpu > 1:
- return "%s%d," % ("-" if cpu-last_cpu > 2 else ",", cpu-1)
- else:
- return ","
- else:
- return ""
-
-
-def print_cpus(mask):
- last_cpu = None
- cpu = 0
- output = ""
- while mask > 0:
- if mask & 1:
- if last_cpu is None:
- last_cpu = cpu
- output += str(cpu)
- else:
- output += finish_cpu_list(last_cpu, cpu)
- last_cpu = None
- mask >>= 1
- cpu += 1
- output += finish_cpu_list(last_cpu, cpu)
- return output.strip(",")
-
-
-if len(sys.argv) > 1:
- print("usage: %s" % os.path.basename(sys.argv[0]).replace("-", " "))
- exit(0 if sys.argv[1] in ("--help", "-h") else 1)
-
-cells = []
-for cell_path in glob.glob('/sys/devices/jailhouse/cells/*'):
- cells.append({
- 'name': os.path.basename(cell_path),
- 'id': open(cell_path + "/id").readline().strip(),
- 'state': open(cell_path + "/state").readline().strip(),
- 'cpus_assigned': read_cpus(cell_path + "/cpus_assigned"),
- 'cpus_failed': read_cpus(cell_path + "/cpus_failed")
- })
-
-line_format = "%-8s%-24s%-16s%-24s%-24s"
-if not cells == []:
- print(line_format % ("ID", "Name", "State",
- "Assigned CPUs", "Failed CPUs"))
-for cell in sorted(cells, key=lambda cell: cell['id']):
- print(line_format % (cell['id'], cell['name'], cell['state'],
- print_cpus(cell['cpus_assigned']),
- print_cpus(cell['cpus_failed'])))
diff --git a/tools/jailhouse.c b/tools/jailhouse.c
index 8a2a2b2..ea3c315 100644
--- a/tools/jailhouse.c
+++ b/tools/jailhouse.c
@@ -14,6 +14,7 @@
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
+#include <dirent.h>
#include <unistd.h>
#include <fcntl.h>
#include <errno.h>
@@ -27,6 +28,7 @@

#define JAILHOUSE_EXEC_DIR LIBEXECDIR "/jailhouse"
#define JAILHOUSE_DEVICE "/dev/jailhouse"
+#define JAILHOUSE_CELLS "/sys/devices/jailhouse/cells/"

enum shutdown_load_mode {LOAD, SHUTDOWN};
static int jailhouse_fd = -1;
@@ -35,11 +37,17 @@ struct extension {
char *cmd, *subcmd, *help;
};

+struct jailhouse_cell_info {
+ struct jailhouse_cell_id id;
+ char *state;
+ char *cpus_assigned;
+ char *cpus_failed;
+};
+
static const struct extension extensions[] = {
{ "cell", "linux", "CELLCONFIG KERNEL [-i | --initrd FILE]\n"
" [-c | --cmdline \"STRING\"] "
"[-w | --write-params FILE]" },
- { "cell", "list", "" },
{ "cell", "stats", "{ ID | [--name] NAME }" },
{ "config", "create", "[-h] [-g] [-r ROOT] "
"[--mem-inmates MEM_INMATES]\n"
@@ -58,6 +66,7 @@ static void __attribute__((noreturn)) help(char *prog, int exit_status)
" enable SYSCONFIG\n"
" disable\n"
" cell create CELLCONFIG\n"
+ " cell list\n"
" cell load { ID | [--name] NAME } "
"{ IMAGE | { -s | --string } \"STRING\" }\n"
" [-a | --address ADDRESS] ...\n"
@@ -185,6 +194,20 @@ static void *read_file(const char *name, size_t *size, bool respect_size)
return buffer;
}

+static char *read_sysfs_entry(const char *name)
+{
+ char *ret;
+ size_t size;
+
+ ret = read_file(name, &size, false);
+
+ /* enforce the string to be null-terminated */
+ if (size && ret[size-1] != '\0')
+ ret[size-1] = '\0';
+
+ return ret;
+}
+

static int enable(int argc, char *argv[])
{
void *config;

@@ -267,6 +290,132 @@ static bool match_opt(const char *argv, const char *short_opt,
strcmp(argv, long_opt) == 0;
}

+static struct jailhouse_cell_info *get_cell_info(const char *name)
+{
+ char buf[sizeof(JAILHOUSE_CELLS) + JAILHOUSE_CELL_ID_NAMELEN +
+ sizeof("/cpus_assigned") + 1];
+ struct jailhouse_cell_info *cinfo;
+ char *tmp;
+
+ cinfo = calloc(1, sizeof(struct jailhouse_cell_info));
+
+ /* copy cell name */
+ strncpy(cinfo->id.name, name, JAILHOUSE_CELL_ID_NAMELEN);
+
+ /* get cell id */
+ snprintf(buf, sizeof(buf), JAILHOUSE_CELLS "/%s/id", cinfo->id.name);
+ tmp = read_sysfs_entry(buf);
+ cinfo->id.id = (__s32)strtol(tmp, NULL, 10);
+ free(tmp);
+
+ /* get cell state */
+ snprintf(buf, sizeof(buf), JAILHOUSE_CELLS "/%s/state", cinfo->id.name);
+ cinfo->state = read_sysfs_entry(buf);
+
+ /* getassigned cpus */
+ snprintf(buf, sizeof(buf), JAILHOUSE_CELLS "/%s/cpus_assigned", cinfo->id.name);
+ cinfo->cpus_assigned = read_sysfs_entry(buf);
+
+ /* get failed cpus */
+ snprintf(buf, sizeof(buf), JAILHOUSE_CELLS "/%s/cpus_failed", cinfo->id.name);
+ cinfo->cpus_failed = read_sysfs_entry(buf);
+
+ return cinfo;
+}
+
+static void cell_info_free(struct jailhouse_cell_info *cinfo)
+{
+ free(cinfo->state);
+ free(cinfo->cpus_assigned);
+ free(cinfo->cpus_failed);
+ free(cinfo);
+}
+
+static char **get_cell_names(void)
+{
+ DIR *dir;
+ struct dirent *dent;
+ char **ret;
+ int num_cells = 0;
+
+ ret = calloc(1, sizeof(char*));
+ if (ret == NULL) {
+ perror("calloc");
+ exit(1);
+ }
+
+ dir = opendir(JAILHOUSE_CELLS);
+ if (dir == NULL) {
+ perror("opendir " JAILHOUSE_CELLS);
+ exit(1);
+ }
+
+ while ((dent = readdir(dir)) != NULL) {
+ /* Skip hidden files, '.' and '..' */
+ if (dent->d_name[0] == '.')
+ continue;
+
+ ret = realloc(ret, (++num_cells+1) * sizeof(char*));
+ if (ret == NULL) {
+ perror("realloc");
+ exit(1);
+ }
+
+ ret[num_cells-1] = strdup(dent->d_name);
+ if (ret[num_cells-1] == NULL) {
+ perror("strdup");
+ exit(1);
+ }
+
+ /* propagate null termination */
+ ret[num_cells] = NULL;
+ }
+
+ if (closedir(dir)) {
+ perror("closedir " JAILHOUSE_CELLS);
+ exit(1);
+ }
+
+ return ret;
+}
+
+static void cell_names_free(char **cell_names)
+{
+ char **cell_name;
+ for (cell_name = cell_names; *cell_name; cell_name++)
+ free(*cell_name);
+ free(cell_names);
+}
+
+static int cell_list(int argc, char *argv[])
+{
+ char **cell_names, **cell_name;
+ struct jailhouse_cell_info *cinfo;
+ (void)argv;
+
+ if (argc != 3)
+ help(argv[0], 1);
+
+ cell_names = get_cell_names();
+ if (cell_names[0] == NULL) {
+ printf("No active jailhouse cells\n");
+ goto out;
+ }
+
+ printf("%-8s%-24s%-16s%-24s%-24s\n",
+ "ID", "Name", "State", "Assigned CPUs", "Failed CPUs");
+ for (cell_name = cell_names; *cell_name; cell_name++) {
+ cinfo = get_cell_info(*cell_name);
+ printf("%-8d%-24s%-16s%-24s%-24s\n", cinfo->id.id, cinfo->id.name, cinfo->state,
+ cinfo->cpus_assigned, cinfo->cpus_failed);
+ cell_info_free(cinfo);
+ }
+
+out:
+ cell_names_free(cell_names);
+ return 0;
+}
+

static int cell_shutdown_load(int argc, char *argv[],

enum shutdown_load_mode mode)
{
@@ -378,6 +527,8 @@ static int cell_management(int argc, char *argv[])

if (strcmp(argv[2], "create") == 0) {
err = cell_create(argc, argv);
+ } else if (strcmp(argv[2], "list") == 0) {
+ err = cell_list(argc, argv);
} else if (strcmp(argv[2], "load") == 0) {
err = cell_shutdown_load(argc, argv, LOAD);
} else if (strcmp(argv[2], "start") == 0) {
--
2.9.2

[Ralf Ramsauer]

read_file() aborts if the number of read bytes does not equal to the
corresponding size in struct stat. st_size and the actual file size may
differ if sysfs is involved.

If read() returns a number smaller than st.st_size does not necessarily
imply that an error must have occured.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>
---

tools/jailhouse.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/tools/jailhouse.c b/tools/jailhouse.c
index 8c7783c..8a2a2b2 100644
--- a/tools/jailhouse.c
+++ b/tools/jailhouse.c
@@ -139,11 +139,12 @@ static void *read_string(const char *string, size_t *size)
return buffer;
}

-static void *read_file(const char *name, size_t *size)
+static void *read_file(const char *name, size_t *size, bool respect_size)
{
struct stat stat;
void *buffer;
int fd;
+ ssize_t read_size;

fd = open(name, O_RDONLY);
if (fd < 0) {
@@ -167,7 +168,8 @@ static void *read_file(const char *name, size_t *size)
exit(1);
}

- if (read(fd, buffer, stat.st_size) < stat.st_size) {
+ read_size = read(fd, buffer, stat.st_size);
+ if (read_size == -1 || (read_size < stat.st_size && respect_size)) {
fprintf(stderr, "reading %s: %s\n", name, strerror(errno));
exit(1);
}
@@ -178,7 +180,7 @@ static void *read_file(const char *name, size_t *size)
}

if (size)
- *size = stat.st_size;
+ *size = (size_t)read_size;

return buffer;
}
@@ -191,7 +193,7 @@ static int enable(int argc, char *argv[])

if (argc != 3)
help(argv[0], 1);

- config = read_file(argv[2], NULL);
+ config = read_file(argv[2], NULL, true);

err = ioctl(jailhouse_fd, JAILHOUSE_ENABLE, config);
if (err)

@@ -211,7 +213,7 @@ static int cell_create(int argc, char *argv[])

if (argc != 4)
help(argv[0], 1);

- cell_create.config_address = (unsigned long)read_file(argv[3], &size);
+ cell_create.config_address = (unsigned long)read_file(argv[3], &size, true);
cell_create.config_size = size;

err = ioctl(jailhouse_fd, JAILHOUSE_CELL_CREATE, &cell_create);

@@ -320,7 +322,7 @@ static int cell_shutdown_load(int argc, char *argv[],
} else {
image->source_address =
(unsigned long)read_file(argv[arg_num++],
- &size);
+ &size, true);
}
image->size = size;
image->target_address = 0;
--
2.9.2

[Ralf Ramsauer]

Jailhouse command line tool makes heavy use of string comparisons for
parsing command line arguments. Simplify string comparisons by using
macros.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>
---

tools/jailhouse.c | 39 +++++++++++++++++++++------------------
1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/tools/jailhouse.c b/tools/jailhouse.c
index ea3c315..6687406 100644
--- a/tools/jailhouse.c
+++ b/tools/jailhouse.c
@@ -26,6 +26,9 @@

#include <jailhouse.h>

+#define STR_EQ(string, candidate) (strcmp(string, candidate) == 0)
+#define STR_NEQ(string, candidate) (strcmp(string, candidate) != 0)
+

#define JAILHOUSE_EXEC_DIR LIBEXECDIR "/jailhouse"
#define JAILHOUSE_DEVICE "/dev/jailhouse"

#define JAILHOUSE_CELLS "/sys/devices/jailhouse/cells/"
@@ -90,8 +93,8 @@ static void call_extension_script(const char *cmd, int argc, char *argv[])
return;

for (ext = extensions; ext->cmd; ext++) {
- if (strcmp(ext->cmd, cmd) != 0 ||
- strcmp(ext->subcmd, argv[2]) != 0)
+ if (STR_NEQ(ext->cmd, cmd) ||
+ STR_NEQ(ext->subcmd, argv[2]))
continue;

snprintf(new_path, sizeof(new_path), "PATH=%s:%s:%s",
@@ -260,7 +263,7 @@ static int parse_cell_id(struct jailhouse_cell_id *cell_id, int argc,
if (argc < 1)
return 0;

- if (strcmp(argv[0], "--name") == 0) {
+ if (STR_EQ(argv[0], "--name")) {
if (argc < 2)
return 0;
arg_pos++;
@@ -286,8 +289,8 @@ static int parse_cell_id(struct jailhouse_cell_id *cell_id, int argc,

static bool match_opt(const char *argv, const char *short_opt,

const char *long_opt)
{
- return strcmp(argv, short_opt) == 0 ||
- strcmp(argv, long_opt) == 0;
+ return STR_EQ(argv, short_opt) ||
+ STR_EQ(argv, long_opt);

}

static struct jailhouse_cell_info *get_cell_info(const char *name)

@@ -525,17 +528,17 @@ static int cell_management(int argc, char *argv[])
if (argc < 3)
help(argv[0], 1);

- if (strcmp(argv[2], "create") == 0) {
+ if (STR_EQ(argv[2], "create")) {
err = cell_create(argc, argv);
- } else if (strcmp(argv[2], "list") == 0) {
+ } else if (STR_EQ(argv[2], "list")) {
err = cell_list(argc, argv);
- } else if (strcmp(argv[2], "load") == 0) {
+ } else if (STR_EQ(argv[2], "load")) {

err = cell_shutdown_load(argc, argv, LOAD);

- } else if (strcmp(argv[2], "start") == 0) {
+ } else if (STR_EQ(argv[2], "start")) {
err = cell_simple_cmd(argc, argv, JAILHOUSE_CELL_START);
- } else if (strcmp(argv[2], "shutdown") == 0) {
+ } else if (STR_EQ(argv[2], "shutdown")) {
err = cell_shutdown_load(argc, argv, SHUTDOWN);
- } else if (strcmp(argv[2], "destroy") == 0) {
+ } else if (STR_EQ(argv[2], "destroy")) {
err = cell_simple_cmd(argc, argv, JAILHOUSE_CELL_DESTROY);
} else {
call_extension_script("cell", argc, argv);
@@ -552,25 +555,25 @@ int main(int argc, char *argv[])
if (argc < 2)
help(argv[0], 1);

- if (strcmp(argv[1], "enable") == 0) {
+ if (STR_EQ(argv[1], "enable")) {

open_dev();
err = enable(argc, argv);

- } else if (strcmp(argv[1], "disable") == 0) {
+ } else if (STR_EQ(argv[1], "disable")) {
open_dev();

err = ioctl(jailhouse_fd, JAILHOUSE_DISABLE);
if (err)
perror("JAILHOUSE_DISABLE");

- } else if (strcmp(argv[1], "cell") == 0) {
+ } else if (STR_EQ(argv[1], "cell")) {

open_dev();
err = cell_management(argc, argv);

- } else if (strcmp(argv[1], "config") == 0 ||
- strcmp(argv[1], "hardware") == 0) {
+ } else if (STR_EQ(argv[1], "config") ||
+ STR_EQ(argv[1], "hardware")) {
call_extension_script(argv[1], argc, argv);
help(argv[0], 1);
- } else if (strcmp(argv[1], "--version") == 0) {
+ } else if (STR_EQ(argv[1], "--version")) {
printf("Jailhouse management tool %s\n", JAILHOUSE_VERSION);
return 0;
- } else if (strcmp(argv[1], "--help") == 0) {
+ } else if (STR_EQ(argv[1], "--help")) {
help(argv[0], 0);
} else {
help(argv[0], 1);
--
2.9.2

[Jan Kiszka]

On 2016-08-04 21:28, Ralf Ramsauer wrote:
> Hi,
>
> this is a short series that does not introduce any functional changes.
>
> I'd like to replace jailhouse-cell-list. It's a rudimentary function and
> requires python. So I reimplemented cell-list natively in C and did some
> cleanup work on tools/jailhouse.c.
>
> First three patches are only housekeeping work. tools/jailhouse.c didn't respect
> the return values of close(). Though it's very unlikely that close() fails,
> return values should be checked. I introduced a global file descriptor to minimise
> the occurence of open()s and close()s.
>
> Fourth patch introduces some preparatory changes on read_file() as we need to read
> files from sysfs.
>
> Fifth patch is discussible. I don't know if it's a good idea to change the output
> format of /sys/devices/jailhouse/cells/*/cpus_*. On the other hand -- who's gonna
> parse it? I'm open for comments on that.

sysfs is primarily about being machine-readable, but your patch works a
bit against this. E.g., the libvirt driver for Jailhouse may want to (or
already does - didn't check) make use of that information. If at all, it
has to be an additional node that exposes the information in
human-readable format.

You still need to explain more clearly why we need beautified cell
listing as core feature of the tool. Or IOW, which use case is behind
having a minimal Linux system without Python and, thus, also without the
add-on scripts that is still frequently human-operated (there is still
sysfs for infrequent checks).

Jan

--
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

[Henning Schild]

Am Thu, 4 Aug 2016 21:28:14 +0200
schrieb Ralf Ramsauer <ra...@ramses-pyramidenbau.de>:

> Check if file size is not zero and check the return value of close()
> as it might fail, though it's very unlikely.
>
> Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>
> ---
> tools/jailhouse.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/tools/jailhouse.c b/tools/jailhouse.c
> index 5bf9b0f..8c7783c 100644
> --- a/tools/jailhouse.c
> +++ b/tools/jailhouse.c
> @@ -156,6 +156,11 @@ static void *read_file(const char *name, size_t
> *size) exit(1);
> }
>
> + if (stat.st_size == 0) {
> + fprintf(stderr, "reading empty file: %s\n", name);
> + exit(1);
> + }
> +

Being too smart about such things is - in general - not a good idea. It
is usually better to try whatever you want to do and handle errors.
Size > 0 checks will fail in procfs and readable size == stat.size will
fail in sysfs. Similar things can be true for permission checks and
funny filesystems like fuse or cifs.

[Ralf Ramsauer]

Hi Henning,

On 08/05/2016 09:55 AM, Henning Schild wrote:
> Am Thu, 4 Aug 2016 21:28:14 +0200
> schrieb Ralf Ramsauer <ra...@ramses-pyramidenbau.de>:
>
>> Check if file size is not zero and check the return value of close()
>> as it might fail, though it's very unlikely.
>>
>> Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>
>> ---
>> tools/jailhouse.c | 10 +++++++++-
>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/jailhouse.c b/tools/jailhouse.c
>> index 5bf9b0f..8c7783c 100644
>> --- a/tools/jailhouse.c
>> +++ b/tools/jailhouse.c
>> @@ -156,6 +156,11 @@ static void *read_file(const char *name, size_t
>> *size) exit(1);
>> }
>>
>> + if (stat.st_size == 0) {
>> + fprintf(stderr, "reading empty file: %s\n", name);
>> + exit(1);
>> + }
>> +
>
> Being too smart about such things is - in general - not a good idea. It
> is usually better to try whatever you want to do and handle errors.
> Size > 0 checks will fail in procfs and readable size == stat.size will
> fail in sysfs. Similar things can be true for permission checks and

Current code is not suitable for procfs, as malloc() is aligned to
st_size. sysfs will return PAGE_SIZE as an upper boundary. Patch 4/7
fixes this issue by allowing read() to return smaller values as st_size.

> funny filesystems like fuse or cifs.

Didn't experience such behaviour yet - guess you did :)
>
>> buffer = malloc(stat.st_size);
Ok, so if we don't check and st_size is zero, we're calling malloc(0)
and according to the malloc manpage, it might return NULL, and the
produced error message "insufficient memory" is not the proper message.

Ralf

>> if (!buffer) {
>> fprintf(stderr, "insufficient memory\n");
>> @@ -167,7 +172,10 @@ static void *read_file(const char *name, size_t
>> *size) exit(1);
>> }
>>
>> - close(fd);
>> + if (close(fd)) {
>> + fprintf(stderr, "closing %s: %s\n", name,
>> strerror(errno));
>> + exit(1);
>> + }
>>
>> if (size)
>> *size = stat.st_size;
>

--
Ralf Ramsauer
PGP: 0x8F10049B

[Valentine Sinitsyn]

Hi all,

On 05.08.2016 14:28, Ralf Ramsauer wrote:
> Hi Henning,
>
> On 08/05/2016 09:55 AM, Henning Schild wrote:
>> Am Thu, 4 Aug 2016 21:28:14 +0200
>> schrieb Ralf Ramsauer <ra...@ramses-pyramidenbau.de>:
>>
>>> Check if file size is not zero and check the return value of close()
>>> as it might fail, though it's very unlikely.
>>>
>>> Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>
>>> ---
>>> tools/jailhouse.c | 10 +++++++++-
>>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/jailhouse.c b/tools/jailhouse.c
>>> index 5bf9b0f..8c7783c 100644
>>> --- a/tools/jailhouse.c
>>> +++ b/tools/jailhouse.c
>>> @@ -156,6 +156,11 @@ static void *read_file(const char *name, size_t
>>> *size) exit(1);
>>> }
>>>
>>> + if (stat.st_size == 0) {
>>> + fprintf(stderr, "reading empty file: %s\n", name);
>>> + exit(1);

This hardcode look scary. What if one want a different message or
behavior? Wouldn't it be better to return some error code and let the
caller decide?

>>> + }
>>> +
>>
>> Being too smart about such things is - in general - not a good idea. It
>> is usually better to try whatever you want to do and handle errors.
>> Size > 0 checks will fail in procfs and readable size == stat.size will
>> fail in sysfs. Similar things can be true for permission checks and
> Current code is not suitable for procfs, as malloc() is aligned to
> st_size. sysfs will return PAGE_SIZE as an upper boundary. Patch 4/7
> fixes this issue by allowing read() to return smaller values as st_size.
>> funny filesystems like fuse or cifs.
> Didn't experience such behaviour yet - guess you did :)
>>
>>> buffer = malloc(stat.st_size);
> Ok, so if we don't check and st_size is zero, we're calling malloc(0)
> and according to the malloc manpage, it might return NULL, and the
> produced error message "insufficient memory" is not the proper message.

Allocating a reasonable initial amount then realloc() as you read the
file should work.

Valentine

[Ralf Ramsauer]

Hi,

You can find this kind of exit-on-fail strategy all over
tools/jailhouse.c (open_dev(), read_string(), read_file(), ...)

That's why I chose to exit in this case.

>
>>>> + }
>>>> +
>>>
>>> Being too smart about such things is - in general - not a good idea. It
>>> is usually better to try whatever you want to do and handle errors.
>>> Size > 0 checks will fail in procfs and readable size == stat.size will
>>> fail in sysfs. Similar things can be true for permission checks and
>> Current code is not suitable for procfs, as malloc() is aligned to
>> st_size. sysfs will return PAGE_SIZE as an upper boundary. Patch 4/7
>> fixes this issue by allowing read() to return smaller values as st_size.
>>> funny filesystems like fuse or cifs.
>> Didn't experience such behaviour yet - guess you did :)
>>>
>>>> buffer = malloc(stat.st_size);
>> Ok, so if we don't check and st_size is zero, we're calling malloc(0)
>> and according to the malloc manpage, it might return NULL, and the
>> produced error message "insufficient memory" is not the proper message.
> Allocating a reasonable initial amount then realloc() as you read the
> file should work.

Hm, results in looped read(). I'll think about that.

Ralf

>
> Valentine
>
>>
>> Ralf
>>>> if (!buffer) {
>>>> fprintf(stderr, "insufficient memory\n");
>>>> @@ -167,7 +172,10 @@ static void *read_file(const char *name, size_t
>>>> *size) exit(1);
>>>> }
>>>>
>>>> - close(fd);
>>>> + if (close(fd)) {
>>>> + fprintf(stderr, "closing %s: %s\n", name,
>>>> strerror(errno));
>>>> + exit(1);
>>>> + }
>>>>
>>>> if (size)
>>>> *size = stat.st_size;
>>>
>>

[Ralf Ramsauer]

Hi Jan,

On 08/05/2016 07:57 AM, Jan Kiszka wrote:
> On 2016-08-04 21:28, Ralf Ramsauer wrote:
>> Hi,
>>
>> this is a short series that does not introduce any functional changes.
>>
>> I'd like to replace jailhouse-cell-list. It's a rudimentary function and
>> requires python. So I reimplemented cell-list natively in C and did some
>> cleanup work on tools/jailhouse.c.
>>
>> First three patches are only housekeeping work. tools/jailhouse.c didn't respect
>> the return values of close(). Though it's very unlikely that close() fails,
>> return values should be checked. I introduced a global file descriptor to minimise
>> the occurence of open()s and close()s.
>>
>> Fourth patch introduces some preparatory changes on read_file() as we need to read
>> files from sysfs.
>>
>> Fifth patch is discussible. I don't know if it's a good idea to change the output
>> format of /sys/devices/jailhouse/cells/*/cpus_*. On the other hand -- who's gonna
>> parse it? I'm open for comments on that.
>
> sysfs is primarily about being machine-readable, but your patch works a
> bit against this. E.g., the libvirt driver for Jailhouse may want to (or
> already does - didn't check) make use of that information. If at all, it
> has to be an additional node that exposes the information in
> human-readable format.

Yep, I understand. I didn't consider that those device might already be
in use.

On the machine-readability: Actually the human readable format contains
the same amount of information as the machine-readable format and in
fact, both are machine-readable :-)
But I second, we should not break any existing interface.

Other than that, a binary bitmap as sysfs nodes for cpumasks would be
yet easier for machines to read, as you don't need to parse ASCII back
to a more general datatype.

So there are a few possibilities to solve this:
- create cell-specific human-readable nodes for cpumasks (like the
cells/state node, which is also human-readable)
- provide binary bitmaps as additional nodes (could be confusing for
people traversing through the /sys tree)
- leave driver code and parse those hex-comma-separated bitmaps in
userspace. (probably would result in a lot of userspace code and
documentation of this format looks a bit unclear to me. Is it e.g.
guaranteed, that %*pb produces the same output as
cpumask_scnprintf?)
- Do all the pretty printing stuff of cell-list inside the driver and
provide a /sys/devices/jailhouse/cell_list node with pretty printed
output. But this would require the sysfs driver to access
struct list_head cells in driver/cell.c.

Could you live with one of these options?

>
> You still need to explain more clearly why we need beautified cell
> listing as core feature of the tool. Or IOW, which use case is behind
> having a minimal Linux system without Python and, thus, also without the
> add-on scripts that is still frequently human-operated (there is still
> sysfs for infrequent checks).

Think of development convenience or ssh access to root cells inside
busybox, where you'd like list active cells. You won't have python on
such systems.

Ralf
>
> Jan

[Jan Kiszka]

Is this an assumption about a potential use case or something concrete
you plan / are working on? I'm poking on this as a technical solution,
as we see above, will not be a 3-lines patch.

BTW, the last jailhouse libvirt driver I saw did even more weird stuff:
it actually parsed the output if cell list instead of processing sysfs
as it was supposed to.

[Ralf Ramsauer]

It's surely not a one-liner but this series also 'perfects' some other
parts.

I have a system here that failed on cell-list because it misses python.
Besides that I think it's a good idea to reduce python code for
rudimentary functions. config-create, config-collect or hardware-check
should of course not be reimplemented in C.

Ah, additionally you minimise areas, where you always have to take care
to be compatible with Python 2 and 3.

>
> BTW, the last jailhouse libvirt driver I saw did even more weird stuff:
> it actually parsed the output if cell list instead of processing sysfs
> as it was supposed to.

Oh. Well, then at least my patches would not break libvirt as it keeps
the output format of the former python code.

[Ralf Ramsauer]

On 08/05/2016 11:43 AM, Ralf Ramsauer wrote:
>>>> >>> Being too smart about such things is - in general - not a good idea. It
>>>> >>> is usually better to try whatever you want to do and handle errors.
>>>> >>> Size > 0 checks will fail in procfs and readable size == stat.size will
>>>> >>> fail in sysfs. Similar things can be true for permission checks and
>>> >> Current code is not suitable for procfs, as malloc() is aligned to
>>> >> st_size. sysfs will return PAGE_SIZE as an upper boundary. Patch 4/7
>>> >> fixes this issue by allowing read() to return smaller values as st_size.
>>>> >>> funny filesystems like fuse or cifs.
>>> >> Didn't experience such behaviour yet - guess you did :)
>>>> >>>
>>>>> >>>> buffer = malloc(stat.st_size);
>>> >> Ok, so if we don't check and st_size is zero, we're calling malloc(0)
>>> >> and according to the malloc manpage, it might return NULL, and the
>>> >> produced error message "insufficient memory" is not the proper message.
>> > Allocating a reasonable initial amount then realloc() as you read the
>> > file should work.
> Hm, results in looped read(). I'll think about that.

Thinking about that: Wouldn't it be nicer to drop the struct stat and
not respect st_size at all?

Like Valentine mentioned before, preallocation a reasonable amount of
memory (1024, 4096, PAGE_SIZE?) and reallocating it should be fine and
this would work in all three cases: regular files, procfs and sysfs. No
need to fail on files with size 0.

Ralf
>
> Ralf

[Jan Kiszka]

On 2016-08-04 21:28, Ralf Ramsauer wrote:

Makes sense, queued.

Jan

[Jan Kiszka]

On 2016-08-04 21:28, Ralf Ramsauer wrote:

I don't see an added value here, neither in introducing a global
variable nor in reporting close() errors.

Jan

[Jan Kiszka]

On 2016-08-04 21:28, Ralf Ramsauer wrote:

Two changes, two patches - but none of them is needed or might even be
harmful (what if some user script intentionally inject an empty file as
place holder?).

Jan

[Jan Kiszka]

On 2016-08-04 21:28, Ralf Ramsauer wrote:

I think we can just generally relax the test here: first of all, the
error reported will be confusing for read_size >= 0 and, second, it is
not our problem if a real file changes its size due to a race with a
concurrent modification.

So let's just check for read_size < 0.

Jan

[Jan Kiszka]

On 2016-08-04 21:28, Ralf Ramsauer wrote:

I would suggest to have this format reported via cpus_assigned_list and
cpus_failed_list. The existing nodes should continue to provide an
easily machine-readable format (which is clearly not the above form).

Jan

[Jan Kiszka]

On 2016-08-04 21:28, Ralf Ramsauer wrote:

If we want to go down this way, sysfs should first of all be improved
to support this better:

diff --git a/Documentation/sysfs-entries.txt b/Documentation/sysfs-entries.txt
index 2845220..0fc315a 100644
--- a/Documentation/sysfs-entries.txt
+++ b/Documentation/sysfs-entries.txt
@@ -11,8 +11,8 @@ can be used for monitoring the state of the hypervisor and its cells.
|- remap_pool_size - number of pages in hypervisor remapping pool
|- remap_pool_used - used pages of hypervisor remapping pool
`- cells
- |- <name of cell>
- | |- id - unique numerical ID
+ |- <id> - unique numerical ID
+ | |- name - cell name
| |- state - "running", "running/locked", "shut down", or
| | "failed"
| |- cpus_assigned - bitmask of assigned logical CPUs

This would allow to sort the cells by ID while reading their directory
entries, thus should make the code above simpler. Would you give this a
try?

Jan

[Jan Kiszka]

On 2016-08-04 21:28, Ralf Ramsauer wrote:

This is ugly, and I've never seen it elsewhere so far.

Jan

[Ralf Ramsauer]

Many paths need the fd, and each path closes on its own. With this one,
we only have one single close() path.

close() is a function that - even very unlikely - might fail. I doubt
that silently accepting errors is a good idea.

I can drop this one in the next round too, if you want.

Ralf
>
> Jan
>


--
Ralf Ramsauer

GPG: 0x8F10049B

[Ralf Ramsauer]

On 08/06/2016 11:59 AM, Jan Kiszka wrote:

I already replaced this one in my local repository by dropping the
struct stat and looped read() calls, as Valentine already suggested.

It will return a free()able pointer and set size to zero, if an empty
file was read. Otherwise, it will read and realloc as long as EOF is
reached.

In this way, the function can handle regular files as well as sysfs of
procfs.

Ralf

[Ralf Ramsauer]

On 08/06/2016 11:59 AM, Jan Kiszka wrote:

Yep, I'll change it in the next round and introduce looped read()s.
Then we don't need the confusing bool respect_size as well.

Ralf

[Ralf Ramsauer]

I understand and agree.

(but it is machine-readable :-) If only bitmap_parselist or bitmap_parse
would exist in userland...)

Ralf
>
> Jan

[Ralf Ramsauer]

I'd like to.

But this changes the existing jailhouse sysfs interface. Are you sure
that no one relies on the structure? Symlinks could keep compatibility,
but this gets problematic as soon as someone has the great idea to
choose '0' as cell name.

So you don't want to use the sysfs interface to expose human-readable
bitmasks? Then bitmasks will have to be parsed in userspace.

Cell ids do not necessarily have to be continuous, right? Then I still
have to iterate over the cells/ directory.

[Ralf Ramsauer]

Ok, let's drop this one. Just a personal flavour.

[Jan Kiszka]

Not *easily* (it requires lots of extra logic) - I suppose you can't
disagree here.

Jan

[Jan Kiszka]

I don't consider anything in Jailhouse ultimately stable yet. If we can
avoid breakages, we should, but if it is about cleaning something up, we
may also break backward compatibility. Up to version 1.0, I would say.

>
> So you don't want to use the sysfs interface to expose human-readable
> bitmasks? Then bitmasks will have to be parsed in userspace.

See my reply on patch 5.

>
> Cell ids do not necessarily have to be continuous, right? Then I still
> have to iterate over the cells/ directory.

Try scandir(3) - should allow to read and print in a single loop.

Jan

[Ralf Ramsauer]

Hi all,

I'd like to replace jailhouse-cell-list. It's a rudimentary function and
requires python. So I reimplemented cell-list natively in C and did some
cleanup work on tools/jailhouse.c.

This series also changes the syfs structure as Jan proposed.

changes since v1:
- change sysfs structure (swap id and name)
- provide human readable sysfs nodes
- improve changes on read_file()

Ralf Ramsauer (8):
driver: cell: add jailhouse_cell_id member to struct cell
driver: sysfs: do not pass cell name to jh_sysfs_cell_create
driver: sysfs: swap name and id
driver: sysfs: consolidate cpumask printing
driver: sysfs: consolidate failed_cpus to extra function
driver:sysfs add new sysfs nodes cpus_{assigned,failed}_list
tools: jailhouse: do not use struct stat to predict buffer size
tools: jailhouse: reimplement jailhouse-cell-list in C

Documentation/sysfs-entries.txt | 4 +-
driver/cell.c | 16 +++--
driver/cell.h | 2 +-
driver/sysfs.c | 114 ++++++++++++++++++++-----------
driver/sysfs.h | 2 +-
tools/jailhouse-cell-list | 71 --------------------
tools/jailhouse.c | 145 ++++++++++++++++++++++++++++++++++------
7 files changed, 214 insertions(+), 140 deletions(-)
delete mode 100755 tools/jailhouse-cell-list

--
2.9.2

[Ralf Ramsauer]

struct cell did not know the cell name.

struct jailhouse_cell_id provides place for both: the cell id and the
cell name. Exchange the id by struct jailhouse_cell_id.

This patch is preparatory work for exchanging the jailhouse sysfs cell
structure.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>
---

driver/cell.c | 14 ++++++++------
driver/cell.h | 2 +-
driver/sysfs.c | 4 ++--
3 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/driver/cell.c b/driver/cell.c
index 05ac7b2..c317c0e 100644
--- a/driver/cell.c
+++ b/driver/cell.c
@@ -64,6 +64,8 @@ struct cell *jailhouse_cell_create(const struct jailhouse_cell_desc *cell_desc)
return ERR_PTR(-ENOMEM);
}

+ memcpy(cell->id.name, cell_desc->name, JAILHOUSE_CELL_NAME_MAXLEN+1);
+
memcpy(cell->memory_regions, jailhouse_cell_mem_regions(cell_desc),
sizeof(struct jailhouse_memory) * cell->num_memory_regions);

@@ -93,7 +95,7 @@ static struct cell *find_cell(struct jailhouse_cell_id *cell_id)
struct cell *cell;

list_for_each_entry(cell, &cells, entry)
- if (cell_id->id == cell->id ||
+ if (cell_id->id == cell->id.id ||
(cell_id->id == JAILHOUSE_CELL_ID_UNUSED &&
strcmp(kobject_name(&cell->kobj), cell_id->name) == 0))
return cell;
@@ -123,7 +125,7 @@ void jailhouse_cell_register_root(void)
jailhouse_pci_do_all_devices(root_cell, JAILHOUSE_PCI_TYPE_IVSHMEM,
JAILHOUSE_PCI_ACTION_ADD);

- root_cell->id = 0;
+ root_cell->id.id = 0;
jailhouse_cell_register(root_cell);
}

@@ -233,7 +235,7 @@ int jailhouse_cmd_cell_create(struct jailhouse_cell_create __user *arg)
goto error_cpu_online;
}

- cell->id = id;
+ cell->id.id = id;
jailhouse_cell_register(cell);

pr_info("Created Jailhouse cell \"%s\"\n", config->name);
@@ -353,7 +355,7 @@ int jailhouse_cmd_cell_load(struct jailhouse_cell_load __user *arg)
if (err)
return err;

- err = jailhouse_call_arg1(JAILHOUSE_HC_CELL_SET_LOADABLE, cell->id);
+ err = jailhouse_call_arg1(JAILHOUSE_HC_CELL_SET_LOADABLE, cell->id.id);
if (err)
goto unlock_out;

@@ -382,7 +384,7 @@ int jailhouse_cmd_cell_start(const char __user *arg)
if (err)
return err;

- err = jailhouse_call_arg1(JAILHOUSE_HC_CELL_START, cell->id);
+ err = jailhouse_call_arg1(JAILHOUSE_HC_CELL_START, cell->id.id);

mutex_unlock(&jailhouse_lock);

@@ -403,7 +405,7 @@ int jailhouse_cmd_cell_destroy(const char __user *arg)
if (err)
return err;

- err = jailhouse_call_arg1(JAILHOUSE_HC_CELL_DESTROY, cell->id);
+ err = jailhouse_call_arg1(JAILHOUSE_HC_CELL_DESTROY, cell->id.id);
if (err)
goto unlock_out;

diff --git a/driver/cell.h b/driver/cell.h
index 1ee5a48..6ea9c0d 100644
--- a/driver/cell.h
+++ b/driver/cell.h
@@ -25,7 +25,7 @@
struct cell {
struct kobject kobj;
struct list_head entry;
- unsigned int id;
+ struct jailhouse_cell_id id;
cpumask_t cpus_assigned;
u32 num_memory_regions;
struct jailhouse_memory *memory_regions;
diff --git a/driver/sysfs.c b/driver/sysfs.c
index 4959b56..7833c7b 100644
--- a/driver/sysfs.c
+++ b/driver/sysfs.c
@@ -144,7 +144,7 @@ static ssize_t id_show(struct kobject *kobj, struct kobj_attribute *attr,
{
struct cell *cell = container_of(kobj, struct cell, kobj);

- return sprintf(buffer, "%u\n", cell->id);
+ return sprintf(buffer, "%d\n", cell->id.id);
}

static ssize_t state_show(struct kobject *kobj, struct kobj_attribute *attr,
@@ -152,7 +152,7 @@ static ssize_t state_show(struct kobject *kobj, struct kobj_attribute *attr,
{
struct cell *cell = container_of(kobj, struct cell, kobj);

- switch (jailhouse_call_arg1(JAILHOUSE_HC_CELL_GET_STATE, cell->id)) {
+ switch (jailhouse_call_arg1(JAILHOUSE_HC_CELL_GET_STATE, cell->id.id)) {
case JAILHOUSE_CELL_RUNNING:
return sprintf(buffer, "running\n");
case JAILHOUSE_CELL_RUNNING_LOCKED:
--
2.9.2

[Ralf Ramsauer]

struct cell now has information about the cell name. It is not necessary
to pass the name of the cell to jh_sysfs_cell_create.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>
---

driver/cell.c | 2 +-
driver/sysfs.c | 4 ++--
driver/sysfs.h | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/driver/cell.c b/driver/cell.c
index c317c0e..b0aceda 100644
--- a/driver/cell.c
+++ b/driver/cell.c
@@ -76,7 +76,7 @@ struct cell *jailhouse_cell_create(const struct jailhouse_cell_desc *cell_desc)
return ERR_PTR(err);
}

- err = jailhouse_sysfs_cell_create(cell, cell_desc->name);
+ err = jailhouse_sysfs_cell_create(cell);
if (err)
/* cleanup done by jailhouse_sysfs_cell_create */
return ERR_PTR(err);
diff --git a/driver/sysfs.c b/driver/sysfs.c
index 7833c7b..5be994d 100644
--- a/driver/sysfs.c
+++ b/driver/sysfs.c
@@ -232,12 +232,12 @@ static struct kobj_type cell_type = {
.default_attrs = cell_attrs,
};

-int jailhouse_sysfs_cell_create(struct cell *cell, const char *name)
+int jailhouse_sysfs_cell_create(struct cell *cell)
{
int err;

err = kobject_init_and_add(&cell->kobj, &cell_type, cells_dir, "%s",
- name);
+ cell->id.name);
if (err) {
jailhouse_cell_kobj_release(&cell->kobj);
return err;
diff --git a/driver/sysfs.h b/driver/sysfs.h
index ad0e2f9..f5cdc34 100644
--- a/driver/sysfs.h
+++ b/driver/sysfs.h
@@ -15,7 +15,7 @@

#include <linux/device.h>

-int jailhouse_sysfs_cell_create(struct cell *cell, const char *name);
+int jailhouse_sysfs_cell_create(struct cell *cell);
void jailhouse_sysfs_cell_register(struct cell *cell);
void jailhouse_sysfs_cell_delete(struct cell *cell);

--
2.9.2

[Ralf Ramsauer]

Directories in /sys/jailhouse/cells/ formerly represented cell names.

This patch changes the structure. From now on, /sys/jailhouse/cells/
will contain cell ids. The name of a specific cell is provided through
/sys/jailhouse/cells/<id>/name.

This makes the sysfs interface easier to be parsed by applications.

Additionally, amend documentation and jailhouse-cell-list.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>
---

Documentation/sysfs-entries.txt | 4 ++--
driver/sysfs.c | 14 +++++++-------
tools/jailhouse-cell-list | 4 ++--
3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/Documentation/sysfs-entries.txt b/Documentation/sysfs-entries.txt
index 2845220..0fc315a 100644
--- a/Documentation/sysfs-entries.txt
+++ b/Documentation/sysfs-entries.txt
@@ -11,8 +11,8 @@ can be used for monitoring the state of the hypervisor and its cells.
|- remap_pool_size - number of pages in hypervisor remapping pool
|- remap_pool_used - used pages of hypervisor remapping pool
`- cells
- |- <name of cell>
- | |- id - unique numerical ID
+ |- <id> - unique numerical ID
+ | |- name - cell name
| |- state - "running", "running/locked", "shut down", or
| | "failed"
| |- cpus_assigned - bitmask of assigned logical CPUs

diff --git a/driver/sysfs.c b/driver/sysfs.c
index 5be994d..4910b42 100644
--- a/driver/sysfs.c
+++ b/driver/sysfs.c
@@ -139,12 +139,12 @@ static struct attribute_group stats_attr_group = {
.name = "statistics"
};

-static ssize_t id_show(struct kobject *kobj, struct kobj_attribute *attr,
- char *buffer)
+static ssize_t name_show(struct kobject *kobj, struct kobj_attribute *attr,
+ char *buffer)

{
struct cell *cell = container_of(kobj, struct cell, kobj);

- return sprintf(buffer, "%d\n", cell->id.id);
+ return sprintf(buffer, "%s\n", cell->id.name);

}

static ssize_t state_show(struct kobject *kobj, struct kobj_attribute *attr,

@@ -212,14 +212,14 @@ static ssize_t cpus_failed_show(struct kobject *kobj,
return written;
}

-static struct kobj_attribute cell_id_attr = __ATTR_RO(id);
+static struct kobj_attribute cell_name_attr = __ATTR_RO(name);
static struct kobj_attribute cell_state_attr = __ATTR_RO(state);
static struct kobj_attribute cell_cpus_assigned_attr =
__ATTR_RO(cpus_assigned);
static struct kobj_attribute cell_cpus_failed_attr = __ATTR_RO(cpus_failed);

static struct attribute *cell_attrs[] = {
- &cell_id_attr.attr,
+ &cell_name_attr.attr,
&cell_state_attr.attr,
&cell_cpus_assigned_attr.attr,
&cell_cpus_failed_attr.attr,
@@ -236,8 +236,8 @@ int jailhouse_sysfs_cell_create(struct cell *cell)
{
int err;

- err = kobject_init_and_add(&cell->kobj, &cell_type, cells_dir, "%s",
- cell->id.name);
+ err = kobject_init_and_add(&cell->kobj, &cell_type, cells_dir, "%d",
+ cell->id.id);

if (err) {
jailhouse_cell_kobj_release(&cell->kobj);
return err;

diff --git a/tools/jailhouse-cell-list b/tools/jailhouse-cell-list
index 3b5bc20..d4123f6 100755
--- a/tools/jailhouse-cell-list
+++ b/tools/jailhouse-cell-list
@@ -54,8 +54,8 @@ if len(sys.argv) > 1:
cells = []

for cell_path in glob.glob('/sys/devices/jailhouse/cells/*'):

cells.append({
- 'name': os.path.basename(cell_path),
- 'id': open(cell_path + "/id").readline().strip(),

+ 'id': os.path.basename(cell_path),
+ 'name': open(cell_path + "/name").readline().strip(),

'state': open(cell_path + "/state").readline().strip(),

'cpus_assigned': read_cpus(cell_path + "/cpus_assigned"),

'cpus_failed': read_cpus(cell_path + "/cpus_failed")

--
2.9.2

[Ralf Ramsauer]

We want to add two additional sysfs nodes for each cell:
cpus_assigned_list and cpus_failed_list.

A lot of printing-work would be redundant. As preparatory work,
consolidate it and add an additional as_list flag to allow
human readable cpumasks representation.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>
---

driver/sysfs.c | 39 +++++++++++++++++++++++----------------
1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/driver/sysfs.c b/driver/sysfs.c
index 4910b42..ac4ae76 100644
--- a/driver/sysfs.c
+++ b/driver/sysfs.c
@@ -139,6 +139,27 @@ static struct attribute_group stats_attr_group = {
.name = "statistics"
};

+static int print_cpumask(char *buf, size_t size, cpumask_t *mask, bool as_list)
+{
+ int written;
+
+ if (as_list)
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,0,0)
+ written = scnprintf(buf, size, "%*pbl\n",
+ cpumask_pr_args(mask));
+ else
+ written = scnprintf(buf, size, "%*pb\n",
+ cpumask_pr_args(mask));
+#else
+ written = cpulist_scnprintf(buf, size, mask);
+ else
+ written = cpumask_scnprintf(buf, size, mask);
+ written += scnprintf(buf + written, size - written, "\n");
+#endif
+
+ return written;
+}
+

static ssize_t name_show(struct kobject *kobj, struct kobj_attribute *attr,

char *buffer)
{
@@ -170,16 +191,8 @@ static ssize_t cpus_assigned_show(struct kobject *kobj,
struct kobj_attribute *attr, char *buf)

{
struct cell *cell = container_of(kobj, struct cell, kobj);

- int written;

-#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,0,0)

- written = scnprintf(buf, PAGE_SIZE, "%*pb\n",

- cpumask_pr_args(&cell->cpus_assigned));
-#else

- written = cpumask_scnprintf(buf, PAGE_SIZE, &cell->cpus_assigned);

- written += scnprintf(buf + written, PAGE_SIZE - written, "\n");
-#endif
- return written;
+ return print_cpumask(buf, PAGE_SIZE, &cell->cpus_assigned, false);

}

static ssize_t cpus_failed_show(struct kobject *kobj,

@@ -199,13 +212,7 @@ static ssize_t cpus_failed_show(struct kobject *kobj,
JAILHOUSE_CPU_FAILED)
cpumask_set_cpu(cpu, cpus_failed);

-#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,0,0)

- written = scnprintf(buf, PAGE_SIZE, "%*pb\n",

- cpumask_pr_args(cpus_failed));
-#else

- written = cpumask_scnprintf(buf, PAGE_SIZE, cpus_failed);

- written += scnprintf(buf + written, PAGE_SIZE - written, "\n");
-#endif
+ written = print_cpumask(buf, PAGE_SIZE, cpus_failed, false);

free_cpumask_var(cpus_failed);

--
2.9.2

[Ralf Ramsauer]

consolidate printing of failed_cpus to extra function and provide an
as_list format flag.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>
---

driver/sysfs.c | 41 ++++++++++++++++++++++++-----------------
1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/driver/sysfs.c b/driver/sysfs.c
index ac4ae76..7761bb1 100644
--- a/driver/sysfs.c
+++ b/driver/sysfs.c
@@ -160,6 +160,29 @@ static int print_cpumask(char *buf, size_t size, cpumask_t *mask, bool as_list)
return written;
}

+static int print_failed_cpus(char *buf, size_t size, const struct cell *cell,
+ bool as_list)
+{
+ cpumask_var_t cpus_failed;
+ unsigned int cpu;
+ int written;
+
+ if (!zalloc_cpumask_var(&cpus_failed, GFP_KERNEL))
+ return -ENOMEM;
+
+ for_each_cpu(cpu, &cell->cpus_assigned)
+ if (jailhouse_call_arg2(JAILHOUSE_HC_CPU_GET_INFO, cpu,
+ JAILHOUSE_CPU_INFO_STATE) ==
+ JAILHOUSE_CPU_FAILED)
+ cpumask_set_cpu(cpu, cpus_failed);
+
+ written = print_cpumask(buf, size, cpus_failed, as_list);
+
+ free_cpumask_var(cpus_failed);

+
+ return written;
+}
+
static ssize_t name_show(struct kobject *kobj, struct kobj_attribute *attr,
char *buffer)
{

@@ -199,24 +222,8 @@ static ssize_t cpus_failed_show(struct kobject *kobj,

struct kobj_attribute *attr, char *buf)
{
struct cell *cell = container_of(kobj, struct cell, kobj);

- cpumask_var_t cpus_failed;
- unsigned int cpu;
- int written;
-
- if (!zalloc_cpumask_var(&cpus_failed, GFP_KERNEL))
- return -ENOMEM;
-
- for_each_cpu(cpu, &cell->cpus_assigned)
- if (jailhouse_call_arg2(JAILHOUSE_HC_CPU_GET_INFO, cpu,
- JAILHOUSE_CPU_INFO_STATE) ==
- JAILHOUSE_CPU_FAILED)
- cpumask_set_cpu(cpu, cpus_failed);

- written = print_cpumask(buf, PAGE_SIZE, cpus_failed, false);
-
- free_cpumask_var(cpus_failed);
-
- return written;
+ return print_failed_cpus(buf, PAGE_SIZE, cell, false);

}

static struct kobj_attribute cell_name_attr = __ATTR_RO(name);

--
2.9.2

[Ralf Ramsauer]

Add human readable nodes for cpus_assigned and cpus_failed and let
jailhouse-cell-list use those nodes.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>
---

driver/sysfs.c | 22 ++++++++++++++++++++++
tools/jailhouse-cell-list | 41 +++++------------------------------------
2 files changed, 27 insertions(+), 36 deletions(-)

diff --git a/driver/sysfs.c b/driver/sysfs.c
index 7761bb1..a841ce8 100644
--- a/driver/sysfs.c
+++ b/driver/sysfs.c
@@ -218,6 +218,14 @@ static ssize_t cpus_assigned_show(struct kobject *kobj,

return print_cpumask(buf, PAGE_SIZE, &cell->cpus_assigned, false);
}

+static ssize_t cpus_assigned_list_show(struct kobject *kobj,
+ struct kobj_attribute *attr, char *buf)
+{
+ struct cell *cell = container_of(kobj, struct cell, kobj);
+
+ return print_cpumask(buf, PAGE_SIZE, &cell->cpus_assigned, true);
+}
+

static ssize_t cpus_failed_show(struct kobject *kobj,
struct kobj_attribute *attr, char *buf)
{

@@ -226,17 +234,31 @@ static ssize_t cpus_failed_show(struct kobject *kobj,

return print_failed_cpus(buf, PAGE_SIZE, cell, false);
}

+static ssize_t cpus_failed_list_show(struct kobject *kobj,
+ struct kobj_attribute *attr, char *buf)
+{
+ struct cell *cell = container_of(kobj, struct cell, kobj);
+
+ return print_failed_cpus(buf, PAGE_SIZE, cell, true);
+}
+

static struct kobj_attribute cell_name_attr = __ATTR_RO(name);

static struct kobj_attribute cell_state_attr = __ATTR_RO(state);
static struct kobj_attribute cell_cpus_assigned_attr =
__ATTR_RO(cpus_assigned);

+static struct kobj_attribute cell_cpus_assigned_list_attr =
+ __ATTR_RO(cpus_assigned_list);

static struct kobj_attribute cell_cpus_failed_attr = __ATTR_RO(cpus_failed);

+static struct kobj_attribute cell_cpus_failed_list_attr =
+ __ATTR_RO(cpus_failed_list);

static struct attribute *cell_attrs[] = {

&cell_name_attr.attr,
&cell_state_attr.attr,
&cell_cpus_assigned_attr.attr,
+ &cell_cpus_assigned_list_attr.attr,
&cell_cpus_failed_attr.attr,
+ &cell_cpus_failed_list_attr.attr,
NULL,
};

diff --git a/tools/jailhouse-cell-list b/tools/jailhouse-cell-list
index d4123f6..d5a535b 100755
--- a/tools/jailhouse-cell-list
+++ b/tools/jailhouse-cell-list
@@ -15,38 +15,6 @@ import os
import sys

if len(sys.argv) > 1:

print("usage: %s" % os.path.basename(sys.argv[0]).replace("-", " "))

exit(0 if sys.argv[1] in ("--help", "-h") else 1)

@@ -57,8 +25,10 @@ for cell_path in glob.glob('/sys/devices/jailhouse/cells/*'):
'id': os.path.basename(cell_path),

'name': open(cell_path + "/name").readline().strip(),
'state': open(cell_path + "/state").readline().strip(),

- 'cpus_assigned': read_cpus(cell_path + "/cpus_assigned"),
- 'cpus_failed': read_cpus(cell_path + "/cpus_failed")
+ 'cpus_assigned_list':
+ open(cell_path + "/cpus_assigned_list").readline().strip(),
+ 'cpus_failed_list':
+ open(cell_path + "/cpus_failed_list").readline().strip()
})

line_format = "%-8s%-24s%-16s%-24s%-24s"
@@ -67,5 +37,4 @@ if not cells == []:

"Assigned CPUs", "Failed CPUs"))

for cell in sorted(cells, key=lambda cell: cell['id']):

print(line_format % (cell['id'], cell['name'], cell['state'],
- print_cpus(cell['cpus_assigned']),
- print_cpus(cell['cpus_failed'])))

+ cell['cpus_assigned_list'], cell['cpus_failed_list']))
--
2.9.2

[Ralf Ramsauer]

struct stat may predict a misleading st_size if procfs or sysfs is involved.

Preallocate a memory array and repeatedly read() from the descriptor until
EOF is reached. Reallocate memory by steps of CHUNK_SIZE.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>
---

tools/jailhouse.c | 37 +++++++++++++++++++------------------
1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/tools/jailhouse.c b/tools/jailhouse.c
index be5b833..4cf36b6 100644
--- a/tools/jailhouse.c
+++ b/tools/jailhouse.c
@@ -21,13 +21,14 @@
#include <libgen.h>
#include <sys/types.h>
#include <sys/ioctl.h>
-#include <sys/stat.h>

#include <jailhouse.h>

#define JAILHOUSE_EXEC_DIR LIBEXECDIR "/jailhouse"
#define JAILHOUSE_DEVICE "/dev/jailhouse"

+#define CHUNK_SIZE 1024
+
enum shutdown_load_mode {LOAD, SHUTDOWN};

struct extension {
@@ -127,9 +128,10 @@ static void *read_string(const char *string, size_t *size)

static void *read_file(const char *name, size_t *size)
{
- struct stat stat;
- void *buffer;
+ void *buffer = NULL;
int fd;
+ size_t size_total = 0;
+ ssize_t chunk;

fd = open(name, O_RDONLY);
if (fd < 0) {

@@ -137,26 +139,25 @@ static void *read_file(const char *name, size_t *size)
exit(1);
}

- if (fstat(fd, &stat) < 0) {
- perror("fstat");
- exit(1);
- }
-
- buffer = malloc(stat.st_size);
- if (!buffer) {
- fprintf(stderr, "insufficient memory\n");
- exit(1);
- }
+ do {
+ buffer = realloc(buffer, size_total + CHUNK_SIZE);
+ if (buffer == NULL) {
+ fprintf(stderr, "insufficient memory\n");
+ exit(1);
+ }

- if (read(fd, buffer, stat.st_size) < stat.st_size) {

- fprintf(stderr, "reading %s: %s\n", name, strerror(errno));
- exit(1);
- }
+ chunk = read(fd, buffer + size_total, CHUNK_SIZE);
+ if (chunk == -1) {
+ perror("read");
+ exit(1);
+ }
+ size_total += chunk;
+ } while(chunk == CHUNK_SIZE);

close(fd);

if (size)
- *size = stat.st_size;

+ *size = size_total;

return buffer;
}
--
2.9.2

[Ralf Ramsauer]

And keep the output format of the former python script.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>
---

tools/jailhouse-cell-list | 40 -----------------
tools/jailhouse.c | 108 +++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 107 insertions(+), 41 deletions(-)
delete mode 100755 tools/jailhouse-cell-list

diff --git a/tools/jailhouse-cell-list b/tools/jailhouse-cell-list
deleted file mode 100755

index d5a535b..0000000
--- a/tools/jailhouse-cell-list
+++ /dev/null
@@ -1,40 +0,0 @@

-#!/usr/bin/env python
-
-# Jailhouse, a Linux-based partitioning hypervisor
-#
-# Copyright (c) Siemens AG, 2014
-#
-# Authors:
-# Jan Kiszka <jan.k...@siemens.com>
-#
-# This work is licensed under the terms of the GNU GPL, version 2. See
-# the COPYING file in the top-level directory.
-
-import glob
-import os
-import sys
-
-

-if len(sys.argv) > 1:
- print("usage: %s" % os.path.basename(sys.argv[0]).replace("-", " "))
- exit(0 if sys.argv[1] in ("--help", "-h") else 1)
-
-cells = []
-for cell_path in glob.glob('/sys/devices/jailhouse/cells/*'):
- cells.append({
- 'id': os.path.basename(cell_path),
- 'name': open(cell_path + "/name").readline().strip(),
- 'state': open(cell_path + "/state").readline().strip(),
- 'cpus_assigned_list':
- open(cell_path + "/cpus_assigned_list").readline().strip(),
- 'cpus_failed_list':
- open(cell_path + "/cpus_failed_list").readline().strip()

- })
-
-line_format = "%-8s%-24s%-16s%-24s%-24s"
-if not cells == []:
- print(line_format % ("ID", "Name", "State",

- "Assigned CPUs", "Failed CPUs"))
-for cell in sorted(cells, key=lambda cell: cell['id']):
- print(line_format % (cell['id'], cell['name'], cell['state'],
- cell['cpus_assigned_list'], cell['cpus_failed_list']))
diff --git a/tools/jailhouse.c b/tools/jailhouse.c
index 4cf36b6..07a31cc 100644
--- a/tools/jailhouse.c
+++ b/tools/jailhouse.c

@@ -14,6 +14,7 @@
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
+#include <dirent.h>
#include <unistd.h>
#include <fcntl.h>
#include <errno.h>

@@ -26,6 +27,7 @@

#define JAILHOUSE_EXEC_DIR LIBEXECDIR "/jailhouse"
#define JAILHOUSE_DEVICE "/dev/jailhouse"

+#define JAILHOUSE_CELLS "/sys/devices/jailhouse/cells/"

#define CHUNK_SIZE 1024

@@ -35,11 +37,17 @@ struct extension {
char *cmd, *subcmd, *help;
};

+struct jailhouse_cell_info {
+ struct jailhouse_cell_id id;
+ char *state;

+ char *cpus_assigned_list;
+ char *cpus_failed_list;

+};
+
static const struct extension extensions[] = {
{ "cell", "linux", "CELLCONFIG KERNEL [-i | --initrd FILE]\n"
" [-c | --cmdline \"STRING\"] "
"[-w | --write-params FILE]" },
- { "cell", "list", "" },
{ "cell", "stats", "{ ID | [--name] NAME }" },
{ "config", "create", "[-h] [-g] [-r ROOT] "
"[--mem-inmates MEM_INMATES]\n"
@@ -58,6 +66,7 @@ static void __attribute__((noreturn)) help(char *prog, int exit_status)
" enable SYSCONFIG\n"
" disable\n"
" cell create CELLCONFIG\n"
+ " cell list\n"
" cell load { ID | [--name] NAME } "
"{ IMAGE | { -s | --string } \"STRING\" }\n"
" [-a | --address ADDRESS] ...\n"

@@ -162,6 +171,20 @@ static void *read_file(const char *name, size_t *size)

return buffer;
}

+static char *read_sysfs_entry(const char *name)
+{
+ char *ret;
+ size_t size;
+

+ ret = read_file(name, &size);

+
+ /* enforce the string to be null-terminated */
+ if (size && ret[size-1] != '\0')
+ ret[size-1] = '\0';
+
+ return ret;
+}
+
static int enable(int argc, char *argv[])
{
void *config;

@@ -250,6 +273,87 @@ static bool match_opt(const char *argv, const char *short_opt,
strcmp(argv, long_opt) == 0;
}

+static struct jailhouse_cell_info *get_cell_info(const int id)
+{
+ char buf[sizeof(JAILHOUSE_CELLS) + 3 + sizeof("/cpus_assigned_list") + 1];

+ struct jailhouse_cell_info *cinfo;
+ char *tmp;
+
+ cinfo = calloc(1, sizeof(struct jailhouse_cell_info));
+

+ /* set cell id */
+ cinfo->id.id = id;
+
+ /* get cell name */
+ snprintf(buf, sizeof(buf), JAILHOUSE_CELLS "%d/name", id);
+ tmp = read_sysfs_entry(buf);
+ strncpy(cinfo->id.name, tmp, JAILHOUSE_CELL_ID_NAMELEN);

+ free(tmp);
+
+ /* get cell state */

+ snprintf(buf, sizeof(buf), JAILHOUSE_CELLS "%d/state", id);

+ cinfo->state = read_sysfs_entry(buf);
+

+ /* get assigned cpu list */
+ snprintf(buf, sizeof(buf), JAILHOUSE_CELLS "%d/cpus_assigned_list", id);
+ cinfo->cpus_assigned_list = read_sysfs_entry(buf);
+
+ /* get failed cpu list */
+ snprintf(buf, sizeof(buf), JAILHOUSE_CELLS "%d/cpus_failed_list", id);
+ cinfo->cpus_failed_list = read_sysfs_entry(buf);

+
+ return cinfo;
+}
+
+static void cell_info_free(struct jailhouse_cell_info *cinfo)
+{
+ free(cinfo->state);

+ free(cinfo->cpus_assigned_list);
+ free(cinfo->cpus_failed_list);
+ free(cinfo);
+}
+

+static int cell_list(int argc, char *argv[])
+{

+ DIR *dir;
+ struct dirent *dent;

+ struct jailhouse_cell_info *cinfo;
+ int id;

+ (void)argv;
+
+ if (argc != 3)
+ help(argv[0], 1);
+

+ printf("%-8s%-24s%-16s%-24s%-24s\n",

+ "ID", "Name", "State", "Assigned CPUs", "Failed CPUs");

+
+ dir = opendir(JAILHOUSE_CELLS);
+ if (dir == NULL) {
+ perror("opendir " JAILHOUSE_CELLS);
+ exit(1);
+ }
+
+ while ((dent = readdir(dir)) != NULL) {

+ /* Skip '.' and '..' */

+ if (dent->d_name[0] == '.')
+ continue;
+

+ id = (int)strtol(dent->d_name, NULL, 10);
+
+ cinfo = get_cell_info(id);

+ printf("%-8d%-24s%-16s%-24s%-24s\n", cinfo->id.id, cinfo->id.name,

+ cinfo->state, cinfo->cpus_assigned_list, cinfo->cpus_failed_list);
+ cell_info_free(cinfo);

+ }
+
+ if (closedir(dir)) {
+ perror("closedir " JAILHOUSE_CELLS);
+ exit(1);
+ }
+

+ return 0;
+}
+
static int cell_shutdown_load(int argc, char *argv[],
enum shutdown_load_mode mode)
{

@@ -368,6 +472,8 @@ static int cell_management(int argc, char *argv[])

if (strcmp(argv[2], "create") == 0) {

err = cell_create(argc, argv);
+ } else if (strcmp(argv[2], "list") == 0) {
+ err = cell_list(argc, argv);

} else if (strcmp(argv[2], "load") == 0) {

err = cell_shutdown_load(argc, argv, LOAD);

} else if (strcmp(argv[2], "start") == 0) {

--
2.9.2

[Jan Kiszka]

On 2016-08-07 19:41, Ralf Ramsauer wrote:
> struct cell did not know the cell name.
>
> struct jailhouse_cell_id provides place for both: the cell id and the
> cell name. Exchange the id by struct jailhouse_cell_id.
>
> This patch is preparatory work for exchanging the jailhouse sysfs cell
> structure.
>
> Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>
> ---
> driver/cell.c | 14 ++++++++------
> driver/cell.h | 2 +-
> driver/sysfs.c | 4 ++--
> 3 files changed, 11 insertions(+), 9 deletions(-)
>

...

> diff --git a/driver/cell.h b/driver/cell.h
> index 1ee5a48..6ea9c0d 100644
> --- a/driver/cell.h
> +++ b/driver/cell.h
> @@ -25,7 +25,7 @@
> struct cell {
> struct kobject kobj;
> struct list_head entry;
> - unsigned int id;
> + struct jailhouse_cell_id id;

Let's unfold this into id and name. You do not make use of the struct as
a whole, and "id.id" reads funny.

Jan

[Jan Kiszka]

On 2016-08-07 19:41, Ralf Ramsauer wrote:

> consolidate printing of failed_cpus to extra function and provide an
> as_list format flag.

Nit: This is actually no "consolidation" - you only factor out a helper
that has a just one user, at least at this point.

Jan

[Jan Kiszka]

On 2016-08-07 19:41, Ralf Ramsauer wrote:

> Add human readable nodes for cpus_assigned and cpus_failed and let
> jailhouse-cell-list use those nodes.
>
> Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>
> ---
> driver/sysfs.c | 22 ++++++++++++++++++++++
> tools/jailhouse-cell-list | 41 +++++------------------------------------
> 2 files changed, 27 insertions(+), 36 deletions(-)
>

...

And it took me so long to come up with this logic...

Already a value of its own, this patch.

Jan

[Jan Kiszka]

On 2016-08-07 19:41, Ralf Ramsauer wrote:

> struct stat may predict a misleading st_size if procfs or sysfs is involved.

Can it also be too small? If not, I don't see why allocating iteratively.

For /sys/devices/jailhouse, using 4K as minimum buffer size will
practically avoid the loop.

>
> Preallocate a memory array and repeatedly read() from the descriptor until
> EOF is reached. Reallocate memory by steps of CHUNK_SIZE.
>
> Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>
> ---
> tools/jailhouse.c | 37 +++++++++++++++++++------------------
> 1 file changed, 19 insertions(+), 18 deletions(-)
>
> diff --git a/tools/jailhouse.c b/tools/jailhouse.c
> index be5b833..4cf36b6 100644
> --- a/tools/jailhouse.c
> +++ b/tools/jailhouse.c
> @@ -21,13 +21,14 @@
> #include <libgen.h>
> #include <sys/types.h>
> #include <sys/ioctl.h>
> -#include <sys/stat.h>
>
> #include <jailhouse.h>
>
> #define JAILHOUSE_EXEC_DIR LIBEXECDIR "/jailhouse"
> #define JAILHOUSE_DEVICE "/dev/jailhouse"
>
> +#define CHUNK_SIZE 1024

Might be a bit too small.

> +
> enum shutdown_load_mode {LOAD, SHUTDOWN};
>
> struct extension {
> @@ -127,9 +128,10 @@ static void *read_string(const char *string, size_t *size)
>
> static void *read_file(const char *name, size_t *size)
> {
> - struct stat stat;
> - void *buffer;
> + void *buffer = NULL;
> int fd;
> + size_t size_total = 0;
> + ssize_t chunk;
>
> fd = open(name, O_RDONLY);
> if (fd < 0) {
> @@ -137,26 +139,25 @@ static void *read_file(const char *name, size_t *size)
> exit(1);
> }
>
> - if (fstat(fd, &stat) < 0) {
> - perror("fstat");
> - exit(1);
> - }

Why not keeping this as a good hint for the initial allocation?

[Jan Kiszka]

On 2016-08-07 19:41, Ralf Ramsauer wrote:

> And keep the output format of the former python script.
>

Not yet using scandir with alphasort- why?

Jan

[Ralf Ramsauer]

On 08/07/2016 09:41 PM, Jan Kiszka wrote:
> On 2016-08-07 19:41, Ralf Ramsauer wrote:
>> struct cell did not know the cell name.
>>
>> struct jailhouse_cell_id provides place for both: the cell id and the
>> cell name. Exchange the id by struct jailhouse_cell_id.
>>
>> This patch is preparatory work for exchanging the jailhouse sysfs cell
>> structure.
>>
>> Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>
>> ---
>> driver/cell.c | 14 ++++++++------
>> driver/cell.h | 2 +-
>> driver/sysfs.c | 4 ++--
>> 3 files changed, 11 insertions(+), 9 deletions(-)
>>
>
> ...

git am skips this. I'll leave out diffstats in future.

>
>> diff --git a/driver/cell.h b/driver/cell.h
>> index 1ee5a48..6ea9c0d 100644
>> --- a/driver/cell.h
>> +++ b/driver/cell.h
>> @@ -25,7 +25,7 @@
>> struct cell {
>> struct kobject kobj;
>> struct list_head entry;
>> - unsigned int id;
>> + struct jailhouse_cell_id id;
>
> Let's unfold this into id and name. You do not make use of the struct as
> a whole, and "id.id" reads funny.

Ok, but there's only the padding left in struct jailhouse_cell_id.

Allocate the name statically or dynamically?

Ralf
>
> Jan

[Ralf Ramsauer]

Hehe, I'm sorry.

Offtopic:
Before offloading this task to sysfs I thought about implementing this
logic in C and I realised that this could be implemented as a single
function:
http://pastebin.com/diTUftVr

[Ralf Ramsauer]

Does order matter? I'll change it in the next round.
Ralf

[Ralf Ramsauer]

On 08/07/2016 09:52 PM, Jan Kiszka wrote:
> On 2016-08-07 19:41, Ralf Ramsauer wrote:
>> struct stat may predict a misleading st_size if procfs or sysfs is involved.
>
> Can it also be too small? If not, I don't see why allocating iteratively.
>
> For /sys/devices/jailhouse, using 4K as minimum buffer size will
> practically avoid the loop.

Yes, but regarding sysfs files, fstat returns 4096 for st_size and
read() returns only actually set bytes.

Content in /sys/devices/jailhouse is usually smaller than 1024 bytes, so
why wasting too much space?

You can't avoid the loop as regular files might be bigger than 4K.
Config files which are a regular files are read with this function as well.
Ralf

[Jan Kiszka]

On 2016-08-07 22:07, Ralf Ramsauer wrote:
> On 08/07/2016 09:41 PM, Jan Kiszka wrote:
>> On 2016-08-07 19:41, Ralf Ramsauer wrote:
>>> struct cell did not know the cell name.
>>>
>>> struct jailhouse_cell_id provides place for both: the cell id and the
>>> cell name. Exchange the id by struct jailhouse_cell_id.
>>>
>>> This patch is preparatory work for exchanging the jailhouse sysfs cell
>>> structure.
>>>
>>> Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>
>>> ---
>>> driver/cell.c | 14 ++++++++------
>>> driver/cell.h | 2 +-
>>> driver/sysfs.c | 4 ++--
>>> 3 files changed, 11 insertions(+), 9 deletions(-)
>>>
>>
>> ...
> git am skips this. I'll leave out diffstats in future.

Why? Just skip with the defaults.

>>
>>> diff --git a/driver/cell.h b/driver/cell.h
>>> index 1ee5a48..6ea9c0d 100644
>>> --- a/driver/cell.h
>>> +++ b/driver/cell.h
>>> @@ -25,7 +25,7 @@
>>> struct cell {
>>> struct kobject kobj;
>>> struct list_head entry;
>>> - unsigned int id;
>>> + struct jailhouse_cell_id id;
>>
>> Let's unfold this into id and name. You do not make use of the struct as
>> a whole, and "id.id" reads funny.
> Ok, but there's only the padding left in struct jailhouse_cell_id.
>
> Allocate the name statically or dynamically?

Statically - maximum length is part of the ABI.

Jan

[Jan Kiszka]

On 2016-08-07 22:08, Ralf Ramsauer wrote:
> On 08/07/2016 09:52 PM, Jan Kiszka wrote:
>> On 2016-08-07 19:41, Ralf Ramsauer wrote:
>>> struct stat may predict a misleading st_size if procfs or sysfs is involved.
>>
>> Can it also be too small? If not, I don't see why allocating iteratively.
>>
>> For /sys/devices/jailhouse, using 4K as minimum buffer size will
>> practically avoid the loop.
> Yes, but regarding sysfs files, fstat returns 4096 for st_size and
> read() returns only actually set bytes.
>
> Content in /sys/devices/jailhouse is usually smaller than 1024 bytes, so
> why wasting too much space?

1K, 4k - what is the difference?

>
> You can't avoid the loop as regular files might be bigger than 4K.
> Config files which are a regular files are read with this function as well.

So, with the current workload, there is no need for looping at all.

Jan

[Ralf Ramsauer]

On 08/08/2016 07:25 AM, Jan Kiszka wrote:
> On 2016-08-07 22:08, Ralf Ramsauer wrote:
>> On 08/07/2016 09:52 PM, Jan Kiszka wrote:
>>> On 2016-08-07 19:41, Ralf Ramsauer wrote:
>>>> struct stat may predict a misleading st_size if procfs or sysfs is involved.
>>>
>>> Can it also be too small? If not, I don't see why allocating iteratively.
>>>
>>> For /sys/devices/jailhouse, using 4K as minimum buffer size will
>>> practically avoid the loop.
>> Yes, but regarding sysfs files, fstat returns 4096 for st_size and
>> read() returns only actually set bytes.
>>
>> Content in /sys/devices/jailhouse is usually smaller than 1024 bytes, so
>> why wasting too much space?
>
> 1K, 4k - what is the difference?

In most cases, file content is probably smaller than 4K and 4K requires
allocation of a full page while 1K could still fit somewhere else.

But I'm fine with 4K.

>
>>
>> You can't avoid the loop as regular files might be bigger than 4K.
>> Config files which are a regular files are read with this function as well.
>
> So, with the current workload, there is no need for looping at all.

And what if workload changes? It's just a simple loop to be compatible
with arbitrary sizes. Furthermore, read_file() isn't called in hot paths.

I don't get the point. So far, st_size was used to predict the size of
regular files and eventually to read the _whole_ file.
If we would now stop reading after a fix amount of bytes, we change this
logic and don't report any errors in this case - we just snip after the
first 4K.

So what would happen to this guy?
12K -rw-r--r-- 1 ralf ralf 8.2K Aug 7 19:29 apic-demo.cell

Ralf
>
> Jan

[Valentine Sinitsyn]

Hi,

On 08.08.2016 13:26, Ralf Ramsauer wrote:
> On 08/08/2016 07:25 AM, Jan Kiszka wrote:
>> On 2016-08-07 22:08, Ralf Ramsauer wrote:
>>> On 08/07/2016 09:52 PM, Jan Kiszka wrote:
>>>> On 2016-08-07 19:41, Ralf Ramsauer wrote:
>>>>> struct stat may predict a misleading st_size if procfs or sysfs is involved.
>>>>
>>>> Can it also be too small? If not, I don't see why allocating iteratively.
>>>>
>>>> For /sys/devices/jailhouse, using 4K as minimum buffer size will
>>>> practically avoid the loop.
>>> Yes, but regarding sysfs files, fstat returns 4096 for st_size and
>>> read() returns only actually set bytes.
>>>
>>> Content in /sys/devices/jailhouse is usually smaller than 1024 bytes, so
>>> why wasting too much space?
>>
>> 1K, 4k - what is the difference?
> In most cases, file content is probably smaller than 4K and 4K requires
> allocation of a full page while 1K could still fit somewhere else.
>
> But I'm fine with 4K.
>>
>>>
>>> You can't avoid the loop as regular files might be bigger than 4K.
>>> Config files which are a regular files are read with this function as well.
>>
>> So, with the current workload, there is no need for looping at all.
> And what if workload changes? It's just a simple loop to be compatible
> with arbitrary sizes. Furthermore, read_file() isn't called in hot paths.

Ack here. read_file() suggests it's a somewhat general function, and
it's cheap to handle files above 1K/4K as well.

Valentine

[Jan Kiszka]

Let's just do

diff --git a/tools/jailhouse.c b/tools/jailhouse.c
index be5b833..ae2ed3f 100644
--- a/tools/jailhouse.c
+++ b/tools/jailhouse.c
@@ -128,6 +128,7 @@ static void *read_string(const char *string, size_t *size)

static void *read_file(const char *name, size_t *size)
{

struct stat stat;
+ ssize_t result;
void *buffer;
int fd;

@@ -148,7 +149,8 @@ static void *read_file(const char *name, size_t *size)
exit(1);
}

- if (read(fd, buffer, stat.st_size) < stat.st_size) {
+ result = read(fd, buffer, stat.st_size);
+ if (result < 0) {

fprintf(stderr, "reading %s: %s\n", name, strerror(errno));

exit(1);
}
@@ -156,7 +158,7 @@ static void *read_file(const char *name, size_t *size)

close(fd);

if (size)
- *size = stat.st_size;

+ *size = (size_t)result;

return buffer;
}

and call it a day. I see zero point in doing thousands of syscalls and
reallocs for reading already moderately large files.

Jan

[Jan Kiszka]

On 2016-08-07 22:08, Ralf Ramsauer wrote:

> On 08/07/2016 09:53 PM, Jan Kiszka wrote:
>> On 2016-08-07 19:41, Ralf Ramsauer wrote:
>>> And keep the output format of the former python script.
>>>
>>
>> Not yet using scandir with alphasort- why?
>>
>> Jan
>>
> Does order matter? I'll change it in the next round.

We order the output according to the numerical ID, yes.

Jan

[Valentine Sinitsyn]

Is it on a hotpath somewhere? You won't get more than one syscall and
allocation in the case you envision. One extra comparison is what we are
speaking about, actually.

But I'd be highly surprised with the function which could return a
halfway read file without even indicating it to the caller.

Valentine

>
> Jan
>

[Jan Kiszka]

Ralf's code reads chunk-wise, and that is 1K, 4K or whatever fixed size
you choose. The existing code reads everything at once.

>
> But I'd be highly surprised with the function which could return a
> halfway read file without even indicating it to the caller.

The current function works perfectly fine for normal files. The only
case was sysfs with files smaller than their reported size.

Jan

[Valentine Sinitsyn]

For files below CHUNK_SIZE (which we can set to 4096 safely), Ralf's
code and the original one are reading all data at once. The only
difference is Ralf's code checks if it read less than CHUNK_SIZE, and
the current one just goes ahead. Am I missing anything?

>
>>
>> But I'd be highly surprised with the function which could return a
>> halfway read file without even indicating it to the caller.
>
> The current function works perfectly fine for normal files. The only
> case was sysfs with files smaller than their reported size.

That's true. So Ralf made it compatible with "abnormal" files as well.
And it's a userspace tool, not a VMEXIT habdler. I understand simplicity
is our design goal, but can't get what harm it does to make single
function a bit more generic.

Valentine

>
> Jan
>

[Jan Kiszka]

Yes, the removal of fstat().

>
>>
>>>
>>> But I'd be highly surprised with the function which could return a
>>> halfway read file without even indicating it to the caller.
>>
>> The current function works perfectly fine for normal files. The only
>> case was sysfs with files smaller than their reported size.
> That's true. So Ralf made it compatible with "abnormal" files as well.
> And it's a userspace tool, not a VMEXIT habdler. I understand simplicity
> is our design goal, but can't get what harm it does to make single
> function a bit more generic.

Yes, but not with such unnecessary side-effects. Therefore my preference
to address the core issue only.

Jan

[Valentine Sinitsyn]

OK, so the patch does one syscall less than the original.

>
>>
>>>
>>>>
>>>> But I'd be highly surprised with the function which could return a
>>>> halfway read file without even indicating it to the caller.
>>>
>>> The current function works perfectly fine for normal files. The only
>>> case was sysfs with files smaller than their reported size.
>> That's true. So Ralf made it compatible with "abnormal" files as well.
>> And it's a userspace tool, not a VMEXIT habdler. I understand simplicity
>> is our design goal, but can't get what harm it does to make single
>> function a bit more generic.
>
> Yes, but not with such unnecessary side-effects. Therefore my preference
> to address the core issue only.

Sorry, which side effects?

Valentine

[Jan Kiszka]

Come on, even the change log tells it to you:

"Preallocate a memory array and repeatedly read() from the descriptor until
EOF is reached. Reallocate memory by steps of CHUNK_SIZE."

Jan

[Valentine Sinitsyn]

On 08.08.2016 14:30, Jan Kiszka wrote:
...

> Come on, even the change log tells it to you:
>
> "Preallocate a memory array and repeatedly read() from the descriptor until
> EOF is reached. Reallocate memory by steps of CHUNK_SIZE."

I was reading the code in the first place, and honestly I don't see any
contradiction (maybe I'm just too tired).

Guess this discussion goes a bit counter-constructive, however, so I'd
leave it to Ralf for decide.

Thanks,
Valentine

[Ralf Ramsauer]

Hi all,

Though I still think that looped reads would be a more general approach,
both variants (hopefully) lead to the same result in the end of the day.
In userspace and especially in this very case, it doesn't play a
significant role if you reallocate once, twice or more.

Nevertheless, I'll adapt it as Jan proposed...

Ralf
>
> Thanks,
> Valentine

--
Ralf Ramsauer
PGP: 0x8F10049B

[Ralf Ramsauer]

struct cell did not know about the cell name. Add it.

This patch is preparatory work for exchanging the cell structure of the
jailhouse sysfs interface.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>

diff --git a/driver/cell.c b/driver/cell.c
index 05ac7b2..8681175 100644
--- a/driver/cell.c
+++ b/driver/cell.c
@@ -64,6 +64,8 @@ struct cell *jailhouse_cell_create(const struct jailhouse_cell_desc *cell_desc)
return ERR_PTR(-ENOMEM);
}

+ memcpy(cell->name, cell_desc->name, JAILHOUSE_CELL_NAME_MAXLEN+1);
+
memcpy(cell->memory_regions, jailhouse_cell_mem_regions(cell_desc),
sizeof(struct jailhouse_memory) * cell->num_memory_regions);

diff --git a/driver/cell.h b/driver/cell.h
index 1ee5a48..d30bba8 100644
--- a/driver/cell.h
+++ b/driver/cell.h
@@ -26,6 +26,7 @@ struct cell {

struct kobject kobj;
struct list_head entry;

unsigned int id;
+ char name[JAILHOUSE_CELL_ID_NAMELEN+1];
cpumask_t cpus_assigned;
u32 num_memory_regions;
struct jailhouse_memory *memory_regions;
--
2.9.2

[Ralf Ramsauer]

We want to add two additional sysfs nodes for each cell:
cpus_assigned_list and cpus_failed_list.

A lot of printing-work would be redundant. As preparatory work,
consolidate it and add an additional as_list flag to allow
human readable cpumasks representation.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>

diff --git a/driver/sysfs.c b/driver/sysfs.c
index 960c7b3..a9e5243 100644
--- a/driver/sysfs.c
+++ b/driver/sysfs.c
@@ -139,6 +139,27 @@ static struct attribute_group stats_attr_group = {
.name = "statistics"
};

+static int print_cpumask(char *buf, size_t size, cpumask_t *mask, bool as_list)
+{
+ int written;
+
+ if (as_list)
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,0,0)
+ written = scnprintf(buf, size, "%*pbl\n",
+ cpumask_pr_args(mask));
+ else
+ written = scnprintf(buf, size, "%*pb\n",
+ cpumask_pr_args(mask));
+#else
+ written = cpulist_scnprintf(buf, size, mask);
+ else
+ written = cpumask_scnprintf(buf, size, mask);
+ written += scnprintf(buf + written, size - written, "\n");
+#endif

+
+ return written;
+}
+
static ssize_t name_show(struct kobject *kobj, struct kobj_attribute *attr,
char *buffer)
{

@@ -170,16 +191,8 @@ static ssize_t cpus_assigned_show(struct kobject *kobj,

struct kobj_attribute *attr, char *buf)
{
struct cell *cell = container_of(kobj, struct cell, kobj);

- int written;

-#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,0,0)
- written = scnprintf(buf, PAGE_SIZE, "%*pb\n",
- cpumask_pr_args(&cell->cpus_assigned));
-#else
- written = cpumask_scnprintf(buf, PAGE_SIZE, &cell->cpus_assigned);
- written += scnprintf(buf + written, PAGE_SIZE - written, "\n");
-#endif
- return written;
+ return print_cpumask(buf, PAGE_SIZE, &cell->cpus_assigned, false);

}

static ssize_t cpus_failed_show(struct kobject *kobj,

@@ -199,13 +212,7 @@ static ssize_t cpus_failed_show(struct kobject *kobj,
JAILHOUSE_CPU_FAILED)
cpumask_set_cpu(cpu, cpus_failed);

-#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,0,0)
- written = scnprintf(buf, PAGE_SIZE, "%*pb\n",
- cpumask_pr_args(cpus_failed));
-#else
- written = cpumask_scnprintf(buf, PAGE_SIZE, cpus_failed);
- written += scnprintf(buf + written, PAGE_SIZE - written, "\n");
-#endif
+ written = print_cpumask(buf, PAGE_SIZE, cpus_failed, false);

free_cpumask_var(cpus_failed);

--
2.9.2

[Ralf Ramsauer]

Hi all,

I'd like to replace jailhouse-cell-list. It's a rudimentary function and
requires python. So I reimplemented cell-list natively in C.

This series also changes the syfs structure as Jan proposed.

For 8/8, I did not use a filter function for scandir(), as unwanted
entries can easily be filtered out inline.

changes since v2:
- do not use struct jailhouse_cell_id inside struct cell
- adapt read_file() as suggested by Jan
- amend commit messages
- use scandir() and alphasort for sorted output

changes since v1:
- change sysfs structure (swap id and name)
- provide human readable sysfs nodes
- improve changes on read_file()

Ralf Ramsauer (8):
driver: cell: add cell name to struct cell
driver: sysfs: do not pass cell name to jh_sysfs_cell_create
driver: sysfs: swap sysfs entries name and id
driver: sysfs: consolidate cpumask printing
driver: sysfs: factor out printing of failed_cpus
driver: sysfs: add new sysfs nodes cpus_{assigned,failed}_list
tools: jailhouse: make read_file() compatible with sysfs files
tools: jailhouse: reimplement jailhouse-cell-list in C

Documentation/sysfs-entries.txt | 4 +-
driver/cell.c | 4 +-
driver/cell.h | 1 +
driver/sysfs.c | 112 ++++++++++++++++++++++++++--------------
driver/sysfs.h | 2 +-
tools/jailhouse-cell-list | 71 -------------------------
tools/jailhouse.c | 109 ++++++++++++++++++++++++++++++++++++--
7 files changed, 187 insertions(+), 116 deletions(-)
delete mode 100755 tools/jailhouse-cell-list

--
2.9.2

[Ralf Ramsauer]

struct cell now has information about the cell name. It is not necessary
to pass the name of the cell to jh_sysfs_cell_create.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>

diff --git a/driver/cell.c b/driver/cell.c
index 8681175..7cced42 100644
--- a/driver/cell.c
+++ b/driver/cell.c
@@ -76,7 +76,7 @@ struct cell *jailhouse_cell_create(const struct jailhouse_cell_desc *cell_desc)
return ERR_PTR(err);
}

- err = jailhouse_sysfs_cell_create(cell, cell_desc->name);
+ err = jailhouse_sysfs_cell_create(cell);
if (err)
/* cleanup done by jailhouse_sysfs_cell_create */
return ERR_PTR(err);
diff --git a/driver/sysfs.c b/driver/sysfs.c
index 4959b56..a18c2b9 100644
--- a/driver/sysfs.c
+++ b/driver/sysfs.c
@@ -232,12 +232,12 @@ static struct kobj_type cell_type = {
.default_attrs = cell_attrs,
};

-int jailhouse_sysfs_cell_create(struct cell *cell, const char *name)
+int jailhouse_sysfs_cell_create(struct cell *cell)
{
int err;

err = kobject_init_and_add(&cell->kobj, &cell_type, cells_dir, "%s",
- name);
+ cell->name);
if (err) {
jailhouse_cell_kobj_release(&cell->kobj);
return err;
diff --git a/driver/sysfs.h b/driver/sysfs.h
index ad0e2f9..f5cdc34 100644
--- a/driver/sysfs.h
+++ b/driver/sysfs.h
@@ -15,7 +15,7 @@

#include <linux/device.h>

-int jailhouse_sysfs_cell_create(struct cell *cell, const char *name);
+int jailhouse_sysfs_cell_create(struct cell *cell);
void jailhouse_sysfs_cell_register(struct cell *cell);
void jailhouse_sysfs_cell_delete(struct cell *cell);

--
2.9.2

[Ralf Ramsauer]

Directories in /sys/jailhouse/cells/ formerly represented cell names.

This patch changes the structure. From now on, /sys/jailhouse/cells/
will contain cell ids. The name of a specific cell is provided through
/sys/jailhouse/cells/<id>/name.

This makes the sysfs interface easier to be parsed by applications.

Additionally, amend documentation and jailhouse-cell-list.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>

diff --git a/Documentation/sysfs-entries.txt b/Documentation/sysfs-entries.txt
index 2845220..0fc315a 100644
--- a/Documentation/sysfs-entries.txt
+++ b/Documentation/sysfs-entries.txt
@@ -11,8 +11,8 @@ can be used for monitoring the state of the hypervisor and its cells.
|- remap_pool_size - number of pages in hypervisor remapping pool
|- remap_pool_used - used pages of hypervisor remapping pool
`- cells
- |- <name of cell>
- | |- id - unique numerical ID
+ |- <id> - unique numerical ID
+ | |- name - cell name
| |- state - "running", "running/locked", "shut down", or
| | "failed"
| |- cpus_assigned - bitmask of assigned logical CPUs
diff --git a/driver/sysfs.c b/driver/sysfs.c
index a18c2b9..960c7b3 100644
--- a/driver/sysfs.c
+++ b/driver/sysfs.c
@@ -139,12 +139,12 @@ static struct attribute_group stats_attr_group = {
.name = "statistics"
};

-static ssize_t id_show(struct kobject *kobj, struct kobj_attribute *attr,
- char *buffer)
+static ssize_t name_show(struct kobject *kobj, struct kobj_attribute *attr,
+ char *buffer)

{
struct cell *cell = container_of(kobj, struct cell, kobj);

- return sprintf(buffer, "%u\n", cell->id);
+ return sprintf(buffer, "%s\n", cell->name);
}

static ssize_t state_show(struct kobject *kobj, struct kobj_attribute *attr,
@@ -212,14 +212,14 @@ static ssize_t cpus_failed_show(struct kobject *kobj,
return written;
}

-static struct kobj_attribute cell_id_attr = __ATTR_RO(id);
+static struct kobj_attribute cell_name_attr = __ATTR_RO(name);
static struct kobj_attribute cell_state_attr = __ATTR_RO(state);
static struct kobj_attribute cell_cpus_assigned_attr =
__ATTR_RO(cpus_assigned);
static struct kobj_attribute cell_cpus_failed_attr = __ATTR_RO(cpus_failed);

static struct attribute *cell_attrs[] = {
- &cell_id_attr.attr,
+ &cell_name_attr.attr,
&cell_state_attr.attr,
&cell_cpus_assigned_attr.attr,
&cell_cpus_failed_attr.attr,
@@ -236,8 +236,8 @@ int jailhouse_sysfs_cell_create(struct cell *cell)
{
int err;

- err = kobject_init_and_add(&cell->kobj, &cell_type, cells_dir, "%s",
- cell->name);
+ err = kobject_init_and_add(&cell->kobj, &cell_type, cells_dir, "%u",
+ cell->id);

if (err) {
jailhouse_cell_kobj_release(&cell->kobj);
return err;

diff --git a/tools/jailhouse-cell-list b/tools/jailhouse-cell-list
index 3b5bc20..d4123f6 100755
--- a/tools/jailhouse-cell-list
+++ b/tools/jailhouse-cell-list
@@ -54,8 +54,8 @@ if len(sys.argv) > 1:
cells = []
for cell_path in glob.glob('/sys/devices/jailhouse/cells/*'):
cells.append({
- 'name': os.path.basename(cell_path),
- 'id': open(cell_path + "/id").readline().strip(),
+ 'id': os.path.basename(cell_path),
+ 'name': open(cell_path + "/name").readline().strip(),
'state': open(cell_path + "/state").readline().strip(),
'cpus_assigned': read_cpus(cell_path + "/cpus_assigned"),
'cpus_failed': read_cpus(cell_path + "/cpus_failed")
--
2.9.2

[Ralf Ramsauer]

Files in sysfs or procfs may be smaller than st_size actually reports.
We will read file from sysfs in future.

This patch makes the strong assumption that the st_size field in struct
stat can be seen as an upper bound for the actual file content.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>

diff --git a/tools/jailhouse.c b/tools/jailhouse.c
index be5b833..ae2ed3f 100644
--- a/tools/jailhouse.c
+++ b/tools/jailhouse.c
@@ -128,6 +128,7 @@ static void *read_string(const char *string, size_t *size)
static void *read_file(const char *name, size_t *size)
{
struct stat stat;
+ ssize_t result;
void *buffer;
int fd;

@@ -148,7 +149,8 @@ static void *read_file(const char *name, size_t *size)
exit(1);
}

- if (read(fd, buffer, stat.st_size) < stat.st_size) {
+ result = read(fd, buffer, stat.st_size);
+ if (result < 0) {
fprintf(stderr, "reading %s: %s\n", name, strerror(errno));
exit(1);
}
@@ -156,7 +158,7 @@ static void *read_file(const char *name, size_t *size)
close(fd);

if (size)
- *size = stat.st_size;
+ *size = (size_t)result;

return buffer;
}

--
2.9.2

[Ralf Ramsauer]

And keep the sorted output format of the former python script.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>

diff --git a/tools/jailhouse-cell-list b/tools/jailhouse-cell-list
deleted file mode 100755
index d5a535b..0000000
--- a/tools/jailhouse-cell-list
+++ /dev/null
@@ -1,40 +0,0 @@
-#!/usr/bin/env python
-
-# Jailhouse, a Linux-based partitioning hypervisor
-#
-# Copyright (c) Siemens AG, 2014
-#
-# Authors:
-# Jan Kiszka <jan.k...@siemens.com>
-#
-# This work is licensed under the terms of the GNU GPL, version 2. See
-# the COPYING file in the top-level directory.
-
-import glob
-import os
-import sys
-
-
-if len(sys.argv) > 1:
- print("usage: %s" % os.path.basename(sys.argv[0]).replace("-", " "))
- exit(0 if sys.argv[1] in ("--help", "-h") else 1)
-
-cells = []
-for cell_path in glob.glob('/sys/devices/jailhouse/cells/*'):
- cells.append({
- 'id': os.path.basename(cell_path),
- 'name': open(cell_path + "/name").readline().strip(),
- 'state': open(cell_path + "/state").readline().strip(),
- 'cpus_assigned_list':
- open(cell_path + "/cpus_assigned_list").readline().strip(),
- 'cpus_failed_list':
- open(cell_path + "/cpus_failed_list").readline().strip()
- })
-
-line_format = "%-8s%-24s%-16s%-24s%-24s"
-if not cells == []:
- print(line_format % ("ID", "Name", "State",
- "Assigned CPUs", "Failed CPUs"))
-for cell in sorted(cells, key=lambda cell: cell['id']):
- print(line_format % (cell['id'], cell['name'], cell['state'],
- cell['cpus_assigned_list'], cell['cpus_failed_list']))
diff --git a/tools/jailhouse.c b/tools/jailhouse.c
index ae2ed3f..fe4aa6e 100644
--- a/tools/jailhouse.c
+++ b/tools/jailhouse.c
@@ -14,6 +14,7 @@
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
+#include <dirent.h>
#include <unistd.h>
#include <fcntl.h>
#include <errno.h>
@@ -27,6 +28,7 @@

#define JAILHOUSE_EXEC_DIR LIBEXECDIR "/jailhouse"
#define JAILHOUSE_DEVICE "/dev/jailhouse"

+#define JAILHOUSE_CELLS "/sys/devices/jailhouse/cells/"

enum shutdown_load_mode {LOAD, SHUTDOWN};

@@ -34,11 +36,17 @@ struct extension {
char *cmd, *subcmd, *help;
};

+struct jailhouse_cell_info {
+ struct jailhouse_cell_id id;
+ char *state;
+ char *cpus_assigned_list;
+ char *cpus_failed_list;
+};
+
static const struct extension extensions[] = {
{ "cell", "linux", "CELLCONFIG KERNEL [-i | --initrd FILE]\n"
" [-c | --cmdline \"STRING\"] "
"[-w | --write-params FILE]" },
- { "cell", "list", "" },
{ "cell", "stats", "{ ID | [--name] NAME }" },
{ "config", "create", "[-h] [-g] [-r ROOT] "
"[--mem-inmates MEM_INMATES]\n"
@@ -57,6 +65,7 @@ static void __attribute__((noreturn)) help(char *prog, int exit_status)
" enable SYSCONFIG\n"
" disable\n"
" cell create CELLCONFIG\n"
+ " cell list\n"
" cell load { ID | [--name] NAME } "
"{ IMAGE | { -s | --string } \"STRING\" }\n"
" [-a | --address ADDRESS] ...\n"
@@ -163,6 +172,20 @@ static void *read_file(const char *name, size_t *size)
return buffer;
}

+static char *read_sysfs_entry(const char *name)
+{
+ char *ret;
+ size_t size;
+
+ ret = read_file(name, &size);
+
+ /* enforce the string to be null-terminated */
+ if (size && ret[size-1] != '\0')
+ ret[size-1] = '\0';
+
+ return ret;
+}
+
static int enable(int argc, char *argv[])
{
void *config;
@@ -251,6 +274,82 @@ static bool match_opt(const char *argv, const char *short_opt,
strcmp(argv, long_opt) == 0;
}

+static struct jailhouse_cell_info *get_cell_info(const int id)
+{
+ char buf[sizeof(JAILHOUSE_CELLS) + 3 + sizeof("/cpus_assigned_list") + 1];
+ struct jailhouse_cell_info *cinfo;
+ char *tmp;
+
+ cinfo = calloc(1, sizeof(struct jailhouse_cell_info));
+
+ /* set cell id */
+ cinfo->id.id = id;
+
+ /* get cell name */
+ snprintf(buf, sizeof(buf), JAILHOUSE_CELLS "%d/name", id);
+ tmp = read_sysfs_entry(buf);
+ strncpy(cinfo->id.name, tmp, JAILHOUSE_CELL_ID_NAMELEN);
+ free(tmp);
+
+ /* get cell state */
+ snprintf(buf, sizeof(buf), JAILHOUSE_CELLS "%d/state", id);
+ cinfo->state = read_sysfs_entry(buf);
+
+ /* get assigned cpu list */
+ snprintf(buf, sizeof(buf), JAILHOUSE_CELLS "%d/cpus_assigned_list", id);
+ cinfo->cpus_assigned_list = read_sysfs_entry(buf);
+
+ /* get failed cpu list */
+ snprintf(buf, sizeof(buf), JAILHOUSE_CELLS "%d/cpus_failed_list", id);
+ cinfo->cpus_failed_list = read_sysfs_entry(buf);
+
+ return cinfo;
+}
+
+static void cell_info_free(struct jailhouse_cell_info *cinfo)
+{
+ free(cinfo->state);
+ free(cinfo->cpus_assigned_list);
+ free(cinfo->cpus_failed_list);
+ free(cinfo);
+}
+
+static int cell_list(int argc, char *argv[])
+{
+ struct dirent **namelist;
+ struct jailhouse_cell_info *cinfo;
+ int id, i, num_entries;
+ (void)argv;
+
+ if (argc != 3)
+ help(argv[0], 1);
+
+ printf("%-8s%-24s%-16s%-24s%-24s\n",
+ "ID", "Name", "State", "Assigned CPUs", "Failed CPUs");
+
+ num_entries = scandir(JAILHOUSE_CELLS, &namelist, 0, alphasort);
+ if (num_entries < 0) {
+ perror("scandir");
+ exit(1);
+ }
+
+ for (i = 0; i < num_entries; i++) {
+ /* Skip '.' and '..' */
+ if (namelist[i]->d_name[0] != '.') {
+ id = (int)strtol(namelist[i]->d_name, NULL, 10);
+
+ cinfo = get_cell_info(id);
+ printf("%-8d%-24s%-16s%-24s%-24s\n", cinfo->id.id, cinfo->id.name,
+ cinfo->state, cinfo->cpus_assigned_list, cinfo->cpus_failed_list);
+ cell_info_free(cinfo);
+ }
+ free(namelist[i]);
+ }
+ free(namelist);
+
+ return 0;
+}
+
static int cell_shutdown_load(int argc, char *argv[],
enum shutdown_load_mode mode)
{
@@ -369,6 +468,8 @@ static int cell_management(int argc, char *argv[])

if (strcmp(argv[2], "create") == 0) {
err = cell_create(argc, argv);
+ } else if (strcmp(argv[2], "list") == 0) {
+ err = cell_list(argc, argv);
} else if (strcmp(argv[2], "load") == 0) {
err = cell_shutdown_load(argc, argv, LOAD);
} else if (strcmp(argv[2], "start") == 0) {
--
2.9.2

[Ralf Ramsauer]

Factor out printing of failed_cpus to an extra function and provide
as_list format flag.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>

diff --git a/driver/sysfs.c b/driver/sysfs.c
index a9e5243..8bada38 100644
--- a/driver/sysfs.c
+++ b/driver/sysfs.c
@@ -160,6 +160,29 @@ static int print_cpumask(char *buf, size_t size, cpumask_t *mask, bool as_list)
return written;
}

+static int print_failed_cpus(char *buf, size_t size, const struct cell *cell,
+ bool as_list)
+{
+ cpumask_var_t cpus_failed;
+ unsigned int cpu;
+ int written;
+
+ if (!zalloc_cpumask_var(&cpus_failed, GFP_KERNEL))
+ return -ENOMEM;
+
+ for_each_cpu(cpu, &cell->cpus_assigned)
+ if (jailhouse_call_arg2(JAILHOUSE_HC_CPU_GET_INFO, cpu,
+ JAILHOUSE_CPU_INFO_STATE) ==
+ JAILHOUSE_CPU_FAILED)
+ cpumask_set_cpu(cpu, cpus_failed);
+
+ written = print_cpumask(buf, size, cpus_failed, as_list);
+
+ free_cpumask_var(cpus_failed);

+
+ return written;
+}
+

static ssize_t name_show(struct kobject *kobj, struct kobj_attribute *attr,

char *buffer)
{
@@ -199,24 +222,8 @@ static ssize_t cpus_failed_show(struct kobject *kobj,

struct kobj_attribute *attr, char *buf)
{

struct cell *cell = container_of(kobj, struct cell, kobj);

- cpumask_var_t cpus_failed;
- unsigned int cpu;
- int written;
-
- if (!zalloc_cpumask_var(&cpus_failed, GFP_KERNEL))
- return -ENOMEM;
-
- for_each_cpu(cpu, &cell->cpus_assigned)
- if (jailhouse_call_arg2(JAILHOUSE_HC_CPU_GET_INFO, cpu,
- JAILHOUSE_CPU_INFO_STATE) ==
- JAILHOUSE_CPU_FAILED)
- cpumask_set_cpu(cpu, cpus_failed);

- written = print_cpumask(buf, PAGE_SIZE, cpus_failed, false);
-
- free_cpumask_var(cpus_failed);
-
- return written;
+ return print_failed_cpus(buf, PAGE_SIZE, cell, false);

}

static struct kobj_attribute cell_name_attr = __ATTR_RO(name);

--
2.9.2

[Ralf Ramsauer]

Add human readable nodes for cpus_assigned and cpus_failed and let
jailhouse-cell-list use those nodes.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>

diff --git a/driver/sysfs.c b/driver/sysfs.c

index 8bada38..6a736af 100644
--- a/driver/sysfs.c
+++ b/driver/sysfs.c
@@ -218,6 +218,14 @@ static ssize_t cpus_assigned_show(struct kobject *kobj,

return print_cpumask(buf, PAGE_SIZE, &cell->cpus_assigned, false);
}

+static ssize_t cpus_assigned_list_show(struct kobject *kobj,
+ struct kobj_attribute *attr, char *buf)
+{
+ struct cell *cell = container_of(kobj, struct cell, kobj);
+
+ return print_cpumask(buf, PAGE_SIZE, &cell->cpus_assigned, true);
+}
+

static ssize_t cpus_failed_show(struct kobject *kobj,
struct kobj_attribute *attr, char *buf)
{

@@ -226,17 +234,31 @@ static ssize_t cpus_failed_show(struct kobject *kobj,

return print_failed_cpus(buf, PAGE_SIZE, cell, false);
}

+static ssize_t cpus_failed_list_show(struct kobject *kobj,
+ struct kobj_attribute *attr, char *buf)
+{
+ struct cell *cell = container_of(kobj, struct cell, kobj);
+
+ return print_failed_cpus(buf, PAGE_SIZE, cell, true);
+}
+

static struct kobj_attribute cell_name_attr = __ATTR_RO(name);

static struct kobj_attribute cell_state_attr = __ATTR_RO(state);
static struct kobj_attribute cell_cpus_assigned_attr =
__ATTR_RO(cpus_assigned);

+static struct kobj_attribute cell_cpus_assigned_list_attr =
+ __ATTR_RO(cpus_assigned_list);

static struct kobj_attribute cell_cpus_failed_attr = __ATTR_RO(cpus_failed);

+static struct kobj_attribute cell_cpus_failed_list_attr =
+ __ATTR_RO(cpus_failed_list);

static struct attribute *cell_attrs[] = {

&cell_name_attr.attr,
&cell_state_attr.attr,
&cell_cpus_assigned_attr.attr,
+ &cell_cpus_assigned_list_attr.attr,
&cell_cpus_failed_attr.attr,
+ &cell_cpus_failed_list_attr.attr,
NULL,
};

diff --git a/tools/jailhouse-cell-list b/tools/jailhouse-cell-list
index d4123f6..d5a535b 100755
--- a/tools/jailhouse-cell-list
+++ b/tools/jailhouse-cell-list

if len(sys.argv) > 1:

print("usage: %s" % os.path.basename(sys.argv[0]).replace("-", " "))

exit(0 if sys.argv[1] in ("--help", "-h") else 1)

@@ -57,8 +25,10 @@ for cell_path in glob.glob('/sys/devices/jailhouse/cells/*'):
'id': os.path.basename(cell_path),

'name': open(cell_path + "/name").readline().strip(),

'state': open(cell_path + "/state").readline().strip(),

- 'cpus_assigned': read_cpus(cell_path + "/cpus_assigned"),
- 'cpus_failed': read_cpus(cell_path + "/cpus_failed")
+ 'cpus_assigned_list':
+ open(cell_path + "/cpus_assigned_list").readline().strip(),
+ 'cpus_failed_list':
+ open(cell_path + "/cpus_failed_list").readline().strip()
})

line_format = "%-8s%-24s%-16s%-24s%-24s"
@@ -67,5 +37,4 @@ if not cells == []:

"Assigned CPUs", "Failed CPUs"))

for cell in sorted(cells, key=lambda cell: cell['id']):

print(line_format % (cell['id'], cell['name'], cell['state'],

- print_cpus(cell['cpus_assigned']),
- print_cpus(cell['cpus_failed'])))
+ cell['cpus_assigned_list'], cell['cpus_failed_list']))
--
2.9.2

[Ralf Ramsauer]

On 08/08/2016 10:39 PM, Ralf Ramsauer wrote:
> struct cell did not know about the cell name. Add it.
>
> This patch is preparatory work for exchanging the cell structure of the
> jailhouse sysfs interface.
>
> Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>
>
> diff --git a/driver/cell.c b/driver/cell.c
> index 05ac7b2..8681175 100644
> --- a/driver/cell.c
> +++ b/driver/cell.c
> @@ -64,6 +64,8 @@ struct cell *jailhouse_cell_create(const struct jailhouse_cell_desc *cell_desc)
> return ERR_PTR(-ENOMEM);
> }
>
> + memcpy(cell->name, cell_desc->name, JAILHOUSE_CELL_NAME_MAXLEN+1);

Damn, size should be sizeof(cell->name). Mixed it up.

Ralf

> +
> memcpy(cell->memory_regions, jailhouse_cell_mem_regions(cell_desc),
> sizeof(struct jailhouse_memory) * cell->num_memory_regions);
>
> diff --git a/driver/cell.h b/driver/cell.h
> index 1ee5a48..d30bba8 100644
> --- a/driver/cell.h
> +++ b/driver/cell.h
> @@ -26,6 +26,7 @@ struct cell {
> struct kobject kobj;
> struct list_head entry;
> unsigned int id;
> + char name[JAILHOUSE_CELL_ID_NAMELEN+1];
> cpumask_t cpus_assigned;
> u32 num_memory_regions;
> struct jailhouse_memory *memory_regions;
>


--

Ralf Ramsauer
GPG: 0x8F10049B

[Ralf Ramsauer]

tools/Makefile needs to be updated as well. Forgot it, I'm sorry.
Ralf

On 08/08/2016 10:39 PM, Ralf Ramsauer wrote:

Ralf Ramsauer
GPG: 0x8F10049B

[Ralf Ramsauer]

Hi,

just realised that sysfs entries are created before valid cell ID are
assigned.

Patch 3/8: "driver: sysfs: swap sysfs entries name and id" doesn't
respect this and jailhouse fails creating non-root cells.

I'll get back and take my time to fix this.

Sorry

Ralf

On 08/08/2016 10:39 PM, Ralf Ramsauer wrote:

Ralf Ramsauer
GPG: 0x8F10049B

[Jan Kiszka]

Second check is not needed: in the worst case, we overwrite 0 with 0...

> + ret[size-1] = '\0';
> +
> + return ret;
> +}
> +
> static int enable(int argc, char *argv[])
> {
> void *config;
> @@ -251,6 +274,82 @@ static bool match_opt(const char *argv, const char *short_opt,
> strcmp(argv, long_opt) == 0;
> }
>
> +static struct jailhouse_cell_info *get_cell_info(const int id)
> +{
> + char buf[sizeof(JAILHOUSE_CELLS) + 3 + sizeof("/cpus_assigned_list") + 1];

This buffer size calculation wants to be careful, but if some other
string length changes, it will also break. Let's just use a safe larger
size, like 64 or 128. You use snprintf below anyway.

> + struct jailhouse_cell_info *cinfo;
> + char *tmp;
> +
> + cinfo = calloc(1, sizeof(struct jailhouse_cell_info));

calloc could fail and return NULL. OK, unlikely.

Why calloc (for one element) and not malloc?

Single-caller function...

> +
> +static int cell_list(int argc, char *argv[])
> +{
> + struct dirent **namelist;
> + struct jailhouse_cell_info *cinfo;
> + int id, i, num_entries;
> + (void)argv;
> +
> + if (argc != 3)
> + help(argv[0], 1);
> +
> + printf("%-8s%-24s%-16s%-24s%-24s\n",
> + "ID", "Name", "State", "Assigned CPUs", "Failed CPUs");
> +
> + num_entries = scandir(JAILHOUSE_CELLS, &namelist, 0, alphasort);
> + if (num_entries < 0) {
> + perror("scandir");
> + exit(1);

Will we panic here if jailhouse isn't loaded yet? The current version
just does nothing, not even print the banner.

> + }
> +
> + for (i = 0; i < num_entries; i++) {
> + /* Skip '.' and '..' */
> + if (namelist[i]->d_name[0] != '.') {

If you invert the test here and just do "continue;", you could save
indention below.

> + id = (int)strtol(namelist[i]->d_name, NULL, 10);
> +
> + cinfo = get_cell_info(id);
> + printf("%-8d%-24s%-16s%-24s%-24s\n", cinfo->id.id, cinfo->id.name,
> + cinfo->state, cinfo->cpus_assigned_list, cinfo->cpus_failed_list);

Overlong lines.

> + cell_info_free(cinfo);
> + }
> + free(namelist[i]);
> + }
> + free(namelist);
> +
> + return 0;
> +}
> +
> static int cell_shutdown_load(int argc, char *argv[],
> enum shutdown_load_mode mode)
> {
> @@ -369,6 +468,8 @@ static int cell_management(int argc, char *argv[])
>
> if (strcmp(argv[2], "create") == 0) {
> err = cell_create(argc, argv);
> + } else if (strcmp(argv[2], "list") == 0) {
> + err = cell_list(argc, argv);
> } else if (strcmp(argv[2], "load") == 0) {
> err = cell_shutdown_load(argc, argv, LOAD);
> } else if (strcmp(argv[2], "start") == 0) {
>

Just minor comments now, looks good in general.

But take your time for v4, I'm not going to close a merge window these
days ;).

Jan

[Ralf Ramsauer]

calloc was an artifact from the last round as calloc initalises memory
with zeroes. Will convert it to malloc.

get_cell_info() is a single-caller function as well, so either inline
both or leave it as it is. What's the problem with single caller functions?

>
>> +
>> +static int cell_list(int argc, char *argv[])
>> +{
>> + struct dirent **namelist;
>> + struct jailhouse_cell_info *cinfo;
>> + int id, i, num_entries;
>> + (void)argv;
>> +
>> + if (argc != 3)
>> + help(argv[0], 1);
>> +
>> + printf("%-8s%-24s%-16s%-24s%-24s\n",
>> + "ID", "Name", "State", "Assigned CPUs", "Failed CPUs");
>> +
>> + num_entries = scandir(JAILHOUSE_CELLS, &namelist, 0, alphasort);
>> + if (num_entries < 0) {
>> + perror("scandir");
>> + exit(1);
>
> Will we panic here if jailhouse isn't loaded yet? The current version
> just does nothing, not even print the banner.

Did it happen on purpose that jailhouse cell list 'fails' quietly if
jailhouse isn't loaded yet? What about printing leading messages like
'Jailhouse is not enabled' and 'Jailhouse module is not loaded'?

Other that that, scandir can still fail with ENOMEM. We will have to
print this error, even python would print some errors in this case.

>
>> + }
>> +
>> + for (i = 0; i < num_entries; i++) {
>> + /* Skip '.' and '..' */
>> + if (namelist[i]->d_name[0] != '.') {
>
> If you invert the test here and just do "continue;", you could save
> indention below.

Then I get two locations where I would have to free the namelist entry.

[Ralf Ramsauer]

Hi,

On 08/08/2016 11:23 PM, Ralf Ramsauer wrote:
> Hi,
>
> just realised that sysfs entries are created before valid cell ID are
> assigned.

a short general question on the creation of cells:

For any type of cell, the corresponding sysfs entries are created by
cell_create() before the cell is created by the hypervisor or jailhouse
is enabled. This applies to the root cell as well as to non-root cells
and was possible as the sysfs cell id can only be queried once the cell
was registered. In the current code, the registration of cells does not
fail.

If we want to change the sysfs structure (swap name and id), it is
necessary to know the eventual cell id when the sysfs structure is
created. That's no problem in case of the root cell, as it will
definitely get id 0. We can rely on this assumption. But how to deal
with non-root cells?

One possibility could be to delay the creation of sysfs entries after
jailhouse was successfully enabled or the cell was successfully created.
This comes with the strong disadvantage that the creation of sysfs
entries might fail. In an error case, we would need a full rollback
(e.g. disable jailhouse or destroy the cell though the hypervisor
actually was successful).

We could also create a 'dummy' or placeholder sysfs entry for pending
cells (e.g. define a JAILHOUSE_CELL_ID_PENDING id) and rename it after
creation/enabling was successful.

But kobject_rename might also fail. So in any case we would end up in a
rollback.

Any ideas on this?

Ralf

[Jan Kiszka]

I didn't notice the symmetry. Then it's fine.

>>
>>> +
>>> +static int cell_list(int argc, char *argv[])
>>> +{
>>> + struct dirent **namelist;
>>> + struct jailhouse_cell_info *cinfo;
>>> + int id, i, num_entries;
>>> + (void)argv;
>>> +
>>> + if (argc != 3)
>>> + help(argv[0], 1);
>>> +
>>> + printf("%-8s%-24s%-16s%-24s%-24s\n",
>>> + "ID", "Name", "State", "Assigned CPUs", "Failed CPUs");
>>> +
>>> + num_entries = scandir(JAILHOUSE_CELLS, &namelist, 0, alphasort);
>>> + if (num_entries < 0) {
>>> + perror("scandir");
>>> + exit(1);
>>
>> Will we panic here if jailhouse isn't loaded yet? The current version
>> just does nothing, not even print the banner.
> Did it happen on purpose that jailhouse cell list 'fails' quietly if
> jailhouse isn't loaded yet? What about printing leading messages like
> 'Jailhouse is not enabled' and 'Jailhouse module is not loaded'?

It's a bit like lspci or lsusb returning nothing when there are no
corresponding buses - no Jailhouse, no cells.

>
> Other that that, scandir can still fail with ENOMEM. We will have to
> print this error, even python would print some errors in this case.

That is fine to report, of course.

>>
>>> + }
>>> +
>>> + for (i = 0; i < num_entries; i++) {
>>> + /* Skip '.' and '..' */
>>> + if (namelist[i]->d_name[0] != '.') {
>>
>> If you invert the test here and just do "continue;", you could save
>> indention below.
> Then I get two locations where I would have to free the namelist entry.

You are right.

Jan

[Jan Kiszka]

Good points. To make the list of options longer:

- guess the next ID also for non-root cells - practically safe because
only the driver is supposed to talk to the hypervisor

- move ID selection into the driver and let the hypervisor only
validate its uniqueness - a bit cleaner, but also more work

- warn about but otherwise ignore sysfs errors

I'm leaning a bit to the second option, but I'm open for further thoughts.

BTW, you will also have to adjust jailhouse-completion.bash when
changing the sysfs structure.

Jan

[Ralf Ramsauer]

We want to add two additional sysfs nodes for each cell:
cpus_assigned_list and cpus_failed_list.

A lot of printing-work would be redundant. As preparatory work,
consolidate it and add an additional as_list flag to allow
human readable cpumasks representation.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>

diff --git a/driver/sysfs.c b/driver/sysfs.c
index 8c2a11f..33931d9 100644
--- a/driver/sysfs.c
+++ b/driver/sysfs.c
@@ -139,6 +139,27 @@ static struct attribute_group stats_attr_group = {
.name = "statistics"
};

+static int print_cpumask(char *buf, size_t size, cpumask_t *mask, bool as_list)
+{
+ int written;
+

+ if (as_list)
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,0,0)
+ written = scnprintf(buf, size, "%*pbl\n",
+ cpumask_pr_args(mask));
+ else
+ written = scnprintf(buf, size, "%*pb\n",
+ cpumask_pr_args(mask));
+#else
+ written = cpulist_scnprintf(buf, size, mask);
+ else
+ written = cpumask_scnprintf(buf, size, mask);
+ written += scnprintf(buf + written, size - written, "\n");
+#endif

+
+ return written;
+}
+

static ssize_t name_show(struct kobject *kobj, struct kobj_attribute *attr,
char *buffer)
{
@@ -170,16 +191,8 @@ static ssize_t cpus_assigned_show(struct kobject *kobj,

struct kobj_attribute *attr, char *buf)

{

struct cell *cell = container_of(kobj, struct cell, kobj);

- int written;

-#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,0,0)
- written = scnprintf(buf, PAGE_SIZE, "%*pb\n",
- cpumask_pr_args(&cell->cpus_assigned));
-#else
- written = cpumask_scnprintf(buf, PAGE_SIZE, &cell->cpus_assigned);
- written += scnprintf(buf + written, PAGE_SIZE - written, "\n");
-#endif
- return written;

+ return print_cpumask(buf, PAGE_SIZE, &cell->cpus_assigned, false);

}

static ssize_t cpus_failed_show(struct kobject *kobj,

@@ -199,13 +212,7 @@ static ssize_t cpus_failed_show(struct kobject *kobj,
JAILHOUSE_CPU_FAILED)
cpumask_set_cpu(cpu, cpus_failed);

-#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,0,0)
- written = scnprintf(buf, PAGE_SIZE, "%*pb\n",
- cpumask_pr_args(cpus_failed));
-#else
- written = cpumask_scnprintf(buf, PAGE_SIZE, cpus_failed);
- written += scnprintf(buf + written, PAGE_SIZE - written, "\n");
-#endif

+ written = print_cpumask(buf, PAGE_SIZE, cpus_failed, false);

free_cpumask_var(cpus_failed);

--
2.9.3

[Ralf Ramsauer]

Directories in /sys/devices/jailhouse/cells/ formerly represented cell names.

This patch changes this structure. From now on, /sys/devices/jailhouse/cells/
will contain cell ids. The name of a specific cell got moved to
/sys/devices/jailhouse/cells/<id>/name.

This makes the sysfs interface easier to be parsed by applications.

Also align documentation, jailhouse-cell-list and
jailhouse-completion.bash to the changes.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>

diff --git a/Documentation/sysfs-entries.txt b/Documentation/sysfs-entries.txt
index 2845220..0fc315a 100644
--- a/Documentation/sysfs-entries.txt
+++ b/Documentation/sysfs-entries.txt
@@ -11,8 +11,8 @@ can be used for monitoring the state of the hypervisor and its cells.
|- remap_pool_size - number of pages in hypervisor remapping pool
|- remap_pool_used - used pages of hypervisor remapping pool
`- cells
- |- <name of cell>
- | |- id - unique numerical ID
+ |- <id> - unique numerical ID
+ | |- name - cell name
| |- state - "running", "running/locked", "shut down", or
| | "failed"
| |- cpus_assigned - bitmask of assigned logical CPUs

diff --git a/driver/sysfs.c b/driver/sysfs.c
index a18c2b9..8c2a11f 100644
--- a/driver/sysfs.c
+++ b/driver/sysfs.c
@@ -139,12 +139,12 @@ static struct attribute_group stats_attr_group = {
.name = "statistics"
};

-static ssize_t id_show(struct kobject *kobj, struct kobj_attribute *attr,
- char *buffer)
+static ssize_t name_show(struct kobject *kobj, struct kobj_attribute *attr,
+ char *buffer)

{
struct cell *cell = container_of(kobj, struct cell, kobj);

- return sprintf(buffer, "%u\n", cell->id);
+ return sprintf(buffer, "%s\n", cell->name);
}

static ssize_t state_show(struct kobject *kobj, struct kobj_attribute *attr,
@@ -212,14 +212,14 @@ static ssize_t cpus_failed_show(struct kobject *kobj,
return written;
}

-static struct kobj_attribute cell_id_attr = __ATTR_RO(id);

+static struct kobj_attribute cell_name_attr = __ATTR_RO(name);

static struct kobj_attribute cell_state_attr = __ATTR_RO(state);
static struct kobj_attribute cell_cpus_assigned_attr =
__ATTR_RO(cpus_assigned);

static struct kobj_attribute cell_cpus_failed_attr = __ATTR_RO(cpus_failed);

static struct attribute *cell_attrs[] = {

- &cell_id_attr.attr,
+ &cell_name_attr.attr,
&cell_state_attr.attr,
&cell_cpus_assigned_attr.attr,
&cell_cpus_failed_attr.attr,
@@ -236,8 +236,8 @@ int jailhouse_sysfs_cell_create(struct cell *cell)
{
int err;

- err = kobject_init_and_add(&cell->kobj, &cell_type, cells_dir, "%s",
- cell->name);

+ err = kobject_init_and_add(&cell->kobj, &cell_type, cells_dir, "%d",

+ cell->id);
if (err) {
jailhouse_cell_kobj_release(&cell->kobj);
return err;

diff --git a/tools/jailhouse-cell-list b/tools/jailhouse-cell-list
index 3b5bc20..d4123f6 100755
--- a/tools/jailhouse-cell-list
+++ b/tools/jailhouse-cell-list

@@ -54,8 +54,8 @@ if len(sys.argv) > 1:
cells = []

for cell_path in glob.glob('/sys/devices/jailhouse/cells/*'):

cells.append({
- 'name': os.path.basename(cell_path),
- 'id': open(cell_path + "/id").readline().strip(),
+ 'id': os.path.basename(cell_path),

+ 'name': open(cell_path + "/name").readline().strip(),

'state': open(cell_path + "/state").readline().strip(),

'cpus_assigned': read_cpus(cell_path + "/cpus_assigned"),

'cpus_failed': read_cpus(cell_path + "/cpus_failed")

diff --git a/tools/jailhouse-completion.bash b/tools/jailhouse-completion.bash
index a20ea3c..2e4b379 100644
--- a/tools/jailhouse-completion.bash
+++ b/tools/jailhouse-completion.bash
@@ -66,12 +66,9 @@ function _jailhouse_get_id() {

# get possible ids and names
if [ -d /sys/devices/jailhouse/cells ]; then
- for i in /sys/devices/jailhouse/cells/*/id; do
- ids="${ids} $(cat "${i}" | tr '\n' ' ')"
- done
- for n in /sys/devices/jailhouse/cells/*; do
- _quote_readline_by_ref "${n##*/}" quoted
- names="${names} ${quoted}"
+ for i in /sys/devices/jailhouse/cells/*; do
+ ids="${ids} ${i##*/}"
+ names="${names} $(<${i}/name)"
done
fi

@@ -85,9 +82,8 @@ function _jailhouse_get_id() {

# get possible names
if [ -d /sys/devices/jailhouse/cells ]; then
- for n in /sys/devices/jailhouse/cells/*; do
- _quote_readline_by_ref "${n##*/}" quoted
- names="${names} ${quoted}"
+ for n in /sys/devices/jailhouse/cells/*/name; do
+ names="${names} $(<${n})"
done
fi

--
2.9.3

[Ralf Ramsauer]

Factor out printing of failed_cpus to an extra function and provide
as_list format flag.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>

diff --git a/driver/sysfs.c b/driver/sysfs.c
index 33931d9..b6bf545 100644
--- a/driver/sysfs.c
+++ b/driver/sysfs.c

@@ -160,6 +160,29 @@ static int print_cpumask(char *buf, size_t size, cpumask_t *mask, bool as_list)
return written;
}

+static int print_failed_cpus(char *buf, size_t size, const struct cell *cell,
+ bool as_list)
+{
+ cpumask_var_t cpus_failed;
+ unsigned int cpu;

+ int written;
+

+ if (!zalloc_cpumask_var(&cpus_failed, GFP_KERNEL))
+ return -ENOMEM;
+
+ for_each_cpu(cpu, &cell->cpus_assigned)
+ if (jailhouse_call_arg2(JAILHOUSE_HC_CPU_GET_INFO, cpu,
+ JAILHOUSE_CPU_INFO_STATE) ==
+ JAILHOUSE_CPU_FAILED)
+ cpumask_set_cpu(cpu, cpus_failed);
+
+ written = print_cpumask(buf, size, cpus_failed, as_list);
+
+ free_cpumask_var(cpus_failed);

+
+ return written;
+}
+

static ssize_t name_show(struct kobject *kobj, struct kobj_attribute *attr,

char *buffer)
{
@@ -199,24 +222,8 @@ static ssize_t cpus_failed_show(struct kobject *kobj,

struct kobj_attribute *attr, char *buf)
{

struct cell *cell = container_of(kobj, struct cell, kobj);

- cpumask_var_t cpus_failed;
- unsigned int cpu;
- int written;
-
- if (!zalloc_cpumask_var(&cpus_failed, GFP_KERNEL))
- return -ENOMEM;
-
- for_each_cpu(cpu, &cell->cpus_assigned)
- if (jailhouse_call_arg2(JAILHOUSE_HC_CPU_GET_INFO, cpu,
- JAILHOUSE_CPU_INFO_STATE) ==
- JAILHOUSE_CPU_FAILED)
- cpumask_set_cpu(cpu, cpus_failed);

- written = print_cpumask(buf, PAGE_SIZE, cpus_failed, false);
-
- free_cpumask_var(cpus_failed);
-
- return written;

+ return print_failed_cpus(buf, PAGE_SIZE, cell, false);
}

static struct kobj_attribute cell_name_attr = __ATTR_RO(name);

--
2.9.3

[Ralf Ramsauer]

If the cell id is chosen by the driver instead of the hypervisor, the
cell id gets part of the cell configuration.

As part of the configuration, move the cell id from hypervisor's struct
cell to struct jailhouse_cell_desc, as this structure is passed from the
driver to the hypervisor.

Let the hypervisor as well as the driver check collisions of cells.
Both, cell name and cell id are still both unique values.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>

diff --git a/driver/cell.c b/driver/cell.c
index 06e2b7f..b8c4403 100644
--- a/driver/cell.c
+++ b/driver/cell.c
@@ -40,18 +40,30 @@ void jailhouse_cell_kobj_release(struct kobject *kobj)
static struct cell *cell_create(const struct jailhouse_cell_desc *cell_desc)
{
struct cell *cell;
+ unsigned int id;
int err;

if (cell_desc->num_memory_regions >=
ULONG_MAX / sizeof(struct jailhouse_memory))
return ERR_PTR(-EINVAL);

+ /* determine cell id */
+ id = 0;
+retry:
+ list_for_each_entry(cell, &cells, entry)
+ if (cell->id == id) {
+ id++;
+ goto retry;
+ }
+
cell = kzalloc(sizeof(*cell), GFP_KERNEL);
if (!cell)
return ERR_PTR(-ENOMEM);

INIT_LIST_HEAD(&cell->entry);

+ cell->id = id;
+
bitmap_copy(cpumask_bits(&cell->cpus_assigned),
jailhouse_cell_cpu_set(cell_desc),
min(nr_cpumask_bits, (int)cell_desc->cpu_set_size * 8));
@@ -146,7 +158,7 @@ int jailhouse_cmd_cell_create(struct jailhouse_cell_create __user *arg)
void __user *user_config;
struct cell *cell;
unsigned int cpu;
- int id, err = 0;
+ int err = 0;

if (copy_from_user(&cell_params, arg, sizeof(cell_params)))
return -EFAULT;
@@ -193,6 +205,8 @@ int jailhouse_cmd_cell_create(struct jailhouse_cell_create __user *arg)
goto unlock_out;
}

+ config->id = cell->id;
+
if (!cpumask_subset(&cell->cpus_assigned, &root_cell->cpus_assigned)) {
err = -EBUSY;
goto error_cell_delete;
@@ -211,13 +225,10 @@ int jailhouse_cmd_cell_create(struct jailhouse_cell_create __user *arg)
jailhouse_pci_do_all_devices(cell, JAILHOUSE_PCI_TYPE_DEVICE,
JAILHOUSE_PCI_ACTION_CLAIM);

- id = jailhouse_call_arg1(JAILHOUSE_HC_CELL_CREATE, __pa(config));
- if (id < 0) {
- err = id;
+ err = jailhouse_call_arg1(JAILHOUSE_HC_CELL_CREATE, __pa(config));
+ if (err < 0)
goto error_cpu_online;
- }

- cell->id = id;
cell_register(cell);

pr_info("Created Jailhouse cell \"%s\"\n", config->name);
diff --git a/hypervisor/arch/arm/mmu_cell.c b/hypervisor/arch/arm/mmu_cell.c
index d26c9d3..45f0f0e 100644
--- a/hypervisor/arch/arm/mmu_cell.c
+++ b/hypervisor/arch/arm/mmu_cell.c
@@ -92,7 +92,7 @@ void arm_cell_dcaches_flush(struct cell *cell, enum dcache_flush flush)

int arm_paging_cell_init(struct cell *cell)
{
- if (cell->id > 0xff)
+ if (cell->config->id > 0xff)
return trace_error(-E2BIG);

cell->arch.mm.root_paging = cell_paging;
@@ -115,7 +115,7 @@ void arm_paging_vcpu_init(struct paging_structures *pg_structs)
unsigned long cell_table = paging_hvirt2phys(pg_structs->root_table);
u64 vttbr = 0;

- vttbr |= (u64)this_cell()->id << VTTBR_VMID_SHIFT;
+ vttbr |= (u64)this_cell()->config->id << VTTBR_VMID_SHIFT;
vttbr |= (u64)(cell_table & TTBR_MASK);

arm_write_sysreg(VTTBR_EL2, vttbr);
diff --git a/hypervisor/arch/x86/amd_iommu.c b/hypervisor/arch/x86/amd_iommu.c
index b3b8087..d91b7b3 100644
--- a/hypervisor/arch/x86/amd_iommu.c
+++ b/hypervisor/arch/x86/amd_iommu.c
@@ -360,7 +360,7 @@ int iommu_cell_init(struct cell *cell)
if (iommu_units_count == 0)
return 0;

- if (cell->id > 0xffff)
+ if (cell->config->id > 0xffff)
return trace_error(-ERANGE);

return 0;
@@ -529,7 +529,7 @@ int iommu_add_pci_device(struct cell *cell, struct pci_device *device)
memset(dte, 0, sizeof(*dte));

/* DomainID */
- dte->raw64[1] = cell->id & 0xffff;
+ dte->raw64[1] = cell->config->id & 0xffff;

/* Translation information */
dte->raw64[0] = DTE_IR | DTE_IW |
@@ -689,9 +689,9 @@ void iommu_config_commit(struct cell *cell_added_removed)
/* Flush caches */
if (cell_added_removed) {
amd_iommu_invalidate_pages(iommu,
- cell_added_removed->id & 0xffff);
+ cell_added_removed->config->id & 0xffff);
amd_iommu_invalidate_pages(iommu,
- root_cell.id & 0xffff);
+ root_cell.config->id & 0xffff);
}
/* Execute all commands in the buffer */
amd_iommu_completion_wait(iommu);
diff --git a/hypervisor/arch/x86/vtd.c b/hypervisor/arch/x86/vtd.c
index 546f160..0a75fb3 100644
--- a/hypervisor/arch/x86/vtd.c
+++ b/hypervisor/arch/x86/vtd.c
@@ -809,7 +809,7 @@ int iommu_add_pci_device(struct cell *cell, struct pci_device *device)
paging_hvirt2phys(cell->arch.vtd.pg_structs.root_table);
context_entry->hi_word =
(dmar_pt_levels == 3 ? VTD_CTX_AGAW_39 : VTD_CTX_AGAW_48) |
- (cell->id << VTD_CTX_DID_SHIFT);
+ (cell->config->id << VTD_CTX_DID_SHIFT);
arch_paging_flush_cpu_caches(context_entry, sizeof(*context_entry));

return 0;
@@ -853,7 +853,7 @@ int iommu_cell_init(struct cell *cell)
unsigned int n;
int result;

- if (cell->id >= dmar_num_did)
+ if (cell->config->id >= dmar_num_did)
return trace_error(-ERANGE);

cell->arch.vtd.pg_structs.root_paging = vtd_paging;
@@ -1033,8 +1033,8 @@ void iommu_config_commit(struct cell *cell_added_removed)
dmar_units_initialized = true;
} else {
if (cell_added_removed)
- vtd_flush_domain_caches(cell_added_removed->id);
- vtd_flush_domain_caches(root_cell.id);
+ vtd_flush_domain_caches(cell_added_removed->config->id);
+ vtd_flush_domain_caches(root_cell.config->id);
}
}

diff --git a/hypervisor/control.c b/hypervisor/control.c
index 8fd9b88..1d48623 100644
--- a/hypervisor/control.c
+++ b/hypervisor/control.c
@@ -147,21 +147,6 @@ static void cell_reconfig_completed(void)
MSG_INFORMATION);
}

-static unsigned int get_free_cell_id(void)
-{
- unsigned int id = 0;
- struct cell *cell;
-
-retry:
- for_each_cell(cell)
- if (cell->id == id) {
- id++;
- goto retry;
- }
-
- return id;
-}
-
/**
* Initialize a new cell.
* @param cell Cell to be initialized.
@@ -178,8 +163,6 @@ int cell_init(struct cell *cell)
struct cpu_set *cpu_set;
int err;

- cell->id = get_free_cell_id();
-
if (cpu_set_size > PAGE_SIZE)
return trace_error(-EINVAL);
if (cpu_set_size > sizeof(cell->small_cpu_set.bitmap)) {
@@ -367,7 +350,8 @@ static int cell_create(struct per_cpu *cpu_data, unsigned long config_address)
* sizeof(cell->config->name) == sizeof(cfg->name) and
* cell->config->name is guaranteed to be null-terminated.
*/
- if (strcmp(cell->config->name, cfg->name) == 0) {
+ if (strcmp(cell->config->name, cfg->name) == 0 ||
+ cell->config->id == cfg->id) {
err = -EEXIST;
goto err_resume;
}
@@ -468,7 +452,7 @@ static int cell_create(struct per_cpu *cpu_data, unsigned long config_address)

cell_resume(cpu_data);

- return cell->id;
+ return 0;

err_destroy_cell:
cell_destroy_internal(cpu_data, cell);
@@ -501,7 +485,7 @@ static int cell_management_prologue(enum management_task task,
cell_suspend(&root_cell, cpu_data);

for_each_cell(*cell_ptr)
- if ((*cell_ptr)->id == id)
+ if ((*cell_ptr)->config->id == id)
break;

if (!*cell_ptr) {
@@ -652,7 +636,7 @@ static int cell_get_state(struct per_cpu *cpu_data, unsigned long id)
* this hypercall.
*/
for_each_cell(cell)
- if (cell->id == id) {
+ if (cell->config->id == id) {
u32 state = cell->comm_page.comm_region.cell_state;

switch (state) {
diff --git a/hypervisor/include/jailhouse/cell-config.h b/hypervisor/include/jailhouse/cell-config.h
index 8e8495d..4e2b5cb 100644
--- a/hypervisor/include/jailhouse/cell-config.h
+++ b/hypervisor/include/jailhouse/cell-config.h
@@ -48,6 +48,7 @@
struct jailhouse_cell_desc {
char signature[8];
char name[JAILHOUSE_CELL_NAME_MAXLEN+1];
+ unsigned int id; /* set by the driver */
__u32 flags;

__u32 cpu_set_size;
diff --git a/hypervisor/include/jailhouse/cell.h b/hypervisor/include/jailhouse/cell.h
index 024af9e..75e4fd1 100644
--- a/hypervisor/include/jailhouse/cell.h
+++ b/hypervisor/include/jailhouse/cell.h
@@ -35,8 +35,6 @@ struct cell {
/** Architecture-specific fields. */
struct arch_cell arch;

- /** ID of the cell. */
- unsigned int id;
/** Number of pages used for storing cell-specific states and
* configuration data. */
unsigned int data_pages;
diff --git a/hypervisor/setup.c b/hypervisor/setup.c
index 4b79189..b76f845 100644
--- a/hypervisor/setup.c
+++ b/hypervisor/setup.c
@@ -53,7 +53,6 @@ static void init_early(unsigned int cpu_id)

root_cell.config = &system_config->root_cell;

- root_cell.id = -1;
error = cell_init(&root_cell);
if (error)
return;
--
2.9.3

[Ralf Ramsauer]

From: Ralf Ramsauer <ralf.r...@othr.de>

Hi all,

I'd like to replace jailhouse-cell-list. It's a rudimentary function and
requires python. So I reimplemented cell-list natively in C.

This series also changes the sysfs structure as Jan proposed.

Just an additional idea, don't know if this could be useful: Jailhouse driver
now chooses the cell ID which is stored inside struct jailhouse_cell_desc. With
a few more lines of code, the cell id could also be optionally statically be pinned
to the configuration.

Though in total we have more lines of C code, the hypervisor shrinked a bit.

I tested the code on a Jetson TK1 and Qemu VM. For me, it works so far ;-)
Applies against next.

changes since v3:
- let the driver choose the cell ID
- use a filter function for scandir()
- remove jailhouse-cell-list from tools/Makefile
- adapt tools/jailhouse-completion.bash
- minor cosmetic changes

changes since v2:
- do not use struct jailhouse_cell_id inside struct cell
- adapt read_file() as suggested by Jan
- amend commit messages
- use scandir() and alphasort for sorted output

changes since v1:
- change sysfs structure (swap id and name)
- provide human readable sysfs nodes
- improve changes on read_file()

Ralf Ramsauer (9):

driver: cell: add cell name to struct cell
driver: sysfs: do not pass cell name to jh_sysfs_cell_create

hypervisor, driver: let the driver choose cell ids

driver: sysfs: swap sysfs entries name and id
driver: sysfs: consolidate cpumask printing
driver: sysfs: factor out printing of failed_cpus
driver: sysfs: add new sysfs nodes cpus_{assigned,failed}_list
tools: jailhouse: make read_file() compatible with sysfs files
tools: jailhouse: reimplement jailhouse-cell-list in C

Documentation/sysfs-entries.txt | 4 +-

driver/cell.c | 36 ++++++---
driver/cell.h | 1 +
driver/sysfs.c | 112 +++++++++++++++++---------
driver/sysfs.h | 2 +-
hypervisor/arch/arm/mmu_cell.c | 4 +-
hypervisor/arch/x86/amd_iommu.c | 8 +-
hypervisor/arch/x86/vtd.c | 8 +-
hypervisor/control.c | 26 ++----
hypervisor/include/jailhouse/cell-config.h | 1 +
hypervisor/include/jailhouse/cell.h | 2 -
hypervisor/setup.c | 1 -
tools/Makefile | 1 -
tools/jailhouse-cell-list | 71 -----------------
tools/jailhouse-completion.bash | 14 ++--
tools/jailhouse.c | 124 ++++++++++++++++++++++++++++-
16 files changed, 244 insertions(+), 171 deletions(-)
delete mode 100755 tools/jailhouse-cell-list

--
2.9.3

[Ralf Ramsauer]

Files in sysfs or procfs may be smaller than st_size actually reports.
We will read file from sysfs in future.

This patch makes the strong assumption that the st_size field in struct
stat can be seen as an upper bound for the actual file content.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>

diff --git a/tools/jailhouse.c b/tools/jailhouse.c
index be5b833..ae2ed3f 100644
--- a/tools/jailhouse.c
+++ b/tools/jailhouse.c

@@ -128,6 +128,7 @@ static void *read_string(const char *string, size_t *size)

static void *read_file(const char *name, size_t *size)

{
struct stat stat;
+ ssize_t result;
void *buffer;
int fd;

@@ -148,7 +149,8 @@ static void *read_file(const char *name, size_t *size)
exit(1);
}

- if (read(fd, buffer, stat.st_size) < stat.st_size) {
+ result = read(fd, buffer, stat.st_size);
+ if (result < 0) {
fprintf(stderr, "reading %s: %s\n", name, strerror(errno));
exit(1);
}
@@ -156,7 +158,7 @@ static void *read_file(const char *name, size_t *size)
close(fd);

if (size)
- *size = stat.st_size;
+ *size = (size_t)result;

return buffer;
}
--

2.9.3

[Ralf Ramsauer]

And keep the output format of the former python script.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>

diff --git a/tools/Makefile b/tools/Makefile
index 6975a76..9b6e176 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -21,7 +21,6 @@ TARGETS := jailhouse
INST_TARGETS := $(TARGETS)
HELPERS := \
jailhouse-cell-linux \
- jailhouse-cell-list \
jailhouse-cell-stats \
jailhouse-config-create \
jailhouse-hardware-check

diff --git a/tools/jailhouse.c b/tools/jailhouse.c
index ae2ed3f..58decf1 100644
--- a/tools/jailhouse.c
+++ b/tools/jailhouse.c

+ if (size)

+ ret[size-1] = 0;
+
+ return ret;
+}
+
static int enable(int argc, char *argv[])
{
void *config;

@@ -251,6 +274,97 @@ static bool match_opt(const char *argv, const char *short_opt,

strcmp(argv, long_opt) == 0;
}

+static struct jailhouse_cell_info *get_cell_info(const int id)
+{

+ char buf[128];

+ struct jailhouse_cell_info *cinfo;
+ char *tmp;
+

+ cinfo = malloc(sizeof(struct jailhouse_cell_info));
+ if (cinfo == NULL) {
+ fprintf(stderr, "insufficient memory\n");
+ exit(1);
+ }

+
+ /* set cell id */
+ cinfo->id.id = id;
+
+ /* get cell name */
+ snprintf(buf, sizeof(buf), JAILHOUSE_CELLS "%d/name", id);
+ tmp = read_sysfs_entry(buf);
+ strncpy(cinfo->id.name, tmp, JAILHOUSE_CELL_ID_NAMELEN);

+ cinfo->id.name[JAILHOUSE_CELL_ID_NAMELEN] = 0;

+ free(tmp);
+
+ /* get cell state */
+ snprintf(buf, sizeof(buf), JAILHOUSE_CELLS "%d/state", id);
+ cinfo->state = read_sysfs_entry(buf);
+
+ /* get assigned cpu list */
+ snprintf(buf, sizeof(buf), JAILHOUSE_CELLS "%d/cpus_assigned_list", id);
+ cinfo->cpus_assigned_list = read_sysfs_entry(buf);
+
+ /* get failed cpu list */
+ snprintf(buf, sizeof(buf), JAILHOUSE_CELLS "%d/cpus_failed_list", id);
+ cinfo->cpus_failed_list = read_sysfs_entry(buf);
+
+ return cinfo;
+}
+
+static void cell_info_free(struct jailhouse_cell_info *cinfo)
+{
+ free(cinfo->state);
+ free(cinfo->cpus_assigned_list);
+ free(cinfo->cpus_failed_list);
+ free(cinfo);
+}

+
+static int cell_match(const struct dirent *dirent)
+{
+ return dirent->d_name[0] != '.';
+}

+
+static int cell_list(int argc, char *argv[])
+{
+ struct dirent **namelist;
+ struct jailhouse_cell_info *cinfo;
+ int id, i, num_entries;
+ (void)argv;
+
+ if (argc != 3)
+ help(argv[0], 1);
+

+ num_entries = scandir(JAILHOUSE_CELLS, &namelist, cell_match, alphasort);
+ if (num_entries == -1) {
+ /* Silently return if kernel module is not loaded */
+ if (errno == ENOENT)
+ return 0;
+
+ perror("scandir");
+ return -1;
+ }
+
+ if (num_entries == 0)
+ /* Silently return if jailhouse is not enabled */
+ goto namelist_free;

+
+ printf("%-8s%-24s%-16s%-24s%-24s\n",
+ "ID", "Name", "State", "Assigned CPUs", "Failed CPUs");

+ for (i = 0; i < num_entries; i++) {

+ id = (int)strtol(namelist[i]->d_name, NULL, 10);
+
+ cinfo = get_cell_info(id);
+ printf("%-8d%-24s%-16s%-24s%-24s\n", cinfo->id.id, cinfo->id.name,
+ cinfo->state, cinfo->cpus_assigned_list, cinfo->cpus_failed_list);

+ cell_info_free(cinfo);

+ free(namelist[i]);
+ }
+

+namelist_free:
+ free(namelist);

+ return 0;
+}
+
static int cell_shutdown_load(int argc, char *argv[],
enum shutdown_load_mode mode)
{

@@ -369,6 +483,8 @@ static int cell_management(int argc, char *argv[])

if (strcmp(argv[2], "create") == 0) {
err = cell_create(argc, argv);
+ } else if (strcmp(argv[2], "list") == 0) {
+ err = cell_list(argc, argv);
} else if (strcmp(argv[2], "load") == 0) {
err = cell_shutdown_load(argc, argv, LOAD);
} else if (strcmp(argv[2], "start") == 0) {

--
2.9.3

[Ralf Ramsauer]

Add human readable nodes for cpus_assigned and cpus_failed and let
jailhouse-cell-list use those nodes.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>

diff --git a/driver/sysfs.c b/driver/sysfs.c
index b6bf545..ce4dba7 100644
--- a/driver/sysfs.c
+++ b/driver/sysfs.c
@@ -218,6 +218,14 @@ static ssize_t cpus_assigned_show(struct kobject *kobj,

return print_cpumask(buf, PAGE_SIZE, &cell->cpus_assigned, false);
}

+static ssize_t cpus_assigned_list_show(struct kobject *kobj,

+ struct kobj_attribute *attr, char *buf)
+{
+ struct cell *cell = container_of(kobj, struct cell, kobj);
+
+ return print_cpumask(buf, PAGE_SIZE, &cell->cpus_assigned, true);
+}
+

static ssize_t cpus_failed_show(struct kobject *kobj,
struct kobj_attribute *attr, char *buf)
{

@@ -226,17 +234,31 @@ static ssize_t cpus_failed_show(struct kobject *kobj,

return print_failed_cpus(buf, PAGE_SIZE, cell, false);
}

+static ssize_t cpus_failed_list_show(struct kobject *kobj,

+ struct kobj_attribute *attr, char *buf)
+{
+ struct cell *cell = container_of(kobj, struct cell, kobj);
+
+ return print_failed_cpus(buf, PAGE_SIZE, cell, true);
+}
+

static struct kobj_attribute cell_name_attr = __ATTR_RO(name);

static struct kobj_attribute cell_state_attr = __ATTR_RO(state);
static struct kobj_attribute cell_cpus_assigned_attr =
__ATTR_RO(cpus_assigned);

+static struct kobj_attribute cell_cpus_assigned_list_attr =
+ __ATTR_RO(cpus_assigned_list);

static struct kobj_attribute cell_cpus_failed_attr = __ATTR_RO(cpus_failed);

+static struct kobj_attribute cell_cpus_failed_list_attr =
+ __ATTR_RO(cpus_failed_list);

static struct attribute *cell_attrs[] = {

&cell_name_attr.attr,
&cell_state_attr.attr,
&cell_cpus_assigned_attr.attr,
+ &cell_cpus_assigned_list_attr.attr,
&cell_cpus_failed_attr.attr,
+ &cell_cpus_failed_list_attr.attr,
NULL,
};

diff --git a/tools/jailhouse-cell-list b/tools/jailhouse-cell-list
index d4123f6..d5a535b 100755
--- a/tools/jailhouse-cell-list
+++ b/tools/jailhouse-cell-list

print("usage: %s" % os.path.basename(sys.argv[0]).replace("-", " "))

exit(0 if sys.argv[1] in ("--help", "-h") else 1)

@@ -57,8 +25,10 @@ for cell_path in glob.glob('/sys/devices/jailhouse/cells/*'):
'id': os.path.basename(cell_path),

'name': open(cell_path + "/name").readline().strip(),

'state': open(cell_path + "/state").readline().strip(),

- 'cpus_assigned': read_cpus(cell_path + "/cpus_assigned"),
- 'cpus_failed': read_cpus(cell_path + "/cpus_failed")
+ 'cpus_assigned_list':
+ open(cell_path + "/cpus_assigned_list").readline().strip(),
+ 'cpus_failed_list':
+ open(cell_path + "/cpus_failed_list").readline().strip()
})

line_format = "%-8s%-24s%-16s%-24s%-24s"
@@ -67,5 +37,4 @@ if not cells == []:

"Assigned CPUs", "Failed CPUs"))

for cell in sorted(cells, key=lambda cell: cell['id']):

print(line_format % (cell['id'], cell['name'], cell['state'],

- print_cpus(cell['cpus_assigned']),
- print_cpus(cell['cpus_failed'])))

+ cell['cpus_assigned_list'], cell['cpus_failed_list']))
--
2.9.3

[Jan Kiszka]

On 2016-08-22 11:39, Ralf Ramsauer wrote:
> From: Ralf Ramsauer <ralf.r...@othr.de>
>
> Hi all,
>
> I'd like to replace jailhouse-cell-list. It's a rudimentary function and
> requires python. So I reimplemented cell-list natively in C.
>
> This series also changes the sysfs structure as Jan proposed.
>
> Just an additional idea, don't know if this could be useful: Jailhouse driver
> now chooses the cell ID which is stored inside struct jailhouse_cell_desc. With
> a few more lines of code, the cell id could also be optionally statically be pinned
> to the configuration.

Not sure, not seeing a use case yet. It increases the risk of
collisions, though.

>
> Though in total we have more lines of C code, the hypervisor shrinked a bit.

Good!

Update to Documentation/hypervisor-interfaces.txt missing.

BTW, nothing speaks against keeping the per-patch diff stats. They
actually help providing an overview what a patch touches.

> driver/cell.c | 36 ++++++---
> driver/cell.h | 1 +
> driver/sysfs.c | 112 +++++++++++++++++---------
> driver/sysfs.h | 2 +-
> hypervisor/arch/arm/mmu_cell.c | 4 +-
> hypervisor/arch/x86/amd_iommu.c | 8 +-
> hypervisor/arch/x86/vtd.c | 8 +-
> hypervisor/control.c | 26 ++----
> hypervisor/include/jailhouse/cell-config.h | 1 +
> hypervisor/include/jailhouse/cell.h | 2 -
> hypervisor/setup.c | 1 -
> tools/Makefile | 1 -
> tools/jailhouse-cell-list | 71 -----------------
> tools/jailhouse-completion.bash | 14 ++--
> tools/jailhouse.c | 124 ++++++++++++++++++++++++++++-
> 16 files changed, 244 insertions(+), 171 deletions(-)
> delete mode 100755 tools/jailhouse-cell-list
>

Thanks,
Jan

--
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

[Jan Kiszka]

On 2016-08-22 11:39, Ralf Ramsauer wrote:

read_sysfs_string - not everything in sysfs is a string, so this
functions is not as generic as it sounds.

> +{
> + char *ret;
> + size_t size;
> +
> + ret = read_file(name, &size);
> +
> + /* enforce the string to be null-terminated */
> + if (size)
> + ret[size-1] = 0;

What happens if size is 0? And shouldn't this append the 0, not
overwrite the last character? Or is the last one always a linefeed?

snprintf could be pushed into read_sysfs_string. Just pass the entry
name and the id and build everything there. Would avoid this repeating
pattern.

...or:

if (num_entries > 0)
printf(banner)

But that's just a matter of taste.

[Ralf Ramsauer]

Good point.

>
>> +{
>> + char *ret;
>> + size_t size;
>> +
>> + ret = read_file(name, &size);
>> +
>> + /* enforce the string to be null-terminated */
>> + if (size)
>> + ret[size-1] = 0;
>
> What happens if size is 0? And shouldn't this append the 0, not
> overwrite the last character? Or is the last one always a linefeed?

In case of jailhouse, it's always a linefeed. I'll fix it and try to
make it more general.

Ack

Good taste :-)

Ralf

Ralf Ramsauer
GPG: 0x8F10049B

[Ralf Ramsauer]

struct cell now has information about the cell name. It is not necessary
to pass the name of the cell to jh_sysfs_cell_create.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>

---
driver/cell.c | 2 +-
driver/sysfs.c | 4 ++--
driver/sysfs.h | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/driver/cell.c b/driver/cell.c
index 3cccf80..06e2b7f 100644
--- a/driver/cell.c
+++ b/driver/cell.c
@@ -77,7 +77,7 @@ static struct cell *cell_create(const struct jailhouse_cell_desc *cell_desc)

return ERR_PTR(err);
}

- err = jailhouse_sysfs_cell_create(cell, cell_desc->name);
+ err = jailhouse_sysfs_cell_create(cell);
if (err)
/* cleanup done by jailhouse_sysfs_cell_create */
return ERR_PTR(err);

diff --git a/driver/sysfs.c b/driver/sysfs.c
index 4959b56..a18c2b9 100644
--- a/driver/sysfs.c
+++ b/driver/sysfs.c

@@ -232,12 +232,12 @@ static struct kobj_type cell_type = {
.default_attrs = cell_attrs,
};

-int jailhouse_sysfs_cell_create(struct cell *cell, const char *name)

+int jailhouse_sysfs_cell_create(struct cell *cell)
{
int err;

err = kobject_init_and_add(&cell->kobj, &cell_type, cells_dir, "%s",

- name);
+ cell->name);

if (err) {
jailhouse_cell_kobj_release(&cell->kobj);
return err;

diff --git a/driver/sysfs.h b/driver/sysfs.h
index ad0e2f9..f5cdc34 100644
--- a/driver/sysfs.h
+++ b/driver/sysfs.h
@@ -15,7 +15,7 @@

#include <linux/device.h>

-int jailhouse_sysfs_cell_create(struct cell *cell, const char *name);
+int jailhouse_sysfs_cell_create(struct cell *cell);
void jailhouse_sysfs_cell_register(struct cell *cell);
void jailhouse_sysfs_cell_delete(struct cell *cell);

--

2.9.3

[Ralf Ramsauer]

struct cell did not know the cell name. Add it.

This patch is preparatory work for exchanging the jailhouse sysfs cell
structure.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>
---

driver/cell.c | 11 ++++++-----
driver/cell.h | 1 +
2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/driver/cell.c b/driver/cell.c
index 8c4c0d8..3cccf80 100644
--- a/driver/cell.c
+++ b/driver/cell.c
@@ -64,6 +64,9 @@ static struct cell *cell_create(const struct jailhouse_cell_desc *cell_desc)
return ERR_PTR(-ENOMEM);
}

+ memcpy(cell->name, cell_desc->name, JAILHOUSE_CELL_ID_NAMELEN);
+ cell->name[JAILHOUSE_CELL_ID_NAMELEN] = 0;

+
memcpy(cell->memory_regions, jailhouse_cell_mem_regions(cell_desc),
sizeof(struct jailhouse_memory) * cell->num_memory_regions);

@@ -95,7 +98,7 @@ static struct cell *find_cell(struct jailhouse_cell_id *cell_id)
list_for_each_entry(cell, &cells, entry)
if (cell_id->id == cell->id ||
(cell_id->id == JAILHOUSE_CELL_ID_UNUSED &&
- strcmp(kobject_name(&cell->kobj), cell_id->name) == 0))
+ strcmp(cell->name, cell_id->name) == 0))
return cell;
return NULL;
}
@@ -400,8 +403,7 @@ static int cell_destroy(struct cell *cell)
jailhouse_pci_do_all_devices(cell, JAILHOUSE_PCI_TYPE_DEVICE,
JAILHOUSE_PCI_ACTION_RELEASE);

- pr_info("Destroyed Jailhouse cell \"%s\"\n",
- kobject_name(&cell->kobj));
+ pr_info("Destroyed Jailhouse cell \"%s\"\n", cell->name);

cell_delete(cell);

@@ -438,8 +440,7 @@ int jailhouse_cmd_cell_destroy_non_root(void)
continue;
err = cell_destroy(cell);
if (err) {
- pr_err("Jailhouse: failed to destroy cell \"%s\"\n",
- kobject_name(&cell->kobj));
+ pr_err("Jailhouse: failed to destroy cell \"%s\"\n", cell->name);
return err;
}
}
diff --git a/driver/cell.h b/driver/cell.h
index 649cca0..b0be1a4 100644

--- a/driver/cell.h
+++ b/driver/cell.h
@@ -26,6 +26,7 @@ struct cell {
struct kobject kobj;
struct list_head entry;
unsigned int id;
+ char name[JAILHOUSE_CELL_ID_NAMELEN+1];
cpumask_t cpus_assigned;
u32 num_memory_regions;
struct jailhouse_memory *memory_regions;
--

2.9.3

[Ralf Ramsauer]

Hi all,

I'd like to replace jailhouse-cell-list. It's a rudimentary function and
requires python. So I reimplemented cell-list natively in C.

This series also changes the sysfs structure as Jan proposed.

Though in total we have more lines of C code, the hypervisor shrinked a bit.

I tested the code on a Jetson TK1 and Qemu VM. For me, it works so far.
Applies against next.

changes since v4:
- maintain Documentation/hypervisor-interfaces.txt
- rename read_sysfs_entry to read_sysfs_cell_string
and avoid repeating snprintf()'s

Documentation/hypervisor-interfaces.txt | 4 +-
Documentation/sysfs-entries.txt | 4 +-
driver/cell.c | 36 +++++---
driver/cell.h | 1 +
driver/sysfs.c | 112 ++++++++++++++++---------

driver/sysfs.h | 2 +-
hypervisor/arch/arm/mmu_cell.c | 4 +-
hypervisor/arch/x86/amd_iommu.c | 8 +-
hypervisor/arch/x86/vtd.c | 8 +-
hypervisor/control.c | 26 ++----
hypervisor/include/jailhouse/cell-config.h | 1 +
hypervisor/include/jailhouse/cell.h | 2 -
hypervisor/setup.c | 1 -
tools/Makefile | 1 -
tools/jailhouse-cell-list | 71 ----------------

tools/jailhouse-completion.bash | 14 ++--
tools/jailhouse.c | 127 ++++++++++++++++++++++++++++-
17 files changed, 249 insertions(+), 173 deletions(-)
delete mode 100755 tools/jailhouse-cell-list

--
2.9.3

[Ralf Ramsauer]

If the cell id is chosen by the driver instead of the hypervisor, the
cell id gets part of the cell configuration.

As part of the configuration, move the cell id from hypervisor's struct
cell to struct jailhouse_cell_desc, as this structure is passed from the
driver to the hypervisor.

Let the hypervisor as well as the driver check collisions of cells.
Both, cell name and cell id are still both unique values.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>
---

Documentation/hypervisor-interfaces.txt | 4 ++--
driver/cell.c | 23 +++++++++++++++++------
hypervisor/arch/arm/mmu_cell.c | 4 ++--
hypervisor/arch/x86/amd_iommu.c | 8 ++++----
hypervisor/arch/x86/vtd.c | 8 ++++----
hypervisor/control.c | 26 +++++---------------------
hypervisor/include/jailhouse/cell-config.h | 1 +
hypervisor/include/jailhouse/cell.h | 2 --
hypervisor/setup.c | 1 -
9 files changed, 35 insertions(+), 42 deletions(-)

diff --git a/Documentation/hypervisor-interfaces.txt b/Documentation/hypervisor-interfaces.txt
index 50e6fac..6b8dead 100644
--- a/Documentation/hypervisor-interfaces.txt
+++ b/Documentation/hypervisor-interfaces.txt
@@ -105,7 +105,7 @@ This hypercall can only be issued on CPUs belonging to the Linux cell.
Arguments: 1. Guest-physical address of cell configuration (see [2] for
details)

-Return code: Positive cell ID or negative error code
+Return code: 0 on success or negative error code

Possible errors are:
-EPERM (-1) - hypercall was issued over a non-root cell or an active
@@ -115,7 +115,7 @@ Return code: Positive cell ID or negative error code
-EBUSY (-16) - a resource of the new cell is already in use by another
non-root cell, or the caller's CPU is supposed to be
given to the new cell
- -EEXIST (-17) - a cell with the given name already exists
+ -EEXIST (-17) - a cell with the given name or id already exists
-EINVAL (-22) - incorrect or inconsistent configuration data


diff --git a/driver/cell.c b/driver/cell.c
index 06e2b7f..b8c4403 100644
--- a/driver/cell.c
+++ b/driver/cell.c

@@ -40,18 +40,30 @@ void jailhouse_cell_kobj_release(struct kobject *kobj)

static struct cell *cell_create(const struct jailhouse_cell_desc *cell_desc)

index 8fd9b88..bdb8eae 100644

[Ralf Ramsauer]

We want to add two additional sysfs nodes for each cell:
cpus_assigned_list and cpus_failed_list.

A lot of printing-work would be redundant. As preparatory work,
consolidate it and add an additional as_list flag to allow
human readable cpumasks representation.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>
---

driver/sysfs.c | 39 +++++++++++++++++++++++----------------
1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/driver/sysfs.c b/driver/sysfs.c
index 8c2a11f..33931d9 100644
--- a/driver/sysfs.c
+++ b/driver/sysfs.c
@@ -139,6 +139,27 @@ static struct attribute_group stats_attr_group = {
.name = "statistics"
};

+static int print_cpumask(char *buf, size_t size, cpumask_t *mask, bool as_list)
+{
+ int written;
+

+ if (as_list)
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,0,0)
+ written = scnprintf(buf, size, "%*pbl\n",
+ cpumask_pr_args(mask));
+ else
+ written = scnprintf(buf, size, "%*pb\n",
+ cpumask_pr_args(mask));
+#else
+ written = cpulist_scnprintf(buf, size, mask);
+ else
+ written = cpumask_scnprintf(buf, size, mask);
+ written += scnprintf(buf + written, size - written, "\n");
+#endif

+
+ return written;
+}
+

static ssize_t name_show(struct kobject *kobj, struct kobj_attribute *attr,
char *buffer)
{
@@ -170,16 +191,8 @@ static ssize_t cpus_assigned_show(struct kobject *kobj,

struct kobj_attribute *attr, char *buf)

{

struct cell *cell = container_of(kobj, struct cell, kobj);

- int written;

-#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,0,0)
- written = scnprintf(buf, PAGE_SIZE, "%*pb\n",
- cpumask_pr_args(&cell->cpus_assigned));
-#else
- written = cpumask_scnprintf(buf, PAGE_SIZE, &cell->cpus_assigned);
- written += scnprintf(buf + written, PAGE_SIZE - written, "\n");
-#endif
- return written;

+ return print_cpumask(buf, PAGE_SIZE, &cell->cpus_assigned, false);

}

static ssize_t cpus_failed_show(struct kobject *kobj,

@@ -199,13 +212,7 @@ static ssize_t cpus_failed_show(struct kobject *kobj,
JAILHOUSE_CPU_FAILED)
cpumask_set_cpu(cpu, cpus_failed);

-#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,0,0)
- written = scnprintf(buf, PAGE_SIZE, "%*pb\n",
- cpumask_pr_args(cpus_failed));
-#else
- written = cpumask_scnprintf(buf, PAGE_SIZE, cpus_failed);
- written += scnprintf(buf + written, PAGE_SIZE - written, "\n");
-#endif

+ written = print_cpumask(buf, PAGE_SIZE, cpus_failed, false);

free_cpumask_var(cpus_failed);

--
2.9.3

[Ralf Ramsauer]

Factor out printing of failed_cpus to an extra function and provide
as_list format flag.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>
---

driver/sysfs.c | 41 ++++++++++++++++++++++++-----------------
1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/driver/sysfs.c b/driver/sysfs.c
index 33931d9..b6bf545 100644
--- a/driver/sysfs.c
+++ b/driver/sysfs.c

@@ -160,6 +160,29 @@ static int print_cpumask(char *buf, size_t size, cpumask_t *mask, bool as_list)
return written;
}

+static int print_failed_cpus(char *buf, size_t size, const struct cell *cell,
+ bool as_list)
+{
+ cpumask_var_t cpus_failed;
+ unsigned int cpu;

+ int written;
+

+ if (!zalloc_cpumask_var(&cpus_failed, GFP_KERNEL))
+ return -ENOMEM;
+
+ for_each_cpu(cpu, &cell->cpus_assigned)
+ if (jailhouse_call_arg2(JAILHOUSE_HC_CPU_GET_INFO, cpu,
+ JAILHOUSE_CPU_INFO_STATE) ==
+ JAILHOUSE_CPU_FAILED)
+ cpumask_set_cpu(cpu, cpus_failed);
+
+ written = print_cpumask(buf, size, cpus_failed, as_list);
+
+ free_cpumask_var(cpus_failed);

+
+ return written;
+}
+
static ssize_t name_show(struct kobject *kobj, struct kobj_attribute *attr,
char *buffer)
{

@@ -199,24 +222,8 @@ static ssize_t cpus_failed_show(struct kobject *kobj,

struct kobj_attribute *attr, char *buf)
{
struct cell *cell = container_of(kobj, struct cell, kobj);

- cpumask_var_t cpus_failed;
- unsigned int cpu;
- int written;
-
- if (!zalloc_cpumask_var(&cpus_failed, GFP_KERNEL))
- return -ENOMEM;
-
- for_each_cpu(cpu, &cell->cpus_assigned)
- if (jailhouse_call_arg2(JAILHOUSE_HC_CPU_GET_INFO, cpu,
- JAILHOUSE_CPU_INFO_STATE) ==
- JAILHOUSE_CPU_FAILED)
- cpumask_set_cpu(cpu, cpus_failed);

- written = print_cpumask(buf, PAGE_SIZE, cpus_failed, false);
-
- free_cpumask_var(cpus_failed);
-
- return written;
+ return print_failed_cpus(buf, PAGE_SIZE, cell, false);

}

static struct kobj_attribute cell_name_attr = __ATTR_RO(name);

--
2.9.3

[Ralf Ramsauer]

Add human readable nodes for cpus_assigned and cpus_failed and let
jailhouse-cell-list use those nodes.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>
---

driver/sysfs.c | 22 ++++++++++++++++++++++
tools/jailhouse-cell-list | 41 +++++------------------------------------
2 files changed, 27 insertions(+), 36 deletions(-)

diff --git a/driver/sysfs.c b/driver/sysfs.c
index b6bf545..ce4dba7 100644
--- a/driver/sysfs.c
+++ b/driver/sysfs.c
@@ -218,6 +218,14 @@ static ssize_t cpus_assigned_show(struct kobject *kobj,

return print_cpumask(buf, PAGE_SIZE, &cell->cpus_assigned, false);
}

+static ssize_t cpus_assigned_list_show(struct kobject *kobj,

+ struct kobj_attribute *attr, char *buf)
+{
+ struct cell *cell = container_of(kobj, struct cell, kobj);
+
+ return print_cpumask(buf, PAGE_SIZE, &cell->cpus_assigned, true);
+}
+

static ssize_t cpus_failed_show(struct kobject *kobj,
struct kobj_attribute *attr, char *buf)
{

@@ -226,17 +234,31 @@ static ssize_t cpus_failed_show(struct kobject *kobj,

return print_failed_cpus(buf, PAGE_SIZE, cell, false);
}

+static ssize_t cpus_failed_list_show(struct kobject *kobj,

+ struct kobj_attribute *attr, char *buf)
+{
+ struct cell *cell = container_of(kobj, struct cell, kobj);
+
+ return print_failed_cpus(buf, PAGE_SIZE, cell, true);
+}
+

static struct kobj_attribute cell_name_attr = __ATTR_RO(name);

print("usage: %s" % os.path.basename(sys.argv[0]).replace("-", " "))

exit(0 if sys.argv[1] in ("--help", "-h") else 1)

@@ -57,8 +25,10 @@ for cell_path in glob.glob('/sys/devices/jailhouse/cells/*'):
'id': os.path.basename(cell_path),

'name': open(cell_path + "/name").readline().strip(),

'state': open(cell_path + "/state").readline().strip(),

- 'cpus_assigned': read_cpus(cell_path + "/cpus_assigned"),
- 'cpus_failed': read_cpus(cell_path + "/cpus_failed")
+ 'cpus_assigned_list':
+ open(cell_path + "/cpus_assigned_list").readline().strip(),
+ 'cpus_failed_list':
+ open(cell_path + "/cpus_failed_list").readline().strip()
})

line_format = "%-8s%-24s%-16s%-24s%-24s"
@@ -67,5 +37,4 @@ if not cells == []:

"Assigned CPUs", "Failed CPUs"))

for cell in sorted(cells, key=lambda cell: cell['id']):

print(line_format % (cell['id'], cell['name'], cell['state'],

- print_cpus(cell['cpus_assigned']),
- print_cpus(cell['cpus_failed'])))

+ cell['cpus_assigned_list'], cell['cpus_failed_list']))
--
2.9.3

[Ralf Ramsauer]

Directories in /sys/devices/jailhouse/cells/ formerly represented cell names.

This patch changes this structure. From now on, /sys/devices/jailhouse/cells/
will contain cell ids. The name of a specific cell got moved to
/sys/devices/jailhouse/cells/<id>/name.

This makes the sysfs interface easier to be parsed by applications.

Also align documentation, jailhouse-cell-list and
jailhouse-completion.bash to the changes.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>
---

Documentation/sysfs-entries.txt | 4 ++--
driver/sysfs.c | 14 +++++++-------
tools/jailhouse-cell-list | 4 ++--
tools/jailhouse-completion.bash | 14 +++++---------
4 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/Documentation/sysfs-entries.txt b/Documentation/sysfs-entries.txt
index 2845220..0fc315a 100644
--- a/Documentation/sysfs-entries.txt
+++ b/Documentation/sysfs-entries.txt
@@ -11,8 +11,8 @@ can be used for monitoring the state of the hypervisor and its cells.
|- remap_pool_size - number of pages in hypervisor remapping pool
|- remap_pool_used - used pages of hypervisor remapping pool
`- cells
- |- <name of cell>
- | |- id - unique numerical ID
+ |- <id> - unique numerical ID
+ | |- name - cell name
| |- state - "running", "running/locked", "shut down", or
| | "failed"
| |- cpus_assigned - bitmask of assigned logical CPUs

diff --git a/driver/sysfs.c b/driver/sysfs.c
index a18c2b9..8c2a11f 100644
--- a/driver/sysfs.c
+++ b/driver/sysfs.c
@@ -139,12 +139,12 @@ static struct attribute_group stats_attr_group = {
.name = "statistics"
};

-static ssize_t id_show(struct kobject *kobj, struct kobj_attribute *attr,
- char *buffer)

+static ssize_t name_show(struct kobject *kobj, struct kobj_attribute *attr,
+ char *buffer)
{

struct cell *cell = container_of(kobj, struct cell, kobj);

- return sprintf(buffer, "%u\n", cell->id);
+ return sprintf(buffer, "%s\n", cell->name);
}

static ssize_t state_show(struct kobject *kobj, struct kobj_attribute *attr,
@@ -212,14 +212,14 @@ static ssize_t cpus_failed_show(struct kobject *kobj,
return written;
}

-static struct kobj_attribute cell_id_attr = __ATTR_RO(id);

+static struct kobj_attribute cell_name_attr = __ATTR_RO(name);

static struct kobj_attribute cell_state_attr = __ATTR_RO(state);
static struct kobj_attribute cell_cpus_assigned_attr =
__ATTR_RO(cpus_assigned);

static struct kobj_attribute cell_cpus_failed_attr = __ATTR_RO(cpus_failed);

static struct attribute *cell_attrs[] = {

- &cell_id_attr.attr,
+ &cell_name_attr.attr,
&cell_state_attr.attr,
&cell_cpus_assigned_attr.attr,
&cell_cpus_failed_attr.attr,

@@ -236,8 +236,8 @@ int jailhouse_sysfs_cell_create(struct cell *cell)
{
int err;

- err = kobject_init_and_add(&cell->kobj, &cell_type, cells_dir, "%s",
- cell->name);
+ err = kobject_init_and_add(&cell->kobj, &cell_type, cells_dir, "%d",
+ cell->id);

if (err) {
jailhouse_cell_kobj_release(&cell->kobj);
return err;

diff --git a/tools/jailhouse-cell-list b/tools/jailhouse-cell-list
index 3b5bc20..d4123f6 100755
--- a/tools/jailhouse-cell-list
+++ b/tools/jailhouse-cell-list

@@ -54,8 +54,8 @@ if len(sys.argv) > 1:
cells = []

for cell_path in glob.glob('/sys/devices/jailhouse/cells/*'):

cells.append({
- 'name': os.path.basename(cell_path),
- 'id': open(cell_path + "/id").readline().strip(),
+ 'id': os.path.basename(cell_path),

+ 'name': open(cell_path + "/name").readline().strip(),

'state': open(cell_path + "/state").readline().strip(),

'cpus_assigned': read_cpus(cell_path + "/cpus_assigned"),

'cpus_failed': read_cpus(cell_path + "/cpus_failed")