From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57430) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bVblZ-0002np-9M for qemu-devel@nongnu.org; Fri, 05 Aug 2016 05:50:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bVblS-0000Jr-TH for qemu-devel@nongnu.org; Fri, 05 Aug 2016 05:50:08 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52786) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bVblS-0000JO-L8 for qemu-devel@nongnu.org; Fri, 05 Aug 2016 05:50:02 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8DDCB6CB29 for ; Fri, 5 Aug 2016 09:50:01 +0000 (UTC) Date: Fri, 5 Aug 2016 10:49:59 +0100 From: "Richard W.M. Jones" Message-ID: <20160805094959.GZ1688@redhat.com> References: <1470389697-3537-1-git-send-email-pbonzini@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1470389697-3537-1-git-send-email-pbonzini@redhat.com> Subject: Re: [Qemu-devel] [PATCH for 2.7] linuxboot_dma: avoid guest ABI breakage on gcc vs. clang compilation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org On Fri, Aug 05, 2016 at 11:34:57AM +0200, Paolo Bonzini wrote: > GCC compiles linuxboot_dma.c to 921 bytes, while clang needs 1527. > This causes the API to break between a GCC-compiled ROM and > one that was obtained with clang. I don't understand this justification. Which API? Between which bits? Can you expand on this bit ... Rich. > First, this patch fixes this by preventing clang's happy inlining (which > -Os cannot prevent). This only requires adding a noinline attribute. > > Second, it makes sure that an unexpected guest ABI breakage cannot happen > in the future. The size must now hardcoded in the file that is passed to > signrom.py, as was the case before commit 6f71b77 ("scripts/signrom.py: > Allow option ROM checksum script to write the size header.", 2016-05-23); > signrom.py however will still pad the input to the requested size, to > avoid the need for -fno-toplevel-reorder which clang doesn't support. > signrom.py can then error out if the requested size is too small for > the actual size of the compiled ROM. > > Signed-off-by: Paolo Bonzini > --- > pc-bios/optionrom/linuxboot_dma.c | 8 ++++++-- > scripts/signrom.py | 27 +++++++++++---------------- > 2 files changed, 17 insertions(+), 18 deletions(-) > > diff --git a/pc-bios/optionrom/linuxboot_dma.c b/pc-bios/optionrom/linuxboot_dma.c > index 8509b28..8584a49 100644 > --- a/pc-bios/optionrom/linuxboot_dma.c > +++ b/pc-bios/optionrom/linuxboot_dma.c > @@ -25,7 +25,7 @@ asm( > ".global _start\n" > "_start:\n" > " .short 0xaa55\n" > -" .byte 0\n" /* size in 512 units, filled in by signrom.py */ > +" .byte 2\n" /* desired size in 512 units; signrom.py adds padding */ > " .byte 0xcb\n" /* far return without prefix */ > " .org 0x18\n" > " .short 0\n" > @@ -157,7 +157,11 @@ static inline uint32_t be32_to_cpu(uint32_t x) > return bswap32(x); > } > > -static void bios_cfg_read_entry(void *buf, uint16_t entry, uint32_t len) > +/* clang is happy to inline this function, and bloats the > + * ROM. > + */ > +static __attribute__((__noinline__)) > +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 > diff --git a/scripts/signrom.py b/scripts/signrom.py > index 5629bca..d1dabe0 100644 > --- a/scripts/signrom.py > +++ b/scripts/signrom.py > @@ -23,26 +23,21 @@ if magic != '\x55\xaa': > > size_byte = ord(fin.read(1)) > fin.seek(0) > +data = fin.read() > > -if size_byte == 0: > - # If the caller left the size field blank then we will fill it in, > - # also rounding the whole input to a multiple of 512 bytes. > - data = fin.read() > - # +1 because we need a byte to store the checksum. > - size = len(data) + 1 > - # Round up to next multiple of 512. > - size += 511 > - size -= size % 512 > - if size >= 65536: > - sys.exit("%s: option ROM size too large" % sys.argv[1]) > +size = size_byte * 512 > +if len(data) > size: > + sys.stderr.write('error: ROM is too large (%d > %d)\n' % (len(data), size)) > + sys.exit(1) > +elif len(data) < size: > + # Add padding if necessary, rounding the whole input to a multiple of > + # 512 bytes according to the third byte of the input. > # size-1 because a final byte is added below to store the checksum. > data = data.ljust(size-1, '\0') > - data = data[:2] + chr(size/512) + data[3:] > else: > - # Otherwise the input file specifies the size so use it. > - # -1 because we overwrite the last byte of the file with the checksum. > - size = size_byte * 512 - 1 > - data = fin.read(size) > + if ord(data[-1:]) != 0: > + sys.stderr.write('WARNING: ROM includes nonzero checksum\n') > + data = data[:size-1] > > fout.write(data) > > -- > 2.7.4 -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v