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

Eric Farman posted 2 patches 3 years, 5 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20201119165729.63351-1-farman@linux.ibm.com
Maintainers: Christian Borntraeger <borntraeger@de.ibm.com>, Cornelia Huck <cohuck@redhat.com>, Thomas Huth <thuth@redhat.com>
There is a newer version of this series
pc-bios/s390-ccw/dasd-ipl.c |  3 +++
pc-bios/s390-ccw/jump2ipl.c | 20 ++++++++++----------
2 files changed, 13 insertions(+), 10 deletions(-)
[PATCH 0/2] pc-bios/s390 fixes for reboot-to-vfio-ccw
Posted by Eric Farman 3 years, 5 months ago
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


Re: [PATCH 0/2] pc-bios/s390 fixes for reboot-to-vfio-ccw
Posted by Cornelia Huck 3 years, 5 months ago
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(-)
>