Re: [dpdk-dev] [PATCH v3] net/axgbe: enabling VLAN support in axgbe

["Sardar, Shamsher singh" <Shamshersingh.Sardar@amd.com>]

[AMD Official Use Only - Internal Distribution Only]

Hi Ferruh,
Thanks for knowledge sharing.
Yes etlt - 0x09 is nothing but indicate " ■ 4’b1001: The packet is type packet with Single CVLAN tag."
And you are right it should be as below and will do changes on same:

if (vlan) {
        mbuf->ol_flags |= PKT_RX_VLAN;
        mbuf->vlan_tci = xxx
        if (vlan_stripped) {
                mbuf->ol_flags |= PKT_RX_VLAN_STRIPPED;
        }
}

But rest of the items will do as incremental development.
1. Like in axgbe_dev_configure() need to check for default setting when system comes up.
2. There are lot of changes need to be add for QinQ support.
Will add all as incremental development work.
Currently working to make other vendor's SFP to work and in parallel above changes will add for upstream.
Hope this should be fine.

Can you please put some more light on below, what type issues may occur?
" And I can see you may hit the problem becuase of VLAN offload but the issue seems generic, not directly related to VLAN support, and this can be separate fix I think."

Thanks & regards
S Shamsher Singh
(+91)7259016141

-----Original Message-----
From: Ferruh Yigit <ferruh.yigit@intel.com>
Sent: Friday, May 1, 2020 5:04 PM
To: Sardar, Shamsher singh <Shamshersingh.Sardar@amd.com>; dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v3] net/axgbe: enabling VLAN support in axgbe

[CAUTION: External Email]

On 4/30/2020 7:59 AM, ssardar@amd.com wrote:
> From: Sardar Shamsher Singh <Shamshersingh.Sardar@amd.com>
>
> adding below APIs for axgbe
> - axgbe_enable_rx_vlan_stripping: to enable vlan header stipping
> - axgbe_disable_rx_vlan_stripping: to disable vlan header stipping
> - axgbe_enable_rx_vlan_filtering: to enable vlan filter mode
> - axgbe_disable_rx_vlan_filtering: to disable vlan filter mode
> - axgbe_update_vlan_hash_table: crc calculation and hash table update
> based on vlan values post filter enable
> - axgbe_vlan_filter_set: setting of active vlan out of max 4K values
> before doing hash update of same
> - axgbe_vlan_tpid_set: setting of default tpid values
> - axgbe_vlan_offload_set: a top layer function to call stip/filter etc
> based on mask values
>
> Signed-off-by: Sardar Shamsher Singh <Shamshersingh.Sardar@amd.com>

<...>

> +static int
> +axgbe_vlan_offload_set(struct rte_eth_dev *dev, int mask) {
> +     struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
> +     struct axgbe_port *pdata = dev->data->dev_private;
> +
> +     /* Indicate that VLAN Tx CTAGs come from context descriptors */
> +     AXGMAC_IOWRITE_BITS(pdata, MAC_VLANIR, CSVL, 0);
> +     AXGMAC_IOWRITE_BITS(pdata, MAC_VLANIR, VLTI, 1);
> +
> +     if (mask & ETH_VLAN_STRIP_MASK) {
> +             if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_STRIP) {
> +                     PMD_DRV_LOG(DEBUG, "Strip ON for device = %s\n",
> +                                 pdata->eth_dev->device->name);
> +                     pdata->hw_if.enable_rx_vlan_stripping(pdata);
> +             } else {
> +                     PMD_DRV_LOG(DEBUG, "Strip OFF for device = %s\n",
> +                                 pdata->eth_dev->device->name);
> +                     pdata->hw_if.disable_rx_vlan_stripping(pdata);
> +             }
> +     }
> +     if (mask & ETH_VLAN_FILTER_MASK) {
> +             if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_FILTER) {
> +                     PMD_DRV_LOG(DEBUG, "Filter ON for device = %s\n",
> +                                 pdata->eth_dev->device->name);
> +                     pdata->hw_if.enable_rx_vlan_filtering(pdata);
> +             } else {
> +                     PMD_DRV_LOG(DEBUG, "Filter OFF for device = %s\n",
> +                                 pdata->eth_dev->device->name);
> +                     pdata->hw_if.disable_rx_vlan_filtering(pdata);
> +             }
> +     }
> +     if (mask & ETH_VLAN_EXTEND_MASK) {
> +             if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_EXTEND) {
> +                     PMD_DRV_LOG(DEBUG, "enabling vlan extended mode\n");
> +                     axgbe_vlan_extend_enable(pdata);
> +                     /* Set global registers with default ethertype*/
> +                     axgbe_vlan_tpid_set(dev, ETH_VLAN_TYPE_OUTER,
> +                                         RTE_ETHER_TYPE_VLAN);
> +                     axgbe_vlan_tpid_set(dev, ETH_VLAN_TYPE_INNER,
> +                                         RTE_ETHER_TYPE_VLAN);
> +             } else {
> +                     PMD_DRV_LOG(DEBUG, "disabling vlan extended mode\n");
> +                     axgbe_vlan_extend_disable(pdata);
> +             }
> +     }


Is the intention here to enable disable QinQ stip, because enable/disable fucntions talks about qinq, if so 'ETH_QINQ_STRIP_MASK' should be used.

<...>
> @@ -275,6 +275,23 @@ axgbe_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
>               /* Get the RSS hash */
>               if (AXGMAC_GET_BITS_LE(desc->write.desc3, RX_NORMAL_DESC3, RSV))
>                       mbuf->hash.rss =
> rte_le_to_cpu_32(desc->write.desc1);
> +             etlt = AXGMAC_GET_BITS_LE(desc->write.desc3,
> +                             RX_NORMAL_DESC3, ETLT);
> +             if (!err || !etlt) {
> +                     if (etlt == 0x09 &&
> +                     (rxq->pdata->eth_dev->data->dev_conf.rxmode.offloads &
> +                             DEV_RX_OFFLOAD_VLAN_STRIP)) {
> +                             mbuf->ol_flags |=
> +                                     PKT_RX_VLAN | PKT_RX_VLAN_STRIPPED;
> +                             mbuf->vlan_tci =
> +                                     AXGMAC_GET_BITS_LE(desc->write.desc0,
> +                                                     RX_NORMAL_DESC0,
> + OVT);

I don't know HW capabilities, but if 'etlt == 0x09' means packet has VLAN tag, you can use it independent from strip, like below, up to you.

if (vlan) {
        mbuf->ol_flags |= PKT_RX_VLAN;
        mbuf->vlan_tci = xxx
        if (vlan_stripped) {
                mbuf->ol_flags |= PKT_RX_VLAN_STRIPPED;
        }
}

<...>

> @@ -487,6 +520,7 @@ int axgbe_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
>       struct axgbe_tx_queue *txq;
>       unsigned int tsize;
>       const struct rte_memzone *tz;
> +     uint64_t offloads;
>
>       tx_desc = nb_desc;
>       pdata = dev->data->dev_private;
> @@ -506,7 +540,8 @@ int axgbe_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
>       if (!txq)
>               return -ENOMEM;
>       txq->pdata = pdata;
> -
> +     offloads = tx_conf->offloads |
> +             txq->pdata->eth_dev->data->dev_conf.txmode.offloads;

As far as I can see PMD doesn't support queue specific offloads, so 'tx_conf->offloads' should be always 0.

And since there is no queue specific offload and this 'offloads' information is only used to set burst function, which is again only port bases (this will keep overwrite same info per each queue), why not do this check in the 'axgbe_dev_configure()' instead of per queue?

And I can see you may hit the problem becuase of VLAN offload but the issue seems generic, not directly related to VLAN support, and this can be separate fix I think.