From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1ACB3C5ACC4 for ; Thu, 20 Feb 2020 00:06:36 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id E5DA62465D for ; Thu, 20 Feb 2020 00:06:35 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726949AbgBTAGf (ORCPT ); Wed, 19 Feb 2020 19:06:35 -0500 Received: from mga06.intel.com ([134.134.136.31]:62460 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726647AbgBTAGe (ORCPT ); Wed, 19 Feb 2020 19:06:34 -0500 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 19 Feb 2020 16:06:34 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,462,1574150400"; d="scan'208";a="382965389" Received: from jekeller-mobl1.amr.corp.intel.com (HELO [134.134.177.86]) ([134.134.177.86]) by orsmga004.jf.intel.com with ESMTP; 19 Feb 2020 16:06:34 -0800 Subject: Re: [RFC PATCH v2 06/22] ice: add basic handler for devlink .info_get To: Jakub Kicinski Cc: netdev@vger.kernel.org, jiri@resnulli.us, valex@mellanox.com, linyunsheng@huawei.com, lihong.yang@intel.com References: <20200214232223.3442651-1-jacob.e.keller@intel.com> <20200214232223.3442651-7-jacob.e.keller@intel.com> <20200218184552.7077647b@kicinski-fedora-PC1C0HJN> <6b4dd025-bcf8-12de-99b0-1e05e16333e8@intel.com> <20200219115757.5af395c5@kicinski-fedora-PC1C0HJN> <70001e87-b369-bab4-f318-ad4514e7dcfb@intel.com> <20200219154727.5b52aa73@kicinski-fedora-PC1C0HJN> From: Jacob Keller Organization: Intel Corporation Message-ID: <1dd1cff3-dbbf-9b04-663d-15c7e4e5a3bd@intel.com> Date: Wed, 19 Feb 2020 16:06:34 -0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 MIME-Version: 1.0 In-Reply-To: <20200219154727.5b52aa73@kicinski-fedora-PC1C0HJN> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On 2/19/2020 3:47 PM, Jakub Kicinski wrote: > On Wed, 19 Feb 2020 13:37:50 -0800 Jacob Keller wrote: >> Jakub, >> >> Thanks for your excellent feedback. >> >> On 2/19/2020 11:57 AM, Jakub Kicinski wrote: >>> On Wed, 19 Feb 2020 09:33:09 -0800 Jacob Keller wrote: >>>> "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? >>> >> >> I'm actually not sure if we have any way to identify the parameters. >> I'll ask around about that. My understanding is that these would include >> parameters that can be modified by the driver such as Wake on LAN >> settings, so I'm also not sure if they'd be covered in the fw.bundle >> either. The 'defaults' that were selected when the image was created >> would be covered, but changes to them wouldn't update the value. >> >> Hmmmmm. > > Ah, so these are just defaults, then if there's no existing version > I wouldn't worry. > Ok. >>>> 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? >> >> There's a version of this within the flash, but it is limited, and many >> device features get disabled if you don't load the DDP package file. >> (You may have seen patches for this for implementing "safe mode"). >> >> My understanding is there is no mechanism for persisting a different DDP >> to the flash. > > I see, so this really isn't just parser extensions. > Right. I'm not entirely sure what pieces of logic the contents interact with. > I'm a little surprised you guys went this way, loading FW from disk > becomes painful for network boot and provisioning :S All the first > stage images must have it built in, which is surprisingly painful. > Right. I don't have the context for why this was chosen over making it a portion that can be updated independently. Unfortunately I don't think it's a decision that can be changed, at least not easily. > Perhaps the "safe mode" FW is enough to boot, but then I guess once > real FW is available there may be a loss of link as the device resets? > it's enough to boot up and handle basic functionality. I'm not sure exactly how it would be handled in regards to device reset. >>> Does it not have some fun implications for firmware signing to have >>> part of the config/ucode loaded from the host? >> >> I'm not sure how it works exactly. As far as I know, the DDP file is >> itself signed. > > Right, that'd make sense :) > >>> IIRC you could also load multiple of those DDP packages? Perhaps they >>> could get names like fw.app0, fw.app1, etc? >> >> You can load different ones, each has their own version and name >> embedded. However, only one can be loaded at any given time, so I'm not >> sure if multiples like this make sense. > > I see. Maybe just fw.app works then.. > Ok >>> Also if DDP controls a >>> particular part of the datapath (parser?) feel free to come up with a >>> more targeted name, up to you. >> >> Right, it's my understanding that this defines the parsing logic, and >> not the complete datapath microcode. >> >> In theory, there could be at least 3 DDP versions >> >> 1) the version in the NVM, which would be the very basic "safe mode" >> compatible one. >> >> 2) the version in the ddp firmware file that we search for when we load >> >> 3) the one that actually got activated. It's a sort of >> first-come-first-serve and sticks around until a device global reset. >> This should in theory always be the same as (2) unless you do something >> weird like load different drivers on the multiple functions. >> >> I suppose we could use "running" and "stored" for this, to have "stored" >> be what's in the NVM, and "running" for the active one.. but that's ugly >> and misusing what stored vs running is supposed to represent. > > Ouff. Having something loaded from disk breaks the running vs stored > comparison :( But I think Dave was pretty clear on his opinion about > load FW from disk and interpret it in the kernel to extract the version. >> Can we leave stored meaning "stored on the device" and running being > loaded on the chip? > Yes. > It's perfectly fine for a component to only be reported in running and > not stored, nfp already does that: > Right, that is my plan. I'll probably just display only the running DDP fields. If it turns out displaying the NVM default values makes sense then it could be done through another field something like "fw.app.default" or similar. Not convinced yet that it will be necessary or useful. Even in regards to other versions, I'm not entirely sure if I can read the values for what's "stored" vs what is running anyways. > https://elixir.bootlin.com/linux/v5.6-rc1/source/drivers/net/ethernet/netronome/nfp/nfp_devlink.c#L238 > Thanks, Jake