Re: [Qemu-devel] [PATCH v3] Add optionrom compatible with fw_cfg DMA version

[Kevin O'Connor <kevin@koconnor.net>]

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;