Re: [Qemu-devel] [SeaBIOS] [PATCH 2/2] lsi53c895a boot support

[Kevin O'Connor <kevin@koconnor.net>]

On Thu, Jul 19, 2012 at 06:15:03PM +0200, Gerd Hoffmann wrote:
> This patch adds support for the lsi53c895a scsi host adapter,
> allowing seabios to boot from scsi disks and cdroms connected
> to the lsi scsi hba emulated by qemu.
> 
> This driver was written by looking at the expectations of qemu's
> lsi emulation.  I have no idea idea how close this is to work on
> real hardware, and I somehow doubt anyone cares given the age of
> physical lsi scsi cards.  It depends on !COREBOOT for that reason.

Thanks.  Looks good to me.  I definitely agree on marking it as
!COREBOOT.  Some minor comments below.

[...]
> --- a/src/Kconfig
> +++ b/src/Kconfig
> @@ -119,6 +119,12 @@ menu "Hardware support"
>          default y
>          help
>              Support boot from virtio-scsi storage.
> +    config LSI_SCSI
> +        depends on DRIVES && !COREBOOT
> +        bool "lsi53c895a scsi controllers"
> +        default y
> +        help
> +            Support boot from lsi53c895a scsi storage.

It's probably worthwhile to point out in the help that this is for
QEMU emulation only.

[...]
> +static int
> +lsi_scsi_cmd(struct lsi_lun_s *llun, struct disk_op_s *op,
> +             void *cdbcmd, u16 target, u16 lun, u16 blocksize)
> +{
> +    u32 iobase = GET_GLOBAL(llun->iobase);
> +    u32 dma = ((cdb_is_read(cdbcmd, blocksize) ? 0x01000000 : 0x00000000) |
> +               (op->count * blocksize));
> +    u8 msgout[] = {
> +        0x80 | lun,                 // select lun
> +        0x08,
> +    };
> +    u8 status = 0xff;
> +    u8 msgin_tmp[2];
> +    u8 msgin = 0xff;
> +
> +    /* FIXME: probably not the best idea to have this on the stack ... */
> +    u32 script[] = {

Several places in the code DMA to the stack already (usb-ehci,
usb-uhci, ata).

[...]
> --- /dev/null
> +++ b/src/lsi-scsi.h
> @@ -0,0 +1,2 @@
> +void lsi_scsi_setup(void);
> +int lsi_scsi_cmd_data(struct disk_op_s *op, void *cdbcmd, u16 blocksize);

Should have a header guard.

-Kevin