Re: [SeaBIOS] [PATCH] Support for booting from LSI Logic LSI53C1030, SAS1068, SAS1068e

[Kevin O'Connor]

On Wed, Jan 27, 2016 at 03:51:02PM +0100, Paolo Bonzini wrote:
> From: Don Slutz
>
> Also known as Fusion MPT disk; this controller model is supported
> by VirtualBox and VMware, and QEMU support patches have been
> posted.
>
> Signed-off-by: Don Slutz 
> Signed-off-by: Paolo Bonzini

Thanks.  Is the upstream support for this in QEMU now?

See my comments below.
> Makefile          |   2 +-
>  src/Kconfig       |   6 ++
>  src/block.c       |   3 +
>  src/block.h       |   1 +
>  src/hw/mpt-scsi.c | 307 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/hw/mpt-scsi.h |   8 ++
>  src/post.c        |   2 +
>  7 files changed, 328 insertions(+), 1 deletion(-)
>  create mode 100644 src/hw/mpt-scsi.c
>  create mode 100644 src/hw/mpt-scsi.h
>
> diff --git a/Makefile b/Makefile
> index 3a0d2e8..fcbd88a 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -34,7 +34,7 @@ SRCBOTH=misc.c stacks.c output.c string.c block.c cdrom.c disk.c mouse.c kbd.c \
>      hw/usb.c hw/usb-uhci.c hw/usb-ohci.c hw/usb-ehci.c \
>      hw/usb-hid.c hw/usb-msc.c hw/usb-uas.c \
>      hw/blockcmd.c hw/floppy.c hw/ata.c hw/ramdisk.c \
> hw/lsi-scsi.c hw/esp-scsi.c hw/megasas.c
> hw/lsi-scsi.c hw/esp-scsi.c hw/megasas.c hw/mpt-scsi.c
>
> SRC16=$(SRCBOTH)
>  SRC32FLAT=$(SRCBOTH) post.c memmap.c malloc.c romfile.c x86.c optionroms.c \
>      pmm.c font.c boot.c bootsplash.c jpeg.c bmp.c tcgbios.c sha1.c \
> diff --git a/src/Kconfig b/src/Kconfig
> index b873cd3..8250702 100644
> --- a/src/Kconfig
> +++ b/src/Kconfig
> @@ -208,6 +208,12 @@ menu "Hardware support"
>          default y
>          help
>              Support boot from LSI MegaRAID SAS scsi storage.
> config MPT_SCSI
>    depends on DRIVES
>    bool "LSI MPT Fusion controllers"
>    default y
>    help
>        Support boot from LSI MPT Fusion scsi storage.

Unless there is good reason to believe this will work on real
hardware, I think this should depend on QEMU_HARDWARE and the
mpt_scsi_setup() function should check runningOnQEMU().
>  config FLOPPY
>      depends on DRIVES && HARDWARE_IRQ
>      bool "Floppy controller"
>
> diff --git a/src/block.c b/src/block.c
> index 97e05fa..ef1cd9f 100644
> --- a/src/block.c
> +++ b/src/block.c
> @@ -13,6 +13,7 @@
>  #include "hw/esp-scsi.h" // esp_scsi_process_op
>  #include "hw/lsi-scsi.h" // lsi_scsi_process_op
>  #include "hw/megasas.h" // megasas_process_op
> +#include "hw/mpt-scsi.h" // mpt_scsi_process_op
>  #include "hw/pci.h" // pci_bdf_to_bus
>  #include "hw/pvscsi.h" // pvscsi_process_op
>  #include "hw/rtc.h" // rtc_read
> @@ -521,6 +522,8 @@ process_op_both(struct disk_op_s *op)
>          return esp_scsi_process_op(op);
>      case DTYPE_MEGASAS:
>          return megasas_process_op(op);
> case DTYPE_MPT_SCSI:
>    return mpt_scsi_process_op(op);
> default:
>        if (!MODESEGMENT)
>            return DISK_RET_EPARAM;
>
> diff --git a/src/block.h b/src/block.h
> index 2ff359f..7de6f54 100644
> --- a/src/block.h
> +++ b/src/block.h
> @@ -80,6 +80,7 @@ struct drive_s {
>  #define DTYPE_ESP_SCSI     0x81
>  #define DTYPE_MEGASAS      0x82
>  #define DTYPE_PVSCSI       0x83
> +#define DTYPE_MPT_SCSI     0x84
>  #define DTYPE_SDCARD       0x90
>
> #define MAXDESCSIZE 80
> diff --git a/src/hw/mpt-scsi.c b/src/hw/mpt-scsi.c
> new file mode 100644
> index 0000000..c3b227b
> --- /dev/null
> +++ b/src/hw/mpt-scsi.c
> @@ -0,0 +1,307 @@
> +// MPT Fusion boot support.
> +//
> +// This file may be distributed under the terms of the GNU LGPLv3 license.

I think there should be an explicit copyright statement - for example:

// Copyright (C) 2016  Some One
> +#include "biosvar.h" // GET_GLOBALFLAT
> +#include "block.h" // struct drive_s
> +#include "blockcmd.h" // scsi_drive_setup
> +#include "config.h" // CONFIG_*
> +#include "malloc.h" // free
> +#include "output.h" // dprintf
> +#include "pci.h" // foreachpci
> +#include "pci_ids.h" // PCI_DEVICE_ID
> +#include "pci_regs.h" // PCI_VENDOR_ID
> +#include "std/disk.h" // DISK_RET_SUCCESS
> +#include "string.h" // memset
> +#include "util.h" // usleep
>
> +#define MPT_REG_DOORBELL  0x00
> +#define MPT_REG_WRITE_SEQ 0x04
> +#define MPT_REG_HOST_DIAG 0x08
> +#define MPT_REG_TEST      0x0c
> +#define MPT_REG_DIAG_DATA 0x10
> +#define MPT_REG_DIAG_ADDR 0x14
> +#define MPT_REG_ISTATUS   0x30
> +#define MPT_REG_IMASK     0x34
> +#define MPT_REG_REQ_Q     0x40
> +#define MPT_REG_REP_Q     0x44
>
> +#define MPT_DOORBELL_MSG_RESET 0x40
> +#define MPT_DOORBELL_HANDSHAKE 0x42
>
> +#define MPT_IMASK_DOORBELL 0x01
> +#define MPT_IMASK_REPLY    0x08
>
> +struct mpt_lun_s {
> struct drive_s drive;
> struct pci_device *pci;
> u32 iobase;
> u8 target;
> u8 lun;
>
> +};
>
> +u8 reply_msg[4] __attribute((aligned(4))) VARFSEG;

If I'm reading this correctly, this variable is a target of DMA.  I
don't think it is a good idea to attempt DMA to the f-segment (eg, the
f-segment is read-only at runtime).  If this needs to be a global
variable (instead of stack allocated) then VARLOW should work.
> +#define MPT_MESSAGE_HDR_FUNCTION_SCSI_IO_REQUEST        (0x00)
> +#define MPT_MESSAGE_HDR_FUNCTION_IOC_INIT               (0x02)
>
> +static struct MptIOCInitRequest
> +{
> u8     WhoInit;             /* Which system sent this init request. */
> u8     Reserved1;           /* Reserved */
> u8     ChainOffset;         /* Chain offset in the SG list. */
> u8     Function;            /* Function to execute. */
> u8     Flags;               /* Flags */
> u8     MaxDevices;          /* Max devices the driver can handle. */
> u8     MaxBuses;            /* Max buses the driver can handle. */
> u8     MessageFlags;        /* Message flags. */
> u32    MessageContext;      /* Message context ID. */
> u16    ReplyFrameSize;      /* Reply frame size. */
> u16    Reserved2;           /* Reserved */
> u32    HostMfaHighAddr;     /* Upper 32bit of the message frames. */
> u32    SenseBufferHighAddr; /* Upper 32bit of the sense buffer. */
>
> +} MptIOCInitRequest = {
> .WhoInit = 2,
> .Function = MPT_MESSAGE_HDR_FUNCTION_IOC_INIT,
> .MaxDevices = 8,
> .MaxBuses = 1,
> .ReplyFrameSize = sizeof(reply_msg),
> .HostMfaHighAddr = 0,
> .SenseBufferHighAddr = 0
>
> +};
>
> +struct MptIOCInitReply {
> u8     WhoInit;     /* Which subsystem sent this init request. */
> u8     Reserved1;   /* Reserved */
> u8     MessageLength; /* Message length */
> u8     Function;    /* Function. */
> u8     Flags;       /* Flags */
> u8     MaxDevices;  /* Maximum number of devices the driver can handle. */
> u8     MaxBuses;    /* Maximum number of busses the driver can handle. */
> u8     MessageFlags; /* Message flags. */
> u32    MessageContext; /* Message context ID */
> u16    Reserved2;   /* Reserved */
> u16    IOCStatus;   /* IO controller status. */
> u32    IOCLogInfo;  /* IO controller log information. */
>
> +};
>
> +typedef struct MptSCSIIORequest {
> u8     TargetID;            /* Target ID */
> u8     Bus;                 /* Bus number */
> u8     ChainOffset;         /* Chain offset */
> u8     Function;            /* Function number. */
> u8     CDBLength;           /* CDB length. */
> u8     SenseBufferLength;   /* Sense buffer length. */
> u8     Reserved;            /* Reserved */
> u8     MessageFlags;        /* Message flags. */
> u32    MessageContext;      /* Message context ID. */
> u8     LUN[8];              /* LUN */
> u32    Control;             /* Control values. */
> u8     CDB[16];             /* The CDB. */
> u32    DataLength;          /* Data length. */
> u32    SenseBufferLowAddr;  /* Sense buffer low 32bit address. */
>
> +} __attribute__((packed)) MptSCSIIORequest_t;
>
> +#define MPT_CONTEXT_MAGIC 0x5555aaaa
>
> +typedef struct MptSGEntrySimple32 {
> u32 FlagsLength;
> u32 DataBufferAddressLow;
>
> +} __attribute__((packed)) MptSGEntrySimple32_t;
>
> +static int
> +mpt_scsi_cmd(u32 iobase, struct disk_op_s *op,
>         u8 *cdb, u16 target, u16 lun, u16 blocksize)
>
> +{
> if (lun != 0)
>    return DISK_RET_ENOTREADY;
> u8 sense_buf[18];
> struct scsi_req {
>    MptSCSIIORequest_t      scsi_io;
>    MptSGEntrySimple32_t    sge;
> } __attribute__((packed, aligned(4))) req = {
>    .scsi_io = {
>        .TargetID = target,
>        .Bus = 0,
>        .Function = MPT_MESSAGE_HDR_FUNCTION_SCSI_IO_REQUEST,
>        .CDBLength = 16,
>        .SenseBufferLength = 18,
>        .MessageContext = MPT_CONTEXT_MAGIC,
>        .DataLength = op->count * blocksize,
>        .SenseBufferLowAddr = (u32)MAKE_FLATPTR(GET_SEG(SS), &sense_buf[0]),
>    },
>    .sge = {
>        /* end of list, simple entry, end of buffer, last element */
>        .FlagsLength = (op->count * blocksize) | 0xD1000000,
>        .DataBufferAddressLow = (u32)op->buf_fl,
>    }
> };
> req.scsi_io.LUN[1] = lun;
> memcpy(req.scsi_io.CDB, cdb, 16);
> if (blocksize) {
>    if (scsi_is_read(op)) {
>        req.scsi_io.Control = 2 << 24;
>    } else {
>        req.scsi_io.Control = 1 << 24;
>        req.sge.FlagsLength |= 0x04000000;
>    }
> }
> outl((u32)MAKE_FLATPTR(GET_SEG(SS), &req), iobase + MPT_REG_REQ_Q);
> for (;;) {
>    u32 istatus = inl(iobase + MPT_REG_ISTATUS);
>    if (istatus & MPT_IMASK_REPLY) {
>        u32 resp = inl(iobase + MPT_REG_REP_Q);
>        /* another read to turn interrupt off */
>        inl(iobase + MPT_REG_REP_Q);
>        if (resp == MPT_CONTEXT_MAGIC) {
>            return DISK_RET_SUCCESS;
>        } else if (resp & 0x80000000) {
>            outl((u32)&reply_msg[0], iobase + MPT_REG_REP_Q);
>            return DISK_RET_EBADTRACK;
>        }
>    }
>    usleep(50);
> }

This could loop forever on a hardware error.  I know other SeaBIOS
drivers do this as well, but I'd prefer if new code would have some
eventual timeout (eg, 30 seconds) and fail the request.

-Kevin