search

ibm-openbmc / openbmc

https://github.com
Other
22 stars 52 forks source link

assetTag: Add persistent configuration of AssetTag for IPS #251

Closed lxwinspur closed 1 year ago

lxwinspur commented 1 year ago

IPS has a requirement that when restoring the factory settings, it is not allowed to restore the AssetTag property. There is a solution to persist the AssetTag property by executing the set/getEnv method.

Signed-off-by: George Liu liuxiwei@inspur.com

jenkins-openbmc-ibm commented 1 year ago

Can one of the admins verify this patch?

geissonator commented 1 year ago

https://github.com/ibm-openbmc/openbmc/pull/238 was the PR we discussed on why using u-boot to preserve userspace values was not a good idea.

lxwinspur commented 1 year ago

@geissonator Yes, I know we discussed this issue. But now IBM has created an ips branch for IPS, and after discussions with our team, we hope that this feature can be merged into the release version before GA, which will not affect IBM's 1030 branch.

Also, Could you kindly please elaborate onwhy this approach is dangerous? For example security etc? Thanks :)

geissonator commented 1 year ago

Also, Could you kindly please elaborate onwhy this approach is dangerous? For example security etc?

I've added @shenki to see if there are any hard points that will break in adding this requisite between this new u-boot env variable and userspace services. I think in general, it's just been strongly discouraged. But in the end you are right, IPS does have it's own branch so as long as there's nothing that becomes more insecure or breaks, we can probably just go with this.

For P11, we are headed in a direction to have a section of the eMMC chip that is not cleared on factory resets specifically for these types of use cases.

shenki commented 1 year ago

Future secure boot plans may disable the u-boot environment, or stop userspace from being able to write to it. This is not implemented for 1030, so it will work for now, but may not in the future.

I assume other options were explored and this was the only way to solve the problem. If this is the case then I can't see any impacts for IPS putting this in their 1030 branch, other than creating headaches for future upgrades.

lxwinspur commented 1 year ago

Thanks @shenki @geissonator So I suggest merging this function in the 1030. ips branch. In the future, we will synchronize this function again according to the new method of ibm