-
Notifications
You must be signed in to change notification settings - Fork 248
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
OS-7458 bhyve should allow pci_slot addressing for NICs #940
base: master
Are you sure you want to change the base?
Conversation
src/vm/man/vmadm.1m.md
Outdated
@@ -1745,6 +1745,35 @@ tab-complete UUIDs rather than having to type them out for every command. | |||
create: yes | |||
update yes (requires zone stop/boot) | |||
|
|||
nics.*.pci_slot: | |||
|
|||
Specifies the virtual PCI slot that this NIC will occupy. Bhyve places |
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.
I also have a partial branch for this that I never got to work due to VM.js being rather confusing around nics...
Perhaps move the generic explenation of the bus/dev/function, we repeat it nearly verbatem for all *.pci_slot property and just keep the default device number used?
The man page is already pretyt long and just repeating nearly the same text a few times makes editing/reading it harder.
@@ -6342,6 +6342,11 @@ function buildNicZonecfg(vmobj, payload, log) | |||
+ nic.allowed_dhcp_cids.join(',') + '")\n'; | |||
} | |||
|
|||
if (nic.hasOwnProperty('pci_slot')) { |
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.
How are we accountin for the primary
property? Should that nic not be kept in the first slot as it currently is?
If i remember the boot.c for bhyve currently the primary nic is always slot 0. (I have not seen your boot.c changes as I can't find the PR for that atm?)
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.
It should still work the same way as today -- we aren't requiring pci_slot
for NICs, so if omitted, the NIC goes in the next available function in bus 0, slot 6
Bump, someone on the mailing list was looking at this to get more than 6 nics. |
Hi guys, any chance this gets merged? :) |
can we merge this four years later? :) |
I see no testing notes in the ticket. I don't have the cycles to test this right now... if someone here does, would LOVE to see the results. (EDIT: addition) Also wondering how it will interact with VMAPI on Triton. Looks like at worst the enhancement will need extra help to be exploited by Triton CNs. |
Unfortunately, I can't recall what testing I did originally. I suspect I likely did basic sanity testing, but was waiting for approvals (that never came) before spending the time to do more formal testing (and run the test suites, etc), and it just got dropped in lieu of other things... |
A couple of things. 1.) Don't forget about TritonDataCenter/illumos-joyent#309 that goes along with this. 2.) Please remember how it may/might/will interact with VMAPI on Triton. Looks like, at worst, the enhancement may/might/will need extra help to be exploited by Triton CNs. 3.) For the adventurous and those who wish to contribute test results, I've placed ISO, USB, PI, and test-tars with this change and the illumos-joyent change on top of release-20241017 here: https://kebe.com/~danmcd/webrevs/OS-7458/ |
Many thanks Dan, Jason. Here are some results with Dan's test platform-20241018T181254Z image. Test: Create a HVH with 8 autoassigned PCI slot VNICs. After creaton, use
Test: Remove the extra added VNIC to see if HVM is still able to start.
|
Test: Create a HVH with 8 manually assigned PCI slot VNICs using the new
|
No description provided.