From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MtHhp-0002sD-4Z for qemu-devel@nongnu.org; Thu, 01 Oct 2009 05:12:09 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MtHhj-0002mH-Ci for qemu-devel@nongnu.org; Thu, 01 Oct 2009 05:12:08 -0400 Received: from [199.232.76.173] (port=52500 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MtHhj-0002mB-2x for qemu-devel@nongnu.org; Thu, 01 Oct 2009 05:12:03 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51129) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MtHhi-0001GJ-NU for qemu-devel@nongnu.org; Thu, 01 Oct 2009 05:12:02 -0400 Message-ID: <4AC4725D.1020009@redhat.com> Date: Thu, 01 Oct 2009 11:11:57 +0200 From: Gerd Hoffmann MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] Reorganize option rom (+linux kernel) loading. References: <1254325401-18777-1-git-send-email-kraxel@redhat.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Blue Swirl Cc: qemu-devel@nongnu.org Hi, > Nice idea. The implementation seems to be buggy, I only ever see one > rom On !pc platforms only elf roms will show up. > and for example sparc-test crashes when issuing 'info roms'. Huh? Works for me. Details please. sparc is one of the archs I've actually tested: (qemu) info roms addr=0000000070000000 size=0x0e4000 name="phdr #0: /home/kraxel/projects/qemu/pc-bios/openbios-sparc32" ppc has two entries from the two elf phdr sections: (qemu) info roms addr=00000000fff00000 size=0x052c88 name="phdr #0: /home/kraxel/projects/qemu/pc-bios/openbios-ppc" addr=00000000fffffffc size=0x000004 name="phdr #1: /home/kraxel/projects/qemu/pc-bios/openbios-ppc" pc has multiple entries when loading option roms (for network booting), looks like this then: (qemu) info roms addr=00000000000c0000 size=0x008c00 name="vgabios-cirrus.bin" addr=00000000000c9000 size=0x008000 name="pxe-e1000.bin" Booting linux kernels (pc too) directly will produce even more entries: (qemu) info roms addr=0000000000010000 size=0x003800 name="linux-setup" addr=0000000000020000 size=0x000001 name="linux-cmdline" addr=00000000000c0000 size=0x008c00 name="vgabios-cirrus.bin" addr=00000000000c9000 size=0x000200 name="linux-bootsect" addr=0000000000100000 size=0x342910 name="/boot/vmlinuz-2.6.30.5-43.fc11.x86_64" > Perhaps 'info roms' could also check whether the rom has been changed? Point being? > On most platforms roms are really read only memories. Yep, I've noticed on a quick glimpse. So in that case writing roms on reset isn't very useful because the guest shouldn't be able to modify them in the first place. It doesn't harm though, except that we keep a copy of the roms around for no reason. Maybe make restore-on-reset opt-in? >> + fd = open(rom->path, O_RDONLY); >> + if (-1 == fd) { > > Not again! Oops. Fixed. >> + rom->align = align; >> + rom->min = min; >> + rom->max = max; >> + rom->romsize = lseek(fd, 0, SEEK_END); >> + rom->data = qemu_mallocz(rom->romsize); > > There are plenty of extra spaces. The vertical alignment makes the code more readable IMHO. cheers, Gerd