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

[Marc Marí]

On Wed, 27 Jan 2016 19:14:54 -0500
"Kevin O'Connor" <address@hidden> 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í <address@hidden>
> > + */
> > +
> > +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) {