Message-Id: <20190127161256.GA985@rapoport-lnx>
Date:   Sun, 27 Jan 2019 18:13:00 +0200
From:   Mike Rapoport <rppt@...ux.ibm.com>
To:     Oded Gabbay <oded.gabbay@...il.com>
Cc:     gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
        ogabbay@...ana.ai, Omer Shpigelman <oshpigelman@...ana.ai>
Subject: Re: [PATCH 12/15] habanalabs: add virtual memory and MMU modules

On Wed, Jan 23, 2019 at 02:00:54AM +0200, Oded Gabbay wrote:
> From: Omer Shpigelman <oshpigelman@...ana.ai>
> 
> This patch adds the Virtual Memory and MMU modules.
> 
> Goya has an internal MMU which provides process isolation on the internal
> DDR. The internal MMU also performs translations for transactions that go
> from Goya to the Host.
> 
> The driver is responsible for allocating and freeing memory on the DDR
> upon user request. It also provides an interface to map and unmap DDR and
> Host memory to the device address space.
> 
> Signed-off-by: Omer Shpigelman <oshpigelman@...ana.ai>
> Signed-off-by: Oded Gabbay <oded.gabbay@...il.com>
> ---
>  drivers/misc/habanalabs/Makefile              |    2 +-
>  drivers/misc/habanalabs/context.c             |   19 +-
>  drivers/misc/habanalabs/device.c              |   20 +-
>  drivers/misc/habanalabs/goya/goya.c           |  391 +++++
>  drivers/misc/habanalabs/habanalabs.h          |  195 +++
>  drivers/misc/habanalabs/habanalabs_drv.c      |    2 +-
>  drivers/misc/habanalabs/habanalabs_ioctl.c    |    3 +-
>  drivers/misc/habanalabs/include/goya/goya.h   |    6 +-
>  .../include/hw_ip/mmu/mmu_general.h           |   45 +
>  .../habanalabs/include/hw_ip/mmu/mmu_v1_0.h   |   15 +
>  drivers/misc/habanalabs/memory.c              | 1506 +++++++++++++++++
>  drivers/misc/habanalabs/mmu.c                 |  604 +++++++
>  include/uapi/misc/habanalabs.h                |  122 +-
>  13 files changed, 2922 insertions(+), 8 deletions(-)
>  create mode 100644 drivers/misc/habanalabs/include/hw_ip/mmu/mmu_general.h
>  create mode 100644 drivers/misc/habanalabs/include/hw_ip/mmu/mmu_v1_0.h
>  create mode 100644 drivers/misc/habanalabs/mmu.c
 
[ ... ]

> diff --git a/drivers/misc/habanalabs/goya/goya.c b/drivers/misc/habanalabs/goya/goya.c
> index e3867615b974..94ee4cb00a49 100644
> --- a/drivers/misc/habanalabs/goya/goya.c
> +++ b/drivers/misc/habanalabs/goya/goya.c

[ ... ]

> @@ -265,6 +332,10 @@ static u32 goya_non_fatal_events[GOYA_ASYC_EVENT_GROUP_NON_FATAL_SIZE] = {
>  };
>  
>  static int goya_armcp_info_get(struct hl_device *hdev);
> +static void goya_mmu_prepare(struct hl_device *hdev, u32 asid);
> +static int goya_mmu_clear_pgt_range(struct hl_device *hdev);
> +static int goya_mmu_update_asid_hop0_addr(struct hl_device *hdev, u32 asid,
> +					u64 phys_addr);

Nit: are the static declarations are necessary? Or it's a matter of moving
code around?

>  
>  static void goya_get_fixed_properties(struct hl_device *hdev)
>  {
> @@ -303,6 +374,16 @@ static void goya_get_fixed_properties(struct hl_device *hdev)
>  	prop->sram_user_base_address = prop->sram_base_address +
>  						SRAM_USER_BASE_OFFSET;
>  
> +	prop->mmu_pgt_addr = MMU_PAGE_TABLES_ADDR;
> +	if (hdev->pldm)
> +		prop->mmu_pgt_size = 0x800000; /* 8MB */
> +	else
> +		prop->mmu_pgt_size = MMU_PAGE_TABLES_SIZE;
> +	prop->mmu_pte_size = PTE_SIZE;
> +	prop->mmu_hop_table_size = HOP_TABLE_SIZE;
> +	prop->mmu_hop0_tables_total_size = HOP0_TABLES_TOTAL_SIZE;
> +	prop->dram_page_size = PAGE_SIZE_2MB;
> +
>  	prop->host_phys_base_address = HOST_PHYS_BASE;
>  	prop->va_space_host_start_address = VA_HOST_SPACE_START;
>  	prop->va_space_host_end_address = VA_HOST_SPACE_END;

[ ... ]

> diff --git a/drivers/misc/habanalabs/include/hw_ip/mmu/mmu_general.h b/drivers/misc/habanalabs/include/hw_ip/mmu/mmu_general.h
> new file mode 100644
> index 000000000000..8d61ee4f2d17
> --- /dev/null
> +++ b/drivers/misc/habanalabs/include/hw_ip/mmu/mmu_general.h
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + *
> + * Copyright 2016-2018 HabanaLabs, Ltd.
> + * All Rights Reserved.
> + *
> + */
> +
> +#ifndef INCLUDE_MMU_GENERAL_H_
> +#define INCLUDE_MMU_GENERAL_H_
> +
> +#define PAGE_SHIFT_4KB			12
> +#define PAGE_SHIFT_2MB			21
> +#define PAGE_SIZE_2MB			(_AC(1, UL) << PAGE_SHIFT_2MB)
> +#define PAGE_SIZE_4KB			(_AC(1, UL) << PAGE_SHIFT_4KB)
> +
> +#define PAGE_PRESENT_MASK		0x0000000000001
> +#define SWAP_OUT_MASK			0x0000000000004
> +#define LAST_MASK			0x0000000000800
> +#define PHYS_ADDR_MASK			0x3FFFFFFFFF000ull
> +#define HOP0_MASK			0x3000000000000ull
> +#define HOP1_MASK			0x0FF8000000000ull
> +#define HOP2_MASK			0x0007FC0000000ull
> +#define HOP3_MASK			0x000003FE00000
> +#define HOP4_MASK			0x00000001FF000
> +#define OFFSET_MASK			0x0000000000FFF
> +
> +#define HOP0_SHIFT			48
> +#define HOP1_SHIFT			39
> +#define HOP2_SHIFT			30
> +#define HOP3_SHIFT			21
> +#define HOP4_SHIFT			12
> +
> +#define PTE_PHYS_ADDR_SHIFT		12
> +#define PTE_PHYS_ADDR_MASK		~0xFFF
> +
> +#define PTE_SIZE			sizeof(u64)

I suspect some architectures define PTE_SIZE in arch/*/include/asm
Probably you'd want to namespace this.

> +#define HOP_TABLE_SIZE			PAGE_SIZE_4KB
> +#define HOP0_TABLES_TOTAL_SIZE		(HOP_TABLE_SIZE * MAX_ASID)
> +
> +#define MMU_HOP0_PA43_12_SHIFT		12
> +#define MMU_HOP0_PA49_44_SHIFT		(12 + 32)
> +
> +#define MMU_CONFIG_TIMEOUT_USEC		2000 /* 2 ms */
> +
> +#endif /* INCLUDE_MMU_GENERAL_H_ */
> diff --git a/drivers/misc/habanalabs/memory.c b/drivers/misc/habanalabs/memory.c
> index 94cbb252656d..c41ea19502e5 100644
> --- a/drivers/misc/habanalabs/memory.c
> +++ b/drivers/misc/habanalabs/memory.c
> @@ -5,12 +5,1193 @@
>   * All Rights Reserved.
>   */
>  
> +#include <uapi/misc/habanalabs.h>
>  #include "habanalabs.h"
> +#include "include/hw_ip/mmu/mmu_general.h"
>  
>  #include <linux/sched.h>
>  #include <linux/uaccess.h>
>  #include <linux/genalloc.h>
>  
> +#define HL_MMU_DEBUG	0
> +
> +/*
> + * The va ranges in context object contain a list with the available chunks of
> + * device virtual memory.
> + * There is one range for host allocations and one for DRAM allocations.
> + *
> + * On initialization each range contains one chunk of all of its available
> + * virtual range which is a half of the total device virtual range.
> + *
> + * On each mapping of physical pages, a suitable virtual range chunk (with a
> + * minimum size) is selected from the list. If the chunk size equals the
> + * requested size, the chunk is returned. Otherwise, the chunk is split into
> + * two chunks - one to return as result and a remainder to stay in the list.
> + *
> + * On each Unmapping of a virtual address, the relevant virtual chunk is
> + * returned to the list. The chunk is added to the list and if its edges match
> + * the edges of the adjacent chunks (means a contiguous chunk can be created),
> + * the chunks are merged.
> + *
> + * On finish, the list is checked to have only one chunk of all the relevant
> + * virtual range (which is a half of the device total virtual range).
> + * If not (means not all mappings were unmapped), a warning is printed.
> + */
> +
> +/**
> + * alloc_device_memory - allocate device memory
> + *
> + * @ctx                 : current context
> + * @args                : host parameters containing the requested size
> + * @ret_handle          : result handle
> + *
> + * This function does the following:
> + * - Allocate the requested size rounded up to 2MB pages
> + * - Return unique handle
> + */
> +static int alloc_device_memory(struct hl_ctx *ctx, struct hl_mem_in *args,
> +				u32 *ret_handle)
> +{
> +	struct hl_device *hdev = ctx->hdev;
> +	struct hl_vm *vm = &hdev->vm;
> +	struct hl_vm_phys_pg_list *phys_pg_list;
> +	struct hl_vm_phys_pg *phys_pg, *tmp;
> +	u64 paddr = 0;
> +	u32 total_size, num_pgs, page_size, page_shift;
> +	int handle, rc, i;
> +	bool contiguous;
> +
> +	page_size = hdev->asic_prop.dram_page_size;
> +	page_shift = __ffs(page_size);

Maybe it's worth storing page_shift in the asi_prop and calculating
page_size.

> +	num_pgs = (args->alloc.mem_size + (page_size - 1)) >> page_shift;
> +	total_size = num_pgs << page_shift;
> +
> +	contiguous = args->flags & HL_MEM_CONTIGUOUS;
> +
> +	if (contiguous) {
> +		paddr = (u64) gen_pool_alloc(vm->dram_pg_pool, total_size);
> +		if (!paddr) {
> +			dev_err(hdev->dev,
> +				"failed to allocate %u huge contiguous pages\n",
> +				num_pgs);
> +			return -ENOMEM;
> +		}
> +	}
> +
> +	phys_pg_list = kzalloc(sizeof(*phys_pg_list), GFP_KERNEL);
> +	if (!phys_pg_list) {
> +		rc = -ENOMEM;
> +		goto page_list_err;
> +	}
> +
> +	phys_pg_list->vm_type = VM_TYPE_PHYS_LIST;
> +	phys_pg_list->asid = ctx->asid;
> +	phys_pg_list->total_size = total_size;
> +	phys_pg_list->flags = args->flags;
> +	phys_pg_list->contiguous = contiguous;
> +	INIT_LIST_HEAD(&phys_pg_list->list);
> +
> +	for (i = 0 ; i < num_pgs ; i++) {
> +		phys_pg = kzalloc(sizeof(*phys_pg), GFP_KERNEL);

Consider adding *phys_pgs to phys_pg_list using kcalloc() before the loop.

> +		if (!phys_pg) {
> +			rc = -ENOMEM;
> +			goto pb_err;
> +		}
> +
> +		phys_pg->page_size = page_size;
> +
> +		if (phys_pg_list->contiguous) {
> +			phys_pg->paddr = paddr + i * phys_pg->page_size;
> +		} else {
> +			phys_pg->paddr =
> +				(u64) gen_pool_alloc(vm->dram_pg_pool,
> +							phys_pg->page_size);
> +			if (!phys_pg->paddr) {
> +				dev_err(hdev->dev, "ioctl failed to allocate page\n");
> +				kfree(phys_pg);
> +				rc = -ENOMEM;
> +				goto pb_err;
> +			}
> +		}
> +
> +		list_add_tail(&phys_pg->node, &phys_pg_list->list);
> +	}
> +
> +	spin_lock(&vm->idr_lock);
> +	handle = idr_alloc(&vm->phys_pg_list_handles, phys_pg_list, 1, 0,
> +				GFP_ATOMIC);
> +	spin_unlock(&vm->idr_lock);
> +
> +	if (handle < 0) {
> +		dev_err(hdev->dev, "Failed to get handle for page\n");
> +		rc = -EFAULT;
> +		goto idr_err;
> +	}
> +
> +	for (i = 0; i < num_pgs ; i++)
> +		kref_get(&vm->dram_pg_pool_refcount);
> +
> +	phys_pg_list->handle = handle;
> +
> +	atomic64_add(phys_pg_list->total_size, &ctx->dram_phys_mem);
> +	atomic64_add(phys_pg_list->total_size, &hdev->dram_used_mem);
> +
> +	*ret_handle = handle;
> +
> +	return 0;
> +
> +idr_err:
> +pb_err:
> +	list_for_each_entry_safe(phys_pg, tmp, &phys_pg_list->list, node) {
> +		if (!phys_pg_list->contiguous)
> +			gen_pool_free(vm->dram_pg_pool, phys_pg->paddr,
> +					phys_pg->page_size);
> +
> +		list_del(&phys_pg->node);
> +		kfree(phys_pg);
> +	}
> +
> +	kfree(phys_pg_list);
> +page_list_err:
> +	if (contiguous)
> +		gen_pool_free(vm->dram_pg_pool, paddr, total_size);
> +
> +	return rc;
> +}

[ ... ]

> +/**
> + * free_phys_pg_list    - free physical page list
> + *
> + * @hdev                : habanalabs device structure
> + * @phys_pg_list        : physical page list to free
> + *
> + * This function does the following:
> + * - Iterate over the list and free each physical block structure
> + * - In case of allocated memory, return the physical memory to the general pool
> + * - Free the hl_vm_phys_pg_list structure
> + */
> +static void free_phys_pg_list(struct hl_device *hdev,
> +		struct hl_vm_phys_pg_list *phys_pg_list)
> +{
> +	struct hl_vm *vm = &hdev->vm;
> +	struct hl_vm_phys_pg *phys_pg, *tmp;
> +	u32 num_pgs;
> +	bool first = true;
> +	int i;
> +
> +	list_for_each_entry_safe(phys_pg, tmp, &phys_pg_list->list, node) {
> +		/*
> +		 * this if statement is relevant only when called from
> +		 * hl_vm_ctx_fini() and free_device_memory()
> +		 */
> +		if (!phys_pg_list->created_from_userptr) {
> +			if ((phys_pg_list->contiguous) && (first)) {
> +				first = false;
> +				gen_pool_free(vm->dram_pg_pool,
> +						phys_pg->paddr,
> +						phys_pg_list->total_size);
> +
> +				num_pgs = phys_pg_list->total_size >>
> +					__ffs(hdev->asic_prop.dram_page_size);
> +
> +				for (i = 0; i < num_pgs ; i++)
> +					kref_put(&vm->dram_pg_pool_refcount,
> +						dram_pg_pool_do_release);
> +
> +			} else if (!phys_pg_list->contiguous) {
> +				gen_pool_free(vm->dram_pg_pool, phys_pg->paddr,
> +						phys_pg->page_size);
> +				kref_put(&vm->dram_pg_pool_refcount,
> +						dram_pg_pool_do_release);
> +			}
> +		}
> +
> +		list_del(&phys_pg->node);
> +		kfree(phys_pg);
> +	}

Unless I'm missing something this can be simplified a bit:

if (!phys_pg_list->created_from_userptr) {
	for (i = 0; i < num_pgs ; i++)
		kref_put(&vm->dram_pg_pool_refcount,
			 dram_pg_pool_do_release);
	if (phys_pg_list->contiguous)
		gen_pool_free(vm->dram_pg_pool, phys_pg->paddr,
			      phys_pg_list->total_size);
}

list_for_each_entry_safe(phys_pg, tmp, &phys_pg_list->list, node) {
	if (!phys_pg_list->created_from_userptr &&
	    !phys_pg_list->contiguous)
		gen_pool_free(vm->dram_pg_pool, phys_pg->paddr,
			      phys_pg->page_size);
	list_del(&phys_pg->node);
	kfree(phys_pg);
}

nd with phys_pg's array hanging from phys_pg_list it would be even simpler
;-)

> +
> +	kfree(phys_pg_list);
> +}
> +

-- 
Sincerely yours,
Mike.

Powered by blists - more mailing lists