Re: [PATCH v2] usertools: add check for IOMMU support in dpdk-devbind

[Bruce Richardson <bruce.richardson@intel.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
>