Re: [PATCH 03/12] pc-bios/s390-ccw/bootmap: Improve the guessing logic in zipl_load_vblk()

[Eric Farman]

On Tue, 2022-06-28 at 15:10 +0200, Thomas Huth wrote:
> The logic of trying an final ISO or ECKD boot on virtio-block devices
> is
> very weird: Since the geometry hardly ever matches in
> virtio_disk_is_scsi(),
> virtio_blk_setup_device() always sets a "guessed" disk geometry via
> virtio_assume_scsi() (which is certainly also wrong in a lot of
> cases).
> 
> zipl_load_vblk() then sees that there's been a
> "virtio_guessed_disk_nature"
> and tries to fix up the geometry again via virtio_assume_iso9660()
> before
> always trying to do ipl_iso_el_torito(). That's a very brain-twisting
> way of attempting to boot from ISO images, which won't work anymore
> after
> the following patches that will clean up the virtio_assume_scsi()
> mess
> (and thus get rid of the "virtio_guessed_disk_nature" here).
> 
> Let's try a better approach instead: ISO files always have a magic
> string "CD001" at offset 0x8001 (see e.g. the ECMA-119 specification)
> which we can use to decide whether we should try to boot in ISO 9660
> mode (which we should also try if we see a sector size of 2048).
> 
> And if we were not able to boot in ISO mode here, the final boot
> attempt
> before panicking is to boot in ECKD mode. Since this is our last boot
> attempt anyway, simply always assume the ECKD geometry here (if the
> sector
> size was not 4096 yet), so that we also do not depend on the guessed
> disk
> geometry from virtio_blk_setup_device() here anymore.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  pc-bios/s390-ccw/bootmap.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
> index 56411ab3b6..3181b05382 100644
> --- a/pc-bios/s390-ccw/bootmap.c
> +++ b/pc-bios/s390-ccw/bootmap.c
> @@ -780,18 +780,35 @@ static void ipl_iso_el_torito(void)
>      }
>  }
>  
> +/**
> + * Detect whether we're trying to boot from an .ISO image.
> + * These always have a signature string "CD001" at offset 0x8001.
> + */
> +static bool has_iso_signature(void)
> +{
> +    if (virtio_read(0x8000 / virtio_get_block_size(), sec)) {

Certainly unlikely, but virtio_get_block_size is able to return zero.

> +        return false;
> +    }
> +
> +    return !memcmp("CD001", &sec[1], 5);
> +}
> +
>  /*******************************************************************
> ****
>   * Bus specific IPL sequences
>   */
>  
>  static void zipl_load_vblk(void)
>  {
> -    if (virtio_guessed_disk_nature()) {
> -        virtio_assume_iso9660();
> +    int blksize = virtio_get_block_size();
> +
> +    if (blksize == VIRTIO_ISO_BLOCK_SIZE || has_iso_signature()) {
> +        if (blksize != VIRTIO_ISO_BLOCK_SIZE) {
> +            virtio_assume_iso9660();
> +        }
> +        ipl_iso_el_torito();
>      }
> -    ipl_iso_el_torito();
>  
> -    if (virtio_guessed_disk_nature()) {
> +    if (blksize != VIRTIO_DASD_BLOCK_SIZE) {
>          sclp_print("Using guessed DASD geometry.\n");
>          virtio_assume_eckd();
>      }