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

[Paolo Bonzini]

On 27/01/2016 16:57, Kevin O'Connor wrote:
> On Wed, Jan 27, 2016 at 03:51:02PM +0100, Paolo Bonzini wrote:
>> From: Don Slutz <Don.Slutz at Gmail.com>
>>
>> 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 <Don.Slutz at Gmail.com>
>> Signed-off-by: Paolo Bonzini <pbonzini at redhat.com>
> 
> Thanks.  Is the upstream support for this in QEMU now?

Not yet, but it should be in 2.6.

>> 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().

It should work on real hardware; the submission of requests works more
or less the same as for a full-blown driver.  I wouldn't have problems
with adding the dependency though, since real hardware would come with
an option ROM.

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

It is the target of DMA, but we don't read it; because we are only
sending out one request at a time, we know that the hardware will write
0x80000000|((u32)reply_msg >> 1).  We still have to set up four free
bytes for hardware to use.

But VARLOW is better indeed.

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

It's a bit more complicated than that; the request is outstanding and I
cannot send another one until it completes.  So I would have to do one
of two things:

1) reset the adapter, which would probably not have a timeout so that in
the end there is no difference from the current code;

2) bring the disk offline entirely in the case of a timeout, which can
possibly be a cure worse than the diseases.

So for something as simple as INT 13h I think anything more complex than
such a loop has no actual benefit.  Of course that would be different in
a more complex environment.

Paolo