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

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

[Michael S. Tsirkin]

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 <mst@redhat.com>
---
 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) {
             bios_size = load_image_targphys(bios_filename, ZIPL_IMAGE_START,
                                             4096);
             ipl->start_addr = ZIPL_IMAGE_START;
-- 
MST

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 <mst@redhat.com> 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 <mst@redhat.com>
> ---
> 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 <agraf@suse.de>


Alex

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

[Cornelia Huck]

On Thu, 21 Nov 2013 14:08:22 +0200
"Michael S. Tsirkin" <mst@redhat.com> 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 <mst@redhat.com>
> ---
>  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) {
>              bios_size = load_image_targphys(bios_filename, ZIPL_IMAGE_START,
>                                              4096);
>              ipl->start_addr = ZIPL_IMAGE_START;

Makes sense, but doesn't the kernel loader just below suffer from just
the same problem?

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

[Alexander Graf]

On 21.11.2013, at 13:24, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Thu, 21 Nov 2013 14:08:22 +0200
> "Michael S. Tsirkin" <mst@redhat.com> 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 <mst@redhat.com>
>> ---
>> 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) {
>>             bios_size = load_image_targphys(bios_filename, ZIPL_IMAGE_START,
>>                                             4096);
>>             ipl->start_addr = ZIPL_IMAGE_START;
> 
> Makes sense, but doesn't the kernel loader just below suffer from just
> the same problem?

Yes, initrd too.


Alex

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

[Michael S. Tsirkin]

On Thu, Nov 21, 2013 at 01:24:13PM +0100, Cornelia Huck wrote:
> On Thu, 21 Nov 2013 14:08:22 +0200
> "Michael S. Tsirkin" <mst@redhat.com> 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 <mst@redhat.com>
> > ---
> >  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) {
> >              bios_size = load_image_targphys(bios_filename, ZIPL_IMAGE_START,
> >                                              4096);
> >              ipl->start_addr = ZIPL_IMAGE_START;
> 
> Makes sense, but doesn't the kernel loader just below suffer from just
> the same problem?

Yes, v2 fixes this.