On Mon, 23 Sep 2019 14:35:10 +0100
Alexandru Elisei <alexandru.elisei@xxxxxxx> wrote:
Hi,
> kvmtool accepts generic and architecture specific parameters. When creating
> a virtual machine, only the generic parameters are checked against sane
> values. Add a function to sanitize the architecture specific configuration
> options and call it before the initialization routines.
>
> This patch was inspired by Julien Grall's patch.
>
> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
That's a bit confusing: If it is based on Julien's patch, you should keep him as the author, adding a short comment here about what *you* changed.
If it's not, you should not have a S-o-b: line from Julien.
> Signed-off-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
The code looks good to me:
Reviewed-by: Andre Przywara <andre.przywara@xxxxxxx>
Cheers,
Andre.
> ---
> arm/aarch64/include/kvm/kvm-arch.h | 2 +-
> arm/include/arm-common/kvm-arch.h | 4 ++++
> arm/kvm.c | 11 +++++++++--
> builtin-run.c | 2 ++
> mips/include/kvm/kvm-arch.h | 4 ++++
> mips/kvm.c | 5 +++++
> powerpc/include/kvm/kvm-arch.h | 4 ++++
> powerpc/kvm.c | 5 +++++
> x86/include/kvm/kvm-arch.h | 4 ++++
> x86/kvm.c | 24 ++++++++++++------------
> 10 files changed, 50 insertions(+), 15 deletions(-)
>
> diff --git a/arm/aarch64/include/kvm/kvm-arch.h b/arm/aarch64/include/kvm/kvm-arch.h
> index 9de623ac6cb9..1b3d0a5fb1b4 100644
> --- a/arm/aarch64/include/kvm/kvm-arch.h
> +++ b/arm/aarch64/include/kvm/kvm-arch.h
> @@ -5,7 +5,7 @@
> 0x8000 : \
> 0x80000)
>
> -#define ARM_MAX_MEMORY(kvm) ((kvm)->cfg.arch.aarch32_guest ? \
> +#define ARM_MAX_MEMORY(cfg) ((cfg)->arch.aarch32_guest ? \
> ARM_LOMAP_MAX_MEMORY : \
> ARM_HIMAP_MAX_MEMORY)
>
> diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
> index b9d486d5eac2..965978d7cfb5 100644
> --- a/arm/include/arm-common/kvm-arch.h
> +++ b/arm/include/arm-common/kvm-arch.h
> @@ -74,4 +74,8 @@ struct kvm_arch {
> u64 dtb_guest_start;
> };
>
> +struct kvm_config;
> +
> +void kvm__arch_sanitize_cfg(struct kvm_config *cfg);
> +
> #endif /* ARM_COMMON__KVM_ARCH_H */
> diff --git a/arm/kvm.c b/arm/kvm.c
> index 198cee5c0997..5decc138fd3e 100644
> --- a/arm/kvm.c
> +++ b/arm/kvm.c
> @@ -57,11 +57,18 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
> {
> }
>
> +void kvm__arch_sanitize_cfg(struct kvm_config *cfg)
> +{
> + if (cfg->ram_size > ARM_MAX_MEMORY(cfg)) {
> + cfg->ram_size = ARM_MAX_MEMORY(cfg);
> + pr_warning("Capping memory to %lluMB", cfg->ram_size >> 20);
> + }
> +}
> +
> void kvm__arch_init(struct kvm *kvm)
> {
> unsigned long alignment;
> /* Convenience aliases */
> - u64 ram_size = kvm->cfg.ram_size;
> const char *hugetlbfs_path = kvm->cfg.hugetlbfs_path;
>
> /*
> @@ -87,7 +94,7 @@ void kvm__arch_init(struct kvm *kvm)
> alignment = SZ_2M;
> }
>
> - kvm->ram_size = min(ram_size, (u64)ARM_MAX_MEMORY(kvm));
> + kvm->ram_size = kvm->cfg.ram_size;
> kvm->arch.ram_alloc_size = kvm->ram_size + alignment;
> kvm->arch.ram_alloc_start = mmap_anon_or_hugetlbfs(kvm, hugetlbfs_path,
> kvm->arch.ram_alloc_size);
> diff --git a/builtin-run.c b/builtin-run.c
> index c867c8ba0892..532c06f90ba0 100644
> --- a/builtin-run.c
> +++ b/builtin-run.c
> @@ -642,6 +642,8 @@ static struct kvm *kvm_cmd_run_init(int argc, const char **argv)
>
> kvm->cfg.real_cmdline = real_cmdline;
>
> + kvm__arch_sanitize_cfg(&kvm->cfg);
> +
> if (kvm->cfg.kernel_filename) {
> printf(" # %s run -k %s -m %Lu -c %d --name %s\n", KVM_BINARY_NAME,
> kvm->cfg.kernel_filename,
> diff --git a/mips/include/kvm/kvm-arch.h b/mips/include/kvm/kvm-arch.h
> index fdc09d830263..f0bfff50c7c9 100644
> --- a/mips/include/kvm/kvm-arch.h
> +++ b/mips/include/kvm/kvm-arch.h
> @@ -47,4 +47,8 @@ struct kvm_arch {
> bool is64bit;
> };
>
> +struct kvm_config;
> +
> +void kvm__arch_sanitize_cfg(struct kvm_config *cfg);
> +
> #endif /* KVM__KVM_ARCH_H */
> diff --git a/mips/kvm.c b/mips/kvm.c
> index e2a0c63b14b8..63d651f29f70 100644
> --- a/mips/kvm.c
> +++ b/mips/kvm.c
> @@ -56,6 +56,11 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
>
> }
>
> +void kvm__arch_sanitize_cfg(struct kvm_config *cfg)
> +{
> + /* We don't have any arch specific configuration. */
> +}
> +
> /* Architecture-specific KVM init */
> void kvm__arch_init(struct kvm *kvm)
> {
> diff --git a/powerpc/include/kvm/kvm-arch.h b/powerpc/include/kvm/kvm-arch.h
> index 8126b96cb66a..42ea7df1325f 100644
> --- a/powerpc/include/kvm/kvm-arch.h
> +++ b/powerpc/include/kvm/kvm-arch.h
> @@ -64,4 +64,8 @@ struct kvm_arch {
> struct spapr_phb *phb;
> };
>
> +struct kvm_config;
> +
> +void kvm__arch_sanitize_cfg(struct kvm_config *cfg);
> +
> #endif /* KVM__KVM_ARCH_H */
> diff --git a/powerpc/kvm.c b/powerpc/kvm.c
> index 034bc4608ad9..73965640cf82 100644
> --- a/powerpc/kvm.c
> +++ b/powerpc/kvm.c
> @@ -87,6 +87,11 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
> /* We don't need anything unusual in here. */
> }
>
> +void kvm__arch_sanitize_cfg(struct kvm_config *cfg)
> +{
> + /* We don't have any arch specific configuration. */
> +}
> +
> /* Architecture-specific KVM init */
> void kvm__arch_init(struct kvm *kvm)
> {
> diff --git a/x86/include/kvm/kvm-arch.h b/x86/include/kvm/kvm-arch.h
> index bfdd3438a9de..2cc65f30fcd2 100644
> --- a/x86/include/kvm/kvm-arch.h
> +++ b/x86/include/kvm/kvm-arch.h
> @@ -40,4 +40,8 @@ struct kvm_arch {
> struct interrupt_table interrupt_table;
> };
>
> +struct kvm_config;
> +
> +void kvm__arch_sanitize_cfg(struct kvm_config *cfg);
> +
> #endif /* KVM__KVM_ARCH_H */
> diff --git a/x86/kvm.c b/x86/kvm.c
> index 5abb41e370bb..df5d48106c80 100644
> --- a/x86/kvm.c
> +++ b/x86/kvm.c
> @@ -129,6 +129,17 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
> strcat(cmdline, " earlyprintk=serial i8042.noaux=1");
> }
>
> +void kvm__arch_sanitize_cfg(struct kvm_config *cfg)
> +{
> + /* vidmode should be either specified or set by default */
> + if (cfg->vnc || cfg->sdl || cfg->gtk) {
> + if (!cfg->arch.vidmode)
> + cfg->arch.vidmode = 0x312;
> + } else {
> + cfg->arch.vidmode = 0;
> + }
> +}
> +
> /* Architecture-specific KVM init */
> void kvm__arch_init(struct kvm *kvm)
> {
> @@ -239,7 +250,6 @@ static bool load_bzimage(struct kvm *kvm, int fd_kernel, int fd_initrd,
> size_t cmdline_size;
> ssize_t file_size;
> void *p;
> - u16 vidmode;
>
> /*
> * See Documentation/x86/boot.txt for details no bzImage on-disk and
> @@ -282,23 +292,13 @@ static bool load_bzimage(struct kvm *kvm, int fd_kernel, int fd_initrd,
> memcpy(p, kernel_cmdline, cmdline_size - 1);
> }
>
> - /* vidmode should be either specified or set by default */
> - if (kvm->cfg.vnc || kvm->cfg.sdl || kvm->cfg.gtk) {
> - if (!kvm->cfg.arch.vidmode)
> - vidmode = 0x312;
> - else
> - vidmode = kvm->cfg.arch.vidmode;
> - } else {
> - vidmode = 0;
> - }
> -
> kern_boot = guest_real_to_host(kvm, BOOT_LOADER_SELECTOR, 0x00);
>
> kern_boot->hdr.cmd_line_ptr = BOOT_CMDLINE_OFFSET;
> kern_boot->hdr.type_of_loader = 0xff;
> kern_boot->hdr.heap_end_ptr = 0xfe00;
> kern_boot->hdr.loadflags |= CAN_USE_HEAP;
> - kern_boot->hdr.vid_mode = vidmode;
> + kern_boot->hdr.vid_mode = kvm->cfg.arch.vidmode;
>
> /*
> * Read initrd image into guest memory
