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

[Marc Marí]

On Mon, 18 Jan 2016 14:42:06 +0000
Stefan Hajnoczi <address@hidden> 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 <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?

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)&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)?

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.

> > +    /* 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.

I need only these two lines:

#define NO_QEMU_PROTOS
#include "../../include/hw/nvram/fw_cfg.h"

Which can be added to linuxboot_dma.c instead of including optionrom.h

And the macros cannot be reused (that was the original idea) because
those macros are written to be used directly in ASM sources. They
cannot be used as embedded ASM in a C source (they are missing, at
least, the line feed at the end of each instruction). What I can do, is
branch the macro definition with "ifdef C_CODE".

Thanks
Marc