On 01/02/14 10:41, Alexander Graf wrote:
> On 02.01.2014, at 16:31, Alexander Graf <ag...@suse.de> wrote:
>
>> On 18.10.2013, at 14:33, Nathan Whitehorn <nwhiteh...@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 <nwhiteh...@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.
>>
>>> ---
>>> 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 ... :).
I've tried to maintain the existing (questionable) style here. See
vscsi_device_find() for instance. This seemed like a bad occasion for a
global style cleanup.
>>> + 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?
It's the hierarchical LUN fields. vscsi_device_find() defines the LUN
format used by this module if you are curious.
-Nathan
