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

[Hannes Reinecke <hare@suse.de>]

On 07/02/2011 06:14 PM, Stefan Hajnoczi wrote:
> On Fri, Jul 1, 2011 at 4:35 PM, Hannes Reinecke<hare@suse.de>  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
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)