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 ... :). >> + 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