Re: [PATCH v4 1/3] virtio-blk: store opt_io_size with correct size

[Roman Kagan <rvkagan@yandex-team.ru>]

On Wed, May 20, 2020 at 06:44:44AM -0400, Michael S. Tsirkin wrote:
> On Wed, May 20, 2020 at 11:06:55AM +0300, Roman Kagan wrote:
> > The width of opt_io_size in virtio_blk_topology is 32bit.
> > 
> > Use the appropriate accessor to store it.
> > 
> > Signed-off-by: Roman Kagan <rvkagan@yandex-team.ru>
> 
> 
> Thanks for the patch!
> Could you add a bit of analysis - when does this cause
> bugs? I'm guessing on BE systems with legacy virtio, right?

I guess so too.  It was found just by eye inspection, trying to figure
out the potential truncation of opt_io_size in virtio-blk and why it's
different from scsi.  I don't have any analysis to add :(

> Also, should we convert virtio_stw_p and friends to get the
> pointer to the correct value type, as opposed to void *?

I dunno.  I guess they were designed to be used with untyped buffers and
modeled after virtio_{st,ld}*_phys.  The same question applies to the
underlying {st,ld}_{b,l}e_p.

> This will catch bugs like this ...

I'll try and see if this change doesn't cause too much churn / pain.
But I suggest to decouple it from the simple patch at hand.

Thanks,
Roman.

> > ---
> > v4: new patch
> > 
> >  hw/block/virtio-blk.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > index f5f6fc925e..413083e62f 100644
> > --- a/hw/block/virtio-blk.c
> > +++ b/hw/block/virtio-blk.c
> > @@ -918,7 +918,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
> >      virtio_stw_p(vdev, &blkcfg.geometry.cylinders, conf->cyls);
> >      virtio_stl_p(vdev, &blkcfg.blk_size, blk_size);
> >      virtio_stw_p(vdev, &blkcfg.min_io_size, conf->min_io_size / blk_size);
> > -    virtio_stw_p(vdev, &blkcfg.opt_io_size, conf->opt_io_size / blk_size);
> > +    virtio_stl_p(vdev, &blkcfg.opt_io_size, conf->opt_io_size / blk_size);
> >      blkcfg.geometry.heads = conf->heads;
> >      /*
> >       * We must ensure that the block device capacity is a multiple of
> > -- 
> > 2.26.2
>