[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [Qemu-devel] [PATCH] spapr_vscsi: Fix REPORT_LUNS handlin
From: |
Alexander Graf |
Subject: |
Re: [Qemu-ppc] [Qemu-devel] [PATCH] spapr_vscsi: Fix REPORT_LUNS handling |
Date: |
Thu, 2 Jan 2014 16:41:18 +0100 |
On 02.01.2014, at 16:31, Alexander Graf <address@hidden> wrote:
>
> 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);
Ah, I almost forgot this one. Please convert that into something more verbose
through a define. Whatever that bit means ... :).
>> + resp_data[i] |= dev->id;
>> + resp_data[i+1] = (dev->channel << 5);
>> + resp_data[i+1] |= dev->lun;
What are the other 6 bytes reserved for?
Alex
Re: [Qemu-ppc] [Qemu-devel] [PATCH] spapr_vscsi: Fix REPORT_LUNS handling, Nathan Whitehorn, 2014/01/02