Hi,
On 11/6/19 4:48 PM, Andre Przywara wrote:
> 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.
I wasn't really sure what to do in this case. I'll keep him at the author, thanks
for the suggestion.
Thanks,
Alex
>
>> 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
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
