Re: [Qemu-devel] [PATCH][v9] megasas: LSI Megaraid SAS HBA emulation

["Andreas Färber" <afaerber@suse.de>]

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.
> 
> Changes since v8:
> - Remove 'disable' keyword from trace definitions
> - Convert hand-crafted debugging statements with trace
>   definitions
> - Treat 'context' tag as little endian
> 
> Changes since v7:
> - Port to new memory API
> - Port to new PCI infrastructure
> - Use fixed buffers for sense processing
> - Update to updated SCSI infrastructure
> 
> 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
> 
> 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.
> 
> 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.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  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 = megasas_frame_is_write(cmd);
> +
> +    for (i = 0; i < cmd->iov_cnt; i++) {
> +        cpu_physical_memory_unmap(cmd->iov[i].iov_base, cmd->iov[i].iov_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 defaults",
> +                                            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 Wait",
> +                                            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[] = {
> +    {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 = 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 = -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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg