[PATCH v1 0/2] Memory allocation change in scm/mdt_loader

[PATCH v1 0/2] Memory allocation change in scm/mdt_loader

[Gokul krishna Krishnakumar]

This patch attempts to remove the memory allocation call for the
qcom_metadata from the scm driver and move it to mdt_loader.

Gokul krishna Krishnakumar (2):
  firmware: qcom_scm: Remove memory alloc call from
    qcom_scm_pas_init_image()
  soc: qcom: mdt_loader: Move the memory allocation into mdt loader

 drivers/firmware/qcom_scm.c         | 33 +++-------------------------
 drivers/remoteproc/qcom_q6v5_mss.c  |  2 +-
 drivers/soc/qcom/mdt_loader.c       | 44 +++++++++++++++++++++++++++----------
 include/linux/qcom_scm.h            |  4 +---
 include/linux/soc/qcom/mdt_loader.h |  5 +++--
 5 files changed, 41 insertions(+), 47 deletions(-)

-- 
2.7.4

[PATCH v1 1/2] firmware: qcom_scm: Remove memory alloc call from qcom_scm_pas_init_image()

[Gokul krishna Krishnakumar]

The memory for metadata is allocated in mdt loader and scm driver

1].  163  data = kmalloc(ehdr_size + hash_size, GFP_KERNEL);
2].  477  mdata_buf = dma_alloc_coherent(__scm->dev, size, &mdata_phys,
     478                                        GFP_KERNEL);

The SCM driver is meant to only pack arguments provided to it from clients
for making the secure world calls. This change removes the logic of doing
a dma alloc coherent from the SCM driver and moves it into the mdt loader

Signed-off-by: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com>
---
 drivers/firmware/qcom_scm.c   | 33 +++------------------------------
 drivers/soc/qcom/mdt_loader.c |  3 ++-
 include/linux/qcom_scm.h      |  4 +---
 3 files changed, 6 insertions(+), 34 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index cdbfe54..05ec03c 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -442,11 +442,9 @@ static void qcom_scm_set_download_mode(bool enable)
  *			       state machine for a given peripheral, using the
  *			       metadata
  * @peripheral: peripheral id
- * @metadata:	pointer to memory containing ELF header, program header table
+ * @mdata_phys:	pointer to memory containing ELF header, program header table
  *		and optional blob of data used for authenticating the metadata
  *		and the rest of the firmware
- * @size:	size of the metadata
- * @ctx:	optional metadata context
  *
  * Return: 0 on success.
  *
@@ -454,11 +452,8 @@ static void qcom_scm_set_download_mode(bool enable)
  * track the metadata allocation, this needs to be released by invoking
  * qcom_scm_pas_metadata_release() by the caller.
  */
-int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
-			    struct qcom_scm_pas_metadata *ctx)
+int qcom_scm_pas_init_image(u32 peripheral, dma_addr_t mdata_phys)
 {
-	dma_addr_t mdata_phys;
-	void *mdata_buf;
 	int ret;
 	struct qcom_scm_desc desc = {
 		.svc = QCOM_SCM_SVC_PIL,
@@ -469,22 +464,9 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
 	};
 	struct qcom_scm_res res;
 
-	/*
-	 * During the scm call memory protection will be enabled for the meta
-	 * data blob, so make sure it's physically contiguous, 4K aligned and
-	 * non-cachable to avoid XPU violations.
-	 */
-	mdata_buf = dma_alloc_coherent(__scm->dev, size, &mdata_phys,
-				       GFP_KERNEL);
-	if (!mdata_buf) {
-		dev_err(__scm->dev, "Allocation of metadata buffer failed.\n");
-		return -ENOMEM;
-	}
-	memcpy(mdata_buf, metadata, size);
-
 	ret = qcom_scm_clk_enable();
 	if (ret)
-		goto out;
+		return ret;
 
 	ret = qcom_scm_bw_enable();
 	if (ret)
@@ -497,15 +479,6 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
 	qcom_scm_bw_disable();
 	qcom_scm_clk_disable();
 
-out:
-	if (ret < 0 || !ctx) {
-		dma_free_coherent(__scm->dev, size, mdata_buf, mdata_phys);
-	} else if (ctx) {
-		ctx->ptr = mdata_buf;
-		ctx->phys = mdata_phys;
-		ctx->size = size;
-	}
-
 	return ret ? : res.result[0];
 }
 EXPORT_SYMBOL(qcom_scm_pas_init_image);
diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
index 3f11554..8d06125 100644
--- a/drivers/soc/qcom/mdt_loader.c
+++ b/drivers/soc/qcom/mdt_loader.c
@@ -210,6 +210,7 @@ int qcom_mdt_pas_init(struct device *dev, const struct firmware *fw,
 	const struct elf32_hdr *ehdr;
 	phys_addr_t min_addr = PHYS_ADDR_MAX;
 	phys_addr_t max_addr = 0;
+	dma_addr_t mdata_phys;
 	size_t metadata_len;
 	void *metadata;
 	int ret;
@@ -238,7 +239,7 @@ int qcom_mdt_pas_init(struct device *dev, const struct firmware *fw,
 		goto out;
 	}
 
-	ret = qcom_scm_pas_init_image(pas_id, metadata, metadata_len, ctx);
+	ret = qcom_scm_pas_init_image(pas_id, mdata_phys);
 	kfree(metadata);
 	if (ret) {
 		/* Invalid firmware metadata */
diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
index f833564..751436a 100644
--- a/include/linux/qcom_scm.h
+++ b/include/linux/qcom_scm.h
@@ -74,9 +74,7 @@ struct qcom_scm_pas_metadata {
 	ssize_t size;
 };
 
-extern int qcom_scm_pas_init_image(u32 peripheral, const void *metadata,
-				   size_t size,
-				   struct qcom_scm_pas_metadata *ctx);
+extern int qcom_scm_pas_init_image(u32 peripheral, dma_addr_t metadata);
 void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx);
 extern int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr,
 				  phys_addr_t size);
-- 
2.7.4

[PATCH v1 2/2] soc: qcom: mdt_loader: Move the memory allocation into mdt loader

[Gokul krishna Krishnakumar]

By moving the memory allocation to mdt loader we can simplify the scm
call, by just packing arguments provided to it from the clients for
making secuer world calls. We can also simplify the memory allocation
for the qcom metadata, by just doing one memory allocation in the
mdt loader.

Signed-off-by: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com>
---
 drivers/remoteproc/qcom_q6v5_mss.c  |  2 +-
 drivers/soc/qcom/mdt_loader.c       | 41 ++++++++++++++++++++++++++++---------
 include/linux/soc/qcom/mdt_loader.h |  5 +++--
 3 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
index fddb63c..1919bfc 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -947,7 +947,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw,
 	int ret;
 	int i;
 
-	metadata = qcom_mdt_read_metadata(fw, &size, fw_name, qproc->dev);
+	metadata = qcom_mdt_read_metadata(fw, &size, fw_name, qproc->dev, NULL);
 	if (IS_ERR(metadata))
 		return PTR_ERR(metadata);
 
diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
index 8d06125..e730413 100644
--- a/drivers/soc/qcom/mdt_loader.c
+++ b/drivers/soc/qcom/mdt_loader.c
@@ -16,6 +16,7 @@
 #include <linux/sizes.h>
 #include <linux/slab.h>
 #include <linux/soc/qcom/mdt_loader.h>
+#include <linux/dma-mapping.h>
 
 static bool mdt_phdr_valid(const struct elf32_phdr *phdr)
 {
@@ -110,6 +111,7 @@ EXPORT_SYMBOL_GPL(qcom_mdt_get_size);
  * @data_len:	length of the read metadata blob
  * @fw_name:	name of the firmware, for construction of segment file names
  * @dev:	device handle to associate resources with
+ * @mdata_phys:	phys address for the assigned metadata buffer
  *
  * The mechanism that performs the authentication of the loading firmware
  * expects an ELF header directly followed by the segment of hashes, with no
@@ -124,11 +126,13 @@ EXPORT_SYMBOL_GPL(qcom_mdt_get_size);
  * Return: pointer to data, or ERR_PTR()
  */
 void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len,
-			     const char *fw_name, struct device *dev)
+			     const char *fw_name, struct device *dev,
+			     dma_addr_t *mdata_phys)
 {
 	const struct elf32_phdr *phdrs;
 	const struct elf32_hdr *ehdr;
 	unsigned int hash_segment = 0;
+	struct device *scm_dev = NULL;
 	size_t hash_offset;
 	size_t hash_size;
 	size_t ehdr_size;
@@ -160,9 +164,18 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len,
 	ehdr_size = phdrs[0].p_filesz;
 	hash_size = phdrs[hash_segment].p_filesz;
 
-	data = kmalloc(ehdr_size + hash_size, GFP_KERNEL);
-	if (!data)
-		return ERR_PTR(-ENOMEM);
+	/*
+	 * During the scm call memory protection will be enabled for the meta
+	 * data blob, so make sure it's physically contiguous, 4K aligned and
+	 * non-cachable to avoid XPU violations.
+	 */
+	scm_dev = qcom_get_scm_device();
+	data = dma_alloc_coherent(scm_dev, ehdr_size + hash_size, mdata_phys,
+				       GFP_KERNEL);
+	if (!data) {
+		dev_err(dev, "Allocation of metadata buffer failed.\n");
+		return NULL;
+	}
 
 	/* Copy ELF header */
 	memcpy(data, fw->data, ehdr_size);
@@ -179,7 +192,7 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len,
 		/* Hash is in its own segment, beyond the loaded file */
 		ret = mdt_load_split_segment(data + ehdr_size, phdrs, hash_segment, fw_name, dev);
 		if (ret) {
-			kfree(data);
+			dma_free_coherent(scm_dev, ehdr_size + hash_size, data, mdata_phys);
 			return ERR_PTR(ret);
 		}
 	}
@@ -209,10 +222,11 @@ int qcom_mdt_pas_init(struct device *dev, const struct firmware *fw,
 	const struct elf32_phdr *phdr;
 	const struct elf32_hdr *ehdr;
 	phys_addr_t min_addr = PHYS_ADDR_MAX;
+	struct device *scm_dev = NULL;
 	phys_addr_t max_addr = 0;
 	dma_addr_t mdata_phys;
 	size_t metadata_len;
-	void *metadata;
+	void *mdata_buf;
 	int ret;
 	int i;
 
@@ -232,15 +246,22 @@ int qcom_mdt_pas_init(struct device *dev, const struct firmware *fw,
 			max_addr = ALIGN(phdr->p_paddr + phdr->p_memsz, SZ_4K);
 	}
 
-	metadata = qcom_mdt_read_metadata(fw, &metadata_len, fw_name, dev);
-	if (IS_ERR(metadata)) {
-		ret = PTR_ERR(metadata);
+	mdata_buf = qcom_mdt_read_metadata(fw, &metadata_len, fw_name, dev, &mdata_phys);
+	if (IS_ERR(mdata_buf)) {
+		ret = PTR_ERR(mdata_buf);
 		dev_err(dev, "error %d reading firmware %s metadata\n", ret, fw_name);
 		goto out;
 	}
 
 	ret = qcom_scm_pas_init_image(pas_id, mdata_phys);
-	kfree(metadata);
+	if (ret || !ctx) {
+		dma_free_coherent(scm_dev, metadata_len, mdata_buf, mdata_phys);
+	} else if (ctx) {
+		ctx->ptr = mdata_buf;
+		ctx->phys = mdata_phys;
+		ctx->size = metadata_len;
+	}
+
 	if (ret) {
 		/* Invalid firmware metadata */
 		dev_err(dev, "error %d initializing firmware %s\n", ret, fw_name);
diff --git a/include/linux/soc/qcom/mdt_loader.h b/include/linux/soc/qcom/mdt_loader.h
index 9e8e604..d438442 100644
--- a/include/linux/soc/qcom/mdt_loader.h
+++ b/include/linux/soc/qcom/mdt_loader.h
@@ -28,7 +28,8 @@ int qcom_mdt_load_no_init(struct device *dev, const struct firmware *fw,
 			  phys_addr_t mem_phys, size_t mem_size,
 			  phys_addr_t *reloc_base);
 void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len,
-			     const char *fw_name, struct device *dev);
+			     const char *fw_name, struct device *dev,
+			     dma_addr_t *mdata_phys);
 
 #else /* !IS_ENABLED(CONFIG_QCOM_MDT_LOADER) */
 
@@ -64,7 +65,7 @@ static inline int qcom_mdt_load_no_init(struct device *dev,
 
 static inline void *qcom_mdt_read_metadata(const struct firmware *fw,
 					   size_t *data_len, const char *fw_name,
-					   struct device *dev)
+					   struct device *dev, dma_addr_t *mdata_phys)
 {
 	return ERR_PTR(-ENODEV);
 }
-- 
2.7.4

Re: [PATCH v1 2/2] soc: qcom: mdt_loader: Move the memory allocation into mdt loader

[Bjorn Andersson]

On Mon, Sep 12, 2022 at 11:41:32AM -0700, Gokul krishna Krishnakumar wrote:
> By moving the memory allocation to mdt loader we can simplify the scm
> call, by just packing arguments provided to it from the clients for
> making secuer world calls. We can also simplify the memory allocation
> for the qcom metadata, by just doing one memory allocation in the
> mdt loader.
> 
> Signed-off-by: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com>
> ---
>  drivers/remoteproc/qcom_q6v5_mss.c  |  2 +-
>  drivers/soc/qcom/mdt_loader.c       | 41 ++++++++++++++++++++++++++++---------
>  include/linux/soc/qcom/mdt_loader.h |  5 +++--
>  3 files changed, 35 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> index fddb63c..1919bfc 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> @@ -947,7 +947,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw,
>  	int ret;
>  	int i;
>  
> -	metadata = qcom_mdt_read_metadata(fw, &size, fw_name, qproc->dev);
> +	metadata = qcom_mdt_read_metadata(fw, &size, fw_name, qproc->dev, NULL);

At the end of this function we invoke kfree(metadata), which would be
bad if that comes from dma_alloc_coherent().

>  	if (IS_ERR(metadata))
>  		return PTR_ERR(metadata);
>  
> diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
[..]
> @@ -160,9 +164,18 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len,
>  	ehdr_size = phdrs[0].p_filesz;
>  	hash_size = phdrs[hash_segment].p_filesz;
>  
> -	data = kmalloc(ehdr_size + hash_size, GFP_KERNEL);
> -	if (!data)
> -		return ERR_PTR(-ENOMEM);
> +	/*
> +	 * During the scm call memory protection will be enabled for the meta
> +	 * data blob, so make sure it's physically contiguous, 4K aligned and
> +	 * non-cachable to avoid XPU violations.
> +	 */
> +	scm_dev = qcom_get_scm_device();

As LKP points out, I don't seem to have this function.

> +	data = dma_alloc_coherent(scm_dev, ehdr_size + hash_size, mdata_phys,
> +				       GFP_KERNEL);

I am not thrilled about the idea of doing dma_alloc_coherent() in this
file and dma_free_coherent() in the scm driver. Similarly, I consider
these functions to operate in the context of the caller, so operating on
the scm device's struct device isn't so nice.


After trying various models I came to the conclusion that it was better
to try to keep the MDT loader to just load MDT files, and move the
SCM/PAS interaction out of that. Unfortunately we have a number of
client drivers that would then need to (essentially) duplicate the
content of qcom_mdt_pas_init() - so I left that in there.

I still believe that keeping the MDT loader focused on loading MDTs is a
good idea, but I'm open to any suggestions for improvements in the
interaction between these different components.

Regards,
Bjorn

RE: [PATCH v1 2/2] soc: qcom: mdt_loader: Move the memory allocation into mdt loader

[Gokul krishna Krishnakumar (QUIC)]

[-- Attachment #1: Type: text/plain, Size: 5712 bytes --]

>At the end of this function we invoke kfree(metadata), which would be bad if that comes from dma_alloc_coherent().
+       if (mdata_phys) {
+               data = dma_alloc_coherent(dev, ehdr_size + hash_size, mdata_phys,
+                                      GFP_KERNEL);
+       } else {
+               data = kmalloc(ehdr_size + hash_size, GFP_KERNEL);
Adding dma_alloc_coherent without affecting the mss driver.


> As LKP points out, I don't seem to have this function.
Removing the qcom_get_scm_device() and calling dma_alloc_coherent from device context.
+               data = dma_alloc_coherent(dev, ehdr_size + hash_size, mdata_phys,
+                                      GFP_KERNEL);

>I am not thrilled about the idea of doing dma_alloc_coherent() in this file and dma_free_coherent() in the scm driver. Similarly, I consider these functions to operate in the context of the caller, so operating on the scm device's struct device isn't so nice.
>After trying various models I came to the conclusion that it was better to try to keep the MDT loader to just load MDT files, and move the SCM/PAS interaction out of that. Unfortunately we have a number of client drivers that would then need to (essentially) duplicate the content of qcom_mdt_pas_init() - so I left >that in there.
>I still believe that keeping the MDT loader focused on loading MDTs is a good idea, but I'm open to any suggestions for improvements in the interaction between these different components.

With this patch we moving all the dma_alloc_coherent() and dma_free_coherent() to the MDT loader.
So now the MDT loader has the functionality of loading and allocating memory
and the SCM driver packs the arguments and makes a call to the secure world.

-----Original Message-----
From: Bjorn Andersson <andersson@kernel.org> 
Sent: Tuesday, September 13, 2022 4:11 PM
To: Gokul krishna Krishnakumar (QUIC) <quic_gokukris@quicinc.com>
Cc: Andy Gross <agross@kernel.org>; Konrad Dybcio <konrad.dybcio@somainline.org>; Philipp Zabel <p.zabel@pengutronix.de>; linux-arm-msm@vger.kernel.org; linux-kernel@vger.kernel.org; Trilok Soni (QUIC) <quic_tsoni@quicinc.com>; Satya Durga Srinivasu Prabhala (QUIC) <quic_satyap@quicinc.com>; Rajendra Nayak (QUIC) <quic_rjendra@quicinc.com>; Elliot Berman (QUIC) <quic_eberman@quicinc.com>; Guru Das Srinagesh (QUIC) <quic_gurus@quicinc.com>
Subject: Re: [PATCH v1 2/2] soc: qcom: mdt_loader: Move the memory allocation into mdt loader

WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.

On Mon, Sep 12, 2022 at 11:41:32AM -0700, Gokul krishna Krishnakumar wrote:
> By moving the memory allocation to mdt loader we can simplify the scm 
> call, by just packing arguments provided to it from the clients for 
> making secuer world calls. We can also simplify the memory allocation 
> for the qcom metadata, by just doing one memory allocation in the mdt 
> loader.
>
> Signed-off-by: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com>
> ---
>  drivers/remoteproc/qcom_q6v5_mss.c  |  2 +-
>  drivers/soc/qcom/mdt_loader.c       | 41 ++++++++++++++++++++++++++++---------
>  include/linux/soc/qcom/mdt_loader.h |  5 +++--
>  3 files changed, 35 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c 
> b/drivers/remoteproc/qcom_q6v5_mss.c
> index fddb63c..1919bfc 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> @@ -947,7 +947,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw,
>       int ret;
>       int i;
>
> -     metadata = qcom_mdt_read_metadata(fw, &size, fw_name, qproc->dev);
> +     metadata = qcom_mdt_read_metadata(fw, &size, fw_name, 
> + qproc->dev, NULL);

At the end of this function we invoke kfree(metadata), which would be bad if that comes from dma_alloc_coherent().

>       if (IS_ERR(metadata))
>               return PTR_ERR(metadata);
>
> diff --git a/drivers/soc/qcom/mdt_loader.c 
> b/drivers/soc/qcom/mdt_loader.c
[..]
> @@ -160,9 +164,18 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len,
>       ehdr_size = phdrs[0].p_filesz;
>       hash_size = phdrs[hash_segment].p_filesz;
>
> -     data = kmalloc(ehdr_size + hash_size, GFP_KERNEL);
> -     if (!data)
> -             return ERR_PTR(-ENOMEM);
> +     /*
> +      * During the scm call memory protection will be enabled for the meta
> +      * data blob, so make sure it's physically contiguous, 4K aligned and
> +      * non-cachable to avoid XPU violations.
> +      */
> +     scm_dev = qcom_get_scm_device();

As LKP points out, I don't seem to have this function.

> +     data = dma_alloc_coherent(scm_dev, ehdr_size + hash_size, mdata_phys,
> +                                    GFP_KERNEL);

I am not thrilled about the idea of doing dma_alloc_coherent() in this file and dma_free_coherent() in the scm driver. Similarly, I consider these functions to operate in the context of the caller, so operating on the scm device's struct device isn't so nice.


After trying various models I came to the conclusion that it was better to try to keep the MDT loader to just load MDT files, and move the SCM/PAS interaction out of that. Unfortunately we have a number of client drivers that would then need to (essentially) duplicate the content of qcom_mdt_pas_init() - so I left that in there.

I still believe that keeping the MDT loader focused on loading MDTs is a good idea, but I'm open to any suggestions for improvements in the interaction between these different components.

Regards,
Bjorn

[-- Attachment #2: v2-0001-firmware-qcom_scm-Remove-memory-alloc-call-from-q.patch --]
[-- Type: application/octet-stream, Size: 4946 bytes --]

From 012f865aa9951f8dabd1a17980dc4687adea94dc Mon Sep 17 00:00:00 2001
Message-Id: <012f865aa9951f8dabd1a17980dc4687adea94dc.1663777662.git.quic_gokukris@quicinc.com>
In-Reply-To: <cover.1663777662.git.quic_gokukris@quicinc.com>
References: <cover.1663777662.git.quic_gokukris@quicinc.com>
From: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com>
Date: Mon, 12 Sep 2022 11:41:31 -0700
Subject: [PATCH v2 1/2] firmware: qcom_scm: Remove memory alloc call from
 qcom_scm_pas_init_image()

The memory for metadata is allocated in mdt loader and scm driver

1].  163  data = kmalloc(ehdr_size + hash_size, GFP_KERNEL);
2].  477  mdata_buf = dma_alloc_coherent(__scm->dev, size, &mdata_phys,
     478                                        GFP_KERNEL);

The SCM driver is meant to only pack arguments provided to it from clients
for making the secure world calls. This change removes the logic of doing
a dma alloc coherent from the SCM driver and moves it into the mdt loader

Signed-off-by: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com>
---
 drivers/firmware/qcom_scm.c   | 33 +++------------------------------
 drivers/soc/qcom/mdt_loader.c |  3 ++-
 include/linux/qcom_scm.h      |  4 +---
 3 files changed, 6 insertions(+), 34 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index cdbfe54..05ec03c 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -442,11 +442,9 @@ static void qcom_scm_set_download_mode(bool enable)
  *			       state machine for a given peripheral, using the
  *			       metadata
  * @peripheral: peripheral id
- * @metadata:	pointer to memory containing ELF header, program header table
+ * @mdata_phys:	pointer to memory containing ELF header, program header table
  *		and optional blob of data used for authenticating the metadata
  *		and the rest of the firmware
- * @size:	size of the metadata
- * @ctx:	optional metadata context
  *
  * Return: 0 on success.
  *
@@ -454,11 +452,8 @@ static void qcom_scm_set_download_mode(bool enable)
  * track the metadata allocation, this needs to be released by invoking
  * qcom_scm_pas_metadata_release() by the caller.
  */
-int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
-			    struct qcom_scm_pas_metadata *ctx)
+int qcom_scm_pas_init_image(u32 peripheral, dma_addr_t mdata_phys)
 {
-	dma_addr_t mdata_phys;
-	void *mdata_buf;
 	int ret;
 	struct qcom_scm_desc desc = {
 		.svc = QCOM_SCM_SVC_PIL,
@@ -469,22 +464,9 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
 	};
 	struct qcom_scm_res res;
 
-	/*
-	 * During the scm call memory protection will be enabled for the meta
-	 * data blob, so make sure it's physically contiguous, 4K aligned and
-	 * non-cachable to avoid XPU violations.
-	 */
-	mdata_buf = dma_alloc_coherent(__scm->dev, size, &mdata_phys,
-				       GFP_KERNEL);
-	if (!mdata_buf) {
-		dev_err(__scm->dev, "Allocation of metadata buffer failed.\n");
-		return -ENOMEM;
-	}
-	memcpy(mdata_buf, metadata, size);
-
 	ret = qcom_scm_clk_enable();
 	if (ret)
-		goto out;
+		return ret;
 
 	ret = qcom_scm_bw_enable();
 	if (ret)
@@ -497,15 +479,6 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
 	qcom_scm_bw_disable();
 	qcom_scm_clk_disable();
 
-out:
-	if (ret < 0 || !ctx) {
-		dma_free_coherent(__scm->dev, size, mdata_buf, mdata_phys);
-	} else if (ctx) {
-		ctx->ptr = mdata_buf;
-		ctx->phys = mdata_phys;
-		ctx->size = size;
-	}
-
 	return ret ? : res.result[0];
 }
 EXPORT_SYMBOL(qcom_scm_pas_init_image);
diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
index 3f11554..8d06125 100644
--- a/drivers/soc/qcom/mdt_loader.c
+++ b/drivers/soc/qcom/mdt_loader.c
@@ -210,6 +210,7 @@ int qcom_mdt_pas_init(struct device *dev, const struct firmware *fw,
 	const struct elf32_hdr *ehdr;
 	phys_addr_t min_addr = PHYS_ADDR_MAX;
 	phys_addr_t max_addr = 0;
+	dma_addr_t mdata_phys;
 	size_t metadata_len;
 	void *metadata;
 	int ret;
@@ -238,7 +239,7 @@ int qcom_mdt_pas_init(struct device *dev, const struct firmware *fw,
 		goto out;
 	}
 
-	ret = qcom_scm_pas_init_image(pas_id, metadata, metadata_len, ctx);
+	ret = qcom_scm_pas_init_image(pas_id, mdata_phys);
 	kfree(metadata);
 	if (ret) {
 		/* Invalid firmware metadata */
diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
index f833564..751436a 100644
--- a/include/linux/qcom_scm.h
+++ b/include/linux/qcom_scm.h
@@ -74,9 +74,7 @@ struct qcom_scm_pas_metadata {
 	ssize_t size;
 };
 
-extern int qcom_scm_pas_init_image(u32 peripheral, const void *metadata,
-				   size_t size,
-				   struct qcom_scm_pas_metadata *ctx);
+extern int qcom_scm_pas_init_image(u32 peripheral, dma_addr_t metadata);
 void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx);
 extern int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr,
 				  phys_addr_t size);
-- 
2.7.4


[-- Attachment #3: v2-0002-soc-qcom-mdt_loader-Move-the-memory-allocation-in.patch --]
[-- Type: application/octet-stream, Size: 9147 bytes --]

From 87fb21a33713079bdedf5102f4f156eddb5770e5 Mon Sep 17 00:00:00 2001
Message-Id: <87fb21a33713079bdedf5102f4f156eddb5770e5.1663777662.git.quic_gokukris@quicinc.com>
In-Reply-To: <cover.1663777662.git.quic_gokukris@quicinc.com>
References: <cover.1663777662.git.quic_gokukris@quicinc.com>
From: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com>
Date: Mon, 12 Sep 2022 11:41:32 -0700
Subject: [PATCH v2 2/2] soc: qcom: mdt_loader: Move the memory allocation into
 mdt loader

By moving the memory allocation to mdt loader we can simplify the scm
call, by just packing arguments provided to it from the clients for
making secuer world calls. We can also simplify the memory allocation
for the qcom metadata, by just doing one memory allocation in the
mdt loader.

Signed-off-by: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com>
---
 drivers/firmware/qcom_scm.c         | 16 ----------
 drivers/remoteproc/qcom_q6v5_mss.c  |  2 +-
 drivers/remoteproc/qcom_q6v5_pas.c  |  4 +--
 drivers/soc/qcom/mdt_loader.c       | 60 ++++++++++++++++++++++++++++++++-----
 include/linux/qcom_scm.h            |  1 -
 include/linux/soc/qcom/mdt_loader.h | 13 ++++++--
 6 files changed, 66 insertions(+), 30 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 05ec03c..ea0eba65 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -483,22 +483,6 @@ int qcom_scm_pas_init_image(u32 peripheral, dma_addr_t mdata_phys)
 }
 EXPORT_SYMBOL(qcom_scm_pas_init_image);
 
-/**
- * qcom_scm_pas_metadata_release() - release metadata context
- * @ctx:	metadata context
- */
-void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx)
-{
-	if (!ctx->ptr)
-		return;
-
-	dma_free_coherent(__scm->dev, ctx->size, ctx->ptr, ctx->phys);
-
-	ctx->ptr = NULL;
-	ctx->phys = 0;
-	ctx->size = 0;
-}
-EXPORT_SYMBOL(qcom_scm_pas_metadata_release);
 
 /**
  * qcom_scm_pas_mem_setup() - Prepare the memory related to a given peripheral
diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
index fddb63c..1919bfc 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -947,7 +947,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw,
 	int ret;
 	int i;
 
-	metadata = qcom_mdt_read_metadata(fw, &size, fw_name, qproc->dev);
+	metadata = qcom_mdt_read_metadata(fw, &size, fw_name, qproc->dev, NULL);
 	if (IS_ERR(metadata))
 		return PTR_ERR(metadata);
 
diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
index 6afd094..f112ff4 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -159,7 +159,7 @@ static int adsp_unprepare(struct rproc *rproc)
 	 * auth_and_reset() was successful, but in other cases clean it up
 	 * here.
 	 */
-	qcom_scm_pas_metadata_release(&adsp->pas_metadata);
+	qcom_scm_pas_metadata_release(adsp->dev, &adsp->pas_metadata);
 
 	return 0;
 }
@@ -232,7 +232,7 @@ static int adsp_start(struct rproc *rproc)
 		goto disable_px_supply;
 	}
 
-	qcom_scm_pas_metadata_release(&adsp->pas_metadata);
+	qcom_scm_pas_metadata_release(adsp->dev, &adsp->pas_metadata);
 
 	return 0;
 
diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
index 8d06125..ad8725e 100644
--- a/drivers/soc/qcom/mdt_loader.c
+++ b/drivers/soc/qcom/mdt_loader.c
@@ -16,6 +16,7 @@
 #include <linux/sizes.h>
 #include <linux/slab.h>
 #include <linux/soc/qcom/mdt_loader.h>
+#include <linux/dma-mapping.h>
 
 static bool mdt_phdr_valid(const struct elf32_phdr *phdr)
 {
@@ -110,6 +111,7 @@ EXPORT_SYMBOL_GPL(qcom_mdt_get_size);
  * @data_len:	length of the read metadata blob
  * @fw_name:	name of the firmware, for construction of segment file names
  * @dev:	device handle to associate resources with
+ * @mdata_phys:	phys address for the assigned metadata buffer
  *
  * The mechanism that performs the authentication of the loading firmware
  * expects an ELF header directly followed by the segment of hashes, with no
@@ -124,7 +126,8 @@ EXPORT_SYMBOL_GPL(qcom_mdt_get_size);
  * Return: pointer to data, or ERR_PTR()
  */
 void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len,
-			     const char *fw_name, struct device *dev)
+			     const char *fw_name, struct device *dev,
+			     dma_addr_t *mdata_phys)
 {
 	const struct elf32_phdr *phdrs;
 	const struct elf32_hdr *ehdr;
@@ -160,10 +163,21 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len,
 	ehdr_size = phdrs[0].p_filesz;
 	hash_size = phdrs[hash_segment].p_filesz;
 
-	data = kmalloc(ehdr_size + hash_size, GFP_KERNEL);
-	if (!data)
+	/*
+	 * During the scm call memory protection will be enabled for the meta
+	 * data blob, so make sure it's physically contiguous, 4K aligned and
+	 * non-cachable to avoid XPU violations.
+	 */
+	if (mdata_phys) {
+		data = dma_alloc_coherent(dev, ehdr_size + hash_size, mdata_phys,
+				       GFP_KERNEL);
+	} else {
+		data = kmalloc(ehdr_size + hash_size, GFP_KERNEL);
+	}
+	if (!data) {
+		dev_err(dev, "Allocation of metadata buffer failed.\n");
 		return ERR_PTR(-ENOMEM);
-
+	}
 	/* Copy ELF header */
 	memcpy(data, fw->data, ehdr_size);
 
@@ -179,7 +193,11 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len,
 		/* Hash is in its own segment, beyond the loaded file */
 		ret = mdt_load_split_segment(data + ehdr_size, phdrs, hash_segment, fw_name, dev);
 		if (ret) {
-			kfree(data);
+			if (mdata_phys) {
+				dma_free_coherent(dev, ehdr_size + hash_size, data, *mdata_phys);
+			} else {
+				kfree(data);
+			}
 			return ERR_PTR(ret);
 		}
 	}
@@ -191,6 +209,23 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len,
 EXPORT_SYMBOL_GPL(qcom_mdt_read_metadata);
 
 /**
+ * qcom_pas_metadata_release() - release metadata context
+ * @ctx:	metadata context
+ */
+void qcom_scm_pas_metadata_release(struct device *dev, struct qcom_scm_pas_metadata *ctx)
+{
+	if (!ctx->ptr)
+		return;
+
+	dma_free_coherent(dev, ctx->size, ctx->ptr, ctx->phys);
+
+	ctx->ptr = NULL;
+	ctx->phys = 0;
+	ctx->size = 0;
+}
+EXPORT_SYMBOL(qcom_scm_pas_metadata_release);
+
+/**
  * qcom_mdt_pas_init() - initialize PAS region for firmware loading
  * @dev:	device handle to associate resources with
  * @fw:		firmware object for the mdt file
@@ -232,7 +267,7 @@ int qcom_mdt_pas_init(struct device *dev, const struct firmware *fw,
 			max_addr = ALIGN(phdr->p_paddr + phdr->p_memsz, SZ_4K);
 	}
 
-	metadata = qcom_mdt_read_metadata(fw, &metadata_len, fw_name, dev);
+	metadata = qcom_mdt_read_metadata(fw, &metadata_len, fw_name, dev, &mdata_phys);
 	if (IS_ERR(metadata)) {
 		ret = PTR_ERR(metadata);
 		dev_err(dev, "error %d reading firmware %s metadata\n", ret, fw_name);
@@ -240,7 +275,18 @@ int qcom_mdt_pas_init(struct device *dev, const struct firmware *fw,
 	}
 
 	ret = qcom_scm_pas_init_image(pas_id, mdata_phys);
-	kfree(metadata);
+	if (ret || !ctx) {
+		if (mdata_phys) {
+			dma_free_coherent(dev, metadata_len, metadata, mdata_phys);
+		} else {
+			kfree(metadata);
+		}
+	} else if (ctx) {
+		ctx->ptr = metadata;
+		ctx->phys = mdata_phys;
+		ctx->size = metadata_len;
+	}
+
 	if (ret) {
 		/* Invalid firmware metadata */
 		dev_err(dev, "error %d initializing firmware %s\n", ret, fw_name);
diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
index 751436a..0053936 100644
--- a/include/linux/qcom_scm.h
+++ b/include/linux/qcom_scm.h
@@ -75,7 +75,6 @@ struct qcom_scm_pas_metadata {
 };
 
 extern int qcom_scm_pas_init_image(u32 peripheral, dma_addr_t metadata);
-void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx);
 extern int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr,
 				  phys_addr_t size);
 extern int qcom_scm_pas_auth_and_reset(u32 peripheral);
diff --git a/include/linux/soc/qcom/mdt_loader.h b/include/linux/soc/qcom/mdt_loader.h
index 9e8e604..856b6f9 100644
--- a/include/linux/soc/qcom/mdt_loader.h
+++ b/include/linux/soc/qcom/mdt_loader.h
@@ -28,8 +28,11 @@ int qcom_mdt_load_no_init(struct device *dev, const struct firmware *fw,
 			  phys_addr_t mem_phys, size_t mem_size,
 			  phys_addr_t *reloc_base);
 void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len,
-			     const char *fw_name, struct device *dev);
+			     const char *fw_name, struct device *dev,
+			     dma_addr_t *mdata_phys);
 
+void qcom_scm_pas_metadata_release(struct device *dev,
+				   struct qcom_scm_pas_metadata *ctx);
 #else /* !IS_ENABLED(CONFIG_QCOM_MDT_LOADER) */
 
 static inline ssize_t qcom_mdt_get_size(const struct firmware *fw)
@@ -64,11 +67,15 @@ static inline int qcom_mdt_load_no_init(struct device *dev,
 
 static inline void *qcom_mdt_read_metadata(const struct firmware *fw,
 					   size_t *data_len, const char *fw_name,
-					   struct device *dev)
+					   struct device *dev, dma_addr_t *mdata_phys)
 {
 	return ERR_PTR(-ENODEV);
 }
-
+void qcom_scm_pas_metadata_release(struct device *dev,
+				   struct qcom_scm_pas_metadata *ctx)
+{
+	return NULL;
+}
 #endif /* !IS_ENABLED(CONFIG_QCOM_MDT_LOADER) */
 
 #endif
-- 
2.7.4

Re: [PATCH v1 2/2] soc: qcom: mdt_loader: Move the memory allocation into mdt loader

[Gokul Krishna Krishnakumar]

Hi Bjorn,
With this patch we have moved the dma_alloc_coherent/dma_free_coherent 
is called from the mdt loader and is operating in the context of the 
caller, the scm device's struct device is not used in this patch. For 
the clients which do not pass the metadata physical argument to the 
qcom_mdt_read_metadata() - the memory is allocated using kmalloc- so the 
clients like qcom_q6v5_mss.c, where kfree is called will not be broken 
with this change.
Thanks,
Gokul

On 9/21/2022 12:39 PM, Gokul krishna Krishnakumar (QUIC) wrote:
>> At the end of this function we invoke kfree(metadata), which would be bad if that comes from dma_alloc_coherent().
> +       if (mdata_phys) {
> +               data = dma_alloc_coherent(dev, ehdr_size + hash_size, mdata_phys,
> +                                      GFP_KERNEL);
> +       } else {
> +               data = kmalloc(ehdr_size + hash_size, GFP_KERNEL);
> Adding dma_alloc_coherent without affecting the mss driver.
> 
> 
>> As LKP points out, I don't seem to have this function.
> Removing the qcom_get_scm_device() and calling dma_alloc_coherent from device context.
> +               data = dma_alloc_coherent(dev, ehdr_size + hash_size, mdata_phys,
> +                                      GFP_KERNEL);
> 
>> I am not thrilled about the idea of doing dma_alloc_coherent() in this file and dma_free_coherent() in the scm driver. Similarly, I consider these functions to operate in the context of the caller, so operating on the scm device's struct device isn't so nice.
>> After trying various models I came to the conclusion that it was better to try to keep the MDT loader to just load MDT files, and move the SCM/PAS interaction out of that. Unfortunately we have a number of client drivers that would then need to (essentially) duplicate the content of qcom_mdt_pas_init() - so I left >that in there.
>> I still believe that keeping the MDT loader focused on loading MDTs is a good idea, but I'm open to any suggestions for improvements in the interaction between these different components.
> 
> With this patch we moving all the dma_alloc_coherent() and dma_free_coherent() to the MDT loader.
> So now the MDT loader has the functionality of loading and allocating memory
> and the SCM driver packs the arguments and makes a call to the secure world.
> 
> -----Original Message-----
> From: Bjorn Andersson <andersson@kernel.org>
> Sent: Tuesday, September 13, 2022 4:11 PM
> To: Gokul krishna Krishnakumar (QUIC) <quic_gokukris@quicinc.com>
> Cc: Andy Gross <agross@kernel.org>; Konrad Dybcio <konrad.dybcio@somainline.org>; Philipp Zabel <p.zabel@pengutronix.de>; linux-arm-msm@vger.kernel.org; linux-kernel@vger.kernel.org; Trilok Soni (QUIC) <quic_tsoni@quicinc.com>; Satya Durga Srinivasu Prabhala (QUIC) <quic_satyap@quicinc.com>; Rajendra Nayak (QUIC) <quic_rjendra@quicinc.com>; Elliot Berman (QUIC) <quic_eberman@quicinc.com>; Guru Das Srinagesh (QUIC) <quic_gurus@quicinc.com>
> Subject: Re: [PATCH v1 2/2] soc: qcom: mdt_loader: Move the memory allocation into mdt loader
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.
> 
> On Mon, Sep 12, 2022 at 11:41:32AM -0700, Gokul krishna Krishnakumar wrote:
>> By moving the memory allocation to mdt loader we can simplify the scm
>> call, by just packing arguments provided to it from the clients for
>> making secuer world calls. We can also simplify the memory allocation
>> for the qcom metadata, by just doing one memory allocation in the mdt
>> loader.
>>
>> Signed-off-by: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com>
>> ---
>>   drivers/remoteproc/qcom_q6v5_mss.c  |  2 +-
>>   drivers/soc/qcom/mdt_loader.c       | 41 ++++++++++++++++++++++++++++---------
>>   include/linux/soc/qcom/mdt_loader.h |  5 +++--
>>   3 files changed, 35 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c
>> b/drivers/remoteproc/qcom_q6v5_mss.c
>> index fddb63c..1919bfc 100644
>> --- a/drivers/remoteproc/qcom_q6v5_mss.c
>> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
>> @@ -947,7 +947,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw,
>>        int ret;
>>        int i;
>>
>> -     metadata = qcom_mdt_read_metadata(fw, &size, fw_name, qproc->dev);
>> +     metadata = qcom_mdt_read_metadata(fw, &size, fw_name,
>> + qproc->dev, NULL);
> 
> At the end of this function we invoke kfree(metadata), which would be bad if that comes from dma_alloc_coherent().
> 
>>        if (IS_ERR(metadata))
>>                return PTR_ERR(metadata);
>>
>> diff --git a/drivers/soc/qcom/mdt_loader.c
>> b/drivers/soc/qcom/mdt_loader.c
> [..]
>> @@ -160,9 +164,18 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len,
>>        ehdr_size = phdrs[0].p_filesz;
>>        hash_size = phdrs[hash_segment].p_filesz;
>>
>> -     data = kmalloc(ehdr_size + hash_size, GFP_KERNEL);
>> -     if (!data)
>> -             return ERR_PTR(-ENOMEM);
>> +     /*
>> +      * During the scm call memory protection will be enabled for the meta
>> +      * data blob, so make sure it's physically contiguous, 4K aligned and
>> +      * non-cachable to avoid XPU violations.
>> +      */
>> +     scm_dev = qcom_get_scm_device();
> 
> As LKP points out, I don't seem to have this function.
> 
>> +     data = dma_alloc_coherent(scm_dev, ehdr_size + hash_size, mdata_phys,
>> +                                    GFP_KERNEL);
> 
> I am not thrilled about the idea of doing dma_alloc_coherent() in this file and dma_free_coherent() in the scm driver. Similarly, I consider these functions to operate in the context of the caller, so operating on the scm device's struct device isn't so nice.
> 
> 
> After trying various models I came to the conclusion that it was better to try to keep the MDT loader to just load MDT files, and move the SCM/PAS interaction out of that. Unfortunately we have a number of client drivers that would then need to (essentially) duplicate the content of qcom_mdt_pas_init() - so I left that in there.
> 
> I still believe that keeping the MDT loader focused on loading MDTs is a good idea, but I'm open to any suggestions for improvements in the interaction between these different components.
> 
> Regards,
> Bjorn