Re: [Qemu-devel] [PATCH 2/5] virtio-blk: disable scsi passthrough for 1.0 device

[Kevin Wolf <kwolf@redhat.com>]

Am 13.07.2015 um 11:00 hat Jason Wang geschrieben:
> 
> 
> On 07/13/2015 03:46 PM, Michael S. Tsirkin wrote:
> > On Mon, Jul 13, 2015 at 01:46:48PM +0800, Jason Wang wrote:
> >> VIRTIO_BLK_F_SCSI was no longer supported in 1.0. So disable it.
> >>
> >> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> >> Cc: Kevin Wolf <kwolf@redhat.com>
> >> Cc: qemu-block@nongnu.org
> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> > Interesting, I noticed we have a field scsi - see
> > 	commit 1ba1f2e319afdcb485963cd3f426fdffd1b725f2
> > 	Author: Paolo Bonzini <pbonzini@redhat.com>
> > 	Date:   Fri Dec 23 15:39:03 2011 +0100
> >
> > 	    virtio-blk: refuse SG_IO requests with scsi=off
> >
> > but it doesn't seem to be propagated to guest features in
> > any way.
> >
> > Maybe we should fix that, making that flag AutoOnOff?
> 
> Looks ok but auto may need some compat work since default is true.
> 
> > Then, if user explicitly requested scsi=on with a modern
> > interface then we can error out cleanly.
> >
> > Given scsi flag is currently ignored, I think
> > this can be a patch on top.
> 
> Looks like virtio_blk_handle_scsi_req() check this:
> 
>     if (!blk->conf.scsi) {
>         status = VIRTIO_BLK_S_UNSUPP;
>         goto fail;
>     }

So we should be checking the same condition for the feature flag and
error out in the init function if we have a VERSION_1 device and
blk->conf.scsi is set.

> >> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> >> index 6aefda4..f30ad25 100644
> >> --- a/hw/block/virtio-blk.c
> >> +++ b/hw/block/virtio-blk.c
> >> @@ -730,7 +730,8 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features)
> >>      virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY);
> >>      virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY);
> >>      virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE);
> >> -    virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
> >> +    if (!__virtio_has_feature(features, VIRTIO_F_VERSION_1))
> >> +        virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);

Coding style: Braces are needed here. (Same in the other patch you CCed
me on).

Kevin