onomondo / nrf-softsim

Manifest repo for integrating SoftSIM and nrf-sdk
30 stars 6 forks source link

f_cache: possibly redundant? #14

Open JordanYates opened 11 months ago

JordanYates commented 11 months ago

The custom NVS caching implementation in f_cache.c is potentially redundant, as the Zephyr NVS filesystem has built-in caching functionality, CONFIG_NVS_LOOKUP_CACHE.

The caching mechanism makes lookup times linear, instead of needing to walk the linked list. NVS additionally already performs duplicate data checking to reduce writes: https://github.com/zephyrproject-rtos/zephyr/blob/f9051d626d7607f1e0e18f5bafd2acb3a199ac12/subsys/fs/nvs/nvs.c#L1078-L1083

A major advantage of using the built-in caching is that there is no need to buffer entries in RAM, as the cache contains a pointer directly to the allocated address in the NVS filesystem.

This is just a suggestion, as I can't know if there is additional de-duplication occurring inside libstorage.a based on the _b_dirty field.

peterbornerup commented 11 months ago

Hi Jordan,

Thanks for the comments. I've opened a ticked internally to address this! 😄


Yes, we've considered relying on the internals of NVS more heavily. I'll make the use of cache in f_cache optional.

JordanYates commented 11 months ago

The SIM is typically written to every time a new authentication happens and/or new network is selected etc. By not committing anything to flash until last second (modem is turned off) we can save a few writes to flash (well, unless that is cached as well). Specifically the nrf91 series is low-enough power to be on at all times so most writes could be omitted.

I thought that might be the case, but that seemed dangerous to me with the possibility of application land assertions/reboots and unexpected power cuts. But my knowledge of the SIM card spec is 2 minutes skimming the link in your readme 😄

libstorage opens by path and not by ID. f_cache was originally just to translate from char * -> uint16_t - the caching part is just a bi-product of that.

From my brief skim, I can see that files have a unique 2 byte ID per DF, is there a practical limit to the level of DF nesting in the spec? I could see some alternate NVS mode working with 64bit ID's instead of 16, which could give 4 levels of nesting without any conversion necessary.

peterbornerup commented 11 months ago

I thought that might be the case, but that seemed dangerous to me with the possibility of application land assertions/reboots and unexpected power cuts. But my knowledge of the SIM card spec is 2 minutes skimming the link in your readme 😄

All authentication related files are committed as soon as the "file" is closed. This is encoded in the file IDs. This is done to mitigate all risk of replay attacks etc. If other file updates are lost the next attach will at worst just be slower.

From my brief skim, I can see that files have a unique 2 byte ID per DF, is there a practical limit to the level of DF nesting in the spec? I could see some alternate NVS mode working with 64bit ID's instead of 16, which could give 4 levels of nesting without any conversion necessary.

We unfortunately already reach the limit at this point. The max depth is currently 4 layers. However, we need to distinguish between actual files and their corresponding FCP (file control parameters). For this SoftSIM currently uses the ".def" suffix to annotate this.

So, /3f00/7ff0/5f3b/4f20.def isn't trivially encoded unfortunately. Unless the 3f00 is assumed to always be present.