On Wed, 8 Oct 2025 23:23:17 +0200 Heinrich Schuchardt <[email protected]> wrote:
Hi Heinrich, thanks for the quick reply! > On 10/8/25 17:39, Andre Przywara wrote: > > On Sat, 30 Aug 2025 22:39:54 +0200 > > Heinrich Schuchardt <[email protected]> wrote: > > > > Hi Heinrich, > > > >> QEMU allows to specify the logical block size via parameter > >> logical_block_size of a virtio-blk-device. > >> > >> The communication channel via virtqueues remains based on 512 byte blocks > >> even if the logical_block_size is larger. > >> > >> Consider the logical block size in the block device driver. > >> > >> Reported-by: Emil Renner Berthing <[email protected]> > >> Signed-off-by: Heinrich Schuchardt <[email protected]> > >> --- > >> v2: > >> remove superfluous include linux/err.h > >> --- > >> drivers/virtio/virtio_blk.c | 25 ++++++++++++++++++++++--- > >> 1 file changed, 22 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/virtio/virtio_blk.c b/drivers/virtio/virtio_blk.c > >> index 2f999fc8bbe..4224e3c17f4 100644 > >> --- a/drivers/virtio/virtio_blk.c > >> +++ b/drivers/virtio/virtio_blk.c > >> @@ -12,10 +12,17 @@ > >> #include <virtio_types.h> > >> #include <virtio.h> > >> #include <virtio_ring.h> > >> +#include <linux/log2.h> > >> #include "virtio_blk.h" > >> > >> +/** > >> + * struct virtio_blk_priv - private date for virtio block device > >> + */ > >> struct virtio_blk_priv { > >> + /** @virtqueue - virtqueue to process */ > >> struct virtqueue *vq; > >> + /** @blksz_shift - log2 of block size divided by 512 */ > >> + u32 blksz_shift; > >> }; > >> > >> static const u32 feature[] = { > >> @@ -71,6 +78,8 @@ static ulong virtio_blk_do_req(struct udevice *dev, u64 > >> sector, > >> u8 status; > >> int ret; > >> > >> + sector <<= priv->blksz_shift; > >> + blkcnt <<= priv->blksz_shift; > >> virtio_blk_init_header_sg(dev, sector, type, &out_hdr, &hdr_sg); > >> sgs[num_out++] = &hdr_sg; > >> > >> @@ -109,7 +118,7 @@ static ulong virtio_blk_do_req(struct udevice *dev, > >> u64 sector, > >> ; > >> log_debug("done\n"); > >> > >> - return status == VIRTIO_BLK_S_OK ? blkcnt : -EIO; > >> + return status == VIRTIO_BLK_S_OK ? blkcnt >> priv->blksz_shift : -EIO; > >> } > >> > >> static ulong virtio_blk_read(struct udevice *dev, lbaint_t start, > >> @@ -177,15 +186,25 @@ static int virtio_blk_probe(struct udevice *dev) > >> struct blk_desc *desc = dev_get_uclass_plat(dev); > >> u64 cap; > >> int ret; > >> + u32 blk_size; > >> > >> ret = virtio_find_vqs(dev, 1, &priv->vq); > >> if (ret) > >> return ret; > >> > >> - desc->blksz = 512; > >> - desc->log2blksz = 9; > >> virtio_cread(dev, struct virtio_blk_config, capacity, &cap); > >> desc->lba = cap; > >> + if (!virtio_has_feature(dev, VIRTIO_BLK_F_BLK_SIZE)) { > > > > Is that actually the right condition check? I would assume that we only > > query the blk_size field if the feature bit is *set*, not when it's > > cleared? > > We see a failure (virtio block device not registering), because blk_size is > > 0 on the Arm FVP fast model, which is fine I think because the feature bit > > is clear as well. > > > > Cheers, > > Andre > > Hello Andre, > > Thanks for sharing your test results. > > Looking at these QEMU lines, I think the evaluated feature flag is correct: > > pc-bios/s390-ccw/virtio.h:163: > uint32_t blk_size; /* block size of device (if VIRTIO_BLK_F_BLK_SIZE) */ > > include/standard-headers/linux/virtio_blk.h:72-73: > /* block size of device (if VIRTIO_BLK_F_BLK_SIZE) */ > __virtio32 blk_size; > > virtio_has_feature() cannot detect a feature if is not in the feature > set of the U-Boot driver. See function virtio_uclass_child_pre_probe(): > > uc_priv->features = driver_features_legacy & device_features; > > VIRTIO_BLK_F_BLK_SIZE is missing in the feature set of virtio_blk. > The not in front of virtio_has_feature() is wrong. Ah, indeed a double whammy: the feature bit was filtered, AND the logic twisted, that's why it worked. I applied your patch, and tested with the FVP and with QEMU, trying both block sizes there: seems to work fine now. Are you going to send a proper patch (since you have it already)? If so, add: Reported-by: Debbie Horsfall <[email protected]>, because she found the issue. Thanks! Andre > > Could you, please, retest with this diff: > > diff --git a/drivers/virtio/virtio_blk.c b/drivers/virtio/virtio_blk.c > index 4224e3c17f4..ec2e28b54f7 100644 > --- a/drivers/virtio/virtio_blk.c > +++ b/drivers/virtio/virtio_blk.c > @@ -26,6 +26,7 @@ struct virtio_blk_priv { > }; > > static const u32 feature[] = { > + VIRTIO_BLK_F_BLK_SIZE, > VIRTIO_BLK_F_WRITE_ZEROES > }; > > @@ -194,7 +195,7 @@ static int virtio_blk_probe(struct udevice *dev) > > virtio_cread(dev, struct virtio_blk_config, capacity, &cap); > desc->lba = cap; > - if (!virtio_has_feature(dev, VIRTIO_BLK_F_BLK_SIZE)) { > + if (virtio_has_feature(dev, VIRTIO_BLK_F_BLK_SIZE)) { > virtio_cread(dev, struct virtio_blk_config, blk_size, > &blk_size); > desc->blksz = blk_size; > if (!is_power_of_2(blk_size) || desc->blksz < 512) > > Best regards > > Heinrich > > > > >> + virtio_cread(dev, struct virtio_blk_config, blk_size, > >> &blk_size); > >> + desc->blksz = blk_size; > >> + if (!is_power_of_2(blk_size) || desc->blksz < 512) > >> + return -EIO; > >> + } else { > >> + desc->blksz = 512; > >> + } > >> + desc->log2blksz = LOG2(desc->blksz); > >> + priv->blksz_shift = desc->log2blksz - 9; > >> + desc->lba >>= priv->blksz_shift; > >> > >> return 0; > >> } > > >
