On Thu, Sep 17, 2020 at 04:23:52PM +0100, Daniel P. Berrangé wrote:
From: Prakhar Bansal <prakharbansal0910@gmail.com>
--- include/libvirt/virterror.h | 1 + po/POTFILES.in | 2 + src/jailhouse/Makefile.inc.am | 34 ++- src/jailhouse/jailhouse.conf | 10 + src/jailhouse/jailhouse_api.c | 372 ++++++++++++++++++++++++++++ src/jailhouse/jailhouse_api.h | 74 ++++++ src/jailhouse/jailhouse_driver.c | 302 +++++++++++++++++----- src/jailhouse/jailhouse_driver.h | 51 ++++ src/jailhouse/meson.build | 1 + src/libvirt.c | 10 - src/remote/remote_daemon.c | 4 + src/remote/remote_daemon_dispatch.c | 3 +- 12 files changed, 783 insertions(+), 81 deletions(-) create mode 100644 src/jailhouse/jailhouse.conf create mode 100644 src/jailhouse/jailhouse_api.c create mode 100644 src/jailhouse/jailhouse_api.h
diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index 97f2ac16d8..9f1bca2684 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -137,6 +137,7 @@ typedef enum { VIR_FROM_TPM = 70, /* Error from TPM */ VIR_FROM_BPF = 71, /* Error from BPF code */ VIR_FROM_JAILHOUSE = 72, /* Error from Jailhouse driver */ + # ifdef VIR_ENUM_SENTINELS VIR_ERR_DOMAIN_LAST # endif
Should be in the previous patch.
diff --git a/src/jailhouse/Makefile.inc.am b/src/jailhouse/Makefile.inc.am index 02822b2ea1..324c3b1b16 100644 --- a/src/jailhouse/Makefile.inc.am +++ b/src/jailhouse/Makefile.inc.am
This file shouldn't exist any more.
diff --git a/src/jailhouse/jailhouse_api.c b/src/jailhouse/jailhouse_api.c new file mode 100644 index 0000000000..cda00b50e7 --- /dev/null +++ b/src/jailhouse/jailhouse_api.c @@ -0,0 +1,372 @@ +/* + * jailhouse_api.c: Jailhouse API + * + * Copyright (C) 2020 Prakhar Bansal + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + */ + +#include <config.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <dirent.h> +#include <fcntl.h> +#include <unistd.h> +#include <errno.h> +#include <sys/types.h> +#include <sys/ioctl.h> +#include <sys/stat.h> +#include <linux/types.h> + +#include "viralloc.h" +#include "virerror.h" +#include "virfile.h" +#include "virlog.h" +#include "virstring.h" +#include "jailhouse_api.h" + +#define JAILHOUSE_DEVICE "/dev/jailhouse" +#define JAILHOUSE_CELLS "/sys/devices/jailhouse/cells" +#define MAX_JAILHOUSE_SYS_CONFIG_FILE_SIZE 1024*1024 +#define MAX_JAILHOUSE_CELL_CONFIG_FILE_SIZE 1024 + + +#define JAILHOUSE_ENABLE _IOW(0, 0, void *) +#define JAILHOUSE_DISABLE _IO(0, 1) +#define JAILHOUSE_CELL_CREATE _IOW(0, 2, virJailhouseCellCreate) +#define JAILHOUSE_CELL_DESTROY _IOW(0, 5, virJailhouseCellId) + +#define VIR_FROM_THIS VIR_FROM_JAILHOUSE + +VIR_LOG_INIT("jailhouse.jailhouse_api"); + +#define JAILHOUSE_CELL_FILE_EXTENSION ".cell" + +/* Forward declarations */ + +/* Open the Jailhouse device for ioctl APIs */ +int openDev(void); + +/* Reads cell's property given by 'entry' using sysfs API */ +char *readSysfsCellString(const unsigned int id, const char *entry); + +int cell_match(const struct dirent *dirent); + +int createCell(const char *conf_file); + +int destroyCell(virJailhouseCellId cell_id); + +int getCellInfo(const unsigned int id, + virJailhouseCellInfoPtr * cell_info);
We expect that all methods have a consistent name prefix. So everything in this driver should really be named with "virJailhouse" as an API prefix. This naming rule includes static methods
+ +int +jailhouseEnable(const char *sys_conf_file_path) +{ + int err = -1, len; + g_autofree char *buffer = NULL; + VIR_AUTOCLOSE fd = -1; + + if (!virFileExists(sys_conf_file_path)) + return 0; + + len = virFileReadAll(sys_conf_file_path, MAX_JAILHOUSE_SYS_CONFIG_FILE_SIZE, &buffer); + if (len < 0 || !buffer) {
buffer will be non-NULL if len >= 0, so the second test isn't needed.
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("Failed to read the system configuration file")); + return -1; + } + + fd = openDev();
You need to check for error opening files...
+ + err = ioctl(fd, JAILHOUSE_ENABLE, buffer);
or this uses a closed FD.
+ if (err) { + virReportSystemError(errno, "%s", _("Failed to enable jailhouse")); + return err; + } + + VIR_DEBUG("Jailhouse hypervisor is enabled"); + + return 1; +} + +int +jailhouseDisable(void) +{ + int err = -1; + VIR_AUTOCLOSE fd = -1; + + fd = openDev();
Again check for error
+ + err = ioctl(fd, JAILHOUSE_DISABLE); + if (err) + virReportSystemError(errno, + "%s", + _("Failed to disable jailhouse: %s")); + + VIR_DEBUG("Jailhouse hypervisor is disabled"); + + return err; +} + +int +cell_match(const struct dirent *dirent) +{ + char *ext = strrchr(dirent->d_name, '.'); + + return dirent->d_name[0] != '.' + && (STREQ(ext, JAILHOUSE_CELL_FILE_EXTENSION) == 0);
You need to check for "ext" being NULL.
+} + +int +createJailhouseCells(const char *dir_path) +{ + + struct dirent **namelist; + int num_entries, ret = -1; + size_t i; + + if (strlen(dir_path) == 0) + return ret; + + num_entries = scandir(dir_path, &namelist, cell_match, alphasort); + if (num_entries == -1) { + if (errno == ENOENT) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("No cells found in %s, scandir failed."), + dir_path); + goto fail; + } + + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Error reading cell configurations in %s."), + dir_path); + goto fail; + } + + + for (i = 0; i < num_entries; i++) { + g_autofree char *file_path = g_strdup_printf("%s/%s", dir_path, namelist[i]->d_name); + + if (createCell(file_path) != 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Cell creation failed with conf found in %s."), + namelist[i]->d_name); + goto fail; + } + } + + ret = 0; + + fail:
Our naming convention is "cleanup" for paths that are shared with successful exit, or "error" for exclusively failure paths. So this one should be "cleanup"
+ VIR_FREE(namelist); + return ret; +}
+int +createCell(const char *conf_file) +{ + virJailhouseCellCreate cell_create; + int err = -1, len; + g_autofree char *buffer = NULL; + VIR_AUTOCLOSE fd = -1; + + if (strlen(conf_file) == 0) + return err; + + len = virFileReadAll(conf_file, MAX_JAILHOUSE_CELL_CONFIG_FILE_SIZE, &buffer); + if (len < 0 || !buffer) {
No need for !buffer
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("Failed to read the system configuration file")); + return err; + } + + cell_create.config_address = (unsigned long) buffer; + cell_create.config_size = len; + + fd = openDev();
Check error.
+ + err = ioctl(fd, JAILHOUSE_CELL_CREATE, &cell_create); + if (err) + virReportSystemError(errno, + "%s", + _("Cell creation failed: %s")); + + return err; +}
+char * +readSysfsCellString(const unsigned int id, const char *entry) +{ + g_autofree char *buffer = NULL; + g_autofree char *file_path = NULL; + int len = -1; + + file_path = g_strdup_printf(JAILHOUSE_CELLS "%u/%s", id, entry); + + len = virFileReadAll(file_path, 1024, &buffer); + if (len < 0 || !buffer) {
No need for !buffer
+ virReportError(VIR_ERR_INTERNAL_ERROR, + _("Error reading cell(%u) %s from %s failed"), + id, entry, file_path); + return NULL; + } + + virTrimSpaces(buffer, NULL); + + return buffer; +}
+ + /* Allocate memory for 1 more than num_entries and make the last entry NULL. */ + if (VIR_ALLOC_N(cell_info_list, num_entries + 1) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", + _("Insufficient memory for cells info list")); + }
Don't need to check for failure as it aborts().
+ + /* Set the last entry to NULL. */ + cell_info_list[num_entries] = NULL; + + for (i = 0; i < num_entries; i++) { + if (virStrToLong_ui(namelist[i]->d_name, NULL, 10, &id) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cell ID %s could not be converted to a long"), + namelist[i]->d_name); + continue; + } + + /* get the cell's information(name, state etc.) using sysfs */ + getCellInfo(id, &cell_info_list[i]); + VIR_FREE(namelist[i]); + } + + VIR_FREE(namelist); + return cell_info_list; +} + +int +destroyCell(virJailhouseCellId cell_id) +{ + int err = -1; + VIR_AUTOCLOSE fd = -1; + + fd = openDev();
Check for error
+ + err = ioctl(fd, JAILHOUSE_CELL_DESTROY, &cell_id); + if (err) + virReportSystemError(errno, + _("Destroying cell %d failed"), + cell_id.id); + + return err; +}
diff --git a/src/jailhouse/jailhouse_api.h b/src/jailhouse/jailhouse_api.h new file mode 100644 index 0000000000..8362cb3d0f --- /dev/null +++ b/src/jailhouse/jailhouse_api.h @@ -0,0 +1,74 @@ +/* + * jailhouse_api.h: Jailhouse hypervisor API implementation + * + * Copyright (C) 2020 Prakhar Bansal + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#pragma once + +#define JAILHOUSE_CELL_ID_NAMELEN 31 + +typedef struct _virJailhouseCellId virJailhouseCellId; + +struct _virJailhouseCellId { + __s32 id;
Shouldn't be using these types, instead int32_t
+ __u32 padding;
uint32_t
+ char name[JAILHOUSE_CELL_ID_NAMELEN + 1]; +}; + +typedef struct _virJailhouseCellInfo virJailhouseCellInfo; +typedef virJailhouseCellInfo *virJailhouseCellInfoPtr; + +struct _virJailhouseCellInfo { + struct _virJailhouseCellId id; + char *state; + char *cpus_assigned_list; + char *cpus_failed_list; +}; + +typedef struct _virJailhouseCellCreate virJailhouseCellCreate; + +struct _virJailhouseCellCreate { + __u64 config_address;
uint64_t ..etc
+ __u32 config_size; + __u32 padding; +}; + +// Enables the Jailhouse hypervisor by reading the hypervisor system +// configuration from the given file and calls the ioctl API to +// enable the hypervisor. +int jailhouseEnable(const char *sys_conf_file_path); + +// Disables the Jailhouse hypervisor. +int jailhouseDisable(void); + +/* Cell API methods */ + +// Creates Jailhouse cells using the cells configurations +// provided in the dir_name. +int createJailhouseCells(const char *dir_path); + +// Destroys Jailhouse cells using the cell IDs provided in +// the cell_info_list. +int destroyJailhouseCells(virJailhouseCellInfoPtr *cell_info_list); + +// Returns cell's information in a null-terminated array of +// virJailhouseCellInfoPtr for all the Jailhouse cells. +virJailhouseCellInfoPtr *getJailhouseCellsInfo(void); + +// Free the cell info object. +void cellInfoFree(virJailhouseCellInfoPtr cell_info);
@@ -69,36 +254,16 @@ jailhouseConnectGetHostname(virConnectPtr conn) }
static int -jailhouseNodeGetInfo(virConnectPtr conn, - virNodeInfoPtr info) +jailhouseNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info)
This formatting change should be done in the original patch. Same for the changes that follow
{ UNUSED(conn); UNUSED(info); return -1; }
-static int -jailhouseConnectListDomains(virConnectPtr conn, - int *ids, - int maxids) -{ - UNUSED(conn); - UNUSED(ids); - UNUSED(maxids); - return -1; -} - -static int -jailhouseConnectNumOfDomains(virConnectPtr conn) -{ - UNUSED(conn); - return -1; -} - static int jailhouseConnectListAllDomains(virConnectPtr conn, - virDomainPtr **domain, - unsigned int flags) + virDomainPtr ** domain, unsigned int flags) { UNUSED(conn); UNUSED(domain); @@ -107,8 +272,7 @@ jailhouseConnectListAllDomains(virConnectPtr conn, }
static virDomainPtr -jailhouseDomainLookupByID(virConnectPtr conn, - int id) +jailhouseDomainLookupByID(virConnectPtr conn, int id) { UNUSED(conn); UNUSED(id); @@ -116,8 +280,7 @@ jailhouseDomainLookupByID(virConnectPtr conn, }
static virDomainPtr -jailhouseDomainLookupByName(virConnectPtr conn, - const char *name) +jailhouseDomainLookupByName(virConnectPtr conn, const char *name) { UNUSED(conn); UNUSED(name); @@ -125,8 +288,7 @@ jailhouseDomainLookupByName(virConnectPtr conn, }
static virDomainPtr -jailhouseDomainLookupByUUID(virConnectPtr conn, - const unsigned char *uuid) +jailhouseDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) { UNUSED(conn); UNUSED(uuid); @@ -157,8 +319,7 @@ jailhouseDomainDestroy(virDomainPtr domain) }
static int -jailhouseDomainGetInfo(virDomainPtr domain, - virDomainInfoPtr info) +jailhouseDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info) { UNUSED(domain); UNUSED(info); @@ -167,9 +328,7 @@ jailhouseDomainGetInfo(virDomainPtr domain,
static int jailhouseDomainGetState(virDomainPtr domain, - int *state, - int *reason, - unsigned int flags) + int *state, int *reason, unsigned int flags) { UNUSED(domain); UNUSED(state); @@ -179,8 +338,7 @@ jailhouseDomainGetState(virDomainPtr domain, }
static char * -jailhouseDomainGetXMLDesc(virDomainPtr domain, - unsigned int flags) +jailhouseDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) { UNUSED(domain); UNUSED(flags); @@ -189,31 +347,43 @@ jailhouseDomainGetXMLDesc(virDomainPtr domain,
static virHypervisorDriver jailhouseHypervisorDriver = { .name = "JAILHOUSE", - .connectOpen = jailhouseConnectOpen, /* 6.3.0 */ - .connectClose = jailhouseConnectClose, /* 6.3.0 */ - .connectListDomains = jailhouseConnectListDomains, /* 6.3.0 */ - .connectNumOfDomains = jailhouseConnectNumOfDomains, /* 6.3.0 */ - .connectListAllDomains = jailhouseConnectListAllDomains, /* 6.3.0 */ - .domainLookupByID = jailhouseDomainLookupByID, /* 6.3.0 */ - .domainLookupByUUID = jailhouseDomainLookupByUUID, /* 6.3.0 */ - .domainLookupByName = jailhouseDomainLookupByName, /* 6.3.0 */ - .domainGetXMLDesc = jailhouseDomainGetXMLDesc, /* 6.3.0 */ - .domainCreate = jailhouseDomainCreate, /* 6.3.0 */ - .connectGetType = jailhouseConnectGetType, /* 6.3.0 */ - .connectGetHostname = jailhouseConnectGetHostname, /* 6.3.0 */ - .nodeGetInfo = jailhouseNodeGetInfo, /* 6.3.0 */ - .domainShutdown = jailhouseDomainShutdown, /* 6.3.0 */ - .domainDestroy = jailhouseDomainDestroy, /* 6.3.0 */ - .domainGetInfo = jailhouseDomainGetInfo, /* 6.3.0 */ - .domainGetState = jailhouseDomainGetState, /* 6.3.0 */ + .connectOpen = jailhouseConnectOpen, /* 6.3.0 */ + .connectClose = jailhouseConnectClose, /* 6.3.0 */ + .connectListAllDomains = jailhouseConnectListAllDomains, /* 6.3.0 */ + .domainLookupByID = jailhouseDomainLookupByID, /* 6.3.0 */ + .domainLookupByUUID = jailhouseDomainLookupByUUID, /* 6.3.0 */ + .domainLookupByName = jailhouseDomainLookupByName, /* 6.3.0 */ + .domainGetXMLDesc = jailhouseDomainGetXMLDesc, /* 6.3.0 */ + .domainCreate = jailhouseDomainCreate, /* 6.3.0 */ + .connectGetType = jailhouseConnectGetType, /* 6.3.0 */ + .connectGetHostname = jailhouseConnectGetHostname, /* 6.3.0 */ + .nodeGetInfo = jailhouseNodeGetInfo, /* 6.3.0 */ + .domainShutdown = jailhouseDomainShutdown, /* 6.3.0 */ + .domainDestroy = jailhouseDomainDestroy, /* 6.3.0 */ + .domainGetInfo = jailhouseDomainGetInfo, /* 6.3.0 */ + .domainGetState = jailhouseDomainGetState, /* 6.3.0 */
Don't do this formatting change - trying to horizontally align such comments is a waste of time in the long term.
diff --git a/src/jailhouse/jailhouse_driver.h b/src/jailhouse/jailhouse_driver.h index b0dbc8d033..8a0e111676 100644 --- a/src/jailhouse/jailhouse_driver.h +++ b/src/jailhouse/jailhouse_driver.h @@ -20,4 +20,55 @@
#pragma once
+#include <linux/types.h> + +#include "jailhouse_api.h" + int jailhouseRegister(void); + +#define JAILHOUSE_CONFIG_FILE SYSCONFDIR "/libvirt/jailhouse/jailhouse.conf" +#define JAILHOUSE_STATE_DIR RUNSTATEDIR "/libvirt/jailhouse" + +#define JAILHOUSE_DEV "/dev/jailhouse" + +#define JAILHOUSE_SYSFS_DEV "/sys/devices/jailhouse/" + +typedef struct _virJailhouseDriver virJailhouseDriver; +typedef virJailhouseDriver *virJailhouseDriverPtr; + +typedef struct _virJailhouseDriverConfig virJailhouseDriverConfig; +typedef virJailhouseDriverConfig *virJailhouseDriverConfigPtr; + +struct _virJailhouseDriverConfig { + virObject parent; + + char *stateDir; + + // File path of the jailhouse system configuration + // for jailhouse enable/disable. + char *sys_config_file_path; + + // Config directory where all jailhouse cell configurations + // are stored. + char *cell_config_dir; +}; + +struct _virJailhouseDriver { + virMutex lock; + + // Jailhouse configuration read from the jailhouse.conf + virJailhouseDriverConfigPtr config; + + /* pid file FD, ensures two copies of the driver can't use the same root */ + int lockFD; + + // All the cells created during connect open on the hypervisor. + virJailhouseCellInfoPtr *cell_info_list; +}; + +struct _jailhouseCell { + __s32 id;
int32_t
+ char *state; + char *cpus_assigned_list; + char *cpus_failed_list; +};
diff --git a/src/libvirt.c b/src/libvirt.c index 8a78cbcf3a..0748eb2352 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -75,9 +75,6 @@ #ifdef WITH_BHYVE # include "bhyve/bhyve_driver.h" #endif -#ifdef WITH_JAILHOUSE -# include "jailhouse/jailhouse_driver.h" -#endif #include "access/viraccessmanager.h"
#define VIR_FROM_THIS VIR_FROM_NONE @@ -274,10 +271,6 @@ virGlobalInit(void) if (hypervRegister() == -1) goto error; #endif -#ifdef WITH_JAILHOUSE - if (jailhouseRegister() == -1) - goto error; -#endif #ifdef WITH_REMOTE if (remoteRegister() == -1) goto error; @@ -1052,9 +1045,6 @@ virConnectOpenInternal(const char *name, #endif #ifndef WITH_VZ STRCASEEQ(ret->uri->scheme, "parallels") || -#endif -#ifndef WITH_JAILHOUSE - STRCASEEQ(ret->uri->scheme, "jailhouse") || #endif false)) { virReportErrorHelper(VIR_FROM_NONE, VIR_ERR_CONFIG_UNSUPPORTED,
You just added all these lines in the previous patch. If they're not relevant, then don't add them in the first place. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|