On 01/02/14 10:56, ronnie sahlberg wrote:
> On Thu, Jan 2, 2014 at 7:41 AM, Alexander Graf <ag...@suse.de> 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);
> This looks odd.
> Shouldn't this rather be
> resp_data[i] = (1 << 6);
> to set the LUN address method to 01b meaning Single Level LUN structure.
> (SAM5 4.7.3)
>
> He is setting the address method to 10b but there is no such address
> method afaik
There is. This uses the "Logical unit addressing" method of SAM5 section
4.7.7.4, following the rest of the code in this module.
>
>> 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;
> He should do something like :
> resp_data[i] |= dev->id & 0x3f;
> here to avoid a dev->id > 63 from spilling into the address method field.
>
> Or probably should have a check for
> if dev->id > 3 then fail
OK.
>
>>>> + resp_data[i+1] = (dev->channel << 5);
>>>> + resp_data[i+1] |= dev->lun;
>> What are the other 6 bytes reserved for?
>>
>>
>> Alex
>>
>>
SRP, and so VSCSI, uses hierarchical LUNs. QEMU doesn't actually
implement that, however, so the hierarchy only has one level -- the
remaining 6 bytes are therefore zero.
-Nathan
