Re: [Qemu-devel] [PATCH] spapr_vscsi: Fix REPORT_LUNS handling

[Alexander Graf]

On 18.10.2013, at 14:33, Nathan Whitehorn <address@hidden> wrote:

> Intercept REPORT_LUNS commands addressed either to SRP LUN 0 or the well-known
> LUN for REPORT_LUNS commands. This is required to implement the SAM and SPC
> specifications.
> 
> Since SRP implements only a single SCSI target port per connection, the SRP
> target is required to report all available LUNs in response to a REPORT_LUNS
> command addressed either to LUN 0 or the well-known LUN. Instead, QEMU was
> forwarding such requests to the first QEMU SCSI target, with the result that
> initiators that relied on this feature would only see LUNs on the first QEMU
> SCSI target.
> 
> Behavior for REPORT_LUNS commands addressed to any other LUN is not specified
> by the standard and so is left unchanged. This preserves behavior under Linux
> and SLOF, which enumerate possible LUNs by hand and so address no commands
> either to LUN 0 or the well-known REPORT_LUNS LUN.
> 
> Signed-off-by: Nathan Whitehorn <address@hidden>

This patch fails on checkpatch.pl. Please fix those warnings up :).

WARNING: braces {} are necessary for all arms of this statement
#65: FILE: hw/scsi/spapr_vscsi.c:738:
+        if (dev->channel == 0 && dev->id == 0 && dev->lun == 0)
[...]

WARNING: braces {} are necessary for all arms of this statement
#81: FILE: hw/scsi/spapr_vscsi.c:754:
+        if (dev->id == 0 && dev->channel == 0)
[...]
+        else
[...]

WARNING: line over 80 characters
#108: FILE: hw/scsi/spapr_vscsi.c:781:
+    if ((srp->cmd.lun == 0 || be64_to_cpu(srp->cmd.lun) == 
SRP_REPORT_LUNS_WLUN)      && srp->cmd.cdb[0] == REPORT_LUNS) {

total: 0 errors, 3 warnings, 75 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

> ---
> hw/scsi/spapr_vscsi.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 57 insertions(+)
> 
> diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
> index 2a26042..87e0fb3 100644
> --- a/hw/scsi/spapr_vscsi.c
> +++ b/hw/scsi/spapr_vscsi.c
> @@ -63,6 +63,8 @@
> #define SCSI_SENSE_BUF_SIZE     96
> #define SRP_RSP_SENSE_DATA_LEN  18
> 
> +#define SRP_REPORT_LUNS_WLUN    0xc10100000000000
> +
> typedef union vscsi_crq {
>     struct viosrp_crq s;
>     uint8_t raw[16];
> @@ -720,12 +722,67 @@ static void vscsi_inquiry_no_target(VSCSIState *s, 
> vscsi_req *req)
>     }
> }
> 
> +static void vscsi_report_luns(VSCSIState *s, vscsi_req *req)
> +{
> +    BusChild *kid;
> +    int i, len, n, rc;
> +    uint8_t *resp_data;
> +    bool found_lun0;
> +
> +    n = 0;
> +    found_lun0 = false;
> +    QTAILQ_FOREACH(kid, &s->bus.qbus.children, sibling) {
> +        SCSIDevice *dev = SCSI_DEVICE(kid->child);
> +
> +        n += 8;
> +        if (dev->channel == 0 && dev->id == 0 && dev->lun == 0)
> +            found_lun0 = true;
> +    }
> +    if (!found_lun0) {
> +        n += 8;
> +    }
> +    len = n+8;

Let me try to grasp what you're doing here. You're trying to figure out how 
many devices there are attached to the bus. For every device you reserve a 
buffer block. Lun0 is mandatory, all others are optional.

First off, I think the code would be easier to grasp if you'd count "number of 
entries" rather than "number of bytes". That way we don't have to mentally deal 
with the 8 byte block granularity.

Then IIUC you're jumping through a lot of hoops to count lun0 if it's there, 
but keep it reserved when it's not there. Why don't you just always reserve 
entry 0 for lun0? In the loop where you're actually filling in data you just 
skip lun0. Or is lun0 a terminator and always has to come last?


> +
> +    resp_data = malloc(len);

g_malloc0

> +    memset(resp_data, 0, len);
> +    stl_be_p(resp_data, n);
> +    i = found_lun0 ? 8 : 16;
> +    QTAILQ_FOREACH(kid, &s->bus.qbus.children, sibling) {
> +        DeviceState *qdev = kid->child;
> +        SCSIDevice *dev = SCSI_DEVICE(qdev);
> +
> +        if (dev->id == 0 && dev->channel == 0)
> +            resp_data[i] = 0;
> +        else
> +            resp_data[i] = (2 << 6);
> +        resp_data[i] |= dev->id;
> +        resp_data[i+1] = (dev->channel << 5);
> +        resp_data[i+1] |= dev->lun;
> +        i += 8;
> +    }
> +
> +    vscsi_preprocess_desc(req);
> +    rc = vscsi_srp_transfer_data(s, req, 0, resp_data, len);
> +    free(resp_data);

g_free


Alex

> +    if (rc < 0) {
> +        vscsi_makeup_sense(s, req, HARDWARE_ERROR, 0, 0);
> +        vscsi_send_rsp(s, req, CHECK_CONDITION, 0, 0);
> +    } else {
> +        vscsi_send_rsp(s, req, 0, len - rc, 0);
> +    }
> +}
> +
> static int vscsi_queue_cmd(VSCSIState *s, vscsi_req *req)
> {
>     union srp_iu *srp = &req->iu.srp;
>     SCSIDevice *sdev;
>     int n, lun;
> 
> +    if ((srp->cmd.lun == 0 || be64_to_cpu(srp->cmd.lun) == 
> SRP_REPORT_LUNS_WLUN)      && srp->cmd.cdb[0] == REPORT_LUNS) {
> +        vscsi_report_luns(s, req);
> +        return 0;
> +    }
> +
>     sdev = vscsi_device_find(&s->bus, be64_to_cpu(srp->cmd.lun), &lun);
>     if (!sdev) {
>         DPRINTF("VSCSI: Command for lun %08" PRIx64 " with no drive\n",
> -- 
> 1.8.4
> 
>