<div dir="ltr">So the prototype change implementing support for multiple flash devices in skiboot is pretty ugly right now since we didn't cleanly separate one time mailbox init features from the individual flash init functionality. It works and meets the protocol spec, but it needs to be rewritten on top of the current master / your patchset if we want something clean. My understanding is that our current mboxd implementation is equally hacky and will need to be rewritten too. I can start working on the skiboot side now.<div><br></div><div>My understanding given how we implemented our changes is that the skiboot side would work fine without actually supporting the multiple flash. All of the commands that take a flash_id had it appended to the end of the V2 argument list and would pass 0 since the mailbox message is always zeroed out. For backward compatibility we would always have the BIOS flash at id 0 anyway and everything would work fine.</div><div><br></div><div>MBOXd would be trickier since it would have to support multiple flashes when advertising V3.</div></div><br><div class="gmail_quote"><div dir="ltr">On Thu, Jun 29, 2017 at 4:04 PM Cyril Bur <<a href="mailto:cyril.bur@au1.ibm.com">cyril.bur@au1.ibm.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Thu, 2017-06-29 at 16:05 +0000, William Kennington wrote:<br>
> This is somewhat tangential to the patch, but we haven't seen any tracking for the effort to define a V3 protocol and already started making our own changes to accommodate our own needs for the mailbox daemon. I wrote up our initial proposal for the changes to the protocol. So far it is just a simple addition of the ability to use multiple flash devices.<br>
> <a href="https://github.com/wak-google/mboxbridge/commit/e9b7c482fcff5278ee1ad46ecd8d6240d5b8886e" rel="noreferrer" target="_blank">https://github.com/wak-google/mboxbridge/commit/e9b7c482fcff5278ee1ad46ecd8d6240d5b8886e</a><br>
<br>
Thanks for letting us know. Admittedly Suraj and I did silo this a bit<br>
- although I'm not sure where we should all agree to announce/discuss<br>
ideas and proposals, something to think about.<br>
<br>
><br>
> We already have prototype skiboot and mboxd code implementing V3. We would like to get this into the current proposal for V3 but it is clearly late in the cycle and could jump on an iteration for V4.<br>
<br>
>From skiboot point of view it doesn't need to always use all the<br>
features of the version it declares to know. So if you sent your work<br>
to apply on top of this series I don't see a problem.<br>
<br>
mboxd may not be as smooth, I suppose both Surajs work and your work<br>
could go in separately but only once its all merged mboxd would expose<br>
v3.<br>
<br>
Most of the discussion would have to be around your new command, from<br>
eyeballing the link, I don't see any problems.<br>
<br>
I suppose the question is how ready are you to start upstreaming?<br>
<br>
Thanks,<br>
<br>
Cyril<br>
<br>
><br>
> On Thu, Jun 29, 2017 at 5:50 AM Cyril Bur <<a href="mailto:cyril.bur@au1.ibm.com" target="_blank">cyril.bur@au1.ibm.com</a>> wrote:<br>
> > Hello,<br>
> ><br>
> > Firstly, let me apologise for the amount of rework in this series.<br>
> > Ultimately the goal here was really the last three patches.<br>
> > Unfortunately in the process of writing them I started thinking about<br>
> > some of the problems in V2.<br>
> ><br>
> > The easiest to address is a bug that came up in testing V3 which is<br>
> > that if opening a window fails, skiboot will assume that the previously<br>
> > opened window is still valid this is incorrect. Patch 2 address this<br>
> > problem.<br>
> ><br>
> > Then I got to thinking about errors and timeouts. It turns out that<br>
> > currently if mbox-flash decides a message times out, it will not be<br>
> > able to send anymore due to the lpc-mbox driver needing a response for<br>
> > every message it sent out - patch 4 deals with this. However after<br>
> > writing a significantly less invasive fix I realised that the entire<br>
> > handling of timeouts/retries/resends/late responses was racey. So a<br>
> > much simpler version of patch 4 became patches 3-6.<br>
> ><br>
> > Then there was some extra V2 stuff that wasn't necessary but is nice<br>
> > to have - patch 7.<br>
> ><br>
> > Then on to actually implementing V3 - at the time of writing V2 Suraj<br>
> > did suggest what has become patch 8. This actually makes the V3 patch<br>
> > - patch 9 very easy.<br>
> ><br>
> > The final patch is an extended, reworked and even more improved<br>
> > version of the tests I sent with V2 (which weren't merged at the time)<br>
> ><br>
> > One final note: this series passes all the tests I can throw at it<br>
> > however I do not have reliable access to a MBOX V3 Daemon running on<br>
> > real hardware. Suraj has been kind enough to use (very) similar<br>
> > versions of this series in his testing and he informs me that there<br>
> > are no problems, however this exact code has never run on real<br>
> > hardware.<br>
> ><br>
> ><br>
> > Thanks,<br>
> ><br>
> > Cyril<br>
> ><br>
> > Cyril Bur (11):<br>
> > libflash/mbox-flash: Add v2 error codes<br>
> > libflash/mbox-flash: Always close windows before opening a new window<br>
> > libflash/mbox-flash: Move sequence handling to driver level<br>
> > libflash/mbox-flash: Allow mbox-flash to tell the driver msg timeouts<br>
> > hw/lpc-mbox: Simplify message bookkeeping and timeouts<br>
> > libflash/mbox-flash: Simplify message sending<br>
> > libflash/mbox-flash: Use BMC suggested timeout value<br>
> > libflash/mbox-flash: Use static arrays of function pointers<br>
> > libflash/mbox-flash: Understand v3<br>
> > libflash/mbox-flash: Add the ability to lock flash<br>
> > libflash/test: Add tests for mbox-flash<br>
> ><br>
> > .gitignore | 1 +<br>
> > hw/lpc-mbox.c | 53 +++--<br>
> > include/lpc-mbox.h | 9 +-<br>
> > include/skiboot.h | 2 +<br>
> > libflash/mbox-flash.c | 404 +++++++++++++++++++++--------------<br>
> > libflash/mbox-flash.h | 1 +<br>
> > libflash/test/Makefile.check | 17 +-<br>
> > libflash/test/mbox-server.c | 499 +++++++++++++++++++++++++++++++++++++++++++<br>
> > libflash/test/mbox-server.h | 10 +<br>
> > libflash/test/stubs.c | 78 ++++++-<br>
> > libflash/test/stubs.h | 27 +++<br>
> > libflash/test/test-mbox.c | 341 +++++++++++++++++++++++++++++<br>
> > 12 files changed, 1254 insertions(+), 188 deletions(-)<br>
> > create mode 100644 libflash/test/mbox-server.c<br>
> > create mode 100644 libflash/test/mbox-server.h<br>
> > create mode 100644 libflash/test/stubs.h<br>
> > create mode 100644 libflash/test/test-mbox.c<br>
> ><br>
> > --<br>
> > 2.13.2<br>
> ><br>
> > _______________________________________________<br>
> > Skiboot mailing list<br>
> > <a href="mailto:Skiboot@lists.ozlabs.org" target="_blank">Skiboot@lists.ozlabs.org</a><br>
> > <a href="https://lists.ozlabs.org/listinfo/skiboot" rel="noreferrer" target="_blank">https://lists.ozlabs.org/listinfo/skiboot</a><br>
<br>
</blockquote></div>