Patchwork [RFC,3/6] scsi-generic: allow customization of the lun

login
register
mail settings
Submitter Paolo Bonzini
Date May 25, 2011, 3:20 p.m.
Message ID <4DDD1E5B.4010900@redhat.com>
Download mbox | patch
Permalink /patch/97366/
State New
Headers show

Comments

Paolo Bonzini - May 25, 2011, 3:20 p.m.
On 05/25/2011 03:10 PM, Christoph Hellwig wrote:
> On Fri, May 20, 2011 at 05:03:34PM +0200, Paolo Bonzini wrote:
>> This allows passthrough of devices with LUN != 0, by redirecting them to
>> LUN0 in the emulated target.
>
> I'm not quite sure what this code is for.  Each /dev/sg device reresents
> a LUN.  So if we want to suport multiple LUNs in qemu for devices that
> are backed by scsi-generic devices we need to take REPORT_LUNs emulation
> into the core scsi code, as any qemu target is completely independent
> of the underlying scsi device topology.

Yes, I did that.

The problem this patch solves is as follows.  scsi-generic rejects 
requests whose LUN does not match the host device's LUN.  However, since 
right now each scsi-disk and scsi-generic instance _must_ implement the 
whole target, this basically makes passthrough impossible when the 
device has a LUN that is not 0.

That said, this patch has now a completely different subject and meaning 
in my work branch.  I attach it FYI.

>> +    case INQUIRY:
>> +        if (req->lun != s->lun) {
>
> This seems odd.  I'd expect the SCSI core to handle the LUN addressing.
> For now that is just rejecting wrongs ones, and if multiple LUN
> support is added dispatching it to the correct drivers instance.

I agree.  This case of INQUIRY is needed because (for simplicity and 
backwards compatibility) you can hang a scsi-disk or scsi-generic device 
directly off the HBA, without the intermediate pseudo-device that 
handles dispatching commands and reporting LUNs.  In this case, the 
scsi-disk and scsi-generic devices see requests for other LUN than 
theirs.  In the case of INQUIRY and REQUEST SENSE, they must reply too.

A similar case happens with REPORT LUNS.  If a device's LUN is non-zero, 
the device will be attached to the intermediate pseudo-device, which 
will handle REPORT LUNS for it.  The simple REPORT LUNS code in 
scsi-disk and scsi-generic is only for the case in which the target has 
a single LUN.  That must be LUN 0 so the reply is hardcoded, and I'm 
asserting that the hardcoded reply is correct.  Note that I'm not 
asserting that the message is _sent_ to LUN 0; I'm asserting that the 
device itself is on LUN 0.

I thought about making REPORT LUNS really handled by the core, but it's 
a mess because I must put the answer in the buffer that scsi_req_get_buf 
will report.  The devices currently do not expose to the core a way to 
allocate that buffer, or to query its size.

Paolo
Christoph Hellwig - May 27, 2011, 1:04 p.m.
On Wed, May 25, 2011 at 05:20:59PM +0200, Paolo Bonzini wrote:
> I agree.  This case of INQUIRY is needed because (for simplicity and 
> backwards compatibility) you can hang a scsi-disk or scsi-generic device 
> directly off the HBA, without the intermediate pseudo-device that handles 
> dispatching commands and reporting LUNs.  In this case, the scsi-disk and 
> scsi-generic devices see requests for other LUN than theirs.  In the case 
> of INQUIRY and REQUEST SENSE, they must reply too.

Requiring this code in the scsi drivers is a really bad idea.  Not only
does it mean duplicating the implementation of REPORT LUNS and the illegal
LUN version of INQUIRY in every scsi LUN handler and the target driver,
but also an inconsitent topology of the qemu-internal objects representing
the SCSI implementation, which is a pretty clear path to all kinds of nast
bugs only showing up for the legacy case some time down the road.

The right way to solve this is to make sure we always have the proper
target object by creating it under the hood for the legacy case.
Paolo Bonzini - May 27, 2011, 1:31 p.m.
On 05/27/2011 03:04 PM, Christoph Hellwig wrote:
> Requiring this code in the scsi drivers is a really bad idea.  Not only
> does it mean duplicating the implementation of REPORT LUNS and the illegal
> LUN version of INQUIRY in every scsi LUN handler and the target driver,
> but also an inconsitent topology of the qemu-internal objects representing
> the SCSI implementation, which is a pretty clear path to all kinds of nast
> bugs only showing up for the legacy case some time down the road.
>
> The right way to solve this is to make sure we always have the proper
> target object by creating it under the hood for the legacy case.

I know, but this requires changes to the basic qdev layer so I planned 
to do this later.  Also because for bisectability, to avoid dropping a 
huge patch, I first need to work around the legacy cases and then kill them.

Paolo

Patch

From 925ced58d6b4a3163e720f5a6a7f9fda12f1f6eb Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Tue, 19 Apr 2011 07:34:07 +0200
Subject: [PATCH] scsi-generic: fix passthrough of devices with LUN != 0

This allows redirecting them to LUN0 in the emulated target.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-generic.c |   39 +++++++++++++++++++++++++++++++++------
 1 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c
index 94aca18..1611023 100644
--- a/hw/scsi-generic.c
+++ b/hw/scsi-generic.c
@@ -60,7 +60,6 @@  struct SCSIGenericState
 {
     SCSIDevice qdev;
     BlockDriverState *bs;
-    int lun;
     int driver_status;
     uint8_t sensebuf[SCSI_SENSE_BUF_SIZE];
     uint8_t senselen;
@@ -232,8 +231,11 @@  static void scsi_read_data(SCSIRequest *req)
         return;
     }
 
-    if (r->req.cmd.buf[0] == REQUEST_SENSE && s->driver_status & SG_ERR_DRIVER_SENSE)
-    {
+    switch (r->req.cmd.buf[0]) {
+    case REQUEST_SENSE:
+        if (!(s->driver_status & SG_ERR_DRIVER_SENSE)) {
+            break;
+        }
         s->senselen = MIN(r->len, s->senselen);
         memcpy(r->buf, s->sensebuf, s->senselen);
         r->io_header.driver_status = 0;
@@ -248,6 +250,32 @@  static void scsi_read_data(SCSIRequest *req)
         /* Clear sensebuf after REQUEST_SENSE */
         scsi_clear_sense(s);
         return;
+
+    case REPORT_LUNS:
+	assert(!s->qdev.lun);
+        if (r->req.cmd.xfer < 16) {
+            scsi_command_complete(r, -EINVAL);
+            return;
+        }
+        r->io_header.driver_status = 0;
+        r->io_header.status = 0;
+        r->io_header.dxfer_len  = 16;
+        r->len = -1;
+        r->buf[3] = 8;
+        scsi_req_data(&r->req, 16);
+        scsi_command_complete(r, 0);
+        return;
+
+    case INQUIRY:
+        if (req->lun != s->qdev.lun) {
+            if (r->req.cmd.xfer < 1) {
+                scsi_command_complete(r, -EINVAL);
+                return;
+            }
+            r->buf[0] = 0x7f;
+            return;
+        }
+        break;
     }
 
     ret = execute_command(s->bs, r, SG_DXFER_FROM_DEV, scsi_read_complete);
@@ -338,7 +366,8 @@  static int32_t scsi_send_command(SCSIRequest *req, uint8_t *cmd)
     SCSIGenericReq *r = DO_UPCAST(SCSIGenericReq, req, req);
     int ret;
 
-    if (cmd[0] != REQUEST_SENSE && req->lun != s->lun) {
+    if (cmd[0] != REQUEST_SENSE && cmd[0] != INQUIRY &&
+	req->lun != s->qdev.lun) {
         DPRINTF("Unimplemented LUN %d\n", req->lun);
         scsi_set_sense(s, SENSE_CODE(LUN_NOT_SUPPORTED));
         r->req.status = CHECK_CONDITION;
@@ -505,8 +534,6 @@  static int scsi_generic_initfn(SCSIDevice *dev)
     }
 
     /* define device state */
-    s->lun = scsiid.lun;
-    DPRINTF("LUN %d\n", s->lun);
     s->qdev.type = scsiid.scsi_type;
     DPRINTF("device type %d\n", s->qdev.type);
     if (s->qdev.type == TYPE_TAPE) {
-- 
1.7.4.4