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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 15 Jan 2013 16:11:06 +0100
From:	Sjur BRENDELAND <sjur.brandeland@...ricsson.com>
To:	Ido Yariv <ido@...ery.com>
Cc:	Ohad Ben-Cohen <ohad@...ery.com>,
	Linus Walleij <linus.walleij@...aro.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Sjur Brændeland <sjur@...ndeland.net>
Subject: RE: [RFCv2 05/11] remoteproc: Load firmware once.

Hi Ido,

> From: Ido Yariv [mailto:ido@...ery.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@...ricsson.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/