IssueTracker

It is coincidentally in CBMEM already, actually (as part of the persistent vb2_context left over from coreboot), but currently not accessed (or kept in sync) by crossystem.

I'm not sure if this is really a good idea, though. This isn't really the same situation as VPD -- VPD is meant to basically be written once and then never again (barring very rare circumstances). For VPD the risk of cached data going stale is very low, and the consequences don't really matter in practice. vboot nvdata on the other hand gets overwritten all the time and it is pretty important that changes are visible immediately or things could get badly confused. If we did cache it, we would have to guarantee that writes are always synced back to the cache and the entire thing would get quite complicated.

Is there any specific motivation to this other than "this might be neat"? I know flash access times are slow, but generally we should not be reading vboot nvdata in any critical path (I think once upon a time we made sure that is true, not sure if it has crept back in since). If that is the motivation I think it would probably be better to try to eliminate those cases instead.

If we do really want to do this, I'm not convinced it should be a kernel module (because you'd want to make sure the entire thing -- reading, writing, syncing back to the cache -- is implemented in the same place, and doing all that in the kernel sounds like a layering violation). I guess it could all be done in crossystem via /dev/mem, but again, it would be very complicated and require a lot of rewriting (everything inside crossystem is currently based on copies of these data structures in FDT/ACPI, whereas we would then be switching to finding the originals in CBMEM instead) which I doubt is worth it in this case.

Two motivations:

1. Critical path (explained below)
2. This is the easiest way to guarantee that the way we find VBNV is consistent between coreboot+depthcharge+crossystem -- by using the same actual data.

Is there any specific motivation to this other than "this might be neat"? I know flash access times are slow, but generally we should not be reading vboot nvdata in any critical path (I think once upon a time we made sure that is true, not sure if it has crept back in since). If that is the motivation I think it would probably be better to try to eliminate those cases instead.

Yeah, it's in the critical path (and, AFAICT, it always has been). The statistics provider in chrome reads this before starting the UI.

If we do really want to do this, I'm not convinced it should be a kernel module (because you'd want to make sure the entire thing -- reading, writing, syncing back to the cache -- is implemented in the same place, and doing all that in the kernel sounds like a layering violation). I guess it could all be done in crossystem via /dev/mem, but again, it would be very complicated and require a lot of rewriting (everything inside crossystem is currently based on copies of these data structures in FDT/ACPI, whereas we would then be switching to finding the originals in CBMEM instead) which I doubt is worth it in this case.

Ideally this pairs with a kernel driver. We would have to write to sysfs for writes (so no /dev/mem), and then call flashrom.

Fortunately, there's only one tool which writes to VBNV from linux, and that's crossystem. So only one place to write to two.

It is coincidentally in CBMEM already, actually

Would rather not rely on coincidence here and instead create our own CBMEM tag.

This is the easiest way to guarantee that the way we find VBNV is consistent between coreboot+depthcharge+crossystem -- by using the same actual data.

I mean, the source of truth for all of these should be flash -- coreboot and depthcharge do cache it but they make sure it is always written back to flash before handing off to any other environment. I don't think this is the reason for any of the weird corruption cases we've seen (I think that rather has to do with the different lookup algorithms).

Yeah, it's in the critical path (and, AFAICT, it always has been). The statistics provider in chrome reads this before starting the UI.

In Chrome?! Okay, that's news to me. And they can't just parallelize that into a background task? Why does statistics gathering need to hold up the login screen?

Ideally this pairs with a kernel driver. We would have to write to sysfs for writes (so no /dev/mem), and then call flashrom.

Well, then you split the responsibility for keeping this in sync between kernel and userspace, and if anyone wrote directly to the sysfs files that would be bad. I guess maybe this is okay if the sysfs interface was so unwieldy that nobody could use it without proper tooling anyway. I don't think this should be a thing like VPD where the kernel parses all the data and makes individual fields available through sysfs nodes... both reads and writes (and all the data format interpretation) should still go through crossystem, and then maybe crossystem can use some opaque sysfs interface that just presents the whole CBMEM region as a file in the background. We already have the "coreboot bus" concept in sysfs today (where every coreboot table entry is presented as a device), maybe that could be expanded a bit to just allow raw reading and writing for all cbmem_ref tags. (But then again, at that point you might as well just expose the address and size through sysfs and let clients use those in /dev/mem instead.)

Would rather not rely on coincidence here and instead create our own CBMEM tag.

I mean, this work buffer is the source of truth for all vboot runtime data, I don't think there's a point in creating yet another stale copy here somewhere (we already have all the hoops in place to make sure nvdata is synced between flash and work buffer at all firmware exit points, no reason to make this even more complicated with a third copy). If you want to read this from memory, this is the place to do it from.

FWIW we have been wanting crossystem to access that directly for some time already anyway, right now it's still using an FDT/ACPI copy of the same information that needs to be awkwardly translated back into the older vboot 1 VBSD format by firmware. Getting rid of that would be nice. It's something that Joel meant to do at some point but never got around to before he left.

Well, then you split the responsibility for keeping this in sync between kernel and userspace, and if anyone wrote directly to the sysfs files that would be bad. I guess maybe this is okay if the sysfs interface was so unwieldy that nobody could use it without proper tooling anyway. I don't think this should be a thing like VPD where the kernel parses all the data and makes individual fields available through sysfs nodes... both reads and writes (and all the data format interpretation) should still go through crossystem, and then maybe crossystem can use some opaque sysfs interface that just presents the whole CBMEM region as a file in the background. We already have the "coreboot bus" concept in sysfs today (where every coreboot table entry is presented as a device), maybe that could be expanded a bit to just allow raw reading and writing for all cbmem_ref tags. (But then again, at that point you might as well just expose the address and size through sysfs and let clients use those in /dev/mem instead.)

You've got the right idea here. By no means exposing the fields like VPD, but just the raw binary content. Thus, crossystem would be a necessity to work with the data.

FWIW we have been wanting crossystem to access that directly for some time already anyway, right now it's still using an FDT/ACPI copy of the same information that needs to be awkwardly translated back into the older vboot 1 VBSD format by firmware. Getting rid of that would be nice. It's something that Joel meant to do at some point but never got around to before he left.

If we plan to use the cbmem copy of all the vboot context in crossystem, I think it makes sense, and we can expand the scope of this bug to that. VBNV would just be a part of that.

The kernel end of this landed, but I'm no longer on the firmware team or have context if this is still important. Will unassign from myself; someone else feel free to pick this up if you want to implement the userspace end of this.

Let's throw it in the backlog for now. Should probably wait to see how the whole Aluminium BootControl thing turns out in the end and how much of crossystem actually survives in the long term before we bother to look into this again. (But it's nice to have the kernel part landed upstream already so if we do ever pick it up again it will probably have trickled through to most of our non-AUE kernels by then.)