-
Notifications
You must be signed in to change notification settings - Fork 182
win_virtio_drivers: add viosock driver support for update tests #4423
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
WalkthroughAdded VirtIO Socket (viosock) support: a new "viosock" entry in Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
e85e56c to
22de3c1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
qemu/tests/cfg/win_virtio_driver_update_test.cfg (1)
335-356: Consider removing redundantcdroms += " virtio"inwith_viosockThe
with_viosockvariant wiring (driver name, device name, HWIDs, vsock settings, transfer commands) looks consistent with the Python driver metadata and the stress variant. However, the base test already setscdroms += " virtio winutils", so addingcdroms += " virtio"again here likely just duplicates the same CD-ROM alias and can be confusing. If no extra CD is actually needed, consider dropping this line.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
provider/win_driver_utils.py(1 hunks)qemu/tests/cfg/driver_load_stress.cfg(1 hunks)qemu/tests/cfg/win_virtio_driver_update_test.cfg(1 hunks)
🔇 Additional comments (1)
provider/win_driver_utils.py (1)
63-66: Viosock driver metadata is consistent with existing entriesThe new
viosockentry mirrors the structure of other PCI drivers and matches thedevice_hwid/device_nameused in the cfg variants, so it should integrate cleanly with the existing helper functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
qemu/tests/cfg/driver_load_stress.cfg (1)
236-257: Previous review feedback still applies.A past review already flagged the missing
driver_id_patternand redundantcdroms += " virtio"line. Additionally,virtio_win_media_type = isoon line 246 is also redundant since the base configuration already sets it at line 10.
🧹 Nitpick comments (1)
qemu/tests/cfg/win_virtio_driver_update_test.cfg (1)
335-356: Adddriver_id_patternand remove redundant settings.The variant is missing
driver_id_pattern, which other driver variants in this file use for device identification (seewith_viomemat line 333,with_viofsat line 256). Additionally, lines 344-345 are redundant since the base configuration already setscdroms += " virtio winutils"(line 6) andvirtio_win_media_type = iso(line 10).- with_viosock: no Host_RHEL.m6 Host_RHEL.m7 Host_RHEL.m8 no during_bg_test vsocks = vhost_vsock0 driver_name = "viosock" driver_verifier = "VirtioSocket" device_name = "VirtIO Socket Driver" device_hwid = '"PCI\VEN_1AF4&DEV_1053" "PCI\VEN_1AF4&DEV_1012"' + driver_id_pattern = "(.*?):.*?VirtIO Socket Driver" run_bgstress = vsock_transfer_data - cdroms += " virtio" - virtio_win_media_type = iso required_virtio_win_prewhql = [0.1.294, ) required_virtio_win = [1.9.51.0, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
provider/win_driver_utils.py(1 hunks)qemu/tests/cfg/driver_load_stress.cfg(1 hunks)qemu/tests/cfg/win_virtio_driver_update_test.cfg(1 hunks)
🔇 Additional comments (1)
provider/win_driver_utils.py (1)
63-66: LGTM!The viosock driver entry follows the established pattern and is consistent with the viomem entry above (both use raw strings). The hardware IDs (DEV_1053 for modern/1.0+ and DEV_1012 for legacy/transitional) are correct for the VirtIO Socket device.
22de3c1 to
fd31e94
Compare
|
depends on avocado-framework/avocado-vt#4287 |
|
depends on #4422 |
|
uninstall_install and downgrade_upgrade test results: |
fd31e94 to
0ed60ef
Compare
|
disable/enable test results: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
qemu/tests/cfg/win_virtio_driver_update_test.cfg (1)
335-356: Viosock downgrade/upgrade still hits missing-driver ISO; consider gating + verify driver_verifier nameGiven your job log,
with_viosock.*.downgrade_upgradewill continue to fail untilcdrom_virtio_downgradepoints to an ISO that actually containsviosock.inf. Right now this variant only disablesduring_bg_test, so downgrade/upgrade remains in the matrix.If you don’t plan to update the downgrade ISO immediately, I’d recommend gating downgrade/upgrade for viosock to avoid environment‑driven false negatives:
- with_viosock: no Host_RHEL.m6 Host_RHEL.m7 Host_RHEL.m8 no during_bg_test + no downgrade_upgrade vsocks = vhost_vsock0 driver_name = "viosock" driver_verifier = "VirtioSocket"Also, please double‑check that
driver_verifier = "VirtioSocket"matches the actual name Windows Driver Verifier expects for this driver; if the string is off, DV won’t actually be applied even though the test passes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
provider/win_driver_utils.py(1 hunks)qemu/tests/cfg/driver_load_stress.cfg(1 hunks)qemu/tests/cfg/win_virtio_driver_update_test.cfg(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- qemu/tests/cfg/driver_load_stress.cfg
- provider/win_driver_utils.py
0ed60ef to
b56b187
Compare
|
Hi @xiagao Could you please review this PR?Thanks. |
- Add viosock driver info(hwid/device name) in win_driver_utils.py - Configure viosock test variant in driver_load_stress.cfg - Configure viosock test variant in win_virtio_driver_update_test.cfg - Set vsock transfer parameters and background stress testing options ID: 4313,4314,4315 Signed-off-by: Leidong Wang <[email protected]>
b56b187 to
c9c25c6
Compare
|
Latest test results: JOB LOG : /root/avocado/job-results/job-2025-12-23T00.17-7c3b28a/job.log |
xiagao
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
ID: 4313,4314,4315
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.