[SeaBIOS] [PATCH] block: add NVMe boot support
Stecklina, Julian
jsteckli at amazon.com
Tue Jan 24 22:43:41 CET 2017
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
More information about the SeaBIOS
mailing list