[SeaBIOS] [PATCH] block: add NVMe boot support

[Stecklina, Julian]

On Tue, 2017-01-24 at 12:26 -0500, Kevin O'Connor wrote:
> Thanks.  See my comments below.  Mostly minor things I noticed.

Thanks for looking into this. It'll take me a couple of days to post a
new patch, because I'm currently traveling. Expect a new patch next
week.

Btw, some of the issues you pointed out also apply to the AHCI code,
because that was what I was looking at while implementing the NVMe
code. ;)

Some questions below:

> > +    config NVME
> > +        depends on DRIVES
> > +        bool "NVMe controllers"
> > +        default y
> > +        help
> > +            Support for NVMe disk code.
> Is this device also available in real hardware?  Is it expected to
> work on real hardware and/or has it been tested?

It's expected to work on real hardware. I'll see whether I can find
something to test it on other than the Qemu code.

> > +_Static_assert(sizeof(struct nvme_sqe) == 1U << NVME_SQE_SIZE_LOG,
> > "invalid queue entry size");
> > +_Static_assert(sizeof(struct nvme_cqe) == 1U << NVME_CQE_SIZE_LOG,
> > "invalid queue entry size");
> Current SeaBIOS supports being compiled on gcc v3.4 and this
> construct
> isn't supported there.  That compiler is pretty old, but for now it's
> going to be easier to just change this in your patch.

I'll update the patch to remove C11 features and be C89 compatible.

> > +/* Sequentially consistent writes. We have a volatile version for
> > doorbell registers. */
> > +static void nvme_seq_writev(u32 volatile *p, u32 v) { *(_Atomic
> > volatile u32 *)p = v; }
> Same gcc v3.4 issue with _Atomic.  Is _Atomic necessary or just a
> decoration?  The seabios code typically uses readl/writel to make the
> accesses atomic.

The access needs to order memory operations. I'll try to follow what
the rest of SeaBIOS does.

> > +        nvme_controller_setup(pci);
> Ideally the code would start a thread here:
>     run_thread(nvme_controller_setup, pci);

What's the advantage of that?


Julian