Re: [PATCH] pc-bios/s390-ccw: Fix booting with logical block size < physical block size

[Cornelia Huck <cohuck@redhat.com>]

On Fri, Aug 05 2022, Thomas Huth <thuth@redhat.com> wrote:

> For accessing single blocks during boot, it's the logical block size that
> matters. (Physical block sizes are rather interesting e.g. for creating
> file systems with the correct alignment for speed reasons etc.).
> So the s390-ccw bios has to use the logical block size for calculating
> sector numbers during the boot phase, the "physical_block_exp" shift
> value must not be taken into account. This change fixes the boot process
> when the guest hast been installed on a disk where the logical block size
> differs from the physical one, e.g. if the guest has been installed
> like this:
>
>  qemu-system-s390x -nographic -accel kvm -m 2G \
>   -drive if=none,id=d1,file=fedora.iso,format=raw,media=cdrom \
>   -device virtio-scsi -device scsi-cd,drive=d1 \
>   -drive if=none,id=d2,file=test.qcow2,format=qcow2
>   -device virtio-blk,drive=d2,physical_block_size=4096,logical_block_size=512
>
> Linux correctly uses the logical block size of 512 for the installation,
> but the s390-ccw bios tries to boot from a disk with 4096 block size so
> far, as long as this patch has not been applied yet (well, it used to work
> by accident in the past due to the virtio_assume_scsi() hack that used to
> enforce 512 byte sectors on all virtio-block disks, but that hack has been
> well removed in commit 5447de2619050a0a4d to fix other scenarios).

I wonder whether there's more stuff lurking in there; the old code seems
to have "worked" in many cases by accident, and cleaning up things might
expose more odd code. Generally, reading ccw bios code gives me a
headache :)

>
> Fixes: 5447de2619 ("pc-bios/s390-ccw/virtio-blkdev: Remove virtio_assume_scsi()")
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2112303
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  pc-bios/s390-ccw/virtio-blkdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Looks sane to me.

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