> 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 <pbonz...@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
>
