diff mbox series

[v2,kvmtool,02/12] builtin-run: Always use RAM size in bytes

Message ID 20220516155526.181694-3-alexandru.elisei@arm.com (mailing list archive)
State New, archived
Headers show
Series arm64: Allow the user to set RAM base address | expand

Commit Message

Alexandru Elisei May 16, 2022, 3:55 p.m. UTC 
The user can specify the virtual machine memory size in MB, which is saved
in cfg->ram_size. kvmtool validates it against the host memory size,
converted from bytes to MB. ram_size is then converted to bytes, and this
is how it is used throughout the rest of kvmtool.

To avoid any confusion about the unit of measurement, especially once the
user is allowed to specify the unit of measurement, always use ram_size in
bytes.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 builtin-run.c            | 19 ++++++++++---------
 include/kvm/kvm-config.h |  7 ++++---
 include/kvm/kvm.h        |  2 +-
 3 files changed, 15 insertions(+), 13 deletions(-)

Comments

Andre Przywara May 23, 2022, 5:18 p.m. UTC | #1 
On Mon, 16 May 2022 16:55:16 +0100
Alexandru Elisei <alexandru.elisei@arm.com> wrote:

> The user can specify the virtual machine memory size in MB, which is saved
> in cfg->ram_size. kvmtool validates it against the host memory size,
> converted from bytes to MB. ram_size is then converted to bytes, and this
> is how it is used throughout the rest of kvmtool.
> 
> To avoid any confusion about the unit of measurement, especially once the
> user is allowed to specify the unit of measurement, always use ram_size in
> bytes.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  builtin-run.c            | 19 ++++++++++---------
>  include/kvm/kvm-config.h |  7 ++++---
>  include/kvm/kvm.h        |  2 +-
>  3 files changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/builtin-run.c b/builtin-run.c
> index 0126c9fbcba6..236c59d2f86f 100644
> --- a/builtin-run.c
> +++ b/builtin-run.c
> @@ -36,6 +36,7 @@
>  
>  #include <linux/types.h>
>  #include <linux/err.h>
> +#include <linux/sizes.h>
>  
>  #include <sys/utsname.h>
>  #include <sys/types.h>
> @@ -264,7 +265,7 @@ static u64 host_ram_size(void)
>  		return 0;
>  	}
>  
> -	return (nr_pages * page_size) >> MB_SHIFT;
> +	return nr_pages * page_size;

So while looking for potential overflows on 32-bit systems, I realised
that this was already broken: On my Calxeda Highbank (arm32) with 8GB of
memory, this returns a negative number, because of the signed longs.
Can you cast nr_pages to u64 while at it, to fix that? I don't think we
need a separate patch for that ...

>  }
>  
>  /*
> @@ -278,11 +279,11 @@ static u64 get_ram_size(int nr_cpus)
>  	u64 available;
>  	u64 ram_size;
>  
> -	ram_size	= 64 * (nr_cpus + 3);
> +	ram_size	= SZ_64M * (nr_cpus + 3);

This overflows on 32-bit system starting with 29 vCPUs. If you use "3ULL",
this problem goes away.

Cheers,
Andre

>  
>  	available	= host_ram_size() * RAM_SIZE_RATIO;
>  	if (!available)
> -		available = MIN_RAM_SIZE_MB;
> +		available = MIN_RAM_SIZE;
>  
>  	if (ram_size > available)
>  		ram_size	= available;
> @@ -595,13 +596,13 @@ static struct kvm *kvm_cmd_run_init(int argc, const char **argv)
>  
>  	if (!kvm->cfg.ram_size)
>  		kvm->cfg.ram_size = get_ram_size(kvm->cfg.nrcpus);
> +	else
> +		kvm->cfg.ram_size <<= MB_SHIFT;
>  
>  	if (kvm->cfg.ram_size > host_ram_size())
>  		pr_warning("Guest memory size %lluMB exceeds host physical RAM size %lluMB",
> -			(unsigned long long)kvm->cfg.ram_size,
> -			(unsigned long long)host_ram_size());
> -
> -	kvm->cfg.ram_size <<= MB_SHIFT;
> +			(unsigned long long)kvm->cfg.ram_size >> MB_SHIFT,
> +			(unsigned long long)host_ram_size() >> MB_SHIFT);
>  
>  	if (!kvm->cfg.dev)
>  		kvm->cfg.dev = DEFAULT_KVM_DEV;
> @@ -676,12 +677,12 @@ static struct kvm *kvm_cmd_run_init(int argc, const char **argv)
>  	if (kvm->cfg.kernel_filename) {
>  		printf("  # %s run -k %s -m %Lu -c %d --name %s\n", KVM_BINARY_NAME,
>  		       kvm->cfg.kernel_filename,
> -		       (unsigned long long)kvm->cfg.ram_size / 1024 / 1024,
> +		       (unsigned long long)kvm->cfg.ram_size >> MB_SHIFT,
>  		       kvm->cfg.nrcpus, kvm->cfg.guest_name);
>  	} else if (kvm->cfg.firmware_filename) {
>  		printf("  # %s run --firmware %s -m %Lu -c %d --name %s\n", KVM_BINARY_NAME,
>  		       kvm->cfg.firmware_filename,
> -		       (unsigned long long)kvm->cfg.ram_size / 1024 / 1024,
> +		       (unsigned long long)kvm->cfg.ram_size >> MB_SHIFT,
>  		       kvm->cfg.nrcpus, kvm->cfg.guest_name);
>  	}
>  
> diff --git a/include/kvm/kvm-config.h b/include/kvm/kvm-config.h
> index 6a5720c4c7d4..31bc89520d52 100644
> --- a/include/kvm/kvm-config.h
> +++ b/include/kvm/kvm-config.h
> @@ -5,6 +5,8 @@
>  #include "kvm/vfio.h"
>  #include "kvm/kvm-config-arch.h"
>  
> +#include <linux/sizes.h>
> +
>  #define DEFAULT_KVM_DEV		"/dev/kvm"
>  #define DEFAULT_CONSOLE		"serial"
>  #define DEFAULT_NETWORK		"user"
> @@ -15,14 +17,13 @@
>  #define DEFAULT_SCRIPT		"none"
>  #define DEFAULT_SANDBOX_FILENAME "guest/sandbox.sh"
>  
> -#define MIN_RAM_SIZE_MB		(64ULL)
> -#define MIN_RAM_SIZE_BYTE	(MIN_RAM_SIZE_MB << MB_SHIFT)
> +#define MIN_RAM_SIZE		SZ_64M
>  
>  struct kvm_config {
>  	struct kvm_config_arch arch;
>  	struct disk_image_params disk_image[MAX_DISK_IMAGES];
>  	struct vfio_device_params *vfio_devices;
> -	u64 ram_size;
> +	u64 ram_size;		/* Guest memory size, in bytes */
>  	u8 num_net_devices;
>  	u8 num_vfio_devices;
>  	u64 vsock_cid;
> diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
> index ad732e56f5ed..7b14b33b50ca 100644
> --- a/include/kvm/kvm.h
> +++ b/include/kvm/kvm.h
> @@ -87,7 +87,7 @@ struct kvm {
>  	struct kvm_cpu		**cpus;
>  
>  	u32			mem_slots;	/* for KVM_SET_USER_MEMORY_REGION */
> -	u64			ram_size;
> +	u64			ram_size;	/* Guest memory size, in bytes */
>  	void			*ram_start;
>  	u64			ram_pagesize;
>  	struct mutex		mem_banks_lock;
diff mbox series

Patch

diff --git a/builtin-run.c b/builtin-run.c
index 0126c9fbcba6..236c59d2f86f 100644
--- a/builtin-run.c
+++ b/builtin-run.c
@@ -36,6 +36,7 @@ 
 
 #include <linux/types.h>
 #include <linux/err.h>
+#include <linux/sizes.h>
 
 #include <sys/utsname.h>
 #include <sys/types.h>
@@ -264,7 +265,7 @@  static u64 host_ram_size(void)
 		return 0;
 	}
 
-	return (nr_pages * page_size) >> MB_SHIFT;
+	return nr_pages * page_size;
 }
 
 /*
@@ -278,11 +279,11 @@  static u64 get_ram_size(int nr_cpus)
 	u64 available;
 	u64 ram_size;
 
-	ram_size	= 64 * (nr_cpus + 3);
+	ram_size	= SZ_64M * (nr_cpus + 3);
 
 	available	= host_ram_size() * RAM_SIZE_RATIO;
 	if (!available)
-		available = MIN_RAM_SIZE_MB;
+		available = MIN_RAM_SIZE;
 
 	if (ram_size > available)
 		ram_size	= available;
@@ -595,13 +596,13 @@  static struct kvm *kvm_cmd_run_init(int argc, const char **argv)
 
 	if (!kvm->cfg.ram_size)
 		kvm->cfg.ram_size = get_ram_size(kvm->cfg.nrcpus);
+	else
+		kvm->cfg.ram_size <<= MB_SHIFT;
 
 	if (kvm->cfg.ram_size > host_ram_size())
 		pr_warning("Guest memory size %lluMB exceeds host physical RAM size %lluMB",
-			(unsigned long long)kvm->cfg.ram_size,
-			(unsigned long long)host_ram_size());
-
-	kvm->cfg.ram_size <<= MB_SHIFT;
+			(unsigned long long)kvm->cfg.ram_size >> MB_SHIFT,
+			(unsigned long long)host_ram_size() >> MB_SHIFT);
 
 	if (!kvm->cfg.dev)
 		kvm->cfg.dev = DEFAULT_KVM_DEV;
@@ -676,12 +677,12 @@  static struct kvm *kvm_cmd_run_init(int argc, const char **argv)
 	if (kvm->cfg.kernel_filename) {
 		printf("  # %s run -k %s -m %Lu -c %d --name %s\n", KVM_BINARY_NAME,
 		       kvm->cfg.kernel_filename,
-		       (unsigned long long)kvm->cfg.ram_size / 1024 / 1024,
+		       (unsigned long long)kvm->cfg.ram_size >> MB_SHIFT,
 		       kvm->cfg.nrcpus, kvm->cfg.guest_name);
 	} else if (kvm->cfg.firmware_filename) {
 		printf("  # %s run --firmware %s -m %Lu -c %d --name %s\n", KVM_BINARY_NAME,
 		       kvm->cfg.firmware_filename,
-		       (unsigned long long)kvm->cfg.ram_size / 1024 / 1024,
+		       (unsigned long long)kvm->cfg.ram_size >> MB_SHIFT,
 		       kvm->cfg.nrcpus, kvm->cfg.guest_name);
 	}
 
diff --git a/include/kvm/kvm-config.h b/include/kvm/kvm-config.h
index 6a5720c4c7d4..31bc89520d52 100644
--- a/include/kvm/kvm-config.h
+++ b/include/kvm/kvm-config.h
@@ -5,6 +5,8 @@ 
 #include "kvm/vfio.h"
 #include "kvm/kvm-config-arch.h"
 
+#include <linux/sizes.h>
+
 #define DEFAULT_KVM_DEV		"/dev/kvm"
 #define DEFAULT_CONSOLE		"serial"
 #define DEFAULT_NETWORK		"user"
@@ -15,14 +17,13 @@ 
 #define DEFAULT_SCRIPT		"none"
 #define DEFAULT_SANDBOX_FILENAME "guest/sandbox.sh"
 
-#define MIN_RAM_SIZE_MB		(64ULL)
-#define MIN_RAM_SIZE_BYTE	(MIN_RAM_SIZE_MB << MB_SHIFT)
+#define MIN_RAM_SIZE		SZ_64M
 
 struct kvm_config {
 	struct kvm_config_arch arch;
 	struct disk_image_params disk_image[MAX_DISK_IMAGES];
 	struct vfio_device_params *vfio_devices;
-	u64 ram_size;
+	u64 ram_size;		/* Guest memory size, in bytes */
 	u8 num_net_devices;
 	u8 num_vfio_devices;
 	u64 vsock_cid;
diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
index ad732e56f5ed..7b14b33b50ca 100644
--- a/include/kvm/kvm.h
+++ b/include/kvm/kvm.h
@@ -87,7 +87,7 @@  struct kvm {
 	struct kvm_cpu		**cpus;
 
 	u32			mem_slots;	/* for KVM_SET_USER_MEMORY_REGION */
-	u64			ram_size;
+	u64			ram_size;	/* Guest memory size, in bytes */
 	void			*ram_start;
 	u64			ram_pagesize;
 	struct mutex		mem_banks_lock;