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