| Message ID | 1470389697-3537-1-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <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
> 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 ... It's ABI, not API (the subject gets it right). ROMs must be the same size on both sides of migration. Here it would mismatch is you migrate from a GCC-compiled QEMU to a clang-compiled QEMU or vice versa. Migration would fail, but it's nicer to fix it. 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 >
On Fri, Aug 05, 2016 at 08:02:25AM -0400, Paolo Bonzini wrote: > > > 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 ... > > It's ABI, not API (the subject gets it right). > > ROMs must be the same size on both sides of migration. Here it would > mismatch is you migrate from a GCC-compiled QEMU to a clang-compiled > QEMU or vice versa. Migration would fail, but it's nicer to fix it. OK, understood now thanks. 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? 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 > >
On 5 August 2016 at 13:22, Richard W.M. Jones <rjones@redhat.com> wrote: > I sort of question whether anyone would do this (mix different > compilers and expect migration to work). If we support cross-QEMU-version migration I would certainly expect to support migration where the same version of QEMU just happened to be built with a different compiler. Consider the hypothetical future where Fedora switches its system compiler from gcc to clang :-) Migration should still work between old-Fedora and new-Fedora. thanks -- PMM
> 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 >
On 05/08/2016 14:30, Peter Maydell wrote: > On 5 August 2016 at 13:22, Richard W.M. Jones <rjones@redhat.com> wrote: >> I sort of question whether anyone would do this (mix different >> compilers and expect migration to work). > > If we support cross-QEMU-version migration I would certainly > expect to support migration where the same version of QEMU > just happened to be built with a different compiler. We even support (at least in theory) migrating a TCG guest across host architectures! Paolo > Consider the hypothetical future where Fedora switches its > system compiler from gcc to clang :-) Migration should > still work between old-Fedora and new-Fedora. > > thanks > -- PMM >
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
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)
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(-)