Re: [PATCH 3/3] megasas: LSI Megaraid SAS emulation

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Jul 01, 2011 at 11:16:03AM +0200, Alexander Graf wrote:
> 
> On 01.07.2011, at 09:42, Hannes Reinecke wrote:
> 
> > This patch adds an emulation for the LSI Megaraid SAS 8708EM2 HBA.
> > 
> > Signed-off-by: Hannes Reinecke <hare@suse.de>
> > ---
> > Makefile.objs           |    1 +
> > default-configs/pci.mak |    1 +
> > hw/megasas.c            | 1923 +++++++++++++++++++++++++++++++++++++++++++++++
> > hw/mfi.h                | 1197 +++++++++++++++++++++++++++++
> > hw/pci_ids.h            |    3 +-
> > 5 files changed, 3124 insertions(+), 1 deletions(-)
> > create mode 100644 hw/megasas.c
> > create mode 100644 hw/mfi.h
> > 
> > diff --git a/Makefile.objs b/Makefile.objs
> > index cea15e4..6f5d113 100644
> > --- a/Makefile.objs
> > +++ b/Makefile.objs
> > @@ -258,6 +258,7 @@ hw-obj-$(CONFIG_AHCI) += ide/ich.o
> > 
> > # SCSI layer
> > hw-obj-$(CONFIG_LSI_SCSI_PCI) += lsi53c895a.o
> > +hw-obj-$(CONFIG_MEGASAS_SCSI_PCI) += megasas.o
> > hw-obj-$(CONFIG_ESP) += esp.o
> > 
> > hw-obj-y += dma-helpers.o sysbus.o isa-bus.o
> > diff --git a/default-configs/pci.mak b/default-configs/pci.mak
> > index 22bd350..fabb56c 100644
> > --- a/default-configs/pci.mak
> > +++ b/default-configs/pci.mak
> > @@ -9,6 +9,7 @@ CONFIG_EEPRO100_PCI=y
> > CONFIG_PCNET_PCI=y
> > CONFIG_PCNET_COMMON=y
> > CONFIG_LSI_SCSI_PCI=y
> > +CONFIG_MEGASAS_SCSI_PCI=y
> > CONFIG_RTL8139_PCI=y
> > CONFIG_E1000_PCI=y
> > CONFIG_IDE_CORE=y
> > diff --git a/hw/megasas.c b/hw/megasas.c
> > new file mode 100644
> > index 0000000..75f9be3
> > --- /dev/null
> > +++ b/hw/megasas.c
> > @@ -0,0 +1,1923 @@
> > +/*
> > + * QEMU MegaRAID SAS 8708EM2 Host Bus Adapter emulation
> > + *
> > + * Copyright (c) 2009-2011 Hannes Reinecke, SUSE Labs
> > + *
> > + * This code is licenced under the LGPL.
> 
> Please take a look at the license header of other LGPL code and just copy it :).
> 
> > + */
> > +
> > +#include <time.h>
> > +#include <assert.h>
> 
> Are you sure you need to manually include those?
> 
> > +
> > +#include "hw.h"
> > +#include "pci.h"
> > +#include "dma.h"
> > +#include "iov.h"
> > +#include "scsi.h"
> > +#include "scsi-defs.h"
> > +#include "block_int.h"
> > +#ifdef __linux__
> > +# include <scsi/sg.h>
> 
> Is this really necessary? Device code shouldn't be host dependent IMHO. I also haven't found any user of this in the actual code, so it might be as easy as merely removing the include :).
> 
> > +#endif
> > +
> > +#include "mfi.h"
> > +
> > +#define DEBUG_MEGASAS
> > +#undef DEBUG_MEGASAS_REG
> > +#undef DEBUG_MEGASAS_QUEUE
> > +#undef DEBUG_MEGASAS_MFI
> > +#undef DEBUG_MEGASAS_IO
> > +#undef DEBUG_MEGASAS_DCMD
> > +
> > +#ifdef DEBUG_MEGASAS
> > +#define DPRINTF(fmt, ...) \
> > +do { printf("megasas: " fmt , ## __VA_ARGS__); } while (0)
> > +#define BADF(fmt, ...) \
> > +do { fprintf(stderr, "megasas: error: " fmt , ## __VA_ARGS__); exit(1);} while (0)
> > +#ifdef DEBUG_MEGASAS_REG
> > +#define DPRINTF_REG DPRINTF
> > +#else
> > +#define DPRINTF_REG(fmt, ...) do {} while(0)
> > +#endif
> > +#ifdef DEBUG_MEGASAS_QUEUE
> > +#define DPRINTF_QUEUE DPRINTF
> > +#else
> > +#define DPRINTF_QUEUE(fmt, ...) do {} while(0)
> > +#endif
> > +#ifdef DEBUG_MEGASAS_MFI
> > +#define DPRINTF_MFI DPRINTF
> > +#else
> > +#define DPRINTF_MFI(fmt, ...) do {} while(0)
> > +#endif
> > +#ifdef DEBUG_MEGASAS_IO
> > +#define DPRINTF_IO DPRINTF
> > +#else
> > +#define DPRINTF_IO(fmt, ...) do {} while(0)
> > +#endif
> > +#ifdef DEBUG_MEGASAS_DCMD
> > +#define DPRINTF_DCMD DPRINTF
> > +#else
> > +#define DPRINTF_DCMD(fmt, ...) do {} while(0)
> > +#endif
> > +#else
> > +#define DPRINTF(fmt, ...) do {} while(0)
> > +#define DPRINTF_REG DPRINTF
> > +#define DPRINTF_QUEUE DPRINTF
> > +#define DPRINTF_MFI DPRINTF
> > +#define DPRINTF_IO DPRINTF
> > +#define DPRINTF_DCMD DPRINTF
> > +#define BADF(fmt, ...) \
> > +do { fprintf(stderr, "megasas: error: " fmt , ## __VA_ARGS__);} while (0)
> > +#endif
> > +
> > +/* Static definitions */
> > +#define MEGASAS_VERSION "1.20"
> > +#define MEGASAS_MAX_FRAMES 2048         /* Firmware limit at 65535 */
> > +#define MEGASAS_DEFAULT_FRAMES 1000     /* Windows requires this */
> > +#define MEGASAS_MAX_SGE 256             /* Firmware limit */
> > +#define MEGASAS_DEFAULT_SGE 80
> > +#define MEGASAS_MAX_SECTORS 0xFFFF      /* No real limit */
> > +#define MEGASAS_MAX_ARRAYS 128
> > +
> > +const char *mfi_frame_desc[] = {
> > +    "MFI init", "LD Read", "LD Write", "LD SCSI", "PD SCSI",
> > +    "MFI Doorbell", "MFI Abort", "MFI SMP", "MFI Stop"};
> > +
> > +struct megasas_cmd_t {
> > +    int index;
> > +    int context;
> > +    int count;
> > +
> > +    target_phys_addr_t pa;
> > +    target_phys_addr_t pa_size;
> > +    union mfi_frame *frame;
> > +    SCSIRequest *req;
> > +    struct iovec *iov;
> > +    void *iov_buf;
> > +    long iov_cnt;
> > +    long iov_size;
> > +    long iov_offset;
> 
> Why would anything be a long? It's either target_ulong or uintXX_t for device code usually :).
> 
> > +    SCSIDevice *sdev;
> > +    struct megasas_state_t *state;
> > +};
> > +
> > +typedef struct megasas_state_t {
> > +    PCIDevice dev;
> > +    int mmio_io_addr;
> > +    int io_addr;
> > +    int queue_addr;
> > +    uint32_t frame_hi;
> > +
> > +    int fw_state;
> > +    uint32_t fw_sge;
> > +    uint32_t fw_cmds;
> > +    int fw_luns;
> > +    int intr_mask;
> > +    int doorbell;
> > +    int busy;
> > +    char *raid_mode_str;
> > +    int is_jbod;
> > +
> > +    int event_count;
> > +    int shutdown_event;
> > +    int boot_event;
> > +
> > +    uint64_t reply_queue_pa;
> > +    void *reply_queue;
> > +    int reply_queue_len;
> > +    int reply_queue_index;
> > +    uint64_t consumer_pa;
> > +    uint64_t producer_pa;
> > +
> > +    struct megasas_cmd_t frames[MEGASAS_MAX_FRAMES];
> > +
> > +    SCSIBus bus;
> > +} MPTState;
> > +
> > +#define MEGASAS_INTR_DISABLED_MASK 0xFFFFFFFF
> > +
> > +#define MEGASAS_INTR_ENABLED(s) (((s)->intr_mask & MEGASAS_INTR_DISABLED_MASK ) != MEGASAS_INTR_DISABLED_MASK)
> > +
> > +#define megasas_frame_set_cmd_status(f,v)                               \
> > +    stb_phys((f) + offsetof(struct mfi_frame_header, cmd_status), v);
> > +
> > +#define megasas_frame_set_scsi_status(f,v)                              \
> > +    stb_phys((f) + offsetof(struct mfi_frame_header, scsi_status), v);
> > +
> > +#define megasas_frame_get_cmd(f)                                        \
> > +    ldub_phys((f) + offsetof(struct mfi_frame_header, frame_cmd))
> > +
> > +#define megasas_frame_get_context(f)                                    \
> > +    ldl_phys(frame_addr + offsetof(struct mfi_frame_header, context));
> > +
> > +static void megasas_soft_reset(MPTState *s);
> > +
> > +static int megasas_map_sgl(struct megasas_cmd_t *cmd, int pa_offset)
> > +{
> > +    int i;
> > +    uint16_t flags = le16_to_cpu(cmd->frame->header.flags);
> > +    int is_sgl64 = (flags & MFI_FRAME_SGL64) ? 1 : 0;
> > +    int is_write = (flags & MFI_FRAME_DIR_WRITE) ? 1 : 0;
> > +    int sgl_addr_size = is_sgl64 ? sizeof(uint64_t) : sizeof(uint32_t);
> > +    size_t iov_count = 0;
> > +
> > +    cmd->iov = qemu_malloc(sizeof(struct iovec) * (cmd->frame->header.sge_count + 1));
> > +    for (i = 0; i < cmd->frame->header.sge_count; i++) {
> > +        target_phys_addr_t pa, iov_pa, iov_size;
> > +
> > +        pa = cmd->pa + pa_offset;
> > +        if (is_sgl64)
> > +            iov_pa = ldq_phys(pa);
> 
> Could you please send your patch through scripts/checkpatch.pl, so that coding style issues don't hold up the commit process?
> 
> [...]
> 
> > +
> > +static void megasas_xfer_complete(SCSIRequest *req, uint32_t len)
> > +{
> > +    struct megasas_cmd_t *cmd;
> > +    uint8_t *buf;
> > +
> > +    cmd = req->hba_private;
> > +    if (!cmd) {
> 
> 
> Is this still necessary with the new pointer infrastructure?
> 
> > +        /*
> > +	 * Bad. A command has been completed but we couldn't find it.
> > +	 * Only safe way out of here is to terminate everything and
> > +	 * hope the HBA recovers.
> > +	 */
> > +        DPRINTF("SCSI request %p not found", req);
> > +        return;
> > +    }
> > +
> > +    DPRINTF_IO("%s req %p cmd %p lun %p xfer completed, len %u\n",
> > +               mfi_frame_desc[cmd->frame->header.frame_cmd], req, cmd,
> > +               cmd->sdev, len);
> > +
> > +    if (len) {
> > +        uint16_t flags = le16_to_cpu(cmd->frame->header.flags);
> > +        int is_write = (flags & MFI_FRAME_DIR_WRITE) ? 1 : 0;
> > +        size_t bytes;
> > +
> > +        buf = scsi_req_get_buf(req);
> > +        if (is_write) {
> > +            DPRINTF_IO("%s req %p cmd %p lun %p write finished, left %u\n",
> > +                       mfi_frame_desc[cmd->frame->header.frame_cmd], req,
> > +                       cmd, cmd->sdev, len);
> > +            bytes = iov_to_buf(cmd->iov, cmd->iov_cnt, buf,
> > +                               cmd->iov_offset, len);
> > +            if (bytes != len) {
> > +                len = bytes;
> > +            }
> > +            cmd->iov_offset += bytes;
> > +        } else {
> > +            DPRINTF_IO("%s req %p cmd %p lun %p read finished, len %u\n",
> > +                       mfi_frame_desc[cmd->frame->header.frame_cmd], req,
> > +                       cmd, cmd->sdev, len);
> > +            bytes = iov_from_buf(cmd->iov, cmd->iov_cnt, buf,
> > +                                 cmd->iov_offset, len);
> > +            if (bytes != len) {
> > +                len = bytes;
> > +            }
> > +            cmd->iov_offset += bytes;
> > +        }
> > +    }
> > +    cmd->iov_size -= len;
> > +    scsi_req_continue(req);
> > +}
> > +
> > +static void megasas_command_complete(SCSIRequest *req, uint32_t status)
> > +{
> > +    struct megasas_cmd_t *cmd;
> > +    uint8_t cmd_status = MFI_STAT_OK;
> > +
> > +    cmd = req->hba_private;
> > +    if (!cmd) {
> 
> same here
> 
> [...]
> 
> > diff --git a/hw/pci_ids.h b/hw/pci_ids.h
> > index d94578c..796aaf1 100644
> > --- a/hw/pci_ids.h
> > +++ b/hw/pci_ids.h
> > @@ -12,9 +12,9 @@
> > 
> > #define PCI_BASE_CLASS_STORAGE           0x01
> > #define PCI_BASE_CLASS_NETWORK           0x02
> > -
> 

Yes this separates base class (1 byte) from class
word codes.

> I don't think this is intentional, no?
> 
> > #define PCI_CLASS_STORAGE_SCSI           0x0100
> > #define PCI_CLASS_STORAGE_IDE            0x0101
> > +#define PCI_CLASS_STORAGE_RAID           0x0104
> > #define PCI_CLASS_STORAGE_SATA           0x0106
> > #define PCI_CLASS_STORAGE_OTHER          0x0180
> > 
> > @@ -47,6 +47,7 @@
> > 
> > #define PCI_VENDOR_ID_LSI_LOGIC          0x1000
> > #define PCI_DEVICE_ID_LSI_53C895A        0x0012
> > +#define PCI_DEVICE_ID_LSI_SAS1078        0x0060
> 
> Michael, these go to your table :)
> 
> 
> Alex