megasas: Update to version 1.01

This patch updates the megasas HBA emulation to version 1.01.
It fixes the following issues:

- Remove hand-crafted inquiry command
- Remove bounce-buffer for direct commands
- Implements qdev properties to set 'max_sge', 'max_cmds'.
- Implement JBOD mode
- Improve direct command handling
- Minor cleanups

Signed-off-by: Hannes Reinecke <hare@suse.de>

On Tue, 2010-06-08 at 16:15 +0200, Hannes Reinecke wrote:
> This patch updates the megasas HBA emulation to version 1.01.
> It fixes the following issues:
> 
> - Remove hand-crafted inquiry command
> - Remove bounce-buffer for direct commands
> - Implements qdev properties to set 'max_sge', 'max_cmds'.
> - Implement JBOD mode
> - Improve direct command handling
> - Minor cleanups
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> 

Hi Hannes,

I applied your changes and everything looks good with the exception of
the new MEGASAS_DEFAULT_SGE=80 setting..

> diff --git a/hw/megasas.c b/hw/megasas.c
> index 250c3fb..19569a8 100644
> --- a/hw/megasas.c
> +++ b/hw/megasas.c
> @@ -40,38 +40,17 @@ do { fprintf(stderr, "megasas: error: " fmt , ## __VA_ARGS__);} while (0)
>  #endif
>  
>  /* Static definitions */
> -#define MEGASAS_MAX_FRAMES 1000
> -#define MEGASAS_MAX_SGE 8

<snip>

> +#define MEGASAS_VERSION "1.01"
> +#define MEGASAS_MAX_FRAMES 2048		/* Firmware limit at 65535 */
> +#define MEGASAS_DEFAULT_FRAMES 1000	/* Windows requires this */
> +#define MEGASAS_MAX_SGE 255		/* Firmware limit */
> +#define MEGASAS_DEFAULT_SGE 80

Ok, I have been running some LTP disktest raw bandwith benchmarks with a
256K blocksize with megasas -> TCM_Loop -> TCM/RAMDISK_DR LUNs into a
v2.6.26 x86_64 Linux guest (4 VCPUs and 2048 memory) and I noticed
something interesting..

With the new MEGASAS_DEFAULT_SGE 80 setting for fw_sge, read/write tests
have dropped from the original ~1050 MB/sec to roughly ~400 MB/sec.
Passing in the new qdev option using the old default of max_sge=8 the
speed jumps back up to the range that where previously observed w/o this
patch.  Going a bit further, using max_sge=16 jumps up bandwith up to
~1600 MB/sec, and max_sge=24 takes it up to ~2200 MB/sec..!  Using
max_sge=32 then sharply drops back to ~800 MB/sec, and increasing to
larger values brings bandwith down lower and lower..

Taking a look at the megaraid_sas LLD in the KVM guest, the struct
scsi_host is being registered with sg_tablesize=28 which appears to be
where the sharp dropoff for max_sge > 28 begins to occur.  I see that
MFI_DCMD_CTRL_GET_INFO is returning the configured fw_sge to the guest,
but AFAICT megaraid_sas does not adjust itself to use the larger value
reported by GET_INFO.

So that said, I think we want to use MEGASAS_DEFAULT_SGE 28 to match
what the Linux driver is using.  I have not checked what the equivlient
sg_tablesize for the MSFT LLD is doing, but it appears we need to error
on the conserative side here.  What do you think..?

Best,

--nab

Nicholas A. Bellinger wrote:
> On Tue, 2010-06-08 at 16:15 +0200, Hannes Reinecke wrote:
>> This patch updates the megasas HBA emulation to version 1.01.
>> It fixes the following issues:
>>
>> - Remove hand-crafted inquiry command
>> - Remove bounce-buffer for direct commands
>> - Implements qdev properties to set 'max_sge', 'max_cmds'.
>> - Implement JBOD mode
>> - Improve direct command handling
>> - Minor cleanups
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>
> 
> Hi Hannes,
> 
> I applied your changes and everything looks good with the exception of
> the new MEGASAS_DEFAULT_SGE=80 setting..
> 
>> diff --git a/hw/megasas.c b/hw/megasas.c
>> index 250c3fb..19569a8 100644
>> --- a/hw/megasas.c
>> +++ b/hw/megasas.c
>> @@ -40,38 +40,17 @@ do { fprintf(stderr, "megasas: error: " fmt , ## __VA_ARGS__);} while (0)
>>  #endif
>>  
>>  /* Static definitions */
>> -#define MEGASAS_MAX_FRAMES 1000
>> -#define MEGASAS_MAX_SGE 8
> 
> <snip>
> 
>> +#define MEGASAS_VERSION "1.01"
>> +#define MEGASAS_MAX_FRAMES 2048		/* Firmware limit at 65535 */
>> +#define MEGASAS_DEFAULT_FRAMES 1000	/* Windows requires this */
>> +#define MEGASAS_MAX_SGE 255		/* Firmware limit */
>> +#define MEGASAS_DEFAULT_SGE 80
> 
> Ok, I have been running some LTP disktest raw bandwith benchmarks with a
> 256K blocksize with megasas -> TCM_Loop -> TCM/RAMDISK_DR LUNs into a
> v2.6.26 x86_64 Linux guest (4 VCPUs and 2048 memory) and I noticed
> something interesting..
> 
> With the new MEGASAS_DEFAULT_SGE 80 setting for fw_sge, read/write tests
> have dropped from the original ~1050 MB/sec to roughly ~400 MB/sec.
> Passing in the new qdev option using the old default of max_sge=8 the
> speed jumps back up to the range that where previously observed w/o this
> patch.  Going a bit further, using max_sge=16 jumps up bandwith up to
> ~1600 MB/sec, and max_sge=24 takes it up to ~2200 MB/sec..!  Using
> max_sge=32 then sharply drops back to ~800 MB/sec, and increasing to
> larger values brings bandwith down lower and lower..
> 
> Taking a look at the megaraid_sas LLD in the KVM guest, the struct
> scsi_host is being registered with sg_tablesize=28 which appears to be
> where the sharp dropoff for max_sge > 28 begins to occur.  I see that
> MFI_DCMD_CTRL_GET_INFO is returning the configured fw_sge to the guest,
> but AFAICT megaraid_sas does not adjust itself to use the larger value
> reported by GET_INFO.
> 
Thanks for confirmation. You just confirmed _why_ I made
the SGE setting configurable.

The SGE default setting as found on 'real' HBAs is in fact 80,
hence this value.
However, I always suspected that we will have problems with
direct SGL mapping if the settings from the underlying hardware
and the emulation don't match.
Which was the reason for the LSF discussion topic, if you remember :-)
So thanks for the confirmation here.

Hence I made the SGE setting configurable, so that it can be
adjusted (manually for starters) to the underlying hardware.
If you do a:

-device megasas,id=megasas,max_sge=28,mode=jbod

you have the desired behaviour.

Currently we cannot do this tuning automatically; we just have
_one_ setting for the entire HBA emulation whereas the underlying
disks connected to the megasas might have different settings.

Again, the proper handling here should be discussed on the LSF.

> So that said, I think we want to use MEGASAS_DEFAULT_SGE 28 to match
> what the Linux driver is using.  I have not checked what the equivlient
> sg_tablesize for the MSFT LLD is doing, but it appears we need to error
> on the conserative side here.  What do you think..?
> 
As said, this is _not_ what linux is using. This is what you particular
HBA is using. On one of my machines I have:

cat /sys/class/scsi_host/host?/sg_tablesize 
128
128
128
64
64
128
128

So maybe you should consider updating your HBA ...

I would advocate setting it to the real HBA setting of
80 (which works just find for file-based backends)
and have it adjusted manually if an sg-based backend
is used.

Proper handling here will be discussed at LSF.

Cheers,

Hannes

On Wed, 2010-06-09 at 03:14 -0700, Nicholas A. Bellinger wrote:
> On Tue, 2010-06-08 at 16:15 +0200, Hannes Reinecke wrote:
> > This patch updates the megasas HBA emulation to version 1.01.
> > It fixes the following issues:
> > 
> > - Remove hand-crafted inquiry command
> > - Remove bounce-buffer for direct commands
> > - Implements qdev properties to set 'max_sge', 'max_cmds'.
> > - Implement JBOD mode
> > - Improve direct command handling
> > - Minor cleanups
> > 
> > Signed-off-by: Hannes Reinecke <hare@suse.de>
> > 
> 
> Hi Hannes,
> 
> I applied your changes and everything looks good with the exception of
> the new MEGASAS_DEFAULT_SGE=80 setting..
> 
> > diff --git a/hw/megasas.c b/hw/megasas.c
> > index 250c3fb..19569a8 100644
> > --- a/hw/megasas.c
> > +++ b/hw/megasas.c
> > @@ -40,38 +40,17 @@ do { fprintf(stderr, "megasas: error: " fmt , ## __VA_ARGS__);} while (0)
> >  #endif
> >  
> >  /* Static definitions */
> > -#define MEGASAS_MAX_FRAMES 1000
> > -#define MEGASAS_MAX_SGE 8
> 
> <snip>
> 
> > +#define MEGASAS_VERSION "1.01"
> > +#define MEGASAS_MAX_FRAMES 2048		/* Firmware limit at 65535 */
> > +#define MEGASAS_DEFAULT_FRAMES 1000	/* Windows requires this */
> > +#define MEGASAS_MAX_SGE 255		/* Firmware limit */
> > +#define MEGASAS_DEFAULT_SGE 80
> 
> Ok, I have been running some LTP disktest raw bandwith benchmarks with a
> 256K blocksize with megasas -> TCM_Loop -> TCM/RAMDISK_DR LUNs into a
> v2.6.26 x86_64 Linux guest (4 VCPUs and 2048 memory) and I noticed
> something interesting..
> 
> With the new MEGASAS_DEFAULT_SGE 80 setting for fw_sge, read/write tests
> have dropped from the original ~1050 MB/sec to roughly ~400 MB/sec.
> Passing in the new qdev option using the old default of max_sge=8 the
> speed jumps back up to the range that where previously observed w/o this
> patch.  Going a bit further, using max_sge=16 jumps up bandwith up to
> ~1600 MB/sec, and max_sge=24 takes it up to ~2200 MB/sec..!  Using
> max_sge=32 then sharply drops back to ~800 MB/sec, and increasing to
> larger values brings bandwith down lower and lower..
> 
> Taking a look at the megaraid_sas LLD in the KVM guest, the struct
> scsi_host is being registered with sg_tablesize=28 which appears to be
> where the sharp dropoff for max_sge > 28 begins to occur.  I see that
> MFI_DCMD_CTRL_GET_INFO is returning the configured fw_sge to the guest,
> but AFAICT megaraid_sas does not adjust itself to use the larger value
> reported by GET_INFO.
> 

Actually scratch that last part about the megaraid_sas LLD..

drivers/scsi/megaraid/megaraid_sas.c:megasas_io_attach() is setting the
MFI GET_INFO returned fw_sge value to struct scsi_host->sg_tablesize
before calling scsi_add_host(), but there still appears to be an
performance issue somewhere when using max_sge > 28..

Note for TCM_Loop LLD running on KVM host we are currently using a
default of sg_tablesize=256.  Running the same LTP disktest benchmark
with TCM/RAMDISK LUNs as above for a Linux guest on a baremetal KVM host
(5500 series Nehalem) with v2.6.34 I am now seeing ~12900 MB/sec (yes,
100 Gbit/sec) of sustained read/write bandwith into the same single
Linux/SCSI LUN.  So AFAICT the limiting factor with the larger megasas
max_sge mentioned above appears to be outside of any bottlenecks that
may exist in Linux/SCSI or TCM_Loop+SG_IO backstore used with megasas.

Best,

--nab

On Wed, 2010-06-09 at 12:32 +0200, Hannes Reinecke wrote:
> Nicholas A. Bellinger wrote:
> > Hi Hannes,
> > 
> > I applied your changes and everything looks good with the exception of
> > the new MEGASAS_DEFAULT_SGE=80 setting..
> > 
> >> diff --git a/hw/megasas.c b/hw/megasas.c
> >> index 250c3fb..19569a8 100644
> >> --- a/hw/megasas.c
> >> +++ b/hw/megasas.c
> >> @@ -40,38 +40,17 @@ do { fprintf(stderr, "megasas: error: " fmt , ## __VA_ARGS__);} while (0)
> >>  #endif
> >>  
> >>  /* Static definitions */
> >> -#define MEGASAS_MAX_FRAMES 1000
> >> -#define MEGASAS_MAX_SGE 8
> > 
> > <snip>
> > 
> >> +#define MEGASAS_VERSION "1.01"
> >> +#define MEGASAS_MAX_FRAMES 2048		/* Firmware limit at 65535 */
> >> +#define MEGASAS_DEFAULT_FRAMES 1000	/* Windows requires this */
> >> +#define MEGASAS_MAX_SGE 255		/* Firmware limit */
> >> +#define MEGASAS_DEFAULT_SGE 80
> > 
> > Ok, I have been running some LTP disktest raw bandwith benchmarks with a
> > 256K blocksize with megasas -> TCM_Loop -> TCM/RAMDISK_DR LUNs into a
> > v2.6.26 x86_64 Linux guest (4 VCPUs and 2048 memory) and I noticed
> > something interesting..
> > 
> > With the new MEGASAS_DEFAULT_SGE 80 setting for fw_sge, read/write tests
> > have dropped from the original ~1050 MB/sec to roughly ~400 MB/sec.
> > Passing in the new qdev option using the old default of max_sge=8 the
> > speed jumps back up to the range that where previously observed w/o this
> > patch.  Going a bit further, using max_sge=16 jumps up bandwith up to
> > ~1600 MB/sec, and max_sge=24 takes it up to ~2200 MB/sec..!  Using
> > max_sge=32 then sharply drops back to ~800 MB/sec, and increasing to
> > larger values brings bandwith down lower and lower..
> > 
> > Taking a look at the megaraid_sas LLD in the KVM guest, the struct
> > scsi_host is being registered with sg_tablesize=28 which appears to be
> > where the sharp dropoff for max_sge > 28 begins to occur.  I see that
> > MFI_DCMD_CTRL_GET_INFO is returning the configured fw_sge to the guest,
> > but AFAICT megaraid_sas does not adjust itself to use the larger value
> > reported by GET_INFO.
> > 
> Thanks for confirmation. You just confirmed _why_ I made
> the SGE setting configurable.
> 
> The SGE default setting as found on 'real' HBAs is in fact 80,
> hence this value.
> However, I always suspected that we will have problems with
> direct SGL mapping if the settings from the underlying hardware
> and the emulation don't match.
> Which was the reason for the LSF discussion topic, if you remember :-)
> So thanks for the confirmation here.

Indeed, I was looking at best case large block bandwith with
TCM/RAMDISK_DR and zero-copy struct scatterlist mapping with the
can_queue and max_sectors using 1024.   Having a TCM IBLOCK/FILEIO/pSCSI
backstore for a real backend struct block_device is going to have a
certain overhead compared to raw struct page ramdisk, but I think the
RAMDISK_DR subsystem plugin gives us a good idea of where we are at with
TCM_Loop struct scsi_devices..  ;)

> 
> Hence I made the SGE setting configurable, so that it can be
> adjusted (manually for starters) to the underlying hardware.
> If you do a:
> 
> -device megasas,id=megasas,max_sge=28,mode=jbod
> 
> you have the desired behaviour.

Perfect.. I will check out mode=jbod as well..

> 
> Currently we cannot do this tuning automatically; we just have
> _one_ setting for the entire HBA emulation whereas the underlying
> disks connected to the megasas might have different settings.
> 
> Again, the proper handling here should be discussed on the LSF.
> 

<nod>

> > So that said, I think we want to use MEGASAS_DEFAULT_SGE 28 to match
> > what the Linux driver is using.  I have not checked what the equivlient
> > sg_tablesize for the MSFT LLD is doing, but it appears we need to error
> > on the conserative side here.  What do you think..?
> > 
> As said, this is _not_ what linux is using. This is what you particular
> HBA is using. On one of my machines I have:
> 
> cat /sys/class/scsi_host/host?/sg_tablesize 
> 128
> 128
> 128
> 64
> 64
> 128
> 128
> 
> So maybe you should consider updating your HBA ...
> 

Yes, my mistake.  megaraid_sas is actually querying for it's struct
scsi_host->sg_tablesize..

> I would advocate setting it to the real HBA setting of
> 80 (which works just find for file-based backends)
> and have it adjusted manually if an sg-based backend
> is used.
> 

Hmm, then it appears that there is a known bottleneck somewhere in the
v2.6.26 Linux guest stack or perhaps somewhere else or something with
SG_IO..?

I am still using include/scsi/sg.h:SG_MAX_QUEUE 128, but I am not sure
if this would be effectted by the larger max_sge too..?  I am also
wondering if the conversion to use BSG here will have an effect with the
larger max_sge values..?

Best,

--nab

On Tue, Jun 8, 2010 at 2:15 PM, Hannes Reinecke <hare@suse.de> wrote:
>
> This patch updates the megasas HBA emulation to version 1.01.
> It fixes the following issues:
>
> - Remove hand-crafted inquiry command
> - Remove bounce-buffer for direct commands
> - Implements qdev properties to set 'max_sge', 'max_cmds'.
> - Implement JBOD mode
> - Improve direct command handling
> - Minor cleanups
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
>
> diff --git a/hw/megasas.c b/hw/megasas.c
> index 250c3fb..19569a8 100644
> --- a/hw/megasas.c
> +++ b/hw/megasas.c
> @@ -40,38 +40,17 @@ do { fprintf(stderr, "megasas: error: " fmt , ## __VA_ARGS__);} while (0)
>  #endif
>
>  /* Static definitions */
> -#define MEGASAS_MAX_FRAMES 1000
> -#define MEGASAS_MAX_SGE 8
> -#define MEGASAS_MAX_LUNS 128
> -
> -/* Frame definitions */
> -#define MEGASAS_FRAME_CMD_OFFSET               0x00
> -#define MEGASAS_FRAME_SENSE_LEN_OFFSET         0x01
> -#define MEGASAS_FRAME_CMD_STATUS_OFFSET        0x02
> -#define MEGASAS_FRAME_SCSI_STATUS_OFFSET       0x03
> -#define MEGASAS_FRAME_TARGET_ID_OFFSET         0x04
> -#define MEGASAS_FRAME_LUN_ID_OFFSET            0x05
> -#define MEGASAS_FRAME_CDB_LEN_OFFSET           0x06
> -#define MEGASAS_FRAME_SGE_COUNT_OFFSET         0x07
> -#define MEGASAS_FRAME_CONTEXT_OFFSET           0x08
> -#define MEGASAS_FRAME_FLAGS_OFFSET             0x10
> -#define MEGASAS_FRAME_XFER_LEN_OFFSET          0x14
> -
> -#define MEGASAS_DCMD_SGL_OFFSET                        0x28
> -
> -#define MEGASAS_PTHRU_SGL_OFFSET               0x30
> -
> -#define MEGASAS_IO_SGL_OFFSET                  0x28
> +#define MEGASAS_VERSION "1.01"
> +#define MEGASAS_MAX_FRAMES 2048                /* Firmware limit at 65535 */
> +#define MEGASAS_DEFAULT_FRAMES 1000    /* Windows requires this */
> +#define MEGASAS_MAX_SGE 255            /* Firmware limit */
> +#define MEGASAS_DEFAULT_SGE 80
> +#define MEGASAS_MAX_ARRAYS 128
>
>  const char *mfi_frame_desc[] = {
>     "MFI init", "LD Read", "LD Write", "LD SCSI", "PD SCSI",
>     "MFI Doorbell", "MFI Abort", "MFI SMP", "MFI Stop"};
>
> -struct megasas_lun_t {
> -    SCSIDevice *sdev;
> -    BlockDriverAIOCB *aiocb;
> -};
> -
>  struct megasas_cmd_t {
>     int index;
>
> @@ -81,8 +60,8 @@ struct megasas_cmd_t {
>     QEMUSGList sg;
>     void *iov_buf;
>     long iov_size;
> +    SCSIDevice *sdev;
>     struct megasas_state_t *state;
> -    struct megasas_lun_t *lun;
>  };
>
>  typedef struct megasas_state_t {
> @@ -93,12 +72,14 @@ typedef struct megasas_state_t {
>     uint32_t frame_hi;
>
>     int fw_state;
> -    int fw_sge;
> -    int fw_cmds;
> +    uint32_t fw_sge;
> +    uint32_t fw_cmds;
>     int fw_luns;
>     int intr_mask;
>     int doorbell;
>     int busy;
> +    char *raid_mode_str;
> +    int is_jbod;
>
>     int event_count;
>     int shutdown_event;
> @@ -113,8 +94,6 @@ typedef struct megasas_state_t {
>
>     struct megasas_cmd_t frames[MEGASAS_MAX_FRAMES];
>
> -    struct megasas_lun_t luns[MEGASAS_MAX_LUNS];
> -
>     SCSIBus bus;
>  } MPTState;
>
> @@ -123,13 +102,19 @@ typedef struct megasas_state_t {
>  #define MEGASAS_INTR_ENABLED(s) (((s)->intr_mask & MEGASAS_INTR_DISABLED_MASK ) != MEGASAS_INTR_DISABLED_MASK)
>
>  #define megasas_frame_set_cmd_status(f,v)              \
> -    stb_phys((f) + MEGASAS_FRAME_CMD_STATUS_OFFSET, v);
> +    stb_phys((f) + offsetof(struct mfi_frame_header, cmd_status), v);
>
>  #define megasas_frame_set_sense_len(f,v)               \
> -    stb_phys((f) + MEGASAS_FRAME_SENSE_LEN_OFFSET, v);
> +    stb_phys((f) + offsetof(struct mfi_frame_header, sense_len), v);
>
>  #define megasas_frame_set_scsi_status(f,v)             \
> -    stb_phys((f) + MEGASAS_FRAME_SCSI_STATUS_OFFSET, v);
> +    stb_phys((f) + offsetof(struct mfi_frame_header, scsi_status), v);
> +
> +#define megasas_frame_get_cmd(f)                       \
> +    ldub_phys((f) + offsetof(struct mfi_frame_header, frame_cmd))
> +
> +#define megasas_frame_get_context(f)                   \
> +    ldl_phys(frame_addr + offsetof(struct mfi_frame_header, context));
>
>  static void megasas_soft_reset(MPTState *s);
>
> @@ -181,37 +166,20 @@ static void megasas_build_sense(struct megasas_cmd_t *cmd, SCSISense sense)
>     qemu_free(sense_ptr);
>  }
>
> -static int megasas_get_inq(SCSIDevice *sdev, int pg, uint8_t *buf, int buflen)
> -{
> -    SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, sdev->qdev.parent_bus);
> -
> -    memset(buf, 0, buflen);
> -    if (pg == 0) {
> -       memcpy(&buf[16], "QEMU HARDDISK  ", 16);
> -       memcpy(&buf[8], "QEMU   ", 8);
> -       memcpy(&buf[32], QEMU_VERSION, 4);
> -       /* Identify device as SCSI-3 rev 1 */
> -       buf[2] = 3;
> -       buf[3] = 2; /* Format 2 */
> -       buf[4] = buflen - 5; /* Additional Length = (Len - 1) - 4 */
> -       /* Sync data transfer and TCQ.  */
> -       buf[7] = 0x10 | (bus->tcq ? 0x02 : 0);
> -    } else if (pg == 0x83) {
> -       int id_len = strlen(bdrv_get_device_name(sdev->conf.dinfo->bdrv));
> -
> -       buflen = 0;
> -       buf[buflen++] = 0;
> -       buf[buflen++] = pg;
> -       buf[buflen++] = 0x00;
> -       buf[buflen++] = 3 + id_len;
> -       buf[buflen++] = 0x2; // ASCII
> -       buf[buflen++] = 0;   // not officially assigned
> -       buf[buflen++] = 0;   // reserved
> -       buf[buflen++] = id_len; // length of data following
> -       memcpy(buf + buflen, bdrv_get_device_name(sdev->conf.dinfo->bdrv), id_len);
> -       buflen += id_len;
> +static int megasas_setup_inquiry(SCSIRequest *req, int pg,
> +                                uint8_t *buf, int len)
> +{
> +    uint8_t cdb[6] = { INQUIRY, 0, 0, 0, 0, 0};
> +
> +    if (pg > 0) {
> +       cdb[1] = 0x1;
> +       cdb[2] = pg;
>     }
> -    return buflen;
> +    cdb[3] = (len >> 8) & 0xff;
> +    cdb[4] = (len & 0xff);
> +    scsi_req_parse(req, cdb);
> +    scsi_req_buf(req, buf);
> +    return len;
>  }
>
>  /*
> @@ -234,6 +202,16 @@ static uint64_t megasas_fw_time(void)
>     return bcd_time;
>  }
>
> +static uint64_t megasas_gen_sas_addr(unsigned long id)
> +{
> +    uint64_t addr;
> +
> +    addr = ((uint64_t)0x5001a4a << 36);

With 0x5001a4aULL the cast could be avoided.

> +    addr |= ((uint64_t)id & 0xfffffffff);

This cast could be avoided by making id uint64_t.

> +
> +    return addr;
> +}
> +
>  /*
>  * Frame handling
>  */
> @@ -254,7 +232,7 @@ static inline struct megasas_cmd_t *megasas_lookup_frame(MPTState *s,
>
>     index = s->reply_queue_index;
>
> -    while (num < MEGASAS_MAX_FRAMES) {
> +    while (num < s->fw_cmds) {
>        if (s->frames[index].pa && s->frames[index].pa == frame) {
>            cmd = &s->frames[index];
>            break;
> @@ -282,7 +260,7 @@ static inline struct megasas_cmd_t *megasas_next_frame(MPTState *s,
>     }
>     index = s->reply_queue_index;
>     num = 0;
> -    while (num < MEGASAS_MAX_FRAMES) {
> +    while (num < s->fw_cmds) {
>        if (!s->frames[index].pa) {
>            cmd = &s->frames[index];
>            break;
> @@ -377,7 +355,7 @@ static void megasas_dump_frame(struct megasas_cmd_t *cmd)
>
>  static int megasas_finish_command(MPTState *s, struct megasas_cmd_t *cmd)
>  {
> -    int context;
> +    int context = -1;
>
>     if (!cmd) {
>  #ifdef DEBUG_MEGASAS_QUEUE
> @@ -386,8 +364,8 @@ static int megasas_finish_command(MPTState *s, struct megasas_cmd_t *cmd)
>        s->event_count++;
>        return -1;
>     }
> -    context = cmd->frame->header.context;
> -    cmd->lun = NULL;
> +    if (cmd->frame)
> +       context = cmd->frame->header.context;
>
>     return context;
>  }
> @@ -483,14 +461,14 @@ static int megasas_finish_dcmd(struct megasas_cmd_t *cmd, uint32_t size)
>     }
>     cpu_physical_memory_unmap(cmd->iov_buf, cmd->iov_size, 1, size);
>     if (cmd->iov_size > size)
> -       stl_phys(cmd->pa + MEGASAS_DCMD_SGL_OFFSET + sgl_addr_size, size);
> +       stl_phys(cmd->pa + offsetof(struct mfi_dcmd_frame,sgl) + sgl_addr_size, size);
>
>     return size;
>  }
>
>  static int megasas_ctrl_get_info(MPTState *s, struct megasas_cmd_t *cmd)
>  {
> -    struct mfi_ctrl_info info;
> +    struct mfi_ctrl_info *info = cmd->iov_buf;
>     int n, num_ld_disks = 0;
>
>     for (n = 0; n < s->fw_luns; n++) {
> @@ -498,134 +476,189 @@ static int megasas_ctrl_get_info(MPTState *s, struct megasas_cmd_t *cmd)
>            num_ld_disks++;
>     }
>
> -    memset(&info, 0x0, sizeof(info));
> -    info.pci.vendor = PCI_VENDOR_ID_LSI_LOGIC;
> -    info.pci.device = PCI_DEVICE_ID_LSI_SAS1078;
> -    info.pci.subvendor = PCI_VENDOR_ID_LSI_LOGIC;
> -    info.pci.subdevice = 0x1013;
> -
> -    info.host.type = MFI_INFO_HOST_PCIX;
> -    info.device.type = MFI_INFO_DEV_SAS3G;
> -    info.device.port_count = 2;
> -
> -    memcpy(info.product_name,"MegaRAID SAS 8708EM2", 20);
> -    sprintf(info.package_version,"%s-QEMU", QEMU_VERSION);
> -    info.current_fw_time = megasas_fw_time();
> -    info.max_arms = 32;
> -    info.max_spans = 8;
> -    info.max_arrays = MEGASAS_MAX_LUNS;
> -    info.max_lds = s->fw_luns;
> -    info.max_cmds = s->fw_cmds;
> -    info.max_sg_elements = s->fw_sge;
> -    info.max_request_size = 8192;
> -    info.lds_present = num_ld_disks;
> -    info.pd_present = num_ld_disks + 1;
> -    info.pd_disks_present = num_ld_disks;
> -    info.memory_size = 512;
> -    info.nvram_size = 32;
> -    info.flash_size = 16;
> -    info.raid_levels = MFI_INFO_RAID_0;
> -    info.adapter_ops = MFI_INFO_AOPS_RBLD_RATE |
> +    memset(cmd->iov_buf, 0x0, cmd->iov_size);
> +    if (cmd->iov_size != sizeof(struct mfi_ctrl_info)) {
> +#ifdef DEBUG_MEGASAS_DCMD
> +       DPRINTF("Ctrl Get Info: invalid xfer_len %ld\n", cmd->iov_size);
> +#endif

Please avoid the #ifdef/#endif with a new macro DPRINTF_DCMD() if necessary.

> +       return MFI_STAT_INVALID_PARAMETER;
> +    }
> +
> +#ifdef DEBUG_MEGASAS_DCMD
> +    DPRINTF("MFI DCMD get controller info\n");
> +#endif
> +    info->pci.vendor = PCI_VENDOR_ID_LSI_LOGIC;
> +    info->pci.device = PCI_DEVICE_ID_LSI_SAS1078;
> +    info->pci.subvendor = PCI_VENDOR_ID_LSI_LOGIC;
> +    info->pci.subdevice = 0x1013;
> +
> +    info->host.type = MFI_INFO_HOST_PCIX;
> +    info->device.type = MFI_INFO_DEV_SAS3G;
> +    info->device.port_count = 2;
> +    info->device.port_addr[0] = megasas_gen_sas_addr((unsigned long)s);
> +
> +    memcpy(info->product_name,"MegaRAID SAS 8708EM2", 20);
> +    sprintf(info->serial_number,"QEMU%08lx",(unsigned long)s & 0xFFFFFFFF);

Please use snprintf(), OpenBSD linker issues warnings for all uses of sprintf().

> +    sprintf(info->package_version,"%s-QEMU", QEMU_VERSION);
> +    strcpy(info->image_component[0].name, "APP");

Same problem with strcpy(), please use pstrcpy(), snprintf() or memcpy().

> +    strcpy(info->image_component[0].version, MEGASAS_VERSION "-QEMU");
> +    strcpy(info->image_component[0].build_date, __DATE__);
> +    strcpy(info->image_component[0].build_time, __TIME__);
> +    info->image_component_count = 1;
> +    info->current_fw_time = megasas_fw_time();
> +    info->max_arms = 32;
> +    info->max_spans = 8;
> +    info->max_arrays = MEGASAS_MAX_ARRAYS;
> +    info->max_lds = s->fw_luns;
> +    info->max_cmds = s->fw_cmds;
> +    info->max_sg_elements = s->fw_sge;
> +    info->max_request_size = 8192;
> +    info->lds_present = num_ld_disks;
> +    info->pd_present = num_ld_disks + 1;
> +    info->pd_disks_present = num_ld_disks;
> +    info->hw_present = MFI_INFO_HW_NVRAM | MFI_INFO_HW_MEM | MFI_INFO_HW_FLASH;
> +    info->memory_size = 512;
> +    info->nvram_size = 32;
> +    info->flash_size = 16;
> +    info->raid_levels = MFI_INFO_RAID_0;
> +    info->adapter_ops = MFI_INFO_AOPS_RBLD_RATE |
>         MFI_INFO_AOPS_SELF_DIAGNOSTIC |
>         MFI_INFO_AOPS_MIXED_ARRAY;
> -    info.ld_ops = MFI_INFO_LDOPS_DISK_CACHE_POLICY |
> +    info->ld_ops = MFI_INFO_LDOPS_DISK_CACHE_POLICY |
>         MFI_INFO_LDOPS_ACCESS_POLICY |
>         MFI_INFO_LDOPS_IO_POLICY |
>         MFI_INFO_LDOPS_WRITE_POLICY |
>         MFI_INFO_LDOPS_READ_POLICY;
> -    info.stripe_sz_ops.min = 4;
> -    info.stripe_sz_ops.max = 0xf;
> -    info.properties.pred_fail_poll_interval = 300;
> -    info.properties.intr_throttle_cnt = 16;
> -    info.pd_ops = 0x3;
> -    info.pd_mix_support = MFI_INFO_PDMIX_SAS | MFI_INFO_PDMIX_LD;
> -    memcpy(cmd->iov_buf, (uint8_t *)&info, sizeof(info));
> +    info->max_strips_per_io = 42;
> +    info->stripe_sz_ops.min = 4;
> +    info->stripe_sz_ops.max = 0xf;
> +    info->properties.pred_fail_poll_interval = 300;
> +    info->properties.intr_throttle_cnt = 16;
> +    info->properties.intr_throttle_timeout = 50;
> +    info->properties.rebuild_rate = 30;
> +    info->properties.patrol_read_rate = 30;
> +    info->properties.bgi_rate = 30;
> +    info->properties.cc_rate = 30;
> +    info->properties.recon_rate = 30;
> +    info->properties.cache_flush_interval = 4;
> +    info->properties.spinup_drv_cnt = 2;
> +    info->properties.spinup_delay = 6;
> +    info->properties.ecc_bucket_size = 15;
> +    info->properties.ecc_bucket_leak_rate = 1440;
> +    info->properties.expose_encl_devices = 1;
> +    info->pd_ops = MFI_INFO_PDOPS_FORCE_ONLINE | MFI_INFO_PDOPS_FORCE_OFFLINE;
> +    info->pd_mix_support = MFI_INFO_PDMIX_SAS | MFI_INFO_PDMIX_SATA | MFI_INFO_PDMIX_LD;
>
> -    return sizeof(info);
> +    return MFI_STAT_OK;
>  }
>
>  static int megasas_mfc_get_defaults(MPTState *s, struct megasas_cmd_t *cmd)
>  {
> -    struct mfi_defaults info;
> +    struct mfi_defaults *info = cmd->iov_buf;
>
> -    memset(&info, 0x0, sizeof(info));
> +    memset(cmd->iov_buf, 0x0, cmd->iov_size);
> +    if (cmd->iov_size != sizeof(struct mfi_defaults)) {
> +#ifdef DEBUG_MEGASAS_DCMD
> +       DPRINTF("MFC Get defaults: invalid xfer_len %ld\n", cmd->iov_size);
> +#endif
> +       return MFI_STAT_INVALID_PARAMETER;
> +    }
>
> -    info.stripe_size = 8;
> -    info.flush_time = 4;
> -    info.background_rate = 30;
> -    info.allow_mix_in_enclosure = 1;
> -    info.allow_mix_in_ld = 1;
> -    info.direct_pd_mapping = 1;
> -    info.bios_enumerate_lds = 1;
> -    info.disable_ctrl_r = 1;
> -    memcpy(cmd->iov_buf, (uint8_t *)&info, sizeof(info));
> +    info->stripe_size = 8;
> +    info->flush_time = 4;
> +    info->background_rate = 30;
> +    info->allow_mix_in_enclosure = 1;
> +    info->allow_mix_in_ld = 1;
> +    info->direct_pd_mapping = 1;
> +    info->bios_enumerate_lds = 1;
> +    info->disable_ctrl_r = 1;
> +    info->expose_enclosure_devices = 1;
> +    info->disable_preboot_cli = 1;
> +    info->cluster_disable = 1;
>
> -    return sizeof(info);
> +    return MFI_STAT_OK;
>  }
>
> -static int megasas_dcmd_get_bios_info(MPTState *d, struct megasas_cmd_t *cmd)
> +static int megasas_dcmd_get_bios_info(MPTState *s, struct megasas_cmd_t *cmd)
>  {
> -    struct mfi_bios_data info;
> +    struct mfi_bios_data *info = cmd->iov_buf;
>
> -    memset(&info, 0x0, sizeof(info));
> -    info.continue_on_error = 1;
> -    memcpy(cmd->iov_buf, (uint8_t *)&info, sizeof(info));
> +    memset(cmd->iov_buf, 0x0, cmd->iov_size);
> +    if (cmd->iov_size != sizeof(struct mfi_bios_data)) {
> +#ifdef DEBUG_MEGASAS_DCMD
> +       DPRINTF("Get BIOS info: invalid xfer_len %ld\n", cmd->iov_size);
> +#endif
> +       return MFI_STAT_INVALID_PARAMETER;
> +    }
> +    info->continue_on_error = 1;
>
> -return sizeof(info);
> +    return MFI_STAT_OK;
>  }
>
> -static int megasas_dcmd_get_fw_time(MPTState *d, struct megasas_cmd_t *cmd)
> +static int megasas_dcmd_get_fw_time(MPTState *s, struct megasas_cmd_t *cmd)
>  {
>     uint64_t fw_time;
>
>     fw_time = megasas_fw_time();
> -    memcpy(cmd->iov_buf, (uint8_t *)&fw_time, sizeof(fw_time));
>
> -    return sizeof(fw_time);
> +    memcpy(cmd->iov_buf, &fw_time, sizeof(fw_time));
> +    return MFI_STAT_OK;
>  }
>
> -static int megasas_dcmd_set_fw_time(MPTState *d, struct megasas_cmd_t *cmd)
> +static int megasas_dcmd_set_fw_time(MPTState *s, struct megasas_cmd_t *cmd)
>  {
>     uint64_t fw_time;
>
> +    memset(cmd->iov_buf, 0x0, cmd->iov_size);
>     memcpy(&fw_time, cmd->frame->dcmd.mbox, sizeof(fw_time));
>     DPRINTF("set fw time %lx\n", fw_time);
>     fw_time = megasas_fw_time();
> -    memcpy(cmd->iov_buf, (uint8_t *)&fw_time, sizeof(fw_time));
> -
> -    return sizeof(fw_time);
> +    memcpy(cmd->iov_buf, &fw_time, sizeof(fw_time));
> +    return MFI_STAT_OK;
>  }
>
> -
>  static int megasas_event_info(MPTState *s, struct megasas_cmd_t *cmd)
>  {
> -    struct mfi_evt_log_state info;
> +    struct mfi_evt_log_state *info = cmd->iov_buf;
>
> -    memset(&info, 0, sizeof(info));
> -    info.newest_seq_num = s->event_count;
> -    info.shutdown_seq_num = s->shutdown_event;
> -    info.boot_seq_num = s->boot_event;
> +    memset(info, 0, cmd->iov_size);
> +    info->newest_seq_num = s->event_count;
> +    info->shutdown_seq_num = s->shutdown_event;
> +    info->boot_seq_num = s->boot_event;
>
> -    memcpy(cmd->iov_buf, (uint8_t *)&info, sizeof(info));
> -
> -    return sizeof(info);
> +    return MFI_STAT_OK;
>  }
>
>  static int megasas_dcmd_pd_get_list(MPTState *s, struct megasas_cmd_t *cmd)
>  {
> -    struct mfi_pd_list info;
> -    uint32_t offset, num_pd_disks = 0;
> +    struct mfi_pd_list *info = cmd->iov_buf;
> +    uint32_t offset, num_pd_disks = 0, max_luns;
>     uint16_t dev_id;
>
> -    memset(&info, 0x0, sizeof(info));
> +    memset(cmd->iov_buf, 0, cmd->iov_size);
>     offset = 8;
> -    for (dev_id = 0; dev_id < s->fw_luns; dev_id++) {
> +    if (cmd->iov_size < (offset + sizeof(struct mfi_pd_address))) {
> +#ifdef DEBUG_MEGASAS_DCMD
> +       DPRINTF("PD get list: invalid xfer_len %ld\n", cmd->iov_size);
> +#endif
> +       return MFI_STAT_INVALID_PARAMETER;
> +    }
> +
> +    max_luns = (cmd->iov_size - offset) / sizeof(struct mfi_pd_address);
> +    if (max_luns > s->fw_luns)
> +       max_luns = s->fw_luns;
> +#ifdef DEBUG_MEGASAS_DCMD
> +    DPRINTF("PD get list: returning info for %d PDs\n", max_luns);
> +#endif
> +
> +    for (dev_id = 0; dev_id < max_luns; dev_id++) {
>        SCSIDevice *sdev;
>
>        sdev = s->bus.devs[dev_id];
>        if (sdev) {
> -           info.addr[num_pd_disks].device_id = dev_id;
> -           info.addr[num_pd_disks].encl_device_id = dev_id;
> +           info->addr[num_pd_disks].device_id = dev_id;
> +           info->addr[num_pd_disks].encl_device_id = dev_id;
> +           info->addr[num_pd_disks].sas_addr[0] = megasas_gen_sas_addr((unsigned long)sdev);
>            num_pd_disks ++;
>            offset += sizeof(struct mfi_pd_address);
>        }
> @@ -634,19 +667,87 @@ static int megasas_dcmd_pd_get_list(MPTState *s, struct megasas_cmd_t *cmd)
>     DPRINTF("PD get list: %d PDs, size %d\n", num_pd_disks, offset);
>  #endif
>
> -    info.size = offset;
> -    info.count = num_pd_disks;
> -    memcpy(cmd->iov_buf, (uint8_t *)&info, offset);
> +    info->size = offset;
> +    info->count = num_pd_disks;
>
> -    return offset;
> +    return MFI_STAT_OK;
> +}
> +
> +static int megasas_dcmd_pd_list_query(MPTState *s, struct megasas_cmd_t *cmd)
> +{
> +    uint16_t flags;
> +
> +    /* mbox0 contains flags */
> +    flags = le16_to_cpu(cmd->frame->dcmd.mbox[0]);
> +
> +#ifdef DEBUG_MEGASAS_DCMD
> +    DPRINTF("PD query list: flags %x\n", flags);
> +#endif
> +
> +    if (flags == MR_PD_QUERY_TYPE_ALL || s->is_jbod)
> +       return megasas_dcmd_pd_get_list(s, cmd);
> +
> +    return MFI_STAT_OK;
> +}
> +
> +static int megasas_pd_get_info_submit(SCSIDevice * sdev, int lun,
> +                                     struct megasas_cmd_t *cmd)
> +{
> +    struct mfi_pd_info * info = cmd->iov_buf;
> +    SCSIRequest *req;
> +
> +    if (info->inquiry_data[4] == 0) {
> +       /* Additional length is zero, resubmit */
> +       req = scsi_req_get(sdev, (uint32_t) -1, lun);
> +       if (!req)
> +           return MFI_STAT_FLASH_ALLOC_FAIL;
> +#ifdef DEBUG_MEGASAS_DCMD
> +       DPRINTF("PD get info submit std inquiry to dev %d\n", lun);
> +#endif
> +       req->hba_private = cmd;
> +       megasas_setup_inquiry(req, 0, info->inquiry_data,
> +                             sizeof(info->inquiry_data));
> +       return MFI_STAT_INVALID_STATUS;
> +    } else if (info->vpd_page83[3] == 0) {
> +       /* Additional length is zero, resubmit */
> +       req = scsi_req_get(sdev, (uint32_t) -1, lun);
> +       if (!req)
> +           return MFI_STAT_FLASH_ALLOC_FAIL;
> +#ifdef DEBUG_MEGASAS_DCMD
> +       DPRINTF("PD get info submit vpd inquiry to dev %d\n", lun);
> +#endif
> +       req->hba_private = cmd;
> +       megasas_setup_inquiry(req, 0x83,(uint8_t *)info->vpd_page83,
> +                             sizeof(info->vpd_page83));
> +       return MFI_STAT_INVALID_STATUS;
> +    }
> +
> +    /* Finished, set FW state */
> +    if (cmd->state->is_jbod)
> +       info->fw_state = MFI_PD_STATE_SYSTEM;
> +    else
> +       info->fw_state = MFI_PD_STATE_ONLINE;
> +#ifdef DEBUG_MEGASAS_DCMD
> +    DPRINTF("PD get info set state for dev %d to %x\n", lun, info->fw_state);
> +#endif
> +    return MFI_STAT_OK;
>  }
>
>  static int megasas_dcmd_pd_get_info(MPTState *s, struct megasas_cmd_t *cmd)
>  {
> -    struct mfi_pd_info info;
> +    struct mfi_pd_info *info = cmd->iov_buf;
>     uint64_t pd_size;
>     uint16_t pd_id;
>     SCSIDevice *sdev = NULL;
> +    int retval = MFI_STAT_OK;
> +
> +    memset(cmd->iov_buf, 0, cmd->iov_size);
> +    if (cmd->iov_size != sizeof(struct mfi_pd_info)) {
> +#ifdef DEBUG_MEGASAS_DCMD
> +       DPRINTF("PD get info: invalid xfer_len %ld\n", cmd->iov_size);
> +#endif
> +       return MFI_STAT_INVALID_PARAMETER;
> +    }
>
>     /* mbox0 has the ID */
>     pd_id = le16_to_cpu(cmd->frame->dcmd.mbox[0]);
> @@ -655,64 +756,106 @@ static int megasas_dcmd_pd_get_info(MPTState *s, struct megasas_cmd_t *cmd)
>     DPRINTF("PD get info for dev %d\n", pd_id);
>  #endif
>     sdev = s->bus.devs[pd_id];
> -    memset((uint8_t *)&info, 0x0, sizeof(info));
> -    info.ref.v.device_id = pd_id;
> +    info->ref.v.device_id = pd_id;
>
>     if (sdev) {
> -       /* Submit inquiry */
> -       megasas_get_inq(sdev, 0, (uint8_t *)&info.inquiry_data,
> -                       sizeof(info.inquiry_data));
> -       megasas_get_inq(sdev, 0x83, (uint8_t *)&info.vpd_page83,
> -                       sizeof(info.vpd_page83));
> -       info.fw_state = 0;
> -       info.state.ddf.v.pd_type.in_vd = 1;
> -       info.state.ddf.v.pd_type.intf = 0x2;
> +       info->state.ddf.v.pd_type.in_vd = 1;
> +       info->state.ddf.v.pd_type.intf = 0x2;
>        bdrv_get_geometry(sdev->conf.dinfo->bdrv, &pd_size);
> -       info.raw_size = pd_size;
> -       info.non_coerced_size = pd_size;
> -       info.coerced_size = pd_size;
> +       info->raw_size = pd_size;
> +       info->non_coerced_size = pd_size;
> +       info->coerced_size = pd_size;
> +       info->fw_state = MFI_PD_STATE_OFFLINE;
> +       info->path_info.count = 1;
> +       info->path_info.sas_addr[0] = megasas_gen_sas_addr((unsigned long)sdev);
> +       /* Submit inquiry */
> +       retval = megasas_pd_get_info_submit(cmd->sdev, pd_id, cmd);
>     }
> -    memcpy(cmd->iov_buf, (uint8_t *)&info, sizeof(info));
>
> -    return sizeof(info);
> +    return retval;
>  }
>
>  static int megasas_dcmd_ld_get_list(MPTState *s, struct megasas_cmd_t *cmd)
>  {
> -    struct mfi_ld_list info;
> -    uint32_t num_ld_disks = 0;
> +    struct mfi_ld_list *info = cmd->iov_buf;
> +    uint32_t num_ld_disks = 0, max_ld_disks = s->fw_luns;
>     uint64_t ld_size;
>     uint8_t n;
>     int offset;
>
> -    memset(&info, 0x0, sizeof(info));
> -    offset = 8;
> -    for (n = 0; n < s->fw_luns; n++) {
> +    memset(cmd->iov_buf, 0, cmd->iov_size);
> +    if (cmd->iov_size != sizeof(struct mfi_ld_list)) {
> +#ifdef DEBUG_MEGASAS_DCMD
> +       DPRINTF("LD get list: invalid xfer_len %ld\n", cmd->iov_size);
> +#endif
> +       return MFI_STAT_INVALID_PARAMETER;
> +    }
> +
> +    if (s->is_jbod)
> +       max_ld_disks = 0;
> +
> +#ifdef DEBUG_MEGASAS_DCMD
> +    DPRINTF("LD get list: returning info for %d LDs\n", max_ld_disks);
> +#endif
> +    for (n = 0; n < max_ld_disks; n++) {
>        SCSIDevice *sdev;
>
>        sdev = s->bus.devs[n];
>        if (sdev) {
>            bdrv_get_geometry(sdev->conf.dinfo->bdrv, &ld_size);
>            ld_size *= 512;
> -           info.ld_list[num_ld_disks].ld.v.target_id = n;
> -           info.ld_list[num_ld_disks].state = MFI_LD_STATE_OPTIMAL;
> -           info.ld_list[num_ld_disks].size = ld_size;
> +           info->ld_list[num_ld_disks].ld.v.target_id = n;
> +           info->ld_list[num_ld_disks].state = MFI_LD_STATE_OPTIMAL;
> +           info->ld_list[num_ld_disks].size = ld_size;
>            num_ld_disks ++;
>            offset += 18;
>        }
>     }
> -    info.ld_count = num_ld_disks;
> -    memcpy(cmd->iov_buf, (uint8_t *)&info, offset);
> +    info->ld_count = num_ld_disks;
> +#ifdef DEBUG_MEGASAS_DCMD
> +    DPRINTF("LD get list: found %d LDs\n", num_ld_disks);
> +#endif
> +
> +    return MFI_STAT_OK;
> +}
> +
> +static int megasas_ld_get_info_submit(SCSIDevice * sdev, int lun,
> +                                     struct megasas_cmd_t *cmd)
> +{
> +    struct mfi_ld_info * info = cmd->iov_buf;
> +    SCSIRequest *req;
>
> -    return offset;
> +    if (info->vpd_page83[3] == 0) {
> +       req = scsi_req_get(sdev, (uint32_t) -1, lun);
> +       if (!req)

This would be against CODING_STYLE but nobody seems to care.

> +           return MFI_STAT_FLASH_ALLOC_FAIL;
> +#ifdef DEBUG_MEGASAS_DCMD
> +       DPRINTF("LD get info submit vpd inquiry to dev %d\n", lun);
> +#endif
> +       req->hba_private = cmd;
> +       megasas_setup_inquiry(req, 0x83,(uint8_t *)info->vpd_page83,
> +                             sizeof(info->vpd_page83));
> +       return MFI_STAT_INVALID_STATUS;
> +    }
> +    info->ld_config.params.state = MFI_LD_STATE_OPTIMAL;
> +    return MFI_STAT_OK;
>  }
>
>  static int megasas_dcmd_ld_get_info(MPTState *s, struct megasas_cmd_t *cmd)
>  {
> -    struct mfi_ld_info info;
> +    struct mfi_ld_info *info = cmd->iov_buf;
>     uint64_t ld_size;
>     uint16_t ld_id;
>     SCSIDevice *sdev = NULL;
> +    int retval = MFI_STAT_OK;
> +
> +    memset(cmd->iov_buf, 0, cmd->iov_size);
> +    if (cmd->iov_size != sizeof(struct mfi_ld_info)) {
> +#ifdef DEBUG_MEGASAS_DCMD
> +       DPRINTF("LD get info: invalid xfer_len %ld\n", cmd->iov_size);
> +#endif
> +       return MFI_STAT_INVALID_PARAMETER;
> +    }
>
>     /* mbox0 has the ID */
>     ld_id = le16_to_cpu(cmd->frame->dcmd.mbox[0]);
> @@ -721,51 +864,71 @@ static int megasas_dcmd_ld_get_info(MPTState *s, struct megasas_cmd_t *cmd)
>     DPRINTF("LD get info for dev %d\n", ld_id);
>  #endif
>     sdev = s->bus.devs[ld_id];
> -    memset((void *)&info, 0x0, sizeof(info));
> -    info.ld_config.properties.ld.v.target_id = ld_id;
> +    info->ld_config.properties.ld.v.target_id = ld_id;
>
>     if (sdev) {
> -       memcpy(&info.ld_config.properties.name, "QEMU HARDDISK  ", 16);
> -       info.ld_config.params.stripe_size = 64;
> -       info.ld_config.params.num_drives = 1;
> -       info.ld_config.params.state = MFI_LD_STATE_OPTIMAL;
> -       info.ld_config.params.is_consistent = 1;
> +       info->ld_config.params.stripe_size = 64;
> +       info->ld_config.params.num_drives = 1;
> +       info->ld_config.params.state = MFI_LD_STATE_OFFLINE;
> +       info->ld_config.params.is_consistent = 1;
>        bdrv_get_geometry(sdev->conf.dinfo->bdrv, &ld_size);
> -       info.size = ld_size;
> -       megasas_get_inq(sdev, 0x83, (uint8_t *)&info.vpd_page83,
> -            sizeof(info.vpd_page83));
> +       info->size = ld_size;
> +       retval = megasas_ld_get_info_submit(cmd->sdev, ld_id, cmd);
>     }
> -    memcpy(cmd->iov_buf, (uint8_t *)&info, sizeof(info));
>
> -    return sizeof(info);
> +    return retval;
>  }
>
>  static int megasas_dcmd_get_properties(MPTState *s, struct megasas_cmd_t *cmd)
>  {
> -    struct mfi_ctrl_props info;
> +    struct mfi_ctrl_props *info = cmd->iov_buf;
>
> -#ifdef DEBUG_MEGASAS_MFI
> +    if (cmd->iov_size != sizeof(struct mfi_ctrl_props)) {
> +#ifdef DEBUG_MEGASAS_DCMD
> +       DPRINTF("DCMD get properties: invalid xfer_len %ld\n", cmd->iov_size);
> +#endif
> +       memset(cmd->iov_buf, 0, cmd->iov_size);
> +       return MFI_STAT_INVALID_PARAMETER;
> +    }
> +
> +#ifdef DEBUG_MEGASAS_DCMD
>     DPRINTF("DCMD get properties: xfer_len %d sge_count %d\n",
>            cmd->frame->header.data_len, cmd->frame->header.sge_count);
>  #endif
> -    info.pred_fail_poll_interval = 300;
> -    info.intr_throttle_cnt = 16;
> -    info.intr_throttle_timeout = 50;
> -    info.rebuild_rate = 30;
> -    info.patrol_read_rate = 30;
> -    info.bgi_rate = 30;
> -    info.cc_rate = 30;
> -    info.recon_rate = 30;
> -    info.cache_flush_interval = 4;
> -    info.spinup_drv_cnt = 2;
> -    info.spinup_delay = 6;
> -    info.ecc_bucket_size = 15;
> -    info.ecc_bucket_leak_rate = 1440;
> -    info.expose_encl_devices = 1;
> -
> -    memcpy(cmd->iov_buf, (uint8_t *)&info, sizeof(info));
> -
> -    return sizeof(info);
> +    info->pred_fail_poll_interval = 300;
> +    info->intr_throttle_cnt = 16;
> +    info->intr_throttle_timeout = 50;
> +    info->rebuild_rate = 30;
> +    info->patrol_read_rate = 30;
> +    info->bgi_rate = 30;
> +    info->cc_rate = 30;
> +    info->recon_rate = 30;
> +    info->cache_flush_interval = 4;
> +    info->spinup_drv_cnt = 2;
> +    info->spinup_delay = 6;
> +    info->ecc_bucket_size = 15;
> +    info->ecc_bucket_leak_rate = 1440;
> +    info->expose_encl_devices = 1;
> +
> +    return MFI_STAT_OK;
> +}
> +
> +static int megasas_cache_flush(MPTState *s, struct megasas_cmd_t *cmd)
> +{
> +#ifdef DEBUG_MEGASAS_DCMD
> +    DPRINTF("MFI DCMD Cache flush\n");
> +#endif
> +    qemu_aio_flush();
> +    return MFI_STAT_OK;
> +}
> +
> +static int megasas_ctrl_shutdown(MPTState *s, struct megasas_cmd_t *cmd)
> +{
> +#ifdef DEBUG_MEGASAS_DCMD
> +    DPRINTF("MFI DCMD Controller shutdown\n");
> +#endif
> +    s->fw_state = MFI_FWSTATE_READY;
> +    return MFI_STAT_OK;
>  }
>
>  static int megasas_dcmd_set_properties(MPTState *s, struct megasas_cmd_t *cmd)
> @@ -814,7 +977,7 @@ static int megasas_dcmd_set_properties(MPTState *s, struct megasas_cmd_t *cmd)
>     DPRINTF("%02x %02x %02x %0x2 %02x %02x %02x %02x\n",
>            dummy[0x38], dummy[0x39], dummy[0x3a], dummy[0x3b],
>            dummy[0x3c], dummy[0x3d], dummy[0x3e], dummy[0x3f]);
> -    return cmd->frame->header.data_len;
> +    return MFI_STAT_OK;
>  }
>
>  static int megasas_dcmd_dummy(MPTState *s, struct megasas_cmd_t *cmd)
> @@ -825,13 +988,67 @@ static int megasas_dcmd_dummy(MPTState *s, struct megasas_cmd_t *cmd)
>  #endif
>     memset(cmd->iov_buf, 0, cmd->frame->header.data_len);
>
> -    return cmd->frame->header.data_len;
> +    return MFI_STAT_OK;
>  }
>
> +
> +struct dcmd_cmd_tbl_t {

static const?

> +    int opcode;
> +    int (*func)(MPTState *s, struct megasas_cmd_t *cmd);
> +} dcmd_cmd_tbl[] = {
> +    {MFI_DCMD_CTRL_MFI_HOST_MEM_ALLOC, megasas_dcmd_dummy},
> +    {MFI_DCMD_CTRL_GET_INFO, megasas_ctrl_get_info},
> +    {MFI_DCMD_CTRL_GET_PROPERTIES, megasas_dcmd_get_properties},
> +    {MFI_DCMD_CTRL_SET_PROPERTIES, megasas_dcmd_set_properties},
> +    {MFI_DCMD_SPEAKER_GET, megasas_dcmd_dummy},
> +    {MFI_DCMD_SPEAKER_ENABLE, megasas_dcmd_dummy},
> +    {MFI_DCMD_SPEAKER_DISABLE, megasas_dcmd_dummy},
> +    {MFI_DCMD_SPEAKER_SILENCE, megasas_dcmd_dummy},
> +    {MFI_DCMD_SPEAKER_TEST, megasas_dcmd_dummy},
> +    {MFI_DCMD_CTRL_EVENT_GETINFO, megasas_event_info},
> +    {MFI_DCMD_CTRL_EVENT_GET, megasas_dcmd_dummy},
> +    {MFI_DCMD_CTRL_EVENT_WAIT, megasas_dcmd_dummy},
> +    {MFI_DCMD_CTRL_SHUTDOWN, megasas_ctrl_shutdown},
> +    {MFI_DCMD_HIBERNATE_SHUTDOWN, megasas_dcmd_dummy},
> +    {MFI_DCMD_CTRL_GET_TIME, megasas_dcmd_get_fw_time},
> +    {MFI_DCMD_CTRL_SET_TIME, megasas_dcmd_set_fw_time},
> +    {MFI_DCMD_CTRL_GET_BIOS_INFO, megasas_dcmd_get_bios_info},
> +    {MFI_DCMD_CTRL_FACTORY_DEFAULTS, megasas_dcmd_dummy},
> +    {MFI_DCMD_CTRL_MFC_DEFAULTS_GET, megasas_mfc_get_defaults},
> +    {MFI_DCMD_CTRL_MFC_DEFAULTS_SET, megasas_dcmd_dummy},
> +    {MFI_DCMD_CTRL_CACHE_FLUSH, megasas_cache_flush},
> +    {MFI_DCMD_PD_GET_LIST, megasas_dcmd_pd_get_list},
> +    {MFI_DCMD_PD_LIST_QUERY, megasas_dcmd_pd_list_query},
> +    {MFI_DCMD_PD_GET_INFO, megasas_dcmd_pd_get_info},
> +    {MFI_DCMD_PD_STATE_SET, megasas_dcmd_dummy},
> +    {MFI_DCMD_PD_REBUILD, megasas_dcmd_dummy},
> +    {MFI_DCMD_PD_BLINK, megasas_dcmd_dummy},
> +    {MFI_DCMD_PD_UNBLINK, megasas_dcmd_dummy},
> +    {MFI_DCMD_LD_GET_LIST, megasas_dcmd_ld_get_list},
> +    {MFI_DCMD_LD_GET_INFO, megasas_dcmd_ld_get_info},
> +    {MFI_DCMD_LD_GET_PROP, megasas_dcmd_dummy},
> +    {MFI_DCMD_LD_SET_PROP, megasas_dcmd_dummy},
> +    {MFI_DCMD_LD_DELETE, megasas_dcmd_dummy},
> +    {MFI_DCMD_CFG_READ, megasas_dcmd_dummy},
> +    {MFI_DCMD_CFG_ADD, megasas_dcmd_dummy},
> +    {MFI_DCMD_CFG_CLEAR, megasas_dcmd_dummy},
> +    {MFI_DCMD_CFG_FOREIGN_READ, megasas_dcmd_dummy},
> +    {MFI_DCMD_CFG_FOREIGN_IMPORT, megasas_dcmd_dummy},
> +    {MFI_DCMD_BBU_STATUS, megasas_dcmd_dummy},
> +    {MFI_DCMD_BBU_CAPACITY_INFO, megasas_dcmd_dummy},
> +    {MFI_DCMD_BBU_DESIGN_INFO, megasas_dcmd_dummy},
> +    {MFI_DCMD_BBU_PROP_GET, megasas_dcmd_dummy},
> +    {MFI_DCMD_CLUSTER, megasas_dcmd_dummy},
> +    {MFI_DCMD_CLUSTER_RESET_ALL, megasas_dcmd_dummy},
> +    {MFI_DCMD_CLUSTER_RESET_LD, megasas_dcmd_dummy},
> +    {-1, NULL}
> +};
> +
>  static int megasas_handle_dcmd(MPTState *s, struct megasas_cmd_t *cmd)
>  {
>     int opcode, size = 0, len;
>     int retval = 0;
> +    struct dcmd_cmd_tbl_t *cmdptr = dcmd_cmd_tbl;
>
>     opcode = le32_to_cpu(cmd->frame->dcmd.opcode);
>  #ifdef DEBUG_MEGASAS_DCMD
> @@ -841,158 +1058,83 @@ static int megasas_handle_dcmd(MPTState *s, struct megasas_cmd_t *cmd)
>     if (len < 0) {
>        return MFI_STAT_MEMORY_NOT_AVAILABLE;
>     }
> -    switch (opcode) {
> -       case MFI_DCMD_CTRL_MFC_DEFAULTS_GET:
> -#ifdef DEBUG_MEGASAS_DCMD
> -           DPRINTF("MFI DCMD get MFC defaults\n");
> -#endif
> -           size = megasas_mfc_get_defaults(s, cmd);
> -           retval = MFI_STAT_OK;
> -           break;
> -       case MFI_DCMD_CTRL_GET_INFO:
> -#ifdef DEBUG_MEGASAS_DCMD
> -           DPRINTF("MFI DCMD get controller info\n");
> -#endif
> -           size = megasas_ctrl_get_info(s, cmd);
> -           retval = MFI_STAT_OK;
> -           break;
> -       case MFI_DCMD_CTRL_CACHE_FLUSH:
> -#ifdef DEBUG_MEGASAS_DCMD
> -           DPRINTF("MFI DCMD Cache flush\n");
> -#endif
> -           qemu_aio_flush();
> -           retval = MFI_STAT_OK;
> -           break;
> -       case MFI_DCMD_CTRL_SHUTDOWN:
> -#ifdef DEBUG_MEGASAS_DCMD
> -           DPRINTF("MFI DCMD Controller shutdown\n");
> -#endif
> -           s->fw_state = MFI_FWSTATE_READY;
> -           retval = MFI_STAT_OK;
> -           break;
> -       case MFI_DCMD_PD_LIST_QUERY:
> +    while (cmdptr->opcode != -1 && cmdptr->opcode != opcode)
> +       cmdptr++;
> +    if (cmdptr->opcode == -1) {
> +       DPRINTF("MFI DCMD %x unhandled (len %d)\n", opcode, len);
> +       retval = megasas_dcmd_dummy(s, cmd);
> +    } else {
> +       retval = cmdptr->func(s, cmd);
> +    }
> +    if (retval != MFI_STAT_INVALID_STATUS) {
> +       size = megasas_finish_dcmd(cmd, cmd->iov_size);
>  #ifdef DEBUG_MEGASAS_DCMD
> -           DPRINTF("MFI DCMD query physical devices\n");
> +       DPRINTF("MFI DCMD wrote %d bytes\n", size);
>  #endif
> -           retval = MFI_STAT_INVALID_DCMD;
> -           break;
> -       case MFI_DCMD_PD_GET_LIST:
> +    }
> +    return retval;
> +}
> +
> +static int megasas_finish_internal_dcmd(struct megasas_cmd_t *cmd,
> +                                       SCSIRequest *req)
> +{
> +    int opcode;
> +    int retval = MFI_STAT_OK;
> +    int lun = req->lun;
> +
> +    opcode = le32_to_cpu(cmd->frame->dcmd.opcode);
> +    scsi_req_put(req);
>  #ifdef DEBUG_MEGASAS_DCMD
> -           DPRINTF("MFI DCMD PD get list\n");
> +    DPRINTF("DCMD finish internal cmd %x lun %d\n", opcode, lun);
>  #endif
> -           size = megasas_dcmd_pd_get_list(s, cmd);
> -           retval = MFI_STAT_OK;
> -           break;
> +    switch (opcode) {
>        case MFI_DCMD_PD_GET_INFO:
>  #ifdef DEBUG_MEGASAS_DCMD
> -           DPRINTF("MFI DCMD PD get info\n");
> -#endif
> -           size = megasas_dcmd_pd_get_info(s, cmd);
> -           retval = MFI_STAT_OK;
> -           break;
> -       case MFI_DCMD_LD_GET_LIST:
> -#ifdef DEBUG_MEGASAS_DCMD
> -           DPRINTF("MFI DCMD LD get list\n");
> +           DPRINTF("Internal DCMD PD get info\n");
>  #endif
> -           size = megasas_dcmd_ld_get_list(s, cmd);
> -           retval = MFI_STAT_OK;
> +           retval = megasas_pd_get_info_submit(cmd->sdev, lun, cmd);
>            break;
>        case MFI_DCMD_LD_GET_INFO:
>  #ifdef DEBUG_MEGASAS_DCMD
> -           DPRINTF("MFI DCMD LD get info\n");
> +           DPRINTF("Internal DCMD LD get info\n");
>  #endif
> -           size = megasas_dcmd_ld_get_info(s, cmd);
> -           retval = MFI_STAT_OK;
> +           retval = megasas_ld_get_info_submit(cmd->sdev, lun, cmd);
>            break;
> -       case MFI_DCMD_CTRL_GET_PROPERTIES:
> -#ifdef DEBUG_MEGASAS_DCMD
> -           DPRINTF("MFI DCMD Get Properties\n");
> -#endif
> -           size = megasas_dcmd_get_properties(s, cmd);
> -           retval = MFI_STAT_OK;
> -           break;
> -       case MFI_DCMD_CTRL_SET_PROPERTIES:
> -#ifdef DEBUG_MEGASAS_DCMD
> -           DPRINTF("MFI DCMD Set Properties\n");
> -#endif
> -           size = megasas_dcmd_set_properties(s, cmd);
> -           retval = MFI_STAT_OK;
> -           break;
> -       case MFI_DCMD_CTRL_EVENT_GETINFO:
> -           size = megasas_event_info(s, cmd);
> -           retval = MFI_STAT_OK;
> -           break;
> -       case MFI_DCMD_CFG_READ:
> -       case MFI_DCMD_CFG_FOREIGN_READ:
> -           size = megasas_dcmd_dummy(s, cmd);
> -           retval = MFI_STAT_OK;
> -           break;
> -       case MFI_DCMD_CTRL_EVENT_WAIT:
> +       default:
>  #ifdef DEBUG_MEGASAS_DCMD
> -           DPRINTF("MFI DCMD controller event wait\n");
> +           DPRINTF("Invalid internal DCMD\n");
>  #endif
>            retval = MFI_STAT_INVALID_DCMD;
>            break;
> -    case MFI_DCMD_CLUSTER_RESET_ALL:
> -    case MFI_DCMD_CLUSTER_RESET_LD:
> -        /* Cluster reset commands have a size of 0 */
> -        size = 0;
> -        retval = MFI_STAT_OK;
> -        break;
> -       case MFI_DCMD_CTRL_GET_BIOS_INFO:
> -           size = megasas_dcmd_get_bios_info(s, cmd);
> -           retval = MFI_STAT_OK;
> -           break;
> -       case MFI_DCMD_CTRL_GET_TIME:
> -           size = megasas_dcmd_get_fw_time(s, cmd);
> -           retval = MFI_STAT_OK;
> -           break;
> -    case MFI_DCMD_CTRL_SET_TIME:
> -        size = megasas_dcmd_set_fw_time(s, cmd);
> -        retval = MFI_STAT_OK;
> -        break;
> -       case 0x010e0301:
> -       case 0x010e0302:
> -       case 0x010e8481:
> -           DPRINTF("MFI DCMD %x dummy return %d bytes\n", opcode, len);
> -           size = megasas_dcmd_dummy(s, cmd);
> -           retval = MFI_STAT_OK;
> -           break;
> -       default:
> -           DPRINTF("MFI DCMD %x unhandled (len %d)\n", opcode, len);
> -           retval = MFI_STAT_INVALID_DCMD;
> -           break;
>     }
> -    size = megasas_finish_dcmd(cmd, size);
> -#ifdef DEBUG_MEGASAS_DCMD
> -    DPRINTF("MFI DCMD wrote %d bytes\n", size);
> -#endif
> +    if (retval != MFI_STAT_INVALID_STATUS)
> +       megasas_finish_dcmd(cmd, cmd->iov_size);
>     return retval;
>  }
>
> -static int megasas_handle_scsi(MPTState *s, struct megasas_cmd_t *cmd)
> +static int megasas_handle_scsi(MPTState *s, struct megasas_cmd_t *cmd, int is_logical)
>  {
>     uint8_t *cdb;
>
>     cdb = cmd->frame->pass.cdb;
>
>     if (cmd->frame->header.target_id < s->fw_luns)
> -       cmd->lun = &s->luns[cmd->frame->header.target_id];
> -
> -    if (cmd->lun)
> -       cmd->lun->sdev = s->bus.devs[cmd->frame->header.target_id];
> +       cmd->sdev = s->bus.devs[cmd->frame->header.target_id];
>
>  #ifdef DEBUG_MEGASAS_IO
> -    DPRINTF("%s dev %x lun %x sdev %p xfer %d\n",
> +    DPRINTF("%s %s dev %x lun %x sdev %p xfer %d\n",
>            mfi_frame_desc[cmd->frame->header.frame_cmd],
> +           is_logical?"logical":"physical",
>            cmd->frame->header.target_id, cmd->frame->header.lun_id,
> -           cmd->lun?cmd->lun->sdev:NULL, cmd->frame->header.data_len);
> +           cmd->sdev, cmd->frame->header.data_len);
>  #endif
>
> -    if (!cmd->lun || !cmd->lun->sdev) {
> +    if (!cmd->sdev || (s->is_jbod && is_logical)) {
>  #ifdef DEBUG_MEGASAS_IO
> -       DPRINTF("%s dev %x/%x target not present\n",
> -               mfi_frame_desc[cmd->frame->header.frame_cmd], cmd->frame->header.target_id,
> +       DPRINTF("%s %s dev %x/%x target not present\n",
> +               mfi_frame_desc[cmd->frame->header.frame_cmd],
> +               is_logical?"logical":"physical",
> +               cmd->frame->header.target_id,
>                cmd->frame->header.lun_id);
>  #endif
>        return MFI_STAT_DEVICE_NOT_FOUND;
> @@ -1009,7 +1151,7 @@ static int megasas_handle_scsi(MPTState *s, struct megasas_cmd_t *cmd)
>        return MFI_STAT_SCSI_DONE_WITH_ERROR;
>     }
>
> -    cmd->req = scsi_req_get(cmd->lun->sdev, cmd->frame->header.context, cmd->frame->header.lun_id);
> +    cmd->req = scsi_req_get(cmd->sdev, cmd->frame->header.context, cmd->frame->header.lun_id);
>     cmd->req->hba_private = cmd;
>     scsi_req_parse(cmd->req, cdb);
>     if (cmd->frame->header.data_len != cmd->req->cmd.xfer) {
> @@ -1019,7 +1161,7 @@ static int megasas_handle_scsi(MPTState *s, struct megasas_cmd_t *cmd)
>        s->event_count++;
>     }
>
> -    megasas_map_sgl(cmd, MEGASAS_PTHRU_SGL_OFFSET);
> +    megasas_map_sgl(cmd, offsetof(struct mfi_pass_frame, sgl));
>     scsi_req_sgl(cmd->req, &cmd->sg);
>
>     return MFI_STAT_INVALID_STATUS;
> @@ -1037,10 +1179,7 @@ static int megasas_handle_io(MPTState *s, struct megasas_cmd_t *cmd)
>     lba_start = ((uint64_t)lba_start_hi << 32) | lba_start_lo;
>
>     if (cmd->frame->header.target_id < s->fw_luns)
> -       cmd->lun = &s->luns[cmd->frame->header.target_id];
> -
> -    if (cmd->lun)
> -       cmd->lun->sdev = s->bus.devs[cmd->frame->header.target_id];
> +       cmd->sdev = s->bus.devs[cmd->frame->header.target_id];
>
>  #ifdef DEBUG_MEGASAS_IO
>     DPRINTF("%s dev %x lun %x lba %lx count %lx\n",
> @@ -1048,7 +1187,7 @@ static int megasas_handle_io(MPTState *s, struct megasas_cmd_t *cmd)
>            cmd->frame->header.target_id, cmd->frame->header.lun_id,
>            (unsigned long)lba_start, (unsigned long)lba_count);
>  #endif
> -    if (!cmd->lun || !cmd->lun->sdev) {
> +    if (!cmd->sdev) {
>  #ifdef DEBUG_MEGSAS_IO
>        DPRINTF("%s dev %x/%x LUN not present\n",
>                mfi_frame_desc[cmd->frame->header.frame_cmd],
> @@ -1068,9 +1207,9 @@ static int megasas_handle_io(MPTState *s, struct megasas_cmd_t *cmd)
>        return MFI_STAT_SCSI_DONE_WITH_ERROR;
>     }
>
> -    cmd->req = scsi_req_get(cmd->lun->sdev, cmd->frame->header.context, cmd->frame->header.lun_id);
> +    cmd->req = scsi_req_get(cmd->sdev, cmd->frame->header.context, cmd->frame->header.lun_id);
>     cmd->req->hba_private = cmd;
> -    megasas_map_sgl(cmd, MEGASAS_IO_SGL_OFFSET);
> +    megasas_map_sgl(cmd, offsetof(struct mfi_io_frame, sgl));
>
>     scsi_req_setup(cmd->req, write, lba_start, lba_count);
>     scsi_req_sgl(cmd->req, &cmd->sg);
> @@ -1078,6 +1217,21 @@ static int megasas_handle_io(MPTState *s, struct megasas_cmd_t *cmd)
>     return MFI_STAT_INVALID_STATUS;
>  }
>
> +static int megasas_finish_internal_command(struct megasas_cmd_t *cmd,
> +                                          SCSIRequest *req)
> +{
> +    int retval = MFI_STAT_INVALID_CMD;
> +
> +    switch (cmd->frame->header.frame_cmd) {
> +       case MFI_CMD_DCMD:
> +           retval = megasas_finish_internal_dcmd(cmd, req);
> +           break;
> +       default:
> +           break;
> +    }
> +    return retval;
> +}
> +
>  static void megasas_command_complete(SCSIRequest *req)
>  {
>     struct megasas_cmd_t *cmd;
> @@ -1095,24 +1249,38 @@ static void megasas_command_complete(SCSIRequest *req)
>        return;
>     }
>
> +    if (cmd->req != req) {
> +       /*
> +        * Internal command complete
> +        */
> +       cmd_status = megasas_finish_internal_command(cmd, req);
> +       if (cmd_status == MFI_STAT_INVALID_STATUS)
> +           return;
> +    } else {
> +
>  #ifdef DEBUG_MEGASAS_IO
> -    DPRINTF("%s req %p cmd %p lun %p finished with status %x len %u\n",
> -           mfi_frame_desc[cmd->frame->header.frame_cmd], req, cmd, cmd->lun->sdev,
> -           req->status, (unsigned)req->xferlen);
> -#endif
> -    if (req->status == CHECK_CONDITION) {
> -       megasas_build_sense(cmd, cmd->lun->sdev->sense);
> -       cmd_status = MFI_STAT_SCSI_DONE_WITH_ERROR;
> -       scsi_dev_clear_sense(cmd->lun->sdev);
> -    }
> +       DPRINTF("%s req %p cmd %p lun %p finished with status %x len %u\n",
> +               mfi_frame_desc[cmd->frame->header.frame_cmd], req, cmd, cmd->sdev,
> +               req->status, (unsigned)req->xferlen);
> +#endif
> +       if (req->status == CHECK_CONDITION) {
> +           megasas_build_sense(cmd, cmd->sdev->sense);
> +           cmd_status = MFI_STAT_SCSI_DONE_WITH_ERROR;
> +           scsi_dev_clear_sense(cmd->sdev);
> +       }
>
> -    megasas_unmap_sgl(cmd);
> -    megasas_frame_set_scsi_status(cmd->pa, cmd->req->status);
> -    scsi_req_put(cmd->req);
> -    cmd->req = NULL;
> +       megasas_unmap_sgl(cmd);
> +       megasas_frame_set_scsi_status(cmd->pa, cmd->req->status);
> +       scsi_req_put(cmd->req);
> +       cmd->req = NULL;
> +    }
>     context = megasas_finish_command(cmd->state, cmd);
> -    megasas_frame_set_cmd_status(cmd->pa, cmd_status);
> -    megasas_dequeue_frame(cmd->state, context);
> +    if (context == -1) {
> +       DPRINTF("Invalid context for cmd %p\n", cmd);
> +    } else {
> +       megasas_frame_set_cmd_status(cmd->pa, cmd_status);
> +       megasas_dequeue_frame(cmd->state, context);
> +    }
>  }
>
>  static int megasas_handle_abort(MPTState *s, struct megasas_cmd_t *cmd)
> @@ -1152,8 +1320,8 @@ static void megasas_handle_frame(MPTState *s, target_phys_addr_t frame_addr,
>     uint32_t frame_context = 0;
>     struct megasas_cmd_t *cmd;
>
> -    frame_cmd = ldub_phys(frame_addr + MEGASAS_FRAME_CMD_OFFSET);
> -    frame_context = ldl_phys(frame_addr + MEGASAS_FRAME_CONTEXT_OFFSET);
> +    frame_cmd = megasas_frame_get_cmd(frame_addr);
> +    frame_context = megasas_frame_get_context(frame_addr);
>
>  #ifdef DEBUG_MEGASAS_MFI
>     DPRINTF("MFI cmd %x context %x count %d\n",
> @@ -1180,8 +1348,10 @@ static void megasas_handle_frame(MPTState *s, target_phys_addr_t frame_addr,
>            frame_status = megasas_handle_abort(s, cmd);
>            break;
>        case MFI_CMD_PD_SCSI_IO:
> +           frame_status = megasas_handle_scsi(s, cmd, 0);
> +           break;
>        case MFI_CMD_LD_SCSI_IO:
> -           frame_status = megasas_handle_scsi(s, cmd);
> +           frame_status = megasas_handle_scsi(s, cmd, 1);
>            break;
>        case MFI_CMD_LD_READ:
>        case MFI_CMD_LD_WRITE:
> @@ -1445,7 +1615,9 @@ static void megasas_mmio_mapfunc(PCIDevice *pci_dev, int region_num,
>  {
>     MPTState *s = DO_UPCAST(MPTState, dev, pci_dev);
>
> +#ifdef DEBUG_MEGASAS_REG
>     DPRINTF("Mapping MMIO region %d at %08lx\n", region_num, (unsigned long)addr);
> +#endif
>     cpu_register_physical_memory(addr, size, s->mmio_io_addr);
>     s->event_count++;
>  }
> @@ -1455,8 +1627,9 @@ static void megasas_io_mapfunc(PCIDevice *pci_dev, int region_num,
>  {
>     MPTState *s = DO_UPCAST(MPTState, dev, pci_dev);
>
> +#ifdef DEBUG_MEGASAS_REG
>     DPRINTF("Mapping IO region %d at %08lx\n", region_num, (unsigned long)addr);
> -
> +#endif
>     register_ioport_write(addr, size, 1, megasas_io_writeb, s);
>     register_ioport_write(addr, size, 2, megasas_io_writew, s);
>     register_ioport_write(addr, size, 4, megasas_io_writel, s);
> @@ -1471,7 +1644,9 @@ static void megasas_queue_mapfunc(PCIDevice *pci_dev, int region_num,
>  {
>     MPTState *s = DO_UPCAST(MPTState, dev, pci_dev);
>
> +#ifdef DEBUG_MEGASAS_REG
>     DPRINTF("Mapping QUEUE region %d at %08lx\n", region_num, (unsigned long)addr);
> +#endif
>     cpu_register_physical_memory(addr, size, s->queue_addr);
>     s->event_count++;
>  }
> @@ -1555,10 +1730,20 @@ static int megasas_scsi_init(PCIDevice *dev)
>                            PCI_BASE_ADDRESS_SPACE_IO, megasas_io_mapfunc);
>     pci_register_bar((struct PCIDevice *)s, 3, 0x40000,
>                            PCI_BASE_ADDRESS_SPACE_MEMORY, megasas_queue_mapfunc);
> -    s->fw_sge = MEGASAS_MAX_SGE;
> -    s->fw_cmds = MEGASAS_MAX_FRAMES;
> -    s->fw_luns = (MEGASAS_MAX_LUNS > MAX_SCSI_DEVS) ?
> -       MAX_SCSI_DEVS : MEGASAS_MAX_LUNS;
> +    if (s->fw_sge > MEGASAS_MAX_SGE)
> +       s->fw_sge = MEGASAS_MAX_SGE;
> +    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");

Please add spaces around '?' and ':'.

> +    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++) {
> @@ -1581,6 +1766,12 @@ static PCIDeviceInfo megasas_info = {
>     .qdev.size  = sizeof(MPTState),
>     .init       = megasas_scsi_init,
>     .exit       = megasas_scsi_uninit,
> +    .qdev.props = (Property[]) {
> +       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_END_OF_LIST(),
> +    },
>  };
>
>  static void megaraid1078_register_devices(void)
> diff --git a/hw/mfi.h b/hw/mfi.h
> index e7a5fde..90334b1 100644
> --- a/hw/mfi.h
> +++ b/hw/mfi.h
> @@ -167,7 +167,7 @@ typedef enum {
>        MFI_DCMD_CTRL_SHUTDOWN =                0x01050000,
>        MFI_DCMD_HIBERNATE_SHUTDOWN =           0x01060000,
>        MFI_DCMD_CTRL_GET_TIME =                0x01080101,
> -    MFI_DCMD_CTRL_SET_TIME =        0x01080102,
> +       MFI_DCMD_CTRL_SET_TIME =                0x01080102,
>        MFI_DCMD_CTRL_GET_BIOS_INFO =           0x010c0100,
>        MFI_DCMD_CTRL_FACTORY_DEFAULTS =        0x010d0000,
>        MFI_DCMD_CTRL_MFC_DEFAULTS_GET =        0x010e0201,
> @@ -644,7 +644,16 @@ struct mfi_defaults {
>        uint8_t         restored_hot_spare_on_insertion;
>        uint8_t         expose_enclosure_devices;
>        uint8_t         maintain_pd_fail_history;
> -       uint8_t         resv[28];
> +       uint8_t         disable_puncture;
> +       uint8_t         zero_based_enumeration;
> +       uint8_t         disable_preboot_cli;
> +       uint8_t         show_drive_led_on_activity;
> +       uint8_t         cluster_disable;
> +       uint8_t         sas_disable;
> +       uint8_t         auto_detect_backplane;
> +       uint8_t         fde_only;
> +       uint8_t         delay_during_post;
> +       uint8_t         resv[19];
>  } __attribute__ ((packed));
>
>  /* Controller default settings */
> @@ -685,6 +694,8 @@ struct mfi_ctrl_info {
>  #define MFI_INFO_HW_ALARM      0x02
>  #define MFI_INFO_HW_NVRAM      0x04
>  #define MFI_INFO_HW_UART       0x08
> +#define MFI_INFO_HW_MEM                0x10
> +#define MFI_INFO_HW_FLASH      0x20
>        uint32_t                current_fw_time;
>        uint16_t                max_cmds;
>        uint16_t                max_sg_elements;
>
>

Blue Swirl wrote:
> On Tue, Jun 8, 2010 at 2:15 PM, Hannes Reinecke <hare@suse.de> wrote:
>> This patch updates the megasas HBA emulation to version 1.01.
>> It fixes the following issues:
>>
>> - Remove hand-crafted inquiry command
>> - Remove bounce-buffer for direct commands
>> - Implements qdev properties to set 'max_sge', 'max_cmds'.
>> - Implement JBOD mode
>> - Improve direct command handling
>> - Minor cleanups
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>
[ .. ]
>> +static uint64_t megasas_gen_sas_addr(unsigned long id)
>> +{
>> +    uint64_t addr;
>> +
>> +    addr = ((uint64_t)0x5001a4a << 36);
> 
> With 0x5001a4aULL the cast could be avoided.
> 
>> +    addr |= ((uint64_t)id & 0xfffffffff);
> 
> This cast could be avoided by making id uint64_t.
> 
Ok. Fixed.

[ .. ]
>> +    memcpy(info->product_name,"MegaRAID SAS 8708EM2", 20);
>> +    sprintf(info->serial_number,"QEMU%08lx",(unsigned long)s & 0xFFFFFFFF);
> 
> Please use snprintf(), OpenBSD linker issues warnings for all uses of sprintf().
> 
>> +    sprintf(info->package_version,"%s-QEMU", QEMU_VERSION);
>> +    strcpy(info->image_component[0].name, "APP");
> 
> Same problem with strcpy(), please use pstrcpy(), snprintf() or memcpy().
> 
Ah. Ok, Fixed.

>> -    return offset;
>> +    if (info->vpd_page83[3] == 0) {
>> +       req = scsi_req_get(sdev, (uint32_t) -1, lun);
>> +       if (!req)
> 
> This would be against CODING_STYLE but nobody seems to care.
> 
... And quite some other places, notably the tabs vs. spaces issue.
Fixed now.

[ .. ]
>> +
>> +struct dcmd_cmd_tbl_t {
> 
> static const?
> 
If you feel like it, okay.

[ .. ]
>> +    DPRINTF("Using %d sges, %d cmds, %s mode\n",
>> +           s->fw_sge, s->fw_cmds, s->is_jbod?"jbod":"raid");
> 
> Please add spaces around '?' and ':'.
> 
Ok, fixed.

Thanks for the feedback. I'll be including the fixes with
the next patchset.

Cheers,

Hannes