Add support for PCI device pass through #205
Conversation
There was a problem hiding this comment.
Hi @romantomjak,
Thanks for this PR! I've taken a look at the options you're introducing and responded to your comments. I also added a few comments and suggestions that should be fixed before we can merge, but overall this looks good to me.
I'll let you address those comments, and I'll circle back later.
As always please let me know if you need any kind of help!
romantomjak
force-pushed
the
main
branch
2 times, most recently
from
5c9d64c
to
0e6bfe9
Compare
| @@ -441,3 +442,59 @@ func TestStartVMWithForce(t *testing.T) { | |||
| }) | |||
| } | |||
| } | |||
|
|
|||
| func TestStartVM_AssertInitialQuemuConfig(t *testing.T) { | |||
There was a problem hiding this comment.
Added this test to make sure the value is actually inverted when the config is assembled
| @@ -365,18 +368,30 @@ or responding to pattern `/dev/.+`. Example: | |||
|
|
|||
| - `machine` - (string) - Set the machine type. Supported values are 'pc' or 'q35'. | |||
|
|
|||
| #### VirtIO RNG device | |||
There was a problem hiding this comment.
De-dented header size. The config reference section has h2 headers, so I think this must be h3.
There was a problem hiding this comment.
Hi @romantomjak,
Thanks for the quick reroll, this looks good to me already :)
I left a couple more comments on the reworked PR, but as I said above, this looks really good, so I don't think there's a lot left to change here.
That being said, I'll let you have a look at what I point out/suggest, and let me know which deserve some attention.
I'll circle back later when you've had time to address those.
Thanks again for this PR!
There was a problem hiding this comment.
Hi @romantomjak,
Thanks again for the reroll! I left a couple of comments regarding the docs (especially the HCL example), and regarding usage of setDeviceParamIfDefined, especially after the discussion on #207.
I implemented changes locally, but I can't push them on your branch unfortunately because of permission issues. Let me know if you want me to share patches, otherwise please feel free to address those and we can circle back on it later.
In the meantime, I'm pre-approving this PR, we are very close to completion here I think, good job and thanks again for this enhancement!
| // | ||
| // ```hcl | ||
| // pci_devices { | ||
| // host = "0000:0d:00.1" |
There was a problem hiding this comment.
In HCL2 generally, the contents of a block are intented (with 2 spaces) relative to their immediate parent, I suggest we do the same here, is this OK?
There was a problem hiding this comment.
Sorry, I can't get my editor to do that. If I add two spaces it just reindents everything extremely weirdly like the rng0. I tried changing that via vim, but as soon as I save the file in vscode it just reindents it again 🙈
There was a problem hiding this comment.
It's OK! I can do this change in a subsequent PR either way, so I think I'll merge your PR if everything checks out, and fix this later :)
There was a problem hiding this comment.
With the last changes, LGTM!
I'll merge this as soon as tests are green. Many thanks once more @romantomjak!
| // | ||
| // ```hcl | ||
| // pci_devices { | ||
| // host = "0000:0d:00.1" |
There was a problem hiding this comment.
It's OK! I can do this change in a subsequent PR either way, so I think I'll merge your PR if everything checks out, and fix this later :)
This PR introduces the
pci_devicesblock to allow mapping host's PCI devices into the guest VM. This is often used to passthrough a graphics card or a network adapter. Devices that are mapped into a guest VM are no longer available on the host.