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 h...@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)
