On Mon, May 02, 2022 at 04:09:19PM -0400, Dave Voutila wrote:
>
> Dave Voutila <d...@sisu.io> 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
>
Thanks. ok mlarkin@
-ml
>
> 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
>
