On Tue, Sep 23, 2008 at 04:11:34PM +0200, Mathias Krause wrote: > > I see. But why not just add "ecx" to clobber list instead? Then a dummy > > variable isn't needed. > > When doing so the compiler is unable to find another register called > %ecx to assign the value of f as input value. He cannot use %ecx because > by mentioning it in the clobber list you already told him that register > may change it's value at any time within the asm statement. That for the > "kudge" with the dummy output variable is needed but it should get > optimized away since it's value is never used. It's only usage is to > tell the compiler that the value of %ecx has changed.
Ok. > >> The memory clobber is needed since you cannot know what the called > >> function will actually do with the memory and to ensure all cached > >> values are actually written back to memory before calling f(). > > > > Is this really a problem? If the payload expects to return, it isn't > > supposed to modify coreboot's memory at all. If it does, I'd say it's > > normal that things break. > > Not the return of f() is the problem but missing memory writes _before_ > calling it. If coreboot does some memory modifications that are relevant > for f() they should be taken out of registers and written back to memory > before calling f(). Otherwise f() may break. Makes sense. Thanks for the explanation. Here's a new patch incorporating your suggestions. -- Robert Millan The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and how) you may access your data; but nobody's threatening your freedom: we still allow you to remove your data and not access it at all."
Signed-off-by: Robert Millan <[EMAIL PROTECTED]> Index: include/arch/x86/multiboot.h =================================================================== --- include/arch/x86/multiboot.h (revision 0) +++ include/arch/x86/multiboot.h (revision 0) @@ -0,0 +1,179 @@ +/* multiboot.h - multiboot header file. */ +/* + * Copyright (C) 2003 Free Software Foundation, Inc. + * Copyright (C) 2008 Robert Millan + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, see <http://www.gnu.org/licenses/>. + */ + +#ifndef MULTIBOOT_H +#define MULTIBOOT_H + +#include <types.h> +typedef u16 uint16_t; +typedef u32 uint32_t; +typedef u64 uint64_t; + +/* How many bytes from the start of the file we search for the header. */ +#define MB_SEARCH 8192 + +/* The magic field should contain this. */ +#define MB_MAGIC 0x1BADB002 + +/* This should be in %eax. */ +#define MB_MAGIC2 0x2BADB002 + +/* The bits in the required part of flags field we don't support. */ +#define MB_UNSUPPORTED 0x0000fffc + +/* Alignment of multiboot modules. */ +#define MB_MOD_ALIGN 0x00001000 + +/* + * Flags set in the 'flags' member of the multiboot header. + */ + +/* Align all boot modules on i386 page (4KB) boundaries. */ +#define MB_PAGE_ALIGN 0x00000001 + +/* Must pass memory information to OS. */ +#define MB_MEMORY_INFO 0x00000002 + +/* Must pass video information to OS. */ +#define MB_VIDEO_MODE 0x00000004 + +/* This flag indicates the use of the address fields in the header. */ +#define MB_AOUT_KLUDGE 0x00010000 + +/* + * Flags to be set in the 'flags' member of the multiboot info structure. + */ + +/* is there basic lower/upper memory information? */ +#define MB_INFO_MEMORY 0x00000001 +/* is there a boot device set? */ +#define MB_INFO_BOOTDEV 0x00000002 +/* is the command-line defined? */ +#define MB_INFO_CMDLINE 0x00000004 +/* are there modules to do something with? */ +#define MB_INFO_MODS 0x00000008 + +/* These next two are mutually exclusive */ + +/* is there a symbol table loaded? */ +#define MB_INFO_AOUT_SYMS 0x00000010 +/* is there an ELF section header table? */ +#define MB_INFO_ELF_SHDR 0x00000020 + +/* is there a full memory map? */ +#define MB_INFO_MEM_MAP 0x00000040 + +/* Is there drive info? */ +#define MB_INFO_DRIVE_INFO 0x00000080 + +/* Is there a config table? */ +#define MB_INFO_CONFIG_TABLE 0x00000100 + +/* Is there a boot loader name? */ +#define MB_INFO_BOOT_LOADER_NAME 0x00000200 + +/* Is there a APM table? */ +#define MB_INFO_APM_TABLE 0x00000400 + +/* Is there video information? */ +#define MB_INFO_VIDEO_INFO 0x00000800 + +struct multiboot_header +{ + /* Must be MB_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 MB_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 MB_VIDEO_MODE is set. */ + uint32_t mode_type; + uint32_t width; + uint32_t height; + uint32_t depth; +}; + +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; + + uint32_t syms[4]; + + /* 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; +}; + +struct multiboot_mmap_entry +{ + uint32_t size; + uint64_t addr; + uint64_t len; + uint32_t type; +} __attribute__((packed)); + +unsigned long write_multiboot_info(unsigned long, unsigned long, unsigned long, unsigned long); + +#endif Index: arch/x86/archtables.c =================================================================== --- arch/x86/archtables.c (revision 867) +++ arch/x86/archtables.c (working copy) @@ -24,6 +24,7 @@ #include <console.h> #include <string.h> #include <tables.h> +#include <multiboot.h> //#include <cpu/cpu.h> //#include <pirq_routing.h> //#include <smp/mpspec.h> @@ -78,6 +79,14 @@ post_code(POST_STAGE2_ARCH_WRITE_TABLES_ENTER); + /* The Multiboot information structure must be in 0xf0000 */ + rom_table_end = write_multiboot_info( + low_table_start, low_table_end, + rom_table_start, rom_table_end); + + /* FIXME: is this alignment needed for PIRQ table? */ + rom_table_end = (rom_table_end + 1023) & ~1023; + /* This table must be betweeen 0xf0000 & 0x100000 */ /* we need to make a decision: create empty functions * in .h files if the cpp variable is undefined, or #ifdef? Index: arch/x86/multiboot.c =================================================================== --- arch/x86/multiboot.c (revision 0) +++ arch/x86/multiboot.c (revision 0) @@ -0,0 +1,70 @@ +/* + * support for Multiboot payloads + * + * Copyright (C) 2008 Robert Millan + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, see <http://www.gnu.org/licenses/>. + * + */ + +#include <multiboot.h> +#include <string.h> +#include <device/resource.h> +#include <console.h> + +static struct multiboot_mmap_entry *mb_mem; + +static void build_mb_mem_range(void *gp, struct device *dev, struct resource *res) +{ + mb_mem->addr = res->base; + mb_mem->len = res->size; + mb_mem->type = 1; + mb_mem->size = sizeof(*mb_mem) - sizeof(mb_mem->size); + mb_mem++; +} + +unsigned long write_multiboot_info( + unsigned long low_table_start, unsigned long low_table_end, + unsigned long rom_table_start, unsigned long rom_table_end) +{ + struct multiboot_info *mbi = rom_table_end; + memset(mbi, 0, sizeof(*mbi)); + rom_table_end += sizeof(*mbi); + + mbi->mmap_addr = (u32) rom_table_end; + mb_mem = rom_table_end; + + /* free regions */ + search_global_resources( IORESOURCE_MEM | IORESOURCE_CACHEABLE, + IORESOURCE_MEM | IORESOURCE_CACHEABLE, build_mb_mem_range, NULL); + + /* reserved regions */ + mb_mem->addr = low_table_start; + mb_mem->len = low_table_end - low_table_start; + mb_mem->type = 2; + mb_mem->size = sizeof(*mb_mem) - sizeof(mb_mem->size); + mb_mem++; + mb_mem->addr = rom_table_start; + mb_mem->len = rom_table_end - rom_table_start; + mb_mem->type = 2; + mb_mem->size = sizeof(*mb_mem) - sizeof(mb_mem->size); + mb_mem++; + + mbi->mmap_length = ((u32) mb_mem) - mbi->mmap_addr; + mbi->flags |= MB_INFO_MEM_MAP; + + printk(BIOS_INFO, "Multiboot Information structure has been written.\n"); + + return mb_mem; +} Index: arch/x86/stage1.c =================================================================== --- arch/x86/stage1.c (revision 867) +++ arch/x86/stage1.c (working copy) @@ -28,6 +28,7 @@ #include <lib.h> #include <mc146818rtc.h> #include <cpu.h> +#include <multiboot.h> #ifdef CONFIG_PAYLOAD_ELF_LOADER /* ah, well, what a mess! This is a hard code. FIX ME but how? @@ -139,6 +140,14 @@ } #endif /* CONFIG_PAYLOAD_ELF_LOADER */ + +static int run_address_multiboot(void *f) +{ + int ret, dummy; + __asm__ __volatile__ ("call *%4" : "=a" (ret), "=c" (dummy) : "a" (MB_MAGIC2), "b" (0xf0000), "c" (f) : "edx", "memory"); + return ret; +} + /** * This function is called from assembler code with its argument on the * stack. Force the compiler to generate always correct code for this case. @@ -259,7 +268,7 @@ if (entry != (void*)-1) { /* Final coreboot call before handing off to the payload. */ mainboard_pre_payload(); - run_address(entry); + run_address_multiboot(entry); } else { die("FATAL: No usable payload found.\n"); } Index: arch/x86/Makefile =================================================================== --- arch/x86/Makefile (revision 867) +++ arch/x86/Makefile (working copy) @@ -188,7 +188,7 @@ STAGE2_LIB_SRC = stage2.c clog2.c mem.c tables.c delay.c \ compute_ip_checksum.c string.c -STAGE2_ARCH_X86_SRC = archtables.c coreboot_table.c udelay_io.c +STAGE2_ARCH_X86_SRC = archtables.c coreboot_table.c multiboot.c udelay_io.c STAGE2_ARCH_X86_SRC += pci_ops_auto.c STAGE2_ARCH_X86_SRC += keyboard.c i8259.c isa-dma.c
-- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
