[Qemu-devel] [PATCH v3 1/1] s390x/ipl: Try to detect Linux vs non Linux for initial IPL PSW

[Qemu-devel] [PATCH v3 1/1] s390x/ipl: Try to detect Linux vs non Linux for initial IPL PSW

[Christian Borntraeger]

Right now the IPL device always starts from address 0x10000 (the usual
Linux entry point). To run other guests (e.g. test programs) it is
useful to use the IPL PSW from address 0. We can use the Linux magic
at 0x10008 to decide.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
v2->v3:
	- check for iplpsw to avoid assert on file errors
	- use 4 bytes at 4 instead of 8 bytes at 0
v1->v2:
	- use LINUX_MAGIC_ADDR define
	- use assert for valid iplpsw pointer
	- add endianess conversion
 hw/s390x/ipl.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 04245b5258..1a42630962 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -29,6 +29,7 @@
 #include "exec/exec-all.h"
 
 #define KERN_IMAGE_START                0x010000UL
+#define LINUX_MAGIC_ADDR                0x010008UL
 #define KERN_PARM_AREA                  0x010480UL
 #define INITRD_START                    0x800000UL
 #define INITRD_PARM_START               0x010408UL
@@ -105,7 +106,9 @@ static uint64_t bios_translate_addr(void *opaque, uint64_t srcaddr)
 static void s390_ipl_realize(DeviceState *dev, Error **errp)
 {
     S390IPLState *ipl = S390_IPL(dev);
-    uint64_t pentry = KERN_IMAGE_START;
+    uint32_t *iplpsw;
+    uint64_t pentry;
+    char *magic;
     int kernel_size;
     Error *err = NULL;
 
@@ -157,6 +160,20 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
                                NULL, 1, EM_S390, 0, 0);
         if (kernel_size < 0) {
             kernel_size = load_image_targphys(ipl->kernel, 0, ram_size);
+            /* if this is Linux use KERN_IMAGE_START */
+            magic = rom_ptr(LINUX_MAGIC_ADDR);
+            if (magic && !memcmp(magic, "S390EP", 6)) {
+                pentry = KERN_IMAGE_START;
+            } else {
+                /* if not Linux use the IPL PSW */
+                iplpsw = rom_ptr(4);
+                if (iplpsw) {
+                    pentry = be32_to_cpu(*iplpsw) & 0x7fffffffUL;
+                } else {
+                    error_setg(&err, "Could not get IPL PSW");
+                    goto error;
+                }
+            }
         }
         if (kernel_size < 0) {
             error_setg(&err, "could not load kernel '%s'", ipl->kernel);
-- 
2.17.0

Re: [Qemu-devel] [PATCH v3 1/1] s390x/ipl: Try to detect Linux vs non Linux for initial IPL PSW

[David Hildenbrand]

On 12.06.2018 12:04, Christian Borntraeger wrote:
> Right now the IPL device always starts from address 0x10000 (the usual
> Linux entry point). To run other guests (e.g. test programs) it is
> useful to use the IPL PSW from address 0. We can use the Linux magic
> at 0x10008 to decide.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> v2->v3:
> 	- check for iplpsw to avoid assert on file errors
> 	- use 4 bytes at 4 instead of 8 bytes at 0

I trust you that this is the right thing to do :)

Reviewed-by: David Hildenbrand <david@redhat.com>

> v1->v2:
> 	- use LINUX_MAGIC_ADDR define
> 	- use assert for valid iplpsw pointer
> 	- add endianess conversion
>  hw/s390x/ipl.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 04245b5258..1a42630962 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -29,6 +29,7 @@
>  #include "exec/exec-all.h"
>  
>  #define KERN_IMAGE_START                0x010000UL
> +#define LINUX_MAGIC_ADDR                0x010008UL
>  #define KERN_PARM_AREA                  0x010480UL
>  #define INITRD_START                    0x800000UL
>  #define INITRD_PARM_START               0x010408UL
> @@ -105,7 +106,9 @@ static uint64_t bios_translate_addr(void *opaque, uint64_t srcaddr)
>  static void s390_ipl_realize(DeviceState *dev, Error **errp)
>  {
>      S390IPLState *ipl = S390_IPL(dev);
> -    uint64_t pentry = KERN_IMAGE_START;
> +    uint32_t *iplpsw;
> +    uint64_t pentry;
> +    char *magic;
>      int kernel_size;
>      Error *err = NULL;
>  
> @@ -157,6 +160,20 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
>                                 NULL, 1, EM_S390, 0, 0);
>          if (kernel_size < 0) {
>              kernel_size = load_image_targphys(ipl->kernel, 0, ram_size);
> +            /* if this is Linux use KERN_IMAGE_START */
> +            magic = rom_ptr(LINUX_MAGIC_ADDR);
> +            if (magic && !memcmp(magic, "S390EP", 6)) {
> +                pentry = KERN_IMAGE_START;
> +            } else {
> +                /* if not Linux use the IPL PSW */
> +                iplpsw = rom_ptr(4);
> +                if (iplpsw) {
> +                    pentry = be32_to_cpu(*iplpsw) & 0x7fffffffUL;
> +                } else {
> +                    error_setg(&err, "Could not get IPL PSW");
> +                    goto error;
> +                }
> +            }
>          }
>          if (kernel_size < 0) {
>              error_setg(&err, "could not load kernel '%s'", ipl->kernel);
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v3 1/1] s390x/ipl: Try to detect Linux vs non Linux for initial IPL PSW

[Thomas Huth]

On 12.06.2018 12:04, Christian Borntraeger wrote:
> Right now the IPL device always starts from address 0x10000 (the usual
> Linux entry point). To run other guests (e.g. test programs) it is
> useful to use the IPL PSW from address 0. We can use the Linux magic
> at 0x10008 to decide.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> v2->v3:
> 	- check for iplpsw to avoid assert on file errors
> 	- use 4 bytes at 4 instead of 8 bytes at 0
> v1->v2:
> 	- use LINUX_MAGIC_ADDR define
> 	- use assert for valid iplpsw pointer
> 	- add endianess conversion
>  hw/s390x/ipl.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 04245b5258..1a42630962 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -29,6 +29,7 @@
>  #include "exec/exec-all.h"
>  
>  #define KERN_IMAGE_START                0x010000UL
> +#define LINUX_MAGIC_ADDR                0x010008UL
>  #define KERN_PARM_AREA                  0x010480UL
>  #define INITRD_START                    0x800000UL
>  #define INITRD_PARM_START               0x010408UL
> @@ -105,7 +106,9 @@ static uint64_t bios_translate_addr(void *opaque, uint64_t srcaddr)
>  static void s390_ipl_realize(DeviceState *dev, Error **errp)
>  {
>      S390IPLState *ipl = S390_IPL(dev);
> -    uint64_t pentry = KERN_IMAGE_START;
> +    uint32_t *iplpsw;
> +    uint64_t pentry;
> +    char *magic;
>      int kernel_size;
>      Error *err = NULL;
>  
> @@ -157,6 +160,20 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
>                                 NULL, 1, EM_S390, 0, 0);
>          if (kernel_size < 0) {
>              kernel_size = load_image_targphys(ipl->kernel, 0, ram_size);
> +            /* if this is Linux use KERN_IMAGE_START */
> +            magic = rom_ptr(LINUX_MAGIC_ADDR);
> +            if (magic && !memcmp(magic, "S390EP", 6)) {
> +                pentry = KERN_IMAGE_START;
> +            } else {
> +                /* if not Linux use the IPL PSW */
> +                iplpsw = rom_ptr(4);
> +                if (iplpsw) {
> +                    pentry = be32_to_cpu(*iplpsw) & 0x7fffffffUL;
> +                } else {
> +                    error_setg(&err, "Could not get IPL PSW");
> +                    goto error;
> +                }
> +            }
>          }
>          if (kernel_size < 0) {
>              error_setg(&err, "could not load kernel '%s'", ipl->kernel);
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v3 1/1] s390x/ipl: Try to detect Linux vs non Linux for initial IPL PSW

[Cornelia Huck]

On Tue, 12 Jun 2018 12:04:48 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> Right now the IPL device always starts from address 0x10000 (the usual
> Linux entry point). To run other guests (e.g. test programs) it is
> useful to use the IPL PSW from address 0. We can use the Linux magic
> at 0x10008 to decide.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> v2->v3:
> 	- check for iplpsw to avoid assert on file errors
> 	- use 4 bytes at 4 instead of 8 bytes at 0
> v1->v2:
> 	- use LINUX_MAGIC_ADDR define
> 	- use assert for valid iplpsw pointer
> 	- add endianess conversion
>  hw/s390x/ipl.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 04245b5258..1a42630962 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -29,6 +29,7 @@
>  #include "exec/exec-all.h"
>  
>  #define KERN_IMAGE_START                0x010000UL
> +#define LINUX_MAGIC_ADDR                0x010008UL
>  #define KERN_PARM_AREA                  0x010480UL
>  #define INITRD_START                    0x800000UL
>  #define INITRD_PARM_START               0x010408UL
> @@ -105,7 +106,9 @@ static uint64_t bios_translate_addr(void *opaque, uint64_t srcaddr)
>  static void s390_ipl_realize(DeviceState *dev, Error **errp)
>  {
>      S390IPLState *ipl = S390_IPL(dev);
> -    uint64_t pentry = KERN_IMAGE_START;
> +    uint32_t *iplpsw;

In case you respin: Can this be ipl_psw, please?

> +    uint64_t pentry;
> +    char *magic;
>      int kernel_size;
>      Error *err = NULL;
>  
> @@ -157,6 +160,20 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
>                                 NULL, 1, EM_S390, 0, 0);
>          if (kernel_size < 0) {
>              kernel_size = load_image_targphys(ipl->kernel, 0, ram_size);
> +            /* if this is Linux use KERN_IMAGE_START */
> +            magic = rom_ptr(LINUX_MAGIC_ADDR);
> +            if (magic && !memcmp(magic, "S390EP", 6)) {
> +                pentry = KERN_IMAGE_START;
> +            } else {
> +                /* if not Linux use the IPL PSW */

"use the address of the IPL PSW"?

> +                iplpsw = rom_ptr(4);
> +                if (iplpsw) {
> +                    pentry = be32_to_cpu(*iplpsw) & 0x7fffffffUL;
> +                } else {
> +                    error_setg(&err, "Could not get IPL PSW");
> +                    goto error;
> +                }
> +            }
>          }
>          if (kernel_size < 0) {
>              error_setg(&err, "could not load kernel '%s'", ipl->kernel);

Is this ever reached now? In any case, it might make sense to move this
up, as the "could not load kernel 'plahh'" message seems more useful
than "Could not get IPL PSW"...

Also, further below we copy ipl->cmdline if we think we're dealing with
a Linux image. Would it make sense to explicitly check for the Linux
magic there as well? (Separate patch, of course.)

Can/should netboot do a similar check? It uses KERN_IMAGE_START
unconditionally if it wasn't an ELF file.

Re: [Qemu-devel] [PATCH v3 1/1] s390x/ipl: Try to detect Linux vs non Linux for initial IPL PSW

[Christian Borntraeger]

On 06/12/2018 01:55 PM, Cornelia Huck wrote:
> On Tue, 12 Jun 2018 12:04:48 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> Right now the IPL device always starts from address 0x10000 (the usual
>> Linux entry point). To run other guests (e.g. test programs) it is
>> useful to use the IPL PSW from address 0. We can use the Linux magic
>> at 0x10008 to decide.
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>> v2->v3:
>> 	- check for iplpsw to avoid assert on file errors
>> 	- use 4 bytes at 4 instead of 8 bytes at 0
>> v1->v2:
>> 	- use LINUX_MAGIC_ADDR define
>> 	- use assert for valid iplpsw pointer
>> 	- add endianess conversion
>>  hw/s390x/ipl.c | 19 ++++++++++++++++++-
>>  1 file changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index 04245b5258..1a42630962 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -29,6 +29,7 @@
>>  #include "exec/exec-all.h"
>>  
>>  #define KERN_IMAGE_START                0x010000UL
>> +#define LINUX_MAGIC_ADDR                0x010008UL
>>  #define KERN_PARM_AREA                  0x010480UL
>>  #define INITRD_START                    0x800000UL
>>  #define INITRD_PARM_START               0x010408UL
>> @@ -105,7 +106,9 @@ static uint64_t bios_translate_addr(void *opaque, uint64_t srcaddr)
>>  static void s390_ipl_realize(DeviceState *dev, Error **errp)
>>  {
>>      S390IPLState *ipl = S390_IPL(dev);
>> -    uint64_t pentry = KERN_IMAGE_START;
>> +    uint32_t *iplpsw;
> 
> In case you respin: Can this be ipl_psw, please?

sure. 
> 
>> +    uint64_t pentry;
>> +    char *magic;
>>      int kernel_size;
>>      Error *err = NULL;
>>  
>> @@ -157,6 +160,20 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
>>                                 NULL, 1, EM_S390, 0, 0);
>>          if (kernel_size < 0) {
>>              kernel_size = load_image_targphys(ipl->kernel, 0, ram_size);
>> +            /* if this is Linux use KERN_IMAGE_START */
>> +            magic = rom_ptr(LINUX_MAGIC_ADDR);
>> +            if (magic && !memcmp(magic, "S390EP", 6)) {
>> +                pentry = KERN_IMAGE_START;
>> +            } else {
>> +                /* if not Linux use the IPL PSW */
> 
> "use the address of the IPL PSW"?

Well we use the IPL PSW (and we need the address to get that) See the * before iplpsw.
> 
>> +                iplpsw = rom_ptr(4);
>> +                if (iplpsw) {
>> +                    pentry = be32_to_cpu(*iplpsw) & 0x7fffffffUL;
>> +                } else {
>> +                    error_setg(&err, "Could not get IPL PSW");
>> +                    goto error;
>> +                }
>> +            }
>>          }
>>          if (kernel_size < 0) {
>>              error_setg(&err, "could not load kernel '%s'", ipl->kernel);
> 
> Is this ever reached now? In any case, it might make sense to move this

Hmm, Right. If kernel_size < 0  then magic and ipl_psw should be NULL and we
error out early


> up, as the "could not load kernel 'plahh'" message seems more useful
> than "Could not get IPL PSW"...
> 
> Also, further below we copy ipl->cmdline if we think we're dealing with
> a Linux image. Would it make sense to explicitly check for the Linux
> magic there as well? (Separate patch, of course.)
> 
> Can/should netboot do a similar check? It uses KERN_IMAGE_START
> unconditionally if it wasn't an ELF file.

I think we already changed that there (I asked for that change).