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

[Michael S. Tsirkin]

On Mon, Feb 27, 2012 at 10:17:21AM +0100, Hannes Reinecke wrote:
> 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 ...

Document where's the file from as suggested below.

> >> +    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.

It is important that the configuration is fully specified
by the flags.
For example, otherwise migration to another host where
MSIX does work will then fail.


> >> 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.
> > 
> >> +
> >> +/* 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.

Bits are pretty comment as an interface.
The sane thing to do is to have some macros or enums
specifying the bits. Then you can do
le32_to_cpu(x) & MFI_DISABLE_SPIN_DOWN_HS

I don't see the shuffle in code.
E.g. info->properties.OnOffProperties.enableJBOD = 1
and no shuffle in sight.  Add a comment
here and where you do the shuffle?