Message-ID: <81C3A93C17462B4BBD7E272753C1057924576E509F@EXDCVYMBSTM005.EQ1STM.local>
Date:	Fri, 21 Dec 2012 15:04:09 +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,
]
> > > 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
--
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/

Powered by blists - more mailing lists