typed PCI registers, virtio MAC randomization, SMP syscall trap#2367
typed PCI registers, virtio MAC randomization, SMP syscall trap#2367uadhran wants to merge 2 commits into
Conversation
…P syscall trap - Replace PCI config register #defines with typed enum classes (includeos#2333) - Randomize virtio-net host MAC bytes on init to avoid local collisions (includeos#1409) - Set up syscall trap on AP cores via shared init_syscall_trap() (includeos#2358)
Drivers initialize before RNG::init(), so rng_extract() was reading uninitialized state. Use RDRAND/RDSEED when available, with a pci_addr + clock fallback otherwise.
uadhran
changed the title
uadhran
changed the title
|
When looking statically PCI/enum and static casting seems fine. Syscall handler for SMP is very much wanted, but maybe the init function should be part of |
|
Thanks for the review @torgeiru . PCI enums — noted, glad the static casts look fine. SMP syscall trap — agree that Virtio MAC — addresses #1409: QEMU gives the same default virtio-net MAC for every instance, which breaks running multiple copies locally. After reading the MAC from virtio config we keep the prefix from the device and randomize the host-specific bytes ( Tests — unit tests pass here as well ( |
|
I suppose MAC identifier generation is not crypto. But why did you make a distinct function for MAC RNG within VirtioNET? I believe this generation can be decoupled from the driver and reused by other drivers e.g. As a side note: These commits/PRs can be broken into multiple distinct PRs. SMP and PCI are probably an instant accept but because it is coupled with MAC the merging is stalled (or would be if the maintainers weren't on vacation hehe). Edit: I think these things could be fixed for follow up PRs. See #2352 for vision :) |
|
Thanks for this! Nice cleanup. I just ran all the tests, and everything seems to be working fine. ✨ A few notes, though. Looking at the changes, it seems there are two separate PRs: one regarding the PCI devices, and one regarding the initialization of the syscall traps. Would it be inconvenient to separate this into two PRs? I know the maintainers have a preference for smaller PRs: they are quicker to review. For the PCI devices, I don't think we have any proper integration tests (what was failing before this PR? maybe this is an opportunity to test for that?). We have Regarding syscall trap initialization, it always felt weird to me that this is part of the libc initialization. I suspect this is necessary cuz of the C++ runtime, but I haven't dug too deep into that. Nice to separate that, thanks. Otherwise, I love the code style :) lgtm |
3 participants
Tested:
nix-build unittests.nix— 85/85 unit tests passed (Nix 2.34.7, Linux).Not run yet:
./test/test.sh, integration/unikernel builds.Three small fixes:
PCI config registers (#2333)
PCI register offsets were plain #defines in the header. Moved them to
enum classes (
config_reg,command,cap_id) for type safety.Updates in
pci_device.cppandpci_msi.cpp.VirtioNet MAC (#1409)
QEMU hands out the same default MAC on every virtio-net device, which is
awkward when running multiple instances locally. After reading the MAC
from virtio config we randomize the host-specific bytes and set the
locally-administered bit. Uses RDRAND/RDSEED when available, with a
pci_addr + clock fallback (drivers init before
RNG::init()).SMP syscall trap (#2358)
Only CPU 0 had the syscall MSR set up. Pulled that into
init_syscall_trap()and call it from both the BSP libc init pathand
revenant_main. Handler still panics — syscalls aren'timplemented — but APs get a trap instead of nothing.
Related: #2333, #1409, #2358