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

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

[Thomas Huth]

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

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

diff --git a/pc-bios/s390-ccw/virtio-blkdev.c b/pc-bios/s390-ccw/virtio-blkdev.c
index 8271c47296..794f99b42c 100644
--- a/pc-bios/s390-ccw/virtio-blkdev.c
+++ b/pc-bios/s390-ccw/virtio-blkdev.c
@@ -173,7 +173,7 @@ int virtio_get_block_size(void)
 
     switch (vdev->senseid.cu_model) {
     case VIRTIO_ID_BLOCK:
-        return vdev->config.blk.blk_size << vdev->config.blk.physical_block_exp;
+        return vdev->config.blk.blk_size;
     case VIRTIO_ID_SCSI:
         return vdev->scsi_block_size;
     }
-- 
2.31.1

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

[Cornelia Huck]

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>

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

[Eric Farman]

On Fri, 2022-08-05 at 12:14 +0200, Cornelia Huck wrote:
> 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 :)

Same here. But Thomas' recent changes are good medicine. :)

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

Reviewed-by: Eric Farman <farman@linux.ibm.com>