Messages in this thread | | | From | Julius Werner <> | Date | Wed, 10 Aug 2022 18:17:15 -0700 | Subject | Re: [PATCH v7] firmware: google: Implement cbmem in sysfs driver |
| |
> Got it. Thanks for the background. Is it possible to create new entries > in the table? Or to resize existing entries? Or to delete entries > entirely?
Not easily (would have to see if there's still space at the end and rewrite the table header), and more importantly there should be no reason to ever do that at OS runtime. This table is only meant for coreboot to publish information about itself or store data that needs to stay resident for whatever reason. Userspace should be able to access the things that are already there but it isn't meant as a free-for-all for other environments to add on to.
> The /dev/mem interface has been restricted over the years. At this point > we're trying to steer users away from /dev/mem to anything else. I > suspect it happens to work right now because coreboot tells the kernel > that there isn't actually memory in this address range so that devmem > can map it. I don't know but I wonder if the memory is being mapped > uncached on ARM systems, leading to slower access times? Usually when > memory addresses aren't marked as normal memory that's reserved it > doesn't get mapped until the memremap() time, and that would be mapped > with whatever attributes are used in the call. /dev/mem doesn't optimize > this from what I recall.
Yes, we mark those areas as reserved in the e820 / device tree. The kernel drivers (this one and the older ones) use MEMREMAP_WB which should do the right thing. `cbmem` uses mmap(MAP_SHARED) on /dev/mem which I thought does the right thing but I'm not quite sure. That's another good reason to implement a dedicated sysfs interface where we have finer control about these things, once we have that we can make the older tools use it as well on supporting kernels. (`cbmem` is currently not called in any critical boot path on Chrome OS as far as I know, so its performance is not that critical. The new use case Jack wants to build _is_ going to be in a critical path, though, so we should make sure it is as performant as it can be.)
> Fair enough. How similar is this to efivars? I don't know, and you may > not either, but at a high level it looks similar. The sysfs interface to > efivars was deprecated and I saw recently that there's an effort to > remove it entirely. The new way of interacting with those firmware > variables is through a filesystem that's mounted at > /sys/firmware/efi/efivars. The documentation[1] states that the sysfs > interface didn't work when the variables got large. Hopefully that won't > be a similar scenario here?
I don't even know what efivars is tbh. But whatever other cases with other firmwares may exist that may or may not be more standardized and make more sense to implement high-touch drivers for in kernel space, here we really have something that's really opaque and really not meant to be tampered with by external code, so I think this agnostic byte buffer access is the only thing that makes sense. I don't see any reason why there would be size restrictions with the implementation Jack proposed? FWIW, the total size of the CBMEM buffer we want to access is 80K on current builds of coreboot (but our use case will likely only need to read the first couple hundred bytes).
| |