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

[Hannes Reinecke <hare@suse.de>]

Hi Micheal,

thanks for your review.
You'll find the answers inline.

On 02/23/2012 04:34 PM, Michael S. Tsirkin wrote:
> On Tue, Feb 21, 2012 at 10:36:43AM +0100, Hannes Reinecke wrote:
>> This patch adds an emulation for the LSI Megaraid SAS 8708EM2 HBA.
>> I've tested it to work with Linux, Windows Vista, and Windows7.
>> MSI-X support is currently broken; have to investigate.
>>
[ .. ]
> 
> So Alex asked whether I can merge this, which made me
> take a look. I don't know much about what this does
> so just general comments on all of the code.
> 
> Also, some issues related to msix - want to rip that code
> out for now since it does not work anyway?
> 
Well, I left it in as a reminder how things _should_ be coded.
I was hoping to get it to work eventually.

But then I didn't so maybe you're right.

> qemu has two styles for struct and enum types:
> 1. documented: typedef struct CamelCase CamelCase;
> 2. undocumented but still widely used: struct lower_case; (no typedef)
> *_t type typedef is used for numeric types such as target_phys_addr_t.
> 
> This code mixes them in arbitrary ways. Pls don't do that,
> pls be consistent.
> 
Ok, done.

> 
> 
> 
>> ---
>>  Makefile.objs           |    1 +
>>  default-configs/pci.mak |    1 +
>>  hw/megasas.c            | 1865 +++++++++++++++++++++++++++++++++++++++++++++++
>>  hw/mfi.h                | 1281 ++++++++++++++++++++++++++++++++
>>  hw/pci_ids.h            |    3 +-
>>  5 files changed, 3150 insertions(+), 1 deletions(-)
>>  create mode 100644 hw/megasas.c
>>  create mode 100644 hw/mfi.h
>>
>> diff --git a/Makefile.objs b/Makefile.objs
>> index 391e524..5841998 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -283,6 +283,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 9d3e1db..4b49c00 100644
>> --- a/default-configs/pci.mak
>> +++ b/default-configs/pci.mak
>> @@ -10,6 +10,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..083c3d3
>> --- /dev/null
>> +++ b/hw/megasas.c
>> @@ -0,0 +1,1865 @@
>> +/*
>> + * QEMU MegaRAID SAS 8708EM2 Host Bus Adapter emulation
> 
> Link to documentation/hardware spec/driver code?
> 
I would if I had some.
The only reference I had is the Linux megaraid_sas.c driver.

>> + *
>> + * Copyright (c) 2009-2012 Hannes Reinecke, SUSE Labs
>> + *
>> + * This library is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2 of the License, or (at your option) any later version.
>> + *
>> + * This library is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include "hw.h"
>> +#include "pci.h"
>> +#include "dma.h"
>> +#include "msix.h"
>> +#include "iov.h"
>> +#include "scsi.h"
>> +#include "scsi-defs.h"
>> +#include "block_int.h"
>> +
>> +#include "mfi.h"
>> +
>> +/* Static definitions */
> 
> Remove above comment.
> 
Ok.

>> +#define MEGASAS_VERSION "1.60"
>> +#define MEGASAS_MAX_FRAMES 2048         /* Firmware limit at 65535 */
>> +#define MEGASAS_DEFAULT_FRAMES 1000     /* Windows requires this */
>> +#define MEGASAS_MAX_SGE 128             /* Firmware limit */
>> +#define MEGASAS_DEFAULT_SGE 80
>> +#define MEGASAS_MAX_SECTORS 0xFFFF      /* No real limit */
>> +#define MEGASAS_MAX_ARRAYS 128
>> +
>> +#define MEGASAS_FLAG_USE_JBOD      0
>> +#define MEGASAS_MASK_USE_JBOD      (1 << MEGASAS_FLAG_USE_JBOD)
>> +#define MEGASAS_FLAG_USE_MSIX      1
>> +#define MEGASAS_MASK_USE_MSIX      (1 << MEGASAS_FLAG_USE_MSIX)
>> +#define MEGASAS_FLAG_USE_QUEUE64   2
>> +#define MEGASAS_MASK_USE_QUEUE64   (1 << MEGASAS_FLAG_USE_QUEUE64)
>> +
>> +const char *megasas_raid_modes[] = {
>> +    "raid", "jbod"
>> +};
>> +
>> +const char *mfi_frame_desc[] = {
>> +    "MFI init", "LD Read", "LD Write", "LD SCSI", "PD SCSI",
>> +    "MFI Doorbell", "MFI Abort", "MFI SMP", "MFI Stop"};
> 
> Can't the above go into mfi.h?
> 
Hmm. Well.

The problem with mfi.h is that it's not actually _my_ file, but
rather copied over from NetBSD. I felt a bit stupid doing a recoding
of all the values which are already present in NetBSD ...
Hence the name 'mfi.h', and the different copyright.
For the same reason I try to keep the differences between the
NetBSD and my version as small as possible.

If you have a better solution on how I should handle this
I'm willing to listen ...

>> +
>> +struct megasas_cmd_t {
>> +    uint32_t index;
>> +    uint16_t flags;
>> +    uint16_t count;
>> +    uint64_t context;
>> +
>> +    target_phys_addr_t pa;
>> +    target_phys_addr_t pa_size;
>> +    union mfi_frame *frame;
>> +    SCSIRequest *req;
>> +    struct iovec iov[MEGASAS_MAX_SGE];
>> +    int iov_cnt;
>> +    size_t iov_size;
>> +    size_t iov_offset;
>> +    struct megasas_state_t *state;
>> +};
>> +
>> +typedef struct megasas_state_t {
>> +    PCIDevice dev;
>> +    MemoryRegion mmio_io;
>> +    MemoryRegion port_io;
>> +    MemoryRegion queue_io;
>> +    uint32_t frame_hi;
>> +
>> +    int fw_state;
>> +    uint32_t fw_sge;
>> +    uint32_t fw_cmds;
>> +    uint32_t flags;
>> +    int fw_luns;
>> +    int intr_mask;
>> +    int doorbell;
>> +    int busy;
>> +    char *raid_mode_str;
> 
> make it const char * and you will not need
> casts from const char * to char * which are just wrong.
> 
I've replaced it by using the flag directly, so the
pointer and the values above are removed.

>> +
>> +    struct megasas_cmd_t *event_cmd;
>> +    int event_locale;
>> +    int event_class;
>> +    int event_count;
>> +    int shutdown_event;
>> +    int boot_event;
>> +
>> +    uint64_t reply_queue_pa;
>> +    void *reply_queue;
>> +    int reply_queue_len;
>> +    int reply_queue_head;
>> +    int reply_queue_tail;
>> +    uint64_t consumer_pa;
>> +    uint64_t producer_pa;
>> +
>> +    struct megasas_cmd_t frames[MEGASAS_MAX_FRAMES];
>> +
>> +    SCSIBus bus;
>> +} MPTState;
> 
> Prefix with megasas or mfi, consistently.
> 
D'accord. Leftover from the original code, which was a copy of the
LSI SCSI driver.

[ .. ]
>> +
>> +static int megasas_handle_scsi(MPTState *s, struct megasas_cmd_t *cmd,
>> +                               int is_logical)
>> +{
>> +    uint8_t *cdb;
>> +    int len;
>> +    struct SCSIDevice *sdev = NULL;
>> +
>> +    cdb = cmd->frame->pass.cdb;
>> +
>> +    if (cmd->frame->header.target_id < s->fw_luns) {
>> +        sdev = scsi_device_find(&s->bus, 0, cmd->frame->header.target_id,
>> +                                cmd->frame->header.lun_id);
>> +    }
>> +    cmd->iov_size = le32_to_cpu(cmd->frame->header.data_len);
>> +
>> +    if (!sdev || (megasas_is_jbod(s) && is_logical)) {
>> +        return MFI_STAT_DEVICE_NOT_FOUND;
>> +    }
>> +
>> +    if (cmd->frame->header.cdb_len > 16) {
>> +        megasas_write_sense(cmd, SENSE_CODE(INVALID_OPCODE));
>> +        cmd->frame->header.scsi_status = CHECK_CONDITION;
>> +        s->event_count++;
>> +        return MFI_STAT_SCSI_DONE_WITH_ERROR;
>> +    }
>> +
>> +    if (megasas_map_sgl(cmd, &cmd->frame->pass.sgl)) {
>> +        megasas_write_sense(cmd, SENSE_CODE(TARGET_FAILURE));
>> +        cmd->frame->header.scsi_status = CHECK_CONDITION;
>> +        s->event_count++;
>> +        return MFI_STAT_SCSI_DONE_WITH_ERROR;
>> +    }
>> +
>> +    cmd->req = scsi_req_new(sdev, cmd->index,
>> +                            cmd->frame->header.lun_id, cdb, cmd);
>> +    if (!cmd->req) {
>> +        megasas_write_sense(cmd, SENSE_CODE(NO_SENSE));
>> +        cmd->frame->header.scsi_status = BUSY;
>> +        s->event_count++;
>> +        return MFI_STAT_SCSI_DONE_WITH_ERROR;
>> +    }
>> +
>> +    len = scsi_req_enqueue(cmd->req);
>> +    if (len > 0) {
>> +        if (len < cmd->iov_size) {
>> +            cmd->iov_size = len;
>> +        }
>> +        scsi_req_continue(cmd->req);
>> +    } else if (len < 0) {
>> +        if (-len < cmd->iov_size) {
>> +            cmd->iov_size = -len;
>> +        }
>> +        scsi_req_continue(cmd->req);
>> +    }
> 
> Shorter:
> 
> if (len < 0) {
>     len = -len;
> }
> 
> if (len) {
>     cmd->iov_size = MIN(len, cmd->iov_size);
>     scsi_req_continue(cmd->req);
> }
> 
> 
Ok.

>> +            cmd->iov_size = -len;
>> +        }
> 
>> +    return MFI_STAT_INVALID_STATUS;
>> +}
>> +
>> +static int megasas_handle_io(MPTState *s, struct megasas_cmd_t *cmd)
>> +{
>> +    uint32_t lba_count, lba_start_hi, lba_start_lo;
>> +    uint64_t lba_start;
>> +    int write = cmd->frame->header.frame_cmd == MFI_CMD_LD_WRITE ? 1 : 0;
>> +    uint8_t cdb[16];
>> +    int len;
>> +    struct SCSIDevice *sdev = NULL;
>> +
>> +    lba_count = le32_to_cpu(cmd->frame->io.header.data_len);
>> +    lba_start_lo = le32_to_cpu(cmd->frame->io.lba_lo);
>> +    lba_start_hi = le32_to_cpu(cmd->frame->io.lba_hi);
>> +    lba_start = ((uint64_t)lba_start_hi << 32) | lba_start_lo;
>> +
>> +    if (cmd->frame->header.target_id < s->fw_luns) {
>> +        sdev = scsi_device_find(&s->bus, 0, cmd->frame->header.target_id,
>> +                                cmd->frame->header.lun_id);
>> +    }
>> +
>> +    if (!sdev) {
>> +        return MFI_STAT_DEVICE_NOT_FOUND;
>> +    }
>> +
>> +    if (cmd->frame->header.cdb_len > 16) {
>> +        megasas_write_sense(cmd, SENSE_CODE(INVALID_OPCODE));
>> +        cmd->frame->header.scsi_status = CHECK_CONDITION;
>> +        s->event_count++;
>> +        return MFI_STAT_SCSI_DONE_WITH_ERROR;
>> +    }
>> +
>> +    cmd->iov_size = lba_count * sdev->blocksize;
>> +    if (megasas_map_sgl(cmd, &cmd->frame->io.sgl)) {
>> +        megasas_write_sense(cmd, SENSE_CODE(TARGET_FAILURE));
>> +        cmd->frame->header.scsi_status = CHECK_CONDITION;
>> +        s->event_count++;
>> +        return MFI_STAT_SCSI_DONE_WITH_ERROR;
>> +    }
>> +
>> +    megasas_encode_lba(cdb, lba_start, lba_count, write);
>> +    cmd->req = scsi_req_new(sdev, cmd->index,
>> +                            cmd->frame->header.lun_id, cdb, cmd);
>> +    if (!cmd->req) {
>> +        megasas_write_sense(cmd, SENSE_CODE(NO_SENSE));
>> +        cmd->frame->header.scsi_status = BUSY;
>> +        s->event_count++;
>> +        return MFI_STAT_SCSI_DONE_WITH_ERROR;
>> +    }
>> +
>> +    len = scsi_req_enqueue(cmd->req);
>> +    if (len > 0) {
>> +        if (len < cmd->iov_size) {
>> +            cmd->iov_size = len;
>> +        }
>> +        scsi_req_continue(cmd->req);
>> +    } else if (len < 0) {
>> +        if (-len < cmd->iov_size) {
>> +            cmd->iov_size = -len;
>> +        }
>> +        scsi_req_continue(cmd->req);
>> +    }
>> +    return MFI_STAT_INVALID_STATUS;
> 
> code duplicated from above. make it a function?
> 
Yes, can do.

>> +}
>> +
>> +static int megasas_finish_internal_command(struct megasas_cmd_t *cmd,
>> +                                           SCSIRequest *req)
>> +{
>> +    int retval = MFI_STAT_INVALID_CMD;
>> +
>> +    if (cmd->frame->header.frame_cmd == MFI_CMD_DCMD) {
>> +            retval = megasas_finish_internal_dcmd(cmd, req);
>> +    }
>> +    return retval;
>> +}
>> +
>> +static void megasas_xfer_complete(SCSIRequest *req, uint32_t len)
>> +{
>> +    struct megasas_cmd_t *cmd = req->hba_private;
>> +    uint8_t *buf;
>> +
>> +    if (len) {
>> +        int is_write = megasas_frame_is_write(cmd);
>> +        size_t bytes;
>> +
>> +        buf = scsi_req_get_buf(req);
>> +        if (is_write) {
>> +            bytes = iov_to_buf(cmd->iov, cmd->iov_cnt, buf,
>> +                               cmd->iov_offset, len);
>> +            if (bytes != len) {
>> +                len = bytes;
>> +            }
> 
> as len is unused below this is dead code?
> 
Yes, can be removed.

>> +            cmd->iov_offset += bytes;
>> +        } else {
>> +            bytes = iov_from_buf(cmd->iov, cmd->iov_cnt, buf,
>> +                                 cmd->iov_offset, len);
>> +            if (bytes != len) {
>> +                len = bytes;
>> +            }
> 
> as len is unused below this is dead code?
> 
>> +            cmd->iov_offset += bytes;
>> +        }
>> +    }
> 
> so do we discard 0 length or continue?
> previous functions discard this one continues.
> Intentional?
> 
yes. SCSI magic. The previous function kick off data transfer
(which they only have to if there _is_ data to transfer), hence
the discard.
This one is the callback once data transfer is finished, hence we
need to continue.

Wasn't me who designed this ...

>> +    scsi_req_continue(req);
>> +}
>> +
>> +static void megasas_command_complete(SCSIRequest *req, uint32_t status)
>> +{
>> +    struct megasas_cmd_t *cmd = req->hba_private;
>> +    uint8_t cmd_status = MFI_STAT_OK;
>> +
>> +    if (cmd->req != req) {
>> +        /*
>> +         * Internal command complete
>> +         */
>> +        cmd_status = megasas_finish_internal_command(cmd, req);
>> +        if (cmd_status == MFI_STAT_INVALID_STATUS) {
>> +            return;
>> +        }
>> +    } else {
>> +        req->status = status;
>> +        if (req->status != GOOD) {
>> +            cmd_status = MFI_STAT_SCSI_DONE_WITH_ERROR;
>> +        }
>> +        if (req->status == CHECK_CONDITION) {
>> +            megasas_copy_sense(cmd);
>> +        }
>> +
>> +        megasas_unmap_sgl(cmd);
>> +        cmd->frame->header.scsi_status = req->status;
>> +        scsi_req_unref(cmd->req);
>> +        cmd->req = NULL;
>> +    }
>> +    cmd->frame->header.cmd_status = cmd_status;
>> +    megasas_complete_frame(cmd->state, cmd->context);
>> +}
>> +
>> +static void megasas_command_cancel(SCSIRequest *req)
>> +{
>> +    struct megasas_cmd_t *cmd = req->hba_private;
>> +
>> +    if (cmd) {
>> +        megasas_abort_command(cmd);
>> +    } else {
>> +        scsi_req_unref(req);
>> +    }
>> +    return;
> 
> return here is useless
> 
Indeed.

[ .. ]
>> +
>> +static int megasas_scsi_uninit(PCIDevice *d)
>> +{
>> +    MPTState *s = DO_UPCAST(MPTState, dev, d);
> 
> You must also uinit msix.
> It is harmless to do this uncoditionally.
> 
Ok.

> 
>> +
>> +    memory_region_destroy(&s->mmio_io);
>> +    memory_region_destroy(&s->port_io);
>> +    memory_region_destroy(&s->queue_io);
>> +    return 0;
>> +}
>> +
>> +static const struct SCSIBusInfo megasas_scsi_info = {
>> +    .tcq = true,
>> +    .max_target = MFI_MAX_LD,
>> +    .max_lun = 255,
>> +
>> +    .transfer_data = megasas_xfer_complete,
>> +    .complete = megasas_command_complete,
>> +    .cancel = megasas_command_cancel,
>> +};
>> +
>> +static int megasas_scsi_init(PCIDevice *dev)
>> +{
>> +    MPTState *s = DO_UPCAST(MPTState, dev, dev);
>> +    uint8_t *pci_conf;
>> +    int i;
>> +
>> +    pci_conf = s->dev.config;
>> +
>> +    /* PCI latency timer = 0 */
>> +    pci_conf[PCI_LATENCY_TIMER] = 0;
>> +    /* Interrupt pin 1 */
>> +    pci_conf[PCI_INTERRUPT_PIN] = 0x01;
>> +
>> +    memory_region_init_io(&s->mmio_io, &megasas_mmio_ops, s,
>> +                          "megasas-mmio", 0x4000);
>> +    memory_region_init_io(&s->port_io, &megasas_port_ops, s,
>> +                          "megasas-io", 256);
>> +    memory_region_init_io(&s->queue_io, &megasas_queue_ops, s,
>> +                          "megasas-queue", 0x40000);
>> +
>> +    if (megasas_use_msix(s) &&
>> +        msix_init(&s->dev, 15, &s->mmio_io, 0, 0x2000)) {
>> +        s->flags &= ~MEGASAS_MASK_USE_MSIX;
> 
> You'd want an error message here. maybe even fail init.
> 
But why? The driver continues happily without MSI-X.
And the failure message will be printed out via trace events,
in case someone is interested.

>> +    }
>> +
>> +    pci_register_bar(&s->dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->mmio_io);
>> +    pci_register_bar(&s->dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &s->port_io);
>> +    pci_register_bar(&s->dev, 3, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->queue_io);
>> +
>> +    if (megasas_use_msix(s)) {
>> +        msix_vector_use(&s->dev, 0);
> 
> You can do this unconditionally.  But I have a question: are you using a
> single vector? I note that you request 15 vectors from the OS.
The original hardware does. So I thought it best to simulate this as
close as possible.

> The vector allocator 
> 
>> +    }
>> +
>> +    if (s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
>> +        s->fw_sge = MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE;
>> +    } else if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
>> +        s->fw_sge = 128 - MFI_PASS_FRAME_SIZE;
>> +    } else {
>> +        s->fw_sge = 64 - MFI_PASS_FRAME_SIZE;
>> +    }
>> +    if (s->fw_cmds > MEGASAS_MAX_FRAMES) {
>> +        s->fw_cmds = MEGASAS_MAX_FRAMES;
>> +    }
>> +    if (s->raid_mode_str) {
>> +        if (!strcmp(s->raid_mode_str, "jbod")) {
>> +            s->flags |= MEGASAS_MASK_USE_JBOD;
>> +        } else {
>> +            s->flags &= ~MEGASAS_MASK_USE_JBOD;
>> +            s->raid_mode_str = (char *)megasas_raid_modes[0];
>> +        }
> 
> validate mode value. Anything that is not raid or jbod
> should fail.
> 
See above. Code has been removed.

>> +    } else {
>> +        s->raid_mode_str = (char *)megasas_raid_modes[0];
>> +    }
> 
> Don't cast, use consisten styles.
> 
>> +    s->fw_luns = (MFI_MAX_LD > MAX_SCSI_DEVS) ?
>> +        MAX_SCSI_DEVS : MFI_MAX_LD;
>> +    s->producer_pa = 0;
>> +    s->consumer_pa = 0;
>> +    for (i = 0; i < s->fw_cmds; i++) {
>> +        s->frames[i].index = i;
>> +        s->frames[i].context = -1;
>> +        s->frames[i].pa = 0;
>> +        s->frames[i].state = s;
>> +    }
>> +
>> +    scsi_bus_new(&s->bus, &dev->qdev, &megasas_scsi_info);
>> +    scsi_bus_legacy_handle_cmdline(&s->bus);
>> +    return 0;
>> +}
>> +
>> +static Property megasas_properties[] = {
>> +    DEFINE_PROP_UINT32("max_sge", MPTState, fw_sge,
>> +                       MEGASAS_DEFAULT_SGE),
>> +    DEFINE_PROP_UINT32("max_cmds", MPTState, fw_cmds,
>> +                       MEGASAS_DEFAULT_FRAMES),
>> +    DEFINE_PROP_STRING("mode", MPTState, raid_mode_str),
>> +    DEFINE_PROP_BIT("use_msix", MPTState, flags,
>> +                    MEGASAS_FLAG_USE_MSIX, false),
> 
> This is just a workaround for debugging, right?
> So either just remove all msix code for now,
> if 0 all msix code, or name property x-use_msix
> 
Ok. I'll go for the '#if 0'.

>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void megasas_class_init(ObjectClass *oc, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(oc);
>> +    PCIDeviceClass *pc = PCI_DEVICE_CLASS(oc);
>> +
>> +    pc->init = megasas_scsi_init;
>> +    pc->exit = megasas_scsi_uninit;
>> +    pc->vendor_id = PCI_VENDOR_ID_LSI_LOGIC;
>> +    pc->device_id = PCI_DEVICE_ID_LSI_SAS1078;
>> +    pc->subsystem_vendor_id = PCI_VENDOR_ID_LSI_LOGIC;
>> +    pc->subsystem_id = 0x1013;
>> +    pc->class_id = PCI_CLASS_STORAGE_RAID;
>> +    dc->props = megasas_properties;
>> +    dc->reset = megasas_scsi_reset;
>> +    dc->vmsd = &vmstate_megasas;
>> +    dc->desc = "LSI MegaRAID SAS 1078";
>> +}
>> +
>> +static TypeInfo megasas_info = {
>> +    .name  = "megasas",
>> +    .parent = TYPE_PCI_DEVICE,
>> +    .instance_size = sizeof(MPTState),
>> +    .class_init = megasas_class_init,
>> +};
>> +
>> +static void megaraid1078_register_types(void)
>> +{
>> +    type_register_static(&megasas_info);
>> +}
>> +
>> +type_init(megaraid1078_register_types);
> 
> 
> why not megasas_ ?
> 
Because?
But yes, good point.

>> diff --git a/hw/mfi.h b/hw/mfi.h
>> new file mode 100644
>> index 0000000..4790c7c
>> --- /dev/null
>> +++ b/hw/mfi.h
> 
> Sorry if this was discussed already, where is this
> code from? freebsd? it seems to have this:
> http://gitorious.org/freebsd/freebsd/blobs/HEAD/sys/dev/mfi/mfireg.h
Yes, that's the case.

> Want to name the file the same and add a link?
> This would be an explanation why we keep the
> file in a weird style incompatible with qemu.
> 
> Still some things I think are better off fixed.
> Noted below.
> 
>> @@ -0,0 +1,1281 @@
>> +/*-
>> + * Copyright (c) 2006 IronPort Systems
>> + * All rights reserved.
>> + *
>> + * Redistribution and use in source and binary forms, with or without
>> + * modification, are permitted provided that the following conditions
>> + * are met:
>> + * 1. Redistributions of source code must retain the above copyright
>> + *    notice, this list of conditions and the following disclaimer.
>> + * 2. Redistributions in binary form must reproduce the above copyright
>> + *    notice, this list of conditions and the following disclaimer in the
>> + *    documentation and/or other materials provided with the distribution.
>> + *
>> + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
>> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
>> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
>> + * ARE DISCLAIMED.  IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
>> + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
>> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
>> + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
>> + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
>> + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
>> + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
>> + * SUCH DAMAGE.
>> + */
>> +/*-
>> + * Copyright (c) 2007 LSI Corp.
>> + * Copyright (c) 2007 Rajesh Prabhakaran.
>> + * All rights reserved.
>> + *
>> + * Redistribution and use in source and binary forms, with or without
>> + * modification, are permitted provided that the following conditions
>> + * are met:
>> + * 1. Redistributions of source code must retain the above copyright
>> + *    notice, this list of conditions and the following disclaimer.
>> + * 2. Redistributions in binary form must reproduce the above copyright
>> + *    notice, this list of conditions and the following disclaimer in the
>> + *    documentation and/or other materials provided with the distribution.
>> + *
>> + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
>> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
>> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
>> + * ARE DISCLAIMED.  IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
>> + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
>> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
>> + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
>> + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
>> + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
>> + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
>> + * SUCH DAMAGE.
>> + */
> 
> Why do we need two of these? They appear identical.
> Just combine all copyrights.
> 
Ok.

>> +
>> +#ifndef _MFI_H
>> +#define _MFI_H
> 
> Don't start identifiers with a single _ followed
> by an upper case letter. MFI_REG_H would be a better name.
> 
>> +
>> +/*
>> + * MegaRAID SAS MFI firmware definitions
>> + *
>> + * Calling this driver 'MegaRAID SAS' is a bit misleading.  It's a completely
>> + * new firmware interface from the old AMI MegaRAID one, and there is no
>> + * reason why this interface should be limited to just SAS.  In any case, LSI
>> + * seems to also call this interface 'MFI', so that will be used here.
>> + */
> 
> *Which* driver name is misleading?
> I'd suggest droping this comment that argues with itself,
> and explaining what this header *is*. E.g. is the below right?
> /*
>   MFI is a common firmware interface used by MegaRAID
>   family of controllers by SAS and LSI
>  */
>> +
>> +/*
>> + * Start with the register set.
>>  All registers are 32 bits wide.
>> + * The usual Intel IOP style setup.
>> + */
>> +#define MFI_IMSG0 0x10    /* Inbound message 0 */
>> +#define MFI_IMSG1 0x14    /* Inbound message 1 */
>> +#define MFI_OMSG0 0x18    /* Outbound message 0 */
>> +#define MFI_OMSG1 0x1c    /* Outbound message 1 */
>> +#define MFI_IDB   0x20    /* Inbound doorbell */
>> +#define MFI_ISTS  0x24    /* Inbound interrupt status */
>> +#define MFI_IMSK  0x28    /* Inbound interrupt mask */
>> +#define MFI_ODB   0x2c    /* Outbound doorbell */
>> +#define MFI_OSTS  0x30    /* Outbound interrupt status */
>> +#define MFI_OMSK  0x34    /* Outbound interrupt mask */
>> +#define MFI_IQP   0x40    /* Inbound queue port */
>> +#define MFI_OQP   0x44    /* Outbound queue port */
>> +
>> +/*
>> + * 1078 specific related register
>> + */
>> +#define MFI_ODR0        0x9c            /* outbound doorbell register0 */
>> +#define MFI_ODCR0       0xa0            /* outbound doorbell clear register0  */
>> +#define MFI_OSP0        0xb0            /* outbound scratch pad0  */
>> +#define MFI_IQPL        0xc0            /* Inbound queue port (low bytes)  */
>> +#define MFI_IQPH        0xc4            /* Inbound queue port (high bytes)  */
>> +#define MFI_DIAG        0xf8            /* Host diag */
>> +#define MFI_SEQ         0xfc            /* Sequencer offset */
>> +#define MFI_1078_EIM    0x80000004      /* 1078 enable intrrupt mask  */
>> +#define MFI_RMI         0x2             /* reply message interrupt  */
>> +#define MFI_1078_RM     0x80000000      /* reply 1078 message interrupt  */
>> +#define MFI_ODC         0x4             /* outbound doorbell change interrupt */
>> +
>> +/*
>> + * gen2 specific changes
>> + */
>> +#define MFI_GEN2_EIM    0x00000005      /* gen2 enable interrupt mask */
>> +#define MFI_GEN2_RM     0x00000001      /* reply gen2 message interrupt */
>> +
>> +/*
>> + * skinny specific changes
>> + */
>> +#define MFI_SKINNY_IDB  0x00    /* Inbound doorbell is at 0x00 for skinny */
>> +#define MFI_SKINNY_RM   0x00000001      /* reply skinny message interrupt */
>> +
>> +/* Bits for MFI_OSTS */
>> +#define MFI_OSTS_INTR_VALID     0x00000002
>> +
>> +/*
>> + * Firmware state values.  Found in OMSG0 during initialization.
>> + */
>> +#define MFI_FWSTATE_MASK                0xf0000000
>> +#define MFI_FWSTATE_UNDEFINED           0x00000000
>> +#define MFI_FWSTATE_BB_INIT             0x10000000
>> +#define MFI_FWSTATE_FW_INIT             0x40000000
>> +#define MFI_FWSTATE_WAIT_HANDSHAKE      0x60000000
>> +#define MFI_FWSTATE_FW_INIT_2           0x70000000
>> +#define MFI_FWSTATE_DEVICE_SCAN         0x80000000
>> +#define MFI_FWSTATE_BOOT_MSG_PENDING    0x90000000
>> +#define MFI_FWSTATE_FLUSH_CACHE         0xa0000000
>> +#define MFI_FWSTATE_READY               0xb0000000
>> +#define MFI_FWSTATE_OPERATIONAL         0xc0000000
>> +#define MFI_FWSTATE_FAULT               0xf0000000
>> +#define MFI_FWSTATE_MAXSGL_MASK         0x00ff0000
>> +#define MFI_FWSTATE_MAXCMD_MASK         0x0000ffff
>> +#define MFI_FWSTATE_MSIX_SUPPORTED      0x04000000
>> +#define MFI_FWSTATE_HOSTMEMREQD_MASK    0x08000000
>> +
>> +/*
>> + * Control bits to drive the card to ready state.  These go into the IDB
>> + * register.
>> + */
>> +#define MFI_FWINIT_ABORT        0x00000001 /* Abort all pending commands */
>> +#define MFI_FWINIT_READY        0x00000002 /* Move from operational to ready */
>> +#define MFI_FWINIT_MFIMODE      0x00000004 /* unknown */
>> +#define MFI_FWINIT_CLEAR_HANDSHAKE 0x00000008 /* Respond to WAIT_HANDSHAKE */
>> +#define MFI_FWINIT_HOTPLUG      0x00000010
>> +#define MFI_FWINIT_STOP_ADP     0x00000020 /* Move to operational, stop */
>> +#define MFI_FWINIT_ADP_RESET    0x00000040 /* Reset ADP */
>> +
>> +/* MFI Commands */
>> +typedef enum {
>> +    MFI_CMD_INIT = 0x00,
>> +    MFI_CMD_LD_READ,
>> +    MFI_CMD_LD_WRITE,
>> +    MFI_CMD_LD_SCSI_IO,
>> +    MFI_CMD_PD_SCSI_IO,
>> +    MFI_CMD_DCMD,
>> +    MFI_CMD_ABORT,
>> +    MFI_CMD_SMP,
>> +    MFI_CMD_STP
>> +} mfi_cmd_t ;
> 
> space before ;, here and elsewhere.
> 
Fixed.

> 
>> +
>> +/* Direct commands */
>> +typedef enum {
>> +    MFI_DCMD_CTRL_MFI_HOST_MEM_ALLOC =  0x0100e100,
>> +    MFI_DCMD_CTRL_GET_INFO =            0x01010000,
>> +    MFI_DCMD_CTRL_GET_PROPERTIES =      0x01020100,
>> +    MFI_DCMD_CTRL_SET_PROPERTIES =      0x01020200,
>> +    MFI_DCMD_CTRL_ALARM =               0x01030000,
>> +    MFI_DCMD_CTRL_ALARM_GET =           0x01030100,
>> +    MFI_DCMD_CTRL_ALARM_ENABLE =        0x01030200,
>> +    MFI_DCMD_CTRL_ALARM_DISABLE =       0x01030300,
>> +    MFI_DCMD_CTRL_ALARM_SILENCE =       0x01030400,
>> +    MFI_DCMD_CTRL_ALARM_TEST =          0x01030500,
>> +    MFI_DCMD_CTRL_EVENT_GETINFO =       0x01040100,
>> +    MFI_DCMD_CTRL_EVENT_CLEAR =         0x01040200,
>> +    MFI_DCMD_CTRL_EVENT_GET =           0x01040300,
>> +    MFI_DCMD_CTRL_EVENT_COUNT =         0x01040400,
>> +    MFI_DCMD_CTRL_EVENT_WAIT =          0x01040500,
>> +    MFI_DCMD_CTRL_SHUTDOWN =            0x01050000,
>> +    MFI_DCMD_HIBERNATE_STANDBY =        0x01060000,
>> +    MFI_DCMD_CTRL_GET_TIME =            0x01080101,
>> +    MFI_DCMD_CTRL_SET_TIME =            0x01080102,
>> +    MFI_DCMD_CTRL_BIOS_DATA_GET =       0x010c0100,
>> +    MFI_DCMD_CTRL_BIOS_DATA_SET =       0x010c0200,
>> +    MFI_DCMD_CTRL_FACTORY_DEFAULTS =    0x010d0000,
>> +    MFI_DCMD_CTRL_MFC_DEFAULTS_GET =    0x010e0201,
>> +    MFI_DCMD_CTRL_MFC_DEFAULTS_SET =    0x010e0202,
>> +    MFI_DCMD_CTRL_CACHE_FLUSH =         0x01101000,
>> +    MFI_DCMD_PD_GET_LIST =              0x02010000,
>> +    MFI_DCMD_PD_LIST_QUERY =            0x02010100,
>> +    MFI_DCMD_PD_GET_INFO =              0x02020000,
>> +    MFI_DCMD_PD_STATE_SET =             0x02030100,
>> +    MFI_DCMD_PD_REBUILD =               0x02040100,
>> +    MFI_DCMD_PD_BLINK =                 0x02070100,
>> +    MFI_DCMD_PD_UNBLINK =               0x02070200,
>> +    MFI_DCMD_LD_GET_LIST =              0x03010000,
>> +    MFI_DCMD_LD_GET_INFO =              0x03020000,
>> +    MFI_DCMD_LD_GET_PROP =              0x03030000,
>> +    MFI_DCMD_LD_SET_PROP =              0x03040000,
>> +    MFI_DCMD_LD_DELETE =                0x03090000,
>> +    MFI_DCMD_CFG_READ =                 0x04010000,
>> +    MFI_DCMD_CFG_ADD =                  0x04020000,
>> +    MFI_DCMD_CFG_CLEAR =                0x04030000,
>> +    MFI_DCMD_CFG_FOREIGN_READ =         0x04060100,
>> +    MFI_DCMD_CFG_FOREIGN_IMPORT =       0x04060400,
>> +    MFI_DCMD_BBU_STATUS =               0x05010000,
>> +    MFI_DCMD_BBU_CAPACITY_INFO =        0x05020000,
>> +    MFI_DCMD_BBU_DESIGN_INFO =          0x05030000,
>> +    MFI_DCMD_BBU_PROP_GET =             0x05050100,
>> +    MFI_DCMD_CLUSTER =                  0x08000000,
>> +    MFI_DCMD_CLUSTER_RESET_ALL =        0x08010100,
>> +    MFI_DCMD_CLUSTER_RESET_LD =         0x08010200
>> +} mfi_dcmd_t ;
> 
> 
> space before ;
> 
Fixed.

>> +
>> +/* The queue init structure that is passed with the init message */
>> +struct mfi_init_qinfo {
>> +    uint32_t flags;
>> +    uint32_t rq_entries;
>> +    uint32_t rq_addr_lo;
>> +    uint32_t rq_addr_hi;
>> +    uint32_t pi_addr_lo;
>> +    uint32_t pi_addr_hi;
>> +    uint32_t ci_addr_lo;
>> +    uint32_t ci_addr_hi;
>> +} __attribute__ ((packed));
> 
> Don't use a packed attribute - it's not necessary here and creates
> much worse code as gcc must then assume unaligned struct address.
> You had to tweak the attribute anyway, just drop it.
> 
Ok. Just had it there for consistency.

>> +
>> +/* Controller properties */
>> +struct mfi_ctrl_props {
>> +    uint16_t seq_num;
>> +    uint16_t pred_fail_poll_interval;
>> +    uint16_t intr_throttle_cnt;
>> +    uint16_t intr_throttle_timeout;
>> +    uint8_t rebuild_rate;
>> +    uint8_t patrol_read_rate;
>> +    uint8_t bgi_rate;
>> +    uint8_t cc_rate;
>> +    uint8_t recon_rate;
>> +    uint8_t cache_flush_interval;
>> +    uint8_t spinup_drv_cnt;
>> +    uint8_t spinup_delay;
>> +    uint8_t cluster_enable;
>> +    uint8_t coercion_mode;
>> +    uint8_t alarm_enable;
>> +    uint8_t disable_auto_rebuild;
>> +    uint8_t disable_battery_warn;
>> +    uint8_t ecc_bucket_size;
>> +    uint16_t ecc_bucket_leak_rate;
>> +    uint8_t restore_hotspare_on_insertion;
>> +    uint8_t expose_encl_devices;
>> +    uint8_t maintainPdFailHistory;
>> +    uint8_t disallowHostRequestReordering;
>> +    uint8_t abortCCOnError;
>> +    uint8_t loadBalanceMode;
>> +    uint8_t disableAutoDetectBackplane;
>> +    uint8_t snapVDSpace;
>> +    struct {
>> +        /* set TRUE to disable copyBack (0=copyback enabled) */
>> +        uint32_t copyBackDisabled:1,
>> +            SMARTerEnabled:1,
>> +            prCorrectUnconfiguredAreas:1,
>> +            useFdeOnly:1,
>> +            disableNCQ:1,
>> +            SSDSMARTerEnabled:1,
>> +            SSDPatrolReadEnabled:1,
>> +            enableSpinDownUnconfigured:1,
>> +            autoEnhancedImport:1,
>> +            enableSecretKeyControl:1,
>> +            disableOnlineCtrlReset:1,
>> +            allowBootWithPinnedCache:1,
>> +            disableSpinDownHS:1,
>> +            enableJBOD:1,
>> +            reserved:18;
>> +    } OnOffProperties;
> 
> Using bitfields for anything where you care about endian-ness
> is IMO wrong, and you normally do care for BE host + LE guest.
> No idea what bsd does to handle this.
> 
Well, I don't really have a choice. That the firmware interface,
which is using this kind of construct.
So I'm getting passed in a bitfield, which I then have to evaluate.
I should be okay as I'll be doing a byteshuffle before evaluation.
But yes, this interface is absolutely horrible.

[ .. ]
>> +union mfi_spare_type {
>> +    struct {
>> +        uint8_t is_dedicate:1,
>> +            is_revertable:1,
>> +            is_encl_affinity:1,
>> +            reserved:5;
>> +    } v;
>> +    uint8_t type;
>> +} __attribute__ ((packed));
>> +
>> +#define MAX_ARRAYS 16
> 
> Use a prefix to avoid global namespace pollution.
> 
>> +struct mfi_spare {
>> +    union mfi_pd_ref ref;
>> +    union mfi_spare_type spare_type;
>> +    uint8_t reserved[2];
>> +    uint8_t array_count;
>> +    uint16_t array_refd[MAX_ARRAYS];
>> +} __attribute__ ((packed));
>> +
>> +#define MAX_ROW_SIZE 32
> 
> Use a prefix to avoid global namespace pollution.
> 
Prefixed with MFI_

>> +struct mfi_array {
>> +    uint64_t size;
>> +    uint8_t num_drives;
>> +    uint8_t reserved;
>> +    uint16_t array_ref;
>> +    uint8_t pad[20];
>> +    struct {
>> +        union mfi_pd_ref ref;
>> +        uint16_t fw_state;
>> +        struct {
>> +            uint8_t pd;
>> +            uint8_t slot;
>> +        } encl;
>> +    } pd[MAX_ROW_SIZE];
>> +} __attribute__ ((packed));
>> +
>> +struct mfi_config_data {
>> +    uint32_t size;
>> +    uint16_t array_count;
>> +    uint16_t array_size;
>> +    uint16_t log_drv_count;
>> +    uint16_t log_drv_size;
>> +    uint16_t spares_count;
>> +    uint16_t spares_size;
>> +    uint8_t reserved[16];
>> +    uint8_t data;
>> +    /*
>> +      struct mfi_array  array[];
>> +      struct mfi_ld_config ld[];
>> +      struct mfi_spare  spare[];
>> +    */
>> +} __attribute__ ((packed));
>> +
>> +#define MFI_SCSI_MAX_t ARGETS  128
> 
> What's this?
> 
Ouch.

sed script gone bonkers.

>> +#define MFI_SCSI_MAX_LUNS       8
>> +#define MFI_SCSI_INITIATOR_ID 255
>> +#define MFI_SCSI_MAX_CMDS       8
>> +#define MFI_SCSI_MAX_CDB_LEN   16
>> +
>> +#endif /* _MFI_H */
> 
> make below a separate patch pls.
> 
Yeah, convinced me. Will be doing so.

>> diff --git a/hw/pci_ids.h b/hw/pci_ids.h
>> index e8235a7..0306255 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
>> -
> 
> Why? This separates base class list from storage
> subclasses.
> 
Oversight. sorry.

>>  #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
>>  
>>  #define PCI_VENDOR_ID_DEC                0x1011
>>  #define PCI_DEVICE_ID_DEC_21154          0x0026
>> -- 
>> 1.7.3.4
>>

Again, thanks for the review.
Will be merged in the next version.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)