[RFC ADD HYPERVISOR CALL SUPPORT 0/2]: Add hypervisor call support to enable mss rproc on msm8996

[RFC ADD HYPERVISOR CALL SUPPORT 0/2]: Add hypervisor call support to enable mss rproc on msm8996

[Avaneesh Kumar Dwivedi]

This Patch series consist two patches.
	1- Add hypervisor call support to enable stage two translation applicable for armv8 and above.
	2- Add and enable mss remoteproc support for msm8996

This patchseries is dependednt on https://patchwork.kernel.org/patch/9492177/

Avaneesh Kumar Dwivedi (2):
  soc: qcom: Add hypervisor stage two translation request support.
  remoteproc: qcom: Add mss rproc support for msm8996.

 .../devicetree/bindings/remoteproc/qcom,q6v5.txt   |   2 +-
 drivers/firmware/qcom_scm-64.c                     |  16 ++
 drivers/firmware/qcom_scm.c                        |  13 +
 drivers/firmware/qcom_scm.h                        |  10 +
 drivers/remoteproc/qcom_q6v5_pil.c                 | 275 ++++++++++++++++++---
 drivers/soc/qcom/Kconfig                           |   8 +
 drivers/soc/qcom/Makefile                          |   1 +
 drivers/soc/qcom/secure_buffer.c                   | 229 +++++++++++++++++
 include/linux/qcom_scm.h                           |   1 +
 include/soc/qcom/secure_buffer.h                   |  51 ++++
 10 files changed, 574 insertions(+), 32 deletions(-)
 create mode 100644 drivers/soc/qcom/secure_buffer.c
 create mode 100644 include/soc/qcom/secure_buffer.h

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[RFC ADD HYPERVISOR CALL SUPPORT 1/2] soc: qcom: Add hypervisor stage two translation request support.

[Avaneesh Kumar Dwivedi]

This patch add hypervisor support for mss bring up on msm8996.
MSS rproc driver make hypervisor request to add certain region
in IPA table owned by hepervisor and assign access permission
to modem. These regions are used to load MBA, MDT, FW into DDR.

Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
---
 drivers/firmware/qcom_scm-64.c     |  16 +++
 drivers/firmware/qcom_scm.c        |  13 +++
 drivers/firmware/qcom_scm.h        |  10 ++
 drivers/remoteproc/qcom_q6v5_pil.c |  96 +++++++++++++++-
 drivers/soc/qcom/Kconfig           |   8 ++
 drivers/soc/qcom/Makefile          |   1 +
 drivers/soc/qcom/secure_buffer.c   | 229 +++++++++++++++++++++++++++++++++++++
 include/linux/qcom_scm.h           |   1 +
 include/soc/qcom/secure_buffer.h   |  51 +++++++++
 9 files changed, 419 insertions(+), 6 deletions(-)
 create mode 100644 drivers/soc/qcom/secure_buffer.c
 create mode 100644 include/soc/qcom/secure_buffer.h

diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
index 4a0f5ea..63b814c 100644
--- a/drivers/firmware/qcom_scm-64.c
+++ b/drivers/firmware/qcom_scm-64.c
@@ -358,3 +358,19 @@ int __qcom_scm_pas_mss_reset(struct device *dev, bool reset)
 
 	return ret ? : res.a1;
 }
+
+int __qcom_scm_assign_mem(struct device *dev, void *data)
+{
+	int ret;
+	struct qcom_scm_desc *desc = (struct qcom_scm_desc *)data;
+	struct arm_smccc_res res;
+
+	desc->arginfo = QCOM_SCM_ARGS(7, SCM_RO, SCM_VAL, SCM_RO,
+					SCM_VAL, SCM_RO, SCM_VAL, SCM_VAL);
+
+	ret = qcom_scm_call(dev, QCOM_SCM_SVC_MP,
+				QCOM_MEM_PROT_ASSIGN_ID,
+				desc, &res);
+
+	return ret ? : res.a1;
+}
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 893f953ea..52394ac 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -292,6 +292,19 @@ int qcom_scm_pas_shutdown(u32 peripheral)
 }
 EXPORT_SYMBOL(qcom_scm_pas_shutdown);
 
+/**
+ * qcom_scm_assign_mem() -
+ * @desc:	descriptor to send to hypervisor
+ *
+ * Return 0 on success.
+ */
+int qcom_scm_assign_mem(void *desc)
+{
+
+	return __qcom_scm_assign_mem(__scm->dev, desc);
+}
+EXPORT_SYMBOL(qcom_scm_assign_mem);
+
 static int qcom_scm_pas_reset_assert(struct reset_controller_dev *rcdev,
 				     unsigned long idx)
 {
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index 3584b00..f248dc9 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -47,6 +47,8 @@ extern int __qcom_scm_hdcp_req(struct device *dev,
 #define QCOM_SCM_PAS_SHUTDOWN_CMD	0x6
 #define QCOM_SCM_PAS_IS_SUPPORTED_CMD	0x7
 #define QCOM_SCM_PAS_MSS_RESET		0xa
+#define QCOM_SCM_SVC_MP				0xc
+#define QCOM_MEM_PROT_ASSIGN_ID		0x16
 extern bool __qcom_scm_pas_supported(struct device *dev, u32 peripheral);
 extern int  __qcom_scm_pas_init_image(struct device *dev, u32 peripheral,
 		dma_addr_t metadata_phys);
@@ -55,6 +57,7 @@ extern int  __qcom_scm_pas_mem_setup(struct device *dev, u32 peripheral,
 extern int  __qcom_scm_pas_auth_and_reset(struct device *dev, u32 peripheral);
 extern int  __qcom_scm_pas_shutdown(struct device *dev, u32 peripheral);
 extern int  __qcom_scm_pas_mss_reset(struct device *dev, bool reset);
+extern int  __qcom_scm_assign_mem(struct device *dev, void *desc);
 
 /* common error codes */
 #define QCOM_SCM_V2_EBUSY	-12
@@ -83,4 +86,11 @@ static inline int qcom_scm_remap_error(int err)
 	return -EINVAL;
 }
 
+enum scm_arg_types {
+	SCM_VAL,
+	SCM_RO,
+	SCM_RW,
+	SCM_BUFVAL,
+};
+
 #endif
diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index e5edefa..cbf833c 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -31,6 +31,7 @@
 #include <linux/soc/qcom/smem.h>
 #include <linux/soc/qcom/smem_state.h>
 #include <linux/of_device.h>
+#include <soc/qcom/secure_buffer.h>
 
 #include "remoteproc_internal.h"
 #include "qcom_mdt_loader.h"
@@ -105,12 +106,19 @@ struct qcom_mss_reg_res {
 	int uA;
 };
 
+struct src_dest_vmid {
+	int *srcVM;
+	int *destVM;
+	int *destVMperm;
+};
+
 struct rproc_hexagon_res {
 	const char *hexagon_mba_image;
 	struct qcom_mss_reg_res proxy_supply[4];
 	struct qcom_mss_reg_res active_supply[2];
 	char **proxy_clk_names;
 	char **active_clk_names;
+	int version;
 };
 
 struct q6v5 {
@@ -127,6 +135,8 @@ struct q6v5 {
 
 	struct reset_control *mss_restart;
 
+	struct src_dest_vmid vmid_details;
+
 	struct qcom_smem_state *state;
 	unsigned stop_bit;
 
@@ -152,6 +162,13 @@ struct q6v5 {
 	phys_addr_t mpss_reloc;
 	void *mpss_region;
 	size_t mpss_size;
+	int version;
+};
+
+enum {
+	MSS_MSM8916,
+	MSS_MSM8974,
+	MSS_MSM8996,
 };
 
 static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,
@@ -273,13 +290,46 @@ static void q6v5_clk_disable(struct device *dev,
 		clk_disable_unprepare(clks[i]);
 }
 
+int hyp_mem_access(struct q6v5 *qproc, phys_addr_t addr, size_t size)
+{
+	int src_count = 0;
+	int dest_count = 0;
+	int ret;
+	int i;
+
+	if (qproc->version != MSS_MSM8996)
+		return 0;
+
+	for (i = 0; i < 2; i++) {
+		if (qproc->vmid_details.srcVM[i] != 0)
+		src_count++;
+		if (qproc->vmid_details.destVM[i] != 0)
+		dest_count++;
+	}
+	ret = hyp_assign_phys(qproc->dev, addr, size,
+			qproc->vmid_details.srcVM,
+			src_count, qproc->vmid_details.destVM,
+			qproc->vmid_details.destVMperm, dest_count);
+	if (ret)
+		dev_err(qproc->dev,
+			"%s: failed for %pa address of size %zx\n",
+			__func__, &addr, size);
+	return ret;
+}
+
 static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
 {
 	struct q6v5 *qproc = rproc->priv;
+	int ret;
 
 	memcpy(qproc->mba_region, fw->data, fw->size);
-
-	return 0;
+	qproc->vmid_details.srcVM = (int[2]) {VMID_HLOS, 0};
+	qproc->vmid_details.destVM = (int[2]) {VMID_MSS_MSA, 0};
+	qproc->vmid_details.destVMperm = (int[2]) {PERM_READ | PERM_WRITE, 0};
+	ret = hyp_mem_access(qproc, qproc->mba_phys, qproc->mba_size);
+	if (ret)
+		dev_err(qproc->dev, "Failed to assign memory, ret - %d\n", ret);
+	return ret;
 }
 
 static const struct rproc_fw_ops q6v5_fw_ops = {
@@ -445,6 +495,13 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
 	}
 
 	memcpy(ptr, fw->data, fw->size);
+	/* Hypervisor support to access metadata by modem */
+	qproc->vmid_details.srcVM = (int[2]) {VMID_HLOS, 0};
+	qproc->vmid_details.destVM = (int[2]) {VMID_MSS_MSA, 0};
+	qproc->vmid_details.destVMperm = (int[2]) {PERM_READ | PERM_WRITE, 0};
+	ret = hyp_mem_access(qproc, phys, ALIGN(fw->size, SZ_4K));
+	if (ret)
+		dev_err(qproc->dev, "Failed to assign memory, ret - %d\n", ret);
 
 	writel(phys, qproc->rmb_base + RMB_PMI_META_DATA_REG);
 	writel(RMB_CMD_META_DATA_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG);
@@ -454,7 +511,14 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
 		dev_err(qproc->dev, "MPSS header authentication timed out\n");
 	else if (ret < 0)
 		dev_err(qproc->dev, "MPSS header authentication failed: %d\n", ret);
-
+	/* Metadata authentication done, remove modem access */
+	qproc->vmid_details.srcVM = (int[2]) {VMID_MSS_MSA, 0};
+	qproc->vmid_details.destVM = (int[2]) {VMID_HLOS, 0};
+	qproc->vmid_details.destVMperm =
+		(int[2]) {PERM_READ | PERM_WRITE | PERM_EXEC, 0};
+	ret = hyp_mem_access(qproc, phys, ALIGN(fw->size, SZ_4K));
+	if (ret)
+		dev_err(qproc->dev, "Failed to assign memory, ret - %d\n", ret);
 	dma_free_attrs(qproc->dev, fw->size, ptr, phys, dma_attrs);
 
 	return ret < 0 ? ret : 0;
@@ -482,7 +546,14 @@ static int q6v5_mpss_validate(struct q6v5 *qproc, const struct firmware *fw)
 		boot_addr = qproc->mpss_phys;
 	else
 		boot_addr = fw_addr;
-
+	/* Hypervisor support to modem to access modem fw */
+	qproc->vmid_details.srcVM = (int[2]) {VMID_HLOS, 0};
+	qproc->vmid_details.destVM = (int[2]) {VMID_HLOS, VMID_MSS_MSA};
+	qproc->vmid_details.destVMperm =
+		(int[2]) {PERM_READ | PERM_WRITE, PERM_READ | PERM_WRITE};
+	ret = hyp_mem_access(qproc, boot_addr, qproc->mpss_size);
+	if (ret)
+		dev_err(qproc->dev, "Failed to assign memory, ret - %d\n", ret);
 	ehdr = (struct elf32_hdr *)fw->data;
 	phdrs = (struct elf32_phdr *)(ehdr + 1);
 	for (i = 0; i < ehdr->e_phnum; i++, phdr++) {
@@ -560,6 +631,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
 static int q6v5_start(struct rproc *rproc)
 {
 	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
+	int status;
 	int ret;
 
 	ret = q6v5_regulator_enable(qproc, qproc->proxy_regs,
@@ -616,14 +688,14 @@ static int q6v5_start(struct rproc *rproc)
 
 	ret = q6v5_mpss_load(qproc);
 	if (ret)
-		goto halt_axi_ports;
+		goto reclaim_mem;
 
 	ret = wait_for_completion_timeout(&qproc->start_done,
 					  msecs_to_jiffies(5000));
 	if (ret == 0) {
 		dev_err(qproc->dev, "start timed out\n");
 		ret = -ETIMEDOUT;
-		goto halt_axi_ports;
+		goto reclaim_mem;
 	}
 
 	qproc->running = true;
@@ -634,6 +706,15 @@ static int q6v5_start(struct rproc *rproc)
 				qproc->proxy_reg_count);
 	return 0;
 
+reclaim_mem:
+	qproc->vmid_details.srcVM = (int[2]) {VMID_HLOS, VMID_MSS_MSA};
+	qproc->vmid_details.destVM = (int[2]) {VMID_HLOS, 0};
+	qproc->vmid_details.destVMperm =
+		(int[2]) {PERM_READ | PERM_WRITE | PERM_EXEC, 0};
+	status = hyp_mem_access(qproc, qproc->mpss_phys, qproc->mpss_size);
+	if (status)
+		dev_err(qproc->dev,
+			"Failed to reclaim memory, ret - %d\n", ret);
 halt_axi_ports:
 	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6);
 	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
@@ -977,6 +1058,7 @@ static int q6v5_probe(struct platform_device *pdev)
 	if (ret)
 		goto free_rproc;
 
+	qproc->version = desc->version;
 	ret = q6v5_request_irq(qproc, pdev, "wdog", q6v5_wdog_interrupt);
 	if (ret < 0)
 		goto free_rproc;
@@ -1056,6 +1138,7 @@ static int q6v5_remove(struct platform_device *pdev)
 			"mem",
 			NULL
 	},
+	.version = MSS_MSM8916,
 };
 
 static const struct rproc_hexagon_res msm8974_mss = {
@@ -1093,6 +1176,7 @@ static int q6v5_remove(struct platform_device *pdev)
 			"mem",
 			NULL
 	},
+	.version = MSS_MSM8974,
 };
 static const struct of_device_id q6v5_of_match[] = {
 	{ .compatible = "qcom,q6v5-pil", .data = &msm8916_mss},
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 461b387..9459894 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -76,3 +76,11 @@ config QCOM_WCNSS_CTRL
 	help
 	  Client driver for the WCNSS_CTRL SMD channel, used to download nv
 	  firmware to a newly booted WCNSS chip.
+
+config QCOM_SECURE_BUFFER
+	bool "Helper functions for securing buffers through TZ"
+	help
+	 Say 'Y' here for targets that need to call into TZ to secure
+	 memory buffers. This ensures that only the correct clients can
+	 use this memory and no unauthorized access is made to the
+	 buffer
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index fdd664e..e85dc66 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -6,4 +6,5 @@ obj-$(CONFIG_QCOM_SMEM) +=	smem.o
 obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
 obj-$(CONFIG_QCOM_SMP2P)	+= smp2p.o
 obj-$(CONFIG_QCOM_SMSM)	+= smsm.o
+obj-$(CONFIG_QCOM_SECURE_BUFFER) += secure_buffer.o
 obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
diff --git a/drivers/soc/qcom/secure_buffer.c b/drivers/soc/qcom/secure_buffer.c
new file mode 100644
index 0000000..54eaa6f
--- /dev/null
+++ b/drivers/soc/qcom/secure_buffer.c
@@ -0,0 +1,229 @@
+/*
+ * Copyright (c) 2011-2017, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/highmem.h>
+#include <linux/kernel.h>
+#include <linux/kref.h>
+#include <linux/mutex.h>
+#include <linux/scatterlist.h>
+#include <linux/slab.h>
+#include <linux/dma-mapping.h>
+#include <linux/qcom_scm.h>
+#include <linux/dma-mapping.h>
+#include <soc/qcom/secure_buffer.h>
+
+DEFINE_MUTEX(secure_buffer_mutex);
+
+struct scm_desc {
+	u32 arginfo;
+	u64 args[10];
+};
+
+struct mem_prot_info {
+	phys_addr_t addr;
+	u64 size;
+};
+
+struct info_list {
+	struct mem_prot_info *list_head;
+	u64 list_size;
+};
+
+struct dest_vm_and_perm_info {
+	u32 vm;
+	u32 perm;
+	u32 *ctx;
+	u32 ctx_size;
+};
+
+struct dest_info_list {
+	struct dest_vm_and_perm_info *dest_info;
+	u64 list_size;
+};
+
+static void *qcom_secure_mem;
+#define QCOM_SECURE_MEM_SIZE (512 * 1024)
+#define PADDING 32
+
+static void populate_dest_info(int *dest_vmids, int nelements,
+			int *dest_perms, struct dest_info_list **list,
+			void *current_qcom_secure_mem)
+{
+	struct dest_vm_and_perm_info *dest_info;
+	int i;
+
+	dest_info = (struct dest_vm_and_perm_info *)current_qcom_secure_mem;
+
+	for (i = 0; i < nelements; i++) {
+		dest_info[i].vm = dest_vmids[i];
+		dest_info[i].perm = dest_perms[i];
+		dest_info[i].ctx = NULL;
+		dest_info[i].ctx_size = 0;
+	}
+
+	*list = (struct dest_info_list *)&dest_info[i];
+
+	(*list)->dest_info = dest_info;
+	(*list)->list_size = nelements * sizeof(struct dest_vm_and_perm_info);
+}
+
+static void get_info_list_from_table(struct sg_table *table,
+					struct info_list **list)
+{
+	int i;
+	struct scatterlist *sg;
+	struct mem_prot_info *info;
+
+	info = (struct mem_prot_info *)qcom_secure_mem;
+
+	for_each_sg(table->sgl, sg, table->nents, i) {
+		info[i].addr = page_to_phys(sg_page(sg));
+		info[i].size = sg->length;
+	}
+
+	*list = (struct info_list *)&(info[i]);
+
+	(*list)->list_head = info;
+	(*list)->list_size = table->nents * sizeof(struct mem_prot_info);
+}
+
+int hyp_assign_table(struct device *dev, struct sg_table *table,
+			u32 *source_vm_list, int source_nelems,
+			int *dest_vmids, int *dest_perms,
+			int dest_nelems)
+{
+	int ret;
+	struct info_list *info_list = NULL;
+	struct dest_info_list *dest_info_list = NULL;
+	struct scm_desc desc = {0};
+	u32 *source_vm_copy;
+	void *current_qcom_secure_mem;
+
+	size_t reqd_size = dest_nelems * sizeof(struct dest_vm_and_perm_info) +
+			table->nents * sizeof(struct mem_prot_info) +
+			sizeof(dest_info_list) + sizeof(info_list) + PADDING;
+
+	if (!qcom_secure_mem) {
+		pr_err("%s is not functional as qcom_secure_mem is not allocated.\n",
+				__func__);
+		return -ENOMEM;
+	}
+
+	if (reqd_size > QCOM_SECURE_MEM_SIZE) {
+		pr_err("%s: Not enough memory allocated. Required size %zd\n",
+				__func__, reqd_size);
+		return -EINVAL;
+	}
+
+	/*
+	 * We can only pass cache-aligned sizes to hypervisor, so we need
+	 * to kmalloc and memcpy the source_vm_list here.
+	 */
+	source_vm_copy = kmalloc_array(
+		source_nelems, sizeof(*source_vm_copy), GFP_KERNEL);
+	if (!source_vm_copy)
+		return -ENOMEM;
+	memcpy(source_vm_copy, source_vm_list,
+	       sizeof(*source_vm_list) * source_nelems);
+
+	mutex_lock(&secure_buffer_mutex);
+
+	get_info_list_from_table(table, &info_list);
+
+	current_qcom_secure_mem = &(info_list[1]);
+	populate_dest_info(dest_vmids, dest_nelems, dest_perms,
+				&dest_info_list, current_qcom_secure_mem);
+
+	desc.args[0] = virt_to_phys(info_list->list_head);
+	desc.args[1] = info_list->list_size;
+	desc.args[2] = virt_to_phys(source_vm_copy);
+	desc.args[3] = sizeof(*source_vm_copy) * source_nelems;
+	desc.args[4] = virt_to_phys(dest_info_list->dest_info);
+	desc.args[5] = dest_info_list->list_size;
+	desc.args[6] = 0;
+
+	dma_set_mask(dev, DMA_BIT_MASK(64));
+	dma_map_single(dev, (void *)source_vm_copy,
+			(size_t)(source_vm_copy + source_nelems),
+			DMA_TO_DEVICE);
+	dma_map_single(dev, (void *)info_list->list_head,
+		(size_t)(info_list->list_head +
+		(info_list->list_size / sizeof(*info_list->list_head))),
+		DMA_TO_DEVICE);
+	dma_map_single(dev,
+		(void *)dest_info_list->dest_info,
+		(size_t)(dest_info_list->dest_info +
+		(dest_info_list->list_size /
+		sizeof(*dest_info_list->dest_info))),
+		DMA_TO_DEVICE);
+
+	ret = qcom_scm_assign_mem((void *)&desc);
+	if (ret)
+		pr_info("%s: Failed to assign memory protection, ret = %d\n",
+			__func__, ret);
+
+	mutex_unlock(&secure_buffer_mutex);
+	kfree(source_vm_copy);
+	return ret;
+}
+
+int hyp_assign_phys(struct device *dev, phys_addr_t addr, u64 size,
+			u32 *source_vm_list, int source_nelems,
+			int *dest_vmids, int *dest_perms,
+			int dest_nelems)
+{
+	struct sg_table *table;
+	int ret;
+
+	table = kzalloc(sizeof(struct sg_table), GFP_KERNEL);
+	if (!table)
+		return -ENOMEM;
+	ret = sg_alloc_table(table, 1, GFP_KERNEL);
+	if (ret)
+		goto err1;
+
+	sg_set_page(table->sgl, phys_to_page(addr), size, 0);
+
+	ret = hyp_assign_table(dev, table, source_vm_list,
+				source_nelems, dest_vmids,
+				dest_perms, dest_nelems);
+	if (ret)
+		goto err2;
+
+	return ret;
+err2:
+	sg_free_table(table);
+err1:
+	kfree(table);
+	return ret;
+}
+
+static int __init alloc_secure_shared_memory(void)
+{
+	int ret = 0;
+
+	qcom_secure_mem = kzalloc(QCOM_SECURE_MEM_SIZE, GFP_KERNEL);
+	if (!qcom_secure_mem) {
+		/* Fallback to CMA-DMA memory */
+		qcom_secure_mem = dma_alloc_coherent(NULL, QCOM_SECURE_MEM_SIZE,
+						NULL, GFP_KERNEL);
+		if (!qcom_secure_mem) {
+			pr_err("Couldn't allocate memory for secure use-cases. hyp_assign_table will not work\n");
+			return -ENOMEM;
+		}
+	}
+
+	return ret;
+}
+pure_initcall(alloc_secure_shared_memory);
diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
index cc32ab8..f9ecf35 100644
--- a/include/linux/qcom_scm.h
+++ b/include/linux/qcom_scm.h
@@ -36,6 +36,7 @@ extern int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr,
 		phys_addr_t size);
 extern int qcom_scm_pas_auth_and_reset(u32 peripheral);
 extern int qcom_scm_pas_shutdown(u32 peripheral);
+extern int qcom_scm_assign_mem(void *desc);
 
 #define QCOM_SCM_CPU_PWR_DOWN_L2_ON	0x0
 #define QCOM_SCM_CPU_PWR_DOWN_L2_OFF	0x1
diff --git a/include/soc/qcom/secure_buffer.h b/include/soc/qcom/secure_buffer.h
new file mode 100644
index 0000000..2c7015d
--- /dev/null
+++ b/include/soc/qcom/secure_buffer.h
@@ -0,0 +1,51 @@
+/*
+ * Copyright (c) 2015-2016, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#ifndef __MSM_SECURE_BUFFER_H__
+#define __MSM_SECURE_BUFFER_H__
+
+enum vmid {
+	VMID_HLOS = 0x3,
+	VMID_MSS_MSA = 0xF,
+	VMID_INVAL = -1
+};
+
+#define PERM_READ			0x4
+#define PERM_WRITE			0x2
+#define PERM_EXEC			0x1
+
+#ifdef CONFIG_QCOM_SECURE_BUFFER
+int hyp_assign_table(struct device *dev, struct sg_table *table,
+			u32 *source_vm_list, int source_nelems,
+			int *dest_vmids, int *dest_perms,
+			int dest_nelems);
+int hyp_assign_phys(struct device *dev, phys_addr_t addr, u64 size,
+			u32 *source_vmlist, int source_nelems,
+			int *dest_vmids, int *dest_perms, int dest_nelems);
+#else
+static inline int hyp_assign_table(struct device *dev, struct sg_table *table,
+			u32 *source_vm_list, int source_nelems,
+			int *dest_vmids, int *dest_perms,
+			int dest_nelems)
+{
+	return -EPROTONOSUPPORT;
+}
+static inline int hyp_assign_phys(struct device *dev, phys_addr_t addr,
+			u64 size, u32 *source_vmlist, int source_nelems,
+			int *dest_vmids, int *dest_perms, int dest_nelems)
+{
+	return -EPROTONOSUPPORT;
+}
+#endif
+#endif
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[RFC ADD HYPERVISOR CALL SUPPORT 2/2] remoteproc: qcom: Add mss rproc support for msm8996.

[Avaneesh Kumar Dwivedi]

This patch add msm8996 specific code piece to
take mss out of reset.

Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
---
 .../devicetree/bindings/remoteproc/qcom,q6v5.txt   |   2 +-
 drivers/remoteproc/qcom_q6v5_pil.c                 | 179 ++++++++++++++++++---
 2 files changed, 155 insertions(+), 26 deletions(-)

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
index 674ebc6..7217647 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
@@ -10,7 +10,7 @@ on the Qualcomm Hexagon core.
 		"qcom,q6v5-pil"
 		"qcom,msm8916-mss-pil"
 		"qcom,msm8974-mss-pil"
-
+		"qcom,msm8996-mss-pil"
 - reg:
 	Usage: required
 	Value type: <prop-encoded-array>
diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index cbf833c..c512c1d 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -31,6 +31,7 @@
 #include <linux/soc/qcom/smem.h>
 #include <linux/soc/qcom/smem_state.h>
 #include <linux/of_device.h>
+#include <linux/iopoll.h>
 #include <soc/qcom/secure_buffer.h>
 
 #include "remoteproc_internal.h"
@@ -66,6 +67,8 @@
 #define QDSP6SS_RESET_REG		0x014
 #define QDSP6SS_GFMUX_CTL_REG		0x020
 #define QDSP6SS_PWR_CTL_REG		0x030
+#define QDSP6SS_MEM_PWR_CTL		0x0B0
+#define QDSP6SS_STRAP_ACC		0x110
 
 /* AXI Halt Register Offsets */
 #define AXI_HALTREQ_REG			0x0
@@ -94,6 +97,15 @@
 #define QDSS_BHS_ON			BIT(21)
 #define QDSS_LDO_BYP			BIT(22)
 
+/* QDSP6v56 parameters */
+#define QDSP6v56_LDO_BYP                BIT(25)
+#define QDSP6v56_BHS_ON                 BIT(24)
+#define QDSP6v56_CLAMP_WL               BIT(21)
+#define QDSP6v56_CLAMP_QMC_MEM          BIT(22)
+#define HALT_CHECK_MAX_LOOPS		200
+#define QDSP6SS_XO_CBCR			0x0038
+#define QDSP6SS_ACC_OVERRIDE_VAL	0x20
+
 struct reg_info {
 	struct regulator *reg;
 	int uV;
@@ -385,35 +397,99 @@ static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms)
 
 static int q6v5proc_reset(struct q6v5 *qproc)
 {
-	u32 val;
+	u64 val;
 	int ret;
+	int i;
 
-	/* Assert resets, stop core */
-	val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
-	val |= (Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE);
-	writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
 
-	/* Enable power block headswitch, and wait for it to stabilize */
-	val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-	val |= QDSS_BHS_ON | QDSS_LDO_BYP;
-	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-	udelay(1);
-
-	/*
-	 * Turn on memories. L2 banks should be done individually
-	 * to minimize inrush current.
-	 */
-	val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-	val |= Q6SS_SLP_RET_N | Q6SS_L2TAG_SLP_NRET_N |
-		Q6SS_ETB_SLP_NRET_N | Q6SS_L2DATA_STBY_N;
-	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-	val |= Q6SS_L2DATA_SLP_NRET_N_2;
-	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-	val |= Q6SS_L2DATA_SLP_NRET_N_1;
-	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
-	val |= Q6SS_L2DATA_SLP_NRET_N_0;
-	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+	if (qproc->version == MSS_MSM8996) {
+		/* Override the ACC value if required */
+		writel(QDSP6SS_ACC_OVERRIDE_VAL,
+			qproc->reg_base + QDSP6SS_STRAP_ACC);
+
+		/* Assert resets, stop core */
+		val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
+		val |= (Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE);
+		writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
+
+		/* BHS require xo cbcr to be enabled */
+		val = readl(qproc->reg_base + QDSP6SS_XO_CBCR);
+		val |= 0x1;
+		writel(val, qproc->reg_base + QDSP6SS_XO_CBCR);
 
+		/* Read CLKOFF bit to go low indicating CLK is enabled */
+		ret = readl_poll_timeout(qproc->reg_base + QDSP6SS_XO_CBCR,
+				val, !(val & BIT(31)), 1, HALT_CHECK_MAX_LOOPS);
+		if (ret) {
+			dev_err(qproc->dev,
+				"xo cbcr enabling timed out (rc:%d)\n", ret);
+			return ret;
+		}
+		/* Enable power block headswitch and wait for it to stabilize */
+		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val |= QDSP6v56_BHS_ON;
+		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val |= readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		udelay(1);
+
+		/* Put LDO in bypass mode */
+		val |= QDSP6v56_LDO_BYP;
+		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+		/* Deassert QDSP6 compiler memory clamp */
+		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val &= ~QDSP6v56_CLAMP_QMC_MEM;
+		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+		/* Deassert memory peripheral sleep and L2 memory standby */
+		val |= (Q6SS_L2DATA_STBY_N | Q6SS_SLP_RET_N);
+		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+		/* Turn on L1, L2, ETB and JU memories 1 at a time */
+		val = readl(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
+		for (i = 19; i >= 0; i--) {
+			val |= BIT(i);
+			writel(val, qproc->reg_base +
+						QDSP6SS_MEM_PWR_CTL);
+			/*
+			 * Read back value to ensure the write is done then
+			 * wait for 1us for both memory peripheral and data
+			 * array to turn on.
+			 */
+			val |= readl(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
+			udelay(1);
+		}
+		/* Remove word line clamp */
+		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val &= ~QDSP6v56_CLAMP_WL;
+		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+	} else {
+		/* Assert resets, stop core */
+		val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
+		val |= (Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE);
+		writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
+
+		/* Enable power block headswitch and wait for it to stabilize */
+		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val |= QDSS_BHS_ON | QDSS_LDO_BYP;
+		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val |= readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		udelay(1);
+		/*
+		 * Turn on memories. L2 banks should be done individually
+		 * to minimize inrush current.
+		 */
+		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val |= Q6SS_SLP_RET_N | Q6SS_L2TAG_SLP_NRET_N |
+			Q6SS_ETB_SLP_NRET_N | Q6SS_L2DATA_STBY_N;
+		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val |= Q6SS_L2DATA_SLP_NRET_N_2;
+		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val |= Q6SS_L2DATA_SLP_NRET_N_1;
+		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val |= Q6SS_L2DATA_SLP_NRET_N_0;
+		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+	}
 	/* Remove IO clamp */
 	val &= ~Q6SS_CLAMP_IO;
 	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
@@ -739,6 +815,7 @@ static int q6v5_stop(struct rproc *rproc)
 {
 	struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
 	int ret;
+	int val;
 
 	qproc->running = false;
 
@@ -756,6 +833,15 @@ static int q6v5_stop(struct rproc *rproc)
 	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
 	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
 
+	if (qproc->version == MSS_MSM8996) {
+		/*
+		 * To avoid high MX current during LPASS/MSS restart.
+		 */
+		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val |= Q6SS_CLAMP_IO | QDSP6v56_CLAMP_WL |
+			QDSP6v56_CLAMP_QMC_MEM;
+		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+	}
 	reset_control_assert(qproc->mss_restart);
 	q6v5_clk_disable(qproc->dev, qproc->active_clks,
 				qproc->active_clk_count);
@@ -1103,6 +1189,47 @@ static int q6v5_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct rproc_hexagon_res msm8996_mss = {
+	.hexagon_mba_image = "mba.mbn",
+	.proxy_supply = (struct qcom_mss_reg_res[]) {
+		{
+			.supply = "vdd_mx",
+			.uV = 6,
+		},
+		{
+			.supply = "vdd_cx",
+			.uV = 7,
+			.uA = 100000,
+		},
+		{
+			.supply = "vdd_pll",
+			.uV = 1800000,
+			.uA = 100000,
+		},
+		{}
+	},
+	.active_supply = (struct qcom_mss_reg_res[]) {
+			{},
+			{}
+	},
+	.proxy_clk_names = (char*[]){
+			"xo",
+			"pnoc",
+			"qdss",
+			NULL
+	},
+	.active_clk_names = (char*[]){
+			"iface",
+			"bus",
+			"mem",
+			"gpll0_mss_clk",
+			"snoc_axi_clk",
+			"mnoc_axi_clk",
+			NULL
+	},
+	.version = MSS_MSM8996,
+};
+
 static const struct rproc_hexagon_res msm8916_mss = {
 	.hexagon_mba_image = "mba.mbn",
 	.proxy_supply = (struct qcom_mss_reg_res[]) {
@@ -1178,10 +1305,12 @@ static int q6v5_remove(struct platform_device *pdev)
 	},
 	.version = MSS_MSM8974,
 };
+
 static const struct of_device_id q6v5_of_match[] = {
 	{ .compatible = "qcom,q6v5-pil", .data = &msm8916_mss},
 	{ .compatible = "qcom,msm8916-mss-pil", .data = &msm8916_mss},
 	{ .compatible = "qcom,msm8974-mss-pil", .data = &msm8974_mss},
+	{ .compatible = "qcom,msm8996-mss-pil", .data = &msm8996_mss},
 	{ },
 };
 MODULE_DEVICE_TABLE(of, q6v5_of_match);
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [RFC ADD HYPERVISOR CALL SUPPORT 1/2] soc: qcom: Add hypervisor stage two translation request support.

[Stephen Boyd]

On 01/09, Avaneesh Kumar Dwivedi wrote:
> This patch add hypervisor support for mss bring up on msm8996.
> MSS rproc driver make hypervisor request to add certain region
> in IPA table owned by hepervisor and assign access permission

Please drop the use of IPA here. There's an IPA acronym for the
IP accelerator and that is confused with Intermediate Physical
Address. Instead, say something like "in the stage 2 page tables
of the SMMU". Also hypervisor is misspelled here.

> to modem. These regions are used to load MBA, MDT, FW into DDR.
> 
> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> ---
>  drivers/firmware/qcom_scm-64.c     |  16 +++
>  drivers/firmware/qcom_scm.c        |  13 +++
>  drivers/firmware/qcom_scm.h        |  10 ++
>  drivers/remoteproc/qcom_q6v5_pil.c |  96 +++++++++++++++-

Please split the remoteproc code off from this patch.

>  drivers/soc/qcom/Kconfig           |   8 ++
>  drivers/soc/qcom/Makefile          |   1 +
>  drivers/soc/qcom/secure_buffer.c   | 229 +++++++++++++++++++++++++++++++++++++
>  include/linux/qcom_scm.h           |   1 +
>  include/soc/qcom/secure_buffer.h   |  51 +++++++++
>  9 files changed, 419 insertions(+), 6 deletions(-)
>  create mode 100644 drivers/soc/qcom/secure_buffer.c
>  create mode 100644 include/soc/qcom/secure_buffer.h
> 
> diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
> index 4a0f5ea..63b814c 100644
> --- a/drivers/firmware/qcom_scm-64.c
> +++ b/drivers/firmware/qcom_scm-64.c
> @@ -358,3 +358,19 @@ int __qcom_scm_pas_mss_reset(struct device *dev, bool reset)
>  
>  	return ret ? : res.a1;
>  }
> +
> +int __qcom_scm_assign_mem(struct device *dev, void *data)
> +{
> +	int ret;
> +	struct qcom_scm_desc *desc = (struct qcom_scm_desc *)data;

Useless cast from void.

> +	struct arm_smccc_res res;
> +
> +	desc->arginfo = QCOM_SCM_ARGS(7, SCM_RO, SCM_VAL, SCM_RO,
> +					SCM_VAL, SCM_RO, SCM_VAL, SCM_VAL);
> +
> +	ret = qcom_scm_call(dev, QCOM_SCM_SVC_MP,
> +				QCOM_MEM_PROT_ASSIGN_ID,
> +				desc, &res);
> +
> +	return ret ? : res.a1;
> +}
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 893f953ea..52394ac 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -292,6 +292,19 @@ int qcom_scm_pas_shutdown(u32 peripheral)
>  }
>  EXPORT_SYMBOL(qcom_scm_pas_shutdown);
>  
> +/**
> + * qcom_scm_assign_mem() -

Some words on what the function does?

> + * @desc:	descriptor to send to hypervisor
> + *
> + * Return 0 on success.
> + */
> +int qcom_scm_assign_mem(void *desc)
> +{
> +

Nitpick: Drop the extra newline

> +	return __qcom_scm_assign_mem(__scm->dev, desc);
> +}
> +EXPORT_SYMBOL(qcom_scm_assign_mem);
> +
>  static int qcom_scm_pas_reset_assert(struct reset_controller_dev *rcdev,
>  				     unsigned long idx)
>  {
> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
> index 3584b00..f248dc9 100644
> --- a/drivers/firmware/qcom_scm.h
> +++ b/drivers/firmware/qcom_scm.h
> @@ -47,6 +47,8 @@ extern int __qcom_scm_hdcp_req(struct device *dev,
>  #define QCOM_SCM_PAS_SHUTDOWN_CMD	0x6
>  #define QCOM_SCM_PAS_IS_SUPPORTED_CMD	0x7
>  #define QCOM_SCM_PAS_MSS_RESET		0xa
> +#define QCOM_SCM_SVC_MP				0xc
> +#define QCOM_MEM_PROT_ASSIGN_ID		0x16
>  extern bool __qcom_scm_pas_supported(struct device *dev, u32 peripheral);
>  extern int  __qcom_scm_pas_init_image(struct device *dev, u32 peripheral,
>  		dma_addr_t metadata_phys);
> @@ -55,6 +57,7 @@ extern int  __qcom_scm_pas_mem_setup(struct device *dev, u32 peripheral,
>  extern int  __qcom_scm_pas_auth_and_reset(struct device *dev, u32 peripheral);
>  extern int  __qcom_scm_pas_shutdown(struct device *dev, u32 peripheral);
>  extern int  __qcom_scm_pas_mss_reset(struct device *dev, bool reset);
> +extern int  __qcom_scm_assign_mem(struct device *dev, void *desc);
>  
>  /* common error codes */
>  #define QCOM_SCM_V2_EBUSY	-12
> @@ -83,4 +86,11 @@ static inline int qcom_scm_remap_error(int err)
>  	return -EINVAL;
>  }
>  
> +enum scm_arg_types {
> +	SCM_VAL,
> +	SCM_RO,
> +	SCM_RW,
> +	SCM_BUFVAL,
> +};

We already have this enum? It's just prepended with QCOM_
instead.

> +
>  #endif
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> index e5edefa..cbf833c 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -31,6 +31,7 @@
>  #include <linux/soc/qcom/smem.h>
>  #include <linux/soc/qcom/smem_state.h>
>  #include <linux/of_device.h>
> +#include <soc/qcom/secure_buffer.h>
>  
>  #include "remoteproc_internal.h"
>  #include "qcom_mdt_loader.h"
> @@ -105,12 +106,19 @@ struct qcom_mss_reg_res {
>  	int uA;
>  };
>  
> +struct src_dest_vmid {
> +	int *srcVM;
> +	int *destVM;
> +	int *destVMperm;

Why not have an array of these structures instead of a structure
with three arrays inside? Can you map one source VM to multiple
destination VMs?

> +};
> +
>  struct rproc_hexagon_res {
>  	const char *hexagon_mba_image;
>  	struct qcom_mss_reg_res proxy_supply[4];
>  	struct qcom_mss_reg_res active_supply[2];
>  	char **proxy_clk_names;
>  	char **active_clk_names;
> +	int version;
>  };
>  
>  struct q6v5 {
> @@ -127,6 +135,8 @@ struct q6v5 {
>  
>  	struct reset_control *mss_restart;
>  
> +	struct src_dest_vmid vmid_details;
> +
>  	struct qcom_smem_state *state;
>  	unsigned stop_bit;
>  
> @@ -152,6 +162,13 @@ struct q6v5 {
>  	phys_addr_t mpss_reloc;
>  	void *mpss_region;
>  	size_t mpss_size;
> +	int version;
> +};
> +
> +enum {
> +	MSS_MSM8916,
> +	MSS_MSM8974,
> +	MSS_MSM8996,
>  };
>  
>  static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,
> @@ -273,13 +290,46 @@ static void q6v5_clk_disable(struct device *dev,
>  		clk_disable_unprepare(clks[i]);
>  }
>  
> +int hyp_mem_access(struct q6v5 *qproc, phys_addr_t addr, size_t size)
> +{
> +	int src_count = 0;
> +	int dest_count = 0;
> +	int ret;
> +	int i;
> +
> +	if (qproc->version != MSS_MSM8996)
> +		return 0;
> +
> +	for (i = 0; i < 2; i++) {
> +		if (qproc->vmid_details.srcVM[i] != 0)
> +		src_count++;

Bad tabbing here.

> +		if (qproc->vmid_details.destVM[i] != 0)
> +		dest_count++;

And here.

When is it ever == 0? The hardcoded 2 in the for loop is also
suspect.

> +	}

Add newline

> +	ret = hyp_assign_phys(qproc->dev, addr, size,
> +			qproc->vmid_details.srcVM,
> +			src_count, qproc->vmid_details.destVM,
> +			qproc->vmid_details.destVMperm, dest_count);

At the least this API could take a vmid_details structure instead
of all these arguments:

	struct vm_perms {
		u32 vm;
		u32 perm;
	};

	struct vmid_details {
		u32 *src;
		size_t src_count;
		struct vm_perms *dest;
		size_t dest_count;
	};

> +	if (ret)
> +		dev_err(qproc->dev,
> +			"%s: failed for %pa address of size %zx\n",
> +			__func__, &addr, size);
> +	return ret;
> +}
> +
[...]
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 461b387..9459894 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -76,3 +76,11 @@ config QCOM_WCNSS_CTRL
>  	help
>  	  Client driver for the WCNSS_CTRL SMD channel, used to download nv
>  	  firmware to a newly booted WCNSS chip.
> +
> +config QCOM_SECURE_BUFFER

Please no. Just fold this into the qcom_scm.c code and drop the
new config and new file.

> +	bool "Helper functions for securing buffers through TZ"
> +	help
> +	 Say 'Y' here for targets that need to call into TZ to secure
> +	 memory buffers. This ensures that only the correct clients can
> +	 use this memory and no unauthorized access is made to the
> +	 buffer
> diff --git a/drivers/soc/qcom/secure_buffer.c b/drivers/soc/qcom/secure_buffer.c
> new file mode 100644
> index 0000000..54eaa6f
> --- /dev/null
> +++ b/drivers/soc/qcom/secure_buffer.c
> @@ -0,0 +1,229 @@
> +/*
> + * Copyright (c) 2011-2017, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/highmem.h>

Used?

> +#include <linux/kernel.h>
> +#include <linux/kref.h>

Used?

> +#include <linux/mutex.h>
> +#include <linux/scatterlist.h>
> +#include <linux/slab.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/qcom_scm.h>
> +#include <linux/dma-mapping.h>
> +#include <soc/qcom/secure_buffer.h>
> +
> +DEFINE_MUTEX(secure_buffer_mutex);

static?

> +
> +struct scm_desc {
> +	u32 arginfo;
> +	u64 args[10];
> +};

This would be in qcom_scm-64.c already.

> +
> +struct mem_prot_info {
> +	phys_addr_t addr;
> +	u64 size;

Both should be __le64 presumably.

> +};
> +
> +struct info_list {
> +	struct mem_prot_info *list_head;
> +	u64 list_size;
> +};
> +
> +struct dest_vm_and_perm_info {
> +	u32 vm;
> +	u32 perm;
> +	u32 *ctx;

Why is this a pointer? Is this structure put into memory and
passed to the firmware? We should use the appropriate __le32 and
__le64 types then.

> +	u32 ctx_size;
> +};
> +
> +struct dest_info_list {
> +	struct dest_vm_and_perm_info *dest_info;
> +	u64 list_size;
> +};

These structures are confusing. From what I can tell we need an
array of struct mem_prot_info and an array of struct
dest_vm_and_perm_info in our pre-allocated buffer. The other
info_list and dest_info_list structures are bookkeeping things
that we pass to the firmware via registers. So let's drop the
info_list structures and have the functions that populate arrays
take pointers to the memory region to populate and return size_t?

static size_t populate_dest_info(struct dest_vm_and_perm_info *info,
				  struct int *dest_vmids, int nelements,
				  int *dest_perms);

static size_t populate_prot_info_from_table(struct mem_prot_info *info,
					    struct sg_table *table);

And then the callers can add the size returned to the memory
chunk pointer.

	void *qcom_secure_mem;
	struct mem_prot_info *prot_info = qcom_secure_mem;
	struct dest_vm_and_perm_info *dest_info;
	size_t size;

	size = populate_prot_info_from_table(prot_info, ...);
	...
	dest_info = qcom_secure_mem + size;
	size = populate_dest_info(dest_info, ...);

	smc_call()...

}
> +
> +static void *qcom_secure_mem;
> +#define QCOM_SECURE_MEM_SIZE (512 * 1024)
> +#define PADDING 32

What is the padding for? cache aligning? 32 seems magical.

> +
> +static void populate_dest_info(int *dest_vmids, int nelements,
> +			int *dest_perms, struct dest_info_list **list,

s/list/list_p/

> +			void *current_qcom_secure_mem)
> +{
> +	struct dest_vm_and_perm_info *dest_info;
> +	int i;
> +
> +	dest_info = (struct dest_vm_and_perm_info *)current_qcom_secure_mem;

Useless cast, please remove.

> +
> +	for (i = 0; i < nelements; i++) {
> +		dest_info[i].vm = dest_vmids[i];
> +		dest_info[i].perm = dest_perms[i];
> +		dest_info[i].ctx = NULL;
> +		dest_info[i].ctx_size = 0;
> +	}
> +
> +	*list = (struct dest_info_list *)&dest_info[i];

Grow a local variable:

	struct dest_info_list *list = *list_p;

	list = &dest_info[i];
	list->dest_info = dest_info;
	list->list_size = nelements * sizeof(*dest_info);

Of course this may all change anyway.

> +
> +	(*list)->dest_info = dest_info;
> +	(*list)->list_size = nelements * sizeof(struct dest_vm_and_perm_info);
> +}
> +
> +static void get_info_list_from_table(struct sg_table *table,
> +					struct info_list **list)
> +{
> +	int i;
> +	struct scatterlist *sg;
> +	struct mem_prot_info *info;
> +
> +	info = (struct mem_prot_info *)qcom_secure_mem;

Useless cast from void.

> +
> +	for_each_sg(table->sgl, sg, table->nents, i) {
> +		info[i].addr = page_to_phys(sg_page(sg));
> +		info[i].size = sg->length;
> +	}
> +
> +	*list = (struct info_list *)&(info[i]);
> +
> +	(*list)->list_head = info;
> +	(*list)->list_size = table->nents * sizeof(struct mem_prot_info);
> +}
> +
> +int hyp_assign_table(struct device *dev, struct sg_table *table,
> +			u32 *source_vm_list, int source_nelems,
> +			int *dest_vmids, int *dest_perms,
> +			int dest_nelems)
> +{
> +	int ret;
> +	struct info_list *info_list = NULL;
> +	struct dest_info_list *dest_info_list = NULL;
> +	struct scm_desc desc = {0};
> +	u32 *source_vm_copy;
> +	void *current_qcom_secure_mem;
> +
> +	size_t reqd_size = dest_nelems * sizeof(struct dest_vm_and_perm_info) +
> +			table->nents * sizeof(struct mem_prot_info) +
> +			sizeof(dest_info_list) + sizeof(info_list) + PADDING;
> +
> +	if (!qcom_secure_mem) {
> +		pr_err("%s is not functional as qcom_secure_mem is not allocated.\n",
> +				__func__);
> +		return -ENOMEM;
> +	}
> +
> +	if (reqd_size > QCOM_SECURE_MEM_SIZE) {
> +		pr_err("%s: Not enough memory allocated. Required size %zd\n",
> +				__func__, reqd_size);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * We can only pass cache-aligned sizes to hypervisor, so we need
> +	 * to kmalloc and memcpy the source_vm_list here.
> +	 */
> +	source_vm_copy = kmalloc_array(
> +		source_nelems, sizeof(*source_vm_copy), GFP_KERNEL);
> +	if (!source_vm_copy)
> +		return -ENOMEM;
> +	memcpy(source_vm_copy, source_vm_list,
> +	       sizeof(*source_vm_list) * source_nelems);

All of these u32s need an endian swap on big-endian platforms
when they're copied.

> +
> +	mutex_lock(&secure_buffer_mutex);
> +
> +	get_info_list_from_table(table, &info_list);
> +
> +	current_qcom_secure_mem = &(info_list[1]);
> +	populate_dest_info(dest_vmids, dest_nelems, dest_perms,
> +				&dest_info_list, current_qcom_secure_mem);
> +
> +	desc.args[0] = virt_to_phys(info_list->list_head);
> +	desc.args[1] = info_list->list_size;
> +	desc.args[2] = virt_to_phys(source_vm_copy);
> +	desc.args[3] = sizeof(*source_vm_copy) * source_nelems;
> +	desc.args[4] = virt_to_phys(dest_info_list->dest_info);
> +	desc.args[5] = dest_info_list->list_size;
> +	desc.args[6] = 0;
> +
> +	dma_set_mask(dev, DMA_BIT_MASK(64));

The dev passed here should be the scm->dev because we don't want
to allocate memory from a memory region that may be associated
with some random device using this API, and also we want to be
able to have the right mask set for communicating with the
firmware, which may be different than whatever mask is needed for
DMA with a device.

> +	dma_map_single(dev, (void *)source_vm_copy,

Useless cast to void.

> +			(size_t)(source_vm_copy + source_nelems),

This looks wrong? source_vm_copy is a pointer and we're adding
source_nelems and then casting that address to a size_t which
would be potentially very large? Shouldn't it just be
desc.args[3]?

> +			DMA_TO_DEVICE);
> +	dma_map_single(dev, (void *)info_list->list_head,
> +		(size_t)(info_list->list_head +
> +		(info_list->list_size / sizeof(*info_list->list_head))),
> +		DMA_TO_DEVICE);
> +	dma_map_single(dev,
> +		(void *)dest_info_list->dest_info,

Useless cast to void.

> +		(size_t)(dest_info_list->dest_info +
> +		(dest_info_list->list_size /
> +		sizeof(*dest_info_list->dest_info))),
> +		DMA_TO_DEVICE);
> +
> +	ret = qcom_scm_assign_mem((void *)&desc);

Useless cast to void.

> +	if (ret)
> +		pr_info("%s: Failed to assign memory protection, ret = %d\n",

pr_err?

> +			__func__, ret);
> +
> +	mutex_unlock(&secure_buffer_mutex);

Missing the dma_unmap calls here? Failure to do that could lead
to a leak if we bounce the page.

Also, shouldn't we skip the dma mapping if we have allocated
coherent memory instead of kmalloc for the qcom_secure_mem
buffer? The code seems to ignore the dma allocation case.

> +	kfree(source_vm_copy);
> +	return ret;
> +}
> +
> +int hyp_assign_phys(struct device *dev, phys_addr_t addr, u64 size,
> +			u32 *source_vm_list, int source_nelems,
> +			int *dest_vmids, int *dest_perms,
> +			int dest_nelems)
> +{
> +	struct sg_table *table;
> +	int ret;
> +
> +	table = kzalloc(sizeof(struct sg_table), GFP_KERNEL);

sizeof(*table)

> +	if (!table)
> +		return -ENOMEM;

Newline here

> +	ret = sg_alloc_table(table, 1, GFP_KERNEL);
> +	if (ret)
> +		goto err1;
> +
> +	sg_set_page(table->sgl, phys_to_page(addr), size, 0);
> +
> +	ret = hyp_assign_table(dev, table, source_vm_list,
> +				source_nelems, dest_vmids,
> +				dest_perms, dest_nelems);

I'd prefer two user facing APIs exist. One that takes a single
address and size argument, and one that takes an sg_table. Both
APIs can then call some common function that passes that info off
to the firmware, but the first one can be used here without
requiring us to make an sg_table for no reason besides undoing the
sg_table in hyp_assign_table().

> +	if (ret)
> +		goto err2;
> +
> +	return ret;
> +err2:
> +	sg_free_table(table);
> +err1:
> +	kfree(table);
> +	return ret;
> +}
> +
> +static int __init alloc_secure_shared_memory(void)
> +{
> +	int ret = 0;

ret is 0...

> +
> +	qcom_secure_mem = kzalloc(QCOM_SECURE_MEM_SIZE, GFP_KERNEL);
> +	if (!qcom_secure_mem) {
> +		/* Fallback to CMA-DMA memory */
> +		qcom_secure_mem = dma_alloc_coherent(NULL, QCOM_SECURE_MEM_SIZE,

We can't really pass NULL here anymore. Use the scm device.

> +						NULL, GFP_KERNEL);
> +		if (!qcom_secure_mem) {
> +			pr_err("Couldn't allocate memory for secure use-cases. hyp_assign_table will not work\n");

Memory allocation messages are practically useless. Please remove
them.

> +			return -ENOMEM;
> +		}
> +	}
> +
> +	return ret;

And that's all for ret. Always 0.

> +}
> +pure_initcall(alloc_secure_shared_memory);

pure initcall? Maybe we should take the lazy approach and
allocate this big chunk once someone calls into the API the first
time? If we merge this with scm device, then we can probably do
the allocation when scm probes and we have a proper device for
dma allocation.

> diff --git a/include/soc/qcom/secure_buffer.h b/include/soc/qcom/secure_buffer.h
> new file mode 100644
> index 0000000..2c7015d
> --- /dev/null
> +++ b/include/soc/qcom/secure_buffer.h
> @@ -0,0 +1,51 @@
> +/*
> + * Copyright (c) 2015-2016, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#ifndef __MSM_SECURE_BUFFER_H__
> +#define __MSM_SECURE_BUFFER_H__
> +
> +enum vmid {
> +	VMID_HLOS = 0x3,
> +	VMID_MSS_MSA = 0xF,
> +	VMID_INVAL = -1

Just drop the enum and use #defines please.

> +};
> +
> +#define PERM_READ			0x4
> +#define PERM_WRITE			0x2
> +#define PERM_EXEC			0x1

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC ADD HYPERVISOR CALL SUPPORT 1/2] soc: qcom: Add hypervisor stage two translation request support.

[Dwivedi, Avaneesh Kumar (avani)]

On 1/21/2017 2:39 PM, Stephen Boyd wrote:
> On 01/09, Avaneesh Kumar Dwivedi wrote:
>> This patch add hypervisor support for mss bring up on msm8996.
>> MSS rproc driver make hypervisor request to add certain region
>> in IPA table owned by hepervisor and assign access permission
> Please drop the use of IPA here. There's an IPA acronym for the
> IP accelerator and that is confused with Intermediate Physical
> Address. Instead, say something like "in the stage 2 page tables
> of the SMMU". Also hypervisor is misspelled here.
OK.
>
>> to modem. These regions are used to load MBA, MDT, FW into DDR.
>>
>> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
>> ---
>>   drivers/firmware/qcom_scm-64.c     |  16 +++
>>   drivers/firmware/qcom_scm.c        |  13 +++
>>   drivers/firmware/qcom_scm.h        |  10 ++
>>   drivers/remoteproc/qcom_q6v5_pil.c |  96 +++++++++++++++-
> Please split the remoteproc code off from this patch.
Ok.
>
>>   drivers/soc/qcom/Kconfig           |   8 ++
>>   drivers/soc/qcom/Makefile          |   1 +
>>   drivers/soc/qcom/secure_buffer.c   | 229 +++++++++++++++++++++++++++++++++++++
>>   include/linux/qcom_scm.h           |   1 +
>>   include/soc/qcom/secure_buffer.h   |  51 +++++++++
>>   9 files changed, 419 insertions(+), 6 deletions(-)
>>   create mode 100644 drivers/soc/qcom/secure_buffer.c
>>   create mode 100644 include/soc/qcom/secure_buffer.h
>>
>> diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
>> index 4a0f5ea..63b814c 100644
>> --- a/drivers/firmware/qcom_scm-64.c
>> +++ b/drivers/firmware/qcom_scm-64.c
>> @@ -358,3 +358,19 @@ int __qcom_scm_pas_mss_reset(struct device *dev, bool reset)
>>   
>>   	return ret ? : res.a1;
>>   }
>> +
>> +int __qcom_scm_assign_mem(struct device *dev, void *data)
>> +{
>> +	int ret;
>> +	struct qcom_scm_desc *desc = (struct qcom_scm_desc *)data;
> Useless cast from void.
Yes, will correct.
>> +	struct arm_smccc_res res;
>> +
>> +	desc->arginfo = QCOM_SCM_ARGS(7, SCM_RO, SCM_VAL, SCM_RO,
>> +					SCM_VAL, SCM_RO, SCM_VAL, SCM_VAL);
>> +
>> +	ret = qcom_scm_call(dev, QCOM_SCM_SVC_MP,
>> +				QCOM_MEM_PROT_ASSIGN_ID,
>> +				desc, &res);
>> +
>> +	return ret ? : res.a1;
>> +}
>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
>> index 893f953ea..52394ac 100644
>> --- a/drivers/firmware/qcom_scm.c
>> +++ b/drivers/firmware/qcom_scm.c
>> @@ -292,6 +292,19 @@ int qcom_scm_pas_shutdown(u32 peripheral)
>>   }
>>   EXPORT_SYMBOL(qcom_scm_pas_shutdown);
>>   
>> +/**
>> + * qcom_scm_assign_mem() -
> Some words on what the function does?
OK.
>
>> + * @desc:	descriptor to send to hypervisor
>> + *
>> + * Return 0 on success.
>> + */
>> +int qcom_scm_assign_mem(void *desc)
>> +{
>> +
> Nitpick: Drop the extra newline
OK.
>
>> +	return __qcom_scm_assign_mem(__scm->dev, desc);
>> +}
>> +EXPORT_SYMBOL(qcom_scm_assign_mem);
>> +
>>   static int qcom_scm_pas_reset_assert(struct reset_controller_dev *rcdev,
>>   				     unsigned long idx)
>>   {
>> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
>> index 3584b00..f248dc9 100644
>> --- a/drivers/firmware/qcom_scm.h
>> +++ b/drivers/firmware/qcom_scm.h
>> @@ -47,6 +47,8 @@ extern int __qcom_scm_hdcp_req(struct device *dev,
>>   #define QCOM_SCM_PAS_SHUTDOWN_CMD	0x6
>>   #define QCOM_SCM_PAS_IS_SUPPORTED_CMD	0x7
>>   #define QCOM_SCM_PAS_MSS_RESET		0xa
>> +#define QCOM_SCM_SVC_MP				0xc
>> +#define QCOM_MEM_PROT_ASSIGN_ID		0x16
>>   extern bool __qcom_scm_pas_supported(struct device *dev, u32 peripheral);
>>   extern int  __qcom_scm_pas_init_image(struct device *dev, u32 peripheral,
>>   		dma_addr_t metadata_phys);
>> @@ -55,6 +57,7 @@ extern int  __qcom_scm_pas_mem_setup(struct device *dev, u32 peripheral,
>>   extern int  __qcom_scm_pas_auth_and_reset(struct device *dev, u32 peripheral);
>>   extern int  __qcom_scm_pas_shutdown(struct device *dev, u32 peripheral);
>>   extern int  __qcom_scm_pas_mss_reset(struct device *dev, bool reset);
>> +extern int  __qcom_scm_assign_mem(struct device *dev, void *desc);
>>   
>>   /* common error codes */
>>   #define QCOM_SCM_V2_EBUSY	-12
>> @@ -83,4 +86,11 @@ static inline int qcom_scm_remap_error(int err)
>>   	return -EINVAL;
>>   }
>>   
>> +enum scm_arg_types {
>> +	SCM_VAL,
>> +	SCM_RO,
>> +	SCM_RW,
>> +	SCM_BUFVAL,
>> +};
> We already have this enum? It's just prepended with QCOM_
> instead.
I think i missed somehow, will see and correct.
>
>> +
>>   #endif
>> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
>> index e5edefa..cbf833c 100644
>> --- a/drivers/remoteproc/qcom_q6v5_pil.c
>> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
>> @@ -31,6 +31,7 @@
>>   #include <linux/soc/qcom/smem.h>
>>   #include <linux/soc/qcom/smem_state.h>
>>   #include <linux/of_device.h>
>> +#include <soc/qcom/secure_buffer.h>
>>   
>>   #include "remoteproc_internal.h"
>>   #include "qcom_mdt_loader.h"
>> @@ -105,12 +106,19 @@ struct qcom_mss_reg_res {
>>   	int uA;
>>   };
>>   
>> +struct src_dest_vmid {
>> +	int *srcVM;
>> +	int *destVM;
>> +	int *destVMperm;
> Why not have an array of these structures instead of a structure
> with three arrays inside? Can you map one source VM to multiple
> destination VMs?
Yes one to multiple or multiple to one mapping is possible, i.e. grant 
access from HLOS only to HLOS+MSA.
>
>> +};
>> +
>>   struct rproc_hexagon_res {
>>   	const char *hexagon_mba_image;
>>   	struct qcom_mss_reg_res proxy_supply[4];
>>   	struct qcom_mss_reg_res active_supply[2];
>>   	char **proxy_clk_names;
>>   	char **active_clk_names;
>> +	int version;
>>   };
>>   
>>   struct q6v5 {
>> @@ -127,6 +135,8 @@ struct q6v5 {
>>   
>>   	struct reset_control *mss_restart;
>>   
>> +	struct src_dest_vmid vmid_details;
>> +
>>   	struct qcom_smem_state *state;
>>   	unsigned stop_bit;
>>   
>> @@ -152,6 +162,13 @@ struct q6v5 {
>>   	phys_addr_t mpss_reloc;
>>   	void *mpss_region;
>>   	size_t mpss_size;
>> +	int version;
>> +};
>> +
>> +enum {
>> +	MSS_MSM8916,
>> +	MSS_MSM8974,
>> +	MSS_MSM8996,
>>   };
>>   
>>   static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,
>> @@ -273,13 +290,46 @@ static void q6v5_clk_disable(struct device *dev,
>>   		clk_disable_unprepare(clks[i]);
>>   }
>>   
>> +int hyp_mem_access(struct q6v5 *qproc, phys_addr_t addr, size_t size)
>> +{
>> +	int src_count = 0;
>> +	int dest_count = 0;
>> +	int ret;
>> +	int i;
>> +
>> +	if (qproc->version != MSS_MSM8996)
>> +		return 0;
>> +
>> +	for (i = 0; i < 2; i++) {
>> +		if (qproc->vmid_details.srcVM[i] != 0)
>> +		src_count++;
> Bad tabbing here.
Yes, will correct.
>
>> +		if (qproc->vmid_details.destVM[i] != 0)
>> +		dest_count++;
> And here.
>
> When is it ever == 0? The hardcoded 2 in the for loop is also
> suspect.
In downstream, VMID 0 is not assigned for any subsystem, my purpose here 
is to check if destination vmid is valid for some subsystem.
On hard coded value question, will try to define a macro which will 
indicate MAX number of source or destination subsys VMID. which in
this case is two i.e. HLOS and MODEM can be source or destination.

>> +	}
> Add newline
OK.
>
>> +	ret = hyp_assign_phys(qproc->dev, addr, size,
>> +			qproc->vmid_details.srcVM,
>> +			src_count, qproc->vmid_details.destVM,
>> +			qproc->vmid_details.destVMperm, dest_count);
> At the least this API could take a vmid_details structure instead
> of all these arguments:
>
> 	struct vm_perms {
> 		u32 vm;
> 		u32 perm;
> 	};
>
> 	struct vmid_details {
> 		u32 *src;
> 		size_t src_count;
> 		struct vm_perms *dest;
> 		size_t dest_count;
> 	};
OK, will try to correct.
>
>> +	if (ret)
>> +		dev_err(qproc->dev,
>> +			"%s: failed for %pa address of size %zx\n",
>> +			__func__, &addr, size);
>> +	return ret;
>> +}
>> +
> [...]
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index 461b387..9459894 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -76,3 +76,11 @@ config QCOM_WCNSS_CTRL
>>   	help
>>   	  Client driver for the WCNSS_CTRL SMD channel, used to download nv
>>   	  firmware to a newly booted WCNSS chip.
>> +
>> +config QCOM_SECURE_BUFFER
> Please no. Just fold this into the qcom_scm.c code and drop the
> new config and new file.
Will try to incorporate this functionality in qcom_scm related source files.
>
>> +	bool "Helper functions for securing buffers through TZ"
>> +	help
>> +	 Say 'Y' here for targets that need to call into TZ to secure
>> +	 memory buffers. This ensures that only the correct clients can
>> +	 use this memory and no unauthorized access is made to the
>> +	 buffer
>> diff --git a/drivers/soc/qcom/secure_buffer.c b/drivers/soc/qcom/secure_buffer.c
>> new file mode 100644
>> index 0000000..54eaa6f
>> --- /dev/null
>> +++ b/drivers/soc/qcom/secure_buffer.c
>> @@ -0,0 +1,229 @@
>> +/*
>> + * Copyright (c) 2011-2017, The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>> +
>> +#include <linux/highmem.h>
> Used?
I think i missed to remove these, will remove them.
>
>> +#include <linux/kernel.h>
>> +#include <linux/kref.h>
> Used?
Will check and remove.
>
>> +#include <linux/mutex.h>
>> +#include <linux/scatterlist.h>
>> +#include <linux/slab.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/qcom_scm.h>
>> +#include <linux/dma-mapping.h>
>> +#include <soc/qcom/secure_buffer.h>
>> +
>> +DEFINE_MUTEX(secure_buffer_mutex);
> static?
OK, will correct.
>
>> +
>> +struct scm_desc {
>> +	u32 arginfo;
>> +	u64 args[10];
>> +};
> This would be in qcom_scm-64.c already.
OK.
>
>> +
>> +struct mem_prot_info {
>> +	phys_addr_t addr;
>> +	u64 size;
> Both should be __le64 presumably.
Yes, it should be in basic data type, but then i took it from downstream 
which keep address and size detail in above format.
>> +};
>> +
>> +struct info_list {
>> +	struct mem_prot_info *list_head;
>> +	u64 list_size;
>> +};
>> +
>> +struct dest_vm_and_perm_info {
>> +	u32 vm;
>> +	u32 perm;
>> +	u32 *ctx;
> Why is this a pointer?
I had taken reference from 3.18 kernel based downstream source code, it 
is not used and initialized with NULL but that is how TZ accept parameters.
> Is this structure put into memory and
> passed to the firmware? We should use the appropriate __le32 and
> __le64 types then.
OK.
>
>> +	u32 ctx_size;
>> +};
>> +
>> +struct dest_info_list {
>> +	struct dest_vm_and_perm_info *dest_info;
>> +	u64 list_size;
>> +};
> These structures are confusing. From what I can tell we need an
> array of struct mem_prot_info and an array of struct
> dest_vm_and_perm_info in our pre-allocated buffer. The other
> info_list and dest_info_list structures are bookkeeping things
> that we pass to the firmware via registers. So let's drop the
> info_list structures and have the functions that populate arrays
> take pointers to the memory region to populate and return size_t?
>
> static size_t populate_dest_info(struct dest_vm_and_perm_info *info,
> 				  struct int *dest_vmids, int nelements,
> 				  int *dest_perms);
>
> static size_t populate_prot_info_from_table(struct mem_prot_info *info,
> 					    struct sg_table *table);
>
> And then the callers can add the size returned to the memory
> chunk pointer.
>
> 	void *qcom_secure_mem;
> 	struct mem_prot_info *prot_info = qcom_secure_mem;
> 	struct dest_vm_and_perm_info *dest_info;
> 	size_t size;
>
> 	size = populate_prot_info_from_table(prot_info, ...);
> 	...
> 	dest_info = qcom_secure_mem + size;
> 	size = populate_dest_info(dest_info, ...);
>
> 	smc_call()...
>
> }
OK, Will try to modify as above.
>> +
>> +static void *qcom_secure_mem;
>> +#define QCOM_SECURE_MEM_SIZE (512 * 1024)
>> +#define PADDING 32
> What is the padding for? cache aligning? 32 seems magical.
I have taken bare minimum version of this driver to make hyp_assign call 
for msm8996, this PADDING does not seem doing anything useful.
It is not related with cache alignment. its only purpose seems that size 
of qcom_secure_mem should be at least 32 byte more than size of all info 
to be passed.
>
>> +
>> +static void populate_dest_info(int *dest_vmids, int nelements,
>> +			int *dest_perms, struct dest_info_list **list,
> s/list/list_p/
Did not get what is ask here?
>
>> +			void *current_qcom_secure_mem)
>> +{
>> +	struct dest_vm_and_perm_info *dest_info;
>> +	int i;
>> +
>> +	dest_info = (struct dest_vm_and_perm_info *)current_qcom_secure_mem;
> Useless cast, please remove.
OK
>> +
>> +	for (i = 0; i < nelements; i++) {
>> +		dest_info[i].vm = dest_vmids[i];
>> +		dest_info[i].perm = dest_perms[i];
>> +		dest_info[i].ctx = NULL;
>> +		dest_info[i].ctx_size = 0;
>> +	}
>> +
>> +	*list = (struct dest_info_list *)&dest_info[i];
> Grow a local variable:
OK
>
> 	struct dest_info_list *list = *list_p;
>
> 	list = &dest_info[i];
> 	list->dest_info = dest_info;
> 	list->list_size = nelements * sizeof(*dest_info);
>
> Of course this may all change anyway.
OK.
>
>> +
>> +	(*list)->dest_info = dest_info;
>> +	(*list)->list_size = nelements * sizeof(struct dest_vm_and_perm_info);
>> +}
>> +
>> +static void get_info_list_from_table(struct sg_table *table,
>> +					struct info_list **list)
>> +{
>> +	int i;
>> +	struct scatterlist *sg;
>> +	struct mem_prot_info *info;
>> +
>> +	info = (struct mem_prot_info *)qcom_secure_mem;
> Useless cast from void.
OK
>
>> +
>> +	for_each_sg(table->sgl, sg, table->nents, i) {
>> +		info[i].addr = page_to_phys(sg_page(sg));
>> +		info[i].size = sg->length;
>> +	}
>> +
>> +	*list = (struct info_list *)&(info[i]);
>> +
>> +	(*list)->list_head = info;
>> +	(*list)->list_size = table->nents * sizeof(struct mem_prot_info);
>> +}
>> +
>> +int hyp_assign_table(struct device *dev, struct sg_table *table,
>> +			u32 *source_vm_list, int source_nelems,
>> +			int *dest_vmids, int *dest_perms,
>> +			int dest_nelems)
>> +{
>> +	int ret;
>> +	struct info_list *info_list = NULL;
>> +	struct dest_info_list *dest_info_list = NULL;
>> +	struct scm_desc desc = {0};
>> +	u32 *source_vm_copy;
>> +	void *current_qcom_secure_mem;
>> +
>> +	size_t reqd_size = dest_nelems * sizeof(struct dest_vm_and_perm_info) +
>> +			table->nents * sizeof(struct mem_prot_info) +
>> +			sizeof(dest_info_list) + sizeof(info_list) + PADDING;
>> +
>> +	if (!qcom_secure_mem) {
>> +		pr_err("%s is not functional as qcom_secure_mem is not allocated.\n",
>> +				__func__);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	if (reqd_size > QCOM_SECURE_MEM_SIZE) {
>> +		pr_err("%s: Not enough memory allocated. Required size %zd\n",
>> +				__func__, reqd_size);
>> +		return -EINVAL;
>> +	}
>> +
>> +	/*
>> +	 * We can only pass cache-aligned sizes to hypervisor, so we need
>> +	 * to kmalloc and memcpy the source_vm_list here.
>> +	 */
>> +	source_vm_copy = kmalloc_array(
>> +		source_nelems, sizeof(*source_vm_copy), GFP_KERNEL);
>> +	if (!source_vm_copy)
>> +		return -ENOMEM;
>> +	memcpy(source_vm_copy, source_vm_list,
>> +	       sizeof(*source_vm_list) * source_nelems);
> All of these u32s need an endian swap on big-endian platforms
> when they're copied.
OK.
>
>> +
>> +	mutex_lock(&secure_buffer_mutex);
>> +
>> +	get_info_list_from_table(table, &info_list);
>> +
>> +	current_qcom_secure_mem = &(info_list[1]);
>> +	populate_dest_info(dest_vmids, dest_nelems, dest_perms,
>> +				&dest_info_list, current_qcom_secure_mem);
>> +
>> +	desc.args[0] = virt_to_phys(info_list->list_head);
>> +	desc.args[1] = info_list->list_size;
>> +	desc.args[2] = virt_to_phys(source_vm_copy);
>> +	desc.args[3] = sizeof(*source_vm_copy) * source_nelems;
>> +	desc.args[4] = virt_to_phys(dest_info_list->dest_info);
>> +	desc.args[5] = dest_info_list->list_size;
>> +	desc.args[6] = 0;
>> +
>> +	dma_set_mask(dev, DMA_BIT_MASK(64));
> The dev passed here should be the scm->dev because we don't want
> to allocate memory from a memory region that may be associated
> with some random device using this API, and
OK.
> also we want to be
> able to have the right mask set for communicating with the
> firmware, which may be different than whatever mask is needed for
> DMA with a device.
dma_map_single need a non NULL dev pointer to set mask, which scm->dev 
will provide.
>> +	dma_map_single(dev, (void *)source_vm_copy,
> Useless cast to void.
OK.
>
>> +			(size_t)(source_vm_copy + source_nelems),
> This looks wrong? source_vm_copy is a pointer and we're adding
> source_nelems and then casting that address to a size_t which
> would be potentially very large? Shouldn't it just be
> desc.args[3]?
My Bad, yes will correct.
>
>> +			DMA_TO_DEVICE);
>> +	dma_map_single(dev, (void *)info_list->list_head,
>> +		(size_t)(info_list->list_head +
>> +		(info_list->list_size / sizeof(*info_list->list_head))),
>> +		DMA_TO_DEVICE);
>> +	dma_map_single(dev,
>> +		(void *)dest_info_list->dest_info,
> Useless cast to void.
OK.
>
>> +		(size_t)(dest_info_list->dest_info +
>> +		(dest_info_list->list_size /
>> +		sizeof(*dest_info_list->dest_info))),
>> +		DMA_TO_DEVICE);
>> +
>> +	ret = qcom_scm_assign_mem((void *)&desc);
> Useless cast to void.
OK.
>
>> +	if (ret)
>> +		pr_info("%s: Failed to assign memory protection, ret = %d\n",
> pr_err?
OK.
>
>> +			__func__, ret);
>> +
>> +	mutex_unlock(&secure_buffer_mutex);
> Missing the dma_unmap calls here? Failure to do that could lead
> to a leak if we bounce the page.
>
> Also, shouldn't we skip the dma mapping if we have allocated
> coherent memory instead of kmalloc for the qcom_secure_mem
> buffer? The code seems to ignore the dma allocation case.
problem with coherent memory allocation is uncertainty of getting such 
chunk or carving it out for always.
>
>> +	kfree(source_vm_copy);
>> +	return ret;
>> +}
>> +
>> +int hyp_assign_phys(struct device *dev, phys_addr_t addr, u64 size,
>> +			u32 *source_vm_list, int source_nelems,
>> +			int *dest_vmids, int *dest_perms,
>> +			int dest_nelems)
>> +{
>> +	struct sg_table *table;
>> +	int ret;
>> +
>> +	table = kzalloc(sizeof(struct sg_table), GFP_KERNEL);
> sizeof(*table)
OK.
>
>> +	if (!table)
>> +		return -ENOMEM;
> Newline here
OK.
>
>> +	ret = sg_alloc_table(table, 1, GFP_KERNEL);
>> +	if (ret)
>> +		goto err1;
>> +
>> +	sg_set_page(table->sgl, phys_to_page(addr), size, 0);
>> +
>> +	ret = hyp_assign_table(dev, table, source_vm_list,
>> +				source_nelems, dest_vmids,
>> +				dest_perms, dest_nelems);
> I'd prefer two user facing APIs exist. One that takes a single
> address and size argument, and one that takes an sg_table. Both
> APIs can then call some common function that passes that info off
> to the firmware, but the first one can be used here without
> requiring us to make an sg_table for no reason besides undoing the
> sg_table in hyp_assign_table().
if you are OK, i will use a private API which will do away requirement 
of allocating sg_table as well as doing dma_single_map()
i will directly pass src and dest VM info to TZ.

>
>> +	if (ret)
>> +		goto err2;
>> +
>> +	return ret;
>> +err2:
>> +	sg_free_table(table);
>> +err1:
>> +	kfree(table);
>> +	return ret;
>> +}
>> +
>> +static int __init alloc_secure_shared_memory(void)
>> +{
>> +	int ret = 0;
> ret is 0...
>
>> +
>> +	qcom_secure_mem = kzalloc(QCOM_SECURE_MEM_SIZE, GFP_KERNEL);
>> +	if (!qcom_secure_mem) {
>> +		/* Fallback to CMA-DMA memory */
>> +		qcom_secure_mem = dma_alloc_coherent(NULL, QCOM_SECURE_MEM_SIZE,
> We can't really pass NULL here anymore. Use the scm device.
OK.
>
>> +						NULL, GFP_KERNEL);
>> +		if (!qcom_secure_mem) {
>> +			pr_err("Couldn't allocate memory for secure use-cases. hyp_assign_table will not work\n");
> Memory allocation messages are practically useless. Please remove
> them.
OK.
>> +			return -ENOMEM;
>> +		}
>> +	}
>> +
>> +	return ret;
> And that's all for ret. Always 0.
OK.
>
>> +}
>> +pure_initcall(alloc_secure_shared_memory);
> pure initcall? Maybe we should take the lazy approach and
> allocate this big chunk once someone calls into the API the first
> time? If we merge this with scm device, then we can probably do
> the allocation when scm probes and we have a proper device for
> dma allocation.
OK.
>
>> diff --git a/include/soc/qcom/secure_buffer.h b/include/soc/qcom/secure_buffer.h
>> new file mode 100644
>> index 0000000..2c7015d
>> --- /dev/null
>> +++ b/include/soc/qcom/secure_buffer.h
>> @@ -0,0 +1,51 @@
>> +/*
>> + * Copyright (c) 2015-2016, The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>> +
>> +#ifndef __MSM_SECURE_BUFFER_H__
>> +#define __MSM_SECURE_BUFFER_H__
>> +
>> +enum vmid {
>> +	VMID_HLOS = 0x3,
>> +	VMID_MSS_MSA = 0xF,
>> +	VMID_INVAL = -1
> Just drop the enum and use #defines please.
OK.
>
>> +};
>> +
>> +#define PERM_READ			0x4
>> +#define PERM_WRITE			0x2
>> +#define PERM_EXEC			0x1

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.