From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51677) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VymnT-000484-5S for qemu-devel@nongnu.org; Thu, 02 Jan 2014 13:15:12 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VymnN-0001kf-P1 for qemu-devel@nongnu.org; Thu, 02 Jan 2014 13:15:07 -0500 MIME-version: 1.0 Content-transfer-encoding: 7BIT Content-type: text/plain; CHARSET=US-ASCII Message-id: <52C5AC9E.5060906@freebsd.org> Date: Thu, 02 Jan 2014 13:14:54 -0500 From: Nathan Whitehorn References: <1382099622-87967-1-git-send-email-nwhitehorn@freebsd.org> <46A0A8D3-7588-4C43-9137-67AE089FAEAB@suse.de> In-reply-to: Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH] spapr_vscsi: Fix REPORT_LUNS handling List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: ronnie sahlberg , Alexander Graf Cc: Paolo Bonzini , qemu-ppc , QEMU Developers On 01/02/14 10:56, ronnie sahlberg wrote: > On Thu, Jan 2, 2014 at 7:41 AM, Alexander Graf wrote: >> On 02.01.2014, at 16:31, Alexander Graf wrote: >> >>> On 18.10.2013, at 14: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 >>> 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