DPDK patches and discussions
 help / color / mirror / Atom feed
From: Bruce Richardson <bruce.richardson@intel.com>
To: Fidaullah Noonari <fidaullah.noonari@emumba.com>
Cc: stephen@networkplumber.org, dev@dpdk.org
Subject: Re: [PATCH v2] usertools: add check for IOMMU support in dpdk-devbind
Date: Tue, 15 Mar 2022 09:17:48 +0000	[thread overview]
Message-ID: <YjBZvI4LKJhLB1GS@bricha3-MOBL.ger.corp.intel.com> (raw)
In-Reply-To: <20220315062652.78332-1-fidaullah.noonari@emumba.com>

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 <fidaullah.noonari@emumba.com>
> ---

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
> 

  reply	other threads:[~2022-03-15  9:17 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-15  6:26 Fidaullah Noonari
2022-03-15  9:17 ` Bruce Richardson [this message]
2022-03-21 12:27 ` [PATCH v3] " Fidaullah Noonari
2022-03-31 14:37   ` Burakov, Anatoly
2022-03-31 14:44     ` Bruce Richardson
2022-10-10 23:02       ` Thomas Monjalon
2022-10-12 12:38   ` [PATCH v4] " Fidaullah Noonari
2024-07-01 17:51     ` Stephen Hemminger
2024-10-02 18:13     ` [PATCH] usertools/devbind: add support for non-IOMMU mode Stephen Hemminger
2024-10-03  9:27       ` Bruce Richardson
2024-10-17 15:34         ` Thomas Monjalon
2023-07-06 18:22   ` [PATCH v3] usertools: add check for IOMMU support in dpdk-devbind Stephen Hemminger
2023-10-31 18:34   ` Stephen Hemminger

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YjBZvI4LKJhLB1GS@bricha3-MOBL.ger.corp.intel.com \
    --to=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=fidaullah.noonari@emumba.com \
    --cc=stephen@networkplumber.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).