PCI ATA+AHCI Support

[thesilvanator]

Hey,

I took a shot at adding basic AHCI+SATA support. Still needs a lot of work, it can only read and write one block at a time, and it's only within the kernel space. And only supports ATA requests, not ATAPI.

Let me know if it works for you and what you think. I'd be happy to make adjustments based on your feedback and whatnot. Once I get it aligned with what you're looking for, I'm planning to add some more features as well and make it a bit robust. Maybe after that it can be integrated within the whole system so the root filesystem doesn't have to be baked into the GRUB image anymore.

Also, need some guidance on allocating physical and virtual memory both contiguously that can be mapped to one another. See ahci.c line 88 and sata.c line 80. I can elaborate a bit more if you'd like.

Let me know what you think.

[29jm]

Wow, this is great! Really appreciate the effort, this is a big one.

It works for me, a few things that come to mind:

• making the ISO itself a drive in qemu is really cool
• you're doing memory mapping right (similar examples in term.c:51 and fb.c:28), as long as the memory you remapped is never freed (or you restore the original mapping before freeing it)
• you could replace the implementation of read_block/write_block/clear_block in ext2.c with equivalent sata calls to try on-disk ext2 (using isodir/modules/disk.img as the drive image for qemu).
• eventually ext2_fs_t::device needs to be replaced by an abstraction (block_device_t?) that provides read_block/write_block. It'd be passed as argument in init_ext2 as the underlying storage instead of the current ram buffer. sata.c would provide a function that turned a specific device into a block_device_t (the same way an ext2_fs_t implements fs_t)
• I like the way you organized things between ahci and sata
• you could remove ide.c/ide.h I think
• your code style is slightly different from the project's, but I can fix that once merged, I don't want to bother you with that
• adding a max try count (10000 or something?) to the waiting/infinite-looking while loops could be a good idea
• I have so much reading to do to get up to speed on this :)

For what it's worth, I tried booting on my old laptop and got a black screen with the pci/ahci init calls in kernel.c. It has an SSD (with stuff on it I don't care about =). I also tried it with the branch rebased on master, and/or with the ps2-debugging branch merged, same outcome. It could be that one of the init functions of ahci/pci is stuck in a loop, but it could also be that one of them crashes and the kernel ends up in the fault handler. It would be nice if things booted even if something went wrong during ahci/pci init, but it's not a breaker if this turns out to be hard to fix. If I can run any test for you let me know.

One thing I would ask before merging would be to fix the warnings (you can compile with make strict to treat them as errors), but then I will merge whenever you think it's ready. I don't mind if you prefer working in one PR or several. This will need to be merged to master sometime soon also.

I'll keep reading the code for now, let me know when merge time comes, or if there's anything I can do.

[thesilvanator]

Wow! Thanks for the detailed reply! I'm glad you're happy with what I've added so far!

Sorry about the warnings. I had every intention on having them fixed before submitting the pull request, but I got so focused on finishing before my summer Uni courses started that I just saw the build pass and the warnings completely slipped my mind haha.

I just wanna confirm some things quickly then about the memory allocation:

• From what I saw, unmapping a virtual address from a physical address also free's the underlying physical frames, correct? So, for example in ahci.c:90, the physical frames that were allocated using kamalloc are now free and not being wasted? Just looking for your confirmation on that.
• Additionally, if I want to expand the size of an ATA request beyond just (1-8 blocks, ie. a frame size), I would need to be able to allocate physical frames contiguously. From what I saw in malloc.c, the whole kernel heap is mapped contiguously into physical memory, so if I kamalloc space that takes up multiple pages, I can also be sure the physical frames are also one after another, correct?

I'm hoping I can get a quick commit in with some simple short term changes within the next week or so. Mainly it would be:

• Fix the warnings
• Add a spin check/max try count to the while loops
• Expand the size of the read and write SATA calls based on your reply to my memory questions above.

Then long term maybe I can add some of the other things discussed like getting it working with ext2 and a block abstraction layer, as well as getting it to work on physical hardware.

Lastly, here are some of the resources I used when adding this. Hopefully they can be of help to you as well!

• The osdev wiki pages for PCI and AHCI (I'm sure you've been all over the osdev wiki page already though haha). Also the osdev wiki for a PCI IDE controller was helpful in understanding the FIS structures in terms of ports on the controller.
• The Intel AHCI manual. It's a PDF with clickable links and a table of contents which is super helpful
• SATA 3.2 Specification. Apparently you have to pay for the SATA manual, but I managed to find this one for free online.
• Documentation for the ATA command set

Thanks again!

[29jm]

• From what I saw, unmapping a virtual address from a physical address also free's the underlying physical frames, correct? So, for example in ahci.c:90, the physical frames that were allocated using kamalloc are now free and not being wasted?

You're right on both counts, but I realize now that it's not the best way to do things because of this deallocation+reallocation of the physical frames, which can be avoided (and should, fragmentation is scary). It should be done in a similar way as in

SnowflakeOS/kernel/src/devices/fb.c

Lines 28 to 35 in 8fc4dce

	// Remap our framebuffer
	uint32_t size = fb.height*fb.pitch;
	uintptr_t buff = (uintptr_t) kamalloc(size, 0x1000);
	for (uint32_t i = 0; i < size/0x1000; i++) {
	page_t* p = paging_get_page(buff + 0x1000*i, false, 0);
	*p = (address + 0x1000*i) | PAGE_PRESENT | PAGE_RW;
	}

which is a more manual-looking approach, but more proper let's say. It would justify a paging_remap_pages function probably, but it'll do fine for now :)

• if I kamalloc space that takes up multiple pages, I can also be sure the physical frames are also one after another, correct?

That's right, and I think it's fair to assume it will stay that way although it's not yet documented (I will so I don't forget). Just to make sure, is this for a one-time (or per-device) allocation ? If at all possible, this is the way to go.

Thank you for the resources, these are great and definitely a needed addition to my reading.
Also sorry for the late reply, let me know if I've missed anything.

edit: silly mistake in the code above with the simple division by 0x1000, which you did correctly. There's even a function for that, divide_up in kernel/sys.h. I guess I need to audit all my divisions.

[thesilvanator]

Hey!

Firstly, no worries about the late response. Please don't feel rushed at all with this. I'm working on a few other projects, as well as some school stuff, so I'm taking the pace of this a bit slower.

I added the spin/loop checks and fixed the warnings like discussed. In regards to expanding the size of a read/write beyond a single 512 byte block, some of the other examples I've been looking at simply have the function caller provide the buffer location, and the read/write function simply sets that as the location for the ahci controller to write to. I think this would probably be a better method, especially considering the case where a caller may want the memory location to actually be a mmio'd address. Additionally, it removes the overhead of memcpying over the data after the read is complete/before the write.

Changing it to this method would be super fast for any changes made to the driver, so I'm thinking of just leaving that until I begin trying to implement the driver within the system (which like I said will be a bit more of a long term goal haha).

So basically, this is what I have for now, and I may have some bigger, longer term changes coming in the next months if possible.

Hope that's alright. Let me know what you think!

(oh, and thanks for confirming my questions above regarding the mem allocation!)

[29jm]

Hi! Just wanted to let you know that I plan to merge this PR in the coming week, I like those changes and from brief testing it looks good. I think your idea of having the caller provide the buffer to write to makes a ton of sense, I think this is the way to go.