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: <CO1PR11MB508985697264AF426CF01AA7D6129@CO1PR11MB5089.namprd11.prod.outlook.com>
Date:   Tue, 29 Nov 2022 00:18:52 +0000
From:   "Keller, Jacob E" <jacob.e.keller@...el.com>
To:     Shannon Nelson <shnelson@....com>, Jakub Kicinski <kuba@...nel.org>
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: Shannon Nelson <shnelson@....com>
> Sent: Monday, November 28, 2022 3:46 PM
> To: Jakub Kicinski <kuba@...nel.org>
> 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 11/28/22 3:33 PM, Jakub Kicinski wrote:
> > 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.

This could potentially be handled by having DELVINK_PARAM_FW_LOAD_POLICY_FLASH be the automatic "select best version", and if a user has set a manual value then don't allow flashing until a reboot or the value is set back to POLICY_FLASH?

> 
> Yes, the action of flashing will set the new bank/slot to use on the
> next boot.  However, we have the ability to select from multiple valid
> images and we want to pass this flexibility to the user rather than
> force them to go through a whole flash sequence just to get to the other
> bank.
> 
> >
> > 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?
> >
> > 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?
> 
> We are listing the stored images in the devlink dev info output, just
> want to let the user choose which of those valid images to use next.
> 
> Cheers,
> sln

Technically I think we could do something similar in ice to switch between the banks, at least as long as there is a valid image in the bank. The big trick is that I am not sure we can verify ahead of time whether we have a valid image and if you happen to boot into an invalid or blank image. There is some recovery firmware that should activate in that case, but I think our current driver doesn't implement enough of a recovery mode to actually handle this case to allow user to switch back.

Still, I think the ability to select the bank is valuable, and finding the right way to expose it is good.

Thanks,
Jake