Re: [Qemu-devel] [PATCH for-1.7] s390x: fix flat rom load on 32 bit systems

[Alexander Graf]

On 21.11.2013, at 13:08, Michael S. Tsirkin <address@hidden> wrote:

> pc-bios/s390-zipl.rom is a flat image so it's expected that
> loading it as elf will fail.
> It should fall back on loading a flat file, but doesn't
> on 32 bit systems, instead it fails printing:
>    qemu: hardware error: could not load bootloader 's390-zipl.rom'
> 
> The result is boot failure.
> 
> The reason is that a 64 bit unsigned interger which is set
> to -1 on error is compared to -1UL which on a 32 bit system
> with gcc is a 32 bit unsigned interger.
> Since both are unsigned, no sign extension takes place and
> comparison evaluates to non-equal.
> 
> There's no reason to do clever tricks: -1 will cause
> sign extension to happen correctly automatically.
> 
> Signed-off-by: Michael S. Tsirkin <address@hidden>
> ---
> hw/s390x/ipl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index d69adb2..88115e9 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -80,7 +80,7 @@ static int s390_ipl_init(SysBusDevice *dev)
> 
>         bios_size = load_elf(bios_filename, NULL, NULL, &ipl->start_addr, 
> NULL,
>                              NULL, 1, ELF_MACHINE, 0);
> -        if (bios_size == -1UL) {
> +        if (bios_size == -1) {

I still dislike that we have 2 completely separate checks for the same thing. 
One here, one a few lines below checking for (long)bios_size < 0. I would very 
much like to see them at least fail identically :). I also don't think that 
comparing to == -1 is really more safe than comparing to -1UL (what is -1UL 
anyway? Negatives aren't unsigned, are they?).

But as a quick fix for 1.7 it's good enough IMHO.

Acked-by: Alexander Graf <address@hidden>


Alex