Date:   Tue, 14 Mar 2023 08:38:51 +0000
From:   Martin Habets <habetsm.xilinx@gmail.com>
To:     Gautam Dawar <gdawar@amd.com>
Cc:     Jason Wang <jasowang@redhat.com>,
        Gautam Dawar <gautam.dawar@amd.com>, linux-net-drivers@amd.com,
        Edward Cree <ecree.xilinx@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Richard Cochran <richardcochran@gmail.com>,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        eperezma@redhat.com, harpreet.anand@amd.com, tanuj.kamde@amd.com,
        koushik.dutta@amd.com
Subject: Re: [PATCH net-next v2 04/14] sfc: evaluate vdpa support based on FW
 capability CLIENT_CMD_VF_PROXY
Message-ID: <ZBAym05znKuFjJ3G@gmail.com>
Mail-Followup-To: Gautam Dawar <gdawar@amd.com>,
        Jason Wang <jasowang@redhat.com>,
        Gautam Dawar <gautam.dawar@amd.com>, linux-net-drivers@amd.com,
        Edward Cree <ecree.xilinx@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
        Richard Cochran <richardcochran@gmail.com>,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        eperezma@redhat.com, harpreet.anand@amd.com, tanuj.kamde@amd.com,
        koushik.dutta@amd.com
References: <20230307113621.64153-1-gautam.dawar@amd.com>
 <20230307113621.64153-5-gautam.dawar@amd.com>
 <CACGkMEvUhC3HfizpiM8zxMa2RwgkR=yLm-GDpY120_32aBmWFw@mail.gmail.com>
 <85ad74df-147b-8d27-dd39-cc9d828ada4b@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <85ad74df-147b-8d27-dd39-cc9d828ada4b@amd.com>
Precedence: bulk

On Mon, Mar 13, 2023 at 06:09:19PM +0530, Gautam Dawar wrote:
> 
> On 3/10/23 10:34, Jason Wang wrote:
> > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> > 
> > 
> > On Tue, Mar 7, 2023 at 7:37 PM Gautam Dawar <gautam.dawar@amd.com> wrote:
> > > Add and update vdpa_supported field to struct efx_nic to true if
> > > running Firmware supports CLIENT_CMD_VF_PROXY capability. This is
> > > required to ensure DMA isolation between MCDI command buffer and guest
> > > buffers.
> > > 
> > > Signed-off-by: Gautam Dawar <gautam.dawar@amd.com>
> > > ---
> > >   drivers/net/ethernet/sfc/ef100_netdev.c | 26 +++++++++++++++---
> > >   drivers/net/ethernet/sfc/ef100_nic.c    | 35 +++++++++----------------
> > >   drivers/net/ethernet/sfc/ef100_nic.h    |  6 +++--
> > >   drivers/net/ethernet/sfc/ef100_vdpa.h   |  5 ++--
> > >   4 files changed, 41 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/sfc/ef100_netdev.c b/drivers/net/ethernet/sfc/ef100_netdev.c
> > > index d916877b5a9a..5d93e870d9b7 100644
> > > --- a/drivers/net/ethernet/sfc/ef100_netdev.c
> > > +++ b/drivers/net/ethernet/sfc/ef100_netdev.c
> > > @@ -355,6 +355,28 @@ void ef100_remove_netdev(struct efx_probe_data *probe_data)
> > >          efx->state = STATE_PROBED;
> > >   }
> > > 
> > > +static void efx_ef100_update_tso_features(struct efx_nic *efx)
> > > +{
> > > +       struct ef100_nic_data *nic_data = efx->nic_data;
> > > +       struct net_device *net_dev = efx->net_dev;
> > > +       netdev_features_t tso;
> > > +
> > > +       if (!efx_ef100_has_cap(nic_data->datapath_caps2, TX_TSO_V3))
> > > +               return;
> > > +
> > > +       tso = NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_GSO_PARTIAL |
> > > +             NETIF_F_GSO_UDP_TUNNEL | NETIF_F_GSO_UDP_TUNNEL_CSUM |
> > > +             NETIF_F_GSO_GRE | NETIF_F_GSO_GRE_CSUM;
> > > +
> > > +       net_dev->features |= tso;
> > > +       net_dev->hw_features |= tso;
> > > +       net_dev->hw_enc_features |= tso;
> > > +       /* EF100 HW can only offload outer checksums if they are UDP,
> > > +        * so for GRE_CSUM we have to use GSO_PARTIAL.
> > > +        */
> > > +       net_dev->gso_partial_features |= NETIF_F_GSO_GRE_CSUM;
> > > +}
> > I don't see a direct relationship between vDPA and the TSO capability.
> > Is this an independent fix?
> This isn't actually fixing any issue. This a minor code refactoring that
> wraps-up updating of the TSO capabilities in a separate function for better
> readability.

There definity was an issue here: the vDPA code now needs access to the NIC
capabilities. For this is should use the efx_ef100_init_datapath_caps below,
but that was also doing this netdev specific stuff.
The solution is to split up efx_ef100_init_datapath_caps into a generic API
that vDPA can use, and this netdev specific API which should not be used by vDPA.

Gautam, you could explain this API split in the description.

Martin

> > 
> > > +
> > >   int ef100_probe_netdev(struct efx_probe_data *probe_data)
> > >   {
> > >          struct efx_nic *efx = &probe_data->efx;
> > > @@ -387,9 +409,7 @@ int ef100_probe_netdev(struct efx_probe_data *probe_data)
> > >                                 ESE_EF100_DP_GZ_TSO_MAX_HDR_NUM_SEGS_DEFAULT);
> > >          efx->mdio.dev = net_dev;
> > > 
> > > -       rc = efx_ef100_init_datapath_caps(efx);
> > > -       if (rc < 0)
> > > -               goto fail;
> > > +       efx_ef100_update_tso_features(efx);
> > > 
> > >          rc = ef100_phy_probe(efx);
> > >          if (rc)
> > > diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c
> > > index 8cbe5e0f4bdf..ef6e295efcf7 100644
> > > --- a/drivers/net/ethernet/sfc/ef100_nic.c
> > > +++ b/drivers/net/ethernet/sfc/ef100_nic.c
> > > @@ -161,7 +161,7 @@ int ef100_get_mac_address(struct efx_nic *efx, u8 *mac_address,
> > >          return 0;
> > >   }
> > > 
> > > -int efx_ef100_init_datapath_caps(struct efx_nic *efx)
> > > +static int efx_ef100_init_datapath_caps(struct efx_nic *efx)
> > >   {
> > >          MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_CAPABILITIES_V7_OUT_LEN);
> > >          struct ef100_nic_data *nic_data = efx->nic_data;
> > > @@ -197,25 +197,15 @@ int efx_ef100_init_datapath_caps(struct efx_nic *efx)
> > >          if (rc)
> > >                  return rc;
> > > 
> > > -       if (efx_ef100_has_cap(nic_data->datapath_caps2, TX_TSO_V3)) {
> > > -               struct net_device *net_dev = efx->net_dev;
> > > -               netdev_features_t tso = NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_GSO_PARTIAL |
> > > -                                       NETIF_F_GSO_UDP_TUNNEL | NETIF_F_GSO_UDP_TUNNEL_CSUM |
> > > -                                       NETIF_F_GSO_GRE | NETIF_F_GSO_GRE_CSUM;
> > > -
> > > -               net_dev->features |= tso;
> > > -               net_dev->hw_features |= tso;
> > > -               net_dev->hw_enc_features |= tso;
> > > -               /* EF100 HW can only offload outer checksums if they are UDP,
> > > -                * so for GRE_CSUM we have to use GSO_PARTIAL.
> > > -                */
> > > -               net_dev->gso_partial_features |= NETIF_F_GSO_GRE_CSUM;
> > > -       }
> > >          efx->num_mac_stats = MCDI_WORD(outbuf,
> > >                                         GET_CAPABILITIES_V4_OUT_MAC_STATS_NUM_STATS);
> > >          netif_dbg(efx, probe, efx->net_dev,
> > >                    "firmware reports num_mac_stats = %u\n",
> > >                    efx->num_mac_stats);
> > > +
> > > +       nic_data->vdpa_supported = efx_ef100_has_cap(nic_data->datapath_caps3,
> > > +                                                    CLIENT_CMD_VF_PROXY) &&
> > > +                                  efx->type->is_vf;
> > >          return 0;
> > >   }
> > > 
> > > @@ -806,13 +796,6 @@ static char *bar_config_name[] = {
> > >          [EF100_BAR_CONFIG_VDPA] = "vDPA",
> > >   };
> > > 
> > > -#ifdef CONFIG_SFC_VDPA
> > > -static bool efx_vdpa_supported(struct efx_nic *efx)
> > > -{
> > > -       return efx->type->is_vf;
> > > -}
> > > -#endif
> > > -
> > >   int efx_ef100_set_bar_config(struct efx_nic *efx,
> > >                               enum ef100_bar_config new_config)
> > >   {
> > > @@ -828,7 +811,7 @@ int efx_ef100_set_bar_config(struct efx_nic *efx,
> > > 
> > >   #ifdef CONFIG_SFC_VDPA
> > >          /* Current EF100 hardware supports vDPA on VFs only */
> > > -       if (new_config == EF100_BAR_CONFIG_VDPA && !efx_vdpa_supported(efx)) {
> > > +       if (new_config == EF100_BAR_CONFIG_VDPA && !nic_data->vdpa_supported) {
> > >                  pci_err(efx->pci_dev, "vdpa over PF not supported : %s",
> > >                          efx->name);
> > >                  return -EOPNOTSUPP;
> > > @@ -1208,6 +1191,12 @@ static int ef100_probe_main(struct efx_nic *efx)
> > >                  goto fail;
> > >          }
> > > 
> > > +       rc = efx_ef100_init_datapath_caps(efx);
> > > +       if (rc) {
> > > +               pci_info(efx->pci_dev, "Unable to initialize datapath caps\n");
> > > +               goto fail;
> > > +       }
> > > +
> > >          return 0;
> > >   fail:
> > >          return rc;
> > > diff --git a/drivers/net/ethernet/sfc/ef100_nic.h b/drivers/net/ethernet/sfc/ef100_nic.h
> > > index 4562982f2965..117a73d0795c 100644
> > > --- a/drivers/net/ethernet/sfc/ef100_nic.h
> > > +++ b/drivers/net/ethernet/sfc/ef100_nic.h
> > > @@ -76,6 +76,9 @@ struct ef100_nic_data {
> > >          u32 datapath_caps3;
> > >          unsigned int pf_index;
> > >          u16 warm_boot_count;
> > > +#ifdef CONFIG_SFC_VDPA
> > > +       bool vdpa_supported; /* true if vdpa is supported on this PCIe FN */
> > > +#endif
> > >          u8 port_id[ETH_ALEN];
> > >          DECLARE_BITMAP(evq_phases, EFX_MAX_CHANNELS);
> > >          enum ef100_bar_config bar_config;
> > > @@ -95,9 +98,8 @@ struct ef100_nic_data {
> > >   };
> > > 
> > >   #define efx_ef100_has_cap(caps, flag) \
> > > -       (!!((caps) & BIT_ULL(MC_CMD_GET_CAPABILITIES_V4_OUT_ ## flag ## _LBN)))
> > > +       (!!((caps) & BIT_ULL(MC_CMD_GET_CAPABILITIES_V7_OUT_ ## flag ## _LBN)))
> > > 
> > > -int efx_ef100_init_datapath_caps(struct efx_nic *efx);
> > >   int ef100_phy_probe(struct efx_nic *efx);
> > >   int ef100_filter_table_probe(struct efx_nic *efx);
> > > 
> > > diff --git a/drivers/net/ethernet/sfc/ef100_vdpa.h b/drivers/net/ethernet/sfc/ef100_vdpa.h
> > > index f6564448d0c7..90062fd8a25d 100644
> > > --- a/drivers/net/ethernet/sfc/ef100_vdpa.h
> > > +++ b/drivers/net/ethernet/sfc/ef100_vdpa.h
> > > @@ -1,7 +1,6 @@
> > >   /* SPDX-License-Identifier: GPL-2.0 */
> > > -/* Driver for Xilinx network controllers and boards
> > > - * Copyright (C) 2020-2022, Xilinx, Inc.
> > > - * Copyright (C) 2022, Advanced Micro Devices, Inc.
> > > +/* Driver for AMD network controllers and boards
> > > + * Copyright (C) 2023, Advanced Micro Devices, Inc.
> > Let's fix this in the patch that introduces this.
> 
> Sure, will fix.
> 
> Thanks
> 
> > 
> > Thanks
> > 
> > 
> > 
> > >    *
> > >    * This program is free software; you can redistribute it and/or modify it
> > >    * under the terms of the GNU General Public License version 2 as published
> > > --
> > > 2.30.1
> > >