| Message ID | 1382099622-87967-1-git-send-email-nwhitehorn@freebsd.org |
|---|---|
| State | New |
| Headers | show |
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",
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?
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
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 > >
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
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 > >
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
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
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
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
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
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
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",
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(+)