[coreboot-gerrit] Change in coreboot[master]: vpd: Add support for reading BD addreses from the VPD

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/28711 )

Change subject: vpd: Add support for reading BD addreses from the VPD
......................................................................


Patch Set 1:

Hmm... hmmm... how much of my yak would you be willing to shave? ;)

I mean, we can take this, and it will work (you'll need a little depthcharge change as well, to call dt_set_mac_addresses()). But this code and the whole pass-MACs-through-coreboot-table mechanism is sort of deprecated, because we now have a VPD driver in depthcharge. You could really implement everything we need for this solely in depthcharge. (I've also never been a big fan of most of this code, honestly...)

The other problem is that even though this can match multiple template strings, it was only ever used with one group per device. Cheza is the first one that will have both WiFi and Bluetooth MACs now, and for those this mechanism becomes very ugly. If you look at how it is matched (e.g. depthcharge/src/board/gale/board.c:mac_maps[]), things are only assigned by index, there is no matching back to the original VPD name. That means that you'll just have to happen to know that e.g. the first two MACs are WiFi MACs and the next three are Bluetooth MACs. It also means that an attacker with temporary root access can permanently hose your bluetooth by sneaking a couple of wifi_mac2, wifi_mac3 keys into your RW_VPD, in a way that recovery will not normally fix.

I think it would be much better to design a new mechanism for Cheza that lives entirely within depthcharge and gets rid of all the weird design quirks of this legacy crap. I imagine something where you just do this in depthcharge/src/cheza/board.c:

 static const VpdDeviceTreeMap vpd_dt_map[] = {
   { "wifi_mac0", { "path_to", "WiFi_MAC0", "DTS_node", NULL } },
   { "wifi_mac1", { "path_to", "WiFi_MAC1", "DTS_node", NULL } },
   { "bluetooth_mac0", { "path_to", "BT_MAC0", "DTS_node", NULL } },
 }

 static int board_setup(void) {
   ...
   install_vpd_dt_fixup(&vpd_dt_map, sizeof(vpd_dt_map));
   ...
 }

And then you have a fixup function that just looks up a single VPD entry directly (and you don't need all this "increase number to search for all entries" crap), runs depthcharge's vpd_gets() to find it, decode it and throw it in the DT if found.


-- 
To view, visit https://review.coreboot.org/28711
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib290fba2504f6ed80c3a95e24627ae5922e608af
Gerrit-Change-Number: 28711
Gerrit-PatchSet: 1
Gerrit-Owner: Matthias Kaehlcke <mka at chromium.org>
Gerrit-Reviewer: Brian Norris <briannorris at chromium.org>
Gerrit-Reviewer: Julius Werner <jwerner at chromium.org>
Gerrit-Reviewer: Matthias Kaehlcke <mka at chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-Comment-Date: Fri, 21 Sep 2018 23:39:49 +0000
Gerrit-HasComments: No
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20180921/a8d05852/attachment.html>