e1000 driver by cozis · Pull Request #241 · vvaltchev/tilck

cozis · 2026-03-26T09:56:03Z

This is the e1000 I've been working on. I just wanted to get your first thoughts on the general structure. If everything looks good, I will go through the code again and make sure it follows the contributing guidelines (including squashing the branch and restored and modified files that didn't need to be, such as syscalls.h).

cozis · 2026-03-26T14:23:52Z

Hmm looks like there are some issues with RISC-V

vvaltchev

Good change, @cozis! There are few things to address, mostly cosmetic. Plus all the CI/CD failures need to be addressed. Finally, the commits should be squashed into one.

vvaltchev · 2026-03-29T20:16:00Z

include/tilck/kernel/modules.h

 #define MOD_serial_prio                      400
 #define MOD_sb16_prio                        410
 #define MOD_null_prio                        420
+#define MOD_e1000_prio                       430 // TODO: check that this is right


This priority is OK.

vvaltchev · 2026-03-29T20:16:35Z

include/tilck/kernel/net.h

@@ -0,0 +1,23 @@
+


Missing:

/* SPDX-License-Identifier: BSD-2-Clause */

vvaltchev · 2026-03-29T20:17:02Z

include/tilck/kernel/sync.h


 void kcond_init(struct kcond *c);
-void kcond_destory(struct kcond *c);
+void kcond_destroy(struct kcond *c);


Thanks for noticing this typo! I don't know how it survived so long unnoticed!

vvaltchev · 2026-03-29T20:17:20Z

include/tilck/kernel/sys_types.h

 #include <sys/utsname.h>  // system header
 #include <sys/stat.h>     // system header
 #include <fcntl.h>        // system header
+#include <arpa/inet.h>


Missing // system header comment

vvaltchev · 2026-03-29T20:18:06Z

kernel/arch/generic_x86/pic.c

   outb(PIC2_IMR, ICW4_8086);
   pic_io_wait();

+   outb(PIC1_IMR, 0xff);


Can you clarify why do we need that?

The changes in pic.c are redundant. I was having troubles getting the IRQ handler to fire, so I added these lines to ensure the PIC was letting IRQs through. I'll restore it as it was before

vvaltchev · 2026-03-29T20:41:54Z

modules/e1000/e1000.c

+    printk("e1000: Sending frame len=%d\n", len);
+
+    /* Number of descriptors that would be required to send this message. */
+    u32 num_desc = CEIL((u32) len, TX_BUF_SIZE);


If len was unsigned we wouldn't need those casts.

vvaltchev · 2026-03-29T20:42:20Z

modules/e1000/e1000.c

+            /*
+            * Slow path. Packet spans multiple entries. Not implemented yet.
+            */
+            ASSERT(0);


Style: NOT_REACHED().

Is NOT_IMPLEMENTED() more appropriate here?

vvaltchev · 2026-03-29T20:42:41Z

modules/e1000/e1000.c

+
+            void  *src = PA_TO_KERNEL_VA(rx_ring[rx_tail].addr);
+            size_t len = rx_ring[rx_tail].length;
+


Style: extra empty line.

vvaltchev · 2026-03-29T20:43:38Z

modules/e1000/e1000.c

+            */
+
+            void  *src = PA_TO_KERNEL_VA(rx_ring[rx_tail].addr);
+            size_t len = rx_ring[rx_tail].length;


You're using size_t while length is u16. Why not using everywhere u32 (except in the struct, of course).

vvaltchev · 2026-03-29T20:49:27Z

scripts/templates/qemu/run_qemu

   -gdb tcp::$GDB_PORT                                 \
   @QEMU_RAM_OPT@                                      \
   -drive id=img1,format=raw,if=none,file=@IMG_FILE@   \
+   -netdev user,id=net0,hostfwd=udp::2222-:22          \


If you add this to the QEMU scripts, you should make the e1000 module disabled by default as it's in-development at the moment. You can do this by adding it to this list:

list( APPEND disabled_modules_list # No modules in this list, at the moment )

In the main CMakeLists.txt file. If something is not mature enough, shouldn't be enabled by default.

Just curious, why is adding the netdev to qemu related to disabling the module by default?

Also, what is the normal way to enable it without modifying the CMakeLists.txt?

cozis · 2026-03-29T21:19:31Z

Thanks for the review! I'll go through it in the next couple days

vvaltchev · 2026-03-30T18:26:17Z

In kernel mode we don't use `size_t` as much as in userspace. size_t is the goto/nobrainer type for "size" in userspace. In the kernel, usually, we try to use the more efficient type for the purpose. Even on x86-64, 64-bit operations are a bit more expensive than 32-bit ones, due to the need for the REX.W prefix. When interacting with HW buffers almost where we operate with multi GB buffers. So, u32 is already big enough. Notice the length field in the struct is 32 bit, anyway. So, yeah, in summary, the rule of the thumb is to use u32 by default for unsigned types, unless there's a serious use-case for 64 bit, like file sizes and offsets. For memory addresses, we should use the variable size `long` / `ulong`.

…

On Mon, Mar 30, 2026 at 1:38 AM Francesco Cozzuto ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In modules/e1000/e1000.c <#241?email_source=notifications&email_token=AALYOLCOWCELQY7ILR26UOL4TIW7TA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIMBSHA4TKNBZGQ3KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJL3QOJPXEZLWNFSXOX3DNRUWG2Y#discussion_r3008353501> : > +} + +static void write_reg(u32 off, u32 val) +{ + if (is_mmio) { + *(volatile u32*) (io_addr + off) = val; + } else { + outl(io_addr + 0x0, off); + outl(io_addr + 0x4, val); + } +} + +/* + * Send packet + */ +static int e1000_send(char *src, int len) I default to signed types to catch overflows with ubsan, but using an unsigned field here is okay. Should it be an u32 or size_t? I think the latter is more appropriate for a number of bytes — Reply to this email directly, view it on GitHub <#241?email_source=notifications&email_token=AALYOLBL36VKIWSWHK6GHST4TIW7TA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIMBSHA4TKNBZGQ3KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJPLQOJPXEZLWNFSXOX3ON52GSZTJMNQXI2LPNZZV6Y3MNFRWW#discussion_r3008353501>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALYOLCJ3SCZDQSGLJTQPYD4TIW7TAVCNFSM6AAAAACXAE5OJ2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DAMRYHE2TIOJUGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

vvaltchev · 2026-03-30T18:27:46Z

I'm sure you can find it. I think it was something like string_util.h, but I cannot access the code at the moment to check.

…

On Mon, Mar 30, 2026 at 1:39 AM Francesco Cozzuto ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In modules/e1000/e1000.c <#241?email_source=notifications&email_token=AALYOLFVPHCYZEFWER37IWT4TIXEXA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIMBSHA4TMMJYGM4KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJL3QOJPXEZLWNFSXOX3DNRUWG2Y#discussion_r3008359689> : > + + /* Number of descriptors that would be required to send this message. */ + u32 num_desc = CEIL((u32) len, TX_BUF_SIZE); + + u32 head = read_reg(REG_TDH); + u32 used = (tx_tail - head + TX_RING_CAP) % TX_RING_CAP; + u32 free = TX_RING_CAP - used - 1; + + if (num_desc > free) + return -ENOMEM; + + for (u32 i = 0, off = 0; i < num_desc; i++) { + + u32 num = MIN((u32) TX_BUF_SIZE, (u32) len - off); + + __builtin_memcpy(PA_TO_KERNEL_VA(tx_ring[tx_tail].addr), src + off, num); I think I was having a hard time finding the definition of memcpy — Reply to this email directly, view it on GitHub <#241?email_source=notifications&email_token=AALYOLDD47YGLRJLXILQZW34TIXEXA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIMBSHA4TMMJYGM4KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJPLQOJPXEZLWNFSXOX3ON52GSZTJMNQXI2LPNZZV6Y3MNFRWW#discussion_r3008359689>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALYOLCPXAOPG3OF657WW7T4TIXEXAVCNFSM6AAAAACXAE5OJ2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DAMRYHE3DCOBTHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

cozis added 17 commits March 9, 2026 11:10

Draft of the e1000 driver

642e3a6

Draft of the ARP protocol

a239a28

Skeleton of the entire stack (IP, ICMP, UDP)

512c6dd

Draft of the socket syscalls

c41a7a7

Add userspace app to test syscalls manually

ce4fb44

Add missing fs_ops function members and properly reference count mnt_fs

a07c587

Draft of the bind system call

060e05e

Bug fixes

5a4225b

Bug fixes

bf79f99

Bug fixes

30a2702

Bug fixes

10a3def

Finally got UDP to work!!!

a0a5626

Add TCP draft

825bb35

Refactor tcp.c

a690505

Move general socket fields to TCP/UDP specific code

e8a2384

Remove network stack code other than the e1000 driver

d9165d3

Fix printk specifier in e1000.c

cff037c

vvaltchev requested changes Mar 29, 2026

View reviewed changes

Implement code review changes

00b66fd

cozis force-pushed the e1000_driver branch from 04c4faf to 00b66fd Compare March 30, 2026 09:13


		void *src = PA_TO_KERNEL_VA(rx_ring[rx_tail].addr);
		size_t len = rx_ring[rx_tail].length;

Conversation

cozis commented Mar 26, 2026

Uh oh!

cozis commented Mar 26, 2026

Uh oh!

vvaltchev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cozis Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cozis Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cozis commented Mar 29, 2026

Uh oh!

vvaltchev commented Mar 30, 2026 via email

Uh oh!

vvaltchev commented Mar 30, 2026 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cozis Mar 30, 2026 •

edited

Loading

cozis Mar 30, 2026 •

edited

Loading