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

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

[Nathan Whitehorn]

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>
---
 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;
+
+    resp_data = malloc(len);
+    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);
+    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

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

[Nathan Whitehorn]

Any news on this? FreeBSD is unbootable from CDROM devices in 
QEMU/pseries without this patch.
-Nathan

On 10/18/13 07:33, Nathan Whitehorn 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>
> ---
>   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;
> +
> +    resp_data = malloc(len);
> +    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);
> +    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",

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

[Paolo Bonzini]

Il 02/12/2013 18:51, Nathan Whitehorn ha scritto:
> Any news on this? FreeBSD is unbootable from CDROM devices in
> QEMU/pseries without this patch.
> -Nathan

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Alex, can you pick it up?

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

[Nathan Whitehorn]

On 12/02/13 12:58, Paolo Bonzini wrote:
> Il 02/12/2013 18:51, Nathan Whitehorn ha scritto:
>> Any news on this? FreeBSD is unbootable from CDROM devices in
>> QEMU/pseries without this patch.
>> -Nathan
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
>
> Alex, can you pick it up?

Any updates?
-Nathan

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

[Alexander Graf]

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.

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

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

[Alexander Graf]

On 02.01.2014, at 16:31, Alexander Graf <agraf@suse.de> 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.
> 
>> ---
>> 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-devel] [Qemu-ppc] [PATCH] spapr_vscsi: Fix REPORT_LUNS handling

[ronnie sahlberg]

On Thu, Jan 2, 2014 at 7:41 AM, Alexander Graf <agraf@suse.de> wrote:
>
> On 02.01.2014, at 16:31, Alexander Graf <agraf@suse.de> 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.
>>
>>> ---
>>> 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.


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



>>> +        resp_data[i+1] = (dev->channel << 5);
>>> +        resp_data[i+1] |= dev->lun;
>
> What are the other 6 bytes reserved for?
>
>
> Alex
>
>

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

[Nathan Whitehorn]

On 01/02/14 10:56, ronnie sahlberg wrote:
> On Thu, Jan 2, 2014 at 7:41 AM, Alexander Graf <agraf@suse.de> wrote:
>> On 02.01.2014, at 16:31, Alexander Graf <agraf@suse.de> 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.
>>>
>>>> ---
>>>> 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

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

[Nathan Whitehorn]

On 01/02/14 10:41, Alexander Graf wrote:
> On 02.01.2014, at 16:31, Alexander Graf <agraf@suse.de> 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.
>>
>>> ---
>>> 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

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

[Nathan Whitehorn]

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

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

[Paolo Bonzini]

Il 02/01/2014 19:14, Nathan Whitehorn ha scritto:
>> > 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.

No need for that:

    .max_channel = 7, /* logical unit addressing format */
    .max_target = 63,
    .max_lun = 31,

dev->id is thus bounded to 0..63.

Paolo

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

[Paolo Bonzini]

Il 02/01/2014 19:23, Nathan Whitehorn ha scritto:
>>> 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 simply because you should not report lun 0 twice; even if it is
not defined, LUN 0 is there as a dummy device that only answers a
handful of commands (including INQUIRY and REPORT LUNS).  There are many
ways to write it, but unless you use GArray or something like that, it
will look very much like Nathan and hw/scsi/scsi-bus.c's code.

Paolo

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

[Nathan Whitehorn]

On 01/03/14 08:27, Paolo Bonzini wrote:
> Il 02/01/2014 19:23, Nathan Whitehorn ha scritto:
>>>> 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 simply because you should not report lun 0 twice; even if it is
> not defined, LUN 0 is there as a dummy device that only answers a
> handful of commands (including INQUIRY and REPORT LUNS).  There are many
> ways to write it, but unless you use GArray or something like that, it
> will look very much like Nathan and hw/scsi/scsi-bus.c's code.
>
> Paolo

I've sent a new version of this patch reflecting the discussion here.
Please let me know if I have missed anything.
-Nathan