[1/1] crypto: ccp - Allocate TEE ring and cmd buffer using DMA APIs

  For AMD Secure Processor (ASP) to map and access TEE ring buffer, the
ring buffer address sent by host to ASP must be a real physical
address and the pages must be physically contiguous.

In a virtualized environment though, when the driver is running in a
guest VM, the pages allocated by __get_free_pages() may not be
contiguous in the host (or machine) physical address space. Guests
will see a guest (or pseudo) physical address and not the actual host
(or machine) physical address. The TEE running on ASP cannot decipher
pseudo physical addresses. It needs host or machine physical address.

To resolve this problem, use DMA APIs for allocating buffers that must
be shared with TEE. This will ensure that the pages are contiguous in
host (or machine) address space. If the DMA handle is an IOVA,
translate it into a physical address before sending it to ASP.

This patch also exports two APIs (one for buffer allocation and
another to free the buffer). This API can be used by AMD-TEE driver to
share buffers with TEE.

Fixes: 33960acccfbd ("crypto: ccp - add TEE support for Raven Ridge")
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: stable@vger.kernel.org
Signed-off-by: Rijo Thomas <Rijo-john.Thomas@amd.com>
Co-developed-by: Jeshwanth <JESHWANTHKUMAR.NK@amd.com>
Signed-off-by: Jeshwanth <JESHWANTHKUMAR.NK@amd.com>
Reviewed-by: Devaraj Rangasamy <Devaraj.Rangasamy@amd.com>
---
 drivers/crypto/ccp/psp-dev.c |   6 +-
 drivers/crypto/ccp/tee-dev.c | 116 ++++++++++++++++++++++-------------
 drivers/crypto/ccp/tee-dev.h |   9 +--
 include/linux/psp-tee.h      |  47 ++++++++++++++
 4 files changed, 127 insertions(+), 51 deletions(-)
  

Am 06.12.22 um 13:30 schrieb Rijo Thomas:
> For AMD Secure Processor (ASP) to map and access TEE ring buffer, the
> ring buffer address sent by host to ASP must be a real physical
> address and the pages must be physically contiguous.
>
> In a virtualized environment though, when the driver is running in a
> guest VM, the pages allocated by __get_free_pages() may not be
> contiguous in the host (or machine) physical address space. Guests
> will see a guest (or pseudo) physical address and not the actual host
> (or machine) physical address. The TEE running on ASP cannot decipher
> pseudo physical addresses. It needs host or machine physical address.
>
> To resolve this problem, use DMA APIs for allocating buffers that must
> be shared with TEE. This will ensure that the pages are contiguous in
> host (or machine) address space. If the DMA handle is an IOVA,
> translate it into a physical address before sending it to ASP.
>
> This patch also exports two APIs (one for buffer allocation and
> another to free the buffer). This API can be used by AMD-TEE driver to
> share buffers with TEE.

Maybe use some other name than dma_buffer for your structure, cause that 
is usually something completely different in the Linux kernel.

Regards,
Christian.

>
> Fixes: 33960acccfbd ("crypto: ccp - add TEE support for Raven Ridge")
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Rijo Thomas <Rijo-john.Thomas@amd.com>
> Co-developed-by: Jeshwanth <JESHWANTHKUMAR.NK@amd.com>
> Signed-off-by: Jeshwanth <JESHWANTHKUMAR.NK@amd.com>
> Reviewed-by: Devaraj Rangasamy <Devaraj.Rangasamy@amd.com>
> ---
>   drivers/crypto/ccp/psp-dev.c |   6 +-
>   drivers/crypto/ccp/tee-dev.c | 116 ++++++++++++++++++++++-------------
>   drivers/crypto/ccp/tee-dev.h |   9 +--
>   include/linux/psp-tee.h      |  47 ++++++++++++++
>   4 files changed, 127 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
> index c9c741ac8442..2b86158d7435 100644
> --- a/drivers/crypto/ccp/psp-dev.c
> +++ b/drivers/crypto/ccp/psp-dev.c
> @@ -161,13 +161,13 @@ int psp_dev_init(struct sp_device *sp)
>   		goto e_err;
>   	}
>   
> +	if (sp->set_psp_master_device)
> +		sp->set_psp_master_device(sp);
> +
>   	ret = psp_init(psp);
>   	if (ret)
>   		goto e_irq;
>   
> -	if (sp->set_psp_master_device)
> -		sp->set_psp_master_device(sp);
> -
>   	/* Enable interrupt */
>   	iowrite32(-1, psp->io_regs + psp->vdata->inten_reg);
>   
> diff --git a/drivers/crypto/ccp/tee-dev.c b/drivers/crypto/ccp/tee-dev.c
> index 5c9d47f3be37..1631d9851e54 100644
> --- a/drivers/crypto/ccp/tee-dev.c
> +++ b/drivers/crypto/ccp/tee-dev.c
> @@ -12,8 +12,9 @@
>   #include <linux/mutex.h>
>   #include <linux/delay.h>
>   #include <linux/slab.h>
> +#include <linux/dma-direct.h>
> +#include <linux/iommu.h>
>   #include <linux/gfp.h>
> -#include <linux/psp-sev.h>
>   #include <linux/psp-tee.h>
>   
>   #include "psp-dev.h"
> @@ -21,25 +22,64 @@
>   
>   static bool psp_dead;
>   
> +struct dma_buffer *psp_tee_alloc_dmabuf(unsigned long size, gfp_t gfp)
> +{
> +	struct psp_device *psp = psp_get_master_device();
> +	struct dma_buffer *dma_buf;
> +	struct iommu_domain *dom;
> +
> +	if (!psp || !size)
> +		return NULL;
> +
> +	dma_buf = kzalloc(sizeof(*dma_buf), GFP_KERNEL);
> +	if (!dma_buf)
> +		return NULL;
> +
> +	dma_buf->vaddr = dma_alloc_coherent(psp->dev, size, &dma_buf->dma, gfp);
> +	if (!dma_buf->vaddr || !dma_buf->dma) {
> +		kfree(dma_buf);
> +		return NULL;
> +	}
> +
> +	dma_buf->size = size;
> +
> +	dom = iommu_get_domain_for_dev(psp->dev);
> +	if (dom)
> +		dma_buf->paddr = iommu_iova_to_phys(dom, dma_buf->dma);
> +	else
> +		dma_buf->paddr = dma_buf->dma;
> +
> +	return dma_buf;
> +}
> +EXPORT_SYMBOL(psp_tee_alloc_dmabuf);
> +
> +void psp_tee_free_dmabuf(struct dma_buffer *dma_buf)
> +{
> +	struct psp_device *psp = psp_get_master_device();
> +
> +	if (!psp || !dma_buf)
> +		return;
> +
> +	dma_free_coherent(psp->dev, dma_buf->size,
> +			  dma_buf->vaddr, dma_buf->dma);
> +
> +	kfree(dma_buf);
> +}
> +EXPORT_SYMBOL(psp_tee_free_dmabuf);
> +
>   static int tee_alloc_ring(struct psp_tee_device *tee, int ring_size)
>   {
>   	struct ring_buf_manager *rb_mgr = &tee->rb_mgr;
> -	void *start_addr;
>   
>   	if (!ring_size)
>   		return -EINVAL;
>   
> -	/* We need actual physical address instead of DMA address, since
> -	 * Trusted OS running on AMD Secure Processor will map this region
> -	 */
> -	start_addr = (void *)__get_free_pages(GFP_KERNEL, get_order(ring_size));
> -	if (!start_addr)
> +	rb_mgr->ring_buf = psp_tee_alloc_dmabuf(ring_size,
> +						GFP_KERNEL | __GFP_ZERO);
> +	if (!rb_mgr->ring_buf) {
> +		dev_err(tee->dev, "ring allocation failed\n");
>   		return -ENOMEM;
> -
> -	memset(start_addr, 0x0, ring_size);
> -	rb_mgr->ring_start = start_addr;
> -	rb_mgr->ring_size = ring_size;
> -	rb_mgr->ring_pa = __psp_pa(start_addr);
> +	}
>   	mutex_init(&rb_mgr->mutex);
>   
>   	return 0;
> @@ -49,15 +89,8 @@ static void tee_free_ring(struct psp_tee_device *tee)
>   {
>   	struct ring_buf_manager *rb_mgr = &tee->rb_mgr;
>   
> -	if (!rb_mgr->ring_start)
> -		return;
> +	psp_tee_free_dmabuf(rb_mgr->ring_buf);
>   
> -	free_pages((unsigned long)rb_mgr->ring_start,
> -		   get_order(rb_mgr->ring_size));
> -
> -	rb_mgr->ring_start = NULL;
> -	rb_mgr->ring_size = 0;
> -	rb_mgr->ring_pa = 0;
>   	mutex_destroy(&rb_mgr->mutex);
>   }
>   
> @@ -81,35 +114,36 @@ static int tee_wait_cmd_poll(struct psp_tee_device *tee, unsigned int timeout,
>   	return -ETIMEDOUT;
>   }
>   
> -static
> -struct tee_init_ring_cmd *tee_alloc_cmd_buffer(struct psp_tee_device *tee)
> +struct dma_buffer *tee_alloc_cmd_buffer(struct psp_tee_device *tee)
>   {
>   	struct tee_init_ring_cmd *cmd;
> +	struct dma_buffer *cmd_buffer;
>   
> -	cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
> -	if (!cmd)
> +	cmd_buffer = psp_tee_alloc_dmabuf(sizeof(*cmd),
> +					  GFP_KERNEL | __GFP_ZERO);
> +	if (!cmd_buffer)
>   		return NULL;
>   
> -	cmd->hi_addr = upper_32_bits(tee->rb_mgr.ring_pa);
> -	cmd->low_addr = lower_32_bits(tee->rb_mgr.ring_pa);
> -	cmd->size = tee->rb_mgr.ring_size;
> +	cmd = (struct tee_init_ring_cmd *)cmd_buffer->vaddr;
> +	cmd->hi_addr = upper_32_bits(tee->rb_mgr.ring_buf->paddr);
> +	cmd->low_addr = lower_32_bits(tee->rb_mgr.ring_buf->paddr);
> +	cmd->size = tee->rb_mgr.ring_buf->size;
>   
>   	dev_dbg(tee->dev, "tee: ring address: high = 0x%x low = 0x%x size = %u\n",
>   		cmd->hi_addr, cmd->low_addr, cmd->size);
>   
> -	return cmd;
> +	return cmd_buffer;
>   }
>   
> -static inline void tee_free_cmd_buffer(struct tee_init_ring_cmd *cmd)
> +static inline void tee_free_cmd_buffer(struct dma_buffer *cmd_buffer)
>   {
> -	kfree(cmd);
> +	psp_tee_free_dmabuf(cmd_buffer);
>   }
>   
>   static int tee_init_ring(struct psp_tee_device *tee)
>   {
>   	int ring_size = MAX_RING_BUFFER_ENTRIES * sizeof(struct tee_ring_cmd);
> -	struct tee_init_ring_cmd *cmd;
> -	phys_addr_t cmd_buffer;
> +	struct dma_buffer *cmd_buffer;
>   	unsigned int reg;
>   	int ret;
>   
> @@ -123,21 +157,19 @@ static int tee_init_ring(struct psp_tee_device *tee)
>   
>   	tee->rb_mgr.wptr = 0;
>   
> -	cmd = tee_alloc_cmd_buffer(tee);
> -	if (!cmd) {
> +	cmd_buffer = tee_alloc_cmd_buffer(tee);
> +	if (!cmd_buffer) {
>   		tee_free_ring(tee);
>   		return -ENOMEM;
>   	}
>   
> -	cmd_buffer = __psp_pa((void *)cmd);
> -
>   	/* Send command buffer details to Trusted OS by writing to
>   	 * CPU-PSP message registers
>   	 */
>   
> -	iowrite32(lower_32_bits(cmd_buffer),
> +	iowrite32(lower_32_bits(cmd_buffer->paddr),
>   		  tee->io_regs + tee->vdata->cmdbuff_addr_lo_reg);
> -	iowrite32(upper_32_bits(cmd_buffer),
> +	iowrite32(upper_32_bits(cmd_buffer->paddr),
>   		  tee->io_regs + tee->vdata->cmdbuff_addr_hi_reg);
>   	iowrite32(TEE_RING_INIT_CMD,
>   		  tee->io_regs + tee->vdata->cmdresp_reg);
> @@ -157,7 +189,7 @@ static int tee_init_ring(struct psp_tee_device *tee)
>   	}
>   
>   free_buf:
> -	tee_free_cmd_buffer(cmd);
> +	tee_free_cmd_buffer(cmd_buffer);
>   
>   	return ret;
>   }
> @@ -167,7 +199,7 @@ static void tee_destroy_ring(struct psp_tee_device *tee)
>   	unsigned int reg;
>   	int ret;
>   
> -	if (!tee->rb_mgr.ring_start)
> +	if (!tee->rb_mgr.ring_buf->vaddr)
>   		return;
>   
>   	if (psp_dead)
> @@ -256,7 +288,7 @@ static int tee_submit_cmd(struct psp_tee_device *tee, enum tee_cmd_id cmd_id,
>   	do {
>   		/* Get pointer to ring buffer command entry */
>   		cmd = (struct tee_ring_cmd *)
> -			(tee->rb_mgr.ring_start + tee->rb_mgr.wptr);
> +			(tee->rb_mgr.ring_buf->vaddr + tee->rb_mgr.wptr);
>   
>   		rptr = ioread32(tee->io_regs + tee->vdata->ring_rptr_reg);
>   
> @@ -305,7 +337,7 @@ static int tee_submit_cmd(struct psp_tee_device *tee, enum tee_cmd_id cmd_id,
>   
>   	/* Update local copy of write pointer */
>   	tee->rb_mgr.wptr += sizeof(struct tee_ring_cmd);
> -	if (tee->rb_mgr.wptr >= tee->rb_mgr.ring_size)
> +	if (tee->rb_mgr.wptr >= tee->rb_mgr.ring_buf->size)
>   		tee->rb_mgr.wptr = 0;
>   
>   	/* Trigger interrupt to Trusted OS */
> diff --git a/drivers/crypto/ccp/tee-dev.h b/drivers/crypto/ccp/tee-dev.h
> index 49d26158b71e..9238487ee8bf 100644
> --- a/drivers/crypto/ccp/tee-dev.h
> +++ b/drivers/crypto/ccp/tee-dev.h
> @@ -16,6 +16,7 @@
>   
>   #include <linux/device.h>
>   #include <linux/mutex.h>
> +#include <linux/psp-tee.h>
>   
>   #define TEE_DEFAULT_TIMEOUT		10
>   #define MAX_BUFFER_SIZE			988
> @@ -48,17 +49,13 @@ struct tee_init_ring_cmd {
>   
>   /**
>    * struct ring_buf_manager - Helper structure to manage ring buffer.
> - * @ring_start:  starting address of ring buffer
> - * @ring_size:   size of ring buffer in bytes
> - * @ring_pa:     physical address of ring buffer
>    * @wptr:        index to the last written entry in ring buffer
> + * @ring_buf:    ring buffer allocated using DMA api
>    */
>   struct ring_buf_manager {
>   	struct mutex mutex;	/* synchronizes access to ring buffer */
> -	void *ring_start;
> -	u32 ring_size;
> -	phys_addr_t ring_pa;
>   	u32 wptr;
> +	struct dma_buffer *ring_buf;
>   };
>   
>   struct psp_tee_device {
> diff --git a/include/linux/psp-tee.h b/include/linux/psp-tee.h
> index cb0c95d6d76b..c0fa922f24d4 100644
> --- a/include/linux/psp-tee.h
> +++ b/include/linux/psp-tee.h
> @@ -13,6 +13,7 @@
>   
>   #include <linux/types.h>
>   #include <linux/errno.h>
> +#include <linux/dma-mapping.h>
>   
>   /* This file defines the Trusted Execution Environment (TEE) interface commands
>    * and the API exported by AMD Secure Processor driver to communicate with
> @@ -40,6 +41,20 @@ enum tee_cmd_id {
>   	TEE_CMD_ID_UNMAP_SHARED_MEM,
>   };
>   
> +/**
> + * struct dma_buffer - Structure for a DMA buffer.
> + * @dma:    DMA buffer address
> + * @paddr:  Physical address of DMA buffer
> + * @vaddr:  CPU virtual address of DMA buffer
> + * @size:   Size of DMA buffer in bytes
> + */
> +struct dma_buffer {
> +	dma_addr_t dma;
> +	phys_addr_t paddr;
> +	void *vaddr;
> +	unsigned long size;
> +};
> +
>   #ifdef CONFIG_CRYPTO_DEV_SP_PSP
>   /**
>    * psp_tee_process_cmd() - Process command in Trusted Execution Environment
> @@ -75,6 +90,28 @@ int psp_tee_process_cmd(enum tee_cmd_id cmd_id, void *buf, size_t len,
>    */
>   int psp_check_tee_status(void);
>   
> +/**
> + * psp_tee_alloc_dmabuf() - Allocates memory of requested size and flags using
> + * dma_alloc_coherent() API.
> + *
> + * This function can be used to allocate a shared memory region between the
> + * host and PSP TEE.
> + *
> + * Returns:
> + * non-NULL   a valid pointer to struct dma_buffer
> + * NULL       on failure
> + */
> +struct dma_buffer *psp_tee_alloc_dmabuf(unsigned long size, gfp_t gfp);
> +
> +/**
> + * psp_tee_free_dmabuf() - Deallocates memory using dma_free_coherent() API.
> + *
> + * This function can be used to release shared memory region between host
> + * and PSP TEE.
> + *
> + */
> +void psp_tee_free_dmabuf(struct dma_buffer *dma_buffer);
> +
>   #else /* !CONFIG_CRYPTO_DEV_SP_PSP */
>   
>   static inline int psp_tee_process_cmd(enum tee_cmd_id cmd_id, void *buf,
> @@ -87,5 +124,15 @@ static inline int psp_check_tee_status(void)
>   {
>   	return -ENODEV;
>   }
> +
> +static inline
> +struct dma_buffer *psp_tee_alloc_dmabuf(unsigned long size, gfp_t gfp)
> +{
> +	return NULL;
> +}
> +
> +static inline void psp_tee_free_dmabuf(struct dma_buffer *dma_buffer)
> +{
> +}
>   #endif /* CONFIG_CRYPTO_DEV_SP_PSP */
>   #endif /* __PSP_TEE_H_ */
  

On 12/6/2022 6:11 PM, Christian König wrote:
> Am 06.12.22 um 13:30 schrieb Rijo Thomas:
>> For AMD Secure Processor (ASP) to map and access TEE ring buffer, the
>> ring buffer address sent by host to ASP must be a real physical
>> address and the pages must be physically contiguous.
>>
>> In a virtualized environment though, when the driver is running in a
>> guest VM, the pages allocated by __get_free_pages() may not be
>> contiguous in the host (or machine) physical address space. Guests
>> will see a guest (or pseudo) physical address and not the actual host
>> (or machine) physical address. The TEE running on ASP cannot decipher
>> pseudo physical addresses. It needs host or machine physical address.
>>
>> To resolve this problem, use DMA APIs for allocating buffers that must
>> be shared with TEE. This will ensure that the pages are contiguous in
>> host (or machine) address space. If the DMA handle is an IOVA,
>> translate it into a physical address before sending it to ASP.
>>
>> This patch also exports two APIs (one for buffer allocation and
>> another to free the buffer). This API can be used by AMD-TEE driver to
>> share buffers with TEE.
> 
> Maybe use some other name than dma_buffer for your structure, cause that is usually something completely different in the Linux kernel.
> 

Sure Christian. I shall rename "struct dma_buffer" to "struct shm_buffer" (Shared Memory Buffer) in the file include/linux/psp-tee.h
The functions psp_tee_alloc_dmabuf() and psp_tee_free_dmabuf() shall be renamed to psp_tee_alloc_shmbuf() and psp_tee_free_shmbuf(), respectively.
I shall do correction in next patch revision. I will wait for others to review as well before I post update.

Thanks,
Rijo

> Regards,
> Christian.
> 
>>
>> Fixes: 33960acccfbd ("crypto: ccp - add TEE support for Raven Ridge")
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Rijo Thomas <Rijo-john.Thomas@amd.com>
>> Co-developed-by: Jeshwanth <JESHWANTHKUMAR.NK@amd.com>
>> Signed-off-by: Jeshwanth <JESHWANTHKUMAR.NK@amd.com>
>> Reviewed-by: Devaraj Rangasamy <Devaraj.Rangasamy@amd.com>
>> ---
>>   drivers/crypto/ccp/psp-dev.c |   6 +-
>>   drivers/crypto/ccp/tee-dev.c | 116 ++++++++++++++++++++++-------------
>>   drivers/crypto/ccp/tee-dev.h |   9 +--
>>   include/linux/psp-tee.h      |  47 ++++++++++++++
>>   4 files changed, 127 insertions(+), 51 deletions(-)
>>
>> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
>> index c9c741ac8442..2b86158d7435 100644
>> --- a/drivers/crypto/ccp/psp-dev.c
>> +++ b/drivers/crypto/ccp/psp-dev.c
>> @@ -161,13 +161,13 @@ int psp_dev_init(struct sp_device *sp)
>>           goto e_err;
>>       }
>>   +    if (sp->set_psp_master_device)
>> +        sp->set_psp_master_device(sp);
>> +
>>       ret = psp_init(psp);
>>       if (ret)
>>           goto e_irq;
>>   -    if (sp->set_psp_master_device)
>> -        sp->set_psp_master_device(sp);
>> -
>>       /* Enable interrupt */
>>       iowrite32(-1, psp->io_regs + psp->vdata->inten_reg);
>>   diff --git a/drivers/crypto/ccp/tee-dev.c b/drivers/crypto/ccp/tee-dev.c
>> index 5c9d47f3be37..1631d9851e54 100644
>> --- a/drivers/crypto/ccp/tee-dev.c
>> +++ b/drivers/crypto/ccp/tee-dev.c
>> @@ -12,8 +12,9 @@
>>   #include <linux/mutex.h>
>>   #include <linux/delay.h>
>>   #include <linux/slab.h>
>> +#include <linux/dma-direct.h>
>> +#include <linux/iommu.h>
>>   #include <linux/gfp.h>
>> -#include <linux/psp-sev.h>
>>   #include <linux/psp-tee.h>
>>     #include "psp-dev.h"
>> @@ -21,25 +22,64 @@
>>     static bool psp_dead;
>>   +struct dma_buffer *psp_tee_alloc_dmabuf(unsigned long size, gfp_t gfp)
>> +{
>> +    struct psp_device *psp = psp_get_master_device();
>> +    struct dma_buffer *dma_buf;
>> +    struct iommu_domain *dom;
>> +
>> +    if (!psp || !size)
>> +        return NULL;
>> +
>> +    dma_buf = kzalloc(sizeof(*dma_buf), GFP_KERNEL);
>> +    if (!dma_buf)
>> +        return NULL;
>> +
>> +    dma_buf->vaddr = dma_alloc_coherent(psp->dev, size, &dma_buf->dma, gfp);
>> +    if (!dma_buf->vaddr || !dma_buf->dma) {
>> +        kfree(dma_buf);
>> +        return NULL;
>> +    }
>> +
>> +    dma_buf->size = size;
>> +
>> +    dom = iommu_get_domain_for_dev(psp->dev);
>> +    if (dom)
>> +        dma_buf->paddr = iommu_iova_to_phys(dom, dma_buf->dma);
>> +    else
>> +        dma_buf->paddr = dma_buf->dma;
>> +
>> +    return dma_buf;
>> +}
>> +EXPORT_SYMBOL(psp_tee_alloc_dmabuf);
>> +
>> +void psp_tee_free_dmabuf(struct dma_buffer *dma_buf)
>> +{
>> +    struct psp_device *psp = psp_get_master_device();
>> +
>> +    if (!psp || !dma_buf)
>> +        return;
>> +
>> +    dma_free_coherent(psp->dev, dma_buf->size,
>> +              dma_buf->vaddr, dma_buf->dma);
>> +
>> +    kfree(dma_buf);
>> +}
>> +EXPORT_SYMBOL(psp_tee_free_dmabuf);
>> +
>>   static int tee_alloc_ring(struct psp_tee_device *tee, int ring_size)
>>   {
>>       struct ring_buf_manager *rb_mgr = &tee->rb_mgr;
>> -    void *start_addr;
>>         if (!ring_size)
>>           return -EINVAL;
>>   -    /* We need actual physical address instead of DMA address, since
>> -     * Trusted OS running on AMD Secure Processor will map this region
>> -     */
>> -    start_addr = (void *)__get_free_pages(GFP_KERNEL, get_order(ring_size));
>> -    if (!start_addr)
>> +    rb_mgr->ring_buf = psp_tee_alloc_dmabuf(ring_size,
>> +                        GFP_KERNEL | __GFP_ZERO);
>> +    if (!rb_mgr->ring_buf) {
>> +        dev_err(tee->dev, "ring allocation failed\n");
>>           return -ENOMEM;
>> -
>> -    memset(start_addr, 0x0, ring_size);
>> -    rb_mgr->ring_start = start_addr;
>> -    rb_mgr->ring_size = ring_size;
>> -    rb_mgr->ring_pa = __psp_pa(start_addr);
>> +    }
>>       mutex_init(&rb_mgr->mutex);
>>         return 0;
>> @@ -49,15 +89,8 @@ static void tee_free_ring(struct psp_tee_device *tee)
>>   {
>>       struct ring_buf_manager *rb_mgr = &tee->rb_mgr;
>>   -    if (!rb_mgr->ring_start)
>> -        return;
>> +    psp_tee_free_dmabuf(rb_mgr->ring_buf);
>>   -    free_pages((unsigned long)rb_mgr->ring_start,
>> -           get_order(rb_mgr->ring_size));
>> -
>> -    rb_mgr->ring_start = NULL;
>> -    rb_mgr->ring_size = 0;
>> -    rb_mgr->ring_pa = 0;
>>       mutex_destroy(&rb_mgr->mutex);
>>   }
>>   @@ -81,35 +114,36 @@ static int tee_wait_cmd_poll(struct psp_tee_device *tee, unsigned int timeout,
>>       return -ETIMEDOUT;
>>   }
>>   -static
>> -struct tee_init_ring_cmd *tee_alloc_cmd_buffer(struct psp_tee_device *tee)
>> +struct dma_buffer *tee_alloc_cmd_buffer(struct psp_tee_device *tee)
>>   {
>>       struct tee_init_ring_cmd *cmd;
>> +    struct dma_buffer *cmd_buffer;
>>   -    cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
>> -    if (!cmd)
>> +    cmd_buffer = psp_tee_alloc_dmabuf(sizeof(*cmd),
>> +                      GFP_KERNEL | __GFP_ZERO);
>> +    if (!cmd_buffer)
>>           return NULL;
>>   -    cmd->hi_addr = upper_32_bits(tee->rb_mgr.ring_pa);
>> -    cmd->low_addr = lower_32_bits(tee->rb_mgr.ring_pa);
>> -    cmd->size = tee->rb_mgr.ring_size;
>> +    cmd = (struct tee_init_ring_cmd *)cmd_buffer->vaddr;
>> +    cmd->hi_addr = upper_32_bits(tee->rb_mgr.ring_buf->paddr);
>> +    cmd->low_addr = lower_32_bits(tee->rb_mgr.ring_buf->paddr);
>> +    cmd->size = tee->rb_mgr.ring_buf->size;
>>         dev_dbg(tee->dev, "tee: ring address: high = 0x%x low = 0x%x size = %u\n",
>>           cmd->hi_addr, cmd->low_addr, cmd->size);
>>   -    return cmd;
>> +    return cmd_buffer;
>>   }
>>   -static inline void tee_free_cmd_buffer(struct tee_init_ring_cmd *cmd)
>> +static inline void tee_free_cmd_buffer(struct dma_buffer *cmd_buffer)
>>   {
>> -    kfree(cmd);
>> +    psp_tee_free_dmabuf(cmd_buffer);
>>   }
>>     static int tee_init_ring(struct psp_tee_device *tee)
>>   {
>>       int ring_size = MAX_RING_BUFFER_ENTRIES * sizeof(struct tee_ring_cmd);
>> -    struct tee_init_ring_cmd *cmd;
>> -    phys_addr_t cmd_buffer;
>> +    struct dma_buffer *cmd_buffer;
>>       unsigned int reg;
>>       int ret;
>>   @@ -123,21 +157,19 @@ static int tee_init_ring(struct psp_tee_device *tee)
>>         tee->rb_mgr.wptr = 0;
>>   -    cmd = tee_alloc_cmd_buffer(tee);
>> -    if (!cmd) {
>> +    cmd_buffer = tee_alloc_cmd_buffer(tee);
>> +    if (!cmd_buffer) {
>>           tee_free_ring(tee);
>>           return -ENOMEM;
>>       }
>>   -    cmd_buffer = __psp_pa((void *)cmd);
>> -
>>       /* Send command buffer details to Trusted OS by writing to
>>        * CPU-PSP message registers
>>        */
>>   -    iowrite32(lower_32_bits(cmd_buffer),
>> +    iowrite32(lower_32_bits(cmd_buffer->paddr),
>>             tee->io_regs + tee->vdata->cmdbuff_addr_lo_reg);
>> -    iowrite32(upper_32_bits(cmd_buffer),
>> +    iowrite32(upper_32_bits(cmd_buffer->paddr),
>>             tee->io_regs + tee->vdata->cmdbuff_addr_hi_reg);
>>       iowrite32(TEE_RING_INIT_CMD,
>>             tee->io_regs + tee->vdata->cmdresp_reg);
>> @@ -157,7 +189,7 @@ static int tee_init_ring(struct psp_tee_device *tee)
>>       }
>>     free_buf:
>> -    tee_free_cmd_buffer(cmd);
>> +    tee_free_cmd_buffer(cmd_buffer);
>>         return ret;
>>   }
>> @@ -167,7 +199,7 @@ static void tee_destroy_ring(struct psp_tee_device *tee)
>>       unsigned int reg;
>>       int ret;
>>   -    if (!tee->rb_mgr.ring_start)
>> +    if (!tee->rb_mgr.ring_buf->vaddr)
>>           return;
>>         if (psp_dead)
>> @@ -256,7 +288,7 @@ static int tee_submit_cmd(struct psp_tee_device *tee, enum tee_cmd_id cmd_id,
>>       do {
>>           /* Get pointer to ring buffer command entry */
>>           cmd = (struct tee_ring_cmd *)
>> -            (tee->rb_mgr.ring_start + tee->rb_mgr.wptr);
>> +            (tee->rb_mgr.ring_buf->vaddr + tee->rb_mgr.wptr);
>>             rptr = ioread32(tee->io_regs + tee->vdata->ring_rptr_reg);
>>   @@ -305,7 +337,7 @@ static int tee_submit_cmd(struct psp_tee_device *tee, enum tee_cmd_id cmd_id,
>>         /* Update local copy of write pointer */
>>       tee->rb_mgr.wptr += sizeof(struct tee_ring_cmd);
>> -    if (tee->rb_mgr.wptr >= tee->rb_mgr.ring_size)
>> +    if (tee->rb_mgr.wptr >= tee->rb_mgr.ring_buf->size)
>>           tee->rb_mgr.wptr = 0;
>>         /* Trigger interrupt to Trusted OS */
>> diff --git a/drivers/crypto/ccp/tee-dev.h b/drivers/crypto/ccp/tee-dev.h
>> index 49d26158b71e..9238487ee8bf 100644
>> --- a/drivers/crypto/ccp/tee-dev.h
>> +++ b/drivers/crypto/ccp/tee-dev.h
>> @@ -16,6 +16,7 @@
>>     #include <linux/device.h>
>>   #include <linux/mutex.h>
>> +#include <linux/psp-tee.h>
>>     #define TEE_DEFAULT_TIMEOUT        10
>>   #define MAX_BUFFER_SIZE            988
>> @@ -48,17 +49,13 @@ struct tee_init_ring_cmd {
>>     /**
>>    * struct ring_buf_manager - Helper structure to manage ring buffer.
>> - * @ring_start:  starting address of ring buffer
>> - * @ring_size:   size of ring buffer in bytes
>> - * @ring_pa:     physical address of ring buffer
>>    * @wptr:        index to the last written entry in ring buffer
>> + * @ring_buf:    ring buffer allocated using DMA api
>>    */
>>   struct ring_buf_manager {
>>       struct mutex mutex;    /* synchronizes access to ring buffer */
>> -    void *ring_start;
>> -    u32 ring_size;
>> -    phys_addr_t ring_pa;
>>       u32 wptr;
>> +    struct dma_buffer *ring_buf;
>>   };
>>     struct psp_tee_device {
>> diff --git a/include/linux/psp-tee.h b/include/linux/psp-tee.h
>> index cb0c95d6d76b..c0fa922f24d4 100644
>> --- a/include/linux/psp-tee.h
>> +++ b/include/linux/psp-tee.h
>> @@ -13,6 +13,7 @@
>>     #include <linux/types.h>
>>   #include <linux/errno.h>
>> +#include <linux/dma-mapping.h>
>>     /* This file defines the Trusted Execution Environment (TEE) interface commands
>>    * and the API exported by AMD Secure Processor driver to communicate with
>> @@ -40,6 +41,20 @@ enum tee_cmd_id {
>>       TEE_CMD_ID_UNMAP_SHARED_MEM,
>>   };
>>   +/**
>> + * struct dma_buffer - Structure for a DMA buffer.
>> + * @dma:    DMA buffer address
>> + * @paddr:  Physical address of DMA buffer
>> + * @vaddr:  CPU virtual address of DMA buffer
>> + * @size:   Size of DMA buffer in bytes
>> + */
>> +struct dma_buffer {
>> +    dma_addr_t dma;
>> +    phys_addr_t paddr;
>> +    void *vaddr;
>> +    unsigned long size;
>> +};
>> +
>>   #ifdef CONFIG_CRYPTO_DEV_SP_PSP
>>   /**
>>    * psp_tee_process_cmd() - Process command in Trusted Execution Environment
>> @@ -75,6 +90,28 @@ int psp_tee_process_cmd(enum tee_cmd_id cmd_id, void *buf, size_t len,
>>    */
>>   int psp_check_tee_status(void);
>>   +/**
>> + * psp_tee_alloc_dmabuf() - Allocates memory of requested size and flags using
>> + * dma_alloc_coherent() API.
>> + *
>> + * This function can be used to allocate a shared memory region between the
>> + * host and PSP TEE.
>> + *
>> + * Returns:
>> + * non-NULL   a valid pointer to struct dma_buffer
>> + * NULL       on failure
>> + */
>> +struct dma_buffer *psp_tee_alloc_dmabuf(unsigned long size, gfp_t gfp);
>> +
>> +/**
>> + * psp_tee_free_dmabuf() - Deallocates memory using dma_free_coherent() API.
>> + *
>> + * This function can be used to release shared memory region between host
>> + * and PSP TEE.
>> + *
>> + */
>> +void psp_tee_free_dmabuf(struct dma_buffer *dma_buffer);
>> +
>>   #else /* !CONFIG_CRYPTO_DEV_SP_PSP */
>>     static inline int psp_tee_process_cmd(enum tee_cmd_id cmd_id, void *buf,
>> @@ -87,5 +124,15 @@ static inline int psp_check_tee_status(void)
>>   {
>>       return -ENODEV;
>>   }
>> +
>> +static inline
>> +struct dma_buffer *psp_tee_alloc_dmabuf(unsigned long size, gfp_t gfp)
>> +{
>> +    return NULL;
>> +}
>> +
>> +static inline void psp_tee_free_dmabuf(struct dma_buffer *dma_buffer)
>> +{
>> +}
>>   #endif /* CONFIG_CRYPTO_DEV_SP_PSP */
>>   #endif /* __PSP_TEE_H_ */
>
  

Am 06.12.22 um 13:54 schrieb Rijo Thomas:
>
> On 12/6/2022 6:11 PM, Christian König wrote:
>> Am 06.12.22 um 13:30 schrieb Rijo Thomas:
>>> For AMD Secure Processor (ASP) to map and access TEE ring buffer, the
>>> ring buffer address sent by host to ASP must be a real physical
>>> address and the pages must be physically contiguous.
>>>
>>> In a virtualized environment though, when the driver is running in a
>>> guest VM, the pages allocated by __get_free_pages() may not be
>>> contiguous in the host (or machine) physical address space. Guests
>>> will see a guest (or pseudo) physical address and not the actual host
>>> (or machine) physical address. The TEE running on ASP cannot decipher
>>> pseudo physical addresses. It needs host or machine physical address.
>>>
>>> To resolve this problem, use DMA APIs for allocating buffers that must
>>> be shared with TEE. This will ensure that the pages are contiguous in
>>> host (or machine) address space. If the DMA handle is an IOVA,
>>> translate it into a physical address before sending it to ASP.
>>>
>>> This patch also exports two APIs (one for buffer allocation and
>>> another to free the buffer). This API can be used by AMD-TEE driver to
>>> share buffers with TEE.
>> Maybe use some other name than dma_buffer for your structure, cause that is usually something completely different in the Linux kernel.
>>
> Sure Christian. I shall rename "struct dma_buffer" to "struct shm_buffer" (Shared Memory Buffer) in the file include/linux/psp-tee.h
> The functions psp_tee_alloc_dmabuf() and psp_tee_free_dmabuf() shall be renamed to psp_tee_alloc_shmbuf() and psp_tee_free_shmbuf(), respectively.
> I shall do correction in next patch revision. I will wait for others to review as well before I post update.

I strongly suggest to use the name psp_buffer or something similar 
because shm_buffer is usually used for something else as well.

Regards,
Christian.

>
> Thanks,
> Rijo
>
>> Regards,
>> Christian.
>>
>>> Fixes: 33960acccfbd ("crypto: ccp - add TEE support for Raven Ridge")
>>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Rijo Thomas <Rijo-john.Thomas@amd.com>
>>> Co-developed-by: Jeshwanth <JESHWANTHKUMAR.NK@amd.com>
>>> Signed-off-by: Jeshwanth <JESHWANTHKUMAR.NK@amd.com>
>>> Reviewed-by: Devaraj Rangasamy <Devaraj.Rangasamy@amd.com>
>>> ---
>>>    drivers/crypto/ccp/psp-dev.c |   6 +-
>>>    drivers/crypto/ccp/tee-dev.c | 116 ++++++++++++++++++++++-------------
>>>    drivers/crypto/ccp/tee-dev.h |   9 +--
>>>    include/linux/psp-tee.h      |  47 ++++++++++++++
>>>    4 files changed, 127 insertions(+), 51 deletions(-)
>>>
>>> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
>>> index c9c741ac8442..2b86158d7435 100644
>>> --- a/drivers/crypto/ccp/psp-dev.c
>>> +++ b/drivers/crypto/ccp/psp-dev.c
>>> @@ -161,13 +161,13 @@ int psp_dev_init(struct sp_device *sp)
>>>            goto e_err;
>>>        }
>>>    +    if (sp->set_psp_master_device)
>>> +        sp->set_psp_master_device(sp);
>>> +
>>>        ret = psp_init(psp);
>>>        if (ret)
>>>            goto e_irq;
>>>    -    if (sp->set_psp_master_device)
>>> -        sp->set_psp_master_device(sp);
>>> -
>>>        /* Enable interrupt */
>>>        iowrite32(-1, psp->io_regs + psp->vdata->inten_reg);
>>>    diff --git a/drivers/crypto/ccp/tee-dev.c b/drivers/crypto/ccp/tee-dev.c
>>> index 5c9d47f3be37..1631d9851e54 100644
>>> --- a/drivers/crypto/ccp/tee-dev.c
>>> +++ b/drivers/crypto/ccp/tee-dev.c
>>> @@ -12,8 +12,9 @@
>>>    #include <linux/mutex.h>
>>>    #include <linux/delay.h>
>>>    #include <linux/slab.h>
>>> +#include <linux/dma-direct.h>
>>> +#include <linux/iommu.h>
>>>    #include <linux/gfp.h>
>>> -#include <linux/psp-sev.h>
>>>    #include <linux/psp-tee.h>
>>>      #include "psp-dev.h"
>>> @@ -21,25 +22,64 @@
>>>      static bool psp_dead;
>>>    +struct dma_buffer *psp_tee_alloc_dmabuf(unsigned long size, gfp_t gfp)
>>> +{
>>> +    struct psp_device *psp = psp_get_master_device();
>>> +    struct dma_buffer *dma_buf;
>>> +    struct iommu_domain *dom;
>>> +
>>> +    if (!psp || !size)
>>> +        return NULL;
>>> +
>>> +    dma_buf = kzalloc(sizeof(*dma_buf), GFP_KERNEL);
>>> +    if (!dma_buf)
>>> +        return NULL;
>>> +
>>> +    dma_buf->vaddr = dma_alloc_coherent(psp->dev, size, &dma_buf->dma, gfp);
>>> +    if (!dma_buf->vaddr || !dma_buf->dma) {
>>> +        kfree(dma_buf);
>>> +        return NULL;
>>> +    }
>>> +
>>> +    dma_buf->size = size;
>>> +
>>> +    dom = iommu_get_domain_for_dev(psp->dev);
>>> +    if (dom)
>>> +        dma_buf->paddr = iommu_iova_to_phys(dom, dma_buf->dma);
>>> +    else
>>> +        dma_buf->paddr = dma_buf->dma;
>>> +
>>> +    return dma_buf;
>>> +}
>>> +EXPORT_SYMBOL(psp_tee_alloc_dmabuf);
>>> +
>>> +void psp_tee_free_dmabuf(struct dma_buffer *dma_buf)
>>> +{
>>> +    struct psp_device *psp = psp_get_master_device();
>>> +
>>> +    if (!psp || !dma_buf)
>>> +        return;
>>> +
>>> +    dma_free_coherent(psp->dev, dma_buf->size,
>>> +              dma_buf->vaddr, dma_buf->dma);
>>> +
>>> +    kfree(dma_buf);
>>> +}
>>> +EXPORT_SYMBOL(psp_tee_free_dmabuf);
>>> +
>>>    static int tee_alloc_ring(struct psp_tee_device *tee, int ring_size)
>>>    {
>>>        struct ring_buf_manager *rb_mgr = &tee->rb_mgr;
>>> -    void *start_addr;
>>>          if (!ring_size)
>>>            return -EINVAL;
>>>    -    /* We need actual physical address instead of DMA address, since
>>> -     * Trusted OS running on AMD Secure Processor will map this region
>>> -     */
>>> -    start_addr = (void *)__get_free_pages(GFP_KERNEL, get_order(ring_size));
>>> -    if (!start_addr)
>>> +    rb_mgr->ring_buf = psp_tee_alloc_dmabuf(ring_size,
>>> +                        GFP_KERNEL | __GFP_ZERO);
>>> +    if (!rb_mgr->ring_buf) {
>>> +        dev_err(tee->dev, "ring allocation failed\n");
>>>            return -ENOMEM;
>>> -
>>> -    memset(start_addr, 0x0, ring_size);
>>> -    rb_mgr->ring_start = start_addr;
>>> -    rb_mgr->ring_size = ring_size;
>>> -    rb_mgr->ring_pa = __psp_pa(start_addr);
>>> +    }
>>>        mutex_init(&rb_mgr->mutex);
>>>          return 0;
>>> @@ -49,15 +89,8 @@ static void tee_free_ring(struct psp_tee_device *tee)
>>>    {
>>>        struct ring_buf_manager *rb_mgr = &tee->rb_mgr;
>>>    -    if (!rb_mgr->ring_start)
>>> -        return;
>>> +    psp_tee_free_dmabuf(rb_mgr->ring_buf);
>>>    -    free_pages((unsigned long)rb_mgr->ring_start,
>>> -           get_order(rb_mgr->ring_size));
>>> -
>>> -    rb_mgr->ring_start = NULL;
>>> -    rb_mgr->ring_size = 0;
>>> -    rb_mgr->ring_pa = 0;
>>>        mutex_destroy(&rb_mgr->mutex);
>>>    }
>>>    @@ -81,35 +114,36 @@ static int tee_wait_cmd_poll(struct psp_tee_device *tee, unsigned int timeout,
>>>        return -ETIMEDOUT;
>>>    }
>>>    -static
>>> -struct tee_init_ring_cmd *tee_alloc_cmd_buffer(struct psp_tee_device *tee)
>>> +struct dma_buffer *tee_alloc_cmd_buffer(struct psp_tee_device *tee)
>>>    {
>>>        struct tee_init_ring_cmd *cmd;
>>> +    struct dma_buffer *cmd_buffer;
>>>    -    cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
>>> -    if (!cmd)
>>> +    cmd_buffer = psp_tee_alloc_dmabuf(sizeof(*cmd),
>>> +                      GFP_KERNEL | __GFP_ZERO);
>>> +    if (!cmd_buffer)
>>>            return NULL;
>>>    -    cmd->hi_addr = upper_32_bits(tee->rb_mgr.ring_pa);
>>> -    cmd->low_addr = lower_32_bits(tee->rb_mgr.ring_pa);
>>> -    cmd->size = tee->rb_mgr.ring_size;
>>> +    cmd = (struct tee_init_ring_cmd *)cmd_buffer->vaddr;
>>> +    cmd->hi_addr = upper_32_bits(tee->rb_mgr.ring_buf->paddr);
>>> +    cmd->low_addr = lower_32_bits(tee->rb_mgr.ring_buf->paddr);
>>> +    cmd->size = tee->rb_mgr.ring_buf->size;
>>>          dev_dbg(tee->dev, "tee: ring address: high = 0x%x low = 0x%x size = %u\n",
>>>            cmd->hi_addr, cmd->low_addr, cmd->size);
>>>    -    return cmd;
>>> +    return cmd_buffer;
>>>    }
>>>    -static inline void tee_free_cmd_buffer(struct tee_init_ring_cmd *cmd)
>>> +static inline void tee_free_cmd_buffer(struct dma_buffer *cmd_buffer)
>>>    {
>>> -    kfree(cmd);
>>> +    psp_tee_free_dmabuf(cmd_buffer);
>>>    }
>>>      static int tee_init_ring(struct psp_tee_device *tee)
>>>    {
>>>        int ring_size = MAX_RING_BUFFER_ENTRIES * sizeof(struct tee_ring_cmd);
>>> -    struct tee_init_ring_cmd *cmd;
>>> -    phys_addr_t cmd_buffer;
>>> +    struct dma_buffer *cmd_buffer;
>>>        unsigned int reg;
>>>        int ret;
>>>    @@ -123,21 +157,19 @@ static int tee_init_ring(struct psp_tee_device *tee)
>>>          tee->rb_mgr.wptr = 0;
>>>    -    cmd = tee_alloc_cmd_buffer(tee);
>>> -    if (!cmd) {
>>> +    cmd_buffer = tee_alloc_cmd_buffer(tee);
>>> +    if (!cmd_buffer) {
>>>            tee_free_ring(tee);
>>>            return -ENOMEM;
>>>        }
>>>    -    cmd_buffer = __psp_pa((void *)cmd);
>>> -
>>>        /* Send command buffer details to Trusted OS by writing to
>>>         * CPU-PSP message registers
>>>         */
>>>    -    iowrite32(lower_32_bits(cmd_buffer),
>>> +    iowrite32(lower_32_bits(cmd_buffer->paddr),
>>>              tee->io_regs + tee->vdata->cmdbuff_addr_lo_reg);
>>> -    iowrite32(upper_32_bits(cmd_buffer),
>>> +    iowrite32(upper_32_bits(cmd_buffer->paddr),
>>>              tee->io_regs + tee->vdata->cmdbuff_addr_hi_reg);
>>>        iowrite32(TEE_RING_INIT_CMD,
>>>              tee->io_regs + tee->vdata->cmdresp_reg);
>>> @@ -157,7 +189,7 @@ static int tee_init_ring(struct psp_tee_device *tee)
>>>        }
>>>      free_buf:
>>> -    tee_free_cmd_buffer(cmd);
>>> +    tee_free_cmd_buffer(cmd_buffer);
>>>          return ret;
>>>    }
>>> @@ -167,7 +199,7 @@ static void tee_destroy_ring(struct psp_tee_device *tee)
>>>        unsigned int reg;
>>>        int ret;
>>>    -    if (!tee->rb_mgr.ring_start)
>>> +    if (!tee->rb_mgr.ring_buf->vaddr)
>>>            return;
>>>          if (psp_dead)
>>> @@ -256,7 +288,7 @@ static int tee_submit_cmd(struct psp_tee_device *tee, enum tee_cmd_id cmd_id,
>>>        do {
>>>            /* Get pointer to ring buffer command entry */
>>>            cmd = (struct tee_ring_cmd *)
>>> -            (tee->rb_mgr.ring_start + tee->rb_mgr.wptr);
>>> +            (tee->rb_mgr.ring_buf->vaddr + tee->rb_mgr.wptr);
>>>              rptr = ioread32(tee->io_regs + tee->vdata->ring_rptr_reg);
>>>    @@ -305,7 +337,7 @@ static int tee_submit_cmd(struct psp_tee_device *tee, enum tee_cmd_id cmd_id,
>>>          /* Update local copy of write pointer */
>>>        tee->rb_mgr.wptr += sizeof(struct tee_ring_cmd);
>>> -    if (tee->rb_mgr.wptr >= tee->rb_mgr.ring_size)
>>> +    if (tee->rb_mgr.wptr >= tee->rb_mgr.ring_buf->size)
>>>            tee->rb_mgr.wptr = 0;
>>>          /* Trigger interrupt to Trusted OS */
>>> diff --git a/drivers/crypto/ccp/tee-dev.h b/drivers/crypto/ccp/tee-dev.h
>>> index 49d26158b71e..9238487ee8bf 100644
>>> --- a/drivers/crypto/ccp/tee-dev.h
>>> +++ b/drivers/crypto/ccp/tee-dev.h
>>> @@ -16,6 +16,7 @@
>>>      #include <linux/device.h>
>>>    #include <linux/mutex.h>
>>> +#include <linux/psp-tee.h>
>>>      #define TEE_DEFAULT_TIMEOUT        10
>>>    #define MAX_BUFFER_SIZE            988
>>> @@ -48,17 +49,13 @@ struct tee_init_ring_cmd {
>>>      /**
>>>     * struct ring_buf_manager - Helper structure to manage ring buffer.
>>> - * @ring_start:  starting address of ring buffer
>>> - * @ring_size:   size of ring buffer in bytes
>>> - * @ring_pa:     physical address of ring buffer
>>>     * @wptr:        index to the last written entry in ring buffer
>>> + * @ring_buf:    ring buffer allocated using DMA api
>>>     */
>>>    struct ring_buf_manager {
>>>        struct mutex mutex;    /* synchronizes access to ring buffer */
>>> -    void *ring_start;
>>> -    u32 ring_size;
>>> -    phys_addr_t ring_pa;
>>>        u32 wptr;
>>> +    struct dma_buffer *ring_buf;
>>>    };
>>>      struct psp_tee_device {
>>> diff --git a/include/linux/psp-tee.h b/include/linux/psp-tee.h
>>> index cb0c95d6d76b..c0fa922f24d4 100644
>>> --- a/include/linux/psp-tee.h
>>> +++ b/include/linux/psp-tee.h
>>> @@ -13,6 +13,7 @@
>>>      #include <linux/types.h>
>>>    #include <linux/errno.h>
>>> +#include <linux/dma-mapping.h>
>>>      /* This file defines the Trusted Execution Environment (TEE) interface commands
>>>     * and the API exported by AMD Secure Processor driver to communicate with
>>> @@ -40,6 +41,20 @@ enum tee_cmd_id {
>>>        TEE_CMD_ID_UNMAP_SHARED_MEM,
>>>    };
>>>    +/**
>>> + * struct dma_buffer - Structure for a DMA buffer.
>>> + * @dma:    DMA buffer address
>>> + * @paddr:  Physical address of DMA buffer
>>> + * @vaddr:  CPU virtual address of DMA buffer
>>> + * @size:   Size of DMA buffer in bytes
>>> + */
>>> +struct dma_buffer {
>>> +    dma_addr_t dma;
>>> +    phys_addr_t paddr;
>>> +    void *vaddr;
>>> +    unsigned long size;
>>> +};
>>> +
>>>    #ifdef CONFIG_CRYPTO_DEV_SP_PSP
>>>    /**
>>>     * psp_tee_process_cmd() - Process command in Trusted Execution Environment
>>> @@ -75,6 +90,28 @@ int psp_tee_process_cmd(enum tee_cmd_id cmd_id, void *buf, size_t len,
>>>     */
>>>    int psp_check_tee_status(void);
>>>    +/**
>>> + * psp_tee_alloc_dmabuf() - Allocates memory of requested size and flags using
>>> + * dma_alloc_coherent() API.
>>> + *
>>> + * This function can be used to allocate a shared memory region between the
>>> + * host and PSP TEE.
>>> + *
>>> + * Returns:
>>> + * non-NULL   a valid pointer to struct dma_buffer
>>> + * NULL       on failure
>>> + */
>>> +struct dma_buffer *psp_tee_alloc_dmabuf(unsigned long size, gfp_t gfp);
>>> +
>>> +/**
>>> + * psp_tee_free_dmabuf() - Deallocates memory using dma_free_coherent() API.
>>> + *
>>> + * This function can be used to release shared memory region between host
>>> + * and PSP TEE.
>>> + *
>>> + */
>>> +void psp_tee_free_dmabuf(struct dma_buffer *dma_buffer);
>>> +
>>>    #else /* !CONFIG_CRYPTO_DEV_SP_PSP */
>>>      static inline int psp_tee_process_cmd(enum tee_cmd_id cmd_id, void *buf,
>>> @@ -87,5 +124,15 @@ static inline int psp_check_tee_status(void)
>>>    {
>>>        return -ENODEV;
>>>    }
>>> +
>>> +static inline
>>> +struct dma_buffer *psp_tee_alloc_dmabuf(unsigned long size, gfp_t gfp)
>>> +{
>>> +    return NULL;
>>> +}
>>> +
>>> +static inline void psp_tee_free_dmabuf(struct dma_buffer *dma_buffer)
>>> +{
>>> +}
>>>    #endif /* CONFIG_CRYPTO_DEV_SP_PSP */
>>>    #endif /* __PSP_TEE_H_ */
  

On 12/6/2022 6:26 PM, Christian König wrote:
> Am 06.12.22 um 13:54 schrieb Rijo Thomas:
>>
>> On 12/6/2022 6:11 PM, Christian König wrote:
>>> Am 06.12.22 um 13:30 schrieb Rijo Thomas:
>>>> For AMD Secure Processor (ASP) to map and access TEE ring buffer, the
>>>> ring buffer address sent by host to ASP must be a real physical
>>>> address and the pages must be physically contiguous.
>>>>
>>>> In a virtualized environment though, when the driver is running in a
>>>> guest VM, the pages allocated by __get_free_pages() may not be
>>>> contiguous in the host (or machine) physical address space. Guests
>>>> will see a guest (or pseudo) physical address and not the actual host
>>>> (or machine) physical address. The TEE running on ASP cannot decipher
>>>> pseudo physical addresses. It needs host or machine physical address.
>>>>
>>>> To resolve this problem, use DMA APIs for allocating buffers that must
>>>> be shared with TEE. This will ensure that the pages are contiguous in
>>>> host (or machine) address space. If the DMA handle is an IOVA,
>>>> translate it into a physical address before sending it to ASP.
>>>>
>>>> This patch also exports two APIs (one for buffer allocation and
>>>> another to free the buffer). This API can be used by AMD-TEE driver to
>>>> share buffers with TEE.
>>> Maybe use some other name than dma_buffer for your structure, cause that is usually something completely different in the Linux kernel.
>>>
>> Sure Christian. I shall rename "struct dma_buffer" to "struct shm_buffer" (Shared Memory Buffer) in the file include/linux/psp-tee.h
>> The functions psp_tee_alloc_dmabuf() and psp_tee_free_dmabuf() shall be renamed to psp_tee_alloc_shmbuf() and psp_tee_free_shmbuf(), respectively.
>> I shall do correction in next patch revision. I will wait for others to review as well before I post update.
> 
> I strongly suggest to use the name psp_buffer or something similar because shm_buffer is usually used for something else as well.

I see that the TEE subsystem defines "struct tee_shm".

Okay, I will name the newly added structure as "struct psp_tee_buffer" and the APIs as psp_tee_alloc_buffer() and psp_tee_free_buffer().

Complete function prototype:

struct psp_tee_buffer *psp_tee_alloc_buffer(unsigned long size, gfp_t gfp);

void psp_tee_free_buffer(struct psp_tee_buffer *buffer);

Does this look okay?

Thanks,
Rijo

> 
> Regards,
> Christian.
> 
>>
>> Thanks,
>> Rijo
>>
>>> Regards,
>>> Christian.
>>>
>>>> Fixes: 33960acccfbd ("crypto: ccp - add TEE support for Raven Ridge")
>>>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>>>> Cc: stable@vger.kernel.org
>>>> Signed-off-by: Rijo Thomas <Rijo-john.Thomas@amd.com>
>>>> Co-developed-by: Jeshwanth <JESHWANTHKUMAR.NK@amd.com>
>>>> Signed-off-by: Jeshwanth <JESHWANTHKUMAR.NK@amd.com>
>>>> Reviewed-by: Devaraj Rangasamy <Devaraj.Rangasamy@amd.com>
>>>> ---
>>>>    drivers/crypto/ccp/psp-dev.c |   6 +-
>>>>    drivers/crypto/ccp/tee-dev.c | 116 ++++++++++++++++++++++-------------
>>>>    drivers/crypto/ccp/tee-dev.h |   9 +--
>>>>    include/linux/psp-tee.h      |  47 ++++++++++++++
>>>>    4 files changed, 127 insertions(+), 51 deletions(-)
>>>>
>>>> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
>>>> index c9c741ac8442..2b86158d7435 100644
>>>> --- a/drivers/crypto/ccp/psp-dev.c
>>>> +++ b/drivers/crypto/ccp/psp-dev.c
>>>> @@ -161,13 +161,13 @@ int psp_dev_init(struct sp_device *sp)
>>>>            goto e_err;
>>>>        }
>>>>    +    if (sp->set_psp_master_device)
>>>> +        sp->set_psp_master_device(sp);
>>>> +
>>>>        ret = psp_init(psp);
>>>>        if (ret)
>>>>            goto e_irq;
>>>>    -    if (sp->set_psp_master_device)
>>>> -        sp->set_psp_master_device(sp);
>>>> -
>>>>        /* Enable interrupt */
>>>>        iowrite32(-1, psp->io_regs + psp->vdata->inten_reg);
>>>>    diff --git a/drivers/crypto/ccp/tee-dev.c b/drivers/crypto/ccp/tee-dev.c
>>>> index 5c9d47f3be37..1631d9851e54 100644
>>>> --- a/drivers/crypto/ccp/tee-dev.c
>>>> +++ b/drivers/crypto/ccp/tee-dev.c
>>>> @@ -12,8 +12,9 @@
>>>>    #include <linux/mutex.h>
>>>>    #include <linux/delay.h>
>>>>    #include <linux/slab.h>
>>>> +#include <linux/dma-direct.h>
>>>> +#include <linux/iommu.h>
>>>>    #include <linux/gfp.h>
>>>> -#include <linux/psp-sev.h>
>>>>    #include <linux/psp-tee.h>
>>>>      #include "psp-dev.h"
>>>> @@ -21,25 +22,64 @@
>>>>      static bool psp_dead;
>>>>    +struct dma_buffer *psp_tee_alloc_dmabuf(unsigned long size, gfp_t gfp)
>>>> +{
>>>> +    struct psp_device *psp = psp_get_master_device();
>>>> +    struct dma_buffer *dma_buf;
>>>> +    struct iommu_domain *dom;
>>>> +
>>>> +    if (!psp || !size)
>>>> +        return NULL;
>>>> +
>>>> +    dma_buf = kzalloc(sizeof(*dma_buf), GFP_KERNEL);
>>>> +    if (!dma_buf)
>>>> +        return NULL;
>>>> +
>>>> +    dma_buf->vaddr = dma_alloc_coherent(psp->dev, size, &dma_buf->dma, gfp);
>>>> +    if (!dma_buf->vaddr || !dma_buf->dma) {
>>>> +        kfree(dma_buf);
>>>> +        return NULL;
>>>> +    }
>>>> +
>>>> +    dma_buf->size = size;
>>>> +
>>>> +    dom = iommu_get_domain_for_dev(psp->dev);
>>>> +    if (dom)
>>>> +        dma_buf->paddr = iommu_iova_to_phys(dom, dma_buf->dma);
>>>> +    else
>>>> +        dma_buf->paddr = dma_buf->dma;
>>>> +
>>>> +    return dma_buf;
>>>> +}
>>>> +EXPORT_SYMBOL(psp_tee_alloc_dmabuf);
>>>> +
>>>> +void psp_tee_free_dmabuf(struct dma_buffer *dma_buf)
>>>> +{
>>>> +    struct psp_device *psp = psp_get_master_device();
>>>> +
>>>> +    if (!psp || !dma_buf)
>>>> +        return;
>>>> +
>>>> +    dma_free_coherent(psp->dev, dma_buf->size,
>>>> +              dma_buf->vaddr, dma_buf->dma);
>>>> +
>>>> +    kfree(dma_buf);
>>>> +}
>>>> +EXPORT_SYMBOL(psp_tee_free_dmabuf);
>>>> +
>>>>    static int tee_alloc_ring(struct psp_tee_device *tee, int ring_size)
>>>>    {
>>>>        struct ring_buf_manager *rb_mgr = &tee->rb_mgr;
>>>> -    void *start_addr;
>>>>          if (!ring_size)
>>>>            return -EINVAL;
>>>>    -    /* We need actual physical address instead of DMA address, since
>>>> -     * Trusted OS running on AMD Secure Processor will map this region
>>>> -     */
>>>> -    start_addr = (void *)__get_free_pages(GFP_KERNEL, get_order(ring_size));
>>>> -    if (!start_addr)
>>>> +    rb_mgr->ring_buf = psp_tee_alloc_dmabuf(ring_size,
>>>> +                        GFP_KERNEL | __GFP_ZERO);
>>>> +    if (!rb_mgr->ring_buf) {
>>>> +        dev_err(tee->dev, "ring allocation failed\n");
>>>>            return -ENOMEM;
>>>> -
>>>> -    memset(start_addr, 0x0, ring_size);
>>>> -    rb_mgr->ring_start = start_addr;
>>>> -    rb_mgr->ring_size = ring_size;
>>>> -    rb_mgr->ring_pa = __psp_pa(start_addr);
>>>> +    }
>>>>        mutex_init(&rb_mgr->mutex);
>>>>          return 0;
>>>> @@ -49,15 +89,8 @@ static void tee_free_ring(struct psp_tee_device *tee)
>>>>    {
>>>>        struct ring_buf_manager *rb_mgr = &tee->rb_mgr;
>>>>    -    if (!rb_mgr->ring_start)
>>>> -        return;
>>>> +    psp_tee_free_dmabuf(rb_mgr->ring_buf);
>>>>    -    free_pages((unsigned long)rb_mgr->ring_start,
>>>> -           get_order(rb_mgr->ring_size));
>>>> -
>>>> -    rb_mgr->ring_start = NULL;
>>>> -    rb_mgr->ring_size = 0;
>>>> -    rb_mgr->ring_pa = 0;
>>>>        mutex_destroy(&rb_mgr->mutex);
>>>>    }
>>>>    @@ -81,35 +114,36 @@ static int tee_wait_cmd_poll(struct psp_tee_device *tee, unsigned int timeout,
>>>>        return -ETIMEDOUT;
>>>>    }
>>>>    -static
>>>> -struct tee_init_ring_cmd *tee_alloc_cmd_buffer(struct psp_tee_device *tee)
>>>> +struct dma_buffer *tee_alloc_cmd_buffer(struct psp_tee_device *tee)
>>>>    {
>>>>        struct tee_init_ring_cmd *cmd;
>>>> +    struct dma_buffer *cmd_buffer;
>>>>    -    cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
>>>> -    if (!cmd)
>>>> +    cmd_buffer = psp_tee_alloc_dmabuf(sizeof(*cmd),
>>>> +                      GFP_KERNEL | __GFP_ZERO);
>>>> +    if (!cmd_buffer)
>>>>            return NULL;
>>>>    -    cmd->hi_addr = upper_32_bits(tee->rb_mgr.ring_pa);
>>>> -    cmd->low_addr = lower_32_bits(tee->rb_mgr.ring_pa);
>>>> -    cmd->size = tee->rb_mgr.ring_size;
>>>> +    cmd = (struct tee_init_ring_cmd *)cmd_buffer->vaddr;
>>>> +    cmd->hi_addr = upper_32_bits(tee->rb_mgr.ring_buf->paddr);
>>>> +    cmd->low_addr = lower_32_bits(tee->rb_mgr.ring_buf->paddr);
>>>> +    cmd->size = tee->rb_mgr.ring_buf->size;
>>>>          dev_dbg(tee->dev, "tee: ring address: high = 0x%x low = 0x%x size = %u\n",
>>>>            cmd->hi_addr, cmd->low_addr, cmd->size);
>>>>    -    return cmd;
>>>> +    return cmd_buffer;
>>>>    }
>>>>    -static inline void tee_free_cmd_buffer(struct tee_init_ring_cmd *cmd)
>>>> +static inline void tee_free_cmd_buffer(struct dma_buffer *cmd_buffer)
>>>>    {
>>>> -    kfree(cmd);
>>>> +    psp_tee_free_dmabuf(cmd_buffer);
>>>>    }
>>>>      static int tee_init_ring(struct psp_tee_device *tee)
>>>>    {
>>>>        int ring_size = MAX_RING_BUFFER_ENTRIES * sizeof(struct tee_ring_cmd);
>>>> -    struct tee_init_ring_cmd *cmd;
>>>> -    phys_addr_t cmd_buffer;
>>>> +    struct dma_buffer *cmd_buffer;
>>>>        unsigned int reg;
>>>>        int ret;
>>>>    @@ -123,21 +157,19 @@ static int tee_init_ring(struct psp_tee_device *tee)
>>>>          tee->rb_mgr.wptr = 0;
>>>>    -    cmd = tee_alloc_cmd_buffer(tee);
>>>> -    if (!cmd) {
>>>> +    cmd_buffer = tee_alloc_cmd_buffer(tee);
>>>> +    if (!cmd_buffer) {
>>>>            tee_free_ring(tee);
>>>>            return -ENOMEM;
>>>>        }
>>>>    -    cmd_buffer = __psp_pa((void *)cmd);
>>>> -
>>>>        /* Send command buffer details to Trusted OS by writing to
>>>>         * CPU-PSP message registers
>>>>         */
>>>>    -    iowrite32(lower_32_bits(cmd_buffer),
>>>> +    iowrite32(lower_32_bits(cmd_buffer->paddr),
>>>>              tee->io_regs + tee->vdata->cmdbuff_addr_lo_reg);
>>>> -    iowrite32(upper_32_bits(cmd_buffer),
>>>> +    iowrite32(upper_32_bits(cmd_buffer->paddr),
>>>>              tee->io_regs + tee->vdata->cmdbuff_addr_hi_reg);
>>>>        iowrite32(TEE_RING_INIT_CMD,
>>>>              tee->io_regs + tee->vdata->cmdresp_reg);
>>>> @@ -157,7 +189,7 @@ static int tee_init_ring(struct psp_tee_device *tee)
>>>>        }
>>>>      free_buf:
>>>> -    tee_free_cmd_buffer(cmd);
>>>> +    tee_free_cmd_buffer(cmd_buffer);
>>>>          return ret;
>>>>    }
>>>> @@ -167,7 +199,7 @@ static void tee_destroy_ring(struct psp_tee_device *tee)
>>>>        unsigned int reg;
>>>>        int ret;
>>>>    -    if (!tee->rb_mgr.ring_start)
>>>> +    if (!tee->rb_mgr.ring_buf->vaddr)
>>>>            return;
>>>>          if (psp_dead)
>>>> @@ -256,7 +288,7 @@ static int tee_submit_cmd(struct psp_tee_device *tee, enum tee_cmd_id cmd_id,
>>>>        do {
>>>>            /* Get pointer to ring buffer command entry */
>>>>            cmd = (struct tee_ring_cmd *)
>>>> -            (tee->rb_mgr.ring_start + tee->rb_mgr.wptr);
>>>> +            (tee->rb_mgr.ring_buf->vaddr + tee->rb_mgr.wptr);
>>>>              rptr = ioread32(tee->io_regs + tee->vdata->ring_rptr_reg);
>>>>    @@ -305,7 +337,7 @@ static int tee_submit_cmd(struct psp_tee_device *tee, enum tee_cmd_id cmd_id,
>>>>          /* Update local copy of write pointer */
>>>>        tee->rb_mgr.wptr += sizeof(struct tee_ring_cmd);
>>>> -    if (tee->rb_mgr.wptr >= tee->rb_mgr.ring_size)
>>>> +    if (tee->rb_mgr.wptr >= tee->rb_mgr.ring_buf->size)
>>>>            tee->rb_mgr.wptr = 0;
>>>>          /* Trigger interrupt to Trusted OS */
>>>> diff --git a/drivers/crypto/ccp/tee-dev.h b/drivers/crypto/ccp/tee-dev.h
>>>> index 49d26158b71e..9238487ee8bf 100644
>>>> --- a/drivers/crypto/ccp/tee-dev.h
>>>> +++ b/drivers/crypto/ccp/tee-dev.h
>>>> @@ -16,6 +16,7 @@
>>>>      #include <linux/device.h>
>>>>    #include <linux/mutex.h>
>>>> +#include <linux/psp-tee.h>
>>>>      #define TEE_DEFAULT_TIMEOUT        10
>>>>    #define MAX_BUFFER_SIZE            988
>>>> @@ -48,17 +49,13 @@ struct tee_init_ring_cmd {
>>>>      /**
>>>>     * struct ring_buf_manager - Helper structure to manage ring buffer.
>>>> - * @ring_start:  starting address of ring buffer
>>>> - * @ring_size:   size of ring buffer in bytes
>>>> - * @ring_pa:     physical address of ring buffer
>>>>     * @wptr:        index to the last written entry in ring buffer
>>>> + * @ring_buf:    ring buffer allocated using DMA api
>>>>     */
>>>>    struct ring_buf_manager {
>>>>        struct mutex mutex;    /* synchronizes access to ring buffer */
>>>> -    void *ring_start;
>>>> -    u32 ring_size;
>>>> -    phys_addr_t ring_pa;
>>>>        u32 wptr;
>>>> +    struct dma_buffer *ring_buf;
>>>>    };
>>>>      struct psp_tee_device {
>>>> diff --git a/include/linux/psp-tee.h b/include/linux/psp-tee.h
>>>> index cb0c95d6d76b..c0fa922f24d4 100644
>>>> --- a/include/linux/psp-tee.h
>>>> +++ b/include/linux/psp-tee.h
>>>> @@ -13,6 +13,7 @@
>>>>      #include <linux/types.h>
>>>>    #include <linux/errno.h>
>>>> +#include <linux/dma-mapping.h>
>>>>      /* This file defines the Trusted Execution Environment (TEE) interface commands
>>>>     * and the API exported by AMD Secure Processor driver to communicate with
>>>> @@ -40,6 +41,20 @@ enum tee_cmd_id {
>>>>        TEE_CMD_ID_UNMAP_SHARED_MEM,
>>>>    };
>>>>    +/**
>>>> + * struct dma_buffer - Structure for a DMA buffer.
>>>> + * @dma:    DMA buffer address
>>>> + * @paddr:  Physical address of DMA buffer
>>>> + * @vaddr:  CPU virtual address of DMA buffer
>>>> + * @size:   Size of DMA buffer in bytes
>>>> + */
>>>> +struct dma_buffer {
>>>> +    dma_addr_t dma;
>>>> +    phys_addr_t paddr;
>>>> +    void *vaddr;
>>>> +    unsigned long size;
>>>> +};
>>>> +
>>>>    #ifdef CONFIG_CRYPTO_DEV_SP_PSP
>>>>    /**
>>>>     * psp_tee_process_cmd() - Process command in Trusted Execution Environment
>>>> @@ -75,6 +90,28 @@ int psp_tee_process_cmd(enum tee_cmd_id cmd_id, void *buf, size_t len,
>>>>     */
>>>>    int psp_check_tee_status(void);
>>>>    +/**
>>>> + * psp_tee_alloc_dmabuf() - Allocates memory of requested size and flags using
>>>> + * dma_alloc_coherent() API.
>>>> + *
>>>> + * This function can be used to allocate a shared memory region between the
>>>> + * host and PSP TEE.
>>>> + *
>>>> + * Returns:
>>>> + * non-NULL   a valid pointer to struct dma_buffer
>>>> + * NULL       on failure
>>>> + */
>>>> +struct dma_buffer *psp_tee_alloc_dmabuf(unsigned long size, gfp_t gfp);
>>>> +
>>>> +/**
>>>> + * psp_tee_free_dmabuf() - Deallocates memory using dma_free_coherent() API.
>>>> + *
>>>> + * This function can be used to release shared memory region between host
>>>> + * and PSP TEE.
>>>> + *
>>>> + */
>>>> +void psp_tee_free_dmabuf(struct dma_buffer *dma_buffer);
>>>> +
>>>>    #else /* !CONFIG_CRYPTO_DEV_SP_PSP */
>>>>      static inline int psp_tee_process_cmd(enum tee_cmd_id cmd_id, void *buf,
>>>> @@ -87,5 +124,15 @@ static inline int psp_check_tee_status(void)
>>>>    {
>>>>        return -ENODEV;
>>>>    }
>>>> +
>>>> +static inline
>>>> +struct dma_buffer *psp_tee_alloc_dmabuf(unsigned long size, gfp_t gfp)
>>>> +{
>>>> +    return NULL;
>>>> +}
>>>> +
>>>> +static inline void psp_tee_free_dmabuf(struct dma_buffer *dma_buffer)
>>>> +{
>>>> +}
>>>>    #endif /* CONFIG_CRYPTO_DEV_SP_PSP */
>>>>    #endif /* __PSP_TEE_H_ */
>
  

Hi Rijo,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on herbert-cryptodev-2.6/master]
[also build test WARNING on herbert-crypto-2.6/master linus/master v6.1-rc8 next-20221206]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Rijo-Thomas/crypto-ccp-Allocate-TEE-ring-and-cmd-buffer-using-DMA-APIs/20221206-203201
base:   https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master
patch link:    https://lore.kernel.org/r/43568d5e6395fcab48262fa5b3d1a5112918fbe8.1669372199.git.Rijo-john.Thomas%40amd.com
patch subject: [PATCH 1/1] crypto: ccp - Allocate TEE ring and cmd buffer using DMA APIs
config: x86_64-allyesconfig
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/5615778a8f19c66bbacb611c78175e3130aa22c0
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Rijo-Thomas/crypto-ccp-Allocate-TEE-ring-and-cmd-buffer-using-DMA-APIs/20221206-203201
        git checkout 5615778a8f19c66bbacb611c78175e3130aa22c0
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/crypto/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/crypto/ccp/tee-dev.c:117:20: warning: no previous prototype for 'tee_alloc_cmd_buffer' [-Wmissing-prototypes]
     117 | struct dma_buffer *tee_alloc_cmd_buffer(struct psp_tee_device *tee)
         |                    ^~~~~~~~~~~~~~~~~~~~


vim +/tee_alloc_cmd_buffer +117 drivers/crypto/ccp/tee-dev.c

   116	
 > 117	struct dma_buffer *tee_alloc_cmd_buffer(struct psp_tee_device *tee)
   118	{
   119		struct tee_init_ring_cmd *cmd;
   120		struct dma_buffer *cmd_buffer;
   121	
   122		cmd_buffer = psp_tee_alloc_dmabuf(sizeof(*cmd),
   123						  GFP_KERNEL | __GFP_ZERO);
   124		if (!cmd_buffer)
   125			return NULL;
   126	
   127		cmd = (struct tee_init_ring_cmd *)cmd_buffer->vaddr;
   128		cmd->hi_addr = upper_32_bits(tee->rb_mgr.ring_buf->paddr);
   129		cmd->low_addr = lower_32_bits(tee->rb_mgr.ring_buf->paddr);
   130		cmd->size = tee->rb_mgr.ring_buf->size;
   131	
   132		dev_dbg(tee->dev, "tee: ring address: high = 0x%x low = 0x%x size = %u\n",
   133			cmd->hi_addr, cmd->low_addr, cmd->size);
   134	
   135		return cmd_buffer;
   136	}
   137
  

On 12/6/22 06:30, Rijo Thomas wrote:
> For AMD Secure Processor (ASP) to map and access TEE ring buffer, the
> ring buffer address sent by host to ASP must be a real physical
> address and the pages must be physically contiguous.
> 
> In a virtualized environment though, when the driver is running in a
> guest VM, the pages allocated by __get_free_pages() may not be
> contiguous in the host (or machine) physical address space. Guests
> will see a guest (or pseudo) physical address and not the actual host
> (or machine) physical address. The TEE running on ASP cannot decipher
> pseudo physical addresses. It needs host or machine physical address.
> 
> To resolve this problem, use DMA APIs for allocating buffers that must
> be shared with TEE. This will ensure that the pages are contiguous in
> host (or machine) address space. If the DMA handle is an IOVA,
> translate it into a physical address before sending it to ASP.
> 
> This patch also exports two APIs (one for buffer allocation and
> another to free the buffer). This API can be used by AMD-TEE driver to
> share buffers with TEE.
> 
> Fixes: 33960acccfbd ("crypto: ccp - add TEE support for Raven Ridge")
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Rijo Thomas <Rijo-john.Thomas@amd.com>
> Co-developed-by: Jeshwanth <JESHWANTHKUMAR.NK@amd.com>
> Signed-off-by: Jeshwanth <JESHWANTHKUMAR.NK@amd.com>
> Reviewed-by: Devaraj Rangasamy <Devaraj.Rangasamy@amd.com>
> ---
>   drivers/crypto/ccp/psp-dev.c |   6 +-
>   drivers/crypto/ccp/tee-dev.c | 116 ++++++++++++++++++++++-------------
>   drivers/crypto/ccp/tee-dev.h |   9 +--
>   include/linux/psp-tee.h      |  47 ++++++++++++++
>   4 files changed, 127 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
> index c9c741ac8442..2b86158d7435 100644
> --- a/drivers/crypto/ccp/psp-dev.c
> +++ b/drivers/crypto/ccp/psp-dev.c
> @@ -161,13 +161,13 @@ int psp_dev_init(struct sp_device *sp)
>   		goto e_err;
>   	}
>   
> +	if (sp->set_psp_master_device)
> +		sp->set_psp_master_device(sp);
> +

This worries me a bit...  if psp_init() fails, it may still be referenced 
as the master device. What's the reason for moving it?

>   	ret = psp_init(psp);
>   	if (ret)
>   		goto e_irq;
>   
> -	if (sp->set_psp_master_device)
> -		sp->set_psp_master_device(sp);
> -
>   	/* Enable interrupt */
>   	iowrite32(-1, psp->io_regs + psp->vdata->inten_reg);
>   
> diff --git a/drivers/crypto/ccp/tee-dev.c b/drivers/crypto/ccp/tee-dev.c
> index 5c9d47f3be37..1631d9851e54 100644
> --- a/drivers/crypto/ccp/tee-dev.c
> +++ b/drivers/crypto/ccp/tee-dev.c
> @@ -12,8 +12,9 @@
>   #include <linux/mutex.h>
>   #include <linux/delay.h>
>   #include <linux/slab.h>
> +#include <linux/dma-direct.h>
> +#include <linux/iommu.h>
>   #include <linux/gfp.h>
> -#include <linux/psp-sev.h>
>   #include <linux/psp-tee.h>
>   
>   #include "psp-dev.h"
> @@ -21,25 +22,64 @@
>   
>   static bool psp_dead;
>   
> +struct dma_buffer *psp_tee_alloc_dmabuf(unsigned long size, gfp_t gfp)

It looks like both calls to this use the same gfp_t values, you can 
probably eliminate from the call and just specify them in here.

> +{
> +	struct psp_device *psp = psp_get_master_device();
> +	struct dma_buffer *dma_buf;
> +	struct iommu_domain *dom;
> +
> +	if (!psp || !size)
> +		return NULL;
> +
> +	dma_buf = kzalloc(sizeof(*dma_buf), GFP_KERNEL);
> +	if (!dma_buf)
> +		return NULL;
> +
> +	dma_buf->vaddr = dma_alloc_coherent(psp->dev, size, &dma_buf->dma, gfp);
> +	if (!dma_buf->vaddr || !dma_buf->dma) {

I don't think you can have one of these be NULL without both being NULL, 
but I guess it doesn't hurt to check.

> +		kfree(dma_buf);
> +		return NULL;
> +	}
> +
> +	dma_buf->size = size;
> + > +	dom = iommu_get_domain_for_dev(psp->dev);
> +	if (dom)

You need a nice comment above this all explaining that. I guess you're 
using the presence of a domain to determine whether you're running on 
bare-metal vs within a hypervisor? I'm not sure what will happen if the 
guest ever gets an emulated IOMMU...

> +		dma_buf->paddr = iommu_iova_to_phys(dom, dma_buf->dma);

If you're just looking to get the physical address, why not just to an 
__pa(dma_buf->vaddr)?

Also, paddr might not be the best name, since it isn't always a physical 
address, but I can't really think of something right now.

Thanks,
Tom

> +	else
> +		dma_buf->paddr = dma_buf->dma;
> +
> +	return dma_buf;
> +}
> +EXPORT_SYMBOL(psp_tee_alloc_dmabuf);
> +
> +void psp_tee_free_dmabuf(struct dma_buffer *dma_buf)
> +{
> +	struct psp_device *psp = psp_get_master_device();
> +
> +	if (!psp || !dma_buf)
> +		return;
> +
> +	dma_free_coherent(psp->dev, dma_buf->size,
> +			  dma_buf->vaddr, dma_buf->dma);
> +
> +	kfree(dma_buf);
> +}
> +EXPORT_SYMBOL(psp_tee_free_dmabuf);
> +
>   static int tee_alloc_ring(struct psp_tee_device *tee, int ring_size)
>   {
>   	struct ring_buf_manager *rb_mgr = &tee->rb_mgr;
> -	void *start_addr;
>   
>   	if (!ring_size)
>   		return -EINVAL;
>   
> -	/* We need actual physical address instead of DMA address, since
> -	 * Trusted OS running on AMD Secure Processor will map this region
> -	 */
> -	start_addr = (void *)__get_free_pages(GFP_KERNEL, get_order(ring_size));
> -	if (!start_addr)
> +	rb_mgr->ring_buf = psp_tee_alloc_dmabuf(ring_size,
> +						GFP_KERNEL | __GFP_ZERO);
> +	if (!rb_mgr->ring_buf) {
> +		dev_err(tee->dev, "ring allocation failed\n");
>   		return -ENOMEM;
> -
> -	memset(start_addr, 0x0, ring_size);
> -	rb_mgr->ring_start = start_addr;
> -	rb_mgr->ring_size = ring_size;
> -	rb_mgr->ring_pa = __psp_pa(start_addr);
> +	}
>   	mutex_init(&rb_mgr->mutex);
>   
>   	return 0;
> @@ -49,15 +89,8 @@ static void tee_free_ring(struct psp_tee_device *tee)
>   {
>   	struct ring_buf_manager *rb_mgr = &tee->rb_mgr;
>   
> -	if (!rb_mgr->ring_start)
> -		return;
> +	psp_tee_free_dmabuf(rb_mgr->ring_buf);
>   
> -	free_pages((unsigned long)rb_mgr->ring_start,
> -		   get_order(rb_mgr->ring_size));
> -
> -	rb_mgr->ring_start = NULL;
> -	rb_mgr->ring_size = 0;
> -	rb_mgr->ring_pa = 0;
>   	mutex_destroy(&rb_mgr->mutex);
>   }
>   
> @@ -81,35 +114,36 @@ static int tee_wait_cmd_poll(struct psp_tee_device *tee, unsigned int timeout,
>   	return -ETIMEDOUT;
>   }
>   
> -static
> -struct tee_init_ring_cmd *tee_alloc_cmd_buffer(struct psp_tee_device *tee)
> +struct dma_buffer *tee_alloc_cmd_buffer(struct psp_tee_device *tee)
>   {
>   	struct tee_init_ring_cmd *cmd;
> +	struct dma_buffer *cmd_buffer;
>   
> -	cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
> -	if (!cmd)
> +	cmd_buffer = psp_tee_alloc_dmabuf(sizeof(*cmd),
> +					  GFP_KERNEL | __GFP_ZERO);
> +	if (!cmd_buffer)
>   		return NULL;
>   
> -	cmd->hi_addr = upper_32_bits(tee->rb_mgr.ring_pa);
> -	cmd->low_addr = lower_32_bits(tee->rb_mgr.ring_pa);
> -	cmd->size = tee->rb_mgr.ring_size;
> +	cmd = (struct tee_init_ring_cmd *)cmd_buffer->vaddr;
> +	cmd->hi_addr = upper_32_bits(tee->rb_mgr.ring_buf->paddr);
> +	cmd->low_addr = lower_32_bits(tee->rb_mgr.ring_buf->paddr);
> +	cmd->size = tee->rb_mgr.ring_buf->size;
>   
>   	dev_dbg(tee->dev, "tee: ring address: high = 0x%x low = 0x%x size = %u\n",
>   		cmd->hi_addr, cmd->low_addr, cmd->size);
>   
> -	return cmd;
> +	return cmd_buffer;
>   }
>   
> -static inline void tee_free_cmd_buffer(struct tee_init_ring_cmd *cmd)
> +static inline void tee_free_cmd_buffer(struct dma_buffer *cmd_buffer)
>   {
> -	kfree(cmd);
> +	psp_tee_free_dmabuf(cmd_buffer);
>   }
>   
>   static int tee_init_ring(struct psp_tee_device *tee)
>   {
>   	int ring_size = MAX_RING_BUFFER_ENTRIES * sizeof(struct tee_ring_cmd);
> -	struct tee_init_ring_cmd *cmd;
> -	phys_addr_t cmd_buffer;
> +	struct dma_buffer *cmd_buffer;
>   	unsigned int reg;
>   	int ret;
>   
> @@ -123,21 +157,19 @@ static int tee_init_ring(struct psp_tee_device *tee)
>   
>   	tee->rb_mgr.wptr = 0;
>   
> -	cmd = tee_alloc_cmd_buffer(tee);
> -	if (!cmd) {
> +	cmd_buffer = tee_alloc_cmd_buffer(tee);
> +	if (!cmd_buffer) {
>   		tee_free_ring(tee);
>   		return -ENOMEM;
>   	}
>   
> -	cmd_buffer = __psp_pa((void *)cmd);
> -
>   	/* Send command buffer details to Trusted OS by writing to
>   	 * CPU-PSP message registers
>   	 */
>   
> -	iowrite32(lower_32_bits(cmd_buffer),
> +	iowrite32(lower_32_bits(cmd_buffer->paddr),
>   		  tee->io_regs + tee->vdata->cmdbuff_addr_lo_reg);
> -	iowrite32(upper_32_bits(cmd_buffer),
> +	iowrite32(upper_32_bits(cmd_buffer->paddr),
>   		  tee->io_regs + tee->vdata->cmdbuff_addr_hi_reg);
>   	iowrite32(TEE_RING_INIT_CMD,
>   		  tee->io_regs + tee->vdata->cmdresp_reg);
> @@ -157,7 +189,7 @@ static int tee_init_ring(struct psp_tee_device *tee)
>   	}
>   
>   free_buf:
> -	tee_free_cmd_buffer(cmd);
> +	tee_free_cmd_buffer(cmd_buffer);
>   
>   	return ret;
>   }
> @@ -167,7 +199,7 @@ static void tee_destroy_ring(struct psp_tee_device *tee)
>   	unsigned int reg;
>   	int ret;
>   
> -	if (!tee->rb_mgr.ring_start)
> +	if (!tee->rb_mgr.ring_buf->vaddr)
>   		return;
>   
>   	if (psp_dead)
> @@ -256,7 +288,7 @@ static int tee_submit_cmd(struct psp_tee_device *tee, enum tee_cmd_id cmd_id,
>   	do {
>   		/* Get pointer to ring buffer command entry */
>   		cmd = (struct tee_ring_cmd *)
> -			(tee->rb_mgr.ring_start + tee->rb_mgr.wptr);
> +			(tee->rb_mgr.ring_buf->vaddr + tee->rb_mgr.wptr);
>   
>   		rptr = ioread32(tee->io_regs + tee->vdata->ring_rptr_reg);
>   
> @@ -305,7 +337,7 @@ static int tee_submit_cmd(struct psp_tee_device *tee, enum tee_cmd_id cmd_id,
>   
>   	/* Update local copy of write pointer */
>   	tee->rb_mgr.wptr += sizeof(struct tee_ring_cmd);
> -	if (tee->rb_mgr.wptr >= tee->rb_mgr.ring_size)
> +	if (tee->rb_mgr.wptr >= tee->rb_mgr.ring_buf->size)
>   		tee->rb_mgr.wptr = 0;
>   
>   	/* Trigger interrupt to Trusted OS */
> diff --git a/drivers/crypto/ccp/tee-dev.h b/drivers/crypto/ccp/tee-dev.h
> index 49d26158b71e..9238487ee8bf 100644
> --- a/drivers/crypto/ccp/tee-dev.h
> +++ b/drivers/crypto/ccp/tee-dev.h
> @@ -16,6 +16,7 @@
>   
>   #include <linux/device.h>
>   #include <linux/mutex.h>
> +#include <linux/psp-tee.h>
>   
>   #define TEE_DEFAULT_TIMEOUT		10
>   #define MAX_BUFFER_SIZE			988
> @@ -48,17 +49,13 @@ struct tee_init_ring_cmd {
>   
>   /**
>    * struct ring_buf_manager - Helper structure to manage ring buffer.
> - * @ring_start:  starting address of ring buffer
> - * @ring_size:   size of ring buffer in bytes
> - * @ring_pa:     physical address of ring buffer
>    * @wptr:        index to the last written entry in ring buffer
> + * @ring_buf:    ring buffer allocated using DMA api
>    */
>   struct ring_buf_manager {
>   	struct mutex mutex;	/* synchronizes access to ring buffer */
> -	void *ring_start;
> -	u32 ring_size;
> -	phys_addr_t ring_pa;
>   	u32 wptr;
> +	struct dma_buffer *ring_buf;
>   };
>   
>   struct psp_tee_device {
> diff --git a/include/linux/psp-tee.h b/include/linux/psp-tee.h
> index cb0c95d6d76b..c0fa922f24d4 100644
> --- a/include/linux/psp-tee.h
> +++ b/include/linux/psp-tee.h
> @@ -13,6 +13,7 @@
>   
>   #include <linux/types.h>
>   #include <linux/errno.h>
> +#include <linux/dma-mapping.h>
>   
>   /* This file defines the Trusted Execution Environment (TEE) interface commands
>    * and the API exported by AMD Secure Processor driver to communicate with
> @@ -40,6 +41,20 @@ enum tee_cmd_id {
>   	TEE_CMD_ID_UNMAP_SHARED_MEM,
>   };
>   
> +/**
> + * struct dma_buffer - Structure for a DMA buffer.
> + * @dma:    DMA buffer address
> + * @paddr:  Physical address of DMA buffer
> + * @vaddr:  CPU virtual address of DMA buffer
> + * @size:   Size of DMA buffer in bytes
> + */
> +struct dma_buffer {
> +	dma_addr_t dma;
> +	phys_addr_t paddr;
> +	void *vaddr;
> +	unsigned long size;
> +};
> +
>   #ifdef CONFIG_CRYPTO_DEV_SP_PSP
>   /**
>    * psp_tee_process_cmd() - Process command in Trusted Execution Environment
> @@ -75,6 +90,28 @@ int psp_tee_process_cmd(enum tee_cmd_id cmd_id, void *buf, size_t len,
>    */
>   int psp_check_tee_status(void);
>   
> +/**
> + * psp_tee_alloc_dmabuf() - Allocates memory of requested size and flags using
> + * dma_alloc_coherent() API.
> + *
> + * This function can be used to allocate a shared memory region between the
> + * host and PSP TEE.
> + *
> + * Returns:
> + * non-NULL   a valid pointer to struct dma_buffer
> + * NULL       on failure
> + */
> +struct dma_buffer *psp_tee_alloc_dmabuf(unsigned long size, gfp_t gfp);
> +
> +/**
> + * psp_tee_free_dmabuf() - Deallocates memory using dma_free_coherent() API.
> + *
> + * This function can be used to release shared memory region between host
> + * and PSP TEE.
> + *
> + */
> +void psp_tee_free_dmabuf(struct dma_buffer *dma_buffer);
> +
>   #else /* !CONFIG_CRYPTO_DEV_SP_PSP */
>   
>   static inline int psp_tee_process_cmd(enum tee_cmd_id cmd_id, void *buf,
> @@ -87,5 +124,15 @@ static inline int psp_check_tee_status(void)
>   {
>   	return -ENODEV;
>   }
> +
> +static inline
> +struct dma_buffer *psp_tee_alloc_dmabuf(unsigned long size, gfp_t gfp)
> +{
> +	return NULL;
> +}
> +
> +static inline void psp_tee_free_dmabuf(struct dma_buffer *dma_buffer)
> +{
> +}
>   #endif /* CONFIG_CRYPTO_DEV_SP_PSP */
>   #endif /* __PSP_TEE_H_ */
  

On 12/10/2022 2:31 AM, Tom Lendacky wrote:
> On 12/6/22 06:30, Rijo Thomas wrote:
>> For AMD Secure Processor (ASP) to map and access TEE ring buffer, the
>> ring buffer address sent by host to ASP must be a real physical
>> address and the pages must be physically contiguous.
>>
>> In a virtualized environment though, when the driver is running in a
>> guest VM, the pages allocated by __get_free_pages() may not be
>> contiguous in the host (or machine) physical address space. Guests
>> will see a guest (or pseudo) physical address and not the actual host
>> (or machine) physical address. The TEE running on ASP cannot decipher
>> pseudo physical addresses. It needs host or machine physical address.
>>
>> To resolve this problem, use DMA APIs for allocating buffers that must
>> be shared with TEE. This will ensure that the pages are contiguous in
>> host (or machine) address space. If the DMA handle is an IOVA,
>> translate it into a physical address before sending it to ASP.
>>
>> This patch also exports two APIs (one for buffer allocation and
>> another to free the buffer). This API can be used by AMD-TEE driver to
>> share buffers with TEE.
>>
>> Fixes: 33960acccfbd ("crypto: ccp - add TEE support for Raven Ridge")
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Rijo Thomas <Rijo-john.Thomas@amd.com>
>> Co-developed-by: Jeshwanth <JESHWANTHKUMAR.NK@amd.com>
>> Signed-off-by: Jeshwanth <JESHWANTHKUMAR.NK@amd.com>
>> Reviewed-by: Devaraj Rangasamy <Devaraj.Rangasamy@amd.com>
>> ---
>>   drivers/crypto/ccp/psp-dev.c |   6 +-
>>   drivers/crypto/ccp/tee-dev.c | 116 ++++++++++++++++++++++-------------
>>   drivers/crypto/ccp/tee-dev.h |   9 +--
>>   include/linux/psp-tee.h      |  47 ++++++++++++++
>>   4 files changed, 127 insertions(+), 51 deletions(-)
>>
>> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
>> index c9c741ac8442..2b86158d7435 100644
>> --- a/drivers/crypto/ccp/psp-dev.c
>> +++ b/drivers/crypto/ccp/psp-dev.c
>> @@ -161,13 +161,13 @@ int psp_dev_init(struct sp_device *sp)
>>           goto e_err;
>>       }
>>   +    if (sp->set_psp_master_device)
>> +        sp->set_psp_master_device(sp);
>> +
> 
> This worries me a bit...  if psp_init() fails, it may still be referenced as the master device. What's the reason for moving it?

Hmm. Okay, I see your point.

In psp_tee_alloc_dmabuf(), we call psp_get_master_device(). Without above change, psp_get_master_device() returns NULL.

I think in psp_dev_init(), we can add below error handling:

ret = psp_init(psp);
        if (ret)
                goto e_init;
     ...

e_init:
    if (sp->clear_psp_master_device)
        sp->clear_psp_master_device(sp);

Will this help address your concern?

> 
>>       ret = psp_init(psp);
>>       if (ret)
>>           goto e_irq;
>>   -    if (sp->set_psp_master_device)
>> -        sp->set_psp_master_device(sp);
>> -
>>       /* Enable interrupt */
>>       iowrite32(-1, psp->io_regs + psp->vdata->inten_reg);
>>   diff --git a/drivers/crypto/ccp/tee-dev.c b/drivers/crypto/ccp/tee-dev.c
>> index 5c9d47f3be37..1631d9851e54 100644
>> --- a/drivers/crypto/ccp/tee-dev.c
>> +++ b/drivers/crypto/ccp/tee-dev.c
>> @@ -12,8 +12,9 @@
>>   #include <linux/mutex.h>
>>   #include <linux/delay.h>
>>   #include <linux/slab.h>
>> +#include <linux/dma-direct.h>
>> +#include <linux/iommu.h>
>>   #include <linux/gfp.h>
>> -#include <linux/psp-sev.h>
>>   #include <linux/psp-tee.h>
>>     #include "psp-dev.h"
>> @@ -21,25 +22,64 @@
>>     static bool psp_dead;
>>   +struct dma_buffer *psp_tee_alloc_dmabuf(unsigned long size, gfp_t gfp)
> 
> It looks like both calls to this use the same gfp_t values, you can probably eliminate from the call and just specify them in here.
> 

Okay, I will remove gfp_t flag from the function argument.

>> +{
>> +    struct psp_device *psp = psp_get_master_device();
>> +    struct dma_buffer *dma_buf;
>> +    struct iommu_domain *dom;
>> +
>> +    if (!psp || !size)
>> +        return NULL;
>> +
>> +    dma_buf = kzalloc(sizeof(*dma_buf), GFP_KERNEL);
>> +    if (!dma_buf)
>> +        return NULL;
>> +
>> +    dma_buf->vaddr = dma_alloc_coherent(psp->dev, size, &dma_buf->dma, gfp);
>> +    if (!dma_buf->vaddr || !dma_buf->dma) {
> 
> I don't think you can have one of these be NULL without both being NULL, but I guess it doesn't hurt to check.
> 

Okay, we will keep both checks for now.

>> +        kfree(dma_buf);
>> +        return NULL;
>> +    }
>> +
>> +    dma_buf->size = size;
>> + > +    dom = iommu_get_domain_for_dev(psp->dev);
>> +    if (dom)
> 
> You need a nice comment above this all explaining that. I guess you're using the presence of a domain to determine whether you're running on bare-metal vs within a hypervisor? I'm not sure what will happen if the guest ever gets an emulated IOMMU...

Sure we will add a comment.

We are not trying to determive bare-metal vs hypervisor here, but determine whether DMA handle returned by dma_alloc_coherent() is an IOVA or not.
If the address is an IOVA, we convert IOVA to physical address using iommu_iova_to_phys(). This was our intention.

> 
>> +        dma_buf->paddr = iommu_iova_to_phys(dom, dma_buf->dma);
> 
> If you're just looking to get the physical address, why not just to an __pa(dma_buf->vaddr)?
> 
> Also, paddr might not be the best name, since it isn't always a physical address, but I can't really think of something right now.
> 

We can use __pa(dma_buf->vaddr) only on bare-metal. In hypervisor, __pa(dma_buf->vaddr) gives pseudo-physical address; pseudo-physical address cannot be understood by ASP.
ASP needs real physical address (aka machine address). Please see commit message.

We chose the name paddr, since it's a (real) physical address that we want to send across to ASP. I am not sure, why you say - it isn't always a physical address.

Thanks,
Rijo

> Thanks,
> Tom
> 
>> +    else
>> +        dma_buf->paddr = dma_buf->dma;
>> +
>> +    return dma_buf;
>> +}
>> +EXPORT_SYMBOL(psp_tee_alloc_dmabuf);
>> +
>> +void psp_tee_free_dmabuf(struct dma_buffer *dma_buf)
>> +{
>> +    struct psp_device *psp = psp_get_master_device();
>> +
>> +    if (!psp || !dma_buf)
>> +        return;
>> +
>> +    dma_free_coherent(psp->dev, dma_buf->size,
>> +              dma_buf->vaddr, dma_buf->dma);
>> +
>> +    kfree(dma_buf);
>> +}
>> +EXPORT_SYMBOL(psp_tee_free_dmabuf);
>> +
>>   static int tee_alloc_ring(struct psp_tee_device *tee, int ring_size)
>>   {
>>       struct ring_buf_manager *rb_mgr = &tee->rb_mgr;
>> -    void *start_addr;
>>         if (!ring_size)
>>           return -EINVAL;
>>   -    /* We need actual physical address instead of DMA address, since
>> -     * Trusted OS running on AMD Secure Processor will map this region
>> -     */
>> -    start_addr = (void *)__get_free_pages(GFP_KERNEL, get_order(ring_size));
>> -    if (!start_addr)
>> +    rb_mgr->ring_buf = psp_tee_alloc_dmabuf(ring_size,
>> +                        GFP_KERNEL | __GFP_ZERO);
>> +    if (!rb_mgr->ring_buf) {
>> +        dev_err(tee->dev, "ring allocation failed\n");
>>           return -ENOMEM;
>> -
>> -    memset(start_addr, 0x0, ring_size);
>> -    rb_mgr->ring_start = start_addr;
>> -    rb_mgr->ring_size = ring_size;
>> -    rb_mgr->ring_pa = __psp_pa(start_addr);
>> +    }
>>       mutex_init(&rb_mgr->mutex);
>>         return 0;
>> @@ -49,15 +89,8 @@ static void tee_free_ring(struct psp_tee_device *tee)
>>   {
>>       struct ring_buf_manager *rb_mgr = &tee->rb_mgr;
>>   -    if (!rb_mgr->ring_start)
>> -        return;
>> +    psp_tee_free_dmabuf(rb_mgr->ring_buf);
>>   -    free_pages((unsigned long)rb_mgr->ring_start,
>> -           get_order(rb_mgr->ring_size));
>> -
>> -    rb_mgr->ring_start = NULL;
>> -    rb_mgr->ring_size = 0;
>> -    rb_mgr->ring_pa = 0;
>>       mutex_destroy(&rb_mgr->mutex);
>>   }
>>   @@ -81,35 +114,36 @@ static int tee_wait_cmd_poll(struct psp_tee_device *tee, unsigned int timeout,
>>       return -ETIMEDOUT;
>>   }
>>   -static
>> -struct tee_init_ring_cmd *tee_alloc_cmd_buffer(struct psp_tee_device *tee)
>> +struct dma_buffer *tee_alloc_cmd_buffer(struct psp_tee_device *tee)
>>   {
>>       struct tee_init_ring_cmd *cmd;
>> +    struct dma_buffer *cmd_buffer;
>>   -    cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
>> -    if (!cmd)
>> +    cmd_buffer = psp_tee_alloc_dmabuf(sizeof(*cmd),
>> +                      GFP_KERNEL | __GFP_ZERO);
>> +    if (!cmd_buffer)
>>           return NULL;
>>   -    cmd->hi_addr = upper_32_bits(tee->rb_mgr.ring_pa);
>> -    cmd->low_addr = lower_32_bits(tee->rb_mgr.ring_pa);
>> -    cmd->size = tee->rb_mgr.ring_size;
>> +    cmd = (struct tee_init_ring_cmd *)cmd_buffer->vaddr;
>> +    cmd->hi_addr = upper_32_bits(tee->rb_mgr.ring_buf->paddr);
>> +    cmd->low_addr = lower_32_bits(tee->rb_mgr.ring_buf->paddr);
>> +    cmd->size = tee->rb_mgr.ring_buf->size;
>>         dev_dbg(tee->dev, "tee: ring address: high = 0x%x low = 0x%x size = %u\n",
>>           cmd->hi_addr, cmd->low_addr, cmd->size);
>>   -    return cmd;
>> +    return cmd_buffer;
>>   }
>>   -static inline void tee_free_cmd_buffer(struct tee_init_ring_cmd *cmd)
>> +static inline void tee_free_cmd_buffer(struct dma_buffer *cmd_buffer)
>>   {
>> -    kfree(cmd);
>> +    psp_tee_free_dmabuf(cmd_buffer);
>>   }
>>     static int tee_init_ring(struct psp_tee_device *tee)
>>   {
>>       int ring_size = MAX_RING_BUFFER_ENTRIES * sizeof(struct tee_ring_cmd);
>> -    struct tee_init_ring_cmd *cmd;
>> -    phys_addr_t cmd_buffer;
>> +    struct dma_buffer *cmd_buffer;
>>       unsigned int reg;
>>       int ret;
>>   @@ -123,21 +157,19 @@ static int tee_init_ring(struct psp_tee_device *tee)
>>         tee->rb_mgr.wptr = 0;
>>   -    cmd = tee_alloc_cmd_buffer(tee);
>> -    if (!cmd) {
>> +    cmd_buffer = tee_alloc_cmd_buffer(tee);
>> +    if (!cmd_buffer) {
>>           tee_free_ring(tee);
>>           return -ENOMEM;
>>       }
>>   -    cmd_buffer = __psp_pa((void *)cmd);
>> -
>>       /* Send command buffer details to Trusted OS by writing to
>>        * CPU-PSP message registers
>>        */
>>   -    iowrite32(lower_32_bits(cmd_buffer),
>> +    iowrite32(lower_32_bits(cmd_buffer->paddr),
>>             tee->io_regs + tee->vdata->cmdbuff_addr_lo_reg);
>> -    iowrite32(upper_32_bits(cmd_buffer),
>> +    iowrite32(upper_32_bits(cmd_buffer->paddr),
>>             tee->io_regs + tee->vdata->cmdbuff_addr_hi_reg);
>>       iowrite32(TEE_RING_INIT_CMD,
>>             tee->io_regs + tee->vdata->cmdresp_reg);
>> @@ -157,7 +189,7 @@ static int tee_init_ring(struct psp_tee_device *tee)
>>       }
>>     free_buf:
>> -    tee_free_cmd_buffer(cmd);
>> +    tee_free_cmd_buffer(cmd_buffer);
>>         return ret;
>>   }
>> @@ -167,7 +199,7 @@ static void tee_destroy_ring(struct psp_tee_device *tee)
>>       unsigned int reg;
>>       int ret;
>>   -    if (!tee->rb_mgr.ring_start)
>> +    if (!tee->rb_mgr.ring_buf->vaddr)
>>           return;
>>         if (psp_dead)
>> @@ -256,7 +288,7 @@ static int tee_submit_cmd(struct psp_tee_device *tee, enum tee_cmd_id cmd_id,
>>       do {
>>           /* Get pointer to ring buffer command entry */
>>           cmd = (struct tee_ring_cmd *)
>> -            (tee->rb_mgr.ring_start + tee->rb_mgr.wptr);
>> +            (tee->rb_mgr.ring_buf->vaddr + tee->rb_mgr.wptr);
>>             rptr = ioread32(tee->io_regs + tee->vdata->ring_rptr_reg);
>>   @@ -305,7 +337,7 @@ static int tee_submit_cmd(struct psp_tee_device *tee, enum tee_cmd_id cmd_id,
>>         /* Update local copy of write pointer */
>>       tee->rb_mgr.wptr += sizeof(struct tee_ring_cmd);
>> -    if (tee->rb_mgr.wptr >= tee->rb_mgr.ring_size)
>> +    if (tee->rb_mgr.wptr >= tee->rb_mgr.ring_buf->size)
>>           tee->rb_mgr.wptr = 0;
>>         /* Trigger interrupt to Trusted OS */
>> diff --git a/drivers/crypto/ccp/tee-dev.h b/drivers/crypto/ccp/tee-dev.h
>> index 49d26158b71e..9238487ee8bf 100644
>> --- a/drivers/crypto/ccp/tee-dev.h
>> +++ b/drivers/crypto/ccp/tee-dev.h
>> @@ -16,6 +16,7 @@
>>     #include <linux/device.h>
>>   #include <linux/mutex.h>
>> +#include <linux/psp-tee.h>
>>     #define TEE_DEFAULT_TIMEOUT        10
>>   #define MAX_BUFFER_SIZE            988
>> @@ -48,17 +49,13 @@ struct tee_init_ring_cmd {
>>     /**
>>    * struct ring_buf_manager - Helper structure to manage ring buffer.
>> - * @ring_start:  starting address of ring buffer
>> - * @ring_size:   size of ring buffer in bytes
>> - * @ring_pa:     physical address of ring buffer
>>    * @wptr:        index to the last written entry in ring buffer
>> + * @ring_buf:    ring buffer allocated using DMA api
>>    */
>>   struct ring_buf_manager {
>>       struct mutex mutex;    /* synchronizes access to ring buffer */
>> -    void *ring_start;
>> -    u32 ring_size;
>> -    phys_addr_t ring_pa;
>>       u32 wptr;
>> +    struct dma_buffer *ring_buf;
>>   };
>>     struct psp_tee_device {
>> diff --git a/include/linux/psp-tee.h b/include/linux/psp-tee.h
>> index cb0c95d6d76b..c0fa922f24d4 100644
>> --- a/include/linux/psp-tee.h
>> +++ b/include/linux/psp-tee.h
>> @@ -13,6 +13,7 @@
>>     #include <linux/types.h>
>>   #include <linux/errno.h>
>> +#include <linux/dma-mapping.h>
>>     /* This file defines the Trusted Execution Environment (TEE) interface commands
>>    * and the API exported by AMD Secure Processor driver to communicate with
>> @@ -40,6 +41,20 @@ enum tee_cmd_id {
>>       TEE_CMD_ID_UNMAP_SHARED_MEM,
>>   };
>>   +/**
>> + * struct dma_buffer - Structure for a DMA buffer.
>> + * @dma:    DMA buffer address
>> + * @paddr:  Physical address of DMA buffer
>> + * @vaddr:  CPU virtual address of DMA buffer
>> + * @size:   Size of DMA buffer in bytes
>> + */
>> +struct dma_buffer {
>> +    dma_addr_t dma;
>> +    phys_addr_t paddr;
>> +    void *vaddr;
>> +    unsigned long size;
>> +};
>> +
>>   #ifdef CONFIG_CRYPTO_DEV_SP_PSP
>>   /**
>>    * psp_tee_process_cmd() - Process command in Trusted Execution Environment
>> @@ -75,6 +90,28 @@ int psp_tee_process_cmd(enum tee_cmd_id cmd_id, void *buf, size_t len,
>>    */
>>   int psp_check_tee_status(void);
>>   +/**
>> + * psp_tee_alloc_dmabuf() - Allocates memory of requested size and flags using
>> + * dma_alloc_coherent() API.
>> + *
>> + * This function can be used to allocate a shared memory region between the
>> + * host and PSP TEE.
>> + *
>> + * Returns:
>> + * non-NULL   a valid pointer to struct dma_buffer
>> + * NULL       on failure
>> + */
>> +struct dma_buffer *psp_tee_alloc_dmabuf(unsigned long size, gfp_t gfp);
>> +
>> +/**
>> + * psp_tee_free_dmabuf() - Deallocates memory using dma_free_coherent() API.
>> + *
>> + * This function can be used to release shared memory region between host
>> + * and PSP TEE.
>> + *
>> + */
>> +void psp_tee_free_dmabuf(struct dma_buffer *dma_buffer);
>> +
>>   #else /* !CONFIG_CRYPTO_DEV_SP_PSP */
>>     static inline int psp_tee_process_cmd(enum tee_cmd_id cmd_id, void *buf,
>> @@ -87,5 +124,15 @@ static inline int psp_check_tee_status(void)
>>   {
>>       return -ENODEV;
>>   }
>> +
>> +static inline
>> +struct dma_buffer *psp_tee_alloc_dmabuf(unsigned long size, gfp_t gfp)
>> +{
>> +    return NULL;
>> +}
>> +
>> +static inline void psp_tee_free_dmabuf(struct dma_buffer *dma_buffer)
>> +{
>> +}
>>   #endif /* CONFIG_CRYPTO_DEV_SP_PSP */
>>   #endif /* __PSP_TEE_H_ */
  

On 12/12/22 05:21, Rijo Thomas wrote:
> On 12/10/2022 2:31 AM, Tom Lendacky wrote:
>> On 12/6/22 06:30, Rijo Thomas wrote:
>>> For AMD Secure Processor (ASP) to map and access TEE ring buffer, the
>>> ring buffer address sent by host to ASP must be a real physical
>>> address and the pages must be physically contiguous.
>>>
>>> In a virtualized environment though, when the driver is running in a
>>> guest VM, the pages allocated by __get_free_pages() may not be
>>> contiguous in the host (or machine) physical address space. Guests
>>> will see a guest (or pseudo) physical address and not the actual host
>>> (or machine) physical address. The TEE running on ASP cannot decipher
>>> pseudo physical addresses. It needs host or machine physical address.
>>>
>>> To resolve this problem, use DMA APIs for allocating buffers that must
>>> be shared with TEE. This will ensure that the pages are contiguous in
>>> host (or machine) address space. If the DMA handle is an IOVA,
>>> translate it into a physical address before sending it to ASP.
>>>
>>> This patch also exports two APIs (one for buffer allocation and
>>> another to free the buffer). This API can be used by AMD-TEE driver to
>>> share buffers with TEE.
>>>
>>> Fixes: 33960acccfbd ("crypto: ccp - add TEE support for Raven Ridge")
>>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Rijo Thomas <Rijo-john.Thomas@amd.com>
>>> Co-developed-by: Jeshwanth <JESHWANTHKUMAR.NK@amd.com>
>>> Signed-off-by: Jeshwanth <JESHWANTHKUMAR.NK@amd.com>
>>> Reviewed-by: Devaraj Rangasamy <Devaraj.Rangasamy@amd.com>
>>> ---
>>>    drivers/crypto/ccp/psp-dev.c |   6 +-
>>>    drivers/crypto/ccp/tee-dev.c | 116 ++++++++++++++++++++++-------------
>>>    drivers/crypto/ccp/tee-dev.h |   9 +--
>>>    include/linux/psp-tee.h      |  47 ++++++++++++++
>>>    4 files changed, 127 insertions(+), 51 deletions(-)
>>>
>>> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
>>> index c9c741ac8442..2b86158d7435 100644
>>> --- a/drivers/crypto/ccp/psp-dev.c
>>> +++ b/drivers/crypto/ccp/psp-dev.c
>>> @@ -161,13 +161,13 @@ int psp_dev_init(struct sp_device *sp)
>>>            goto e_err;
>>>        }
>>>    +    if (sp->set_psp_master_device)
>>> +        sp->set_psp_master_device(sp);
>>> +
>>
>> This worries me a bit...  if psp_init() fails, it may still be referenced as the master device. What's the reason for moving it?
> 
> Hmm. Okay, I see your point.
> 
> In psp_tee_alloc_dmabuf(), we call psp_get_master_device(). Without above change, psp_get_master_device() returns NULL.
> 
> I think in psp_dev_init(), we can add below error handling:
> 
> ret = psp_init(psp);
>          if (ret)
>                  goto e_init;
>       ...
> 
> e_init:
>      if (sp->clear_psp_master_device)
>          sp->clear_psp_master_device(sp);
> 
> Will this help address your concern?

Yes, that works.

> 
>>
>>>        ret = psp_init(psp);
>>>        if (ret)
>>>            goto e_irq;
>>>    -    if (sp->set_psp_master_device)
>>> -        sp->set_psp_master_device(sp);
>>> -
>>>        /* Enable interrupt */
>>>        iowrite32(-1, psp->io_regs + psp->vdata->inten_reg);
>>>    diff --git a/drivers/crypto/ccp/tee-dev.c b/drivers/crypto/ccp/tee-dev.c
>>> index 5c9d47f3be37..1631d9851e54 100644
>>> --- a/drivers/crypto/ccp/tee-dev.c
>>> +++ b/drivers/crypto/ccp/tee-dev.c
>>> @@ -12,8 +12,9 @@
>>>    #include <linux/mutex.h>
>>>    #include <linux/delay.h>
>>>    #include <linux/slab.h>
>>> +#include <linux/dma-direct.h>
>>> +#include <linux/iommu.h>
>>>    #include <linux/gfp.h>
>>> -#include <linux/psp-sev.h>
>>>    #include <linux/psp-tee.h>
>>>      #include "psp-dev.h"
>>> @@ -21,25 +22,64 @@
>>>      static bool psp_dead;
>>>    +struct dma_buffer *psp_tee_alloc_dmabuf(unsigned long size, gfp_t gfp)
>>
>> It looks like both calls to this use the same gfp_t values, you can probably eliminate from the call and just specify them in here.
>>
> 
> Okay, I will remove gfp_t flag from the function argument.
> 
>>> +{
>>> +    struct psp_device *psp = psp_get_master_device();
>>> +    struct dma_buffer *dma_buf;
>>> +    struct iommu_domain *dom;
>>> +
>>> +    if (!psp || !size)
>>> +        return NULL;
>>> +
>>> +    dma_buf = kzalloc(sizeof(*dma_buf), GFP_KERNEL);
>>> +    if (!dma_buf)
>>> +        return NULL;
>>> +
>>> +    dma_buf->vaddr = dma_alloc_coherent(psp->dev, size, &dma_buf->dma, gfp);
>>> +    if (!dma_buf->vaddr || !dma_buf->dma) {
>>
>> I don't think you can have one of these be NULL without both being NULL, but I guess it doesn't hurt to check.
>>
> 
> Okay, we will keep both checks for now.
> 
>>> +        kfree(dma_buf);
>>> +        return NULL;
>>> +    }
>>> +
>>> +    dma_buf->size = size;
>>> + > +    dom = iommu_get_domain_for_dev(psp->dev);
>>> +    if (dom)
>>
>> You need a nice comment above this all explaining that. I guess you're using the presence of a domain to determine whether you're running on bare-metal vs within a hypervisor? I'm not sure what will happen if the guest ever gets an emulated IOMMU...
> 
> Sure we will add a comment.
> 
> We are not trying to determive bare-metal vs hypervisor here, but determine whether DMA handle returned by dma_alloc_coherent() is an IOVA or not.
> If the address is an IOVA, we convert IOVA to physical address using iommu_iova_to_phys(). This was our intention.

Ok, but if a VM gets an emulated IOMMU and ends up with an IOVA, that IOVA 
points to a GPA - and if the size is over a page, then you aren't 
guaranteed to have contiguous physical memory.

Probably not a concern at the moment, but not sure about what happens in 
the future.

> 
>>
>>> +        dma_buf->paddr = iommu_iova_to_phys(dom, dma_buf->dma);
>>
>> If you're just looking to get the physical address, why not just to an __pa(dma_buf->vaddr)?
>>
>> Also, paddr might not be the best name, since it isn't always a physical address, but I can't really think of something right now.
>>
> 
> We can use __pa(dma_buf->vaddr) only on bare-metal. In hypervisor, __pa(dma_buf->vaddr) gives pseudo-physical address; pseudo-physical address cannot be understood by ASP.
> ASP needs real physical address (aka machine address). Please see commit message.
> 
> We chose the name paddr, since it's a (real) physical address that we want to send across to ASP. I am not sure, why you say - it isn't always a physical address.

Then a nice comment above this explaining all that and the fact that when 
there is no IOMMU the DMA address is actually the physical address, would 
be appropriate. A number of years from now, this will raise questions 
without thorough documentation.

Thanks,
Tom

> 
> Thanks,
> Rijo
> 
>> Thanks,
>> Tom
>>
>>> +    else
>>> +        dma_buf->paddr = dma_buf->dma;
>>> +
>>> +    return dma_buf;
>>> +}
>>> +EXPORT_SYMBOL(psp_tee_alloc_dmabuf);
>>> +
>>> +void psp_tee_free_dmabuf(struct dma_buffer *dma_buf)
>>> +{
>>> +    struct psp_device *psp = psp_get_master_device();
>>> +
>>> +    if (!psp || !dma_buf)
>>> +        return;
>>> +
>>> +    dma_free_coherent(psp->dev, dma_buf->size,
>>> +              dma_buf->vaddr, dma_buf->dma);
>>> +
>>> +    kfree(dma_buf);
>>> +}
>>> +EXPORT_SYMBOL(psp_tee_free_dmabuf);
>>> +
>>>    static int tee_alloc_ring(struct psp_tee_device *tee, int ring_size)
>>>    {
>>>        struct ring_buf_manager *rb_mgr = &tee->rb_mgr;
>>> -    void *start_addr;
>>>          if (!ring_size)
>>>            return -EINVAL;
>>>    -    /* We need actual physical address instead of DMA address, since
>>> -     * Trusted OS running on AMD Secure Processor will map this region
>>> -     */
>>> -    start_addr = (void *)__get_free_pages(GFP_KERNEL, get_order(ring_size));
>>> -    if (!start_addr)
>>> +    rb_mgr->ring_buf = psp_tee_alloc_dmabuf(ring_size,
>>> +                        GFP_KERNEL | __GFP_ZERO);
>>> +    if (!rb_mgr->ring_buf) {
>>> +        dev_err(tee->dev, "ring allocation failed\n");
>>>            return -ENOMEM;
>>> -
>>> -    memset(start_addr, 0x0, ring_size);
>>> -    rb_mgr->ring_start = start_addr;
>>> -    rb_mgr->ring_size = ring_size;
>>> -    rb_mgr->ring_pa = __psp_pa(start_addr);
>>> +    }
>>>        mutex_init(&rb_mgr->mutex);
>>>          return 0;
>>> @@ -49,15 +89,8 @@ static void tee_free_ring(struct psp_tee_device *tee)
>>>    {
>>>        struct ring_buf_manager *rb_mgr = &tee->rb_mgr;
>>>    -    if (!rb_mgr->ring_start)
>>> -        return;
>>> +    psp_tee_free_dmabuf(rb_mgr->ring_buf);
>>>    -    free_pages((unsigned long)rb_mgr->ring_start,
>>> -           get_order(rb_mgr->ring_size));
>>> -
>>> -    rb_mgr->ring_start = NULL;
>>> -    rb_mgr->ring_size = 0;
>>> -    rb_mgr->ring_pa = 0;
>>>        mutex_destroy(&rb_mgr->mutex);
>>>    }
>>>    @@ -81,35 +114,36 @@ static int tee_wait_cmd_poll(struct psp_tee_device *tee, unsigned int timeout,
>>>        return -ETIMEDOUT;
>>>    }
>>>    -static
>>> -struct tee_init_ring_cmd *tee_alloc_cmd_buffer(struct psp_tee_device *tee)
>>> +struct dma_buffer *tee_alloc_cmd_buffer(struct psp_tee_device *tee)
>>>    {
>>>        struct tee_init_ring_cmd *cmd;
>>> +    struct dma_buffer *cmd_buffer;
>>>    -    cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
>>> -    if (!cmd)
>>> +    cmd_buffer = psp_tee_alloc_dmabuf(sizeof(*cmd),
>>> +                      GFP_KERNEL | __GFP_ZERO);
>>> +    if (!cmd_buffer)
>>>            return NULL;
>>>    -    cmd->hi_addr = upper_32_bits(tee->rb_mgr.ring_pa);
>>> -    cmd->low_addr = lower_32_bits(tee->rb_mgr.ring_pa);
>>> -    cmd->size = tee->rb_mgr.ring_size;
>>> +    cmd = (struct tee_init_ring_cmd *)cmd_buffer->vaddr;
>>> +    cmd->hi_addr = upper_32_bits(tee->rb_mgr.ring_buf->paddr);
>>> +    cmd->low_addr = lower_32_bits(tee->rb_mgr.ring_buf->paddr);
>>> +    cmd->size = tee->rb_mgr.ring_buf->size;
>>>          dev_dbg(tee->dev, "tee: ring address: high = 0x%x low = 0x%x size = %u\n",
>>>            cmd->hi_addr, cmd->low_addr, cmd->size);
>>>    -    return cmd;
>>> +    return cmd_buffer;
>>>    }
>>>    -static inline void tee_free_cmd_buffer(struct tee_init_ring_cmd *cmd)
>>> +static inline void tee_free_cmd_buffer(struct dma_buffer *cmd_buffer)
>>>    {
>>> -    kfree(cmd);
>>> +    psp_tee_free_dmabuf(cmd_buffer);
>>>    }
>>>      static int tee_init_ring(struct psp_tee_device *tee)
>>>    {
>>>        int ring_size = MAX_RING_BUFFER_ENTRIES * sizeof(struct tee_ring_cmd);
>>> -    struct tee_init_ring_cmd *cmd;
>>> -    phys_addr_t cmd_buffer;
>>> +    struct dma_buffer *cmd_buffer;
>>>        unsigned int reg;
>>>        int ret;
>>>    @@ -123,21 +157,19 @@ static int tee_init_ring(struct psp_tee_device *tee)
>>>          tee->rb_mgr.wptr = 0;
>>>    -    cmd = tee_alloc_cmd_buffer(tee);
>>> -    if (!cmd) {
>>> +    cmd_buffer = tee_alloc_cmd_buffer(tee);
>>> +    if (!cmd_buffer) {
>>>            tee_free_ring(tee);
>>>            return -ENOMEM;
>>>        }
>>>    -    cmd_buffer = __psp_pa((void *)cmd);
>>> -
>>>        /* Send command buffer details to Trusted OS by writing to
>>>         * CPU-PSP message registers
>>>         */
>>>    -    iowrite32(lower_32_bits(cmd_buffer),
>>> +    iowrite32(lower_32_bits(cmd_buffer->paddr),
>>>              tee->io_regs + tee->vdata->cmdbuff_addr_lo_reg);
>>> -    iowrite32(upper_32_bits(cmd_buffer),
>>> +    iowrite32(upper_32_bits(cmd_buffer->paddr),
>>>              tee->io_regs + tee->vdata->cmdbuff_addr_hi_reg);
>>>        iowrite32(TEE_RING_INIT_CMD,
>>>              tee->io_regs + tee->vdata->cmdresp_reg);
>>> @@ -157,7 +189,7 @@ static int tee_init_ring(struct psp_tee_device *tee)
>>>        }
>>>      free_buf:
>>> -    tee_free_cmd_buffer(cmd);
>>> +    tee_free_cmd_buffer(cmd_buffer);
>>>          return ret;
>>>    }
>>> @@ -167,7 +199,7 @@ static void tee_destroy_ring(struct psp_tee_device *tee)
>>>        unsigned int reg;
>>>        int ret;
>>>    -    if (!tee->rb_mgr.ring_start)
>>> +    if (!tee->rb_mgr.ring_buf->vaddr)
>>>            return;
>>>          if (psp_dead)
>>> @@ -256,7 +288,7 @@ static int tee_submit_cmd(struct psp_tee_device *tee, enum tee_cmd_id cmd_id,
>>>        do {
>>>            /* Get pointer to ring buffer command entry */
>>>            cmd = (struct tee_ring_cmd *)
>>> -            (tee->rb_mgr.ring_start + tee->rb_mgr.wptr);
>>> +            (tee->rb_mgr.ring_buf->vaddr + tee->rb_mgr.wptr);
>>>              rptr = ioread32(tee->io_regs + tee->vdata->ring_rptr_reg);
>>>    @@ -305,7 +337,7 @@ static int tee_submit_cmd(struct psp_tee_device *tee, enum tee_cmd_id cmd_id,
>>>          /* Update local copy of write pointer */
>>>        tee->rb_mgr.wptr += sizeof(struct tee_ring_cmd);
>>> -    if (tee->rb_mgr.wptr >= tee->rb_mgr.ring_size)
>>> +    if (tee->rb_mgr.wptr >= tee->rb_mgr.ring_buf->size)
>>>            tee->rb_mgr.wptr = 0;
>>>          /* Trigger interrupt to Trusted OS */
>>> diff --git a/drivers/crypto/ccp/tee-dev.h b/drivers/crypto/ccp/tee-dev.h
>>> index 49d26158b71e..9238487ee8bf 100644
>>> --- a/drivers/crypto/ccp/tee-dev.h
>>> +++ b/drivers/crypto/ccp/tee-dev.h
>>> @@ -16,6 +16,7 @@
>>>      #include <linux/device.h>
>>>    #include <linux/mutex.h>
>>> +#include <linux/psp-tee.h>
>>>      #define TEE_DEFAULT_TIMEOUT        10
>>>    #define MAX_BUFFER_SIZE            988
>>> @@ -48,17 +49,13 @@ struct tee_init_ring_cmd {
>>>      /**
>>>     * struct ring_buf_manager - Helper structure to manage ring buffer.
>>> - * @ring_start:  starting address of ring buffer
>>> - * @ring_size:   size of ring buffer in bytes
>>> - * @ring_pa:     physical address of ring buffer
>>>     * @wptr:        index to the last written entry in ring buffer
>>> + * @ring_buf:    ring buffer allocated using DMA api
>>>     */
>>>    struct ring_buf_manager {
>>>        struct mutex mutex;    /* synchronizes access to ring buffer */
>>> -    void *ring_start;
>>> -    u32 ring_size;
>>> -    phys_addr_t ring_pa;
>>>        u32 wptr;
>>> +    struct dma_buffer *ring_buf;
>>>    };
>>>      struct psp_tee_device {
>>> diff --git a/include/linux/psp-tee.h b/include/linux/psp-tee.h
>>> index cb0c95d6d76b..c0fa922f24d4 100644
>>> --- a/include/linux/psp-tee.h
>>> +++ b/include/linux/psp-tee.h
>>> @@ -13,6 +13,7 @@
>>>      #include <linux/types.h>
>>>    #include <linux/errno.h>
>>> +#include <linux/dma-mapping.h>
>>>      /* This file defines the Trusted Execution Environment (TEE) interface commands
>>>     * and the API exported by AMD Secure Processor driver to communicate with
>>> @@ -40,6 +41,20 @@ enum tee_cmd_id {
>>>        TEE_CMD_ID_UNMAP_SHARED_MEM,
>>>    };
>>>    +/**
>>> + * struct dma_buffer - Structure for a DMA buffer.
>>> + * @dma:    DMA buffer address
>>> + * @paddr:  Physical address of DMA buffer
>>> + * @vaddr:  CPU virtual address of DMA buffer
>>> + * @size:   Size of DMA buffer in bytes
>>> + */
>>> +struct dma_buffer {
>>> +    dma_addr_t dma;
>>> +    phys_addr_t paddr;
>>> +    void *vaddr;
>>> +    unsigned long size;
>>> +};
>>> +
>>>    #ifdef CONFIG_CRYPTO_DEV_SP_PSP
>>>    /**
>>>     * psp_tee_process_cmd() - Process command in Trusted Execution Environment
>>> @@ -75,6 +90,28 @@ int psp_tee_process_cmd(enum tee_cmd_id cmd_id, void *buf, size_t len,
>>>     */
>>>    int psp_check_tee_status(void);
>>>    +/**
>>> + * psp_tee_alloc_dmabuf() - Allocates memory of requested size and flags using
>>> + * dma_alloc_coherent() API.
>>> + *
>>> + * This function can be used to allocate a shared memory region between the
>>> + * host and PSP TEE.
>>> + *
>>> + * Returns:
>>> + * non-NULL   a valid pointer to struct dma_buffer
>>> + * NULL       on failure
>>> + */
>>> +struct dma_buffer *psp_tee_alloc_dmabuf(unsigned long size, gfp_t gfp);
>>> +
>>> +/**
>>> + * psp_tee_free_dmabuf() - Deallocates memory using dma_free_coherent() API.
>>> + *
>>> + * This function can be used to release shared memory region between host
>>> + * and PSP TEE.
>>> + *
>>> + */
>>> +void psp_tee_free_dmabuf(struct dma_buffer *dma_buffer);
>>> +
>>>    #else /* !CONFIG_CRYPTO_DEV_SP_PSP */
>>>      static inline int psp_tee_process_cmd(enum tee_cmd_id cmd_id, void *buf,
>>> @@ -87,5 +124,15 @@ static inline int psp_check_tee_status(void)
>>>    {
>>>        return -ENODEV;
>>>    }
>>> +
>>> +static inline
>>> +struct dma_buffer *psp_tee_alloc_dmabuf(unsigned long size, gfp_t gfp)
>>> +{
>>> +    return NULL;
>>> +}
>>> +
>>> +static inline void psp_tee_free_dmabuf(struct dma_buffer *dma_buffer)
>>> +{
>>> +}
>>>    #endif /* CONFIG_CRYPTO_DEV_SP_PSP */
>>>    #endif /* __PSP_TEE_H_ */
  

On 12/12/2022 9:13 PM, Tom Lendacky wrote:
> On 12/12/22 05:21, Rijo Thomas wrote:
>> On 12/10/2022 2:31 AM, Tom Lendacky wrote:
>>> On 12/6/22 06:30, Rijo Thomas wrote:
>>>> For AMD Secure Processor (ASP) to map and access TEE ring buffer, the
>>>> ring buffer address sent by host to ASP must be a real physical
>>>> address and the pages must be physically contiguous.
>>>>
>>>> In a virtualized environment though, when the driver is running in a
>>>> guest VM, the pages allocated by __get_free_pages() may not be
>>>> contiguous in the host (or machine) physical address space. Guests
>>>> will see a guest (or pseudo) physical address and not the actual host
>>>> (or machine) physical address. The TEE running on ASP cannot decipher
>>>> pseudo physical addresses. It needs host or machine physical address.
>>>>
>>>> To resolve this problem, use DMA APIs for allocating buffers that must
>>>> be shared with TEE. This will ensure that the pages are contiguous in
>>>> host (or machine) address space. If the DMA handle is an IOVA,
>>>> translate it into a physical address before sending it to ASP.
>>>>
>>>> This patch also exports two APIs (one for buffer allocation and
>>>> another to free the buffer). This API can be used by AMD-TEE driver to
>>>> share buffers with TEE.
>>>>
>>>> Fixes: 33960acccfbd ("crypto: ccp - add TEE support for Raven Ridge")
>>>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>>>> Cc: stable@vger.kernel.org
>>>> Signed-off-by: Rijo Thomas <Rijo-john.Thomas@amd.com>
>>>> Co-developed-by: Jeshwanth <JESHWANTHKUMAR.NK@amd.com>
>>>> Signed-off-by: Jeshwanth <JESHWANTHKUMAR.NK@amd.com>
>>>> Reviewed-by: Devaraj Rangasamy <Devaraj.Rangasamy@amd.com>
>>>> ---
>>>>    drivers/crypto/ccp/psp-dev.c |   6 +-
>>>>    drivers/crypto/ccp/tee-dev.c | 116 ++++++++++++++++++++++-------------
>>>>    drivers/crypto/ccp/tee-dev.h |   9 +--
>>>>    include/linux/psp-tee.h      |  47 ++++++++++++++
>>>>    4 files changed, 127 insertions(+), 51 deletions(-)
>>>>
>>>> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
>>>> index c9c741ac8442..2b86158d7435 100644
>>>> --- a/drivers/crypto/ccp/psp-dev.c
>>>> +++ b/drivers/crypto/ccp/psp-dev.c
>>>> @@ -161,13 +161,13 @@ int psp_dev_init(struct sp_device *sp)
>>>>            goto e_err;
>>>>        }
>>>>    +    if (sp->set_psp_master_device)
>>>> +        sp->set_psp_master_device(sp);
>>>> +
>>>
>>> This worries me a bit...  if psp_init() fails, it may still be referenced as the master device. What's the reason for moving it?
>>
>> Hmm. Okay, I see your point.
>>
>> In psp_tee_alloc_dmabuf(), we call psp_get_master_device(). Without above change, psp_get_master_device() returns NULL.
>>
>> I think in psp_dev_init(), we can add below error handling:
>>
>> ret = psp_init(psp);
>>          if (ret)
>>                  goto e_init;
>>       ...
>>
>> e_init:
>>      if (sp->clear_psp_master_device)
>>          sp->clear_psp_master_device(sp);
>>
>> Will this help address your concern?
> 
> Yes, that works.
> 
>>
>>>
>>>>        ret = psp_init(psp);
>>>>        if (ret)
>>>>            goto e_irq;
>>>>    -    if (sp->set_psp_master_device)
>>>> -        sp->set_psp_master_device(sp);
>>>> -
>>>>        /* Enable interrupt */
>>>>        iowrite32(-1, psp->io_regs + psp->vdata->inten_reg);
>>>>    diff --git a/drivers/crypto/ccp/tee-dev.c b/drivers/crypto/ccp/tee-dev.c
>>>> index 5c9d47f3be37..1631d9851e54 100644
>>>> --- a/drivers/crypto/ccp/tee-dev.c
>>>> +++ b/drivers/crypto/ccp/tee-dev.c
>>>> @@ -12,8 +12,9 @@
>>>>    #include <linux/mutex.h>
>>>>    #include <linux/delay.h>
>>>>    #include <linux/slab.h>
>>>> +#include <linux/dma-direct.h>
>>>> +#include <linux/iommu.h>
>>>>    #include <linux/gfp.h>
>>>> -#include <linux/psp-sev.h>
>>>>    #include <linux/psp-tee.h>
>>>>      #include "psp-dev.h"
>>>> @@ -21,25 +22,64 @@
>>>>      static bool psp_dead;
>>>>    +struct dma_buffer *psp_tee_alloc_dmabuf(unsigned long size, gfp_t gfp)
>>>
>>> It looks like both calls to this use the same gfp_t values, you can probably eliminate from the call and just specify them in here.
>>>
>>
>> Okay, I will remove gfp_t flag from the function argument.
>>
>>>> +{
>>>> +    struct psp_device *psp = psp_get_master_device();
>>>> +    struct dma_buffer *dma_buf;
>>>> +    struct iommu_domain *dom;
>>>> +
>>>> +    if (!psp || !size)
>>>> +        return NULL;
>>>> +
>>>> +    dma_buf = kzalloc(sizeof(*dma_buf), GFP_KERNEL);
>>>> +    if (!dma_buf)
>>>> +        return NULL;
>>>> +
>>>> +    dma_buf->vaddr = dma_alloc_coherent(psp->dev, size, &dma_buf->dma, gfp);
>>>> +    if (!dma_buf->vaddr || !dma_buf->dma) {
>>>
>>> I don't think you can have one of these be NULL without both being NULL, but I guess it doesn't hurt to check.
>>>
>>
>> Okay, we will keep both checks for now.
>>
>>>> +        kfree(dma_buf);
>>>> +        return NULL;
>>>> +    }
>>>> +
>>>> +    dma_buf->size = size;
>>>> + > +    dom = iommu_get_domain_for_dev(psp->dev);
>>>> +    if (dom)
>>>
>>> You need a nice comment above this all explaining that. I guess you're using the presence of a domain to determine whether you're running on bare-metal vs within a hypervisor? I'm not sure what will happen if the guest ever gets an emulated IOMMU...
>>
>> Sure we will add a comment.
>>
>> We are not trying to determive bare-metal vs hypervisor here, but determine whether DMA handle returned by dma_alloc_coherent() is an IOVA or not.
>> If the address is an IOVA, we convert IOVA to physical address using iommu_iova_to_phys(). This was our intention.
> 
> Ok, but if a VM gets an emulated IOMMU and ends up with an IOVA, that IOVA points to a GPA - and if the size is over a page, then you aren't guaranteed to have contiguous physical memory.
> 
> Probably not a concern at the moment, but not sure about what happens in the future.
> 

We are not having a VM with an emulated IOMMU. We will have to handle it in case a need arises in future.

If iommu_iova_to_phys() returns a GPA, then this patch will not be functional.

We ran the VM as a paravirtualized guest. So, the guest allocations using dma_alloc_coherent() gave us memory that was contiguous in host or machine address space.

>>
>>>
>>>> +        dma_buf->paddr = iommu_iova_to_phys(dom, dma_buf->dma);
>>>
>>> If you're just looking to get the physical address, why not just to an __pa(dma_buf->vaddr)?
>>>
>>> Also, paddr might not be the best name, since it isn't always a physical address, but I can't really think of something right now.
>>>
>>
>> We can use __pa(dma_buf->vaddr) only on bare-metal. In hypervisor, __pa(dma_buf->vaddr) gives pseudo-physical address; pseudo-physical address cannot be understood by ASP.
>> ASP needs real physical address (aka machine address). Please see commit message.
>>
>> We chose the name paddr, since it's a (real) physical address that we want to send across to ASP. I am not sure, why you say - it isn't always a physical address.
> 
> Then a nice comment above this explaining all that and the fact that when there is no IOMMU the DMA address is actually the physical address, would be appropriate. A number of years from now, this will raise questions without thorough documentation.

Agreed. We will add proper comment. We will keep the name paddr for now.

Thanks,
Rijo

> 
> Thanks,
> Tom
> 
>>
>> Thanks,
>> Rijo
>>
>>> Thanks,
>>> Tom
>>>
>>>> +    else
>>>> +        dma_buf->paddr = dma_buf->dma;
>>>> +
>>>> +    return dma_buf;
>>>> +}
>>>> +EXPORT_SYMBOL(psp_tee_alloc_dmabuf);
>>>> +
>>>> +void psp_tee_free_dmabuf(struct dma_buffer *dma_buf)
>>>> +{
>>>> +    struct psp_device *psp = psp_get_master_device();
>>>> +
>>>> +    if (!psp || !dma_buf)
>>>> +        return;
>>>> +
>>>> +    dma_free_coherent(psp->dev, dma_buf->size,
>>>> +              dma_buf->vaddr, dma_buf->dma);
>>>> +
>>>> +    kfree(dma_buf);
>>>> +}
>>>> +EXPORT_SYMBOL(psp_tee_free_dmabuf);
>>>> +
>>>>    static int tee_alloc_ring(struct psp_tee_device *tee, int ring_size)
>>>>    {
>>>>        struct ring_buf_manager *rb_mgr = &tee->rb_mgr;
>>>> -    void *start_addr;
>>>>          if (!ring_size)
>>>>            return -EINVAL;
>>>>    -    /* We need actual physical address instead of DMA address, since
>>>> -     * Trusted OS running on AMD Secure Processor will map this region
>>>> -     */
>>>> -    start_addr = (void *)__get_free_pages(GFP_KERNEL, get_order(ring_size));
>>>> -    if (!start_addr)
>>>> +    rb_mgr->ring_buf = psp_tee_alloc_dmabuf(ring_size,
>>>> +                        GFP_KERNEL | __GFP_ZERO);
>>>> +    if (!rb_mgr->ring_buf) {
>>>> +        dev_err(tee->dev, "ring allocation failed\n");
>>>>            return -ENOMEM;
>>>> -
>>>> -    memset(start_addr, 0x0, ring_size);
>>>> -    rb_mgr->ring_start = start_addr;
>>>> -    rb_mgr->ring_size = ring_size;
>>>> -    rb_mgr->ring_pa = __psp_pa(start_addr);
>>>> +    }
>>>>        mutex_init(&rb_mgr->mutex);
>>>>          return 0;
>>>> @@ -49,15 +89,8 @@ static void tee_free_ring(struct psp_tee_device *tee)
>>>>    {
>>>>        struct ring_buf_manager *rb_mgr = &tee->rb_mgr;
>>>>    -    if (!rb_mgr->ring_start)
>>>> -        return;
>>>> +    psp_tee_free_dmabuf(rb_mgr->ring_buf);
>>>>    -    free_pages((unsigned long)rb_mgr->ring_start,
>>>> -           get_order(rb_mgr->ring_size));
>>>> -
>>>> -    rb_mgr->ring_start = NULL;
>>>> -    rb_mgr->ring_size = 0;
>>>> -    rb_mgr->ring_pa = 0;
>>>>        mutex_destroy(&rb_mgr->mutex);
>>>>    }
>>>>    @@ -81,35 +114,36 @@ static int tee_wait_cmd_poll(struct psp_tee_device *tee, unsigned int timeout,
>>>>        return -ETIMEDOUT;
>>>>    }
>>>>    -static
>>>> -struct tee_init_ring_cmd *tee_alloc_cmd_buffer(struct psp_tee_device *tee)
>>>> +struct dma_buffer *tee_alloc_cmd_buffer(struct psp_tee_device *tee)
>>>>    {
>>>>        struct tee_init_ring_cmd *cmd;
>>>> +    struct dma_buffer *cmd_buffer;
>>>>    -    cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
>>>> -    if (!cmd)
>>>> +    cmd_buffer = psp_tee_alloc_dmabuf(sizeof(*cmd),
>>>> +                      GFP_KERNEL | __GFP_ZERO);
>>>> +    if (!cmd_buffer)
>>>>            return NULL;
>>>>    -    cmd->hi_addr = upper_32_bits(tee->rb_mgr.ring_pa);
>>>> -    cmd->low_addr = lower_32_bits(tee->rb_mgr.ring_pa);
>>>> -    cmd->size = tee->rb_mgr.ring_size;
>>>> +    cmd = (struct tee_init_ring_cmd *)cmd_buffer->vaddr;
>>>> +    cmd->hi_addr = upper_32_bits(tee->rb_mgr.ring_buf->paddr);
>>>> +    cmd->low_addr = lower_32_bits(tee->rb_mgr.ring_buf->paddr);
>>>> +    cmd->size = tee->rb_mgr.ring_buf->size;
>>>>          dev_dbg(tee->dev, "tee: ring address: high = 0x%x low = 0x%x size = %u\n",
>>>>            cmd->hi_addr, cmd->low_addr, cmd->size);
>>>>    -    return cmd;
>>>> +    return cmd_buffer;
>>>>    }
>>>>    -static inline void tee_free_cmd_buffer(struct tee_init_ring_cmd *cmd)
>>>> +static inline void tee_free_cmd_buffer(struct dma_buffer *cmd_buffer)
>>>>    {
>>>> -    kfree(cmd);
>>>> +    psp_tee_free_dmabuf(cmd_buffer);
>>>>    }
>>>>      static int tee_init_ring(struct psp_tee_device *tee)
>>>>    {
>>>>        int ring_size = MAX_RING_BUFFER_ENTRIES * sizeof(struct tee_ring_cmd);
>>>> -    struct tee_init_ring_cmd *cmd;
>>>> -    phys_addr_t cmd_buffer;
>>>> +    struct dma_buffer *cmd_buffer;
>>>>        unsigned int reg;
>>>>        int ret;
>>>>    @@ -123,21 +157,19 @@ static int tee_init_ring(struct psp_tee_device *tee)
>>>>          tee->rb_mgr.wptr = 0;
>>>>    -    cmd = tee_alloc_cmd_buffer(tee);
>>>> -    if (!cmd) {
>>>> +    cmd_buffer = tee_alloc_cmd_buffer(tee);
>>>> +    if (!cmd_buffer) {
>>>>            tee_free_ring(tee);
>>>>            return -ENOMEM;
>>>>        }
>>>>    -    cmd_buffer = __psp_pa((void *)cmd);
>>>> -
>>>>        /* Send command buffer details to Trusted OS by writing to
>>>>         * CPU-PSP message registers
>>>>         */
>>>>    -    iowrite32(lower_32_bits(cmd_buffer),
>>>> +    iowrite32(lower_32_bits(cmd_buffer->paddr),
>>>>              tee->io_regs + tee->vdata->cmdbuff_addr_lo_reg);
>>>> -    iowrite32(upper_32_bits(cmd_buffer),
>>>> +    iowrite32(upper_32_bits(cmd_buffer->paddr),
>>>>              tee->io_regs + tee->vdata->cmdbuff_addr_hi_reg);
>>>>        iowrite32(TEE_RING_INIT_CMD,
>>>>              tee->io_regs + tee->vdata->cmdresp_reg);
>>>> @@ -157,7 +189,7 @@ static int tee_init_ring(struct psp_tee_device *tee)
>>>>        }
>>>>      free_buf:
>>>> -    tee_free_cmd_buffer(cmd);
>>>> +    tee_free_cmd_buffer(cmd_buffer);
>>>>          return ret;
>>>>    }
>>>> @@ -167,7 +199,7 @@ static void tee_destroy_ring(struct psp_tee_device *tee)
>>>>        unsigned int reg;
>>>>        int ret;
>>>>    -    if (!tee->rb_mgr.ring_start)
>>>> +    if (!tee->rb_mgr.ring_buf->vaddr)
>>>>            return;
>>>>          if (psp_dead)
>>>> @@ -256,7 +288,7 @@ static int tee_submit_cmd(struct psp_tee_device *tee, enum tee_cmd_id cmd_id,
>>>>        do {
>>>>            /* Get pointer to ring buffer command entry */
>>>>            cmd = (struct tee_ring_cmd *)
>>>> -            (tee->rb_mgr.ring_start + tee->rb_mgr.wptr);
>>>> +            (tee->rb_mgr.ring_buf->vaddr + tee->rb_mgr.wptr);
>>>>              rptr = ioread32(tee->io_regs + tee->vdata->ring_rptr_reg);
>>>>    @@ -305,7 +337,7 @@ static int tee_submit_cmd(struct psp_tee_device *tee, enum tee_cmd_id cmd_id,
>>>>          /* Update local copy of write pointer */
>>>>        tee->rb_mgr.wptr += sizeof(struct tee_ring_cmd);
>>>> -    if (tee->rb_mgr.wptr >= tee->rb_mgr.ring_size)
>>>> +    if (tee->rb_mgr.wptr >= tee->rb_mgr.ring_buf->size)
>>>>            tee->rb_mgr.wptr = 0;
>>>>          /* Trigger interrupt to Trusted OS */
>>>> diff --git a/drivers/crypto/ccp/tee-dev.h b/drivers/crypto/ccp/tee-dev.h
>>>> index 49d26158b71e..9238487ee8bf 100644
>>>> --- a/drivers/crypto/ccp/tee-dev.h
>>>> +++ b/drivers/crypto/ccp/tee-dev.h
>>>> @@ -16,6 +16,7 @@
>>>>      #include <linux/device.h>
>>>>    #include <linux/mutex.h>
>>>> +#include <linux/psp-tee.h>
>>>>      #define TEE_DEFAULT_TIMEOUT        10
>>>>    #define MAX_BUFFER_SIZE            988
>>>> @@ -48,17 +49,13 @@ struct tee_init_ring_cmd {
>>>>      /**
>>>>     * struct ring_buf_manager - Helper structure to manage ring buffer.
>>>> - * @ring_start:  starting address of ring buffer
>>>> - * @ring_size:   size of ring buffer in bytes
>>>> - * @ring_pa:     physical address of ring buffer
>>>>     * @wptr:        index to the last written entry in ring buffer
>>>> + * @ring_buf:    ring buffer allocated using DMA api
>>>>     */
>>>>    struct ring_buf_manager {
>>>>        struct mutex mutex;    /* synchronizes access to ring buffer */
>>>> -    void *ring_start;
>>>> -    u32 ring_size;
>>>> -    phys_addr_t ring_pa;
>>>>        u32 wptr;
>>>> +    struct dma_buffer *ring_buf;
>>>>    };
>>>>      struct psp_tee_device {
>>>> diff --git a/include/linux/psp-tee.h b/include/linux/psp-tee.h
>>>> index cb0c95d6d76b..c0fa922f24d4 100644
>>>> --- a/include/linux/psp-tee.h
>>>> +++ b/include/linux/psp-tee.h
>>>> @@ -13,6 +13,7 @@
>>>>      #include <linux/types.h>
>>>>    #include <linux/errno.h>
>>>> +#include <linux/dma-mapping.h>
>>>>      /* This file defines the Trusted Execution Environment (TEE) interface commands
>>>>     * and the API exported by AMD Secure Processor driver to communicate with
>>>> @@ -40,6 +41,20 @@ enum tee_cmd_id {
>>>>        TEE_CMD_ID_UNMAP_SHARED_MEM,
>>>>    };
>>>>    +/**
>>>> + * struct dma_buffer - Structure for a DMA buffer.
>>>> + * @dma:    DMA buffer address
>>>> + * @paddr:  Physical address of DMA buffer
>>>> + * @vaddr:  CPU virtual address of DMA buffer
>>>> + * @size:   Size of DMA buffer in bytes
>>>> + */
>>>> +struct dma_buffer {
>>>> +    dma_addr_t dma;
>>>> +    phys_addr_t paddr;
>>>> +    void *vaddr;
>>>> +    unsigned long size;
>>>> +};
>>>> +
>>>>    #ifdef CONFIG_CRYPTO_DEV_SP_PSP
>>>>    /**
>>>>     * psp_tee_process_cmd() - Process command in Trusted Execution Environment
>>>> @@ -75,6 +90,28 @@ int psp_tee_process_cmd(enum tee_cmd_id cmd_id, void *buf, size_t len,
>>>>     */
>>>>    int psp_check_tee_status(void);
>>>>    +/**
>>>> + * psp_tee_alloc_dmabuf() - Allocates memory of requested size and flags using
>>>> + * dma_alloc_coherent() API.
>>>> + *
>>>> + * This function can be used to allocate a shared memory region between the
>>>> + * host and PSP TEE.
>>>> + *
>>>> + * Returns:
>>>> + * non-NULL   a valid pointer to struct dma_buffer
>>>> + * NULL       on failure
>>>> + */
>>>> +struct dma_buffer *psp_tee_alloc_dmabuf(unsigned long size, gfp_t gfp);
>>>> +
>>>> +/**
>>>> + * psp_tee_free_dmabuf() - Deallocates memory using dma_free_coherent() API.
>>>> + *
>>>> + * This function can be used to release shared memory region between host
>>>> + * and PSP TEE.
>>>> + *
>>>> + */
>>>> +void psp_tee_free_dmabuf(struct dma_buffer *dma_buffer);
>>>> +
>>>>    #else /* !CONFIG_CRYPTO_DEV_SP_PSP */
>>>>      static inline int psp_tee_process_cmd(enum tee_cmd_id cmd_id, void *buf,
>>>> @@ -87,5 +124,15 @@ static inline int psp_check_tee_status(void)
>>>>    {
>>>>        return -ENODEV;
>>>>    }
>>>> +
>>>> +static inline
>>>> +struct dma_buffer *psp_tee_alloc_dmabuf(unsigned long size, gfp_t gfp)
>>>> +{
>>>> +    return NULL;
>>>> +}
>>>> +
>>>> +static inline void psp_tee_free_dmabuf(struct dma_buffer *dma_buffer)
>>>> +{
>>>> +}
>>>>    #endif /* CONFIG_CRYPTO_DEV_SP_PSP */
>>>>    #endif /* __PSP_TEE_H_ */