Skip to content
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

qemu: add usb host passthrough #20540

Merged

Conversation

victortoso
Copy link
Contributor

@victortoso victortoso commented Oct 30, 2023

Hi,

This PR is a WIP related to the request in #16707

At this moment, the PR works for QEMU hypervisor and allows multiple USB devices to be passthrough to podman machine.

Example using vendor:product

toso@tapioca ~/s/podman (usb-host-passthrough)> ./bin/podman machine init tosovm1 --usb vendor=13d3,product=5406 --usb vendor=08ec,product=0016
toso@tapioca ~/s/podman (usb-host-passthrough)> ./bin/podman machine start tosovm1
toso@tapioca ~/s/podman (usb-host-passthrough)> ./bin/podman machine ssh tosovm1 dmesg | grep -i "13d3\|08ec"
[    1.382442] usb 1-1: New USB device found, idVendor=13d3, idProduct=5406, bcdDevice=60.01
[    1.877370] usb 1-2: New USB device found, idVendor=08ec, idProduct=0016, bcdDevice= 2.00
[   10.948755] usb 1-1: Found UVC 1.10 device Integrated Camera (13d3:5406)
[   10.957727] usb 1-1: Found UVC 1.50 device Integrated Camera (13d3:5406)
t

Example using bus-device_number

toso@tapioca ~/s/podman (usb-host-passthrough)> ./bin/podman machine init tosovm2 --usb bus=1,devnum=3 --usb bus=1,devnum=13
toso@tapioca ~/s/podman (usb-host-passthrough)> ./bin/podman machine start tosovm2
toso@tapioca ~/s/podman (usb-host-passthrough)> ./bin/podman machine ssh tosovm2 dmesg | grep -i "13d3\|08ec"
[    1.401638] usb 1-1: New USB device found, idVendor=13d3, idProduct=5406, bcdDevice=60.01
[    1.893233] usb 1-2: New USB device found, idVendor=08ec, idProduct=0016, bcdDevice= 2.00
[   10.928367] usb 1-1: Found UVC 1.10 device Integrated Camera (13d3:5406)
[   10.934437] usb 1-1: Found UVC 1.50 device Integrated Camera (13d3:5406)

Note that this PR does not handle device permissions from the host. Both devices are under my user group, that is:

toso@tapioca ~/s/podman (usb-host-passthrough)> whoami
toso
toso@tapioca ~/s/podman (usb-host-passthrough)> groups
toso wheel docker
toso@tapioca ~/s/podman (usb-host-passthrough)> ls -l /dev/bus/usb/001/003
crw-rw-r--. 1 root toso 189, 2 Oct 30 23:30 /dev/bus/usb/001/003
toso@tapioca ~/s/podman (usb-host-passthrough)> ls -l /dev/bus/usb/001/013
crw-rw----. 1 root toso 189, 12 Oct 30 23:50 /dev/bus/usb/001/013

Disclaimer: This is my first contribution to Podman. Please do feel free to point out anything that I might be doing wrong.

Does this PR introduce a user-facing change?

Yes, it adds a new -usb command line option to podman machine init.

Add USB host passthrough to podman machine (QEMU)

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note labels Oct 30, 2023
@victortoso
Copy link
Contributor Author

@ashley-cui, do you think this is a good way of implementing this feature?

/cc @beriberikix (for knowledge)

@ashley-cui
Copy link
Member

Thanks for the contribution!

Ah, this seems to get past the issue I was running into when looking at this, which was the host perms.

I'd prefer the syntax like --usb vendorid=3d3,productid=5406. This is mostly because this makes the syntax more universal for different platforms, as eventually we may need to support different providers that are not qemu.

@victortoso
Copy link
Contributor Author

Thanks for the review!

I'd prefer the syntax like --usb vendorid=3d3,productid=5406. This is mostly because this makes the syntax more universal for different platforms, as eventually we may need to support different providers that are not qemu.

No problem, I'll change it.

Questions that I have:

  • Does podman need to handle device's permissions somehow or is that on user's side?
  • Applying USB host passthrough on podman machine init is enough for now? Just wondering if some sort of hotplug can/should be consider in the future too (e.g: podman machine init does not have USB but we could add at podman machine start without storing the devices to config...)

(Asking also to know podman machine scope!)

@victortoso victortoso force-pushed the usb-host-passthrough branch from fee662d to a71b0c6 Compare October 31, 2023 11:20
@ashley-cui
Copy link
Member

  • Does podman need to handle device's permissions somehow or is that on user's side?

I think we would prefer things to be as easy as possible for the user. I've been doing experiments with USB passthrough for the past week and discovered that even if the user changes the permissions of the usb device on /dev/bus/usb, on sleeps, the USB permissions revert. This might end up meaning that we need to let the user deal with perms, since we can't really constantly monitor the device.

  • Applying USB host passthrough on podman machine init is enough for now? Just wondering if some sort of hotplug can/should be consider in the future too (e.g: podman machine init does not have USB but we could add at podman machine start without storing the devices to config...)

I think we should definitely consider hotplugging, but I suppose this isn't an initial requirement. At the very least, I think --usb should also be plumbed through podman machine set. This should help out with broken USBs, as I mentioned above, USBs tend to disconnect on sleeps.

@victortoso
Copy link
Contributor Author

This might end up meaning that we need to let the user deal with perms, since we can't really constantly monitor the device.

One possibility is to interact with udev, but that's a linux only solution. I'm sure Window's would have its own APIs in a way that we can manage permissions as well. I don't see podman using udev as of now so we might want to do it as a follow up.

My question was also related to the expectations wrt permissions. We agree that the least the user has to do, the better.

I think we should definitely consider hotplugging, but I suppose this isn't an initial requirement.

Ok

At the very least, I think --usb should also be plumbed through podman machine set. This should help out with broken USBs, as I mentioned above, USBs tend to disconnect on sleeps.

Ok, I'll start working on it then.

@beriberikix
Copy link

RE: windows & hotplugging - I think that's a can of worms 🪱

A neat tool usbipd-win had to essentially implement a udev-like thing in Windows. Instead of the native WinUSB stack they opted to leverage the VirtualBox drivers. More discussion here.

@baude
Copy link
Member

baude commented Oct 31, 2023

if this PR is only for qemu, then we should try to fence off the option by provider or error out in every other provider.

@victortoso victortoso force-pushed the usb-host-passthrough branch 2 times, most recently from 98abb42 to cb80399 Compare October 31, 2023 14:29
@victortoso
Copy link
Contributor Author

if this PR is only for qemu, then we should try to fence off the option by provider or error out in every other provider.

Makes sense to me. Any suggestions on how to do that? I plan to use a conditional provider.VMType() == QemuVirt for exposing --usb option. Problem is that we might need some refactor as provider is not yet initialized on cmd/podman/machine/init.go). I'll tackle this later Today, but any other suggestions are welcome. Thanks for the review!

Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@ashley-cui
Copy link
Member

I think in the Init() functions in pkg/machine/<provider>, for non qemu providers, you can return an err if usb has been set.

@victortoso victortoso force-pushed the usb-host-passthrough branch from cb80399 to c0aaa0f Compare November 1, 2023 20:19
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 1, 2023
@github-actions github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Nov 1, 2023
@victortoso victortoso force-pushed the usb-host-passthrough branch from c0aaa0f to 1520609 Compare November 1, 2023 20:25
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 1, 2023
@github-actions github-actions bot removed the kind/api-change Change to remote API; merits scrutiny label Nov 1, 2023
@victortoso
Copy link
Contributor Author

  • Fixed the make validate (CI)
  • Error out on other hypervisors when using --usb (baude)
  • Add some documentation (Ashley)
  • Squashed the commits (Ashley)

I need some more time for the e2e test. I need to mock a USB or run the e2e test in VM, not sure what is easier.
I'll back on this only next Monday.

@victortoso victortoso changed the title wip: qemu: add usb host passthrough qemu: add usb host passthrough Nov 1, 2023
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 1, 2023
@beriberikix
Copy link

Emulating a USB on Linux is pretty trivial, depending on how much you need to emulate. In the past I've used USB/IP with a little hackery but quick bit of research led me to this neat tool:https://github.com/jtornosm/USBIP-Virtual-USB-Device (details here.)

@victortoso victortoso force-pushed the usb-host-passthrough branch from 1520609 to 78dc8cd Compare November 1, 2023 20:46
@victortoso
Copy link
Contributor Author

Emulating a USB on Linux is pretty trivial, depending on how much you need to emulate.

Nothing fancy, just to show in the dmesg of guest os.

In the past I've used USB/IP with a little hackery but quick bit of research led me to this neat tool:

I'm not familiar with podman's ci yet, we need to check if kernel has the right flags and it is fine to modprobe those drivers in the host.

In the KubeVirt feature I was working, we solved this with device emulation in QEMU as the CI was running in a VM, we just added an emulated storage to that VM (see kubevirt/kubevirtci#996)

Again, not 100% sure what the best course is for the tests. Mocking with umockdev might be best but I'm open to suggestions.

@victortoso victortoso force-pushed the usb-host-passthrough branch 3 times, most recently from af6f9cf to 99b56ce Compare November 1, 2023 21:38
Copy link
Contributor

openshift-ci bot commented Nov 8, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashley-cui, victortoso

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 8, 2023
pkg/machine/qemu/command.go Outdated Show resolved Hide resolved
@victortoso victortoso force-pushed the usb-host-passthrough branch from c7b20a3 to b3b8715 Compare November 8, 2023 21:11
@victortoso
Copy link
Contributor Author

hm, CI is failing but it seems to be on quay.io side.

@victortoso victortoso force-pushed the usb-host-passthrough branch from b3b8715 to a2fc3a3 Compare November 8, 2023 21:51
Copy link

Cockpit tests failed for commit a2fc3a3ef4da811a2a1af9b2e0ea69ee12ddea64. @martinpitt, @jelly, @mvollmer please check.

QEMU usb-host driver which is the one for passthrough, supports two
options for selecting an USB devices in the host to provide it to the
VM:
 - Bus and Device number the device is plugged
 - Vendor and Product information of the USB devices

    https://qemu-project.gitlab.io/qemu/system/devices/usb.html

This commit allows a user to configure podman machine with either of
options, with new --usb command line option for podman machine init.

Examples
  podman machine init tosovm4 --usb vendor=13d3,product=5406
  podman machine init tosovm3 --usb bus=1,devnum=4 --usb bus=1,devnum=3

This commit also allows a user to change the USBs configured with
--usb command line option for podman machine set.

Note that this commit does not handle host device permissions nor
verify that the USB devices exists.

Signed-off-by: Victor Toso <[email protected]>
@victortoso victortoso force-pushed the usb-host-passthrough branch from a2fc3a3 to c23963d Compare November 8, 2023 22:38
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@rhatdan
Copy link
Member

rhatdan commented Nov 13, 2023

/lgtm
Thanks @victortoso

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 13, 2023

vendor, err := strconv.ParseInt(vendorStr, 16, 0)
if err != nil {
return configs, fmt.Errorf("fail to convert vendor of %s: %s", str, err)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think usb: adds more context and consistent with other error message.

Suggested change
return configs, fmt.Errorf("fail to convert vendor of %s: %s", str, err)
return configs, fmt.Errorf("usb: fail to convert vendor of %s: %s", str, err)


product, err := strconv.ParseInt(productStr, 16, 0)
if err != nil {
return configs, fmt.Errorf("fail to convert product of %s: %s", str, err)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

Suggested change
return configs, fmt.Errorf("fail to convert product of %s: %s", str, err)
return configs, fmt.Errorf("usb: fail to convert product of %s: %s", str, err)

@openshift-merge-bot openshift-merge-bot bot merged commit 7dd33b3 into containers:main Nov 13, 2023
90 of 92 checks passed
Comment on lines +89 to +118
option := ""
if (left[0] == "bus" && right[0] == "devnum") ||
(right[0] == "bus" && left[0] == "devnum") {
option = "bus_devnum"
}
if (left[0] == "vendor" && right[0] == "product") ||
(right[0] == "vendor" && left[0] == "product") {
option = "vendor_product"
}

switch option {
case "bus_devnum":
bus, devnumber := left[1], right[1]
if right[0] == "bus" {
bus, devnumber = devnumber, bus
}

configs = append(configs, machine.USBConfig{
Bus: bus,
DevNumber: devnumber,
})
case "vendor_product":
vendorStr, productStr := left[1], right[1]
if right[0] == "vendor" {
vendorStr, productStr = productStr, vendorStr
}

vendor, err := strconv.ParseInt(vendorStr, 16, 0)
if err != nil {
return configs, fmt.Errorf("fail to convert vendor of %s: %s", str, err)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be simplified

option := left[0] +"_"+left[1] // and switch can be match against "vendor_product" and "product_vendor" both.

switch

@flouthoc
Copy link
Collaborator

Created a PR #20678

flouthoc added a commit to flouthoc/podman that referenced this pull request Nov 13, 2023
Some comments from containers#20540

[NO NEW TESTS NEEDED]

Signed-off-by: Aditya R <[email protected]>
@beriberikix
Copy link

Hi! Is there a specific reason why USB passthrough is not supported on Windows?
image

@rhatdan
Copy link
Member

rhatdan commented Dec 7, 2023

@n1hility WDYT?

@victortoso
Copy link
Contributor Author

Hi! Is there a specific reason why USB passthrough is not supported on Windows?

@beriberikix you should probably open a new issue for WSL

@beriberikix
Copy link

Does Podman on Windows support QEMU or only WSL? USB support on WSL is complicated , so targeting QEMU would be ideal!

@rhatdan
Copy link
Member

rhatdan commented Jan 4, 2024

We support hyperV and WSL no QEMU

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Apr 4, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants