From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41916) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLB7X-0005Kt-Hy for qemu-devel@nongnu.org; Mon, 18 Jan 2016 09:49:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aLB7T-0003Lb-9H for qemu-devel@nongnu.org; Mon, 18 Jan 2016 09:49:27 -0500 Received: from mail-wm0-x231.google.com ([2a00:1450:400c:c09::231]:35894) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLB7S-0003LP-UK for qemu-devel@nongnu.org; Mon, 18 Jan 2016 09:49:23 -0500 Received: by mail-wm0-x231.google.com with SMTP id l65so102503980wmf.1 for ; Mon, 18 Jan 2016 06:49:22 -0800 (PST) Date: Mon, 18 Jan 2016 14:42:06 +0000 From: Stefan Hajnoczi Message-ID: <20160118144206.GA32286@stefanha-x1.localdomain> References: <1453111140-15128-1-git-send-email-markmb@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="EeQfGwPcQSOJBaQU" Content-Disposition: inline In-Reply-To: <1453111140-15128-1-git-send-email-markmb@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2] Add optionrom compatible with fw_cfg DMA version List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marc =?iso-8859-1?Q?Mar=ED?= Cc: "Gabriel L. Somlo" , qemu-devel@nongnu.org, Kevin O'Connor , Stefan Hajnoczi , Paolo Bonzini , Laszlo --EeQfGwPcQSOJBaQU Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Jan 18, 2016 at 10:59:00AM +0100, Marc Mar=ED 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); > =20 > - option_rom[nb_option_roms].name =3D "linuxboot.bin"; > - option_rom[nb_option_roms].bootindex =3D 0; > + if (fw_cfg_dma_enabled(fw_cfg)) { > + option_rom[nb_option_roms].name =3D "linuxboot_dma.bin"; > + option_rom[nb_option_roms].bootindex =3D 0; > + } else { > + option_rom[nb_option_roms].name =3D "linuxboot.bin"; > + option_rom[nb_option_roms].bootindex =3D 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 > =20 > +BULD_DIR=3D$(CURDIR) Is this a typo (BUILD_DIR)? Is it even needed? > include ../../config-host.mak > include $(SRC_PATH)/rules.mak > =20 > @@ -11,15 +12,20 @@ $(call set-vpath, $(SRC_PATH)/pc-bios/optionrom) > =20 > CFLAGS :=3D -Wall -Wstrict-prototypes -Werror -fomit-frame-pointer -fno-= builtin > CFLAGS +=3D -I$(SRC_PATH) > +CFLAGS +=3D -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 =3D (entry << 16) | BIOS_CFG_DMA_CTL_SELECT > + | BIOS_CFG_DMA_CTL_READ; > + > + access.address =3D cpu_to_be64((uint64_t)(uint32_t)buf); > + access.length =3D cpu_to_be32(len); > + access.control =3D 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: > -=09 > + > +#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. --EeQfGwPcQSOJBaQU Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJWnPm+AAoJEJykq7OBq3PIk6oH/jRwiS/4CNMGyiPAwOEOF9XU TZ5hMZ53qzUyMShN70e4MAbRX0Etp8jSktjqrobhRa4YhnX+3uyDoIMSZz1ZJT5U qAXdtTRVsjydZoBMliXwo/6MHXrJmzhL8dIpEv7k1YmiCxnuc1mau5PSMbaYGnXc zgoOs795Pe12QENIqvStASk6iTbqmRyX4Daw/IEnOjfV9kMNQ7RqV9t6zmfprGJ+ VsJ4lvWF4+RXaKGLNhPhW6UEAe+4ZuHJDgpJCmudNDYXPYicOVARaJSVMz92kw2a iTLFR9lImIa6FWiqZOEuYX2DiL6YvvMKwoWJ693y+epTq4jLVfcsLZzO7VluxP4= =WztX -----END PGP SIGNATURE----- --EeQfGwPcQSOJBaQU--