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

[Hannes Reinecke]

On 07/02/2011 06:14 PM, Stefan Hajnoczi wrote:
> On Fri, Jul 1, 2011 at 4:35 PM, Hannes Reinecke<address@hidden>  wrote:
> > +static void megasas_mmio_writel(void *opaque, target_phys_addr_t addr,
> > +                                uint32_t val)
> > +{
> > +    MPTState *s = opaque;
> > +    target_phys_addr_t frame_addr;
> > +    uint32_t frame_count;
> > +    int i;
> > +
> > +    DPRINTF_REG("writel mmio %lx: %x\n", (unsigned long)addr, val);
> > +
> > +    switch (addr) {
> > +    case MFI_IDB:
> > +        if (val&  MFI_FWINIT_ABORT) {
> > +            /* Abort all pending cmds */
> > +            for (i = 0; i<= s->fw_cmds; i++) {
> > +                megasas_abort_command(&s->frames[i]);
> > +            }
> > +        }
> > +        if (val&  MFI_FWINIT_READY) {
> > +            /* move to FW READY */
> > +            megasas_soft_reset(s);
> > +        }
> > +        if (val&  MFI_FWINIT_MFIMODE) {
> > +            /* discard MFIs */
> > +        }
> > +        break;
> > +    case MFI_OMSK:
> > +        s->intr_mask = val;
> > +        if (!MEGASAS_INTR_ENABLED(s)) {
> > +            qemu_irq_lower(s->dev.irq[0]);
> > +        }
> > +        break;
> > +    case MFI_ODCR0:
> > +        /* Update reply queue pointer */
> > +        DPRINTF_QUEUE("Update reply queue head %x busy %d\n",
> > +                      s->reply_queue_index, s->busy);
> > +        stl_phys(s->producer_pa, s->reply_queue_index);
> > +        s->doorbell = 0;
> > +        qemu_irq_lower(s->dev.irq[0]);
> > +        break;
> > +    case MFI_IQPH:
> > +        s->frame_hi = val;
> > +        break;
> > +    case MFI_IQPL:
> > +    case MFI_IQP:
> > +        /* Received MFI frame address */
> > +        frame_addr = (val&  ~0xFF);
> > +        /* Add possible 64 bit offset */
> > +        frame_addr |= (uint64_t)s->frame_hi;
>
> Is this missing<<  32 before ORing the high bits?
Yes, true. Linux doesn't use the high part, so
I haven't seen it yet.
Might explain the strange Win7 failures I've had :-)
Fixed.

> > +static int megasas_scsi_uninit(PCIDevice *d)
> > +{
> > +    MPTState *s = DO_UPCAST(MPTState, dev, d);
> > +
> > +    cpu_unregister_io_memory(s->mmio_io_addr);
>
> Need to unregister io_addr and queue_addr.
Yeah, the unregister function is a bit of a stub currently.
Fixed.

> > +
> > +    return 0;
> > +}
> > +
> > +static const struct SCSIBusOps megasas_scsi_ops = {
> > +    .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 Vendor ID (word) */
> > +    pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_LSI_LOGIC);
> > +    /* PCI device ID (word) */
> > +    pci_config_set_device_id(pci_conf,  PCI_DEVICE_ID_LSI_SAS1078);
> > +    /* PCI subsystem ID */
> > +    pci_set_word(&pci_conf[PCI_SUBSYSTEM_VENDOR_ID], 0x1000);
> > +    pci_set_word(&pci_conf[PCI_SUBSYSTEM_ID], 0x1013);
> > +    /* PCI base class code */
> > +    pci_config_set_class(pci_conf, PCI_CLASS_STORAGE_RAID);
>
>
> PCIDeviceInfo now has vendor_id, device_id, and other fields.  These
> values can be set in the megasas_info definition below.
Argl. Interface change again. Okay, will be doing so.

> > +
> > +    /* PCI latency timer = 0 */
> > +    pci_conf[0x0d] = 0;
> > +    /* Interrupt pin 1 */
> > +    pci_conf[0x3d] = 0x01;
> > +
> > +    s->mmio_io_addr = cpu_register_io_memory(megasas_mmio_readfn,
> > +                                             megasas_mmio_writefn, s,
> > +                                             DEVICE_NATIVE_ENDIAN);
> > +    s->io_addr = cpu_register_io_memory(megasas_io_readfn,
> > +                                        megasas_io_writefn, s,
> > +                                        DEVICE_NATIVE_ENDIAN);
> > +    s->queue_addr = cpu_register_io_memory(megasas_queue_readfn,
> > +                                           megasas_queue_writefn, s,
> > +                                           DEVICE_NATIVE_ENDIAN);
>
> Should these be little-endian?
Presumably. Haven't tested on big-endian emulations as of now.

> > +    pci_register_bar((struct PCIDevice *)s, 0, 0x40000,
> > +                     PCI_BASE_ADDRESS_SPACE_MEMORY, megasas_mmio_mapfunc);
> > +    pci_register_bar((struct PCIDevice *)s, 2, 256,
> > +                     PCI_BASE_ADDRESS_SPACE_IO, megasas_io_mapfunc);
> > +    pci_register_bar((struct PCIDevice *)s, 3, 0x40000,
> > +                     PCI_BASE_ADDRESS_SPACE_MEMORY, megasas_queue_mapfunc);
> > +    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->is_jbod = 1;
> > +        } else {
> > +            s->is_jbod = 0;
> > +        }
> > +    }
> > +    DPRINTF("Using %d sges, %d cmds, %s mode\n",
> > +            s->fw_sge, s->fw_cmds, s->is_jbod ? "jbod" : "raid");
> > +    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;
> > +    }
>
> It is not clear to me that all register state is initialized here.
> megasas_soft_reset() seems to touch fw_state and intr_mask but will
> not be called until mmio_writel(MFI_IDB, MFI_FWINIT_READY).  Are there
> any missing fields that need to be initialized here?

I was under the impression that we'll be getting a reset after 
initialising the device, so this should've been taken care of.
And most of the mmio writes are safe against accidental misuse.
I'll be adding a check to MFI_ODCR0, as this indeed might cause
an error when used uninitialized.

Cheers,

Hannes
--
Dr. Hannes Reinecke                   zSeries & Storage
address@hidden                        +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)