Re: [Qemu-devel] [PATCH for 2.7] linuxboot_dma: avoid guest ABI breakage on gcc vs. clang compilation

[Fam Zheng <famz@redhat.com>]

On Fri, 08/05 11:34, 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.
> 
> 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 <pbonzini@redhat.com>
> ---
>  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
> 
> 

This size check fails for me on centos6 docker image:

  Signing optionrom/linuxboot_dma.bin
  error: ROM is too large (1029 > 1024)
  make[1]: *** [linuxboot_dma.bin] Error 1
  make[1]: *** Deleting file `linuxboot_dma.bin'
  make: *** [romsubdir-optionrom] Error 2
  make: *** Waiting for unfinished jobs....
  tests/docker/Makefile.include:104: recipe for target 'docker-run-test-quick@centos6' failed

Fam