From patchwork Wed May 16 10:54:03 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paolo Bonzini X-Patchwork-Id: 159587 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id BDA6AB6FB4 for ; Wed, 16 May 2012 20:55:17 +1000 (EST) Received: from localhost ([::1]:43665 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SUbsx-00087W-Ha for incoming@patchwork.ozlabs.org; Wed, 16 May 2012 06:55:15 -0400 Received: from eggs.gnu.org ([208.118.235.92]:34764) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SUbsB-0006Kj-8A for qemu-devel@nongnu.org; Wed, 16 May 2012 06:54:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SUbs4-0001h7-1r for qemu-devel@nongnu.org; Wed, 16 May 2012 06:54:26 -0400 Received: from mail-pb0-f45.google.com ([209.85.160.45]:41551) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SUbs3-0001g1-PW for qemu-devel@nongnu.org; Wed, 16 May 2012 06:54:19 -0400 Received: by mail-pb0-f45.google.com with SMTP id ro12so1218249pbb.4 for ; Wed, 16 May 2012 03:54:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:from:to:cc:subject:date:message-id:x-mailer:in-reply-to :references; bh=GacbRuC1FB6CsV/r2pZkgFI+97AsWbsJuX+bk1HQrZ4=; b=kxDJxIaKqul8McGbx+sDNNvhm7AygNRzZ6Zvnwhzf8xIGd2GhmUf51/FgUvm6UoppC oIYOAgutbkkA9NlmiZLrjXGxwazq/5/ih1Lns4Z0MpaRZM9ksa/lSf8p8x9ppNf/1QpS gLTdh8kN80WvzaHvUz3CFQTnV4WmMKjgqG7KF+I8n0WuZSQ9Ww0kIVHDbz14gewjL0yi wYF5O6V4FF1EWdvG8R7uxEdJfHFAoQYuvPkd1woEwN0XEtpT1cfPUs2RO8UHH/j2OG9w ZMvQYsWoXj9zEFmOuIICTn0loYDXKQWpyXVPA6uiFZ6uMtH4n5499UdD5H5luhWrxSPQ KUwg== Received: by 10.68.233.102 with SMTP id tv6mr15282630pbc.153.1337165658732; Wed, 16 May 2012 03:54:18 -0700 (PDT) Received: from yakj.usersys.redhat.com (93-34-182-16.ip50.fastwebnet.it. [93.34.182.16]) by mx.google.com with ESMTPS id d2sm5088478pbw.39.2012.05.16.03.54.15 (version=TLSv1/SSLv3 cipher=OTHER); Wed, 16 May 2012 03:54:17 -0700 (PDT) From: Paolo Bonzini To: qemu-devel@nongnu.org Date: Wed, 16 May 2012 12:54:03 +0200 Message-Id: <1337165646-28118-2-git-send-email-pbonzini@redhat.com> X-Mailer: git-send-email 1.7.10.1 In-Reply-To: <1337165646-28118-1-git-send-email-pbonzini@redhat.com> References: <1337165646-28118-1-git-send-email-pbonzini@redhat.com> X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 209.85.160.45 Cc: aliguori@us.ibm.com, mst@redhat.com Subject: [Qemu-devel] [PATCH 1.1 1/4] virtio-blk: report non-zero status when failing SG_IO requests X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Linux really looks only at scsi->errors for SG_IO requests; it does not look at the virtio request status at all. Because of this, when a SG_IO request is failed early with virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP), without writing hdr.status, it will look like a success to the guest. This is their bug, but we can make it safe for older guests now by forcing scsi->errors to have a non-zero value whenever a request has to be failed. But if we fix the bug in the guest driver, we will have another problem because QEMU returns VIRTIO_BLK_S_IOERR if the status is non-zero, and Linux translates that to -EIO. Rather, the guest should succeed the request and pass the non-zero status via the userspace-provided SG_IO structure. So, remove the case where virtio_blk_handle_scsi can return VIRTIO_BLK_S_IOERR. Signed-off-by: Paolo Bonzini --- hw/virtio-blk.c | 51 +++++++++++++++++++++++---------------------------- 1 file changed, 23 insertions(+), 28 deletions(-) diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c index d4bb400..7148572 100644 --- a/hw/virtio-blk.c +++ b/hw/virtio-blk.c @@ -145,20 +145,12 @@ static VirtIOBlockReq *virtio_blk_get_request(VirtIOBlock *s) return req; } -#ifdef __linux__ static void virtio_blk_handle_scsi(VirtIOBlockReq *req) { - struct sg_io_hdr hdr; int ret; - int status; + int status = VIRTIO_BLK_S_OK; int i; - if ((req->dev->vdev.guest_features & (1 << VIRTIO_BLK_F_SCSI)) == 0) { - virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP); - g_free(req); - return; - } - /* * We require at least one output segment each for the virtio_blk_outhdr * and the SCSI command block. @@ -173,20 +165,26 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req) } /* - * No support for bidirection commands yet. + * The scsi inhdr is placed in the second-to-last input segment, just + * before the regular inhdr. */ - if (req->elem.out_num > 2 && req->elem.in_num > 3) { - virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP); - g_free(req); - return; + req->scsi = (void *)req->elem.in_sg[req->elem.in_num - 2].iov_base; + + if ((req->dev->vdev.guest_features & (1 << VIRTIO_BLK_F_SCSI)) == 0) { + status = VIRTIO_BLK_S_UNSUPP; + goto fail; } /* - * The scsi inhdr is placed in the second-to-last input segment, just - * before the regular inhdr. + * No support for bidirection commands yet. */ - req->scsi = (void *)req->elem.in_sg[req->elem.in_num - 2].iov_base; + if (req->elem.out_num > 2 && req->elem.in_num > 3) { + status = VIRTIO_BLK_S_UNSUPP; + goto fail; + } +#ifdef __linux__ + struct sg_io_hdr hdr; memset(&hdr, 0, sizeof(struct sg_io_hdr)); hdr.interface_id = 'S'; hdr.cmd_len = req->elem.out_sg[1].iov_len; @@ -230,12 +228,7 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req) ret = bdrv_ioctl(req->dev->bs, SG_IO, &hdr); if (ret) { status = VIRTIO_BLK_S_UNSUPP; - hdr.status = ret; - hdr.resid = hdr.dxfer_len; - } else if (hdr.status) { - status = VIRTIO_BLK_S_IOERR; - } else { - status = VIRTIO_BLK_S_OK; + goto fail; } /* @@ -258,14 +251,16 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req) virtio_blk_req_complete(req, status); g_free(req); -} #else -static void virtio_blk_handle_scsi(VirtIOBlockReq *req) -{ - virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP); + abort(); +#endif + +fail: + /* Just put anything nonzero so that the ioctl fails in the guest. */ + stl_p(&req->scsi->errors, 255); + virtio_blk_req_complete(req, status); g_free(req); } -#endif /* __linux__ */ typedef struct MultiReqBuffer { BlockRequest blkreq[32];