Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190616134813.4094-1-ttayar@habana.ai>
In-Reply-To: <20190616134813.4094-1-ttayar@habana.ai>
From:   Oded Gabbay <oded.gabbay@gmail.com>
Date:   Mon, 17 Jun 2019 15:08:59 +0300
Message-ID: <CAFCwf11aXwM57rXbunROFzB-EVkCFH6St1=C13-x4N99+STXTw@mail.gmail.com>
Subject: Re: [PATCH] habanalabs: Allow accessing host mapped addresses via debugfs
To:     Tomer Tayar <ttayar@habana.ai>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, Jun 16, 2019 at 4:48 PM Tomer Tayar <ttayar@habana.ai> wrote:
>
> Allows using the addr/data32 debugfs nodes to access a device VA of a
> host mapped memory when the IOMMU is disabled.
>
> Due to the possible large amount of a user host mapped memory, the
> driver doesn't maintain a database with the host addresses per device VA.
> When the IOMMU is disabled, this missing info is being overcome by
> simply using phys_to_virt(). However, this is not useful when the IOMMU
> is enabled, and thus the enforced limitation.
>
> Signed-off-by: Tomer Tayar <ttayar@habana.ai>
> ---
>  .../ABI/testing/debugfs-driver-habanalabs     | 11 ++++--
>  drivers/misc/habanalabs/debugfs.c             | 35 ++++++++++++-------
>  drivers/misc/habanalabs/goya/goya.c           | 19 +++++++---
>  3 files changed, 46 insertions(+), 19 deletions(-)
>
> diff --git a/Documentation/ABI/testing/debugfs-driver-habanalabs b/Documentation/ABI/testing/debugfs-driver-habanalabs
> index 2f5b80be07a3..18191c2becab 100644
> --- a/Documentation/ABI/testing/debugfs-driver-habanalabs
> +++ b/Documentation/ABI/testing/debugfs-driver-habanalabs
> @@ -3,7 +3,10 @@ Date:           Jan 2019
>  KernelVersion:  5.1
>  Contact:        oded.gabbay@gmail.com
>  Description:    Sets the device address to be used for read or write through
> -                PCI bar. The acceptable value is a string that starts with "0x"
> +                PCI bar, or the device VA of a host mapped memory to be read or
> +                written directly from the host. The latter option is allowed
> +                only when the IOMMU is disabled.
> +                The acceptable value is a string that starts with "0x"
>
>  What:           /sys/kernel/debug/habanalabs/hl<n>/command_buffers
>  Date:           Jan 2019
> @@ -33,10 +36,12 @@ Contact:        oded.gabbay@gmail.com
>  Description:    Allows the root user to read or write directly through the
>                  device's PCI bar. Writing to this file generates a write
>                  transaction while reading from the file generates a read
> -                transcation. This custom interface is needed (instead of using
> +                transaction. This custom interface is needed (instead of using
>                  the generic Linux user-space PCI mapping) because the DDR bar
>                  is very small compared to the DDR memory and only the driver can
> -                move the bar before and after the transaction
> +                move the bar before and after the transaction.
> +                If the IOMMU is disabled, it also allows the root user to read
> +                or write from the host a device VA of a host mapped memory
>
>  What:           /sys/kernel/debug/habanalabs/hl<n>/device
>  Date:           Jan 2019
> diff --git a/drivers/misc/habanalabs/debugfs.c b/drivers/misc/habanalabs/debugfs.c
> index 886f8ea82499..17974919b760 100644
> --- a/drivers/misc/habanalabs/debugfs.c
> +++ b/drivers/misc/habanalabs/debugfs.c
> @@ -500,6 +500,25 @@ static ssize_t mmu_write(struct file *file, const char __user *buf,
>         return -EINVAL;
>  }
>
> +static bool hl_is_device_va(struct hl_device *hdev, u64 addr)
> +{
> +       struct asic_fixed_properties *prop = &hdev->asic_prop;
> +
> +       if (!hdev->mmu_enable)
> +               goto out;
> +
> +       if (hdev->dram_supports_virtual_memory &&
> +                       addr >= prop->va_space_dram_start_address &&
> +                       addr < prop->va_space_dram_end_address)
> +               return true;
> +
> +       if (addr >= prop->va_space_host_start_address &&
> +                       addr < prop->va_space_host_end_address)
> +               return true;
> +out:
> +       return false;
> +}
> +
>  static int device_va_to_pa(struct hl_device *hdev, u64 virt_addr,
>                                 u64 *phys_addr)
>  {
> @@ -573,7 +592,6 @@ static ssize_t hl_data_read32(struct file *f, char __user *buf,
>  {
>         struct hl_dbg_device_entry *entry = file_inode(f)->i_private;
>         struct hl_device *hdev = entry->hdev;
> -       struct asic_fixed_properties *prop = &hdev->asic_prop;
>         char tmp_buf[32];
>         u64 addr = entry->addr;
>         u32 val;
> @@ -582,11 +600,8 @@ static ssize_t hl_data_read32(struct file *f, char __user *buf,
>         if (*ppos)
>                 return 0;
>
> -       if (addr >= prop->va_space_dram_start_address &&
> -                       addr < prop->va_space_dram_end_address &&
> -                       hdev->mmu_enable &&
> -                       hdev->dram_supports_virtual_memory) {
> -               rc = device_va_to_pa(hdev, entry->addr, &addr);
> +       if (hl_is_device_va(hdev, addr)) {
> +               rc = device_va_to_pa(hdev, addr, &addr);
>                 if (rc)
>                         return rc;
>         }
> @@ -607,7 +622,6 @@ static ssize_t hl_data_write32(struct file *f, const char __user *buf,
>  {
>         struct hl_dbg_device_entry *entry = file_inode(f)->i_private;
>         struct hl_device *hdev = entry->hdev;
> -       struct asic_fixed_properties *prop = &hdev->asic_prop;
>         u64 addr = entry->addr;
>         u32 value;
>         ssize_t rc;
> @@ -616,11 +630,8 @@ static ssize_t hl_data_write32(struct file *f, const char __user *buf,
>         if (rc)
>                 return rc;
>
> -       if (addr >= prop->va_space_dram_start_address &&
> -                       addr < prop->va_space_dram_end_address &&
> -                       hdev->mmu_enable &&
> -                       hdev->dram_supports_virtual_memory) {
> -               rc = device_va_to_pa(hdev, entry->addr, &addr);
> +       if (hl_is_device_va(hdev, addr)) {
> +               rc = device_va_to_pa(hdev, addr, &addr);
>                 if (rc)
>                         return rc;
>         }
> diff --git a/drivers/misc/habanalabs/goya/goya.c b/drivers/misc/habanalabs/goya/goya.c
> index e8b3a31d211f..ce127a6f606f 100644
> --- a/drivers/misc/habanalabs/goya/goya.c
> +++ b/drivers/misc/habanalabs/goya/goya.c
> @@ -14,6 +14,7 @@
>  #include <linux/genalloc.h>
>  #include <linux/hwmon.h>
>  #include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/iommu.h>
>
>  /*
>   * GOYA security scheme:
> @@ -3941,10 +3942,11 @@ static void goya_clear_sm_regs(struct hl_device *hdev)
>  }
>
>  /*
> - * goya_debugfs_read32 - read a 32bit value from a given device address
> + * goya_debugfs_read32 - read a 32bit value from a given device or a host mapped
> + *                       address.
>   *
>   * @hdev:      pointer to hl_device structure
> - * @addr:      address in device
> + * @addr:      device or host mapped address
>   * @val:       returned value
>   *
>   * In case of DDR address that is not mapped into the default aperture that
> @@ -3985,6 +3987,10 @@ static int goya_debugfs_read32(struct hl_device *hdev, u64 addr, u32 *val)
>                 }
>                 if (ddr_bar_addr == U64_MAX)
>                         rc = -EIO;
> +
> +       } else if (addr >= HOST_PHYS_BASE && !iommu_present(&pci_bus_type)) {
> +               *val = *(u32 *) phys_to_virt(addr - HOST_PHYS_BASE);
> +
>         } else {
>                 rc = -EFAULT;
>         }
> @@ -3993,10 +3999,11 @@ static int goya_debugfs_read32(struct hl_device *hdev, u64 addr, u32 *val)
>  }
>
>  /*
> - * goya_debugfs_write32 - write a 32bit value to a given device address
> + * goya_debugfs_write32 - write a 32bit value to a given device or a host mapped
> + *                        address.
>   *
>   * @hdev:      pointer to hl_device structure
> - * @addr:      address in device
> + * @addr:      device or host mapped address
>   * @val:       returned value
>   *
>   * In case of DDR address that is not mapped into the default aperture that
> @@ -4037,6 +4044,10 @@ static int goya_debugfs_write32(struct hl_device *hdev, u64 addr, u32 val)
>                 }
>                 if (ddr_bar_addr == U64_MAX)
>                         rc = -EIO;
> +
> +       } else if (addr >= HOST_PHYS_BASE && !iommu_present(&pci_bus_type)) {
> +               *(u32 *) phys_to_virt(addr - HOST_PHYS_BASE) = val;
> +
>         } else {
>                 rc = -EFAULT;
>         }
> --
> 2.17.1
>

This patch is:
Reviewed-by: Oded Gabbay <oded.gabbay@gmail.com>