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 DD9A3A00BE; Tue, 15 Mar 2022 10:17:56 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D79F140395; Tue, 15 Mar 2022 10:17:55 +0100 (CET) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by mails.dpdk.org (Postfix) with ESMTP id 6464B4014F for ; Tue, 15 Mar 2022 10:17:54 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1647335874; x=1678871874; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=yFwKdwZ4pjgw+8OSsADpoi1ESsybgGsWrfqGrYqjJ6U=; b=Jx5DJ3j44W3M8Mb/qiQlbs/BUdf8Ne7wUcLq9iAGAbKFaCWdhMgIrj/O SiOP9ymazXtqv7wgjI7REsoxwnvcargw3v7ZITANw733DdVGJLC2ane2M wf+dK1+bcdIE1ahia+YxoiQ+v95hhe0nq1jL3yuMbeixSboE5ZQVL72Bk xszNiyJTBbmPyUQM6ZbV8e0ywX0ayCHVg+yRWUK6anOEtUxb5FzWlP9W6 j35wPj26DMqBeWkMxNPOx/yK0ARzWe1hLSt0w+a9fZn/AutlbWT37xIK3 r1ECEPC/IJ5G5bX/48uihuS88S4BMvVzt3XeOJlfMhWEltkeVnPnnZyKA w==; X-IronPort-AV: E=McAfee;i="6200,9189,10286"; a="256205455" X-IronPort-AV: E=Sophos;i="5.90,182,1643702400"; d="scan'208";a="256205455" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Mar 2022 02:17:53 -0700 X-IronPort-AV: E=Sophos;i="5.90,182,1643702400"; d="scan'208";a="556838146" Received: from bricha3-mobl.ger.corp.intel.com ([10.252.7.20]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 15 Mar 2022 02:17:51 -0700 Date: Tue, 15 Mar 2022 09:17:48 +0000 From: Bruce Richardson To: Fidaullah Noonari Cc: stephen@networkplumber.org, dev@dpdk.org Subject: Re: [PATCH v2] usertools: add check for IOMMU support in dpdk-devbind Message-ID: References: <20220315062652.78332-1-fidaullah.noonari@emumba.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220315062652.78332-1-fidaullah.noonari@emumba.com> 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 On Tue, Mar 15, 2022 at 11:26:52AM +0500, Fidaullah Noonari wrote: > binding with vfio driver, when IOMMU is disabled, causes program to crash. > this patch adds a flag for noiommmu-mode. when this is set, if IOMMU is > disabled, it changes vfio into unsafe noiommu mode and prints warning > message. > > Signed-off-by: Fidaullah Noonari > --- Hi, patch generally looks good, but some more feedback inline below. The main thing missing is that we should probably also check the current value of the vfio noiommu flag and take that into account when printing warnings or errors. /Bruce > usertools/dpdk-devbind.py | 39 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/usertools/dpdk-devbind.py b/usertools/dpdk-devbind.py > index ace4627218..91fa4c7620 100755 > --- a/usertools/dpdk-devbind.py > +++ b/usertools/dpdk-devbind.py > @@ -96,6 +96,7 @@ > b_flag = None > status_flag = False > force_flag = False > +noiommu_flag = False > args = [] > > > @@ -466,9 +467,30 @@ def unbind_all(dev_list, force=False): > unbind_one(d, force) > > > +def check_iommu(): > + """Check if IOMMU is enabled on system""" > + if len(os.listdir("/sys/class/iommu")) != 0: > + return True > + return False I'd suggest renaming the function for clarity, since "check_iommu" is a little unclear as to what the return value is i.e. false could mean you can't check, rather than it doesn't have one. "has_iommu()" might be clearer. Can probably be shortened to one-line body: def has_iommu(): """...""" return len(os.listdir("/sys/class/iommu")) > 0 > + > + > +def enable_noiommu_mode(): > + """Enables the noiommu mode for vfio drivers""" > + filename = "/sys/module/vfio/parameters/enable_unsafe_noiommu_mode" > + try: > + f = open(filename, "w") > + except OSError as err: > + sys.exit("Error: failed to enable unsafe noiommu mode - Cannot open %s: %s" > + % (filename, err)) > + f.write("1") > + f.close() > + print("Warning: enabling unsafe no IOMMU mode for vfio drivers") > + > + > def bind_all(dev_list, driver, force=False): > """Bind method, takes a list of device locations""" > global devices > + global noiommu_flag > > # a common user error is to forget to specify the driver the devices need to > # be bound to. check if the driver is a valid device, and if it is, show > @@ -492,6 +514,14 @@ def bind_all(dev_list, driver, force=False): > except ValueError as ex: > sys.exit(ex) > > + # check for IOMMU support > + if not check_iommu(): > + if noiommu_flag: > + enable_noiommu_mode() > + elif driver == "vfio-pci": > + print("Warning: IOMMU support is disabled") > + print("Info: use --noiommu-mode for binding in noiommu mode") > + Rather than just warning here, I think this needs a little extra check to see if no-iommu mode is already enabled. If so, just continue, if it is not, then the warning should be an error and no binding should be attempted. > for d in dev_list: > bind_one(d, driver, force) > > @@ -634,6 +664,7 @@ def parse_args(): > global status_flag > global status_dev > global force_flag > + global noiommu_flag > global args > > parser = argparse.ArgumentParser( > @@ -687,6 +718,12 @@ def parse_args(): > Override restriction on binding devices in use by Linux" > WARNING: This can lead to loss of network connection and should be used with caution. > """) > + parser.add_argument( > + '--noiommu-mode', > + action='store_true', > + help=""" > +if IOMMU is not available, Enables no IOMMU mode for vfio drivers. > + """) > parser.add_argument( > 'devices', > metavar='DEVICE', > @@ -706,6 +743,8 @@ def parse_args(): > status_dev = "all" > if opt.force: > force_flag = True > + if opt.noiommu_mode: > + noiommu_flag = True > if opt.bind: > b_flag = opt.bind > elif opt.unbind: > -- > 2.25.1 >