On Thu, Oct 14, 2021 at 10:45:25AM +0800, Jason Wang wrote:
> On Wed, Oct 13, 2021 at 8:46 PM Michael S. Tsirkin <mst@xxxxxxxxxx> wrote:
> >
> > It turns out that access to config space before completing the feature
> > negotiation is broken for big endian guests at least with QEMU hosts up
> > to 6.1 inclusive. This affects any device that accesses config space in
> > the validate callback: at the moment that is virtio-net with
> > VIRTIO_NET_F_MTU but since 82e89ea077b9 ("virtio-blk: Add validation for
> > block size in config space") that also started affecting virtio-blk with
> > VIRTIO_BLK_F_BLK_SIZE.
>
> Ok, so it looks like I need to do something similar in my series to
> validate max_nr_ports in probe() instead of validate()? (multi port is
> not enabled by default).
I think what we should do is fix upstream QEMU first. With that, plus
Halil's work-around writing out the version 1 feature, we should be
good reading config in validate.
>
> I wonder if we need to document this and rename the validate to
> validate_features() (since we can do very little thing if we can't
> read config in .validate()).
That's not a bad idea anyway - any validation that does not
need to clear features can be done in probe in any case.
> > Further, unlike VIRTIO_NET_F_MTU which is off by
> > default on QEMU, VIRTIO_BLK_F_BLK_SIZE is on by default, which resulted
> > in lots of people not being able to boot VMs on BE.
> >
> > The spec is very clear that what we are doing is legal so QEMU needs to
> > be fixed, but given it's been broken for so many years and no one
> > noticed, we need to give QEMU a bit more time before applying this.
> >
> > Further, this patch is incomplete (does not check blk size is a power
> > of two) and it duplicates the logic from nbd.
> >
> > Revert for now, and we'll reapply a cleaner logic in the next release.
> >
> > Cc: stable@xxxxxxxxxxxxxxx
> > Fixes: 82e89ea077b9 ("virtio-blk: Add validation for block size in config space")
> > Cc: Xie Yongji <xieyongji@xxxxxxxxxxxxx>
> > Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
>
> Acked-by: jason Wang <jasowang@xxxxxxxxxx>
>
> > ---
> > drivers/block/virtio_blk.c | 37 ++++++-------------------------------
> > 1 file changed, 6 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > index 9b3bd083b411..303caf2d17d0 100644
> > --- a/drivers/block/virtio_blk.c
> > +++ b/drivers/block/virtio_blk.c
> > @@ -689,28 +689,6 @@ static const struct blk_mq_ops virtio_mq_ops = {
> > static unsigned int virtblk_queue_depth;
> > module_param_named(queue_depth, virtblk_queue_depth, uint, 0444);
> >
> > -static int virtblk_validate(struct virtio_device *vdev)
> > -{
> > - u32 blk_size;
> > -
> > - if (!vdev->config->get) {
> > - dev_err(&vdev->dev, "%s failure: config access disabled\n",
> > - __func__);
> > - return -EINVAL;
> > - }
> > -
> > - if (!virtio_has_feature(vdev, VIRTIO_BLK_F_BLK_SIZE))
> > - return 0;
> > -
> > - blk_size = virtio_cread32(vdev,
> > - offsetof(struct virtio_blk_config, blk_size));
> > -
> > - if (blk_size < SECTOR_SIZE || blk_size > PAGE_SIZE)
> > - __virtio_clear_bit(vdev, VIRTIO_BLK_F_BLK_SIZE);
> > -
> > - return 0;
> > -}
> > -
> > static int virtblk_probe(struct virtio_device *vdev)
> > {
> > struct virtio_blk *vblk;
> > @@ -722,6 +700,12 @@ static int virtblk_probe(struct virtio_device *vdev)
> > u8 physical_block_exp, alignment_offset;
> > unsigned int queue_depth;
> >
> > + if (!vdev->config->get) {
> > + dev_err(&vdev->dev, "%s failure: config access disabled\n",
> > + __func__);
> > + return -EINVAL;
> > + }
> > +
> > err = ida_simple_get(&vd_index_ida, 0, minor_to_index(1 << MINORBITS),
> > GFP_KERNEL);
> > if (err < 0)
> > @@ -836,14 +820,6 @@ static int virtblk_probe(struct virtio_device *vdev)
> > else
> > blk_size = queue_logical_block_size(q);
> >
> > - if (blk_size < SECTOR_SIZE || blk_size > PAGE_SIZE) {
> > - dev_err(&vdev->dev,
> > - "block size is changed unexpectedly, now is %u\n",
> > - blk_size);
> > - err = -EINVAL;
> > - goto out_cleanup_disk;
> > - }
> > -
> > /* Use topology information if available */
> > err = virtio_cread_feature(vdev, VIRTIO_BLK_F_TOPOLOGY,
> > struct virtio_blk_config, physical_block_exp,
> > @@ -1009,7 +985,6 @@ static struct virtio_driver virtio_blk = {
> > .driver.name = KBUILD_MODNAME,
> > .driver.owner = THIS_MODULE,
> > .id_table = id_table,
> > - .validate = virtblk_validate,
> > .probe = virtblk_probe,
> > .remove = virtblk_remove,
> > .config_changed = virtblk_config_changed,
> > --
> > MST
> >
