[RFCv2 00/10] remoteproc: Support bi-directional vdev config space

[RFCv2 00/10] remoteproc: Support bi-directional vdev config space

[Sjur Brændeland]

Changes since v1:
- Separate vring allocation and registration
- Break up patches in smaller parts to facilitate review.
  (The patches here are perhaps too fine grained and can be squashed later).

This patch-set adds support for shared resource table between
Linux kernel and remote devices. 
Features:
- dynamically-allocated address of the vrings can be communicated
- vdev statuses can be communicated
- virtio config space becomes bi-directional
- virtio feature negotiation is two-way 

NOTE: This change may not be backwards compatible with existing
device firmware! The memory allocation order may change. Previously
the Virtio Rings were always allocated at start of the share memory
area, but with this patch-set the memory allocation are done in the
order defined resource table.

A number of changes are introduced to achieve this:
- The firmware is loaded once.
- The allocations of resources is done before registering 
  the virtio devices.
- The virtio device uses resource bits, status and config
  space in memory shared with the device.

Review comments and feedback are welcomed!

Thanks,
Sjur

Sjur Brændeland (11):
  remoteproc: Code cleanup of resource parsing
  remoteproc: Move check on firmware name to rproc_add
  remoteproc: Set vring addresses in resource table
  remoteproc: Add state RPROC_LOADED
  remoteproc: Load firmware once.
  remoteproc: Add resource entry number to callbacks
  remoteproc: Register virtio devices after vring allocation
  remoteproc: Refactor function rproc_elf_find_rsc_table
  remoteproc: Add operation to find resource table in memory
  remoteproc: Find resource table in device memory
  remoteproc: Support virtio config space.

 drivers/remoteproc/remoteproc_core.c       |  216 ++++++++++++---------------
 drivers/remoteproc/remoteproc_debugfs.c    |    1 +
 drivers/remoteproc/remoteproc_elf_loader.c |   99 +++++++++-----
 drivers/remoteproc/remoteproc_internal.h   |   13 ++
 drivers/remoteproc/remoteproc_virtio.c     |   30 ++++-
 drivers/remoteproc/ste_modem_rproc.c       |   43 ++++--
 include/linux/remoteproc.h                 |    9 +-
 7 files changed, 238 insertions(+), 173 deletions(-)

-- 
1.7.5.4

[RFCv2 01/11] remoteproc: Code cleanup of resource parsing

[Sjur Brændeland]

Combine the almost identical functions rproc_handle_virtio_rsc
and rproc_handle_boot_rsc.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/remoteproc/remoteproc_core.c |   51 ++++++++--------------------------
 1 files changed, 12 insertions(+), 39 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index dd3bfaf..feb39a7 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -677,16 +677,22 @@ free_carv:
  * A lookup table for resource handlers. The indices are defined in
  * enum fw_resource_type.
  */
-static rproc_handle_resource_t rproc_handle_rsc[] = {
+static rproc_handle_resource_t rproc_handle_rsc[RSC_LAST] = {
 	[RSC_CARVEOUT] = (rproc_handle_resource_t)rproc_handle_carveout,
 	[RSC_DEVMEM] = (rproc_handle_resource_t)rproc_handle_devmem,
 	[RSC_TRACE] = (rproc_handle_resource_t)rproc_handle_trace,
 	[RSC_VDEV] = NULL, /* VDEVs were handled upon registrarion */
 };
 
+static rproc_handle_resource_t rproc_handle_vdev_rsc[RSC_LAST] = {
+	[RSC_VDEV] = (rproc_handle_resource_t)rproc_handle_vdev,
+};
+
 /* handle firmware resource entries before booting the remote processor */
 static int
-rproc_handle_boot_rsc(struct rproc *rproc, struct resource_table *table, int len)
+rproc_handle_resource(struct rproc *rproc, struct resource_table *table,
+				int len,
+				rproc_handle_resource_t handlers[RSC_LAST])
 {
 	struct device *dev = &rproc->dev;
 	rproc_handle_resource_t handler;
@@ -711,7 +717,7 @@ rproc_handle_boot_rsc(struct rproc *rproc, struct resource_table *table, int len
 			continue;
 		}
 
-		handler = rproc_handle_rsc[hdr->type];
+		handler = handlers[hdr->type];
 		if (!handler)
 			continue;
 
@@ -723,40 +729,6 @@ rproc_handle_boot_rsc(struct rproc *rproc, struct resource_table *table, int len
 	return ret;
 }
 
-/* handle firmware resource entries while registering the remote processor */
-static int
-rproc_handle_virtio_rsc(struct rproc *rproc, struct resource_table *table, int len)
-{
-	struct device *dev = &rproc->dev;
-	int ret = 0, i;
-
-	for (i = 0; i < table->num; i++) {
-		int offset = table->offset[i];
-		struct fw_rsc_hdr *hdr = (void *)table + offset;
-		int avail = len - offset - sizeof(*hdr);
-		struct fw_rsc_vdev *vrsc;
-
-		/* make sure table isn't truncated */
-		if (avail < 0) {
-			dev_err(dev, "rsc table is truncated\n");
-			return -EINVAL;
-		}
-
-		dev_dbg(dev, "%s: rsc type %d\n", __func__, hdr->type);
-
-		if (hdr->type != RSC_VDEV)
-			continue;
-
-		vrsc = (struct fw_rsc_vdev *)hdr->data;
-
-		ret = rproc_handle_vdev(rproc, vrsc, avail);
-		if (ret)
-			break;
-	}
-
-	return ret;
-}
-
 /**
  * rproc_resource_cleanup() - clean up and free all acquired resources
  * @rproc: rproc handle
@@ -836,7 +808,7 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	}
 
 	/* handle fw resources which are required to boot rproc */
-	ret = rproc_handle_boot_rsc(rproc, table, tablesz);
+	ret = rproc_handle_resource(rproc, table, tablesz, rproc_handle_rsc);
 	if (ret) {
 		dev_err(dev, "Failed to process resources: %d\n", ret);
 		goto clean_up;
@@ -891,7 +863,8 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
 		goto out;
 
 	/* look for virtio devices and register them */
-	ret = rproc_handle_virtio_rsc(rproc, table, tablesz);
+	ret = rproc_handle_resource(rproc, table, tablesz,
+						rproc_handle_vdev_rsc);
 	if (ret)
 		goto out;
 
-- 
1.7.5.4

[RFCv2 02/11] remoteproc: Move check on firmware name to rproc_add

[Sjur Brændeland]

Verify that firmware name is defined in rproc_add.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/remoteproc/remoteproc_core.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index feb39a7..7d593a1 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -989,13 +989,6 @@ int rproc_boot(struct rproc *rproc)
 		return ret;
 	}
 
-	/* loading a firmware is required */
-	if (!rproc->firmware) {
-		dev_err(dev, "%s: no firmware to load\n", __func__);
-		ret = -EINVAL;
-		goto unlock_mutex;
-	}
-
 	/* prevent underlying implementation from being removed */
 	if (!try_module_get(dev->parent->driver->owner)) {
 		dev_err(dev, "%s: can't get owner\n", __func__);
@@ -1120,6 +1113,12 @@ int rproc_add(struct rproc *rproc)
 	struct device *dev = &rproc->dev;
 	int ret;
 
+	/* loading a firmware is required */
+	if (!rproc->firmware) {
+		dev_err(dev, "%s: no firmware to load\n", __func__);
+		return -EINVAL;
+	}
+
 	ret = device_add(dev);
 	if (ret < 0)
 		return ret;
-- 
1.7.5.4

[RFCv2 03/11] remoteproc: Set vring addresses in resource table

[Sjur Brændeland]

Set the vring addresses in the resource table so that
the remote device can read the actual addresses used.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/remoteproc/remoteproc_core.c |    5 +++++
 include/linux/remoteproc.h           |    2 ++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 7d593a1..151e138 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -238,6 +238,8 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 	rvring->dma = dma;
 	rvring->notifyid = notifyid;
 
+	rvdev->rsc->vring[i].da = dma;
+	rvdev->rsc->vring[i].notifyid = notifyid;
 	return 0;
 }
 
@@ -365,6 +367,9 @@ static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
 	/* remember the device features */
 	rvdev->dfeatures = rsc->dfeatures;
 
+	/* remember the resource entry */
+	rvdev->rsc = rsc;
+
 	list_add_tail(&rvdev->node, &rproc->rvdevs);
 
 	/* it is now safe to add the virtio device */
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index faf3332..cdacd66 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -464,6 +464,7 @@ struct rproc_vring {
  * @vring: the vrings for this vdev
  * @dfeatures: virtio device features
  * @gfeatures: virtio guest features
+ * @rsc: vdev resource entry
  */
 struct rproc_vdev {
 	struct list_head node;
@@ -472,6 +473,7 @@ struct rproc_vdev {
 	struct rproc_vring vring[RVDEV_NUM_VRINGS];
 	unsigned long dfeatures;
 	unsigned long gfeatures;
+	struct fw_rsc_vdev *rsc;
 };
 
 struct rproc *rproc_alloc(struct device *dev, const char *name,
-- 
1.7.5.4

Re: [RFCv2 03/11] remoteproc: Set vring addresses in resource table

[Ido Yariv]

Hi Sjur,

On Fri, Dec 14, 2012 at 05:06:52PM +0100, Sjur Brændeland wrote:
> Set the vring addresses in the resource table so that
> the remote device can read the actual addresses used.
> 
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>

Since this new member will only be referenced in the last patch, perhaps
squash the two patches?

> ---
>  drivers/remoteproc/remoteproc_core.c |    5 +++++
>  include/linux/remoteproc.h           |    2 ++
>  2 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 7d593a1..151e138 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -238,6 +238,8 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
>  	rvring->dma = dma;
>  	rvring->notifyid = notifyid;
>  
> +	rvdev->rsc->vring[i].da = dma;

This could be a bit problematic when iommu is used. Since not all
platforms use dma_alloc_coherent to automatically set up the iommu, the
dma variable might hold the physical address instead of the device
address. Setting da may be a bit misleading, so this should probably be
documented.

Thanks,
Ido.

RE: [RFCv2 03/11] remoteproc: Set vring addresses in resource table

[Sjur BRENDELAND]

Hi Ido,
> > Set the vring addresses in the resource table so that
> > the remote device can read the actual addresses used.
> >
> > Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
> 
> Since this new member will only be referenced in the last patch, perhaps
> squash the two patches?

This patch fixes two TODOs in the remoteproc_core.c so I think
it deserves to be a separate patch.

Regards,
Sjur

[RFCv2 04/11] remoteproc: Add state RPROC_LOADED

[Sjur Brændeland]

Add state RPROC_LOADED as firmware loading and startup will be
different states.

Signed-off-by: Sjur Brændeland <sjur.brandeland@steicsson.com>
---
 drivers/remoteproc/remoteproc_debugfs.c |    1 +
 include/linux/remoteproc.h              |    3 ++-
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
index 157a573..a026359 100644
--- a/drivers/remoteproc/remoteproc_debugfs.c
+++ b/drivers/remoteproc/remoteproc_debugfs.c
@@ -68,6 +68,7 @@ static const char * const rproc_state_string[] = {
 	"suspended",
 	"running",
 	"crashed",
+	"loaded",
 	"invalid",
 };
 
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index cdacd66..932edc7 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -357,7 +357,8 @@ enum rproc_state {
 	RPROC_SUSPENDED	= 1,
 	RPROC_RUNNING	= 2,
 	RPROC_CRASHED	= 3,
-	RPROC_LAST	= 4,
+	RPROC_LOADED    = 4,
+	RPROC_LAST	= 5,
 };
 
 /**
-- 
1.7.5.4

Re: [RFCv2 04/11] remoteproc: Add state RPROC_LOADED

[Ido Yariv]

Hi Sjur,

On Fri, Dec 14, 2012 at 05:06:53PM +0100, Sjur Brændeland wrote:
> Add state RPROC_LOADED as firmware loading and startup will be
> different states.
> 
> Signed-off-by: Sjur Brændeland <sjur.brandeland@steicsson.com>

The rproc_state holds the state of the remote processor and it will
still be offline even when the FW is loaded, so I'm not sure if a new
state should be added.

Thanks,
Ido.

RE: [RFCv2 04/11] remoteproc: Add state RPROC_LOADED

[Sjur BRENDELAND]

> > Add state RPROC_LOADED as firmware loading and startup will be
> > different states.
> >
> > Signed-off-by: Sjur Brændeland <sjur.brandeland@steicsson.com>
> 
> The rproc_state holds the state of the remote processor and it will
> still be offline even when the FW is loaded, so I'm not sure if a new
> state should be added.

In the new set of patches I am using this state in order to know if I need
to reload the firmware before starting the device.

So I need this state in order to differentiate between a boot/reboot and
rproc_shutdown, rproc_boot sequence. In the latter case I need to reload
the firmware in order to initialize static variables and resource table in
device memory.

Regards,
Sjur

[RFCv2 05/11] remoteproc: Load firmware once.

[Sjur Brændeland]

Load the firmware once and pave the way for two way
communication of virtio configuration and status bits.
The virtio ring device address is now stored in the resource
table.

NOTE:  This patch requires device firmware change! The device
	memory layout changes. The virtio rings are no longer
	located at start of device memory. But the vring address
	can be read from the resource table. Also Carveout
	must be located before the virtio rings in the resource
	table.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/remoteproc/remoteproc_core.c |  111 ++++++++++++----------------------
 1 files changed, 38 insertions(+), 73 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 151e138..ea4b76a 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -686,14 +686,10 @@ static rproc_handle_resource_t rproc_handle_rsc[RSC_LAST] = {
 	[RSC_CARVEOUT] = (rproc_handle_resource_t)rproc_handle_carveout,
 	[RSC_DEVMEM] = (rproc_handle_resource_t)rproc_handle_devmem,
 	[RSC_TRACE] = (rproc_handle_resource_t)rproc_handle_trace,
-	[RSC_VDEV] = NULL, /* VDEVs were handled upon registrarion */
-};
-
-static rproc_handle_resource_t rproc_handle_vdev_rsc[RSC_LAST] = {
 	[RSC_VDEV] = (rproc_handle_resource_t)rproc_handle_vdev,
 };
 
-/* handle firmware resource entries before booting the remote processor */
+/* handle firmware resource entries */
 static int
 rproc_handle_resource(struct rproc *rproc, struct resource_table *table,
 				int len,
@@ -778,18 +774,27 @@ static void rproc_resource_cleanup(struct rproc *rproc)
 }
 
 /*
- * take a firmware and boot a remote processor with it.
+ * take a firmware, parse the resouce table and load it
+ *
+ * Note: this function is called asynchronously upon registration of the
+ * remote processor (so we must wait until it completes before we try
+ * to unregister the device. one other option is just to use kref here,
+ * that might be cleaner).
  */
-static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
+static void rproc_fw_load(const struct firmware *fw, void *context)
 {
+	struct rproc *rproc = context;
 	struct device *dev = &rproc->dev;
 	const char *name = rproc->firmware;
 	struct resource_table *table;
 	int ret, tablesz;
 
+	if (!fw)
+		goto release_fw;
+
 	ret = rproc_fw_sanity_check(rproc, fw);
 	if (ret)
-		return ret;
+		goto release_fw;
 
 	dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size);
 
@@ -800,7 +805,7 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	ret = rproc_enable_iommu(rproc);
 	if (ret) {
 		dev_err(dev, "can't enable iommu: %d\n", ret);
-		return ret;
+		goto release_fw;
 	}
 
 	rproc->bootaddr = rproc_get_boot_addr(rproc, fw);
@@ -826,60 +831,23 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 		goto clean_up;
 	}
 
-	/* power up the remote processor */
-	ret = rproc->ops->start(rproc);
-	if (ret) {
-		dev_err(dev, "can't start rproc %s: %d\n", rproc->name, ret);
-		goto clean_up;
-	}
-
-	rproc->state = RPROC_RUNNING;
-
-	dev_info(dev, "remote processor %s is now up\n", rproc->name);
+	rproc->state = RPROC_LOADED;
 
-	return 0;
+	dev_info(dev, "remote processor %s is loaded to memory\n", rproc->name);
 
 clean_up:
-	rproc_resource_cleanup(rproc);
-	rproc_disable_iommu(rproc);
-	return ret;
-}
-
-/*
- * take a firmware and look for virtio devices to register.
- *
- * Note: this function is called asynchronously upon registration of the
- * remote processor (so we must wait until it completes before we try
- * to unregister the device. one other option is just to use kref here,
- * that might be cleaner).
- */
-static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
-{
-	struct rproc *rproc = context;
-	struct resource_table *table;
-	int ret, tablesz;
-
-	if (rproc_fw_sanity_check(rproc, fw) < 0)
-		goto out;
-
-	/* look for the resource table */
-	table = rproc_find_rsc_table(rproc, fw,  &tablesz);
-	if (!table)
-		goto out;
-
-	/* look for virtio devices and register them */
-	ret = rproc_handle_resource(rproc, table, tablesz,
-						rproc_handle_vdev_rsc);
-	if (ret)
-		goto out;
-
-out:
+	if (ret) {
+		rproc->state = RPROC_OFFLINE;
+		rproc_resource_cleanup(rproc);
+		rproc_disable_iommu(rproc);
+	}
+release_fw:
 	release_firmware(fw);
 	/* allow rproc_del() contexts, if any, to proceed */
 	complete_all(&rproc->firmware_loading_complete);
 }
 
-static int rproc_add_virtio_devices(struct rproc *rproc)
+static int rproc_load_firmware(struct rproc *rproc)
 {
 	int ret;
 
@@ -887,16 +855,12 @@ static int rproc_add_virtio_devices(struct rproc *rproc)
 	init_completion(&rproc->firmware_loading_complete);
 
 	/*
-	 * We must retrieve early virtio configuration info from
-	 * the firmware (e.g. whether to register a virtio device,
-	 * what virtio features does it support, ...).
-	 *
 	 * We're initiating an asynchronous firmware loading, so we can
 	 * be built-in kernel code, without hanging the boot process.
 	 */
 	ret = request_firmware_nowait(THIS_MODULE, FW_ACTION_HOTPLUG,
 				      rproc->firmware, &rproc->dev, GFP_KERNEL,
-				      rproc, rproc_fw_config_virtio);
+				      rproc, rproc_fw_load);
 	if (ret < 0) {
 		dev_err(&rproc->dev, "request_firmware_nowait err: %d\n", ret);
 		complete_all(&rproc->firmware_loading_complete);
@@ -930,7 +894,7 @@ int rproc_trigger_recovery(struct rproc *rproc)
 	/* wait until there is no more rproc users */
 	wait_for_completion(&rproc->crash_comp);
 
-	return rproc_add_virtio_devices(rproc);
+	return rproc_load_firmware(rproc);
 }
 
 /**
@@ -977,7 +941,6 @@ static void rproc_crash_handler_work(struct work_struct *work)
  */
 int rproc_boot(struct rproc *rproc)
 {
-	const struct firmware *firmware_p;
 	struct device *dev;
 	int ret;
 
@@ -1004,27 +967,28 @@ int rproc_boot(struct rproc *rproc)
 	/* skip the boot process if rproc is already powered up */
 	if (atomic_inc_return(&rproc->power) > 1) {
 		ret = 0;
-		goto unlock_mutex;
+		goto downref_driver;
 	}
 
 	dev_info(dev, "powering up %s\n", rproc->name);
 
-	/* load firmware */
-	ret = request_firmware(&firmware_p, rproc->firmware, dev);
-	if (ret < 0) {
-		dev_err(dev, "request_firmware failed: %d\n", ret);
+	/* power up the remote processor */
+	ret = rproc->ops->start(rproc);
+	if (ret) {
+		dev_err(dev, "can't start rproc %s: %d\n", rproc->name, ret);
+		rproc->state = RPROC_OFFLINE;
 		goto downref_rproc;
 	}
 
-	ret = rproc_fw_boot(rproc, firmware_p);
-
-	release_firmware(firmware_p);
+	rproc->state = RPROC_RUNNING;
+	dev_info(dev, "powering up %s\n", rproc->name);
 
 downref_rproc:
-	if (ret) {
+	if (ret)
 		module_put(dev->parent->driver->owner);
+downref_driver:
+	if (ret)
 		atomic_dec(&rproc->power);
-	}
 unlock_mutex:
 	mutex_unlock(&rproc->lock);
 	return ret;
@@ -1136,7 +1100,8 @@ int rproc_add(struct rproc *rproc)
 	/* create debugfs entries */
 	rproc_create_debug_dir(rproc);
 
-	return rproc_add_virtio_devices(rproc);
+	return rproc_load_firmware(rproc);
+
 }
 EXPORT_SYMBOL(rproc_add);
 
-- 
1.7.5.4

Re: [RFCv2 05/11] remoteproc: Load firmware once.

[Ido Yariv]

Hi Sjur,

On Fri, Dec 14, 2012 at 05:06:54PM +0100, Sjur Brændeland wrote:
> Load the firmware once and pave the way for two way
> communication of virtio configuration and status bits.
> The virtio ring device address is now stored in the resource
> table.
> 
> NOTE:  This patch requires device firmware change! The device
> 	memory layout changes. The virtio rings are no longer
> 	located at start of device memory. But the vring address
> 	can be read from the resource table. Also Carveout
> 	must be located before the virtio rings in the resource
> 	table.
> 
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>

In case the remote processor is added, booted and then rebooted, the
program sections wont be reinitialized. This can be a bit problematic,
since the firmware might assume values are initialized in its data
sections.

How about the following alternative approach - instead of loading the
firmware in advance, we could allocate just a small (private) buffer,
holding a copy of the resource table. Our copy will be updated with the
addresses of the vrings we allocate, along with the notification ids.
Each time the remote processor is booted, we could reload the firmware
and overwrite the resource table section with our own private copy.
virtio's configuration space and status will probably need to be updated
in both copies of the resource table, so it would be consistent across
boots.

...

> -static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
> +static void rproc_fw_load(const struct firmware *fw, void *context)
>  {
> +	struct rproc *rproc = context;
>  	struct device *dev = &rproc->dev;
>  	const char *name = rproc->firmware;
>  	struct resource_table *table;
>  	int ret, tablesz;
>  
> +	if (!fw)
> +		goto release_fw;
> +
>  	ret = rproc_fw_sanity_check(rproc, fw);
>  	if (ret)
> -		return ret;
> +		goto release_fw;
>  
>  	dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size);
>  
> @@ -800,7 +805,7 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>  	ret = rproc_enable_iommu(rproc);

iommu is enabled when the firmware is loaded, but disabled on shutdown,
so it could get out of balance when the remote processor is rebooted.

...

>  int rproc_boot(struct rproc *rproc)
>  {
> -	const struct firmware *firmware_p;
>  	struct device *dev;
>  	int ret;
>  
> @@ -1004,27 +967,28 @@ int rproc_boot(struct rproc *rproc)
>  	/* skip the boot process if rproc is already powered up */
>  	if (atomic_inc_return(&rproc->power) > 1) {
>  		ret = 0;
> -		goto unlock_mutex;
> +		goto downref_driver;

In case the rproc is already powered up, module_put should probably be
called to balance try_module_get. This is actually an existing bug in the
code.

Thanks,
Ido.

RE: [RFCv2 05/11] remoteproc: Load firmware once.

[Sjur BRENDELAND]

Hi Ido and Ohad,

> In case the remote processor is added, booted and then rebooted, the
> program sections wont be reinitialized. This can be a bit problematic,
> since the firmware might assume values are initialized in its data
> sections.
> 
> How about the following alternative approach - instead of loading the
> firmware in advance, we could allocate just a small (private) buffer,
> holding a copy of the resource table. Our copy will be updated with the
> addresses of the vrings we allocate, along with the notification ids.
> Each time the remote processor is booted, we could reload the firmware
> and overwrite the resource table section with our own private copy.
> virtio's configuration space and status will probably need to be updated
> in both copies of the resource table, so it would be consistent across
> boots.

Thank you for review comments Ido!

Hm, are you suggesting to load firmware once or twice?

We could keep loading firmware twice and copy
the resource table. This solves the issue of initializing variables.
We could then keep the original approach with loading firmware
twice. But one question comes to mind:  is it safe to assume that
the firmware's resource table is unchanged after a reboot?

[Ohad Aug 10, 2012 wrote.]
>The general direction I have in mind is to put the resource table in
>its final location while we do the first pass of fw parsing.
...
>This will solve all sort of open issues we have (or going to have soon):
...
>1. dynamically-allocated address of the vrings can be communicated
>2. vdev statuses can be communicated
>3. virtio config space will finally become bi-directional as it should
>4. dynamically probed rproc-to-rproc IPC could then take place
>
>It's the real deal :)
>
>The only problem with this approach is that the resource table isn't
>reloaded throughout cycles of power up/down, and that is insecure.
>We'll have to manually reload it somewhere after the rproc is powered
>down (or before it is powered up again).
>
>This change will break existing firmwares, but it looks required and inevitable.

This patch is a stab on implementing what Ohad suggested originally, and loads firmware
once. We can probably make this work as well, but I missed handling reload properly.
For this to work I need to add support for reloading firmware upon reboot.

I go on vacation for a while now, but please get back to me on what direction we
should go in: loading once or twice :-)

Regards,
Sjur

Re: [RFCv2 05/11] remoteproc: Load firmware once.

[Ido Yariv]

Hi Sjur,

On Fri, Dec 21, 2012 at 01:02:02PM +0100, Sjur BRENDELAND wrote:
> Hi Ido and Ohad,
> 
> > In case the remote processor is added, booted and then rebooted, the
> > program sections wont be reinitialized. This can be a bit problematic,
> > since the firmware might assume values are initialized in its data
> > sections.
> > 
> > How about the following alternative approach - instead of loading the
> > firmware in advance, we could allocate just a small (private) buffer,
> > holding a copy of the resource table. Our copy will be updated with the
> > addresses of the vrings we allocate, along with the notification ids.
> > Each time the remote processor is booted, we could reload the firmware
> > and overwrite the resource table section with our own private copy.
> > virtio's configuration space and status will probably need to be updated
> > in both copies of the resource table, so it would be consistent across
> > boots.
> 
> Thank you for review comments Ido!
> 
> Hm, are you suggesting to load firmware once or twice?
> 
> We could keep loading firmware twice and copy
> the resource table. This solves the issue of initializing variables.
> We could then keep the original approach with loading firmware
> twice. But one question comes to mind:  is it safe to assume that
> the firmware's resource table is unchanged after a reboot?
> 
> [Ohad Aug 10, 2012 wrote.]
> >The general direction I have in mind is to put the resource table in
> >its final location while we do the first pass of fw parsing.
> ...
> >This will solve all sort of open issues we have (or going to have soon):
> ...
> >1. dynamically-allocated address of the vrings can be communicated
> >2. vdev statuses can be communicated
> >3. virtio config space will finally become bi-directional as it should
> >4. dynamically probed rproc-to-rproc IPC could then take place
> >
> >It's the real deal :)
> >
> >The only problem with this approach is that the resource table isn't
> >reloaded throughout cycles of power up/down, and that is insecure.
> >We'll have to manually reload it somewhere after the rproc is powered
> >down (or before it is powered up again).
> >
> >This change will break existing firmwares, but it looks required and inevitable.
> 
> This patch is a stab on implementing what Ohad suggested originally, and loads firmware
> once. We can probably make this work as well, but I missed handling reload properly.
> For this to work I need to add support for reloading firmware upon reboot.
> 
> I go on vacation for a while now, but please get back to me on what direction we
> should go in: loading once or twice :-)

If we need to reload the firmware on remote processor reboots, we either
need to load the firmware more than once, or save a whole copy of it.
Since firmwares can get quite large, it would be a waste to hold two
copies in memory, so I'm not sure we can avoid loading the firmware more
than once.

We could read the firmware's checksum either from the elf file or from
an entry in the resource table. When requesting the firmware again, we
could compare the read checksum with the stored one, and abort if they
don't match. Given that only root can modify the firmware file, it
should be good enough.

Thanks,
Ido.

RE: [RFCv2 05/11] remoteproc: Load firmware once.

[Sjur BRENDELAND]

Hi Ido,
]
> > > In case the remote processor is added, booted and then rebooted, the
> > > program sections wont be reinitialized. This can be a bit problematic,
> > > since the firmware might assume values are initialized in its data
> > > sections.
> > >
> > > How about the following alternative approach - instead of loading the
> > > firmware in advance, we could allocate just a small (private) buffer,
> > > holding a copy of the resource table. Our copy will be updated with the
> > > addresses of the vrings we allocate, along with the notification ids.
> > > Each time the remote processor is booted, we could reload the firmware
> > > and overwrite the resource table section with our own private copy.
> > > virtio's configuration space and status will probably need to be updated
> > > in both copies of the resource table, so it would be consistent across
> > > boots.
> >
> > Thank you for review comments Ido!
> >
> > Hm, are you suggesting to load firmware once or twice?
> >
> > We could keep loading firmware twice and copy
> > the resource table. This solves the issue of initializing variables.
> > We could then keep the original approach with loading firmware
> > twice. But one question comes to mind:  is it safe to assume that
> > the firmware's resource table is unchanged after a reboot?
> >
> > [Ohad Aug 10, 2012 wrote.]
> > >The general direction I have in mind is to put the resource table in
> > >its final location while we do the first pass of fw parsing.
> > ...
> > >This will solve all sort of open issues we have (or going to have soon):
> > ...
> > >1. dynamically-allocated address of the vrings can be communicated
> > >2. vdev statuses can be communicated
> > >3. virtio config space will finally become bi-directional as it should
> > >4. dynamically probed rproc-to-rproc IPC could then take place
> > >
> > >It's the real deal :)
> > >
> > >The only problem with this approach is that the resource table isn't
> > >reloaded throughout cycles of power up/down, and that is insecure.
> > >We'll have to manually reload it somewhere after the rproc is powered
> > >down (or before it is powered up again).
> > >
> > >This change will break existing firmwares, but it looks required and
> inevitable.
> >
> > This patch is a stab on implementing what Ohad suggested originally, and
> loads firmware
> > once. We can probably make this work as well, but I missed handling reload
> properly.
> > For this to work I need to add support for reloading firmware upon reboot.
> >
> > I go on vacation for a while now, but please get back to me on what
> direction we
> > should go in: loading once or twice :-)
> 
> If we need to reload the firmware on remote processor reboots, we either
> need to load the firmware more than once, 

Yes, we're fully aligned here. I want to reload the firmware upon reboot.

...
>or save a whole copy of it.
> Since firmwares can get quite large, it would be a waste to hold two
> copies in memory, so I'm not sure we can avoid loading the firmware more
> than once.

I think you got me wrong. When I talk about loading firmware twice
I was referring to the current approach of calling request_firmware_nowait()
when rproc_add() is called and then calling request_firmware() again
when the virtio devices are probed (from rproc_boot).
The implementation I have done, avoids the second firmware loading by
storing the image into memory first time around.
So my question is: do you want to load firmware once or twice at the initial
boot?

> We could read the firmware's checksum either from the elf file or from
> an entry in the resource table. When requesting the firmware again, we
> could compare the read checksum with the stored one, and abort if they
> don't match. Given that only root can modify the firmware file, it
> should be good enough.

Yes, verifying the checksum sounds like a good idea.

Thanks,
Sjur

RE: [RFCv2 05/11] remoteproc: Load firmware once.

[Sjur BRENDELAND]

Hi Ido,

> From: Ido Yariv [mailto:ido@wizery.com]  Friday, December 21, 2012 3:16 AM
> > Load the firmware once and pave the way for two way
> > communication of virtio configuration and status bits.
> > The virtio ring device address is now stored in the resource
> > table.
> >
> > NOTE:  This patch requires device firmware change! The device
> > 	memory layout changes. The virtio rings are no longer
> > 	located at start of device memory. But the vring address
> > 	can be read from the resource table. Also Carveout
> > 	must be located before the virtio rings in the resource
> > 	table.
> >
> > Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
> 
> In case the remote processor is added, booted and then rebooted, the
> program sections wont be reinitialized. This can be a bit problematic,
> since the firmware might assume values are initialized in its data
> sections.
> 
> How about the following alternative approach - instead of loading the
> firmware in advance, we could allocate just a small (private) buffer,
> holding a copy of the resource table. Our copy will be updated with the
> addresses of the vrings we allocate, along with the notification ids.
> Each time the remote processor is booted, we could reload the firmware
> and overwrite the resource table section with our own private copy.
> virtio's configuration space and status will probably need to be updated
> in both copies of the resource table, so it would be consistent across
> boots.

I have kept the approach of loading firmware once for initial boot and crash.
But added reloading of firmware if the system is stopped and then started again.
This should solve your concerns above. This avoids requesting the same firmware
twice when calling rproc_add and at reboot. This will should improve boot time as
firmware will be loaded once not twice. But there is a catch, the memory layout
will change, the vrings are no longer allocated first, instead the firmware must read
the vring address from the resource table.

> > -static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
> > +static void rproc_fw_load(const struct firmware *fw, void *context)
> >  {
> > +	struct rproc *rproc = context;
> >  	struct device *dev = &rproc->dev;
> >  	const char *name = rproc->firmware;
> >  	struct resource_table *table;
> >  	int ret, tablesz;
> >
> > +	if (!fw)
> > +		goto release_fw;
> > +
> >  	ret = rproc_fw_sanity_check(rproc, fw);
> >  	if (ret)
> > -		return ret;
> > +		goto release_fw;
> >
> >  	dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size);
> >
> > @@ -800,7 +805,7 @@ static int rproc_fw_boot(struct rproc *rproc, const
> struct firmware *fw)
> >  	ret = rproc_enable_iommu(rproc);
> 
> iommu is enabled when the firmware is loaded, but disabled on shutdown,
> so it could get out of balance when the remote processor is rebooted.

OK, I'll make a separate patch that calls iommu_enable from rproc_boot
and iommu_disable from rproc_shutdown in order to keep the handling of
this symmetric.

> 
> ...
> 
> >  int rproc_boot(struct rproc *rproc)
> >  {
> > -	const struct firmware *firmware_p;
> >  	struct device *dev;
> >  	int ret;
> >
> > @@ -1004,27 +967,28 @@ int rproc_boot(struct rproc *rproc)
> >  	/* skip the boot process if rproc is already powered up */
> >  	if (atomic_inc_return(&rproc->power) > 1) {
> >  		ret = 0;
> > -		goto unlock_mutex;
> > +		goto downref_driver;
> 
> In case the rproc is already powered up, module_put should probably be
> called to balance try_module_get. This is actually an existing bug in the
> code.

I have changed the ref-counting so that  rproc->power is handled first in rproc_boot()
and rproc_shutdown(), and then module ref-counting follows device's start() and
stop() functions. Hopefully this should make it easier to keep the module ref in sync.

Regards,
Sjur

[RFCv2 06/11] remoteproc: Add resource entry number to callbacks

[Sjur Brændeland]

Add information on the resource entry sequence number
to the callback functions.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/remoteproc/remoteproc_core.c |   19 +++++++++++--------
 1 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index ea4b76a..2c78ea5 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -43,9 +43,8 @@
 
 #include "remoteproc_internal.h"
 
-typedef int (*rproc_handle_resources_t)(struct rproc *rproc,
-				struct resource_table *table, int len);
-typedef int (*rproc_handle_resource_t)(struct rproc *rproc, void *, int avail);
+typedef int (*rproc_handle_resource_t)(struct rproc *rproc, void *,
+							int avail, int seqno);
 
 /* Unique indices for remoteproc devices */
 static DEFINE_IDA(rproc_dev_index);
@@ -300,6 +299,7 @@ void rproc_free_vring(struct rproc_vring *rvring)
  * @rproc: the remote processor
  * @rsc: the vring resource descriptor
  * @avail: size of available data (for sanity checking the image)
+ * @seqno: resource entry sequence number
  *
  * This resource entry requests the host to statically register a virtio
  * device (vdev), and setup everything needed to support it. It contains
@@ -323,7 +323,7 @@ void rproc_free_vring(struct rproc_vring *rvring)
  * Returns 0 on success, or an appropriate error code otherwise
  */
 static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
-								int avail)
+							int avail, int seqno)
 {
 	struct device *dev = &rproc->dev;
 	struct rproc_vdev *rvdev;
@@ -389,6 +389,7 @@ free_rvdev:
  * @rproc: the remote processor
  * @rsc: the trace resource descriptor
  * @avail: size of available data (for sanity checking the image)
+ * @seqno: resource entry sequence number
  *
  * In case the remote processor dumps trace logs into memory,
  * export it via debugfs.
@@ -401,7 +402,7 @@ free_rvdev:
  * Returns 0 on success, or an appropriate error code otherwise
  */
 static int rproc_handle_trace(struct rproc *rproc, struct fw_rsc_trace *rsc,
-								int avail)
+							int avail, int seqno)
 {
 	struct rproc_mem_entry *trace;
 	struct device *dev = &rproc->dev;
@@ -462,6 +463,7 @@ static int rproc_handle_trace(struct rproc *rproc, struct fw_rsc_trace *rsc,
  * @rproc: remote processor handle
  * @rsc: the devmem resource entry
  * @avail: size of available data (for sanity checking the image)
+ * @seqno: resource entry sequence number
  *
  * Remote processors commonly need to access certain on-chip peripherals.
  *
@@ -483,7 +485,7 @@ static int rproc_handle_trace(struct rproc *rproc, struct fw_rsc_trace *rsc,
  * are outside those ranges.
  */
 static int rproc_handle_devmem(struct rproc *rproc, struct fw_rsc_devmem *rsc,
-								int avail)
+							int avail, int seqno)
 {
 	struct rproc_mem_entry *mapping;
 	struct device *dev = &rproc->dev;
@@ -542,6 +544,7 @@ out:
  * @rproc: rproc handle
  * @rsc: the resource entry
  * @avail: size of available data (for image validation)
+ * @seqno: resource entry sequence number
  *
  * This function will handle firmware requests for allocation of physically
  * contiguous memory regions.
@@ -556,7 +559,7 @@ out:
  * pressure is important; it may have a substantial impact on performance.
  */
 static int rproc_handle_carveout(struct rproc *rproc,
-				struct fw_rsc_carveout *rsc, int avail)
+			struct fw_rsc_carveout *rsc, int avail, int seqno)
 {
 	struct rproc_mem_entry *carveout, *mapping;
 	struct device *dev = &rproc->dev;
@@ -722,7 +725,7 @@ rproc_handle_resource(struct rproc *rproc, struct resource_table *table,
 		if (!handler)
 			continue;
 
-		ret = handler(rproc, rsc, avail);
+		ret = handler(rproc, rsc, avail, i);
 		if (ret)
 			break;
 	}
-- 
1.7.5.4

[RFCv2 07/11] remoteproc: Register virtio devices after vring allocation

[Sjur Brændeland]

Postpone the registration of virtio devices until all
vritio ring resource has been allocated.
This fixes the following bug: The driver's start callback
is called before all vring notify ids are allocated and
max_notifyid will be increased after starting the remoteproc.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/remoteproc/remoteproc_core.c |   26 ++++++++++++++++++++++++++
 include/linux/remoteproc.h           |    2 ++
 2 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 2c78ea5..8a7de5c 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -367,6 +367,8 @@ static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
 	/* remember the device features */
 	rvdev->dfeatures = rsc->dfeatures;
 
+	/* remember the device resource entry number */
+	rvdev->rsc_seqno = seqno;
 	/* remember the resource entry */
 	rvdev->rsc = rsc;
 
@@ -384,6 +386,20 @@ free_rvdev:
 	return ret;
 }
 
+static int rproc_register_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
+							int avail, int seqno)
+{
+	struct rproc_vdev *rvdev, *rvtmp;
+
+	dev_dbg(&rproc->dev, "register device %s\n", rproc->name);
+
+	list_for_each_entry_safe(rvdev, rvtmp, &rproc->rvdevs, node)
+		if (seqno == rvdev->rsc_seqno)
+			return rproc_add_virtio_dev(rvdev, rsc->id);
+
+	return -ENODEV;
+}
+
 /**
  * rproc_handle_trace() - handle a shared trace buffer resource
  * @rproc: the remote processor
@@ -692,6 +708,10 @@ static rproc_handle_resource_t rproc_handle_rsc[RSC_LAST] = {
 	[RSC_VDEV] = (rproc_handle_resource_t)rproc_handle_vdev,
 };
 
+static rproc_handle_resource_t rproc_handle_reg[RSC_LAST] = {
+	[RSC_VDEV] = (rproc_handle_resource_t)rproc_register_vdev,
+};
+
 /* handle firmware resource entries */
 static int
 rproc_handle_resource(struct rproc *rproc, struct resource_table *table,
@@ -834,6 +854,12 @@ static void rproc_fw_load(const struct firmware *fw, void *context)
 		goto clean_up;
 	}
 
+	ret = rproc_handle_resource(rproc, table, tablesz, rproc_handle_reg);
+	if (ret) {
+		dev_err(dev, "Failed to process resources: %d\n", ret);
+		goto clean_up;
+	}
+
 	rproc->state = RPROC_LOADED;
 
 	dev_info(dev, "remote processor %s is loaded to memory\n", rproc->name);
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 932edc7..60a1002 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -465,6 +465,7 @@ struct rproc_vring {
  * @vring: the vrings for this vdev
  * @dfeatures: virtio device features
  * @gfeatures: virtio guest features
+ * @rsc_seqno: sequence number in resource table
  * @rsc: vdev resource entry
  */
 struct rproc_vdev {
@@ -474,6 +475,7 @@ struct rproc_vdev {
 	struct rproc_vring vring[RVDEV_NUM_VRINGS];
 	unsigned long dfeatures;
 	unsigned long gfeatures;
+	u32 rsc_seqno;
 	struct fw_rsc_vdev *rsc;
 };
 
-- 
1.7.5.4

Re: [RFCv2 07/11] remoteproc: Register virtio devices after vring allocation

[Ido Yariv]

Hi Sjur,

On Fri, Dec 14, 2012 at 05:06:56PM +0100, Sjur Brændeland wrote:
> Postpone the registration of virtio devices until all
> vritio ring resource has been allocated.
> This fixes the following bug: The driver's start callback
> is called before all vring notify ids are allocated and
> max_notifyid will be increased after starting the remoteproc.

It seems that this patch wont solve this issue, since
rproc_add_virtio_dev eventually increases max_notifyid, so the same
problem will simply occur at a later stage.

Also, it seems that the original rproc_add_virtio_dev calls were not
removed, so devices would be added twice.

If max_notifyid needs to be set before the first virtio device is added,
how about counting the number of notification ids in advance before
adding the devices, by traversing the resource list twice?

Thanks,
Ido.

RE: [RFCv2 07/11] remoteproc: Register virtio devices after vring allocation

[Sjur BRENDELAND]

Hi Ido,

> From: Ido Yariv [mailto:ido@wizery.com]
> > Postpone the registration of virtio devices until all
> > vritio ring resource has been allocated.
> > This fixes the following bug: The driver's start callback
> > is called before all vring notify ids are allocated and
> > max_notifyid will be increased after starting the remoteproc.
> 
> It seems that this patch wont solve this issue, since
> rproc_add_virtio_dev eventually increases max_notifyid, so the same
> problem will simply occur at a later stage.
> 
> Also, it seems that the original rproc_add_virtio_dev calls were not
> removed, so devices would be added twice.

Indeed. This was introduced by a faulty merge - I'll fix this up.

> If max_notifyid needs to be set before the first virtio device is added,
> how about counting the number of notification ids in advance before
> adding the devices, by traversing the resource list twice?

Ugh, thanks I missed this. How about calling rproc_alloc_vring from
rproc_parse_vring so that the resource allocation are done before
the virtio devices are started? 

BTW, I think most of the other review comments from you are OK. I'll
try to address them in my next patch-set sometime early next year.

Regards,
Sjur

Re: [RFCv2 07/11] remoteproc: Register virtio devices after vring allocation

[Ido Yariv]

Hi Sjur,

On Fri, Dec 21, 2012 at 01:20:02PM +0100, Sjur BRENDELAND wrote:
> Hi Ido,
> 
> > From: Ido Yariv [mailto:ido@wizery.com]
> > > Postpone the registration of virtio devices until all
> > > vritio ring resource has been allocated.
> > > This fixes the following bug: The driver's start callback
> > > is called before all vring notify ids are allocated and
> > > max_notifyid will be increased after starting the remoteproc.
> > 
> > It seems that this patch wont solve this issue, since
> > rproc_add_virtio_dev eventually increases max_notifyid, so the same
> > problem will simply occur at a later stage.
> > 
> > Also, it seems that the original rproc_add_virtio_dev calls were not
> > removed, so devices would be added twice.
> 
> Indeed. This was introduced by a faulty merge - I'll fix this up.
> 
> > If max_notifyid needs to be set before the first virtio device is added,
> > how about counting the number of notification ids in advance before
> > adding the devices, by traversing the resource list twice?
> 
> Ugh, thanks I missed this. How about calling rproc_alloc_vring from
> rproc_parse_vring so that the resource allocation are done before
> the virtio devices are started? 

I don't think that would solve it - rproc_parse_vring is called from
rproc_handle_vdev which is called more than once (for each vdev), so
max_notifyid could still be increased after the first call to
rproc_add_virtio_dev.

> BTW, I think most of the other review comments from you are OK. I'll
> try to address them in my next patch-set sometime early next year.

Great, thanks!
Happy new year :)

Ido.

RE: [RFCv2 07/11] remoteproc: Register virtio devices after vring allocation

[Sjur BRENDELAND]

Hi Ido,
> > Ugh, thanks I missed this. How about calling rproc_alloc_vring from
> > rproc_parse_vring so that the resource allocation are done before
> > the virtio devices are started?

> I don't think that would solve it - rproc_parse_vring is called from
> rproc_handle_vdev which is called more than once (for each vdev), so
> max_notifyid could still be increased after the first call to
> rproc_add_virtio_dev.

Yes, but I think this will work if we allocate resources first,
and then in the next step register the virtio devices.
All the resources for all the vdevs will be allocated first
and then rproc_boot will be called.

> Great, thanks!
> Happy new year :)

Thanks, Same to you.

Regards,
Sjur

Re: [RFCv2 07/11] remoteproc: Register virtio devices after vring allocation

[Ohad Ben-Cohen]

Hi Sjur,

On Fri, Dec 21, 2012 at 3:50 PM, Sjur BRENDELAND
<sjur.brandeland@stericsson.com> wrote:
> Yes, but I think this will work if we allocate resources first,
> and then in the next step register the virtio devices.
> All the resources for all the vdevs will be allocated first
> and then rproc_boot will be called.

This sounds similar to what we had originally, before commit 6db20ea
"remoteproc: allocate vrings on demand, free when not needed". We can
revert this, but we then lose the properties 6db20ea provided.

Will Ido's suggestion to count the number of notifiyids in advance
work for you? at foresight it sounds like it could be just a few lines
of code, with no real hit on performance.

Thanks and happy holidays!
Ohad.

RE: [RFCv2 07/11] remoteproc: Register virtio devices after vring allocation

[Sjur BRENDELAND]

Hi Ido,

> On Fri, Dec 14, 2012 at 05:06:56PM +0100, Sjur Brændeland wrote:
> > Postpone the registration of virtio devices until all
> > vritio ring resource has been allocated.
> > This fixes the following bug: The driver's start callback
> > is called before all vring notify ids are allocated and
> > max_notifyid will be increased after starting the remoteproc.
> 
> It seems that this patch wont solve this issue, since
> rproc_add_virtio_dev eventually increases max_notifyid, so the same
> problem will simply occur at a later stage.
> 
> Also, it seems that the original rproc_add_virtio_dev calls were not
> removed, so devices would be added twice.
> 
> If max_notifyid needs to be set before the first virtio device is added,
> how about counting the number of notification ids in advance before
> adding the devices, by traversing the resource list twice?

OK, I'll just simply count the number of vrings registered then.

Regards,
Sjur

[RFCv2 08/11] remoteproc: Refactor function rproc_elf_find_rsc_table

[Sjur Brændeland]

Refatcor rproc_elf_find_rsc_table and split out the scanning
for the section header named resource table. This is done to
prepare for loading firmware once.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/remoteproc/remoteproc_elf_loader.c |   83 +++++++++++++++++-----------
 1 files changed, 51 insertions(+), 32 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
index e1f89d6..69832d9 100644
--- a/drivers/remoteproc/remoteproc_elf_loader.c
+++ b/drivers/remoteproc/remoteproc_elf_loader.c
@@ -208,38 +208,19 @@ rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
 	return ret;
 }
 
-/**
- * rproc_elf_find_rsc_table() - find the resource table
- * @rproc: the rproc handle
- * @fw: the ELF firmware image
- * @tablesz: place holder for providing back the table size
- *
- * This function finds the resource table inside the remote processor's
- * firmware. It is used both upon the registration of @rproc (in order
- * to look for and register the supported virito devices), and when the
- * @rproc is booted.
- *
- * Returns the pointer to the resource table if it is found, and write its
- * size into @tablesz. If a valid table isn't found, NULL is returned
- * (and @tablesz isn't set).
- */
-static struct resource_table *
-rproc_elf_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
-							int *tablesz)
+static struct elf32_shdr *
+find_rsc_shdr(struct device *dev, struct elf32_hdr *ehdr)
 {
-	struct elf32_hdr *ehdr;
 	struct elf32_shdr *shdr;
+	int i;
 	const char *name_table;
-	struct device *dev = &rproc->dev;
 	struct resource_table *table = NULL;
-	int i;
-	const u8 *elf_data = fw->data;
+	const u8 *elf_data = (void *)ehdr;
 
-	ehdr = (struct elf32_hdr *)elf_data;
+	/* look for the resource table and handle it */
 	shdr = (struct elf32_shdr *)(elf_data + ehdr->e_shoff);
 	name_table = elf_data + shdr[ehdr->e_shstrndx].sh_offset;
 
-	/* look for the resource table and handle it */
 	for (i = 0; i < ehdr->e_shnum; i++, shdr++) {
 		int size = shdr->sh_size;
 		int offset = shdr->sh_offset;
@@ -249,12 +230,6 @@ rproc_elf_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
 
 		table = (struct resource_table *)(elf_data + offset);
 
-		/* make sure we have the entire table */
-		if (offset + size > fw->size) {
-			dev_err(dev, "resource table truncated\n");
-			return NULL;
-		}
-
 		/* make sure table has at least the header */
 		if (sizeof(struct resource_table) > size) {
 			dev_err(dev, "header-less resource table\n");
@@ -280,10 +255,54 @@ rproc_elf_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
 			return NULL;
 		}
 
-		*tablesz = shdr->sh_size;
-		break;
+		return shdr;
+	}
+
+	return NULL;
+}
+
+/**
+ * rproc_elf_find_rsc_table() - find the resource table
+ * @rproc: the rproc handle
+ * @fw: the ELF firmware image
+ * @tablesz: place holder for providing back the table size
+ *
+ * This function finds the resource table inside the remote processor's
+ * firmware. It is used both upon the registration of @rproc (in order
+ * to look for and register the supported virito devices), and when the
+ * @rproc is booted.
+ *
+ * Returns the pointer to the resource table if it is found, and write its
+ * size into @tablesz. If a valid table isn't found, NULL is returned
+ * (and @tablesz isn't set).
+ */
+static struct resource_table *
+rproc_elf_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
+							int *tablesz)
+{
+	struct elf32_hdr *ehdr;
+	struct elf32_shdr *shdr;
+
+	struct device *dev = &rproc->dev;
+	struct resource_table *table = NULL;
+
+	const u8 *elf_data = fw->data;
+
+	ehdr = (struct elf32_hdr *)elf_data;
+
+	shdr = find_rsc_shdr(dev, ehdr);
+	if (!shdr)
+		return NULL;
+
+	/* make sure we have the entire table */
+	if (shdr->sh_offset + shdr->sh_size > fw->size) {
+		dev_err(dev, "resource table truncated\n");
+		return NULL;
 	}
 
+	table = (struct resource_table *)(elf_data + shdr->sh_offset);
+	*tablesz = shdr->sh_size;
+
 	return table;
 }
 
-- 
1.7.5.4

Re: [RFCv2 08/11] remoteproc: Refactor function rproc_elf_find_rsc_table

[Ido Yariv]

Hi Sjur,

On Fri, Dec 14, 2012 at 05:06:57PM +0100, Sjur Brændeland wrote:
> Refatcor rproc_elf_find_rsc_table and split out the scanning

Small typo there.

> for the section header named resource table. This is done to
> prepare for loading firmware once.
> 
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>

...

> +static struct elf32_shdr *
> +find_rsc_shdr(struct device *dev, struct elf32_hdr *ehdr)
>  {
> -	struct elf32_hdr *ehdr;
>  	struct elf32_shdr *shdr;
> +	int i;
>  	const char *name_table;
> -	struct device *dev = &rproc->dev;
>  	struct resource_table *table = NULL;
> -	int i;
> -	const u8 *elf_data = fw->data;
> +	const u8 *elf_data = (void *)ehdr;
>  
> -	ehdr = (struct elf32_hdr *)elf_data;
> +	/* look for the resource table and handle it */
>  	shdr = (struct elf32_shdr *)(elf_data + ehdr->e_shoff);
>  	name_table = elf_data + shdr[ehdr->e_shstrndx].sh_offset;
>  
> -	/* look for the resource table and handle it */
>  	for (i = 0; i < ehdr->e_shnum; i++, shdr++) {
>  		int size = shdr->sh_size;
>  		int offset = shdr->sh_offset;
> @@ -249,12 +230,6 @@ rproc_elf_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
>  
>  		table = (struct resource_table *)(elf_data + offset);
>  
> -		/* make sure we have the entire table */
> -		if (offset + size > fw->size) {
> -			dev_err(dev, "resource table truncated\n");
> -			return NULL;
> -		}
> -

This should probably be kept in the internal function, since it
dereferences the table as well. Moreover, this function will also be
called from other function locations.

It might also be a good idea to verify the offset as well, not just the
size.

Thanks,
Ido.

RE: [RFCv2 08/11] remoteproc: Refactor function rproc_elf_find_rsc_table

[Sjur BRENDELAND]

Hi Ido,

> > +static struct elf32_shdr *
> > +find_rsc_shdr(struct device *dev, struct elf32_hdr *ehdr)
> >  {
> > -	struct elf32_hdr *ehdr;
> >  	struct elf32_shdr *shdr;
> > +	int i;
> >  	const char *name_table;
> > -	struct device *dev = &rproc->dev;
> >  	struct resource_table *table = NULL;
> > -	int i;
> > -	const u8 *elf_data = fw->data;
> > +	const u8 *elf_data = (void *)ehdr;
> >
> > -	ehdr = (struct elf32_hdr *)elf_data;
> > +	/* look for the resource table and handle it */
> >  	shdr = (struct elf32_shdr *)(elf_data + ehdr->e_shoff);
> >  	name_table = elf_data + shdr[ehdr->e_shstrndx].sh_offset;
> >
> > -	/* look for the resource table and handle it */
> >  	for (i = 0; i < ehdr->e_shnum; i++, shdr++) {
> >  		int size = shdr->sh_size;
> >  		int offset = shdr->sh_offset;
> > @@ -249,12 +230,6 @@ rproc_elf_find_rsc_table(struct rproc *rproc, const
> struct firmware *fw,
> >
> >  		table = (struct resource_table *)(elf_data + offset);
> >
> > -		/* make sure we have the entire table */
> > -		if (offset + size > fw->size) {
> > -			dev_err(dev, "resource table truncated\n");
> > -			return NULL;
> > -		}
> > -
> 
> This should probably be kept in the internal function, since it
> dereferences the table as well. Moreover, this function will also be
> called from other function locations.

OK, I can do that.

> 
> It might also be a good idea to verify the offset as well, not just the
> size.

I'm not sure what you have in mind here. What sort of checks would you do on offset?

Regards,
Sjur

[RFCv2 09/11] remoteproc: Add operation to find resource table in memory

[Sjur Brændeland]

Add function find_rsc_table_va to firmware ops. This function
returns the location of the resource table in shared memory
after loading.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/remoteproc/remoteproc_elf_loader.c |   16 ++++++++++-
 drivers/remoteproc/remoteproc_internal.h   |   13 ++++++++
 drivers/remoteproc/ste_modem_rproc.c       |   43 +++++++++++++++++++---------
 3 files changed, 57 insertions(+), 15 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
index 69832d9..3f6e315 100644
--- a/drivers/remoteproc/remoteproc_elf_loader.c
+++ b/drivers/remoteproc/remoteproc_elf_loader.c
@@ -306,9 +306,23 @@ rproc_elf_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
 	return table;
 }
 
+struct resource_table *rproc_elf_get_rsctab_addr(struct rproc *rproc,
+						 const struct firmware *fw)
+{
+	struct elf32_shdr *shdr;
+
+	shdr = find_rsc_shdr(&rproc->dev, (struct elf32_hdr *)fw->data);
+	if (!shdr)
+		return NULL;
+
+	/* Find resource table in loaded segments */
+	return rproc_da_to_va(rproc, shdr->sh_addr, shdr->sh_size);
+}
+
 const struct rproc_fw_ops rproc_elf_fw_ops = {
 	.load = rproc_elf_load_segments,
 	.find_rsc_table = rproc_elf_find_rsc_table,
 	.sanity_check = rproc_elf_sanity_check,
-	.get_boot_addr = rproc_elf_get_boot_addr
+	.get_boot_addr = rproc_elf_get_boot_addr,
+	.get_rsctab_addr = rproc_elf_get_rsctab_addr
 };
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index 7bb6648..3a5cb7d 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -32,6 +32,7 @@ struct rproc;
  *			expects to find it
  * @sanity_check:	sanity check the fw image
  * @get_boot_addr:	get boot address to entry point specified in firmware
+ * @get_rsctab_addr:	get resouce table address as specified in firmware
  */
 struct rproc_fw_ops {
 	struct resource_table *(*find_rsc_table) (struct rproc *rproc,
@@ -40,6 +41,8 @@ struct rproc_fw_ops {
 	int (*load)(struct rproc *rproc, const struct firmware *fw);
 	int (*sanity_check)(struct rproc *rproc, const struct firmware *fw);
 	u32 (*get_boot_addr)(struct rproc *rproc, const struct firmware *fw);
+	struct resource_table *(*get_rsctab_addr)(struct rproc *rproc,
+						const struct firmware *fw);
 };
 
 /* from remoteproc_core.c */
@@ -102,6 +105,16 @@ struct resource_table *rproc_find_rsc_table(struct rproc *rproc,
 	return NULL;
 }
 
+static inline
+struct resource_table *rproc_get_rsctab_addr(struct rproc *rproc,
+				const struct firmware *fw)
+{
+	if (rproc->fw_ops->get_rsctab_addr)
+		return rproc->fw_ops->get_rsctab_addr(rproc, fw);
+
+	return NULL;
+}
+
 extern const struct rproc_fw_ops rproc_elf_fw_ops;
 
 #endif /* REMOTEPROC_INTERNAL_H */
diff --git a/drivers/remoteproc/ste_modem_rproc.c b/drivers/remoteproc/ste_modem_rproc.c
index a7743c0..59e99f1 100644
--- a/drivers/remoteproc/ste_modem_rproc.c
+++ b/drivers/remoteproc/ste_modem_rproc.c
@@ -64,26 +64,18 @@ static int sproc_load_segments(struct rproc *rproc, const struct firmware *fw)
 }
 
 /* Find the entry for resource table in the Table of Content */
-static struct ste_toc_entry *sproc_find_rsc_entry(const struct firmware *fw)
+static const struct ste_toc_entry *sproc_find_rsc_entry(const void *data)
 {
 	int i;
-	struct ste_toc *toc;
-
-	if (!fw)
-		return NULL;
-
-	toc = (void *)fw->data;
+	const struct ste_toc *toc;
+	toc = data;
 
 	/* Search the table for the resource table */
 	for (i = 0; i < SPROC_MAX_TOC_ENTRIES &&
 		    toc->table[i].start != 0xffffffff; i++) {
-
 		if (!strncmp(toc->table[i].name, SPROC_RESOURCE_NAME,
-			     sizeof(toc->table[i].name))) {
-			if (toc->table[i].start > fw->size)
-				return NULL;
+			     sizeof(toc->table[i].name)))
 			return &toc->table[i];
-		}
 	}
 
 	return NULL;
@@ -96,9 +88,12 @@ sproc_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
 {
 	struct sproc *sproc = rproc->priv;
 	struct resource_table *table;
-	struct ste_toc_entry *entry;
+	const struct ste_toc_entry *entry;
 
-	entry = sproc_find_rsc_entry(fw);
+	if (!fw)
+		return NULL;
+
+	entry = sproc_find_rsc_entry(fw->data);
 	if (!entry) {
 		sproc_err(sproc, "resource table not found in fw\n");
 		return NULL;
@@ -149,10 +144,30 @@ sproc_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
 	return table;
 }
 
+/* Find the resource table inside the remote processor's firmware. */
+static struct resource_table *
+sproc_get_rsctab_addr(struct rproc *rproc, const struct firmware *fw)
+{
+	struct sproc *sproc = rproc->priv;
+	const struct ste_toc_entry *entry;
+
+	if (!fw || !sproc->fw_addr)
+		return NULL;
+
+	entry = sproc_find_rsc_entry(sproc->fw_addr);
+	if (!entry) {
+		sproc_err(sproc, "resource table not found in fw\n");
+		return NULL;
+	}
+
+	return sproc->fw_addr + entry->start;
+}
+
 /* STE modem firmware handler operations */
 const struct rproc_fw_ops sproc_fw_ops = {
 	.load = sproc_load_segments,
 	.find_rsc_table = sproc_find_rsc_table,
+	.get_rsctab_addr = sproc_get_rsctab_addr,
 };
 
 /* Kick the modem with specified notification id */
-- 
1.7.5.4

Re: [RFCv2 09/11] remoteproc: Add operation to find resource table in memory

[Ido Yariv]

Hi Sjur,

On Fri, Dec 14, 2012 at 05:06:58PM +0100, Sjur Brændeland wrote:
> Add function find_rsc_table_va to firmware ops. This function
> returns the location of the resource table in shared memory
> after loading.
> 
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
> ---
>  drivers/remoteproc/remoteproc_elf_loader.c |   16 ++++++++++-
>  drivers/remoteproc/remoteproc_internal.h   |   13 ++++++++
>  drivers/remoteproc/ste_modem_rproc.c       |   43 +++++++++++++++++++---------
>  3 files changed, 57 insertions(+), 15 deletions(-)

Perhaps split this patch into two - one that adds the new firmware op
and another that modifies the ste driver?

> 
> diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
> index 69832d9..3f6e315 100644
> --- a/drivers/remoteproc/remoteproc_elf_loader.c
> +++ b/drivers/remoteproc/remoteproc_elf_loader.c
> @@ -306,9 +306,23 @@ rproc_elf_find_rsc_table(struct rproc *rproc, const struct firmware *fw,
>  	return table;
>  }
>  
> +struct resource_table *rproc_elf_get_rsctab_addr(struct rproc *rproc,
> +						 const struct firmware *fw)
> +{
> +	struct elf32_shdr *shdr;
> +
> +	shdr = find_rsc_shdr(&rproc->dev, (struct elf32_hdr *)fw->data);
> +	if (!shdr)
> +		return NULL;

Instead of traversing the headers twice, perhaps we could save the
address and size in advance and use it here?

Thanks,
Ido.

RE: [RFCv2 09/11] remoteproc: Add operation to find resource table in memory

[Sjur BRENDELAND]

Hi Ido,

> From: Ido Yariv [mailto:ido@wizery.com]
> > +struct resource_table *rproc_elf_get_rsctab_addr(struct rproc *rproc,
> > +						 const struct firmware *fw)
> > +{
> > +	struct elf32_shdr *shdr;
> > +
> > +	shdr = find_rsc_shdr(&rproc->dev, (struct elf32_hdr *)fw->data);
> > +	if (!shdr)
> > +		return NULL;
> 
> Instead of traversing the headers twice, perhaps we could save the
> address and size in advance and use it here?

To do that we would have to store the struct elf32_shdr in struct rproc.
So this is a trade-off between bloating rproc structure with shdr or 
spending some more cpu-cycles to scan the elf sections.
It shouldn't be that many sections to scan, and as we've just parsed
this firmware sections earlier so it might be hot in cache.
This shouldn't be very expensive, so I'd prefer to keep this as is.

Are you OK with that?

Regards,
Sjur

[RFCv2 10/11] remoteproc: Find resource table in device memory

[Sjur Brændeland]

Call rproc_get_rsctab_addr() to get the address
of the resource table in shared memory.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/remoteproc/remoteproc_core.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 8a7de5c..f95c08c 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -854,6 +854,13 @@ static void rproc_fw_load(const struct firmware *fw, void *context)
 		goto clean_up;
 	}
 
+	/* Find the new location of the resource table in device memory */
+	table  = rproc_get_rsctab_addr(rproc, fw);
+	if (!table) {
+		ret = -EINVAL;
+		goto clean_up;
+	}
+
 	ret = rproc_handle_resource(rproc, table, tablesz, rproc_handle_reg);
 	if (ret) {
 		dev_err(dev, "Failed to process resources: %d\n", ret);
-- 
1.7.5.4

Re: [RFCv2 10/11] remoteproc: Find resource table in device memory

[Ido Yariv]

Hi Sjur,

On Fri, Dec 14, 2012 at 05:06:59PM +0100, Sjur Brændeland wrote:
> Call rproc_get_rsctab_addr() to get the address
> of the resource table in shared memory.
> 
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>

This patch should probably be squashed with the next one, since it
doesn't do much on its own.

Thanks,
Ido.

RE: [RFCv2 10/11] remoteproc: Find resource table in device memory

[Sjur BRENDELAND]

> This patch should probably be squashed with the next one, since it
> doesn't do much on its own.

OK.

Regards,
Sjur

[RFCv2 11/11] remoteproc: Support virtio config space.

[Sjur Brændeland]

Support virtio configuration space and device status and
feature negotiation with remote device. This virtio device
can now access the resource table in shared memory.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/remoteproc/remoteproc_core.c   |   10 ++++------
 drivers/remoteproc/remoteproc_virtio.c |   30 +++++++++++++++++++++++++++---
 include/linux/remoteproc.h             |    2 --
 3 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index f95c08c..d59c4cf 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -364,13 +364,8 @@ static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
 			goto free_rvdev;
 	}
 
-	/* remember the device features */
-	rvdev->dfeatures = rsc->dfeatures;
-
 	/* remember the device resource entry number */
 	rvdev->rsc_seqno = seqno;
-	/* remember the resource entry */
-	rvdev->rsc = rsc;
 
 	list_add_tail(&rvdev->node, &rproc->rvdevs);
 
@@ -394,8 +389,11 @@ static int rproc_register_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
 	dev_dbg(&rproc->dev, "register device %s\n", rproc->name);
 
 	list_for_each_entry_safe(rvdev, rvtmp, &rproc->rvdevs, node)
-		if (seqno == rvdev->rsc_seqno)
+		if (seqno == rvdev->rsc_seqno) {
+			/* remember the resource entry */
+			rvdev->rsc = rsc;
 			return rproc_add_virtio_dev(rvdev, rsc->id);
+		}
 
 	return -ENODEV;
 }
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index e7a4780..9ebc5d9 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -176,16 +176,21 @@ error:
  */
 static u8 rproc_virtio_get_status(struct virtio_device *vdev)
 {
-	return 0;
+	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
+	return rvdev->rsc->status;
 }
 
 static void rproc_virtio_set_status(struct virtio_device *vdev, u8 status)
 {
+	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
+	rvdev->rsc->status = status;
 	dev_dbg(&vdev->dev, "status: %d\n", status);
 }
 
 static void rproc_virtio_reset(struct virtio_device *vdev)
 {
+	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
+	rvdev->rsc->status = 0;
 	dev_dbg(&vdev->dev, "reset !\n");
 }
 
@@ -194,7 +199,7 @@ static u32 rproc_virtio_get_features(struct virtio_device *vdev)
 {
 	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
 
-	return rvdev->dfeatures;
+	return rvdev->rsc->dfeatures;
 }
 
 static void rproc_virtio_finalize_features(struct virtio_device *vdev)
@@ -213,7 +218,23 @@ static void rproc_virtio_finalize_features(struct virtio_device *vdev)
 	 * fixed as part of a small resource table overhaul and then an
 	 * extension of the virtio resource entries.
 	 */
-	rvdev->gfeatures = vdev->features[0];
+	rvdev->rsc->gfeatures = vdev->features[0];
+}
+
+void rproc_virtio_get(struct virtio_device *vdev, unsigned offset,
+							void *buf, unsigned len)
+{
+	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
+	void *cfg = &rvdev->rsc->vring[rvdev->rsc->num_of_vrings];
+	memcpy(buf, cfg + offset, len);
+}
+
+void rproc_virtio_set(struct virtio_device *vdev, unsigned offset,
+		      const void *buf, unsigned len)
+{
+	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
+	void *cfg = &rvdev->rsc->vring[rvdev->rsc->num_of_vrings];
+	memcpy(cfg + offset, buf, len);
 }
 
 static struct virtio_config_ops rproc_virtio_config_ops = {
@@ -224,6 +245,9 @@ static struct virtio_config_ops rproc_virtio_config_ops = {
 	.reset		= rproc_virtio_reset,
 	.set_status	= rproc_virtio_set_status,
 	.get_status	= rproc_virtio_get_status,
+	.get		= rproc_virtio_get,
+	.set		= rproc_virtio_set,
+
 };
 
 /*
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 60a1002..de20e39 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -473,8 +473,6 @@ struct rproc_vdev {
 	struct rproc *rproc;
 	struct virtio_device vdev;
 	struct rproc_vring vring[RVDEV_NUM_VRINGS];
-	unsigned long dfeatures;
-	unsigned long gfeatures;
 	u32 rsc_seqno;
 	struct fw_rsc_vdev *rsc;
 };
-- 
1.7.5.4

Re: [RFCv2 11/11] remoteproc: Support virtio config space.

[Ido Yariv]

Hi Sjur,

On Fri, Dec 14, 2012 at 05:07:00PM +0100, Sjur Brændeland wrote:
> Support virtio configuration space and device status and
> feature negotiation with remote device. This virtio device
> can now access the resource table in shared memory.
> 
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
> ---

...

> +void rproc_virtio_get(struct virtio_device *vdev, unsigned offset,
> +							void *buf, unsigned len)
> +{
> +	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
> +	void *cfg = &rvdev->rsc->vring[rvdev->rsc->num_of_vrings];
> +	memcpy(buf, cfg + offset, len);
> +}
> +
> +void rproc_virtio_set(struct virtio_device *vdev, unsigned offset,
> +		      const void *buf, unsigned len)
> +{
> +	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
> +	void *cfg = &rvdev->rsc->vring[rvdev->rsc->num_of_vrings];
> +	memcpy(cfg + offset, buf, len);
>  }

Perhaps verify the offset and length to avoid overwriting memory?

Thanks,
Ido.

RE: [RFCv2 11/11] remoteproc: Support virtio config space.

[Sjur BRENDELAND]

Hi Ido,

> > +void rproc_virtio_get(struct virtio_device *vdev, unsigned offset,
> > +							void *buf, unsigned
> len)
> > +{
> > +	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
> > +	void *cfg = &rvdev->rsc->vring[rvdev->rsc->num_of_vrings];
> > +	memcpy(buf, cfg + offset, len);
> > +}
> > +
> > +void rproc_virtio_set(struct virtio_device *vdev, unsigned offset,
> > +		      const void *buf, unsigned len)
> > +{
> > +	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
> > +	void *cfg = &rvdev->rsc->vring[rvdev->rsc->num_of_vrings];
> > +	memcpy(cfg + offset, buf, len);
> >  }
> 
> Perhaps verify the offset and length to avoid overwriting memory?

OK, I'll add checks on size of the config space.

Regards,
Sjur