netdev - RE: [RFC PATCH net-next 06/19] pds_core: add FW update feature to devlink

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CO1PR11MB508942BE965E63893DE9B86AD6129@CO1PR11MB5089.namprd11.prod.outlook.com>
Date:   Tue, 29 Nov 2022 00:13:44 +0000
From:   "Keller, Jacob E" <jacob.e.keller@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>, Shannon Nelson <shnelson@....com>
CC:     Shannon Nelson <snelson@...sando.io>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "mst@...hat.com" <mst@...hat.com>,
        "jasowang@...hat.com" <jasowang@...hat.com>,
        "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>,
        "drivers@...sando.io" <drivers@...sando.io>
Subject: RE: [RFC PATCH net-next 06/19] pds_core: add FW update feature to
 devlink



> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>
> Sent: Monday, November 28, 2022 3:33 PM
> To: Shannon Nelson <shnelson@....com>
> Cc: Shannon Nelson <snelson@...sando.io>; netdev@...r.kernel.org;
> davem@...emloft.net; mst@...hat.com; jasowang@...hat.com;
> virtualization@...ts.linux-foundation.org; drivers@...sando.io; Keller, Jacob E
> <jacob.e.keller@...el.com>
> Subject: Re: [RFC PATCH net-next 06/19] pds_core: add FW update feature to
> devlink
> 
> On Mon, 28 Nov 2022 14:25:46 -0800 Shannon Nelson wrote:
> > I don't think Intel selects which FW image to boot, but it looks like
> > mlxsw and nfp use the PARAM_GENERIC_FW_LOAD_POLICY to select between
> > DRIVER, FLASH, or DISK.  Shall I add a couple of generic SLOT_x items to
> > the enum devlink_param_fw_load_policy_value and use this API?  For example:
> >
> > 	DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_0,
> > 	DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_1,
> > 	DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_2,
> > 	DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_3,
> 
> Not the worst idea, although I presume normal FW flashing should switch
> between slots to activate the new image by default? Which means the
> action of fw flashing would alter the policy set by the user. A little
> awkward from an API purist standpoint.
> 
> I'd just expose the active "bank" via netlink directly.
> 
> > I could then modify the devlink dev info printed to refer to fw_slot_0,
> > fw.slot_1, and fw.slot_2 instead of our vendor specific names.
> 
> Jake, didn't you have a similar capability in ice?
> 

We have two banks of flash, the active bank, and an inactive bank used for updates. We can determine the active bank from the Shadow RAM contents which are generated as the EMP firmware boots up.

> Knowing my memory I may have acquiesced to something in another driver
> already. That said - I think it's cleaner if we just list the stored
> versions per bank, no?

I think it would make sense to store them per bank and make the bank number some index instead of something separate as like this DEVLINK_PARAM_FW_LOAD_POLICY_VALUE_SLOT_<X> where each <X> makes a separate parameter.

Currently devlink info reports "stored" and "active", which aligns with our current use of the active vs inactive flash bank. We could be explicit and indicate which bank it is, though its a bit tricky since most of the firmware interface deals with it in terms of "active" and "inactive" rather than the absolute position of "bank 0 or bank 1".

Especially if another device has more than 2 banks I think its a good extension to devlink info, and we could probably get away with something like a new info attribute that specifies which bank index it is, and an attribute which indicates whether its active or not.

Thanks,
Jake