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

Kevin O'Connor kevin at koconnor.net
Tue Jan 24 23:06:07 CET 2017


On Tue, Jan 24, 2017 at 09:43:41PM +0000, Stecklina, Julian wrote:
> 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.

Thanks.

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

Can you point me to which ones?  I see one of the AHCI allocations
could potentially be moved out of the fseg, but I don't see anything
else.

[...]
> > > +_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.

Thanks.  C99 is fine - it's just _Atomic, _Static_assert, and the one
for(u16 i ...).

[...]
> > Ideally the code would start a thread here:
> >     run_thread(nvme_controller_setup, pci);
> 
> What's the advantage of that?

It runs the controller setup in it's own "thread" - see:
  https://www.seabios.org/Execution_and_code_flow#Threads
It has no real impact on VMs, but it can be significant on real
hardware.  I prefer to see the drivers use it regardless because the
drivers tend to get copied.

-Kevin



More information about the SeaBIOS mailing list