From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id C6C6B45DAB; Tue, 26 Nov 2024 17:15:48 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A4E7640268; Tue, 26 Nov 2024 17:15:47 +0100 (CET) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mails.dpdk.org (Postfix) with ESMTP id DE8D44025F for ; Tue, 26 Nov 2024 17:15:45 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1732637745; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=7iUXM/BpewSsayoybdAFftLkg/IqYzR3nRLjGd7SphY=; b=GbAmoY3UNdg9eogANKRWqZqOQZNrzVutDKw5PgBwR15du/2jX39oB9qHpl3Tu/qmsQMUs3 9CH4HYESHe0gIP/s9YouNqOFuF+yz+n2sZcsIfooYjzCs0hGAEdl85NyYZdkH5VAFgdSQr N/pNcSHRejL/ez/4Amqw7dhgWYhX75Q= Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-32-r5TEbfTKMpGNzabhwH_H2g-1; Tue, 26 Nov 2024 11:15:44 -0500 X-MC-Unique: r5TEbfTKMpGNzabhwH_H2g-1 X-Mimecast-MFC-AGG-ID: r5TEbfTKMpGNzabhwH_H2g Received: by mail-wr1-f69.google.com with SMTP id ffacd0b85a97d-38240c98b68so2971950f8f.2 for ; Tue, 26 Nov 2024 08:15:44 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1732637743; x=1733242543; h=in-reply-to:references:user-agent:to:from:subject:message-id:date :content-transfer-encoding:mime-version:x-gm-message-state:from:to :cc:subject:date:message-id:reply-to; bh=7iUXM/BpewSsayoybdAFftLkg/IqYzR3nRLjGd7SphY=; b=U4i8xiby1eobJYFo6SHrqFwqEVKQQdQPsiR5E13U/kHoVQxjs49IWZ6GLSmEfSyaTd N8pSCRkvE1SR9Hrl2mKB8BmCiQsVHkx6Mu+5rhtx9NOuzPJMs/gID4Krla0w95PYrK6O CHLI/tmbF6bJTN6aQ9+HtvjqcINzhYwYPgiOqU2/gNGqnLzd9EpMQso7viuFcE9erM9X JVy+6xu0cckmcapiW/Kp0DrLYyBVKZU+tQRH6jLsKnjQ5fk4QZd+flhNo6CNmjMNhe50 I0e8Z3fpLwWSuCQpjfs4XjFdoQEIAMpY3pi15gYQMNeMm71rwWteI175GHWlVLDUYBrP 5k1g== X-Forwarded-Encrypted: i=1; AJvYcCWHXXAZ2NxI/xhTL07cvB2/d8BTccQO0yxZOiB3TEi+Bd8nd+ciQva9aarPvfdt6USCeAo=@dpdk.org X-Gm-Message-State: AOJu0Yy4dtPrwNU1gecqzD6pCnOpZuWax248c79wwNnGfyGi2bTeh0xH OH8p+GEyzkkJd0y74p4/2G27FWndIQT9GMrE7Qwt6942awXZ5jr2ECEVSHSLyt7MGauXAN1J3Po 3bH93EDdMo79YFNYjKKoXNlQELuzbA01CPhTBOz/F X-Gm-Gg: ASbGncuyhCRkcotszeSIThGQ6bwoUMYflMGvhba+VDqDdxG2/lbR0W6Sr/AQe0uVH+D oNzJzgCsw9s0hxYnD12zPx/u+Yt2G3NyZn3w/ysNT5XsyWs8HAFt+J9YHMiq1lxyyPH0vbWeQvV 7ar4Mf9qMuyPuQcQKpWZZqUFvWMhzmG7CKlKhX2usjWinE/xLpHHoeqIvhqtCBjHYVCwUPX5aDo 5YUXZCr3Rav7dftyYrAAQw0EdM4QVAn1jnup7CN+AZmkl4TEP05rQvj7XYC8kRY58NtQUsII72p x0QeTvjW/qVyyF0SZSzoymvHFOG0eBWo X-Received: by 2002:a5d:5f8f:0:b0:382:3754:38fa with SMTP id ffacd0b85a97d-38260be1c77mr15134675f8f.51.1732637742915; Tue, 26 Nov 2024 08:15:42 -0800 (PST) X-Google-Smtp-Source: AGHT+IFedIoTNrbbBog7rgGchlxshvy2llMbvUybT3vPBBbrcdhuHa4UHKPjVAZ9VF7qWdlNfMNm0w== X-Received: by 2002:a5d:5f8f:0:b0:382:3754:38fa with SMTP id ffacd0b85a97d-38260be1c77mr15134526f8f.51.1732637741015; Tue, 26 Nov 2024 08:15:41 -0800 (PST) Received: from localhost (2a01cb00025433006239e1f47a0b2371.ipv6.abo.wanadoo.fr. [2a01:cb00:254:3300:6239:e1f4:7a0b:2371]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-3825fbed9a8sm13639893f8f.92.2024.11.26.08.15.40 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 26 Nov 2024 08:15:40 -0800 (PST) Mime-Version: 1.0 Date: Tue, 26 Nov 2024 17:15:40 +0100 Message-Id: Subject: Re: [PATCH v2 1/1] usertools/devbind: allow changing UID/GID for VFIO From: "Robin Jarry" To: "Anatoly Burakov" , User-Agent: aerc/0.18.2-104-gae6ebb03d2df References: <4cd0282dabfa59e715028ecf255468529655b487.1725285449.git.anatoly.burakov@intel.com> <3dba72cacdb5bc71e743dd84cd44e5dcde77aee7.1732633351.git.anatoly.burakov@intel.com> In-Reply-To: <3dba72cacdb5bc71e743dd84cd44e5dcde77aee7.1732633351.git.anatoly.burakov@intel.com> X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: VGen0YH8DlsNpgcVvaEFb764HodKsjyN7RYFQc1V3fc_1732637743 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8; format=Flowed X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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-roo= t > 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 > --- > > 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=20 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=20 alphabetically? > =20 > from glob import glob > from os.path import exists, basename > @@ -108,6 +110,8 @@ > status_flag =3D False > force_flag =3D False > noiommu_flag =3D False > +vfio_uid =3D "" > +vfio_gid =3D "" These are supposed to be integers. Initialize them to -1. > args =3D [] > =20 > =20 > @@ -463,6 +467,22 @@ def bind_one(dev_id, driver, force): > % (dev_id, filename, err)) > =20 > =20 > +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 =3D os.path.join("/sys/bus/pci/devices", dev_id,= "iommu_group") > + try: > + iommu_grp =3D os.path.basename(os.readlink(iommu_grp_base_path)) > + # we found IOMMU group, now find the device > + dev_path =3D os.path.join("/dev/vfio", iommu_grp) > + # set the ownership > + _uid =3D pwd.getpwnam(uid).pw_uid if uid else -1 > + _gid =3D grp.getgrnam(gid).gr_gid if gid else -1 The validity of these values should be checked when parsing command line=20 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=20 probably does not require a dedicated function. Moreover, the name=20 own_one() is ambiguous. > + > + > def unbind_all(dev_list, force=3DFalse): > """Unbind method, takes a list of device locations""" > =20 > @@ -512,7 +532,7 @@ def check_noiommu_mode(): > print("Warning: enabling unsafe no IOMMU mode for VFIO drivers") > =20 > =20 > -def bind_all(dev_list, driver, force=3DFalse): > +def bind_all(dev_list, driver, uid, gid, force=3DFalse): Not required. These are global variables. > """Bind method, takes a list of device locations""" > global devices > =20 > @@ -544,6 +564,9 @@ def bind_all(dev_list, driver, force=3DFalse): > =20 > for d in dev_list: > bind_one(d, driver, force) > + # if we're binding to vfio-pci, set the IOMMU user/group ownersh= ip if one was specified > + if driver =3D=3D "vfio-pci" and (uid or gid): if driver =3D=3D "vfio-pci" and (vfio_uid !=3D -1 or vfio_gid != =3D -1): > + own_one(d, uid, gid) It will be better to store the chmod code path here: iommu_grp =3D os.path.join("/sys/bus/pci/devices", d, "iommu= _group") iommu_grp =3D os.path.basename(os.readlink(iommu_grp)) os.chown(os.path.join("/dev/vfio", iommu_grp), vfio_uid, vfi= o_gid) > =20 > # 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 dev= ices > @@ -697,6 +720,8 @@ def parse_args(): > global force_flag > global noiommu_flag > global args > + global vfio_uid > + global vfio_gid > =20 > parser =3D argparse.ArgumentParser( > description=3D'Utility to bind and unbind devices from Linux ker= nel', > @@ -746,6 +771,12 @@ def parse_args(): > '--noiommu-mode', > action=3D'store_true', > help=3D"If IOMMU is not available, enable no IOMMU mode for VFIO= drivers") > + parser.add_argument( > + "-U", "--uid", help=3D"For VFIO, specify the UID to set IOMMU gr= oup ownership" In order to fail early if an invalid user name is passed, add these two=20 lines: type=3Dlambda u: pwd.getpwnam(u).pw_uid, default=3D-1, > + ) > + parser.add_argument( > + "-G", "--gid", help=3D"For VFIO, specify the GID to set IOMMU gr= oup ownership" In order to fail early if an invalid group name is passed, add these two=20 lines: type=3Dlambda g: grp.getgrnam(g).gr_gid, default=3D-1, > + ) > parser.add_argument( > '--force', > action=3D'store_true', > @@ -778,6 +809,10 @@ def parse_args(): > b_flag =3D opt.bind > elif opt.unbind: > b_flag =3D "none" > + if opt.uid: > + vfio_uid =3D opt.uid > + if opt.gid: > + vfio_gid =3D opt.gid No need for ifs here, the default values are set to -1 which means: "use=20 default" for os.chmod(). vfio_uid =3D opt.uid vfio_gid =3D opt.gid > args =3D opt.devices > =20 > 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. > =20 > 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() > --=20 > 2.43.5 Thanks.