From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:40505) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RlkQg-0001TO-Hq for qemu-devel@nongnu.org; Fri, 13 Jan 2012 11:56:43 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RlkQa-0001v3-1E for qemu-devel@nongnu.org; Fri, 13 Jan 2012 11:56:38 -0500 Received: from cantor2.suse.de ([195.135.220.15]:38436 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RlkQZ-0001th-O0 for qemu-devel@nongnu.org; Fri, 13 Jan 2012 11:56:31 -0500 Message-ID: <4F1061CE.50005@suse.de> Date: Fri, 13 Jan 2012 17:54:38 +0100 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1326453552-17189-1-git-send-email-hare@suse.de> In-Reply-To: <1326453552-17189-1-git-send-email-hare@suse.de> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH][v9] megasas: LSI Megaraid SAS HBA emulation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Hannes Reinecke Cc: Avi Kivity , qemu-devel@nongnu.org, Stefan Hajnoczi , Alexander Graf Am 13.01.2012 12:19, schrieb Hannes Reinecke: > This patch adds an emulation for the LSI Megaraid SAS 8708EM2 HBA. > I've tested it to work with Linux, Windows Vista, and Windows7. >=20 > Changes since v8: > - Remove 'disable' keyword from trace definitions > - Convert hand-crafted debugging statements with trace > definitions > - Treat 'context' tag as little endian >=20 > Changes since v7: > - Port to new memory API > - Port to new PCI infrastructure > - Use fixed buffers for sense processing > - Update to updated SCSI infrastructure >=20 > Changes since v6: > - Preliminary patches pushed to Kevins block tree > - Implement 64bit contexts, required for Windows7 > - Use iovecs for DCMD processing > - Add MSI-X support > Latest Linux driver now happily uses MSI-X. > - Static iovec allocation > We have a fixed upper number of iovecs, so we can > save us the allocation. Suggested by Alex Graf. > - Update MFI header > Latest Linux driver has some more definitions, > add them > - Fixup AEN handling > - Update tracing details > - Remove sdev pointer from megasas_cmd_t >=20 > Changes since v5: > - megasas: Use tracing infrastructure instead of DPRINTF > - megasas: Use new PCI infrastructure > - megasas: Check for iovec mapping failure > cpu_map_physical_memory() might fail, so we need to check for > it when mapping iovecs. > - megasas: Trace scsi buffer overflow > The transfer length as specified in the SCSI command might > disagree with the length of the iovec. We should be tracing > these issues. > - megasas: Reset frames after init firmware > When receiving an INIT FIRMWARE command we need reset all > frames, otherwise some frames might point to invalid memory. >=20 > Chances since v4: > - megasas: checkpatch.pl fixes and update to work with the > changed interface in scsi_req_new(). Also included the > suggested fixes from Alex. >=20 > Signed-off-by: Hannes Reinecke > --- > Makefile.objs | 1 + > default-configs/pci.mak | 1 + > hw/megasas.c | 2119 +++++++++++++++++++++++++++++++++++++++= ++++++++ > hw/mfi.h | 1281 ++++++++++++++++++++++++++++ > hw/pci_ids.h | 3 +- > trace-events | 73 ++ > 6 files changed, 3477 insertions(+), 1 deletions(-) > create mode 100644 hw/megasas.c > create mode 100644 hw/mfi.h > diff --git a/hw/megasas.c b/hw/megasas.c > new file mode 100644 > index 0000000..c49edd0 > --- /dev/null > +++ b/hw/megasas.c > @@ -0,0 +1,2119 @@ > +/* > + * QEMU MegaRAID SAS 8708EM2 Host Bus Adapter emulation > + * > + * Copyright (c) 2009-2011 Hannes Reinecke, SUSE Labs > + * > + * This code is licensed under the LGPL. Please clarify the license versions(s). Or-later kindly requested. > + if (cmd->iov_size > iov_size) { > + trace_megasas_iovec_len(cmd->index, "overflow", iov_size, > + cmd->iov_size); > + } else if (cmd->iov_size < iov_size) { > + trace_megasas_iovec_len(cmd->iov_size, "underflow", iov_size, > + cmd->iov_size); > + } For anything except stderr these are going to be problematic. For simpletrace "overflow" and "underflow" will just show up as a pointer value and cannot be looked up post-mortem. > +static void megasas_unmap_sgl(struct megasas_cmd_t *cmd) > +{ > + int i, is_write =3D megasas_frame_is_write(cmd); > + > + for (i =3D 0; i < cmd->iov_cnt; i++) { > + cpu_physical_memory_unmap(cmd->iov[i].iov_base, cmd->iov[i].io= v_len, > + is_write, cmd->iov[i].iov_len); Not sure, but cpu_physical_memory_* sounds old-fashioned. Might need an update to MemoryRegion? > + if (cmd->iov_size < sizeof(struct mfi_defaults)) { > + trace_megasas_dcmd_invalid_xfer_len(cmd->index, "MFC Get defau= lts", > + cmd->iov_size); String in tracepoint. > + trace_megasas_dcmd_invalid_xfer_len(cmd->index, "Get BIOS info= ", > + cmd->iov_size); dito > + trace_megasas_dcmd_enter(cmd->index, "Get Event Info"); dito > + trace_megasas_dcmd_invalid_xfer_len(cmd->index, "Get Event Wai= t", > + cmd->iov_size); Dito, rest as exercise for the reader. > +static const struct dcmd_cmd_tbl_t { > + int opcode; > + int (*func)(MPTState *s, struct megasas_cmd_t *cmd); > +} dcmd_cmd_tbl[] =3D { > + {MFI_DCMD_CTRL_MFI_HOST_MEM_ALLOC, megasas_dcmd_dummy}, Spaces after { and before } missing. > + if (!sdev || (megasas_is_jbod(s) && is_logical)) { > + trace_megasas_scsi_target_not_present( > + mfi_frame_desc[cmd->frame->header.frame_cmd], > + is_logical ? "logical" : "physical", > + cmd->frame->header.target_id, cmd->frame->header.lun_id); Two-state string could be 0/1-encoded here. > + if (len > cmd->iov_size) { > + trace_megasas_scsi_overflow(cmd->index, "scsi read", > + len, cmd->iov_size); > + } > + if (len < cmd->iov_size) { > + trace_megasas_scsi_underflow(cmd->index, "scsi read", > + len, cmd->iov_size); > + cmd->iov_size =3D len; > + } > + trace_megasas_scsi_start(cmd->index, "read", len); > + scsi_req_continue(cmd->req); > + } else if (len < 0) { > + if (-len > cmd->iov_size) { > + trace_megasas_scsi_overflow(cmd->index, "scsi write", > + -len, cmd->iov_size); > + } > + if (-len < cmd->iov_size) { > + trace_megasas_scsi_underflow(cmd->index, "scsi write", > + -len, cmd->iov_size); > + cmd->iov_size =3D -len; > + } > + trace_megasas_scsi_start(cmd->index, "write", -len); > + scsi_req_continue(cmd->req); > + } else { Here you have tracepoints for overflow and underflow and pass "scsi read" vs. "scsi write" as arguments; above (iovec) you were passing "overflow" and "underflow" as arguments. Unify the scheme? You already introduce quite a few tracepoints (a shining example, one might say) so maybe just do ..._scsi_read_overflow() and _scsi_read_underflow()? On the other hand, when someone is actually looking for a particular nesting of events, DTrace for example allows to set some per-CPU flag so not passing the specific argument might work as well. > + if (desc) { > + trace_megasas_readl_reg("mmio", desc, retval); > + } else { > + trace_megasas_readl("mmio", addr, retval); > + } Drop _reg version? Didn't review the SCSI bits. Patch is pretty large, too. ;) Regards, Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg