Re: [PATCH net-next 07/11] sfc: implement filters for receiving traffic

[Gautam Dawar <gdawar@amd.com>]

On 12/14/22 12:15, Jason Wang wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Wed, Dec 7, 2022 at 10:57 PM Gautam Dawar <gautam.dawar@amd.com> wrote:
>> Implement unicast, broadcast and unknown multicast
>> filters for receiving different types of traffic.
> I suggest squashing this into the patch that implements set_config().

The patch that implements set_config() also implements device level vdpa 
config operations. I think that squashing the filtering support (this 
patch) in to that patch will make it unnecessary long and combine lot of 
unrelated functionality (except the call to configure filters in 
set_config operation).

>
>> Signed-off-by: Gautam Dawar <gautam.dawar@amd.com>
>> ---
>>   drivers/net/ethernet/sfc/ef100_vdpa.c     | 159 ++++++++++++++++++++++
>>   drivers/net/ethernet/sfc/ef100_vdpa.h     |  35 +++++
>>   drivers/net/ethernet/sfc/ef100_vdpa_ops.c |  27 +++-
>>   3 files changed, 220 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/sfc/ef100_vdpa.c b/drivers/net/ethernet/sfc/ef100_vdpa.c
>> index 41eb7aef6798..04d64bfe3c93 100644
>> --- a/drivers/net/ethernet/sfc/ef100_vdpa.c
>> +++ b/drivers/net/ethernet/sfc/ef100_vdpa.c
>> @@ -17,12 +17,168 @@
>>   #include "mcdi_filters.h"
>>   #include "mcdi_functions.h"
>>   #include "ef100_netdev.h"
>> +#include "filter.h"
>> +#include "efx.h"
>>
>> +#define EFX_INVALID_FILTER_ID -1
>> +
>> +/* vDPA queues starts from 2nd VI or qid 1 */
>> +#define EF100_VDPA_BASE_RX_QID 1
>> +
>> +static const char * const filter_names[] = { "bcast", "ucast", "mcast" };
>>   static struct virtio_device_id ef100_vdpa_id_table[] = {
>>          { .device = VIRTIO_ID_NET, .vendor = PCI_VENDOR_ID_REDHAT_QUMRANET },
>>          { 0 },
>>   };
>>
>> +static int ef100_vdpa_set_mac_filter(struct efx_nic *efx,
>> +                                    struct efx_filter_spec *spec,
>> +                                    u32 qid,
>> +                                    u8 *mac_addr)
>> +{
>> +       int rc;
>> +
>> +       efx_filter_init_rx(spec, EFX_FILTER_PRI_AUTO, 0, qid);
>> +
>> +       if (mac_addr) {
>> +               rc = efx_filter_set_eth_local(spec, EFX_FILTER_VID_UNSPEC,
>> +                                             mac_addr);
>> +               if (rc)
>> +                       pci_err(efx->pci_dev,
>> +                               "Filter set eth local failed, err: %d\n", rc);
>> +       } else {
>> +               efx_filter_set_mc_def(spec);
>> +       }
>> +
>> +       rc = efx_filter_insert_filter(efx, spec, true);
>> +       if (rc < 0)
>> +               pci_err(efx->pci_dev,
>> +                       "Filter insert failed, err: %d\n", rc);
>> +
>> +       return rc;
>> +}
>> +
>> +static int ef100_vdpa_delete_filter(struct ef100_vdpa_nic *vdpa_nic,
>> +                                   enum ef100_vdpa_mac_filter_type type)
>> +{
>> +       struct vdpa_device *vdev = &vdpa_nic->vdpa_dev;
>> +       int rc;
>> +
>> +       if (vdpa_nic->filters[type].filter_id == EFX_INVALID_FILTER_ID)
>> +               return rc;
>> +
>> +       rc = efx_filter_remove_id_safe(vdpa_nic->efx,
>> +                                      EFX_FILTER_PRI_AUTO,
>> +                                      vdpa_nic->filters[type].filter_id);
>> +       if (rc) {
>> +               dev_err(&vdev->dev, "%s filter id: %d remove failed, err: %d\n",
>> +                       filter_names[type], vdpa_nic->filters[type].filter_id,
>> +                       rc);
>> +       } else {
>> +               vdpa_nic->filters[type].filter_id = EFX_INVALID_FILTER_ID;
>> +               vdpa_nic->filter_cnt--;
>> +       }
>> +       return rc;
>> +}
>> +
>> +int ef100_vdpa_add_filter(struct ef100_vdpa_nic *vdpa_nic,
>> +                         enum ef100_vdpa_mac_filter_type type)
>> +{
>> +       struct vdpa_device *vdev = &vdpa_nic->vdpa_dev;
>> +       struct efx_nic *efx = vdpa_nic->efx;
>> +       /* Configure filter on base Rx queue only */
>> +       u32 qid = EF100_VDPA_BASE_RX_QID;
>> +       struct efx_filter_spec *spec;
>> +       u8 baddr[ETH_ALEN];
>> +       int rc;
>> +
>> +       /* remove existing filter */
>> +       rc = ef100_vdpa_delete_filter(vdpa_nic, type);
>> +       if (rc < 0) {
>> +               dev_err(&vdev->dev, "%s MAC filter deletion failed, err: %d",
>> +                       filter_names[type], rc);
>> +               return rc;
>> +       }
>> +
>> +       /* Configure MAC Filter */
>> +       spec = &vdpa_nic->filters[type].spec;
>> +       if (type == EF100_VDPA_BCAST_MAC_FILTER) {
>> +               eth_broadcast_addr(baddr);
>> +               rc = ef100_vdpa_set_mac_filter(efx, spec, qid, baddr);
>> +       } else if (type == EF100_VDPA_UNKNOWN_MCAST_MAC_FILTER) {
>> +               rc = ef100_vdpa_set_mac_filter(efx, spec, qid, NULL);
>> +       } else {
>> +               /* Ensure we have everything required to insert the filter */
>> +               if (!vdpa_nic->mac_configured ||
>> +                   !vdpa_nic->vring[0].vring_created ||
>> +                   !is_valid_ether_addr(vdpa_nic->mac_address))
>> +                       return -EINVAL;
>> +
>> +               rc = ef100_vdpa_set_mac_filter(efx, spec, qid,
>> +                                              vdpa_nic->mac_address);
>> +       }
>> +
>> +       if (rc >= 0) {
>> +               vdpa_nic->filters[type].filter_id = rc;
>> +               vdpa_nic->filter_cnt++;
> I'm not sure I get this, but the code seems doesn't allow the driver
> to set multiple uc filters, so I think filter_cnt should be always 3?
> If yes, I don't see how filter_cnt can help here.
The filter_cnt is supposed to maintain the total number of filters 
created on a vdpa device. For now, this count will always be 3 and may 
not be of much use but later it will include the count of multiple 
multicast and unicast filters.
>
>> +
>> +               return 0;
>> +       }
>> +
>> +       dev_err(&vdev->dev, "%s MAC filter insert failed, err: %d\n",
>> +               filter_names[type], rc);
>> +
>> +       if (type != EF100_VDPA_UNKNOWN_MCAST_MAC_FILTER) {
>> +               ef100_vdpa_filter_remove(vdpa_nic);
>> +               return rc;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +int ef100_vdpa_filter_remove(struct ef100_vdpa_nic *vdpa_nic)
>> +{
>> +       enum ef100_vdpa_mac_filter_type filter;
>> +       int err = 0;
>> +       int rc;
>> +
>> +       for (filter = EF100_VDPA_BCAST_MAC_FILTER;
>> +            filter <= EF100_VDPA_UNKNOWN_MCAST_MAC_FILTER; filter++) {
>> +               rc = ef100_vdpa_delete_filter(vdpa_nic, filter);
>> +               if (rc < 0)
>> +                       /* store status of last failed filter remove */
>> +                       err = rc;
>> +       }
>> +       return err;
>> +}
>> +
>> +int ef100_vdpa_filter_configure(struct ef100_vdpa_nic *vdpa_nic)
>> +{
>> +       struct vdpa_device *vdev = &vdpa_nic->vdpa_dev;
>> +       enum ef100_vdpa_mac_filter_type filter;
>> +       int rc;
>> +
>> +       /* remove existing filters, if any */
>> +       rc = ef100_vdpa_filter_remove(vdpa_nic);
>> +       if (rc < 0) {
>> +               dev_err(&vdev->dev,
>> +                       "MAC filter deletion failed, err: %d", rc);
>> +               goto fail;
>> +       }
>> +
>> +       for (filter = EF100_VDPA_BCAST_MAC_FILTER;
>> +            filter <= EF100_VDPA_UNKNOWN_MCAST_MAC_FILTER; filter++) {
>> +               if (filter == EF100_VDPA_UCAST_MAC_FILTER &&
>> +                   !vdpa_nic->mac_configured)
>> +                       continue;
>> +               rc = ef100_vdpa_add_filter(vdpa_nic, filter);
>> +               if (rc < 0)
>> +                       goto fail;
>> +       }
>> +fail:
>> +       return rc;
>> +}
>> +
>>   int ef100_vdpa_init(struct efx_probe_data *probe_data)
>>   {
>>          struct efx_nic *efx = &probe_data->efx;
>> @@ -168,6 +324,9 @@ static struct ef100_vdpa_nic *ef100_vdpa_create(struct efx_nic *efx,
>>          ether_addr_copy(vdpa_nic->mac_address, mac);
>>          vdpa_nic->mac_configured = true;
>>
>> +       for (i = 0; i < EF100_VDPA_MAC_FILTER_NTYPES; i++)
>> +               vdpa_nic->filters[i].filter_id = EFX_INVALID_FILTER_ID;
>> +
>>          for (i = 0; i < (2 * vdpa_nic->max_queue_pairs); i++)
>>                  vdpa_nic->vring[i].irq = -EINVAL;
>>
>> diff --git a/drivers/net/ethernet/sfc/ef100_vdpa.h b/drivers/net/ethernet/sfc/ef100_vdpa.h
>> index 3cc33daa0431..a33edd6dda12 100644
>> --- a/drivers/net/ethernet/sfc/ef100_vdpa.h
>> +++ b/drivers/net/ethernet/sfc/ef100_vdpa.h
>> @@ -72,6 +72,22 @@ enum ef100_vdpa_vq_type {
>>          EF100_VDPA_VQ_NTYPES
>>   };
>>
>> +/**
>> + * enum ef100_vdpa_mac_filter_type - vdpa filter types
>> + *
>> + * @EF100_VDPA_BCAST_MAC_FILTER: Broadcast MAC filter
>> + * @EF100_VDPA_UCAST_MAC_FILTER: Unicast MAC filter
>> + * @EF100_VDPA_UNKNOWN_MCAST_MAC_FILTER: Unknown multicast MAC filter to allow
>> + *     IPv6 Neighbor Solicitation Message
>> + * @EF100_VDPA_MAC_FILTER_NTYPES: Number of vDPA filter types
>> + */
>> +enum ef100_vdpa_mac_filter_type {
>> +       EF100_VDPA_BCAST_MAC_FILTER,
>> +       EF100_VDPA_UCAST_MAC_FILTER,
>> +       EF100_VDPA_UNKNOWN_MCAST_MAC_FILTER,
>> +       EF100_VDPA_MAC_FILTER_NTYPES,
>> +};
>> +
>>   /**
>>    * struct ef100_vdpa_vring_info - vDPA vring data structure
>>    *
>> @@ -109,6 +125,17 @@ struct ef100_vdpa_vring_info {
>>          struct vdpa_callback cb;
>>   };
>>
>> +/**
>> + * struct ef100_vdpa_filter - vDPA filter data structure
>> + *
>> + * @filter_id: filter id of this filter
>> + * @efx_filter_spec: hardware filter specs for this vdpa device
>> + */
>> +struct ef100_vdpa_filter {
>> +       s32 filter_id;
>> +       struct efx_filter_spec spec;
>> +};
>> +
>>   /**
>>    *  struct ef100_vdpa_nic - vDPA NIC data structure
>>    *
>> @@ -118,6 +145,7 @@ struct ef100_vdpa_vring_info {
>>    * @lock: Managing access to vdpa config operations
>>    * @pf_index: PF index of the vDPA VF
>>    * @vf_index: VF index of the vDPA VF
>> + * @filter_cnt: total number of filters created on this vdpa device
>>    * @status: device status as per VIRTIO spec
>>    * @features: negotiated feature bits
>>    * @max_queue_pairs: maximum number of queue pairs supported
>> @@ -125,6 +153,7 @@ struct ef100_vdpa_vring_info {
>>    * @vring: vring information of the vDPA device.
>>    * @mac_address: mac address of interface associated with this vdpa device
>>    * @mac_configured: true after MAC address is configured
>> + * @filters: details of all filters created on this vdpa device
>>    * @cfg_cb: callback for config change
>>    */
>>   struct ef100_vdpa_nic {
>> @@ -135,6 +164,7 @@ struct ef100_vdpa_nic {
>>          struct mutex lock;
>>          u32 pf_index;
>>          u32 vf_index;
>> +       u32 filter_cnt;
>>          u8 status;
>>          u64 features;
>>          u32 max_queue_pairs;
>> @@ -142,6 +172,7 @@ struct ef100_vdpa_nic {
>>          struct ef100_vdpa_vring_info vring[EF100_VDPA_MAX_QUEUES_PAIRS * 2];
>>          u8 *mac_address;
>>          bool mac_configured;
>> +       struct ef100_vdpa_filter filters[EF100_VDPA_MAC_FILTER_NTYPES];
>>          struct vdpa_callback cfg_cb;
>>   };
>>
>> @@ -149,6 +180,10 @@ int ef100_vdpa_init(struct efx_probe_data *probe_data);
>>   void ef100_vdpa_fini(struct efx_probe_data *probe_data);
>>   int ef100_vdpa_register_mgmtdev(struct efx_nic *efx);
>>   void ef100_vdpa_unregister_mgmtdev(struct efx_nic *efx);
>> +int ef100_vdpa_filter_configure(struct ef100_vdpa_nic *vdpa_nic);
>> +int ef100_vdpa_filter_remove(struct ef100_vdpa_nic *vdpa_nic);
>> +int ef100_vdpa_add_filter(struct ef100_vdpa_nic *vdpa_nic,
>> +                         enum ef100_vdpa_mac_filter_type type);
>>   int ef100_vdpa_irq_vectors_alloc(struct pci_dev *pci_dev, u16 nvqs);
>>   void ef100_vdpa_irq_vectors_free(void *data);
>>
>> diff --git a/drivers/net/ethernet/sfc/ef100_vdpa_ops.c b/drivers/net/ethernet/sfc/ef100_vdpa_ops.c
>> index b7efd3e0c901..132ddb4a647b 100644
>> --- a/drivers/net/ethernet/sfc/ef100_vdpa_ops.c
>> +++ b/drivers/net/ethernet/sfc/ef100_vdpa_ops.c
>> @@ -135,6 +135,15 @@ static int delete_vring(struct ef100_vdpa_nic *vdpa_nic, u16 idx)
>>          if (vdpa_nic->vring[idx].vring_ctx)
>>                  delete_vring_ctx(vdpa_nic, idx);
>>
>> +       if (idx == 0 && vdpa_nic->filter_cnt != 0) {
>> +               rc = ef100_vdpa_filter_remove(vdpa_nic);
>> +               if (rc < 0) {
>> +                       dev_err(&vdpa_nic->vdpa_dev.dev,
>> +                               "%s: vdpa remove filter failed, err:%d\n",
>> +                               __func__, rc);
>> +               }
>> +       }
>> +
>>          return rc;
>>   }
>>
>> @@ -193,8 +202,22 @@ static int create_vring(struct ef100_vdpa_nic *vdpa_nic, u16 idx)
>>                  vdpa_nic->vring[idx].doorbell_offset_valid = true;
>>          }
>>
>> +       /* Configure filters on rxq 0 */
>> +       if (idx == 0) {
> This seems tricky, can we move this to set_status() when DRIVER_OK is set?
For a virtqueue to be created, qenable must be set to 1 and status 
should be set to DRIVER_OK. The current implementation ensures both 
conditions before creating the HW queue. This will handle the situation 
where DRIVER_OK is set by a bad guest virtio-net driver but queue has 
not been enabled.
>
> Thanks
>
>
>> +               rc = ef100_vdpa_filter_configure(vdpa_nic);
>> +               if (rc < 0) {
>> +                       dev_err(&vdpa_nic->vdpa_dev.dev,
>> +                               "%s: vdpa configure filter failed, err:%d\n",
>> +                               __func__, rc);
>> +                       goto err_filter_configure;
>> +               }
>> +       }
>> +
>>          return 0;
>>
>> +err_filter_configure:
>> +       ef100_vdpa_filter_remove(vdpa_nic);
>> +       vdpa_nic->vring[idx].doorbell_offset_valid = false;
>>   err_get_doorbell_offset:
>>          efx_vdpa_vring_destroy(vdpa_nic->vring[idx].vring_ctx,
>>                                 &vring_dyn_cfg);
>> @@ -578,8 +601,10 @@ static void ef100_vdpa_set_config(struct vdpa_device *vdev, unsigned int offset,
>>          }
>>
>>          memcpy((u8 *)&vdpa_nic->net_config + offset, buf, len);
>> -       if (is_valid_ether_addr(vdpa_nic->mac_address))
>> +       if (is_valid_ether_addr(vdpa_nic->mac_address)) {
>>                  vdpa_nic->mac_configured = true;
>> +               ef100_vdpa_add_filter(vdpa_nic, EF100_VDPA_UCAST_MAC_FILTER);
>> +       }
>>   }
>>
>>   static void ef100_vdpa_free(struct vdpa_device *vdev)
>> --
>> 2.30.1
>>