| Message ID | 1453111140-15128-1-git-send-email-markmb@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 18/01/2016 10:59, Marc Marí wrote: > This optionrom is based on linuxboot.S. > > Signed-off-by: Marc Marí <markmb@redhat.com> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> > --- > .gitignore | 4 + > hw/i386/pc.c | 9 +- > hw/nvram/fw_cfg.c | 2 +- > include/hw/nvram/fw_cfg.h | 1 + > pc-bios/optionrom/Makefile | 8 +- > pc-bios/optionrom/linuxboot_dma.c | 262 ++++++++++++++++++++++++++++++++++++++ > pc-bios/optionrom/optionrom.h | 4 +- > 7 files changed, 285 insertions(+), 5 deletions(-) > create mode 100644 pc-bios/optionrom/linuxboot_dma.c > > diff --git a/.gitignore b/.gitignore > index 88a80ff..101d1e0 100644 > --- a/.gitignore > +++ b/.gitignore > @@ -94,6 +94,10 @@ > /pc-bios/optionrom/linuxboot.bin > /pc-bios/optionrom/linuxboot.raw > /pc-bios/optionrom/linuxboot.img > +/pc-bios/optionrom/linuxboot_dma.asm > +/pc-bios/optionrom/linuxboot_dma.bin > +/pc-bios/optionrom/linuxboot_dma.raw > +/pc-bios/optionrom/linuxboot_dma.img > /pc-bios/optionrom/multiboot.asm > /pc-bios/optionrom/multiboot.bin > /pc-bios/optionrom/multiboot.raw > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 459260b..00339fa 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1007,8 +1007,13 @@ static void load_linux(PCMachineState *pcms, > fw_cfg_add_i32(fw_cfg, FW_CFG_SETUP_SIZE, setup_size); > fw_cfg_add_bytes(fw_cfg, FW_CFG_SETUP_DATA, setup, setup_size); > > - option_rom[nb_option_roms].name = "linuxboot.bin"; > - option_rom[nb_option_roms].bootindex = 0; > + if (fw_cfg_dma_enabled(fw_cfg)) { > + option_rom[nb_option_roms].name = "linuxboot_dma.bin"; > + option_rom[nb_option_roms].bootindex = 0; > + } else { > + option_rom[nb_option_roms].name = "linuxboot.bin"; > + option_rom[nb_option_roms].bootindex = 0; > + } > nb_option_roms++; > } > > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > index a1d650d..d0a5753 100644 > --- a/hw/nvram/fw_cfg.c > +++ b/hw/nvram/fw_cfg.c > @@ -546,7 +546,7 @@ static bool is_version_1(void *opaque, int version_id) > return version_id == 1; > } > > -static bool fw_cfg_dma_enabled(void *opaque) > +bool fw_cfg_dma_enabled(void *opaque) > { > FWCfgState *s = opaque; > > diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h > index 664eaf6..953e58d 100644 > --- a/include/hw/nvram/fw_cfg.h > +++ b/include/hw/nvram/fw_cfg.h > @@ -219,6 +219,7 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr, > hwaddr dma_addr, AddressSpace *dma_as); > > FWCfgState *fw_cfg_find(void); > +bool fw_cfg_dma_enabled(void *opaque); > > #endif /* NO_QEMU_PROTOS */ > > diff --git a/pc-bios/optionrom/Makefile b/pc-bios/optionrom/Makefile > index ce4852a..076f351 100644 > --- a/pc-bios/optionrom/Makefile > +++ b/pc-bios/optionrom/Makefile > @@ -2,6 +2,7 @@ all: build-all > # Dummy command so that make thinks it has done something > @true > > +BULD_DIR=$(CURDIR) > include ../../config-host.mak > include $(SRC_PATH)/rules.mak > > @@ -11,15 +12,20 @@ $(call set-vpath, $(SRC_PATH)/pc-bios/optionrom) > > CFLAGS := -Wall -Wstrict-prototypes -Werror -fomit-frame-pointer -fno-builtin > CFLAGS += -I$(SRC_PATH) > +CFLAGS += -I$(SRC_PATH)/include > CFLAGS += $(call cc-option, $(CFLAGS), -fno-stack-protector) > CFLAGS += $(CFLAGS_NOPIE) > +CFLAGS += -m32 > QEMU_CFLAGS = $(CFLAGS) > > -build-all: multiboot.bin linuxboot.bin kvmvapic.bin > +build-all: multiboot.bin linuxboot.bin linuxboot_dma.bin kvmvapic.bin > > # suppress auto-removal of intermediate files > .SECONDARY: > > +linuxboot_dma.img: linuxboot_dma.o > + $(call quiet-command,$(LD) $(LDFLAGS_NOPIE) -m elf_i386 -static -Ttext 0 -e _start -s -o $@ $<," Building $(TARGET_DIR)$@") > + > %.img: %.o > $(call quiet-command,$(LD) $(LDFLAGS_NOPIE) -Ttext 0 -e _start -s -o $@ $<," Building $(TARGET_DIR)$@") > > diff --git a/pc-bios/optionrom/linuxboot_dma.c b/pc-bios/optionrom/linuxboot_dma.c > new file mode 100644 > index 0000000..9c355fd > --- /dev/null > +++ b/pc-bios/optionrom/linuxboot_dma.c > @@ -0,0 +1,262 @@ > +/* > + * Linux Boot Option ROM for fw_cfg DMA > + * > + * 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/>. > + * > + * Copyright (c) 2015 Red Hat Inc. > + * Authors: Marc Marí <markmb@redhat.com> > + */ > + > +asm( > +".text\n" > +".global _start\n" > +"_start:\n" > +" .short 0xaa55\n" > +" .byte (_end - _start) / 512\n" > +" lret\n" > +" .org 0x18\n" > +" .short 0\n" > +" .short _pnph\n" > +"_pnph:\n" > +" .ascii \"$PnP\"\n" > +" .byte 0x01\n" > +" .byte ( _pnph_len / 16 )\n" > +" .short 0x0000\n" > +" .byte 0x00\n" > +" .byte 0x00\n" > +" .long 0x00000000\n" > +" .short _manufacturer\n" > +" .short _product\n" > +" .long 0x00000000\n" > +" .short 0x0000\n" > +" .short 0x0000\n" > +" .short _bev\n" > +" .short 0x0000\n" > +" .short 0x0000\n" > +" .equ _pnph_len, . - _pnph\n" > +" .align 4, 0\n" > +"_bev:\n" > +".code16gcc\n" > +/* DS = CS */ > +" movw %cs, %ax\n" > +" movw %ax, %ds\n" > +" movl %esp, %ebp\n" > +"run_linuxboot:\n" > +" cli\n" > +" cld\n" > +" jmp load_kernel\n" > +); > + > +#define C_CODE > + > +/* Do not include all QEMU dependencies */ > +#include <stdint.h> > +#include <byteswap.h> > +#include "optionrom.h" > + > +#define BOOT_ROM_PRODUCT "Linux loader" > + > +/* QEMU_CFG_DMA_CONTROL bits */ > +#define BIOS_CFG_DMA_CTL_ERROR 0x01 > +#define BIOS_CFG_DMA_CTL_READ 0x02 > +#define BIOS_CFG_DMA_CTL_SKIP 0x04 > +#define BIOS_CFG_DMA_CTL_SELECT 0x08 > + > +#define BIOS_CFG_DMA_ADDR_HIGH 0x514 > +#define BIOS_CFG_DMA_ADDR_LOW 0x518 > + > +#define _stringify(S) #S > +#define stringify(S) _stringify(S) > + > +#define barrier() asm("": : :"memory") > + > +typedef struct FWCfgDmaAccess { > + uint32_t control; > + uint32_t length; > + uint64_t address; > +} __attribute__((gcc_struct, packed)) FWCfgDmaAccess; > + > +static inline void outl(uint32_t value, uint16_t port) { > + asm("outl %0, %w1" : : "a"(value), "Nd"(port)); > +} > + > +static inline uint16_t readw_addr32(const void *addr) { > + uint16_t val; > + asm("addr32 movw %1, %0" : "=r"(val) : "g"(addr)); > + barrier(); > + return val; > +} > + > +static inline uint32_t readl_addr32(const void *addr) { > + uint32_t val; > + asm("addr32 movl %1, %0" : "=r"(val) : "g"(addr)); > + barrier(); > + return val; > +} > + > +static inline void writel_addr32(void *addr, uint32_t val) { > + barrier(); > + asm("addr32 movl %0, %1" : : "r"(val), "g"(addr)); > +} > + > +static inline uint64_t cpu_to_be64(uint64_t x) { > + return bswap_64(x); > +} > + > +static inline uint32_t cpu_to_be32(uint32_t x) { > + return bswap_32(x); > +} > + > +static inline uint32_t be32_to_cpu(uint32_t x) { > + return bswap_32(x); > +} > + > +static void bios_cfg_read_entry(void *buf, uint16_t entry, uint32_t len) > +{ > + FWCfgDmaAccess access; > + uint32_t control = (entry << 16) | BIOS_CFG_DMA_CTL_SELECT > + | BIOS_CFG_DMA_CTL_READ; > + > + access.address = cpu_to_be64((uint64_t)(uint32_t)buf); > + access.length = cpu_to_be32(len); > + access.control = cpu_to_be32(control); > + > + barrier(); > + > + outl(cpu_to_be32((uint32_t)&access), BIOS_CFG_DMA_ADDR_LOW); > + > + while(be32_to_cpu(access.control) & ~BIOS_CFG_DMA_CTL_ERROR) { > + barrier(); > + } > +} > + > +static uint32_t get_e801_addr(void) > +{ > + uint32_t eax, ebx, ecx, edx; > + uint32_t ret; > + > + eax = 0xe801; > + ebx = 0; > + ecx = 0; > + edx = 0; > + asm("int $0x15\n" > + : "+a"(eax) > + : "b"(ebx), "c"(ecx), "d"(edx)); > + > + /* Output could be in AX/BX or CX/DX */ > + if ((uint16_t)ecx || (uint16_t)edx) { > + if(!(uint16_t)edx) { > + /* Add 1 MB and convert to bytes */ > + ret = (ecx + 1024) << 10; > + } else { > + /* Add 16 MB and convert to bytes */ > + ret = (edx + 256) << 16; > + } > + } else { > + if(!(uint16_t)ebx) { > + /* Add 1 MB and convert to bytes */ > + ret = (eax + 1024) << 10; > + } else { > + /* Add 16 MB and convert to bytes */ > + ret = (ebx + 256) << 16; > + } > + } > + > + return ret; > +} > + > +void load_kernel(void) > +{ > + void *setup_addr; > + void *initrd_addr; > + void *kernel_addr; > + void *cmdline_addr; > + uint32_t setup_size; > + uint32_t initrd_size; > + uint32_t kernel_size; > + uint32_t cmdline_size; > + uint32_t initrd_end_page, max_allowed_page; > + uint32_t segment_addr, stack_addr; > + > + bios_cfg_read_entry(&setup_addr, FW_CFG_SETUP_ADDR, 4); > + bios_cfg_read_entry(&setup_size, FW_CFG_SETUP_SIZE, 4); > + bios_cfg_read_entry(setup_addr, FW_CFG_SETUP_DATA, setup_size); > + > + if (readw_addr32(setup_addr + 0x206) < 0x203) { > + /* Assume initrd_max 0x37ffffff */ > + writel_addr32(setup_addr + 0x22c, 0x37ffffff); > + } > + > + bios_cfg_read_entry(&initrd_addr, FW_CFG_INITRD_ADDR, 4); > + bios_cfg_read_entry(&initrd_size, FW_CFG_INITRD_SIZE, 4); > + > + initrd_end_page = ((uint32_t)(initrd_addr + initrd_size) & -4096); > + max_allowed_page = (readl_addr32(setup_addr + 0x22c) & -4096); > + > + if (initrd_end_page != 0 && max_allowed_page != 0 && > + initrd_end_page != max_allowed_page) { > + /* Initrd at the end of memory. Compute better initrd address > + * based on e801 data > + */ > + initrd_addr = (void *)((get_e801_addr() - initrd_size) & -4096); > + writel_addr32(setup_addr + 0x218, (uint32_t)initrd_addr); > + > + } > + > + bios_cfg_read_entry(initrd_addr, FW_CFG_INITRD_DATA, initrd_size); > + > + bios_cfg_read_entry(&kernel_addr, FW_CFG_KERNEL_ADDR, 4); > + bios_cfg_read_entry(&kernel_size, FW_CFG_KERNEL_SIZE, 4); > + bios_cfg_read_entry(kernel_addr, FW_CFG_KERNEL_DATA, kernel_size); > + > + bios_cfg_read_entry(&cmdline_addr, FW_CFG_CMDLINE_ADDR, 4); > + bios_cfg_read_entry(&cmdline_size, FW_CFG_CMDLINE_SIZE, 4); > + bios_cfg_read_entry(cmdline_addr, FW_CFG_CMDLINE_DATA, cmdline_size); > + > + /* Boot linux */ > + segment_addr = ((uint32_t)setup_addr >> 4); > + stack_addr = (uint32_t)(cmdline_addr - setup_addr - 16); > + > + /* As we are changing crytical registers, we cannot leave freedom to the > + * compiler. > + */ > + asm("movw %%ax, %%ds\n" > + "movw %%ax, %%es\n" > + "movw %%ax, %%fs\n" > + "movw %%ax, %%gs\n" > + "movw %%ax, %%ss\n" > + "movl %%ebx, %%esp\n" > + "addw $0x20, %%ax\n" > + "pushw %%ax\n" /* CS */ > + "pushw $0\n" /* IP */ > + /* Clear registers and jump to Linux */ > + "xor %%ebx, %%ebx\n" > + "xor %%ecx, %%ecx\n" > + "xor %%edx, %%edx\n" > + "xor %%edi, %%edi\n" > + "xor %%ebp, %%ebp\n" > + "lretw\n" > + : : "a"(segment_addr), "b"(stack_addr)); > +} > + > +asm( > +"_manufacturer:\n" > +".asciz \"QEMU\"\n" > +"_product:\n" > +".asciz "stringify(BOOT_ROM_PRODUCT)"\n" > +".byte 0\n" > +".align 512, 0\n" > +"_end:\n" > +); > + > diff --git a/pc-bios/optionrom/optionrom.h b/pc-bios/optionrom/optionrom.h > index f1a9021..25c765f 100644 > --- a/pc-bios/optionrom/optionrom.h > +++ b/pc-bios/optionrom/optionrom.h > @@ -29,7 +29,8 @@ > #define DEBUG_HERE \ > jmp 1f; \ > 1: > - > + > +#ifndef C_CODE > /* > * Read a variable from the fw_cfg device. > * Clobbers: %edx > @@ -49,6 +50,7 @@ > inb (%dx), %al > bswap %eax > .endm > +#endif > > #define read_fw_blob_pre(var) \ > read_fw var ## _SIZE; \ >
On Mon, Jan 18, 2016 at 10:59:00AM +0100, Marc Marí wrote: > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 459260b..00339fa 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1007,8 +1007,13 @@ static void load_linux(PCMachineState *pcms, > fw_cfg_add_i32(fw_cfg, FW_CFG_SETUP_SIZE, setup_size); > fw_cfg_add_bytes(fw_cfg, FW_CFG_SETUP_DATA, setup, setup_size); > > - option_rom[nb_option_roms].name = "linuxboot.bin"; > - option_rom[nb_option_roms].bootindex = 0; > + if (fw_cfg_dma_enabled(fw_cfg)) { > + option_rom[nb_option_roms].name = "linuxboot_dma.bin"; > + option_rom[nb_option_roms].bootindex = 0; > + } else { > + option_rom[nb_option_roms].name = "linuxboot.bin"; > + option_rom[nb_option_roms].bootindex = 0; > + } Live migration compatibility requires that guest-visible changes to the machine are only introduced in a new -machine <machine-type>. This ensures that migrating from an older QEMU version to a newer QEMU version will not change the guest-visible memory layout or device behavior. The Option ROM should not change when migration between different QEMU versions. I've CCed Gerd and Juan, I think they know how changes to Option ROMs affect live migration better than me. What needs to be done to preserve live migration compatibility? > diff --git a/pc-bios/optionrom/Makefile b/pc-bios/optionrom/Makefile > index ce4852a..076f351 100644 > --- a/pc-bios/optionrom/Makefile > +++ b/pc-bios/optionrom/Makefile > @@ -2,6 +2,7 @@ all: build-all > # Dummy command so that make thinks it has done something > @true > > +BULD_DIR=$(CURDIR) Is this a typo (BUILD_DIR)? Is it even needed? > include ../../config-host.mak > include $(SRC_PATH)/rules.mak > > @@ -11,15 +12,20 @@ $(call set-vpath, $(SRC_PATH)/pc-bios/optionrom) > > CFLAGS := -Wall -Wstrict-prototypes -Werror -fomit-frame-pointer -fno-builtin > CFLAGS += -I$(SRC_PATH) > +CFLAGS += -I$(SRC_PATH)/include Are you using any QEMU headers? If not, then this change can be dropped. > +linuxboot_dma.img: linuxboot_dma.o > + $(call quiet-command,$(LD) $(LDFLAGS_NOPIE) -m elf_i386 -static -Ttext 0 -e _start -s -o $@ $<," Building $(TARGET_DIR)$@") Why is -static necessary? > +#define BOOT_ROM_PRODUCT "Linux loader" Please differentiate from linuxboot.S. Maybe call it "Linux loader DMA". > +typedef struct FWCfgDmaAccess { > + uint32_t control; > + uint32_t length; > + uint64_t address; > +} __attribute__((gcc_struct, packed)) FWCfgDmaAccess; gcc_struct is for gcc vs Windows compiler compatibility when the struct contains bitfields. No bitfields are used and no Windows compiled code is linked, so it seems unnecessary. > +static void bios_cfg_read_entry(void *buf, uint16_t entry, uint32_t len) > +{ > + FWCfgDmaAccess access; > + uint32_t control = (entry << 16) | BIOS_CFG_DMA_CTL_SELECT > + | BIOS_CFG_DMA_CTL_READ; > + > + access.address = cpu_to_be64((uint64_t)(uint32_t)buf); > + access.length = cpu_to_be32(len); > + access.control = cpu_to_be32(control); > + > + barrier(); > + > + outl(cpu_to_be32((uint32_t)&access), BIOS_CFG_DMA_ADDR_LOW); > + > + while(be32_to_cpu(access.control) & ~BIOS_CFG_DMA_CTL_ERROR) { > + barrier(); > + } > +} This function is the unique part of linuxboot_dma.c. Everything else is copied and translated from linuxboot.S. The refactorer in me wonders how hard it would be to extend linuxboot.S instead of introducing this new boot ROM. Was there a technical reason why linuxboot.S cannot be extended (e.g. a size limit)? > + /* As we are changing crytical registers, we cannot leave freedom to the s/crytical/critical/ > diff --git a/pc-bios/optionrom/optionrom.h b/pc-bios/optionrom/optionrom.h > index f1a9021..25c765f 100644 > --- a/pc-bios/optionrom/optionrom.h > +++ b/pc-bios/optionrom/optionrom.h > @@ -29,7 +29,8 @@ > #define DEBUG_HERE \ > jmp 1f; \ > 1: > - > + > +#ifndef C_CODE > /* > * Read a variable from the fw_cfg device. > * Clobbers: %edx > @@ -49,6 +50,7 @@ > inb (%dx), %al > bswap %eax > .endm > +#endif Why do you need optionrom.h? You seem to have expanded its macros anyway (OPTION_ROM_START, BOOT_ROM_START, OPTION_ROM_END, BOOT_ROM_END). If you need optionrom.h, please actually use its macros instead of expanding them. Otherwise, please don't include it.
On Mon, 18 Jan 2016 14:42:06 +0000 Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Mon, Jan 18, 2016 at 10:59:00AM +0100, Marc Marí wrote: > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > index 459260b..00339fa 100644 > > --- a/hw/i386/pc.c > > +++ b/hw/i386/pc.c > > @@ -1007,8 +1007,13 @@ static void load_linux(PCMachineState *pcms, > > fw_cfg_add_i32(fw_cfg, FW_CFG_SETUP_SIZE, setup_size); > > fw_cfg_add_bytes(fw_cfg, FW_CFG_SETUP_DATA, setup, setup_size); > > > > - option_rom[nb_option_roms].name = "linuxboot.bin"; > > - option_rom[nb_option_roms].bootindex = 0; > > + if (fw_cfg_dma_enabled(fw_cfg)) { > > + option_rom[nb_option_roms].name = "linuxboot_dma.bin"; > > + option_rom[nb_option_roms].bootindex = 0; > > + } else { > > + option_rom[nb_option_roms].name = "linuxboot.bin"; > > + option_rom[nb_option_roms].bootindex = 0; > > + } > > Live migration compatibility requires that guest-visible changes to > the machine are only introduced in a new -machine <machine-type>. > > This ensures that migrating from an older QEMU version to a newer QEMU > version will not change the guest-visible memory layout or device > behavior. > > The Option ROM should not change when migration between different QEMU > versions. > > I've CCed Gerd and Juan, I think they know how changes to Option ROMs > affect live migration better than me. What needs to be done to > preserve live migration compatibility? They are CC'd now :) > > diff --git a/pc-bios/optionrom/Makefile b/pc-bios/optionrom/Makefile > > index ce4852a..076f351 100644 > > --- a/pc-bios/optionrom/Makefile > > +++ b/pc-bios/optionrom/Makefile > > @@ -2,6 +2,7 @@ all: build-all > > # Dummy command so that make thinks it has done something > > @true > > > > +BULD_DIR=$(CURDIR) > > Is this a typo (BUILD_DIR)? Is it even needed? Experiments and trials in the past that are not necessary now and I forgot to remove. They have been removed now. > > include ../../config-host.mak > > include $(SRC_PATH)/rules.mak > > > > @@ -11,15 +12,20 @@ $(call set-vpath, $(SRC_PATH)/pc-bios/optionrom) > > > > CFLAGS := -Wall -Wstrict-prototypes -Werror -fomit-frame-pointer > > -fno-builtin CFLAGS += -I$(SRC_PATH) > > +CFLAGS += -I$(SRC_PATH)/include > > Are you using any QEMU headers? If not, then this change can be > dropped. Same as above. > > +linuxboot_dma.img: linuxboot_dma.o > > + $(call quiet-command,$(LD) $(LDFLAGS_NOPIE) -m elf_i386 > > -static -Ttext 0 -e _start -s -o $@ $<," Building > > $(TARGET_DIR)$@") > > Why is -static necessary? Same as above. > > > +#define BOOT_ROM_PRODUCT "Linux loader" > > Please differentiate from linuxboot.S. Maybe call it "Linux loader > DMA". > > > +typedef struct FWCfgDmaAccess { > > + uint32_t control; > > + uint32_t length; > > + uint64_t address; > > +} __attribute__((gcc_struct, packed)) FWCfgDmaAccess; > > gcc_struct is for gcc vs Windows compiler compatibility when the > struct contains bitfields. No bitfields are used and no Windows > compiled code is linked, so it seems unnecessary. > > > +static void bios_cfg_read_entry(void *buf, uint16_t entry, > > uint32_t len) +{ > > + FWCfgDmaAccess access; > > + uint32_t control = (entry << 16) | BIOS_CFG_DMA_CTL_SELECT > > + | BIOS_CFG_DMA_CTL_READ; > > + > > + access.address = cpu_to_be64((uint64_t)(uint32_t)buf); > > + access.length = cpu_to_be32(len); > > + access.control = cpu_to_be32(control); > > + > > + barrier(); > > + > > + outl(cpu_to_be32((uint32_t)&access), BIOS_CFG_DMA_ADDR_LOW); > > + > > + while(be32_to_cpu(access.control) & ~BIOS_CFG_DMA_CTL_ERROR) { > > + barrier(); > > + } > > +} > > This function is the unique part of linuxboot_dma.c. Everything else > is copied and translated from linuxboot.S. The refactorer in me > wonders how hard it would be to extend linuxboot.S instead of > introducing this new boot ROM. > > Was there a technical reason why linuxboot.S cannot be extended > (e.g. a size limit)? I don't think there's a technical reason. It is a lot simpler to write the fw_cfg DMA stuff in C. To extend linuxboot.S these things should be modified: - Add fw_cfg DMA detection support - Change read_fw from a macro to a function that checks for fw_cfg DMA support and does the operation using IO or memory - Extract bits and pieces from linuxboot.S into functions, that are only necessary when there is no support for fw_cfg DMA (the most important is jumping to 32 bits to read and copy the kernel). This way, you check for support from the very beggining (when configuring the machine), and you don't have to branch the code anymore. (I think I discussed this with somebody in the past. But I'm not sure with whom, or when. So I'll suppose it was a dream and it is not on the archives). If you really think they should be merged, I'd even propose to merge the ASM version onto the C version (convert this patch into linuxboot.S). This slightly improves readability. > > + /* As we are changing crytical registers, we cannot leave > > freedom to the > > s/crytical/critical/ > > > diff --git a/pc-bios/optionrom/optionrom.h > > b/pc-bios/optionrom/optionrom.h index f1a9021..25c765f 100644 > > --- a/pc-bios/optionrom/optionrom.h > > +++ b/pc-bios/optionrom/optionrom.h > > @@ -29,7 +29,8 @@ > > #define DEBUG_HERE \ > > jmp 1f; \ > > 1: > > - > > + > > +#ifndef C_CODE > > /* > > * Read a variable from the fw_cfg device. > > * Clobbers: %edx > > @@ -49,6 +50,7 @@ > > inb (%dx), %al > > bswap %eax > > .endm > > +#endif > > Why do you need optionrom.h? You seem to have expanded its macros > anyway (OPTION_ROM_START, BOOT_ROM_START, OPTION_ROM_END, > BOOT_ROM_END). > > If you need optionrom.h, please actually use its macros instead of > expanding them. > > Otherwise, please don't include it. I need only these two lines: #define NO_QEMU_PROTOS #include "../../include/hw/nvram/fw_cfg.h" Which can be added to linuxboot_dma.c instead of including optionrom.h And the macros cannot be reused (that was the original idea) because those macros are written to be used directly in ASM sources. They cannot be used as embedded ASM in a C source (they are missing, at least, the line feed at the end of each instruction). What I can do, is branch the macro definition with "ifdef C_CODE". Thanks Marc
On Mon, Jan 18, 2016 at 05:22:09PM +0100, Marc Marí wrote: > On Mon, 18 Jan 2016 14:42:06 +0000 > Stefan Hajnoczi <stefanha@gmail.com> wrote: > > Was there a technical reason why linuxboot.S cannot be extended > > (e.g. a size limit)? > > I don't think there's a technical reason. It is a lot simpler to write > the fw_cfg DMA stuff in C. To extend linuxboot.S these things should be > modified: > - Add fw_cfg DMA detection support > - Change read_fw from a macro to a function that checks for fw_cfg DMA > support and does the operation using IO or memory > - Extract bits and pieces from linuxboot.S into functions, that are > only necessary when there is no support for fw_cfg DMA (the most > important is jumping to 32 bits to read and copy the kernel). > > This way, you check for support from the very beggining (when > configuring the machine), and you don't have to branch the code > anymore. > > (I think I discussed this with somebody in the past. But I'm not sure > with whom, or when. So I'll suppose it was a dream and it is not on the > archives). > > If you really think they should be merged, I'd even propose to > merge the ASM version onto the C version (convert this patch into > linuxboot.S). This slightly improves readability. FYI, if the existing linuxboot.S jumps to 32bit mode just to copy the kernel then that isn't necessary. By specification, option roms run in "big real" mode and thus can directly read/write to the first 4G of ram. -Kevin
Hi, > > > + if (fw_cfg_dma_enabled(fw_cfg)) { > > > + option_rom[nb_option_roms].name = "linuxboot_dma.bin"; > > > + option_rom[nb_option_roms].bootindex = 0; > > > + } else { > > > + option_rom[nb_option_roms].name = "linuxboot.bin"; > > > + option_rom[nb_option_roms].bootindex = 0; > > > + } > > > > Live migration compatibility requires that guest-visible changes to > > the machine are only introduced in a new -machine <machine-type>. > > I've CCed Gerd and Juan, I think they know how changes to Option ROMs > > affect live migration better than me. What needs to be done to > > preserve live migration compatibility? > > They are CC'd now :) I think we are fine here. The dma interface is enabled for new machine types only, thats why we have fw_cfg_dma_enabled() in the first place ;) > > Was there a technical reason why linuxboot.S cannot be extended > > (e.g. a size limit)? > > I don't think there's a technical reason. It is a lot simpler to write > the fw_cfg DMA stuff in C. To extend linuxboot.S these things should be > modified: > - Add fw_cfg DMA detection support > - Change read_fw from a macro to a function that checks for fw_cfg DMA > support and does the operation using IO or memory > - Extract bits and pieces from linuxboot.S into functions, that are > only necessary when there is no support for fw_cfg DMA (the most > important is jumping to 32 bits to read and copy the kernel). > > This way, you check for support from the very beggining (when > configuring the machine), and you don't have to branch the code > anymore. > > (I think I discussed this with somebody in the past. But I'm not sure > with whom, or when. So I'll suppose it was a dream and it is not on the > archives). Could have been /me. The fw_cfg macros in linuxboot.S are messy, looks like because they got extended a few times. Piling DMA support on top of that didn't look very appealing to me. Also DMA support simplifies things, there is no need to switch processor modes to load the kernel above 1M. > If you really think they should be merged, I'd even propose to > merge the ASM version onto the C version (convert this patch into > linuxboot.S). This slightly improves readability. Fully agree. I'm personally fine with having two roms, but when merging them into one we surely should ditch the fw_cfg asm macros and go with something more maintainable. cheers, Gerd
On Tue, Jan 19, 2016 at 01:11:47PM +0100, Gerd Hoffmann wrote: > > If you really think they should be merged, I'd even propose to > > merge the ASM version onto the C version (convert this patch into > > linuxboot.S). This slightly improves readability. > > Fully agree. I'm personally fine with having two roms, but when merging > them into one we surely should ditch the fw_cfg asm macros and go with > something more maintainable. There is no technical requirement for a unified linuxboot ROM. If there is no disadvantage to having 2 ROMs then let's stick to Marc's approach. Stefan
diff --git a/.gitignore b/.gitignore index 88a80ff..101d1e0 100644 --- a/.gitignore +++ b/.gitignore @@ -94,6 +94,10 @@ /pc-bios/optionrom/linuxboot.bin /pc-bios/optionrom/linuxboot.raw /pc-bios/optionrom/linuxboot.img +/pc-bios/optionrom/linuxboot_dma.asm +/pc-bios/optionrom/linuxboot_dma.bin +/pc-bios/optionrom/linuxboot_dma.raw +/pc-bios/optionrom/linuxboot_dma.img /pc-bios/optionrom/multiboot.asm /pc-bios/optionrom/multiboot.bin /pc-bios/optionrom/multiboot.raw diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 459260b..00339fa 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1007,8 +1007,13 @@ static void load_linux(PCMachineState *pcms, fw_cfg_add_i32(fw_cfg, FW_CFG_SETUP_SIZE, setup_size); fw_cfg_add_bytes(fw_cfg, FW_CFG_SETUP_DATA, setup, setup_size); - option_rom[nb_option_roms].name = "linuxboot.bin"; - option_rom[nb_option_roms].bootindex = 0; + if (fw_cfg_dma_enabled(fw_cfg)) { + option_rom[nb_option_roms].name = "linuxboot_dma.bin"; + option_rom[nb_option_roms].bootindex = 0; + } else { + option_rom[nb_option_roms].name = "linuxboot.bin"; + option_rom[nb_option_roms].bootindex = 0; + } nb_option_roms++; } diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c index a1d650d..d0a5753 100644 --- a/hw/nvram/fw_cfg.c +++ b/hw/nvram/fw_cfg.c @@ -546,7 +546,7 @@ static bool is_version_1(void *opaque, int version_id) return version_id == 1; } -static bool fw_cfg_dma_enabled(void *opaque) +bool fw_cfg_dma_enabled(void *opaque) { FWCfgState *s = opaque; diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h index 664eaf6..953e58d 100644 --- a/include/hw/nvram/fw_cfg.h +++ b/include/hw/nvram/fw_cfg.h @@ -219,6 +219,7 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr, hwaddr dma_addr, AddressSpace *dma_as); FWCfgState *fw_cfg_find(void); +bool fw_cfg_dma_enabled(void *opaque); #endif /* NO_QEMU_PROTOS */ diff --git a/pc-bios/optionrom/Makefile b/pc-bios/optionrom/Makefile index ce4852a..076f351 100644 --- a/pc-bios/optionrom/Makefile +++ b/pc-bios/optionrom/Makefile @@ -2,6 +2,7 @@ all: build-all # Dummy command so that make thinks it has done something @true +BULD_DIR=$(CURDIR) include ../../config-host.mak include $(SRC_PATH)/rules.mak @@ -11,15 +12,20 @@ $(call set-vpath, $(SRC_PATH)/pc-bios/optionrom) CFLAGS := -Wall -Wstrict-prototypes -Werror -fomit-frame-pointer -fno-builtin CFLAGS += -I$(SRC_PATH) +CFLAGS += -I$(SRC_PATH)/include CFLAGS += $(call cc-option, $(CFLAGS), -fno-stack-protector) CFLAGS += $(CFLAGS_NOPIE) +CFLAGS += -m32 QEMU_CFLAGS = $(CFLAGS) -build-all: multiboot.bin linuxboot.bin kvmvapic.bin +build-all: multiboot.bin linuxboot.bin linuxboot_dma.bin kvmvapic.bin # suppress auto-removal of intermediate files .SECONDARY: +linuxboot_dma.img: linuxboot_dma.o + $(call quiet-command,$(LD) $(LDFLAGS_NOPIE) -m elf_i386 -static -Ttext 0 -e _start -s -o $@ $<," Building $(TARGET_DIR)$@") + %.img: %.o $(call quiet-command,$(LD) $(LDFLAGS_NOPIE) -Ttext 0 -e _start -s -o $@ $<," Building $(TARGET_DIR)$@") diff --git a/pc-bios/optionrom/linuxboot_dma.c b/pc-bios/optionrom/linuxboot_dma.c new file mode 100644 index 0000000..9c355fd --- /dev/null +++ b/pc-bios/optionrom/linuxboot_dma.c @@ -0,0 +1,262 @@ +/* + * Linux Boot Option ROM for fw_cfg DMA + * + * 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/>. + * + * Copyright (c) 2015 Red Hat Inc. + * Authors: Marc Marí <markmb@redhat.com> + */ + +asm( +".text\n" +".global _start\n" +"_start:\n" +" .short 0xaa55\n" +" .byte (_end - _start) / 512\n" +" lret\n" +" .org 0x18\n" +" .short 0\n" +" .short _pnph\n" +"_pnph:\n" +" .ascii \"$PnP\"\n" +" .byte 0x01\n" +" .byte ( _pnph_len / 16 )\n" +" .short 0x0000\n" +" .byte 0x00\n" +" .byte 0x00\n" +" .long 0x00000000\n" +" .short _manufacturer\n" +" .short _product\n" +" .long 0x00000000\n" +" .short 0x0000\n" +" .short 0x0000\n" +" .short _bev\n" +" .short 0x0000\n" +" .short 0x0000\n" +" .equ _pnph_len, . - _pnph\n" +" .align 4, 0\n" +"_bev:\n" +".code16gcc\n" +/* DS = CS */ +" movw %cs, %ax\n" +" movw %ax, %ds\n" +" movl %esp, %ebp\n" +"run_linuxboot:\n" +" cli\n" +" cld\n" +" jmp load_kernel\n" +); + +#define C_CODE + +/* Do not include all QEMU dependencies */ +#include <stdint.h> +#include <byteswap.h> +#include "optionrom.h" + +#define BOOT_ROM_PRODUCT "Linux loader" + +/* QEMU_CFG_DMA_CONTROL bits */ +#define BIOS_CFG_DMA_CTL_ERROR 0x01 +#define BIOS_CFG_DMA_CTL_READ 0x02 +#define BIOS_CFG_DMA_CTL_SKIP 0x04 +#define BIOS_CFG_DMA_CTL_SELECT 0x08 + +#define BIOS_CFG_DMA_ADDR_HIGH 0x514 +#define BIOS_CFG_DMA_ADDR_LOW 0x518 + +#define _stringify(S) #S +#define stringify(S) _stringify(S) + +#define barrier() asm("": : :"memory") + +typedef struct FWCfgDmaAccess { + uint32_t control; + uint32_t length; + uint64_t address; +} __attribute__((gcc_struct, packed)) FWCfgDmaAccess; + +static inline void outl(uint32_t value, uint16_t port) { + asm("outl %0, %w1" : : "a"(value), "Nd"(port)); +} + +static inline uint16_t readw_addr32(const void *addr) { + uint16_t val; + asm("addr32 movw %1, %0" : "=r"(val) : "g"(addr)); + barrier(); + return val; +} + +static inline uint32_t readl_addr32(const void *addr) { + uint32_t val; + asm("addr32 movl %1, %0" : "=r"(val) : "g"(addr)); + barrier(); + return val; +} + +static inline void writel_addr32(void *addr, uint32_t val) { + barrier(); + asm("addr32 movl %0, %1" : : "r"(val), "g"(addr)); +} + +static inline uint64_t cpu_to_be64(uint64_t x) { + return bswap_64(x); +} + +static inline uint32_t cpu_to_be32(uint32_t x) { + return bswap_32(x); +} + +static inline uint32_t be32_to_cpu(uint32_t x) { + return bswap_32(x); +} + +static void bios_cfg_read_entry(void *buf, uint16_t entry, uint32_t len) +{ + FWCfgDmaAccess access; + uint32_t control = (entry << 16) | BIOS_CFG_DMA_CTL_SELECT + | BIOS_CFG_DMA_CTL_READ; + + access.address = cpu_to_be64((uint64_t)(uint32_t)buf); + access.length = cpu_to_be32(len); + access.control = cpu_to_be32(control); + + barrier(); + + outl(cpu_to_be32((uint32_t)&access), BIOS_CFG_DMA_ADDR_LOW); + + while(be32_to_cpu(access.control) & ~BIOS_CFG_DMA_CTL_ERROR) { + barrier(); + } +} + +static uint32_t get_e801_addr(void) +{ + uint32_t eax, ebx, ecx, edx; + uint32_t ret; + + eax = 0xe801; + ebx = 0; + ecx = 0; + edx = 0; + asm("int $0x15\n" + : "+a"(eax) + : "b"(ebx), "c"(ecx), "d"(edx)); + + /* Output could be in AX/BX or CX/DX */ + if ((uint16_t)ecx || (uint16_t)edx) { + if(!(uint16_t)edx) { + /* Add 1 MB and convert to bytes */ + ret = (ecx + 1024) << 10; + } else { + /* Add 16 MB and convert to bytes */ + ret = (edx + 256) << 16; + } + } else { + if(!(uint16_t)ebx) { + /* Add 1 MB and convert to bytes */ + ret = (eax + 1024) << 10; + } else { + /* Add 16 MB and convert to bytes */ + ret = (ebx + 256) << 16; + } + } + + return ret; +} + +void load_kernel(void) +{ + void *setup_addr; + void *initrd_addr; + void *kernel_addr; + void *cmdline_addr; + uint32_t setup_size; + uint32_t initrd_size; + uint32_t kernel_size; + uint32_t cmdline_size; + uint32_t initrd_end_page, max_allowed_page; + uint32_t segment_addr, stack_addr; + + bios_cfg_read_entry(&setup_addr, FW_CFG_SETUP_ADDR, 4); + bios_cfg_read_entry(&setup_size, FW_CFG_SETUP_SIZE, 4); + bios_cfg_read_entry(setup_addr, FW_CFG_SETUP_DATA, setup_size); + + if (readw_addr32(setup_addr + 0x206) < 0x203) { + /* Assume initrd_max 0x37ffffff */ + writel_addr32(setup_addr + 0x22c, 0x37ffffff); + } + + bios_cfg_read_entry(&initrd_addr, FW_CFG_INITRD_ADDR, 4); + bios_cfg_read_entry(&initrd_size, FW_CFG_INITRD_SIZE, 4); + + initrd_end_page = ((uint32_t)(initrd_addr + initrd_size) & -4096); + max_allowed_page = (readl_addr32(setup_addr + 0x22c) & -4096); + + if (initrd_end_page != 0 && max_allowed_page != 0 && + initrd_end_page != max_allowed_page) { + /* Initrd at the end of memory. Compute better initrd address + * based on e801 data + */ + initrd_addr = (void *)((get_e801_addr() - initrd_size) & -4096); + writel_addr32(setup_addr + 0x218, (uint32_t)initrd_addr); + + } + + bios_cfg_read_entry(initrd_addr, FW_CFG_INITRD_DATA, initrd_size); + + bios_cfg_read_entry(&kernel_addr, FW_CFG_KERNEL_ADDR, 4); + bios_cfg_read_entry(&kernel_size, FW_CFG_KERNEL_SIZE, 4); + bios_cfg_read_entry(kernel_addr, FW_CFG_KERNEL_DATA, kernel_size); + + bios_cfg_read_entry(&cmdline_addr, FW_CFG_CMDLINE_ADDR, 4); + bios_cfg_read_entry(&cmdline_size, FW_CFG_CMDLINE_SIZE, 4); + bios_cfg_read_entry(cmdline_addr, FW_CFG_CMDLINE_DATA, cmdline_size); + + /* Boot linux */ + segment_addr = ((uint32_t)setup_addr >> 4); + stack_addr = (uint32_t)(cmdline_addr - setup_addr - 16); + + /* As we are changing crytical registers, we cannot leave freedom to the + * compiler. + */ + asm("movw %%ax, %%ds\n" + "movw %%ax, %%es\n" + "movw %%ax, %%fs\n" + "movw %%ax, %%gs\n" + "movw %%ax, %%ss\n" + "movl %%ebx, %%esp\n" + "addw $0x20, %%ax\n" + "pushw %%ax\n" /* CS */ + "pushw $0\n" /* IP */ + /* Clear registers and jump to Linux */ + "xor %%ebx, %%ebx\n" + "xor %%ecx, %%ecx\n" + "xor %%edx, %%edx\n" + "xor %%edi, %%edi\n" + "xor %%ebp, %%ebp\n" + "lretw\n" + : : "a"(segment_addr), "b"(stack_addr)); +} + +asm( +"_manufacturer:\n" +".asciz \"QEMU\"\n" +"_product:\n" +".asciz "stringify(BOOT_ROM_PRODUCT)"\n" +".byte 0\n" +".align 512, 0\n" +"_end:\n" +); + diff --git a/pc-bios/optionrom/optionrom.h b/pc-bios/optionrom/optionrom.h index f1a9021..25c765f 100644 --- a/pc-bios/optionrom/optionrom.h +++ b/pc-bios/optionrom/optionrom.h @@ -29,7 +29,8 @@ #define DEBUG_HERE \ jmp 1f; \ 1: - + +#ifndef C_CODE /* * Read a variable from the fw_cfg device. * Clobbers: %edx @@ -49,6 +50,7 @@ inb (%dx), %al bswap %eax .endm +#endif #define read_fw_blob_pre(var) \ read_fw var ## _SIZE; \
This optionrom is based on linuxboot.S. Signed-off-by: Marc Marí <markmb@redhat.com> --- .gitignore | 4 + hw/i386/pc.c | 9 +- hw/nvram/fw_cfg.c | 2 +- include/hw/nvram/fw_cfg.h | 1 + pc-bios/optionrom/Makefile | 8 +- pc-bios/optionrom/linuxboot_dma.c | 262 ++++++++++++++++++++++++++++++++++++++ pc-bios/optionrom/optionrom.h | 4 +- 7 files changed, 285 insertions(+), 5 deletions(-) create mode 100644 pc-bios/optionrom/linuxboot_dma.c