Re: [Minios-devel] [UNIKRAFT PATCH 8/9] plat/kvm: Add bootparams as VMM info option

["Haibo Xu (Arm Technology China)" <Haibo.Xu@xxxxxxx>]

This patch looks good except one nit below.

Reviewed-by: Haibo Xu <haibo.xu@xxxxxxx>

On 2019/6/3 22:56, Florian Schmidt wrote:
> Firecracker uses a Linux-style bootparams struct to convey information
> about memory etc. to the OS. Add this as a second option alongside the
> multiboot header. QEMU will now use multiboot, while Firecracker will
> use bootparams.
>
> Signed-off-by: Florian Schmidt <florian.schmidt@xxxxxxxxx>
> ---
>  plat/kvm/include/kvm-x86/_bootparams.h | 127 +++++++++++++++++++++++++
>  plat/kvm/include/kvm-x86/bootparams.h  |  42 ++++++++
>  plat/kvm/include/kvm-x86/vmminfo.h     |   8 ++
>  3 files changed, 177 insertions(+)
>  create mode 100644 plat/kvm/include/kvm-x86/_bootparams.h
>  create mode 100644 plat/kvm/include/kvm-x86/bootparams.h
>
> diff --git a/plat/kvm/include/kvm-x86/_bootparams.h 
> b/plat/kvm/include/kvm-x86/_bootparams.h
> new file mode 100644
> index 00000000..aabb7feb
> --- /dev/null
> +++ b/plat/kvm/include/kvm-x86/_bootparams.h
> @@ -0,0 +1,127 @@
> +/* SPDX-License-Identifier: BSD-3-Clause */
> +/*
> + * Copyright (c) 2019, NEC Europe Ltd., NEC Corporation. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of the copyright holder nor the names of its
> + *    contributors may be used to endorse or promote products derived from
> + *    this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS 
> IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGE.
> + *
> + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
> + */
> +
> +#ifndef KVM_VMMINFO_H
> +#error Do not include _bootparams.h directly!
> +#endif
> +
> +#include <string.h>
> +#include <uk/print.h>
> +#include <uk/plat/config.h>
> +#include <kvm/config.h>
> +
> +static inline void _bp_get_cmdline(struct boot_params *bp)
> +{
> +     __u64 cmdline_addr;
> +     char *bp_cmdline;
> +     size_t bp_cmdline_len = bp->hdr.cmdline_size;
> +
> +     cmdline_addr = bp->hdr.cmd_line_ptr;
> +     cmdline_addr |= (__u64)bp->ext_ramdisk_size << 32;
> +     bp_cmdline = (char *)cmdline_addr;
> +     uk_pr_info("command line at 0x%lx\n", cmdline_addr);
> +     uk_pr_info("command line size 0x%lx\n", bp_cmdline_len);
> +
> +     if (!bp_cmdline) {
> +             uk_pr_info("No command line provided\n");
> +             strncpy(cmdline, CONFIG_UK_NAME, sizeof(cmdline));
> +             return;
> +     }
> +
> +     if (bp_cmdline_len >= sizeof(cmdline)) {
> +             bp_cmdline_len = sizeof(cmdline) - 1;
> +             uk_pr_info("Command line too long, truncated\n");
> +     }
> +     memcpy(cmdline, bp_cmdline, bp_cmdline_len);
> +     /* ensure null termination */
> +     cmdline[bp_cmdline_len] = '\0';
> +
> +     uk_pr_info("Command line: %s\n", cmdline);
> +}
> +
> +static inline void _bp_init_mem(struct boot_params *bp)
> +{
> +     int i;
> +     size_t max_addr;
> +     struct boot_e820_entry *e820_entry = NULL;
> +
> +     uk_pr_info("boot_params: %d entries in e820\n", bp->e820_entries);
> +     for (i=0; i < bp->e820_entries; i++) {
> +             uk_pr_info("  e820 entry %d:\n", i);
> +             uk_pr_info("    addr: 0x%lx\n", bp->e820_table[i].addr);
> +             uk_pr_info("    size: 0x%lx\n", bp->e820_table[i].size);
> +             uk_pr_info("    type: 0x%x\n", bp->e820_table[i].type);
> +     }
> +
> +     for (i = 0; i < bp->e820_entries; i++) {
> +             uk_pr_info("Checking e820 entry %d\n", i);
> +             if (bp->e820_table[i].addr == PLATFORM_MEM_START
> +                 && bp->e820_table[i].type == 0x1) {
> +                     e820_entry = &bp->e820_table[i];
> +                     break;
> +             }
> +     }
> +     if (!e820_entry)
> +             UK_CRASH("Could not find suitable memory region!\n");
> +
> +     uk_pr_info("Using e820 memory region %d\n", i);
> +     max_addr = e820_entry->addr + e820_entry->size;
> +     if (max_addr > PLATFORM_MAX_MEM_ADDR)
> +             max_addr = PLATFORM_MAX_MEM_ADDR;
> +     UK_ASSERT((size_t)__END <= max_addr);
> +
> +     _libkvmplat_cfg.heap.start = ALIGN_UP((uintptr_t)__END, __PAGE_SIZE);
> +     _libkvmplat_cfg.heap.end   = (uintptr_t) max_addr - __STACK_SIZE;
> +     _libkvmplat_cfg.heap.len   = _libkvmplat_cfg.heap.end
> +                                  - _libkvmplat_cfg.heap.start;
> +     _libkvmplat_cfg.bstack.start = _libkvmplat_cfg.heap.end;
> +     _libkvmplat_cfg.bstack.end   = max_addr;
> +     _libkvmplat_cfg.bstack.len   = __STACK_SIZE;
> +}
> +
> +static inline void _bp_init_initrd(struct boot_params *bp __unused)
> +{
> +     /* Firecracker does not have initrd support yet. */
> +}
> +
> +static inline void process_vmminfo(void *arg __unused)
> +{
> +     /* Location of boot parameters is currently hardcoded to 0x7000
> +      * in Firecracker, but this might change at later point.
> +      */
> +     struct boot_params *bp = (struct boot_params *)0x7000;

Better use a macro for the boot parameter address.

> +
> +     uk_pr_info("     boot params: %p\n", bp);
> +     _bp_init_mem(bp);
> +     _bp_get_cmdline(bp);
> +     _bp_init_initrd(bp);
> +}
> diff --git a/plat/kvm/include/kvm-x86/bootparams.h 
> b/plat/kvm/include/kvm-x86/bootparams.h
> new file mode 100644
> index 00000000..1307e98f
> --- /dev/null
> +++ b/plat/kvm/include/kvm-x86/bootparams.h
> @@ -0,0 +1,42 @@
> +#ifndef BOOTPARAMS_HEADER
> +#define BOOTPARAMS_HEADER
> +
> +#include <uk/essentials.h>
> +
> +struct setup_header {
> +     __u8 _pad1[39];
> +     __u32 ramdisk_image;
> +     __u32 ramdisk_size;
> +     __u8 _pad2[4];
> +     __u16 heap_end_ptr;
> +     __u8 _pad3[2];
> +     __u32 cmd_line_ptr;
> +     __u8 _pad4[12];
> +     __u32 cmdline_size;
> +     __u8 _pad5[44];
> +} __attribute__((packed));
> +
> +struct boot_e820_entry {
> +     __u64 addr;
> +     __u64 size;
> +     __u32 type;
> +} __attribute__((packed));
> +
> +#define E820_MAX_ENTRIES 128
> +
> +struct boot_params {
> +     __u8 _pad1[192];
> +     __u32 ext_ramdisk_image;
> +     __u32 ext_ramdisk_size;
> +     __u32 ext_cmd_line_ptr;
> +     __u8 _pad2[284];
> +     __u8 e820_entries;
> +     __u8 _pad3[8];
> +     struct setup_header hdr;
> +     __u8 _pad4[104];
> +     struct boot_e820_entry e820_table[E820_MAX_ENTRIES];
> +     __u8 _pad5[2560-(sizeof(struct boot_e820_entry) * E820_MAX_ENTRIES)];
> +     __u8 _pad6[816];
> +} __attribute__((packed));
> +
> +#endif /* ! BOOTPARAMS_HEADER */
> diff --git a/plat/kvm/include/kvm-x86/vmminfo.h 
> b/plat/kvm/include/kvm-x86/vmminfo.h
> index 3bc86bef..3f0c6539 100644
> --- a/plat/kvm/include/kvm-x86/vmminfo.h
> +++ b/plat/kvm/include/kvm-x86/vmminfo.h
> @@ -41,8 +41,16 @@
>  #define MAX_CMDLINE_SIZE 8192
>  static char cmdline[MAX_CMDLINE_SIZE];
>
> +/* include the respective info storage method depending on VM monitor */
> +#if KVMQPLAT
>  #include <kvm-x86/multiboot.h>
>  #include <kvm-x86/multiboot_defs.h>
>  #include <kvm-x86/_multiboot.h>
> +#elif KVMFCPLAT
> +#include <kvm-x86/bootparams.h>
> +#include <kvm-x86/_bootparams.h>
> +#else
> +#error No VMM chosen for KVM environment!
> +#endif /* CONFIG_KVM_VMM_* */
>
>  #endif /* KVM_VMMINFO_H */
>
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.
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel