[RFC] ethdev: add generic MAC address rewrite actions

  There is a need to offload rewrite MAC address for both destination and source
from the matched flow

The proposed actions could make above easily achieved


Signed-off-by: Xiaoyu Min <jackmin@mellanox.com<mailto:jackmin@mellanox.com>>
---
lib/librte_ethdev/rte_flow.h | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)

--
2.17.1
  

On Tuesday, August 08/07/18, 2018 at 14:20:10 +0000, Jack Min wrote:
> There is a need to offload rewrite MAC address for both destination and source
> from the matched flow
> 
> The proposed actions could make above easily achieved
> 

+1.

We're also looking to offload these actions. In addition, we also have
a requirement to offload an action to swap the source and destination
MAC addresses (i.e. source MAC address will get overwritten with the
destination MAC address and vice-versa).

Could you please add one more action RTE_FLOW_ACTION_TYPE_MAC_SWAP
to achieve this? This action will not take any arguments. Let us
know your thoughts.

Thanks,
Rahul

> 
> Signed-off-by: Xiaoyu Min <jackmin@mellanox.com<mailto:jackmin@mellanox.com>>
> ---
> lib/librte_ethdev/rte_flow.h | 32 ++++++++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
> 
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index f8ba71cdb..4a51ab2a3 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -1505,6 +1505,26 @@ enum rte_flow_action_type {
>          * error.
>          */
>         RTE_FLOW_ACTION_TYPE_NVGRE_DECAP,
> +
> +       /**
> +        * Set source MAC address from matched flow.
> +        *
> +        * If flow pattern does not define a valid RTE_FLOW_ITEM_TYPE_ETH,
> +        * the PMD should return a RTE_FLOW_ERROR_TYPE_ACTION error.
> +        *
> +        * See struct rte_flow_action_set_mac.
> +        */
> +       RTE_FLOW_ACTION_TYPE_SET_MAC_SRC,
> +
> +       /**
> +        * Set destination MAC address from matched flow.
> +        *
> +        * If flow pattern does not define a valid RTE_FLOW_ITEM_TYPE_ETH,
> +        * the PMD should return a RTE_FLOW_ERROR_TYPE_ACTION error.
> +        *
> +        * See struct rte_flow_action_set_mac.
> +        */
> +       RTE_FLOW_ACTION_TYPE_SET_MAC_DST,
> };
> 
> /**
> @@ -1868,6 +1888,18 @@ struct rte_flow_action_nvgre_encap {
>         struct rte_flow_item *definition;
> };
> 
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice
> + *
> + * RTE_FLOW_ACTION_TYPE_SET_MAC
> + *
> + * Set MAC address from the matched flow
> + */
> +struct rte_flow_action_set_mac {
> +       uint8_t mac_addr[ETHER_ADDR_LEN];
> +};
> +
> /*
>   * Definition of a single action.
>   *
> --
> 2.17.1
  

On Mon, Aug 13, 2018 at 03:38:18PM +0530, Rahul Lakkireddy wrote:
> On Tuesday, August 08/07/18, 2018 at 14:20:10 +0000, Jack Min wrote:
> > There is a need to offload rewrite MAC address for both destination and source
> > from the matched flow
> > 
> > The proposed actions could make above easily achieved
> > 
> 
> +1.
> 
> We're also looking to offload these actions. In addition, we also have
> a requirement to offload an action to swap the source and destination
> MAC addresses (i.e. source MAC address will get overwritten with the
> destination MAC address and vice-versa).
> 
Hey,

Just want to know in which specific scenario we need to swap MAC address ?

> Could you please add one more action RTE_FLOW_ACTION_TYPE_MAC_SWAP
> to achieve this? This action will not take any arguments. Let us
> know your thoughts.
> 
> Thanks,
> Rahul
> 
> > 
> > Signed-off-by: Xiaoyu Min <jackmin@mellanox.com<mailto:jackmin@mellanox.com>>
> > ---
> > lib/librte_ethdev/rte_flow.h | 32 ++++++++++++++++++++++++++++++++
> > 1 file changed, 32 insertions(+)
> > 
> > diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> > index f8ba71cdb..4a51ab2a3 100644
> > --- a/lib/librte_ethdev/rte_flow.h
> > +++ b/lib/librte_ethdev/rte_flow.h
> > @@ -1505,6 +1505,26 @@ enum rte_flow_action_type {
> >          * error.
> >          */
> >         RTE_FLOW_ACTION_TYPE_NVGRE_DECAP,
> > +
> > +       /**
> > +        * Set source MAC address from matched flow.
> > +        *
> > +        * If flow pattern does not define a valid RTE_FLOW_ITEM_TYPE_ETH,
> > +        * the PMD should return a RTE_FLOW_ERROR_TYPE_ACTION error.
> > +        *
> > +        * See struct rte_flow_action_set_mac.
> > +        */
> > +       RTE_FLOW_ACTION_TYPE_SET_MAC_SRC,
> > +
> > +       /**
> > +        * Set destination MAC address from matched flow.
> > +        *
> > +        * If flow pattern does not define a valid RTE_FLOW_ITEM_TYPE_ETH,
> > +        * the PMD should return a RTE_FLOW_ERROR_TYPE_ACTION error.
> > +        *
> > +        * See struct rte_flow_action_set_mac.
> > +        */
> > +       RTE_FLOW_ACTION_TYPE_SET_MAC_DST,
> > };
> > 
> > /**
> > @@ -1868,6 +1888,18 @@ struct rte_flow_action_nvgre_encap {
> >         struct rte_flow_item *definition;
> > };
> > 
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this structure may change without prior notice
> > + *
> > + * RTE_FLOW_ACTION_TYPE_SET_MAC
> > + *
> > + * Set MAC address from the matched flow
> > + */
> > +struct rte_flow_action_set_mac {
> > +       uint8_t mac_addr[ETHER_ADDR_LEN];
> > +};
> > +
> > /*
> >   * Definition of a single action.
> >   *
> > --
> > 2.17.1
  

On Tuesday, August 08/14/18, 2018 at 14:45:45 +0530, Jack MIN wrote:
> On Mon, Aug 13, 2018 at 03:38:18PM +0530, Rahul Lakkireddy wrote:
> > On Tuesday, August 08/07/18, 2018 at 14:20:10 +0000, Jack Min wrote:
> > > There is a need to offload rewrite MAC address for both destination and source
> > > from the matched flow
> > > 
> > > The proposed actions could make above easily achieved
> > > 
> > 
> > +1.
> > 
> > We're also looking to offload these actions. In addition, we also have
> > a requirement to offload an action to swap the source and destination
> > MAC addresses (i.e. source MAC address will get overwritten with the
> > destination MAC address and vice-versa).
> > 
> Hey,
> 
> Just want to know in which specific scenario we need to swap MAC address ?
> 

Sorry for the delay in response.

This action is needed to offload loopback mode; where the hardware
will automatically swap the source and destination MAC before looping
back the packet.

Also, MAC swap can be used in conjunction with the other rewrite actions
to achieve MAC Layer Transparent NAT. When used in this way, the MAC
addresses will be swapped before the source or destination MAC address
is rewritten and NAT is performed.

Thanks,
Rahul

> > Could you please add one more action RTE_FLOW_ACTION_TYPE_MAC_SWAP
> > to achieve this? This action will not take any arguments. Let us
> > know your thoughts.
> > 
> > Thanks,
> > Rahul
> > 
> > > 
> > > Signed-off-by: Xiaoyu Min <jackmin@mellanox.com<mailto:jackmin@mellanox.com>>
> > > ---
> > > lib/librte_ethdev/rte_flow.h | 32 ++++++++++++++++++++++++++++++++
> > > 1 file changed, 32 insertions(+)
> > > 
> > > diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> > > index f8ba71cdb..4a51ab2a3 100644
> > > --- a/lib/librte_ethdev/rte_flow.h
> > > +++ b/lib/librte_ethdev/rte_flow.h
> > > @@ -1505,6 +1505,26 @@ enum rte_flow_action_type {
> > >          * error.
> > >          */
> > >         RTE_FLOW_ACTION_TYPE_NVGRE_DECAP,
> > > +
> > > +       /**
> > > +        * Set source MAC address from matched flow.
> > > +        *
> > > +        * If flow pattern does not define a valid RTE_FLOW_ITEM_TYPE_ETH,
> > > +        * the PMD should return a RTE_FLOW_ERROR_TYPE_ACTION error.
> > > +        *
> > > +        * See struct rte_flow_action_set_mac.
> > > +        */
> > > +       RTE_FLOW_ACTION_TYPE_SET_MAC_SRC,
> > > +
> > > +       /**
> > > +        * Set destination MAC address from matched flow.
> > > +        *
> > > +        * If flow pattern does not define a valid RTE_FLOW_ITEM_TYPE_ETH,
> > > +        * the PMD should return a RTE_FLOW_ERROR_TYPE_ACTION error.
> > > +        *
> > > +        * See struct rte_flow_action_set_mac.
> > > +        */
> > > +       RTE_FLOW_ACTION_TYPE_SET_MAC_DST,
> > > };
> > > 
> > > /**
> > > @@ -1868,6 +1888,18 @@ struct rte_flow_action_nvgre_encap {
> > >         struct rte_flow_item *definition;
> > > };
> > > 
> > > +/**
> > > + * @warning
> > > + * @b EXPERIMENTAL: this structure may change without prior notice
> > > + *
> > > + * RTE_FLOW_ACTION_TYPE_SET_MAC
> > > + *
> > > + * Set MAC address from the matched flow
> > > + */
> > > +struct rte_flow_action_set_mac {
> > > +       uint8_t mac_addr[ETHER_ADDR_LEN];
> > > +};
> > > +
> > > /*
> > >   * Definition of a single action.
> > >   *
> > > --
> > > 2.17.1
  

On Tue, Aug 21, 2018 at 01:44:32PM +0530, Rahul Lakkireddy wrote:
> On Tuesday, August 08/14/18, 2018 at 14:45:45 +0530, Jack MIN wrote:
> > On Mon, Aug 13, 2018 at 03:38:18PM +0530, Rahul Lakkireddy wrote:
> > > On Tuesday, August 08/07/18, 2018 at 14:20:10 +0000, Jack Min wrote:
> > > > There is a need to offload rewrite MAC address for both destination and source
> > > > from the matched flow
> > > > 
> > > > The proposed actions could make above easily achieved
> > > > 
> > > 
> > > +1.
> > > 
> > > We're also looking to offload these actions. In addition, we also have
> > > a requirement to offload an action to swap the source and destination
> > > MAC addresses (i.e. source MAC address will get overwritten with the
> > > destination MAC address and vice-versa).
> > > 
> > Hey,
> > 
> > Just want to know in which specific scenario we need to swap MAC address ?
> > 
> 
> Sorry for the delay in response.
> 
> This action is needed to offload loopback mode; where the hardware
> will automatically swap the source and destination MAC before looping
> back the packet.
> 
> Also, MAC swap can be used in conjunction with the other rewrite actions
> to achieve MAC Layer Transparent NAT. When used in this way, the MAC
> addresses will be swapped before the source or destination MAC address
> is rewritten and NAT is performed.
> 
Thank you, Rahul. 
It sounds reasonable to have RTE_FLOW_ACTION_TYPE_MAC_SWAP action.

Could you please make a new RFC for this ?

At this moment we haven't enough resource to implement it so a separated
RFC could be better to manage.

What do you think?

-Jack

> Thanks,
> Rahul
> 
> > > Could you please add one more action RTE_FLOW_ACTION_TYPE_MAC_SWAP
> > > to achieve this? This action will not take any arguments. Let us
> > > know your thoughts.
> > > 
> > > Thanks,
> > > Rahul
> > > 
> > > > 
> > > > Signed-off-by: Xiaoyu Min <jackmin@mellanox.com<mailto:jackmin@mellanox.com>>
> > > > ---
> > > > lib/librte_ethdev/rte_flow.h | 32 ++++++++++++++++++++++++++++++++
> > > > 1 file changed, 32 insertions(+)
> > > > 
> > > > diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> > > > index f8ba71cdb..4a51ab2a3 100644
> > > > --- a/lib/librte_ethdev/rte_flow.h
> > > > +++ b/lib/librte_ethdev/rte_flow.h
> > > > @@ -1505,6 +1505,26 @@ enum rte_flow_action_type {
> > > >          * error.
> > > >          */
> > > >         RTE_FLOW_ACTION_TYPE_NVGRE_DECAP,
> > > > +
> > > > +       /**
> > > > +        * Set source MAC address from matched flow.
> > > > +        *
> > > > +        * If flow pattern does not define a valid RTE_FLOW_ITEM_TYPE_ETH,
> > > > +        * the PMD should return a RTE_FLOW_ERROR_TYPE_ACTION error.
> > > > +        *
> > > > +        * See struct rte_flow_action_set_mac.
> > > > +        */
> > > > +       RTE_FLOW_ACTION_TYPE_SET_MAC_SRC,
> > > > +
> > > > +       /**
> > > > +        * Set destination MAC address from matched flow.
> > > > +        *
> > > > +        * If flow pattern does not define a valid RTE_FLOW_ITEM_TYPE_ETH,
> > > > +        * the PMD should return a RTE_FLOW_ERROR_TYPE_ACTION error.
> > > > +        *
> > > > +        * See struct rte_flow_action_set_mac.
> > > > +        */
> > > > +       RTE_FLOW_ACTION_TYPE_SET_MAC_DST,
> > > > };
> > > > 
> > > > /**
> > > > @@ -1868,6 +1888,18 @@ struct rte_flow_action_nvgre_encap {
> > > >         struct rte_flow_item *definition;
> > > > };
> > > > 
> > > > +/**
> > > > + * @warning
> > > > + * @b EXPERIMENTAL: this structure may change without prior notice
> > > > + *
> > > > + * RTE_FLOW_ACTION_TYPE_SET_MAC
> > > > + *
> > > > + * Set MAC address from the matched flow
> > > > + */
> > > > +struct rte_flow_action_set_mac {
> > > > +       uint8_t mac_addr[ETHER_ADDR_LEN];
> > > > +};
> > > > +
> > > > /*
> > > >   * Definition of a single action.
> > > >   *
> > > > --
> > > > 2.17.1
  

On Thursday, August 08/23/18, 2018 at 07:45:27 +0530, Jack MIN wrote:
> On Tue, Aug 21, 2018 at 01:44:32PM +0530, Rahul Lakkireddy wrote:
> > On Tuesday, August 08/14/18, 2018 at 14:45:45 +0530, Jack MIN wrote:
> > > On Mon, Aug 13, 2018 at 03:38:18PM +0530, Rahul Lakkireddy wrote:
> > > > On Tuesday, August 08/07/18, 2018 at 14:20:10 +0000, Jack Min wrote:
> > > > > There is a need to offload rewrite MAC address for both destination and source
> > > > > from the matched flow
> > > > > 
> > > > > The proposed actions could make above easily achieved
> > > > > 
> > > > 
> > > > +1.
> > > > 
> > > > We're also looking to offload these actions. In addition, we also have
> > > > a requirement to offload an action to swap the source and destination
> > > > MAC addresses (i.e. source MAC address will get overwritten with the
> > > > destination MAC address and vice-versa).
> > > > 
> > > Hey,
> > > 
> > > Just want to know in which specific scenario we need to swap MAC address ?
> > > 
> > 
> > Sorry for the delay in response.
> > 
> > This action is needed to offload loopback mode; where the hardware
> > will automatically swap the source and destination MAC before looping
> > back the packet.
> > 
> > Also, MAC swap can be used in conjunction with the other rewrite actions
> > to achieve MAC Layer Transparent NAT. When used in this way, the MAC
> > addresses will be swapped before the source or destination MAC address
> > is rewritten and NAT is performed.
> > 
> Thank you, Rahul. 
> It sounds reasonable to have RTE_FLOW_ACTION_TYPE_MAC_SWAP action.
> 
> Could you please make a new RFC for this ?
> 
> At this moment we haven't enough resource to implement it so a separated
> RFC could be better to manage.
> 
> What do you think?

Sure, no problem. Will send RTE_FLOW_ACTION_TYPE_MAC_SWAP as a separate
RFC.

> 
> -Jack
> 
> > Thanks,
> > Rahul
> > 
> > > > Could you please add one more action RTE_FLOW_ACTION_TYPE_MAC_SWAP
> > > > to achieve this? This action will not take any arguments. Let us
> > > > know your thoughts.
> > > > 
> > > > Thanks,
> > > > Rahul
> > > > 
> > > > > 
> > > > > Signed-off-by: Xiaoyu Min <jackmin@mellanox.com<mailto:jackmin@mellanox.com>>
> > > > > ---
> > > > > lib/librte_ethdev/rte_flow.h | 32 ++++++++++++++++++++++++++++++++
> > > > > 1 file changed, 32 insertions(+)
> > > > > 
> > > > > diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> > > > > index f8ba71cdb..4a51ab2a3 100644
> > > > > --- a/lib/librte_ethdev/rte_flow.h
> > > > > +++ b/lib/librte_ethdev/rte_flow.h
> > > > > @@ -1505,6 +1505,26 @@ enum rte_flow_action_type {
> > > > >          * error.
> > > > >          */
> > > > >         RTE_FLOW_ACTION_TYPE_NVGRE_DECAP,
> > > > > +
> > > > > +       /**
> > > > > +        * Set source MAC address from matched flow.
> > > > > +        *
> > > > > +        * If flow pattern does not define a valid RTE_FLOW_ITEM_TYPE_ETH,
> > > > > +        * the PMD should return a RTE_FLOW_ERROR_TYPE_ACTION error.
> > > > > +        *
> > > > > +        * See struct rte_flow_action_set_mac.
> > > > > +        */
> > > > > +       RTE_FLOW_ACTION_TYPE_SET_MAC_SRC,
> > > > > +
> > > > > +       /**
> > > > > +        * Set destination MAC address from matched flow.
> > > > > +        *
> > > > > +        * If flow pattern does not define a valid RTE_FLOW_ITEM_TYPE_ETH,
> > > > > +        * the PMD should return a RTE_FLOW_ERROR_TYPE_ACTION error.
> > > > > +        *
> > > > > +        * See struct rte_flow_action_set_mac.
> > > > > +        */
> > > > > +       RTE_FLOW_ACTION_TYPE_SET_MAC_DST,
> > > > > };
> > > > > 
> > > > > /**
> > > > > @@ -1868,6 +1888,18 @@ struct rte_flow_action_nvgre_encap {
> > > > >         struct rte_flow_item *definition;
> > > > > };
> > > > > 
> > > > > +/**
> > > > > + * @warning
> > > > > + * @b EXPERIMENTAL: this structure may change without prior notice
> > > > > + *
> > > > > + * RTE_FLOW_ACTION_TYPE_SET_MAC
> > > > > + *
> > > > > + * Set MAC address from the matched flow
> > > > > + */
> > > > > +struct rte_flow_action_set_mac {
> > > > > +       uint8_t mac_addr[ETHER_ADDR_LEN];
> > > > > +};
> > > > > +
> > > > > /*
> > > > >   * Definition of a single action.
> > > > >   *
> > > > > --
> > > > > 2.17.1
  

On 8/7/2018 3:20 PM, Jack Min wrote:
> There is a need to offload rewrite MAC address for both destination and source
> from the matched flow
> 
> The proposed actions could make above easily achieved
> 
> 
> Signed-off-by: Xiaoyu Min <jackmin@mellanox.com<mailto:jackmin@mellanox.com>>

Looks good to me.

@Adrien, @Andrew, @Nelio, any comment on patch?

Thanks,
ferruh
  

On 08/07/2018 05:20 PM, Jack Min wrote:
> There is a need to offload rewrite MAC address for both destination and source
> from the matched flow
>
> The proposed actions could make above easily achieved
>
>
> Signed-off-by: Xiaoyu Min <jackmin@mellanox.com<mailto:jackmin@mellanox.com>>

The only question I have is which Ethernet header is edited, if there 
are many
Ethernet headers in the case of tunnels. I guess the outermost, but it 
should
be stated in the description.
  

On Tue, Aug 28, 2018 at 01:15:09PM +0300, Andrew Rybchenko wrote:
> On 08/07/2018 05:20 PM, Jack Min wrote:
> > There is a need to offload rewrite MAC address for both destination and source
> > from the matched flow
> > 
> > The proposed actions could make above easily achieved
> > 
> > 
> > Signed-off-by: Xiaoyu Min <jackmin@mellanox.com<mailto:jackmin@mellanox.com>>
> 
> The only question I have is which Ethernet header is edited, if there are
> many
> Ethernet headers in the case of tunnels. I guess the outermost, but it
> should
> be stated in the description.
> 
OK. I'll add it in V2.

Thanks
-Jack