From: Andre Przywara <andre.przywara@arm.com>
To: Alexandru Elisei <alexandru.elisei@arm.com>
Cc: kvm@vger.kernel.org, Will Deacon <will@kernel.org>,
Julien Thierry <julien.thierry.kdev@gmail.com>,
Marc Zyngier <maz@kernel.org>,
Suzuki Poulose <suzuki.poulose@arm.com>,
Julien Grall <julien.grall.oss@gmail.com>
Subject: Re: [PATCH kvmtool 04/16] kvmtool: Add helper to sanitize arch specific KVM configuration
Date: Wed, 6 Nov 2019 16:48:48 +0000 [thread overview]
Message-ID: <20191106164848.4f2e1fbc@donnerap.cambridge.arm.com> (raw)
In-Reply-To: <1569245722-23375-5-git-send-email-alexandru.elisei@arm.com>
On Mon, 23 Sep 2019 14:35:10 +0100
Alexandru Elisei <alexandru.elisei@arm.com> 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@arm.com>
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@arm.com>
The code looks good to me:
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
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
next prev parent reply other threads:[~2019-11-06 16:48 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-23 13:35 [PATCH kvmtool 00/16] arm: Allow the user to define the memory layout Alexandru Elisei
2019-09-23 13:35 ` [PATCH kvmtool 01/16] arm: Allow use of hugepage with 16K pagesize host Alexandru Elisei
2019-11-06 16:47 ` Andre Przywara
2019-11-06 17:29 ` Alexandru Elisei
2019-09-23 13:35 ` [PATCH kvmtool 02/16] kvm__arch_init: Don't pass hugetlbfs_path and ram_size in parameter Alexandru Elisei
2019-11-06 16:47 ` Andre Przywara
2019-11-07 10:03 ` Alexandru Elisei
2019-09-23 13:35 ` [PATCH kvmtool 03/16] virtio/scsi: Allow the use of multiple banks Alexandru Elisei
2019-11-06 16:48 ` Andre Przywara
2020-02-05 18:07 ` Suzuki Kuruppassery Poulose
2019-09-23 13:35 ` [PATCH kvmtool 04/16] kvmtool: Add helper to sanitize arch specific KVM configuration Alexandru Elisei
2019-11-06 16:48 ` Andre Przywara [this message]
2019-11-07 10:05 ` Alexandru Elisei
2020-02-05 18:16 ` Suzuki Kuruppassery Poulose
2019-09-23 13:35 ` [PATCH kvmtool 05/16] kvmtool: Use MB consistently Alexandru Elisei
2019-11-06 16:49 ` Andre Przywara
2020-02-05 18:17 ` Suzuki Kuruppassery Poulose
2019-09-23 13:35 ` [PATCH kvmtool 06/16] builtin-run.c: Always use ram_size in bytes Alexandru Elisei
2019-11-06 16:49 ` Andre Przywara
2019-11-07 10:08 ` Alexandru Elisei
2020-02-05 19:03 ` Suzuki Kuruppassery Poulose
2019-09-23 13:35 ` [PATCH kvmtool 07/16] arm: Remove redundant define ARM_PCI_CFG_SIZE Alexandru Elisei
2019-11-06 16:49 ` Andre Przywara
2020-02-06 11:49 ` Suzuki Kuruppassery Poulose
2019-09-23 13:35 ` [PATCH kvmtool 08/16] arm: Move anything related to RAM initialization in kvm__init_ram Alexandru Elisei
2019-11-07 13:46 ` Andre Przywara
2019-09-23 13:35 ` [PATCH kvmtool 09/16] arm: Allow the user to specify RAM base address Alexandru Elisei
2019-11-07 13:54 ` Andre Przywara
2020-02-06 12:20 ` Suzuki Kuruppassery Poulose
2019-09-23 13:35 ` [PATCH kvmtool 10/16] kvmtool: Allow standard size specifiers for memory Alexandru Elisei
2019-11-07 13:55 ` Andre Przywara
2019-09-23 13:35 ` [PATCH kvmtool 11/16] arm/pci: Remove unused ioports Alexandru Elisei
2019-09-23 13:35 ` [PATCH kvmtool 12/16] Fold kvm__init_ram call in kvm__arch_init and rename it Alexandru Elisei
2019-09-23 13:35 ` [PATCH kvmtool 13/16] arm: Allow any base address for RAM Alexandru Elisei
2019-09-23 13:35 ` [PATCH kvmtool 14/16] arm: Move memory related code to memory.c Alexandru Elisei
2019-09-23 13:35 ` [PATCH kvmtool 15/16] kvmtool: Make the size@addr option parser globally visible Alexandru Elisei
2019-09-23 13:35 ` [PATCH kvmtool 16/16] arm: Allow the user to define the MMIO regions Alexandru Elisei
2020-02-05 17:16 ` [PATCH kvmtool 00/16] arm: Allow the user to define the memory layout Will Deacon
2020-02-05 17:18 ` Alexandru Elisei
2020-02-06 9:20 ` Marc Zyngier
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191106164848.4f2e1fbc@donnerap.cambridge.arm.com \
--to=andre.przywara@arm.com \
--cc=alexandru.elisei@arm.com \
--cc=julien.grall.oss@gmail.com \
--cc=julien.thierry.kdev@gmail.com \
--cc=kvm@vger.kernel.org \
--cc=maz@kernel.org \
--cc=suzuki.poulose@arm.com \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox