Hi, On Thu, 30 May 2024 at 09:17, Tim Harvey <thar...@gateworks.com> wrote: > > On Wed, May 29, 2024 at 6:39 PM Marek Vasut <ma...@denx.de> wrote: > > > > On 5/29/24 7:05 PM, Simon Glass wrote: > > > > [...] > > > > >>>> that is not yet implemented as DM_RNG. We also skip this if > > >>>> MEASURED_BOOT is enabled as in that case any modifications to the > > >>>> dt will cause measured boot to fail (although there are many other > > >>>> places the dt is altered). > > >>>> > > >>>> As this fdt node is added elsewhere create a library function and > > >>>> use it to deduplicate code. We will provide a parameter to specify the > > >>>> index of the rng device as well as a boolean to overwrite if present. > > >>>> > > >>>> For our automatic injection, we will use the first rng device and > > >>>> not overwrite if already present with a non-zero value (which may > > >>>> have been populated by an earlier boot stage). This way if a board > > >>>> specific ft_board_setup() function wants to customize this behavior > > >>>> it can call fdt_kaslrseed with a rng device index of its choosing and > > >>>> set overwrite true. > > >>> > > >>> I suggest creating a sysinfo driver which can provide the RNG device. > > >> > > >> I'm not really clear what you mean but that seems out of scope for > > >> this patch. If/when someone needs to extend the functionality of > > >> specifying one of many RNG's perhaps they can follow that approach. > > > > > > The thing is, you are creating a way to specify the RNG which will > > > presumably lead to different vendors all doing it a different way. > > > > > > Specifically, fdt_kaslrseed() takes a device number (would be better > > > to take a struct udevice, BTW). > > > > > > Instead, I think you should define the standard way that the chosen > > > RNG should be specified by the board. The sysinfo driver can provide a > > > way to do this. Then boards can handle this themselves, using sysinfo, > > > rather than directly calling a function in the fixup. After all, we > > > want the fixup flow to be as generic as possible. > > > > I think this patch simply picks up the first RNG that is available, > > pulls a number out of it, and uses that as the KASLR seed in DT. That's > > what the implementation(s) seems to be doing so far, this patch is a > > deduplication only, so maybe we should retain the original behavior > > first, and do architectural changes in follow up patch(es) ? > > Marek, > > Thank you - I had a draft saying exactly the same thing that I hadn't > sent out yet. > > This is an incremental improvement... for sure there are follow-on > improvements that can be made and should be made but the sysinfo and > policy creation are something I don't have time for at the moment and > may not be the right person to implement that. I'm happy to accept > something to add to my series though! > > So the question is, if we can agree that improvements can continue to > be made later should I do one more revision that split this out into > three patches and fixes the typo in the commit?
OK, well specifically, my objection is to the 'index' argument, since someone will pass 1 to it and then we have a new API. So if you can drop that and go back to the hard-coded 0, then I agree this is just a refactoring and yes, this is fine. Regards, Simon
