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

2016-01-21 Thread Stefan Hajnoczi 
On Tue, Jan 19, 2016 at 01:11:47PM +0100, Gerd Hoffmann wrote:
> > If you really think they should be merged, I'd even propose to
> > merge the ASM version onto the C version (convert this patch into
> > linuxboot.S). This slightly improves readability.
> 
> Fully agree.  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.

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.

Stefan


signature.asc
Description: PGP signature

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

2016-01-19 Thread Gerd Hoffmann 
  Hi,

> > > +if (fw_cfg_dma_enabled(fw_cfg)) {
> > > +option_rom[nb_option_roms].name = "linuxboot_dma.bin";
> > > +option_rom[nb_option_roms].bootindex = 0;
> > > +} else {
> > > +option_rom[nb_option_roms].name = "linuxboot.bin";
> > > +option_rom[nb_option_roms].bootindex = 0;
> > > +}  
> > 
> > Live migration compatibility requires that guest-visible changes to
> > the machine are only introduced in a new -machine .

> > I've CCed Gerd and Juan, I think they know how changes to Option ROMs
> > affect live migration better than me.  What needs to be done to
> > preserve live migration compatibility?
> 
> They are CC'd now :)

I think we are fine here.  The dma interface is enabled for new machine
types only, thats why we have fw_cfg_dma_enabled() in the first place ;)

> > Was there a technical reason why linuxboot.S cannot be extended
> > (e.g.  a size limit)?
> 
> I don't think there's a technical reason. It is a lot simpler to write
> the fw_cfg DMA stuff in C. To extend linuxboot.S these things should be
> modified:
>  - Add fw_cfg DMA detection support
>  - Change read_fw from a macro to a function that checks for fw_cfg DMA
>support and does the operation using IO or memory
>  - Extract bits and pieces from linuxboot.S into functions, that are
>only necessary when there is no support for fw_cfg DMA (the most
>important is jumping to 32 bits to read and copy the kernel).
> 
> This way, you check for support from the very beggining (when
> configuring the machine), and you don't have to branch the code
> anymore.
> 
> (I think I discussed this with somebody in the past. But I'm not sure
> with whom, or when. So I'll suppose it was a dream and it is not on the
> archives).

Could have been /me.

The fw_cfg macros in linuxboot.S are messy, looks like because they got
extended a few times.  Piling DMA support on top of that didn't look
very appealing to me.

Also DMA support simplifies things, there is no need to switch processor
modes to load the kernel above 1M.

> If you really think they should be merged, I'd even propose to
> merge the ASM version onto the C version (convert this patch into
> linuxboot.S). This slightly improves readability.

Fully agree.  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.

cheers,
  Gerd

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

2016-01-18 Thread Stefan Hajnoczi 
On Mon, Jan 18, 2016 at 10:59:00AM +0100, Marc Marí wrote:
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 459260b..00339fa 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1007,8 +1007,13 @@ static void load_linux(PCMachineState *pcms,
>  fw_cfg_add_i32(fw_cfg, FW_CFG_SETUP_SIZE, setup_size);
>  fw_cfg_add_bytes(fw_cfg, FW_CFG_SETUP_DATA, setup, setup_size);
>  
> -option_rom[nb_option_roms].name = "linuxboot.bin";
> -option_rom[nb_option_roms].bootindex = 0;
> +if (fw_cfg_dma_enabled(fw_cfg)) {
> +option_rom[nb_option_roms].name = "linuxboot_dma.bin";
> +option_rom[nb_option_roms].bootindex = 0;
> +} else {
> +option_rom[nb_option_roms].name = "linuxboot.bin";
> +option_rom[nb_option_roms].bootindex = 0;
> +}

Live migration compatibility requires that guest-visible changes to the
machine are only introduced in a new -machine .

This ensures that migrating from an older QEMU version to a newer QEMU
version will not change the guest-visible memory layout or device
behavior.

The Option ROM should not change when migration between different QEMU
versions.

I've CCed Gerd and Juan, I think they know how changes to Option ROMs
affect live migration better than me.  What needs to be done to preserve
live migration compatibility?

> diff --git a/pc-bios/optionrom/Makefile b/pc-bios/optionrom/Makefile
> index ce4852a..076f351 100644
> --- a/pc-bios/optionrom/Makefile
> +++ b/pc-bios/optionrom/Makefile
> @@ -2,6 +2,7 @@ all: build-all
>  # Dummy command so that make thinks it has done something
>   @true
>  
> +BULD_DIR=$(CURDIR)

Is this a typo (BUILD_DIR)?  Is it even needed?

>  include ../../config-host.mak
>  include $(SRC_PATH)/rules.mak
>  
> @@ -11,15 +12,20 @@ $(call set-vpath, $(SRC_PATH)/pc-bios/optionrom)
>  
>  CFLAGS := -Wall -Wstrict-prototypes -Werror -fomit-frame-pointer -fno-builtin
>  CFLAGS += -I$(SRC_PATH)
> +CFLAGS += -I$(SRC_PATH)/include

Are you using any QEMU headers?  If not, then this change can be
dropped.

> +linuxboot_dma.img: linuxboot_dma.o
> + $(call quiet-command,$(LD) $(LDFLAGS_NOPIE) -m elf_i386 -static -Ttext 
> 0 -e _start -s -o $@ $<,"  Building $(TARGET_DIR)$@")

Why is -static necessary?

> +#define BOOT_ROM_PRODUCT "Linux loader"

Please differentiate from linuxboot.S.  Maybe call it "Linux loader
DMA".

> +typedef struct FWCfgDmaAccess {
> +uint32_t control;
> +uint32_t length;
> +uint64_t address;
> +} __attribute__((gcc_struct, packed)) FWCfgDmaAccess;

gcc_struct is for gcc vs Windows compiler compatibility when the struct
contains bitfields.  No bitfields are used and no Windows compiled code
is linked, so it seems unnecessary.

> +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)), BIOS_CFG_DMA_ADDR_LOW);
> +
> +while(be32_to_cpu(access.control) & ~BIOS_CFG_DMA_CTL_ERROR) {
> +barrier();
> +}
> +}

This function is the unique part of linuxboot_dma.c.  Everything else is
copied and translated from linuxboot.S.  The refactorer in me wonders
how hard it would be to extend linuxboot.S instead of introducing this
new boot ROM.

Was there a technical reason why linuxboot.S cannot be extended (e.g.  a
size limit)?

> +/* As we are changing crytical registers, we cannot leave freedom to the

s/crytical/critical/

> diff --git a/pc-bios/optionrom/optionrom.h b/pc-bios/optionrom/optionrom.h
> index f1a9021..25c765f 100644
> --- a/pc-bios/optionrom/optionrom.h
> +++ b/pc-bios/optionrom/optionrom.h
> @@ -29,7 +29,8 @@
>  #define DEBUG_HERE \
>   jmp 1f; \
>   1:
> - 
> +
> +#ifndef C_CODE
>  /*
>   * Read a variable from the fw_cfg device.
>   * Clobbers: %edx
> @@ -49,6 +50,7 @@
>   inb (%dx), %al
>   bswap   %eax
>  .endm
> +#endif

Why do you need optionrom.h?  You seem to have expanded its macros
anyway (OPTION_ROM_START, BOOT_ROM_START, OPTION_ROM_END, BOOT_ROM_END).

If you need optionrom.h, please actually use its macros instead of
expanding them.

Otherwise, please don't include it.


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH v2] Add optionrom compatible with fw_cfg DMA version

2016-01-18 Thread Marc Marí 
This optionrom is based on linuxboot.S.

Signed-off-by: Marc Marí 
---
 .gitignore|   4 +
 hw/i386/pc.c  |   9 +-
 hw/nvram/fw_cfg.c |   2 +-
 include/hw/nvram/fw_cfg.h |   1 +
 pc-bios/optionrom/Makefile|   8 +-
 pc-bios/optionrom/linuxboot_dma.c | 262 ++
 pc-bios/optionrom/optionrom.h |   4 +-
 7 files changed, 285 insertions(+), 5 deletions(-)
 create mode 100644 pc-bios/optionrom/linuxboot_dma.c

diff --git a/.gitignore b/.gitignore
index 88a80ff..101d1e0 100644
--- a/.gitignore
+++ b/.gitignore
@@ -94,6 +94,10 @@
 /pc-bios/optionrom/linuxboot.bin
 /pc-bios/optionrom/linuxboot.raw
 /pc-bios/optionrom/linuxboot.img
+/pc-bios/optionrom/linuxboot_dma.asm
+/pc-bios/optionrom/linuxboot_dma.bin
+/pc-bios/optionrom/linuxboot_dma.raw
+/pc-bios/optionrom/linuxboot_dma.img
 /pc-bios/optionrom/multiboot.asm
 /pc-bios/optionrom/multiboot.bin
 /pc-bios/optionrom/multiboot.raw
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 459260b..00339fa 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1007,8 +1007,13 @@ static void load_linux(PCMachineState *pcms,
 fw_cfg_add_i32(fw_cfg, FW_CFG_SETUP_SIZE, setup_size);
 fw_cfg_add_bytes(fw_cfg, FW_CFG_SETUP_DATA, setup, setup_size);
 
-option_rom[nb_option_roms].name = "linuxboot.bin";
-option_rom[nb_option_roms].bootindex = 0;
+if (fw_cfg_dma_enabled(fw_cfg)) {
+option_rom[nb_option_roms].name = "linuxboot_dma.bin";
+option_rom[nb_option_roms].bootindex = 0;
+} else {
+option_rom[nb_option_roms].name = "linuxboot.bin";
+option_rom[nb_option_roms].bootindex = 0;
+}
 nb_option_roms++;
 }
 
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index a1d650d..d0a5753 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -546,7 +546,7 @@ static bool is_version_1(void *opaque, int version_id)
 return version_id == 1;
 }
 
-static bool fw_cfg_dma_enabled(void *opaque)
+bool fw_cfg_dma_enabled(void *opaque)
 {
 FWCfgState *s = opaque;
 
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index 664eaf6..953e58d 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -219,6 +219,7 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
  hwaddr dma_addr, AddressSpace *dma_as);
 
 FWCfgState *fw_cfg_find(void);
+bool fw_cfg_dma_enabled(void *opaque);
 
 #endif /* NO_QEMU_PROTOS */
 
diff --git a/pc-bios/optionrom/Makefile b/pc-bios/optionrom/Makefile
index ce4852a..076f351 100644
--- a/pc-bios/optionrom/Makefile
+++ b/pc-bios/optionrom/Makefile
@@ -2,6 +2,7 @@ all: build-all
 # Dummy command so that make thinks it has done something
@true
 
+BULD_DIR=$(CURDIR)
 include ../../config-host.mak
 include $(SRC_PATH)/rules.mak
 
@@ -11,15 +12,20 @@ $(call set-vpath, $(SRC_PATH)/pc-bios/optionrom)
 
 CFLAGS := -Wall -Wstrict-prototypes -Werror -fomit-frame-pointer -fno-builtin
 CFLAGS += -I$(SRC_PATH)
+CFLAGS += -I$(SRC_PATH)/include
 CFLAGS += $(call cc-option, $(CFLAGS), -fno-stack-protector)
 CFLAGS += $(CFLAGS_NOPIE)
+CFLAGS += -m32
 QEMU_CFLAGS = $(CFLAGS)
 
-build-all: multiboot.bin linuxboot.bin kvmvapic.bin
+build-all: multiboot.bin linuxboot.bin linuxboot_dma.bin kvmvapic.bin
 
 # suppress auto-removal of intermediate files
 .SECONDARY:
 
+linuxboot_dma.img: linuxboot_dma.o
+   $(call quiet-command,$(LD) $(LDFLAGS_NOPIE) -m elf_i386 -static -Ttext 
0 -e _start -s -o $@ $<,"  Building $(TARGET_DIR)$@")
+
 %.img: %.o
$(call quiet-command,$(LD) $(LDFLAGS_NOPIE) -Ttext 0 -e _start -s -o $@ 
$<,"  Building $(TARGET_DIR)$@")
 
diff --git a/pc-bios/optionrom/linuxboot_dma.c 
b/pc-bios/optionrom/linuxboot_dma.c
new file mode 100644
index 000..9c355fd
--- /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 0x\n"
+"   .byte 0x00\n"

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

2016-01-18 Thread Paolo Bonzini 


On 18/01/2016 10:59, Marc Marí wrote:
> This optionrom is based on linuxboot.S.
> 
> Signed-off-by: Marc Marí 

Reviewed-by: Paolo Bonzini 

> ---
>  .gitignore|   4 +
>  hw/i386/pc.c  |   9 +-
>  hw/nvram/fw_cfg.c |   2 +-
>  include/hw/nvram/fw_cfg.h |   1 +
>  pc-bios/optionrom/Makefile|   8 +-
>  pc-bios/optionrom/linuxboot_dma.c | 262 
> ++
>  pc-bios/optionrom/optionrom.h |   4 +-
>  7 files changed, 285 insertions(+), 5 deletions(-)
>  create mode 100644 pc-bios/optionrom/linuxboot_dma.c
> 
> diff --git a/.gitignore b/.gitignore
> index 88a80ff..101d1e0 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -94,6 +94,10 @@
>  /pc-bios/optionrom/linuxboot.bin
>  /pc-bios/optionrom/linuxboot.raw
>  /pc-bios/optionrom/linuxboot.img
> +/pc-bios/optionrom/linuxboot_dma.asm
> +/pc-bios/optionrom/linuxboot_dma.bin
> +/pc-bios/optionrom/linuxboot_dma.raw
> +/pc-bios/optionrom/linuxboot_dma.img
>  /pc-bios/optionrom/multiboot.asm
>  /pc-bios/optionrom/multiboot.bin
>  /pc-bios/optionrom/multiboot.raw
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 459260b..00339fa 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1007,8 +1007,13 @@ static void load_linux(PCMachineState *pcms,
>  fw_cfg_add_i32(fw_cfg, FW_CFG_SETUP_SIZE, setup_size);
>  fw_cfg_add_bytes(fw_cfg, FW_CFG_SETUP_DATA, setup, setup_size);
>  
> -option_rom[nb_option_roms].name = "linuxboot.bin";
> -option_rom[nb_option_roms].bootindex = 0;
> +if (fw_cfg_dma_enabled(fw_cfg)) {
> +option_rom[nb_option_roms].name = "linuxboot_dma.bin";
> +option_rom[nb_option_roms].bootindex = 0;
> +} else {
> +option_rom[nb_option_roms].name = "linuxboot.bin";
> +option_rom[nb_option_roms].bootindex = 0;
> +}
>  nb_option_roms++;
>  }
>  
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index a1d650d..d0a5753 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -546,7 +546,7 @@ static bool is_version_1(void *opaque, int version_id)
>  return version_id == 1;
>  }
>  
> -static bool fw_cfg_dma_enabled(void *opaque)
> +bool fw_cfg_dma_enabled(void *opaque)
>  {
>  FWCfgState *s = opaque;
>  
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index 664eaf6..953e58d 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -219,6 +219,7 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
>   hwaddr dma_addr, AddressSpace *dma_as);
>  
>  FWCfgState *fw_cfg_find(void);
> +bool fw_cfg_dma_enabled(void *opaque);
>  
>  #endif /* NO_QEMU_PROTOS */
>  
> diff --git a/pc-bios/optionrom/Makefile b/pc-bios/optionrom/Makefile
> index ce4852a..076f351 100644
> --- a/pc-bios/optionrom/Makefile
> +++ b/pc-bios/optionrom/Makefile
> @@ -2,6 +2,7 @@ all: build-all
>  # Dummy command so that make thinks it has done something
>   @true
>  
> +BULD_DIR=$(CURDIR)
>  include ../../config-host.mak
>  include $(SRC_PATH)/rules.mak
>  
> @@ -11,15 +12,20 @@ $(call set-vpath, $(SRC_PATH)/pc-bios/optionrom)
>  
>  CFLAGS := -Wall -Wstrict-prototypes -Werror -fomit-frame-pointer -fno-builtin
>  CFLAGS += -I$(SRC_PATH)
> +CFLAGS += -I$(SRC_PATH)/include
>  CFLAGS += $(call cc-option, $(CFLAGS), -fno-stack-protector)
>  CFLAGS += $(CFLAGS_NOPIE)
> +CFLAGS += -m32
>  QEMU_CFLAGS = $(CFLAGS)
>  
> -build-all: multiboot.bin linuxboot.bin kvmvapic.bin
> +build-all: multiboot.bin linuxboot.bin linuxboot_dma.bin kvmvapic.bin
>  
>  # suppress auto-removal of intermediate files
>  .SECONDARY:
>  
> +linuxboot_dma.img: linuxboot_dma.o
> + $(call quiet-command,$(LD) $(LDFLAGS_NOPIE) -m elf_i386 -static -Ttext 
> 0 -e _start -s -o $@ $<,"  Building $(TARGET_DIR)$@")
> +
>  %.img: %.o
>   $(call quiet-command,$(LD) $(LDFLAGS_NOPIE) -Ttext 0 -e _start -s -o $@ 
> $<,"  Building $(TARGET_DIR)$@")
>  
> diff --git a/pc-bios/optionrom/linuxboot_dma.c 
> b/pc-bios/optionrom/linuxboot_dma.c
> new file mode 100644
> index 000..9c355fd
> --- /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.
> + *

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

2016-01-18 Thread Kevin O'Connor 
On Mon, Jan 18, 2016 at 05:22:09PM +0100, Marc Marí wrote:
> On Mon, 18 Jan 2016 14:42:06 +
> Stefan Hajnoczi  wrote:
> > Was there a technical reason why linuxboot.S cannot be extended
> > (e.g.  a size limit)?
> 
> I don't think there's a technical reason. It is a lot simpler to write
> the fw_cfg DMA stuff in C. To extend linuxboot.S these things should be
> modified:
>  - Add fw_cfg DMA detection support
>  - Change read_fw from a macro to a function that checks for fw_cfg DMA
>support and does the operation using IO or memory
>  - Extract bits and pieces from linuxboot.S into functions, that are
>only necessary when there is no support for fw_cfg DMA (the most
>important is jumping to 32 bits to read and copy the kernel).
> 
> This way, you check for support from the very beggining (when
> configuring the machine), and you don't have to branch the code
> anymore.
> 
> (I think I discussed this with somebody in the past. But I'm not sure
> with whom, or when. So I'll suppose it was a dream and it is not on the
> archives).
> 
> If you really think they should be merged, I'd even propose to
> merge the ASM version onto the C version (convert this patch into
> linuxboot.S). This slightly improves readability.

FYI, if the existing linuxboot.S jumps to 32bit mode just to copy the
kernel then that isn't necessary.  By specification, option roms run
in "big real" mode and thus can directly read/write to the first 4G of
ram.

-Kevin

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

2016-01-18 Thread Marc Marí 
On Mon, 18 Jan 2016 14:42:06 +
Stefan Hajnoczi  wrote:

> On Mon, Jan 18, 2016 at 10:59:00AM +0100, Marc Marí wrote:
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 459260b..00339fa 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1007,8 +1007,13 @@ static void load_linux(PCMachineState *pcms,
> >  fw_cfg_add_i32(fw_cfg, FW_CFG_SETUP_SIZE, setup_size);
> >  fw_cfg_add_bytes(fw_cfg, FW_CFG_SETUP_DATA, setup, setup_size);
> >  
> > -option_rom[nb_option_roms].name = "linuxboot.bin";
> > -option_rom[nb_option_roms].bootindex = 0;
> > +if (fw_cfg_dma_enabled(fw_cfg)) {
> > +option_rom[nb_option_roms].name = "linuxboot_dma.bin";
> > +option_rom[nb_option_roms].bootindex = 0;
> > +} else {
> > +option_rom[nb_option_roms].name = "linuxboot.bin";
> > +option_rom[nb_option_roms].bootindex = 0;
> > +}  
> 
> Live migration compatibility requires that guest-visible changes to
> the machine are only introduced in a new -machine .
> 
> This ensures that migrating from an older QEMU version to a newer QEMU
> version will not change the guest-visible memory layout or device
> behavior.
> 
> The Option ROM should not change when migration between different QEMU
> versions.
> 
> I've CCed Gerd and Juan, I think they know how changes to Option ROMs
> affect live migration better than me.  What needs to be done to
> preserve live migration compatibility?

They are CC'd now :)

> > diff --git a/pc-bios/optionrom/Makefile b/pc-bios/optionrom/Makefile
> > index ce4852a..076f351 100644
> > --- a/pc-bios/optionrom/Makefile
> > +++ b/pc-bios/optionrom/Makefile
> > @@ -2,6 +2,7 @@ all: build-all
> >  # Dummy command so that make thinks it has done something
> > @true
> >  
> > +BULD_DIR=$(CURDIR)  
> 
> Is this a typo (BUILD_DIR)?  Is it even needed?

Experiments and trials in the past that are not necessary now and I
forgot to remove. They have been removed now.

> >  include ../../config-host.mak
> >  include $(SRC_PATH)/rules.mak
> >  
> > @@ -11,15 +12,20 @@ $(call set-vpath, $(SRC_PATH)/pc-bios/optionrom)
> >  
> >  CFLAGS := -Wall -Wstrict-prototypes -Werror -fomit-frame-pointer
> > -fno-builtin CFLAGS += -I$(SRC_PATH)
> > +CFLAGS += -I$(SRC_PATH)/include  
> 
> Are you using any QEMU headers?  If not, then this change can be
> dropped.

Same as above.
 
> > +linuxboot_dma.img: linuxboot_dma.o
> > +   $(call quiet-command,$(LD) $(LDFLAGS_NOPIE) -m elf_i386
> > -static -Ttext 0 -e _start -s -o $@ $<,"  Building
> > $(TARGET_DIR)$@")  
> 
> Why is -static necessary?

Same as above.

> 
> > +#define BOOT_ROM_PRODUCT "Linux loader"  
> 
> Please differentiate from linuxboot.S.  Maybe call it "Linux loader
> DMA".
> 
> > +typedef struct FWCfgDmaAccess {
> > +uint32_t control;
> > +uint32_t length;
> > +uint64_t address;
> > +} __attribute__((gcc_struct, packed)) FWCfgDmaAccess;  
> 
> gcc_struct is for gcc vs Windows compiler compatibility when the
> struct contains bitfields.  No bitfields are used and no Windows
> compiled code is linked, so it seems unnecessary.
> 
> > +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)), BIOS_CFG_DMA_ADDR_LOW);
> > +
> > +while(be32_to_cpu(access.control) & ~BIOS_CFG_DMA_CTL_ERROR) {
> > +barrier();
> > +}
> > +}  
> 
> This function is the unique part of linuxboot_dma.c.  Everything else
> is copied and translated from linuxboot.S.  The refactorer in me
> wonders how hard it would be to extend linuxboot.S instead of
> introducing this new boot ROM.
> 
> Was there a technical reason why linuxboot.S cannot be extended
> (e.g.  a size limit)?

I don't think there's a technical reason. It is a lot simpler to write
the fw_cfg DMA stuff in C. To extend linuxboot.S these things should be
modified:
 - Add fw_cfg DMA detection support
 - Change read_fw from a macro to a function that checks for fw_cfg DMA
   support and does the operation using IO or memory
 - Extract bits and pieces from linuxboot.S into functions, that are
   only necessary when there is no support for fw_cfg DMA (the most
   important is jumping to 32 bits to read and copy the kernel).

This way, you check for support from the very beggining (when
configuring the machine), and you don't have to branch the code
anymore.

(I think I discussed this with somebody in the past. But I'm not sure
with whom, or when. So I'll suppose it was a dream and it is not on the
archives).

If you really think they should be merged, I'd even propose to
merge the ASM version onto the C version (convert this patch