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

[Paolo Bonzini <pbonzini@redhat.com>]

> I sort of question whether anyone would do this (mix different
> compilers and expect migration to work).  And if we do allow it, why
> don't we copy the ROMs across with the rest of the memory?

We do, but the sizes need to match.

Paolo

> It seems
> to me as if migration would still break if you migrated at the exact
> moment that the guest was booting and running the ROM.
> 
> But yes, I guess the patch is OK -- so ACK -- but would be nice to
> change the commit message to note that it's the migration ABI which is
> of concern.
> 
> Rich.
> 
> > Paolo
> > 
> > > 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 <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
> > > 
> > > --
> > > 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
> > > 
> 
> --
> Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
> Read my programming and virtualization blog: http://rwmj.wordpress.com
> virt-top is 'top' for virtual machines.  Tiny program with many
> powerful monitoring features, net stats, disk stats, logging, etc.
> http://people.redhat.com/~rjones/virt-top
>