KolibriOS/kolibrios
15
24
Fork 61
Code Issues 132 Pull Requests 32 Actions Projects 1 Releases Wiki Activity

WIP: Test code to detect VirtIO devices #373

Draft
YVishere wants to merge 10 commits from YVishere/kolibrios:detect_virtio into main
pull from: YVishere/kolibrios:detect_virtio
merge into: KolibriOS:main
Conversation 4 Commits 10 Files Changed 3
+175
YVishere commented 2026-03-16 04:34:08 +00:00
Contributor
Copy Link
Copy Source

Compares Vendor IDs listed in qemu's website with IDs extracted from PCIList to determine whether the driver is VirtIO or not

Compares Vendor IDs listed in qemu's website with IDs extracted from PCIList to determine whether the driver is VirtIO or not
YVishere requested review from dunkaist 2026-03-16 20:42:15 +00:00
dunkaist requested changes 2026-03-16 22:10:41 +00:00
drivers/virtio/virt_io_det.asm
@@ -0,0 +50,4 @@
push ebx
invoke GetPCIList
test eax, eax
dunkaist commented 2026-03-16 22:00:26 +00:00
Owner
Copy Link
Copy Source

It is safe to remove this check: GetPCIList returns a pointer to a statically allocated object which is never zero.

It is safe to remove this check: GetPCIList returns a pointer to a statically allocated object which is never zero.
YVishere marked this conversation as resolved
drivers/virtio/virt_io_det.asm Outdated
@@ -0,0 +57,4 @@
.next_dev:
mov eax, [eax+PCIDEV.fd]
test eax, eax
dunkaist commented 2026-03-16 22:02:45 +00:00
Owner
Copy Link
Copy Source

I'm not sure this can be zero too

I'm not sure this can be zero too
YVishere marked this conversation as resolved
drivers/virtio/virt_io_det.asm Outdated
@@ -0,0 +64,4 @@
jz .finish
mov edx, [eax+PCIDEV.vendor_device_id]
test edx, edx
dunkaist commented 2026-03-16 22:05:37 +00:00
Owner
Copy Link
Copy Source

Can a PCI device have both vendor_id and device_id zeroed?

Can a PCI device have both vendor_id and device_id zeroed?
YVishere marked this conversation as resolved
drivers/virtio/virt_io_det.asm Outdated
@@ -0,0 +77,4 @@
jmp @F
.isVirt:
mov esi, msgVirtIO
dunkaist commented 2026-03-16 22:07:03 +00:00
Owner
Copy Link
Copy Source

You can move this to the line 69 and remove the jmp one line above

You can move this to the line 69 and remove the jmp one line above
YVishere marked this conversation as resolved
drivers/virtio/virt_io_det.asm
@@ -0,0 +80,4 @@
mov esi, msgVirtIO
@@:
DEBUGF 1, "I: %s %x\n", esi, edx
dunkaist commented 2026-03-16 22:10:24 +00:00
Owner
Copy Link
Copy Source

Please, log vendor_id and device_id separately like 1234:5678

Please, log vendor_id and device_id separately like 1234:5678
YVishere marked this conversation as resolved
drivers/virtio/virt_io_det.asm Outdated
@@ -0,0 +95,4 @@
ret
endp
proc service_proc stdcall, ioctl:dword
dunkaist commented 2026-03-16 21:51:15 +00:00
Owner
Copy Link
Copy Source

Is this proc needed?

Is this proc needed?
YVishere commented 2026-03-16 23:41:52 +00:00
Author
Contributor
Copy Link
Copy Source

I added this when I couldn't get my driver to work. I thought maybe having this would solve the issue but I just verified it works without this and the invoke RegService, my_service, service_proc line

I added this when I couldn't get my driver to work. I thought maybe having this would solve the issue but I just verified it works without this and the `invoke RegService, my_service, service_proc` line
YVishere marked this conversation as resolved
YVishere requested review from dunkaist 2026-03-16 23:43:47 +00:00
dunkaist commented 2026-03-17 00:25:18 +00:00
Owner
Copy Link
Copy Source

@YVishere, detection of VirtIO devices seems to work, good job! Since you plan to work on specific VirtIO devices, could you, please, add detection of those now? I.e. not just vendor id's but also device id's. Let's say, for block devices, gpus, network devices.

@YVishere, detection of VirtIO devices seems to work, good job! Since you plan to work on specific VirtIO devices, could you, please, add detection of those now? I.e. not just vendor id's but also device id's. Let's say, for block devices, gpus, network devices.
👍 1 🎉 1
dunkaist marked the pull request as work in progress 2026-03-18 02:56:17 +00:00
YVishere added 1 commit 2026-03-18 04:40:45 +00:00
Added some documentation on register use
Build system / Build (pull_request) Successful in 17m52s
Details
Build system / Check kernel codestyle (pull_request) Successful in 48s
Details
3c8782552d
YVishere force-pushed detect_virtio from ab87edf417 to 3c8782552d 2026-03-18 04:40:45 +00:00 Compare
dunkaist requested changes 2026-03-19 18:09:01 +00:00
drivers/virtio/device_table.inc
@@ -0,0 +11,4 @@
; Returns pointer to the device id stored in data
; clears and returns value in esi
if used PCI_find_dev
dunkaist commented 2026-03-19 17:53:03 +00:00
Owner
Copy Link
Copy Source

Is this 'if' needed?

Is this 'if' needed?
YVishere commented 2026-03-19 23:04:23 +00:00
Author
Contributor
Copy Link
Copy Source

I assumed it helps in memory optimization like an #ifdef in c. I noticed in another .inc file I was looking at so I thought to add it as a good practice.

I assumed it helps in memory optimization like an #ifdef in c. I noticed in another .inc file I was looking at so I thought to add it as a good practice.
dunkaist commented 2026-03-19 23:46:45 +00:00
Owner
Copy Link
Copy Source

If a file can be included into several other files within one project, then such a logic can take place, indeed. For example, macros.inc includes struct.inc, some other file includes struct.inc. It might make sense to put some guard into struct.inc to be included just once. In your case device_table.inc is always included only one time by virt_io_det.asm. You can build the driver with this 'if' and without it and see no difference.

If a file can be included into several other files within one project, then such a logic can take place, indeed. For example, macros.inc includes struct.inc, some other file includes struct.inc. It might make sense to put some guard into struct.inc to be included just once. In your case device_table.inc is always included only one time by virt_io_det.asm. You can build the driver with this 'if' and without it and see no difference.
YVishere commented 2026-03-20 02:10:16 +00:00
Author
Contributor
Copy Link
Copy Source

Oh ok that makes sense

Oh ok that makes sense
YVishere marked this conversation as resolved
drivers/virtio/device_table.inc
@@ -0,0 +21,4 @@
mov esi, ven2_array
cmp [ven_id], VirtVendorID2
jnz @F ;unknown vendor id
dunkaist commented 2026-03-19 18:04:33 +00:00
Owner
Copy Link
Copy Source

Anonymous labels are convenient when jumping for a few lines for an obvious reason. Here you are jumping quite far over several named labels. Please, make this label named too.

Anonymous labels are convenient when jumping for a few lines for an obvious reason. Here you are jumping quite far over several named labels. Please, make this label named too.
YVishere marked this conversation as resolved
drivers/virtio/device_table.inc Outdated
@@ -0,0 +24,4 @@
jnz @F ;unknown vendor id
.pre_loop:
push edx
dunkaist commented 2026-03-19 18:03:12 +00:00
Owner
Copy Link
Copy Source

You push/pop edx in several places. It will be easier to push just once at the begin of the function and pop at the end

You push/pop edx in several places. It will be easier to push just once at the begin of the function and pop at the end
YVishere commented 2026-03-19 23:14:18 +00:00
Author
Contributor
Copy Link
Copy Source

Yeah I came up with this so that there would be fewer memory accesses. Maybe the branches could minimize the benefits though

Yeah I came up with this so that there would be fewer memory accesses. Maybe the branches could minimize the benefits though
YVishere marked this conversation as resolved
drivers/virtio/device_table.inc
@@ -0,0 +26,4 @@
.pre_loop:
push edx
.loop:
dunkaist commented 2026-03-19 17:59:36 +00:00
Owner
Copy Link
Copy Source

'.loop' is a faceless name, use something more descriptive like '.check_next_device', '.next_dev', or similar

'.loop' is a faceless name, use something more descriptive like '.check_next_device', '.next_dev', or similar
YVishere marked this conversation as resolved
drivers/virtio/device_table.inc Outdated
@@ -0,0 +35,4 @@
cmp edx, [dev_id]
jz .found
add esi, VEN_STRIDE
dunkaist commented 2026-03-19 17:58:24 +00:00
Owner
Copy Link
Copy Source

If you move this addition a few lines above, you can replace these two branches

        jz     .found
        jmp     .loop

with just one
jnz .loop

If you move this addition a few lines above, you can replace these two branches ``` jz .found jmp .loop ``` with just one ` jnz .loop`
YVishere marked this conversation as resolved
drivers/virtio/virt_io_det.asm
@@ -0,0 +13,4 @@
__DEBUG__ equ 1
__DEBUG_LEVEL__ equ 1
API_VERSION equ 0 ;debug
dunkaist commented 2026-03-19 18:05:57 +00:00
Owner
Copy Link
Copy Source

Not used

Not used
YVishere marked this conversation as resolved
drivers/virtio/virt_io_det.asm
@@ -0,0 +15,4 @@
API_VERSION equ 0 ;debug
STRIDE equ 4 ;size of row in devices table
dunkaist commented 2026-03-19 18:05:40 +00:00
Owner
Copy Link
Copy Source

Not used

Not used
YVishere marked this conversation as resolved
drivers/virtio/virt_io_det.asm
@@ -0,0 +17,4 @@
STRIDE equ 4 ;size of row in devices table
SRV_GETVERSION equ 0
dunkaist commented 2026-03-19 18:06:08 +00:00
Owner
Copy Link
Copy Source

Not used

Not used
YVishere marked this conversation as resolved
drivers/virtio/virt_io_det.asm Outdated
@@ -0,0 +62,4 @@
shr ecx, 16 ;cx upper half - device id, dx lower half - vendor id
;prepare regs for find dev proc
movzx ecx, cx
dunkaist commented 2026-03-19 18:07:09 +00:00
Owner
Copy Link
Copy Source

The upper half of ecx is already 0 because of 'shr' above

The upper half of ecx is already 0 because of 'shr' above
YVishere marked this conversation as resolved
drivers/virtio/virt_io_det.asm
@@ -0,0 +76,4 @@
jmp .next_dev
.finish:
xor eax, eax
dunkaist commented 2026-03-19 18:08:33 +00:00
Owner
Copy Link
Copy Source

Code style: https://board.kolibrios.org/viewtopic.php?t=1950

Code style: https://board.kolibrios.org/viewtopic.php?t=1950
YVishere marked this conversation as resolved
YVishere added 1 commit 2026-03-19 23:25:13 +00:00
Improved logic flow and code readability based on suggestions
Build system / Check kernel codestyle (pull_request) Successful in 1m21s
Details
Build system / Build (pull_request) Successful in 10m31s
Details
cd73ea0da6
YVishere requested review from dunkaist 2026-03-19 23:25:25 +00:00
YVishere added 1 commit 2026-03-20 02:13:14 +00:00
Conditional compile for array helper proc
Build system / Check kernel codestyle (pull_request) Successful in 56s
Details
Build system / Build (pull_request) Successful in 10m8s
Details
b878e4e8cc
dunkaist commented 2026-03-23 20:14:44 +00:00
Owner
Copy Link
Copy Source

@YVishere, looks good to me. You can consider your test task done.
Feel free to start writing a real VirtIO driver based on this code, if you wish. Formally you don't have to, of course.

@YVishere, looks good to me. You can consider your test task done. Feel free to start writing a real VirtIO driver based on this code, if you wish. Formally you don't have to, of course.
All checks were successful
Build system / Check kernel codestyle (pull_request) Successful in 56s
Required
Details
Build system / Build (pull_request) Successful in 10m8s
Required
Details
This pull request is marked as a work in progress.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u http://git.kolibrios.org/YVishere/kolibrios detect_virtio:YVishere-detect_virtio
git checkout YVishere-detect_virtio
Sign in to join this conversation.
Reviewers
No Reviewers
dunkaist
Labels
Clear labels
C
Category/Applications
Category/Drivers
Category/General
Category/Kernel
Category/Libraries
Eolite
FASM
FS
GSoC
This issue in GSoC program
 HardwareTested
HLL
Influence/Settings
Influence/Text/TYPO
IRCC
Kernel
Kind
Breaking
Breaking change that won't be backward compatible
Kind
Bug
Something is not working
Kind
Build
Kind
Documentation
Documentation changes
Kind
Enhancement
Improve existing functionality
Kind
Feature
New functionality
Kind
Security
This is security issue
Kind
Testing
Issue or pull request related to testing
Pay for the code
Paid task
PR
Conflicts
PR conflicts with main
PR
Dependent
This PR is dependent on another PR
Priority
Critical
The priority is critical
Priority
High
The priority is high
Priority
Low
The priority is low
Priority
Medium
The priority is medium
PR
Ready to merge
Pull request is ready for merge
PR
Request changes
Changes requested in pull request
PR
Review required
Reviewed
Confirmed
Issue has been confirmed
Reviewed
Duplicate
This issue or pull request already exists
Reviewed
Invalid
Invalid issue
Reviewed
Won't Fix
This issue won't be fixed
Status
Abandoned
Somebody has started to work on this but abandoned work
Status
Blocked
Something is blocking this issue or pull request
Status
Need More Info
Feedback is required to reproduce issue or to continue work
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
ace-dent (Andrew) Burer Doczom (Mikhail Frolov) dunkaist (Ivan B) Egor00f (Егор) hidnplayr IgorA Leency (Kiril Lipatov) mxlgv (Max Logaev) ramenu rgimad sdongles (Serhii Sakhno) Sweetbread (Gleb Zaharov)
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: KolibriOS/kolibrios#373
Delete Branch "YVishere/kolibrios:detect_virtio"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?