Re: [RFC PATCH v2 06/22] ice: add basic handler for devlink .info_get

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 19 Feb 2020 09:33:09 -0800 Jacob Keller wrote:
> >  - the PSID stuff was added, which IIUC is either (a) an identifier 
> >    for configuration parameters which are not visible via normal Linux
> >    tools, or (b) a way for an OEM to label a product.
> >    This changes where this thing should reside because we don't expect
> >    OEM to relabel the product/SKU (or do we?) and hence it's a fixed
> >    version.
> >    If it's an identifier for random parameters of the board (serdes
> >    params, temperature info, IDK) which is expected to maybe be updated
> >    or tuned it should be in running/stored.
> >   
> 
> Hmm. In my case nvm.psid is basically describing the format of the NVM
> parameter set, but I don't think it actually covers the contents. This
> version can update if you update to a newer image.
> 
> I probably need to re-word the versions to be "fw.bundle" and "fw.psid",
> rather than using "nvm", given how you're describing the fields above.
> 
> >    So any further info on what's an EETRACK in your case?
> >   
> 
> EETRACK is basically the name we used for "bundle", as it is a unique
> identifier generated when new images are prepared.
> 
> I think this should probably just become "fw.bundle".

Okay, cool!

> What I have now as "fw.mgmt.bundle" is a little different. It's
> basically a unique identifier obtained from the build system of the
> management firmware that can be used to identify exactly what got built
> for that firmware. (i.e. it would change even if the developers failed
> to update their version number).
> 
> >    For MLX there's bunch of documents which tell us how we can create 
> >    an ini file with parameters, but no info on what those parameters
> >    actually are. 
> > 
> >    Jiri would you be able to help? Please chime in..
> > 
> > 
> > Sorry for the painful review process, it's quite hard to review what
> > people are doing without knowing the back end. Hopefully above gives
> > you an idea of the intentions when this code was added :)
> >   
> 
> I understand the difficulty.
> 
> > I see that the next patch adds a 'fixed' version, so if that's
> > sufficient to identify your board there isn't any blocker here.  
> 
> Yes, the board.id is the unique identifier of the physical board design.
> It's what we've called the Product Board Assembly identifier.
> 
> > 
> > What I'd still like to consider is:
> >  - if fw.mgmt.bundle needs to be a bundle if it doesn't bundle multiple
> >    things? If it's hard to inject the build ID into the fw.mgmt version
> >    that's fine.  
> 
> I mostly didn't like having it as part of the same version because it is
> somewhat distinct. I don't think it's a "bundle" in the sense of what
> you're describing.
> 
> It is basically just an identifier from the build system of that
> component and will be changed even if the developer did not update the
> firmware version. It's useful primarily to identify precisely where that
> build of the firmware binary came from. (Hence why I originally used
> ".build").

Okay.

> >  - fw.undi.orom - do we need to say orom? Is there anything else than
> >    orom for UNDI in the flash?  
> 
> Hmm.. I'll double check this. I wasn't entirely sure if we had other
> components which is why I went that route. I think you're right though
> and this can just be "fw.undi".
> 
> >  - nvm.psid may perhaps be better as nvm.psid.api? Following your
> >    fw.mgmt.api?  
> 
> Hmm. Yea this isn't really a parameter set id, but more of describing
> the format. I am not sure I fully understand it myself yet.
> 
> >  - nvm.bundle - eetrack sounds mode like a stream, so perhaps this is
> >    the PSID?
> >   
> 
> So, I think this should probably become "fw.bundle", and I can drop the
> nvm bits altogether. The EETRACK id is a unique identifier we create
> when new images are created. If you have the eetrack you can look up
> data on the source binary that the NVM image came from.
> 
> It wouldn't cover the parameters that can be changed, so I don't think
> it's a psid.
> 
> 
> Given this discussion, here is what I have so far:
> 
> "fw.bundle" -> What was "nvm.bundle", the identifier for the combined fw
> image. This would be our EETRACK id.

👍

> "fw.mgmt" -> The management firmware 3 digit version

👍

> "fw.mgmt.api" -> The version of API exposed by this firmware

👍

> "fw.mgmt.build" -> The build identifier. I really do think this should
> be ".build" rather than .bundle, as it's definitely not a bundle in the
> same sense. I *could* simply make "fw.mgmt" be "maj.min.patch build" but
> I think it makes sense as its own field.

okay

> "fw.undi" -> Version of the Option ROM containing the UEFI driver

👍

> "fw.psid.api" -> what was the "nvm.psid". This I think needs a bit more
> work to define. It makes sense to me as some sort of "api" as (if I
> understand it correctly) it is the format for the parameters, but does
> not itself define the parameter contents.

Sounds good. So the contents of parameters would be covered by the
fw.bundle now and not have a separate version?

> The original reason for using "fw" and "nvm" was because we (internally)
> use fw to mean the management firmware.. where as these APIs really
> combine the blocks and use "fw.mgmt" for the management firmware. Thus I
> think it makes sense to move from
> 
> I also have a couple other oddities that need to be sorted out. We want
> to display the DDP version (piece of "firmware" that is loaded during
> driver load, and is not permanent to the NVM). In some sense this is our
> "fw.app", but because it's loaded by driver at load and not as
> permanently stored in the NVM... I'm not really sure that makes sense to
> put this as the "fw.app", since it is not updated or really touched by
> the firmware flash update.

Interesting, can DDP be persisted to the flash, though? Is there some
default DDP, or is it _never_ in the flash? 

Does it not have some fun implications for firmware signing to have
part of the config/ucode loaded from the host?

IIRC you could also load multiple of those DDP packages? Perhaps they
could get names like fw.app0, fw.app1, etc? Also if DDP controls a
particular part of the datapath (parser?) feel free to come up with a
more targeted name, up to you.

> Finally we also have a component we call the "netlist", which I'm still
> not fully up to speed on exactly what it represents, but it has multiple
> pieces of data including a 2-digit Major.Minor version of the base, a
> type field indicating the format, and a 2-digit revision field that is
> incremented on internal and external changes to the contents. Finally
> there is a hash that I think might *actually* be something like a psid
> or a bundle to uniquely represent this component. I haven't included
> this component yet because I'm still trying to grasp exactly what it
> represents and how best to describe each piece.

Hmm. netlist is a Si term, perhaps it's chip init data? nfp had
something called chip.init which I think loaded all the very low 
level Si configs.

My current guess is that psid is more of the serdes and maybe clock
data. 

Thinking about it now, it seems these versions mirror the company
structure. chip.init comes from the Si team. psid comes from the 
board design guys. fw.mgmt comes from the BSP/FW team.

None of them are really fixed but the frequency of changes increases
from chip.init changing very rarely to mgmt fw having a regular release
cadence.