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

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4541 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]