Differential D19495 Diff 69728 lib/libvmmapi/vmmapi.c

Changeset View

Standalone View

lib/libvmmapi/vmmapi.c

Show All 38 Lines
#include <sys/module.h>		#include <sys/module.h>
#include <sys/_iovec.h>		#include <sys/_iovec.h>
#include <sys/cpuset.h>		#include <sys/cpuset.h>

#include <x86/segments.h>		#include <x86/segments.h>
#include <machine/specialreg.h>		#include <machine/specialreg.h>

#include <errno.h>		#include <errno.h>
		#include <stdbool.h>
#include <stdio.h>		#include <stdio.h>
#include <stdlib.h>		#include <stdlib.h>
#include <assert.h>		#include <assert.h>
#include <string.h>		#include <string.h>
#include <fcntl.h>		#include <fcntl.h>
#include <unistd.h>		#include <unistd.h>

#include <libutil.h>		#include <libutil.h>

		#include <vm/vm.h>
#include <machine/vmm.h>		#include <machine/vmm.h>
#include <machine/vmm_dev.h>		#include <machine/vmm_dev.h>
		#include <machine/vmm_snapshot.h>

#include "vmmapi.h"		#include "vmmapi.h"

#define MB (1024 * 1024UL)		#define MB (1024 * 1024UL)
#define GB (1024 * 1024 * 1024UL)		#define GB (1024 * 1024 * 1024UL)

/*		/*
* Size of the guard region before and after the virtual address space		* Size of the guard region before and after the virtual address space
Show All 37 Lines
}		}

int		int
vm_create(const char *name)		vm_create(const char *name)
{		{
/* Try to load vmm(4) module before creating a guest. */		/* Try to load vmm(4) module before creating a guest. */
if (modfind("vmm") < 0)		if (modfind("vmm") < 0)
kldload("vmm");		kldload("vmm");
return (CREATE((char *)name));		return (CREATE((char *)name));
}		}

struct vmctx *		struct vmctx *
		dabUnsubmitted Done Inline Actions This code could be somewhat simplified to: asprintf(&vm_checkpoint_file, "/dev/vmm/%s_mem", name); assert(vm_checkpoint_file != NULL); dab: This code could be somewhat simplified to: ``` asprintf(&vm_checkpoint_file, "/dev/vmm/%s_mem"…
vm_open(const char *name)		vm_open(const char *name)
{		{
struct vmctx *vm;		struct vmctx *vm;

vm = malloc(sizeof(struct vmctx) + strlen(name) + 1);		vm = malloc(sizeof(struct vmctx) + strlen(name) + 1);
assert(vm != NULL);		assert(vm != NULL);

vm->fd = -1;		vm->fd = -1;
vm->memflags = 0;		vm->memflags = 0;
vm->lowmem_limit = 3 * GB;		vm->lowmem_limit = 3 * GB;
vm->name = (char *)(vm + 1);		vm->name = (char *)(vm + 1);
strcpy(vm->name, name);		strcpy(vm->name, name);

if ((vm->fd = vm_device_open(vm->name)) < 0)		if ((vm->fd = vm_device_open(vm->name)) < 0)
goto err;		goto err;

return (vm);		return (vm);
err:		err:
		dabUnsubmitted Done Inline Actions Is failure to open the checkpoint device not an overall failure of this function? In other words, should there be a `goto err` here? Also, I am not sure why failure to open the checkpoint device merits a `printf()` but failure to open the vm device does not. Should this just silently fail (`goto err`) as in the above case? dab: Is failure to open the checkpoint device not an overall failure of this function? In other…
		emasteUnsubmitted Done Inline Actions Agreed the `printf` is probably unnecessary, but if we want to keep it, should probably be `warnx` instead. emaste: Agreed the `printf` is probably unnecessary, but if we want to keep it, should probably be…
		darius.mihaim_gmail.comUnsubmitted Done Inline Actions Removed the print for now and replaced it with a `goto err` as suggested. May be useful to have the VM simply not support suspend/resume, instead of failing to create it when the device is not created. darius.mihaim_gmail.com: Removed the print for now and replaced it with a `goto err` as suggested. May be useful to have…
vm_destroy(vm);		vm_destroy(vm);
return (NULL);		return (NULL);
}		}

void		void
vm_destroy(struct vmctx *vm)		vm_destroy(struct vmctx *vm)
{		{
assert(vm != NULL);		assert(vm != NULL);
▲ Show 20 Lines • Show All 92 Lines • ▼ Show 20 Lines	if (error == 0 && gpa == memmap.gpa) {
}		}
}		}

error = ioctl(ctx->fd, VM_MMAP_MEMSEG, &memmap);		error = ioctl(ctx->fd, VM_MMAP_MEMSEG, &memmap);
return (error);		return (error);
}		}

int		int
		vm_get_guestmem_from_ctx(struct vmctx ctx, char *guest_baseaddr,
		size_t lowmem_size, size_t highmem_size)
		{

		*guest_baseaddr = ctx->baseaddr;
		*lowmem_size = ctx->lowmem;
		*highmem_size = ctx->highmem;
		dabUnsubmitted Not Done Inline Actions Why not just `return (ioctl(...))` and skip the `error` variable? dab: Why not just `return (ioctl(...))` and skip the `error` variable?
		darius.mihaim_gmail.comUnsubmitted Not Done Inline Actions I'm not particularly fond of `return (func(params));`. Would it be more inline with the coding style rewriting it like this? darius.mihaim_gmail.com: I'm not particularly fond of `return (func(params));`. Would it be more inline with the coding…
		dabUnsubmitted Done Inline Actions I saw other places in this code where `return (ioctl(...))` was done, so it wouldn't be out of character with the rest of the code. That being said, I don't think style(9) makes any mention of this one way or the other. I like the simplified code, but it is totally up to you. dab: I saw other places in this code where `return (ioctl(...))` was done, so it wouldn't be out of…
		emasteUnsubmitted Not Done Inline Actions For something very simple like this I would go with `return(function(...))`, but the current one is fine. However, style(9) would omit the extra blank line before the `return`. emaste: For something very simple like this I would go with `return(function(...))`, but the current…
		cemUnsubmitted Not Done Inline Actions style(9) mentions this explicitly: "Values in return statements should be enclosed in parentheses" (as well as by example). cem: style(9) mentions this explicitly: "Values in return statements should be enclosed in…
		emasteUnsubmitted Not Done Inline Actions It's not a question of parens vs not, it's err = foo() return (err); vs return(foo()); emaste: It's not a question of parens vs not, it's ``` err = foo() return (err); ``` vs ``` return(foo…
		darius.mihaim_gmail.comUnsubmitted Done Inline Actions Went with `return (ioctl(...));` in the end, since the function is simple enough. darius.mihaim_gmail.com: Went with `return (ioctl(...));` in the end, since the function is simple enough.
		return (0);
		}

		int
vm_mmap_getnext(struct vmctx ctx, vm_paddr_t gpa, int *segid,		vm_mmap_getnext(struct vmctx ctx, vm_paddr_t gpa, int *segid,
vm_ooffset_t segoff, size_t len, int prot, int flags)		vm_ooffset_t segoff, size_t len, int prot, int flags)
{		{
struct vm_memmap memmap;		struct vm_memmap memmap;
int error;		int error;

bzero(&memmap, sizeof(struct vm_memmap));		bzero(&memmap, sizeof(struct vm_memmap));
memmap.gpa = *gpa;		memmap.gpa = *gpa;
error = ioctl(ctx->fd, VM_MMAP_GETNEXT, &memmap);		error = ioctl(ctx->fd, VM_MMAP_GETNEXT, &memmap);
if (error == 0) {		if (error == 0) {
*gpa = memmap.gpa;		*gpa = memmap.gpa;
*segid = memmap.segid;		*segid = memmap.segid;
*segoff = memmap.segoff;		*segoff = memmap.segoff;
*len = memmap.len;		*len = memmap.len;
*prot = memmap.prot;		*prot = memmap.prot;
*flags = memmap.flags;		*flags = memmap.flags;
}		}
		emasteUnsubmitted Done Inline Actions style(9) wants /* * Multiline comments * to look like this. / emaste:* style(9) wants ``` /* * Multiline comments * to look like this. */ ```
return (error);		return (error);
}		}

/*		/*
* Return 0 if the segments are identical and non-zero otherwise.		* Return 0 if the segments are identical and non-zero otherwise.
*		*
* This is slightly complicated by the fact that only device memory segments		* This is slightly complicated by the fact that only device memory segments
* are named.		* are named.
*/		*/
static int		static int
cmpseg(size_t len, const char str, size_t len2, const char str2)		cmpseg(size_t len, const char str, size_t len2, const char str2)
		dabUnsubmitted Not Done Inline Actions Why `#if 1`? dab: Why `#if 1`?
		darius.mihaim_gmail.comUnsubmitted Done Inline Actions Was forgotten there as a sanity check. I have removed it. darius.mihaim_gmail.com: Was forgotten there as a sanity check. I have removed it.
{		{

if (len == len2) {		if (len == len2) {
if ((!str && !str2) \|\| (str && str2 && !strcmp(str, str2)))		if ((!str && !str2) \|\| (str && str2 && !strcmp(str, str2)))
return (0);		return (0);
}		}
return (-1);		return (-1);
}		}
▲ Show 20 Lines • Show All 166 Lines • ▼ Show 20 Lines	if (ctx->highmem > 0) {
gaddr + len <= 4*GB + ctx->highmem)		gaddr + len <= 4*GB + ctx->highmem)
return (ctx->baseaddr + gaddr);		return (ctx->baseaddr + gaddr);
}		}
}		}

return (NULL);		return (NULL);
}		}

		vm_paddr_t
		vm_rev_map_gpa(struct vmctx ctx, void addr)
		{
		vm_paddr_t offaddr;

		offaddr = (char *)addr - ctx->baseaddr;
		jhbAuthorUnsubmitted Not Done Inline Actions This probably triggers warnings. Pointer math on void * isn't defined in standard C. GNU C allows it as an extension. Char * would be better. It seems like 'offaddr' should also be vm_paddr_t, not off_t. FreeBSD's style also doesn't do spaces after casts, but that's minor. jhb: This probably triggers warnings. Pointer math on void * isn't defined in standard C. GNU C…
		darius.mihaim_gmail.comUnsubmitted Done Inline Actions No warnings were generated at compile time, but using char * makes more sense since baseaddr is also char * darius.mihaim_gmail.com: No warnings were generated at compile time, but using char * makes more sense since baseaddr is…

		if (ctx->lowmem > 0)
		if (offaddr >= 0 && offaddr <= ctx->lowmem)
		return (offaddr);

		if (ctx->highmem > 0)
		if (offaddr >= 4GB && offaddr < 4GB + ctx->highmem)
		return (offaddr);

		return ((vm_paddr_t)-1);
		}

		/* TODO: maximum size for vmname */
		int
		vm_get_name(struct vmctx ctx, char buf, size_t max_len)
		{
		dabUnsubmitted Done Inline Actions It would seem useful to return an error if `max_len` is too small to hold the name. Also, I would use `strlcpy()` in preference to `snprintf()`. Lastly, I would declare `max_len` to be `size_t`. dab: It would seem useful to return an error if `max_len` is too small to hold the name. Also, I…

		if (strlcpy(buf, ctx->name, max_len) >= max_len)
		return (EINVAL);
		return (0);
		}

size_t		size_t
vm_get_lowmem_size(struct vmctx *ctx)		vm_get_lowmem_size(struct vmctx *ctx)
{		{

return (ctx->lowmem);		return (ctx->lowmem);
}		}

size_t		size_t
▲ Show 20 Lines • Show All 167 Lines • ▼ Show 20 Lines

int		int
vm_run(struct vmctx ctx, int vcpu, struct vm_exit vmexit)		vm_run(struct vmctx ctx, int vcpu, struct vm_exit vmexit)
{		{
int error;		int error;
struct vm_run vmrun;		struct vm_run vmrun;

bzero(&vmrun, sizeof(vmrun));		bzero(&vmrun, sizeof(vmrun));
vmrun.cpuid = vcpu;		vmrun.cpuid = vcpu;

error = ioctl(ctx->fd, VM_RUN, &vmrun);		error = ioctl(ctx->fd, VM_RUN, &vmrun);
bcopy(&vmrun.vm_exit, vmexit, sizeof(struct vm_exit));		bcopy(&vmrun.vm_exit, vmexit, sizeof(struct vm_exit));
return (error);		return (error);
}		}

int		int
		pmooney_pfmooney.comUnsubmitted Not Done Inline Actions Have you quantified the overhead added by these mutex/condvar operations for exit-heavy workloads? VM exits which go all the way to userspace are relatively expensive, so the relative delta might be small, but it would be useful to know. It might make sense to move this logic from libvmmapi into the bhyve process itself, where all of the other userspace vCPU state tracking is going on. If contention turned out to be high when lots of userspace exits are occurring (since there's only the one lock today) it would be possible, armed with knowledge of the number of active vCPU threads, to do something like a per-vCPU lock and pause state. Going that far might only make sense if the vCPU threads happen to contend in such a way. pmooney_pfmooney.com: Have you quantified the overhead added by these mutex/condvar operations for exit-heavy…
		darius.mihaim_gmail.comUnsubmitted Done Inline Actions The vCPU pause mechanism has been changed thanks to @jhb. It is now relying on the debug functionality and should no longer cause heavy contention on mutexes. darius.mihaim_gmail.com: The vCPU pause mechanism has been changed thanks to @jhb. It is now relying on the debug…
vm_suspend(struct vmctx *ctx, enum vm_suspend_how how)		vm_suspend(struct vmctx *ctx, enum vm_suspend_how how)
{		{
struct vm_suspend vmsuspend;		struct vm_suspend vmsuspend;

bzero(&vmsuspend, sizeof(vmsuspend));		bzero(&vmsuspend, sizeof(vmsuspend));
vmsuspend.how = how;		vmsuspend.how = how;
return (ioctl(ctx->fd, VM_SUSPEND, &vmsuspend));		return (ioctl(ctx->fd, VM_SUSPEND, &vmsuspend));
}		}
▲ Show 20 Lines • Show All 851 Lines • ▼ Show 20 Lines
}		}

int		int
vm_restart_instruction(void *arg, int vcpu)		vm_restart_instruction(void *arg, int vcpu)
{		{
struct vmctx *ctx = arg;		struct vmctx *ctx = arg;

return (ioctl(ctx->fd, VM_RESTART_INSTRUCTION, &vcpu));		return (ioctl(ctx->fd, VM_RESTART_INSTRUCTION, &vcpu));
		}

		int
		vm_snapshot_req(struct vm_snapshot_meta *meta)
		{

		if (ioctl(meta->ctx->fd, VM_SNAPSHOT_REQ, meta) == -1) {
		#ifdef SNAPSHOT_DEBUG
		fprintf(stderr, "%s: snapshot failed for %s: %d\r\n",
		__func__, meta->dev_name, errno);
		#endif
		return (-1);
		}
		return (0);
		}

		int
		vm_restore_time(struct vmctx *ctx)
		{
		int dummy;

		dummy = 0;
		return (ioctl(ctx->fd, VM_RESTORE_TIME, &dummy));
}		}

int		int
vm_set_topology(struct vmctx *ctx,		vm_set_topology(struct vmctx *ctx,
uint16_t sockets, uint16_t cores, uint16_t threads, uint16_t maxcpus)		uint16_t sockets, uint16_t cores, uint16_t threads, uint16_t maxcpus)
{		{
struct vm_cpu_topology topology;		struct vm_cpu_topology topology;

bzero(&topology, sizeof (struct vm_cpu_topology));		bzero(&topology, sizeof (struct vm_cpu_topology));
topology.sockets = sockets;		topology.sockets = sockets;
topology.cores = cores;		topology.cores = cores;
topology.threads = threads;		topology.threads = threads;
topology.maxcpus = maxcpus;		topology.maxcpus = maxcpus;
return (ioctl(ctx->fd, VM_SET_TOPOLOGY, &topology));		return (ioctl(ctx->fd, VM_SET_TOPOLOGY, &topology));
}		}

int		int
vm_get_topology(struct vmctx *ctx,		vm_get_topology(struct vmctx *ctx,
uint16_t sockets, uint16_t cores, uint16_t threads, uint16_t maxcpus)		uint16_t sockets, uint16_t cores, uint16_t threads, uint16_t maxcpus)
{		{
struct vm_cpu_topology topology;		struct vm_cpu_topology topology;
		pmooney_pfmooney.comUnsubmitted Not Done Inline Actions Error logging to stderr is left up to the library consumer (read: the bhyve binary) today. It probably makes sense to continue that. Here and the two functions below could just be sure to leave the error state (`meta->dev_name` or `errno`) in place to be logged by the consumer, should they so choose. pmooney_pfmooney.com: Error logging to stderr is left up to the library consumer (read: the bhyve binary) today. It…
		darius.mihaim_gmail.comUnsubmitted Not Done Inline Actions Will update this at a later time. The messages from the entire lib will have to be moved where functions are used. darius.mihaim_gmail.com: Will update this at a later time. The messages from the entire lib will have to be moved where…
int error;		int error;

bzero(&topology, sizeof (struct vm_cpu_topology));		bzero(&topology, sizeof (struct vm_cpu_topology));
error = ioctl(ctx->fd, VM_GET_TOPOLOGY, &topology);		error = ioctl(ctx->fd, VM_GET_TOPOLOGY, &topology);
if (error == 0) {		if (error == 0) {
*sockets = topology.sockets;		*sockets = topology.sockets;
*cores = topology.cores;		*cores = topology.cores;
*threads = topology.threads;		*threads = topology.threads;
*maxcpus = topology.maxcpus;		*maxcpus = topology.maxcpus;
		emasteUnsubmitted Done Inline Actions style discourages `//` comments emaste: style discourages `//` comments
}		}
return (error);		return (error);
}		}

int		int
vm_get_device_fd(struct vmctx *ctx)		vm_get_device_fd(struct vmctx *ctx)
{		{

Show All 39 Lines