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

[Kevin O'Connor]

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