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

[Alexander Graf]

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 <address@hidden>
> ---
> 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
> -

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