From patchwork Thu Jan 28 11:20:33 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Marc_Mar=C3=AD?= X-Patchwork-Id: 8149511 Return-Path: X-Original-To: patchwork-qemu-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 5D0029FE6C for ; Thu, 28 Jan 2016 11:20:59 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 2DBA92035B for ; Thu, 28 Jan 2016 11:20:58 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 9CDB1201FE for ; Thu, 28 Jan 2016 11:20:56 +0000 (UTC) Received: from localhost ([::1]:55478 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aOkdD-0008Rg-PJ for patchwork-qemu-devel@patchwork.kernel.org; Thu, 28 Jan 2016 06:20:55 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47699) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aOkd5-0008RV-1s for qemu-devel@nongnu.org; Thu, 28 Jan 2016 06:20:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aOkd0-0003nK-6D for qemu-devel@nongnu.org; Thu, 28 Jan 2016 06:20:47 -0500 Received: from mx1.redhat.com ([209.132.183.28]:39272) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aOkcz-0003n6-Us for qemu-devel@nongnu.org; Thu, 28 Jan 2016 06:20:42 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id DB55C32D3A3; Thu, 28 Jan 2016 11:20:39 +0000 (UTC) Received: from markmb_rh (vpn1-5-137.ams2.redhat.com [10.36.5.137]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u0SBKalg025346 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Thu, 28 Jan 2016 06:20:37 -0500 Date: Thu, 28 Jan 2016 12:20:33 +0100 From: Marc =?UTF-8?B?TWFyw60=?= To: "Kevin O'Connor" Message-ID: <20160128122033.10347566@markmb_rh> In-Reply-To: <20160128001454.GA27233@morn.lan> References: <1453727868-11147-1-git-send-email-markmb@redhat.com> <20160128001454.GA27233@morn.lan> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: "Gabriel L. Somlo" , qemu-devel@nongnu.org, Gerd Hoffmann , Stefan Hajnoczi , Paolo Bonzini , Laszlo Subject: Re: [Qemu-devel] [PATCH v3] Add optionrom compatible with fw_cfg DMA version X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Wed, 27 Jan 2016 19:14:54 -0500 "Kevin O'Connor" 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 > > . > > + * > > + * Copyright (c) 2015 Red Hat Inc. > > + * Authors: Marc Marí > > + */ > > + > > +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: static inline uint64_t cpu_to_be64(uint64_t x) { --- 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)); }