All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v3] Add optionrom compatible with fw_cfg DMA version
@ 2016-01-25 13:17 Marc Marí
  2016-01-26 11:11 ` Stefan Hajnoczi
  2016-01-28  0:14 ` Kevin O'Connor
  0 siblings, 2 replies; 13+ messages in thread
From: Marc Marí @ 2016-01-25 13:17 UTC (permalink / raw)
  To: qemu-devel
  Cc: Gabriel L. Somlo, Kevin O'Connor, Gerd Hoffmann,
	Stefan Hajnoczi, Paolo Bonzini, Marc Marí,
	Laszlo

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        |   6 +-
 pc-bios/optionrom/linuxboot_dma.c | 262 ++++++++++++++++++++++++++++++++++++++
 6 files changed, 280 insertions(+), 4 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..8c769ef 100644
--- a/pc-bios/optionrom/Makefile
+++ b/pc-bios/optionrom/Makefile
@@ -13,13 +13,17 @@ CFLAGS := -Wall -Wstrict-prototypes -Werror -fomit-frame-pointer -fno-builtin
 CFLAGS += -I$(SRC_PATH)
 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 -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..43594c2
--- /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"
+);
+
+
+#include <stdint.h>
+#include <byteswap.h>
+
+#define NO_QEMU_PROTOS
+#include "../../include/hw/nvram/fw_cfg.h"
+
+#define BOOT_ROM_PRODUCT "Linux loader DMA"
+
+/* 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__((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 critical 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"
+);
+
-- 
2.4.3

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH v3] Add optionrom compatible with fw_cfg DMA version
  2016-01-25 13:17 [Qemu-devel] [PATCH v3] Add optionrom compatible with fw_cfg DMA version Marc Marí
@ 2016-01-26 11:11 ` Stefan Hajnoczi
  2016-01-26 11:20   ` Marc Marí
  2016-01-28  0:14 ` Kevin O'Connor
  1 sibling, 1 reply; 13+ messages in thread
From: Stefan Hajnoczi @ 2016-01-26 11:11 UTC (permalink / raw)
  To: Marc Marí
  Cc: Gabriel L. Somlo, qemu-devel, Kevin O'Connor, Gerd Hoffmann,
	Paolo Bonzini, Laszlo

[-- Attachment #1: Type: text/plain, Size: 509 bytes --]

On Mon, Jan 25, 2016 at 02:17:48PM +0100, Marc Marí wrote:
> +linuxboot_dma.img: linuxboot_dma.o
> +	$(call quiet-command,$(LD) $(LDFLAGS_NOPIE) -m elf_i386 -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)$@")

Why is -m elf_i386 necessary for linuxboot_dma.img but not for the other
*.img files?


Otherwise:
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH v3] Add optionrom compatible with fw_cfg DMA version
  2016-01-26 11:11 ` Stefan Hajnoczi
@ 2016-01-26 11:20   ` Marc Marí
  2016-01-26 11:26     ` Gerd Hoffmann
  0 siblings, 1 reply; 13+ messages in thread
From: Marc Marí @ 2016-01-26 11:20 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: Gabriel L. Somlo, qemu-devel, Kevin O'Connor, Gerd Hoffmann,
	Paolo Bonzini, Laszlo

On Tue, 26 Jan 2016 11:11:54 +0000
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> On Mon, Jan 25, 2016 at 02:17:48PM +0100, Marc Marí wrote:
> > +linuxboot_dma.img: linuxboot_dma.o
> > +	$(call quiet-command,$(LD) $(LDFLAGS_NOPIE) -m elf_i386
> > -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)$@")  
> 
> Why is -m elf_i386 necessary for linuxboot_dma.img but not for the
> other *.img files?

I cannot give a precise explanation. But if I don't force an output
type, I get this error:

Building optionrom/linuxboot_dma.img
ld: i386 architecture of input file `linuxboot_dma.o' is incompatible
with i386:x86-64 output

Marc

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH v3] Add optionrom compatible with fw_cfg DMA version
  2016-01-26 11:20   ` Marc Marí
@ 2016-01-26 11:26     ` Gerd Hoffmann
  2016-01-27 16:43       ` Stefan Hajnoczi
  0 siblings, 1 reply; 13+ messages in thread
From: Gerd Hoffmann @ 2016-01-26 11:26 UTC (permalink / raw)
  To: Marc Marí
  Cc: Gabriel L. Somlo, qemu-devel, Kevin O'Connor,
	Stefan Hajnoczi, Paolo Bonzini, Laszlo

On Di, 2016-01-26 at 12:20 +0100, Marc Marí wrote:
> On Tue, 26 Jan 2016 11:11:54 +0000
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> > On Mon, Jan 25, 2016 at 02:17:48PM +0100, Marc Marí wrote:
> > > +linuxboot_dma.img: linuxboot_dma.o
> > > +	$(call quiet-command,$(LD) $(LDFLAGS_NOPIE) -m elf_i386
> > > -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)$@")  
> > 
> > Why is -m elf_i386 necessary for linuxboot_dma.img but not for the
> > other *.img files?
> 
> I cannot give a precise explanation. But if I don't force an output
> type, I get this error:
> 
> Building optionrom/linuxboot_dma.img
> ld: i386 architecture of input file `linuxboot_dma.o' is incompatible
> with i386:x86-64 output

Any chance the linker needs -m32 too?

cheers,
  Gerd

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH v3] Add optionrom compatible with fw_cfg DMA version
  2016-01-26 11:26     ` Gerd Hoffmann
@ 2016-01-27 16:43       ` Stefan Hajnoczi
  2016-01-27 16:57         ` Marc Marí
  0 siblings, 1 reply; 13+ messages in thread
From: Stefan Hajnoczi @ 2016-01-27 16:43 UTC (permalink / raw)
  To: Gerd Hoffmann
  Cc: Gabriel L. Somlo, qemu-devel, Kevin O'Connor, Paolo Bonzini,
	Marc Marí,
	Laszlo

[-- Attachment #1: Type: text/plain, Size: 1164 bytes --]

On Tue, Jan 26, 2016 at 12:26:12PM +0100, Gerd Hoffmann wrote:
> On Di, 2016-01-26 at 12:20 +0100, Marc Marí wrote:
> > On Tue, 26 Jan 2016 11:11:54 +0000
> > Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > 
> > > On Mon, Jan 25, 2016 at 02:17:48PM +0100, Marc Marí wrote:
> > > > +linuxboot_dma.img: linuxboot_dma.o
> > > > +	$(call quiet-command,$(LD) $(LDFLAGS_NOPIE) -m elf_i386
> > > > -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)$@")  
> > > 
> > > Why is -m elf_i386 necessary for linuxboot_dma.img but not for the
> > > other *.img files?
> > 
> > I cannot give a precise explanation. But if I don't force an output
> > type, I get this error:
> > 
> > Building optionrom/linuxboot_dma.img
> > ld: i386 architecture of input file `linuxboot_dma.o' is incompatible
> > with i386:x86-64 output
> 
> Any chance the linker needs -m32 too?

I wonder why this isn't a problem for the existing firmware code.  Are
we really building x86_64 ELF files for our firmware?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH v3] Add optionrom compatible with fw_cfg DMA version
  2016-01-27 16:43       ` Stefan Hajnoczi
@ 2016-01-27 16:57         ` Marc Marí
  2016-01-27 17:17           ` Kevin O'Connor
  0 siblings, 1 reply; 13+ messages in thread
From: Marc Marí @ 2016-01-27 16:57 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: Gabriel L. Somlo, qemu-devel, Kevin O'Connor, Gerd Hoffmann,
	Paolo Bonzini, Laszlo

On Wed, 27 Jan 2016 16:43:29 +0000
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> On Tue, Jan 26, 2016 at 12:26:12PM +0100, Gerd Hoffmann wrote:
> > On Di, 2016-01-26 at 12:20 +0100, Marc Marí wrote:  
> > > On Tue, 26 Jan 2016 11:11:54 +0000
> > > Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > >   
> > > > On Mon, Jan 25, 2016 at 02:17:48PM +0100, Marc Marí wrote:  
> > > > > +linuxboot_dma.img: linuxboot_dma.o
> > > > > +	$(call quiet-command,$(LD) $(LDFLAGS_NOPIE) -m
> > > > > elf_i386 -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)$@")    
> > > > 
> > > > Why is -m elf_i386 necessary for linuxboot_dma.img but not for
> > > > the other *.img files?  
> > > 
> > > I cannot give a precise explanation. But if I don't force an
> > > output type, I get this error:
> > > 
> > > Building optionrom/linuxboot_dma.img
> > > ld: i386 architecture of input file `linuxboot_dma.o' is
> > > incompatible with i386:x86-64 output  
> > 
> > Any chance the linker needs -m32 too?  
> 
> I wonder why this isn't a problem for the existing firmware code.  Are
> we really building x86_64 ELF files for our firmware?

Yes they are x86_64:

pc-bios/optionrom/linuxboot.img: ELF 64-bit LSB executable, x86-64,
version 1 (SYSV), statically linked, stripped

But as they are written directly in assembly, it does not give any
problem. Whereas when mixing C and ASM, it does give problems.

Marc

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH v3] Add optionrom compatible with fw_cfg DMA version
  2016-01-27 16:57         ` Marc Marí
@ 2016-01-27 17:17           ` Kevin O'Connor
  2016-01-28 10:18             ` Marc Marí
  0 siblings, 1 reply; 13+ messages in thread
From: Kevin O'Connor @ 2016-01-27 17:17 UTC (permalink / raw)
  To: Marc Marí
  Cc: Gabriel L. Somlo, qemu-devel, Gerd Hoffmann, Stefan Hajnoczi,
	Paolo Bonzini, Laszlo

On Wed, Jan 27, 2016 at 05:57:28PM +0100, Marc Marí wrote:
> On Wed, 27 Jan 2016 16:43:29 +0000
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> > On Tue, Jan 26, 2016 at 12:26:12PM +0100, Gerd Hoffmann wrote:
> > > On Di, 2016-01-26 at 12:20 +0100, Marc Marí wrote:  
> > > > On Tue, 26 Jan 2016 11:11:54 +0000
> > > > Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > > >   
> > > > > On Mon, Jan 25, 2016 at 02:17:48PM +0100, Marc Marí wrote:  
> > > > > > +linuxboot_dma.img: linuxboot_dma.o
> > > > > > +	$(call quiet-command,$(LD) $(LDFLAGS_NOPIE) -m
> > > > > > elf_i386 -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)$@")    
> > > > > 
> > > > > Why is -m elf_i386 necessary for linuxboot_dma.img but not for
> > > > > the other *.img files?  
> > > > 
> > > > I cannot give a precise explanation. But if I don't force an
> > > > output type, I get this error:
> > > > 
> > > > Building optionrom/linuxboot_dma.img
> > > > ld: i386 architecture of input file `linuxboot_dma.o' is
> > > > incompatible with i386:x86-64 output  
> > > 
> > > Any chance the linker needs -m32 too?  
> > 
> > I wonder why this isn't a problem for the existing firmware code.  Are
> > we really building x86_64 ELF files for our firmware?
> 
> Yes they are x86_64:
> 
> pc-bios/optionrom/linuxboot.img: ELF 64-bit LSB executable, x86-64,
> version 1 (SYSV), statically linked, stripped
> 
> But as they are written directly in assembly, it does not give any
> problem. Whereas when mixing C and ASM, it does give problems.

Would it hurt anything to add "-m elf_i386" to the generic assembler
rule and then use that for both targets?  (Just to keep the makefile a
little simpler.)

-Kevin

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH v3] Add optionrom compatible with fw_cfg DMA version
  2016-01-25 13:17 [Qemu-devel] [PATCH v3] Add optionrom compatible with fw_cfg DMA version Marc Marí
  2016-01-26 11:11 ` Stefan Hajnoczi
@ 2016-01-28  0:14 ` Kevin O'Connor
  2016-01-28 11:20   ` Marc Marí
  2016-01-28 21:06   ` Marc Marí
  1 sibling, 2 replies; 13+ messages in thread
From: Kevin O'Connor @ 2016-01-28  0:14 UTC (permalink / raw)
  To: Marc Marí
  Cc: Gabriel L. Somlo, qemu-devel, Gerd Hoffmann, Stefan Hajnoczi,
	Paolo Bonzini, Laszlo

On Mon, Jan 25, 2016 at 02:17:48PM +0100, Marc Marí wrote:
> This optionrom is based on linuxboot.S.

Hi Marc,

Out of curiousity, how does the timing with this option rom compare to
the previous SeaBIOS patches that implemented linux dma loading?


When I first tried to compile this (on fc23), I got:

In file included from /usr/include/features.h:389:0,
                 from /usr/include/stdint.h:25,
                 from /usr/lib/gcc/x86_64-redhat-linux/5.3.1/include/stdint.h:9,
                 from linuxboot_dma.c:62:
/usr/include/gnu/stubs.h:7:27: fatal error: gnu/stubs-32.h: No such file or directory
compilation terminated.

which I fixed by running "dnf install glibc-devel.i686".  Is a
configure check needed?


See further comments below.

[...]
> --- /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"
> +);

The run_linuxboot label doesn't seem to be used anywhere.

[...]
> +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));
> +}

The above does not look correct to me.  Since the code is running in
16bit mode the above memory accesses are relative to the %ds segment.
Because %ds=%cs this is going to access a different address than
expected.

What I think you want to do is assign %es=setup_addr>>4 and then
perform the read at the given offset (eg, 0x206).

[...]
> +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();
> +    }
> +}

FYI, I think with a small incremental patch (see below) one could
entirely replace the existing linuxboot.rom with your new code.

The one caveat is that this patch requires that kvm support "big real
mode" and I know there were quirks with that on some older Intel
chips.  However, I think the "insb" instruction would trap anyway, so
maybe it's not an issue.

-Kevin


--- a/pc-bios/optionrom/linuxboot_dma.c
+++ b/pc-bios/optionrom/linuxboot_dma.c
@@ -73,6 +73,8 @@ asm(
 #define BIOS_CFG_DMA_CTL_SKIP    0x04
 #define BIOS_CFG_DMA_CTL_SELECT  0x08
 
+#define BIOS_CFG_CTL           0x510
+#define BIOS_CFG_DATA          0x511
 #define BIOS_CFG_DMA_ADDR_HIGH 0x514
 #define BIOS_CFG_DMA_ADDR_LOW  0x518
 
@@ -87,6 +89,16 @@ typedef struct FWCfgDmaAccess {
     uint64_t address;
 } __attribute__((packed)) FWCfgDmaAccess;
 
+static inline void outw(uint16_t value, uint16_t port) {
+    asm("outw %w0, %w1" : : "a"(value), "Nd"(port));
+}
+
+static inline uint32_t inl(uint16_t port) {
+    uint32_t value;
+    __asm__ __volatile__("inl %w1, %0" : "=a"(value) : "Nd"(port));
+    return value;
+}
+
 static inline void outl(uint32_t value, uint16_t port) {
     asm("outl %0, %w1" : : "a"(value), "Nd"(port));
 }
@@ -124,6 +136,15 @@ static inline uint32_t be32_to_cpu(uint32_t x) {
 
 static void bios_cfg_read_entry(void *buf, uint16_t entry, uint32_t len)
 {
+    if (inl(BIOS_CFG_DMA_ADDR_LOW) != 0x47464320) {
+        // Legacy PIO fw_cfg
+        outw(entry, BIOS_CFG_CTL);
+        asm volatile("movw %w0, %%es" :: "r"(0) : "memory");
+        asm volatile("rep insb (%%dx), %%es:(%%edi)"
+                     : "+c"(len), "+D"(buf) : "d"(BIOS_CFG_DATA) : "memory");
+        return;
+    }
+
     FWCfgDmaAccess access;
     uint32_t control = (entry << 16) | BIOS_CFG_DMA_CTL_SELECT
                         | BIOS_CFG_DMA_CTL_READ;

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH v3] Add optionrom compatible with fw_cfg DMA version
  2016-01-27 17:17           ` Kevin O'Connor
@ 2016-01-28 10:18             ` Marc Marí
  2016-01-28 10:55               ` Stefan Hajnoczi
  0 siblings, 1 reply; 13+ messages in thread
From: Marc Marí @ 2016-01-28 10:18 UTC (permalink / raw)
  To: Kevin O'Connor
  Cc: Gabriel L. Somlo, qemu-devel, Gerd Hoffmann, Stefan Hajnoczi,
	Paolo Bonzini, Laszlo

On Wed, 27 Jan 2016 12:17:27 -0500
"Kevin O'Connor" <kevin@koconnor.net> wrote:

> On Wed, Jan 27, 2016 at 05:57:28PM +0100, Marc Marí wrote:
> > On Wed, 27 Jan 2016 16:43:29 +0000
> > Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >   
> > > On Tue, Jan 26, 2016 at 12:26:12PM +0100, Gerd Hoffmann wrote:  
> > > > On Di, 2016-01-26 at 12:20 +0100, Marc Marí wrote:    
> > > > > On Tue, 26 Jan 2016 11:11:54 +0000
> > > > > Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > > > >     
> > > > > > On Mon, Jan 25, 2016 at 02:17:48PM +0100, Marc Marí
> > > > > > wrote:    
> > > > > > > +linuxboot_dma.img: linuxboot_dma.o
> > > > > > > +	$(call quiet-command,$(LD) $(LDFLAGS_NOPIE) -m
> > > > > > > elf_i386 -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)$@")      
> > > > > > 
> > > > > > Why is -m elf_i386 necessary for linuxboot_dma.img but not
> > > > > > for the other *.img files?    
> > > > > 
> > > > > I cannot give a precise explanation. But if I don't force an
> > > > > output type, I get this error:
> > > > > 
> > > > > Building optionrom/linuxboot_dma.img
> > > > > ld: i386 architecture of input file `linuxboot_dma.o' is
> > > > > incompatible with i386:x86-64 output    
> > > > 
> > > > Any chance the linker needs -m32 too?    
> > > 
> > > I wonder why this isn't a problem for the existing firmware
> > > code.  Are we really building x86_64 ELF files for our firmware?  
> > 
> > Yes they are x86_64:
> > 
> > pc-bios/optionrom/linuxboot.img: ELF 64-bit LSB executable, x86-64,
> > version 1 (SYSV), statically linked, stripped
> > 
> > But as they are written directly in assembly, it does not give any
> > problem. Whereas when mixing C and ASM, it does give problems.  
> 
> Would it hurt anything to add "-m elf_i386" to the generic assembler
> rule and then use that for both targets?  (Just to keep the makefile a
> little simpler.)

It doesn't seem to hurt, and it is simpler.

Marc

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH v3] Add optionrom compatible with fw_cfg DMA version
  2016-01-28 10:18             ` Marc Marí
@ 2016-01-28 10:55               ` Stefan Hajnoczi
  0 siblings, 0 replies; 13+ messages in thread
From: Stefan Hajnoczi @ 2016-01-28 10:55 UTC (permalink / raw)
  To: Marc Marí
  Cc: Gabriel L. Somlo, qemu-devel, Kevin O'Connor, Gerd Hoffmann,
	Stefan Hajnoczi, Paolo Bonzini, Laszlo

[-- Attachment #1: Type: text/plain, Size: 2352 bytes --]

On Thu, Jan 28, 2016 at 11:18:07AM +0100, Marc Marí wrote:
> On Wed, 27 Jan 2016 12:17:27 -0500
> "Kevin O'Connor" <kevin@koconnor.net> wrote:
> 
> > On Wed, Jan 27, 2016 at 05:57:28PM +0100, Marc Marí wrote:
> > > On Wed, 27 Jan 2016 16:43:29 +0000
> > > Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > >   
> > > > On Tue, Jan 26, 2016 at 12:26:12PM +0100, Gerd Hoffmann wrote:  
> > > > > On Di, 2016-01-26 at 12:20 +0100, Marc Marí wrote:    
> > > > > > On Tue, 26 Jan 2016 11:11:54 +0000
> > > > > > Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > > > > >     
> > > > > > > On Mon, Jan 25, 2016 at 02:17:48PM +0100, Marc Marí
> > > > > > > wrote:    
> > > > > > > > +linuxboot_dma.img: linuxboot_dma.o
> > > > > > > > +	$(call quiet-command,$(LD) $(LDFLAGS_NOPIE) -m
> > > > > > > > elf_i386 -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)$@")      
> > > > > > > 
> > > > > > > Why is -m elf_i386 necessary for linuxboot_dma.img but not
> > > > > > > for the other *.img files?    
> > > > > > 
> > > > > > I cannot give a precise explanation. But if I don't force an
> > > > > > output type, I get this error:
> > > > > > 
> > > > > > Building optionrom/linuxboot_dma.img
> > > > > > ld: i386 architecture of input file `linuxboot_dma.o' is
> > > > > > incompatible with i386:x86-64 output    
> > > > > 
> > > > > Any chance the linker needs -m32 too?    
> > > > 
> > > > I wonder why this isn't a problem for the existing firmware
> > > > code.  Are we really building x86_64 ELF files for our firmware?  
> > > 
> > > Yes they are x86_64:
> > > 
> > > pc-bios/optionrom/linuxboot.img: ELF 64-bit LSB executable, x86-64,
> > > version 1 (SYSV), statically linked, stripped
> > > 
> > > But as they are written directly in assembly, it does not give any
> > > problem. Whereas when mixing C and ASM, it does give problems.  
> > 
> > Would it hurt anything to add "-m elf_i386" to the generic assembler
> > rule and then use that for both targets?  (Just to keep the makefile a
> > little simpler.)
> 
> It doesn't seem to hurt, and it is simpler.

Great, I think this makes sense.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH v3] Add optionrom compatible with fw_cfg DMA version
  2016-01-28  0:14 ` Kevin O'Connor
@ 2016-01-28 11:20   ` Marc Marí
  2016-01-28 15:24     ` Kevin O'Connor
  2016-01-28 21:06   ` Marc Marí
  1 sibling, 1 reply; 13+ messages in thread
From: Marc Marí @ 2016-01-28 11:20 UTC (permalink / raw)
  To: Kevin O'Connor
  Cc: Gabriel L. Somlo, qemu-devel, Gerd Hoffmann, Stefan Hajnoczi,
	Paolo Bonzini, Laszlo

On Wed, 27 Jan 2016 19:14:54 -0500
"Kevin O'Connor" <kevin@koconnor.net> wrote:

> On Mon, Jan 25, 2016 at 02:17:48PM +0100, Marc Marí wrote:
> > This optionrom is based on linuxboot.S.  
> 
> Hi Marc,
> 
> Out of curiousity, how does the timing with this option rom compare to
> the previous SeaBIOS patches that implemented linux dma loading?

This patch
QEMU startup time: .092
BIOS startup time: .047
Kernel setup time: .003
Total time: .142

Current master (fw_cfg DMA enabled, but using linuxboot.img)
QEMU startup time: .083
BIOS startup time: .047
Kernel setup time: .600
Total time: .730

You can see the time loading the kernel (between SeaBIOS function
do_boot and the last instruction in the optionrom) is reduced a lot.

> When I first tried to compile this (on fc23), I got:
> 
> In file included from /usr/include/features.h:389:0,
>                  from /usr/include/stdint.h:25,
>                  from /usr/lib/gcc/x86_64-redhat-linux/5.3.1/include/stdint.h:9,
>                  from linuxboot_dma.c:62:
> /usr/include/gnu/stubs.h:7:27: fatal error: gnu/stubs-32.h: No such
> file or directory compilation terminated.
> 
> which I fixed by running "dnf install glibc-devel.i686".  Is a
> configure check needed?
> 
> 
> See further comments below.
> 
> [...]
> > --- /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"
> > +);  
> 
> The run_linuxboot label doesn't seem to be used anywhere.

No it isn't, I can remove it or leave it as a reference of "code starts
here".

> [...]
> > +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));
> > +}  
> 
> The above does not look correct to me.  Since the code is running in
> 16bit mode the above memory accesses are relative to the %ds segment.
> Because %ds=%cs this is going to access a different address than
> expected.
> 
> What I think you want to do is assign %es=setup_addr>>4 and then
> perform the read at the given offset (eg, 0x206).

I was wondering why it does work, when it shouldn't:

asm("addr32 movl %1, %0" : "=r"(val) : "g"(addr));

This is copying addr to val, but not reading what is in addr. Stupid
mistake!

What do you think of the patch at the end?

> [...]
> > +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();
> > +    }
> > +}  
> 
> FYI, I think with a small incremental patch (see below) one could
> entirely replace the existing linuxboot.rom with your new code.
> 
> The one caveat is that this patch requires that kvm support "big real
> mode" and I know there were quirks with that on some older Intel
> chips.  However, I think the "insb" instruction would trap anyway, so
> maybe it's not an issue.

This has already been mentioned in the previous patch version. There
is no problem on doing it (and it is very simple, as you show). The
last comments on this topics were:

Gerd Hoffman:
>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.

Stefan Hajnoczi:
>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.

I don't mind joining them either.

> 
> 
> --- a/pc-bios/optionrom/linuxboot_dma.c
> +++ b/pc-bios/optionrom/linuxboot_dma.c
> @@ -73,6 +73,8 @@ asm(
>  #define BIOS_CFG_DMA_CTL_SKIP    0x04
>  #define BIOS_CFG_DMA_CTL_SELECT  0x08
>  
> +#define BIOS_CFG_CTL           0x510
> +#define BIOS_CFG_DATA          0x511
>  #define BIOS_CFG_DMA_ADDR_HIGH 0x514
>  #define BIOS_CFG_DMA_ADDR_LOW  0x518
>  
> @@ -87,6 +89,16 @@ typedef struct FWCfgDmaAccess {
>      uint64_t address;
>  } __attribute__((packed)) FWCfgDmaAccess;
>  
> +static inline void outw(uint16_t value, uint16_t port) {
> +    asm("outw %w0, %w1" : : "a"(value), "Nd"(port));
> +}
> +
> +static inline uint32_t inl(uint16_t port) {
> +    uint32_t value;
> +    __asm__ __volatile__("inl %w1, %0" : "=a"(value) : "Nd"(port));
> +    return value;
> +}
> +
>  static inline void outl(uint32_t value, uint16_t port) {
>      asm("outl %0, %w1" : : "a"(value), "Nd"(port));
>  }
> @@ -124,6 +136,15 @@ static inline uint32_t be32_to_cpu(uint32_t x) {
>  
>  static void bios_cfg_read_entry(void *buf, uint16_t entry, uint32_t
> len) {
> +    if (inl(BIOS_CFG_DMA_ADDR_LOW) != 0x47464320) {
> +        // Legacy PIO fw_cfg
> +        outw(entry, BIOS_CFG_CTL);
> +        asm volatile("movw %w0, %%es" :: "r"(0) : "memory");
> +        asm volatile("rep insb (%%dx), %%es:(%%edi)"
> +                     : "+c"(len), "+D"(buf) : "d"(BIOS_CFG_DATA) :
> "memory");
> +        return;
> +    }
> +
>      FWCfgDmaAccess access;
>      uint32_t control = (entry << 16) | BIOS_CFG_DMA_CTL_SELECT
>                          | BIOS_CFG_DMA_CTL_READ;


A small cosmetic comment in this patch: is there any practical reason to
mix the three ways to put inline assembly (asm(), asm volatile() and
__asm__ __volatile__ ()) in this patch?

Thanks for your comments
Marc

Patch for segmentation:

--- a/pc-bios/optionrom/linuxboot_dma.c
+++ b/pc-bios/optionrom/linuxboot_dma.c
@@ -91,23 +91,28 @@ 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) {
+static inline void set_setup_addr(void *addr) {
+    uint32_t seg = (uint32_t)addr >> 4;
+    asm("movl %0, %%es\n" : : "r"(seg));
+}
+
+static inline uint16_t readw_setup(uint16_t offset) {
     uint16_t val;
-    asm("addr32 movw %1, %0" : "=r"(val) : "g"(addr));
+    asm("addr32 movw %%es:(%1), %0" : "=r"(val) :
"r"((uint32_t)offset)); barrier();
     return val;
 }
 
-static inline uint32_t readl_addr32(const void *addr) {
+static inline uint32_t readl_setup(uint16_t offset) {
     uint32_t val;
-    asm("addr32 movl %1, %0" : "=r"(val) : "g"(addr));
+    asm("addr32 movl %%es:(%1), %0" : "=r"(val) :
"r"((uint32_t)offset)); barrier();
     return val;
 }
 
-static inline void writel_addr32(void *addr, uint32_t val) {
+static inline void writel_setup(uint16_t offset, uint32_t val) {
     barrier();
-    asm("addr32 movl %0, %1" : : "r"(val), "g"(addr));
+    asm("addr32 movl %0, %%es:(%1)" : : "r"(val),
"r"((uint32_t)offset)); }
 
 static inline uint64_t cpu_to_be64(uint64_t x) {

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH v3] Add optionrom compatible with fw_cfg DMA version
  2016-01-28 11:20   ` Marc Marí
@ 2016-01-28 15:24     ` Kevin O'Connor
  0 siblings, 0 replies; 13+ messages in thread
From: Kevin O'Connor @ 2016-01-28 15:24 UTC (permalink / raw)
  To: Marc Marí
  Cc: Gabriel L. Somlo, qemu-devel, Gerd Hoffmann, Stefan Hajnoczi,
	Paolo Bonzini, Laszlo

On Thu, Jan 28, 2016 at 12:20:33PM +0100, Marc Marí wrote:
> A small cosmetic comment in this patch: is there any practical reason to
> mix the three ways to put inline assembly (asm(), asm volatile() and
> __asm__ __volatile__ ()) in this patch?

No reason - it was just copy-and-paste crud.

> 
> Thanks for your comments
> Marc
> 
> Patch for segmentation:
> 
> --- a/pc-bios/optionrom/linuxboot_dma.c
> +++ b/pc-bios/optionrom/linuxboot_dma.c
> @@ -91,23 +91,28 @@ 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) {
> +static inline void set_setup_addr(void *addr) {
> +    uint32_t seg = (uint32_t)addr >> 4;
> +    asm("movl %0, %%es\n" : : "r"(seg));
> +}

As a minor comment, I think using "set_es()" and "readw_es()" would be
a little more descriptive.

> +static inline uint16_t readw_setup(uint16_t offset) {
>      uint16_t val;
> -    asm("addr32 movw %1, %0" : "=r"(val) : "g"(addr));
> +    asm("addr32 movw %%es:(%1), %0" : "=r"(val) :
> "r"((uint32_t)offset)); barrier();
>      return val;
>  }

SeaBIOS uses slightly different assembler - see the READx_SEG() macros
at the top of farptr.h.  That said, the above assembler is probably
also fine.

https://github.com/KevinOConnor/seabios/blob/rel-1.9.0/src/farptr.h#L16

-Kevin

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH v3] Add optionrom compatible with fw_cfg DMA version
  2016-01-28  0:14 ` Kevin O'Connor
  2016-01-28 11:20   ` Marc Marí
@ 2016-01-28 21:06   ` Marc Marí
  1 sibling, 0 replies; 13+ messages in thread
From: Marc Marí @ 2016-01-28 21:06 UTC (permalink / raw)
  To: Kevin O'Connor
  Cc: Gabriel L. Somlo, qemu-devel, Gerd Hoffmann, Stefan Hajnoczi,
	Paolo Bonzini, Laszlo


> When I first tried to compile this (on fc23), I got:
> 
> In file included from /usr/include/features.h:389:0,
>                  from /usr/include/stdint.h:25,
>                  from /usr/lib/gcc/x86_64-redhat-linux/5.3.1/include/stdint.h:9,
>                  from linuxboot_dma.c:62:
> /usr/include/gnu/stubs.h:7:27: fatal error: gnu/stubs-32.h: No such
> file or directory compilation terminated.
> 
> which I fixed by running "dnf install glibc-devel.i686".  Is a
> configure check needed?

I forgot to address this point this morning. I now put up a clean setup
and I had the same issue. I'll add the configure check, but I don't know
if there is a nicer solution.

Thanks
Marc

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2016-01-28 21:06 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-25 13:17 [Qemu-devel] [PATCH v3] Add optionrom compatible with fw_cfg DMA version Marc Marí
2016-01-26 11:11 ` Stefan Hajnoczi
2016-01-26 11:20   ` Marc Marí
2016-01-26 11:26     ` Gerd Hoffmann
2016-01-27 16:43       ` Stefan Hajnoczi
2016-01-27 16:57         ` Marc Marí
2016-01-27 17:17           ` Kevin O'Connor
2016-01-28 10:18             ` Marc Marí
2016-01-28 10:55               ` Stefan Hajnoczi
2016-01-28  0:14 ` Kevin O'Connor
2016-01-28 11:20   ` Marc Marí
2016-01-28 15:24     ` Kevin O'Connor
2016-01-28 21:06   ` Marc Marí

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.