Dave Voutila <[email protected]> writes:
> tech@,
>
> tl;dr: standardize vmd/vmm/vmctl on counting memory in bytes at all
> times instead of a mix of MiB and bytes.
>
> There's some design friction between vmd(8)/vmctl(8) and vmm(4).
>
> For instance, the user-facing code deals in MiB, but internally a vm's
> memory ranges are defined in terms of bytes...but only after being
> converted at vm launch.
>
> Consequently, at different points in vmd's lifecycle, the same struct
> member for storing a vm's requested memory size contains a value in
> bytes OR in MiB meaning any code accessing the value needs to be
> contextually aware of if/when the value must be scaled.
>
> Given we dropped vmm(4) on i386 awhile ago, let's make use of 64-bit
> values! Plus this helps my other queued up changes simpler as they can
> avoid confusing scaling at points.
>
> There *is* some existing code duplication between vmd/vmctl related to
> parsing user provided memory values via scan_scaled(3), but I'm not
> looking to consolidate that now.
>
> If you're going to test, you'll need to build the kernel and either copy
> or link the patched vmmvar.h into /usr/include/machine/ before building
> vmd(8)/vmctl(8). (Don't forget to actually boot the kernel.)
>
> Otherwise, looking for ok's so I can continue squashing a few bugs in
> vmd that will be easier/cleaner to fix once this goes in.
>
> While the diff looks long-ish, it shouldn't require deep vmm/vmd
> knowledge to help review ;)
>
Updated with a fix (printing wrong limit value) and a tweak (checking a
size_t == 0 vs < 1). No functional changes so if by chance you already
applied the previous, please feel free to continue to test.
-dv
diff refs/heads/master refs/heads/vmd-bytes
blob - 765fc19bca559dbfd83cd14c48dee94f86c4b3cc
blob + 699798c1bbffafe7074fea43755ef7e20f073a90
--- sys/arch/amd64/amd64/vmm.c
+++ sys/arch/amd64/amd64/vmm.c
@@ -1575,7 +1575,7 @@ vm_create_check_mem_ranges(struct vm_create_params *vc
{
size_t i, memsize = 0;
struct vm_mem_range *vmr, *pvmr;
- const paddr_t maxgpa = (uint64_t)VMM_MAX_VM_MEM_SIZE * 1024 * 1024;
+ const paddr_t maxgpa = VMM_MAX_VM_MEM_SIZE;
if (vcp->vcp_nmemranges == 0 ||
vcp->vcp_nmemranges > VMM_MAX_MEM_RANGES)
blob - 94bb172832d4c2847b1e83ebb9cc05538db6ac80
blob + 012a023943b9fbc70339166889070ff0b4619046
--- sys/arch/amd64/include/vmmvar.h
+++ sys/arch/amd64/include/vmmvar.h
@@ -31,7 +31,7 @@
#define VMM_MAX_KERNEL_PATH 128
#define VMM_MAX_VCPUS 512
#define VMM_MAX_VCPUS_PER_VM 64
-#define VMM_MAX_VM_MEM_SIZE 32768
+#define VMM_MAX_VM_MEM_SIZE 32L * 1024 * 1024 * 1024 /* 32 GiB */
#define VMM_MAX_NICS_PER_VM 4
#define VMM_PCI_MMIO_BAR_BASE 0xF0000000ULL
blob - 0f7e4329a00d54a64fe41e1fb2bd2afcbaa9d68a
blob + c54aebcb982fdc14cc7a02910301d561e6623e4d
--- usr.sbin/vmctl/main.c
+++ usr.sbin/vmctl/main.c
@@ -404,24 +404,39 @@ parse_network(struct parse_result *res, char *word)
int
parse_size(struct parse_result *res, char *word)
{
- long long val = 0;
+ char result[FMT_SCALED_STRSIZE];
+ long long val = 0;
if (word != NULL) {
if (scan_scaled(word, &val) != 0) {
- warn("invalid size: %s", word);
+ warn("invalid memory size: %s", word);
return (-1);
}
}
if (val < (1024 * 1024)) {
- warnx("size must be at least one megabyte");
+ warnx("memory size must be at least 1M");
return (-1);
- } else
- res->size = val / 1024 / 1024;
+ }
- if ((res->size * 1024 * 1024) != val)
- warnx("size rounded to %lld megabytes", res->size);
+ if (val > VMM_MAX_VM_MEM_SIZE) {
+ if (fmt_scaled(VMM_MAX_VM_MEM_SIZE, result) == 0)
+ warnx("memory size too large (limit is %s)", result);
+ else
+ warnx("memory size too large");
+ return (-1);
+ }
+ /* Round down to the megabyte. */
+ res->size = (val / (1024 * 1024)) * (1024 * 1024);
+
+ if (res->size != (size_t)val) {
+ if (fmt_scaled(res->size, result) == 0)
+ warnx("memory size rounded to %s", result);
+ else
+ warnx("memory size rounded to %zu bytes", res->size);
+ }
+
return (0);
}
blob - 4c0b62fc6e16adbeb5cf951dcafbaebdbc356da8
blob + 15e6dd89ec15fa2501dcf6539c9ae9d90879ba56
--- usr.sbin/vmctl/vmctl.c
+++ usr.sbin/vmctl/vmctl.c
@@ -73,7 +73,7 @@ struct imsgbuf *ibuf;
* ENOMEM if a memory allocation failure occurred.
*/
int
-vm_start(uint32_t start_id, const char *name, int memsize, int nnics,
+vm_start(uint32_t start_id, const char *name, size_t memsize, int nnics,
char **nics, int ndisks, char **disks, int *disktypes, char *kernel,
char *iso, char *instance, unsigned int bootdevice)
{
@@ -122,7 +122,7 @@ vm_start(uint32_t start_id, const char *name, int mems
/*
* XXX: vmd(8) fills in the actual memory ranges. vmctl(8)
- * just passes in the actual memory size in MB here.
+ * just passes in the actual memory size here.
*/
vcp->vcp_nmemranges = 1;
vcp->vcp_memranges[0].vmr_size = memsize;
blob - 4fd2b787debb3a0e6bea0eced9508fcce3a09991
blob + 7d44c88f8fac106508807edcad393334d806edf2
--- usr.sbin/vmctl/vmctl.h
+++ usr.sbin/vmctl/vmctl.h
@@ -48,7 +48,7 @@ struct parse_result {
char *name;
char *path;
char *isopath;
- long long size;
+ size_t size;
int nifs;
char **nets;
int nnets;
@@ -93,7 +93,7 @@ int open_imagefile(int, const char *, int,
int create_imagefile(int, const char *, const char *, long, const char **);
int create_raw_imagefile(const char *, long);
int create_qc2_imagefile(const char *, const char *, long);
-int vm_start(uint32_t, const char *, int, int, char **, int,
+int vm_start(uint32_t, const char *, size_t, int, char **, int,
char **, int *, char *, char *, char *, unsigned int);
int vm_start_complete(struct imsg *, int *, int);
void terminate_vm(uint32_t, const char *, unsigned int);
blob - ebebbf24750b075414dc872d1290a0bf9c20d52b
blob + d37e53e6e7eeeb1f5b907ebb10d6a3793bd21ffd
--- usr.sbin/vmd/parse.y
+++ usr.sbin/vmd/parse.y
@@ -1248,26 +1248,42 @@ symget(const char *nam)
ssize_t
parse_size(char *word, int64_t val)
{
+ char result[FMT_SCALED_STRSIZE];
ssize_t size;
long long res;
if (word != NULL) {
if (scan_scaled(word, &res) != 0) {
- log_warn("invalid size: %s", word);
+ log_warn("invalid memory size: %s", word);
return (-1);
}
val = (int64_t)res;
}
if (val < (1024 * 1024)) {
- log_warnx("size must be at least one megabyte");
+ log_warnx("memory size must be at least 1M");
return (-1);
- } else
- size = val / 1024 / 1024;
+ }
- if ((size * 1024 * 1024) != val)
- log_warnx("size rounded to %zd megabytes", size);
+ if (val > VMM_MAX_VM_MEM_SIZE) {
+ if (fmt_scaled(VMM_MAX_VM_MEM_SIZE, result) == 0)
+ log_warnx("memory size too large (limit is %s)",
+ result);
+ else
+ log_warnx("memory size too large");
+ return (-1);
+ }
+ /* Round down to the megabyte. */
+ size = (val / (1024 * 1024)) * (1024 * 1024);
+
+ if (size != val) {
+ if (fmt_scaled(size, result) == 0)
+ log_warnx("memory size rounded to %s", result);
+ else
+ log_warnx("memory size rounded to %zd bytes", size);
+ }
+
return ((ssize_t)size);
}
blob - 55d938ed1d118f204dc19492148fc171080c6961
blob + fd63bf9ae5a84aee992473714efd939f37612094
--- usr.sbin/vmd/vm.c
+++ usr.sbin/vmd/vm.c
@@ -871,15 +871,13 @@ vcpu_reset(uint32_t vmid, uint32_t vcpu_id, struct vcp
void
create_memory_map(struct vm_create_params *vcp)
{
- size_t len, mem_bytes, mem_mb;
+ size_t len, mem_bytes;
- mem_mb = vcp->vcp_memranges[0].vmr_size;
+ mem_bytes = vcp->vcp_memranges[0].vmr_size;
vcp->vcp_nmemranges = 0;
- if (mem_mb < 1 || mem_mb > VMM_MAX_VM_MEM_SIZE)
+ if (mem_bytes == 0 || mem_bytes > VMM_MAX_VM_MEM_SIZE)
return;
- mem_bytes = mem_mb * 1024 * 1024;
-
/* First memory region: 0 - LOWMEM_KB (DOS low mem) */
len = LOWMEM_KB * 1024;
vcp->vcp_memranges[0].vmr_gpa = 0x0;
blob - 5f33b64831792420fd9231f0b76b11f31ad6b31c
blob + 4d2a5a4aecf78957b3b035c1a5b5e9449f6fe769
--- usr.sbin/vmd/vmd.h
+++ usr.sbin/vmd/vmd.h
@@ -56,7 +56,7 @@
#define MAX_TAP 256
#define NR_BACKLOG 5
#define VMD_SWITCH_TYPE "bridge"
-#define VM_DEFAULT_MEMORY 512
+#define VM_DEFAULT_MEMORY 512 * 1024 * 1024 /* 512 MiB */
#define VMD_DEFAULT_STAGGERED_START_DELAY 30
