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

[Cornelia Huck <cohuck@redhat.com>]

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(-)
>