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

[Nathan Whitehorn <nwhitehorn@freebsd.org>]

On 01/02/14 10:31, Alexander Graf wrote:
> On 18.10.2013, at 14:33, Nathan Whitehorn <nwhitehorn@freebsd.org> 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 <nwhitehorn@freebsd.org>
> 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.

Will do. I'm traveling at the moment, so it may take a little while. If
you felt like fixing it yourself, I'd appreciate it.

>> ---
>> 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?

This is a duplicate of the logic (and code) in
scsi_target_emulate_report_luns() in scsi-bus.c. I tried to keep them as
similar as possible for maintenance reasons. LUN 0 doesn't actually even
need to exist to follow SAM-5, but QEMU seems to add it (and doing so is
non-harmful), so I've kept that. The whole code should probably be
refactored anyway to support things like hierarchical LUNs in the SCSI
core, which this SRP driver is trying to emulate for one level by
wrapping the SCSI core, but that's a larger project.

>> +
>> +    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
>
>

Thanks for the pointers here. I'm not too familiar with QEMU internals.
-Nathan