s390 qemu boot failure in -next

s390 qemu boot failure in -next

[Guenter Roeck]

Hi,

starting with commit 's390/boot: make head.S and als.c be part of the
decompressor only' in -next, s390 immages no longer boot in qemu.
As far as I can see, the reason is that the command line is no longer
passed from qemu to the kernel, which results in a panic because the
root file system can not be mounted.

Was this change made on purpose ? If so, is there a way to get qemu
back to working ?

Thanks,
Guenter

---
bisect log:

# bad: [2e6381681ec48ed6dd455473a657d8a393518aa6] Add linux-next specific files for 20180622
# good: [ce397d215ccd07b8ae3f71db689aedb85d56ab40] Linux 4.18-rc1
git bisect start 'HEAD' 'v4.18-rc1'
# bad: [07b762f7baf30aaa93d35f36ff367d4b36632059] Merge remote-tracking branch 'spi-nor/spi-nor/next'
git bisect bad 07b762f7baf30aaa93d35f36ff367d4b36632059
# good: [6d92c59898b58a2ee80e7aa15cc72c035d33ba20] Merge remote-tracking branch 'realtek/for-next'
git bisect good 6d92c59898b58a2ee80e7aa15cc72c035d33ba20
# bad: [0e743bf802047ea83ce2345bbe9df52e96bf6d1a] Merge remote-tracking branch 'ext4/dev'
git bisect bad 0e743bf802047ea83ce2345bbe9df52e96bf6d1a
# good: [8039f3664e6b07b6797579e29bcc1c51c022e07b] Merge remote-tracking branch 'microblaze/next'
git bisect good 8039f3664e6b07b6797579e29bcc1c51c022e07b
# bad: [c6c285ec2bb43d90176dda4a0cd63c48df7dee98] Merge remote-tracking branch 's390/features'
git bisect bad c6c285ec2bb43d90176dda4a0cd63c48df7dee98
# good: [0e56f3749f0b28166a29691a5422d62a516877ae] MIPS: loongson64: use generic dma noncoherent ops
git bisect good 0e56f3749f0b28166a29691a5422d62a516877ae
# bad: [32bf311a88e9fa70eb3604a697c2390f02ef2de2] init/Kconfig: add an option for uncompressed kernel
git bisect bad 32bf311a88e9fa70eb3604a697c2390f02ef2de2
# bad: [34139a786a40f9f97f84e7f422a9c29d59e2606d] s390/decompressor: rename entry point to startup_decompressor
git bisect bad 34139a786a40f9f97f84e7f422a9c29d59e2606d
# good: [d035a77cb9256f52dc6c4940cbdb2051adb5420c] s390/decompressor: correct build flags
git bisect good d035a77cb9256f52dc6c4940cbdb2051adb5420c
# good: [652e75c51463aead06389dd6f5756f9b91f7fd75] s390/setup: do not reserve the decompressor code
git bisect good 652e75c51463aead06389dd6f5756f9b91f7fd75
# bad: [1b78ef09741223c630a37311ae12d6f6bd212e1a] s390/boot: make head.S and als.c be part of the decompressor only
git bisect bad 1b78ef09741223c630a37311ae12d6f6bd212e1a
# good: [aa71c78a63a3ac82e47714f48b3ea14e2ef94dfc] s390/decompressor: trim the kernel image up to 1M
git bisect good aa71c78a63a3ac82e47714f48b3ea14e2ef94dfc
# first bad commit: [1b78ef09741223c630a37311ae12d6f6bd212e1a] s390/boot: make head.S and als.c be part of the decompressor only

Re: s390 qemu boot failure in -next

[Christian Borntraeger]

On 06/22/2018 09:47 PM, Guenter Roeck wrote:
> Hi,
> 
> starting with commit 's390/boot: make head.S and als.c be part of the
> decompressor only' in -next, s390 immages no longer boot in qemu.
> As far as I can see, the reason is that the command line is no longer
> passed from qemu to the kernel, which results in a panic because the
> root file system can not be mounted.
> 
> Was this change made on purpose ? If so, is there a way to get qemu
> back to working ?

Certainly not on purpose.

Vasily, I can reproduce this with KVM and an external kernel boot of the vmlinux file (the elf file)

e.g.

qemu-system-s390 -enable-kvm -nographic -kernel vmlinux -append "this string no longer is command line"

The compressed image (bzImage) seems to work fine though.

This seems to be an unfortunate side effect of QEMUs ways to "guess" its Linux (checking for start
address 0x10000, which is no longer true for the vmlinux file). With the pure vmlinux elf file
the load address is 0x100000 as there is no unpacker.

Guenter, can you check if arch/s390/boot/bzImage works for you as a workaround?

Re: s390 qemu boot failure in -next

[Christian Borntraeger]

Also adding QEMU.

On 06/25/2018 09:10 AM, Christian Borntraeger wrote:
> 
> 
> On 06/22/2018 09:47 PM, Guenter Roeck wrote:
>> Hi,
>>
>> starting with commit 's390/boot: make head.S and als.c be part of the
>> decompressor only' in -next, s390 immages no longer boot in qemu.
>> As far as I can see, the reason is that the command line is no longer
>> passed from qemu to the kernel, which results in a panic because the
>> root file system can not be mounted.
>>
>> Was this change made on purpose ? If so, is there a way to get qemu
>> back to working ?
> 
> Certainly not on purpose.
> 
> Vasily, I can reproduce this with KVM and an external kernel boot of the vmlinux file (the elf file)
> 
> e.g.
> 
> qemu-system-s390 -enable-kvm -nographic -kernel vmlinux -append "this string no longer is command line"
> 
> The compressed image (bzImage) seems to work fine though.
> 
> This seems to be an unfortunate side effect of QEMUs ways to "guess" its Linux (checking for start
> address 0x10000, which is no longer true for the vmlinux file). With the pure vmlinux elf file
> the load address is 0x100000 as there is no unpacker.
> 
> Guenter, can you check if arch/s390/boot/bzImage works for you as a workaround?

Something like this in QEMU 

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index f278036fa7..14153ce880 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -187,11 +187,13 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
          */
         if (pentry == KERN_IMAGE_START || pentry == 0x800) {
             ipl->start_addr = KERN_IMAGE_START;
-            /* Overwrite parameters in the kernel image, which are "rom" */
-            strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline);
         } else {
             ipl->start_addr = pentry;
         }
+       if (ipl->cmdline) {
+            /* If there is a command line, put it in the right place */
+            strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline);
+       }
 
         if (ipl->initrd) {
             ram_addr_t initrd_offset;

would put the command line in no matter what the start address is.

Re: [qemu-s390x] s390 qemu boot failure in -next

[Christian Borntraeger]

On 06/25/2018 09:27 AM, Christian Borntraeger wrote:
> Also adding QEMU.
> 
> On 06/25/2018 09:10 AM, Christian Borntraeger wrote:
>>
>>
>> On 06/22/2018 09:47 PM, Guenter Roeck wrote:
>>> Hi,
>>>
>>> starting with commit 's390/boot: make head.S and als.c be part of the
>>> decompressor only' in -next, s390 immages no longer boot in qemu.
>>> As far as I can see, the reason is that the command line is no longer
>>> passed from qemu to the kernel, which results in a panic because the
>>> root file system can not be mounted.
>>>
>>> Was this change made on purpose ? If so, is there a way to get qemu
>>> back to working ?
>>
>> Certainly not on purpose.
>>
>> Vasily, I can reproduce this with KVM and an external kernel boot of the vmlinux file (the elf file)
>>
>> e.g.
>>
>> qemu-system-s390 -enable-kvm -nographic -kernel vmlinux -append "this string no longer is command line"
>>
>> The compressed image (bzImage) seems to work fine though.
>>
>> This seems to be an unfortunate side effect of QEMUs ways to "guess" its Linux (checking for start
>> address 0x10000, which is no longer true for the vmlinux file). With the pure vmlinux elf file
>> the load address is 0x100000 as there is no unpacker.
>>
>> Guenter, can you check if arch/s390/boot/bzImage works for you as a workaround?
> 
> Something like this in QEMU 
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index f278036fa7..14153ce880 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -187,11 +187,13 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
>           */
>          if (pentry == KERN_IMAGE_START || pentry == 0x800) {
>              ipl->start_addr = KERN_IMAGE_START;
> -            /* Overwrite parameters in the kernel image, which are "rom" */
> -            strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline);
>          } else {
>              ipl->start_addr = pentry;
>          }
> +       if (ipl->cmdline) {
> +            /* If there is a command line, put it in the right place */
> +            strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline);
> +       }
>  
>          if (ipl->initrd) {
>              ram_addr_t initrd_offset;
> 
> would put the command line in no matter what the start address is.

Ideally we would do 2 changes:
- change QEMU to add a commandline to 10480 if specified
- have a way to fix the kernel elf file to still boot with older QEMUs 

Re: s390 qemu boot failure in -next

[Cornelia Huck]

On Mon, 25 Jun 2018 09:27:59 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> Also adding QEMU.
> 
> On 06/25/2018 09:10 AM, Christian Borntraeger wrote:
> > 
> > 
> > On 06/22/2018 09:47 PM, Guenter Roeck wrote:  
> >> Hi,
> >>
> >> starting with commit 's390/boot: make head.S and als.c be part of the
> >> decompressor only' in -next, s390 immages no longer boot in qemu.
> >> As far as I can see, the reason is that the command line is no longer
> >> passed from qemu to the kernel, which results in a panic because the
> >> root file system can not be mounted.
> >>
> >> Was this change made on purpose ? If so, is there a way to get qemu
> >> back to working ?  
> > 
> > Certainly not on purpose.
> > 
> > Vasily, I can reproduce this with KVM and an external kernel boot of the vmlinux file (the elf file)
> > 
> > e.g.
> > 
> > qemu-system-s390 -enable-kvm -nographic -kernel vmlinux -append "this string no longer is command line"
> > 
> > The compressed image (bzImage) seems to work fine though.
> > 
> > This seems to be an unfortunate side effect of QEMUs ways to "guess" its Linux (checking for start
> > address 0x10000, which is no longer true for the vmlinux file). With the pure vmlinux elf file
> > the load address is 0x100000 as there is no unpacker.

Do we consider these locations to be an exported interface, or is it
just QEMU guessing?

> > 
> > Guenter, can you check if arch/s390/boot/bzImage works for you as a workaround?  
> 
> Something like this in QEMU 
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index f278036fa7..14153ce880 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -187,11 +187,13 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
>           */
>          if (pentry == KERN_IMAGE_START || pentry == 0x800) {
>              ipl->start_addr = KERN_IMAGE_START;
> -            /* Overwrite parameters in the kernel image, which are "rom" */
> -            strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline);
>          } else {
>              ipl->start_addr = pentry;
>          }
> +       if (ipl->cmdline) {
> +            /* If there is a command line, put it in the right place */
> +            strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline);
> +       }

Check for the magic Linux string (like in the non-elf case) first?

>  
>          if (ipl->initrd) {
>              ram_addr_t initrd_offset;
> 
> would put the command line in no matter what the start address is.

I'm for putting that one in (and backporting it to qemu-stable). It's a
bit worrying, though, that our ipl code is so fragile...

Re: [qemu-s390x] s390 qemu boot failure in -next

[Cornelia Huck]

On Mon, 25 Jun 2018 10:02:28 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 06/25/2018 09:27 AM, Christian Borntraeger wrote:
> > Also adding QEMU.
> > 
> > On 06/25/2018 09:10 AM, Christian Borntraeger wrote:  
> >>
> >>
> >> On 06/22/2018 09:47 PM, Guenter Roeck wrote:  
> >>> Hi,
> >>>
> >>> starting with commit 's390/boot: make head.S and als.c be part of the
> >>> decompressor only' in -next, s390 immages no longer boot in qemu.
> >>> As far as I can see, the reason is that the command line is no longer
> >>> passed from qemu to the kernel, which results in a panic because the
> >>> root file system can not be mounted.
> >>>
> >>> Was this change made on purpose ? If so, is there a way to get qemu
> >>> back to working ?  
> >>
> >> Certainly not on purpose.
> >>
> >> Vasily, I can reproduce this with KVM and an external kernel boot of the vmlinux file (the elf file)
> >>
> >> e.g.
> >>
> >> qemu-system-s390 -enable-kvm -nographic -kernel vmlinux -append "this string no longer is command line"
> >>
> >> The compressed image (bzImage) seems to work fine though.
> >>
> >> This seems to be an unfortunate side effect of QEMUs ways to "guess" its Linux (checking for start
> >> address 0x10000, which is no longer true for the vmlinux file). With the pure vmlinux elf file
> >> the load address is 0x100000 as there is no unpacker.
> >>
> >> Guenter, can you check if arch/s390/boot/bzImage works for you as a workaround?  
> > 
> > Something like this in QEMU 
> > 
> > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> > index f278036fa7..14153ce880 100644
> > --- a/hw/s390x/ipl.c
> > +++ b/hw/s390x/ipl.c
> > @@ -187,11 +187,13 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
> >           */
> >          if (pentry == KERN_IMAGE_START || pentry == 0x800) {
> >              ipl->start_addr = KERN_IMAGE_START;
> > -            /* Overwrite parameters in the kernel image, which are "rom" */
> > -            strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline);
> >          } else {
> >              ipl->start_addr = pentry;
> >          }
> > +       if (ipl->cmdline) {
> > +            /* If there is a command line, put it in the right place */
> > +            strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline);
> > +       }
> >  
> >          if (ipl->initrd) {
> >              ram_addr_t initrd_offset;
> > 
> > would put the command line in no matter what the start address is.  
> 
> Ideally we would do 2 changes:
> - change QEMU to add a commandline to 10480 if specified
> - have a way to fix the kernel elf file to still boot with older QEMUs 
> 

Agreed on both. Even if we change QEMU now (+ stable), there are many
versions out there that will now fail.

Re: [qemu-s390x] s390 qemu boot failure in -next

[Thomas Huth]

On 25.06.2018 10:02, Christian Borntraeger wrote:
> 
> 
> On 06/25/2018 09:27 AM, Christian Borntraeger wrote:
>> Also adding QEMU.
>>
>> On 06/25/2018 09:10 AM, Christian Borntraeger wrote:
>>>
>>>
>>> On 06/22/2018 09:47 PM, Guenter Roeck wrote:
>>>> Hi,
>>>>
>>>> starting with commit 's390/boot: make head.S and als.c be part of the
>>>> decompressor only' in -next, s390 immages no longer boot in qemu.
>>>> As far as I can see, the reason is that the command line is no longer
>>>> passed from qemu to the kernel, which results in a panic because the
>>>> root file system can not be mounted.
>>>>
>>>> Was this change made on purpose ? If so, is there a way to get qemu
>>>> back to working ?
>>>
>>> Certainly not on purpose.
>>>
>>> Vasily, I can reproduce this with KVM and an external kernel boot of the vmlinux file (the elf file)
>>>
>>> e.g.
>>>
>>> qemu-system-s390 -enable-kvm -nographic -kernel vmlinux -append "this string no longer is command line"
>>>
>>> The compressed image (bzImage) seems to work fine though.
>>>
>>> This seems to be an unfortunate side effect of QEMUs ways to "guess" its Linux (checking for start
>>> address 0x10000, which is no longer true for the vmlinux file). With the pure vmlinux elf file
>>> the load address is 0x100000 as there is no unpacker.
>>>
>>> Guenter, can you check if arch/s390/boot/bzImage works for you as a workaround?
>>
>> Something like this in QEMU 
>>
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index f278036fa7..14153ce880 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -187,11 +187,13 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
>>           */
>>          if (pentry == KERN_IMAGE_START || pentry == 0x800) {
>>              ipl->start_addr = KERN_IMAGE_START;
>> -            /* Overwrite parameters in the kernel image, which are "rom" */
>> -            strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline);
>>          } else {
>>              ipl->start_addr = pentry;
>>          }
>> +       if (ipl->cmdline) {
>> +            /* If there is a command line, put it in the right place */
>> +            strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline);

You definitely need to check for rom_ptr() != NULL first before calling
strcpy. Otherwise QEMU segfaults in case the user tried to load a kernel
to a different location.

See also:
https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg04227.html

 Thomas

Re: s390 qemu boot failure in -next

[Christian Borntraeger]

On 06/25/2018 10:05 AM, Cornelia Huck wrote:
> On Mon, 25 Jun 2018 09:27:59 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> Also adding QEMU.
>>
>> On 06/25/2018 09:10 AM, Christian Borntraeger wrote:
>>>
>>>
>>> On 06/22/2018 09:47 PM, Guenter Roeck wrote:  
>>>> Hi,
>>>>
>>>> starting with commit 's390/boot: make head.S and als.c be part of the
>>>> decompressor only' in -next, s390 immages no longer boot in qemu.
>>>> As far as I can see, the reason is that the command line is no longer
>>>> passed from qemu to the kernel, which results in a panic because the
>>>> root file system can not be mounted.
>>>>
>>>> Was this change made on purpose ? If so, is there a way to get qemu
>>>> back to working ?  
>>>
>>> Certainly not on purpose.
>>>
>>> Vasily, I can reproduce this with KVM and an external kernel boot of the vmlinux file (the elf file)
>>>
>>> e.g.
>>>
>>> qemu-system-s390 -enable-kvm -nographic -kernel vmlinux -append "this string no longer is command line"
>>>
>>> The compressed image (bzImage) seems to work fine though.
>>>
>>> This seems to be an unfortunate side effect of QEMUs ways to "guess" its Linux (checking for start
>>> address 0x10000, which is no longer true for the vmlinux file). With the pure vmlinux elf file
>>> the load address is 0x100000 as there is no unpacker.
> 
> Do we consider these locations to be an exported interface, or is it
> just QEMU guessing?

It is using  what zipl has hardcoded. This works fine for the final image (with the 
unpacker), but being able to boot the elf file was just a very nice feature of QEMU.

> 
>>>
>>> Guenter, can you check if arch/s390/boot/bzImage works for you as a workaround?  
>>
>> Something like this in QEMU 
>>
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index f278036fa7..14153ce880 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -187,11 +187,13 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
>>           */
>>          if (pentry == KERN_IMAGE_START || pentry == 0x800) {
>>              ipl->start_addr = KERN_IMAGE_START;
>> -            /* Overwrite parameters in the kernel image, which are "rom" */
>> -            strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline);
>>          } else {
>>              ipl->start_addr = pentry;
>>          }
>> +       if (ipl->cmdline) {
>> +            /* If there is a command line, put it in the right place */
>> +            strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline);
>> +       }
> 
> Check for the magic Linux string (like in the non-elf case) first?

Even that does not exists in vmlinux but only in bzImage with the latest patchset
(in next, but not upstream yet)
> 
>>  
>>          if (ipl->initrd) {
>>              ram_addr_t initrd_offset;
>>
>> would put the command line in no matter what the start address is.
> 
> I'm for putting that one in (and backporting it to qemu-stable). It's a
> bit worrying, though, that our ipl code is so fragile...

We actually have to combine this with Thomas fix (to check for rom_ptr returning
something sane). It seems that ipl->commandline is always there, so we have to
check for strlen!=0 it seems..

I mean if somebody ask for "-append something" we can certainly always write something
if there is rom/ram.

Re: s390 qemu boot failure in -next

[Vasily Gorbik]

This change has been done on purpose. Uncompressed image is not going
to be bootable any more. In future the decompressor phase would get
more function (early memory detection as an example) and there is no
chance to duplicate that code in uncompressed image as well (to keep it
bootable on its own). The patch series commit messages contain more
technical details.

For qemu either bzImage or arch/s390/boot/compressed/vmlinux should be
used, which are bootable images.

But that's really confusing that uncompressed vmlinux is still kind
of booting. May be we should discuss how to avoid this confusion
(may be change uncompressed image enty point to a function doing
disabled wait with badb007 or smth) and how to encourage people to use
arch/s390/boot/compressed/vmlinux instead.

On Mon, Jun 25, 2018 at 10:05:48AM +0200, Cornelia Huck wrote:
> On Mon, 25 Jun 2018 09:27:59 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
> > Also adding QEMU.
> > 
> > On 06/25/2018 09:10 AM, Christian Borntraeger wrote:
> > > 
> > > 
> > > On 06/22/2018 09:47 PM, Guenter Roeck wrote:  
> > >> Hi,
> > >>
> > >> starting with commit 's390/boot: make head.S and als.c be part of the
> > >> decompressor only' in -next, s390 immages no longer boot in qemu.
> > >> As far as I can see, the reason is that the command line is no longer
> > >> passed from qemu to the kernel, which results in a panic because the
> > >> root file system can not be mounted.
> > >>
> > >> Was this change made on purpose ? If so, is there a way to get qemu
> > >> back to working ?  
> > > 
> > > Certainly not on purpose.
> > > 
> > > Vasily, I can reproduce this with KVM and an external kernel boot of the vmlinux file (the elf file)
> > > 
> > > e.g.
> > > 
> > > qemu-system-s390 -enable-kvm -nographic -kernel vmlinux -append "this string no longer is command line"
> > > 
> > > The compressed image (bzImage) seems to work fine though.
> > > 
> > > This seems to be an unfortunate side effect of QEMUs ways to "guess" its Linux (checking for start
> > > address 0x10000, which is no longer true for the vmlinux file). With the pure vmlinux elf file
> > > the load address is 0x100000 as there is no unpacker.
> 
> Do we consider these locations to be an exported interface, or is it
> just QEMU guessing?
> 
> > > 
> > > Guenter, can you check if arch/s390/boot/bzImage works for you as a workaround?  
> > 
> > Something like this in QEMU 
> > 
> > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> > index f278036fa7..14153ce880 100644
> > --- a/hw/s390x/ipl.c
> > +++ b/hw/s390x/ipl.c
> > @@ -187,11 +187,13 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
> >           */
> >          if (pentry == KERN_IMAGE_START || pentry == 0x800) {
> >              ipl->start_addr = KERN_IMAGE_START;
> > -            /* Overwrite parameters in the kernel image, which are "rom" */
> > -            strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline);
> >          } else {
> >              ipl->start_addr = pentry;
> >          }
> > +       if (ipl->cmdline) {
> > +            /* If there is a command line, put it in the right place */
> > +            strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline);
> > +       }
> 
> Check for the magic Linux string (like in the non-elf case) first?
> 
> >  
> >          if (ipl->initrd) {
> >              ram_addr_t initrd_offset;
> > 
> > would put the command line in no matter what the start address is.
> 
> I'm for putting that one in (and backporting it to qemu-stable). It's a
> bit worrying, though, that our ipl code is so fragile...
> 

Re: s390 qemu boot failure in -next

[Cornelia Huck]

On Mon, 25 Jun 2018 10:36:33 +0200
Vasily Gorbik <gor@linux.ibm.com> wrote:

> This change has been done on purpose. Uncompressed image is not going
> to be bootable any more. In future the decompressor phase would get
> more function (early memory detection as an example) and there is no
> chance to duplicate that code in uncompressed image as well (to keep it
> bootable on its own). The patch series commit messages contain more
> technical details.
> 
> For qemu either bzImage or arch/s390/boot/compressed/vmlinux should be
> used, which are bootable images.
> 
> But that's really confusing that uncompressed vmlinux is still kind
> of booting. May be we should discuss how to avoid this confusion
> (may be change uncompressed image enty point to a function doing
> disabled wait with badb007 or smth) and how to encourage people to use
> arch/s390/boot/compressed/vmlinux instead.

So, the intention is that you can't boot the uncompressed image
anywhere? (Was it possible before, e.g. when punching the image under
z/VM?) If yes, it would make sense to explicitly fence it. But I'm
worried that it would break previously working setups (did we document
the purpose of the images anywhere?)

Re: s390 qemu boot failure in -next

[Christian Borntraeger]

On 06/25/2018 10:49 AM, Cornelia Huck wrote:
> On Mon, 25 Jun 2018 10:36:33 +0200
> Vasily Gorbik <gor@linux.ibm.com> wrote:
> 
>> This change has been done on purpose. Uncompressed image is not going
>> to be bootable any more. In future the decompressor phase would get
>> more function (early memory detection as an example) and there is no
>> chance to duplicate that code in uncompressed image as well (to keep it
>> bootable on its own). The patch series commit messages contain more
>> technical details.
>>
>> For qemu either bzImage or arch/s390/boot/compressed/vmlinux should be
>> used, which are bootable images.
>>
>> But that's really confusing that uncompressed vmlinux is still kind
>> of booting. May be we should discuss how to avoid this confusion
>> (may be change uncompressed image enty point to a function doing
>> disabled wait with badb007 or smth) and how to encourage people to use
>> arch/s390/boot/compressed/vmlinux instead.
> 
> So, the intention is that you can't boot the uncompressed image
> anywhere? (Was it possible before, e.g. when punching the image under
> z/VM?)

The uncompressed image (the vmlinux file) was never bootable in LPAR or z/VM.
It was just a "nice hack" that QEMU was able to do so. (even qemu on x86 can not
boot the pure vmlinux file as far as I know).

I talked to Vasily and the vmlinux file in the linux source path is just an 
intermediate file. Future changes will happen that will make that ELF file
unsuitable for direct boot anyway (e.g. think about potential ASLR or Kasan
changes).

 If yes, it would make sense to explicitly fence it. But I'm
> worried that it would break previously working setups (did we document
> the purpose of the images anywhere?)
> 


I think by referring to arch/s390/boot/compressed/vmlinux things are probably
good enough. That will still load from 0x10000.

We might still "change" the way that we add the parameters (e.g.
make that not depend on the load address), but looking forward this might
become less important for the "intermediate vmlnux file".

Re: s390 qemu boot failure in -next

[Guenter Roeck]

On 06/25/2018 05:26 AM, Christian Borntraeger wrote:
> 
> 
> On 06/25/2018 10:49 AM, Cornelia Huck wrote:
>> On Mon, 25 Jun 2018 10:36:33 +0200
>> Vasily Gorbik <gor@linux.ibm.com> wrote:
>>
>>> This change has been done on purpose. Uncompressed image is not going
>>> to be bootable any more. In future the decompressor phase would get
>>> more function (early memory detection as an example) and there is no
>>> chance to duplicate that code in uncompressed image as well (to keep it
>>> bootable on its own). The patch series commit messages contain more
>>> technical details.
>>>
>>> For qemu either bzImage or arch/s390/boot/compressed/vmlinux should be
>>> used, which are bootable images.
>>>
>>> But that's really confusing that uncompressed vmlinux is still kind
>>> of booting. May be we should discuss how to avoid this confusion
>>> (may be change uncompressed image enty point to a function doing
>>> disabled wait with badb007 or smth) and how to encourage people to use
>>> arch/s390/boot/compressed/vmlinux instead.
>>
>> So, the intention is that you can't boot the uncompressed image
>> anywhere? (Was it possible before, e.g. when punching the image under
>> z/VM?)
> 
> The uncompressed image (the vmlinux file) was never bootable in LPAR or z/VM.
> It was just a "nice hack" that QEMU was able to do so. (even qemu on x86 can not
> boot the pure vmlinux file as far as I know).
> 

"even" is relative. vmlinux boots on some arm platforms, metag, mips64, nios2,
parisc, ppc/ppc64, and riscv.

If an image is not expected to be bootable, a message such as "This image does
not boot. Please use <correct image>" would be nice. Unfortunately, which image
to boot under qemu is pretty much undocumented, and it is guesswork for each
architecture/platform.

Guenter

> I talked to Vasily and the vmlinux file in the linux source path is just an
> intermediate file. Future changes will happen that will make that ELF file
> unsuitable for direct boot anyway (e.g. think about potential ASLR or Kasan
> changes).
> 
>   If yes, it would make sense to explicitly fence it. But I'm
>> worried that it would break previously working setups (did we document
>> the purpose of the images anywhere?
>>
> 
> 
> I think by referring to arch/s390/boot/compressed/vmlinux things are probably
> good enough. That will still load from 0x10000.
> 
> We might still "change" the way that we add the parameters (e.g.
> make that not depend on the load address), but looking forward this might
> become less important for the "intermediate vmlnux file".
> 
> 

Re: s390 qemu boot failure in -next

[Vasily Gorbik]

On Mon, Jun 25, 2018 at 06:35:30AM -0700, Guenter Roeck wrote:
> On 06/25/2018 05:26 AM, Christian Borntraeger wrote:
> > 
> > 
> > On 06/25/2018 10:49 AM, Cornelia Huck wrote:
> > > On Mon, 25 Jun 2018 10:36:33 +0200
> > > Vasily Gorbik <gor@linux.ibm.com> wrote:
> > > 
> > > > This change has been done on purpose. Uncompressed image is not going
> > > > to be bootable any more. In future the decompressor phase would get
> > > > more function (early memory detection as an example) and there is no
> > > > chance to duplicate that code in uncompressed image as well (to keep it
> > > > bootable on its own). The patch series commit messages contain more
> > > > technical details.
> > > > 
> > > > For qemu either bzImage or arch/s390/boot/compressed/vmlinux should be
> > > > used, which are bootable images.
> > > > 
> > > > But that's really confusing that uncompressed vmlinux is still kind
> > > > of booting. May be we should discuss how to avoid this confusion
> > > > (may be change uncompressed image enty point to a function doing
> > > > disabled wait with badb007 or smth) and how to encourage people to use
> > > > arch/s390/boot/compressed/vmlinux instead.
> > > 
> 
> If an image is not expected to be bootable, a message such as "This image does
> not boot. Please use <correct image>" would be nice. Unfortunately, which image
> to boot under qemu is pretty much undocumented, and it is guesswork for each
> architecture/platform.
> 
> Guenter
> 
> > I talked to Vasily and the vmlinux file in the linux source path is just an
> > intermediate file. Future changes will happen that will make that ELF file
> > unsuitable for direct boot anyway (e.g. think about potential ASLR or Kasan
> > changes).
> > 
> >   If yes, it would make sense to explicitly fence it. But I'm
> > > worried that it would break previously working setups (did we document
> > > the purpose of the images anywhere?
> > > 

To avoid confusion with trying to boot uncompressed vmlinux under qemu
we could detect it and print "nice" message in the kernel.

Please consider the following patch.

Vasily Gorbik (1):
  s390/boot: block uncompressed vmlinux booting attempts

 arch/s390/boot/head.S         |  4 ++--
 arch/s390/include/asm/setup.h |  3 ++-
 arch/s390/kernel/early.c      | 12 ++++++++++++
 3 files changed, 16 insertions(+), 3 deletions(-)

-- 
⣔⢻⣟⢢ 2.18.0.rc2.13.g4da9a5d
⣿⢿⡿⣿ pacman edition

[PATCH] s390/boot: block uncompressed vmlinux booting attempts

[Vasily Gorbik]

Since uncompressed kernel image "vmlinux" elf file is not bootable under
qemu anymore, add a check which would report that.

Qemu users are encouraged to use bzImage or
arch/s390/boot/compressed/vmlinux instead.

The check relies on s390 linux entry point ABI definition, which is only
present in bzImage and arch/s390/boot/compressed/vmlinux.

Signed-off-by: Vasily Gorbik <gor@linux.ibm.com>
---
 arch/s390/boot/head.S         |  4 ++--
 arch/s390/include/asm/setup.h |  3 ++-
 arch/s390/kernel/early.c      | 12 ++++++++++++
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/arch/s390/boot/head.S b/arch/s390/boot/head.S
index f09e792df495..f721913b73f1 100644
--- a/arch/s390/boot/head.S
+++ b/arch/s390/boot/head.S
@@ -272,14 +272,14 @@ iplstart:
 	.org	0x10000
 ENTRY(startup)
 	j	.Lep_startup_normal
-	.org	0x10008
+	.org	EP_OFFSET
 #
 # This is a list of s390 kernel entry points. At address 0x1000f the number of
 # valid entry points is stored.
 #
 # IMPORTANT: Do not change this table, it is s390 kernel ABI!
 #
-	.ascii	"S390EP"
+	.ascii	EP_STRING
 	.byte	0x00,0x01
 #
 # kdump startup-code at 0x10010, running in 64 bit absolute addressing mode
diff --git a/arch/s390/include/asm/setup.h b/arch/s390/include/asm/setup.h
index be02f0558048..1d66016f4170 100644
--- a/arch/s390/include/asm/setup.h
+++ b/arch/s390/include/asm/setup.h
@@ -9,7 +9,8 @@
 #include <linux/const.h>
 #include <uapi/asm/setup.h>
 
-
+#define EP_OFFSET		0x10008
+#define EP_STRING		"S390EP"
 #define PARMAREA		0x10400
 #define PARMAREA_END		0x11000
 
diff --git a/arch/s390/kernel/early.c b/arch/s390/kernel/early.c
index 827699eb48fa..45c5be3d8777 100644
--- a/arch/s390/kernel/early.c
+++ b/arch/s390/kernel/early.c
@@ -331,8 +331,20 @@ static void __init setup_boot_command_line(void)
 	append_to_cmdline(append_ipl_scpdata);
 }
 
+static void __init check_image_bootable(void)
+{
+	if (!memcmp(EP_STRING, (void *)EP_OFFSET, strlen(EP_STRING)))
+		return;
+
+	sclp_early_printk("The linux kernel boot failure: the image is corrupted or not bootable.\n");
+	sclp_early_printk("Please check that you are using bootable kernel image \"bzImage\".\n");
+	sclp_early_printk("(or alternatively \"arch/s390/boot/compressed/vmlinux\" image for qemu)\n");
+	disabled_wait(0xbadb007);
+}
+
 void __init startup_init(void)
 {
+	check_image_bootable();
 	time_early_init();
 	init_kernel_storage_key();
 	lockdep_off();
-- 
⣔⢻⣟⢢ 2.18.0.rc2.13.g4da9a5d
⣿⢿⡿⣿ pacman edition

Re: [PATCH] s390/boot: block uncompressed vmlinux booting attempts

[Guenter Roeck]

On Mon, Jun 25, 2018 at 05:09:19PM +0200, Vasily Gorbik wrote:
> Since uncompressed kernel image "vmlinux" elf file is not bootable under
> qemu anymore, add a check which would report that.
> 
> Qemu users are encouraged to use bzImage or
> arch/s390/boot/compressed/vmlinux instead.
> 
> The check relies on s390 linux entry point ABI definition, which is only
> present in bzImage and arch/s390/boot/compressed/vmlinux.
> 
> Signed-off-by: Vasily Gorbik <gor@linux.ibm.com>

With qemu:

Tested-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  arch/s390/boot/head.S         |  4 ++--
>  arch/s390/include/asm/setup.h |  3 ++-
>  arch/s390/kernel/early.c      | 12 ++++++++++++
>  3 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/s390/boot/head.S b/arch/s390/boot/head.S
> index f09e792df495..f721913b73f1 100644
> --- a/arch/s390/boot/head.S
> +++ b/arch/s390/boot/head.S
> @@ -272,14 +272,14 @@ iplstart:
>  	.org	0x10000
>  ENTRY(startup)
>  	j	.Lep_startup_normal
> -	.org	0x10008
> +	.org	EP_OFFSET
>  #
>  # This is a list of s390 kernel entry points. At address 0x1000f the number of
>  # valid entry points is stored.
>  #
>  # IMPORTANT: Do not change this table, it is s390 kernel ABI!
>  #
> -	.ascii	"S390EP"
> +	.ascii	EP_STRING
>  	.byte	0x00,0x01
>  #
>  # kdump startup-code at 0x10010, running in 64 bit absolute addressing mode
> diff --git a/arch/s390/include/asm/setup.h b/arch/s390/include/asm/setup.h
> index be02f0558048..1d66016f4170 100644
> --- a/arch/s390/include/asm/setup.h
> +++ b/arch/s390/include/asm/setup.h
> @@ -9,7 +9,8 @@
>  #include <linux/const.h>
>  #include <uapi/asm/setup.h>
>  
> -
> +#define EP_OFFSET		0x10008
> +#define EP_STRING		"S390EP"
>  #define PARMAREA		0x10400
>  #define PARMAREA_END		0x11000
>  
> diff --git a/arch/s390/kernel/early.c b/arch/s390/kernel/early.c
> index 827699eb48fa..45c5be3d8777 100644
> --- a/arch/s390/kernel/early.c
> +++ b/arch/s390/kernel/early.c
> @@ -331,8 +331,20 @@ static void __init setup_boot_command_line(void)
>  	append_to_cmdline(append_ipl_scpdata);
>  }
>  
> +static void __init check_image_bootable(void)
> +{
> +	if (!memcmp(EP_STRING, (void *)EP_OFFSET, strlen(EP_STRING)))
> +		return;
> +
> +	sclp_early_printk("The linux kernel boot failure: the image is corrupted or not bootable.\n");
> +	sclp_early_printk("Please check that you are using bootable kernel image \"bzImage\".\n");
> +	sclp_early_printk("(or alternatively \"arch/s390/boot/compressed/vmlinux\" image for qemu)\n");
> +	disabled_wait(0xbadb007);
> +}
> +
>  void __init startup_init(void)
>  {
> +	check_image_bootable();
>  	time_early_init();
>  	init_kernel_storage_key();
>  	lockdep_off();
> -- 
> ⣔⢻⣟⢢ 2.18.0.rc2.13.g4da9a5d
> ⣿⢿⡿⣿ pacman edition
> 

Re: s390 qemu boot failure in -next

[Georgi Guninski]

On Mon, Jun 25, 2018 at 09:27:59AM +0200, Christian Borntraeger wrote:
> -            /* Overwrite parameters in the kernel image, which are "rom" */
> -            strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline);

> +            strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline);

Why not replace strcpy() with strncpy() or snprintf()?
strcpy() may overflow.

Re: s390 qemu boot failure in -next

[Thomas Huth]

On 26.06.2018 07:32, Georgi Guninski wrote:
> On Mon, Jun 25, 2018 at 09:27:59AM +0200, Christian Borntraeger wrote:
>> -            /* Overwrite parameters in the kernel image, which are "rom" */
>> -            strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline);
> 
>> +            strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline);
> 
> Why not replace strcpy() with strncpy() or snprintf()?
> strcpy() may overflow.

This will be fixed by
https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg04227.html by
adding a check for a valid size.

 Thomas

Re: [PATCH] s390/boot: block uncompressed vmlinux booting attempts

[Christian Borntraeger]

On 06/25/2018 05:09 PM, Vasily Gorbik wrote:
> Since uncompressed kernel image "vmlinux" elf file is not bootable under
> qemu anymore, add a check which would report that.
> 
> Qemu users are encouraged to use bzImage or
> arch/s390/boot/compressed/vmlinux instead.
> 
> The check relies on s390 linux entry point ABI definition, which is only
> present in bzImage and arch/s390/boot/compressed/vmlinux.
> 
> Signed-off-by: Vasily Gorbik <gor@linux.ibm.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>

some proposals regarding the wording below..

[...]
> +
> +	sclp_early_printk("The linux kernel boot failure: the image is corrupted or not bootable.\n");
> +	sclp_early_printk("Please check that you are using bootable kernel image \"bzImage\".\n");
> +	sclp_early_printk("(or alternatively \"arch/s390/boot/compressed/vmlinux\" image for qemu)\n");

What about making this explain things a bit more, e.g. something like

Linux kernel boot failure: The boot image does not contain all necessary
components (like the entry point and decompressor). The plain vmlinux ELF
file no longer carries all necessary parts for starting up. Please use
bzImage or arch/s390/boot/compressed/vmlinux.

Re: [PATCH] s390/boot: block uncompressed vmlinux booting attempts

[Cornelia Huck]

On Tue, 26 Jun 2018 09:30:19 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 06/25/2018 05:09 PM, Vasily Gorbik wrote:
> > Since uncompressed kernel image "vmlinux" elf file is not bootable under
> > qemu anymore, add a check which would report that.
> > 
> > Qemu users are encouraged to use bzImage or
> > arch/s390/boot/compressed/vmlinux instead.
> > 
> > The check relies on s390 linux entry point ABI definition, which is only
> > present in bzImage and arch/s390/boot/compressed/vmlinux.
> > 
> > Signed-off-by: Vasily Gorbik <gor@linux.ibm.com>  
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> some proposals regarding the wording below..
> 
> [...]
> > +
> > +	sclp_early_printk("The linux kernel boot failure: the image is corrupted or not bootable.\n");
> > +	sclp_early_printk("Please check that you are using bootable kernel image \"bzImage\".\n");
> > +	sclp_early_printk("(or alternatively \"arch/s390/boot/compressed/vmlinux\" image for qemu)\n");  
> 
> What about making this explain things a bit more, e.g. something like
> 
> Linux kernel boot failure: The boot image does not contain all necessary
> components (like the entry point and decompressor). The plain vmlinux ELF
> file no longer carries all necessary parts for starting up. Please use
> bzImage or arch/s390/boot/compressed/vmlinux.

Yes, that sounds good. With the changed message,

Acked-by: Cornelia Huck <cohuck@redhat.com>

Re: s390 qemu boot failure in -next

[Cornelia Huck]

On Mon, 25 Jun 2018 10:29:46 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 06/25/2018 10:05 AM, Cornelia Huck wrote:
> > On Mon, 25 Jun 2018 09:27:59 +0200
> > Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> >> Something like this in QEMU 
> >>
> >> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> >> index f278036fa7..14153ce880 100644
> >> --- a/hw/s390x/ipl.c
> >> +++ b/hw/s390x/ipl.c
> >> @@ -187,11 +187,13 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
> >>           */
> >>          if (pentry == KERN_IMAGE_START || pentry == 0x800) {
> >>              ipl->start_addr = KERN_IMAGE_START;
> >> -            /* Overwrite parameters in the kernel image, which are "rom" */
> >> -            strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline);
> >>          } else {
> >>              ipl->start_addr = pentry;
> >>          }
> >> +       if (ipl->cmdline) {
> >> +            /* If there is a command line, put it in the right place */
> >> +            strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline);
> >> +       }  
> > 
> > Check for the magic Linux string (like in the non-elf case) first?  
> 
> Even that does not exists in vmlinux but only in bzImage with the latest patchset
> (in next, but not upstream yet)

Ok.

> >   
> >>  
> >>          if (ipl->initrd) {
> >>              ram_addr_t initrd_offset;
> >>
> >> would put the command line in no matter what the start address is.  
> > 
> > I'm for putting that one in (and backporting it to qemu-stable). It's a
> > bit worrying, though, that our ipl code is so fragile...  
> 
> We actually have to combine this with Thomas fix (to check for rom_ptr returning
> something sane). It seems that ipl->commandline is always there, so we have to
> check for strlen!=0 it seems..
> 
> I mean if somebody ask for "-append something" we can certainly always write something
> if there is rom/ram.

Given that the uncompressed image is not supposed to be bootable
anymore, does it make sense to add this anyway?

I'll go ahead and queue Thomas' fix, though.

Re: s390 qemu boot failure in -next

[Christian Borntraeger]

On 06/26/2018 10:29 AM, Cornelia Huck wrote:
[...]
>>>>  
>>>>          if (ipl->initrd) {
>>>>              ram_addr_t initrd_offset;
>>>>
>>>> would put the command line in no matter what the start address is.  
>>>
>>> I'm for putting that one in (and backporting it to qemu-stable). It's a
>>> bit worrying, though, that our ipl code is so fragile...  
>>
>> We actually have to combine this with Thomas fix (to check for rom_ptr returning
>> something sane). It seems that ipl->commandline is always there, so we have to
>> check for strlen!=0 it seems..
>>
>> I mean if somebody ask for "-append something" we can certainly always write something
>> if there is rom/ram.
> 
> Given that the uncompressed image is not supposed to be bootable
> anymore, does it make sense to add this anyway?

I think we can drop this patch for now.

> 
> I'll go ahead and queue Thomas' fix, though.

yes, please