firmware: qcom_scm: Introduce batching of hyp assign calls

  Expose an API qcom_scm_assign_table to allow client drivers to batch
multiple memory regions in a single hyp assign call.

In the existing situation, if our goal is to process an sg_table and
transfer its ownership from the current VM to a different VM, we have a
couple of strategies. The first strategy involves processing the entire
sg_table at once and then transferring the ownership. However, this
method may have an adverse impact on the system because during an SMC
call, the NS interrupts are disabled, and this delay could be
significant when dealing with large sg_tables. To address this issue, we
can adopt a second strategy, which involves processing each sg_list in
the sg_table individually and reassigning memory ownership. Although
this method is slower and potentially impacts performance, it will not
keep the NS interrupts disabled for an extended period.

A more efficient strategy is to process the sg_table in batches. This
approach addresses both scenarios by involving memory processing in
batches, thus avoiding prolonged NS interrupt disablement for longer
duration when dealing with large sg_tables. Moreover, since we process
in batches, this method is faster compared to processing each item
individually. The observations on testing both the approaches for
performance is as follows:

Allocation Size/            256MB            512MB            1024MB
Algorithm Used           ===========      ===========      ============

Processing each sg_list   73708(us)        149289(us)       266964(us)
in sg_table one by one

Processing sg_table in    46925(us)         92691(us)       176893(us)
batches

This implementation serves as a wrapper around the helper function
__qcom_scm_assign_mem, which takes an sg_list and processes it in
batches. We’ve set the limit to a minimum of 32 sg_list in a batch or a
total batch size of 512 pages. The selection of these numbers is
heuristic, based on the test runs conducted. Opting for a smaller number
would compromise performance, while a larger number would result in
non-secure interrupts being disabled for an extended duration.

Co-developed-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>
Signed-off-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>
Signed-off-by: Dibakar Singh <quic_dibasing@quicinc.com>
---
 drivers/firmware/qcom/qcom_scm.c       | 211 +++++++++++++++++++++++++
 include/linux/firmware/qcom/qcom_scm.h |   7 +
 2 files changed, 218 insertions(+)
  

On Fri, Feb 09, 2024 at 04:55:36PM +0530, Dibakar Singh wrote:
> Expose an API qcom_scm_assign_table to allow client drivers to batch
> multiple memory regions in a single hyp assign call.
> 
> In the existing situation, if our goal is to process an sg_table and
> transfer its ownership from the current VM to a different VM, we have a
> couple of strategies. The first strategy involves processing the entire
> sg_table at once and then transferring the ownership. However, this
> method may have an adverse impact on the system because during an SMC
> call, the NS interrupts are disabled, and this delay could be
> significant when dealing with large sg_tables. To address this issue, we
> can adopt a second strategy, which involves processing each sg_list in
> the sg_table individually and reassigning memory ownership. Although
> this method is slower and potentially impacts performance, it will not
> keep the NS interrupts disabled for an extended period.
> 
> A more efficient strategy is to process the sg_table in batches. This
> approach addresses both scenarios by involving memory processing in
> batches, thus avoiding prolonged NS interrupt disablement for longer
> duration when dealing with large sg_tables. Moreover, since we process
> in batches, this method is faster compared to processing each item
> individually. The observations on testing both the approaches for
> performance is as follows:
> 
> Allocation Size/            256MB            512MB            1024MB
> Algorithm Used           ===========      ===========      ============
> 
> Processing each sg_list   73708(us)        149289(us)       266964(us)
> in sg_table one by one
> 
> Processing sg_table in    46925(us)         92691(us)       176893(us)
> batches
> 
> This implementation serves as a wrapper around the helper function
> __qcom_scm_assign_mem, which takes an sg_list and processes it in
> batches. We’ve set the limit to a minimum of 32 sg_list in a batch or a
> total batch size of 512 pages. The selection of these numbers is
> heuristic, based on the test runs conducted. Opting for a smaller number
> would compromise performance, while a larger number would result in
> non-secure interrupts being disabled for an extended duration.
>
> Co-developed-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>
> Signed-off-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>
> Signed-off-by: Dibakar Singh <quic_dibasing@quicinc.com>
> ---
>  drivers/firmware/qcom/qcom_scm.c       | 211 +++++++++++++++++++++++++
>  include/linux/firmware/qcom/qcom_scm.h |   7 +
>  2 files changed, 218 insertions(+)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 520de9b5633a..038b96503d65 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -21,6 +21,8 @@
>  #include <linux/platform_device.h>
>  #include <linux/reset-controller.h>
>  #include <linux/types.h>
> +#include <linux/scatterlist.h>
> +#include <linux/slab.h>
>  
>  #include "qcom_scm.h"
>  
> @@ -1048,6 +1050,215 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_assign_mem);
>  
> +/**
> + * qcom_scm_assign_mem_batch() - Make a secure call to reassign memory
> + *				   ownership of several memory regions
> + * @mem_regions:    A buffer describing the set of memory regions that need to
> + *		    be reassigned
> + * @nr_mem_regions: The number of memory regions that need to be reassigned
> + * @srcvms:	    A buffer populated with he vmid(s) for the current set of
> + *		    owners
> + * @src_sz:	    The size of the srcvms buffer (in bytes)
> + * @destvms:	    A buffer populated with the new owners and corresponding
> + *		    permission flags.
> + * @dest_sz:	    The size of the destvms buffer (in bytes)
> + *
> + * Return negative errno on failure, 0 on success.
> + */
> +static int qcom_scm_assign_mem_batch(struct qcom_scm_mem_map_info *mem_regions,
> +				     size_t nr_mem_regions, u32 *srcvms,
> +				     size_t src_sz,
> +				     struct qcom_scm_current_perm_info *destvms,
> +				     size_t dest_sz)
> +{
> +	dma_addr_t mem_dma_addr;
> +	size_t mem_regions_sz;
> +	int ret = 0, i;
> +
> +	for (i = 0; i < nr_mem_regions; i++) {
> +		mem_regions[i].mem_addr = cpu_to_le64(mem_regions[i].mem_addr);
> +		mem_regions[i].mem_size = cpu_to_le64(mem_regions[i].mem_size);
> +	}
> +
> +	mem_regions_sz = nr_mem_regions * sizeof(*mem_regions);
> +	mem_dma_addr = dma_map_single(__scm->dev, mem_regions, mem_regions_sz,
> +				      DMA_TO_DEVICE);
> +	if (dma_mapping_error(__scm->dev, mem_dma_addr)) {
> +		dev_err(__scm->dev, "mem_dma_addr mapping failed\n");
> +		return -ENOMEM;
> +	}
> +
> +	ret = __qcom_scm_assign_mem(__scm->dev, virt_to_phys(mem_regions),
> +				    mem_regions_sz, virt_to_phys(srcvms), src_sz,
> +				    virt_to_phys(destvms), dest_sz);
> +
> +	dma_unmap_single(__scm->dev, mem_dma_addr, mem_regions_sz, DMA_TO_DEVICE);
> +	return ret;
> +}
> +
> +/**
> + * qcom_scm_prepare_mem_batch() - Prepare batches of memory regions
> + * @sg_table:       A scatter list whose memory needs to be reassigned
> + * @srcvms:	    A buffer populated with he vmid(s) for the current set of
> + *		    owners
> + * @nr_src:	    The number of the src_vms buffer
> + * @destvms:	    A buffer populated with he vmid(s) for the new owners
> + * @destvms_perms:  A buffer populated with the permission flags of new owners
> + * @nr_dest:	    The number of the destvms
> + * @last_sgl:	    Denotes to the last scatter list element. Used in case of rollback
> + * @roll_back:	    Identifies whether we are executing rollback in case of failure
> + *
> + * Return negative errno on failure, 0 on success.
> + */
> +static int qcom_scm_prepare_mem_batch(struct sg_table *table,
> +				      u32 *srcvms, int nr_src,
> +				      int *destvms, int *destvms_perms,
> +				      int nr_dest,
> +				      struct scatterlist *last_sgl, bool roll_back)
> +{
> +	struct qcom_scm_current_perm_info *destvms_cp;
> +	struct qcom_scm_mem_map_info *mem_regions_buf;
> +	struct scatterlist *curr_sgl = table->sgl;
> +	dma_addr_t source_dma_addr, dest_dma_addr;
> +	size_t batch_iterator;
> +	size_t batch_start = 0;
> +	size_t destvms_cp_sz;
> +	size_t srcvms_cp_sz;
> +	size_t batch_size;
> +	u32 *srcvms_cp;
> +	int ret = 0;
> +	int i;
> +
> +	if (!table || !table->sgl || !srcvms || !nr_src ||
> +	    !destvms || !destvms_perms || !nr_dest || !table->nents)
> +		return -EINVAL;
> +
> +	srcvms_cp_sz = sizeof(*srcvms_cp) * nr_src;
> +	srcvms_cp = kmemdup(srcvms, srcvms_cp_sz, GFP_KERNEL);
> +	if (!srcvms_cp)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < nr_src; i++)
> +		srcvms_cp[i] = cpu_to_le32(srcvms_cp[i]);
> +
> +	source_dma_addr = dma_map_single(__scm->dev, srcvms_cp,
> +					 srcvms_cp_sz, DMA_TO_DEVICE);

Please use the new tzmem allocator:

https://lore.kernel.org/all/20240205182810.58382-1-brgl@bgdev.pl/

> +
> +	if (dma_mapping_error(__scm->dev, source_dma_addr)) {
> +		ret = -ENOMEM;
> +		goto out_free_source;
> +	}
> +
> +	destvms_cp_sz = sizeof(*destvms_cp) * nr_dest;
> +	destvms_cp = kzalloc(destvms_cp_sz, GFP_KERNEL);
> +
> +	if (!destvms_cp) {
> +		ret = -ENOMEM;
> +		goto out_unmap_source;
> +	}
> +
> +	for (i = 0; i < nr_dest; i++) {
> +		destvms_cp[i].vmid = cpu_to_le32(destvms[i]);
> +		destvms_cp[i].perm = cpu_to_le32(destvms_perms[i]);
> +		destvms_cp[i].ctx = 0;
> +		destvms_cp[i].ctx_size = 0;
> +	}
> +
> +	dest_dma_addr = dma_map_single(__scm->dev, destvms_cp,
> +				       destvms_cp_sz, DMA_TO_DEVICE);
> +	if (dma_mapping_error(__scm->dev, dest_dma_addr)) {
> +		ret = -ENOMEM;
> +		goto out_free_dest;
> +	}
> +
> +	mem_regions_buf = kcalloc(QCOM_SCM_MAX_BATCH_SECTION, sizeof(*mem_regions_buf),
> +				  GFP_KERNEL);
> +	if (!mem_regions_buf)
> +		return -ENOMEM;
> +
> +	while (batch_start < table->nents) {
> +		batch_size = 0;
> +		batch_iterator = 0;
> +
> +		do {
> +			mem_regions_buf[batch_iterator].mem_addr = page_to_phys(sg_page(curr_sgl));
> +			mem_regions_buf[batch_iterator].mem_size = curr_sgl->length;
> +			batch_size += curr_sgl->length;
> +			batch_iterator++;
> +			if (roll_back && curr_sgl == last_sgl)
> +				break;
> +			curr_sgl = sg_next(curr_sgl);
> +		} while (curr_sgl && batch_iterator < QCOM_SCM_MAX_BATCH_SECTION &&
> +				curr_sgl->length + batch_size < QCOM_SCM_MAX_BATCH_SIZE);
> +
> +		batch_start += batch_iterator;
> +
> +		ret = qcom_scm_assign_mem_batch(mem_regions_buf, batch_iterator,
> +						srcvms_cp, srcvms_cp_sz, destvms_cp, destvms_cp_sz);
> +
> +		if (ret) {
> +			dev_info(__scm->dev, "Failed to assign memory protection, ret = %d\n", ret);
> +			last_sgl = curr_sgl;
> +			ret = -EADDRNOTAVAIL;

Probably should not be changing the ret value.

What happens to the memory that has been assigned so far? How do we
reclaim that?

> +			break;
> +		}
> +		if (roll_back && curr_sgl == last_sgl)
> +			break;
> +	}
> +	kfree(mem_regions_buf);
> +
> +	dma_unmap_single(__scm->dev, dest_dma_addr,
> +			 destvms_cp_sz, DMA_TO_DEVICE);
> +
> +out_free_dest:
> +	kfree(destvms_cp);
> +out_unmap_source:
> +	dma_unmap_single(__scm->dev, source_dma_addr,
> +			 srcvms_cp_sz, DMA_TO_DEVICE);
> +out_free_source:
> +	kfree(srcvms_cp);
> +	return ret;
> +}
> +
> +/**
> + * qcom_scm_assign_table() - Make a call to prepare batches of memory regions
> + *			     and reassign memory ownership of several memory regions at once
> + * @sg_table:       A scatter list whose memory needs to be reassigned
> + * @srcvms:	    A buffer populated with he vmid(s) for the current set of
> + *		    owners
> + * @nr_src:	    The number of the src_vms buffer
> + * @destvms:	    A buffer populated with he vmid(s) for the new owners
> + * @destvms_perms:  A buffer populated with the permission flags of new owners
> + * @nr_dest:	    The number of the destvms
> + *
> + * Return negative errno on failure, 0 on success.
> + */
> +int qcom_scm_assign_table(struct sg_table *table,
> +			  u32 *srcvms, int nr_src,
> +			  int *destvms, int *destvms_perms,
> +			  int nr_dest)
> +{
> +	struct scatterlist *last_sgl = NULL;
> +	int rb_ret = 0;
> +	u32 new_dests;
> +	int new_perms;
> +	int ret = 0;
> +
> +	ret = qcom_scm_prepare_mem_batch(table, srcvms, nr_src,
> +					 destvms, destvms_perms, nr_dest, last_sgl, false);
> +
> +	if (!ret)
> +		goto out;
> +	new_dests = QCOM_SCM_VMID_HLOS;

We have the original srcvms. Will it always be HLOS? If so, then do we
need to pass srcvms at all?

> +	new_perms = QCOM_SCM_PERM_EXEC | QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_READ;
> +	rb_ret = qcom_scm_prepare_mem_batch(table, destvms, nr_dest, &new_dests,
> +					    &new_perms, nr_src, last_sgl, true);
> +	WARN_ON_ONCE(rb_ret);
> +out:
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(qcom_scm_assign_table);

Who uses this function?

> +
>  /**
>   * qcom_scm_ocmem_lock_available() - is OCMEM lock/unlock interface available
>   */
> diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
> index ccaf28846054..abd675c7ef49 100644
> --- a/include/linux/firmware/qcom/qcom_scm.h
> +++ b/include/linux/firmware/qcom/qcom_scm.h
> @@ -8,6 +8,7 @@
>  #include <linux/err.h>
>  #include <linux/types.h>
>  #include <linux/cpumask.h>
> +#include <linux/scatterlist.h>
>  
>  #include <dt-bindings/firmware/qcom,scm.h>
>  
> @@ -15,6 +16,8 @@
>  #define QCOM_SCM_CPU_PWR_DOWN_L2_ON	0x0
>  #define QCOM_SCM_CPU_PWR_DOWN_L2_OFF	0x1
>  #define QCOM_SCM_HDCP_MAX_REQ_CNT	5
> +#define QCOM_SCM_MAX_BATCH_SECTION	32
> +#define QCOM_SCM_MAX_BATCH_SIZE		SZ_2M
>  
>  struct qcom_scm_hdcp_req {
>  	u32 addr;
> @@ -93,6 +96,10 @@ int qcom_scm_mem_protect_video_var(u32 cp_start, u32 cp_size,
>  int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, u64 *src,
>  			const struct qcom_scm_vmperm *newvm,
>  			unsigned int dest_cnt);
> +int qcom_scm_assign_table(struct sg_table *table,
> +			  u32 *srcvms, int nr_src,
> +			  int *destvms, int *destvms_perms,
> +			  int nr_dest);
>  
>  bool qcom_scm_ocmem_lock_available(void);
>  int qcom_scm_ocmem_lock(enum qcom_scm_ocmem_client id, u32 offset, u32 size,
> -- 
> 2.34.1
>
  

On 9.02.2024 19:06, Elliot Berman wrote:
> On Fri, Feb 09, 2024 at 04:55:36PM +0530, Dibakar Singh wrote:
>> Expose an API qcom_scm_assign_table to allow client drivers to batch
>> multiple memory regions in a single hyp assign call.
>>
>> In the existing situation, if our goal is to process an sg_table and
>> transfer its ownership from the current VM to a different VM, we have a
>> couple of strategies. The first strategy involves processing the entire
>> sg_table at once and then transferring the ownership. However, this
>> method may have an adverse impact on the system because during an SMC
>> call, the NS interrupts are disabled, and this delay could be
>> significant when dealing with large sg_tables. To address this issue, we
>> can adopt a second strategy, which involves processing each sg_list in
>> the sg_table individually and reassigning memory ownership. Although
>> this method is slower and potentially impacts performance, it will not
>> keep the NS interrupts disabled for an extended period.
>>
>> A more efficient strategy is to process the sg_table in batches. This
>> approach addresses both scenarios by involving memory processing in
>> batches, thus avoiding prolonged NS interrupt disablement for longer
>> duration when dealing with large sg_tables. Moreover, since we process
>> in batches, this method is faster compared to processing each item
>> individually. The observations on testing both the approaches for
>> performance is as follows:
>>
>> Allocation Size/            256MB            512MB            1024MB
>> Algorithm Used           ===========      ===========      ============
>>
>> Processing each sg_list   73708(us)        149289(us)       266964(us)
>> in sg_table one by one
>>
>> Processing sg_table in    46925(us)         92691(us)       176893(us)
>> batches
>>
>> This implementation serves as a wrapper around the helper function
>> __qcom_scm_assign_mem, which takes an sg_list and processes it in
>> batches. We’ve set the limit to a minimum of 32 sg_list in a batch or a
>> total batch size of 512 pages. The selection of these numbers is
>> heuristic, based on the test runs conducted. Opting for a smaller number
>> would compromise performance, while a larger number would result in
>> non-secure interrupts being disabled for an extended duration.
>>
>> Co-developed-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>
>> Signed-off-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>
>> Signed-off-by: Dibakar Singh <quic_dibasing@quicinc.com>
>> ---
>>  drivers/firmware/qcom/qcom_scm.c       | 211 +++++++++++++++++++++++++
>>  include/linux/firmware/qcom/qcom_scm.h |   7 +
>>  2 files changed, 218 insertions(+)
>>
>> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
>> index 520de9b5633a..038b96503d65 100644
>> --- a/drivers/firmware/qcom/qcom_scm.c
>> +++ b/drivers/firmware/qcom/qcom_scm.c
>> @@ -21,6 +21,8 @@
>>  #include <linux/platform_device.h>
>>  #include <linux/reset-controller.h>
>>  #include <linux/types.h>
>> +#include <linux/scatterlist.h>
>> +#include <linux/slab.h>
>>  
>>  #include "qcom_scm.h"
>>  
>> @@ -1048,6 +1050,215 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
>>  }
>>  EXPORT_SYMBOL_GPL(qcom_scm_assign_mem);
>>  
>> +/**
>> + * qcom_scm_assign_mem_batch() - Make a secure call to reassign memory
>> + *				   ownership of several memory regions
>> + * @mem_regions:    A buffer describing the set of memory regions that need to
>> + *		    be reassigned
>> + * @nr_mem_regions: The number of memory regions that need to be reassigned
>> + * @srcvms:	    A buffer populated with he vmid(s) for the current set of
>> + *		    owners
>> + * @src_sz:	    The size of the srcvms buffer (in bytes)
>> + * @destvms:	    A buffer populated with the new owners and corresponding
>> + *		    permission flags.
>> + * @dest_sz:	    The size of the destvms buffer (in bytes)
>> + *
>> + * Return negative errno on failure, 0 on success.
>> + */
>> +static int qcom_scm_assign_mem_batch(struct qcom_scm_mem_map_info *mem_regions,
>> +				     size_t nr_mem_regions, u32 *srcvms,
>> +				     size_t src_sz,
>> +				     struct qcom_scm_current_perm_info *destvms,
>> +				     size_t dest_sz)
>> +{
>> +	dma_addr_t mem_dma_addr;
>> +	size_t mem_regions_sz;
>> +	int ret = 0, i;
>> +
>> +	for (i = 0; i < nr_mem_regions; i++) {
>> +		mem_regions[i].mem_addr = cpu_to_le64(mem_regions[i].mem_addr);
>> +		mem_regions[i].mem_size = cpu_to_le64(mem_regions[i].mem_size);
>> +	}
>> +
>> +	mem_regions_sz = nr_mem_regions * sizeof(*mem_regions);
>> +	mem_dma_addr = dma_map_single(__scm->dev, mem_regions, mem_regions_sz,
>> +				      DMA_TO_DEVICE);
>> +	if (dma_mapping_error(__scm->dev, mem_dma_addr)) {
>> +		dev_err(__scm->dev, "mem_dma_addr mapping failed\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	ret = __qcom_scm_assign_mem(__scm->dev, virt_to_phys(mem_regions),
>> +				    mem_regions_sz, virt_to_phys(srcvms), src_sz,
>> +				    virt_to_phys(destvms), dest_sz);
>> +
>> +	dma_unmap_single(__scm->dev, mem_dma_addr, mem_regions_sz, DMA_TO_DEVICE);
>> +	return ret;
>> +}
>> +
>> +/**
>> + * qcom_scm_prepare_mem_batch() - Prepare batches of memory regions
>> + * @sg_table:       A scatter list whose memory needs to be reassigned
>> + * @srcvms:	    A buffer populated with he vmid(s) for the current set of
>> + *		    owners
>> + * @nr_src:	    The number of the src_vms buffer
>> + * @destvms:	    A buffer populated with he vmid(s) for the new owners
>> + * @destvms_perms:  A buffer populated with the permission flags of new owners
>> + * @nr_dest:	    The number of the destvms
>> + * @last_sgl:	    Denotes to the last scatter list element. Used in case of rollback
>> + * @roll_back:	    Identifies whether we are executing rollback in case of failure
>> + *
>> + * Return negative errno on failure, 0 on success.
>> + */
>> +static int qcom_scm_prepare_mem_batch(struct sg_table *table,
>> +				      u32 *srcvms, int nr_src,
>> +				      int *destvms, int *destvms_perms,
>> +				      int nr_dest,
>> +				      struct scatterlist *last_sgl, bool roll_back)
>> +{
>> +	struct qcom_scm_current_perm_info *destvms_cp;
>> +	struct qcom_scm_mem_map_info *mem_regions_buf;
>> +	struct scatterlist *curr_sgl = table->sgl;
>> +	dma_addr_t source_dma_addr, dest_dma_addr;
>> +	size_t batch_iterator;
>> +	size_t batch_start = 0;
>> +	size_t destvms_cp_sz;
>> +	size_t srcvms_cp_sz;
>> +	size_t batch_size;
>> +	u32 *srcvms_cp;
>> +	int ret = 0;
>> +	int i;
>> +
>> +	if (!table || !table->sgl || !srcvms || !nr_src ||
>> +	    !destvms || !destvms_perms || !nr_dest || !table->nents)
>> +		return -EINVAL;
>> +
>> +	srcvms_cp_sz = sizeof(*srcvms_cp) * nr_src;
>> +	srcvms_cp = kmemdup(srcvms, srcvms_cp_sz, GFP_KERNEL);
>> +	if (!srcvms_cp)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < nr_src; i++)
>> +		srcvms_cp[i] = cpu_to_le32(srcvms_cp[i]);
>> +
>> +	source_dma_addr = dma_map_single(__scm->dev, srcvms_cp,
>> +					 srcvms_cp_sz, DMA_TO_DEVICE);
> 
> Please use the new tzmem allocator:
> 
> https://lore.kernel.org/all/20240205182810.58382-1-brgl@bgdev.pl/

And the new __free annotations, please

Konrad
  

On 09-Feb-24 11:36 PM, Elliot Berman wrote:
> On Fri, Feb 09, 2024 at 04:55:36PM +0530, Dibakar Singh wrote:
>> Expose an API qcom_scm_assign_table to allow client drivers to batch
>> multiple memory regions in a single hyp assign call.
>>
>> In the existing situation, if our goal is to process an sg_table and
>> transfer its ownership from the current VM to a different VM, we have a
>> couple of strategies. The first strategy involves processing the entire
>> sg_table at once and then transferring the ownership. However, this
>> method may have an adverse impact on the system because during an SMC
>> call, the NS interrupts are disabled, and this delay could be
>> significant when dealing with large sg_tables. To address this issue, we
>> can adopt a second strategy, which involves processing each sg_list in
>> the sg_table individually and reassigning memory ownership. Although
>> this method is slower and potentially impacts performance, it will not
>> keep the NS interrupts disabled for an extended period.
>>
>> A more efficient strategy is to process the sg_table in batches. This
>> approach addresses both scenarios by involving memory processing in
>> batches, thus avoiding prolonged NS interrupt disablement for longer
>> duration when dealing with large sg_tables. Moreover, since we process
>> in batches, this method is faster compared to processing each item
>> individually. The observations on testing both the approaches for
>> performance is as follows:
>>
>> Allocation Size/            256MB            512MB            1024MB
>> Algorithm Used           ===========      ===========      ============
>>
>> Processing each sg_list   73708(us)        149289(us)       266964(us)
>> in sg_table one by one
>>
>> Processing sg_table in    46925(us)         92691(us)       176893(us)
>> batches
>>
>> This implementation serves as a wrapper around the helper function
>> __qcom_scm_assign_mem, which takes an sg_list and processes it in
>> batches. We’ve set the limit to a minimum of 32 sg_list in a batch or a
>> total batch size of 512 pages. The selection of these numbers is
>> heuristic, based on the test runs conducted. Opting for a smaller number
>> would compromise performance, while a larger number would result in
>> non-secure interrupts being disabled for an extended duration.
>>
>> Co-developed-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>
>> Signed-off-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>
>> Signed-off-by: Dibakar Singh <quic_dibasing@quicinc.com>
>> ---
>>   drivers/firmware/qcom/qcom_scm.c       | 211 +++++++++++++++++++++++++
>>   include/linux/firmware/qcom/qcom_scm.h |   7 +
>>   2 files changed, 218 insertions(+)
>>
>> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
>> index 520de9b5633a..038b96503d65 100644
>> --- a/drivers/firmware/qcom/qcom_scm.c
>> +++ b/drivers/firmware/qcom/qcom_scm.c
>> @@ -21,6 +21,8 @@
>>   #include <linux/platform_device.h>
>>   #include <linux/reset-controller.h>
>>   #include <linux/types.h>
>> +#include <linux/scatterlist.h>
>> +#include <linux/slab.h>
>>   
>>   #include "qcom_scm.h"
>>   
>> @@ -1048,6 +1050,215 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
>>   }
>>   EXPORT_SYMBOL_GPL(qcom_scm_assign_mem);
>>   
>> +/**
>> + * qcom_scm_assign_mem_batch() - Make a secure call to reassign memory
>> + *				   ownership of several memory regions
>> + * @mem_regions:    A buffer describing the set of memory regions that need to
>> + *		    be reassigned
>> + * @nr_mem_regions: The number of memory regions that need to be reassigned
>> + * @srcvms:	    A buffer populated with he vmid(s) for the current set of
>> + *		    owners
>> + * @src_sz:	    The size of the srcvms buffer (in bytes)
>> + * @destvms:	    A buffer populated with the new owners and corresponding
>> + *		    permission flags.
>> + * @dest_sz:	    The size of the destvms buffer (in bytes)
>> + *
>> + * Return negative errno on failure, 0 on success.
>> + */
>> +static int qcom_scm_assign_mem_batch(struct qcom_scm_mem_map_info *mem_regions,
>> +				     size_t nr_mem_regions, u32 *srcvms,
>> +				     size_t src_sz,
>> +				     struct qcom_scm_current_perm_info *destvms,
>> +				     size_t dest_sz)
>> +{
>> +	dma_addr_t mem_dma_addr;
>> +	size_t mem_regions_sz;
>> +	int ret = 0, i;
>> +
>> +	for (i = 0; i < nr_mem_regions; i++) {
>> +		mem_regions[i].mem_addr = cpu_to_le64(mem_regions[i].mem_addr);
>> +		mem_regions[i].mem_size = cpu_to_le64(mem_regions[i].mem_size);
>> +	}
>> +
>> +	mem_regions_sz = nr_mem_regions * sizeof(*mem_regions);
>> +	mem_dma_addr = dma_map_single(__scm->dev, mem_regions, mem_regions_sz,
>> +				      DMA_TO_DEVICE);
>> +	if (dma_mapping_error(__scm->dev, mem_dma_addr)) {
>> +		dev_err(__scm->dev, "mem_dma_addr mapping failed\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	ret = __qcom_scm_assign_mem(__scm->dev, virt_to_phys(mem_regions),
>> +				    mem_regions_sz, virt_to_phys(srcvms), src_sz,
>> +				    virt_to_phys(destvms), dest_sz);
>> +
>> +	dma_unmap_single(__scm->dev, mem_dma_addr, mem_regions_sz, DMA_TO_DEVICE);
>> +	return ret;
>> +}
>> +
>> +/**
>> + * qcom_scm_prepare_mem_batch() - Prepare batches of memory regions
>> + * @sg_table:       A scatter list whose memory needs to be reassigned
>> + * @srcvms:	    A buffer populated with he vmid(s) for the current set of
>> + *		    owners
>> + * @nr_src:	    The number of the src_vms buffer
>> + * @destvms:	    A buffer populated with he vmid(s) for the new owners
>> + * @destvms_perms:  A buffer populated with the permission flags of new owners
>> + * @nr_dest:	    The number of the destvms
>> + * @last_sgl:	    Denotes to the last scatter list element. Used in case of rollback
>> + * @roll_back:	    Identifies whether we are executing rollback in case of failure
>> + *
>> + * Return negative errno on failure, 0 on success.
>> + */
>> +static int qcom_scm_prepare_mem_batch(struct sg_table *table,
>> +				      u32 *srcvms, int nr_src,
>> +				      int *destvms, int *destvms_perms,
>> +				      int nr_dest,
>> +				      struct scatterlist *last_sgl, bool roll_back)
>> +{
>> +	struct qcom_scm_current_perm_info *destvms_cp;
>> +	struct qcom_scm_mem_map_info *mem_regions_buf;
>> +	struct scatterlist *curr_sgl = table->sgl;
>> +	dma_addr_t source_dma_addr, dest_dma_addr;
>> +	size_t batch_iterator;
>> +	size_t batch_start = 0;
>> +	size_t destvms_cp_sz;
>> +	size_t srcvms_cp_sz;
>> +	size_t batch_size;
>> +	u32 *srcvms_cp;
>> +	int ret = 0;
>> +	int i;
>> +
>> +	if (!table || !table->sgl || !srcvms || !nr_src ||
>> +	    !destvms || !destvms_perms || !nr_dest || !table->nents)
>> +		return -EINVAL;
>> +
>> +	srcvms_cp_sz = sizeof(*srcvms_cp) * nr_src;
>> +	srcvms_cp = kmemdup(srcvms, srcvms_cp_sz, GFP_KERNEL);
>> +	if (!srcvms_cp)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < nr_src; i++)
>> +		srcvms_cp[i] = cpu_to_le32(srcvms_cp[i]);
>> +
>> +	source_dma_addr = dma_map_single(__scm->dev, srcvms_cp,
>> +					 srcvms_cp_sz, DMA_TO_DEVICE);
> 
> Please use the new tzmem allocator:
> 
> https://lore.kernel.org/all/20240205182810.58382-1-brgl@bgdev.pl/

Noted, I will use this allocator in V2 patch.

> 
>> +
>> +	if (dma_mapping_error(__scm->dev, source_dma_addr)) {
>> +		ret = -ENOMEM;
>> +		goto out_free_source;
>> +	}
>> +
>> +	destvms_cp_sz = sizeof(*destvms_cp) * nr_dest;
>> +	destvms_cp = kzalloc(destvms_cp_sz, GFP_KERNEL);
>> +
>> +	if (!destvms_cp) {
>> +		ret = -ENOMEM;
>> +		goto out_unmap_source;
>> +	}
>> +
>> +	for (i = 0; i < nr_dest; i++) {
>> +		destvms_cp[i].vmid = cpu_to_le32(destvms[i]);
>> +		destvms_cp[i].perm = cpu_to_le32(destvms_perms[i]);
>> +		destvms_cp[i].ctx = 0;
>> +		destvms_cp[i].ctx_size = 0;
>> +	}
>> +
>> +	dest_dma_addr = dma_map_single(__scm->dev, destvms_cp,
>> +				       destvms_cp_sz, DMA_TO_DEVICE);
>> +	if (dma_mapping_error(__scm->dev, dest_dma_addr)) {
>> +		ret = -ENOMEM;
>> +		goto out_free_dest;
>> +	}
>> +
>> +	mem_regions_buf = kcalloc(QCOM_SCM_MAX_BATCH_SECTION, sizeof(*mem_regions_buf),
>> +				  GFP_KERNEL);
>> +	if (!mem_regions_buf)
>> +		return -ENOMEM;
>> +
>> +	while (batch_start < table->nents) {
>> +		batch_size = 0;
>> +		batch_iterator = 0;
>> +
>> +		do {
>> +			mem_regions_buf[batch_iterator].mem_addr = page_to_phys(sg_page(curr_sgl));
>> +			mem_regions_buf[batch_iterator].mem_size = curr_sgl->length;
>> +			batch_size += curr_sgl->length;
>> +			batch_iterator++;
>> +			if (roll_back && curr_sgl == last_sgl)
>> +				break;
>> +			curr_sgl = sg_next(curr_sgl);
>> +		} while (curr_sgl && batch_iterator < QCOM_SCM_MAX_BATCH_SECTION &&
>> +				curr_sgl->length + batch_size < QCOM_SCM_MAX_BATCH_SIZE);
>> +
>> +		batch_start += batch_iterator;
>> +
>> +		ret = qcom_scm_assign_mem_batch(mem_regions_buf, batch_iterator,
>> +						srcvms_cp, srcvms_cp_sz, destvms_cp, destvms_cp_sz);
>> +
>> +		if (ret) {
>> +			dev_info(__scm->dev, "Failed to assign memory protection, ret = %d\n", ret);
>> +			last_sgl = curr_sgl;
>> +			ret = -EADDRNOTAVAIL;
> 
> Probably should not be changing the ret value.

We are adjusting the return value here because we have already printed 
the failed return value earlier. Our goal is to ensure that clients 
invoking this API consistently receive the same failed values, without 
needing to worry about the precise details of the failure.

> 
> What happens to the memory that has been assigned so far? How do we
> reclaim that?

We’ve already added rollback functionality in qcom_scm_assign_table that 
allows batches to be rolled back to the HLOS if there’s a failure.

> 
>> +			break;
>> +		}
>> +		if (roll_back && curr_sgl == last_sgl)
>> +			break;
>> +	}
>> +	kfree(mem_regions_buf);
>> +
>> +	dma_unmap_single(__scm->dev, dest_dma_addr,
>> +			 destvms_cp_sz, DMA_TO_DEVICE);
>> +
>> +out_free_dest:
>> +	kfree(destvms_cp);
>> +out_unmap_source:
>> +	dma_unmap_single(__scm->dev, source_dma_addr,
>> +			 srcvms_cp_sz, DMA_TO_DEVICE);
>> +out_free_source:
>> +	kfree(srcvms_cp);
>> +	return ret;
>> +}
>> +
>> +/**
>> + * qcom_scm_assign_table() - Make a call to prepare batches of memory regions
>> + *			     and reassign memory ownership of several memory regions at once
>> + * @sg_table:       A scatter list whose memory needs to be reassigned
>> + * @srcvms:	    A buffer populated with he vmid(s) for the current set of
>> + *		    owners
>> + * @nr_src:	    The number of the src_vms buffer
>> + * @destvms:	    A buffer populated with he vmid(s) for the new owners
>> + * @destvms_perms:  A buffer populated with the permission flags of new owners
>> + * @nr_dest:	    The number of the destvms
>> + *
>> + * Return negative errno on failure, 0 on success.
>> + */
>> +int qcom_scm_assign_table(struct sg_table *table,
>> +			  u32 *srcvms, int nr_src,
>> +			  int *destvms, int *destvms_perms,
>> +			  int nr_dest)
>> +{
>> +	struct scatterlist *last_sgl = NULL;
>> +	int rb_ret = 0;
>> +	u32 new_dests;
>> +	int new_perms;
>> +	int ret = 0;
>> +
>> +	ret = qcom_scm_prepare_mem_batch(table, srcvms, nr_src,
>> +					 destvms, destvms_perms, nr_dest, last_sgl, false);
>> +
>> +	if (!ret)
>> +		goto out;
>> +	new_dests = QCOM_SCM_VMID_HLOS;
> 
> We have the original srcvms. Will it always be HLOS? If so, then do we
> need to pass srcvms at all?

No, srcvms can be VMs other than HLOS. However, this API does not expect 
clients to provide srcvms permissions. As a result, when such 
permissions are absent, we proceed by assigning the previously processed 
batches to HLOS.

> 
>> +	new_perms = QCOM_SCM_PERM_EXEC | QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_READ;
>> +	rb_ret = qcom_scm_prepare_mem_batch(table, destvms, nr_dest, &new_dests,
>> +					    &new_perms, nr_src, last_sgl, true);
>> +	WARN_ON_ONCE(rb_ret);
>> +out:
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(qcom_scm_assign_table);
> 
> Who uses this function?

https://lore.kernel.org/all/cover.1700544802.git.quic_vjitta@quicinc.com/

This patch responsible for extending qcom secure heap support which will 
be the user of this API. In the current implementation, we use 
qcom_scm_assign_mem and iterate over each sg_list in the sg_table 
individually to secure the memory. Going forward, we will replace this 
approach with our API.

> 
>> +
>>   /**
>>    * qcom_scm_ocmem_lock_available() - is OCMEM lock/unlock interface available
>>    */
>> diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
>> index ccaf28846054..abd675c7ef49 100644
>> --- a/include/linux/firmware/qcom/qcom_scm.h
>> +++ b/include/linux/firmware/qcom/qcom_scm.h
>> @@ -8,6 +8,7 @@
>>   #include <linux/err.h>
>>   #include <linux/types.h>
>>   #include <linux/cpumask.h>
>> +#include <linux/scatterlist.h>
>>   
>>   #include <dt-bindings/firmware/qcom,scm.h>
>>   
>> @@ -15,6 +16,8 @@
>>   #define QCOM_SCM_CPU_PWR_DOWN_L2_ON	0x0
>>   #define QCOM_SCM_CPU_PWR_DOWN_L2_OFF	0x1
>>   #define QCOM_SCM_HDCP_MAX_REQ_CNT	5
>> +#define QCOM_SCM_MAX_BATCH_SECTION	32
>> +#define QCOM_SCM_MAX_BATCH_SIZE		SZ_2M
>>   
>>   struct qcom_scm_hdcp_req {
>>   	u32 addr;
>> @@ -93,6 +96,10 @@ int qcom_scm_mem_protect_video_var(u32 cp_start, u32 cp_size,
>>   int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, u64 *src,
>>   			const struct qcom_scm_vmperm *newvm,
>>   			unsigned int dest_cnt);
>> +int qcom_scm_assign_table(struct sg_table *table,
>> +			  u32 *srcvms, int nr_src,
>> +			  int *destvms, int *destvms_perms,
>> +			  int nr_dest);
>>   
>>   bool qcom_scm_ocmem_lock_available(void);
>>   int qcom_scm_ocmem_lock(enum qcom_scm_ocmem_client id, u32 offset, u32 size,
>> -- 
>> 2.34.1
>>
  

On 12-Feb-24 4:39 PM, Konrad Dybcio wrote:
> On 9.02.2024 19:06, Elliot Berman wrote:
>> On Fri, Feb 09, 2024 at 04:55:36PM +0530, Dibakar Singh wrote:
>>> Expose an API qcom_scm_assign_table to allow client drivers to batch
>>> multiple memory regions in a single hyp assign call.
>>>
>>> In the existing situation, if our goal is to process an sg_table and
>>> transfer its ownership from the current VM to a different VM, we have a
>>> couple of strategies. The first strategy involves processing the entire
>>> sg_table at once and then transferring the ownership. However, this
>>> method may have an adverse impact on the system because during an SMC
>>> call, the NS interrupts are disabled, and this delay could be
>>> significant when dealing with large sg_tables. To address this issue, we
>>> can adopt a second strategy, which involves processing each sg_list in
>>> the sg_table individually and reassigning memory ownership. Although
>>> this method is slower and potentially impacts performance, it will not
>>> keep the NS interrupts disabled for an extended period.
>>>
>>> A more efficient strategy is to process the sg_table in batches. This
>>> approach addresses both scenarios by involving memory processing in
>>> batches, thus avoiding prolonged NS interrupt disablement for longer
>>> duration when dealing with large sg_tables. Moreover, since we process
>>> in batches, this method is faster compared to processing each item
>>> individually. The observations on testing both the approaches for
>>> performance is as follows:
>>>
>>> Allocation Size/            256MB            512MB            1024MB
>>> Algorithm Used           ===========      ===========      ============
>>>
>>> Processing each sg_list   73708(us)        149289(us)       266964(us)
>>> in sg_table one by one
>>>
>>> Processing sg_table in    46925(us)         92691(us)       176893(us)
>>> batches
>>>
>>> This implementation serves as a wrapper around the helper function
>>> __qcom_scm_assign_mem, which takes an sg_list and processes it in
>>> batches. We’ve set the limit to a minimum of 32 sg_list in a batch or a
>>> total batch size of 512 pages. The selection of these numbers is
>>> heuristic, based on the test runs conducted. Opting for a smaller number
>>> would compromise performance, while a larger number would result in
>>> non-secure interrupts being disabled for an extended duration.
>>>
>>> Co-developed-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>
>>> Signed-off-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>
>>> Signed-off-by: Dibakar Singh <quic_dibasing@quicinc.com>
>>> ---
>>>   drivers/firmware/qcom/qcom_scm.c       | 211 +++++++++++++++++++++++++
>>>   include/linux/firmware/qcom/qcom_scm.h |   7 +
>>>   2 files changed, 218 insertions(+)
>>>
>>> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
>>> index 520de9b5633a..038b96503d65 100644
>>> --- a/drivers/firmware/qcom/qcom_scm.c
>>> +++ b/drivers/firmware/qcom/qcom_scm.c
>>> @@ -21,6 +21,8 @@
>>>   #include <linux/platform_device.h>
>>>   #include <linux/reset-controller.h>
>>>   #include <linux/types.h>
>>> +#include <linux/scatterlist.h>
>>> +#include <linux/slab.h>
>>>   
>>>   #include "qcom_scm.h"
>>>   
>>> @@ -1048,6 +1050,215 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
>>>   }
>>>   EXPORT_SYMBOL_GPL(qcom_scm_assign_mem);
>>>   
>>> +/**
>>> + * qcom_scm_assign_mem_batch() - Make a secure call to reassign memory
>>> + *				   ownership of several memory regions
>>> + * @mem_regions:    A buffer describing the set of memory regions that need to
>>> + *		    be reassigned
>>> + * @nr_mem_regions: The number of memory regions that need to be reassigned
>>> + * @srcvms:	    A buffer populated with he vmid(s) for the current set of
>>> + *		    owners
>>> + * @src_sz:	    The size of the srcvms buffer (in bytes)
>>> + * @destvms:	    A buffer populated with the new owners and corresponding
>>> + *		    permission flags.
>>> + * @dest_sz:	    The size of the destvms buffer (in bytes)
>>> + *
>>> + * Return negative errno on failure, 0 on success.
>>> + */
>>> +static int qcom_scm_assign_mem_batch(struct qcom_scm_mem_map_info *mem_regions,
>>> +				     size_t nr_mem_regions, u32 *srcvms,
>>> +				     size_t src_sz,
>>> +				     struct qcom_scm_current_perm_info *destvms,
>>> +				     size_t dest_sz)
>>> +{
>>> +	dma_addr_t mem_dma_addr;
>>> +	size_t mem_regions_sz;
>>> +	int ret = 0, i;
>>> +
>>> +	for (i = 0; i < nr_mem_regions; i++) {
>>> +		mem_regions[i].mem_addr = cpu_to_le64(mem_regions[i].mem_addr);
>>> +		mem_regions[i].mem_size = cpu_to_le64(mem_regions[i].mem_size);
>>> +	}
>>> +
>>> +	mem_regions_sz = nr_mem_regions * sizeof(*mem_regions);
>>> +	mem_dma_addr = dma_map_single(__scm->dev, mem_regions, mem_regions_sz,
>>> +				      DMA_TO_DEVICE);
>>> +	if (dma_mapping_error(__scm->dev, mem_dma_addr)) {
>>> +		dev_err(__scm->dev, "mem_dma_addr mapping failed\n");
>>> +		return -ENOMEM;
>>> +	}
>>> +
>>> +	ret = __qcom_scm_assign_mem(__scm->dev, virt_to_phys(mem_regions),
>>> +				    mem_regions_sz, virt_to_phys(srcvms), src_sz,
>>> +				    virt_to_phys(destvms), dest_sz);
>>> +
>>> +	dma_unmap_single(__scm->dev, mem_dma_addr, mem_regions_sz, DMA_TO_DEVICE);
>>> +	return ret;
>>> +}
>>> +
>>> +/**
>>> + * qcom_scm_prepare_mem_batch() - Prepare batches of memory regions
>>> + * @sg_table:       A scatter list whose memory needs to be reassigned
>>> + * @srcvms:	    A buffer populated with he vmid(s) for the current set of
>>> + *		    owners
>>> + * @nr_src:	    The number of the src_vms buffer
>>> + * @destvms:	    A buffer populated with he vmid(s) for the new owners
>>> + * @destvms_perms:  A buffer populated with the permission flags of new owners
>>> + * @nr_dest:	    The number of the destvms
>>> + * @last_sgl:	    Denotes to the last scatter list element. Used in case of rollback
>>> + * @roll_back:	    Identifies whether we are executing rollback in case of failure
>>> + *
>>> + * Return negative errno on failure, 0 on success.
>>> + */
>>> +static int qcom_scm_prepare_mem_batch(struct sg_table *table,
>>> +				      u32 *srcvms, int nr_src,
>>> +				      int *destvms, int *destvms_perms,
>>> +				      int nr_dest,
>>> +				      struct scatterlist *last_sgl, bool roll_back)
>>> +{
>>> +	struct qcom_scm_current_perm_info *destvms_cp;
>>> +	struct qcom_scm_mem_map_info *mem_regions_buf;
>>> +	struct scatterlist *curr_sgl = table->sgl;
>>> +	dma_addr_t source_dma_addr, dest_dma_addr;
>>> +	size_t batch_iterator;
>>> +	size_t batch_start = 0;
>>> +	size_t destvms_cp_sz;
>>> +	size_t srcvms_cp_sz;
>>> +	size_t batch_size;
>>> +	u32 *srcvms_cp;
>>> +	int ret = 0;
>>> +	int i;
>>> +
>>> +	if (!table || !table->sgl || !srcvms || !nr_src ||
>>> +	    !destvms || !destvms_perms || !nr_dest || !table->nents)
>>> +		return -EINVAL;
>>> +
>>> +	srcvms_cp_sz = sizeof(*srcvms_cp) * nr_src;
>>> +	srcvms_cp = kmemdup(srcvms, srcvms_cp_sz, GFP_KERNEL);
>>> +	if (!srcvms_cp)
>>> +		return -ENOMEM;
>>> +
>>> +	for (i = 0; i < nr_src; i++)
>>> +		srcvms_cp[i] = cpu_to_le32(srcvms_cp[i]);
>>> +
>>> +	source_dma_addr = dma_map_single(__scm->dev, srcvms_cp,
>>> +					 srcvms_cp_sz, DMA_TO_DEVICE);
>>
>> Please use the new tzmem allocator:
>>
>> https://lore.kernel.org/all/20240205182810.58382-1-brgl@bgdev.pl/
> 
> And the new __free annotations, please
> 

Noted, I will incorporate these changes in the V2 patch.

> Konrad

Thanks,
Dibakar
  

On Tue, Feb 20, 2024 at 11:24:33AM +0530, Dibakar Singh wrote:
> 
> 
> On 09-Feb-24 11:36 PM, Elliot Berman wrote:
> > On Fri, Feb 09, 2024 at 04:55:36PM +0530, Dibakar Singh wrote:
> > > Expose an API qcom_scm_assign_table to allow client drivers to batch
> > > multiple memory regions in a single hyp assign call.
> > > 
> > > In the existing situation, if our goal is to process an sg_table and
> > > transfer its ownership from the current VM to a different VM, we have a
> > > couple of strategies. The first strategy involves processing the entire
> > > sg_table at once and then transferring the ownership. However, this
> > > method may have an adverse impact on the system because during an SMC
> > > call, the NS interrupts are disabled, and this delay could be
> > > significant when dealing with large sg_tables. To address this issue, we
> > > can adopt a second strategy, which involves processing each sg_list in
> > > the sg_table individually and reassigning memory ownership. Although
> > > this method is slower and potentially impacts performance, it will not
> > > keep the NS interrupts disabled for an extended period.
> > > 
> > > A more efficient strategy is to process the sg_table in batches. This
> > > approach addresses both scenarios by involving memory processing in
> > > batches, thus avoiding prolonged NS interrupt disablement for longer
> > > duration when dealing with large sg_tables. Moreover, since we process
> > > in batches, this method is faster compared to processing each item
> > > individually. The observations on testing both the approaches for
> > > performance is as follows:
> > > 
> > > Allocation Size/            256MB            512MB            1024MB
> > > Algorithm Used           ===========      ===========      ============
> > > 
> > > Processing each sg_list   73708(us)        149289(us)       266964(us)
> > > in sg_table one by one
> > > 
> > > Processing sg_table in    46925(us)         92691(us)       176893(us)
> > > batches
> > > 
> > > This implementation serves as a wrapper around the helper function
> > > __qcom_scm_assign_mem, which takes an sg_list and processes it in
> > > batches. We’ve set the limit to a minimum of 32 sg_list in a batch or a
> > > total batch size of 512 pages. The selection of these numbers is
> > > heuristic, based on the test runs conducted. Opting for a smaller number
> > > would compromise performance, while a larger number would result in
> > > non-secure interrupts being disabled for an extended duration.
> > > 
> > > Co-developed-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>
> > > Signed-off-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>
> > > Signed-off-by: Dibakar Singh <quic_dibasing@quicinc.com>
> > > ---
> > >   drivers/firmware/qcom/qcom_scm.c       | 211 +++++++++++++++++++++++++
> > >   include/linux/firmware/qcom/qcom_scm.h |   7 +
> > >   2 files changed, 218 insertions(+)
> > > 
> > > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> > > index 520de9b5633a..038b96503d65 100644
> > > --- a/drivers/firmware/qcom/qcom_scm.c
> > > +++ b/drivers/firmware/qcom/qcom_scm.c
> > > @@ -21,6 +21,8 @@
> > >   #include <linux/platform_device.h>
> > >   #include <linux/reset-controller.h>
> > >   #include <linux/types.h>
> > > +#include <linux/scatterlist.h>
> > > +#include <linux/slab.h>
> > >   #include "qcom_scm.h"
> > > @@ -1048,6 +1050,215 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
> > >   }
> > >   EXPORT_SYMBOL_GPL(qcom_scm_assign_mem);
> > > +/**
> > > + * qcom_scm_assign_mem_batch() - Make a secure call to reassign memory
> > > + *				   ownership of several memory regions
> > > + * @mem_regions:    A buffer describing the set of memory regions that need to
> > > + *		    be reassigned
> > > + * @nr_mem_regions: The number of memory regions that need to be reassigned
> > > + * @srcvms:	    A buffer populated with he vmid(s) for the current set of
> > > + *		    owners
> > > + * @src_sz:	    The size of the srcvms buffer (in bytes)
> > > + * @destvms:	    A buffer populated with the new owners and corresponding
> > > + *		    permission flags.
> > > + * @dest_sz:	    The size of the destvms buffer (in bytes)
> > > + *
> > > + * Return negative errno on failure, 0 on success.
> > > + */
> > > +static int qcom_scm_assign_mem_batch(struct qcom_scm_mem_map_info *mem_regions,
> > > +				     size_t nr_mem_regions, u32 *srcvms,
> > > +				     size_t src_sz,
> > > +				     struct qcom_scm_current_perm_info *destvms,
> > > +				     size_t dest_sz)
> > > +{
> > > +	dma_addr_t mem_dma_addr;
> > > +	size_t mem_regions_sz;
> > > +	int ret = 0, i;
> > > +
> > > +	for (i = 0; i < nr_mem_regions; i++) {
> > > +		mem_regions[i].mem_addr = cpu_to_le64(mem_regions[i].mem_addr);
> > > +		mem_regions[i].mem_size = cpu_to_le64(mem_regions[i].mem_size);
> > > +	}
> > > +
> > > +	mem_regions_sz = nr_mem_regions * sizeof(*mem_regions);
> > > +	mem_dma_addr = dma_map_single(__scm->dev, mem_regions, mem_regions_sz,
> > > +				      DMA_TO_DEVICE);
> > > +	if (dma_mapping_error(__scm->dev, mem_dma_addr)) {
> > > +		dev_err(__scm->dev, "mem_dma_addr mapping failed\n");
> > > +		return -ENOMEM;
> > > +	}
> > > +
> > > +	ret = __qcom_scm_assign_mem(__scm->dev, virt_to_phys(mem_regions),
> > > +				    mem_regions_sz, virt_to_phys(srcvms), src_sz,
> > > +				    virt_to_phys(destvms), dest_sz);
> > > +
> > > +	dma_unmap_single(__scm->dev, mem_dma_addr, mem_regions_sz, DMA_TO_DEVICE);
> > > +	return ret;
> > > +}
> > > +
> > > +/**
> > > + * qcom_scm_prepare_mem_batch() - Prepare batches of memory regions
> > > + * @sg_table:       A scatter list whose memory needs to be reassigned
> > > + * @srcvms:	    A buffer populated with he vmid(s) for the current set of
> > > + *		    owners
> > > + * @nr_src:	    The number of the src_vms buffer
> > > + * @destvms:	    A buffer populated with he vmid(s) for the new owners
> > > + * @destvms_perms:  A buffer populated with the permission flags of new owners
> > > + * @nr_dest:	    The number of the destvms
> > > + * @last_sgl:	    Denotes to the last scatter list element. Used in case of rollback
> > > + * @roll_back:	    Identifies whether we are executing rollback in case of failure
> > > + *
> > > + * Return negative errno on failure, 0 on success.
> > > + */
> > > +static int qcom_scm_prepare_mem_batch(struct sg_table *table,
> > > +				      u32 *srcvms, int nr_src,
> > > +				      int *destvms, int *destvms_perms,
> > > +				      int nr_dest,
> > > +				      struct scatterlist *last_sgl, bool roll_back)
> > > +{
> > > +	struct qcom_scm_current_perm_info *destvms_cp;
> > > +	struct qcom_scm_mem_map_info *mem_regions_buf;
> > > +	struct scatterlist *curr_sgl = table->sgl;
> > > +	dma_addr_t source_dma_addr, dest_dma_addr;
> > > +	size_t batch_iterator;
> > > +	size_t batch_start = 0;
> > > +	size_t destvms_cp_sz;
> > > +	size_t srcvms_cp_sz;
> > > +	size_t batch_size;
> > > +	u32 *srcvms_cp;
> > > +	int ret = 0;
> > > +	int i;
> > > +
> > > +	if (!table || !table->sgl || !srcvms || !nr_src ||
> > > +	    !destvms || !destvms_perms || !nr_dest || !table->nents)
> > > +		return -EINVAL;
> > > +
> > > +	srcvms_cp_sz = sizeof(*srcvms_cp) * nr_src;
> > > +	srcvms_cp = kmemdup(srcvms, srcvms_cp_sz, GFP_KERNEL);
> > > +	if (!srcvms_cp)
> > > +		return -ENOMEM;
> > > +
> > > +	for (i = 0; i < nr_src; i++)
> > > +		srcvms_cp[i] = cpu_to_le32(srcvms_cp[i]);
> > > +
> > > +	source_dma_addr = dma_map_single(__scm->dev, srcvms_cp,
> > > +					 srcvms_cp_sz, DMA_TO_DEVICE);
> > 
> > Please use the new tzmem allocator:
> > 
> > https://lore.kernel.org/all/20240205182810.58382-1-brgl@bgdev.pl/
> 
> Noted, I will use this allocator in V2 patch.
> 
> > 
> > > +
> > > +	if (dma_mapping_error(__scm->dev, source_dma_addr)) {
> > > +		ret = -ENOMEM;
> > > +		goto out_free_source;
> > > +	}
> > > +
> > > +	destvms_cp_sz = sizeof(*destvms_cp) * nr_dest;
> > > +	destvms_cp = kzalloc(destvms_cp_sz, GFP_KERNEL);
> > > +
> > > +	if (!destvms_cp) {
> > > +		ret = -ENOMEM;
> > > +		goto out_unmap_source;
> > > +	}
> > > +
> > > +	for (i = 0; i < nr_dest; i++) {
> > > +		destvms_cp[i].vmid = cpu_to_le32(destvms[i]);
> > > +		destvms_cp[i].perm = cpu_to_le32(destvms_perms[i]);
> > > +		destvms_cp[i].ctx = 0;
> > > +		destvms_cp[i].ctx_size = 0;
> > > +	}
> > > +
> > > +	dest_dma_addr = dma_map_single(__scm->dev, destvms_cp,
> > > +				       destvms_cp_sz, DMA_TO_DEVICE);
> > > +	if (dma_mapping_error(__scm->dev, dest_dma_addr)) {
> > > +		ret = -ENOMEM;
> > > +		goto out_free_dest;
> > > +	}
> > > +
> > > +	mem_regions_buf = kcalloc(QCOM_SCM_MAX_BATCH_SECTION, sizeof(*mem_regions_buf),
> > > +				  GFP_KERNEL);
> > > +	if (!mem_regions_buf)
> > > +		return -ENOMEM;
> > > +
> > > +	while (batch_start < table->nents) {
> > > +		batch_size = 0;
> > > +		batch_iterator = 0;
> > > +
> > > +		do {
> > > +			mem_regions_buf[batch_iterator].mem_addr = page_to_phys(sg_page(curr_sgl));
> > > +			mem_regions_buf[batch_iterator].mem_size = curr_sgl->length;
> > > +			batch_size += curr_sgl->length;
> > > +			batch_iterator++;
> > > +			if (roll_back && curr_sgl == last_sgl)
> > > +				break;

Is roll_back bool necessary? last_sgl is NULL in case of not
rolling back and I think is good sentinel value when not rolling back.

If last_sgl is NULL, then all sgl entries are assigned. If last_sgl is
not NULL, then all the sgl entries are assigned until last_sgl is
encountered (inclusive).

> > > +			curr_sgl = sg_next(curr_sgl);
> > > +		} while (curr_sgl && batch_iterator < QCOM_SCM_MAX_BATCH_SECTION &&
> > > +				curr_sgl->length + batch_size < QCOM_SCM_MAX_BATCH_SIZE);
> > > +
> > > +		batch_start += batch_iterator;
> > > +
> > > +		ret = qcom_scm_assign_mem_batch(mem_regions_buf, batch_iterator,
> > > +						srcvms_cp, srcvms_cp_sz, destvms_cp, destvms_cp_sz);
> > > +
> > > +		if (ret) {
> > > +			dev_info(__scm->dev, "Failed to assign memory protection, ret = %d\n", ret);
> > > +			last_sgl = curr_sgl;
> > > +			ret = -EADDRNOTAVAIL;
> > 
> > Probably should not be changing the ret value.
> 
> We are adjusting the return value here because we have already printed the
> failed return value earlier. Our goal is to ensure that clients invoking
> this API consistently receive the same failed values, without needing to
> worry about the precise details of the failure.
> 

Clients will need special handling for ENOMEM, ENODEV, or EPROBEDEFER.
Clients can ignore the details of the error if they don't care, but
don't mask the error.

> > 
> > What happens to the memory that has been assigned so far? How do we
> > reclaim that?
> 
> We’ve already added rollback functionality in qcom_scm_assign_table that
> allows batches to be rolled back to the HLOS if there’s a failure.
> 
> > 
> > > +			break;
> > > +		}
> > > +		if (roll_back && curr_sgl == last_sgl)
> > > +			break;
> > > +	}
> > > +	kfree(mem_regions_buf);
> > > +
> > > +	dma_unmap_single(__scm->dev, dest_dma_addr,
> > > +			 destvms_cp_sz, DMA_TO_DEVICE);
> > > +
> > > +out_free_dest:
> > > +	kfree(destvms_cp);
> > > +out_unmap_source:
> > > +	dma_unmap_single(__scm->dev, source_dma_addr,
> > > +			 srcvms_cp_sz, DMA_TO_DEVICE);
> > > +out_free_source:
> > > +	kfree(srcvms_cp);
> > > +	return ret;
> > > +}
> > > +
> > > +/**
> > > + * qcom_scm_assign_table() - Make a call to prepare batches of memory regions
> > > + *			     and reassign memory ownership of several memory regions at once
> > > + * @sg_table:       A scatter list whose memory needs to be reassigned
> > > + * @srcvms:	    A buffer populated with he vmid(s) for the current set of
> > > + *		    owners
> > > + * @nr_src:	    The number of the src_vms buffer
> > > + * @destvms:	    A buffer populated with he vmid(s) for the new owners
> > > + * @destvms_perms:  A buffer populated with the permission flags of new owners
> > > + * @nr_dest:	    The number of the destvms
> > > + *
> > > + * Return negative errno on failure, 0 on success.
> > > + */
> > > +int qcom_scm_assign_table(struct sg_table *table,
> > > +			  u32 *srcvms, int nr_src,
> > > +			  int *destvms, int *destvms_perms,
> > > +			  int nr_dest)
> > > +{
> > > +	struct scatterlist *last_sgl = NULL;
> > > +	int rb_ret = 0;
> > > +	u32 new_dests;
> > > +	int new_perms;
> > > +	int ret = 0;
> > > +
> > > +	ret = qcom_scm_prepare_mem_batch(table, srcvms, nr_src,
> > > +					 destvms, destvms_perms, nr_dest, last_sgl, false);
> > > +
> > > +	if (!ret)
> > > +		goto out;
> > > +	new_dests = QCOM_SCM_VMID_HLOS;
> > 
> > We have the original srcvms. Will it always be HLOS? If so, then do we
> > need to pass srcvms at all?
> 
> No, srcvms can be VMs other than HLOS. However, this API does not expect
> clients to provide srcvms permissions. As a result, when such permissions
> are absent, we proceed by assigning the previously processed batches to
> HLOS.
> 

This is a wrong assumption though, right? You are allowing srcvms to be
something other than HLOS but assuming HLOS VMID is the only one in the
error path.

> > 
> > > +	new_perms = QCOM_SCM_PERM_EXEC | QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_READ;
> > > +	rb_ret = qcom_scm_prepare_mem_batch(table, destvms, nr_dest, &new_dests,
> > > +					    &new_perms, nr_src, last_sgl, true);
> > > +	WARN_ON_ONCE(rb_ret);
> > > +out:
> > > +	return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(qcom_scm_assign_table);
> > 
> > Who uses this function?
> 
> https://lore.kernel.org/all/cover.1700544802.git.quic_vjitta@quicinc.com/
> 
> This patch responsible for extending qcom secure heap support which will be
> the user of this API. In the current implementation, we use
> qcom_scm_assign_mem and iterate over each sg_list in the sg_table
> individually to secure the memory. Going forward, we will replace this
> approach with our API.
> 
> > 
> > > +
> > >   /**
> > >    * qcom_scm_ocmem_lock_available() - is OCMEM lock/unlock interface available
> > >    */
> > > diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
> > > index ccaf28846054..abd675c7ef49 100644
> > > --- a/include/linux/firmware/qcom/qcom_scm.h
> > > +++ b/include/linux/firmware/qcom/qcom_scm.h
> > > @@ -8,6 +8,7 @@
> > >   #include <linux/err.h>
> > >   #include <linux/types.h>
> > >   #include <linux/cpumask.h>
> > > +#include <linux/scatterlist.h>
> > >   #include <dt-bindings/firmware/qcom,scm.h>
> > > @@ -15,6 +16,8 @@
> > >   #define QCOM_SCM_CPU_PWR_DOWN_L2_ON	0x0
> > >   #define QCOM_SCM_CPU_PWR_DOWN_L2_OFF	0x1
> > >   #define QCOM_SCM_HDCP_MAX_REQ_CNT	5
> > > +#define QCOM_SCM_MAX_BATCH_SECTION	32
> > > +#define QCOM_SCM_MAX_BATCH_SIZE		SZ_2M

Move the macros to qcom_scm.c

> > >   struct qcom_scm_hdcp_req {
> > >   	u32 addr;
> > > @@ -93,6 +96,10 @@ int qcom_scm_mem_protect_video_var(u32 cp_start, u32 cp_size,
> > >   int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, u64 *src,
> > >   			const struct qcom_scm_vmperm *newvm,
> > >   			unsigned int dest_cnt);
> > > +int qcom_scm_assign_table(struct sg_table *table,
> > > +			  u32 *srcvms, int nr_src,
> > > +			  int *destvms, int *destvms_perms,
> > > +			  int nr_dest);
> > >   bool qcom_scm_ocmem_lock_available(void);
> > >   int qcom_scm_ocmem_lock(enum qcom_scm_ocmem_client id, u32 offset, u32 size,
> > > -- 
> > > 2.34.1
> > > 
>
  

On 21-Feb-24 12:13 AM, Elliot Berman wrote:
> On Tue, Feb 20, 2024 at 11:24:33AM +0530, Dibakar Singh wrote:
>>
>>
>> On 09-Feb-24 11:36 PM, Elliot Berman wrote:
>>> On Fri, Feb 09, 2024 at 04:55:36PM +0530, Dibakar Singh wrote:
>>>> Expose an API qcom_scm_assign_table to allow client drivers to batch
>>>> multiple memory regions in a single hyp assign call.
>>>>
>>>> In the existing situation, if our goal is to process an sg_table and
>>>> transfer its ownership from the current VM to a different VM, we have a
>>>> couple of strategies. The first strategy involves processing the entire
>>>> sg_table at once and then transferring the ownership. However, this
>>>> method may have an adverse impact on the system because during an SMC
>>>> call, the NS interrupts are disabled, and this delay could be
>>>> significant when dealing with large sg_tables. To address this issue, we
>>>> can adopt a second strategy, which involves processing each sg_list in
>>>> the sg_table individually and reassigning memory ownership. Although
>>>> this method is slower and potentially impacts performance, it will not
>>>> keep the NS interrupts disabled for an extended period.
>>>>
>>>> A more efficient strategy is to process the sg_table in batches. This
>>>> approach addresses both scenarios by involving memory processing in
>>>> batches, thus avoiding prolonged NS interrupt disablement for longer
>>>> duration when dealing with large sg_tables. Moreover, since we process
>>>> in batches, this method is faster compared to processing each item
>>>> individually. The observations on testing both the approaches for
>>>> performance is as follows:
>>>>
>>>> Allocation Size/            256MB            512MB            1024MB
>>>> Algorithm Used           ===========      ===========      ============
>>>>
>>>> Processing each sg_list   73708(us)        149289(us)       266964(us)
>>>> in sg_table one by one
>>>>
>>>> Processing sg_table in    46925(us)         92691(us)       176893(us)
>>>> batches
>>>>
>>>> This implementation serves as a wrapper around the helper function
>>>> __qcom_scm_assign_mem, which takes an sg_list and processes it in
>>>> batches. We’ve set the limit to a minimum of 32 sg_list in a batch or a
>>>> total batch size of 512 pages. The selection of these numbers is
>>>> heuristic, based on the test runs conducted. Opting for a smaller number
>>>> would compromise performance, while a larger number would result in
>>>> non-secure interrupts being disabled for an extended duration.
>>>>
>>>> Co-developed-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>
>>>> Signed-off-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>
>>>> Signed-off-by: Dibakar Singh <quic_dibasing@quicinc.com>
>>>> ---
>>>>    drivers/firmware/qcom/qcom_scm.c       | 211 +++++++++++++++++++++++++
>>>>    include/linux/firmware/qcom/qcom_scm.h |   7 +
>>>>    2 files changed, 218 insertions(+)
>>>>
>>>> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
>>>> index 520de9b5633a..038b96503d65 100644
>>>> --- a/drivers/firmware/qcom/qcom_scm.c
>>>> +++ b/drivers/firmware/qcom/qcom_scm.c
>>>> @@ -21,6 +21,8 @@
>>>>    #include <linux/platform_device.h>
>>>>    #include <linux/reset-controller.h>
>>>>    #include <linux/types.h>
>>>> +#include <linux/scatterlist.h>
>>>> +#include <linux/slab.h>
>>>>    #include "qcom_scm.h"
>>>> @@ -1048,6 +1050,215 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(qcom_scm_assign_mem);
>>>> +/**
>>>> + * qcom_scm_assign_mem_batch() - Make a secure call to reassign memory
>>>> + *				   ownership of several memory regions
>>>> + * @mem_regions:    A buffer describing the set of memory regions that need to
>>>> + *		    be reassigned
>>>> + * @nr_mem_regions: The number of memory regions that need to be reassigned
>>>> + * @srcvms:	    A buffer populated with he vmid(s) for the current set of
>>>> + *		    owners
>>>> + * @src_sz:	    The size of the srcvms buffer (in bytes)
>>>> + * @destvms:	    A buffer populated with the new owners and corresponding
>>>> + *		    permission flags.
>>>> + * @dest_sz:	    The size of the destvms buffer (in bytes)
>>>> + *
>>>> + * Return negative errno on failure, 0 on success.
>>>> + */
>>>> +static int qcom_scm_assign_mem_batch(struct qcom_scm_mem_map_info *mem_regions,
>>>> +				     size_t nr_mem_regions, u32 *srcvms,
>>>> +				     size_t src_sz,
>>>> +				     struct qcom_scm_current_perm_info *destvms,
>>>> +				     size_t dest_sz)
>>>> +{
>>>> +	dma_addr_t mem_dma_addr;
>>>> +	size_t mem_regions_sz;
>>>> +	int ret = 0, i;
>>>> +
>>>> +	for (i = 0; i < nr_mem_regions; i++) {
>>>> +		mem_regions[i].mem_addr = cpu_to_le64(mem_regions[i].mem_addr);
>>>> +		mem_regions[i].mem_size = cpu_to_le64(mem_regions[i].mem_size);
>>>> +	}
>>>> +
>>>> +	mem_regions_sz = nr_mem_regions * sizeof(*mem_regions);
>>>> +	mem_dma_addr = dma_map_single(__scm->dev, mem_regions, mem_regions_sz,
>>>> +				      DMA_TO_DEVICE);
>>>> +	if (dma_mapping_error(__scm->dev, mem_dma_addr)) {
>>>> +		dev_err(__scm->dev, "mem_dma_addr mapping failed\n");
>>>> +		return -ENOMEM;
>>>> +	}
>>>> +
>>>> +	ret = __qcom_scm_assign_mem(__scm->dev, virt_to_phys(mem_regions),
>>>> +				    mem_regions_sz, virt_to_phys(srcvms), src_sz,
>>>> +				    virt_to_phys(destvms), dest_sz);
>>>> +
>>>> +	dma_unmap_single(__scm->dev, mem_dma_addr, mem_regions_sz, DMA_TO_DEVICE);
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +/**
>>>> + * qcom_scm_prepare_mem_batch() - Prepare batches of memory regions
>>>> + * @sg_table:       A scatter list whose memory needs to be reassigned
>>>> + * @srcvms:	    A buffer populated with he vmid(s) for the current set of
>>>> + *		    owners
>>>> + * @nr_src:	    The number of the src_vms buffer
>>>> + * @destvms:	    A buffer populated with he vmid(s) for the new owners
>>>> + * @destvms_perms:  A buffer populated with the permission flags of new owners
>>>> + * @nr_dest:	    The number of the destvms
>>>> + * @last_sgl:	    Denotes to the last scatter list element. Used in case of rollback
>>>> + * @roll_back:	    Identifies whether we are executing rollback in case of failure
>>>> + *
>>>> + * Return negative errno on failure, 0 on success.
>>>> + */
>>>> +static int qcom_scm_prepare_mem_batch(struct sg_table *table,
>>>> +				      u32 *srcvms, int nr_src,
>>>> +				      int *destvms, int *destvms_perms,
>>>> +				      int nr_dest,
>>>> +				      struct scatterlist *last_sgl, bool roll_back)
>>>> +{
>>>> +	struct qcom_scm_current_perm_info *destvms_cp;
>>>> +	struct qcom_scm_mem_map_info *mem_regions_buf;
>>>> +	struct scatterlist *curr_sgl = table->sgl;
>>>> +	dma_addr_t source_dma_addr, dest_dma_addr;
>>>> +	size_t batch_iterator;
>>>> +	size_t batch_start = 0;
>>>> +	size_t destvms_cp_sz;
>>>> +	size_t srcvms_cp_sz;
>>>> +	size_t batch_size;
>>>> +	u32 *srcvms_cp;
>>>> +	int ret = 0;
>>>> +	int i;
>>>> +
>>>> +	if (!table || !table->sgl || !srcvms || !nr_src ||
>>>> +	    !destvms || !destvms_perms || !nr_dest || !table->nents)
>>>> +		return -EINVAL;
>>>> +
>>>> +	srcvms_cp_sz = sizeof(*srcvms_cp) * nr_src;
>>>> +	srcvms_cp = kmemdup(srcvms, srcvms_cp_sz, GFP_KERNEL);
>>>> +	if (!srcvms_cp)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	for (i = 0; i < nr_src; i++)
>>>> +		srcvms_cp[i] = cpu_to_le32(srcvms_cp[i]);
>>>> +
>>>> +	source_dma_addr = dma_map_single(__scm->dev, srcvms_cp,
>>>> +					 srcvms_cp_sz, DMA_TO_DEVICE);
>>>
>>> Please use the new tzmem allocator:
>>>
>>> https://lore.kernel.org/all/20240205182810.58382-1-brgl@bgdev.pl/
>>
>> Noted, I will use this allocator in V2 patch.
>>
>>>
>>>> +
>>>> +	if (dma_mapping_error(__scm->dev, source_dma_addr)) {
>>>> +		ret = -ENOMEM;
>>>> +		goto out_free_source;
>>>> +	}
>>>> +
>>>> +	destvms_cp_sz = sizeof(*destvms_cp) * nr_dest;
>>>> +	destvms_cp = kzalloc(destvms_cp_sz, GFP_KERNEL);
>>>> +
>>>> +	if (!destvms_cp) {
>>>> +		ret = -ENOMEM;
>>>> +		goto out_unmap_source;
>>>> +	}
>>>> +
>>>> +	for (i = 0; i < nr_dest; i++) {
>>>> +		destvms_cp[i].vmid = cpu_to_le32(destvms[i]);
>>>> +		destvms_cp[i].perm = cpu_to_le32(destvms_perms[i]);
>>>> +		destvms_cp[i].ctx = 0;
>>>> +		destvms_cp[i].ctx_size = 0;
>>>> +	}
>>>> +
>>>> +	dest_dma_addr = dma_map_single(__scm->dev, destvms_cp,
>>>> +				       destvms_cp_sz, DMA_TO_DEVICE);
>>>> +	if (dma_mapping_error(__scm->dev, dest_dma_addr)) {
>>>> +		ret = -ENOMEM;
>>>> +		goto out_free_dest;
>>>> +	}
>>>> +
>>>> +	mem_regions_buf = kcalloc(QCOM_SCM_MAX_BATCH_SECTION, sizeof(*mem_regions_buf),
>>>> +				  GFP_KERNEL);
>>>> +	if (!mem_regions_buf)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	while (batch_start < table->nents) {
>>>> +		batch_size = 0;
>>>> +		batch_iterator = 0;
>>>> +
>>>> +		do {
>>>> +			mem_regions_buf[batch_iterator].mem_addr = page_to_phys(sg_page(curr_sgl));
>>>> +			mem_regions_buf[batch_iterator].mem_size = curr_sgl->length;
>>>> +			batch_size += curr_sgl->length;
>>>> +			batch_iterator++;
>>>> +			if (roll_back && curr_sgl == last_sgl)
>>>> +				break;
> 
> Is roll_back bool necessary? last_sgl is NULL in case of not
> rolling back and I think is good sentinel value when not rolling back.
> 
> If last_sgl is NULL, then all sgl entries are assigned. If last_sgl is
> not NULL, then all the sgl entries are assigned until last_sgl is
> encountered (inclusive).

Thanks Elliot for pointing it out. However, in next patchset, I will be 
changing the strategy a little bit. Currently, I am rolling back till 
the current failed batch, but I think this is not correct because we are 
not sure of which state the current batch is in. So, in this case, I am 
planning to implement it in a way that it’ll roll back to the last 
successful batch and leak the current batch by setting it private.

> 
>>>> +			curr_sgl = sg_next(curr_sgl);
>>>> +		} while (curr_sgl && batch_iterator < QCOM_SCM_MAX_BATCH_SECTION &&
>>>> +				curr_sgl->length + batch_size < QCOM_SCM_MAX_BATCH_SIZE);
>>>> +
>>>> +		batch_start += batch_iterator;
>>>> +
>>>> +		ret = qcom_scm_assign_mem_batch(mem_regions_buf, batch_iterator,
>>>> +						srcvms_cp, srcvms_cp_sz, destvms_cp, destvms_cp_sz);
>>>> +
>>>> +		if (ret) {
>>>> +			dev_info(__scm->dev, "Failed to assign memory protection, ret = %d\n", ret);
>>>> +			last_sgl = curr_sgl;
>>>> +			ret = -EADDRNOTAVAIL;
>>>
>>> Probably should not be changing the ret value.
>>
>> We are adjusting the return value here because we have already printed the
>> failed return value earlier. Our goal is to ensure that clients invoking
>> this API consistently receive the same failed values, without needing to
>> worry about the precise details of the failure.
>>
> 
> Clients will need special handling for ENOMEM, ENODEV, or EPROBEDEFER.
> Clients can ignore the details of the error if they don't care, but
> don't mask the error.
> 

Noted. I'll take care of this in next patchset.

>>>
>>> What happens to the memory that has been assigned so far? How do we
>>> reclaim that?
>>
>> We’ve already added rollback functionality in qcom_scm_assign_table that
>> allows batches to be rolled back to the HLOS if there’s a failure.
>>
>>>
>>>> +			break;
>>>> +		}
>>>> +		if (roll_back && curr_sgl == last_sgl)
>>>> +			break;
>>>> +	}
>>>> +	kfree(mem_regions_buf);
>>>> +
>>>> +	dma_unmap_single(__scm->dev, dest_dma_addr,
>>>> +			 destvms_cp_sz, DMA_TO_DEVICE);
>>>> +
>>>> +out_free_dest:
>>>> +	kfree(destvms_cp);
>>>> +out_unmap_source:
>>>> +	dma_unmap_single(__scm->dev, source_dma_addr,
>>>> +			 srcvms_cp_sz, DMA_TO_DEVICE);
>>>> +out_free_source:
>>>> +	kfree(srcvms_cp);
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +/**
>>>> + * qcom_scm_assign_table() - Make a call to prepare batches of memory regions
>>>> + *			     and reassign memory ownership of several memory regions at once
>>>> + * @sg_table:       A scatter list whose memory needs to be reassigned
>>>> + * @srcvms:	    A buffer populated with he vmid(s) for the current set of
>>>> + *		    owners
>>>> + * @nr_src:	    The number of the src_vms buffer
>>>> + * @destvms:	    A buffer populated with he vmid(s) for the new owners
>>>> + * @destvms_perms:  A buffer populated with the permission flags of new owners
>>>> + * @nr_dest:	    The number of the destvms
>>>> + *
>>>> + * Return negative errno on failure, 0 on success.
>>>> + */
>>>> +int qcom_scm_assign_table(struct sg_table *table,
>>>> +			  u32 *srcvms, int nr_src,
>>>> +			  int *destvms, int *destvms_perms,
>>>> +			  int nr_dest)
>>>> +{
>>>> +	struct scatterlist *last_sgl = NULL;
>>>> +	int rb_ret = 0;
>>>> +	u32 new_dests;
>>>> +	int new_perms;
>>>> +	int ret = 0;
>>>> +
>>>> +	ret = qcom_scm_prepare_mem_batch(table, srcvms, nr_src,
>>>> +					 destvms, destvms_perms, nr_dest, last_sgl, false);
>>>> +
>>>> +	if (!ret)
>>>> +		goto out;
>>>> +	new_dests = QCOM_SCM_VMID_HLOS;
>>>
>>> We have the original srcvms. Will it always be HLOS? If so, then do we
>>> need to pass srcvms at all?
>>
>> No, srcvms can be VMs other than HLOS. However, this API does not expect
>> clients to provide srcvms permissions. As a result, when such permissions
>> are absent, we proceed by assigning the previously processed batches to
>> HLOS.
>>
> 
> This is a wrong assumption though, right? You are allowing srcvms to be
> something other than HLOS but assuming HLOS VMID is the only one in the
> error path.
> 

Yes, assuming that this API allows the transfer of page ownership from 
any VM to any other VM, rolling it back to HLOS in case of failure 
doesn’t make much sense. I am thinking of doing it in a way where if the 
source VM is HLOS, then we roll it back to HLOS; otherwise, since we 
don’t have permission for the source VMs, we’ll make the pages private 
and let the clients handle it in such cases.

>>>
>>>> +	new_perms = QCOM_SCM_PERM_EXEC | QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_READ;
>>>> +	rb_ret = qcom_scm_prepare_mem_batch(table, destvms, nr_dest, &new_dests,
>>>> +					    &new_perms, nr_src, last_sgl, true);
>>>> +	WARN_ON_ONCE(rb_ret);
>>>> +out:
>>>> +	return ret;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(qcom_scm_assign_table);
>>>
>>> Who uses this function?
>>
>> https://lore.kernel.org/all/cover.1700544802.git.quic_vjitta@quicinc.com/
>>
>> This patch responsible for extending qcom secure heap support which will be
>> the user of this API. In the current implementation, we use
>> qcom_scm_assign_mem and iterate over each sg_list in the sg_table
>> individually to secure the memory. Going forward, we will replace this
>> approach with our API.
>>
>>>
>>>> +
>>>>    /**
>>>>     * qcom_scm_ocmem_lock_available() - is OCMEM lock/unlock interface available
>>>>     */
>>>> diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
>>>> index ccaf28846054..abd675c7ef49 100644
>>>> --- a/include/linux/firmware/qcom/qcom_scm.h
>>>> +++ b/include/linux/firmware/qcom/qcom_scm.h
>>>> @@ -8,6 +8,7 @@
>>>>    #include <linux/err.h>
>>>>    #include <linux/types.h>
>>>>    #include <linux/cpumask.h>
>>>> +#include <linux/scatterlist.h>
>>>>    #include <dt-bindings/firmware/qcom,scm.h>
>>>> @@ -15,6 +16,8 @@
>>>>    #define QCOM_SCM_CPU_PWR_DOWN_L2_ON	0x0
>>>>    #define QCOM_SCM_CPU_PWR_DOWN_L2_OFF	0x1
>>>>    #define QCOM_SCM_HDCP_MAX_REQ_CNT	5
>>>> +#define QCOM_SCM_MAX_BATCH_SECTION	32
>>>> +#define QCOM_SCM_MAX_BATCH_SIZE		SZ_2M
> 
> Move the macros to qcom_scm.c

Noted. I'll take care of this in the next patchset.

> 
>>>>    struct qcom_scm_hdcp_req {
>>>>    	u32 addr;
>>>> @@ -93,6 +96,10 @@ int qcom_scm_mem_protect_video_var(u32 cp_start, u32 cp_size,
>>>>    int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, u64 *src,
>>>>    			const struct qcom_scm_vmperm *newvm,
>>>>    			unsigned int dest_cnt);
>>>> +int qcom_scm_assign_table(struct sg_table *table,
>>>> +			  u32 *srcvms, int nr_src,
>>>> +			  int *destvms, int *destvms_perms,
>>>> +			  int nr_dest);
>>>>    bool qcom_scm_ocmem_lock_available(void);
>>>>    int qcom_scm_ocmem_lock(enum qcom_scm_ocmem_client id, u32 offset, u32 size,
>>>> -- 
>>>> 2.34.1
>>>>
>>