linux-kernel - Re: [PATCH net-next v2 01/14] sfc: add function personality support for EF100 devices

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]

Date:   Wed, 15 Mar 2023 13:11:23 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     Gautam Dawar <gautam.dawar@....com>, linux-net-drivers@....com,
        Edward Cree <ecree.xilinx@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Richard Cochran <richardcochran@...il.com>,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        eperezma@...hat.com, harpreet.anand@....com, tanuj.kamde@....com,
        koushik.dutta@....com
Subject: Re: [PATCH net-next v2 01/14] sfc: add function personality support
 for EF100 devices


在 2023/3/13 19:50, Martin Habets 写道:
> On Fri, Mar 10, 2023 at 01:04:14PM +0800, Jason Wang wrote:
>> On Tue, Mar 7, 2023 at 7:36 PM Gautam Dawar <gautam.dawar@....com> wrote:
>>> A function personality defines the location and semantics of
>>> registers in the BAR. EF100 NICs allow different personalities
>>> of a PCIe function and changing it at run-time. A total of three
>>> function personalities are defined as of now: EF100, vDPA and
>>> None with EF100 being the default.
>>> For now, vDPA net devices can be created on a EF100 virtual
>>> function and the VF personality will be changed to vDPA in the
>>> process.
>>>
>>> Co-developed-by: Martin Habets <habetsm.xilinx@...il.com>
>>> Signed-off-by: Martin Habets <habetsm.xilinx@...il.com>
>>> Signed-off-by: Gautam Dawar <gautam.dawar@....com>
>>> ---
>>>   drivers/net/ethernet/sfc/ef100.c     |  6 +-
>>>   drivers/net/ethernet/sfc/ef100_nic.c | 98 +++++++++++++++++++++++++++-
>>>   drivers/net/ethernet/sfc/ef100_nic.h | 11 ++++
>>>   3 files changed, 111 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/sfc/ef100.c b/drivers/net/ethernet/sfc/ef100.c
>>> index 71aab3d0480f..c1c69783db7b 100644
>>> --- a/drivers/net/ethernet/sfc/ef100.c
>>> +++ b/drivers/net/ethernet/sfc/ef100.c
>>> @@ -429,8 +429,7 @@ static void ef100_pci_remove(struct pci_dev *pci_dev)
>>>          if (!efx)
>>>                  return;
>>>
>>> -       probe_data = container_of(efx, struct efx_probe_data, efx);
>>> -       ef100_remove_netdev(probe_data);
>>> +       efx_ef100_set_bar_config(efx, EF100_BAR_CONFIG_NONE);
>>>   #ifdef CONFIG_SFC_SRIOV
>>>          efx_fini_struct_tc(efx);
>>>   #endif
>>> @@ -443,6 +442,7 @@ static void ef100_pci_remove(struct pci_dev *pci_dev)
>>>          pci_disable_pcie_error_reporting(pci_dev);
>>>
>>>          pci_set_drvdata(pci_dev, NULL);
>>> +       probe_data = container_of(efx, struct efx_probe_data, efx);
>>>          efx_fini_struct(efx);
>>>          kfree(probe_data);
>>>   };
>>> @@ -508,7 +508,7 @@ static int ef100_pci_probe(struct pci_dev *pci_dev,
>>>                  goto fail;
>>>
>>>          efx->state = STATE_PROBED;
>>> -       rc = ef100_probe_netdev(probe_data);
>>> +       rc = efx_ef100_set_bar_config(efx, EF100_BAR_CONFIG_EF100);
>>>          if (rc)
>>>                  goto fail;
>>>
>>> diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c
>>> index 4dc643b0d2db..8cbe5e0f4bdf 100644
>>> --- a/drivers/net/ethernet/sfc/ef100_nic.c
>>> +++ b/drivers/net/ethernet/sfc/ef100_nic.c
>>> @@ -772,6 +772,99 @@ static int efx_ef100_get_base_mport(struct efx_nic *efx)
>>>          return 0;
>>>   }
>>>
>>> +/* BAR configuration.
>>> + * To change BAR configuration, tear down the current configuration (which
>>> + * leaves the hardware in the PROBED state), and then initialise the new
>>> + * BAR state.
>>> + */
>>> +struct ef100_bar_config_ops {
>>> +       int (*init)(struct efx_probe_data *probe_data);
>>> +       void (*fini)(struct efx_probe_data *probe_data);
>>> +};
>>> +
>>> +static const struct ef100_bar_config_ops bar_config_ops[] = {
>>> +       [EF100_BAR_CONFIG_EF100] = {
>>> +               .init = ef100_probe_netdev,
>>> +               .fini = ef100_remove_netdev
>>> +       },
>>> +#ifdef CONFIG_SFC_VDPA
>>> +       [EF100_BAR_CONFIG_VDPA] = {
>>> +               .init = NULL,
>>> +               .fini = NULL
>>> +       },
>>> +#endif
>>> +       [EF100_BAR_CONFIG_NONE] = {
>>> +               .init = NULL,
>>> +               .fini = NULL
>>> +       },
>>> +};
>> This looks more like a mini bus implementation. I wonder if we can
>> reuse an auxiliary bus here which is more user friendly for management
>> tools.
> When we were in the design phase of vDPA for EF100 it was still called
> virtbus, and the virtbus discussion was in full swing at that time.
> We could not afford to add risk to the project by depending on it, as
> it might not have been merged at all.


Right.


> If we were doing the same design now I would definitely consider using
> the auxiliary bus.
>
> Martin


But it's not late to do the change now. Auxiliary bus has been used by a 
lot of devices (even with vDPA device). The change looks not too 
complicated.

This looks more scalable and convenient for management layer.

Thanks