* [Qemu-devel] (no subject)
@ 2017-10-12 23:54 Anatol Pomozov
2017-10-12 23:54 ` [Qemu-devel] [PATCH 1/4] multiboot: Change multiboot_info from array of bytes to a C struct Anatol Pomozov
` (3 more replies)
0 siblings, 4 replies; 33+ messages in thread
From: Anatol Pomozov @ 2017-10-12 23:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, rth, ehabkost, pbonzini, agraf
It is V3 of multiboot improvements to Qemu
Changes made sinse V2:
- rebase on top of qemu master changes
- make multiboot/sections test more reliable
Add generate_sections_out.py script that generates ELF sections information
- rename 'struct section_data' to 'struct SectionData' to match naming
convention in include/hw/loader.h
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 1/4] multiboot: Change multiboot_info from array of bytes to a C struct
2017-10-12 23:54 [Qemu-devel] (no subject) Anatol Pomozov
@ 2017-10-12 23:54 ` Anatol Pomozov
2018-01-15 14:49 ` Kevin Wolf
2017-10-12 23:54 ` [Qemu-devel] [PATCH 2/4] multiboot: load any machine type of ELF Anatol Pomozov
` (2 subsequent siblings)
3 siblings, 1 reply; 33+ messages in thread
From: Anatol Pomozov @ 2017-10-12 23:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, rth, ehabkost, pbonzini, agraf, Anatol Pomozov
Using C structs makes the code more readable and prevents type conversion
errors.
Borrow multiboot1 header from GRUB project.
Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
---
hw/i386/multiboot.c | 124 +++++++++------------
| 254 ++++++++++++++++++++++++++++++++++++++++++++
tests/multiboot/mmap.c | 14 +--
tests/multiboot/mmap.out | 10 ++
tests/multiboot/modules.c | 12 ++-
tests/multiboot/modules.out | 10 ++
tests/multiboot/multiboot.h | 66 ------------
7 files changed, 339 insertions(+), 151 deletions(-)
create mode 100644 hw/i386/multiboot_header.h
delete mode 100644 tests/multiboot/multiboot.h
diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index c7b70c91d5..c9254f313e 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -28,6 +28,7 @@
#include "hw/hw.h"
#include "hw/nvram/fw_cfg.h"
#include "multiboot.h"
+#include "multiboot_header.h"
#include "hw/loader.h"
#include "elf.h"
#include "sysemu/sysemu.h"
@@ -47,39 +48,9 @@
#error multiboot struct needs to fit in 16 bit real mode
#endif
-enum {
- /* Multiboot info */
- MBI_FLAGS = 0,
- MBI_MEM_LOWER = 4,
- MBI_MEM_UPPER = 8,
- MBI_BOOT_DEVICE = 12,
- MBI_CMDLINE = 16,
- MBI_MODS_COUNT = 20,
- MBI_MODS_ADDR = 24,
- MBI_MMAP_ADDR = 48,
- MBI_BOOTLOADER = 64,
-
- MBI_SIZE = 88,
-
- /* Multiboot modules */
- MB_MOD_START = 0,
- MB_MOD_END = 4,
- MB_MOD_CMDLINE = 8,
-
- MB_MOD_SIZE = 16,
-
- /* Region offsets */
- ADDR_E820_MAP = MULTIBOOT_STRUCT_ADDR + 0,
- ADDR_MBI = ADDR_E820_MAP + 0x500,
-
- /* Multiboot flags */
- MULTIBOOT_FLAGS_MEMORY = 1 << 0,
- MULTIBOOT_FLAGS_BOOT_DEVICE = 1 << 1,
- MULTIBOOT_FLAGS_CMDLINE = 1 << 2,
- MULTIBOOT_FLAGS_MODULES = 1 << 3,
- MULTIBOOT_FLAGS_MMAP = 1 << 6,
- MULTIBOOT_FLAGS_BOOTLOADER = 1 << 9,
-};
+/* Region offsets */
+#define ADDR_E820_MAP MULTIBOOT_STRUCT_ADDR
+#define ADDR_MBI (ADDR_E820_MAP + 0x500)
typedef struct {
/* buffer holding kernel, cmdlines and mb_infos */
@@ -128,14 +99,15 @@ static void mb_add_mod(MultibootState *s,
hwaddr start, hwaddr end,
hwaddr cmdline_phys)
{
- char *p;
+ multiboot_module_t *mod;
assert(s->mb_mods_count < s->mb_mods_avail);
- p = (char *)s->mb_buf + s->offset_mbinfo + MB_MOD_SIZE * s->mb_mods_count;
+ mod = s->mb_buf + s->offset_mbinfo +
+ sizeof(multiboot_module_t) * s->mb_mods_count;
- stl_p(p + MB_MOD_START, start);
- stl_p(p + MB_MOD_END, end);
- stl_p(p + MB_MOD_CMDLINE, cmdline_phys);
+ stl_p(&mod->mod_start, start);
+ stl_p(&mod->mod_end, end);
+ stl_p(&mod->cmdline, cmdline_phys);
mb_debug("mod%02d: "TARGET_FMT_plx" - "TARGET_FMT_plx"\n",
s->mb_mods_count, start, end);
@@ -151,26 +123,29 @@ int load_multiboot(FWCfgState *fw_cfg,
int kernel_file_size,
uint8_t *header)
{
- int i, is_multiboot = 0;
+ int i;
+ bool is_multiboot = false;
uint32_t flags = 0;
uint32_t mh_entry_addr;
uint32_t mh_load_addr;
uint32_t mb_kernel_size;
MultibootState mbs;
- uint8_t bootinfo[MBI_SIZE];
+ multiboot_info_t bootinfo;
uint8_t *mb_bootinfo_data;
uint32_t cmdline_len;
+ struct multiboot_header *multiboot_header;
/* Ok, let's see if it is a multiboot image.
The header is 12x32bit long, so the latest entry may be 8192 - 48. */
for (i = 0; i < (8192 - 48); i += 4) {
- if (ldl_p(header+i) == 0x1BADB002) {
- uint32_t checksum = ldl_p(header+i+8);
- flags = ldl_p(header+i+4);
+ multiboot_header = (struct multiboot_header *)(header + i);
+ if (ldl_p(&multiboot_header->magic) == MULTIBOOT_HEADER_MAGIC) {
+ uint32_t checksum = ldl_p(&multiboot_header->checksum);
+ flags = ldl_p(&multiboot_header->flags);
checksum += flags;
- checksum += (uint32_t)0x1BADB002;
+ checksum += (uint32_t)MULTIBOOT_HEADER_MAGIC;
if (!checksum) {
- is_multiboot = 1;
+ is_multiboot = true;
break;
}
}
@@ -180,13 +155,13 @@ int load_multiboot(FWCfgState *fw_cfg,
return 0; /* no multiboot */
mb_debug("qemu: I believe we found a multiboot image!\n");
- memset(bootinfo, 0, sizeof(bootinfo));
+ memset(&bootinfo, 0, sizeof(bootinfo));
memset(&mbs, 0, sizeof(mbs));
- if (flags & 0x00000004) { /* MULTIBOOT_HEADER_HAS_VBE */
+ if (flags & MULTIBOOT_VIDEO_MODE) {
fprintf(stderr, "qemu: multiboot knows VBE. we don't.\n");
}
- if (!(flags & 0x00010000)) { /* MULTIBOOT_HEADER_HAS_ADDR */
+ if (!(flags & MULTIBOOT_AOUT_KLUDGE)) {
uint64_t elf_entry;
uint64_t elf_low, elf_high;
int kernel_size;
@@ -217,12 +192,12 @@ int load_multiboot(FWCfgState *fw_cfg,
mb_debug("qemu: loading multiboot-elf kernel (%#x bytes) with entry %#zx\n",
mb_kernel_size, (size_t)mh_entry_addr);
} else {
- /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_ADDR. */
- uint32_t mh_header_addr = ldl_p(header+i+12);
- uint32_t mh_load_end_addr = ldl_p(header+i+20);
- uint32_t mh_bss_end_addr = ldl_p(header+i+24);
+ /* Valid if mh_flags sets MULTIBOOT_AOUT_KLUDGE. */
+ uint32_t mh_header_addr = ldl_p(&multiboot_header->header_addr);
+ uint32_t mh_load_end_addr = ldl_p(&multiboot_header->load_end_addr);
+ uint32_t mh_bss_end_addr = ldl_p(&multiboot_header->bss_end_addr);
+ mh_load_addr = ldl_p(&multiboot_header->load_addr);
- mh_load_addr = ldl_p(header+i+16);
if (mh_header_addr < mh_load_addr) {
fprintf(stderr, "invalid mh_load_addr address\n");
exit(1);
@@ -230,7 +205,7 @@ int load_multiboot(FWCfgState *fw_cfg,
uint32_t mb_kernel_text_offset = i - (mh_header_addr - mh_load_addr);
uint32_t mb_load_size = 0;
- mh_entry_addr = ldl_p(header+i+28);
+ mh_entry_addr = ldl_p(&multiboot_header->entry_addr);
if (mh_load_end_addr) {
if (mh_bss_end_addr < mh_load_addr) {
@@ -253,11 +228,11 @@ int load_multiboot(FWCfgState *fw_cfg,
mb_load_size = mb_kernel_size;
}
- /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_VBE.
- uint32_t mh_mode_type = ldl_p(header+i+32);
- uint32_t mh_width = ldl_p(header+i+36);
- uint32_t mh_height = ldl_p(header+i+40);
- uint32_t mh_depth = ldl_p(header+i+44); */
+ /* Valid if mh_flags sets MULTIBOOT_VIDEO_MODE.
+ uint32_t mh_mode_type = ldl_p(&multiboot_header->mode_type);
+ uint32_t mh_width = ldl_p(&multiboot_header->width);
+ uint32_t mh_height = ldl_p(&multiboot_header->height);
+ uint32_t mh_depth = ldl_p(&multiboot_header->depth); */
mb_debug("multiboot: mh_header_addr = %#x\n", mh_header_addr);
mb_debug("multiboot: mh_load_addr = %#x\n", mh_load_addr);
@@ -295,14 +270,15 @@ int load_multiboot(FWCfgState *fw_cfg,
}
mbs.mb_buf_size += cmdline_len;
- mbs.mb_buf_size += MB_MOD_SIZE * mbs.mb_mods_avail;
+ mbs.mb_buf_size += sizeof(multiboot_module_t) * mbs.mb_mods_avail;
mbs.mb_buf_size += strlen(bootloader_name) + 1;
mbs.mb_buf_size = TARGET_PAGE_ALIGN(mbs.mb_buf_size);
/* enlarge mb_buf to hold cmdlines, bootloader, mb-info structs */
mbs.mb_buf = g_realloc(mbs.mb_buf, mbs.mb_buf_size);
- mbs.offset_cmdlines = mbs.offset_mbinfo + mbs.mb_mods_avail * MB_MOD_SIZE;
+ mbs.offset_cmdlines = mbs.offset_mbinfo +
+ mbs.mb_mods_avail * sizeof(multiboot_module_t);
mbs.offset_bootloader = mbs.offset_cmdlines + cmdline_len;
if (initrd_filename) {
@@ -348,22 +324,22 @@ int load_multiboot(FWCfgState *fw_cfg,
char kcmdline[strlen(kernel_filename) + strlen(kernel_cmdline) + 2];
snprintf(kcmdline, sizeof(kcmdline), "%s %s",
kernel_filename, kernel_cmdline);
- stl_p(bootinfo + MBI_CMDLINE, mb_add_cmdline(&mbs, kcmdline));
+ stl_p(&bootinfo.cmdline, mb_add_cmdline(&mbs, kcmdline));
- stl_p(bootinfo + MBI_BOOTLOADER, mb_add_bootloader(&mbs, bootloader_name));
+ stl_p(&bootinfo.boot_loader_name, mb_add_bootloader(&mbs, bootloader_name));
- stl_p(bootinfo + MBI_MODS_ADDR, mbs.mb_buf_phys + mbs.offset_mbinfo);
- stl_p(bootinfo + MBI_MODS_COUNT, mbs.mb_mods_count); /* mods_count */
+ stl_p(&bootinfo.mods_addr, mbs.mb_buf_phys + mbs.offset_mbinfo);
+ stl_p(&bootinfo.mods_count, mbs.mb_mods_count); /* mods_count */
/* the kernel is where we want it to be now */
- stl_p(bootinfo + MBI_FLAGS, MULTIBOOT_FLAGS_MEMORY
- | MULTIBOOT_FLAGS_BOOT_DEVICE
- | MULTIBOOT_FLAGS_CMDLINE
- | MULTIBOOT_FLAGS_MODULES
- | MULTIBOOT_FLAGS_MMAP
- | MULTIBOOT_FLAGS_BOOTLOADER);
- stl_p(bootinfo + MBI_BOOT_DEVICE, 0x8000ffff); /* XXX: use the -boot switch? */
- stl_p(bootinfo + MBI_MMAP_ADDR, ADDR_E820_MAP);
+ stl_p(&bootinfo.flags, MULTIBOOT_INFO_MEMORY
+ | MULTIBOOT_INFO_BOOTDEV
+ | MULTIBOOT_INFO_CMDLINE
+ | MULTIBOOT_INFO_MODS
+ | MULTIBOOT_INFO_MEM_MAP
+ | MULTIBOOT_INFO_BOOT_LOADER_NAME);
+ stl_p(&bootinfo.boot_device, 0x8000ffff); /* XXX: use the -boot switch? */
+ stl_p(&bootinfo.mmap_addr, ADDR_E820_MAP);
mb_debug("multiboot: mh_entry_addr = %#x\n", mh_entry_addr);
mb_debug(" mb_buf_phys = "TARGET_FMT_plx"\n", mbs.mb_buf_phys);
@@ -371,7 +347,7 @@ int load_multiboot(FWCfgState *fw_cfg,
mb_debug(" mb_mods_count = %d\n", mbs.mb_mods_count);
/* save bootinfo off the stack */
- mb_bootinfo_data = g_memdup(bootinfo, sizeof(bootinfo));
+ mb_bootinfo_data = g_memdup(&bootinfo, sizeof(bootinfo));
/* Pass variables to option rom */
fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_ENTRY, mh_entry_addr);
--git a/hw/i386/multiboot_header.h b/hw/i386/multiboot_header.h
new file mode 100644
index 0000000000..563014698c
--- /dev/null
+++ b/hw/i386/multiboot_header.h
@@ -0,0 +1,254 @@
+/* multiboot.h - Multiboot header file. */
+/* Copyright (C) 1999,2003,2007,2008,2009,2010 Free Software Foundation, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL ANY
+ * DEVELOPER OR DISTRIBUTOR BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR
+ * IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#ifndef MULTIBOOT_HEADER
+#define MULTIBOOT_HEADER 1
+
+/* How many bytes from the start of the file we search for the header. */
+#define MULTIBOOT_SEARCH 8192
+#define MULTIBOOT_HEADER_ALIGN 4
+
+/* The magic field should contain this. */
+#define MULTIBOOT_HEADER_MAGIC 0x1BADB002
+
+/* This should be in %eax. */
+#define MULTIBOOT_BOOTLOADER_MAGIC 0x2BADB002
+
+/* Alignment of multiboot modules. */
+#define MULTIBOOT_MOD_ALIGN 0x00001000
+
+/* Alignment of the multiboot info structure. */
+#define MULTIBOOT_INFO_ALIGN 0x00000004
+
+/* Flags set in the 'flags' member of the multiboot header. */
+
+/* Align all boot modules on i386 page (4KB) boundaries. */
+#define MULTIBOOT_PAGE_ALIGN 0x00000001
+
+/* Must pass memory information to OS. */
+#define MULTIBOOT_MEMORY_INFO 0x00000002
+
+/* Must pass video information to OS. */
+#define MULTIBOOT_VIDEO_MODE 0x00000004
+
+/* This flag indicates the use of the address fields in the header. */
+#define MULTIBOOT_AOUT_KLUDGE 0x00010000
+
+/* Flags to be set in the 'flags' member of the multiboot info structure. */
+
+/* is there basic lower/upper memory information? */
+#define MULTIBOOT_INFO_MEMORY 0x00000001
+/* is there a boot device set? */
+#define MULTIBOOT_INFO_BOOTDEV 0x00000002
+/* is the command-line defined? */
+#define MULTIBOOT_INFO_CMDLINE 0x00000004
+/* are there modules to do something with? */
+#define MULTIBOOT_INFO_MODS 0x00000008
+
+/* These next two are mutually exclusive */
+
+/* is there a symbol table loaded? */
+#define MULTIBOOT_INFO_AOUT_SYMS 0x00000010
+/* is there an ELF section header table? */
+#define MULTIBOOT_INFO_ELF_SHDR 0X00000020
+
+/* is there a full memory map? */
+#define MULTIBOOT_INFO_MEM_MAP 0x00000040
+
+/* Is there drive info? */
+#define MULTIBOOT_INFO_DRIVE_INFO 0x00000080
+
+/* Is there a config table? */
+#define MULTIBOOT_INFO_CONFIG_TABLE 0x00000100
+
+/* Is there a boot loader name? */
+#define MULTIBOOT_INFO_BOOT_LOADER_NAME 0x00000200
+
+/* Is there a APM table? */
+#define MULTIBOOT_INFO_APM_TABLE 0x00000400
+
+/* Is there video information? */
+#define MULTIBOOT_INFO_VBE_INFO 0x00000800
+#define MULTIBOOT_INFO_FRAMEBUFFER_INFO 0x00001000
+
+struct multiboot_header {
+ /* Must be MULTIBOOT_MAGIC - see above. */
+ uint32_t magic;
+
+ /* Feature flags. */
+ uint32_t flags;
+
+ /* The above fields plus this one must equal 0 mod 2^32. */
+ uint32_t checksum;
+
+ /* These are only valid if MULTIBOOT_AOUT_KLUDGE is set. */
+ uint32_t header_addr;
+ uint32_t load_addr;
+ uint32_t load_end_addr;
+ uint32_t bss_end_addr;
+ uint32_t entry_addr;
+
+ /* These are only valid if MULTIBOOT_VIDEO_MODE is set. */
+ uint32_t mode_type;
+ uint32_t width;
+ uint32_t height;
+ uint32_t depth;
+};
+
+/* The symbol table for a.out. */
+struct multiboot_aout_symbol_table {
+ uint32_t tabsize;
+ uint32_t strsize;
+ uint32_t addr;
+ uint32_t reserved;
+};
+typedef struct multiboot_aout_symbol_table multiboot_aout_symbol_table_t;
+
+/* The section header table for ELF. */
+struct multiboot_elf_section_header_table {
+ uint32_t num;
+ uint32_t size;
+ uint32_t addr;
+ uint32_t shndx;
+};
+typedef struct multiboot_elf_section_header_table
+ multiboot_elf_section_header_table_t;
+
+struct multiboot_info {
+ /* Multiboot info version number */
+ uint32_t flags;
+
+ /* Available memory from BIOS */
+ uint32_t mem_lower;
+ uint32_t mem_upper;
+
+ /* "root" partition */
+ uint32_t boot_device;
+
+ /* Kernel command line */
+ uint32_t cmdline;
+
+ /* Boot-Module list */
+ uint32_t mods_count;
+ uint32_t mods_addr;
+
+ union {
+ multiboot_aout_symbol_table_t aout_sym;
+ multiboot_elf_section_header_table_t elf_sec;
+ } u;
+
+ /* Memory Mapping buffer */
+ uint32_t mmap_length;
+ uint32_t mmap_addr;
+
+ /* Drive Info buffer */
+ uint32_t drives_length;
+ uint32_t drives_addr;
+
+ /* ROM configuration table */
+ uint32_t config_table;
+
+ /* Boot Loader Name */
+ uint32_t boot_loader_name;
+
+ /* APM table */
+ uint32_t apm_table;
+
+ /* Video */
+ uint32_t vbe_control_info;
+ uint32_t vbe_mode_info;
+ uint16_t vbe_mode;
+ uint16_t vbe_interface_seg;
+ uint16_t vbe_interface_off;
+ uint16_t vbe_interface_len;
+
+ uint64_t framebuffer_addr;
+ uint32_t framebuffer_pitch;
+ uint32_t framebuffer_width;
+ uint32_t framebuffer_height;
+ uint8_t framebuffer_bpp;
+#define MULTIBOOT_FRAMEBUFFER_TYPE_INDEXED 0
+#define MULTIBOOT_FRAMEBUFFER_TYPE_RGB 1
+#define MULTIBOOT_FRAMEBUFFER_TYPE_EGA_TEXT 2
+ uint8_t framebuffer_type;
+ union {
+ struct {
+ uint32_t framebuffer_palette_addr;
+ uint16_t framebuffer_palette_num_colors;
+ };
+ struct {
+ uint8_t framebuffer_red_field_position;
+ uint8_t framebuffer_red_mask_size;
+ uint8_t framebuffer_green_field_position;
+ uint8_t framebuffer_green_mask_size;
+ uint8_t framebuffer_blue_field_position;
+ uint8_t framebuffer_blue_mask_size;
+ };
+ };
+};
+typedef struct multiboot_info multiboot_info_t;
+
+struct multiboot_color {
+ uint8_t red;
+ uint8_t green;
+ uint8_t blue;
+};
+
+struct multiboot_mmap_entry {
+ uint32_t size;
+ uint64_t addr;
+ uint64_t len;
+#define MULTIBOOT_MEMORY_AVAILABLE 1
+#define MULTIBOOT_MEMORY_RESERVED 2
+#define MULTIBOOT_MEMORY_ACPI_RECLAIMABLE 3
+#define MULTIBOOT_MEMORY_NVS 4
+#define MULTIBOOT_MEMORY_BADRAM 5
+ uint32_t type;
+} __attribute__ ((packed));
+typedef struct multiboot_mmap_entry multiboot_memory_map_t;
+
+struct multiboot_mod_list {
+ /* the memory used goes from bytes 'mod_start' to 'mod_end-1' inclusive */
+ uint32_t mod_start;
+ uint32_t mod_end;
+
+ /* Module command line */
+ uint32_t cmdline;
+
+ /* padding to take it to 16 bytes (must be zero) */
+ uint32_t pad;
+};
+typedef struct multiboot_mod_list multiboot_module_t;
+
+/* APM BIOS info. */
+struct multiboot_apm_info {
+ uint16_t version;
+ uint16_t cseg;
+ uint32_t offset;
+ uint16_t cseg_16;
+ uint16_t dseg;
+ uint16_t flags;
+ uint16_t cseg_len;
+ uint16_t cseg_16_len;
+ uint16_t dseg_len;
+};
+
+#endif /* ! MULTIBOOT_HEADER */
diff --git a/tests/multiboot/mmap.c b/tests/multiboot/mmap.c
index 766b003f38..9cba8b07e0 100644
--- a/tests/multiboot/mmap.c
+++ b/tests/multiboot/mmap.c
@@ -21,15 +21,17 @@
*/
#include "libc.h"
-#include "multiboot.h"
+#include "../../hw/i386/multiboot_header.h"
-int test_main(uint32_t magic, struct mb_info *mbi)
+int test_main(uint32_t magic, struct multiboot_info *mbi)
{
uintptr_t entry_addr;
- struct mb_mmap_entry *entry;
+ struct multiboot_mmap_entry *entry;
(void) magic;
+ printf("Multiboot header at %x\n\n", mbi);
+
printf("Lower memory: %dk\n", mbi->mem_lower);
printf("Upper memory: %dk\n", mbi->mem_upper);
@@ -39,11 +41,11 @@ int test_main(uint32_t magic, struct mb_info *mbi)
entry_addr < mbi->mmap_addr + mbi->mmap_length;
entry_addr += entry->size + 4)
{
- entry = (struct mb_mmap_entry*) entry_addr;
+ entry = (struct multiboot_mmap_entry*) entry_addr;
printf("%#llx - %#llx: type %d [entry size: %d]\n",
- entry->base_addr,
- entry->base_addr + entry->length,
+ entry->addr,
+ entry->addr + entry->len,
entry->type,
entry->size);
}
diff --git a/tests/multiboot/mmap.out b/tests/multiboot/mmap.out
index 003e109b4c..e3a28237ab 100644
--- a/tests/multiboot/mmap.out
+++ b/tests/multiboot/mmap.out
@@ -3,6 +3,8 @@
=== Running test case: mmap.elf ===
+Multiboot header at 9500
+
Lower memory: 639k
Upper memory: 129920k
@@ -21,6 +23,8 @@ real mmap end: 0x9090
=== Running test case: mmap.elf -m 1.1M ===
+Multiboot header at 9500
+
Lower memory: 639k
Upper memory: 104k
@@ -38,6 +42,8 @@ real mmap end: 0x9078
=== Running test case: mmap.elf -m 2G ===
+Multiboot header at 9500
+
Lower memory: 639k
Upper memory: 2096000k
@@ -56,6 +62,8 @@ real mmap end: 0x9090
=== Running test case: mmap.elf -m 4G ===
+Multiboot header at 9500
+
Lower memory: 639k
Upper memory: 3144576k
@@ -75,6 +83,8 @@ real mmap end: 0x90a8
=== Running test case: mmap.elf -m 8G ===
+Multiboot header at 9500
+
Lower memory: 639k
Upper memory: 3144576k
diff --git a/tests/multiboot/modules.c b/tests/multiboot/modules.c
index 531601fb30..a1da0ded44 100644
--- a/tests/multiboot/modules.c
+++ b/tests/multiboot/modules.c
@@ -21,19 +21,21 @@
*/
#include "libc.h"
-#include "multiboot.h"
+#include "../../hw/i386/multiboot_header.h"
-int test_main(uint32_t magic, struct mb_info *mbi)
+int test_main(uint32_t magic, struct multiboot_info *mbi)
{
- struct mb_module *mod;
+ struct multiboot_mod_list *mod;
unsigned int i;
(void) magic;
+ printf("Multiboot header at %x\n\n", mbi);
+
printf("Module list with %d entries at %x\n",
mbi->mods_count, mbi->mods_addr);
- for (i = 0, mod = (struct mb_module*) mbi->mods_addr;
+ for (i = 0, mod = (struct multiboot_mod_list*) mbi->mods_addr;
i < mbi->mods_count;
i++, mod++)
{
@@ -41,7 +43,7 @@ int test_main(uint32_t magic, struct mb_info *mbi)
unsigned int size = mod->mod_end - mod->mod_start;
printf("[%p] Module: %x - %x (%d bytes) '%s'\n",
- mod, mod->mod_start, mod->mod_end, size, mod->string);
+ mod, mod->mod_start, mod->mod_end, size, mod->cmdline);
/* Print test file, but remove the newline at the end */
if (size < sizeof(buf)) {
diff --git a/tests/multiboot/modules.out b/tests/multiboot/modules.out
index 1636708035..0da7b39374 100644
--- a/tests/multiboot/modules.out
+++ b/tests/multiboot/modules.out
@@ -3,11 +3,15 @@
=== Running test case: modules.elf ===
+Multiboot header at 9500
+
Module list with 0 entries at 102000
=== Running test case: modules.elf -initrd module.txt ===
+Multiboot header at 9500
+
Module list with 1 entries at 102000
[102000] Module: 103000 - 103038 (56 bytes) 'module.txt'
Content: 'This is a test file that is used as a multiboot module.'
@@ -15,6 +19,8 @@ Module list with 1 entries at 102000
=== Running test case: modules.elf -initrd module.txt argument ===
+Multiboot header at 9500
+
Module list with 1 entries at 102000
[102000] Module: 103000 - 103038 (56 bytes) 'module.txt argument'
Content: 'This is a test file that is used as a multiboot module.'
@@ -22,6 +28,8 @@ Module list with 1 entries at 102000
=== Running test case: modules.elf -initrd module.txt argument,,with,,commas ===
+Multiboot header at 9500
+
Module list with 1 entries at 102000
[102000] Module: 103000 - 103038 (56 bytes) 'module.txt argument,with,commas'
Content: 'This is a test file that is used as a multiboot module.'
@@ -29,6 +37,8 @@ Module list with 1 entries at 102000
=== Running test case: modules.elf -initrd module.txt,module.txt argument,module.txt ===
+Multiboot header at 9500
+
Module list with 3 entries at 102000
[102000] Module: 103000 - 103038 (56 bytes) 'module.txt'
Content: 'This is a test file that is used as a multiboot module.'
diff --git a/tests/multiboot/multiboot.h b/tests/multiboot/multiboot.h
deleted file mode 100644
index 4eb1fbe5d4..0000000000
--- a/tests/multiboot/multiboot.h
+++ /dev/null
@@ -1,66 +0,0 @@
-/*
- * Copyright (c) 2013 Kevin Wolf <kwolf@redhat.com>
- *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to deal
- * in the Software without restriction, including without limitation the rights
- * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
- * copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
- * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
- * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
- * THE SOFTWARE.
- */
-
-#ifndef MULTIBOOT_H
-#define MULTIBOOT_H
-
-#include "libc.h"
-
-struct mb_info {
- uint32_t flags;
- uint32_t mem_lower;
- uint32_t mem_upper;
- uint32_t boot_device;
- uint32_t cmdline;
- uint32_t mods_count;
- uint32_t mods_addr;
- char syms[16];
- uint32_t mmap_length;
- uint32_t mmap_addr;
- uint32_t drives_length;
- uint32_t drives_addr;
- uint32_t config_table;
- uint32_t boot_loader_name;
- uint32_t apm_table;
- uint32_t vbe_control_info;
- uint32_t vbe_mode_info;
- uint16_t vbe_mode;
- uint16_t vbe_interface_seg;
- uint16_t vbe_interface_off;
- uint16_t vbe_interface_len;
-} __attribute__((packed));
-
-struct mb_module {
- uint32_t mod_start;
- uint32_t mod_end;
- uint32_t string;
- uint32_t reserved;
-} __attribute__((packed));
-
-struct mb_mmap_entry {
- uint32_t size;
- uint64_t base_addr;
- uint64_t length;
- uint32_t type;
-} __attribute__((packed));
-
-#endif
--
2.15.0.rc0.271.g36b669edcc-goog
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 2/4] multiboot: load any machine type of ELF
2017-10-12 23:54 [Qemu-devel] (no subject) Anatol Pomozov
2017-10-12 23:54 ` [Qemu-devel] [PATCH 1/4] multiboot: Change multiboot_info from array of bytes to a C struct Anatol Pomozov
@ 2017-10-12 23:54 ` Anatol Pomozov
2017-10-13 19:25 ` Eduardo Habkost
2017-10-12 23:54 ` [Qemu-devel] [PATCH 3/4] multiboot: load elf sections and section headers Anatol Pomozov
2017-10-12 23:54 ` [Qemu-devel] [PATCH 4/4] multiboot: make tests work with clang Anatol Pomozov
3 siblings, 1 reply; 33+ messages in thread
From: Anatol Pomozov @ 2017-10-12 23:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, rth, ehabkost, pbonzini, agraf, Anatol Pomozov
x86 is not the only architecture supported by multiboot.
For example GRUB supports MIPS architecture as well.
Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
---
hw/i386/multiboot.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index c9254f313e..7dacd6d827 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -173,7 +173,7 @@ int load_multiboot(FWCfgState *fw_cfg,
}
kernel_size = load_elf(kernel_filename, NULL, NULL, &elf_entry,
- &elf_low, &elf_high, 0, I386_ELF_MACHINE,
+ &elf_low, &elf_high, 0, EM_NONE,
0, 0);
if (kernel_size < 0) {
fprintf(stderr, "Error while loading elf kernel\n");
--
2.15.0.rc0.271.g36b669edcc-goog
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 3/4] multiboot: load elf sections and section headers
2017-10-12 23:54 [Qemu-devel] (no subject) Anatol Pomozov
2017-10-12 23:54 ` [Qemu-devel] [PATCH 1/4] multiboot: Change multiboot_info from array of bytes to a C struct Anatol Pomozov
2017-10-12 23:54 ` [Qemu-devel] [PATCH 2/4] multiboot: load any machine type of ELF Anatol Pomozov
@ 2017-10-12 23:54 ` Anatol Pomozov
2017-10-18 17:22 ` Anatol Pomozov
2018-01-15 15:41 ` Kevin Wolf
2017-10-12 23:54 ` [Qemu-devel] [PATCH 4/4] multiboot: make tests work with clang Anatol Pomozov
3 siblings, 2 replies; 33+ messages in thread
From: Anatol Pomozov @ 2017-10-12 23:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, rth, ehabkost, pbonzini, agraf, Anatol Pomozov
Multiboot may load section headers and all sections (even those that are
not part of any segment) to target memory.
Tested with an ELF application that uses data from strings table
section.
Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
---
hw/core/loader.c | 8 +--
hw/i386/multiboot.c | 83 +++++++++++++-----------
hw/s390x/ipl.c | 2 +-
include/hw/elf_ops.h | 107 ++++++++++++++++++++++++++++++-
include/hw/loader.h | 11 +++-
tests/multiboot/Makefile | 8 ++-
tests/multiboot/generate_sections_out.py | 33 ++++++++++
tests/multiboot/modules.out | 22 +++----
tests/multiboot/run_test.sh | 6 +-
tests/multiboot/sections.c | 55 ++++++++++++++++
tests/multiboot/start.S | 2 +-
11 files changed, 276 insertions(+), 61 deletions(-)
create mode 100755 tests/multiboot/generate_sections_out.py
create mode 100644 tests/multiboot/sections.c
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 4593061445..a8787f7685 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -439,7 +439,7 @@ int load_elf_as(const char *filename,
{
return load_elf_ram(filename, translate_fn, translate_opaque,
pentry, lowaddr, highaddr, big_endian, elf_machine,
- clear_lsb, data_swab, as, true);
+ clear_lsb, data_swab, as, true, NULL);
}
/* return < 0 if error, otherwise the number of bytes loaded in memory */
@@ -448,7 +448,7 @@ int load_elf_ram(const char *filename,
void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
uint64_t *highaddr, int big_endian, int elf_machine,
int clear_lsb, int data_swab, AddressSpace *as,
- bool load_rom)
+ bool load_rom, SectionsData *sections)
{
int fd, data_order, target_data_order, must_swab, ret = ELF_LOAD_FAILED;
uint8_t e_ident[EI_NIDENT];
@@ -488,11 +488,11 @@ int load_elf_ram(const char *filename,
if (e_ident[EI_CLASS] == ELFCLASS64) {
ret = load_elf64(filename, fd, translate_fn, translate_opaque, must_swab,
pentry, lowaddr, highaddr, elf_machine, clear_lsb,
- data_swab, as, load_rom);
+ data_swab, as, load_rom, sections);
} else {
ret = load_elf32(filename, fd, translate_fn, translate_opaque, must_swab,
pentry, lowaddr, highaddr, elf_machine, clear_lsb,
- data_swab, as, load_rom);
+ data_swab, as, load_rom, sections);
}
fail:
diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index 7dacd6d827..841d05160a 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -125,7 +125,7 @@ int load_multiboot(FWCfgState *fw_cfg,
{
int i;
bool is_multiboot = false;
- uint32_t flags = 0;
+ uint32_t flags = 0, bootinfo_flags = 0;
uint32_t mh_entry_addr;
uint32_t mh_load_addr;
uint32_t mb_kernel_size;
@@ -134,6 +134,7 @@ int load_multiboot(FWCfgState *fw_cfg,
uint8_t *mb_bootinfo_data;
uint32_t cmdline_len;
struct multiboot_header *multiboot_header;
+ SectionsData sections;
/* Ok, let's see if it is a multiboot image.
The header is 12x32bit long, so the latest entry may be 8192 - 48. */
@@ -161,37 +162,8 @@ int load_multiboot(FWCfgState *fw_cfg,
if (flags & MULTIBOOT_VIDEO_MODE) {
fprintf(stderr, "qemu: multiboot knows VBE. we don't.\n");
}
- if (!(flags & MULTIBOOT_AOUT_KLUDGE)) {
- uint64_t elf_entry;
- uint64_t elf_low, elf_high;
- int kernel_size;
- fclose(f);
- if (((struct elf64_hdr*)header)->e_machine == EM_X86_64) {
- fprintf(stderr, "Cannot load x86-64 image, give a 32bit one.\n");
- exit(1);
- }
-
- kernel_size = load_elf(kernel_filename, NULL, NULL, &elf_entry,
- &elf_low, &elf_high, 0, EM_NONE,
- 0, 0);
- if (kernel_size < 0) {
- fprintf(stderr, "Error while loading elf kernel\n");
- exit(1);
- }
- mh_load_addr = elf_low;
- mb_kernel_size = elf_high - elf_low;
- mh_entry_addr = elf_entry;
-
- mbs.mb_buf = g_malloc(mb_kernel_size);
- if (rom_copy(mbs.mb_buf, mh_load_addr, mb_kernel_size) != mb_kernel_size) {
- fprintf(stderr, "Error while fetching elf kernel from rom\n");
- exit(1);
- }
-
- mb_debug("qemu: loading multiboot-elf kernel (%#x bytes) with entry %#zx\n",
- mb_kernel_size, (size_t)mh_entry_addr);
- } else {
+ if (flags & MULTIBOOT_AOUT_KLUDGE) {
/* Valid if mh_flags sets MULTIBOOT_AOUT_KLUDGE. */
uint32_t mh_header_addr = ldl_p(&multiboot_header->header_addr);
uint32_t mh_load_end_addr = ldl_p(&multiboot_header->load_end_addr);
@@ -228,12 +200,6 @@ int load_multiboot(FWCfgState *fw_cfg,
mb_load_size = mb_kernel_size;
}
- /* Valid if mh_flags sets MULTIBOOT_VIDEO_MODE.
- uint32_t mh_mode_type = ldl_p(&multiboot_header->mode_type);
- uint32_t mh_width = ldl_p(&multiboot_header->width);
- uint32_t mh_height = ldl_p(&multiboot_header->height);
- uint32_t mh_depth = ldl_p(&multiboot_header->depth); */
-
mb_debug("multiboot: mh_header_addr = %#x\n", mh_header_addr);
mb_debug("multiboot: mh_load_addr = %#x\n", mh_load_addr);
mb_debug("multiboot: mh_load_end_addr = %#x\n", mh_load_end_addr);
@@ -248,9 +214,45 @@ int load_multiboot(FWCfgState *fw_cfg,
exit(1);
}
memset(mbs.mb_buf + mb_load_size, 0, mb_kernel_size - mb_load_size);
- fclose(f);
+ } else {
+ uint64_t elf_entry;
+ uint64_t elf_low, elf_high;
+ int kernel_size;
+
+ kernel_size = load_elf_ram(kernel_filename, NULL, NULL, &elf_entry,
+ &elf_low, &elf_high, 0, I386_ELF_MACHINE,
+ 0, 0, NULL, true, §ions);
+ if (kernel_size < 0) {
+ fprintf(stderr, "Error while loading elf kernel\n");
+ exit(1);
+ }
+ mh_load_addr = elf_low;
+ mb_kernel_size = elf_high - elf_low;
+ mh_entry_addr = elf_entry;
+
+ mbs.mb_buf = g_malloc(mb_kernel_size);
+ if (rom_copy(mbs.mb_buf, mh_load_addr, mb_kernel_size) != mb_kernel_size) {
+ fprintf(stderr, "Error while fetching elf kernel from rom\n");
+ exit(1);
+ }
+
+ mb_debug("qemu: loading multiboot-elf kernel (%#x bytes) with entry %#zx\n",
+ mb_kernel_size, (size_t)mh_entry_addr);
+
+ stl_p(&bootinfo.u.elf_sec.num, sections.num);
+ stl_p(&bootinfo.u.elf_sec.size, sections.size);
+ stl_p(&bootinfo.u.elf_sec.addr, sections.addr);
+ stl_p(&bootinfo.u.elf_sec.shndx, sections.shndx);
+
+ bootinfo_flags |= MULTIBOOT_INFO_ELF_SHDR;
}
+ /* Valid if mh_flags sets MULTIBOOT_VIDEO_MODE.
+ uint32_t mh_mode_type = ldl_p(&multiboot_header->mode_type);
+ uint32_t mh_width = ldl_p(&multiboot_header->width);
+ uint32_t mh_height = ldl_p(&multiboot_header->height);
+ uint32_t mh_depth = ldl_p(&multiboot_header->depth); */
+
mbs.mb_buf_phys = mh_load_addr;
mbs.mb_buf_size = TARGET_PAGE_ALIGN(mb_kernel_size);
@@ -332,7 +334,8 @@ int load_multiboot(FWCfgState *fw_cfg,
stl_p(&bootinfo.mods_count, mbs.mb_mods_count); /* mods_count */
/* the kernel is where we want it to be now */
- stl_p(&bootinfo.flags, MULTIBOOT_INFO_MEMORY
+ stl_p(&bootinfo.flags, bootinfo_flags
+ | MULTIBOOT_INFO_MEMORY
| MULTIBOOT_INFO_BOOTDEV
| MULTIBOOT_INFO_CMDLINE
| MULTIBOOT_INFO_MODS
@@ -365,5 +368,7 @@ int load_multiboot(FWCfgState *fw_cfg,
option_rom[nb_option_roms].bootindex = 0;
nb_option_roms++;
+ fclose(f);
+
return 1; /* yes, we are multiboot */
}
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 0d06fc12b6..4d9cc6261a 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -327,7 +327,7 @@ static int load_netboot_image(Error **errp)
}
img_size = load_elf_ram(netboot_filename, NULL, NULL, &ipl->start_addr,
- NULL, NULL, 1, EM_S390, 0, 0, NULL, false);
+ NULL, NULL, 1, EM_S390, 0, 0, NULL, false, NULL);
if (img_size < 0) {
img_size = load_image_size(netboot_filename, ram_ptr, ram_size);
diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
index d192e7e2a3..7a7f7983a4 100644
--- a/include/hw/elf_ops.h
+++ b/include/hw/elf_ops.h
@@ -264,12 +264,13 @@ static int glue(load_elf, SZ)(const char *name, int fd,
int must_swab, uint64_t *pentry,
uint64_t *lowaddr, uint64_t *highaddr,
int elf_machine, int clear_lsb, int data_swab,
- AddressSpace *as, bool load_rom)
+ AddressSpace *as, bool load_rom,
+ SectionsData *sections)
{
struct elfhdr ehdr;
struct elf_phdr *phdr = NULL, *ph;
int size, i, total_size;
- elf_word mem_size, file_size;
+ elf_word mem_size, file_size, sec_size;
uint64_t addr, low = (uint64_t)-1, high = 0;
uint8_t *data = NULL;
char label[128];
@@ -481,6 +482,108 @@ static int glue(load_elf, SZ)(const char *name, int fd,
}
}
g_free(phdr);
+
+ if (sections) {
+ struct elf_shdr *shdr = NULL, *sh;
+ int shsize;
+
+ // user requested loading all ELF sections
+ shsize = ehdr.e_shnum * sizeof(shdr[0]);
+ if (lseek(fd, ehdr.e_shoff, SEEK_SET) != ehdr.e_shoff) {
+ goto fail;
+ }
+ shdr = g_malloc0(shsize);
+ if (!shdr)
+ goto fail;
+ if (read(fd, shdr, shsize) != shsize)
+ goto fail;
+ if (must_swab) {
+ for (i = 0; i < ehdr.e_shnum; i++) {
+ sh = &shdr[i];
+ glue(bswap_shdr, SZ)(sh);
+ }
+ }
+
+ for(i = 0; i < ehdr.e_shnum; i++) {
+ sh = &shdr[i];
+ if (sh->sh_type == SHT_NULL)
+ continue;
+ // if section has address then it is loaded (XXX: is it true?), no need to load it again
+ if (sh->sh_addr)
+ continue;
+
+ // append section data at the end of the loaded segments
+ addr = ROUND_UP(high, sh->sh_addralign);
+ sh->sh_addr = addr;
+
+ sec_size = sh->sh_size; /* Size of the section data */
+ data = g_malloc0(sec_size);
+ if (sec_size > 0) {
+ if (lseek(fd, sh->sh_offset, SEEK_SET) < 0) {
+ goto fail;
+ }
+ if (read(fd, data, sec_size) != sec_size) {
+ goto fail;
+ }
+ }
+
+ // As these sections are not loadable no need to perform reloaction
+ // using translate_fn()
+
+ if (data_swab) {
+ int j;
+ for (j = 0; j < sec_size; j += (1 << data_swab)) {
+ uint8_t *dp = data + j;
+ switch (data_swab) {
+ case (1):
+ *(uint16_t *)dp = bswap16(*(uint16_t *)dp);
+ break;
+ case (2):
+ *(uint32_t *)dp = bswap32(*(uint32_t *)dp);
+ break;
+ case (3):
+ *(uint64_t *)dp = bswap64(*(uint64_t *)dp);
+ break;
+ default:
+ g_assert_not_reached();
+ }
+ }
+ }
+
+ if (load_rom) {
+ snprintf(label, sizeof(label), "shdr #%d: %s", i, name);
+
+ /* rom_add_elf_program() seize the ownership of 'data' */
+ rom_add_elf_program(label, data, sec_size, sec_size, addr, as);
+ } else {
+ cpu_physical_memory_write(addr, data, sec_size);
+ g_free(data);
+ }
+
+ total_size += sec_size;
+ high = addr + sec_size;
+
+ data = NULL;
+ }
+
+ sections->num = ehdr.e_shnum;
+ sections->size = ehdr.e_shentsize;
+ sections->addr = high; // Address where we load the sections header
+ sections->shndx = ehdr.e_shstrndx;
+
+ // Append section headers at the end of loaded segments/sections
+ if (load_rom) {
+ snprintf(label, sizeof(label), "shdrs");
+
+ /* rom_add_elf_program() seize the ownership of 'shdr' */
+ rom_add_elf_program(label, shdr, shsize, shsize, high, as);
+ } else {
+ cpu_physical_memory_write(high, shdr, shsize);
+ g_free(shdr);
+ }
+ high += shsize;
+ }
+
if (lowaddr)
*lowaddr = (uint64_t)(elf_sword)low;
if (highaddr)
diff --git a/include/hw/loader.h b/include/hw/loader.h
index 355fe0f5a2..3cf2d6da0f 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -65,6 +65,13 @@ int load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz);
#define ELF_LOAD_WRONG_ENDIAN -4
const char *load_elf_strerror(int error);
+typedef struct {
+ uint32_t num; // number of loaded sections
+ uint32_t size; // size of entry in sections header list
+ uint32_t addr; // address of loaded sections header
+ uint32_t shndx; // number of section that contains string table
+} SectionsData;
+
/** load_elf_ram:
* @filename: Path of ELF file
* @translate_fn: optional function to translate load addresses
@@ -82,6 +89,8 @@ const char *load_elf_strerror(int error);
* @as: The AddressSpace to load the ELF to. The value of address_space_memory
* is used if nothing is supplied here.
* @load_rom : Load ELF binary as ROM
+ * @sections: If parameter is non-NULL then all ELF sections loaded into memory
+ * and this structure is populated.
*
* Load an ELF file's contents to the emulated system's address space.
* Clients may optionally specify a callback to perform address
@@ -99,7 +108,7 @@ int load_elf_ram(const char *filename,
void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
uint64_t *highaddr, int big_endian, int elf_machine,
int clear_lsb, int data_swab, AddressSpace *as,
- bool load_rom);
+ bool load_rom, SectionsData *sections);
/** load_elf_as:
* Same as load_elf_ram(), but always loads the elf as ROM
diff --git a/tests/multiboot/Makefile b/tests/multiboot/Makefile
index 36f01dc647..b6a5056347 100644
--- a/tests/multiboot/Makefile
+++ b/tests/multiboot/Makefile
@@ -6,7 +6,7 @@ LD=ld
LDFLAGS=-melf_i386 -T link.ld
LIBS=$(shell $(CC) $(CCFLAGS) -print-libgcc-file-name)
-all: mmap.elf modules.elf
+all: mmap.elf modules.elf sections.elf sections.out
mmap.elf: start.o mmap.o libc.o
$(LD) $(LDFLAGS) -o $@ $^ $(LIBS)
@@ -14,6 +14,12 @@ mmap.elf: start.o mmap.o libc.o
modules.elf: start.o modules.o libc.o
$(LD) $(LDFLAGS) -o $@ $^ $(LIBS)
+sections.elf: start.o sections.o libc.o
+ $(LD) $(LDFLAGS) -o $@ $^ $(LIBS)
+
+sections.out: sections.elf generate_sections_out.py
+ python2 generate_sections_out.py $^ > $@
+
%.o: %.c
$(CC) $(CCFLAGS) -c -o $@ $^
diff --git a/tests/multiboot/generate_sections_out.py b/tests/multiboot/generate_sections_out.py
new file mode 100755
index 0000000000..8077856823
--- /dev/null
+++ b/tests/multiboot/generate_sections_out.py
@@ -0,0 +1,33 @@
+#!/usr/bin/env python2
+
+import sys
+
+from elftools.elf.elffile import ELFFile
+
+def roundup(num, align):
+ return (num + align - 1) / align * align
+
+def main(filename):
+ print "\n\n\n=== Running test case: sections.elf ===\n"
+ print "Multiboot header at 9500, ELF section headers present\n"
+
+ with open(filename, 'rb') as f:
+ elf = ELFFile(f)
+ header = elf.header
+ load_addr = 0x100000 # we load image starting from 1M
+ sections = ""
+ for s in elf.iter_sections():
+ align = s.header.sh_addralign
+ addr = 0
+ if s.header.sh_type != 'SHT_NULL':
+ addr = s.header.sh_addr
+ if addr == 0:
+ addr = roundup(load_addr, s.header.sh_addralign)
+ load_addr = addr + s.header.sh_size
+ sections += "Elf section name=%s addr=%x size=%d\n" % (s.name, addr, s.header.sh_size)
+
+ print "Sections list with %d entries of size %d at %x, string index %d" % (header.e_shnum, header.e_shentsize, load_addr, header.e_shstrndx)
+ print sections,
+
+if __name__ == '__main__':
+ main(sys.argv[1])
diff --git a/tests/multiboot/modules.out b/tests/multiboot/modules.out
index 0da7b39374..b6c1a01be1 100644
--- a/tests/multiboot/modules.out
+++ b/tests/multiboot/modules.out
@@ -5,15 +5,15 @@
Multiboot header at 9500
-Module list with 0 entries at 102000
+Module list with 0 entries at 104000
=== Running test case: modules.elf -initrd module.txt ===
Multiboot header at 9500
-Module list with 1 entries at 102000
-[102000] Module: 103000 - 103038 (56 bytes) 'module.txt'
+Module list with 1 entries at 104000
+[104000] Module: 105000 - 105038 (56 bytes) 'module.txt'
Content: 'This is a test file that is used as a multiboot module.'
@@ -21,8 +21,8 @@ Module list with 1 entries at 102000
Multiboot header at 9500
-Module list with 1 entries at 102000
-[102000] Module: 103000 - 103038 (56 bytes) 'module.txt argument'
+Module list with 1 entries at 104000
+[104000] Module: 105000 - 105038 (56 bytes) 'module.txt argument'
Content: 'This is a test file that is used as a multiboot module.'
@@ -30,8 +30,8 @@ Module list with 1 entries at 102000
Multiboot header at 9500
-Module list with 1 entries at 102000
-[102000] Module: 103000 - 103038 (56 bytes) 'module.txt argument,with,commas'
+Module list with 1 entries at 104000
+[104000] Module: 105000 - 105038 (56 bytes) 'module.txt argument,with,commas'
Content: 'This is a test file that is used as a multiboot module.'
@@ -39,10 +39,10 @@ Module list with 1 entries at 102000
Multiboot header at 9500
-Module list with 3 entries at 102000
-[102000] Module: 103000 - 103038 (56 bytes) 'module.txt'
+Module list with 3 entries at 104000
+[104000] Module: 105000 - 105038 (56 bytes) 'module.txt'
Content: 'This is a test file that is used as a multiboot module.'
-[102010] Module: 104000 - 104038 (56 bytes) 'module.txt argument'
+[104010] Module: 106000 - 106038 (56 bytes) 'module.txt argument'
Content: 'This is a test file that is used as a multiboot module.'
-[102020] Module: 105000 - 105038 (56 bytes) 'module.txt'
+[104020] Module: 107000 - 107038 (56 bytes) 'module.txt'
Content: 'This is a test file that is used as a multiboot module.'
diff --git a/tests/multiboot/run_test.sh b/tests/multiboot/run_test.sh
index 0278148b43..f04e35cbf0 100755
--- a/tests/multiboot/run_test.sh
+++ b/tests/multiboot/run_test.sh
@@ -56,9 +56,13 @@ modules() {
run_qemu modules.elf -initrd "module.txt,module.txt argument,module.txt"
}
+sections() {
+ run_qemu sections.elf
+}
+
make all
-for t in mmap modules; do
+for t in mmap modules sections; do
echo > test.log
$t
diff --git a/tests/multiboot/sections.c b/tests/multiboot/sections.c
new file mode 100644
index 0000000000..05a88f99ac
--- /dev/null
+++ b/tests/multiboot/sections.c
@@ -0,0 +1,55 @@
+/*
+ * Copyright (c) 2017 Anatol Pomozov <anatol.pomozov@gmail.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "libc.h"
+#include "../../hw/i386/multiboot_header.h"
+#include "../../include/elf.h"
+
+int test_main(uint32_t magic, struct multiboot_info *mbi)
+{
+ void *p;
+ unsigned int i;
+
+ (void) magic;
+ multiboot_elf_section_header_table_t shdr;
+
+ printf("Multiboot header at %x, ELF section headers %s\n\n", mbi,
+ mbi->flags & MULTIBOOT_INFO_ELF_SHDR ? "present" : "don't present");
+
+ shdr = mbi->u.elf_sec;
+ printf("Sections list with %d entries of size %d at %x, string index %d\n",
+ shdr.num, shdr.size, shdr.addr, shdr.shndx);
+
+ const char *string_table = (char *)((Elf32_Shdr *)(uintptr_t)(shdr.addr + shdr.shndx * shdr.size))->sh_addr;
+
+ for (i = 0, p = (void *)shdr.addr;
+ i < shdr.num;
+ i++, p += shdr.size)
+ {
+ Elf32_Shdr *sec;
+
+ sec = (Elf32_Shdr *)p;
+ printf("Elf section name=%s addr=%lx size=%ld\n", string_table + sec->sh_name, sec->sh_addr, sec->sh_size);
+ }
+
+ return 0;
+}
diff --git a/tests/multiboot/start.S b/tests/multiboot/start.S
index 7d33959650..bd404100c2 100644
--- a/tests/multiboot/start.S
+++ b/tests/multiboot/start.S
@@ -23,7 +23,7 @@
.section multiboot
#define MB_MAGIC 0x1badb002
-#define MB_FLAGS 0x0
+#define MB_FLAGS 0x2
#define MB_CHECKSUM -(MB_MAGIC + MB_FLAGS)
.align 4
--
2.15.0.rc0.271.g36b669edcc-goog
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 4/4] multiboot: make tests work with clang
2017-10-12 23:54 [Qemu-devel] (no subject) Anatol Pomozov
` (2 preceding siblings ...)
2017-10-12 23:54 ` [Qemu-devel] [PATCH 3/4] multiboot: load elf sections and section headers Anatol Pomozov
@ 2017-10-12 23:54 ` Anatol Pomozov
2017-10-13 8:01 ` Thomas Huth
3 siblings, 1 reply; 33+ messages in thread
From: Anatol Pomozov @ 2017-10-12 23:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, rth, ehabkost, pbonzini, agraf, Anatol Pomozov
* clang 3.8 enables SSE even for 32bit code. Generate code for pentium
CPU to make sure no new instructions are used.
* add memset() implementation. Clang implements array zeroing in
print_num() via memset() function call.
---
tests/multiboot/Makefile | 2 +-
tests/multiboot/libc.c | 9 +++++++++
tests/multiboot/libc.h | 2 ++
tests/multiboot/run_test.sh | 1 +
4 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/tests/multiboot/Makefile b/tests/multiboot/Makefile
index b6a5056347..79dfe85afc 100644
--- a/tests/multiboot/Makefile
+++ b/tests/multiboot/Makefile
@@ -1,5 +1,5 @@
CC=gcc
-CCFLAGS=-m32 -Wall -Wextra -Werror -fno-stack-protector -nostdinc -fno-builtin
+CCFLAGS=-m32 -Wall -Wextra -Werror -fno-stack-protector -nostdinc -fno-builtin -march=pentium
ASFLAGS=-m32
LD=ld
diff --git a/tests/multiboot/libc.c b/tests/multiboot/libc.c
index 6df9bda96d..512fccd7fa 100644
--- a/tests/multiboot/libc.c
+++ b/tests/multiboot/libc.c
@@ -33,6 +33,15 @@ void* memcpy(void *dest, const void *src, int n)
return dest;
}
+void *memset(void *s, int c, size_t n)
+{
+ size_t i;
+ char *d = s;
+ for (i = 0; i < n; i++) {
+ *d++ = c;
+ }
+ return s;
+}
static void print_char(char c)
{
diff --git a/tests/multiboot/libc.h b/tests/multiboot/libc.h
index 04c9922c27..bdf7c34287 100644
--- a/tests/multiboot/libc.h
+++ b/tests/multiboot/libc.h
@@ -36,6 +36,7 @@ typedef signed short int16_t;
typedef signed char int8_t;
typedef uint32_t uintptr_t;
+typedef uint32_t size_t;
/* stdarg.h */
@@ -58,5 +59,6 @@ static inline void outb(uint16_t port, uint8_t data)
void printf(const char *fmt, ...);
void* memcpy(void *dest, const void *src, int n);
+void* memset(void *s, int c, size_t n);
#endif
diff --git a/tests/multiboot/run_test.sh b/tests/multiboot/run_test.sh
index f04e35cbf0..38dcfef42c 100755
--- a/tests/multiboot/run_test.sh
+++ b/tests/multiboot/run_test.sh
@@ -29,6 +29,7 @@ run_qemu() {
printf %b "\n\n=== Running test case: $kernel $* ===\n\n" >> test.log
$QEMU \
+ -cpu pentium \
-kernel $kernel \
-display none \
-device isa-debugcon,chardev=stdio \
--
2.15.0.rc0.271.g36b669edcc-goog
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 4/4] multiboot: make tests work with clang
2017-10-12 23:54 ` [Qemu-devel] [PATCH 4/4] multiboot: make tests work with clang Anatol Pomozov
@ 2017-10-13 8:01 ` Thomas Huth
0 siblings, 0 replies; 33+ messages in thread
From: Thomas Huth @ 2017-10-13 8:01 UTC (permalink / raw)
To: Anatol Pomozov, qemu-devel; +Cc: kwolf, ehabkost, agraf, pbonzini, rth
On 13.10.2017 01:54, Anatol Pomozov wrote:
> * clang 3.8 enables SSE even for 32bit code. Generate code for pentium
> CPU to make sure no new instructions are used.
> * add memset() implementation. Clang implements array zeroing in
> print_num() via memset() function call.
> ---
> tests/multiboot/Makefile | 2 +-
> tests/multiboot/libc.c | 9 +++++++++
> tests/multiboot/libc.h | 2 ++
> tests/multiboot/run_test.sh | 1 +
> 4 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/tests/multiboot/Makefile b/tests/multiboot/Makefile
> index b6a5056347..79dfe85afc 100644
> --- a/tests/multiboot/Makefile
> +++ b/tests/multiboot/Makefile
> @@ -1,5 +1,5 @@
> CC=gcc
> -CCFLAGS=-m32 -Wall -Wextra -Werror -fno-stack-protector -nostdinc -fno-builtin
> +CCFLAGS=-m32 -Wall -Wextra -Werror -fno-stack-protector -nostdinc -fno-builtin -march=pentium
> ASFLAGS=-m32
>
> LD=ld
> diff --git a/tests/multiboot/libc.c b/tests/multiboot/libc.c
> index 6df9bda96d..512fccd7fa 100644
> --- a/tests/multiboot/libc.c
> +++ b/tests/multiboot/libc.c
> @@ -33,6 +33,15 @@ void* memcpy(void *dest, const void *src, int n)
>
> return dest;
> }
> +void *memset(void *s, int c, size_t n)
Add an empty line before the new function?
> +{
> + size_t i;
> + char *d = s;
> + for (i = 0; i < n; i++) {
while (n-- > 0) ?
... that way you don't need the i variable.
> + *d++ = c;
> + }
> + return s;
> +}
Thomas
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 2/4] multiboot: load any machine type of ELF
2017-10-12 23:54 ` [Qemu-devel] [PATCH 2/4] multiboot: load any machine type of ELF Anatol Pomozov
@ 2017-10-13 19:25 ` Eduardo Habkost
2017-10-13 21:25 ` Anatol Pomozov
0 siblings, 1 reply; 33+ messages in thread
From: Eduardo Habkost @ 2017-10-13 19:25 UTC (permalink / raw)
To: Anatol Pomozov
Cc: qemu-devel, kwolf, rth, pbonzini, agraf, Michael S. Tsirkin
On Thu, Oct 12, 2017 at 04:54:37PM -0700, Anatol Pomozov wrote:
> x86 is not the only architecture supported by multiboot.
> For example GRUB supports MIPS architecture as well.
>
> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
> ---
> hw/i386/multiboot.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
> index c9254f313e..7dacd6d827 100644
> --- a/hw/i386/multiboot.c
> +++ b/hw/i386/multiboot.c
> @@ -173,7 +173,7 @@ int load_multiboot(FWCfgState *fw_cfg,
> }
>
> kernel_size = load_elf(kernel_filename, NULL, NULL, &elf_entry,
> - &elf_low, &elf_high, 0, I386_ELF_MACHINE,
> + &elf_low, &elf_high, 0, EM_NONE,
> 0, 0);
I assume we still want PC to reject non-x86 ELF files. Isn't it
better to add a elf_machine argument to load_multiboot() so each
load_multiboot() caller can specify what's the expected
architecture?
> if (kernel_size < 0) {
> fprintf(stderr, "Error while loading elf kernel\n");
> --
> 2.15.0.rc0.271.g36b669edcc-goog
>
--
Eduardo
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 2/4] multiboot: load any machine type of ELF
2017-10-13 19:25 ` Eduardo Habkost
@ 2017-10-13 21:25 ` Anatol Pomozov
2017-10-13 23:21 ` Eduardo Habkost
0 siblings, 1 reply; 33+ messages in thread
From: Anatol Pomozov @ 2017-10-13 21:25 UTC (permalink / raw)
To: Eduardo Habkost
Cc: qemu-devel, Kevin Wolf, Richard Henderson, Paolo Bonzini,
Alexander Graf, Michael S. Tsirkin
Hi
On Fri, Oct 13, 2017 at 12:25 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Thu, Oct 12, 2017 at 04:54:37PM -0700, Anatol Pomozov wrote:
>> x86 is not the only architecture supported by multiboot.
>> For example GRUB supports MIPS architecture as well.
>>
>> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
>> ---
>> hw/i386/multiboot.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
>> index c9254f313e..7dacd6d827 100644
>> --- a/hw/i386/multiboot.c
>> +++ b/hw/i386/multiboot.c
>> @@ -173,7 +173,7 @@ int load_multiboot(FWCfgState *fw_cfg,
>> }
>>
>> kernel_size = load_elf(kernel_filename, NULL, NULL, &elf_entry,
>> - &elf_low, &elf_high, 0, I386_ELF_MACHINE,
>> + &elf_low, &elf_high, 0, EM_NONE,
>> 0, 0);
>
> I assume we still want PC to reject non-x86 ELF files.
Does multiboot spec states this restriction? I've heard that there are
attempts to implement multiboot at ARM [1] [2]. Also multiboot2 spec
mentions MIPS as one of the target architectures.
[1] https://github.com/jncronin/rpi-boot/blob/master/MULTIBOOT-ARM
[2] https://wiki.linaro.org/AndrePrzywara/Multiboot
> Isn't it
> better to add a elf_machine argument to load_multiboot() so each
> load_multiboot() caller can specify what's the expected
> architecture?
>
>
>> if (kernel_size < 0) {
>> fprintf(stderr, "Error while loading elf kernel\n");
>> --
>> 2.15.0.rc0.271.g36b669edcc-goog
>>
>
> --
> Eduardo
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 2/4] multiboot: load any machine type of ELF
2017-10-13 21:25 ` Anatol Pomozov
@ 2017-10-13 23:21 ` Eduardo Habkost
2017-10-14 13:41 ` Peter Maydell
0 siblings, 1 reply; 33+ messages in thread
From: Eduardo Habkost @ 2017-10-13 23:21 UTC (permalink / raw)
To: Anatol Pomozov
Cc: qemu-devel, Kevin Wolf, Richard Henderson, Paolo Bonzini,
Alexander Graf, Michael S. Tsirkin
On Fri, Oct 13, 2017 at 02:25:43PM -0700, Anatol Pomozov wrote:
> Hi
>
> On Fri, Oct 13, 2017 at 12:25 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Thu, Oct 12, 2017 at 04:54:37PM -0700, Anatol Pomozov wrote:
> >> x86 is not the only architecture supported by multiboot.
> >> For example GRUB supports MIPS architecture as well.
> >>
> >> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
> >> ---
> >> hw/i386/multiboot.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
> >> index c9254f313e..7dacd6d827 100644
> >> --- a/hw/i386/multiboot.c
> >> +++ b/hw/i386/multiboot.c
> >> @@ -173,7 +173,7 @@ int load_multiboot(FWCfgState *fw_cfg,
> >> }
> >>
> >> kernel_size = load_elf(kernel_filename, NULL, NULL, &elf_entry,
> >> - &elf_low, &elf_high, 0, I386_ELF_MACHINE,
> >> + &elf_low, &elf_high, 0, EM_NONE,
> >> 0, 0);
> >
> > I assume we still want PC to reject non-x86 ELF files.
>
> Does multiboot spec states this restriction? I've heard that there are
> attempts to implement multiboot at ARM [1] [2]. Also multiboot2 spec
> mentions MIPS as one of the target architectures.
>
> [1] https://github.com/jncronin/rpi-boot/blob/master/MULTIBOOT-ARM
> [2] https://wiki.linaro.org/AndrePrzywara/Multiboot
I don't believe the spec restricts that, but I don't see why it
would be useful to load an ELF file that doesn't match the target
architecture (e.g. loading non-x86 ELF files on a x86 machine
like PC).
>
> > Isn't it
> > better to add a elf_machine argument to load_multiboot() so each
> > load_multiboot() caller can specify what's the expected
> > architecture?
> >
> >
> >> if (kernel_size < 0) {
> >> fprintf(stderr, "Error while loading elf kernel\n");
> >> --
> >> 2.15.0.rc0.271.g36b669edcc-goog
> >>
> >
> > --
> > Eduardo
--
Eduardo
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 2/4] multiboot: load any machine type of ELF
2017-10-13 23:21 ` Eduardo Habkost
@ 2017-10-14 13:41 ` Peter Maydell
2017-10-16 8:27 ` Kevin Wolf
0 siblings, 1 reply; 33+ messages in thread
From: Peter Maydell @ 2017-10-14 13:41 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Anatol Pomozov, Kevin Wolf, Michael S. Tsirkin, Alexander Graf,
QEMU Developers, Paolo Bonzini, Richard Henderson
On 14 October 2017 at 00:21, Eduardo Habkost <ehabkost@redhat.com> wrote:
> I don't believe the spec restricts that, but I don't see why it
> would be useful to load an ELF file that doesn't match the target
> architecture (e.g. loading non-x86 ELF files on a x86 machine
> like PC).
Agreed. If we have non i386 boards that want to use multiboot
we should probably move the common code out of hw/i386...
thanks
-- PMM
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 2/4] multiboot: load any machine type of ELF
2017-10-14 13:41 ` Peter Maydell
@ 2017-10-16 8:27 ` Kevin Wolf
2017-10-16 18:38 ` Anatol Pomozov
0 siblings, 1 reply; 33+ messages in thread
From: Kevin Wolf @ 2017-10-16 8:27 UTC (permalink / raw)
To: Peter Maydell
Cc: Eduardo Habkost, Anatol Pomozov, Michael S. Tsirkin,
Alexander Graf, QEMU Developers, Paolo Bonzini,
Richard Henderson
Am 14.10.2017 um 15:41 hat Peter Maydell geschrieben:
> On 14 October 2017 at 00:21, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > I don't believe the spec restricts that, but I don't see why it
> > would be useful to load an ELF file that doesn't match the target
> > architecture (e.g. loading non-x86 ELF files on a x86 machine
> > like PC).
>
> Agreed. If we have non i386 boards that want to use multiboot
> we should probably move the common code out of hw/i386...
Impossible with Multiboot 1, it's a spec that is really made for i386.
The spec isn't really explicit about it being a requirement, but it does
say that its target are 32-bit OSes on PCs, and it defines the boot
state in terms of i386 registers, so it doesn't make sense for non-x86.
>From my interpretation of the spec, even support for 64-bit ELFs seems
to be (implicitly) out of spec (there is one place where it even says
"refer to the i386 ELF documentation for details"), but if GRUB
implements it...
Kevin
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 2/4] multiboot: load any machine type of ELF
2017-10-16 8:27 ` Kevin Wolf
@ 2017-10-16 18:38 ` Anatol Pomozov
2018-01-15 14:52 ` Kevin Wolf
0 siblings, 1 reply; 33+ messages in thread
From: Anatol Pomozov @ 2017-10-16 18:38 UTC (permalink / raw)
To: Kevin Wolf
Cc: Peter Maydell, Eduardo Habkost, Michael S. Tsirkin,
Alexander Graf, QEMU Developers, Paolo Bonzini,
Richard Henderson
Hi
On Mon, Oct 16, 2017 at 1:27 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 14.10.2017 um 15:41 hat Peter Maydell geschrieben:
>> On 14 October 2017 at 00:21, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> > I don't believe the spec restricts that, but I don't see why it
>> > would be useful to load an ELF file that doesn't match the target
>> > architecture (e.g. loading non-x86 ELF files on a x86 machine
>> > like PC).
I see what do you mean Eduardo. Yes it makes sense to restrict ELF to
the currently emulated platform.
On a second thought adding multiboot support for non x86 needs to go
with other changes, e.g. multiboot.c should be moved out of hw/i386 to
some platform-independent location. It probably worth to postpone this
change until after Qemu gets multiboot2 support that explicitly states
MIPS support, thus it will be easier to test this codepath on multiple
platforms.
So if you don't mind I'll remove this patch from the current patch
series and put it into later one.
>>
>> Agreed. If we have non i386 boards that want to use multiboot
>> we should probably move the common code out of hw/i386...
>
> Impossible with Multiboot 1, it's a spec that is really made for i386.
>
> The spec isn't really explicit about it being a requirement, but it does
> say that its target are 32-bit OSes on PCs, and it defines the boot
> state in terms of i386 registers, so it doesn't make sense for non-x86.
>
> From my interpretation of the spec, even support for 64-bit ELFs seems
> to be (implicitly) out of spec (there is one place where it even says
> "refer to the i386 ELF documentation for details"), but if GRUB
> implements it...
Yes GRUB has support for ELF64 multiboot loading and it successfully
used by many projects in osdev community.
This functionality has been added to GRUB 12 years ago
https://github.com/coreos/grub/commit/ea4097134fbd834d2f688363f9a8208bf6a49ecd
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 3/4] multiboot: load elf sections and section headers
2017-10-12 23:54 ` [Qemu-devel] [PATCH 3/4] multiboot: load elf sections and section headers Anatol Pomozov
@ 2017-10-18 17:22 ` Anatol Pomozov
2017-10-19 9:36 ` Kevin Wolf
2018-01-15 15:41 ` Kevin Wolf
1 sibling, 1 reply; 33+ messages in thread
From: Anatol Pomozov @ 2017-10-18 17:22 UTC (permalink / raw)
To: QEMU Developers
Cc: Kevin Wolf, Richard Henderson, Eduardo Habkost, Paolo Bonzini,
Alexander Graf, Anatol Pomozov
Hello qemu-devs,
This patchset has been posted a while ago. I would like to move
forward with it and look at the next task (e.g. multiboot2 support in
QEMU). Who is the multiboot code maintainer who can help to review
this patchset and give go/no-go answer?
On Thu, Oct 12, 2017 at 4:54 PM, Anatol Pomozov
<anatol.pomozov@gmail.com> wrote:
> Multiboot may load section headers and all sections (even those that are
> not part of any segment) to target memory.
>
> Tested with an ELF application that uses data from strings table
> section.
>
> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
> ---
> hw/core/loader.c | 8 +--
> hw/i386/multiboot.c | 83 +++++++++++++-----------
> hw/s390x/ipl.c | 2 +-
> include/hw/elf_ops.h | 107 ++++++++++++++++++++++++++++++-
> include/hw/loader.h | 11 +++-
> tests/multiboot/Makefile | 8 ++-
> tests/multiboot/generate_sections_out.py | 33 ++++++++++
> tests/multiboot/modules.out | 22 +++----
> tests/multiboot/run_test.sh | 6 +-
> tests/multiboot/sections.c | 55 ++++++++++++++++
> tests/multiboot/start.S | 2 +-
> 11 files changed, 276 insertions(+), 61 deletions(-)
> create mode 100755 tests/multiboot/generate_sections_out.py
> create mode 100644 tests/multiboot/sections.c
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 4593061445..a8787f7685 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -439,7 +439,7 @@ int load_elf_as(const char *filename,
> {
> return load_elf_ram(filename, translate_fn, translate_opaque,
> pentry, lowaddr, highaddr, big_endian, elf_machine,
> - clear_lsb, data_swab, as, true);
> + clear_lsb, data_swab, as, true, NULL);
> }
>
> /* return < 0 if error, otherwise the number of bytes loaded in memory */
> @@ -448,7 +448,7 @@ int load_elf_ram(const char *filename,
> void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
> uint64_t *highaddr, int big_endian, int elf_machine,
> int clear_lsb, int data_swab, AddressSpace *as,
> - bool load_rom)
> + bool load_rom, SectionsData *sections)
> {
> int fd, data_order, target_data_order, must_swab, ret = ELF_LOAD_FAILED;
> uint8_t e_ident[EI_NIDENT];
> @@ -488,11 +488,11 @@ int load_elf_ram(const char *filename,
> if (e_ident[EI_CLASS] == ELFCLASS64) {
> ret = load_elf64(filename, fd, translate_fn, translate_opaque, must_swab,
> pentry, lowaddr, highaddr, elf_machine, clear_lsb,
> - data_swab, as, load_rom);
> + data_swab, as, load_rom, sections);
> } else {
> ret = load_elf32(filename, fd, translate_fn, translate_opaque, must_swab,
> pentry, lowaddr, highaddr, elf_machine, clear_lsb,
> - data_swab, as, load_rom);
> + data_swab, as, load_rom, sections);
> }
>
> fail:
> diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
> index 7dacd6d827..841d05160a 100644
> --- a/hw/i386/multiboot.c
> +++ b/hw/i386/multiboot.c
> @@ -125,7 +125,7 @@ int load_multiboot(FWCfgState *fw_cfg,
> {
> int i;
> bool is_multiboot = false;
> - uint32_t flags = 0;
> + uint32_t flags = 0, bootinfo_flags = 0;
> uint32_t mh_entry_addr;
> uint32_t mh_load_addr;
> uint32_t mb_kernel_size;
> @@ -134,6 +134,7 @@ int load_multiboot(FWCfgState *fw_cfg,
> uint8_t *mb_bootinfo_data;
> uint32_t cmdline_len;
> struct multiboot_header *multiboot_header;
> + SectionsData sections;
>
> /* Ok, let's see if it is a multiboot image.
> The header is 12x32bit long, so the latest entry may be 8192 - 48. */
> @@ -161,37 +162,8 @@ int load_multiboot(FWCfgState *fw_cfg,
> if (flags & MULTIBOOT_VIDEO_MODE) {
> fprintf(stderr, "qemu: multiboot knows VBE. we don't.\n");
> }
> - if (!(flags & MULTIBOOT_AOUT_KLUDGE)) {
> - uint64_t elf_entry;
> - uint64_t elf_low, elf_high;
> - int kernel_size;
> - fclose(f);
>
> - if (((struct elf64_hdr*)header)->e_machine == EM_X86_64) {
> - fprintf(stderr, "Cannot load x86-64 image, give a 32bit one.\n");
> - exit(1);
> - }
> -
> - kernel_size = load_elf(kernel_filename, NULL, NULL, &elf_entry,
> - &elf_low, &elf_high, 0, EM_NONE,
> - 0, 0);
> - if (kernel_size < 0) {
> - fprintf(stderr, "Error while loading elf kernel\n");
> - exit(1);
> - }
> - mh_load_addr = elf_low;
> - mb_kernel_size = elf_high - elf_low;
> - mh_entry_addr = elf_entry;
> -
> - mbs.mb_buf = g_malloc(mb_kernel_size);
> - if (rom_copy(mbs.mb_buf, mh_load_addr, mb_kernel_size) != mb_kernel_size) {
> - fprintf(stderr, "Error while fetching elf kernel from rom\n");
> - exit(1);
> - }
> -
> - mb_debug("qemu: loading multiboot-elf kernel (%#x bytes) with entry %#zx\n",
> - mb_kernel_size, (size_t)mh_entry_addr);
> - } else {
> + if (flags & MULTIBOOT_AOUT_KLUDGE) {
> /* Valid if mh_flags sets MULTIBOOT_AOUT_KLUDGE. */
> uint32_t mh_header_addr = ldl_p(&multiboot_header->header_addr);
> uint32_t mh_load_end_addr = ldl_p(&multiboot_header->load_end_addr);
> @@ -228,12 +200,6 @@ int load_multiboot(FWCfgState *fw_cfg,
> mb_load_size = mb_kernel_size;
> }
>
> - /* Valid if mh_flags sets MULTIBOOT_VIDEO_MODE.
> - uint32_t mh_mode_type = ldl_p(&multiboot_header->mode_type);
> - uint32_t mh_width = ldl_p(&multiboot_header->width);
> - uint32_t mh_height = ldl_p(&multiboot_header->height);
> - uint32_t mh_depth = ldl_p(&multiboot_header->depth); */
> -
> mb_debug("multiboot: mh_header_addr = %#x\n", mh_header_addr);
> mb_debug("multiboot: mh_load_addr = %#x\n", mh_load_addr);
> mb_debug("multiboot: mh_load_end_addr = %#x\n", mh_load_end_addr);
> @@ -248,9 +214,45 @@ int load_multiboot(FWCfgState *fw_cfg,
> exit(1);
> }
> memset(mbs.mb_buf + mb_load_size, 0, mb_kernel_size - mb_load_size);
> - fclose(f);
> + } else {
> + uint64_t elf_entry;
> + uint64_t elf_low, elf_high;
> + int kernel_size;
> +
> + kernel_size = load_elf_ram(kernel_filename, NULL, NULL, &elf_entry,
> + &elf_low, &elf_high, 0, I386_ELF_MACHINE,
> + 0, 0, NULL, true, §ions);
> + if (kernel_size < 0) {
> + fprintf(stderr, "Error while loading elf kernel\n");
> + exit(1);
> + }
> + mh_load_addr = elf_low;
> + mb_kernel_size = elf_high - elf_low;
> + mh_entry_addr = elf_entry;
> +
> + mbs.mb_buf = g_malloc(mb_kernel_size);
> + if (rom_copy(mbs.mb_buf, mh_load_addr, mb_kernel_size) != mb_kernel_size) {
> + fprintf(stderr, "Error while fetching elf kernel from rom\n");
> + exit(1);
> + }
> +
> + mb_debug("qemu: loading multiboot-elf kernel (%#x bytes) with entry %#zx\n",
> + mb_kernel_size, (size_t)mh_entry_addr);
> +
> + stl_p(&bootinfo.u.elf_sec.num, sections.num);
> + stl_p(&bootinfo.u.elf_sec.size, sections.size);
> + stl_p(&bootinfo.u.elf_sec.addr, sections.addr);
> + stl_p(&bootinfo.u.elf_sec.shndx, sections.shndx);
> +
> + bootinfo_flags |= MULTIBOOT_INFO_ELF_SHDR;
> }
>
> + /* Valid if mh_flags sets MULTIBOOT_VIDEO_MODE.
> + uint32_t mh_mode_type = ldl_p(&multiboot_header->mode_type);
> + uint32_t mh_width = ldl_p(&multiboot_header->width);
> + uint32_t mh_height = ldl_p(&multiboot_header->height);
> + uint32_t mh_depth = ldl_p(&multiboot_header->depth); */
> +
> mbs.mb_buf_phys = mh_load_addr;
>
> mbs.mb_buf_size = TARGET_PAGE_ALIGN(mb_kernel_size);
> @@ -332,7 +334,8 @@ int load_multiboot(FWCfgState *fw_cfg,
> stl_p(&bootinfo.mods_count, mbs.mb_mods_count); /* mods_count */
>
> /* the kernel is where we want it to be now */
> - stl_p(&bootinfo.flags, MULTIBOOT_INFO_MEMORY
> + stl_p(&bootinfo.flags, bootinfo_flags
> + | MULTIBOOT_INFO_MEMORY
> | MULTIBOOT_INFO_BOOTDEV
> | MULTIBOOT_INFO_CMDLINE
> | MULTIBOOT_INFO_MODS
> @@ -365,5 +368,7 @@ int load_multiboot(FWCfgState *fw_cfg,
> option_rom[nb_option_roms].bootindex = 0;
> nb_option_roms++;
>
> + fclose(f);
> +
> return 1; /* yes, we are multiboot */
> }
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 0d06fc12b6..4d9cc6261a 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -327,7 +327,7 @@ static int load_netboot_image(Error **errp)
> }
>
> img_size = load_elf_ram(netboot_filename, NULL, NULL, &ipl->start_addr,
> - NULL, NULL, 1, EM_S390, 0, 0, NULL, false);
> + NULL, NULL, 1, EM_S390, 0, 0, NULL, false, NULL);
>
> if (img_size < 0) {
> img_size = load_image_size(netboot_filename, ram_ptr, ram_size);
> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
> index d192e7e2a3..7a7f7983a4 100644
> --- a/include/hw/elf_ops.h
> +++ b/include/hw/elf_ops.h
> @@ -264,12 +264,13 @@ static int glue(load_elf, SZ)(const char *name, int fd,
> int must_swab, uint64_t *pentry,
> uint64_t *lowaddr, uint64_t *highaddr,
> int elf_machine, int clear_lsb, int data_swab,
> - AddressSpace *as, bool load_rom)
> + AddressSpace *as, bool load_rom,
> + SectionsData *sections)
> {
> struct elfhdr ehdr;
> struct elf_phdr *phdr = NULL, *ph;
> int size, i, total_size;
> - elf_word mem_size, file_size;
> + elf_word mem_size, file_size, sec_size;
> uint64_t addr, low = (uint64_t)-1, high = 0;
> uint8_t *data = NULL;
> char label[128];
> @@ -481,6 +482,108 @@ static int glue(load_elf, SZ)(const char *name, int fd,
> }
> }
> g_free(phdr);
> +
> + if (sections) {
> + struct elf_shdr *shdr = NULL, *sh;
> + int shsize;
> +
> + // user requested loading all ELF sections
> + shsize = ehdr.e_shnum * sizeof(shdr[0]);
> + if (lseek(fd, ehdr.e_shoff, SEEK_SET) != ehdr.e_shoff) {
> + goto fail;
> + }
> + shdr = g_malloc0(shsize);
> + if (!shdr)
> + goto fail;
> + if (read(fd, shdr, shsize) != shsize)
> + goto fail;
> + if (must_swab) {
> + for (i = 0; i < ehdr.e_shnum; i++) {
> + sh = &shdr[i];
> + glue(bswap_shdr, SZ)(sh);
> + }
> + }
> +
> + for(i = 0; i < ehdr.e_shnum; i++) {
> + sh = &shdr[i];
> + if (sh->sh_type == SHT_NULL)
> + continue;
> + // if section has address then it is loaded (XXX: is it true?), no need to load it again
> + if (sh->sh_addr)
> + continue;
> +
> + // append section data at the end of the loaded segments
> + addr = ROUND_UP(high, sh->sh_addralign);
> + sh->sh_addr = addr;
> +
> + sec_size = sh->sh_size; /* Size of the section data */
> + data = g_malloc0(sec_size);
> + if (sec_size > 0) {
> + if (lseek(fd, sh->sh_offset, SEEK_SET) < 0) {
> + goto fail;
> + }
> + if (read(fd, data, sec_size) != sec_size) {
> + goto fail;
> + }
> + }
> +
> + // As these sections are not loadable no need to perform reloaction
> + // using translate_fn()
> +
> + if (data_swab) {
> + int j;
> + for (j = 0; j < sec_size; j += (1 << data_swab)) {
> + uint8_t *dp = data + j;
> + switch (data_swab) {
> + case (1):
> + *(uint16_t *)dp = bswap16(*(uint16_t *)dp);
> + break;
> + case (2):
> + *(uint32_t *)dp = bswap32(*(uint32_t *)dp);
> + break;
> + case (3):
> + *(uint64_t *)dp = bswap64(*(uint64_t *)dp);
> + break;
> + default:
> + g_assert_not_reached();
> + }
> + }
> + }
> +
> + if (load_rom) {
> + snprintf(label, sizeof(label), "shdr #%d: %s", i, name);
> +
> + /* rom_add_elf_program() seize the ownership of 'data' */
> + rom_add_elf_program(label, data, sec_size, sec_size, addr, as);
> + } else {
> + cpu_physical_memory_write(addr, data, sec_size);
> + g_free(data);
> + }
> +
> + total_size += sec_size;
> + high = addr + sec_size;
> +
> + data = NULL;
> + }
> +
> + sections->num = ehdr.e_shnum;
> + sections->size = ehdr.e_shentsize;
> + sections->addr = high; // Address where we load the sections header
> + sections->shndx = ehdr.e_shstrndx;
> +
> + // Append section headers at the end of loaded segments/sections
> + if (load_rom) {
> + snprintf(label, sizeof(label), "shdrs");
> +
> + /* rom_add_elf_program() seize the ownership of 'shdr' */
> + rom_add_elf_program(label, shdr, shsize, shsize, high, as);
> + } else {
> + cpu_physical_memory_write(high, shdr, shsize);
> + g_free(shdr);
> + }
> + high += shsize;
> + }
> +
> if (lowaddr)
> *lowaddr = (uint64_t)(elf_sword)low;
> if (highaddr)
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 355fe0f5a2..3cf2d6da0f 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -65,6 +65,13 @@ int load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz);
> #define ELF_LOAD_WRONG_ENDIAN -4
> const char *load_elf_strerror(int error);
>
> +typedef struct {
> + uint32_t num; // number of loaded sections
> + uint32_t size; // size of entry in sections header list
> + uint32_t addr; // address of loaded sections header
> + uint32_t shndx; // number of section that contains string table
> +} SectionsData;
> +
> /** load_elf_ram:
> * @filename: Path of ELF file
> * @translate_fn: optional function to translate load addresses
> @@ -82,6 +89,8 @@ const char *load_elf_strerror(int error);
> * @as: The AddressSpace to load the ELF to. The value of address_space_memory
> * is used if nothing is supplied here.
> * @load_rom : Load ELF binary as ROM
> + * @sections: If parameter is non-NULL then all ELF sections loaded into memory
> + * and this structure is populated.
> *
> * Load an ELF file's contents to the emulated system's address space.
> * Clients may optionally specify a callback to perform address
> @@ -99,7 +108,7 @@ int load_elf_ram(const char *filename,
> void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
> uint64_t *highaddr, int big_endian, int elf_machine,
> int clear_lsb, int data_swab, AddressSpace *as,
> - bool load_rom);
> + bool load_rom, SectionsData *sections);
>
> /** load_elf_as:
> * Same as load_elf_ram(), but always loads the elf as ROM
> diff --git a/tests/multiboot/Makefile b/tests/multiboot/Makefile
> index 36f01dc647..b6a5056347 100644
> --- a/tests/multiboot/Makefile
> +++ b/tests/multiboot/Makefile
> @@ -6,7 +6,7 @@ LD=ld
> LDFLAGS=-melf_i386 -T link.ld
> LIBS=$(shell $(CC) $(CCFLAGS) -print-libgcc-file-name)
>
> -all: mmap.elf modules.elf
> +all: mmap.elf modules.elf sections.elf sections.out
>
> mmap.elf: start.o mmap.o libc.o
> $(LD) $(LDFLAGS) -o $@ $^ $(LIBS)
> @@ -14,6 +14,12 @@ mmap.elf: start.o mmap.o libc.o
> modules.elf: start.o modules.o libc.o
> $(LD) $(LDFLAGS) -o $@ $^ $(LIBS)
>
> +sections.elf: start.o sections.o libc.o
> + $(LD) $(LDFLAGS) -o $@ $^ $(LIBS)
> +
> +sections.out: sections.elf generate_sections_out.py
> + python2 generate_sections_out.py $^ > $@
> +
> %.o: %.c
> $(CC) $(CCFLAGS) -c -o $@ $^
>
> diff --git a/tests/multiboot/generate_sections_out.py b/tests/multiboot/generate_sections_out.py
> new file mode 100755
> index 0000000000..8077856823
> --- /dev/null
> +++ b/tests/multiboot/generate_sections_out.py
> @@ -0,0 +1,33 @@
> +#!/usr/bin/env python2
> +
> +import sys
> +
> +from elftools.elf.elffile import ELFFile
> +
> +def roundup(num, align):
> + return (num + align - 1) / align * align
> +
> +def main(filename):
> + print "\n\n\n=== Running test case: sections.elf ===\n"
> + print "Multiboot header at 9500, ELF section headers present\n"
> +
> + with open(filename, 'rb') as f:
> + elf = ELFFile(f)
> + header = elf.header
> + load_addr = 0x100000 # we load image starting from 1M
> + sections = ""
> + for s in elf.iter_sections():
> + align = s.header.sh_addralign
> + addr = 0
> + if s.header.sh_type != 'SHT_NULL':
> + addr = s.header.sh_addr
> + if addr == 0:
> + addr = roundup(load_addr, s.header.sh_addralign)
> + load_addr = addr + s.header.sh_size
> + sections += "Elf section name=%s addr=%x size=%d\n" % (s.name, addr, s.header.sh_size)
> +
> + print "Sections list with %d entries of size %d at %x, string index %d" % (header.e_shnum, header.e_shentsize, load_addr, header.e_shstrndx)
> + print sections,
> +
> +if __name__ == '__main__':
> + main(sys.argv[1])
> diff --git a/tests/multiboot/modules.out b/tests/multiboot/modules.out
> index 0da7b39374..b6c1a01be1 100644
> --- a/tests/multiboot/modules.out
> +++ b/tests/multiboot/modules.out
> @@ -5,15 +5,15 @@
>
> Multiboot header at 9500
>
> -Module list with 0 entries at 102000
> +Module list with 0 entries at 104000
>
>
> === Running test case: modules.elf -initrd module.txt ===
>
> Multiboot header at 9500
>
> -Module list with 1 entries at 102000
> -[102000] Module: 103000 - 103038 (56 bytes) 'module.txt'
> +Module list with 1 entries at 104000
> +[104000] Module: 105000 - 105038 (56 bytes) 'module.txt'
> Content: 'This is a test file that is used as a multiboot module.'
>
>
> @@ -21,8 +21,8 @@ Module list with 1 entries at 102000
>
> Multiboot header at 9500
>
> -Module list with 1 entries at 102000
> -[102000] Module: 103000 - 103038 (56 bytes) 'module.txt argument'
> +Module list with 1 entries at 104000
> +[104000] Module: 105000 - 105038 (56 bytes) 'module.txt argument'
> Content: 'This is a test file that is used as a multiboot module.'
>
>
> @@ -30,8 +30,8 @@ Module list with 1 entries at 102000
>
> Multiboot header at 9500
>
> -Module list with 1 entries at 102000
> -[102000] Module: 103000 - 103038 (56 bytes) 'module.txt argument,with,commas'
> +Module list with 1 entries at 104000
> +[104000] Module: 105000 - 105038 (56 bytes) 'module.txt argument,with,commas'
> Content: 'This is a test file that is used as a multiboot module.'
>
>
> @@ -39,10 +39,10 @@ Module list with 1 entries at 102000
>
> Multiboot header at 9500
>
> -Module list with 3 entries at 102000
> -[102000] Module: 103000 - 103038 (56 bytes) 'module.txt'
> +Module list with 3 entries at 104000
> +[104000] Module: 105000 - 105038 (56 bytes) 'module.txt'
> Content: 'This is a test file that is used as a multiboot module.'
> -[102010] Module: 104000 - 104038 (56 bytes) 'module.txt argument'
> +[104010] Module: 106000 - 106038 (56 bytes) 'module.txt argument'
> Content: 'This is a test file that is used as a multiboot module.'
> -[102020] Module: 105000 - 105038 (56 bytes) 'module.txt'
> +[104020] Module: 107000 - 107038 (56 bytes) 'module.txt'
> Content: 'This is a test file that is used as a multiboot module.'
> diff --git a/tests/multiboot/run_test.sh b/tests/multiboot/run_test.sh
> index 0278148b43..f04e35cbf0 100755
> --- a/tests/multiboot/run_test.sh
> +++ b/tests/multiboot/run_test.sh
> @@ -56,9 +56,13 @@ modules() {
> run_qemu modules.elf -initrd "module.txt,module.txt argument,module.txt"
> }
>
> +sections() {
> + run_qemu sections.elf
> +}
> +
> make all
>
> -for t in mmap modules; do
> +for t in mmap modules sections; do
>
> echo > test.log
> $t
> diff --git a/tests/multiboot/sections.c b/tests/multiboot/sections.c
> new file mode 100644
> index 0000000000..05a88f99ac
> --- /dev/null
> +++ b/tests/multiboot/sections.c
> @@ -0,0 +1,55 @@
> +/*
> + * Copyright (c) 2017 Anatol Pomozov <anatol.pomozov@gmail.com>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "libc.h"
> +#include "../../hw/i386/multiboot_header.h"
> +#include "../../include/elf.h"
> +
> +int test_main(uint32_t magic, struct multiboot_info *mbi)
> +{
> + void *p;
> + unsigned int i;
> +
> + (void) magic;
> + multiboot_elf_section_header_table_t shdr;
> +
> + printf("Multiboot header at %x, ELF section headers %s\n\n", mbi,
> + mbi->flags & MULTIBOOT_INFO_ELF_SHDR ? "present" : "don't present");
> +
> + shdr = mbi->u.elf_sec;
> + printf("Sections list with %d entries of size %d at %x, string index %d\n",
> + shdr.num, shdr.size, shdr.addr, shdr.shndx);
> +
> + const char *string_table = (char *)((Elf32_Shdr *)(uintptr_t)(shdr.addr + shdr.shndx * shdr.size))->sh_addr;
> +
> + for (i = 0, p = (void *)shdr.addr;
> + i < shdr.num;
> + i++, p += shdr.size)
> + {
> + Elf32_Shdr *sec;
> +
> + sec = (Elf32_Shdr *)p;
> + printf("Elf section name=%s addr=%lx size=%ld\n", string_table + sec->sh_name, sec->sh_addr, sec->sh_size);
> + }
> +
> + return 0;
> +}
> diff --git a/tests/multiboot/start.S b/tests/multiboot/start.S
> index 7d33959650..bd404100c2 100644
> --- a/tests/multiboot/start.S
> +++ b/tests/multiboot/start.S
> @@ -23,7 +23,7 @@
> .section multiboot
>
> #define MB_MAGIC 0x1badb002
> -#define MB_FLAGS 0x0
> +#define MB_FLAGS 0x2
> #define MB_CHECKSUM -(MB_MAGIC + MB_FLAGS)
>
> .align 4
> --
> 2.15.0.rc0.271.g36b669edcc-goog
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 3/4] multiboot: load elf sections and section headers
2017-10-18 17:22 ` Anatol Pomozov
@ 2017-10-19 9:36 ` Kevin Wolf
2017-10-31 18:38 ` Anatol Pomozov
0 siblings, 1 reply; 33+ messages in thread
From: Kevin Wolf @ 2017-10-19 9:36 UTC (permalink / raw)
To: Anatol Pomozov
Cc: QEMU Developers, Richard Henderson, Eduardo Habkost,
Paolo Bonzini, Alexander Graf
Am 18.10.2017 um 19:22 hat Anatol Pomozov geschrieben:
> Hello qemu-devs,
>
> This patchset has been posted a while ago. I would like to move
> forward with it and look at the next task (e.g. multiboot2 support in
> QEMU). Who is the multiboot code maintainer who can help to review
> this patchset and give go/no-go answer?
I think that's exactly your problem, there is nobody who feels
responsible for Multiboot support. Officially, it is part of the PC/x86
subsystems:
$ scripts/get_maintainer.pl -f hw/i386/multiboot.c
"Michael S. Tsirkin" <mst@redhat.com> (supporter:PC)
Paolo Bonzini <pbonzini@redhat.com> (maintainer:X86)
Richard Henderson <rth@twiddle.net> (maintainer:X86)
Eduardo Habkost <ehabkost@redhat.com> (maintainer:X86)
qemu-devel@nongnu.org (open list:All patches CC here)
I'm not sure if any of them know much about Multiboot. Myself, I am
personally interested in having Multiboot support, which is why I've
even commented on your patches, but I'm not the maintainer.
The last few Multiboot patches seem to have been merged by Paolo.
Kevin
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 3/4] multiboot: load elf sections and section headers
2017-10-19 9:36 ` Kevin Wolf
@ 2017-10-31 18:38 ` Anatol Pomozov
2017-11-17 21:33 ` Anatol Pomozov
0 siblings, 1 reply; 33+ messages in thread
From: Anatol Pomozov @ 2017-10-31 18:38 UTC (permalink / raw)
To: Kevin Wolf
Cc: QEMU Developers, Richard Henderson, Eduardo Habkost,
Paolo Bonzini, Alexander Graf
Hi
On Thu, Oct 19, 2017 at 2:36 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 18.10.2017 um 19:22 hat Anatol Pomozov geschrieben:
>> Hello qemu-devs,
>>
>> This patchset has been posted a while ago. I would like to move
>> forward with it and look at the next task (e.g. multiboot2 support in
>> QEMU). Who is the multiboot code maintainer who can help to review
>> this patchset and give go/no-go answer?
>
> I think that's exactly your problem, there is nobody who feels
> responsible for Multiboot support. Officially, it is part of the PC/x86
> subsystems:
In this case I would like to become the official Qemu/Multiboot
maintainer. I do development related to x86 osdev and actively use
qemu for my projects. I am interested in having a feature-rich and
robust Multiboot implementation. In mid term I see several tasks for
me:
- finish and merge my cleanup + MULTIBOOT_INFO_ELF_SHDR patch series
- add Multiboot2 support
- (optional) look at MIPS support for Multiboot. I do not have/use
MIPS but it is beneficial to add multiplatform support to Multiboot
- reviewing incoming patches
A bit about me. My name is Anatol Pomozov and at my spare time I
participate at several open sources projects. I am an Arch developer
since 2013 where I maintain a bunch of packages (including Arch qemu
package [1]). I have ~40 patches in the Linux kernel tree, where the
biggest contribution is a driver for audio codec NAU8825. I
contributed a lot of small patches to numerous projects (compilers,
toolchains, ...)
[1] https://www.archlinux.org/packages/extra/i686/qemu/
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 3/4] multiboot: load elf sections and section headers
2017-10-31 18:38 ` Anatol Pomozov
@ 2017-11-17 21:33 ` Anatol Pomozov
2017-11-20 16:45 ` Kevin Wolf
0 siblings, 1 reply; 33+ messages in thread
From: Anatol Pomozov @ 2017-11-17 21:33 UTC (permalink / raw)
To: Kevin Wolf
Cc: QEMU Developers, Richard Henderson, Eduardo Habkost,
Paolo Bonzini, Alexander Graf
Hello
On Tue, Oct 31, 2017 at 11:38 AM, Anatol Pomozov
<anatol.pomozov@gmail.com> wrote:
> Hi
>
> On Thu, Oct 19, 2017 at 2:36 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>> Am 18.10.2017 um 19:22 hat Anatol Pomozov geschrieben:
>>> Hello qemu-devs,
>>>
>>> This patchset has been posted a while ago. I would like to move
>>> forward with it and look at the next task (e.g. multiboot2 support in
>>> QEMU). Who is the multiboot code maintainer who can help to review
>>> this patchset and give go/no-go answer?
>>
>> I think that's exactly your problem, there is nobody who feels
>> responsible for Multiboot support. Officially, it is part of the PC/x86
>> subsystems:
>
> In this case I would like to become the official Qemu/Multiboot
> maintainer. I do development related to x86 osdev and actively use
> qemu for my projects. I am interested in having a feature-rich and
> robust Multiboot implementation.
It is very unfortunate to see lack of interest in improving
qemu/multiboot functionality. I guess my best option for now is to
avoid using qemu multiboot implementation and use grub instead.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 3/4] multiboot: load elf sections and section headers
2017-11-17 21:33 ` Anatol Pomozov
@ 2017-11-20 16:45 ` Kevin Wolf
0 siblings, 0 replies; 33+ messages in thread
From: Kevin Wolf @ 2017-11-20 16:45 UTC (permalink / raw)
To: Anatol Pomozov
Cc: QEMU Developers, Richard Henderson, Eduardo Habkost,
Paolo Bonzini, Alexander Graf, peter.maydell
Am 17.11.2017 um 22:33 hat Anatol Pomozov geschrieben:
> On Tue, Oct 31, 2017 at 11:38 AM, Anatol Pomozov
> <anatol.pomozov@gmail.com> wrote:
> > Hi
> >
> > On Thu, Oct 19, 2017 at 2:36 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> >> Am 18.10.2017 um 19:22 hat Anatol Pomozov geschrieben:
> >>> Hello qemu-devs,
> >>>
> >>> This patchset has been posted a while ago. I would like to move
> >>> forward with it and look at the next task (e.g. multiboot2 support in
> >>> QEMU). Who is the multiboot code maintainer who can help to review
> >>> this patchset and give go/no-go answer?
> >>
> >> I think that's exactly your problem, there is nobody who feels
> >> responsible for Multiboot support. Officially, it is part of the PC/x86
> >> subsystems:
> >
> > In this case I would like to become the official Qemu/Multiboot
> > maintainer. I do development related to x86 osdev and actively use
> > qemu for my projects. I am interested in having a feature-rich and
> > robust Multiboot implementation.
>
> It is very unfortunate to see lack of interest in improving
> qemu/multiboot functionality. I guess my best option for now is to
> avoid using qemu multiboot implementation and use grub instead.
Let's wait until 2.11 is out and we're out of freeze, and then I think
your best bet is to just prepare a pull request that adds you to
MAINTAINERS and applies the patches you created so far.
Peter, does this sound right?
Kevin
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] multiboot: Change multiboot_info from array of bytes to a C struct
2017-10-12 23:54 ` [Qemu-devel] [PATCH 1/4] multiboot: Change multiboot_info from array of bytes to a C struct Anatol Pomozov
@ 2018-01-15 14:49 ` Kevin Wolf
2018-01-29 18:21 ` Anatol Pomozov
0 siblings, 1 reply; 33+ messages in thread
From: Kevin Wolf @ 2018-01-15 14:49 UTC (permalink / raw)
To: Anatol Pomozov; +Cc: qemu-devel, ehabkost, agraf, pbonzini, rth
Am 13.10.2017 um 01:54 hat Anatol Pomozov geschrieben:
> Using C structs makes the code more readable and prevents type conversion
> errors.
>
> Borrow multiboot1 header from GRUB project.
>
> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
> ---
> hw/i386/multiboot.c | 124 +++++++++------------
> hw/i386/multiboot_header.h | 254 ++++++++++++++++++++++++++++++++++++++++++++
> tests/multiboot/mmap.c | 14 +--
> tests/multiboot/mmap.out | 10 ++
> tests/multiboot/modules.c | 12 ++-
> tests/multiboot/modules.out | 10 ++
> tests/multiboot/multiboot.h | 66 ------------
> 7 files changed, 339 insertions(+), 151 deletions(-)
> create mode 100644 hw/i386/multiboot_header.h
> delete mode 100644 tests/multiboot/multiboot.h
This is a good change in general. However, I'm not sure if we should
really replace the header used in the tests. After all, the tests also
test whether things are at the right place, and if there is an error in
the header file, we can only catch it if the tests keep using their own
definitions.
Kevin
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 2/4] multiboot: load any machine type of ELF
2017-10-16 18:38 ` Anatol Pomozov
@ 2018-01-15 14:52 ` Kevin Wolf
2018-01-29 18:35 ` Anatol Pomozov
0 siblings, 1 reply; 33+ messages in thread
From: Kevin Wolf @ 2018-01-15 14:52 UTC (permalink / raw)
To: Anatol Pomozov
Cc: Peter Maydell, Eduardo Habkost, Michael S. Tsirkin,
QEMU Developers, Alexander Graf, Paolo Bonzini,
Richard Henderson
Am 16.10.2017 um 20:38 hat Anatol Pomozov geschrieben:
> Hi
>
> On Mon, Oct 16, 2017 at 1:27 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> > Am 14.10.2017 um 15:41 hat Peter Maydell geschrieben:
> >> On 14 October 2017 at 00:21, Eduardo Habkost <ehabkost@redhat.com> wrote:
> >> > I don't believe the spec restricts that, but I don't see why it
> >> > would be useful to load an ELF file that doesn't match the target
> >> > architecture (e.g. loading non-x86 ELF files on a x86 machine
> >> > like PC).
>
> I see what do you mean Eduardo. Yes it makes sense to restrict ELF to
> the currently emulated platform.
>
> On a second thought adding multiboot support for non x86 needs to go
> with other changes, e.g. multiboot.c should be moved out of hw/i386 to
> some platform-independent location. It probably worth to postpone this
> change until after Qemu gets multiboot2 support that explicitly states
> MIPS support, thus it will be easier to test this codepath on multiple
> platforms.
>
> So if you don't mind I'll remove this patch from the current patch
> series and put it into later one.
I can't find a new version of this patch series with this patch dropped.
Are you still planning to send one?
Kevin
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 3/4] multiboot: load elf sections and section headers
2017-10-12 23:54 ` [Qemu-devel] [PATCH 3/4] multiboot: load elf sections and section headers Anatol Pomozov
2017-10-18 17:22 ` Anatol Pomozov
@ 2018-01-15 15:41 ` Kevin Wolf
2018-01-29 19:16 ` Anatol Pomozov
1 sibling, 1 reply; 33+ messages in thread
From: Kevin Wolf @ 2018-01-15 15:41 UTC (permalink / raw)
To: Anatol Pomozov; +Cc: qemu-devel, ehabkost, agraf, pbonzini, rth
Am 13.10.2017 um 01:54 hat Anatol Pomozov geschrieben:
> Multiboot may load section headers and all sections (even those that are
> not part of any segment) to target memory.
>
> Tested with an ELF application that uses data from strings table
> section.
>
> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
> ---
> hw/core/loader.c | 8 +--
> hw/i386/multiboot.c | 83 +++++++++++++-----------
> hw/s390x/ipl.c | 2 +-
> include/hw/elf_ops.h | 107 ++++++++++++++++++++++++++++++-
> include/hw/loader.h | 11 +++-
> tests/multiboot/Makefile | 8 ++-
> tests/multiboot/generate_sections_out.py | 33 ++++++++++
> tests/multiboot/modules.out | 22 +++----
> tests/multiboot/run_test.sh | 6 +-
> tests/multiboot/sections.c | 55 ++++++++++++++++
> tests/multiboot/start.S | 2 +-
> 11 files changed, 276 insertions(+), 61 deletions(-)
> create mode 100755 tests/multiboot/generate_sections_out.py
> create mode 100644 tests/multiboot/sections.c
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 4593061445..a8787f7685 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -439,7 +439,7 @@ int load_elf_as(const char *filename,
> {
> return load_elf_ram(filename, translate_fn, translate_opaque,
> pentry, lowaddr, highaddr, big_endian, elf_machine,
> - clear_lsb, data_swab, as, true);
> + clear_lsb, data_swab, as, true, NULL);
> }
>
> /* return < 0 if error, otherwise the number of bytes loaded in memory */
> @@ -448,7 +448,7 @@ int load_elf_ram(const char *filename,
> void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
> uint64_t *highaddr, int big_endian, int elf_machine,
> int clear_lsb, int data_swab, AddressSpace *as,
> - bool load_rom)
> + bool load_rom, SectionsData *sections)
> {
> int fd, data_order, target_data_order, must_swab, ret = ELF_LOAD_FAILED;
> uint8_t e_ident[EI_NIDENT];
> @@ -488,11 +488,11 @@ int load_elf_ram(const char *filename,
> if (e_ident[EI_CLASS] == ELFCLASS64) {
> ret = load_elf64(filename, fd, translate_fn, translate_opaque, must_swab,
> pentry, lowaddr, highaddr, elf_machine, clear_lsb,
> - data_swab, as, load_rom);
> + data_swab, as, load_rom, sections);
> } else {
> ret = load_elf32(filename, fd, translate_fn, translate_opaque, must_swab,
> pentry, lowaddr, highaddr, elf_machine, clear_lsb,
> - data_swab, as, load_rom);
> + data_swab, as, load_rom, sections);
> }
>
> fail:
> diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
> index 7dacd6d827..841d05160a 100644
> --- a/hw/i386/multiboot.c
> +++ b/hw/i386/multiboot.c
> @@ -125,7 +125,7 @@ int load_multiboot(FWCfgState *fw_cfg,
> {
> int i;
> bool is_multiboot = false;
> - uint32_t flags = 0;
> + uint32_t flags = 0, bootinfo_flags = 0;
> uint32_t mh_entry_addr;
> uint32_t mh_load_addr;
> uint32_t mb_kernel_size;
> @@ -134,6 +134,7 @@ int load_multiboot(FWCfgState *fw_cfg,
> uint8_t *mb_bootinfo_data;
> uint32_t cmdline_len;
> struct multiboot_header *multiboot_header;
> + SectionsData sections;
>
> /* Ok, let's see if it is a multiboot image.
> The header is 12x32bit long, so the latest entry may be 8192 - 48. */
> @@ -161,37 +162,8 @@ int load_multiboot(FWCfgState *fw_cfg,
> if (flags & MULTIBOOT_VIDEO_MODE) {
> fprintf(stderr, "qemu: multiboot knows VBE. we don't.\n");
> }
> - if (!(flags & MULTIBOOT_AOUT_KLUDGE)) {
> - uint64_t elf_entry;
> - uint64_t elf_low, elf_high;
> - int kernel_size;
> - fclose(f);
>
> - if (((struct elf64_hdr*)header)->e_machine == EM_X86_64) {
> - fprintf(stderr, "Cannot load x86-64 image, give a 32bit one.\n");
> - exit(1);
> - }
I'm not sure why you're reversing the condition and moving this branch
down, but in the code movements the EM_X86_64 check got lost. Please
keep it, we don't support 64 bit ELFs at the moment. If you want to
change this, it should be a separate patch.
> - kernel_size = load_elf(kernel_filename, NULL, NULL, &elf_entry,
> - &elf_low, &elf_high, 0, EM_NONE,
> - 0, 0);
> - if (kernel_size < 0) {
> - fprintf(stderr, "Error while loading elf kernel\n");
> - exit(1);
> - }
> - mh_load_addr = elf_low;
> - mb_kernel_size = elf_high - elf_low;
> - mh_entry_addr = elf_entry;
> -
> - mbs.mb_buf = g_malloc(mb_kernel_size);
> - if (rom_copy(mbs.mb_buf, mh_load_addr, mb_kernel_size) != mb_kernel_size) {
> - fprintf(stderr, "Error while fetching elf kernel from rom\n");
> - exit(1);
> - }
> -
> - mb_debug("qemu: loading multiboot-elf kernel (%#x bytes) with entry %#zx\n",
> - mb_kernel_size, (size_t)mh_entry_addr);
> - } else {
> + if (flags & MULTIBOOT_AOUT_KLUDGE) {
> /* Valid if mh_flags sets MULTIBOOT_AOUT_KLUDGE. */
> uint32_t mh_header_addr = ldl_p(&multiboot_header->header_addr);
> uint32_t mh_load_end_addr = ldl_p(&multiboot_header->load_end_addr);
> @@ -228,12 +200,6 @@ int load_multiboot(FWCfgState *fw_cfg,
> mb_load_size = mb_kernel_size;
> }
>
> - /* Valid if mh_flags sets MULTIBOOT_VIDEO_MODE.
> - uint32_t mh_mode_type = ldl_p(&multiboot_header->mode_type);
> - uint32_t mh_width = ldl_p(&multiboot_header->width);
> - uint32_t mh_height = ldl_p(&multiboot_header->height);
> - uint32_t mh_depth = ldl_p(&multiboot_header->depth); */
> -
> mb_debug("multiboot: mh_header_addr = %#x\n", mh_header_addr);
> mb_debug("multiboot: mh_load_addr = %#x\n", mh_load_addr);
> mb_debug("multiboot: mh_load_end_addr = %#x\n", mh_load_end_addr);
> @@ -248,9 +214,45 @@ int load_multiboot(FWCfgState *fw_cfg,
> exit(1);
> }
> memset(mbs.mb_buf + mb_load_size, 0, mb_kernel_size - mb_load_size);
> - fclose(f);
> + } else {
> + uint64_t elf_entry;
> + uint64_t elf_low, elf_high;
> + int kernel_size;
> +
> + kernel_size = load_elf_ram(kernel_filename, NULL, NULL, &elf_entry,
> + &elf_low, &elf_high, 0, I386_ELF_MACHINE,
> + 0, 0, NULL, true, §ions);
> + if (kernel_size < 0) {
> + fprintf(stderr, "Error while loading elf kernel\n");
> + exit(1);
> + }
> + mh_load_addr = elf_low;
> + mb_kernel_size = elf_high - elf_low;
> + mh_entry_addr = elf_entry;
> +
> + mbs.mb_buf = g_malloc(mb_kernel_size);
> + if (rom_copy(mbs.mb_buf, mh_load_addr, mb_kernel_size) != mb_kernel_size) {
> + fprintf(stderr, "Error while fetching elf kernel from rom\n");
> + exit(1);
> + }
> +
> + mb_debug("qemu: loading multiboot-elf kernel (%#x bytes) with entry %#zx\n",
> + mb_kernel_size, (size_t)mh_entry_addr);
> +
> + stl_p(&bootinfo.u.elf_sec.num, sections.num);
> + stl_p(&bootinfo.u.elf_sec.size, sections.size);
> + stl_p(&bootinfo.u.elf_sec.addr, sections.addr);
> + stl_p(&bootinfo.u.elf_sec.shndx, sections.shndx);
> +
> + bootinfo_flags |= MULTIBOOT_INFO_ELF_SHDR;
> }
>
> + /* Valid if mh_flags sets MULTIBOOT_VIDEO_MODE.
> + uint32_t mh_mode_type = ldl_p(&multiboot_header->mode_type);
> + uint32_t mh_width = ldl_p(&multiboot_header->width);
> + uint32_t mh_height = ldl_p(&multiboot_header->height);
> + uint32_t mh_depth = ldl_p(&multiboot_header->depth); */
> +
> mbs.mb_buf_phys = mh_load_addr;
>
> mbs.mb_buf_size = TARGET_PAGE_ALIGN(mb_kernel_size);
> @@ -332,7 +334,8 @@ int load_multiboot(FWCfgState *fw_cfg,
> stl_p(&bootinfo.mods_count, mbs.mb_mods_count); /* mods_count */
>
> /* the kernel is where we want it to be now */
> - stl_p(&bootinfo.flags, MULTIBOOT_INFO_MEMORY
> + stl_p(&bootinfo.flags, bootinfo_flags
> + | MULTIBOOT_INFO_MEMORY
> | MULTIBOOT_INFO_BOOTDEV
> | MULTIBOOT_INFO_CMDLINE
> | MULTIBOOT_INFO_MODS
> @@ -365,5 +368,7 @@ int load_multiboot(FWCfgState *fw_cfg,
> option_rom[nb_option_roms].bootindex = 0;
> nb_option_roms++;
>
> + fclose(f);
> +
> return 1; /* yes, we are multiboot */
> }
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 0d06fc12b6..4d9cc6261a 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -327,7 +327,7 @@ static int load_netboot_image(Error **errp)
> }
>
> img_size = load_elf_ram(netboot_filename, NULL, NULL, &ipl->start_addr,
> - NULL, NULL, 1, EM_S390, 0, 0, NULL, false);
> + NULL, NULL, 1, EM_S390, 0, 0, NULL, false, NULL);
>
> if (img_size < 0) {
> img_size = load_image_size(netboot_filename, ram_ptr, ram_size);
> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
> index d192e7e2a3..7a7f7983a4 100644
> --- a/include/hw/elf_ops.h
> +++ b/include/hw/elf_ops.h
The code in this file is rather old and not quite compliant with the
QEMU coding style. This is not an excuse not to apply the correct coding
style to new additions to the file:
> @@ -264,12 +264,13 @@ static int glue(load_elf, SZ)(const char *name, int fd,
> int must_swab, uint64_t *pentry,
> uint64_t *lowaddr, uint64_t *highaddr,
> int elf_machine, int clear_lsb, int data_swab,
> - AddressSpace *as, bool load_rom)
> + AddressSpace *as, bool load_rom,
> + SectionsData *sections)
> {
> struct elfhdr ehdr;
> struct elf_phdr *phdr = NULL, *ph;
> int size, i, total_size;
> - elf_word mem_size, file_size;
> + elf_word mem_size, file_size, sec_size;
> uint64_t addr, low = (uint64_t)-1, high = 0;
> uint8_t *data = NULL;
> char label[128];
> @@ -481,6 +482,108 @@ static int glue(load_elf, SZ)(const char *name, int fd,
> }
> }
> g_free(phdr);
> +
> + if (sections) {
> + struct elf_shdr *shdr = NULL, *sh;
> + int shsize;
> +
> + // user requested loading all ELF sections
Please use C-style comments: /* ... */
> + shsize = ehdr.e_shnum * sizeof(shdr[0]);
> + if (lseek(fd, ehdr.e_shoff, SEEK_SET) != ehdr.e_shoff) {
> + goto fail;
> + }
> + shdr = g_malloc0(shsize);
> + if (!shdr)
> + goto fail;
g_malloc0() never returns NULL. If you want it to return NULL instead of
aborting qemu when the allocation fails, you need g_try_malloc0().
Also, the QEMU coding style requires braces after an if condition. I'm
mentioning this only here, but it applies to multiple places in the
whole patch.
> + if (read(fd, shdr, shsize) != shsize)
> + goto fail;
> + if (must_swab) {
> + for (i = 0; i < ehdr.e_shnum; i++) {
> + sh = &shdr[i];
> + glue(bswap_shdr, SZ)(sh);
> + }
> + }
> +
> + for(i = 0; i < ehdr.e_shnum; i++) {
A space character is required between for and the bracket.
> + sh = &shdr[i];
> + if (sh->sh_type == SHT_NULL)
> + continue;
> + // if section has address then it is loaded (XXX: is it true?), no need to load it again
This exceeds the limit of 80 characters per line.
> + if (sh->sh_addr)
> + continue;
> +
> + // append section data at the end of the loaded segments
> + addr = ROUND_UP(high, sh->sh_addralign);
> + sh->sh_addr = addr;
> +
> + sec_size = sh->sh_size; /* Size of the section data */
> + data = g_malloc0(sec_size);
> + if (sec_size > 0) {
> + if (lseek(fd, sh->sh_offset, SEEK_SET) < 0) {
> + goto fail;
> + }
> + if (read(fd, data, sec_size) != sec_size) {
> + goto fail;
> + }
> + }
> +
> + // As these sections are not loadable no need to perform reloaction
> + // using translate_fn()
> +
> + if (data_swab) {
> + int j;
> + for (j = 0; j < sec_size; j += (1 << data_swab)) {
Is sec_size guaranteed to be aligned? I don't see a check to verify
this. Otherwise, could we call bswap64() on the last byte of section,
accessing seven more bytes outside the allocated buffer?
> + uint8_t *dp = data + j;
> + switch (data_swab) {
> + case (1):
Why 'case (1):' instead of 'case 1:' looks odd.
> + *(uint16_t *)dp = bswap16(*(uint16_t *)dp);
> + break;
> + case (2):
> + *(uint32_t *)dp = bswap32(*(uint32_t *)dp);
> + break;
> + case (3):
> + *(uint64_t *)dp = bswap64(*(uint64_t *)dp);
> + break;
> + default:
> + g_assert_not_reached();
> + }
> + }
> + }
> +
> + if (load_rom) {
> + snprintf(label, sizeof(label), "shdr #%d: %s", i, name);
> +
> + /* rom_add_elf_program() seize the ownership of 'data' */
> + rom_add_elf_program(label, data, sec_size, sec_size, addr, as);
> + } else {
> + cpu_physical_memory_write(addr, data, sec_size);
> + g_free(data);
> + }
> +
> + total_size += sec_size;
> + high = addr + sec_size;
> +
> + data = NULL;
> + }
> +
> + sections->num = ehdr.e_shnum;
> + sections->size = ehdr.e_shentsize;
> + sections->addr = high; // Address where we load the sections header
> + sections->shndx = ehdr.e_shstrndx;
> +
> + // Append section headers at the end of loaded segments/sections
> + if (load_rom) {
> + snprintf(label, sizeof(label), "shdrs");
> +
> + /* rom_add_elf_program() seize the ownership of 'shdr' */
> + rom_add_elf_program(label, shdr, shsize, shsize, high, as);
> + } else {
> + cpu_physical_memory_write(high, shdr, shsize);
> + g_free(shdr);
> + }
> + high += shsize;
> + }
> +
> if (lowaddr)
> *lowaddr = (uint64_t)(elf_sword)low;
> if (highaddr)
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 355fe0f5a2..3cf2d6da0f 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -65,6 +65,13 @@ int load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz);
> #define ELF_LOAD_WRONG_ENDIAN -4
> const char *load_elf_strerror(int error);
>
> +typedef struct {
> + uint32_t num; // number of loaded sections
> + uint32_t size; // size of entry in sections header list
> + uint32_t addr; // address of loaded sections header
> + uint32_t shndx; // number of section that contains string table
> +} SectionsData;
> +
> /** load_elf_ram:
> * @filename: Path of ELF file
> * @translate_fn: optional function to translate load addresses
> @@ -82,6 +89,8 @@ const char *load_elf_strerror(int error);
> * @as: The AddressSpace to load the ELF to. The value of address_space_memory
> * is used if nothing is supplied here.
> * @load_rom : Load ELF binary as ROM
> + * @sections: If parameter is non-NULL then all ELF sections loaded into memory
> + * and this structure is populated.
> *
> * Load an ELF file's contents to the emulated system's address space.
> * Clients may optionally specify a callback to perform address
> @@ -99,7 +108,7 @@ int load_elf_ram(const char *filename,
> void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
> uint64_t *highaddr, int big_endian, int elf_machine,
> int clear_lsb, int data_swab, AddressSpace *as,
> - bool load_rom);
> + bool load_rom, SectionsData *sections);
>
> /** load_elf_as:
> * Same as load_elf_ram(), but always loads the elf as ROM
> diff --git a/tests/multiboot/Makefile b/tests/multiboot/Makefile
> index 36f01dc647..b6a5056347 100644
> --- a/tests/multiboot/Makefile
> +++ b/tests/multiboot/Makefile
> @@ -6,7 +6,7 @@ LD=ld
> LDFLAGS=-melf_i386 -T link.ld
> LIBS=$(shell $(CC) $(CCFLAGS) -print-libgcc-file-name)
>
> -all: mmap.elf modules.elf
> +all: mmap.elf modules.elf sections.elf sections.out
>
> mmap.elf: start.o mmap.o libc.o
> $(LD) $(LDFLAGS) -o $@ $^ $(LIBS)
> @@ -14,6 +14,12 @@ mmap.elf: start.o mmap.o libc.o
> modules.elf: start.o modules.o libc.o
> $(LD) $(LDFLAGS) -o $@ $^ $(LIBS)
>
> +sections.elf: start.o sections.o libc.o
> + $(LD) $(LDFLAGS) -o $@ $^ $(LIBS)
> +
> +sections.out: sections.elf generate_sections_out.py
> + python2 generate_sections_out.py $^ > $@
> +
> %.o: %.c
> $(CC) $(CCFLAGS) -c -o $@ $^
>
> diff --git a/tests/multiboot/generate_sections_out.py b/tests/multiboot/generate_sections_out.py
> new file mode 100755
> index 0000000000..8077856823
> --- /dev/null
> +++ b/tests/multiboot/generate_sections_out.py
> @@ -0,0 +1,33 @@
> +#!/usr/bin/env python2
> +
> +import sys
> +
> +from elftools.elf.elffile import ELFFile
I don't think we can expect this module to be present. For example, on
my system, attempting to run the test fails:
ImportError: No module named elftools.elf.elffile
Maybe we can parse the output of readelf instead?
> +def roundup(num, align):
> + return (num + align - 1) / align * align
> +
> +def main(filename):
> + print "\n\n\n=== Running test case: sections.elf ===\n"
> + print "Multiboot header at 9500, ELF section headers present\n"
> +
> + with open(filename, 'rb') as f:
> + elf = ELFFile(f)
> + header = elf.header
> + load_addr = 0x100000 # we load image starting from 1M
> + sections = ""
> + for s in elf.iter_sections():
> + align = s.header.sh_addralign
> + addr = 0
> + if s.header.sh_type != 'SHT_NULL':
> + addr = s.header.sh_addr
> + if addr == 0:
> + addr = roundup(load_addr, s.header.sh_addralign)
> + load_addr = addr + s.header.sh_size
> + sections += "Elf section name=%s addr=%x size=%d\n" % (s.name, addr, s.header.sh_size)
> +
> + print "Sections list with %d entries of size %d at %x, string index %d" % (header.e_shnum, header.e_shentsize, load_addr, header.e_shstrndx)
We're not as strict about the 80 characters rule in Python code, but I
think this line could use being wrapped.
> + print sections,
> +
> +if __name__ == '__main__':
> + main(sys.argv[1])
> diff --git a/tests/multiboot/modules.out b/tests/multiboot/modules.out
> index 0da7b39374..b6c1a01be1 100644
> --- a/tests/multiboot/modules.out
> +++ b/tests/multiboot/modules.out
> @@ -5,15 +5,15 @@
>
> Multiboot header at 9500
>
> -Module list with 0 entries at 102000
> +Module list with 0 entries at 104000
>
>
> === Running test case: modules.elf -initrd module.txt ===
>
> Multiboot header at 9500
>
> -Module list with 1 entries at 102000
> -[102000] Module: 103000 - 103038 (56 bytes) 'module.txt'
> +Module list with 1 entries at 104000
> +[104000] Module: 105000 - 105038 (56 bytes) 'module.txt'
> Content: 'This is a test file that is used as a multiboot module.'
>
>
> @@ -21,8 +21,8 @@ Module list with 1 entries at 102000
>
> Multiboot header at 9500
>
> -Module list with 1 entries at 102000
> -[102000] Module: 103000 - 103038 (56 bytes) 'module.txt argument'
> +Module list with 1 entries at 104000
> +[104000] Module: 105000 - 105038 (56 bytes) 'module.txt argument'
> Content: 'This is a test file that is used as a multiboot module.'
>
>
> @@ -30,8 +30,8 @@ Module list with 1 entries at 102000
>
> Multiboot header at 9500
>
> -Module list with 1 entries at 102000
> -[102000] Module: 103000 - 103038 (56 bytes) 'module.txt argument,with,commas'
> +Module list with 1 entries at 104000
> +[104000] Module: 105000 - 105038 (56 bytes) 'module.txt argument,with,commas'
> Content: 'This is a test file that is used as a multiboot module.'
>
>
> @@ -39,10 +39,10 @@ Module list with 1 entries at 102000
>
> Multiboot header at 9500
>
> -Module list with 3 entries at 102000
> -[102000] Module: 103000 - 103038 (56 bytes) 'module.txt'
> +Module list with 3 entries at 104000
> +[104000] Module: 105000 - 105038 (56 bytes) 'module.txt'
> Content: 'This is a test file that is used as a multiboot module.'
> -[102010] Module: 104000 - 104038 (56 bytes) 'module.txt argument'
> +[104010] Module: 106000 - 106038 (56 bytes) 'module.txt argument'
> Content: 'This is a test file that is used as a multiboot module.'
> -[102020] Module: 105000 - 105038 (56 bytes) 'module.txt'
> +[104020] Module: 107000 - 107038 (56 bytes) 'module.txt'
> Content: 'This is a test file that is used as a multiboot module.'
> diff --git a/tests/multiboot/run_test.sh b/tests/multiboot/run_test.sh
> index 0278148b43..f04e35cbf0 100755
> --- a/tests/multiboot/run_test.sh
> +++ b/tests/multiboot/run_test.sh
> @@ -56,9 +56,13 @@ modules() {
> run_qemu modules.elf -initrd "module.txt,module.txt argument,module.txt"
> }
>
> +sections() {
> + run_qemu sections.elf
> +}
> +
> make all
>
> -for t in mmap modules; do
> +for t in mmap modules sections; do
>
> echo > test.log
> $t
> diff --git a/tests/multiboot/sections.c b/tests/multiboot/sections.c
> new file mode 100644
> index 0000000000..05a88f99ac
> --- /dev/null
> +++ b/tests/multiboot/sections.c
> @@ -0,0 +1,55 @@
> +/*
> + * Copyright (c) 2017 Anatol Pomozov <anatol.pomozov@gmail.com>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "libc.h"
> +#include "../../hw/i386/multiboot_header.h"
> +#include "../../include/elf.h"
> +
> +int test_main(uint32_t magic, struct multiboot_info *mbi)
> +{
> + void *p;
> + unsigned int i;
> +
> + (void) magic;
> + multiboot_elf_section_header_table_t shdr;
> +
> + printf("Multiboot header at %x, ELF section headers %s\n\n", mbi,
> + mbi->flags & MULTIBOOT_INFO_ELF_SHDR ? "present" : "don't present");
> +
> + shdr = mbi->u.elf_sec;
> + printf("Sections list with %d entries of size %d at %x, string index %d\n",
> + shdr.num, shdr.size, shdr.addr, shdr.shndx);
> +
> + const char *string_table = (char *)((Elf32_Shdr *)(uintptr_t)(shdr.addr + shdr.shndx * shdr.size))->sh_addr;
80 characters again.
> +
> + for (i = 0, p = (void *)shdr.addr;
> + i < shdr.num;
> + i++, p += shdr.size)
> + {
> + Elf32_Shdr *sec;
> +
> + sec = (Elf32_Shdr *)p;
> + printf("Elf section name=%s addr=%lx size=%ld\n", string_table + sec->sh_name, sec->sh_addr, sec->sh_size);
And here.
> + }
> +
> + return 0;
> +}
Should we try to access one of the section headers to make sure that we
didn't only get the correct addresses, but that data is actually loaded
there?
Kevin
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] multiboot: Change multiboot_info from array of bytes to a C struct
2018-01-15 14:49 ` Kevin Wolf
@ 2018-01-29 18:21 ` Anatol Pomozov
2018-01-29 20:02 ` Kevin Wolf
0 siblings, 1 reply; 33+ messages in thread
From: Anatol Pomozov @ 2018-01-29 18:21 UTC (permalink / raw)
To: Kevin Wolf
Cc: QEMU Developers, Eduardo Habkost, Alexander Graf, Paolo Bonzini,
Richard Henderson
Hi
On Mon, Jan 15, 2018 at 6:49 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 13.10.2017 um 01:54 hat Anatol Pomozov geschrieben:
>> Using C structs makes the code more readable and prevents type conversion
>> errors.
>>
>> Borrow multiboot1 header from GRUB project.
>>
>> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
>> ---
>> hw/i386/multiboot.c | 124 +++++++++------------
>> hw/i386/multiboot_header.h | 254 ++++++++++++++++++++++++++++++++++++++++++++
>> tests/multiboot/mmap.c | 14 +--
>> tests/multiboot/mmap.out | 10 ++
>> tests/multiboot/modules.c | 12 ++-
>> tests/multiboot/modules.out | 10 ++
>> tests/multiboot/multiboot.h | 66 ------------
>> 7 files changed, 339 insertions(+), 151 deletions(-)
>> create mode 100644 hw/i386/multiboot_header.h
>> delete mode 100644 tests/multiboot/multiboot.h
>
> This is a good change in general. However, I'm not sure if we should
> really replace the header used in the tests. After all, the tests also
> test whether things are at the right place, and if there is an error in
> the header file, we can only catch it if the tests keep using their own
> definitions.
The added multibooh.h is from GRUB - the developers of multiboot. I
think we can trust it. Having a single header will make future tests
maintainence easier.
But if you feel strongly that qemu tests should use it's own version
of multiboot header then I can add it back.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 2/4] multiboot: load any machine type of ELF
2018-01-15 14:52 ` Kevin Wolf
@ 2018-01-29 18:35 ` Anatol Pomozov
0 siblings, 0 replies; 33+ messages in thread
From: Anatol Pomozov @ 2018-01-29 18:35 UTC (permalink / raw)
To: Kevin Wolf
Cc: Peter Maydell, Eduardo Habkost, Michael S. Tsirkin,
QEMU Developers, Alexander Graf, Paolo Bonzini,
Richard Henderson
Hi
On Mon, Jan 15, 2018 at 6:52 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 16.10.2017 um 20:38 hat Anatol Pomozov geschrieben:
>> Hi
>>
>> On Mon, Oct 16, 2017 at 1:27 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>> > Am 14.10.2017 um 15:41 hat Peter Maydell geschrieben:
>> >> On 14 October 2017 at 00:21, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> >> > I don't believe the spec restricts that, but I don't see why it
>> >> > would be useful to load an ELF file that doesn't match the target
>> >> > architecture (e.g. loading non-x86 ELF files on a x86 machine
>> >> > like PC).
>>
>> I see what do you mean Eduardo. Yes it makes sense to restrict ELF to
>> the currently emulated platform.
>>
>> On a second thought adding multiboot support for non x86 needs to go
>> with other changes, e.g. multiboot.c should be moved out of hw/i386 to
>> some platform-independent location. It probably worth to postpone this
>> change until after Qemu gets multiboot2 support that explicitly states
>> MIPS support, thus it will be easier to test this codepath on multiple
>> platforms.
>>
>> So if you don't mind I'll remove this patch from the current patch
>> series and put it into later one.
>
> I can't find a new version of this patch series with this patch dropped.
> Are you still planning to send one?
Yes I dropped it. It belongs to future multiboot refactoring patch series.
> The spec isn't really explicit about it being a requirement, but it does
> say that its target are 32-bit OSes on PCs, and it defines the boot
> state in terms of i386 registers, so it doesn't make sense for non-x86.
>From my reading of multiboot specification it says that boot state is
32-bit. Thus OS image should make sure its entry point is 32bit code.
But specification does not say that ELF should not contain 64bit code.
GRUB project, the reference multiboot implementation, supports 64bit
ELF.
> From my interpretation of the spec, even support for 64-bit ELFs seems
> to be (implicitly) out of spec (there is one place where it even says
> "refer to the i386 ELF documentation for details"), but if GRUB
> implements it...
Yes, GRUB supports it from its early days. One reason why I sent this
patch is to make QEMU multiboot functionality compatible with GRUB. So
the same binary can work in emulator and at real hardware.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 3/4] multiboot: load elf sections and section headers
2018-01-15 15:41 ` Kevin Wolf
@ 2018-01-29 19:16 ` Anatol Pomozov
0 siblings, 0 replies; 33+ messages in thread
From: Anatol Pomozov @ 2018-01-29 19:16 UTC (permalink / raw)
To: Kevin Wolf
Cc: QEMU Developers, Eduardo Habkost, Alexander Graf, Paolo Bonzini,
Richard Henderson
Hi
On Mon, Jan 15, 2018 at 7:41 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 13.10.2017 um 01:54 hat Anatol Pomozov geschrieben:
>> Multiboot may load section headers and all sections (even those that are
>> not part of any segment) to target memory.
>>
>> Tested with an ELF application that uses data from strings table
>> section.
>>
>> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
>> ---
>> hw/core/loader.c | 8 +--
>> hw/i386/multiboot.c | 83 +++++++++++++-----------
>> hw/s390x/ipl.c | 2 +-
>> include/hw/elf_ops.h | 107 ++++++++++++++++++++++++++++++-
>> include/hw/loader.h | 11 +++-
>> tests/multiboot/Makefile | 8 ++-
>> tests/multiboot/generate_sections_out.py | 33 ++++++++++
>> tests/multiboot/modules.out | 22 +++----
>> tests/multiboot/run_test.sh | 6 +-
>> tests/multiboot/sections.c | 55 ++++++++++++++++
>> tests/multiboot/start.S | 2 +-
>> 11 files changed, 276 insertions(+), 61 deletions(-)
>> create mode 100755 tests/multiboot/generate_sections_out.py
>> create mode 100644 tests/multiboot/sections.c
>>
>> diff --git a/hw/core/loader.c b/hw/core/loader.c
>> index 4593061445..a8787f7685 100644
>> --- a/hw/core/loader.c
>> +++ b/hw/core/loader.c
>> @@ -439,7 +439,7 @@ int load_elf_as(const char *filename,
>> {
>> return load_elf_ram(filename, translate_fn, translate_opaque,
>> pentry, lowaddr, highaddr, big_endian, elf_machine,
>> - clear_lsb, data_swab, as, true);
>> + clear_lsb, data_swab, as, true, NULL);
>> }
>>
>> /* return < 0 if error, otherwise the number of bytes loaded in memory */
>> @@ -448,7 +448,7 @@ int load_elf_ram(const char *filename,
>> void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
>> uint64_t *highaddr, int big_endian, int elf_machine,
>> int clear_lsb, int data_swab, AddressSpace *as,
>> - bool load_rom)
>> + bool load_rom, SectionsData *sections)
>> {
>> int fd, data_order, target_data_order, must_swab, ret = ELF_LOAD_FAILED;
>> uint8_t e_ident[EI_NIDENT];
>> @@ -488,11 +488,11 @@ int load_elf_ram(const char *filename,
>> if (e_ident[EI_CLASS] == ELFCLASS64) {
>> ret = load_elf64(filename, fd, translate_fn, translate_opaque, must_swab,
>> pentry, lowaddr, highaddr, elf_machine, clear_lsb,
>> - data_swab, as, load_rom);
>> + data_swab, as, load_rom, sections);
>> } else {
>> ret = load_elf32(filename, fd, translate_fn, translate_opaque, must_swab,
>> pentry, lowaddr, highaddr, elf_machine, clear_lsb,
>> - data_swab, as, load_rom);
>> + data_swab, as, load_rom, sections);
>> }
>>
>> fail:
>> diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
>> index 7dacd6d827..841d05160a 100644
>> --- a/hw/i386/multiboot.c
>> +++ b/hw/i386/multiboot.c
>> @@ -125,7 +125,7 @@ int load_multiboot(FWCfgState *fw_cfg,
>> {
>> int i;
>> bool is_multiboot = false;
>> - uint32_t flags = 0;
>> + uint32_t flags = 0, bootinfo_flags = 0;
>> uint32_t mh_entry_addr;
>> uint32_t mh_load_addr;
>> uint32_t mb_kernel_size;
>> @@ -134,6 +134,7 @@ int load_multiboot(FWCfgState *fw_cfg,
>> uint8_t *mb_bootinfo_data;
>> uint32_t cmdline_len;
>> struct multiboot_header *multiboot_header;
>> + SectionsData sections;
>>
>> /* Ok, let's see if it is a multiboot image.
>> The header is 12x32bit long, so the latest entry may be 8192 - 48. */
>> @@ -161,37 +162,8 @@ int load_multiboot(FWCfgState *fw_cfg,
>> if (flags & MULTIBOOT_VIDEO_MODE) {
>> fprintf(stderr, "qemu: multiboot knows VBE. we don't.\n");
>> }
>> - if (!(flags & MULTIBOOT_AOUT_KLUDGE)) {
>> - uint64_t elf_entry;
>> - uint64_t elf_low, elf_high;
>> - int kernel_size;
>> - fclose(f);
>>
>> - if (((struct elf64_hdr*)header)->e_machine == EM_X86_64) {
>> - fprintf(stderr, "Cannot load x86-64 image, give a 32bit one.\n");
>> - exit(1);
>> - }
>
> I'm not sure why you're reversing the condition and moving this branch
> down, but in the code movements the EM_X86_64 check got lost. Please
> keep it, we don't support 64 bit ELFs at the moment. If you want to
> change this, it should be a separate patch.
Ok I moved it to a separate patch.
>
>> - kernel_size = load_elf(kernel_filename, NULL, NULL, &elf_entry,
>> - &elf_low, &elf_high, 0, EM_NONE,
>> - 0, 0);
>> - if (kernel_size < 0) {
>> - fprintf(stderr, "Error while loading elf kernel\n");
>> - exit(1);
>> - }
>> - mh_load_addr = elf_low;
>> - mb_kernel_size = elf_high - elf_low;
>> - mh_entry_addr = elf_entry;
>> -
>> - mbs.mb_buf = g_malloc(mb_kernel_size);
>> - if (rom_copy(mbs.mb_buf, mh_load_addr, mb_kernel_size) != mb_kernel_size) {
>> - fprintf(stderr, "Error while fetching elf kernel from rom\n");
>> - exit(1);
>> - }
>> -
>> - mb_debug("qemu: loading multiboot-elf kernel (%#x bytes) with entry %#zx\n",
>> - mb_kernel_size, (size_t)mh_entry_addr);
>> - } else {
>> + if (flags & MULTIBOOT_AOUT_KLUDGE) {
>> /* Valid if mh_flags sets MULTIBOOT_AOUT_KLUDGE. */
>> uint32_t mh_header_addr = ldl_p(&multiboot_header->header_addr);
>> uint32_t mh_load_end_addr = ldl_p(&multiboot_header->load_end_addr);
>> @@ -228,12 +200,6 @@ int load_multiboot(FWCfgState *fw_cfg,
>> mb_load_size = mb_kernel_size;
>> }
>>
>> - /* Valid if mh_flags sets MULTIBOOT_VIDEO_MODE.
>> - uint32_t mh_mode_type = ldl_p(&multiboot_header->mode_type);
>> - uint32_t mh_width = ldl_p(&multiboot_header->width);
>> - uint32_t mh_height = ldl_p(&multiboot_header->height);
>> - uint32_t mh_depth = ldl_p(&multiboot_header->depth); */
>> -
>> mb_debug("multiboot: mh_header_addr = %#x\n", mh_header_addr);
>> mb_debug("multiboot: mh_load_addr = %#x\n", mh_load_addr);
>> mb_debug("multiboot: mh_load_end_addr = %#x\n", mh_load_end_addr);
>> @@ -248,9 +214,45 @@ int load_multiboot(FWCfgState *fw_cfg,
>> exit(1);
>> }
>> memset(mbs.mb_buf + mb_load_size, 0, mb_kernel_size - mb_load_size);
>> - fclose(f);
>> + } else {
>> + uint64_t elf_entry;
>> + uint64_t elf_low, elf_high;
>> + int kernel_size;
>> +
>> + kernel_size = load_elf_ram(kernel_filename, NULL, NULL, &elf_entry,
>> + &elf_low, &elf_high, 0, I386_ELF_MACHINE,
>> + 0, 0, NULL, true, §ions);
>> + if (kernel_size < 0) {
>> + fprintf(stderr, "Error while loading elf kernel\n");
>> + exit(1);
>> + }
>> + mh_load_addr = elf_low;
>> + mb_kernel_size = elf_high - elf_low;
>> + mh_entry_addr = elf_entry;
>> +
>> + mbs.mb_buf = g_malloc(mb_kernel_size);
>> + if (rom_copy(mbs.mb_buf, mh_load_addr, mb_kernel_size) != mb_kernel_size) {
>> + fprintf(stderr, "Error while fetching elf kernel from rom\n");
>> + exit(1);
>> + }
>> +
>> + mb_debug("qemu: loading multiboot-elf kernel (%#x bytes) with entry %#zx\n",
>> + mb_kernel_size, (size_t)mh_entry_addr);
>> +
>> + stl_p(&bootinfo.u.elf_sec.num, sections.num);
>> + stl_p(&bootinfo.u.elf_sec.size, sections.size);
>> + stl_p(&bootinfo.u.elf_sec.addr, sections.addr);
>> + stl_p(&bootinfo.u.elf_sec.shndx, sections.shndx);
>> +
>> + bootinfo_flags |= MULTIBOOT_INFO_ELF_SHDR;
>> }
>>
>> + /* Valid if mh_flags sets MULTIBOOT_VIDEO_MODE.
>> + uint32_t mh_mode_type = ldl_p(&multiboot_header->mode_type);
>> + uint32_t mh_width = ldl_p(&multiboot_header->width);
>> + uint32_t mh_height = ldl_p(&multiboot_header->height);
>> + uint32_t mh_depth = ldl_p(&multiboot_header->depth); */
>> +
>> mbs.mb_buf_phys = mh_load_addr;
>>
>> mbs.mb_buf_size = TARGET_PAGE_ALIGN(mb_kernel_size);
>> @@ -332,7 +334,8 @@ int load_multiboot(FWCfgState *fw_cfg,
>> stl_p(&bootinfo.mods_count, mbs.mb_mods_count); /* mods_count */
>>
>> /* the kernel is where we want it to be now */
>> - stl_p(&bootinfo.flags, MULTIBOOT_INFO_MEMORY
>> + stl_p(&bootinfo.flags, bootinfo_flags
>> + | MULTIBOOT_INFO_MEMORY
>> | MULTIBOOT_INFO_BOOTDEV
>> | MULTIBOOT_INFO_CMDLINE
>> | MULTIBOOT_INFO_MODS
>> @@ -365,5 +368,7 @@ int load_multiboot(FWCfgState *fw_cfg,
>> option_rom[nb_option_roms].bootindex = 0;
>> nb_option_roms++;
>>
>> + fclose(f);
>> +
>> return 1; /* yes, we are multiboot */
>> }
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index 0d06fc12b6..4d9cc6261a 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -327,7 +327,7 @@ static int load_netboot_image(Error **errp)
>> }
>>
>> img_size = load_elf_ram(netboot_filename, NULL, NULL, &ipl->start_addr,
>> - NULL, NULL, 1, EM_S390, 0, 0, NULL, false);
>> + NULL, NULL, 1, EM_S390, 0, 0, NULL, false, NULL);
>>
>> if (img_size < 0) {
>> img_size = load_image_size(netboot_filename, ram_ptr, ram_size);
>> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
>> index d192e7e2a3..7a7f7983a4 100644
>> --- a/include/hw/elf_ops.h
>> +++ b/include/hw/elf_ops.h
>
> The code in this file is rather old and not quite compliant with the
> QEMU coding style.
The code I added follows existing style in that file.
> This is not an excuse not to apply the correct coding
> style to new additions to the file:
While fixing coding style in this file is a great idea I am not sure
if it makes sense to mix functional changes with formatting changes.
It sounds like formatting should be sent separately.
Even better if there is a way to reformat code automatically. Have you
considered tools like 'clang-format'? I use it at my daytime project
and it is a huge time saver.
>
>> @@ -264,12 +264,13 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>> int must_swab, uint64_t *pentry,
>> uint64_t *lowaddr, uint64_t *highaddr,
>> int elf_machine, int clear_lsb, int data_swab,
>> - AddressSpace *as, bool load_rom)
>> + AddressSpace *as, bool load_rom,
>> + SectionsData *sections)
>> {
>> struct elfhdr ehdr;
>> struct elf_phdr *phdr = NULL, *ph;
>> int size, i, total_size;
>> - elf_word mem_size, file_size;
>> + elf_word mem_size, file_size, sec_size;
>> uint64_t addr, low = (uint64_t)-1, high = 0;
>> uint8_t *data = NULL;
>> char label[128];
>> @@ -481,6 +482,108 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>> }
>> }
>> g_free(phdr);
>> +
>> + if (sections) {
>> + struct elf_shdr *shdr = NULL, *sh;
>> + int shsize;
>> +
>> + // user requested loading all ELF sections
>
> Please use C-style comments: /* ... */
done
>
>> + shsize = ehdr.e_shnum * sizeof(shdr[0]);
>> + if (lseek(fd, ehdr.e_shoff, SEEK_SET) != ehdr.e_shoff) {
>> + goto fail;
>> + }
>> + shdr = g_malloc0(shsize);
>> + if (!shdr)
>> + goto fail;
>
> g_malloc0() never returns NULL. If you want it to return NULL instead of
> aborting qemu when the allocation fails, you need g_try_malloc0().
>
> Also, the QEMU coding style requires braces after an if condition. I'm
> mentioning this only here, but it applies to multiple places in the
> whole patch.
>
>> + if (read(fd, shdr, shsize) != shsize)
>> + goto fail;
>> + if (must_swab) {
>> + for (i = 0; i < ehdr.e_shnum; i++) {
>> + sh = &shdr[i];
>> + glue(bswap_shdr, SZ)(sh);
>> + }
>> + }
>> +
>> + for(i = 0; i < ehdr.e_shnum; i++) {
>
> A space character is required between for and the bracket.
Fixed.
BTW current QEMU code contains 383 such formatting errors.
$ git grep 'for(' | wc -l
383
clang-format will help to fix it.
>
>> + sh = &shdr[i];
>> + if (sh->sh_type == SHT_NULL)
>> + continue;
>> + // if section has address then it is loaded (XXX: is it true?), no need to load it again
>
> This exceeds the limit of 80 characters per line.
fixed
>
>> + if (sh->sh_addr)
>> + continue;
>> +
>> + // append section data at the end of the loaded segments
>> + addr = ROUND_UP(high, sh->sh_addralign);
>> + sh->sh_addr = addr;
>> +
>> + sec_size = sh->sh_size; /* Size of the section data */
>> + data = g_malloc0(sec_size);
>> + if (sec_size > 0) {
>> + if (lseek(fd, sh->sh_offset, SEEK_SET) < 0) {
>> + goto fail;
>> + }
>> + if (read(fd, data, sec_size) != sec_size) {
>> + goto fail;
>> + }
>> + }
>> +
>> + // As these sections are not loadable no need to perform reloaction
>> + // using translate_fn()
>> +
>> + if (data_swab) {
>> + int j;
>> + for (j = 0; j < sec_size; j += (1 << data_swab)) {
>
> Is sec_size guaranteed to be aligned? I don't see a check to verify
> this. Otherwise, could we call bswap64() on the last byte of section,
> accessing seven more bytes outside the allocated buffer?
In this particular case I was following existing code in this file
above glue(load_elf, SZ). As long as original code works this code
should work as well.
I looked at code and the only place that has non-zero data_swab value
is at hw/arm/boot.c. But ARM is not supported by multiboot (at least
now). In other cases, when data_swab is zero no byteswapping is
performed.
>
>> + uint8_t *dp = data + j;
>> + switch (data_swab) {
>> + case (1):
>
> Why 'case (1):' instead of 'case 1:' looks odd.
I have no idea. My code follows existing codestyle in this file below.
>
>> + *(uint16_t *)dp = bswap16(*(uint16_t *)dp);
>> + break;
>> + case (2):
>> + *(uint32_t *)dp = bswap32(*(uint32_t *)dp);
>> + break;
>> + case (3):
>> + *(uint64_t *)dp = bswap64(*(uint64_t *)dp);
>> + break;
>> + default:
>> + g_assert_not_reached();
>> + }
>> + }
>> + }
>> +
>> + if (load_rom) {
>> + snprintf(label, sizeof(label), "shdr #%d: %s", i, name);
>> +
>> + /* rom_add_elf_program() seize the ownership of 'data' */
>> + rom_add_elf_program(label, data, sec_size, sec_size, addr, as);
>> + } else {
>> + cpu_physical_memory_write(addr, data, sec_size);
>> + g_free(data);
>> + }
>> +
>> + total_size += sec_size;
>> + high = addr + sec_size;
>> +
>> + data = NULL;
>> + }
>> +
>> + sections->num = ehdr.e_shnum;
>> + sections->size = ehdr.e_shentsize;
>> + sections->addr = high; // Address where we load the sections header
>> + sections->shndx = ehdr.e_shstrndx;
>> +
>> + // Append section headers at the end of loaded segments/sections
>> + if (load_rom) {
>> + snprintf(label, sizeof(label), "shdrs");
>> +
>> + /* rom_add_elf_program() seize the ownership of 'shdr' */
>> + rom_add_elf_program(label, shdr, shsize, shsize, high, as);
>> + } else {
>> + cpu_physical_memory_write(high, shdr, shsize);
>> + g_free(shdr);
>> + }
>> + high += shsize;
>> + }
>> +
>> if (lowaddr)
>> *lowaddr = (uint64_t)(elf_sword)low;
>> if (highaddr)
>> diff --git a/include/hw/loader.h b/include/hw/loader.h
>> index 355fe0f5a2..3cf2d6da0f 100644
>> --- a/include/hw/loader.h
>> +++ b/include/hw/loader.h
>> @@ -65,6 +65,13 @@ int load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz);
>> #define ELF_LOAD_WRONG_ENDIAN -4
>> const char *load_elf_strerror(int error);
>>
>> +typedef struct {
>> + uint32_t num; // number of loaded sections
>> + uint32_t size; // size of entry in sections header list
>> + uint32_t addr; // address of loaded sections header
>> + uint32_t shndx; // number of section that contains string table
>> +} SectionsData;
>> +
>> /** load_elf_ram:
>> * @filename: Path of ELF file
>> * @translate_fn: optional function to translate load addresses
>> @@ -82,6 +89,8 @@ const char *load_elf_strerror(int error);
>> * @as: The AddressSpace to load the ELF to. The value of address_space_memory
>> * is used if nothing is supplied here.
>> * @load_rom : Load ELF binary as ROM
>> + * @sections: If parameter is non-NULL then all ELF sections loaded into memory
>> + * and this structure is populated.
>> *
>> * Load an ELF file's contents to the emulated system's address space.
>> * Clients may optionally specify a callback to perform address
>> @@ -99,7 +108,7 @@ int load_elf_ram(const char *filename,
>> void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
>> uint64_t *highaddr, int big_endian, int elf_machine,
>> int clear_lsb, int data_swab, AddressSpace *as,
>> - bool load_rom);
>> + bool load_rom, SectionsData *sections);
>>
>> /** load_elf_as:
>> * Same as load_elf_ram(), but always loads the elf as ROM
>> diff --git a/tests/multiboot/Makefile b/tests/multiboot/Makefile
>> index 36f01dc647..b6a5056347 100644
>> --- a/tests/multiboot/Makefile
>> +++ b/tests/multiboot/Makefile
>> @@ -6,7 +6,7 @@ LD=ld
>> LDFLAGS=-melf_i386 -T link.ld
>> LIBS=$(shell $(CC) $(CCFLAGS) -print-libgcc-file-name)
>>
>> -all: mmap.elf modules.elf
>> +all: mmap.elf modules.elf sections.elf sections.out
>>
>> mmap.elf: start.o mmap.o libc.o
>> $(LD) $(LDFLAGS) -o $@ $^ $(LIBS)
>> @@ -14,6 +14,12 @@ mmap.elf: start.o mmap.o libc.o
>> modules.elf: start.o modules.o libc.o
>> $(LD) $(LDFLAGS) -o $@ $^ $(LIBS)
>>
>> +sections.elf: start.o sections.o libc.o
>> + $(LD) $(LDFLAGS) -o $@ $^ $(LIBS)
>> +
>> +sections.out: sections.elf generate_sections_out.py
>> + python2 generate_sections_out.py $^ > $@
>> +
>> %.o: %.c
>> $(CC) $(CCFLAGS) -c -o $@ $^
>>
>> diff --git a/tests/multiboot/generate_sections_out.py b/tests/multiboot/generate_sections_out.py
>> new file mode 100755
>> index 0000000000..8077856823
>> --- /dev/null
>> +++ b/tests/multiboot/generate_sections_out.py
>> @@ -0,0 +1,33 @@
>> +#!/usr/bin/env python2
>> +
>> +import sys
>> +
>> +from elftools.elf.elffile import ELFFile
>
> I don't think we can expect this module to be present. For example, on
> my system, attempting to run the test fails:
>
> ImportError: No module named elftools.elf.elffile
>
> Maybe we can parse the output of readelf instead?
Does readelf avilable at all supported platforms (Windows, MacOSX)?
How can we sure that readelf output stays consistent across versions/platforms?
Using this library is a great way to avoid the problems.
>> +def roundup(num, align):
>> + return (num + align - 1) / align * align
>> +
>> +def main(filename):
>> + print "\n\n\n=== Running test case: sections.elf ===\n"
>> + print "Multiboot header at 9500, ELF section headers present\n"
>> +
>> + with open(filename, 'rb') as f:
>> + elf = ELFFile(f)
>> + header = elf.header
>> + load_addr = 0x100000 # we load image starting from 1M
>> + sections = ""
>> + for s in elf.iter_sections():
>> + align = s.header.sh_addralign
>> + addr = 0
>> + if s.header.sh_type != 'SHT_NULL':
>> + addr = s.header.sh_addr
>> + if addr == 0:
>> + addr = roundup(load_addr, s.header.sh_addralign)
>> + load_addr = addr + s.header.sh_size
>> + sections += "Elf section name=%s addr=%x size=%d\n" % (s.name, addr, s.header.sh_size)
>> +
>> + print "Sections list with %d entries of size %d at %x, string index %d" % (header.e_shnum, header.e_shentsize, load_addr, header.e_shstrndx)
>
> We're not as strict about the 80 characters rule in Python code, but I
> think this line could use being wrapped.
>
>> + print sections,
>> +
>> +if __name__ == '__main__':
>> + main(sys.argv[1])
>> diff --git a/tests/multiboot/modules.out b/tests/multiboot/modules.out
>> index 0da7b39374..b6c1a01be1 100644
>> --- a/tests/multiboot/modules.out
>> +++ b/tests/multiboot/modules.out
>> @@ -5,15 +5,15 @@
>>
>> Multiboot header at 9500
>>
>> -Module list with 0 entries at 102000
>> +Module list with 0 entries at 104000
>>
>>
>> === Running test case: modules.elf -initrd module.txt ===
>>
>> Multiboot header at 9500
>>
>> -Module list with 1 entries at 102000
>> -[102000] Module: 103000 - 103038 (56 bytes) 'module.txt'
>> +Module list with 1 entries at 104000
>> +[104000] Module: 105000 - 105038 (56 bytes) 'module.txt'
>> Content: 'This is a test file that is used as a multiboot module.'
>>
>>
>> @@ -21,8 +21,8 @@ Module list with 1 entries at 102000
>>
>> Multiboot header at 9500
>>
>> -Module list with 1 entries at 102000
>> -[102000] Module: 103000 - 103038 (56 bytes) 'module.txt argument'
>> +Module list with 1 entries at 104000
>> +[104000] Module: 105000 - 105038 (56 bytes) 'module.txt argument'
>> Content: 'This is a test file that is used as a multiboot module.'
>>
>>
>> @@ -30,8 +30,8 @@ Module list with 1 entries at 102000
>>
>> Multiboot header at 9500
>>
>> -Module list with 1 entries at 102000
>> -[102000] Module: 103000 - 103038 (56 bytes) 'module.txt argument,with,commas'
>> +Module list with 1 entries at 104000
>> +[104000] Module: 105000 - 105038 (56 bytes) 'module.txt argument,with,commas'
>> Content: 'This is a test file that is used as a multiboot module.'
>>
>>
>> @@ -39,10 +39,10 @@ Module list with 1 entries at 102000
>>
>> Multiboot header at 9500
>>
>> -Module list with 3 entries at 102000
>> -[102000] Module: 103000 - 103038 (56 bytes) 'module.txt'
>> +Module list with 3 entries at 104000
>> +[104000] Module: 105000 - 105038 (56 bytes) 'module.txt'
>> Content: 'This is a test file that is used as a multiboot module.'
>> -[102010] Module: 104000 - 104038 (56 bytes) 'module.txt argument'
>> +[104010] Module: 106000 - 106038 (56 bytes) 'module.txt argument'
>> Content: 'This is a test file that is used as a multiboot module.'
>> -[102020] Module: 105000 - 105038 (56 bytes) 'module.txt'
>> +[104020] Module: 107000 - 107038 (56 bytes) 'module.txt'
>> Content: 'This is a test file that is used as a multiboot module.'
>> diff --git a/tests/multiboot/run_test.sh b/tests/multiboot/run_test.sh
>> index 0278148b43..f04e35cbf0 100755
>> --- a/tests/multiboot/run_test.sh
>> +++ b/tests/multiboot/run_test.sh
>> @@ -56,9 +56,13 @@ modules() {
>> run_qemu modules.elf -initrd "module.txt,module.txt argument,module.txt"
>> }
>>
>> +sections() {
>> + run_qemu sections.elf
>> +}
>> +
>> make all
>>
>> -for t in mmap modules; do
>> +for t in mmap modules sections; do
>>
>> echo > test.log
>> $t
>> diff --git a/tests/multiboot/sections.c b/tests/multiboot/sections.c
>> new file mode 100644
>> index 0000000000..05a88f99ac
>> --- /dev/null
>> +++ b/tests/multiboot/sections.c
>> @@ -0,0 +1,55 @@
>> +/*
>> + * Copyright (c) 2017 Anatol Pomozov <anatol.pomozov@gmail.com>
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#include "libc.h"
>> +#include "../../hw/i386/multiboot_header.h"
>> +#include "../../include/elf.h"
>> +
>> +int test_main(uint32_t magic, struct multiboot_info *mbi)
>> +{
>> + void *p;
>> + unsigned int i;
>> +
>> + (void) magic;
>> + multiboot_elf_section_header_table_t shdr;
>> +
>> + printf("Multiboot header at %x, ELF section headers %s\n\n", mbi,
>> + mbi->flags & MULTIBOOT_INFO_ELF_SHDR ? "present" : "don't present");
>> +
>> + shdr = mbi->u.elf_sec;
>> + printf("Sections list with %d entries of size %d at %x, string index %d\n",
>> + shdr.num, shdr.size, shdr.addr, shdr.shndx);
>> +
>> + const char *string_table = (char *)((Elf32_Shdr *)(uintptr_t)(shdr.addr + shdr.shndx * shdr.size))->sh_addr;
>
> 80 characters again.
fixed.
>
>> +
>> + for (i = 0, p = (void *)shdr.addr;
>> + i < shdr.num;
>> + i++, p += shdr.size)
>> + {
>> + Elf32_Shdr *sec;
>> +
>> + sec = (Elf32_Shdr *)p;
>> + printf("Elf section name=%s addr=%lx size=%ld\n", string_table + sec->sh_name, sec->sh_addr, sec->sh_size);
>
> And here.
fixed.
>
>> + }
>> +
>> + return 0;
>> +}
>
> Should we try to access one of the section headers to make sure that we
> didn't only get the correct addresses, but that data is actually loaded
> there?
>
> Kevin
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] multiboot: Change multiboot_info from array of bytes to a C struct
2018-01-29 18:21 ` Anatol Pomozov
@ 2018-01-29 20:02 ` Kevin Wolf
2018-02-09 21:48 ` Anatol Pomozov
0 siblings, 1 reply; 33+ messages in thread
From: Kevin Wolf @ 2018-01-29 20:02 UTC (permalink / raw)
To: Anatol Pomozov
Cc: QEMU Developers, Eduardo Habkost, Alexander Graf, Paolo Bonzini,
Richard Henderson
Am 29.01.2018 um 19:21 hat Anatol Pomozov geschrieben:
> Hi
>
> On Mon, Jan 15, 2018 at 6:49 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> > Am 13.10.2017 um 01:54 hat Anatol Pomozov geschrieben:
> >> Using C structs makes the code more readable and prevents type conversion
> >> errors.
> >>
> >> Borrow multiboot1 header from GRUB project.
> >>
> >> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
> >> ---
> >> hw/i386/multiboot.c | 124 +++++++++------------
> >> hw/i386/multiboot_header.h | 254 ++++++++++++++++++++++++++++++++++++++++++++
> >> tests/multiboot/mmap.c | 14 +--
> >> tests/multiboot/mmap.out | 10 ++
> >> tests/multiboot/modules.c | 12 ++-
> >> tests/multiboot/modules.out | 10 ++
> >> tests/multiboot/multiboot.h | 66 ------------
> >> 7 files changed, 339 insertions(+), 151 deletions(-)
> >> create mode 100644 hw/i386/multiboot_header.h
> >> delete mode 100644 tests/multiboot/multiboot.h
> >
> > This is a good change in general. However, I'm not sure if we should
> > really replace the header used in the tests. After all, the tests also
> > test whether things are at the right place, and if there is an error in
> > the header file, we can only catch it if the tests keep using their own
> > definitions.
>
> The added multibooh.h is from GRUB - the developers of multiboot. I
> think we can trust it. Having a single header will make future tests
> maintainence easier.
>
> But if you feel strongly that qemu tests should use it's own version
> of multiboot header then I can add it back.
No, I don't feel strongly, just wanted to mention that there is an
advantage of having a separate header in case you had not thought of it.
The decision is up to you.
Kevin
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] multiboot: Change multiboot_info from array of bytes to a C struct
2018-01-29 20:02 ` Kevin Wolf
@ 2018-02-09 21:48 ` Anatol Pomozov
2018-02-09 21:52 ` Anatol Pomozov
2018-02-12 13:37 ` Kevin Wolf
0 siblings, 2 replies; 33+ messages in thread
From: Anatol Pomozov @ 2018-02-09 21:48 UTC (permalink / raw)
To: Kevin Wolf
Cc: QEMU Developers, Eduardo Habkost, Alexander Graf, Paolo Bonzini,
Richard Henderson
Hi Kevin
Is the patch series look good? Are there any other unresolved issues?
On Mon, Jan 29, 2018 at 12:02 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 29.01.2018 um 19:21 hat Anatol Pomozov geschrieben:
>> Hi
>>
>> On Mon, Jan 15, 2018 at 6:49 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>> > Am 13.10.2017 um 01:54 hat Anatol Pomozov geschrieben:
>> >> Using C structs makes the code more readable and prevents type conversion
>> >> errors.
>> >>
>> >> Borrow multiboot1 header from GRUB project.
>> >>
>> >> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
>> >> ---
>> >> hw/i386/multiboot.c | 124 +++++++++------------
>> >> hw/i386/multiboot_header.h | 254 ++++++++++++++++++++++++++++++++++++++++++++
>> >> tests/multiboot/mmap.c | 14 +--
>> >> tests/multiboot/mmap.out | 10 ++
>> >> tests/multiboot/modules.c | 12 ++-
>> >> tests/multiboot/modules.out | 10 ++
>> >> tests/multiboot/multiboot.h | 66 ------------
>> >> 7 files changed, 339 insertions(+), 151 deletions(-)
>> >> create mode 100644 hw/i386/multiboot_header.h
>> >> delete mode 100644 tests/multiboot/multiboot.h
>> >
>> > This is a good change in general. However, I'm not sure if we should
>> > really replace the header used in the tests. After all, the tests also
>> > test whether things are at the right place, and if there is an error in
>> > the header file, we can only catch it if the tests keep using their own
>> > definitions.
>>
>> The added multibooh.h is from GRUB - the developers of multiboot. I
>> think we can trust it. Having a single header will make future tests
>> maintainence easier.
>>
>> But if you feel strongly that qemu tests should use it's own version
>> of multiboot header then I can add it back.
>
> No, I don't feel strongly, just wanted to mention that there is an
> advantage of having a separate header in case you had not thought of it.
> The decision is up to you.
I think it is better to use one standard trusted header. This way we
reduce maintenance cost.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] multiboot: Change multiboot_info from array of bytes to a C struct
2018-02-09 21:48 ` Anatol Pomozov
@ 2018-02-09 21:52 ` Anatol Pomozov
2018-02-12 13:33 ` Kevin Wolf
2018-02-12 13:37 ` Kevin Wolf
1 sibling, 1 reply; 33+ messages in thread
From: Anatol Pomozov @ 2018-02-09 21:52 UTC (permalink / raw)
To: Kevin Wolf
Cc: QEMU Developers, Eduardo Habkost, Alexander Graf, Paolo Bonzini,
Richard Henderson
Hello
Actually I just fetched recent chnages and tests/multiboot/run_test.sh
does not work for me anymore. I rebuilt 'master' branch without my
changes and see the same issue. It looks like debug console does not
print to stdio anymore.
Does anyone see the same issue?
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] multiboot: Change multiboot_info from array of bytes to a C struct
2018-02-09 21:52 ` Anatol Pomozov
@ 2018-02-12 13:33 ` Kevin Wolf
0 siblings, 0 replies; 33+ messages in thread
From: Kevin Wolf @ 2018-02-12 13:33 UTC (permalink / raw)
To: Anatol Pomozov
Cc: QEMU Developers, Eduardo Habkost, Alexander Graf, Paolo Bonzini,
Richard Henderson
Am 09.02.2018 um 22:52 hat Anatol Pomozov geschrieben:
> Actually I just fetched recent chnages and tests/multiboot/run_test.sh
> does not work for me anymore. I rebuilt 'master' branch without my
> changes and see the same issue. It looks like debug console does not
> print to stdio anymore.
>
> Does anyone see the same issue?
Still works for me on current git master.
Kevin
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] multiboot: Change multiboot_info from array of bytes to a C struct
2018-02-09 21:48 ` Anatol Pomozov
2018-02-09 21:52 ` Anatol Pomozov
@ 2018-02-12 13:37 ` Kevin Wolf
1 sibling, 0 replies; 33+ messages in thread
From: Kevin Wolf @ 2018-02-12 13:37 UTC (permalink / raw)
To: Anatol Pomozov
Cc: QEMU Developers, Eduardo Habkost, Alexander Graf, Paolo Bonzini,
Richard Henderson
Am 09.02.2018 um 22:48 hat Anatol Pomozov geschrieben:
> Hi Kevin
>
> Is the patch series look good? Are there any other unresolved issues?
This is the email thread for the first version. No, it doesn't look
good.
In the thread for your second version, Jack had a few comments that you
didn't respond to yet.
Kevin
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] multiboot: Change multiboot_info from array of bytes to a C struct
2018-02-05 21:43 ` Anatol Pomozov
@ 2018-02-06 20:35 ` Jack Schwartz
0 siblings, 0 replies; 33+ messages in thread
From: Jack Schwartz @ 2018-02-06 20:35 UTC (permalink / raw)
To: Anatol Pomozov, Kevin Wolf; +Cc: QEMU Developers
Hi Anatol and Kevin.
Kevin and Anatol, thanks for your replies.
A few comments inline close to the bottom...
On 2018-02-05 13:43, Anatol Pomozov wrote:
> Hi
>
> On Wed, Jan 31, 2018 at 1:12 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>> Am 31.01.2018 um 00:15 hat Jack Schwartz geschrieben:
>>> Hi Anatol and Kevin.
>>>
>>> Even though I am new to Qemu, I have a patch to deliver for
>>> multiboot.c (as you know) and so I feel familiar enough to do a review
>>> of this patch. One comment is probably more for maintainers.
>> The Multiboot code is essentially unmaintained. It's technically part of
>> the PC/x86 subsystem, but their maintainers don't seem to care at all.
>> So in order to make any progress here, I decided that I will send a
>> pull request for Multiboot once we have the patches ready (as a one-time
>> thing, without officially making myself the maintainer).
>>
>> So I am the closest thing to a maintainer that we have here, and while
>> I'm familiar with some of the Multiboot-specific code, I don't really
>> know the ELF code and don't have a lot of time to spend here.
>>
>> Therefore it's very welcome if you review the patches of each other,
>> even if you're not perfectly familiar with the code, as there is
>> probably noone else who could do a better review.
>>
>>> On 01/29/18 12:43, Anatol Pomozov wrote:
>>>> @@ -253,11 +228,11 @@ int load_multiboot(FWCfgState *fw_cfg,
>>>> mb_load_size = mb_kernel_size;
>>>> }
>>>> - /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_VBE.
>>>> - uint32_t mh_mode_type = ldl_p(header+i+32);
>>>> - uint32_t mh_width = ldl_p(header+i+36);
>>>> - uint32_t mh_height = ldl_p(header+i+40);
>>>> - uint32_t mh_depth = ldl_p(header+i+44); */
>>>> + /* Valid if mh_flags sets MULTIBOOT_VIDEO_MODE.
>>>> + uint32_t mh_mode_type = ldl_p(&multiboot_header->mode_type);
>>>> + uint32_t mh_width = ldl_p(&multiboot_header->width);
>>>> + uint32_t mh_height = ldl_p(&multiboot_header->height);
>>>> + uint32_t mh_depth = ldl_p(&multiboot_header->depth); */
>>> This question is probably more for maintainers...
>>>
>>> In the patch series I submitted, people were OK that I was going to delete
>>> these lines since they were only comments anyway. Your approach leaves
>>> these lines in and updates them even though they are comments. Which way is
>>> preferred?
>> As far as I am concerned, I honestly don't mind either way. It's
>> trivial code, so we won't lose anything by removing it.
> This change suppose to be a refactoring and tries to avoid functional
> changes. I can remove it in a separate change.
>
>> The ideal solution would be just implementing support for it, of course.
>> If we wanted to do that, I think we would have to pass the values
>> through fw_cfg and then set the VBE mode in the Mutiboot option rom.
>>
>>>> mb_debug("multiboot: mh_header_addr = %#x\n", mh_header_addr);
>>>> mb_debug("multiboot: mh_load_addr = %#x\n", mh_load_addr);
>>>> @@ -295,14 +270,15 @@ int load_multiboot(FWCfgState *fw_cfg,
>>>> }
>>>> mbs.mb_buf_size += cmdline_len;
>>>> - mbs.mb_buf_size += MB_MOD_SIZE * mbs.mb_mods_avail;
>>>> + mbs.mb_buf_size += sizeof(multiboot_module_t) * mbs.mb_mods_avail;
>>>> mbs.mb_buf_size += strlen(bootloader_name) + 1;
>>>> mbs.mb_buf_size = TARGET_PAGE_ALIGN(mbs.mb_buf_size);
>>>> /* enlarge mb_buf to hold cmdlines, bootloader, mb-info structs */
>>>> mbs.mb_buf = g_realloc(mbs.mb_buf, mbs.mb_buf_size);
>>>> - mbs.offset_cmdlines = mbs.offset_mbinfo + mbs.mb_mods_avail * MB_MOD_SIZE;
>>>> + mbs.offset_cmdlines = mbs.offset_mbinfo +
>>>> + mbs.mb_mods_avail * sizeof(multiboot_module_t);
>>>> mbs.offset_bootloader = mbs.offset_cmdlines + cmdline_len;
>>>> if (initrd_filename) {
>>>> @@ -348,22 +324,22 @@ int load_multiboot(FWCfgState *fw_cfg,
>>>> char kcmdline[strlen(kernel_filename) + strlen(kernel_cmdline) + 2];
>>>> snprintf(kcmdline, sizeof(kcmdline), "%s %s",
>>>> kernel_filename, kernel_cmdline);
>>>> - stl_p(bootinfo + MBI_CMDLINE, mb_add_cmdline(&mbs, kcmdline));
>>>> + stl_p(&bootinfo.cmdline, mb_add_cmdline(&mbs, kcmdline));
>>>> - stl_p(bootinfo + MBI_BOOTLOADER, mb_add_bootloader(&mbs, bootloader_name));
>>>> + stl_p(&bootinfo.boot_loader_name, mb_add_bootloader(&mbs, bootloader_name));
>>>> - stl_p(bootinfo + MBI_MODS_ADDR, mbs.mb_buf_phys + mbs.offset_mbinfo);
>>>> - stl_p(bootinfo + MBI_MODS_COUNT, mbs.mb_mods_count); /* mods_count */
>>>> + stl_p(&bootinfo.mods_addr, mbs.mb_buf_phys + mbs.offset_mbinfo);
>>>> + stl_p(&bootinfo.mods_count, mbs.mb_mods_count); /* mods_count */
>>>> /* the kernel is where we want it to be now */
>>>> - stl_p(bootinfo + MBI_FLAGS, MULTIBOOT_FLAGS_MEMORY
>>>> - | MULTIBOOT_FLAGS_BOOT_DEVICE
>>>> - | MULTIBOOT_FLAGS_CMDLINE
>>>> - | MULTIBOOT_FLAGS_MODULES
>>>> - | MULTIBOOT_FLAGS_MMAP
>>>> - | MULTIBOOT_FLAGS_BOOTLOADER);
>>>> - stl_p(bootinfo + MBI_BOOT_DEVICE, 0x8000ffff); /* XXX: use the -boot switch? */
>>>> - stl_p(bootinfo + MBI_MMAP_ADDR, ADDR_E820_MAP);
>>>> + stl_p(&bootinfo.flags, MULTIBOOT_INFO_MEMORY
>>>> + | MULTIBOOT_INFO_BOOTDEV
>>>> + | MULTIBOOT_INFO_CMDLINE
>>>> + | MULTIBOOT_INFO_MODS
>>>> + | MULTIBOOT_INFO_MEM_MAP
>>>> + | MULTIBOOT_INFO_BOOT_LOADER_NAME);
>>>> + stl_p(&bootinfo.boot_device, 0x8000ffff); /* XXX: use the -boot switch? */
>>>> + stl_p(&bootinfo.mmap_addr, ADDR_E820_MAP);
>>>> mb_debug("multiboot: mh_entry_addr = %#x\n", mh_entry_addr);
>>>> mb_debug(" mb_buf_phys = "TARGET_FMT_plx"\n", mbs.mb_buf_phys);
>>>> @@ -371,7 +347,7 @@ int load_multiboot(FWCfgState *fw_cfg,
>>>> mb_debug(" mb_mods_count = %d\n", mbs.mb_mods_count);
>>>> /* save bootinfo off the stack */
>>>> - mb_bootinfo_data = g_memdup(bootinfo, sizeof(bootinfo));
>>>> + mb_bootinfo_data = g_memdup(&bootinfo, sizeof(bootinfo));
>>>> /* Pass variables to option rom */
>>>> fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_ENTRY, mh_entry_addr);
>>> <snip>
>>>> diff --git a/tests/multiboot/mmap.c b/tests/multiboot/mmap.c
>>>> index 766b003f38..9cba8b07e0 100644
>>>> --- a/tests/multiboot/mmap.c
>>>> +++ b/tests/multiboot/mmap.c
>>>> @@ -21,15 +21,17 @@
>>>> */
>>>> #include "libc.h"
>>>> -#include "multiboot.h"
>>>> +#include "../../hw/i386/multiboot_header.h"
>>> Suggestion: create a multiboot subdir under include, and put
>>> multiboot_header.h and other multiboot includes there. This way you can
>>> just #include "multiboot/multiboot_header.h" which seems cleaner to me than
>>> "../../hw/i386/multiboot_header.h"
>> Good point, but do we even have multiple header files for Multiboot to
>> justify a whole subdirectory?
> I do not think there is a justification for it. +1 for keeping it in "/include/"
>> There is include/hw/loader.h and include/elf.h, so I would suggest that
>> we add the new header as either include/hw/multiboot.h or directly
>> include/multiboot.h.
OK. This makes the include list cleaner without having to add another
subdirectory.
... or put both multiboot header files under include/hw/i386 to keep
them together in an existing, appropriate and more specific directory.
>> There is already ./hw/i386/multiboot.h file so it is going to be
>> confusing to have multiple files with the same name.
>>
>> I propose to rename ./hw/i386/multiboot.h to something like
>> ./hw/i386/multiboot_loader.h or ./hw/i386/load_multiboot.h
Filename change sounds good.
Also, to avoid confusion with tests/multiboot/multiboot.h, I suggest
renaming that header file to tests/multiboot/multiboot_test.h
Thanks,
Jack
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] multiboot: Change multiboot_info from array of bytes to a C struct
2018-01-31 9:12 ` Kevin Wolf
@ 2018-02-05 21:43 ` Anatol Pomozov
2018-02-06 20:35 ` Jack Schwartz
0 siblings, 1 reply; 33+ messages in thread
From: Anatol Pomozov @ 2018-02-05 21:43 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Jack Schwartz, QEMU Developers
Hi
On Wed, Jan 31, 2018 at 1:12 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 31.01.2018 um 00:15 hat Jack Schwartz geschrieben:
>> Hi Anatol and Kevin.
>>
>> Even though I am new to Qemu, I have a patch to deliver for
>> multiboot.c (as you know) and so I feel familiar enough to do a review
>> of this patch. One comment is probably more for maintainers.
>
> The Multiboot code is essentially unmaintained. It's technically part of
> the PC/x86 subsystem, but their maintainers don't seem to care at all.
> So in order to make any progress here, I decided that I will send a
> pull request for Multiboot once we have the patches ready (as a one-time
> thing, without officially making myself the maintainer).
>
> So I am the closest thing to a maintainer that we have here, and while
> I'm familiar with some of the Multiboot-specific code, I don't really
> know the ELF code and don't have a lot of time to spend here.
>
> Therefore it's very welcome if you review the patches of each other,
> even if you're not perfectly familiar with the code, as there is
> probably noone else who could do a better review.
>
>> On 01/29/18 12:43, Anatol Pomozov wrote:
>> > @@ -253,11 +228,11 @@ int load_multiboot(FWCfgState *fw_cfg,
>> > mb_load_size = mb_kernel_size;
>> > }
>> > - /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_VBE.
>> > - uint32_t mh_mode_type = ldl_p(header+i+32);
>> > - uint32_t mh_width = ldl_p(header+i+36);
>> > - uint32_t mh_height = ldl_p(header+i+40);
>> > - uint32_t mh_depth = ldl_p(header+i+44); */
>> > + /* Valid if mh_flags sets MULTIBOOT_VIDEO_MODE.
>> > + uint32_t mh_mode_type = ldl_p(&multiboot_header->mode_type);
>> > + uint32_t mh_width = ldl_p(&multiboot_header->width);
>> > + uint32_t mh_height = ldl_p(&multiboot_header->height);
>> > + uint32_t mh_depth = ldl_p(&multiboot_header->depth); */
>> This question is probably more for maintainers...
>>
>> In the patch series I submitted, people were OK that I was going to delete
>> these lines since they were only comments anyway. Your approach leaves
>> these lines in and updates them even though they are comments. Which way is
>> preferred?
>
> As far as I am concerned, I honestly don't mind either way. It's
> trivial code, so we won't lose anything by removing it.
This change suppose to be a refactoring and tries to avoid functional
changes. I can remove it in a separate change.
>
> The ideal solution would be just implementing support for it, of course.
> If we wanted to do that, I think we would have to pass the values
> through fw_cfg and then set the VBE mode in the Mutiboot option rom.
>
>> > mb_debug("multiboot: mh_header_addr = %#x\n", mh_header_addr);
>> > mb_debug("multiboot: mh_load_addr = %#x\n", mh_load_addr);
>> > @@ -295,14 +270,15 @@ int load_multiboot(FWCfgState *fw_cfg,
>> > }
>> > mbs.mb_buf_size += cmdline_len;
>> > - mbs.mb_buf_size += MB_MOD_SIZE * mbs.mb_mods_avail;
>> > + mbs.mb_buf_size += sizeof(multiboot_module_t) * mbs.mb_mods_avail;
>> > mbs.mb_buf_size += strlen(bootloader_name) + 1;
>> > mbs.mb_buf_size = TARGET_PAGE_ALIGN(mbs.mb_buf_size);
>> > /* enlarge mb_buf to hold cmdlines, bootloader, mb-info structs */
>> > mbs.mb_buf = g_realloc(mbs.mb_buf, mbs.mb_buf_size);
>> > - mbs.offset_cmdlines = mbs.offset_mbinfo + mbs.mb_mods_avail * MB_MOD_SIZE;
>> > + mbs.offset_cmdlines = mbs.offset_mbinfo +
>> > + mbs.mb_mods_avail * sizeof(multiboot_module_t);
>> > mbs.offset_bootloader = mbs.offset_cmdlines + cmdline_len;
>> > if (initrd_filename) {
>> > @@ -348,22 +324,22 @@ int load_multiboot(FWCfgState *fw_cfg,
>> > char kcmdline[strlen(kernel_filename) + strlen(kernel_cmdline) + 2];
>> > snprintf(kcmdline, sizeof(kcmdline), "%s %s",
>> > kernel_filename, kernel_cmdline);
>> > - stl_p(bootinfo + MBI_CMDLINE, mb_add_cmdline(&mbs, kcmdline));
>> > + stl_p(&bootinfo.cmdline, mb_add_cmdline(&mbs, kcmdline));
>> > - stl_p(bootinfo + MBI_BOOTLOADER, mb_add_bootloader(&mbs, bootloader_name));
>> > + stl_p(&bootinfo.boot_loader_name, mb_add_bootloader(&mbs, bootloader_name));
>> > - stl_p(bootinfo + MBI_MODS_ADDR, mbs.mb_buf_phys + mbs.offset_mbinfo);
>> > - stl_p(bootinfo + MBI_MODS_COUNT, mbs.mb_mods_count); /* mods_count */
>> > + stl_p(&bootinfo.mods_addr, mbs.mb_buf_phys + mbs.offset_mbinfo);
>> > + stl_p(&bootinfo.mods_count, mbs.mb_mods_count); /* mods_count */
>> > /* the kernel is where we want it to be now */
>> > - stl_p(bootinfo + MBI_FLAGS, MULTIBOOT_FLAGS_MEMORY
>> > - | MULTIBOOT_FLAGS_BOOT_DEVICE
>> > - | MULTIBOOT_FLAGS_CMDLINE
>> > - | MULTIBOOT_FLAGS_MODULES
>> > - | MULTIBOOT_FLAGS_MMAP
>> > - | MULTIBOOT_FLAGS_BOOTLOADER);
>> > - stl_p(bootinfo + MBI_BOOT_DEVICE, 0x8000ffff); /* XXX: use the -boot switch? */
>> > - stl_p(bootinfo + MBI_MMAP_ADDR, ADDR_E820_MAP);
>> > + stl_p(&bootinfo.flags, MULTIBOOT_INFO_MEMORY
>> > + | MULTIBOOT_INFO_BOOTDEV
>> > + | MULTIBOOT_INFO_CMDLINE
>> > + | MULTIBOOT_INFO_MODS
>> > + | MULTIBOOT_INFO_MEM_MAP
>> > + | MULTIBOOT_INFO_BOOT_LOADER_NAME);
>> > + stl_p(&bootinfo.boot_device, 0x8000ffff); /* XXX: use the -boot switch? */
>> > + stl_p(&bootinfo.mmap_addr, ADDR_E820_MAP);
>> > mb_debug("multiboot: mh_entry_addr = %#x\n", mh_entry_addr);
>> > mb_debug(" mb_buf_phys = "TARGET_FMT_plx"\n", mbs.mb_buf_phys);
>> > @@ -371,7 +347,7 @@ int load_multiboot(FWCfgState *fw_cfg,
>> > mb_debug(" mb_mods_count = %d\n", mbs.mb_mods_count);
>> > /* save bootinfo off the stack */
>> > - mb_bootinfo_data = g_memdup(bootinfo, sizeof(bootinfo));
>> > + mb_bootinfo_data = g_memdup(&bootinfo, sizeof(bootinfo));
>> > /* Pass variables to option rom */
>> > fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_ENTRY, mh_entry_addr);
>> <snip>
>> > diff --git a/tests/multiboot/mmap.c b/tests/multiboot/mmap.c
>> > index 766b003f38..9cba8b07e0 100644
>> > --- a/tests/multiboot/mmap.c
>> > +++ b/tests/multiboot/mmap.c
>> > @@ -21,15 +21,17 @@
>> > */
>> > #include "libc.h"
>> > -#include "multiboot.h"
>> > +#include "../../hw/i386/multiboot_header.h"
>> Suggestion: create a multiboot subdir under include, and put
>> multiboot_header.h and other multiboot includes there. This way you can
>> just #include "multiboot/multiboot_header.h" which seems cleaner to me than
>> "../../hw/i386/multiboot_header.h"
>
> Good point, but do we even have multiple header files for Multiboot to
> justify a whole subdirectory?
I do not think there is a justification for it. +1 for keeping it in "/include/"
> There is include/hw/loader.h and include/elf.h, so I would suggest that
> we add the new header as either include/hw/multiboot.h or directly
> include/multiboot.h.
There is already ./hw/i386/multiboot.h file so it is going to be
confusing to have multiple files with the same name.
I propose to rename ./hw/i386/multiboot.h to something like
./hw/i386/multiboot_loader.h or ./hw/i386/load_multiboot.h
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] multiboot: Change multiboot_info from array of bytes to a C struct
2018-01-30 23:15 ` Jack Schwartz
@ 2018-01-31 9:12 ` Kevin Wolf
2018-02-05 21:43 ` Anatol Pomozov
0 siblings, 1 reply; 33+ messages in thread
From: Kevin Wolf @ 2018-01-31 9:12 UTC (permalink / raw)
To: Jack Schwartz; +Cc: Anatol Pomozov, qemu-devel
Am 31.01.2018 um 00:15 hat Jack Schwartz geschrieben:
> Hi Anatol and Kevin.
>
> Even though I am new to Qemu, I have a patch to deliver for
> multiboot.c (as you know) and so I feel familiar enough to do a review
> of this patch. One comment is probably more for maintainers.
The Multiboot code is essentially unmaintained. It's technically part of
the PC/x86 subsystem, but their maintainers don't seem to care at all.
So in order to make any progress here, I decided that I will send a
pull request for Multiboot once we have the patches ready (as a one-time
thing, without officially making myself the maintainer).
So I am the closest thing to a maintainer that we have here, and while
I'm familiar with some of the Multiboot-specific code, I don't really
know the ELF code and don't have a lot of time to spend here.
Therefore it's very welcome if you review the patches of each other,
even if you're not perfectly familiar with the code, as there is
probably noone else who could do a better review.
> On 01/29/18 12:43, Anatol Pomozov wrote:
> > @@ -253,11 +228,11 @@ int load_multiboot(FWCfgState *fw_cfg,
> > mb_load_size = mb_kernel_size;
> > }
> > - /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_VBE.
> > - uint32_t mh_mode_type = ldl_p(header+i+32);
> > - uint32_t mh_width = ldl_p(header+i+36);
> > - uint32_t mh_height = ldl_p(header+i+40);
> > - uint32_t mh_depth = ldl_p(header+i+44); */
> > + /* Valid if mh_flags sets MULTIBOOT_VIDEO_MODE.
> > + uint32_t mh_mode_type = ldl_p(&multiboot_header->mode_type);
> > + uint32_t mh_width = ldl_p(&multiboot_header->width);
> > + uint32_t mh_height = ldl_p(&multiboot_header->height);
> > + uint32_t mh_depth = ldl_p(&multiboot_header->depth); */
> This question is probably more for maintainers...
>
> In the patch series I submitted, people were OK that I was going to delete
> these lines since they were only comments anyway. Your approach leaves
> these lines in and updates them even though they are comments. Which way is
> preferred?
As far as I am concerned, I honestly don't mind either way. It's
trivial code, so we won't lose anything by removing it.
The ideal solution would be just implementing support for it, of course.
If we wanted to do that, I think we would have to pass the values
through fw_cfg and then set the VBE mode in the Mutiboot option rom.
> > mb_debug("multiboot: mh_header_addr = %#x\n", mh_header_addr);
> > mb_debug("multiboot: mh_load_addr = %#x\n", mh_load_addr);
> > @@ -295,14 +270,15 @@ int load_multiboot(FWCfgState *fw_cfg,
> > }
> > mbs.mb_buf_size += cmdline_len;
> > - mbs.mb_buf_size += MB_MOD_SIZE * mbs.mb_mods_avail;
> > + mbs.mb_buf_size += sizeof(multiboot_module_t) * mbs.mb_mods_avail;
> > mbs.mb_buf_size += strlen(bootloader_name) + 1;
> > mbs.mb_buf_size = TARGET_PAGE_ALIGN(mbs.mb_buf_size);
> > /* enlarge mb_buf to hold cmdlines, bootloader, mb-info structs */
> > mbs.mb_buf = g_realloc(mbs.mb_buf, mbs.mb_buf_size);
> > - mbs.offset_cmdlines = mbs.offset_mbinfo + mbs.mb_mods_avail * MB_MOD_SIZE;
> > + mbs.offset_cmdlines = mbs.offset_mbinfo +
> > + mbs.mb_mods_avail * sizeof(multiboot_module_t);
> > mbs.offset_bootloader = mbs.offset_cmdlines + cmdline_len;
> > if (initrd_filename) {
> > @@ -348,22 +324,22 @@ int load_multiboot(FWCfgState *fw_cfg,
> > char kcmdline[strlen(kernel_filename) + strlen(kernel_cmdline) + 2];
> > snprintf(kcmdline, sizeof(kcmdline), "%s %s",
> > kernel_filename, kernel_cmdline);
> > - stl_p(bootinfo + MBI_CMDLINE, mb_add_cmdline(&mbs, kcmdline));
> > + stl_p(&bootinfo.cmdline, mb_add_cmdline(&mbs, kcmdline));
> > - stl_p(bootinfo + MBI_BOOTLOADER, mb_add_bootloader(&mbs, bootloader_name));
> > + stl_p(&bootinfo.boot_loader_name, mb_add_bootloader(&mbs, bootloader_name));
> > - stl_p(bootinfo + MBI_MODS_ADDR, mbs.mb_buf_phys + mbs.offset_mbinfo);
> > - stl_p(bootinfo + MBI_MODS_COUNT, mbs.mb_mods_count); /* mods_count */
> > + stl_p(&bootinfo.mods_addr, mbs.mb_buf_phys + mbs.offset_mbinfo);
> > + stl_p(&bootinfo.mods_count, mbs.mb_mods_count); /* mods_count */
> > /* the kernel is where we want it to be now */
> > - stl_p(bootinfo + MBI_FLAGS, MULTIBOOT_FLAGS_MEMORY
> > - | MULTIBOOT_FLAGS_BOOT_DEVICE
> > - | MULTIBOOT_FLAGS_CMDLINE
> > - | MULTIBOOT_FLAGS_MODULES
> > - | MULTIBOOT_FLAGS_MMAP
> > - | MULTIBOOT_FLAGS_BOOTLOADER);
> > - stl_p(bootinfo + MBI_BOOT_DEVICE, 0x8000ffff); /* XXX: use the -boot switch? */
> > - stl_p(bootinfo + MBI_MMAP_ADDR, ADDR_E820_MAP);
> > + stl_p(&bootinfo.flags, MULTIBOOT_INFO_MEMORY
> > + | MULTIBOOT_INFO_BOOTDEV
> > + | MULTIBOOT_INFO_CMDLINE
> > + | MULTIBOOT_INFO_MODS
> > + | MULTIBOOT_INFO_MEM_MAP
> > + | MULTIBOOT_INFO_BOOT_LOADER_NAME);
> > + stl_p(&bootinfo.boot_device, 0x8000ffff); /* XXX: use the -boot switch? */
> > + stl_p(&bootinfo.mmap_addr, ADDR_E820_MAP);
> > mb_debug("multiboot: mh_entry_addr = %#x\n", mh_entry_addr);
> > mb_debug(" mb_buf_phys = "TARGET_FMT_plx"\n", mbs.mb_buf_phys);
> > @@ -371,7 +347,7 @@ int load_multiboot(FWCfgState *fw_cfg,
> > mb_debug(" mb_mods_count = %d\n", mbs.mb_mods_count);
> > /* save bootinfo off the stack */
> > - mb_bootinfo_data = g_memdup(bootinfo, sizeof(bootinfo));
> > + mb_bootinfo_data = g_memdup(&bootinfo, sizeof(bootinfo));
> > /* Pass variables to option rom */
> > fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_ENTRY, mh_entry_addr);
> <snip>
> > diff --git a/tests/multiboot/mmap.c b/tests/multiboot/mmap.c
> > index 766b003f38..9cba8b07e0 100644
> > --- a/tests/multiboot/mmap.c
> > +++ b/tests/multiboot/mmap.c
> > @@ -21,15 +21,17 @@
> > */
> > #include "libc.h"
> > -#include "multiboot.h"
> > +#include "../../hw/i386/multiboot_header.h"
> Suggestion: create a multiboot subdir under include, and put
> multiboot_header.h and other multiboot includes there. This way you can
> just #include "multiboot/multiboot_header.h" which seems cleaner to me than
> "../../hw/i386/multiboot_header.h"
Good point, but do we even have multiple header files for Multiboot to
justify a whole subdirectory?
There is include/hw/loader.h and include/elf.h, so I would suggest that
we add the new header as either include/hw/multiboot.h or directly
include/multiboot.h.
Kevin
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] multiboot: Change multiboot_info from array of bytes to a C struct
2018-01-29 20:43 [Qemu-devel] [PATCH 1/4] multiboot: Change multiboot_info from array of bytes to a C struct Anatol Pomozov
@ 2018-01-30 23:15 ` Jack Schwartz
2018-01-31 9:12 ` Kevin Wolf
0 siblings, 1 reply; 33+ messages in thread
From: Jack Schwartz @ 2018-01-30 23:15 UTC (permalink / raw)
To: Anatol Pomozov, qemu-devel, kwolf
Hi Anatol and Kevin.
Even though I am new to Qemu, I have a patch to deliver for multiboot.c
(as you know) and so I feel familiar enough to do a review of this
patch. One comment is probably more for maintainers.
Comments inline...
On 01/29/18 12:43, Anatol Pomozov wrote:
> Using C structs makes the code more readable and prevents type conversion
> errors.
>
> Borrow multiboot1 header from GRUB project.
>
> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
> ---
> hw/i386/multiboot.c | 124 +++++++++------------
> hw/i386/multiboot_header.h | 254 ++++++++++++++++++++++++++++++++++++++++++++
> tests/multiboot/mmap.c | 14 +--
> tests/multiboot/mmap.out | 10 ++
> tests/multiboot/modules.c | 12 ++-
> tests/multiboot/modules.out | 10 ++
> tests/multiboot/multiboot.h | 66 ------------
> 7 files changed, 339 insertions(+), 151 deletions(-)
> create mode 100644 hw/i386/multiboot_header.h
> delete mode 100644 tests/multiboot/multiboot.h
>
> diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
> index c7b70c91d5..c6d05ca46b 100644
> --- a/hw/i386/multiboot.c
> +++ b/hw/i386/multiboot.c
> @@ -28,6 +28,7 @@
> #include "hw/hw.h"
> #include "hw/nvram/fw_cfg.h"
> #include "multiboot.h"
> +#include "multiboot_header.h"
> #include "hw/loader.h"
> #include "elf.h"
> #include "sysemu/sysemu.h"
> @@ -47,39 +48,9 @@
> #error multiboot struct needs to fit in 16 bit real mode
> #endif
>
> -enum {
> - /* Multiboot info */
> - MBI_FLAGS = 0,
> - MBI_MEM_LOWER = 4,
> - MBI_MEM_UPPER = 8,
> - MBI_BOOT_DEVICE = 12,
> - MBI_CMDLINE = 16,
> - MBI_MODS_COUNT = 20,
> - MBI_MODS_ADDR = 24,
> - MBI_MMAP_ADDR = 48,
> - MBI_BOOTLOADER = 64,
> -
> - MBI_SIZE = 88,
> -
> - /* Multiboot modules */
> - MB_MOD_START = 0,
> - MB_MOD_END = 4,
> - MB_MOD_CMDLINE = 8,
> -
> - MB_MOD_SIZE = 16,
> -
> - /* Region offsets */
> - ADDR_E820_MAP = MULTIBOOT_STRUCT_ADDR + 0,
> - ADDR_MBI = ADDR_E820_MAP + 0x500,
> -
> - /* Multiboot flags */
> - MULTIBOOT_FLAGS_MEMORY = 1 << 0,
> - MULTIBOOT_FLAGS_BOOT_DEVICE = 1 << 1,
> - MULTIBOOT_FLAGS_CMDLINE = 1 << 2,
> - MULTIBOOT_FLAGS_MODULES = 1 << 3,
> - MULTIBOOT_FLAGS_MMAP = 1 << 6,
> - MULTIBOOT_FLAGS_BOOTLOADER = 1 << 9,
> -};
> +/* Region offsets */
> +#define ADDR_E820_MAP MULTIBOOT_STRUCT_ADDR
> +#define ADDR_MBI (ADDR_E820_MAP + 0x500)
>
> typedef struct {
> /* buffer holding kernel, cmdlines and mb_infos */
> @@ -128,14 +99,15 @@ static void mb_add_mod(MultibootState *s,
> hwaddr start, hwaddr end,
> hwaddr cmdline_phys)
> {
> - char *p;
> + multiboot_module_t *mod;
> assert(s->mb_mods_count < s->mb_mods_avail);
>
> - p = (char *)s->mb_buf + s->offset_mbinfo + MB_MOD_SIZE * s->mb_mods_count;
> + mod = s->mb_buf + s->offset_mbinfo +
> + sizeof(multiboot_module_t) * s->mb_mods_count;
>
> - stl_p(p + MB_MOD_START, start);
> - stl_p(p + MB_MOD_END, end);
> - stl_p(p + MB_MOD_CMDLINE, cmdline_phys);
> + stl_p(&mod->mod_start, start);
> + stl_p(&mod->mod_end, end);
> + stl_p(&mod->cmdline, cmdline_phys);
>
> mb_debug("mod%02d: "TARGET_FMT_plx" - "TARGET_FMT_plx"\n",
> s->mb_mods_count, start, end);
> @@ -151,26 +123,29 @@ int load_multiboot(FWCfgState *fw_cfg,
> int kernel_file_size,
> uint8_t *header)
> {
> - int i, is_multiboot = 0;
> + int i;
> + bool is_multiboot = false;
> uint32_t flags = 0;
> uint32_t mh_entry_addr;
> uint32_t mh_load_addr;
> uint32_t mb_kernel_size;
> MultibootState mbs;
> - uint8_t bootinfo[MBI_SIZE];
> + multiboot_info_t bootinfo;
> uint8_t *mb_bootinfo_data;
> uint32_t cmdline_len;
> + struct multiboot_header *multiboot_header;
>
> /* Ok, let's see if it is a multiboot image.
> The header is 12x32bit long, so the latest entry may be 8192 - 48. */
> for (i = 0; i < (8192 - 48); i += 4) {
> - if (ldl_p(header+i) == 0x1BADB002) {
> - uint32_t checksum = ldl_p(header+i+8);
> - flags = ldl_p(header+i+4);
> + multiboot_header = (struct multiboot_header *)(header + i);
> + if (ldl_p(&multiboot_header->magic) == MULTIBOOT_HEADER_MAGIC) {
> + uint32_t checksum = ldl_p(&multiboot_header->checksum);
> + flags = ldl_p(&multiboot_header->flags);
> checksum += flags;
> - checksum += (uint32_t)0x1BADB002;
> + checksum += (uint32_t)MULTIBOOT_HEADER_MAGIC;
> if (!checksum) {
> - is_multiboot = 1;
> + is_multiboot = true;
> break;
> }
> }
> @@ -180,13 +155,13 @@ int load_multiboot(FWCfgState *fw_cfg,
> return 0; /* no multiboot */
>
> mb_debug("qemu: I believe we found a multiboot image!\n");
> - memset(bootinfo, 0, sizeof(bootinfo));
> + memset(&bootinfo, 0, sizeof(bootinfo));
> memset(&mbs, 0, sizeof(mbs));
>
> - if (flags & 0x00000004) { /* MULTIBOOT_HEADER_HAS_VBE */
> + if (flags & MULTIBOOT_VIDEO_MODE) {
> fprintf(stderr, "qemu: multiboot knows VBE. we don't.\n");
> }
> - if (!(flags & 0x00010000)) { /* MULTIBOOT_HEADER_HAS_ADDR */
> + if (!(flags & MULTIBOOT_AOUT_KLUDGE)) {
> uint64_t elf_entry;
> uint64_t elf_low, elf_high;
> int kernel_size;
> @@ -217,12 +192,12 @@ int load_multiboot(FWCfgState *fw_cfg,
> mb_debug("qemu: loading multiboot-elf kernel (%#x bytes) with entry %#zx\n",
> mb_kernel_size, (size_t)mh_entry_addr);
> } else {
> - /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_ADDR. */
> - uint32_t mh_header_addr = ldl_p(header+i+12);
> - uint32_t mh_load_end_addr = ldl_p(header+i+20);
> - uint32_t mh_bss_end_addr = ldl_p(header+i+24);
> + /* Valid if mh_flags sets MULTIBOOT_AOUT_KLUDGE. */
> + uint32_t mh_header_addr = ldl_p(&multiboot_header->header_addr);
> + uint32_t mh_load_end_addr = ldl_p(&multiboot_header->load_end_addr);
> + uint32_t mh_bss_end_addr = ldl_p(&multiboot_header->bss_end_addr);
> + mh_load_addr = ldl_p(&multiboot_header->load_addr);
>
> - mh_load_addr = ldl_p(header+i+16);
> if (mh_header_addr < mh_load_addr) {
> fprintf(stderr, "invalid mh_load_addr address\n");
> exit(1);
> @@ -230,7 +205,7 @@ int load_multiboot(FWCfgState *fw_cfg,
>
> uint32_t mb_kernel_text_offset = i - (mh_header_addr - mh_load_addr);
> uint32_t mb_load_size = 0;
> - mh_entry_addr = ldl_p(header+i+28);
> + mh_entry_addr = ldl_p(&multiboot_header->entry_addr);
>
> if (mh_load_end_addr) {
> if (mh_bss_end_addr < mh_load_addr) {
> @@ -253,11 +228,11 @@ int load_multiboot(FWCfgState *fw_cfg,
> mb_load_size = mb_kernel_size;
> }
>
> - /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_VBE.
> - uint32_t mh_mode_type = ldl_p(header+i+32);
> - uint32_t mh_width = ldl_p(header+i+36);
> - uint32_t mh_height = ldl_p(header+i+40);
> - uint32_t mh_depth = ldl_p(header+i+44); */
> + /* Valid if mh_flags sets MULTIBOOT_VIDEO_MODE.
> + uint32_t mh_mode_type = ldl_p(&multiboot_header->mode_type);
> + uint32_t mh_width = ldl_p(&multiboot_header->width);
> + uint32_t mh_height = ldl_p(&multiboot_header->height);
> + uint32_t mh_depth = ldl_p(&multiboot_header->depth); */
This question is probably more for maintainers...
In the patch series I submitted, people were OK that I was going to
delete these lines since they were only comments anyway. Your approach
leaves these lines in and updates them even though they are comments.
Which way is preferred?
> mb_debug("multiboot: mh_header_addr = %#x\n", mh_header_addr);
> mb_debug("multiboot: mh_load_addr = %#x\n", mh_load_addr);
> @@ -295,14 +270,15 @@ int load_multiboot(FWCfgState *fw_cfg,
> }
>
> mbs.mb_buf_size += cmdline_len;
> - mbs.mb_buf_size += MB_MOD_SIZE * mbs.mb_mods_avail;
> + mbs.mb_buf_size += sizeof(multiboot_module_t) * mbs.mb_mods_avail;
> mbs.mb_buf_size += strlen(bootloader_name) + 1;
>
> mbs.mb_buf_size = TARGET_PAGE_ALIGN(mbs.mb_buf_size);
>
> /* enlarge mb_buf to hold cmdlines, bootloader, mb-info structs */
> mbs.mb_buf = g_realloc(mbs.mb_buf, mbs.mb_buf_size);
> - mbs.offset_cmdlines = mbs.offset_mbinfo + mbs.mb_mods_avail * MB_MOD_SIZE;
> + mbs.offset_cmdlines = mbs.offset_mbinfo +
> + mbs.mb_mods_avail * sizeof(multiboot_module_t);
> mbs.offset_bootloader = mbs.offset_cmdlines + cmdline_len;
>
> if (initrd_filename) {
> @@ -348,22 +324,22 @@ int load_multiboot(FWCfgState *fw_cfg,
> char kcmdline[strlen(kernel_filename) + strlen(kernel_cmdline) + 2];
> snprintf(kcmdline, sizeof(kcmdline), "%s %s",
> kernel_filename, kernel_cmdline);
> - stl_p(bootinfo + MBI_CMDLINE, mb_add_cmdline(&mbs, kcmdline));
> + stl_p(&bootinfo.cmdline, mb_add_cmdline(&mbs, kcmdline));
>
> - stl_p(bootinfo + MBI_BOOTLOADER, mb_add_bootloader(&mbs, bootloader_name));
> + stl_p(&bootinfo.boot_loader_name, mb_add_bootloader(&mbs, bootloader_name));
>
> - stl_p(bootinfo + MBI_MODS_ADDR, mbs.mb_buf_phys + mbs.offset_mbinfo);
> - stl_p(bootinfo + MBI_MODS_COUNT, mbs.mb_mods_count); /* mods_count */
> + stl_p(&bootinfo.mods_addr, mbs.mb_buf_phys + mbs.offset_mbinfo);
> + stl_p(&bootinfo.mods_count, mbs.mb_mods_count); /* mods_count */
>
> /* the kernel is where we want it to be now */
> - stl_p(bootinfo + MBI_FLAGS, MULTIBOOT_FLAGS_MEMORY
> - | MULTIBOOT_FLAGS_BOOT_DEVICE
> - | MULTIBOOT_FLAGS_CMDLINE
> - | MULTIBOOT_FLAGS_MODULES
> - | MULTIBOOT_FLAGS_MMAP
> - | MULTIBOOT_FLAGS_BOOTLOADER);
> - stl_p(bootinfo + MBI_BOOT_DEVICE, 0x8000ffff); /* XXX: use the -boot switch? */
> - stl_p(bootinfo + MBI_MMAP_ADDR, ADDR_E820_MAP);
> + stl_p(&bootinfo.flags, MULTIBOOT_INFO_MEMORY
> + | MULTIBOOT_INFO_BOOTDEV
> + | MULTIBOOT_INFO_CMDLINE
> + | MULTIBOOT_INFO_MODS
> + | MULTIBOOT_INFO_MEM_MAP
> + | MULTIBOOT_INFO_BOOT_LOADER_NAME);
> + stl_p(&bootinfo.boot_device, 0x8000ffff); /* XXX: use the -boot switch? */
> + stl_p(&bootinfo.mmap_addr, ADDR_E820_MAP);
>
> mb_debug("multiboot: mh_entry_addr = %#x\n", mh_entry_addr);
> mb_debug(" mb_buf_phys = "TARGET_FMT_plx"\n", mbs.mb_buf_phys);
> @@ -371,7 +347,7 @@ int load_multiboot(FWCfgState *fw_cfg,
> mb_debug(" mb_mods_count = %d\n", mbs.mb_mods_count);
>
> /* save bootinfo off the stack */
> - mb_bootinfo_data = g_memdup(bootinfo, sizeof(bootinfo));
> + mb_bootinfo_data = g_memdup(&bootinfo, sizeof(bootinfo));
>
> /* Pass variables to option rom */
> fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_ENTRY, mh_entry_addr);
<snip>
> diff --git a/tests/multiboot/mmap.c b/tests/multiboot/mmap.c
> index 766b003f38..9cba8b07e0 100644
> --- a/tests/multiboot/mmap.c
> +++ b/tests/multiboot/mmap.c
> @@ -21,15 +21,17 @@
> */
>
> #include "libc.h"
> -#include "multiboot.h"
> +#include "../../hw/i386/multiboot_header.h"
Suggestion: create a multiboot subdir under include, and put
multiboot_header.h and other multiboot includes there. This way you can
just #include "multiboot/multiboot_header.h" which seems cleaner to me
than "../../hw/i386/multiboot_header.h"
> -int test_main(uint32_t magic, struct mb_info *mbi)
> +int test_main(uint32_t magic, struct multiboot_info *mbi)
> {
> uintptr_t entry_addr;
> - struct mb_mmap_entry *entry;
> + struct multiboot_mmap_entry *entry;
>
> (void) magic;
>
> + printf("Multiboot header at %x\n\n", mbi);
> +
> printf("Lower memory: %dk\n", mbi->mem_lower);
> printf("Upper memory: %dk\n", mbi->mem_upper);
>
> @@ -39,11 +41,11 @@ int test_main(uint32_t magic, struct mb_info *mbi)
> entry_addr < mbi->mmap_addr + mbi->mmap_length;
> entry_addr += entry->size + 4)
> {
> - entry = (struct mb_mmap_entry*) entry_addr;
> + entry = (struct multiboot_mmap_entry*) entry_addr;
>
> printf("%#llx - %#llx: type %d [entry size: %d]\n",
> - entry->base_addr,
> - entry->base_addr + entry->length,
> + entry->addr,
> + entry->addr + entry->len,
> entry->type,
> entry->size);
> }
<snip>
>
> diff --git a/tests/multiboot/modules.c b/tests/multiboot/modules.c
> index 531601fb30..a1da0ded44 100644
> --- a/tests/multiboot/modules.c
> +++ b/tests/multiboot/modules.c
> @@ -21,19 +21,21 @@
> */
>
> #include "libc.h"
> -#include "multiboot.h"
> +#include "../../hw/i386/multiboot_header.h"
Same comment about putting multiboot_header.h into a multiboot subdir in
"include" subtree.
I have this same comment, as well as one other, for patch 2 sent separately.
Thanks,
Jack
> -int test_main(uint32_t magic, struct mb_info *mbi)
> +int test_main(uint32_t magic, struct multiboot_info *mbi)
> {
> - struct mb_module *mod;
> + struct multiboot_mod_list *mod;
> unsigned int i;
>
> (void) magic;
>
> + printf("Multiboot header at %x\n\n", mbi);
> +
> printf("Module list with %d entries at %x\n",
> mbi->mods_count, mbi->mods_addr);
>
> - for (i = 0, mod = (struct mb_module*) mbi->mods_addr;
> + for (i = 0, mod = (struct multiboot_mod_list*) mbi->mods_addr;
> i < mbi->mods_count;
> i++, mod++)
> {
> @@ -41,7 +43,7 @@ int test_main(uint32_t magic, struct mb_info *mbi)
> unsigned int size = mod->mod_end - mod->mod_start;
>
> printf("[%p] Module: %x - %x (%d bytes) '%s'\n",
> - mod, mod->mod_start, mod->mod_end, size, mod->string);
> + mod, mod->mod_start, mod->mod_end, size, mod->cmdline);
>
> /* Print test file, but remove the newline at the end */
> if (size < sizeof(buf)) {
>
<snip>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH 1/4] multiboot: Change multiboot_info from array of bytes to a C struct
@ 2018-01-29 20:43 Anatol Pomozov
2018-01-30 23:15 ` Jack Schwartz
0 siblings, 1 reply; 33+ messages in thread
From: Anatol Pomozov @ 2018-01-29 20:43 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, jack.schwartz, Anatol Pomozov
Using C structs makes the code more readable and prevents type conversion
errors.
Borrow multiboot1 header from GRUB project.
Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
---
hw/i386/multiboot.c | 124 +++++++++------------
| 254 ++++++++++++++++++++++++++++++++++++++++++++
tests/multiboot/mmap.c | 14 +--
tests/multiboot/mmap.out | 10 ++
tests/multiboot/modules.c | 12 ++-
tests/multiboot/modules.out | 10 ++
tests/multiboot/multiboot.h | 66 ------------
7 files changed, 339 insertions(+), 151 deletions(-)
create mode 100644 hw/i386/multiboot_header.h
delete mode 100644 tests/multiboot/multiboot.h
diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c
index c7b70c91d5..c6d05ca46b 100644
--- a/hw/i386/multiboot.c
+++ b/hw/i386/multiboot.c
@@ -28,6 +28,7 @@
#include "hw/hw.h"
#include "hw/nvram/fw_cfg.h"
#include "multiboot.h"
+#include "multiboot_header.h"
#include "hw/loader.h"
#include "elf.h"
#include "sysemu/sysemu.h"
@@ -47,39 +48,9 @@
#error multiboot struct needs to fit in 16 bit real mode
#endif
-enum {
- /* Multiboot info */
- MBI_FLAGS = 0,
- MBI_MEM_LOWER = 4,
- MBI_MEM_UPPER = 8,
- MBI_BOOT_DEVICE = 12,
- MBI_CMDLINE = 16,
- MBI_MODS_COUNT = 20,
- MBI_MODS_ADDR = 24,
- MBI_MMAP_ADDR = 48,
- MBI_BOOTLOADER = 64,
-
- MBI_SIZE = 88,
-
- /* Multiboot modules */
- MB_MOD_START = 0,
- MB_MOD_END = 4,
- MB_MOD_CMDLINE = 8,
-
- MB_MOD_SIZE = 16,
-
- /* Region offsets */
- ADDR_E820_MAP = MULTIBOOT_STRUCT_ADDR + 0,
- ADDR_MBI = ADDR_E820_MAP + 0x500,
-
- /* Multiboot flags */
- MULTIBOOT_FLAGS_MEMORY = 1 << 0,
- MULTIBOOT_FLAGS_BOOT_DEVICE = 1 << 1,
- MULTIBOOT_FLAGS_CMDLINE = 1 << 2,
- MULTIBOOT_FLAGS_MODULES = 1 << 3,
- MULTIBOOT_FLAGS_MMAP = 1 << 6,
- MULTIBOOT_FLAGS_BOOTLOADER = 1 << 9,
-};
+/* Region offsets */
+#define ADDR_E820_MAP MULTIBOOT_STRUCT_ADDR
+#define ADDR_MBI (ADDR_E820_MAP + 0x500)
typedef struct {
/* buffer holding kernel, cmdlines and mb_infos */
@@ -128,14 +99,15 @@ static void mb_add_mod(MultibootState *s,
hwaddr start, hwaddr end,
hwaddr cmdline_phys)
{
- char *p;
+ multiboot_module_t *mod;
assert(s->mb_mods_count < s->mb_mods_avail);
- p = (char *)s->mb_buf + s->offset_mbinfo + MB_MOD_SIZE * s->mb_mods_count;
+ mod = s->mb_buf + s->offset_mbinfo +
+ sizeof(multiboot_module_t) * s->mb_mods_count;
- stl_p(p + MB_MOD_START, start);
- stl_p(p + MB_MOD_END, end);
- stl_p(p + MB_MOD_CMDLINE, cmdline_phys);
+ stl_p(&mod->mod_start, start);
+ stl_p(&mod->mod_end, end);
+ stl_p(&mod->cmdline, cmdline_phys);
mb_debug("mod%02d: "TARGET_FMT_plx" - "TARGET_FMT_plx"\n",
s->mb_mods_count, start, end);
@@ -151,26 +123,29 @@ int load_multiboot(FWCfgState *fw_cfg,
int kernel_file_size,
uint8_t *header)
{
- int i, is_multiboot = 0;
+ int i;
+ bool is_multiboot = false;
uint32_t flags = 0;
uint32_t mh_entry_addr;
uint32_t mh_load_addr;
uint32_t mb_kernel_size;
MultibootState mbs;
- uint8_t bootinfo[MBI_SIZE];
+ multiboot_info_t bootinfo;
uint8_t *mb_bootinfo_data;
uint32_t cmdline_len;
+ struct multiboot_header *multiboot_header;
/* Ok, let's see if it is a multiboot image.
The header is 12x32bit long, so the latest entry may be 8192 - 48. */
for (i = 0; i < (8192 - 48); i += 4) {
- if (ldl_p(header+i) == 0x1BADB002) {
- uint32_t checksum = ldl_p(header+i+8);
- flags = ldl_p(header+i+4);
+ multiboot_header = (struct multiboot_header *)(header + i);
+ if (ldl_p(&multiboot_header->magic) == MULTIBOOT_HEADER_MAGIC) {
+ uint32_t checksum = ldl_p(&multiboot_header->checksum);
+ flags = ldl_p(&multiboot_header->flags);
checksum += flags;
- checksum += (uint32_t)0x1BADB002;
+ checksum += (uint32_t)MULTIBOOT_HEADER_MAGIC;
if (!checksum) {
- is_multiboot = 1;
+ is_multiboot = true;
break;
}
}
@@ -180,13 +155,13 @@ int load_multiboot(FWCfgState *fw_cfg,
return 0; /* no multiboot */
mb_debug("qemu: I believe we found a multiboot image!\n");
- memset(bootinfo, 0, sizeof(bootinfo));
+ memset(&bootinfo, 0, sizeof(bootinfo));
memset(&mbs, 0, sizeof(mbs));
- if (flags & 0x00000004) { /* MULTIBOOT_HEADER_HAS_VBE */
+ if (flags & MULTIBOOT_VIDEO_MODE) {
fprintf(stderr, "qemu: multiboot knows VBE. we don't.\n");
}
- if (!(flags & 0x00010000)) { /* MULTIBOOT_HEADER_HAS_ADDR */
+ if (!(flags & MULTIBOOT_AOUT_KLUDGE)) {
uint64_t elf_entry;
uint64_t elf_low, elf_high;
int kernel_size;
@@ -217,12 +192,12 @@ int load_multiboot(FWCfgState *fw_cfg,
mb_debug("qemu: loading multiboot-elf kernel (%#x bytes) with entry %#zx\n",
mb_kernel_size, (size_t)mh_entry_addr);
} else {
- /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_ADDR. */
- uint32_t mh_header_addr = ldl_p(header+i+12);
- uint32_t mh_load_end_addr = ldl_p(header+i+20);
- uint32_t mh_bss_end_addr = ldl_p(header+i+24);
+ /* Valid if mh_flags sets MULTIBOOT_AOUT_KLUDGE. */
+ uint32_t mh_header_addr = ldl_p(&multiboot_header->header_addr);
+ uint32_t mh_load_end_addr = ldl_p(&multiboot_header->load_end_addr);
+ uint32_t mh_bss_end_addr = ldl_p(&multiboot_header->bss_end_addr);
+ mh_load_addr = ldl_p(&multiboot_header->load_addr);
- mh_load_addr = ldl_p(header+i+16);
if (mh_header_addr < mh_load_addr) {
fprintf(stderr, "invalid mh_load_addr address\n");
exit(1);
@@ -230,7 +205,7 @@ int load_multiboot(FWCfgState *fw_cfg,
uint32_t mb_kernel_text_offset = i - (mh_header_addr - mh_load_addr);
uint32_t mb_load_size = 0;
- mh_entry_addr = ldl_p(header+i+28);
+ mh_entry_addr = ldl_p(&multiboot_header->entry_addr);
if (mh_load_end_addr) {
if (mh_bss_end_addr < mh_load_addr) {
@@ -253,11 +228,11 @@ int load_multiboot(FWCfgState *fw_cfg,
mb_load_size = mb_kernel_size;
}
- /* Valid if mh_flags sets MULTIBOOT_HEADER_HAS_VBE.
- uint32_t mh_mode_type = ldl_p(header+i+32);
- uint32_t mh_width = ldl_p(header+i+36);
- uint32_t mh_height = ldl_p(header+i+40);
- uint32_t mh_depth = ldl_p(header+i+44); */
+ /* Valid if mh_flags sets MULTIBOOT_VIDEO_MODE.
+ uint32_t mh_mode_type = ldl_p(&multiboot_header->mode_type);
+ uint32_t mh_width = ldl_p(&multiboot_header->width);
+ uint32_t mh_height = ldl_p(&multiboot_header->height);
+ uint32_t mh_depth = ldl_p(&multiboot_header->depth); */
mb_debug("multiboot: mh_header_addr = %#x\n", mh_header_addr);
mb_debug("multiboot: mh_load_addr = %#x\n", mh_load_addr);
@@ -295,14 +270,15 @@ int load_multiboot(FWCfgState *fw_cfg,
}
mbs.mb_buf_size += cmdline_len;
- mbs.mb_buf_size += MB_MOD_SIZE * mbs.mb_mods_avail;
+ mbs.mb_buf_size += sizeof(multiboot_module_t) * mbs.mb_mods_avail;
mbs.mb_buf_size += strlen(bootloader_name) + 1;
mbs.mb_buf_size = TARGET_PAGE_ALIGN(mbs.mb_buf_size);
/* enlarge mb_buf to hold cmdlines, bootloader, mb-info structs */
mbs.mb_buf = g_realloc(mbs.mb_buf, mbs.mb_buf_size);
- mbs.offset_cmdlines = mbs.offset_mbinfo + mbs.mb_mods_avail * MB_MOD_SIZE;
+ mbs.offset_cmdlines = mbs.offset_mbinfo +
+ mbs.mb_mods_avail * sizeof(multiboot_module_t);
mbs.offset_bootloader = mbs.offset_cmdlines + cmdline_len;
if (initrd_filename) {
@@ -348,22 +324,22 @@ int load_multiboot(FWCfgState *fw_cfg,
char kcmdline[strlen(kernel_filename) + strlen(kernel_cmdline) + 2];
snprintf(kcmdline, sizeof(kcmdline), "%s %s",
kernel_filename, kernel_cmdline);
- stl_p(bootinfo + MBI_CMDLINE, mb_add_cmdline(&mbs, kcmdline));
+ stl_p(&bootinfo.cmdline, mb_add_cmdline(&mbs, kcmdline));
- stl_p(bootinfo + MBI_BOOTLOADER, mb_add_bootloader(&mbs, bootloader_name));
+ stl_p(&bootinfo.boot_loader_name, mb_add_bootloader(&mbs, bootloader_name));
- stl_p(bootinfo + MBI_MODS_ADDR, mbs.mb_buf_phys + mbs.offset_mbinfo);
- stl_p(bootinfo + MBI_MODS_COUNT, mbs.mb_mods_count); /* mods_count */
+ stl_p(&bootinfo.mods_addr, mbs.mb_buf_phys + mbs.offset_mbinfo);
+ stl_p(&bootinfo.mods_count, mbs.mb_mods_count); /* mods_count */
/* the kernel is where we want it to be now */
- stl_p(bootinfo + MBI_FLAGS, MULTIBOOT_FLAGS_MEMORY
- | MULTIBOOT_FLAGS_BOOT_DEVICE
- | MULTIBOOT_FLAGS_CMDLINE
- | MULTIBOOT_FLAGS_MODULES
- | MULTIBOOT_FLAGS_MMAP
- | MULTIBOOT_FLAGS_BOOTLOADER);
- stl_p(bootinfo + MBI_BOOT_DEVICE, 0x8000ffff); /* XXX: use the -boot switch? */
- stl_p(bootinfo + MBI_MMAP_ADDR, ADDR_E820_MAP);
+ stl_p(&bootinfo.flags, MULTIBOOT_INFO_MEMORY
+ | MULTIBOOT_INFO_BOOTDEV
+ | MULTIBOOT_INFO_CMDLINE
+ | MULTIBOOT_INFO_MODS
+ | MULTIBOOT_INFO_MEM_MAP
+ | MULTIBOOT_INFO_BOOT_LOADER_NAME);
+ stl_p(&bootinfo.boot_device, 0x8000ffff); /* XXX: use the -boot switch? */
+ stl_p(&bootinfo.mmap_addr, ADDR_E820_MAP);
mb_debug("multiboot: mh_entry_addr = %#x\n", mh_entry_addr);
mb_debug(" mb_buf_phys = "TARGET_FMT_plx"\n", mbs.mb_buf_phys);
@@ -371,7 +347,7 @@ int load_multiboot(FWCfgState *fw_cfg,
mb_debug(" mb_mods_count = %d\n", mbs.mb_mods_count);
/* save bootinfo off the stack */
- mb_bootinfo_data = g_memdup(bootinfo, sizeof(bootinfo));
+ mb_bootinfo_data = g_memdup(&bootinfo, sizeof(bootinfo));
/* Pass variables to option rom */
fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_ENTRY, mh_entry_addr);
--git a/hw/i386/multiboot_header.h b/hw/i386/multiboot_header.h
new file mode 100644
index 0000000000..563014698c
--- /dev/null
+++ b/hw/i386/multiboot_header.h
@@ -0,0 +1,254 @@
+/* multiboot.h - Multiboot header file. */
+/* Copyright (C) 1999,2003,2007,2008,2009,2010 Free Software Foundation, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL ANY
+ * DEVELOPER OR DISTRIBUTOR BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR
+ * IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#ifndef MULTIBOOT_HEADER
+#define MULTIBOOT_HEADER 1
+
+/* How many bytes from the start of the file we search for the header. */
+#define MULTIBOOT_SEARCH 8192
+#define MULTIBOOT_HEADER_ALIGN 4
+
+/* The magic field should contain this. */
+#define MULTIBOOT_HEADER_MAGIC 0x1BADB002
+
+/* This should be in %eax. */
+#define MULTIBOOT_BOOTLOADER_MAGIC 0x2BADB002
+
+/* Alignment of multiboot modules. */
+#define MULTIBOOT_MOD_ALIGN 0x00001000
+
+/* Alignment of the multiboot info structure. */
+#define MULTIBOOT_INFO_ALIGN 0x00000004
+
+/* Flags set in the 'flags' member of the multiboot header. */
+
+/* Align all boot modules on i386 page (4KB) boundaries. */
+#define MULTIBOOT_PAGE_ALIGN 0x00000001
+
+/* Must pass memory information to OS. */
+#define MULTIBOOT_MEMORY_INFO 0x00000002
+
+/* Must pass video information to OS. */
+#define MULTIBOOT_VIDEO_MODE 0x00000004
+
+/* This flag indicates the use of the address fields in the header. */
+#define MULTIBOOT_AOUT_KLUDGE 0x00010000
+
+/* Flags to be set in the 'flags' member of the multiboot info structure. */
+
+/* is there basic lower/upper memory information? */
+#define MULTIBOOT_INFO_MEMORY 0x00000001
+/* is there a boot device set? */
+#define MULTIBOOT_INFO_BOOTDEV 0x00000002
+/* is the command-line defined? */
+#define MULTIBOOT_INFO_CMDLINE 0x00000004
+/* are there modules to do something with? */
+#define MULTIBOOT_INFO_MODS 0x00000008
+
+/* These next two are mutually exclusive */
+
+/* is there a symbol table loaded? */
+#define MULTIBOOT_INFO_AOUT_SYMS 0x00000010
+/* is there an ELF section header table? */
+#define MULTIBOOT_INFO_ELF_SHDR 0X00000020
+
+/* is there a full memory map? */
+#define MULTIBOOT_INFO_MEM_MAP 0x00000040
+
+/* Is there drive info? */
+#define MULTIBOOT_INFO_DRIVE_INFO 0x00000080
+
+/* Is there a config table? */
+#define MULTIBOOT_INFO_CONFIG_TABLE 0x00000100
+
+/* Is there a boot loader name? */
+#define MULTIBOOT_INFO_BOOT_LOADER_NAME 0x00000200
+
+/* Is there a APM table? */
+#define MULTIBOOT_INFO_APM_TABLE 0x00000400
+
+/* Is there video information? */
+#define MULTIBOOT_INFO_VBE_INFO 0x00000800
+#define MULTIBOOT_INFO_FRAMEBUFFER_INFO 0x00001000
+
+struct multiboot_header {
+ /* Must be MULTIBOOT_MAGIC - see above. */
+ uint32_t magic;
+
+ /* Feature flags. */
+ uint32_t flags;
+
+ /* The above fields plus this one must equal 0 mod 2^32. */
+ uint32_t checksum;
+
+ /* These are only valid if MULTIBOOT_AOUT_KLUDGE is set. */
+ uint32_t header_addr;
+ uint32_t load_addr;
+ uint32_t load_end_addr;
+ uint32_t bss_end_addr;
+ uint32_t entry_addr;
+
+ /* These are only valid if MULTIBOOT_VIDEO_MODE is set. */
+ uint32_t mode_type;
+ uint32_t width;
+ uint32_t height;
+ uint32_t depth;
+};
+
+/* The symbol table for a.out. */
+struct multiboot_aout_symbol_table {
+ uint32_t tabsize;
+ uint32_t strsize;
+ uint32_t addr;
+ uint32_t reserved;
+};
+typedef struct multiboot_aout_symbol_table multiboot_aout_symbol_table_t;
+
+/* The section header table for ELF. */
+struct multiboot_elf_section_header_table {
+ uint32_t num;
+ uint32_t size;
+ uint32_t addr;
+ uint32_t shndx;
+};
+typedef struct multiboot_elf_section_header_table
+ multiboot_elf_section_header_table_t;
+
+struct multiboot_info {
+ /* Multiboot info version number */
+ uint32_t flags;
+
+ /* Available memory from BIOS */
+ uint32_t mem_lower;
+ uint32_t mem_upper;
+
+ /* "root" partition */
+ uint32_t boot_device;
+
+ /* Kernel command line */
+ uint32_t cmdline;
+
+ /* Boot-Module list */
+ uint32_t mods_count;
+ uint32_t mods_addr;
+
+ union {
+ multiboot_aout_symbol_table_t aout_sym;
+ multiboot_elf_section_header_table_t elf_sec;
+ } u;
+
+ /* Memory Mapping buffer */
+ uint32_t mmap_length;
+ uint32_t mmap_addr;
+
+ /* Drive Info buffer */
+ uint32_t drives_length;
+ uint32_t drives_addr;
+
+ /* ROM configuration table */
+ uint32_t config_table;
+
+ /* Boot Loader Name */
+ uint32_t boot_loader_name;
+
+ /* APM table */
+ uint32_t apm_table;
+
+ /* Video */
+ uint32_t vbe_control_info;
+ uint32_t vbe_mode_info;
+ uint16_t vbe_mode;
+ uint16_t vbe_interface_seg;
+ uint16_t vbe_interface_off;
+ uint16_t vbe_interface_len;
+
+ uint64_t framebuffer_addr;
+ uint32_t framebuffer_pitch;
+ uint32_t framebuffer_width;
+ uint32_t framebuffer_height;
+ uint8_t framebuffer_bpp;
+#define MULTIBOOT_FRAMEBUFFER_TYPE_INDEXED 0
+#define MULTIBOOT_FRAMEBUFFER_TYPE_RGB 1
+#define MULTIBOOT_FRAMEBUFFER_TYPE_EGA_TEXT 2
+ uint8_t framebuffer_type;
+ union {
+ struct {
+ uint32_t framebuffer_palette_addr;
+ uint16_t framebuffer_palette_num_colors;
+ };
+ struct {
+ uint8_t framebuffer_red_field_position;
+ uint8_t framebuffer_red_mask_size;
+ uint8_t framebuffer_green_field_position;
+ uint8_t framebuffer_green_mask_size;
+ uint8_t framebuffer_blue_field_position;
+ uint8_t framebuffer_blue_mask_size;
+ };
+ };
+};
+typedef struct multiboot_info multiboot_info_t;
+
+struct multiboot_color {
+ uint8_t red;
+ uint8_t green;
+ uint8_t blue;
+};
+
+struct multiboot_mmap_entry {
+ uint32_t size;
+ uint64_t addr;
+ uint64_t len;
+#define MULTIBOOT_MEMORY_AVAILABLE 1
+#define MULTIBOOT_MEMORY_RESERVED 2
+#define MULTIBOOT_MEMORY_ACPI_RECLAIMABLE 3
+#define MULTIBOOT_MEMORY_NVS 4
+#define MULTIBOOT_MEMORY_BADRAM 5
+ uint32_t type;
+} __attribute__ ((packed));
+typedef struct multiboot_mmap_entry multiboot_memory_map_t;
+
+struct multiboot_mod_list {
+ /* the memory used goes from bytes 'mod_start' to 'mod_end-1' inclusive */
+ uint32_t mod_start;
+ uint32_t mod_end;
+
+ /* Module command line */
+ uint32_t cmdline;
+
+ /* padding to take it to 16 bytes (must be zero) */
+ uint32_t pad;
+};
+typedef struct multiboot_mod_list multiboot_module_t;
+
+/* APM BIOS info. */
+struct multiboot_apm_info {
+ uint16_t version;
+ uint16_t cseg;
+ uint32_t offset;
+ uint16_t cseg_16;
+ uint16_t dseg;
+ uint16_t flags;
+ uint16_t cseg_len;
+ uint16_t cseg_16_len;
+ uint16_t dseg_len;
+};
+
+#endif /* ! MULTIBOOT_HEADER */
diff --git a/tests/multiboot/mmap.c b/tests/multiboot/mmap.c
index 766b003f38..9cba8b07e0 100644
--- a/tests/multiboot/mmap.c
+++ b/tests/multiboot/mmap.c
@@ -21,15 +21,17 @@
*/
#include "libc.h"
-#include "multiboot.h"
+#include "../../hw/i386/multiboot_header.h"
-int test_main(uint32_t magic, struct mb_info *mbi)
+int test_main(uint32_t magic, struct multiboot_info *mbi)
{
uintptr_t entry_addr;
- struct mb_mmap_entry *entry;
+ struct multiboot_mmap_entry *entry;
(void) magic;
+ printf("Multiboot header at %x\n\n", mbi);
+
printf("Lower memory: %dk\n", mbi->mem_lower);
printf("Upper memory: %dk\n", mbi->mem_upper);
@@ -39,11 +41,11 @@ int test_main(uint32_t magic, struct mb_info *mbi)
entry_addr < mbi->mmap_addr + mbi->mmap_length;
entry_addr += entry->size + 4)
{
- entry = (struct mb_mmap_entry*) entry_addr;
+ entry = (struct multiboot_mmap_entry*) entry_addr;
printf("%#llx - %#llx: type %d [entry size: %d]\n",
- entry->base_addr,
- entry->base_addr + entry->length,
+ entry->addr,
+ entry->addr + entry->len,
entry->type,
entry->size);
}
diff --git a/tests/multiboot/mmap.out b/tests/multiboot/mmap.out
index 003e109b4c..e3a28237ab 100644
--- a/tests/multiboot/mmap.out
+++ b/tests/multiboot/mmap.out
@@ -3,6 +3,8 @@
=== Running test case: mmap.elf ===
+Multiboot header at 9500
+
Lower memory: 639k
Upper memory: 129920k
@@ -21,6 +23,8 @@ real mmap end: 0x9090
=== Running test case: mmap.elf -m 1.1M ===
+Multiboot header at 9500
+
Lower memory: 639k
Upper memory: 104k
@@ -38,6 +42,8 @@ real mmap end: 0x9078
=== Running test case: mmap.elf -m 2G ===
+Multiboot header at 9500
+
Lower memory: 639k
Upper memory: 2096000k
@@ -56,6 +62,8 @@ real mmap end: 0x9090
=== Running test case: mmap.elf -m 4G ===
+Multiboot header at 9500
+
Lower memory: 639k
Upper memory: 3144576k
@@ -75,6 +83,8 @@ real mmap end: 0x90a8
=== Running test case: mmap.elf -m 8G ===
+Multiboot header at 9500
+
Lower memory: 639k
Upper memory: 3144576k
diff --git a/tests/multiboot/modules.c b/tests/multiboot/modules.c
index 531601fb30..a1da0ded44 100644
--- a/tests/multiboot/modules.c
+++ b/tests/multiboot/modules.c
@@ -21,19 +21,21 @@
*/
#include "libc.h"
-#include "multiboot.h"
+#include "../../hw/i386/multiboot_header.h"
-int test_main(uint32_t magic, struct mb_info *mbi)
+int test_main(uint32_t magic, struct multiboot_info *mbi)
{
- struct mb_module *mod;
+ struct multiboot_mod_list *mod;
unsigned int i;
(void) magic;
+ printf("Multiboot header at %x\n\n", mbi);
+
printf("Module list with %d entries at %x\n",
mbi->mods_count, mbi->mods_addr);
- for (i = 0, mod = (struct mb_module*) mbi->mods_addr;
+ for (i = 0, mod = (struct multiboot_mod_list*) mbi->mods_addr;
i < mbi->mods_count;
i++, mod++)
{
@@ -41,7 +43,7 @@ int test_main(uint32_t magic, struct mb_info *mbi)
unsigned int size = mod->mod_end - mod->mod_start;
printf("[%p] Module: %x - %x (%d bytes) '%s'\n",
- mod, mod->mod_start, mod->mod_end, size, mod->string);
+ mod, mod->mod_start, mod->mod_end, size, mod->cmdline);
/* Print test file, but remove the newline at the end */
if (size < sizeof(buf)) {
diff --git a/tests/multiboot/modules.out b/tests/multiboot/modules.out
index 1636708035..0da7b39374 100644
--- a/tests/multiboot/modules.out
+++ b/tests/multiboot/modules.out
@@ -3,11 +3,15 @@
=== Running test case: modules.elf ===
+Multiboot header at 9500
+
Module list with 0 entries at 102000
=== Running test case: modules.elf -initrd module.txt ===
+Multiboot header at 9500
+
Module list with 1 entries at 102000
[102000] Module: 103000 - 103038 (56 bytes) 'module.txt'
Content: 'This is a test file that is used as a multiboot module.'
@@ -15,6 +19,8 @@ Module list with 1 entries at 102000
=== Running test case: modules.elf -initrd module.txt argument ===
+Multiboot header at 9500
+
Module list with 1 entries at 102000
[102000] Module: 103000 - 103038 (56 bytes) 'module.txt argument'
Content: 'This is a test file that is used as a multiboot module.'
@@ -22,6 +28,8 @@ Module list with 1 entries at 102000
=== Running test case: modules.elf -initrd module.txt argument,,with,,commas ===
+Multiboot header at 9500
+
Module list with 1 entries at 102000
[102000] Module: 103000 - 103038 (56 bytes) 'module.txt argument,with,commas'
Content: 'This is a test file that is used as a multiboot module.'
@@ -29,6 +37,8 @@ Module list with 1 entries at 102000
=== Running test case: modules.elf -initrd module.txt,module.txt argument,module.txt ===
+Multiboot header at 9500
+
Module list with 3 entries at 102000
[102000] Module: 103000 - 103038 (56 bytes) 'module.txt'
Content: 'This is a test file that is used as a multiboot module.'
diff --git a/tests/multiboot/multiboot.h b/tests/multiboot/multiboot.h
deleted file mode 100644
index 4eb1fbe5d4..0000000000
--- a/tests/multiboot/multiboot.h
+++ /dev/null
@@ -1,66 +0,0 @@
-/*
- * Copyright (c) 2013 Kevin Wolf <kwolf@redhat.com>
- *
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to deal
- * in the Software without restriction, including without limitation the rights
- * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
- * copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
- * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
- * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
- * THE SOFTWARE.
- */
-
-#ifndef MULTIBOOT_H
-#define MULTIBOOT_H
-
-#include "libc.h"
-
-struct mb_info {
- uint32_t flags;
- uint32_t mem_lower;
- uint32_t mem_upper;
- uint32_t boot_device;
- uint32_t cmdline;
- uint32_t mods_count;
- uint32_t mods_addr;
- char syms[16];
- uint32_t mmap_length;
- uint32_t mmap_addr;
- uint32_t drives_length;
- uint32_t drives_addr;
- uint32_t config_table;
- uint32_t boot_loader_name;
- uint32_t apm_table;
- uint32_t vbe_control_info;
- uint32_t vbe_mode_info;
- uint16_t vbe_mode;
- uint16_t vbe_interface_seg;
- uint16_t vbe_interface_off;
- uint16_t vbe_interface_len;
-} __attribute__((packed));
-
-struct mb_module {
- uint32_t mod_start;
- uint32_t mod_end;
- uint32_t string;
- uint32_t reserved;
-} __attribute__((packed));
-
-struct mb_mmap_entry {
- uint32_t size;
- uint64_t base_addr;
- uint64_t length;
- uint32_t type;
-} __attribute__((packed));
-
-#endif
--
2.16.0.rc1.238.g530d649a79-goog
^ permalink raw reply related [flat|nested] 33+ messages in thread
end of thread, other threads:[~2018-02-12 13:38 UTC | newest]
Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-12 23:54 [Qemu-devel] (no subject) Anatol Pomozov
2017-10-12 23:54 ` [Qemu-devel] [PATCH 1/4] multiboot: Change multiboot_info from array of bytes to a C struct Anatol Pomozov
2018-01-15 14:49 ` Kevin Wolf
2018-01-29 18:21 ` Anatol Pomozov
2018-01-29 20:02 ` Kevin Wolf
2018-02-09 21:48 ` Anatol Pomozov
2018-02-09 21:52 ` Anatol Pomozov
2018-02-12 13:33 ` Kevin Wolf
2018-02-12 13:37 ` Kevin Wolf
2017-10-12 23:54 ` [Qemu-devel] [PATCH 2/4] multiboot: load any machine type of ELF Anatol Pomozov
2017-10-13 19:25 ` Eduardo Habkost
2017-10-13 21:25 ` Anatol Pomozov
2017-10-13 23:21 ` Eduardo Habkost
2017-10-14 13:41 ` Peter Maydell
2017-10-16 8:27 ` Kevin Wolf
2017-10-16 18:38 ` Anatol Pomozov
2018-01-15 14:52 ` Kevin Wolf
2018-01-29 18:35 ` Anatol Pomozov
2017-10-12 23:54 ` [Qemu-devel] [PATCH 3/4] multiboot: load elf sections and section headers Anatol Pomozov
2017-10-18 17:22 ` Anatol Pomozov
2017-10-19 9:36 ` Kevin Wolf
2017-10-31 18:38 ` Anatol Pomozov
2017-11-17 21:33 ` Anatol Pomozov
2017-11-20 16:45 ` Kevin Wolf
2018-01-15 15:41 ` Kevin Wolf
2018-01-29 19:16 ` Anatol Pomozov
2017-10-12 23:54 ` [Qemu-devel] [PATCH 4/4] multiboot: make tests work with clang Anatol Pomozov
2017-10-13 8:01 ` Thomas Huth
2018-01-29 20:43 [Qemu-devel] [PATCH 1/4] multiboot: Change multiboot_info from array of bytes to a C struct Anatol Pomozov
2018-01-30 23:15 ` Jack Schwartz
2018-01-31 9:12 ` Kevin Wolf
2018-02-05 21:43 ` Anatol Pomozov
2018-02-06 20:35 ` Jack Schwartz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.