Hi Anatoly,
Anatoly Burakov, Nov 26, 2024 at 16:02:
Currently, when binding a device to VFIO, the UID/GID for the device will
always stay as system default (`root`). Yet, when running DPDK as non-root
user, one has to change the UID/GID of the device to match the user's
UID/GID to use the device.
This patch adds an option to `dpdk-devbind.py` to change the UID/GID of
the device when binding it to VFIO.
Signed-off-by: Anatoly Burakov <anatoly.bura...@intel.com>
---
Notes:
v1 -> v2:
- Replaced hard exit with an error printout
Sorry I had missed that particular detail.
I don't think this should only print a warning. Otherwise, the user has
no way to detect if the operation failed.
usertools/dpdk-devbind.py | 41 +++++++++++++++++++++++++++++++++++++--
1 file changed, 39 insertions(+), 2 deletions(-)
diff --git a/usertools/dpdk-devbind.py b/usertools/dpdk-devbind.py
index f2a2a9a12f..496d0e90e8 100755
--- a/usertools/dpdk-devbind.py
+++ b/usertools/dpdk-devbind.py
@@ -8,6 +8,8 @@
import subprocess
import argparse
import platform
+import grp
+import pwd
We may already be past this but could you try to sort these imports
alphabetically?
from glob import glob
from os.path import exists, basename
@@ -108,6 +110,8 @@
status_flag = False
force_flag = False
noiommu_flag = False
+vfio_uid = ""
+vfio_gid = ""
These are supposed to be integers. Initialize them to -1.
args = []
@@ -463,6 +467,22 @@ def bind_one(dev_id, driver, force):
% (dev_id, filename, err))
+def own_one(dev_id, uid, gid):
+ """Set the IOMMU group ownership for a device"""
+ # find IOMMU group for a particular device
+ iommu_grp_base_path = os.path.join("/sys/bus/pci/devices", dev_id,
"iommu_group")
+ try:
+ iommu_grp = os.path.basename(os.readlink(iommu_grp_base_path))
+ # we found IOMMU group, now find the device
+ dev_path = os.path.join("/dev/vfio", iommu_grp)
+ # set the ownership
+ _uid = pwd.getpwnam(uid).pw_uid if uid else -1
+ _gid = grp.getgrnam(gid).gr_gid if gid else -1
The validity of these values should be checked when parsing command line
arguments.
+ os.chown(dev_path, _uid, _gid)
+ except OSError as err:
+ print(f"Error: failed to read IOMMU group for {dev_id}: {err}")
Remove the try/except block and let the error bubble up the stack. This
probably does not require a dedicated function. Moreover, the name
own_one() is ambiguous.
+
+
def unbind_all(dev_list, force=False):
"""Unbind method, takes a list of device locations"""
@@ -512,7 +532,7 @@ def check_noiommu_mode():
print("Warning: enabling unsafe no IOMMU mode for VFIO drivers")
-def bind_all(dev_list, driver, force=False):
+def bind_all(dev_list, driver, uid, gid, force=False):
Not required. These are global variables.
"""Bind method, takes a list of device locations"""
global devices
@@ -544,6 +564,9 @@ def bind_all(dev_list, driver, force=False):
for d in dev_list:
bind_one(d, driver, force)
+ # if we're binding to vfio-pci, set the IOMMU user/group ownership if
one was specified
+ if driver == "vfio-pci" and (uid or gid):
if driver == "vfio-pci" and (vfio_uid != -1 or vfio_gid != -1):
+ own_one(d, uid, gid)
It will be better to store the chmod code path here:
iommu_grp = os.path.join("/sys/bus/pci/devices", d, "iommu_group")
iommu_grp = os.path.basename(os.readlink(iommu_grp))
os.chown(os.path.join("/dev/vfio", iommu_grp), vfio_uid, vfio_gid)
# For kernels < 3.15 when binding devices to a generic driver
# (i.e. one that doesn't have a PCI ID table) using new_id, some devices
@@ -697,6 +720,8 @@ def parse_args():
global force_flag
global noiommu_flag
global args
+ global vfio_uid
+ global vfio_gid
parser = argparse.ArgumentParser(
description='Utility to bind and unbind devices from Linux kernel',
@@ -746,6 +771,12 @@ def parse_args():
'--noiommu-mode',
action='store_true',
help="If IOMMU is not available, enable no IOMMU mode for VFIO
drivers")
+ parser.add_argument(
+ "-U", "--uid", help="For VFIO, specify the UID to set IOMMU group
ownership"
In order to fail early if an invalid user name is passed, add these two
lines:
type=lambda u: pwd.getpwnam(u).pw_uid,
default=-1,
+ )
+ parser.add_argument(
+ "-G", "--gid", help="For VFIO, specify the GID to set IOMMU group
ownership"
In order to fail early if an invalid group name is passed, add these two
lines:
type=lambda g: grp.getgrnam(g).gr_gid,
default=-1,
+ )
parser.add_argument(
'--force',
action='store_true',
@@ -778,6 +809,10 @@ def parse_args():
b_flag = opt.bind
elif opt.unbind:
b_flag = "none"
+ if opt.uid:
+ vfio_uid = opt.uid
+ if opt.gid:
+ vfio_gid = opt.gid
No need for ifs here, the default values are set to -1 which means: "use
default" for os.chmod().
vfio_uid = opt.uid
vfio_gid = opt.gid
args = opt.devices
if not b_flag and not status_flag:
@@ -805,11 +840,13 @@ def do_arg_actions():
global status_flag
global force_flag
global args
+ global vfio_uid
+ global vfio_gid
The global keyword is not required here.
if b_flag in ["none", "None"]:
unbind_all(args, force_flag)
elif b_flag is not None:
- bind_all(args, b_flag, force_flag)
+ bind_all(args, b_flag, vfio_uid, vfio_gid, force_flag)
Not required. These are global variables.
if status_flag:
if b_flag is not None:
clear_data()
--
2.43.5
Thanks.
