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