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 <machine-type>.
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)&access), 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
