On 23/09/2019 14:35, Alexandru Elisei wrote:
At the moment, the user can specify the amount of RAM a virtual machine
has, which starts at the fixed address ARM_MEMORY_AREA. The memory below
this address is used by MMIO and PCI devices.
However, it might be interesting to specify a different starting address
for the guest RAM. With this patch, the user can specify the address and
the amount of RAM a virtual machine has from the command line by using the
syntax -m/--mem size[@addr]. The address is optional, and must be higher or
equal to ARM_MEMORY_AREA. If it's not specified, the old behavior is
preserved.
This option is guarded by the define ARCH_SUPPORT_CFG_RAM_BASE, and at
the moment only the arm architecture has support for it. If an
architecture doesn't implement this feature, then the old behavior is
preserved and specifying the RAM base address is considered an user
error.
This patch is based upon previous work by Julien Grall.
Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
Signed-off-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
---
arm/aarch32/include/kvm/kvm-arch.h | 2 +-
arm/aarch64/include/kvm/kvm-arch.h | 6 ++---
arm/include/arm-common/kvm-arch.h | 6 +++--
arm/kvm.c | 17 ++++++++++----
builtin-run.c | 48 ++++++++++++++++++++++++++++++++++----
include/kvm/kvm-config.h | 3 +++
kvm.c | 6 +++++
7 files changed, 73 insertions(+), 15 deletions(-)
diff --git a/arm/aarch32/include/kvm/kvm-arch.h b/arm/aarch32/include/kvm/kvm-arch.h
index cd31e72971bd..0aa5db40502d 100644
--- a/arm/aarch32/include/kvm/kvm-arch.h
+++ b/arm/aarch32/include/kvm/kvm-arch.h
@@ -3,7 +3,7 @@
#define ARM_KERN_OFFSET(...) 0x8000
-#define ARM_MAX_MEMORY(...) ARM_LOMAP_MAX_MEMORY
+#define ARM_MAX_ADDR(...) ARM_LOMAP_MAX_ADDR
#include "arm-common/kvm-arch.h"
void kvm__arch_sanitize_cfg(struct kvm_config *cfg)
{
- if (cfg->ram_size > ARM_MAX_MEMORY(cfg)) {
- cfg->ram_size = ARM_MAX_MEMORY(cfg);
+ if (cfg->ram_base == INVALID_MEM_ADDR)
+ cfg->ram_base = ARM_MEMORY_AREA;
+
+ if (cfg->ram_base < ARM_MEMORY_AREA ||
+ cfg->ram_base >= ARM_MAX_ADDR(cfg)) {
+ cfg->ram_base = ARM_MEMORY_AREA;
+ pr_warning("Changing RAM base to 0x%llx", cfg->ram_base);
+ }
+
+ if (cfg->ram_base + cfg->ram_size > ARM_MAX_ADDR(cfg)) {
+ cfg->ram_size = ARM_MAX_ADDR(cfg) - cfg->ram_base;
pr_warning("Capping memory to %lluMB", cfg->ram_size >> 20);
}
}
@@ -229,7 +238,7 @@ bool kvm__load_firmware(struct kvm *kvm, const char *firmware_filename)
/* For default firmware address, lets load it at the begining of RAM */
if (fw_addr == 0)
- fw_addr = ARM_MEMORY_AREA;
+ fw_addr = kvm->cfg.ram_base;
At this time, we have kvm->arch.memory_guest_start set. Even though they
both might be the same, I think kvm->arch.memory_guest_start is safer
here.
if (!validate_fw_addr(kvm, fw_addr))
die("Bad firmware destination: 0x%016llx", fw_addr);
diff --git a/builtin-run.c b/builtin-run.c
index 4e0c52b3e027..df255cc44078 100644
--- a/builtin-run.c
+++ b/builtin-run.c
@@ -87,6 +87,44 @@ void kvm_run_set_wrapper_sandbox(void)
kvm_run_wrapper = KVM_RUN_SANDBOX;
}
+static int mem_parser(const struct option *opt, const char *arg, int unset)
+{
+ struct kvm_config *cfg = opt->value;
+ const char *p = arg;
+ char *next;
+ u64 size, addr = INVALID_MEM_ADDR;
+
+ /* Parse memory size. */
+ size = strtoll(p, &next, 0);
+ if (next == p)
+ die("Invalid memory size");
+
+ /* The user specifies the memory in MB, we use bytes. */
+ size <<= MB_SHIFT;
+
+ if (*next == '\0')
+ goto out;
+ else if (*next == '@')
+ p = next + 1;
+ else
+ die("Unexpected character after memory size: %c", *next);
+
+ addr = strtoll(p, &next, 0);
+ if (next == p)
+ die("Invalid memory address");
Could we use "memory base address" to be explicit ?
+
+#ifndef ARCH_SUPPORT_CFG_RAM_BASE
+ if (addr != INVALID_MEM_ADDR)
+ die("Specifying the memory address not supported by the architecture");
Same here ^
+#endif
+
+out:
+ cfg->ram_base = addr;
+ cfg->ram_size = size;
+
+ return 0;
+}
+
#ifndef OPT_ARCH_RUN
#define OPT_ARCH_RUN(...)
#endif
@@ -97,8 +135,11 @@ void kvm_run_set_wrapper_sandbox(void)
OPT_STRING('\0', "name", &(cfg)->guest_name, "guest name", \
"A name for the guest"), \
OPT_INTEGER('c', "cpus", &(cfg)->nrcpus, "Number of CPUs"), \
- OPT_U64('m', "mem", &(cfg)->ram_size, "Virtual machine memory" \
- " size in MB."), \
+ OPT_CALLBACK('m', "mem", cfg, \
+ "size[@addr]", \
+ "Virtual machine memory size in MB," \
+ " optionally starting at <addr>.", \
+ mem_parser, NULL), \
Given that we only support this option for archs who opt-in, could we
keep the "help" message consistent with what is built ?
#ifdef ARCH_SUPPORT_CFG_RAM_BASE
#define MEM_OPT_HELP_SHORT "size[@addr]"
#define MEM_OPT_HELP_DESC \
"Virtual machine memory size in MB, optionally start at <addr>"
#else
#define MEM_OPT_HELP_SHORT "size"
#define MEM_OPT_HELP_DESC "Virtual machine memory size in MB."
#endif
Or some other means, so that the help text is consistent with what
we build.
Rest looks fine to me.
Suzuki
