From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH] fix extboot from boot with cache=off Date: Thu, 19 Mar 2009 12:03:12 +0200 Message-ID: <49C21860.50302@redhat.com> References: <1237258333.15350.62.camel@voxel> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org To: Nolan Return-path: Received: from mx2.redhat.com ([66.187.237.31]:34521 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752766AbZCSKDR (ORCPT ); Thu, 19 Mar 2009 06:03:17 -0400 In-Reply-To: <1237258333.15350.62.camel@voxel> Sender: kvm-owner@vger.kernel.org List-ID: Nolan wrote: > Extboot submits requests with whatever buffer alignment the guest gave > to the BIOS. This breaks with O_DIRECT disks, as they require 512 byte > alignment. > > Most guest bootloaders sector align their requests out of paranoia, but > the OpenBSD bootloader does not. > > This patch always copies. Since extboot is only used at boot time to > load limited amounts of data, the overhead is not problematic. I also > switched to using cpu_physical_memory_* instead of groveling around with > phys_ram_base directly. > > Signed-off-by: Nolan Leake sigbus.net> > > --- > > qemu/hw/extboot.c | 35 ++++++++++++++++++++++++----------- > 1 files changed, 24 insertions(+), 11 deletions(-) > > diff --git a/qemu/hw/extboot.c b/qemu/hw/extboot.c > index ada0fdd..ea16b3e 100644 > --- a/qemu/hw/extboot.c > +++ b/qemu/hw/extboot.c > @@ -77,19 +77,29 @@ static void extboot_write_cmd(void *opaque, uint32_t addr, uint32_t value) > BlockDriverState *bs = opaque; > int cylinders, heads, sectors, err; > uint64_t nb_sectors; > - > - get_translated_chs(bs, &cylinders, &heads, §ors); > + target_phys_addr_t pa; > + int blen; > + void *buf = NULL; > > if (cmd->type == 0x01 || cmd->type == 0x02) { > - target_ulong pa = cmd->xfer.segment * 16 + cmd->xfer.segment; > + pa = cmd->xfer.segment * 16 + cmd->xfer.offset; > + blen = cmd->xfer.nb_sectors * 512; > + buf = qemu_memalign(512, blen); > + if (!buf) { > + printf("qemu_memalign failed\n"); > + return; > + } > Don't check for allocation failures. qemu_malloc() terminates gracefully on failure, qemu_memalign() does not, but should. > > /* possible buffer overflow */ > - if ((pa + cmd->xfer.nb_sectors * 512) > phys_ram_size) > + if ((pa + blen) > phys_ram_size) { > + qemu_free(buf); > return; > + } > cpu_physical_memory_rw() will check these for you. All in all, a nice improvement in addition to the bug fix. -- error compiling committee.c: too many arguments to function