From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:58353) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QdeeT-0003nY-6k for qemu-devel@nongnu.org; Mon, 04 Jul 2011 04:37:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QdeeQ-00029l-Mx for qemu-devel@nongnu.org; Mon, 04 Jul 2011 04:37:08 -0400 Received: from cantor2.suse.de ([195.135.220.15]:37496 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QdeeQ-00029g-7p for qemu-devel@nongnu.org; Mon, 04 Jul 2011 04:37:06 -0400 Message-ID: <4E117BAD.2040306@suse.de> Date: Mon, 04 Jul 2011 10:37:01 +0200 From: Hannes Reinecke MIME-Version: 1.0 References: <1309534555-22178-1-git-send-email-hare@suse.de> <1309534555-22178-2-git-send-email-hare@suse.de> <1309534555-22178-3-git-send-email-hare@suse.de> <1309534555-22178-4-git-send-email-hare@suse.de> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 3/3] megasas: LSI Megaraid SAS emulation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Paolo Bonzini , Alexander Graf , qemu-devel@nongnu.org, kvm@vger.kernel.org, Stefan Haynoczi On 07/02/2011 06:14 PM, Stefan Hajnoczi wrote: > On Fri, Jul 1, 2011 at 4:35 PM, Hannes Reinecke wrote: >> +static void megasas_mmio_writel(void *opaque, target_phys_addr_t addr= , >> + uint32_t val) >> +{ >> + MPTState *s =3D 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 =3D 0; i<=3D 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 =3D 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 =3D 0; >> + qemu_irq_lower(s->dev.irq[0]); >> + break; >> + case MFI_IQPH: >> + s->frame_hi =3D val; >> + break; >> + case MFI_IQPL: >> + case MFI_IQP: >> + /* Received MFI frame address */ >> + frame_addr =3D (val& ~0xFF); >> + /* Add possible 64 bit offset */ >> + frame_addr |=3D (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 =3D 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 =3D { >> + .transfer_data =3D megasas_xfer_complete, >> + .complete =3D megasas_command_complete, >> + .cancel =3D megasas_command_cancel, >> +}; >> + >> +static int megasas_scsi_init(PCIDevice *dev) >> +{ >> + MPTState *s =3D DO_UPCAST(MPTState, dev, dev); >> + uint8_t *pci_conf; >> + int i; >> + >> + pci_conf =3D 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 =3D 0 */ >> + pci_conf[0x0d] =3D 0; >> + /* Interrupt pin 1 */ >> + pci_conf[0x3d] =3D 0x01; >> + >> + s->mmio_io_addr =3D cpu_register_io_memory(megasas_mmio_readfn, >> + megasas_mmio_writefn, s, >> + DEVICE_NATIVE_ENDIAN); >> + s->io_addr =3D cpu_register_io_memory(megasas_io_readfn, >> + megasas_io_writefn, s, >> + DEVICE_NATIVE_ENDIAN); >> + s->queue_addr =3D 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_mapf= unc); >> + 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_map= func); >> + if (s->fw_sge>=3D MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) { >> + s->fw_sge =3D MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE; >> + } else if (s->fw_sge>=3D 128 - MFI_PASS_FRAME_SIZE) { >> + s->fw_sge =3D 128 - MFI_PASS_FRAME_SIZE; >> + } else { >> + s->fw_sge =3D 64 - MFI_PASS_FRAME_SIZE; >> + } >> + if (s->fw_cmds> MEGASAS_MAX_FRAMES) { >> + s->fw_cmds =3D MEGASAS_MAX_FRAMES; >> + } >> + if (s->raid_mode_str) { >> + if (!strcmp(s->raid_mode_str, "jbod")) { >> + s->is_jbod =3D 1; >> + } else { >> + s->is_jbod =3D 0; >> + } >> + } >> + DPRINTF("Using %d sges, %d cmds, %s mode\n", >> + s->fw_sge, s->fw_cmds, s->is_jbod ? "jbod" : "raid"); >> + s->fw_luns =3D (MFI_MAX_LD> MAX_SCSI_DEVS) ? >> + MAX_SCSI_DEVS : MFI_MAX_LD; >> + s->producer_pa =3D 0; >> + s->consumer_pa =3D 0; >> + for (i =3D 0; i< s->fw_cmds; i++) { >> + s->frames[i].index =3D i; >> + s->frames[i].context =3D -1; >> + s->frames[i].pa =3D 0; >> + s->frames[i].state =3D 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=20 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 --=20 Dr. Hannes Reinecke zSeries & Storage hare@suse.de +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg GF: J. Hawn, J. Guild, F. Imend=F6rffer, HRB 16746 (AG N=FCrnberg)