Re: [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
> >
> > 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?

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