<div dir="ltr">Thanks for the input. It took a little while (other commitments ...), but I now have a version that solves the problem and shouldn't break existing behavior: <a href="https://gerrit.openbmc.org/c/openbmc/phosphor-host-ipmid/+/71530" target="_blank">https://gerrit.openbmc.org/c/openbmc/phosphor-host-ipmid/+/71530</a><div><div><br></div><div>I added you as a reviewer, Willy. </div><div><br></div><div>Oskar</div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, May 9, 2024 at 6:17 PM Willy Tu <<a href="mailto:wltu@google.com" target="_blank">wltu@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">I am not aware of a way to differentiate between the clients. Might just have to error out as you suggested.<div><br></div><div>Willy Tu</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, May 7, 2024 at 5:05 PM Oskar Senft <<a href="mailto:osk@google.com" target="_blank">osk@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">I got some code changes that look right, but haven't done much testing yet. I realized that a read cache is useful, so I kept it. I split read and write cache by defining a new class that keeps all the related data together.<div><br></div><div>Any suggestions on how to distinguish different clients? Or maybe we just error out when we receive a request for a different FRU while we're still not done with the first one?</div><div><br></div><div>Oskar.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, May 7, 2024 at 7:49 PM Willy Tu <<a href="mailto:wltu@google.com" target="_blank">wltu@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">I think we should make it so that if a different IPMI client tries to write the Fru we prevent it and only allow one write at a time. I think the fruCache for Read is mostly from some commands that is dealing with one device ID multiple times. When reading subsequent ids... then it doesn't do anything.<div><br></div><div>I think we will need more discussion on that topic since it will be a larger refactor to make that work.</div><div><br></div><div>Sounds good. Please let me know how it goes.</div><div><br></div><div>Willy Tu</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, May 7, 2024 at 10:32 AM Oskar Senft <<a href="mailto:osk@google.com" target="_blank">osk@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Hi Willy<div><br></div><div>Thanks for your input! </div><div><br></div><div>From what I can tell, the current implementation will fail in wondrous ways if there's more than one IPMI client trying to write FRU at the same time. The existing getFru guards against changing target devId between calls to not hand-out the same cache for different requests. However, this will clearly break when different IPMI clients attempt to write the same or different FRUs at the same time.</div><div><br></div><div>We could argue whether that's a supported use case or if we just assume that'll never happen ... it does seem like quite a bit of an edge case, though.</div><div><br></div><div>I do see it as an issue if there were multiple clients with only one writing, but others reading - that'll fail in similarly weird ways.</div><div><br></div><div>I'm wondering: Do we want to have the fruCache for _reads_ at all? It seems actually quite wrong, since subsequent reads for the same FRU would always return the same result, even if the FRU changed through some other mechanism.</div><div><br></div><div>Let me work on a fix that would use the cache only for writing and would keep it around until the timeout expired.</div><div><br></div><div>Oskar.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, May 3, 2024 at 11:38 AM Willy Tu <<a href="mailto:wltu@google.com" target="_blank">wltu@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Hi Oskar,<div><br></div><div>> C) ipmiStorageWriteFruData clears the cache immediately after WriteFru was called. Maybe we should keep that cache around until the timeout expires?</div><div><br></div><div><div>It seems like this is an issue of multiple clients taking to ipmid. In the middle of writing... There is another client that is reading or something</div><div>else and will reset the fruCache as you mentioned. In that case, I think it may be best to refactor it out to use another getFru method... maybe like getFruToWrite</div><div><br></div><div>Maybe just something like this</div><div><br></div><div>```</div><div>std::vector<uint8_t> getFruToWrite(...){</div><div> if (writeTimer->isRunning()) {</div><div> return fruCacheForWrite;</div><div> }</div><div> auto [_, fruCacheForWrite] = getFru(...);</div><div> return fruCacheForWrite;</div><div>}</div><div>```</div><div><br></div><div>Also need to change `writeFruCache` and such to use `fruCacheForWrite` and such.</div></div><div><br></div><div>Please let me know if that works for you. Feel free to reach out internally for faster feedback.</div><div><br></div><div>Willy Tu</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, May 2, 2024 at 11:32 AM Oskar Senft <<a href="mailto:osk@google.com" target="_blank">osk@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi everyone<br>
<br>
tl;dr - Attempting "ipmitool fru write" with an input file that<br>
contains additional bytes beyond the actual FRU data does not actually<br>
update the FRU on OpenBMC at head w/ entity-manager.<br>
<br>
Details:<br>
<br>
I ran into an issue with updating the "baseboard" FRU (0), which is<br>
stored as /etc/fru/baseboard.fru.bin. However, I don't think this is<br>
specific to FRU 0 and could apply to other updateable FRUs in the same<br>
way.<br>
<br>
<br>
1. Start off with a "minimal" /etc/fru/baseboard.fru.bin which just<br>
contains chassis type (that's required for entity-manager's fru-device<br>
to recognize the file.<br>
<br>
root@akita:~# ls -al /etc/fru/baseboard.fru.bin<br>
-rw-r--r-- 1 root root 512 Sep 21 05:21<br>
/etc/fru/baseboard.fru.bin<br>
<br>
root@akita:~# hexdump -C /etc/fru/baseboard.fru.bin<br>
00000000 01 00 01 00 00 00 00 fe 01 01 17 c0 c0 c1 00 a6 |................|<br>
00000010 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|<br>
*<br>
00000200<br>
<br>
root@akita:~# ipmitool fru print 0<br>
Chassis Type : Rack Mount Chassis<br>
Chassis Area Checksum : OK<br>
<br>
<br>
2. Prepare a FRU file with additional data, e.g. with frugy:<br>
<br>
(frugy) osk@osk:~$ cat demo.yml<br>
ChassisInfo:<br>
type: 23<br>
part_number: '4711'<br>
serial_number: '12345'<br>
<br>
(frugy) osk@osk:~$ frugy demo.yml -o demo.bin<br>
<br>
(frugy) osk@osk:~$ ls -al demo.bin<br>
-rw-r----- 1 osk primarygroup 24 May 2 13:51 demo.bin<br>
<br>
(frugy) osk@osk:~$ hexdump -C demo.bin<br>
00000000 01 00 01 00 00 00 00 fe 01 02 17 c4 34 37 31 31 |............4711|<br>
00000010 c5 31 32 33 34 35 c1 d0 |.12345..|<br>
00000018<br>
<br>
Note that frugy generates a minimal binary by default. However, other<br>
processes may not and instead produce a fixed size binary blob. This<br>
happens, e.g. when performing "ipmitool fru read" which returns the<br>
whole contents of the underlying storage. A subsequent "ipmitool fru<br>
write" with that blob will fail as explained here.<br>
<br>
To simulate this here, increase the file to 256 bytes:<br>
<br>
(frugy) osk@osk:~$ cp demo.bin demo-256.bin<br>
(frugy) osk@osk:~$ dd if=/dev/zero bs=1 seek=256 count=0 of=demo-256.bin<br>
0+0 records in<br>
0+0 records out<br>
0 bytes copied, 5.1138e-05 s, 0.0 kB/s<br>
<br>
(frugy) osk@osk:~$ ls -al demo-256.bin<br>
-rw-r----- 1 osk primarygroup 256 May 2 13:57 demo-256.bin<br>
<br>
(frugy) osk@osk:~$ hexdump -C demo-256.bin<br>
00000000 01 00 01 00 00 00 00 fe 01 02 17 c4 34 37 31 31 |............4711|<br>
00000010 c5 31 32 33 34 35 c1 d0 00 00 00 00 00 00 00 00 |.12345..........|<br>
00000020 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|<br>
*<br>
00000100<br>
<br>
<br>
3. Write the newly generated FRU:<br>
<br>
root@akita:~# ipmitool fru write 0 demo-256.bin<br>
Fru Size : 512 bytes<br>
Size to Write : 256 bytes<br>
<br>
Wait ~10 seconds for the fru-device process to reload the contents.<br>
<br>
<br>
4. Re-read the FRU contents:<br>
<br>
root@akita:~# ipmitool fru print 0<br>
Chassis Type : Rack Mount Chassis<br>
Chassis Area Checksum : OK<br>
<br>
<br>
5. For comparison, write only the minimal FRU:<br>
<br>
root@akita:~# ipmitool fru write 0 demo.bin<br>
Fru Size : 512 bytes<br>
Size to Write : 24 bytes<br>
<br>
Wait ~10 seconds.<br>
<br>
root@akita:~# ipmitool fru print 0<br>
Chassis Type : Rack Mount Chassis<br>
Chassis Part Number : 4711<br>
Chassis Serial : 12345<br>
Chassis Area Checksum : OK<br>
<br>
<br>
I dug into this and found that this is caused by an interaction<br>
between <a href="https://github.com/openbmc/phosphor-host-ipmid/blob/master/dbus-sdr/storagecommands.cpp" rel="noreferrer" target="_blank">https://github.com/openbmc/phosphor-host-ipmid/blob/master/dbus-sdr/storagecommands.cpp</a><br>
and <a href="https://github.com/openbmc/entity-manager/blob/master/src/fru_device.cpp" rel="noreferrer" target="_blank">https://github.com/openbmc/entity-manager/blob/master/src/fru_device.cpp</a>.<br>
<br>
Here's what happens:<br>
- The "ipmitool fru write" request is handled by storagecommands.cpp<br>
ipmiStorageWriteFruData.<br>
<br>
- ipmiStorageWriteFruData receives the whole FRU blob from IPMI across<br>
several calls. On the initial call, it requests the current FRU blob<br>
via D-bus from fru-device and caches it in the process. It then<br>
overwrites sections of that cache with the received data from IPMI.<br>
<br>
- ipmiStorageWriteFruData uses information from the FRU header to<br>
check whether it received all the bytes that make up the new FRU. Note<br>
that this could be fewer bytes than the size of the cached blob. Once<br>
it receives all the bytes for the new FRU, it calls<br>
/xyz/openbmc_project/FruDevice WriteFru via D-Bus with the full FRU<br>
blob (i.e. the full cache with modifications on top). After that the<br>
cache is cleared.<br>
<br>
- The D-Bus call to WriteFru is handled by fru_device.cpp writeFRU. It<br>
first performs a sanity check and then writes the blob to the<br>
underlying device (or the /etc/fru/baseboard.fru.bin for FRU 0). It<br>
subsequently calls rescanBusses() which actually executes after about<br>
1 second.<br>
<br>
- Meanwhile, "ipmitool fru write" continues to happily send additional<br>
bytes to ipmiStorageWriteFruData (as in step #3 above). Since its<br>
cache has just been cleared, it retrieves the current FRU from<br>
fru-device again. However, since that has not yet completed<br>
"rescanBusses", it actually received the original FRU again, not the<br>
modified version. The above cycle repeats, but this time the original<br>
FRU with additional modifications from the additional bytes.<br>
<br>
In the best case (if the new FRU data is larger than the original FRU<br>
data), the result is that the FRU did not get updated at all, since we<br>
effectively just wrote back the original FRU. However, if the new FRU<br>
data is smaller than the original FRU data, this may result in<br>
corrupted FRU data to be persisted.<br>
<br>
<br>
I was trying to figure out how to best fix that, but couldn't come up<br>
with a design that would still work. Some ideas:<br>
<br>
A) I think what we're missing is feedback from fru-device to ipmid<br>
that the FRU write operation has actually completed and the FRU data<br>
was re-read. The ipmid should probably not accept any additional write<br>
requests until the previous write request has completed and the new<br>
FRU data is available.<br>
<br>
B) If ipmiStorageWriteFruData cannot detect the expected size of the<br>
FRU data, it relies on a timeout to eventually write the blob if no<br>
more data was received from IPMI. I wonder if this mechanism is "too<br>
clever" and we should simply always just wait for the timeout?<br>
<br>
C) ipmiStorageWriteFruData clears the cache immediately after WriteFru<br>
was called. Maybe we should keep that cache around until the timeout<br>
expires?<br>
<br>
Thoughts?<br>
<br>
Thanks<br>
Oskar.<br>
</blockquote></div>
</blockquote></div>
</blockquote></div>
</blockquote></div>
</blockquote></div>
</blockquote></div>