On Thu, Sep 17, 2020 at 04:23:52PM +0100, Daniel P. Berrangé wrote: > From: Prakhar Bansal <prakharbansal0910@xxxxxxxxx> > > --- > 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 :|