From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:44297)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <kevin@koconnor.net>) id 1aOoQh-0007Nn-DQ
	for qemu-devel@nongnu.org; Thu, 28 Jan 2016 10:24:19 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <kevin@koconnor.net>) id 1aOoQe-0000Js-53
	for qemu-devel@nongnu.org; Thu, 28 Jan 2016 10:24:15 -0500
Received: from mail-qk0-x235.google.com ([2607:f8b0:400d:c09::235]:36274)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <kevin@koconnor.net>) id 1aOoQd-0000Jl-WD
	for qemu-devel@nongnu.org; Thu, 28 Jan 2016 10:24:12 -0500
Received: by mail-qk0-x235.google.com with SMTP id s68so12195812qkh.3
	for <qemu-devel@nongnu.org>; Thu, 28 Jan 2016 07:24:11 -0800 (PST)
Date: Thu, 28 Jan 2016 10:24:10 -0500
From: Kevin O'Connor <kevin@koconnor.net>
Message-ID: <20160128152410.GA22389@morn.lan>
References: <1453727868-11147-1-git-send-email-markmb@redhat.com>
	<20160128001454.GA27233@morn.lan>
	<20160128122033.10347566@markmb_rh>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20160128122033.10347566@markmb_rh>
Subject: Re: [Qemu-devel] [PATCH v3] Add optionrom compatible with fw_cfg
	DMA version
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Marc =?iso-8859-1?Q?Mar=ED?= <markmb@redhat.com>
Cc: "Gabriel L. Somlo" <somlo@cmu.edu>, qemu-devel@nongnu.org, Gerd Hoffmann <kraxel@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Laszlo <lersek@redhat.com>

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