[PATCH 0/2] pc-bios/s390 fixes for reboot-to-vfio-ccw

[PATCH 0/2] pc-bios/s390 fixes for reboot-to-vfio-ccw

[Eric Farman]

Hi Conny, Thomas,

I've come across a couple problems when installing a guest onto a
vfio-ccw disk. They were noticed when booting off an ISO via virtio,
and writing the new guest OS to a vfio-ccw connected disk. The
installation works correctly, in that the disk can be booted and
used perfectly fine. But the end of the install process reboots to
the new disk, and we end up with an error that looks like this:

Rebooting.
LOADPARM=[        ]
cio device error
  ssid  : 0x0000000000000000
  cssid : 0x0000000000000000
  sch_no: 0x0000000000000002
  ctrl-unit type: 0x0000000000003990

Interrupt Response Block Data:
: 0x0000000000003990
    Function Ctrl : [Start]
    Activity Ctrl : [Start-Pending]
    Status Ctrl : [Alert] [Primary] [Secondary] [Status-Pending]
    Device Status :
    Channel Status : [Program-Check]
    cpa=: 0x0000000000000008
    prev_ccw=: 0x0008000080002018
    this_ccw=: 0x0600001860000080
Eckd Dasd Sense Data (fmt 32-bytes):
    Channel Status : [Program-Check]
    Sense Condition Flags :
    Residual Count     =: 0x0000000000000000
    Phys Drive ID      =: 0x0000000000000000
    low cyl address    =: 0x0000000000000000
    head addr & hi cyl =: 0x0000000000000000
    format/message     =: 0x0000000000000000
    fmt-dependent[0-7] =: 0x0000000000000000
    fmt-dependent[8-15]=: 0x000000004a200f00
    prog action code   =: 0x0000000000000000
    Configuration info =: 0x00000000000040e0
    mcode / hi-cyl     =: 0x0000000000000000
    cyl & head addr [0]=: 0x0000000000000000
    cyl & head addr [1]=: 0x0000000000000000
    cyl & head addr [2]=: 0x0000000000000000
dasd-ipl: Failed to run Read IPL channel program

The problem can be easily (and quickly) reproduced by defining
XML with two different guest disks, such as:

    <disk type='block' device='disk'>
      <driver name='qemu' type='raw'/>
      <source dev='/dev/disk/by-path/ccw-0.0.1234'/>
      <target dev='vda' bus='virtio'/>
      <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0000'/>
    </disk>
    <hostdev mode='subsystem' type='mdev' managed='no' model='vfio-ccw'>
      <source>
        <address uuid='12345678-1234-1234-1234-123456789abc'/>
      </source>
      <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x1235'/>
    </hostdev>

Boot the guest off disk 1234, issue `chreipl 0.0.1235`, and reboot.
Interestingly, going the other direction (1235->1234) works fine.

Two things occur here. First, the DASD IPL code builds a Read IPL
Format-0 CCW at address zero, but assumes that the memory is already
zero. If data is already present (which on reboot it does, and is
probably a PSW), it might be included in that CCW. Patch 1 is a
pretty straightforward fix for this. (Might this one be appropriate
for 5.2?)

Secondly, the jump code looks at a couple memory locations to determine
what jump to perform. But in this virtio-to-vfio jump, we should give
precedence to the data at address zero, rather than the not-yet-cleared
memory. Patch 2 swaps the order of these checks to enable this, and
doesn't appear to affect normal disk boots. But there are a hundred
other boot combinations that I haven't tried here, so could use some
feedback on this one. I do recognize that this leaves no difference
between the "if (!memcmp())" case and not, but left it this way to
help better visualize the change I'm making.

Coda:
I didn't include rebuilt bios patch(es) here, but certainly can.
Also, I'm on holiday all next week; so apologies in advance for
sending a couple patches and then hiding shortly thereafter. :)

Eric Farman (2):
  pc-bios: s390x: Ensure Read IPL memory is clean
  pc-bios: s390x: Give precedence to reset PSW

 pc-bios/s390-ccw/dasd-ipl.c |  3 +++
 pc-bios/s390-ccw/jump2ipl.c | 20 ++++++++++----------
 2 files changed, 13 insertions(+), 10 deletions(-)

-- 
2.17.1

[PATCH 1/2] pc-bios: s390x: Ensure Read IPL memory is clean

[Eric Farman]

If, for example, we boot off a virtio device and chreipl to a vfio-ccw
device, the space at lowcore will be non-zero. We build a Read IPL CCW
at address zero, but it will have leftover PSW data that will conflict
with the Format-0 CCW being generated:

0x0: 00080000 80010000
       ------ Ccw0.cda
              -- Ccw0.chainData
                -- Reserved bits

The data address will be overwritten with the correct value (0x0), but
the apparent data chain bit will cause subsequent memory to be used as
the target of the data store, which may not be where we expect (0x0).

Clear out this space when we boot from DASD, so that we know it exists
exactly as we expect.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 pc-bios/s390-ccw/dasd-ipl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/pc-bios/s390-ccw/dasd-ipl.c b/pc-bios/s390-ccw/dasd-ipl.c
index 0fc879bb8e..71cbae2f16 100644
--- a/pc-bios/s390-ccw/dasd-ipl.c
+++ b/pc-bios/s390-ccw/dasd-ipl.c
@@ -100,6 +100,9 @@ static void make_readipl(void)
 {
     Ccw0 *ccwIplRead = (Ccw0 *)0x00;
 
+    /* Clear out any existing data */
+    memset(ccwIplRead, 0, sizeof(Ccw0));
+
     /* Create Read IPL ccw at address 0 */
     ccwIplRead->cmd_code = CCW_CMD_READ_IPL;
     ccwIplRead->cda = 0x00; /* Read into address 0x00 in main memory */
-- 
2.17.1

[PATCH 2/2] pc-bios: s390x: Give precedence to reset PSW

[Eric Farman]

Let's look at the Reset PSW first instead of the contents of memory.
It might be leftover from an earlier system boot when processing
a chreipl.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 pc-bios/s390-ccw/jump2ipl.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
index fbae45b03c..67b4afb6a0 100644
--- a/pc-bios/s390-ccw/jump2ipl.c
+++ b/pc-bios/s390-ccw/jump2ipl.c
@@ -72,16 +72,6 @@ void jump_to_IPL_code(uint64_t address)
 
 void jump_to_low_kernel(void)
 {
-    /*
-     * If it looks like a Linux binary, i.e. there is the "S390EP" magic from
-     * arch/s390/kernel/head.S here, then let's jump to the well-known Linux
-     * kernel start address (when jumping to the PSW-at-zero address instead,
-     * the kernel startup code fails when we booted from a network device).
-     */
-    if (!memcmp((char *)0x10008, "S390EP", 6)) {
-        jump_to_IPL_code(KERN_IMAGE_START);
-    }
-
     /* Trying to get PSW at zero address */
     if (*((uint64_t *)0) & RESET_PSW_MASK) {
         /*
@@ -92,6 +82,16 @@ void jump_to_low_kernel(void)
         jump_to_IPL_code(0);
     }
 
+    /*
+     * If it looks like a Linux binary, i.e. there is the "S390EP" magic from
+     * arch/s390/kernel/head.S here, then let's jump to the well-known Linux
+     * kernel start address (when jumping to the PSW-at-zero address instead,
+     * the kernel startup code fails when we booted from a network device).
+     */
+    if (!memcmp((char *)0x10008, "S390EP", 6)) {
+        jump_to_IPL_code(KERN_IMAGE_START);
+    }
+
     /* No other option left, so use the Linux kernel start address */
     jump_to_IPL_code(KERN_IMAGE_START);
 }
-- 
2.17.1

Re: [PATCH 0/2] pc-bios/s390 fixes for reboot-to-vfio-ccw

[Cornelia Huck]

On Thu, 19 Nov 2020 17:57:27 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> Hi Conny, Thomas,
> 
> I've come across a couple problems when installing a guest onto a
> vfio-ccw disk. They were noticed when booting off an ISO via virtio,
> and writing the new guest OS to a vfio-ccw connected disk. The
> installation works correctly, in that the disk can be booted and
> used perfectly fine. But the end of the install process reboots to
> the new disk, and we end up with an error that looks like this:
> 
> Rebooting.
> LOADPARM=[        ]
> cio device error
>   ssid  : 0x0000000000000000
>   cssid : 0x0000000000000000
>   sch_no: 0x0000000000000002
>   ctrl-unit type: 0x0000000000003990
> 
> Interrupt Response Block Data:
> : 0x0000000000003990
>     Function Ctrl : [Start]
>     Activity Ctrl : [Start-Pending]
>     Status Ctrl : [Alert] [Primary] [Secondary] [Status-Pending]
>     Device Status :
>     Channel Status : [Program-Check]
>     cpa=: 0x0000000000000008
>     prev_ccw=: 0x0008000080002018
>     this_ccw=: 0x0600001860000080
> Eckd Dasd Sense Data (fmt 32-bytes):
>     Channel Status : [Program-Check]
>     Sense Condition Flags :
>     Residual Count     =: 0x0000000000000000
>     Phys Drive ID      =: 0x0000000000000000
>     low cyl address    =: 0x0000000000000000
>     head addr & hi cyl =: 0x0000000000000000
>     format/message     =: 0x0000000000000000
>     fmt-dependent[0-7] =: 0x0000000000000000
>     fmt-dependent[8-15]=: 0x000000004a200f00
>     prog action code   =: 0x0000000000000000
>     Configuration info =: 0x00000000000040e0
>     mcode / hi-cyl     =: 0x0000000000000000
>     cyl & head addr [0]=: 0x0000000000000000
>     cyl & head addr [1]=: 0x0000000000000000
>     cyl & head addr [2]=: 0x0000000000000000
> dasd-ipl: Failed to run Read IPL channel program
> 
> The problem can be easily (and quickly) reproduced by defining
> XML with two different guest disks, such as:
> 
>     <disk type='block' device='disk'>
>       <driver name='qemu' type='raw'/>
>       <source dev='/dev/disk/by-path/ccw-0.0.1234'/>
>       <target dev='vda' bus='virtio'/>
>       <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x0000'/>
>     </disk>
>     <hostdev mode='subsystem' type='mdev' managed='no' model='vfio-ccw'>
>       <source>
>         <address uuid='12345678-1234-1234-1234-123456789abc'/>
>       </source>
>       <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x1235'/>
>     </hostdev>
> 
> Boot the guest off disk 1234, issue `chreipl 0.0.1235`, and reboot.
> Interestingly, going the other direction (1235->1234) works fine.
> 
> Two things occur here. First, the DASD IPL code builds a Read IPL
> Format-0 CCW at address zero, but assumes that the memory is already
> zero. If data is already present (which on reboot it does, and is
> probably a PSW), it might be included in that CCW. Patch 1 is a
> pretty straightforward fix for this. (Might this one be appropriate
> for 5.2?)

That one is straightforward enough for 5.2, I guess.

> 
> Secondly, the jump code looks at a couple memory locations to determine
> what jump to perform. But in this virtio-to-vfio jump, we should give
> precedence to the data at address zero, rather than the not-yet-cleared
> memory. Patch 2 swaps the order of these checks to enable this, and
> doesn't appear to affect normal disk boots. But there are a hundred
> other boot combinations that I haven't tried here, so could use some
> feedback on this one. I do recognize that this leaves no difference
> between the "if (!memcmp())" case and not, but left it this way to
> help better visualize the change I'm making.

This is in code that usually gives me a headache :(

> 
> Coda:
> I didn't include rebuilt bios patch(es) here, but certainly can.

I think it should not be too hard to have the person picking the
patches generate them :) Especially if we end up going with patch 1
only for now.

> Also, I'm on holiday all next week; so apologies in advance for
> sending a couple patches and then hiding shortly thereafter. :)

That's a time-proven strategy :)

> 
> Eric Farman (2):
>   pc-bios: s390x: Ensure Read IPL memory is clean
>   pc-bios: s390x: Give precedence to reset PSW
> 
>  pc-bios/s390-ccw/dasd-ipl.c |  3 +++
>  pc-bios/s390-ccw/jump2ipl.c | 20 ++++++++++----------
>  2 files changed, 13 insertions(+), 10 deletions(-)
> 

Re: [PATCH 1/2] pc-bios: s390x: Ensure Read IPL memory is clean

[Thomas Huth]

On 19/11/2020 17.57, Eric Farman wrote:
> If, for example, we boot off a virtio device and chreipl to a vfio-ccw
> device, the space at lowcore will be non-zero. We build a Read IPL CCW
> at address zero, but it will have leftover PSW data that will conflict
> with the Format-0 CCW being generated:
> 
> 0x0: 00080000 80010000
>        ------ Ccw0.cda
>               -- Ccw0.chainData
>                 -- Reserved bits
> 
> The data address will be overwritten with the correct value (0x0), but
> the apparent data chain bit will cause subsequent memory to be used as
> the target of the data store, which may not be where we expect (0x0).
> 
> Clear out this space when we boot from DASD, so that we know it exists
> exactly as we expect.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  pc-bios/s390-ccw/dasd-ipl.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/pc-bios/s390-ccw/dasd-ipl.c b/pc-bios/s390-ccw/dasd-ipl.c
> index 0fc879bb8e..71cbae2f16 100644
> --- a/pc-bios/s390-ccw/dasd-ipl.c
> +++ b/pc-bios/s390-ccw/dasd-ipl.c
> @@ -100,6 +100,9 @@ static void make_readipl(void)
>  {
>      Ccw0 *ccwIplRead = (Ccw0 *)0x00;
>  
> +    /* Clear out any existing data */
> +    memset(ccwIplRead, 0, sizeof(Ccw0));
> +
>      /* Create Read IPL ccw at address 0 */
>      ccwIplRead->cmd_code = CCW_CMD_READ_IPL;
>      ccwIplRead->cda = 0x00; /* Read into address 0x00 in main memory */

Sounds reasonable.

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

Re: [PATCH 2/2] pc-bios: s390x: Give precedence to reset PSW

[Thomas Huth]

On 19/11/2020 17.57, Eric Farman wrote:
> Let's look at the Reset PSW first instead of the contents of memory.
> It might be leftover from an earlier system boot when processing
> a chreipl.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  pc-bios/s390-ccw/jump2ipl.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
> index fbae45b03c..67b4afb6a0 100644
> --- a/pc-bios/s390-ccw/jump2ipl.c
> +++ b/pc-bios/s390-ccw/jump2ipl.c
> @@ -72,16 +72,6 @@ void jump_to_IPL_code(uint64_t address)
>  
>  void jump_to_low_kernel(void)
>  {
> -    /*
> -     * If it looks like a Linux binary, i.e. there is the "S390EP" magic from
> -     * arch/s390/kernel/head.S here, then let's jump to the well-known Linux
> -     * kernel start address (when jumping to the PSW-at-zero address instead,
> -     * the kernel startup code fails when we booted from a network device).
> -     */
> -    if (!memcmp((char *)0x10008, "S390EP", 6)) {
> -        jump_to_IPL_code(KERN_IMAGE_START);
> -    }
> -
>      /* Trying to get PSW at zero address */
>      if (*((uint64_t *)0) & RESET_PSW_MASK) {
>          /*
> @@ -92,6 +82,16 @@ void jump_to_low_kernel(void)
>          jump_to_IPL_code(0);
>      }
>  
> +    /*
> +     * If it looks like a Linux binary, i.e. there is the "S390EP" magic from
> +     * arch/s390/kernel/head.S here, then let's jump to the well-known Linux
> +     * kernel start address (when jumping to the PSW-at-zero address instead,
> +     * the kernel startup code fails when we booted from a network device).
> +     */
> +    if (!memcmp((char *)0x10008, "S390EP", 6)) {
> +        jump_to_IPL_code(KERN_IMAGE_START);
> +    }

That feels a little bit dangerous ... I assume the order has been that way
for a reason, e.g. I think we had to jump to KERN_IMAGE_START for some older
versions of the Linux kernel since the startup code that was referenced by
the PSW at address zero was not working in KVM...

What do you think about this alternate idea instead: Clear the memory at
location 0x10008 at the very beginning of the main() function (or maybe in
boot_setup())? Then we can be sure that there is no stale S390EP magic
dangling around anymore once we've loaded the new kernel...

 Thomas

Re: [PATCH 2/2] pc-bios: s390x: Give precedence to reset PSW

[Eric Farman]

On 11/19/20 3:20 PM, Thomas Huth wrote:
> On 19/11/2020 17.57, Eric Farman wrote:
>> Let's look at the Reset PSW first instead of the contents of memory.
>> It might be leftover from an earlier system boot when processing
>> a chreipl.
>>
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>   pc-bios/s390-ccw/jump2ipl.c | 20 ++++++++++----------
>>   1 file changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
>> index fbae45b03c..67b4afb6a0 100644
>> --- a/pc-bios/s390-ccw/jump2ipl.c
>> +++ b/pc-bios/s390-ccw/jump2ipl.c
>> @@ -72,16 +72,6 @@ void jump_to_IPL_code(uint64_t address)
>>   
>>   void jump_to_low_kernel(void)
>>   {
>> -    /*
>> -     * If it looks like a Linux binary, i.e. there is the "S390EP" magic from
>> -     * arch/s390/kernel/head.S here, then let's jump to the well-known Linux
>> -     * kernel start address (when jumping to the PSW-at-zero address instead,
>> -     * the kernel startup code fails when we booted from a network device).
>> -     */
>> -    if (!memcmp((char *)0x10008, "S390EP", 6)) {
>> -        jump_to_IPL_code(KERN_IMAGE_START);
>> -    }
>> -
>>       /* Trying to get PSW at zero address */
>>       if (*((uint64_t *)0) & RESET_PSW_MASK) {
>>           /*
>> @@ -92,6 +82,16 @@ void jump_to_low_kernel(void)
>>           jump_to_IPL_code(0);
>>       }
>>   
>> +    /*
>> +     * If it looks like a Linux binary, i.e. there is the "S390EP" magic from
>> +     * arch/s390/kernel/head.S here, then let's jump to the well-known Linux
>> +     * kernel start address (when jumping to the PSW-at-zero address instead,
>> +     * the kernel startup code fails when we booted from a network device).
>> +     */
>> +    if (!memcmp((char *)0x10008, "S390EP", 6)) {
>> +        jump_to_IPL_code(KERN_IMAGE_START);
>> +    }
> 
> That feels a little bit dangerous ... I assume the order has been that way
> for a reason, e.g. I think we had to jump to KERN_IMAGE_START for some older
> versions of the Linux kernel since the startup code that was referenced by
> the PSW at address zero was not working in KVM...

Makes sense.  It does seem like a precarious piece of code.

> 
> What do you think about this alternate idea instead: Clear the memory at
> location 0x10008 at the very beginning of the main() function (or maybe in
> boot_setup())? 

This seems to work too (I put it in boot_setup(), prior to call to 
store_iplb()).

Then we can be sure that there is no stale S390EP magic
> dangling around anymore once we've loaded the new kernel...
> 
>   Thomas
> 

Re: [PATCH 2/2] pc-bios: s390x: Give precedence to reset PSW

[Thomas Huth]

On 19/11/2020 22.11, Eric Farman wrote:
> 
> 
> On 11/19/20 3:20 PM, Thomas Huth wrote:
>> On 19/11/2020 17.57, Eric Farman wrote:
>>> Let's look at the Reset PSW first instead of the contents of memory.
>>> It might be leftover from an earlier system boot when processing
>>> a chreipl.
>>>
>>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>>> ---
>>>   pc-bios/s390-ccw/jump2ipl.c | 20 ++++++++++----------
>>>   1 file changed, 10 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
>>> index fbae45b03c..67b4afb6a0 100644
>>> --- a/pc-bios/s390-ccw/jump2ipl.c
>>> +++ b/pc-bios/s390-ccw/jump2ipl.c
>>> @@ -72,16 +72,6 @@ void jump_to_IPL_code(uint64_t address)
>>>     void jump_to_low_kernel(void)
>>>   {
>>> -    /*
>>> -     * If it looks like a Linux binary, i.e. there is the "S390EP" magic
>>> from
>>> -     * arch/s390/kernel/head.S here, then let's jump to the well-known
>>> Linux
>>> -     * kernel start address (when jumping to the PSW-at-zero address
>>> instead,
>>> -     * the kernel startup code fails when we booted from a network device).
>>> -     */
>>> -    if (!memcmp((char *)0x10008, "S390EP", 6)) {
>>> -        jump_to_IPL_code(KERN_IMAGE_START);
>>> -    }
>>> -
>>>       /* Trying to get PSW at zero address */
>>>       if (*((uint64_t *)0) & RESET_PSW_MASK) {
>>>           /*
>>> @@ -92,6 +82,16 @@ void jump_to_low_kernel(void)
>>>           jump_to_IPL_code(0);
>>>       }
>>>   +    /*
>>> +     * If it looks like a Linux binary, i.e. there is the "S390EP" magic
>>> from
>>> +     * arch/s390/kernel/head.S here, then let's jump to the well-known
>>> Linux
>>> +     * kernel start address (when jumping to the PSW-at-zero address
>>> instead,
>>> +     * the kernel startup code fails when we booted from a network device).
>>> +     */
>>> +    if (!memcmp((char *)0x10008, "S390EP", 6)) {
>>> +        jump_to_IPL_code(KERN_IMAGE_START);
>>> +    }
>>
>> That feels a little bit dangerous ... I assume the order has been that way
>> for a reason, e.g. I think we had to jump to KERN_IMAGE_START for some older
>> versions of the Linux kernel since the startup code that was referenced by
>> the PSW at address zero was not working in KVM...
> 
> Makes sense.  It does seem like a precarious piece of code.
> 
>>
>> What do you think about this alternate idea instead: Clear the memory at
>> location 0x10008 at the very beginning of the main() function (or maybe in
>> boot_setup())? 
> 
> This seems to work too (I put it in boot_setup(), prior to call to
> store_iplb()).

Great, can you send the patch before your holidays next week? (If you don't
have enough time, that's ok, too, it's trivial enough, so I think I could
write such a patch, too)

 Thomas

Re: [PATCH 1/2] pc-bios: s390x: Ensure Read IPL memory is clean

[Janosch Frank]

[-- Attachment #1.1.1: Type: text/plain, Size: 1694 bytes --]

On 11/19/20 5:57 PM, Eric Farman wrote:
> If, for example, we boot off a virtio device and chreipl to a vfio-ccw
> device, the space at lowcore will be non-zero. We build a Read IPL CCW
> at address zero, but it will have leftover PSW data that will conflict
> with the Format-0 CCW being generated:
> 
> 0x0: 00080000 80010000
>        ------ Ccw0.cda
>               -- Ccw0.chainData
>                 -- Reserved bits
> 
> The data address will be overwritten with the correct value (0x0), but
> the apparent data chain bit will cause subsequent memory to be used as
> the target of the data store, which may not be where we expect (0x0).
> 
> Clear out this space when we boot from DASD, so that we know it exists
> exactly as we expect.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>

Seems like I should have been more specific with my LGTM:
Reviewed-by: Janosch Frank <frankja@de.ibm.com>

> ---
>  pc-bios/s390-ccw/dasd-ipl.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/pc-bios/s390-ccw/dasd-ipl.c b/pc-bios/s390-ccw/dasd-ipl.c
> index 0fc879bb8e..71cbae2f16 100644
> --- a/pc-bios/s390-ccw/dasd-ipl.c
> +++ b/pc-bios/s390-ccw/dasd-ipl.c
> @@ -100,6 +100,9 @@ static void make_readipl(void)
>  {
>      Ccw0 *ccwIplRead = (Ccw0 *)0x00;
>  
> +    /* Clear out any existing data */
> +    memset(ccwIplRead, 0, sizeof(Ccw0));
> +
>      /* Create Read IPL ccw at address 0 */
>      ccwIplRead->cmd_code = CCW_CMD_READ_IPL;
>      ccwIplRead->cda = 0x00; /* Read into address 0x00 in main memory */
> 


[-- Attachment #1.1.2: OpenPGP_0xE354E6B8E238B9F8.asc --]
[-- Type: application/pgp-keys, Size: 7995 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 1/2] pc-bios: s390x: Ensure Read IPL memory is clean

[Cornelia Huck]

On Thu, 19 Nov 2020 17:57:28 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> If, for example, we boot off a virtio device and chreipl to a vfio-ccw
> device, the space at lowcore will be non-zero. We build a Read IPL CCW
> at address zero, but it will have leftover PSW data that will conflict
> with the Format-0 CCW being generated:
> 
> 0x0: 00080000 80010000
>        ------ Ccw0.cda
>               -- Ccw0.chainData
>                 -- Reserved bits
> 
> The data address will be overwritten with the correct value (0x0), but
> the apparent data chain bit will cause subsequent memory to be used as
> the target of the data store, which may not be where we expect (0x0).
> 
> Clear out this space when we boot from DASD, so that we know it exists
> exactly as we expect.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  pc-bios/s390-ccw/dasd-ipl.c | 3 +++
>  1 file changed, 3 insertions(+)

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

Re: [PATCH 2/2] pc-bios: s390x: Give precedence to reset PSW

[Eric Farman]

On 11/20/20 1:02 AM, Thomas Huth wrote:
> On 19/11/2020 22.11, Eric Farman wrote:
>>
>>
>> On 11/19/20 3:20 PM, Thomas Huth wrote:
>>> On 19/11/2020 17.57, Eric Farman wrote:
>>>> Let's look at the Reset PSW first instead of the contents of memory.
>>>> It might be leftover from an earlier system boot when processing
>>>> a chreipl.
>>>>
>>>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>>>> ---
>>>>    pc-bios/s390-ccw/jump2ipl.c | 20 ++++++++++----------
>>>>    1 file changed, 10 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
>>>> index fbae45b03c..67b4afb6a0 100644
>>>> --- a/pc-bios/s390-ccw/jump2ipl.c
>>>> +++ b/pc-bios/s390-ccw/jump2ipl.c
>>>> @@ -72,16 +72,6 @@ void jump_to_IPL_code(uint64_t address)
>>>>      void jump_to_low_kernel(void)
>>>>    {
>>>> -    /*
>>>> -     * If it looks like a Linux binary, i.e. there is the "S390EP" magic
>>>> from
>>>> -     * arch/s390/kernel/head.S here, then let's jump to the well-known
>>>> Linux
>>>> -     * kernel start address (when jumping to the PSW-at-zero address
>>>> instead,
>>>> -     * the kernel startup code fails when we booted from a network device).
>>>> -     */
>>>> -    if (!memcmp((char *)0x10008, "S390EP", 6)) {
>>>> -        jump_to_IPL_code(KERN_IMAGE_START);
>>>> -    }
>>>> -
>>>>        /* Trying to get PSW at zero address */
>>>>        if (*((uint64_t *)0) & RESET_PSW_MASK) {
>>>>            /*
>>>> @@ -92,6 +82,16 @@ void jump_to_low_kernel(void)
>>>>            jump_to_IPL_code(0);
>>>>        }
>>>>    +    /*
>>>> +     * If it looks like a Linux binary, i.e. there is the "S390EP" magic
>>>> from
>>>> +     * arch/s390/kernel/head.S here, then let's jump to the well-known
>>>> Linux
>>>> +     * kernel start address (when jumping to the PSW-at-zero address
>>>> instead,
>>>> +     * the kernel startup code fails when we booted from a network device).
>>>> +     */
>>>> +    if (!memcmp((char *)0x10008, "S390EP", 6)) {
>>>> +        jump_to_IPL_code(KERN_IMAGE_START);
>>>> +    }
>>>
>>> That feels a little bit dangerous ... I assume the order has been that way
>>> for a reason, e.g. I think we had to jump to KERN_IMAGE_START for some older
>>> versions of the Linux kernel since the startup code that was referenced by
>>> the PSW at address zero was not working in KVM...
>>
>> Makes sense.  It does seem like a precarious piece of code.
>>
>>>
>>> What do you think about this alternate idea instead: Clear the memory at
>>> location 0x10008 at the very beginning of the main() function (or maybe in
>>> boot_setup())?
>>
>> This seems to work too (I put it in boot_setup(), prior to call to
>> store_iplb()).
> 
> Great, can you send the patch before your holidays next week? (If you don't
> have enough time, that's ok, too, it's trivial enough, so I think I could
> write such a patch, too)

Will do.  I have it written up, just didn't want to send it last night 
and have surprises when I came in this morning. :)

> 
>   Thomas
> 

Re: [PATCH 1/2] pc-bios: s390x: Ensure Read IPL memory is clean

[Eric Farman]

On 11/20/20 3:26 AM, Janosch Frank wrote:
> On 11/19/20 5:57 PM, Eric Farman wrote:
>> If, for example, we boot off a virtio device and chreipl to a vfio-ccw
>> device, the space at lowcore will be non-zero. We build a Read IPL CCW
>> at address zero, but it will have leftover PSW data that will conflict
>> with the Format-0 CCW being generated:
>>
>> 0x0: 00080000 80010000
>>         ------ Ccw0.cda
>>                -- Ccw0.chainData
>>                  -- Reserved bits
>>
>> The data address will be overwritten with the correct value (0x0), but
>> the apparent data chain bit will cause subsequent memory to be used as
>> the target of the data store, which may not be where we expect (0x0).
>>
>> Clear out this space when we boot from DASD, so that we know it exists
>> exactly as we expect.
>>
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> Seems like I should have been more specific with my LGTM:
> Reviewed-by: Janosch Frank <frankja@de.ibm.com>

Thanks, Janosch. I didn't want to put words in your acronym. :)

> 
>> ---
>>   pc-bios/s390-ccw/dasd-ipl.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/pc-bios/s390-ccw/dasd-ipl.c b/pc-bios/s390-ccw/dasd-ipl.c
>> index 0fc879bb8e..71cbae2f16 100644
>> --- a/pc-bios/s390-ccw/dasd-ipl.c
>> +++ b/pc-bios/s390-ccw/dasd-ipl.c
>> @@ -100,6 +100,9 @@ static void make_readipl(void)
>>   {
>>       Ccw0 *ccwIplRead = (Ccw0 *)0x00;
>>   
>> +    /* Clear out any existing data */
>> +    memset(ccwIplRead, 0, sizeof(Ccw0));
>> +
>>       /* Create Read IPL ccw at address 0 */
>>       ccwIplRead->cmd_code = CCW_CMD_READ_IPL;
>>       ccwIplRead->cda = 0x00; /* Read into address 0x00 in main memory */
>>
>